[PATCH] libata: Handle bay devices in dock stations

[PATCH] libata: Handle bay devices in dock stations

[Holger Macht]

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

* Call ACPI _EJ0 for detached devices

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

Changes regarding the previous patch:

 * Make sure kobj does not go away outside locking
 * Coding style cleanups
 * Call _EJ0 for detached devices

diff --git a/drivers/ata/libata-acpi.c b/drivers/ata/libata-acpi.c
index dbf6ca7..e0e49fd 100644
--- a/drivers/ata/libata-acpi.c
+++ b/drivers/ata/libata-acpi.c
@@ -118,8 +118,40 @@ static void ata_acpi_associate_ide_port(struct ata_port *ap)
 		ap->pflags |= ATA_PFLAG_INIT_GTM_VALID;
 }
 
+static void ata_acpi_eject_device(acpi_handle handle)
+{
+	struct acpi_object_list arg_list;
+	union acpi_object arg;
+
+	arg_list.count = 1;
+	arg_list.pointer = &arg;
+	arg.type = ACPI_TYPE_INTEGER;
+	arg.integer.value = 1;
+
+	if (ACPI_FAILURE(acpi_evaluate_object(handle, "_EJ0",
+					      &arg_list, NULL)))
+		printk(KERN_ERR "Failed to evaluate _EJ0!\n");
+}
+
+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 +167,98 @@ static void ata_acpi_handle_hotplug(struct ata_port *ap, struct ata_device
 		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);
+	if (ACPI_FAILURE(status))
+		/* 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;
-	}
+	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");
 
-	spin_unlock_irqrestore(ap->lock, flags);
+		if (!is_dock_event)
+			break;
 
-	if (wait)
-		ata_port_wait_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;
+	/* make sure kobj doesn't go away while ap->lock is released */
+	kobject_get(kobj);
+
+	spin_unlock_irqrestore(ap->lock, flags);
 
