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
