Re: [PATCH net-next RFC 01/13] devlink: Add reload level option to devlink reload command

[Jakub Kicinski]

On Tue, 28 Jul 2020 12:18:30 -0700 Jacob Keller wrote:
> On 7/28/2020 11:44 AM, Jakub Kicinski wrote:
> > From user perspective what's important is what the reset achieves (and
> > perhaps how destructive it is). We can define the reset levels as:
> > 
> > $ devlink dev reload pci/0000:82:00.0 net-ns-respawn
> > $ devlink dev reload pci/0000:82:00.0 driver-param-init
> > $ devlink dev reload pci/0000:82:00.0 fw-activate
> > 
> > combining should be possible when user wants multiple things to happen:
> > 
> > $ devlink dev reload pci/0000:82:00.0 fw-activate driver-param-init
> 
> Where today "driver-param-init" is the default behavior. But didn't we
> just say that mlxsw also does the equivalent of fw-activate?

Actually the default should probably be the combination of
driver-param-init and net-ns-respawn.

My expectations would be that the driver must perform the lowest reset 
level possible that satisfies the requested functional change. 
IOW driver may do more, in fact it should be acceptable for the driver
to always for a full HW reset (unless --live or other constraint is
specified).

> > We can also add the "reset level" specifier - for the cases where
> > device is misbehaving:
> > 
> > $ devlink dev reload pci/0000:82:00.0 level [driver|fw|hardware]
> 
> I guess I don't quite see how level fits in? This is orthogonal to the
> other settings?

Yup, it is, it's already orthogonal to what reload does today, hence the
need for the "driver default" hack.

> > But I don't think that we can go from the current reload command
> > cleanly to just a level reset. The driver-specific default is a bad
> > smell which indicates we're changing semantics from what user wants 
> > to what the reset depth is. Our semantics with the patch as it stands
> > are in fact:
> >  - if you want to load new params or change netns, don't pass the level
> >    - the "driver default" workaround dictates the right reset level for
> >    param init;
> >  - if you want to activate new firmware - select the reset level you'd
> >    like from the reset level options.
> >   
> 
> I think I agree, having the "what gets reloaded" as a separate vector
> makes sense and avoids confusion. For example for ice hardware,
> "fw-activate" really does mean "Do an EMP reset" rather than just a
> "device reset" which could be interpreted differently. ice can do
> function level reset, or core device reset also. Neither of those two
> resets activates firmware.
> 
> Additionally the current function load process in ice doesn't support
> driver-init at all. That's something I'd like to see happen but is quite
> a significant refactor for how the driver loads. We need to refactor
> everything out so that devlink is created early on and factor out
> load/unload into handlers that can be called by the devlink reload. As I
> have primarily been focused on flash update I sort of left that for the
> future because it was a huge task to solve.

Cool! That was what I was concerned about, but I didn't know any
existing driver already has the problem. "FW reset" is not nearly
a clear enough operation. We'd end up with drivers differing and 
users having to refer to vendor documentation to find out which 
"reset level" maps to what.

I think the components in ethtool-reset try to address the same
problem, and they have the notion of per-port, and per-device.
In the modern world we lack the per-host notion, but that's still 
strictly clearer than the limited API proposed here.