diff mbox series

[v1,04/16] thunderbolt: ACPI: Use acpi_find_child_by_adr()

Message ID 7414189.EvYhyI6sBW@kreacher (mailing list archive)
State New, archived
Headers show
Series None | expand

Commit Message

Rafael J. Wysocki June 9, 2022, 1:54 p.m. UTC
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(-)

Comments

Andy Shevchenko June 9, 2022, 3:25 p.m. UTC | #1
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.

...

>  	if (!adev)
>  		return NULL;

Already checked in the below call, IIUC. Hence can be removed.

> +	return acpi_find_child_by_adr(adev, port->port);
Rafael J. Wysocki June 9, 2022, 3:36 p.m. UTC | #2
On Thu, Jun 9, 2022 at 5:26 PM Andy Shevchenko
<andriy.shevchenko@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.
>
> ...
>
> >       if (!adev)
> >               return NULL;
>
> Already checked in the below call, IIUC. Hence can be removed.

Yes, it can, will update.

>
> > +     return acpi_find_child_by_adr(adev, port->port);
>
> --
Heikki Krogerus June 10, 2022, 6:46 a.m. UTC | #3
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?

thanks,
Rafael J. Wysocki June 10, 2022, 1:12 p.m. UTC | #4
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.
diff mbox series

Patch

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)