| Message ID | 20231012071800.468868-3-dlemoal@kernel.org |
|---|---|
| State | New |
| Headers | show |
| Series | Improve drive spinup on resume | expand |
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
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>
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.
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
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.
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);
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(+)