[PATCH] nvme: copy firmware_rev on each init

[PATCH] nvme: copy firmware_rev on each init

[Keith Busch]

From: Keith Busch <kbusch@kernel.org>

The firmware revision can change on after a reset so copy the most
recent info each time instead of just the first time, otherwise the
sysfs firmware_rev entry may contain stale data.

Reported-by: Jeff Lien <jeff.lien@wdc.com>
Signed-off-by: Keith Busch <kbusch@kernel.org>
---
 drivers/nvme/host/core.c | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/drivers/nvme/host/core.c b/drivers/nvme/host/core.c
index ea6694fd550f..e56ecc7fda2d 100644
--- a/drivers/nvme/host/core.c
+++ b/drivers/nvme/host/core.c
@@ -2893,7 +2893,6 @@ static int nvme_init_subsystem(struct nvme_ctrl *ctrl, struct nvme_id_ctrl *id)
 	nvme_init_subnqn(subsys, ctrl, id);
 	memcpy(subsys->serial, id->sn, sizeof(subsys->serial));
 	memcpy(subsys->model, id->mn, sizeof(subsys->model));
-	memcpy(subsys->firmware_rev, id->fr, sizeof(subsys->firmware_rev));
 	subsys->vendor_id = le16_to_cpu(id->vid);
 	subsys->cmic = id->cmic;
 
@@ -3112,6 +3111,8 @@ static int nvme_init_identify(struct nvme_ctrl *ctrl)
 				ctrl->quirks |= core_quirks[i].quirks;
 		}
 	}
+	memcpy(ctrl->subsys->firmware_rev, id->fr,
+	       sizeof(ctrl->subsys->firmware_rev));
 
 	if (force_apst && (ctrl->quirks & NVME_QUIRK_NO_DEEPEST_PS)) {
 		dev_warn(ctrl->device, "forcibly allowing all power states due to nvme_core.force_apst -- use at your own risk\n");
-- 
2.30.2

Re: [PATCH] nvme: copy firmware_rev on each init

[Chaitanya Kulkarni]

On 9/19/22 12:45, Keith Busch wrote:
> From: Keith Busch <kbusch@kernel.org>
> 
> The firmware revision can change on after a reset so copy the most
> recent info each time instead of just the first time, otherwise the
> sysfs firmware_rev entry may contain stale data.
> 
> Reported-by: Jeff Lien <jeff.lien@wdc.com>
> Signed-off-by: Keith Busch <kbusch@kernel.org>
>


Looks good.

Reviewed-by: Chaitanya Kulkarni <kch@nvidia.com>

-ck

Re: [PATCH] nvme: copy firmware_rev on each init

[Sagi Grimberg]

Reviewed-by: Sagi Grimberg <sagi@grimberg.me>

Re: [PATCH] nvme: copy firmware_rev on each init

[Chao Leng]

On 2022/9/20 3:45, Keith Busch wrote:
> From: Keith Busch <kbusch@kernel.org>
> 
> The firmware revision can change on after a reset so copy the most
> recent info each time instead of just the first time, otherwise the
> sysfs firmware_rev entry may contain stale data.
> 
> Reported-by: Jeff Lien <jeff.lien@wdc.com>
> Signed-off-by: Keith Busch <kbusch@kernel.org>
> ---
>   drivers/nvme/host/core.c | 3 ++-
>   1 file changed, 2 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/nvme/host/core.c b/drivers/nvme/host/core.c
> index ea6694fd550f..e56ecc7fda2d 100644
> --- a/drivers/nvme/host/core.c
> +++ b/drivers/nvme/host/core.c
> @@ -2893,7 +2893,6 @@ static int nvme_init_subsystem(struct nvme_ctrl *ctrl, struct nvme_id_ctrl *id)
>   	nvme_init_subnqn(subsys, ctrl, id);
>   	memcpy(subsys->serial, id->sn, sizeof(subsys->serial));
>   	memcpy(subsys->model, id->mn, sizeof(subsys->model));
> -	memcpy(subsys->firmware_rev, id->fr, sizeof(subsys->firmware_rev));
>   	subsys->vendor_id = le16_to_cpu(id->vid);
>   	subsys->cmic = id->cmic;
>   
> @@ -3112,6 +3111,8 @@ static int nvme_init_identify(struct nvme_ctrl *ctrl)
>   				ctrl->quirks |= core_quirks[i].quirks;
>   		}
>   	}
> +	memcpy(ctrl->subsys->firmware_rev, id->fr,
> +	       sizeof(ctrl->subsys->firmware_rev));
Perhaps it would be better to place it near the call to nvme_init_subsystem.
>   
>   	if (force_apst && (ctrl->quirks & NVME_QUIRK_NO_DEEPEST_PS)) {
>   		dev_warn(ctrl->device, "forcibly allowing all power states due to nvme_core.force_apst -- use at your own risk\n");
> 
Anyway, looks good to me.

Reviewed-by: Chao Leng <lengchao@huawei.com>