[PATCH v2 0/2] Support temperature trips by HWMON core and LM90 driver

[PATCH v2 0/2] Support temperature trips by HWMON core and LM90 driver

[Dmitry Osipenko]

Hi,

It's typical for embedded devices to use LM90-compatible sensor for
monitoring of CPU core and skin temperatures. The sensor is often
used by thermal zone that performs passive cooling and emergency
shutdown on overheat, hence it's more optimal to use interrupt for
a faster notification about temperature changes. Thermal framework
provides set_trips() callback for programming of temperature trips,
let's support it by HWMON.

Changelog:

v2: - Reworked set_trips() by making it generic. Now callback invokes
      the min/max temperature write method directly, instead of using
      additional new hwmon callback. This was suggested by Guenter Roeck.

    - Added new patch that fixes integer overflows in the LM90 driver.
      The fixes are necessary for supporting set_trips().

Dmitry Osipenko (2):
  hwmon: (lm90) Prevent integer overflow of temperature calculations
  hwmon: Support set_trips() of thermal device ops

 drivers/hwmon/hwmon.c | 32 ++++++++++++++++++++++++++++++++
 drivers/hwmon/lm90.c  |  6 ++++++
 2 files changed, 38 insertions(+)

-- 
2.30.2

[PATCH v2 1/2] hwmon: (lm90) Prevent integer overflow of temperature calculations

[Dmitry Osipenko]

The minimum temperature value that is passed to the driver is unlimited
and value that is close to INT_MIN results in integer overflow of
temperature calculations made by the driver. Limit the value in order
to prevent the overflow. For now the overflow condition is harmless,
but thermal framework won't work properly once we will support the
set_trips() callback because it will pass INT_MIN value to the driver.

Signed-off-by: Dmitry Osipenko <digetx@gmail.com>
---
 drivers/hwmon/lm90.c | 6 ++++++
 1 file changed, 6 insertions(+)

diff --git a/drivers/hwmon/lm90.c b/drivers/hwmon/lm90.c
index b53f17511b05..6e2fa976098f 100644
--- a/drivers/hwmon/lm90.c
+++ b/drivers/hwmon/lm90.c
@@ -1028,6 +1028,9 @@ static int lm90_set_temp11(struct lm90_data *data, int index, long val)
 	struct reg *regp = &reg[index];
 	int err;
 
+	/* prevent integer overflow */
+	val = max(val, -128000l);
+
 	/* +16 degrees offset for temp2 for the LM99 */
 	if (data->kind == lm99 && index <= 2)
 		val -= 16000;
@@ -1088,6 +1091,9 @@ static int lm90_set_temp8(struct lm90_data *data, int index, long val)
 	struct i2c_client *client = data->client;
 	int err;
 
+	/* prevent integer overflow */
+	val = max(val, -128000l);
+
 	/* +16 degrees offset for temp2 for the LM99 */
 	if (data->kind == lm99 && index == 3)
 		val -= 16000;
-- 
2.30.2

[PATCH v2 2/2] hwmon: Support set_trips() of thermal device ops

[Dmitry Osipenko]

Support set_trips() callback of thermal device ops. This allows HWMON
device to operatively notify thermal core about temperature changes, which
is very handy to have in a case where HWMON sensor is used by CPU thermal
zone that performs passive cooling and emergency shutdown on overheat.
Thermal core will be able to react faster to temperature changes.

Signed-off-by: Dmitry Osipenko <digetx@gmail.com>
---
 drivers/hwmon/hwmon.c | 32 ++++++++++++++++++++++++++++++++
 1 file changed, 32 insertions(+)

diff --git a/drivers/hwmon/hwmon.c b/drivers/hwmon/hwmon.c
index fd47ab4e6892..e74dc81e650d 100644
--- a/drivers/hwmon/hwmon.c
+++ b/drivers/hwmon/hwmon.c
@@ -153,8 +153,40 @@ static int hwmon_thermal_get_temp(void *data, int *temp)
 	return 0;
 }
 