-	if (kobj) {
+	if (kobj && !is_dock_event) {
 		sprintf(event_string, "BAY_EVENT=%d", event);
 		kobject_uevent_env(kobj, KOBJ_CHANGE, envp);
 	}
+
+	kobject_put(kobj);
+
+	if (wait) {
+		ata_port_wait_eh(ap);
+		ata_acpi_eject_device(handle);
+	}
+}
+
+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);
 }
 
 /**
@@ -252,7 +299,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++) {
@@ -264,7 +311,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] libata: Handle bay devices in dock stations

[Tejun Heo]

Holger Macht wrote:
> * 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>)
> 
> * Call ACPI _EJ0 for detached devices
> 
> Signed-off-by: Holger Macht <hmacht@suse.de>

Acked-by: Tejun Heo <htejun@gmail.com>

-- 
tejun

Re: [PATCH] libata: Handle bay devices in dock stations

[Andrew Morton]

On Wed, 28 May 2008 16:38:57 +0200 Holger Macht <hmacht@suse.de> wrote:

> * 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>)
> 
> * Call ACPI _EJ0 for detached devices
> 
> ...
> 
> +static void ata_acpi_eject_device(acpi_handle handle)
> +{
> +	struct acpi_object_list arg_list;
> +	union acpi_object arg;
> +
> +	arg_list.count = 1;
> +	arg_list.pointer = &arg;
> +	arg.type = ACPI_TYPE_INTEGER;
> +	arg.integer.value = 1;

This might look nicer (and safer) if it used the

	union acpi_object arg = {
		.foo = bar,
		...
	};

form.  However this can cause gcc to emit poor code.

> +	if (ACPI_FAILURE(acpi_evaluate_object(handle, "_EJ0",
> +					      &arg_list, NULL)))
> +		printk(KERN_ERR "Failed to evaluate _EJ0!\n");
> +}

It would be better if the printk were to self-identify where it is
coming from.  If your kernel just blurts "Failed to evaluate _EJ0!"
it's a bit of a head-scratcher.

"libata-acpi: failed to evaluate _EJ0", perhaps?


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

I guess the significance of `dev==0' is known to those who are steeped
in ata_acpi_handle_hotplug() knowledge, but it's pretty inscrutable to
the occasional visitor.  Some comments would help.

Re: [PATCH] libata: Handle bay devices in dock stations

[Tejun Heo]

Andrew Morton wrote:
>> +	if (ACPI_FAILURE(acpi_evaluate_object(handle, "_EJ0",
>> +					      &arg_list, NULL)))
>> +		printk(KERN_ERR "Failed to evaluate _EJ0!\n");
>> +}
> 
> It would be better if the printk were to self-identify where it is
> coming from.  If your kernel just blurts "Failed to evaluate _EJ0!"
> it's a bit of a head-scratcher.
> 
> "libata-acpi: failed to evaluate _EJ0", perhaps?

ata_port_printk() would be better.

Thanks.

-- 
tejun

Re: [PATCH] libata: Handle bay devices in dock stations

[Matthew Garrett]

Hm. This deadlocks my Thinkpad when I pull a device from the internal 
bay.

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

Re: [PATCH] libata: Handle bay devices in dock stations

[Matthew Garrett]

On Thu, May 29, 2008 at 03:33:02PM +0200, Holger Macht wrote:

> Can you please give a bit more details? Does your userspace react on the
> eject request? I tested with Thinkpads, too.

No, I'm not doing anything with userspace - just pulling the drive out 
directly. I'm looking into it now.

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

Re: [PATCH] libata: Handle bay devices in dock stations

[Holger Macht]

On Thu 29. May - 14:22:53, Matthew Garrett wrote:
> Hm. This deadlocks my Thinkpad when I pull a device from the internal 
> bay.

Can you please give a bit more details? Does your userspace react on the
eject request? I tested with Thinkpads, too.

 - Holger

Re: [PATCH] libata: Handle bay devices in dock stations

[Holger Macht]

On Thu 29. May - 14:32:01, Matthew Garrett wrote:
> On Thu, May 29, 2008 at 03:33:02PM +0200, Holger Macht wrote:
> 
> > Can you please give a bit more details? Does your userspace react on the
> > eject request? I tested with Thinkpads, too.
> 
> No, I'm not doing anything with userspace - just pulling the drive out 
> directly. I'm looking into it now.

This was one of my test scenarios. Just a hint for debugging, before my
patch, HAL accessed /dev/sr0 directly after removing the device, and that
caused the lockup. Hopefully that doesn't bite us again...

 - Holger

Re: [PATCH] libata: Handle bay devices in dock stations

[Matthew Garrett]

Running with init=/bin/bash, I still get it. The only kernel output I 
get is libata telling me that the device is disabled, and the machine is 
dead after that.

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

Re: [PATCH] libata: Handle bay devices in dock stations

[Tejun Heo]

Does this fix the problem?

---
 drivers/ata/libata-acpi.c |  166 +++++++++++++++++++++++++++++++---------------
 1 file changed, 115 insertions(+), 51 deletions(-)

diff --git a/drivers/ata/libata-acpi.c b/drivers/ata/libata-acpi.c
index dbf6ca7..1ac3e8f 100644
--- a/drivers/ata/libata-acpi.c
+++ b/drivers/ata/libata-acpi.c
@@ -118,12 +118,63 @@ 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_eject_device(acpi_handle handle)
+{
+	struct acpi_object_list arg_list;
+	union acpi_object arg;
+
+	arg_list.count = 1;
+	arg_list.pointer = &arg;
+	arg.type = ACPI_TYPE_INTEGER;
+	arg.integer.value = 1;
+
+	if (ACPI_FAILURE(acpi_evaluate_object(handle, "_EJ0",
+					      &arg_list, NULL)))
+		printk(KERN_ERR "Failed to evaluate _EJ0!\n");
+}
+
+/* @ap and @dev are the same as ata_acpi_handle_hotplug() */
+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);
+}
+
+/**
+ * ata_acpi_handle_hotplug - ACPI event handler backend
+ * @ap: ATA port ACPI event occurred
+ * @dev: ATA device ACPI event occurred (can be NULL)
+ * @event: ACPI event which occurred
+ * @is_dock_event: boolean indicating whether the event was a dock one
+ *
+ * All ACPI bay / device realted events end up in this function.  If
+ * the event is port-wide @dev is NULL.  If the event is specific to a
+ * device, @dev points to it.
+ *
+ * Hotplug (as opposed to unplug) notification is always handled as
+ * port-wide while unplug only kills the target device on device-wide
+ * event.
+ *
+ * LOCKING:
+ * ACPI notify handler context.  May sleep.
+ */
+static void ata_acpi_handle_hotplug(struct ata_port *ap, struct ata_device *dev,
+				    u32 event, int is_dock_event)
 {
 	char event_string[12];
 	char *envp[] = { event_string, NULL };
-	struct ata_eh_info *ehi;
+	struct ata_eh_info *ehi = &ap->link.eh_info;
 	struct kobject *kobj = NULL;
 	int wait = 0;
 	unsigned long flags;
@@ -131,87 +182,100 @@ static void ata_acpi_handle_hotplug(struct ata_port *ap, struct ata_device
 	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)
+	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);
+	if (ACPI_FAILURE(status))
+		/* 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;
-	}
+	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)) {
+			ata_port_printk(ap, KERN_ERR,
+				"acpi: failed to determine bay status (0x%x)\n",
+				status);
+			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;
 	}
 
+	/* make sure kobj doesn't go away while ap->lock is released */
+	kobject_get(kobj);
+
 	spin_unlock_irqrestore(ap->lock, flags);
 
