On Wed, Jan 23, 2019 at 3:48 AM Jakub Kicinski
[off-list ref] wrote:

On Fri, 18 Jan 2019 15:33:19 +0100, Michal Kubecek wrote:

quoted

On Fri, Jan 18, 2019 at 12:39:37PM +0530, Vasundhara Volam wrote:

quoted

There is difference of opinion on adding WOL parameter to devlink, between
Jakub Kicinski and Michael Chan.

Quote from Jakud Kicinski:
********
As explained previously I think it's a very bad idea to add existing
configuration options to devlink, just because devlink has the ability
to persist the setting in NVM.  Especially that for WoL you have to get
the link up so you potentially have all link config stuff as well.  And
that n-tuple filters are one of the WoL options, meaning we'd need the
ability to persist n-tuple filters via devlink.

The effort would be far better spent helping with migrating ethtool to
netlink, and allowing persisting there.

I have not heard any reason why devlink is a better fit.  I can imagine
you're just doing it here because it's less effort for you since
ethtool is not yet migrated.
********

Quote from Michael Chan:
********
The devlink's WoL parameter is a persistent WoL parameter stored in the
NIC's NVRAM. It is different from ethtool's WoL parameter in a number of
ways. ethtool WoL is not persistent over AC power cycle and is considered
OS-present WoL. As such, ethtool WoL can use a more sophisticated pattern
including n-tuple with IP address in addition to the more basic types
(e.g. magic packet). Whereas OS-absent power up WoL should only include
magic packet and other simple types.

If I understand correctly, it's that way now. I'm not sure there is a
technical reason preventing more complex WoL types in the OS-absent case
in the future. Also, even with traditional ethtool WoL setting, most
NICs only support some of the types (I'm not sure if there is a NIC
which would support all of them.)

quoted

The devlink WoL setting does not have to match the ethtool WoL
setting.

IMHO this is not really a problem. We can either use an additional flag
telling kernel/driver if we are setting runtime or persistent WoL mask
or we can pass (up to) two bitmaps.

Jakub, I will add another two bitmask parameters for WoL named as
wake_on_lan_runtime
and wake_on_lan_persistent. This will give information about what
types are runtime and
what types are persistent, does the device support for any given WoL types.

Does that sound good?

I think reusing new netlink ethtool with a special flag would be a nice,
complete solution.  We could also address link settings this way (which
are a pre-requisite for WoL).

I have no strong preference on the mechanism, but for ease of setting
as well as dumping would it be workable to use a nesting, like this:

Run time settings:
  [ETHTOOL_SETTINGS_BLA]
    [ETHTOOL_BLA_VAL_1]
    [ETHTOOL_BLA_VAL_2]
    ...

Persistent:
  [ETHTOOL_PERSISTENT]
    [ETHTOOL_SETTINGS_BLA]
      [ETHTOOL_BLA_VAL_1]
      [ETHTOOL_BLA_VAL_2]

IOW encapsulate settings into a "persistent" attribute?

How does that look to you, Michal?

quoted

quoted

The card will autoneg up to the speed supported by Vaux so no special
devlink link setting is needed.
********

Like Jakub, I'm not convinced there is a strong technical reason to have
each of the WoL settings handled through a different interface. I don't
say, though, that ethtool is necessarily the right one. If there is
a consensus that it better fits into devlink, I can imagine that both
could be accessible through devlink (for start, in drivers which choose
so, e.g. because they want to implement the persistent setting).

Michal Kubecek