| Message ID | 20091214103443.13082.84079.sendpatchset@rxone.opensource.se |
|---|---|
| State | Changes Requested, archived |
| Delegated to: | David Miller |
| Headers | show |
From: Magnus Damm <magnus.damm@gmail.com> Date: Mon, 14 Dec 2009 19:34:43 +0900 > From: Magnus Damm <damm@opensource.se> > > Fix sh_eth for sh7724 by adding NET_IP_ALIGN support. > Without this patch the receive data is misaligned. > > Signed-off-by: Magnus Damm <damm@opensource.se> How does this interact with sh_eth_set_receive_align() and why isn't all of the skb_reserve() logic confined to one place? The sh7763 sh_eth_my_cpu_data sets rpadir unconditionally, yet your NET_IP_ALIGN behavior is controlled by the new setting of .rpadir for sh7724, is this ok or is it going to break sh7763 or cause it to misbehave? Could you possibly generalize this so that you don't need the ifdefs and it doesn't matter what the NET_IP_ALIGN value actually is? That is making this code more confusing than it needs to be. Thanks. -- 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
On Tue, Dec 15, 2009 at 2:57 PM, David Miller <davem@davemloft.net> wrote: > From: Magnus Damm <magnus.damm@gmail.com> > Date: Mon, 14 Dec 2009 19:34:43 +0900 > >> From: Magnus Damm <damm@opensource.se> >> >> Fix sh_eth for sh7724 by adding NET_IP_ALIGN support. >> Without this patch the receive data is misaligned. >> >> Signed-off-by: Magnus Damm <damm@opensource.se> > > How does this interact with sh_eth_set_receive_align() and > why isn't all of the skb_reserve() logic confined to one > place? It looks like the driver is written to support a few different processors, and the alignment requirement varies from processor to processor. I assume that the mdp->rx_buf_sz calculation takes care of the alignment, but it all looks pretty messy to me. Some processors support RPADIR and some do not. The processors that do support RPADIR can tie in NET_IP_ALIGN support easily. The sh7763 hardware should support RPADIR, but only a partial software implementation seems to be in place without this patch. And I can't unfortunately test sh7763 support since I only have sh7724 hardware available. That aside, unifying the alignment code and doing a major cleanup would be nice, yes. > The sh7763 sh_eth_my_cpu_data sets rpadir unconditionally, > yet your NET_IP_ALIGN behavior is controlled by the new > setting of .rpadir for sh7724, is this ok or is it going > to break sh7763 or cause it to misbehave? Good point. It may be better to remove the .rpadir setting from sh7763 since it looks unused and broken at this point. OTOH, if RPADIR on sh7763 does what I suspect then this patch will actually unbreak sh7763 using the skb_reserve() hunk. > Could you possibly generalize this so that you don't > need the ifdefs and it doesn't matter what the NET_IP_ALIGN > value actually is? That is making this code more confusing > than it needs to be. Sorry about the confusion, but the #ifdef was actually intentional. =) I can't find any description of the RPADIR register in the sh7724 data sheet, but testing shows that the buffers are adjusted properly. So since I don't know the register layout I prefer to optimize for the known working combination of NET_IP_ALIGN == 2 and simply disable .rpadir usage for other NET_IP_ALIGN cases. Thanks! / magnus -- 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
From: Magnus Damm <magnus.damm@gmail.com> Date: Tue, 15 Dec 2009 16:13:47 +0900 > Sorry about the confusion, but the #ifdef was actually intentional. =) > I can't find any description of the RPADIR register in the sh7724 data > sheet, but testing shows that the buffers are adjusted properly. So > since I don't know the register layout I prefer to optimize for the > known working combination of NET_IP_ALIGN == 2 and simply disable > .rpadir usage for other NET_IP_ALIGN cases. NET_IP_ALIGN is never != 2 on SUPERH SH_ETH depends on SUPERH It's a fantasy ifdef, of sorts. :-) -- 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
On Tue, Dec 15, 2009 at 4:15 PM, David Miller <davem@davemloft.net> wrote: > NET_IP_ALIGN is never != 2 on SUPERH > > SH_ETH depends on SUPERH > > It's a fantasy ifdef, of sorts. :-) Sure, but _maybe_ someone will put the sh_eth hardware block together with MIPS or ARM in the future. I guess it makes more sense to just use the NET_IP_ALIGN value blindly and not care so much about what is known working and what is not...? Cheers, / magnus -- 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
On Tue, Dec 15, 2009 at 4:22 PM, Magnus Damm <magnus.damm@gmail.com> wrote: > On Tue, Dec 15, 2009 at 4:15 PM, David Miller <davem@davemloft.net> wrote: >> NET_IP_ALIGN is never != 2 on SUPERH >> >> SH_ETH depends on SUPERH >> >> It's a fantasy ifdef, of sorts. :-) > > Sure, but _maybe_ someone will put the sh_eth hardware block together > with MIPS or ARM in the future. I guess it makes more sense to just > use the NET_IP_ALIGN value blindly and not care so much about what is > known working and what is not...? Well, it's in the sh7724 specific portion of the code and I guess that's SUPERH. =) / magnus -- 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
From: Magnus Damm <magnus.damm@gmail.com> Date: Tue, 15 Dec 2009 16:22:11 +0900 > On Tue, Dec 15, 2009 at 4:15 PM, David Miller <davem@davemloft.net> wrote: >> NET_IP_ALIGN is never != 2 on SUPERH >> >> SH_ETH depends on SUPERH >> >> It's a fantasy ifdef, of sorts. :-) > > Sure, but _maybe_ someone will put the sh_eth hardware block together > with MIPS or ARM in the future. MIPS and ARM use the default NET_IP_ALIGN of 2 as well. Next? -- 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
On Tue, Dec 15, 2009 at 4:34 PM, David Miller <davem@davemloft.net> wrote: > From: Magnus Damm <magnus.damm@gmail.com> > Date: Tue, 15 Dec 2009 16:22:11 +0900 > >> On Tue, Dec 15, 2009 at 4:15 PM, David Miller <davem@davemloft.net> wrote: >>> NET_IP_ALIGN is never != 2 on SUPERH >>> >>> SH_ETH depends on SUPERH >>> >>> It's a fantasy ifdef, of sorts. :-) >> >> Sure, but _maybe_ someone will put the sh_eth hardware block together >> with MIPS or ARM in the future. > > MIPS and ARM use the default NET_IP_ALIGN of 2 as well. > > Next? I'll just get rid of the #ifdef and make a V2. Thanks! =) / magnus -- 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
--- 0001/drivers/net/sh_eth.c +++ work/drivers/net/sh_eth.c 2009-12-14 15:27:25.000000000 +0900 @@ -84,6 +84,10 @@ static struct sh_eth_cpu_data sh_eth_my_ .mpr = 1, .tpauser = 1, .hw_swap = 1, +#if NET_IP_ALIGN == 2 + .rpadir = 1, + .rpadir_value = 0x00020000, +#endif }; #elif defined(CONFIG_CPU_SUBTYPE_SH7763) @@ -501,6 +505,8 @@ static int sh_eth_ring_init(struct net_d */ mdp->rx_buf_sz = (ndev->mtu <= 1492 ? PKT_BUF_SZ : (((ndev->mtu + 26 + 7) & ~7) + 2 + 16)); + if (mdp->cd->rpadir) + mdp->rx_buf_sz += NET_IP_ALIGN; /* Allocate RX and TX skb rings */ mdp->rx_skbuff = kmalloc(sizeof(*mdp->rx_skbuff) * RX_RING_SIZE, @@ -715,6 +721,8 @@ static int sh_eth_rx(struct net_device * pkt_len + 2); skb = mdp->rx_skbuff[entry]; mdp->rx_skbuff[entry] = NULL; + if (mdp->cd->rpadir) + skb_reserve(skb, NET_IP_ALIGN); skb_put(skb, pkt_len); skb->protocol = eth_type_trans(skb, ndev); netif_rx(skb);