[PATCH v2 0/2] platform/x86: dell: Add new dell-wmi-ddv driver

[PATCH v2 0/2] platform/x86: dell: Add new dell-wmi-ddv driver

[Armin Wolf]

This patch series adds a new driver for a WMI interface found in
many newer Dell machines. This interface allows to read battery
properties like temperature and the ePPID (Dell-specific), while
also providing fan and thermal sensor information.

The interface does support multiple batteries which are indentified
by an "index", which appears to be the battery ACPI UID. Since
the interface also appears to omit any bounts checking of the
index, the ACPI battery hook mechanism is used to discover batteries.

Since the information returned when querying fan/thermal sensor
information is currently unknown, a debugfs entry is created to
allow for easier reverse engineering. The interface is likely
to be replaced by a proper hwmon interface in the future.

Since the driver can potentially be instantiated multiple times,
the ACPI battery hook mechanism had to be extended.

The first patch passes a pointer to the battery hook to the
hook callbacks, so that they can access instance-specific data
with container_of().

The second patch finally adds the new driver. It was called
dell-wmi-ddv since the interface is called "DDV" by Dell software,
likely meaning "Dell Data Vault".

The driver was tested, together with the changes made to the
ACPI battery driver, on a Dell Inspiron 3505. Other drivers
already using the battery hook mechanism where changed as well,
but could only be compile-tested due to missing hardware.

---
Changes in v2:
- Significantly lower the amount of changes being made to the
acpi battery driver
- drop unnecessary ABI description of the temp attribute
- return 0 when a unsupported battery is found to avoid being
unloaded

Armin Wolf (2):
  ACPI: battery: Pass battery hook pointer to hook callbacks
  platform/x86: dell: Add new dell-wmi-ddv driver

 .../ABI/testing/debugfs-dell-wmi-ddv          |  21 +
 .../ABI/testing/sysfs-platform-dell-wmi-ddv   |   7 +
 MAINTAINERS                                   |   7 +
 drivers/acpi/battery.c                        |   8 +-
 drivers/platform/x86/asus-wmi.c               |   4 +-
 drivers/platform/x86/dell/Kconfig             |  13 +
 drivers/platform/x86/dell/Makefile            |   1 +
 drivers/platform/x86/dell/dell-wmi-ddv.c      | 361 ++++++++++++++++++
 drivers/platform/x86/huawei-wmi.c             |   4 +-
 drivers/platform/x86/lg-laptop.c              |   4 +-
 drivers/platform/x86/system76_acpi.c          |   4 +-
 drivers/platform/x86/thinkpad_acpi.c          |   4 +-
 drivers/platform/x86/toshiba_acpi.c           |   4 +-
 drivers/platform/x86/wmi.c                    |   1 +
 include/acpi/battery.h                        |   4 +-
 15 files changed, 429 insertions(+), 18 deletions(-)
 create mode 100644 Documentation/ABI/testing/debugfs-dell-wmi-ddv
 create mode 100644 Documentation/ABI/testing/sysfs-platform-dell-wmi-ddv
 create mode 100644 drivers/platform/x86/dell/dell-wmi-ddv.c

--
2.30.2

[PATCH v2 1/2] ACPI: battery: Pass battery hook pointer to hook callbacks

[Armin Wolf]

Right now, is impossible for battery hook callbacks
to access instance-specific data, forcing most drivers
to provide some sort of global state. This however is
difficult for drivers which can be instantiated multiple
times and/or are hotplug-capable.

Pass a pointer to the batetry hook to those callbacks
for usage with container_of().

Signed-off-by: Armin Wolf <W_Armin@gmx.de>
---
 drivers/acpi/battery.c               | 8 ++++----
 drivers/platform/x86/asus-wmi.c      | 4 ++--
 drivers/platform/x86/huawei-wmi.c    | 4 ++--
 drivers/platform/x86/lg-laptop.c     | 4 ++--
 drivers/platform/x86/system76_acpi.c | 4 ++--
 drivers/platform/x86/thinkpad_acpi.c | 4 ++--
 drivers/platform/x86/toshiba_acpi.c  | 4 ++--
 include/acpi/battery.h               | 4 ++--
 8 files changed, 18 insertions(+), 18 deletions(-)

diff --git a/drivers/acpi/battery.c b/drivers/acpi/battery.c
index 306513fec1e1..9482b0b6eadc 100644
--- a/drivers/acpi/battery.c
+++ b/drivers/acpi/battery.c
@@ -696,7 +696,7 @@ static void __battery_hook_unregister(struct acpi_battery_hook *hook, int lock)
 	if (lock)
 		mutex_lock(&hook_mutex);
 	list_for_each_entry(battery, &acpi_battery_list, list) {
-		hook->remove_battery(battery->bat);
+		hook->remove_battery(battery->bat, hook);
 	}
 	list_del(&hook->list);
 	if (lock)
