[PATCH 1/2] hwmon: (max16065) Fix overflows seen when writing limits

[PATCH 1/2] hwmon: (max16065) Fix overflows seen when writing limits

[Guenter Roeck]

Writing large limits resulted in overflows as reported by module tests.

in0_lcrit: Suspected overflow: [max=5538, read 0, written 2147483647]
in0_crit: Suspected overflow: [max=5538, read 0, written 2147483647]
in0_min: Suspected overflow: [max=5538, read 0, written 2147483647]

Fix the problem by clamping prior to multiplications and the use of
DIV_ROUND_CLOSEST, and by using consistent variable types.

Fixes: f5bae2642e3d ("hwmon: Driver for MAX16065 System Manager and compatibles")
Signed-off-by: Guenter Roeck <linux@roeck-us.net>
---
 drivers/hwmon/max16065.c | 5 +++--
 1 file changed, 3 insertions(+), 2 deletions(-)

diff --git a/drivers/hwmon/max16065.c b/drivers/hwmon/max16065.c
index 7ce9a89f93a0..5b2a174c6bad 100644
--- a/drivers/hwmon/max16065.c
+++ b/drivers/hwmon/max16065.c
@@ -114,9 +114,10 @@ static inline int LIMIT_TO_MV(int limit, int range)
 	return limit * range / 256;
 }
 
-static inline int MV_TO_LIMIT(int mv, int range)
+static inline int MV_TO_LIMIT(unsigned long mv, int range)
 {
-	return clamp_val(DIV_ROUND_CLOSEST(mv * 256, range), 0, 255);
+	mv = clamp_val(mv, 0, ULONG_MAX / 256);
+	return DIV_ROUND_CLOSEST(clamp_val(mv * 256, 0, range * 255), range);
 }
 
 static inline int ADC_TO_CURR(int adc, int gain)
-- 
2.39.2

[PATCH 2/2] hwmon: (max16065) Fix alarm attributes

[Guenter Roeck]

Chips reporting overcurrent alarms report it in the second alarm register.
That means the second alarm register has to be read, even if the chip only
supports 8 or fewer ADC channels.

MAX16067 and MAX16068 report undervoltage and overvoltage alarms in
separate registers. Fold register contents together to report both with
the existing alarm attribute. This requires actually storing the chip type
in struct max16065_data. Rename the variable 'chip' to match the variable
name used in the probe function.

Fixes: f5bae2642e3d ("hwmon: Driver for MAX16065 System Manager and compatibles")
Signed-off-by: Guenter Roeck <linux@roeck-us.net>
---
 drivers/hwmon/max16065.c | 12 ++++++++++--
 1 file changed, 10 insertions(+), 2 deletions(-)

diff --git a/drivers/hwmon/max16065.c b/drivers/hwmon/max16065.c
index 5b2a174c6bad..0ccb5eb596fc 100644
--- a/drivers/hwmon/max16065.c
+++ b/drivers/hwmon/max16065.c
@@ -79,7 +79,7 @@ static const bool max16065_have_current[] = {
 };
 
 struct max16065_data {
-	enum chips type;
+	enum chips chip;
 	struct i2c_client *client;
 	const struct attribute_group *groups[4];
 	struct mutex update_lock;
@@ -162,10 +162,17 @@ static struct max16065_data *max16065_update_device(struct device *dev)
 						     MAX16065_CURR_SENSE);
 		}
 
-		for (i = 0; i < DIV_ROUND_UP(data->num_adc, 8); i++)
+		for (i = 0; i < 2; i++)
 			data->fault[i]
 			  = i2c_smbus_read_byte_data(client, MAX16065_FAULT(i));
 
+		/*
+		 * MAX16067 and MAX16068 have separate undervoltage and
+		 * overvoltage alarm bits. Squash them together.
+		 */
+		if (data->chip == max16067 || data->chip == max16068)
+			data->fault[0] |= data->fault[1];
+
 		data->last_updated = jiffies;
 		data->valid = true;
 	}
@@ -514,6 +521,7 @@ static int max16065_probe(struct i2c_client *client)
 	if (unlikely(!data))
 		return -ENOMEM;
 
+	data->chip = chip;
 	data->client = client;
 	mutex_init(&data->update_lock);
 
-- 
2.39.2

Re: [PATCH 1/2] hwmon: (max16065) Fix overflows seen when writing limits

[Tzung-Bi Shih]

On Sun, Jul 21, 2024 at 12:35:05PM -0700, Guenter Roeck wrote:
> Writing large limits resulted in overflows as reported by module tests.
> 
> in0_lcrit: Suspected overflow: [max=5538, read 0, written 2147483647]
> in0_crit: Suspected overflow: [max=5538, read 0, written 2147483647]
> in0_min: Suspected overflow: [max=5538, read 0, written 2147483647]
> 
> Fix the problem by clamping prior to multiplications and the use of
> DIV_ROUND_CLOSEST, and by using consistent variable types.
> 
> Fixes: f5bae2642e3d ("hwmon: Driver for MAX16065 System Manager and compatibles")
> Signed-off-by: Guenter Roeck <linux@roeck-us.net>

