[PATCH 0/2] Improve drive spinup on resume

[PATCH 0/2] Improve drive spinup on resume

[Damien Le Moal]

Two patches to improve disk spinup on resume:
 - Patch 1: Move the spinup operation to after the drive revalidation to
   ensure that the VERIFY command is executed with the link speed
   properly setup.
 - Patch 2: Attempt to spinup the drive only if it is not already spun
   up.

Geert,

It would be great if you could test this with your RCAR setup. I ran
this on my usual machines and qemu. All good. But your setup has been
useful to flush out issues before :)
Thanks !

Damien Le Moal (2):
  ata: libata-eh: Spinup disk on resume after revalidation
  ata: libata-core: Improve ata_dev_power_set_active()

 drivers/ata/libata-core.c | 34 ++++++++++++++++++++++++++++++++++
 drivers/ata/libata-eh.c   | 20 +++++++++++---------
 2 files changed, 45 insertions(+), 9 deletions(-)

-- 
2.41.0

[PATCH 1/2] ata: libata-eh: Spinup disk on resume after revalidation

[Damien Le Moal]

Move the call to ata_dev_power_set_active() to transition a disk in
standby power mode to the active power mode from
ata_eh_revalidate_and_attach() before doing revalidation to the end of
ata_eh_recover(), after the link speed for the device is reconfigured
(if that was necessary). This is safer as this ensure that the VERIFY
command executed to spinup the disk is executed with the drive properly
reconfigured first.

Signed-off-by: Damien Le Moal <dlemoal@kernel.org>
---
 drivers/ata/libata-eh.c | 20 +++++++++++---------
 1 file changed, 11 insertions(+), 9 deletions(-)

