Messages in this thread | | | Subject | Re: [PATCH net-next RFC 01/13] devlink: Add reload level option to devlink reload command | From | Moshe Shemesh <> | Date | Wed, 29 Jul 2020 17:54:08 +0300 |
| |
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.
| |