Re: [PATCH 5/5] hwmon: (pmbus) add support for MAX20826 and similar devices

Thank you for your contribution! Sashiko AI review found 6 potential issue(s) to consider:
- [Critical] Fatal NULL pointer dereference during probe for devices configured in page mode due to premature `pmbus_set_page()` calls.
- [High] The driver forcefully turns on voltage regulator outputs and unconditionally asserts hardware enable GPIOs during probe, bypassing system power sequencing.
- [High] Probe fails on AVSBus-enabled systems due to a missing register workaround during phase detection.
- [Medium] Inconsistent register redirection in custom PMBus callbacks breaks power limit configuration when Over Power Limit (OPL) is enabled.
- [Medium] Global PMBus commands in direct addressing mode are arbitrarily routed to a non-deterministic I2C client.
- [Medium] Hardware state (GPIO) is modified without rollback if the subsequent PMBus I2C operation fails.
--

--- Patch [5]: [PATCH 5/5] hwmon: (pmbus) add support for MAX20826 and similar devices ---
commit 8b8b70e16f48ffac7a09e30045ceff6976a47f0b
Author: Nuno Sá <[email protected]>
Subject: hwmon: (pmbus) add support for MAX20826 and similar devices

The MAX20826 IC and similar provide a high-density, flexible and scalable
dual-loop solution for high current cores for AI applications. These are
dual loop solutions multiphase voltage regulators. Between Rails A and B,
MAX20855B and MAX20908 supports up to 8 phases total configurable from
8+0 to 4+4 phases, MAX20912 supports up to 12 phases from 12+0 to 6+6,
and MAX20826 and MAX20916 supports up to 16 phases from 16+0 to 8+8.