diff --git a/drivers/ata/libata-eh.c b/drivers/ata/libata-eh.c
index 945675f6b822..b0d6e69c4a5b 100644
--- a/drivers/ata/libata-eh.c
+++ b/drivers/ata/libata-eh.c
@@ -3051,15 +3051,6 @@ static int ata_eh_revalidate_and_attach(struct ata_link *link,
 		if (ehc->i.flags & ATA_EHI_DID_RESET)
 			readid_flags |= ATA_READID_POSTRESET;
 
-		/*
-		 * When resuming, before executing any command, make sure to
-		 * transition the device to the active power mode.
-		 */
-		if ((action & ATA_EH_SET_ACTIVE) && ata_dev_enabled(dev)) {
-			ata_dev_power_set_active(dev);
-			ata_eh_done(link, dev, ATA_EH_SET_ACTIVE);
-		}
-
 		if ((action & ATA_EH_REVALIDATE) && ata_dev_enabled(dev)) {
 			WARN_ON(dev->class == ATA_DEV_PMP);
 
@@ -3856,6 +3847,17 @@ int ata_eh_recover(struct ata_port *ap, ata_prereset_fn_t prereset,
 			}
 		}
 
+		/*
+		 * Make sure to transition devices to the active power mode
+		 * if needed (e.g. if we were scheduled on system resume).
+		 */
+		ata_for_each_dev(dev, link, ENABLED) {
+			if (ehc->i.dev_action[dev->devno] & ATA_EH_SET_ACTIVE) {
+				ata_dev_power_set_active(dev);
+				ata_eh_done(link, dev, ATA_EH_SET_ACTIVE);
+			}
+		}
+
 		/* retry flush if necessary */
 		ata_for_each_dev(dev, link, ALL) {
 			if (dev->class != ATA_DEV_ATA &&
-- 
2.41.0

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

[Damien Le Moal]

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

Re: [PATCH 1/2] ata: libata-eh: Spinup disk on resume after revalidation

[Niklas Cassel]

On Thu, Oct 12, 2023 at 04:17:59PM +0900, Damien Le Moal wrote:
> Move the call to ata_dev_power_set_active() to transition a disk in
> standby power mode to the active power mode from
> ata_eh_revalidate_and_attach() before doing revalidation to the end of
> ata_eh_recover(), after the link speed for the device is reconfigured
> (if that was necessary). This is safer as this ensure that the VERIFY
> command executed to spinup the disk is executed with the drive properly
> reconfigured first.
> 
> Signed-off-by: Damien Le Moal <dlemoal@kernel.org>
> ---
>  drivers/ata/libata-eh.c | 20 +++++++++++---------
>  1 file changed, 11 insertions(+), 9 deletions(-)
> 
> diff --git a/drivers/ata/libata-eh.c b/drivers/ata/libata-eh.c
> index 945675f6b822..b0d6e69c4a5b 100644
> --- a/drivers/ata/libata-eh.c
> +++ b/drivers/ata/libata-eh.c
> @@ -3051,15 +3051,6 @@ static int ata_eh_revalidate_and_attach(struct ata_link *link,
>  		if (ehc->i.flags & ATA_EHI_DID_RESET)
>  			readid_flags |= ATA_READID_POSTRESET;
>  
> -		/*
> -		 * When resuming, before executing any command, make sure to
> -		 * transition the device to the active power mode.
> -		 */
> -		if ((action & ATA_EH_SET_ACTIVE) && ata_dev_enabled(dev)) {
> -			ata_dev_power_set_active(dev);
> -			ata_eh_done(link, dev, ATA_EH_SET_ACTIVE);
> -		}
> -
>  		if ((action & ATA_EH_REVALIDATE) && ata_dev_enabled(dev)) {
>  			WARN_ON(dev->class == ATA_DEV_PMP);
>  
> @@ -3856,6 +3847,17 @@ int ata_eh_recover(struct ata_port *ap, ata_prereset_fn_t prereset,
>  			}
>  		}
>  
> +		/*
> +		 * Make sure to transition devices to the active power mode
> +		 * if needed (e.g. if we were scheduled on system resume).
> +		 */
> +		ata_for_each_dev(dev, link, ENABLED) {
> +			if (ehc->i.dev_action[dev->devno] & ATA_EH_SET_ACTIVE) {
> +				ata_dev_power_set_active(dev);
> +				ata_eh_done(link, dev, ATA_EH_SET_ACTIVE);
> +			}
> +		}
> +
>  		/* retry flush if necessary */
>  		ata_for_each_dev(dev, link, ALL) {
>  			if (dev->class != ATA_DEV_ATA &&
> -- 
> 2.41.0
> 

Reviewed-by: Niklas Cassel <niklas.cassel@wdc.com>

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

[Niklas Cassel]

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

Re: [PATCH 0/2] Improve drive spinup on resume

[Geert Uytterhoeven]

Hi Damien,

On Thu, Oct 12, 2023 at 9:18 AM Damien Le Moal <dlemoal@kernel.org> wrote:
> Two patches to improve disk spinup on resume:
>  - Patch 1: Move the spinup operation to after the drive revalidation to
>    ensure that the VERIFY command is executed with the link speed
>    properly setup.
>  - Patch 2: Attempt to spinup the drive only if it is not already spun
>    up.

Thanks for your series!

> Geert,
>
> It would be great if you could test this with your RCAR setup. I ran
> this on my usual machines and qemu. All good. But your setup has been
> useful to flush out issues before :)

Nothing special to see here...
Tested-by: Geert Uytterhoeven <geert+renesas@glider.be>

Gr{oetje,eeting}s,

                        Geert

-- 
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@linux-m68k.org

In personal conversations with technical people, I call myself a hacker. But
when I'm talking to journalists I just say "programmer" or something like that.
                                -- Linus Torvalds

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

[Niklas Cassel]

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
> 

From your explaination in:
https://lore.kernel.org/linux-ide/c0b086ab-dcd5-4b7b-b931-4d407dd7ad47@kernel.org/

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@wdc.com>

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

[Phillip Susi]

Damien Le Moal <dlemoal@kernel.org> writes:
>  /**
>   *	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);

This bit didn't apply cleanly to what I just pulled from Linus.  It
seems there are soem differences in how the tf is set up.  Why not move
this check to before the tf is set up?  There isn't much point in
setting it up if it isn't going to be used.

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

[Niklas Cassel]

On Fri, Oct 13, 2023 at 11:14:09AM -0400, Phillip Susi wrote:
> Damien Le Moal <dlemoal@kernel.org> writes:
> >  /**
> >   *	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);
> 
> This bit didn't apply cleanly to what I just pulled from Linus.  It
> seems there are soem differences in how the tf is set up.  Why not move
> this check to before the tf is set up?  There isn't much point in
> setting it up if it isn't going to be used.

Hello Phillip,

This series applies cleanly to:
https://git.kernel.org/pub/scm/linux/kernel/git/dlemoal/libata.git/log/?h=for-next

The for-next branch also has a bunch of suspend/resume patches that will
not go in to v6.6.


Kind regards,
Niklas

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

[Damien Le Moal]

On 10/14/23 00:14, Phillip Susi wrote:
> Damien Le Moal <dlemoal@kernel.org> writes:
>>  /**
>>   *	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);
> 
> This bit didn't apply cleanly to what I just pulled from Linus.  It

The patches are for 6.7 so they are based on for-6.7 branch (for-next is the same).

> seems there are soem differences in how the tf is set up.  Why not move
> this check to before the tf is set up?  There isn't much point in
> setting it up if it isn't going to be used.

ata_dev_power_init_tf() also checks for the device type and horkage flags and
return false if the operation is not necessary or desired. I left these checks
in that function instead of moving them to different places. That is easier to
maintain this way. This is not the hot path, so I really prefer prioritizing
simplicity here.

-- 
Damien Le Moal
Western Digital Research