| Message ID | 5195965.AgE9G8aXP3@vostro.rjw.lan |
|---|---|
| State | Not Applicable |
| Headers | show |
On Wed, 2013-11-06 at 00:27 +0100, Rafael J. Wysocki wrote: > From: Rafael J. Wysocki <rafael.j.wysocki@intel.com> > > In theory, an ACPI device object may be the parent of another > device object whose hotplug is disabled by user space through its > scan handler. In that case, the eject operation targeting the > parent should fail as though the parent's own hotplug was disabled, > but currently this is not the case, because acpi_scan_hot_remove() > doesn't check the disable/enable hotplug status of the children > of the top-most object passed to it. > > To fix this, modify acpi_bus_offline_companions() to return an > error code if hotplug is disabled for the given device object. > [Also change the name of the function to acpi_bus_offline(), > because it is not only about companions any more, and change > the name of acpi_bus_online_companions() accordingly.] Make > acpi_scan_hot_remove() propagate that error to its callers. > : > +static acpi_status acpi_bus_online(acpi_handle handle, u32 lvl, void *data, > + void **ret_p) > { > struct acpi_device *device = NULL; > struct acpi_device_physical_node *pn; > @@ -214,26 +220,32 @@ static int acpi_scan_hot_remove(struct a > * If the first pass is successful, the second one isn't needed, though. > */ > errdev = NULL; > - acpi_walk_namespace(ACPI_TYPE_ANY, handle, ACPI_UINT32_MAX, > - NULL, acpi_bus_offline_companions, > - (void *)false, (void **)&errdev); > - acpi_bus_offline_companions(handle, 0, (void *)false, (void **)&errdev); > + status = acpi_walk_namespace(ACPI_TYPE_ANY, handle, ACPI_UINT32_MAX, > + NULL, acpi_bus_offline, (void *)false, > + (void **)&errdev); > + if (status == AE_SUPPORT) { > + dev_warn(errdev, "Offline disabled.\n"); > + acpi_walk_namespace(ACPI_TYPE_ANY, handle, ACPI_UINT32_MAX, > + acpi_bus_online, NULL, NULL, NULL); > + put_device(&device->dev); > + return -EPERM; > + } > + acpi_bus_offline(handle, 0, (void *)false, (void **)&errdev); > if (errdev) { If the target object failed with AE_SUPPORT, shouldn't we skip the 2nd pass and return with -EPERM after rollback? Thanks, -Toshi > errdev = NULL; > acpi_walk_namespace(ACPI_TYPE_ANY, handle, ACPI_UINT32_MAX, > - NULL, acpi_bus_offline_companions, > - (void *)true , (void **)&errdev); > + NULL, acpi_bus_offline, (void *)true, > + (void **)&errdev); > if (!errdev || acpi_force_hot_remove) > - acpi_bus_offline_companions(handle, 0, (void *)true, > - (void **)&errdev); > + acpi_bus_offline(handle, 0, (void *)true, > + (void **)&errdev); > > if (errdev && !acpi_force_hot_remove) { > dev_warn(errdev, "Offline failed.\n"); > - acpi_bus_online_companions(handle, 0, NULL, NULL); > + acpi_bus_online(handle, 0, NULL, NULL); > acpi_walk_namespace(ACPI_TYPE_ANY, handle, > - ACPI_UINT32_MAX, > - acpi_bus_online_companions, NULL, > - NULL, NULL); > + ACPI_UINT32_MAX, acpi_bus_online, > + NULL, NULL, NULL); > put_device(&device->dev); > return -EBUSY; > } -- To unsubscribe from this list: send the line "unsubscribe linux-pci" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Wed, 2013-11-06 at 02:35 +0100, Rafael J. Wysocki wrote: > On Tuesday, November 05, 2013 05:39:27 PM Toshi Kani wrote: > > On Wed, 2013-11-06 at 00:27 +0100, Rafael J. Wysocki wrote: > > > From: Rafael J. Wysocki <rafael.j.wysocki@intel.com> > > > > > > In theory, an ACPI device object may be the parent of another > > > device object whose hotplug is disabled by user space through its > > > scan handler. In that case, the eject operation targeting the > > > parent should fail as though the parent's own hotplug was disabled, > > > but currently this is not the case, because acpi_scan_hot_remove() > > > doesn't check the disable/enable hotplug status of the children > > > of the top-most object passed to it. > > > > > > To fix this, modify acpi_bus_offline_companions() to return an > > > error code if hotplug is disabled for the given device object. > > > [Also change the name of the function to acpi_bus_offline(), > > > because it is not only about companions any more, and change > > > the name of acpi_bus_online_companions() accordingly.] Make > > > acpi_scan_hot_remove() propagate that error to its callers. > > > > > : > > > +static acpi_status acpi_bus_online(acpi_handle handle, u32 lvl, void *data, > > > + void **ret_p) > > > { > > > struct acpi_device *device = NULL; > > > struct acpi_device_physical_node *pn; > > > @@ -214,26 +220,32 @@ static int acpi_scan_hot_remove(struct a > > > * If the first pass is successful, the second one isn't needed, though. > > > */ > > > errdev = NULL; > > > - acpi_walk_namespace(ACPI_TYPE_ANY, handle, ACPI_UINT32_MAX, > > > - NULL, acpi_bus_offline_companions, > > > - (void *)false, (void **)&errdev); > > > - acpi_bus_offline_companions(handle, 0, (void *)false, (void **)&errdev); > > > + status = acpi_walk_namespace(ACPI_TYPE_ANY, handle, ACPI_UINT32_MAX, > > > + NULL, acpi_bus_offline, (void *)false, > > > + (void **)&errdev); > > > + if (status == AE_SUPPORT) { > > > + dev_warn(errdev, "Offline disabled.\n"); > > > + acpi_walk_namespace(ACPI_TYPE_ANY, handle, ACPI_UINT32_MAX, > > > + acpi_bus_online, NULL, NULL, NULL); > > > + put_device(&device->dev); > > > + return -EPERM; > > > + } > > > + acpi_bus_offline(handle, 0, (void *)false, (void **)&errdev); > > > if (errdev) { > > > > If the target object failed with AE_SUPPORT, shouldn't we skip the 2nd > > pass and return with -EPERM after rollback? > > We've checked the target object already in acpi_hotplug_notify_cb() or in > acpi_eject_store(). Oh, I see. Thanks for the clarification. > > Which is telling me that the previous version of the patch was better after > all, because the hotplug.enabled thing takes precedence over > acpi_force_hot_remove in the other places. So this: > > https://patchwork.kernel.org/patch/3135841/ > > is the right version. Sorry for the confusion. Agreed. Acked-by: Toshi Kani <toshi.kani@hp.com> -Toshi -- To unsubscribe from this list: send the line "unsubscribe linux-pci" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Tuesday, November 05, 2013 05:39:27 PM Toshi Kani wrote: > On Wed, 2013-11-06 at 00:27 +0100, Rafael J. Wysocki wrote: > > From: Rafael J. Wysocki <rafael.j.wysocki@intel.com> > > > > In theory, an ACPI device object may be the parent of another > > device object whose hotplug is disabled by user space through its > > scan handler. In that case, the eject operation targeting the > > parent should fail as though the parent's own hotplug was disabled, > > but currently this is not the case, because acpi_scan_hot_remove() > > doesn't check the disable/enable hotplug status of the children > > of the top-most object passed to it. > > > > To fix this, modify acpi_bus_offline_companions() to return an > > error code if hotplug is disabled for the given device object. > > [Also change the name of the function to acpi_bus_offline(), > > because it is not only about companions any more, and change > > the name of acpi_bus_online_companions() accordingly.] Make > > acpi_scan_hot_remove() propagate that error to its callers. > > > : > > +static acpi_status acpi_bus_online(acpi_handle handle, u32 lvl, void *data, > > + void **ret_p) > > { > > struct acpi_device *device = NULL; > > struct acpi_device_physical_node *pn; > > @@ -214,26 +220,32 @@ static int acpi_scan_hot_remove(struct a > > * If the first pass is successful, the second one isn't needed, though. > > */ > > errdev = NULL; > > - acpi_walk_namespace(ACPI_TYPE_ANY, handle, ACPI_UINT32_MAX, > > - NULL, acpi_bus_offline_companions, > > - (void *)false, (void **)&errdev); > > - acpi_bus_offline_companions(handle, 0, (void *)false, (void **)&errdev); > > + status = acpi_walk_namespace(ACPI_TYPE_ANY, handle, ACPI_UINT32_MAX, > > + NULL, acpi_bus_offline, (void *)false, > > + (void **)&errdev); > > + if (status == AE_SUPPORT) { > > + dev_warn(errdev, "Offline disabled.\n"); > > + acpi_walk_namespace(ACPI_TYPE_ANY, handle, ACPI_UINT32_MAX, > > + acpi_bus_online, NULL, NULL, NULL); > > + put_device(&device->dev); > > + return -EPERM; > > + } > > + acpi_bus_offline(handle, 0, (void *)false, (void **)&errdev); > > if (errdev) { > > If the target object failed with AE_SUPPORT, shouldn't we skip the 2nd > pass and return with -EPERM after rollback? We've checked the target object already in acpi_hotplug_notify_cb() or in acpi_eject_store(). Which is telling me that the previous version of the patch was better after all, because the hotplug.enabled thing takes precedence over acpi_force_hot_remove in the other places. So this: https://patchwork.kernel.org/patch/3135841/ is the right version. Sorry for the confusion. Thanks, Rafael -- To unsubscribe from this list: send the line "unsubscribe linux-pci" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Index: linux-pm/drivers/acpi/scan.c =================================================================== --- linux-pm.orig/drivers/acpi/scan.c +++ linux-pm/drivers/acpi/scan.c @@ -125,8 +125,8 @@ acpi_device_modalias_show(struct device } static DEVICE_ATTR(modalias, 0444, acpi_device_modalias_show, NULL); -static acpi_status acpi_bus_offline_companions(acpi_handle handle, u32 lvl, - void *data, void **ret_p) +static acpi_status acpi_bus_offline(acpi_handle handle, u32 lvl, void *data, + void **ret_p) { struct acpi_device *device = NULL; struct acpi_device_physical_node *pn; @@ -136,6 +136,12 @@ static acpi_status acpi_bus_offline_comp if (acpi_bus_get_device(handle, &device)) return AE_OK; + if (device->handler && !device->handler->hotplug.enabled + && !acpi_force_hot_remove) { + *ret_p = &device->dev; + return AE_SUPPORT; + } + mutex_lock(&device->physical_node_lock); list_for_each_entry(pn, &device->physical_node_list, node) { @@ -168,8 +174,8 @@ static acpi_status acpi_bus_offline_comp return status; } -static acpi_status acpi_bus_online_companions(acpi_handle handle, u32 lvl, - void *data, void **ret_p) +static acpi_status acpi_bus_online(acpi_handle handle, u32 lvl, void *data, + void **ret_p) { struct acpi_device *device = NULL; struct acpi_device_physical_node *pn; @@ -214,26 +220,32 @@ static int acpi_scan_hot_remove(struct a * If the first pass is successful, the second one isn't needed, though. */ errdev = NULL; - acpi_walk_namespace(ACPI_TYPE_ANY, handle, ACPI_UINT32_MAX, - NULL, acpi_bus_offline_companions, - (void *)false, (void **)&errdev); - acpi_bus_offline_companions(handle, 0, (void *)false, (void **)&errdev); + status = acpi_walk_namespace(ACPI_TYPE_ANY, handle, ACPI_UINT32_MAX, + NULL, acpi_bus_offline, (void *)false, + (void **)&errdev); + if (status == AE_SUPPORT) { + dev_warn(errdev, "Offline disabled.\n"); + acpi_walk_namespace(ACPI_TYPE_ANY, handle, ACPI_UINT32_MAX, + acpi_bus_online, NULL, NULL, NULL); + put_device(&device->dev); + return -EPERM; + } + acpi_bus_offline(handle, 0, (void *)false, (void **)&errdev); if (errdev) { errdev = NULL; acpi_walk_namespace(ACPI_TYPE_ANY, handle, ACPI_UINT32_MAX, - NULL, acpi_bus_offline_companions, - (void *)true , (void **)&errdev); + NULL, acpi_bus_offline, (void *)true, + (void **)&errdev); if (!errdev || acpi_force_hot_remove) - acpi_bus_offline_companions(handle, 0, (void *)true, - (void **)&errdev); + acpi_bus_offline(handle, 0, (void *)true, + (void **)&errdev); if (errdev && !acpi_force_hot_remove) { dev_warn(errdev, "Offline failed.\n"); - acpi_bus_online_companions(handle, 0, NULL, NULL); + acpi_bus_online(handle, 0, NULL, NULL); acpi_walk_namespace(ACPI_TYPE_ANY, handle, - ACPI_UINT32_MAX, - acpi_bus_online_companions, NULL, - NULL, NULL); + ACPI_UINT32_MAX, acpi_bus_online, + NULL, NULL, NULL); put_device(&device->dev); return -EBUSY; } @@ -290,10 +302,9 @@ static void acpi_bus_device_eject(void * goto err_out; handler = device->handler; - if (!handler || !handler->hotplug.enabled) { - ost_code = ACPI_OST_SC_EJECT_NOT_SUPPORTED; - goto err_out; - } + if (!handler || !handler->hotplug.enabled) + goto err_support; + acpi_evaluate_hotplug_ost(handle, ACPI_NOTIFY_EJECT_REQUEST, ACPI_OST_SC_EJECT_IN_PROGRESS, NULL); if (handler->hotplug.mode == AHM_CONTAINER) @@ -301,14 +312,19 @@ static void acpi_bus_device_eject(void * get_device(&device->dev); error = acpi_scan_hot_remove(device); - if (error) + if (error == -EPERM) { + goto err_support; + } else if (error) { goto err_out; + } out: mutex_unlock(&acpi_scan_lock); unlock_device_hotplug(); return; + err_support: + ost_code = ACPI_OST_SC_EJECT_NOT_SUPPORTED; err_out: acpi_evaluate_hotplug_ost(handle, ACPI_NOTIFY_EJECT_REQUEST, ost_code, NULL);