lkml.org 
[lkml]   [2022]   [Jun]   [9]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
    Patch in this message
    /
    Date
    SubjectRe: [PATCH v1 14/16] soundwire: Use acpi_dev_for_each_child()
    From
    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;
    > }
    >
    >
    >

    \
     
     \ /
      Last update: 2022-06-09 17:24    [W:4.053 / U:0.040 seconds]
    ©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site