[PATCH] Fixups to ATA ACPI hotplug

[PATCH] Fixups to ATA ACPI hotplug

[Matthew Garrett]

The libata-acpi.c code currently accepts hotplug messages from both the 
port and the device. This does not match the behaviour of the bay 
driver, and may result in confusion when two hotplug requests are 
received for the same device. This patch limits the hotplug notification 
to removable ACPI devices, which in turn allows it to use the _STA 
method to determine whether the device has been removed or inserted. 
On removal, devices are marked as detached. On insertion, a hotplug scan 
is started. This should avoid lockups caused by the ata layer attempting 
to scan devices which have been removed. The uevent sending is moved 
outside the spinlock in order to avoid a warning generated by it firing 
when interrupts are disabled.

Signed-off-by: Matthew Garrett <mjg@redhat.com>

---

Holger, I'm pretty sure that this deals with the docking station removal 
case, but don't have the hardware to test. If EJECT_REQUEST is genuinely 
the only notification we receive (ie, no BUS_CHECK or DEVICE_CHECK) then 
we'll need to add a separate callback for docking.

diff --git a/drivers/ata/libata-acpi.c b/drivers/ata/libata-acpi.c
index 70b77e0..865a552 100644
--- a/drivers/ata/libata-acpi.c
+++ b/drivers/ata/libata-acpi.c
@@ -118,8 +118,8 @@ static void ata_acpi_associate_ide_port(struct ata_port *ap)
 		ap->pflags |= ATA_PFLAG_INIT_GTM_VALID;
 }
 
