mbox series

[v4,net-next,0/5] ionic: add devlink dev flash support

Message ID 20200917030204.50098-1-snelson@pensando.io
Headers show
Series ionic: add devlink dev flash support | expand

Message

Shannon Nelson Sept. 17, 2020, 3:01 a.m. UTC
Add support for using devlink's dev flash facility to update the
firmware on an ionic device, and add a new timeout parameter to the
devlink flash netlink message.

For long-running flash commands, we add a timeout element to the dev
flash notify message in order for a userland utility to display a timeout
deadline to the user.  This allows the userland utility to display a
count down to the user when a firmware update action is otherwise going
to go for ahile without any updates.  An example use is added to the
netdevsim module.

The ionic driver uses this timeout element in its new flash function.
The driver uses a simple model of pushing the firmware file to the NIC,
asking the NIC to unpack and install the file into the device, and then
selecting it for the next boot.  If any of these steps fail, the whole
transaction is failed.  A couple of the steps can take a long time,
so we use the timeout status message rather than faking it with bogus
done/total messages.

The driver doesn't currently support doing these steps individually.
In the future we want to be able to list the FW that is installed and
selectable but we don't yet have the API to fully support that.

v4: Added a new devlink status notify message for showing timeout
    information, and modified the ionic fw update to use it for its long
    running firmware commands.

v3: Changed long dev_cmd timeout on status check calls to a loop around
    calls with a normal timeout, which allows for more intermediate log
    messaging when in a long wait, and for letting other threads run
    dev_cmds if waiting.

v2: Changed "Activate" to "Select" in status messages.

Shannon Nelson (5):
  devlink: add timeout information to status_notify
  devlink: collect flash notify params into a struct
  netdevsim: devlink flash timeout message
  ionic: update the fw update api
  ionic: add devlink firmware update

 drivers/net/ethernet/pensando/ionic/Makefile  |   2 +-
 .../ethernet/pensando/ionic/ionic_devlink.c   |  14 ++
 .../ethernet/pensando/ionic/ionic_devlink.h   |   3 +
 .../net/ethernet/pensando/ionic/ionic_fw.c    | 206 ++++++++++++++++++
 .../net/ethernet/pensando/ionic/ionic_if.h    |  33 ++-
 .../net/ethernet/pensando/ionic/ionic_main.c  |  27 ++-
 drivers/net/netdevsim/dev.c                   |   2 +
 include/net/devlink.h                         |  25 +++
 include/uapi/linux/devlink.h                  |   3 +
 net/core/devlink.c                            |  83 ++++---
 10 files changed, 350 insertions(+), 48 deletions(-)
 create mode 100644 drivers/net/ethernet/pensando/ionic/ionic_fw.c

Comments

Jakub Kicinski Sept. 17, 2020, 7:46 p.m. UTC | #1
On Wed, 16 Sep 2020 20:02:00 -0700 Shannon Nelson wrote:
> Add a timeout element to the DEVLINK_CMD_FLASH_UPDATE_STATUS

> netlink message for use by a userland utility to show that

> a particular firmware flash activity may take a long but

> bounded time to finish.  Also add a handy helper for drivers

> to make use of the new timeout value.

> 

> UI usage hints:

>  - if non-zero, add timeout display to the end of the status line

>  	[component] status_msg  ( Xm Ys : Am Bs )

>      using the timeout value for Am Bs and updating the Xm Ys

>      every second

>  - if the timeout expires while awaiting the next update,

>    display something like

>  	[component] status_msg  ( timeout reached : Am Bs )

>  - if new status notify messages are received, remove

>    the timeout and start over

> 

> Signed-off-by: Shannon Nelson <snelson@pensando.io>


Minor nits, otherwise LGTM:

Reviewed-by: Jakub Kicinski <kuba@kernel.org>


