Fix libata suspend/resume handling and code cleanup

The first 9 patches of this series fix several issues with suspend/resume power management operations in scsi and libata. The most significant changes introduced are in patch 4 and 5, where the manage_start_stop flag of scsi devices is split into the manage_system_start_stop and manage_runtime_start_stop flags to allow keeping scsi runtime power operations for spining up/down ATA devices but have libata do its own system suspend/resume device power state management using EH.

The remaining patches are code cleanup that do not introduce any significant functional change.

This series was tested on qemu and on various PCs and servers. I am CC-ing people who recently reported issues with suspend/resume. Additional testing would be much appreciated.

Changes from v7:
* Fixed patch 2 commit message incorrect mention of WARN_ON() removal
* Changed patch 4 to use bool type for the new manage_xxx_start_top flags
* Changed patch 12 to keep the early return for when start stop management on resume is not necessary.
* Added review tags

Changes from v6:
* Changed patch 9 to use a bool for the new suspended flag.

Changes from v5:
* Typo and style corrections in patch 4 commit message
* Changed patch 9 to use a new flag to track a disk suspended state instead of using the scsi device state
* Added review tags

Changes from v4:
* Remove ata_scsi_dev_alloc() function in patch 3, coding it directly in ata_scsi_slave_alloc()
* Correct typo in patch 19 commit message
* Added Tested and review tags

Changes from v3:
* Corrected patch 1 (typo in commit message and WARN_ON() removal)
* Changed path 3 as suggested by Niklas (moved definition of ->slave_alloc)
* Rebased on rc2
* Added review tags

Changes from v2:
* Added patch 4 as simply disabling manage_start_stop from libata was breaking individual disk runtime suspend/autosuspend. Patch 5 was reworked accordingly to the changes in patch 4.
* Fixed patch 3: applying the link creation was missing and the link creation itself was also incorrect, preventing sd probe to execute correctly. Thanks to Geert for testing and reporting this issue.
* Split the "Fix delayed scsi_rescan_device() execution" patch into patch 6 (scsi part) and patch 7 (ata part).
* Modified patch 9 to not call sd_shutdown() from sd_remove() for devices that are not running.
* Added Chia-Lin Tested tag to unchanged patches

Changes from v1:
* Added patch 8 and 9 to fix compilation warnings with W=1
* Addressed John comment in patch 19
* Fixed patch 20 commit message (Sergei)
* Added Hannes Review tag

Damien Le Moal (23):
ata: libata-core: Fix ata_port_request_pm() locking
ata: libata-core: Fix port and device removal
ata: libata-scsi: link ata port and scsi device
scsi: sd: Differentiate system and runtime start/stop management
ata: libata-scsi: Disable scsi device manage_system_start_stop
scsi: Do not attempt to rescan suspended devices
ata: libata-scsi: Fix delayed scsi_rescan_device() execution
ata: libata-core: Do not register PM operations for SAS ports
scsi: sd: Do not issue commands to suspended disks on shutdown
ata: libata-core: Fix compilation warning in ata_dev_config_ncq()
ata: libata-eh: Fix compilation warning in ata_eh_link_report()
scsi: Remove scsi device no_start_on_resume flag
ata: libata-scsi: Cleanup ata_scsi_start_stop_xlat()
ata: libata-core: Synchronize ata_port_detach() with hotplug
ata: libata-core: Detach a port devices on shutdown
ata: libata-core: Remove ata_port_suspend_async()
ata: libata-core: Remove ata_port_resume_async()
ata: libata-core: Do not poweroff runtime suspended ports
ata: libata-core: Do not resume runtime suspended ports
ata: libata-sata: Improve ata_sas_slave_configure()
ata: libata-eh: Improve reset error messages
ata: libata-eh: Reduce "disable device" message verbosity
ata: libata: Cleanup inline DMA helper functions

 drivers/ata/libata-core.c      | 242 +++++++++++++++++++++++++--------
 drivers/ata/libata-eh.c        |  76 +++++++++--
 drivers/ata/libata-sata.c      |   5 +-
 drivers/ata/libata-scsi.c      | 142 ++++++++++---------
 drivers/ata/libata-transport.c |   9 +-
 drivers/ata/libata.h           |   6 +
 drivers/firewire/sbp2.c        |   9 +-
 drivers/scsi/scsi_scan.c       |  18 ++-
 drivers/scsi/sd.c              | 102 +++++++++++---
 drivers/scsi/sd.h              |   1 +
 include/linux/libata.h         |  26 ++--
 include/scsi/scsi_device.h     |   4 +-
 include/scsi/scsi_host.h       |   2 +-
 13 files changed, 457 insertions(+), 185 deletions(-)

2.41.0


*** BLURB HERE ***

Damien Le Moal (23):
  ata: libata-core: Fix ata_port_request_pm() locking
  ata: libata-core: Fix port and device removal
  ata: libata-scsi: link ata port and scsi device
  scsi: sd: Differentiate system and runtime start/stop management
  ata: libata-scsi: Disable scsi device manage_system_start_stop
  scsi: Do not attempt to rescan suspended devices
  ata: libata-scsi: Fix delayed scsi_rescan_device() execution
  ata: libata-core: Do not register PM operations for SAS ports
  scsi: sd: Do not issue commands to suspended disks on shutdown
  ata: libata-core: Fix compilation warning in ata_dev_config_ncq()
  ata: libata-eh: Fix compilation warning in ata_eh_link_report()
  scsi: Remove scsi device no_start_on_resume flag
  ata: libata-scsi: Cleanup ata_scsi_start_stop_xlat()
  ata: libata-core: Synchronize ata_port_detach() with hotplug
  ata: libata-core: Detach a port devices on shutdown
  ata: libata-core: Remove ata_port_suspend_async()
  ata: libata-core: Remove ata_port_resume_async()
  ata: libata-core: Do not poweroff runtime suspended ports
  ata: libata-core: Do not resume runtime suspended ports
  ata: libata-sata: Improve ata_sas_slave_configure()
  ata: libata-eh: Improve reset error messages
  ata: libata-eh: Reduce "disable device" message verbosity
  ata: libata: Cleanup inline DMA helper functions

 drivers/ata/libata-core.c      | 242 +++++++++++++++++++++++++--------
 drivers/ata/libata-eh.c        |  76 +++++++++--
 drivers/ata/libata-sata.c      |   5 +-
 drivers/ata/libata-scsi.c      | 142 ++++++++++---------
 drivers/ata/libata-transport.c |   9 +-
 drivers/ata/libata.h           |   6 +
 drivers/firewire/sbp2.c        |   9 +-
 drivers/scsi/scsi_scan.c       |  18 ++-
 drivers/scsi/sd.c              | 108 +++++++++++----
 drivers/scsi/sd.h              |   1 +
 include/linux/libata.h         |  26 ++--
 include/scsi/scsi_device.h     |   6 +-
 include/scsi/scsi_host.h       |   2 +-
 13 files changed, 463 insertions(+), 187 deletions(-)

-- 
2.41.0

The function ata_port_request_pm() checks the port flag ATA_PFLAG_PM_PENDING and calls ata_port_wait_eh() if this flag is set to ensure that power management operations for a port are not scheduled simultaneously. However, this flag check is done without holding the port lock.

Fix this by taking the port lock on entry to the function and checking the flag under this lock. The lock is released and re-taken if ata_port_wait_eh() needs to be called. The two WARN_ON() macros checking that the ATA_PFLAG_PM_PENDING flag was cleared are removed as the first call is racy and the second one done without holding the port lock.

---
 drivers/ata/libata-core.c | 18 +++++++++---------
 1 file changed, 9 insertions(+), 9 deletions(-)

