[PATCH] 3c59x: fix bad split of cpu_to_le32(pci_map_single())

[PATCH] 3c59x: fix bad split of cpu_to_le32(pci_map_single())

[Sylvain 'ythier' Hitier]

Author: Sylvain "ythier" Hitier <sylvain.hitier@gmail.com>
Date:   Wed Sep 24 09:22:16 2014 +0000

        3c59x: fix bad split of cpu_to_le32(pci_map_single())
    
        Change the #else branch like the #if DO_ZEROCOPY branch was changed.
    
        Fixes: 6f2b6a3005b2c34c39f207a87667564f64f2f91a
          # 3c59x: Add dma error checking and recovery
    
        Signed-off-by: Sylvain "ythier" Hitier <sylvain.hitier@gmail.com>

diff --git a/drivers/net/ethernet/3com/3c59x.c b/drivers/net/ethernet/3com/3c59x.c
index 8ca49f04..0a3108b3 100644
--- a/drivers/net/ethernet/3com/3c59x.c
+++ b/drivers/net/ethernet/3com/3c59x.c
@@ -2214,7 +2214,7 @@ boomerang_start_xmit(struct sk_buff *skb, struct net_device *dev)
 		}
 	}
 #else
-	dma_addr = cpu_to_le32(pci_map_single(VORTEX_PCI(vp), skb->data, skb->len, PCI_DMA_TODEVICE));
+	dma_addr = pci_map_single(VORTEX_PCI(vp), skb->data, skb->len, PCI_DMA_TODEVICE);
 	if (dma_mapping_error(&VORTEX_PCI(vp)->dev, dma_addr))
 		goto out_dma_err;
 	vp->tx_ring[entry].addr = cpu_to_le32(dma_addr);



Regards,
Sylvain "ythier" Hitier

-- 
Business is about being busy, not being rich...
Lived 777 days in a Debian package => http://en.wikipedia.org/wiki/Apt,_Vaucluse
There's THE room for ideals in this mechanical place!

Re: [PATCH] 3c59x: fix bad split of cpu_to_le32(pci_map_single())

[Neil Horman]

On Wed, Sep 24, 2014 at 09:30:21AM +0000, Sylvain 'ythier' Hitier wrote:
> Author: Sylvain "ythier" Hitier <sylvain.hitier@gmail.com>
> Date:   Wed Sep 24 09:22:16 2014 +0000
> 
>         3c59x: fix bad split of cpu_to_le32(pci_map_single())
>     
>         Change the #else branch like the #if DO_ZEROCOPY branch was changed.
>     
>         Fixes: 6f2b6a3005b2c34c39f207a87667564f64f2f91a
>           # 3c59x: Add dma error checking and recovery
>     
>         Signed-off-by: Sylvain "ythier" Hitier <sylvain.hitier@gmail.com>
> 
> diff --git a/drivers/net/ethernet/3com/3c59x.c b/drivers/net/ethernet/3com/3c59x.c
> index 8ca49f04..0a3108b3 100644
> --- a/drivers/net/ethernet/3com/3c59x.c
> +++ b/drivers/net/ethernet/3com/3c59x.c
> @@ -2214,7 +2214,7 @@ boomerang_start_xmit(struct sk_buff *skb, struct net_device *dev)
>  		}
>  	}
>  #else
> -	dma_addr = cpu_to_le32(pci_map_single(VORTEX_PCI(vp), skb->data, skb->len, PCI_DMA_TODEVICE));
> +	dma_addr = pci_map_single(VORTEX_PCI(vp), skb->data, skb->len, PCI_DMA_TODEVICE);
>  	if (dma_mapping_error(&VORTEX_PCI(vp)->dev, dma_addr))
>  		goto out_dma_err;
>  	vp->tx_ring[entry].addr = cpu_to_le32(dma_addr);
> 
> 
> 
> Regards,
> Sylvain "ythier" Hitier
> 
NAK, this isn't how the other side of that was changed.  I added an assignmnt of
the dma_addr to check mapping errors, but still used cpu_to_le32 when assigning
dma_addr to tx_ring[entry].addr.  No change is needed here as far as I see.
Neil

> -- 
> Business is about being busy, not being rich...
> Lived 777 days in a Debian package => http://en.wikipedia.org/wiki/Apt,_Vaucluse
> There's THE room for ideals in this mechanical place!
> 

Re: [PATCH] 3c59x: fix bad split of cpu_to_le32(pci_map_single())

[Neil Horman]

