libata: Integrate ACPI-based PATA/SATA hotplug

Modern laptops with hotswap bays still tend to utilise a PATA interface on a SATA bridge, generally with the host controller in some legacy emulation mode rather than AHCI. This means that the existing hotplug code in libata is unable to work. The ACPI specification states that these devices can send notifications when hotswapped, which avoids the need to obtain notification from the controller. This patch uses the existing libata-acpi code and simply registers a notification in order to trigger a rescan whenever the firmware signals an event.

Signed-off-by Matthew Garrett <[email protected]>

---

Testing on an HP with the hotplug device as the slave on a PATA channel flagged a bug - the notification needs to be tied to the channel handle as well as the device ones. With this version, I can happily hotswap the HP device. It ends up sitting for a few seconds while failing to revalidate, but then recovers with everything working fine.

the code looks correct. I have one main reservation.

how can we be sure that this is active only where other hand-programmed hotplug code is absent?

Quoted 4 lines · Jeff Garzik

the code looks correct. I have one main reservation.

how can we be sure that this is active only where other hand-programmed hotplug code is absent?

Yes, that's difficult. As Tejun pointed out, there's missing locking here at the moment - if I add that, am I right in thinking that the worst case scenario is that the hotplugging path will be called twice?

One option would be to limit this to PATA-style controllers - I'm not aware of any mobile hardware shipping with natively hotplug-capable controllers, so that would probably do for the moment. If (when) they move to AHCI, with luck the native hotplugging will work and we won't have to worry about this so much.

The alternative would be to add a flag to the ap structure indicating whether the hotplugging is handled by the firmware or not. If we find a reference to a controller or port in the firmware tables, it probably indicates that the hardware has opinions about how this should be handled. We might be safer leaving it to the firmware in those cases, and using that flag to skip the controller-specific hotplug code.

Quoted 6 lines · Matthew Garrett

The alternative would be to add a flag to the ap structure indicating whether the hotplugging is handled by the firmware or not. If we find a reference to a controller or port in the firmware tables, it probably indicates that the hardware has opinions about how this should be handled. We might be safer leaving it to the firmware in those cases, and using that flag to skip the controller-specific hotplug code.

I was thinking the other way around. I'd rather depend on hardware provided events than firmware provided ones. How about flagging drivers which can do native hoplugging and using ACPI hotplugging only if the driver can't do it natively?

Quoted 11 lines · Tejun Heo

Quoted 6 lines · Matthew Garrett

The alternative would be to add a flag to the ap structure indicating whether the hotplugging is handled by the firmware or not. If we find a reference to a controller or port in the firmware tables, it probably indicates that the hardware has opinions about how this should be handled. We might be safer leaving it to the firmware in those cases, and using that flag to skip the controller-specific hotplug code.

I was thinking the other way around. I'd rather depend on hardware provided events than firmware provided ones. How about flagging drivers which can do native hoplugging and using ACPI hotplugging only if the driver can't do it natively?

If the manufacturers have added firmware-level hotswap interrupts, then there's all sorts of insane ways they might have wired the bay up. I don't really trust them to have managed to do so without breaking native hotplug :) It doesn't really matter at the moment, though, since I haven't actually seen any examples of hardware using anything that can manage native hotplug. If anyone out there does have one, it would be nice to get some feedback about what it does.

Maybe just letting both events in is the best idea. It's not like two duplicate events are gonna break anything and I don't think many vendors are gonna implement separate mechanism when the default SATA phy based one works.

Quoted 4 lines · Tejun Heo

Maybe just letting both events in is the best idea. It's not like two duplicate events are gonna break anything and I don't think many vendors are gonna implement separate mechanism when the default SATA phy based one works.

Works for me. Unrelatedly, is it expected that the EH take some time attempting to revalidate the port before finally deciding that the drive has gone? I seem to lose 15 seconds or so to that, which is more irritating on PATA systems where it tends to block the channel.

(Quite why HP put their hotswap optical drives on the same PATA channel as the internal drive is somewhat beyond me, but...)