index 0072e0f9ad39..732f3d0b4fd9 100644
@@ -5037,17 +5037,19 @@ static void ata_port_request_pm(struct ata_port *ap, pm_message_t mesg,
 	struct ata_link *link;
 	unsigned long flags;
 
-	/* Previous resume operation might still be in
-	 * progress.  Wait for PM_PENDING to clear.
+	spin_lock_irqsave(ap->lock, flags);
+
+	/*
+	 * A previous PM operation might still be in progress. Wait for
+	 * ATA_PFLAG_PM_PENDING to clear.
 	 */
 	if (ap->pflags & ATA_PFLAG_PM_PENDING) {
+		spin_unlock_irqrestore(ap->lock, flags);
 		ata_port_wait_eh(ap);
-		WARN_ON(ap->pflags & ATA_PFLAG_PM_PENDING);
+		spin_lock_irqsave(ap->lock, flags);
 	}
 
-	/* request PM ops to EH */
-	spin_lock_irqsave(ap->lock, flags);
-
+	/* Request PM operation to EH */
 	ap->pm_mesg = mesg;
 	ap->pflags |= ATA_PFLAG_PM_PENDING;
 	ata_for_each_link(link, ap, HOST_FIRST) {

@@ -5059,10 +5061,8 @@ static void ata_port_request_pm(struct ata_port *ap, pm_message_t mesg,
 
 	spin_unlock_irqrestore(ap->lock, flags);
 
-	if (!async) {
+	if (!async)
 		ata_port_wait_eh(ap);
-		WARN_ON(ap->pflags & ATA_PFLAG_PM_PENDING);
-	}
 }
 
 /*

2.41.0

Whenever an ATA adapter driver is removed (e.g. rmmod), ata_port_detach() is called repeatedly for all the adapter ports to remove (unload) the devices attached to the port and delete the port device itself. Removing of devices is done using libata EH with the ATA_PFLAG_UNLOADING port flag set. This causes libata EH to execute ata_eh_unload() which disables all devices attached to the port.

ata_port_detach() finishes by calling scsi_remove_host() to remove the scsi host associated with the port. This function will trigger the removal of all scsi devices attached to the host and in the case of disks, calls to sd_shutdown() which will flush the device write cache and stop the device. However, given that the devices were already disabled by ata_eh_unload(), the synchronize write cache command and start stop unit commands fail. E.g. running "rmmod ahci" with first removing sd_mod results in error messages like:

ata13.00: disable device
sd 0:0:0:0: [sda] Synchronizing SCSI cache
sd 0:0:0:0: [sda] Synchronize Cache(10) failed: Result: hostbyte=DID_BAD_TARGET driverbyte=DRIVER_OK sd 0:0:0:0: [sda] Stopping disk
sd 0:0:0:0: [sda] Start/Stop Unit failed: Result: hostbyte=DID_BAD_TARGET driverbyte=DRIVER_OK

Fix this by removing all scsi devices of the ata devices connected to the port before scheduling libata EH to disable the ATA devices.

---
 drivers/ata/libata-core.c | 21 ++++++++++++++++++++-
 1 file changed, 20 insertions(+), 1 deletion(-)

index 732f3d0b4fd9..8e35afe5e560 100644
@@ -5948,11 +5948,30 @@ static void ata_port_detach(struct ata_port *ap)
 	struct ata_link *link;
 	struct ata_device *dev;
 
-	/* tell EH we're leaving & flush EH */
+	/* Wait for any ongoing EH */
+	ata_port_wait_eh(ap);
+
+	mutex_lock(&ap->scsi_scan_mutex);
 	spin_lock_irqsave(ap->lock, flags);
+
+	/* Remove scsi devices */
+	ata_for_each_link(link, ap, HOST_FIRST) {
+		ata_for_each_dev(dev, link, ALL) {
+			if (dev->sdev) {
+				spin_unlock_irqrestore(ap->lock, flags);
+				scsi_remove_device(dev->sdev);
+				spin_lock_irqsave(ap->lock, flags);
+				dev->sdev = NULL;
+			}
+		}
+	}
+
+	/* Tell EH to disable all devices */
 	ap->pflags |= ATA_PFLAG_UNLOADING;
 	ata_port_schedule_eh(ap);
+
 	spin_unlock_irqrestore(ap->lock, flags);
+	mutex_unlock(&ap->scsi_scan_mutex);
 
 	/* wait till EH commits suicide */
 	ata_port_wait_eh(ap);

2.41.0

There is no direct device ancestry defined between an ata_device and its scsi device which prevents the power management code from correctly ordering suspend and resume operations. Create such ancestry with the ata device as the parent to ensure that the scsi device (child) is suspended before the ata device and that resume handles the ata device before the scsi device.

The parent-child (supplier-consumer) relationship is established between the ata_port (parent) and the scsi device (child) with the function device_add_link(). The parent used is not the ata_device as the PM operations are defined per port and the status of all devices connected through that port is controlled from the port operations.

The device link is established with the new function ata_scsi_slave_alloc(), and this function is used to define the ->slave_alloc callback of the scsi host template of all ata drivers.

---
 drivers/ata/libata-scsi.c | 45 ++++++++++++++++++++++++++++++++++-----
 include/linux/libata.h    |  2 ++
 2 files changed, 42 insertions(+), 5 deletions(-)

index fb73c145b49a..8b43290ca2cd 100644
@@ -1089,6 +1089,42 @@ int ata_scsi_dev_config(struct scsi_device *sdev, struct ata_device *dev)
 	return 0;
 }
 
+/**
+ *	ata_scsi_slave_alloc - Early setup of SCSI device
+ *	@sdev: SCSI device to examine
+ *
+ *	This is called from scsi_alloc_sdev() when the scsi device
+ *	associated with an ATA device is scanned on a port.
+ *
+ *	LOCKING:
+ *	Defined by SCSI layer.  We don't really care.
+ */
+
+int ata_scsi_slave_alloc(struct scsi_device *sdev)
+{
+	struct ata_port *ap = ata_shost_to_port(sdev->host);
+	struct device_link *link;
+
+	ata_scsi_sdev_config(sdev);
+
+	/*
+	 * Create a link from the ata_port device to the scsi device to ensure
+	 * that PM does suspend/resume in the correct order: the scsi device is
+	 * consumer (child) and the ata port the supplier (parent).
+	 */
+	link = device_link_add(&sdev->sdev_gendev, &ap->tdev,
+			       DL_FLAG_STATELESS |
+			       DL_FLAG_PM_RUNTIME | DL_FLAG_RPM_ACTIVE);
+	if (!link) {
+		ata_port_err(ap, "Failed to create link to scsi device %s\n",
+			     dev_name(&sdev->sdev_gendev));
+		return -ENODEV;
+	}
+
+	return 0;
+}
+EXPORT_SYMBOL_GPL(ata_scsi_slave_alloc);
+
 /**
  *	ata_scsi_slave_config - Set SCSI device attributes
  *	@sdev: SCSI device to examine

@@ -1105,14 +1141,11 @@ int ata_scsi_slave_config(struct scsi_device *sdev)
 {
 	struct ata_port *ap = ata_shost_to_port(sdev->host);
 	struct ata_device *dev = __ata_scsi_find_dev(ap, sdev);
-	int rc = 0;
-
-	ata_scsi_sdev_config(sdev);
 
 	if (dev)
-		rc = ata_scsi_dev_config(sdev, dev);
+		return ata_scsi_dev_config(sdev, dev);
 
-	return rc;
+	return 0;
 }
 EXPORT_SYMBOL_GPL(ata_scsi_slave_config);
 

@@ -1136,6 +1169,8 @@ void ata_scsi_slave_destroy(struct scsi_device *sdev)
 	unsigned long flags;
 	struct ata_device *dev;
 
+	device_link_remove(&sdev->sdev_gendev, &ap->tdev);
+
 	spin_lock_irqsave(ap->lock, flags);
 	dev = __ata_scsi_find_dev(ap, sdev);
 	if (dev && dev->sdev) {

index bf4913f4d7ac..4ece1b7a2a5b 100644
@@ -1148,6 +1148,7 @@ extern int ata_std_bios_param(struct scsi_device *sdev,
 			      struct block_device *bdev,
 			      sector_t capacity, int geom[]);
 extern void ata_scsi_unlock_native_capacity(struct scsi_device *sdev);
+extern int ata_scsi_slave_alloc(struct scsi_device *sdev);
 extern int ata_scsi_slave_config(struct scsi_device *sdev);
 extern void ata_scsi_slave_destroy(struct scsi_device *sdev);
 extern int ata_scsi_change_queue_depth(struct scsi_device *sdev,

@@ -1396,6 +1397,7 @@ extern const struct attribute_group *ata_common_sdev_groups[];
 	.this_id		= ATA_SHT_THIS_ID,		\
 	.emulated		= ATA_SHT_EMULATED,		\
 	.proc_name		= drv_name,			\
+	.slave_alloc		= ata_scsi_slave_alloc,		\
 	.slave_destroy		= ata_scsi_slave_destroy,	\
 	.bios_param		= ata_std_bios_param,		\
 	.unlock_native_capacity	= ata_scsi_unlock_native_capacity,\

2.41.0

The underlying device and driver of a SCSI disk may have different system and runtime power mode control requirements. This is because runtime power management affects only the SCSI disk, while sustem level power management affects all devices, including the controller for the SCSI disk.

For instance, issuing a START STOP UNIT command when a SCSI disk is runtime suspended and resumed is fine: the command is translated to a STANDBY IMMEDIATE command to spin down the ATA disk and to a VERIFY command to wake it up. The SCSI disk runtime operations have no effect on the ata port device used to connect the ATA disk. However, for system suspend/resume operations, the ATA port used to connect the device will also be suspended and resumed, with the resume operation requiring re-validating the device link and the device itself. In this case, issuing a VERIFY command to spinup the disk must be done before starting to revalidate the device, when the ata port is being resumed. In such case, we must not allow the SCSI disk driver to issue START STOP UNIT commands.

Allow a low level driver to refine the SCSI disk start/stop management by differentiating system and runtime cases with two new SCSI device
flags: manage_system_start_stop and manage_runtime_start_stop. These new flags replace the current manage_start_stop flag. Drivers setting the manage_start_stop are modifed to set both new flags, thus preserving the existing start/stop management behavior. For backward compatibility, the old manage_start_stop sysfs device attribute is kept as a read-only attribute showing a value of 1 for devices enabling both new flags and 0 otherwise.

---
 drivers/ata/libata-scsi.c  |  3 +-
 drivers/firewire/sbp2.c    |  9 ++--
 drivers/scsi/sd.c          | 90 ++++++++++++++++++++++++++++++--------
 include/scsi/scsi_device.h |  5 ++-
 4 files changed, 84 insertions(+), 23 deletions(-)

index 8b43290ca2cd..73428ad0c8d2 100644
@@ -1056,7 +1056,8 @@ int ata_scsi_dev_config(struct scsi_device *sdev, struct ata_device *dev)
 		 * will be woken up by ata_port_pm_resume() with a port reset
 		 * and device revalidation.
 		 */
-		sdev->manage_start_stop = 1;
+		sdev->manage_system_start_stop = true;
+		sdev->manage_runtime_start_stop = true;
 		sdev->no_start_on_resume = 1;
 	}
 