> @@ -3052,6 +3054,9 @@ static int devlink_nl_flash_update_fill(struct sk_buff *msg,

>  	if (nla_put_u64_64bit(msg, DEVLINK_ATTR_FLASH_UPDATE_STATUS_TOTAL,

>  			      total, DEVLINK_ATTR_PAD))

>  		goto nla_put_failure;

> +	if (nla_put_u64_64bit(msg, DEVLINK_ATTR_FLASH_UPDATE_STATUS_TIMEOUT,

> +			      timeout, DEVLINK_ATTR_PAD))

> +		goto nla_put_failure;


nit: since old kernels don't report this user space has to deal with it
     not being present so I'd be tempted to only report it if timeout
     is not 0

> +void devlink_flash_update_timeout_notify(struct devlink *devlink,

> +					 const char *status_msg,

> +					 const char *component,

> +					 unsigned long timeout)

> +{

> +	__devlink_flash_update_notify(devlink,

> +				      DEVLINK_CMD_FLASH_UPDATE_STATUS,

> +				      status_msg, component, 0, 0, timeout);


nit: did we ever report cmd == UPDATE_STATUS and total == 0?
     could this cause a division by zero in some unsuspecting
     implementation? Perhaps we should pass 1 here?

> +}

> +EXPORT_SYMBOL_GPL(devlink_flash_update_timeout_notify);
Jakub Kicinski Sept. 17, 2020, 7:47 p.m. UTC | #2
On Wed, 16 Sep 2020 20:02:01 -0700 Shannon Nelson wrote:
> The dev flash status notify function parameter lists are getting
> rather long, so add a struct to be filled and passed rather than
> continuously changing the function signatures.
> 
> Signed-off-by: Shannon Nelson <snelson@pensando.io>
> ---
>  include/net/devlink.h | 21 ++++++++++++
>  net/core/devlink.c    | 80 +++++++++++++++++++++++--------------------
>  2 files changed, 63 insertions(+), 38 deletions(-)
> 
> diff --git a/include/net/devlink.h b/include/net/devlink.h
> index f206accf80ad..9ab2014885cb 100644
> --- a/include/net/devlink.h
> +++ b/include/net/devlink.h
> @@ -391,6 +391,27 @@ struct devlink_param_gset_ctx {
>  	enum devlink_param_cmode cmode;
>  };
>  
> +/**
> + * struct devlink_flash_notify - devlink dev flash notify data
> + * @cmd: devlink notify command code
> + * @status_msg: current status string
> + * @component: firmware component being updated
> + * @done: amount of work completed of total amount
> + * @total: amount of work expected to be done
> + * @timeout: expected max timeout in seconds
> + *
> + * These are values to be given to userland to be displayed in order
> + * to show current activity in a firmware update process.
> + */
> +struct devlink_flash_notify {
> +	enum devlink_command cmd;

I'd leave out cmd out of the params structure, otherwise I'll be
slightly awkward for drivers to fill in given the current helpers 
are per cmd.

> +	const char *status_msg;
> +	const char *component;
> +	unsigned long done;
> +	unsigned long total;
> +	unsigned long timeout;
> +};
Jacob Keller Sept. 17, 2020, 7:50 p.m. UTC | #3
On 9/16/2020 8:02 PM, Shannon Nelson wrote:
> Add a timeout element to the DEVLINK_CMD_FLASH_UPDATE_STATUS
> netlink message for use by a userland utility to show that
> a particular firmware flash activity may take a long but
> bounded time to finish.  Also add a handy helper for drivers
> to make use of the new timeout value.
> 
> UI usage hints:
>  - if non-zero, add timeout display to the end of the status line
>  	[component] status_msg  ( Xm Ys : Am Bs )
>      using the timeout value for Am Bs and updating the Xm Ys
>      every second
>  - if the timeout expires while awaiting the next update,
>    display something like
>  	[component] status_msg  ( timeout reached : Am Bs )
>  - if new status notify messages are received, remove
>    the timeout and start over
> 
> Signed-off-by: Shannon Nelson <snelson@pensando.io>
> ---

This one looks good to me. I think the only other things that I saw from
previous discussion are:

a) we could convert the internal helper devlink_nl_flash_update_fill and
__devlink_flash_update_notify to use structs for their fields, and..

b) Jakub pointed out that most drivers don't currently use the component
field so we could remove that from the helpers.

However, I don't have strong feelings about those either way, so:

Reviewed-by: Jacob Keller <jacob.e.keller@intel.com>

