Fix libata suspend/resume handling and code cleanup

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 secheduled

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.

Fixes 5ef41082912b ("ata: add ata port system PM callbacks")

Cc [email protected]

Signed-off-by Damien Le Moal <[email protected]>

Reviewed-by Hannes Reinecke <[email protected]>

Tested-by Chia-Lin Kao (AceLan) <[email protected]>

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

drivers/ata/libata-core.c +8 -5

index 74314311295f..c4898483d716 100644
@@ -5040,17 +5040,20 @@ 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);
+		spin_lock_irqsave(ap->lock, flags);
 		WARN_ON(ap->pflags & ATA_PFLAG_PM_PENDING);
 	}
 
-	/* 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) {

Quoted 3 lines · Damien Le Moal

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 secheduled

s/secheduled/scheduled/

Quoted 47 lines

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.

Fixes 5ef41082912b ("ata: add ata port system PM callbacks")

Cc [email protected]

Signed-off-by Damien Le Moal <[email protected]>

Reviewed-by Hannes Reinecke <[email protected]>

Tested-by Chia-Lin Kao (AceLan) <[email protected]>

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

drivers/ata/libata-core.c +8 -5

index 74314311295f..c4898483d716 100644
@@ -5040,17 +5040,20 @@ 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);
+		spin_lock_irqsave(ap->lock, flags);
 		WARN_ON(ap->pflags & ATA_PFLAG_PM_PENDING);
 	}
 
-	/* 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) {

drivers/ata/libata-core.c static void ata_port_request_pm(struct ata_port *ap, pm_message_t mesg, · line 5067

Quoted 7 lines

drivers/ata/libata-core.c +1 -2

@@ -5062,10 +5065,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);

Perhaps you should mention why this WARN_ON() is removed in the commit message.

I don't understand why you keep the WARN_ON() higher up in this function, but remove this WARN_ON(). They seem to have equal worth to me. Perhaps just take and release the lock around the WARN_ON() here as well?

Quoted 65 lines · Niklas Cassel

Quoted 3 lines · Damien Le Moal

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 secheduled

s/secheduled/scheduled/

Quoted 47 lines

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.

Fixes 5ef41082912b ("ata: add ata port system PM callbacks")

Cc [email protected]

Signed-off-by Damien Le Moal <[email protected]>

Reviewed-by Hannes Reinecke <[email protected]>

Tested-by Chia-Lin Kao (AceLan) <[email protected]>

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

drivers/ata/libata-core.c +8 -5

index 74314311295f..c4898483d716 100644
@@ -5040,17 +5040,20 @@ 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);
+		spin_lock_irqsave(ap->lock, flags);
 		WARN_ON(ap->pflags & ATA_PFLAG_PM_PENDING);
 	}
 
-	/* 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) {

drivers/ata/libata-core.c static void ata_port_request_pm(struct ata_port *ap, pm_message_t mesg, · line 5067

Quoted 7 lines

drivers/ata/libata-core.c +1 -2

@@ -5062,10 +5065,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);

Perhaps you should mention why this WARN_ON() is removed in the commit message.

Quoted 3 lines

I don't understand why you keep the WARN_ON() higher up in this function, but remove this WARN_ON(). They seem to have equal worth to me. Perhaps just take and release the lock around the WARN_ON() here as well?

Yes, they have the same worth == not super useful... I kept the one higher up as it is OK because we hold the lock, but removed the second one as checking pflags without the lock is just plain wrong. Thinking of it, the first WRN_ON() is also wrong I think because EH could be rescheduled right after wait_eh and before we take the lock. In that case, the warn on would be a flase positive. I will remove it as well.

Quoted 70 lines · Damien Le Moal

Quoted 65 lines · Niklas Cassel

Quoted 3 lines · Damien Le Moal

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 secheduled

s/secheduled/scheduled/

Quoted 47 lines

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.

Fixes 5ef41082912b ("ata: add ata port system PM callbacks")

Cc [email protected]

Signed-off-by Damien Le Moal <[email protected]>

Reviewed-by Hannes Reinecke <[email protected]>

Tested-by Chia-Lin Kao (AceLan) <[email protected]>

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

drivers/ata/libata-core.c +8 -5

index 74314311295f..c4898483d716 100644
@@ -5040,17 +5040,20 @@ 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);
+		spin_lock_irqsave(ap->lock, flags);
 		WARN_ON(ap->pflags & ATA_PFLAG_PM_PENDING);
 	}
 
-	/* 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) {

drivers/ata/libata-core.c static void ata_port_request_pm(struct ata_port *ap, pm_message_t mesg, · line 5067

Quoted 7 lines

drivers/ata/libata-core.c +1 -2

@@ -5062,10 +5065,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);

Perhaps you should mention why this WARN_ON() is removed in the commit message.

Quoted 3 lines

I don't understand why you keep the WARN_ON() higher up in this function, but remove this WARN_ON(). They seem to have equal worth to me. Perhaps just take and release the lock around the WARN_ON() here as well?

Quoted 6 lines

Yes, they have the same worth == not super useful... I kept the one higher up as it is OK because we hold the lock, but removed the second one as checking pflags without the lock is just plain wrong. Thinking of it, the first WRN_ON() is also wrong I think because EH could be rescheduled right after wait_eh and before we take the lock. In that case, the warn on would be a flase positive. I will remove it as well.

We are checking if ATA_PFLAG_PM_PENDING is set, if it is, we do ata_port_wait_eh(), which will wait until both ATA_PFLAG_EH_PENDING and ATA_PFLAG_EH_IN_PROGRESS is cleared.

Note that ATA_PFLAG_PM_PENDING and ATA_PFLAG_EH_PENDING have very similar names... I really think we should rename ATA_PFLAG_PM_PENDING to something like ATA_PFLAG_EH_PM_PENDING (the PM is performed by EH), in order to make it harder to mix them up.

Quoted 82 lines · Niklas Cassel

Quoted 70 lines · Damien Le Moal

Quoted 65 lines · Niklas Cassel

Quoted 3 lines · Damien Le Moal

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 secheduled

s/secheduled/scheduled/

Quoted 47 lines

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.

Fixes 5ef41082912b ("ata: add ata port system PM callbacks")

Cc [email protected]

Signed-off-by Damien Le Moal <[email protected]>

Reviewed-by Hannes Reinecke <[email protected]>

Tested-by Chia-Lin Kao (AceLan) <[email protected]>

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

drivers/ata/libata-core.c +8 -5

index 74314311295f..c4898483d716 100644
@@ -5040,17 +5040,20 @@ 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);
+		spin_lock_irqsave(ap->lock, flags);
 		WARN_ON(ap->pflags & ATA_PFLAG_PM_PENDING);
 	}
 
-	/* 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) {

drivers/ata/libata-core.c static void ata_port_request_pm(struct ata_port *ap, pm_message_t mesg, · line 5067

Quoted 7 lines

drivers/ata/libata-core.c +1 -2

@@ -5062,10 +5065,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);

Perhaps you should mention why this WARN_ON() is removed in the commit message.

Quoted 3 lines

I don't understand why you keep the WARN_ON() higher up in this function, but remove this WARN_ON(). They seem to have equal worth to me. Perhaps just take and release the lock around the WARN_ON() here as well?

Quoted 6 lines

Yes, they have the same worth == not super useful... I kept the one higher up as it is OK because we hold the lock, but removed the second one as checking pflags without the lock is just plain wrong. Thinking of it, the first WRN_ON() is also wrong I think because EH could be rescheduled right after wait_eh and before we take the lock. In that case, the warn on would be a flase positive. I will remove it as well.

We are checking if ATA_PFLAG_PM_PENDING is set, if it is, we do ata_port_wait_eh(), which will wait until both ATA_PFLAG_EH_PENDING and ATA_PFLAG_EH_IN_PROGRESS is cleared.

Quoted 4 lines

Note that ATA_PFLAG_PM_PENDING and ATA_PFLAG_EH_PENDING have very similar names... I really think we should rename ATA_PFLAG_PM_PENDING to something like ATA_PFLAG_EH_PM_PENDING (the PM is performed by EH), in order to make it harder to mix them up.

Perhaps ATA_PFLAG_POWER_STATE_PENDING is a better name?

That way we make it even harder to mix them up, since my previous suggestion ATA_PFLAG_EH_PM_PENDING, people might still miss the _PM_ part when reading quickly and could still confuse it with ATA_PFLAG_EH_PENDING.

Kind regards,
Niklas

Quoted 70 lines · Damien Le Moal

Quoted 65 lines · Niklas Cassel

Quoted 3 lines · Damien Le Moal

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 secheduled

s/secheduled/scheduled/

Quoted 47 lines

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.

Fixes 5ef41082912b ("ata: add ata port system PM callbacks")

Cc [email protected]

Signed-off-by Damien Le Moal <[email protected]>

Reviewed-by Hannes Reinecke <[email protected]>

Tested-by Chia-Lin Kao (AceLan) <[email protected]>

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

drivers/ata/libata-core.c +8 -5

index 74314311295f..c4898483d716 100644
@@ -5040,17 +5040,20 @@ 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);
+		spin_lock_irqsave(ap->lock, flags);
 		WARN_ON(ap->pflags & ATA_PFLAG_PM_PENDING);
 	}
 
-	/* 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) {

drivers/ata/libata-core.c static void ata_port_request_pm(struct ata_port *ap, pm_message_t mesg, · line 5067

Quoted 7 lines

drivers/ata/libata-core.c +1 -2

@@ -5062,10 +5065,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);

Perhaps you should mention why this WARN_ON() is removed in the commit message.

Quoted 3 lines

I don't understand why you keep the WARN_ON() higher up in this function, but remove this WARN_ON(). They seem to have equal worth to me. Perhaps just take and release the lock around the WARN_ON() here as well?

Quoted 6 lines

Yes, they have the same worth == not super useful... I kept the one higher up as it is OK because we hold the lock, but removed the second one as checking pflags without the lock is just plain wrong. Thinking of it, the first WRN_ON() is also wrong I think because EH could be rescheduled right after wait_eh and before we take the lock. In that case, the warn on would be a flase positive. I will remove it as well.

We are checking if ATA_PFLAG_PM_PENDING is set, if it is, we do ata_port_wait_eh(), which will wait until both ATA_PFLAG_EH_PENDING and ATA_PFLAG_EH_IN_PROGRESS is cleared.

Note that ATA_PFLAG_PM_PENDING and ATA_PFLAG_EH_PENDING have very similar names... I really think we should rename ATA_PFLAG_PM_PENDING to something like ATA_PFLAG_EH_PM_PENDING (the PM is performed by EH), in order to make it harder to mix them up.

Since the only place that sets ATA_PFLAG_PM_PENDING is ata_port_request_pm() and since PM core holds the device lock (device_lock()), I don't think that ATA_PFLAG_PM_PENDING can get set while inside ata_port_request_pm(). And since we wait for EH to complete, and since both ata_eh_handle_port_suspend() and ata_eh_handle_port_resume() are called unconditionally by EH, they will only return if ATA_PFLAG_PM_PENDING is not set, and since these functions both clear ATA_PFLAG_PM_PENDING unconditionally, I would agree with you, that these two WARN_ON() seem superfluous.

(Yes, EH could trigger again if we got an error IRQ before ata_port_request_pm() takes the lock the second time, but that can only set ATA_PFLAG_EH_PENDING, it can not set ATA_PFLAG_PM_PENDING.)

Kind regards,
Niklas

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.

Fixes 720ba12620ee ("[PATCH] libata-hp: update unload-unplug")

Cc [email protected]

Signed-off-by Damien Le Moal <[email protected]>

Reviewed-by Hannes Reinecke <[email protected]>

Tested-by Chia-Lin Kao (AceLan) <[email protected]>

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

drivers/ata/libata-core.c +20 -1

index c4898483d716..693cb3cd70cd 100644
@@ -5952,11 +5952,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);

Signature · 1 line

2.41.0

Quoted 73 lines · Damien Le Moal

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.

Fixes 720ba12620ee ("[PATCH] libata-hp: update unload-unplug")

Cc [email protected]

Signed-off-by Damien Le Moal <[email protected]>

Reviewed-by Hannes Reinecke <[email protected]>

Tested-by Chia-Lin Kao (AceLan) <[email protected]>

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

drivers/ata/libata-core.c +20 -1

index c4898483d716..693cb3cd70cd 100644
@@ -5952,11 +5952,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);

Signature · 1 line

2.41.0

Looks good:

Reviewed-by Niklas Cassel <[email protected]>

Could you perhaps also fix the WARN_ON(!(ap->pflags & ATA_PFLAG_UNLOADED)); in this very same function.

The pflag is checked without holding the lock.

Kind regards,
Niklas

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_dev_alloc(). This function is used to define the ->slave_alloc callback of the scsi host template of most drivers.

Fixes a19a93e4c6a9 ("scsi: core: pm: Rely on the device driver core for async power management")

Cc [email protected]

Signed-off-by Damien Le Moal <[email protected]>

Reviewed-by Hannes Reinecke <[email protected]>

---
 drivers/ata/ahci.h        |  1 +
 drivers/ata/libata-scsi.c | 49 +++++++++++++++++++++++++++++++++++----
 drivers/ata/libata.h      |  1 +
 drivers/ata/pata_macio.c  |  1 +
 drivers/ata/sata_mv.c     |  1 +
 drivers/ata/sata_nv.c     |  2 ++
 drivers/ata/sata_sil24.c  |  1 +
 include/linux/libata.h    |  3 +++
 8 files changed, 54 insertions(+), 5 deletions(-)

drivers/ata/ahci.h +1 -0

index 4bae95b06ae3..72085756f4ba 100644
@@ -398,6 +398,7 @@ extern const struct attribute_group *ahci_sdev_groups[];
 	.sdev_groups		= ahci_sdev_groups,			\
 	.change_queue_depth     = ata_scsi_change_queue_depth,		\
 	.tag_alloc_policy       = BLK_TAG_ALLOC_RR,             	\
+	.slave_alloc		= ata_scsi_slave_alloc,			\
 	.slave_configure        = ata_scsi_slave_config
 
 extern struct ata_port_operations ahci_ops;

drivers/ata/libata-scsi.c +44 -5

index d3f28b82c97b..eef76af1af90 100644
@@ -1089,6 +1089,46 @@ int ata_scsi_dev_config(struct scsi_device *sdev, struct ata_device *dev)
 	return 0;
 }
 
+int ata_scsi_dev_alloc(struct scsi_device *sdev, struct ata_port *ap)
+{
+	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;
+}
+
+/**
+ *	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)
+{
+	return ata_scsi_dev_alloc(sdev, ata_shost_to_port(sdev->host));
+}
+EXPORT_SYMBOL_GPL(ata_scsi_slave_alloc);
+
 /**
  *	ata_scsi_slave_config - Set SCSI device attributes
  *	@sdev: SCSI device to examine

@@ -1105,14 +1145,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 +1173,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) {

drivers/ata/libata.h +1 -0

index 6e7d352803bd..079981e7156a 100644
@@ -111,6 +111,7 @@ extern struct ata_device *ata_scsi_find_dev(struct ata_port *ap,
 extern int ata_scsi_add_hosts(struct ata_host *host,
 			      const struct scsi_host_template *sht);
 extern void ata_scsi_scan_host(struct ata_port *ap, int sync);
+extern int ata_scsi_dev_alloc(struct scsi_device *sdev, struct ata_port *ap);
 extern int ata_scsi_offline_dev(struct ata_device *dev);
 extern bool ata_scsi_sense_is_valid(u8 sk, u8 asc, u8 ascq);
 extern void ata_scsi_set_sense(struct ata_device *dev,

drivers/ata/pata_macio.c +1 -0

index 17f6ccee53c7..32968b4cf8e4 100644
@@ -918,6 +918,7 @@ static const struct scsi_host_template pata_macio_sht = {
 	 * use 64K minus 256
 	 */
 	.max_segment_size	= MAX_DBDMA_SEG,