index 26db5b8dfc1e..749868b9e80d 100644
@@ -81,7 +81,8 @@ MODULE_PARM_DESC(exclusive_login, "Exclusive login to sbp2 device "
  *
  * - power condition
  *   Set the power condition field in the START STOP UNIT commands sent by
- *   sd_mod on suspend, resume, and shutdown (if manage_start_stop is on).
+ *   sd_mod on suspend, resume, and shutdown (if manage_system_start_stop or
+ *   manage_runtime_start_stop is on).
  *   Some disks need this to spin down or to resume properly.
  *
  * - override internal blacklist

@@ -1517,8 +1518,10 @@ static int sbp2_scsi_slave_configure(struct scsi_device *sdev)
 
 	sdev->use_10_for_rw = 1;
 
-	if (sbp2_param_exclusive_login)
-		sdev->manage_start_stop = 1;
+	if (sbp2_param_exclusive_login) {
+		sdev->manage_system_start_stop = true;
+		sdev->manage_runtime_start_stop = true;
+	}
 
 	if (sdev->type == TYPE_ROM)
 		sdev->use_10_for_ms = 1;

index b9230b6add04..fd41fdac0a8e 100644
@@ -161,6 +161,10 @@ struct scsi_device {
 				 * pass settings from slave_alloc to scsi
 				 * core. */
 	unsigned int eh_timeout; /* Error handling timeout */
+
+	bool manage_system_start_stop; /* Let HLD (sd) manage system start/stop */
+	bool manage_runtime_start_stop; /* Let HLD (sd) manage runtime start/stop */
+
 	unsigned removable:1;
 	unsigned changed:1;	/* Data invalid due to media change */
 	unsigned busy:1;	/* Used to prevent races */

@@ -193,7 +197,6 @@ struct scsi_device {
 	unsigned use_192_bytes_for_3f:1; /* ask for 192 bytes from page 0x3f */
 	unsigned no_start_on_add:1;	/* do not issue start on add */
 	unsigned allow_restart:1; /* issue START_UNIT in error handler */
-	unsigned manage_start_stop:1;	/* Let HLD (sd) manage start/stop */
 	unsigned no_start_on_resume:1; /* Do not issue START_STOP_UNIT on resume */
 	unsigned start_stop_pwr_cond:1;	/* Set power cond. in START_STOP_UNIT */
 	unsigned no_uld_attach:1; /* disable connecting to upper level drivers */

2.41.0

Damien,

/s/sustem/system/

Martin K. Petersen	Oracle Linux Engineering

Why must a VERIFY be issued to spinup the disk before revalidating? Before these patches, by default, manage_start_stop was on, and so sd would cause a VERIFY in the system resume path. That resume however ( sd and its issuing START UNIT ), would have happened AFTER the link was resumed and the ATA device was revalidated, woudldn't it? So at that point, the drive would already be spinning. And if manage_start_stop was disabled, then there would be no VERIFY at all, and this did not seem to cause a problem before.

If this VERIFY were skipped, the next thing that would happen is for ata_dev_revalidate() to issue IDENTIFY, which would wait for the drive to spin up before returning wouldn't it? ( unless the drive has PuiS enabled ).

We can most likely move that VERIFY to after revalidation. That should shorten delays on first access after resume as many drive do actually spinup with the reset done before revalidating (but note that the specs say that even a reset shall not take a drive out of standby mode, without specifying the reset type, so this is rather loosely defined).

In theory, yes, that was the intent. In practice, the verify was issued from scsi PM resume context while the actual drive port reset + revalidation is done in libata EH context, triggered from ATA port resume context which itself was not synchronized/ordered with the scsi disk resume. So we ended up with the verify command execution sometimes being attempted with the drive not even revalidated yet, or with the port/link not even active sometimes (depending on timing). So problems all over and deadlocks due to scsi revalidate using the device lock, which PM use too.

See above. With the switch to async PM ops in scsi in kernel 5.16, things broke badly due to the lack of synchronization that sync PM provided before that.

ACS defines that only media access commands can get a drive out of standby mode back into active mode. So an IDENTIFY command would not (normally) spinup a drive. Nor would READ LOG. Normally, IDENTIFY, READ LOG etc done in revalidate can be processed with the drive spun down (*but* I have seen drives that react badly to some of these commands when spun down). Hence why I put it first, before revalidate. Now that all the synchronization is in place and libata EH does its own manage_start_stop for system suspend/resume, I will see if moving the verify to the end of revalidate works.

Damien Le Moal
Western Digital Research

For SCSI disks that are runtime suspended, it looks like they skip waking the disk on system resume, leaving them in runtime suspend. After these patches, it looks like libata always wakes up the disk, but I don't see any calls to pm_runtime_disable/set_active/enable to mark the scsi disk as active after the system resume. That should result in a disk that is spinning, but runtime pm thinks is not, and so will not put it into suspend after the inactivity timeout.

Yes, correct, but this does not create any issues in practice beside the undesired disk spinup.

Fixing that is not trivial because using runtime suspend/resume on the SCSI disk is just that, it will affect *only* the SCSI disk and not the ATA device and its port. In other words, a runtime suspend of the SCSI disk will spin down the drive but it will not runtime suspend the ATA port. So if you suspend the system, on resume, the ATA port will not be runtime suspended and so it will be resumed. The SCSI disk will not be resumed, but the ATA port resume will have spun up the disk, which we do not really want in that case.

I am looking into this. Again, that is not a trivial fix. The other thing to notice here is that ATA port runtime suspend/resume is in fact broken: it does not track accesses to the device(s) connected to the port. And given that more than one device may be connected to a port, we need PM runtime reference counting to be done for this to work correctly. That is missing. Solutions are:
fix everything or simply do not support ATA port runtime suspend/resume (i.e. remove code doing it). I am leaning toward the latter as it seems that no one actually noticed these issues because no one is actually using ATA port runtime suspend/resume...

Damien Le Moal
Western Digital Research

The issue it creates is the opposite of that: it breaks the desired spin down. After some period of inactivity, the disk should be suspended, but after a system resume, the kernel thinks that it already is, and so won't suspend it again.

I tested this last week and it appeared to work. I enabled runtime pm on the disk, as well as the ata port, and as soon as the disk suspended, the port did as well.

Right, I would rather the disk stay asleep if it has PuiS enabled, and I'm working on a patch for that. In the process of doing that though, I noticed that despite waking the disk up, it does not inform runtime pm about that.

Again, it seems to me that the child reference counting IS working.

Probably nobody is using it yes, but that doesn't mean we shouldn't try to get it working. It would be nice to have the drive go into deep SLEEP instead of standby, as well as suspend the ata port, and possibly even the whole AHCI controller rather than relying on the old APM drive internal auto standby mode.

That one should be fixable, though it I do not see an elegant method to do it. It would be easy with ugly code, e.g. tweaking the scsi device runtime pm state from libata... Not great.

Never saw that in my tests when enabling runtime pm on the scsi disk only. Which is the important point here: there is no propagation of the suspend state down to the device parent it seems.

But there are no runtime PM operations defined for ATA devices, only for ports. So not sure that matters... I am probably still missing something about runtime PM and devices ancestry. I focused a lot on system suspend/resume to fix the issues. runtime suspend/resume is next.

I am not sure of that, especially with cases of ATA ports with multiple disks (e.g. pmp or IDE).

Damien Le Moal
Western Digital Research

What would be not great about it? libata already takes over the system suspend/resume from sd. I'm currently testing having libata do just this right now. I just got ahold of some jumpers today to put the drives back into PuiS and do some further testing tonight.

Last night I again saw the port auto suspend when the scsi disk was runtime suspended. Tonight I'll test with PuiS, as well as with system resume while runtime suspended. Maybe I'll even try to get the whole AHCI controller to auto suspend. It seems like it should once all of the ports do.

Good point. I have an eSATA dock with PMP. I'll check tonight if the children are counted properly.

To have to add code in libata that touches the scsi device PM status is what's not going to be great. Such code should stay in sd.c and scsi_pm.c. But not sure we actually need anything.

With my tests, I never set the ata port power/control to "auto", which may be why I did not see the port being runtime suspended when the scsi disk was runtime suspended. Will try again.

With the device links in place between port and scsi devices, we should be OK. But still need to check that we do not need runtime_get/put calls added. Ideally, we should have the chain:

scsi disk -> scsi target -> scsi host -> ata port

for runtime suspend, and the reverse for runtime resume. If there is a system suspend/resume between runtime suspend/resume, the port should not be resumed if it is runtime suspended.

Damien Le Moal
Western Digital Research

It looks to me like there is an additional generic block device that sits on top and that is what actually has the idle timeout. Or maybe that's the scsi disk, since it's name incldues the SCSI LUN, but in the structure, its called sdev_gendev. But then there's also sdev_dev, and sdev_target.

I'm not sure about it. The port has to be resumed so that we can attempt to revalidate the devices on it. For disks that have spun up on their own, we should not leave then marked as runtime suspended, but really they are spinning. I suppose we could put them to sleep, though I was leaning to just marking them as active, and leaving the runtime pm timer to put them to sleep later, which then could allow the port to suspend again.

On my system, I see:

cat /sys/class/ata_port/ata1/power/runtime_active_kids 0

and same for port 10 which is a PMP box with 3 drives. So it means that the children will be ignored, which is wrong. Note that the corresponding scsi_host device also shows 0. So to be safe with port runtime PM, we need to fix that first. Otherwise, the port may end up being runtime suspended with running drives still attached to it.

/sys/class/ata_port/ata1/power/control is set to "auto" by default.

Damien Le Moal
Western Digital Research

I see a 1 there, which is the single scsi_host. The scsi_host has 2 active kids; the two disks. When I enabled runtime pm, only when the second disk was suspended did that allow the scsi_host to suspend, which then allowed the port to suspend. Everything looked fine there so far. Then I tried:

echo 1 > /sys/block/sdf/device/delete

And the SCSI EH appears to have tried to wake up the disk, and hung in the process.

[  314.246282] sd 7:0:0:0: [sde] Synchronizing SCSI cache
[  314.246445] sd 7:0:0:0: [sde] Stopping disk

First disk suspends.

[  388.518295] sd 7:1:0:0: [sdf] Synchronizing SCSI cache
[  388.518519] sd 7:1:0:0: [sdf] Stopping disk

Second disk suspends some time later.

[  388.930428] ata8.00: Entering standby power mode
[  389.330651] ata8.01: Entering standby power mode

That allowed the port to suspend. This is when I tried to detach the disk driver, which I think tried to resume the disk before detaching, which resumed the port.

[  467.511878] ata8.15: SATA link down (SStatus 0 SControl 310)
[  468.142726] ata8.15: failed to read PMP GSCR[0] (Emask=0x100)
[  468.142741] ata8.15: PMP revalidation failed (errno=-5)

I ran hdparm -C on the other disk at this point. I just noticed that the ata8.15 that represents the PMP itself was NOT suspended along with the two drive links, and then maybe was not resumed before trying to revalidate the PMP? And that's why it failed?

[  473.172792] ata8.15: SATA link up 1.5 Gbps (SStatus 113 SControl 310)
[  473.486860] ata8.00: SATA link up 1.5 Gbps (SStatus 113 SControl 310)
[  473.802139] ata8.01: SATA link up 1.5 Gbps (SStatus 113 SControl 310)

It seems like it ended up recovering here though? And yet the scsi_eh remained hung, as did the hdparm -C:

[  605.566814] INFO: task scsi_eh_7:173 blocked for more than 120 seconds.
[  605.566829]       Not tainted 6.6.0-rc5+ #5
[  605.566834] "echo 0 > /proc/sys/kernel/hung_task_timeout_secs" disables this message.
[  605.566838] task:scsi_eh_7       state:D stack:0     pid:173   ppid:2      flags:0x00004000
[  605.566850] Call Trace:
[  605.566853]  <TASK>
[  605.566860]  __schedule+0x37c/0xb70
[  605.566878]  schedule+0x61/0xd0
[  605.566888]  rpm_resume+0x156/0x760
[  605.566896]  ? sched_energy_aware_handler+0xb0/0xb0
[  605.566907]  rpm_resume+0x255/0x760
[  605.566915]  rpm_resume+0x255/0x760
[  605.566923]  rpm_resume+0x255/0x760
[  605.566931]  __pm_runtime_resume+0x4e/0x80
[  605.566941]  ata_eh_recover+0x695/0x1060 [libata]
[  605.567001]  ? ata_port_pm_suspend+0x50/0x50 [libata]
[  605.567048]  ? ahci_do_softreset+0x2d0/0x2d0 [libahci]
[  605.567067]  ? ata_host_release+0x80/0x80 [libata]
[  605.567108]  ? ata_port_runtime_idle+0x110/0x110 [libata]
[  605.567151]  ? sata_pmp_configure+0x72/0x210 [libata]
[  605.567204]  sata_pmp_error_handler+0x357/0xac0 [libata]
[  605.567249]  ? ata_port_pm_suspend+0x50/0x50 [libata]
[  605.567291]  ? ahci_stop_engine+0xe0/0xe0 [libahci]
[  605.567309]  ? ahci_do_hardreset+0x140/0x140 [libahci]
[  605.567325]  ? ahci_do_softreset+0x2d0/0x2d0 [libahci]
[  605.567344]  ? _raw_spin_unlock_irqrestore+0x27/0x40
[  605.567355]  ahci_error_handler+0x36/0x60 [libahci]
[  605.567373]  ata_scsi_port_error_handler+0x3de/0x8a0 [libata]
[  605.567424]  ? scsi_eh_get_sense+0x250/0x250 [scsi_mod]
[  605.567464]  ata_scsi_error+0x95/0xc0 [libata]
[  605.567511]  scsi_error_handler+0xb9/0x580 [scsi_mod]
[  605.567547]  ? preempt_count_add+0x6c/0xa0
[  605.567556]  ? scsi_eh_get_sense+0x250/0x250 [scsi_mod]
[  605.567587]  kthread+0xf2/0x120
[  605.567594]  ? kthread_complete_and_exit+0x20/0x20
[  605.567602]  ret_from_fork+0x31/0x50
[  605.567611]  ? kthread_complete_and_exit+0x20/0x20
[  605.567617]  ret_from_fork_asm+0x11/0x20
[  605.567630]  </TASK>
[  605.567663] INFO: task bash:1305 blocked for more than 120 seconds.
[  605.567670]       Not tainted 6.6.0-rc5+ #5
[  605.567675] "echo 0 > /proc/sys/kernel/hung_task_timeout_secs" disables this message.
[  605.567678] task:bash            state:D stack:0     pid:1305  ppid:1300   flags:0x00004004
[  605.567687] Call Trace:
[  605.567689]  <TASK>
[  605.567693]  __schedule+0x37c/0xb70
[  605.567703]  ? try_to_wake_up+0xb2/0x5e0
[  605.567715]  schedule+0x61/0xd0
[  605.567725]  ata_port_wait_eh+0x7c/0xf0 [libata]
[  605.567776]  ? sched_energy_aware_handler+0xb0/0xb0
[  605.567784]  ? ata_sas_port_resume+0x30/0x30 [libata]
[  605.567829]  ata_port_runtime_resume+0x27/0x30 [libata]
[  605.567870]  __rpm_callback+0x41/0x110
[  605.567879]  ? ata_sas_port_resume+0x30/0x30 [libata]
[  605.567917]  rpm_callback+0x35/0x70
[  605.567925]  rpm_resume+0x513/0x760
[  605.567931]  ? _raw_read_lock_irqsave+0x28/0x50
[  605.567938]  ? _raw_read_unlock_irqrestore+0x2a/0x40
[  605.567944]  ? ep_poll_callback+0x269/0x2d0
[  605.567955]  rpm_resume+0x255/0x760
[  605.567962]  ? __slab_free+0xc7/0x320
[  605.567972]  rpm_resume+0x255/0x760
[  605.567978]  ? kernfs_should_drain_open_files+0x38/0x50
[  605.567989]  rpm_resume+0x255/0x760
[  605.567994]  ? kernfs_should_drain_open_files+0x38/0x50
[  605.568002]  ? kernfs_drain+0xec/0x120
[  605.568010]  __pm_runtime_resume+0x4e/0x80
[  605.568018]  device_release_driver_internal+0xa8/0x200
[  605.568028]  bus_remove_device+0xc0/0x120
[  605.568035]  device_del+0x158/0x3d0
[  605.568045]  ? mutex_lock+0x12/0x30
[  605.568051]  __scsi_remove_device+0x12b/0x180 [scsi_mod]
[  605.568095]  sdev_store_delete+0x6a/0xd0 [scsi_mod]
[  605.568132]  kernfs_fop_write_iter+0x129/0x1c0
[  605.568141]  vfs_write+0x2d3/0x3f0
[  605.568155]  ksys_write+0x63/0xe0
[  605.568165]  do_syscall_64+0x5a/0xb0
[  605.568173]  ? syscall_exit_to_user_mode+0x2b/0x40
[  605.568178]  ? do_syscall_64+0x67/0xb0
[  605.568184]  entry_SYSCALL_64_after_hwframe+0x46/0xb0
[  605.568193] RIP: 0033:0x7f0c1e9b6473
[  605.568200] RSP: 002b:00007ffe01b0bd28 EFLAGS: 00000246 ORIG_RAX: 0000000000000001
[  605.568208] RAX: ffffffffffffffda RBX: 0000000000000002 RCX: 00007f0c1e9b6473
[  605.568213] RDX: 0000000000000002 RSI: 000056115e4b86f0 RDI: 0000000000000001
[  605.568216] RBP: 000056115e4b86f0 R08: 000000000000000a R09: 00007f0c1ea5a0c0
[  605.568220] R10: 00007f0c1ea59fc0 R11: 0000000000000246 R12: 0000000000000002
[  605.568224] R13: 00007f0c1ea9a6a0 R14: 0000000000000002 R15: 00007f0c1ea95880
[  605.568232]  </TASK>

Looks like a deadlock somewhere, likely with the device remove that you triggered with the "echo 1 > /sys/block/sdf/device/delete".

Can you send the exact list of commands & events you executed to get to that point ? Also please share your kernel config.

Damien Le Moal
Western Digital Research

I wrote auto to /sys/block/sd[ef]/device/power/config and 10000 to /sys/block/sd[ef]/device/power/auto_suspend_delay_ms, and auto to the control file of their corresponding ata8 port ( both drives are behind an PMP in an eSATA dock on ata8 ). As I said before, it the dmesg showed that the ata port only suspended after BOTH drives had suspended, which is as it should be. The problem seems to be after I echo 1 > /sys/block/sdf/device/delete, when this happens:

Oct 26 16:43:00 faldara kernel: ata8.15: failed to read PMP GSCR[0] (Emask=0x100)
Oct 26 16:43:00 faldara kernel: ata8.15: PMP revalidation failed (errno=-5)
Oct 26 16:43:05 faldara kernel: ata8.15: SATA link up 3.0 Gbps (SStatus 123 SControl 300)
Oct 26 16:43:05 faldara kernel: ata8.00: SATA link up 3.0 Gbps (SStatus 123 SControl 320)
Oct 26 16:43:05 faldara kernel: ata8.01: SATA link up 3.0 Gbps (SStatus

123 SControl 320)

Then I get the hung task. I reverted the PuiS patch that I have been working on, and the hang no longer happens, however, the above errors do still happen. I think that is a problem in itself, and may or may not be related to the hang. I see no reason why the PMP revalidation should fail after the two disks and the port are runtime pm suspended, but for some reason, with my patch applied, that then leads to the hang.

Here is my git log showing your two patches applied on the upstream kernel, plus my patch:

commit bb5db8bb505171fd2b8b67c3f22b16d8355d2a8b

Author: Phillip Susi <[email protected]>
Date:   Mon Oct 16 17:03:51 2023 -0400

Olibata: don't start disks on resume

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.

commit 4f1a1a9da4ff833417fa520d097b3f07c20e3c5d

Author: Damien Le Moal <[email protected]>
Date:   Thu Oct 12 16:18:00 2023 +0900

[PATCH 2/2] ata: libata-core: Improve ata_dev_power_set_active()

Improve the function ata_dev_power_set_active() by having it do nothing for a disk that is already in the active power state. To do that, introduce the function ata_dev_power_is_active() to test the current power state of the disk and return true if the disk is in the PM0:
active or PM1: idle state (0xff value for the count field of the CHECK POWER MODE command output).

To preserve the existing behavior, if the CHECK POWER MODE command issued in ata_dev_power_is_active() fails, the drive is assumed to be in standby mode and false is returned.

With this change, issuing the VERIFY command to access the disk media to spin it up becomes unnecessary most of the time during system resume as the port reset done by libata-eh on resume often result in the drive to spin-up (this behavior is not clearly defined by the ACS specifications and may thus vary between disk models).

commit bb7e1fcd9e3a207820e4b828e9f4f02986942d8d

Author: Damien Le Moal <[email protected]>
Date:   Thu Oct 12 16:17:59 2023 +0900

ata: libata-eh: Spinup disk on resume after revalidation

Move the call to ata_dev_power_set_active() to transition a disk in standby power mode to the active power mode from ata_eh_revalidate_and_attach() before doing revalidation to the end of ata_eh_recover(), after the link speed for the device is reconfigured (if that was necessary). This is safer as this ensure that the VERIFY command executed to spinup the disk is executed with the drive properly reconfigured first.

commit 727fb83765049981e342db4c5a8b51aca72201d8
Merge: 8cb1f10d8c4b 5c15c60e7be6
Author: Linus Torvalds <[email protected]>
Date: Fri Oct 13 23:19:16 2023 -0700

Merge tag 'input-for-v6.6-rc5' of git://git.kernel.org/pub/scm/linux/kernel/git/dtor/input

Pull input fixes from Dmitry Torokhov:

- a reworked way for handling reset delay on SMBus-connected Synaptics touchpads (the original one, while being correct, uncovered an old bug in fallback to PS/2 code that was fixed separately; the new one however avoids having delay in serio port "fast" resume, and instead has the wait in the RMI4 code)

- a fix for potential crashes when devices with Elan controllers (and Synaptics) fall back to PS/2 code. Can't be hit without the original patch above, but still good to have it fixed

- a couple new device IDs in xpad Xbox driver

- another quirk for Goodix driver to deal with stuff vendors put in ACPI tables

- a fix for use-after-free on disconnect for powermate driver

- a quirk to not initialize PS/2 mouse port on Fujitsu Lifebook E5411 laptop as it makes keyboard not usable and the device uses hid-over-i2c touchpad anyways

* tag 'input-for-v6.6-rc5' of git://git.kernel.org/pub/scm/linux/kernel/git/dtor/input:
Input: powermate - fix use-after-free in powermate_config_complete
Input: xpad - add PXN V900 support
Input: synaptics-rmi4 - handle reset delay when using SMBus trsnsport
Input: psmouse - fix fast_reconnect function for PS/2 mode
Revert "Input: psmouse - add delay when deactivating for SMBus mode"
Input: goodix - ensure int GPIO is in input for gpio_count == 1 && gpio_int_idx == 0 case
Input: i8042 - add Fujitsu Lifebook E5411 to i8042 quirk table
Input: xpad - add HyperX Clutch Gladiate Support

And here is my patch, which basically checks for PuiS during system resume, and either forces the disk to suspend or resume depending on whether it is PuiS. Since I have not even engaged in any system suspend/resume for this test, this patch should not have any effect.

So far in my testing of this patch on my 3 internal drives that I have enabled PuiS on, it appears to work in so much as after a suspend/resume, the runtime status of the disks is suspended ( as long as I have *enabled* runtime pm on them, otherwise not ). Another problem seems to be that while the disks are left suspended after system resume, they very quickly are resumed due to begnign IO eminating from either ext4 or udsisks2 that does not cause a drive to spin up when it is suspended with hdparm -y. This would be the case of either FLUSH CASH or CHECK POWER CONDITION not causing the drive to spin itself up when given the commands, but the runtime pm core does not know that these commands can be completed without resuming the disk, so it resumes them first.

From 77a3511d4058e3afccc4ba745c8c6ad6f7ac07a3 Mon Sep 17 00:00:00 2001
From: Phillip Susi <[email protected]>
Date: Mon, 16 Dec 2013 18:30:55 -0500
Subject: [PATCH] libata: don't start disks on resume

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 | 25 +++++++++++++++++++++----
 drivers/ata/libata-eh.c   | 19 ++++++++++++++++++-
 drivers/ata/libata.h      |  1 +
 include/linux/libata.h    |  1 +
 4 files changed, 41 insertions(+), 5 deletions(-)

index 7c261907a1d0..cd4718fe02e1 100644
@@ -1912,7 +1912,20 @@ 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
+		 */
+		dev->flags |= ATA_DFLAG_SLEEPING;
+		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

@@ -3956,6 +3969,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;
 

@@ -5172,6 +5187,10 @@ static void ata_port_request_pm(struct ata_port *ap, pm_message_t mesg,
 		spin_lock_irqsave(ap->lock, flags);
 	}
 
+	/* 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;

@@ -5269,9 +5288,7 @@ static void ata_port_resume_async(struct ata_port *ap, pm_message_t mesg)
 static int ata_port_pm_resume(struct device *dev)
 {
 	ata_port_resume_async(to_ata_port(dev), PMSG_RESUME);
-	pm_runtime_disable(dev);
-	pm_runtime_set_active(dev);
-	pm_runtime_enable(dev);
+	printk(KERN_INFO "resume device: %p", dev);
 	return 0;
 }
 

index 09be31723a3c..e77805ba46b0 100644
@@ -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>
 

@@ -3042,6 +3043,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);

@@ -3071,9 +3074,23 @@ 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 */
+				printk(KERN_INFO "sdev: %p", &dev->sdev->sdev_dev);
+				pm_runtime_disable(&dev->sdev->sdev_dev);
+				pm_runtime_set_suspended(&dev->sdev->sdev_dev);
+				pm_runtime_enable(&dev->sdev->sdev_dev);
+
+				continue;
+			}
+			else if (rc)
 				goto err;
 
+			printk(KERN_INFO "sdev: %p", &dev->sdev->sdev_dev);
+			pm_runtime_disable(&dev->sdev->sdev_dev);
+			pm_runtime_set_active(&dev->sdev->sdev_dev);
+			pm_runtime_enable(&dev->sdev->sdev_dev);
+
 			ata_eh_done(link, dev, ATA_EH_REVALIDATE);
 
 			/* Configuration may have changed, reconfigure

index 05ac80da8ebc..cc13777d2811 100644
@@ -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 */

index 2a7d2af0ed80..77769351ab99 100644
@@ -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.41.0

The introduction of a device link to create a consumer/supplier relationship between the scsi device of an ATA device and the ATA port of that ATA device fixes the ordering of system suspend and resume operations. For suspend, the scsi device is suspended first and the ata port after it. This is fine as this allows the synchronize cache and START STOP UNIT commands issued by the scsi disk driver to be executed before the ata port is disabled.

For resume operations, the ata port is resumed first, followed by the scsi device. This allows having the request queue of the scsi device to be unfrozen after the ata port resume is scheduled in EH, thus avoiding to see new requests prematurely issued to the ATA device. Since libata sets manage_system_start_stop to 1, the scsi disk resume operation also results in issuing a START STOP UNIT command to the device being resumed so that the device exits standby power mode.

However, restoring the ATA device to the active power mode must be synchronized with libata EH processing of the port resume operation to avoid either 1) seeing the start stop unit command being received too early when the port is not yet resumed and ready to accept commands, or after the port resume process issues commands such as IDENTIFY to revalidate the device. In this last case, the risk is that the device revalidation fails with timeout errors as the drive is still spun down.

Commit 0a8589055936 ("ata,scsi: do not issue START STOP UNIT on resume") disabled issuing the START STOP UNIT command to avoid issues with it. But this is incorrect as transitioning a device to the active power mode from the standby power mode set on suspend requires a media access command. The IDENTIFY, READ LOG and SET FEATURES commands executed in libata EH context triggered by the ata port resume operation may thus fail.

Fix these synchronization issues is by handling a device power mode transitions for system suspend and resume directly in libata EH context, without relying on the scsi disk driver management triggered with the manage_system_start_stop flag.

To do this, the following libata helper functions are introduced:

1) ata_dev_power_set_standby():

