On Mon 03-04-17 09:40:23, Michal Hocko wrote: > From: Michal Hocko <[email protected]> > > /sys/firmware/acpi/hotplug/force_remove was presumably added to support > auto offlining in the past. This is, however, inherently dangerous for > some hotplugable resources like memory. The memory offlining fails when > the memory is still in use and cannot be dropped or migrated. If we > ignore the failure we are basically allowing for subtle memory > corruption or a crash. > > We have actually noticed the later while hitting BUG() during the memory > hotremove (remove_memory): > ret = walk_memory_range(PFN_DOWN(start), PFN_UP(start + size - 1), NULL, > check_memblock_offlined_cb); > if (ret) > BUG(); > > it took us quite non-trivial time realize that the customer had > force_remove enabled. Even if the BUG was removed here and we could > propagate the error up the call chain it wouldn't help at all because > then we would hit a crash or a memory corruption later and harder to > debug. So force_remove is unfixable for the memory hotremove. We haven't > checked other hotplugable resources to be prone to a similar problems. > > Remove the force_remove functionality because it is not fixable currently. > Keep the sysfs file and report an error if somebody tries to enable it. > Encourage users to report about the missing functionality and work with > them with an alternative solution. > > Reviewed-by: Lee, Chun-Yi <[email protected]> > Signed-off-by: Michal Hocko <[email protected]>
ping on this? > --- > Hi, > this has been previously discussed [1] without acpi mailing list CCed. > So I am resending it now with hopefully everybody involved. > > [1] http://lkml.kernel.org/r/[email protected] > > Documentation/ABI/obsolete/sysfs-firmware-acpi | 8 ++++++++ > Documentation/ABI/testing/sysfs-firmware-acpi | 10 ---------- > drivers/acpi/internal.h | 2 -- > drivers/acpi/scan.c | 16 +++------------- > drivers/acpi/sysfs.c | 9 +++++---- > 5 files changed, 16 insertions(+), 29 deletions(-) > create mode 100644 Documentation/ABI/obsolete/sysfs-firmware-acpi > > diff --git a/Documentation/ABI/obsolete/sysfs-firmware-acpi > b/Documentation/ABI/obsolete/sysfs-firmware-acpi > new file mode 100644 > index 000000000000..6715a71bec3d > --- /dev/null > +++ b/Documentation/ABI/obsolete/sysfs-firmware-acpi > @@ -0,0 +1,8 @@ > +What: /sys/firmware/acpi/hotplug/force_remove > +Date: Mar 2017 > +Contact: Rafael J. Wysocki <[email protected]> > +Description: > + Since the force_remove is inherently broken and dangerous to > + use for some hotplugable resources like memory (because ignoring > + the offline failure might lead to memory corruption and crashes) > + enabling this knob is not safe and thus unsupported. > diff --git a/Documentation/ABI/testing/sysfs-firmware-acpi > b/Documentation/ABI/testing/sysfs-firmware-acpi > index c7fc72d4495c..613f42a9d5cd 100644 > --- a/Documentation/ABI/testing/sysfs-firmware-acpi > +++ b/Documentation/ABI/testing/sysfs-firmware-acpi > @@ -44,16 +44,6 @@ Contact: Rafael J. Wysocki <[email protected]> > or 0 (unset). Attempts to write any other values to it will > cause -EINVAL to be returned. > > -What: /sys/firmware/acpi/hotplug/force_remove > -Date: May 2013 > -Contact: Rafael J. Wysocki <[email protected]> > -Description: > - The number in this file (0 or 1) determines whether (1) or not > - (0) the ACPI subsystem will allow devices to be hot-removed even > - if they cannot be put offline gracefully (from the kernel's > - viewpoint). That number can be changed by writing a boolean > - value to this file. > - > What: /sys/firmware/acpi/interrupts/ > Date: February 2008 > Contact: Len Brown <[email protected]> > diff --git a/drivers/acpi/internal.h b/drivers/acpi/internal.h > index f15900132912..66229ffa909b 100644 > --- a/drivers/acpi/internal.h > +++ b/drivers/acpi/internal.h > @@ -65,8 +65,6 @@ static inline void acpi_cmos_rtc_init(void) {} > #endif > int acpi_rev_override_setup(char *str); > > -extern bool acpi_force_hot_remove; > - > void acpi_sysfs_add_hotplug_profile(struct acpi_hotplug_profile *hotplug, > const char *name); > int acpi_scan_add_handler_with_hotplug(struct acpi_scan_handler *handler, > diff --git a/drivers/acpi/scan.c b/drivers/acpi/scan.c > index 192691880d55..e2080b6e54aa 100644 > --- a/drivers/acpi/scan.c > +++ b/drivers/acpi/scan.c > @@ -30,12 +30,6 @@ extern struct acpi_device *acpi_root; > > #define INVALID_ACPI_HANDLE ((acpi_handle)empty_zero_page) > > -/* > - * If set, devices will be hot-removed even if they cannot be put offline > - * gracefully (from the kernel's standpoint). > - */ > -bool acpi_force_hot_remove; > - > static const char *dummy_hid = "device"; > > static LIST_HEAD(acpi_dep_list); > @@ -170,9 +164,6 @@ static acpi_status acpi_bus_offline(acpi_handle handle, > u32 lvl, void *data, > pn->put_online = false; > } > ret = device_offline(pn->dev); > - if (acpi_force_hot_remove) > - continue; > - > if (ret >= 0) { > pn->put_online = !ret; > } else { > @@ -241,11 +232,11 @@ static int acpi_scan_try_to_offline(struct acpi_device > *device) > acpi_walk_namespace(ACPI_TYPE_ANY, handle, ACPI_UINT32_MAX, > NULL, acpi_bus_offline, (void *)true, > (void **)&errdev); > - if (!errdev || acpi_force_hot_remove) > + if (!errdev) > acpi_bus_offline(handle, 0, (void *)true, > (void **)&errdev); > > - if (errdev && !acpi_force_hot_remove) { > + if (errdev) { > dev_warn(errdev, "Offline failed.\n"); > acpi_bus_online(handle, 0, NULL, NULL); > acpi_walk_namespace(ACPI_TYPE_ANY, handle, > @@ -263,8 +254,7 @@ static int acpi_scan_hot_remove(struct acpi_device > *device) > unsigned long long sta; > acpi_status status; > > - if (device->handler && device->handler->hotplug.demand_offline > - && !acpi_force_hot_remove) { > + if (device->handler && device->handler->hotplug.demand_offline) { > if (!acpi_scan_is_offline(device, true)) > return -EBUSY; > } else { > diff --git a/drivers/acpi/sysfs.c b/drivers/acpi/sysfs.c > index cf05ae973381..1b5ee1e0e5a3 100644 > --- a/drivers/acpi/sysfs.c > +++ b/drivers/acpi/sysfs.c > @@ -921,7 +921,7 @@ void acpi_sysfs_add_hotplug_profile(struct > acpi_hotplug_profile *hotplug, > static ssize_t force_remove_show(struct kobject *kobj, > struct kobj_attribute *attr, char *buf) > { > - return sprintf(buf, "%d\n", !!acpi_force_hot_remove); > + return sprintf(buf, "%d\n", 0); > } > > static ssize_t force_remove_store(struct kobject *kobj, > @@ -935,9 +935,10 @@ static ssize_t force_remove_store(struct kobject *kobj, > if (ret < 0) > return ret; > > - lock_device_hotplug(); > - acpi_force_hot_remove = val; > - unlock_device_hotplug(); > + if (val) { > + pr_err("Enabling force_remove is not supported anymore. Please > report to [email protected] if you depend on this functionality\n"); > + return -EINVAL; > + } > return size; > } > > -- > 2.11.0 > -- Michal Hocko SUSE Labs
