Skip to content

Commit

Permalink
ata: libata-eh: avoid needless hard reset when revalidating link
Browse files Browse the repository at this point in the history
Performing a revalidation on a AHCI controller supporting LPM,
while using a lpm mode of e.g. med_power_with_dip (hipm + dipm) or
medium_power (hipm), will currently always lead to a hard reset.

The expected behavior is that a hard reset is only performed when
revalidate fails, because the properties of the drive has changed.

A revalidate performed after e.g. a NCQ error, or such a simple thing
as disabling write-caching (hdparm -W 0 /dev/sda), should succeed on
the first try (and should therefore not cause the link to be reset).

This unwarranted hard reset happens because ata_phys_link_offline()
returns true for a link that is in deep sleep. Thus the call to
ata_phys_link_offline() in ata_eh_revalidate_and_attach() will cause
the revalidation to fail, which causes ata_eh_handle_dev_fail() to be
called, which will set ehc->i.action |= ATA_EH_RESET, such that the
link is reset before retrying revalidation.

When the link is reset, the link is reestablished, so when
ata_eh_revalidate_and_attach() is called the second time, directly
after the link has been reset, ata_phys_link_offline() will return
false, and the revalidation will succeed.

Looking at "8.3.1.3 HBA Initiated" in the AHCI 1.3.1 specification,
it is clear the when host software writes a new command to memory,
by setting a bit in the PxCI/PxSACT HBA port registers, the HBA will
automatically bring back the link before sending out the Command FIS.

However, simply reading a SCR (like ata_phys_link_offline() does),
will not cause the HBA to automatically bring back the link.

As long as hipm is enabled, the HBA will put an idle link into deep
sleep. Avoid this needless hard reset on revalidation by temporarily
disabling hipm, by setting the LPM mode to ATA_LPM_MAX_POWER.

After revalidation is complete, ata_eh_recover() will restore the link
policy by setting the LPM mode to ap->target_lpm_policy.

Signed-off-by: Niklas Cassel <niklas.cassel@wdc.com>
Signed-off-by: Damien Le Moal <damien.lemoal@opensource.wdc.com>
  • Loading branch information
Niklas Cassel authored and Damien Le Moal committed Sep 24, 2022
1 parent e3b1fff commit 71d7b6e
Showing 1 changed file with 19 additions and 0 deletions.
19 changes: 19 additions & 0 deletions drivers/ata/libata-eh.c
Original file line number Diff line number Diff line change
Expand Up @@ -151,6 +151,8 @@ ata_eh_cmd_timeout_table[ATA_EH_CMD_TIMEOUT_TABLE_SIZE] = {
#undef CMDS

static void __ata_port_freeze(struct ata_port *ap);
static int ata_eh_set_lpm(struct ata_link *link, enum ata_lpm_policy policy,
struct ata_device **r_failed_dev);
#ifdef CONFIG_PM
static void ata_eh_handle_port_suspend(struct ata_port *ap);
static void ata_eh_handle_port_resume(struct ata_port *ap);
Expand Down Expand Up @@ -2934,6 +2936,23 @@ static int ata_eh_revalidate_and_attach(struct ata_link *link,
if ((action & ATA_EH_REVALIDATE) && ata_dev_enabled(dev)) {
WARN_ON(dev->class == ATA_DEV_PMP);

/*
* The link may be in a deep sleep, wake it up.
*
* If the link is in deep sleep, ata_phys_link_offline()
* will return true, causing the revalidation to fail,
* which leads to a (potentially) needless hard reset.
*
* ata_eh_recover() will later restore the link policy
* to ap->target_lpm_policy after revalidation is done.
*/
if (link->lpm_policy > ATA_LPM_MAX_POWER) {
rc = ata_eh_set_lpm(link, ATA_LPM_MAX_POWER,
r_failed_dev);
if (rc)
goto err;
}

if (ata_phys_link_offline(ata_dev_phys_link(dev))) {
rc = -EIO;
goto err;
Expand Down

0 comments on commit 71d7b6e

Please sign in to comment.