Yeah, that's the intended behavior. SATA PHY link can break from time to time (have ever seen a SATA storage box going through ECC testing? PHY goes offline as soon as you begin to hit it with some EM pulses) and you don't really wanna lose your root partition over power fluctuation.

Quoted 4 lines · Tejun Heo

Yeah, that's the intended behavior. SATA PHY link can break from time to time (have ever seen a SATA storage box going through ECC testing? PHY goes offline as soon as you begin to hit it with some EM pulses) and you don't really wanna lose your root partition over power fluctuation.

In this case we explicitly know that it's in response to a hotplug event (well, either that or the firmware is on impressive crack). Would it be possible to communicate that in order to avoid the revalidation?

Modern laptops with hotswap bays still tend to utilise a PATA interface on a SATA bridge, generally with the host controller in some legacy emulation mode rather than AHCI. This means that the existing hotplug code in libata is unable to work. The ACPI specification states that these devices can send notifications when hotswapped, which avoids the need to obtain notification from the controller. This patch uses the existing libata-acpi code and simply registers a notification in order to trigger a rescan whenever the firmware signals an event.

Signed-off-by Matthew Garrett <[email protected]>

---

This makes two changes to the previous patch:

1) It implements the locking suggested by Tejun
2) It sends a uevent on the device kobject. I've implemented this because grabbing the notification handler means that the bay driver can no longer do it, so it's necessary to generate compatible events. If the event type is 3, it indicates that the user has merely requested an eject - the drive hasn't gone at this point. Sending the notification allows userspace to attempt to unmount the filesystems before sending a command to initiate the eject.

I'm not especially happy about the chain used to get the scsi device kobject. Is there a cleaner way to do that? Other than that, I've now tested this on multiple systems (a 965-based Thinkpad, a 915-era Dell and even an HP with no SATA whatsoever) without any obvious breakage.

drivers/ata/libata-acpi.c +53 -1

index c059f78..68bb7fa 100644
@@ -14,6 +14,7 @@
 #include <linux/acpi.h>
 #include <linux/libata.h>
 #include <linux/pci.h>
+#include <scsi/scsi_device.h>
 #include "libata.h"
 
 #include <acpi/acpi_bus.h>

@@ -66,6 +67,41 @@ static void ata_acpi_associate_ide_port(struct ata_port *ap)
 	}
 }
 
+static void ata_acpi_notify(acpi_handle handle, u32 event, void *data)
+{
+	struct ata_port *ap = data;
+	struct ata_eh_info *ehi = &ap->eh_info;
+	char event_string[12];
+	char *envp[] = { event_string, NULL };
+	struct kobject *kobj = NULL;
+	int i;
+
+	if (ap->acpi_handle && handle == ap->acpi_handle)
+	        kobj = &ap->dev->kobj;
+	else {
+		for (i = 0; i < ata_port_max_devices(ap); i++) {
+			struct ata_device *dev = &ap->device[i];
+			if (dev->acpi_handle && handle == dev->acpi_handle) 
+			        kobj = &dev->sdev->sdev_gendev.kobj;
+		}
+	}
+
+	if (event == 0 || event == 1) {
+	       unsigned long flags;
+	       spin_lock_irqsave(ap->lock, flags);
+	       ata_ehi_clear_desc(ehi);
+	       ata_ehi_push_desc(ehi, "ACPI event");
+	       ata_ehi_hotplugged(ehi);
+	       ata_port_freeze(ap);
+	       spin_unlock_irqrestore(ap->lock, flags);
+	}
+
+	if (kobj) {
+	        sprintf(event_string, "BAY_EVENT=%d\n", event);
+		kobject_uevent_env(kobj, KOBJ_CHANGE, envp);
+	}
+}
+
 /**
  * ata_acpi_associate - associate ATA host with ACPI objects
  * @host: target ATA host

@@ -81,7 +117,7 @@ static void ata_acpi_associate_ide_port(struct ata_port *ap)
  */
 void ata_acpi_associate(struct ata_host *host)
 {
-	int i;
+	int i, j;
 
 	if (!is_pci_dev(host->dev) || libata_noacpi)
 		return;

@@ -97,6 +133,22 @@ void ata_acpi_associate(struct ata_host *host)
 			ata_acpi_associate_sata_port(ap);
 		else
 			ata_acpi_associate_ide_port(ap);
+
+		if (ap->acpi_handle)
+			acpi_install_notify_handler (ap->acpi_handle,
+						     ACPI_SYSTEM_NOTIFY,
+						     ata_acpi_notify,
+						     ap);
+
+		for (j = 0; j < ata_port_max_devices(ap); j++) {
+			struct ata_device *dev = &ap->device[j];
+
+			if (dev->acpi_handle)
+				acpi_install_notify_handler (dev->acpi_handle,
+							     ACPI_SYSTEM_NOTIFY,
+							     ata_acpi_notify,
+							     ap);
+		}
 	}
 }
 

