Re: [PATCH] nvme: avoid bogus CRTO values

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

Hello Keith,

On Wed, Sep 13, 2023 at 08:20:09AM -0700, Keith Busch wrote:
> On Wed, Sep 13, 2023 at 12:25:29PM +0000, Niklas Cassel wrote:
> > > +		if (ctrl->ctrl_config & NVME_CC_CRIME)
> > > +			timeout = max(timeout, NVME_CRTO_CRIMT(crto));
> > > +		else
> > > +			timeout = max(timeout, NVME_CRTO_CRWMT(crto));
> > 
> > I saw the original bug report.
> > But wasn't the problem that these were compared before NVME_CC_CRIME had
> > been written?
> >
> > i.e. is this max() check still needed for the bug reporter's NVMe drive,
> > after NVME_CC_CRIME was been written and CAP has been re-read?
> > (If so, would a quirk be better?)
> 
> The values reported in CRTO don't change with the CC writes. It's only
> CAP.TO that can change, so we still can't rely on CRTO at any point in
> the sequence for these devices.

Yes, I know that CRTO (which is the new register), doesn't change.
It is supposed to have to values:
CRTO.CRIMT
CRTO.CRWMT

The hack to modify CAP.TO to be in sync with either CRTO.CRIMT or
CRTO.CRWMT is really ugly.

Considering that CRTO.CRIMT and CRTO.CRWMT are also 16-bit,
so wider than CAP.TO, suggests that software should read these
if available, i.e. no need to ever read CAP.TO if
CAP.CRMS.CRWMS is 1 or if CAP.CRMS.CRIMS is 1.

CRTO.CRIMT is allowed to be 0, if CAP.CRMS.CRIMS is 0.
CRTO.CRWMT is not allowed to be 0 if CAP.CRMS.CRWMS is 1.
(and this bit should be set to 1 according to NVMe 2.0).

Additionally, NVMe 2.0b, Figure 35, clearly states register
CRTO as Mandatory for I/O and Admin controllers.


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.

Such a controller is so obviously broken that it deserves a quirk.

Although, I understand that using quirks is not always the best
solution for end users...


Kind regards,
Niklas