| Message ID | 20190906160101.14866-3-simon.horman@netronome.com |
|---|---|
| State | Changes Requested |
| Delegated to: | David Miller |
| Headers | show |
| Series | nfp: implement firmware loading policy | expand |
Fri, Sep 06, 2019 at 06:00:52PM CEST, simon.horman@netronome.com wrote: >From: Dirk van der Merwe <dirk.vandermerwe@netronome.com> > >Add the 'reset_dev_on_drv_probe' devlink parameter, controlling the >device reset policy on driver probe. > >This parameter is useful in conjunction with the existing >'fw_load_policy' parameter. > >Signed-off-by: Dirk van der Merwe <dirk.vandermerwe@netronome.com> >Signed-off-by: Simon Horman <simon.horman@netronome.com> >--- > Documentation/networking/devlink-params.txt | 14 ++++++++++++++ > include/net/devlink.h | 4 ++++ > include/uapi/linux/devlink.h | 7 +++++++ > net/core/devlink.c | 5 +++++ > 4 files changed, 30 insertions(+) > >diff --git a/Documentation/networking/devlink-params.txt b/Documentation/networking/devlink-params.txt >index fadb5436188d..f9e30d686243 100644 >--- a/Documentation/networking/devlink-params.txt >+++ b/Documentation/networking/devlink-params.txt >@@ -51,3 +51,17 @@ fw_load_policy [DEVICE, GENERIC] > * DEVLINK_PARAM_FW_LOAD_POLICY_VALUE_DISK (2) > Load firmware currently available on host's disk. > Type: u8 >+ >+reset_dev_on_drv_probe [DEVICE, GENERIC] >+ Controls the device's reset policy on driver probe. >+ Valid values: >+ * DEVLINK_PARAM_RESET_DEV_VALUE_UNKNOWN (0) >+ Unknown or invalid value. Why do you need this? Do you have usecase for this value? >+ * DEVLINK_PARAM_RESET_DEV_VALUE_ALWAYS (1) >+ Always reset device on driver probe. >+ * DEVLINK_PARAM_RESET_DEV_VALUE_NEVER (2) >+ Never reset device on driver probe. >+ * DEVLINK_PARAM_RESET_DEV_VALUE_DISK (3) >+ Reset only if device firmware can be found in the >+ filesystem. >+ Type: u8 >diff --git a/include/net/devlink.h b/include/net/devlink.h >index 460bc629d1a4..d880de5b8d3a 100644 >--- a/include/net/devlink.h >+++ b/include/net/devlink.h >@@ -398,6 +398,7 @@ enum devlink_param_generic_id { > DEVLINK_PARAM_GENERIC_ID_MSIX_VEC_PER_PF_MAX, > DEVLINK_PARAM_GENERIC_ID_MSIX_VEC_PER_PF_MIN, > DEVLINK_PARAM_GENERIC_ID_FW_LOAD_POLICY, >+ DEVLINK_PARAM_GENERIC_ID_RESET_DEV, > > /* add new param generic ids above here*/ > __DEVLINK_PARAM_GENERIC_ID_MAX, >@@ -428,6 +429,9 @@ enum devlink_param_generic_id { > #define DEVLINK_PARAM_GENERIC_FW_LOAD_POLICY_NAME "fw_load_policy" > #define DEVLINK_PARAM_GENERIC_FW_LOAD_POLICY_TYPE DEVLINK_PARAM_TYPE_U8 > >+#define DEVLINK_PARAM_GENERIC_RESET_DEV_NAME "reset_dev_on_drv_probe" The name of the define and name of the string should be the same. Please adjust. >+#define DEVLINK_PARAM_GENERIC_RESET_DEV_TYPE DEVLINK_PARAM_TYPE_U8 >+ > #define DEVLINK_PARAM_GENERIC(_id, _cmodes, _get, _set, _validate) \ > { \ > .id = DEVLINK_PARAM_GENERIC_ID_##_id, \ >diff --git a/include/uapi/linux/devlink.h b/include/uapi/linux/devlink.h >index c25cc29a6647..3172d1b3329f 100644 >--- a/include/uapi/linux/devlink.h >+++ b/include/uapi/linux/devlink.h >@@ -205,6 +205,13 @@ enum devlink_param_fw_load_policy_value { > DEVLINK_PARAM_FW_LOAD_POLICY_VALUE_DISK, > }; > >+enum devlink_param_reset_dev_value { >+ DEVLINK_PARAM_RESET_DEV_VALUE_UNKNOWN, >+ DEVLINK_PARAM_RESET_DEV_VALUE_ALWAYS, >+ DEVLINK_PARAM_RESET_DEV_VALUE_NEVER, >+ DEVLINK_PARAM_RESET_DEV_VALUE_DISK, >+}; >+ > enum { > DEVLINK_ATTR_STATS_RX_PACKETS, /* u64 */ > DEVLINK_ATTR_STATS_RX_BYTES, /* u64 */ >diff --git a/net/core/devlink.c b/net/core/devlink.c >index 6e52d639dac6..e8bc96f104a7 100644 >--- a/net/core/devlink.c >+++ b/net/core/devlink.c >@@ -2852,6 +2852,11 @@ static const struct devlink_param devlink_param_generic[] = { > .name = DEVLINK_PARAM_GENERIC_FW_LOAD_POLICY_NAME, > .type = DEVLINK_PARAM_GENERIC_FW_LOAD_POLICY_TYPE, > }, >+ { >+ .id = DEVLINK_PARAM_GENERIC_ID_RESET_DEV, >+ .name = DEVLINK_PARAM_GENERIC_RESET_DEV_NAME, >+ .type = DEVLINK_PARAM_GENERIC_RESET_DEV_TYPE, >+ }, > }; > > static int devlink_param_generic_verify(const struct devlink_param *param) >-- >2.11.0 >
On 9/6/19 11:31 AM, Jiri Pirko wrote: > Fri, Sep 06, 2019 at 06:00:52PM CEST, simon.horman@netronome.com wrote: >> From: Dirk van der Merwe <dirk.vandermerwe@netronome.com> >> >> Add the 'reset_dev_on_drv_probe' devlink parameter, controlling the >> device reset policy on driver probe. >> >> This parameter is useful in conjunction with the existing >> 'fw_load_policy' parameter. >> >> Signed-off-by: Dirk van der Merwe <dirk.vandermerwe@netronome.com> >> Signed-off-by: Simon Horman <simon.horman@netronome.com> >> --- >> Documentation/networking/devlink-params.txt | 14 ++++++++++++++ >> include/net/devlink.h | 4 ++++ >> include/uapi/linux/devlink.h | 7 +++++++ >> net/core/devlink.c | 5 +++++ >> 4 files changed, 30 insertions(+) >> >> diff --git a/Documentation/networking/devlink-params.txt b/Documentation/networking/devlink-params.txt >> index fadb5436188d..f9e30d686243 100644 >> --- a/Documentation/networking/devlink-params.txt >> +++ b/Documentation/networking/devlink-params.txt >> @@ -51,3 +51,17 @@ fw_load_policy [DEVICE, GENERIC] >> * DEVLINK_PARAM_FW_LOAD_POLICY_VALUE_DISK (2) >> Load firmware currently available on host's disk. >> Type: u8 >> + >> +reset_dev_on_drv_probe [DEVICE, GENERIC] >> + Controls the device's reset policy on driver probe. >> + Valid values: >> + * DEVLINK_PARAM_RESET_DEV_VALUE_UNKNOWN (0) >> + Unknown or invalid value. > Why do you need this? Do you have usecase for this value? I added this in to avoid having the entire netlink dump fail when there are invalid values read from hardware. This way, it can report an unknown or invalid value instead of failing the operation. > > >> + * DEVLINK_PARAM_RESET_DEV_VALUE_ALWAYS (1) >> + Always reset device on driver probe. >> + * DEVLINK_PARAM_RESET_DEV_VALUE_NEVER (2) >> + Never reset device on driver probe. >> + * DEVLINK_PARAM_RESET_DEV_VALUE_DISK (3) >> + Reset only if device firmware can be found in the >> + filesystem. >> + Type: u8 >> diff --git a/include/net/devlink.h b/include/net/devlink.h >> index 460bc629d1a4..d880de5b8d3a 100644 >> --- a/include/net/devlink.h >> +++ b/include/net/devlink.h >> @@ -398,6 +398,7 @@ enum devlink_param_generic_id { >> DEVLINK_PARAM_GENERIC_ID_MSIX_VEC_PER_PF_MAX, >> DEVLINK_PARAM_GENERIC_ID_MSIX_VEC_PER_PF_MIN, >> DEVLINK_PARAM_GENERIC_ID_FW_LOAD_POLICY, >> + DEVLINK_PARAM_GENERIC_ID_RESET_DEV, >> >> /* add new param generic ids above here*/ >> __DEVLINK_PARAM_GENERIC_ID_MAX, >> @@ -428,6 +429,9 @@ enum devlink_param_generic_id { >> #define DEVLINK_PARAM_GENERIC_FW_LOAD_POLICY_NAME "fw_load_policy" >> #define DEVLINK_PARAM_GENERIC_FW_LOAD_POLICY_TYPE DEVLINK_PARAM_TYPE_U8 >> >> +#define DEVLINK_PARAM_GENERIC_RESET_DEV_NAME "reset_dev_on_drv_probe" > The name of the define and name of the string should be the same. Please > adjust. Sure, I will make the change. Thanks for the review.
On Fri, 6 Sep 2019 11:40:54 -0700, Dirk van der Merwe wrote: > >> DEVLINK_PARAM_RESET_DEV_VALUE_UNKNOWN (0) > >> + Unknown or invalid value. > > Why do you need this? Do you have usecase for this value? > > I added this in to avoid having the entire netlink dump fail when there > are invalid values read from hardware. > > This way, it can report an unknown or invalid value instead of failing > the operation. That's the first reason, the second is that we also want to report the unknown value if it's not recognized by the driver. For u8/enum parameters the value may possibly be set to a value older driver doesn't understand, but users should still be able to set them to one of the known ones. We'd also like to add that to 'fw_load_policy'. WDYT?
Sat, Sep 07, 2019 at 06:17:30AM CEST, jakub.kicinski@netronome.com wrote: >On Fri, 6 Sep 2019 11:40:54 -0700, Dirk van der Merwe wrote: >> >> DEVLINK_PARAM_RESET_DEV_VALUE_UNKNOWN (0) >> >> + Unknown or invalid value. >> > Why do you need this? Do you have usecase for this value? >> >> I added this in to avoid having the entire netlink dump fail when there >> are invalid values read from hardware. >> >> This way, it can report an unknown or invalid value instead of failing >> the operation. > >That's the first reason, the second is that we also want to report >the unknown value if it's not recognized by the driver. For u8/enum >parameters the value may possibly be set to a value older driver >doesn't understand, but users should still be able to set them to one >of the known ones. Ok. > >We'd also like to add that to 'fw_load_policy'. WDYT? Ok.
diff --git a/Documentation/networking/devlink-params.txt b/Documentation/networking/devlink-params.txt index fadb5436188d..f9e30d686243 100644 --- a/Documentation/networking/devlink-params.txt +++ b/Documentation/networking/devlink-params.txt @@ -51,3 +51,17 @@ fw_load_policy [DEVICE, GENERIC] * DEVLINK_PARAM_FW_LOAD_POLICY_VALUE_DISK (2) Load firmware currently available on host's disk. Type: u8 + +reset_dev_on_drv_probe [DEVICE, GENERIC] + Controls the device's reset policy on driver probe. + Valid values: + * DEVLINK_PARAM_RESET_DEV_VALUE_UNKNOWN (0) + Unknown or invalid value. + * DEVLINK_PARAM_RESET_DEV_VALUE_ALWAYS (1) + Always reset device on driver probe. + * DEVLINK_PARAM_RESET_DEV_VALUE_NEVER (2) + Never reset device on driver probe. + * DEVLINK_PARAM_RESET_DEV_VALUE_DISK (3) + Reset only if device firmware can be found in the + filesystem. + Type: u8 diff --git a/include/net/devlink.h b/include/net/devlink.h index 460bc629d1a4..d880de5b8d3a 100644 --- a/include/net/devlink.h +++ b/include/net/devlink.h @@ -398,6 +398,7 @@ enum devlink_param_generic_id { DEVLINK_PARAM_GENERIC_ID_MSIX_VEC_PER_PF_MAX, DEVLINK_PARAM_GENERIC_ID_MSIX_VEC_PER_PF_MIN, DEVLINK_PARAM_GENERIC_ID_FW_LOAD_POLICY, + DEVLINK_PARAM_GENERIC_ID_RESET_DEV, /* add new param generic ids above here*/ __DEVLINK_PARAM_GENERIC_ID_MAX, @@ -428,6 +429,9 @@ enum devlink_param_generic_id { #define DEVLINK_PARAM_GENERIC_FW_LOAD_POLICY_NAME "fw_load_policy" #define DEVLINK_PARAM_GENERIC_FW_LOAD_POLICY_TYPE DEVLINK_PARAM_TYPE_U8 +#define DEVLINK_PARAM_GENERIC_RESET_DEV_NAME "reset_dev_on_drv_probe" +#define DEVLINK_PARAM_GENERIC_RESET_DEV_TYPE DEVLINK_PARAM_TYPE_U8 + #define DEVLINK_PARAM_GENERIC(_id, _cmodes, _get, _set, _validate) \ { \ .id = DEVLINK_PARAM_GENERIC_ID_##_id, \ diff --git a/include/uapi/linux/devlink.h b/include/uapi/linux/devlink.h index c25cc29a6647..3172d1b3329f 100644 --- a/include/uapi/linux/devlink.h +++ b/include/uapi/linux/devlink.h @@ -205,6 +205,13 @@ enum devlink_param_fw_load_policy_value { DEVLINK_PARAM_FW_LOAD_POLICY_VALUE_DISK, }; +enum devlink_param_reset_dev_value { + DEVLINK_PARAM_RESET_DEV_VALUE_UNKNOWN, + DEVLINK_PARAM_RESET_DEV_VALUE_ALWAYS, + DEVLINK_PARAM_RESET_DEV_VALUE_NEVER, + DEVLINK_PARAM_RESET_DEV_VALUE_DISK, +}; + enum { DEVLINK_ATTR_STATS_RX_PACKETS, /* u64 */ DEVLINK_ATTR_STATS_RX_BYTES, /* u64 */ diff --git a/net/core/devlink.c b/net/core/devlink.c index 6e52d639dac6..e8bc96f104a7 100644 --- a/net/core/devlink.c +++ b/net/core/devlink.c @@ -2852,6 +2852,11 @@ static const struct devlink_param devlink_param_generic[] = { .name = DEVLINK_PARAM_GENERIC_FW_LOAD_POLICY_NAME, .type = DEVLINK_PARAM_GENERIC_FW_LOAD_POLICY_TYPE, }, + { + .id = DEVLINK_PARAM_GENERIC_ID_RESET_DEV, + .name = DEVLINK_PARAM_GENERIC_RESET_DEV_NAME, + .type = DEVLINK_PARAM_GENERIC_RESET_DEV_TYPE, + }, }; static int devlink_param_generic_verify(const struct devlink_param *param)