Re: [PATCHv2 3/3] nvme: Expose cntrltype and dctype through sysfs

[Hannes Reinecke <hare@suse.de>]

On 2/6/22 09:54, Sagi Grimberg wrote:
> Hey Greg,
> 
>>> diff --git a/drivers/nvme/host/core.c b/drivers/nvme/host/core.c
>>> index b5e452aa3c0e..4e3db5ec3924 100644
>>> --- a/drivers/nvme/host/core.c
>>> +++ b/drivers/nvme/host/core.c
>>> @@ -2990,6 +2990,9 @@ static int nvme_init_identify(struct nvme_ctrl 
>>> *ctrl)
>>>       ctrl->max_namespaces = le32_to_cpu(id->mnan);
>>>       ctrl->ctratt = le32_to_cpu(id->ctratt);
>>> +    ctrl->cntrltype = id->cntrltype;
>>> +    ctrl->dctype = id->dctype;
>>> +
>>>       if (id->rtd3e) {
>>>           /* us -> s */
>>>           u32 transition_time = le32_to_cpu(id->rtd3e) / USEC_PER_SEC;
>>> @@ -3115,6 +3118,10 @@ int nvme_init_ctrl_finish(struct nvme_ctrl *ctrl)
>>>               return ret;
>>>       }
>>> +    ret = device_update_groups(ctrl->device);
>>> +    if (ret)
>>> +        return ret;
>>
>> Why is this needed here?  How did the class or type just change?  That
>> should never change over the lifespan of a device.  If it does, you need
>> to tear down the old device and create a new one as something really
>> wrong just happened.
> 
> The class and type do not change. The fundamental need here is to update
> visible attributes of the device.
> 
> In nvme, we create the device node with its sysfs attributes and next
> comes a process where we interrogate it for capabilities/personality.
> In this example, the attributes cntrltype and dctype are learned from
> the device identification, and the desire is that dctype will only be
> visible for specific cntrltype (i.e. cntrltype is I/O controllers vs.
> discovery controllers and dctype is the discovery controller type).
> 
> So perhaps a more narrowed interface to update attributes
> visibility of the controller would be more appropriate instead?
> 
> Something like:
> -- 
>      ret = device_update_visibility(ctrl->device->groups);
> -- 

I guess it can be even simpler; to my reading the prime objection is 
that we change attributes (and attribute visibility) without informing 
userspace that we did so, thus udev and the device configuration driven 
by uevents won't work as expected.

Which means that everything should be okay if we issue an KOBJ_CHANGE 
uevent after we modify the visibility of attributes, no?

Cheers,

Hannes
-- 
Dr. Hannes Reinecke                Kernel Storage Architect
hare@suse.de                              +49 911 74053 688
SUSE Software Solutions GmbH, Maxfeldstr. 5, 90409 Nürnberg
HRB 36809 (AG Nürnberg), Geschäftsführer: Felix Imendörffer