在 2021/7/13 21:53, Andrew Lunn 写道:
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.
This way looks much better. I'm being troubled to split these changes from the single huge patch. These wrappers can help. Thanks!
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.
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.