[PATCH v1 00/16] ACPI: Get rid of the list of children in struct acpi_device

[PATCH v1 00/16] ACPI: Get rid of the list of children in struct acpi_device

[Rafael J. Wysocki]

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.

Thanks!

[PATCH v1 01/16] ACPI: glue: Use acpi_dev_for_each_child()

[Rafael J. Wysocki]

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>
---
 drivers/acpi/glue.c |  103 +++++++++++++++++++++++++++++++---------------------
 1 file changed, 63 insertions(+), 40 deletions(-)

Index: linux-pm/drivers/acpi/glue.c
===================================================================
--- linux-pm.orig/drivers/acpi/glue.c
+++ linux-pm/drivers/acpi/glue.c
@@ -105,51 +105,74 @@ static int find_child_checks(struct acpi
 	return FIND_CHILD_MAX_SCORE;
 }
 
+struct find_child_walk_data {
+	struct acpi_device *adev;
+	u64 address;
+	int score;
+	bool check_children;
+};
+
+static int check_one_child(struct acpi_device *adev, void *data)
+{
+	struct find_child_walk_data *wd = data;
+	int score;
+
+	if (!adev->pnp.type.bus_address || acpi_device_adr(adev) != wd->address)
+		return 0;
+
+	if (!wd->adev) {
+		/* This is the first matching object.  Save it and continue. */
+		wd->adev = adev;
+		return 0;
+	}
+
+	/*
+	 * There is more than one matching device object with the same _ADR
+	 * value.  That really is unexpected, so we are kind of beyond the scope
+	 * of the spec here.  We have to choose which one to return, though.
+	 *
+	 * First, get the score for the previously found object and terminate
+	 * the walk if it is maximum.
+	*/
+	if (!wd->score) {
+		score = find_child_checks(wd->adev, wd->check_children);
+		if (score == FIND_CHILD_MAX_SCORE)
+			return 1;
+
+		wd->score = score;
+	}
+	/*
+	 * Second, if the object that has just been found has a better score,
+	 * replace the previously found one with it and terminate the walk if
+	 * the new score is maximum.
+	 */
+	score = find_child_checks(adev, wd->check_children);
+	if (score > wd->score) {
+		wd->adev = adev;
+		if (score == FIND_CHILD_MAX_SCORE)
+			return 1;
+
+		wd->score = score;
+	}
+
+	/* Continue, because there may be better matches. */
+	return 0;
+}
+
 struct acpi_device *acpi_find_child_device(struct acpi_device *parent,
 					   u64 address, bool check_children)
 {
-	struct acpi_device *adev, *ret = NULL;
-	int ret_score = 0;
+	struct find_child_walk_data wd = {
+		.address = address,
+		.check_children = check_children,
+		.adev = NULL,
+		.score = 0,
+	};
 
-	if (!parent)
-		return NULL;
+	if (parent)
+		acpi_dev_for_each_child(parent, check_one_child, &wd);
 
-	list_for_each_entry(adev, &parent->children, node) {
-		acpi_bus_address addr = acpi_device_adr(adev);
-		int score;
-
-		if (!adev->pnp.type.bus_address || addr != address)
-			continue;
-
-		if (!ret) {
-			/* This is the first matching object.  Save it. */
-			ret = adev;
-			continue;
-		}
-		/*
-		 * There is more than one matching device object with the same
-		 * _ADR value.  That really is unexpected, so we are kind of
-		 * beyond the scope of the spec here.  We have to choose which
-		 * one to return, though.
-		 *
-		 * First, check if the previously found object is good enough
-		 * and return it if so.  Second, do the same for the object that
-		 * we've just found.
-		 */
-		if (!ret_score) {
-			ret_score = find_child_checks(ret, check_children);
-			if (ret_score == FIND_CHILD_MAX_SCORE)
-				return ret;
-		}
-		score = find_child_checks(adev, check_children);
-		if (score == FIND_CHILD_MAX_SCORE) {
-			return adev;
-		} else if (score > ret_score) {
-			ret = adev;
-			ret_score = score;
-		}
-	}
-	return ret;
+	return wd.adev;
 }
 EXPORT_SYMBOL_GPL(acpi_find_child_device);
 

[PATCH v1 02/16] ACPI: glue: Introduce acpi_dev_has_children()

[Rafael J. Wysocki]

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

Define acpi_dev_has_children() as a wrapper around
acpi_dev_for_each_child() and use it to check if the given ACPI
device has any children instead of checking the children list
head in struct acpi_device.

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/acpi/glue.c |   12 +++++++++++-
 1 file changed, 11 insertions(+), 1 deletion(-)

Index: linux-pm/drivers/acpi/glue.c
===================================================================
--- linux-pm.orig/drivers/acpi/glue.c
+++ linux-pm/drivers/acpi/glue.c
@@ -77,12 +77,22 @@ static struct acpi_bus_type *acpi_get_bu
 #define FIND_CHILD_MIN_SCORE	1
 #define FIND_CHILD_MAX_SCORE	2
 
+static int match_any(struct acpi_device *adev, void *not_used)
+{
+	return 1;
+}
+
+static bool acpi_dev_has_children(struct acpi_device *adev)
+{
+	return acpi_dev_for_each_child(adev, match_any, NULL) > 0;
+}
+
 static int find_child_checks(struct acpi_device *adev, bool check_children)
 {
 	unsigned long long sta;
 	acpi_status status;
 
-	if (check_children && list_empty(&adev->children))
+	if (check_children && !acpi_dev_has_children(adev))
 		return -ENODEV;
 
 	status = acpi_evaluate_integer(adev->handle, "_STA", NULL, &sta);

[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 v1 06/16] ACPI: container: Use acpi_dev_for_each_child()

[Rafael J. Wysocki]

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>
---
 drivers/acpi/container.c |   18 ++++++++++--------
 1 file changed, 10 insertions(+), 8 deletions(-)

Index: linux-pm/drivers/acpi/container.c
===================================================================
--- linux-pm.orig/drivers/acpi/container.c
+++ linux-pm/drivers/acpi/container.c
@@ -23,17 +23,19 @@ static const struct acpi_device_id conta
 
 #ifdef CONFIG_ACPI_CONTAINER
 
-static int acpi_container_offline(struct container_dev *cdev)
+static int check_offline(struct acpi_device *adev, void *not_used)
 {
-	struct acpi_device *adev = ACPI_COMPANION(&cdev->dev);
-	struct acpi_device *child;
+	if (acpi_scan_is_offline(adev, false))
+		return 0;
 
-	/* Check all of the dependent devices' physical companions. */
-	list_for_each_entry(child, &adev->children, node)
-		if (!acpi_scan_is_offline(child, false))
-			return -EBUSY;
+	return -EBUSY;
+}
 
-	return 0;
+static int acpi_container_offline(struct container_dev *cdev)
+{
+	/* Check all of the dependent devices' physical companions. */
+	return acpi_dev_for_each_child(ACPI_COMPANION(&cdev->dev),
+				       check_offline, NULL);
 }
 
 static void acpi_container_release(struct device *dev)

Re: [PATCH v1 06/16] ACPI: container: Use acpi_dev_for_each_child()

[Andy Shevchenko]

On Thu, Jun 09, 2022 at 03:58:24PM +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.

...

> +	return acpi_dev_for_each_child(ACPI_COMPANION(&cdev->dev),
> +				       check_offline, NULL);

I would find this on one line better and not missing important details after
80th character.

-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH v1 06/16] ACPI: container: Use acpi_dev_for_each_child()

[Rafael J. Wysocki]

On Thu, Jun 9, 2022 at 5:29 PM Andy Shevchenko
<andriy.shevchenko@linux.intel.com> wrote:
>
> On Thu, Jun 09, 2022 at 03:58:24PM +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.
>
> ...
>
> > +     return acpi_dev_for_each_child(ACPI_COMPANION(&cdev->dev),
> > +                                    check_offline, NULL);
>
> I would find this on one line better and not missing important details after
> 80th character.

I see that you've made similar comments on a few other patches.

I'll change all of them to be one line (longer that 80 chars), but
OTOH there are people who still don't like that.

[PATCH v1 07/16] ACPI: property: Use acpi_dev_for_each_child() for child lookup

[Rafael J. Wysocki]

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

Instead of using the list of children of an ACPI device directly,
use acpi_dev_for_each_child() to find the next child of a given
ACPI device.

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/acpi/property.c |   47 +++++++++++++++++++++++++----------------------
 1 file changed, 25 insertions(+), 22 deletions(-)

Index: linux-pm/drivers/acpi/property.c
===================================================================
--- linux-pm.orig/drivers/acpi/property.c
+++ linux-pm/drivers/acpi/property.c
@@ -1012,6 +1012,22 @@ static int acpi_node_prop_read(const str
 				   propname, proptype, val, nval);
 }
 
+static int stop_on_next(struct acpi_device *adev, void *data)
+{
+	struct acpi_device **ret_p = data;
+
+	if (!*ret_p) {
+		*ret_p = adev;
+		return 1;
+	}
+
+	/* Skip until the "previous" object is found. */
+	if (*ret_p == adev)
+		*ret_p = NULL;
+
+	return 0;
+}
+
 /**
  * acpi_get_next_subnode - Return the next child node handle for a fwnode
  * @fwnode: Firmware node to find the next child node for.
@@ -1020,35 +1036,22 @@ static int acpi_node_prop_read(const str
 struct fwnode_handle *acpi_get_next_subnode(const struct fwnode_handle *fwnode,
 					    struct fwnode_handle *child)
 {
-	const struct acpi_device *adev = to_acpi_device_node(fwnode);
-	const struct list_head *head;
-	struct list_head *next;
+	struct acpi_device *adev = to_acpi_device_node(fwnode);
 
 	if ((!child || is_acpi_device_node(child)) && adev) {
-		struct acpi_device *child_adev;
+		struct acpi_device *child_adev = to_acpi_device_node(child);
+
+		acpi_dev_for_each_child(adev, stop_on_next, &child_adev);
+		if (child_adev)
+			return acpi_fwnode_handle(child_adev);
 
-		head = &adev->children;
-		if (list_empty(head))
-			goto nondev;
-
-		if (child) {
-			adev = to_acpi_device_node(child);
-			next = adev->node.next;
-			if (next == head) {
-				child = NULL;
-				goto nondev;
-			}
-			child_adev = list_entry(next, struct acpi_device, node);
-		} else {
-			child_adev = list_first_entry(head, struct acpi_device,
-						      node);
-		}
-		return acpi_fwnode_handle(child_adev);
+		child = NULL;
 	}
 
- nondev:
 	if (!child || is_acpi_data_node(child)) {
 		const struct acpi_data_node *data = to_acpi_data_node(fwnode);
+		const struct list_head *head;
+		struct list_head *next;
 		struct acpi_data_node *dn;
 
 		/*

[PATCH v1 08/16] ACPI: bus: Export acpi_dev_for_each_child() to modules

[Rafael J. Wysocki]

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

Some pieces of modular code can benefit from using
acpi_dev_for_each_child(), so export it to modules.

Signed-off-by: Rafael J. Wysocki <rafael.j.wysocki@intel.com>
---
 drivers/acpi/bus.c |    1 +
 1 file changed, 1 insertion(+)

Index: linux-pm/drivers/acpi/bus.c
===================================================================
--- linux-pm.orig/drivers/acpi/bus.c
+++ linux-pm/drivers/acpi/bus.c
@@ -1102,6 +1102,7 @@ static int acpi_dev_for_one_check(struct
 
 	return adwc->fn(to_acpi_device(dev), adwc->data);
 }
+EXPORT_SYMBOL_GPL(acpi_dev_for_each_child);
 
 int acpi_dev_for_each_child(struct acpi_device *adev,
 			    int (*fn)(struct acpi_device *, void *), void *data)

[PATCH v1 09/16] ACPI: video: Use acpi_dev_for_each_child()

[Rafael J. Wysocki]

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>
---
 drivers/acpi/acpi_video.c |   42 +++++++++++++++++-------------------------
 1 file changed, 17 insertions(+), 25 deletions(-)

Index: linux-pm/drivers/acpi/acpi_video.c
===================================================================
--- linux-pm.orig/drivers/acpi/acpi_video.c
+++ linux-pm/drivers/acpi/acpi_video.c
@@ -1149,24 +1149,25 @@ acpi_video_get_device_type(struct acpi_v
 	return 0;
 }
 
-static int
-acpi_video_bus_get_one_device(struct acpi_device *device,
-			      struct acpi_video_bus *video)
+static int acpi_video_bus_get_one_device(struct acpi_device *device, void *arg)
 {
-	unsigned long long device_id;
-	int status, device_type;
-	struct acpi_video_device *data;
+	struct acpi_video_bus *video = arg;
 	struct acpi_video_device_attrib *attribute;
+	struct acpi_video_device *data;
+	unsigned long long device_id;
+	acpi_status status;
+	int device_type;
 
-	status =
-	    acpi_evaluate_integer(device->handle, "_ADR", NULL, &device_id);
-	/* Some device omits _ADR, we skip them instead of fail */
+	status = acpi_evaluate_integer(device->handle, "_ADR", NULL, &device_id);
+	/* Skip devices without _ADR instead of failing. */
 	if (ACPI_FAILURE(status))
-		return 0;
+		goto exit;
 
 	data = kzalloc(sizeof(struct acpi_video_device), GFP_KERNEL);
-	if (!data)
+	if (!data) {
+		dev_dbg(&device->dev, "Cannot attach\n");
 		return -ENOMEM;
+	}
 
 	strcpy(acpi_device_name(device), ACPI_VIDEO_DEVICE_NAME);
 	strcpy(acpi_device_class(device), ACPI_VIDEO_CLASS);
@@ -1226,7 +1227,9 @@ acpi_video_bus_get_one_device(struct acp
 	list_add_tail(&data->entry, &video->video_device_list);
 	mutex_unlock(&video->device_list_lock);
 
-	return status;
+exit:
+	video->child_count++;
+	return 0;
 }
 
 /*
@@ -1538,9 +1541,6 @@ static int
 acpi_video_bus_get_devices(struct acpi_video_bus *video,
 			   struct acpi_device *device)
 {
-	int status = 0;
-	struct acpi_device *dev;
-
 	/*
 	 * There are systems where video module known to work fine regardless
 	 * of broken _DOD and ignoring returned value here doesn't cause
@@ -1548,16 +1548,8 @@ acpi_video_bus_get_devices(struct acpi_v
 	 */
 	acpi_video_device_enumerate(video);
 