> diff --git a/drivers/hwmon/pmbus/max20826.c b/drivers/hwmon/pmbus/max20826.c
> new file mode 100644
> index 0000000000000..1b481cdd8c930
> --- /dev/null
> +++ b/drivers/hwmon/pmbus/max20826.c
> @@ -156,12 +156,12 @@ static struct i2c_client *max20826_select_rail(struct max20826 *st,
>  	if (st->client_b) {
>  		/* if 0xff just return the last client */
>  		if (page < 0)
>  			return st->curr_client;

[Severity: Medium]
Will global commands be arbitrarily routed to a non-deterministic I2C client?

For global commands, the PMBus core passes page = -1. When page < 0, this
simply returns st->curr_client without updating it, which holds whichever
rail was accessed last. If userspace reads PMBUS_STATUS_CML globally, won't
it only query the CML status of the most recently accessed rail, potentially
masking communication faults on the other rail? Similarly, will global write
protection apply to only one rail?

>  		if (page)
>  			st->curr_client = st->client_b;
>  		else
>  			st->curr_client = st->client;
>  
>  		return st->curr_client;
>  	}
>  
>  	if (!probing)
>  		ret = pmbus_set_page(st->client, page, 0xff);

[Severity: Critical]
Does this cause a fatal NULL pointer dereference during probe for devices
configured in page mode?

During probe, max20826_setup() -> max20826_setup_vout_format() calls
__max20826_read_block_data() (as seen below). The wrapper hardcodes
probing = false. Because of this flag, max20826_select_rail() calls
pmbus_set_page(). 

pmbus_set_page() internally retrieves pmbus_data via i2c_get_clientdata().
However, because pmbus_do_probe() has not been executed yet, clientdata is
NULL, leading to a fatal dereference of data->info when pmbus_set_page()
is called.

>  	else
>  		ret = i2c_smbus_write_byte_data(st->client, PMBUS_PAGE, page);

[ ... ]

> @@ -201,11 +201,11 @@ static int __max20826_read_block_data(struct max20826 *st, int page,
>  	struct i2c_client *rail;
>  
>  	rail = max20826_select_rail(st, page, false);
>  	if (IS_ERR(rail))
>  		return PTR_ERR(rail);

[ ... ]

> @@ -396,17 +396,28 @@ static int max20826_read_word_data(struct i2c_client *client, int page,
[ ... ]
>  	case PMBUS_POUT_OP_FAULT_LIMIT:
>  		/*
>  		 * If Over Power Limit is enabled, PMBUS_IOUT_OC_FAULT_LIMIT
>  		 * shows the power limit and hence we need to report it
>  		 * properly in Watts.
>  		 */
>  		if (!st->chip_info->has_opl)
>  			return __max20826_read_word_data(st, page, reg);
>  
>  		if (st->opl_enabled[page])
>  			return __max20826_read_word_data(st, page,
>  							 PMBUS_IOUT_OC_FAULT_LIMIT);
>  		return -EIO;

[ ... ]

> @@ -417,14 +428,14 @@ static int max20826_write_word_data(struct i2c_client *client, int page,
>  				    int reg, u16 word)
>  {
>  	const struct pmbus_driver_info *info = pmbus_get_driver_info(client);
>  	struct max20826 *st = to_max20826(info);
>  	struct i2c_client *rail;
>  
>  	rail = max20826_select_rail(st, page, false);
>  	if (IS_ERR(rail))
>  		return PTR_ERR(rail);
>  
>  	return i2c_smbus_write_word_data(rail, reg, word);
>  }

[Severity: Medium]
Will writes to the power limit target the wrong PMBus register directly on
the hardware when Over Power Limit (OPL) is enabled?

While max20826_read_word_data() properly implements the redirection mapping
from PMBUS_POUT_OP_FAULT_LIMIT to PMBUS_IOUT_OC_FAULT_LIMIT when OPL is
enabled, max20826_write_word_data() blindly passes all writes straight to
the wire without interception. Should the redirection logic also be applied
to the write callback?

[ ... ]

> @@ -451,10 +462,10 @@ static int max20826_regulator_enable(struct max20826 *st, int page, u8 byte)
>  	if (page)
>  		gpiod_set_value_cansleep(st->bvren, !!(PB_OPERATION_CONTROL_ON & byte));
>  	else
>  		gpiod_set_value_cansleep(st->avren, !!(PB_OPERATION_CONTROL_ON & byte));
>  
>  	return i2c_smbus_write_byte_data(rail, PMBUS_OPERATION, byte);
>  }

[Severity: Medium]
Can the hardware state (GPIO) become desynchronized with the software PMBus
state if the subsequent I2C operation fails?

If i2c_smbus_write_byte_data() returns a negative error code, the regulator
core treats the operation as failed, but the hardware GPIO pin was left in
the newly toggled state without being rolled back.

[ ... ]

> @@ -807,11 +807,11 @@ static int max20826_detect_phases(struct max20826 *st, struct i2c_client *rail,
>  	if (!(ret & PB_OPERATION_CONTROL_ON)) {
>  		oper_save = ret;
>  		ret |= PB_OPERATION_CONTROL_ON;
>  		ret = i2c_smbus_write_byte_data(rail, PMBUS_OPERATION, ret);

[Severity: High]
Does this unconditionally force the PMBus rail ON during phase detection,
potentially bypassing system power sequencing rules?

In max20826_probe() and max20826_setup() (as seen below), the driver
unconditionally requests the optional avren and bvren GPIOs with
GPIOD_OUT_HIGH. Then, here in max20826_detect_phases(), it forces the
PMBus OPERATION register to ON just to read the active phase count. While
it restores the PMBus register state, the GPIOs are left permanently asserted
high. Could this violate strict SoC power sequencing rules and damage the
processor or downstream loads?

Also, as documented in max20826_regulator_enable(), if AVSBus is active
(bits 4 and 5 set), the device hardware refuses to set bit 7 unless the
AVSBus bits are first cleared. Because max20826_detect_phases() fails to
apply this workaround before the I2C write, won't the device ignore the ON
command, resulting in a read of 0 phases and aborting the probe on
AVSBus-enabled systems?

>  		if (ret < 0)
>  			return ret;
>  	}

[ ... ]

> @@ -898,11 +898,11 @@ static int max20826_setup_vout_format(struct max20826 *st)
>  	if (st->chip_info->is_vout_direct) {
>  		u8 vout_res[2];
>  
>  		ret = __max20826_read_block_data(st, RAIL_A,
>  						 MAX20826_REG_VOUT_RES,
>  						 vout_res, sizeof(vout_res));

[ ... ]

> @@ -946,8 +946,8 @@ static int max20826_setup(struct max20826 *st)
>  	/* Let's see if there's something on RAIL_B */
>  	st->bvren = devm_gpiod_get_optional(dev, "bvren", GPIOD_OUT_HIGH);
>  	if (IS_ERR(st->bvren))
>  		return PTR_ERR(st->bvren);

[ ... ]

> @@ -999,8 +999,8 @@ static int max20826_probe(struct i2c_client *client)
>  	st->chip_info = i2c_get_match_data(client);
>  	if (!st->chip_info)
>  		return -EINVAL;
>  
>  	st->avren = devm_gpiod_get_optional(&client->dev, "avren", GPIOD_OUT_HIGH);
>  	if (IS_ERR(st->avren))
>  		return PTR_ERR(st->avren);

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/[email protected]?part=5