Re: [PATCH 1/1] nvmet: allow setting model_number once

[Max Gurtovoy <mgurtovoy@nvidia.com>]

On 2/24/2021 11:43 AM, Christoph Hellwig wrote:
> So looking at this and Chaitanya patch I think this version simplifies
> things quite nicely, and it also happens to get rid of the RCU annotation
> sparse warnings.
>
> Let me know what you think of the incremental cleanup and micro-opimization
> below, though:
>
>
> diff --git a/drivers/nvme/target/admin-cmd.c b/drivers/nvme/target/admin-cmd.c
> index 3da285e22e9209..2ca13dd7e7b88a 100644
> --- a/drivers/nvme/target/admin-cmd.c
> +++ b/drivers/nvme/target/admin-cmd.c
> @@ -313,34 +313,40 @@ static void nvmet_execute_get_log_page(struct nvmet_req *req)
>   	nvmet_req_complete(req, NVME_SC_INVALID_FIELD | NVME_SC_DNR);
>   }
>   
> -static int nvmet_id_set_model_number(struct nvme_id_ctrl *id,
> -				      struct nvmet_subsys *subsys)
> +static u16 nvmet_set_model_number(struct nvme_id_ctrl *id,
> +		struct nvmet_subsys *subsys)
>   {
> -	int ret = 0;
> +	u16 status = 0;
>   
>   	mutex_lock(&subsys->lock);
>   	if (!subsys->model_number) {
> -		subsys->model_number = kstrdup(NVMET_DEFAULT_CTRL_MODEL,
> -					       GFP_KERNEL);
> -		if (!subsys->model_number) {
> -			ret = -ENOMEM;
> -			goto out_unlock;
> -		}
> +		subsys->model_number =
> +			kstrdup(NVMET_DEFAULT_CTRL_MODEL, GFP_KERNEL);
> +		if (!subsys->model_number)
> +			status = NVME_SC_INTERNAL;
>   	}
> -	memcpy_and_pad(id->mn, sizeof(id->mn), subsys->model_number,
> -		       strlen(subsys->model_number), ' ');
> -out_unlock:
>   	mutex_unlock(&subsys->lock);
> -	return ret;
> +
> +	return status;
>   }
>   
>   static void nvmet_execute_identify_ctrl(struct nvmet_req *req)
>   {
>   	struct nvmet_ctrl *ctrl = req->sq->ctrl;
> +	struct nvmet_subsys *subsys = ctrl->subsys;
>   	struct nvme_id_ctrl *id;
>   	u32 cmd_capsule_size;
>   	u16 status = 0;
> -	int ret;
> +
> +	/*
> +	 * If there is no model number yet, so it now.  It will then remain
> +	 * stable for the life time of the subsystem.
> +	 */
> +	if (!subsys->model_number) {
> +		status = nvmet_set_model_number(id, subsys);

you don't need the id argument here.

Otherwise looks good and it can be squashed to my patch.



_______________________________________________
Linux-nvme mailing list
Linux-nvme@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-nvme