* Re: [PATCH 2/2] ata: libata-core: Improve ata_dev_power_set_active()
2023-10-12 7:18 ` [PATCH 2/2] ata: libata-core: Improve ata_dev_power_set_active() Damien Le Moal
@ 2023-10-12 11:07 ` Niklas Cassel
2023-10-13 8:51 ` Niklas Cassel
2023-10-13 15:14 ` Phillip Susi
2 siblings, 0 replies; 10+ messages in thread
From: Niklas Cassel @ 2023-10-12 11:07 UTC (permalink / raw)
To: Damien Le Moal
Cc: linux-ide@vger.kernel.org, Geert Uytterhoeven, Phillip Susi
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
^ permalink raw reply [flat|nested] 10+ messages in thread* Re: [PATCH 2/2] ata: libata-core: Improve ata_dev_power_set_active()
2023-10-12 7:18 ` [PATCH 2/2] ata: libata-core: Improve ata_dev_power_set_active() Damien Le Moal
2023-10-12 11:07 ` Niklas Cassel
@ 2023-10-13 8:51 ` Niklas Cassel
2023-10-13 15:14 ` Phillip Susi
2 siblings, 0 replies; 10+ messages in thread
From: Niklas Cassel @ 2023-10-13 8:51 UTC (permalink / raw)
To: Damien Le Moal
Cc: linux-ide@vger.kernel.org, Geert Uytterhoeven, Phillip Susi
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>
^ permalink raw reply [flat|nested] 10+ messages in thread* Re: [PATCH 2/2] ata: libata-core: Improve ata_dev_power_set_active()
2023-10-12 7:18 ` [PATCH 2/2] ata: libata-core: Improve ata_dev_power_set_active() Damien Le Moal
2023-10-12 11:07 ` Niklas Cassel
2023-10-13 8:51 ` Niklas Cassel
@ 2023-10-13 15:14 ` Phillip Susi
2023-10-13 20:12 ` Niklas Cassel
2023-10-15 22:12 ` Damien Le Moal
2 siblings, 2 replies; 10+ messages in thread
From: Phillip Susi @ 2023-10-13 15:14 UTC (permalink / raw)
To: Damien Le Moal, linux-ide; +Cc: Geert Uytterhoeven
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.
^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [PATCH 2/2] ata: libata-core: Improve ata_dev_power_set_active()
2023-10-13 15:14 ` Phillip Susi
@ 2023-10-13 20:12 ` Niklas Cassel
2023-10-15 22:12 ` Damien Le Moal
1 sibling, 0 replies; 10+ messages in thread
From: Niklas Cassel @ 2023-10-13 20:12 UTC (permalink / raw)
To: Phillip Susi
Cc: Damien Le Moal, linux-ide@vger.kernel.org, Geert Uytterhoeven
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
^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [PATCH 2/2] ata: libata-core: Improve ata_dev_power_set_active()
2023-10-13 15:14 ` Phillip Susi
2023-10-13 20:12 ` Niklas Cassel
@ 2023-10-15 22:12 ` Damien Le Moal
1 sibling, 0 replies; 10+ messages in thread
From: Damien Le Moal @ 2023-10-15 22:12 UTC (permalink / raw)
To: Phillip Susi, linux-ide; +Cc: Geert Uytterhoeven
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
^ permalink raw reply [flat|nested] 10+ messages in thread