Re: [RFC net-next] net: extend netdev features

Hi Andrew,

Thanks for your advice.

I have thought of using linkmode_ before (https://lists.openwall.net/netdev/
2021/06/19/11).

In my humble opinion, there are too many logical operations in stack and
ethernet drivers,
I'm not sure changing them to the linkmode_set_ API is the best way. For 
example,

below is codes in ethernet drivre:
netdev->features |= NETIF_F_HW_VLAN_CTAG_FILTER |
        NETIF_F_HW_VLAN_CTAG_TX | NETIF_F_HW_VLAN_CTAG_RX |
        NETIF_F_RXCSUM | NETIF_F_SG | NETIF_F_GSO |
        NETIF_F_GRO | NETIF_F_TSO | NETIF_F_TSO6 | NETIF_F_GSO_GRE |
        NETIF_F_GSO_GRE_CSUM | NETIF_F_GSO_UDP_TUNNEL |
        NETIF_F_SCTP_CRC | NETIF_F_FRAGLIST;

When using linkmode_ API, I have two choices, one is to define several feature
arrays or
a whole features array including all the feature bits supported by the ethernet
driver.
const int hns3_nic_vlan_features_array[] = {
    NETIF_F_HW_VLAN_CTAG_FILTER,
    NETIF_F_HW_VLAN_CTAG_TX,
    NETIF_F_HW_VLAN_CTAG_RX,
};
const int hns3_nic_tso_features_array[] = {
    NETIF_F_GSO,
    NETIF_F_TSO,
    NETIF_F_TSO6,
    NETIF_F_GSO_GRE,
    ...
};

Using arrays is the way to go. Hopefully Coccinelle can do most of the
work for you.

linkmode_set_bit_array(hns3_nic_vlan_features_array, ARRAY_SIZE
(hns3_nic_vlan_features_array), netedv->features);

I would probably add wrappers. So an API like

netdev_feature_set_array(ndev, int hns3_nic_tso_features_array),
                         ARRAY_SIZE(int hns3_nic_tso_features_array);

netdev_hw_feature_set_array(ndev, int hns3_nic_tso_features_array),
                            ARRAY_SIZE(int hns3_nic_vlan_features_array);

etc. This will help during the conversion. You can keep
netdev_features_t as a u64 while you convert to the mew API. Once the
conversion is done, you can then convert the implementation of the API
to a linux bitmap.

When using netdev_feature_t *, then just need to add an arrary index.

netdev->features[0] |= NETIF_F_HW_VLAN_CTAG_FILTER |
        NETIF_F_HW_VLAN_CTAG_TX | NETIF_F_HW_VLAN_CTAG_RX | xxxxxx

And you want to add

netdev->features[1] |= NETIF_F_NEW_FEATURE;

This is going to result in errors, where developers add
NETIF_F_NEW_FEATURE to feature[0], and the compiler will happily do
it, no warnings. Either you need the compiler to enforce the correct
assignment to the array members somehow, or you need a uniform API
which you cannot get wrong.

By the way, could you introduce me some code re-writing tools ? 

Coccinelle.

https://www.kernel.org/doc/html/latest/dev-tools/coccinelle.html
https://coccinelle.gitlabpages.inria.fr/website/

I've no idea if it can do this level of code re-write. You probably
want to post on the mailing list, describe what you want to do.

     Andrew

在 2021/7/13 21:53, Andrew Lunn 写道:

quoted

Hi Andrew,

Thanks for your advice.

I have thought of using linkmode_ before (https://lists.openwall.net/netdev/
2021/06/19/11).

In my humble opinion, there are too many logical operations in stack and
ethernet drivers,
I'm not sure changing them to the linkmode_set_ API is the best way. For
example,

below is codes in ethernet drivre:
netdev->features |= NETIF_F_HW_VLAN_CTAG_FILTER |
         NETIF_F_HW_VLAN_CTAG_TX | NETIF_F_HW_VLAN_CTAG_RX |
         NETIF_F_RXCSUM | NETIF_F_SG | NETIF_F_GSO |
         NETIF_F_GRO | NETIF_F_TSO | NETIF_F_TSO6 | NETIF_F_GSO_GRE |
         NETIF_F_GSO_GRE_CSUM | NETIF_F_GSO_UDP_TUNNEL |
         NETIF_F_SCTP_CRC | NETIF_F_FRAGLIST;

When using linkmode_ API, I have two choices, one is to define several feature
arrays or
a whole features array including all the feature bits supported by the ethernet
driver.
const int hns3_nic_vlan_features_array[] = {
     NETIF_F_HW_VLAN_CTAG_FILTER,
     NETIF_F_HW_VLAN_CTAG_TX,
     NETIF_F_HW_VLAN_CTAG_RX,
};
const int hns3_nic_tso_features_array[] = {
     NETIF_F_GSO,
     NETIF_F_TSO,
     NETIF_F_TSO6,
     NETIF_F_GSO_GRE,
     ...
};

Using arrays is the way to go. Hopefully Coccinelle can do most of the
work for you.

quoted

linkmode_set_bit_array(hns3_nic_vlan_features_array, ARRAY_SIZE
(hns3_nic_vlan_features_array), netedv->features);

I would probably add wrappers. So an API like

netdev_feature_set_array(ndev, int hns3_nic_tso_features_array),
                          ARRAY_SIZE(int hns3_nic_tso_features_array);

netdev_hw_feature_set_array(ndev, int hns3_nic_tso_features_array),
                             ARRAY_SIZE(int hns3_nic_vlan_features_array);

etc. This will help during the conversion. You can keep
netdev_features_t as a u64 while you convert to the mew API. Once the
conversion is done, you can then convert the implementation of the API
to a linux bitmap.

This way looks much better. I'm being troubled to split these changes 
from the
single huge patch. These wrappers can help. Thanks!

quoted

When using netdev_feature_t *, then just need to add an arrary index.

netdev->features[0] |= NETIF_F_HW_VLAN_CTAG_FILTER |
         NETIF_F_HW_VLAN_CTAG_TX | NETIF_F_HW_VLAN_CTAG_RX | xxxxxx

And you want to add

netdev->features[1] |= NETIF_F_NEW_FEATURE;

This is going to result in errors, where developers add
NETIF_F_NEW_FEATURE to feature[0], and the compiler will happily do
it, no warnings. Either you need the compiler to enforce the correct
assignment to the array members somehow, or you need a uniform API
which you cannot get wrong.

Yes, this is prone to error, and compiler won't warn it. I will 
reconsider the
array and linux bitmap.

quoted

By the way, could you introduce me some code re-writing tools ?

Coccinelle.

https://www.kernel.org/doc/html/latest/dev-tools/coccinelle.html
https://coccinelle.gitlabpages.inria.fr/website/
I've no idea if it can do this level of code re-write. You probably
want to post on the mailing list, describe what you want to do.

      Andrew
.

Thanks, I'll see how to use it.