Re: [PATCH] nvme: avoid bogus CRTO values

[Keith Busch <kbusch@kernel.org>]

On Fri, Sep 15, 2023 at 09:31:44PM +0000, Niklas Cassel wrote:
> 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?

Perhaps I missed something, but I didn't get any indication that these
controllers were reporting CRIMS capability. They're just reporting
CRWMS as far as I know, so CRIME doesn't apply. I only included that
case in the patch for completeness.
 
> 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)

There is a corner case that I am somewhat purposefully ignoring: CAP.TO
is worst case for both CC.EN 0->1 and 1->0, whereas both CRTO values are
only for the 0->1 transition. It's entirely possible some implementation
needs a longer 1->0 transition, so CAP.TO *could* validly be greater
than the either CRTO value. I just don't see that happening in practice,
and even if we do encounter such a device, waiting a little longer on
init for a broken controller doesn't make any situation worse.