Re: [PATCH] nvme: Restore device naming sanity

[Chaitanya Kulkarni <Chaitanya.Kulkarni@wdc.com>]

Apart from the some nits, this looks okay.

Reviewed-by: Chaitanya Kulkarni <chaitanya.kulkarni@wdc.com>

May be subject line should be 's/nvme/nvme-core/' ?

On 09/04/2019 10:34 AM, Keith Busch wrote:
> The namespace names must be unique for the lifetime of the subsystem.
> This was accomplished by using their parent subsystems' instances which
> was independent of the controllers connected to that subsystem.
>
> The consequence of that naming scheme meant that name prefixes given to
> namespaces may match a controller from an unrelated subsystem. This has
> understandbly invited confusion when examining device nodes.
's/understandbly/understandably/'
>
> Ensure the namespace's subsystem instance never clashes with a
> controller instance of another subsystem by transferring the instance
> ownership to parent subsystem from the first controller discovered in
> that subsystem.
>
> Reviewed-by: Logan Gunthorpe <logang@deltatee.com>
> Signed-off-by: Keith Busch <kbusch@kernel.org>
> ---
>   drivers/nvme/host/core.c | 21 ++++++++++-----------
>   1 file changed, 10 insertions(+), 11 deletions(-)
>
> diff --git a/drivers/nvme/host/core.c b/drivers/nvme/host/core.c
> index 14c0bfb55615..8a8279ece5ee 100644
> --- a/drivers/nvme/host/core.c
> +++ b/drivers/nvme/host/core.c
> @@ -81,7 +81,6 @@ EXPORT_SYMBOL_GPL(nvme_reset_wq);
>   struct workqueue_struct *nvme_delete_wq;
>   EXPORT_SYMBOL_GPL(nvme_delete_wq);
>
> -static DEFINE_IDA(nvme_subsystems_ida);
>   static LIST_HEAD(nvme_subsystems);
>   static DEFINE_MUTEX(nvme_subsystems_lock);
>
> @@ -2344,7 +2343,8 @@ static void nvme_release_subsystem(struct device *dev)
>   	struct nvme_subsystem *subsys =
>   		container_of(dev, struct nvme_subsystem, dev);
>
> -	ida_simple_remove(&nvme_subsystems_ida, subsys->instance);
> +	if (subsys->instance >= 0)
> +		ida_simple_remove(&nvme_instance_ida, subsys->instance);
>   	kfree(subsys);
>   }
>
> @@ -2473,12 +2473,8 @@ static int nvme_init_subsystem(struct nvme_ctrl *ctrl, struct nvme_id_ctrl *id)
>   	subsys = kzalloc(sizeof(*subsys), GFP_KERNEL);
>   	if (!subsys)
>   		return -ENOMEM;
> -	ret = ida_simple_get(&nvme_subsystems_ida, 0, 0, GFP_KERNEL);
> -	if (ret < 0) {
> -		kfree(subsys);
> -		return ret;
> -	}
> -	subsys->instance = ret;
> +
> +	subsys->instance = -1;
>   	mutex_init(&subsys->lock);
>   	kref_init(&subsys->ref);
>   	INIT_LIST_HEAD(&subsys->ctrls);
> @@ -2497,7 +2493,7 @@ static int nvme_init_subsystem(struct nvme_ctrl *ctrl, struct nvme_id_ctrl *id)
>   	subsys->dev.class = nvme_subsys_class;
>   	subsys->dev.release = nvme_release_subsystem;
>   	subsys->dev.groups = nvme_subsys_attrs_groups;
> -	dev_set_name(&subsys->dev, "nvme-subsys%d", subsys->instance);
> +	dev_set_name(&subsys->dev, "nvme-subsys%d", ctrl->instance);
>   	device_initialize(&subsys->dev);
>
>   	mutex_lock(&nvme_subsystems_lock);
> @@ -2528,6 +2524,8 @@ static int nvme_init_subsystem(struct nvme_ctrl *ctrl, struct nvme_id_ctrl *id)
>   		goto out_put_subsystem;
>   	}
>
> +	if (!found)
> +		subsys->instance = ctrl->instance;
>   	ctrl->subsys = subsys;
>   	list_add_tail(&ctrl->subsys_entry, &subsys->ctrls);
>   	mutex_unlock(&nvme_subsystems_lock);
> @@ -3803,7 +3801,9 @@ static void nvme_free_ctrl(struct device *dev)
>   		container_of(dev, struct nvme_ctrl, ctrl_device);
>   	struct nvme_subsystem *subsys = ctrl->subsys;
>
> -	ida_simple_remove(&nvme_instance_ida, ctrl->instance);
> +	if (subsys && ctrl->instance != subsys->instance)
> +		ida_simple_remove(&nvme_instance_ida, ctrl->instance);
> +
>   	kfree(ctrl->effects);
>   	nvme_mpath_uninit(ctrl);
>   	__free_page(ctrl->discard_page);
> @@ -4085,7 +4085,6 @@ static int __init nvme_core_init(void)
>
>   static void __exit nvme_core_exit(void)
>   {
> -	ida_destroy(&nvme_subsystems_ida);
>   	class_destroy(nvme_subsys_class);
>   	class_destroy(nvme_class);
>   	unregister_chrdev_region(nvme_chr_devt, NVME_MINORS);
>