[PATCH v2] nvme: avoid "(efault)" from nvme_sysfs_show_subsysnqn()

[Keith Busch]

On Wed, Feb 03, 2021 at 01:31:13PM +0100, Martin Wilck wrote:
> On Tue, 2021-02-02 at 08:58 +0100, Christoph Hellwig wrote:
> > On Tue, Feb 02, 2021 at 01:59:51AM +0100, mwilck at suse.com wrote:
> > > From: Martin Wilck <mwilck at suse.com>
> > > 
> > > While a controller is still connecting, ctrl->subsys is NULL and
> > > thus reading the controller's "subsysnqn" sysfs attribute returns
> > > "(efault)". This happens all the time when user space processes
> > > "add" events for NVMe controller devices.
> > > 
> > > Fix it by returning the nvmf_ctrl_options' subsysnqn attribute
> > > if ctrl->subsys isn't initialized yet.
> > 
> > This is just papering over symptoms.  We should not be sending
> > events or display sysfs files until the controlle is live.
> 
> I don't follow. What's wrong with a "connecting" controller device in
> sysfs? 
> 
> Controllers can loose connectivity any time. What's the difference
> between a controller that is reconnecting after a connection loss and a
> freshly created controller that's just not live yet? You wouldn't
> delete the former from sysfs, I suppose.
> 
> From the user space point of view, it's the same, a controller device
> that is not in usable state. I don't see a general problem with that.
> Seeing a controller appear in connecting state might actually be useful
> for user space.
> 
> The subsysnqn attribute is correct for controllers that were once
> connected and lost connectivity. My patch was only intended to fix the
> "just created and never connected" case, where it is not, nothing more.

I think Christoph is suggesting just wait until we have all the
properties the driver exports rather than specifically exporting only
during a 'live' state. Something like this (untested) should do the
trick:

---
diff --git a/drivers/nvme/host/core.c b/drivers/nvme/host/core.c
index 1a3cdc6b1036..4c0258d7eb91 100644
--- a/drivers/nvme/host/core.c
+++ b/drivers/nvme/host/core.c
@@ -3069,6 +3069,9 @@ int nvme_init_identify(struct nvme_ctrl *ctrl)
 		if (ret)
 			goto out_free;
 
+		ret = cdev_device_add(&ctrl->cdev, ctrl->device);
+		if (ret)
+			goto out_free;
 		/*
 		 * Check for quirks.  Quirk can depend on firmware version,
 		 * so, in principle, the set of quirks present can change
@@ -4545,9 +4548,6 @@ int nvme_init_ctrl(struct nvme_ctrl *ctrl, struct device *dev,
 	nvme_get_ctrl(ctrl);
 	cdev_init(&ctrl->cdev, &nvme_dev_fops);
 	ctrl->cdev.owner = ops->module;
-	ret = cdev_device_add(&ctrl->cdev, ctrl->device);
-	if (ret)
-		goto out_free_name;
 
 	/*
 	 * Initialize latency tolerance controls.  The sysfs files won't
@@ -4560,9 +4560,6 @@ int nvme_init_ctrl(struct nvme_ctrl *ctrl, struct device *dev,
 	nvme_fault_inject_init(&ctrl->fault_inject, dev_name(ctrl->device));
 
 	return 0;
-out_free_name:
-	nvme_put_ctrl(ctrl);
-	kfree_const(ctrl->device->kobj.name);
 out_release_instance:
 	ida_simple_remove(&nvme_instance_ida, ctrl->instance);
 out:
--