| Message ID | 1511096.zmfmBrfdmu@vostro.rjw.lan (mailing list archive) |
|---|---|
| State | Accepted, archived |
| Headers | show |
On Thu, Jan 24, 2013 at 2:56 PM, Rafael J. Wysocki <rjw@sisk.pl> wrote: > From: Rafael J. Wysocki <rafael.j.wysocki@intel.com> > > Since acpi_bus_trim() cannot fail, change its definition to a void > function, so that its callers don't check the return value in vain > and update the callers. > > Signed-off-by: Rafael J. Wysocki <rafael.j.wysocki@intel.com> > --- Acked-by: Yinghai Lu <yinghai@kernel.org> -- 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
2013/01/25 7:56, Rafael J. Wysocki wrote: > From: Rafael J. Wysocki <rafael.j.wysocki@intel.com> > > Since acpi_bus_trim() cannot fail, change its definition to a void > function, so that its callers don't check the return value in vain > and update the callers. > > Signed-off-by: Rafael J. Wysocki <rafael.j.wysocki@intel.com> > --- Acked-by: Yasuaki Ishimatsu <isimatu.yasuaki@jp.fujitsu.com> > > On top of current linux-pm.git/linux-next. > > Thanks, > Rafael > > --- > drivers/acpi/dock.c | 8 ++------ > drivers/acpi/scan.c | 12 +++--------- > drivers/pci/hotplug/acpiphp_glue.c | 10 +++------- > include/acpi/acpi_bus.h | 2 +- > 4 files changed, 9 insertions(+), 23 deletions(-) > > Index: linux-pm/drivers/acpi/scan.c > =================================================================== > --- linux-pm.orig/drivers/acpi/scan.c > +++ linux-pm/drivers/acpi/scan.c > @@ -127,13 +127,8 @@ void acpi_bus_hot_remove_device(void *co > ACPI_DEBUG_PRINT((ACPI_DB_INFO, > "Hot-removing device %s...\n", dev_name(&device->dev))); > > - if (acpi_bus_trim(device)) { > - printk(KERN_ERR PREFIX > - "Removing device failed\n"); > - goto err_out; > - } > - > - /* device has been freed */ > + acpi_bus_trim(device); > + /* Device node has been released. */ > device = NULL; > > /* power off device */ > @@ -1656,7 +1651,7 @@ static acpi_status acpi_bus_remove(acpi_ > return AE_OK; > } > > -int acpi_bus_trim(struct acpi_device *start) > +void acpi_bus_trim(struct acpi_device *start) > { > /* > * Execute acpi_bus_device_detach() as a post-order callback to detach > @@ -1672,7 +1667,6 @@ int acpi_bus_trim(struct acpi_device *st > acpi_walk_namespace(ACPI_TYPE_ANY, start->handle, ACPI_UINT32_MAX, NULL, > acpi_bus_remove, NULL, NULL); > acpi_bus_remove(start->handle, 0, NULL, NULL); > - return 0; > } > EXPORT_SYMBOL_GPL(acpi_bus_trim); > > Index: linux-pm/include/acpi/acpi_bus.h > =================================================================== > --- linux-pm.orig/include/acpi/acpi_bus.h > +++ linux-pm/include/acpi/acpi_bus.h > @@ -386,7 +386,7 @@ int acpi_bus_register_driver(struct acpi > void acpi_bus_unregister_driver(struct acpi_driver *driver); > int acpi_bus_scan(acpi_handle handle); > void acpi_bus_hot_remove_device(void *context); > -int acpi_bus_trim(struct acpi_device *start); > +void acpi_bus_trim(struct acpi_device *start); > acpi_status acpi_bus_get_ejd(acpi_handle handle, acpi_handle * ejd); > int acpi_match_device_ids(struct acpi_device *device, > const struct acpi_device_id *ids); > Index: linux-pm/drivers/acpi/dock.c > =================================================================== > --- linux-pm.orig/drivers/acpi/dock.c > +++ linux-pm/drivers/acpi/dock.c > @@ -336,13 +336,9 @@ static struct acpi_device * dock_create_ > static void dock_remove_acpi_device(acpi_handle handle) > { > struct acpi_device *device; > - int ret; > > - if (!acpi_bus_get_device(handle, &device)) { > - ret = acpi_bus_trim(device); > - if (ret) > - pr_debug("error removing bus, %x\n", -ret); > - } > + if (!acpi_bus_get_device(handle, &device)) > + acpi_bus_trim(device); > } > > /** > Index: linux-pm/drivers/pci/hotplug/acpiphp_glue.c > =================================================================== > --- linux-pm.orig/drivers/pci/hotplug/acpiphp_glue.c > +++ linux-pm/drivers/pci/hotplug/acpiphp_glue.c > @@ -742,8 +742,7 @@ static int acpiphp_bus_add(struct acpiph > /* this shouldn't be in here, so remove > * the bus then re-add it... > */ > - ret_val = acpi_bus_trim(device); > - dbg("acpi_bus_trim return %x\n", ret_val); > + acpi_bus_trim(device); > } > > ret_val = acpi_bus_scan(func->handle); > @@ -772,11 +771,8 @@ static int acpiphp_bus_trim(acpi_handle > return retval; > } > > - retval = acpi_bus_trim(device); > - if (retval) > - err("cannot remove from acpi list\n"); > - > - return retval; > + acpi_bus_trim(device); > + return 0; > } > > static void acpiphp_set_acpi_region(struct acpiphp_slot *slot) > > -- > 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 > -- 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 Thu, 2013-01-24 at 23:56 +0100, Rafael J. Wysocki wrote: > From: Rafael J. Wysocki <rafael.j.wysocki@intel.com> > > Since acpi_bus_trim() cannot fail, change its definition to a void > function, so that its callers don't check the return value in vain > and update the callers. > > Signed-off-by: Rafael J. Wysocki <rafael.j.wysocki@intel.com> Acked-by: Toshi Kani <toshi.kani@hp.com> Thanks, -Toshi > --- > > On top of current linux-pm.git/linux-next. > > Thanks, > Rafael > > --- > drivers/acpi/dock.c | 8 ++------ > drivers/acpi/scan.c | 12 +++--------- > drivers/pci/hotplug/acpiphp_glue.c | 10 +++------- > include/acpi/acpi_bus.h | 2 +- > 4 files changed, 9 insertions(+), 23 deletions(-) > > Index: linux-pm/drivers/acpi/scan.c > =================================================================== > --- linux-pm.orig/drivers/acpi/scan.c > +++ linux-pm/drivers/acpi/scan.c > @@ -127,13 +127,8 @@ void acpi_bus_hot_remove_device(void *co > ACPI_DEBUG_PRINT((ACPI_DB_INFO, > "Hot-removing device %s...\n", dev_name(&device->dev))); > > - if (acpi_bus_trim(device)) { > - printk(KERN_ERR PREFIX > - "Removing device failed\n"); > - goto err_out; > - } > - > - /* device has been freed */ > + acpi_bus_trim(device); > + /* Device node has been released. */ > device = NULL; > > /* power off device */ > @@ -1656,7 +1651,7 @@ static acpi_status acpi_bus_remove(acpi_ > return AE_OK; > } > > -int acpi_bus_trim(struct acpi_device *start) > +void acpi_bus_trim(struct acpi_device *start) > { > /* > * Execute acpi_bus_device_detach() as a post-order callback to detach > @@ -1672,7 +1667,6 @@ int acpi_bus_trim(struct acpi_device *st > acpi_walk_namespace(ACPI_TYPE_ANY, start->handle, ACPI_UINT32_MAX, NULL, > acpi_bus_remove, NULL, NULL); > acpi_bus_remove(start->handle, 0, NULL, NULL); > - return 0; > } > EXPORT_SYMBOL_GPL(acpi_bus_trim); > > Index: linux-pm/include/acpi/acpi_bus.h > =================================================================== > --- linux-pm.orig/include/acpi/acpi_bus.h > +++ linux-pm/include/acpi/acpi_bus.h > @@ -386,7 +386,7 @@ int acpi_bus_register_driver(struct acpi > void acpi_bus_unregister_driver(struct acpi_driver *driver); > int acpi_bus_scan(acpi_handle handle); > void acpi_bus_hot_remove_device(void *context); > -int acpi_bus_trim(struct acpi_device *start); > +void acpi_bus_trim(struct acpi_device *start); > acpi_status acpi_bus_get_ejd(acpi_handle handle, acpi_handle * ejd); > int acpi_match_device_ids(struct acpi_device *device, > const struct acpi_device_id *ids); > Index: linux-pm/drivers/acpi/dock.c > =================================================================== > --- linux-pm.orig/drivers/acpi/dock.c > +++ linux-pm/drivers/acpi/dock.c > @@ -336,13 +336,9 @@ static struct acpi_device * dock_create_ > static void dock_remove_acpi_device(acpi_handle handle) > { > struct acpi_device *device; > - int ret; > > - if (!acpi_bus_get_device(handle, &device)) { > - ret = acpi_bus_trim(device); > - if (ret) > - pr_debug("error removing bus, %x\n", -ret); > - } > + if (!acpi_bus_get_device(handle, &device)) > + acpi_bus_trim(device); > } > > /** > Index: linux-pm/drivers/pci/hotplug/acpiphp_glue.c > =================================================================== > --- linux-pm.orig/drivers/pci/hotplug/acpiphp_glue.c > +++ linux-pm/drivers/pci/hotplug/acpiphp_glue.c > @@ -742,8 +742,7 @@ static int acpiphp_bus_add(struct acpiph > /* this shouldn't be in here, so remove > * the bus then re-add it... > */ > - ret_val = acpi_bus_trim(device); > - dbg("acpi_bus_trim return %x\n", ret_val); > + acpi_bus_trim(device); > } > > ret_val = acpi_bus_scan(func->handle); > @@ -772,11 +771,8 @@ static int acpiphp_bus_trim(acpi_handle > return retval; > } > > - retval = acpi_bus_trim(device); > - if (retval) > - err("cannot remove from acpi list\n"); > - > - return retval; > + acpi_bus_trim(device); > + return 0; > } > > static void acpiphp_set_acpi_region(struct acpiphp_slot *slot) > -- 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
Hi, On Thu, Jan 24, 2013 at 11:56:50PM +0100, Rafael J. Wysocki wrote: > From: Rafael J. Wysocki <rafael.j.wysocki@intel.com> > > Since acpi_bus_trim() cannot fail, change its definition to a void > function, so that its callers don't check the return value in vain > and update the callers. I have missed a few patchsets/discussions in the last month and wanted to ask a question related to this: Does the new always-succeed 2-pass trim_device design guarantee safe memory hot-remove operations? Afaict if memory offline fails now, the device is ejected (_EJ0) anyways causing a panic. Tested in a VM with linux-next-20130207 and linux-next-20130218 by doing an SCI-eject request on a hot-plugged dimm. Are there more patches in development for safe memory hot-remove? thanks, - Vasilis -- 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 Monday, February 18, 2013 01:25:49 PM Vasilis Liaskovitis wrote: > Hi, > > On Thu, Jan 24, 2013 at 11:56:50PM +0100, Rafael J. Wysocki wrote: > > From: Rafael J. Wysocki <rafael.j.wysocki@intel.com> > > > > Since acpi_bus_trim() cannot fail, change its definition to a void > > function, so that its callers don't check the return value in vain > > and update the callers. > > I have missed a few patchsets/discussions in the last month and wanted to > ask a question related to this: Does the new always-succeed 2-pass > trim_device design guarantee safe memory hot-remove operations? I doesn't by itself. Nor it really can, because the .remove() callbacks of device drivers are not allowed to fail. > Afaict if memory offline fails now, the device is ejected (_EJ0) anyways > causing a panic. Tested in a VM with linux-next-20130207 and > linux-next-20130218 by doing an SCI-eject request on a hot-plugged dimm. > > Are there more patches in development for safe memory hot-remove? Yes, there are. I sent a patch series yesterday introducing some safety measures (you can disable memory hotplug from user space or disable automatic ejection). There's more to come still. The plan is to introduce offline/online operations for memory modules (in analogy with CPU core online/offline) that can be started by user space and memory eject will only be possible after offline (i.e. when the memory module is known to be not in use). Thanks, Rafael
On Mon, Feb 18, 2013 at 02:17:02PM +0100, Rafael J. Wysocki wrote: > On Monday, February 18, 2013 01:25:49 PM Vasilis Liaskovitis wrote: > > Hi, > > > > On Thu, Jan 24, 2013 at 11:56:50PM +0100, Rafael J. Wysocki wrote: > > > From: Rafael J. Wysocki <rafael.j.wysocki@intel.com> > > > > > > Since acpi_bus_trim() cannot fail, change its definition to a void > > > function, so that its callers don't check the return value in vain > > > and update the callers. > > > > I have missed a few patchsets/discussions in the last month and wanted to > > ask a question related to this: Does the new always-succeed 2-pass > > trim_device design guarantee safe memory hot-remove operations? > > I doesn't by itself. Nor it really can, because the .remove() callbacks of > device drivers are not allowed to fail. right. > > > Afaict if memory offline fails now, the device is ejected (_EJ0) anyways > > causing a panic. Tested in a VM with linux-next-20130207 and > > linux-next-20130218 by doing an SCI-eject request on a hot-plugged dimm. > > > > Are there more patches in development for safe memory hot-remove? > > Yes, there are. I sent a patch series yesterday introducing some safety > measures (you can disable memory hotplug from user space or disable > automatic ejection). There's more to come still. > > The plan is to introduce offline/online operations for memory modules (in > analogy with CPU core online/offline) that can be started by user space and > memory eject will only be possible after offline (i.e. when the memory module > is known to be not in use). ok, thanks for the info. I 'll test these and the follow ups. thanks again, - Vasilis -- 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
Index: linux-pm/drivers/acpi/scan.c =================================================================== --- linux-pm.orig/drivers/acpi/scan.c +++ linux-pm/drivers/acpi/scan.c @@ -127,13 +127,8 @@ void acpi_bus_hot_remove_device(void *co ACPI_DEBUG_PRINT((ACPI_DB_INFO, "Hot-removing device %s...\n", dev_name(&device->dev))); - if (acpi_bus_trim(device)) { - printk(KERN_ERR PREFIX - "Removing device failed\n"); - goto err_out; - } - - /* device has been freed */ + acpi_bus_trim(device); + /* Device node has been released. */ device = NULL; /* power off device */ @@ -1656,7 +1651,7 @@ static acpi_status acpi_bus_remove(acpi_ return AE_OK; } -int acpi_bus_trim(struct acpi_device *start) +void acpi_bus_trim(struct acpi_device *start) { /* * Execute acpi_bus_device_detach() as a post-order callback to detach @@ -1672,7 +1667,6 @@ int acpi_bus_trim(struct acpi_device *st acpi_walk_namespace(ACPI_TYPE_ANY, start->handle, ACPI_UINT32_MAX, NULL, acpi_bus_remove, NULL, NULL); acpi_bus_remove(start->handle, 0, NULL, NULL); - return 0; } EXPORT_SYMBOL_GPL(acpi_bus_trim); Index: linux-pm/include/acpi/acpi_bus.h =================================================================== --- linux-pm.orig/include/acpi/acpi_bus.h +++ linux-pm/include/acpi/acpi_bus.h @@ -386,7 +386,7 @@ int acpi_bus_register_driver(struct acpi void acpi_bus_unregister_driver(struct acpi_driver *driver); int acpi_bus_scan(acpi_handle handle); void acpi_bus_hot_remove_device(void *context); -int acpi_bus_trim(struct acpi_device *start); +void acpi_bus_trim(struct acpi_device *start); acpi_status acpi_bus_get_ejd(acpi_handle handle, acpi_handle * ejd); int acpi_match_device_ids(struct acpi_device *device, const struct acpi_device_id *ids); Index: linux-pm/drivers/acpi/dock.c =================================================================== --- linux-pm.orig/drivers/acpi/dock.c +++ linux-pm/drivers/acpi/dock.c @@ -336,13 +336,9 @@ static struct acpi_device * dock_create_ static void dock_remove_acpi_device(acpi_handle handle) { struct acpi_device *device; - int ret; - if (!acpi_bus_get_device(handle, &device)) { - ret = acpi_bus_trim(device); - if (ret) - pr_debug("error removing bus, %x\n", -ret); - } + if (!acpi_bus_get_device(handle, &device)) + acpi_bus_trim(device); } /** Index: linux-pm/drivers/pci/hotplug/acpiphp_glue.c =================================================================== --- linux-pm.orig/drivers/pci/hotplug/acpiphp_glue.c +++ linux-pm/drivers/pci/hotplug/acpiphp_glue.c @@ -742,8 +742,7 @@ static int acpiphp_bus_add(struct acpiph /* this shouldn't be in here, so remove * the bus then re-add it... */ - ret_val = acpi_bus_trim(device); - dbg("acpi_bus_trim return %x\n", ret_val); + acpi_bus_trim(device); } ret_val = acpi_bus_scan(func->handle); @@ -772,11 +771,8 @@ static int acpiphp_bus_trim(acpi_handle return retval; } - retval = acpi_bus_trim(device); - if (retval) - err("cannot remove from acpi list\n"); - - return retval; + acpi_bus_trim(device); + return 0; } static void acpiphp_set_acpi_region(struct acpiphp_slot *slot)