Re: [PATCH net-next RFC 7/7] bnxt_en: Add bnxt_en initial port params table and register it

[Jakub Kicinski <jakub.kicinski@netronome.com>]

On Wed, 5 Dec 2018 17:18:52 -0800, Michael Chan wrote:
> On Wed, Dec 5, 2018 at 4:42 PM Jakub Kicinski wrote:
> > On Wed, 5 Dec 2018 16:01:08 -0800, Michael Chan wrote:  
> > > On Wed, Dec 5, 2018 at 3:33 PM Jakub Kicinski wrote:
> > > > On Wed,  5 Dec 2018 11:27:00 +0530, Vasundhara Volam wrote:  
> > > > > Register devlink_port with devlink and create initial port params
> > > > > table for bnxt_en. The table consists of a generic parameter:
> > > > >
> > > > > wake-on-lan: Enables Wake on Lan for this port when magic packet
> > > > > is received with this port's MAC address using ACPI pattern.
> > > > > If enabled, the controller asserts a wake pin upon reception of
> > > > > WoL packet.  ACPI (Advanced Configuration and Power Interface) is
> > > > > an industry specification for the efficient handling of power
> > > > > consumption in desktop and mobile computers.
> > > > >
> > > > > Cc: Michael Chan <michael.chan@broadcom.com>
> > > > > Signed-off-by: Vasundhara Volam <vasundhara-v.volam@broadcom.com>  
> > > >
> > > > Why do we need a WoL as a devlink parameter (rather than ethtool -s)?  
> > >
> > > I believe ethtool -s for WoL is a non-persistent setting, meaning that
> > > if you power cycle the system, the WoL setting will go back to
> > > default.
> > >
> > > devlink on the other hand is a permanent setting.  ethtool should
> > > initially report the default WoL setting and it can then be changed
> > > (in a non permanent way) using ethtool -s.  
> >
> > All network configuration settings in Linux are non-persistent AFAIK.
> > That's why network configuration daemons exist:
> >
> > https://wiki.debian.org/WakeOnLan
> >
> > Perhaps the objective to move more of the network configuration into the
> > firmware?  That'd be a bleak scenario, so probably not..
> >
> > My understanding was the persistent devlink settings are for things
> > which have to be set at device init time.  Like say PCI endpoint
> > configuration.  FW loading configuration.
> >
> > Besides, the parameter you add is just true/false, when ethtool has
> > multiple options.
> >
> > It feels to me like we moved from ioctls to Netlink, and now even
> > before ethtool was converted to Netlink we may move to unstructured
> > strings.  That's not a step forward, if you ask me.  
> 
> We do have a parameter in NVRAM that controls default WoL.  I think
> this is to expose that parameter so it can be set one way or the
> other. There are scenarios where Linux has not booted yet (and so
> there is no opportunity to run ethtool -s or any daemons yet) and this
> parameter will control whether the machine will wake up or not.

Isn't that set in BIOS/setup?  The config before any OS boots?  Because
the BMC or whatnot has to actually configure the board to power
appropriate things up.  Please clarify.

And *if* it is proven this config is more than just setting the default
IMHO the setting belongs in the ethtool API.  We can't just add devlink
params for all existing config APIs just because it has persistence.