| Message ID | 1843211.tdWV9SEqCh@kreacher |
|---|---|
| Headers | show |
| Series | ACPI: Get rid of the list of children in struct acpi_device | expand |
On Fri, Jun 10, 2022 at 8:46 AM Heikki Krogerus <heikki.krogerus@linux.intel.com> wrote: > > On Thu, Jun 09, 2022 at 03:54:40PM +0200, Rafael J. Wysocki wrote: > > From: Rafael J. Wysocki <rafael.j.wysocki@intel.com> > > > > Instead of walking the list of children of an ACPI device directly > > in order to find the child matching a given bus address, use > > acpi_find_child_by_adr() for this purpose. > > > > Apart from simplifying the code, this will help to eliminate the > > children list head from struct acpi_device as it is redundant and it > > is used in questionable ways in some places (in particular, locking is > > needed for walking the list pointed to it safely, but it is often > > missing). > > > > Signed-off-by: Rafael J. Wysocki <rafael.j.wysocki@intel.com> > > --- > > drivers/thunderbolt/acpi.c | 9 +-------- > > 1 file changed, 1 insertion(+), 8 deletions(-) > > > > Index: linux-pm/drivers/thunderbolt/acpi.c > > =================================================================== > > --- linux-pm.orig/drivers/thunderbolt/acpi.c > > +++ linux-pm/drivers/thunderbolt/acpi.c > > @@ -304,8 +304,6 @@ static bool tb_acpi_bus_match(struct dev > > static struct acpi_device *tb_acpi_find_port(struct acpi_device *adev, > > const struct tb_port *port) > > { > > - struct acpi_device *port_adev; > > - > > if (!adev) > > return NULL; > > > > @@ -313,12 +311,7 @@ static struct acpi_device *tb_acpi_find_ > > * Device routers exists under the downstream facing USB4 port > > * of the parent router. Their _ADR is always 0. > > */ > > - list_for_each_entry(port_adev, &adev->children, node) { > > - if (acpi_device_adr(port_adev) == port->port) > > - return port_adev; > > - } > > - > > - return NULL; > > + return acpi_find_child_by_adr(adev, port->port); > > } > > > > static struct acpi_device *tb_acpi_switch_find_companion(struct tb_switch *sw) > > I don't think you need tb_acpi_find_port() anymore. You can just call > acpi_find_child_by_ard(ACPI_COMPANION(...), port->port) directly, no? Technically I can, but I thought that the comment in tb_acpi_find_port() was worth retaining.
On Thursday, June 9, 2022 3:44:27 PM CEST Rafael J. Wysocki wrote: > Hi All, > > Confusingly enough, the ACPI subsystem stores the information on the given ACPI > device's children in two places: as the list of children in struct acpi_device > and (as a result of device registration) in the list of children in the embedded > struct device. > > These two lists agree with each other most of the time, but not always (like in > error paths in some cases), and the list of children in struct acpi_device is > not generally safe to use without locking. In principle, it should always be > walked under acpi_device_lock, but in practice holding acpi_scan_lock is > sufficient for that too. However, its users may not know whether or not > they operate under acpi_scan_lock and at least in some cases it is not accessed > in a safe way (note that ACPI devices may go away as a result of hot-remove, > unlike OF nodes). > > For this reason, it is better to consolidate the code that needs to walk the > children of an ACPI device which is the purpose of this patch series. > > Overall, it switches over all of the users of the list of children in struct > acpi_device to using helpers based on the driver core's mechanics and finally > drops that list, but some extra cleanups are done on the way. > > Please refer to the patch changelogs for details. Here's a v2 of this series, mostly addressing review comments, but the subjects of the Thunderbolt and USB patches have been changed too. Thanks!
On Mon, Jun 13, 2022 at 08:39:37PM +0200, Rafael J. Wysocki wrote: > From: Rafael J. Wysocki <rafael.j.wysocki@intel.com> > > Instead of walking the list of children of an ACPI device directly > in order to find the child matching a given bus address, use > acpi_find_child_by_adr() for this purpose. > > Also notice that if acpi_find_child_by_adr() doesn't find a matching > child, acpi_find_child_device() will not find it too, so directly > replace usb_acpi_find_port() in usb_acpi_get_companion_for_port() with > acpi_find_child_by_adr() and drop it entirely. > > Apart from simplifying the code, this will help to eliminate the > children list head from struct acpi_device as it is redundant and it > is used in questionable ways in some places (in particular, locking is > needed for walking the list pointed to it safely, but it is often > missing). Reviewed-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com> > Signed-off-by: Rafael J. Wysocki <rafael.j.wysocki@intel.com> > --- > > v1 -> v2: > * Drop usb_acpi_find_port() (Heikki, Andy). > * Change the subject accordingly. > > --- > drivers/usb/core/usb-acpi.c | 18 +----------------- > 1 file changed, 1 insertion(+), 17 deletions(-) > > Index: linux-pm/drivers/usb/core/usb-acpi.c > =================================================================== > --- linux-pm.orig/drivers/usb/core/usb-acpi.c > +++ linux-pm/drivers/usb/core/usb-acpi.c > @@ -124,22 +124,6 @@ out: > */ > #define USB_ACPI_LOCATION_VALID (1 << 31) > > -static struct acpi_device *usb_acpi_find_port(struct acpi_device *parent, > - int raw) > -{ > - struct acpi_device *adev; > - > - if (!parent) > - return NULL; > - > - list_for_each_entry(adev, &parent->children, node) { > - if (acpi_device_adr(adev) == raw) > - return adev; > - } > - > - return acpi_find_child_device(parent, raw, false); > -} > - > static struct acpi_device * > usb_acpi_get_companion_for_port(struct usb_port *port_dev) > { > @@ -170,7 +154,7 @@ usb_acpi_get_companion_for_port(struct u > port1 = port_dev->portnum; > } > > - return usb_acpi_find_port(adev, port1); > + return acpi_find_child_by_adr(adev, port1); > } > > static struct acpi_device * > > >
On Mon, Jun 13, 2022 at 08:30:19PM +0200, Rafael J. Wysocki wrote: > From: Rafael J. Wysocki <rafael.j.wysocki@intel.com> > > Instead of walking the list of children of an ACPI device directly, > use acpi_dev_for_each_child() to carry out an action for all of > the given ACPI device's children. > > This will help to eliminate the children list head from struct > acpi_device as it is redundant and it is used in questionable ways > in some places (in particular, locking is needed for walking the > list pointed to it safely, but it is often missing). Reviewed-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com> > Signed-off-by: Rafael J. Wysocki <rafael.j.wysocki@intel.com> > --- > > v1 -> v2: > * Eliminate unnecessary branch (Andy). > > --- > drivers/platform/x86/thinkpad_acpi.c | 53 +++++++++++++++++------------------ > 1 file changed, 27 insertions(+), 26 deletions(-) > > Index: linux-pm/drivers/platform/x86/thinkpad_acpi.c > =================================================================== > --- linux-pm.orig/drivers/platform/x86/thinkpad_acpi.c > +++ linux-pm/drivers/platform/x86/thinkpad_acpi.c > @@ -6841,6 +6841,31 @@ static const struct backlight_ops ibm_ba > > /* --------------------------------------------------------------------- */ > > +static int __init tpacpi_evaluate_bcl(struct acpi_device *adev, void *not_used) > +{ > + struct acpi_buffer buffer = { ACPI_ALLOCATE_BUFFER, NULL }; > + union acpi_object *obj; > + acpi_status status; > + int rc; > + > + status = acpi_evaluate_object(adev->handle, "_BCL", NULL, &buffer); > + if (ACPI_FAILURE(status)) > + return 0; > + > + obj = buffer.pointer; > + if (!obj || obj->type != ACPI_TYPE_PACKAGE) { > + acpi_handle_info(adev->handle, > + "Unknown _BCL data, please report this to %s\n", > + TPACPI_MAIL); > + rc = 0; > + } else { > + rc = obj->package.count; > + } > + kfree(obj); > + > + return rc; > +} > + > /* > * Call _BCL method of video device. On some ThinkPads this will > * switch the firmware to the ACPI brightness control mode. > @@ -6848,37 +6873,13 @@ static const struct backlight_ops ibm_ba > > static int __init tpacpi_query_bcl_levels(acpi_handle handle) > { > - struct acpi_buffer buffer = { ACPI_ALLOCATE_BUFFER, NULL }; > - union acpi_object *obj; > - struct acpi_device *device, *child; > - int rc; > + struct acpi_device *device; > > device = acpi_fetch_acpi_dev(handle); > if (!device) > return 0; > > - rc = 0; > - list_for_each_entry(child, &device->children, node) { > - acpi_status status = acpi_evaluate_object(child->handle, "_BCL", > - NULL, &buffer); > - if (ACPI_FAILURE(status)) { > - buffer.length = ACPI_ALLOCATE_BUFFER; > - continue; > - } > - > - obj = (union acpi_object *)buffer.pointer; > - if (!obj || (obj->type != ACPI_TYPE_PACKAGE)) { > - pr_err("Unknown _BCL data, please report this to %s\n", > - TPACPI_MAIL); > - rc = 0; > - } else { > - rc = obj->package.count; > - } > - break; > - } > - > - kfree(buffer.pointer); > - return rc; > + return acpi_dev_for_each_child(device, tpacpi_evaluate_bcl, NULL); > } > > > > >
On Mon, Jun 13, 2022 at 08:11:36PM +0200, Rafael J. Wysocki wrote: > From: Rafael J. Wysocki <rafael.j.wysocki@intel.com> > > Use acpi_find_child_by_adr() to find the child matching a given bus > address instead of tb_acpi_find_port() that walks the list of children > of an ACPI device directly for this purpose and drop the latter. > > Apart from simplifying the code, this will help to eliminate the > children list head from struct acpi_device as it is redundant and it > is used in questionable ways in some places (in particular, locking is > needed for walking the list pointed to it safely, but it is often > missing). Reviewed-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com> > Signed-off-by: Rafael J. Wysocki <rafael.j.wysocki@intel.com> > --- > > v1 -> v2: > * Drop tb_acpi_find_port() (Heikki, Andy). > * Change the subject accordingly > > --- > drivers/thunderbolt/acpi.c | 27 ++++----------------------- > 1 file changed, 4 insertions(+), 23 deletions(-) > > Index: linux-pm/drivers/thunderbolt/acpi.c > =================================================================== > --- linux-pm.orig/drivers/thunderbolt/acpi.c > +++ linux-pm/drivers/thunderbolt/acpi.c > @@ -301,26 +301,6 @@ static bool tb_acpi_bus_match(struct dev > return tb_is_switch(dev) || tb_is_usb4_port_device(dev); > } > > -static struct acpi_device *tb_acpi_find_port(struct acpi_device *adev, > - const struct tb_port *port) > -{ > - struct acpi_device *port_adev; > - > - if (!adev) > - return NULL; > - > - /* > - * Device routers exists under the downstream facing USB4 port > - * of the parent router. Their _ADR is always 0. > - */ > - list_for_each_entry(port_adev, &adev->children, node) { > - if (acpi_device_adr(port_adev) == port->port) > - return port_adev; > - } > - > - return NULL; > -} > - > static struct acpi_device *tb_acpi_switch_find_companion(struct tb_switch *sw) > { > struct acpi_device *adev = NULL; > @@ -331,7 +311,8 @@ static struct acpi_device *tb_acpi_switc > struct tb_port *port = tb_port_at(tb_route(sw), parent_sw); > struct acpi_device *port_adev; > > - port_adev = tb_acpi_find_port(ACPI_COMPANION(&parent_sw->dev), port); > + port_adev = acpi_find_child_by_adr(ACPI_COMPANION(&parent_sw->dev), > + port->port); > if (port_adev) > adev = acpi_find_child_device(port_adev, 0, false); > } else { > @@ -364,8 +345,8 @@ static struct acpi_device *tb_acpi_find_ > if (tb_is_switch(dev)) > return tb_acpi_switch_find_companion(tb_to_switch(dev)); > else if (tb_is_usb4_port_device(dev)) > - return tb_acpi_find_port(ACPI_COMPANION(dev->parent), > - tb_to_usb4_port_device(dev)->port); > + return acpi_find_child_by_adr(ACPI_COMPANION(dev->parent), > + tb_to_usb4_port_device(dev)->port->port); > return NULL; > } > > > >
Hi, On 6/13/22 20:30, Rafael J. Wysocki wrote: > From: Rafael J. Wysocki <rafael.j.wysocki@intel.com> > > Instead of walking the list of children of an ACPI device directly, > use acpi_dev_for_each_child() to carry out an action for all of > the given ACPI device's children. > > This will help to eliminate the children list head from struct > acpi_device as it is redundant and it is used in questionable ways > in some places (in particular, locking is needed for walking the > list pointed to it safely, but it is often missing). > > Signed-off-by: Rafael J. Wysocki <rafael.j.wysocki@intel.com> Thanks, patch looks good to me: Reviewed-by: Hans de Goede <hdegoede@redhat.com> Rafael, feel free to take this upstream through the apci tree. Regards, Hans > --- > > v1 -> v2: > * Eliminate unnecessary branch (Andy). > > --- > drivers/platform/x86/thinkpad_acpi.c | 53 +++++++++++++++++------------------ > 1 file changed, 27 insertions(+), 26 deletions(-) > > Index: linux-pm/drivers/platform/x86/thinkpad_acpi.c > =================================================================== > --- linux-pm.orig/drivers/platform/x86/thinkpad_acpi.c > +++ linux-pm/drivers/platform/x86/thinkpad_acpi.c > @@ -6841,6 +6841,31 @@ static const struct backlight_ops ibm_ba > > /* --------------------------------------------------------------------- */ > > +static int __init tpacpi_evaluate_bcl(struct acpi_device *adev, void *not_used) > +{ > + struct acpi_buffer buffer = { ACPI_ALLOCATE_BUFFER, NULL }; > + union acpi_object *obj; > + acpi_status status; > + int rc; > + > + status = acpi_evaluate_object(adev->handle, "_BCL", NULL, &buffer); > + if (ACPI_FAILURE(status)) > + return 0; > + > + obj = buffer.pointer; > + if (!obj || obj->type != ACPI_TYPE_PACKAGE) { > + acpi_handle_info(adev->handle, > + "Unknown _BCL data, please report this to %s\n", > + TPACPI_MAIL); > + rc = 0; > + } else { > + rc = obj->package.count; > + } > + kfree(obj); > + > + return rc; > +} > + > /* > * Call _BCL method of video device. On some ThinkPads this will > * switch the firmware to the ACPI brightness control mode. > @@ -6848,37 +6873,13 @@ static const struct backlight_ops ibm_ba > > static int __init tpacpi_query_bcl_levels(acpi_handle handle) > { > - struct acpi_buffer buffer = { ACPI_ALLOCATE_BUFFER, NULL }; > - union acpi_object *obj; > - struct acpi_device *device, *child; > - int rc; > + struct acpi_device *device; > > device = acpi_fetch_acpi_dev(handle); > if (!device) > return 0; > > - rc = 0; > - list_for_each_entry(child, &device->children, node) { > - acpi_status status = acpi_evaluate_object(child->handle, "_BCL", > - NULL, &buffer); > - if (ACPI_FAILURE(status)) { > - buffer.length = ACPI_ALLOCATE_BUFFER; > - continue; > - } > - > - obj = (union acpi_object *)buffer.pointer; > - if (!obj || (obj->type != ACPI_TYPE_PACKAGE)) { > - pr_err("Unknown _BCL data, please report this to %s\n", > - TPACPI_MAIL); > - rc = 0; > - } else { > - rc = obj->package.count; > - } > - break; > - } > - > - kfree(buffer.pointer); > - return rc; > + return acpi_dev_for_each_child(device, tpacpi_evaluate_bcl, NULL); > } > > > > >
On Mon, Jun 13, 2022 at 08:11:36PM +0200, Rafael J. Wysocki wrote: > From: Rafael J. Wysocki <rafael.j.wysocki@intel.com> > > Use acpi_find_child_by_adr() to find the child matching a given bus > address instead of tb_acpi_find_port() that walks the list of children > of an ACPI device directly for this purpose and drop the latter. > > Apart from simplifying the code, this will help to eliminate the > children list head from struct acpi_device as it is redundant and it > is used in questionable ways in some places (in particular, locking is > needed for walking the list pointed to it safely, but it is often > missing). > > Signed-off-by: Rafael J. Wysocki <rafael.j.wysocki@intel.com> Reviewed-by: Heikki Krogerus <heikki.krogerus@linux.intel.com> > --- > > v1 -> v2: > * Drop tb_acpi_find_port() (Heikki, Andy). > * Change the subject accordingly > > --- > drivers/thunderbolt/acpi.c | 27 ++++----------------------- > 1 file changed, 4 insertions(+), 23 deletions(-) > > Index: linux-pm/drivers/thunderbolt/acpi.c > =================================================================== > --- linux-pm.orig/drivers/thunderbolt/acpi.c > +++ linux-pm/drivers/thunderbolt/acpi.c > @@ -301,26 +301,6 @@ static bool tb_acpi_bus_match(struct dev > return tb_is_switch(dev) || tb_is_usb4_port_device(dev); > } > > -static struct acpi_device *tb_acpi_find_port(struct acpi_device *adev, > - const struct tb_port *port) > -{ > - struct acpi_device *port_adev; > - > - if (!adev) > - return NULL; > - > - /* > - * Device routers exists under the downstream facing USB4 port > - * of the parent router. Their _ADR is always 0. > - */ > - list_for_each_entry(port_adev, &adev->children, node) { > - if (acpi_device_adr(port_adev) == port->port) > - return port_adev; > - } > - > - return NULL; > -} > - > static struct acpi_device *tb_acpi_switch_find_companion(struct tb_switch *sw) > { > struct acpi_device *adev = NULL; > @@ -331,7 +311,8 @@ static struct acpi_device *tb_acpi_switc > struct tb_port *port = tb_port_at(tb_route(sw), parent_sw); > struct acpi_device *port_adev; > > - port_adev = tb_acpi_find_port(ACPI_COMPANION(&parent_sw->dev), port); > + port_adev = acpi_find_child_by_adr(ACPI_COMPANION(&parent_sw->dev), > + port->port); > if (port_adev) > adev = acpi_find_child_device(port_adev, 0, false); > } else { > @@ -364,8 +345,8 @@ static struct acpi_device *tb_acpi_find_ > if (tb_is_switch(dev)) > return tb_acpi_switch_find_companion(tb_to_switch(dev)); > else if (tb_is_usb4_port_device(dev)) > - return tb_acpi_find_port(ACPI_COMPANION(dev->parent), > - tb_to_usb4_port_device(dev)->port); > + return acpi_find_child_by_adr(ACPI_COMPANION(dev->parent), > + tb_to_usb4_port_device(dev)->port->port); > return NULL; > } > > >