[RFC v2 0/2] Enable RX-FCS in e100

[RFC v2 0/2] Enable RX-FCS in e100

[greearb]

From: Ben Greear <greearb@candelatech.com>

This is on top of Miroslaw's patches.  No new patches
are required to ethtool.

This is for RFC only until his patches go in.

Ben Greear (2):
  net:  Support RXFCS feature flag.
  e100:  Support RXFCS feature flag.

 drivers/net/e100.c              |   15 ++++++++++++---
 include/linux/netdev_features.h |    2 ++
 net/core/ethtool.c              |    1 +
 3 files changed, 15 insertions(+), 3 deletions(-)

-- 
1.7.3.4

[RFC v2 1/2] net: Support RXFCS feature flag.

[greearb]

From: Ben Greear <greearb@candelatech.com>

When set, this causes the Ethernet FCS to be appended
to the end of the skb.

Useful for sniffing packets.

Signed-off-by: Ben Greear <greearb@candelatech.com>
---
:100644 100644 d0ab610... 55f359e... M	include/linux/netdev_features.h
:100644 100644 80b88fe... 62f15e9... M	net/core/ethtool.c
 include/linux/netdev_features.h |    2 ++
 net/core/ethtool.c              |    1 +
 2 files changed, 3 insertions(+), 0 deletions(-)

diff --git a/include/linux/netdev_features.h b/include/linux/netdev_features.h
index d0ab610..55f359e 100644
--- a/include/linux/netdev_features.h
+++ b/include/linux/netdev_features.h
@@ -54,6 +54,7 @@ enum {
 	NETIF_F_RXCSUM_BIT,		/* Receive checksumming offload */
 	NETIF_F_NOCACHE_COPY_BIT,	/* Use no-cache copyfromuser */
 	NETIF_F_LOOPBACK_BIT,		/* Enable loopback */
+	NETIF_F_RXFCS_BIT,		/* Append FCS to skb */
 
 	/*
 	 * Add your fresh new feature above and remember to update
@@ -98,6 +99,7 @@ enum {
 #define NETIF_F_RXCSUM		__NETIF_F(RXCSUM)
 #define NETIF_F_NOCACHE_COPY	__NETIF_F(NOCACHE_COPY)
 #define NETIF_F_LOOPBACK	__NETIF_F(LOOPBACK)
+#define NETIF_F_RXFCS		__NETIF_F(RXFCS)
 
 /* Features valid for ethtool to change */
 /* = all defined minus driver/device-class-related */
diff --git a/net/core/ethtool.c b/net/core/ethtool.c
index 80b88fe..62f15e9 100644
--- a/net/core/ethtool.c
+++ b/net/core/ethtool.c
@@ -74,6 +74,7 @@ static const char netdev_features_strings[NETDEV_FEATURE_COUNT][ETH_GSTRING_LEN]
 	[NETIF_F_RXCSUM_BIT] =           "rx-checksum",
 	[NETIF_F_NOCACHE_COPY_BIT] =     "tx-nocache-copy",
 	[NETIF_F_LOOPBACK_BIT] =         "loopback",
+	[NETIF_F_RXFCS_BIT] =            "rx-fcs",
 };
 
 static int ethtool_get_features(struct net_device *dev, void __user *useraddr)
-- 
1.7.3.4

[RFC v2 2/2] e100: Support RXFCS feature flag.

[greearb]

From: Ben Greear <greearb@candelatech.com>

This allows e100 to be configured to append the
Ethernet FCS to the skb.

Useful for sniffing networks.

Signed-off-by: Ben Greear <greearb@candelatech.com>
---
:100644 100644 c1352c6... 761f6f5... M	drivers/net/e100.c
 drivers/net/e100.c |   15 ++++++++++++---
 1 files changed, 12 insertions(+), 3 deletions(-)

diff --git a/drivers/net/e100.c b/drivers/net/e100.c
index c1352c6..761f6f5 100644
--- a/drivers/net/e100.c
+++ b/drivers/net/e100.c
@@ -1089,6 +1089,7 @@ static void e100_configure(struct nic *nic, struct cb *cb, struct sk_buff *skb)
 {
 	struct config *config = &cb->u.config;
 	u8 *c = (u8 *)config;
+	struct net_device *netdev = nic->netdev;
 
 	cb->command = cpu_to_le16(cb_config);
 
@@ -1132,6 +1133,9 @@ static void e100_configure(struct nic *nic, struct cb *cb, struct sk_buff *skb)
 		config->promiscuous_mode = 0x1;		/* 1=on, 0=off */
 	}
 
+	if (netdev->wanted_features & NETIF_F_RXFCS)
+		config->rx_crc_transfer = 0x1;	/* 1=save, 0=discard */
+
 	if (nic->flags & multicast_all)
 		config->multicast_all = 0x1;		/* 1=accept, 0=no */
 
