diff mbox series

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

Message ID 20230218115318.20662-1-W_Armin@gmx.de
State New
Headers
Series [v2,1/2] platform/x86: dell-ddv: Fix cache invalidation on resume |

Commit Message

Armin Wolf Feb. 18, 2023, 11:53 a.m. UTC 
  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>
---
Changes in v2:
- move checking of the "active" flag inside
  dell_wmi_ddv_hwmon_cache_invalidate()
---
 drivers/platform/x86/dell/dell-wmi-ddv.c | 8 +++++++-
 1 file changed, 7 insertions(+), 1 deletion(-)

--
2.30.2

Comments

Armin Wolf Feb. 28, 2023, 1:21 p.m. UTC | #1 
Am 18.02.23 um 12:53 schrieb 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.

Hello,

what is the status of this series? Both patches are tested on real hardware
and work flawlessly.

Armin Wolf

> Fixes: 3b7eeff93d29 ("platform/x86: dell-ddv: Add hwmon support")
> Signed-off-by: Armin Wolf <W_Armin@gmx.de>
> ---
> Changes in v2:
> - move checking of the "active" flag inside
>    dell_wmi_ddv_hwmon_cache_invalidate()
> ---
>   drivers/platform/x86/dell/dell-wmi-ddv.c | 8 +++++++-
>   1 file changed, 7 insertions(+), 1 deletion(-)
>
> diff --git a/drivers/platform/x86/dell/dell-wmi-ddv.c b/drivers/platform/x86/dell/dell-wmi-ddv.c
> index d547c9d09725..eff4e9649faf 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;
> @@ -520,6 +521,9 @@ static struct hwmon_channel_info *dell_wmi_ddv_channel_create(struct device *dev
>
>   static void dell_wmi_ddv_hwmon_cache_invalidate(struct dell_wmi_ddv_sensors *sensors)
>   {
> +	if (!sensors->active)
> +		return;
> +
>   	mutex_lock(&sensors->lock);
>   	kfree(sensors->obj);
>   	sensors->obj = NULL;
> @@ -530,6 +534,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 +554,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,7 +858,7 @@ 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 */
> +	/* Force re-reading of all active sensors */
>   	dell_wmi_ddv_hwmon_cache_invalidate(&data->fans);
>   	dell_wmi_ddv_hwmon_cache_invalidate(&data->temps);
>
> --
> 2.30.2
>
Hans de Goede March 1, 2023, 12:15 p.m. UTC | #2 
Hi,

On 2/18/23 12:53, 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>
> ---
> Changes in v2:
> - move checking of the "active" flag inside
>   dell_wmi_ddv_hwmon_cache_invalidate()

Thanks, I've applied this patch to my review-hans branch:
https://git.kernel.org/pub/scm/linux/kernel/git/pdx86/platform-drivers-x86.git/log/?h=review-hans

I'll rebase that branch once 6.3-rc1 is out and then push the rebased
patch to the fixes branch and include it in my next 6.3 fixes pull-req
to Linus.

Regards,

Hans




> ---
>  drivers/platform/x86/dell/dell-wmi-ddv.c | 8 +++++++-
>  1 file changed, 7 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/platform/x86/dell/dell-wmi-ddv.c b/drivers/platform/x86/dell/dell-wmi-ddv.c
> index d547c9d09725..eff4e9649faf 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;
> @@ -520,6 +521,9 @@ static struct hwmon_channel_info *dell_wmi_ddv_channel_create(struct device *dev
> 
>  static void dell_wmi_ddv_hwmon_cache_invalidate(struct dell_wmi_ddv_sensors *sensors)
>  {
> +	if (!sensors->active)
> +		return;
> +
>  	mutex_lock(&sensors->lock);
>  	kfree(sensors->obj);
>  	sensors->obj = NULL;
> @@ -530,6 +534,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 +554,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,7 +858,7 @@ 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 */
> +	/* Force re-reading of all active sensors */
>  	dell_wmi_ddv_hwmon_cache_invalidate(&data->fans);
>  	dell_wmi_ddv_hwmon_cache_invalidate(&data->temps);
> 
> --
> 2.30.2
>
diff mbox series

Patch

diff --git a/drivers/platform/x86/dell/dell-wmi-ddv.c b/drivers/platform/x86/dell/dell-wmi-ddv.c
index d547c9d09725..eff4e9649faf 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;
@@ -520,6 +521,9 @@  static struct hwmon_channel_info *dell_wmi_ddv_channel_create(struct device *dev

 static void dell_wmi_ddv_hwmon_cache_invalidate(struct dell_wmi_ddv_sensors *sensors)
 {
+	if (!sensors->active)
+		return;
+
 	mutex_lock(&sensors->lock);
 	kfree(sensors->obj);
 	sensors->obj = NULL;
@@ -530,6 +534,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 +554,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,7 +858,7 @@  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 */
+	/* Force re-reading of all active sensors */
 	dell_wmi_ddv_hwmon_cache_invalidate(&data->fans);
 	dell_wmi_ddv_hwmon_cache_invalidate(&data->temps);