[PATCH] libata-core: Allow longer timeout for drive spinup from PUIS

[PATCH] libata-core: Allow longer timeout for drive spinup from PUIS

[Damien Le Moal]

When spinning a drive from powered on standby mode (PUIS,
SETFEATURES_SPINUP is executed with the default timeout used
for any SETFEATURES subcommand, that is 5+10 seconds. The
total 15s is too short for some drives to complete spinup
(e.g. drives with a large indirection table stored on media),
resulting in ata_dev_read_id to fail twice on the execution
of SETFEATURES_SPINUP. For this feature, allow a larger
default timeout of 30 seconds. However, in the same spirit
as with the timeout of other feature subcommands, do not
ignore ata_probe_timeout if it is set).

Signed-off-by: Damien Le Moal <damien.lemoal@hgst.com>

diff --git a/drivers/ata/libata-core.c b/drivers/ata/libata-core.c
index 55e257c..c94a2a1 100644
--- a/drivers/ata/libata-core.c
+++ b/drivers/ata/libata-core.c
@@ -4528,6 +4528,7 @@ unsigned int ata_dev_set_feature(struct ata_device *dev, u8 enable, u8 feature)
 {
 	struct ata_taskfile tf;
 	unsigned int err_mask;
+	unsigned long timeout = 0;
 
 	/* set up set-features taskfile */
 	DPRINTK("set features - SATA features\n");
@@ -4539,7 +4540,10 @@ unsigned int ata_dev_set_feature(struct ata_device *dev, u8 enable, u8 feature)
 	tf.protocol = ATA_PROT_NODATA;
 	tf.nsect = feature;
 
-	err_mask = ata_exec_internal(dev, &tf, NULL, DMA_NONE, NULL, 0, 0);
+	if (enable == SETFEATURES_SPINUP)
+		timeout = (ata_probe_timeout ?
+			   ata_probe_timeout : SETFEATURES_SPINUP_TIMEOUT) * 1000;
+	err_mask = ata_exec_internal(dev, &tf, NULL, DMA_NONE, NULL, 0, timeout);
 
 	DPRINTK("EXIT, err_mask=%x\n", err_mask);
 	return err_mask;
diff --git a/include/linux/ata.h b/include/linux/ata.h
index c1a2f34..f460e7f 100644
--- a/include/linux/ata.h
+++ b/include/linux/ata.h
@@ -371,7 +371,8 @@ enum {
 	SETFEATURES_AAM_ON	= 0x42,
 	SETFEATURES_AAM_OFF	= 0xC2,
 
-	SETFEATURES_SPINUP	= 0x07, /* Spin-up drive */
+	SETFEATURES_SPINUP		= 0x07, /* Spin-up drive */
+	SETFEATURES_SPINUP_TIMEOUT	= 30, /* 30s timeout for drive spin-up from PUIS */
 
 	SETFEATURES_SATA_ENABLE = 0x10, /* Enable use of SATA feature */
 	SETFEATURES_SATA_DISABLE = 0x90, /* Disable use of SATA feature */

------------------------
Damien Le Moal, Ph.D.
Sr. Manager, System Software Group, HGST Research,
HGST, a Western Digital company
Damien.LeMoal@hgst.com
(+81) 0466-98-3593 (ext. 513593)
1 kirihara-cho, Fujisawa, 
Kanagawa, 252-0888 Japan
www.hgst.com 
Western Digital Corporation (and its subsidiaries) E-mail Confidentiality Notice & Disclaimer:

This e-mail and any files transmitted with it may contain confidential or legally privileged information of WDC and/or its affiliates, and are intended solely for the use of the individual or entity to which they are addressed. If you are not the intended recipient, any disclosure, copying, distribution or any action taken or omitted to be taken in reliance on it, is prohibited. If you have received this e-mail in error, please notify the sender immediately and delete the e-mail in its entirety from your system.

Re: [PATCH] libata-core: Allow longer timeout for drive spinup from PUIS

[Tejun Heo]

Hello, Damien.

On Tue, Mar 22, 2016 at 08:22:22AM +0000, Damien Le Moal wrote:
> diff --git a/include/linux/ata.h b/include/linux/ata.h
> index c1a2f34..f460e7f 100644
> --- a/include/linux/ata.h
> +++ b/include/linux/ata.h
> @@ -371,7 +371,8 @@ enum {
>  	SETFEATURES_AAM_ON	= 0x42,
>  	SETFEATURES_AAM_OFF	= 0xC2,
>  
> -	SETFEATURES_SPINUP	= 0x07, /* Spin-up drive */
> +	SETFEATURES_SPINUP		= 0x07, /* Spin-up drive */
> +	SETFEATURES_SPINUP_TIMEOUT	= 30, /* 30s timeout for drive spin-up from PUIS */

Other timeout constants are defined in msecs.  Let's do the same here.
Also, can you please send the email as plain-text without the base64
encoding?  If you can't, please send the patch as an attachment
instead.

Thanks.

-- 
tejun

Re: [PATCH] libata-core: Allow longer timeout for drive spinup from PUIS

[Damien Le Moal]

[-- Attachment #1: Type: text/plain, Size: 1928 bytes --]


Tejun,

>On Tue, Mar 22, 2016 at 08:22:22AM +0000, Damien Le Moal wrote:
>> diff --git a/include/linux/ata.h b/include/linux/ata.h
>> index c1a2f34..f460e7f 100644
>> --- a/include/linux/ata.h
>> +++ b/include/linux/ata.h
>> @@ -371,7 +371,8 @@ enum {
>>  	SETFEATURES_AAM_ON	= 0x42,
>>  	SETFEATURES_AAM_OFF	= 0xC2,
>>  
>> -	SETFEATURES_SPINUP	= 0x07, /* Spin-up drive */
>> +	SETFEATURES_SPINUP		= 0x07, /* Spin-up drive */
>> +	SETFEATURES_SPINUP_TIMEOUT	= 30, /* 30s timeout for drive spin-up from PUIS */
>
>Other timeout constants are defined in msecs.  Let's do the same here.
>Also, can you please send the email as plain-text without the base64
>encoding?  If you can't, please send the patch as an attachment
>instead.

Sorry about the email format (I am sending emails in plain text, but my
Outlook exchange client/exchange server does not seem to be respecting
that format).

Here is the patch as attachment, reworked to define the timeout in
milliseconds as you suggested.

Thank you.

Best regards.


--
Damien Le Moal, Ph.D.
Sr Manager, System Software Group, WW Research,
HGST, a Western Digital company
Damien.LeMoal@hgst.com
Tel: (+81) 0466-98-3593 (Ext. 51-3593)
1 kirihara-cho, Fujisawa, Kanagawa, 252-0888 Japan
www.hgst.com <http://www.hgst.com>
Western Digital Corporation (and its subsidiaries) E-mail Confidentiality Notice & Disclaimer:

This e-mail and any files transmitted with it may contain confidential or legally privileged information of WDC and/or its affiliates, and are intended solely for the use of the individual or entity to which they are addressed. If you are not the intended recipient, any disclosure, copying, distribution or any action taken or omitted to be taken in reliance on it, is prohibited. If you have received this e-mail in error, please notify the sender immediately and delete the e-mail in its entirety from your system.

[-- Attachment #2: set-features-spinup-timeout.patch --]
[-- Type: application/octet-stream, Size: 2361 bytes --]

commit c75ab67dcbf5ac1e88abd967b07a0279ab488559
Author: Damien Le Moal <damien.lemoal@hgst.com>
Date:   Tue Mar 22 10:36:48 2016 +0900

    libata-core: Allow longer timeout for drive spinup from PUIS
    
    When spinning up a drive from powered on standby mode (PUIS),
    SETFEATURES_SPINUP is executed with the default timeout used
    for any SETFEATURES subcommand, that is 5+10 seconds. The
    total 15s is too short for some drives to complete spinup
    (e.g. drives with a large indirection table stored on media),
    resulting in ata_dev_read_id to fail twice on the execution
    of SETFEATURES_SPINUP. For this feature, allow a larger
    default timeout of 30 seconds. However, in the same spirit
    as with the timeout of other feature subcommands, do not
    ignore ata_probe_timeout if it is set).
    
    Signed-off-by: Damien Le Moal <damien.lemoal@hgst.com>

diff --git a/drivers/ata/libata-core.c b/drivers/ata/libata-core.c
index 55e257c..c94a2a1 100644
--- a/drivers/ata/libata-core.c
+++ b/drivers/ata/libata-core.c
@@ -4528,6 +4528,7 @@ unsigned int ata_dev_set_feature(struct ata_device *dev, u8 enable, u8 feature)
 {
 	struct ata_taskfile tf;
 	unsigned int err_mask;
+	unsigned long timeout = 0;
 
 	/* set up set-features taskfile */
 	DPRINTK("set features - SATA features\n");
@@ -4539,7 +4540,10 @@ unsigned int ata_dev_set_feature(struct ata_device *dev, u8 enable, u8 feature)
 	tf.protocol = ATA_PROT_NODATA;
 	tf.nsect = feature;
 
-	err_mask = ata_exec_internal(dev, &tf, NULL, DMA_NONE, NULL, 0, 0);
+	if (enable == SETFEATURES_SPINUP)
+		timeout = ata_probe_timeout ?
+			  ata_probe_timeout * 1000 : SETFEATURES_SPINUP_TIMEOUT;
+	err_mask = ata_exec_internal(dev, &tf, NULL, DMA_NONE, NULL, 0, timeout);
 
 	DPRINTK("EXIT, err_mask=%x\n", err_mask);
 	return err_mask;
diff --git a/include/linux/ata.h b/include/linux/ata.h
index c1a2f34..f460e7f 100644
--- a/include/linux/ata.h
+++ b/include/linux/ata.h
@@ -371,7 +371,8 @@ enum {
 	SETFEATURES_AAM_ON	= 0x42,
 	SETFEATURES_AAM_OFF	= 0xC2,
 
-	SETFEATURES_SPINUP	= 0x07, /* Spin-up drive */
+	SETFEATURES_SPINUP		= 0x07, /* Spin-up drive */
+	SETFEATURES_SPINUP_TIMEOUT	= 30000, /* 30s timeout for drive spin-up from PUIS */
 
 	SETFEATURES_SATA_ENABLE = 0x10, /* Enable use of SATA feature */
 	SETFEATURES_SATA_DISABLE = 0x90, /* Disable use of SATA feature */

Re: [PATCH] libata-core: Allow longer timeout for drive spinup from PUIS

[Tejun Heo]

On Sat, Apr 02, 2016 at 12:23:02PM +0000, Damien Le Moal wrote:
> Sorry about the email format (I am sending emails in plain text, but my
> Outlook exchange client/exchange server does not seem to be respecting
> that format).
> 
> Here is the patch as attachment, reworked to define the timeout in
> milliseconds as you suggested.

Applied to libata/for-4.7.  Can you please use git format-patch to
generate the patch file from next time?

Thanks.

-- 
tejun

Re: [PATCH] libata-core: Allow longer timeout for drive spinup from PUIS

[Damien Le Moal]

Tejun,

>Applied to libata/for-4.7.  Can you please use git format-patch to
>generate the patch file from next time?

Thanks !
For the patch, I used “git show xxx > file”...
Is this not correct ? Should I be using a “—pretty=xxx” formatting ?

Best regards.

--
Damien Le Moal, Ph.D.
Sr Manager, System Software Group, WW Research,
HGST, a Western Digital company
Damien.LeMoal@hgst.com
Tel: (+81) 0466-98-3593 (Ext. 51-3593)
1 kirihara-cho, Fujisawa, Kanagawa, 252-0888 Japan
www.hgst.com <http://www.hgst.com>

Western Digital Corporation (and its subsidiaries) E-mail Confidentiality Notice & Disclaimer:

This e-mail and any files transmitted with it may contain confidential or legally privileged information of WDC and/or its affiliates, and are intended solely for the use of the individual or entity to which they are addressed. If you are not the intended recipient, any disclosure, copying, distribution or any action taken or omitted to be taken in reliance on it, is prohibited. If you have received this e-mail in error, please notify the sender immediately and delete the e-mail in its entirety from your system.

Re: [PATCH] libata-core: Allow longer timeout for drive spinup from PUIS

[James Bottomley]

On Thu, 2016-04-07 at 01:22 +0000, Damien Le Moal wrote:
> Tejun,
> 
> > Applied to libata/for-4.7.  Can you please use git format-patch to
> > generate the patch file from next time?
> 
> Thanks !
> For the patch, I used “git show xxx > file”...
> Is this not correct ? Should I be using a “—pretty=xxx” formatting ?

If you want to use git show, then you need --pretty=email (depending on
whether you want to add the commit statistics, you can also add --stat 
--patch).

If you have a patch series, it's often better to use

git-format-patch -n --cover-letter

Which generates a set of numbered files for the entire patch series.

James

Re: [PATCH] libata-core: Allow longer timeout for drive spinup from PUIS

[Tejun Heo]

Hello,

On Thu, Apr 07, 2016 at 01:22:19AM +0000, Damien Le Moal wrote:
> >Applied to libata/for-4.7.  Can you please use git format-patch to
> >generate the patch file from next time?
> 
> Thanks !
> For the patch, I used “git show xxx > file”...
> Is this not correct ? Should I be using a “—pretty=xxx” formatting ?

I always use git format-patch.  For a single patch, you can simply do
"git format-patch HEAD^".

Thanks.

-- 
tejun