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

[Greg KH <gregkh@linuxfoundation.org>]

On Fri, Feb 04, 2022 at 01:51:27PM +0000, Belanger, Martin wrote:
> > On Thu, Feb 03, 2022 at 04:17:48PM -0500, Martin Belanger wrote:
> > > From: Martin Belanger <martin.belanger@dell.com>
> > >
> > > TP8010 introduces the Discovery Controller Type attribute (dctype).
> > > The dctype is returned in the response to the Identify command. This
> > > patch exposes the dctype through the sysfs. Since the dctype depends
> > > on the Controller Type (cntrltype), another attribute of the Identify
> > > response, the patch also exposes the cntrltype as well. The dctype
> > > will only be displayed for discovery controllers.
> > >
> > > Signed-off-by: Martin Belanger <martin.belanger@dell.com>
> > > ---
> > >  drivers/nvme/host/core.c | 45
> > > ++++++++++++++++++++++++++++++++++++++++
> > >  drivers/nvme/host/nvme.h |  3 +++
> > >  include/linux/nvme.h     | 10 ++++++++-
> > >  3 files changed, 57 insertions(+), 1 deletion(-)
> > >
> > > 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.
> 
> Hi Greg,
> I need to expose 2 nvme attributes through the sysfs. There is the controller type (cntrltype), which can take the values "io", "discovery", "admin", and "reserved". And there is the Discovery Controller Type (dctype), which takes the values "none", "ddc", "cdc", and "reserved". Note that the dctype is only meaningful for Discovery Controllers (i.e. cntrltype=discovery).

/me handes you some '\n' characters...

That's fine, this should not have anything to do with the groups
associated with a device as this type is known at creation, right?

> In my first iteration of this patch, I plainly exposed both attributes for all types of controllers. However, Sagi asked me to only expose the dctype if we're dealing with a Discovery Controller. Unfortunately, both cntrltype and dctype are not known at object creation. This means that when the is_visible method (i.e. nvme_dev_attrs_are_visible) is called, it is still not known whether dctype should be exposed. The value s for cntrltype and dctype are only known after a connection has been established with the controller and we have completed the Identify command.

How can this not be known at creation?  Why not figure it out first
before you create the device?  Why not wait?

> Hannes suggested that we combine both attributes into a single one that would take the values: "io", "discovery-none", "discovery-ddc", "discovery-cdc", "discovery-reserved", and "admin". But Sagi did not like this proposal and instead proposed the changes that you see here. Basically, we want to be able to re-execute the is_visible() method after we have identified the controller so that we can determine whether the dctype should be exposed.

Nope, that's not how the driver model works and you break all userspace
tools by doing this as it only scans the list of attributes when the
device is created and announced to userspace.

By changing the files and types and such afterward no one ever notices
and so userspace is "blind" to the change unless you want it to
constantly walk all of sysfs all the time to find this out (hint, you
don't want that...)

So there's a reason you are being forced to add a driver-core change
like this, because it's not something you should do :)

> So now I'm in a bit of a pickle. Sagi suggested this current patch set, but from what you're saying this is not a good approach. If you could suggest a solution that would satisfy all parties, I would greatly appreciate it.

Wait to create the device when you know the device type.

thanks,

greg k-h