-static void ata_acpi_handle_hotplug(struct ata_port *ap, struct ata_device *dev,
-				    u32 event)
+static void ata_acpi_handle_hotplug(struct ata_port *ap, struct ata_device 
+				    *dev, u32 event)
 {
 	char event_string[12];
 	char *envp[] = { event_string, NULL };
@@ -127,39 +127,67 @@ static void ata_acpi_handle_hotplug(struct ata_port *ap, struct ata_device *dev,
 	struct kobject *kobj = NULL;
 	int wait = 0;
 	unsigned long flags;
-
+	acpi_handle handle, tmphandle;
+	unsigned long sta;
+	acpi_status status;
+	
 	if (!ap)
 		ap = dev->link->ap;
 	ehi = &ap->link.eh_info;
 
 	spin_lock_irqsave(ap->lock, flags);
 
+	if (dev)
+		handle = dev->acpi_handle;
+	else
+		handle = ap->acpi_handle;
+
+	status = acpi_get_handle(handle, "_EJ0", &tmphandle);
+	if (ACPI_FAILURE(status)) {
+		/* This device is not ejectable */
+		spin_unlock_irqrestore(ap->lock, flags);
+		return;
+	}
+
+	status = acpi_evaluate_integer(handle, "_STA", NULL, &sta);
+	if (ACPI_FAILURE(status)) {
+		printk ("Unable to determine bay status\n");
+		spin_unlock_irqrestore(ap->lock, flags);
+		return;
+	}
+
 	switch (event) {
 	case ACPI_NOTIFY_BUS_CHECK:
 	case ACPI_NOTIFY_DEVICE_CHECK:
 		ata_ehi_push_desc(ehi, "ACPI event");
-		ata_ehi_hotplugged(ehi);
-		ata_port_freeze(ap);
-		break;
-
-	case ACPI_NOTIFY_EJECT_REQUEST:
-		ata_ehi_push_desc(ehi, "ACPI event");
-		if (dev)
-			dev->flags |= ATA_DFLAG_DETACH;
-		else {
-			struct ata_link *tlink;
-			struct ata_device *tdev;
-
-			ata_port_for_each_link(tlink, ap)
-				ata_link_for_each_dev(tdev, tlink)
-					tdev->flags |= ATA_DFLAG_DETACH;
+		if (!sta) {
+                /* Device has been unplugged */
+			if (dev)
+				dev->flags |= ATA_DFLAG_DETACH;
+			else {
+				struct ata_link *tlink;
+				struct ata_device *tdev;
+				
+				ata_port_for_each_link(tlink, ap) {
+					ata_link_for_each_dev(tdev, tlink) {
+						tdev->flags |= 
+							ATA_DFLAG_DETACH;
+					}
+				}
+			}
+			ata_port_schedule_eh(ap);
+			wait = 1;
+		} else {
+			ata_ehi_hotplugged(ehi);
+			ata_port_freeze(ap);
 		}
-
-		ata_port_schedule_eh(ap);
-		wait = 1;
-		break;
 	}
 
+	spin_unlock_irqrestore(ap->lock, flags);
+
+	if (wait)
+		ata_port_wait_eh(ap);
+
 	if (dev) {
 		if (dev->sdev)
 			kobj = &dev->sdev->sdev_gendev.kobj;
@@ -170,11 +198,6 @@ static void ata_acpi_handle_hotplug(struct ata_port *ap, struct ata_device *dev,
 		sprintf(event_string, "BAY_EVENT=%d", event);
 		kobject_uevent_env(kobj, KOBJ_CHANGE, envp);
 	}
-
-	spin_unlock_irqrestore(ap->lock, flags);
-
-	if (wait)
-		ata_port_wait_eh(ap);
 }
 
 static void ata_acpi_dev_notify(acpi_handle handle, u32 event, void *data)

-- 
Matthew Garrett | mjg59@srcf.ucam.org

Re: [PATCH] Fixups to ATA ACPI hotplug

[Holger Macht]

On Mon 19. May - 17:29:34, Matthew Garrett wrote:
> The libata-acpi.c code currently accepts hotplug messages from both the 
> port and the device. This does not match the behaviour of the bay 
> driver, and may result in confusion when two hotplug requests are 
> received for the same device. This patch limits the hotplug notification 
> to removable ACPI devices, which in turn allows it to use the _STA 
> method to determine whether the device has been removed or inserted. 
> On removal, devices are marked as detached. On insertion, a hotplug scan 
> is started. This should avoid lockups caused by the ata layer attempting 
> to scan devices which have been removed. The uevent sending is moved 
> outside the spinlock in order to avoid a warning generated by it firing 
> when interrupts are disabled.

Ok, it seems we're currently doing the same work two times. I've also got
a patch for this. Already tested, so please have a look if this would also
match your requirements:

Handle bay devices in dock stations

* Differentiate between bay devices in dock stations and others:

 - When an ACPI_NOTIFY_EJECT_REQUEST appears, just signal uevent to
   userspace (that is when the optional eject button on a bay device is
   pressed/pulled) giving the possibility to unmount file systems and to
   clean up. Also, only send uevent in case we get an
   ACPI_NOTIFY_EJECT_REQUEST without doing anything else. In other cases,
   you'll get an add/remove event because libata attaches/detaches the
   device.

 - In case of a dock event, which in turn signals an
   ACPI_NOTIFY_EJECT_REQUEST, immediately detach the device, because it
   may already have been gone

* In case of an ACPI_NOTIFY_DEVICE/BUS_CHECK, evaluate _STA to check if
  the device has been plugged or unplugged. If plugged, hotplug it, if
  unplugged, just signal event to userspace
  (initial patch by Matthew Garrett <mjg59@srcf.ucam.org>)

Signed-off-by: Holger Macht <hmacht@suse.de>
---

diff --git a/drivers/ata/libata-acpi.c b/drivers/ata/libata-acpi.c
index 70b77e0..62d94e4 100644
--- a/drivers/ata/libata-acpi.c
+++ b/drivers/ata/libata-acpi.c
@@ -118,77 +118,118 @@ static void ata_acpi_associate_ide_port(struct ata_port *ap)
 		ap->pflags |= ATA_PFLAG_INIT_GTM_VALID;
 }
 
+static void ata_acpi_detach_device(struct ata_port *ap, struct ata_device *dev)
+{
+	if (dev)
+		dev->flags |= ATA_DFLAG_DETACH;
+	else {
+		struct ata_link *tlink;
+		struct ata_device *tdev;
+
+		ata_port_for_each_link(tlink, ap)
+			ata_link_for_each_dev(tdev, tlink)
+			tdev->flags |= ATA_DFLAG_DETACH;
+	}
+
+	ata_port_freeze(ap);
+	ata_port_schedule_eh(ap);
+}
+
 static void ata_acpi_handle_hotplug(struct ata_port *ap, struct ata_device *dev,
-				    u32 event)
+				    u32 event, int is_dock_event)
 {
 	char event_string[12];
 	char *envp[] = { event_string, NULL };
 	struct ata_eh_info *ehi;
 	struct kobject *kobj = NULL;
 	int wait = 0;
-	unsigned long flags;
+	unsigned long flags, sta;
+	acpi_status status;
+	acpi_handle handle;
 
 	if (!ap)
 		ap = dev->link->ap;
 	ehi = &ap->link.eh_info;
 
+	if (dev) {
+		if (dev->sdev)
+			kobj = &dev->sdev->sdev_gendev.kobj;
+		handle = dev->acpi_handle;
+	} else {
+		kobj = &ap->dev->kobj;
+		handle = ap->acpi_handle;
+	}
+
+	if (kobj && !is_dock_event) {
+		sprintf(event_string, "BAY_EVENT=%d", event);
+		kobject_uevent_env(kobj, KOBJ_CHANGE, envp);
+	}
+
 	spin_lock_irqsave(ap->lock, flags);
 
 	switch (event) {
 	case ACPI_NOTIFY_BUS_CHECK:
 	case ACPI_NOTIFY_DEVICE_CHECK:
 		ata_ehi_push_desc(ehi, "ACPI event");
-		ata_ehi_hotplugged(ehi);
-		ata_port_freeze(ap);
-		break;
+		status = acpi_evaluate_integer(handle, "_STA", NULL, &sta);
+		if (!ACPI_SUCCESS(status)) {
+			printk(KERN_INFO "Unable to evaluate bay status\n");
+			break;
+		}
+
+		if (sta) {
+			ata_ehi_hotplugged(ehi);
+			ata_port_freeze(ap);
+		} else {
+			/* The device has gone - unplug it */
+			ata_acpi_detach_device(ap, dev);
 
+			wait = 1;
+		}
+		break;
 	case ACPI_NOTIFY_EJECT_REQUEST:
 		ata_ehi_push_desc(ehi, "ACPI event");
-		if (dev)
-			dev->flags |= ATA_DFLAG_DETACH;
-		else {
-			struct ata_link *tlink;
-			struct ata_device *tdev;
-
-			ata_port_for_each_link(tlink, ap)
-				ata_link_for_each_dev(tdev, tlink)
-					tdev->flags |= ATA_DFLAG_DETACH;
-		}
+		if (!is_dock_event)
+			break;
 
-		ata_port_schedule_eh(ap);
+		/* undock event - immediate unplug */
+		ata_acpi_detach_device(ap, dev);
 		wait = 1;
 		break;
 	}
 
-	if (dev) {
-		if (dev->sdev)
-			kobj = &dev->sdev->sdev_gendev.kobj;
-	} else
-		kobj = &ap->dev->kobj;
-
-	if (kobj) {
-		sprintf(event_string, "BAY_EVENT=%d", event);
-		kobject_uevent_env(kobj, KOBJ_CHANGE, envp);
-	}
-
 	spin_unlock_irqrestore(ap->lock, flags);
 
 	if (wait)
 		ata_port_wait_eh(ap);
 }
 
+static void ata_acpi_dev_notify_dock(acpi_handle handle, u32 event, void *data)
+{
+	struct ata_device *dev = data;
+
+	ata_acpi_handle_hotplug(NULL, dev, event, 1);
+}
+
+static void ata_acpi_ap_notify_dock(acpi_handle handle, u32 event, void *data)
+{
+	struct ata_port *ap = data;
+
+	ata_acpi_handle_hotplug(ap, NULL, event, 1);
+}
+
 static void ata_acpi_dev_notify(acpi_handle handle, u32 event, void *data)
 {
 	struct ata_device *dev = data;
 
-	ata_acpi_handle_hotplug(NULL, dev, event);
+	ata_acpi_handle_hotplug(NULL, dev, event, 0);
 }
 
 static void ata_acpi_ap_notify(acpi_handle handle, u32 event, void *data)
 {
 	struct ata_port *ap = data;
 
-	ata_acpi_handle_hotplug(ap, NULL, event);
+	ata_acpi_handle_hotplug(ap, NULL, event, 0);
 }
 
 /**
@@ -229,7 +270,7 @@ void ata_acpi_associate(struct ata_host *host)
 						    ata_acpi_ap_notify, ap);
 			/* we might be on a docking station */
 			register_hotplug_dock_device(ap->acpi_handle,
-						     ata_acpi_ap_notify, ap);
+					     ata_acpi_ap_notify_dock, ap);
 		}
 
 		for (j = 0; j < ata_link_max_devices(&ap->link); j++) {
@@ -241,7 +282,7 @@ void ata_acpi_associate(struct ata_host *host)
 						ata_acpi_dev_notify, dev);
 				/* we might be on a docking station */
 				register_hotplug_dock_device(dev->acpi_handle,
-						ata_acpi_dev_notify, dev);
+					     ata_acpi_dev_notify_dock, dev);
 			}
 		}
 	}