-	if (wait)
+	if (wait) {
 		ata_port_wait_eh(ap);
+		ata_acpi_eject_device(handle);
+	}
 
-	if (dev) {
-		if (dev->sdev)
-			kobj = &dev->sdev->sdev_gendev.kobj;
-	} else
-		kobj = &ap->dev->kobj;
-
-	if (kobj) {
+	if (kobj && !is_dock_event) {
 		sprintf(event_string, "BAY_EVENT=%d", event);
 		kobject_uevent_env(kobj, KOBJ_CHANGE, envp);
 	}
+
+	kobject_put(kobj);
+}
+
+static void ata_acpi_dev_notify_dock(acpi_handle handle, u32 event, void *data)
+{
+	struct ata_device *dev = data;
+
+	ata_acpi_handle_hotplug(dev->link->ap, 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(dev->link->ap, 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 +316,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++) {
@@ -264,7 +328,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] libata: Handle bay devices in dock stations

[Matthew Garrett]

On Thu, May 29, 2008 at 10:44:29PM +0900, Tejun Heo wrote:
> Does this fix the problem?

No, I'm seeing an identical hang. Mainline works fine.
-- 
Matthew Garrett | mjg59@srcf.ucam.org

Re: [PATCH] libata: Handle bay devices in dock stations

[Holger Macht]

On Thu 29. May - 15:02:13, Matthew Garrett wrote:
> On Thu, May 29, 2008 at 10:44:29PM +0900, Tejun Heo wrote:
> > Does this fix the problem?
> 
> No, I'm seeing an identical hang. Mainline works fine.

Maybe you could also try this on top of Tejun's patch?

--- linux-2.6/drivers/ata/libata-acpi.c.orig	2008-05-29 16:13:00.000000000 +0200
+++ linux-2.6/drivers/ata/libata-acpi.c	2008-05-29 16:13:04.000000000 +0200
@@ -237,10 +237,8 @@
 
 	spin_unlock_irqrestore(ap->lock, flags);
 
-	if (wait) {
+	if (wait)
 		ata_port_wait_eh(ap);
-		ata_acpi_eject_device(handle);
-	}
 
 	if (kobj && !is_dock_event) {
 		sprintf(event_string, "BAY_EVENT=%d", event);

Re: [PATCH] libata: Handle bay devices in dock stations

[Matthew Garrett]

On Thu, May 29, 2008 at 04:14:21PM +0200, Holger Macht wrote:
> On Thu 29. May - 15:02:13, Matthew Garrett wrote:
> > No, I'm seeing an identical hang. Mainline works fine.
> 
> Maybe you could also try this on top of Tejun's patch?

No luck there either, I'm afraid.
-- 
Matthew Garrett | mjg59@srcf.ucam.org

Re: [PATCH] libata: Handle bay devices in dock stations

[Tejun Heo]

Matthew Garrett wrote:
> On Thu, May 29, 2008 at 04:14:21PM +0200, Holger Macht wrote:
>> On Thu 29. May - 15:02:13, Matthew Garrett wrote:
>>> No, I'm seeing an identical hang. Mainline works fine.
>> Maybe you could also try this on top of Tejun's patch?
> 
> No luck there either, I'm afraid.

That means device isn't being detached or something afterwards triggers
hotplug event on the detached port.  Can you please sprinkle printks
around and see what ata_acpi_handle_hotplug() does?

Thanks.

-- 
tejun

Re: [PATCH] libata: Handle bay devices in dock stations

[Matthew Garrett]

Got it. It works if I lose the ata_port_freeze() from 
ata_acpi_detach_device. 

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

Re: [PATCH] libata: Handle bay devices in dock stations

[Holger Macht]

On Thu 29. May - 15:49:34, Matthew Garrett wrote:
> Got it. It works if I lose the ata_port_freeze() from 
> ata_acpi_detach_device. 

Tejun, any idea? This is in for quite some time now. Maybe there's some
involved...

Regards,
	Holger

Re: [PATCH] libata: Handle bay devices in dock stations

[Tejun Heo]

Holger Macht wrote:
> On Thu 29. May - 15:49:34, Matthew Garrett wrote:
>> Got it. It works if I lose the ata_port_freeze() from 
>> ata_acpi_detach_device. 
> 
> Tejun, any idea? This is in for quite some time now. Maybe there's some
> involved...

Does changing it to ata_port_schedule_eh() make any difference?

-- 
tejun

Re: [PATCH] libata: Handle bay devices in dock stations

[Holger Macht]

On Thu 29. May - 18:32:02, Holger Macht wrote:
> On Thu 29. May - 15:49:34, Matthew Garrett wrote:
> > Got it. It works if I lose the ata_port_freeze() from 
> > ata_acpi_detach_device. 
> 
> Tejun, any idea? This is in for quite some time now. Maybe there's some
> involved...

The last sentence misses the one important word 'race'.

 - Holger

Re: [PATCH] libata: Handle bay devices in dock stations

[Henrique de Moraes Holschuh]

On Thu, 29 May 2008, Holger Macht wrote:
> On Thu 29. May - 15:49:34, Matthew Garrett wrote:
> > Got it. It works if I lose the ata_port_freeze() from 
> > ata_acpi_detach_device. 
> 
> Tejun, any idea? This is in for quite some time now. Maybe there's some
> involved...

<naivë question> Do we handle the possible fact that _EJ0 might want the
port (and maybe the device) to be active when it is called AND that _EJ0
might attempt to shutdown the port or the device itself?  </naivë
question>

Please ignore if it is something not even worth answering.

-- 
  "One disk to rule them all, One disk to find them. One disk to bring
  them all and in the darkness grind them. In the Land of Redmond
  where the shadows lie." -- The Silicon Valley Tarot
  Henrique Holschuh

Re: [PATCH] libata: Handle bay devices in dock stations

[tom]

Quoting Holger Macht <hmacht@suse.de>:

> * 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>)
>
> * Call ACPI _EJ0 for detached devices

Will this patch or the other bay/dock related patches you send in the  
past days allow me to undock my laptop and still be able to  
suspend/resume without locking the laptop up? And without having to  
run any userspace scripts?

My dock is not a simple port replicator, it has an USB hub and an ATA  
bay in it. My tests showed that I need to 'echo 1 >  
/sys/.../scsi/.../eject' or something like that before I can take the  
laptop out of the docking station. If I don't do that and try to  
access the cdrom in the bay (or even rescaning the scsi bus) after I  
have taken the laptop out of the dock it results in a hard lockup.  
That in itself would not be a problem, because it's just a simple  
command that I can do in an acpid script. But far worse is that even  
if I do that, the computer locks up when I resume from suspend. I run  
2.6.24.7 which locks up every time I resume. In recent versions from  
git it has somehow improved, there are situations where it doesn't  
lock up, but there are still a few left where it does (I don't  
remember the exact actions/commands I have to take).

tom

Re: [PATCH] libata: Handle bay devices in dock stations

[Holger Macht]

On Fri 30. May - 01:40:19, Tejun Heo wrote:
> Holger Macht wrote:
> > On Thu 29. May - 15:49:34, Matthew Garrett wrote:
> >> Got it. It works if I lose the ata_port_freeze() from 
> >> ata_acpi_detach_device. 
> > 
> > Tejun, any idea? This is in for quite some time now. Maybe there's some
> > involved...
> 
> Does changing it to ata_port_schedule_eh() make any difference?

Finally found a system to reproduce the freeze...

And yes, removing ata_port_freeze() and only doing ata_port_schedule_eh()
in the case where the device has already gone works and prevents the
freeze. Also the docking case works. If this change is not only for
finding out what's going wrong, I'm going resend the patch with your
documentation additions, ok?

Regards,
	Holger

Re: [PATCH] libata: Handle bay devices in dock stations

[Holger Macht]

On Fri 30. May - 13:07:39, tom@dbservice.com wrote:
> Quoting Holger Macht <hmacht@suse.de>:
>
>> * 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>)
>>
>> * Call ACPI _EJ0 for detached devices
>
> Will this patch or the other bay/dock related patches you send in the past 
> days allow me to undock my laptop and still be able to suspend/resume 
> without locking the laptop up? And without having to run any userspace 
> scripts?
>
> My dock is not a simple port replicator, it has an USB hub and an ATA bay 
> in it. My tests showed that I need to 'echo 1 > /sys/.../scsi/.../eject' or 
> something like that before I can take the laptop out of the docking 
> station. If I don't do that and try to access the cdrom in the bay (or even 
> rescaning the scsi bus) after I have taken the laptop out of the dock it 
> results in a hard lockup. That in itself would not be a problem, because 
> it's just a simple command that I can do in an acpid script. But far worse 
> is that even if I do that, the computer locks up when I resume from 
> suspend. I run 2.6.24.7 which locks up every time I resume. In recent 
> versions from git it has somehow improved, there are situations where it 
> doesn't lock up, but there are still a few left where it does (I don't 
> remember the exact actions/commands I have to take).

