Messages in this thread | | | Date | Wed, 28 May 2008 20:02:27 -0700 | From | Andrew Morton <> | Subject | Re: [PATCH] libata: Handle bay devices in dock stations |
| |
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.
| |