Date:   Mon, 10 Apr 2017 19:13:44 +0200
From:   Michal Hocko <mhocko@...nel.org>
To:     "Rafael J. Wysocki" <rjw@...ysocki.net>
Cc:     Joey Lee <jlee@...e.com>, Jiri Kosina <jkosina@...e.cz>,
        Kani Toshimitsu <toshi.kani@....com>,
        Len Brown <lenb@...nel.org>,
        LKML <linux-kernel@...r.kernel.org>, linux-acpi@...r.kernel.org,
        linux-api@...r.kernel.org
Subject: Re: [PATCH] acpi: drop support for force_remove

On Mon 03-04-17 09:40:23, Michal Hocko wrote:
> From: Michal Hocko <mhocko@...e.com>
> 
> /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 <jlee@...e.com>
> Signed-off-by: Michal Hocko <mhocko@...e.com>

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/20170320192938.GA11363@dhcp22.suse.cz
> 
>  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 <rafael.j.wysocki@...el.com>
> +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 <rafael.j.wysocki@...el.com>
>  		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 <rafael.j.wysocki@...el.com>
> -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 <lenb@...nel.org>
> 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 linux-acpi@...r.kernel.org if you depend on this functionality\n");
> +		return -EINVAL;
> +	}
>  	return size;
>  }
>  
> -- 
> 2.11.0
> 

-- 
Michal Hocko
SUSE Labs

Powered by blists - more mailing lists