Re: [PATCH net] net: ethtool: fix off-by-one error in max RSS context IDs

[Jakub Kicinski <kuba@kernel.org>]

On Wed, 7 Aug 2024 11:30:54 +0100 Edward Cree wrote:
> > also1 if we want to switch to exclusive I maintain we should rename the
> > field  
> 
> Okay, will do.  I misunderstood your "if we change the definition
>  of the field" remark, because in my head I'm not changing the
>  definition ;)
> How about rxfh_max_num_contexts?

As good as anything I can come up with :)
It's hard to name it as "this is just for width of the contexts"
without implying that it's inclusive :S
I was thinking about hinting at this being the limit fed into XArray,
but Xarray's limit is inclusive.
Another thought I had was FIELD_MAX(), again, inclusive :D

Maybe we can forgo the max as it could imply max value, and insert id
instead because we're talking about ids not contexts?

rxfh_context_id_cnt ? Or give up and rxfh_field1_read_the_doc ;)

> > also2 check that it's not 1 during registration, that'd be nonsense  
> 
> Fair enough.
> 
> > also3 you're breaking bnxt, it _wants_ 32 to be a valid ID, with max 32  
> 
> Fwiw the limit in bnxt existed purely for the sake of the bitmap[1]
>  which you removed when you converted it to the new API.
> My reading of the bnxt code is that context allocation happens via
>  a firmware RPC.  Pavan, if the firmware can be trusted to reject
>  this RPC when it has no contexts left to give, then you shouldn't
>  need an rxfh_max_context_id in the driver at all and you can
>  remove it from ethtool_ops for net-next.
> To avoid a regression in net I'll change it to 33 in my patch.
> 
> (Typically rxfh_max_context_id is only needed if either driver or
>  firmware is using the context ID as an index into a fixed-size
>  array.  This is why I consider an exclusive limit -- which would
>  be set to an ARRAY_SIZE() -- more appropriate.)

Got it. I had the vague plan of piggy backing on this to express
the order of magnitude of how many contexts are supported, but FWIW
I no longer think it's a good fit, anyway.