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

[Jakub Kicinski <jakub.kicinski@netronome.com>]

On Mon, 11 Feb 2019 17:45:13 +0100, Jiri Pirko wrote:
> 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

Agreed, that looks good!  TBH in case of Netronome the binary
image contains an identifier so it will update the correct component
automatically.  That's why I say "no immediate need" :)  (How about
"component" instead of "id", BTW?)

I will drop the target ID, I just added it for full backward compat
with ethtool, but it may be confusing, given it would be mostly unused.
I'll drop it in non-RFC, do you want me to add the id/component or leave
it out for now?