-	list_for_each_entry(dev, &device->children, node) {
-
-		status = acpi_video_bus_get_one_device(dev, video);
-		if (status) {
-			dev_err(&dev->dev, "Can't attach device\n");
-			break;
-		}
-		video->child_count++;
-	}
-	return status;
+	return acpi_dev_for_each_child(device, acpi_video_bus_get_one_device,
+				       video);
 }
 
 /* acpi_video interface */

Re: [PATCH v1 09/16] ACPI: video: Use acpi_dev_for_each_child()

[Andy Shevchenko]

On Thu, Jun 09, 2022 at 04:03: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,
> use acpi_dev_for_each_child() to carry out an action for all of
> the given ACPI device's children.

...

> +	return acpi_dev_for_each_child(device, acpi_video_bus_get_one_device,
> +				       video);

Perhaps one line?

-- 
With Best Regards,
Andy Shevchenko

[PATCH v1 10/16] ACPI: bus: Introduce acpi_dev_for_each_child_reverse()

[Rafael J. Wysocki]

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

Make it possible to walk the children of an ACPI device in the revese
order by defining acpi_dev_for_each_child_reverse() in analogy with
acpi_dev_for_each_child().

Signed-off-by: Rafael J. Wysocki <rafael.j.wysocki@intel.com>
---
 drivers/acpi/bus.c      |   13 +++++++++++++
 include/acpi/acpi_bus.h |    3 +++
 2 files changed, 16 insertions(+)

Index: linux-pm/drivers/acpi/bus.c
===================================================================
--- linux-pm.orig/drivers/acpi/bus.c
+++ linux-pm/drivers/acpi/bus.c
@@ -1115,6 +1115,19 @@ int acpi_dev_for_each_child(struct acpi_
 	return device_for_each_child(&adev->dev, &adwc, acpi_dev_for_one_check);
 }
 
+int acpi_dev_for_each_child_reverse(struct acpi_device *adev,
+				    int (*fn)(struct acpi_device *, void *),
+				    void *data)
+{
+	struct acpi_dev_walk_context adwc = {
+		.fn = fn,
+		.data = data,
+	};
+
+	return device_for_each_child_reverse(&adev->dev, &adwc,
+					     acpi_dev_for_one_check);
+}
+
 /* --------------------------------------------------------------------------
                              Initialization/Cleanup
    -------------------------------------------------------------------------- */
Index: linux-pm/include/acpi/acpi_bus.h
===================================================================
--- linux-pm.orig/include/acpi/acpi_bus.h
+++ linux-pm/include/acpi/acpi_bus.h
@@ -483,6 +483,9 @@ extern struct bus_type acpi_bus_type;
 int acpi_bus_for_each_dev(int (*fn)(struct device *, void *), void *data);
 int acpi_dev_for_each_child(struct acpi_device *adev,
 			    int (*fn)(struct acpi_device *, void *), void *data);
+int acpi_dev_for_each_child_reverse(struct acpi_device *adev,
+				    int (*fn)(struct acpi_device *, void *),
+				    void *data);
 
 /*
  * Events

Re: [PATCH v1 10/16] ACPI: bus: Introduce acpi_dev_for_each_child_reverse()

[Andy Shevchenko]

On Thu, Jun 09, 2022 at 04:06:21PM +0200, Rafael J. Wysocki wrote:
> From: Rafael J. Wysocki <rafael.j.wysocki@intel.com>
> 
> Make it possible to walk the children of an ACPI device in the revese
> order by defining acpi_dev_for_each_child_reverse() in analogy with
> acpi_dev_for_each_child().

...

> +	return device_for_each_child_reverse(&adev->dev, &adwc,
> +					     acpi_dev_for_one_check);

Perhaps one line?

-- 
With Best Regards,
Andy Shevchenko

[PATCH v1 11/16] ACPI: scan: Walk ACPI device's children using driver core

[Rafael J. Wysocki]

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() or acpi_dev_for_each_child_reverse() 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>
---
 drivers/acpi/scan.c |   59 +++++++++++++++++++++++++---------------------------
 1 file changed, 29 insertions(+), 30 deletions(-)

Index: linux-pm/drivers/acpi/scan.c
===================================================================
--- linux-pm.orig/drivers/acpi/scan.c
+++ linux-pm/drivers/acpi/scan.c
@@ -334,10 +334,9 @@ static int acpi_scan_device_check(struct
 	return error;
 }
 
-static int acpi_scan_bus_check(struct acpi_device *adev)
+static int acpi_scan_bus_check(struct acpi_device *adev, void *not_used)
 {
 	struct acpi_scan_handler *handler = adev->handler;
-	struct acpi_device *child;
 	int error;
 
 	acpi_bus_get_status(adev);
@@ -353,19 +352,14 @@ static int acpi_scan_bus_check(struct ac
 		dev_warn(&adev->dev, "Namespace scan failure\n");
 		return error;
 	}
-	list_for_each_entry(child, &adev->children, node) {
-		error = acpi_scan_bus_check(child);
-		if (error)
-			return error;
-	}
-	return 0;
+	return acpi_dev_for_each_child(adev, acpi_scan_bus_check, NULL);
 }
 
 static int acpi_generic_hotplug_event(struct acpi_device *adev, u32 type)
 {
 	switch (type) {
 	case ACPI_NOTIFY_BUS_CHECK:
-		return acpi_scan_bus_check(adev);
+		return acpi_scan_bus_check(adev, NULL);
 	case ACPI_NOTIFY_DEVICE_CHECK:
 		return acpi_scan_device_check(adev);
 	case ACPI_NOTIFY_EJECT_REQUEST:
@@ -2187,9 +2181,8 @@ static int acpi_scan_attach_handler(stru
 	return ret;
 }
 
-static void acpi_bus_attach(struct acpi_device *device, bool first_pass)
+static int acpi_bus_attach(struct acpi_device *device, void *first_pass)
 {
-	struct acpi_device *child;
 	bool skip = !first_pass && device->flags.visited;
 	acpi_handle ejd;
 	int ret;
@@ -2206,7 +2199,7 @@ static void acpi_bus_attach(struct acpi_
 		device->flags.initialized = false;
 		acpi_device_clear_enumerated(device);
 		device->flags.power_manageable = 0;
-		return;
+		return 0;
 	}
 	if (device->handler)
 		goto ok;
@@ -2224,7 +2217,7 @@ static void acpi_bus_attach(struct acpi_
 
 	ret = acpi_scan_attach_handler(device);
 	if (ret < 0)
-		return;
+		return 0;
 
 	device->flags.match_driver = true;
 	if (ret > 0 && !device->flags.enumeration_by_parent) {
@@ -2234,19 +2227,20 @@ static void acpi_bus_attach(struct acpi_
 
 	ret = device_attach(&device->dev);
 	if (ret < 0)
-		return;
+		return 0;
 
 	if (device->pnp.type.platform_id || device->flags.enumeration_by_parent)
 		acpi_default_enumeration(device);
 	else
 		acpi_device_set_enumerated(device);
 
- ok:
-	list_for_each_entry(child, &device->children, node)
-		acpi_bus_attach(child, first_pass);
+ok:
+	acpi_dev_for_each_child(device, acpi_bus_attach, first_pass);
 
 	if (!skip && device->handler && device->handler->hotplug.notify_online)
 		device->handler->hotplug.notify_online(device);
+
+	return 0;
 }
 
 static int acpi_dev_get_first_consumer_dev_cb(struct acpi_dep_data *dep, void *data)
@@ -2274,7 +2268,7 @@ static void acpi_scan_clear_dep_fn(struc
 	cdw = container_of(work, struct acpi_scan_clear_dep_work, work);
 
 	acpi_scan_lock_acquire();
-	acpi_bus_attach(cdw->adev, true);
+	acpi_bus_attach(cdw->adev, (void *)true);
 	acpi_scan_lock_release();
 
 	acpi_dev_put(cdw->adev);
@@ -2432,7 +2426,7 @@ int acpi_bus_scan(acpi_handle handle)
 	if (!device)
 		return -ENODEV;
 
-	acpi_bus_attach(device, true);
+	acpi_bus_attach(device, (void *)true);
 
 	if (!acpi_bus_scan_second_pass)
 		return 0;
@@ -2446,25 +2440,17 @@ int acpi_bus_scan(acpi_handle handle)
 				    acpi_bus_check_add_2, NULL, NULL,
 				    (void **)&device);
 
-	acpi_bus_attach(device, false);
+	acpi_bus_attach(device, NULL);
 
 	return 0;
 }
 EXPORT_SYMBOL(acpi_bus_scan);
 
-/**
- * acpi_bus_trim - Detach scan handlers and drivers from ACPI device objects.
- * @adev: Root of the ACPI namespace scope to walk.
- *
- * Must be called under acpi_scan_lock.
- */
-void acpi_bus_trim(struct acpi_device *adev)
+int acpi_bus_trim_one(struct acpi_device *adev, void *not_used)
 {
 	struct acpi_scan_handler *handler = adev->handler;
-	struct acpi_device *child;
 
-	list_for_each_entry_reverse(child, &adev->children, node)
-		acpi_bus_trim(child);
+	acpi_dev_for_each_child_reverse(adev, acpi_bus_trim_one, NULL);
 
 	adev->flags.match_driver = false;
 	if (handler) {
@@ -2482,6 +2468,19 @@ void acpi_bus_trim(struct acpi_device *a
 	acpi_device_set_power(adev, ACPI_STATE_D3_COLD);
 	adev->flags.initialized = false;
 	acpi_device_clear_enumerated(adev);
+
+	return 0;
+}
+
+/**
+ * acpi_bus_trim - Detach scan handlers and drivers from ACPI device objects.
+ * @adev: Root of the ACPI namespace scope to walk.
+ *
+ * Must be called under acpi_scan_lock.
+ */
+void acpi_bus_trim(struct acpi_device *adev)
+{
+	acpi_bus_trim_one(adev, NULL);
 }
 EXPORT_SYMBOL_GPL(acpi_bus_trim);
 

[PATCH v1 12/16] platform/x86/thinkpad_acpi: Use acpi_dev_for_each_child()

[Rafael J. Wysocki]

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>
---
 drivers/platform/x86/thinkpad_acpi.c |   54 +++++++++++++++++++----------------
 1 file changed, 30 insertions(+), 24 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,18 @@ 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;
+	struct acpi_device *device;
 	int rc;
 
 	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;
-	}
+	rc = acpi_dev_for_each_child(device, tpacpi_evaluate_bcl, NULL);
+	if (rc > 0)
+		return rc;
 
-	kfree(buffer.pointer);
-	return rc;
+	return 0;
 }
 
 

Re: [PATCH v1 12/16] platform/x86/thinkpad_acpi: Use acpi_dev_for_each_child()

[Andy Shevchenko]

On Thu, Jun 09, 2022 at 04:09:45PM +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.

...

> +	rc = acpi_dev_for_each_child(device, tpacpi_evaluate_bcl, NULL);
> +	if (rc > 0)
> +		return rc;
>  
> +	return 0;

It can be simply 'return acpi_dev_for_each_child();', no?

AFAICS the caller is prepared for negative returns.

-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH v1 12/16] platform/x86/thinkpad_acpi: Use acpi_dev_for_each_child()

[Rafael J. Wysocki]

On Thu, Jun 9, 2022 at 5:49 PM Andy Shevchenko
<andriy.shevchenko@linux.intel.com> wrote:
>
> On Thu, Jun 09, 2022 at 04:09:45PM +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.
>
> ...
>
> > +     rc = acpi_dev_for_each_child(device, tpacpi_evaluate_bcl, NULL);
> > +     if (rc > 0)
> > +             return rc;
> >
> > +     return 0;
>
> It can be simply 'return acpi_dev_for_each_child();', no?

It can.

> AFAICS the caller is prepared for negative returns.

It will not return negative though unless the ACPI tables are horribly broken.

[PATCH v1 13/16] mfd: core: Use acpi_dev_for_each_child()

[Rafael J. Wysocki]

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>
---
 drivers/mfd/mfd-core.c |   31 ++++++++++++++++++++++++-------
 1 file changed, 24 insertions(+), 7 deletions(-)

