Re: [RFC 1/3] devlink: add flash update command

[Jiri Pirko <jiri@resnulli.us>]

Mon, Feb 11, 2019 at 07:59:20AM CET, jakub.kicinski@netronome.com wrote:
>Add devlink flash update command. Advanced NICs have firmware
>stored in flash and often cryptographically secured. Updating
>that flash is handled by management firmware. Ethtool has a
>flash update command which served us well, however, it has two
>shortcomings:
> - it takes rtnl_lock unnecessarily - really flash update has
>   nothing to do with networking, so using a networking device
>   as a handle is suboptimal, which leads us to the second one:
> - it requires a functioning netdev - in case device enters an
>   error state and can't spawn a netdev (e.g. communication
>   with the device fails) there is no netdev to use as a handle
>   for flashing.
>
>Devlink already has the ability to report the firmware versions,
>now with the ability to update the firmware/flash we will be
>able to recover devices in bad state.
>
>To enable easy interoperability with ethtool add the target
>partition ID. We may or may not add a different method of
>identification, but there is no such immediate need.
>
>Signed-off-by: Jakub Kicinski <jakub.kicinski@netronome.com>
>---
> include/net/devlink.h        |  2 ++
> include/uapi/linux/devlink.h |  6 ++++++
> net/core/devlink.c           | 30 ++++++++++++++++++++++++++++++
> 3 files changed, 38 insertions(+)
>
>diff --git a/include/net/devlink.h b/include/net/devlink.h
>index 07660fe4c0e3..55b3478b1291 100644
>--- a/include/net/devlink.h
>+++ b/include/net/devlink.h
>@@ -529,6 +529,8 @@ struct devlink_ops {
> 				      struct netlink_ext_ack *extack);
> 	int (*info_get)(struct devlink *devlink, struct devlink_info_req *req,
> 			struct netlink_ext_ack *extack);
>+	int (*flash_update)(struct devlink *devlink, const char *path,
>+			    u32 target, struct netlink_ext_ack *extack);
> };
> 
> static inline void *devlink_priv(struct devlink *devlink)
>diff --git a/include/uapi/linux/devlink.h b/include/uapi/linux/devlink.h
>index 72d9f7c89190..f4417283fd1b 100644
>--- a/include/uapi/linux/devlink.h
>+++ b/include/uapi/linux/devlink.h
>@@ -103,6 +103,8 @@ enum devlink_command {
> 	DEVLINK_CMD_HEALTH_REPORTER_DUMP_GET,
> 	DEVLINK_CMD_HEALTH_REPORTER_DUMP_CLEAR,
> 
>+	DEVLINK_CMD_FLASH_UPDATE,
>+
> 	/* add new commands above here */
> 	__DEVLINK_CMD_MAX,
> 	DEVLINK_CMD_MAX = __DEVLINK_CMD_MAX - 1
>@@ -326,6 +328,10 @@ enum devlink_attr {
> 	DEVLINK_ATTR_HEALTH_REPORTER_DUMP_TS,		/* u64 */
> 	DEVLINK_ATTR_HEALTH_REPORTER_GRACEFUL_PERIOD,	/* u64 */
> 	DEVLINK_ATTR_HEALTH_REPORTER_AUTO_RECOVER,	/* u8 */
>+
>+	DEVLINK_ATTR_FLASH_UPDATE_FILE_NAME,	/* string */
>+	DEVLINK_ATTR_FLASH_UPDATE_TARGET_ID,	/* u32 */

Do we need to carry this on? I mean, the ef->region is only checked in 4
drivers against ETHTOOL_FLASH_ALL_REGIONS, which is default.
There is this bnxt driver which is the only one working with this value.
I think that for compat, there should be some id-region mapping
provided by driver which the compat layer would use to translate.

I also think that this should be in sync with what is returned in
DEVLINK_ATTR_INFO_VERSION_NAME.

For example:
$ devlink dev info pci/0000:82:00.0
pci/0000:82:00.0:
  driver nfp
  serial_number 16240145
  versions:
      fixed:
        board.id AMDA0081-0001
        board.rev 15
        board.vendor SMA
        board.model hydrogen
      running:
        fw.mgmt 010181.010181.0101d4
        fw.cpld 0x1030000
        fw.app abm-d372b6
        fw.undi 0.0.2
        chip.init AMDA-0081-0001  20160318164536
      stored:
        fw.mgmt 010181.010181.0101d4
        fw.app abm-d372b6
        fw.undi 0.0.2
        chip.init AMDA-0081-0001  20160318164536

Now user should be able to use one of the identifiers to flash relevant
fw, like:

devlink dev flash pci/0000:82:00.0 XXX fw.mgmt file flash-boot.bin

I'm not sure about the name of "xxx" attribute. Maybe "id":

devlink dev flash pci/0000:82:00.0 id fw.mgmt file flash-boot.bin
devlink dev flash pci/0000:82:00.0 id fw.cpld file some-other.bin



[...]