| Message ID | 1349374497-9540-11-git-send-email-monstr@monstr.eu |
|---|---|
| State | Not Applicable, archived |
| Delegated to: | David Miller |
| Headers | show |
On Thu, 2012-10-04 at 20:14 +0200, Michal Simek wrote: > Just show current setting in bootlog. [...] > --- a/drivers/net/ethernet/xilinx/ll_temac_main.c > +++ b/drivers/net/ethernet/xilinx/ll_temac_main.c > @@ -1052,12 +1052,14 @@ static int __devinit temac_of_probe(struct platform_device *op) > /* Setup checksum offload, but default to off if not specified */ > lp->temac_features = 0; > p = (__be32 *)of_get_property(op->dev.of_node, "xlnx,txcsum", NULL); > + dev_info(&op->dev, "TX_CSUM %d\n", be32_to_cpup(p)); > if (p && be32_to_cpu(*p)) { > lp->temac_features |= TEMAC_FEATURE_TX_CSUM; > /* Can checksum TCP/UDP over IPv4. */ > ndev->features |= NETIF_F_IP_CSUM; > } > p = (__be32 *)of_get_property(op->dev.of_node, "xlnx,rxcsum", NULL); > + dev_info(&op->dev, "RX_CSUM %d\n", be32_to_cpup(p)); [...] Is there any particular reason you think this needs to be logged by default, rather than letting users run ethtool -k? I suggest using dev_dbg() instead. Ben.
On 10/04/2012 09:15 PM, Ben Hutchings wrote: > On Thu, 2012-10-04 at 20:14 +0200, Michal Simek wrote: >> Just show current setting in bootlog. > [...] >> --- a/drivers/net/ethernet/xilinx/ll_temac_main.c >> +++ b/drivers/net/ethernet/xilinx/ll_temac_main.c >> @@ -1052,12 +1052,14 @@ static int __devinit temac_of_probe(struct platform_device *op) >> /* Setup checksum offload, but default to off if not specified */ >> lp->temac_features = 0; >> p = (__be32 *)of_get_property(op->dev.of_node, "xlnx,txcsum", NULL); >> + dev_info(&op->dev, "TX_CSUM %d\n", be32_to_cpup(p)); >> if (p && be32_to_cpu(*p)) { >> lp->temac_features |= TEMAC_FEATURE_TX_CSUM; >> /* Can checksum TCP/UDP over IPv4. */ >> ndev->features |= NETIF_F_IP_CSUM; >> } >> p = (__be32 *)of_get_property(op->dev.of_node, "xlnx,rxcsum", NULL); >> + dev_info(&op->dev, "RX_CSUM %d\n", be32_to_cpup(p)); > [...] > > Is there any particular reason you think this needs to be logged by > default, rather than letting users run ethtool -k? I suggest using > dev_dbg() instead. The reason was just to show it in the bootlog. I will check ethtool support for these drivers. Thanks for your comments, Michal
On 10/04/2012 09:15 PM, Ben Hutchings wrote: > On Thu, 2012-10-04 at 20:14 +0200, Michal Simek wrote: >> Just show current setting in bootlog. > [...] >> --- a/drivers/net/ethernet/xilinx/ll_temac_main.c >> +++ b/drivers/net/ethernet/xilinx/ll_temac_main.c >> @@ -1052,12 +1052,14 @@ static int __devinit temac_of_probe(struct platform_device *op) >> /* Setup checksum offload, but default to off if not specified */ >> lp->temac_features = 0; >> p = (__be32 *)of_get_property(op->dev.of_node, "xlnx,txcsum", NULL); >> + dev_info(&op->dev, "TX_CSUM %d\n", be32_to_cpup(p)); >> if (p && be32_to_cpu(*p)) { >> lp->temac_features |= TEMAC_FEATURE_TX_CSUM; >> /* Can checksum TCP/UDP over IPv4. */ >> ndev->features |= NETIF_F_IP_CSUM; >> } >> p = (__be32 *)of_get_property(op->dev.of_node, "xlnx,rxcsum", NULL); >> + dev_info(&op->dev, "RX_CSUM %d\n", be32_to_cpup(p)); > [...] > > Is there any particular reason you think this needs to be logged by > default, rather than letting users run ethtool -k? I suggest using > dev_dbg() instead. Ok. I have looked at it and there are missing some bits in ndev->features. Can you please check that my setting is correct? It is SG DMA ip/driver. ndev->features = NETIF_F_FRAGLIST | NETIF_F_SG With two options for csum on RX/TX. They can be selected independently. tx Partial csum over IPv4. -> NETIF_F_IP_CSUM tx Full csum. -> NETIF_F_HW_CSUM rx Full csum -> NETIF_F_RXCSUM Is there any option to support partial csum? Thanks, Michal
On Fri, 2012-10-05 at 07:48 +0200, Michal Simek wrote: > On 10/04/2012 09:15 PM, Ben Hutchings wrote: > > On Thu, 2012-10-04 at 20:14 +0200, Michal Simek wrote: > >> Just show current setting in bootlog. > > [...] > >> --- a/drivers/net/ethernet/xilinx/ll_temac_main.c > >> +++ b/drivers/net/ethernet/xilinx/ll_temac_main.c > >> @@ -1052,12 +1052,14 @@ static int __devinit temac_of_probe(struct platform_device *op) > >> /* Setup checksum offload, but default to off if not specified */ > >> lp->temac_features = 0; > >> p = (__be32 *)of_get_property(op->dev.of_node, "xlnx,txcsum", NULL); > >> + dev_info(&op->dev, "TX_CSUM %d\n", be32_to_cpup(p)); > >> if (p && be32_to_cpu(*p)) { > >> lp->temac_features |= TEMAC_FEATURE_TX_CSUM; > >> /* Can checksum TCP/UDP over IPv4. */ > >> ndev->features |= NETIF_F_IP_CSUM; > >> } > >> p = (__be32 *)of_get_property(op->dev.of_node, "xlnx,rxcsum", NULL); > >> + dev_info(&op->dev, "RX_CSUM %d\n", be32_to_cpup(p)); > > [...] > > > > Is there any particular reason you think this needs to be logged by > > default, rather than letting users run ethtool -k? I suggest using > > dev_dbg() instead. > > The reason was just to show it in the bootlog. > I will check ethtool support for these drivers. 'ethtool -k' should just work so long as the driver sets the right bits in ndev->features. Ben.
On Fri, 2012-10-05 at 11:35 +0200, Michal Simek wrote: > On 10/04/2012 09:15 PM, Ben Hutchings wrote: > > On Thu, 2012-10-04 at 20:14 +0200, Michal Simek wrote: > >> Just show current setting in bootlog. > > [...] > >> --- a/drivers/net/ethernet/xilinx/ll_temac_main.c > >> +++ b/drivers/net/ethernet/xilinx/ll_temac_main.c > >> @@ -1052,12 +1052,14 @@ static int __devinit temac_of_probe(struct platform_device *op) > >> /* Setup checksum offload, but default to off if not specified */ > >> lp->temac_features = 0; > >> p = (__be32 *)of_get_property(op->dev.of_node, "xlnx,txcsum", NULL); > >> + dev_info(&op->dev, "TX_CSUM %d\n", be32_to_cpup(p)); > >> if (p && be32_to_cpu(*p)) { > >> lp->temac_features |= TEMAC_FEATURE_TX_CSUM; > >> /* Can checksum TCP/UDP over IPv4. */ > >> ndev->features |= NETIF_F_IP_CSUM; > >> } > >> p = (__be32 *)of_get_property(op->dev.of_node, "xlnx,rxcsum", NULL); > >> + dev_info(&op->dev, "RX_CSUM %d\n", be32_to_cpup(p)); > > [...] > > > > Is there any particular reason you think this needs to be logged by > > default, rather than letting users run ethtool -k? I suggest using > > dev_dbg() instead. > > Ok. I have looked at it and there are missing some bits in ndev->features. > > Can you please check that my setting is correct? > > It is SG DMA ip/driver. > ndev->features = NETIF_F_FRAGLIST | NETIF_F_SG NETIF_F_SG only; NETIF_F_FRAGLIST means you can handle skbs chained through the frag_list pointer. > With two options for csum on RX/TX. They can be selected independently. > tx Partial csum over IPv4. -> NETIF_F_IP_CSUM > tx Full csum. -> NETIF_F_HW_CSUM NETIF_F_IP_CSUM means you have hardware checksum generation for TCP/IPv4 and UDP/IPv4 only (XAE_FEATURE_FULL_TX_CSUM). NETIF_F_HW_CSUM means you have generic TCP-style checksum generation using the csum_start and csum_offset fields of the skb (XAE_FEATURE_PARTIAL_TX_CSUM). By the way, you're actually testing XAE_FEATURE_PARTIAL_RX_CSUM in axienet_start_xmit()... > rx Full csum -> NETIF_F_RXCSUM > > Is there any option to support partial csum? There is no need to differentiate these in the device features. For TX the stack needs to know whether to use a software fallback before passing the skb to you, but on RX it looks at the ip_summed field of each skb you pass up. Ben.
On 10/05/2012 03:51 PM, Ben Hutchings wrote: > On Fri, 2012-10-05 at 11:35 +0200, Michal Simek wrote: >> On 10/04/2012 09:15 PM, Ben Hutchings wrote: >>> On Thu, 2012-10-04 at 20:14 +0200, Michal Simek wrote: >>>> Just show current setting in bootlog. >>> [...] >>>> --- a/drivers/net/ethernet/xilinx/ll_temac_main.c >>>> +++ b/drivers/net/ethernet/xilinx/ll_temac_main.c >>>> @@ -1052,12 +1052,14 @@ static int __devinit temac_of_probe(struct platform_device *op) >>>> /* Setup checksum offload, but default to off if not specified */ >>>> lp->temac_features = 0; >>>> p = (__be32 *)of_get_property(op->dev.of_node, "xlnx,txcsum", NULL); >>>> + dev_info(&op->dev, "TX_CSUM %d\n", be32_to_cpup(p)); >>>> if (p && be32_to_cpu(*p)) { >>>> lp->temac_features |= TEMAC_FEATURE_TX_CSUM; >>>> /* Can checksum TCP/UDP over IPv4. */ >>>> ndev->features |= NETIF_F_IP_CSUM; >>>> } >>>> p = (__be32 *)of_get_property(op->dev.of_node, "xlnx,rxcsum", NULL); >>>> + dev_info(&op->dev, "RX_CSUM %d\n", be32_to_cpup(p)); >>> [...] >>> >>> Is there any particular reason you think this needs to be logged by >>> default, rather than letting users run ethtool -k? I suggest using >>> dev_dbg() instead. >> >> Ok. I have looked at it and there are missing some bits in ndev->features. >> >> Can you please check that my setting is correct? >> >> It is SG DMA ip/driver. >> ndev->features = NETIF_F_FRAGLIST | NETIF_F_SG > > NETIF_F_SG only; NETIF_F_FRAGLIST means you can handle skbs chained > through the frag_list pointer. The driver is able to handle skb fragments too. temac_start_xmit >> With two options for csum on RX/TX. They can be selected independently. >> tx Partial csum over IPv4. -> NETIF_F_IP_CSUM >> tx Full csum. -> NETIF_F_HW_CSUM > > NETIF_F_IP_CSUM means you have hardware checksum generation for TCP/IPv4 > and UDP/IPv4 only (XAE_FEATURE_FULL_TX_CSUM). > > NETIF_F_HW_CSUM means you have generic TCP-style checksum generation > using the csum_start and csum_offset fields of the skb > (XAE_FEATURE_PARTIAL_TX_CSUM). By the way, you're actually testing > XAE_FEATURE_PARTIAL_RX_CSUM in axienet_start_xmit()... We have used non mainline ll_temac driver for a long time but we will move to this mainline version soon. It is on my todo list to clean this driver and test all these options. Also performance tests will be necessary to do. >> rx Full csum -> NETIF_F_RXCSUM >> >> Is there any option to support partial csum? > > There is no need to differentiate these in the device features. For TX > the stack needs to know whether to use a software fallback before > passing the skb to you, but on RX it looks at the ip_summed field of > each skb you pass up. Hardware can be setup asymmetrically. It means enable CSUM only on RX or TX. All combination are valid. The point here is if Linux is not able to handle this then we have to create logic in the driver to support these options too. Thanks, Michal
On Tue, 2012-10-09 at 11:08 +0200, Michal Simek wrote: > On 10/05/2012 03:51 PM, Ben Hutchings wrote: > > On Fri, 2012-10-05 at 11:35 +0200, Michal Simek wrote: > >> On 10/04/2012 09:15 PM, Ben Hutchings wrote: > >>> On Thu, 2012-10-04 at 20:14 +0200, Michal Simek wrote: > >>>> Just show current setting in bootlog. > >>> [...] > >>>> --- a/drivers/net/ethernet/xilinx/ll_temac_main.c > >>>> +++ b/drivers/net/ethernet/xilinx/ll_temac_main.c > >>>> @@ -1052,12 +1052,14 @@ static int __devinit temac_of_probe(struct platform_device *op) > >>>> /* Setup checksum offload, but default to off if not specified */ > >>>> lp->temac_features = 0; > >>>> p = (__be32 *)of_get_property(op->dev.of_node, "xlnx,txcsum", NULL); > >>>> + dev_info(&op->dev, "TX_CSUM %d\n", be32_to_cpup(p)); > >>>> if (p && be32_to_cpu(*p)) { > >>>> lp->temac_features |= TEMAC_FEATURE_TX_CSUM; > >>>> /* Can checksum TCP/UDP over IPv4. */ > >>>> ndev->features |= NETIF_F_IP_CSUM; > >>>> } > >>>> p = (__be32 *)of_get_property(op->dev.of_node, "xlnx,rxcsum", NULL); > >>>> + dev_info(&op->dev, "RX_CSUM %d\n", be32_to_cpup(p)); > >>> [...] > >>> > >>> Is there any particular reason you think this needs to be logged by > >>> default, rather than letting users run ethtool -k? I suggest using > >>> dev_dbg() instead. > >> > >> Ok. I have looked at it and there are missing some bits in ndev->features. > >> > >> Can you please check that my setting is correct? > >> > >> It is SG DMA ip/driver. > >> ndev->features = NETIF_F_FRAGLIST | NETIF_F_SG > > > > NETIF_F_SG only; NETIF_F_FRAGLIST means you can handle skbs chained > > through the frag_list pointer. > > The driver is able to handle skb fragments too. temac_start_xmit "git grep -w -E 'frag_list|skb_walk_frags|skb_to_sgvec' drivers/net/ethernet/xilinx" returns nothing in net-next. [...] > >> rx Full csum -> NETIF_F_RXCSUM > >> > >> Is there any option to support partial csum? > > > > There is no need to differentiate these in the device features. For TX > > the stack needs to know whether to use a software fallback before > > passing the skb to you, but on RX it looks at the ip_summed field of > > each skb you pass up. > > Hardware can be setup asymmetrically. It means enable CSUM only on RX or TX. > All combination are valid. > The point here is if Linux is not able to handle this then we have to > create logic in the driver to support these options too. Linux handles this just fine. The point is you don't have to tell the stack in advance whether or what kind of RX checksum validation your devices will do. (In fact the only reason that feature flag exists at all is so that it can be generically exposed through the ethtool API.) Ben.
2012/10/9 Ben Hutchings <bhutchings@solarflare.com>: > On Tue, 2012-10-09 at 11:08 +0200, Michal Simek wrote: >> On 10/05/2012 03:51 PM, Ben Hutchings wrote: >> > On Fri, 2012-10-05 at 11:35 +0200, Michal Simek wrote: >> >> On 10/04/2012 09:15 PM, Ben Hutchings wrote: >> >>> On Thu, 2012-10-04 at 20:14 +0200, Michal Simek wrote: >> >>>> Just show current setting in bootlog. >> >>> [...] >> >>>> --- a/drivers/net/ethernet/xilinx/ll_temac_main.c >> >>>> +++ b/drivers/net/ethernet/xilinx/ll_temac_main.c >> >>>> @@ -1052,12 +1052,14 @@ static int __devinit temac_of_probe(struct platform_device *op) >> >>>> /* Setup checksum offload, but default to off if not specified */ >> >>>> lp->temac_features = 0; >> >>>> p = (__be32 *)of_get_property(op->dev.of_node, "xlnx,txcsum", NULL); >> >>>> + dev_info(&op->dev, "TX_CSUM %d\n", be32_to_cpup(p)); >> >>>> if (p && be32_to_cpu(*p)) { >> >>>> lp->temac_features |= TEMAC_FEATURE_TX_CSUM; >> >>>> /* Can checksum TCP/UDP over IPv4. */ >> >>>> ndev->features |= NETIF_F_IP_CSUM; >> >>>> } >> >>>> p = (__be32 *)of_get_property(op->dev.of_node, "xlnx,rxcsum", NULL); >> >>>> + dev_info(&op->dev, "RX_CSUM %d\n", be32_to_cpup(p)); >> >>> [...] >> >>> >> >>> Is there any particular reason you think this needs to be logged by >> >>> default, rather than letting users run ethtool -k? I suggest using >> >>> dev_dbg() instead. >> >> >> >> Ok. I have looked at it and there are missing some bits in ndev->features. >> >> >> >> Can you please check that my setting is correct? >> >> >> >> It is SG DMA ip/driver. >> >> ndev->features = NETIF_F_FRAGLIST | NETIF_F_SG >> > >> > NETIF_F_SG only; NETIF_F_FRAGLIST means you can handle skbs chained >> > through the frag_list pointer. >> >> The driver is able to handle skb fragments too. temac_start_xmit > > "git grep -w -E 'frag_list|skb_walk_frags|skb_to_sgvec' drivers/net/ethernet/xilinx" > returns nothing in net-next. What about this? Maybe it is different fragmentation. drivers/net/ethernet/xilinx/ll_temac_main.c 688 num_frag = skb_shinfo(skb)->nr_frags; 689 frag = &skb_shinfo(skb)->frags[0]; > > [...] >> >> rx Full csum -> NETIF_F_RXCSUM >> >> >> >> Is there any option to support partial csum? >> > >> > There is no need to differentiate these in the device features. For TX >> > the stack needs to know whether to use a software fallback before >> > passing the skb to you, but on RX it looks at the ip_summed field of >> > each skb you pass up. >> >> Hardware can be setup asymmetrically. It means enable CSUM only on RX or TX. >> All combination are valid. >> The point here is if Linux is not able to handle this then we have to >> create logic in the driver to support these options too. > > Linux handles this just fine. The point is you don't have to tell the > stack in advance whether or what kind of RX checksum validation your > devices will do. (In fact the only reason that feature flag exists at > all is so that it can be generically exposed through the ethtool API.) Ok. Thanks, Michal
On Wed, 2012-10-10 at 18:06 +0200, Michal Simek wrote: > 2012/10/9 Ben Hutchings <bhutchings@solarflare.com>: > > On Tue, 2012-10-09 at 11:08 +0200, Michal Simek wrote: > >> On 10/05/2012 03:51 PM, Ben Hutchings wrote: > >> > On Fri, 2012-10-05 at 11:35 +0200, Michal Simek wrote: > >> >> On 10/04/2012 09:15 PM, Ben Hutchings wrote: > >> >>> On Thu, 2012-10-04 at 20:14 +0200, Michal Simek wrote: > >> >>>> Just show current setting in bootlog. > >> >>> [...] > >> >>>> --- a/drivers/net/ethernet/xilinx/ll_temac_main.c > >> >>>> +++ b/drivers/net/ethernet/xilinx/ll_temac_main.c > >> >>>> @@ -1052,12 +1052,14 @@ static int __devinit temac_of_probe(struct platform_device *op) > >> >>>> /* Setup checksum offload, but default to off if not specified */ > >> >>>> lp->temac_features = 0; > >> >>>> p = (__be32 *)of_get_property(op->dev.of_node, "xlnx,txcsum", NULL); > >> >>>> + dev_info(&op->dev, "TX_CSUM %d\n", be32_to_cpup(p)); > >> >>>> if (p && be32_to_cpu(*p)) { > >> >>>> lp->temac_features |= TEMAC_FEATURE_TX_CSUM; > >> >>>> /* Can checksum TCP/UDP over IPv4. */ > >> >>>> ndev->features |= NETIF_F_IP_CSUM; > >> >>>> } > >> >>>> p = (__be32 *)of_get_property(op->dev.of_node, "xlnx,rxcsum", NULL); > >> >>>> + dev_info(&op->dev, "RX_CSUM %d\n", be32_to_cpup(p)); > >> >>> [...] > >> >>> > >> >>> Is there any particular reason you think this needs to be logged by > >> >>> default, rather than letting users run ethtool -k? I suggest using > >> >>> dev_dbg() instead. > >> >> > >> >> Ok. I have looked at it and there are missing some bits in ndev->features. > >> >> > >> >> Can you please check that my setting is correct? > >> >> > >> >> It is SG DMA ip/driver. > >> >> ndev->features = NETIF_F_FRAGLIST | NETIF_F_SG > >> > > >> > NETIF_F_SG only; NETIF_F_FRAGLIST means you can handle skbs chained > >> > through the frag_list pointer. > >> > >> The driver is able to handle skb fragments too. temac_start_xmit > > > > "git grep -w -E 'frag_list|skb_walk_frags|skb_to_sgvec' drivers/net/ethernet/xilinx" > > returns nothing in net-next. > > What about this? Maybe it is different fragmentation. > drivers/net/ethernet/xilinx/ll_temac_main.c > 688 num_frag = skb_shinfo(skb)->nr_frags; > 689 frag = &skb_shinfo(skb)->frags[0]; [...] Yes, the fact that you handle the frags array is indicated by feature flag NETIF_F_SG. Ben.
diff --git a/drivers/net/ethernet/xilinx/ll_temac_main.c b/drivers/net/ethernet/xilinx/ll_temac_main.c index 58b2869..8c31fcd 100644 --- a/drivers/net/ethernet/xilinx/ll_temac_main.c +++ b/drivers/net/ethernet/xilinx/ll_temac_main.c @@ -1052,12 +1052,14 @@ static int __devinit temac_of_probe(struct platform_device *op) /* Setup checksum offload, but default to off if not specified */ lp->temac_features = 0; p = (__be32 *)of_get_property(op->dev.of_node, "xlnx,txcsum", NULL); + dev_info(&op->dev, "TX_CSUM %d\n", be32_to_cpup(p)); if (p && be32_to_cpu(*p)) { lp->temac_features |= TEMAC_FEATURE_TX_CSUM; /* Can checksum TCP/UDP over IPv4. */ ndev->features |= NETIF_F_IP_CSUM; } p = (__be32 *)of_get_property(op->dev.of_node, "xlnx,rxcsum", NULL); + dev_info(&op->dev, "RX_CSUM %d\n", be32_to_cpup(p)); if (p && be32_to_cpu(*p)) lp->temac_features |= TEMAC_FEATURE_RX_CSUM; diff --git a/drivers/net/ethernet/xilinx/xilinx_axienet_main.c b/drivers/net/ethernet/xilinx/xilinx_axienet_main.c index 4ef148f..9ea5be6 100644 --- a/drivers/net/ethernet/xilinx/xilinx_axienet_main.c +++ b/drivers/net/ethernet/xilinx/xilinx_axienet_main.c @@ -1528,6 +1528,7 @@ static int __devinit axienet_of_probe(struct platform_device *op) lp->features = 0; p = (__be32 *) of_get_property(op->dev.of_node, "xlnx,txcsum", NULL); + dev_info(&op->dev, "TX_CSUM %d\n", be32_to_cpup(p)); if (p) { switch (be32_to_cpup(p)) { case 1: @@ -1549,6 +1550,7 @@ static int __devinit axienet_of_probe(struct platform_device *op) } } p = (__be32 *) of_get_property(op->dev.of_node, "xlnx,rxcsum", NULL); + dev_info(&op->dev, "RX_CSUM %d\n", be32_to_cpup(p)); if (p) { switch (be32_to_cpup(p)) { case 1:
Just show current setting in bootlog. Signed-off-by: Michal Simek <monstr@monstr.eu> CC: Anirudha Sarangi <anirudh@xilinx.com> CC: John Linn <John.Linn@xilinx.com> CC: Grant Likely <grant.likely@secretlab.ca> CC: Rob Herring <rob.herring@calxeda.com> CC: David S. Miller <davem@davemloft.net> --- drivers/net/ethernet/xilinx/ll_temac_main.c | 2 ++ drivers/net/ethernet/xilinx/xilinx_axienet_main.c | 2 ++ 2 files changed, 4 insertions(+), 0 deletions(-)