This function issues a STANDBY IMMEDIATE command to transitiom a device to the standby power mode. For HDDs, this spins down the disks. This function applies only to ATA and ZAC devices and does nothing otherwise. This function also does nothing for devices that have the ATA_FLAG_NO_POWEROFF_SPINDOWN or ATA_FLAG_NO_HIBERNATE_SPINDOWN flag set.

For suspend, call ata_dev_power_set_standby() in ata_eh_handle_port_suspend() before the port is disabled and frozen. ata_eh_unload() is also modified to transition all enabled devices to the standby power mode when the system is shutdown or devices removed.

2) ata_dev_power_set_active() and

This function applies to ATA or ZAC devices and issues a VERIFY command for 1 sector at LBA 0 to transition the device to the active power mode. For HDDs, since this function will complete only once the disk spin up. Its execution uses the same timeouts as for reset, to give the drive enough time to complete spinup without triggering a command timeout.

For resume, call ata_dev_power_set_active() in ata_eh_revalidate_and_attach() after the port has been enabled and before any other command is issued to the device.

With these changes, the manage_system_start_stop and no_start_on_resume scsi device flags do not need to be set in ata_scsi_dev_config(). The flag manage_runtime_start_stop is still set to allow the sd driver to spinup/spindown a disk through the sd runtime operations.

---
 drivers/ata/libata-core.c | 90 +++++++++++++++++++++++++++++++++++++++
 drivers/ata/libata-eh.c   | 46 +++++++++++++++++++-
 drivers/ata/libata-scsi.c | 16 +++----
 drivers/ata/libata.h      |  2 +
 include/linux/libata.h    |  6 ++-
 5 files changed, 148 insertions(+), 12 deletions(-)

index 8e35afe5e560..a0bc01606b30 100644
@@ -1972,6 +1972,96 @@ int ata_dev_read_id(struct ata_device *dev, unsigned int *p_class,
 	return rc;
 }
 
+/**
+ *	ata_dev_power_set_standby - Set a device power mode to standby
+ *	@dev: target device
+ *
+ *	Issue a STANDBY IMMEDIATE command to set a device power mode to standby.
+ *	For an HDD device, this spins down the disks.
+ *
+ *	LOCKING:
+ *	Kernel thread context (may sleep).
+ */
+void ata_dev_power_set_standby(struct ata_device *dev)
+{
+	unsigned long ap_flags = dev->link->ap->flags;
+	struct ata_taskfile tf;
+	unsigned int err_mask;
+
+	/* Issue STANDBY IMMEDIATE command only if supported by the device */
+	if (dev->class != ATA_DEV_ATA && dev->class != ATA_DEV_ZAC)
+		return;
+
+	/*
+	 * Some odd clown BIOSes issue spindown on power off (ACPI S4 or S5)
+	 * causing some drives to spin up and down again. For these, do nothing
+	 * if we are being called on shutdown.
+	 */
+	if ((ap_flags & ATA_FLAG_NO_POWEROFF_SPINDOWN) &&
+	    system_state == SYSTEM_POWER_OFF)
+		return;
+
+	if ((ap_flags & ATA_FLAG_NO_HIBERNATE_SPINDOWN) &&
+	    system_entering_hibernation())
+		return;
+
+	ata_tf_init(dev, &tf);
+	tf.flags |= ATA_TFLAG_DEVICE | ATA_TFLAG_ISADDR;
+	tf.protocol = ATA_PROT_NODATA;
+	tf.command = ATA_CMD_STANDBYNOW1;
+
+	ata_dev_notice(dev, "Entering standby power mode\n");
+
+	err_mask = ata_exec_internal(dev, &tf, NULL, DMA_NONE, NULL, 0, 0);
+	if (err_mask)
+		ata_dev_err(dev, "STANDBY IMMEDIATE failed (err_mask=0x%x)\n",
+			    err_mask);
+}
+
+/**
+ *	ata_dev_power_set_active -  Set a device power mode to active
+ *	@dev: target device
+ *
+ *	Issue a VERIFY command to enter to ensure that the device is in the
+ *	active power mode. For a spun-down HDD (standby or idle power mode),
+ *	the VERIFY command will complete after the disk spins up.
+ *
+ *	LOCKING:
+ *	Kernel thread context (may sleep).
+ */
+void ata_dev_power_set_active(struct ata_device *dev)
+{
+	struct ata_taskfile tf;
+	unsigned int err_mask;
+
+	/*
+	 * Issue READ VERIFY SECTORS command for 1 sector at lba=0 only
+	 * if supported by the device.
+	 */
+	if (dev->class != ATA_DEV_ATA && dev->class != ATA_DEV_ZAC)
+		return;
+
+	ata_tf_init(dev, &tf);
+	tf.flags |= ATA_TFLAG_DEVICE | ATA_TFLAG_ISADDR;
+	tf.protocol = ATA_PROT_NODATA;
+	tf.command = ATA_CMD_VERIFY;
+	tf.nsect = 1;
+	if (dev->flags & ATA_DFLAG_LBA) {
+		tf.flags |= ATA_TFLAG_LBA;
+		tf.device |= ATA_LBA;
+	} else {
+		/* CHS */
+		tf.lbal = 0x1; /* sect */
+	}
+
+	ata_dev_notice(dev, "Entering active power mode\n");
+
+	err_mask = ata_exec_internal(dev, &tf, NULL, DMA_NONE, NULL, 0, 0);
+	if (err_mask)
+		ata_dev_err(dev, "VERIFY failed (err_mask=0x%x)\n",
+			    err_mask);
+}
+
 /**
  *	ata_read_log_page - read a specific log page
  *	@dev: target device

index 73428ad0c8d2..a0e58d22d222 100644
@@ -1050,15 +1050,13 @@ int ata_scsi_dev_config(struct scsi_device *sdev, struct ata_device *dev)
 		}
 	} else {
 		sdev->sector_size = ata_id_logical_sector_size(dev->id);
+
 		/*
-		 * Stop the drive on suspend but do not issue START STOP UNIT
-		 * on resume as this is not necessary and may fail: the device
-		 * will be woken up by ata_port_pm_resume() with a port reset
-		 * and device revalidation.
+		 * Ask the sd driver to issue START STOP UNIT on runtime suspend
+		 * and resume only. For system level suspend/resume, devices
+		 * power state is handled directly by libata EH.
 		 */
