Re: [PATCH 1/6] ACPI: scan: Add acpi_dev_get_next_consumer_dev()

[Hans de Goede <hdegoede@redhat.com>]

Hi,

On 2/21/22 10:50, Hans de Goede wrote:
> Hi,
> 
> On 2/16/22 23:52, Daniel Scally wrote:
>> In commit b83e2b306736 ("ACPI: scan: Add function to fetch dependent of ACPI
>> device") we added a means of fetching the first device to declare itself
>> dependent on another ACPI device in the _DEP method. One assumption
>> in that patch was that there would only be a single consuming device,
>> but this has not held.
>>
>> Extend the functionality by adding a new function that fetches the
>> next consumer of a supplier device. We can then simplify the original
>> function by simply calling the new one.
>>
>> Signed-off-by: Daniel Scally <djrscally@gmail.com>
>> ---
>>  drivers/acpi/scan.c     | 47 ++++++++++++++++++++++++++++++++++-------
>>  include/acpi/acpi_bus.h |  2 ++
>>  2 files changed, 41 insertions(+), 8 deletions(-)
>>
>> diff --git a/drivers/acpi/scan.c b/drivers/acpi/scan.c
>> index 4463c2eda61e..b3ed664ee1cb 100644
>> --- a/drivers/acpi/scan.c
>> +++ b/drivers/acpi/scan.c
>> @@ -2256,9 +2256,21 @@ static void acpi_bus_attach(struct acpi_device *device, bool first_pass)
>>  		device->handler->hotplug.notify_online(device);
>>  }
>>  
>> -static int acpi_dev_get_first_consumer_dev_cb(struct acpi_dep_data *dep, void *data)
>> +static int acpi_dev_get_next_consumer_dev_cb(struct acpi_dep_data *dep, void *data)
>>  {
>> -	struct acpi_device *adev;
>> +	struct acpi_device *adev = *(struct acpi_device **)data;
>> +
>> +	/*
>> +	 * If we're passed a 'previous' consumer device then we need to skip
>> +	 * any consumers until we meet the previous one, and then NULL @data
>> +	 * so the next one can be returned.
>> +	 */
>> +	if (adev) {
>> +		if (dep->consumer == adev->handle)
>> +			*(struct acpi_device **)data = NULL;
>> +
>> +		return 0;
>> +	}
>>  
>>  	adev = acpi_bus_get_acpi_device(dep->consumer);
>>  	if (adev) {
>> @@ -2389,23 +2401,42 @@ bool acpi_dev_ready_for_enumeration(const struct acpi_device *device)
>>  EXPORT_SYMBOL_GPL(acpi_dev_ready_for_enumeration);
>>  
>>  /**
>> - * acpi_dev_get_first_consumer_dev - Return ACPI device dependent on @supplier
>> + * acpi_dev_get_next_consumer_dev - Return the next adev dependent on @supplier
>>   * @supplier: Pointer to the dependee device
>> + * @start: Pointer to the current dependent device
>>   *
>> - * Returns the first &struct acpi_device which declares itself dependent on
>> + * Returns the next &struct acpi_device which declares itself dependent on
>>   * @supplier via the _DEP buffer, parsed from the acpi_dep_list.
>>   *
>>   * The caller is responsible for putting the reference to adev when it is no
>>   * longer needed.
> 
> This bit of the help text seems to not be entirely correct, since the reference to
> start gets consumed by this, so the caller only needs to put the device when it
> does NOT pass it as start to another acpi_dev_get_next_consumer_dev call.
> 
> 
> 
>>   */
>> -struct acpi_device *acpi_dev_get_first_consumer_dev(struct acpi_device *supplier)
>> +struct acpi_device *acpi_dev_get_next_consumer_dev(struct acpi_device *supplier,
>> +						   struct acpi_device *start)
>>  {
>> -	struct acpi_device *adev = NULL;
>> +	struct acpi_device *adev = start;
>>  
>>  	acpi_walk_dep_device_list(supplier->handle,
>> -				  acpi_dev_get_first_consumer_dev_cb, &adev);
>> +				  acpi_dev_get_next_consumer_dev_cb, &adev);
>>  
>> -	return adev;
>> +	acpi_dev_put(start);
>> +	return adev == start ? NULL : adev;
>> +}
>> +EXPORT_SYMBOL_GPL(acpi_dev_get_next_consumer_dev);
>> +
>> +/**
>> + * acpi_dev_get_first_consumer_dev - Return ACPI device dependent on @supplier
>> + * @supplier: Pointer to the dependee device
>> + *
>> + * Returns the first &struct acpi_device which declares itself dependent on
>> + * @supplier via the _DEP buffer, parsed from the acpi_dep_list.
>> + *
>> + * The caller is responsible for putting the reference to adev when it is no
>> + * longer needed.
>> + */
>> +struct acpi_device *acpi_dev_get_first_consumer_dev(struct acpi_device *supplier)
>> +{
>> +	return acpi_dev_get_next_consumer_dev(supplier, NULL);
>>  }
>>  EXPORT_SYMBOL_GPL(acpi_dev_get_first_consumer_dev);
> 
> The only caller of this is skl_int3472_get_sensor_adev_and_name() IMHO it would
> be better to just move that over to acpi_dev_get_next_consumer_dev(..., NULL);
> in this same patch and just drop acpi_dev_get_first_consumer_dev() all together.
> 
> I expect this entire series to get merged through the pdx86 tree, so from
> that pov doing this should be fine..

I forgot to add: that otherwise this looks good to me, so with the above
addressed you may add my:

Reviewed-by: Hans de Goede <hdegoede@redhat.com>

Regards,

Hans


>> diff --git a/include/acpi/acpi_bus.h b/include/acpi/acpi_bus.h
>> index ca88c4706f2b..8b06fef04722 100644
>> --- a/include/acpi/acpi_bus.h
>> +++ b/include/acpi/acpi_bus.h
>> @@ -720,6 +720,8 @@ bool acpi_dev_hid_uid_match(struct acpi_device *adev, const char *hid2, const ch
>>  
>>  void acpi_dev_clear_dependencies(struct acpi_device *supplier);
>>  bool acpi_dev_ready_for_enumeration(const struct acpi_device *device);
>> +struct acpi_device *acpi_dev_get_next_consumer_dev(struct acpi_device *supplier,
>> +						   struct acpi_device *start);
>>  struct acpi_device *acpi_dev_get_first_consumer_dev(struct acpi_device *supplier);
>>  struct acpi_device *
>>  acpi_dev_get_next_match_dev(struct acpi_device *adev, const char *hid, const char *uid, s64 hrv);