@@ -724,7 +724,7 @@ void battery_hook_register(struct acpi_battery_hook *hook)
 	 * its attributes.
 	 */
 	list_for_each_entry(battery, &acpi_battery_list, list) {
-		if (hook->add_battery(battery->bat)) {
+		if (hook->add_battery(battery->bat, hook)) {
 			/*
 			 * If a add-battery returns non-zero,
 			 * the registration of the extension has failed,
@@ -762,7 +762,7 @@ static void battery_hook_add_battery(struct acpi_battery *battery)
 	 * during the battery module initialization.
 	 */
 	list_for_each_entry_safe(hook_node, tmp, &battery_hook_list, list) {
-		if (hook_node->add_battery(battery->bat)) {
+		if (hook_node->add_battery(battery->bat, hook_node)) {
 			/*
 			 * The notification of the extensions has failed, to
 			 * prevent further errors we will unload the extension.
@@ -785,7 +785,7 @@ static void battery_hook_remove_battery(struct acpi_battery *battery)
 	 * custom attributes from the battery.
 	 */
 	list_for_each_entry(hook, &battery_hook_list, list) {
-		hook->remove_battery(battery->bat);
+		hook->remove_battery(battery->bat, hook);
 	}
 	/* Then, just remove the battery from the list */
 	list_del(&battery->list);
diff --git a/drivers/platform/x86/asus-wmi.c b/drivers/platform/x86/asus-wmi.c
index ae46af731de9..446669d11095 100644
--- a/drivers/platform/x86/asus-wmi.c
+++ b/drivers/platform/x86/asus-wmi.c
@@ -882,7 +882,7 @@ static ssize_t charge_control_end_threshold_show(struct device *device,

 static DEVICE_ATTR_RW(charge_control_end_threshold);

-static int asus_wmi_battery_add(struct power_supply *battery)
+static int asus_wmi_battery_add(struct power_supply *battery, struct acpi_battery_hook *hook)
 {
 	/* The WMI method does not provide a way to specific a battery, so we
 	 * just assume it is the first battery.
@@ -909,7 +909,7 @@ static int asus_wmi_battery_add(struct power_supply *battery)
 	return 0;
 }

-static int asus_wmi_battery_remove(struct power_supply *battery)
+static int asus_wmi_battery_remove(struct power_supply *battery, struct acpi_battery_hook *hook)
 {
 	device_remove_file(&battery->dev,
 			   &dev_attr_charge_control_end_threshold);
diff --git a/drivers/platform/x86/huawei-wmi.c b/drivers/platform/x86/huawei-wmi.c
index eac3e6b4ea11..1dec4427053a 100644
--- a/drivers/platform/x86/huawei-wmi.c
+++ b/drivers/platform/x86/huawei-wmi.c
@@ -468,7 +468,7 @@ static DEVICE_ATTR_RW(charge_control_start_threshold);
 static DEVICE_ATTR_RW(charge_control_end_threshold);
 static DEVICE_ATTR_RW(charge_control_thresholds);

-static int huawei_wmi_battery_add(struct power_supply *battery)
+static int huawei_wmi_battery_add(struct power_supply *battery, struct acpi_battery_hook *hook)
 {
 	int err = 0;

@@ -483,7 +483,7 @@ static int huawei_wmi_battery_add(struct power_supply *battery)
 	return err;
 }

-static int huawei_wmi_battery_remove(struct power_supply *battery)
+static int huawei_wmi_battery_remove(struct power_supply *battery, struct acpi_battery_hook *hook)
 {
 	device_remove_file(&battery->dev, &dev_attr_charge_control_start_threshold);
 	device_remove_file(&battery->dev, &dev_attr_charge_control_end_threshold);
diff --git a/drivers/platform/x86/lg-laptop.c b/drivers/platform/x86/lg-laptop.c
index 332868b140ed..d662b64b0ba9 100644
--- a/drivers/platform/x86/lg-laptop.c
+++ b/drivers/platform/x86/lg-laptop.c
@@ -546,7 +546,7 @@ static DEVICE_ATTR_RW(fn_lock);
 static DEVICE_ATTR_RW(charge_control_end_threshold);
 static DEVICE_ATTR_RW(battery_care_limit);

-static int lg_battery_add(struct power_supply *battery)
+static int lg_battery_add(struct power_supply *battery, struct acpi_battery_hook *hook)
 {
 	if (device_create_file(&battery->dev,
 			       &dev_attr_charge_control_end_threshold))
@@ -555,7 +555,7 @@ static int lg_battery_add(struct power_supply *battery)
 	return 0;
 }

-static int lg_battery_remove(struct power_supply *battery)
+static int lg_battery_remove(struct power_supply *battery, struct acpi_battery_hook *hook)
 {
 	device_remove_file(&battery->dev,
 			   &dev_attr_charge_control_end_threshold);
diff --git a/drivers/platform/x86/system76_acpi.c b/drivers/platform/x86/system76_acpi.c
index 958df41ad509..9031bd53253f 100644
--- a/drivers/platform/x86/system76_acpi.c
+++ b/drivers/platform/x86/system76_acpi.c
@@ -254,7 +254,7 @@ static struct attribute *system76_battery_attrs[] = {

 ATTRIBUTE_GROUPS(system76_battery);

-static int system76_battery_add(struct power_supply *battery)
+static int system76_battery_add(struct power_supply *battery, struct acpi_battery_hook *hook)
 {
 	// System76 EC only supports 1 battery
 	if (strcmp(battery->desc->name, "BAT0") != 0)
@@ -266,7 +266,7 @@ static int system76_battery_add(struct power_supply *battery)
 	return 0;
 }

-static int system76_battery_remove(struct power_supply *battery)
+static int system76_battery_remove(struct power_supply *battery, struct acpi_battery_hook *hook)
 {
 	device_remove_groups(&battery->dev, system76_battery_groups);
 	return 0;
diff --git a/drivers/platform/x86/thinkpad_acpi.c b/drivers/platform/x86/thinkpad_acpi.c
index 8fbe21ebcc52..75ba9e61264e 100644
--- a/drivers/platform/x86/thinkpad_acpi.c
+++ b/drivers/platform/x86/thinkpad_acpi.c
@@ -9897,7 +9897,7 @@ ATTRIBUTE_GROUPS(tpacpi_battery);

 /* ACPI battery hooking */

-static int tpacpi_battery_add(struct power_supply *battery)
+static int tpacpi_battery_add(struct power_supply *battery, struct acpi_battery_hook *hook)
 {
 	int batteryid = tpacpi_battery_get_id(battery->desc->name);

@@ -9908,7 +9908,7 @@ static int tpacpi_battery_add(struct power_supply *battery)
 	return 0;
 }

-static int tpacpi_battery_remove(struct power_supply *battery)
+static int tpacpi_battery_remove(struct power_supply *battery, struct acpi_battery_hook *hook)
 {
 	device_remove_groups(&battery->dev, tpacpi_battery_groups);
 	return 0;
diff --git a/drivers/platform/x86/toshiba_acpi.c b/drivers/platform/x86/toshiba_acpi.c
index 43cc25351aea..c8f01f8f435d 100644
--- a/drivers/platform/x86/toshiba_acpi.c
+++ b/drivers/platform/x86/toshiba_acpi.c
@@ -3113,7 +3113,7 @@ static struct attribute *toshiba_acpi_battery_attrs[] = {

 ATTRIBUTE_GROUPS(toshiba_acpi_battery);

-static int toshiba_acpi_battery_add(struct power_supply *battery)
+static int toshiba_acpi_battery_add(struct power_supply *battery, struct acpi_battery_hook *hook)
 {
 	if (toshiba_acpi == NULL) {
 		pr_err("Init order issue\n");
@@ -3126,7 +3126,7 @@ static int toshiba_acpi_battery_add(struct power_supply *battery)
 	return 0;
 }

-static int toshiba_acpi_battery_remove(struct power_supply *battery)
+static int toshiba_acpi_battery_remove(struct power_supply *battery, struct acpi_battery_hook *hook)
 {
 	device_remove_groups(&battery->dev, toshiba_acpi_battery_groups);
 	return 0;
diff --git a/include/acpi/battery.h b/include/acpi/battery.h
index b8d56b702c7a..611a2561a014 100644
--- a/include/acpi/battery.h
+++ b/include/acpi/battery.h
@@ -12,8 +12,8 @@

 struct acpi_battery_hook {
 	const char *name;
-	int (*add_battery)(struct power_supply *battery);
-	int (*remove_battery)(struct power_supply *battery);
+	int (*add_battery)(struct power_supply *battery, struct acpi_battery_hook *hook);
+	int (*remove_battery)(struct power_supply *battery, struct acpi_battery_hook *hook);
 	struct list_head list;
 };

--
2.30.2

Re: [PATCH v2 1/2] ACPI: battery: Pass battery hook pointer to hook callbacks

[Rafael J. Wysocki]

On Tue, Sep 27, 2022 at 10:45 PM Armin Wolf <W_Armin@gmx.de> wrote:
>
> Right now, is impossible for battery hook callbacks
> to access instance-specific data, forcing most drivers
> to provide some sort of global state. This however is
> difficult for drivers which can be instantiated multiple
> times and/or are hotplug-capable.
>
> Pass a pointer to the batetry hook to those callbacks
> for usage with container_of().
>
> Signed-off-by: Armin Wolf <W_Armin@gmx.de>

Acked-by: Rafael J. Wysocki <rafael.j.wysocki@intel.com>

> ---
>  drivers/acpi/battery.c               | 8 ++++----
>  drivers/platform/x86/asus-wmi.c      | 4 ++--
>  drivers/platform/x86/huawei-wmi.c    | 4 ++--
>  drivers/platform/x86/lg-laptop.c     | 4 ++--
>  drivers/platform/x86/system76_acpi.c | 4 ++--
>  drivers/platform/x86/thinkpad_acpi.c | 4 ++--
>  drivers/platform/x86/toshiba_acpi.c  | 4 ++--
>  include/acpi/battery.h               | 4 ++--
>  8 files changed, 18 insertions(+), 18 deletions(-)
>
> diff --git a/drivers/acpi/battery.c b/drivers/acpi/battery.c
> index 306513fec1e1..9482b0b6eadc 100644
> --- a/drivers/acpi/battery.c
> +++ b/drivers/acpi/battery.c
> @@ -696,7 +696,7 @@ static void __battery_hook_unregister(struct acpi_battery_hook *hook, int lock)
>         if (lock)
>                 mutex_lock(&hook_mutex);
>         list_for_each_entry(battery, &acpi_battery_list, list) {
> -               hook->remove_battery(battery->bat);
> +               hook->remove_battery(battery->bat, hook);
>         }
>         list_del(&hook->list);
>         if (lock)
> @@ -724,7 +724,7 @@ void battery_hook_register(struct acpi_battery_hook *hook)
>          * its attributes.
>          */
>         list_for_each_entry(battery, &acpi_battery_list, list) {
> -               if (hook->add_battery(battery->bat)) {
> +               if (hook->add_battery(battery->bat, hook)) {
>                         /*
>                          * If a add-battery returns non-zero,
>                          * the registration of the extension has failed,
> @@ -762,7 +762,7 @@ static void battery_hook_add_battery(struct acpi_battery *battery)
>          * during the battery module initialization.
>          */
>         list_for_each_entry_safe(hook_node, tmp, &battery_hook_list, list) {
> -               if (hook_node->add_battery(battery->bat)) {
> +               if (hook_node->add_battery(battery->bat, hook_node)) {
>                         /*
>                          * The notification of the extensions has failed, to
>                          * prevent further errors we will unload the extension.
> @@ -785,7 +785,7 @@ static void battery_hook_remove_battery(struct acpi_battery *battery)
>          * custom attributes from the battery.
>          */
>         list_for_each_entry(hook, &battery_hook_list, list) {
> -               hook->remove_battery(battery->bat);
> +               hook->remove_battery(battery->bat, hook);
>         }
>         /* Then, just remove the battery from the list */
>         list_del(&battery->list);
> diff --git a/drivers/platform/x86/asus-wmi.c b/drivers/platform/x86/asus-wmi.c
> index ae46af731de9..446669d11095 100644
> --- a/drivers/platform/x86/asus-wmi.c
> +++ b/drivers/platform/x86/asus-wmi.c
> @@ -882,7 +882,7 @@ static ssize_t charge_control_end_threshold_show(struct device *device,
>
>  static DEVICE_ATTR_RW(charge_control_end_threshold);
>
> -static int asus_wmi_battery_add(struct power_supply *battery)
> +static int asus_wmi_battery_add(struct power_supply *battery, struct acpi_battery_hook *hook)
>  {
>         /* The WMI method does not provide a way to specific a battery, so we
>          * just assume it is the first battery.
> @@ -909,7 +909,7 @@ static int asus_wmi_battery_add(struct power_supply *battery)
>         return 0;
>  }
>
> -static int asus_wmi_battery_remove(struct power_supply *battery)
> +static int asus_wmi_battery_remove(struct power_supply *battery, struct acpi_battery_hook *hook)
>  {
>         device_remove_file(&battery->dev,
>                            &dev_attr_charge_control_end_threshold);
> diff --git a/drivers/platform/x86/huawei-wmi.c b/drivers/platform/x86/huawei-wmi.c
> index eac3e6b4ea11..1dec4427053a 100644
> --- a/drivers/platform/x86/huawei-wmi.c
> +++ b/drivers/platform/x86/huawei-wmi.c
> @@ -468,7 +468,7 @@ static DEVICE_ATTR_RW(charge_control_start_threshold);
>  static DEVICE_ATTR_RW(charge_control_end_threshold);
>  static DEVICE_ATTR_RW(charge_control_thresholds);
>
> -static int huawei_wmi_battery_add(struct power_supply *battery)
> +static int huawei_wmi_battery_add(struct power_supply *battery, struct acpi_battery_hook *hook)
>  {
>         int err = 0;
>
> @@ -483,7 +483,7 @@ static int huawei_wmi_battery_add(struct power_supply *battery)
>         return err;
>  }
>
> -static int huawei_wmi_battery_remove(struct power_supply *battery)
> +static int huawei_wmi_battery_remove(struct power_supply *battery, struct acpi_battery_hook *hook)
>  {
>         device_remove_file(&battery->dev, &dev_attr_charge_control_start_threshold);
>         device_remove_file(&battery->dev, &dev_attr_charge_control_end_threshold);
> diff --git a/drivers/platform/x86/lg-laptop.c b/drivers/platform/x86/lg-laptop.c
> index 332868b140ed..d662b64b0ba9 100644
> --- a/drivers/platform/x86/lg-laptop.c
> +++ b/drivers/platform/x86/lg-laptop.c
> @@ -546,7 +546,7 @@ static DEVICE_ATTR_RW(fn_lock);
>  static DEVICE_ATTR_RW(charge_control_end_threshold);
>  static DEVICE_ATTR_RW(battery_care_limit);
>
> -static int lg_battery_add(struct power_supply *battery)
> +static int lg_battery_add(struct power_supply *battery, struct acpi_battery_hook *hook)
>  {
>         if (device_create_file(&battery->dev,
>                                &dev_attr_charge_control_end_threshold))
> @@ -555,7 +555,7 @@ static int lg_battery_add(struct power_supply *battery)
>         return 0;
>  }
>
> -static int lg_battery_remove(struct power_supply *battery)
> +static int lg_battery_remove(struct power_supply *battery, struct acpi_battery_hook *hook)
>  {
>         device_remove_file(&battery->dev,
>                            &dev_attr_charge_control_end_threshold);
> diff --git a/drivers/platform/x86/system76_acpi.c b/drivers/platform/x86/system76_acpi.c
> index 958df41ad509..9031bd53253f 100644
> --- a/drivers/platform/x86/system76_acpi.c
> +++ b/drivers/platform/x86/system76_acpi.c
> @@ -254,7 +254,7 @@ static struct attribute *system76_battery_attrs[] = {
>
>  ATTRIBUTE_GROUPS(system76_battery);
>
> -static int system76_battery_add(struct power_supply *battery)
> +static int system76_battery_add(struct power_supply *battery, struct acpi_battery_hook *hook)
>  {
>         // System76 EC only supports 1 battery
>         if (strcmp(battery->desc->name, "BAT0") != 0)
> @@ -266,7 +266,7 @@ static int system76_battery_add(struct power_supply *battery)
>         return 0;
>  }
>
> -static int system76_battery_remove(struct power_supply *battery)
> +static int system76_battery_remove(struct power_supply *battery, struct acpi_battery_hook *hook)
>  {
>         device_remove_groups(&battery->dev, system76_battery_groups);
>         return 0;
> diff --git a/drivers/platform/x86/thinkpad_acpi.c b/drivers/platform/x86/thinkpad_acpi.c
> index 8fbe21ebcc52..75ba9e61264e 100644
> --- a/drivers/platform/x86/thinkpad_acpi.c
> +++ b/drivers/platform/x86/thinkpad_acpi.c
> @@ -9897,7 +9897,7 @@ ATTRIBUTE_GROUPS(tpacpi_battery);
>
>  /* ACPI battery hooking */
>
> -static int tpacpi_battery_add(struct power_supply *battery)
> +static int tpacpi_battery_add(struct power_supply *battery, struct acpi_battery_hook *hook)
>  {
>         int batteryid = tpacpi_battery_get_id(battery->desc->name);
>
> @@ -9908,7 +9908,7 @@ static int tpacpi_battery_add(struct power_supply *battery)
>         return 0;
>  }
>
> -static int tpacpi_battery_remove(struct power_supply *battery)
> +static int tpacpi_battery_remove(struct power_supply *battery, struct acpi_battery_hook *hook)
>  {
>         device_remove_groups(&battery->dev, tpacpi_battery_groups);
>         return 0;
> diff --git a/drivers/platform/x86/toshiba_acpi.c b/drivers/platform/x86/toshiba_acpi.c
> index 43cc25351aea..c8f01f8f435d 100644
> --- a/drivers/platform/x86/toshiba_acpi.c
> +++ b/drivers/platform/x86/toshiba_acpi.c
> @@ -3113,7 +3113,7 @@ static struct attribute *toshiba_acpi_battery_attrs[] = {
>
>  ATTRIBUTE_GROUPS(toshiba_acpi_battery);
>
> -static int toshiba_acpi_battery_add(struct power_supply *battery)
> +static int toshiba_acpi_battery_add(struct power_supply *battery, struct acpi_battery_hook *hook)
>  {
>         if (toshiba_acpi == NULL) {
>                 pr_err("Init order issue\n");
> @@ -3126,7 +3126,7 @@ static int toshiba_acpi_battery_add(struct power_supply *battery)
>         return 0;
>  }
>
> -static int toshiba_acpi_battery_remove(struct power_supply *battery)
> +static int toshiba_acpi_battery_remove(struct power_supply *battery, struct acpi_battery_hook *hook)
>  {
>         device_remove_groups(&battery->dev, toshiba_acpi_battery_groups);
>         return 0;
> diff --git a/include/acpi/battery.h b/include/acpi/battery.h
> index b8d56b702c7a..611a2561a014 100644
> --- a/include/acpi/battery.h
> +++ b/include/acpi/battery.h
> @@ -12,8 +12,8 @@
>
>  struct acpi_battery_hook {
>         const char *name;
> -       int (*add_battery)(struct power_supply *battery);
> -       int (*remove_battery)(struct power_supply *battery);
> +       int (*add_battery)(struct power_supply *battery, struct acpi_battery_hook *hook);
> +       int (*remove_battery)(struct power_supply *battery, struct acpi_battery_hook *hook);
>         struct list_head list;
>  };
>
> --
> 2.30.2
>

Re: [PATCH v2 1/2] ACPI: battery: Pass battery hook pointer to hook callbacks

[Hans de Goede]

Hi,

On 9/27/22 22:45, Armin Wolf wrote:
> Right now, is impossible for battery hook callbacks
> to access instance-specific data, forcing most drivers
> to provide some sort of global state. This however is
> difficult for drivers which can be instantiated multiple
> times and/or are hotplug-capable.
> 
> Pass a pointer to the batetry hook to those callbacks
> for usage with container_of().
> 
> Signed-off-by: Armin Wolf <W_Armin@gmx.de>

Thank you for your patch-series, I've applied the series to my
review-hans branch:
https://git.kernel.org/pub/scm/linux/kernel/git/pdx86/platform-drivers-x86.git/log/?h=review-hans

Note it will show up in my review-hans branch once I've pushed my
local branch there, which might take a while.

Once I've run some tests on this branch the patches there will be
added to the platform-drivers-x86/for-next branch and eventually
will be included in the pdx86 pull-request to Linus for the next
merge-window.

Regards,

Hans



> ---
>  drivers/acpi/battery.c               | 8 ++++----
>  drivers/platform/x86/asus-wmi.c      | 4 ++--
>  drivers/platform/x86/huawei-wmi.c    | 4 ++--
>  drivers/platform/x86/lg-laptop.c     | 4 ++--
>  drivers/platform/x86/system76_acpi.c | 4 ++--
>  drivers/platform/x86/thinkpad_acpi.c | 4 ++--
>  drivers/platform/x86/toshiba_acpi.c  | 4 ++--
>  include/acpi/battery.h               | 4 ++--
>  8 files changed, 18 insertions(+), 18 deletions(-)
> 
> diff --git a/drivers/acpi/battery.c b/drivers/acpi/battery.c
> index 306513fec1e1..9482b0b6eadc 100644
> --- a/drivers/acpi/battery.c
> +++ b/drivers/acpi/battery.c
> @@ -696,7 +696,7 @@ static void __battery_hook_unregister(struct acpi_battery_hook *hook, int lock)
>  	if (lock)
>  		mutex_lock(&hook_mutex);
>  	list_for_each_entry(battery, &acpi_battery_list, list) {
> -		hook->remove_battery(battery->bat);
> +		hook->remove_battery(battery->bat, hook);
>  	}
>  	list_del(&hook->list);
>  	if (lock)
> @@ -724,7 +724,7 @@ void battery_hook_register(struct acpi_battery_hook *hook)
>  	 * its attributes.
>  	 */
>  	list_for_each_entry(battery, &acpi_battery_list, list) {
> -		if (hook->add_battery(battery->bat)) {
> +		if (hook->add_battery(battery->bat, hook)) {
>  			/*
>  			 * If a add-battery returns non-zero,
>  			 * the registration of the extension has failed,
> @@ -762,7 +762,7 @@ static void battery_hook_add_battery(struct acpi_battery *battery)
>  	 * during the battery module initialization.
>  	 */
>  	list_for_each_entry_safe(hook_node, tmp, &battery_hook_list, list) {
> -		if (hook_node->add_battery(battery->bat)) {
> +		if (hook_node->add_battery(battery->bat, hook_node)) {
>  			/*
>  			 * The notification of the extensions has failed, to
>  			 * prevent further errors we will unload the extension.
> @@ -785,7 +785,7 @@ static void battery_hook_remove_battery(struct acpi_battery *battery)
>  	 * custom attributes from the battery.
>  	 */
>  	list_for_each_entry(hook, &battery_hook_list, list) {
> -		hook->remove_battery(battery->bat);
> +		hook->remove_battery(battery->bat, hook);
>  	}
>  	/* Then, just remove the battery from the list */
>  	list_del(&battery->list);
> diff --git a/drivers/platform/x86/asus-wmi.c b/drivers/platform/x86/asus-wmi.c
> index ae46af731de9..446669d11095 100644
> --- a/drivers/platform/x86/asus-wmi.c
> +++ b/drivers/platform/x86/asus-wmi.c
> @@ -882,7 +882,7 @@ static ssize_t charge_control_end_threshold_show(struct device *device,
> 
>  static DEVICE_ATTR_RW(charge_control_end_threshold);
> 
> -static int asus_wmi_battery_add(struct power_supply *battery)
> +static int asus_wmi_battery_add(struct power_supply *battery, struct acpi_battery_hook *hook)
>  {
>  	/* The WMI method does not provide a way to specific a battery, so we
>  	 * just assume it is the first battery.
> @@ -909,7 +909,7 @@ static int asus_wmi_battery_add(struct power_supply *battery)
>  	return 0;
>  }
> 
> -static int asus_wmi_battery_remove(struct power_supply *battery)
> +static int asus_wmi_battery_remove(struct power_supply *battery, struct acpi_battery_hook *hook)
>  {
>  	device_remove_file(&battery->dev,
>  			   &dev_attr_charge_control_end_threshold);
> diff --git a/drivers/platform/x86/huawei-wmi.c b/drivers/platform/x86/huawei-wmi.c
> index eac3e6b4ea11..1dec4427053a 100644
> --- a/drivers/platform/x86/huawei-wmi.c
> +++ b/drivers/platform/x86/huawei-wmi.c
> @@ -468,7 +468,7 @@ static DEVICE_ATTR_RW(charge_control_start_threshold);
>  static DEVICE_ATTR_RW(charge_control_end_threshold);
>  static DEVICE_ATTR_RW(charge_control_thresholds);
> 
> -static int huawei_wmi_battery_add(struct power_supply *battery)
> +static int huawei_wmi_battery_add(struct power_supply *battery, struct acpi_battery_hook *hook)
>  {
>  	int err = 0;
> 
> @@ -483,7 +483,7 @@ static int huawei_wmi_battery_add(struct power_supply *battery)
>  	return err;
>  }
> 
> -static int huawei_wmi_battery_remove(struct power_supply *battery)
> +static int huawei_wmi_battery_remove(struct power_supply *battery, struct acpi_battery_hook *hook)
>  {
>  	device_remove_file(&battery->dev, &dev_attr_charge_control_start_threshold);
>  	device_remove_file(&battery->dev, &dev_attr_charge_control_end_threshold);
> diff --git a/drivers/platform/x86/lg-laptop.c b/drivers/platform/x86/lg-laptop.c
> index 332868b140ed..d662b64b0ba9 100644
> --- a/drivers/platform/x86/lg-laptop.c
> +++ b/drivers/platform/x86/lg-laptop.c
> @@ -546,7 +546,7 @@ static DEVICE_ATTR_RW(fn_lock);
>  static DEVICE_ATTR_RW(charge_control_end_threshold);
>  static DEVICE_ATTR_RW(battery_care_limit);
> 
> -static int lg_battery_add(struct power_supply *battery)
> +static int lg_battery_add(struct power_supply *battery, struct acpi_battery_hook *hook)
>  {
>  	if (device_create_file(&battery->dev,
>  			       &dev_attr_charge_control_end_threshold))
> @@ -555,7 +555,7 @@ static int lg_battery_add(struct power_supply *battery)
>  	return 0;
>  }
> 
> -static int lg_battery_remove(struct power_supply *battery)
> +static int lg_battery_remove(struct power_supply *battery, struct acpi_battery_hook *hook)
>  {
>  	device_remove_file(&battery->dev,
>  			   &dev_attr_charge_control_end_threshold);
> diff --git a/drivers/platform/x86/system76_acpi.c b/drivers/platform/x86/system76_acpi.c
> index 958df41ad509..9031bd53253f 100644
> --- a/drivers/platform/x86/system76_acpi.c
> +++ b/drivers/platform/x86/system76_acpi.c
> @@ -254,7 +254,7 @@ static struct attribute *system76_battery_attrs[] = {
> 
>  ATTRIBUTE_GROUPS(system76_battery);
> 
> -static int system76_battery_add(struct power_supply *battery)
> +static int system76_battery_add(struct power_supply *battery, struct acpi_battery_hook *hook)
>  {
>  	// System76 EC only supports 1 battery
>  	if (strcmp(battery->desc->name, "BAT0") != 0)
> @@ -266,7 +266,7 @@ static int system76_battery_add(struct power_supply *battery)
>  	return 0;
>  }
> 
> -static int system76_battery_remove(struct power_supply *battery)
> +static int system76_battery_remove(struct power_supply *battery, struct acpi_battery_hook *hook)
>  {
>  	device_remove_groups(&battery->dev, system76_battery_groups);
>  	return 0;
> diff --git a/drivers/platform/x86/thinkpad_acpi.c b/drivers/platform/x86/thinkpad_acpi.c
> index 8fbe21ebcc52..75ba9e61264e 100644
> --- a/drivers/platform/x86/thinkpad_acpi.c
> +++ b/drivers/platform/x86/thinkpad_acpi.c
> @@ -9897,7 +9897,7 @@ ATTRIBUTE_GROUPS(tpacpi_battery);
> 
>  /* ACPI battery hooking */
> 
> -static int tpacpi_battery_add(struct power_supply *battery)
> +static int tpacpi_battery_add(struct power_supply *battery, struct acpi_battery_hook *hook)
>  {
>  	int batteryid = tpacpi_battery_get_id(battery->desc->name);
> 
> @@ -9908,7 +9908,7 @@ static int tpacpi_battery_add(struct power_supply *battery)
>  	return 0;
>  }
> 
> -static int tpacpi_battery_remove(struct power_supply *battery)
> +static int tpacpi_battery_remove(struct power_supply *battery, struct acpi_battery_hook *hook)
>  {
>  	device_remove_groups(&battery->dev, tpacpi_battery_groups);
>  	return 0;
> diff --git a/drivers/platform/x86/toshiba_acpi.c b/drivers/platform/x86/toshiba_acpi.c
> index 43cc25351aea..c8f01f8f435d 100644
> --- a/drivers/platform/x86/toshiba_acpi.c
> +++ b/drivers/platform/x86/toshiba_acpi.c
> @@ -3113,7 +3113,7 @@ static struct attribute *toshiba_acpi_battery_attrs[] = {
> 
>  ATTRIBUTE_GROUPS(toshiba_acpi_battery);
> 
> -static int toshiba_acpi_battery_add(struct power_supply *battery)
> +static int toshiba_acpi_battery_add(struct power_supply *battery, struct acpi_battery_hook *hook)
>  {
>  	if (toshiba_acpi == NULL) {
>  		pr_err("Init order issue\n");
> @@ -3126,7 +3126,7 @@ static int toshiba_acpi_battery_add(struct power_supply *battery)
>  	return 0;
>  }
> 
> -static int toshiba_acpi_battery_remove(struct power_supply *battery)
> +static int toshiba_acpi_battery_remove(struct power_supply *battery, struct acpi_battery_hook *hook)
>  {
>  	device_remove_groups(&battery->dev, toshiba_acpi_battery_groups);
>  	return 0;
> diff --git a/include/acpi/battery.h b/include/acpi/battery.h
> index b8d56b702c7a..611a2561a014 100644
> --- a/include/acpi/battery.h
> +++ b/include/acpi/battery.h
> @@ -12,8 +12,8 @@
> 
>  struct acpi_battery_hook {
>  	const char *name;
> -	int (*add_battery)(struct power_supply *battery);
> -	int (*remove_battery)(struct power_supply *battery);
> +	int (*add_battery)(struct power_supply *battery, struct acpi_battery_hook *hook);
> +	int (*remove_battery)(struct power_supply *battery, struct acpi_battery_hook *hook);
>  	struct list_head list;
>  };
> 
> --
> 2.30.2
> 

[PATCH v2 2/2] platform/x86: dell: Add new dell-wmi-ddv driver

[Armin Wolf]

The dell-wmi-ddv driver adds support for reading
the current temperature and ePPID of ACPI batteries
on supported Dell machines.

Since the WMI interface used by this driver does not
do any input validation and thus cannot be used for probing,
the driver depends on the ACPI battery extension machanism
to discover batteries.

The driver also supports a debugfs interface for retrieving
buffers containing fan and thermal sensor information.
Since the meaing of the content of those buffers is currently
unknown, the interface is meant for reverse-engineering and
will likely be replaced with an hwmon interface once the
meaning has been understood.

The driver was tested on a Dell Inspiron 3505.

Signed-off-by: Armin Wolf <W_Armin@gmx.de>
---
 .../ABI/testing/debugfs-dell-wmi-ddv          |  21 +
 .../ABI/testing/sysfs-platform-dell-wmi-ddv   |   7 +
 MAINTAINERS                                   |   7 +
 drivers/platform/x86/dell/Kconfig             |  13 +
 drivers/platform/x86/dell/Makefile            |   1 +
 drivers/platform/x86/dell/dell-wmi-ddv.c      | 361 ++++++++++++++++++
 drivers/platform/x86/wmi.c                    |   1 +
 7 files changed, 411 insertions(+)
 create mode 100644 Documentation/ABI/testing/debugfs-dell-wmi-ddv
 create mode 100644 Documentation/ABI/testing/sysfs-platform-dell-wmi-ddv
 create mode 100644 drivers/platform/x86/dell/dell-wmi-ddv.c

diff --git a/Documentation/ABI/testing/debugfs-dell-wmi-ddv b/Documentation/ABI/testing/debugfs-dell-wmi-ddv
new file mode 100644
index 000000000000..fbcc5d6f7388
--- /dev/null
+++ b/Documentation/ABI/testing/debugfs-dell-wmi-ddv
@@ -0,0 +1,21 @@
+What:		/sys/kernel/debug/dell-wmi-ddv-<wmi_device_name>/fan_sensor_information
+Date:		September 2022
+KernelVersion:	6.1
+Contact:	Armin Wolf <W_Armin@gmx.de>
+Description:
+		This file contains the contents of the fan sensor information buffer,
+		which contains fan sensor entries and a terminating character (0xFF).
+
+		Each fan sensor entry consists of three bytes with an unknown meaning,
+		interested people may use this file for reverse-engineering.
+
+What:		/sys/kernel/debug/dell-wmi-ddv-<wmi_device_name>/thermal_sensor_information
+Date:		September 2022
+KernelVersion:	6.1
+Contact:	Armin Wolf <W_Armin@gmx.de>
+Description:
+		This file contains the contents of the thermal sensor information buffer,
+		which contains thermal sensor entries and a terminating character (0xFF).
+
+		Each thermal sensor entry consists of five bytes with an unknown meaning,
+		interested people may use this file for reverse-engineering.
diff --git a/Documentation/ABI/testing/sysfs-platform-dell-wmi-ddv b/Documentation/ABI/testing/sysfs-platform-dell-wmi-ddv
new file mode 100644
index 000000000000..1d97ad615c66
--- /dev/null
+++ b/Documentation/ABI/testing/sysfs-platform-dell-wmi-ddv
@@ -0,0 +1,7 @@
+What:		/sys/class/power_supply/<battery_name>/eppid
+Date:		September 2022
+KernelVersion:	6.1
+Contact:	Armin Wolf <W_Armin@gmx.de>
+Description:
+		Reports the Dell ePPID (electronic Dell Piece Part Identification)
+		of the ACPI battery.
diff --git a/MAINTAINERS b/MAINTAINERS
index 6bb894ea4a77..d9fd4c9eebbc 100644
--- a/MAINTAINERS
+++ b/MAINTAINERS
@@ -5821,6 +5821,13 @@ L:	Dell.Client.Kernel@dell.com
 S:	Maintained
 F:	drivers/platform/x86/dell/dell-wmi-descriptor.c

+DELL WMI DDV DRIVER
+M:	Armin Wolf <W_Armin@gmx.de>
+S:	Maintained
+F:	Documentation/ABI/testing/debugfs-dell-wmi-ddv
+F:	Documentation/ABI/testing/sysfs-platform-dell-wmi-ddv
+F:	drivers/platform/x86/dell/dell-wmi-ddv.c
+
 DELL WMI SYSMAN DRIVER
 M:	Divya Bharathi <divya.bharathi@dell.com>
 M:	Prasanth Ksr <prasanth.ksr@dell.com>
diff --git a/drivers/platform/x86/dell/Kconfig b/drivers/platform/x86/dell/Kconfig
index 25421e061c47..d319de8f2132 100644
--- a/drivers/platform/x86/dell/Kconfig
+++ b/drivers/platform/x86/dell/Kconfig
@@ -189,6 +189,19 @@ config DELL_WMI_DESCRIPTOR
 	default n
 	depends on ACPI_WMI

+config DELL_WMI_DDV
+	tristate "Dell WMI sensors Support"
+	default m
+	depends on ACPI_BATTERY
+	depends on ACPI_WMI
+	help
+	  This option adds support for WMI-based sensors like
+	  battery temperature sensors found on some Dell notebooks.
+	  It also supports reading of the battery ePPID.
+
+	  To compile this drivers as a module, choose M here: the module will
+	  be called dell-wmi-ddv.
+
 config DELL_WMI_LED
 	tristate "External LED on Dell Business Netbooks"
 	default m
diff --git a/drivers/platform/x86/dell/Makefile b/drivers/platform/x86/dell/Makefile
index ddba1df71e80..1b8942426622 100644
--- a/drivers/platform/x86/dell/Makefile
+++ b/drivers/platform/x86/dell/Makefile
@@ -19,5 +19,6 @@ dell-wmi-objs				:= dell-wmi-base.o
 dell-wmi-$(CONFIG_DELL_WMI_PRIVACY)	+= dell-wmi-privacy.o
 obj-$(CONFIG_DELL_WMI_AIO)		+= dell-wmi-aio.o
 obj-$(CONFIG_DELL_WMI_DESCRIPTOR)	+= dell-wmi-descriptor.o
+obj-$(CONFIG_DELL_WMI_DDV)		+= dell-wmi-ddv.o
 obj-$(CONFIG_DELL_WMI_LED)		+= dell-wmi-led.o
 obj-$(CONFIG_DELL_WMI_SYSMAN)		+= dell-wmi-sysman/
diff --git a/drivers/platform/x86/dell/dell-wmi-ddv.c b/drivers/platform/x86/dell/dell-wmi-ddv.c
new file mode 100644
index 000000000000..6ccce90f475d
--- /dev/null
+++ b/drivers/platform/x86/dell/dell-wmi-ddv.c
@@ -0,0 +1,361 @@
+// SPDX-License-Identifier: GPL-2.0-or-later
+/*
+ * dell-wmi-ddv.c -- Linux driver for WMI sensor information on Dell notebooks.
+ *
+ * Copyright (C) 2022 Armin Wolf <W_Armin@gmx.de>
+ */
+
+#define pr_format(fmt) KBUILD_MODNAME ": " fmt
+
+#include <acpi/battery.h>
+#include <linux/acpi.h>
+#include <linux/debugfs.h>
+#include <linux/device.h>
+#include <linux/kernel.h>
+#include <linux/kstrtox.h>
+#include <linux/math.h>
+#include <linux/module.h>
+#include <linux/limits.h>
+#include <linux/power_supply.h>
+#include <linux/seq_file.h>
+#include <linux/sysfs.h>
+#include <linux/wmi.h>
+
+#define DRIVER_NAME	"dell-wmi-ddv"
+
+#define DELL_DDV_SUPPORTED_INTERFACE 2
+#define DELL_DDV_GUID	"8A42EA14-4F2A-FD45-6422-0087F7A7E608"
+
+enum dell_ddv_method {
+	DELL_DDV_BATTERY_DESIGN_CAPACITY	= 0x01,
+	DELL_DDV_BATTERY_FULL_CHARGE_CAPACITY	= 0x02,
+	DELL_DDV_BATTERY_MANUFACTURE_NAME	= 0x03,
+	DELL_DDV_BATTERY_MANUFACTURE_DATE	= 0x04,
+	DELL_DDV_BATTERY_SERIAL_NUMBER		= 0x05,
+	DELL_DDV_BATTERY_CHEMISTRY_VALUE	= 0x06,
+	DELL_DDV_BATTERY_TEMPERATURE		= 0x07,
+	DELL_DDV_BATTERY_CURRENT		= 0x08,
+	DELL_DDV_BATTERY_VOLTAGE		= 0x09,
+	DELL_DDV_BATTERY_MANUFACTURER_ACCESS	= 0x0A,
+	DELL_DDV_BATTERY_RELATIVE_CHARGE_STATE	= 0x0B,
+	DELL_DDV_BATTERY_CYCLE_COUNT		= 0x0C,
+	DELL_DDV_BATTERY_EPPID			= 0x0D,
+	DELL_DDV_BATTERY_RAW_ANALYTICS_START	= 0x0E,
+	DELL_DDV_BATTERY_RAW_ANALYTICS		= 0x0F,
+	DELL_DDV_BATTERY_DESIGN_VOLTAGE		= 0x10,
+
+	DELL_DDV_INTERFACE_VERSION		= 0x12,
+
+	DELL_DDV_FAN_SENSOR_INFORMATION		= 0x20,
+	DELL_DDV_THERMAL_SENSOR_INFORMATION	= 0x22,
+};
+
+struct dell_wmi_ddv_data {
+	struct acpi_battery_hook hook;
+	struct device_attribute temp_attr, eppid_attr;
+	struct wmi_device *wdev;
+};
+
+static int dell_wmi_ddv_query_type(struct wmi_device *wdev, enum dell_ddv_method method, u32 arg,
+				   union acpi_object **result, acpi_object_type type)
+{
+	struct acpi_buffer out = { ACPI_ALLOCATE_BUFFER, NULL };
+	const struct acpi_buffer in = {
+		.length = sizeof(arg),
+		.pointer = &arg,
+	};
+	union acpi_object *obj;
+	acpi_status ret;
+
+	ret = wmidev_evaluate_method(wdev, 0x0, method, &in, &out);
+	if (ACPI_FAILURE(ret))
+		return -EIO;
+
+	obj = out.pointer;
+	if (!obj)
+		return -ENODATA;
+
+	if (obj->type != type) {
+		kfree(obj);
+		return -EIO;
+	}
+
+	*result = obj;
+
+	return 0;
+}
+
+static int dell_wmi_ddv_query_integer(struct wmi_device *wdev, enum dell_ddv_method method,
+				      u32 arg, u32 *res)
+{
+	union acpi_object *obj;
+	int ret;
+
+	ret = dell_wmi_ddv_query_type(wdev, method, arg, &obj, ACPI_TYPE_INTEGER);
+	if (ret < 0)
+		return ret;
+
+	if (obj->integer.value <= U32_MAX)
+		*res = (u32)obj->integer.value;
+	else
+		ret = -ERANGE;
+
+	kfree(obj);
+
+	return ret;
+}
+
+static int dell_wmi_ddv_query_buffer(struct wmi_device *wdev, enum dell_ddv_method method,
+				     u32 arg, union acpi_object **result)
+{
+	union acpi_object *obj;
+	u64 buffer_size;
+	int ret;
+
+	ret = dell_wmi_ddv_query_type(wdev, method, arg, &obj, ACPI_TYPE_PACKAGE);
+	if (ret < 0)
+		return ret;
+
+	if (obj->package.count != 2)
+		goto err_free;
+
+	if (obj->package.elements[0].type != ACPI_TYPE_INTEGER)
+		goto err_free;
+
+	buffer_size = obj->package.elements[0].integer.value;
+
+	if (obj->package.elements[1].type != ACPI_TYPE_BUFFER)
+		goto err_free;
+
+	if (buffer_size != obj->package.elements[1].buffer.length) {
+		dev_warn(&wdev->dev,
+			 FW_WARN "ACPI buffer size (%llu) does not match WMI buffer size (%d)\n",
+			 buffer_size, obj->package.elements[1].buffer.length);
+
+		goto err_free;
+	}
+
+	*result = obj;
+
+	return 0;
+
+err_free:
+	kfree(obj);
+
+	return -EIO;
+}
+
+static int dell_wmi_ddv_query_string(struct wmi_device *wdev, enum dell_ddv_method method,
+				     u32 arg, union acpi_object **result)
+{
+	return dell_wmi_ddv_query_type(wdev, method, arg, result, ACPI_TYPE_STRING);
+}
+
+static int dell_wmi_ddv_battery_index(struct acpi_device *acpi_dev, u32 *index)
+{
+	const char *uid_str = acpi_device_uid(acpi_dev);
+
+	if (!uid_str)
+		return -ENODEV;
+
+	return kstrtou32(uid_str, 10, index);
+}
+
+static ssize_t temp_show(struct device *dev, struct device_attribute *attr, char *buf)
+{
+	struct dell_wmi_ddv_data *data = container_of(attr, struct dell_wmi_ddv_data, temp_attr);
+	u32 index, value;
+	int ret;
+
+	ret = dell_wmi_ddv_battery_index(to_acpi_device(dev->parent), &index);
+	if (ret < 0)
+		return ret;
+
+	ret = dell_wmi_ddv_query_integer(data->wdev, DELL_DDV_BATTERY_TEMPERATURE, index, &value);
+	if (ret < 0)
+		return ret;
+
+	return sysfs_emit(buf, "%d\n", DIV_ROUND_CLOSEST(value, 10));
+}
+
+static ssize_t eppid_show(struct device *dev, struct device_attribute *attr, char *buf)
+{
+	struct dell_wmi_ddv_data *data = container_of(attr, struct dell_wmi_ddv_data, eppid_attr);
+	union acpi_object *obj;
+	u32 index;
+	int ret;
+
+	ret = dell_wmi_ddv_battery_index(to_acpi_device(dev->parent), &index);
+	if (ret < 0)
+		return ret;
+
+	ret = dell_wmi_ddv_query_string(data->wdev, DELL_DDV_BATTERY_EPPID, index, &obj);
+	if (ret < 0)
+		return ret;
+
+	ret = sysfs_emit(buf, "%s\n", obj->string.pointer);
+
+	kfree(obj);
+
+	return ret;
+}
+
+static int dell_wmi_ddv_add_battery(struct power_supply *battery, struct acpi_battery_hook *hook)
+{
+	struct dell_wmi_ddv_data *data = container_of(hook, struct dell_wmi_ddv_data, hook);
+	u32 index;
+	int ret;
+
+	/* Return 0 instead of error to avoid being unloaded */
+	ret = dell_wmi_ddv_battery_index(to_acpi_device(battery->dev.parent), &index);
+	if (ret < 0)
+		return 0;
+
+	ret = device_create_file(&battery->dev, &data->temp_attr);
+	if (ret < 0)
+		return ret;
+
+	ret = device_create_file(&battery->dev, &data->eppid_attr);
+	if (ret < 0) {
+		device_remove_file(&battery->dev, &data->temp_attr);
+
+		return ret;
+	}
+
+	return 0;
+}
+
+static int dell_wmi_ddv_remove_battery(struct power_supply *battery, struct acpi_battery_hook *hook)
+{
+	struct dell_wmi_ddv_data *data = container_of(hook, struct dell_wmi_ddv_data, hook);
+
+	device_remove_file(&battery->dev, &data->temp_attr);
+	device_remove_file(&battery->dev, &data->eppid_attr);
+
+	return 0;
+}
+
+static void dell_wmi_ddv_battery_remove(void *data)
+{
+	struct acpi_battery_hook *hook = data;
+
+	battery_hook_unregister(hook);
+}
+
+static int dell_wmi_ddv_battery_add(struct dell_wmi_ddv_data *data)
+{
+	data->hook.name = "Dell DDV Battery Extension";
+	data->hook.add_battery = dell_wmi_ddv_add_battery;
+	data->hook.remove_battery = dell_wmi_ddv_remove_battery;
+
+	sysfs_attr_init(&data->temp_attr.attr);
+	data->temp_attr.attr.name = "temp";
+	data->temp_attr.attr.mode = 0444;
+	data->temp_attr.show = temp_show;
+
+	sysfs_attr_init(&data->eppid_attr.attr);
+	data->eppid_attr.attr.name = "eppid";
+	data->eppid_attr.attr.mode = 0444;
+	data->eppid_attr.show = eppid_show;
+
+	battery_hook_register(&data->hook);
+
+	return devm_add_action_or_reset(&data->wdev->dev, dell_wmi_ddv_battery_remove, &data->hook);
+}
+
+static int dell_wmi_ddv_buffer_read(struct seq_file *seq, enum dell_ddv_method method)
+{
+	struct device *dev = seq->private;
+	struct dell_wmi_ddv_data *data = dev_get_drvdata(dev);
+	union acpi_object *obj;
+	union acpi_object buf;
+	int ret;
+
+	ret = dell_wmi_ddv_query_buffer(data->wdev, method, 0, &obj);
+	if (ret < 0)
+		return ret;
+
+	buf = obj->package.elements[1];
+	ret = seq_write(seq, buf.buffer.pointer, buf.buffer.length);
+	kfree(obj);
+
+	return ret;
+}
+
+static int dell_wmi_ddv_fan_read(struct seq_file *seq, void *offset)
+{
+	return dell_wmi_ddv_buffer_read(seq, DELL_DDV_FAN_SENSOR_INFORMATION);
+}
+
+static int dell_wmi_ddv_temp_read(struct seq_file *seq, void *offset)
+{
+	return dell_wmi_ddv_buffer_read(seq, DELL_DDV_THERMAL_SENSOR_INFORMATION);
+}
+
+static void dell_wmi_ddv_debugfs_remove(void *data)
+{
+	struct dentry *entry = data;
+
+	debugfs_remove(entry);
+}
+
+static void dell_wmi_ddv_debugfs_init(struct wmi_device *wdev)
+{
+	struct dentry *entry;
+	char name[64];
+
+	scnprintf(name, ARRAY_SIZE(name), "%s-%s", DRIVER_NAME, dev_name(&wdev->dev));
+	entry = debugfs_create_dir(name, NULL);
+
+	debugfs_create_devm_seqfile(&wdev->dev, "fan_sensor_information", entry,
+				    dell_wmi_ddv_fan_read);
+	debugfs_create_devm_seqfile(&wdev->dev, "thermal_sensor_information", entry,
+				    dell_wmi_ddv_temp_read);
+
+	devm_add_action_or_reset(&wdev->dev, dell_wmi_ddv_debugfs_remove, entry);
+}
+
+static int dell_wmi_ddv_probe(struct wmi_device *wdev, const void *context)
+{
+	struct dell_wmi_ddv_data *data;
+	u32 version;
+	int ret;
+
+	ret = dell_wmi_ddv_query_integer(wdev, DELL_DDV_INTERFACE_VERSION, 0, &version);
+	if (ret < 0)
+		return ret;
+
+	dev_dbg(&wdev->dev, "WMI interface version: %d\n", version);
+	if (version != DELL_DDV_SUPPORTED_INTERFACE)
+		return -ENODEV;
+
+	data = devm_kzalloc(&wdev->dev, sizeof(*data), GFP_KERNEL);
+	if (!data)
+		return -ENOMEM;
+
+	dev_set_drvdata(&wdev->dev, data);
+	data->wdev = wdev;
+
+	dell_wmi_ddv_debugfs_init(wdev);
+
+	return dell_wmi_ddv_battery_add(data);
+}
+
+static const struct wmi_device_id dell_wmi_ddv_id_table[] = {
+	{ DELL_DDV_GUID, NULL },
+	{ }
+};
+MODULE_DEVICE_TABLE(wmi, dell_wmi_ddv_id_table);
+
+static struct wmi_driver dell_wmi_ddv_driver = {
+	.driver = {
+		.name = DRIVER_NAME,
+	},
+	.id_table = dell_wmi_ddv_id_table,
+	.probe = dell_wmi_ddv_probe,
+};
+module_wmi_driver(dell_wmi_ddv_driver);
+
+MODULE_AUTHOR("Armin Wolf <W_Armin@gmx.de>");
+MODULE_DESCRIPTION("Dell WMI sensor driver");
+MODULE_LICENSE("GPL");
diff --git a/drivers/platform/x86/wmi.c b/drivers/platform/x86/wmi.c
index aff23309b5d3..f307d8c5c6c3 100644
--- a/drivers/platform/x86/wmi.c
+++ b/drivers/platform/x86/wmi.c
@@ -108,6 +108,7 @@ MODULE_DEVICE_TABLE(acpi, wmi_device_ids);
 /* allow duplicate GUIDs as these device drivers use struct wmi_driver */
 static const char * const allow_duplicates[] = {
 	"05901221-D566-11D1-B2F0-00A0C9062910",	/* wmi-bmof */
+	"8A42EA14-4F2A-FD45-6422-0087F7A7E608",	/* dell-wmi-ddv */
 	NULL
 };

--
2.30.2

Re: [PATCH v2 2/2] platform/x86: dell: Add new dell-wmi-ddv driver

[Andy Shevchenko]

On Tue, Sep 27, 2022 at 10:45:21PM +0200, Armin Wolf wrote:
> The dell-wmi-ddv driver adds support for reading
> the current temperature and ePPID of ACPI batteries
> on supported Dell machines.
> 
> Since the WMI interface used by this driver does not
> do any input validation and thus cannot be used for probing,
> the driver depends on the ACPI battery extension machanism
> to discover batteries.
> 
> The driver also supports a debugfs interface for retrieving
> buffers containing fan and thermal sensor information.
> Since the meaing of the content of those buffers is currently
> unknown, the interface is meant for reverse-engineering and
> will likely be replaced with an hwmon interface once the
> meaning has been understood.
> 
> The driver was tested on a Dell Inspiron 3505.

...

> +config DELL_WMI_DDV
> +	tristate "Dell WMI sensors Support"

> +	default m

Why? (Imagine I have Dell, but old machine)

(And yes, I see that other Kconfig options are using it, but we shall avoid
 cargo cult and each default choice like this has to be explained at least.)

...

> + * dell-wmi-ddv.c -- Linux driver for WMI sensor information on Dell notebooks.

Please, remove file name from the file. This will be an additional burden in
the future in case it will be renamed.

...

> +#include <acpi/battery.h>

Is it required to be the first? Otherwise it seems ACPI specific to me and the
general rule is to put inclusions from generic towards custom. I.o.w. can you
move it after linux/wmi.h with a blank line in between?

> +#include <linux/acpi.h>
> +#include <linux/debugfs.h>
> +#include <linux/device.h>
> +#include <linux/kernel.h>
> +#include <linux/kstrtox.h>
> +#include <linux/math.h>
> +#include <linux/module.h>
> +#include <linux/limits.h>
> +#include <linux/power_supply.h>
> +#include <linux/seq_file.h>
> +#include <linux/sysfs.h>
> +#include <linux/wmi.h>

...

> +struct dell_wmi_ddv_data {
> +	struct acpi_battery_hook hook;
> +	struct device_attribute temp_attr, eppid_attr;

It's hard to read and easy to miss that the data type has two members here.
Please, put one member per one line.

> +	struct wmi_device *wdev;
> +};

...

> +	if (obj->type != type) {
> +		kfree(obj);
> +		return -EIO;

EINVAL?

> +	}

...

> +	kfree(obj);

I'm wondering what is the best to use in the drivers:
 1) kfree()
 2) acpi_os_free()
 3) ACPI_FREE()

?

...

> +static int dell_wmi_ddv_battery_index(struct acpi_device *acpi_dev, u32 *index)
> +{

> +	const char *uid_str = acpi_device_uid(acpi_dev);
> +
> +	if (!uid_str)
> +		return -ENODEV;

It will be better for maintaining to have

	const char *uid_str...;

	uid_str = ...
	if (!uid_str)
		...

> +	return kstrtou32(uid_str, 10, index);
> +}

...

> +	/* Return 0 instead of error to avoid being unloaded */
> +	ret = dell_wmi_ddv_battery_index(to_acpi_device(battery->dev.parent), &index);
> +	if (ret < 0)
> +		return 0;

How index is used?

...

> +	ret = device_create_file(&battery->dev, &data->temp_attr);
> +	if (ret < 0)
> +		return ret;
> +
> +	ret = device_create_file(&battery->dev, &data->eppid_attr);
> +	if (ret < 0) {
> +		device_remove_file(&battery->dev, &data->temp_attr);
> +
> +		return ret;
> +	}

Why dev_groups member can't be utilized?

...

> +static void dell_wmi_ddv_debugfs_init(struct wmi_device *wdev)

Strictly speaking this should return int (see below).

> +{
> +	struct dentry *entry;
> +	char name[64];
> +
> +	scnprintf(name, ARRAY_SIZE(name), "%s-%s", DRIVER_NAME, dev_name(&wdev->dev));
> +	entry = debugfs_create_dir(name, NULL);
> +
> +	debugfs_create_devm_seqfile(&wdev->dev, "fan_sensor_information", entry,
> +				    dell_wmi_ddv_fan_read);
> +	debugfs_create_devm_seqfile(&wdev->dev, "thermal_sensor_information", entry,
> +				    dell_wmi_ddv_temp_read);
> +
> +	devm_add_action_or_reset(&wdev->dev, dell_wmi_ddv_debugfs_remove, entry);

return devm...

This is not related to debugfs and there is no rule to avoid checking error
codes from devm_add_action_or_reset().

> +}

...

> +static struct wmi_driver dell_wmi_ddv_driver = {
> +	.driver = {
> +		.name = DRIVER_NAME,

I would use explicit literal since this is a (semi-) ABI, and having it as
a define feels not fully right.

> +	},
> +	.id_table = dell_wmi_ddv_id_table,
> +	.probe = dell_wmi_ddv_probe,
> +};

-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH v2 2/2] platform/x86: dell: Add new dell-wmi-ddv driver

[Hans de Goede]

Hi,

On 9/28/22 12:47, Andy Shevchenko wrote:
> On Tue, Sep 27, 2022 at 10:45:21PM +0200, Armin Wolf wrote:
>> The dell-wmi-ddv driver adds support for reading
>> the current temperature and ePPID of ACPI batteries
>> on supported Dell machines.
>>
>> Since the WMI interface used by this driver does not
>> do any input validation and thus cannot be used for probing,
>> the driver depends on the ACPI battery extension machanism
>> to discover batteries.
>>
>> The driver also supports a debugfs interface for retrieving
>> buffers containing fan and thermal sensor information.
>> Since the meaing of the content of those buffers is currently
>> unknown, the interface is meant for reverse-engineering and
>> will likely be replaced with an hwmon interface once the
>> meaning has been understood.
>>
>> The driver was tested on a Dell Inspiron 3505.
> 
> ...
> 
>> +config DELL_WMI_DDV
>> +	tristate "Dell WMI sensors Support"
> 
>> +	default m
> 
> Why? (Imagine I have Dell, but old machine)

Then you can select N if you really want to.

> (And yes, I see that other Kconfig options are using it, but we shall avoid
>  cargo cult and each default choice like this has to be explained at least.)

This has been discussed during the review of v1 already.

There are quite a few dell modules and the choice has
been made to put these all behind a dell platform drivers
options and then default all the individual modules to 'm'.

> ...
> 
>> + * dell-wmi-ddv.c -- Linux driver for WMI sensor information on Dell notebooks.
> 
> Please, remove file name from the file. This will be an additional burden in
> the future in case it will be renamed.
> 
> ...
> 
>> +#include <acpi/battery.h>
> 
> Is it required to be the first? Otherwise it seems ACPI specific to me and the
> general rule is to put inclusions from generic towards custom. I.o.w. can you
> move it after linux/wmi.h with a blank line in between?
> 
>> +#include <linux/acpi.h>
>> +#include <linux/debugfs.h>
>> +#include <linux/device.h>
>> +#include <linux/kernel.h>
>> +#include <linux/kstrtox.h>
>> +#include <linux/math.h>
>> +#include <linux/module.h>
>> +#include <linux/limits.h>
>> +#include <linux/power_supply.h>
>> +#include <linux/seq_file.h>
>> +#include <linux/sysfs.h>
>> +#include <linux/wmi.h>
> 
> ...
> 
>> +struct dell_wmi_ddv_data {
>> +	struct acpi_battery_hook hook;
>> +	struct device_attribute temp_attr, eppid_attr;
> 
> It's hard to read and easy to miss that the data type has two members here.
> Please, put one member per one line.
> 
>> +	struct wmi_device *wdev;
>> +};
> 
> ...
> 
>> +	if (obj->type != type) {
>> +		kfree(obj);
>> +		return -EIO;
> 
> EINVAL?
> 
>> +	}
> 
> ...
> 
>> +	kfree(obj);
> 
> I'm wondering what is the best to use in the drivers:
>  1) kfree()
>  2) acpi_os_free()
>  3) ACPI_FREE()
> 
> ?

Most ACPI driver code I know of just uses kfree() the other 2
are more ACPI-core / ACPICA internal helpers.


> 
> ...
> 
>> +static int dell_wmi_ddv_battery_index(struct acpi_device *acpi_dev, u32 *index)
>> +{
> 
>> +	const char *uid_str = acpi_device_uid(acpi_dev);
>> +
>> +	if (!uid_str)
>> +		return -ENODEV;
> 
> It will be better for maintaining to have
> 
> 	const char *uid_str...;
> 
> 	uid_str = ...
> 	if (!uid_str)
> 		...
> 
>> +	return kstrtou32(uid_str, 10, index);
>> +}
> 
> ...
> 
>> +	/* Return 0 instead of error to avoid being unloaded */
>> +	ret = dell_wmi_ddv_battery_index(to_acpi_device(battery->dev.parent), &index);
>> +	if (ret < 0)
>> +		return 0;
> 
> How index is used?

index is used in the registered sysfs attr show functions,
so if this fails then the sysfs attr should not be registered.

> 
> ...
> 
>> +	ret = device_create_file(&battery->dev, &data->temp_attr);
>> +	if (ret < 0)
>> +		return ret;
>> +
>> +	ret = device_create_file(&battery->dev, &data->eppid_attr);
>> +	if (ret < 0) {
>> +		device_remove_file(&battery->dev, &data->temp_attr);
>> +
>> +		return ret;
>> +	}
> 
> Why dev_groups member can't be utilized?

Because this is an extension to the ACPI battery driver, IOW
this adds extra attributes to the power-supply-class device
registered by the ACPI battery driver. Note that the device
in this case is managed by the power-supply-class code, so
there is no access to dev_groups even in the ACPI battery code.

> 
> ...
> 
>> +static void dell_wmi_ddv_debugfs_init(struct wmi_device *wdev)
> 
> Strictly speaking this should return int (see below).
> 
>> +{
>> +	struct dentry *entry;
>> +	char name[64];
>> +
>> +	scnprintf(name, ARRAY_SIZE(name), "%s-%s", DRIVER_NAME, dev_name(&wdev->dev));
>> +	entry = debugfs_create_dir(name, NULL);
>> +
>> +	debugfs_create_devm_seqfile(&wdev->dev, "fan_sensor_information", entry,
>> +				    dell_wmi_ddv_fan_read);
>> +	debugfs_create_devm_seqfile(&wdev->dev, "thermal_sensor_information", entry,
>> +				    dell_wmi_ddv_temp_read);
>> +
>> +	devm_add_action_or_reset(&wdev->dev, dell_wmi_ddv_debugfs_remove, entry);
> 
> return devm...
> 
> This is not related to debugfs and there is no rule to avoid checking error
> codes from devm_add_action_or_reset().
> 
>> +}
> 
> ...
> 
>> +static struct wmi_driver dell_wmi_ddv_driver = {
>> +	.driver = {
>> +		.name = DRIVER_NAME,
> 
> I would use explicit literal since this is a (semi-) ABI, and having it as
> a define feels not fully right.
> 
>> +	},
>> +	.id_table = dell_wmi_ddv_id_table,
>> +	.probe = dell_wmi_ddv_probe,
>> +};
> 


Regards,

Hans

Re: [PATCH v2 2/2] platform/x86: dell: Add new dell-wmi-ddv driver

[Andy Shevchenko]

On Wed, Sep 28, 2022 at 01:33:53PM +0200, Hans de Goede wrote:
> On 9/28/22 12:47, Andy Shevchenko wrote:
> > On Tue, Sep 27, 2022 at 10:45:21PM +0200, Armin Wolf wrote:

...

> >> +	default m
> > 
> > Why? (Imagine I have Dell, but old machine)
> 
> Then you can select N if you really want to.
> 
> > (And yes, I see that other Kconfig options are using it, but we shall avoid
> >  cargo cult and each default choice like this has to be explained at least.)
> 
> This has been discussed during the review of v1 already.
> 
> There are quite a few dell modules and the choice has
> been made to put these all behind a dell platform drivers
> options and then default all the individual modules to 'm'.

Okay, thanks for pointing out that this was discussed. I was not aware.

...

> >> +	kfree(obj);
> > 
> > I'm wondering what is the best to use in the drivers:
> >  1) kfree()
> >  2) acpi_os_free()
> >  3) ACPI_FREE()

> > ?
> 
> Most ACPI driver code I know of just uses kfree() the other 2
> are more ACPI-core / ACPICA internal helpers.

To me 2) would look more consistent, esp. in case if it is extended in
the future.

...

> >> +	ret = device_create_file(&battery->dev, &data->temp_attr);
> >> +	if (ret < 0)
> >> +		return ret;
> >> +
> >> +	ret = device_create_file(&battery->dev, &data->eppid_attr);
> >> +	if (ret < 0) {
> >> +		device_remove_file(&battery->dev, &data->temp_attr);
> >> +
> >> +		return ret;
> >> +	}
> > 
> > Why dev_groups member can't be utilized?
> 
> Because this is an extension to the ACPI battery driver, IOW
> this adds extra attributes to the power-supply-class device
> registered by the ACPI battery driver. Note that the device
> in this case is managed by the power-supply-class code, so
> there is no access to dev_groups even in the ACPI battery code.

Ah, I see now, so we extend the attributes of the 3rd party driver here.

-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH v2 2/2] platform/x86: dell: Add new dell-wmi-ddv driver

[Armin Wolf]

Am 28.09.22 um 12:47 schrieb Andy Shevchenko:

> On Tue, Sep 27, 2022 at 10:45:21PM +0200, Armin Wolf wrote:
>> The dell-wmi-ddv driver adds support for reading
>> the current temperature and ePPID of ACPI batteries
>> on supported Dell machines.
>>
>> Since the WMI interface used by this driver does not
>> do any input validation and thus cannot be used for probing,
>> the driver depends on the ACPI battery extension machanism
>> to discover batteries.
>>
>> The driver also supports a debugfs interface for retrieving
>> buffers containing fan and thermal sensor information.
>> Since the meaing of the content of those buffers is currently
>> unknown, the interface is meant for reverse-engineering and
>> will likely be replaced with an hwmon interface once the
>> meaning has been understood.
>>
>> The driver was tested on a Dell Inspiron 3505.
> ...
>
>> +config DELL_WMI_DDV
>> +	tristate "Dell WMI sensors Support"
>> +	default m
> Why? (Imagine I have Dell, but old machine)
>
> (And yes, I see that other Kconfig options are using it, but we shall avoid
>   cargo cult and each default choice like this has to be explained at least.)
>
> ...
>
>> + * dell-wmi-ddv.c -- Linux driver for WMI sensor information on Dell notebooks.
> Please, remove file name from the file. This will be an additional burden in
> the future in case it will be renamed.
>
> ...
>
>> +#include <acpi/battery.h>
> Is it required to be the first? Otherwise it seems ACPI specific to me and the
> general rule is to put inclusions from generic towards custom. I.o.w. can you
> move it after linux/wmi.h with a blank line in between?
>
>> +#include <linux/acpi.h>
>> +#include <linux/debugfs.h>
>> +#include <linux/device.h>
>> +#include <linux/kernel.h>
>> +#include <linux/kstrtox.h>
>> +#include <linux/math.h>
>> +#include <linux/module.h>
>> +#include <linux/limits.h>
>> +#include <linux/power_supply.h>
>> +#include <linux/seq_file.h>
>> +#include <linux/sysfs.h>
>> +#include <linux/wmi.h>
> ...
>
>> +struct dell_wmi_ddv_data {
>> +	struct acpi_battery_hook hook;
>> +	struct device_attribute temp_attr, eppid_attr;
> It's hard to read and easy to miss that the data type has two members here.
> Please, put one member per one line.
>
>> +	struct wmi_device *wdev;
>> +};
> ...
>
>> +	if (obj->type != type) {
>> +		kfree(obj);
>> +		return -EIO;
> EINVAL?

In my opinion, EINVAL should be returned if the parameters are invalid.
In this case however, the error comes from the wmi device returning invalid
data, which would be represented better with EIO.

>> +	}
> ...
>
>> +	kfree(obj);
> I'm wondering what is the best to use in the drivers:
>   1) kfree()
>   2) acpi_os_free()
>   3) ACPI_FREE()
>
> ?
>
> ...
>
>> +static int dell_wmi_ddv_battery_index(struct acpi_device *acpi_dev, u32 *index)
>> +{
>> +	const char *uid_str = acpi_device_uid(acpi_dev);
>> +
>> +	if (!uid_str)
>> +		return -ENODEV;
> It will be better for maintaining to have
>
> 	const char *uid_str...;
>
> 	uid_str = ...
> 	if (!uid_str)
> 		...
>
>> +	return kstrtou32(uid_str, 10, index);
>> +}
> ...
>
>> +	/* Return 0 instead of error to avoid being unloaded */
>> +	ret = dell_wmi_ddv_battery_index(to_acpi_device(battery->dev.parent), &index);
>> +	if (ret < 0)
>> +		return 0;
> How index is used?
>
> ...
>
>> +	ret = device_create_file(&battery->dev, &data->temp_attr);
>> +	if (ret < 0)
>> +		return ret;
>> +
>> +	ret = device_create_file(&battery->dev, &data->eppid_attr);
>> +	if (ret < 0) {
>> +		device_remove_file(&battery->dev, &data->temp_attr);
>> +
>> +		return ret;
>> +	}
> Why dev_groups member can't be utilized?
>
> ...
>
>> +static void dell_wmi_ddv_debugfs_init(struct wmi_device *wdev)
> Strictly speaking this should return int (see below).
>
>> +{
>> +	struct dentry *entry;
>> +	char name[64];
>> +
>> +	scnprintf(name, ARRAY_SIZE(name), "%s-%s", DRIVER_NAME, dev_name(&wdev->dev));
>> +	entry = debugfs_create_dir(name, NULL);
>> +
>> +	debugfs_create_devm_seqfile(&wdev->dev, "fan_sensor_information", entry,
>> +				    dell_wmi_ddv_fan_read);
>> +	debugfs_create_devm_seqfile(&wdev->dev, "thermal_sensor_information", entry,
>> +				    dell_wmi_ddv_temp_read);
>> +
>> +	devm_add_action_or_reset(&wdev->dev, dell_wmi_ddv_debugfs_remove, entry);
> return devm...
>
> This is not related to debugfs and there is no rule to avoid checking error
> codes from devm_add_action_or_reset().
>
According to the documentation of debugfs_create_dir(), drivers should work fine if debugfs
initialization fails. Thus the the return value of dell_wmi_ddv_debugfs_init() would be ignored
when called, which means that returning an error would serve no purpose.
Additionally, devm_add_action_or_reset() automatically executes the cleanup function if devres
registration fails, so we do not have to care about that.

>> +}
> ...
>
>> +static struct wmi_driver dell_wmi_ddv_driver = {
>> +	.driver = {
>> +		.name = DRIVER_NAME,
> I would use explicit literal since this is a (semi-) ABI, and having it as
> a define feels not fully right.

The driver name is used in two places (init and debugfs), so having a define for it
avoids problems in case someone forgets to change both.

Armin Wolf

>> +	},
>> +	.id_table = dell_wmi_ddv_id_table,
>> +	.probe = dell_wmi_ddv_probe,
>> +};

Re: [PATCH v2 2/2] platform/x86: dell: Add new dell-wmi-ddv driver

[Andy Shevchenko]

On Wed, Sep 28, 2022 at 10:57:16PM +0200, Armin Wolf wrote:
> Am 28.09.22 um 12:47 schrieb Andy Shevchenko:
> > On Tue, Sep 27, 2022 at 10:45:21PM +0200, Armin Wolf wrote:

...

> > > +static void dell_wmi_ddv_debugfs_init(struct wmi_device *wdev)
> > Strictly speaking this should return int (see below).
> > 
> > > +{
> > > +	struct dentry *entry;
> > > +	char name[64];
> > > +
> > > +	scnprintf(name, ARRAY_SIZE(name), "%s-%s", DRIVER_NAME, dev_name(&wdev->dev));
> > > +	entry = debugfs_create_dir(name, NULL);
> > > +
> > > +	debugfs_create_devm_seqfile(&wdev->dev, "fan_sensor_information", entry,
> > > +				    dell_wmi_ddv_fan_read);
> > > +	debugfs_create_devm_seqfile(&wdev->dev, "thermal_sensor_information", entry,
> > > +				    dell_wmi_ddv_temp_read);
> > > +
> > > +	devm_add_action_or_reset(&wdev->dev, dell_wmi_ddv_debugfs_remove, entry);
> > return devm...
> > 
> > This is not related to debugfs and there is no rule to avoid checking error
> > codes from devm_add_action_or_reset().
> > 
> According to the documentation of debugfs_create_dir(), drivers should work fine if debugfs
> initialization fails. Thus the the return value of dell_wmi_ddv_debugfs_init() would be ignored
> when called, which means that returning an error would serve no purpose.
> Additionally, devm_add_action_or_reset() automatically executes the cleanup function if devres
> registration fails, so we do not have to care about that.

The problem with your code that if devm_ call fails and you ain't stop probing
the remove-insert module (or unbind-bind) cycle will fail, because of existing
(leaked) debugfs dentries.

> > > +}

That said, you must check error code of devm_ call above. This is a potential
leak of resources right now in the code.

...

> > > +		.name = DRIVER_NAME,
> > I would use explicit literal since this is a (semi-) ABI, and having it as
> > a define feels not fully right.
> 
> The driver name is used in two places (init and debugfs), so having a define for it
> avoids problems in case someone forgets to change both.

Which is exactly what we must prevent developer to do. If changing debugfs it
mustn't change the driver name, because the latter is ABI, while the former is
not.

I think now you got my point.

-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH v2 2/2] platform/x86: dell: Add new dell-wmi-ddv driver

[Hans de Goede]

Hi,

On 9/29/22 11:50, Andy Shevchenko wrote:
> On Wed, Sep 28, 2022 at 10:57:16PM +0200, Armin Wolf wrote:
>> Am 28.09.22 um 12:47 schrieb Andy Shevchenko:
>>> On Tue, Sep 27, 2022 at 10:45:21PM +0200, Armin Wolf wrote:
> 
> ...
> 
>>>> +static void dell_wmi_ddv_debugfs_init(struct wmi_device *wdev)
>>> Strictly speaking this should return int (see below).
>>>
>>>> +{
>>>> +	struct dentry *entry;
>>>> +	char name[64];
>>>> +
>>>> +	scnprintf(name, ARRAY_SIZE(name), "%s-%s", DRIVER_NAME, dev_name(&wdev->dev));
>>>> +	entry = debugfs_create_dir(name, NULL);
>>>> +
>>>> +	debugfs_create_devm_seqfile(&wdev->dev, "fan_sensor_information", entry,
>>>> +				    dell_wmi_ddv_fan_read);
>>>> +	debugfs_create_devm_seqfile(&wdev->dev, "thermal_sensor_information", entry,
>>>> +				    dell_wmi_ddv_temp_read);
>>>> +
>>>> +	devm_add_action_or_reset(&wdev->dev, dell_wmi_ddv_debugfs_remove, entry);
>>> return devm...
>>>
>>> This is not related to debugfs and there is no rule to avoid checking error
>>> codes from devm_add_action_or_reset().
>>>
>> According to the documentation of debugfs_create_dir(), drivers should work fine if debugfs
>> initialization fails. Thus the the return value of dell_wmi_ddv_debugfs_init() would be ignored
>> when called, which means that returning an error would serve no purpose.
>> Additionally, devm_add_action_or_reset() automatically executes the cleanup function if devres
>> registration fails, so we do not have to care about that.
> 
> The problem with your code that if devm_ call fails and you ain't stop probing
> the remove-insert module (or unbind-bind) cycle will fail, because of existing
> (leaked) debugfs dentries.

No it won't if the devm_ call fails then it will directly call
the passed in handler so in this case we can simply continue
without debugfs entries (which will have been removed by the
handler). The directly calling of the action handler on
failure is the whole difference between devm_add_action()
and devm_add_action_or_reset()
 
So using it this way in the case of a debugfs init function
is fine.

>>>> +		.name = DRIVER_NAME,
>>> I would use explicit literal since this is a (semi-) ABI, and having it as
>>> a define feels not fully right.
>>
>> The driver name is used in two places (init and debugfs), so having a define for it
>> avoids problems in case someone forgets to change both.
> 
> Which is exactly what we must prevent developer to do. If changing debugfs it
> mustn't change the driver name, because the latter is ABI, while the former is
> not.

Arguably both are not really ABI. Drivers have been renamed in the past
without issues for userspace.

Regards,

Hans

Re: [PATCH v2 2/2] platform/x86: dell: Add new dell-wmi-ddv driver

[Armin Wolf]

Am 29.09.22 um 15:12 schrieb Hans de Goede:

> Hi,
>
> On 9/29/22 11:50, Andy Shevchenko wrote:
>> On Wed, Sep 28, 2022 at 10:57:16PM +0200, Armin Wolf wrote:
>>> Am 28.09.22 um 12:47 schrieb Andy Shevchenko:
>>>> On Tue, Sep 27, 2022 at 10:45:21PM +0200, Armin Wolf wrote:
>> ...
>>
>>>>> +static void dell_wmi_ddv_debugfs_init(struct wmi_device *wdev)
>>>> Strictly speaking this should return int (see below).
>>>>
>>>>> +{
>>>>> +	struct dentry *entry;
>>>>> +	char name[64];
>>>>> +Fujitsu Academy
>>>>>
>>>>> +	scnprintf(name, ARRAY_SIZE(name), "%s-%s", DRIVER_NAME, dev_name(&wdev->dev));
>>>>> +	entry = debugfs_create_dir(name, NULL);
>>>>> +
>>>>> +	debugfs_create_devm_seqfile(&wdev->dev, "fan_sensor_information", entry,
>>>>> +				    dell_wmi_ddv_fan_read);
>>>>> +	debugfs_create_devm_seqfile(&wdev->dev, "thermal_sensor_information", entry,
>>>>> +				    dell_wmi_ddv_temp_read);
>>>>> +
>>>>> +	devm_add_action_or_reset(&wdev->dev, dell_wmi_ddv_debugfs_remove, entry);
>>>> return devm...
>>>>
>>>> This is not related to debugfs and there is no rule to avoid checking error
>>>> codes from devm_add_action_or_reset().
>>>>
>>> According to the documentation of debugfs_create_dir(), drivers should work fine if debugfs
>>> initialization fails. Thus the the return value of dell_wmi_ddv_debugfs_init() would be ignored
>>> when called, which means that returning an error would serve no purpose.
>>> Additionally, devm_add_action_or_reset() automatically executes the cleanup function if devres
>>> registration fails, so we do not have to care about that.
>> The problem with your code that if devm_ call fails and you ain't stop probing
>> the remove-insert module (or unbind-bind) cycle will fail, because of existing
>> (leaked) debugfs dentries.
> No it won't if the devm_ call fails then it will directly call
> the passed in handler so in this case we can simply continue
> without debugfs entries (which will have been removed by the
> handler). The directly calling of the action handler on
> failure is the whole difference between devm_add_action()
> and devm_add_action_or_reset()
>
> So using it this way in the case of a debugfs init function
> is fine.
>
>>>>> +		.name = DRIVER_NAME,
>>>> I would use explicit literal since this is a (semi-) ABI, and having it as
>>>> a define feels not fully right.
>>> The driver name is used in two places (init and debugfs), so having a define for it
>>> avoids problems in case someone forgets to change both.
>> Which is exactly what we must prevent developer to do. If changing debugfs it
>> mustn't change the driver name, because the latter is ABI, while the former is
>> not.
> Arguably both are not really ABI. Drivers have been renamed in the past
> without issues for userspace.
>
> Regards,
>
> Hans
>
What is the current status of this patch set? If necessary, i can submit an v3 patch set which includes the
patch regarding the minor style fixes. I also tested the driver on my Dell Insprion 3505 for some time, so
i can proof it works.

Armin Wolf

Re: [PATCH v2 2/2] platform/x86: dell: Add new dell-wmi-ddv driver

[Hans de Goede]

Hi Armin, Rafael,

On 10/21/22 11:34, Armin Wolf wrote:
> Am 29.09.22 um 15:12 schrieb Hans de Goede:
> 
>> Hi,
>>
>> On 9/29/22 11:50, Andy Shevchenko wrote:
>>> On Wed, Sep 28, 2022 at 10:57:16PM +0200, Armin Wolf wrote:
>>>> Am 28.09.22 um 12:47 schrieb Andy Shevchenko:
>>>>> On Tue, Sep 27, 2022 at 10:45:21PM +0200, Armin Wolf wrote:
>>> ...
>>>
>>>>>> +static void dell_wmi_ddv_debugfs_init(struct wmi_device *wdev)
>>>>> Strictly speaking this should return int (see below).
>>>>>
>>>>>> +{
>>>>>> +    struct dentry *entry;
>>>>>> +    char name[64];
>>>>>> +Fujitsu Academy
>>>>>>
>>>>>> +    scnprintf(name, ARRAY_SIZE(name), "%s-%s", DRIVER_NAME, dev_name(&wdev->dev));
>>>>>> +    entry = debugfs_create_dir(name, NULL);
>>>>>> +
>>>>>> +    debugfs_create_devm_seqfile(&wdev->dev, "fan_sensor_information", entry,
>>>>>> +                    dell_wmi_ddv_fan_read);
>>>>>> +    debugfs_create_devm_seqfile(&wdev->dev, "thermal_sensor_information", entry,
>>>>>> +                    dell_wmi_ddv_temp_read);
>>>>>> +
>>>>>> +    devm_add_action_or_reset(&wdev->dev, dell_wmi_ddv_debugfs_remove, entry);
>>>>> return devm...
>>>>>
>>>>> This is not related to debugfs and there is no rule to avoid checking error
>>>>> codes from devm_add_action_or_reset().
>>>>>
>>>> According to the documentation of debugfs_create_dir(), drivers should work fine if debugfs
>>>> initialization fails. Thus the the return value of dell_wmi_ddv_debugfs_init() would be ignored
>>>> when called, which means that returning an error would serve no purpose.
>>>> Additionally, devm_add_action_or_reset() automatically executes the cleanup function if devres
>>>> registration fails, so we do not have to care about that.
>>> The problem with your code that if devm_ call fails and you ain't stop probing
>>> the remove-insert module (or unbind-bind) cycle will fail, because of existing
>>> (leaked) debugfs dentries.
>> No it won't if the devm_ call fails then it will directly call
>> the passed in handler so in this case we can simply continue
>> without debugfs entries (which will have been removed by the
>> handler). The directly calling of the action handler on
>> failure is the whole difference between devm_add_action()
>> and devm_add_action_or_reset()
>>
>> So using it this way in the case of a debugfs init function
>> is fine.
>>
>>>>>> +        .name = DRIVER_NAME,
>>>>> I would use explicit literal since this is a (semi-) ABI, and having it as
>>>>> a define feels not fully right.
>>>> The driver name is used in two places (init and debugfs), so having a define for it
>>>> avoids problems in case someone forgets to change both.
>>> Which is exactly what we must prevent developer to do. If changing debugfs it
>>> mustn't change the driver name, because the latter is ABI, while the former is
>>> not.
>> Arguably both are not really ABI. Drivers have been renamed in the past
>> without issues for userspace.
>>
>> Regards,
>>
>> Hans
>>
> What is the current status of this patch set?

I indicated to Rafael (ACPI subsys maintainer) that I consider this ready
for merging and tried to coordinate this with Rafael, but that email
seems to have fallen through the cracks, likely due to it being pretty
close to the 6.1 merge window. So lets try again:

Rafael, from my pov this patch-set is ready for merging, since 2/2 depends
on "[PATCH v2 1/2] ACPI: battery: Pass battery hook pointer to hook callbacks"
we need to coordinate this.

Since even patch 1/2 mostly touches files under drivers/platform/x86 I would
prefer to merge this through the pdx86 tree, may I have your ack for this ?

> If necessary, i can submit an v3 patch set which includes the
> patch regarding the minor style fixes. I also tested the driver on my Dell Insprion 3505 for some time, so
> i can proof it works.

I can squash the follow up patch into 2/2 when merging this myself. From
my pov no action is needed from you on this at this moment on time. But
it is good that you send a friendly ping about this :)

Regards,

Hans

Re: [PATCH v2 2/2] platform/x86: dell: Add new dell-wmi-ddv driver

[Andy Shevchenko]

On Wed, Sep 28, 2022 at 10:57:16PM +0200, Armin Wolf wrote:
> Am 28.09.22 um 12:47 schrieb Andy Shevchenko:
> > On Tue, Sep 27, 2022 at 10:45:21PM +0200, Armin Wolf wrote:

Side note: When answering to emails, please remove unrelated context, do not
make this the answering party problem.

-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH v2 0/2] platform/x86: dell: Add new dell-wmi-ddv driver

[Hans de Goede]

Hi,

On 9/27/22 22:45, Armin Wolf wrote:
> This patch series adds a new driver for a WMI interface found in
> many newer Dell machines. This interface allows to read battery
> properties like temperature and the ePPID (Dell-specific), while
> also providing fan and thermal sensor information.
> 
> The interface does support multiple batteries which are indentified
> by an "index", which appears to be the battery ACPI UID. Since
> the interface also appears to omit any bounts checking of the
> index, the ACPI battery hook mechanism is used to discover batteries.
> 
> Since the information returned when querying fan/thermal sensor
> information is currently unknown, a debugfs entry is created to
> allow for easier reverse engineering. The interface is likely
> to be replaced by a proper hwmon interface in the future.
> 
> Since the driver can potentially be instantiated multiple times,
> the ACPI battery hook mechanism had to be extended.
> 
> The first patch passes a pointer to the battery hook to the
> hook callbacks, so that they can access instance-specific data
> with container_of().
> 
> The second patch finally adds the new driver. It was called
> dell-wmi-ddv since the interface is called "DDV" by Dell software,
> likely meaning "Dell Data Vault".
> 
> The driver was tested, together with the changes made to the
> ACPI battery driver, on a Dell Inspiron 3505. Other drivers
> already using the battery hook mechanism where changed as well,
> but could only be compile-tested due to missing hardware.
> 
> ---
> Changes in v2:
> - Significantly lower the amount of changes being made to the
> acpi battery driver
> - drop unnecessary ABI description of the temp attribute
> - return 0 when a unsupported battery is found to avoid being
> unloaded
> 
> Armin Wolf (2):
>   ACPI: battery: Pass battery hook pointer to hook callbacks
>   platform/x86: dell: Add new dell-wmi-ddv driver

Thanks.

The new version looks good to me:

Reviewed-by: Hans de Goede <hdegoede@redhat.com>

for the series.

Rafael, from my POV this can be merged through either your
tree or mine. Feel free to merge this through your tree,
or please give your Ack for merging through mine
(assuming you are ok with the changes of course).

Regards,

Hans

Re: [PATCH v2 0/2] platform/x86: dell: Add new dell-wmi-ddv driver

[Andy Shevchenko]

On Wed, Sep 28, 2022 at 11:52:51AM +0200, Hans de Goede wrote:
> On 9/27/22 22:45, Armin Wolf wrote:
> > This patch series adds a new driver for a WMI interface found in
> > many newer Dell machines. This interface allows to read battery
> > properties like temperature and the ePPID (Dell-specific), while
> > also providing fan and thermal sensor information.
> > 
> > The interface does support multiple batteries which are indentified
> > by an "index", which appears to be the battery ACPI UID. Since
> > the interface also appears to omit any bounts checking of the
> > index, the ACPI battery hook mechanism is used to discover batteries.
> > 
> > Since the information returned when querying fan/thermal sensor
> > information is currently unknown, a debugfs entry is created to
> > allow for easier reverse engineering. The interface is likely
> > to be replaced by a proper hwmon interface in the future.
> > 
> > Since the driver can potentially be instantiated multiple times,
> > the ACPI battery hook mechanism had to be extended.
> > 
> > The first patch passes a pointer to the battery hook to the
> > hook callbacks, so that they can access instance-specific data
> > with container_of().
> > 
> > The second patch finally adds the new driver. It was called
> > dell-wmi-ddv since the interface is called "DDV" by Dell software,
> > likely meaning "Dell Data Vault".
> > 
> > The driver was tested, together with the changes made to the
> > ACPI battery driver, on a Dell Inspiron 3505. Other drivers
> > already using the battery hook mechanism where changed as well,
> > but could only be compile-tested due to missing hardware.
> > 
> > ---
> > Changes in v2:
> > - Significantly lower the amount of changes being made to the
> > acpi battery driver
> > - drop unnecessary ABI description of the temp attribute
> > - return 0 when a unsupported battery is found to avoid being
> > unloaded
> > 
> > Armin Wolf (2):
> >   ACPI: battery: Pass battery hook pointer to hook callbacks
> >   platform/x86: dell: Add new dell-wmi-ddv driver
> 
> Thanks.
> 
> The new version looks good to me:
> 
> Reviewed-by: Hans de Goede <hdegoede@redhat.com>
> 
> for the series.
> 
> Rafael, from my POV this can be merged through either your
> tree or mine. Feel free to merge this through your tree,
> or please give your Ack for merging through mine
> (assuming you are ok with the changes of course).

I gave some comments, but it's up to you if they have to be addressed now or as
a follow up.

-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH v2 0/2] platform/x86: dell: Add new dell-wmi-ddv driver

[Hans de Goede]

Hi,

On 9/28/22 12:48, Andy Shevchenko wrote:
> On Wed, Sep 28, 2022 at 11:52:51AM +0200, Hans de Goede wrote:
>> On 9/27/22 22:45, Armin Wolf wrote:
>>> This patch series adds a new driver for a WMI interface found in
>>> many newer Dell machines. This interface allows to read battery
>>> properties like temperature and the ePPID (Dell-specific), while
>>> also providing fan and thermal sensor information.
>>>
>>> The interface does support multiple batteries which are indentified
>>> by an "index", which appears to be the battery ACPI UID. Since
>>> the interface also appears to omit any bounts checking of the
>>> index, the ACPI battery hook mechanism is used to discover batteries.
>>>
>>> Since the information returned when querying fan/thermal sensor
>>> information is currently unknown, a debugfs entry is created to
>>> allow for easier reverse engineering. The interface is likely
>>> to be replaced by a proper hwmon interface in the future.
>>>
>>> Since the driver can potentially be instantiated multiple times,
>>> the ACPI battery hook mechanism had to be extended.
>>>
>>> The first patch passes a pointer to the battery hook to the
>>> hook callbacks, so that they can access instance-specific data
>>> with container_of().
>>>
>>> The second patch finally adds the new driver. It was called
>>> dell-wmi-ddv since the interface is called "DDV" by Dell software,
>>> likely meaning "Dell Data Vault".
>>>
>>> The driver was tested, together with the changes made to the
>>> ACPI battery driver, on a Dell Inspiron 3505. Other drivers
>>> already using the battery hook mechanism where changed as well,
>>> but could only be compile-tested due to missing hardware.
>>>
>>> ---
>>> Changes in v2:
>>> - Significantly lower the amount of changes being made to the
>>> acpi battery driver
>>> - drop unnecessary ABI description of the temp attribute
>>> - return 0 when a unsupported battery is found to avoid being
>>> unloaded
>>>
>>> Armin Wolf (2):
>>>   ACPI: battery: Pass battery hook pointer to hook callbacks
>>>   platform/x86: dell: Add new dell-wmi-ddv driver
>>
>> Thanks.
>>
>> The new version looks good to me:
>>
>> Reviewed-by: Hans de Goede <hdegoede@redhat.com>
>>
>> for the series.
>>
>> Rafael, from my POV this can be merged through either your
>> tree or mine. Feel free to merge this through your tree,
>> or please give your Ack for merging through mine
>> (assuming you are ok with the changes of course).
> 
> I gave some comments, but it's up to you if they have to be addressed now or as
> a follow up.

I have answered a couple of your comments.

What remains is very small / trivial. So IMHO this can go in as
is and then a follow-up patch can be done to address your remaining
comments. Armin, can you do a follow-up patch addressing Andy's
remaining comments please?

I guess it also depends on Rafael and if Rafael is ok with taking
this for 6.1 or if this is going to be 6.2 material.

If this is going to be delayed till 6.2, then we can squash in
the follow-up patch while merging.

Regards,

Hans