[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]
Message-ID: <20240806070702.3359e2fe@kernel.org>
Date: Tue, 6 Aug 2024 07:07:02 -0700
From: Jakub Kicinski <kuba@...nel.org>
To: Edward Cree <ecree.xilinx@...il.com>
Cc: davem@...emloft.net, netdev@...r.kernel.org, edumazet@...gle.com,
pabeni@...hat.com, dxu@...uu.xyz, przemyslaw.kitszel@...el.com,
donald.hunter@...il.com, gal.pressman@...ux.dev, tariqt@...dia.com,
willemdebruijn.kernel@...il.com, jdamato@...tly.com
Subject: Re: [PATCH net-next v2 06/12] ethtool: rss: don't report key if
device doesn't support it
[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@...tly.com>
> > Signed-off-by: Jakub Kicinski <kuba@...nel.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.
Powered by blists - more mailing lists