| Message ID | 9017824.rMLUfLXkoz@kreacher |
|---|---|
| State | New |
| Headers | show |
| Series | None | expand |
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
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?
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
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!
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