Re: [PATCH net-next v2 06/12] ethtool: rss: don't report key if device doesn't support it

[Jakub Kicinski <kuba@kernel.org>]

[found this stuck in my outgoing mail :S]

On Mon, 5 Aug 2024 15:36:28 +0100 Edward Cree wrote:
> On 03/08/2024 05:26, Jakub Kicinski wrote:
> > marvell/otx2 and mvpp2 do not support setting different
> > keys for different RSS contexts. Contexts have separate
> > indirection tables but key is shared with all other contexts.
> > This is likely fine, indirection table is the most important
> > piece.  
> 
> Since drivers that do not support this are the odd ones out,
>  would it be better to invert the sense of the flag?  Or is
>  this to make sure that driver authors who don't think/know
>  about the distinction automatically get safe behaviour?

Yes, I wanted the 0 / default / sloppy choice to be the safe one.
As annoying as it is to have to set it in most drivers, I still
prefer that to the inevitable false-negatives.

> > Don't report the key-related parameters from such drivers.
> > This prevents driver-errors, e.g. otx2 always writes
> > the main key, even when user asks to change per-context key.
> > The second reason is that without this change tracking
> > the keys by the core gets complicated. Even if the driver
> > correctly reject setting key with rss_context != 0,
> > change of the main key would have to be reflected in
> > the XArray for all additional contexts.
> > 
> > Since the additional contexts don't have their own keys
> > not including the attributes (in Netlink speak) seems
> > intuitive. ethtool CLI seems to deal with it just fine.
> > 
> > Reviewed-by: Joe Damato <jdamato@fastly.com>
> > Signed-off-by: Jakub Kicinski <kuba@kernel.org>  
> ...
> > diff --git a/drivers/net/ethernet/sfc/ef100_ethtool.c b/drivers/net/ethernet/sfc/ef100_ethtool.c
> > index 746b5314acb5..127b9d6ade6f 100644
> > --- a/drivers/net/ethernet/sfc/ef100_ethtool.c
> > +++ b/drivers/net/ethernet/sfc/ef100_ethtool.c
> > @@ -58,6 +58,7 @@ const struct ethtool_ops ef100_ethtool_ops = {
> >  
> >  	.get_rxfh_indir_size	= efx_ethtool_get_rxfh_indir_size,
> >  	.get_rxfh_key_size	= efx_ethtool_get_rxfh_key_size,
> > +	.rxfh_per_ctx_key	= 1,  
> 
> I would prefer 'true' for the sfc drivers, I think that
>  better fits the general style of our code.

Sure thing.

> >  	.rxfh_priv_size		= sizeof(struct efx_rss_context_priv),
> >  	.get_rxfh		= efx_ethtool_get_rxfh,
> >  	.set_rxfh		= efx_ethtool_set_rxfh,

> > diff --git a/drivers/net/ethernet/sfc/siena/ethtool.c b/drivers/net/ethernet/sfc/siena/ethtool.c
> > index 4c182d4edfc2..6d4e5101433a 100644
> > --- a/drivers/net/ethernet/sfc/siena/ethtool.c
> > +++ b/drivers/net/ethernet/sfc/siena/ethtool.c
> > @@ -241,6 +241,7 @@ static int efx_ethtool_get_ts_info(struct net_device *net_dev,
> >  
> >  const struct ethtool_ops efx_siena_ethtool_ops = {
> >  	.cap_rss_ctx_supported	= true,
> > +	.rxfh_per_ctx_key	= true,  
> 
> For the record, Siena hardware doesn't actually support
>  custom RSS contexts; the code is only present in the
>  driver as a holdover from when Siena and EF10 used the
>  same driver.  Trying to actually use them on Siena will
>  fail -EOPNOTSUPP.[1]
> I'll send a patch to rip it out.

Ack, will drop this chunk to avoid conflicts, then.

> >  	.supported_coalesce_params = ETHTOOL_COALESCE_USECS |
> >  				     ETHTOOL_COALESCE_USECS_IRQ |
> >  				     ETHTOOL_COALESCE_USE_ADAPTIVE_RX,
> > diff --git a/include/linux/ethtool.h b/include/linux/ethtool.h
> > index 55c9f613ab64..16f72a556fe9 100644
> > --- a/include/linux/ethtool.h
> > +++ b/include/linux/ethtool.h
> > @@ -731,6 +731,8 @@ struct kernel_ethtool_ts_info {
> >   *	do not have to set this bit.
> >   * @cap_rss_sym_xor_supported: indicates if the driver supports symmetric-xor
> >   *	RSS.
> > + * @rxfh_per_ctx_key: device supports setting different RSS key for each
> > + *	additional context.  
> 
> This comment should really make clear that it covers hfunc and
>  input_xfrm as well, not just the key itself.

Ack.