Re: [PATCH] Fixups to ATA ACPI hotplug

[Matthew Garrett]

On Tue, May 20, 2008 at 09:44:42AM +0200, Holger Macht wrote:

> * In case of an ACPI_NOTIFY_DEVICE/BUS_CHECK, evaluate _STA to check if
>   the device has been plugged or unplugged. If plugged, hotplug it, if
>   unplugged, just signal event to userspace
>   (initial patch by Matthew Garrett <mjg59@srcf.ucam.org>)

The only issue I can see is that this one doesn't check _EJ0. Unless 
that's done, you'll (on some hardware) evaluate _STA on the bay itself 
rather than the device within the bay, which leads to confusion as to 
whether the device has been inserted. Other than that, looks good. 
Jeff's sent my patch to Linus, so can you redo this on top and I'll sign 
it off?

Thanks,
-- 
Matthew Garrett | mjg59@srcf.ucam.org

Re: [PATCH] Fixups to ATA ACPI hotplug

[Holger Macht]

On Tue 20. May - 11:20:38, Matthew Garrett wrote:
> On Tue, May 20, 2008 at 09:44:42AM +0200, Holger Macht wrote:
> 
> > * In case of an ACPI_NOTIFY_DEVICE/BUS_CHECK, evaluate _STA to check if
> >   the device has been plugged or unplugged. If plugged, hotplug it, if
> >   unplugged, just signal event to userspace
> >   (initial patch by Matthew Garrett <mjg59@srcf.ucam.org>)
> 
> The only issue I can see is that this one doesn't check _EJ0. Unless 
> that's done, you'll (on some hardware) evaluate _STA on the bay itself 
> rather than the device within the bay, which leads to confusion as to 
> whether the device has been inserted. Other than that, looks good. 
> Jeff's sent my patch to Linus, so can you redo this on top and I'll sign 
> it off?

