| Message ID | 20170408214830.17650-1-hdegoede@redhat.com (mailing list archive) |
|---|---|
| State | Superseded, archived |
| Headers | show |
On Sat, Apr 08, 2017 at 11:48:27PM +0200, 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. > > Like acpi_dev_found, acpi_dev_present take a HID as argument, but > it also has 2 extra optional arguments to only check for an ACPI > device with a specific UID and/or HRV value. This makes it more > generic and allows it to replace custom code doing similar checks > in several places. > > 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. > > Cc: Mika Westerberg <mika.westerberg@linux.intel.com> > Cc: Lukas Wunner <lukas@wunner.de> > Signed-off-by: Hans de Goede <hdegoede@redhat.com> > --- > Changes in v2: > -Switch to using bus_find_device() to avoid "Traversing the namespace > over and over" > -Add optional (may be NULL / -1) uid and hrv arguments, this will > allow this new function to replace the custom code for this in > drivers/firmware/efi/dev-path-parser.c as well as in > sound/soc/intel/common/sst-match-acpi.c and will allow it to be > used to implement blacklists to avoid loading the ACPI ac / battery > driver on systems which have a PMIC / charger acpi device with a > native driver which offers a better (often working vs not working) > user experience > -Dropped Mika's reviewd by as this is almost a total rewrite > Changes in v3: > -memset the entire acpi_dev_present_info struct, this fixes > acpi_device_id.cls not getting cleared > Changes in v4: > -Use empty initializer to zero the acpi_dev_present_info struct > --- > drivers/acpi/utils.c | 71 +++++++++++++++++++++++++++++++++++++++++++++++++ > include/acpi/acpi_bus.h | 1 + > include/linux/acpi.h | 5 ++++ > 3 files changed, 77 insertions(+) > > diff --git a/drivers/acpi/utils.c b/drivers/acpi/utils.c > index 22c0995..ecd86a9 100644 > --- a/drivers/acpi/utils.c > +++ b/drivers/acpi/utils.c > @@ -736,6 +736,77 @@ bool acpi_dev_found(const char *hid) > } > EXPORT_SYMBOL(acpi_dev_found); > > +struct acpi_dev_present_info { > + struct acpi_device_id hid[2]; > + const char *uid; > + int hrv; > +}; There's a (somewhat theoretical) issue on 32 bit arches with your choice of int for hrv: _HRV returns an "Integer (DWORD)" per the spec. That's 32 bit and clashes with your use of -1 to signify "don't care" if the arch uses 32 bit to represent an int. So you need to use a "signed long long" or "s64" here. > + > +static int acpi_dev_present_cb(struct device *dev, void *data) > +{ > + struct acpi_device *adev = to_acpi_device(dev); > + struct acpi_dev_present_info *match = data; > + unsigned long long hrv; > + acpi_status status; > + > + if (acpi_match_device_ids(adev, match->hid)) > + return 0; > + > + if (match->uid && adev->pnp.unique_id && > + strcmp(adev->pnp.unique_id, match->uid)) > + return 0; > + > + if (match->uid && !adev->pnp.unique_id && > + strcmp("0", match->uid)) > + return 0; > + > + if (match->hrv == -1) > + return 1; > + > + status = acpi_evaluate_integer(adev->handle, "_HRV", NULL, &hrv); > + if (ACPI_FAILURE(status)) > + return 0; > + > + return hrv == match->hrv; > +} > + > +/** > + * acpi_dev_present - Detect that a given ACPI device is present > + * @hid: Hardware ID of the device. > + * @uid: Unique ID of the device, pass "0" for devices without a _UID, > + * pass NULL to not check _UID > + * @hrv: Hardware Revision of the device, pass -1 to not check _HRV > + * > + * Return %true if a matching 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 ^ Minor nit: I think readability improves if you start a new sentence or insert a comma here. Sorry, just my grammar OCD. Otherwise, Reviewed-by: Lukas Wunner <lukas@wunner.de> I'm clueless about ACPI battery or charging, so can't say anything about the remainder of the series. Note that the code in drivers/firmware/efi/dev-path-parser.c deliberately increments the refcount of the device found and returns a pointer to it, whereas acpi_dev_present() as introduced by this patch merely checks presence and returns a bool. So you won't be able to replace the code in dev-path-parser.c with acpi_dev_present(), but I think that's perfectly fine. Thanks, Lukas > + * which are disabled (their _STA callback returns 0) this function > + * will return false. > + * > + * For this function to work, acpi_bus_scan() must have been executed > + * which happens in the subsys_initcall() subsection. Hence, do not > + * call from a subsys_initcall() or earlier (use acpi_get_devices() > + * instead). Calling from module_init() is fine (which is synonymous > + * with device_initcall()). > + */ > +bool acpi_dev_present(const char *hid, const char *uid, int hrv) > +{ > + struct acpi_dev_present_info match = {}; > + struct device *dev; > + > + strlcpy(match.hid[0].id, hid, sizeof(match.hid[0].id)); > + match.uid = uid; > + match.hrv = hrv; > + > + dev = bus_find_device(&acpi_bus_type, NULL, &match, > + acpi_dev_present_cb); > + > + return dev ? true : false; > +} > +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..64498d5 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, const char *uid, int hrv); > > #ifdef CONFIG_ACPI > > diff --git a/include/linux/acpi.h b/include/linux/acpi.h > index 9b05886..e5dd0f1 100644 > --- a/include/linux/acpi.h > +++ b/include/linux/acpi.h > @@ -611,6 +611,11 @@ static inline bool acpi_dev_found(const char *hid) > return false; > } > > +static inline bool acpi_dev_present(const char *hid, const char *uid, int hrv) > +{ > + return false; > +} > + > static inline bool is_acpi_node(struct fwnode_handle *fwnode) > { > return false; > -- > 2.9.3
Hi All, Self-nack for patches 3-4 in this series, it turns out that doing the global blacklist for the ACPI ac / battery is not a good idea. Some Bay Trail / Cherry Trail users are running kernels build from my personal tree to get early access to various fixes in there and I got a regression report on the DELL 5855, where the blacklisting of the ACPI battery driver if INT33F4 is present caused the battery monitoring to stop working, that devices has an INT33F4 node with _STA returning 15 yet it is not using an axp288 PMIC at all, I'm still gathering more info, but I believe atm that Dell simply disabled the i2c controller to which the axp288 would be connected if present and left the other bits of the DTSD unmodified. One option which comes to mind would be to only count devices as present if all their deps are met, but that will only work if the blacklist check is done after all other drivers have loaded which is not how things work. So I believe that my earlier attempts at fixing the double power_supply registration by unregistering the ACPI one when the native one has successfully loaded is better. That guarantees regressions like this one will not happen, because the ACPI power_supply does not get unregistered until the native one has loaded. Regards, Hans On Sat, Apr 08, 2017 at 11:48:27PM +0200, 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. > > Like acpi_dev_found, acpi_dev_present take a HID as argument, but > it also has 2 extra optional arguments to only check for an ACPI > device with a specific UID and/or HRV value. This makes it more > generic and allows it to replace custom code doing similar checks > in several places. > > 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. > > Cc: Mika Westerberg <mika.westerberg@linux.intel.com> > Cc: Lukas Wunner <lukas@wunner.de> > Signed-off-by: Hans de Goede <hdegoede@redhat.com> > --- > Changes in v2: > -Switch to using bus_find_device() to avoid "Traversing the namespace > over and over" > -Add optional (may be NULL / -1) uid and hrv arguments, this will > allow this new function to replace the custom code for this in > drivers/firmware/efi/dev-path-parser.c as well as in > sound/soc/intel/common/sst-match-acpi.c and will allow it to be > used to implement blacklists to avoid loading the ACPI ac / battery > driver on systems which have a PMIC / charger acpi device with a > native driver which offers a better (often working vs not working) > user experience > -Dropped Mika's reviewd by as this is almost a total rewrite > Changes in v3: > -memset the entire acpi_dev_present_info struct, this fixes > acpi_device_id.cls not getting cleared > Changes in v4: > -Use empty initializer to zero the acpi_dev_present_info struct > --- > drivers/acpi/utils.c | 71 +++++++++++++++++++++++++++++++++++++++++++++++++ > include/acpi/acpi_bus.h | 1 + > include/linux/acpi.h | 5 ++++ > 3 files changed, 77 insertions(+) > > diff --git a/drivers/acpi/utils.c b/drivers/acpi/utils.c > index 22c0995..ecd86a9 100644 > --- a/drivers/acpi/utils.c > +++ b/drivers/acpi/utils.c > @@ -736,6 +736,77 @@ bool acpi_dev_found(const char *hid) > } > EXPORT_SYMBOL(acpi_dev_found); > > +struct acpi_dev_present_info { > + struct acpi_device_id hid[2]; > + const char *uid; > + int hrv; > +}; > + > +static int acpi_dev_present_cb(struct device *dev, void *data) > +{ > + struct acpi_device *adev = to_acpi_device(dev); > + struct acpi_dev_present_info *match = data; > + unsigned long long hrv; > + acpi_status status; > + > + if (acpi_match_device_ids(adev, match->hid)) > + return 0; > + > + if (match->uid && adev->pnp.unique_id && > + strcmp(adev->pnp.unique_id, match->uid)) > + return 0; > + > + if (match->uid && !adev->pnp.unique_id && > + strcmp("0", match->uid)) > + return 0; > + > + if (match->hrv == -1) > + return 1; > + > + status = acpi_evaluate_integer(adev->handle, "_HRV", NULL, &hrv); > + if (ACPI_FAILURE(status)) > + return 0; > + > + return hrv == match->hrv; > +} > + > +/** > + * acpi_dev_present - Detect that a given ACPI device is present > + * @hid: Hardware ID of the device. > + * @uid: Unique ID of the device, pass "0" for devices without a _UID, > + * pass NULL to not check _UID > + * @hrv: Hardware Revision of the device, pass -1 to not check _HRV > + * > + * Return %true if a matching 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. > + * > + * For this function to work, acpi_bus_scan() must have been executed > + * which happens in the subsys_initcall() subsection. Hence, do not > + * call from a subsys_initcall() or earlier (use acpi_get_devices() > + * instead). Calling from module_init() is fine (which is synonymous > + * with device_initcall()). > + */ > +bool acpi_dev_present(const char *hid, const char *uid, int hrv) > +{ > + struct acpi_dev_present_info match = {}; > + struct device *dev; > + > + strlcpy(match.hid[0].id, hid, sizeof(match.hid[0].id)); > + match.uid = uid; > + match.hrv = hrv; > + > + dev = bus_find_device(&acpi_bus_type, NULL, &match, > + acpi_dev_present_cb); > + > + return dev ? true : false; > +} > +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..64498d5 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, const char *uid, int hrv); > > #ifdef CONFIG_ACPI > > diff --git a/include/linux/acpi.h b/include/linux/acpi.h > index 9b05886..e5dd0f1 100644 > --- a/include/linux/acpi.h > +++ b/include/linux/acpi.h > @@ -611,6 +611,11 @@ static inline bool acpi_dev_found(const char *hid) > return false; > } > > +static inline bool acpi_dev_present(const char *hid, const char *uid, int hrv) > +{ > + return false; > +} > + > static inline bool is_acpi_node(struct fwnode_handle *fwnode) > { > return false; > -- > 2.9.3
Hi, On 10-04-17 09:23, Hans de Goede wrote: > Hi All, > > Self-nack for patches 3-4 in this series, it turns out that > doing the global blacklist for the ACPI ac / battery is not a > good idea. > > Some Bay Trail / Cherry Trail users are running kernels build > from my personal tree to get early access to various fixes > in there and I got a regression report on the DELL 5855, where > the blacklisting of the ACPI battery driver if INT33F4 is > present caused the battery monitoring to stop working, that > devices has an INT33F4 node with _STA returning 15 yet it > is not using an axp288 PMIC at all, I'm still gathering more > info, but I believe atm that Dell simply disabled the i2c > controller to which the axp288 would be connected if present > and left the other bits of the DTSD unmodified. > > One option which comes to mind would be to only count devices > as present if all their deps are met, but that will only > work if the blacklist check is done after all other drivers > have loaded which is not how things work. > > So I believe that my earlier attempts at fixing the double > power_supply registration by unregistering the ACPI one when > the native one has successfully loaded is better. That guarantees > regressions like this one will not happen, because the ACPI > power_supply does not get unregistered until the native one > has loaded. Ok scrap that, I've some more info and the INT33F4 node on the Dell 5855 is returning 0 from _STA and the blacklist is causing problems on that machine for other reason, it could be the user was using an older version with the uninitialized .cls match info problem, I've asked the user to test the latest version. So assuming that does work, we are good to go with the blacklist approach (which seems to be the solution everyone prefers), sorry about the noise. I will send a v5 of this patch addressing Lukas' comments, I will re-submit patches 2-4 when I hear back from the Dell 5855 user. Regards, Hans > > Regards, > > Hans > > > > On Sat, Apr 08, 2017 at 11:48:27PM +0200, 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. >> >> Like acpi_dev_found, acpi_dev_present take a HID as argument, but >> it also has 2 extra optional arguments to only check for an ACPI >> device with a specific UID and/or HRV value. This makes it more >> generic and allows it to replace custom code doing similar checks >> in several places. >> >> 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. >> >> Cc: Mika Westerberg <mika.westerberg@linux.intel.com> >> Cc: Lukas Wunner <lukas@wunner.de> >> Signed-off-by: Hans de Goede <hdegoede@redhat.com> >> --- >> Changes in v2: >> -Switch to using bus_find_device() to avoid "Traversing the namespace >> over and over" >> -Add optional (may be NULL / -1) uid and hrv arguments, this will >> allow this new function to replace the custom code for this in >> drivers/firmware/efi/dev-path-parser.c as well as in >> sound/soc/intel/common/sst-match-acpi.c and will allow it to be >> used to implement blacklists to avoid loading the ACPI ac / battery >> driver on systems which have a PMIC / charger acpi device with a >> native driver which offers a better (often working vs not working) >> user experience >> -Dropped Mika's reviewd by as this is almost a total rewrite >> Changes in v3: >> -memset the entire acpi_dev_present_info struct, this fixes >> acpi_device_id.cls not getting cleared >> Changes in v4: >> -Use empty initializer to zero the acpi_dev_present_info struct >> --- >> drivers/acpi/utils.c | 71 +++++++++++++++++++++++++++++++++++++++++++++++++ >> include/acpi/acpi_bus.h | 1 + >> include/linux/acpi.h | 5 ++++ >> 3 files changed, 77 insertions(+) >> >> diff --git a/drivers/acpi/utils.c b/drivers/acpi/utils.c >> index 22c0995..ecd86a9 100644 >> --- a/drivers/acpi/utils.c >> +++ b/drivers/acpi/utils.c >> @@ -736,6 +736,77 @@ bool acpi_dev_found(const char *hid) >> } >> EXPORT_SYMBOL(acpi_dev_found); >> >> +struct acpi_dev_present_info { >> + struct acpi_device_id hid[2]; >> + const char *uid; >> + int hrv; >> +}; >> + >> +static int acpi_dev_present_cb(struct device *dev, void *data) >> +{ >> + struct acpi_device *adev = to_acpi_device(dev); >> + struct acpi_dev_present_info *match = data; >> + unsigned long long hrv; >> + acpi_status status; >> + >> + if (acpi_match_device_ids(adev, match->hid)) >> + return 0; >> + >> + if (match->uid && adev->pnp.unique_id && >> + strcmp(adev->pnp.unique_id, match->uid)) >> + return 0; >> + >> + if (match->uid && !adev->pnp.unique_id && >> + strcmp("0", match->uid)) >> + return 0; >> + >> + if (match->hrv == -1) >> + return 1; >> + >> + status = acpi_evaluate_integer(adev->handle, "_HRV", NULL, &hrv); >> + if (ACPI_FAILURE(status)) >> + return 0; >> + >> + return hrv == match->hrv; >> +} >> + >> +/** >> + * acpi_dev_present - Detect that a given ACPI device is present >> + * @hid: Hardware ID of the device. >> + * @uid: Unique ID of the device, pass "0" for devices without a _UID, >> + * pass NULL to not check _UID >> + * @hrv: Hardware Revision of the device, pass -1 to not check _HRV >> + * >> + * Return %true if a matching 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. >> + * >> + * For this function to work, acpi_bus_scan() must have been executed >> + * which happens in the subsys_initcall() subsection. Hence, do not >> + * call from a subsys_initcall() or earlier (use acpi_get_devices() >> + * instead). Calling from module_init() is fine (which is synonymous >> + * with device_initcall()). >> + */ >> +bool acpi_dev_present(const char *hid, const char *uid, int hrv) >> +{ >> + struct acpi_dev_present_info match = {}; >> + struct device *dev; >> + >> + strlcpy(match.hid[0].id, hid, sizeof(match.hid[0].id)); >> + match.uid = uid; >> + match.hrv = hrv; >> + >> + dev = bus_find_device(&acpi_bus_type, NULL, &match, >> + acpi_dev_present_cb); >> + >> + return dev ? true : false; >> +} >> +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..64498d5 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, const char *uid, int hrv); >> >> #ifdef CONFIG_ACPI >> >> diff --git a/include/linux/acpi.h b/include/linux/acpi.h >> index 9b05886..e5dd0f1 100644 >> --- a/include/linux/acpi.h >> +++ b/include/linux/acpi.h >> @@ -611,6 +611,11 @@ static inline bool acpi_dev_found(const char *hid) >> return false; >> } >> >> +static inline bool acpi_dev_present(const char *hid, const char *uid, int hrv) >> +{ >> + return false; >> +} >> + >> static inline bool is_acpi_node(struct fwnode_handle *fwnode) >> { >> return false; >> -- >> 2.9.3
Hi, On 09-04-17 10:16, Lukas Wunner wrote: > On Sat, Apr 08, 2017 at 11:48:27PM +0200, 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. >> >> Like acpi_dev_found, acpi_dev_present take a HID as argument, but >> it also has 2 extra optional arguments to only check for an ACPI >> device with a specific UID and/or HRV value. This makes it more >> generic and allows it to replace custom code doing similar checks >> in several places. >> >> 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. >> >> Cc: Mika Westerberg <mika.westerberg@linux.intel.com> >> Cc: Lukas Wunner <lukas@wunner.de> >> Signed-off-by: Hans de Goede <hdegoede@redhat.com> >> --- >> Changes in v2: >> -Switch to using bus_find_device() to avoid "Traversing the namespace >> over and over" >> -Add optional (may be NULL / -1) uid and hrv arguments, this will >> allow this new function to replace the custom code for this in >> drivers/firmware/efi/dev-path-parser.c as well as in >> sound/soc/intel/common/sst-match-acpi.c and will allow it to be >> used to implement blacklists to avoid loading the ACPI ac / battery >> driver on systems which have a PMIC / charger acpi device with a >> native driver which offers a better (often working vs not working) >> user experience >> -Dropped Mika's reviewd by as this is almost a total rewrite >> Changes in v3: >> -memset the entire acpi_dev_present_info struct, this fixes >> acpi_device_id.cls not getting cleared >> Changes in v4: >> -Use empty initializer to zero the acpi_dev_present_info struct >> --- >> drivers/acpi/utils.c | 71 +++++++++++++++++++++++++++++++++++++++++++++++++ >> include/acpi/acpi_bus.h | 1 + >> include/linux/acpi.h | 5 ++++ >> 3 files changed, 77 insertions(+) >> >> diff --git a/drivers/acpi/utils.c b/drivers/acpi/utils.c >> index 22c0995..ecd86a9 100644 >> --- a/drivers/acpi/utils.c >> +++ b/drivers/acpi/utils.c >> @@ -736,6 +736,77 @@ bool acpi_dev_found(const char *hid) >> } >> EXPORT_SYMBOL(acpi_dev_found); >> >> +struct acpi_dev_present_info { >> + struct acpi_device_id hid[2]; >> + const char *uid; >> + int hrv; >> +}; > > There's a (somewhat theoretical) issue on 32 bit arches with your choice > of int for hrv: _HRV returns an "Integer (DWORD)" per the spec. That's > 32 bit and clashes with your use of -1 to signify "don't care" if the > arch uses 32 bit to represent an int. > > So you need to use a "signed long long" or "s64" here. Switched to s64 for v5. >> + >> +static int acpi_dev_present_cb(struct device *dev, void *data) >> +{ >> + struct acpi_device *adev = to_acpi_device(dev); >> + struct acpi_dev_present_info *match = data; >> + unsigned long long hrv; >> + acpi_status status; >> + >> + if (acpi_match_device_ids(adev, match->hid)) >> + return 0; >> + >> + if (match->uid && adev->pnp.unique_id && >> + strcmp(adev->pnp.unique_id, match->uid)) >> + return 0; >> + >> + if (match->uid && !adev->pnp.unique_id && >> + strcmp("0", match->uid)) >> + return 0; >> + >> + if (match->hrv == -1) >> + return 1; >> + >> + status = acpi_evaluate_integer(adev->handle, "_HRV", NULL, &hrv); >> + if (ACPI_FAILURE(status)) >> + return 0; >> + >> + return hrv == match->hrv; >> +} >> + >> +/** >> + * acpi_dev_present - Detect that a given ACPI device is present >> + * @hid: Hardware ID of the device. >> + * @uid: Unique ID of the device, pass "0" for devices without a _UID, >> + * pass NULL to not check _UID >> + * @hrv: Hardware Revision of the device, pass -1 to not check _HRV >> + * >> + * Return %true if a matching 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 > ^ > Minor nit: I think readability improves if you start a new sentence or > insert a comma here. Sorry, just my grammar OCD. Started a new sentence at "So for" for v5. > > Otherwise, > Reviewed-by: Lukas Wunner <lukas@wunner.de> Thank you. > I'm clueless about ACPI battery or charging, so can't say anything about > the remainder of the series. > > Note that the code in drivers/firmware/efi/dev-path-parser.c deliberately > increments the refcount of the device found and returns a pointer to it, > whereas acpi_dev_present() as introduced by this patch merely checks > presence and returns a bool. So you won't be able to replace the code > in dev-path-parser.c with acpi_dev_present(), but I think that's > perfectly fine. Ah, bummer, would have been nice to be able to re-use the code. Regards, Hans > > Thanks, > > Lukas > >> + * which are disabled (their _STA callback returns 0) this function >> + * will return false. >> + * >> + * For this function to work, acpi_bus_scan() must have been executed >> + * which happens in the subsys_initcall() subsection. Hence, do not >> + * call from a subsys_initcall() or earlier (use acpi_get_devices() >> + * instead). Calling from module_init() is fine (which is synonymous >> + * with device_initcall()). >> + */ >> +bool acpi_dev_present(const char *hid, const char *uid, int hrv) >> +{ >> + struct acpi_dev_present_info match = {}; >> + struct device *dev; >> + >> + strlcpy(match.hid[0].id, hid, sizeof(match.hid[0].id)); >> + match.uid = uid; >> + match.hrv = hrv; >> + >> + dev = bus_find_device(&acpi_bus_type, NULL, &match, >> + acpi_dev_present_cb); >> + >> + return dev ? true : false; >> +} >> +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..64498d5 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, const char *uid, int hrv); >> >> #ifdef CONFIG_ACPI >> >> diff --git a/include/linux/acpi.h b/include/linux/acpi.h >> index 9b05886..e5dd0f1 100644 >> --- a/include/linux/acpi.h >> +++ b/include/linux/acpi.h >> @@ -611,6 +611,11 @@ static inline bool acpi_dev_found(const char *hid) >> return false; >> } >> >> +static inline bool acpi_dev_present(const char *hid, const char *uid, int hrv) >> +{ >> + return false; >> +} >> + >> static inline bool is_acpi_node(struct fwnode_handle *fwnode) >> { >> return false; >> -- >> 2.9.3
diff --git a/drivers/acpi/utils.c b/drivers/acpi/utils.c index 22c0995..ecd86a9 100644 --- a/drivers/acpi/utils.c +++ b/drivers/acpi/utils.c @@ -736,6 +736,77 @@ bool acpi_dev_found(const char *hid) } EXPORT_SYMBOL(acpi_dev_found); +struct acpi_dev_present_info { + struct acpi_device_id hid[2]; + const char *uid; + int hrv; +}; + +static int acpi_dev_present_cb(struct device *dev, void *data) +{ + struct acpi_device *adev = to_acpi_device(dev); + struct acpi_dev_present_info *match = data; + unsigned long long hrv; + acpi_status status; + + if (acpi_match_device_ids(adev, match->hid)) + return 0; + + if (match->uid && adev->pnp.unique_id && + strcmp(adev->pnp.unique_id, match->uid)) + return 0; + + if (match->uid && !adev->pnp.unique_id && + strcmp("0", match->uid)) + return 0; + + if (match->hrv == -1) + return 1; + + status = acpi_evaluate_integer(adev->handle, "_HRV", NULL, &hrv); + if (ACPI_FAILURE(status)) + return 0; + + return hrv == match->hrv; +} + +/** + * acpi_dev_present - Detect that a given ACPI device is present + * @hid: Hardware ID of the device. + * @uid: Unique ID of the device, pass "0" for devices without a _UID, + * pass NULL to not check _UID + * @hrv: Hardware Revision of the device, pass -1 to not check _HRV + * + * Return %true if a matching 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. + * + * For this function to work, acpi_bus_scan() must have been executed + * which happens in the subsys_initcall() subsection. Hence, do not + * call from a subsys_initcall() or earlier (use acpi_get_devices() + * instead). Calling from module_init() is fine (which is synonymous + * with device_initcall()). + */ +bool acpi_dev_present(const char *hid, const char *uid, int hrv) +{ + struct acpi_dev_present_info match = {}; + struct device *dev; + + strlcpy(match.hid[0].id, hid, sizeof(match.hid[0].id)); + match.uid = uid; + match.hrv = hrv; + + dev = bus_find_device(&acpi_bus_type, NULL, &match, + acpi_dev_present_cb); + + return dev ? true : false; +} +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..64498d5 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, const char *uid, int hrv); #ifdef CONFIG_ACPI diff --git a/include/linux/acpi.h b/include/linux/acpi.h index 9b05886..e5dd0f1 100644 --- a/include/linux/acpi.h +++ b/include/linux/acpi.h @@ -611,6 +611,11 @@ static inline bool acpi_dev_found(const char *hid) return false; } +static inline bool acpi_dev_present(const char *hid, const char *uid, int hrv) +{ + 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. Like acpi_dev_found, acpi_dev_present take a HID as argument, but it also has 2 extra optional arguments to only check for an ACPI device with a specific UID and/or HRV value. This makes it more generic and allows it to replace custom code doing similar checks in several places. 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. Cc: Mika Westerberg <mika.westerberg@linux.intel.com> Cc: Lukas Wunner <lukas@wunner.de> Signed-off-by: Hans de Goede <hdegoede@redhat.com> --- Changes in v2: -Switch to using bus_find_device() to avoid "Traversing the namespace over and over" -Add optional (may be NULL / -1) uid and hrv arguments, this will allow this new function to replace the custom code for this in drivers/firmware/efi/dev-path-parser.c as well as in sound/soc/intel/common/sst-match-acpi.c and will allow it to be used to implement blacklists to avoid loading the ACPI ac / battery driver on systems which have a PMIC / charger acpi device with a native driver which offers a better (often working vs not working) user experience -Dropped Mika's reviewd by as this is almost a total rewrite Changes in v3: -memset the entire acpi_dev_present_info struct, this fixes acpi_device_id.cls not getting cleared Changes in v4: -Use empty initializer to zero the acpi_dev_present_info struct --- drivers/acpi/utils.c | 71 +++++++++++++++++++++++++++++++++++++++++++++++++ include/acpi/acpi_bus.h | 1 + include/linux/acpi.h | 5 ++++ 3 files changed, 77 insertions(+)