From: Daniel Scally <djrscally@gmail.com>
To: "Rafael J. Wysocki" <rafael@kernel.org>
Cc: Linux Kernel Mailing List <linux-kernel@vger.kernel.org>,
ACPI Devel Maling List <linux-acpi@vger.kernel.org>,
linux-gpio@vger.kernel.org, linux-i2c <linux-i2c@vger.kernel.org>,
Platform Driver <platform-driver-x86@vger.kernel.org>,
"open list:ACPI COMPONENT ARCHITECTURE (ACPICA)"
<devel@acpica.org>, "Rafael J. Wysocki" <rjw@rjwysocki.net>,
Len Brown <lenb@kernel.org>,
andy@kernel.org,
Mika Westerberg <mika.westerberg@linux.intel.com>,
Linus Walleij <linus.walleij@linaro.org>,
Bartosz Golaszewski <bgolaszewski@baylibre.com>,
Wolfram Sang <wsa@kernel.org>, Lee Jones <lee.jones@linaro.org>,
Hans de Goede <hdegoede@redhat.com>,
Mark Gross <mgross@linux.intel.com>,
Robert Moore <robert.moore@intel.com>,
Erik Kaneda <erik.kaneda@intel.com>,
Sakari Ailus <sakari.ailus@linux.intel.com>,
Andy Shevchenko <andriy.shevchenko@linux.intel.com>,
Laurent Pinchart <laurent.pinchart@ideasonboard.com>,
Kieran Bingham <kieran.bingham@ideasonboard.com>
Subject: Re: [PATCH v2 2/7] acpi: utils: Add function to fetch dependent acpi_devices
Date: Tue, 2 Feb 2021 09:58:17 +0000 [thread overview]
Message-ID: <d9ec0439-4323-51a2-70e7-c258fe63cd86@gmail.com> (raw)
In-Reply-To: <ee8f6b58-55c8-e0a0-c161-bdef361f9e0a@gmail.com>
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@gmail.com> wrote:
>>>
>>> On 21/01/2021 14:39, Rafael J. Wysocki wrote:
>>>> On Thu, Jan 21, 2021 at 1:04 PM Daniel Scally <djrscally@gmail.com> wrote:
>>>>> On 21/01/2021 11:58, Rafael J. Wysocki wrote:
>>>>>> On Thu, Jan 21, 2021 at 10:47 AM Daniel Scally <djrscally@gmail.com> 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@gmail.com> wrote:
>>>>>>>>> On 18/01/2021 16:14, Rafael J. Wysocki wrote:
>>>>>>>>>> On Mon, Jan 18, 2021 at 1:37 AM Daniel Scally <djrscally@gmail.com> 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?
next prev parent reply other threads:[~2021-02-02 9:59 UTC|newest]
Thread overview: 98+ messages / expand[flat|nested] mbox.gz Atom feed top
2021-01-18 0:34 [PATCH v2 0/7] Introduce intel_skl_int3472 driver Daniel Scally
2021-01-18 0:34 ` [PATCH v2 1/7] acpi: utils: move acpi_lpss_dep() to utils Daniel Scally
2021-01-18 7:24 ` Laurent Pinchart
2021-01-18 8:31 ` Daniel Scally
2021-01-18 12:29 ` Andy Shevchenko
2021-01-18 12:35 ` Daniel Scally
2021-01-18 12:28 ` Andy Shevchenko
2021-01-18 16:06 ` Rafael J. Wysocki
2021-01-18 16:42 ` Andy Shevchenko
2021-01-18 0:34 ` [PATCH v2 2/7] acpi: utils: Add function to fetch dependent acpi_devices Daniel Scally
2021-01-18 7:34 ` Laurent Pinchart
2021-01-18 8:37 ` Daniel Scally
2021-01-18 12:33 ` Andy Shevchenko
2021-01-18 13:37 ` Daniel Scally
2021-01-18 16:14 ` Rafael J. Wysocki
2021-01-18 20:51 ` Daniel Scally
2021-01-19 13:15 ` Rafael J. Wysocki
2021-01-19 13:28 ` Daniel Scally
2021-01-21 9:47 ` Daniel Scally
2021-01-21 11:58 ` Rafael J. Wysocki
2021-01-21 12:04 ` Daniel Scally
2021-01-21 14:39 ` Rafael J. Wysocki
2021-01-21 16:34 ` Daniel Scally
2021-01-21 18:08 ` Rafael J. Wysocki
2021-01-21 21:06 ` Daniel Scally
2021-02-02 9:58 ` Daniel Scally [this message]
2021-02-02 11:27 ` Andy Shevchenko
2021-02-02 14:02 ` Rafael J. Wysocki
2021-01-18 0:34 ` [PATCH v2 3/7] i2c: i2c-core-base: Use format macro in i2c_dev_set_name() Daniel Scally
2021-01-18 7:28 ` Laurent Pinchart
2021-01-18 12:41 ` Andy Shevchenko
2021-01-18 9:41 ` Sakari Ailus
2021-01-18 9:42 ` Sakari Ailus
2021-01-18 9:48 ` Wolfram Sang
2021-01-18 12:39 ` Andy Shevchenko
2021-01-18 0:34 ` [PATCH v2 4/7] i2c: i2c-core-acpi: Add i2c_acpi_dev_name() Daniel Scally
2021-01-18 9:18 ` Laurent Pinchart
2021-01-18 13:41 ` Andy Shevchenko
2021-01-19 13:19 ` Rafael J. Wysocki
2021-01-28 9:00 ` Wolfram Sang
2021-01-28 9:15 ` Daniel Scally
2021-01-28 9:17 ` Wolfram Sang
2021-01-28 9:22 ` Daniel Scally
2021-01-18 13:39 ` Andy Shevchenko
2021-01-18 18:43 ` Joe Perches
2021-01-18 18:56 ` Andy Shevchenko
2021-01-18 19:00 ` Joe Perches
2021-01-18 19:01 ` Andy Shevchenko
2021-01-18 0:34 ` [PATCH v2 5/7] gpio: gpiolib-acpi: Export acpi_get_gpiod() Daniel Scally
2021-01-18 7:37 ` Laurent Pinchart
2021-01-18 13:45 ` Andy Shevchenko
2021-01-18 13:46 ` Andy Shevchenko
2021-01-18 21:32 ` Daniel Scally
2021-01-18 0:34 ` [PATCH v2 6/7] platform: x86: Add intel_skl_int3472 driver Daniel Scally
2021-01-18 9:15 ` Laurent Pinchart
2021-01-18 14:46 ` Andy Shevchenko
2021-01-18 21:19 ` Daniel Scally
2021-01-19 0:11 ` Daniel Scally
2021-01-19 6:21 ` Laurent Pinchart
2021-01-19 9:35 ` Andy Shevchenko
2021-01-19 16:49 ` Laurent Pinchart
2021-01-19 9:33 ` Andy Shevchenko
2021-01-19 9:34 ` Daniel Scally
2021-01-19 16:36 ` Laurent Pinchart
2021-01-19 17:43 ` Andy Shevchenko
2021-01-20 4:18 ` Laurent Pinchart
2021-01-20 11:44 ` Andy Shevchenko
2021-01-21 21:08 ` Daniel Scally
2021-01-19 9:24 ` Andy Shevchenko
2021-01-19 10:40 ` Daniel Scally
2021-01-19 11:08 ` Andy Shevchenko
2021-01-19 16:48 ` Laurent Pinchart
2021-01-19 17:51 ` Andy Shevchenko
2021-01-20 4:21 ` Laurent Pinchart
2021-01-20 12:57 ` Andy Shevchenko
2021-01-21 0:18 ` Daniel Scally
2021-02-07 11:00 ` Daniel Scally
2021-02-07 11:56 ` Andy Shevchenko
2021-01-18 20:46 ` Daniel Scally
2021-01-19 6:19 ` Laurent Pinchart
2021-01-19 8:43 ` Daniel Scally
2021-01-19 16:33 ` Laurent Pinchart
2021-01-18 11:12 ` Barnabás Pőcze
2021-01-18 13:51 ` andriy.shevchenko
2021-01-18 14:51 ` Barnabás Pőcze
2021-01-18 15:23 ` andriy.shevchenko
2021-01-18 15:32 ` Hans de Goede
2021-01-18 15:48 ` andriy.shevchenko
2021-01-18 16:00 ` Daniel Scally
2021-01-18 16:03 ` Hans de Goede
2021-01-18 17:05 ` Laurent Pinchart
2021-01-19 10:56 ` Kieran Bingham
2021-01-19 11:11 ` Andy Shevchenko
2021-01-19 11:12 ` Daniel Scally
2021-01-18 0:34 ` [PATCH v2 7/7] mfd: Remove tps68470 MFD driver Daniel Scally
2021-01-18 7:42 ` Laurent Pinchart
2021-01-18 13:53 ` Andy Shevchenko
2021-01-18 20:07 ` Joe Perches
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=d9ec0439-4323-51a2-70e7-c258fe63cd86@gmail.com \
--to=djrscally@gmail.com \
--cc=andriy.shevchenko@linux.intel.com \
--cc=andy@kernel.org \
--cc=bgolaszewski@baylibre.com \
--cc=devel@acpica.org \
--cc=erik.kaneda@intel.com \
--cc=hdegoede@redhat.com \
--cc=kieran.bingham@ideasonboard.com \
--cc=laurent.pinchart@ideasonboard.com \
--cc=lee.jones@linaro.org \
--cc=lenb@kernel.org \
--cc=linus.walleij@linaro.org \
--cc=linux-acpi@vger.kernel.org \
--cc=linux-gpio@vger.kernel.org \
--cc=linux-i2c@vger.kernel.org \
--cc=linux-kernel@vger.kernel.org \
--cc=mgross@linux.intel.com \
--cc=mika.westerberg@linux.intel.com \
--cc=platform-driver-x86@vger.kernel.org \
--cc=rafael@kernel.org \
--cc=rjw@rjwysocki.net \
--cc=robert.moore@intel.com \
--cc=sakari.ailus@linux.intel.com \
--cc=wsa@kernel.org \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).