On Tue, Feb 2, 2021 at 10:58 AM Daniel Scally wrote: > > 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 wrote: > >>> > >>> On 21/01/2021 14:39, Rafael J. Wysocki wrote: > >>>> On Thu, Jan 21, 2021 at 1:04 PM Daniel Scally wrote: > >>>>> On 21/01/2021 11:58, Rafael J. Wysocki wrote: > >>>>>> On Thu, Jan 21, 2021 at 10:47 AM Daniel Scally wrote: > >>>>>>> Hi Rafael > >>>>>>> > >>>>>>> On 19/01/2021 13:15, Rafael J. Wysocki wrote: > >>>>>>>> On Mon, Jan 18, 2021 at 9:51 PM Daniel Scally wrote: > >>>>>>>>> On 18/01/2021 16:14, Rafael J. Wysocki wrote: > >>>>>>>>>> On Mon, Jan 18, 2021 at 1:37 AM Daniel Scally 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. That's what I was thinking about. > 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. This can be addressed by rotating the list while walking it, but that becomes problematic if there are concurrent walkers. OK, I guess running the callback under the lock is not really a big deal (and for the deletion case this is actually necessary), so let's do that.