hwmon: adm1177: fix sysfs ABI violation and current unit conversion

[PATCH] hwmon: adm1177: fix sysfs ABI violation and current unit conversion

Posted by Pradhan, Sanman 1 week, 2 days ago

From: Sanman Pradhan <psanman@juniper.net>

The adm1177 driver exposes the current alert threshold using
hwmon_curr_max_alarm. Per the hwmon sysfs ABI, *_alarm attributes
are read-only status flags; the writable threshold should use
hwmon_curr_max instead.

Additionally, the threshold is stored internally in microamps
(alert_threshold_ua) but the ABI requires milliamps for currN_max.
Convert appropriately on both the read and write paths, and
propagate the return value of adm1177_write_alert_thr() which was
previously discarded.

Clamp write values to the range the hardware can represent rather
than rejecting out-of-range input, and use DIV_ROUND_CLOSEST on the
read path to minimise rounding error during the uA-to-mA conversion.

Fixes: 09b08ac9e8d5 ("hwmon: (adm1177) Add ADM1177 Hot Swap Controller and Digital Power Monitor driver")
Signed-off-by: Sanman Pradhan <psanman@juniper.net>
---
 drivers/hwmon/adm1177.c | 16 +++++++++-------
 1 file changed, 9 insertions(+), 7 deletions(-)

diff --git a/drivers/hwmon/adm1177.c b/drivers/hwmon/adm1177.c
index 8b2c965480e3f..8742b8b5314b6 100644
--- a/drivers/hwmon/adm1177.c
+++ b/drivers/hwmon/adm1177.c
@@ -10,6 +10,7 @@
 #include <linux/hwmon.h>
 #include <linux/i2c.h>
 #include <linux/init.h>
+#include <linux/minmax.h>
 #include <linux/module.h>
 #include <linux/regulator/consumer.h>
 
