Re: [PATCH v2 2/7] acpi: utils: Add function to fetch dependent acpi_devices

[Daniel Scally <djrscally@xxxxxxxxx>]

Hi Rafael

On 21/01/2021 21:06, Daniel Scally wrote:
> 
> On 21/01/2021 18:08, Rafael J. Wysocki wrote:
>> On Thu, Jan 21, 2021 at 5:34 PM Daniel Scally <djrscally@xxxxxxxxx> wrote:
>>>
>>> On 21/01/2021 14:39, Rafael J. Wysocki wrote:
>>>> On Thu, Jan 21, 2021 at 1:04 PM Daniel Scally <djrscally@xxxxxxxxx> wrote:
>>>>> On 21/01/2021 11:58, Rafael J. Wysocki wrote:
>>>>>> On Thu, Jan 21, 2021 at 10:47 AM Daniel Scally <djrscally@xxxxxxxxx> wrote:
>>>>>>> Hi Rafael
>>>>>>>
>>>>>>> On 19/01/2021 13:15, Rafael J. Wysocki wrote:
>>>>>>>> On Mon, Jan 18, 2021 at 9:51 PM Daniel Scally <djrscally@xxxxxxxxx> wrote:
>>>>>>>>> On 18/01/2021 16:14, Rafael J. Wysocki wrote:
>>>>>>>>>> On Mon, Jan 18, 2021 at 1:37 AM Daniel Scally <djrscally@xxxxxxxxx> wrote:
>>>>>>>>>>> In some ACPI tables we encounter, devices use the _DEP method to assert
>>>>>>>>>>> a dependence on other ACPI devices as opposed to the OpRegions that the
>>>>>>>>>>> specification intends. We need to be able to find those devices "from"
>>>>>>>>>>> the dependee, so add a function to parse all ACPI Devices and check if
>>>>>>>>>>> the include the handle of the dependee device in their _DEP buffer.
>>>>>>>>>> What exactly do you need this for?
>>>>>>>>> So, in our DSDT we have devices with _HID INT3472, plus sensors which
>>>>>>>>> refer to those INT3472's in their _DEP method. The driver binds to the
>>>>>>>>> INT3472 device, we need to find the sensors dependent on them.
>>>>>>>>>
>>>>>>>> Well, this is an interesting concept. :-)
>>>>>>>>
>>>>>>>> Why does _DEP need to be used for that?  Isn't there any other way to
>>>>>>>> look up the dependent sensors?
>>>>>>>>
>>>>>>>>>> Would it be practical to look up the suppliers in acpi_dep_list instead?
>>>>>>>>>>
>>>>>>>>>> Note that supplier drivers may remove entries from there, but does
>>>>>>>>>> that matter for your use case?
>>>>>>>>> Ah - that may work, yes. Thank you, let me test that.
>>>>>>>> Even if that doesn't work right away, but it can be made work, I would
>>>>>>>> very much prefer that to the driver parsing _DEP for every device in
>>>>>>>> the namespace by itself.
>>>>>>> This does work; do you prefer it in scan.c, or in utils.c (in which case
>>>>>>> with acpi_dep_list declared as external var in internal.h)?
>>>>>> Let's put it in scan.c for now, because there is the lock protecting
>>>>>> the list in there too.
>>>>>>
>>>>>> How do you want to implement this?  Something like "walk the list and
>>>>>> run a callback for the matching entries" or do you have something else
>>>>>> in mind?
>>>>> Something like this (though with a mutex_lock()). It could be simplified
>>>>> by dropping the prev stuff, but we have seen INT3472 devices with
>>>>> multiple sensors declaring themselves dependent on the same device
>>>>>
>>>>>
>>>>> struct acpi_device *
>>>>> acpi_dev_get_next_dependent_dev(struct acpi_device *supplier,
>>>>>                 struct acpi_device *prev)
>>>>> {
>>>>>     struct acpi_dep_data *dep;
>>>>>     struct acpi_device *adev;
>>>>>     int ret;
>>>>>
>>>>>     if (!supplier)
>>>>>         return ERR_PTR(-EINVAL);
>>>>>
>>>>>     if (prev) {
>>>>>         /*
>>>>>          * We need to find the previous device in the list, so we know
>>>>>          * where to start iterating from.
>>>>>          */
>>>>>         list_for_each_entry(dep, &acpi_dep_list, node)
>>>>>             if (dep->consumer == prev->handle &&
>>>>>                 dep->supplier == supplier->handle)
>>>>>                 break;
>>>>>
>>>>>         dep = list_next_entry(dep, node);
>>>>>     } else {
>>>>>         dep = list_first_entry(&acpi_dep_list, struct acpi_dep_data,
>>>>>                        node);
>>>>>     }
>>>>>
>>>>>
>>>>>     list_for_each_entry_from(dep, &acpi_dep_list, node) {
>>>>>         if (dep->supplier == supplier->handle) {
>>>>>             ret = acpi_bus_get_device(dep->consumer, &adev);
>>>>>             if (ret)
>>>>>                 return ERR_PTR(ret);
>>>>>
>>>>>             return adev;
>>>>>         }
>>>>>     }
>>>>>
>>>>>     return NULL;
>>>>> }
>>>> That would work I think, but would it be practical to modify
>>>> acpi_walk_dep_device_list() so that it runs a callback for every
>>>> consumer found instead of or in addition to the "delete from the list
>>>> and free the entry" operation?
>>>
>>> I think that this would work fine, if that's the way you want to go.
>>> We'd just need to move everything inside the if (dep->supplier ==
>>> handle) block to a new callback, and for my purposes I think also add a
>>> way to stop parsing the list from the callback (so like have the
>>> callbacks return int and stop parsing on a non-zero return). Do you want
>>> to expose that ability to pass a callback outside of ACPI?
>> Yes.
>>
>>> Or just export helpers to call each of the callbacks (one to fetch the next
>>> dependent device, one to decrement the unmet dependencies counter)
>> If you can run a callback for every matching entry, you don't really
>> need to have a callback to return the next matching entry.  You can do
>> stuff for all of them in one go
> 
> Well it my case it's more to return a pointer to the dep->consumer's
> acpi_device for a matching entry, so my idea was where there's multiple
> dependents you could use this as an iterator...but it could just be
> extended to that if needed later; I don't actually need to do it right now.
> 
> 
>> note that it probably is not a good
>> idea to run the callback under the lock, so the for loop currently in
>> there is not really suitable for that
> 
> No problem;  I'll tweak that then

Slightly walking back my "No problem" here; as I understand this there's
kinda two options:

1. Walk over the (locked) list, when a match is found unlock, run the
callback and re-lock.

The problem with that idea is unless I'm mistaken there's no guarantee
that the .next pointer is still valid then (even using the *_safe()
methods) because either the next or the next + 1 entry could have been
removed whilst the list was unlocked and the callback was being ran, so
this seems a little unsafe.

2. Walk over the (locked) list twice, the first time counting matching
entries and using that to allocate a temporary buffer, then walk again
to store the matching entries into the buffer. Finally, run the callback
for everything in the buffer, free it and return.

Obviously that's a lot less efficient than the current function, which
isn't particularly palatable.

Apologies if I've missed a better option that would work fine; but
failing that do you still want me to go ahead and change
acpi_walk_dep_device_list() to do this (I'd choose #2 of the above), or
fallback to using acpi_dev_get_next_dependent_dev() described above? If
the latter, does acpi_walk_dep_device_list() maybe need re-naming to
make clear it's not a generalised function?