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

[Belanger, Martin]

> On Thu, Feb 03, 2022 at 04:17:48PM -0500, Martin Belanger wrote:
> > From: Martin Belanger <martin.belanger at 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 at 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).

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.

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.

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.

Regards,
Martin

> 
> thanks,
> 
> greg k-h