Re: [RFCv5 PATCH net-next 02/20] net: introduce operation helpers for netdev features

[Jakub Kicinski <kuba@kernel.org>]

On Sat, 16 Apr 2022 11:33:58 +0800 shenjian (K) wrote:
> 在 2022/3/25 9:09, Jakub Kicinski 写道:
> > On Thu, 24 Mar 2022 23:49:14 +0800 Jian Shen wrote:  
> >> Introduce a set of bitmap operation helpers for netdev features,
> >> then we can use them to replace the logical operation with them.
> >> As the nic driversare not supposed to modify netdev_features
> >> directly, it also introduces wrappers helpers to this.
> >>
> >> The implementation of these helpers are based on the old prototype
> >> of netdev_features_t is still u64. I will rewrite them on the last
> >> patch, when the prototype changes.
> >>
> >> Signed-off-by: Jian Shen <shenjian15@huawei.com>
> >> ---
> >>   include/linux/netdevice.h | 597 ++++++++++++++++++++++++++++++++++++++  
> > Please move these helpers to a new header file which won't be included
> > by netdevice.h and include it at users appropriately.  
> I introduced a new header file "netdev_features_helper",  and moved 
> thses helpers
> to it.  Some helpers need to include struct  net_device which defined in 
> netdevice.h,
> but there are also some inline functions in netdevice.h need to use 
> these netdev_features
> helpers. It's conflicted.
> 
> So far I thought 3 ways to solved it, but all of them are not satisfactory.
> 1) Split netdevice.h, move the definition of struct net_device and its 
> relative definitions to
> a new header file A( haven't got a reasonable name).  Both the 
> netdev_features_helper.h
> and the netdevice include A.
> 
> 2) Split netdevice.h, move the inline functions to a new header file B. 
> The netdev_features_helper.h
> inlucde netdevice.h， and B include netdev_features_helper.h and 
> netdevice.h. All the source files
> which using these ininline functions should include B.
> 
> 3) Split netdevice.h, move the inline functions to to 
> netdev_featurer_helper.h. The netdev_features_helper.h
> inlucde netdevice.h, All the source files which using these ininline 
> functions should include netde_features_helper.h.
> 
> I'd like to get more advice to this.

Larger surgery is probably too much. What does netdevice.h need? Looks
like it mostly needs the type and the helper for testing if feature is
set. So maybe we can put those in netdevice.h and the rest in a new
header?
More advanced helpers like netdev_get_wanted_features() can move to the
new header as well.