Handle bay devices in dock stations

* Differentiate between bay devices in dock stations and others:

 - When an ACPI_NOTIFY_EJECT_REQUEST appears, just signal uevent to
   userspace (that is when the optional eject button on a bay device is
   pressed/pulled) giving the possibility to unmount file systems and to
   clean up. Also, only send uevent in case we get an EJECT_REQUEST
   without doing anything else. In other cases, you'll get an add/remove
   event because libata attaches/detaches the device.

 - In case of a dock event, which in turn signals an
   ACPI_NOTIFY_EJECT_REQUEST, immediately detach the device, because it
   may already have been gone

* In case of an ACPI_NOTIFY_DEVICE/BUS_CHECK, evaluate _STA to check if
  the device has been plugged or unplugged. If plugged, hotplug it, if
  unplugged, just signal event to userspace
  (initial patch by Matthew Garrett <mjg59@srcf.ucam.org>)

Signed-off-by: Holger Macht <hmacht@suse.de>
---

--- linux-2.6.25/drivers/ata/libata-acpi.c.orig	2008-05-20 13:25:50.000000000 +0200
+++ linux-2.6.25/drivers/ata/libata-acpi.c	2008-05-20 15:12:03.000000000 +0200
@@ -118,8 +118,25 @@ static void ata_acpi_associate_ide_port(
 		ap->pflags |= ATA_PFLAG_INIT_GTM_VALID;
 }
 
+static void ata_acpi_detach_device(struct ata_port *ap, struct ata_device *dev)
+{
+	if (dev)
+		dev->flags |= ATA_DFLAG_DETACH;
+	else {
+		struct ata_link *tlink;
+		struct ata_device *tdev;
+
+		ata_port_for_each_link(tlink, ap)
+			ata_link_for_each_dev(tdev, tlink)
+			tdev->flags |= ATA_DFLAG_DETACH;
+	}
+
+	ata_port_freeze(ap);
+	ata_port_schedule_eh(ap);
+}
+
 static void ata_acpi_handle_hotplug(struct ata_port *ap, struct ata_device
-				    *dev, u32 event)
+				    *dev, u32 event, int is_dock_event)
 {
 	char event_string[12];
 	char *envp[] = { event_string, NULL };
@@ -135,83 +152,92 @@ static void ata_acpi_handle_hotplug(stru
 		ap = dev->link->ap;
 	ehi = &ap->link.eh_info;
 
-	spin_lock_irqsave(ap->lock, flags);
-
-	if (dev)
+	if (dev) {
+		if (dev->sdev)
+			kobj = &dev->sdev->sdev_gendev.kobj;
 		handle = dev->acpi_handle;
-	else
+	} else {
+		kobj = &ap->dev->kobj;
 		handle = ap->acpi_handle;
-
-	status = acpi_get_handle(handle, "_EJ0", &tmphandle);
-	if (ACPI_FAILURE(status)) {
-		/* This device is not ejectable */
-		spin_unlock_irqrestore(ap->lock, flags);
-		return;
 	}
 
-	status = acpi_evaluate_integer(handle, "_STA", NULL, &sta);
-	if (ACPI_FAILURE(status)) {
-		printk ("Unable to determine bay status\n");
-		spin_unlock_irqrestore(ap->lock, flags);
-		return;
+	if (kobj && !is_dock_event) {
+		sprintf(event_string, "BAY_EVENT=%d", event);
+		kobject_uevent_env(kobj, KOBJ_CHANGE, envp);
 	}
 
+	spin_lock_irqsave(ap->lock, flags);
+
 	switch (event) {
 	case ACPI_NOTIFY_BUS_CHECK:
 	case ACPI_NOTIFY_DEVICE_CHECK:
 		ata_ehi_push_desc(ehi, "ACPI event");
-		if (!sta) {
-                /* Device has been unplugged */
-			if (dev)
-				dev->flags |= ATA_DFLAG_DETACH;
-			else {
-				struct ata_link *tlink;
-				struct ata_device *tdev;
-
-				ata_port_for_each_link(tlink, ap) {
-					ata_link_for_each_dev(tdev, tlink) {
-						tdev->flags |=
-							ATA_DFLAG_DETACH;
-					}
-				}
-			}
-			ata_port_schedule_eh(ap);
-			wait = 1;
-		} else {
+
+		status = acpi_get_handle(handle, "_EJ0", &tmphandle);
+		if (ACPI_FAILURE(status)) {
+			/* This device is not ejectable */
+			break;
+		}
+
+		status = acpi_evaluate_integer(handle, "_STA", NULL, &sta);
+		if (ACPI_FAILURE(status)) {
+			printk(KERN_ERR "Unable to determine bay status\n");
+			break;
+		}
+
+		if (sta) {
 			ata_ehi_hotplugged(ehi);
 			ata_port_freeze(ap);
+		} else {
+			/* The device has gone - unplug it */
+			ata_acpi_detach_device(ap, dev);
+			wait = 1;
 		}
+		break;
+	case ACPI_NOTIFY_EJECT_REQUEST:
+		ata_ehi_push_desc(ehi, "ACPI event");
+
+		if (!is_dock_event)
+			break;
+
+		/* undock event - immediate unplug */
+		ata_acpi_detach_device(ap, dev);
+		wait = 1;
+		break;
 	}
 
 	spin_unlock_irqrestore(ap->lock, flags);
 
 	if (wait)
 		ata_port_wait_eh(ap);
+}
 
-	if (dev) {
-		if (dev->sdev)
-			kobj = &dev->sdev->sdev_gendev.kobj;
-	} else
-		kobj = &ap->dev->kobj;
+static void ata_acpi_dev_notify_dock(acpi_handle handle, u32 event, void *data)
+{
+	struct ata_device *dev = data;
 
-	if (kobj) {
-		sprintf(event_string, "BAY_EVENT=%d", event);
-		kobject_uevent_env(kobj, KOBJ_CHANGE, envp);
-	}
+	ata_acpi_handle_hotplug(NULL, dev, event, 1);
+}
+
+static void ata_acpi_ap_notify_dock(acpi_handle handle, u32 event, void *data)
+{
+	struct ata_port *ap = data;
+
+	ata_acpi_handle_hotplug(ap, NULL, event, 1);
 }
 
 static void ata_acpi_dev_notify(acpi_handle handle, u32 event, void *data)
 {
 	struct ata_device *dev = data;
 
-	ata_acpi_handle_hotplug(NULL, dev, event);
+	ata_acpi_handle_hotplug(NULL, dev, event, 0);
 }
 
 static void ata_acpi_ap_notify(acpi_handle handle, u32 event, void *data)
 {
 	struct ata_port *ap = data;
 
-	ata_acpi_handle_hotplug(ap, NULL, event);
+	ata_acpi_handle_hotplug(ap, NULL, event, 0);
 }
 
 /**
@@ -252,7 +278,7 @@ void ata_acpi_associate(struct ata_host
 						    ata_acpi_ap_notify, ap);
 			/* we might be on a docking station */
 			register_hotplug_dock_device(ap->acpi_handle,
-						     ata_acpi_ap_notify, ap);
+					     ata_acpi_ap_notify_dock, ap);
 		}
 
 		for (j = 0; j < ata_link_max_devices(&ap->link); j++) {
@@ -264,7 +290,7 @@ void ata_acpi_associate(struct ata_host
 						ata_acpi_dev_notify, dev);
 				/* we might be on a docking station */
 				register_hotplug_dock_device(dev->acpi_handle,
-						ata_acpi_dev_notify, dev);
+					     ata_acpi_dev_notify_dock, dev);
 			}
 		}
 	}

