[RFC,2/4] b44: Fix off-by-one error in acceptable address range
diff mbox series

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
Related show

Commit Message

George Spelvin Dec. 8, 2019, 8:36 a.m. UTC
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(-)

Comments

Michael Chan Dec. 9, 2019, 5:29 p.m. UTC | #1
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.
George Spelvin Dec. 9, 2019, 6:01 p.m. UTC | #2
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.)

Patch
diff mbox series

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;
 		}