Date:   Wed, 29 Jul 2020 17:54:08 +0300
From:   Moshe Shemesh <moshe@...lanox.com>
To:     Jakub Kicinski <kuba@...nel.org>,
        Jacob Keller <jacob.e.keller@...el.com>
Cc:     Jiri Pirko <jiri@...nulli.us>, netdev@...r.kernel.org,
        linux-kernel@...r.kernel.org,
        "David S. Miller" <davem@...emloft.net>,
        Jiri Pirko <jiri@...lanox.com>,
        Vasundhara Volam <vasundhara-v.volam@...adcom.com>
Subject: Re: [PATCH net-next RFC 01/13] devlink: Add reload level option to
 devlink reload command


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.

Powered by blists - more mailing lists