[PATCH v2] nvme: update firmware version after commit

[PATCH v2] nvme: update firmware version after commit

[Daniel Wagner]

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. So you might want to wait until I am back from my vacation.

 drivers/nvme/host/core.c | 17 ++++++++++++++++-
 1 file changed, 16 insertions(+), 1 deletion(-)

diff --git a/drivers/nvme/host/core.c b/drivers/nvme/host/core.c
index 21783aa2ee8e..686478555585 100644
--- a/drivers/nvme/host/core.c
+++ b/drivers/nvme/host/core.c
@@ -4067,14 +4067,29 @@ static bool nvme_ctrl_pp_status(struct nvme_ctrl *ctrl)
 static void nvme_get_fw_slot_info(struct nvme_ctrl *ctrl)
 {
 	struct nvme_fw_slot_info_log *log;
+	u64 afi;
 
 	log = kmalloc(sizeof(*log), GFP_KERNEL);
 	if (!log)
 		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;
+	}
+
+	afi = le64_to_cpu(log->afi);
+	if (afi & 0x30) {
+		dev_info(ctrl->device,
+			 "Firmware is activated after next Controller Level Reset\n");
+		goto out_free_log;
+	}
+
+	memcpy(ctrl->subsys->firmware_rev, &log->frs[afi & 0x3],
+		sizeof(ctrl->subsys->firmware_rev));
+
+out_free_log:
 	kfree(log);
 }
 
-- 
2.42.0

Re: [PATCH v2] nvme: update firmware version after commit

[Keith Busch]

On Fri, Oct 13, 2023 at 06:34:20PM +0200, Daniel Wagner wrote:
>  	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;
> +	}
> +
> +	afi = le64_to_cpu(log->afi);
> +	if (afi & 0x30) {

That should be '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[afi & 0x3],

and 'afi & 0x7'.

Re: [PATCH v2] nvme: update firmware version after commit

[Niklas Cassel]

On Fri, Oct 13, 2023 at 10:43:48AM -0600, Keith Busch wrote:
> On Fri, Oct 13, 2023 at 06:34:20PM +0200, Daniel Wagner wrote:
> >  	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;
> > +	}
> > +
> > +	afi = le64_to_cpu(log->afi);
> > +	if (afi & 0x30) {
> 
> That should be 'afi & 0x70'.

Personally, I like GENMASK().
In this specific case it would be GENMASK_ULL(6, 4);

I find that more readable than 0x70.
Although for some reason GENMASK()/GENMASK_ULL() does not
seem to be used in drivers/nvme/


Kind regards,
Niklas

Re: [PATCH v2] nvme: update firmware version after commit

[Niklas Cassel]

On Fri, Oct 13, 2023 at 10:43:48AM -0600, Keith Busch wrote:
> On Fri, Oct 13, 2023 at 06:34:20PM +0200, Daniel Wagner wrote:
> >  	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;
> > +	}
> > +
> > +	afi = le64_to_cpu(log->afi);

BTW, this field is a single byte, so there should be no need to
use *_to_cpu() on this. (Byte order is only important when the
field is more than one byte.)


Kind regards,
Niklas

Re: [PATCH v2] nvme: update firmware version after commit

[Chaitanya Kulkarni]

On 10/13/23 09:34, 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.
>
>

why did nvme-cli stopped using id-ctrl ?

-ck

Re: [PATCH v2] nvme: update firmware version after commit

[Keith Busch]

On Tue, Oct 24, 2023 at 06:21:45AM +0000, Chaitanya Kulkarni wrote:
> On 10/13/23 09:34, 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.
> >
> >
> 
> why did nvme-cli stopped using id-ctrl ?

We have exported attributes. We should be able to use them so that we're
not interrupting the device to provide info that the driver already
caches. The driver just needs to make sure the contents are reliable.

Re: [PATCH v2] nvme: update firmware version after commit

[Daniel Wagner]

On Fri, Oct 13, 2023 at 08:37:32PM +0000, Niklas Cassel wrote:
> On Fri, Oct 13, 2023 at 10:43:48AM -0600, Keith Busch wrote:
> > On Fri, Oct 13, 2023 at 06:34:20PM +0200, Daniel Wagner wrote:
> > >  	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;
> > > +	}
> > > +
> > > +	afi = le64_to_cpu(log->afi);
> 
> BTW, this field is a single byte, so there should be no need to
> use *_to_cpu() on this. (Byte order is only important when the
> field is more than one byte.)

Indeed, I somehow thought afi is of type __le64.

Re: [PATCH v2] nvme: update firmware version after commit

[Daniel Wagner]

On Fri, Oct 13, 2023 at 10:43:48AM -0600, Keith Busch wrote:
> On Fri, Oct 13, 2023 at 06:34:20PM +0200, Daniel Wagner wrote:
> >  	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;
> > +	}
> > +
> > +	afi = le64_to_cpu(log->afi);
> > +	if (afi & 0x30) {
> 
> That should be 'afi & 0x70'.

I've update the patch accordingly and send Kenij and one of our customer
to test it.