From: Niklas Cassel <Niklas.Cassel@wdc.com>
To: Damien Le Moal <dlemoal@kernel.org>
Cc: "linux-ide@vger.kernel.org" <linux-ide@vger.kernel.org>,
Geert Uytterhoeven <geert@linux-m68k.org>,
Phillip Susi <phill@thesusis.net>
Subject: Re: [PATCH 2/2] ata: libata-core: Improve ata_dev_power_set_active()
Date: Thu, 12 Oct 2023 11:07:57 +0000 [thread overview]
Message-ID: <ZSfTjEBKb3t2HS7f@x1-carbon> (raw)
In-Reply-To: <20231012071800.468868-3-dlemoal@kernel.org>
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
next prev parent reply other threads:[~2023-10-12 11:08 UTC|newest]
Thread overview: 10+ messages / expand[flat|nested] mbox.gz Atom feed top
2023-10-12 7:17 [PATCH 0/2] Improve drive spinup on resume Damien Le Moal
2023-10-12 7:17 ` [PATCH 1/2] ata: libata-eh: Spinup disk on resume after revalidation Damien Le Moal
2023-10-12 11:03 ` Niklas Cassel
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 [this message]
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
2023-10-12 11:47 ` [PATCH 0/2] Improve drive spinup on resume Geert Uytterhoeven
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=ZSfTjEBKb3t2HS7f@x1-carbon \
--to=niklas.cassel@wdc.com \
--cc=dlemoal@kernel.org \
--cc=geert@linux-m68k.org \
--cc=linux-ide@vger.kernel.org \
--cc=phill@thesusis.net \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for read-only IMAP folder(s) and NNTP newsgroup(s).