[PATCH 6/8] libata hotplug to align with dock driver

[PATCH 6/8] libata hotplug to align with dock driver

[Shaohua Li]

dock driver can handle ata(bay) hotplug now. dock driver already handles
_EJ0 and _STA, so remove them. Also libata doesn't need register
notification handler anymore. 

Signed-off-by: Shaohua Li <shaohua.li@intel.com>
---
 drivers/acpi/dock.c       |    2 -
 drivers/ata/libata-acpi.c |   57 +++++++++++++---------------------------------
 2 files changed, 18 insertions(+), 41 deletions(-)

Index: linux/drivers/ata/libata-acpi.c
===================================================================
--- linux.orig/drivers/ata/libata-acpi.c	2008-05-22 10:54:32.000000000 +0800
+++ linux/drivers/ata/libata-acpi.c	2008-05-22 14:09:13.000000000 +0800
@@ -127,9 +127,7 @@ static void ata_acpi_handle_hotplug(stru
 	struct kobject *kobj = NULL;
 	int wait = 0;
 	unsigned long flags;
-	acpi_handle handle, tmphandle;
-	unsigned long sta;
-	acpi_status status;
+	acpi_handle handle;
 
 	if (!ap)
 		ap = dev->link->ap;
@@ -142,45 +140,30 @@ static void ata_acpi_handle_hotplug(stru
 	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");
-		if (!sta) {
+		ata_ehi_hotplugged(ehi);
+		ata_port_freeze(ap);
+		break;
+	case ACPI_NOTIFY_EJECT_REQUEST:
+		ata_ehi_push_desc(ehi, "ACPI event");
                 /* 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;
-					}
+		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;
 	}
 
 	spin_unlock_irqrestore(ap->lock, flags);
@@ -247,9 +230,6 @@ void ata_acpi_associate(struct ata_host 
 			ata_acpi_associate_ide_port(ap);
 
 		if (ap->acpi_handle) {
-			acpi_install_notify_handler(ap->acpi_handle,
-						    ACPI_SYSTEM_NOTIFY,
-						    ata_acpi_ap_notify, ap);
 			/* we might be on a docking station */
 			register_hotplug_dock_device(ap->acpi_handle,
 						     ata_acpi_ap_notify, ap);
@@ -259,9 +239,6 @@ void ata_acpi_associate(struct ata_host 
 			struct ata_device *dev = &ap->link.device[j];
 
 			if (dev->acpi_handle) {
-				acpi_install_notify_handler(dev->acpi_handle,
-						ACPI_SYSTEM_NOTIFY,
-						ata_acpi_dev_notify, dev);
 				/* we might be on a docking station */
 				register_hotplug_dock_device(dev->acpi_handle,
 						ata_acpi_dev_notify, dev);
Index: linux/drivers/acpi/dock.c
===================================================================
--- linux.orig/drivers/acpi/dock.c	2008-05-22 10:58:08.000000000 +0800
+++ linux/drivers/acpi/dock.c	2008-05-22 14:08:37.000000000 +0800
@@ -717,7 +717,7 @@ static void dock_notify(acpi_handle hand
 	case ACPI_NOTIFY_EJECT_REQUEST:
 		dock_lock(ds, 0);
 		begin_undock(ds);
-		if (immediate_undock)
+		if (immediate_undock && !(ds->flags & DOCK_IS_ATA))
 			handle_eject_request(ds, event);
 		else
 			dock_event(ds, event, UNDOCK_EVENT);

[PATCH 6/8] libata hotplug to align with dock driver

[Shaohua Li]

dock driver can handle ata(bay) hotplug now. dock driver already handles
_EJ0 and _STA, so remove them. Also libata doesn't need register
notification handler anymore. 

Signed-off-by: Shaohua Li <shaohua.li@intel.com> 
---
 drivers/acpi/dock.c       |    3 -
 drivers/ata/libata-acpi.c |  105 ++++++----------------------------------------
 2 files changed, 16 insertions(+), 92 deletions(-)

Index: linux/drivers/acpi/dock.c
===================================================================
--- linux.orig/drivers/acpi/dock.c	2008-06-06 10:58:05.000000000 +0800
+++ linux/drivers/acpi/dock.c	2008-06-06 10:58:12.000000000 +0800
@@ -735,7 +735,8 @@ static void dock_notify(acpi_handle hand
 		/* Fall back */
 	case ACPI_NOTIFY_EJECT_REQUEST:
 		begin_undock(ds);
-		if (immediate_undock || surprise_removal)
+		if ((immediate_undock && !(ds->flags & DOCK_IS_ATA))
+		   || surprise_removal)
 			handle_eject_request(ds, event);
 		else
 			dock_event(ds, event, UNDOCK_EVENT);
Index: linux/drivers/ata/libata-acpi.c
===================================================================
--- linux.orig/drivers/ata/libata-acpi.c	2008-06-06 10:55:28.000000000 +0800
+++ linux/drivers/ata/libata-acpi.c	2008-06-06 10:58:12.000000000 +0800
@@ -118,21 +118,6 @@ static void ata_acpi_associate_ide_port(
 		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");
-}
-
 /* @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)
 {
@@ -155,7 +140,6 @@ static void ata_acpi_detach_device(struc
  * @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
@@ -169,112 +153,57 @@ static void ata_acpi_detach_device(struc
  * 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)
+				    u32 event)
 {
-	char event_string[12];
-	char *envp[] = { event_string, NULL };
 	struct ata_eh_info *ehi = &ap->link.eh_info;
-	struct kobject *kobj = NULL;
 	int wait = 0;
 	unsigned long flags;
-	acpi_handle handle, tmphandle;
-	unsigned long sta;
-	acpi_status status;
+	acpi_handle handle;
 
-	if (dev) {
-		if (dev->sdev)
-			kobj = &dev->sdev->sdev_gendev.kobj;
+	if (dev)
 		handle = dev->acpi_handle;
-	} else {
-		kobj = &ap->dev->kobj;
+	else
 		handle = ap->acpi_handle;
-	}
-
-	status = acpi_get_handle(handle, "_EJ0", &tmphandle);
-	if (ACPI_FAILURE(status))
-		/* This device does not support hotplug */
-		return;
 
 	spin_lock_irqsave(ap->lock, flags);
 
+	/*
+	 * When dock driver calls into the routine, it will always use
+	 * ACPI_NOTIFY_BUS_CHECK/ACPI_NOTIFY_DEVICE_CHECK for add and
+	 * ACPI_NOTIFY_EJECT_REQUEST for remove
+	 */
 	switch (event) {
 	case ACPI_NOTIFY_BUS_CHECK:
 	case ACPI_NOTIFY_DEVICE_CHECK:
 		ata_ehi_push_desc(ehi, "ACPI event");
-
-		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;
-		}
+		ata_ehi_hotplugged(ehi);
+		ata_port_freeze(ap);
 		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 (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);
+	ata_acpi_handle_hotplug(dev->link->ap, dev, event);
 }
 
 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(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, 0);
+	ata_acpi_handle_hotplug(ap, NULL, event);
 }
 
 /**
@@ -310,9 +239,6 @@ void ata_acpi_associate(struct ata_host 
 			ata_acpi_associate_ide_port(ap);
 
 		if (ap->acpi_handle) {
-			acpi_install_notify_handler(ap->acpi_handle,
-						    ACPI_SYSTEM_NOTIFY,
-						    ata_acpi_ap_notify, ap);
 			/* we might be on a docking station */
 			register_hotplug_dock_device(ap->acpi_handle,
 					     ata_acpi_ap_notify_dock, ap);
@@ -322,9 +248,6 @@ void ata_acpi_associate(struct ata_host 
 			struct ata_device *dev = &ap->link.device[j];
 
 			if (dev->acpi_handle) {
-				acpi_install_notify_handler(dev->acpi_handle,
-						ACPI_SYSTEM_NOTIFY,
-						ata_acpi_dev_notify, dev);
 				/* we might be on a docking station */
 				register_hotplug_dock_device(dev->acpi_handle,
 					     ata_acpi_dev_notify_dock, dev);


--
To unsubscribe from this list: send the line "unsubscribe linux-acpi" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH 6/8] libata hotplug to align with dock driver

[Matthew Garrett]

On Fri, Jun 06, 2008 at 01:23:08PM +0800, Shaohua Li wrote:

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

Ok, just to check that I've understood the other patches - this will 
only be called if the device has actually been removed, and not if you 
merely get an EJECT_REQUEST, right? An EJECT_REQUEST from a bay device 
should always just signal userspace, and never actually cause the device 
to be deleted. I don't really like the way that you're remapping event 
types inside the dock driver - it'd be cleaner if the per-driver 
handlers received ADD_DEVICE or REMOVE_DEVICE or something.

Other than that, this looks basically fine. I'll try to test it in the 
next couple of days.

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

Re: [PATCH 6/8] libata hotplug to align with dock driver

[Henrique de Moraes Holschuh]

On Mon, 9 Jun 2008 02:37:19 +0100, "Matthew Garrett" <mjg59@srcf.ucam.org> said:
> On Fri, Jun 06, 2008 at 01:23:08PM +0800, Shaohua Li wrote:
> 
> >  	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);
> 
> Ok, just to check that I've understood the other patches - this will 
> only be called if the device has actually been removed, and not if you 
> merely get an EJECT_REQUEST, right? An EJECT_REQUEST from a bay device 
> should always just signal userspace, and never actually cause the device 
> to be deleted. I don't really like the way that you're remapping event 

That's my doubt as well.   We can't have unconditional ejects of either
docks or bays on hardware that allows you to refuse to eject if something
is wrong, that choice belongs to USERSPACE, so the ejects must not be
unconditional.

-- 
  "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 6/8] libata hotplug to align with dock driver

[Shaohua Li]

On Mon, 2008-06-09 at 02:37 +0100, Matthew Garrett wrote:
> On Fri, Jun 06, 2008 at 01:23:08PM +0800, Shaohua Li wrote:
> 
> >  	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);
> 
> Ok, just to check that I've understood the other patches - this will 
> only be called if the device has actually been removed, and not if you 
> merely get an EJECT_REQUEST, right? An EJECT_REQUEST from a bay device 
> should always just signal userspace, and never actually cause the device 
> to be deleted. I don't really like the way that you're remapping event 
> types inside the dock driver - it'd be cleaner if the per-driver 
> handlers received ADD_DEVICE or REMOVE_DEVICE or something.
It's not forcely ejected. This patch adds a check in dock.c, if eject
request is for an ata bay, we just signal userspace. Latter if a
device/bus check invoked, dock driver will check status, and doing force
eject. At this time, ata's EJECT_REQUEST handler will be called.
Remapping event is to make per-driver handlers easy. complex is all in
dock driver.

Thanks,
Shaohua

Re: [PATCH 6/8] libata hotplug to align with dock driver

[Holger Macht]

On Tue 10. Jun - 09:34:36, Shaohua Li wrote:
> On Mon, 2008-06-09 at 02:37 +0100, Matthew Garrett wrote:
> > On Fri, Jun 06, 2008 at 01:23:08PM +0800, Shaohua Li wrote:
> > 
> > >  	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);
> > 
> > Ok, just to check that I've understood the other patches - this will 
> > only be called if the device has actually been removed, and not if you 
> > merely get an EJECT_REQUEST, right? An EJECT_REQUEST from a bay device 
> > should always just signal userspace, and never actually cause the device 
> > to be deleted. I don't really like the way that you're remapping event 
> > types inside the dock driver - it'd be cleaner if the per-driver 
> > handlers received ADD_DEVICE or REMOVE_DEVICE or something.
> It's not forcely ejected. This patch adds a check in dock.c, if eject
> request is for an ata bay, we just signal userspace. Latter if a
> device/bus check invoked, dock driver will check status, and doing force
> eject. At this time, ata's EJECT_REQUEST handler will be called.
> Remapping event is to make per-driver handlers easy. complex is all in
> dock driver.

I still don't get a uevent signalled to userspace for a bay device in my
dock station when pressing the lever on the bay device. I think I'll get
to debug this by the end of the week or the weekend if you like...

Regards,
	Holger

Re: [PATCH 6/8] libata hotplug to align with dock driver

[Shaohua Li]

On Tue, 2008-06-10 at 10:15 +0200, Holger Macht wrote:
> On Tue 10. Jun - 09:34:36, Shaohua Li wrote:
> > On Mon, 2008-06-09 at 02:37 +0100, Matthew Garrett wrote:
> > > On Fri, Jun 06, 2008 at 01:23:08PM +0800, Shaohua Li wrote:
> > > 
> > > >  	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);
> > > 
> > > Ok, just to check that I've understood the other patches - this will 
> > > only be called if the device has actually been removed, and not if you 
> > > merely get an EJECT_REQUEST, right? An EJECT_REQUEST from a bay device 
> > > should always just signal userspace, and never actually cause the device 
> > > to be deleted. I don't really like the way that you're remapping event 
> > > types inside the dock driver - it'd be cleaner if the per-driver 
> > > handlers received ADD_DEVICE or REMOVE_DEVICE or something.
> > It's not forcely ejected. This patch adds a check in dock.c, if eject
> > request is for an ata bay, we just signal userspace. Latter if a
> > device/bus check invoked, dock driver will check status, and doing force
> > eject. At this time, ata's EJECT_REQUEST handler will be called.
> > Remapping event is to make per-driver handlers easy. complex is all in
> > dock driver.
> 
> I still don't get a uevent signalled to userspace for a bay device in my
> dock station when pressing the lever on the bay device. I think I'll get
> to debug this by the end of the week or the weekend if you like...
can you send me the acpidump output, so I can have a quick check first?

Thanks,
Shaohua

Re: [PATCH 6/8] libata hotplug to align with dock driver

[Holger Macht]

[-- Attachment #1: Type: text/plain, Size: 1839 bytes --]

On Tue 10. Jun - 16:27:32, Shaohua Li wrote:
> On Tue, 2008-06-10 at 10:15 +0200, Holger Macht wrote:
> > On Tue 10. Jun - 09:34:36, Shaohua Li wrote:
> > > On Mon, 2008-06-09 at 02:37 +0100, Matthew Garrett wrote:
> > > > On Fri, Jun 06, 2008 at 01:23:08PM +0800, Shaohua Li wrote:
> > > > 
> > > > >  	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);
> > > > 
> > > > Ok, just to check that I've understood the other patches - this will 
> > > > only be called if the device has actually been removed, and not if you 
> > > > merely get an EJECT_REQUEST, right? An EJECT_REQUEST from a bay device 
> > > > should always just signal userspace, and never actually cause the device 
> > > > to be deleted. I don't really like the way that you're remapping event 
> > > > types inside the dock driver - it'd be cleaner if the per-driver 
> > > > handlers received ADD_DEVICE or REMOVE_DEVICE or something.
> > > It's not forcely ejected. This patch adds a check in dock.c, if eject
> > > request is for an ata bay, we just signal userspace. Latter if a
> > > device/bus check invoked, dock driver will check status, and doing force
> > > eject. At this time, ata's EJECT_REQUEST handler will be called.
> > > Remapping event is to make per-driver handlers easy. complex is all in
> > > dock driver.
> > 
> > I still don't get a uevent signalled to userspace for a bay device in my
> > dock station when pressing the lever on the bay device. I think I'll get
> > to debug this by the end of the week or the weekend if you like...
> can you send me the acpidump output, so I can have a quick check first?

Attached.

Regards,
	Holger

[-- Attachment #2: acpidump-thinkpad-x60.tar.gz --]
[-- Type: application/x-compressed-tar, Size: 71988 bytes --]

Re: [PATCH 6/8] libata hotplug to align with dock driver

[Shaohua Li]

On Tue, 2008-06-10 at 18:43 +0200, Holger Macht wrote:
> On Tue 10. Jun - 16:27:32, Shaohua Li wrote:
> > On Tue, 2008-06-10 at 10:15 +0200, Holger Macht wrote:
> > > On Tue 10. Jun - 09:34:36, Shaohua Li wrote:
> > > > On Mon, 2008-06-09 at 02:37 +0100, Matthew Garrett wrote:
> > > > > On Fri, Jun 06, 2008 at 01:23:08PM +0800, Shaohua Li wrote:
> > > > > 
> > > > > >  	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);
> > > > > 
> > > > > Ok, just to check that I've understood the other patches - this will 
> > > > > only be called if the device has actually been removed, and not if you 
> > > > > merely get an EJECT_REQUEST, right? An EJECT_REQUEST from a bay device 
> > > > > should always just signal userspace, and never actually cause the device 
> > > > > to be deleted. I don't really like the way that you're remapping event 
> > > > > types inside the dock driver - it'd be cleaner if the per-driver 
> > > > > handlers received ADD_DEVICE or REMOVE_DEVICE or something.
> > > > It's not forcely ejected. This patch adds a check in dock.c, if eject
> > > > request is for an ata bay, we just signal userspace. Latter if a
> > > > device/bus check invoked, dock driver will check status, and doing force
> > > > eject. At this time, ata's EJECT_REQUEST handler will be called.
> > > > Remapping event is to make per-driver handlers easy. complex is all in
> > > > dock driver.
> > > 
> > > I still don't get a uevent signalled to userspace for a bay device in my
> > > dock station when pressing the lever on the bay device. I think I'll get
> > > to debug this by the end of the week or the weekend if you like...
> > can you send me the acpidump output, so I can have a quick check first?
Looks my patch didn't handle bay device in a dock station correctly. Can
you please try below patch?

bay device can be in a dock, and itself can be ejected separately.
Handle such device correctly in dock driver.

Signed-off-by: Shaohua Li <shaohua.li@intel.com>
---
 drivers/acpi/dock.c |   14 ++++++++++----
 1 file changed, 10 insertions(+), 4 deletions(-)

Index: linux/drivers/acpi/dock.c
===================================================================
--- linux.orig/drivers/acpi/dock.c	2008-06-11 11:08:56.000000000 +0800
+++ linux/drivers/acpi/dock.c	2008-06-11 11:09:14.000000000 +0800
@@ -602,6 +602,7 @@ register_hotplug_dock_device(acpi_handle
 {
 	struct dock_dependent_device *dd;
 	struct dock_station *dock_station;
+	int ret = -EINVAL;
 
 	if (!dock_station_count)
 		return -ENODEV;
@@ -611,16 +612,21 @@ register_hotplug_dock_device(acpi_handle
 	 * this would include the dock station itself
 	 */
 	list_for_each_entry(dock_station, &dock_stations, sibiling) {
+		/*
+		 * An ATA bay can be in a dock and itself can be ejected
+		 * separately, so there are two 'dock stations' which need the
+		 * ops
+		 */
 		dd = find_dock_dependent_device(dock_station, handle);
 		if (dd) {
 			dd->ops = ops;
 			dd->context = context;
 			dock_add_hotplug_device(dock_station, dd);
-			return 0;
+			ret = 0;
 		}
 	}
 
-	return -EINVAL;
+	return ret;
 }
 
 EXPORT_SYMBOL_GPL(register_hotplug_dock_device);
@@ -1064,8 +1070,8 @@ find_dock(acpi_handle handle, u32 lvl, v
 static acpi_status
 find_bay(acpi_handle handle, u32 lvl, void *context, void **rv)
 {
-	/* If bay is in a dock, it's already handled */
-	if (is_ejectable_bay(handle) && !is_dock_device(handle))
+	/* If bay is a dock, it's already handled */
+	if (is_ejectable_bay(handle) && !is_dock(handle))
 		dock_add(handle);
 	return AE_OK;
 }

Re: [PATCH 6/8] libata hotplug to align with dock driver

[Holger Macht]

On Wed 11. Jun - 11:23:41, Shaohua Li wrote:
> On Tue, 2008-06-10 at 18:43 +0200, Holger Macht wrote:
> > On Tue 10. Jun - 16:27:32, Shaohua Li wrote:
> > > On Tue, 2008-06-10 at 10:15 +0200, Holger Macht wrote:
> > > > On Tue 10. Jun - 09:34:36, Shaohua Li wrote:
> > > > > On Mon, 2008-06-09 at 02:37 +0100, Matthew Garrett wrote:
> > > > > > On Fri, Jun 06, 2008 at 01:23:08PM +0800, Shaohua Li wrote:
> > > > > > 
> > > > > > >  	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);
> > > > > > 
> > > > > > Ok, just to check that I've understood the other patches - this will 
> > > > > > only be called if the device has actually been removed, and not if you 
> > > > > > merely get an EJECT_REQUEST, right? An EJECT_REQUEST from a bay device 
> > > > > > should always just signal userspace, and never actually cause the device 
> > > > > > to be deleted. I don't really like the way that you're remapping event 
> > > > > > types inside the dock driver - it'd be cleaner if the per-driver 
> > > > > > handlers received ADD_DEVICE or REMOVE_DEVICE or something.
> > > > > It's not forcely ejected. This patch adds a check in dock.c, if eject
> > > > > request is for an ata bay, we just signal userspace. Latter if a
> > > > > device/bus check invoked, dock driver will check status, and doing force
> > > > > eject. At this time, ata's EJECT_REQUEST handler will be called.
> > > > > Remapping event is to make per-driver handlers easy. complex is all in
> > > > > dock driver.
> > > > 
> > > > I still don't get a uevent signalled to userspace for a bay device in my
> > > > dock station when pressing the lever on the bay device. I think I'll get
> > > > to debug this by the end of the week or the weekend if you like...
> > > can you send me the acpidump output, so I can have a quick check first?
> Looks my patch didn't handle bay device in a dock station correctly. Can
> you please try below patch?

Now I get the uevent, however, when trying with init=/bin/bash, the device
never comes back once ejected.

Furthermore, the must be a race somewhere, when docking, the system
freezes sporadically. First dock worked, second time didn't.

Regards,
	Holger

Re: [PATCH 6/8] libata hotplug to align with dock driver

[Shaohua Li]

On Wed, 2008-06-11 at 14:52 +0200, Holger Macht wrote:
> On Wed 11. Jun - 11:23:41, Shaohua Li wrote:
> > On Tue, 2008-06-10 at 18:43 +0200, Holger Macht wrote:
> > > On Tue 10. Jun - 16:27:32, Shaohua Li wrote:
> > > > On Tue, 2008-06-10 at 10:15 +0200, Holger Macht wrote:
> > > > > On Tue 10. Jun - 09:34:36, Shaohua Li wrote:
> > > > > > On Mon, 2008-06-09 at 02:37 +0100, Matthew Garrett wrote:
> > > > > > > On Fri, Jun 06, 2008 at 01:23:08PM +0800, Shaohua Li wrote:
> > > > > > > 
> > > > > > > >  	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);
> > > > > > > 
> > > > > > > Ok, just to check that I've understood the other patches - this will 
> > > > > > > only be called if the device has actually been removed, and not if you 
> > > > > > > merely get an EJECT_REQUEST, right? An EJECT_REQUEST from a bay device 
> > > > > > > should always just signal userspace, and never actually cause the device 
> > > > > > > to be deleted. I don't really like the way that you're remapping event 
> > > > > > > types inside the dock driver - it'd be cleaner if the per-driver 
> > > > > > > handlers received ADD_DEVICE or REMOVE_DEVICE or something.
> > > > > > It's not forcely ejected. This patch adds a check in dock.c, if eject
> > > > > > request is for an ata bay, we just signal userspace. Latter if a
> > > > > > device/bus check invoked, dock driver will check status, and doing force
> > > > > > eject. At this time, ata's EJECT_REQUEST handler will be called.
> > > > > > Remapping event is to make per-driver handlers easy. complex is all in
> > > > > > dock driver.
> > > > > 
> > > > > I still don't get a uevent signalled to userspace for a bay device in my
> > > > > dock station when pressing the lever on the bay device. I think I'll get
> > > > > to debug this by the end of the week or the weekend if you like...
> > > > can you send me the acpidump output, so I can have a quick check first?
> > Looks my patch didn't handle bay device in a dock station correctly. Can
> > you please try below patch?
> 
> Now I get the uevent, however, when trying with init=/bin/bash, the device
> never comes back once ejected.
> 
> Furthermore, the must be a race somewhere, when docking, the system
> freezes sporadically. First dock worked, second time didn't.
Double checked a T61 here, can't reproduce, please help do some
debug. Does this work without my patches?

Thanks,
Shaohua

--
To unsubscribe from this list: send the line "unsubscribe linux-acpi" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH 6/8] libata hotplug to align with dock driver

[Holger Macht]

On Thu 12. Jun - 09:23:48, Shaohua Li wrote:
> On Wed, 2008-06-11 at 14:52 +0200, Holger Macht wrote:
> > On Wed 11. Jun - 11:23:41, Shaohua Li wrote:
> > > On Tue, 2008-06-10 at 18:43 +0200, Holger Macht wrote:
> > > > On Tue 10. Jun - 16:27:32, Shaohua Li wrote:
> > > > > On Tue, 2008-06-10 at 10:15 +0200, Holger Macht wrote:
> > > > > > On Tue 10. Jun - 09:34:36, Shaohua Li wrote:
> > > > > > > On Mon, 2008-06-09 at 02:37 +0100, Matthew Garrett wrote:
> > > > > > > > On Fri, Jun 06, 2008 at 01:23:08PM +0800, Shaohua Li wrote:
> > > > > > > > 
> > > > > > > > >  	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);
> > > > > > > > 
> > > > > > > > Ok, just to check that I've understood the other patches - this will 
> > > > > > > > only be called if the device has actually been removed, and not if you 
> > > > > > > > merely get an EJECT_REQUEST, right? An EJECT_REQUEST from a bay device 
> > > > > > > > should always just signal userspace, and never actually cause the device 
> > > > > > > > to be deleted. I don't really like the way that you're remapping event 
> > > > > > > > types inside the dock driver - it'd be cleaner if the per-driver 
> > > > > > > > handlers received ADD_DEVICE or REMOVE_DEVICE or something.
> > > > > > > It's not forcely ejected. This patch adds a check in dock.c, if eject
> > > > > > > request is for an ata bay, we just signal userspace. Latter if a
> > > > > > > device/bus check invoked, dock driver will check status, and doing force
> > > > > > > eject. At this time, ata's EJECT_REQUEST handler will be called.
> > > > > > > Remapping event is to make per-driver handlers easy. complex is all in
> > > > > > > dock driver.
> > > > > > 
> > > > > > I still don't get a uevent signalled to userspace for a bay device in my
> > > > > > dock station when pressing the lever on the bay device. I think I'll get
> > > > > > to debug this by the end of the week or the weekend if you like...
> > > > > can you send me the acpidump output, so I can have a quick check first?
> > > Looks my patch didn't handle bay device in a dock station correctly. Can
> > > you please try below patch?
> > 
> > Now I get the uevent, however, when trying with init=/bin/bash, the device
> > never comes back once ejected.
> > 
> > Furthermore, the must be a race somewhere, when docking, the system
> > freezes sporadically. First dock worked, second time didn't.
> Double checked a T61 here, can't reproduce, please help do some
> debug. Does this work without my patches?

Yes, it does.

I think the main difference between those devices in the system and those
in the dock station itself, is that for the separate device in the dock,
no ACPI_DEVICE_CHECK is emitted when the device is replugged.

After an APCI_NOTIFY_EJECT_REQUEST, if I do
'echo 1 > /sys/devices/platform/dock.2/undock',
the device is properly ejected and comes back when replugging.  When doing
'echo 1 > /sys/class/scsi_device/.../delete', or doing nothing
the device never comes back (except undocking/redocking).

Further debugging when I find some time...

Regards,
	Holger
--
To unsubscribe from this list: send the line "unsubscribe linux-acpi" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH 6/8] libata hotplug to align with dock driver