Re: [PATCH] Fixups to ATA ACPI hotplug

[Matthew Garrett]

On Tue, May 20, 2008 at 03:18:32PM +0200, Holger Macht wrote:

> +	if (kobj && !is_dock_event) {
> +		sprintf(event_string, "BAY_EVENT=%d", event);
> +		kobject_uevent_env(kobj, KOBJ_CHANGE, envp);

I think we want to do the _EJ0 checking before this, otherwise we'll 
generate two uevents for the same removal on some hardware. Otherwise, 
looks good.

-- 
Matthew Garrett | mjg59@srcf.ucam.org

Re: [PATCH] Fixups to ATA ACPI hotplug

[Holger Macht]

On Tue 20. May - 14:22:17, Matthew Garrett wrote:
> On Tue, May 20, 2008 at 03:18:32PM +0200, Holger Macht wrote:
> 
> > +	if (kobj && !is_dock_event) {
> > +		sprintf(event_string, "BAY_EVENT=%d", event);
> > +		kobject_uevent_env(kobj, KOBJ_CHANGE, envp);
> 
> I think we want to do the _EJ0 checking before this, otherwise we'll 
> generate two uevents for the same removal on some hardware. Otherwise, 
> looks good.

So once again:

Handle bay devices in dock stations

* Differentiate between bay devices in dock stations and others:

 - When an ACPI_NOTIFY_EJECT_REQUEST appears, just signal uevent to
   userspace (that is when the optional eject button on a bay device is
   pressed/pulled) giving the possibility to unmount file systems and to
   clean up. Also, only send uevent in case we get an EJECT_REQUEST
   without doing anything else. In other cases, you'll get an add/remove
   event because libata attaches/detaches the device.

 - In case of a dock event, which in turn signals an
   ACPI_NOTIFY_EJECT_REQUEST, immediately detach the device, because it
   may already have been gone

* In case of an ACPI_NOTIFY_DEVICE/BUS_CHECK, evaluate _STA to check if
  the device has been plugged or unplugged. If plugged, hotplug it, if
  unplugged, just signal event to userspace
  (initial patch by Matthew Garrett <mjg59@srcf.ucam.org>)

Signed-off-by: Holger Macht <hmacht@suse.de>
---

--- linux-2.6.25/drivers/ata/libata-acpi.c.orig	2008-05-20 13:25:50.000000000 +0200
+++ linux-2.6.25/drivers/ata/libata-acpi.c	2008-05-20 15:56:38.000000000 +0200
@@ -118,8 +118,25 @@ static void ata_acpi_associate_ide_port(
 		ap->pflags |= ATA_PFLAG_INIT_GTM_VALID;
 }
 
+static void ata_acpi_detach_device(struct ata_port *ap, struct ata_device *dev)
+{
+	if (dev)
+		dev->flags |= ATA_DFLAG_DETACH;
+	else {
+		struct ata_link *tlink;
+		struct ata_device *tdev;
+
+		ata_port_for_each_link(tlink, ap)
+			ata_link_for_each_dev(tdev, tlink)
+			tdev->flags |= ATA_DFLAG_DETACH;
+	}
+
+	ata_port_freeze(ap);
+	ata_port_schedule_eh(ap);
+}
+
 static void ata_acpi_handle_hotplug(struct ata_port *ap, struct ata_device
-				    *dev, u32 event)
+				    *dev, u32 event, int is_dock_event)
 {
 	char event_string[12];
 	char *envp[] = { event_string, NULL };
@@ -135,83 +152,92 @@ static void ata_acpi_handle_hotplug(stru
 		ap = dev->link->ap;
 	ehi = &ap->link.eh_info;
 
-	spin_lock_irqsave(ap->lock, flags);
-
-	if (dev)
+	if (dev) {
+		if (dev->sdev)
+			kobj = &dev->sdev->sdev_gendev.kobj;
 		handle = dev->acpi_handle;
-	else
+	} else {
+		kobj = &ap->dev->kobj;
 		handle = ap->acpi_handle;
+	}
 
 	status = acpi_get_handle(handle, "_EJ0", &tmphandle);
 	if (ACPI_FAILURE(status)) {
-		/* This device is not ejectable */
-		spin_unlock_irqrestore(ap->lock, flags);
+		/* This device does not support hotplug */
 		return;
 	}
 
-	status = acpi_evaluate_integer(handle, "_STA", NULL, &sta);
-	if (ACPI_FAILURE(status)) {
-		printk ("Unable to determine bay status\n");
-		spin_unlock_irqrestore(ap->lock, flags);
-		return;
+	if (kobj && !is_dock_event) {
+		sprintf(event_string, "BAY_EVENT=%d", event);
+		kobject_uevent_env(kobj, KOBJ_CHANGE, envp);
 	}
 
+	spin_lock_irqsave(ap->lock, flags);
+
 	switch (event) {
 	case ACPI_NOTIFY_BUS_CHECK:
 	case ACPI_NOTIFY_DEVICE_CHECK:
 		ata_ehi_push_desc(ehi, "ACPI event");
-		if (!sta) {
-                /* Device has been unplugged */
-			if (dev)
-				dev->flags |= ATA_DFLAG_DETACH;
-			else {
-				struct ata_link *tlink;
-				struct ata_device *tdev;
-
-				ata_port_for_each_link(tlink, ap) {
-					ata_link_for_each_dev(tdev, tlink) {
-						tdev->flags |=
-							ATA_DFLAG_DETACH;
-					}
-				}
-			}
-			ata_port_schedule_eh(ap);
-			wait = 1;
-		} else {
+
+		status = acpi_evaluate_integer(handle, "_STA", NULL, &sta);
+		if (ACPI_FAILURE(status)) {
+			printk(KERN_ERR "Unable to determine bay status\n");
+			break;
+		}
+
+		if (sta) {
 			ata_ehi_hotplugged(ehi);
 			ata_port_freeze(ap);
+		} else {
+			/* The device has gone - unplug it */
+			ata_acpi_detach_device(ap, dev);
+			wait = 1;
 		}
+		break;
+	case ACPI_NOTIFY_EJECT_REQUEST:
+		ata_ehi_push_desc(ehi, "ACPI event");
+
+		if (!is_dock_event)
+			break;
+
+		/* undock event - immediate unplug */
+		ata_acpi_detach_device(ap, dev);
+		wait = 1;
+		break;
 	}
 
 	spin_unlock_irqrestore(ap->lock, flags);
 
 	if (wait)
 		ata_port_wait_eh(ap);
+}
 
-	if (dev) {
-		if (dev->sdev)
-			kobj = &dev->sdev->sdev_gendev.kobj;
-	} else
-		kobj = &ap->dev->kobj;
+static void ata_acpi_dev_notify_dock(acpi_handle handle, u32 event, void *data)
+{
+	struct ata_device *dev = data;
 
-	if (kobj) {
-		sprintf(event_string, "BAY_EVENT=%d", event);
-		kobject_uevent_env(kobj, KOBJ_CHANGE, envp);
-	}
+	ata_acpi_handle_hotplug(NULL, dev, event, 1);
+}
+
+static void ata_acpi_ap_notify_dock(acpi_handle handle, u32 event, void *data)
+{
+	struct ata_port *ap = data;
+
+	ata_acpi_handle_hotplug(ap, NULL, event, 1);
 }
 
 static void ata_acpi_dev_notify(acpi_handle handle, u32 event, void *data)
 {
 	struct ata_device *dev = data;
 
-	ata_acpi_handle_hotplug(NULL, dev, event);
+	ata_acpi_handle_hotplug(NULL, dev, event, 0);
 }
 
 static void ata_acpi_ap_notify(acpi_handle handle, u32 event, void *data)
 {
 	struct ata_port *ap = data;
 
-	ata_acpi_handle_hotplug(ap, NULL, event);
+	ata_acpi_handle_hotplug(ap, NULL, event, 0);
 }
 
 /**
@@ -252,7 +278,7 @@ void ata_acpi_associate(struct ata_host
 						    ata_acpi_ap_notify, ap);
 			/* we might be on a docking station */
 			register_hotplug_dock_device(ap->acpi_handle,
-						     ata_acpi_ap_notify, ap);
+					     ata_acpi_ap_notify_dock, ap);
 		}
 
 		for (j = 0; j < ata_link_max_devices(&ap->link); j++) {
@@ -264,7 +290,7 @@ void ata_acpi_associate(struct ata_host
 						ata_acpi_dev_notify, dev);
 				/* we might be on a docking station */
 				register_hotplug_dock_device(dev->acpi_handle,
-						ata_acpi_dev_notify, dev);
+					     ata_acpi_dev_notify_dock, dev);
 			}
 		}
 	}

