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

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

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.