From: Sergei Shtylyov <sergei.shtylyov@cogentembedded.com>
To: Yoshihiro Kaneko <ykaneko0929@gmail.com>, netdev@vger.kernel.org
Cc: "David S. Miller" <davem@davemloft.net>,
Simon Horman <horms@verge.net.au>,
Magnus Damm <magnus.damm@gmail.com>,
linux-sh@vger.kernel.org
Subject: Re: [PATCH 2/3] sh_eth: Fix skb alloc size and alignment adjust rule.
Date: Thu, 13 Nov 2014 22:48:39 +0000 [thread overview]
Message-ID: <54653547.9050207@cogentembedded.com> (raw)
In-Reply-To: <1415862301-28032-3-git-send-email-ykaneko0929@gmail.com>
On 11/13/2014 10:05 AM, Yoshihiro Kaneko wrote:
> From: Mitsuhiro Kimura <mitsuhiro.kimura.kc@renesas.com>
> In the current driver, allocation size of skb does not care the alignment
> adjust after allocation.
> And also, in the current implementation, buffer alignment method by
> sh_eth_set_receive_align function has a bug that this function displace
> buffer start address forcedly when the alignment is corrected.
> In the result, tail of the skb will exceed allocated area and kernel panic
> will be occurred.
Oh, have never seen panic but Geert has reported WARNINGs from the DMA
debug code...
> This patch fix this issue.
> Signed-off-by: Mitsuhiro Kimura <mitsuhiro.kimura.kc@renesas.com>
> Signed-off-by: Yoshihiro Kaneko <ykaneko0929@gmail.com>
> ---
> drivers/net/ethernet/renesas/sh_eth.c | 35 ++++++++++++++---------------------
> drivers/net/ethernet/renesas/sh_eth.h | 2 ++
> 2 files changed, 16 insertions(+), 21 deletions(-)
> diff --git a/drivers/net/ethernet/renesas/sh_eth.c b/drivers/net/ethernet/renesas/sh_eth.c
> index 49e963e..0e4a407 100644
> --- a/drivers/net/ethernet/renesas/sh_eth.c
> +++ b/drivers/net/ethernet/renesas/sh_eth.c
> @@ -917,21 +917,12 @@ static int sh_eth_reset(struct net_device *ndev)
> return ret;
> }
>
> -#if defined(CONFIG_CPU_SH4) || defined(CONFIG_ARCH_SHMOBILE)
> static void sh_eth_set_receive_align(struct sk_buff *skb)
> {
> - int reserve;
> -
> - reserve = SH4_SKB_RX_ALIGN - ((u32)skb->data & (SH4_SKB_RX_ALIGN - 1));
> + u32 reserve = (u32)skb->data & (SH_ETH_RX_ALIGN - 1);
Please keep an empty line after declaration, as it was before this patch.
> if (reserve)
> - skb_reserve(skb, reserve);
> -}
> -#else
> -static void sh_eth_set_receive_align(struct sk_buff *skb)
> -{
> - skb_reserve(skb, SH2_SH3_SKB_RX_ALIGN);
> + skb_reserve(skb, SH_ETH_RX_ALIGN - reserve);
> }
> -#endif
>
>
> /* CPU <-> EDMAC endian convert */
> @@ -1119,6 +1110,7 @@ static void sh_eth_ring_format(struct net_device *ndev)
> struct sh_eth_txdesc *txdesc = NULL;
> int rx_ringsize = sizeof(*rxdesc) * mdp->num_rx_ring;
> int tx_ringsize = sizeof(*txdesc) * mdp->num_tx_ring;
> + int skbuff_size = mdp->rx_buf_sz + SH_ETH_RX_ALIGN - 1;
>
> mdp->cur_rx = 0;
> mdp->cur_tx = 0;
> @@ -1131,21 +1123,21 @@ static void sh_eth_ring_format(struct net_device *ndev)
> for (i = 0; i < mdp->num_rx_ring; i++) {
> /* skb */
> mdp->rx_skbuff[i] = NULL;
> - skb = netdev_alloc_skb(ndev, mdp->rx_buf_sz);
> + skb = netdev_alloc_skb(ndev, skbuff_size);
> mdp->rx_skbuff[i] = skb;
> if (skb = NULL)
> break;
> - dma_map_single(&ndev->dev, skb->data, mdp->rx_buf_sz,
> - DMA_FROM_DEVICE);
> sh_eth_set_receive_align(skb);
>
> /* RX descriptor */
> rxdesc = &mdp->rx_ring[i];
> + /* The size of the buffer is 16 byte boundary. */
Is *on* 16 byte boundary, you mean?
> + rxdesc->buffer_length = ALIGN(mdp->rx_buf_sz, 16);
> + dma_map_single(&ndev->dev, skb->data, rxdesc->buffer_length,
> + DMA_FROM_DEVICE);
> rxdesc->addr = virt_to_phys(skb->data);
> rxdesc->status = cpu_to_edmac(mdp, RD_RACT | RD_RFP);
>
> - /* The size of the buffer is 16 byte boundary. */
Ah, you're just copying an existent comment... well, seems a good time to
fix it then. :-)
[...]
> @@ -1448,8 +1441,8 @@ static int sh_eth_rx(struct net_device *ndev, u32 intr_status, int *quota)
> if (mdp->cd->rpadir)
> skb_reserve(skb, NET_IP_ALIGN);
> dma_sync_single_for_cpu(&ndev->dev, rxdesc->addr,
> - mdp->rx_buf_sz,
> - DMA_FROM_DEVICE);
> + ALIGN(mdp->rx_buf_sz, 16),
> + DMA_FROM_DEVICE);
Please keep the original alignment of the continuation lines.
> skb_put(skb, pkt_len);
> skb->protocol = eth_type_trans(skb, ndev);
> netif_receive_skb(skb);
> @@ -1468,13 +1461,13 @@ static int sh_eth_rx(struct net_device *ndev, u32 intr_status, int *quota)
> rxdesc->buffer_length = ALIGN(mdp->rx_buf_sz, 16);
>
> if (mdp->rx_skbuff[entry] = NULL) {
> - skb = netdev_alloc_skb(ndev, mdp->rx_buf_sz);
> + skb = netdev_alloc_skb(ndev, skbuff_size);
> mdp->rx_skbuff[entry] = skb;
> if (skb = NULL)
> break; /* Better luck next round. */
> - dma_map_single(&ndev->dev, skb->data, mdp->rx_buf_sz,
> - DMA_FROM_DEVICE);
> sh_eth_set_receive_align(skb);
> + dma_map_single(&ndev->dev, skb->data,
> + rxdesc->buffer_length, DMA_FROM_DEVICE);
>
> skb_checksum_none_assert(skb);
> rxdesc->addr = virt_to_phys(skb->data);
> diff --git a/drivers/net/ethernet/renesas/sh_eth.h b/drivers/net/ethernet/renesas/sh_eth.h
> index b37c427..d138ebe 100644
> --- a/drivers/net/ethernet/renesas/sh_eth.h
> +++ b/drivers/net/ethernet/renesas/sh_eth.h
> @@ -163,8 +163,10 @@ enum {
> /* Driver's parameters */
> #if defined(CONFIG_CPU_SH4) || defined(CONFIG_ARCH_SHMOBILE)
> #define SH4_SKB_RX_ALIGN 32
> +#define SH_ETH_RX_ALIGN (SH4_SKB_RX_ALIGN)
() not needed.
> #else
> #define SH2_SH3_SKB_RX_ALIGN 2
> +#define SH_ETH_RX_ALIGN (SH2_SH3_SKB_RX_ALIGN)
Likewise.
And I don't think we still need {SH2_SH3|SH4}_SKB_RX_ALIGN after this patch.
[...]
WBR, Sergei
next prev parent reply other threads:[~2014-11-13 22:48 UTC|newest]
Thread overview: 11+ messages / expand[flat|nested] mbox.gz Atom feed top
2014-11-13 7:04 [PATCH 0/3] sh_eth: Remove redundant alignment adjustment Yoshihiro Kaneko
2014-11-13 7:04 ` [PATCH 1/3] " Yoshihiro Kaneko
2014-11-13 22:37 ` Sergei Shtylyov
2014-11-14 0:43 ` Simon Horman
2014-11-13 7:05 ` [PATCH 2/3] sh_eth: Fix skb alloc size and alignment adjust rule Yoshihiro Kaneko
2014-11-13 22:48 ` Sergei Shtylyov [this message]
2014-11-13 7:05 ` [PATCH 3/3] sh_eth: Fix dma mapping issue Yoshihiro Kaneko
2014-11-13 23:05 ` Sergei Shtylyov
2014-11-17 4:09 ` Simon Horman
2014-11-25 20:07 ` Sergei Shtylyov
2014-11-13 9:04 ` [PATCH 0/3] sh_eth: Remove redundant alignment adjustment Geert Uytterhoeven
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=54653547.9050207@cogentembedded.com \
--to=sergei.shtylyov@cogentembedded.com \
--cc=davem@davemloft.net \
--cc=horms@verge.net.au \
--cc=linux-sh@vger.kernel.org \
--cc=magnus.damm@gmail.com \
--cc=netdev@vger.kernel.org \
--cc=ykaneko0929@gmail.com \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).