Re: [Update 3][PATCH 2/7] ACPI / scan: Introduce common code for ACPI-based device hotplug

[Rafael J. Wysocki]

On Friday, February 22, 2013 05:51:28 PM Yasuaki Ishimatsu wrote:
> 2013/02/22 10:50, Rafael J. Wysocki wrote:
> > On Thursday, February 21, 2013 06:12:21 PM Toshi Kani wrote:
> >> On Fri, 2013-02-22 at 00:06 +0100, Rafael J. Wysocki wrote:
> >>> From: Rafael J. Wysocki <rafael.j.wysocki@xxxxxxxxx>
> >>>
> >>> Multiple drivers handling hotplug-capable ACPI device nodes install
> >>> notify handlers covering the same types of events in a very similar
> >>> way.  Moreover, those handlers are installed in separate namespace
> >>> walks, although that really should be done during namespace scans
> >>> carried out by acpi_bus_scan().  This leads to substantial code
> >>> duplication, unnecessary overhead and behavior that is hard to
> >>> follow.
> >>>
> >>> For this reason, introduce common code in drivers/acpi/scan.c for
> >>> handling hotplug-related notification and carrying out device
> >>> insertion and eject operations in a generic fashion, such that it
> >>> may be used by all of the relevant drivers in the future.  To cover
> >>> the existing differences between those drivers introduce struct
> >>> acpi_hotplug_profile for representing collections of hotplug
> >>> settings associated with different ACPI scan handlers that can be
> >>> used by the drivers to make the common code reflect their current
> >>> behavior.
> >>>
> >>> Signed-off-by: Rafael J. Wysocki <rafael.j.wysocki@xxxxxxxxx>
> >>> ---
> >>>
> >>> This update fixes an issue pointed out by Toshi Kani (that
> >>> ACPI_OST_EC_OSPM_* event source codes should not be used with _OST for events
> >>> that we received a notification for from the platform firmware).
> >>>
> >>> Thanks,
> >>> Rafael
> >>>
> >>> ---
> >>>   drivers/acpi/scan.c     |  270 ++++++++++++++++++++++++++++++++++++++----------
> >>>   include/acpi/acpi_bus.h |    7 +
> >>>   2 files changed, 224 insertions(+), 53 deletions(-)
> >>   :
> >>> +static void acpi_bus_device_eject(void *context)
> >>> +{
> >>> +	acpi_handle handle = context;
> >>> +	struct acpi_device *device = NULL;
> >>> +	struct acpi_scan_handler *handler;
> >>> +	u32 ost_code = ACPI_OST_SC_NON_SPECIFIC_FAILURE;
> >>> +
> >>> +	mutex_lock(&acpi_scan_lock);
> >>> +
> >>> +	acpi_bus_get_device(handle, &device);
> >>> +	if (!device)
> >>> +		goto err_out;
> >>> +
> >>> +	handler = device->handler;
> >>> +	if (!handler || !handler->hotplug.enabled) {
> >>> +		ost_code = ACPI_OST_SC_EJECT_NOT_SUPPORTED;
> >>> +		goto err_out;
> >>> +	}
> >>> +	acpi_evaluate_hotplug_ost(handle, ACPI_NOTIFY_EJECT_REQUEST,
> >>> +				  ACPI_OST_SC_EJECT_IN_PROGRESS, NULL);
> >>> +	if (handler->hotplug.autoeject) {
> >>> +		int error;
> >>> +
> >>> +		get_device(&device->dev);
> >>> +		error = acpi_scan_hot_remove(device);
> >>> +		if (error)
> >>> +			goto err_out;
> >>> +	} else {
> >>> +		device->flags.eject_pending = true;
> >>>   	}
> >>> +	if (handler->hotplug.uevents)
> >>> +		kobject_uevent(&device->dev.kobj, KOBJ_OFFLINE);
> >>
> >> I confirmed that the _OST issue with hot-add is fixed.  Here is a new
> >> one.  When autoeject is enabled, it crashes in kobject_uevent() since
> >> the device is no longer valid.
> >
> > Well, this one is more difficult.
> >
> > I can change the ordering so that kobject_uevent() is called before
> > acpi_scan_hot_remove(), but then user space may not know that the device is
> > being removed at the moment (which still may fail).  Still, maybe this is
> > OK, because user space will get KOBJ_REMOVE when the device actually goes
> > away anyway.
> >
> > Or perhaps we can emit KOBJ_OFFLINE before acpi_scan_hot_remove() and if
> > it fails, emit KOBJ_ONLINE?
> 
> How about following patch? My system cannot send EJECT notification.
> So I have not tested this patch.

No, that's not correct, acpi_scan_hot_remove(device) will remove the device
from sysfs, if successful, among other things.

We can't emit uevents for a device that has been, even though the data
structure is still around.

The following are the choices we have, in my opinion:
- Emit KOBJ_OFFLINE before removal.
- Emit KOBJ_OFFLINE before removal and KOBJ_ONLINE afterwards if it fails.
- Do not emit KOBJ_OFFLINE at all with autoeject.

Each of them has some disadvantages, so I'm not sure.  The last one is the
easiest, so I'll probably send another update implementing it.

Thanks,
Rafael


> # Recently when I send a patch, tabs of the patch is changed to spaces often.
> # So I attached the patch.
> 
> ---
> 
> When hotplug.autoeject and uevents are enabled, it crashes in
> kobject_uevent() since the device is no longer valid.
> 
> This patch fixes the problem.
> 
> Reported-by: Toshi Kani <toshi.kani@xxxxxx>
> Signed-off-by: Yasuaki Ishimatsu <isimatu.yasuaki@xxxxxxxxxxxxxx>
> 
> ---
>   drivers/acpi/scan.c |    7 +++----
>   1 file changed, 3 insertions(+), 4 deletions(-)
> 
> Index: linux-pm/drivers/acpi/scan.c
> ===================================================================
> --- linux-pm.orig/drivers/acpi/scan.c	2013-02-22 16:02:07.000000000 +0900
> +++ linux-pm/drivers/acpi/scan.c	2013-02-22 16:06:36.792816699 +0900
> @@ -139,9 +139,6 @@ static int acpi_scan_hot_remove(struct a
>   		"Hot-removing device %s...\n", dev_name(&device->dev)));
>   
>   	acpi_bus_trim(device);
> -	/* Device node has been unregistered. */
> -	put_device(&device->dev);
> -	device = NULL;
>   
>   	if (ACPI_SUCCESS(acpi_get_handle(handle, "_LCK", &not_used))) {
>   		arg_list.count = 1;
> @@ -191,10 +188,10 @@ static void acpi_bus_device_eject(void *
>   	}
>   	acpi_evaluate_hotplug_ost(handle, ACPI_NOTIFY_EJECT_REQUEST,
>   				  ACPI_OST_SC_EJECT_IN_PROGRESS, NULL);
> +	get_device(&device->dev);
>   	if (handler->hotplug.autoeject) {
>   		int error;
>   
> -		get_device(&device->dev);
>   		error = acpi_scan_hot_remove(device);
>   		if (error)
>   			goto err_out;
> @@ -204,6 +201,7 @@ static void acpi_bus_device_eject(void *
>   	if (handler->hotplug.uevents)
>   		kobject_uevent(&device->dev.kobj, KOBJ_OFFLINE);
>   
> +	put_device(&device->dev);
>    out:
>   	mutex_unlock(&acpi_scan_lock);
>   	return;
> @@ -312,6 +310,7 @@ void acpi_bus_hot_remove_device(void *co
>   					  ACPI_OST_SC_NON_SPECIFIC_FAILURE,
>   					  NULL);
>   
> +	put_device(&acpi_device->dev);
>   	mutex_unlock(&acpi_scan_lock);
>   	kfree(context);
>   }
> 
-- 
I speak only for myself.
Rafael J. Wysocki, Intel Open Source Technology Center.
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/