[PATCH v1 03/16] ACPI: glue: Introduce acpi_find_child_by_adr()

[PATCH v1 03/16] ACPI: glue: Introduce acpi_find_child_by_adr()

[Rafael J. Wysocki]

From: Rafael J. Wysocki <rafael.j.wysocki@intel.com>

Rearrange the ACPI device lookup code used internally by
acpi_find_child_device() so it can avoid extra checks after finding
one object with a matching _ADR and use it for defining
acpi_find_child_by_adr() that will allow the callers to find a given
ACPI device's child matching a given bus address without doing any
other checks in check_one_child().

Signed-off-by: Rafael J. Wysocki <rafael.j.wysocki@intel.com>
---
 drivers/acpi/glue.c     |   28 ++++++++++++++++++++++++----
 include/acpi/acpi_bus.h |    2 ++
 2 files changed, 26 insertions(+), 4 deletions(-)

Index: linux-pm/include/acpi/acpi_bus.h
===================================================================
--- linux-pm.orig/include/acpi/acpi_bus.h
+++ linux-pm/include/acpi/acpi_bus.h
@@ -622,6 +622,8 @@ static inline int acpi_dma_configure(str
 }
 struct acpi_device *acpi_find_child_device(struct acpi_device *parent,
 					   u64 address, bool check_children);
+struct acpi_device *acpi_find_child_by_adr(struct acpi_device *adev,
+					   acpi_bus_address adr);
 int acpi_is_root_bridge(acpi_handle);
 struct acpi_pci_root *acpi_pci_find_root(acpi_handle handle);
 
Index: linux-pm/drivers/acpi/glue.c
===================================================================
--- linux-pm.orig/drivers/acpi/glue.c
+++ linux-pm/drivers/acpi/glue.c
@@ -119,6 +119,7 @@ struct find_child_walk_data {
 	struct acpi_device *adev;
 	u64 address;
 	int score;
+	bool check_sta;
 	bool check_children;
 };
 
