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
Western Digital Research
