[4/4] libata: don't start PuiS disks on resume

Disks with Power Up In Standby enabled that required the
SET FEATURES command to start up were being issued the
command during resume.  Suppress this until the disk
is actually accessed.
---
 drivers/ata/libata-core.c | 24 ++++++++++++++++++++++--
 drivers/ata/libata-eh.c   | 12 +++++++++++-
 drivers/ata/libata.h      |  1 +
 include/linux/libata.h    |  1 +
 4 files changed, 35 insertions(+), 3 deletions(-)

On 1/5/24 1:39 AM, Phillip Susi wrote:

> Disks with Power Up In Standby enabled that required the
> SET FEATURES command to start up were being issued the
> command during resume.  Suppress this until the disk
> is actually accessed.
> ---
>  drivers/ata/libata-core.c | 24 ++++++++++++++++++++++--
>  drivers/ata/libata-eh.c   | 12 +++++++++++-
>  drivers/ata/libata.h      |  1 +
>  include/linux/libata.h    |  1 +
>  4 files changed, 35 insertions(+), 3 deletions(-)
> 
> diff --git a/drivers/ata/libata-core.c b/drivers/ata/libata-core.c
> index d9e889fa2881..3f6187c75b16 100644
> --- a/drivers/ata/libata-core.c
> +++ b/drivers/ata/libata-core.c
> @@ -1912,7 +1912,25 @@ int ata_dev_read_id(struct ata_device *dev, unsigned int *p_class,
>  			goto err_out;
>  	}
>  
> -	if (!tried_spinup && (id[2] == 0x37c8 || id[2] == 0x738c)) {
> +	if (flags & ATA_READID_NOSTART && id[2] == 0x37c8)
> +	{
> +		/*
> +		 * the drive has powered up in standby, and returned incomplete IDENTIFY info

   s/the/The/.

> +		 * so we can't revalidate it yet.  We have also been asked to avoid starting the
> +		 * drive, so stop  here and leave the drive asleep and the EH pending, to be

   Double space...

> +		 * restarted on later IO request

   I think more common form is I/O.

[...]
> diff --git a/drivers/ata/libata-eh.c b/drivers/ata/libata-eh.c
> index 799a1b8bc384..74e0d54a204e 100644
> --- a/drivers/ata/libata-eh.c
> +++ b/drivers/ata/libata-eh.c
[...]
> @@ -3075,9 +3078,16 @@ static int ata_eh_revalidate_and_attach(struct ata_link *link,
>  			ata_eh_about_to_do(link, dev, ATA_EH_REVALIDATE);
>  			rc = ata_dev_revalidate(dev, ehc->classes[dev->devno],
>  						readid_flags);
> -			if (rc)
> +			if (rc == -EAGAIN) {
> +				rc = 0; /* start required but suppressed, handle later */
> +				ehc->i.dev_action[dev->devno] &= ~ATA_EH_SET_ACTIVE;
> +				ata_dev_warn(dev, "Leaving PuiS disk asleep for now");
> +				continue;
> +			}
> +			else if (rc)

			} else if (rc) {

>  				goto err;

			}

   You need {} on all *if* branches if at least one has it, as dictated
by the kernel coding style...

[...]

MBR, Sergey

