[PATCH 1/2] platform/x86: dell-ddv: Fix cache invalidation on resume

[PATCH 1/2] platform/x86: dell-ddv: Fix cache invalidation on resume

[Armin Wolf]

If one or both sensor buffers could not be initialized, either
due to missing hardware support or due to some error during probing,
the resume handler will encounter undefined behaviour when
attempting to lock buffers then protected by an uninitialized or
destroyed mutex.
Fix this by introducing a "active" flag which is set during probe,
and only invalidate buffers which where flaged as "active".

Tested on a Dell Inspiron 3505.

Fixes: 3b7eeff93d29 ("platform/x86: dell-ddv: Add hwmon support")
Signed-off-by: Armin Wolf <W_Armin@gmx.de>
---
 drivers/platform/x86/dell/dell-wmi-ddv.c | 12 +++++++++---
 1 file changed, 9 insertions(+), 3 deletions(-)

diff --git a/drivers/platform/x86/dell/dell-wmi-ddv.c b/drivers/platform/x86/dell/dell-wmi-ddv.c
index d547c9d09725..58f996b3b374 100644
--- a/drivers/platform/x86/dell/dell-wmi-ddv.c
+++ b/drivers/platform/x86/dell/dell-wmi-ddv.c
@@ -96,6 +96,7 @@ struct combined_chip_info {
 };

 struct dell_wmi_ddv_sensors {
+	bool active;
 	struct mutex lock;	/* protect caching */
 	unsigned long timestamp;
 	union acpi_object *obj;
@@ -530,6 +531,7 @@ static void dell_wmi_ddv_hwmon_cache_destroy(void *data)
 {
 	struct dell_wmi_ddv_sensors *sensors = data;

+	sensors->active = false;
 	mutex_destroy(&sensors->lock);
 	kfree(sensors->obj);
 }
@@ -549,6 +551,7 @@ static struct hwmon_channel_info *dell_wmi_ddv_channel_init(struct wmi_device *w
 		return ERR_PTR(ret);

 	mutex_init(&sensors->lock);
+	sensors->active = true;

 	ret = devm_add_action_or_reset(&wdev->dev, dell_wmi_ddv_hwmon_cache_destroy, sensors);
 	if (ret < 0)
@@ -852,9 +855,12 @@ static int dell_wmi_ddv_resume(struct device *dev)
 {
 	struct dell_wmi_ddv_data *data = dev_get_drvdata(dev);

-	/* Force re-reading of all sensors */
-	dell_wmi_ddv_hwmon_cache_invalidate(&data->fans);
-	dell_wmi_ddv_hwmon_cache_invalidate(&data->temps);
+	/* Force re-reading of all active sensors */
+	if (data->fans.active)
+		dell_wmi_ddv_hwmon_cache_invalidate(&data->fans);
+
+	if (data->temps.active)
+		dell_wmi_ddv_hwmon_cache_invalidate(&data->temps);

 	return 0;
 }
--
2.30.2

[PATCH 2/2] platform/x86: dell-ddv: Fix temperature scaling

[Armin Wolf]

After using the built-in UEFI hardware diagnostics to compare
the measured battery temperature, i noticed that the temperature
is actually expressed in tenth degree kelvin, similar to the
SBS-Data standard. For example, a value of 2992 is displayed as
26 degrees celsius.
Fix the scaling so that the correct values are being displayed.

Tested on a Dell Inspiron 3505.

Fixes: a77272c16041 ("platform/x86: dell: Add new dell-wmi-ddv driver")
Signed-off-by: Armin Wolf <W_Armin@gmx.de>
---
 drivers/platform/x86/dell/dell-wmi-ddv.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/platform/x86/dell/dell-wmi-ddv.c b/drivers/platform/x86/dell/dell-wmi-ddv.c
index 58f996b3b374..efa860dbff6c 100644
--- a/drivers/platform/x86/dell/dell-wmi-ddv.c
+++ b/drivers/platform/x86/dell/dell-wmi-ddv.c
@@ -662,7 +662,7 @@ static ssize_t temp_show(struct device *dev, struct device_attribute *attr, char
 	if (ret < 0)
 		return ret;

-	return sysfs_emit(buf, "%d\n", DIV_ROUND_CLOSEST(value, 10));
+	return sysfs_emit(buf, "%d\n", DIV_ROUND_CLOSEST(value * 10 - 27315, 10));
 }

 static ssize_t eppid_show(struct device *dev, struct device_attribute *attr, char *buf)
--
2.30.2

Re: [PATCH 1/2] platform/x86: dell-ddv: Fix cache invalidation on resume

[Hans de Goede]

Hi Armin,

On 2/13/23 19:22, Armin Wolf wrote:
> If one or both sensor buffers could not be initialized, either
> due to missing hardware support or due to some error during probing,
> the resume handler will encounter undefined behaviour when
> attempting to lock buffers then protected by an uninitialized or
> destroyed mutex.
> Fix this by introducing a "active" flag which is set during probe,
> and only invalidate buffers which where flaged as "active".
> 
> Tested on a Dell Inspiron 3505.
> 
> Fixes: 3b7eeff93d29 ("platform/x86: dell-ddv: Add hwmon support")
> Signed-off-by: Armin Wolf <W_Armin@gmx.de>

Can you move the "if (...active)" check to inside
dell_wmi_ddv_hwmon_cache_invalidate() please ?

Otherwise this patch as well as patch 2/2 look good to me.

Regards,

Hans



> ---
>  drivers/platform/x86/dell/dell-wmi-ddv.c | 12 +++++++++---
>  1 file changed, 9 insertions(+), 3 deletions(-)
> 
> diff --git a/drivers/platform/x86/dell/dell-wmi-ddv.c b/drivers/platform/x86/dell/dell-wmi-ddv.c
> index d547c9d09725..58f996b3b374 100644
> --- a/drivers/platform/x86/dell/dell-wmi-ddv.c
> +++ b/drivers/platform/x86/dell/dell-wmi-ddv.c
> @@ -96,6 +96,7 @@ struct combined_chip_info {
>  };
> 
>  struct dell_wmi_ddv_sensors {
> +	bool active;
>  	struct mutex lock;	/* protect caching */
>  	unsigned long timestamp;
>  	union acpi_object *obj;
> @@ -530,6 +531,7 @@ static void dell_wmi_ddv_hwmon_cache_destroy(void *data)
>  {
>  	struct dell_wmi_ddv_sensors *sensors = data;
> 
> +	sensors->active = false;
>  	mutex_destroy(&sensors->lock);
>  	kfree(sensors->obj);
>  }
> @@ -549,6 +551,7 @@ static struct hwmon_channel_info *dell_wmi_ddv_channel_init(struct wmi_device *w
>  		return ERR_PTR(ret);
> 
>  	mutex_init(&sensors->lock);
> +	sensors->active = true;
> 
>  	ret = devm_add_action_or_reset(&wdev->dev, dell_wmi_ddv_hwmon_cache_destroy, sensors);
>  	if (ret < 0)
> @@ -852,9 +855,12 @@ static int dell_wmi_ddv_resume(struct device *dev)
>  {
>  	struct dell_wmi_ddv_data *data = dev_get_drvdata(dev);
> 
> -	/* Force re-reading of all sensors */
> -	dell_wmi_ddv_hwmon_cache_invalidate(&data->fans);
> -	dell_wmi_ddv_hwmon_cache_invalidate(&data->temps);
> +	/* Force re-reading of all active sensors */
> +	if (data->fans.active)
> +		dell_wmi_ddv_hwmon_cache_invalidate(&data->fans);
> +
> +	if (data->temps.active)
> +		dell_wmi_ddv_hwmon_cache_invalidate(&data->temps);
> 
>  	return 0;
>  }
> --
> 2.30.2
>