@@ -131,9 +132,13 @@ static int check_one_child(struct acpi_d
 		return 0;
 
 	if (!wd->adev) {
-		/* This is the first matching object.  Save it and continue. */
+		/*
+		 * This is the first matching object, so save it.  If it is not
+		 * necessary to look for any other matching objects, stop the
+		 * search.
+		 */
 		wd->adev = adev;
-		return 0;
+		return !(wd->check_sta || wd->check_children);
 	}
 
 	/*
@@ -169,12 +174,14 @@ static int check_one_child(struct acpi_d
 	return 0;
 }
 
-struct acpi_device *acpi_find_child_device(struct acpi_device *parent,
-					   u64 address, bool check_children)
+static struct acpi_device *acpi_find_child(struct acpi_device *parent,
+					   u64 address, bool check_children,
+					   bool check_sta)
 {
 	struct find_child_walk_data wd = {
 		.address = address,
 		.check_children = check_children,
+		.check_sta = check_sta,
 		.adev = NULL,
 		.score = 0,
 	};
@@ -184,8 +191,21 @@ struct acpi_device *acpi_find_child_devi
 
 	return wd.adev;
 }
+
+struct acpi_device *acpi_find_child_device(struct acpi_device *parent,
+					   u64 address, bool check_children)
+{
+	return acpi_find_child(parent, address, check_children, true);
+}
 EXPORT_SYMBOL_GPL(acpi_find_child_device);
 
+struct acpi_device *acpi_find_child_by_adr(struct acpi_device *adev,
+					   acpi_bus_address adr)
+{
+	return acpi_find_child(adev, adr, false, false);
+}
+EXPORT_SYMBOL_GPL(acpi_find_child_by_adr);
+
 static void acpi_physnode_link_name(char *buf, unsigned int node_id)
 {
 	if (node_id > 0)

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

[Rafael J. Wysocki]

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)

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

[Andy Shevchenko]

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);

-- 
With Best Regards,
Andy Shevchenko

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

[Rafael J. Wysocki]

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);
>
> --

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

[Heikki Krogerus]

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,

-- 
heikki

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

[Rafael J. Wysocki]

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.

[PATCH v1 05/16] USB: ACPI: Use acpi_find_child_by_adr()

[Rafael J. Wysocki]

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/usb/core/usb-acpi.c |    9 +--------
 1 file changed, 1 insertion(+), 8 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
@@ -127,17 +127,10 @@ out:
 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);
+	return acpi_find_child_by_adr(parent, raw);
 }
 
 static struct acpi_device *

Re: [PATCH v1 05/16] USB: ACPI: Use acpi_find_child_by_adr()

[Andy Shevchenko]

On Thu, Jun 09, 2022 at 03:56:42PM +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 (!parent)
>  		return NULL;

Can be removed because it's embedded in the call below, no?

> +	return acpi_find_child_by_adr(parent, raw);

-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH v1 05/16] USB: ACPI: Use acpi_find_child_by_adr()

[Rafael J. Wysocki]

On Thu, Jun 9, 2022 at 5:27 PM Andy Shevchenko
<andriy.shevchenko@linux.intel.com> wrote:
>
> On Thu, Jun 09, 2022 at 03:56:42PM +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 (!parent)
> >               return NULL;
>
> Can be removed because it's embedded in the call below, no?

Yes, it can, in analogy with the thunderbolt code.  Will update.

> > +     return acpi_find_child_by_adr(parent, raw);
>
> --

Re: [PATCH v1 05/16] USB: ACPI: Use acpi_find_child_by_adr()

[Heikki Krogerus]

On Thu, Jun 09, 2022 at 03:56:42PM +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/usb/core/usb-acpi.c |    9 +--------
>  1 file changed, 1 insertion(+), 8 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
> @@ -127,17 +127,10 @@ out:
>  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);
> +	return acpi_find_child_by_adr(parent, raw);
>  }
>  
>  static struct acpi_device *

I think usb_acpi_find_port() can also be dropped now.

thanks,

-- 
heikki

Re: [PATCH v1 05/16] USB: ACPI: Use acpi_find_child_by_adr()

[Rafael J. Wysocki]

On Fri, Jun 10, 2022 at 8:47 AM Heikki Krogerus
<heikki.krogerus@linux.intel.com> wrote:
>
> On Thu, Jun 09, 2022 at 03:56:42PM +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/usb/core/usb-acpi.c |    9 +--------
> >  1 file changed, 1 insertion(+), 8 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
> > @@ -127,17 +127,10 @@ out:
> >  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);
> > +     return acpi_find_child_by_adr(parent, raw);
> >  }
> >
> >  static struct acpi_device *
>
> I think usb_acpi_find_port() can also be dropped now.

Yes, it can.

I'm dropping it in the new version of the patch to be posted.

[PATCH v2 03/16] ACPI: glue: Introduce acpi_find_child_by_adr()

[Rafael J. Wysocki]

From: Rafael J. Wysocki <rafael.j.wysocki@intel.com>

Rearrange the ACPI device lookup code used internally by
acpi_find_child_device() so it can avoid extra checks after finding
one object with a matching _ADR and use it for defining
acpi_find_child_by_adr() that will allow the callers to find a given
ACPI device's child matching a given bus address without doing any
other checks in check_one_child().

Signed-off-by: Rafael J. Wysocki <rafael.j.wysocki@intel.com>
Reviewed-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
---

v1 -> v2:
   * Add R-by from Andy.

---
 drivers/acpi/glue.c     |   28 ++++++++++++++++++++++++----
 include/acpi/acpi_bus.h |    2 ++
 2 files changed, 26 insertions(+), 4 deletions(-)

Index: linux-pm/include/acpi/acpi_bus.h
===================================================================
--- linux-pm.orig/include/acpi/acpi_bus.h
+++ linux-pm/include/acpi/acpi_bus.h
@@ -622,6 +622,8 @@ static inline int acpi_dma_configure(str
 }
 struct acpi_device *acpi_find_child_device(struct acpi_device *parent,
 					   u64 address, bool check_children);
+struct acpi_device *acpi_find_child_by_adr(struct acpi_device *adev,
+					   acpi_bus_address adr);
 int acpi_is_root_bridge(acpi_handle);
 struct acpi_pci_root *acpi_pci_find_root(acpi_handle handle);
 
Index: linux-pm/drivers/acpi/glue.c
===================================================================
--- linux-pm.orig/drivers/acpi/glue.c
+++ linux-pm/drivers/acpi/glue.c
@@ -119,6 +119,7 @@ struct find_child_walk_data {
 	struct acpi_device *adev;
 	u64 address;
 	int score;
+	bool check_sta;
 	bool check_children;
 };
 
@@ -131,9 +132,13 @@ static int check_one_child(struct acpi_d
 		return 0;
 
 	if (!wd->adev) {
-		/* This is the first matching object.  Save it and continue. */
+		/*
+		 * This is the first matching object, so save it.  If it is not
+		 * necessary to look for any other matching objects, stop the
+		 * search.
+		 */
 		wd->adev = adev;
-		return 0;
+		return !(wd->check_sta || wd->check_children);
 	}
 
 	/*
@@ -169,12 +174,14 @@ static int check_one_child(struct acpi_d
 	return 0;
 }
 
-struct acpi_device *acpi_find_child_device(struct acpi_device *parent,
-					   u64 address, bool check_children)
+static struct acpi_device *acpi_find_child(struct acpi_device *parent,
+					   u64 address, bool check_children,
+					   bool check_sta)
 {
 	struct find_child_walk_data wd = {
 		.address = address,
 		.check_children = check_children,
+		.check_sta = check_sta,
 		.adev = NULL,
 		.score = 0,
 	};
@@ -184,8 +191,21 @@ struct acpi_device *acpi_find_child_devi
 
 	return wd.adev;
 }
+
+struct acpi_device *acpi_find_child_device(struct acpi_device *parent,
+					   u64 address, bool check_children)
+{
+	return acpi_find_child(parent, address, check_children, true);
+}
 EXPORT_SYMBOL_GPL(acpi_find_child_device);
 
+struct acpi_device *acpi_find_child_by_adr(struct acpi_device *adev,
+					   acpi_bus_address adr)
+{
+	return acpi_find_child(adev, adr, false, false);
+}
+EXPORT_SYMBOL_GPL(acpi_find_child_by_adr);
+
 static void acpi_physnode_link_name(char *buf, unsigned int node_id)
 {
 	if (node_id > 0)

[PATCH v2 04/16] thunderbolt: ACPI: Replace tb_acpi_find_port() with acpi_find_child_by_adr()

[Rafael J. Wysocki]

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);
+		return acpi_find_child_by_adr(ACPI_COMPANION(dev->parent),
+					      tb_to_usb4_port_device(dev)->port->port);
 	return NULL;
 }
 

Re: [PATCH v2 04/16] thunderbolt: ACPI: Replace tb_acpi_find_port() with acpi_find_child_by_adr()

[Andy Shevchenko]

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;
>  }
>  
> 
> 
> 

-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH v2 04/16] thunderbolt: ACPI: Replace tb_acpi_find_port() with acpi_find_child_by_adr()

[Mika Westerberg]

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?

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;
>  }
>  
> 
> 

Re: [PATCH v2 04/16] thunderbolt: ACPI: Replace tb_acpi_find_port() with acpi_find_child_by_adr()

[Rafael J. Wysocki]

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!

Re: [PATCH v2 04/16] thunderbolt: ACPI: Replace tb_acpi_find_port() with acpi_find_child_by_adr()

[Mika Westerberg]

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 ;-))

Re: [PATCH v2 04/16] thunderbolt: ACPI: Replace tb_acpi_find_port() with acpi_find_child_by_adr()

[Rafael J. Wysocki]

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!

Re: [PATCH v2 04/16] thunderbolt: ACPI: Replace tb_acpi_find_port() with acpi_find_child_by_adr()

[Heikki Krogerus]

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;
>  }
>  
> 
> 

-- 
heikki

[PATCH v2 05/16] USB: ACPI: Replace usb_acpi_find_port() with acpi_find_child_by_adr()

[Rafael J. Wysocki]

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).

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 *

Re: [PATCH v2 05/16] USB: ACPI: Replace usb_acpi_find_port() with acpi_find_child_by_adr()

[Andy Shevchenko]

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 *
> 
> 
> 

-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH v2 05/16] USB: ACPI: Replace usb_acpi_find_port() with acpi_find_child_by_adr()

[Heikki Krogerus]

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).
> 
> Signed-off-by: Rafael J. Wysocki <rafael.j.wysocki@intel.com>

Reviewed-by: Heikki Krogerus <heikki.krogerus@linux.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 *
> 
> 

-- 
heikki