| Message ID | 20210118003428.568892-3-djrscally@gmail.com |
|---|---|
| State | New |
| Headers | show |
| Series | Introduce intel_skl_int3472 driver | expand |
Hi Daniel, Thank you for the patch. On Mon, Jan 18, 2021 at 12:34:23AM +0000, 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. > > Signed-off-by: Daniel Scally <djrscally@gmail.com> > --- > Changes in v2: > - Used acpi_lpss_dep() as Andy suggested. > > drivers/acpi/utils.c | 34 ++++++++++++++++++++++++++++++++++ > include/acpi/acpi_bus.h | 2 ++ > 2 files changed, 36 insertions(+) > > diff --git a/drivers/acpi/utils.c b/drivers/acpi/utils.c > index 78b38775f18b..ec6a2406a886 100644 > --- a/drivers/acpi/utils.c > +++ b/drivers/acpi/utils.c > @@ -831,6 +831,18 @@ bool acpi_lpss_dep(struct acpi_device *adev, acpi_handle handle) > return false; > } > > +static int acpi_dev_match_by_dep(struct device *dev, const void *data) > +{ > + struct acpi_device *adev = to_acpi_device(dev); > + const struct acpi_device *dependee = data; > + acpi_handle handle = dependee->handle; > + > + if (acpi_lpss_dep(adev, handle)) > + return 1; > + > + return 0; > +} > + I think I'd move this just before acpi_dev_get_next_dep_dev() to keep the two together. > /** > * acpi_dev_present - Detect that a given ACPI device is present > * @hid: Hardware ID of the device. > @@ -866,6 +878,28 @@ bool acpi_dev_present(const char *hid, const char *uid, s64 hrv) > } > EXPORT_SYMBOL(acpi_dev_present); > > +/** > + * acpi_dev_get_next_dep_dev - Return next ACPI device dependent on input dev Maybe acpi_dev_get_next_dependent_dev() ? "dep" could mean either dependent or dependency. > + * @adev: Pointer to the dependee device > + * @prev: Pointer to the previous dependent device (or NULL for first match) > + * > + * Return the next ACPI device which declares itself dependent on @adev in > + * the _DEP buffer. > + * > + * The caller is responsible to call put_device() on the returned device. > + */ > +struct acpi_device *acpi_dev_get_next_dep_dev(struct acpi_device *adev, > + struct acpi_device *prev) > +{ > + struct device *start = prev ? &prev->dev : NULL; > + struct device *dev; > + > + dev = bus_find_device(&acpi_bus_type, start, adev, acpi_dev_match_by_dep); Having to loop over all ACPI devices is quite inefficient, but if we need a reverse lookup, we don't really have a choice. We could create a reverse map, but I don't think it's worth it. > + > + return dev ? to_acpi_device(dev) : NULL; > +} > +EXPORT_SYMBOL(acpi_dev_get_next_dep_dev); I would have used EXPORT_SYMBOL_GPL. I'm not sure what the policy is in the ACPI subsystem, and it's also a personal choice. Reviewed-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com> > + > /** > * acpi_dev_get_next_match_dev - Return the next match of ACPI device > * @adev: Pointer to the previous acpi_device matching this @hid, @uid and @hrv > diff --git a/include/acpi/acpi_bus.h b/include/acpi/acpi_bus.h > index 02a716a0af5d..33deb22294f2 100644 > --- a/include/acpi/acpi_bus.h > +++ b/include/acpi/acpi_bus.h > @@ -683,6 +683,8 @@ static inline bool acpi_device_can_poweroff(struct acpi_device *adev) > > bool acpi_dev_hid_uid_match(struct acpi_device *adev, const char *hid2, const char *uid2); > > +struct acpi_device * > +acpi_dev_get_next_dep_dev(struct acpi_device *adev, struct acpi_device *prev); > struct acpi_device * > acpi_dev_get_next_match_dev(struct acpi_device *adev, const char *hid, const char *uid, s64 hrv); > struct acpi_device *
Morning Laurent On 18/01/2021 07:34, Laurent Pinchart wrote: > Hi Daniel, > > Thank you for the patch. > > On Mon, Jan 18, 2021 at 12:34:23AM +0000, 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. >> >> Signed-off-by: Daniel Scally <djrscally@gmail.com> >> --- >> Changes in v2: >> - Used acpi_lpss_dep() as Andy suggested. >> >> drivers/acpi/utils.c | 34 ++++++++++++++++++++++++++++++++++ >> include/acpi/acpi_bus.h | 2 ++ >> 2 files changed, 36 insertions(+) >> >> diff --git a/drivers/acpi/utils.c b/drivers/acpi/utils.c >> index 78b38775f18b..ec6a2406a886 100644 >> --- a/drivers/acpi/utils.c >> +++ b/drivers/acpi/utils.c >> @@ -831,6 +831,18 @@ bool acpi_lpss_dep(struct acpi_device *adev, acpi_handle handle) >> return false; >> } >> >> +static int acpi_dev_match_by_dep(struct device *dev, const void *data) >> +{ >> + struct acpi_device *adev = to_acpi_device(dev); >> + const struct acpi_device *dependee = data; >> + acpi_handle handle = dependee->handle; >> + >> + if (acpi_lpss_dep(adev, handle)) >> + return 1; >> + >> + return 0; >> +} >> + > I think I'd move this just before acpi_dev_get_next_dep_dev() to keep > the two together. Will do > >> /** >> * acpi_dev_present - Detect that a given ACPI device is present >> * @hid: Hardware ID of the device. >> @@ -866,6 +878,28 @@ bool acpi_dev_present(const char *hid, const char *uid, s64 hrv) >> } >> EXPORT_SYMBOL(acpi_dev_present); >> >> +/** >> + * acpi_dev_get_next_dep_dev - Return next ACPI device dependent on input dev > Maybe acpi_dev_get_next_dependent_dev() ? "dep" could mean either > dependent or dependency. Yes, good point, I agree. > >> + * @adev: Pointer to the dependee device >> + * @prev: Pointer to the previous dependent device (or NULL for first match) >> + * >> + * Return the next ACPI device which declares itself dependent on @adev in >> + * the _DEP buffer. >> + * >> + * The caller is responsible to call put_device() on the returned device. >> + */ >> +struct acpi_device *acpi_dev_get_next_dep_dev(struct acpi_device *adev, >> + struct acpi_device *prev) >> +{ >> + struct device *start = prev ? &prev->dev : NULL; >> + struct device *dev; >> + >> + dev = bus_find_device(&acpi_bus_type, start, adev, acpi_dev_match_by_dep); > Having to loop over all ACPI devices is quite inefficient, but if we > need a reverse lookup, we don't really have a choice. We could create a > reverse map, but I don't think it's worth it. > >> + >> + return dev ? to_acpi_device(dev) : NULL; >> +} >> +EXPORT_SYMBOL(acpi_dev_get_next_dep_dev); > I would have used EXPORT_SYMBOL_GPL. I'm not sure what the policy is in > the ACPI subsystem, and it's also a personal choice. EXPORT_SYMBOL_GPL would be my usual choice, but the other functions in the file all use EXPORT_SYMBOL, so I assumed there was some policy that that be used (since basically everywhere else I've touched in the kernel so far defaults to the GPL version) > > Reviewed-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com> Thanks! > >> + >> /** >> * acpi_dev_get_next_match_dev - Return the next match of ACPI device >> * @adev: Pointer to the previous acpi_device matching this @hid, @uid and @hrv >> diff --git a/include/acpi/acpi_bus.h b/include/acpi/acpi_bus.h >> index 02a716a0af5d..33deb22294f2 100644 >> --- a/include/acpi/acpi_bus.h >> +++ b/include/acpi/acpi_bus.h >> @@ -683,6 +683,8 @@ static inline bool acpi_device_can_poweroff(struct acpi_device *adev) >> >> bool acpi_dev_hid_uid_match(struct acpi_device *adev, const char *hid2, const char *uid2); >> >> +struct acpi_device * >> +acpi_dev_get_next_dep_dev(struct acpi_device *adev, struct acpi_device *prev); >> struct acpi_device * >> acpi_dev_get_next_match_dev(struct acpi_device *adev, const char *hid, const char *uid, s64 hrv); >> struct acpi_device *
On Mon, Jan 18, 2021 at 12:34:23AM +0000, 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. Fix prefix to be "ACPI / utils: " and rebase on top of function name changes as suggested by Laurent. ... > +/** > + * acpi_dev_get_next_dep_dev - Return next ACPI device dependent on input dev Are you expecting some kind of for_each_*() macro to be added and used? Otherwise there is probably no need to have it "_next_" in the name as it will be a bit confusing. > + * @adev: Pointer to the dependee device > + * @prev: Pointer to the previous dependent device (or NULL for first match) > + * > + * Return the next ACPI device which declares itself dependent on @adev in > + * the _DEP buffer. > + * > + * The caller is responsible to call put_device() on the returned device. > + */
On 18/01/2021 12:33, Andy Shevchenko wrote: > On Mon, Jan 18, 2021 at 12:34:23AM +0000, 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. > Fix prefix to be "ACPI / utils: " and rebase on top of function name changes as > suggested by Laurent. OK. >> +/** >> + * acpi_dev_get_next_dep_dev - Return next ACPI device dependent on input dev > Are you expecting some kind of for_each_*() macro to be added and used? > Otherwise there is probably no need to have it "_next_" in the name as it will > be a bit confusing. I thought that somebody might want to do that in the future yes, although I've no need for it at the minute because each of the dummy INT3472 devices only has one dependent sensor that we've seen so far. But for the INT3472 devices representing a physical TPS68470, there are platforms where 2 sensors are called out as dependent on the same PMIC ACPI device (Surface Go2 for example). It can be renamed and just cross that bridge when we come to it though, I have no problem with that. > >> + * @adev: Pointer to the dependee device >> + * @prev: Pointer to the previous dependent device (or NULL for first match) >> + * >> + * Return the next ACPI device which declares itself dependent on @adev in >> + * the _DEP buffer. >> + * >> + * The caller is responsible to call put_device() on the returned device. >> + */
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? 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? > Signed-off-by: Daniel Scally <djrscally@gmail.com> > --- > Changes in v2: > - Used acpi_lpss_dep() as Andy suggested. > > drivers/acpi/utils.c | 34 ++++++++++++++++++++++++++++++++++ > include/acpi/acpi_bus.h | 2 ++ > 2 files changed, 36 insertions(+) > > diff --git a/drivers/acpi/utils.c b/drivers/acpi/utils.c > index 78b38775f18b..ec6a2406a886 100644 > --- a/drivers/acpi/utils.c > +++ b/drivers/acpi/utils.c > @@ -831,6 +831,18 @@ bool acpi_lpss_dep(struct acpi_device *adev, acpi_handle handle) > return false; > } > > +static int acpi_dev_match_by_dep(struct device *dev, const void *data) > +{ > + struct acpi_device *adev = to_acpi_device(dev); > + const struct acpi_device *dependee = data; > + acpi_handle handle = dependee->handle; > + > + if (acpi_lpss_dep(adev, handle)) > + return 1; > + > + return 0; > +} > + > /** > * acpi_dev_present - Detect that a given ACPI device is present > * @hid: Hardware ID of the device. > @@ -866,6 +878,28 @@ bool acpi_dev_present(const char *hid, const char *uid, s64 hrv) > } > EXPORT_SYMBOL(acpi_dev_present); > > +/** > + * acpi_dev_get_next_dep_dev - Return next ACPI device dependent on input dev > + * @adev: Pointer to the dependee device > + * @prev: Pointer to the previous dependent device (or NULL for first match) > + * > + * Return the next ACPI device which declares itself dependent on @adev in > + * the _DEP buffer. > + * > + * The caller is responsible to call put_device() on the returned device. > + */ > +struct acpi_device *acpi_dev_get_next_dep_dev(struct acpi_device *adev, > + struct acpi_device *prev) > +{ > + struct device *start = prev ? &prev->dev : NULL; > + struct device *dev; > + > + dev = bus_find_device(&acpi_bus_type, start, adev, acpi_dev_match_by_dep); > + > + return dev ? to_acpi_device(dev) : NULL; > +} > +EXPORT_SYMBOL(acpi_dev_get_next_dep_dev); > + > /** > * acpi_dev_get_next_match_dev - Return the next match of ACPI device > * @adev: Pointer to the previous acpi_device matching this @hid, @uid and @hrv > diff --git a/include/acpi/acpi_bus.h b/include/acpi/acpi_bus.h > index 02a716a0af5d..33deb22294f2 100644 > --- a/include/acpi/acpi_bus.h > +++ b/include/acpi/acpi_bus.h > @@ -683,6 +683,8 @@ static inline bool acpi_device_can_poweroff(struct acpi_device *adev) > > bool acpi_dev_hid_uid_match(struct acpi_device *adev, const char *hid2, const char *uid2); > > +struct acpi_device * > +acpi_dev_get_next_dep_dev(struct acpi_device *adev, struct acpi_device *prev); > struct acpi_device * > acpi_dev_get_next_match_dev(struct acpi_device *adev, const char *hid, const char *uid, s64 hrv); > struct acpi_device * > -- > 2.25.1 >
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. > 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. > >> Signed-off-by: Daniel Scally <djrscally@gmail.com> >> --- >> Changes in v2: >> - Used acpi_lpss_dep() as Andy suggested. >> >> drivers/acpi/utils.c | 34 ++++++++++++++++++++++++++++++++++ >> include/acpi/acpi_bus.h | 2 ++ >> 2 files changed, 36 insertions(+) >> >> diff --git a/drivers/acpi/utils.c b/drivers/acpi/utils.c >> index 78b38775f18b..ec6a2406a886 100644 >> --- a/drivers/acpi/utils.c >> +++ b/drivers/acpi/utils.c >> @@ -831,6 +831,18 @@ bool acpi_lpss_dep(struct acpi_device *adev, acpi_handle handle) >> return false; >> } >> >> +static int acpi_dev_match_by_dep(struct device *dev, const void *data) >> +{ >> + struct acpi_device *adev = to_acpi_device(dev); >> + const struct acpi_device *dependee = data; >> + acpi_handle handle = dependee->handle; >> + >> + if (acpi_lpss_dep(adev, handle)) >> + return 1; >> + >> + return 0; >> +} >> + >> /** >> * acpi_dev_present - Detect that a given ACPI device is present >> * @hid: Hardware ID of the device. >> @@ -866,6 +878,28 @@ bool acpi_dev_present(const char *hid, const char *uid, s64 hrv) >> } >> EXPORT_SYMBOL(acpi_dev_present); >> >> +/** >> + * acpi_dev_get_next_dep_dev - Return next ACPI device dependent on input dev >> + * @adev: Pointer to the dependee device >> + * @prev: Pointer to the previous dependent device (or NULL for first match) >> + * >> + * Return the next ACPI device which declares itself dependent on @adev in >> + * the _DEP buffer. >> + * >> + * The caller is responsible to call put_device() on the returned device. >> + */ >> +struct acpi_device *acpi_dev_get_next_dep_dev(struct acpi_device *adev, >> + struct acpi_device *prev) >> +{ >> + struct device *start = prev ? &prev->dev : NULL; >> + struct device *dev; >> + >> + dev = bus_find_device(&acpi_bus_type, start, adev, acpi_dev_match_by_dep); >> + >> + return dev ? to_acpi_device(dev) : NULL; >> +} >> +EXPORT_SYMBOL(acpi_dev_get_next_dep_dev); >> + >> /** >> * acpi_dev_get_next_match_dev - Return the next match of ACPI device >> * @adev: Pointer to the previous acpi_device matching this @hid, @uid and @hrv >> diff --git a/include/acpi/acpi_bus.h b/include/acpi/acpi_bus.h >> index 02a716a0af5d..33deb22294f2 100644 >> --- a/include/acpi/acpi_bus.h >> +++ b/include/acpi/acpi_bus.h >> @@ -683,6 +683,8 @@ static inline bool acpi_device_can_poweroff(struct acpi_device *adev) >> >> bool acpi_dev_hid_uid_match(struct acpi_device *adev, const char *hid2, const char *uid2); >> >> +struct acpi_device * >> +acpi_dev_get_next_dep_dev(struct acpi_device *adev, struct acpi_device *prev); >> struct acpi_device * >> acpi_dev_get_next_match_dev(struct acpi_device *adev, const char *hid, const char *uid, s64 hrv); >> struct acpi_device * >> -- >> 2.25.1 >>
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.
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? If there is, I'm not aware of it, I don't see a reference to the sensor in the INT3472 device (named "PMI0", with the corresponding sensor being "CAM0") in DSDT [1] >>> 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. Alright; I haven't looked too closely yet, but I think an iterator over acpi_dep_list exported from the ACPI subsystem would also work in a pretty similar way to the function introduced in this patch does, without much work [1] https://gist.githubusercontent.com/djrscally/e64d112180517352fa3392878b0f4a7d/raw/88b90b3ea4204fd7845257b6666fdade47cc2981/dsdt.dsl
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)?
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?
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; }
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?
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? Or just export helpers to call each of the callbacks (one to fetch the next dependent device, one to decrement the unmet dependencies counter) Otherwise, I'd just need to update the 5 users of that function either to use the new helper or else to also pass the decrement dependencies callback.
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 (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). > Otherwise, I'd just need to update the 5 users of that function either > to use the new helper or else to also pass the decrement dependencies > callback. Or have a wrapper around it passing the decrement dependencies callback for the "typical" users.
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 >> Otherwise, I'd just need to update the 5 users of that function either >> to use the new helper or else to also pass the decrement dependencies >> callback. > Or have a wrapper around it passing the decrement dependencies > callback for the "typical" users. Yeah that's what I mean by helper; I'll do that then; thanks
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?
On Tue, Feb 02, 2021 at 09:58:17AM +0000, Daniel Scally wrote: > On 21/01/2021 21:06, Daniel Scally wrote: > > On 21/01/2021 18:08, Rafael J. Wysocki wrote: ... > > 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. It's easy to solve. See an example in deferred_probe_work_func(). https://elixir.bootlin.com/linux/latest/source/drivers/base/dd.c#L75 > 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?
On Tue, Feb 2, 2021 at 10:58 AM Daniel Scally <djrscally@gmail.com> 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 <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. 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.
diff --git a/drivers/acpi/utils.c b/drivers/acpi/utils.c index 78b38775f18b..ec6a2406a886 100644 --- a/drivers/acpi/utils.c +++ b/drivers/acpi/utils.c @@ -831,6 +831,18 @@ bool acpi_lpss_dep(struct acpi_device *adev, acpi_handle handle) return false; } +static int acpi_dev_match_by_dep(struct device *dev, const void *data) +{ + struct acpi_device *adev = to_acpi_device(dev); + const struct acpi_device *dependee = data; + acpi_handle handle = dependee->handle; + + if (acpi_lpss_dep(adev, handle)) + return 1; + + return 0; +} + /** * acpi_dev_present - Detect that a given ACPI device is present * @hid: Hardware ID of the device. @@ -866,6 +878,28 @@ bool acpi_dev_present(const char *hid, const char *uid, s64 hrv) } EXPORT_SYMBOL(acpi_dev_present); +/** + * acpi_dev_get_next_dep_dev - Return next ACPI device dependent on input dev + * @adev: Pointer to the dependee device + * @prev: Pointer to the previous dependent device (or NULL for first match) + * + * Return the next ACPI device which declares itself dependent on @adev in + * the _DEP buffer. + * + * The caller is responsible to call put_device() on the returned device. + */ +struct acpi_device *acpi_dev_get_next_dep_dev(struct acpi_device *adev, + struct acpi_device *prev) +{ + struct device *start = prev ? &prev->dev : NULL; + struct device *dev; + + dev = bus_find_device(&acpi_bus_type, start, adev, acpi_dev_match_by_dep); + + return dev ? to_acpi_device(dev) : NULL; +} +EXPORT_SYMBOL(acpi_dev_get_next_dep_dev); + /** * acpi_dev_get_next_match_dev - Return the next match of ACPI device * @adev: Pointer to the previous acpi_device matching this @hid, @uid and @hrv diff --git a/include/acpi/acpi_bus.h b/include/acpi/acpi_bus.h index 02a716a0af5d..33deb22294f2 100644 --- a/include/acpi/acpi_bus.h +++ b/include/acpi/acpi_bus.h @@ -683,6 +683,8 @@ static inline bool acpi_device_can_poweroff(struct acpi_device *adev) bool acpi_dev_hid_uid_match(struct acpi_device *adev, const char *hid2, const char *uid2); +struct acpi_device * +acpi_dev_get_next_dep_dev(struct acpi_device *adev, struct acpi_device *prev); struct acpi_device * acpi_dev_get_next_match_dev(struct acpi_device *adev, const char *hid, const char *uid, s64 hrv); struct acpi_device *
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. Signed-off-by: Daniel Scally <djrscally@gmail.com> --- Changes in v2: - Used acpi_lpss_dep() as Andy suggested. drivers/acpi/utils.c | 34 ++++++++++++++++++++++++++++++++++ include/acpi/acpi_bus.h | 2 ++ 2 files changed, 36 insertions(+)