acpi : cpu hot-remove returns error when cpu_down() fails
Subsystems (3)
PATCH v2 1/3 acpi : cpu hot-remove returns error when cpu_down() fails
Even if cpu_down() fails, acpi_processor_remove() continues to remove the cpu. But in this case, it should return error number since some process may run on the cpu. If the cpu has a running process and the cpu is turned the power off, the system may not work well.
--- drivers/acpi/processor_driver.c | 18 ++++++++++++------ 1 file changed, 12 insertions(+), 6 deletions(-)
drivers/acpi/processor_driver.c +12 -6
@@ -610,7 +610,7 @@ err_free_pr:static int acpi_processor_remove (struct acpi_device *device,int type) {struct acpi_processor *pr =NULL ; - +int ret;if (!device || !acpi_driver_data (device))return -EINVAL ;
@@ -621,8 +621,9 @@ static int acpi_processor_remove(struct goto free;if (type ==ACPI_BUS_REMOVAL_EJECT ) { -if (acpi_processor_handle_eject (pr)) -return -EINVAL ; + ret =acpi_processor_handle_eject (pr); +if (ret) +return ret; }acpi_processor_power_exit (pr, device);
@@ -841,12 +842,17 @@ static acpi_status acpi_processor_hotaddstatic int acpi_processor_handle_eject (struct acpi_processor *pr) { -if (cpu_online (pr->id)) -cpu_down (pr->id); +int ret; + +if (cpu_online (pr->id)) { + ret =cpu_down (pr->id); +if (ret) +return ret; + }arch_unregister_cpu (pr->id);acpi_unmap_lsapic (pr->id); -return (0 ); +return ret; }#else static acpi_status acpi_processor_hotadd_init (struct acpi_processor *pr)
PATCH v2 2/3 acpi : prevent cpu from becoming online
Even if acpi_processor_handle_eject() offlines cpu, there is a chance to online the cpu after that. So the patch closes the window by using get/put_online_cpus().
Why does the patch change _cpu_up() logic?
The patch cares the race of hot-remove cpu and _cpu_up(). If the patch does not change it, there is the following race.
hot-remove cpu | _cpu_up()
------------------------------------- ------------------------------------
call acpi_processor_handle_eject() |
call cpu_down() |
call get_online_cpus() |
| call cpu_hotplug_begin() and stop here
call arch_unregister_cpu() |
call acpi_unmap_lsapic() |
call put_online_cpus() |
| start and continue _cpu_up()
return acpi_processor_remove() |
continue hot-remove the cpu |So _cpu_up() can continue to itself. And hot-remove cpu can also continue itself. If the patch changes _cpu_cp() logic, the race disappears as below:
hot-remove cpu | _cpu_up()
-----------------------------------------------------------------------
call acpi_processor_handle_eject() |
call cpu_down() |
call get_online_cpus() |
| call cpu_hotplug_begin() and stop here
call arch_unregister_cpu() |
call acpi_unmap_lsapic() |
cpu's cpu_present is set |
to false by set_cpu_present()|
call put_online_cpus() |
| start _cpu_up()
| check cpu_present() and return -EINVAL
return acpi_processor_remove() |
continue hot-remove the cpu |--- drivers/acpi/processor_driver.c | 12 ++++++++++++ kernel/cpu.c | 8 +++++--- 2 files changed, 17 insertions(+), 3 deletions(-)
drivers/acpi/processor_driver.c +10 -0
@@ -850,8 +850,18 @@ static int acpi_processor_handle_eject(s return ret; } +get_online_cpus (); +/* +* Someone might online the cpu again at this point. So we check that +* cpu has been onlined or not. If cpu is online, it returns -EBUSY. +*/ +if (unlikely (cpu_online (pr->id))) { +put_online_cpus (); +return -EBUSY ; + }arch_unregister_cpu (pr->id);acpi_unmap_lsapic (pr->id); +put_online_cpus ();return ret; }#else
kernel/cpu.c +6 -3
@@ -343,11 +343,14 @@ static int __cpuinit _cpu_up(unsigned inunsigned long mod = tasks_frozen ?CPU_TASKS_FROZEN :0 ;struct task_struct *idle; -if (cpu_online (cpu) || !cpu_present (cpu)) -return -EINVAL ; -cpu_hotplug_begin (); +if (cpu_online (cpu) || !cpu_present (cpu)) { + ret = -EINVAL ; + goto out; + } + + idle =idle_thread_get (cpu);if (IS_ERR (idle)) { ret =PTR_ERR (idle);
PATCH v2 3/3 acpi : acpi_bus_trim() stops removing devices when failing to remove the device
In reply to Yasuaki Ishimatsuacpi_bus_trim() stops removing devices, when acpi_bus_remove() return error number. But acpi_bus_remove() cannot return error number correctly. acpi_bus_remove() only return -EINVAL, when dev argument is NULL. Thus even if device cannot be removed correctly, acpi_bus_trim() ignores and continues to remove devices. acpi_bus_hot_remove_device() uses acpi_bus_trim() for removing devices. Therefore acpi_bus_hot_remove_device() can send "_EJ0" to firmware, even if the device is running on the system. In this case, the system cannot work well. So acpi_bus_trim() should check whether device was removed or not correctly. The patch adds error check into some functions to remove the device.
--- drivers/acpi/scan.c | 15 ++++++++++++--- drivers/base/dd.c | 22 +++++++++++++++++----- include/linux/device.h | 2 +- 3 files changed, 30 insertions(+), 9 deletions(-)
drivers/acpi/scan.c +12 -3
@@ -425,12 +425,17 @@ static int acpi_device_remove(struct dev {struct acpi_device *acpi_dev =to_acpi_device (dev);struct acpi_driver *acpi_drv = acpi_dev->driver; +int ret;if (acpi_drv) {if (acpi_drv->ops.notify)acpi_device_remove_notify_handler (acpi_dev); -if (acpi_drv->ops.remove) - acpi_drv->ops.remove (acpi_dev, acpi_dev->removal_type); +if (acpi_drv->ops.remove) { + ret = acpi_drv->ops.remove (acpi_dev, + acpi_dev->removal_type); +if (ret) +return ret; + } } acpi_dev->driver =NULL ; acpi_dev->driver_data =NULL ;
@@ -1208,11 +1213,15 @@ static int acpi_device_set_context(strucstatic int acpi_bus_remove (struct acpi_device *dev,int rmdevice) { +int ret; +if (!dev)return -EINVAL ; dev->removal_type =ACPI_BUS_REMOVAL_EJECT ; -device_release_driver (&dev->dev); + ret =device_release_driver (&dev->dev); +if (ret) +return ret;if (!rmdevice)return 0 ;
drivers/base/dd.c +17 -5
@@ -464,9 +464,10 @@ EXPORT_SYMBOL_GPL(driver_attach);* __device_release_driver() must be called with @dev lock held. * When called for a USB interface, @dev->parent lock must be held as well. */ -static void __device_release_driver (struct device *dev) +static int __device_release_driver (struct device *dev) {struct device_driver *drv; +int ret; drv = dev->driver;if (drv) {
@@ -482,9 +483,11 @@ static void __device_release_driver(strupm_runtime_put_sync (dev);if (dev->bus && dev->bus->remove) - dev->bus->remove (dev); + ret = dev->bus->remove (dev);else if (drv->remove) - drv->remove (dev); + ret = drv->remove (dev); +if (ret) + goto rollback;devres_release_all (dev); dev->driver =NULL ;klist_remove (&dev->p->knode_driver);
@@ -494,6 +497,12 @@ static void __device_release_driver(stru dev); } + +return ret; + +rollback: +driver_sysfs_add (dev); +return ret; }/**
@@ -503,16 +512,19 @@ static void __device_release_driver(stru* Manually detach device from driver. * When called for a USB interface, @dev->parent lock must be held. */ -void device_release_driver (struct device *dev) +int device_release_driver (struct device *dev) { +int ret;/* * If anyone calls device_release_driver() recursively from * within their ->remove callback for the same device, they * will deadlock right here. */ device_lock (dev); -__device_release_driver (dev); + ret =__device_release_driver (dev);device_unlock (dev); + +return ret; }EXPORT_SYMBOL_GPL (device_release_driver);
include/linux/device.h +1 -1
@@ -827,7 +827,7 @@ static inline void *dev_get_platdata(con* for information on use. */ extern int __must_check device_bind_driver (struct device *dev); -extern void device_release_driver (struct device *dev); +extern int device_release_driver (struct device *dev);extern int __must_check device_attach (struct device *dev);extern int __must_check driver_attach (struct device_driver *drv);extern int __must_check device_reprobe (struct device *dev);