[PATCH net-next] net: mscc: allow extracting the FCS into the skb

[PATCH net-next] net: mscc: allow extracting the FCS into the skb

[Antoine Tenart]

This patch adds support for the NETIF_F_RXFCS feature in the Mscc
Ethernet driver. This feature is disabled by default and allow an user
to request the driver not to drop the FCS and to extract it into the skb
for debugging purposes.

Signed-off-by: Antoine Tenart <antoine.tenart@bootlin.com>
---
 drivers/net/ethernet/mscc/ocelot.c       | 2 +-
 drivers/net/ethernet/mscc/ocelot_board.c | 7 ++++++-
 2 files changed, 7 insertions(+), 2 deletions(-)

diff --git a/drivers/net/ethernet/mscc/ocelot.c b/drivers/net/ethernet/mscc/ocelot.c
index 1a4f2bb48ead..eb5119e7e60b 100644
--- a/drivers/net/ethernet/mscc/ocelot.c
+++ b/drivers/net/ethernet/mscc/ocelot.c
@@ -1606,7 +1606,7 @@ int ocelot_probe_port(struct ocelot *ocelot, u8 port,
 	dev->ethtool_ops = &ocelot_ethtool_ops;
 	dev->switchdev_ops = &ocelot_port_switchdev_ops;
 
-	dev->hw_features |= NETIF_F_HW_VLAN_CTAG_FILTER;
+	dev->hw_features |= NETIF_F_HW_VLAN_CTAG_FILTER | NETIF_F_RXFCS;
 	dev->features |= NETIF_F_HW_VLAN_CTAG_FILTER;
 
 	memcpy(dev->dev_addr, ocelot->base_mac, ETH_ALEN);
diff --git a/drivers/net/ethernet/mscc/ocelot_board.c b/drivers/net/ethernet/mscc/ocelot_board.c
index 3cdf63e35b53..245452a0f244 100644
--- a/drivers/net/ethernet/mscc/ocelot_board.c
+++ b/drivers/net/ethernet/mscc/ocelot_board.c
@@ -126,11 +126,16 @@ static irqreturn_t ocelot_xtr_irq_handler(int irq, void *arg)
 			len += sz;
 		} while (len < buf_len);
 
-		/* Read the FCS and discard it */
+		/* Read the FCS */
 		sz = ocelot_rx_frame_word(ocelot, grp, false, &val);
 		/* Update the statistics if part of the FCS was read before */
 		len -= ETH_FCS_LEN - sz;
 