>  include/net/devlink.h        |  4 ++++
>  include/uapi/linux/devlink.h |  3 +++
>  net/core/devlink.c           | 29 +++++++++++++++++++++++------
>  3 files changed, 30 insertions(+), 6 deletions(-)
> 
> diff --git a/include/net/devlink.h b/include/net/devlink.h
> index eaec0a8cc5ef..f206accf80ad 100644
> --- a/include/net/devlink.h
> +++ b/include/net/devlink.h
> @@ -1400,6 +1400,10 @@ void devlink_flash_update_status_notify(struct devlink *devlink,
>  					const char *component,
>  					unsigned long done,
>  					unsigned long total);
> +void devlink_flash_update_timeout_notify(struct devlink *devlink,
> +					 const char *status_msg,
> +					 const char *component,
> +					 unsigned long timeout);
>  
>  int devlink_traps_register(struct devlink *devlink,
>  			   const struct devlink_trap *traps,
> diff --git a/include/uapi/linux/devlink.h b/include/uapi/linux/devlink.h
> index 40d35145c879..4a6e213cfa04 100644
> --- a/include/uapi/linux/devlink.h
> +++ b/include/uapi/linux/devlink.h
> @@ -460,6 +460,9 @@ enum devlink_attr {
>  
>  	DEVLINK_ATTR_PORT_EXTERNAL,		/* u8 */
>  	DEVLINK_ATTR_PORT_CONTROLLER_NUMBER,	/* u32 */
> +
> +	DEVLINK_ATTR_FLASH_UPDATE_STATUS_TIMEOUT,	/* u64 */
> +
>  	/* add new attributes above here, update the policy in devlink.c */
>  
>  	__DEVLINK_ATTR_MAX,
> diff --git a/net/core/devlink.c b/net/core/devlink.c
> index 19037f114307..01f855e53e06 100644
> --- a/net/core/devlink.c
> +++ b/net/core/devlink.c
> @@ -3024,7 +3024,9 @@ static int devlink_nl_flash_update_fill(struct sk_buff *msg,
>  					enum devlink_command cmd,
>  					const char *status_msg,
>  					const char *component,
> -					unsigned long done, unsigned long total)
> +					unsigned long done,
> +					unsigned long total,
> +					unsigned long timeout)

Number of paramters here is getting rather large. Does it make sense to
convert this one to a struct now?

>  {
>  	void *hdr;
>  
> @@ -3052,6 +3054,9 @@ static int devlink_nl_flash_update_fill(struct sk_buff *msg,
>  	if (nla_put_u64_64bit(msg, DEVLINK_ATTR_FLASH_UPDATE_STATUS_TOTAL,
>  			      total, DEVLINK_ATTR_PAD))
>  		goto nla_put_failure;
> +	if (nla_put_u64_64bit(msg, DEVLINK_ATTR_FLASH_UPDATE_STATUS_TIMEOUT,
> +			      timeout, DEVLINK_ATTR_PAD))
> +		goto nla_put_failure;
>  
>  out:
>  	genlmsg_end(msg, hdr);
> @@ -3067,7 +3072,8 @@ static void __devlink_flash_update_notify(struct devlink *devlink,
>  					  const char *status_msg,
>  					  const char *component,
>  					  unsigned long done,
> -					  unsigned long total)
> +					  unsigned long total,
> +					  unsigned long timeout)
>  {
>  	struct sk_buff *msg;
>  	int err;
> @@ -3081,7 +3087,7 @@ static void __devlink_flash_update_notify(struct devlink *devlink,
>  		return;
>  
>  	err = devlink_nl_flash_update_fill(msg, devlink, cmd, status_msg,
> -					   component, done, total);
> +					   component, done, total, timeout);
>  	if (err)
>  		goto out_free_msg;
>  
> @@ -3097,7 +3103,7 @@ void devlink_flash_update_begin_notify(struct devlink *devlink)
>  {
>  	__devlink_flash_update_notify(devlink,
>  				      DEVLINK_CMD_FLASH_UPDATE,
> -				      NULL, NULL, 0, 0);
> +				      NULL, NULL, 0, 0, 0);
>  }
>  EXPORT_SYMBOL_GPL(devlink_flash_update_begin_notify);
>  
> @@ -3105,7 +3111,7 @@ void devlink_flash_update_end_notify(struct devlink *devlink)
>  {
>  	__devlink_flash_update_notify(devlink,
>  				      DEVLINK_CMD_FLASH_UPDATE_END,
> -				      NULL, NULL, 0, 0);
> +				      NULL, NULL, 0, 0, 0);
>  }
>  EXPORT_SYMBOL_GPL(devlink_flash_update_end_notify);
>  
> @@ -3117,10 +3123,21 @@ void devlink_flash_update_status_notify(struct devlink *devlink,
>  {
>  	__devlink_flash_update_notify(devlink,
>  				      DEVLINK_CMD_FLASH_UPDATE_STATUS,
> -				      status_msg, component, done, total);
> +				      status_msg, component, done, total, 0);
>  }
>  EXPORT_SYMBOL_GPL(devlink_flash_update_status_notify);
>  
> +void devlink_flash_update_timeout_notify(struct devlink *devlink,
> +					 const char *status_msg,
> +					 const char *component,
> +					 unsigned long timeout)

So we dropped the done and total here since in most cases we expect not
to need both a timeout and a done/total. I think that make sense. Most
of the time I think a command will benefit from one or the other.

> +{
> +	__devlink_flash_update_notify(devlink,
> +				      DEVLINK_CMD_FLASH_UPDATE_STATUS,
> +				      status_msg, component, 0, 0, timeout);
> +}
> +EXPORT_SYMBOL_GPL(devlink_flash_update_timeout_notify);
> +
>  static int devlink_nl_cmd_flash_update(struct sk_buff *skb,
>  				       struct genl_info *info)
>  {
>
Jacob Keller Sept. 17, 2020, 7:52 p.m. UTC | #4
On 9/16/2020 8:02 PM, Shannon Nelson wrote:
> The dev flash status notify function parameter lists are getting
> rather long, so add a struct to be filled and passed rather than
> continuously changing the function signatures.
> 
> Signed-off-by: Shannon Nelson <snelson@pensando.io>

Guess I should have read farther in the series. I would have expected
this patch first before introducing the timeout.

LGTM.

Reviewed-by: Jacob Keller <jacob.e.keller@intel.com>

> ---
>  include/net/devlink.h | 21 ++++++++++++
>  net/core/devlink.c    | 80 +++++++++++++++++++++++--------------------
>  2 files changed, 63 insertions(+), 38 deletions(-)
> 
> diff --git a/include/net/devlink.h b/include/net/devlink.h
> index f206accf80ad..9ab2014885cb 100644
> --- a/include/net/devlink.h
> +++ b/include/net/devlink.h
> @@ -391,6 +391,27 @@ struct devlink_param_gset_ctx {
>  	enum devlink_param_cmode cmode;
>  };
>  
> +/**
> + * struct devlink_flash_notify - devlink dev flash notify data
> + * @cmd: devlink notify command code
> + * @status_msg: current status string
> + * @component: firmware component being updated
> + * @done: amount of work completed of total amount
> + * @total: amount of work expected to be done
> + * @timeout: expected max timeout in seconds
> + *
> + * These are values to be given to userland to be displayed in order
> + * to show current activity in a firmware update process.
> + */
> +struct devlink_flash_notify {
> +	enum devlink_command cmd;
> +	const char *status_msg;
> +	const char *component;
> +	unsigned long done;
> +	unsigned long total;
> +	unsigned long timeout;
> +};
> +
>  /**
>   * struct devlink_param - devlink configuration parameter data
>   * @name: name of the parameter
> diff --git a/net/core/devlink.c b/net/core/devlink.c
> index 01f855e53e06..816f27a18b16 100644
> --- a/net/core/devlink.c
> +++ b/net/core/devlink.c
> @@ -3021,41 +3021,36 @@ static int devlink_nl_cmd_reload(struct sk_buff *skb, struct genl_info *info)
>  
>  static int devlink_nl_flash_update_fill(struct sk_buff *msg,
>  					struct devlink *devlink,
> -					enum devlink_command cmd,
> -					const char *status_msg,
> -					const char *component,
> -					unsigned long done,
> -					unsigned long total,
> -					unsigned long timeout)
> +					struct devlink_flash_notify *params)
>  {
>  	void *hdr;
>  
> -	hdr = genlmsg_put(msg, 0, 0, &devlink_nl_family, 0, cmd);
> +	hdr = genlmsg_put(msg, 0, 0, &devlink_nl_family, 0, params->cmd);
>  	if (!hdr)
>  		return -EMSGSIZE;
>  
>  	if (devlink_nl_put_handle(msg, devlink))
>  		goto nla_put_failure;
>  
> -	if (cmd != DEVLINK_CMD_FLASH_UPDATE_STATUS)
> +	if (params->cmd != DEVLINK_CMD_FLASH_UPDATE_STATUS)
>  		goto out;
>  
> -	if (status_msg &&
> +	if (params->status_msg &&
>  	    nla_put_string(msg, DEVLINK_ATTR_FLASH_UPDATE_STATUS_MSG,
> -			   status_msg))
> +			   params->status_msg))
>  		goto nla_put_failure;
> -	if (component &&
> +	if (params->component &&
>  	    nla_put_string(msg, DEVLINK_ATTR_FLASH_UPDATE_COMPONENT,
> -			   component))
> +			   params->component))
>  		goto nla_put_failure;
>  	if (nla_put_u64_64bit(msg, DEVLINK_ATTR_FLASH_UPDATE_STATUS_DONE,
> -			      done, DEVLINK_ATTR_PAD))
> +			      params->done, DEVLINK_ATTR_PAD))
>  		goto nla_put_failure;
>  	if (nla_put_u64_64bit(msg, DEVLINK_ATTR_FLASH_UPDATE_STATUS_TOTAL,
> -			      total, DEVLINK_ATTR_PAD))
> +			      params->total, DEVLINK_ATTR_PAD))
>  		goto nla_put_failure;
>  	if (nla_put_u64_64bit(msg, DEVLINK_ATTR_FLASH_UPDATE_STATUS_TIMEOUT,
> -			      timeout, DEVLINK_ATTR_PAD))
> +			      params->timeout, DEVLINK_ATTR_PAD))
>  		goto nla_put_failure;
>  
>  out:
> @@ -3068,26 +3063,20 @@ static int devlink_nl_flash_update_fill(struct sk_buff *msg,
>  }
>  
>  static void __devlink_flash_update_notify(struct devlink *devlink,
> -					  enum devlink_command cmd,
> -					  const char *status_msg,
> -					  const char *component,
> -					  unsigned long done,
> -					  unsigned long total,
> -					  unsigned long timeout)
> +					  struct devlink_flash_notify *params)
>  {
>  	struct sk_buff *msg;
>  	int err;
>  
> -	WARN_ON(cmd != DEVLINK_CMD_FLASH_UPDATE &&
> -		cmd != DEVLINK_CMD_FLASH_UPDATE_END &&
> -		cmd != DEVLINK_CMD_FLASH_UPDATE_STATUS);
> +	WARN_ON(params->cmd != DEVLINK_CMD_FLASH_UPDATE &&
> +		params->cmd != DEVLINK_CMD_FLASH_UPDATE_END &&
> +		params->cmd != DEVLINK_CMD_FLASH_UPDATE_STATUS);
>  
>  	msg = nlmsg_new(NLMSG_DEFAULT_SIZE, GFP_KERNEL);
>  	if (!msg)
>  		return;
>  
> -	err = devlink_nl_flash_update_fill(msg, devlink, cmd, status_msg,
> -					   component, done, total, timeout);
> +	err = devlink_nl_flash_update_fill(msg, devlink, params);
>  	if (err)
>  		goto out_free_msg;
>  
> @@ -3101,17 +3090,21 @@ static void __devlink_flash_update_notify(struct devlink *devlink,
>  
>  void devlink_flash_update_begin_notify(struct devlink *devlink)
>  {
> -	__devlink_flash_update_notify(devlink,
> -				      DEVLINK_CMD_FLASH_UPDATE,
> -				      NULL, NULL, 0, 0, 0);
> +	struct devlink_flash_notify params = {
> +		.cmd = DEVLINK_CMD_FLASH_UPDATE,
> +	};
> +
> +	__devlink_flash_update_notify(devlink, &params);
>  }
>  EXPORT_SYMBOL_GPL(devlink_flash_update_begin_notify);
>  
>  void devlink_flash_update_end_notify(struct devlink *devlink)
>  {
> -	__devlink_flash_update_notify(devlink,
> -				      DEVLINK_CMD_FLASH_UPDATE_END,
> -				      NULL, NULL, 0, 0, 0);
> +	struct devlink_flash_notify params = {
> +		.cmd = DEVLINK_CMD_FLASH_UPDATE_END,
> +	};
> +
> +	__devlink_flash_update_notify(devlink, &params);
>  }
>  EXPORT_SYMBOL_GPL(devlink_flash_update_end_notify);
>  
> @@ -3121,9 +3114,15 @@ void devlink_flash_update_status_notify(struct devlink *devlink,
>  					unsigned long done,
>  					unsigned long total)
>  {
> -	__devlink_flash_update_notify(devlink,
> -				      DEVLINK_CMD_FLASH_UPDATE_STATUS,
> -				      status_msg, component, done, total, 0);
> +	struct devlink_flash_notify params = {
> +		.cmd = DEVLINK_CMD_FLASH_UPDATE_STATUS,
> +		.status_msg = status_msg,
> +		.component = component,
> +		.done = done,
> +		.total = total,
> +	};
> +
> +	__devlink_flash_update_notify(devlink, &params);
>  }
>  EXPORT_SYMBOL_GPL(devlink_flash_update_status_notify);
>  
> @@ -3132,9 +3131,14 @@ void devlink_flash_update_timeout_notify(struct devlink *devlink,
>  					 const char *component,
>  					 unsigned long timeout)
>  {
> -	__devlink_flash_update_notify(devlink,
> -				      DEVLINK_CMD_FLASH_UPDATE_STATUS,
> -				      status_msg, component, 0, 0, timeout);
> +	struct devlink_flash_notify params = {
> +		.cmd = DEVLINK_CMD_FLASH_UPDATE_STATUS,
> +		.status_msg = status_msg,
> +		.component = component,
> +		.timeout = timeout,
> +	};
> +
> +	__devlink_flash_update_notify(devlink, &params);
>  }
>  EXPORT_SYMBOL_GPL(devlink_flash_update_timeout_notify);
>  
>
Shannon Nelson Sept. 17, 2020, 8:45 p.m. UTC | #5
On 9/17/20 12:46 PM, Jakub Kicinski wrote:
> On Wed, 16 Sep 2020 20:02:00 -0700 Shannon Nelson wrote:
>> Add a timeout element to the DEVLINK_CMD_FLASH_UPDATE_STATUS
>> netlink message for use by a userland utility to show that
>> a particular firmware flash activity may take a long but
>> bounded time to finish.  Also add a handy helper for drivers
>> to make use of the new timeout value.
>>
>> UI usage hints:
>>   - if non-zero, add timeout display to the end of the status line
>>   	[component] status_msg  ( Xm Ys : Am Bs )
>>       using the timeout value for Am Bs and updating the Xm Ys
>>       every second
>>   - if the timeout expires while awaiting the next update,
>>     display something like
>>   	[component] status_msg  ( timeout reached : Am Bs )
>>   - if new status notify messages are received, remove
>>     the timeout and start over
>>
>> Signed-off-by: Shannon Nelson <snelson@pensando.io>
> Minor nits, otherwise LGTM:
>
> Reviewed-by: Jakub Kicinski <kuba@kernel.org>

Thanks.


>
>> @@ -3052,6 +3054,9 @@ static int devlink_nl_flash_update_fill(struct sk_buff *msg,
>>   	if (nla_put_u64_64bit(msg, DEVLINK_ATTR_FLASH_UPDATE_STATUS_TOTAL,
>>   			      total, DEVLINK_ATTR_PAD))
>>   		goto nla_put_failure;
>> +	if (nla_put_u64_64bit(msg, DEVLINK_ATTR_FLASH_UPDATE_STATUS_TIMEOUT,
>> +			      timeout, DEVLINK_ATTR_PAD))
>> +		goto nla_put_failure;
> nit: since old kernels don't report this user space has to deal with it
>       not being present so I'd be tempted to only report it if timeout
>       is not 0

Hmmm... Unless there are any other comments on this, I think we can be 
consistent with the other fields here.

>
>> +void devlink_flash_update_timeout_notify(struct devlink *devlink,
>> +					 const char *status_msg,
>> +					 const char *component,
>> +					 unsigned long timeout)
>> +{
>> +	__devlink_flash_update_notify(devlink,
>> +				      DEVLINK_CMD_FLASH_UPDATE_STATUS,
>> +				      status_msg, component, 0, 0, timeout);
> nit: did we ever report cmd == UPDATE_STATUS and total == 0?
>       could this cause a division by zero in some unsuspecting
>       implementation? Perhaps we should pass 1 here?

Yes, there are several examples of this.  The userland devlink.c does a 
check for total == 0 before calculating the percentage.

sln
Shannon Nelson Sept. 17, 2020, 8:46 p.m. UTC | #6
On 9/17/20 12:47 PM, Jakub Kicinski wrote:
> On Wed, 16 Sep 2020 20:02:01 -0700 Shannon Nelson wrote:

>> The dev flash status notify function parameter lists are getting

>> rather long, so add a struct to be filled and passed rather than

>> continuously changing the function signatures.

>>

>> Signed-off-by: Shannon Nelson <snelson@pensando.io>

>> ---

>>   include/net/devlink.h | 21 ++++++++++++

>>   net/core/devlink.c    | 80 +++++++++++++++++++++++--------------------

>>   2 files changed, 63 insertions(+), 38 deletions(-)

>>

>> diff --git a/include/net/devlink.h b/include/net/devlink.h

>> index f206accf80ad..9ab2014885cb 100644

>> --- a/include/net/devlink.h

>> +++ b/include/net/devlink.h

>> @@ -391,6 +391,27 @@ struct devlink_param_gset_ctx {

>>   	enum devlink_param_cmode cmode;

>>   };

>>   

>> +/**

>> + * struct devlink_flash_notify - devlink dev flash notify data

>> + * @cmd: devlink notify command code

>> + * @status_msg: current status string

>> + * @component: firmware component being updated

>> + * @done: amount of work completed of total amount

>> + * @total: amount of work expected to be done

>> + * @timeout: expected max timeout in seconds

>> + *

>> + * These are values to be given to userland to be displayed in order

>> + * to show current activity in a firmware update process.

>> + */

>> +struct devlink_flash_notify {

>> +	enum devlink_command cmd;

> I'd leave out cmd out of the params structure, otherwise I'll be

> slightly awkward for drivers to fill in given the current helpers

> are per cmd.


Sure.  I wavered back and forth on that, no problem to change it.

sln
Shannon Nelson Sept. 17, 2020, 8:48 p.m. UTC | #7
On 9/17/20 12:50 PM, Jacob Keller wrote:
> On 9/16/2020 8:02 PM, Shannon Nelson wrote:
>> Add a timeout element to the DEVLINK_CMD_FLASH_UPDATE_STATUS
>> netlink message for use by a userland utility to show that
>> a particular firmware flash activity may take a long but
>> bounded time to finish.  Also add a handy helper for drivers
>> to make use of the new timeout value.
>>
>> UI usage hints:
>>   - if non-zero, add timeout display to the end of the status line
>>   	[component] status_msg  ( Xm Ys : Am Bs )
>>       using the timeout value for Am Bs and updating the Xm Ys
>>       every second
>>   - if the timeout expires while awaiting the next update,
>>     display something like
>>   	[component] status_msg  ( timeout reached : Am Bs )
>>   - if new status notify messages are received, remove
>>     the timeout and start over
>>
>> Signed-off-by: Shannon Nelson <snelson@pensando.io>
>> ---
> This one looks good to me. I think the only other things that I saw from
> previous discussion are:
>
> a) we could convert the internal helper devlink_nl_flash_update_fill and
> __devlink_flash_update_notify to use structs for their fields, and..
>
> b) Jakub pointed out that most drivers don't currently use the component
> field so we could remove that from the helpers.
>
> However, I don't have strong feelings about those either way, so:
>
> Reviewed-by: Jacob Keller <jacob.e.keller@intel.com>
>
>
Thanks,
sln