Re: [PATCH net-next 6/8] bnxt_en: Implement ethtool -X to set indirection table.

[Jakub Kicinski <kuba@kernel.org>]

On Mon, 29 Jun 2020 17:38:33 -0700 Michael Chan wrote:
> On Mon, Jun 29, 2020 at 5:06 PM Jakub Kicinski <kuba@kernel.org> wrote:
> > On Mon, 29 Jun 2020 02:34:22 -0400 Michael Chan wrote:
> > > With the new infrastructure in place, we can now support the setting of
> > > the indirection table from ethtool.
> > >
> > > The user-configured indirection table will need to be reset to default
> > > if we are unable to reserve the requested number of RX rings or if the
> > > RSS table size changes.
> > >
> > > Signed-off-by: Michael Chan <michael.chan@broadcom.com>  
> >
> > Hm. Clearing IFF_RXFH_CONFIGURED seems wrong. The user has clearly
> > requested a RSS mapping, if it can't be maintained driver should
> > return an error from the operation which attempts to change the ring
> > count.  
> 
> Right.  In this case the user has requested non default RSS map and is
> now attempting to change rings.  We have a first level check by
> calling bnxt_check_rings().  Firmware will tell us if the requested
> rings are available or not.  If not, we will return error and the
> existing rings and RSS map will be kept.  This should be the expected
> outcome in most cases.
> 
> In rare cases, firmware can return success during bnxt_check_rings()
> but during the actual ring reservation, it fails to reserve all the
> rings it promised were available earlier.  In this case, we fall back
> and accept the fewer rings and set the RSS map to default.  I have
> never seen this scenario but we need to put the code in just in case
> it happens.  It should be rare.

What's the expected application flow? Every time the application 
makes a change to NIC settings it has to re-validate that some of 
the previous configuration didn't get lost? I don't see the driver
returning the error if FW gave it less rings than requested. There 
isn't even a warning printed..

I'd prefer if the driver wrapped the rss indexes, and printed a
warning, but left the config intact. And IMO set_channels should 
return an error.