[PATCH 0/1] Only activate drive once during system resume

[PATCH 0/1] Only activate drive once during system resume

[Phillip Susi]

I have been wondering why I kept seeing drives activated twice
during system resume since this got added.  It seems that the EH
process can run up to 5 times as long as there is still pending
EH.  I am not sure why, but it look like my Intel AHCI controller
issues an error interrupt during the first round of EH, even
though there does not appear to be any error.  This causes
the EH_PENDING flag to be set again, and so a second round of
EH happens, which then tries to activate the disk a second time.

I propose the following patch to only set the ACTIVE flag once.

Phillip Susi (1):
  libata: only wake a drive once on system resume

 drivers/ata/libata-eh.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

-- 
2.30.2

[PATCH 1/1] libata: only wake a drive once on system resume

[Phillip Susi]

In the event that more than one pass of EH is needed during system resume,
only request the drive be started once.
---
 drivers/ata/libata-eh.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/ata/libata-eh.c b/drivers/ata/libata-eh.c
index 120f7d7fb450..97284b5b67d1 100644
--- a/drivers/ata/libata-eh.c
+++ b/drivers/ata/libata-eh.c
@@ -713,7 +713,7 @@ void ata_scsi_port_error_handler(struct Scsi_Host *host, struct ata_port *ap)
 				ehc->saved_ncq_enabled |= 1 << devno;
 
 			/* If we are resuming, wake up the device */
-			if (ap->pflags & ATA_PFLAG_RESUMING)
+			if (ap->eh_tries == ATA_EH_MAX_TRIES && ap->pflags & ATA_PFLAG_RESUMING)
 				ehc->i.dev_action[devno] |= ATA_EH_SET_ACTIVE;
 		}
 	}
-- 
2.30.2

[PATCH 0/1 v2] Only activate drive once during system resume

[Phillip Susi]

This version also works and may be a bit cleaner

Phillip Susi (1):
  libata: only wake a drive once on system resume

 drivers/ata/libata-eh.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

-- 
2.30.2

[PATCH 1/1] libata: only wake a drive once on system resume

[Phillip Susi]

In the event that more than one pass of EH is needed during system resume,
only request the drive be started once.
---
 drivers/ata/libata-core.c | 4 ++--
 drivers/ata/libata-eh.c   | 4 ----
 2 files changed, 2 insertions(+), 6 deletions(-)

diff --git a/drivers/ata/libata-core.c b/drivers/ata/libata-core.c
index d1389ce6943e..ca3ca8107a3e 100644
--- a/drivers/ata/libata-core.c
+++ b/drivers/ata/libata-core.c
@@ -5223,7 +5223,7 @@ static void ata_port_request_pm(struct ata_port *ap, pm_message_t mesg,
 	/* on system resume, don't wake PuiS drives */
 	if (mesg.event == PM_EVENT_RESUME)
 		ehi_flags |= ATA_EHI_NOSTART;
-	
+
 	/* Request PM operation to EH */
 	ap->pm_mesg = mesg;
 	ap->pflags |= ATA_PFLAG_PM_PENDING;
@@ -5297,7 +5297,7 @@ static int ata_port_pm_poweroff(struct device *dev)
 static void ata_port_resume(struct ata_port *ap, pm_message_t mesg,
 			    bool async)
 {
-	ata_port_request_pm(ap, mesg, ATA_EH_RESET,
+	ata_port_request_pm(ap, mesg, ATA_EH_RESET | ATA_EH_SET_ACTIVE,
 			    ATA_EHI_NO_AUTOPSY | ATA_EHI_QUIET,
 			    async);
 }
diff --git a/drivers/ata/libata-eh.c b/drivers/ata/libata-eh.c
index 120f7d7fb450..f44ce7068a8b 100644
--- a/drivers/ata/libata-eh.c
+++ b/drivers/ata/libata-eh.c
@@ -711,10 +711,6 @@ void ata_scsi_port_error_handler(struct Scsi_Host *host, struct ata_port *ap)
 			ehc->saved_xfer_mode[devno] = dev->xfer_mode;
 			if (ata_ncq_enabled(dev))
 				ehc->saved_ncq_enabled |= 1 << devno;
-
-			/* If we are resuming, wake up the device */
-			if (ap->pflags & ATA_PFLAG_RESUMING)
-				ehc->i.dev_action[devno] |= ATA_EH_SET_ACTIVE;
 		}
 	}
 
-- 
2.30.2

Re: [PATCH 1/1] libata: only wake a drive once on system resume

[Sergey Shtylyov]

On 12/30/23 9:21 PM, Phillip Susi wrote:

> In the event that more than one pass of EH is needed during system resume,
> only request the drive be started once.
> ---
>  drivers/ata/libata-core.c | 4 ++--
>  drivers/ata/libata-eh.c   | 4 ----
>  2 files changed, 2 insertions(+), 6 deletions(-)
> 
> diff --git a/drivers/ata/libata-core.c b/drivers/ata/libata-core.c
> index d1389ce6943e..ca3ca8107a3e 100644
> --- a/drivers/ata/libata-core.c
> +++ b/drivers/ata/libata-core.c
> @@ -5223,7 +5223,7 @@ static void ata_port_request_pm(struct ata_port *ap, pm_message_t mesg,
>  	/* on system resume, don't wake PuiS drives */
>  	if (mesg.event == PM_EVENT_RESUME)
>  		ehi_flags |= ATA_EHI_NOSTART;
> -	
> +

   Unrelated whitespace change? :-)

[...]

MBR, Sergey

Re: [PATCH 1/1] libata: only wake a drive once on system resume

[Damien Le Moal]

On 12/31/23 03:21, Phillip Susi wrote:
> In the event that more than one pass of EH is needed during system resume,
> only request the drive be started once.

This is not an improvement... What if the verify command that wakes-up the drive
fails to be issued, or EH does not reach the call to ata_dev_power_set_active()
on the first run ? You would want to retry it but your patch will prevent that.

I do not really see any fundamental issue here given that calling
ata_dev_power_set_active() is indeed useless if the drive is already active, but
that does not hurt either. The only overhead is issuing a check power mode
command (see the call to ata_dev_power_is_active() in ata_dev_power_set_active()).

Are you seeing different behavior with your system ? Any error ?

> ---
>  drivers/ata/libata-core.c | 4 ++--
>  drivers/ata/libata-eh.c   | 4 ----
>  2 files changed, 2 insertions(+), 6 deletions(-)
> 
> diff --git a/drivers/ata/libata-core.c b/drivers/ata/libata-core.c
> index d1389ce6943e..ca3ca8107a3e 100644
> --- a/drivers/ata/libata-core.c
> +++ b/drivers/ata/libata-core.c
> @@ -5223,7 +5223,7 @@ static void ata_port_request_pm(struct ata_port *ap, pm_message_t mesg,
>  	/* on system resume, don't wake PuiS drives */
>  	if (mesg.event == PM_EVENT_RESUME)
>  		ehi_flags |= ATA_EHI_NOSTART;
> -	
> +
>  	/* Request PM operation to EH */
>  	ap->pm_mesg = mesg;
>  	ap->pflags |= ATA_PFLAG_PM_PENDING;
> @@ -5297,7 +5297,7 @@ static int ata_port_pm_poweroff(struct device *dev)
>  static void ata_port_resume(struct ata_port *ap, pm_message_t mesg,
>  			    bool async)
>  {
> -	ata_port_request_pm(ap, mesg, ATA_EH_RESET,
> +	ata_port_request_pm(ap, mesg, ATA_EH_RESET | ATA_EH_SET_ACTIVE,
>  			    ATA_EHI_NO_AUTOPSY | ATA_EHI_QUIET,
>  			    async);
>  }
> diff --git a/drivers/ata/libata-eh.c b/drivers/ata/libata-eh.c
> index 120f7d7fb450..f44ce7068a8b 100644
> --- a/drivers/ata/libata-eh.c
> +++ b/drivers/ata/libata-eh.c
> @@ -711,10 +711,6 @@ void ata_scsi_port_error_handler(struct Scsi_Host *host, struct ata_port *ap)
>  			ehc->saved_xfer_mode[devno] = dev->xfer_mode;
>  			if (ata_ncq_enabled(dev))
>  				ehc->saved_ncq_enabled |= 1 << devno;
> -
> -			/* If we are resuming, wake up the device */
> -			if (ap->pflags & ATA_PFLAG_RESUMING)
> -				ehc->i.dev_action[devno] |= ATA_EH_SET_ACTIVE;
>  		}
>  	}
>  

-- 
Damien Le Moal
Western Digital Research

Re: [PATCH 1/1] libata: only wake a drive once on system resume

[Phillip Susi]

Damien Le Moal <dlemoal@kernel.org> writes:

> This is not an improvement... What if the verify command that wakes-up the drive
> fails to be issued, or EH does not reach the call to ata_dev_power_set_active()
> on the first run ? You would want to retry it but your patch will prevent that.

Perhaps if it fails, then it should set the flag to request it be tried
again?  As long as it succeeds, then there's no need to do it again?

> I do not really see any fundamental issue here given that calling
> ata_dev_power_set_active() is indeed useless if the drive is already active, but
> that does not hurt either. The only overhead is issuing a check power mode
> command (see the call to ata_dev_power_is_active() in ata_dev_power_set_active()).
>
> Are you seeing different behavior with your system ? Any error ?

My main issue with it was that it caused errors with my PuiS patch.  I
tried canceling the SET_ACTIVE flag when PuiS was detected, but then the
flag got turned back on for the second pass of EH, but not the flag for
revalidate_and_attach, so it didn't detect the PuiS and clear the
SET_ACTIVE flag the second time.  Since the SET FEATURES command was not
issued, the VERIFY command failed, and after the 5th attempt, eh gave
up.  Without PuiS, it would also be nice to not waste time with a second
VERIFY command.

Re: [PATCH 1/1] libata: only wake a drive once on system resume

[Damien Le Moal]

On 1/4/24 06:00, Phillip Susi wrote:
> Damien Le Moal <dlemoal@kernel.org> writes:
> 
>> This is not an improvement... What if the verify command that wakes-up the drive
>> fails to be issued, or EH does not reach the call to ata_dev_power_set_active()
>> on the first run ? You would want to retry it but your patch will prevent that.
> 
> Perhaps if it fails, then it should set the flag to request it be tried
> again?  As long as it succeeds, then there's no need to do it again?

Correct, if it succeeds, no need to do it again. The problem with clearing the
flag though is that ATA_EH_SET_ACTIVE is for the device and that is set only if
ATA_PFLAG_RESUMING is set, but that one is for the port. So if resume for the
device succeeds, you can clear the ATA_PFLAG_RESUMING flag *only* if there is
only a single link/device for that port. If not, other devices on the port may
need a retry so we cannot clear ATA_PFLAG_RESUMING.

> 
>> I do not really see any fundamental issue here given that calling
>> ata_dev_power_set_active() is indeed useless if the drive is already active, but
>> that does not hurt either. The only overhead is issuing a check power mode
>> command (see the call to ata_dev_power_is_active() in ata_dev_power_set_active()).
>>
>> Are you seeing different behavior with your system ? Any error ?
> 
> My main issue with it was that it caused errors with my PuiS patch.  I

Ah, now I think I understand: is it your patch that prevents resuming a drive if
it has PUIS on ? If yes, then sure, the verify command to spin-up the drive will
indeed fail immediately if the drive is in standby from PUIS, since getting out
of standby state driven by PUIS requires a set features spinup. So... with your
patch, things will not work.

> tried canceling the SET_ACTIVE flag when PuiS was detected, but then the
> flag got turned back on for the second pass of EH, but not the flag for
> revalidate_and_attach, so it didn't detect the PuiS and clear the
> SET_ACTIVE flag the second time.  Since the SET FEATURES command was not
> issued, the VERIFY command failed, and after the 5th attempt, eh gave
> up.  Without PuiS, it would also be nice to not waste time with a second
> VERIFY command.

If PUIS is not enabled, the only thing wasted is a check power mode command. If
the drive confirms that it is active, verify command is not issued.

With PUIS enabled though, if you leave the drive in standby mode, when/how do
you actually wake it up ?

Scratching my head about this, I think that doing this cleanly should be
possible if:
1) The dive gives complete identify data when that command is issued with the
drive in standby state driven by PUIS.
2) The call to ata_dev_configure() executed by ata EH started from system resume
does not spinup the device if requested not to (libata module & sysfs parameter
can do that). But I think this requires that the drive be instead put into a
state equivalent to *runtime* suspend, that is, with the scsi disk associated
with the device must also be in runtime suspend state.

With that, it would only be a matter of adding a device flag to remember that
the drive is in "PUIS stnadby" instead of regular standby, and then have
ata_dev_power_set_active() use a set feature spinup command instead of a verify.
Drive spinup will then be cleanly driven by accesses to the scsi disk, similarly
to a regular runtime suspend.

With such changes, everything would be cleaner and safer and all work as
expected. The exception will be drives that do not give complete identify data
when PUIS is on. For these, it is too risky to not wake them up to get the full
information first.

Do you want to try to tackle this ? If you do not feel like it, I can give it a
try too.

-- 
Damien Le Moal
Western Digital Research

Re: [PATCH 1/1] libata: only wake a drive once on system resume

[Phillip Susi]

Damien Le Moal <dlemoal@kernel.org> writes:

> Correct, if it succeeds, no need to do it again. The problem with clearing the
> flag though is that ATA_EH_SET_ACTIVE is for the device and that is set only if
> ATA_PFLAG_RESUMING is set, but that one is for the port. So if resume for the
> device succeeds, you can clear the ATA_PFLAG_RESUMING flag *only* if there is
> only a single link/device for that port. If not, other devices on the port may
> need a retry so we cannot clear ATA_PFLAG_RESUMING.

Rather than clear ATA_PFLAG_RESUMING, I was thinking of keeping my
previus change to specify ATA_EH_SET_ACTIVE in the request_pm path
rather than by setting it based on ATA_PFLAG_RESUMING, but just adding a
check to see if the VERIFY fails, and if so, set ATA_EH_SET_ACTIVE again.

> Ah, now I think I understand: is it your patch that prevents resuming a drive if
> it has PUIS on ? If yes, then sure, the verify command to spin-up the drive will
> indeed fail immediately if the drive is in standby from PUIS, since getting out
> of standby state driven by PUIS requires a set features spinup. So... with your
> patch, things will not work.

Right... unless you also apply this patch to make sure that
ATA_EH_SET_ACTIVE isn't turned on again after it is cleared when PuiS is detected.

> If PUIS is not enabled, the only thing wasted is a check power mode command. If
> the drive confirms that it is active, verify command is not issued.

Ahh, right... I forgot you did add a CHECK POWER MODE first.

> With PUIS enabled though, if you leave the drive in standby mode, when/how do
> you actually wake it up ?

Initially I just set the ATA_DFLAG_SLEEPING flag as if the drive had been
put into SLEEP mode, which triggers EH to wake it up when the drive is
accessed.  I have since switched to actually putting the drive to SLEEP
mode when PuiS is detected since it will save a little more power than
leaving it in STANDBY.

> Scratching my head about this, I think that doing this cleanly should be
> possible if:
> 1) The dive gives complete identify data when that command is issued with the
> drive in standby state driven by PUIS.

I think you can do it even if the IDENTIFY data is incomplete, as long
as a revalidate_and_attach is done eventually, when waking the drive up.

> 2) The call to ata_dev_configure() executed by ata EH started from system resume
> does not spinup the device if requested not to (libata module & sysfs parameter
> can do that). But I think this requires that the drive be instead put into a
> state equivalent to *runtime* suspend, that is, with the scsi disk associated
> with the device must also be in runtime suspend state.

Yea, I was trying to make it work with runtime suspend before, but you
indicated that the current device hierarchy seems to make that
impossible, so I put that aside for now.  Currently runtime pm thinks
the drive is running even though it isn't, but that isn't any different
than when you hdparm -y or hdparm -Y, or hdparm -S and let the drive
decide to auto standby.  Eventually I'll try to get the runtime pm
sorted but I figured I'd try to get the basic concept working first.

> With that, it would only be a matter of adding a device flag to remember that
> the drive is in "PUIS stnadby" instead of regular standby, and then have
> ata_dev_power_set_active() use a set feature spinup command instead of a verify.
> Drive spinup will then be cleanly driven by accesses to the scsi disk, similarly
> to a regular runtime suspend.

Right now I'm using the SLEEP flag to do this, and when the disk is
accessed, it triggers a round of EH that does the revalidate_and_attach
and in the process, issues the SET FEATURES command to wake the drive.

> With such changes, everything would be cleaner and safer and all work as
> expected. The exception will be drives that do not give complete identify data
> when PUIS is on. For these, it is too risky to not wake them up to get the full
> information first.
>
> Do you want to try to tackle this ? If you do not feel like it, I can give it a
> try too.

I've already got it working ;)

I think I sent you an earlier version of the patch a few weeks ago.
I'll post my whole series tonight, after I fix the case of retrying the
VERIFY if it fails.

[PATCH 1/4] libata: only wake a drive once on system resume

[Phillip Susi]

In the event that more than one pass of EH is needed during system resume,
only request the drive be started once.
---
 drivers/ata/libata-core.c | 9 +++++----
 drivers/ata/libata-eh.c   | 8 +++-----
 drivers/ata/libata.h      | 2 +-
 3 files changed, 9 insertions(+), 10 deletions(-)

diff --git a/drivers/ata/libata-core.c b/drivers/ata/libata-core.c
index 09ed67772fae..a2d8cc0097a8 100644
--- a/drivers/ata/libata-core.c
+++ b/drivers/ata/libata-core.c
@@ -2080,7 +2080,7 @@ static bool ata_dev_power_is_active(struct ata_device *dev)
  *	LOCKING:
  *	Kernel thread context (may sleep).
  */
