Re: [PATCH] nvme: add support for TP4084 - Time-to-Ready Enhancements

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

On Wed, May 18, 2022 at 05:00:44PM +0200, Christoph Hellwig wrote:
> On Wed, May 18, 2022 at 08:36:47AM -0600, Keith Busch wrote:
> > On Wed, May 18, 2022 at 08:40:40AM +0200, Christoph Hellwig wrote:
> > > +static int nvme_identify_ns_cs_indep(struct nvme_ctrl *ctrl, unsigned nsid,
> > > +			struct nvme_id_ns_cs_indep **id)
> > > +{
> > > +	struct nvme_command c = {
> > > +		.identify.opcode	= nvme_admin_identify,
> > > +		.identify.nsid		= cpu_to_le32(nsid),
> > > +		.identify.cns		= NVME_ID_CNS_NS_CS_INDEP,
> > > +	};
> > > +	int error;
> > 
> > Every other place in this driver calls this value 'ret'.
> 
> Except for nvme_identify_ns, where I copied it from :)
> 
> But I can change it to ret.
> 
> > 
> > > -static int nvme_wait_ready(struct nvme_ctrl *ctrl, u64 cap, bool enabled)
> > > +static int nvme_wait_ready(struct nvme_ctrl *ctrl, bool enabled)
> > >  {
> > > -	unsigned long timeout =
> > > -		((NVME_CAP_TIMEOUT(cap) + 1) * HZ / 2) + jiffies;
> > > +	unsigned long timeout = ((ctrl->enable_timeout + 1) * HZ / 2) + jiffies;
> > 
> > I think we need to continue using the NVME_CAP_TIMEOUT() value when 'enabled'
> > is false. The enable_timeout is only applicable for the 0->1 transition and may
> > be too short if CRIME is set or too long when it's not.

From the CC.CRIME description:
Changing the value of this field may cause a change in the time reported in the
CAP.TO field. Refer to the definition of CAP.TO for more details.

The definition for CAP.TO:
If the Controller Ready Independent of Media Enable (CC.CRIME) bit is set to ‘1’
and the worst-case time for CSTS.RDY to change state is due to enabling the
controller after CC.EN transitions from ‘0’ to ‘1’, then this field shall be set
to:
a) the value in Controller Ready Independent of Media Timeout (CRTO.CRIMT); or
b) FFh if CRTO.CRIMT is greater than FFh.

This phrasing is quite confusing IMO.

I interpret this as:
If CC.CRIME is set, and CC.EN changes value from 0 to 1,
then the the controller shall write the value of CRTO.CRIMT to CAP.TO.
(If we want to make use of CAP.TO, then we would need to read the value
after setting CC.EN.)

When does the controller change CAP.TO to contain the "disable" timeout?
After CSTS.RDY has transitioned to 1...?

Since CAP.TO register contains either the enable or disable timeout,
it is quite important to know at what point in time it will contain
one or the other, so we can know if we should read CAP.TO before or
after setting a new CC.EN value.

In nvme_enable_ctrl(), we read CAP register first, then set the bits.
Since this patch never uses CAP.TO when CRIMS is supported, it should
not matter that we read the register before setting CC.EN.

nvme_disable_ctrl() doesn't re-read the register after setting CC.EN = 0.
It relies on the cached register value that was read by nvme_enable_ctrl().
Since nvme_enable_ctrl() reads the value before setting CC.CRIME
(and because the reset value of CC.CRIME is 0), the cached value should
always be the long timeout.

So nvme_enable_ctrl() and nvme_disable_ctrl() should both use the correct
timeout value with this patch, even though it seems a bit fragile.

If we ever read and update ctrl->caps, depending on the state of the
controller, NVME_CAP_TIMEOUT(ctrl->cap) can potentially contain the
CRTO.CRIMT value, and then using this value in nvme_disable_ctrl()
could potentially cause us to use the (short) enable timeout instead
of the (long) disable timeout.

It would have been way easier if it was just separate registers for
enable and disable timeouts...

If we want things to be more robust, I guess we could read the CAP.TO
register after a reset, and save that value in a ctrl->disable_timeout
which nvme_disable_ctrl() then reuses. Enable timeout can be as it is
in V2, it does not need to be saved.


Kind regards,
Niklas