Which kernel versions did you already try? But yes, those patches will
most likely help. But not sure about the resume issues...

Regards,
	Holger

Re: [PATCH] libata: Handle bay devices in dock stations

[Jeff Garzik]

Holger Macht wrote:
> * 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>)
> 
> * Call ACPI _EJ0 for detached devices
> 
> Signed-off-by: Holger Macht <hmacht@suse.de>
> ---
> 
> Changes regarding the previous patch:
> 
>  * Make sure kobj does not go away outside locking
>  * Coding style cleanups
>  * Call _EJ0 for detached devices

Matthew, any comments?

It would be nice if you and Holger could work together to produce a 
single patch[set]...  right now I have patches from both of you, and I 
was sorta waiting on the thread to die down to see if competing patches 
might merge into a single set

Re: [PATCH] libata: Handle bay devices in dock stations

[Matthew Garrett]

On Tue, Jun 03, 2008 at 02:07:42PM -0400, Jeff Garzik wrote:

> It would be nice if you and Holger could work together to produce a 
> single patch[set]...  right now I have patches from both of you, and I 
> was sorta waiting on the thread to die down to see if competing patches 
> might merge into a single set

I think we were waiting for feedback from Tejun as to why removing the 
port freeze call fixed the hang I was seeing? Beyond that, Holger's 
latest patch looked good to me.

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