+static int hwmon_thermal_set_trips(void *data, int low, int high)
+{
+	struct hwmon_thermal_data *tdata = data;
+	struct hwmon_device *hwdev = to_hwmon_device(tdata->dev);
+	const struct hwmon_chip_info *chip = hwdev->chip;
+	const struct hwmon_channel_info **info = chip->info;
+	unsigned int i;
+
+	if (!chip->ops->write)
+		return 0;
+
+	for (i = 1; info[i] && info[i]->type != hwmon_temp; i++)
+		continue;
+
+	if (info[i] && info[i]->config[tdata->index] & HWMON_T_MIN) {
+		int err = chip->ops->write(tdata->dev, hwmon_temp,
+					   hwmon_temp_min, tdata->index, low);
+		if (err < 0 && err != -EOPNOTSUPP)
+			return err;
+	}
+
+	if (info[i] && info[i]->config[tdata->index] & HWMON_T_MAX) {
+		int err = chip->ops->write(tdata->dev, hwmon_temp,
+					   hwmon_temp_max, tdata->index, high);
+		if (err < 0 && err != -EOPNOTSUPP)
+			return err;
+	}
+
+	return 0;
+}
+
 static const struct thermal_zone_of_device_ops hwmon_thermal_ops = {
 	.get_temp = hwmon_thermal_get_temp,
+	.set_trips = hwmon_thermal_set_trips,
 };
 
 static void hwmon_thermal_remove_sensor(void *data)
-- 
2.30.2

Re: [PATCH v2 1/2] hwmon: (lm90) Prevent integer overflow of temperature calculations

[Guenter Roeck]

On Mon, Jun 21, 2021 at 12:14:07AM +0300, Dmitry Osipenko wrote:
> The minimum temperature value that is passed to the driver is unlimited
> and value that is close to INT_MIN results in integer overflow of
> temperature calculations made by the driver. Limit the value in order
> to prevent the overflow. For now the overflow condition is harmless,
> but thermal framework won't work properly once we will support the
> set_trips() callback because it will pass INT_MIN value to the driver.
> 

AFAICS that should only happen for lm99 because all other values
are bound in the temp_to_xxx functions. Where else do you see an
overflow (or underflow) ?

Thanks,
Guenter

> Signed-off-by: Dmitry Osipenko <digetx@gmail.com>
> ---
>  drivers/hwmon/lm90.c | 6 ++++++
>  1 file changed, 6 insertions(+)
> 
> diff --git a/drivers/hwmon/lm90.c b/drivers/hwmon/lm90.c
> index b53f17511b05..6e2fa976098f 100644
> --- a/drivers/hwmon/lm90.c
> +++ b/drivers/hwmon/lm90.c
> @@ -1028,6 +1028,9 @@ static int lm90_set_temp11(struct lm90_data *data, int index, long val)
>  	struct reg *regp = &reg[index];
>  	int err;
>  
> +	/* prevent integer overflow */
> +	val = max(val, -128000l);
> +
>  	/* +16 degrees offset for temp2 for the LM99 */
>  	if (data->kind == lm99 && index <= 2)
>  		val -= 16000;
> @@ -1088,6 +1091,9 @@ static int lm90_set_temp8(struct lm90_data *data, int index, long val)
>  	struct i2c_client *client = data->client;
>  	int err;
>  
> +	/* prevent integer overflow */
> +	val = max(val, -128000l);
> +
>  	/* +16 degrees offset for temp2 for the LM99 */
>  	if (data->kind == lm99 && index == 3)
>  		val -= 16000;
> -- 
> 2.30.2
> 

Re: [PATCH v2 1/2] hwmon: (lm90) Prevent integer overflow of temperature calculations

[Dmitry Osipenko]

21.06.2021 15:12, Guenter Roeck пишет:
> On Mon, Jun 21, 2021 at 12:14:07AM +0300, Dmitry Osipenko wrote:
>> The minimum temperature value that is passed to the driver is unlimited
>> and value that is close to INT_MIN results in integer overflow of
>> temperature calculations made by the driver. Limit the value in order
>> to prevent the overflow. For now the overflow condition is harmless,
>> but thermal framework won't work properly once we will support the
>> set_trips() callback because it will pass INT_MIN value to the driver.
>>
> AFAICS that should only happen for lm99 because all other values
> are bound in the temp_to_xxx functions. Where else do you see an
> overflow (or underflow) ?

You're correct that the overflow affects only lm99. But why we should
ignore it?

Re: [PATCH v2 1/2] hwmon: (lm90) Prevent integer overflow of temperature calculations

