diff mbox series

[6/9] thermal/core: Protect hwmon accesses to thermal operations with thermal zone mutex

Message ID 20221017130910.2307118-7-linux@roeck-us.net
State New
Headers
Series thermal/core: Protect thermal device operations against removal |

Commit Message

Guenter Roeck Oct. 17, 2022, 1:09 p.m. UTC 
  In preparation to protecting access to thermal operations against thermal
zone device removal, protect hwmon accesses to thermal zone operations
with the thermal zone mutex. After acquiring the mutex, ensure that the
thermal zone device is registered before proceeding.

Signed-off-by: Guenter Roeck <linux@roeck-us.net>
---
 drivers/thermal/thermal_hwmon.c | 14 +++++++++++---
 1 file changed, 11 insertions(+), 3 deletions(-)

Comments

Rafael J. Wysocki Nov. 9, 2022, 7:19 p.m. UTC | #1 
On Mon, Oct 17, 2022 at 3:09 PM Guenter Roeck <linux@roeck-us.net> wrote:
>
> In preparation to protecting access to thermal operations against thermal
> zone device removal, protect hwmon accesses to thermal zone operations
> with the thermal zone mutex. After acquiring the mutex, ensure that the
> thermal zone device is registered before proceeding.
>
> Signed-off-by: Guenter Roeck <linux@roeck-us.net>
> ---
>  drivers/thermal/thermal_hwmon.c | 14 +++++++++++---
>  1 file changed, 11 insertions(+), 3 deletions(-)
>
> diff --git a/drivers/thermal/thermal_hwmon.c b/drivers/thermal/thermal_hwmon.c
> index f53f4ceb6a5d..33bfbaed4236 100644
> --- a/drivers/thermal/thermal_hwmon.c
> +++ b/drivers/thermal/thermal_hwmon.c
> @@ -77,11 +77,19 @@ temp_crit_show(struct device *dev, struct device_attribute *attr, char *buf)
>         int temperature;
>         int ret;
>
> +       mutex_lock(&tz->lock);
> +
> +       if (!device_is_registered(&tz->device)) {
> +               ret = -ENODEV;
> +               goto unlock;
> +       }
> +
>         ret = tz->ops->get_crit_temp(tz, &temperature);

Again, I would do it this way:

        if (device_is_registered(&tz->device))
                ret = tz->ops->get_crit_temp(tz, &temperature);
        else
                ret = -ENODEV;

And I wouldn't change the code below (the ternary operator is out of
fashion in particular).

> -       if (ret)
> -               return ret;
>
> -       return sprintf(buf, "%d\n", temperature);
> +unlock:
> +       mutex_unlock(&tz->lock);
> +
> +       return ret ? ret : sprintf(buf, "%d\n", temperature);
>  }
>
>
> --
Guenter Roeck Nov. 10, 2022, 2:21 p.m. UTC | #2 
On Wed, Nov 09, 2022 at 08:19:13PM +0100, Rafael J. Wysocki wrote:
> On Mon, Oct 17, 2022 at 3:09 PM Guenter Roeck <linux@roeck-us.net> wrote:
[ ... ]
> 
> And I wouldn't change the code below (the ternary operator is out of
> fashion in particular).
> 

I tried to introduce some consistency; the ternary operator is used
in some of the existing thermal code. Guess I went the wrong direction.
Never mind; I don't have a strong opinion either way.
I updated the series patches to no longer use ternary operators in
updated code, but I left existing code alone (changing that should not
be part of this patch set anyway).

Thanks,
Guenter
Rafael J. Wysocki Nov. 10, 2022, 2:24 p.m. UTC | #3 
On Thu, Nov 10, 2022 at 3:21 PM Guenter Roeck <linux@roeck-us.net> wrote:
>
> On Wed, Nov 09, 2022 at 08:19:13PM +0100, Rafael J. Wysocki wrote:
> > On Mon, Oct 17, 2022 at 3:09 PM Guenter Roeck <linux@roeck-us.net> wrote:
> [ ... ]
> >
> > And I wouldn't change the code below (the ternary operator is out of
> > fashion in particular).
> >
>
> I tried to introduce some consistency; the ternary operator is used
> in some of the existing thermal code. Guess I went the wrong direction.
> Never mind; I don't have a strong opinion either way.
> I updated the series patches to no longer use ternary operators in
> updated code, but I left existing code alone (changing that should not
> be part of this patch set anyway).

Thanks, that's what I would have done too.
diff mbox series

Patch

diff --git a/drivers/thermal/thermal_hwmon.c b/drivers/thermal/thermal_hwmon.c
index f53f4ceb6a5d..33bfbaed4236 100644
--- a/drivers/thermal/thermal_hwmon.c
+++ b/drivers/thermal/thermal_hwmon.c
@@ -77,11 +77,19 @@  temp_crit_show(struct device *dev, struct device_attribute *attr, char *buf)
 	int temperature;
 	int ret;
 
+	mutex_lock(&tz->lock);
+
+	if (!device_is_registered(&tz->device)) {
+		ret = -ENODEV;
+		goto unlock;
+	}
+
 	ret = tz->ops->get_crit_temp(tz, &temperature);
-	if (ret)
-		return ret;
 
-	return sprintf(buf, "%d\n", temperature);
+unlock:
+	mutex_unlock(&tz->lock);
+
+	return ret ? ret : sprintf(buf, "%d\n", temperature);
 }