Re: [PATCH] libata: Handle bay devices in dock stations

[Holger Macht]

On Tue 03. Jun - 19:13:46, Matthew Garrett wrote:
> On Tue, Jun 03, 2008 at 02:07:42PM -0400, Jeff Garzik wrote:
> 
> > It would be nice if you and Holger could work together to produce a 
> > single patch[set]...  right now I have patches from both of you, and I 
> > was sorta waiting on the thread to die down to see if competing patches 
> > might merge into a single set
> 
> I think we were waiting for feedback from Tejun as to why removing the 
> port freeze call fixed the hang I was seeing? Beyond that, Holger's 
> latest patch looked good to me.

My suspicion is just that calling ata_port_freeze() on an already frozen
port is not good ;-)

I'll resend the merged patch.

Regards,
	Holger

Re: [PATCH] libata: Handle bay devices in dock stations

[Holger Macht]

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

* Call ACPI _EJ0 for detached devices

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

Previous patch differences:
 * Remove one call to ata_port_freeze
 * Add some documentation from Tejun

diff --git a/drivers/ata/libata-acpi.c b/drivers/ata/libata-acpi.c
index dbf6ca7..3ff8b14 100644
--- a/drivers/ata/libata-acpi.c
+++ b/drivers/ata/libata-acpi.c
@@ -118,12 +118,62 @@ 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_eject_device(acpi_handle handle)
+{
+	struct acpi_object_list arg_list;
+	union acpi_object arg;
+
+	arg_list.count = 1;
+	arg_list.pointer = &arg;
+	arg.type = ACPI_TYPE_INTEGER;
+	arg.integer.value = 1;
+
+	if (ACPI_FAILURE(acpi_evaluate_object(handle, "_EJ0",
+					      &arg_list, NULL)))
+		printk(KERN_ERR "Failed to evaluate _EJ0!\n");
+}
+
+/* @ap and @dev are the same as ata_acpi_handle_hotplug() */
+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_schedule_eh(ap);
+}
+
+/**
+ * ata_acpi_handle_hotplug - ACPI event handler backend
+ * @ap: ATA port ACPI event occurred
+ * @dev: ATA device ACPI event occurred (can be NULL)
+ * @event: ACPI event which occurred
+ * @is_dock_event: boolean indicating whether the event was a dock one
+ *
+ * All ACPI bay / device realted events end up in this function.  If
+ * the event is port-wide @dev is NULL.  If the event is specific to a
+ * device, @dev points to it.
+ *
+ * Hotplug (as opposed to unplug) notification is always handled as
+ * port-wide while unplug only kills the target device on device-wide
+ * event.
+ *
+ * LOCKING:
+ * ACPI notify handler context.  May sleep.
+ */
+static void ata_acpi_handle_hotplug(struct ata_port *ap, struct ata_device *dev,
+				    u32 event, int is_dock_event)
 {
 	char event_string[12];
 	char *envp[] = { event_string, NULL };
-	struct ata_eh_info *ehi;
+	struct ata_eh_info *ehi = &ap->link.eh_info;
 	struct kobject *kobj = NULL;
 	int wait = 0;
 	unsigned long flags;
@@ -131,87 +181,100 @@ static void ata_acpi_handle_hotplug(struct ata_port *ap, struct ata_device
 	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)
+	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);
+	if (ACPI_FAILURE(status))
+		/* 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;
-	}
+	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)) {
+			ata_port_printk(ap, KERN_ERR,
+				"acpi: failed to determine bay status (0x%x)\n",
+				status);
+			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;
 	}
 
+	/* make sure kobj doesn't go away while ap->lock is released */
+	kobject_get(kobj);
+
 	spin_unlock_irqrestore(ap->lock, flags);
 
