| Message ID | 20170403155533.30283-1-jlee@suse.com (mailing list archive) |
|---|---|
| State | Not Applicable, archived |
| Headers | show |
On Mon, Apr 3, 2017 at 5:55 PM, Lee, Chun-Yi <joeyli.kernel@gmail.com> wrote: > The caa73ea1 patch, "ACPI / hotplug / driver core: Handle containers > in a special way", introduced the offline callback of acpi container. > In the patch description, it mentions: > > For ACPI containers that callback simply walks the list of ACPI > device objects right below the container object (its children) and > checks if all of their physical companion devices are offline. If > that's not the case, it returns -EBUSY and the container system > devivce cannot be put offline. Consequently, to put the container > system device offline, it is necessary to put all of the physical > devices depending on its ACPI companion object offline beforehand. > > Looks that it means acpi_container_offline() should walks all physical > companion devices of container's children and checks their offline > state. And, the comment in source code is "Check all of the dependent > devices' physical companions", which means it should checks _all_ > physical companions. > > But, the checking code just stops at the first not-offlined physical > companion device of the first not-offlined child, then kernel only > emits KOBJ_CHANGE uevent to the one device. It doesn't really walk > all children's all physical companion devices and doesn't send change > uevent to them. It is unclear to me from the description whether or not this is a practical issue. Also there is an alternative, which is not to send KOBJ_CHANGE uevents to any children at all. Why is the approach you chose better? Thanks, Rafael -- To unsubscribe from this list: send the line "unsubscribe linux-acpi" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Hi, On Wed, Apr 19, 2017 at 02:30:18AM +0200, Rafael J. Wysocki wrote: > On Mon, Apr 3, 2017 at 5:55 PM, Lee, Chun-Yi <joeyli.kernel@gmail.com> wrote: > > The caa73ea1 patch, "ACPI / hotplug / driver core: Handle containers > > in a special way", introduced the offline callback of acpi container. > > In the patch description, it mentions: > > > > For ACPI containers that callback simply walks the list of ACPI > > device objects right below the container object (its children) and > > checks if all of their physical companion devices are offline. If > > that's not the case, it returns -EBUSY and the container system > > devivce cannot be put offline. Consequently, to put the container > > system device offline, it is necessary to put all of the physical > > devices depending on its ACPI companion object offline beforehand. > > > > Looks that it means acpi_container_offline() should walks all physical > > companion devices of container's children and checks their offline > > state. And, the comment in source code is "Check all of the dependent > > devices' physical companions", which means it should checks _all_ > > physical companions. > > > > But, the checking code just stops at the first not-offlined physical > > companion device of the first not-offlined child, then kernel only > > emits KOBJ_CHANGE uevent to the one device. It doesn't really walk > > all children's all physical companion devices and doesn't send change > > uevent to them. > > It is unclear to me from the description whether or not this is a > practical issue. > > Also there is an alternative, which is not to send KOBJ_CHANGE uevents > to any children at all. > > Why is the approach you chose better? > Please ignore this patch! Thanks for your review. I'd say sorry for that I confused with the code in acpi_scan_hot_remove() when I was sending this patch. At that time I didn't aware that the acpi_container_offline() does not need to send uevent to not-offline-yet devices. Before two weeks ago (around Apr. 5), I sent a mail to linux-acpi for reminding to ignore this patch. I don't know why the mail got filtered... Thanks a lot! Joey Lee -- To unsubscribe from this list: send the line "unsubscribe linux-acpi" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
diff --git a/drivers/acpi/container.c b/drivers/acpi/container.c index 12c2409..1f1537d 100644 --- a/drivers/acpi/container.c +++ b/drivers/acpi/container.c @@ -43,13 +43,14 @@ static int acpi_container_offline(struct container_dev *cdev) { struct acpi_device *adev = ACPI_COMPANION(&cdev->dev); struct acpi_device *child; + int ret = 0; /* Check all of the dependent devices' physical companions. */ list_for_each_entry(child, &adev->children, node) if (!acpi_scan_is_offline(child, false)) - return -EBUSY; + ret = -EBUSY; - return 0; + return ret; } static void acpi_container_release(struct device *dev) diff --git a/drivers/acpi/scan.c b/drivers/acpi/scan.c index 1926918..1a9055c 100644 --- a/drivers/acpi/scan.c +++ b/drivers/acpi/scan.c @@ -134,7 +134,6 @@ bool acpi_scan_is_offline(struct acpi_device *adev, bool uevent) kobject_uevent(&pn->dev->kobj, KOBJ_CHANGE); offline = false; - break; } mutex_unlock(&adev->physical_node_lock);
The caa73ea1 patch, "ACPI / hotplug / driver core: Handle containers in a special way", introduced the offline callback of acpi container. In the patch description, it mentions: For ACPI containers that callback simply walks the list of ACPI device objects right below the container object (its children) and checks if all of their physical companion devices are offline. If that's not the case, it returns -EBUSY and the container system devivce cannot be put offline. Consequently, to put the container system device offline, it is necessary to put all of the physical devices depending on its ACPI companion object offline beforehand. Looks that it means acpi_container_offline() should walks all physical companion devices of container's children and checks their offline state. And, the comment in source code is "Check all of the dependent devices' physical companions", which means it should checks _all_ physical companions. But, the checking code just stops at the first not-offlined physical companion device of the first not-offlined child, then kernel only emits KOBJ_CHANGE uevent to the one device. It doesn't really walk all children's all physical companion devices and doesn't send change uevent to them. This causes that usespace can only receive one uevent for one physical companion device in acpi container when acpi container offline is triggered. Cc: "Rafael J. Wysocki" <rjw@rjwysocki.net> Cc: Len Brown <lenb@kernel.org> Cc: Michal Hocko <mhocko@suse.com> Cc: Jiri Kosina <jkosina@suse.cz> Signed-off-by: "Lee, Chun-Yi" <jlee@suse.com> --- drivers/acpi/container.c | 5 +++-- drivers/acpi/scan.c | 1 - 2 files changed, 3 insertions(+), 3 deletions(-)