Skip to contentSkip to conversation

Power management fixes

PATCH Applied Damien Le Moal 5 participants 23 messages Jan 11, 2024 – Feb 26, 2024 4 messages not in the archive

Subsystems (2)
libata Regressions
lore
Share & tools
lore

Damien Le Moal

author maintainer
#0 lore raw

cover Power management fixes

The first patch is a fairly obvious fix. The second one needs testing.

Dieter, Zhihao,

Can you please try these patches to see if they resolve your issues with system low power state transitions ?

Damien Le Moal (2):
ata: libata-core: Do not try to set sleeping devices to standby
ata: libata-core: Revert "ata: libata-core: Fix

    ata_pci_shutdown_one()"
 drivers/ata/libata-core.c | 75 ++++++++++++++++++++++++---------------
 1 file changed, 47 insertions(+), 28 deletions(-)
Signature · 1 line
2.43.0
author maintainer
#1 lore raw

PATCH 1/2 ata: libata-core: Do not try to set sleeping devices to standby

In ata ata_dev_power_set_standby(), check that the target device is not sleeping. If it is, there is no need to do anything.

Fixes aa3998dbeb3a ("ata: libata-scsi: Disable scsi device manage_system_start_stop")
Cc [email protected]
Signed-off-by Damien Le Moal <[email protected]>
---
 drivers/ata/libata-core.c | 4 ++++
 1 file changed, 4 insertions(+)
drivers/ata/libata-core.c +4 -0
index 09ed67772fae..d9f80f4f70f5 100644
@@ -2017,6 +2017,10 @@ void ata_dev_power_set_standby(struct ata_device *dev)
 	struct ata_taskfile tf;
 	unsigned int err_mask;
 