-void ata_dev_power_set_active(struct ata_device *dev)
+unsigned int ata_dev_power_set_active(struct ata_device *dev)
 {
 	struct ata_taskfile tf;
 	unsigned int err_mask;
@@ -2090,14 +2090,14 @@ void ata_dev_power_set_active(struct ata_device *dev)
 	 * if supported by the device.
 	 */
 	if (!ata_dev_power_init_tf(dev, &tf, true))
-		return;
+		return AC_ERR_OTHER;
 
 	/*
 	 * 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;
+		return AC_ERR_OK;
 
 	ata_dev_notice(dev, "Entering active power mode\n");
 
@@ -2105,6 +2105,7 @@ void ata_dev_power_set_active(struct ata_device *dev)
 	if (err_mask)
 		ata_dev_err(dev, "VERIFY failed (err_mask=0x%x)\n",
 			    err_mask);
+	return err_mask;
 }
 
 /**
@@ -5257,7 +5258,7 @@ static int ata_port_pm_poweroff(struct device *dev)
 static void ata_port_resume(struct ata_port *ap, pm_message_t mesg,
 			    bool async)
 {
-	ata_port_request_pm(ap, mesg, ATA_EH_RESET,
+	ata_port_request_pm(ap, mesg, ATA_EH_RESET | ATA_EH_SET_ACTIVE,
 			    ATA_EHI_NO_AUTOPSY | ATA_EHI_QUIET,
 			    async);
 }
diff --git a/drivers/ata/libata-eh.c b/drivers/ata/libata-eh.c
index b0d6e69c4a5b..799a1b8bc384 100644
--- a/drivers/ata/libata-eh.c
+++ b/drivers/ata/libata-eh.c
@@ -710,10 +710,6 @@ void ata_scsi_port_error_handler(struct Scsi_Host *host, struct ata_port *ap)
 			ehc->saved_xfer_mode[devno] = dev->xfer_mode;
 			if (ata_ncq_enabled(dev))
 				ehc->saved_ncq_enabled |= 1 << devno;
-
-			/* If we are resuming, wake up the device */
-			if (ap->pflags & ATA_PFLAG_RESUMING)
-				ehc->i.dev_action[devno] |= ATA_EH_SET_ACTIVE;
 		}
 	}
 
@@ -3853,7 +3849,9 @@ int ata_eh_recover(struct ata_port *ap, ata_prereset_fn_t prereset,
 		 */
 		ata_for_each_dev(dev, link, ENABLED) {
 			if (ehc->i.dev_action[dev->devno] & ATA_EH_SET_ACTIVE) {
-				ata_dev_power_set_active(dev);
+				unsigned int err_mask = ata_dev_power_set_active(dev);
+				if (err_mask)
+					link->eh_info.dev_action[dev->devno] |= ATA_EH_SET_ACTIVE;
 				ata_eh_done(link, dev, ATA_EH_SET_ACTIVE);
 			}
 		}
diff --git a/drivers/ata/libata.h b/drivers/ata/libata.h
index 5c685bb1939e..43ad1ef9b63a 100644
--- a/drivers/ata/libata.h
+++ b/drivers/ata/libata.h
@@ -65,7 +65,7 @@ extern int ata_dev_configure(struct ata_device *dev);
 extern bool ata_dev_power_init_tf(struct ata_device *dev,
 				  struct ata_taskfile *tf, bool set_active);
 extern void ata_dev_power_set_standby(struct ata_device *dev);
-extern void ata_dev_power_set_active(struct ata_device *dev);
+extern unsigned int ata_dev_power_set_active(struct ata_device *dev);
 extern int sata_down_spd_limit(struct ata_link *link, u32 spd_limit);
 extern int ata_down_xfermask_limit(struct ata_device *dev, unsigned int sel);
 extern unsigned int ata_dev_set_feature(struct ata_device *dev,
-- 
2.30.2

[PATCH 2/4] libata: don't wake sleeping disk during system suspend

[Phillip Susi]

When suspending the system, libata puts the drive in standby mode to
prepare it to lose power.  If the drive is in SLEEP mode, this spins it up
only to spin it back down again.  Don't do that.
---
 drivers/ata/libata-core.c | 4 ++++
 1 file changed, 4 insertions(+)

diff --git a/drivers/ata/libata-core.c b/drivers/ata/libata-core.c
index a2d8cc0097a8..1244da8f77e2 100644
--- a/drivers/ata/libata-core.c
+++ b/drivers/ata/libata-core.c
@@ -2030,6 +2030,10 @@ void ata_dev_power_set_standby(struct ata_device *dev)
 	    system_entering_hibernation())
 		return;
 
+	/* no need to standby if it is alreqady sleeping */
+	if (dev->flags & ATA_DFLAG_SLEEPING)
+		return;
+
 	/* Issue STANDBY IMMEDIATE command only if supported by the device */
 	if (!ata_dev_power_init_tf(dev, &tf, false))
 		return;
-- 
2.30.2

Re: [PATCH 2/4] libata: don't wake sleeping disk during system suspend

[Damien Le Moal]

On 1/5/24 07:39, Phillip Susi wrote:
> When suspending the system, libata puts the drive in standby mode to
> prepare it to lose power.  If the drive is in SLEEP mode, this spins it up
> only to spin it back down again.  Don't do that.
> ---
>  drivers/ata/libata-core.c | 4 ++++
>  1 file changed, 4 insertions(+)
> 
> diff --git a/drivers/ata/libata-core.c b/drivers/ata/libata-core.c
> index a2d8cc0097a8..1244da8f77e2 100644
> --- a/drivers/ata/libata-core.c
> +++ b/drivers/ata/libata-core.c
> @@ -2030,6 +2030,10 @@ void ata_dev_power_set_standby(struct ata_device *dev)
>  	    system_entering_hibernation())
>  		return;
>  
> +	/* no need to standby if it is alreqady sleeping */

s/alreqady/already

The comment should also be improved. It is more than a "no need" given that a
sleeping disk will not respond to any command... So something like:

	/*
	 * If the devices is in SLEEP state, issuing a STANDBY IMMEDIATE
	 * command will fail. But given that the drive is already in a low
	 * power state, we do not need to do anything.
	 */

> +	if (dev->flags & ATA_DFLAG_SLEEPING)
> +		return;
> +
>  	/* Issue STANDBY IMMEDIATE command only if supported by the device */
>  	if (!ata_dev_power_init_tf(dev, &tf, false))
>  		return;

Other than the above comments, this looks OK. And this probably should go first
in the series with a fixes tag.

-- 
Damien Le Moal
Western Digital Research

Re: [PATCH 2/4] libata: don't wake sleeping disk during system suspend

[Phillip Susi]

Damien Le Moal <dlemoal@kernel.org> writes:

> The comment should also be improved. It is more than a "no need" given that a
> sleeping disk will not respond to any command... So something like:

Good point.

> 	/*
> 	 * If the devices is in SLEEP state, issuing a STANDBY IMMEDIATE
> 	 * command will fail. But given that the drive is already in a low
> 	 * power state, we do not need to do anything.
> 	 */

It didn't fail, it just caused the drive to spin up, only to spin right
back down again.

> Other than the above comments, this looks OK. And this probably should go first
> in the series with a fixes tag.

I'm not sure what I'd point a fixes tag at.  I think it's been this way
forever.  Well, at least as long as SLEEP support has been in, which is
basically forever.

[PATCH 3/4] libata: avoid waking disk for several commands

[Phillip Susi]

When a disk is in SLEEP mode it can not respond to any
commands.  Instead of waking up the sleeping disk, fake the
commands.  The commands include:

CHECK POWER
FLUSH CACHE
SLEEP
STANDBY IMMEDIATE
IDENTIFY

If we konw the disk is sleeping, we don't need to wake it up
to to find out if it is in standby, so just pretend it is in
standby.  While alseep, there's no dirty pages in the cache,
so there's no need to flush it.  There's no point in waking
a disk from sleep just to put it back to sleep.  We also have
a cache of the IDENTIFY information so just return that
instead of waking the disk.
---
 drivers/ata/libata-core.c | 16 ++++++++++++++++
 1 file changed, 16 insertions(+)

diff --git a/drivers/ata/libata-core.c b/drivers/ata/libata-core.c
index 1244da8f77e2..d9e889fa2881 100644
--- a/drivers/ata/libata-core.c
+++ b/drivers/ata/libata-core.c
@@ -5045,6 +5045,22 @@ void ata_qc_issue(struct ata_queued_cmd *qc)
 
 	/* if device is sleeping, schedule reset and abort the link */
 	if (unlikely(qc->dev->flags & ATA_DFLAG_SLEEPING)) {
+		if (unlikely(qc->tf.command == ATA_CMD_CHK_POWER ||
+			     qc->tf.command == ATA_CMD_SLEEP ||
+			     qc->tf.command == ATA_CMD_FLUSH ||
+			     qc->tf.command == ATA_CMD_FLUSH_EXT ||
+			     qc->tf.command == ATA_CMD_STANDBYNOW1 ||
+			     (qc->tf.command == ATA_CMD_ID_ATA &&
+			      !ata_tag_internal(qc->tag))))
+		{
+			/* fake reply to avoid waking drive */
+			qc->flags |= ATA_QCFLAG_RTF_FILLED;
+			qc->result_tf.nsect = 0;
+			if (qc->tf.command == ATA_CMD_ID_ATA)
+				sg_copy_from_buffer(qc->sg, 1, qc->dev->id, 2 * ATA_ID_WORDS);
+			ata_qc_complete(qc);
+			return;
+		}
 		link->eh_info.action |= ATA_EH_RESET;
 		ata_ehi_push_desc(&link->eh_info, "waking up from sleep");
 		ata_link_abort(link);
-- 
2.30.2

Re: [PATCH 3/4] libata: avoid waking disk for several commands

[Sergei Shtylyov]

Hello!

On 1/5/24 1:39 AM, Phillip Susi wrote:

> When a disk is in SLEEP mode it can not respond to any
> commands.  Instead of waking up the sleeping disk, fake the
> commands.  The commands include:
> 
> CHECK POWER

  This command is "officially" called CHECK POWER MODE... 

> FLUSH CACHE
> SLEEP
> STANDBY IMMEDIATE
> IDENTIFY
> 
> If we konw the disk is sleeping, we don't need to wake it up

   Know.

> to to find out if it is in standby, so just pretend it is in

   Double "to".

> standby.  While alseep, there's no dirty pages in the cache,

   Asleep.

> so there's no need to flush it.  There's no point in waking
> a disk from sleep just to put it back to sleep.  We also have
> a cache of the IDENTIFY information so just return that
> instead of waking the disk.
> ---
>  drivers/ata/libata-core.c | 16 ++++++++++++++++
>  1 file changed, 16 insertions(+)
> 
> diff --git a/drivers/ata/libata-core.c b/drivers/ata/libata-core.c
> index 1244da8f77e2..d9e889fa2881 100644
> --- a/drivers/ata/libata-core.c
> +++ b/drivers/ata/libata-core.c
> @@ -5045,6 +5045,22 @@ void ata_qc_issue(struct ata_queued_cmd *qc)
>  
>  	/* if device is sleeping, schedule reset and abort the link */
>  	if (unlikely(qc->dev->flags & ATA_DFLAG_SLEEPING)) {
> +		if (unlikely(qc->tf.command == ATA_CMD_CHK_POWER ||
> +			     qc->tf.command == ATA_CMD_SLEEP ||
> +			     qc->tf.command == ATA_CMD_FLUSH ||
> +			     qc->tf.command == ATA_CMD_FLUSH_EXT ||
> +			     qc->tf.command == ATA_CMD_STANDBYNOW1 ||
> +			     (qc->tf.command == ATA_CMD_ID_ATA &&
> +			      !ata_tag_internal(qc->tag))))

   How about a *switch* instead?

> +		{
> +			/* fake reply to avoid waking drive */
> +			qc->flags |= ATA_QCFLAG_RTF_FILLED;
> +			qc->result_tf.nsect = 0;
> +			if (qc->tf.command == ATA_CMD_ID_ATA)
> +				sg_copy_from_buffer(qc->sg, 1, qc->dev->id, 2 * ATA_ID_WORDS);
> +			ata_qc_complete(qc);
> +			return;
> +		}
>  		link->eh_info.action |= ATA_EH_RESET;
>  		ata_ehi_push_desc(&link->eh_info, "waking up from sleep");
>  		ata_link_abort(link);
> 

MBR, Sergey

Re: [PATCH 3/4] libata: avoid waking disk for several commands

[Phillip Susi]

Sergei Shtylyov <sergei.shtylyov@gmail.com> writes:

>   This command is "officially" called CHECK POWER MODE... 

Right.

>    Know.
>    Double "to".

Ooops.

>    Asleep.

Why would you capitalize that A?  It isn't a proper noun, nor the first
word of the sentance.

>    How about a *switch* instead?

Ok.

Re: [PATCH 3/4] libata: avoid waking disk for several commands

[Sergei Shtylyov]

On 1/5/24 7:24 PM, Phillip Susi wrote:
[...]

>>   This command is "officially" called CHECK POWER MODE... 
> 
> Right.
> 
>>    Know.
>>    Double "to".
> 
> Ooops.
> 
>>    Asleep.
> 
> Why would you capitalize that A?  It isn't a proper noun, nor the first
> word of the sentance.

   It was a start of my (single-word) sentence. I didn't mean you
should capitalize it, it just had a typo. :-)

>>    How about a *switch* instead?
> 
> Ok.

   Should be convertable into one...

MBR, Sergey

Re: [PATCH 3/4] libata: avoid waking disk for several commands

[Phillip Susi]

Sergei Shtylyov <sergei.shtylyov@gmail.com> writes:

>    It was a start of my (single-word) sentence. I didn't mean you
> should capitalize it, it just had a typo. :-)

Got it.  Applying changes now.

Re: [PATCH 3/4] libata: avoid waking disk for several commands

[Phillip Susi]

Sergei Shtylyov <sergei.shtylyov@gmail.com> writes:
>    How about a *switch* instead?

So what's the status on switch case fall through these days?  I thought
you just had to add a /* fallthrough */ comment to make it explicit, but
gcc is still complaining.

Re: [PATCH 3/4] libata: avoid waking disk for several commands

[Sergei Shtylyov]

On 1/6/24 11:29 PM, Phillip Susi wrote:
[...]

>>    How about a *switch* instead?
> 
> So what's the status on switch case fall through these days?  I thought
> you just had to add a /* fallthrough */ comment to make it explicit, but
> gcc is still complaining.

   There's a fallthrough macro in <linux//compiler_attributes.h> now,
I think you're suppoed to use it now...

MBR, Sergey

Re: [PATCH 3/4] libata: avoid waking disk for several commands

[Damien Le Moal]

On 1/5/24 07:39, Phillip Susi wrote:
> When a disk is in SLEEP mode it can not respond to any
> commands.  Instead of waking up the sleeping disk, fake the
> commands.  The commands include:
> 
> CHECK POWER
> FLUSH CACHE
> SLEEP
> STANDBY IMMEDIATE
> IDENTIFY
> 
> If we konw the disk is sleeping, we don't need to wake it up
> to to find out if it is in standby, so just pretend it is in
> standby.  While alseep, there's no dirty pages in the cache,
> so there's no need to flush it.  There's no point in waking
> a disk from sleep just to put it back to sleep.  We also have
> a cache of the IDENTIFY information so just return that
> instead of waking the disk.

What ? If you wake up the drive, it will not be in standby... So I do not get
your point here. Can you clarify ? What is the problem you are trying to solve
here ? Is it related to system or runtime suspend/resume ?

And no, not a chance we fake commands like this.

