Skip to contentSkip to conversation

libata: make sure port is thawed when skipping resets

PATCH In review Tejun Heo 2 participants 2 messages Mar 4, 2009 libata

lore
Share & tools
lore

Tejun Heo

author
#0 lore raw

PATCH 1/1 libata: make sure port is thawed when skipping resets

When SCR access is available and the link is offline, softreset is skipped as it only wastes time and some controllers don't respond very well. However, the skip path forgot to thaw the port, which not only blocks further event notification from the port but also causes repeated EH invocations on the same event on drivers which rely on ->thaw() to clear events if the IRQ is shared with another device or port.

This problem has always been there but is uncovered by recent sata_nv nf2/3 change which dropped hardreset support while maintaining SCR access. nf2/3 doesn't clear hotplug event mask from the interrupt handler but relies on ->thaw() to clear them. When the hardreset was there, the reset action was never skipped and the port was always thawed but, with the hardreset gone, ->prereset() determines that there's no need for softreset and both ->softreset() and ->thaw() are skipped. This leads to stuck hotplug event in the IRQ status register triggering hotplug event whenever IRQ is delieverd on the same IRQ. As the controller shares the same IRQ for both ports, this happens on every IO if one port is occpupied and the other isn't.

This patch fixes the problem by making sure that the port is thawed on reset-skip path.

bko#11615 reports this problem.

---
Heh.... this is one convoluted failure case. The only two drivers which have SCR access but doesn't use hardrest are sata_uli and sata_nv at this point. sata_uli doesn't have PHY event or any other sticky events, so sata_nv became the one to get hit by this. Hopefully, this will put an end to sata_nv reset fiasco.

Thanks.

 drivers/ata/libata-eh.c |    5 ++++-
 1 file changed, 4 insertions(+), 1 deletion(-)
drivers/ata/libata-eh.c +4 -1
index ce2ef04..02d47b2 100644
@@ -2423,11 +2423,14 @@ int ata_eh_reset(struct ata_link *link, int classify,
 		}
 
 		/* prereset() might have cleared ATA_EH_RESET.  If so,
-		 * bang classes and return.
+		 * bang classes, thaw and return.
 		 */
 		if (reset && !(ehc->i.action & ATA_EH_RESET)) {
 			ata_for_each_dev(dev, link, ALL)
 				classes[dev->devno] = ATA_DEV_NONE;
+			if ((ap->pflags & ATA_PFLAG_FROZEN) &&
+			    ata_is_host_link(link))
+				ata_eh_thaw_port(ap);
 			rc = 0;
 			goto out;
 		}
#1 lore raw

Hi

Quoted 20 lines · Tejun Heo

When SCR access is available and the link is offline, softreset is skipped as it only wastes time and some controllers don't respond very well. However, the skip path forgot to thaw the port, which not only blocks further event notification from the port but also causes repeated EH invocations on the same event on drivers which rely on ->thaw() to clear events if the IRQ is shared with another device or port.

This problem has always been there but is uncovered by recent sata_nv nf2/3 change which dropped hardreset support while maintaining SCR access. nf2/3 doesn't clear hotplug event mask from the interrupt handler but relies on ->thaw() to clear them. When the hardreset was there, the reset action was never skipped and the port was always thawed but, with the hardreset gone, ->prereset() determines that there's no need for softreset and both ->softreset() and ->thaw() are skipped. This leads to stuck hotplug event in the IRQ status register triggering hotplug event whenever IRQ is delieverd on the same IRQ. As the controller shares the same IRQ for both ports, this happens on every IO if one port is occpupied and the other isn't.

Quoted 4 lines

This patch fixes the problem by making sure that the port is thawed on reset-skip path.

bko#11615 reports this problem.

Sorry that I didn't notice the bugzilla entry earlier, this patch indeed fixes the problem for me (tested on top of 2.6.29-rc7 and 2.6.28.7).

Thank you a lot
Stefan Lippers-Hollmann

Quoted 38 lines

---
Heh.... this is one convoluted failure case. The only two drivers which have SCR access but doesn't use hardrest are sata_uli and sata_nv at this point. sata_uli doesn't have PHY event or any other sticky events, so sata_nv became the one to get hit by this. Hopefully, this will put an end to sata_nv reset fiasco.

Thanks.

 drivers/ata/libata-eh.c |    5 ++++-
 1 file changed, 4 insertions(+), 1 deletion(-)
drivers/ata/libata-eh.c +4 -1
index ce2ef04..02d47b2 100644
@@ -2423,11 +2423,14 @@ int ata_eh_reset(struct ata_link *link, int classify,
 		}
 
 		/* prereset() might have cleared ATA_EH_RESET.  If so,
-		 * bang classes and return.
+		 * bang classes, thaw and return.
 		 */
 		if (reset && !(ehc->i.action & ATA_EH_RESET)) {
 			ata_for_each_dev(dev, link, ALL)
 				classes[dev->devno] = ATA_DEV_NONE;
+			if ((ap->pflags & ATA_PFLAG_FROZEN) &&
+			    ata_is_host_link(link))
+				ata_eh_thaw_port(ap);
 			rc = 0;
 			goto out;
 		}