> -----Original Message----- > From: Zaki, Ahmed <ahmed.zaki@xxxxxxxxx> > Sent: Monday, November 27, 2023 6:15 AM > To: Jakub Kicinski <kuba@xxxxxxxxxx> > Cc: netdev@xxxxxxxxxxxxxxx; intel-wired-lan@xxxxxxxxxxxxxxxx; corbet@xxxxxxx; > Brandeburg, Jesse <jesse.brandeburg@xxxxxxxxx>; Nguyen, Anthony L > <anthony.l.nguyen@xxxxxxxxx>; davem@xxxxxxxxxxxxx; edumazet@xxxxxxxxxx; > pabeni@xxxxxxxxxx; vladimir.oltean@xxxxxxx; andrew@xxxxxxx; > horms@xxxxxxxxxx; mkubecek@xxxxxxx; willemdebruijn.kernel@xxxxxxxxx; > gal@xxxxxxxxxx; alexander.duyck@xxxxxxxxx; linux-doc@xxxxxxxxxxxxxxx; > Bagnucki, Igor <igor.bagnucki@xxxxxxxxx>; Keller, Jacob E > <jacob.e.keller@xxxxxxxxx> > Subject: Re: [PATCH net-next v6 1/7] net: ethtool: pass ethtool_rxfh to > get/set_rxfh ethtool ops > > > > On 2023-11-21 16:29, Jakub Kicinski wrote: > > On Mon, 20 Nov 2023 13:56:08 -0700 Ahmed Zaki wrote: > >> u32 (*get_rxfh_key_size)(struct net_device *); > >> u32 (*get_rxfh_indir_size)(struct net_device *); > >> - int (*get_rxfh)(struct net_device *, u32 *indir, u8 *key, > >> - u8 *hfunc); > >> - int (*set_rxfh)(struct net_device *, const u32 *indir, > >> - const u8 *key, const u8 hfunc); > >> + int (*get_rxfh)(struct net_device *, struct ethtool_rxfh *, > >> + u32 *indir, u8 *key); > >> + int (*set_rxfh)(struct net_device *, struct ethtool_rxfh *, > >> + const u32 *indir, const u8 *key); > >> int (*get_rxfh_context)(struct net_device *, u32 *indir, u8 *key, > >> u8 *hfunc, u32 rss_context); > >> int (*set_rxfh_context)(struct net_device *, const u32 *indir, > > > > This conversion looks 1/4th done. You should do the following: > > > > - First simplify the code by always providing a pointer to all params > > (indir, key and func); the fact that some of them may be NULL seems > > like a weird historic thing or a premature optimization. > > It will simplify the drivers if all pointers are always present. > > You don't have to remove the if () checks in the existing drivers. > > > > - Then make the functions take a dev pointer, and a pointer to a > > single struct wrapping all arguments. The set_* should also take > > an extack. > > Can we skip the "extack" part for this series? There is no > "ETHTOOL_MSG_RSS_SET" netlink message, which is needed for user-space to > get the ACK and adding all the netlink stuff seems a bit out of scope. > > I will do the rest in the next version. Please include the extack now even if there isn't an immediate user. A NULL value is acceptable to pass for "there is no extended ACK available", but this way we don't have to modify the drivers *again* when the extack is available if we add a netlink op in the future. Thanks, Jake
