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!