Index: linux-pm/drivers/mfd/mfd-core.c
===================================================================
--- linux-pm.orig/drivers/mfd/mfd-core.c
+++ linux-pm/drivers/mfd/mfd-core.c
@@ -60,12 +60,29 @@ int mfd_cell_disable(struct platform_dev
 EXPORT_SYMBOL(mfd_cell_disable);
 
 #if IS_ENABLED(CONFIG_ACPI)
+struct match_ids_walk_data {
+	struct acpi_device_id *ids;
+	struct acpi_device *adev;
+};
+
+static int match_device_ids(struct acpi_device *adev, void *data)
+{
+	struct match_ids_walk_data *wd = data;
+
+	if (!acpi_match_device_ids(adev, wd->ids)) {
+		wd->adev = adev;
+		return 1;
+	}
+
+	return 0;
+}
+
 static void mfd_acpi_add_device(const struct mfd_cell *cell,
 				struct platform_device *pdev)
 {
 	const struct mfd_cell_acpi_match *match = cell->acpi_match;
-	struct acpi_device *parent, *child;
 	struct acpi_device *adev = NULL;
+	struct acpi_device *parent;
 
 	parent = ACPI_COMPANION(pdev->dev.parent);
 	if (!parent)
@@ -83,14 +100,14 @@ static void mfd_acpi_add_device(const st
 	if (match) {
 		if (match->pnpid) {
 			struct acpi_device_id ids[2] = {};
+			struct match_ids_walk_data wd = {
+				.adev = NULL,
+				.ids = ids,
+			};
 
 			strlcpy(ids[0].id, match->pnpid, sizeof(ids[0].id));
-			list_for_each_entry(child, &parent->children, node) {
-				if (!acpi_match_device_ids(child, ids)) {
-					adev = child;
-					break;
-				}
-			}
+			acpi_dev_for_each_child(parent, match_device_ids, &wd);
+			adev = wd.adev;
 		} else {
 			adev = acpi_find_child_device(parent, match->adr, false);
 		}

[PATCH v1 14/16] soundwire: Use acpi_dev_for_each_child()

[Rafael J. Wysocki]

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>
---
 drivers/soundwire/slave.c |  115 ++++++++++++++++++++++++++--------------------
 1 file changed, 67 insertions(+), 48 deletions(-)

Index: linux-pm/drivers/soundwire/slave.c
===================================================================
--- linux-pm.orig/drivers/soundwire/slave.c
+++ linux-pm/drivers/soundwire/slave.c
@@ -127,6 +127,71 @@ static bool find_slave(struct sdw_bus *b
 	return true;
 }
 
+struct sdw_acpi_child_walk_data {
+	struct sdw_bus *bus;
+	struct acpi_device *adev;
+	struct sdw_slave_id id;
+	bool ignore_unique_id;
+};
+
+static int sdw_acpi_check_duplicate(struct acpi_device *adev, void *data)
+{
+	struct sdw_acpi_child_walk_data *cwd = data;
+	struct sdw_bus *bus = cwd->bus;
+	struct sdw_slave_id id;
+
+	if (adev == cwd->adev)
+		return 0;
+
+	if (!find_slave(bus, adev, &id))
+		return 0;
+
+	if (cwd->id.sdw_version != id.sdw_version || cwd->id.mfg_id != id.mfg_id ||
+	    cwd->id.part_id != id.part_id || cwd->id.class_id != id.class_id)
+		return 0;
+
+	if (cwd->id.unique_id != id.unique_id) {
+		dev_dbg(bus->dev,
+			"Valid unique IDs 0x%x 0x%x for Slave mfg_id 0x%04x, part_id 0x%04x\n",
+			cwd->id.unique_id, id.unique_id, cwd->id.mfg_id,
+			cwd->id.part_id);
+		cwd->ignore_unique_id = false;
+		return 0;
+	}
+
+	dev_err(bus->dev,
+		"Invalid unique IDs 0x%x 0x%x for Slave mfg_id 0x%04x, part_id 0x%04x\n",
+		cwd->id.unique_id, id.unique_id, cwd->id.mfg_id, cwd->id.part_id);
+	return -ENODEV;
+}
+
+static int sdw_acpi_find_one(struct acpi_device *adev, void *data)
+{
+	struct sdw_bus *bus = data;
+	struct sdw_acpi_child_walk_data cwd = {
+		.bus = bus,
+		.adev = adev,
+		.ignore_unique_id = true,
+	};
+	int ret;
+
+	if (!find_slave(bus, adev, &cwd.id))
+		return 0;
+
+	/* Brute-force O(N^2) search for duplicates. */
+	ret = acpi_dev_for_each_child(ACPI_COMPANION(bus->dev),
+				      sdw_acpi_check_duplicate, &cwd);
+	if (ret)
+		return ret;
+
+	if (cwd.ignore_unique_id)
+		cwd.id.unique_id = SDW_IGNORED_UNIQUE_ID;
+
+	/* Ignore errors and continue. */
+	sdw_slave_add(bus, &cwd.id, acpi_fwnode_handle(adev));
+	return 0;
+}
+
 /*
  * sdw_acpi_find_slaves() - Find Slave devices in Master ACPI node
  * @bus: SDW bus instance
@@ -135,8 +200,7 @@ static bool find_slave(struct sdw_bus *b
  */
 int sdw_acpi_find_slaves(struct sdw_bus *bus)
 {
-	struct acpi_device *adev, *parent;
-	struct acpi_device *adev2, *parent2;
+	struct acpi_device *parent;
 
 	parent = ACPI_COMPANION(bus->dev);
 	if (!parent) {
@@ -144,52 +208,7 @@ int sdw_acpi_find_slaves(struct sdw_bus
 		return -ENODEV;
 	}
 
-	list_for_each_entry(adev, &parent->children, node) {
-		struct sdw_slave_id id;
-		struct sdw_slave_id id2;
-		bool ignore_unique_id = true;
-
-		if (!find_slave(bus, adev, &id))
-			continue;
-
-		/* brute-force O(N^2) search for duplicates */
-		parent2 = parent;
-		list_for_each_entry(adev2, &parent2->children, node) {
-
-			if (adev == adev2)
-				continue;
-
-			if (!find_slave(bus, adev2, &id2))
-				continue;
-
-			if (id.sdw_version != id2.sdw_version ||
-			    id.mfg_id != id2.mfg_id ||
-			    id.part_id != id2.part_id ||
-			    id.class_id != id2.class_id)
-				continue;
-
-			if (id.unique_id != id2.unique_id) {
-				dev_dbg(bus->dev,
-					"Valid unique IDs 0x%x 0x%x for Slave mfg_id 0x%04x, part_id 0x%04x\n",
-					id.unique_id, id2.unique_id, id.mfg_id, id.part_id);
-				ignore_unique_id = false;
-			} else {
-				dev_err(bus->dev,
-					"Invalid unique IDs 0x%x 0x%x for Slave mfg_id 0x%04x, part_id 0x%04x\n",
-					id.unique_id, id2.unique_id, id.mfg_id, id.part_id);
-				return -ENODEV;
-			}
-		}
-
-		if (ignore_unique_id)
-			id.unique_id = SDW_IGNORED_UNIQUE_ID;
-
-		/*
-		 * don't error check for sdw_slave_add as we want to continue
-		 * adding Slaves
-		 */
-		sdw_slave_add(bus, &id, acpi_fwnode_handle(adev));
-	}
+	acpi_dev_for_each_child(parent, sdw_acpi_find_one, bus);
 
 	return 0;
 }

Re: [PATCH v1 14/16] soundwire: Use acpi_dev_for_each_child()

[Pierre-Louis Bossart]

Thanks Rafael. This looks mostly good but I have a doubt on the error
handling, see below.

> +static int sdw_acpi_check_duplicate(struct acpi_device *adev, void *data)
> +{
> +	struct sdw_acpi_child_walk_data *cwd = data;
> +	struct sdw_bus *bus = cwd->bus;
> +	struct sdw_slave_id id;
> +
> +	if (adev == cwd->adev)
> +		return 0;
> +
> +	if (!find_slave(bus, adev, &id))
> +		return 0;
> +
> +	if (cwd->id.sdw_version != id.sdw_version || cwd->id.mfg_id != id.mfg_id ||
> +	    cwd->id.part_id != id.part_id || cwd->id.class_id != id.class_id)
> +		return 0;
> +
> +	if (cwd->id.unique_id != id.unique_id) {
> +		dev_dbg(bus->dev,
> +			"Valid unique IDs 0x%x 0x%x for Slave mfg_id 0x%04x, part_id 0x%04x\n",
> +			cwd->id.unique_id, id.unique_id, cwd->id.mfg_id,
> +			cwd->id.part_id);
> +		cwd->ignore_unique_id = false;
> +		return 0;
> +	}
> +
> +	dev_err(bus->dev,
> +		"Invalid unique IDs 0x%x 0x%x for Slave mfg_id 0x%04x, part_id 0x%04x\n",
> +		cwd->id.unique_id, id.unique_id, cwd->id.mfg_id, cwd->id.part_id);
> +	return -ENODEV;

if this error happens, I would guess it's reported ....

> +}
> +
> +static int sdw_acpi_find_one(struct acpi_device *adev, void *data)
> +{
> +	struct sdw_bus *bus = data;
> +	struct sdw_acpi_child_walk_data cwd = {
> +		.bus = bus,
> +		.adev = adev,
> +		.ignore_unique_id = true,
> +	};
> +	int ret;
> +
> +	if (!find_slave(bus, adev, &cwd.id))
> +		return 0;
> +
> +	/* Brute-force O(N^2) search for duplicates. */
> +	ret = acpi_dev_for_each_child(ACPI_COMPANION(bus->dev),
> +				      sdw_acpi_check_duplicate, &cwd);
> +	if (ret)
> +		return ret;

... here, but I don't see this being propagated further...

> +
> +	if (cwd.ignore_unique_id)
> +		cwd.id.unique_id = SDW_IGNORED_UNIQUE_ID;
> +
> +	/* Ignore errors and continue. */
> +	sdw_slave_add(bus, &cwd.id, acpi_fwnode_handle(adev));
> +	return 0;
> +}
> +
>  /*
>   * sdw_acpi_find_slaves() - Find Slave devices in Master ACPI node
>   * @bus: SDW bus instance
> @@ -135,8 +200,7 @@ static bool find_slave(struct sdw_bus *b
>   */
>  int sdw_acpi_find_slaves(struct sdw_bus *bus)
>  {
> -	struct acpi_device *adev, *parent;
> -	struct acpi_device *adev2, *parent2;
> +	struct acpi_device *parent;
>  
>  	parent = ACPI_COMPANION(bus->dev);
>  	if (!parent) {
> @@ -144,52 +208,7 @@ int sdw_acpi_find_slaves(struct sdw_bus
>  		return -ENODEV;
>  	}
>  
> -	list_for_each_entry(adev, &parent->children, node) {
> -		struct sdw_slave_id id;
> -		struct sdw_slave_id id2;
> -		bool ignore_unique_id = true;
> -
> -		if (!find_slave(bus, adev, &id))
> -			continue;
> -
> -		/* brute-force O(N^2) search for duplicates */
> -		parent2 = parent;
> -		list_for_each_entry(adev2, &parent2->children, node) {
> -
> -			if (adev == adev2)
> -				continue;
> -
> -			if (!find_slave(bus, adev2, &id2))
> -				continue;
> -
> -			if (id.sdw_version != id2.sdw_version ||
> -			    id.mfg_id != id2.mfg_id ||
> -			    id.part_id != id2.part_id ||
> -			    id.class_id != id2.class_id)
> -				continue;
> -
> -			if (id.unique_id != id2.unique_id) {
> -				dev_dbg(bus->dev,
> -					"Valid unique IDs 0x%x 0x%x for Slave mfg_id 0x%04x, part_id 0x%04x\n",
> -					id.unique_id, id2.unique_id, id.mfg_id, id.part_id);
> -				ignore_unique_id = false;
> -			} else {
> -				dev_err(bus->dev,
> -					"Invalid unique IDs 0x%x 0x%x for Slave mfg_id 0x%04x, part_id 0x%04x\n",
> -					id.unique_id, id2.unique_id, id.mfg_id, id.part_id);
> -				return -ENODEV;
> -			}
> -		}
> -
> -		if (ignore_unique_id)
> -			id.unique_id = SDW_IGNORED_UNIQUE_ID;
> -
> -		/*
> -		 * don't error check for sdw_slave_add as we want to continue
> -		 * adding Slaves
> -		 */
> -		sdw_slave_add(bus, &id, acpi_fwnode_handle(adev));
> -	}
> +	acpi_dev_for_each_child(parent, sdw_acpi_find_one, bus);

... here?

It looks like a change in the error handling flow where
sdw_acpi_find_slaves() is now returning 0 (success) always?

Shouldn't the return of sdw_acpi_find_one() be trapped, e.g. with

return acpi_dev_for_each_child(parent, sdw_acpi_find_one, bus);

>  
>  	return 0;
>  }
> 
> 
> 

Re: [PATCH v1 14/16] soundwire: Use acpi_dev_for_each_child()

[Rafael J. Wysocki]

On Thu, Jun 9, 2022 at 5:23 PM Pierre-Louis Bossart
<pierre-louis.bossart@linux.intel.com> wrote:
>
> Thanks Rafael. This looks mostly good but I have a doubt on the error
> handling, see below.
>
> > +static int sdw_acpi_check_duplicate(struct acpi_device *adev, void *data)
> > +{
> > +     struct sdw_acpi_child_walk_data *cwd = data;
> > +     struct sdw_bus *bus = cwd->bus;
> > +     struct sdw_slave_id id;
> > +
> > +     if (adev == cwd->adev)
> > +             return 0;
> > +
> > +     if (!find_slave(bus, adev, &id))
> > +             return 0;
> > +
> > +     if (cwd->id.sdw_version != id.sdw_version || cwd->id.mfg_id != id.mfg_id ||
> > +         cwd->id.part_id != id.part_id || cwd->id.class_id != id.class_id)
> > +             return 0;
> > +
> > +     if (cwd->id.unique_id != id.unique_id) {
> > +             dev_dbg(bus->dev,
> > +                     "Valid unique IDs 0x%x 0x%x for Slave mfg_id 0x%04x, part_id 0x%04x\n",
> > +                     cwd->id.unique_id, id.unique_id, cwd->id.mfg_id,
> > +                     cwd->id.part_id);
> > +             cwd->ignore_unique_id = false;
> > +             return 0;
> > +     }
> > +
> > +     dev_err(bus->dev,
> > +             "Invalid unique IDs 0x%x 0x%x for Slave mfg_id 0x%04x, part_id 0x%04x\n",
> > +             cwd->id.unique_id, id.unique_id, cwd->id.mfg_id, cwd->id.part_id);
> > +     return -ENODEV;
>
> if this error happens, I would guess it's reported ....
>
> > +}
> > +
> > +static int sdw_acpi_find_one(struct acpi_device *adev, void *data)
> > +{
> > +     struct sdw_bus *bus = data;
> > +     struct sdw_acpi_child_walk_data cwd = {
> > +             .bus = bus,
> > +             .adev = adev,
> > +             .ignore_unique_id = true,
> > +     };
> > +     int ret;
> > +
> > +     if (!find_slave(bus, adev, &cwd.id))
> > +             return 0;
> > +
> > +     /* Brute-force O(N^2) search for duplicates. */
> > +     ret = acpi_dev_for_each_child(ACPI_COMPANION(bus->dev),
> > +                                   sdw_acpi_check_duplicate, &cwd);
> > +     if (ret)
> > +             return ret;
>
> ... here, but I don't see this being propagated further...
>
> > +
> > +     if (cwd.ignore_unique_id)
> > +             cwd.id.unique_id = SDW_IGNORED_UNIQUE_ID;
> > +
> > +     /* Ignore errors and continue. */
> > +     sdw_slave_add(bus, &cwd.id, acpi_fwnode_handle(adev));
> > +     return 0;
> > +}
> > +
> >  /*
> >   * sdw_acpi_find_slaves() - Find Slave devices in Master ACPI node
> >   * @bus: SDW bus instance
> > @@ -135,8 +200,7 @@ static bool find_slave(struct sdw_bus *b
> >   */
> >  int sdw_acpi_find_slaves(struct sdw_bus *bus)
> >  {
> > -     struct acpi_device *adev, *parent;
> > -     struct acpi_device *adev2, *parent2;
> > +     struct acpi_device *parent;
> >
> >       parent = ACPI_COMPANION(bus->dev);
> >       if (!parent) {
> > @@ -144,52 +208,7 @@ int sdw_acpi_find_slaves(struct sdw_bus
> >               return -ENODEV;
> >       }
> >
> > -     list_for_each_entry(adev, &parent->children, node) {
> > -             struct sdw_slave_id id;
> > -             struct sdw_slave_id id2;
> > -             bool ignore_unique_id = true;
> > -
> > -             if (!find_slave(bus, adev, &id))
> > -                     continue;
> > -
> > -             /* brute-force O(N^2) search for duplicates */
> > -             parent2 = parent;
> > -             list_for_each_entry(adev2, &parent2->children, node) {
> > -
> > -                     if (adev == adev2)
> > -                             continue;
> > -
> > -                     if (!find_slave(bus, adev2, &id2))
> > -                             continue;
> > -
> > -                     if (id.sdw_version != id2.sdw_version ||
> > -                         id.mfg_id != id2.mfg_id ||
> > -                         id.part_id != id2.part_id ||
> > -                         id.class_id != id2.class_id)
> > -                             continue;
> > -
> > -                     if (id.unique_id != id2.unique_id) {
> > -                             dev_dbg(bus->dev,
> > -                                     "Valid unique IDs 0x%x 0x%x for Slave mfg_id 0x%04x, part_id 0x%04x\n",
> > -                                     id.unique_id, id2.unique_id, id.mfg_id, id.part_id);
> > -                             ignore_unique_id = false;
> > -                     } else {
> > -                             dev_err(bus->dev,
> > -                                     "Invalid unique IDs 0x%x 0x%x for Slave mfg_id 0x%04x, part_id 0x%04x\n",
> > -                                     id.unique_id, id2.unique_id, id.mfg_id, id.part_id);
> > -                             return -ENODEV;
> > -                     }
> > -             }
> > -
> > -             if (ignore_unique_id)
> > -                     id.unique_id = SDW_IGNORED_UNIQUE_ID;
> > -
> > -             /*
> > -              * don't error check for sdw_slave_add as we want to continue
> > -              * adding Slaves
> > -              */
> > -             sdw_slave_add(bus, &id, acpi_fwnode_handle(adev));
> > -     }
> > +     acpi_dev_for_each_child(parent, sdw_acpi_find_one, bus);
>
> ... here?
>
> It looks like a change in the error handling flow where
> sdw_acpi_find_slaves() is now returning 0 (success) always?
>
> Shouldn't the return of sdw_acpi_find_one() be trapped, e.g. with
>
> return acpi_dev_for_each_child(parent, sdw_acpi_find_one, bus);

Sure, I'll do that.  Thanks!

Re: [PATCH v1 14/16] soundwire: Use acpi_dev_for_each_child()

[Pierre-Louis Bossart]

>> Shouldn't the return of sdw_acpi_find_one() be trapped, e.g. with
>>
>> return acpi_dev_for_each_child(parent, sdw_acpi_find_one, bus);
> 
> Sure, I'll do that.  Thanks!

I also added this EXPORT_SYMBOL to work-around link errors, not sure if
this is in your tree already?

diff --git a/drivers/acpi/bus.c b/drivers/acpi/bus.c

index 86fa61a21826c..ade6259c19af6 100644

--- a/drivers/acpi/bus.c

+++ b/drivers/acpi/bus.c

@@ -1113,6 +1113,7 @@ int acpi_dev_for_each_child(struct acpi_device *adev,



        return device_for_each_child(&adev->dev, &adwc,
acpi_dev_for_one_check);

 }

+EXPORT_SYMBOL_GPL(acpi_dev_for_each_child);

Re: [PATCH v1 14/16] soundwire: Use acpi_dev_for_each_child()

[Rafael J. Wysocki]

On Thu, Jun 9, 2022 at 6:21 PM Pierre-Louis Bossart
<pierre-louis.bossart@linux.intel.com> wrote:
>
>
> >> Shouldn't the return of sdw_acpi_find_one() be trapped, e.g. with
> >>
> >> return acpi_dev_for_each_child(parent, sdw_acpi_find_one, bus);
> >
> > Sure, I'll do that.  Thanks!
>
> I also added this EXPORT_SYMBOL to work-around link errors, not sure if
> this is in your tree already?

One of the previous patches in the series is adding the export.

> diff --git a/drivers/acpi/bus.c b/drivers/acpi/bus.c
>
> index 86fa61a21826c..ade6259c19af6 100644
>
> --- a/drivers/acpi/bus.c
>
> +++ b/drivers/acpi/bus.c
>
> @@ -1113,6 +1113,7 @@ int acpi_dev_for_each_child(struct acpi_device *adev,
>
>
>
>         return device_for_each_child(&adev->dev, &adwc,
> acpi_dev_for_one_check);
>
>  }
>
> +EXPORT_SYMBOL_GPL(acpi_dev_for_each_child);
>
>

Re: [PATCH v1 14/16] soundwire: Use acpi_dev_for_each_child()

[Pierre-Louis Bossart]

On 6/9/22 12:35, Rafael J. Wysocki wrote:
> On Thu, Jun 9, 2022 at 6:21 PM Pierre-Louis Bossart
> <pierre-louis.bossart@linux.intel.com> wrote:
>>
>>
>>>> Shouldn't the return of sdw_acpi_find_one() be trapped, e.g. with
>>>>
>>>> return acpi_dev_for_each_child(parent, sdw_acpi_find_one, bus);
>>>
>>> Sure, I'll do that.  Thanks!
>>
>> I also added this EXPORT_SYMBOL to work-around link errors, not sure if
>> this is in your tree already?
> 
> One of the previous patches in the series is adding the export.

ok. I ran a bunch of tests with those two changes, so feel free to take
my tags:

Reviewed-by: Pierre-Louis Bossart <pierre-louis.bossart@linux.intel.com>
Tested-by: Pierre-Louis Bossart <pierre-louis.bossart@linux.intel.com>

[PATCH v1 15/16] ACPI / MMC: PM: Unify fixing up device power

[Rafael J. Wysocki]

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

Introduce acpi_device_fix_up_power_extended() for fixing up power of
a device having an ACPI companion in a manner that takes the device's
children into account and make the MMC code use it in two places
instead of walking the list of the device ACPI companion's children
directly.

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/acpi/device_pm.c          |   22 ++++++++++++++++++++++
 drivers/mmc/host/sdhci-acpi.c     |    7 ++-----
 drivers/mmc/host/sdhci-pci-core.c |   11 +++--------
 include/acpi/acpi_bus.h           |    1 +
 4 files changed, 28 insertions(+), 13 deletions(-)

Index: linux-pm/drivers/mmc/host/sdhci-acpi.c
===================================================================
--- linux-pm.orig/drivers/mmc/host/sdhci-acpi.c
+++ linux-pm/drivers/mmc/host/sdhci-acpi.c
@@ -775,8 +775,8 @@ static int sdhci_acpi_probe(struct platf
 {
 	struct device *dev = &pdev->dev;
 	const struct sdhci_acpi_slot *slot;
-	struct acpi_device *device, *child;
 	const struct dmi_system_id *id;
+	struct acpi_device *device;
 	struct sdhci_acpi_host *c;
 	struct sdhci_host *host;
 	struct resource *iomem;
@@ -796,10 +796,7 @@ static int sdhci_acpi_probe(struct platf
 	slot = sdhci_acpi_get_slot(device);
 
 	/* Power on the SDHCI controller and its children */
-	acpi_device_fix_up_power(device);
-	list_for_each_entry(child, &device->children, node)
-		if (child->status.present && child->status.enabled)
-			acpi_device_fix_up_power(child);
+	acpi_device_fix_up_power_extended(device);
 
 	if (sdhci_acpi_byt_defer(dev))
 		return -EPROBE_DEFER;
Index: linux-pm/drivers/acpi/device_pm.c
===================================================================
--- linux-pm.orig/drivers/acpi/device_pm.c
+++ linux-pm/drivers/acpi/device_pm.c
@@ -369,6 +369,28 @@ int acpi_device_fix_up_power(struct acpi
 }
 EXPORT_SYMBOL_GPL(acpi_device_fix_up_power);
 
+static int fix_up_power_if_applicable(struct acpi_device *adev, void *not_used)
+{
+	if (adev->status.present && adev->status.enabled)
+		acpi_device_fix_up_power(adev);
+
+	return 0;
+}
+
+/**
+ * acpi_device_fix_up_power_extended - Force device and its children into D0.
+ * @adev: Parent device object whose power state is to be fixed up.
+ *
+ * Call acpi_device_fix_up_power() for @adev and its children so long as they
+ * are reported as present and enabled.
+ */
+void acpi_device_fix_up_power_extended(struct acpi_device *adev)
+{
+	acpi_device_fix_up_power(adev);
+	acpi_dev_for_each_child(adev, fix_up_power_if_applicable, NULL);
+}
+EXPORT_SYMBOL_GPL(acpi_device_fix_up_power_extended);
+
 int acpi_device_update_power(struct acpi_device *device, int *state_p)
 {
 	int state;
Index: linux-pm/include/acpi/acpi_bus.h
===================================================================
--- linux-pm.orig/include/acpi/acpi_bus.h
+++ linux-pm/include/acpi/acpi_bus.h
@@ -524,6 +524,7 @@ const char *acpi_power_state_string(int
 int acpi_device_set_power(struct acpi_device *device, int state);
 int acpi_bus_init_power(struct acpi_device *device);
 int acpi_device_fix_up_power(struct acpi_device *device);
+void acpi_device_fix_up_power_extended(struct acpi_device *adev);
 int acpi_bus_update_power(acpi_handle handle, int *state_p);
 int acpi_device_update_power(struct acpi_device *device, int *state_p);
 bool acpi_bus_power_manageable(acpi_handle handle);
Index: linux-pm/drivers/mmc/host/sdhci-pci-core.c
===================================================================
--- linux-pm.orig/drivers/mmc/host/sdhci-pci-core.c
+++ linux-pm/drivers/mmc/host/sdhci-pci-core.c
@@ -1240,16 +1240,11 @@ static const struct sdhci_pci_fixes sdhc
 #ifdef CONFIG_ACPI
 static void intel_mrfld_mmc_fix_up_power_slot(struct sdhci_pci_slot *slot)
 {
-	struct acpi_device *device, *child;
+	struct acpi_device *device;
 
 	device = ACPI_COMPANION(&slot->chip->pdev->dev);
-	if (!device)
-		return;
-
-	acpi_device_fix_up_power(device);
-	list_for_each_entry(child, &device->children, node)
-		if (child->status.present && child->status.enabled)
-			acpi_device_fix_up_power(child);
+	if (device)
+		acpi_device_fix_up_power_extended(device);
 }
 #else
 static inline void intel_mrfld_mmc_fix_up_power_slot(struct sdhci_pci_slot *slot) {}

Re: [PATCH v1 15/16] ACPI / MMC: PM: Unify fixing up device power

[Adrian Hunter]

On 9/06/22 17:18, Rafael J. Wysocki wrote:
> From: Rafael J. Wysocki <rafael.j.wysocki@intel.com>
> 
> Introduce acpi_device_fix_up_power_extended() for fixing up power of
> a device having an ACPI companion in a manner that takes the device's
> children into account and make the MMC code use it in two places
> instead of walking the list of the device ACPI companion's children
> directly.
> 
> 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>

Acked-by: Adrian Hunter <adrian.hunter@intel.com>

> ---
>  drivers/acpi/device_pm.c          |   22 ++++++++++++++++++++++
>  drivers/mmc/host/sdhci-acpi.c     |    7 ++-----
>  drivers/mmc/host/sdhci-pci-core.c |   11 +++--------
>  include/acpi/acpi_bus.h           |    1 +
>  4 files changed, 28 insertions(+), 13 deletions(-)
> 
> Index: linux-pm/drivers/mmc/host/sdhci-acpi.c
> ===================================================================
> --- linux-pm.orig/drivers/mmc/host/sdhci-acpi.c
> +++ linux-pm/drivers/mmc/host/sdhci-acpi.c
> @@ -775,8 +775,8 @@ static int sdhci_acpi_probe(struct platf
>  {
>  	struct device *dev = &pdev->dev;
>  	const struct sdhci_acpi_slot *slot;
> -	struct acpi_device *device, *child;
>  	const struct dmi_system_id *id;
> +	struct acpi_device *device;
>  	struct sdhci_acpi_host *c;
>  	struct sdhci_host *host;
>  	struct resource *iomem;
> @@ -796,10 +796,7 @@ static int sdhci_acpi_probe(struct platf
>  	slot = sdhci_acpi_get_slot(device);
>  
>  	/* Power on the SDHCI controller and its children */
> -	acpi_device_fix_up_power(device);
> -	list_for_each_entry(child, &device->children, node)
> -		if (child->status.present && child->status.enabled)
> -			acpi_device_fix_up_power(child);
> +	acpi_device_fix_up_power_extended(device);
>  
>  	if (sdhci_acpi_byt_defer(dev))
>  		return -EPROBE_DEFER;
> Index: linux-pm/drivers/acpi/device_pm.c
> ===================================================================
> --- linux-pm.orig/drivers/acpi/device_pm.c
> +++ linux-pm/drivers/acpi/device_pm.c
> @@ -369,6 +369,28 @@ int acpi_device_fix_up_power(struct acpi
>  }
>  EXPORT_SYMBOL_GPL(acpi_device_fix_up_power);
>  
> +static int fix_up_power_if_applicable(struct acpi_device *adev, void *not_used)
> +{
> +	if (adev->status.present && adev->status.enabled)
> +		acpi_device_fix_up_power(adev);
> +
> +	return 0;
> +}
> +
> +/**
> + * acpi_device_fix_up_power_extended - Force device and its children into D0.
> + * @adev: Parent device object whose power state is to be fixed up.
> + *
> + * Call acpi_device_fix_up_power() for @adev and its children so long as they
> + * are reported as present and enabled.
> + */
> +void acpi_device_fix_up_power_extended(struct acpi_device *adev)
> +{
> +	acpi_device_fix_up_power(adev);
> +	acpi_dev_for_each_child(adev, fix_up_power_if_applicable, NULL);
> +}
> +EXPORT_SYMBOL_GPL(acpi_device_fix_up_power_extended);
> +
>  int acpi_device_update_power(struct acpi_device *device, int *state_p)
>  {
>  	int state;
> Index: linux-pm/include/acpi/acpi_bus.h
> ===================================================================
> --- linux-pm.orig/include/acpi/acpi_bus.h
> +++ linux-pm/include/acpi/acpi_bus.h
> @@ -524,6 +524,7 @@ const char *acpi_power_state_string(int
>  int acpi_device_set_power(struct acpi_device *device, int state);
>  int acpi_bus_init_power(struct acpi_device *device);
>  int acpi_device_fix_up_power(struct acpi_device *device);
> +void acpi_device_fix_up_power_extended(struct acpi_device *adev);
>  int acpi_bus_update_power(acpi_handle handle, int *state_p);
>  int acpi_device_update_power(struct acpi_device *device, int *state_p);
>  bool acpi_bus_power_manageable(acpi_handle handle);
> Index: linux-pm/drivers/mmc/host/sdhci-pci-core.c
> ===================================================================
> --- linux-pm.orig/drivers/mmc/host/sdhci-pci-core.c
> +++ linux-pm/drivers/mmc/host/sdhci-pci-core.c
> @@ -1240,16 +1240,11 @@ static const struct sdhci_pci_fixes sdhc
>  #ifdef CONFIG_ACPI
>  static void intel_mrfld_mmc_fix_up_power_slot(struct sdhci_pci_slot *slot)
>  {
> -	struct acpi_device *device, *child;
> +	struct acpi_device *device;
>  
>  	device = ACPI_COMPANION(&slot->chip->pdev->dev);
> -	if (!device)
> -		return;
> -
> -	acpi_device_fix_up_power(device);
> -	list_for_each_entry(child, &device->children, node)
> -		if (child->status.present && child->status.enabled)
> -			acpi_device_fix_up_power(child);
> +	if (device)
> +		acpi_device_fix_up_power_extended(device);
>  }
>  #else
>  static inline void intel_mrfld_mmc_fix_up_power_slot(struct sdhci_pci_slot *slot) {}
> 
> 
> 

Re: [PATCH v1 15/16] ACPI / MMC: PM: Unify fixing up device power

[Ulf Hansson]

On Thu, 9 Jun 2022 at 16:20, Rafael J. Wysocki <rjw@rjwysocki.net> wrote:
>
> From: Rafael J. Wysocki <rafael.j.wysocki@intel.com>
>
> Introduce acpi_device_fix_up_power_extended() for fixing up power of
> a device having an ACPI companion in a manner that takes the device's
> children into account and make the MMC code use it in two places
> instead of walking the list of the device ACPI companion's children
> directly.
>
> 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>

Rafael, feel free to pick this via your tree.

Acked-by: Ulf Hansson <ulf.hansson@linaro.org>

Kind regards
Uffe

> ---
>  drivers/acpi/device_pm.c          |   22 ++++++++++++++++++++++
>  drivers/mmc/host/sdhci-acpi.c     |    7 ++-----
>  drivers/mmc/host/sdhci-pci-core.c |   11 +++--------
>  include/acpi/acpi_bus.h           |    1 +
>  4 files changed, 28 insertions(+), 13 deletions(-)
>
> Index: linux-pm/drivers/mmc/host/sdhci-acpi.c
> ===================================================================
> --- linux-pm.orig/drivers/mmc/host/sdhci-acpi.c
> +++ linux-pm/drivers/mmc/host/sdhci-acpi.c
> @@ -775,8 +775,8 @@ static int sdhci_acpi_probe(struct platf
>  {
>         struct device *dev = &pdev->dev;
>         const struct sdhci_acpi_slot *slot;
> -       struct acpi_device *device, *child;
>         const struct dmi_system_id *id;
> +       struct acpi_device *device;
>         struct sdhci_acpi_host *c;
>         struct sdhci_host *host;
>         struct resource *iomem;
> @@ -796,10 +796,7 @@ static int sdhci_acpi_probe(struct platf
>         slot = sdhci_acpi_get_slot(device);
>
>         /* Power on the SDHCI controller and its children */
> -       acpi_device_fix_up_power(device);
> -       list_for_each_entry(child, &device->children, node)
> -               if (child->status.present && child->status.enabled)
> -                       acpi_device_fix_up_power(child);
> +       acpi_device_fix_up_power_extended(device);
>
>         if (sdhci_acpi_byt_defer(dev))
>                 return -EPROBE_DEFER;
> Index: linux-pm/drivers/acpi/device_pm.c
> ===================================================================
> --- linux-pm.orig/drivers/acpi/device_pm.c
> +++ linux-pm/drivers/acpi/device_pm.c
> @@ -369,6 +369,28 @@ int acpi_device_fix_up_power(struct acpi
>  }
>  EXPORT_SYMBOL_GPL(acpi_device_fix_up_power);
>
> +static int fix_up_power_if_applicable(struct acpi_device *adev, void *not_used)
> +{
> +       if (adev->status.present && adev->status.enabled)
> +               acpi_device_fix_up_power(adev);
> +
> +       return 0;
> +}
> +
> +/**
> + * acpi_device_fix_up_power_extended - Force device and its children into D0.
> + * @adev: Parent device object whose power state is to be fixed up.
> + *
> + * Call acpi_device_fix_up_power() for @adev and its children so long as they
> + * are reported as present and enabled.
> + */
> +void acpi_device_fix_up_power_extended(struct acpi_device *adev)
> +{
> +       acpi_device_fix_up_power(adev);
> +       acpi_dev_for_each_child(adev, fix_up_power_if_applicable, NULL);
> +}
> +EXPORT_SYMBOL_GPL(acpi_device_fix_up_power_extended);
> +
>  int acpi_device_update_power(struct acpi_device *device, int *state_p)
>  {
>         int state;
> Index: linux-pm/include/acpi/acpi_bus.h
> ===================================================================
> --- linux-pm.orig/include/acpi/acpi_bus.h
> +++ linux-pm/include/acpi/acpi_bus.h
> @@ -524,6 +524,7 @@ const char *acpi_power_state_string(int
>  int acpi_device_set_power(struct acpi_device *device, int state);
>  int acpi_bus_init_power(struct acpi_device *device);
>  int acpi_device_fix_up_power(struct acpi_device *device);
> +void acpi_device_fix_up_power_extended(struct acpi_device *adev);
>  int acpi_bus_update_power(acpi_handle handle, int *state_p);
>  int acpi_device_update_power(struct acpi_device *device, int *state_p);
>  bool acpi_bus_power_manageable(acpi_handle handle);
> Index: linux-pm/drivers/mmc/host/sdhci-pci-core.c
> ===================================================================
> --- linux-pm.orig/drivers/mmc/host/sdhci-pci-core.c
> +++ linux-pm/drivers/mmc/host/sdhci-pci-core.c
> @@ -1240,16 +1240,11 @@ static const struct sdhci_pci_fixes sdhc
>  #ifdef CONFIG_ACPI
>  static void intel_mrfld_mmc_fix_up_power_slot(struct sdhci_pci_slot *slot)
>  {
> -       struct acpi_device *device, *child;
> +       struct acpi_device *device;
>
>         device = ACPI_COMPANION(&slot->chip->pdev->dev);
> -       if (!device)
> -               return;
> -
> -       acpi_device_fix_up_power(device);
> -       list_for_each_entry(child, &device->children, node)
> -               if (child->status.present && child->status.enabled)
> -                       acpi_device_fix_up_power(child);
> +       if (device)
> +               acpi_device_fix_up_power_extended(device);
>  }
>  #else
>  static inline void intel_mrfld_mmc_fix_up_power_slot(struct sdhci_pci_slot *slot) {}
>
>
>

[PATCH v1 16/16] ACPI: bus: Drop unused list heads from struct acpi_device

[Rafael J. Wysocki]

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

Drop the children and node list heads that have no more users
from struct acpi_device and the code manipulating them from
__acpi_device_add() and acpi_device_del().

Signed-off-by: Rafael J. Wysocki <rafael.j.wysocki@intel.com>
---
 drivers/acpi/scan.c     |   11 +----------
 include/acpi/acpi_bus.h |    2 --
 2 files changed, 1 insertion(+), 12 deletions(-)

Index: linux-pm/include/acpi/acpi_bus.h
===================================================================
--- linux-pm.orig/include/acpi/acpi_bus.h
+++ linux-pm/include/acpi/acpi_bus.h
@@ -365,8 +365,6 @@ struct acpi_device {
 	acpi_handle handle;		/* no handle for fixed hardware */
 	struct fwnode_handle fwnode;
 	struct acpi_device *parent;
-	struct list_head children;
-	struct list_head node;
 	struct list_head wakeup_list;
 	struct list_head del_list;
 	struct acpi_device_status status;
Index: linux-pm/drivers/acpi/scan.c
===================================================================
--- linux-pm.orig/drivers/acpi/scan.c
+++ linux-pm/drivers/acpi/scan.c
@@ -465,8 +465,6 @@ static void acpi_device_del(struct acpi_
 	struct acpi_device_bus_id *acpi_device_bus_id;
 
 	mutex_lock(&acpi_device_lock);
-	if (device->parent)
-		list_del(&device->node);
 
 	list_for_each_entry(acpi_device_bus_id, &acpi_bus_id_list, node)
 		if (!strcmp(acpi_device_bus_id->bus_id,
@@ -482,6 +480,7 @@ static void acpi_device_del(struct acpi_
 		}
 
 	list_del(&device->wakeup_list);
+
 	mutex_unlock(&acpi_device_lock);
 
 	acpi_power_add_remove_device(device, false);
@@ -674,8 +673,6 @@ static int __acpi_device_add(struct acpi
 	 * -------
 	 * Link this device to its parent and siblings.
 	 */
-	INIT_LIST_HEAD(&device->children);
-	INIT_LIST_HEAD(&device->node);
 	INIT_LIST_HEAD(&device->wakeup_list);
 	INIT_LIST_HEAD(&device->physical_node_list);
 	INIT_LIST_HEAD(&device->del_list);
@@ -715,9 +712,6 @@ static int __acpi_device_add(struct acpi
 		list_add_tail(&acpi_device_bus_id->node, &acpi_bus_id_list);
 	}
 
-	if (device->parent)
-		list_add_tail(&device->node, &device->parent->children);
-
 	if (device->wakeup.flags.valid)
 		list_add_tail(&device->wakeup_list, &acpi_wakeup_device_list);
 
@@ -746,9 +740,6 @@ static int __acpi_device_add(struct acpi
 err:
 	mutex_lock(&acpi_device_lock);
 
-	if (device->parent)
-		list_del(&device->node);
-
 	list_del(&device->wakeup_list);
 
 err_unlock:

Re: [PATCH v1 00/16] ACPI: Get rid of the list of children in struct acpi_device

[Andy Shevchenko]

On Thu, Jun 09, 2022 at 03:44:27PM +0200, 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).

Hmm... Does it true for DT overlays? Not an expert in DT overlays, though,
adding Rob and Frank.

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

I'm going to look the individual patches.

-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH v1 00/16] ACPI: Get rid of the list of children in struct acpi_device

[Frank Rowand]

On 6/9/22 11:12, Andy Shevchenko wrote:
> On Thu, Jun 09, 2022 at 03:44:27PM +0200, 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).
> 
> Hmm... Does it true for DT overlays? Not an expert in DT overlays, though,
> adding Rob and Frank.

DT nodes can be removed.  The devicetree code uses devtree_lock and of_mutex
as needed for protection.

-Frank

> 
>> 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.
> 
> I'm going to look the individual patches.
> 

Re: [PATCH v1 00/16] ACPI: Get rid of the list of children in struct acpi_device

[Andy Shevchenko]

On Thu, Jun 09, 2022 at 03:44:27PM +0200, 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.

Cool series, thanks for doing that!

You may add my
Revieweed-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
to all non-commented, by me, patches (excluding soundwire) and to ones
where comment just about one line/two lines split (address them if you
are okay, otherwise ignore those comments).

-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH v1 00/16] ACPI: Get rid of the list of children in struct acpi_device

[Rafael J. Wysocki]

On Thu, Jun 9, 2022 at 5:57 PM Andy Shevchenko
<andriy.shevchenko@linux.intel.com> wrote:
>
> On Thu, Jun 09, 2022 at 03:44:27PM +0200, 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.
>
> Cool series, thanks for doing that!
>
> You may add my
> Revieweed-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
> to all non-commented, by me, patches (excluding soundwire) and to ones
> where comment just about one line/two lines split (address them if you
> are okay, otherwise ignore those comments).

Thank you!

[PATCH v2 00/16] ACPI: Get rid of the list of children in struct acpi_device

[Rafael J. Wysocki]

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!

[PATCH v2 01/16] ACPI: glue: Use acpi_dev_for_each_child()

[Rafael J. Wysocki]

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>
Reviewed-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
---

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

---
 drivers/acpi/glue.c |  103 +++++++++++++++++++++++++++++++---------------------
 1 file changed, 63 insertions(+), 40 deletions(-)

Index: linux-pm/drivers/acpi/glue.c
===================================================================
--- linux-pm.orig/drivers/acpi/glue.c
+++ linux-pm/drivers/acpi/glue.c
@@ -105,51 +105,74 @@ static int find_child_checks(struct acpi
 	return FIND_CHILD_MAX_SCORE;
 }
 
+struct find_child_walk_data {
+	struct acpi_device *adev;
+	u64 address;
+	int score;
+	bool check_children;
+};
+
+static int check_one_child(struct acpi_device *adev, void *data)
+{
+	struct find_child_walk_data *wd = data;
+	int score;
+
+	if (!adev->pnp.type.bus_address || acpi_device_adr(adev) != wd->address)
+		return 0;
+
+	if (!wd->adev) {
+		/* This is the first matching object.  Save it and continue. */
+		wd->adev = adev;
+		return 0;
+	}
+
+	/*
+	 * There is more than one matching device object with the same _ADR
+	 * value.  That really is unexpected, so we are kind of beyond the scope
+	 * of the spec here.  We have to choose which one to return, though.
+	 *
+	 * First, get the score for the previously found object and terminate
+	 * the walk if it is maximum.
+	*/
+	if (!wd->score) {
+		score = find_child_checks(wd->adev, wd->check_children);
+		if (score == FIND_CHILD_MAX_SCORE)
+			return 1;
+
+		wd->score = score;
+	}
+	/*
+	 * Second, if the object that has just been found has a better score,
+	 * replace the previously found one with it and terminate the walk if
+	 * the new score is maximum.
+	 */
+	score = find_child_checks(adev, wd->check_children);
+	if (score > wd->score) {
+		wd->adev = adev;
+		if (score == FIND_CHILD_MAX_SCORE)
+			return 1;
+
+		wd->score = score;
+	}
+
+	/* Continue, because there may be better matches. */
+	return 0;
+}
+
 struct acpi_device *acpi_find_child_device(struct acpi_device *parent,
 					   u64 address, bool check_children)
 {
-	struct acpi_device *adev, *ret = NULL;
-	int ret_score = 0;
+	struct find_child_walk_data wd = {
+		.address = address,
+		.check_children = check_children,
+		.adev = NULL,
+		.score = 0,
+	};
 
-	if (!parent)
-		return NULL;
+	if (parent)
+		acpi_dev_for_each_child(parent, check_one_child, &wd);
 
-	list_for_each_entry(adev, &parent->children, node) {
-		acpi_bus_address addr = acpi_device_adr(adev);
-		int score;
-
-		if (!adev->pnp.type.bus_address || addr != address)
-			continue;
-
-		if (!ret) {
-			/* This is the first matching object.  Save it. */
-			ret = adev;
-			continue;
-		}
-		/*
-		 * There is more than one matching device object with the same
-		 * _ADR value.  That really is unexpected, so we are kind of
-		 * beyond the scope of the spec here.  We have to choose which
-		 * one to return, though.
-		 *
-		 * First, check if the previously found object is good enough
-		 * and return it if so.  Second, do the same for the object that
-		 * we've just found.
-		 */
-		if (!ret_score) {
-			ret_score = find_child_checks(ret, check_children);
-			if (ret_score == FIND_CHILD_MAX_SCORE)
-				return ret;
-		}
-		score = find_child_checks(adev, check_children);
-		if (score == FIND_CHILD_MAX_SCORE) {
-			return adev;
-		} else if (score > ret_score) {
-			ret = adev;
-			ret_score = score;
-		}
-	}
-	return ret;
+	return wd.adev;
 }
 EXPORT_SYMBOL_GPL(acpi_find_child_device);
 

[PATCH v2 02/16] ACPI: glue: Introduce acpi_dev_has_children()

[Rafael J. Wysocki]

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

Define acpi_dev_has_children() as a wrapper around
acpi_dev_for_each_child() and use it to check if the given ACPI
device has any children instead of checking the children list
head in struct acpi_device.

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: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
---

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

---
 drivers/acpi/glue.c |   12 +++++++++++-
 1 file changed, 11 insertions(+), 1 deletion(-)

Index: linux-pm/drivers/acpi/glue.c
===================================================================
--- linux-pm.orig/drivers/acpi/glue.c
+++ linux-pm/drivers/acpi/glue.c
@@ -77,12 +77,22 @@ static struct acpi_bus_type *acpi_get_bu
 #define FIND_CHILD_MIN_SCORE	1
 #define FIND_CHILD_MAX_SCORE	2
 
+static int match_any(struct acpi_device *adev, void *not_used)
+{
+	return 1;
+}
+
+static bool acpi_dev_has_children(struct acpi_device *adev)
+{
+	return acpi_dev_for_each_child(adev, match_any, NULL) > 0;
+}
+
 static int find_child_checks(struct acpi_device *adev, bool check_children)
 {
 	unsigned long long sta;
 	acpi_status status;
 
-	if (check_children && list_empty(&adev->children))
+	if (check_children && !acpi_dev_has_children(adev))
 		return -ENODEV;
 
 	status = acpi_evaluate_integer(adev->handle, "_STA", NULL, &sta);

[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 06/16] ACPI: container: Use acpi_dev_for_each_child()

[Rafael J. Wysocki]

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>
Reviewed-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
---

v1 -> v2:
   * Do not break the acpi_dev_for_each_child() call line (Andy).
   * Add R-by from Andy.

---
 drivers/acpi/container.c |   17 +++++++++--------
 1 file changed, 9 insertions(+), 8 deletions(-)

Index: linux-pm/drivers/acpi/container.c
===================================================================
--- linux-pm.orig/drivers/acpi/container.c
+++ linux-pm/drivers/acpi/container.c
@@ -23,17 +23,18 @@ static const struct acpi_device_id conta
 
 #ifdef CONFIG_ACPI_CONTAINER
 
-static int acpi_container_offline(struct container_dev *cdev)
+static int check_offline(struct acpi_device *adev, void *not_used)
 {
-	struct acpi_device *adev = ACPI_COMPANION(&cdev->dev);
-	struct acpi_device *child;
+	if (acpi_scan_is_offline(adev, false))
+		return 0;
 
-	/* Check all of the dependent devices' physical companions. */
-	list_for_each_entry(child, &adev->children, node)
-		if (!acpi_scan_is_offline(child, false))
-			return -EBUSY;
+	return -EBUSY;
+}
 
-	return 0;
+static int acpi_container_offline(struct container_dev *cdev)
+{
+	/* Check all of the dependent devices' physical companions. */
+	return acpi_dev_for_each_child(ACPI_COMPANION(&cdev->dev), check_offline, NULL);
 }
 
 static void acpi_container_release(struct device *dev)

[PATCH v2 07/16] ACPI: property: Use acpi_dev_for_each_child() for child lookup

[Rafael J. Wysocki]

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

Instead of using the list of children of an ACPI device directly,
use acpi_dev_for_each_child() to find the next child of a given
ACPI device.

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: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
---

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

---
 drivers/acpi/property.c |   47 +++++++++++++++++++++++++----------------------
 1 file changed, 25 insertions(+), 22 deletions(-)

Index: linux-pm/drivers/acpi/property.c
===================================================================
--- linux-pm.orig/drivers/acpi/property.c
+++ linux-pm/drivers/acpi/property.c
@@ -1012,6 +1012,22 @@ static int acpi_node_prop_read(const str
 				   propname, proptype, val, nval);
 }
 
+static int stop_on_next(struct acpi_device *adev, void *data)
+{
+	struct acpi_device **ret_p = data;
+
+	if (!*ret_p) {
+		*ret_p = adev;
+		return 1;
+	}
+
+	/* Skip until the "previous" object is found. */
+	if (*ret_p == adev)
+		*ret_p = NULL;
+
+	return 0;
+}
+
 /**
  * acpi_get_next_subnode - Return the next child node handle for a fwnode
  * @fwnode: Firmware node to find the next child node for.
@@ -1020,35 +1036,22 @@ static int acpi_node_prop_read(const str
 struct fwnode_handle *acpi_get_next_subnode(const struct fwnode_handle *fwnode,
 					    struct fwnode_handle *child)
 {
-	const struct acpi_device *adev = to_acpi_device_node(fwnode);
-	const struct list_head *head;
-	struct list_head *next;
+	struct acpi_device *adev = to_acpi_device_node(fwnode);
 
 	if ((!child || is_acpi_device_node(child)) && adev) {
-		struct acpi_device *child_adev;
+		struct acpi_device *child_adev = to_acpi_device_node(child);
+
+		acpi_dev_for_each_child(adev, stop_on_next, &child_adev);
+		if (child_adev)
+			return acpi_fwnode_handle(child_adev);
 
-		head = &adev->children;
-		if (list_empty(head))
-			goto nondev;
-
-		if (child) {
-			adev = to_acpi_device_node(child);
-			next = adev->node.next;
-			if (next == head) {
-				child = NULL;
-				goto nondev;
-			}
-			child_adev = list_entry(next, struct acpi_device, node);
-		} else {
-			child_adev = list_first_entry(head, struct acpi_device,
-						      node);
-		}
-		return acpi_fwnode_handle(child_adev);
+		child = NULL;
 	}
 
- nondev:
 	if (!child || is_acpi_data_node(child)) {
 		const struct acpi_data_node *data = to_acpi_data_node(fwnode);
+		const struct list_head *head;
+		struct list_head *next;
 		struct acpi_data_node *dn;
 
 		/*

[PATCH v2 08/16] ACPI: bus: Export acpi_dev_for_each_child() to modules

[Rafael J. Wysocki]

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

Some pieces of modular code can benefit from using
acpi_dev_for_each_child(), so export it to modules.

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/bus.c |    1 +
 1 file changed, 1 insertion(+)

Index: linux-pm/drivers/acpi/bus.c
===================================================================
--- linux-pm.orig/drivers/acpi/bus.c
+++ linux-pm/drivers/acpi/bus.c
@@ -1102,6 +1102,7 @@ static int acpi_dev_for_one_check(struct
 
 	return adwc->fn(to_acpi_device(dev), adwc->data);
 }
+EXPORT_SYMBOL_GPL(acpi_dev_for_each_child);
 
 int acpi_dev_for_each_child(struct acpi_device *adev,
 			    int (*fn)(struct acpi_device *, void *), void *data)

[PATCH v2 09/16] ACPI: video: Use acpi_dev_for_each_child()

[Rafael J. Wysocki]

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>
Reviewed-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
---

v1 -> v2:
   * Do not break the acpi_dev_for_each_child() call line (Andy).
   * Add R-by from Andy.

---
 drivers/acpi/acpi_video.c |   41 ++++++++++++++++-------------------------
 1 file changed, 16 insertions(+), 25 deletions(-)

Index: linux-pm/drivers/acpi/acpi_video.c
===================================================================
--- linux-pm.orig/drivers/acpi/acpi_video.c
+++ linux-pm/drivers/acpi/acpi_video.c
@@ -1149,24 +1149,25 @@ acpi_video_get_device_type(struct acpi_v
 	return 0;
 }
 
-static int
-acpi_video_bus_get_one_device(struct acpi_device *device,
-			      struct acpi_video_bus *video)
+static int acpi_video_bus_get_one_device(struct acpi_device *device, void *arg)
 {
-	unsigned long long device_id;
-	int status, device_type;
-	struct acpi_video_device *data;
+	struct acpi_video_bus *video = arg;
 	struct acpi_video_device_attrib *attribute;
+	struct acpi_video_device *data;
+	unsigned long long device_id;
+	acpi_status status;
+	int device_type;
 
-	status =
-	    acpi_evaluate_integer(device->handle, "_ADR", NULL, &device_id);
-	/* Some device omits _ADR, we skip them instead of fail */
+	status = acpi_evaluate_integer(device->handle, "_ADR", NULL, &device_id);
+	/* Skip devices without _ADR instead of failing. */
 	if (ACPI_FAILURE(status))
-		return 0;
+		goto exit;
 
 	data = kzalloc(sizeof(struct acpi_video_device), GFP_KERNEL);
-	if (!data)
+	if (!data) {
+		dev_dbg(&device->dev, "Cannot attach\n");
 		return -ENOMEM;
+	}
 
 	strcpy(acpi_device_name(device), ACPI_VIDEO_DEVICE_NAME);
 	strcpy(acpi_device_class(device), ACPI_VIDEO_CLASS);
@@ -1226,7 +1227,9 @@ acpi_video_bus_get_one_device(struct acp
 	list_add_tail(&data->entry, &video->video_device_list);
 	mutex_unlock(&video->device_list_lock);
 
-	return status;
+exit:
+	video->child_count++;
+	return 0;
 }
 
 /*
@@ -1538,9 +1541,6 @@ static int
 acpi_video_bus_get_devices(struct acpi_video_bus *video,
 			   struct acpi_device *device)
 {
-	int status = 0;
-	struct acpi_device *dev;
-
 	/*
 	 * There are systems where video module known to work fine regardless
 	 * of broken _DOD and ignoring returned value here doesn't cause
@@ -1548,16 +1548,7 @@ acpi_video_bus_get_devices(struct acpi_v
 	 */
 	acpi_video_device_enumerate(video);
 
-	list_for_each_entry(dev, &device->children, node) {
-
-		status = acpi_video_bus_get_one_device(dev, video);
-		if (status) {
-			dev_err(&dev->dev, "Can't attach device\n");
-			break;
-		}
-		video->child_count++;
-	}
-	return status;
+	return acpi_dev_for_each_child(device, acpi_video_bus_get_one_device, video);
 }
 
 /* acpi_video interface */

[PATCH v2 10/16] ACPI: bus: Introduce acpi_dev_for_each_child_reverse()

[Rafael J. Wysocki]

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

Make it possible to walk the children of an ACPI device in the revese
order by defining acpi_dev_for_each_child_reverse() in analogy with
acpi_dev_for_each_child().

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

v1 -> v2:
   * Do not break the acpi_dev_for_each_child() call line (Andy).
   * Add R-by from Andy.

---
 drivers/acpi/bus.c      |   12 ++++++++++++
 include/acpi/acpi_bus.h |    3 +++
 2 files changed, 15 insertions(+)

Index: linux-pm/drivers/acpi/bus.c
===================================================================
--- linux-pm.orig/drivers/acpi/bus.c
+++ linux-pm/drivers/acpi/bus.c
@@ -1115,6 +1115,18 @@ int acpi_dev_for_each_child(struct acpi_
 	return device_for_each_child(&adev->dev, &adwc, acpi_dev_for_one_check);
 }
 
+int acpi_dev_for_each_child_reverse(struct acpi_device *adev,
+				    int (*fn)(struct acpi_device *, void *),
+				    void *data)
+{
+	struct acpi_dev_walk_context adwc = {
+		.fn = fn,
+		.data = data,
+	};
+
+	return device_for_each_child_reverse(&adev->dev, &adwc, acpi_dev_for_one_check);
+}
+
 /* --------------------------------------------------------------------------
                              Initialization/Cleanup
    -------------------------------------------------------------------------- */
Index: linux-pm/include/acpi/acpi_bus.h
===================================================================
--- linux-pm.orig/include/acpi/acpi_bus.h
+++ linux-pm/include/acpi/acpi_bus.h
@@ -483,6 +483,9 @@ extern struct bus_type acpi_bus_type;
 int acpi_bus_for_each_dev(int (*fn)(struct device *, void *), void *data);
 int acpi_dev_for_each_child(struct acpi_device *adev,
 			    int (*fn)(struct acpi_device *, void *), void *data);
+int acpi_dev_for_each_child_reverse(struct acpi_device *adev,
+				    int (*fn)(struct acpi_device *, void *),
+				    void *data);
 
 /*
  * Events

[PATCH v2 11/16] ACPI: scan: Walk ACPI device's children using driver core

[Rafael J. Wysocki]

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() or acpi_dev_for_each_child_reverse() 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>
Reviewed-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
---

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

---
 drivers/acpi/scan.c |   59 +++++++++++++++++++++++++---------------------------
 1 file changed, 29 insertions(+), 30 deletions(-)

Index: linux-pm/drivers/acpi/scan.c
===================================================================
--- linux-pm.orig/drivers/acpi/scan.c
+++ linux-pm/drivers/acpi/scan.c
@@ -334,10 +334,9 @@ static int acpi_scan_device_check(struct
 	return error;
 }
 
-static int acpi_scan_bus_check(struct acpi_device *adev)
+static int acpi_scan_bus_check(struct acpi_device *adev, void *not_used)
 {
 	struct acpi_scan_handler *handler = adev->handler;
-	struct acpi_device *child;
 	int error;
 
 	acpi_bus_get_status(adev);
@@ -353,19 +352,14 @@ static int acpi_scan_bus_check(struct ac
 		dev_warn(&adev->dev, "Namespace scan failure\n");
 		return error;
 	}
-	list_for_each_entry(child, &adev->children, node) {
-		error = acpi_scan_bus_check(child);
-		if (error)
-			return error;
-	}
-	return 0;
+	return acpi_dev_for_each_child(adev, acpi_scan_bus_check, NULL);
 }
 
 static int acpi_generic_hotplug_event(struct acpi_device *adev, u32 type)
 {
 	switch (type) {
 	case ACPI_NOTIFY_BUS_CHECK:
-		return acpi_scan_bus_check(adev);
+		return acpi_scan_bus_check(adev, NULL);
 	case ACPI_NOTIFY_DEVICE_CHECK:
 		return acpi_scan_device_check(adev);
 	case ACPI_NOTIFY_EJECT_REQUEST:
@@ -2187,9 +2181,8 @@ static int acpi_scan_attach_handler(stru
 	return ret;
 }
 
-static void acpi_bus_attach(struct acpi_device *device, bool first_pass)
+static int acpi_bus_attach(struct acpi_device *device, void *first_pass)
 {
-	struct acpi_device *child;
 	bool skip = !first_pass && device->flags.visited;
 	acpi_handle ejd;
 	int ret;
@@ -2206,7 +2199,7 @@ static void acpi_bus_attach(struct acpi_
 		device->flags.initialized = false;
 		acpi_device_clear_enumerated(device);
 		device->flags.power_manageable = 0;
-		return;
+		return 0;
 	}
 	if (device->handler)
 		goto ok;
@@ -2224,7 +2217,7 @@ static void acpi_bus_attach(struct acpi_
 
 	ret = acpi_scan_attach_handler(device);
 	if (ret < 0)
-		return;
+		return 0;
 
 	device->flags.match_driver = true;
 	if (ret > 0 && !device->flags.enumeration_by_parent) {
@@ -2234,19 +2227,20 @@ static void acpi_bus_attach(struct acpi_
 
 	ret = device_attach(&device->dev);
 	if (ret < 0)
-		return;
+		return 0;
 
 	if (device->pnp.type.platform_id || device->flags.enumeration_by_parent)
 		acpi_default_enumeration(device);
 	else
 		acpi_device_set_enumerated(device);
 
- ok:
-	list_for_each_entry(child, &device->children, node)
-		acpi_bus_attach(child, first_pass);
+ok:
+	acpi_dev_for_each_child(device, acpi_bus_attach, first_pass);
 
 	if (!skip && device->handler && device->handler->hotplug.notify_online)
 		device->handler->hotplug.notify_online(device);
+
+	return 0;
 }
 
 static int acpi_dev_get_first_consumer_dev_cb(struct acpi_dep_data *dep, void *data)
@@ -2274,7 +2268,7 @@ static void acpi_scan_clear_dep_fn(struc
 	cdw = container_of(work, struct acpi_scan_clear_dep_work, work);
 
 	acpi_scan_lock_acquire();
-	acpi_bus_attach(cdw->adev, true);
+	acpi_bus_attach(cdw->adev, (void *)true);
 	acpi_scan_lock_release();
 
 	acpi_dev_put(cdw->adev);
@@ -2432,7 +2426,7 @@ int acpi_bus_scan(acpi_handle handle)
 	if (!device)
 		return -ENODEV;
 
-	acpi_bus_attach(device, true);
+	acpi_bus_attach(device, (void *)true);
 
 	if (!acpi_bus_scan_second_pass)
 		return 0;
@@ -2446,25 +2440,17 @@ int acpi_bus_scan(acpi_handle handle)
 				    acpi_bus_check_add_2, NULL, NULL,
 				    (void **)&device);
 
-	acpi_bus_attach(device, false);
+	acpi_bus_attach(device, NULL);
 
 	return 0;
 }
 EXPORT_SYMBOL(acpi_bus_scan);
 
-/**
- * acpi_bus_trim - Detach scan handlers and drivers from ACPI device objects.
- * @adev: Root of the ACPI namespace scope to walk.
- *
- * Must be called under acpi_scan_lock.
- */
-void acpi_bus_trim(struct acpi_device *adev)
+int acpi_bus_trim_one(struct acpi_device *adev, void *not_used)
 {
 	struct acpi_scan_handler *handler = adev->handler;
-	struct acpi_device *child;
 
-	list_for_each_entry_reverse(child, &adev->children, node)
-		acpi_bus_trim(child);
+	acpi_dev_for_each_child_reverse(adev, acpi_bus_trim_one, NULL);
 
 	adev->flags.match_driver = false;
 	if (handler) {
@@ -2482,6 +2468,19 @@ void acpi_bus_trim(struct acpi_device *a
 	acpi_device_set_power(adev, ACPI_STATE_D3_COLD);
 	adev->flags.initialized = false;
 	acpi_device_clear_enumerated(adev);
+
+	return 0;
+}
+
+/**
+ * acpi_bus_trim - Detach scan handlers and drivers from ACPI device objects.
+ * @adev: Root of the ACPI namespace scope to walk.
+ *
+ * Must be called under acpi_scan_lock.
+ */
+void acpi_bus_trim(struct acpi_device *adev)
+{
+	acpi_bus_trim_one(adev, NULL);
 }
 EXPORT_SYMBOL_GPL(acpi_bus_trim);
 

[PATCH v2 12/16] platform/x86/thinkpad_acpi: Use acpi_dev_for_each_child()

[Rafael J. Wysocki]

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

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

Re: [PATCH v2 12/16] platform/x86/thinkpad_acpi: Use acpi_dev_for_each_child()

[Andy Shevchenko]

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

-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH v2 12/16] platform/x86/thinkpad_acpi: Use acpi_dev_for_each_child()

[Hans de Goede]

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

[PATCH v2 13/16] mfd: core: Use acpi_dev_for_each_child()

[Rafael J. Wysocki]

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>
Reviewed-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
---

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

---
 drivers/mfd/mfd-core.c |   31 ++++++++++++++++++++++++-------
 1 file changed, 24 insertions(+), 7 deletions(-)

Index: linux-pm/drivers/mfd/mfd-core.c
===================================================================
--- linux-pm.orig/drivers/mfd/mfd-core.c
+++ linux-pm/drivers/mfd/mfd-core.c
@@ -60,12 +60,29 @@ int mfd_cell_disable(struct platform_dev
 EXPORT_SYMBOL(mfd_cell_disable);
 
 #if IS_ENABLED(CONFIG_ACPI)
+struct match_ids_walk_data {
+	struct acpi_device_id *ids;
+	struct acpi_device *adev;
+};
+
+static int match_device_ids(struct acpi_device *adev, void *data)
+{
+	struct match_ids_walk_data *wd = data;
+
+	if (!acpi_match_device_ids(adev, wd->ids)) {
+		wd->adev = adev;
+		return 1;
+	}
+
+	return 0;
+}
+
 static void mfd_acpi_add_device(const struct mfd_cell *cell,
 				struct platform_device *pdev)
 {
 	const struct mfd_cell_acpi_match *match = cell->acpi_match;
-	struct acpi_device *parent, *child;
 	struct acpi_device *adev = NULL;
+	struct acpi_device *parent;
 
 	parent = ACPI_COMPANION(pdev->dev.parent);
 	if (!parent)
@@ -83,14 +100,14 @@ static void mfd_acpi_add_device(const st
 	if (match) {
 		if (match->pnpid) {
 			struct acpi_device_id ids[2] = {};
+			struct match_ids_walk_data wd = {
+				.adev = NULL,
+				.ids = ids,
+			};
 
 			strlcpy(ids[0].id, match->pnpid, sizeof(ids[0].id));
-			list_for_each_entry(child, &parent->children, node) {
-				if (!acpi_match_device_ids(child, ids)) {
-					adev = child;
-					break;
-				}
-			}
+			acpi_dev_for_each_child(parent, match_device_ids, &wd);
+			adev = wd.adev;
 		} else {
 			adev = acpi_find_child_device(parent, match->adr, false);
 		}

Re: [PATCH v2 13/16] mfd: core: Use acpi_dev_for_each_child()

[Lee Jones]

On Mon, 13 Jun 2022, 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>
> Reviewed-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
> ---
> 
> v1 -> v2:
>    * Add R-by from Andy.
> 
> ---
>  drivers/mfd/mfd-core.c |   31 ++++++++++++++++++++++++-------
>  1 file changed, 24 insertions(+), 7 deletions(-)

Applied, thanks.

-- 
Lee Jones [李琼斯]
Principal Technical Lead - Developer Services
Linaro.org │ Open source software for Arm SoCs
Follow Linaro: Facebook | Twitter | Blog

Re: [PATCH v2 13/16] mfd: core: Use acpi_dev_for_each_child()

[Rafael J. Wysocki]

On Thu, Jun 16, 2022 at 12:39 AM Lee Jones <lee.jones@linaro.org> wrote:
>
> On Mon, 13 Jun 2022, 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>
> > Reviewed-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
> > ---
> >
> > v1 -> v2:
> >    * Add R-by from Andy.
> >
> > ---
> >  drivers/mfd/mfd-core.c |   31 ++++++++++++++++++++++++-------
> >  1 file changed, 24 insertions(+), 7 deletions(-)
>
> Applied, thanks.

Thank you!

Can you please expose a branch containing it for integration?

The last patch in the series depends on this one.

[GIT PULL] Immutable branch between MFD and ACPI due for the v5.20 merge window

[Lee Jones]

Rafael,

As requested.

The following changes since commit f2906aa863381afb0015a9eb7fefad885d4e5a56:

  Linux 5.19-rc1 (2022-06-05 17:18:54 -0700)

are available in the Git repository at:

  git://git.kernel.org/pub/scm/linux/kernel/git/lee/mfd.git tags/ib-mfd-acpi-for-rafael-v5.20

for you to fetch changes up to 0c9b9c2ac0df57b6b5949a51c45043b345698428:

  mfd: core: Use acpi_dev_for_each_child() (2022-06-27 12:22:06 +0100)

----------------------------------------------------------------
Immutable branch between MFD and ACPI due for the v5.20 merge window

----------------------------------------------------------------
Rafael J. Wysocki (1):
      mfd: core: Use acpi_dev_for_each_child()

 drivers/mfd/mfd-core.c | 31 ++++++++++++++++++++++++-------
 1 file changed, 24 insertions(+), 7 deletions(-)

-- 
Lee Jones [李琼斯]
Principal Technical Lead - Developer Services
Linaro.org │ Open source software for Arm SoCs
Follow Linaro: Facebook | Twitter | Blog

Re: [GIT PULL] Immutable branch between MFD and ACPI due for the v5.20 merge window

[Rafael J. Wysocki]

Hi Lee,

On Mon, Jun 27, 2022 at 1:43 PM Lee Jones <lee.jones@linaro.org> wrote:
>
> Rafael,
>
> As requested.
>
> The following changes since commit f2906aa863381afb0015a9eb7fefad885d4e5a56:
>
>   Linux 5.19-rc1 (2022-06-05 17:18:54 -0700)
>
> are available in the Git repository at:
>
>   git://git.kernel.org/pub/scm/linux/kernel/git/lee/mfd.git tags/ib-mfd-acpi-for-rafael-v5.20
>
> for you to fetch changes up to 0c9b9c2ac0df57b6b5949a51c45043b345698428:
>
>   mfd: core: Use acpi_dev_for_each_child() (2022-06-27 12:22:06 +0100)
>
> ----------------------------------------------------------------
> Immutable branch between MFD and ACPI due for the v5.20 merge window
>
> ----------------------------------------------------------------
> Rafael J. Wysocki (1):
>       mfd: core: Use acpi_dev_for_each_child()
>
>  drivers/mfd/mfd-core.c | 31 ++++++++++++++++++++++++-------
>  1 file changed, 24 insertions(+), 7 deletions(-)
>
> --

Pulled, thank you!

[PATCH v2 14/16] soundwire: Use acpi_dev_for_each_child()

[Rafael J. Wysocki]

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>
Reviewed-by: Pierre-Louis Bossart <pierre-louis.bossart@linux.intel.com>
Tested-by: Pierre-Louis Bossart <pierre-louis.bossart@linux.intel.com>
---

v1 -> v2:
   * Make sure errors are not lost (Pierre-Louis).
   * Add R-by and T-by from Pierre-Louis.

---
 drivers/soundwire/slave.c |  117 ++++++++++++++++++++++++++--------------------
 1 file changed, 67 insertions(+), 50 deletions(-)

Index: linux-pm/drivers/soundwire/slave.c
===================================================================
--- linux-pm.orig/drivers/soundwire/slave.c
+++ linux-pm/drivers/soundwire/slave.c
@@ -127,6 +127,71 @@ static bool find_slave(struct sdw_bus *b
 	return true;
 }
 
+struct sdw_acpi_child_walk_data {
+	struct sdw_bus *bus;
+	struct acpi_device *adev;
+	struct sdw_slave_id id;
+	bool ignore_unique_id;
+};
+
+static int sdw_acpi_check_duplicate(struct acpi_device *adev, void *data)
+{
+	struct sdw_acpi_child_walk_data *cwd = data;
+	struct sdw_bus *bus = cwd->bus;
+	struct sdw_slave_id id;
+
+	if (adev == cwd->adev)
+		return 0;
+
+	if (!find_slave(bus, adev, &id))
+		return 0;
+
+	if (cwd->id.sdw_version != id.sdw_version || cwd->id.mfg_id != id.mfg_id ||
+	    cwd->id.part_id != id.part_id || cwd->id.class_id != id.class_id)
+		return 0;
+
+	if (cwd->id.unique_id != id.unique_id) {
+		dev_dbg(bus->dev,
+			"Valid unique IDs 0x%x 0x%x for Slave mfg_id 0x%04x, part_id 0x%04x\n",
+			cwd->id.unique_id, id.unique_id, cwd->id.mfg_id,
+			cwd->id.part_id);
+		cwd->ignore_unique_id = false;
+		return 0;
+	}
+
+	dev_err(bus->dev,
+		"Invalid unique IDs 0x%x 0x%x for Slave mfg_id 0x%04x, part_id 0x%04x\n",
+		cwd->id.unique_id, id.unique_id, cwd->id.mfg_id, cwd->id.part_id);
+	return -ENODEV;
+}
+
+static int sdw_acpi_find_one(struct acpi_device *adev, void *data)
+{
+	struct sdw_bus *bus = data;
+	struct sdw_acpi_child_walk_data cwd = {
+		.bus = bus,
+		.adev = adev,
+		.ignore_unique_id = true,
+	};
+	int ret;
+
+	if (!find_slave(bus, adev, &cwd.id))
+		return 0;
+
+	/* Brute-force O(N^2) search for duplicates. */
+	ret = acpi_dev_for_each_child(ACPI_COMPANION(bus->dev),
+				      sdw_acpi_check_duplicate, &cwd);
+	if (ret)
+		return ret;
+
+	if (cwd.ignore_unique_id)
+		cwd.id.unique_id = SDW_IGNORED_UNIQUE_ID;
+
+	/* Ignore errors and continue. */
+	sdw_slave_add(bus, &cwd.id, acpi_fwnode_handle(adev));
+	return 0;
+}
+
 /*
  * sdw_acpi_find_slaves() - Find Slave devices in Master ACPI node
  * @bus: SDW bus instance
@@ -135,8 +200,7 @@ static bool find_slave(struct sdw_bus *b
  */
 int sdw_acpi_find_slaves(struct sdw_bus *bus)
 {
-	struct acpi_device *adev, *parent;
-	struct acpi_device *adev2, *parent2;
+	struct acpi_device *parent;
 
 	parent = ACPI_COMPANION(bus->dev);
 	if (!parent) {
@@ -144,54 +208,7 @@ int sdw_acpi_find_slaves(struct sdw_bus
 		return -ENODEV;
 	}
 
-	list_for_each_entry(adev, &parent->children, node) {
-		struct sdw_slave_id id;
-		struct sdw_slave_id id2;
-		bool ignore_unique_id = true;
-
-		if (!find_slave(bus, adev, &id))
-			continue;
-
-		/* brute-force O(N^2) search for duplicates */
-		parent2 = parent;
-		list_for_each_entry(adev2, &parent2->children, node) {
-
-			if (adev == adev2)
-				continue;
-
-			if (!find_slave(bus, adev2, &id2))
-				continue;
-
-			if (id.sdw_version != id2.sdw_version ||
-			    id.mfg_id != id2.mfg_id ||
-			    id.part_id != id2.part_id ||
-			    id.class_id != id2.class_id)
-				continue;
-
-			if (id.unique_id != id2.unique_id) {
-				dev_dbg(bus->dev,
-					"Valid unique IDs 0x%x 0x%x for Slave mfg_id 0x%04x, part_id 0x%04x\n",
-					id.unique_id, id2.unique_id, id.mfg_id, id.part_id);
-				ignore_unique_id = false;
-			} else {
-				dev_err(bus->dev,
-					"Invalid unique IDs 0x%x 0x%x for Slave mfg_id 0x%04x, part_id 0x%04x\n",
-					id.unique_id, id2.unique_id, id.mfg_id, id.part_id);
-				return -ENODEV;
-			}
-		}
-
-		if (ignore_unique_id)
-			id.unique_id = SDW_IGNORED_UNIQUE_ID;
-
-		/*
-		 * don't error check for sdw_slave_add as we want to continue
-		 * adding Slaves
-		 */
-		sdw_slave_add(bus, &id, acpi_fwnode_handle(adev));
-	}
-
-	return 0;
+	return acpi_dev_for_each_child(parent, sdw_acpi_find_one, bus);
 }
 
 #endif

Re: [PATCH v2 14/16] soundwire: Use acpi_dev_for_each_child()

[Vinod Koul]

On 13-06-22, 20:35, 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).

Applied, thanks

-- 
~Vinod

Re: [PATCH v2 14/16] soundwire: Use acpi_dev_for_each_child()

[Rafael J. Wysocki]

On Thu, Jun 23, 2022 at 10:10 AM Vinod Koul <vkoul@kernel.org> wrote:
>
> On 13-06-22, 20:35, 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).
>
> Applied, thanks

Thanks, but the export of acpi_dev_for_each_child() is being added by
one of the previous patches in the series, so this one will not
compile without the rest of the series in the modular case.

Is this not a problem?

Re: [PATCH v2 14/16] soundwire: Use acpi_dev_for_each_child()

[Vinod Koul]

On 23-06-22, 14:29, Rafael J. Wysocki wrote:
> On Thu, Jun 23, 2022 at 10:10 AM Vinod Koul <vkoul@kernel.org> wrote:
> >
> > On 13-06-22, 20:35, 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).
> >
> > Applied, thanks
> 
> Thanks, but the export of acpi_dev_for_each_child() is being added by
> one of the previous patches in the series, so this one will not
> compile without the rest of the series in the modular case.

Aha, I checked the symbol exists and my test build passed!
> 
> Is this not a problem?

Yes indeed, so can you give a tag for that and or would you like to taje
this thru ACPI tree, in that case

Acked-By: Vinod Koul <vkoul@kernel.org>

BR
-- 
~Vinod

Re: [PATCH v2 14/16] soundwire: Use acpi_dev_for_each_child()

[Rafael J. Wysocki]

On Thu, Jun 23, 2022 at 2:41 PM Vinod Koul <vkoul@kernel.org> wrote:
>
> On 23-06-22, 14:29, Rafael J. Wysocki wrote:
> > On Thu, Jun 23, 2022 at 10:10 AM Vinod Koul <vkoul@kernel.org> wrote:
> > >
> > > On 13-06-22, 20:35, 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).
> > >
> > > Applied, thanks
> >
> > Thanks, but the export of acpi_dev_for_each_child() is being added by
> > one of the previous patches in the series, so this one will not
> > compile without the rest of the series in the modular case.
>
> Aha, I checked the symbol exists and my test build passed!
> >
> > Is this not a problem?
>
> Yes indeed, so can you give a tag for that and or would you like to taje
> this thru ACPI tree, in that case

I'll take it.

> Acked-By: Vinod Koul <vkoul@kernel.org>

Thank you!

[PATCH v2 15/16] ACPI / MMC: PM: Unify fixing up device power

[Rafael J. Wysocki]

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

Introduce acpi_device_fix_up_power_extended() for fixing up power of
a device having an ACPI companion in a manner that takes the device's
children into account and make the MMC code use it in two places
instead of walking the list of the device ACPI companion's children
directly.

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: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
Acked-by: Adrian Hunter <adrian.hunter@intel.com>
Acked-by: Ulf Hansson <ulf.hansson@linaro.org>
---

v1 -> v2:
   * Add R-by from Andy and ACKs from Adrian and Ulf.

---
 drivers/acpi/device_pm.c          |   22 ++++++++++++++++++++++
 drivers/mmc/host/sdhci-acpi.c     |    7 ++-----
 drivers/mmc/host/sdhci-pci-core.c |   11 +++--------
 include/acpi/acpi_bus.h           |    1 +
 4 files changed, 28 insertions(+), 13 deletions(-)

Index: linux-pm/drivers/mmc/host/sdhci-acpi.c
===================================================================
--- linux-pm.orig/drivers/mmc/host/sdhci-acpi.c
+++ linux-pm/drivers/mmc/host/sdhci-acpi.c
@@ -775,8 +775,8 @@ static int sdhci_acpi_probe(struct platf
 {
 	struct device *dev = &pdev->dev;
 	const struct sdhci_acpi_slot *slot;
-	struct acpi_device *device, *child;
 	const struct dmi_system_id *id;
+	struct acpi_device *device;
 	struct sdhci_acpi_host *c;
 	struct sdhci_host *host;
 	struct resource *iomem;
@@ -796,10 +796,7 @@ static int sdhci_acpi_probe(struct platf
 	slot = sdhci_acpi_get_slot(device);
 
 	/* Power on the SDHCI controller and its children */
-	acpi_device_fix_up_power(device);
-	list_for_each_entry(child, &device->children, node)
-		if (child->status.present && child->status.enabled)
-			acpi_device_fix_up_power(child);
+	acpi_device_fix_up_power_extended(device);
 
 	if (sdhci_acpi_byt_defer(dev))
 		return -EPROBE_DEFER;
Index: linux-pm/drivers/acpi/device_pm.c
===================================================================
--- linux-pm.orig/drivers/acpi/device_pm.c
+++ linux-pm/drivers/acpi/device_pm.c
@@ -369,6 +369,28 @@ int acpi_device_fix_up_power(struct acpi
 }
 EXPORT_SYMBOL_GPL(acpi_device_fix_up_power);
 
+static int fix_up_power_if_applicable(struct acpi_device *adev, void *not_used)
+{
+	if (adev->status.present && adev->status.enabled)
+		acpi_device_fix_up_power(adev);
+
+	return 0;
+}
+
+/**
+ * acpi_device_fix_up_power_extended - Force device and its children into D0.
+ * @adev: Parent device object whose power state is to be fixed up.
+ *
+ * Call acpi_device_fix_up_power() for @adev and its children so long as they
+ * are reported as present and enabled.
+ */
+void acpi_device_fix_up_power_extended(struct acpi_device *adev)
+{
+	acpi_device_fix_up_power(adev);
+	acpi_dev_for_each_child(adev, fix_up_power_if_applicable, NULL);
+}
+EXPORT_SYMBOL_GPL(acpi_device_fix_up_power_extended);
+
 int acpi_device_update_power(struct acpi_device *device, int *state_p)
 {
 	int state;
Index: linux-pm/include/acpi/acpi_bus.h
===================================================================
--- linux-pm.orig/include/acpi/acpi_bus.h
+++ linux-pm/include/acpi/acpi_bus.h
@@ -524,6 +524,7 @@ const char *acpi_power_state_string(int
 int acpi_device_set_power(struct acpi_device *device, int state);
 int acpi_bus_init_power(struct acpi_device *device);
 int acpi_device_fix_up_power(struct acpi_device *device);
+void acpi_device_fix_up_power_extended(struct acpi_device *adev);
 int acpi_bus_update_power(acpi_handle handle, int *state_p);
 int acpi_device_update_power(struct acpi_device *device, int *state_p);
 bool acpi_bus_power_manageable(acpi_handle handle);
Index: linux-pm/drivers/mmc/host/sdhci-pci-core.c
===================================================================
--- linux-pm.orig/drivers/mmc/host/sdhci-pci-core.c
+++ linux-pm/drivers/mmc/host/sdhci-pci-core.c
@@ -1240,16 +1240,11 @@ static const struct sdhci_pci_fixes sdhc
 #ifdef CONFIG_ACPI
 static void intel_mrfld_mmc_fix_up_power_slot(struct sdhci_pci_slot *slot)
 {
-	struct acpi_device *device, *child;
+	struct acpi_device *device;
 
 	device = ACPI_COMPANION(&slot->chip->pdev->dev);
-	if (!device)
-		return;
-
-	acpi_device_fix_up_power(device);
-	list_for_each_entry(child, &device->children, node)
-		if (child->status.present && child->status.enabled)
-			acpi_device_fix_up_power(child);
+	if (device)
+		acpi_device_fix_up_power_extended(device);
 }
 #else
 static inline void intel_mrfld_mmc_fix_up_power_slot(struct sdhci_pci_slot *slot) {}

[PATCH v2 16/16] ACPI: bus: Drop unused list heads from struct acpi_device

[Rafael J. Wysocki]

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

Drop the children and node list heads that have no more users
from struct acpi_device and the code manipulating them from
__acpi_device_add() and acpi_device_del().

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/scan.c     |   11 +----------
 include/acpi/acpi_bus.h |    2 --
 2 files changed, 1 insertion(+), 12 deletions(-)

Index: linux-pm/include/acpi/acpi_bus.h
===================================================================
--- linux-pm.orig/include/acpi/acpi_bus.h
+++ linux-pm/include/acpi/acpi_bus.h
@@ -365,8 +365,6 @@ struct acpi_device {
 	acpi_handle handle;		/* no handle for fixed hardware */
 	struct fwnode_handle fwnode;
 	struct acpi_device *parent;
-	struct list_head children;
-	struct list_head node;
 	struct list_head wakeup_list;
 	struct list_head del_list;
 	struct acpi_device_status status;
Index: linux-pm/drivers/acpi/scan.c
===================================================================
--- linux-pm.orig/drivers/acpi/scan.c
+++ linux-pm/drivers/acpi/scan.c
@@ -465,8 +465,6 @@ static void acpi_device_del(struct acpi_
 	struct acpi_device_bus_id *acpi_device_bus_id;
 
 	mutex_lock(&acpi_device_lock);
-	if (device->parent)
-		list_del(&device->node);
 
 	list_for_each_entry(acpi_device_bus_id, &acpi_bus_id_list, node)
 		if (!strcmp(acpi_device_bus_id->bus_id,
@@ -482,6 +480,7 @@ static void acpi_device_del(struct acpi_
 		}
 
 	list_del(&device->wakeup_list);
+
 	mutex_unlock(&acpi_device_lock);
 
 	acpi_power_add_remove_device(device, false);
@@ -674,8 +673,6 @@ static int __acpi_device_add(struct acpi
 	 * -------
 	 * Link this device to its parent and siblings.
 	 */
-	INIT_LIST_HEAD(&device->children);
-	INIT_LIST_HEAD(&device->node);
 	INIT_LIST_HEAD(&device->wakeup_list);
 	INIT_LIST_HEAD(&device->physical_node_list);
 	INIT_LIST_HEAD(&device->del_list);
@@ -715,9 +712,6 @@ static int __acpi_device_add(struct acpi
 		list_add_tail(&acpi_device_bus_id->node, &acpi_bus_id_list);
 	}
 
-	if (device->parent)
-		list_add_tail(&device->node, &device->parent->children);
-
 	if (device->wakeup.flags.valid)
 		list_add_tail(&device->wakeup_list, &acpi_wakeup_device_list);
 
@@ -746,9 +740,6 @@ static int __acpi_device_add(struct acpi
 err:
 	mutex_lock(&acpi_device_lock);
 
-	if (device->parent)
-		list_del(&device->node);
-
 	list_del(&device->wakeup_list);
 
 err_unlock:

[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