On Wed, Sep 24, 2014 at 09:30:21AM +0000, Sylvain 'ythier' Hitier wrote:
> Author: Sylvain "ythier" Hitier <sylvain.hitier@gmail.com>
> Date:   Wed Sep 24 09:22:16 2014 +0000
> 
>         3c59x: fix bad split of cpu_to_le32(pci_map_single())
>     
>         Change the #else branch like the #if DO_ZEROCOPY branch was changed.
>     
>         Fixes: 6f2b6a3005b2c34c39f207a87667564f64f2f91a
>           # 3c59x: Add dma error checking and recovery
>     
>         Signed-off-by: Sylvain "ythier" Hitier <sylvain.hitier@gmail.com>
> 
> diff --git a/drivers/net/ethernet/3com/3c59x.c b/drivers/net/ethernet/3com/3c59x.c
> index 8ca49f04..0a3108b3 100644
> --- a/drivers/net/ethernet/3com/3c59x.c
> +++ b/drivers/net/ethernet/3com/3c59x.c
> @@ -2214,7 +2214,7 @@ boomerang_start_xmit(struct sk_buff *skb, struct net_device *dev)
>  		}
>  	}
>  #else
> -	dma_addr = cpu_to_le32(pci_map_single(VORTEX_PCI(vp), skb->data, skb->len, PCI_DMA_TODEVICE));
> +	dma_addr = pci_map_single(VORTEX_PCI(vp), skb->data, skb->len, PCI_DMA_TODEVICE);
>  	if (dma_mapping_error(&VORTEX_PCI(vp)->dev, dma_addr))
>  		goto out_dma_err;
>  	vp->tx_ring[entry].addr = cpu_to_le32(dma_addr);
> 
Actually, scratch my last note.  Sorry, too early here.  This isn't about
removing the cpu_to_le32 as its already preformed in the assignment to .addr.
Thats actually somewhat broken for lots of reasons (previously we did 2
cpu_to_le32 swaps, which seems completely wrong).  You do probably want to fixup
the changelog to be a bit more clear as to whats going on here.
Neil

> 
> 
> Regards,
> Sylvain "ythier" Hitier
> 
> -- 
> Business is about being busy, not being rich...
> Lived 777 days in a Debian package => http://en.wikipedia.org/wiki/Apt,_Vaucluse
> There's THE room for ideals in this mechanical place!
> 

[PATCH v2] 3c59x: fix bad split of cpu_to_le32(pci_map_single())

[Sylvain 'ythier' Hitier]

Hello,

When: 2014-09-24_3@06-16-47 -0400
Who:  Neil Horman
What:
> You do probably want to fixup
> the changelog to be a bit more clear as to whats going on here.

Date:   Wed Sep 24 09:22:16 2014 +0000

3c59x: fix bad split of cpu_to_le32(pci_map_single())

In commit 6f2b6a3005b2c34c39f207a87667564f64f2f91a,
  # 3c59x: Add dma error checking and recovery
the intent is to split out the mapping from the byte-swapping in order to
insert a dma_mapping_error() check.

Kinda this semantic patch:

    // See http://coccinelle.lip6.fr/
    //
    // Beware, grouik-and-dirty!
    @@
    expression DEV, X, Y, Z;
    @@
    -   cpu_to_le32(pci_map_single(DEV, X, Y, Z))
    +   dma_addr_t addr = pci_map_single(DEV, X, Y, Z);
    +   if (dma_mapping_error(&DEV->dev, addr))
    +       /* snip */;
    +   cpu_to_le32(addr)

However, the #else part (of the #if DO_ZEROCOPY test) is changed this way:

    -   cpu_to_le32(pci_map_single(DEV, X, Y, Z))
    +   dma_addr_t addr = cpu_to_le32(pci_map_single(DEV, X, Y, Z));
    //                    ^^^^^^^^^^^
    //                    That mismatches the 3 other changes!
    +   if (dma_mapping_error(&DEV->dev, addr))
    +       /* snip */;
    +   cpu_to_le32(addr)

Let's remove the leftover cpu_to_le32() for coherency.

v2: Better changelog.

Fixes: 6f2b6a3005b2c34c39f207a87667564f64f2f91a
  # 3c59x: Add dma error checking and recovery