Signature · 1 line

Matthew Garrett | [email protected]

1. How does it handle things when you have a bay that is located behind a dock and the dock ejects? In the acpi bay driver, I use the mechanism that the dock driver exports to get undock notifications so that the bay can eject as well.

2. What if someone wants to use their bay to charge their battery? While I never bothered to implement this in acpi/bay.c, nothing ever prevented anyone from adding that support to the driver, where now it is prevented since this driver and another cannot coexist.

The spec seems to imply that even if the drive hotswap bay and the battery bay are physically the same, they're logically distinct. 10.2.1

I /suspect/ that _STA on the bay device won't return true if there's a battery in there, and so we aren't expected to call the _EJ0 method if

Modern laptops with hotswap bays still tend to utilise a PATA interface on a SATA bridge, generally with the host controller in some legacy emulation mode rather than AHCI. This means that the existing hotplug code in libata is unable to work. The ACPI specification states that these devices can send notifications when hotswapped, which avoids the need to obtain notification from the controller. This patch uses the existing libata-acpi code and simply registers a notification in order to trigger a rescan whenever the firmware signals an event.

Signed-off-by Matthew Garrett <[email protected]>

---

This makes two changes to the previous patch:

1) It implements the locking suggested by Tejun
2) It sends a uevent on the device kobject. I've implemented this because grabbing the notification handler means that the bay driver can no longer do it, so it's necessary to generate compatible events. If the event type is 3, it indicates that the user has merely requested an eject - the drive hasn't gone at this point. Sending the notification allows userspace to attempt to unmount the filesystems before sending a command to initiate the eject.

I'm not especially happy about the chain used to get the scsi device kobject. Is there a cleaner way to do that? Other than that, I've now tested this on multiple systems (a 965-based Thinkpad, a 915-era Dell and even an HP with no SATA whatsoever) without any obvious breakage.

drivers/ata/libata-acpi.c +42 -1

index c059f78..68bb7fa 100644
@@ -14,6 +14,7 @@
 #include <linux/acpi.h>
 #include <linux/libata.h>
 #include <linux/pci.h>
+#include <scsi/scsi_device.h>
 #include "libata.h"
 
 #include <acpi/acpi_bus.h>

@@ -66,6 +67,41 @@ static void ata_acpi_associate_ide_port(struct ata_port *ap)
 	}
 }
 
