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@xxxxxxxxxxxxxxx> Cc: Lukas Wunner <lukas@xxxxxxxxx> Signed-off-by: Hans de Goede <hdegoede@xxxxxxxxxx> --- 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
-- To unsubscribe from this list: send the line "unsubscribe linux-acpi" in the body of a message to majordomo@xxxxxxxxxxxxxxx More majordomo info at http://vger.kernel.org/majordomo-info.html
