Re: [PATCH 2/2] hwmon: (pmbus/lx1308) Add support for LX1308

[sashiko-bot@kernel.org]

Sashiko AI review found 2 potential issue(s):
- [High] Left shifting a negative mantissa in `linear11_to_int()` invokes undefined behavior in C.
- [Low] Alphabetical sorting violation in Kconfig and Makefile.
--

commit 1f51fd323179e1fa2a4bde6554dcffd04d32b646
Author: Brian Chiang <chiang.brian@inventec.com>

hwmon: (pmbus/lx1308) Add support for LX1308
    
Add support for the Luxshare LX1308, a high-efficiency 12V 860W
DC/DC power module. The module operates from 40-60V input voltage.

> diff --git a/drivers/hwmon/pmbus/Kconfig b/drivers/hwmon/pmbus/Kconfig
> --- a/drivers/hwmon/pmbus/Kconfig
> +++ b/drivers/hwmon/pmbus/Kconfig
> @@ -221,6 +221,16 @@ config SENSORS_ISL68137
>  	  This driver can also be built as a module. If so, the module will
>  	  be called isl68137.
>  
> +config SENSORS_LX1308
> +	tristate "Luxshare LX1308 DC/DC Power Module"

This isn't a bug, but should this be placed further down? SENSORS_LX1308 is
inserted between SENSORS_ISL68137 and SENSORS_LM25066, which breaks
alphabetical ordering.

[ ... ]

> diff --git a/drivers/hwmon/pmbus/Makefile b/drivers/hwmon/pmbus/Makefile
> --- a/drivers/hwmon/pmbus/Makefile
> +++ b/drivers/hwmon/pmbus/Makefile
> @@ -23,6 +23,7 @@ obj-$(CONFIG_SENSORS_IR36021)	+= ir36021.o
>  obj-$(CONFIG_SENSORS_IR38064)	+= ir38064.o
>  obj-$(CONFIG_SENSORS_IRPS5401)	+= irps5401.o
>  obj-$(CONFIG_SENSORS_ISL68137)	+= isl68137.o
> +obj-$(CONFIG_SENSORS_LX1308)	+= lx1308.o
>  obj-$(CONFIG_SENSORS_LM25066)	+= lm25066.o

This isn't a bug, but should this also be placed further down to maintain
alphabetical ordering?

[ ... ]

> diff --git a/drivers/hwmon/pmbus/lx1308.c b/drivers/hwmon/pmbus/lx1308.c
> --- /dev/null
> +++ b/drivers/hwmon/pmbus/lx1308.c
[ ... ]
> +/*
> + * Decode a Linear11-encoded word to an integer value.
> + * Linear11 format: bits[15:11] = signed 5-bit exponent,
> + * bits[10:0] = signed 11-bit mantissa. Result = mant * 2^exp.
> + */
> +static inline int linear11_to_int(u16 word)
> +{
> +	s16 exp = ((s16)word) >> 11;
> +	s16 mant = ((s16)((word & 0x7ff) << 5)) >> 5;
> +
> +	return (exp >= 0) ? (mant << exp) : (mant / (1 << (-exp)));

When a user writes a negative threshold value to a writable sysfs limit
attribute, the PMBus core encodes it into a Linear11 word with a negative
mantissa.

If exp >= 0, this evaluates (mant << exp).

In C, left-shifting a negative signed integer invokes undefined behavior and
can trigger a UBSAN shift-out-of-bounds splat.

Could this invoke undefined behavior when mant is negative?

> +}

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260422-add-support-lx1308-v1-0-9b8322f45aae@inventec.com?part=2