-		sdev->manage_system_start_stop = true;
 		sdev->manage_runtime_start_stop = true;
-		sdev->no_start_on_resume = 1;
 	}
 
 	/*

@@ -1231,7 +1229,7 @@ static unsigned int ata_scsi_start_stop_xlat(struct ata_queued_cmd *qc)
 	}
 
 	if (cdb[4] & 0x1) {
-		tf->nsect = 1;	/* 1 sector, lba=0 */
+		tf->nsect = 1;  /* 1 sector, lba=0 */
 
 		if (qc->dev->flags & ATA_DFLAG_LBA) {
 			tf->flags |= ATA_TFLAG_LBA;

@@ -1247,7 +1245,7 @@ static unsigned int ata_scsi_start_stop_xlat(struct ata_queued_cmd *qc)
 			tf->lbah = 0x0; /* cyl high */
 		}
 
-		tf->command = ATA_CMD_VERIFY;	/* READ VERIFY */
+		tf->command = ATA_CMD_VERIFY;   /* READ VERIFY */
 	} else {
 		/* Some odd clown BIOSen issue spindown on power off (ACPI S4
 		 * or S5) causing some drives to spin up and down again.

@@ -1257,7 +1255,7 @@ static unsigned int ata_scsi_start_stop_xlat(struct ata_queued_cmd *qc)
 			goto skip;
 
 		if ((qc->ap->flags & ATA_FLAG_NO_HIBERNATE_SPINDOWN) &&
-		     system_entering_hibernation())
+		    system_entering_hibernation())
 			goto skip;
 
 		/* Issue ATA STANDBY IMMEDIATE command */

index 6e7d352803bd..820299bd9d06 100644
@@ -60,6 +60,8 @@ extern int ata_dev_reread_id(struct ata_device *dev, unsigned int readid_flags);
 extern int ata_dev_revalidate(struct ata_device *dev, unsigned int new_class,
 			      unsigned int readid_flags);
 extern int ata_dev_configure(struct ata_device *dev);
+extern void ata_dev_power_set_standby(struct ata_device *dev);
+extern void 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,

index 4ece1b7a2a5b..00b4a2b7819a 100644
@@ -192,6 +192,7 @@ enum {
 	ATA_PFLAG_UNLOADING	= (1 << 9), /* driver is being unloaded */
 	ATA_PFLAG_UNLOADED	= (1 << 10), /* driver is unloaded */
 
+	ATA_PFLAG_RESUMING	= (1 << 16),  /* port is being resumed */
 	ATA_PFLAG_SUSPENDED	= (1 << 17), /* port is suspended (power) */
 	ATA_PFLAG_PM_PENDING	= (1 << 18), /* PM operation pending */
 	ATA_PFLAG_INIT_GTM_VALID = (1 << 19), /* initial gtm data valid */

@@ -318,9 +319,10 @@ enum {
 	ATA_EH_ENABLE_LINK	= (1 << 3),
 	ATA_EH_PARK		= (1 << 5), /* unload heads and stop I/O */
 	ATA_EH_GET_SUCCESS_SENSE = (1 << 6), /* Get sense data for successful cmd */
+	ATA_EH_SET_ACTIVE	= (1 << 7), /* Set a device to active power mode */
 
 	ATA_EH_PERDEV_MASK	= ATA_EH_REVALIDATE | ATA_EH_PARK |
-				  ATA_EH_GET_SUCCESS_SENSE,
+				  ATA_EH_GET_SUCCESS_SENSE | ATA_EH_SET_ACTIVE,
 	ATA_EH_ALL_ACTIONS	= ATA_EH_REVALIDATE | ATA_EH_RESET |
 				  ATA_EH_ENABLE_LINK,
 

@@ -357,7 +359,7 @@ enum {
 	/* This should match the actual table size of
 	 * ata_eh_cmd_timeout_table in libata-eh.c.
 	 */
-	ATA_EH_CMD_TIMEOUT_TABLE_SIZE = 7,
+	ATA_EH_CMD_TIMEOUT_TABLE_SIZE = 8,
 
 	/* Horkage types. May be set by libata or controller on drives
 	   (some horkage may be drive/controller pair dependent */

2.41.0

scsi_rescan_device() takes a scsi device lock before executing a device handler and device driver rescan methods. Waiting for the completion of any command issued to the device by these methods will thus be done with the device lock held. As a result, there is a risk of deadlocking within the power management code if scsi_rescan_device() is called to handle a device resume with the associated scsi device not yet resumed.

Avoid such situation by checking that the target scsi device is in the running state, that is, fully capable of executing commands, before proceeding with the rescan and bailout returning -EWOULDBLOCK otherwise. With this error return, the caller can retry rescaning the device after a delay.

The state check is done with the device lock held and is thus safe against incoming suspend power management operations.

---
 drivers/scsi/scsi_scan.c | 18 +++++++++++++++++-
 include/scsi/scsi_host.h |  2 +-
 2 files changed, 18 insertions(+), 2 deletions(-)

index 52014b2d39e1..3db4d31a03a1 100644
@@ -1619,12 +1619,24 @@ int scsi_add_device(struct Scsi_Host *host, uint channel,
 }
 EXPORT_SYMBOL(scsi_add_device);
 
-void scsi_rescan_device(struct scsi_device *sdev)
+int scsi_rescan_device(struct scsi_device *sdev)
 {
 	struct device *dev = &sdev->sdev_gendev;
+	int ret = 0;
 
 	device_lock(dev);
 
+	/*
+	 * Bail out if the device is not running. Otherwise, the rescan may
+	 * block waiting for commands to be executed, with us holding the
+	 * device lock. This can result in a potential deadlock in the power
+	 * management core code when system resume is on-going.
+	 */
+	if (sdev->sdev_state != SDEV_RUNNING) {
+		ret = -EWOULDBLOCK;
+		goto unlock;
+	}
+
 	scsi_attach_vpd(sdev);
 	scsi_cdl_check(sdev);
 

@@ -1638,7 +1650,11 @@ void scsi_rescan_device(struct scsi_device *sdev)
 			drv->rescan(dev);
 		module_put(dev->driver->owner);
 	}
+
+unlock:
 	device_unlock(dev);
+
+	return ret;
 }
 EXPORT_SYMBOL(scsi_rescan_device);
 

index 49f768d0ff37..4c2dc8150c6d 100644
@@ -764,7 +764,7 @@ scsi_template_proc_dir(const struct scsi_host_template *sht);
 #define scsi_template_proc_dir(sht) NULL
 #endif
 extern void scsi_scan_host(struct Scsi_Host *);
-extern void scsi_rescan_device(struct scsi_device *);
+extern int scsi_rescan_device(struct scsi_device *sdev);
 extern void scsi_remove_host(struct Scsi_Host *);
 extern struct Scsi_Host *scsi_host_get(struct Scsi_Host *);
 extern int scsi_host_busy(struct Scsi_Host *shost);

2.41.0

Damien,

Martin K. Petersen	Oracle Linux Engineering

Commit 6aa0365a3c85 ("ata: libata-scsi: Avoid deadlock on rescan after device resume") modified ata_scsi_dev_rescan() to check the scsi device "is_suspended" power field to ensure that the scsi device associated with an ATA device is fully resumed when scsi_rescan_device() is executed. However, this fix is problematic as:
1) It relies on a PM internal field that should not be used without PM device locking protection.
2) The check for is_suspended and the call to scsi_rescan_device() are not atomic and a suspend PM event may be triggered between them, casuing scsi_rescan_device() to be called on a suspended device and in that function blocking while holding the scsi device lock. This would deadlock a following resume operation.
These problems can trigger PM deadlocks on resume, especially with resume operations triggered quickly after or during suspend operations. E.g., a simple bash script like:

for (( i=0; i<10; i++ )); do
echo "+2 > /sys/class/rtc/rtc0/wakealarm
echo mem > /sys/power/state
done

that triggers a resume 2 seconds after starting suspending a system can quickly lead to a PM deadlock preventing the system from correctly resuming.

Fix this by replacing the check on is_suspended with a check on the return value given by scsi_rescan_device() as that function will fail if called against a suspended device. Also make sure rescan tasks already scheduled are first cancelled before suspending an ata port.

---
 drivers/ata/libata-core.c | 16 ++++++++++++++++
 drivers/ata/libata-scsi.c | 33 +++++++++++++++------------------
 2 files changed, 31 insertions(+), 18 deletions(-)

index a0bc01606b30..092372334e92 100644
@@ -5168,11 +5168,27 @@ static const unsigned int ata_port_suspend_ehi = ATA_EHI_QUIET
 
 static void ata_port_suspend(struct ata_port *ap, pm_message_t mesg)
 {
+	/*
+	 * We are about to suspend the port, so we do not care about
+	 * scsi_rescan_device() calls scheduled by previous resume operations.
+	 * The next resume will schedule the rescan again. So cancel any rescan
+	 * that is not done yet.
+	 */
+	cancel_delayed_work_sync(&ap->scsi_rescan_task);
+
 	ata_port_request_pm(ap, mesg, 0, ata_port_suspend_ehi, false);
 }
 
 static void ata_port_suspend_async(struct ata_port *ap, pm_message_t mesg)
 {
+	/*
+	 * We are about to suspend the port, so we do not care about
+	 * scsi_rescan_device() calls scheduled by previous resume operations.
+	 * The next resume will schedule the rescan again. So cancel any rescan
+	 * that is not done yet.
+	 */
+	cancel_delayed_work_sync(&ap->scsi_rescan_task);
+
 	ata_port_request_pm(ap, mesg, 0, ata_port_suspend_ehi, true);
 }
 

index a0e58d22d222..6850cac803c1 100644
@@ -4756,7 +4756,7 @@ void ata_scsi_dev_rescan(struct work_struct *work)
 	struct ata_link *link;
 	struct ata_device *dev;
 	unsigned long flags;
-	bool delay_rescan = false;
+	int ret = 0;
 
 	mutex_lock(&ap->scsi_scan_mutex);
 	spin_lock_irqsave(ap->lock, flags);

@@ -4765,37 +4765,34 @@ void ata_scsi_dev_rescan(struct work_struct *work)
 		ata_for_each_dev(dev, link, ENABLED) {
 			struct scsi_device *sdev = dev->sdev;
 
+			/*
+			 * If the port was suspended before this was scheduled,
+			 * bail out.
+			 */
+			if (ap->pflags & ATA_PFLAG_SUSPENDED)
+				goto unlock;
+
 			if (!sdev)
 				continue;
 			if (scsi_device_get(sdev))
 				continue;
 
-			/*
-			 * If the rescan work was scheduled because of a resume
-			 * event, the port is already fully resumed, but the
-			 * SCSI device may not yet be fully resumed. In such
-			 * case, executing scsi_rescan_device() may cause a
-			 * deadlock with the PM code on device_lock(). Prevent
-			 * this by giving up and retrying rescan after a short
-			 * delay.
-			 */
-			delay_rescan = sdev->sdev_gendev.power.is_suspended;
-			if (delay_rescan) {
-				scsi_device_put(sdev);
-				break;
-			}
-
 			spin_unlock_irqrestore(ap->lock, flags);
-			scsi_rescan_device(sdev);
+			ret = scsi_rescan_device(sdev);
 			scsi_device_put(sdev);
 			spin_lock_irqsave(ap->lock, flags);
+
+			if (ret)
+				goto unlock;
 		}
 	}
 
+unlock:
 	spin_unlock_irqrestore(ap->lock, flags);
 	mutex_unlock(&ap->scsi_scan_mutex);
 
-	if (delay_rescan)
+	/* Reschedule with a delay if scsi_rescan_device() returned an error */
+	if (ret)
 		schedule_delayed_work(&ap->scsi_rescan_task,
 				      msecs_to_jiffies(5));
 }

2.41.0

libsas does its own domain based power management of ports. For such ports, libata should not use a device type defining power management operations as executing these operations for suspend/resume in addition to libsas calls to ata_sas_port_suspend() and ata_sas_port_resume() is not necessary (and likely dangerous to do, even though problems are not seen currently).

Introduce the new ata_port_sas_type device_type for ports managed by libsas. This new device type is used in ata_tport_add() and is defined without power management operations.

---
 drivers/ata/libata-core.c      | 2 +-
 drivers/ata/libata-transport.c | 9 ++++++++-
 drivers/ata/libata.h           | 2 ++
 3 files changed, 11 insertions(+), 2 deletions(-)

