| Message ID | 20170316161736.339-2-hdegoede@redhat.com (mailing list archive) |
|---|---|
| State | Changes Requested, archived |
| Delegated to: | Rafael Wysocki |
| Headers | show |
On Thursday, March 16, 2017 05:17:35 PM Hans de Goede wrote: > acpi_dev_found just iterates over all acpi-ids and sees if one matches. > This means that it will return true for devices which are in the dsdt > but disabled (their _STA method returns 0). > > For some drivers it is useful to be able to check if a certain hid > is not only present in the namespace, but also actually present as in > acpi_device_is_present() will return true for the device. For example > because if a certain device is present then the driver will want to use > an extcon or IIO adc channel provided by that device. > > This commit adds a new acpi_dev_present helper which drivers can use > to this end. > > Arguably acpi_dev_present is what acpi_dev_found should have been, but > there are too many users to just change acpi_dev_found without the risk > of breaking something. > > Signed-off-by: Hans de Goede <hdegoede@redhat.com> Andy, Mika, any concerns about this patch, please? > --- > drivers/acpi/utils.c | 34 ++++++++++++++++++++++++++++++++++ > include/acpi/acpi_bus.h | 1 + > include/linux/acpi.h | 5 +++++ > 3 files changed, 40 insertions(+) > > diff --git a/drivers/acpi/utils.c b/drivers/acpi/utils.c > index 22c0995..40f8953 100644 > --- a/drivers/acpi/utils.c > +++ b/drivers/acpi/utils.c > @@ -736,6 +736,40 @@ bool acpi_dev_found(const char *hid) > } > EXPORT_SYMBOL(acpi_dev_found); > > +static acpi_status acpi_dev_present_cb(acpi_handle ah, u32 level, void *ctx, > + void **retval) > +{ > + /* > + * acpi_get_devices() does all the work for us, if we get called > + * a device with a matching hid has been found and its _STA indicates > + * it is present. > + */ > + *(bool *)ctx = true; > + return AE_CTRL_TERMINATE; > +} > + > +/** > + * acpi_dev_present - Detect that a given ACPI device is present > + * @hid: Hardware ID of the device. > + * > + * Return %true if the device was present at the moment of invocation. > + * Note that if the device is pluggable, it may since have disappeared. > + * > + * Note that unlike acpi_dev_found() this function checks the status > + * of the device so for devices which are present in the dsdt, but > + * which are disabled (their _STA callback returns 0) this function > + * will return false. > + */ > +bool acpi_dev_present(const char *hid) > +{ > + bool present = false; > + > + acpi_get_devices(hid, acpi_dev_present_cb, &present, NULL); > + > + return present; > +} > +EXPORT_SYMBOL(acpi_dev_present); > + > /* > * acpi_backlight= handling, this is done here rather then in video_detect.c > * because __setup cannot be used in modules. > diff --git a/include/acpi/acpi_bus.h b/include/acpi/acpi_bus.h > index ef0ae8a..29f6b5f 100644 > --- a/include/acpi/acpi_bus.h > +++ b/include/acpi/acpi_bus.h > @@ -88,6 +88,7 @@ acpi_evaluate_dsm_typed(acpi_handle handle, const u8 *uuid, u64 rev, u64 func, > } > > bool acpi_dev_found(const char *hid); > +bool acpi_dev_present(const char *hid); > > #ifdef CONFIG_ACPI > > diff --git a/include/linux/acpi.h b/include/linux/acpi.h > index 673acda..059ffdc 100644 > --- a/include/linux/acpi.h > +++ b/include/linux/acpi.h > @@ -614,6 +614,11 @@ static inline bool acpi_dev_found(const char *hid) > return false; > } > > +static inline bool acpi_dev_present(const char *hid) > +{ > + return false; > +} > + > static inline bool is_acpi_node(struct fwnode_handle *fwnode) > { > return false; >
On Tue, Mar 28, 2017 at 11:42:29PM +0200, Rafael J. Wysocki wrote: > On Thursday, March 16, 2017 05:17:35 PM Hans de Goede wrote: > > acpi_dev_found just iterates over all acpi-ids and sees if one matches. > > This means that it will return true for devices which are in the dsdt > > but disabled (their _STA method returns 0). > > > > For some drivers it is useful to be able to check if a certain hid > > is not only present in the namespace, but also actually present as in > > acpi_device_is_present() will return true for the device. For example > > because if a certain device is present then the driver will want to use > > an extcon or IIO adc channel provided by that device. > > > > This commit adds a new acpi_dev_present helper which drivers can use > > to this end. > > > > Arguably acpi_dev_present is what acpi_dev_found should have been, but > > there are too many users to just change acpi_dev_found without the risk > > of breaking something. > > > > Signed-off-by: Hans de Goede <hdegoede@redhat.com> > > Andy, Mika, any concerns about this patch, please? No concerns from my side. Reviewed-by: Mika Westerberg <mika.westerberg@linux.intel.com>
On Wed, 2017-03-29 at 12:26 +0300, Mika Westerberg wrote: > On Tue, Mar 28, 2017 at 11:42:29PM +0200, Rafael J. Wysocki wrote: > > On Thursday, March 16, 2017 05:17:35 PM Hans de Goede wrote: > > > acpi_dev_found just iterates over all acpi-ids and sees if one > > > matches. > > > This means that it will return true for devices which are in the > > > dsdt > > > but disabled (their _STA method returns 0). > > > > > > For some drivers it is useful to be able to check if a certain hid > > > is not only present in the namespace, but also actually present as > > > in > > > acpi_device_is_present() will return true for the device. For > > > example > > > because if a certain device is present then the driver will want > > > to use > > > an extcon or IIO adc channel provided by that device. > > > > > > This commit adds a new acpi_dev_present helper which drivers can > > > use > > > to this end. > > > > > > Arguably acpi_dev_present is what acpi_dev_found should have been, > > > but > > > there are too many users to just change acpi_dev_found without the > > > risk > > > of breaking something. > > > > > > Signed-off-by: Hans de Goede <hdegoede@redhat.com> > > > > Andy, Mika, any concerns about this patch, please? > > No concerns from my side. > > Reviewed-by: Mika Westerberg <mika.westerberg@linux.intel.com> Neither from me. I briefly looked in the code and indeed we have no existing helper as a such, so, if you think this is the best way to go, I would agree.
[cc += Robert Moore] Hi Hans, I'm the author of acpi_dev_found(), please in the future use git blame to determine relevant authors of existing code that should be cc'ed, I noticed your patch only now: On Thursday, March 16, 2017 05:17:35 PM Hans de Goede wrote: > acpi_dev_found just iterates over all acpi-ids and sees if one matches. > This means that it will return true for devices which are in the dsdt > but disabled (their _STA method returns 0). > > For some drivers it is useful to be able to check if a certain hid > is not only present in the namespace, but also actually present as in > acpi_device_is_present() will return true for the device. For example > because if a certain device is present then the driver will want to use > an extcon or IIO adc channel provided by that device. > > This commit adds a new acpi_dev_present helper which drivers can use > to this end. > > Arguably acpi_dev_present is what acpi_dev_found should have been, but > there are too many users to just change acpi_dev_found without the risk > of breaking something. I originally did submit an acpi_dev_present() function which was identical to what you've submitted now: http://www.spinics.net/lists/linux-acpi/msg61865.html However Robert Moore raised an objection that "Traversing the namespace over and over is truly brute force": http://www.spinics.net/lists/linux-acpi/msg61911.html For my use case, which was apple_gmux_present(), just detecting presence of the HID in the namespace was sufficient, I did not have the need to execute _STA. Hence to address Robert Moore's concern I switched to simply traversing the acpi_bus_id_list. Rafael objected to the acpi_dev_present() name as it suggested that _STA is checked even though it wasn't, so I renamed the function to acpi_dev_found() with commit c68ae33e7fb4. The objection raised by Robert Moore applies to your patch as well since it is identical to my original patch. The return value of _STA seems to be cached in the "status" field of struct acpi_device, so you may be able to overcome Robert Moore's objection by calling bus_find_device() with a callback which contains the check in acpi_device_is_present(). See drivers/firmware/efi/dev-path-parser.c for an example (parse_acpi_path() and match_acpi_dev()). This is probably faster than acpi_get_devices() because it just traverses a list instead of walking the namespace and it avoids the call to _STA. (Some devices just return a constant when _STA is called, others may take more time.) FWIW, all existing users of acpi_dev_found(), except for the hisilicon Ethernet driver, originally used acpi_get_devices() and I converted them to acpi_dev_found to deduplicate code. Thus it would be safe to convert those to your new function acpi_dev_present(). I also converted sound/soc/intel/common/sst-match-acpi.c to acpi_dev_found() but the Intel folks switched back to acpi_get_devices() because just like you they had the need to check _STA. If you introduce a new helper which checks _STA, it would be good if you could amend sst-match-acpi.c to use it so as to deduplicate code. Thanks, Lukas
On Thu, Mar 30, 2017 at 10:33 AM, Lukas Wunner <lukas@wunner.de> wrote: > [cc += Robert Moore] > > Hi Hans, > > I'm the author of acpi_dev_found(), please in the future use git blame > to determine relevant authors of existing code that should be cc'ed, > I noticed your patch only now: > > On Thursday, March 16, 2017 05:17:35 PM Hans de Goede wrote: >> acpi_dev_found just iterates over all acpi-ids and sees if one matches. >> This means that it will return true for devices which are in the dsdt >> but disabled (their _STA method returns 0). >> >> For some drivers it is useful to be able to check if a certain hid >> is not only present in the namespace, but also actually present as in >> acpi_device_is_present() will return true for the device. For example >> because if a certain device is present then the driver will want to use >> an extcon or IIO adc channel provided by that device. >> >> This commit adds a new acpi_dev_present helper which drivers can use >> to this end. >> >> Arguably acpi_dev_present is what acpi_dev_found should have been, but >> there are too many users to just change acpi_dev_found without the risk >> of breaking something. > > I originally did submit an acpi_dev_present() function which was identical > to what you've submitted now: > http://www.spinics.net/lists/linux-acpi/msg61865.html > > However Robert Moore raised an objection that "Traversing the namespace > over and over is truly brute force": > http://www.spinics.net/lists/linux-acpi/msg61911.html > > For my use case, which was apple_gmux_present(), just detecting presence > of the HID in the namespace was sufficient, I did not have the need to > execute _STA. Hence to address Robert Moore's concern I switched to > simply traversing the acpi_bus_id_list. > > Rafael objected to the acpi_dev_present() name as it suggested that _STA > is checked even though it wasn't, so I renamed the function to > acpi_dev_found() with commit c68ae33e7fb4. > > The objection raised by Robert Moore applies to your patch as well since > it is identical to my original patch. Good point, actually. > The return value of _STA seems to > be cached in the "status" field of struct acpi_device, so you may > be able to overcome Robert Moore's objection by calling bus_find_device() > with a callback which contains the check in acpi_device_is_present(). > See drivers/firmware/efi/dev-path-parser.c for an example (parse_acpi_path() > and match_acpi_dev()). This is probably faster than acpi_get_devices() > because it just traverses a list instead of walking the namespace and it > avoids the call to _STA. (Some devices just return a constant when _STA > is called, others may take more time.) Yes, that would be preferred. Thanks, Rafael
Hi Lukas, On 30-03-17 10:33, Lukas Wunner wrote: > [cc += Robert Moore] > > Hi Hans, > > I'm the author of acpi_dev_found(), please in the future use git blame > to determine relevant authors of existing code that should be cc'ed, Right, sorry about that. > I noticed your patch only now: > > On Thursday, March 16, 2017 05:17:35 PM Hans de Goede wrote: >> acpi_dev_found just iterates over all acpi-ids and sees if one matches. >> This means that it will return true for devices which are in the dsdt >> but disabled (their _STA method returns 0). >> >> For some drivers it is useful to be able to check if a certain hid >> is not only present in the namespace, but also actually present as in >> acpi_device_is_present() will return true for the device. For example >> because if a certain device is present then the driver will want to use >> an extcon or IIO adc channel provided by that device. >> >> This commit adds a new acpi_dev_present helper which drivers can use >> to this end. >> >> Arguably acpi_dev_present is what acpi_dev_found should have been, but >> there are too many users to just change acpi_dev_found without the risk >> of breaking something. > > I originally did submit an acpi_dev_present() function which was identical > to what you've submitted now: > http://www.spinics.net/lists/linux-acpi/msg61865.html > > However Robert Moore raised an objection that "Traversing the namespace > over and over is truly brute force": > http://www.spinics.net/lists/linux-acpi/msg61911.html > > For my use case, which was apple_gmux_present(), just detecting presence > of the HID in the namespace was sufficient, I did not have the need to > execute _STA. Hence to address Robert Moore's concern I switched to > simply traversing the acpi_bus_id_list. > > Rafael objected to the acpi_dev_present() name as it suggested that _STA > is checked even though it wasn't, so I renamed the function to > acpi_dev_found() with commit c68ae33e7fb4. > > The objection raised by Robert Moore applies to your patch as well since > it is identical to my original patch. The return value of _STA seems to > be cached in the "status" field of struct acpi_device, so you may > be able to overcome Robert Moore's objection by calling bus_find_device() > with a callback which contains the check in acpi_device_is_present(). > See drivers/firmware/efi/dev-path-parser.c for an example (parse_acpi_path() > and match_acpi_dev()). This is probably faster than acpi_get_devices() > because it just traverses a list instead of walking the namespace and it > avoids the call to _STA. (Some devices just return a constant when _STA > is called, others may take more time.) Thank you for your input. I've prepared a new version using your suggestion and slightly extended the function with a uid and hrv argument so that it becomes more generally useful, e.g. it can be used now to replace the code from drivers/firmware/efi/dev-path-parser.c you gave as an example how to do this. I'm going to submit a new version of this patch-set now, including a few patches replacing another series from me which can also use the new extended functionality. If people like the new v2 acpi_dev_present() I will submit patches to replace the custom code for this in drivers/firmware/efi/dev-path-parser.c and sound/soc/intel/common/sst-match-acpi.c once it is merged. Talking about merging, I also have some none ACPI patches which need this pending (such as 2/2 of the orgiinal series) it would be great of at least the first patch of the new series could be merged in time for 4.12, or alternatively an immutable branch with it can be created for 4.13 once 4.12-rc1 is released. Regards, Hans > > FWIW, all existing users of acpi_dev_found(), except for the hisilicon > Ethernet driver, originally used acpi_get_devices() and I converted > them to acpi_dev_found to deduplicate code. Thus it would be safe to > convert those to your new function acpi_dev_present(). I also converted > sound/soc/intel/common/sst-match-acpi.c to acpi_dev_found() but the > Intel folks switched back to acpi_get_devices() because just like you > they had the need to check _STA. If you introduce a new helper which > checks _STA, it would be good if you could amend sst-match-acpi.c to > use it so as to deduplicate code. > > Thanks, > > Lukas >
diff --git a/drivers/acpi/utils.c b/drivers/acpi/utils.c index 22c0995..40f8953 100644 --- a/drivers/acpi/utils.c +++ b/drivers/acpi/utils.c @@ -736,6 +736,40 @@ bool acpi_dev_found(const char *hid) } EXPORT_SYMBOL(acpi_dev_found); +static acpi_status acpi_dev_present_cb(acpi_handle ah, u32 level, void *ctx, + void **retval) +{ + /* + * acpi_get_devices() does all the work for us, if we get called + * a device with a matching hid has been found and its _STA indicates + * it is present. + */ + *(bool *)ctx = true; + return AE_CTRL_TERMINATE; +} + +/** + * acpi_dev_present - Detect that a given ACPI device is present + * @hid: Hardware ID of the device. + * + * Return %true if the device was present at the moment of invocation. + * Note that if the device is pluggable, it may since have disappeared. + * + * Note that unlike acpi_dev_found() this function checks the status + * of the device so for devices which are present in the dsdt, but + * which are disabled (their _STA callback returns 0) this function + * will return false. + */ +bool acpi_dev_present(const char *hid) +{ + bool present = false; + + acpi_get_devices(hid, acpi_dev_present_cb, &present, NULL); + + return present; +} +EXPORT_SYMBOL(acpi_dev_present); + /* * acpi_backlight= handling, this is done here rather then in video_detect.c * because __setup cannot be used in modules. diff --git a/include/acpi/acpi_bus.h b/include/acpi/acpi_bus.h index ef0ae8a..29f6b5f 100644 --- a/include/acpi/acpi_bus.h +++ b/include/acpi/acpi_bus.h @@ -88,6 +88,7 @@ acpi_evaluate_dsm_typed(acpi_handle handle, const u8 *uuid, u64 rev, u64 func, } bool acpi_dev_found(const char *hid); +bool acpi_dev_present(const char *hid); #ifdef CONFIG_ACPI diff --git a/include/linux/acpi.h b/include/linux/acpi.h index 673acda..059ffdc 100644 --- a/include/linux/acpi.h +++ b/include/linux/acpi.h @@ -614,6 +614,11 @@ static inline bool acpi_dev_found(const char *hid) return false; } +static inline bool acpi_dev_present(const char *hid) +{ + return false; +} + static inline bool is_acpi_node(struct fwnode_handle *fwnode) { return false;
acpi_dev_found just iterates over all acpi-ids and sees if one matches. This means that it will return true for devices which are in the dsdt but disabled (their _STA method returns 0). For some drivers it is useful to be able to check if a certain hid is not only present in the namespace, but also actually present as in acpi_device_is_present() will return true for the device. For example because if a certain device is present then the driver will want to use an extcon or IIO adc channel provided by that device. This commit adds a new acpi_dev_present helper which drivers can use to this end. Arguably acpi_dev_present is what acpi_dev_found should have been, but there are too many users to just change acpi_dev_found without the risk of breaking something. Signed-off-by: Hans de Goede <hdegoede@redhat.com> --- drivers/acpi/utils.c | 34 ++++++++++++++++++++++++++++++++++ include/acpi/acpi_bus.h | 1 + include/linux/acpi.h | 5 +++++ 3 files changed, 40 insertions(+)