Re: [PATCH] Fixups to ATA ACPI hotplug

[Andrew Morton <akpm@linux-foundation.org>]

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