| Message ID | 20230218115318.20662-1-W_Armin@gmx.de (mailing list archive) |
|---|---|
| State | Handled Elsewhere |
| Headers | show |
| Series | [v2,1/2] platform/x86: dell-ddv: Fix cache invalidation on resume | expand |
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 >
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 --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);
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