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

[Greg KH <gregkh@linuxfoundation.org>]

On Sun, Feb 06, 2022 at 11:09:40AM +0200, Sagi Grimberg wrote:
> 
> > > 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.
> 
> Just to note that the same fundamental issue exists in both approaches,
> having the "combined" string change after the controller is identified
> is equivalent to having the separated attribute appear after the device
> is initialized.
> 
> > 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...)
> 
> The idea was to add a separate udev announcement that would tell
> userspace that these attributes are stable (it actually serves
> a different purpose, but it does come after that point).

like KOBJECT_CHANGE is there for?

> > 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 :)
> 
> I do agree that its a bit backwards to do this update. But the device
> even today is not fully usable when it is created, so we know no
> userspace program relies on the creation announcement.
> 
> > > 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.
> 
> The initial proposal was to defer the device creation to after the
> device is fully initialized. That though is a heavier lift because the
> device initialization is something that happens on every reset or
> reconnection after connectivity loss,

That's fine, what's wrong with tearing it all down and bringing it back
up then?  That's what the hardware just did, don't fake things out by
not conveying the same stuff to userspace.

> so if we move the device creation there we have to now check if this
> is the first time we are doing this or not, and also error recovery
> becomes more opinionated...

I still think you should not create anything until you know what it is.

thanks,

greg k-h