+	/* If the device is already sleeping, do nothing. */
+	if (dev->flags & ATA_DFLAG_SLEEPING)
+		return;
+
 	/*
 	 * Some odd clown BIOSes issue spindown on power off (ACPI S4 or S5)
 	 * causing some drives to spin up and down again. For these, do nothing
Signature · 1 line
2.43.0
maintainer Applied
#2 lore raw

Re: [PATCH 1/2] ata: libata-core: Do not try to set sleeping devices to standby

Quoted 28 lines · Damien Le Moal

In ata ata_dev_power_set_standby(), check that the target device is not sleeping. If it is, there is no need to do anything.

Fixes aa3998dbeb3a ("ata: libata-scsi: Disable scsi device manage_system_start_stop")
Cc [email protected]
Signed-off-by Damien Le Moal <[email protected]>
---
 drivers/ata/libata-core.c | 4 ++++
 1 file changed, 4 insertions(+)
drivers/ata/libata-core.c +4 -0
index 09ed67772fae..d9f80f4f70f5 100644
@@ -2017,6 +2017,10 @@ void ata_dev_power_set_standby(struct ata_device *dev)
 	struct ata_taskfile tf;
 	unsigned int err_mask;
 
+	/* If the device is already sleeping, do nothing. */
+	if (dev->flags & ATA_DFLAG_SLEEPING)
+		return;
+
 	/*
 	 * Some odd clown BIOSes issue spindown on power off (ACPI S4 or S5)
 	 * causing some drives to spin up and down again. For these, do nothing
Signature · 1 line
2.43.0

Holding off [PATCH 2/2] until we get a better understanding of things, thus for [PATCH 1/2] only:

Applied:
https://git.kernel.org/pub/scm/linux/kernel/git/libata/linux.git/log/?h=for-6.8-fixes

author maintainer
#3 lore raw

PATCH 2/2 ata: libata-core: Revert "ata: libata-core: Fix ata_pci_shutdown_one()"

In reply to Damien Le Moal

This reverts commit fd3a6837d8e18cb7be80dcca1283276290336a7a.

Several users have signaled issues with commit fd3a6837d8e1 ("ata:
libata-core: Fix ata_pci_shutdown_one()") which causes failure of the system SoC to go to a low power state. The reason for this problem is not well understood but given that this patch is harmless with the improvements to ata_dev_power_set_standby(), restore it to allow system lower power state transitions.

For regular system shutdown, ata_dev_power_set_standby() will be executed twice: once the scsi device is removed and another when ata_pci_shutdown_one() executes and EH completes unloading the devices. Make the second call to ata_dev_power_set_standby() do nothing by using ata_dev_power_is_active() and return if the device is already in standby.

Fixes fd3a6837d8e1 ("ata: libata-core: Fix ata_pci_shutdown_one()")
Cc [email protected]
Signed-off-by Damien Le Moal <[email protected]>
---
 drivers/ata/libata-core.c | 75 +++++++++++++++++++++++----------------
 1 file changed, 45 insertions(+), 30 deletions(-)
drivers/ata/libata-core.c +45 -30
index d9f80f4f70f5..20a366942626 100644
@@ -2001,6 +2001,33 @@ bool ata_dev_power_init_tf(struct ata_device *dev, struct ata_taskfile *tf,
 	return true;
 }
 
+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_standby - Set a device power mode to standby
  *	@dev: target device

@@ -2017,8 +2044,9 @@ void ata_dev_power_set_standby(struct ata_device *dev)
 	struct ata_taskfile tf;
 	unsigned int err_mask;
 
-	/* If the device is already sleeping, do nothing. */
-	if (dev->flags & ATA_DFLAG_SLEEPING)
+	/* If the device is already sleeping or in standby, do nothing. */
+	if ((dev->flags & ATA_DFLAG_SLEEPING) ||
+	    !ata_dev_power_is_active(dev))
 		return;
 
 	/*

@@ -2046,33 +2074,6 @@ 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

@@ -6184,10 +6185,24 @@ EXPORT_SYMBOL_GPL(ata_pci_remove_one);
 void ata_pci_shutdown_one(struct pci_dev *pdev)
 {
 	struct ata_host *host = pci_get_drvdata(pdev);
+	struct ata_port *ap;
+	unsigned long flags;
 	int i;
 
+	/* Tell EH to disable all devices */
 	for (i = 0; i < host->n_ports; i++) {
-		struct ata_port *ap = host->ports[i];
+		ap = host->ports[i];
+		spin_lock_irqsave(ap->lock, flags);
+		ap->pflags |= ATA_PFLAG_UNLOADING;
+		ata_port_schedule_eh(ap);
+		spin_unlock_irqrestore(ap->lock, flags);
+	}
+
+	for (i = 0; i < host->n_ports; i++) {
+		ap = host->ports[i];
+
+		/* Wait for EH to complete before freezing the port */
+		ata_port_wait_eh(ap);
 
 		ap->pflags |= ATA_PFLAG_FROZEN;
Signature · 1 line
2.43.0
#4 lore raw

Re: [PATCH 2/2] ata: libata-core: Revert "ata: libata-core: Fix ata_pci_shutdown_one()"

Quoted 28 lines · Damien Le Moal

This reverts commit fd3a6837d8e18cb7be80dcca1283276290336a7a.

Several users have signaled issues with commit fd3a6837d8e1 ("ata:
libata-core: Fix ata_pci_shutdown_one()") which causes failure of the system SoC to go to a low power state. The reason for this problem is not well understood but given that this patch is harmless with the improvements to ata_dev_power_set_standby(), restore it to allow system lower power state transitions.

For regular system shutdown, ata_dev_power_set_standby() will be executed twice: once the scsi device is removed and another when ata_pci_shutdown_one() executes and EH completes unloading the devices. Make the second call to ata_dev_power_set_standby() do nothing by using ata_dev_power_is_active() and return if the device is already in standby.

Fixes fd3a6837d8e1 ("ata: libata-core: Fix ata_pci_shutdown_one()")
Cc [email protected]
Signed-off-by Damien Le Moal <[email protected]>
---
 drivers/ata/libata-core.c | 75 +++++++++++++++++++++++----------------
 1 file changed, 45 insertions(+), 30 deletions(-)
drivers/ata/libata-core.c +0 -0
index d9f80f4f70f5..20a366942626 100644

drivers/ata/libata-core.c bool ata_dev_power_init_tf(struct ata_device *dev, struct ata_taskfile *tf, · line 2010

Quoted 10 lines
drivers/ata/libata-core.c +7 -0
@@ -2001,6 +2001,33 @@ bool ata_dev_power_init_tf(struct ata_device *dev, struct ata_taskfile *tf,
 	return true;
 }
 
+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;

Why set ATA_TFLAG_ISADDR, BTW? This command doesn't use any taskfile
regs but the device/head reg. Material for a fix, I guess... :-)

drivers/ata/libata-core.c bool ata_dev_power_init_tf(struct ata_device *dev, struct ata_taskfile *tf, · line 2013

Quoted 3 lines
drivers/ata/libata-core.c +3 -0
+	tf.protocol = ATA_PROT_NODATA;
+	tf.command = ATA_CMD_CHK_POWER;
+

[...]

MBR, Sergey

author maintainer
#5 lore raw

Re: [PATCH 2/2] ata: libata-core: Revert "ata: libata-core: Fix ata_pci_shutdown_one()"

Quoted 33 lines · Sergei Shtylyov
Quoted 31 lines · Damien Le Moal

This reverts commit fd3a6837d8e18cb7be80dcca1283276290336a7a.

Several users have signaled issues with commit fd3a6837d8e1 ("ata:
libata-core: Fix ata_pci_shutdown_one()") which causes failure of the system SoC to go to a low power state. The reason for this problem is not well understood but given that this patch is harmless with the improvements to ata_dev_power_set_standby(), restore it to allow system lower power state transitions.

For regular system shutdown, ata_dev_power_set_standby() will be executed twice: once the scsi device is removed and another when ata_pci_shutdown_one() executes and EH completes unloading the devices. Make the second call to ata_dev_power_set_standby() do nothing by using ata_dev_power_is_active() and return if the device is already in standby.

Fixes fd3a6837d8e1 ("ata: libata-core: Fix ata_pci_shutdown_one()")
Cc [email protected]
Signed-off-by Damien Le Moal <[email protected]>
---
 drivers/ata/libata-core.c | 75 +++++++++++++++++++++++----------------
 1 file changed, 45 insertions(+), 30 deletions(-)
drivers/ata/libata-core.c +0 -0
index d9f80f4f70f5..20a366942626 100644
@@ -2001,6 +2001,33 @@ bool ata_dev_power_init_tf(struct ata_device *dev, struct ata_taskfile *tf,
 	return true;
 }
Quoted 10 lines

drivers/ata/libata-core.c bool ata_dev_power_init_tf(struct ata_device *dev, struct ata_taskfile *tf, · line 2010

Quoted 7 lines
drivers/ata/libata-core.c +7 -0
+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;

Why set ATA_TFLAG_ISADDR, BTW? This command doesn't use any taskfile
regs but the device/head reg. Material for a fix, I guess... :-)

Good point. I will look into it. This code was moved from libata-scsi translation, so it has been like this for a very long time...

Quoted 7 lines

drivers/ata/libata-core.c bool ata_dev_power_init_tf(struct ata_device *dev, struct ata_taskfile *tf, · line 2013

Quoted 4 lines
drivers/ata/libata-core.c +3 -0
+	tf.protocol = ATA_PROT_NODATA;
+	tf.command = ATA_CMD_CHK_POWER;
+

[...]

MBR, Sergey

Signature · 2 lines
Damien Le Moal
Western Digital Research
maintainer
#6 lore raw

Re: [PATCH 2/2] ata: libata-core: Revert "ata: libata-core: Fix ata_pci_shutdown_one()"

In reply to Sergei Shtylyov

Hello Sergei,

Quoted 44 lines · Sergei Shtylyov
Quoted 28 lines · Damien Le Moal

This reverts commit fd3a6837d8e18cb7be80dcca1283276290336a7a.

Several users have signaled issues with commit fd3a6837d8e1 ("ata:
libata-core: Fix ata_pci_shutdown_one()") which causes failure of the system SoC to go to a low power state. The reason for this problem is not well understood but given that this patch is harmless with the improvements to ata_dev_power_set_standby(), restore it to allow system lower power state transitions.

For regular system shutdown, ata_dev_power_set_standby() will be executed twice: once the scsi device is removed and another when ata_pci_shutdown_one() executes and EH completes unloading the devices. Make the second call to ata_dev_power_set_standby() do nothing by using ata_dev_power_is_active() and return if the device is already in standby.

Fixes fd3a6837d8e1 ("ata: libata-core: Fix ata_pci_shutdown_one()")
Cc [email protected]
Signed-off-by Damien Le Moal <[email protected]>
---
 drivers/ata/libata-core.c | 75 +++++++++++++++++++++++----------------
 1 file changed, 45 insertions(+), 30 deletions(-)
drivers/ata/libata-core.c +0 -0
index d9f80f4f70f5..20a366942626 100644

drivers/ata/libata-core.c bool ata_dev_power_init_tf(struct ata_device *dev, struct ata_taskfile *tf, · line 2010

Quoted 10 lines
drivers/ata/libata-core.c +7 -0
@@ -2001,6 +2001,33 @@ bool ata_dev_power_init_tf(struct ata_device *dev, struct ata_taskfile *tf,
 	return true;
 }
 
+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;

Why set ATA_TFLAG_ISADDR, BTW? This command doesn't use any taskfile
regs but the device/head reg. Material for a fix, I guess... :-)

Quoted 4 lines
Quoted 3 lines
drivers/ata/libata-core.c +3 -0
+	tf.protocol = ATA_PROT_NODATA;
+	tf.command = ATA_CMD_CHK_POWER;
+

[...]

Looking at the definition of the flag:
https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/include/linux/libata.h?h=v6.8-rc5#n76

"enable r/w to nsect/lba regs"

This function does read from the nsect reg:
https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/drivers/ata/libata-core.c?h=v6.8-rc5#n2069

So I would prefer to keep it as it.

Basically the whole motto for libata right now:
"If it ain't broke, don't fix it."

Sure, if we look at:
https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/drivers/ata/libata-sff.c?h=v6.8-rc5#n343

it seems that flags ATA_TFLAG_ISADDR, ATA_TFLAG_LBA48, and ATA_TFLAG_DEVICE is used to "guard" if these regs should be written to the TF.

However, if we look at:
https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/drivers/ata/libata-sff.c?h=v6.8-rc5#n392

is seems that only flag ATA_TFLAG_LBA48 is used to "guard" if the regs should be read from the TF.

So it looks like the intention was that these flags should be used to guard if certain TF regs should be written or read, but in reality, only some of the flags are actually for guarding reads. (While all of the flags are used for guarding writes.)

Personally, I don't really see the point of using flags to guard writes to the host controller. Why would we want to skip writing certain TF regs? The struct ata_taskfile should be zero-initialized before filling it with a command, so why not always write all TF regs and remove these flags?

Anyway, why touch it now and risk introducing regressions on some old PATA hardware?

Kind regards,
Niklas

#7 lore raw

Re: [PATCH 2/2] ata: libata-core: Revert "ata: libata-core: Fix ata_pci_shutdown_one()"

On 2/19/24 6:29 PM, Niklas Cassel wrote:
[...]

Quoted 47 lines
Quoted 42 lines
Quoted 28 lines

This reverts commit fd3a6837d8e18cb7be80dcca1283276290336a7a.

Several users have signaled issues with commit fd3a6837d8e1 ("ata:
libata-core: Fix ata_pci_shutdown_one()") which causes failure of the system SoC to go to a low power state. The reason for this problem is not well understood but given that this patch is harmless with the improvements to ata_dev_power_set_standby(), restore it to allow system lower power state transitions.

For regular system shutdown, ata_dev_power_set_standby() will be executed twice: once the scsi device is removed and another when ata_pci_shutdown_one() executes and EH completes unloading the devices. Make the second call to ata_dev_power_set_standby() do nothing by using ata_dev_power_is_active() and return if the device is already in standby.

Fixes fd3a6837d8e1 ("ata: libata-core: Fix ata_pci_shutdown_one()")
Cc [email protected]
Signed-off-by Damien Le Moal <[email protected]>
---
 drivers/ata/libata-core.c | 75 +++++++++++++++++++++++----------------
 1 file changed, 45 insertions(+), 30 deletions(-)
drivers/ata/libata-core.c +0 -0
index d9f80f4f70f5..20a366942626 100644

drivers/ata/libata-core.c bool ata_dev_power_init_tf(struct ata_device *dev, struct ata_taskfile *tf, · line 2010

Quoted 10 lines
drivers/ata/libata-core.c +7 -0
@@ -2001,6 +2001,33 @@ bool ata_dev_power_init_tf(struct ata_device *dev, struct ata_taskfile *tf,
 	return true;
 }
 
+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;

Why set ATA_TFLAG_ISADDR, BTW? This command doesn't use any taskfile
regs but the device/head reg. Material for a fix, I guess... :-)

Quoted 4 lines
Quoted 3 lines
drivers/ata/libata-core.c +3 -0
+	tf.protocol = ATA_PROT_NODATA;
+	tf.command = ATA_CMD_CHK_POWER;
+

[...]

Quoted 4 lines

Looking at the definition of the flag:
https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/include/linux/libata.h?h=v6.8-rc5#n76

"enable r/w to nsect/lba regs"

I'm afraid this comment doesn't reflect the reality in its r/w part --
if you look at e.g. ata_sff_tf_read(), you'll see that it always reads all the legacy taskfile and only checks ATA_TFLAG_LBA48 in order to determine whether it should read the HOBs as well...

Quoted 4 lines

This function does read from the nsect reg:
https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/drivers/ata/libata-core.c?h=v6.8-rc5#n2069

So I would prefer to keep it as it.

IMO, it doesn't make much sense -- unless you assume that the device
could leave that reg unset as a result of this command...

Quoted 2 lines

Basically the whole motto for libata right now:
"If it ain't broke, don't fix it."

Do you realize that each taskfile reg access takes e.g. 900-990 ns on
the Intel PIIX/ICH (the part # was 82371/82801) IDE controllers (with 33 MHz PCI bus)? Luckily, we just have to write (almost?) whole taskfile on the read/write commands anyway...

Quoted 7 lines

Sure, if we look at:
https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/drivers/ata/libata-sff.c?h=v6.8-rc5#n343

it seems that flags ATA_TFLAG_ISADDR, ATA_TFLAG_LBA48, and ATA_TFLAG_DEVICE is used to "guard" if these regs should be written to the TF.

Quoted 5 lines

However, if we look at:
https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/drivers/ata/libata-sff.c?h=v6.8-rc5#n392

is seems that only flag ATA_TFLAG_LBA48 is used to "guard" if the regs should be read from the TF.

Luckily, we have to read back the whole taskfile only on the read/write
errors...

Quoted 4 lines

So it looks like the intention was that these flags should be used to guard if certain TF regs should be written or read, but in reality, only some of the flags are actually for guarding reads. (While all of the flags are used for guarding writes.)

So you're seeing that inconsistency (I mentioned) yourself... :-)

Quoted 4 lines

Personally, I don't really see the point of using flags to guard writes to the host controller. Why would we want to skip writing certain TF regs?

The struct ata_taskfile should be zero-initialized before filling it with

TBH, I generally hate how libata implemented the taskfile accessors
and I hate how *struct* ata_taskfile looks too... :-)

Quoted 1 line

a command, so why not always write all TF regs and remove these flags?

To stop wasting a good microsecond per a reg R/W cycle, perhaps? :-) Anyway, the ATA standards clearly describe what the regs are used by
each command and what to expect on a normal/erratic command completion...

In drivers/ide/ we finanally ended up with 8-bit reg validity flags,
each bit corresponding to an individual taskfile reg:

https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/commit/?id=60f85019c6c8c1aebf3485a313e0da094bc95d07

Quoted 2 lines

Anyway, why touch it now and risk introducing regressions on some old PATA hardware?

Do you realize that drivers/ide/ wasn't writing out the whole taskfile
when issuing this particular command since:

https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/commit/?id=d364c7f50b3bb6dc77259974038567b821e2cf0a

If there were regressions, we would have seen them a long time ago,
no? :-)

Quoted 2 lines

Kind regards,
Niklas

MBR, Sergey

maintainer
#8 lore raw

Re: [PATCH 2/2] ata: libata-core: Revert "ata: libata-core: Fix ata_pci_shutdown_one()"

Hello Sergey,

Quoted 53 lines · Sergey Shtylyov

On 2/19/24 6:29 PM, Niklas Cassel wrote:
[...]

Quoted 50 lines
Quoted 42 lines
Quoted 28 lines

This reverts commit fd3a6837d8e18cb7be80dcca1283276290336a7a.

Several users have signaled issues with commit fd3a6837d8e1 ("ata:
libata-core: Fix ata_pci_shutdown_one()") which causes failure of the system SoC to go to a low power state. The reason for this problem is not well understood but given that this patch is harmless with the improvements to ata_dev_power_set_standby(), restore it to allow system lower power state transitions.

For regular system shutdown, ata_dev_power_set_standby() will be executed twice: once the scsi device is removed and another when ata_pci_shutdown_one() executes and EH completes unloading the devices. Make the second call to ata_dev_power_set_standby() do nothing by using ata_dev_power_is_active() and return if the device is already in standby.

Fixes fd3a6837d8e1 ("ata: libata-core: Fix ata_pci_shutdown_one()")
Cc [email protected]
Signed-off-by Damien Le Moal <[email protected]>
---
 drivers/ata/libata-core.c | 75 +++++++++++++++++++++++----------------
 1 file changed, 45 insertions(+), 30 deletions(-)
drivers/ata/libata-core.c +0 -0
index d9f80f4f70f5..20a366942626 100644

drivers/ata/libata-core.c bool ata_dev_power_init_tf(struct ata_device *dev, struct ata_taskfile *tf, · line 2010

Quoted 10 lines
drivers/ata/libata-core.c +7 -0
@@ -2001,6 +2001,33 @@ bool ata_dev_power_init_tf(struct ata_device *dev, struct ata_taskfile *tf,
 	return true;
 }
 
+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;

Why set ATA_TFLAG_ISADDR, BTW? This command doesn't use any taskfile
regs but the device/head reg. Material for a fix, I guess... :-)

Quoted 4 lines
Quoted 3 lines
drivers/ata/libata-core.c +3 -0
+	tf.protocol = ATA_PROT_NODATA;
+	tf.command = ATA_CMD_CHK_POWER;
+

[...]

Looking at the definition of the flag:
https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/include/linux/libata.h?h=v6.8-rc5#n76

Quoted 6 lines
Quoted 1 line

"enable r/w to nsect/lba regs"

I'm afraid this comment doesn't reflect the reality in its r/w part --
if you look at e.g. ata_sff_tf_read(), you'll see that it always reads all the legacy taskfile and only checks ATA_TFLAG_LBA48 in order to determine whether it should read the HOBs as well...

Considering that you have experience with cleaning up a similar mess in drivers/ide, patches for drivers/ata which fixes the comment and the functions needlessly setting the ATA_TFLAG_DEVICE flag is more than welcome.

From a quick look, there appears to be quite a few functions (in addition to ata_dev_power_is_active()) which needlessly sets this flag.

Kind regards,
Niklas

A message that is not in the archive (sent off-list or not yet received): DU0P251MB082515FC8FE77424231B475CF4682@DU0P251MB0825.EURP251.PROD.OUTLOOK.COM

author maintainer
#10 lore raw
Quoted 1 line · 王 志豪

thanks a lot！I will take a try and reply you soon

Did you try the patches ?

Signature · 2 lines
Damien Le Moal
Western Digital Research

A message that is not in the archive (sent off-list or not yet received): trinity-0be6e8a8-e6d3-4d60-be0d-59592a9edd65-1706010022623@3c-app-webde-bap10

author maintainer
#12 lore raw
Quoted 4 lines · Dieter Mummenschanz

Damien,
sorry for getting back to you so late. So is this patch series just a revert or is it something new? Can I patch and test against 6.8-rc or should I use 6.7? Anyway I need at least a couple of days since I'm very busy ATM.

Yes, the second patch is essentially a revert. If you can test with 6.8-rc1 it would be great.

Thanks.

Signature · 2 lines
Damien Le Moal
Western Digital Research

A message that is not in the archive (sent off-list or not yet received): trinity-0df92d73-be55-433c-bdb2-4387f7ea590b-1706686178879@3c-app-webde-bap43

author maintainer
#14 lore raw
Quoted 7 lines · Dieter Mummenschanz

Damien,
so I've applied the patch to 6.8-rc2. Interesting thing is that the behaviour is exactly the same as before (w/o the patch). Besides not
honoring CONFIG_SATA_MOBILE_LPM_POLICY=3 after boot my system refuses to transition into lower power states > pc2 after resume even after letting it sit idle for 10 minutes. Transition is only reached after issuing hdparm -Y. So if the patch restores the original behaviour then why did it stop working?!

Interesting... hdparm -Y puts the drive to sleep while the revert puts the drive in standby state. Sleep is a near complete shutdown of the drive, while standby is not.

In any case, I think something else is causing this. Probably PCI or ACPI related changes. So a bisect would be needed to understand this. But this is going to be very painful to do for this as each test take a while I guess ? How long do you need to wait to see the system going into low power state (when it is working that is) ?

Also, which kernel version is the last one you know is OK ?

Quoted 22 lines

dmesg:
https://pastes.io/1vmmvhvfub
Regards
Dieter
*Gesendet:* Dienstag, 23. Januar 2024 um 12:52 Uhr
*Von:* "Damien Le Moal" <[email protected]>
*An:* "Dieter Mummenschanz" <[email protected]>, [email protected] *Betreff:* Re: Aw: Re: [PATCH 0/2] Power management fixes

Quoted 4 lines · Dieter Mummenschanz

Damien,
sorry for getting back to you so late. So is this patch series just a revert or is it something new? Can I patch and test against 6.8-rc or should I use 6.7? Anyway I need at least a couple of days since I'm very busy ATM.

Yes, the second patch is essentially a revert. If you can test with 6.8-rc1 it would be great.

Thanks.

--
Damien Le Moal
Western Digital Research

Signature · 2 lines
Damien Le Moal
Western Digital Research
maintainer
#15 lore raw
Quoted 8 lines · Damien Le Moal
Quoted 7 lines · Dieter Mummenschanz

Damien,
so I've applied the patch to 6.8-rc2. Interesting thing is that the behaviour is exactly the same as before (w/o the patch). Besides not
honoring CONFIG_SATA_MOBILE_LPM_POLICY=3 after boot my system refuses to transition into lower power states > pc2 after resume even after letting it sit idle for 10 minutes. Transition is only reached after issuing hdparm -Y. So if the patch restores the original behaviour then why did it stop working?!

Hello Dieter,

Just to confirm,
while testing Damien's patch on top of 6.8-rc2,
did you still do:

for foo in /sys/class/scsi_host/host*/link_power_management_policy;
do echo med_power_with_dipm > $foo;
done

It should be needed until we add (or modify the existing entry, if any) your PCI vendor and device id to use "board_ahci_low_power" instead of "board_ahci", see e.g.
https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/commit/drivers/ata/ahci.c?id=b8b8b4e0c052b2c06e1c4820a8001f4e0f77900f

Kind regards,
Niklas

author maintainer
#16 lore raw
Quoted 16 lines · Niklas Cassel
Quoted 8 lines · Damien Le Moal
Quoted 7 lines · Dieter Mummenschanz

Damien,
so I've applied the patch to 6.8-rc2. Interesting thing is that the behaviour is exactly the same as before (w/o the patch). Besides not
honoring CONFIG_SATA_MOBILE_LPM_POLICY=3 after boot my system refuses to transition into lower power states > pc2 after resume even after letting it sit idle for 10 minutes. Transition is only reached after issuing hdparm -Y. So if the patch restores the original behaviour then why did it stop working?!

Hello Dieter,

Just to confirm,
while testing Damien's patch on top of 6.8-rc2,
did you still do:

Quoted 8 lines

for foo in /sys/class/scsi_host/host*/link_power_management_policy;
do echo med_power_with_dipm > $foo;
done

It should be needed until we add (or modify the existing entry, if any) your PCI vendor and device id to use "board_ahci_low_power" instead of "board_ahci", see e.g.
https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/commit/drivers/ata/ahci.c?id=b8b8b4e0c052b2c06e1c4820a8001f4e0f77900f

Indeed. What is the AHCI adapter here ?
It would be interesting if it is a Tiger Lake :)

Signature · 2 lines
Damien Le Moal
Western Digital Research
#17 lore raw
Quoted 3 lines

Indeed. What is the AHCI adapter here ?
It would be interesting if it is a Tiger Lake :)

No It's an older Coffeelake/Cannonlake Platform.

lspci -v |grep AHCI shows:

00:17.0 SATA controller: Intel Corporation Cannon Lake PCH SATA AHCI Controller (rev 10) (prog-if 01 [AHCI 1.0])
Subsystem: CLEVO/KAPOK Computer Cannon Lake PCH SATA AHCI Controller

author maintainer
#18 lore raw
Quoted 10 lines · Dieter Mummenschanz
Quoted 3 lines

Indeed. What is the AHCI adapter here ?
It would be interesting if it is a Tiger Lake :)

No It's an older Coffeelake/Cannonlake Platform.

lspci -v |grep AHCI shows:

00:17.0 SATA controller: Intel Corporation Cannon Lake PCH SATA AHCI Controller (rev 10) (prog-if 01 [AHCI 1.0])
Subsystem: CLEVO/KAPOK Computer Cannon Lake PCH SATA AHCI Controller

Thanks. Can you send the PCI-IDs too please ? (lspci -n -s 00:17.0)

Signature · 2 lines
Damien Le Moal
Western Digital Research
#19 lore raw
In reply to Niklas Cassel

Hi Niklas,

Quoted 7 lines

Just to confirm,
while testing Damien's patch on top of 6.8-rc2,
did you still do:

for foo in /sys/class/scsi_host/host*/link_power_management_policy; do echo med_power_with_dipm > $foo;
done

YES! This I always have to do this on this laptop in order to get transitions.

Quoted 4 lines

It should be needed until we add (or modify the existing entry, if any)
your PCI vendor and device id to use "board_ahci_low_power" instead of
"board_ahci", see e.g.
https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/commit/drivers/ata/ahci.c?> id=b8b8b4e0c052b2c06e1c4820a8001f4e0f77900f

Not sure I understand. So this CAN be fined inside the Kernel?

Kind regards,
Dieter

maintainer
#20 lore raw
Quoted 6 lines · Dieter Mummenschanz
Quoted 4 lines

It should be needed until we add (or modify the existing entry, if any)
your PCI vendor and device id to use "board_ahci_low_power" instead of
"board_ahci", see e.g.
https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/commit/drivers/ata/ahci.c?> id=b8b8b4e0c052b2c06e1c4820a8001f4e0f77900f

Not sure I understand. So this CAN be fined inside the Kernel?

Yes, this part of the problem can be avoided by adding an explicit entry that uses "board_ahci_low_power", as I explained in:
https://lore.kernel.org/linux-ide/ZaATdGDOo5jiBqCR@x1-carbon/T/#u

We seem to have a problem with Tiger Lake, but that problem seems to be related to Intel VMD.

From looking at your logs, you don't seem to have Intel VMD, however I'm guessing that some other motherboards that uses Cannon Lake might have Intel VMD, so I guess the safest thing is to wait until that issue has been resolved before adding a "board_ahci_low_power" entry for Cannon Lake.

However, we are still confused why the revert in this series did not enable your system to enter deeper power states, as you have previously confirmed that reverting this change:
https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/diff/drivers/ata/libata-core.c?id=d035e4eb38b3ea5ae9ead342f888fd3c394b0fe0
allowed you to enter deeper power states once again.

I see now that Damien's revert (patch 2/2 in this series) is not a simple

$ git revert fd3a6837d8e18cb7be80dcca1283276290336a7a
it seems to have some other small changes in the same patch as well.

Sorry for asking you to test something once more... But could you please test with:
v6.6-rc2 + git revert fd3a6837d8e18cb7be80dcca1283276290336a7a

Kind regards,
Niklas

#21 lore raw
Quoted 5 lines

Yes, this part of the problem can be avoided by adding an explicit entry
that uses "board_ahci_low_power", as I explained in:
https://lore.kernel.org/linux-ide/ZaATdGDOo5jiBqCR@x1-carbon/T/#u[https://lore.kernel.org/linux-> ide/ZaATdGDOo5jiBqCR@x1-carbon/T/#u]

Quoted 8 lines

We seem to have a problem with Tiger Lake, but that problem seems to be related to Intel VMD.

From looking at your logs, you don't seem to have Intel VMD, however I'm guessing that some other motherboards that uses Cannon Lake might have Intel VMD, so I guess the safest thing is to wait until that issue has been resolved before adding a "board_ahci_low_power" entry for Cannon Lake.

This mighht be kind of naive but what about adding an kernel option to force LPM even if the chipset/board is not oficially supported?

Quoted 7 lines

I see now that Damien's revert (patch 2/2 in this series) is not a simple

$ git revert fd3a6837d8e18cb7be80dcca1283276290336a7a
it seems to have some other small changes in the same patch as well.

Sorry for asking you to test something once more... But could you please test with:
v6.6-rc2 + git revert fd3a6837d8e18cb7be80dcca1283276290336a7a

No problem. I've explicitly reverted with 6.8-rc2 and have the kernel running for 1 1/2 days now with 3 suspends and on all of them my system transitions to pc8 so I guess we're good but I'll keep on testing to be sure. Hope this helps.

Kind regards,
Dieter

maintainer
#22 lore raw

Hello Dieter,

Quoted 8 lines · Dieter Mummenschanz
Quoted 7 lines

Yes, this part of the problem can be avoided by adding an explicit entry
that uses "board_ahci_low_power", as I explained in:
https://lore.kernel.org/linux-ide/ZaATdGDOo5jiBqCR@x1-carbon/T/#u[https://lore.kernel.org/linux-> ide/ZaATdGDOo5jiBqCR@x1-carbon/T/#u]

We seem to have a problem with Tiger Lake, but that problem seems to be related to Intel VMD.

Quoted 7 lines
Quoted 5 lines

From looking at your logs, you don't seem to have Intel VMD, however I'm guessing that some other motherboards that uses Cannon Lake might have Intel VMD, so I guess the safest thing is to wait until that issue has been resolved before adding a "board_ahci_low_power" entry for Cannon Lake.

This mighht be kind of naive but what about adding an kernel option to force LPM even if the chipset/board is not oficially supported?

That is a good suggestion.
However, I'm hoping that this series:
https://lore.kernel.org/linux-ide/[email protected]/T/

Will avoid the need for that, as it would kill the "board_ahci_low_power" board type completely.

Quoted 10 lines
Quoted 8 lines

I see now that Damien's revert (patch 2/2 in this series) is not a simple

$ git revert fd3a6837d8e18cb7be80dcca1283276290336a7a
it seems to have some other small changes in the same patch as well.

Sorry for asking you to test something once more... But could you please test with:
v6.6-rc2 + git revert fd3a6837d8e18cb7be80dcca1283276290336a7a

No problem. I've explicitly reverted with 6.8-rc2 and have the kernel running for 1 1/2 days now with 3 suspends and on all of them my system transitions to pc8 so I guess we're good but I'll keep on testing to be sure. Hope this helps.

After talking with Damien, we are still completely lost.

commit fd3a6837d8e1 ("ata: libata-core: Fix ata_pci_shutdown_one()") only modifies the ata_pci_shutdown_one(), which is only called on shutdown.

It is not called in the system suspend / system resume path, so it should not be possible for this commit to have an impact.

Reading all the emails and bugzilla again, what you are seeing is that:

On a fresh boot, and leaving the computer untouched for a few minutes, and looking at e.g. turbostat, some of your CPUs have spent time in the lower-power C-states, e.g. C8/C9/C10.

However, if you do a system suspend + system resume, and then leave the computer untouched for a few minutes, and look at e.g. turbotstat, _none_ of your CPUs have spend time in the lower-power C-states.

Is this correct?

If so, it seems that suspend/resume must have messed up some setting...

The commit that added the code to ata_pci_shutdown_one() was added in:
5b6fba546da2 ("ata: libata-core: Detach a port devices on shutdown") This was first added in v6.7-rc1.
(So this commit was never included in v6.6.)

The code added in 5b6fba546da2 was later removed in:
commit fd3a6837d8e1 ("ata: libata-core: Fix ata_pci_shutdown_one()") which was first added in v6.7-rc1.

Are you certain that v6.6 works and v6.7 is bad?

There are quite few libata patches between v6.6 and v6.7:

$ git log --oneline v6.7 --not v6.6 -- drivers/ata

Damien has sent many libata patches related to suspend/resume in the last couple of kernel releases, so it is possible that something has regressed. We just have a really hard time seeing how fd3a6837d8e1 could be the bad commit.

Kind regards,
Niklas

A message that is not in the archive (sent off-list or not yet received): trinity-0bc8e6ea-7808-4508-af3a-be22281abf24-1707231996854@3c-app-webde-bs42

maintainer
#24 lore raw

Hello Dieter,

Quoted 10 lines · Dieter Mummenschanz

Hello Niklas,

Quoted 6 lines

That is a good suggestion.
However, I'm hoping that this series:
https://lore.kernel.org/linux-ide/[email protected]/T/[https://lore.kernel.org/linux-> ide/[email protected]/T/]

Will avoid the need for that, as it would kill the "board_ahci_low_power" board type completely.

Very nice. If there is anything "baked" enough to test, please let me know.

It should be ready for testing:
https://github.com/floatious/linux/tree/external-port-v2

With this, you should no longer need:
for foo in /sys/class/scsi_host/host*/link_power_management_policy;
do echo med_power_with_dipm > $foo;
done

(Assuming you have compiled your kernel with CONFIG_SATA_MOBILE_LPM_POLICY=3).

Quoted 9 lines
Quoted 7 lines

After talking with Damien, we are still completely lost.
commit fd3a6837d8e1 ("ata: libata-core: Fix ata_pci_shutdown_one()") only modifies the ata_pci_shutdown_one(), which is only called on shutdown.

It is not called in the system suspend / system resume path, so it should not be possible for this commit to have an impact.

I don't understand it either. For what it's worth, this morning after resuming, my system was stuck at pc2 for 10+ Minutes. After issuing hdparm -Y /dev/sdb I saw lower transitions down to pc8 right away without(!) issuing the same command for sda. So maybe fd3a6837d8e1 isn't the culprit after all. I'll keep testing.

You shouldn't need to do:

$ hdparm -Y

As that bypasses some of the internal logic in libata.

I assume that you didn't need this on v6.6 and older?

(Instead libata should put the device to standby or sleep itself, it shouldn't need to be done explicitly by the user.)

Quoted 26 lines
Quoted 7 lines

Reading all the emails and bugzilla again, what you are seeing is that:

On a fresh boot, and leaving the computer untouched for a few minutes, and looking at e.g. turbostat, some of your CPUs have spent time in the lower-power C-states, e.g. C8/C9/C10.

Correct! Btw. I'm looking at powertop and also have a daemon that feeds the current c-states to conky for display so I have an eye on the c-states at all times.

Quoted 5 lines

However, if you do a system suspend + system resume, and then leave the computer untouched for a few minutes, and look at e.g. turbotstat, _none_ of your CPUs have spend time in the lower-power C-states.

Is this correct?

Yes! The system spends ~90% in pc2 no lower c-states.

Quoted 7 lines

If so, it seems that suspend/resume must have messed up some setting...

The commit that added the code to ata_pci_shutdown_one() was added in:
5b6fba546da2 ("ata: libata-core: Detach a port devices on shutdown") This was first added in v6.7-rc1.
(So this commit was never included in v6.6.)

Quoted 7 lines
Quoted 5 lines

The code added in 5b6fba546da2 was later removed in:
commit fd3a6837d8e1 ("ata: libata-core: Fix ata_pci_shutdown_one()") which was first added in v6.7-rc1.

Are you certain that v6.6 works and v6.7 is bad?

Yes, absolutely!

How certain are you that v6.6 works?

Could it be the same problem there, that it works sometimes and doesn't work sometimes?

The reason why I'm asking is that Damien's major changes got included in v6.6-rc4:
https://lore.kernel.org/linux-ide/[email protected]/

So I would be less surprised if you said that you can enter pc8 on every boot on v6.5, but on v6.6 it only works occasionally.

But if you can enter pc8 on every boot on v6.6, but not on v6.7, then it is probably easier to figure out which commit that broke things, as there were not that many suspend/resume related changes added in v6.7.

Kind regards,
Niklas

#25 lore raw

Hello Niklas,

Quoted 9 lines

It should be ready for testing:
https://github.com/floatious/linux/tree/external-port-v2[https://github.com/floatious/linux/tree/external-port-v2]

With this, you should no longer need:
for foo in /sys/class/scsi_host/host*/link_power_management_policy; do echo med_power_with_dipm > $foo;
done

(Assuming you have compiled your kernel with CONFIG_SATA_MOBILE_LPM_POLICY=3).

Oh sweet! Will give it a spin over the weekend if I find the time.

Quoted 9 lines

You shouldn't need to do:

$ hdparm -Y

As that bypasses some of the internal logic in libata.

I assume that you didn't need this on v6.6 and older?

(Instead libata should put the device to standby or sleep itself, it shouldn't need to be done explicitly by the user.)

I'm at a loss either. No idea why this is actually working. When I encountered the low-power issue I searched the web and stumbled upon hdparm -Y and gave it a shot.

Quoted 1 line

How certain are you that v6.6 works?

Let's put it this way: I've installed most 6.6-rc's so I had it running for several weeks and I always have an eye on conky showing me the c-states in 5 second interval. And during all that time and several suspends it never occured.

It definately started with the first 6.7 -rc I've installed (don't recall wich one first). There were no hardware changes and no BIOS updates during that time. If I find some spare time, I'm gonna give the latest 6.6 from kernel.org a spin to verify.

Quoted 2 lines

Could it be the same problem there, that it works sometimes and doesn't work sometimes?

Again I've never encountered this issue with 6.6 or any other kernel before. I would have remembered that.

Quoted 6 lines

The reason why I'm asking is that Damien's major changes got included
in v6.6-rc4:
https://lore.kernel.org/linux-ide/[email protected]/[https://lore.kernel.org/linux-> ide/[email protected]/]

So I would be less surprised if you said that you can enter pc8 on every boot on v6.5, but on v6.6 it only works occasionally.

To be sure I would have to switch back to 6.6. If I find the time I'll give it a shot.

Quoted 4 lines

But if you can enter pc8 on every boot on v6.6, but not on v6.7, then it is probably easier to figure out which commit that broke things, as there were not that many suspend/resume related changes added in v6.7.

Maybe you could point them out so I can try to bisect?

Kind regards,
Dieter

maintainer
#26 lore raw

Hello Dieter,

Quoted 6 lines · Dieter Mummenschanz
Quoted 4 lines

I assume that you didn't need this on v6.6 and older?

(Instead libata should put the device to standby or sleep itself, it shouldn't need to be done explicitly by the user.)

I'm at a loss either. No idea why this is actually working. When I encountered the low-power issue I searched the web and stumbled upon hdparm -Y and gave it a shot.

Are you saying that you always do this before suspending? Both on v6.6 and v6.7?

Quoted 6 lines
Quoted 4 lines

But if you can enter pc8 on every boot on v6.6, but not on v6.7, then it is probably easier to figure out which commit that broke things, as there were not that many suspend/resume related changes added in v6.7.

Maybe you could point them out so I can try to bisect?

Sure.

These are the libata (and related SCSI) patches added in v6.7 (that are not in v6.6):

b09d7f8fd50f scsi: sd: Fix system start for ATA devices 6371be7aeb98 scsi: Change SCSI device boolean fields to single bit flags fd3a6837d8e1 ata: libata-core: Fix ata_pci_shutdown_one() 2da4c5e24e86 ata: libata-core: Improve ata_dev_power_set_active() 54d7211da7cd ata: libata-eh: Spinup disk on resume after revalidation 1b947279798f ata: libata: Cleanup inline DMA helper functions 0fecb50891aa ata: libata-eh: Reduce "disable device" message verbosity 7f95731c74d7 ata: libata-eh: Improve reset error messages 88b9f8928678 ata: libata-sata: Improve ata_sas_slave_configure() 3341b82368fb ata: libata-core: Do not resume runtime suspended ports 3a94af2488bf ata: libata-core: Do not poweroff runtime suspended ports 09b055cfb0e9 ata: libata-core: Remove ata_port_resume_async() 6702255d700a ata: libata-core: Remove ata_port_suspend_async() 5b6fba546da2 ata: libata-core: Detach a port devices on shutdown cfead0dd81de ata: libata-core: Synchronize ata_port_detach() with hotplug 8c1f08170694 ata: libata-scsi: Cleanup ata_scsi_start_stop_xlat() c4367ac83805 scsi: Remove scsi device no_start_on_resume flag

I would start with v6.6 and apply them one by one. (Starting with c4367ac83805)

However, it is also possible that the regression is completely unrelated to libata changes (e.g. caused by a ACPI suspend/resume patch), and in that case, you will need a complete bisection.

Kind regards,
Niklas