From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: X-Spam-Checker-Version: SpamAssassin 3.4.0 (2014-02-07) on aws-us-west-2-korg-lkml-1.web.codeaurora.org X-Spam-Level: X-Spam-Status: No, score=-8.5 required=3.0 tests=DKIMWL_WL_MED,DKIM_SIGNED, DKIM_VALID,HEADER_FROM_DIFFERENT_DOMAINS,INCLUDES_PATCH,MAILING_LIST_MULTI, SIGNED_OFF_BY,SPF_PASS,URIBL_BLOCKED,USER_AGENT_MUTT autolearn=ham autolearn_force=no version=3.4.0 Received: from mail.kernel.org (mail.kernel.org [198.145.29.99]) by smtp.lore.kernel.org (Postfix) with ESMTP id 11F39C282C4 for ; Tue, 12 Feb 2019 07:36:07 +0000 (UTC) Received: from vger.kernel.org (vger.kernel.org [209.132.180.67]) by mail.kernel.org (Postfix) with ESMTP id BE6302186A for ; Tue, 12 Feb 2019 07:36:06 +0000 (UTC) Authentication-Results: mail.kernel.org; dkim=pass (2048-bit key) header.d=resnulli-us.20150623.gappssmtp.com header.i=@resnulli-us.20150623.gappssmtp.com header.b="GRhF68EM" Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1726864AbfBLHgF (ORCPT ); Tue, 12 Feb 2019 02:36:05 -0500 Received: from mail-wr1-f67.google.com ([209.85.221.67]:36392 "EHLO mail-wr1-f67.google.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1726040AbfBLHgF (ORCPT ); Tue, 12 Feb 2019 02:36:05 -0500 Received: by mail-wr1-f67.google.com with SMTP id o17so1508982wrw.3 for ; Mon, 11 Feb 2019 23:36:03 -0800 (PST) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=resnulli-us.20150623.gappssmtp.com; s=20150623; h=date:from:to:cc:subject:message-id:references:mime-version :content-disposition:in-reply-to:user-agent; bh=72kq7zeL2bAyY5yM8ybXF256q8VOm8bT5A1jsocL9M0=; b=GRhF68EMnYFyChxIVhk/YNIr9wL4j7GDN6foX6a8sZ51UuHUOy1FtpCWeQje7Sg3Te 9cjn+Hul5dY0ZyEkJ816L3gWCtu1H1sXIhMAjPyNDYBOhdp6Zujv/28Xkp25PTXuUrm/ wi/l6drQBYKty7mlhWd97WwWjJTWHih3FOqCIwJBTQVR6LjLSYYdx48UN0rZDBg595/B 0bCuchbfRTtN3ZC4Fl3ZeAN+tpeHvGKwniaQ9hze3yWv9Tc0wveVjw+mMN04SilmZAXN lqvnvgDOH9rhieF20A5/zQ9vWFqR097Y0TCFIhA1QbWw93u/PzPeHDZRnHJVPDPc0KJO XCtw== X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20161025; h=x-gm-message-state:date:from:to:cc:subject:message-id:references :mime-version:content-disposition:in-reply-to:user-agent; bh=72kq7zeL2bAyY5yM8ybXF256q8VOm8bT5A1jsocL9M0=; b=txLjYu+64QTFMoHob7Fdyf8mOyRme1BSCjVG7S0Hjk3+yXrJ5KmKnkVCq/+qWmwI9r 2EpAGjhMx56RBaEdqX+KxnpxcSO3lg3YwYAwoJuhxsOj3cWGjbVWgiKgiUjt/Jpqzy7E SZtL9Z2CXCW6d+tpiWgwJlnTRGDI8LNYQJx9ZG+fWR8Fb3GfRsIRYFDyiYfurC07hPUb AVozisexul3CbgbiZw2y5pEHBh8dhn1lVkCiwTolCLRO43MvyckUc/wTl7wQuyZNprF/ YCxTDH9i5hw9+5Mb0tlbG1vvrsedpzNGPV6DKCqKQJzUHDt9TERukMbC29v2MF1VrF6W cDag== X-Gm-Message-State: AHQUAuZXEhoumUcV5fOwqDApJWE29G8l7DNINVPQJCTQ+O7QVxly6mYF n+HYS7MdJr1HAu9b/oveRFLWr/C6Fzc= X-Google-Smtp-Source: AHgI3IYeVJIaODnYe1IPALeyhUGDrKYFX7OGm4oj5C5o8ROjhHSnt7TPYe14xJQUhykADENibrMFTg== X-Received: by 2002:adf:e385:: with SMTP id e5mr1743623wrm.267.1549956962724; Mon, 11 Feb 2019 23:36:02 -0800 (PST) Received: from localhost (jirka.pirko.cz. [84.16.102.26]) by smtp.gmail.com with ESMTPSA id b13sm1935687wmj.42.2019.02.11.23.36.01 (version=TLS1_2 cipher=ECDHE-RSA-CHACHA20-POLY1305 bits=256/256); Mon, 11 Feb 2019 23:36:02 -0800 (PST) Date: Tue, 12 Feb 2019 08:26:39 +0100 From: Jiri Pirko To: Jakub Kicinski Cc: davem@davemloft.net, netdev@vger.kernel.org, oss-drivers@netronome.com, mkubecek@suse.cz, andrew@lunn.ch Subject: Re: [oss-drivers] Re: [RFC 1/3] devlink: add flash update command Message-ID: <20190212072639.GJ2314@nanopsycho.orion> References: <20190211065923.22670-1-jakub.kicinski@netronome.com> <20190211065923.22670-2-jakub.kicinski@netronome.com> <20190211164513.GI2314@nanopsycho.orion> <20190212082530.48cbb382@cakuba.netronome.com> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: <20190212082530.48cbb382@cakuba.netronome.com> User-Agent: Mutt/1.10.1 (2018-07-13) Sender: netdev-owner@vger.kernel.org Precedence: bulk List-ID: X-Mailing-List: netdev@vger.kernel.org Mon, Feb 11, 2019 at 08:25:30PM CET, jakub.kicinski@netronome.com wrote: >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 >> >--- >> > 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 Okay. So in case the "component" attr is omitted, there would be some flag passed to the driver so it would know that the file contains more component binaries and has to do parsing itself/in-fw. >automatically. That's why I say "no immediate need" :) (How about >"component" instead of "id", BTW?) Component is fine by me. > >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. Ok. >I'll drop it in non-RFC, do you want me to add the id/component or leave >it out for now? I think it would be good to add it and have the api complete.