Message ID | 20231030160044.20355-1-dwagner@suse.de |
---|---|
State | New |
Headers |
Return-Path: <linux-kernel-owner@vger.kernel.org> Delivered-To: ouuuleilei@gmail.com Received: by 2002:a59:d641:0:b0:403:3b70:6f57 with SMTP id cy1csp2318718vqb; Mon, 30 Oct 2023 08:59:37 -0700 (PDT) X-Google-Smtp-Source: AGHT+IHEGcqWBKJRXfbrI4+JanSgGpWQgjcQ81qbJwr3RpoJ7G7rbMegXAntJ3v6REG0GeDD6Dz0 X-Received: by 2002:a17:903:2351:b0:1cc:37b9:4953 with SMTP id c17-20020a170903235100b001cc37b94953mr4420750plh.67.1698681577332; Mon, 30 Oct 2023 08:59:37 -0700 (PDT) ARC-Seal: i=1; a=rsa-sha256; t=1698681577; cv=none; d=google.com; s=arc-20160816; b=D78PGpqQvlpkLnOymHU/YBAcReKo7zdmamdCqv/6O2sQ3q5eH0gMuAyQBoycDytAQj 1Oi3JP9IXyhOj6LcDi13BNE9TamKs9YPatoO9EYc2vyo9eda4ewmPZNcmOxxz5SbR1+i FTB2wckiColloCRuJIXuXwf0N1faWhulHHRxJojhcVWY3zXJWRmCuJrsx4M7NwZsQU/I iMD/Htfl+GDjqy8EKgTbC2Dly9ylnpEWJcXefxHs7SRFQ1gl8279S9DPswX5OeFjOoAt njBghzNDObkXvLwAybywKE5mA3psTrTY1gO1F/aU1pwn+wxi9S8LdYLi/DXx3Mi4LKbc y1Fw== ARC-Message-Signature: i=1; a=rsa-sha256; c=relaxed/relaxed; d=google.com; s=arc-20160816; h=list-id:precedence:content-transfer-encoding:mime-version :message-id:date:subject:cc:to:from:dkim-signature:dkim-signature; bh=ZeP5LZNPVgCOVDKGuBqMoGJSW7piHcySROsgnm697W0=; fh=y18LZfenZEWKaXfvX5UX+EWGZCaPZS2yh2hLlxumw68=; b=mj17rYbQocUfLeJsqxLg/UBFBvXVjZPkJY+KjaSfShVF53GaMUPxO7zbF0xhE+exQN YBiPS1nTfNRdicvBkAxIA01FzQXWH5c2CE6zbSU2Yr0AAVcoDlYA7P6MvDcLZBN++/zO qd0gyVxWA33+s8i3IOsh+tOc9Kpij7+abdMyB5hUnDFLTssXgtB4pMhxvI+PaHhfqd62 bYyzbqKzmxENrCvS+Pfgt7Y6dBLZ2BZhleSjfD7wBnNLDcRNdD0yRPn2ZaPA9wjBgbeC vXbDZiBQbuoszUfaqVP1LCxFSoJEmhymJIZLVjVHpKRnoJnRjGi4/hOnAWEXamfKalJ3 Pt2A== ARC-Authentication-Results: i=1; mx.google.com; dkim=pass header.i=@suse.de header.s=susede2_rsa header.b=J44VmEqc; dkim=neutral (no key) header.i=@suse.de header.s=susede2_ed25519 header.b=YL7UvprA; spf=pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 23.128.96.36 as permitted sender) smtp.mailfrom=linux-kernel-owner@vger.kernel.org; dmarc=pass (p=NONE sp=NONE dis=NONE) header.from=suse.de Received: from pete.vger.email (pete.vger.email. [23.128.96.36]) by mx.google.com with ESMTPS id t8-20020a170902bc4800b001cc2a6624e5si4205711plz.307.2023.10.30.08.59.37 (version=TLS1_3 cipher=TLS_AES_256_GCM_SHA384 bits=256/256); Mon, 30 Oct 2023 08:59:37 -0700 (PDT) Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 23.128.96.36 as permitted sender) client-ip=23.128.96.36; Authentication-Results: mx.google.com; dkim=pass header.i=@suse.de header.s=susede2_rsa header.b=J44VmEqc; dkim=neutral (no key) header.i=@suse.de header.s=susede2_ed25519 header.b=YL7UvprA; spf=pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 23.128.96.36 as permitted sender) smtp.mailfrom=linux-kernel-owner@vger.kernel.org; dmarc=pass (p=NONE sp=NONE dis=NONE) header.from=suse.de Received: from out1.vger.email (depot.vger.email [IPv6:2620:137:e000::3:0]) by pete.vger.email (Postfix) with ESMTP id 1F23280A1A29; Mon, 30 Oct 2023 08:59:33 -0700 (PDT) X-Virus-Status: Clean X-Virus-Scanned: clamav-milter 0.103.10 at pete.vger.email Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S233751AbjJ3P7S (ORCPT <rfc822;chrisjones.unixmen@gmail.com> + 32 others); Mon, 30 Oct 2023 11:59:18 -0400 Received: from lindbergh.monkeyblade.net ([23.128.96.19]:50106 "EHLO lindbergh.monkeyblade.net" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S233311AbjJ3P7Q (ORCPT <rfc822;linux-kernel@vger.kernel.org>); Mon, 30 Oct 2023 11:59:16 -0400 Received: from smtp-out1.suse.de (smtp-out1.suse.de [IPv6:2001:67c:2178:6::1c]) by lindbergh.monkeyblade.net (Postfix) with ESMTPS id B597CE4 for <linux-kernel@vger.kernel.org>; Mon, 30 Oct 2023 08:59:13 -0700 (PDT) Received: from imap2.suse-dmz.suse.de (imap2.suse-dmz.suse.de [192.168.254.74]) (using TLSv1.3 with cipher TLS_AES_256_GCM_SHA384 (256/256 bits) key-exchange X25519 server-signature ECDSA (P-521) server-digest SHA512) (No client certificate requested) by smtp-out1.suse.de (Postfix) with ESMTPS id 4B1DA210E7; Mon, 30 Oct 2023 15:59:12 +0000 (UTC) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=suse.de; s=susede2_rsa; t=1698681552; h=from:from:reply-to:date:date:message-id:message-id:to:to:cc:cc: mime-version:mime-version: content-transfer-encoding:content-transfer-encoding; bh=ZeP5LZNPVgCOVDKGuBqMoGJSW7piHcySROsgnm697W0=; b=J44VmEqcZrDXMGx1wOc89obvtxfjsdVZFguVq7pP3N+MPb0pe0B/voCzU/h48z6UYLdqAd JrrwBBdSJ51wGxjcU52tTozeIPOTIeYO6K83LJsE13UrBkjW7gzzBTIHZuleo5PHBsWSPr mc4LXuvDuRtO7cv1vo31gEo+5jM37Wc= DKIM-Signature: v=1; a=ed25519-sha256; c=relaxed/relaxed; d=suse.de; s=susede2_ed25519; t=1698681552; h=from:from:reply-to:date:date:message-id:message-id:to:to:cc:cc: mime-version:mime-version: content-transfer-encoding:content-transfer-encoding; bh=ZeP5LZNPVgCOVDKGuBqMoGJSW7piHcySROsgnm697W0=; b=YL7UvprAyY6BY8MIOhFh0wB9Pj1IPK/1X6jXBIm7IGrFc6sByhq8KnPHL77tJtfMmX9QmM b2DtEclQYbF7nDCg== Received: from imap2.suse-dmz.suse.de (imap2.suse-dmz.suse.de [192.168.254.74]) (using TLSv1.3 with cipher TLS_AES_256_GCM_SHA384 (256/256 bits) key-exchange X25519 server-signature ECDSA (P-521) server-digest SHA512) (No client certificate requested) by imap2.suse-dmz.suse.de (Postfix) with ESMTPS id 3D9F3138EF; Mon, 30 Oct 2023 15:59:12 +0000 (UTC) Received: from dovecot-director2.suse.de ([192.168.254.65]) by imap2.suse-dmz.suse.de with ESMTPSA id HV0FD9DSP2WufAAAMHmgww (envelope-from <dwagner@suse.de>); Mon, 30 Oct 2023 15:59:12 +0000 From: Daniel Wagner <dwagner@suse.de> To: linux-nvme@lists.infradead.org Cc: linux-kernel@vger.kernel.org, Keith Busch <kbusch@kernel.org>, Christoph Hellwig <hch@lst.de>, Niklas Cassel <Niklas.Cassel@wdc.com>, Daniel Wagner <dwagner@suse.de>, Kenji Tomonaga <tkenbo@gmail.com> Subject: [PATCH v3] nvme: update firmware version after commit Date: Mon, 30 Oct 2023 17:00:44 +0100 Message-ID: <20231030160044.20355-1-dwagner@suse.de> X-Mailer: git-send-email 2.42.0 MIME-Version: 1.0 Content-Transfer-Encoding: 8bit X-Spam-Status: No, score=-0.8 required=5.0 tests=DKIM_SIGNED,DKIM_VALID, DKIM_VALID_AU,HEADER_FROM_DIFFERENT_DOMAINS,MAILING_LIST_MULTI, SPF_HELO_NONE,SPF_PASS autolearn=unavailable autolearn_force=no version=3.4.6 X-Spam-Checker-Version: SpamAssassin 3.4.6 (2021-04-09) on pete.vger.email Precedence: bulk List-ID: <linux-kernel.vger.kernel.org> X-Mailing-List: linux-kernel@vger.kernel.org X-Greylist: Sender passed SPF test, not delayed by milter-greylist-4.6.4 (pete.vger.email [0.0.0.0]); Mon, 30 Oct 2023 08:59:33 -0700 (PDT) X-getmail-retrieved-from-mailbox: INBOX X-GMAIL-THRID: 1779658707289295182 X-GMAIL-MSGID: 1781196733734273281 |
Series |
[v3] nvme: update firmware version after commit
|
|
Commit Message
Daniel Wagner
Oct. 30, 2023, 4 p.m. UTC
The firmware version sysfs entry needs to be updated after a successfully
firmware activation.
nvme-cli stopped issuing an Identify Controller command to list the
current firmware information and relies on sysfs showing the current
firmware version.
Reported-by: Kenji Tomonaga <tkenbo@gmail.com>
Signed-off-by: Daniel Wagner <dwagner@suse.de>
---
Only compile tested. Asked for testing.
changes:
v3:
- use afi variable directly, no _to_cpu helper
- fix bit mask size
v2:
- use fw slot info instead issuing another identify controller command
- https://lore.kernel.org/linux-nvme/20231013163420.3097-1-dwagner@suse.de
v1:
- initial version
- https://lore.kernel.org/linux-nvme/20231013062623.6745-1-dwagner@suse.de/
drivers/nvme/host/core.c | 15 ++++++++++++++-
1 file changed, 14 insertions(+), 1 deletion(-)
Comments
Looks good:
Reviewed-by: Christoph Hellwig <hch@lst.de>
On Mon, Oct 30, 2023 at 05:00:44PM +0100, Daniel Wagner wrote: > The firmware version sysfs entry needs to be updated after a successfully > firmware activation. > > nvme-cli stopped issuing an Identify Controller command to list the > current firmware information and relies on sysfs showing the current > firmware version. > > Reported-by: Kenji Tomonaga <tkenbo@gmail.com> > Signed-off-by: Daniel Wagner <dwagner@suse.de> > --- > > Only compile tested. Asked for testing. > > changes: > > v3: > - use afi variable directly, no _to_cpu helper > - fix bit mask size > > v2: > - use fw slot info instead issuing another identify controller command > - https://lore.kernel.org/linux-nvme/20231013163420.3097-1-dwagner@suse.de > > v1: > - initial version > - https://lore.kernel.org/linux-nvme/20231013062623.6745-1-dwagner@suse.de/ > > > drivers/nvme/host/core.c | 15 ++++++++++++++- > 1 file changed, 14 insertions(+), 1 deletion(-) > > diff --git a/drivers/nvme/host/core.c b/drivers/nvme/host/core.c > index 37b6fa746662..e8511bff78d2 100644 > --- a/drivers/nvme/host/core.c > +++ b/drivers/nvme/host/core.c > @@ -4053,8 +4053,21 @@ static void nvme_get_fw_slot_info(struct nvme_ctrl *ctrl) > return; > > if (nvme_get_log(ctrl, NVME_NSID_ALL, NVME_LOG_FW_SLOT, 0, NVME_CSI_NVM, > - log, sizeof(*log), 0)) > + log, sizeof(*log), 0)) { > dev_warn(ctrl->device, "Get FW SLOT INFO log error\n"); > + goto out_free_log; > + } > + > + if (log->afi & 0x70) { > + dev_info(ctrl->device, > + "Firmware is activated after next Controller Level Reset\n"); > + goto out_free_log; > + } > + > + memcpy(ctrl->subsys->firmware_rev, &log->frs[log->afi & 0x7], > + sizeof(ctrl->subsys->firmware_rev)); > + > +out_free_log: > kfree(log); > } > > -- > 2.42.0 > Reviewed-by: Niklas Cassel <niklas.cassel@wdc.com>
On Mon, Oct 30, 2023 at 05:00:44PM +0100, Daniel Wagner wrote: > The firmware version sysfs entry needs to be updated after a successfully > firmware activation. > > nvme-cli stopped issuing an Identify Controller command to list the > current firmware information and relies on sysfs showing the current > firmware version. > > Reported-by: Kenji Tomonaga <tkenbo@gmail.com> > Signed-off-by: Daniel Wagner <dwagner@suse.de> Unfortunately, Kenji is not able to post on the mailing list, so here his tag: Tested-by: Kenji Tomonaga <tkenbo@gmail.com>
On Mon, Oct 30, 2023 at 05:00:44PM +0100, Daniel Wagner wrote: > The firmware version sysfs entry needs to be updated after a successfully > firmware activation. > > nvme-cli stopped issuing an Identify Controller command to list the > current firmware information and relies on sysfs showing the current > firmware version. > > Reported-by: Kenji Tomonaga <tkenbo@gmail.com> > Signed-off-by: Daniel Wagner <dwagner@suse.de> Thanks, applied for nvme-6.7.
On Tue, Oct 31, 2023 at 10:08:53AM -0600, Keith Busch wrote: > On Mon, Oct 30, 2023 at 05:00:44PM +0100, Daniel Wagner wrote: > > The firmware version sysfs entry needs to be updated after a successfully > > firmware activation. > > > > nvme-cli stopped issuing an Identify Controller command to list the > > current firmware information and relies on sysfs showing the current > > firmware version. > > > > Reported-by: Kenji Tomonaga <tkenbo@gmail.com> > > Signed-off-by: Daniel Wagner <dwagner@suse.de> > > Thanks, applied for nvme-6.7. I've got negative feedback from one of our customer. I've annotate the code with dev_info(ctrl->device, "afi: %#x\n", log->afi); for (i = 0; i < 7; i++) { dev_info(ctrl->device, "frs%d: %.*s\n", i + 1, nvme_strlen((char *)&log->frs[i], sizeof(ctrl->subsys->firmware_rev)), (char *)&log->frs[i]); } [ 124.824812] nvme nvme8: afi: 0x3 [ 124.824824] nvme nvme8: frs1: 0.4.0 [ 124.824828] nvme nvme8: frs2: 0.3.0 [ 124.824832] nvme nvme8: frs3: 0.4.0 [ 124.824835] nvme nvme8: frs4: [ 124.824837] nvme nvme8: frs5: [ 124.824840] nvme nvme8: frs6: [ 124.824842] nvme nvme8: frs7: This particular firmware seem to interpret afi one based, while the this patch assumes it is zero based memcpy(ctrl->subsys->firmware_rev, &log->frs[log->afi & 0x7], sizeof(ctrl->subsys->firmware_rev)); The spec says Active Firmware Info (AFI): Specifies information about the active firmware revision. Bit 7 is reserved. Bits 6:4 indicates the firmware slot that is going to be activated at the next Controller Level Reset. If this field is 0h, then the controller does not indicate the firmware slot that is going to be activated at the next Controller Level Reset. Bit 3 is reserved. Bits 2:0 indicates the firmware slot from which the actively running firmware revision was loaded. It's not clear to me if afi bits 2:0 is zero or one based. Bits 6:4 indicate to be 1 based. Any ideas how to handle this? Thanks, Daniel
On Fri, Nov 03, 2023 at 01:11:02PM +0100, Daniel Wagner wrote: > This particular firmware seem to interpret afi one based, while > the this patch assumes it is zero based > Active Firmware Info (AFI): Specifies information about the active > firmware revision. > > Bit 7 is reserved. > Bits 6:4 indicates the firmware slot that is going to be activated > at the next Controller Level Reset. If this field is 0h, > then the controller does not indicate the firmware slot that > is going to be activated at the next Controller Level Reset. > Bit 3 is reserved. > Bits 2:0 indicates the firmware slot from which the actively running > firmware revision was loaded. > > > It's not clear to me if afi bits 2:0 is zero or one based. Bits 6:4 > indicate to be 1 based. All 0's based (what a stupid term..) fields in NVMe are explicitly marked as such. And even if that wasn't the case I'd very much expect the same encoding for the two sub-fields.
On Fri, Nov 03, 2023 at 02:58:57PM +0100, Christoph Hellwig wrote: > On Fri, Nov 03, 2023 at 01:11:02PM +0100, Daniel Wagner wrote: > > This particular firmware seem to interpret afi one based, while > > the this patch assumes it is zero based > > > Active Firmware Info (AFI): Specifies information about the active > > firmware revision. > > > > Bit 7 is reserved. > > Bits 6:4 indicates the firmware slot that is going to be activated > > at the next Controller Level Reset. If this field is 0h, > > then the controller does not indicate the firmware slot that > > is going to be activated at the next Controller Level Reset. > > Bit 3 is reserved. > > Bits 2:0 indicates the firmware slot from which the actively running > > firmware revision was loaded. > > > > > > It's not clear to me if afi bits 2:0 is zero or one based. Bits 6:4 > > indicate to be 1 based. > > All 0's based (what a stupid term..) fields in NVMe are explicitly > marked as such. And even if that wasn't the case I'd very much > expect the same encoding for the two sub-fields. Yeah, it's just the firmware slot number, taken literally. AFI = 1 means slot 1, AFI = 2 means slot 2, etc... Slot 0 either has special meaning (firmware commit SF field, or fw log AFI bits 6:4), or is reserved value, like in Identify Controller FRMW.NOFS, and has no place in the FW Slot Info log page. Our first slot in the log page is defined as slot one, so we have to subtract one from the AFI field to index into the slot array. I messed up for not catching that earlier, but thanks for pointing it out now.
On Fri, Nov 03, 2023 at 08:22:11AM -0600, Keith Busch wrote: > > All 0's based (what a stupid term..) fields in NVMe are explicitly > > marked as such. And even if that wasn't the case I'd very much > > expect the same encoding for the two sub-fields. > > Yeah, it's just the firmware slot number, taken literally. AFI = 1 means > slot 1, AFI = 2 means slot 2, etc... Slot 0 either has special meaning > (firmware commit SF field, or fw log AFI bits 6:4), or is reserved > value, like in Identify Controller FRMW.NOFS, and has no place in the FW > Slot Info log page. > > Our first slot in the log page is defined as slot one, so we have to > subtract one from the AFI field to index into the slot array. I messed > up for not catching that earlier, but thanks for pointing it out now. Thanks for the clarification. Do you want me to send a follow up patch, a new version of this one or do you fix up yourself?
On Mon, Nov 06, 2023 at 08:00:44AM +0100, Daniel Wagner wrote: > On Fri, Nov 03, 2023 at 08:22:11AM -0600, Keith Busch wrote: > > > All 0's based (what a stupid term..) fields in NVMe are explicitly > > > marked as such. And even if that wasn't the case I'd very much > > > expect the same encoding for the two sub-fields. > > > > Yeah, it's just the firmware slot number, taken literally. AFI = 1 means > > slot 1, AFI = 2 means slot 2, etc... Slot 0 either has special meaning > > (firmware commit SF field, or fw log AFI bits 6:4), or is reserved > > value, like in Identify Controller FRMW.NOFS, and has no place in the FW > > Slot Info log page. > > > > Our first slot in the log page is defined as slot one, so we have to > > subtract one from the AFI field to index into the slot array. I messed > > up for not catching that earlier, but thanks for pointing it out now. > > Thanks for the clarification. > > Do you want me to send a follow up patch, a new version of this one or > do you fix up yourself? Fixed up inline when applying the original patch. Let me know if you have any concerns with the result, currently here: https://git.infradead.org/nvme.git/commitdiff/983a338b96c8a25b81e773b643f80634358e81bc
On Mon, Nov 06, 2023 at 09:44:39AM -0700, Keith Busch wrote: > Fixed up inline when applying the original patch. Let me know if you > have any concerns with the result, currently here: > > https://git.infradead.org/nvme.git/commitdiff/983a338b96c8a25b81e773b643f80634358e81bc Looks good. Thanks!
diff --git a/drivers/nvme/host/core.c b/drivers/nvme/host/core.c index 37b6fa746662..e8511bff78d2 100644 --- a/drivers/nvme/host/core.c +++ b/drivers/nvme/host/core.c @@ -4053,8 +4053,21 @@ static void nvme_get_fw_slot_info(struct nvme_ctrl *ctrl) return; if (nvme_get_log(ctrl, NVME_NSID_ALL, NVME_LOG_FW_SLOT, 0, NVME_CSI_NVM, - log, sizeof(*log), 0)) + log, sizeof(*log), 0)) { dev_warn(ctrl->device, "Get FW SLOT INFO log error\n"); + goto out_free_log; + } + + if (log->afi & 0x70) { + dev_info(ctrl->device, + "Firmware is activated after next Controller Level Reset\n"); + goto out_free_log; + } + + memcpy(ctrl->subsys->firmware_rev, &log->frs[log->afi & 0x7], + sizeof(ctrl->subsys->firmware_rev)); + +out_free_log: kfree(log); }