Received-SPF: pass (google.com: best guess record for domain of linux-kernel-owner@vger.kernel.org designates 209.132.180.67 as permitted sender) client-ip=209.132.180.67;
IronPort-SDR: pS0wKdQTYnjXVSJSz5tRuOZYmLi87KjTpGN2jnf0kh83PkAazfKV8ppha/c7CFDrkK3aWy//eN
 YscEPnxV1j94pkF+ilNwqEKFwi8T+I7n4BJ9PdDkWuvMPWPKgnSVCxG6bfDCJr7krkpP2troCU
 l31BI98oTSHUTILA5YuX4xv8dAMTscYtkLwVw0BZS4/SfgUroZvPlrJGmpsmI79BbF/9u3g2Ic
 KSwLKXbLt1sx2c2NOBYHNxbo1CbVL0VKVx2bWAl8nfar5nQdx7/u/q3Ytef7yTWOCE+pezFPs3
 +o8=
From:   Chaitanya Kulkarni <Chaitanya.Kulkarni@wdc.com>
To:     Keith Busch <kbusch@kernel.org>, Christoph Hellwig <hch@lst.de>,
        "linux-nvme@lists.infradead.org" <linux-nvme@lists.infradead.org>,
        Sagi Grimberg <sagi@grimberg.me>
CC:     Hannes Reinecke <hare@suse.com>,
        "Martin K . Petersen" <martin.petersen@oracle.com>,
        "linux-kernel@vger.kernel.org" <linux-kernel@vger.kernel.org>,
        Jens Axboe <axboe@fb.com>,
        Logan Gunthorpe <logang@deltatee.com>
Subject: Re: [PATCH] nvme: Restore device naming sanity
Thread-Topic: [PATCH] nvme: Restore device naming sanity
Thread-Index: AQHVY0b0r2H6Juc7x0CaNQILnZjrrw==
Date:   Sat, 7 Sep 2019 18:42:49 +0000
Message-ID: <BYAPR04MB5749E4FBF82F23C41A494A6286B50@BYAPR04MB5749.namprd04.prod.outlook.com>
References: <20190904173159.22921-1-kbusch@kernel.org>
Accept-Language: en-US
Content-Language: en-US
wdcipoutbound: EOP-TRUE
Content-Type: text/plain; charset="us-ascii"
Content-Transfer-Encoding: quoted-printable
MIME-Version: 1.0
X-MS-Exchange-CrossTenant-Network-Message-Id: 3fe34546-8bb1-4c24-b256-08d733c32eca
X-MS-Exchange-CrossTenant-originalarrivaltime: 07 Sep 2019 18:42:49.1750
 (UTC)
X-MS-Exchange-CrossTenant-fromentityheader: Hosted
X-MS-Exchange-CrossTenant-id: b61c8803-16f3-4c35-9b17-6f65f441df86
X-MS-Exchange-CrossTenant-mailboxtype: HOSTED
X-MS-Exchange-CrossTenant-userprincipalname: CBZ9xwPG28MPPkc3EudRG5Jt444ZwPfTYevXGBvKEXGBaRsyG7a22aALKNczLh3ECpeAd87KOACUM/KH67VoGOYVhbD1iVjAMcxLY21Q3G4=
X-MS-Exchange-Transport-CrossTenantHeadersStamped: BYAPR04MB5400
Sender: linux-kernel-owner@vger.kernel.org
Precedence: bulk
List-ID: <linux-kernel.vger.kernel.org>
X-Mailing-List: linux-kernel@vger.kernel.org

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