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=-6.9 required=3.0 tests=DKIMWL_WL_HIGH,DKIM_SIGNED, DKIM_VALID,DKIM_VALID_AU,HEADER_FROM_DIFFERENT_DOMAINS,INCLUDES_PATCH, MAILING_LIST_MULTI,SIGNED_OFF_BY,SPF_HELO_NONE,SPF_PASS 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 67AD3C433DF for ; Mon, 1 Jun 2020 08:53:34 +0000 (UTC) Received: from vger.kernel.org (vger.kernel.org [23.128.96.18]) by mail.kernel.org (Postfix) with ESMTP id 35FA1206C3 for ; Mon, 1 Jun 2020 08:53:34 +0000 (UTC) Authentication-Results: mail.kernel.org; dkim=pass (1024-bit key) header.d=broadcom.com header.i=@broadcom.com header.b="D5r016sl" Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1725876AbgFAIxd (ORCPT ); Mon, 1 Jun 2020 04:53:33 -0400 Received: from lindbergh.monkeyblade.net ([23.128.96.19]:35310 "EHLO lindbergh.monkeyblade.net" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1725290AbgFAIxc (ORCPT ); Mon, 1 Jun 2020 04:53:32 -0400 Received: from mail-lf1-x142.google.com (mail-lf1-x142.google.com [IPv6:2a00:1450:4864:20::142]) by lindbergh.monkeyblade.net (Postfix) with ESMTPS id 6B060C061A0E for ; Mon, 1 Jun 2020 01:53:32 -0700 (PDT) Received: by mail-lf1-x142.google.com with SMTP id r125so3440194lff.13 for ; Mon, 01 Jun 2020 01:53:32 -0700 (PDT) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=broadcom.com; s=google; h=mime-version:references:in-reply-to:from:date:message-id:subject:to :cc; bh=fv+vfcvFFtchtnXKCS/2t+mcirEZrhc2fOAtUM0gKwI=; b=D5r016slYQFJP50E5pfkan5cSEWwGqbqU49GaYL76LT0wFhvmxgBN3e8DN0FKyhqU+ DuO+OIlqXyZ9hA0pgvNDgD3t0rHqrsbStgIB/4kugWgXOCFtbUeH77Z/E4mp1hedYVkY B2I0zRdkGlNP/o89kq+qWDgRA1tvNjPdy+Q3k= X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20161025; h=x-gm-message-state:mime-version:references:in-reply-to:from:date :message-id:subject:to:cc; bh=fv+vfcvFFtchtnXKCS/2t+mcirEZrhc2fOAtUM0gKwI=; b=ZQYRXtoka9ccraJr1dxqgriivymnUC7slFm4Py9AvWc0RcYY/qUucyk7qEs5V43VMs kJ8o6G5/THQWfsv4iLNBuAE08gsvakMzE5yrSqWIBKrUCUnCNdIPKrrzsvzONNiN6Elw GnLDWWvHvJw1NMOpGEaR+raWERw/mzBVCASxkyu1AHT4hJ+sO0uUMysuW1iBUrNKnzYJ ioPjmNM2mauX7oigusYj7TvovB/ljtgr3F6nzFkuc3DLLXH3Uyg4JyQ5iSNZ7f//PN2j WRC41Myx0yu/ILDIEtg0z2/W9nbckU50ZeI/cQd/XHEJZatuAlDBgKqfOL7sqkyuabrM Wf/Q== X-Gm-Message-State: AOAM531NfRPRt2Zz2X/94y0IZTyoSlKxfJcNk56z5ffA+3eWRe2d7T0t 8whZeTjJSz1mkNKU8JUcieJxDAQS0Pg06qZMZLxM5A== X-Google-Smtp-Source: ABdhPJy3Thn1edF1ZMgW23oAEJ2PoTboRxG5jz0R/wNRFlFvVPCHsN5HE6yeEkzuh4Ss1DrvOjkEukl7kbp7uhiPk/o= X-Received: by 2002:ac2:5cac:: with SMTP id e12mr10863959lfq.92.1591001610578; Mon, 01 Jun 2020 01:53:30 -0700 (PDT) MIME-Version: 1.0 References: <1590214105-10430-1-git-send-email-vasundhara-v.volam@broadcom.com> <1590214105-10430-2-git-send-email-vasundhara-v.volam@broadcom.com> <20200524045335.GA22938@nanopsycho> <20200525172602.GA14161@nanopsycho> <20200526044727.GB14161@nanopsycho> <20200526134032.GD14161@nanopsycho> <20200601071847.GG2282@nanopsycho> In-Reply-To: <20200601071847.GG2282@nanopsycho> From: Vasundhara Volam Date: Mon, 1 Jun 2020 14:23:19 +0530 Message-ID: Subject: Re: [PATCH v2 net-next 1/4] devlink: Add new "allow_fw_live_reset" generic device parameter. To: Jiri Pirko Cc: David Miller , Netdev , Jiri Pirko , Michael Chan Content-Type: text/plain; charset="UTF-8" Sender: netdev-owner@vger.kernel.org Precedence: bulk List-ID: X-Mailing-List: netdev@vger.kernel.org On Mon, Jun 1, 2020 at 12:48 PM Jiri Pirko wrote: > > Tue, May 26, 2020 at 04:23:48PM CEST, vasundhara-v.volam@broadcom.com wrote: > >On Tue, May 26, 2020 at 7:10 PM Jiri Pirko wrote: > >> > >> Tue, May 26, 2020 at 08:42:28AM CEST, vasundhara-v.volam@broadcom.com wrote: > >> >On Tue, May 26, 2020 at 10:17 AM Jiri Pirko wrote: > >> >> > >> >> Tue, May 26, 2020 at 06:28:59AM CEST, vasundhara-v.volam@broadcom.com wrote: > >> >> >On Mon, May 25, 2020 at 10:56 PM Jiri Pirko wrote: > >> >> >> > >> >> >> Sun, May 24, 2020 at 08:29:56AM CEST, vasundhara-v.volam@broadcom.com wrote: > >> >> >> >On Sun, May 24, 2020 at 10:23 AM Jiri Pirko wrote: > >> >> >> >> > >> >> >> >> Sat, May 23, 2020 at 08:08:22AM CEST, vasundhara-v.volam@broadcom.com wrote: > >> >> >> >> >Add a new "allow_fw_live_reset" generic device bool parameter. When > >> >> >> >> >parameter is set, user is allowed to reset the firmware in real time. > >> >> >> >> > > >> >> >> >> >This parameter is employed to communicate user consent or dissent for > >> >> >> >> >the live reset to happen. A separate command triggers the actual live > >> >> >> >> >reset. > >> >> >> >> > > >> >> >> >> >Cc: Jiri Pirko > >> >> >> >> >Signed-off-by: Vasundhara Volam > >> >> >> >> >Signed-off-by: Michael Chan > >> >> >> >> >--- > >> >> >> >> >v2: Rename param name to "allow_fw_live_reset" from > >> >> >> >> >"enable_hot_fw_reset". > >> >> >> >> >Update documentation for the param in devlink-params.rst file. > >> >> >> >> >--- > >> >> >> >> > Documentation/networking/devlink/devlink-params.rst | 6 ++++++ > >> >> >> >> > include/net/devlink.h | 4 ++++ > >> >> >> >> > net/core/devlink.c | 5 +++++ > >> >> >> >> > 3 files changed, 15 insertions(+) > >> >> >> >> > > >> >> >> >> >diff --git a/Documentation/networking/devlink/devlink-params.rst b/Documentation/networking/devlink/devlink-params.rst > >> >> >> >> >index d075fd0..ad54dfb 100644 > >> >> >> >> >--- a/Documentation/networking/devlink/devlink-params.rst > >> >> >> >> >+++ b/Documentation/networking/devlink/devlink-params.rst > >> >> >> >> >@@ -108,3 +108,9 @@ own name. > >> >> >> >> > * - ``region_snapshot_enable`` > >> >> >> >> > - Boolean > >> >> >> >> > - Enable capture of ``devlink-region`` snapshots. > >> >> >> >> >+ * - ``allow_fw_live_reset`` > >> >> >> >> >+ - Boolean > >> >> >> >> >+ - Firmware live reset allows users to reset the firmware in real time. > >> >> >> >> >+ For example, after firmware upgrade, this feature can immediately reset > >> >> >> >> >+ to run the new firmware without reloading the driver or rebooting the > >> >> >> >> > >> >> >> >> This does not tell me anything about the reset being done on another > >> >> >> >> host. You need to emhasize that, in the name of the param too. > >> >> >> >I am not sure if I completely understand your query. > >> >> >> > > >> >> >> >Reset is actually initiated by one of the PF/host of the device, which > >> >> >> >resets the entire same device. > >> >> >> > > >> >> >> >Reset is not initiated by any other remote device/host. > >> >> >> > >> >> >> Well, in case of multihost system, it might be, right? > >> >> >> > >> >> >In case of multi-host system also, it is one of the host that triggers > >> >> >the reset, which resets the entire same device. I don't think this is > >> >> >remote. > >> >> > > >> >> >As the parameter is a device parameter, it is applicable to the entire > >> >> >device. When a user initiates the reset from any of the host in case > >> >> >of multi-host and any of the PF in case of stand-alone or smartNIC > >> >> >device, the entire device goes for a reset. > >> >> > > >> >> >I will be expanding the description to the following to make it more clear. > >> >> > > >> >> >------------------------ > >> >> >- Firmware live reset allows users to reset the firmware in real time. > >> >> >For example, after firmware upgrade, this feature can immediately > >> >> >reset to run the new firmware without reloading the driver or > >> >> >rebooting the system. > >> >> >When a user initiates the reset from any of the host (in case of > >> >> >multi-host system) / PF (in case of stand-alone or smartNIC device), > >> >> >the entire device goes for a reset when the parameter is enabled. > >> >> > >> >> Sorry, this is still not clear. I think that you are mixing up two > >> >> different things: > >> >> 1) option of devlink reload to indicate that user is interested in "live > >> >> reset" of firmware without reloading driver > >> > > >> >This is the option we are trying to add. If a user is interested in > >> >"live reset", he needs to enable the parameter to enable it in device > >> >capabilities, which is achieved by permanent configuration mode. When > >> >capability is enabled in the device, new firmware which is aware will > >> >allocate the resources and exposes the capability to host drivers. > >> > > >> >But firmware allows the "live reset" only when all the loaded drivers > >> >are aware of/supports the capability. For example, if any of the host > >> >is loaded with an old driver, "live reset" is not allowed until the > >> >driver is upgraded or unloaded. or if the host driver turns it off, > >> >then also "live reset" is not allowed. > >> > > >> >In case of runtime parameter cmode, if any of the host turns off the > >> >capability in the host driver, "live reset" is not allowed until the > >> >driver is unloaded or the user enables it again. > >> > > >> >To make it clear, I can add two parameters. > >> > > >> >1. enable_fw_live_reset - To indicate that the user is interested in > >> >"live reset". This will be a generic param. > >> > >> As I wrote above, I believe this should be an option > >> to "devlink dev reload", not a param. > >I think you are still confused with enabling feature in NVRAM > >configuration of the device and command to trigger reset. This param > >will enable the feature in the device NVRAM configuration and does not > >trigger the actual reset. > > > >Only when the param is set, feature will be enabled in the device and > >firmware supports the "live reset". When the param is disabled, > >firmware cannot support "live reset" and user needs to do PCIe reset > >after flashing the firmware for it to take effect.. > > Does that mean that after reboot, when user triggers fw reset, it will > be always "live" is possible? Meaning, user will no have a way to > specify that per-reset? Right now, there is no option for user to mention the type of reset. As you suggested, we need to extend 'devlink dev reload' for users to mention the type of reset. > > > > > >Once feature is enabled in NVRAM configuration, it will be persistent > >across reboots. > > > >User still needs to use "devlink dev reload" command to do the "live reset". > >> > >> > >> > > >> >2. allow_fw_live_reset - To indicate, if any of the host/PF turns it > >> >off, "live reset" is not allowed. This serves the purpose of what we > >> >are trying to add in runtime cmode. > >> > >> Yeah. > >And this param will enable the feature in the driver for driver to > >allow the firmware to go for "live reset", where as above param will > >enable the feature in NVRAM configuration of the device. > >> > >> >Do you want me to keep it as a driver-specific param? > >> > >> There is nothing driver-specific about this. > >okay. > >> > >> > >> > > >> >Please let me know if this is clear and makes less confusion. > >> > > >> >Thanks, > >> >Vasundhara > >> > > >> >> 2) devlink param that would indicate "I am okay if someone else (not by > >> >> my devlink instance) resets my firmware". > >> >> > >> >> Could you please split? > >> >> > >> >> > >> >> >------------------------ > >> >> > > >> >> >Thanks, > >> >> >Vasundhara > >> >> >> > >> >> >> > > >> >> >> >Thanks, > >> >> >> >Vasundhara > >> >> >> >> > >> >> >> >> > >> >> >> >> > >> >> >> >> >+ system. > >> >> >> >> >diff --git a/include/net/devlink.h b/include/net/devlink.h > >> >> >> >> >index 8ffc1b5c..488b61c 100644 > >> >> >> >> >--- a/include/net/devlink.h > >> >> >> >> >+++ b/include/net/devlink.h > >> >> >> >> >@@ -406,6 +406,7 @@ enum devlink_param_generic_id { > >> >> >> >> > DEVLINK_PARAM_GENERIC_ID_FW_LOAD_POLICY, > >> >> >> >> > DEVLINK_PARAM_GENERIC_ID_RESET_DEV_ON_DRV_PROBE, > >> >> >> >> > DEVLINK_PARAM_GENERIC_ID_ENABLE_ROCE, > >> >> >> >> >+ DEVLINK_PARAM_GENERIC_ID_ALLOW_FW_LIVE_RESET, > >> >> >> >> > > >> >> >> >> > /* add new param generic ids above here*/ > >> >> >> >> > __DEVLINK_PARAM_GENERIC_ID_MAX, > >> >> >> >> >@@ -443,6 +444,9 @@ enum devlink_param_generic_id { > >> >> >> >> > #define DEVLINK_PARAM_GENERIC_ENABLE_ROCE_NAME "enable_roce" > >> >> >> >> > #define DEVLINK_PARAM_GENERIC_ENABLE_ROCE_TYPE DEVLINK_PARAM_TYPE_BOOL > >> >> >> >> > > >> >> >> >> >+#define DEVLINK_PARAM_GENERIC_ALLOW_FW_LIVE_RESET_NAME "allow_fw_live_reset" > >> >> >> >> >+#define DEVLINK_PARAM_GENERIC_ALLOW_FW_LIVE_RESET_TYPE DEVLINK_PARAM_TYPE_BOOL > >> >> >> >> >+ > >> >> >> >> > #define DEVLINK_PARAM_GENERIC(_id, _cmodes, _get, _set, _validate) \ > >> >> >> >> > { \ > >> >> >> >> > .id = DEVLINK_PARAM_GENERIC_ID_##_id, \ > >> >> >> >> >diff --git a/net/core/devlink.c b/net/core/devlink.c > >> >> >> >> >index 7b76e5f..8544f23 100644 > >> >> >> >> >--- a/net/core/devlink.c > >> >> >> >> >+++ b/net/core/devlink.c > >> >> >> >> >@@ -3011,6 +3011,11 @@ static int devlink_nl_cmd_flash_update(struct sk_buff *skb, > >> >> >> >> > .name = DEVLINK_PARAM_GENERIC_ENABLE_ROCE_NAME, > >> >> >> >> > .type = DEVLINK_PARAM_GENERIC_ENABLE_ROCE_TYPE, > >> >> >> >> > }, > >> >> >> >> >+ { > >> >> >> >> >+ .id = DEVLINK_PARAM_GENERIC_ID_ALLOW_FW_LIVE_RESET, > >> >> >> >> >+ .name = DEVLINK_PARAM_GENERIC_ALLOW_FW_LIVE_RESET_NAME, > >> >> >> >> >+ .type = DEVLINK_PARAM_GENERIC_ALLOW_FW_LIVE_RESET_TYPE, > >> >> >> >> >+ }, > >> >> >> >> > }; > >> >> >> >> > > >> >> >> >> > static int devlink_param_generic_verify(const struct devlink_param *param) > >> >> >> >> >-- > >> >> >> >> >1.8.3.1 > >> >> >> >> >