| Message ID | 20171204133327.3505-2-thomas.petazzoni@free-electrons.com (mailing list archive) |
|---|---|
| State | Accepted |
| Delegated to: | Geert Uytterhoeven |
| Headers | show |
On 12/04/2017 04:33 PM, Thomas Petazzoni wrote: > There are two types of "struct device": the one representing the > physical device on its physical bus (platform, SPI, PCI, etc.), and > the one representing the logical device in its device class (net, > etc.). > > The DMA mapping API expects to receive as argument a "struct device" > representing the physical device, as the "struct device" contains > information about the bus that the DMA API needs. > > However, the sh_eth driver mistakenly uses the "struct device" > representing the logical device (embedded in "struct net_device") > rather than the "struct device" representing the physical device on > its bus. > > This commit fixes that by adjusting all calls to the DMA mapping API. > > Signed-off-by: Thomas Petazzoni <thomas.petazzoni@free-electrons.com> Acked-by: Sergei Shtylyov <sergei.shtylyov@cogentembedded.com> I'm still unsure the ravb driver doesn't have this issue... [...] MBR, Sergei
On 12/04/2017 04:33 PM, Thomas Petazzoni wrote: > There are two types of "struct device": the one representing the > physical device on its physical bus (platform, SPI, PCI, etc.), and > the one representing the logical device in its device class (net, > etc.). > > The DMA mapping API expects to receive as argument a "struct device" > representing the physical device, as the "struct device" contains > information about the bus that the DMA API needs. > > However, the sh_eth driver mistakenly uses the "struct device" > representing the logical device (embedded in "struct net_device") > rather than the "struct device" representing the physical device on > its bus. > > This commit fixes that by adjusting all calls to the DMA mapping API. > > Signed-off-by: Thomas Petazzoni <thomas.petazzoni@free-electrons.com> > --- > drivers/net/ethernet/renesas/sh_eth.c | 19 ++++++++++--------- > 1 file changed, 10 insertions(+), 9 deletions(-) > > diff --git a/drivers/net/ethernet/renesas/sh_eth.c b/drivers/net/ethernet/renesas/sh_eth.c > index 7e060aa9fbed..91e918e654fe 100644 > --- a/drivers/net/ethernet/renesas/sh_eth.c > +++ b/drivers/net/ethernet/renesas/sh_eth.c > @@ -1149,7 +1149,8 @@ static int sh_eth_tx_free(struct net_device *ndev, bool sent_only) > entry, le32_to_cpu(txdesc->status)); > /* Free the original skb. */ > if (mdp->tx_skbuff[entry]) { > - dma_unmap_single(&ndev->dev, le32_to_cpu(txdesc->addr), > + dma_unmap_single(&mdp->pdev->dev, Using 'ndev->dev.parent' (as in ravb) also should work... not sure which is better (although I'm seeing very strange things in the ravb driver built with AArch654 gcc 4.8.5). [...] MBR, Sergei
On Mon, Dec 4, 2017 at 2:33 PM, Thomas Petazzoni <thomas.petazzoni@free-electrons.com> wrote: > There are two types of "struct device": the one representing the > physical device on its physical bus (platform, SPI, PCI, etc.), and > the one representing the logical device in its device class (net, > etc.). > > The DMA mapping API expects to receive as argument a "struct device" > representing the physical device, as the "struct device" contains > information about the bus that the DMA API needs. > > However, the sh_eth driver mistakenly uses the "struct device" > representing the logical device (embedded in "struct net_device") > rather than the "struct device" representing the physical device on > its bus. > > This commit fixes that by adjusting all calls to the DMA mapping API. > > Signed-off-by: Thomas Petazzoni <thomas.petazzoni@free-electrons.com> Reviewed-by: Geert Uytterhoeven <geert+renesas@glider.be> Gr{oetje,eeting}s, Geert -- Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@linux-m68k.org In personal conversations with technical people, I call myself a hacker. But when I'm talking to journalists I just say "programmer" or something like that. -- Linus Torvalds
Hi Sergei, On Mon, Dec 4, 2017 at 9:05 PM, Sergei Shtylyov <sergei.shtylyov@cogentembedded.com> wrote: > On 12/04/2017 04:33 PM, Thomas Petazzoni wrote: >> There are two types of "struct device": the one representing the >> physical device on its physical bus (platform, SPI, PCI, etc.), and >> the one representing the logical device in its device class (net, >> etc.). >> >> The DMA mapping API expects to receive as argument a "struct device" >> representing the physical device, as the "struct device" contains >> information about the bus that the DMA API needs. >> >> However, the sh_eth driver mistakenly uses the "struct device" >> representing the logical device (embedded in "struct net_device") >> rather than the "struct device" representing the physical device on >> its bus. >> >> This commit fixes that by adjusting all calls to the DMA mapping API. >> >> Signed-off-by: Thomas Petazzoni <thomas.petazzoni@free-electrons.com> >> --- >> drivers/net/ethernet/renesas/sh_eth.c | 19 ++++++++++--------- >> 1 file changed, 10 insertions(+), 9 deletions(-) >> >> diff --git a/drivers/net/ethernet/renesas/sh_eth.c >> b/drivers/net/ethernet/renesas/sh_eth.c >> index 7e060aa9fbed..91e918e654fe 100644 >> --- a/drivers/net/ethernet/renesas/sh_eth.c >> +++ b/drivers/net/ethernet/renesas/sh_eth.c >> @@ -1149,7 +1149,8 @@ static int sh_eth_tx_free(struct net_device *ndev, >> bool sent_only) >> entry, le32_to_cpu(txdesc->status)); >> /* Free the original skb. */ >> if (mdp->tx_skbuff[entry]) { >> - dma_unmap_single(&ndev->dev, >> le32_to_cpu(txdesc->addr), >> + dma_unmap_single(&mdp->pdev->dev, > > > Using 'ndev->dev.parent' (as in ravb) also should work... not sure which > is better That was going to be my comment, too. I also haven't checked which generates the smallest code. > (although I'm seeing very strange things in the ravb driver built > with AArch654 gcc 4.8.5). Don't worry, you've just been bitten by the %p pointer hashing :-( Use %px instead. Gr{oetje,eeting}s, Geert -- Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@linux-m68k.org In personal conversations with technical people, I call myself a hacker. But when I'm talking to journalists I just say "programmer" or something like that. -- Linus Torvalds
Hello, On Tue, 5 Dec 2017 09:39:35 +0100, Geert Uytterhoeven wrote: > > Using 'ndev->dev.parent' (as in ravb) also should work... not sure which > > is better > > That was going to be my comment, too. I also haven't checked which > generates the smallest code. Using ndev->dev.parent actually generates bigger code (44 bytes larger) : $ size drivers/net/ethernet/renesas/sh_eth.o.* text data bss dec hex filename 27803 696 0 28499 6f53 drivers/net/ethernet/renesas/sh_eth.o.new 27759 696 0 28455 6f27 drivers/net/ethernet/renesas/sh_eth.o.orig .orig is my original proposal, .new is with ndev->dev.parent. I'm using a gcc 6.4.0 compiler. Note also that the driver is already using mdp->pdev->dev all over the place: $ grep -- mdp-\>pdev-\>dev drivers/net/ethernet/renesas/sh_eth.c pm_wakeup_event(&mdp->pdev->dev, 0); pm_runtime_get_sync(&mdp->pdev->dev); pm_runtime_put_sync(&mdp->pdev->dev); device_set_wakeup_enable(&mdp->pdev->dev, mdp->wol_enabled); pm_runtime_get_sync(&mdp->pdev->dev); pm_runtime_put_sync(&mdp->pdev->dev); pm_runtime_put_sync(&mdp->pdev->dev); struct device *dev = &mdp->pdev->dev; Best regards, Thomas Petazzoni
Hi Thomas, On Tue, Dec 5, 2017 at 10:38 AM, Thomas Petazzoni <thomas.petazzoni@free-electrons.com> wrote: > On Tue, 5 Dec 2017 09:39:35 +0100, Geert Uytterhoeven wrote: >> > Using 'ndev->dev.parent' (as in ravb) also should work... not sure which >> > is better >> >> That was going to be my comment, too. I also haven't checked which >> generates the smallest code. > > Using ndev->dev.parent actually generates bigger code (44 bytes larger) : Thanks a lot for checking! Gr{oetje,eeting}s, Geert -- Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@linux-m68k.org In personal conversations with technical people, I call myself a hacker. But when I'm talking to journalists I just say "programmer" or something like that. -- Linus Torvalds
diff --git a/drivers/net/ethernet/renesas/sh_eth.c b/drivers/net/ethernet/renesas/sh_eth.c index 7e060aa9fbed..91e918e654fe 100644 --- a/drivers/net/ethernet/renesas/sh_eth.c +++ b/drivers/net/ethernet/renesas/sh_eth.c @@ -1149,7 +1149,8 @@ static int sh_eth_tx_free(struct net_device *ndev, bool sent_only) entry, le32_to_cpu(txdesc->status)); /* Free the original skb. */ if (mdp->tx_skbuff[entry]) { - dma_unmap_single(&ndev->dev, le32_to_cpu(txdesc->addr), + dma_unmap_single(&mdp->pdev->dev, + le32_to_cpu(txdesc->addr), le32_to_cpu(txdesc->len) >> 16, DMA_TO_DEVICE); dev_kfree_skb_irq(mdp->tx_skbuff[entry]); @@ -1179,7 +1180,7 @@ static void sh_eth_ring_free(struct net_device *ndev) if (mdp->rx_skbuff[i]) { struct sh_eth_rxdesc *rxdesc = &mdp->rx_ring[i]; - dma_unmap_single(&ndev->dev, + dma_unmap_single(&mdp->pdev->dev, le32_to_cpu(rxdesc->addr), ALIGN(mdp->rx_buf_sz, 32), DMA_FROM_DEVICE); @@ -1245,9 +1246,9 @@ static void sh_eth_ring_format(struct net_device *ndev) /* The size of the buffer is a multiple of 32 bytes. */ buf_len = ALIGN(mdp->rx_buf_sz, 32); - dma_addr = dma_map_single(&ndev->dev, skb->data, buf_len, + dma_addr = dma_map_single(&mdp->pdev->dev, skb->data, buf_len, DMA_FROM_DEVICE); - if (dma_mapping_error(&ndev->dev, dma_addr)) { + if (dma_mapping_error(&mdp->pdev->dev, dma_addr)) { kfree_skb(skb); break; } @@ -1527,7 +1528,7 @@ static int sh_eth_rx(struct net_device *ndev, u32 intr_status, int *quota) mdp->rx_skbuff[entry] = NULL; if (mdp->cd->rpadir) skb_reserve(skb, NET_IP_ALIGN); - dma_unmap_single(&ndev->dev, dma_addr, + dma_unmap_single(&mdp->pdev->dev, dma_addr, ALIGN(mdp->rx_buf_sz, 32), DMA_FROM_DEVICE); skb_put(skb, pkt_len); @@ -1555,9 +1556,9 @@ static int sh_eth_rx(struct net_device *ndev, u32 intr_status, int *quota) if (skb == NULL) break; /* Better luck next round. */ sh_eth_set_receive_align(skb); - dma_addr = dma_map_single(&ndev->dev, skb->data, + dma_addr = dma_map_single(&mdp->pdev->dev, skb->data, buf_len, DMA_FROM_DEVICE); - if (dma_mapping_error(&ndev->dev, dma_addr)) { + if (dma_mapping_error(&mdp->pdev->dev, dma_addr)) { kfree_skb(skb); break; } @@ -2441,9 +2442,9 @@ static int sh_eth_start_xmit(struct sk_buff *skb, struct net_device *ndev) /* soft swap. */ if (!mdp->cd->hw_swap) sh_eth_soft_swap(PTR_ALIGN(skb->data, 4), skb->len + 2); - dma_addr = dma_map_single(&ndev->dev, skb->data, skb->len, + dma_addr = dma_map_single(&mdp->pdev->dev, skb->data, skb->len, DMA_TO_DEVICE); - if (dma_mapping_error(&ndev->dev, dma_addr)) { + if (dma_mapping_error(&mdp->pdev->dev, dma_addr)) { kfree_skb(skb); return NETDEV_TX_OK; }
There are two types of "struct device": the one representing the physical device on its physical bus (platform, SPI, PCI, etc.), and the one representing the logical device in its device class (net, etc.). The DMA mapping API expects to receive as argument a "struct device" representing the physical device, as the "struct device" contains information about the bus that the DMA API needs. However, the sh_eth driver mistakenly uses the "struct device" representing the logical device (embedded in "struct net_device") rather than the "struct device" representing the physical device on its bus. This commit fixes that by adjusting all calls to the DMA mapping API. Signed-off-by: Thomas Petazzoni <thomas.petazzoni@free-electrons.com> --- drivers/net/ethernet/renesas/sh_eth.c | 19 ++++++++++--------- 1 file changed, 10 insertions(+), 9 deletions(-)