Message ID | 201912080836.xB88amHd015549@sdf.org |
---|---|
State | RFC |
Delegated to: | David Miller |
Headers | show |
Series | [RFC,1/4] b44: Add DMA_ATTR_NO_WARN to dma_map_single attempts | expand |
On Sun, Dec 8, 2019 at 12:36 AM George Spelvin <lkml@sdf.org> wrote: > > The requirement is dma_addr + size <= 0x40000000, not 0x3fffffff. > > In a logically separate but overlapping change, this patch also > rearranges the logic for detecting failures to use a goto rather > than testing dma_mapping_error() twice. The latter is expensive if > CONFIG_DMA_API_DEBUG is set, but also for bug-proofing reasons I try to > avoid having the same condition in two places that must be kept in sync. > > Signed-off-by: George Spelvin <lkml@sdf.org> > --- > drivers/net/ethernet/broadcom/b44.c | 42 ++++++++++++++++------------- > 1 file changed, 24 insertions(+), 18 deletions(-) > > diff --git a/drivers/net/ethernet/broadcom/b44.c b/drivers/net/ethernet/broadcom/b44.c > index 394671230c1c..e540d5646aef 100644 > --- a/drivers/net/ethernet/broadcom/b44.c > +++ b/drivers/net/ethernet/broadcom/b44.c > @@ -680,12 +680,13 @@ static int b44_alloc_rx_skb(struct b44 *bp, int src_idx, u32 dest_idx_unmasked) > > /* Hardware bug work-around, the chip is unable to do PCI DMA > to/from anything above 1GB :-( */ > - if (dma_mapping_error(bp->sdev->dma_dev, mapping) || > - mapping + RX_PKT_BUF_SZ > DMA_BIT_MASK(30)) { > + if (dma_mapping_error(bp->sdev->dma_dev, mapping)) > + goto workaround; > + if (mapping + RX_PKT_BUF_SZ > DMA_BIT_MASK(30)+1) { The patchset looks ok to me. The only minor suggestion is to define this (DMA_BIT_MASK(30) + 1) as B44_DMA_ADDR_LIMIT or something like that so you don't have to repeat it so many times. Thanks.
From michael.chan@broadcom.com Mon Dec 9 17:30:02 2019 DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=broadcom.com; s=google; h=mime-version:references:in-reply-to:from:date:message-id:subject:to :cc; bh=Dcgdz44q2GCvADR7a/LkLs9ynpEQVQ92g/NDbUJ9ByU=; b=PKOR9wiZIB18b/51jkUG/wXIFefbTZbsIoGHXMC3Wa+HrxHrvmOkKTzDzw+sXBVIlk WIeKjcijz8em6sM32WwxvXsG6eJYecW1+Vr8KQUTsTvwAezJa3uxBtNCwH0rvj3Hax2z 2OapWK7fvk/DbmGvycFL+e8p61RygSe62KIF8= X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20161025; h=x-gm-message-state:mime-version:references:in-reply-to:from:date :message-id:subject:to:cc; bh=Dcgdz44q2GCvADR7a/LkLs9ynpEQVQ92g/NDbUJ9ByU=; b=IFhkqkqmkV1immJLIPu0fZ4IWzVHMsSFers4GqLqTxLaiKuXyQfvRhQSRM37NYwaON IE/0JjtXxZyECNyF1M2sFl/VVo4nVQG28mHeuJK1GG8WtToN+bF40KU+3eGhDWFgJYGK XfZjSIrHORSGYlKRWdpfv+jJizDt0/zB1RvMipk9BSW3mp5MLNHB8gnWmqFpptDdkqYX 4R906dAil3MORLKrH33jpnHlWdZeTPc++n4+b+YhAggBzLCifmfcNP5Gsi9MVV6+hzot 8RxP7p+m1839v84KhyhUAeuJ64QLKaQA9jAMF9IOU/N1DJCFLFE6rPmh0JM5jkl82cDp 4qew== X-Gm-Message-State: APjAAAWuTa/gVp+rOdJT+15VVbVrsYIOYBlX4a4u8GUmZNE3mGqVmIfL 8BYFLElwhyjGzOgl3smgAsybRD66b+oZQTZLOuyTSOxH X-Google-Smtp-Source: APXvYqz4AY07BzHgic5er69wv22sNZkWuNoimJ4dj0xiy8faF2rZTCRbn2c2dvhbKtnKoCqZ27uqaKVyNe2AUdEl/0o= X-Received: by 2002:a05:6830:1e75:: with SMTP id m21mr21646882otr.36.1575912597048; Mon, 09 Dec 2019 09:29:57 -0800 (PST) MIME-Version: 1.0 References: <201912080836.xB88amHd015549@sdf.org> In-Reply-To: <201912080836.xB88amHd015549@sdf.org> From: Michael Chan <michael.chan@broadcom.com> Date: Mon, 9 Dec 2019 09:29:46 -0800 Subject: Re: [RFC PATCH 2/4] b44: Fix off-by-one error in acceptable address range To: George Spelvin <lkml@sdf.org> Cc: Netdev <netdev@vger.kernel.org>, Hauke Mehrtens <hauke@hauke-m.de> Content-Type: text/plain; charset="UTF-8" On Mon, 9 Dec 2019 at 09:29:46 Michael Chan wrote: > The patchset looks ok to me. The only minor suggestion is to define > this (DMA_BIT_MASK(30) + 1) as B44_DMA_ADDR_LIMIT or something like > that so you don't have to repeat it so many times. Thank you for the prompt reply. Yes, that's a sensible thing to do. There's an existing constant I can use: SSB_PCI_DMA_SZ. Do you have any idea about the WARN_ON_ONCE in report_addr()? That's a someone more invasive issue. E.g. should it be changed to also honour the NO_WARN flag? Looking at the 440x datasheet, one significantly more ambitious thing I wonder about is if it would be possible to map the swiotlb using SBToPCITranslation[01] and let that layer do the heavy lifting. The translation registers are set up, but I don't see them ever actually used for anything on these chips. (But I havnn't grokked all the drivers/ssb code yet, so I might have missed something.)
diff --git a/drivers/net/ethernet/broadcom/b44.c b/drivers/net/ethernet/broadcom/b44.c index 394671230c1c..e540d5646aef 100644 --- a/drivers/net/ethernet/broadcom/b44.c +++ b/drivers/net/ethernet/broadcom/b44.c @@ -680,12 +680,13 @@ static int b44_alloc_rx_skb(struct b44 *bp, int src_idx, u32 dest_idx_unmasked) /* Hardware bug work-around, the chip is unable to do PCI DMA to/from anything above 1GB :-( */ - if (dma_mapping_error(bp->sdev->dma_dev, mapping) || - mapping + RX_PKT_BUF_SZ > DMA_BIT_MASK(30)) { + if (dma_mapping_error(bp->sdev->dma_dev, mapping)) + goto workaround; + if (mapping + RX_PKT_BUF_SZ > DMA_BIT_MASK(30)+1) { /* Sigh... */ - if (!dma_mapping_error(bp->sdev->dma_dev, mapping)) - dma_unmap_single(bp->sdev->dma_dev, mapping, - RX_PKT_BUF_SZ, DMA_FROM_DEVICE); + dma_unmap_single(bp->sdev->dma_dev, mapping, + RX_PKT_BUF_SZ, DMA_FROM_DEVICE); +workaround: dev_kfree_skb_any(skb); skb = alloc_skb(RX_PKT_BUF_SZ, GFP_ATOMIC | GFP_DMA); if (skb == NULL) @@ -693,10 +694,12 @@ static int b44_alloc_rx_skb(struct b44 *bp, int src_idx, u32 dest_idx_unmasked) mapping = dma_map_single(bp->sdev->dma_dev, skb->data, RX_PKT_BUF_SZ, DMA_FROM_DEVICE); - if (dma_mapping_error(bp->sdev->dma_dev, mapping) || - mapping + RX_PKT_BUF_SZ > DMA_BIT_MASK(30)) { - if (!dma_mapping_error(bp->sdev->dma_dev, mapping)) - dma_unmap_single(bp->sdev->dma_dev, mapping, RX_PKT_BUF_SZ,DMA_FROM_DEVICE); + if (dma_mapping_error(bp->sdev->dma_dev, mapping)) + goto failed; + if (mapping + RX_PKT_BUF_SZ > DMA_BIT_MASK(30)+1) { + dma_unmap_single(bp->sdev->dma_dev, mapping, + RX_PKT_BUF_SZ, DMA_FROM_DEVICE); +failed: dev_kfree_skb_any(skb); return -ENOMEM; } @@ -990,24 +993,27 @@ static netdev_tx_t b44_start_xmit(struct sk_buff *skb, struct net_device *dev) mapping = dma_map_single_attrs(bp->sdev->dma_dev, skb->data, len, DMA_TO_DEVICE, DMA_ATTR_NO_WARN); - if (dma_mapping_error(bp->sdev->dma_dev, mapping) || mapping + len > DMA_BIT_MASK(30)) { + if (dma_mapping_error(bp->sdev->dma_dev, mapping)) + goto workaround; + if (mapping + len > DMA_BIT_MASK(30)+1) { struct sk_buff *bounce_skb; /* Chip can't handle DMA to/from >1GB, use bounce buffer */ - if (!dma_mapping_error(bp->sdev->dma_dev, mapping)) - dma_unmap_single(bp->sdev->dma_dev, mapping, len, - DMA_TO_DEVICE); - + dma_unmap_single(bp->sdev->dma_dev, mapping, len, + DMA_TO_DEVICE); +workaround: bounce_skb = alloc_skb(len, GFP_ATOMIC | GFP_DMA); if (!bounce_skb) goto err_out; mapping = dma_map_single(bp->sdev->dma_dev, bounce_skb->data, len, DMA_TO_DEVICE); - if (dma_mapping_error(bp->sdev->dma_dev, mapping) || mapping + len > DMA_BIT_MASK(30)) { - if (!dma_mapping_error(bp->sdev->dma_dev, mapping)) - dma_unmap_single(bp->sdev->dma_dev, mapping, - len, DMA_TO_DEVICE); + if (dma_mapping_error(bp->sdev->dma_dev, mapping)) + goto failed; + if (mapping + len > DMA_BIT_MASK(30)+1) { + dma_unmap_single(bp->sdev->dma_dev, mapping, len, + DMA_TO_DEVICE); +failed: dev_kfree_skb_any(bounce_skb); goto err_out; }
The requirement is dma_addr + size <= 0x40000000, not 0x3fffffff. In a logically separate but overlapping change, this patch also rearranges the logic for detecting failures to use a goto rather than testing dma_mapping_error() twice. The latter is expensive if CONFIG_DMA_API_DEBUG is set, but also for bug-proofing reasons I try to avoid having the same condition in two places that must be kept in sync. Signed-off-by: George Spelvin <lkml@sdf.org> --- drivers/net/ethernet/broadcom/b44.c | 42 ++++++++++++++++------------- 1 file changed, 24 insertions(+), 18 deletions(-)