| Message ID | 1536655505-14387-2-git-send-email-vasundhara-v.volam@broadcom.com |
|---|---|
| State | Changes Requested, archived |
| Delegated to: | David Miller |
| Headers | show |
| Series | bnxt_en: devlink param updates | expand |
Tue, Sep 11, 2018 at 10:44:58AM CEST, vasundhara-v.volam@broadcom.com wrote: >hw_tc_offload - Enable/Disable TC flower offload in the device. > >Signed-off-by: Vasundhara Volam <vasundhara-v.volam@broadcom.com> >--- > include/net/devlink.h | 4 ++++ > net/core/devlink.c | 5 +++++ > 2 files changed, 9 insertions(+) > >diff --git a/include/net/devlink.h b/include/net/devlink.h >index b9b89d6..a0e9ce9 100644 >--- a/include/net/devlink.h >+++ b/include/net/devlink.h >@@ -362,6 +362,7 @@ enum devlink_param_generic_id { > DEVLINK_PARAM_GENERIC_ID_MAX_MACS, > DEVLINK_PARAM_GENERIC_ID_ENABLE_SRIOV, > DEVLINK_PARAM_GENERIC_ID_REGION_SNAPSHOT, >+ DEVLINK_PARAM_GENERIC_ID_HW_TC_OFFLOAD, Could you please describe why do you need this here and why the tc_offload flag in ethtool is not enough. How do you imagine the user should use them together?
On Tue, Sep 11, 2018 at 3:25 PM Jiri Pirko <jiri@resnulli.us> wrote: > > Tue, Sep 11, 2018 at 10:44:58AM CEST, vasundhara-v.volam@broadcom.com wrote: > >hw_tc_offload - Enable/Disable TC flower offload in the device. > > > >Signed-off-by: Vasundhara Volam <vasundhara-v.volam@broadcom.com> > >--- > > include/net/devlink.h | 4 ++++ > > net/core/devlink.c | 5 +++++ > > 2 files changed, 9 insertions(+) > > > >diff --git a/include/net/devlink.h b/include/net/devlink.h > >index b9b89d6..a0e9ce9 100644 > >--- a/include/net/devlink.h > >+++ b/include/net/devlink.h > >@@ -362,6 +362,7 @@ enum devlink_param_generic_id { > > DEVLINK_PARAM_GENERIC_ID_MAX_MACS, > > DEVLINK_PARAM_GENERIC_ID_ENABLE_SRIOV, > > DEVLINK_PARAM_GENERIC_ID_REGION_SNAPSHOT, > >+ DEVLINK_PARAM_GENERIC_ID_HW_TC_OFFLOAD, > > Could you please describe why do you need this here and why the > tc_offload flag in ethtool is not enough. How do you imagine the user > should use them together? Jiri, tc_offload flag in ethtool will modify feature in driver at runtime. But I am adding tc_offload param here to toggle this feature in NVM Config of our adapter, whose configuration mode is permanent and will be effective only with a reboot of the server. User has to turn on tc_offload feature in NVM config of the adapter and then enabling the tc_offload flag in ethtool will completely enable the feature in driver.
On Wed, 12 Sep 2018 11:47:59 +0530, Vasundhara Volam wrote: > On Tue, Sep 11, 2018 at 3:25 PM Jiri Pirko <jiri@resnulli.us> wrote: > > > > Tue, Sep 11, 2018 at 10:44:58AM CEST, vasundhara-v.volam@broadcom.com wrote: > > >hw_tc_offload - Enable/Disable TC flower offload in the device. > > > > > >Signed-off-by: Vasundhara Volam <vasundhara-v.volam@broadcom.com> > > >--- > > > include/net/devlink.h | 4 ++++ > > > net/core/devlink.c | 5 +++++ > > > 2 files changed, 9 insertions(+) > > > > > >diff --git a/include/net/devlink.h b/include/net/devlink.h > > >index b9b89d6..a0e9ce9 100644 > > >--- a/include/net/devlink.h > > >+++ b/include/net/devlink.h > > >@@ -362,6 +362,7 @@ enum devlink_param_generic_id { > > > DEVLINK_PARAM_GENERIC_ID_MAX_MACS, > > > DEVLINK_PARAM_GENERIC_ID_ENABLE_SRIOV, > > > DEVLINK_PARAM_GENERIC_ID_REGION_SNAPSHOT, > > >+ DEVLINK_PARAM_GENERIC_ID_HW_TC_OFFLOAD, > > > > Could you please describe why do you need this here and why the > > tc_offload flag in ethtool is not enough. How do you imagine the user > > should use them together? > Jiri, tc_offload flag in ethtool will modify feature in driver at > runtime. But I am adding > tc_offload param here to toggle this feature in NVM Config of our > adapter, whose configuration > mode is permanent and will be effective only with a reboot of the server. > > User has to turn on tc_offload feature in NVM config of the adapter and then > enabling the tc_offload flag in ethtool will completely enable the > feature in driver. Thanks for explaining, however, I don't think we have trouble understanding *what* you are doing, but rather *why* you're doing it.
Wed, Sep 12, 2018 at 08:34:22AM CEST, jakub.kicinski@netronome.com wrote: >On Wed, 12 Sep 2018 11:47:59 +0530, Vasundhara Volam wrote: >> On Tue, Sep 11, 2018 at 3:25 PM Jiri Pirko <jiri@resnulli.us> wrote: >> > >> > Tue, Sep 11, 2018 at 10:44:58AM CEST, vasundhara-v.volam@broadcom.com wrote: >> > >hw_tc_offload - Enable/Disable TC flower offload in the device. >> > > >> > >Signed-off-by: Vasundhara Volam <vasundhara-v.volam@broadcom.com> >> > >--- >> > > include/net/devlink.h | 4 ++++ >> > > net/core/devlink.c | 5 +++++ >> > > 2 files changed, 9 insertions(+) >> > > >> > >diff --git a/include/net/devlink.h b/include/net/devlink.h >> > >index b9b89d6..a0e9ce9 100644 >> > >--- a/include/net/devlink.h >> > >+++ b/include/net/devlink.h >> > >@@ -362,6 +362,7 @@ enum devlink_param_generic_id { >> > > DEVLINK_PARAM_GENERIC_ID_MAX_MACS, >> > > DEVLINK_PARAM_GENERIC_ID_ENABLE_SRIOV, >> > > DEVLINK_PARAM_GENERIC_ID_REGION_SNAPSHOT, >> > >+ DEVLINK_PARAM_GENERIC_ID_HW_TC_OFFLOAD, >> > >> > Could you please describe why do you need this here and why the >> > tc_offload flag in ethtool is not enough. How do you imagine the user >> > should use them together? >> Jiri, tc_offload flag in ethtool will modify feature in driver at >> runtime. But I am adding >> tc_offload param here to toggle this feature in NVM Config of our >> adapter, whose configuration >> mode is permanent and will be effective only with a reboot of the server. >> >> User has to turn on tc_offload feature in NVM config of the adapter and then >> enabling the tc_offload flag in ethtool will completely enable the >> feature in driver. > >Thanks for explaining, however, I don't think we have trouble >understanding *what* you are doing, but rather *why* you're doing it. Exactly. Why ethtool tc_offload flag is not enough?
diff --git a/include/net/devlink.h b/include/net/devlink.h index b9b89d6..a0e9ce9 100644 --- a/include/net/devlink.h +++ b/include/net/devlink.h @@ -362,6 +362,7 @@ enum devlink_param_generic_id { DEVLINK_PARAM_GENERIC_ID_MAX_MACS, DEVLINK_PARAM_GENERIC_ID_ENABLE_SRIOV, DEVLINK_PARAM_GENERIC_ID_REGION_SNAPSHOT, + DEVLINK_PARAM_GENERIC_ID_HW_TC_OFFLOAD, /* add new param generic ids above here*/ __DEVLINK_PARAM_GENERIC_ID_MAX, @@ -380,6 +381,9 @@ enum devlink_param_generic_id { #define DEVLINK_PARAM_GENERIC_REGION_SNAPSHOT_NAME "region_snapshot_enable" #define DEVLINK_PARAM_GENERIC_REGION_SNAPSHOT_TYPE DEVLINK_PARAM_TYPE_BOOL +#define DEVLINK_PARAM_GENERIC_HW_TC_OFFLOAD_NAME "hw_tc_offload" +#define DEVLINK_PARAM_GENERIC_HW_TC_OFFLOAD_TYPE DEVLINK_PARAM_TYPE_BOOL + #define DEVLINK_PARAM_GENERIC(_id, _cmodes, _get, _set, _validate) \ { \ .id = DEVLINK_PARAM_GENERIC_ID_##_id, \ diff --git a/net/core/devlink.c b/net/core/devlink.c index 65fc366..020daa1 100644 --- a/net/core/devlink.c +++ b/net/core/devlink.c @@ -2676,6 +2676,11 @@ static int devlink_nl_cmd_reload(struct sk_buff *skb, struct genl_info *info) .name = DEVLINK_PARAM_GENERIC_REGION_SNAPSHOT_NAME, .type = DEVLINK_PARAM_GENERIC_REGION_SNAPSHOT_TYPE, }, + { + .id = DEVLINK_PARAM_GENERIC_ID_HW_TC_OFFLOAD, + .name = DEVLINK_PARAM_GENERIC_HW_TC_OFFLOAD_NAME, + .type = DEVLINK_PARAM_GENERIC_HW_TC_OFFLOAD_TYPE, + }, }; static int devlink_param_generic_verify(const struct devlink_param *param)
hw_tc_offload - Enable/Disable TC flower offload in the device. Signed-off-by: Vasundhara Volam <vasundhara-v.volam@broadcom.com> --- include/net/devlink.h | 4 ++++ net/core/devlink.c | 5 +++++ 2 files changed, 9 insertions(+)