Re: [PATCH] Fixups to ATA ACPI hotplug

[Matthew Garrett]

On Tue, May 20, 2008 at 03:58:30PM +0200, Holger Macht wrote:

> Handle bay devices in dock stations
> 
> * Differentiate between bay devices in dock stations and others:
> 
>  - When an ACPI_NOTIFY_EJECT_REQUEST appears, just signal uevent to
>    userspace (that is when the optional eject button on a bay device is
>    pressed/pulled) giving the possibility to unmount file systems and to
>    clean up. Also, only send uevent in case we get an EJECT_REQUEST
>    without doing anything else. In other cases, you'll get an add/remove
>    event because libata attaches/detaches the device.
> 
>  - In case of a dock event, which in turn signals an
>    ACPI_NOTIFY_EJECT_REQUEST, immediately detach the device, because it
>    may already have been gone
> 
> * In case of an ACPI_NOTIFY_DEVICE/BUS_CHECK, evaluate _STA to check if
>   the device has been plugged or unplugged. If plugged, hotplug it, if
>   unplugged, just signal event to userspace
>   (initial patch by Matthew Garrett <mjg59@srcf.ucam.org>)
> 
> Signed-off-by: Holger Macht <hmacht@suse.de>

As long as this fixes the hang on dock removal, this looks good to me.

