Re: [PATCH v0 1/2] hwmon: pmbus: Fix vout_mode parsing

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [High] The patch truncates the PMBus VOUT_MODE mode field from 3 bits to 2 bits, violating the PMBus specification, wrongly accepting reserved modes, and introducing dead code.
--

commit b7e943858dbfad31b0cd1d29753290823efc6361
Author: Aryan Srivastava <aryan.srivastava@alliedtelesis.co.nz>

hwmon: pmbus: Fix vout_mode parsing

This patch masks the vout_mode parsing in the PMBus core with 0x3, intending
to handle cases where devices unexpectedly set the top-most bit of the
VOUT_MODE register.

> diff --git a/drivers/hwmon/pmbus/pmbus.c b/drivers/hwmon/pmbus/pmbus.c
> index 7a7eac90d62e5..b629acbffbfb6 100644
> --- a/drivers/hwmon/pmbus/pmbus.c
> +++ b/drivers/hwmon/pmbus/pmbus.c
> @@ -121,7 +121,7 @@ static int pmbus_identify(struct i2c_client *client,
>  
>  		vout_mode = pmbus_read_byte_data(client, 0, PMBUS_VOUT_MODE);
>  		if (vout_mode >= 0 && vout_mode != 0xff) {
> -			switch (vout_mode >> 5) {
> +			switch ((vout_mode >> 5) & 0x3) {

[Severity: High]
Does this change incorrectly alias reserved VOUT_MODE values (4-7) to standard
modes (0-3)?

By masking the top 3 bits down to 2 bits, if a PMBus chip reports a reserved or
manufacturer-specific mode (such as mode 4), it will now be parsed as a
standard mode (like linear mode 0) instead of being properly rejected.
Misinterpreting the voltage format could cause erroneous voltage calculations,
which might lead to system instability or physical hardware damage from
overvoltage.

Should device-specific quirks (such as a specific device erroneously setting
bit 7) be handled via a custom .read_byte_data callback or a quirk in a
dedicated device driver, rather than breaking PMBus protocol compliance for
all devices in the generic pmbus_identify() logic?

>  			case 0:
>  				break;
>  			case 1:

[ ... ]

> diff --git a/drivers/hwmon/pmbus/pmbus_core.c b/drivers/hwmon/pmbus/pmbus_core.c
> index 806c9a4913bb0..60ddc49afb3f7 100644
> --- a/drivers/hwmon/pmbus/pmbus_core.c
> +++ b/drivers/hwmon/pmbus/pmbus_core.c
> @@ -2751,7 +2751,7 @@ static int pmbus_identify_common(struct i2c_client *client,
>  		 * Not all chips support the VOUT_MODE command,
>  		 * so a failure to read it is not an error.
>  		 */
> -		switch (vout_mode >> 5) {
> +		switch ((vout_mode >> 5) & 0x3) {

[Severity: High]
Does masking with 0x3 here introduce dead code in pmbus_identify_common()?

Because the evaluated value is strictly restricted to 0, 1, 2, or 3, and all
four of those cases are explicitly handled below this switch, the default case
and its error handling (return -ENODEV;) can never be reached.

>  		case 0:	/* linear mode      */
>  			if (data->info->format[PSC_VOLTAGE_OUT] != linear)
>  				return -ENODEV;

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260904005351.2136374-1-aryan.srivastava@alliedtelesis.co.nz?part=1