Re: [PATCH] nvme: avoid bogus CRTO values

[Niklas Cassel <Niklas.Cassel@wdc.com>]

On Wed, Sep 13, 2023 at 09:46:08AM -0700, Keith Busch wrote:
>  
> > I guess that I just can't understand how a controller can set
> > bit CAP.CRMS.CRWMS to 1, without setting a value in CRTO.CRWMT.
> > Is it such a silly spec violation, that they seem to have not
> > bothered to read what this bit means at all.
> 
> Yep, but broken controllers are broken. They used to work in Linux
> before the driver started preferring the recommended CRTO, so we gotta
> fix 'em.

I'm not saying that these controllers shouldn't work with Linux.
However, these controller used to work with CC.CRIME == 0, so perhaps
we should continue to use them that way?


My reasoning is that if the controller did not even manage to get the
most fundamental part of this new feature correctly (i.e. defining the
actual timeout values for this feature), then perhaps we shouldn't
assume that the rest of the feature, setting the NRDY bit, sending the
AER, etc., is implemented correctly either.

When CC.CRIME == 1, both fields in CRTO actually have a defined meaning:
CRTO.CRIMT is the maximum time that host software should wait for the
controller to become ready.
CRTO.CRWMT is the maximum time that host software should wait for all
attached namespaces to become ready.

So having both fields defined to zero, or rather, to have both fields
defined to a value smaller than CAP.TO, regardless of CC.CRIME value,
is quite bad.

So perhaps it is better to keep CC.CRIME == 0 for such controllers.


> If we have a way to sanity check for spec non-compliance, I would prefer
> doing that generically rather than quirk specific devices.

It's not going to be beautiful, but one way could be to:
-check CAP.CRMS.CRIMS, if it is set to 1:
-write CC.CRIME == 1,
-re-read CAP register, since it can change depending on CC.CRIME (urgh)
-check if CRTO.CRIMT is less than CAP.TO, if so:
-write CC.CRIME == 0 (disable the feature since it is obviously broken)
-re-read CAP register, since it can change depending on CC.CRIME (urgh)


For the record, I'm obviously happy with whatever decision you make,
I'm just giving my two cents...


Kind regards,
Niklas