index 092372334e92..261445c1851b 100644
@@ -5335,7 +5335,7 @@ EXPORT_SYMBOL_GPL(ata_host_resume);
 #endif
 
 const struct device_type ata_port_type = {
-	.name = "ata_port",
+	.name = ATA_PORT_TYPE_NAME,
 #ifdef CONFIG_PM
 	.pm = &ata_port_pm_ops,
 #endif

index e4fb9d1b9b39..3e49a877500e 100644
@@ -266,6 +266,10 @@ void ata_tport_delete(struct ata_port *ap)
 	put_device(dev);
 }
 
+static const struct device_type ata_port_sas_type = {
+	.name = ATA_PORT_TYPE_NAME,
+};
+
 /** ata_tport_add - initialize a transport ATA port structure
  *
  * @parent:	parent device

@@ -283,7 +287,10 @@ int ata_tport_add(struct device *parent,
 	struct device *dev = &ap->tdev;
 
 	device_initialize(dev);
-	dev->type = &ata_port_type;
+	if (ap->flags & ATA_FLAG_SAS_HOST)
+		dev->type = &ata_port_sas_type;
+	else
+		dev->type = &ata_port_type;
 
 	dev->parent = parent;
 	ata_host_get(ap->host);

index 820299bd9d06..05ac80da8ebc 100644
@@ -30,6 +30,8 @@ enum {
 	ATA_DNXFER_QUIET	= (1 << 31),
 };
 
+#define ATA_PORT_TYPE_NAME	"ata_port"
+
 extern atomic_t ata_print_id;
 extern int atapi_passthru16;
 extern int libata_fua;

2.41.0

If an error occurs when resuming a host adapter before the devices attached to the adapter are resumed, the adapter low level driver may remove the scsi host, resulting in a call to sd_remove() for the disks of the host. This in turn results in a call to sd_shutdown() which will issue a synchronize cache command and a start stop unit command to spindown the disk. sd_shutdown() issues the commands only if the device is not already runtime suspended but does not check the power state for system-wide suspend/resume. That is, the commands may be issued with the device in a suspended state, which causes PM resume to hang, forcing a reset of the machine to recover.

Fix this by tracking the suspended state of a disk by introducing the suspended boolean field in the scsi_disk structure. This flag is set to true when the disk is suspended is sd_suspend_common() and resumed with sd_resume(). When suspended is true, sd_shutdown() is not executed from sd_remove().

---
 drivers/scsi/sd.c | 17 +++++++++++++----
 drivers/scsi/sd.h |  1 +
 2 files changed, 14 insertions(+), 4 deletions(-)

index a1ef4eef904f..f911a64521e6 100644
@@ -3741,7 +3741,8 @@ static int sd_remove(struct device *dev)
 
 	device_del(&sdkp->disk_dev);
 	del_gendisk(sdkp->disk);
-	sd_shutdown(dev);
+	if (!sdkp->suspended)
+		sd_shutdown(dev);
 
 	put_disk(sdkp->disk);
 	return 0;

@@ -3872,6 +3873,9 @@ static int sd_suspend_common(struct device *dev, bool runtime)
 			ret = 0;
 	}
 
+	if (!ret)
+		sdkp->suspended = true;
+
 	return ret;
 }
 

@@ -3891,21 +3895,26 @@ static int sd_suspend_runtime(struct device *dev)
 static int sd_resume(struct device *dev, bool runtime)
 {
 	struct scsi_disk *sdkp = dev_get_drvdata(dev);
-	int ret;
+	int ret = 0;
 
 	if (!sdkp)	/* E.g.: runtime resume at the start of sd_probe() */
 		return 0;
 
-	if (!sd_do_start_stop(sdkp->device, runtime))
+	if (!sd_do_start_stop(sdkp->device, runtime)) {
+		sdkp->suspended = false;
 		return 0;
+	}
 
 	if (!sdkp->device->no_start_on_resume) {
 		sd_printk(KERN_NOTICE, sdkp, "Starting disk\n");
 		ret = sd_start_stop_device(sdkp, 1);
 	}
 
-	if (!ret)
+	if (!ret) {
 		opal_unlock_from_suspend(sdkp->opal_dev);
+		sdkp->suspended = false;
+	}
+
 	return ret;
 }
 