+		if (unlikely(dev->features & NETIF_F_RXFCS)) {
+			buf = (u32 *)skb_put(skb, ETH_FCS_LEN);
+			*buf = val;
+		}
+
 		if (sz < 0) {
 			err = sz;
 			break;
-- 
2.17.1

Re: [PATCH net-next] net: mscc: allow extracting the FCS into the skb

[Alexandre Belloni]

On 01/10/2018 11:57:14+0200, Antoine Ténart wrote:
> This patch adds support for the NETIF_F_RXFCS feature in the Mscc
> Ethernet driver. This feature is disabled by default and allow an user
> to request the driver not to drop the FCS and to extract it into the skb
> for debugging purposes.
> 
> Signed-off-by: Antoine Tenart <antoine.tenart@bootlin.com>
Reviewed-by: Alexandre Belloni <alexandre.belloni@bootlin.com>

> ---
>  drivers/net/ethernet/mscc/ocelot.c       | 2 +-
>  drivers/net/ethernet/mscc/ocelot_board.c | 7 ++++++-
>  2 files changed, 7 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/net/ethernet/mscc/ocelot.c b/drivers/net/ethernet/mscc/ocelot.c
> index 1a4f2bb48ead..eb5119e7e60b 100644
> --- a/drivers/net/ethernet/mscc/ocelot.c
> +++ b/drivers/net/ethernet/mscc/ocelot.c
> @@ -1606,7 +1606,7 @@ int ocelot_probe_port(struct ocelot *ocelot, u8 port,
>  	dev->ethtool_ops = &ocelot_ethtool_ops;
>  	dev->switchdev_ops = &ocelot_port_switchdev_ops;
>  
> -	dev->hw_features |= NETIF_F_HW_VLAN_CTAG_FILTER;
> +	dev->hw_features |= NETIF_F_HW_VLAN_CTAG_FILTER | NETIF_F_RXFCS;
>  	dev->features |= NETIF_F_HW_VLAN_CTAG_FILTER;
>  
>  	memcpy(dev->dev_addr, ocelot->base_mac, ETH_ALEN);
> diff --git a/drivers/net/ethernet/mscc/ocelot_board.c b/drivers/net/ethernet/mscc/ocelot_board.c
> index 3cdf63e35b53..245452a0f244 100644
> --- a/drivers/net/ethernet/mscc/ocelot_board.c
> +++ b/drivers/net/ethernet/mscc/ocelot_board.c
> @@ -126,11 +126,16 @@ static irqreturn_t ocelot_xtr_irq_handler(int irq, void *arg)
>  			len += sz;
>  		} while (len < buf_len);
>  
> -		/* Read the FCS and discard it */
> +		/* Read the FCS */
>  		sz = ocelot_rx_frame_word(ocelot, grp, false, &val);
>  		/* Update the statistics if part of the FCS was read before */
>  		len -= ETH_FCS_LEN - sz;
>  
> +		if (unlikely(dev->features & NETIF_F_RXFCS)) {
> +			buf = (u32 *)skb_put(skb, ETH_FCS_LEN);
> +			*buf = val;
> +		}
> +
>  		if (sz < 0) {
>  			err = sz;
>  			break;
> -- 
> 2.17.1
> 

-- 
Alexandre Belloni, Bootlin
Embedded Linux and Kernel engineering
https://bootlin.com

Re: [PATCH net-next] net: mscc: allow extracting the FCS into the skb

[Florian Fainelli]

On 10/01/2018 02:57 AM, Antoine Tenart wrote:
> This patch adds support for the NETIF_F_RXFCS feature in the Mscc
> Ethernet driver. This feature is disabled by default and allow an user
> to request the driver not to drop the FCS and to extract it into the skb
> for debugging purposes.
> 
> Signed-off-by: Antoine Tenart <antoine.tenart@bootlin.com>
> ---
>  drivers/net/ethernet/mscc/ocelot.c       | 2 +-
>  drivers/net/ethernet/mscc/ocelot_board.c | 7 ++++++-
>  2 files changed, 7 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/net/ethernet/mscc/ocelot.c b/drivers/net/ethernet/mscc/ocelot.c
> index 1a4f2bb48ead..eb5119e7e60b 100644
> --- a/drivers/net/ethernet/mscc/ocelot.c
> +++ b/drivers/net/ethernet/mscc/ocelot.c
> @@ -1606,7 +1606,7 @@ int ocelot_probe_port(struct ocelot *ocelot, u8 port,
>  	dev->ethtool_ops = &ocelot_ethtool_ops;
>  	dev->switchdev_ops = &ocelot_port_switchdev_ops;
>  
> -	dev->hw_features |= NETIF_F_HW_VLAN_CTAG_FILTER;
> +	dev->hw_features |= NETIF_F_HW_VLAN_CTAG_FILTER | NETIF_F_RXFCS;
>  	dev->features |= NETIF_F_HW_VLAN_CTAG_FILTER;
>  
>  	memcpy(dev->dev_addr, ocelot->base_mac, ETH_ALEN);
> diff --git a/drivers/net/ethernet/mscc/ocelot_board.c b/drivers/net/ethernet/mscc/ocelot_board.c
> index 3cdf63e35b53..245452a0f244 100644
> --- a/drivers/net/ethernet/mscc/ocelot_board.c
> +++ b/drivers/net/ethernet/mscc/ocelot_board.c
> @@ -126,11 +126,16 @@ static irqreturn_t ocelot_xtr_irq_handler(int irq, void *arg)
>  			len += sz;
>  		} while (len < buf_len);
>  
> -		/* Read the FCS and discard it */
> +		/* Read the FCS */
>  		sz = ocelot_rx_frame_word(ocelot, grp, false, &val);
>  		/* Update the statistics if part of the FCS was read before */
>  		len -= ETH_FCS_LEN - sz;
>  
Don't this needs to be len -= sz;


> +		if (unlikely(dev->features & NETIF_F_RXFCS)) {
> +			buf = (u32 *)skb_put(skb, ETH_FCS_LEN);
> +			*buf = val;

and here len -= ETH_FCS_LEN

since "len" is later used for accounting how many bytes have been
received by the network device?

I am bit confused by the use of "buf", but presumably if NETIF_F_RXCFS
is turned on, the FCS needs to be part of the buffer passed to the
network stack, so adjusting len accordingly is required anyway.
-- 
Florian

Re: [PATCH net-next] net: mscc: allow extracting the FCS into the skb

[Antoine Tenart]

Hi FLorian,

On Mon, Oct 01, 2018 at 09:35:45AM -0700, Florian Fainelli wrote:
> On 10/01/2018 02:57 AM, Antoine Tenart wrote:
> >  
> >  	memcpy(dev->dev_addr, ocelot->base_mac, ETH_ALEN);
> > diff --git a/drivers/net/ethernet/mscc/ocelot_board.c b/drivers/net/ethernet/mscc/ocelot_board.c
> > index 3cdf63e35b53..245452a0f244 100644
> > --- a/drivers/net/ethernet/mscc/ocelot_board.c
> > +++ b/drivers/net/ethernet/mscc/ocelot_board.c
> > @@ -126,11 +126,16 @@ static irqreturn_t ocelot_xtr_irq_handler(int irq, void *arg)
> >  			len += sz;
> >  		} while (len < buf_len);
> >  
> > -		/* Read the FCS and discard it */
> > +		/* Read the FCS */
> >  		sz = ocelot_rx_frame_word(ocelot, grp, false, &val);
> >  		/* Update the statistics if part of the FCS was read before */
> >  		len -= ETH_FCS_LEN - sz;
> >  
> Don't this needs to be len -= sz;

No, part of the FCS could be read in the loop before. This subtraction
removes those bytes from the stats.

> > +		if (unlikely(dev->features & NETIF_F_RXFCS)) {
> > +			buf = (u32 *)skb_put(skb, ETH_FCS_LEN);
> > +			*buf = val;
> 
> and here len -= ETH_FCS_LEN

len doesn't contain the FCS len at this point, no need to remove it
again. The extraction logic is already designed to drop the FCS and to
not count it in the stats, this patch makes use of it depending on
NETIF_F_RXFCS.

> since "len" is later used for accounting how many bytes have been
> received by the network device?

The question could be "do we need len += ETH_FCS_LEN" to account for the
FCS when NETIF_F_RXFCS is used", but I looked at other drivers and it
seemed to me the FCS is not accounted in the stats. Should it be?

> I am bit confused by the use of "buf", but presumably if NETIF_F_RXCFS
> is turned on, the FCS needs to be part of the buffer passed to the
> network stack, so adjusting len accordingly is required anyway.

This is the way we extract the data to the skb, I'm following the same
logic (I could use buf++ as well, as it's done in the loop before).

Thanks!
Antoine

-- 
Antoine Ténart, Bootlin
Embedded Linux and Kernel engineering
https://bootlin.com

Re: [PATCH net-next] net: mscc: allow extracting the FCS into the skb

[Andrew Lunn]

> The question could be "do we need len += ETH_FCS_LEN" to account for the
> FCS when NETIF_F_RXFCS is used", but I looked at other drivers and it
> seemed to me the FCS is not accounted in the stats. Should it be?

Hi Antoine

There does not appear to be a good answer to that. I submitted a patch
to a driver i'm using to not count it, so that the stats counters we
consistent with another driver. The patch was rejected.

I think the best you can do is flip a coin, and then document it using
a comment about if it is/is not included.

  Andrew

Re: [PATCH net-next] net: mscc: allow extracting the FCS into the skb

[Antoine Tenart]

Hi all,

On Tue, Oct 02, 2018 at 02:43:23PM +0200, Andrew Lunn wrote:
> > The question could be "do we need len += ETH_FCS_LEN" to account for the
> > FCS when NETIF_F_RXFCS is used", but I looked at other drivers and it
> > seemed to me the FCS is not accounted in the stats. Should it be?
> 
> There does not appear to be a good answer to that. I submitted a patch
> to a driver i'm using to not count it, so that the stats counters we
> consistent with another driver. The patch was rejected.
> 
> I think the best you can do is flip a coin, and then document it using
> a comment about if it is/is not included.

I'll leave it as-is then :)

@Dave, Florian: it seems to me no modification was requested after
discussing those changes. Where do we stand regarding the patch?

Thanks!
Antoine

-- 
Antoine Ténart, Bootlin
Embedded Linux and Kernel engineering
https://bootlin.com

Re: [PATCH net-next] net: mscc: allow extracting the FCS into the skb

[Florian Fainelli]

On October 10, 2018 7:46:31 AM PDT, Antoine Tenart <antoine.tenart@bootlin.com> wrote:
>Hi all,
>
>On Tue, Oct 02, 2018 at 02:43:23PM +0200, Andrew Lunn wrote:
>> > The question could be "do we need len += ETH_FCS_LEN" to account
>for the
>> > FCS when NETIF_F_RXFCS is used", but I looked at other drivers and
>it
>> > seemed to me the FCS is not accounted in the stats. Should it be?
>> 
>> There does not appear to be a good answer to that. I submitted a
>patch
>> to a driver i'm using to not count it, so that the stats counters we
>> consistent with another driver. The patch was rejected.
>> 
>> I think the best you can do is flip a coin, and then document it
>using
>> a comment about if it is/is not included.
>
>I'll leave it as-is then :)
>
>@Dave, Florian: it seems to me no modification was requested after
>discussing those changes. Where do we stand regarding the patch?

Silence means agreement, thanks for your explanation, no more questions or concerns on my side.
-- 
Florian

Re: [PATCH net-next] net: mscc: allow extracting the FCS into the skb

[David Miller]

From: Florian Fainelli <f.fainelli@gmail.com>
Date: Wed, 10 Oct 2018 09:25:01 -0700

> 
> 
> On October 10, 2018 7:46:31 AM PDT, Antoine Tenart <antoine.tenart@bootlin.com> wrote:
>>Hi all,
>>
>>On Tue, Oct 02, 2018 at 02:43:23PM +0200, Andrew Lunn wrote:
>>> > The question could be "do we need len += ETH_FCS_LEN" to account
>>for the
>>> > FCS when NETIF_F_RXFCS is used", but I looked at other drivers and
>>it
>>> > seemed to me the FCS is not accounted in the stats. Should it be?
>>> 
>>> There does not appear to be a good answer to that. I submitted a
>>patch
>>> to a driver i'm using to not count it, so that the stats counters we
>>> consistent with another driver. The patch was rejected.
>>> 
>>> I think the best you can do is flip a coin, and then document it
>>using
>>> a comment about if it is/is not included.
>>
>>I'll leave it as-is then :)
>>
>>@Dave, Florian: it seems to me no modification was requested after
>>discussing those changes. Where do we stand regarding the patch?
> 
> Silence means agreement, thanks for your explanation, no more questions or concerns on my side.

You'll probably need to resubmit the patch anew though.

Re: [PATCH net-next] net: mscc: allow extracting the FCS into the skb

[Antoine Tenart]

Hi Florian, Dave,

On Wed, Oct 10, 2018 at 10:26:05AM -0700, David Miller wrote:
> From: Florian Fainelli <f.fainelli@gmail.com>
> Date: Wed, 10 Oct 2018 09:25:01 -0700
> > 
> > On October 10, 2018 7:46:31 AM PDT, Antoine Tenart <antoine.tenart@bootlin.com> wrote:
> >>
> >>@Dave, Florian: it seems to me no modification was requested after
> >>discussing those changes. Where do we stand regarding the patch?
> > 
> > Silence means agreement, thanks for your explanation, no more
> > questions or concerns on my side.
> 
> You'll probably need to resubmit the patch anew though.

I'll rebase the patch on top of net-next and send it again.

Thanks!
Antoine

-- 
Antoine Ténart, Bootlin
Embedded Linux and Kernel engineering
https://bootlin.com