diff mbox series

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

Message ID 20210118003428.568892-3-djrscally@gmail.com (mailing list archive)
State Changes Requested, archived
Headers show
Series Introduce intel_skl_int3472 driver | expand

Commit Message

Daniel Scally Jan. 18, 2021, 12:34 a.m. UTC
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(+)

Comments

Laurent Pinchart Jan. 18, 2021, 7:34 a.m. UTC | #1
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 *
Daniel Scally Jan. 18, 2021, 8:37 a.m. UTC | #2
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 *
Andy Shevchenko Jan. 18, 2021, 12:33 p.m. UTC | #3
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.
> + */
Daniel Scally Jan. 18, 2021, 1:37 p.m. UTC | #4
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.
>> + */
Rafael J. Wysocki Jan. 18, 2021, 4:14 p.m. UTC | #5
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
>
Daniel Scally Jan. 18, 2021, 8:51 p.m. UTC | #6
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
>>
Rafael J. Wysocki Jan. 19, 2021, 1:15 p.m. UTC | #7
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.
Daniel Scally Jan. 19, 2021, 1:28 p.m. UTC | #8
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
Daniel Scally Jan. 21, 2021, 9:47 a.m. UTC | #9
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)?
Rafael J. Wysocki Jan. 21, 2021, 11:58 a.m. UTC | #10
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?
Daniel Scally Jan. 21, 2021, 12:04 p.m. UTC | #11
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;
}
Rafael J. Wysocki Jan. 21, 2021, 2:39 p.m. UTC | #12
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?
Daniel Scally Jan. 21, 2021, 4:34 p.m. UTC | #13
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.
Rafael J. Wysocki Jan. 21, 2021, 6:08 p.m. UTC | #14
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.
Daniel Scally Jan. 21, 2021, 9:06 p.m. UTC | #15
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
Daniel Scally Feb. 2, 2021, 9:58 a.m. UTC | #16
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?
Andy Shevchenko Feb. 2, 2021, 11:27 a.m. UTC | #17
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?
Rafael J. Wysocki Feb. 2, 2021, 2:02 p.m. UTC | #18
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 mbox series

Patch

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 *