Cc: "David S. Miller" <davem@davemloft.net>
Cc: Meelis Roos <mroos@linux.ee>
Cc: Neil Horman <nhorman@tuxdriver.com>
Signed-off-by: Sylvain "ythier" Hitier <sylvain.hitier@gmail.com>
---
 drivers/net/ethernet/3com/3c59x.c |    2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/net/ethernet/3com/3c59x.c b/drivers/net/ethernet/3com/3c59x.c
index 8ca49f04..0a3108b3 100644
--- a/drivers/net/ethernet/3com/3c59x.c
+++ b/drivers/net/ethernet/3com/3c59x.c
@@ -2214,7 +2214,7 @@ boomerang_start_xmit(struct sk_buff *skb, struct net_device *dev)
 		}
 	}
 #else
-	dma_addr = cpu_to_le32(pci_map_single(VORTEX_PCI(vp), skb->data, skb->len, PCI_DMA_TODEVICE));
+	dma_addr = pci_map_single(VORTEX_PCI(vp), skb->data, skb->len, PCI_DMA_TODEVICE);
 	if (dma_mapping_error(&VORTEX_PCI(vp)->dev, dma_addr))
 		goto out_dma_err;
 	vp->tx_ring[entry].addr = cpu_to_le32(dma_addr);

Regards,
Sylvain "ythier" Hitier

-- 
Business is about being busy, not being rich...
Lived 777 days in a Debian package => http://en.wikipedia.org/wiki/Apt,_Vaucluse
There's THE room for ideals in this mechanical place!

Re: [PATCH v2] 3c59x: fix bad split of cpu_to_le32(pci_map_single())

[Neil Horman]

On Thu, Sep 25, 2014 at 03:15:19PM +0000, Sylvain 'ythier' Hitier wrote:
> Hello,
> 
> When: 2014-09-24_3@06-16-47 -0400
> Who:  Neil Horman
> What:
> > You do probably want to fixup
> > the changelog to be a bit more clear as to whats going on here.
> 
> Date:   Wed Sep 24 09:22:16 2014 +0000
> 
> 3c59x: fix bad split of cpu_to_le32(pci_map_single())
> 
> In commit 6f2b6a3005b2c34c39f207a87667564f64f2f91a,
>   # 3c59x: Add dma error checking and recovery
> the intent is to split out the mapping from the byte-swapping in order to
> insert a dma_mapping_error() check.
> 
> Kinda this semantic patch:
> 
>     // See http://coccinelle.lip6.fr/
>     //
>     // Beware, grouik-and-dirty!
>     @@
>     expression DEV, X, Y, Z;
>     @@
>     -   cpu_to_le32(pci_map_single(DEV, X, Y, Z))
>     +   dma_addr_t addr = pci_map_single(DEV, X, Y, Z);
>     +   if (dma_mapping_error(&DEV->dev, addr))
>     +       /* snip */;
>     +   cpu_to_le32(addr)
> 
> However, the #else part (of the #if DO_ZEROCOPY test) is changed this way:
> 
>     -   cpu_to_le32(pci_map_single(DEV, X, Y, Z))
>     +   dma_addr_t addr = cpu_to_le32(pci_map_single(DEV, X, Y, Z));
>     //                    ^^^^^^^^^^^
>     //                    That mismatches the 3 other changes!
>     +   if (dma_mapping_error(&DEV->dev, addr))
>     +       /* snip */;
>     +   cpu_to_le32(addr)
> 
> Let's remove the leftover cpu_to_le32() for coherency.
> 
> v2: Better changelog.
> 
> Fixes: 6f2b6a3005b2c34c39f207a87667564f64f2f91a
>   # 3c59x: Add dma error checking and recovery
> Cc: "David S. Miller" <davem@davemloft.net>
> Cc: Meelis Roos <mroos@linux.ee>
> Cc: Neil Horman <nhorman@tuxdriver.com>
> Signed-off-by: Sylvain "ythier" Hitier <sylvain.hitier@gmail.com>
> ---
>  drivers/net/ethernet/3com/3c59x.c |    2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/drivers/net/ethernet/3com/3c59x.c b/drivers/net/ethernet/3com/3c59x.c
> index 8ca49f04..0a3108b3 100644
> --- a/drivers/net/ethernet/3com/3c59x.c
> +++ b/drivers/net/ethernet/3com/3c59x.c
> @@ -2214,7 +2214,7 @@ boomerang_start_xmit(struct sk_buff *skb, struct net_device *dev)
>  		}
>  	}
>  #else
> -	dma_addr = cpu_to_le32(pci_map_single(VORTEX_PCI(vp), skb->data, skb->len, PCI_DMA_TODEVICE));
> +	dma_addr = pci_map_single(VORTEX_PCI(vp), skb->data, skb->len, PCI_DMA_TODEVICE);
>  	if (dma_mapping_error(&VORTEX_PCI(vp)->dev, dma_addr))
>  		goto out_dma_err;
>  	vp->tx_ring[entry].addr = cpu_to_le32(dma_addr);
> 
> Regards,
> Sylvain "ythier" Hitier
> 
> -- 
> Business is about being busy, not being rich...
> Lived 777 days in a Debian package => http://en.wikipedia.org/wiki/Apt,_Vaucluse
> There's THE room for ideals in this mechanical place!
> 
Acked-by: Neil Horman <nhorman@tuxdriver.com>
Thanks!
Neil

