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

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Date:	Wed, 28 May 2008 20:02:27 -0700
From:	Andrew Morton <akpm@...ux-foundation.org>
To:	Holger Macht <hmacht@...e.de>
Cc:	linux-kernel@...r.kernel.org, Tejun Heo <htejun@...il.com>,
	linux-ide@...r.kernel.org, Jeff Garzik <jeff@...zik.org>,
	linux-acpi@...r.kernel.org, Matthew Garrett <mjg59@...f.ucam.org>
Subject: Re: [PATCH] libata: Handle bay devices in dock stations

On Wed, 28 May 2008 16:38:57 +0200 Holger Macht <hmacht@...e.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@...f.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.


--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/