On Thu, Oct 12, 2023 at 04:18:00PM +0900, Damien Le Moal wrote:
> Improve the function ata_dev_power_set_active() by having it do nothing
> for a disk that is already in the active power state. To do that,
> introduce the function ata_dev_power_is_active() to test the current
> power state of the disk and return true if the disk is in the PM0:
> active or PM1: idle state (0xff value for the count field of the CHECK
> POWER MODE command output).
>
> To preserve the existing behavior, if the CHECK POWER MODE command
> issued in ata_dev_power_is_active() fails, the drive is assumed to be in
> standby mode and false is returned.
>
> With this change, issuing the VERIFY command to access the disk media to
> spin it up becomes unnecessary most of the time during system resume as
> the port reset done by libata-eh on resume often result in the drive to
> spin-up (this behavior is not clearly defined by the ACS specifications
> and may thus vary between disk models).
>
> Signed-off-by: Damien Le Moal <dlemoal@xxxxxxxxxx>
> ---
> drivers/ata/libata-core.c | 34 ++++++++++++++++++++++++++++++++++
> 1 file changed, 34 insertions(+)
>
> diff --git a/drivers/ata/libata-core.c b/drivers/ata/libata-core.c
> index 83613280928b..6fb4e8dc8c3c 100644
> --- a/drivers/ata/libata-core.c
> +++ b/drivers/ata/libata-core.c
> @@ -2042,6 +2042,33 @@ void ata_dev_power_set_standby(struct ata_device *dev)
> err_mask);
> }
>
> +static bool ata_dev_power_is_active(struct ata_device *dev)
> +{
> + struct ata_taskfile tf;
> + unsigned int err_mask;
> +
> + ata_tf_init(dev, &tf);
> + tf.flags |= ATA_TFLAG_DEVICE | ATA_TFLAG_ISADDR;
> + tf.protocol = ATA_PROT_NODATA;
> + tf.command = ATA_CMD_CHK_POWER;
> +
> + err_mask = ata_exec_internal(dev, &tf, NULL, DMA_NONE, NULL, 0, 0);
> + if (err_mask) {
> + ata_dev_err(dev, "Check power mode failed (err_mask=0x%x)\n",
> + err_mask);
> + /*
> + * Assume we are in standby mode so that we always force a
> + * spinup in ata_dev_power_set_active().
> + */
> + return false;
> + }
> +
> + ata_dev_dbg(dev, "Power mode: 0x%02x\n", tf.nsect);
> +
> + /* Active or idle */
> + return tf.nsect == 0xff;
> +}
> +
> /**
> * ata_dev_power_set_active - Set a device power mode to active
> * @dev: target device
> @@ -2065,6 +2092,13 @@ void ata_dev_power_set_active(struct ata_device *dev)
> if (!ata_dev_power_init_tf(dev, &tf, true))
> return;
>
> + /*
> + * Check the device power state & condition and force a spinup with
> + * VERIFY command only if the drive is not already ACTIVE or IDLE.
> + */
> + if (ata_dev_power_is_active(dev))
> + return;
> +
> ata_dev_notice(dev, "Entering active power mode\n");
>
> err_mask = ata_exec_internal(dev, &tf, NULL, DMA_NONE, NULL, 0, 0);
> --
> 2.41.0
>
>From your explaination in:
https://lore.kernel.org/linux-ide/c0b086ab-dcd5-4b7b-b931-4d407dd7ad47@xxxxxxxxxx/
The drive will be in the active power state when the drive is in the
process of spinning up the drive, even if the drive is not fully spun
up, and the reply to the first I/O will be delayed until the drive is
fully spun up.
Therefore, this patch makes a lot of sense:
Reviewed-by: Niklas Cassel <niklas.cassel@xxxxxxx>
