From: Yasuaki Ishimatsu <isimatu.yasuaki@jp.fujitsu.com> To: Wen Congyang <wency@cn.fujitsu.com> Cc: linux-acpi@vger.kernel.org, linux-kernel@vger.kernel.org, lenb@kernel.org, srivatsa.bhat@linux.vnet.ibm.com, toshi.kani@hp.com Subject: Re: [PATCH v3 3/3] acpi : acpi_bus_trim() stops removing devices when failing to remove the device Date: Tue, 9 Oct 2012 17:24:04 +0900 [thread overview] Message-ID: <5073DF24.4090006@jp.fujitsu.com> (raw) In-Reply-To: <5073DA15.4040501@cn.fujitsu.com> 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.yasuaki@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.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 >> =================================================================== >> --- linux-3.5-rc6.orig/include/linux/device.h 2012-07-12 20:11:37.317443779 +0900 >> +++ linux-3.5-rc6/include/linux/device.h 2012-07-12 20:17:17.936185118 +0900 >> @@ -827,7 +827,7 @@ static inline void *dev_get_platdata(con >> * for information on use. >> */ >> extern int __must_check device_bind_driver(struct device *dev); >> -extern void device_release_driver(struct device *dev); >> +extern int device_release_driver(struct device *dev); >> extern int __must_check device_attach(struct device *dev); >> extern int __must_check driver_attach(struct device_driver *drv); >> extern int __must_check device_reprobe(struct device *dev); >> >> -- >> To unsubscribe from this list: send the line "unsubscribe linux-kernel" in >> the body of a message to majordomo@vger.kernel.org >> More majordomo info at http://vger.kernel.org/majordomo-info.html >> Please read the FAQ at http://www.tux.org/lkml/ >> >
WARNING: multiple messages have this Message-ID (diff)
From: Yasuaki Ishimatsu <isimatu.yasuaki@jp.fujitsu.com> To: Wen Congyang <wency@cn.fujitsu.com> Cc: <linux-acpi@vger.kernel.org>, <linux-kernel@vger.kernel.org>, <lenb@kernel.org>, <srivatsa.bhat@linux.vnet.ibm.com>, <toshi.kani@hp.com> Subject: Re: [PATCH v3 3/3] acpi : acpi_bus_trim() stops removing devices when failing to remove the device Date: Tue, 9 Oct 2012 17:24:04 +0900 [thread overview] Message-ID: <5073DF24.4090006@jp.fujitsu.com> (raw) In-Reply-To: <5073DA15.4040501@cn.fujitsu.com> 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.yasuaki@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.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 >> =================================================================== >> --- linux-3.5-rc6.orig/include/linux/device.h 2012-07-12 20:11:37.317443779 +0900 >> +++ linux-3.5-rc6/include/linux/device.h 2012-07-12 20:17:17.936185118 +0900 >> @@ -827,7 +827,7 @@ static inline void *dev_get_platdata(con >> * for information on use. >> */ >> extern int __must_check device_bind_driver(struct device *dev); >> -extern void device_release_driver(struct device *dev); >> +extern int device_release_driver(struct device *dev); >> extern int __must_check device_attach(struct device *dev); >> extern int __must_check driver_attach(struct device_driver *drv); >> extern int __must_check device_reprobe(struct device *dev); >> >> -- >> To unsubscribe from this list: send the line "unsubscribe linux-kernel" in >> the body of a message to majordomo@vger.kernel.org >> More majordomo info at http://vger.kernel.org/majordomo-info.html >> Please read the FAQ at http://www.tux.org/lkml/ >> >
next prev parent reply other threads:[~2012-10-09 8:24 UTC|newest] Thread overview: 26+ messages / expand[flat|nested] mbox.gz Atom feed top 2012-07-12 11:22 [PATCH v3 1/3] acpi : cpu hot-remove returns error when cpu_down() fails Yasuaki Ishimatsu 2012-07-12 11:22 ` Yasuaki Ishimatsu 2012-07-12 11:27 ` [PATCH v3 2/3] acpi : prevent cpu from becoming online Yasuaki Ishimatsu 2012-07-12 11:27 ` Yasuaki Ishimatsu 2012-07-12 11:40 ` [PATCH v3 2/3 RESEND] " Yasuaki Ishimatsu 2012-07-12 11:40 ` Yasuaki Ishimatsu 2012-07-12 12:41 ` Srivatsa S. Bhat 2012-07-13 6:24 ` Yasuaki Ishimatsu 2012-07-13 6:24 ` Yasuaki Ishimatsu 2012-07-12 16:49 ` Toshi Kani 2012-07-13 6:27 ` Yasuaki Ishimatsu 2012-07-13 6:27 ` Yasuaki Ishimatsu 2012-07-12 11:28 ` [PATCH v3 3/3] acpi : acpi_bus_trim() stops removing devices when failing to remove the device Yasuaki Ishimatsu 2012-07-12 11:28 ` Yasuaki Ishimatsu 2012-07-12 16:50 ` Toshi Kani 2012-07-13 7:16 ` Yasuaki Ishimatsu 2012-07-13 7:16 ` Yasuaki Ishimatsu 2012-10-09 8:02 ` Wen Congyang 2012-10-09 8:24 ` Yasuaki Ishimatsu [this message] 2012-10-09 8:24 ` Yasuaki Ishimatsu 2012-07-12 12:32 ` [PATCH v3 1/3] acpi : cpu hot-remove returns error when cpu_down() fails Srivatsa S. Bhat 2012-07-13 6:29 ` Yasuaki Ishimatsu 2012-07-13 6:29 ` Yasuaki Ishimatsu 2012-07-12 16:48 ` Toshi Kani 2012-07-13 6:26 ` Yasuaki Ishimatsu 2012-07-13 6:26 ` Yasuaki Ishimatsu
Reply instructions: You may reply publicly to this message via plain-text email using any one of the following methods: * Save the following mbox file, import it into your mail client, and reply-to-all from there: mbox Avoid top-posting and favor interleaved quoting: https://en.wikipedia.org/wiki/Posting_style#Interleaved_style * Reply using the --to, --cc, and --in-reply-to switches of git-send-email(1): git send-email \ --in-reply-to=5073DF24.4090006@jp.fujitsu.com \ --to=isimatu.yasuaki@jp.fujitsu.com \ --cc=lenb@kernel.org \ --cc=linux-acpi@vger.kernel.org \ --cc=linux-kernel@vger.kernel.org \ --cc=srivatsa.bhat@linux.vnet.ibm.com \ --cc=toshi.kani@hp.com \ --cc=wency@cn.fujitsu.com \ /path/to/YOUR_REPLY https://kernel.org/pub/software/scm/git/docs/git-send-email.html * If your mail client supports setting the In-Reply-To header via mailto: links, try the mailto: linkBe sure your reply has a Subject: header at the top and a blank line before the message body.
This is an external index of several public inboxes, see mirroring instructions on how to clone and mirror all data and code used by this external index.