> ---
>  drivers/ata/libata-core.c | 16 ++++++++++++++++
>  1 file changed, 16 insertions(+)
> 
> diff --git a/drivers/ata/libata-core.c b/drivers/ata/libata-core.c
> index 1244da8f77e2..d9e889fa2881 100644
> --- a/drivers/ata/libata-core.c
> +++ b/drivers/ata/libata-core.c
> @@ -5045,6 +5045,22 @@ void ata_qc_issue(struct ata_queued_cmd *qc)
>  
>  	/* if device is sleeping, schedule reset and abort the link */
>  	if (unlikely(qc->dev->flags & ATA_DFLAG_SLEEPING)) {
> +		if (unlikely(qc->tf.command == ATA_CMD_CHK_POWER ||
> +			     qc->tf.command == ATA_CMD_SLEEP ||
> +			     qc->tf.command == ATA_CMD_FLUSH ||
> +			     qc->tf.command == ATA_CMD_FLUSH_EXT ||
> +			     qc->tf.command == ATA_CMD_STANDBYNOW1 ||
> +			     (qc->tf.command == ATA_CMD_ID_ATA &&
> +			      !ata_tag_internal(qc->tag))))
> +		{
> +			/* fake reply to avoid waking drive */
> +			qc->flags |= ATA_QCFLAG_RTF_FILLED;
> +			qc->result_tf.nsect = 0;
> +			if (qc->tf.command == ATA_CMD_ID_ATA)
> +				sg_copy_from_buffer(qc->sg, 1, qc->dev->id, 2 * ATA_ID_WORDS);
> +			ata_qc_complete(qc);
> +			return;
> +		}
>  		link->eh_info.action |= ATA_EH_RESET;
>  		ata_ehi_push_desc(&link->eh_info, "waking up from sleep");
>  		ata_link_abort(link);

-- 
Damien Le Moal
Western Digital Research

Re: [PATCH 3/4] libata: avoid waking disk for several commands

[Phillip Susi]

Damien Le Moal <dlemoal@kernel.org> writes:

> What ? If you wake up the drive, it will not be in standby... So I do not get
> your point here. Can you clarify ? What is the problem you are trying to solve
> here ? Is it related to system or runtime suspend/resume ?

The whole point is that we don't want to spin up the drive.  A drive
that is in standby simply treats these commands as a NOOP.  One that is
in SLEEP can not do that, so we must do it for the drive.

Without this patch, SLEEP mode is basically useless since the drive will
be woken up by one of these commands quite soon after you put it to
SLEEP.

This is just to make hdparm -Y not useless.  It has nothing to do with
suspend/resume.  I was thinking of splitting this patch series into two
parts, one with just the patches related to SLEEP and one with the
patches related to suspend/resume.

Re: [PATCH 3/4] libata: avoid waking disk for several commands

[Damien Le Moal]

On 1/6/24 01:30, Phillip Susi wrote:
> Damien Le Moal <dlemoal@kernel.org> writes:
> 
>> What ? If you wake up the drive, it will not be in standby... So I do not get
>> your point here. Can you clarify ? What is the problem you are trying to solve
>> here ? Is it related to system or runtime suspend/resume ?
> 
> The whole point is that we don't want to spin up the drive.  A drive
> that is in standby simply treats these commands as a NOOP.  One that is
> in SLEEP can not do that, so we must do it for the drive.

But who is issuing these commands ? If it is systemd/udev, then *that* need to
be patched to avoid it issuing these commands when the drive is sleeping.
Otherwise, there is no end to this. Next time systemd/udev is modified and start
issuing another command, we'll need to ignore that one as well. This is a
dangerous path that I am not willing to accept.

That would mean having a sysfs attribute indicating that the device is sleeping
though. So the sleep case needs more work.

> Without this patch, SLEEP mode is basically useless since the drive will
> be woken up by one of these commands quite soon after you put it to
> SLEEP.>
> This is just to make hdparm -Y not useless.  It has nothing to do with
> suspend/resume.  I was thinking of splitting this patch series into two
> parts, one with just the patches related to SLEEP and one with the
> patches related to suspend/resume.

As long as you can only set sleep mode with hdparm, there is not much we can do.
hdparm uses passthrough commands, and so handling whatever that tool does in the
kernel becomes a nightmare as libata would need to parse the issued commands and
handle their result. Only a few cases are done now. Extending that would be
difficult and fragile.

Rather, I would recommend improving the runtime pm code to allow for "going to
sleep" instead of "going to standby". A sysfs attribute switch can be used for
that, with the default being standby like now.

And yes, please split the series ! One for what you want to do for puis and
another for improved sleep handling. Everything together is simply too confusing.

-- 
Damien Le Moal
Western Digital Research

Re: [PATCH 3/4] libata: avoid waking disk for several commands

[Phillip Susi]

Damien Le Moal <dlemoal@kernel.org> writes:

> But who is issuing these commands ? If it is systemd/udev, then *that* need to
> be patched to avoid it issuing these commands when the drive is sleeping.
> Otherwise, there is no end to this. Next time systemd/udev is modified and start
> issuing another command, we'll need to ignore that one as well. This is a
> dangerous path that I am not willing to accept.

I have seen both smartd and udisks2 do this when they attempt to check
the SMART status of the drive.  They already issue a CHECK POWER MODE
command first and skip the SMART read if the drive is in standby, but
even this causes a drive that is in SLEEP mode to be woken up.

Also filesystems often issue FLUSH CACHE even though they have not
written anything to the disk, and there is nothing in the cache.  Drives
in standby just treat this as a NOOP, but a drive in SLEEP can not.

> That would mean having a sysfs attribute indicating that the device is sleeping
> though. So the sleep case needs more work.

Having a sysfs attribute that smartd/udisks2 could check before issuing
CHECK POWER MODE would help that case, but not the FLUSH CACHE case.

> As long as you can only set sleep mode with hdparm, there is not much we can do.
> hdparm uses passthrough commands, and so handling whatever that tool does in the
> kernel becomes a nightmare as libata would need to parse the issued commands and
> handle their result. Only a few cases are done now. Extending that would be
> difficult and fragile.

It already does that.  When the SLEEP command is issued, libata sets
ATA_DFLAG_SLEEPING.

> And yes, please split the series ! One for what you want to do for puis and
> another for improved sleep handling. Everything together is simply too confusing.

I realized that one of the patches was redundant anyhow, and just
finished writing up a good cover letter for the remaining 3.  Incoming.

[PATCH 0/3] Let sleeping disks lie

[Phillip Susi]

These patches attempt to help disks that are spun down stay that way,
until they are actually accessed.  The first patch deals with disks in
SLEEP mode ( hdparm -Y ).  SLEEP mode is a lower power mode than
STANDBY.  Linux has had support for it for a long time, however, if
you try to make use of it, you quickly find that the disk wakes up for
no apparent reason.  This is because in SLEEP mode, the disk
electronics are powered off so that it can not respond to any command.
Waking the disk up requires a SATA link reset.  There are a number of
commands that are regularly issued to a disk in standby that the disk
treats as a NOOP, but in SLEEP mode, these commands trigger the SATA
link reset to wake up the drive in order to send the command to the
drive.  This patch deals with this by completing the commands in
libata rather than waking the drive.

The third patch is about how system suspend/resume effects disks in
standby.  Archive disks that are very rarely accessed may spend most
of their lives in standby, but every time the system is suspended and
resumed, they spin up, only to be spun back down again.  This wastes
power and puts wear and tear on the drive that is not neccesary.  ATA
drives have a feature called Power Up In Standby that allows the drive
to not automatically spin up when the system is resumed.

There are two different types of PuiS: one that acts like regular
standby, in that any command that requires access to the disk will
automatically spin the disk up, and another that requires an explicit
SET FEATURES command to bring it out of PuiS mode.  The kernel used to
let the former type remain in standby after system resume, but recent
changes have changed that behavior.  This patch attempts to allow both
types to remain spun down after system resume, until they are actually
accessed.

It currently has two paths for doing so.  The one that is currently
active is to simply set the ATA_DFLAG_SLEEPING flag and leave the
drive in standby mode.  This flag causes a later attempt to access the
drive to trigger a round of EH that resets the SATA link, revalidates
the drive, and if needed, issude the SET FEATURES command to bring it
out of PuiS mode.  The alternative path ( currently #if 0'd out )
actually issues the SLEEP command to put the drive into SLEEP mode.
This can possibly save a little more power than leaving it in standby
mode, but the transition from standby to sleep may not be legal
according ot the ATA standards.  This therefore, may upset some drives
( though it works fine with mine ).  It therefore is not suitable as a
default.

I am thinking of adding a sysfs knob to control the PuiS behavior with
3 states:

0: wake the disk anyhow ( current behavior )
1: leave the disk in standby mode
2: put the disk to SLEEP

I think the worst thing that can happen with the third option is that
some disks might error and you will get some clutter in your syslog
while EH happens.  People can try this option and fall back to #1 if
they encounter issues.  Otherwise, they can enjoy the added power
savings.

Finally, the second patch addresses an issue where the third patch's
clearing of the SET_ACTIVE flag was countermanded.  This was because
the flag is set every time the EH starts during system resume.  In my
case, putting the drive to SLEEP caused a second round of EH to happen
as the SATA PHY is shut down during SLEEP.  That turned on the
SET_ACTIVE flag again, causing a VERIFY command to try to start the
drive, which failed because the drive requires the SET FEATURES
command to come out of PuiS mode.

Phillip Susi (3):
  libata: avoid waking disk for several commands
  libata: only wake a drive once on system resume
  libata: don't start PuiS disks on resume

 drivers/ata/libata-core.c | 61 ++++++++++++++++++++++++++++++++++-----
 drivers/ata/libata-eh.c   | 20 +++++++++----
 drivers/ata/libata.h      |  3 +-
 include/linux/libata.h    |  1 +
 4 files changed, 70 insertions(+), 15 deletions(-)

-- 
2.30.2

[PATCH 1/3] libata: avoid waking disk for several commands

[Phillip Susi]

When a disk is in SLEEP mode it can not respond to any
any commands.  Several commands are simply a NOOP to a disk
that is in standby mode, but when a disk is in SLEEP mode,
they frequencly cause the disk to spin up for no reason.
To avoid this, complete these commands in libata without
waking the disk.  These commands are:

CHECK POWER MODE
FLUSH CACHE
SLEEP
STANDBY IMMEDIATE
IDENTIFY

If we know the disk is sleeping, we don't need to wake it up
to find out if it is in standby, so just pretend it is in
standby.  While asleep, there's no dirty pages in the cache,
so there's no need to flush it.  There's no point in waking
a disk from sleep just to put it back to sleep.  We also have
a cache of the IDENTIFY information so just return that
instead of waking the disk.
---
 drivers/ata/libata-core.c | 20 ++++++++++++++++++++
 1 file changed, 20 insertions(+)

diff --git a/drivers/ata/libata-core.c b/drivers/ata/libata-core.c
index 09ed67772fae..6c5269de4bf2 100644
--- a/drivers/ata/libata-core.c
+++ b/drivers/ata/libata-core.c
@@ -5040,6 +5040,26 @@ void ata_qc_issue(struct ata_queued_cmd *qc)
 
 	/* if device is sleeping, schedule reset and abort the link */
 	if (unlikely(qc->dev->flags & ATA_DFLAG_SLEEPING)) {
+		switch (qc->tf.command)
+		{
+		case ATA_CMD_CHK_POWER:
+		case ATA_CMD_SLEEP:
+		case ATA_CMD_FLUSH:
+		case ATA_CMD_FLUSH_EXT:
+		case ATA_CMD_STANDBYNOW1:
+			if (qc->tf.command == ATA_CMD_ID_ATA)
+			{
+				/* only fake the reply for IDENTIFY if it is from userspace */
+				if (ata_tag_internal(qc->tag))
+					break;
+				sg_copy_from_buffer(qc->sg, 1, qc->dev->id, 2 * ATA_ID_WORDS);
+			}
+			/* fake reply to avoid waking drive */
+			qc->flags |= ATA_QCFLAG_RTF_FILLED;
+			qc->result_tf.nsect = 0;
+			ata_qc_complete(qc);
+			return;
+		}
 		link->eh_info.action |= ATA_EH_RESET;
 		ata_ehi_push_desc(&link->eh_info, "waking up from sleep");
 		ata_link_abort(link);
-- 
2.30.2

Re: [PATCH 1/3] libata: avoid waking disk for several commands

[Damien Le Moal]

On 1/8/24 03:02, Phillip Susi wrote:
> When a disk is in SLEEP mode it can not respond to any
> any commands.  Several commands are simply a NOOP to a disk
> that is in standby mode, but when a disk is in SLEEP mode,
> they frequencly cause the disk to spin up for no reason.
> To avoid this, complete these commands in libata without
> waking the disk.  These commands are:

As commented in patch 3/3, please use full 72-char lines for commit messages.

> 
> CHECK POWER MODE
> FLUSH CACHE
> SLEEP
> STANDBY IMMEDIATE
> IDENTIFY
> 
> If we know the disk is sleeping, we don't need to wake it up
> to find out if it is in standby, so just pretend it is in

sleep and standby are different power states. So saying that a disk that is
sleeping is in standby does not make sense. And if you wake up a drive from
sleep mode, it will *not* be in standby (need to re-check, but I think that
holds true even with PUIS enabled).

> standby.  While asleep, there's no dirty pages in the cache,
> so there's no need to flush it.  There's no point in waking
> a disk from sleep just to put it back to sleep.  We also have
> a cache of the IDENTIFY information so just return that
> instead of waking the disk.

The problem here is that ATA_DFLAG_SLEEPING is a horrible hack to not endup with
lots of timeout failures if the user execute "hdparm -Y". Executing such
passthrough command with a disk being used by an FS (for instance) is complete
nonsense and should not be done.

So I would rather see this handled correctly, through the kernel pm runtime
suspend/resume:
1) Define a libata device sysfs attribute that allows going to sleep instead of
the default standby when the disk is runtime suspended. If sleep is used, set
ATA_DFLAG_SLEEPING.
2) With that, any command issued to the disk will trigger runtime resume. If
ATA_DFLAG_SLEEPING is set, then the drive can be woken up with a link reset from
EH, going through ata_port_runtime_resume(), exactly like with the default
standby state for suspend. ATA_DFLAG_SLEEPING being set or not will indicate if
a simple verify command can spinup the disk or if a link abort is needed (like
done now in ata_qc_issue() which is really a nasty place to do that).

Now, the annoying thing is the drive being randomly woken-up due to commands
being issued, like the ones you mention. This is indeed bad, and seeing news
like this:

https://www.phoronix.com/news/Linux-PM-Regulatory-Bugs

I think we really should do better...

But I do not think the kernel is necessarilly the right place to fix this, at
least in the case of commands issued from userspace by things like smartd or
udevd. Patching there is needed to avoid uselessly waking up disks in runtime
suspend. systemd already has power policies etc, so there is integration with
the kernel side power management. Your issues come from using a tool (hdparm)
that has no integration at all with the OS daemons.

For FSes issued commands like flush, these are generally not random at all. If
you see them appearing randomly, then there is a problem with the FS and
patching the FS may be needed. Beside flush, there are other things to consider
here. Ex: FSes using zoned block devices (SMR disks) doing garbage collection
while idle. We cannot prevent this from happening, which is why I seriously
dislike the idea of faking any command for a sleeping disk.


> ---
>  drivers/ata/libata-core.c | 20 ++++++++++++++++++++
>  1 file changed, 20 insertions(+)
> 
> diff --git a/drivers/ata/libata-core.c b/drivers/ata/libata-core.c
> index 09ed67772fae..6c5269de4bf2 100644
> --- a/drivers/ata/libata-core.c
> +++ b/drivers/ata/libata-core.c
> @@ -5040,6 +5040,26 @@ void ata_qc_issue(struct ata_queued_cmd *qc)
>  
>  	/* if device is sleeping, schedule reset and abort the link */
>  	if (unlikely(qc->dev->flags & ATA_DFLAG_SLEEPING)) {
> +		switch (qc->tf.command)
> +		{
> +		case ATA_CMD_CHK_POWER:
> +		case ATA_CMD_SLEEP:
> +		case ATA_CMD_FLUSH:
> +		case ATA_CMD_FLUSH_EXT:
> +		case ATA_CMD_STANDBYNOW1:
> +			if (qc->tf.command == ATA_CMD_ID_ATA)
> +			{
> +				/* only fake the reply for IDENTIFY if it is from userspace */
> +				if (ata_tag_internal(qc->tag))
> +					break;
> +				sg_copy_from_buffer(qc->sg, 1, qc->dev->id, 2 * ATA_ID_WORDS);
> +			}
> +			/* fake reply to avoid waking drive */
> +			qc->flags |= ATA_QCFLAG_RTF_FILLED;
> +			qc->result_tf.nsect = 0;
> +			ata_qc_complete(qc);
> +			return;
> +		}
>  		link->eh_info.action |= ATA_EH_RESET;
>  		ata_ehi_push_desc(&link->eh_info, "waking up from sleep");
>  		ata_link_abort(link);

-- 
Damien Le Moal
Western Digital Research

Re: [PATCH 1/3] libata: avoid waking disk for several commands

[Phillip Susi]

Damien Le Moal <dlemoal@kernel.org> writes:

> sleep and standby are different power states. So saying that a disk that is
> sleeping is in standby does not make sense. And if you wake up a drive from

There is no way to answer CHECK POWER MODE and say the drive is in
sleep.  It can only be either active, or standby, so standby is the
closest fit.  At least it gets smartd and udisks2 to leave the drive
alone.

> The problem here is that ATA_DFLAG_SLEEPING is a horrible hack to not endup with
> lots of timeout failures if the user execute "hdparm -Y". Executing such
> passthrough command with a disk being used by an FS (for instance) is complete
> nonsense and should not be done.

I'm not sure what you propose be done instead.  Regardless, this is how
it has always been done, so I don't think there is any changing it now.
You also have the legacy standby timer that is exposed to users through
udisks2/gnome-disk-utility that still has to be supported.

> So I would rather see this handled correctly, through the kernel pm runtime
> suspend/resume:

I'd eventually like to as well, but it should also work in kernels that
aren't built with runtime pm enabled.

> Now, the annoying thing is the drive being randomly woken-up due to commands
> being issued, like the ones you mention. This is indeed bad, and seeing news
> like this:
>
> https://www.phoronix.com/news/Linux-PM-Regulatory-Bugs
>
> I think we really should do better...

That sounds like broken firmware to me.  When you ask the firmware to
power off the system, it should really be powered off.

> For FSes issued commands like flush, these are generally not random at all. If
> you see them appearing randomly, then there is a problem with the FS and
> patching the FS may be needed. Beside flush, there are other things to
> consider

I'm not sure the filesystem maintainers will see it that way.  They
generally issue barriers as part of a commit at regular intervals, and
that gets turned into FLUSH CACHE.  Also the kernel issues one during
system suspend, and I think that happens even if no filesystem is
mounted.  I think systemd also issues a sync() during shutdown, which
would wake up a sleeping disk only to shut down.

I don't think it is up to all of these other sources to be patched to
avoid this.  libata knows the disk is in sleep mode, so that is the
place to handle it.

> here. Ex: FSes using zoned block devices (SMR disks) doing garbage collection
> while idle. We cannot prevent this from happening, which is why I seriously
> dislike the idea of faking any command for a sleeping disk.

I don't see the connection.  If the FS issues IO in the background, the
disk will wake up, just like it does when in standby.  The faking of the
commands simply replicates the behavior you get from standby in sleep
mode.  With this patch, as far as anyone can tell, a sleeping disk is
exactly the same as one in standby.

Re: [PATCH 1/3] libata: avoid waking disk for several commands

[Damien Le Moal]

On 1/8/24 22:27, Phillip Susi wrote:
> Damien Le Moal <dlemoal@kernel.org> writes:
> 
>> sleep and standby are different power states. So saying that a disk that is
>> sleeping is in standby does not make sense. And if you wake up a drive from
> 
> There is no way to answer CHECK POWER MODE and say the drive is in
> sleep.  It can only be either active, or standby, so standby is the
> closest fit.  At least it gets smartd and udisks2 to leave the drive
> alone.

I was not commenting about what to do about your problem, but about the fact
that your sentence was very hard to understand because it was not technically
accurate.

>> The problem here is that ATA_DFLAG_SLEEPING is a horrible hack to not endup with
>> lots of timeout failures if the user execute "hdparm -Y". Executing such
>> passthrough command with a disk being used by an FS (for instance) is complete
>> nonsense and should not be done.
> 
> I'm not sure what you propose be done instead.  Regardless, this is how
> it has always been done, so I don't think there is any changing it now.

I never proposed to change that in any way. That is fine and can stay as it is.
What I do NOT want to do is expand upon this to try to solve issues. The reason
for that, which I already stated, is that hdparm issue passthrough commands. And
if the user wants to use passthrough commands, then most of the time, he/she
will have to deal with the consequences of not using kernel-provided management
methods.

I did propose to allow for runtime suspend to to use sleep state instead of
standby. That would be fairly easy to do and replace manual "hdparm -Y" with a
well integrated control of the disk power state up to the block layer.
You never commented back about this.

> You also have the legacy standby timer that is exposed to users through
> udisks2/gnome-disk-utility that still has to be supported.

What is this legacy standby timer ? What control path does it trigger ? Do
udisks2/gnome-disk-utility use that timer to issue commands like "hdparm -Y"  ?
Or does that timer tigh into the regular runtime suspend ?

>> So I would rather see this handled correctly, through the kernel pm runtime
>> suspend/resume:
> 
> I'd eventually like to as well, but it should also work in kernels that
> aren't built with runtime pm enabled.

No. As said many times now, I am not going to do anything about the hdparm -Y
hacking. If a user want better power management features, he/she should enable
power management in their kernels.

>> For FSes issued commands like flush, these are generally not random at all. If
>> you see them appearing randomly, then there is a problem with the FS and
>> patching the FS may be needed. Beside flush, there are other things to
>> consider
> 
> I'm not sure the filesystem maintainers will see it that way.  They
> generally issue barriers as part of a commit at regular intervals, and
> that gets turned into FLUSH CACHE.  Also the kernel issues one during
> system suspend, and I think that happens even if no filesystem is
> mounted.  I think systemd also issues a sync() during shutdown, which
> would wake up a sleeping disk only to shut down.

No. The scsi layer issues a FLUSH CACHE whenever a disk is removed, goes to
sleep or the system shutdown. And there is no need to do that if the disk is
already in standby. If you see that happening, then we need to fix that.

If the device is in sleep state from "hdparm -Y", then only libata knows that
the device is sleeping with the ATA_DFLAG_SLEEPING flag. That is the fundamental
problem here: pm-core, scsi and the block layer do not know that the block
device is sleeping (and so already had its write cache flushed). Your patches
are not solving this root cause issue. They are only hidding it by faking the
commands. This is a hack, wich likely will need more hacks in the future for
different cases. See my point ? I do not want to go down such route. Let's fix
things properly.

> I don't think it is up to all of these other sources to be patched to
> avoid this.  libata knows the disk is in sleep mode, so that is the
> place to handle it.

Not that simple. See above.


-- 
Damien Le Moal
Western Digital Research

Re: [PATCH 1/3] libata: avoid waking disk for several commands

[Phillip Susi]

Damien Le Moal <dlemoal@kernel.org> writes:

> I did propose to allow for runtime suspend to to use sleep state instead of
> standby. That would be fairly easy to do and replace manual "hdparm -Y" with a
> well integrated control of the disk power state up to the block layer.
> You never commented back about this.

That would be nice.  I assume that would involve changing how
libata-scsi.c translates SYNCHRONIZE CACHE from the scsi layer?

> What is this legacy standby timer ? What control path does it trigger ? Do
> udisks2/gnome-disk-utility use that timer to issue commands like "hdparm -Y"  ?
> Or does that timer tigh into the regular runtime suspend ?

The ATA disk internal auto standby timer, i.e. hdparm -S.

> No. As said many times now, I am not going to do anything about the hdparm -Y
> hacking. If a user want better power management features, he/she should enable
> power management in their kernels.

So you are saying that we need to patch the kernel to make runtime pm
work better, then patch smartd and udisks2 to check for runtime pm
before issuing their SMART commands, and patch udsisks2 to enable
runtime pm rather than using the legacy ATA standby timer?

> No. The scsi layer issues a FLUSH CACHE whenever a disk is removed, goes to
> sleep or the system shutdown. And there is no need to do that if the disk is
> already in standby. If you see that happening, then we need to fix that.

I'm almost certain that I have seen this happen, and I don't currently
see any code in sd.c that would would prevent it from issuing a FLUSH
CACHE to a disk that is runtime suspended when the system suspends or
shuts down.

The block layer also would need patched to avoid turning a barrier into
a FLUSH CACHE if the disk is runtime suspended, and also the sync()
path.  Is that even sensible to do?  It is true that for all block
devices, their caches do not need flushed while they are runtime
suspended?  It seems like it may be, but I'm not certain.

Re: [PATCH 1/3] libata: avoid waking disk for several commands

[Phillip Susi]

Phillip Susi <phill@thesusis.net> writes:

> The block layer also would need patched to avoid turning a barrier into
> a FLUSH CACHE if the disk is runtime suspended, and also the sync()
> path.  Is that even sensible to do?  It is true that for all block
> devices, their caches do not need flushed while they are runtime
> suspended?  It seems like it may be, but I'm not certain.

I was trying to do this.  I think the right place is in
blkdev_issue_flush(), but apparently bdev->bd_device is not the same
struct device that gets suspended.  I can't seem to work out where the
right struct device is to pass to pm_runtime_suspended() and skip the
flush operation.

Re: [PATCH 1/3] libata: avoid waking disk for several commands

[Phillip Susi]

Phillip Susi <phill@thesusis.net> writes:

> I was trying to do this.  I think the right place is in
> blkdev_issue_flush(), but apparently bdev->bd_device is not the same
> struct device that gets suspended.  I can't seem to work out where the
> right struct device is to pass to pm_runtime_suspended() and skip the
> flush operation.

I don't know what I was thinking yesterday.  It can't rely on
pm_runtime_suspended() because it would continue to flush and reset the
suspend timer before it ever gets suspended.  I wonder if it could use
the performance counters?  Whenever a flush is done, and also when
suspending, store the value of the write counter, and only if it has
changed, issue the flush, otherwise skip it?

Re: [PATCH 1/3] libata: avoid waking disk for several commands

[Damien Le Moal]

On 1/21/24 03:08, Phillip Susi wrote:
> Phillip Susi <phill@thesusis.net> writes:
> 
>> I was trying to do this.  I think the right place is in
>> blkdev_issue_flush(), but apparently bdev->bd_device is not the same
>> struct device that gets suspended.  I can't seem to work out where the
>> right struct device is to pass to pm_runtime_suspended() and skip the
>> flush operation.
> 
> I don't know what I was thinking yesterday.  It can't rely on
> pm_runtime_suspended() because it would continue to flush and reset the
> suspend timer before it ever gets suspended.  I wonder if it could use
> the performance counters?  Whenever a flush is done, and also when
> suspending, store the value of the write counter, and only if it has
> changed, issue the flush, otherwise skip it?

See my reply to your previous comment.
What you are trying to do is not the correct approach.

-- 
Damien Le Moal
Western Digital Research

Re: [PATCH 1/3] libata: avoid waking disk for several commands

[Damien Le Moal]

On 1/20/24 05:43, Phillip Susi wrote:
> Phillip Susi <phill@thesusis.net> writes:
> 
>> The block layer also would need patched to avoid turning a barrier into
>> a FLUSH CACHE if the disk is runtime suspended, and also the sync()
>> path.  Is that even sensible to do?  It is true that for all block
>> devices, their caches do not need flushed while they are runtime
>> suspended?  It seems like it may be, but I'm not certain.
> 
> I was trying to do this.  I think the right place is in
> blkdev_issue_flush(), but apparently bdev->bd_device is not the same
> struct device that gets suspended.  I can't seem to work out where the
> right struct device is to pass to pm_runtime_suspended() and skip the
> flush operation.

Flush issuing is a lot more complicated than just blkdev_issue_flush(). There is
a whole file dedicated to handling flushes (block/blk-flush.c).

But that is beside the point, which is that trying to not execute flush is
simply completely wrong. Please stop trying.

For your case, which is a drive put to sleep with hdparm -Y, only libata is
aware that the drive is sleeping. That first needs to change if you want the
kernel overall to be able to do anything. As I proposed already, using runtime
PM with sleep mode instead of standby would be a good start.

Regarding the flushes and other commands you see that are waking up the drive
for no *apparent* good reasons, please identify what application/component is
issuing these commands and look there to see why the commands are being issued
and if that is done with awareness for the device power state.

Then we can patch properly instead of trying to "hack".

-- 
Damien Le Moal
Western Digital Research

Re: [PATCH 1/3] libata: avoid waking disk for several commands

[Phillip Susi]

Damien Le Moal <dlemoal@kernel.org> writes:

> Flush issuing is a lot more complicated than just blkdev_issue_flush(). There is
> a whole file dedicated to handling flushes (block/blk-flush.c).
>
> But that is beside the point, which is that trying to not execute flush is
> simply completely wrong. Please stop trying.

I tried before to have libata ignore the useless flush and you said to
stop the flush from happening in the first place.  Now you say that's wrong?

> For your case, which is a drive put to sleep with hdparm -Y, only libata is
> aware that the drive is sleeping. That first needs to change if you want the
> kernel overall to be able to do anything. As I proposed already, using runtime
> PM with sleep mode instead of standby would be a good start.

No, I'm working on runtime pm now, as you suggested.  If you try using
runtime pm with disks, you quickly see that it does not work.

> Regarding the flushes and other commands you see that are waking up the drive
> for no *apparent* good reasons, please identify what application/component is
> issuing these commands and look there to see why the commands are being issued
> and if that is done with awareness for the device power state.

Filesystems flush every few seconds.  So does anyone calling sync(),
which the kernel does when you suspend to ram.

Either the filesystems need to keep track of whether a flush is needed
and skip it, or if they all call the same place ( blkdev_issue_flush ),
then it only needs done once in that place.

The core logic needs to be "if nothing has been written, then nothing
needs to be flushed".  Right now filesystems just flush periodically or
when their sync method is called to make sure that anything that has
been written is stable.

In userspace you have smartd and udisks2 to worry about.  The
information they need to know is whether the disk has otherwise been in
recent use and so they should not poll the SMART data.  I don't see
anything exported in the power/ directory that would give an indication
of the current *remaining* time until autosuspend, or how long it has
been since the last access.  Either one would be useful for userspace to
decide that nobody else seems to be using the disk, so I'll leave it
alone for now so it can go to sleep.

Re: [PATCH 1/3] libata: avoid waking disk for several commands

[Damien Le Moal]

On 1/25/24 01:04, Phillip Susi wrote:
> Damien Le Moal <dlemoal@kernel.org> writes:
> 
>> Flush issuing is a lot more complicated than just blkdev_issue_flush(). There is
>> a whole file dedicated to handling flushes (block/blk-flush.c).
>>
>> But that is beside the point, which is that trying to not execute flush is
>> simply completely wrong. Please stop trying.
> 
> I tried before to have libata ignore the useless flush and you said to
> stop the flush from happening in the first place.  Now you say that's wrong?

No, not wrong. But you are looking at the wrong layer. Do not try to prevent
flushes from being issued at the block layer level but *above* it. That is,
FSes, applications etc.

>> For your case, which is a drive put to sleep with hdparm -Y, only libata is
>> aware that the drive is sleeping. That first needs to change if you want the
>> kernel overall to be able to do anything. As I proposed already, using runtime
>> PM with sleep mode instead of standby would be a good start.
> 
> No, I'm working on runtime pm now, as you suggested.  If you try using
> runtime pm with disks, you quickly see that it does not work.

What does not work ? Everything is fine with my testing: the drive is always
woken up whenever someone (FS, applications etc) issue a block IO (including
flush) to the block layer. That is the expected behavior. If you want to have
the disk keep sleeping, the device users must stop poking at the drive.

> 
>> Regarding the flushes and other commands you see that are waking up the drive
>> for no *apparent* good reasons, please identify what application/component is
>> issuing these commands and look there to see why the commands are being issued
>> and if that is done with awareness for the device power state.
> 
> Filesystems flush every few seconds.  So does anyone calling sync(),
> which the kernel does when you suspend to ram.

Filesystems issue flush only if they have dirty data to flush, normally. I have
not looked at ext4/xfs code in a while, but if the FS has not committed any
transaction in the last cycle, there should be no flush issued. If there is,
then someone is reading or writing files (for reading, if you have atime
enabled, that will cause a metadata change and so a transaction commit).

> Either the filesystems need to keep track of whether a flush is needed
> and skip it, or if they all call the same place ( blkdev_issue_flush ),
> then it only needs done once in that place.

See above. That is why I am telling you to run blktrace or any tracer to figure
out the command sequence and who is doing what. There is absolutely no clarity
to what you are describing and so we cannot plan for a solution.

> The core logic needs to be "if nothing has been written, then nothing
> needs to be flushed".  Right now filesystems just flush periodically or
> when their sync method is called to make sure that anything that has
> been written is stable.

I do not think that the periodic flush happens if nothing has been written. For
"sync", someone issues that, not the FS on its own. So trace it to find out who
is doing that.

> In userspace you have smartd and udisks2 to worry about.  The
> information they need to know is whether the disk has otherwise been in
> recent use and so they should not poll the SMART data.  I don't see
> anything exported in the power/ directory that would give an indication
> of the current *remaining* time until autosuspend, or how long it has
> been since the last access.  Either one would be useful for userspace to
> decide that nobody else seems to be using the disk, so I'll leave it
> alone for now so it can go to sleep.

-- 
Damien Le Moal
Western Digital Research

Re: [PATCH 1/3] libata: avoid waking disk for several commands

[Phillip Susi]

Damien Le Moal <dlemoal@kernel.org> writes:

> What does not work ? Everything is fine with my testing: the drive is always
> woken up whenever someone (FS, applications etc) issue a block IO (including
> flush) to the block layer. That is the expected behavior. If you want to have
> the disk keep sleeping, the device users must stop poking at the drive.

It seems that I have put my foot in my mouth.  When I first started
working on these patches way back when, I did see flushes without any
writes in the blktrace keeping the drive awake.  I assumed that was
still a problem that I needed to tackle.  I should have tested it
first.  It seems this problem has been fixed already.

ext4 does still seem to issue a flush with no writes in the sync path
though, causing a drive to spin up for no reason, then right back down
when you suspend-to-ram.  I guess I'll ask about this on the ext4 list.

Circling back to the PuiS patch, did I understand correctly that you are
fine with that as long as it integrates with runtime pm?

I had tried at one point to add support for REQUEST SENSE to libata so
that sd can issue that to find out if the disk is powered up or not, and
set the runtime_pm status of the disk accordingly.  Does that make sense
to you?

Re: [PATCH 1/3] libata: avoid waking disk for several commands

[Damien Le Moal]

On 2/2/24 05:01, Phillip Susi wrote:
> Damien Le Moal <dlemoal@kernel.org> writes:
> 
>> What does not work ? Everything is fine with my testing: the drive is always
>> woken up whenever someone (FS, applications etc) issue a block IO (including
>> flush) to the block layer. That is the expected behavior. If you want to have
>> the disk keep sleeping, the device users must stop poking at the drive.
> 
> It seems that I have put my foot in my mouth.  When I first started
> working on these patches way back when, I did see flushes without any
> writes in the blktrace keeping the drive awake.  I assumed that was
> still a problem that I needed to tackle.  I should have tested it
> first.  It seems this problem has been fixed already.
> 
> ext4 does still seem to issue a flush with no writes in the sync path
> though, causing a drive to spin up for no reason, then right back down
> when you suspend-to-ram.  I guess I'll ask about this on the ext4 list.
> 
> Circling back to the PuiS patch, did I understand correctly that you are
> fine with that as long as it integrates with runtime pm?

Yes, but only for drives that report full identify data when PUIS is enabled.
For drives that report incomplete identify data, we have no choice but to wake
them up. And yes, we need integration with runtime pm to set the initial power
state of the drive to standby (instead of "on") for both the ata device and its
scsi device.

> I had tried at one point to add support for REQUEST SENSE to libata so
> that sd can issue that to find out if the disk is powered up or not, and
> set the runtime_pm status of the disk accordingly.  Does that make sense
> to you?

I need to check that. I think there may be a better/easier way to get the
current power state of a drive. Will get back to you on that.

-- 
Damien Le Moal
Western Digital Research

Re: [PATCH 1/3] libata: avoid waking disk for several commands

[Phillip Susi]

Damien Le Moal <dlemoal@kernel.org> writes:

> Yes, but only for drives that report full identify data when PUIS is enabled.
> For drives that report incomplete identify data, we have no choice but to wake
> them up. And yes, we need integration with runtime pm to set the
> initial power

Why was that again?  I think you said something about needing to set the
speed correctly so you at least need to know if this drive requires a
lower speed than the other in the PATA master/slave pair?  Wouldn't that
only require the speed information, not all identify data?

> state of the drive to standby (instead of "on") for both the ata device and its
> scsi device.

You mean if the whole device hierarchy were changed so that instead of
the scsi_host being a child of the port with the links and devices
hanging off to the side, the scsi_host would be the child of the ata device?

> I need to check that. I think there may be a better/easier way to get the
> current power state of a drive. Will get back to you on that.

That would be good.  At one point that was the way I found, and also I
think it was in the SAT-3 spec that is how REQUEST SENSE should be
implemented for ATA disks.

Re: [PATCH 1/3] libata: avoid waking disk for several commands

[Damien Le Moal]

On 2/3/24 04:53, Phillip Susi wrote:
> Damien Le Moal <dlemoal@kernel.org> writes:
> 
>> Yes, but only for drives that report full identify data when PUIS is enabled.
>> For drives that report incomplete identify data, we have no choice but to wake
>> them up. And yes, we need integration with runtime pm to set the
>> initial power
> 
> Why was that again?  I think you said something about needing to set the
> speed correctly so you at least need to know if this drive requires a
> lower speed than the other in the PATA master/slave pair?  Wouldn't that
> only require the speed information, not all identify data?

See ata_dev_revalidate() and ata_dev_configure() and all the drive features that
are checked using the identify data. We need to preserve that to ensure that
nothing changed on the drive so that its representation in libata is kept in
sync with the drive config. That is why drive starting with PUIS and not giving
full identify data *must* be woken up, which is the current libata behavior.

>> state of the drive to standby (instead of "on") for both the ata device and its
>> scsi device.
> 
> You mean if the whole device hierarchy were changed so that instead of
> the scsi_host being a child of the port with the links and devices
> hanging off to the side, the scsi_host would be the child of the ata device?

You do not need to change the hierarchy of devices. An ata_dev is already a
child of its scsi_dev. So if you want to set the ata device to runtime suspend
state, you have to have the parent in the same state too. runtime suspend work
top-to-bottom in the device chain. You cannot have random device in the middle
of the chain going to suspend without the devices above it also being suspended.

Also, the user does not use ata devices directly. They use the scsi device
representing the ata device. You must thus have that in sync with the ata device
state.

>> I need to check that. I think there may be a better/easier way to get the
>> current power state of a drive. Will get back to you on that.
> 
> That would be good.  At one point that was the way I found, and also I
> think it was in the SAT-3 spec that is how REQUEST SENSE should be
> implemented for ATA disks.

TEST UNIT READY is the command to use. I need to check the specs again about how
it reports the device state though. I think it is through sense data. The scsi
disk driver issues that command already to check that the drive is spun-up. See
sd_revalidate_disk() calling sd_spinup_disk(). So that will also need to change
to be aware of the initial power state to not spinup the drive if not wanted.
That will need to be done extremely carefully though to only affect ata devices
with PUIS. Likely some additional scsi device flag will be needed for this. I
have not looked into that yet.

-- 
Damien Le Moal
Western Digital Research

Re: [PATCH 1/3] libata: avoid waking disk for several commands

[Phillip Susi]

Damien Le Moal <dlemoal@kernel.org> writes:

> See ata_dev_revalidate() and ata_dev_configure() and all the drive features that
> are checked using the identify data. We need to preserve that to ensure that
> nothing changed on the drive so that its representation in libata is kept in
> sync with the drive config. That is why drive starting with PUIS and not giving
> full identify data *must* be woken up, which is the current libata behavior.

Yes, the information is needed to revalidate, but why must this
revalidation be done during system resume, rather than postponed until later?

> You do not need to change the hierarchy of devices. An ata_dev is already a
> child of its scsi_dev. So if you want to set the ata device to runtime
> suspend

How is an ata_dev a child of its own scsi_dev?  Or did you mean to say
the reverse: the scsi_dev is a child of the ata_dev?  But that isn't the
case either.  In sysfs, you have:

ata_port
 - scsi_host
   - scsi_target
     - scsi_lun
 - ata_link
   - ata_dev

The link and dev hang off to the side, not in the ancestry of the scsi
disk.

> state, you have to have the parent in the same state too. runtime suspend work
> top-to-bottom in the device chain. You cannot have random device in the middle
> of the chain going to suspend without the devices above it also being suspended.
>
> Also, the user does not use ata devices directly. They use the scsi device
> representing the ata device. You must thus have that in sync with the ata device
> state.

Right... so when the system is resumed, first the ata_port is resumed,
then it has to be on for the scsi host, target, and lun to resume.  At
that point sd could check if the disk is actually spinning, and if not,
force it to suspend, which then allows the target to suspend, wich then
allows the host to suspend, and finally the ata_port.

Re: [PATCH 1/3] libata: avoid waking disk for several commands

[Sergey Shtylyov]

On 1/7/24 9:02 PM, Phillip Susi wrote:

> When a disk is in SLEEP mode it can not respond to any
> any commands.  Several commands are simply a NOOP to a disk
> that is in standby mode, but when a disk is in SLEEP mode,
> they frequencly cause the disk to spin up for no reason.

   Frequently.

> To avoid this, complete these commands in libata without
> waking the disk.  These commands are:
> 
> CHECK POWER MODE
> FLUSH CACHE
> SLEEP
> STANDBY IMMEDIATE
> IDENTIFY
> 
> If we know the disk is sleeping, we don't need to wake it up
> to find out if it is in standby, so just pretend it is in
> standby.  While asleep, there's no dirty pages in the cache,
> so there's no need to flush it.  There's no point in waking
> a disk from sleep just to put it back to sleep.  We also have
> a cache of the IDENTIFY information so just return that
> instead of waking the disk.
[...]

   Did you run your patches thru scripts/checkpatch.pl? Looking
at this patch, I think you didn't... :-)

> diff --git a/drivers/ata/libata-core.c b/drivers/ata/libata-core.c
> index 09ed67772fae..6c5269de4bf2 100644
> --- a/drivers/ata/libata-core.c
> +++ b/drivers/ata/libata-core.c
> @@ -5040,6 +5040,26 @@ void ata_qc_issue(struct ata_queued_cmd *qc)
>  
>  	/* if device is sleeping, schedule reset and abort the link */
>  	if (unlikely(qc->dev->flags & ATA_DFLAG_SLEEPING)) {
> +		switch (qc->tf.command)
> +		{

		switch (qc->tf.command) {

> +		case ATA_CMD_CHK_POWER:
> +		case ATA_CMD_SLEEP:
> +		case ATA_CMD_FLUSH:
> +		case ATA_CMD_FLUSH_EXT:
> +		case ATA_CMD_STANDBYNOW1:
> +			if (qc->tf.command == ATA_CMD_ID_ATA)

   This can't happen, you forgot:

		case ATA_CMD_ID_ATA:

> +			{

			if (qc->tf.command == ATA_CMD_ID_ATA) {

> +				/* only fake the reply for IDENTIFY if it is from userspace */
> +				if (ata_tag_internal(qc->tag))
> +					break;
> +				sg_copy_from_buffer(qc->sg, 1, qc->dev->id, 2 * ATA_ID_WORDS);
> +			}
> +			/* fake reply to avoid waking drive */
> +			qc->flags |= ATA_QCFLAG_RTF_FILLED;
> +			qc->result_tf.nsect = 0;
> +			ata_qc_complete(qc);
> +			return;
> +		}
>  		link->eh_info.action |= ATA_EH_RESET;
>  		ata_ehi_push_desc(&link->eh_info, "waking up from sleep");
>  		ata_link_abort(link);

MBR, Sergey

Re: [PATCH 1/3] libata: avoid waking disk for several commands

[Phillip Susi]

Sergey Shtylyov <s.shtylyov@omp.ru> writes:

>    Did you run your patches thru scripts/checkpatch.pl? Looking
> at this patch, I think you didn't... :-)

Will do.

>    This can't happen, you forgot:
>
> 		case ATA_CMD_ID_ATA:

Woops.. that must have gotten lost somehow when I was cutting and
pasting to avoid the fallthrough.

[PATCH 2/3] libata: only wake a drive once on system resume

[Phillip Susi]

In the event that more than one pass of EH is needed during system resume,
only request the drive be started once ( if the first time worked ).
---
 drivers/ata/libata-core.c | 9 +++++----
 drivers/ata/libata-eh.c   | 8 +++-----
 drivers/ata/libata.h      | 2 +-
 3 files changed, 9 insertions(+), 10 deletions(-)

diff --git a/drivers/ata/libata-core.c b/drivers/ata/libata-core.c
index 6c5269de4bf2..ef6a2349a6f8 100644
--- a/drivers/ata/libata-core.c
+++ b/drivers/ata/libata-core.c
@@ -2080,7 +2080,7 @@ static bool ata_dev_power_is_active(struct ata_device *dev)
  *	LOCKING:
  *	Kernel thread context (may sleep).
  */
-void ata_dev_power_set_active(struct ata_device *dev)
+unsigned int ata_dev_power_set_active(struct ata_device *dev)
 {
 	struct ata_taskfile tf;
 	unsigned int err_mask;
@@ -2090,14 +2090,14 @@ void ata_dev_power_set_active(struct ata_device *dev)
 	 * if supported by the device.
 	 */
 	if (!ata_dev_power_init_tf(dev, &tf, true))
-		return;
+		return AC_ERR_OK;
 
 	/*
 	 * 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;
+		return AC_ERR_OK;
 
 	ata_dev_notice(dev, "Entering active power mode\n");
 
@@ -2105,6 +2105,7 @@ void ata_dev_power_set_active(struct ata_device *dev)
 	if (err_mask)
 		ata_dev_err(dev, "VERIFY failed (err_mask=0x%x)\n",
 			    err_mask);
+	return err_mask;
 }
 
 /**
@@ -5277,7 +5278,7 @@ static int ata_port_pm_poweroff(struct device *dev)
 static void ata_port_resume(struct ata_port *ap, pm_message_t mesg,
 			    bool async)
 {
-	ata_port_request_pm(ap, mesg, ATA_EH_RESET,
+	ata_port_request_pm(ap, mesg, ATA_EH_RESET | ATA_EH_SET_ACTIVE,
 			    ATA_EHI_NO_AUTOPSY | ATA_EHI_QUIET,
 			    async);
 }
diff --git a/drivers/ata/libata-eh.c b/drivers/ata/libata-eh.c
index b0d6e69c4a5b..799a1b8bc384 100644
--- a/drivers/ata/libata-eh.c
+++ b/drivers/ata/libata-eh.c
@@ -710,10 +710,6 @@ void ata_scsi_port_error_handler(struct Scsi_Host *host, struct ata_port *ap)
 			ehc->saved_xfer_mode[devno] = dev->xfer_mode;
 			if (ata_ncq_enabled(dev))
 				ehc->saved_ncq_enabled |= 1 << devno;
-
-			/* If we are resuming, wake up the device */
-			if (ap->pflags & ATA_PFLAG_RESUMING)
-				ehc->i.dev_action[devno] |= ATA_EH_SET_ACTIVE;
 		}
 	}
 
@@ -3853,7 +3849,9 @@ int ata_eh_recover(struct ata_port *ap, ata_prereset_fn_t prereset,
 		 */
 		ata_for_each_dev(dev, link, ENABLED) {
 			if (ehc->i.dev_action[dev->devno] & ATA_EH_SET_ACTIVE) {
-				ata_dev_power_set_active(dev);
+				unsigned int err_mask = ata_dev_power_set_active(dev);
+				if (err_mask)
+					link->eh_info.dev_action[dev->devno] |= ATA_EH_SET_ACTIVE;
 				ata_eh_done(link, dev, ATA_EH_SET_ACTIVE);
 			}
 		}
diff --git a/drivers/ata/libata.h b/drivers/ata/libata.h
index 5c685bb1939e..43ad1ef9b63a 100644
--- a/drivers/ata/libata.h
+++ b/drivers/ata/libata.h
@@ -65,7 +65,7 @@ extern int ata_dev_configure(struct ata_device *dev);
 extern bool ata_dev_power_init_tf(struct ata_device *dev,
 				  struct ata_taskfile *tf, bool set_active);
 extern void ata_dev_power_set_standby(struct ata_device *dev);
-extern void ata_dev_power_set_active(struct ata_device *dev);
+extern unsigned int ata_dev_power_set_active(struct ata_device *dev);
 extern int sata_down_spd_limit(struct ata_link *link, u32 spd_limit);
 extern int ata_down_xfermask_limit(struct ata_device *dev, unsigned int sel);
 extern unsigned int ata_dev_set_feature(struct ata_device *dev,
-- 
2.30.2

Re: [PATCH 2/3] libata: only wake a drive once on system resume

[Damien Le Moal]

On 1/8/24 03:02, Phillip Susi wrote:
> In the event that more than one pass of EH is needed during system resume,
> only request the drive be started once ( if the first time worked ).

See my comment on patch 3/3. You need this only and only because of how you
handle PUIS state, forcing (an invalid) transition to sleep state.

> ---
>  drivers/ata/libata-core.c | 9 +++++----
>  drivers/ata/libata-eh.c   | 8 +++-----
>  drivers/ata/libata.h      | 2 +-
>  3 files changed, 9 insertions(+), 10 deletions(-)
> 
> diff --git a/drivers/ata/libata-core.c b/drivers/ata/libata-core.c
> index 6c5269de4bf2..ef6a2349a6f8 100644
> --- a/drivers/ata/libata-core.c
> +++ b/drivers/ata/libata-core.c
> @@ -2080,7 +2080,7 @@ static bool ata_dev_power_is_active(struct ata_device *dev)
>   *	LOCKING:
>   *	Kernel thread context (may sleep).
>   */
> -void ata_dev_power_set_active(struct ata_device *dev)
> +unsigned int ata_dev_power_set_active(struct ata_device *dev)
>  {
>  	struct ata_taskfile tf;
>  	unsigned int err_mask;
> @@ -2090,14 +2090,14 @@ void ata_dev_power_set_active(struct ata_device *dev)
>  	 * if supported by the device.
>  	 */
>  	if (!ata_dev_power_init_tf(dev, &tf, true))
> -		return;
> +		return AC_ERR_OK;
>  
>  	/*
>  	 * 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;
> +		return AC_ERR_OK;
>  
>  	ata_dev_notice(dev, "Entering active power mode\n");
>  
> @@ -2105,6 +2105,7 @@ void ata_dev_power_set_active(struct ata_device *dev)
>  	if (err_mask)
>  		ata_dev_err(dev, "VERIFY failed (err_mask=0x%x)\n",
>  			    err_mask);
> +	return err_mask;
>  }
>  
>  /**
> @@ -5277,7 +5278,7 @@ static int ata_port_pm_poweroff(struct device *dev)
>  static void ata_port_resume(struct ata_port *ap, pm_message_t mesg,
>  			    bool async)
>  {
> -	ata_port_request_pm(ap, mesg, ATA_EH_RESET,
> +	ata_port_request_pm(ap, mesg, ATA_EH_RESET | ATA_EH_SET_ACTIVE,
>  			    ATA_EHI_NO_AUTOPSY | ATA_EHI_QUIET,
>  			    async);
>  }
> diff --git a/drivers/ata/libata-eh.c b/drivers/ata/libata-eh.c
> index b0d6e69c4a5b..799a1b8bc384 100644
> --- a/drivers/ata/libata-eh.c
> +++ b/drivers/ata/libata-eh.c
> @@ -710,10 +710,6 @@ void ata_scsi_port_error_handler(struct Scsi_Host *host, struct ata_port *ap)
>  			ehc->saved_xfer_mode[devno] = dev->xfer_mode;
>  			if (ata_ncq_enabled(dev))
>  				ehc->saved_ncq_enabled |= 1 << devno;
> -
> -			/* If we are resuming, wake up the device */
> -			if (ap->pflags & ATA_PFLAG_RESUMING)
> -				ehc->i.dev_action[devno] |= ATA_EH_SET_ACTIVE;
>  		}
>  	}
>  
> @@ -3853,7 +3849,9 @@ int ata_eh_recover(struct ata_port *ap, ata_prereset_fn_t prereset,
>  		 */
>  		ata_for_each_dev(dev, link, ENABLED) {
>  			if (ehc->i.dev_action[dev->devno] & ATA_EH_SET_ACTIVE) {
> -				ata_dev_power_set_active(dev);
> +				unsigned int err_mask = ata_dev_power_set_active(dev);
> +				if (err_mask)
> +					link->eh_info.dev_action[dev->devno] |= ATA_EH_SET_ACTIVE;
>  				ata_eh_done(link, dev, ATA_EH_SET_ACTIVE);
>  			}
>  		}
> diff --git a/drivers/ata/libata.h b/drivers/ata/libata.h
> index 5c685bb1939e..43ad1ef9b63a 100644
> --- a/drivers/ata/libata.h
> +++ b/drivers/ata/libata.h
> @@ -65,7 +65,7 @@ extern int ata_dev_configure(struct ata_device *dev);
>  extern bool ata_dev_power_init_tf(struct ata_device *dev,
>  				  struct ata_taskfile *tf, bool set_active);
>  extern void ata_dev_power_set_standby(struct ata_device *dev);
> -extern void ata_dev_power_set_active(struct ata_device *dev);
> +extern unsigned int ata_dev_power_set_active(struct ata_device *dev);
>  extern int sata_down_spd_limit(struct ata_link *link, u32 spd_limit);
>  extern int ata_down_xfermask_limit(struct ata_device *dev, unsigned int sel);
>  extern unsigned int ata_dev_set_feature(struct ata_device *dev,

-- 
Damien Le Moal
Western Digital Research

[PATCH 3/3] libata: don't start PuiS disks on resume

[Phillip Susi]

Disks with Power Up In Standby enabled that required the
SET FEATURES command to start up were being issued the
command during resume.  Recently this was extended to
include PuiS disks that don't require the SET FEATURES
command.  Suppress this and leave the disk in standby
until the disk is actually accessed.
---
 drivers/ata/libata-core.c | 32 ++++++++++++++++++++++++++++----
 drivers/ata/libata-eh.c   | 12 +++++++++++-
 drivers/ata/libata.h      |  1 +
 include/linux/libata.h    |  1 +
 4 files changed, 41 insertions(+), 5 deletions(-)

diff --git a/drivers/ata/libata-core.c b/drivers/ata/libata-core.c
index ef6a2349a6f8..f758fc88ac19 100644
--- a/drivers/ata/libata-core.c
+++ b/drivers/ata/libata-core.c
@@ -1912,7 +1912,30 @@ int ata_dev_read_id(struct ata_device *dev, unsigned int *p_class,
 			goto err_out;
 	}
 
-	if (!tried_spinup && (id[2] == 0x37c8 || id[2] == 0x738c)) {
+	if (flags & ATA_READID_NOSTART && id[2] == 0x37c8)
+	{
+		/*
+		 * The drive has powered up in standby, and returned incomplete IDENTIFY info
+		 * so we can't revalidate it yet.  We have also been asked to avoid starting the
+		 * drive, so stop here and leave the drive asleep and the EH pending, to be
+		 * restarted on later I/O request
+		 */
+#if 0
+		ata_tf_init(dev, &tf);
+		tf.flags |= ATA_TFLAG_DEVICE | ATA_TFLAG_ISADDR;
+		tf.protocol = ATA_PROT_NODATA;
+		tf.command = ATA_CMD_SLEEP;
+		err_mask = ata_exec_internal(dev, &tf, NULL, DMA_NONE, NULL, 0, 0);
+		ata_dev_info(dev, "PuiS detected, putting drive to sleep");
+#else
+		dev->flags |= ATA_DFLAG_SLEEPING;
+		ata_dev_info(dev, "PuiS detected, leaving drive in standby");
+#endif
+		return -EAGAIN;
+	}
+
+	if (!(flags & ATA_READID_NOSTART) &&
+	    !tried_spinup && (id[2] == 0x37c8 || id[2] == 0x738c)) {
 		tried_spinup = 1;
 		/*
 		 * Drive powered-up in standby mode, and requires a specific
@@ -3969,6 +3992,8 @@ int ata_dev_revalidate(struct ata_device *dev, unsigned int new_class,
 
 	/* re-read ID */
 	rc = ata_dev_reread_id(dev, readid_flags);
+	if (rc == -EAGAIN)
+		return rc;
 	if (rc)
 		goto fail;
 
@@ -5241,8 +5266,7 @@ static void ata_port_suspend(struct ata_port *ap, pm_message_t mesg,
 	 * http://thread.gmane.org/gmane.linux.ide/46764
 	 */
 	ata_port_request_pm(ap, mesg, 0,
-			    ATA_EHI_QUIET | ATA_EHI_NO_AUTOPSY |
-			    ATA_EHI_NO_RECOVERY,
+			    ATA_EHI_QUIET | ATA_EHI_NO_AUTOPSY | ATA_EHI_NO_RECOVERY,
 			    async);
 }
 
@@ -5279,7 +5303,7 @@ static void ata_port_resume(struct ata_port *ap, pm_message_t mesg,
 			    bool async)
 {
 	ata_port_request_pm(ap, mesg, ATA_EH_RESET | ATA_EH_SET_ACTIVE,
-			    ATA_EHI_NO_AUTOPSY | ATA_EHI_QUIET,
+			    ATA_EHI_NO_AUTOPSY | ATA_EHI_QUIET | ATA_EHI_NOSTART,
 			    async);
 }
 
diff --git a/drivers/ata/libata-eh.c b/drivers/ata/libata-eh.c
index 799a1b8bc384..e45e60112951 100644
--- a/drivers/ata/libata-eh.c
+++ b/drivers/ata/libata-eh.c
@@ -22,6 +22,7 @@
 #include <scsi/scsi_cmnd.h>
 #include <scsi/scsi_dbg.h>
 #include "../scsi/scsi_transport_api.h"
+#include <linux/pm_runtime.h>
 
 #include <linux/libata.h>
 
@@ -3046,6 +3047,8 @@ static int ata_eh_revalidate_and_attach(struct ata_link *link,
 
 		if (ehc->i.flags & ATA_EHI_DID_RESET)
 			readid_flags |= ATA_READID_POSTRESET;
+		if (ehc->i.flags & ATA_EHI_NOSTART)
+			readid_flags |= ATA_READID_NOSTART;
 
 		if ((action & ATA_EH_REVALIDATE) && ata_dev_enabled(dev)) {
 			WARN_ON(dev->class == ATA_DEV_PMP);
@@ -3075,8 +3078,15 @@ static int ata_eh_revalidate_and_attach(struct ata_link *link,
 			ata_eh_about_to_do(link, dev, ATA_EH_REVALIDATE);
 			rc = ata_dev_revalidate(dev, ehc->classes[dev->devno],
 						readid_flags);
-			if (rc)
+			if (rc == -EAGAIN) {
+				rc = 0; /* start required but suppressed, handle later */
+				ehc->i.dev_action[dev->devno] &= ~ATA_EH_SET_ACTIVE;
+				continue;
+			}
+			else if (rc)
+			{
 				goto err;
+			}
 
 			ata_eh_done(link, dev, ATA_EH_REVALIDATE);
 
diff --git a/drivers/ata/libata.h b/drivers/ata/libata.h
index 43ad1ef9b63a..cff3facad055 100644
--- a/drivers/ata/libata.h
+++ b/drivers/ata/libata.h
@@ -19,6 +19,7 @@
 enum {
 	/* flags for ata_dev_read_id() */
 	ATA_READID_POSTRESET	= (1 << 0), /* reading ID after reset */
+	ATA_READID_NOSTART	= (1 << 1), /* do not start drive */
 
 	/* selector for ata_down_xfermask_limit() */
 	ATA_DNXFER_PIO		= 0,	/* speed down PIO */
diff --git a/include/linux/libata.h b/include/linux/libata.h
index 1dbb14daccfa..50d6fa933946 100644
--- a/include/linux/libata.h
+++ b/include/linux/libata.h
@@ -328,6 +328,7 @@ enum {
 
 	/* ata_eh_info->flags */
 	ATA_EHI_HOTPLUGGED	= (1 << 0),  /* could have been hotplugged */
+	ATA_EHI_NOSTART		= (1 << 1),  /* don't start the disk */
 	ATA_EHI_NO_AUTOPSY	= (1 << 2),  /* no autopsy */
 	ATA_EHI_QUIET		= (1 << 3),  /* be quiet */
 	ATA_EHI_NO_RECOVERY	= (1 << 4),  /* no recovery */
-- 
2.30.2

Re: [PATCH 3/3] libata: don't start PuiS disks on resume

[Damien Le Moal]

On 1/8/24 03:02, Phillip Susi wrote:
> Disks with Power Up In Standby enabled that required the
> SET FEATURES command to start up were being issued the
> command during resume.  Recently this was extended to
> include PuiS disks that don't require the SET FEATURES
> command.  Suppress this and leave the disk in standby
> until the disk is actually accessed.

Please use full 72-char lines for commit messages. The commit message also does
not clearly describe what the patch does (completely silent on forcing the drive
to sleep).

But this patch is anyway not acceptable (see below) and out of place in this
series (it is not about sleep).

> ---
>  drivers/ata/libata-core.c | 32 ++++++++++++++++++++++++++++----
>  drivers/ata/libata-eh.c   | 12 +++++++++++-
>  drivers/ata/libata.h      |  1 +
>  include/linux/libata.h    |  1 +
>  4 files changed, 41 insertions(+), 5 deletions(-)
> 
> diff --git a/drivers/ata/libata-core.c b/drivers/ata/libata-core.c
> index ef6a2349a6f8..f758fc88ac19 100644
> --- a/drivers/ata/libata-core.c
> +++ b/drivers/ata/libata-core.c
> @@ -1912,7 +1912,30 @@ int ata_dev_read_id(struct ata_device *dev, unsigned int *p_class,
>  			goto err_out;
>  	}
>  
> -	if (!tried_spinup && (id[2] == 0x37c8 || id[2] == 0x738c)) {
> +	if (flags & ATA_READID_NOSTART && id[2] == 0x37c8)
> +	{
> +		/*
> +		 * The drive has powered up in standby, and returned incomplete IDENTIFY info
> +		 * so we can't revalidate it yet.  We have also been asked to avoid starting the
> +		 * drive, so stop here and leave the drive asleep and the EH pending, to be
> +		 * restarted on later I/O request
> +		 */
> +#if 0
> +		ata_tf_init(dev, &tf);
> +		tf.flags |= ATA_TFLAG_DEVICE | ATA_TFLAG_ISADDR;
> +		tf.protocol = ATA_PROT_NODATA;
> +		tf.command = ATA_CMD_SLEEP;
> +		err_mask = ata_exec_internal(dev, &tf, NULL, DMA_NONE, NULL, 0, 0);
> +		ata_dev_info(dev, "PuiS detected, putting drive to sleep");

I already commented that this is not following the ACS specifications and thus
should not be done. So again, nack.

> +#else
> +		dev->flags |= ATA_DFLAG_SLEEPING;
> +		ata_dev_info(dev, "PuiS detected, leaving drive in standby");
> +#endif
> +		return -EAGAIN;
> +	}
> +
> +	if (!(flags & ATA_READID_NOSTART) &&
> +	    !tried_spinup && (id[2] == 0x37c8 || id[2] == 0x738c)) {
>  		tried_spinup = 1;
>  		/*
>  		 * Drive powered-up in standby mode, and requires a specific
> @@ -3969,6 +3992,8 @@ int ata_dev_revalidate(struct ata_device *dev, unsigned int new_class,
>  
>  	/* re-read ID */
>  	rc = ata_dev_reread_id(dev, readid_flags);
> +	if (rc == -EAGAIN)
> +		return rc;
>  	if (rc)
>  		goto fail;
>  
> @@ -5241,8 +5266,7 @@ static void ata_port_suspend(struct ata_port *ap, pm_message_t mesg,
>  	 * http://thread.gmane.org/gmane.linux.ide/46764
>  	 */
>  	ata_port_request_pm(ap, mesg, 0,
> -			    ATA_EHI_QUIET | ATA_EHI_NO_AUTOPSY |
> -			    ATA_EHI_NO_RECOVERY,
> +			    ATA_EHI_QUIET | ATA_EHI_NO_AUTOPSY | ATA_EHI_NO_RECOVERY,
>  			    async);
>  }
>  
> @@ -5279,7 +5303,7 @@ static void ata_port_resume(struct ata_port *ap, pm_message_t mesg,
>  			    bool async)
>  {
>  	ata_port_request_pm(ap, mesg, ATA_EH_RESET | ATA_EH_SET_ACTIVE,
> -			    ATA_EHI_NO_AUTOPSY | ATA_EHI_QUIET,
> +			    ATA_EHI_NO_AUTOPSY | ATA_EHI_QUIET | ATA_EHI_NOSTART,
>  			    async);
>  }
>  
> diff --git a/drivers/ata/libata-eh.c b/drivers/ata/libata-eh.c
> index 799a1b8bc384..e45e60112951 100644
> --- a/drivers/ata/libata-eh.c
> +++ b/drivers/ata/libata-eh.c
> @@ -22,6 +22,7 @@
>  #include <scsi/scsi_cmnd.h>
>  #include <scsi/scsi_dbg.h>
>  #include "../scsi/scsi_transport_api.h"
> +#include <linux/pm_runtime.h>
>  
>  #include <linux/libata.h>
>  
> @@ -3046,6 +3047,8 @@ static int ata_eh_revalidate_and_attach(struct ata_link *link,
>  
>  		if (ehc->i.flags & ATA_EHI_DID_RESET)
>  			readid_flags |= ATA_READID_POSTRESET;
> +		if (ehc->i.flags & ATA_EHI_NOSTART)
> +			readid_flags |= ATA_READID_NOSTART;
>  
>  		if ((action & ATA_EH_REVALIDATE) && ata_dev_enabled(dev)) {
>  			WARN_ON(dev->class == ATA_DEV_PMP);
> @@ -3075,8 +3078,15 @@ static int ata_eh_revalidate_and_attach(struct ata_link *link,
>  			ata_eh_about_to_do(link, dev, ATA_EH_REVALIDATE);
>  			rc = ata_dev_revalidate(dev, ehc->classes[dev->devno],
>  						readid_flags);
> -			if (rc)
> +			if (rc == -EAGAIN) {
> +				rc = 0; /* start required but suppressed, handle later */
> +				ehc->i.dev_action[dev->devno] &= ~ATA_EH_SET_ACTIVE;
> +				continue;
> +			}
> +			else if (rc)
> +			{
>  				goto err;
> +			}
>  
>  			ata_eh_done(link, dev, ATA_EH_REVALIDATE);
>  
> diff --git a/drivers/ata/libata.h b/drivers/ata/libata.h
> index 43ad1ef9b63a..cff3facad055 100644
> --- a/drivers/ata/libata.h
> +++ b/drivers/ata/libata.h
> @@ -19,6 +19,7 @@
>  enum {
>  	/* flags for ata_dev_read_id() */
>  	ATA_READID_POSTRESET	= (1 << 0), /* reading ID after reset */
> +	ATA_READID_NOSTART	= (1 << 1), /* do not start drive */
>  
>  	/* selector for ata_down_xfermask_limit() */
>  	ATA_DNXFER_PIO		= 0,	/* speed down PIO */
> diff --git a/include/linux/libata.h b/include/linux/libata.h
> index 1dbb14daccfa..50d6fa933946 100644
> --- a/include/linux/libata.h
> +++ b/include/linux/libata.h
> @@ -328,6 +328,7 @@ enum {
>  
>  	/* ata_eh_info->flags */
>  	ATA_EHI_HOTPLUGGED	= (1 << 0),  /* could have been hotplugged */
> +	ATA_EHI_NOSTART		= (1 << 1),  /* don't start the disk */
>  	ATA_EHI_NO_AUTOPSY	= (1 << 2),  /* no autopsy */
>  	ATA_EHI_QUIET		= (1 << 3),  /* be quiet */
>  	ATA_EHI_NO_RECOVERY	= (1 << 4),  /* no recovery */

-- 
Damien Le Moal
Western Digital Research

Re: [PATCH 3/3] libata: don't start PuiS disks on resume

[Phillip Susi]

Damien Le Moal <dlemoal@kernel.org> writes:

> Please use full 72-char lines for commit messages. The commit message also does
> not clearly describe what the patch does (completely silent on forcing the drive
> to sleep).

It currently doesn't put it to sleep.

>> +#if 0
>> +		ata_tf_init(dev, &tf);
>> +		tf.flags |= ATA_TFLAG_DEVICE | ATA_TFLAG_ISADDR;
>> +		tf.protocol = ATA_PROT_NODATA;
>> +		tf.command = ATA_CMD_SLEEP;
>> +		err_mask = ata_exec_internal(dev, &tf, NULL, DMA_NONE, NULL, 0, 0);
>> +		ata_dev_info(dev, "PuiS detected, putting drive to sleep");
>
> I already commented that this is not following the ACS specifications and thus
> should not be done. So again, nack.

It is #if 0'd out.  I also addressed this in the cover letter.  Sure,
this shouldn't be done by default, but I don't see a problem with
leaving it as an option that can be activated by those whose drives
don't have a problem with this.

Re: [PATCH 3/3] libata: don't start PuiS disks on resume

[Damien Le Moal]

On 1/8/24 22:39, Phillip Susi wrote:
> Damien Le Moal <dlemoal@kernel.org> writes:
> 
>> Please use full 72-char lines for commit messages. The commit message also does
>> not clearly describe what the patch does (completely silent on forcing the drive
>> to sleep).
> 
> It currently doesn't put it to sleep.
> 
>>> +#if 0
>>> +		ata_tf_init(dev, &tf);
>>> +		tf.flags |= ATA_TFLAG_DEVICE | ATA_TFLAG_ISADDR;
>>> +		tf.protocol = ATA_PROT_NODATA;
>>> +		tf.command = ATA_CMD_SLEEP;
>>> +		err_mask = ata_exec_internal(dev, &tf, NULL, DMA_NONE, NULL, 0, 0);
>>> +		ata_dev_info(dev, "PuiS detected, putting drive to sleep");
>>
>> I already commented that this is not following the ACS specifications and thus
>> should not be done. So again, nack.
> 
> It is #if 0'd out.  I also addressed this in the cover letter.  Sure,
> this shouldn't be done by default, but I don't see a problem with
> leaving it as an option that can be activated by those whose drives
> don't have a problem with this.

We never add dead code. And code under a "#if 0" is by design dead...
So please do not do that.

-- 
Damien Le Moal
Western Digital Research

Re: [PATCH 3/3] libata: don't start PuiS disks on resume

[Phillip Susi]

Damien Le Moal <dlemoal@kernel.org> writes:

> We never add dead code. And code under a "#if 0" is by design dead...
> So please do not do that.

I left it that way temporarily so you could switch it to an #if 1 if you
wanted to test it with your army of drives to see if any of them don't
like it, and to see where I was proposing a runtime selection knob to
switch which branch the code would take there.

Would you be OK with leaving the PuiS -> SLEEP transition code in, but
disabled by default?  Then people who know their drives are OK with it
can choose to enable it.

[PATCH 4/4] libata: don't start PuiS disks on resume

[Phillip Susi]

Disks with Power Up In Standby enabled that required the
SET FEATURES command to start up were being issued the
command during resume.  Suppress this until the disk
is actually accessed.
---
 drivers/ata/libata-core.c | 24 ++++++++++++++++++++++--
 drivers/ata/libata-eh.c   | 12 +++++++++++-
 drivers/ata/libata.h      |  1 +
 include/linux/libata.h    |  1 +
 4 files changed, 35 insertions(+), 3 deletions(-)

diff --git a/drivers/ata/libata-core.c b/drivers/ata/libata-core.c
index d9e889fa2881..3f6187c75b16 100644
--- a/drivers/ata/libata-core.c
+++ b/drivers/ata/libata-core.c
@@ -1912,7 +1912,25 @@ int ata_dev_read_id(struct ata_device *dev, unsigned int *p_class,
 			goto err_out;
 	}
 
-	if (!tried_spinup && (id[2] == 0x37c8 || id[2] == 0x738c)) {
+	if (flags & ATA_READID_NOSTART && id[2] == 0x37c8)
+	{
+		/*
+		 * the drive has powered up in standby, and returned incomplete IDENTIFY info
+		 * so we can't revalidate it yet.  We have also been asked to avoid starting the
+		 * drive, so stop  here and leave the drive asleep and the EH pending, to be
+		 * restarted on later IO request
+		 */
+		ata_tf_init(dev, &tf);
+		tf.flags |= ATA_TFLAG_DEVICE | ATA_TFLAG_ISADDR;
+		tf.protocol = ATA_PROT_NODATA;
+		tf.command = ATA_CMD_SLEEP;
+		err_mask = ata_exec_internal(dev, &tf, NULL, DMA_NONE, NULL, 0, 0);
+		ata_dev_info(dev, "PuiS detected, putting drive to sleep");
+		return -EAGAIN;
+	}
+
+	if (!(flags & ATA_READID_NOSTART) &&
+	    !tried_spinup && (id[2] == 0x37c8 || id[2] == 0x738c)) {
 		tried_spinup = 1;
 		/*
 		 * Drive powered-up in standby mode, and requires a specific
@@ -3973,6 +3991,8 @@ int ata_dev_revalidate(struct ata_device *dev, unsigned int new_class,
 
 	/* re-read ID */
 	rc = ata_dev_reread_id(dev, readid_flags);
+	if (rc == -EAGAIN)
+		return rc;
 	if (rc)
 		goto fail;
 
@@ -5241,7 +5261,7 @@ static void ata_port_suspend(struct ata_port *ap, pm_message_t mesg,
 	 * http://thread.gmane.org/gmane.linux.ide/46764
 	 */
 	ata_port_request_pm(ap, mesg, 0,
-			    ATA_EHI_QUIET | ATA_EHI_NO_AUTOPSY |
+			    ATA_EHI_QUIET | ATA_EHI_NO_AUTOPSY | ATA_EHI_NOSTART
 			    ATA_EHI_NO_RECOVERY,
 			    async);
 }
diff --git a/drivers/ata/libata-eh.c b/drivers/ata/libata-eh.c
index 799a1b8bc384..74e0d54a204e 100644
--- a/drivers/ata/libata-eh.c
+++ b/drivers/ata/libata-eh.c
@@ -22,6 +22,7 @@
 #include <scsi/scsi_cmnd.h>
 #include <scsi/scsi_dbg.h>
 #include "../scsi/scsi_transport_api.h"
+#include <linux/pm_runtime.h>
 
 #include <linux/libata.h>
 
@@ -3046,6 +3047,8 @@ static int ata_eh_revalidate_and_attach(struct ata_link *link,
 
 		if (ehc->i.flags & ATA_EHI_DID_RESET)
 			readid_flags |= ATA_READID_POSTRESET;
+		if (ehc->i.flags & ATA_EHI_NOSTART)
+			readid_flags |= ATA_READID_NOSTART;
 
 		if ((action & ATA_EH_REVALIDATE) && ata_dev_enabled(dev)) {
 			WARN_ON(dev->class == ATA_DEV_PMP);
@@ -3075,9 +3078,16 @@ static int ata_eh_revalidate_and_attach(struct ata_link *link,
 			ata_eh_about_to_do(link, dev, ATA_EH_REVALIDATE);
 			rc = ata_dev_revalidate(dev, ehc->classes[dev->devno],
 						readid_flags);
-			if (rc)
+			if (rc == -EAGAIN) {
+				rc = 0; /* start required but suppressed, handle later */
+				ehc->i.dev_action[dev->devno] &= ~ATA_EH_SET_ACTIVE;
+				ata_dev_warn(dev, "Leaving PuiS disk asleep for now");
+				continue;
+			}
+			else if (rc)
 				goto err;
 
+			pm_runtime_resume(&dev->sdev->sdev_gendev);
 			ata_eh_done(link, dev, ATA_EH_REVALIDATE);
 
 			/* Configuration may have changed, reconfigure
diff --git a/drivers/ata/libata.h b/drivers/ata/libata.h
index 43ad1ef9b63a..cff3facad055 100644
--- a/drivers/ata/libata.h
+++ b/drivers/ata/libata.h
@@ -19,6 +19,7 @@
 enum {
 	/* flags for ata_dev_read_id() */
 	ATA_READID_POSTRESET	= (1 << 0), /* reading ID after reset */
+	ATA_READID_NOSTART	= (1 << 1), /* do not start drive */
 
 	/* selector for ata_down_xfermask_limit() */
 	ATA_DNXFER_PIO		= 0,	/* speed down PIO */
diff --git a/include/linux/libata.h b/include/linux/libata.h
index 1dbb14daccfa..50d6fa933946 100644
--- a/include/linux/libata.h
+++ b/include/linux/libata.h
@@ -328,6 +328,7 @@ enum {
 
 	/* ata_eh_info->flags */
 	ATA_EHI_HOTPLUGGED	= (1 << 0),  /* could have been hotplugged */
+	ATA_EHI_NOSTART		= (1 << 1),  /* don't start the disk */
 	ATA_EHI_NO_AUTOPSY	= (1 << 2),  /* no autopsy */
 	ATA_EHI_QUIET		= (1 << 3),  /* be quiet */
 	ATA_EHI_NO_RECOVERY	= (1 << 4),  /* no recovery */
-- 
2.30.2

Re: [PATCH 4/4] libata: don't start PuiS disks on resume

[Sergei Shtylyov]

On 1/5/24 1:39 AM, Phillip Susi wrote:

> Disks with Power Up In Standby enabled that required the
> SET FEATURES command to start up were being issued the
> command during resume.  Suppress this until the disk
> is actually accessed.
> ---
>  drivers/ata/libata-core.c | 24 ++++++++++++++++++++++--
>  drivers/ata/libata-eh.c   | 12 +++++++++++-
>  drivers/ata/libata.h      |  1 +
>  include/linux/libata.h    |  1 +
>  4 files changed, 35 insertions(+), 3 deletions(-)
> 
> diff --git a/drivers/ata/libata-core.c b/drivers/ata/libata-core.c
> index d9e889fa2881..3f6187c75b16 100644
> --- a/drivers/ata/libata-core.c
> +++ b/drivers/ata/libata-core.c
> @@ -1912,7 +1912,25 @@ int ata_dev_read_id(struct ata_device *dev, unsigned int *p_class,
>  			goto err_out;
>  	}
>  
> -	if (!tried_spinup && (id[2] == 0x37c8 || id[2] == 0x738c)) {
> +	if (flags & ATA_READID_NOSTART && id[2] == 0x37c8)
> +	{
> +		/*
> +		 * the drive has powered up in standby, and returned incomplete IDENTIFY info

   s/the/The/.

> +		 * so we can't revalidate it yet.  We have also been asked to avoid starting the
> +		 * drive, so stop  here and leave the drive asleep and the EH pending, to be

   Double space...

> +		 * restarted on later IO request

   I think more common form is I/O.

[...]
> diff --git a/drivers/ata/libata-eh.c b/drivers/ata/libata-eh.c
> index 799a1b8bc384..74e0d54a204e 100644
> --- a/drivers/ata/libata-eh.c
> +++ b/drivers/ata/libata-eh.c
[...]
> @@ -3075,9 +3078,16 @@ static int ata_eh_revalidate_and_attach(struct ata_link *link,
>  			ata_eh_about_to_do(link, dev, ATA_EH_REVALIDATE);
>  			rc = ata_dev_revalidate(dev, ehc->classes[dev->devno],
>  						readid_flags);
> -			if (rc)
> +			if (rc == -EAGAIN) {
> +				rc = 0; /* start required but suppressed, handle later */
> +				ehc->i.dev_action[dev->devno] &= ~ATA_EH_SET_ACTIVE;
> +				ata_dev_warn(dev, "Leaving PuiS disk asleep for now");
> +				continue;
> +			}
> +			else if (rc)

			} else if (rc) {

>  				goto err;

			}

   You need {} on all *if* branches if at least one has it, as dictated
by the kernel coding style...

[...]

MBR, Sergey

Re: [PATCH 4/4] libata: don't start PuiS disks on resume

[Damien Le Moal]

On 1/5/24 07:39, Phillip Susi wrote:
> Disks with Power Up In Standby enabled that required the
> SET FEATURES command to start up were being issued the
> command during resume.  Suppress this until the disk
> is actually accessed.
> ---
>  drivers/ata/libata-core.c | 24 ++++++++++++++++++++++--
>  drivers/ata/libata-eh.c   | 12 +++++++++++-
>  drivers/ata/libata.h      |  1 +
>  include/linux/libata.h    |  1 +
>  4 files changed, 35 insertions(+), 3 deletions(-)
> 
> diff --git a/drivers/ata/libata-core.c b/drivers/ata/libata-core.c
> index d9e889fa2881..3f6187c75b16 100644
> --- a/drivers/ata/libata-core.c
> +++ b/drivers/ata/libata-core.c
> @@ -1912,7 +1912,25 @@ int ata_dev_read_id(struct ata_device *dev, unsigned int *p_class,
>  			goto err_out;
>  	}
>  
> -	if (!tried_spinup && (id[2] == 0x37c8 || id[2] == 0x738c)) {
> +	if (flags & ATA_READID_NOSTART && id[2] == 0x37c8)
> +	{
> +		/*
> +		 * the drive has powered up in standby, and returned incomplete IDENTIFY info
> +		 * so we can't revalidate it yet.  We have also been asked to avoid starting the
> +		 * drive, so stop  here and leave the drive asleep and the EH pending, to be
> +		 * restarted on later IO request
> +		 */
> +		ata_tf_init(dev, &tf);
> +		tf.flags |= ATA_TFLAG_DEVICE | ATA_TFLAG_ISADDR;
> +		tf.protocol = ATA_PROT_NODATA;
> +		tf.command = ATA_CMD_SLEEP;
> +		err_mask = ata_exec_internal(dev, &tf, NULL, DMA_NONE, NULL, 0, 0);
> +		ata_dev_info(dev, "PuiS detected, putting drive to sleep");

Why ? The drive is in standby. What is the point of putting it into sleep state
? Furthermore, if you check ACS-6 specs, you will see that there is no
transitions defined from PUIS state to sleep state. You have to spin-up the
drive first. So the above is outside the specified behavior and thus not
reliable (even though many drive may actually allow this transition).

> +		return -EAGAIN;
> +	}
> +
> +	if (!(flags & ATA_READID_NOSTART) &&
> +	    !tried_spinup && (id[2] == 0x37c8 || id[2] == 0x738c)) {
>  		tried_spinup = 1;
>  		/*
>  		 * Drive powered-up in standby mode, and requires a specific
> @@ -3973,6 +3991,8 @@ int ata_dev_revalidate(struct ata_device *dev, unsigned int new_class,
>  
>  	/* re-read ID */
>  	rc = ata_dev_reread_id(dev, readid_flags);
> +	if (rc == -EAGAIN)
> +		return rc;
>  	if (rc)
>  		goto fail;
>  
> @@ -5241,7 +5261,7 @@ static void ata_port_suspend(struct ata_port *ap, pm_message_t mesg,
>  	 * http://thread.gmane.org/gmane.linux.ide/46764
>  	 */
>  	ata_port_request_pm(ap, mesg, 0,
> -			    ATA_EHI_QUIET | ATA_EHI_NO_AUTOPSY |
> +			    ATA_EHI_QUIET | ATA_EHI_NO_AUTOPSY | ATA_EHI_NOSTART
>  			    ATA_EHI_NO_RECOVERY,
>  			    async);
>  }
> diff --git a/drivers/ata/libata-eh.c b/drivers/ata/libata-eh.c
> index 799a1b8bc384..74e0d54a204e 100644
> --- a/drivers/ata/libata-eh.c
> +++ b/drivers/ata/libata-eh.c
> @@ -22,6 +22,7 @@
>  #include <scsi/scsi_cmnd.h>
>  #include <scsi/scsi_dbg.h>
>  #include "../scsi/scsi_transport_api.h"
> +#include <linux/pm_runtime.h>
>  
>  #include <linux/libata.h>
>  
> @@ -3046,6 +3047,8 @@ static int ata_eh_revalidate_and_attach(struct ata_link *link,
>  
>  		if (ehc->i.flags & ATA_EHI_DID_RESET)
>  			readid_flags |= ATA_READID_POSTRESET;
> +		if (ehc->i.flags & ATA_EHI_NOSTART)
> +			readid_flags |= ATA_READID_NOSTART;
>  
>  		if ((action & ATA_EH_REVALIDATE) && ata_dev_enabled(dev)) {
>  			WARN_ON(dev->class == ATA_DEV_PMP);
> @@ -3075,9 +3078,16 @@ static int ata_eh_revalidate_and_attach(struct ata_link *link,
>  			ata_eh_about_to_do(link, dev, ATA_EH_REVALIDATE);
>  			rc = ata_dev_revalidate(dev, ehc->classes[dev->devno],
>  						readid_flags);
> -			if (rc)
> +			if (rc == -EAGAIN) {

Rather than playing with the return values, it may be easier to use a device
flag (similar to ATA_DFLAG_SLEEPING) to track standby/spun-down state.

> +				rc = 0; /* start required but suppressed, handle later */
> +				ehc->i.dev_action[dev->devno] &= ~ATA_EH_SET_ACTIVE;
> +				ata_dev_warn(dev, "Leaving PuiS disk asleep for now");
> +				continue;
> +			}
> +			else if (rc)
>  				goto err;
>  
> +			pm_runtime_resume(&dev->sdev->sdev_gendev);

What does this do ??? This look really out of place.

>  			ata_eh_done(link, dev, ATA_EH_REVALIDATE);
>  
>  			/* Configuration may have changed, reconfigure
> diff --git a/drivers/ata/libata.h b/drivers/ata/libata.h
> index 43ad1ef9b63a..cff3facad055 100644
> --- a/drivers/ata/libata.h
> +++ b/drivers/ata/libata.h
> @@ -19,6 +19,7 @@
>  enum {
>  	/* flags for ata_dev_read_id() */
>  	ATA_READID_POSTRESET	= (1 << 0), /* reading ID after reset */
> +	ATA_READID_NOSTART	= (1 << 1), /* do not start drive */
>  
>  	/* selector for ata_down_xfermask_limit() */
>  	ATA_DNXFER_PIO		= 0,	/* speed down PIO */
> diff --git a/include/linux/libata.h b/include/linux/libata.h
> index 1dbb14daccfa..50d6fa933946 100644
> --- a/include/linux/libata.h
> +++ b/include/linux/libata.h
> @@ -328,6 +328,7 @@ enum {
>  
>  	/* ata_eh_info->flags */
>  	ATA_EHI_HOTPLUGGED	= (1 << 0),  /* could have been hotplugged */
> +	ATA_EHI_NOSTART		= (1 << 1),  /* don't start the disk */
>  	ATA_EHI_NO_AUTOPSY	= (1 << 2),  /* no autopsy */
>  	ATA_EHI_QUIET		= (1 << 3),  /* be quiet */
>  	ATA_EHI_NO_RECOVERY	= (1 << 4),  /* no recovery */

-- 
Damien Le Moal
Western Digital Research

Re: [PATCH 4/4] libata: don't start PuiS disks on resume

[Phillip Susi]

Damien Le Moal <dlemoal@kernel.org> writes:

> Why ? The drive is in standby. What is the point of putting it into
> sleep state

I just figured it would save a little more power than just *pretending*
the drive was asleep ( which is what I did at first ).  I guess I can go
back to just setting the sleeping flag without telling the drive to
actually go to sleep.

> ? Furthermore, if you check ACS-6 specs, you will see that there is no
> transitions defined from PUIS state to sleep state. You have to spin-up the
> drive first. So the above is outside the specified behavior and thus not
> reliable (even though many drive may actually allow this transition).

Oh poo.  Though now I'm curious if it actually does fail on any.  It
seems like a pretty obvious thing to do and an oversight on the part of
the spec.  The kernel certainly doesn't do anything to prevent the user
from running hdparm -Y on a drive that is already in standby.

Wait, is that specifically from PuiS to sleep, or standby to sleep in general?

> Rather than playing with the return values, it may be easier to use a device
> flag (similar to ATA_DFLAG_SLEEPING) to track standby/spun-down state.

You mean change each if (rc == -EAGAIN) to if (ATA_DFLAG_SLEEPING)? That
doesn't seem any easier to me, but I'm not opposed to it.

>> +			pm_runtime_resume(&dev->sdev->sdev_gendev);
>
> What does this do ??? This look really out of place.

Darnit, I thought I had removed all of the runtime pm stuff.  My bad.

Re: [PATCH 1/4] libata: only wake a drive once on system resume

[Damien Le Moal]

On 1/5/24 07:39, Phillip Susi wrote:
> In the event that more than one pass of EH is needed during system resume,
> only request the drive be started once.
> ---
>  drivers/ata/libata-core.c | 9 +++++----
>  drivers/ata/libata-eh.c   | 8 +++-----
>  drivers/ata/libata.h      | 2 +-
>  3 files changed, 9 insertions(+), 10 deletions(-)
> 
> diff --git a/drivers/ata/libata-core.c b/drivers/ata/libata-core.c
> index 09ed67772fae..a2d8cc0097a8 100644
> --- a/drivers/ata/libata-core.c
> +++ b/drivers/ata/libata-core.c
> @@ -2080,7 +2080,7 @@ static bool ata_dev_power_is_active(struct ata_device *dev)
>   *	LOCKING:
>   *	Kernel thread context (may sleep).
>   */
> -void ata_dev_power_set_active(struct ata_device *dev)
> +unsigned int ata_dev_power_set_active(struct ata_device *dev)
>  {
>  	struct ata_taskfile tf;
>  	unsigned int err_mask;
> @@ -2090,14 +2090,14 @@ void ata_dev_power_set_active(struct ata_device *dev)
>  	 * if supported by the device.
>  	 */
>  	if (!ata_dev_power_init_tf(dev, &tf, true))
> -		return;
> +		return AC_ERR_OTHER;

Nope. This is wrong. ata_dev_power_init_tf() returns a bool, not an error. The
bool indicates if the drive supports power management.

But beside this, I still do not understand what this fixes... Calling again
ata_dev_power_set_active() will do nothing but issue a check power mode command
if the drive is already active. So I do not see the need for this added complexity.

>  
>  	/*
>  	 * 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;
> +		return AC_ERR_OK;
>  
>  	ata_dev_notice(dev, "Entering active power mode\n");
>  
> @@ -2105,6 +2105,7 @@ void ata_dev_power_set_active(struct ata_device *dev)
>  	if (err_mask)
>  		ata_dev_err(dev, "VERIFY failed (err_mask=0x%x)\n",
>  			    err_mask);
> +	return err_mask;
>  }
>  
>  /**
> @@ -5257,7 +5258,7 @@ static int ata_port_pm_poweroff(struct device *dev)
>  static void ata_port_resume(struct ata_port *ap, pm_message_t mesg,
>  			    bool async)
>  {
> -	ata_port_request_pm(ap, mesg, ATA_EH_RESET,
> +	ata_port_request_pm(ap, mesg, ATA_EH_RESET | ATA_EH_SET_ACTIVE,
>  			    ATA_EHI_NO_AUTOPSY | ATA_EHI_QUIET,
>  			    async);
>  }
> diff --git a/drivers/ata/libata-eh.c b/drivers/ata/libata-eh.c
> index b0d6e69c4a5b..799a1b8bc384 100644
> --- a/drivers/ata/libata-eh.c
> +++ b/drivers/ata/libata-eh.c
> @@ -710,10 +710,6 @@ void ata_scsi_port_error_handler(struct Scsi_Host *host, struct ata_port *ap)
>  			ehc->saved_xfer_mode[devno] = dev->xfer_mode;
>  			if (ata_ncq_enabled(dev))
>  				ehc->saved_ncq_enabled |= 1 << devno;
> -
> -			/* If we are resuming, wake up the device */
> -			if (ap->pflags & ATA_PFLAG_RESUMING)
> -				ehc->i.dev_action[devno] |= ATA_EH_SET_ACTIVE;
>  		}
>  	}
>  
> @@ -3853,7 +3849,9 @@ int ata_eh_recover(struct ata_port *ap, ata_prereset_fn_t prereset,
>  		 */
>  		ata_for_each_dev(dev, link, ENABLED) {
>  			if (ehc->i.dev_action[dev->devno] & ATA_EH_SET_ACTIVE) {
> -				ata_dev_power_set_active(dev);
> +				unsigned int err_mask = ata_dev_power_set_active(dev);
> +				if (err_mask)
> +					link->eh_info.dev_action[dev->devno] |= ATA_EH_SET_ACTIVE;
>  				ata_eh_done(link, dev, ATA_EH_SET_ACTIVE);
>  			}
>  		}
> diff --git a/drivers/ata/libata.h b/drivers/ata/libata.h
> index 5c685bb1939e..43ad1ef9b63a 100644
> --- a/drivers/ata/libata.h
> +++ b/drivers/ata/libata.h
> @@ -65,7 +65,7 @@ extern int ata_dev_configure(struct ata_device *dev);
>  extern bool ata_dev_power_init_tf(struct ata_device *dev,
>  				  struct ata_taskfile *tf, bool set_active);
>  extern void ata_dev_power_set_standby(struct ata_device *dev);
> -extern void ata_dev_power_set_active(struct ata_device *dev);
> +extern unsigned int ata_dev_power_set_active(struct ata_device *dev);
>  extern int sata_down_spd_limit(struct ata_link *link, u32 spd_limit);
>  extern int ata_down_xfermask_limit(struct ata_device *dev, unsigned int sel);
>  extern unsigned int ata_dev_set_feature(struct ata_device *dev,

-- 
Damien Le Moal
Western Digital Research

Re: [PATCH 1/4] libata: only wake a drive once on system resume

[Phillip Susi]

Damien Le Moal <dlemoal@kernel.org> writes:

>> -		return;
>> +		return AC_ERR_OTHER;
>
> Nope. This is wrong. ata_dev_power_init_tf() returns a bool, not an error. The
> bool indicates if the drive supports power management.

Are you saying it should return AC_ERR_OK?  If the drive doesn't support
power management at all, isn't it an error to try to change its power
management state?

> But beside this, I still do not understand what this fixes... Calling again
> ata_dev_power_set_active() will do nothing but issue a check power mode command
> if the drive is already active. So I do not see the need for this added complexity.

If the drive is NOT active because it is PuiS, it would try to start the drive
anyhow, despite the PuiS actively clearing the ATA_EH_SET_ACTVE flag.
Then the VERIFY command fails if the drive requires the SET FEATURES
command to leave PuiS.

My goal here was to make sure that when the PuiS patch clears
ATA_EH_SET_ACTIVE, that it does not get turned back on during a second
round of EH.  Then you pointed out that it SHOULD try on the second
round, if the first attempt failed, so I changed the return to be able
to detect the error, and turn ATA_EH_SET_ACTIVE on for the second round.

It occurs to me now that I only ran into this issue once I changed to
actually giving the drive the SLEEP command instead of just setting the
sleeping flag.  Once the drive actually went to sleep, it shut down the
sata link, which caused an error interrupt, which triggered a second
round of EH, which then issued the failing VERIFY command until all 5
rounds were used and it gave up.  So if I go back to just setting the
sleeping flag instead of actually putting the drive to sleep, I would
not get the error interrupt and would be fine without this patch.  But I
can imagine some other cause for a second round of EH on some system
that would still run into this problem.

Re: [PATCH 1/4] libata: only wake a drive once on system resume

[Damien Le Moal]

On 1/6/24 02:03, Phillip Susi wrote:
> Damien Le Moal <dlemoal@kernel.org> writes:
> 
>>> -		return;
>>> +		return AC_ERR_OTHER;
>>
>> Nope. This is wrong. ata_dev_power_init_tf() returns a bool, not an error. The
>> bool indicates if the drive supports power management.
> 
> Are you saying it should return AC_ERR_OK?  If the drive doesn't support
> power management at all, isn't it an error to try to change its power
> management state?

That is why the function returns doing nothing if ata_dev_power_init_tf()
returns false, meaning "do not do power management". See that function. It
includes ATAPI devices (e.g. CD/DVD) which do not have power management.

>> But beside this, I still do not understand what this fixes... Calling again
>> ata_dev_power_set_active() will do nothing but issue a check power mode command
>> if the drive is already active. So I do not see the need for this added complexity.
> 
> If the drive is NOT active because it is PuiS, it would try to start the drive
> anyhow, despite the PuiS actively clearing the ATA_EH_SET_ACTVE flag.
> Then the VERIFY command fails if the drive requires the SET FEATURES
> command to leave PuiS.

Define a device flag to indicate that the drive has PUIS "on" and so woke up in
standby, e.g. ATA_DFLAG_PUIS_STANDBY. Check that flag in
ata_dev_power_set_active(), similarly to the ATA_DFLAG_SLEEPING flag and return
doing nothing if the puis flag is set. Way easier than playing games with the
return value.

> My goal here was to make sure that when the PuiS patch clears
> ATA_EH_SET_ACTIVE, that it does not get turned back on during a second
> round of EH.  Then you pointed out that it SHOULD try on the second
> round, if the first attempt failed, so I changed the return to be able
> to detect the error, and turn ATA_EH_SET_ACTIVE on for the second round.

See above.

> It occurs to me now that I only ran into this issue once I changed to
> actually giving the drive the SLEEP command instead of just setting the
> sleeping flag.  Once the drive actually went to sleep, it shut down the
> sata link, which caused an error interrupt, which triggered a second
> round of EH, which then issued the failing VERIFY command until all 5
> rounds were used and it gave up.  So if I go back to just setting the
> sleeping flag instead of actually putting the drive to sleep, I would
> not get the error interrupt and would be fine without this patch.  But I
> can imagine some other cause for a second round of EH on some system
> that would still run into this problem.

Please separate handling of sleep and puis. The former can be used even if puis
is off.

-- 
Damien Le Moal
Western Digital Research

Re: [PATCH 1/4] libata: only wake a drive once on system resume

[Damien Le Moal]

On 1/5/24 07:39, Phillip Susi wrote:
> In the event that more than one pass of EH is needed during system resume,
> only request the drive be started once.

Please also add a cover letter that explain the problem that these patches all
together solve.


> ---
>  drivers/ata/libata-core.c | 9 +++++----
>  drivers/ata/libata-eh.c   | 8 +++-----
>  drivers/ata/libata.h      | 2 +-
>  3 files changed, 9 insertions(+), 10 deletions(-)
> 
> diff --git a/drivers/ata/libata-core.c b/drivers/ata/libata-core.c
> index 09ed67772fae..a2d8cc0097a8 100644
> --- a/drivers/ata/libata-core.c
> +++ b/drivers/ata/libata-core.c
> @@ -2080,7 +2080,7 @@ static bool ata_dev_power_is_active(struct ata_device *dev)
>   *	LOCKING:
>   *	Kernel thread context (may sleep).
>   */
> -void ata_dev_power_set_active(struct ata_device *dev)
> +unsigned int ata_dev_power_set_active(struct ata_device *dev)
>  {
>  	struct ata_taskfile tf;
>  	unsigned int err_mask;
> @@ -2090,14 +2090,14 @@ void ata_dev_power_set_active(struct ata_device *dev)
>  	 * if supported by the device.
>  	 */
>  	if (!ata_dev_power_init_tf(dev, &tf, true))
> -		return;
> +		return AC_ERR_OTHER;
>  
>  	/*
>  	 * 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;
> +		return AC_ERR_OK;
>  
>  	ata_dev_notice(dev, "Entering active power mode\n");
>  
> @@ -2105,6 +2105,7 @@ void ata_dev_power_set_active(struct ata_device *dev)
>  	if (err_mask)
>  		ata_dev_err(dev, "VERIFY failed (err_mask=0x%x)\n",
>  			    err_mask);
> +	return err_mask;
>  }
>  
>  /**
> @@ -5257,7 +5258,7 @@ static int ata_port_pm_poweroff(struct device *dev)
>  static void ata_port_resume(struct ata_port *ap, pm_message_t mesg,
>  			    bool async)
>  {
> -	ata_port_request_pm(ap, mesg, ATA_EH_RESET,
> +	ata_port_request_pm(ap, mesg, ATA_EH_RESET | ATA_EH_SET_ACTIVE,
>  			    ATA_EHI_NO_AUTOPSY | ATA_EHI_QUIET,
>  			    async);
>  }
> diff --git a/drivers/ata/libata-eh.c b/drivers/ata/libata-eh.c
> index b0d6e69c4a5b..799a1b8bc384 100644
> --- a/drivers/ata/libata-eh.c
> +++ b/drivers/ata/libata-eh.c
> @@ -710,10 +710,6 @@ void ata_scsi_port_error_handler(struct Scsi_Host *host, struct ata_port *ap)
>  			ehc->saved_xfer_mode[devno] = dev->xfer_mode;
>  			if (ata_ncq_enabled(dev))
>  				ehc->saved_ncq_enabled |= 1 << devno;
> -
> -			/* If we are resuming, wake up the device */
> -			if (ap->pflags & ATA_PFLAG_RESUMING)
> -				ehc->i.dev_action[devno] |= ATA_EH_SET_ACTIVE;
>  		}
>  	}
>  
> @@ -3853,7 +3849,9 @@ int ata_eh_recover(struct ata_port *ap, ata_prereset_fn_t prereset,
>  		 */
>  		ata_for_each_dev(dev, link, ENABLED) {
>  			if (ehc->i.dev_action[dev->devno] & ATA_EH_SET_ACTIVE) {
> -				ata_dev_power_set_active(dev);
> +				unsigned int err_mask = ata_dev_power_set_active(dev);
> +				if (err_mask)
> +					link->eh_info.dev_action[dev->devno] |= ATA_EH_SET_ACTIVE;
>  				ata_eh_done(link, dev, ATA_EH_SET_ACTIVE);
>  			}
>  		}
> diff --git a/drivers/ata/libata.h b/drivers/ata/libata.h
> index 5c685bb1939e..43ad1ef9b63a 100644
> --- a/drivers/ata/libata.h
> +++ b/drivers/ata/libata.h
> @@ -65,7 +65,7 @@ extern int ata_dev_configure(struct ata_device *dev);
>  extern bool ata_dev_power_init_tf(struct ata_device *dev,
>  				  struct ata_taskfile *tf, bool set_active);
>  extern void ata_dev_power_set_standby(struct ata_device *dev);
> -extern void ata_dev_power_set_active(struct ata_device *dev);
> +extern unsigned int ata_dev_power_set_active(struct ata_device *dev);
>  extern int sata_down_spd_limit(struct ata_link *link, u32 spd_limit);
>  extern int ata_down_xfermask_limit(struct ata_device *dev, unsigned int sel);
>  extern unsigned int ata_dev_set_feature(struct ata_device *dev,

-- 
Damien Le Moal
Western Digital Research

Re: [PATCH 0/1 v2] Only activate drive once during system resume

[Niklas Cassel]

On Sat, Dec 30, 2023 at 01:21:27PM -0500, Phillip Susi wrote:
> This version also works and may be a bit cleaner

Hello Phillip,

Thank you for your series!


Some small advice:

1) Your patches are missing a Signed-off-by tag.
Without this, we can't accept your changes, see:
https://www.kernel.org/doc/html/latest/process/submitting-patches.html#sign-your-work-the-developer-s-certificate-of-origin


2) The cover letter should explain and summarize the
overall problem that the patch series addresses.

It is also nice with a small change log, so we know
what changed between V1 and V2.


3) The commit messages should explain the specific
problem that the commit fixes in greater detail, see:
https://www.kernel.org/doc/html/latest/process/submitting-patches.html#describe-your-changes

After the problem is established, describe what you
are actually doing about it in technical detail.


""I have been wondering why I kept seeing drives activated
twice during system resume since this got added.""

If possible, please reference a specific SHA1, otherwise we
might not know what "this got added" actually refers to.


4) Please use git format-patch and git send-email.

Looking at
https://lore.kernel.org/linux-ide/
as well as my local inbox,
the threading seems very wrong.

There is a [PATCH 0/1], and then a patch "[PATCH 0/1 v2]"
that replies to the [PATCH 0/1].

Additionally, there is also a [PATCH 1/4] that also replies
to the [PATCH 0/1].

It is just impossible to follow.

For more info, see:
https://www.kernel.org/doc/html/latest/process/submitting-patches.html#explicit-in-reply-to-headers


If you simply use:
$ git format-patch -v 1 -o my-series-v1 v6.7..
$ git send-email my-series-v1/*.patch

for v1 of your series. And then:

$ git format-patch -v 2 -o my-series-v2 v6.7..
$ git send-email my-series-v2/*.patch

for v2 of your series, there will be no explicit
In-Reply-To headers that messes up the threading.


Kind regards,
Niklas

Re: [PATCH 0/1 v2] Only activate drive once during system resume

[Phillip Susi]

Niklas Cassel <cassel@kernel.org> writes:

> 1) Your patches are missing a Signed-off-by tag.
> Without this, we can't accept your changes, see:
> https://www.kernel.org/doc/html/latest/process/submitting-patches.html#sign-your-work-the-developer-s-certificate-of-origin

Yea, they aren't ready to merge yet, so I didn't add the tag.  I suppose
I could have put RFC in the subject.

> 4) Please use git format-patch and git send-email.

I did.

> Looking at
> https://lore.kernel.org/linux-ide/
> as well as my local inbox,
> the threading seems very wrong.
>
> There is a [PATCH 0/1], and then a patch "[PATCH 0/1 v2]"
> that replies to the [PATCH 0/1].

Yes; I sent the second version as a reply to the first.  Isn't that the
usual way of doing it?  So that you can see the whole thread going back
through the older versions?

> Additionally, there is also a [PATCH 1/4] that also replies
> to the [PATCH 0/1].

That's a few replies down from [PATCH 0/1].  There was some discussion
first, then I sent that patch series as a reply to that discussion.

> It is just impossible to follow.

The flow makes perfect sense to me.

> For more info, see:
> https://www.kernel.org/doc/html/latest/process/submitting-patches.html#explicit-in-reply-to-headers

I see.  I guess I'll avoid that in the future then.

Re: [PATCH 0/1] Only activate drive once during system resume

[Damien Le Moal]

On 12/26/23 00:19, Phillip Susi wrote:
> I have been wondering why I kept seeing drives activated twice
> during system resume since this got added.  It seems that the EH
> process can run up to 5 times as long as there is still pending
> EH.  I am not sure why, but it look like my Intel AHCI controller
> issues an error interrupt during the first round of EH, even
> though there does not appear to be any error.  This causes
> the EH_PENDING flag to be set again, and so a second round of
> EH happens, which then tries to activate the disk a second time.

For a single patch, the commit message should be self explanatory. So the above
should be added to the commit message (may be not as-is though).

> 
> I propose the following patch to only set the ACTIVE flag once.
> 
> Phillip Susi (1):
>   libata: only wake a drive once on system resume
> 
>  drivers/ata/libata-eh.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 

-- 
Damien Le Moal
Western Digital Research