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

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