+	.slave_alloc		= ata_scsi_slave_alloc,
 	.slave_configure	= pata_macio_slave_config,
 	.sdev_groups		= ata_common_sdev_groups,
 	.can_queue		= ATA_DEF_QUEUE,

drivers/ata/sata_mv.c +0 -0

index d105db5c7d81..353ac7b2f14a 100644

drivers/ata/sata_mv.c +0 -0

 	.slave_configure	= ata_scsi_slave_config
 };
 

drivers/ata/sata_nv.c +2 -0

index 0a0cee755bde..5428dc2ec5e3 100644
@@ -380,6 +380,7 @@ static const struct scsi_host_template nv_adma_sht = {
 	.can_queue		= NV_ADMA_MAX_CPBS,
 	.sg_tablesize		= NV_ADMA_SGTBL_TOTAL_LEN,
 	.dma_boundary		= NV_ADMA_DMA_BOUNDARY,
+	.slave_alloc		= ata_scsi_slave_alloc,
 	.slave_configure	= nv_adma_slave_config,
 	.sdev_groups		= ata_ncq_sdev_groups,
 	.change_queue_depth     = ata_scsi_change_queue_depth,

@@ -391,6 +392,7 @@ static const struct scsi_host_template nv_swncq_sht = {
 	.can_queue		= ATA_MAX_QUEUE - 1,
 	.sg_tablesize		= LIBATA_MAX_PRD,
 	.dma_boundary		= ATA_DMA_BOUNDARY,
+	.slave_alloc		= ata_scsi_slave_alloc,
 	.slave_configure	= nv_swncq_slave_config,
 	.sdev_groups		= ata_ncq_sdev_groups,
 	.change_queue_depth     = ata_scsi_change_queue_depth,

drivers/ata/sata_sil24.c +1 -0

index 142e70bfc498..e0b1b3625031 100644
@@ -381,6 +381,7 @@ static const struct scsi_host_template sil24_sht = {
 	.tag_alloc_policy	= BLK_TAG_ALLOC_FIFO,
 	.sdev_groups		= ata_ncq_sdev_groups,
 	.change_queue_depth	= ata_scsi_change_queue_depth,
+	.slave_alloc		= ata_scsi_slave_alloc,
 	.slave_configure	= ata_scsi_slave_config
 };
 

include/linux/libata.h +3 -0

index 52d58b13e5ee..c8cfea386c16 100644
@@ -1144,6 +1144,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,

@@ -1401,12 +1402,14 @@ extern const struct attribute_group *ata_common_sdev_groups[];
 	__ATA_BASE_SHT(drv_name),				\
 	.can_queue		= ATA_DEF_QUEUE,		\
 	.tag_alloc_policy	= BLK_TAG_ALLOC_RR,		\
+	.slave_alloc		= ata_scsi_slave_alloc,		\
 	.slave_configure	= ata_scsi_slave_config
 
 #define ATA_SUBBASE_SHT_QD(drv_name, drv_qd)			\
 	__ATA_BASE_SHT(drv_name),				\
 	.can_queue		= drv_qd,			\
 	.tag_alloc_policy	= BLK_TAG_ALLOC_RR,		\
+	.slave_alloc		= ata_scsi_slave_alloc,		\
 	.slave_configure	= ata_scsi_slave_config
 
 #define ATA_BASE_SHT(drv_name)					\

Signature · 1 line

2.41.0

Quoted 17 lines

drivers/ata/pata_macio.c +1 -0

index 17f6ccee53c7..32968b4cf8e4 100644
@@ -918,6 +918,7 @@ static const struct scsi_host_template pata_macio_sht = {
 	 * use 64K minus 256
 	 */
 	.max_segment_size	= MAX_DBDMA_SEG,
+	.slave_alloc		= ata_scsi_slave_alloc,
 	.slave_configure	= pata_macio_slave_config,
 	.sdev_groups		= ata_common_sdev_groups,
 	.can_queue		= ATA_DEF_QUEUE,

drivers/ata/sata_mv.c +0 -0

index d105db5c7d81..353ac7b2f14a 100644

drivers/ata/sata_mv.c static const struct scsi_host_template mv6_sht = { · line 676

Quoted 4 lines

drivers/ata/sata_mv.c +1 -0

@@ -673,6 +673,7 @@ static const struct scsi_host_template mv6_sht = {
 	.sdev_groups		= ata_ncq_sdev_groups,
 	.change_queue_depth	= ata_scsi_change_queue_depth,
 	.tag_alloc_policy	= BLK_TAG_ALLOC_RR,
+	.slave_alloc		= ata_scsi_slave_alloc,

It seems wrong to add .slave_alloc to all different ata_port_operations structs. The .slave_configure is added on all different ata_port_operations structs, because the callback can be different for different drivers.

However, adding the device link is done in .ata_scsi_slave_alloc, removing the device link is done in .ata_scsi_slave_destroy.

Thus, I suggest that we only add the
.slave_alloc = ata_scsi_slave_alloc,

to __ATA_BASE_SHT() in libata.h, which is currently the only place which has .slave_destroy defined.

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 resum operation requiring re-validating the device link and the device itseld. 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.

Fixes 0a8589055936 ("ata,scsi: do not issue START STOP UNIT on resume")

Cc [email protected]

Signed-off-by Damien Le Moal <[email protected]>

---
  drivers/ata/libata-scsi.c  |  3 +-
  drivers/firewire/sbp2.c    |  9 +++--
  drivers/scsi/sd.c          | 78 ++++++++++++++++++++++++++++----------
  include/scsi/scsi_device.h |  3 +-
  4 files changed, 69 insertions(+), 24 deletions(-)

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.

Fixes 0a8589055936 ("ata,scsi: do not issue START STOP UNIT on resume")

Cc [email protected]

Signed-off-by Damien Le Moal <[email protected]>

---
  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(-)

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.

Fixes 6aa0365a3c85 ("ata: libata-scsi: Avoid deadlock on rescan after device resume")

Cc [email protected]

Signed-off-by Damien Le Moal <[email protected]>

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

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.

Fixes 6aa0365a3c85 ("ata: libata-scsi: Avoid deadlock on rescan after device resume")

Cc [email protected]

Signed-off-by Damien Le Moal <[email protected]>

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

drivers/scsi/scsi_scan.c +17 -1

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);
 

include/scsi/scsi_host.h +1 -1

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);

Signature · 1 line

2.41.0

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.

Fixes 6aa0365a3c85 ("ata: libata-scsi: Avoid deadlock on rescan after device resume")

Cc [email protected]

Signed-off-by Damien Le Moal <[email protected]>

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

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.

Fixes 6aa0365a3c85 ("ata: libata-scsi: Avoid deadlock on rescan after device resume")

Cc [email protected]

Signed-off-by Damien Le Moal <[email protected]>

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

drivers/ata/libata-core.c +16 -0

index 0cf0caf77907..0479493e54bd 100644
@@ -5172,11 +5172,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);
 }
 

drivers/ata/libata-scsi.c +15 -18

index ac2d332b4963..6297f8c16a13 100644
@@ -4760,7 +4760,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);

@@ -4769,37 +4769,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));
 }

Signature · 1 line

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 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 not calling sd_shutdown() in sd_remove() if the device is not running.

Cc [email protected]

Signed-off-by Damien Le Moal <[email protected]>

---
  drivers/scsi/sd.c | 3 ++-
  1 file changed, 2 insertions(+), 1 deletion(-)

drivers/scsi/sd.c +0 -0

index 1d106c8ad5af..d86306d42445 100644

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 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 not calling sd_shutdown() in sd_remove() if the device is not running.

Cc [email protected]

Signed-off-by Damien Le Moal <[email protected]>

---
  drivers/scsi/sd.c | 3 ++-
  1 file changed, 2 insertions(+), 1 deletion(-)

drivers/scsi/sd.c +0 -0

index 1d106c8ad5af..d86306d42445 100644