[Shaohua Li]

On Mon, 2008-06-16 at 14:49 +0200, Holger Macht wrote:
> On Thu 12. Jun - 09:23:48, Shaohua Li wrote:
> > On Wed, 2008-06-11 at 14:52 +0200, Holger Macht wrote:
> > > On Wed 11. Jun - 11:23:41, Shaohua Li wrote:
> > > > On Tue, 2008-06-10 at 18:43 +0200, Holger Macht wrote:
> > > > > On Tue 10. Jun - 16:27:32, Shaohua Li wrote:
> > > > > > On Tue, 2008-06-10 at 10:15 +0200, Holger Macht wrote:
> > > > > > > On Tue 10. Jun - 09:34:36, Shaohua Li wrote:
> > > > > > > > On Mon, 2008-06-09 at 02:37 +0100, Matthew Garrett wrote:
> > > > > > > > > On Fri, Jun 06, 2008 at 01:23:08PM +0800, Shaohua Li wrote:
> > > > > > > > > 
> > > > > > > > > >  	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);
> > > > > > > > > 
> > > > > > > > > Ok, just to check that I've understood the other patches - this will 
> > > > > > > > > only be called if the device has actually been removed, and not if you 
> > > > > > > > > merely get an EJECT_REQUEST, right? An EJECT_REQUEST from a bay device 
> > > > > > > > > should always just signal userspace, and never actually cause the device 
> > > > > > > > > to be deleted. I don't really like the way that you're remapping event 
> > > > > > > > > types inside the dock driver - it'd be cleaner if the per-driver 
> > > > > > > > > handlers received ADD_DEVICE or REMOVE_DEVICE or something.
> > > > > > > > It's not forcely ejected. This patch adds a check in dock.c, if eject
> > > > > > > > request is for an ata bay, we just signal userspace. Latter if a
> > > > > > > > device/bus check invoked, dock driver will check status, and doing force
> > > > > > > > eject. At this time, ata's EJECT_REQUEST handler will be called.
> > > > > > > > Remapping event is to make per-driver handlers easy. complex is all in
> > > > > > > > dock driver.
> > > > > > > 
> > > > > > > I still don't get a uevent signalled to userspace for a bay device in my
> > > > > > > dock station when pressing the lever on the bay device. I think I'll get
> > > > > > > to debug this by the end of the week or the weekend if you like...
> > > > > > can you send me the acpidump output, so I can have a quick check first?
> > > > Looks my patch didn't handle bay device in a dock station correctly. Can
> > > > you please try below patch?
> > > 
> > > Now I get the uevent, however, when trying with init=/bin/bash, the device
> > > never comes back once ejected.
> > > 
> > > Furthermore, the must be a race somewhere, when docking, the system
> > > freezes sporadically. First dock worked, second time didn't.
> > Double checked a T61 here, can't reproduce, please help do some
> > debug. Does this work without my patches?
> 
> Yes, it does.
> 
> I think the main difference between those devices in the system and those
> in the dock station itself, is that for the separate device in the dock,
> no ACPI_DEVICE_CHECK is emitted when the device is replugged.
> 
> After an APCI_NOTIFY_EJECT_REQUEST, if I do
> 'echo 1 > /sys/devices/platform/dock.2/undock',
> the device is properly ejected and comes back when replugging.  When doing
> 'echo 1 > /sys/class/scsi_device/.../delete', or doing nothing
> the device never comes back (except undocking/redocking).
I had some discussions with Holger on the issue. The problem is
ACPI_DEVICE_CHECK event is missed in the system, so dock driver didn't
receive any event when the bay is actually removed. The solution is
easy, userspace should do 'echo 1 > /sys/devices/platform/dock.*/undock'
when NOTIFY_EJECT_REQUEST is received. For system which can send
DEVICE_CHECK like T61, this isn't required, but not harmful. So in any
case, userspace should write the 'undock' file.

'echo 1 > /sys/class/.../delete' just remove the bay from scsi layer.
the device isn't removed from ACPI layer. the device is still present
from ACPI point of view and in Holger's case _EJ0 isn't executed. This
is insane though it works before. For ACPI controlled bay, we should
take the ACPI way, that is to write 'undock' file.

Thanks,
Shaohua

--
To unsubscribe from this list: send the line "unsubscribe linux-acpi" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html