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

[Greg KH <gregkh@linuxfoundation.org>]

On Sun, Feb 06, 2022 at 10:54:54AM +0200, 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.

Then change the attributes and issue a uevent of KOBJECT_CHANGE like
other objects do today.  But be aware that userspace might have problems
with this.  Please test your tools to verify this works or not.

> In nvme, we create the device node with its sysfs attributes and next
> comes a process where we interrogate it for capabilities/personality.

That feels wrong, just delay, there's no rush.

> 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);
> --

Again, suffers from the same issue where userspace is not notified.

Again, just delay and create the real object when you know what you
have.

thanks,

greg k-h