-	if (wait)
+	if (wait) {
 		ata_port_wait_eh(ap);
+		ata_acpi_eject_device(handle);
+	}
 
-	if (dev) {
-		if (dev->sdev)
-			kobj = &dev->sdev->sdev_gendev.kobj;
-	} else
-		kobj = &ap->dev->kobj;
-
-	if (kobj) {
+	if (kobj && !is_dock_event) {
 		sprintf(event_string, "BAY_EVENT=%d", event);
 		kobject_uevent_env(kobj, KOBJ_CHANGE, envp);
 	}
+
+	kobject_put(kobj);
+}
+
+static void ata_acpi_dev_notify_dock(acpi_handle handle, u32 event, void *data)
+{
+	struct ata_device *dev = data;
+
+	ata_acpi_handle_hotplug(dev->link->ap, 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(dev->link->ap, 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 +315,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++) {
@@ -264,7 +327,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] libata: Handle bay devices in dock stations

[Matthew Garrett]

On Tue, Jun 03, 2008 at 08:27:59PM +0200, Holger Macht wrote:
> * 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>)
> 
> * Call ACPI _EJ0 for detached devices
> 
> Signed-off-by: Holger Macht <hmacht@suse.de>

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

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

Re: [PATCH] libata: Handle bay devices in dock stations

[Jeff Garzik]

Matthew Garrett wrote:
> On Tue, Jun 03, 2008 at 08:27:59PM +0200, Holger Macht wrote:
>> * 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>)
>>
>> * Call ACPI _EJ0 for detached devices
>>
>> Signed-off-by: Holger Macht <hmacht@suse.de>
> 
> Acked-by: Matthew Garrett <mjg@redhat.com>

