Re: [PATCH v2 2/2] nvme/host/core: Allow overriding of wait_ready timeout

["Singh, Balbir" <sblbir@amazon.com>]

On Mon, 2019-09-16 at 09:41 +0200, Christoph Hellwig wrote:
> On Fri, Sep 13, 2019 at 11:36:31PM +0000, Balbir Singh wrote:
> > +unsigned int nvme_wait_ready_timeout = 0;
> > +module_param_named(wait_ready_timeout, nvme_wait_ready_timeout,
> > uint, 0644);
> > +MODULE_PARM_DESC(wait_ready_timeout, "timeout in seconds for wait
> > ready on reset");
> 
> This is only used in core.c, so it can be marked static.
> 
> Also it introduces a > 80 char line.

I'll fix that, I must revisit my checkpatch results.

> 
> > +
> >  static unsigned char shutdown_timeout = 5;
> >  module_param(shutdown_timeout, byte, 0644);
> >  MODULE_PARM_DESC(shutdown_timeout, "timeout in seconds for
> > controller shutdown");
> > @@ -1938,6 +1942,9 @@ static int nvme_wait_ready(struct nvme_ctrl
> > *ctrl, u64 cap, bool enabled)
> >  	u32 csts, bit = enabled ? NVME_CSTS_RDY : 0;
> >  	int ret;
> >  
> > +	if (nvme_wait_ready_timeout)
> > +		timeout = NVME_WAIT_READY_TIMEOUT + jiffies;
> 
> I'm not sure the NVME_WAIT_READY_TIMEOUT #define really helps much
> here.
> Also the code is a little confusing as as, why not and if / else with
> the normal timeout definition?

I could refactor the patch

> 
> Then again I'm not even sure we really want this.  The debugging use
> case is somethign where you can easily hack a line in the driver, and
> we really don't want normal users to mess with a random parameter
> like
> this one.

The reason I sent this out is that I've seen some controllers setting
this to max value. I suspect there is no good way for the controller
to set wait ready values either. In any case 128 seconds for a failure
seems a bit too much, specially if the controller does not respond on
boot and hence the debug option. I don't expect it to be used by normal
users on most of their systems

Balbir Singh.

_______________________________________________
Linux-nvme mailing list
Linux-nvme@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-nvme