Re: [PATCH 2/2] ata: libata-core: Improve ata_dev_power_set_active()

[Niklas Cassel <Niklas.Cassel@wdc.com>]

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@kernel.org>
> ---
>  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
> 

Hello Damien,

For a disk that is already spun up, is a READ VERIFY SECTOR(S) – 40h command
with LBA == 0, NSECTORS == 1, really that much slower than a ATA_CMD_CHK_POWER
command?


Kind regards,
Niklas