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

PATCH Superseded Sylvain 'ythier' Hitier 3 participants 9 messages Sep 24, 2014 – Sep 29, 2014

Subsystems (2)

Network drivers 3C59X network driver

lore

Share & tools

lore

Sylvain 'ythier' Hitier

author

2014-09-24 09:30 UTC #0 lore raw

PATCH 1/1 3c59x: fix bad split of cpu_to_le32(pci_map_single())

Author: Sylvain "ythier" Hitier <[email protected]>
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 <[email protected]>

drivers/net/ethernet/3com/3c59x.c +1 -1

index 8ca49f04..0a3108b3 100644
@@ -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

Signature · 3 lines

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!

Neil Horman

NAK

2014-09-24 10:14 UTC #1 lore raw

Quoted 28 lines · Sylvain 'ythier' Hitier

Author: Sylvain "ythier" Hitier <[email protected]>
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 <[email protected]>
drivers/net/ethernet/3com/3c59x.c +1 -1
index 8ca49f04..0a3108b3 100644
@@ -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);

Quoted 3 lines

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

Quoted 5 lines

Signature · 3 lines

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!

Neil Horman

2014-09-24 10:16 UTC #2 lore raw

In reply to Sylvain 'ythier' Hitier

Quoted 18 lines · Sylvain 'ythier' Hitier

Author: Sylvain "ythier" Hitier <[email protected]>
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 <[email protected]>
drivers/net/ethernet/3com/3c59x.c +0 -0
index 8ca49f04..0a3108b3 100644

drivers/net/ethernet/3com/3c59x.c boomerang_start_xmit(struct sk_buff *skb, struct net_device *dev) · line 2220

Quoted 8 lines

drivers/net/ethernet/3com/3c59x.c +1 -1

@@ -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

Quoted 10 lines

Regards,
Sylvain "ythier" Hitier

Signature · 3 lines

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!

Sylvain 'ythier' Hitier

author

2014-09-25 15:15 UTC #3 lore raw

PATCH v2 1/1 3c59x: fix bad split of cpu_to_le32(pci_map_single())

Hello,

When: 2014-09-24_3@06-16-47 -0400
Who:  Neil Horman

What:

Quoted 2 lines

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" <[email protected]>

Cc Meelis Roos <[email protected]>

Cc Neil Horman <[email protected]>

Signed-off-by Sylvain "ythier" Hitier <[email protected]>

---
 drivers/net/ethernet/3com/3c59x.c |    2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

drivers/net/ethernet/3com/3c59x.c +1 -1

index 8ca49f04..0a3108b3 100644
@@ -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

Signature · 3 lines

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!

Neil Horman

Acked-by on v2 1/1

2014-09-25 16:02 UTC #4 lore raw

Quoted 69 lines · Sylvain 'ythier' Hitier

Hello,
When: 2014-09-24_3@06-16-47 -0400
Who:  Neil Horman
What:
Quoted 2 lines
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" <[email protected]>
Cc Meelis Roos <[email protected]>
Cc Neil Horman <[email protected]>
Signed-off-by Sylvain "ythier" Hitier <[email protected]>
---
 drivers/net/ethernet/3com/3c59x.c |    2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)
drivers/net/ethernet/3com/3c59x.c +1 -1
index 8ca49f04..0a3108b3 100644
@@ -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);

Quoted 8 lines

Regards,
Sylvain "ythier" Hitier

Signature · 3 lines

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 <[email protected]>

Thanks!
Neil

David Miller

maintainer

2014-09-29 19:37 UTC #5 lore raw

In reply to Sylvain 'ythier' Hitier

Can someone please clean this up and submit it properly?

Thank you.

Sylvain 'ythier' Hitier

author

2014-09-29 19:45 UTC #6 lore raw

When: 2014-09-29_1@15-37-48 -0400
Who:  David Miller

What:

Quoted 1 line

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

Signature · 3 lines

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!

David Miller

maintainer

2014-09-29 20:08 UTC #7 lore raw

From: Sylvain 'ythier' Hitier <[email protected]>
Date: Mon, 29 Sep 2014 19:45:12 +0000

Quoted 10 lines

When: 2014-09-29_1@15-37-48 -0400
Who:  David Miller
What:
Quoted 1 line
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

Sylvain 'ythier' Hitier

author

2014-09-29 21:27 UTC #8 lore raw

When: 2014-09-29_1@16-08-13 -0400
Who:  David Miller

What:

Quoted 5 lines

From: Sylvain 'ythier' Hitier <[email protected]>
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.

Quoted 6 lines

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.)

Quoted 2 lines

Quoted 2 lines
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

Signature · 3 lines

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!