Reviewed-by: Tzung-Bi Shih <tzungbi@kernel.org>

Re: [PATCH 2/2] hwmon: (max16065) Fix alarm attributes

[Tzung-Bi Shih]

On Sun, Jul 21, 2024 at 12:35:06PM -0700, Guenter Roeck wrote:
> Chips reporting overcurrent alarms report it in the second alarm register.

I can't understand the sentence.  Not sure whether it needs to be rephrased or
not.  s/overcurrent/overvoltage/.

> That means the second alarm register has to be read, even if the chip only
> supports 8 or fewer ADC channels.
> 
> MAX16067 and MAX16068 report undervoltage and overvoltage alarms in
> separate registers. Fold register contents together to report both with
> the existing alarm attribute. This requires actually storing the chip type
> in struct max16065_data. Rename the variable 'chip' to match the variable
> name used in the probe function.
> 
> Fixes: f5bae2642e3d ("hwmon: Driver for MAX16065 System Manager and compatibles")
> Signed-off-by: Guenter Roeck <linux@roeck-us.net>

Reviewed-by: Tzung-Bi Shih <tzungbi@kernel.org>

Re: [PATCH 2/2] hwmon: (max16065) Fix alarm attributes

[Guenter Roeck]

On 7/21/24 20:30, Tzung-Bi Shih wrote:
> On Sun, Jul 21, 2024 at 12:35:06PM -0700, Guenter Roeck wrote:
>> Chips reporting overcurrent alarms report it in the second alarm register.
> 
> I can't understand the sentence.  Not sure whether it needs to be rephrased or
> not.  s/overcurrent/overvoltage/.
> 

No, it is over-current. Not all chips support current measurements.
Those who do support it also support reporting over-current alarms.
Over-current alarms are reported in the second alarm register.

Do you have a suggestion for better wording ?

Thanks,
Guenter

>> That means the second alarm register has to be read, even if the chip only
>> supports 8 or fewer ADC channels.
>>
>> MAX16067 and MAX16068 report undervoltage and overvoltage alarms in
>> separate registers. Fold register contents together to report both with
>> the existing alarm attribute. This requires actually storing the chip type
>> in struct max16065_data. Rename the variable 'chip' to match the variable
>> name used in the probe function.
>>
>> Fixes: f5bae2642e3d ("hwmon: Driver for MAX16065 System Manager and compatibles")
>> Signed-off-by: Guenter Roeck <linux@roeck-us.net>
> 
> Reviewed-by: Tzung-Bi Shih <tzungbi@kernel.org>

Re: [PATCH 2/2] hwmon: (max16065) Fix alarm attributes

[Tzung-Bi Shih]

On Sun, Jul 21, 2024 at 08:48:51PM -0700, Guenter Roeck wrote:
> On 7/21/24 20:30, Tzung-Bi Shih wrote:
> > On Sun, Jul 21, 2024 at 12:35:06PM -0700, Guenter Roeck wrote:
> > > Chips reporting overcurrent alarms report it in the second alarm register.
> > 
> > I can't understand the sentence.  Not sure whether it needs to be rephrased or
> > not.  s/overcurrent/overvoltage/.
> > 
> 
> No, it is over-current. Not all chips support current measurements.
> Those who do support it also support reporting over-current alarms.
> Over-current alarms are reported in the second alarm register.

Table 16 in [1] and Table 11 in [2] use "overvoltage".  Please ignore the
comments if I'm misunderstanding.

[1]: https://www.analog.com/media/en/technical-documentation/data-sheets/max16067.pdf
[2]: https://www.analog.com/media/en/technical-documentation/data-sheets/max16068.pdf

> Do you have a suggestion for better wording ?

No, or I guess I can understand the sentence strucutre a bit now:
Chips (reporting overcurrent alarms) report it in the second alarm register.

Re: [PATCH 2/2] hwmon: (max16065) Fix alarm attributes

[Guenter Roeck]

On 7/21/24 22:47, Tzung-Bi Shih wrote:
> On Sun, Jul 21, 2024 at 08:48:51PM -0700, Guenter Roeck wrote:
>> On 7/21/24 20:30, Tzung-Bi Shih wrote:
>>> On Sun, Jul 21, 2024 at 12:35:06PM -0700, Guenter Roeck wrote:
>>>> Chips reporting overcurrent alarms report it in the second alarm register.
>>>
>>> I can't understand the sentence.  Not sure whether it needs to be rephrased or
>>> not.  s/overcurrent/overvoltage/.
>>>
>>
>> No, it is over-current. Not all chips support current measurements.
>> Those who do support it also support reporting over-current alarms.
>> Over-current alarms are reported in the second alarm register.
> 
> Table 16 in [1] and Table 11 in [2] use "overvoltage".  Please ignore the
> comments if I'm misunderstanding.
> 
> [1]: https://www.analog.com/media/en/technical-documentation/data-sheets/max16067.pdf
> [2]: https://www.analog.com/media/en/technical-documentation/data-sheets/max16068.pdf
> 
Yes, those two chips don't support current or over-current reporting.
Only 160{65,66,70,71} support it.

Thanks,
Guenter