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

[Greg KH <gregkh@linuxfoundation.org>]

On Sun, Feb 06, 2022 at 02:08:32PM +0100, Hannes Reinecke wrote:
> 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?

Maybe.  It depends on what you want userspace to do here.  Most
KOBJ_CHANGE uevents are not watched by the normal userspace rules as
they are used to only watching for add/remove events.  change events are
much more rare and device-specific.

So I would strongly recommend you not use them unless you know your
userspace tools can handle them and will do the right thing.

thanks,

greg k-h