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!

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