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

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite for Android: free password hash cracker in your pocket

[<prev] [next>] [<thread-prev] [day] [month] [year] [list]

Message-ID: <4172ae73-a43d-18be-aca6-9dd02b69ac09@huawei.com>
Date:   Thu, 15 Jul 2021 14:34:38 +0800
From:   "shenjian (K)" <shenjian15@...wei.com>
To:     Andrew Lunn <andrew@...n.ch>
CC:     <davem@...emloft.net>, <kuba@...nel.org>, <netdev@...r.kernel.org>,
        <linuxarm@...neuler.org>,
        "shenjian15@...wei.com" <shenjian15@...wei.com>
Subject: Re: [RFC net-next] net: extend netdev features



在 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.