[Guenter Roeck]

On Mon, Jun 21, 2021 at 03:14:40PM +0300, Dmitry Osipenko wrote:
> 21.06.2021 15:12, Guenter Roeck пишет:
> > On Mon, Jun 21, 2021 at 12:14:07AM +0300, Dmitry Osipenko wrote:
> >> The minimum temperature value that is passed to the driver is unlimited
> >> and value that is close to INT_MIN results in integer overflow of
> >> temperature calculations made by the driver. Limit the value in order
> >> to prevent the overflow. For now the overflow condition is harmless,
> >> but thermal framework won't work properly once we will support the
> >> set_trips() callback because it will pass INT_MIN value to the driver.
> >>
> > AFAICS that should only happen for lm99 because all other values
> > are bound in the temp_to_xxx functions. Where else do you see an
> > overflow (or underflow) ?
> 
> You're correct that the overflow affects only lm99. But why we should
> ignore it?

That isn't the point. The point is that you claimed there would be a
generic underflow, which is not the case. That means we'll only need
to apply the fix to the lm99 specific code (which unconditionally
subtracts an offset from the provided value, causing the underflow).

Anyway, thanks for alerting me to the issue. As it turns out, there are
other underflow issues in the driver. With improved module test scripts,
I get:

Testing lm90 ...
temp1_crit_hyst: Suspected underflow: [min=54000, read 85000, written -9223372036854775808]
Testing lm99 ...
temp1_crit_hyst: Suspected underflow: [min=96000, read 127000, written -9223372036854775808]
temp2_crit: Suspected underflow: [min=-112000, read 143000, written -9223372036854775808]
temp2_min: Suspected underflow: [min=-112000, read 143875, written -9223372036854775808]
temp2_max: Suspected underflow: [min=-112000, read 143875, written -9223372036854775808]

So we'll need fixes for lm99 temp2_{min/max/crit} and for temp1_crit_hyst
(the latter affects all chips supported by the driver).

Thanks,
Guenter

Re: [PATCH v2 1/2] hwmon: (lm90) Prevent integer overflow of temperature calculations

[Dmitry Osipenko]

21.06.2021 17:24, Guenter Roeck пишет:
> On Mon, Jun 21, 2021 at 03:14:40PM +0300, Dmitry Osipenko wrote:
>> 21.06.2021 15:12, Guenter Roeck пишет:
>>> On Mon, Jun 21, 2021 at 12:14:07AM +0300, Dmitry Osipenko wrote:
>>>> The minimum temperature value that is passed to the driver is unlimited
>>>> and value that is close to INT_MIN results in integer overflow of
>>>> temperature calculations made by the driver. Limit the value in order
>>>> to prevent the overflow. For now the overflow condition is harmless,
>>>> but thermal framework won't work properly once we will support the
>>>> set_trips() callback because it will pass INT_MIN value to the driver.
>>>>
>>> AFAICS that should only happen for lm99 because all other values
>>> are bound in the temp_to_xxx functions. Where else do you see an
>>> overflow (or underflow) ?
>>
>> You're correct that the overflow affects only lm99. But why we should
>> ignore it?
> 
> That isn't the point. The point is that you claimed there would be a
> generic underflow, which is not the case. That means we'll only need
> to apply the fix to the lm99 specific code (which unconditionally
> subtracts an offset from the provided value, causing the underflow).
> 
> Anyway, thanks for alerting me to the issue. As it turns out, there are
> other underflow issues in the driver. With improved module test scripts,
> I get:
> 
> Testing lm90 ...
> temp1_crit_hyst: Suspected underflow: [min=54000, read 85000, written -9223372036854775808]
> Testing lm99 ...
> temp1_crit_hyst: Suspected underflow: [min=96000, read 127000, written -9223372036854775808]
> temp2_crit: Suspected underflow: [min=-112000, read 143000, written -9223372036854775808]
> temp2_min: Suspected underflow: [min=-112000, read 143875, written -9223372036854775808]
> temp2_max: Suspected underflow: [min=-112000, read 143875, written -9223372036854775808]
> 
> So we'll need fixes for lm99 temp2_{min/max/crit} and for temp1_crit_hyst
> (the latter affects all chips supported by the driver).

I'll prepare v3 with the updated commit message and fixed
temp1_crit_hyst, thank you.