Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) client-ip=2620:137:e000::1:20;
Message-ID: <63d7f3ed-b5a9-c869-5d25-a33a1d4e63c8@linux.intel.com>
Date:   Thu, 9 Jun 2022 10:22:39 -0500
MIME-Version: 1.0
User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:91.0) Gecko/20100101
 Firefox/91.0 Thunderbird/91.9.1
Subject: Re: [PATCH v1 14/16] soundwire: Use acpi_dev_for_each_child()
Content-Language: en-US
To:     "Rafael J. Wysocki" <rjw@rjwysocki.net>,
        Linux ACPI <linux-acpi@vger.kernel.org>
Cc:     alsa-devel@alsa-project.org, Linux PM <linux-pm@vger.kernel.org>,
        Bard Liao <yung-chuan.liao@linux.intel.com>,
        LKML <linux-kernel@vger.kernel.org>,
        Hans de Goede <hdegoede@redhat.com>,
        Vinod Koul <vkoul@kernel.org>,
        Sakari Ailus <sakari.ailus@linux.intel.com>,
        Sanyog Kale <sanyog.r.kale@intel.com>,
        Andy Shevchenko <andriy.shevchenko@linux.intel.com>,
        Mika Westerberg <mika.westerberg@linux.intel.com>
References: <1843211.tdWV9SEqCh@kreacher> <5296779.Sb9uPGUboI@kreacher>
From:   Pierre-Louis Bossart <pierre-louis.bossart@linux.intel.com>
In-Reply-To: <5296779.Sb9uPGUboI@kreacher>
Content-Type: text/plain; charset=UTF-8
Content-Transfer-Encoding: 7bit
Precedence: bulk

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