On 1/5/24 07:39, Phillip Susi wrote:
> Disks with Power Up In Standby enabled that required the
> SET FEATURES command to start up were being issued the
> command during resume.  Suppress this until the disk
> is actually accessed.
> ---
>  drivers/ata/libata-core.c | 24 ++++++++++++++++++++++--
>  drivers/ata/libata-eh.c   | 12 +++++++++++-
>  drivers/ata/libata.h      |  1 +
>  include/linux/libata.h    |  1 +
>  4 files changed, 35 insertions(+), 3 deletions(-)
> 
> diff --git a/drivers/ata/libata-core.c b/drivers/ata/libata-core.c
> index d9e889fa2881..3f6187c75b16 100644
> --- a/drivers/ata/libata-core.c
> +++ b/drivers/ata/libata-core.c
> @@ -1912,7 +1912,25 @@ int ata_dev_read_id(struct ata_device *dev, unsigned int *p_class,
>  			goto err_out;
>  	}
>  
> -	if (!tried_spinup && (id[2] == 0x37c8 || id[2] == 0x738c)) {
> +	if (flags & ATA_READID_NOSTART && id[2] == 0x37c8)
> +	{
> +		/*
> +		 * the drive has powered up in standby, and returned incomplete IDENTIFY info
> +		 * so we can't revalidate it yet.  We have also been asked to avoid starting the
> +		 * drive, so stop  here and leave the drive asleep and the EH pending, to be
> +		 * restarted on later IO request
> +		 */
> +		ata_tf_init(dev, &tf);
> +		tf.flags |= ATA_TFLAG_DEVICE | ATA_TFLAG_ISADDR;
> +		tf.protocol = ATA_PROT_NODATA;
> +		tf.command = ATA_CMD_SLEEP;
> +		err_mask = ata_exec_internal(dev, &tf, NULL, DMA_NONE, NULL, 0, 0);
> +		ata_dev_info(dev, "PuiS detected, putting drive to sleep");

Why ? The drive is in standby. What is the point of putting it into sleep state
? Furthermore, if you check ACS-6 specs, you will see that there is no
transitions defined from PUIS state to sleep state. You have to spin-up the
drive first. So the above is outside the specified behavior and thus not
reliable (even though many drive may actually allow this transition).

> +		return -EAGAIN;
> +	}
> +
> +	if (!(flags & ATA_READID_NOSTART) &&
> +	    !tried_spinup && (id[2] == 0x37c8 || id[2] == 0x738c)) {
>  		tried_spinup = 1;
>  		/*
>  		 * Drive powered-up in standby mode, and requires a specific
> @@ -3973,6 +3991,8 @@ int ata_dev_revalidate(struct ata_device *dev, unsigned int new_class,
>  
>  	/* re-read ID */
>  	rc = ata_dev_reread_id(dev, readid_flags);
> +	if (rc == -EAGAIN)
> +		return rc;
>  	if (rc)
>  		goto fail;
>  
> @@ -5241,7 +5261,7 @@ static void ata_port_suspend(struct ata_port *ap, pm_message_t mesg,
>  	 * http://thread.gmane.org/gmane.linux.ide/46764
>  	 */
>  	ata_port_request_pm(ap, mesg, 0,
> -			    ATA_EHI_QUIET | ATA_EHI_NO_AUTOPSY |
> +			    ATA_EHI_QUIET | ATA_EHI_NO_AUTOPSY | ATA_EHI_NOSTART
>  			    ATA_EHI_NO_RECOVERY,
>  			    async);
>  }
> diff --git a/drivers/ata/libata-eh.c b/drivers/ata/libata-eh.c
> index 799a1b8bc384..74e0d54a204e 100644
> --- a/drivers/ata/libata-eh.c
> +++ b/drivers/ata/libata-eh.c
> @@ -22,6 +22,7 @@
>  #include <scsi/scsi_cmnd.h>
>  #include <scsi/scsi_dbg.h>
>  #include "../scsi/scsi_transport_api.h"
> +#include <linux/pm_runtime.h>
>  
>  #include <linux/libata.h>
>  
> @@ -3046,6 +3047,8 @@ static int ata_eh_revalidate_and_attach(struct ata_link *link,
>  
>  		if (ehc->i.flags & ATA_EHI_DID_RESET)
>  			readid_flags |= ATA_READID_POSTRESET;
> +		if (ehc->i.flags & ATA_EHI_NOSTART)
> +			readid_flags |= ATA_READID_NOSTART;
>  
>  		if ((action & ATA_EH_REVALIDATE) && ata_dev_enabled(dev)) {
>  			WARN_ON(dev->class == ATA_DEV_PMP);
> @@ -3075,9 +3078,16 @@ static int ata_eh_revalidate_and_attach(struct ata_link *link,
>  			ata_eh_about_to_do(link, dev, ATA_EH_REVALIDATE);
>  			rc = ata_dev_revalidate(dev, ehc->classes[dev->devno],
>  						readid_flags);
> -			if (rc)
> +			if (rc == -EAGAIN) {

Rather than playing with the return values, it may be easier to use a device
flag (similar to ATA_DFLAG_SLEEPING) to track standby/spun-down state.

> +				rc = 0; /* start required but suppressed, handle later */
> +				ehc->i.dev_action[dev->devno] &= ~ATA_EH_SET_ACTIVE;
> +				ata_dev_warn(dev, "Leaving PuiS disk asleep for now");
> +				continue;
> +			}
> +			else if (rc)
>  				goto err;
>  
> +			pm_runtime_resume(&dev->sdev->sdev_gendev);

What does this do ??? This look really out of place.

>  			ata_eh_done(link, dev, ATA_EH_REVALIDATE);
>  
>  			/* Configuration may have changed, reconfigure
> diff --git a/drivers/ata/libata.h b/drivers/ata/libata.h
> index 43ad1ef9b63a..cff3facad055 100644
> --- a/drivers/ata/libata.h
> +++ b/drivers/ata/libata.h
> @@ -19,6 +19,7 @@
>  enum {
>  	/* flags for ata_dev_read_id() */
>  	ATA_READID_POSTRESET	= (1 << 0), /* reading ID after reset */
> +	ATA_READID_NOSTART	= (1 << 1), /* do not start drive */
>  
>  	/* selector for ata_down_xfermask_limit() */
>  	ATA_DNXFER_PIO		= 0,	/* speed down PIO */
> diff --git a/include/linux/libata.h b/include/linux/libata.h
> index 1dbb14daccfa..50d6fa933946 100644
> --- a/include/linux/libata.h
> +++ b/include/linux/libata.h
> @@ -328,6 +328,7 @@ enum {
>  
>  	/* ata_eh_info->flags */
>  	ATA_EHI_HOTPLUGGED	= (1 << 0),  /* could have been hotplugged */
> +	ATA_EHI_NOSTART		= (1 << 1),  /* don't start the disk */
>  	ATA_EHI_NO_AUTOPSY	= (1 << 2),  /* no autopsy */
>  	ATA_EHI_QUIET		= (1 << 3),  /* be quiet */
>  	ATA_EHI_NO_RECOVERY	= (1 << 4),  /* no recovery */

Damien Le Moal <dlemoal@kernel.org> writes:

> Why ? The drive is in standby. What is the point of putting it into
> sleep state

I just figured it would save a little more power than just *pretending*
the drive was asleep ( which is what I did at first ).  I guess I can go
back to just setting the sleeping flag without telling the drive to
actually go to sleep.

> ? Furthermore, if you check ACS-6 specs, you will see that there is no
> transitions defined from PUIS state to sleep state. You have to spin-up the
> drive first. So the above is outside the specified behavior and thus not
> reliable (even though many drive may actually allow this transition).

Oh poo.  Though now I'm curious if it actually does fail on any.  It
seems like a pretty obvious thing to do and an oversight on the part of
the spec.  The kernel certainly doesn't do anything to prevent the user
from running hdparm -Y on a drive that is already in standby.

Wait, is that specifically from PuiS to sleep, or standby to sleep in general?

> Rather than playing with the return values, it may be easier to use a device
> flag (similar to ATA_DFLAG_SLEEPING) to track standby/spun-down state.

You mean change each if (rc == -EAGAIN) to if (ATA_DFLAG_SLEEPING)? That
doesn't seem any easier to me, but I'm not opposed to it.

>> +			pm_runtime_resume(&dev->sdev->sdev_gendev);
>
> What does this do ??? This look really out of place.

Darnit, I thought I had removed all of the runtime pm stuff.  My bad.