+static void ata_acpi_notify(acpi_handle handle, u32 event, void *data)
+{
+	struct ata_port *ap = data;
+	struct ata_eh_info *ehi = &ap->eh_info;
+	char event_string[12];
+	char *envp[] = { event_string, NULL };
+	struct kobject *kobj = NULL;
+	int i;
+
+	if (ap->acpi_handle && handle == ap->acpi_handle)
+	        kobj = &ap->dev->kobj;
+	else {
+		for (i = 0; i < ata_port_max_devices(ap); i++) {
+			struct ata_device *dev = &ap->device[i];
+			if (dev->acpi_handle && handle == dev->acpi_handle) 
+			        kobj = &dev->sdev->sdev_gendev.kobj;
+		}
+	}
+
+	if (event == 0 || event == 1) {
+	       unsigned long flags;
+	       spin_lock_irqsave(ap->lock, flags);
+	       ata_ehi_clear_desc(ehi);
+	       ata_ehi_push_desc(ehi, "ACPI event");
+	       ata_ehi_hotplugged(ehi);
+	       ata_port_freeze(ap);
+	       spin_unlock_irqrestore(ap->lock, flags);
+	}
+
+	if (kobj) {
+	        sprintf(event_string, "BAY_EVENT=%d\n", event);
+		kobject_uevent_env(kobj, KOBJ_CHANGE, envp);
+	}
+}
+
 /**
  * ata_acpi_associate - associate ATA host with ACPI objects
  * @host: target ATA host

@@ -81,7 +117,7 @@ static void ata_acpi_associate_ide_port(struct ata_port *ap)
  */
 void ata_acpi_associate(struct ata_host *host)
 {
-	int i;
+	int i, j;
 
 	if (!is_pci_dev(host->dev) || libata_noacpi)
 		return;

@@ -97,6 +133,22 @@ void ata_acpi_associate(struct ata_host *host)
 			ata_acpi_associate_sata_port(ap);
 		else
 			ata_acpi_associate_ide_port(ap);
+
+		if (ap->acpi_handle)
+			acpi_install_notify_handler (ap->acpi_handle,
+						     ACPI_SYSTEM_NOTIFY,
+						     ata_acpi_notify,

1) Check dev->sdev for NULL

2) remove the unnecessary ata_device loop. If you know the ata_device pointer, you should not throw it away and then do a search to find it again.

You need two functions, ata_acpi_ap_notify() and ata_acpi_dev_notify().
Pass 'ap' to the former, and 'dev' to the latter.

Both functions should marshal their arguments, then call a common function (presumably what 95% of current ata_acpi_notify does).

3) Won't this result in a single hotplug event calling ata_ehi_hotplugged() multiple times -- once for the port, and once for each device attached to the port?

Modern laptops with hotswap bays still tend to utilise a PATA interface on a SATA bridge, generally with the host controller in some legacy emulation mode rather than AHCI. This means that the existing hotplug code in libata is unable to work. The ACPI specification states that these devices can send notifications when hotswapped, which avoids the need to obtain notification from the controller. This patch uses the existing libata-acpi code and simply registers a notification in order to trigger a rescan whenever the firmware signals an event.

Signed-off-by Matthew Garrett <[email protected]>

---

This incorporates Jeff's feedback. sdev is checked for NULL, and different notifications are registered for ap-level and dev-level handlers. The core code is split out into a helper function called by both of these. The other change is the removal of the extraneous newline from the end of the notification event, to match the upstream change in the bay driver.

Quoted 6 lines · Matthew Garrett

Modern laptops with hotswap bays still tend to utilise a PATA interface on a SATA bridge, generally with the host controller in some legacy emulation mode rather than AHCI. This means that the existing hotplug code in libata is unable to work. The ACPI specification states that these devices can send notifications when hotswapped, which avoids the need to obtain notification from the controller. This patch uses the existing libata-acpi code and simply registers a notification in order to trigger a rescan whenever the firmware signals an event.

Signed-off-by Matthew Garrett <[email protected]>

---

This incorporates Jeff's feedback. sdev is checked for NULL, and different notifications are registered for ap-level and dev-level handlers. The core code is split out into a helper function called by both of these. The other change is the removal of the extraneous newline from the end of the notification event, to match the upstream change in the bay driver.

applied

Come on, dude! This doesn't even build:

Fix libata-acpi.c build failure.

Signed-off-by Matthew Garrett <[email protected]>

---

Sorry, I'd diffed the original against libata-dev master rather than ALL. This tidies up the changes.

Modern laptops with hotswap bays still tend to utilise a PATA interface on a SATA bridge, generally with the host controller in some legacy emulation mode rather than AHCI. This means that the existing hotplug code in libata is unable to work. The ACPI specification states that these devices can send notifications when hotswapped, which avoids the need to obtain notification from the controller. This patch uses the existing libata-acpi code and simply registers a notification in order to trigger a rescan whenever the firmware signals an event.

Signed-off-by Matthew Garrett <[email protected]>

---

Diffed against #upstream. Fairly sure I've got it right this time...