| Message ID | 2653857.mvXUDI8C0e@kreacher |
|---|---|
| Headers | show |
| Series | ACPI: Get rid of the list of children in struct acpi_device | expand |
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); > } > > > > >
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; > } > > >
Hi Mika, On Tue, Jun 14, 2022 at 8:07 AM Mika Westerberg <mika.westerberg@linux.intel.com> wrote: > > Hi Rafael, > > 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> > > --- > > > > 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); > > Can you move the above comment here too? Do you mean to move the comment from tb_acpi_find_port() right here or before the if (tb_is_switch(dev)) line above? I think that tb_acpi_switch_find_companion() would be a better place for that comment. At least it would match the code passing 0 to acpi_find_child_device() in there. > Otherwise looks good to me, > > Acked-by: Mika Westerberg <mika.westerberg@linux.intel.com> > > > + return acpi_find_child_by_adr(ACPI_COMPANION(dev->parent), > > + tb_to_usb4_port_device(dev)->port->port); > > return NULL; > > } Thanks!
On Tue, Jun 14, 2022 at 08:25:53PM +0200, Rafael J. Wysocki wrote: > Hi Mika, > > On Tue, Jun 14, 2022 at 8:07 AM Mika Westerberg > <mika.westerberg@linux.intel.com> wrote: > > > > Hi Rafael, > > > > 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> > > > --- > > > > > > 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); > > > > Can you move the above comment here too? > > Do you mean to move the comment from tb_acpi_find_port() right here or > before the if (tb_is_switch(dev)) line above? > > I think that tb_acpi_switch_find_companion() would be a better place > for that comment. At least it would match the code passing 0 to > acpi_find_child_device() in there. Yes, I agree (as long as the comment stays somewhere close ;-))
On Wed, Jun 15, 2022 at 8:27 AM Mika Westerberg <mika.westerberg@linux.intel.com> wrote: > > On Tue, Jun 14, 2022 at 08:25:53PM +0200, Rafael J. Wysocki wrote: > > Hi Mika, > > > > On Tue, Jun 14, 2022 at 8:07 AM Mika Westerberg > > <mika.westerberg@linux.intel.com> wrote: > > > > > > Hi Rafael, > > > > > > 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> > > > > --- > > > > > > > > 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); > > > > > > Can you move the above comment here too? > > > > Do you mean to move the comment from tb_acpi_find_port() right here or > > before the if (tb_is_switch(dev)) line above? > > > > I think that tb_acpi_switch_find_companion() would be a better place > > for that comment. At least it would match the code passing 0 to > > acpi_find_child_device() in there. > > Yes, I agree (as long as the comment stays somewhere close ;-)) OK, I'll move it to tb_acpi_switch_find_companion() then. Thanks!