[net-next,02/11] devlink: add 'reset_dev_on_drv_probe' param
diff mbox series

Message ID 20190906160101.14866-3-simon.horman@netronome.com
State Changes Requested
Delegated to: David Miller
Headers show
Series
  • nfp: implement firmware loading policy
Related show

Commit Message

Simon Horman Sept. 6, 2019, 4 p.m. UTC
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(+)

Comments

Jiri Pirko Sept. 6, 2019, 6:31 p.m. UTC | #1
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
>
Dirk van der Merwe Sept. 6, 2019, 6:40 p.m. UTC | #2
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.
Jakub Kicinski Sept. 7, 2019, 4:17 a.m. UTC | #3
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?
Jiri Pirko Sept. 7, 2019, 10:28 a.m. UTC | #4
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.

Patch
diff mbox series

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)