@@ -1919,6 +1923,7 @@ static int e100_rx_indicate(struct nic *nic, struct rx *rx,
 	struct sk_buff *skb = rx->skb;
 	struct rfd *rfd = (struct rfd *)skb->data;
 	u16 rfd_status, actual_size;
+	u16 fcs_pad = 0;
 
 	if (unlikely(work_done && *work_done >= work_to_do))
 		return -EAGAIN;
@@ -1951,9 +1956,11 @@ static int e100_rx_indicate(struct nic *nic, struct rx *rx,
 	}
 
 	/* Get actual data size */
+	if (dev->wanted_features & NETIF_F_RXFCS)
+		fcs_pad = 4;
 	actual_size = le16_to_cpu(rfd->actual_size) & 0x3FFF;
-	if (unlikely(actual_size > RFD_BUF_LEN - sizeof(struct rfd)))
-		actual_size = RFD_BUF_LEN - sizeof(struct rfd);
+	if (unlikely(actual_size > RFD_BUF_LEN + fcs_pad - sizeof(struct rfd)))
+		actual_size = RFD_BUF_LEN + fcs_pad - sizeof(struct rfd);
 
 	/* Get data */
 	pci_unmap_single(nic->pdev, rx->dma_addr,
@@ -1980,7 +1987,7 @@ static int e100_rx_indicate(struct nic *nic, struct rx *rx,
 	if (unlikely(!(rfd_status & cb_ok))) {
 		/* Don't indicate if hardware indicates errors */
 		dev_kfree_skb_any(skb);
-	} else if (actual_size > ETH_DATA_LEN + VLAN_ETH_HLEN) {
+	} else if (actual_size > ETH_DATA_LEN + VLAN_ETH_HLEN + fcs_pad) {
 		/* Don't indicate oversized frames */
 		nic->rx_over_length_errors++;
 		dev_kfree_skb_any(skb);
@@ -2761,6 +2768,8 @@ static int __devinit e100_probe(struct pci_dev *pdev,
 		return -ENOMEM;
 	}
 
+	netdev->hw_features |= NETIF_F_RXFCS;
+
 	netdev->netdev_ops = &e100_netdev_ops;
 	SET_ETHTOOL_OPS(netdev, &e100_ethtool_ops);
 	netdev->watchdog_timeo = E100_WATCHDOG_PERIOD;
-- 
1.7.3.4

Re: [RFC v2 2/2] e100: Support RXFCS feature flag.

[Michał Mirosław]

2011/6/24  <greearb@candelatech.com>:
> From: Ben Greear <greearb@candelatech.com>
>
> This allows e100 to be configured to append the
> Ethernet FCS to the skb.
>
> Useful for sniffing networks.
>
> Signed-off-by: Ben Greear <greearb@candelatech.com>
> ---
> :100644 100644 c1352c6... 761f6f5... M  drivers/net/e100.c
>  drivers/net/e100.c |   15 ++++++++++++---
>  1 files changed, 12 insertions(+), 3 deletions(-)
>
> diff --git a/drivers/net/e100.c b/drivers/net/e100.c
> index c1352c6..761f6f5 100644
> --- a/drivers/net/e100.c
> +++ b/drivers/net/e100.c
> @@ -1089,6 +1089,7 @@ static void e100_configure(struct nic *nic, struct cb *cb, struct sk_buff *skb)
>  {
>        struct config *config = &cb->u.config;
>        u8 *c = (u8 *)config;
> +       struct net_device *netdev = nic->netdev;
>
>        cb->command = cpu_to_le16(cb_config);
>
> @@ -1132,6 +1133,9 @@ static void e100_configure(struct nic *nic, struct cb *cb, struct sk_buff *skb)
>                config->promiscuous_mode = 0x1;         /* 1=on, 0=off */
>        }
>
> +       if (netdev->wanted_features & NETIF_F_RXFCS)
> +               config->rx_crc_transfer = 0x1;  /* 1=save, 0=discard */
> +

You should check netdev->features here.

>        if (nic->flags & multicast_all)
>                config->multicast_all = 0x1;            /* 1=accept, 0=no */
>
> @@ -1919,6 +1923,7 @@ static int e100_rx_indicate(struct nic *nic, struct rx *rx,
>        struct sk_buff *skb = rx->skb;
>        struct rfd *rfd = (struct rfd *)skb->data;
>        u16 rfd_status, actual_size;
> +       u16 fcs_pad = 0;
>
>        if (unlikely(work_done && *work_done >= work_to_do))
>                return -EAGAIN;
> @@ -1951,9 +1956,11 @@ static int e100_rx_indicate(struct nic *nic, struct rx *rx,
>        }
>
>        /* Get actual data size */
> +       if (dev->wanted_features & NETIF_F_RXFCS)
> +               fcs_pad = 4;

Same here.

Best Regards,
Michał Mirosław

Re: [RFC v2 2/2] e100: Support RXFCS feature flag.

[Ben Greear]

On 06/29/2011 04:37 AM, Michał Mirosław wrote:
> 2011/6/24<greearb@candelatech.com>:
>> From: Ben Greear<greearb@candelatech.com>
>>
>> This allows e100 to be configured to append the
>> Ethernet FCS to the skb.
>>
>> Useful for sniffing networks.
>>
>> Signed-off-by: Ben Greear<greearb@candelatech.com>
>> ---
>> :100644 100644 c1352c6... 761f6f5... M  drivers/net/e100.c
>>   drivers/net/e100.c |   15 ++++++++++++---
>>   1 files changed, 12 insertions(+), 3 deletions(-)
>>
>> diff --git a/drivers/net/e100.c b/drivers/net/e100.c
>> index c1352c6..761f6f5 100644
>> --- a/drivers/net/e100.c
>> +++ b/drivers/net/e100.c
>> @@ -1089,6 +1089,7 @@ static void e100_configure(struct nic *nic, struct cb *cb, struct sk_buff *skb)
>>   {
>>         struct config *config =&cb->u.config;
>>         u8 *c = (u8 *)config;
>> +       struct net_device *netdev = nic->netdev;
>>
>>         cb->command = cpu_to_le16(cb_config);
>>
>> @@ -1132,6 +1133,9 @@ static void e100_configure(struct nic *nic, struct cb *cb, struct sk_buff *skb)
>>                 config->promiscuous_mode = 0x1;         /* 1=on, 0=off */
>>         }
>>
>> +       if (netdev->wanted_features&  NETIF_F_RXFCS)
>> +               config->rx_crc_transfer = 0x1;  /* 1=save, 0=discard */
>> +
>
> You should check netdev->features here.

I thought 'features' was what the NIC could support, and wanted_features
was what the NIC was currently configured to support?  I don't want
to rx the CRC all the time, just when users enable it...

Thanks,
Ben

-- 
Ben Greear <greearb@candelatech.com>
Candela Technologies Inc  http://www.candelatech.com

Re: [RFC v2 2/2] e100: Support RXFCS feature flag.

[Michał Mirosław]

W dniu 29 czerwca 2011 16:22 użytkownik Ben Greear
<greearb@candelatech.com> napisał:
> On 06/29/2011 04:37 AM, Michał Mirosław wrote:
>> 2011/6/24<greearb@candelatech.com>:
>>> From: Ben Greear<greearb@candelatech.com>
>>>
>>> This allows e100 to be configured to append the
>>> Ethernet FCS to the skb.
>>>
>>> Useful for sniffing networks.
>>>
>>> Signed-off-by: Ben Greear<greearb@candelatech.com>
>>> ---
>>> :100644 100644 c1352c6... 761f6f5... M  drivers/net/e100.c
>>>  drivers/net/e100.c |   15 ++++++++++++---
>>>  1 files changed, 12 insertions(+), 3 deletions(-)
>>>
>>> diff --git a/drivers/net/e100.c b/drivers/net/e100.c
>>> index c1352c6..761f6f5 100644
>>> --- a/drivers/net/e100.c
>>> +++ b/drivers/net/e100.c
>>> @@ -1089,6 +1089,7 @@ static void e100_configure(struct nic *nic, struct
>>> cb *cb, struct sk_buff *skb)
>>>  {
>>>        struct config *config =&cb->u.config;
>>>        u8 *c = (u8 *)config;
>>> +       struct net_device *netdev = nic->netdev;
>>>
>>>        cb->command = cpu_to_le16(cb_config);
>>>
>>> @@ -1132,6 +1133,9 @@ static void e100_configure(struct nic *nic, struct
>>> cb *cb, struct sk_buff *skb)
>>>                config->promiscuous_mode = 0x1;         /* 1=on, 0=off */
>>>        }
>>>
>>> +       if (netdev->wanted_features&  NETIF_F_RXFCS)
>>> +               config->rx_crc_transfer = 0x1;  /* 1=save, 0=discard */
>>> +
>> You should check netdev->features here.
> I thought 'features' was what the NIC could support, and wanted_features
> was what the NIC was currently configured to support?  I don't want
> to rx the CRC all the time, just when users enable it...

hw_features is what device could support, and features is what device
has currently turned on.

Best Regards,
Michał Mirosław

Re: [RFC v2 2/2] e100: Support RXFCS feature flag.

[Ben Greear]

On 06/29/2011 07:33 AM, Michał Mirosław wrote:
> W dniu 29 czerwca 2011 16:22 użytkownik Ben Greear
> <greearb@candelatech.com>  napisał:
>> On 06/29/2011 04:37 AM, Michał Mirosław wrote:
>>> 2011/6/24<greearb@candelatech.com>:
>>>> From: Ben Greear<greearb@candelatech.com>
>>>>
>>>> This allows e100 to be configured to append the
>>>> Ethernet FCS to the skb.
>>>>
>>>> Useful for sniffing networks.
>>>>
>>>> Signed-off-by: Ben Greear<greearb@candelatech.com>
>>>> ---
>>>> :100644 100644 c1352c6... 761f6f5... M  drivers/net/e100.c
>>>>   drivers/net/e100.c |   15 ++++++++++++---
>>>>   1 files changed, 12 insertions(+), 3 deletions(-)
>>>>
>>>> diff --git a/drivers/net/e100.c b/drivers/net/e100.c
>>>> index c1352c6..761f6f5 100644
>>>> --- a/drivers/net/e100.c
>>>> +++ b/drivers/net/e100.c
>>>> @@ -1089,6 +1089,7 @@ static void e100_configure(struct nic *nic, struct
>>>> cb *cb, struct sk_buff *skb)
>>>>   {
>>>>         struct config *config =&cb->u.config;
>>>>         u8 *c = (u8 *)config;
>>>> +       struct net_device *netdev = nic->netdev;
>>>>
>>>>         cb->command = cpu_to_le16(cb_config);
>>>>
>>>> @@ -1132,6 +1133,9 @@ static void e100_configure(struct nic *nic, struct
>>>> cb *cb, struct sk_buff *skb)
>>>>                 config->promiscuous_mode = 0x1;         /* 1=on, 0=off */
>>>>         }
>>>>
>>>> +       if (netdev->wanted_features&    NETIF_F_RXFCS)
>>>> +               config->rx_crc_transfer = 0x1;  /* 1=save, 0=discard */
>>>> +
>>> You should check netdev->features here.
>> I thought 'features' was what the NIC could support, and wanted_features
>> was what the NIC was currently configured to support?  I don't want
>> to rx the CRC all the time, just when users enable it...
>
> hw_features is what device could support, and features is what device
> has currently turned on.

Ok, thanks for that correction.

What does wanted_features mean, then?

Thanks,
Ben

>
> Best Regards,
> Michał Mirosław


-- 
Ben Greear <greearb@candelatech.com>
Candela Technologies Inc  http://www.candelatech.com

Re: [RFC v2 2/2] e100: Support RXFCS feature flag.

[Michał Mirosław]

W dniu 29 czerwca 2011 16:35 użytkownik Ben Greear
<greearb@candelatech.com> napisał:
> On 06/29/2011 07:33 AM, Michał Mirosław wrote:
>> W dniu 29 czerwca 2011 16:22 użytkownik Ben Greear
>> <greearb@candelatech.com>  napisał:
>>> On 06/29/2011 04:37 AM, Michał Mirosław wrote:
>>>> 2011/6/24<greearb@candelatech.com>:
>>>>> From: Ben Greear<greearb@candelatech.com>
>>>>>
>>>>> This allows e100 to be configured to append the
>>>>> Ethernet FCS to the skb.
>>>>>
>>>>> Useful for sniffing networks.
>>>>>
>>>>> Signed-off-by: Ben Greear<greearb@candelatech.com>
>>>>> ---
>>>>> :100644 100644 c1352c6... 761f6f5... M  drivers/net/e100.c
>>>>>  drivers/net/e100.c |   15 ++++++++++++---
>>>>>  1 files changed, 12 insertions(+), 3 deletions(-)
>>>>>
>>>>> diff --git a/drivers/net/e100.c b/drivers/net/e100.c
>>>>> index c1352c6..761f6f5 100644
>>>>> --- a/drivers/net/e100.c
>>>>> +++ b/drivers/net/e100.c
>>>>> @@ -1089,6 +1089,7 @@ static void e100_configure(struct nic *nic,
>>>>> struct
>>>>> cb *cb, struct sk_buff *skb)
>>>>>  {
>>>>>        struct config *config =&cb->u.config;
>>>>>        u8 *c = (u8 *)config;
>>>>> +       struct net_device *netdev = nic->netdev;
>>>>>
>>>>>        cb->command = cpu_to_le16(cb_config);
>>>>>
>>>>> @@ -1132,6 +1133,9 @@ static void e100_configure(struct nic *nic,
>>>>> struct
>>>>> cb *cb, struct sk_buff *skb)
>>>>>                config->promiscuous_mode = 0x1;         /* 1=on, 0=off
>>>>> */
>>>>>        }
>>>>>
>>>>> +       if (netdev->wanted_features&    NETIF_F_RXFCS)
>>>>> +               config->rx_crc_transfer = 0x1;  /* 1=save, 0=discard */
>>>>> +
>>>>
>>>> You should check netdev->features here.
>>>
>>> I thought 'features' was what the NIC could support, and wanted_features
>>> was what the NIC was currently configured to support?  I don't want
>>> to rx the CRC all the time, just when users enable it...
>>
>> hw_features is what device could support, and features is what device
>> has currently turned on.
> Ok, thanks for that correction.
> What does wanted_features mean, then?

What user wants to be active. It should be more clear to you if you
read the implemetation: netdev_update_features() and friends.

Best Regards,
Michał Mirosław

Re: [RFC v2 2/2] e100: Support RXFCS feature flag.

[Ben Greear]

On 06/29/2011 08:06 AM, Michał Mirosław wrote:
> W dniu 29 czerwca 2011 16:35 użytkownik Ben Greear
> <greearb@candelatech.com>  napisał:
>> On 06/29/2011 07:33 AM, Michał Mirosław wrote:
>>> W dniu 29 czerwca 2011 16:22 użytkownik Ben Greear
>>> <greearb@candelatech.com>    napisał:
>>>> On 06/29/2011 04:37 AM, Michał Mirosław wrote:
>>>>> 2011/6/24<greearb@candelatech.com>:
>>>>>> From: Ben Greear<greearb@candelatech.com>
>>>>>>
>>>>>> This allows e100 to be configured to append the
>>>>>> Ethernet FCS to the skb.
>>>>>>
>>>>>> Useful for sniffing networks.
>>>>>>
>>>>>> Signed-off-by: Ben Greear<greearb@candelatech.com>
>>>>>> ---
>>>>>> :100644 100644 c1352c6... 761f6f5... M  drivers/net/e100.c
>>>>>>   drivers/net/e100.c |   15 ++++++++++++---
>>>>>>   1 files changed, 12 insertions(+), 3 deletions(-)
>>>>>>
>>>>>> diff --git a/drivers/net/e100.c b/drivers/net/e100.c
>>>>>> index c1352c6..761f6f5 100644
>>>>>> --- a/drivers/net/e100.c
>>>>>> +++ b/drivers/net/e100.c
>>>>>> @@ -1089,6 +1089,7 @@ static void e100_configure(struct nic *nic,
>>>>>> struct
>>>>>> cb *cb, struct sk_buff *skb)
>>>>>>   {
>>>>>>         struct config *config =&cb->u.config;
>>>>>>         u8 *c = (u8 *)config;
>>>>>> +       struct net_device *netdev = nic->netdev;
>>>>>>
>>>>>>         cb->command = cpu_to_le16(cb_config);
>>>>>>
>>>>>> @@ -1132,6 +1133,9 @@ static void e100_configure(struct nic *nic,
>>>>>> struct
>>>>>> cb *cb, struct sk_buff *skb)
>>>>>>                 config->promiscuous_mode = 0x1;         /* 1=on, 0=off
>>>>>> */
>>>>>>         }
>>>>>>
>>>>>> +       if (netdev->wanted_features&      NETIF_F_RXFCS)
>>>>>> +               config->rx_crc_transfer = 0x1;  /* 1=save, 0=discard */
>>>>>> +
>>>>>
>>>>> You should check netdev->features here.
>>>>
>>>> I thought 'features' was what the NIC could support, and wanted_features
>>>> was what the NIC was currently configured to support?  I don't want
>>>> to rx the CRC all the time, just when users enable it...
>>>
>>> hw_features is what device could support, and features is what device
>>> has currently turned on.
>> Ok, thanks for that correction.
>> What does wanted_features mean, then?
>
> What user wants to be active. It should be more clear to you if you
> read the implemetation: netdev_update_features() and friends.

I read it.  Seems the code won't let you turn on something not supported,
so if user wants RXFCS, then wanted_features will have it enabled.  So,
I'm not sure why my e100 patch would be wrong in that case.

Thanks,
Ben

>
> Best Regards,
> Michał Mirosław


-- 
Ben Greear <greearb@candelatech.com>
Candela Technologies Inc  http://www.candelatech.com

Re: [RFC v2 2/2] e100: Support RXFCS feature flag.

[Michał Mirosław]

W dniu 29 czerwca 2011 17:20 użytkownik Ben Greear
<greearb@candelatech.com> napisał:
> On 06/29/2011 08:06 AM, Michał Mirosław wrote:
>> W dniu 29 czerwca 2011 16:35 użytkownik Ben Greear
>> <greearb@candelatech.com>  napisał:
>>> On 06/29/2011 07:33 AM, Michał Mirosław wrote:
>>>>
>>>> W dniu 29 czerwca 2011 16:22 użytkownik Ben Greear
>>>> <greearb@candelatech.com>    napisał:
[...]
>>>>> I thought 'features' was what the NIC could support, and
>>>>> wanted_features
>>>>> was what the NIC was currently configured to support?  I don't want
>>>>> to rx the CRC all the time, just when users enable it...
>>>> hw_features is what device could support, and features is what device
>>>> has currently turned on.
>>> Ok, thanks for that correction.
>>> What does wanted_features mean, then?
>> What user wants to be active. It should be more clear to you if you
>> read the implemetation: netdev_update_features() and friends.
>
> I read it.  Seems the code won't let you turn on something not supported,
> so if user wants RXFCS, then wanted_features will have it enabled.  So,
> I'm not sure why my e100 patch would be wrong in that case.

wanted_features only reflects what is requested by user, this
combination might be invalid. When conditions change this value
combined with other bits in features are passed through
ndo_fix_features callback and netdev_fix_features() to bring it to
valid state, and then (if resulting set is different than current
features) ndo_set_features() is called to reconfigure device for that
new state change to it.

Best Regards,
Michał Mirosław

Re: [RFC v2 2/2] e100: Support RXFCS feature flag.

[Stephen Hemminger]

On Tue, 12 Jul 2011 17:49:49 +0200
Michał Mirosław <mirqus@gmail.com> wrote:

> wanted_features only reflects what is requested by user, this
> combination might be invalid. When conditions change this value
> combined with other bits in features are passed through
> ndo_fix_features callback and netdev_fix_features() to bring it to
> valid state, and then (if resulting set is different than current
> features) ndo_set_features() is called to reconfigure device for that
> new state change to it.

Since this semantic is more complicated than most other parts
of network device interface API; could you please put a detailed
documentation into Documentation/networking/netdevices.txt

The whole netdevices.txt document could use some extending and
rewriting as well.

Re: [RFC v2 2/2] e100: Support RXFCS feature flag.

[Ben Hutchings]

On Tue, 2011-07-12 at 09:00 -0700, Stephen Hemminger wrote:
> On Tue, 12 Jul 2011 17:49:49 +0200
> Michał Mirosław <mirqus@gmail.com> wrote:
> 
> > wanted_features only reflects what is requested by user, this
> > combination might be invalid. When conditions change this value
> > combined with other bits in features are passed through
> > ndo_fix_features callback and netdev_fix_features() to bring it to
> > valid state, and then (if resulting set is different than current
> > features) ndo_set_features() is called to reconfigure device for that
> > new state change to it.
> 
> Since this semantic is more complicated than most other parts
> of network device interface API; could you please put a detailed
> documentation into Documentation/networking/netdevices.txt
> 
> The whole netdevices.txt document could use some extending and
> rewriting as well.

Given that it has missed the last 3 years of API changes (and many
before that) I think it would be better to convert it into kernel-doc
comments in netdevice.h.  That would make it more obvious to driver
authors trying to understand the API, and to those changing the driver
API.

Ben.

-- 
Ben Hutchings, Senior Software Engineer, Solarflare
Not speaking for my employer; that's the marketing department's job.
They asked us to note that Solarflare product names are trademarked.

[PATCH] net: Add documentation for netdev features handling

[Michał Mirosław]

Signed-off-by: Michał Mirosław <mirq-linux@rere.qmqm.pl>
---

Please comment if something is unclear!
Apply otherwise. ;)

---
 Documentation/networking/netdev-features.txt |  155 ++++++++++++++++++++++++++
 1 files changed, 155 insertions(+), 0 deletions(-)

diff --git a/Documentation/networking/netdev-features.txt b/Documentation/networking/netdev-features.txt
new file mode 100644
index 0000000..9c209e6
--- /dev/null
+++ b/Documentation/networking/netdev-features.txt
@@ -0,0 +1,155 @@
+Netdev features mess and how to get out from it alive
+=====================================================
+
+Author:
+	Michał Mirosław <mirq-linux@rere.qmqm.pl>
+
+
+
+ Part I: Feature sets
+======================
+
+Long gone are days, when a network card would just take and give packets
+verbatim.  Todays devices add multiple features and bugs (read: offloads)
+that relieves OS of various tasks like generating and checking checksums,
+splitting packets, classifying them.  Those capabilities and their state
+is commonly referred to as netdev features in Linux kernel world.
+
+There are currently three sets of features relevant to the driver, and
+one used internally by network core:
+
+ 1. netdev->hw_features set contains features whose state may possibly
+    be changed (enabled or disabled) for a particular device by user's
+    request.  This set should be initialized in ndo_init callback and not
+    changed later.
+
+ 2. netdev->features set contains features which are currently enabled
+    for a device.  This should be changed only by network core or in
+    error paths of ndo_set_features callback.
+
+ 3. netdev->vlan_features set contains features whose state is inherited
+    by child VLAN devices (limits netdev->features set).  This is currently
+    used for all VLAN devices whether tags are stripped or inserted in
+    hardware or software.
+
+ 4. netdev->wanted_features set contains feature set requested by user.
+    This set is filtered by ndo_fix_features callback whenever it or
+    some device-specific conditions change. This set is internal to
+    networking core and should not be referenced in drivers.
+
+
+
+ Part II: Controlling enabled features
+=======================================
+
+When current feature set (netdev->features) is to be changed, new set
+is calculated and filtered by calling ndo_fix_features callback
+and netdev_fix_features(). If the resulting set differs from current
+set, it is passed to ndo_set_features callback and (if the callback
+returns success) replaces value stored in netdev->features.
+NETDEV_FEAT_CHANGE notification is issued after that whenever current
+set might have changed.
+
+Following events trigger recalculation:
+ 1. device's registration, after ndo_init returned success
+ 2. user requested changes in features state
+ 3. netdev_update_features() is called
+
+ndo_*_features callbacks are called with rtnl_lock held. Missing callbacks
+are treated as always returning success.
+
+Driver wanting to trigger recalculation must do so by calling
+netdev_update_features() while holding rtnl_lock. This should not be done
+from ndo_*_features callbacks. netdev->features should not be modified by
+driver except by means of ndo_fix_features callback.
+
+
+
+ Part III: Implementation hints
+================================
+
+ * ndo_fix_features:
+
+All dependencies between features should be resolved here. The resulting
+set can be reduced further by networking core imposed limitations (as coded
+in netdev_fix_features()). For this reason its safer to disable a feature
+when its dependencies are not met instead of forcing the dependency on.
+
+This callback should not modify hardware nor driver state (should be
+stateless).  It can be called multiple times between successive
+ndo_set_features calls.
+
+Callback must not alter features contained in NETIF_F_SOFT_FEATURES or
+NETIF_F_NEVER_CHANGE sets. The exception is NETIF_F_VLAN_CHALLENGED but
+care must be taken as the change won't affect already configured VLANs.
+
+ * ndo_set_features:
+
+Hardware should be reconfigured to match passed feature set. The should not
+be altered unless some error condition happens that can't be reliably
+detected in ndo_fix_features. In this case, the callback should update
+netdev->features to match resulting hardware state. Errors returned are
+not (and cannot be) propagated anywhere except dmesg. (Note: successful
+return is zero, >0 is silent error.) 
+
+
+
+ Part IV: Features
+===================
+
+For current list of features, see include/linux/netdev_features.h.
+This section describes semantics of some of them.
+
+ * Transmit checksumming
+
+For complete description, see comments near the top of include/linux/skbuff.h.
+
+Note: NETIF_F_HW_CSUM is a superset of NETIF_F_IP_CSUM + NETIF_F_IPV6_CSUM.
+It means that device can fill TCP/UDP-like checksum anywhere in the packets
+whatever headers there might be.
+
+ * Transmit TCP segmentation offload
+
+NETIF_F_TSO_ECN means that hardware can properly split packets with CWR bit
+set, be it TCPv4 (when NETIF_F_TSO is enabled) or TCPv6 (NETIF_F_TSO6).
+
+ * Transmit DMA from high memory
+
+On platforms where this is relevant, NETIF_F_HIGHDMA signals that
+ndo_start_xmit can handle skbs with frags in high memory.
+
+ * Transmit scatter-gather
+
+Those features say that ndo_start_xmit can handle fragmented skbs:
+NETIF_F_SG --- paged skbs (skb_shinfo()->frags), NETIF_F_FRAGLIST ---
+chained skbs (skb->next/prev list).
+
+ * Software features
+
+Features contained in NETIF_F_SOFT_FEATURES are a features of networking
+stack. Driver should not change behaviour based on them.
+
+ * LLTX driver (deprecated for hardware drivers)
+
+NETIF_F_LLTX should be set in drivers that implement their own locking in
+transmit path or don't need locking at all (e.g. software tunnels).
+In ndo_start_xmit, it is recommended to use a try_lock and return
+NETDEV_TX_LOCKED when the spin lock fails.  The locking should also properly
+protect against other callbacks (the rules you need to find out).
+
+Don't use it for new drivers.
+
+ * netns-local device
+
+NETIF_F_NETNS_LOCAL is set for devices that are not allowed to move between
+network namespaces (e.g. loopback).
+
+Don't use it in drivers.
+
+ * VLAN challenged
+
+NETIF_F_VLAN_CHALLENGED should be set for devices which can't cope with VLAN
+headers. Some drivers set this because the cards can't handle the bigger MTU.
+[FIXME: Those cases could be fixed in VLAN code by allowing only reduced-MTU
+VLANs. This may be not usefull, though.]
+
-- 
1.7.5.4

Re: [PATCH] net: Add documentation for netdev features handling

[Randy Dunlap]

On Tue, 12 Jul 2011 21:01:30 +0200 (CEST) Michał Mirosław wrote:

> Signed-off-by: Michał Mirosław <mirq-linux@rere.qmqm.pl>
> ---
> 
> Please comment if something is unclear!
> Apply otherwise. ;)
> 
> ---
>  Documentation/networking/netdev-features.txt |  155 ++++++++++++++++++++++++++
>  1 files changed, 155 insertions(+), 0 deletions(-)
> 
> diff --git a/Documentation/networking/netdev-features.txt b/Documentation/networking/netdev-features.txt
> new file mode 100644
> index 0000000..9c209e6
> --- /dev/null
> +++ b/Documentation/networking/netdev-features.txt
> @@ -0,0 +1,155 @@
> +Netdev features mess and how to get out from it alive
> +=====================================================
> +
> +Author:
> +	Michał Mirosław <mirq-linux@rere.qmqm.pl>
> +
> +
> +
> + Part I: Feature sets
> +======================
> +
> +Long gone are days, when a network card would just take and give packets

   Long gone are the days when

> +verbatim.  Todays devices add multiple features and bugs (read: offloads)

              Today's

> +that relieves OS of various tasks like generating and checking checksums,

        relieve an OS

> +splitting packets, classifying them.  Those capabilities and their state
> +is commonly referred to as netdev features in Linux kernel world.

   are

> +
> +There are currently three sets of features relevant to the driver, and
> +one used internally by network core:
> +
> + 1. netdev->hw_features set contains features whose state may possibly
> +    be changed (enabled or disabled) for a particular device by user's
> +    request.  This set should be initialized in ndo_init callback and not
> +    changed later.
> +
> + 2. netdev->features set contains features which are currently enabled
> +    for a device.  This should be changed only by network core or in
> +    error paths of ndo_set_features callback.
> +
> + 3. netdev->vlan_features set contains features whose state is inherited
> +    by child VLAN devices (limits netdev->features set).  This is currently
> +    used for all VLAN devices whether tags are stripped or inserted in
> +    hardware or software.
> +
> + 4. netdev->wanted_features set contains feature set requested by user.
> +    This set is filtered by ndo_fix_features callback whenever it or
> +    some device-specific conditions change. This set is internal to
> +    networking core and should not be referenced in drivers.
> +
> +
> +
> + Part II: Controlling enabled features
> +=======================================
> +
> +When current feature set (netdev->features) is to be changed, new set
> +is calculated and filtered by calling ndo_fix_features callback
> +and netdev_fix_features(). If the resulting set differs from current
> +set, it is passed to ndo_set_features callback and (if the callback
> +returns success) replaces value stored in netdev->features.
> +NETDEV_FEAT_CHANGE notification is issued after that whenever current
> +set might have changed.
> +
> +Following events trigger recalculation:

   The following events ...

> + 1. device's registration, after ndo_init returned success
> + 2. user requested changes in features state
> + 3. netdev_update_features() is called
> +
> +ndo_*_features callbacks are called with rtnl_lock held. Missing callbacks
> +are treated as always returning success.
> +
> +Driver wanting to trigger recalculation must do so by calling

   A driver that wants to trigger ...

> +netdev_update_features() while holding rtnl_lock. This should not be done
> +from ndo_*_features callbacks. netdev->features should not be modified by
> +driver except by means of ndo_fix_features callback.
> +
> +
> +
> + Part III: Implementation hints
> +================================
> +
> + * ndo_fix_features:
> +
> +All dependencies between features should be resolved here. The resulting
> +set can be reduced further by networking core imposed limitations (as coded
> +in netdev_fix_features()). For this reason its safer to disable a feature

                                              it is

> +when its dependencies are not met instead of forcing the dependency on.
> +
> +This callback should not modify hardware nor driver state (should be
> +stateless).  It can be called multiple times between successive
> +ndo_set_features calls.
> +
> +Callback must not alter features contained in NETIF_F_SOFT_FEATURES or
> +NETIF_F_NEVER_CHANGE sets. The exception is NETIF_F_VLAN_CHALLENGED but
> +care must be taken as the change won't affect already configured VLANs.
> +
> + * ndo_set_features:
> +
> +Hardware should be reconfigured to match passed feature set. The should not

                                                                The <what> should not

> +be altered unless some error condition happens that can't be reliably
> +detected in ndo_fix_features. In this case, the callback should update
> +netdev->features to match resulting hardware state. Errors returned are
> +not (and cannot be) propagated anywhere except dmesg. (Note: successful
> +return is zero, >0 is silent error.) 
> +
> +
> +
> + Part IV: Features
> +===================
> +
> +For current list of features, see include/linux/netdev_features.h.
> +This section describes semantics of some of them.
> +
> + * Transmit checksumming
> +
> +For complete description, see comments near the top of include/linux/skbuff.h.
> +
> +Note: NETIF_F_HW_CSUM is a superset of NETIF_F_IP_CSUM + NETIF_F_IPV6_CSUM.
> +It means that device can fill TCP/UDP-like checksum anywhere in the packets
> +whatever headers there might be.
> +
> + * Transmit TCP segmentation offload
> +
> +NETIF_F_TSO_ECN means that hardware can properly split packets with CWR bit
> +set, be it TCPv4 (when NETIF_F_TSO is enabled) or TCPv6 (NETIF_F_TSO6).
> +
> + * Transmit DMA from high memory
> +
> +On platforms where this is relevant, NETIF_F_HIGHDMA signals that
> +ndo_start_xmit can handle skbs with frags in high memory.
> +
> + * Transmit scatter-gather
> +
> +Those features say that ndo_start_xmit can handle fragmented skbs:
> +NETIF_F_SG --- paged skbs (skb_shinfo()->frags), NETIF_F_FRAGLIST ---
> +chained skbs (skb->next/prev list).
> +
> + * Software features
> +
> +Features contained in NETIF_F_SOFT_FEATURES are a features of networking

                                                   ^ drop "a"

> +stack. Driver should not change behaviour based on them.
> +
> + * LLTX driver (deprecated for hardware drivers)
> +
> +NETIF_F_LLTX should be set in drivers that implement their own locking in
> +transmit path or don't need locking at all (e.g. software tunnels).
> +In ndo_start_xmit, it is recommended to use a try_lock and return
> +NETDEV_TX_LOCKED when the spin lock fails.  The locking should also properly
> +protect against other callbacks (the rules you need to find out).
> +
> +Don't use it for new drivers.
> +
> + * netns-local device
> +
> +NETIF_F_NETNS_LOCAL is set for devices that are not allowed to move between
> +network namespaces (e.g. loopback).
> +
> +Don't use it in drivers.
> +
> + * VLAN challenged
> +
> +NETIF_F_VLAN_CHALLENGED should be set for devices which can't cope with VLAN
> +headers. Some drivers set this because the cards can't handle the bigger MTU.
> +[FIXME: Those cases could be fixed in VLAN code by allowing only reduced-MTU
> +VLANs. This may be not usefull, though.]

                          useful
> +
> -- 


---
~Randy
*** Remember to use Documentation/SubmitChecklist when testing your code ***

[PATCH v2] net: Add documentation for netdev features handling

[Michał Mirosław]

v2: incorporated suggestions from Randy Dunlap

Signed-off-by: Michał Mirosław <mirq-linux@rere.qmqm.pl>
---
 Documentation/networking/netdev-features.txt |  155 ++++++++++++++++++++++++++
 1 files changed, 155 insertions(+), 0 deletions(-)

diff --git a/Documentation/networking/netdev-features.txt b/Documentation/networking/netdev-features.txt
new file mode 100644
index 0000000..1ade16c
--- /dev/null
+++ b/Documentation/networking/netdev-features.txt
@@ -0,0 +1,155 @@
+Netdev features mess and how to get out from it alive
+=====================================================
+
+Author:
+	Michał Mirosław <mirq-linux@rere.qmqm.pl>
+
+
+
+ Part I: Feature sets
+======================
+
+Long gone are the days when a network card would just take and give packets
+verbatim.  Today's devices add multiple features and bugs (read: offloads)
+that relieve an OS of various tasks like generating and checking checksums,
+splitting packets, classifying them.  Those capabilities and their state
+are commonly referred to as netdev features in Linux kernel world.
+
+There are currently three sets of features relevant to the driver, and
+one used internally by network core:
+
+ 1. netdev->hw_features set contains features whose state may possibly
+    be changed (enabled or disabled) for a particular device by user's
+    request.  This set should be initialized in ndo_init callback and not
+    changed later.
+
+ 2. netdev->features set contains features which are currently enabled
+    for a device.  This should be changed only by network core or in
+    error paths of ndo_set_features callback.
+
+ 3. netdev->vlan_features set contains features whose state is inherited
+    by child VLAN devices (limits netdev->features set).  This is currently
+    used for all VLAN devices whether tags are stripped or inserted in
+    hardware or software.
+
+ 4. netdev->wanted_features set contains feature set requested by user.
+    This set is filtered by ndo_fix_features callback whenever it or
+    some device-specific conditions change. This set is internal to
+    networking core and should not be referenced in drivers.
+
+
+
+ Part II: Controlling enabled features
+=======================================
+
+When current feature set (netdev->features) is to be changed, new set
+is calculated and filtered by calling ndo_fix_features callback
+and netdev_fix_features(). If the resulting set differs from current
+set, it is passed to ndo_set_features callback and (if the callback
+returns success) replaces value stored in netdev->features.
+NETDEV_FEAT_CHANGE notification is issued after that whenever current
+set might have changed.
+
+The following events trigger recalculation:
+ 1. device's registration, after ndo_init returned success
+ 2. user requested changes in features state
+ 3. netdev_update_features() is called
+
+ndo_*_features callbacks are called with rtnl_lock held. Missing callbacks
+are treated as always returning success.
+
+A driver that wants to trigger recalculation must do so by calling
+netdev_update_features() while holding rtnl_lock. This should not be done
+from ndo_*_features callbacks. netdev->features should not be modified by
+driver except by means of ndo_fix_features callback.
+
+
+
+ Part III: Implementation hints
+================================
+
+ * ndo_fix_features:
+
+All dependencies between features should be resolved here. The resulting
+set can be reduced further by networking core imposed limitations (as coded
+in netdev_fix_features()). For this reason it is safer to disable a feature
+when its dependencies are not met instead of forcing the dependency on.
+
+This callback should not modify hardware nor driver state (should be
+stateless).  It can be called multiple times between successive
+ndo_set_features calls.
+
+Callback must not alter features contained in NETIF_F_SOFT_FEATURES or
+NETIF_F_NEVER_CHANGE sets. The exception is NETIF_F_VLAN_CHALLENGED but
+care must be taken as the change won't affect already configured VLANs.
+
+ * ndo_set_features:
+
+Hardware should be reconfigured to match passed feature set. The set
+should not be altered unless some error condition happens that can't
+be reliably detected in ndo_fix_features. In this case, the callback
+should update netdev->features to match resulting hardware state.
+Errors returned are not (and cannot be) propagated anywhere except dmesg.
+(Note: successful return is zero, >0 means silent error.) 
+
+
+
+ Part IV: Features
+===================
+
+For current list of features, see include/linux/netdev_features.h.
+This section describes semantics of some of them.
+
+ * Transmit checksumming
+
+For complete description, see comments near the top of include/linux/skbuff.h.
+
+Note: NETIF_F_HW_CSUM is a superset of NETIF_F_IP_CSUM + NETIF_F_IPV6_CSUM.
+It means that device can fill TCP/UDP-like checksum anywhere in the packets
+whatever headers there might be.
+
+ * Transmit TCP segmentation offload
+
+NETIF_F_TSO_ECN means that hardware can properly split packets with CWR bit
+set, be it TCPv4 (when NETIF_F_TSO is enabled) or TCPv6 (NETIF_F_TSO6).
+
+ * Transmit DMA from high memory
+
+On platforms where this is relevant, NETIF_F_HIGHDMA signals that
+ndo_start_xmit can handle skbs with frags in high memory.
+
+ * Transmit scatter-gather
+
+Those features say that ndo_start_xmit can handle fragmented skbs:
+NETIF_F_SG --- paged skbs (skb_shinfo()->frags), NETIF_F_FRAGLIST ---
+chained skbs (skb->next/prev list).
+
+ * Software features
+
+Features contained in NETIF_F_SOFT_FEATURES are features of networking
+stack. Driver should not change behaviour based on them.
+
+ * LLTX driver (deprecated for hardware drivers)
+
+NETIF_F_LLTX should be set in drivers that implement their own locking in
+transmit path or don't need locking at all (e.g. software tunnels).
+In ndo_start_xmit, it is recommended to use a try_lock and return
+NETDEV_TX_LOCKED when the spin lock fails.  The locking should also properly
+protect against other callbacks (the rules you need to find out).
+
+Don't use it for new drivers.
+
+ * netns-local device
+
+NETIF_F_NETNS_LOCAL is set for devices that are not allowed to move between
+network namespaces (e.g. loopback).
+
+Don't use it in drivers.
+
+ * VLAN challenged
+
+NETIF_F_VLAN_CHALLENGED should be set for devices which can't cope with VLAN
+headers. Some drivers set this because the cards can't handle the bigger MTU.
+[FIXME: Those cases could be fixed in VLAN code by allowing only reduced-MTU
+VLANs. This may be not useful, though.]
+
-- 
1.7.5.4

Re: [PATCH v2] net: Add documentation for netdev features handling

[David Miller]

From: Michał Mirosław <mirq-linux@rere.qmqm.pl>
Date: Tue, 12 Jul 2011 21:41:29 +0200 (CEST)

> v2: incorporated suggestions from Randy Dunlap
> 
> Signed-off-by: Michał Mirosław <mirq-linux@rere.qmqm.pl>

Applied.