@@ -91,8 +92,8 @@ static int adm1177_read(struct device *dev, enum hwmon_sensor_types type,
 			*val = div_u64((105840000ull * dummy),
 				       4096 * st->r_sense_uohm);
 			return 0;
-		case hwmon_curr_max_alarm:
-			*val = st->alert_threshold_ua;
+		case hwmon_curr_max:
+			*val = DIV_ROUND_CLOSEST(st->alert_threshold_ua, 1000);
 			return 0;
 		default:
 			return -EOPNOTSUPP;
@@ -126,9 +127,10 @@ static int adm1177_write(struct device *dev, enum hwmon_sensor_types type,
 	switch (type) {
 	case hwmon_curr:
 		switch (attr) {
-		case hwmon_curr_max_alarm:
-			adm1177_write_alert_thr(st, val);
-			return 0;
+		case hwmon_curr_max:
+			val = clamp_val(val, 0,
+					div_u64(105840000ULL, st->r_sense_uohm));
+			return adm1177_write_alert_thr(st, val * 1000);
 		default:
 			return -EOPNOTSUPP;
 		}
@@ -156,7 +158,7 @@ static umode_t adm1177_is_visible(const void *data,
 			if (st->r_sense_uohm)
 				return 0444;
 			return 0;
-		case hwmon_curr_max_alarm:
+		case hwmon_curr_max:
 			if (st->r_sense_uohm)
 				return 0644;
 			return 0;
@@ -170,7 +172,7 @@ static umode_t adm1177_is_visible(const void *data,
 
 static const struct hwmon_channel_info * const adm1177_info[] = {
 	HWMON_CHANNEL_INFO(curr,
-			   HWMON_C_INPUT | HWMON_C_MAX_ALARM),
+			   HWMON_C_INPUT | HWMON_C_MAX),
 	HWMON_CHANNEL_INFO(in,
 			   HWMON_I_INPUT),
 	NULL
-- 
2.34.1

Re: [PATCH] hwmon: adm1177: fix sysfs ABI violation and current unit conversion

Posted by Nuno Sá 1 week, 1 day ago

On Tue, 2026-03-24 at 18:22 +0000, Pradhan, Sanman wrote:
> From: Sanman Pradhan <psanman@juniper.net>
> 
> The adm1177 driver exposes the current alert threshold using
> hwmon_curr_max_alarm. Per the hwmon sysfs ABI, *_alarm attributes
> are read-only status flags; the writable threshold should use
> hwmon_curr_max instead.
> 
> Additionally, the threshold is stored internally in microamps
> (alert_threshold_ua) but the ABI requires milliamps for currN_max.
> Convert appropriately on both the read and write paths, and
> propagate the return value of adm1177_write_alert_thr() which was
> previously discarded.
> 
> Clamp write values to the range the hardware can represent rather
> than rejecting out-of-range input, and use DIV_ROUND_CLOSEST on the
> read path to minimise rounding error during the uA-to-mA conversion.
> 
> Fixes: 09b08ac9e8d5 ("hwmon: (adm1177) Add ADM1177 Hot Swap Controller and Digital Power Monitor
> driver")
> Signed-off-by: Sanman Pradhan <psanman@juniper.net>
> ---

For the AI comment, typically these applications don't go to ohms for rsense so, in practice, it
might be that we never get he overflow. But I would still play safe given it's so trivial. I also
see you only replace hwmon_curr_max_alarm with hwmon_curr_max. It would be nicer to first fix ABI
and then support hwmon_curr_max_alarm (properly). Though might be a big ask if you don't have HW to
test it. Anyways, after AI feedback addressed:

Acked-by: Nuno Sá <nuno.sa@analog.com>

>  drivers/hwmon/adm1177.c | 16 +++++++++-------
>  1 file changed, 9 insertions(+), 7 deletions(-)
> 
> diff --git a/drivers/hwmon/adm1177.c b/drivers/hwmon/adm1177.c
> index 8b2c965480e3f..8742b8b5314b6 100644
> --- a/drivers/hwmon/adm1177.c
> +++ b/drivers/hwmon/adm1177.c
> @@ -10,6 +10,7 @@
>  #include <linux/hwmon.h>
>  #include <linux/i2c.h>
>  #include <linux/init.h>
> +#include <linux/minmax.h>
>  #include <linux/module.h>
>  #include <linux/regulator/consumer.h>
>  
> @@ -91,8 +92,8 @@ static int adm1177_read(struct device *dev, enum hwmon_sensor_types type,
>  			*val = div_u64((105840000ull * dummy),
>  				       4096 * st->r_sense_uohm);
>  			return 0;
> -		case hwmon_curr_max_alarm:
> -			*val = st->alert_threshold_ua;
> +		case hwmon_curr_max:
> +			*val = DIV_ROUND_CLOSEST(st->alert_threshold_ua, 1000);
>  			return 0;
>  		default:
>  			return -EOPNOTSUPP;
> @@ -126,9 +127,10 @@ static int adm1177_write(struct device *dev, enum hwmon_sensor_types type,
>  	switch (type) {
>  	case hwmon_curr:
>  		switch (attr) {
> -		case hwmon_curr_max_alarm:
> -			adm1177_write_alert_thr(st, val);
> -			return 0;
> +		case hwmon_curr_max:
> +			val = clamp_val(val, 0,
> +					div_u64(105840000ULL, st->r_sense_uohm));
> +			return adm1177_write_alert_thr(st, val * 1000);
>  		default:
>  			return -EOPNOTSUPP;
>  		}
> @@ -156,7 +158,7 @@ static umode_t adm1177_is_visible(const void *data,
>  			if (st->r_sense_uohm)
>  				return 0444;
>  			return 0;
> -		case hwmon_curr_max_alarm:
> +		case hwmon_curr_max:
>  			if (st->r_sense_uohm)
>  				return 0644;
>  			return 0;
> @@ -170,7 +172,7 @@ static umode_t adm1177_is_visible(const void *data,
>  
>  static const struct hwmon_channel_info * const adm1177_info[] = {
>  	HWMON_CHANNEL_INFO(curr,
> -			   HWMON_C_INPUT | HWMON_C_MAX_ALARM),
> +			   HWMON_C_INPUT | HWMON_C_MAX),
>  	HWMON_CHANNEL_INFO(in,
>  			   HWMON_I_INPUT),
>  	NULL

Re: [PATCH] hwmon: adm1177: fix sysfs ABI violation and current unit conversion

Posted by Guenter Roeck 1 week, 1 day ago

On 3/25/26 03:37, Nuno Sá wrote:
> On Tue, 2026-03-24 at 18:22 +0000, Pradhan, Sanman wrote:
>> From: Sanman Pradhan <psanman@juniper.net>
>>
>> The adm1177 driver exposes the current alert threshold using
>> hwmon_curr_max_alarm. Per the hwmon sysfs ABI, *_alarm attributes
>> are read-only status flags; the writable threshold should use
>> hwmon_curr_max instead.
>>
>> Additionally, the threshold is stored internally in microamps
>> (alert_threshold_ua) but the ABI requires milliamps for currN_max.
>> Convert appropriately on both the read and write paths, and
>> propagate the return value of adm1177_write_alert_thr() which was
>> previously discarded.
>>
>> Clamp write values to the range the hardware can represent rather
>> than rejecting out-of-range input, and use DIV_ROUND_CLOSEST on the
>> read path to minimise rounding error during the uA-to-mA conversion.
>>
>> Fixes: 09b08ac9e8d5 ("hwmon: (adm1177) Add ADM1177 Hot Swap Controller and Digital Power Monitor
>> driver")
>> Signed-off-by: Sanman Pradhan <psanman@juniper.net>
>> ---
> 
> For the AI comment, typically these applications don't go to ohms for rsense so, in practice, it

Limiting rsense to a reasonable value (1 Ohm might do)might just be good enough.
That is really unrelated to this change, so it should be a separate patch.

> might be that we never get he overflow. But I would still play safe given it's so trivial. I also
> see you only replace hwmon_curr_max_alarm with hwmon_curr_max. It would be nicer to first fix ABI
> and then support hwmon_curr_max_alarm (properly). Though might be a big ask if you don't have HW to
> test it. Anyways, after AI feedback addressed:

Ah yes, good point. The chip _does_ support actual alerts, so that would be desirable.
However, that should also be a separate patch and, yes, it would be better to have an
actual chip at hand to make sure that it works as intended.

I'll apply this patch as-is.

Thanks,
Guenter

> 
> Acked-by: Nuno Sá <nuno.sa@analog.com>
> 
>>   drivers/hwmon/adm1177.c | 16 +++++++++-------
>>   1 file changed, 9 insertions(+), 7 deletions(-)
>>
>> diff --git a/drivers/hwmon/adm1177.c b/drivers/hwmon/adm1177.c
>> index 8b2c965480e3f..8742b8b5314b6 100644
>> --- a/drivers/hwmon/adm1177.c
>> +++ b/drivers/hwmon/adm1177.c
>> @@ -10,6 +10,7 @@
>>   #include <linux/hwmon.h>
>>   #include <linux/i2c.h>
>>   #include <linux/init.h>
>> +#include <linux/minmax.h>
>>   #include <linux/module.h>
>>   #include <linux/regulator/consumer.h>
>>   
>> @@ -91,8 +92,8 @@ static int adm1177_read(struct device *dev, enum hwmon_sensor_types type,
>>   			*val = div_u64((105840000ull * dummy),
>>   				       4096 * st->r_sense_uohm);
>>   			return 0;
>> -		case hwmon_curr_max_alarm:
>> -			*val = st->alert_threshold_ua;
>> +		case hwmon_curr_max:
>> +			*val = DIV_ROUND_CLOSEST(st->alert_threshold_ua, 1000);
>>   			return 0;
>>   		default:
>>   			return -EOPNOTSUPP;
>> @@ -126,9 +127,10 @@ static int adm1177_write(struct device *dev, enum hwmon_sensor_types type,
>>   	switch (type) {
>>   	case hwmon_curr:
>>   		switch (attr) {
>> -		case hwmon_curr_max_alarm:
>> -			adm1177_write_alert_thr(st, val);
>> -			return 0;
>> +		case hwmon_curr_max:
>> +			val = clamp_val(val, 0,
>> +					div_u64(105840000ULL, st->r_sense_uohm));
>> +			return adm1177_write_alert_thr(st, val * 1000);
>>   		default:
>>   			return -EOPNOTSUPP;
>>   		}
>> @@ -156,7 +158,7 @@ static umode_t adm1177_is_visible(const void *data,
>>   			if (st->r_sense_uohm)
>>   				return 0444;
>>   			return 0;
>> -		case hwmon_curr_max_alarm:
>> +		case hwmon_curr_max:
>>   			if (st->r_sense_uohm)
>>   				return 0644;
>>   			return 0;
>> @@ -170,7 +172,7 @@ static umode_t adm1177_is_visible(const void *data,
>>   
>>   static const struct hwmon_channel_info * const adm1177_info[] = {
>>   	HWMON_CHANNEL_INFO(curr,
>> -			   HWMON_C_INPUT | HWMON_C_MAX_ALARM),
>> +			   HWMON_C_INPUT | HWMON_C_MAX),
>>   	HWMON_CHANNEL_INFO(in,
>>   			   HWMON_I_INPUT),
>>   	NULL

Re: [PATCH] hwmon: adm1177: fix sysfs ABI violation and current unit conversion

Posted by Nuno Sá 1 week, 1 day ago

On Wed, 2026-03-25 at 06:46 -0700, Guenter Roeck wrote:
> On 3/25/26 03:37, Nuno Sá wrote:
> > On Tue, 2026-03-24 at 18:22 +0000, Pradhan, Sanman wrote:
> > > From: Sanman Pradhan <psanman@juniper.net>
> > > 
> > > The adm1177 driver exposes the current alert threshold using
> > > hwmon_curr_max_alarm. Per the hwmon sysfs ABI, *_alarm attributes
> > > are read-only status flags; the writable threshold should use
> > > hwmon_curr_max instead.
> > > 
> > > Additionally, the threshold is stored internally in microamps
> > > (alert_threshold_ua) but the ABI requires milliamps for currN_max.
> > > Convert appropriately on both the read and write paths, and
> > > propagate the return value of adm1177_write_alert_thr() which was
> > > previously discarded.
> > > 
> > > Clamp write values to the range the hardware can represent rather
> > > than rejecting out-of-range input, and use DIV_ROUND_CLOSEST on the
> > > read path to minimise rounding error during the uA-to-mA conversion.
> > > 
> > > Fixes: 09b08ac9e8d5 ("hwmon: (adm1177) Add ADM1177 Hot Swap Controller and Digital Power
> > > Monitor
> > > driver")
> > > Signed-off-by: Sanman Pradhan <psanman@juniper.net>
> > > ---
> > 
> > For the AI comment, typically these applications don't go to ohms for rsense so, in practice, it
> 
> Limiting rsense to a reasonable value (1 Ohm might do)might just be good enough.
> That is really unrelated to this change, so it should be a separate patch.
> 
> > might be that we never get he overflow. But I would still play safe given it's so trivial. I
> > also
> > see you only replace hwmon_curr_max_alarm with hwmon_curr_max. It would be nicer to first fix
> > ABI
> > and then support hwmon_curr_max_alarm (properly). Though might be a big ask if you don't have HW
> > to
> > test it. Anyways, after AI feedback addressed:
> 
> Ah yes, good point. The chip _does_ support actual alerts, so that would be desirable.
> However, that should also be a separate patch and, yes, it would be better to have an
> actual chip at hand to make sure that it works as intended.
> 
> I'll apply this patch as-is.
> 

There is a v2 already which I also missed at first. And the AI comment I mention refers to that
patch. I replied in there but FWIW, I do agree with all of the above.

- Nuno Sá

> 
> > 
> > Acked-by: Nuno Sá <nuno.sa@analog.com>
> > 
> > >   drivers/hwmon/adm1177.c | 16 +++++++++-------
> > >   1 file changed, 9 insertions(+), 7 deletions(-)
> > > 
> > > diff --git a/drivers/hwmon/adm1177.c b/drivers/hwmon/adm1177.c
> > > index 8b2c965480e3f..8742b8b5314b6 100644
> > > --- a/drivers/hwmon/adm1177.c
> > > +++ b/drivers/hwmon/adm1177.c
> > > @@ -10,6 +10,7 @@
> > >   #include <linux/hwmon.h>
> > >   #include <linux/i2c.h>
> > >   #include <linux/init.h>
> > > +#include <linux/minmax.h>
> > >   #include <linux/module.h>
> > >   #include <linux/regulator/consumer.h>
> > >   
> > > @@ -91,8 +92,8 @@ static int adm1177_read(struct device *dev, enum hwmon_sensor_types type,
> > >   			*val = div_u64((105840000ull * dummy),
> > >   				       4096 * st->r_sense_uohm);
> > >   			return 0;
> > > -		case hwmon_curr_max_alarm:
> > > -			*val = st->alert_threshold_ua;
> > > +		case hwmon_curr_max:
> > > +			*val = DIV_ROUND_CLOSEST(st->alert_threshold_ua, 1000);
> > >   			return 0;
> > >   		default:
> > >   			return -EOPNOTSUPP;
> > > @@ -126,9 +127,10 @@ static int adm1177_write(struct device *dev, enum hwmon_sensor_types
> > > type,
> > >   	switch (type) {
> > >   	case hwmon_curr:
> > >   		switch (attr) {
> > > -		case hwmon_curr_max_alarm:
> > > -			adm1177_write_alert_thr(st, val);
> > > -			return 0;
> > > +		case hwmon_curr_max:
> > > +			val = clamp_val(val, 0,
> > > +					div_u64(105840000ULL, st->r_sense_uohm));
> > > +			return adm1177_write_alert_thr(st, val * 1000);
> > >   		default:
> > >   			return -EOPNOTSUPP;
> > >   		}
> > > @@ -156,7 +158,7 @@ static umode_t adm1177_is_visible(const void *data,
> > >   			if (st->r_sense_uohm)
> > >   				return 0444;
> > >   			return 0;
> > > -		case hwmon_curr_max_alarm:
> > > +		case hwmon_curr_max:
> > >   			if (st->r_sense_uohm)
> > >   				return 0644;
> > >   			return 0;
> > > @@ -170,7 +172,7 @@ static umode_t adm1177_is_visible(const void *data,
> > >   
> > >   static const struct hwmon_channel_info * const adm1177_info[] = {
> > >   	HWMON_CHANNEL_INFO(curr,
> > > -			   HWMON_C_INPUT | HWMON_C_MAX_ALARM),
> > > +			   HWMON_C_INPUT | HWMON_C_MAX),
> > >   	HWMON_CHANNEL_INFO(in,
> > >   			   HWMON_I_INPUT),
> > >   	NULL

Re: [PATCH] hwmon: adm1177: fix sysfs ABI violation and current unit conversion

Posted by Guenter Roeck 1 week, 2 days ago

On 3/24/26 11:22, Pradhan, Sanman wrote:
> From: Sanman Pradhan <psanman@juniper.net>
> 
> The adm1177 driver exposes the current alert threshold using
> hwmon_curr_max_alarm. Per the hwmon sysfs ABI, *_alarm attributes
> are read-only status flags; the writable threshold should use
> hwmon_curr_max instead.
> 
> Additionally, the threshold is stored internally in microamps
> (alert_threshold_ua) but the ABI requires milliamps for currN_max.
> Convert appropriately on both the read and write paths, and
> propagate the return value of adm1177_write_alert_thr() which was
> previously discarded.
> 
> Clamp write values to the range the hardware can represent rather
> than rejecting out-of-range input, and use DIV_ROUND_CLOSEST on the
> read path to minimise rounding error during the uA-to-mA conversion.
> 
> Fixes: 09b08ac9e8d5 ("hwmon: (adm1177) Add ADM1177 Hot Swap Controller and Digital Power Monitor driver")
> Signed-off-by: Sanman Pradhan <psanman@juniper.net>

AI: https://sashiko.dev/#/patchset/20260324182231.228195-1-sanman.pradhan%40hpe.com

Various possible under/overflows.

Thanks,
Guenter

> ---
>   drivers/hwmon/adm1177.c | 16 +++++++++-------
>   1 file changed, 9 insertions(+), 7 deletions(-)
> 
> diff --git a/drivers/hwmon/adm1177.c b/drivers/hwmon/adm1177.c
> index 8b2c965480e3f..8742b8b5314b6 100644
> --- a/drivers/hwmon/adm1177.c
> +++ b/drivers/hwmon/adm1177.c
> @@ -10,6 +10,7 @@
>   #include <linux/hwmon.h>
>   #include <linux/i2c.h>
>   #include <linux/init.h>
> +#include <linux/minmax.h>
>   #include <linux/module.h>
>   #include <linux/regulator/consumer.h>
>   
> @@ -91,8 +92,8 @@ static int adm1177_read(struct device *dev, enum hwmon_sensor_types type,
>   			*val = div_u64((105840000ull * dummy),
>   				       4096 * st->r_sense_uohm);
>   			return 0;
> -		case hwmon_curr_max_alarm:
> -			*val = st->alert_threshold_ua;
> +		case hwmon_curr_max:
> +			*val = DIV_ROUND_CLOSEST(st->alert_threshold_ua, 1000);
>   			return 0;
>   		default:
>   			return -EOPNOTSUPP;
> @@ -126,9 +127,10 @@ static int adm1177_write(struct device *dev, enum hwmon_sensor_types type,
>   	switch (type) {
>   	case hwmon_curr:
>   		switch (attr) {
> -		case hwmon_curr_max_alarm:
> -			adm1177_write_alert_thr(st, val);
> -			return 0;
> +		case hwmon_curr_max:
> +			val = clamp_val(val, 0,
> +					div_u64(105840000ULL, st->r_sense_uohm));
> +			return adm1177_write_alert_thr(st, val * 1000);
>   		default:
>   			return -EOPNOTSUPP;
>   		}
> @@ -156,7 +158,7 @@ static umode_t adm1177_is_visible(const void *data,
>   			if (st->r_sense_uohm)
>   				return 0444;
>   			return 0;
> -		case hwmon_curr_max_alarm:
> +		case hwmon_curr_max:
>   			if (st->r_sense_uohm)
>   				return 0644;
>   			return 0;
> @@ -170,7 +172,7 @@ static umode_t adm1177_is_visible(const void *data,
>   
>   static const struct hwmon_channel_info * const adm1177_info[] = {
>   	HWMON_CHANNEL_INFO(curr,
> -			   HWMON_C_INPUT | HWMON_C_MAX_ALARM),
> +			   HWMON_C_INPUT | HWMON_C_MAX),
>   	HWMON_CHANNEL_INFO(in,
>   			   HWMON_I_INPUT),
>   	NULL