Message-ID: <CAACQVJrOYFb9Ps-4GzfJGd7ejyny_P4PdPZp6-feAHhB1Km=Zg@mail.gmail.com>
Date:   Thu, 20 Dec 2018 14:46:03 +0530
From:   Vasundhara Volam <vasundhara-v.volam@...adcom.com>
To:     David Miller <davem@...emloft.net>
Cc:     "michael.chan@...adcom.com" <michael.chan@...adcom.com>,
        Jiri Pirko <jiri@...lanox.com>, jakub.kicinski@...ronome.com,
        mkubecek@...e.cz, Netdev <netdev@...r.kernel.org>
Subject: Re: [PATCH net-next v5 RFC 0/8] devlink: Add configuration parameters
 support for devlink_port

Hi Dave,

Resent the same patchset, as only few patches are sent previously with
error "End data with <CR><LF>.<CR><LF>".

Thanks,
Vasundhara

On Thu, Dec 20, 2018 at 2:41 PM Vasundhara Volam
<vasundhara-v.volam@...adcom.com> wrote:
>
> This patchset adds support for configuration parameters setting through
> devlink_port.  Each device registers supported configuration parameters
> table.
>
> The user can retrieve data on these parameters by
> "devlink port param show" command and can set new value to a
> parameter by "devlink port param set" command.
> All configuration modes supported by devlink_dev are supported
> by devlink_port also.
>
> Command examples and output:
>
> # devlink port param show
> pci/0000:3b:00.0/0:
>   name wake-on-lan type generic
>     values:
>       cmode permanent value false
>
> pci/0000:3b:00.1/1:
>   name wake-on-lan type generic
>     values:
>       cmode permanent value false
>
> pci/0000:af:00.0/0:
>   name wake-on-lan type generic
>     values:
>       cmode permanent value true
>
> # devlink port param show pci/0000:3b:00.0/0 name wake-on-lan
> pci/0000:3b:00.0/0:
>   name wake-on-lan type generic
>     values:
>       cmode permanent value false
>
> # devlink port param set pci/0000:3b:00.0/0 name wake-on-lan cmode permanent value true
>
> 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. The devlink WoL setting does not have
> to match the ethtool WoL setting. The card will autoneg up to the speed
> supported by Vaux so no special devlink link setting is needed.
> --------
>
> v4->v5:
> - Added quotes from Jakub Kicinski and Michael chan on devlink's WOL
>   parameter in the cover letter.
>
> v3->v4:
> - Update changes done from v2 to v3 version in individual patch
>   descriptions.
>
> v2->v3:
> Make following changes as per suggestions from Jiri Pirko and
> Michal Kubecek.
> - Add a helper __devlink_params_register() with common code used by
>   both devlink_params_register() and devlink_port_params_register().
> - Define only WOL types used now and define them as bitfield, so that
>   mutliple WOL types can be enabled upon power on.
> - Modify "wake-on-lan" name to "wake_on_lan" to be symmetric with
>   previous definitions.
> - Rename DEVLINK_PARAM_WOL_XXX to DEVLINK_PARAM_WAKE_XXX to be
>   symmetrical with ethtool WOL definitions.
> - Modify bnxt_dl_wol_validate(), to throw error message when user gives
>   value other than DEVLINK_PARAM_WAKE_MAGIC or to disable WOL.
> - Use netdev_err() instead of netdev_warn(), when devlink_port_register()
>   and devlink_port_params_register() returns error. Also, don't log rc
>   in this message.
>
> v1->v2:
> Make following changes as per suggestions from Jiri Pirko.
> - Remove separate enum devlink_port_param_generic_id for port params.
>   Instead club it with existing device params. Accordingly refactor
>   remaining patchset.
> - Move INIT_LIST_HEAD of port param_list to devlink_port_register()
> - Add a helper devlink_param_verify() to be used for both
>   devlink_params_register() and devlink_port_params_register().
> - Add a helper __devlink_params_unregister() for common code in
>   devlink_params_unregister() and devlink_port_params_unregister().
> - Move DEVLINK_CMD_PORT_PARAM_XXX definitions to the end of the enum.
> - Split the patches for devlink_port_param_driverinit_value_get() and
>   devlink_port_param_driverinit_value_set() into separate patches.
> - define DEVLINK_PARAM_GENERIC_ID_WOL type as u8 and define enum for
>   different types of WOL. Accordingly modify bnxt_en patch to validate
>   wol type.
>
> Vasundhara Volam (8):
>   devlink: Add devlink_param for port register and unregister
>   devlink: Add port param get command
>   devlink: Add port param set command
>   devlink: Add support for driverinit get value for devlink_port
>   devlink: Add support for driverinit set value for devlink_port
>   devlink: Add devlink notifications support for port params
>   devlink: Add a generic wake_on_lan port parameter
>   bnxt_en: Add bnxt_en initial port params table and register it
>
>  drivers/net/ethernet/broadcom/bnxt/bnxt.h         |   1 +
>  drivers/net/ethernet/broadcom/bnxt/bnxt_devlink.c |  43 +-
>  drivers/net/ethernet/broadcom/bnxt/bnxt_devlink.h |   1 +
>  include/net/devlink.h                             |  57 +++
>  include/uapi/linux/devlink.h                      |   5 +
>  net/core/devlink.c                                | 464 ++++++++++++++++++----
>  6 files changed, 489 insertions(+), 82 deletions(-)
>
> --
> 1.8.3.1
>

Powered by blists - more mailing lists