Re: [Intel-wired-lan] [PATCH net-next v6 1/7] net: ethtool: pass ethtool_rxfh to get/set_rxfh ethtool ops

From: Jakub Kicinski <kuba@kernel.org>
To: Ahmed Zaki <ahmed.zaki@intel.com>
Cc: mkubecek@suse.cz, andrew@lunn.ch,
	willemdebruijn.kernel@gmail.com, corbet@lwn.net,
	netdev@vger.kernel.org, gal@nvidia.com,
	linux-doc@vger.kernel.org, jesse.brandeburg@intel.com,
	Igor Bagnucki <igor.bagnucki@intel.com>,
	edumazet@google.com, anthony.l.nguyen@intel.com,
	horms@kernel.org, vladimir.oltean@nxp.com,
	Jacob Keller <jacob.e.keller@intel.com>,
	intel-wired-lan@lists.osuosl.org, pabeni@redhat.com,
	davem@davemloft.net
Subject: Re: [Intel-wired-lan] [PATCH net-next v6 1/7] net: ethtool: pass ethtool_rxfh to get/set_rxfh ethtool ops
Date: Tue, 21 Nov 2023 15:29:06 -0800	[thread overview]
Message-ID: <20231121152906.2dd5f487@kernel.org> (raw)
In-Reply-To: <20231120205614.46350-2-ahmed.zaki@intel.com>

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.

 - Add a rss_context member to the argument struct and a capability
   like cap_link_lanes_supported to indicate whether driver supports
   rss contexts, then you can remove *et_rxfh_context functions,
   and instead call *et_rxfh() with a non-zero rss_context.

 - Add your new member to the struct wrapping all params.

If you just expose struct ethtool_rxfh to the drivers (a) there are
fields in there drivers shouldn't touch, and (b) that struct is uAPI
so we can't add netlink-only fields easily.
_______________________________________________
Intel-wired-lan mailing list
Intel-wired-lan@osuosl.org
https://lists.osuosl.org/mailman/listinfo/intel-wired-lan