[PATCH] skge: fix broken driver

[PATCH] skge: fix broken driver

[Mikulas Patocka]

The patch 136d8f377e1575463b47840bc5f1b22d94bf8f63 broke the skge driver.

Note this part of the patch:
+               if (skge_rx_setup(skge, e, nskb, skge->rx_buf_size) < 0) {
+                       dev_kfree_skb(nskb);
+                       goto resubmit;
+               }
+
                pci_unmap_single(skge->hw->pdev,
                                 dma_unmap_addr(e, mapaddr),
                                 dma_unmap_len(e, maplen),
                                 PCI_DMA_FROMDEVICE);
                skb = e->skb;
                prefetch(skb->data);
-               skge_rx_setup(skge, e, nskb, skge->rx_buf_size);

The function skge_rx_setup modifies e->skb to point to the new skb. Thus,
after this change, the new buffer, not the old, is returned to the
networking stack.

This bug is present in kernels 3.11, 3.11.1 and 3.12-rc1.

Signed-off-by: Mikulas Patocka <mpatocka@redhat.com>
Cc: stable@kernel.org # 3.11

---
 drivers/net/ethernet/marvell/skge.c |    5 +++--
 1 file changed, 3 insertions(+), 2 deletions(-)

Index: linux-3.11.1-fast/drivers/net/ethernet/marvell/skge.c
===================================================================
--- linux-3.11.1-fast.orig/drivers/net/ethernet/marvell/skge.c	2013-09-10 19:46:58.000000000 +0200
+++ linux-3.11.1-fast/drivers/net/ethernet/marvell/skge.c	2013-09-19 18:20:43.000000000 +0200
@@ -3092,6 +3092,9 @@ static struct sk_buff *skge_rx_get(struc
 		if (!nskb)
 			goto resubmit;
 
+		skb = e->skb;
+		prefetch(skb->data);
+
 		if (skge_rx_setup(skge, e, nskb, skge->rx_buf_size) < 0) {
 			dev_kfree_skb(nskb);
 			goto resubmit;
@@ -3101,8 +3104,6 @@ static struct sk_buff *skge_rx_get(struc
 				 dma_unmap_addr(e, mapaddr),
 				 dma_unmap_len(e, maplen),
 				 PCI_DMA_FROMDEVICE);
-		skb = e->skb;
-		prefetch(skb->data);
 	}
 
 	skb_put(skb, len);

Re: [PATCH] skge: fix broken driver

[David Miller]

From: Mikulas Patocka <mpatocka@redhat.com>
Date: Thu, 19 Sep 2013 12:33:30 -0400 (EDT)

> Signed-off-by: Mikulas Patocka <mpatocka@redhat.com>
> Cc: stable@kernel.org # 3.11

First, this is missing the reported-by and tested-by tags provided
by people who tested this patch.

Secondly, CC:'ing stable is not the correct way to submit networking
patches for -stable inclusion.  You simply ask me to queue them up
for -stable explicitly instead.

Please re-submit this with the currect signoffs, thank you.

Re: [PATCH] skge: fix broken driver

[Mikulas Patocka]

On Thu, 19 Sep 2013, David Miller wrote:

> From: Mikulas Patocka <mpatocka@redhat.com>
> Date: Thu, 19 Sep 2013 12:33:30 -0400 (EDT)
> 
> > Signed-off-by: Mikulas Patocka <mpatocka@redhat.com>
> > Cc: stable@kernel.org # 3.11
> 
> First, this is missing the reported-by and tested-by tags provided
> by people who tested this patch.

I noticed the problem and tested the patch on my own machine. So I added 
myself to these tags.

> Secondly, CC:'ing stable is not the correct way to submit networking
> patches for -stable inclusion.  You simply ask me to queue them up
> for -stable explicitly instead.
> 
> Please re-submit this with the currect signoffs, thank you.

Here I'm re-submitting it.

---

skge: fix broken driver

The patch 136d8f377e1575463b47840bc5f1b22d94bf8f63 broke the skge driver.
Note this part of the patch:
+               if (skge_rx_setup(skge, e, nskb, skge->rx_buf_size) < 0) {
+                       dev_kfree_skb(nskb);
+                       goto resubmit;
+               }
+
                pci_unmap_single(skge->hw->pdev,
                                 dma_unmap_addr(e, mapaddr),
                                 dma_unmap_len(e, maplen),
                                 PCI_DMA_FROMDEVICE);
                skb = e->skb;
                prefetch(skb->data);
-               skge_rx_setup(skge, e, nskb, skge->rx_buf_size);

The function skge_rx_setup modifies e->skb to point to the new skb. Thus,
after this change, the new buffer, not the old, is returned to the
networking stack.

This bug is present in kernels 3.11, 3.11.1 and 3.12-rc1. The patch should 
be queued for 3.11-stable.

Signed-off-by: Mikulas Patocka <mpatocka@redhat.com>
Reported-by: Mikulas Patocka <mpatocka@redhat.com>
Tested-by: Mikulas Patocka <mpatocka@redhat.com>

---
 drivers/net/ethernet/marvell/skge.c |    5 +++--
 1 file changed, 3 insertions(+), 2 deletions(-)

Index: linux-3.11.1-fast/drivers/net/ethernet/marvell/skge.c
===================================================================
--- linux-3.11.1-fast.orig/drivers/net/ethernet/marvell/skge.c	2013-09-10 19:46:58.000000000 +0200
+++ linux-3.11.1-fast/drivers/net/ethernet/marvell/skge.c	2013-09-19 18:20:43.000000000 +0200
@@ -3092,6 +3092,9 @@ static struct sk_buff *skge_rx_get(struc
 		if (!nskb)
 			goto resubmit;
 
+		skb = e->skb;
+		prefetch(skb->data);
+
 		if (skge_rx_setup(skge, e, nskb, skge->rx_buf_size) < 0) {
 			dev_kfree_skb(nskb);
 			goto resubmit;
@@ -3101,8 +3104,6 @@ static struct sk_buff *skge_rx_get(struc
 				 dma_unmap_addr(e, mapaddr),
 				 dma_unmap_len(e, maplen),
 				 PCI_DMA_FROMDEVICE);
-		skb = e->skb;
-		prefetch(skb->data);
 	}
 
 	skb_put(skb, len);

Re: [PATCH] skge: fix broken driver

[David Miller]

From: Mikulas Patocka <mpatocka@redhat.com>
Date: Thu, 19 Sep 2013 14:04:38 -0400 (EDT)

> Here I'm re-submitting it.

Please do not hijack an existing discussions to resubmit the patch.

Instead, make a fresh, completely new, mailing list posting for the
patch submissions.

Thank you.

Re: [PATCH] skge: fix broken driver

[Igor Gnatenko]

On Thu, 2013-09-19 at 14:04 -0400, Mikulas Patocka wrote:
> 
> 
> On Thu, 19 Sep 2013, David Miller wrote:
> 
> > From: Mikulas Patocka <mpatocka@redhat.com>
> > Date: Thu, 19 Sep 2013 12:33:30 -0400 (EDT)
> > 
> > > Signed-off-by: Mikulas Patocka <mpatocka@redhat.com>
> > > Cc: stable@kernel.org # 3.11
> > 
> > First, this is missing the reported-by and tested-by tags provided
> > by people who tested this patch.
> 
> I noticed the problem and tested the patch on my own machine. So I added 
> myself to these tags.
> 
> > Secondly, CC:'ing stable is not the correct way to submit networking
> > patches for -stable inclusion.  You simply ask me to queue them up
> > for -stable explicitly instead.
> > 
> > Please re-submit this with the currect signoffs, thank you.
> 
> Here I'm re-submitting it.
> 
> ---
> 
> skge: fix broken driver
> 
> The patch 136d8f377e1575463b47840bc5f1b22d94bf8f63 broke the skge driver.
> Note this part of the patch:
> +               if (skge_rx_setup(skge, e, nskb, skge->rx_buf_size) < 0) {
> +                       dev_kfree_skb(nskb);
> +                       goto resubmit;
> +               }
> +
>                 pci_unmap_single(skge->hw->pdev,
>                                  dma_unmap_addr(e, mapaddr),
>                                  dma_unmap_len(e, maplen),
>                                  PCI_DMA_FROMDEVICE);
>                 skb = e->skb;
>                 prefetch(skb->data);
> -               skge_rx_setup(skge, e, nskb, skge->rx_buf_size);
> 
> The function skge_rx_setup modifies e->skb to point to the new skb. Thus,
> after this change, the new buffer, not the old, is returned to the
> networking stack.
> 
> This bug is present in kernels 3.11, 3.11.1 and 3.12-rc1. The patch should 
> be queued for 3.11-stable.
> 
> Signed-off-by: Mikulas Patocka <mpatocka@redhat.com>
> Reported-by: Mikulas Patocka <mpatocka@redhat.com>
> Tested-by: Mikulas Patocka <mpatocka@redhat.com>
> 
> ---
>  drivers/net/ethernet/marvell/skge.c |    5 +++--
>  1 file changed, 3 insertions(+), 2 deletions(-)
> 
> Index: linux-3.11.1-fast/drivers/net/ethernet/marvell/skge.c
> ===================================================================
> --- linux-3.11.1-fast.orig/drivers/net/ethernet/marvell/skge.c  2013-09-10 19:46:58.000000000 +0200
> +++ linux-3.11.1-fast/drivers/net/ethernet/marvell/skge.c       2013-09-19 18:20:43.000000000 +0200
> @@ -3092,6 +3092,9 @@ static struct sk_buff *skge_rx_get(struc
>                 if (!nskb)
>                         goto resubmit;
>  
> +               skb = e->skb;
> +               prefetch(skb->data);
> +
>                 if (skge_rx_setup(skge, e, nskb, skge->rx_buf_size) < 0) {
>                         dev_kfree_skb(nskb);
>                         goto resubmit;
> @@ -3101,8 +3104,6 @@ static struct sk_buff *skge_rx_get(struc
>                                  dma_unmap_addr(e, mapaddr),
>                                  dma_unmap_len(e, maplen),
>                                  PCI_DMA_FROMDEVICE);
> -               skb = e->skb;
> -               prefetch(skb->data);
>         }
>  
>         skb_put(skb, len);
> --
> To unsubscribe from this list: send the line "unsubscribe netdev" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html

Hey Mikulas! See https://lkml.org/lkml/2013/9/19/38 , plz.
-- 
Igor Gnatenko
Fedora release 20 (Heisenbug)
Linux 3.11.1-300.fc20.x86_64

Re: [PATCH] skge: fix broken driver

[Mikulas Patocka]

On Thu, 19 Sep 2013, Igor Gnatenko wrote:

> On Thu, 2013-09-19 at 14:04 -0400, Mikulas Patocka wrote:
> > 
> > 
> > On Thu, 19 Sep 2013, David Miller wrote:
> > 
> > > From: Mikulas Patocka <mpatocka@redhat.com>
> > > Date: Thu, 19 Sep 2013 12:33:30 -0400 (EDT)
> > > 
> > > > Signed-off-by: Mikulas Patocka <mpatocka@redhat.com>
> > > > Cc: stable@kernel.org # 3.11
> > > 
> > > First, this is missing the reported-by and tested-by tags provided
> > > by people who tested this patch.
> > 
> > I noticed the problem and tested the patch on my own machine. So I added 
> > myself to these tags.
> > 
> > > Secondly, CC:'ing stable is not the correct way to submit networking
> > > patches for -stable inclusion.  You simply ask me to queue them up
> > > for -stable explicitly instead.
> > > 
> > > Please re-submit this with the currect signoffs, thank you.
> > 
> > Here I'm re-submitting it.
> > 
> > ---
> > 
> > skge: fix broken driver
> > 
> > The patch 136d8f377e1575463b47840bc5f1b22d94bf8f63 broke the skge driver.
> > Note this part of the patch:
> > +               if (skge_rx_setup(skge, e, nskb, skge->rx_buf_size) < 0) {
> > +                       dev_kfree_skb(nskb);
> > +                       goto resubmit;
> > +               }
> > +
> >                 pci_unmap_single(skge->hw->pdev,
> >                                  dma_unmap_addr(e, mapaddr),
> >                                  dma_unmap_len(e, maplen),
> >                                  PCI_DMA_FROMDEVICE);
> >                 skb = e->skb;
> >                 prefetch(skb->data);
> > -               skge_rx_setup(skge, e, nskb, skge->rx_buf_size);
> > 
> > The function skge_rx_setup modifies e->skb to point to the new skb. Thus,
> > after this change, the new buffer, not the old, is returned to the
> > networking stack.
> > 
> > This bug is present in kernels 3.11, 3.11.1 and 3.12-rc1. The patch should 
> > be queued for 3.11-stable.
> > 
> > Signed-off-by: Mikulas Patocka <mpatocka@redhat.com>
> > Reported-by: Mikulas Patocka <mpatocka@redhat.com>
> > Tested-by: Mikulas Patocka <mpatocka@redhat.com>
> > 
> > ---
> >  drivers/net/ethernet/marvell/skge.c |    5 +++--
> >  1 file changed, 3 insertions(+), 2 deletions(-)
> > 
> > Index: linux-3.11.1-fast/drivers/net/ethernet/marvell/skge.c
> > ===================================================================
> > --- linux-3.11.1-fast.orig/drivers/net/ethernet/marvell/skge.c  2013-09-10 19:46:58.000000000 +0200
> > +++ linux-3.11.1-fast/drivers/net/ethernet/marvell/skge.c       2013-09-19 18:20:43.000000000 +0200
> > @@ -3092,6 +3092,9 @@ static struct sk_buff *skge_rx_get(struc
> >                 if (!nskb)
> >                         goto resubmit;
> >  
> > +               skb = e->skb;
> > +               prefetch(skb->data);
> > +
> >                 if (skge_rx_setup(skge, e, nskb, skge->rx_buf_size) < 0) {
> >                         dev_kfree_skb(nskb);
> >                         goto resubmit;
> > @@ -3101,8 +3104,6 @@ static struct sk_buff *skge_rx_get(struc
> >                                  dma_unmap_addr(e, mapaddr),
> >                                  dma_unmap_len(e, maplen),
> >                                  PCI_DMA_FROMDEVICE);
> > -               skb = e->skb;
> > -               prefetch(skb->data);
> >         }
> >  
> >         skb_put(skb, len);
> > --
> > To unsubscribe from this list: send the line "unsubscribe netdev" in
> > the body of a message to majordomo@vger.kernel.org
> > More majordomo info at  http://vger.kernel.org/majordomo-info.html
> 
> Hey Mikulas! See https://lkml.org/lkml/2013/9/19/38 , plz.
> -- 
> Igor Gnatenko
> Fedora release 20 (Heisenbug)
> Linux 3.11.1-300.fc20.x86_64

I see. My patch is a bit simpler - it doesn't allocate the structure 
skge_element on the stack.

Mikulas

[PATCH] stable_kernel_rules.txt: Exclude networking from stable rules

[Joe Perches]

Networking is once again "special", so at least document
how it's working today in the hope that doing so makes
less work for all that actually read the documentation.

Signed-off-by: Joe Perches <joe@perches.com>
---
On Thu, 19 Sep 2013, David Miller wrote:
> Secondly, CC:'ing stable is not the correct way to submit networking
> patches for -stable inclusion.  You simply ask me to queue them up
> for -stable explicitly instead.

 Documentation/stable_kernel_rules.txt | 5 +++++
 1 file changed, 5 insertions(+)

diff --git a/Documentation/stable_kernel_rules.txt b/Documentation/stable_kernel_rules.txt
index b0714d8..a2d6da0 100644
--- a/Documentation/stable_kernel_rules.txt
+++ b/Documentation/stable_kernel_rules.txt
@@ -29,6 +29,11 @@ Rules on what kind of patches are accepted, and which ones are not, into the
 
 Procedure for submitting patches to the -stable tree:
 
+ - The networking tree (net/ and drivers/net/) is 'special' and doesn't
+   follow the rules below.  Don't send or cc: patches for the -stable tree to
+   stable@vger.kernel.org.  Don't mark them stable.  Just send the patches to
+   netdev@vger.kernel.org and let the networking maintainer decide what to do
+   with them.
  - Send the patch, after verifying that it follows the above rules, to
    stable@vger.kernel.org.  You must note the upstream commit ID in the
    changelog of your submission, as well as the kernel version you wish

Re: [PATCH] skge: fix broken driver

[Francois Romieu]

Mikulas Patocka <mpatocka@redhat.com> :
[...]
> I see. My patch is a bit simpler - it doesn't allocate the structure 
> skge_element on the stack.

Both patches don't behave exactly the same wrt pci_unmap_single.

-- 
Ueimor

Re: [PATCH] stable_kernel_rules.txt: Exclude networking from stable rules

[Francois Romieu]

Joe Perches <joe@perches.com> :
[...]
> diff --git a/Documentation/stable_kernel_rules.txt b/Documentation/stable_kernel_rules.txt
> index b0714d8..a2d6da0 100644
> --- a/Documentation/stable_kernel_rules.txt
> +++ b/Documentation/stable_kernel_rules.txt
> @@ -29,6 +29,11 @@ Rules on what kind of patches are accepted, and which ones are not, into the
>  
>  Procedure for submitting patches to the -stable tree:
>  
> + - The networking tree (net/ and drivers/net/) is 'special' and doesn't
> +   follow the rules below.  Don't send or cc: patches for the -stable tree to
> +   stable@vger.kernel.org.  Don't mark them stable.  Just send the patches to
> +   netdev@vger.kernel.org and let the networking maintainer decide what to do
> +   with them.

David said "simply ask me to queue them up for -stable explicitly".

He did not say "send the patches and let me decide what to do with them".

-- 
Ueimor

Re: [PATCH] stable_kernel_rules.txt: Exclude networking from stable rules

[Joe Perches]

On Thu, 2013-09-19 at 23:32 +0200, Francois Romieu wrote:
> Joe Perches <joe@perches.com> :
> [...]
> > diff --git a/Documentation/stable_kernel_rules.txt b/Documentation/stable_kernel_rules.txt
> > index b0714d8..a2d6da0 100644
> > --- a/Documentation/stable_kernel_rules.txt
> > +++ b/Documentation/stable_kernel_rules.txt
> > @@ -29,6 +29,11 @@ Rules on what kind of patches are accepted, and which ones are not, into the
> >  
> >  Procedure for submitting patches to the -stable tree:
> >  
> > + - The networking tree (net/ and drivers/net/) is 'special' and doesn't
> > +   follow the rules below.  Don't send or cc: patches for the -stable tree to
> > +   stable@vger.kernel.org.  Don't mark them stable.  Just send the patches to
> > +   netdev@vger.kernel.org and let the networking maintainer decide what to do
> > +   with them.
> 
> David said "simply ask me to queue them up for -stable explicitly".
> 
> He did not say "send the patches and let me decide what to do with them".
> 

David selects them regardless.

from Documentation/networking/netdev-FAQ.txt:

Q: How can I tell what patches are queued up for backporting to the
   various stable releases?

A: Normally Greg Kroah-Hartman collects stable commits himself, but
   for networking, Dave collects up patches he deems critical for the
   networking subsystem, and then hands them off to Greg.

   There is a patchworks queue that you can see here:
	http://patchwork.ozlabs.org/bundle/davem/stable/?state=*

   It contains the patches which Dave has selected, but not yet handed
   off to Greg.  If Greg already has the patch, then it will be here:
	http://git.kernel.org/cgit/linux/kernel/git/stable/stable-queue.git

   A quick way to find whether the patch is in this stable-queue is
   to simply clone the repo, and then git grep the mainline commit ID, e.g.

	stable-queue$ git grep -l 284041ef21fdf2e
	releases/3.0.84/ipv6-fix-possible-crashes-in-ip6_cork_release.patch
	releases/3.4.51/ipv6-fix-possible-crashes-in-ip6_cork_release.patch
	releases/3.9.8/ipv6-fix-possible-crashes-in-ip6_cork_release.patch
	stable/stable-queue$

Q: I see a network patch and I think it should be backported to stable.
   Should I request it via "stable@vger.kernel.org" like the references in
   the kernel's Documentation/stable_kernel_rules.txt file say?

A: No, not for networking.  Check the stable queues as per above 1st to see
   if it is already queued.  If not, then send a mail to netdev, listing
   the upstream commit ID and why you think it should be a stable candidate.

   Before you jump to go do the above, do note that the normal stable rules
   in Documentation/stable_kernel_rules.txt still apply.  So you need to
   explicitly indicate why it is a critical fix and exactly what users are
   impacted.  In addition, you need to convince yourself that you _really_
   think it has been overlooked, vs. having been considered and rejected.

   Generally speaking, the longer it has had a chance to "soak" in mainline,
   the better the odds that it is an OK candidate for stable.  So scrambling
   to request a commit be added the day after it appears should be avoided.

Q: I have created a network patch and I think it should be backported to
   stable.  Should I add a "Cc: stable@vger.kernel.org" like the references
   in the kernel's Documentation/ directory say?

A: No.  See above answer.  In short, if you think it really belongs in
   stable, then ensure you write a decent commit log that describes who
   gets impacted by the bugfix and how it manifests itself, and when the
   bug was introduced.  If you do that properly, then the commit will
   get handled appropriately and most likely get put in the patchworks
   stable queue if it really warrants it.

   If you think there is some valid information relating to it being in
   stable that does _not_ belong in the commit log, then use the three
   dash marker line as described in Documentation/SubmittingPatches to
   temporarily embed that information into the patch that you send.

Re: [PATCH] stable_kernel_rules.txt: Exclude networking from stable rules

[Francois Romieu]

Joe Perches <joe@perches.com> :
[...]
> David selects them regardless.
> 
> from Documentation/networking/netdev-FAQ.txt:

I don't believe that those who read Documentation/stable_kernel_rules.txt
will magically read networking/netdev-FAQ.txt as well nor figure that
while they should not mark the patches stables (skr.txt), they are
expected to apply some extra rules as well (nF.txt).

It's fine if you disagree. I won't argue.

-- 
Ueimor

Re: [PATCH] skge: fix broken driver

[Mikulas Patocka]

On Thu, 19 Sep 2013, Francois Romieu wrote:

> Mikulas Patocka <mpatocka@redhat.com> :
> [...]
> > I see. My patch is a bit simpler - it doesn't allocate the structure 
> > skge_element on the stack.
> 
> Both patches don't behave exactly the same wrt pci_unmap_single.
> 
> -- 
> Ueimor

I see, my patch passes a wrong value to pci_unmap_single. So I made this 
change to make it pass the correct value. Do you agree with this patch?

---

skge: fix invalid value passed to pci_unmap_sigle

In my patch c194992cbe71c20bb3623a566af8d11b0bfaa721 I didn't fix the skge
bug correctly. The value of the new mapping (not old) was passed to
pci_unmap_single.

If we enable CONFIG_DMA_API_DEBUG, it results in this warning:
WARNING: CPU: 0 PID: 0 at lib/dma-debug.c:986 check_sync+0x4c4/0x580()
skge 0000:02:07.0: DMA-API: device driver tries to sync DMA memory it has
not allocated [device address=0x000000023a0096c0] [size=1536 bytes]

This patch makes the skge driver pass the correct value to
pci_unmap_single and fixes the warning. It copies the old descriptor to
on-stack variable "ee" and unmaps it if mapping of the new descriptor
succeeded.

This patch should be backported to 3.11-stable.

Signed-off-by: Mikulas Patocka <mpatocka@redhat.com>
Reported-by: Francois Romieu <romieu@fr.zoreil.com>
Tested-by: Mikulas Patocka <mpatocka@redhat.com>

---
 drivers/net/ethernet/marvell/skge.c |    9 ++++++---
 1 file changed, 6 insertions(+), 3 deletions(-)

Index: linux-3.11.1-fast/drivers/net/ethernet/marvell/skge.c
===================================================================
--- linux-3.11.1-fast.orig/drivers/net/ethernet/marvell/skge.c	2013-09-20 16:13:24.000000000 +0200
+++ linux-3.11.1-fast/drivers/net/ethernet/marvell/skge.c	2013-09-20 16:18:13.000000000 +0200
@@ -3086,13 +3086,16 @@ static struct sk_buff *skge_rx_get(struc
 					       PCI_DMA_FROMDEVICE);
 		skge_rx_reuse(e, skge->rx_buf_size);
 	} else {
+		struct skge_element ee;
 		struct sk_buff *nskb;
 
 		nskb = netdev_alloc_skb_ip_align(dev, skge->rx_buf_size);
 		if (!nskb)
 			goto resubmit;
 
-		skb = e->skb;
+		ee = *e;
+
+		skb = ee.skb;
 		prefetch(skb->data);
 
 		if (skge_rx_setup(skge, e, nskb, skge->rx_buf_size) < 0) {
@@ -3101,8 +3104,8 @@ static struct sk_buff *skge_rx_get(struc
 		}
 
 		pci_unmap_single(skge->hw->pdev,
-				 dma_unmap_addr(e, mapaddr),
-				 dma_unmap_len(e, maplen),
+				 dma_unmap_addr(&ee, mapaddr),
+				 dma_unmap_len(&ee, maplen),
 				 PCI_DMA_FROMDEVICE);
 	}
 

Re: [PATCH] stable_kernel_rules.txt: Exclude networking from stable rules

[Joe Perches]

On Thu, 2013-09-19 at 11:31 -0700, Joe Perches wrote:
> Networking is once again "special", so at least document
> how it's working today in the hope that doing so makes
> less work for all that actually read the documentation.

David, why did you mark this N/A is patchwork?
It's your rule, why not document it?

Re: [PATCH] skge: fix broken driver

[Igor Gnatenko]

On Fri, 2013-09-20 at 10:32 -0400, Mikulas Patocka wrote:
> 
> 
> On Thu, 19 Sep 2013, Francois Romieu wrote:
> 
> > Mikulas Patocka <mpatocka@redhat.com> :
> > [...]
> > > I see. My patch is a bit simpler - it doesn't allocate the structure 
> > > skge_element on the stack.
> > 
> > Both patches don't behave exactly the same wrt pci_unmap_single.
> > 
> > -- 
> > Ueimor
> 
> I see, my patch passes a wrong value to pci_unmap_single. So I made this 
> change to make it pass the correct value. Do you agree with this patch?
> 
> ---
> 
> skge: fix invalid value passed to pci_unmap_sigle
> 
> In my patch c194992cbe71c20bb3623a566af8d11b0bfaa721 I didn't fix the skge
> bug correctly. The value of the new mapping (not old) was passed to
> pci_unmap_single.
> 
> If we enable CONFIG_DMA_API_DEBUG, it results in this warning:
> WARNING: CPU: 0 PID: 0 at lib/dma-debug.c:986 check_sync+0x4c4/0x580()
> skge 0000:02:07.0: DMA-API: device driver tries to sync DMA memory it has
> not allocated [device address=0x000000023a0096c0] [size=1536 bytes]
> 
> This patch makes the skge driver pass the correct value to
> pci_unmap_single and fixes the warning. It copies the old descriptor to
> on-stack variable "ee" and unmaps it if mapping of the new descriptor
> succeeded.
> 
> This patch should be backported to 3.11-stable.
> 
> Signed-off-by: Mikulas Patocka <mpatocka@redhat.com>
> Reported-by: Francois Romieu <romieu@fr.zoreil.com>
> Tested-by: Mikulas Patocka <mpatocka@redhat.com>
> 
> ---
>  drivers/net/ethernet/marvell/skge.c |    9 ++++++---
>  1 file changed, 6 insertions(+), 3 deletions(-)
> 
> Index: linux-3.11.1-fast/drivers/net/ethernet/marvell/skge.c
> ===================================================================
> --- linux-3.11.1-fast.orig/drivers/net/ethernet/marvell/skge.c  2013-09-20 16:13:24.000000000 +0200
> +++ linux-3.11.1-fast/drivers/net/ethernet/marvell/skge.c       2013-09-20 16:18:13.000000000 +0200
> @@ -3086,13 +3086,16 @@ static struct sk_buff *skge_rx_get(struc
>                                                PCI_DMA_FROMDEVICE);
>                 skge_rx_reuse(e, skge->rx_buf_size);
>         } else {
> +               struct skge_element ee;
>                 struct sk_buff *nskb;
>  
>                 nskb = netdev_alloc_skb_ip_align(dev, skge->rx_buf_size);
>                 if (!nskb)
>                         goto resubmit;
>  
> -               skb = e->skb;
> +               ee = *e;
> +
> +               skb = ee.skb;
>                 prefetch(skb->data);
>  
>                 if (skge_rx_setup(skge, e, nskb, skge->rx_buf_size) < 0) {
> @@ -3101,8 +3104,8 @@ static struct sk_buff *skge_rx_get(struc
>                 }
>  
>                 pci_unmap_single(skge->hw->pdev,
> -                                dma_unmap_addr(e, mapaddr),
> -                                dma_unmap_len(e, maplen),
> +                                dma_unmap_addr(&ee, mapaddr),
> +                                dma_unmap_len(&ee, maplen),
>                                  PCI_DMA_FROMDEVICE);
>         }
>  

Mikulas, I think you should send this patch separate..
-- 
Igor Gnatenko
Fedora release 20 (Heisenbug)
Linux 3.11.1-300.fc20.x86_64

Re: [PATCH] stable_kernel_rules.txt: Exclude networking from stable rules

[David Miller]

From: Joe Perches <joe@perches.com>
Date: Fri, 20 Sep 2013 07:54:40 -0700

> On Thu, 2013-09-19 at 11:31 -0700, Joe Perches wrote:
>> Networking is once again "special", so at least document
>> how it's working today in the hope that doing so makes
>> less work for all that actually read the documentation.
> 
> David, why did you mark this N/A is patchwork?
> It's your rule, why not document it?

Because it should go through whoever maintains that document, and
that isn't me.

Re: [PATCH] skge: fix broken driver

[Francois Romieu]

Mikulas Patocka <mpatocka@redhat.com> :
> On Thu, 19 Sep 2013, Francois Romieu wrote:
[...]
> > Both patches don't behave exactly the same wrt pci_unmap_single.
[...]
> I see, my patch passes a wrong value to pci_unmap_single. So I made this 
> change to make it pass the correct value. Do you agree with this patch ?

Yes. I did not report it. Igor did.

You may "struct skge_element ee = *e;" and save a line. Who cares about
the extra copy when netdev_alloc_skb_ip_align fails ?

Something less ugly for the longer term
- use netdev_alloc_skb_ip_align in skge_rx_fill
- have skge_rx_setup return previouly stored sk_buff * - NULL if it was so -
  and ERR_PTR when it fails for whatever reason
- move netdev_alloc_skb_ip_align into skge_rx_setup
- pci_unmap in skge_rx_setup
- profit

Or isolate the struct sk_buff * + DEFINE_DMA_ part in skge_element then
save it as a whole in skge_rx_setup before initializing a new one as a
(netdev_alloc_skb_ip_align + pci_map).

Does someone volunteer to write it for net-next once the fix has been
merged and later pulled into net-next ?

-- 
Ueimor

Re: [PATCH] stable_kernel_rules.txt: Exclude networking from stable rules

[Christoph Hellwig]

This is also the preferred way to do it for XFS.  Maybe word it in a way
that we can easily add subsystems.

To me it generally seems to be the best way to do it - having random Ccs
and lots of stable trees doesn't seem like a very good way of handling
it.

Re: [PATCH] skge: fix broken driver

[Mikulas Patocka]

On Fri, 20 Sep 2013, Francois Romieu wrote:

> Mikulas Patocka <mpatocka@redhat.com> :
> > On Thu, 19 Sep 2013, Francois Romieu wrote:
> [...]
> > > Both patches don't behave exactly the same wrt pci_unmap_single.
> [...]
> > I see, my patch passes a wrong value to pci_unmap_single. So I made this 
> > change to make it pass the correct value. Do you agree with this patch ?
> 
> Yes. I did not report it. Igor did.

You did report it. You said earlier in some email: "Both patches don't 
behave exactly the same wrt pci_unmap_single." I found this bug because of 
that message.

Mikulas

> You may "struct skge_element ee = *e;" and save a line. Who cares about
> the extra copy when netdev_alloc_skb_ip_align fails ?
> 
> Something less ugly for the longer term
> - use netdev_alloc_skb_ip_align in skge_rx_fill
> - have skge_rx_setup return previouly stored sk_buff * - NULL if it was so -
>   and ERR_PTR when it fails for whatever reason
> - move netdev_alloc_skb_ip_align into skge_rx_setup
> - pci_unmap in skge_rx_setup
> - profit
> 
> Or isolate the struct sk_buff * + DEFINE_DMA_ part in skge_element then
> save it as a whole in skge_rx_setup before initializing a new one as a
> (netdev_alloc_skb_ip_align + pci_map).
> 
> Does someone volunteer to write it for net-next once the fix has been
> merged and later pulled into net-next ?
> 
> -- 
> Ueimor
> 

Re: [PATCH] stable_kernel_rules.txt: Exclude networking from stable rules

[Joe Perches]

On Sun, 2013-09-22 at 11:51 -0700, Christoph Hellwig wrote:
> This is also the preferred way to do it for XFS.  Maybe word it in a way
> that we can easily add subsystems.
> 
> To me it generally seems to be the best way to do it - having random Ccs
> and lots of stable trees doesn't seem like a very good way of handling
> it.

Maybe adding a mechanism to MAINTAINERS would be better.
Maybe a default B: (backport?) of stable@vger.kernel.org
with a per-subsystem override?

SUBSYSTEM TYPE
M:	maintainer@email.address
L:	list@email.address
S:	Supported
F:	file/pattern/
B:	stable@email.address


_______________________________________________
xfs mailing list
xfs@oss.sgi.com
http://oss.sgi.com/mailman/listinfo/xfs

Re: [PATCH] stable_kernel_rules.txt: Exclude networking from stable rules

[Christoph Hellwig]

On Mon, Sep 23, 2013 at 01:34:05PM -0700, Joe Perches wrote:
> Maybe adding a mechanism to MAINTAINERS would be better.
> Maybe a default B: (backport?) of stable@vger.kernel.org
> with a per-subsystem override?

Sounds fine to me.

_______________________________________________
xfs mailing list
xfs@oss.sgi.com
http://oss.sgi.com/mailman/listinfo/xfs