Re: [PATCH v2] acpi : acpi_bus_trim() stops removing devices when failing to remove the device
Hi Greg, 2012/10/27 0:25, Greg Kroah-Hartman wrote: On Fri, Oct 26, 2012 at 04:33:49PM +0900, Yasuaki Ishimatsu wrote: Hi Greg, Sorry for late reply. 2012/10/20 2:59, Greg Kroah-Hartman wrote: On Fri, Oct 19, 2012 at 06:29:52AM +0200, Rafael J. Wysocki wrote: On Thursday 11 of October 2012 19:12:28 Yasuaki Ishimatsu wrote: acpi_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. Vasilis hit the bug at memory hotplug and reported it as follow: https://lkml.org/lkml/2012/9/26/318 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. Applying the patch, acpi_bus_trim() stops removing devices when failing to remove the device. But I think there is no impact with the exceptionof CPU and Memory hotplug path. Because other device also fails but the fail is an irregular case like device is NULL. v1->v2 - add a rollback for reinstalling a notify handler. Signed-off-by: Yasuaki Ishimatsu Greg, do you think there may be any problems with the changes in dd.c? Yes, I don't like it. remove should always work, just like the exit call in a module. It means that the core wants to remove the driver, so it is going to happen, a driver can't refuse it. Which brings me to the larger question, why would this solve anything? Now we are developing physical memory hot plug. https://lkml.org/lkml/2012/10/23/213 So if we aplly the patch-set, we can hot remove a physical memory by the following way. "echo 1 > /sys/bus/acpi/devices/PNP/eject" In this case, acpi_bus_hot_remove_device() tries to remove memory device by acpi_bus_trim(). But if the memory has irremovable memory, memory hot remove fails. And the memory remains in kernel. However acpi_bus_trim() cannot notice that memory hot remove fails and retruns 0. So acpi_bus_hot_remove_device() continues to remove memory devices and sends _EJ0 method to firmware. Thus the memory device cannot be used. But the memory remains in kernel yet. So if someone access the memory, kernel panic occurs. Why can't you check to find out if you can do the remove operation before you enter the driver core asking to actually remove the devices? That would allow you to "know" if you can do this before having to go through the whole operation. What happens if you can complete half of the removal, and do that, but not the whole thing? Don't you end up with half of the memory chunk gone from the system now? In other words, please solve this at a higher level than the driver core if at all possible. O.K. I'll check whether the problem is sloved at a higher level or not. Thanks, Yasuaki Ishimatsu greg k-h -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH v2] acpi : acpi_bus_trim() stops removing devices when failing to remove the device
Hi Greg, 2012/10/27 0:25, Greg Kroah-Hartman wrote: On Fri, Oct 26, 2012 at 04:33:49PM +0900, Yasuaki Ishimatsu wrote: Hi Greg, Sorry for late reply. 2012/10/20 2:59, Greg Kroah-Hartman wrote: On Fri, Oct 19, 2012 at 06:29:52AM +0200, Rafael J. Wysocki wrote: On Thursday 11 of October 2012 19:12:28 Yasuaki Ishimatsu wrote: acpi_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. Vasilis hit the bug at memory hotplug and reported it as follow: https://lkml.org/lkml/2012/9/26/318 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. Applying the patch, acpi_bus_trim() stops removing devices when failing to remove the device. But I think there is no impact with the exceptionof CPU and Memory hotplug path. Because other device also fails but the fail is an irregular case like device is NULL. v1-v2 - add a rollback for reinstalling a notify handler. Signed-off-by: Yasuaki Ishimatsu isimatu.yasu...@jp.fujitsu.com Greg, do you think there may be any problems with the changes in dd.c? Yes, I don't like it. remove should always work, just like the exit call in a module. It means that the core wants to remove the driver, so it is going to happen, a driver can't refuse it. Which brings me to the larger question, why would this solve anything? Now we are developing physical memory hot plug. https://lkml.org/lkml/2012/10/23/213 So if we aplly the patch-set, we can hot remove a physical memory by the following way. echo 1 /sys/bus/acpi/devices/PNP/eject In this case, acpi_bus_hot_remove_device() tries to remove memory device by acpi_bus_trim(). But if the memory has irremovable memory, memory hot remove fails. And the memory remains in kernel. However acpi_bus_trim() cannot notice that memory hot remove fails and retruns 0. So acpi_bus_hot_remove_device() continues to remove memory devices and sends _EJ0 method to firmware. Thus the memory device cannot be used. But the memory remains in kernel yet. So if someone access the memory, kernel panic occurs. Why can't you check to find out if you can do the remove operation before you enter the driver core asking to actually remove the devices? That would allow you to know if you can do this before having to go through the whole operation. What happens if you can complete half of the removal, and do that, but not the whole thing? Don't you end up with half of the memory chunk gone from the system now? In other words, please solve this at a higher level than the driver core if at all possible. O.K. I'll check whether the problem is sloved at a higher level or not. Thanks, Yasuaki Ishimatsu greg k-h -- To unsubscribe from this list: send the line unsubscribe linux-kernel in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH v2] acpi : acpi_bus_trim() stops removing devices when failing to remove the device
On Fri, Oct 26, 2012 at 04:33:49PM +0900, Yasuaki Ishimatsu wrote: > Hi Greg, > > Sorry for late reply. > > 2012/10/20 2:59, Greg Kroah-Hartman wrote: > >On Fri, Oct 19, 2012 at 06:29:52AM +0200, Rafael J. Wysocki wrote: > >>On Thursday 11 of October 2012 19:12:28 Yasuaki Ishimatsu wrote: > >>>acpi_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. > >>> > >>>Vasilis hit the bug at memory hotplug and reported it as follow: > >>>https://lkml.org/lkml/2012/9/26/318 > >>> > >>>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. > >>> > >>>Applying the patch, acpi_bus_trim() stops removing devices when failing > >>>to remove the device. But I think there is no impact with the > >>>exceptionof CPU and Memory hotplug path. Because other device also fails > >>>but the fail is an irregular case like device is NULL. > >>> > >>>v1->v2 > >>>- add a rollback for reinstalling a notify handler. > >>> > >>>Signed-off-by: Yasuaki Ishimatsu > >> > >>Greg, do you think there may be any problems with the changes in dd.c? > > > >Yes, I don't like it. > > > >remove should always work, just like the exit call in a module. It > >means that the core wants to remove the driver, so it is going to > >happen, a driver can't refuse it. > > > >Which brings me to the larger question, why would this solve anything? > > Now we are developing physical memory hot plug. > > https://lkml.org/lkml/2012/10/23/213 > > So if we aplly the patch-set, we can hot remove a physical memory > by the following way. > > "echo 1 > /sys/bus/acpi/devices/PNP/eject" > > In this case, acpi_bus_hot_remove_device() tries to remove memory > device by acpi_bus_trim(). But if the memory has irremovable memory, > memory hot remove fails. And the memory remains in kernel. > However acpi_bus_trim() cannot notice that memory hot remove fails and > retruns 0. So acpi_bus_hot_remove_device() continues to remove memory > devices and sends _EJ0 method to firmware. Thus the memory device cannot > be used. But the memory remains in kernel yet. So if someone access the > memory, kernel panic occurs. Why can't you check to find out if you can do the remove operation before you enter the driver core asking to actually remove the devices? That would allow you to "know" if you can do this before having to go through the whole operation. What happens if you can complete half of the removal, and do that, but not the whole thing? Don't you end up with half of the memory chunk gone from the system now? In other words, please solve this at a higher level than the driver core if at all possible. greg k-h -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH v2] acpi : acpi_bus_trim() stops removing devices when failing to remove the device
Hi Greg, Sorry for late reply. 2012/10/20 2:59, Greg Kroah-Hartman wrote: On Fri, Oct 19, 2012 at 06:29:52AM +0200, Rafael J. Wysocki wrote: On Thursday 11 of October 2012 19:12:28 Yasuaki Ishimatsu wrote: acpi_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. Vasilis hit the bug at memory hotplug and reported it as follow: https://lkml.org/lkml/2012/9/26/318 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. Applying the patch, acpi_bus_trim() stops removing devices when failing to remove the device. But I think there is no impact with the exceptionof CPU and Memory hotplug path. Because other device also fails but the fail is an irregular case like device is NULL. v1->v2 - add a rollback for reinstalling a notify handler. Signed-off-by: Yasuaki Ishimatsu Greg, do you think there may be any problems with the changes in dd.c? Yes, I don't like it. remove should always work, just like the exit call in a module. It means that the core wants to remove the driver, so it is going to happen, a driver can't refuse it. Which brings me to the larger question, why would this solve anything? Now we are developing physical memory hot plug. https://lkml.org/lkml/2012/10/23/213 So if we aplly the patch-set, we can hot remove a physical memory by the following way. "echo 1 > /sys/bus/acpi/devices/PNP/eject" In this case, acpi_bus_hot_remove_device() tries to remove memory device by acpi_bus_trim(). But if the memory has irremovable memory, memory hot remove fails. And the memory remains in kernel. However acpi_bus_trim() cannot notice that memory hot remove fails and retruns 0. So acpi_bus_hot_remove_device() continues to remove memory devices and sends _EJ0 method to firmware. Thus the memory device cannot be used. But the memory remains in kernel yet. So if someone access the memory, kernel panic occurs. Thanks, Yasuaki Ishimatsu If the kernel wants to unbind a device, why would we ever not want that to happen? So, NAK on this patch, sorry. Fix up the ACPI core to handle this properly, don't mess with the driver core here. greg k-h -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH v2] acpi : acpi_bus_trim() stops removing devices when failing to remove the device
Hi Greg, Sorry for late reply. 2012/10/20 2:59, Greg Kroah-Hartman wrote: On Fri, Oct 19, 2012 at 06:29:52AM +0200, Rafael J. Wysocki wrote: On Thursday 11 of October 2012 19:12:28 Yasuaki Ishimatsu wrote: acpi_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. Vasilis hit the bug at memory hotplug and reported it as follow: https://lkml.org/lkml/2012/9/26/318 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. Applying the patch, acpi_bus_trim() stops removing devices when failing to remove the device. But I think there is no impact with the exceptionof CPU and Memory hotplug path. Because other device also fails but the fail is an irregular case like device is NULL. v1-v2 - add a rollback for reinstalling a notify handler. Signed-off-by: Yasuaki Ishimatsu isimatu.yasu...@jp.fujitsu.com Greg, do you think there may be any problems with the changes in dd.c? Yes, I don't like it. remove should always work, just like the exit call in a module. It means that the core wants to remove the driver, so it is going to happen, a driver can't refuse it. Which brings me to the larger question, why would this solve anything? Now we are developing physical memory hot plug. https://lkml.org/lkml/2012/10/23/213 So if we aplly the patch-set, we can hot remove a physical memory by the following way. echo 1 /sys/bus/acpi/devices/PNP/eject In this case, acpi_bus_hot_remove_device() tries to remove memory device by acpi_bus_trim(). But if the memory has irremovable memory, memory hot remove fails. And the memory remains in kernel. However acpi_bus_trim() cannot notice that memory hot remove fails and retruns 0. So acpi_bus_hot_remove_device() continues to remove memory devices and sends _EJ0 method to firmware. Thus the memory device cannot be used. But the memory remains in kernel yet. So if someone access the memory, kernel panic occurs. Thanks, Yasuaki Ishimatsu If the kernel wants to unbind a device, why would we ever not want that to happen? So, NAK on this patch, sorry. Fix up the ACPI core to handle this properly, don't mess with the driver core here. greg k-h -- To unsubscribe from this list: send the line unsubscribe linux-kernel in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH v2] acpi : acpi_bus_trim() stops removing devices when failing to remove the device
On Fri, Oct 26, 2012 at 04:33:49PM +0900, Yasuaki Ishimatsu wrote: Hi Greg, Sorry for late reply. 2012/10/20 2:59, Greg Kroah-Hartman wrote: On Fri, Oct 19, 2012 at 06:29:52AM +0200, Rafael J. Wysocki wrote: On Thursday 11 of October 2012 19:12:28 Yasuaki Ishimatsu wrote: acpi_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. Vasilis hit the bug at memory hotplug and reported it as follow: https://lkml.org/lkml/2012/9/26/318 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. Applying the patch, acpi_bus_trim() stops removing devices when failing to remove the device. But I think there is no impact with the exceptionof CPU and Memory hotplug path. Because other device also fails but the fail is an irregular case like device is NULL. v1-v2 - add a rollback for reinstalling a notify handler. Signed-off-by: Yasuaki Ishimatsu isimatu.yasu...@jp.fujitsu.com Greg, do you think there may be any problems with the changes in dd.c? Yes, I don't like it. remove should always work, just like the exit call in a module. It means that the core wants to remove the driver, so it is going to happen, a driver can't refuse it. Which brings me to the larger question, why would this solve anything? Now we are developing physical memory hot plug. https://lkml.org/lkml/2012/10/23/213 So if we aplly the patch-set, we can hot remove a physical memory by the following way. echo 1 /sys/bus/acpi/devices/PNP/eject In this case, acpi_bus_hot_remove_device() tries to remove memory device by acpi_bus_trim(). But if the memory has irremovable memory, memory hot remove fails. And the memory remains in kernel. However acpi_bus_trim() cannot notice that memory hot remove fails and retruns 0. So acpi_bus_hot_remove_device() continues to remove memory devices and sends _EJ0 method to firmware. Thus the memory device cannot be used. But the memory remains in kernel yet. So if someone access the memory, kernel panic occurs. Why can't you check to find out if you can do the remove operation before you enter the driver core asking to actually remove the devices? That would allow you to know if you can do this before having to go through the whole operation. What happens if you can complete half of the removal, and do that, but not the whole thing? Don't you end up with half of the memory chunk gone from the system now? In other words, please solve this at a higher level than the driver core if at all possible. greg k-h -- To unsubscribe from this list: send the line unsubscribe linux-kernel in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH v2] acpi : acpi_bus_trim() stops removing devices when failing to remove the device
On Fri, Oct 19, 2012 at 06:29:52AM +0200, Rafael J. Wysocki wrote: > On Thursday 11 of October 2012 19:12:28 Yasuaki Ishimatsu wrote: > > acpi_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. > > > > Vasilis hit the bug at memory hotplug and reported it as follow: > > https://lkml.org/lkml/2012/9/26/318 > > > > 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. > > > > Applying the patch, acpi_bus_trim() stops removing devices when failing > > to remove the device. But I think there is no impact with the > > exceptionof CPU and Memory hotplug path. Because other device also fails > > but the fail is an irregular case like device is NULL. > > > > v1->v2 > > - add a rollback for reinstalling a notify handler. > > > > Signed-off-by: Yasuaki Ishimatsu > > Greg, do you think there may be any problems with the changes in dd.c? Yes, I don't like it. remove should always work, just like the exit call in a module. It means that the core wants to remove the driver, so it is going to happen, a driver can't refuse it. Which brings me to the larger question, why would this solve anything? If the kernel wants to unbind a device, why would we ever not want that to happen? So, NAK on this patch, sorry. Fix up the ACPI core to handle this properly, don't mess with the driver core here. greg k-h -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH v2] acpi : acpi_bus_trim() stops removing devices when failing to remove the device
On Fri, Oct 19, 2012 at 06:29:52AM +0200, Rafael J. Wysocki wrote: On Thursday 11 of October 2012 19:12:28 Yasuaki Ishimatsu wrote: acpi_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. Vasilis hit the bug at memory hotplug and reported it as follow: https://lkml.org/lkml/2012/9/26/318 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. Applying the patch, acpi_bus_trim() stops removing devices when failing to remove the device. But I think there is no impact with the exceptionof CPU and Memory hotplug path. Because other device also fails but the fail is an irregular case like device is NULL. v1-v2 - add a rollback for reinstalling a notify handler. Signed-off-by: Yasuaki Ishimatsu isimatu.yasu...@jp.fujitsu.com Greg, do you think there may be any problems with the changes in dd.c? Yes, I don't like it. remove should always work, just like the exit call in a module. It means that the core wants to remove the driver, so it is going to happen, a driver can't refuse it. Which brings me to the larger question, why would this solve anything? If the kernel wants to unbind a device, why would we ever not want that to happen? So, NAK on this patch, sorry. Fix up the ACPI core to handle this properly, don't mess with the driver core here. greg k-h -- To unsubscribe from this list: send the line unsubscribe linux-kernel in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH v2] acpi : acpi_bus_trim() stops removing devices when failing to remove the device
On Thursday 11 of October 2012 19:12:28 Yasuaki Ishimatsu wrote:
> acpi_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.
>
> Vasilis hit the bug at memory hotplug and reported it as follow:
> https://lkml.org/lkml/2012/9/26/318
>
> 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.
>
> Applying the patch, acpi_bus_trim() stops removing devices when failing
> to remove the device. But I think there is no impact with the
> exceptionof CPU and Memory hotplug path. Because other device also fails
> but the fail is an irregular case like device is NULL.
>
> v1->v2
> - add a rollback for reinstalling a notify handler.
>
> Signed-off-by: Yasuaki Ishimatsu
Greg, do you think there may be any problems with the changes in dd.c?
Rafael
> ---
> drivers/acpi/scan.c| 21 ++---
> drivers/base/dd.c | 22 +-
> include/linux/device.h |2 +-
> 3 files changed, 36 insertions(+), 9 deletions(-)
>
> Index: linux-3.6/drivers/acpi/scan.c
> ===
> --- linux-3.6.orig/drivers/acpi/scan.c2012-10-11 18:31:40.189019503
> +0900
> +++ linux-3.6/drivers/acpi/scan.c 2012-10-11 18:42:35.669041641 +0900
> @@ -445,18 +445,29 @@ 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)
> + goto rollback;
> + }
> }
> acpi_dev->driver = NULL;
> acpi_dev->driver_data = NULL;
>
> put_device(dev);
> return 0;
> +
> +rollback:
> + if (acpi_drv->ops.notify)
> + acpi_device_install_notify_handler(acpi_dev);
> +
> + return ret;
> }
>
> struct bus_type acpi_bus_type = {
> @@ -1226,11 +1237,15 @@ static int acpi_device_set_context(struc
>
> static 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);
> + ret = device_release_driver(>dev);
> + if (ret)
> + return ret;
>
> if (!rmdevice)
> return 0;
> Index: linux-3.6/drivers/base/dd.c
> ===
> --- linux-3.6.orig/drivers/base/dd.c 2012-10-11 18:31:40.191019505 +0900
> +++ linux-3.6/drivers/base/dd.c 2012-10-11 18:31:46.873020548 +0900
> @@ -475,9 +475,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 = 0;
>
> drv = dev->driver;
> if (drv) {
> @@ -493,9 +494,11 @@ static void __device_release_driver(stru
> pm_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;
> dev_set_drvdata(dev, NULL);
> @@ -506,6 +509,12 @@ static void __device_release_driver(stru
>dev);
>
> }
> +
> + return ret;
> +
> +rollback:
> + driver_sysfs_add(dev);
> + return ret;
> }
>
> /**
> @@ -515,16 +524,19 @@ static void __device_release_driver(stru
> * Manually detach device from driver.
> * When called for a USB interface,
Re: [PATCH v2] acpi : acpi_bus_trim() stops removing devices when failing to remove the device
On Thursday 11 of October 2012 19:12:28 Yasuaki Ishimatsu wrote:
acpi_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.
Vasilis hit the bug at memory hotplug and reported it as follow:
https://lkml.org/lkml/2012/9/26/318
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.
Applying the patch, acpi_bus_trim() stops removing devices when failing
to remove the device. But I think there is no impact with the
exceptionof CPU and Memory hotplug path. Because other device also fails
but the fail is an irregular case like device is NULL.
v1-v2
- add a rollback for reinstalling a notify handler.
Signed-off-by: Yasuaki Ishimatsu isimatu.yasu...@jp.fujitsu.com
Greg, do you think there may be any problems with the changes in dd.c?
Rafael
---
drivers/acpi/scan.c| 21 ++---
drivers/base/dd.c | 22 +-
include/linux/device.h |2 +-
3 files changed, 36 insertions(+), 9 deletions(-)
Index: linux-3.6/drivers/acpi/scan.c
===
--- linux-3.6.orig/drivers/acpi/scan.c2012-10-11 18:31:40.189019503
+0900
+++ linux-3.6/drivers/acpi/scan.c 2012-10-11 18:42:35.669041641 +0900
@@ -445,18 +445,29 @@ 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)
+ goto rollback;
+ }
}
acpi_dev-driver = NULL;
acpi_dev-driver_data = NULL;
put_device(dev);
return 0;
+
+rollback:
+ if (acpi_drv-ops.notify)
+ acpi_device_install_notify_handler(acpi_dev);
+
+ return ret;
}
struct bus_type acpi_bus_type = {
@@ -1226,11 +1237,15 @@ static int acpi_device_set_context(struc
static 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;
Index: linux-3.6/drivers/base/dd.c
===
--- linux-3.6.orig/drivers/base/dd.c 2012-10-11 18:31:40.191019505 +0900
+++ linux-3.6/drivers/base/dd.c 2012-10-11 18:31:46.873020548 +0900
@@ -475,9 +475,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 = 0;
drv = dev-driver;
if (drv) {
@@ -493,9 +494,11 @@ static void __device_release_driver(stru
pm_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;
dev_set_drvdata(dev, NULL);
@@ -506,6 +509,12 @@ static void __device_release_driver(stru
dev);
}
+
+ return ret;
+
+rollback:
+ driver_sysfs_add(dev);
+ return ret;
}
/**
@@ -515,16 +524,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
Re: [PATCH v2] acpi : acpi_bus_trim() stops removing devices when failing to remove the device
Hi Toshi,
2012/10/11 22:58, Toshi Kani wrote:
On Thu, 2012-10-11 at 19:12 +0900, Yasuaki Ishimatsu wrote:
acpi_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.
Vasilis hit the bug at memory hotplug and reported it as follow:
https://lkml.org/lkml/2012/9/26/318
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.
Applying the patch, acpi_bus_trim() stops removing devices when failing
to remove the device. But I think there is no impact with the
exceptionof CPU and Memory hotplug path. Because other device also fails
but the fail is an irregular case like device is NULL.
v1->v2
- add a rollback for reinstalling a notify handler.
Signed-off-by: Yasuaki Ishimatsu
Thanks for the update. Looks good.
Reviewed-by: Toshi Kani
Thank you for reviewing.
Thanks,
Yasauaki Ishimatsu
-Toshi
---
drivers/acpi/scan.c| 21 ++---
drivers/base/dd.c | 22 +-
include/linux/device.h |2 +-
3 files changed, 36 insertions(+), 9 deletions(-)
Index: linux-3.6/drivers/acpi/scan.c
===
--- linux-3.6.orig/drivers/acpi/scan.c 2012-10-11 18:31:40.189019503 +0900
+++ linux-3.6/drivers/acpi/scan.c 2012-10-11 18:42:35.669041641 +0900
@@ -445,18 +445,29 @@ 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)
+ goto rollback;
+ }
}
acpi_dev->driver = NULL;
acpi_dev->driver_data = NULL;
put_device(dev);
return 0;
+
+rollback:
+ if (acpi_drv->ops.notify)
+ acpi_device_install_notify_handler(acpi_dev);
+
+ return ret;
}
struct bus_type acpi_bus_type = {
@@ -1226,11 +1237,15 @@ static int acpi_device_set_context(struc
static 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);
+ ret = device_release_driver(>dev);
+ if (ret)
+ return ret;
if (!rmdevice)
return 0;
Index: linux-3.6/drivers/base/dd.c
===
--- linux-3.6.orig/drivers/base/dd.c2012-10-11 18:31:40.191019505 +0900
+++ linux-3.6/drivers/base/dd.c 2012-10-11 18:31:46.873020548 +0900
@@ -475,9 +475,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 = 0;
drv = dev->driver;
if (drv) {
@@ -493,9 +494,11 @@ static void __device_release_driver(stru
pm_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;
dev_set_drvdata(dev, NULL);
@@ -506,6 +509,12 @@ static void __device_release_driver(stru
dev);
}
+
+ return ret;
+
+rollback:
+ driver_sysfs_add(dev);
+ return ret;
}
/**
@@ -515,16 +524,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
Re: [PATCH v2] acpi : acpi_bus_trim() stops removing devices when failing to remove the device
On Thu, 2012-10-11 at 19:12 +0900, Yasuaki Ishimatsu wrote:
> acpi_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.
>
> Vasilis hit the bug at memory hotplug and reported it as follow:
> https://lkml.org/lkml/2012/9/26/318
>
> 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.
>
> Applying the patch, acpi_bus_trim() stops removing devices when failing
> to remove the device. But I think there is no impact with the
> exceptionof CPU and Memory hotplug path. Because other device also fails
> but the fail is an irregular case like device is NULL.
>
> v1->v2
> - add a rollback for reinstalling a notify handler.
>
> Signed-off-by: Yasuaki Ishimatsu
Thanks for the update. Looks good.
Reviewed-by: Toshi Kani
-Toshi
>
> ---
> drivers/acpi/scan.c| 21 ++---
> drivers/base/dd.c | 22 +-
> include/linux/device.h |2 +-
> 3 files changed, 36 insertions(+), 9 deletions(-)
>
> Index: linux-3.6/drivers/acpi/scan.c
> ===
> --- linux-3.6.orig/drivers/acpi/scan.c2012-10-11 18:31:40.189019503
> +0900
> +++ linux-3.6/drivers/acpi/scan.c 2012-10-11 18:42:35.669041641 +0900
> @@ -445,18 +445,29 @@ 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)
> + goto rollback;
> + }
> }
> acpi_dev->driver = NULL;
> acpi_dev->driver_data = NULL;
>
> put_device(dev);
> return 0;
> +
> +rollback:
> + if (acpi_drv->ops.notify)
> + acpi_device_install_notify_handler(acpi_dev);
> +
> + return ret;
> }
>
> struct bus_type acpi_bus_type = {
> @@ -1226,11 +1237,15 @@ static int acpi_device_set_context(struc
>
> static 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);
> + ret = device_release_driver(>dev);
> + if (ret)
> + return ret;
>
> if (!rmdevice)
> return 0;
> Index: linux-3.6/drivers/base/dd.c
> ===
> --- linux-3.6.orig/drivers/base/dd.c 2012-10-11 18:31:40.191019505 +0900
> +++ linux-3.6/drivers/base/dd.c 2012-10-11 18:31:46.873020548 +0900
> @@ -475,9 +475,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 = 0;
>
> drv = dev->driver;
> if (drv) {
> @@ -493,9 +494,11 @@ static void __device_release_driver(stru
> pm_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;
> dev_set_drvdata(dev, NULL);
> @@ -506,6 +509,12 @@ static void __device_release_driver(stru
>dev);
>
> }
> +
> + return ret;
> +
> +rollback:
> + driver_sysfs_add(dev);
> + return ret;
> }
>
> /**
> @@ -515,16 +524,19 @@ static void __device_release_driver(stru
> * Manually detach device from driver.
> * When called for a USB interface, @dev->parent lock
[PATCH v2] acpi : acpi_bus_trim() stops removing devices when failing to remove the device
acpi_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.
Vasilis hit the bug at memory hotplug and reported it as follow:
https://lkml.org/lkml/2012/9/26/318
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.
Applying the patch, acpi_bus_trim() stops removing devices when failing
to remove the device. But I think there is no impact with the
exceptionof CPU and Memory hotplug path. Because other device also fails
but the fail is an irregular case like device is NULL.
v1->v2
- add a rollback for reinstalling a notify handler.
Signed-off-by: Yasuaki Ishimatsu
---
drivers/acpi/scan.c| 21 ++---
drivers/base/dd.c | 22 +-
include/linux/device.h |2 +-
3 files changed, 36 insertions(+), 9 deletions(-)
Index: linux-3.6/drivers/acpi/scan.c
===
--- linux-3.6.orig/drivers/acpi/scan.c 2012-10-11 18:31:40.189019503 +0900
+++ linux-3.6/drivers/acpi/scan.c 2012-10-11 18:42:35.669041641 +0900
@@ -445,18 +445,29 @@ 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)
+ goto rollback;
+ }
}
acpi_dev->driver = NULL;
acpi_dev->driver_data = NULL;
put_device(dev);
return 0;
+
+rollback:
+ if (acpi_drv->ops.notify)
+ acpi_device_install_notify_handler(acpi_dev);
+
+ return ret;
}
struct bus_type acpi_bus_type = {
@@ -1226,11 +1237,15 @@ static int acpi_device_set_context(struc
static 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);
+ ret = device_release_driver(>dev);
+ if (ret)
+ return ret;
if (!rmdevice)
return 0;
Index: linux-3.6/drivers/base/dd.c
===
--- linux-3.6.orig/drivers/base/dd.c2012-10-11 18:31:40.191019505 +0900
+++ linux-3.6/drivers/base/dd.c 2012-10-11 18:31:46.873020548 +0900
@@ -475,9 +475,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 = 0;
drv = dev->driver;
if (drv) {
@@ -493,9 +494,11 @@ static void __device_release_driver(stru
pm_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;
dev_set_drvdata(dev, NULL);
@@ -506,6 +509,12 @@ static void __device_release_driver(stru
dev);
}
+
+ return ret;
+
+rollback:
+ driver_sysfs_add(dev);
+ return ret;
}
/**
@@ -515,16 +524,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
*
[PATCH v2] acpi : acpi_bus_trim() stops removing devices when failing to remove the device
acpi_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.
Vasilis hit the bug at memory hotplug and reported it as follow:
https://lkml.org/lkml/2012/9/26/318
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.
Applying the patch, acpi_bus_trim() stops removing devices when failing
to remove the device. But I think there is no impact with the
exceptionof CPU and Memory hotplug path. Because other device also fails
but the fail is an irregular case like device is NULL.
v1-v2
- add a rollback for reinstalling a notify handler.
Signed-off-by: Yasuaki Ishimatsu isimatu.yasu...@jp.fujitsu.com
---
drivers/acpi/scan.c| 21 ++---
drivers/base/dd.c | 22 +-
include/linux/device.h |2 +-
3 files changed, 36 insertions(+), 9 deletions(-)
Index: linux-3.6/drivers/acpi/scan.c
===
--- linux-3.6.orig/drivers/acpi/scan.c 2012-10-11 18:31:40.189019503 +0900
+++ linux-3.6/drivers/acpi/scan.c 2012-10-11 18:42:35.669041641 +0900
@@ -445,18 +445,29 @@ 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)
+ goto rollback;
+ }
}
acpi_dev-driver = NULL;
acpi_dev-driver_data = NULL;
put_device(dev);
return 0;
+
+rollback:
+ if (acpi_drv-ops.notify)
+ acpi_device_install_notify_handler(acpi_dev);
+
+ return ret;
}
struct bus_type acpi_bus_type = {
@@ -1226,11 +1237,15 @@ static int acpi_device_set_context(struc
static 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;
Index: linux-3.6/drivers/base/dd.c
===
--- linux-3.6.orig/drivers/base/dd.c2012-10-11 18:31:40.191019505 +0900
+++ linux-3.6/drivers/base/dd.c 2012-10-11 18:31:46.873020548 +0900
@@ -475,9 +475,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 = 0;
drv = dev-driver;
if (drv) {
@@ -493,9 +494,11 @@ static void __device_release_driver(stru
pm_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;
dev_set_drvdata(dev, NULL);
@@ -506,6 +509,12 @@ static void __device_release_driver(stru
dev);
}
+
+ return ret;
+
+rollback:
+ driver_sysfs_add(dev);
+ return ret;
}
/**
@@ -515,16 +524,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
*
Re: [PATCH v2] acpi : acpi_bus_trim() stops removing devices when failing to remove the device
On Thu, 2012-10-11 at 19:12 +0900, Yasuaki Ishimatsu wrote:
acpi_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.
Vasilis hit the bug at memory hotplug and reported it as follow:
https://lkml.org/lkml/2012/9/26/318
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.
Applying the patch, acpi_bus_trim() stops removing devices when failing
to remove the device. But I think there is no impact with the
exceptionof CPU and Memory hotplug path. Because other device also fails
but the fail is an irregular case like device is NULL.
v1-v2
- add a rollback for reinstalling a notify handler.
Signed-off-by: Yasuaki Ishimatsu isimatu.yasu...@jp.fujitsu.com
Thanks for the update. Looks good.
Reviewed-by: Toshi Kani toshi.k...@hp.com
-Toshi
---
drivers/acpi/scan.c| 21 ++---
drivers/base/dd.c | 22 +-
include/linux/device.h |2 +-
3 files changed, 36 insertions(+), 9 deletions(-)
Index: linux-3.6/drivers/acpi/scan.c
===
--- linux-3.6.orig/drivers/acpi/scan.c2012-10-11 18:31:40.189019503
+0900
+++ linux-3.6/drivers/acpi/scan.c 2012-10-11 18:42:35.669041641 +0900
@@ -445,18 +445,29 @@ 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)
+ goto rollback;
+ }
}
acpi_dev-driver = NULL;
acpi_dev-driver_data = NULL;
put_device(dev);
return 0;
+
+rollback:
+ if (acpi_drv-ops.notify)
+ acpi_device_install_notify_handler(acpi_dev);
+
+ return ret;
}
struct bus_type acpi_bus_type = {
@@ -1226,11 +1237,15 @@ static int acpi_device_set_context(struc
static 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;
Index: linux-3.6/drivers/base/dd.c
===
--- linux-3.6.orig/drivers/base/dd.c 2012-10-11 18:31:40.191019505 +0900
+++ linux-3.6/drivers/base/dd.c 2012-10-11 18:31:46.873020548 +0900
@@ -475,9 +475,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 = 0;
drv = dev-driver;
if (drv) {
@@ -493,9 +494,11 @@ static void __device_release_driver(stru
pm_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;
dev_set_drvdata(dev, NULL);
@@ -506,6 +509,12 @@ static void __device_release_driver(stru
dev);
}
+
+ return ret;
+
+rollback:
+ driver_sysfs_add(dev);
+ return ret;
}
/**
@@ -515,16 +524,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
Re: [PATCH v2] acpi : acpi_bus_trim() stops removing devices when failing to remove the device
Hi Toshi,
2012/10/11 22:58, Toshi Kani wrote:
On Thu, 2012-10-11 at 19:12 +0900, Yasuaki Ishimatsu wrote:
acpi_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.
Vasilis hit the bug at memory hotplug and reported it as follow:
https://lkml.org/lkml/2012/9/26/318
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.
Applying the patch, acpi_bus_trim() stops removing devices when failing
to remove the device. But I think there is no impact with the
exceptionof CPU and Memory hotplug path. Because other device also fails
but the fail is an irregular case like device is NULL.
v1-v2
- add a rollback for reinstalling a notify handler.
Signed-off-by: Yasuaki Ishimatsu isimatu.yasu...@jp.fujitsu.com
Thanks for the update. Looks good.
Reviewed-by: Toshi Kani toshi.k...@hp.com
Thank you for reviewing.
Thanks,
Yasauaki Ishimatsu
-Toshi
---
drivers/acpi/scan.c| 21 ++---
drivers/base/dd.c | 22 +-
include/linux/device.h |2 +-
3 files changed, 36 insertions(+), 9 deletions(-)
Index: linux-3.6/drivers/acpi/scan.c
===
--- linux-3.6.orig/drivers/acpi/scan.c 2012-10-11 18:31:40.189019503 +0900
+++ linux-3.6/drivers/acpi/scan.c 2012-10-11 18:42:35.669041641 +0900
@@ -445,18 +445,29 @@ 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)
+ goto rollback;
+ }
}
acpi_dev-driver = NULL;
acpi_dev-driver_data = NULL;
put_device(dev);
return 0;
+
+rollback:
+ if (acpi_drv-ops.notify)
+ acpi_device_install_notify_handler(acpi_dev);
+
+ return ret;
}
struct bus_type acpi_bus_type = {
@@ -1226,11 +1237,15 @@ static int acpi_device_set_context(struc
static 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;
Index: linux-3.6/drivers/base/dd.c
===
--- linux-3.6.orig/drivers/base/dd.c2012-10-11 18:31:40.191019505 +0900
+++ linux-3.6/drivers/base/dd.c 2012-10-11 18:31:46.873020548 +0900
@@ -475,9 +475,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 = 0;
drv = dev-driver;
if (drv) {
@@ -493,9 +494,11 @@ static void __device_release_driver(stru
pm_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;
dev_set_drvdata(dev, NULL);
@@ -506,6 +509,12 @@ static void __device_release_driver(stru
dev);
}
+
+ return ret;
+
+rollback:
+ driver_sysfs_add(dev);
+ return ret;
}
/**
@@ -515,16 +524,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
Re: acpi : acpi_bus_trim() stops removing devices when failing to remove the device
Hi Toshi,
2012/10/10 22:01, Toshi Kani wrote:
On Wed, 2012-10-10 at 10:07 +0900, Yasuaki Ishimatsu wrote:
:
if (acpi_drv) {
if (acpi_drv->ops.notify)
acpi_device_remove_notify_handler(acpi_dev);
THIS CALL
- 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)
Hi Yasuaki,
Shouldn't the notify handler be reinstalled here if it was removed by
the acpi_device_remove_notify_handler() above?
I do not reinstall the notify handler.
The function has not been removed on linux-3.6. And the patch is created
on linux-3.6. So the function remains in the patch.
Umm... I am not sure what you meant. Let me clarify my comment. When
acpi_drv->ops.remove() failed, I thought we would need to roll-back the
procedure done by the acpi_device_remove_notify_handler() call, which I
indicated as "THIS CALL" above. So, in this error path, don't we need
something like below?
if (acpi_drv->ops.notify)
acpi_device_install_notify_handler(acpi_dev)
I understood what you said. I'll update it.
Thanks,
Yasuaki Ishimatsu
Thanks,
-Toshi
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Re: acpi : acpi_bus_trim() stops removing devices when failing to remove the device
On Wed, 2012-10-10 at 10:07 +0900, Yasuaki Ishimatsu wrote:
:
> >>if (acpi_drv) {
> >>if (acpi_drv->ops.notify)
> >>acpi_device_remove_notify_handler(acpi_dev);
THIS CALL
> >> - 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)
> >
> > Hi Yasuaki,
> >
> > Shouldn't the notify handler be reinstalled here if it was removed by
> > the acpi_device_remove_notify_handler() above?
>
> I do not reinstall the notify handler.
> The function has not been removed on linux-3.6. And the patch is created
> on linux-3.6. So the function remains in the patch.
Umm... I am not sure what you meant. Let me clarify my comment. When
acpi_drv->ops.remove() failed, I thought we would need to roll-back the
procedure done by the acpi_device_remove_notify_handler() call, which I
indicated as "THIS CALL" above. So, in this error path, don't we need
something like below?
if (acpi_drv->ops.notify)
acpi_device_install_notify_handler(acpi_dev)
Thanks,
-Toshi
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Re: acpi : acpi_bus_trim() stops removing devices when failing to remove the device
On Wed, 2012-10-10 at 10:07 +0900, Yasuaki Ishimatsu wrote:
:
if (acpi_drv) {
if (acpi_drv-ops.notify)
acpi_device_remove_notify_handler(acpi_dev);
THIS CALL
- 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)
Hi Yasuaki,
Shouldn't the notify handler be reinstalled here if it was removed by
the acpi_device_remove_notify_handler() above?
I do not reinstall the notify handler.
The function has not been removed on linux-3.6. And the patch is created
on linux-3.6. So the function remains in the patch.
Umm... I am not sure what you meant. Let me clarify my comment. When
acpi_drv-ops.remove() failed, I thought we would need to roll-back the
procedure done by the acpi_device_remove_notify_handler() call, which I
indicated as THIS CALL above. So, in this error path, don't we need
something like below?
if (acpi_drv-ops.notify)
acpi_device_install_notify_handler(acpi_dev)
Thanks,
-Toshi
--
To unsubscribe from this list: send the line unsubscribe linux-kernel in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Re: acpi : acpi_bus_trim() stops removing devices when failing to remove the device
Hi Toshi,
2012/10/10 22:01, Toshi Kani wrote:
On Wed, 2012-10-10 at 10:07 +0900, Yasuaki Ishimatsu wrote:
:
if (acpi_drv) {
if (acpi_drv-ops.notify)
acpi_device_remove_notify_handler(acpi_dev);
THIS CALL
- 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)
Hi Yasuaki,
Shouldn't the notify handler be reinstalled here if it was removed by
the acpi_device_remove_notify_handler() above?
I do not reinstall the notify handler.
The function has not been removed on linux-3.6. And the patch is created
on linux-3.6. So the function remains in the patch.
Umm... I am not sure what you meant. Let me clarify my comment. When
acpi_drv-ops.remove() failed, I thought we would need to roll-back the
procedure done by the acpi_device_remove_notify_handler() call, which I
indicated as THIS CALL above. So, in this error path, don't we need
something like below?
if (acpi_drv-ops.notify)
acpi_device_install_notify_handler(acpi_dev)
I understood what you said. I'll update it.
Thanks,
Yasuaki Ishimatsu
Thanks,
-Toshi
--
To unsubscribe from this list: send the line unsubscribe linux-kernel in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Re: acpi : acpi_bus_trim() stops removing devices when failing to remove the device
Hi Toshi,
2012/10/10 1:36, Toshi Kani wrote:
On Tue, 2012-10-09 at 17:48 +0900, Yasuaki Ishimatsu wrote:
acpi_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.
Vasilis hit the bug at memory hotplug and reported it as follow:
https://lkml.org/lkml/2012/9/26/318
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.
Applying the patch, acpi_bus_trim() stops removing devices when failing
to remove the device. But I think there is no impact with the
exceptionof CPU and Memory hotplug path. Because other device also fails
but the fail is an irregular case like device is NULL.
Signed-off-by: Yasuaki Ishimatsu
---
drivers/acpi/scan.c| 15 ---
drivers/base/dd.c | 22 +-
include/linux/device.h |2 +-
3 files changed, 30 insertions(+), 9 deletions(-)
Index: linux-3.6/drivers/acpi/scan.c
===
--- linux-3.6.orig/drivers/acpi/scan.c 2012-10-09 17:25:40.956496325 +0900
+++ linux-3.6/drivers/acpi/scan.c 2012-10-09 17:25:55.405497800 +0900
@@ -445,12 +445,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)
Hi Yasuaki,
Shouldn't the notify handler be reinstalled here if it was removed by
the acpi_device_remove_notify_handler() above?
I do not reinstall the notify handler.
The function has not been removed on linux-3.6. And the patch is created
on linux-3.6. So the function remains in the patch.
Thanks,
Yasuaki Ishimatsu
Thanks,
-Toshi
+ return ret;
+ }
}
acpi_dev->driver = NULL;
acpi_dev->driver_data = NULL;
@@ -1226,11 +1231,15 @@ static int acpi_device_set_context(struc
static 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);
+ ret = device_release_driver(>dev);
+ if (ret)
+ return ret;
if (!rmdevice)
return 0;
Index: linux-3.6/drivers/base/dd.c
===
--- linux-3.6.orig/drivers/base/dd.c2012-10-01 08:47:46.0 +0900
+++ linux-3.6/drivers/base/dd.c 2012-10-09 17:25:55.442497825 +0900
@@ -475,9 +475,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 = 0;
drv = dev->driver;
if (drv) {
@@ -493,9 +494,11 @@ static void __device_release_driver(stru
pm_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;
dev_set_drvdata(dev, NULL);
@@ -506,6 +509,12 @@ static void __device_release_driver(stru
dev);
}
+
+ return ret;
+
+rollback:
+ driver_sysfs_add(dev);
+ return ret;
}
/**
@@ -515,16 +524,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)
{
Re: acpi : acpi_bus_trim() stops removing devices when failing to remove the device
On Tue, 2012-10-09 at 17:48 +0900, Yasuaki Ishimatsu wrote:
> acpi_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.
>
> Vasilis hit the bug at memory hotplug and reported it as follow:
> https://lkml.org/lkml/2012/9/26/318
>
> 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.
>
> Applying the patch, acpi_bus_trim() stops removing devices when failing
> to remove the device. But I think there is no impact with the
> exceptionof CPU and Memory hotplug path. Because other device also fails
> but the fail is an irregular case like device is NULL.
>
> Signed-off-by: Yasuaki Ishimatsu
>
> ---
> drivers/acpi/scan.c| 15 ---
> drivers/base/dd.c | 22 +-
> include/linux/device.h |2 +-
> 3 files changed, 30 insertions(+), 9 deletions(-)
>
> Index: linux-3.6/drivers/acpi/scan.c
> ===
> --- linux-3.6.orig/drivers/acpi/scan.c2012-10-09 17:25:40.956496325
> +0900
> +++ linux-3.6/drivers/acpi/scan.c 2012-10-09 17:25:55.405497800 +0900
> @@ -445,12 +445,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)
Hi Yasuaki,
Shouldn't the notify handler be reinstalled here if it was removed by
the acpi_device_remove_notify_handler() above?
Thanks,
-Toshi
> + return ret;
> + }
> }
> acpi_dev->driver = NULL;
> acpi_dev->driver_data = NULL;
> @@ -1226,11 +1231,15 @@ static int acpi_device_set_context(struc
>
> static 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);
> + ret = device_release_driver(>dev);
> + if (ret)
> + return ret;
>
> if (!rmdevice)
> return 0;
> Index: linux-3.6/drivers/base/dd.c
> ===
> --- linux-3.6.orig/drivers/base/dd.c 2012-10-01 08:47:46.0 +0900
> +++ linux-3.6/drivers/base/dd.c 2012-10-09 17:25:55.442497825 +0900
> @@ -475,9 +475,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 = 0;
>
> drv = dev->driver;
> if (drv) {
> @@ -493,9 +494,11 @@ static void __device_release_driver(stru
> pm_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;
> dev_set_drvdata(dev, NULL);
> @@ -506,6 +509,12 @@ static void __device_release_driver(stru
>dev);
>
> }
> +
> + return ret;
> +
> +rollback:
> + driver_sysfs_add(dev);
> + return ret;
> }
>
> /**
> @@ -515,16 +524,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
>
acpi : acpi_bus_trim() stops removing devices when failing to remove the device
acpi_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.
Vasilis hit the bug at memory hotplug and reported it as follow:
https://lkml.org/lkml/2012/9/26/318
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.
Applying the patch, acpi_bus_trim() stops removing devices when failing
to remove the device. But I think there is no impact with the
exceptionof CPU and Memory hotplug path. Because other device also fails
but the fail is an irregular case like device is NULL.
Signed-off-by: Yasuaki Ishimatsu
---
drivers/acpi/scan.c| 15 ---
drivers/base/dd.c | 22 +-
include/linux/device.h |2 +-
3 files changed, 30 insertions(+), 9 deletions(-)
Index: linux-3.6/drivers/acpi/scan.c
===
--- linux-3.6.orig/drivers/acpi/scan.c 2012-10-09 17:25:40.956496325 +0900
+++ linux-3.6/drivers/acpi/scan.c 2012-10-09 17:25:55.405497800 +0900
@@ -445,12 +445,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;
@@ -1226,11 +1231,15 @@ static int acpi_device_set_context(struc
static 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);
+ ret = device_release_driver(>dev);
+ if (ret)
+ return ret;
if (!rmdevice)
return 0;
Index: linux-3.6/drivers/base/dd.c
===
--- linux-3.6.orig/drivers/base/dd.c2012-10-01 08:47:46.0 +0900
+++ linux-3.6/drivers/base/dd.c 2012-10-09 17:25:55.442497825 +0900
@@ -475,9 +475,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 = 0;
drv = dev->driver;
if (drv) {
@@ -493,9 +494,11 @@ static void __device_release_driver(stru
pm_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;
dev_set_drvdata(dev, NULL);
@@ -506,6 +509,12 @@ static void __device_release_driver(stru
dev);
}
+
+ return ret;
+
+rollback:
+ driver_sysfs_add(dev);
+ return ret;
}
/**
@@ -515,16 +524,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);
Index: linux-3.6/include/linux/device.h
Re: [PATCH v3 3/3] acpi : acpi_bus_trim() stops removing devices when failing to remove the device
Hi Wen,
2012/10/09 17:02, Wen Congyang wrote:
Hi, ishimatsu:
At 07/12/2012 07:28 PM, Yasuaki Ishimatsu Wrote:
acpi_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.
What is the status about this patch?
I need to update the description against Toshi's comment as follows:
"I agree with this change as driver's remove interface can fail.
However, there are other callers to this function, which do not check
the return value. I suppose there is no impact to the other paths since
you only changed the CPU hotplug path to fail properly, but please
confirm this is the case. I recommend documenting this change to the
change log."
I have already checked that the patch does not impact the other path
with the exception of CPU and Memory hotplug path. So I will adds the
result of investigation and following Vasislis's problem into the patch
and resend to lklml.
Vasilis Liaskovitis found a similar bug about the memory hotplug, and this patch
can fix this problem:
https://lkml.org/lkml/2012/9/26/318
Thanks,
Yasuaki Ishimatsu
Thanks
Wen Congyang
Signed-off-by: Yasuaki Ishimatsu
---
drivers/acpi/scan.c| 15 ---
drivers/base/dd.c | 22 +-
include/linux/device.h |2 +-
3 files changed, 30 insertions(+), 9 deletions(-)
Index: linux-3.5-rc6/drivers/acpi/scan.c
===
--- linux-3.5-rc6.orig/drivers/acpi/scan.c 2012-07-12 20:11:37.316443808
+0900
+++ linux-3.5-rc6/drivers/acpi/scan.c 2012-07-12 20:17:17.927185231 +0900
@@ -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(struc
static 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);
+ ret = device_release_driver(>dev);
+ if (ret)
+ return ret;
if (!rmdevice)
return 0;
Index: linux-3.5-rc6/drivers/base/dd.c
===
--- linux-3.5-rc6.orig/drivers/base/dd.c2012-07-12 20:11:37.316443808
+0900
+++ linux-3.5-rc6/drivers/base/dd.c 2012-07-12 20:17:17.928185218 +0900
@@ -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(stru
pm_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(>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
Re: [PATCH v3 3/3] acpi : acpi_bus_trim() stops removing devices when failing to remove the device
Hi, ishimatsu:
At 07/12/2012 07:28 PM, Yasuaki Ishimatsu Wrote:
> acpi_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.
What is the status about this patch?
Vasilis Liaskovitis found a similar bug about the memory hotplug, and this patch
can fix this problem:
https://lkml.org/lkml/2012/9/26/318
Thanks
Wen Congyang
>
> Signed-off-by: Yasuaki Ishimatsu
>
> ---
> drivers/acpi/scan.c| 15 ---
> drivers/base/dd.c | 22 +-
> include/linux/device.h |2 +-
> 3 files changed, 30 insertions(+), 9 deletions(-)
>
> Index: linux-3.5-rc6/drivers/acpi/scan.c
> ===
> --- linux-3.5-rc6.orig/drivers/acpi/scan.c2012-07-12 20:11:37.316443808
> +0900
> +++ linux-3.5-rc6/drivers/acpi/scan.c 2012-07-12 20:17:17.927185231 +0900
> @@ -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(struc
>
> static 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);
> + ret = device_release_driver(>dev);
> + if (ret)
> + return ret;
>
> if (!rmdevice)
> return 0;
> Index: linux-3.5-rc6/drivers/base/dd.c
> ===
> --- linux-3.5-rc6.orig/drivers/base/dd.c 2012-07-12 20:11:37.316443808
> +0900
> +++ linux-3.5-rc6/drivers/base/dd.c 2012-07-12 20:17:17.928185218 +0900
> @@ -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(stru
> pm_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(>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;
> }
>
Re: [PATCH v3 3/3] acpi : acpi_bus_trim() stops removing devices when failing to remove the device
Hi, ishimatsu:
At 07/12/2012 07:28 PM, Yasuaki Ishimatsu Wrote:
acpi_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.
What is the status about this patch?
Vasilis Liaskovitis found a similar bug about the memory hotplug, and this patch
can fix this problem:
https://lkml.org/lkml/2012/9/26/318
Thanks
Wen Congyang
Signed-off-by: Yasuaki Ishimatsu isimatu.yasu...@jp.fujitsu.com
---
drivers/acpi/scan.c| 15 ---
drivers/base/dd.c | 22 +-
include/linux/device.h |2 +-
3 files changed, 30 insertions(+), 9 deletions(-)
Index: linux-3.5-rc6/drivers/acpi/scan.c
===
--- linux-3.5-rc6.orig/drivers/acpi/scan.c2012-07-12 20:11:37.316443808
+0900
+++ linux-3.5-rc6/drivers/acpi/scan.c 2012-07-12 20:17:17.927185231 +0900
@@ -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(struc
static 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;
Index: linux-3.5-rc6/drivers/base/dd.c
===
--- linux-3.5-rc6.orig/drivers/base/dd.c 2012-07-12 20:11:37.316443808
+0900
+++ linux-3.5-rc6/drivers/base/dd.c 2012-07-12 20:17:17.928185218 +0900
@@ -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(stru
pm_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);
Index: linux-3.5-rc6/include/linux/device.h
Re: [PATCH v3 3/3] acpi : acpi_bus_trim() stops removing devices when failing to remove the device
Hi Wen,
2012/10/09 17:02, Wen Congyang wrote:
Hi, ishimatsu:
At 07/12/2012 07:28 PM, Yasuaki Ishimatsu Wrote:
acpi_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.
What is the status about this patch?
I need to update the description against Toshi's comment as follows:
I agree with this change as driver's remove interface can fail.
However, there are other callers to this function, which do not check
the return value. I suppose there is no impact to the other paths since
you only changed the CPU hotplug path to fail properly, but please
confirm this is the case. I recommend documenting this change to the
change log.
I have already checked that the patch does not impact the other path
with the exception of CPU and Memory hotplug path. So I will adds the
result of investigation and following Vasislis's problem into the patch
and resend to lklml.
Vasilis Liaskovitis found a similar bug about the memory hotplug, and this patch
can fix this problem:
https://lkml.org/lkml/2012/9/26/318
Thanks,
Yasuaki Ishimatsu
Thanks
Wen Congyang
Signed-off-by: Yasuaki Ishimatsu isimatu.yasu...@jp.fujitsu.com
---
drivers/acpi/scan.c| 15 ---
drivers/base/dd.c | 22 +-
include/linux/device.h |2 +-
3 files changed, 30 insertions(+), 9 deletions(-)
Index: linux-3.5-rc6/drivers/acpi/scan.c
===
--- linux-3.5-rc6.orig/drivers/acpi/scan.c 2012-07-12 20:11:37.316443808
+0900
+++ linux-3.5-rc6/drivers/acpi/scan.c 2012-07-12 20:17:17.927185231 +0900
@@ -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(struc
static 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;
Index: linux-3.5-rc6/drivers/base/dd.c
===
--- linux-3.5-rc6.orig/drivers/base/dd.c2012-07-12 20:11:37.316443808
+0900
+++ linux-3.5-rc6/drivers/base/dd.c 2012-07-12 20:17:17.928185218 +0900
@@ -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(stru
pm_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
acpi : acpi_bus_trim() stops removing devices when failing to remove the device
acpi_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.
Vasilis hit the bug at memory hotplug and reported it as follow:
https://lkml.org/lkml/2012/9/26/318
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.
Applying the patch, acpi_bus_trim() stops removing devices when failing
to remove the device. But I think there is no impact with the
exceptionof CPU and Memory hotplug path. Because other device also fails
but the fail is an irregular case like device is NULL.
Signed-off-by: Yasuaki Ishimatsu isimatu.yasu...@jp.fujitsu.com
---
drivers/acpi/scan.c| 15 ---
drivers/base/dd.c | 22 +-
include/linux/device.h |2 +-
3 files changed, 30 insertions(+), 9 deletions(-)
Index: linux-3.6/drivers/acpi/scan.c
===
--- linux-3.6.orig/drivers/acpi/scan.c 2012-10-09 17:25:40.956496325 +0900
+++ linux-3.6/drivers/acpi/scan.c 2012-10-09 17:25:55.405497800 +0900
@@ -445,12 +445,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;
@@ -1226,11 +1231,15 @@ static int acpi_device_set_context(struc
static 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;
Index: linux-3.6/drivers/base/dd.c
===
--- linux-3.6.orig/drivers/base/dd.c2012-10-01 08:47:46.0 +0900
+++ linux-3.6/drivers/base/dd.c 2012-10-09 17:25:55.442497825 +0900
@@ -475,9 +475,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 = 0;
drv = dev-driver;
if (drv) {
@@ -493,9 +494,11 @@ static void __device_release_driver(stru
pm_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;
dev_set_drvdata(dev, NULL);
@@ -506,6 +509,12 @@ static void __device_release_driver(stru
dev);
}
+
+ return ret;
+
+rollback:
+ driver_sysfs_add(dev);
+ return ret;
}
/**
@@ -515,16 +524,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);
Index:
Re: acpi : acpi_bus_trim() stops removing devices when failing to remove the device
On Tue, 2012-10-09 at 17:48 +0900, Yasuaki Ishimatsu wrote:
acpi_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.
Vasilis hit the bug at memory hotplug and reported it as follow:
https://lkml.org/lkml/2012/9/26/318
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.
Applying the patch, acpi_bus_trim() stops removing devices when failing
to remove the device. But I think there is no impact with the
exceptionof CPU and Memory hotplug path. Because other device also fails
but the fail is an irregular case like device is NULL.
Signed-off-by: Yasuaki Ishimatsu isimatu.yasu...@jp.fujitsu.com
---
drivers/acpi/scan.c| 15 ---
drivers/base/dd.c | 22 +-
include/linux/device.h |2 +-
3 files changed, 30 insertions(+), 9 deletions(-)
Index: linux-3.6/drivers/acpi/scan.c
===
--- linux-3.6.orig/drivers/acpi/scan.c2012-10-09 17:25:40.956496325
+0900
+++ linux-3.6/drivers/acpi/scan.c 2012-10-09 17:25:55.405497800 +0900
@@ -445,12 +445,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)
Hi Yasuaki,
Shouldn't the notify handler be reinstalled here if it was removed by
the acpi_device_remove_notify_handler() above?
Thanks,
-Toshi
+ return ret;
+ }
}
acpi_dev-driver = NULL;
acpi_dev-driver_data = NULL;
@@ -1226,11 +1231,15 @@ static int acpi_device_set_context(struc
static 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;
Index: linux-3.6/drivers/base/dd.c
===
--- linux-3.6.orig/drivers/base/dd.c 2012-10-01 08:47:46.0 +0900
+++ linux-3.6/drivers/base/dd.c 2012-10-09 17:25:55.442497825 +0900
@@ -475,9 +475,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 = 0;
drv = dev-driver;
if (drv) {
@@ -493,9 +494,11 @@ static void __device_release_driver(stru
pm_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;
dev_set_drvdata(dev, NULL);
@@ -506,6 +509,12 @@ static void __device_release_driver(stru
dev);
}
+
+ return ret;
+
+rollback:
+ driver_sysfs_add(dev);
+ return ret;
}
/**
@@ -515,16 +524,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.
*/
Re: acpi : acpi_bus_trim() stops removing devices when failing to remove the device
Hi Toshi,
2012/10/10 1:36, Toshi Kani wrote:
On Tue, 2012-10-09 at 17:48 +0900, Yasuaki Ishimatsu wrote:
acpi_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.
Vasilis hit the bug at memory hotplug and reported it as follow:
https://lkml.org/lkml/2012/9/26/318
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.
Applying the patch, acpi_bus_trim() stops removing devices when failing
to remove the device. But I think there is no impact with the
exceptionof CPU and Memory hotplug path. Because other device also fails
but the fail is an irregular case like device is NULL.
Signed-off-by: Yasuaki Ishimatsu isimatu.yasu...@jp.fujitsu.com
---
drivers/acpi/scan.c| 15 ---
drivers/base/dd.c | 22 +-
include/linux/device.h |2 +-
3 files changed, 30 insertions(+), 9 deletions(-)
Index: linux-3.6/drivers/acpi/scan.c
===
--- linux-3.6.orig/drivers/acpi/scan.c 2012-10-09 17:25:40.956496325 +0900
+++ linux-3.6/drivers/acpi/scan.c 2012-10-09 17:25:55.405497800 +0900
@@ -445,12 +445,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)
Hi Yasuaki,
Shouldn't the notify handler be reinstalled here if it was removed by
the acpi_device_remove_notify_handler() above?
I do not reinstall the notify handler.
The function has not been removed on linux-3.6. And the patch is created
on linux-3.6. So the function remains in the patch.
Thanks,
Yasuaki Ishimatsu
Thanks,
-Toshi
+ return ret;
+ }
}
acpi_dev-driver = NULL;
acpi_dev-driver_data = NULL;
@@ -1226,11 +1231,15 @@ static int acpi_device_set_context(struc
static 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;
Index: linux-3.6/drivers/base/dd.c
===
--- linux-3.6.orig/drivers/base/dd.c2012-10-01 08:47:46.0 +0900
+++ linux-3.6/drivers/base/dd.c 2012-10-09 17:25:55.442497825 +0900
@@ -475,9 +475,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 = 0;
drv = dev-driver;
if (drv) {
@@ -493,9 +494,11 @@ static void __device_release_driver(stru
pm_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;
dev_set_drvdata(dev, NULL);
@@ -506,6 +509,12 @@ static void __device_release_driver(stru
dev);
}
+
+ return ret;
+
+rollback:
+ driver_sysfs_add(dev);
+ return ret;
}
/**
@@ -515,16 +524,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
Re: [PATCH v4 3/3] acpi : acpi_bus_trim() stops removing devices when failing to remove the device
On Fri, 2012-07-13 at 17:53 +0900, Yasuaki Ishimatsu wrote:
> acpi_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.
>
> device_release_driver() can return error value by the patch. But the change
> does not impact other caller function excluding acpi_bus_trim(), since all
> of them does not check return value of device_releae_driver().
I think potential risk here is that __device_release_driver() now
performs rollback in case of error from driver's remove interface. I
agree with doing the rollback, but this leads to a different situation
if the caller does not check error from device_release_driver() and
proceeds the operation. So, we will need to make sure that:
- Other driver's remove interfaces do not fail (or very unlikely to
fail), or
- If other driver's remove interfaces failed, their end results are no
worse than today.
Thanks,
-Toshi
> Signed-off-by: Yasuaki Ishimatsu
>
> ---
> drivers/acpi/scan.c| 15 ---
> drivers/base/dd.c | 22 +-
> include/linux/device.h |2 +-
> 3 files changed, 30 insertions(+), 9 deletions(-)
>
> Index: linux-3.5-rc6/drivers/acpi/scan.c
> ===
> --- linux-3.5-rc6.orig/drivers/acpi/scan.c2012-07-13 15:10:46.136790418
> +0900
> +++ linux-3.5-rc6/drivers/acpi/scan.c 2012-07-13 15:12:41.364349387 +0900
> @@ -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(struc
>
> static 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);
> + ret = device_release_driver(>dev);
> + if (ret)
> + return ret;
>
> if (!rmdevice)
> return 0;
> Index: linux-3.5-rc6/drivers/base/dd.c
> ===
> --- linux-3.5-rc6.orig/drivers/base/dd.c 2012-07-13 15:10:46.136790418
> +0900
> +++ linux-3.5-rc6/drivers/base/dd.c 2012-07-13 15:14:13.895193383 +0900
> @@ -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 = 0;
>
> drv = dev->driver;
> if (drv) {
> @@ -482,9 +483,11 @@ static void __device_release_driver(stru
> pm_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(>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
[PATCH v4 3/3] acpi : acpi_bus_trim() stops removing devices when failing to remove the device
acpi_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.
device_release_driver() can return error value by the patch. But the change
does not impact other caller function excluding acpi_bus_trim(), since all
of them does not check return value of device_releae_driver().
Signed-off-by: Yasuaki Ishimatsu
---
drivers/acpi/scan.c| 15 ---
drivers/base/dd.c | 22 +-
include/linux/device.h |2 +-
3 files changed, 30 insertions(+), 9 deletions(-)
Index: linux-3.5-rc6/drivers/acpi/scan.c
===
--- linux-3.5-rc6.orig/drivers/acpi/scan.c 2012-07-13 15:10:46.136790418
+0900
+++ linux-3.5-rc6/drivers/acpi/scan.c 2012-07-13 15:12:41.364349387 +0900
@@ -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(struc
static 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);
+ ret = device_release_driver(>dev);
+ if (ret)
+ return ret;
if (!rmdevice)
return 0;
Index: linux-3.5-rc6/drivers/base/dd.c
===
--- linux-3.5-rc6.orig/drivers/base/dd.c2012-07-13 15:10:46.136790418
+0900
+++ linux-3.5-rc6/drivers/base/dd.c 2012-07-13 15:14:13.895193383 +0900
@@ -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 = 0;
drv = dev->driver;
if (drv) {
@@ -482,9 +483,11 @@ static void __device_release_driver(stru
pm_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(>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);
Index: linux-3.5-rc6/include/linux/device.h
===
--- linux-3.5-rc6.orig/include/linux/device.h
Re: [PATCH v3 3/3] acpi : acpi_bus_trim() stops removing devices when failing to remove the device
2012/07/13 1:50, Toshi Kani wrote:
On Thu, 2012-07-12 at 20:28 +0900, Yasuaki Ishimatsu wrote:
acpi_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.
Signed-off-by: Yasuaki Ishimatsu
---
drivers/acpi/scan.c| 15 ---
drivers/base/dd.c | 22 +-
include/linux/device.h |2 +-
3 files changed, 30 insertions(+), 9 deletions(-)
Index: linux-3.5-rc6/drivers/acpi/scan.c
===
--- linux-3.5-rc6.orig/drivers/acpi/scan.c 2012-07-12 20:11:37.316443808
+0900
+++ linux-3.5-rc6/drivers/acpi/scan.c 2012-07-12 20:17:17.927185231 +0900
@@ -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(struc
static 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);
+ ret = device_release_driver(>dev);
+ if (ret)
+ return ret;
if (!rmdevice)
return 0;
Index: linux-3.5-rc6/drivers/base/dd.c
===
--- linux-3.5-rc6.orig/drivers/base/dd.c2012-07-12 20:11:37.316443808
+0900
+++ linux-3.5-rc6/drivers/base/dd.c 2012-07-12 20:17:17.928185218 +0900
@@ -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(stru
pm_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(>p->knode_driver);
@@ -494,6 +497,12 @@ static void __device_release_driver(stru
dev);
}
+
+ return ret;
ret is uninitialized when !drv.
Thanks! I'll update it.
+
+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)
I agree with this change as driver's remove interface can fail.
However, there are other callers to this function, which do not check
the return value. I suppose there is no impact to the other paths since
you only changed the CPU hotplug path to fail properly, but please
confirm this is the case. I recommend documenting this change to the
change log.
Thank you for your agreement. As you know, there are other callers. I
believe the patch does not impact to them, since all of them does not
check return value of device_release_driver().
I will write it to the patch.
Thanks,
Yasuaki Ishimatsu
Thanks,
-Toshi
Re: [PATCH v3 3/3] acpi : acpi_bus_trim() stops removing devices when failing to remove the device
2012/07/13 1:50, Toshi Kani wrote:
On Thu, 2012-07-12 at 20:28 +0900, Yasuaki Ishimatsu wrote:
acpi_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.
Signed-off-by: Yasuaki Ishimatsu isimatu.yasu...@jp.fujitsu.com
---
drivers/acpi/scan.c| 15 ---
drivers/base/dd.c | 22 +-
include/linux/device.h |2 +-
3 files changed, 30 insertions(+), 9 deletions(-)
Index: linux-3.5-rc6/drivers/acpi/scan.c
===
--- linux-3.5-rc6.orig/drivers/acpi/scan.c 2012-07-12 20:11:37.316443808
+0900
+++ linux-3.5-rc6/drivers/acpi/scan.c 2012-07-12 20:17:17.927185231 +0900
@@ -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(struc
static 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;
Index: linux-3.5-rc6/drivers/base/dd.c
===
--- linux-3.5-rc6.orig/drivers/base/dd.c2012-07-12 20:11:37.316443808
+0900
+++ linux-3.5-rc6/drivers/base/dd.c 2012-07-12 20:17:17.928185218 +0900
@@ -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(stru
pm_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;
ret is uninitialized when !drv.
Thanks! I'll update it.
+
+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)
I agree with this change as driver's remove interface can fail.
However, there are other callers to this function, which do not check
the return value. I suppose there is no impact to the other paths since
you only changed the CPU hotplug path to fail properly, but please
confirm this is the case. I recommend documenting this change to the
change log.
Thank you for your agreement. As you know, there are other callers. I
believe the patch does not impact to them, since all of them does not
check return value of device_release_driver().
I will write it to the patch.
Thanks,
Yasuaki Ishimatsu
Thanks,
[PATCH v4 3/3] acpi : acpi_bus_trim() stops removing devices when failing to remove the device
acpi_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.
device_release_driver() can return error value by the patch. But the change
does not impact other caller function excluding acpi_bus_trim(), since all
of them does not check return value of device_releae_driver().
Signed-off-by: Yasuaki Ishimatsu isimatu.yasu...@jp.fujitsu.com
---
drivers/acpi/scan.c| 15 ---
drivers/base/dd.c | 22 +-
include/linux/device.h |2 +-
3 files changed, 30 insertions(+), 9 deletions(-)
Index: linux-3.5-rc6/drivers/acpi/scan.c
===
--- linux-3.5-rc6.orig/drivers/acpi/scan.c 2012-07-13 15:10:46.136790418
+0900
+++ linux-3.5-rc6/drivers/acpi/scan.c 2012-07-13 15:12:41.364349387 +0900
@@ -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(struc
static 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;
Index: linux-3.5-rc6/drivers/base/dd.c
===
--- linux-3.5-rc6.orig/drivers/base/dd.c2012-07-13 15:10:46.136790418
+0900
+++ linux-3.5-rc6/drivers/base/dd.c 2012-07-13 15:14:13.895193383 +0900
@@ -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 = 0;
drv = dev-driver;
if (drv) {
@@ -482,9 +483,11 @@ static void __device_release_driver(stru
pm_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);
Index: linux-3.5-rc6/include/linux/device.h
===
--- linux-3.5-rc6.orig/include/linux/device.h
Re: [PATCH v4 3/3] acpi : acpi_bus_trim() stops removing devices when failing to remove the device
On Fri, 2012-07-13 at 17:53 +0900, Yasuaki Ishimatsu wrote:
acpi_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.
device_release_driver() can return error value by the patch. But the change
does not impact other caller function excluding acpi_bus_trim(), since all
of them does not check return value of device_releae_driver().
I think potential risk here is that __device_release_driver() now
performs rollback in case of error from driver's remove interface. I
agree with doing the rollback, but this leads to a different situation
if the caller does not check error from device_release_driver() and
proceeds the operation. So, we will need to make sure that:
- Other driver's remove interfaces do not fail (or very unlikely to
fail), or
- If other driver's remove interfaces failed, their end results are no
worse than today.
Thanks,
-Toshi
Signed-off-by: Yasuaki Ishimatsu isimatu.yasu...@jp.fujitsu.com
---
drivers/acpi/scan.c| 15 ---
drivers/base/dd.c | 22 +-
include/linux/device.h |2 +-
3 files changed, 30 insertions(+), 9 deletions(-)
Index: linux-3.5-rc6/drivers/acpi/scan.c
===
--- linux-3.5-rc6.orig/drivers/acpi/scan.c2012-07-13 15:10:46.136790418
+0900
+++ linux-3.5-rc6/drivers/acpi/scan.c 2012-07-13 15:12:41.364349387 +0900
@@ -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(struc
static 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;
Index: linux-3.5-rc6/drivers/base/dd.c
===
--- linux-3.5-rc6.orig/drivers/base/dd.c 2012-07-13 15:10:46.136790418
+0900
+++ linux-3.5-rc6/drivers/base/dd.c 2012-07-13 15:14:13.895193383 +0900
@@ -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 = 0;
drv = dev-driver;
if (drv) {
@@ -482,9 +483,11 @@ static void __device_release_driver(stru
pm_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
Re: [PATCH v3 3/3] acpi : acpi_bus_trim() stops removing devices when failing to remove the device
On Thu, 2012-07-12 at 20:28 +0900, Yasuaki Ishimatsu wrote:
> acpi_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.
>
> Signed-off-by: Yasuaki Ishimatsu
>
> ---
> drivers/acpi/scan.c| 15 ---
> drivers/base/dd.c | 22 +-
> include/linux/device.h |2 +-
> 3 files changed, 30 insertions(+), 9 deletions(-)
>
> Index: linux-3.5-rc6/drivers/acpi/scan.c
> ===
> --- linux-3.5-rc6.orig/drivers/acpi/scan.c2012-07-12 20:11:37.316443808
> +0900
> +++ linux-3.5-rc6/drivers/acpi/scan.c 2012-07-12 20:17:17.927185231 +0900
> @@ -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(struc
>
> static 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);
> + ret = device_release_driver(>dev);
> + if (ret)
> + return ret;
>
> if (!rmdevice)
> return 0;
> Index: linux-3.5-rc6/drivers/base/dd.c
> ===
> --- linux-3.5-rc6.orig/drivers/base/dd.c 2012-07-12 20:11:37.316443808
> +0900
> +++ linux-3.5-rc6/drivers/base/dd.c 2012-07-12 20:17:17.928185218 +0900
> @@ -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(stru
> pm_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(>p->knode_driver);
> @@ -494,6 +497,12 @@ static void __device_release_driver(stru
>dev);
>
> }
> +
> + return ret;
ret is uninitialized when !drv.
> +
> +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)
I agree with this change as driver's remove interface can fail.
However, there are other callers to this function, which do not check
the return value. I suppose there is no impact to the other paths since
you only changed the CPU hotplug path to fail properly, but please
confirm this is the case. I recommend documenting this change to the
change log.
Thanks,
-Toshi
> {
> + int ret;
> /*
>* If anyone calls device_release_driver() recursively from
>* within their ->remove callback for the same device, they
>* will deadlock right here.
>*/
[PATCH v3 3/3] acpi : acpi_bus_trim() stops removing devices when failing to remove the device
acpi_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.
Signed-off-by: Yasuaki Ishimatsu
---
drivers/acpi/scan.c| 15 ---
drivers/base/dd.c | 22 +-
include/linux/device.h |2 +-
3 files changed, 30 insertions(+), 9 deletions(-)
Index: linux-3.5-rc6/drivers/acpi/scan.c
===
--- linux-3.5-rc6.orig/drivers/acpi/scan.c 2012-07-12 20:11:37.316443808
+0900
+++ linux-3.5-rc6/drivers/acpi/scan.c 2012-07-12 20:17:17.927185231 +0900
@@ -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(struc
static 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);
+ ret = device_release_driver(>dev);
+ if (ret)
+ return ret;
if (!rmdevice)
return 0;
Index: linux-3.5-rc6/drivers/base/dd.c
===
--- linux-3.5-rc6.orig/drivers/base/dd.c2012-07-12 20:11:37.316443808
+0900
+++ linux-3.5-rc6/drivers/base/dd.c 2012-07-12 20:17:17.928185218 +0900
@@ -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(stru
pm_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(>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);
Index: linux-3.5-rc6/include/linux/device.h
===
--- linux-3.5-rc6.orig/include/linux/device.h 2012-07-12 20:11:37.317443779
+0900
+++ linux-3.5-rc6/include/linux/device.h2012-07-12 20:17:17.936185118
+0900
@@ -827,7 +827,7 @@ static inline void *dev_get_platdata(con
* for information on use.
*/
extern
[PATCH v3 3/3] acpi : acpi_bus_trim() stops removing devices when failing to remove the device
acpi_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.
Signed-off-by: Yasuaki Ishimatsu isimatu.yasu...@jp.fujitsu.com
---
drivers/acpi/scan.c| 15 ---
drivers/base/dd.c | 22 +-
include/linux/device.h |2 +-
3 files changed, 30 insertions(+), 9 deletions(-)
Index: linux-3.5-rc6/drivers/acpi/scan.c
===
--- linux-3.5-rc6.orig/drivers/acpi/scan.c 2012-07-12 20:11:37.316443808
+0900
+++ linux-3.5-rc6/drivers/acpi/scan.c 2012-07-12 20:17:17.927185231 +0900
@@ -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(struc
static 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;
Index: linux-3.5-rc6/drivers/base/dd.c
===
--- linux-3.5-rc6.orig/drivers/base/dd.c2012-07-12 20:11:37.316443808
+0900
+++ linux-3.5-rc6/drivers/base/dd.c 2012-07-12 20:17:17.928185218 +0900
@@ -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(stru
pm_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);
Index: linux-3.5-rc6/include/linux/device.h
===
--- linux-3.5-rc6.orig/include/linux/device.h 2012-07-12 20:11:37.317443779
+0900
+++ linux-3.5-rc6/include/linux/device.h2012-07-12 20:17:17.936185118
+0900
@@ -827,7 +827,7 @@ static inline void *dev_get_platdata(con
* for information on use.
*/
Re: [PATCH v3 3/3] acpi : acpi_bus_trim() stops removing devices when failing to remove the device
On Thu, 2012-07-12 at 20:28 +0900, Yasuaki Ishimatsu wrote:
acpi_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.
Signed-off-by: Yasuaki Ishimatsu isimatu.yasu...@jp.fujitsu.com
---
drivers/acpi/scan.c| 15 ---
drivers/base/dd.c | 22 +-
include/linux/device.h |2 +-
3 files changed, 30 insertions(+), 9 deletions(-)
Index: linux-3.5-rc6/drivers/acpi/scan.c
===
--- linux-3.5-rc6.orig/drivers/acpi/scan.c2012-07-12 20:11:37.316443808
+0900
+++ linux-3.5-rc6/drivers/acpi/scan.c 2012-07-12 20:17:17.927185231 +0900
@@ -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(struc
static 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;
Index: linux-3.5-rc6/drivers/base/dd.c
===
--- linux-3.5-rc6.orig/drivers/base/dd.c 2012-07-12 20:11:37.316443808
+0900
+++ linux-3.5-rc6/drivers/base/dd.c 2012-07-12 20:17:17.928185218 +0900
@@ -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(stru
pm_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;
ret is uninitialized when !drv.
+
+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)
I agree with this change as driver's remove interface can fail.
However, there are other callers to this function, which do not check
the return value. I suppose there is no impact to the other paths since
you only changed the CPU hotplug path to fail properly, but please
confirm this is the case. I recommend documenting this change to the
change log.
Thanks,
-Toshi
{
+ 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);
[PATCH v2 3/3] acpi : acpi_bus_trim() stops removing devices when failing to remove the device
acpi_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.
Signed-off-by: Yasuaki Ishimatsu
---
drivers/acpi/scan.c| 15 ---
drivers/base/dd.c | 22 +-
include/linux/device.h |2 +-
3 files changed, 30 insertions(+), 9 deletions(-)
Index: linux-3.5-rc4/drivers/acpi/scan.c
===
--- linux-3.5-rc4.orig/drivers/acpi/scan.c 2012-07-05 21:14:39.196187725
+0900
+++ linux-3.5-rc4/drivers/acpi/scan.c 2012-07-05 21:15:06.077851657 +0900
@@ -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(struc
static 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);
+ ret = device_release_driver(>dev);
+ if (ret)
+ return ret;
if (!rmdevice)
return 0;
Index: linux-3.5-rc4/drivers/base/dd.c
===
--- linux-3.5-rc4.orig/drivers/base/dd.c2012-07-05 21:14:39.192187776
+0900
+++ linux-3.5-rc4/drivers/base/dd.c 2012-07-05 21:15:06.079851633 +0900
@@ -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(stru
pm_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(>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);
Index: linux-3.5-rc4/include/linux/device.h
===
--- linux-3.5-rc4.orig/include/linux/device.h 2012-07-05 21:14:39.202187649
+0900
+++ linux-3.5-rc4/include/linux/device.h2012-07-05 21:15:06.095851431
+0900
@@ -827,7 +827,7 @@ static inline void *dev_get_platdata(con
* for information on use.
*/
extern
[PATCH v2 3/3] acpi : acpi_bus_trim() stops removing devices when failing to remove the device
acpi_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.
Signed-off-by: Yasuaki Ishimatsu isimatu.yasu...@jp.fujitsu.com
---
drivers/acpi/scan.c| 15 ---
drivers/base/dd.c | 22 +-
include/linux/device.h |2 +-
3 files changed, 30 insertions(+), 9 deletions(-)
Index: linux-3.5-rc4/drivers/acpi/scan.c
===
--- linux-3.5-rc4.orig/drivers/acpi/scan.c 2012-07-05 21:14:39.196187725
+0900
+++ linux-3.5-rc4/drivers/acpi/scan.c 2012-07-05 21:15:06.077851657 +0900
@@ -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(struc
static 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;
Index: linux-3.5-rc4/drivers/base/dd.c
===
--- linux-3.5-rc4.orig/drivers/base/dd.c2012-07-05 21:14:39.192187776
+0900
+++ linux-3.5-rc4/drivers/base/dd.c 2012-07-05 21:15:06.079851633 +0900
@@ -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(stru
pm_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);
Index: linux-3.5-rc4/include/linux/device.h
===
--- linux-3.5-rc4.orig/include/linux/device.h 2012-07-05 21:14:39.202187649
+0900
+++ linux-3.5-rc4/include/linux/device.h2012-07-05 21:15:06.095851431
+0900
@@ -827,7 +827,7 @@ static inline void *dev_get_platdata(con
* for information on use.
*/