Acked-by: Matthew Garrett <mjg@redhat.com>
-- 
Matthew Garrett | mjg59@srcf.ucam.org

Re: [PATCH] Fixups to ATA ACPI hotplug

[Andrew Morton]

On Tue, 20 May 2008 15:58:30 +0200
Holger Macht <hmacht@suse.de> wrote:

> On Tue 20. May - 14:22:17, Matthew Garrett wrote:
> > On Tue, May 20, 2008 at 03:18:32PM +0200, Holger Macht wrote:
> > 
> > > +	if (kobj && !is_dock_event) {
> > > +		sprintf(event_string, "BAY_EVENT=%d", event);
> > > +		kobject_uevent_env(kobj, KOBJ_CHANGE, envp);
> > 
> > I think we want to do the _EJ0 checking before this, otherwise we'll 
> > generate two uevents for the same removal on some hardware. Otherwise, 
> > looks good.
> 
> So once again:
> 
> Handle bay devices in dock stations
> 
> * Differentiate between bay devices in dock stations and others:
> 
>  - When an ACPI_NOTIFY_EJECT_REQUEST appears, just signal uevent to
>    userspace (that is when the optional eject button on a bay device is
>    pressed/pulled) giving the possibility to unmount file systems and to
>    clean up. Also, only send uevent in case we get an EJECT_REQUEST
>    without doing anything else. In other cases, you'll get an add/remove
>    event because libata attaches/detaches the device.
> 
>  - In case of a dock event, which in turn signals an
>    ACPI_NOTIFY_EJECT_REQUEST, immediately detach the device, because it
>    may already have been gone
> 
> * In case of an ACPI_NOTIFY_DEVICE/BUS_CHECK, evaluate _STA to check if
>   the device has been plugged or unplugged. If plugged, hotplug it, if
>   unplugged, just signal event to userspace
>   (initial patch by Matthew Garrett <mjg59@srcf.ucam.org>)
> 
> Signed-off-by: Holger Macht <hmacht@suse.de>
> ---
> 
> --- linux-2.6.25/drivers/ata/libata-acpi.c.orig	2008-05-20 13:25:50.000000000 +0200
> +++ linux-2.6.25/drivers/ata/libata-acpi.c	2008-05-20 15:56:38.000000000 +0200
> @@ -118,8 +118,25 @@ static void ata_acpi_associate_ide_port(
>  		ap->pflags |= ATA_PFLAG_INIT_GTM_VALID;
>  }
>  
> +static void ata_acpi_detach_device(struct ata_port *ap, struct ata_device *dev)
> +{
> +	if (dev)
> +		dev->flags |= ATA_DFLAG_DETACH;
> +	else {
> +		struct ata_link *tlink;
> +		struct ata_device *tdev;
> +
> +		ata_port_for_each_link(tlink, ap)
> +			ata_link_for_each_dev(tdev, tlink)
> +			tdev->flags |= ATA_DFLAG_DETACH;

Looks odd.   I guess this:

--- a/drivers/ata/libata-acpi.c~ata-acpi-hotplug-handle-bay-devices-in-dock-stations-cleanup
+++ a/drivers/ata/libata-acpi.c
@@ -128,7 +128,7 @@ static void ata_acpi_detach_device(struc
 
 		ata_port_for_each_link(tlink, ap)
 			ata_link_for_each_dev(tdev, tlink)
-			tdev->flags |= ATA_DFLAG_DETACH;
+				tdev->flags |= ATA_DFLAG_DETACH;
 	}
 
 	ata_port_freeze(ap);
_

was intended.

> +	}
> +
> +	ata_port_freeze(ap);
> +	ata_port_schedule_eh(ap);
> +}
> +
>  static void ata_acpi_handle_hotplug(struct ata_port *ap, struct ata_device
> -				    *dev, u32 event)
> +				    *dev, u32 event, int is_dock_event)
>  {
>  	char event_string[12];
>  	char *envp[] = { event_string, NULL };
> @@ -135,83 +152,92 @@ static void ata_acpi_handle_hotplug(stru
>  		ap = dev->link->ap;
>  	ehi = &ap->link.eh_info;
>  
> -	spin_lock_irqsave(ap->lock, flags);
> -
> -	if (dev)
> +	if (dev) {
> +		if (dev->sdev)
> +			kobj = &dev->sdev->sdev_gendev.kobj;
>  		handle = dev->acpi_handle;
> -	else
> +	} else {
> +		kobj = &ap->dev->kobj;
>  		handle = ap->acpi_handle;
> +	}
>  
>  	status = acpi_get_handle(handle, "_EJ0", &tmphandle);
>  	if (ACPI_FAILURE(status)) {
> -		/* This device is not ejectable */
> -		spin_unlock_irqrestore(ap->lock, flags);
> +		/* This device does not support hotplug */
>  		return;
>  	}
>  
> -	status = acpi_evaluate_integer(handle, "_STA", NULL, &sta);
> -	if (ACPI_FAILURE(status)) {
> -		printk ("Unable to determine bay status\n");
> -		spin_unlock_irqrestore(ap->lock, flags);
> -		return;
> +	if (kobj && !is_dock_event) {
> +		sprintf(event_string, "BAY_EVENT=%d", event);
> +		kobject_uevent_env(kobj, KOBJ_CHANGE, envp);
>  	}
>  
> +	spin_lock_irqsave(ap->lock, flags);

Wow, lots of stuff happening under spinlock here, but it all looks OK to me.

>  	switch (event) {
>  	case ACPI_NOTIFY_BUS_CHECK:
>  	case ACPI_NOTIFY_DEVICE_CHECK:
>  		ata_ehi_push_desc(ehi, "ACPI event");
> -		if (!sta) {
> -                /* Device has been unplugged */
> -			if (dev)
> -				dev->flags |= ATA_DFLAG_DETACH;
> -			else {
> -				struct ata_link *tlink;
> -				struct ata_device *tdev;
> -
> -				ata_port_for_each_link(tlink, ap) {
> -					ata_link_for_each_dev(tdev, tlink) {
> -						tdev->flags |=
> -							ATA_DFLAG_DETACH;
> -					}
> -				}

The old code was less odd ;)

Re: [PATCH] Fixups to ATA ACPI hotplug

[Holger Macht]

On Mon 19. May - 17:29:34, Matthew Garrett wrote:
> The libata-acpi.c code currently accepts hotplug messages from both the 
> port and the device. This does not match the behaviour of the bay 
> driver, and may result in confusion when two hotplug requests are 
> received for the same device. This patch limits the hotplug notification 
> to removable ACPI devices, which in turn allows it to use the _STA 
> method to determine whether the device has been removed or inserted. 
> On removal, devices are marked as detached. On insertion, a hotplug scan 
> is started. This should avoid lockups caused by the ata layer attempting 
> to scan devices which have been removed. The uevent sending is moved 
> outside the spinlock in order to avoid a warning generated by it firing 
> when interrupts are disabled.
> 
> Signed-off-by: Matthew Garrett <mjg@redhat.com>
> 
> ---
> 
> Holger, I'm pretty sure that this deals with the docking station removal 
> case, but don't have the hardware to test. If EJECT_REQUEST is genuinely 

I tried it, it doesn't.

 Undock --> some userspace app accesses the device --> hard lockup

> the only notification we receive (ie, no BUS_CHECK or DEVICE_CHECK) then 
> we'll need to add a separate callback for docking.

Yes, see my patch.

Regards,
	Holger