Fw: Question on Patch - NVMe: Shutdown controller only for power-off

Fw: Question on Patch - NVMe: Shutdown controller only for power-off

[Jeff Lien]

Forwarding the plain text version to the distribution/

 ----------------------------------------------------------
 Jeff Lien
 Linux Device Driver Development
 Device Host Apps and Drivers
 Western Digital Corporation
 e. ?jeff.lien at hgst.com
 o.? +1-507-322-2416
 m. +1-507-273-9124
 
 
 
 
 
 
 
    



From: Jeff Lien
Sent: Tuesday, June 07, 2016 12:29 PM
To: Busch, Keith; axboe at fb.com; linux-nvme at lists.infradead.org
Cc: David Darrington; Guilherme G. Piccoli; Murali N Iyer
Subject: Question on Patch - NVMe: Shutdown controller only for power-off
? 

Keith, Jens,
I have a question on the subject matter patch that was pushed on 1/12/2016. ? In the nvme_reset_work function in pci.c, the following comment doesn't seem to match what the code is doing.




?
1893???????? /*
1894????????? * If we're called to reset a live controller first shut it down before
1895????????? * moving on.
1896????????? */
1897???????? if (dev->bar)
1898???????????????? nvme_dev_disable(dev, false);


Calling nvme_dev_disable with a shutdown parm of false does not shutdown the device. ? Am I reading the comment or code wrong here? ? We're going thru this code path on a sysfs reset_controller to reset the device and activate a new level of fw and expected a shutdown to happen like it did in the 3.10.0-327 kernel. ??I'm thinking  the shutdown parm should be true in this call. ?




----------------------------------------------------------
Jeff Lien
Linux Device Driver Development
Device Host Apps and Drivers
Western Digital Corporation
e. ?jeff.lien at hgst.com
o.? +1-507-322-2416
m. +1-507-273-9124


1456350886356_PastedImage







       

Question on Patch - NVMe: Shutdown controller only for power-off

[Keith Busch]

On Tue, Jun 07, 2016@05:29:01PM +0000, Jeff Lien wrote:
> Keith, Jens,
> 
> I have a question on the subject matter patch that was pushed on 1/12/2016.   In the nvme_reset_work function in pci.c, the following comment doesn't seem to match what the code is doing.
> 
> 1893         /*
> 1894          * If we're called to reset a live controller first shut it down before
> 1895          * moving on.
> 1896          */
> 1897         if (dev->bar)
> 1898                 nvme_dev_disable(dev, false);
> 
> 
> Calling nvme_dev_disable with a shutdown parm of false does not shutdown the device.   Am I reading the comment or code wrong here?   We're going thru this code path on a sysfs reset_controller to reset the device and activate a new level of fw and expected a shutdown to happen like it did in the 3.10.0-327 kernel.   I'm thinking the shutdown parm should be true in this call.

It still disables the controller but it doesn't hit the shutdown bits. Why
do you need toggle CC.SHN for firmware activation? That's not a valid
firmware activation requirement according to the spec.

Question on Patch - NVMe: Shutdown controller only for power-off

[Jeff Lien]

Keith,
The issue isn't that the fw activate requires a shutdown; it's more that the HGST controller takes much longer to execute the nvme_disable_ctrl if it hasn't been shutdown first.  This issue caused us to add a longer delay before the first read of the csts reg in nvme_wait_ready.

Rather than adding a delay in nvme_disable_controller, I think the better option would be to shutdown the controller.  The comment says "If we're called to reset a live controller first shut it down" so in this case I think the shutdown parm should be true.

----------------------------------------------------------
Jeff Lien
Linux Device Driver Development
Device Host Apps and Drivers
Western Digital Corporation
e.  jeff.lien at hgst.com
o.  +1-507-322-2416
m. +1-507-273-9124





________________________________________
From: Keith Busch <keith.busch@intel.com>
Sent: Tuesday, June 07, 2016 12:58:42 PM
To: Jeff Lien
Cc: axboe at fb.com; linux-nvme at lists.infradead.org; David Darrington; Guilherme G. Piccoli; Murali N Iyer
Subject: Re: Question on Patch - NVMe: Shutdown controller only for power-off

On Tue, Jun 07, 2016@05:29:01PM +0000, Jeff Lien wrote:
> Keith, Jens,
>
> I have a question on the subject matter patch that was pushed on 1/12/2016.   In the nvme_reset_work function in pci.c, the following comment doesn't seem to match what the code is doing.
>
> 1893         /*
> 1894          * If we're called to reset a live controller first shut it down before
> 1895          * moving on.
> 1896          */
> 1897         if (dev->bar)
> 1898                 nvme_dev_disable(dev, false);
>
>
> Calling nvme_dev_disable with a shutdown parm of false does not shutdown the device.   Am I reading the comment or code wrong here?   We're going thru this code path on a sysfs reset_controller to reset the device and activate a new level of fw and expected a shutdown to happen like it did in the 3.10.0-327 kernel.   I'm thinking the shutdown parm should be true in this call.

It still disables the controller but it doesn't hit the shutdown bits. Why
do you need toggle CC.SHN for firmware activation? That's not a valid
firmware activation requirement according to the spec.

Question on Patch - NVMe: Shutdown controller only for power-off

[Keith Busch]

On Tue, Jun 07, 2016@06:42:00PM +0000, Jeff Lien wrote:
> Keith,
> The issue isn't that the fw activate requires a shutdown; it's more that the HGST controller takes much longer to execute the nvme_disable_ctrl if it hasn't been shutdown first.  This issue caused us to add a longer delay before the first read of the csts reg in nvme_wait_ready.
> 
> Rather than adding a delay in nvme_disable_controller, I think the better option would be to shutdown the controller.  The comment says "If we're called to reset a live controller first shut it down" so in this case I think the shutdown parm should be true.

I'll change the comment to say "disable" instead of "shut down". We
want to reserve controller shutdown to power loss scenarios per spec
recommendations, which this is not. A controller reset is generally much
faster than a shutdown, which is why we don't toggle CC.SHN here.