index 5eea762f84d1..14153ef7a414 100644
@@ -131,6 +131,7 @@ struct scsi_disk {
 	u8		provisioning_mode;
 	u8		zeroing_mode;
 	u8		nr_actuators;		/* Number of actuators */
+	bool		suspended;	/* Disk is supended (stopped) */
 	unsigned	ATO : 1;	/* state of disk ATO bit */
 	unsigned	cache_override : 1; /* temp override of WCE,RCD */
 	unsigned	WCE : 1;	/* state of disk WCE bit */

2.41.0

Damien,

Martin K. Petersen	Oracle Linux Engineering

The 24 bytes length allocated to the ncq_desc string in ata_dev_config_lba() for ata_dev_config_ncq() to use is too short, causing the following gcc compilation warnings when compiling with W=1:

drivers/ata/libata-core.c: In function ‘ata_dev_configure’:
drivers/ata/libata-core.c:2378:56: warning: ‘%d’ directive output may be truncated writing between 1 and 2 bytes into a region of size between 1 and 11 [-Wformat-truncation=]
2378 | snprintf(desc, desc_sz, "NCQ (depth %d/%d)%s", hdepth,
| ^~
In function ‘ata_dev_config_ncq’,
inlined from ‘ata_dev_config_lba’ at drivers/ata/libata-core.c:2649:8,
inlined from ‘ata_dev_configure’ at drivers/ata/libata-core.c:2952:9:
drivers/ata/libata-core.c:2378:41: note: directive argument in the range [1, 32]
2378 | snprintf(desc, desc_sz, "NCQ (depth %d/%d)%s", hdepth,

      |                                         ^~~~~~~~~~~~~~~~~~~~~
drivers/ata/libata-core.c:2378:17: note: ‘snprintf’ output between 16 and 31 bytes into a destination of size 24
 2378 |                 snprintf(desc, desc_sz, "NCQ (depth %d/%d)%s", hdepth,
      |                 ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
 2379 |                         ddepth, aa_desc);
      |                         ~~~~~~~~~~~~~~~~

Avoid these warnings and the potential truncation by changing the size of the ncq_desc string to 32 characters.

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

index 261445c1851b..d8cc1e27a125 100644
@@ -2619,7 +2619,7 @@ static int ata_dev_config_lba(struct ata_device *dev)
 {
 	const u16 *id = dev->id;
 	const char *lba_desc;
-	char ncq_desc[24];
+	char ncq_desc[32];
 	int ret;
 
 	dev->flags |= ATA_DFLAG_LBA;

2.41.0

The 6 bytes length of the tries_buf string in ata_eh_link_report() is too short and results in a gcc compilation warning with W-!:

drivers/ata/libata-eh.c: In function ‘ata_eh_link_report’:
drivers/ata/libata-eh.c:2371:59: warning: ‘%d’ directive output may be truncated writing between 1 and 11 bytes into a region of size 4 [-Wformat-truncation=]
2371 | snprintf(tries_buf, sizeof(tries_buf), " t%d",
| ^~
drivers/ata/libata-eh.c:2371:56: note: directive argument in the range [-2147483648, 4]
2371 | snprintf(tries_buf, sizeof(tries_buf), " t%d",
| ^~~~~~
drivers/ata/libata-eh.c:2371:17: note: ‘snprintf’ output between 4 and 14 bytes into a destination of size 6

 2371 |                 snprintf(tries_buf, sizeof(tries_buf), " t%d",
      |                 ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
 2372 |                          ap->eh_tries);
      |                          ~~~~~~~~~~~~~

Avoid this warning by increasing the string size to 16B.

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

index b1b2c276371e..5686353e442c 100644
@@ -2332,7 +2332,7 @@ static void ata_eh_link_report(struct ata_link *link)
 	struct ata_eh_context *ehc = &link->eh_context;
 	struct ata_queued_cmd *qc;
 	const char *frozen, *desc;
-	char tries_buf[6] = "";
+	char tries_buf[16] = "";
 	int tag, nr_failed = 0;
 
 	if (ehc->i.flags & ATA_EHI_QUIET)

2.41.0

The scsi device flag no_start_on_resume is not set by any scsi low level driver. Remove it. This reverts the changes introduced by commit 0a8589055936 ("ata,scsi: do not issue START STOP UNIT on resume").

---
 drivers/scsi/sd.c          | 9 +++------
 include/scsi/scsi_device.h | 1 -
 2 files changed, 3 insertions(+), 7 deletions(-)

index f911a64521e6..37a7d9cd2df3 100644
@@ -3895,7 +3895,7 @@ static int sd_suspend_runtime(struct device *dev)
 static int sd_resume(struct device *dev, bool runtime)
 {
 	struct scsi_disk *sdkp = dev_get_drvdata(dev);
-	int ret = 0;
+	int ret;
 
 	if (!sdkp)	/* E.g.: runtime resume at the start of sd_probe() */
 		return 0;

@@ -3905,11 +3905,8 @@ static int sd_resume(struct device *dev, bool runtime)
 		return 0;
 	}
 
-	if (!sdkp->device->no_start_on_resume) {
-		sd_printk(KERN_NOTICE, sdkp, "Starting disk\n");
-		ret = sd_start_stop_device(sdkp, 1);
-	}
-
+	sd_printk(KERN_NOTICE, sdkp, "Starting disk\n");
+	ret = sd_start_stop_device(sdkp, 1);
 	if (!ret) {
 		opal_unlock_from_suspend(sdkp->opal_dev);
 		sdkp->suspended = false;

index fd41fdac0a8e..e6e476fe2272 100644
@@ -197,7 +197,6 @@ struct scsi_device {
 	unsigned use_192_bytes_for_3f:1; /* ask for 192 bytes from page 0x3f */
 	unsigned no_start_on_add:1;	/* do not issue start on add */
 	unsigned allow_restart:1; /* issue START_UNIT in error handler */
-	unsigned no_start_on_resume:1; /* Do not issue START_STOP_UNIT on resume */
 	unsigned start_stop_pwr_cond:1;	/* Set power cond. in START_STOP_UNIT */
 	unsigned no_uld_attach:1; /* disable connecting to upper level drivers */
 	unsigned select_no_atn:1;

2.41.0

Damien,

Martin K. Petersen	Oracle Linux Engineering

Now that libata does its own internal device power mode management through libata EH, the scsi disk driver will not issue START STOP UNIT commands anymore. We can receive this command only from user passthrough operations. So there is no need to consider the system state and ATA port flags for suspend to translate the command.

Since setting up the taskfile for the verify and standby immediate commands is the same as done in ata_dev_power_set_active() and ata_dev_power_set_standby(), factor out this code into the helper function ata_dev_power_init_tf() to simplify ata_scsi_start_stop_xlat() as well as ata_dev_power_set_active() and ata_dev_power_set_standby().

---
 drivers/ata/libata-core.c | 55 +++++++++++++++++++++++----------------
 drivers/ata/libata-scsi.c | 53 +++++++------------------------------
 drivers/ata/libata.h      |  2 ++
 3 files changed, 44 insertions(+), 66 deletions(-)

index d8cc1e27a125..8e326a445765 100644
@@ -1972,6 +1972,35 @@ int ata_dev_read_id(struct ata_device *dev, unsigned int *p_class,
 	return rc;
 }
 
+bool ata_dev_power_init_tf(struct ata_device *dev, struct ata_taskfile *tf,
+			   bool set_active)
+{
+	/* Only applies to ATA and ZAC devices */
+	if (dev->class != ATA_DEV_ATA && dev->class != ATA_DEV_ZAC)
+		return false;
+
+	ata_tf_init(dev, tf);
+	tf->flags |= ATA_TFLAG_DEVICE | ATA_TFLAG_ISADDR;
+	tf->protocol = ATA_PROT_NODATA;
+
+	if (set_active) {
+		/* VERIFY for 1 sector at lba=0 */
+		tf->command = ATA_CMD_VERIFY;
+		tf->nsect = 1;
+		if (dev->flags & ATA_DFLAG_LBA) {
+			tf->flags |= ATA_TFLAG_LBA;
+			tf->device |= ATA_LBA;
+		} else {
+			/* CHS */
+			tf->lbal = 0x1; /* sect */
+		}
+	} else {
+		tf->command = ATA_CMD_STANDBYNOW1;
+	}
+
+	return true;
+}
+
 /**
  *	ata_dev_power_set_standby - Set a device power mode to standby
  *	@dev: target device

@@ -1988,10 +2017,6 @@ void ata_dev_power_set_standby(struct ata_device *dev)
 	struct ata_taskfile tf;
 	unsigned int err_mask;
 
-	/* Issue STANDBY IMMEDIATE command only if supported by the device */
-	if (dev->class != ATA_DEV_ATA && dev->class != ATA_DEV_ZAC)
-		return;
-
 	/*
 	 * Some odd clown BIOSes issue spindown on power off (ACPI S4 or S5)
 	 * causing some drives to spin up and down again. For these, do nothing

@@ -2005,10 +2030,9 @@ void ata_dev_power_set_standby(struct ata_device *dev)
 	    system_entering_hibernation())
 		return;
 
-	ata_tf_init(dev, &tf);
-	tf.flags |= ATA_TFLAG_DEVICE | ATA_TFLAG_ISADDR;
-	tf.protocol = ATA_PROT_NODATA;
-	tf.command = ATA_CMD_STANDBYNOW1;
+	/* Issue STANDBY IMMEDIATE command only if supported by the device */
+	if (!ata_dev_power_init_tf(dev, &tf, false))
+		return;
 
 	ata_dev_notice(dev, "Entering standby power mode\n");
 

@@ -2038,22 +2062,9 @@ void ata_dev_power_set_active(struct ata_device *dev)
 	 * Issue READ VERIFY SECTORS command for 1 sector at lba=0 only
 	 * if supported by the device.
 	 */
-	if (dev->class != ATA_DEV_ATA && dev->class != ATA_DEV_ZAC)
+	if (!ata_dev_power_init_tf(dev, &tf, true))
 		return;
 
-	ata_tf_init(dev, &tf);
-	tf.flags |= ATA_TFLAG_DEVICE | ATA_TFLAG_ISADDR;
-	tf.protocol = ATA_PROT_NODATA;
-	tf.command = ATA_CMD_VERIFY;
-	tf.nsect = 1;
-	if (dev->flags & ATA_DFLAG_LBA) {
-		tf.flags |= ATA_TFLAG_LBA;
-		tf.device |= ATA_LBA;
-	} else {
-		/* CHS */
-		tf.lbal = 0x1; /* sect */
-	}
-
 	ata_dev_notice(dev, "Entering active power mode\n");
 
 	err_mask = ata_exec_internal(dev, &tf, NULL, DMA_NONE, NULL, 0, 0);

index 05ac80da8ebc..5c685bb1939e 100644
@@ -62,6 +62,8 @@ extern int ata_dev_reread_id(struct ata_device *dev, unsigned int readid_flags);
 extern int ata_dev_revalidate(struct ata_device *dev, unsigned int new_class,
 			      unsigned int readid_flags);
 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 int sata_down_spd_limit(struct ata_link *link, u32 spd_limit);

2.41.0

The call to async_synchronize_cookie() to synchronize a port removal and hotplug probe is done in ata_host_detach() right before calling ata_port_detach(). Move this call at the beginning of ata_port_detach() to ensure that this operation is always synchronized with probe.

---
 drivers/ata/libata-core.c | 8 ++++----
 1 file changed, 4 insertions(+), 4 deletions(-)

index 8e326a445765..de661780a31e 100644
@@ -6065,6 +6065,9 @@ static void ata_port_detach(struct ata_port *ap)
 	struct ata_link *link;
 	struct ata_device *dev;
 
+	/* Ensure ata_port probe has completed */
+	async_synchronize_cookie(ap->cookie + 1);
+
 	/* Wait for any ongoing EH */
 	ata_port_wait_eh(ap);
 

@@ -6129,11 +6132,8 @@ void ata_host_detach(struct ata_host *host)
 {
 	int i;
 
-	for (i = 0; i < host->n_ports; i++) {
-		/* Ensure ata_port probe has completed */
-		async_synchronize_cookie(host->ports[i]->cookie + 1);
+	for (i = 0; i < host->n_ports; i++)
 		ata_port_detach(host->ports[i]);
-	}
 
 	/* the host is dead now, dissociate ACPI */
 	ata_acpi_dissociate(host);

2.41.0

Modify ata_pci_shutdown_one() to schedule EH to unload a port devices before freezing and thawing the port. This ensures that drives are cleanly disabled and transitioned to standby power mode when a PCI adapter is removed or the system is powered off.

---
 drivers/ata/libata-core.c | 16 +++++++++++++++-
 1 file changed, 15 insertions(+), 1 deletion(-)

index de661780a31e..6b38ebaad019 100644
@@ -6164,10 +6164,24 @@ EXPORT_SYMBOL_GPL(ata_pci_remove_one);
 void ata_pci_shutdown_one(struct pci_dev *pdev)
 {
 	struct ata_host *host = pci_get_drvdata(pdev);
+	struct ata_port *ap;
+	unsigned long flags;
 	int i;
 
+	/* Tell EH to disable all devices */
 	for (i = 0; i < host->n_ports; i++) {
-		struct ata_port *ap = host->ports[i];
+		ap = host->ports[i];
+		spin_lock_irqsave(ap->lock, flags);
+		ap->pflags |= ATA_PFLAG_UNLOADING;
+		ata_port_schedule_eh(ap);
+		spin_unlock_irqrestore(ap->lock, flags);
+	}
+
+	for (i = 0; i < host->n_ports; i++) {
+		ap = host->ports[i];
+
+		/* Wait for EH to complete before freezing the port */
+		ata_port_wait_eh(ap);
 
 		ap->pflags |= ATA_PFLAG_FROZEN;
 

2.41.0

ata_port_suspend_async() is only called by ata_sas_port_suspend(). Modify ata_port_suspend() with an additional bool argument indicating an asynchronous or synchronous suspend to allow removing that helper function. With this change, the variable ata_port_resume_ehi can also be removed and its value (ATA_EHI_XXX flags passed directly to ata_port_request_pm().

---
 drivers/ata/libata-core.c | 46 +++++++++++++++------------------------
 1 file changed, 17 insertions(+), 29 deletions(-)

index 6b38ebaad019..291fc686ff08 100644
@@ -5166,18 +5166,8 @@ static void ata_port_request_pm(struct ata_port *ap, pm_message_t mesg,
 		ata_port_wait_eh(ap);
 }
 
-/*
- * On some hardware, device fails to respond after spun down for suspend.  As
- * the device won't be used before being resumed, we don't need to touch the
- * device.  Ask EH to skip the usual stuff and proceed directly to suspend.
- *
- * http://thread.gmane.org/gmane.linux.ide/46764
- */
-static const unsigned int ata_port_suspend_ehi = ATA_EHI_QUIET
-						 | ATA_EHI_NO_AUTOPSY
-						 | ATA_EHI_NO_RECOVERY;
-
-static void ata_port_suspend(struct ata_port *ap, pm_message_t mesg)
+static void ata_port_suspend(struct ata_port *ap, pm_message_t mesg,
+			     bool async)
 {
 	/*
 	 * We are about to suspend the port, so we do not care about

@@ -5187,20 +5177,18 @@ static void ata_port_suspend(struct ata_port *ap, pm_message_t mesg)
 	 */
 	cancel_delayed_work_sync(&ap->scsi_rescan_task);
 
-	ata_port_request_pm(ap, mesg, 0, ata_port_suspend_ehi, false);
-}
-
-static void ata_port_suspend_async(struct ata_port *ap, pm_message_t mesg)
-{
 	/*
-	 * We are about to suspend the port, so we do not care about
-	 * scsi_rescan_device() calls scheduled by previous resume operations.
-	 * The next resume will schedule the rescan again. So cancel any rescan
-	 * that is not done yet.
+	 * On some hardware, device fails to respond after spun down for
+	 * suspend. As the device will not be used until being resumed, we
+	 * do not need to touch the device. Ask EH to skip the usual stuff
+	 * and proceed directly to suspend.
+	 *
+	 * http://thread.gmane.org/gmane.linux.ide/46764
 	 */
-	cancel_delayed_work_sync(&ap->scsi_rescan_task);
-
-	ata_port_request_pm(ap, mesg, 0, ata_port_suspend_ehi, true);
+	ata_port_request_pm(ap, mesg, 0,
+			    ATA_EHI_QUIET | ATA_EHI_NO_AUTOPSY |
+			    ATA_EHI_NO_RECOVERY,
+			    async);
 }
 
 static int ata_port_pm_suspend(struct device *dev)

@@ -5210,7 +5198,7 @@ static int ata_port_pm_suspend(struct device *dev)
 	if (pm_runtime_suspended(dev))
 		return 0;
 
-	ata_port_suspend(ap, PMSG_SUSPEND);
+	ata_port_suspend(ap, PMSG_SUSPEND, false);
 	return 0;
 }
 

@@ -5221,13 +5209,13 @@ static int ata_port_pm_freeze(struct device *dev)
 	if (pm_runtime_suspended(dev))
 		return 0;
 
-	ata_port_suspend(ap, PMSG_FREEZE);
+	ata_port_suspend(ap, PMSG_FREEZE, false);
 	return 0;
 }
 
 static int ata_port_pm_poweroff(struct device *dev)
 {
-	ata_port_suspend(to_ata_port(dev), PMSG_HIBERNATE);
+	ata_port_suspend(to_ata_port(dev), PMSG_HIBERNATE, false);
 	return 0;
 }
 

@@ -5279,7 +5267,7 @@ static int ata_port_runtime_idle(struct device *dev)
 
 static int ata_port_runtime_suspend(struct device *dev)
 {
-	ata_port_suspend(to_ata_port(dev), PMSG_AUTO_SUSPEND);
+	ata_port_suspend(to_ata_port(dev), PMSG_AUTO_SUSPEND, false);
 	return 0;
 }
 

@@ -5309,7 +5297,7 @@ static const struct dev_pm_ops ata_port_pm_ops = {
  */
 void ata_sas_port_suspend(struct ata_port *ap)
 {
-	ata_port_suspend_async(ap, PMSG_SUSPEND);
+	ata_port_suspend(ap, PMSG_SUSPEND, true);
 }
 EXPORT_SYMBOL_GPL(ata_sas_port_suspend);
 

2.41.0

Remove ata_port_resume_async() and replace it with a modified ata_port_resume() taking an additional bool argument indicating if ata EH resume operation should be executed synchronously or asynchronously. With this change, the variable ata_port_resume_ehi is not longer necessary and its value (ATA_EHI_XXX flags) passed directly to ata_port_request_pm().

---
 drivers/ata/libata-core.c | 21 ++++++++-------------
 1 file changed, 8 insertions(+), 13 deletions(-)

index 291fc686ff08..6773a1e52dad 100644
@@ -5219,22 +5219,17 @@ static int ata_port_pm_poweroff(struct device *dev)
 	return 0;
 }
 
-static const unsigned int ata_port_resume_ehi = ATA_EHI_NO_AUTOPSY
-						| ATA_EHI_QUIET;
-
-static void ata_port_resume(struct ata_port *ap, pm_message_t mesg)
+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_resume_ehi, false);
-}
-
-static void ata_port_resume_async(struct ata_port *ap, pm_message_t mesg)
-{
-	ata_port_request_pm(ap, mesg, ATA_EH_RESET, ata_port_resume_ehi, true);
+	ata_port_request_pm(ap, mesg, ATA_EH_RESET,
+			    ATA_EHI_NO_AUTOPSY | ATA_EHI_QUIET,
+			    async);
 }
 
 static int ata_port_pm_resume(struct device *dev)
 {
-	ata_port_resume_async(to_ata_port(dev), PMSG_RESUME);
+	ata_port_resume(to_ata_port(dev), PMSG_RESUME, true);
 	pm_runtime_disable(dev);
 	pm_runtime_set_active(dev);
 	pm_runtime_enable(dev);

@@ -5273,7 +5268,7 @@ static int ata_port_runtime_suspend(struct device *dev)
 
 static int ata_port_runtime_resume(struct device *dev)
 {
-	ata_port_resume(to_ata_port(dev), PMSG_AUTO_RESUME);
+	ata_port_resume(to_ata_port(dev), PMSG_AUTO_RESUME, false);
 	return 0;
 }
 

@@ -5303,7 +5298,7 @@ EXPORT_SYMBOL_GPL(ata_sas_port_suspend);
 
 void ata_sas_port_resume(struct ata_port *ap)
 {
-	ata_port_resume_async(ap, PMSG_RESUME);
+	ata_port_resume(ap, PMSG_RESUME, true);
 }
 EXPORT_SYMBOL_GPL(ata_sas_port_resume);
 

2.41.0

When powering off, there is no need to suspend a port that has already been runtime suspended. Skip the EH PM request in ata_port_pm_poweroff() in this case.

---
 drivers/ata/libata-core.c | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

index 6773a1e52dad..df6ed386e6fc 100644
@@ -5215,7 +5215,8 @@ static int ata_port_pm_freeze(struct device *dev)
 
 static int ata_port_pm_poweroff(struct device *dev)
 {
-	ata_port_suspend(to_ata_port(dev), PMSG_HIBERNATE, false);
+	if (!pm_runtime_suspended(dev))
+		ata_port_suspend(to_ata_port(dev), PMSG_HIBERNATE, false);
 	return 0;
 }
 

2.41.0

The scsi disk driver does not resume disks that have been runtime suspended by the user. To be consistent with this behavior, do the same for ata ports and skip the PM request in ata_port_pm_resume() if the port was already runtime suspended. With this change, it is no longer necessary to force the PM state of the port to ACTIVE as the PM core code will take care of that when handling runtime resume.

---
 drivers/ata/libata-core.c | 6 ++----
 1 file changed, 2 insertions(+), 4 deletions(-)

index df6ed386e6fc..58f03031a259 100644
@@ -5230,10 +5230,8 @@ static void ata_port_resume(struct ata_port *ap, pm_message_t mesg,
 
 static int ata_port_pm_resume(struct device *dev)
 {
-	ata_port_resume(to_ata_port(dev), PMSG_RESUME, true);
-	pm_runtime_disable(dev);
-	pm_runtime_set_active(dev);
-	pm_runtime_enable(dev);
+	if (!pm_runtime_suspended(dev))
+		ata_port_resume(to_ata_port(dev), PMSG_RESUME, true);
 	return 0;
 }
 

2.41.0

Change ata_sas_slave_configure() to return the return value of ata_scsi_dev_config() to ensure that any error from that function is propagated to libsas.

---
 drivers/ata/libata-sata.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

index a701e1538482..83a9497e48e1 100644
@@ -1182,8 +1182,8 @@ EXPORT_SYMBOL_GPL(ata_sas_tport_delete);
 int ata_sas_slave_configure(struct scsi_device *sdev, struct ata_port *ap)
 {
 	ata_scsi_sdev_config(sdev);
-	ata_scsi_dev_config(sdev, ap->link.device);
-	return 0;
+
+	return ata_scsi_dev_config(sdev, ap->link.device);
 }
 EXPORT_SYMBOL_GPL(ata_sas_slave_configure);
 

2.41.0

Some drives are really slow to spinup on resume, resulting is a very slow response to COMRESET and to error messages such as:

ata1: COMRESET failed (errno=-16)
ata1: link is slow to respond, please be patient (ready=0)
ata1: SATA link up 6.0 Gbps (SStatus 133 SControl 300) ata1.00: configured for UDMA/133

Given that the slowness of the response is indicated with the message "link is slow to respond..." and that resets are retried until the device is detected as online after up to 1min (ata_eh_reset_timeouts), there is no point in printing the "COMRESET failed" error message. Let's not scare the user with non fatal errors and only warn about reset failures in ata_eh_reset() when all reset retries have been exhausted.

---
 drivers/ata/libata-eh.c   | 2 ++
 drivers/ata/libata-sata.c | 1 -
 2 files changed, 2 insertions(+), 1 deletion(-)

index 5686353e442c..67387d602735 100644
@@ -2909,6 +2909,8 @@ int ata_eh_reset(struct ata_link *link, int classify,
 		 */
 		if (ata_is_host_link(link))
 			ata_eh_thaw_port(ap);
+		ata_link_warn(link, "%s failed\n",
+			      reset == hardreset ? "hardreset" : "softreset");
 		goto out;
 	}
 

index 83a9497e48e1..b6656c287175 100644
@@ -621,7 +621,6 @@ int sata_link_hardreset(struct ata_link *link, const unsigned int *timing,
 		/* online is set iff link is online && reset succeeded */
 		if (online)
 			*online = false;
-		ata_link_err(link, "COMRESET failed (errno=%d)\n", rc);
 	}
 	return rc;
 }

2.41.0

There is no point in warning about a device being disabled when we expect it to be, that is, on suspend, shutdown or when detaching the device.

Suppress the message "disable device" for these cases by introducing the EH static function ata_eh_dev_disable() and by using it in ata_eh_unload() and ata_eh_detach_dev(). ata_dev_disable() code is modified to call this new function after printing the "disable device" message.

---
 drivers/ata/libata-eh.c | 32 +++++++++++++++++++-------------
 1 file changed, 19 insertions(+), 13 deletions(-)

index 67387d602735..945675f6b822 100644
@@ -494,6 +494,18 @@ void ata_eh_release(struct ata_port *ap)
 	mutex_unlock(&ap->host->eh_mutex);
 }
 
+static void ata_eh_dev_disable(struct ata_device *dev)
+{
+	ata_acpi_on_disable(dev);
+	ata_down_xfermask_limit(dev, ATA_DNXFER_FORCE_PIO0 | ATA_DNXFER_QUIET);
+	dev->class++;
+
+	/* From now till the next successful probe, ering is used to
+	 * track probe failures.  Clear accumulated device error info.
+	 */
+	ata_ering_clear(&dev->ering);
+}
+
 static void ata_eh_unload(struct ata_port *ap)
 {
 	struct ata_link *link;

@@ -517,8 +529,8 @@ static void ata_eh_unload(struct ata_port *ap)
 	 */
 	ata_for_each_link(link, ap, PMP_FIRST) {
 		sata_scr_write(link, SCR_CONTROL, link->saved_scontrol & 0xff0);
-		ata_for_each_dev(dev, link, ALL)
-			ata_dev_disable(dev);
+		ata_for_each_dev(dev, link, ENABLED)
+			ata_eh_dev_disable(dev);
 	}
 
 	/* freeze and set UNLOADED */

@@ -1211,14 +1223,8 @@ void ata_dev_disable(struct ata_device *dev)
 		return;
 
 	ata_dev_warn(dev, "disable device\n");
-	ata_acpi_on_disable(dev);
-	ata_down_xfermask_limit(dev, ATA_DNXFER_FORCE_PIO0 | ATA_DNXFER_QUIET);
-	dev->class++;
 
-	/* From now till the next successful probe, ering is used to
-	 * track probe failures.  Clear accumulated device error info.
-	 */
-	ata_ering_clear(&dev->ering);
+	ata_eh_dev_disable(dev);
 }
 EXPORT_SYMBOL_GPL(ata_dev_disable);
 

@@ -1240,12 +1246,12 @@ void ata_eh_detach_dev(struct ata_device *dev)
 
 	/*
 	 * If the device is still enabled, transition it to standby power mode
-	 * (i.e. spin down HDDs).
+	 * (i.e. spin down HDDs) and disable it.
 	 */
-	if (ata_dev_enabled(dev))
+	if (ata_dev_enabled(dev)) {
 		ata_dev_power_set_standby(dev);
-
-	ata_dev_disable(dev);
+		ata_eh_dev_disable(dev);
+	}
 
 	spin_lock_irqsave(ap->lock, flags);
 

2.41.0

Simplify the inline DMA helper functions ata_using_mwdma(), ata_using_udma() and ata_dma_enabled() to directly return as a boolean the result of their test condition.

---
 include/linux/libata.h | 18 ++++++++----------
 1 file changed, 8 insertions(+), 10 deletions(-)

index 00b4a2b7819a..3c0fd04b0035 100644
@@ -1881,23 +1881,21 @@ static inline unsigned long ata_deadline(unsigned long from_jiffies,
    change in future hardware and specs, secondly 0xFF means 'no DMA' but is
    > UDMA_0. Dyma ddreigiau */
 
-static inline int ata_using_mwdma(struct ata_device *adev)
+static inline bool ata_using_mwdma(struct ata_device *adev)
 {
-	if (adev->dma_mode >= XFER_MW_DMA_0 && adev->dma_mode <= XFER_MW_DMA_4)
-		return 1;
-	return 0;
+	return adev->dma_mode >= XFER_MW_DMA_0 &&
+		adev->dma_mode <= XFER_MW_DMA_4;
 }
 
-static inline int ata_using_udma(struct ata_device *adev)
+static inline bool ata_using_udma(struct ata_device *adev)
 {
-	if (adev->dma_mode >= XFER_UDMA_0 && adev->dma_mode <= XFER_UDMA_7)
-		return 1;
-	return 0;
+	return adev->dma_mode >= XFER_UDMA_0 &&
+		adev->dma_mode <= XFER_UDMA_7;
 }
 
-static inline int ata_dma_enabled(struct ata_device *adev)
+static inline bool ata_dma_enabled(struct ata_device *adev)
 {
-	return (adev->dma_mode == 0xFF ? 0 : 1);
+	return adev->dma_mode != 0xFF;
 }
 
 /**************************************************************************

2.41.0

I went to test these patches tonight and it looks like Linus already merged them ( or mostly? ). I enabled runtime pm on the scsi target and the ata port, and the disk spins down and the port does too.

I noticed though, that when entering system suspend, a disk that has already been runtime suspended is resumed only to immediately be suspended again before the system suspend. That shouldn't happen should it?

Indeed. Will look at this. Geert also reported seeing an issue with resume (one time only, so I suspect there is still a race with libata-eh). So looks like something is still missing.

Damien Le Moal
Western Digital Research

It seems that it is /sys/power/sync_on_suspend. The problem went away when I unmounted the disk, or I could make the disk wake up by running sync. I thought that it used to be that as long as you mounted an ext4 filesystem with -o relatime, it wouldn't keep dirtying the cache as long as you weren't actually writing to the filesystem. Either I'm misremembering something, or this is no longer the case. Also if there are dirty pages in the cache, I thought the default was for them to be written out after 5 seconds, which would keep the disk awake, rather than waiting until system suspend to sync.

I guess I could mount the filesystem readonly. It's probably not a good idea to disable sync_on_suspend for the whole system.

Hmmm... So this could be the fs suspend then, which issues a sync but the device is already suspended and was synced already. In that case, we should turn that sync into a nop to not wakeup the drive unnecessarily. The fix may be needed on scsi sd side rather than libata.
Let me check.

Damien Le Moal
Western Digital Research

I did some tracing today on a test ext4 fs I created on a loopback device, and it seems that the superblocks are written every time you sync, even if no files on the filesystem have even been opened for read access.

OK. So a fix would need to be on the FS side then if one wants to avoid that useless resume. However, this may clash with the FS need to record stuff in its sb and so we may not be able to avoid that.

Damien Le Moal
Western Digital Research

Ok, this is very strange. I went back to my distro kernel, without runtime pm, mounted the filesystems rw again, used hdparm -y to suspend the disk, verified with hdparm -C that they were in suspend, and and suspended the system. In dmesg I see:

Filesystems sync: 0.007 seconds

Now, if it were writing the superblocks to the disk there, I would expect that to take more like 3 seconds while it woke the disks back up, like it did when I was testing the latest kernel with runtime pm.

Another odd thing I noticed with the runtime pm was that sometimes the drives would randomly start up even though I was not accessing them. This never happens when I am normally using the debian kernel with no runtime pm and just running hdparm -y to put the drives to sleep. I can check them hours later and they are still in standby.

I just tried running sync and blktrace and it looks like it is writing the superblock to the drive, and yet, hdparm -C still says it is in standby. This makes no sense. Here is what blktrace said when I ran sync:

  8,0    0        1     0.000000000 34004  Q FWS [sync]
  8,0    0        2     0.000001335 34004  G FWS [sync]
  8,0    0        3     0.000004327 31088  D  FN [kworker/0:2H]
  8,0    0        4     0.000068945     0  C  FN 0 [0]
  8,0    0        5     0.000069466     0  C  WS 0 [0]

I just noticed that this trace doesn't show the 0+8 that I saw when I was testing running sync with a fresh, empty ext4 filesystem on a loop device. That showed 0+8 indicating the first 4k block of the disk, as well as 1023+8, and one or two more offsets that I thought were the backup superblocks.

What the heck is this sync actually writing, and why does it not cause the disk to take itself out of standby, but with runtime pm, it does? Could this just be a FLUSH of some sort, which when the disk is in standby, it ignores, but the kernel runtime pm decides it must wake the disk up before dispatching the command, even though it is useless?

Hmm... May be there was nothing to sync: hdparm -y putting the drive in standby mode should have synced the write cache already and the FS issued sync may have ended up not causing any media write, as long as the FS did not issue any new write (which would have spun up the drive). The specs are clear about this:

In STANDBY IMMEDIATE description:

Processing a STANDBY IMMEDIATE command shall cause the device to prepare for a power cycle (e.g., flush volatile write cache) prior to returning command completion.

So this is all does not seem that strange to me.

Some random access from user space, e.g. systemd doing its perodic "something" with passthrough commands ?

Same user space in that case ?

Then as mentioned above, nothing may be written, which results in the drive not waking up since the write cache is clean already (synced already before spin down).

Not sure. But it may be a write FUA vs actual sync. With runtime pm, any command issued to the disk while it is suspended will cause a call to pm runtime resume which issues a verify command to spinup the drive, regardless if the command issued by the user needs the drive to spin up. So that is normal. With hdparm -y, the driver thinks the drive is running and so does not issue that verify command to the drive. The drive spinning up or not then depends on the command being issued and the drive state (and also likely the drive model and FW implementation... Some may be more intelligent than others in this area).

Given your description, that is my thinking exactly. The problem here for the second part (spinning up the disk for "useless" commands) is that determining if a command needs the drive to spinup or not is not an easy thing to do, and potentially dangerous if mishandled. One possible micro optimization would be to ignore flush commands to suspended disks. But not sure that is a high win change beside *may be* avoiding a spinup on system suspend witha drive already runtime suspended.

Damien Le Moal
Western Digital Research

Yes. I'll try to leave a blktrace running to see what causes the spinup. I suppose it could be another flush or other command that doesn't require media access, but triggers runtime pm to spin up the disk.

One of the things my patch series from a decade ago did was to use the SLEEP flag in libata to decide to complete certain commands without sending them to the drive so it could remain asleep. I'm not sure if it's even possible for the driver to evaluate the command before the pm core orders a resume though.

I wonder if libata could leave the EH pending and return success from the runtime resume, and then actually run the EH and wake up the drive later, when actual IO is done.

On another note, I've been looking over your patches, and I still do not understand why you added the VERIFY command. The only effect it seems to have is moving the delay while the drive spins up from the first real IO to the resume path. Why does that matter?