| Message ID | 51273190.7030106@jp.fujitsu.com (mailing list archive) |
|---|---|
| State | RFC, archived |
| Headers | show |
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@intel.com> > >>> > >>> 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@intel.com> > >>> --- > >>> > >>> 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@hp.com> > Signed-off-by: Yasuaki Ishimatsu <isimatu.yasuaki@jp.fujitsu.com> > > --- > 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", ¬_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); > } >
On Fri, 2013-02-22 at 13:37 +0100, Rafael J. Wysocki wrote: > 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@intel.com> > > >>> > > >>> 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@intel.com> > > >>> --- > > >>> > > >>> 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. I agree with the 3rd option. KOBJ_REMOVE is emitted when a device is removed, so it should be OK to not emitting KOBJ_OFFLINE here. Besides, we are going to rely on a target offlined beforehand, so this code path won't have to do it anyway. BTW, it appears that KOBJ_OFFLINE is used for two different purposes today. sysfs cpu/memory emits KOBJ_OFFLINE when a target has been offlined. The container driver (or autoeject is not set with this patch) emits KOBJ_OFFLINE to request a user to offline a target. Expected action from user space is different from these cases. Thanks, -Toshi -- To unsubscribe from this list: send the line "unsubscribe linux-acpi" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Friday, February 22, 2013 08:54:14 AM Toshi Kani wrote: > On Fri, 2013-02-22 at 13:37 +0100, Rafael J. Wysocki wrote: > > 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@intel.com> > > > >>> > > > >>> 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@intel.com> > > > >>> --- > > > >>> > > > >>> 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. > > I agree with the 3rd option. KOBJ_REMOVE is emitted when a device is > removed, so it should be OK to not emitting KOBJ_OFFLINE here. Besides, > we are going to rely on a target offlined beforehand, so this code path > won't have to do it anyway. Yeah. OK, I'll update the patch this way, thanks! > BTW, it appears that KOBJ_OFFLINE is used for two different purposes > today. sysfs cpu/memory emits KOBJ_OFFLINE when a target has been > offlined. The container driver (or autoeject is not set with this > patch) emits KOBJ_OFFLINE to request a user to offline a target. > Expected action from user space is different from these cases. What a mess. Well, we'll need to resolve it somehow, probably by special casing the processor driver, but let's do that later. Thanks, Rafael
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@hp.com> Signed-off-by: Yasuaki Ishimatsu <isimatu.yasuaki@jp.fujitsu.com> --- 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", ¬_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); }