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

[Sagi Grimberg]

On 2/3/21 8:52 AM, Keith Busch wrote:
> 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;

This happens on every reset, we should probably just do this once?