thanks, both!

Re: [PATCH] libata: Handle bay devices in dock stations

[Jeff Garzik]

Holger Macht wrote:
> * 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>)
> 
> * Call ACPI _EJ0 for detached devices
> 
> Signed-off-by: Holger Macht <hmacht@suse.de>
> ---
> 
> Previous patch differences:
>  * Remove one call to ata_port_freeze
>  * Add some documentation from Tejun

applied

Re: [PATCH] libata: Handle bay devices in dock stations

[Tejun Heo]

Matthew Garrett wrote:
> On Tue, Jun 03, 2008 at 02:07:42PM -0400, Jeff Garzik wrote:
> 
>> It would be nice if you and Holger could work together to produce a 
>> single patch[set]...  right now I have patches from both of you, and I 
>> was sorta waiting on the thread to die down to see if competing patches 
>> might merge into a single set
> 
> I think we were waiting for feedback from Tejun as to why removing the 
> port freeze call fixed the hang I was seeing? Beyond that, Holger's 
> latest patch looked good to me.

Sorry, was off for the last week.

The difference between freezing and scheduling EH is that the former 
immediately aborts all in-flight commands and resets the port while the 
latter waits till the in-flight commands finish or time out (EH 
scheduling kicks fast-drain and the timeout is reduced to three seconds).

TF-based ATA controllers are very sensitive to how the registers are 
accessed and sometimes lock up the whole machine when they are not happy 
by indefinitely holding the PCI bus.  This could have been the case if 
IOs were in flight when the dock event occurred.  Were they?

Thanks.

-- 
tejun

Re: [PATCH] libata: Handle bay devices in dock stations

[Matthew Garrett]

On Mon, Jun 09, 2008 at 10:44:01AM +0900, Tejun Heo wrote:

> TF-based ATA controllers are very sensitive to how the registers are 
> accessed and sometimes lock up the whole machine when they are not happy 
> by indefinitely holding the PCI bus.  This could have been the case if 
> IOs were in flight when the dock event occurred.  Were they?

I'd stopped hal, so I can't imagine that userspace was causing any to be 
generated at that point.

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

Re: [PATCH] libata: Handle bay devices in dock stations

[Tejun Heo]

Matthew Garrett wrote:
> On Mon, Jun 09, 2008 at 10:44:01AM +0900, Tejun Heo wrote:
> 
>> TF-based ATA controllers are very sensitive to how the registers are 
>> accessed and sometimes lock up the whole machine when they are not happy 
>> by indefinitely holding the PCI bus.  This could have been the case if 
>> IOs were in flight when the dock event occurred.  Were they?
> 
> I'd stopped hal, so I can't imagine that userspace was causing any to be 
> generated at that point.

Ah... okay.  Stupid me.  libata EH always resets a frozen port to 
un-freeze it even if it's unoccupied to listen for hotplug events.  So, 
if dock notifies device removal after the actual device is gone && the 
port is frozen as a result, libata EH will try to reset the port after 
the device is gone and in this case the controller locks up the whole 
machine for that.  If schedule_eh is used, libata EH just removes the 
device and does nothing else and the controller is happy.

This isn't too safe tho.  There can be other things which can trigger 
port reset.  ie. hotplug request from userland, in-flight IOs at the 
time of dock removal, etc...  Maybe we need to implement a flag to 
indicate that the port is dead and shouldn't be accessed in any way.

Thanks.

-- 
tejun