Re: [PATCH net-next RFC 01/13] devlink: Add reload level option to devlink reload command
From: Moshe Shemesh
Date: Wed Jul 29 2020 - 10:54:17 EST
On 7/28/2020 11:06 PM, Jakub Kicinski wrote:
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.
What about the support of these combinations, one device needs to reset
fw to apply the param init,
while another device can apply param-init without fw reset, but has to
reload the driver for fw-reset.
So the support per driver will be a matrix of combinations ?
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).
OK, but some combinations may still not be valid for specific driver
even if it tries lowest level possible.
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.