| Message ID | 1545297019-8960-1-git-send-email-vasundhara-v.volam@broadcom.com |
|---|---|
| Headers | show |
| Series | devlink: Add configuration parameters support for devlink_port | expand |
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@broadcom.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 >
On Thu, 20 Dec 2018 14:40:11 +0530, Vasundhara Volam 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: > -------- Patchwork or git may cut off anything under the ------ line.
On Thu, Dec 20, 2018 at 11:25 PM Jakub Kicinski <jakub.kicinski@netronome.com> wrote: > > On Thu, 20 Dec 2018 14:40:11 +0530, Vasundhara Volam 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: > > -------- > > Patchwork or git may cut off anything under the ------ line. Thank you. I will resend the patchset by replacing it with ****.
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(-)