Re: [PATCH v2] 3c59x: fix bad split of cpu_to_le32(pci_map_single())

[David Miller]

Can someone please clean this up and submit it properly?

Thank you.

Re: [PATCH v2] 3c59x: fix bad split of cpu_to_le32(pci_map_single())

[Sylvain 'ythier' Hitier]

When: 2014-09-29_1@15-37-48 -0400
Who:  David Miller
What:
> Can someone please clean this up and submit it properly?

I'm willing to do it, but...
could you elaborate on "clean up" and "properly"?

Would it be OK for me re-sending my part as v3 including Neil's ack?
Or are you expecting a unique patch combining Neil's changes and mine?

Regards,
Sylvain "ythier" Hitier

-- 
Business is about being busy, not being rich...
Lived 777 days in a Debian package => http://en.wikipedia.org/wiki/Apt,_Vaucluse
There's THE room for ideals in this mechanical place!

Re: [PATCH v2] 3c59x: fix bad split of cpu_to_le32(pci_map_single())

[David Miller]

From: Sylvain 'ythier' Hitier <sylvain.hitier@gmail.com>
Date: Mon, 29 Sep 2014 19:45:12 +0000

> When: 2014-09-29_1@15-37-48 -0400
> Who:  David Miller
> What:
>> Can someone please clean this up and submit it properly?
> 
> I'm willing to do it, but...
> could you elaborate on "clean up" and "properly"?
> 
> Would it be OK for me re-sending my part as v3 including Neil's ack?
> Or are you expecting a unique patch combining Neil's changes and mine?

You should not submit patches as a reply to a discussion, it must be
done as a fresh mailing list posting.

The posting must also be properly formed, in that "Subject: " should
be of the form "[PATCH vx] ${subsystem}: Description."  and the mail
message body must be the commit message with appropriate signoffs
and ACKs, then the patch comes next.

This is fully described, in detail, in Documentation/SubmittingPatches

Re: [PATCH v2] 3c59x: fix bad split of cpu_to_le32(pci_map_single())

[Sylvain 'ythier' Hitier]

When: 2014-09-29_1@16-08-13 -0400
Who:  David Miller
What:
> From: Sylvain 'ythier' Hitier <sylvain.hitier@gmail.com>
> Date: Mon, 29 Sep 2014 19:45:12 +0000
> 
> You should not submit patches as a reply to a discussion, it must be
> done as a fresh mailing list posting.

Ah, didn't know.
I've seen this be done before, such as:
    http://thread.gmane.org/gmane.linux.kernel/1759680/focus=1775068
and hence did the same.

And searching case-insensitively "reply", "fresh", "new", "thread" in
Documentation/SubmittingPatches shed no light on this topic.

> The posting must also be properly formed, in that "Subject: " should
> be of the form "[PATCH vx] ${subsystem}: Description."  and the mail
> message body must be the commit message with appropriate signoffs
> and ACKs, then the patch comes next.
> 
> This is fully described, in detail, in Documentation/SubmittingPatches

Ah, OK, you meant:
    Please use git format-patch so that I could use git am.

Did that.

(Thanks, you made me learn about git format-patch by myself.
That is, only describing the format makes you a
teacher-who-let-the-padawan-find-the-right-tool-by-self-work instead of a
teacher-who-gives-the-solution.)

> > Would it be OK for me re-sending my part as v3 including Neil's ack?
> > Or are you expecting a unique patch combining Neil's changes and mine?

I went for a v3.


Regards,
Sylvain "ythier" Hitier

-- 
Business is about being busy, not being rich...
Lived 777 days in a Debian package => http://en.wikipedia.org/wiki/Apt,_Vaucluse
There's THE room for ideals in this mechanical place!