Re: [RFC PATCH net-next v2 2/2] net: pse-pd: Add LTC4266 PSE controller driver

Thank you for your contribution! Sashiko AI review found 6 potential issue(s) to consider:
- [High] I2C read failures in the IRQ handler silently discard clear-on-read hardware events and return 0, risking an IRQ storm.
- [High] Incorrect devres registration sequence exposes the PSE controller before initialization completes and induces use-after-free during teardown.
- [Medium] Admin power limit configuration is inappropriately bounded by the currently connected PD class, preventing configuration on idle ports.
- [Medium] `ltc4266_pi_get_voltage` returns the voltage in microvolts (uV) instead of millivolts (mV).
- [Medium] Routine PD disconnections trigger an unconditional `dev_err` log.
- [Medium] Premature rounding in power calculation causes unnecessary precision loss.
--

--- Patch [2]: [RFC PATCH net-next v2 2/2] net: pse-pd: Add LTC4266 PSE controller driver ---
commit 5319fc10897e2c97c798780d619129c31798bf5a
Author: Kyle Swenson <[email protected]>

net: pse-pd: Add LTC4266 PSE controller driver

Add a new driver for the Linear Technology LTC4266 I2C Power Sourcing
Equipment controller and integrate with the PSE controller core. The
LTC4266 supports Type 1 and Type 2 Powered Devices.

> diff --git a/drivers/net/pse-pd/ltc4266.c b/drivers/net/pse-pd/ltc4266.c
> new file mode 100644
> index 0000000000000..2f9c37ebf1d29
> --- /dev/null
> +++ b/drivers/net/pse-pd/ltc4266.c

[ ... ]

> +static int ltc4266_pi_get_voltage(struct pse_controller_dev *pcdev, int id)
> +{
> +	return ltc4266_read_iv(ltc4266_pi_port(pcdev, id), READ_VOLTAGE);
> +}

[Severity: Medium]
ltc4266_read_iv() returns the voltage in microvolts. Does the PSE core API
expect this value in millivolts instead?

Returning microvolts here might cause ethtool to display a voltage 1000 times
larger than the actual value.

[ ... ]

> +static int ltc4266_pi_get_actual_pw(struct pse_controller_dev *pcdev, int id)
> +{
> +	struct ltc4266_port *port = ltc4266_pi_port(pcdev, id);
> +	int uA, uV;
> +	u64 uW;
> +
> +	uA = ltc4266_read_iv(port, READ_CURRENT);
> +	if (uA < 0)
> +		return uA;
> +
> +	uV = ltc4266_read_iv(port, READ_VOLTAGE);
> +	if (uV < 0)
> +		return uV;
> +
> +	/* Convert uA to mA and uV to mV; mA * mV = uW */
> +	uW = DIV_ROUND_CLOSEST_ULL(uA, 1000) * DIV_ROUND_CLOSEST_ULL(uV, 1000);
> +
> +	return (int)DIV_ROUND_CLOSEST_ULL(uW, 1000);
> +}

[Severity: Medium]
Could this calculation cause unnecessary precision loss?

Since uA * uV safely fits within a 64-bit unsigned integer, perhaps the
multiplication could happen before dividing by 1000 to avoid up to a 10%
mathematical error for smaller loads?

[ ... ]

> +static int ltc4266_pi_get_pw_limit_ranges(struct pse_controller_dev *pcdev, int id,
> +					  struct pse_pw_limit_ranges *pw_limit_ranges)
> +{
> +	struct ethtool_c33_pse_pw_limit_range *c33_pw_limit_ranges;
> +	int class_pw_limit;
> +
> +	c33_pw_limit_ranges = kzalloc_obj(*c33_pw_limit_ranges);
> +	if (!c33_pw_limit_ranges)
> +		return -ENOMEM;
> +
> +	class_pw_limit =  ltc4266_port_max_pw(ltc4266_pi_port(pcdev, id));
> +	if (class_pw_limit < 0)
> +		class_pw_limit = 0;
> +
> +	c33_pw_limit_ranges[0].min = LTC4266_PW_LIMIT_MIN;
> +	c33_pw_limit_ranges[0].max = class_pw_limit ? class_pw_limit : LTC4266_PW_LIMIT_MAX;

[Severity: Medium]
This bounds the advertised maximum limit by the currently connected device
class. Will this prevent users from configuring an administrative power limit
for a higher-class device on an idle port?

[ ... ]

> +static int ltc4266_pi_set_pw_limit(struct pse_controller_dev *pcdev,
> +				   int id, int max_mw)
> +{
> +	struct ltc4266_port *port = ltc4266_pi_port(pcdev, id);
> +	int class_pw_limit;
> +	int class;
> +	int icut;
> +	int ret;
> +
> +	class = ltc4266_port_get_class(port);
> +	if (class < 0)
> +		return class;
> +
> +	class_pw_limit = ltc4266_class_pw[class];
> +
> +	if (max_mw < LTC4266_PW_LIMIT_MIN || max_mw > class_pw_limit) {

[Severity: Medium]
Similar to the range function above, tying the administrative configuration
to the instantaneously connected device class seems to prevent configuring
limits on an idle port.

Should administrative limits be independent of the current PD state so
they can restrict future connected devices?

[ ... ]

> +static int ltc4266_map_event(int irq, struct pse_controller_dev *pcdev,
> +			     unsigned long *notifs, unsigned long *notifs_mask)
> +{
[ ... ]
> +	if (intstat & (LTC4266_INT_DIS | LTC4266_INT_TCUT)) {
> +		ret = regmap_read(ltc4266->regmap, LTC4266_REG_FLTEVN_COR, &fltevn);
> +		if (ret < 0) {
> +			dev_err(&ltc4266->client->dev, "Failed to read fltevn err=%d\n", ret);
> +			goto done;
> +		}
> +	}
[ ... ]
> +			if ((tsevn & (LTC4266_EVN_LO(chan) | LTC4266_EVN_HI(chan))) ||
> +			    (fltevn & LTC4266_EVN_LO(chan)))
> +				notifs[i] |= ETHTOOL_PSE_EVENT_OVER_CURRENT;
> +
> +			dev_err(&ltc4266->client->dev, "tsevn=0x%02X fltevn=0x%02X\n",
> +				tsevn, fltevn);
> +			continue;

[Severity: Medium]
Does this code unconditionally print a dev_err() for routine device
disconnections?

It seems like this might generate spurious error logs whenever a user simply
unplugs a cable from the device.

[ ... ]

> +done:
> +	ltc4266_enable_interrupts(ltc4266);
> +	return 0;
> +}

[Severity: High]
If the I2C read fails in the IRQ handler here, the clear-on-read interrupt
status register is not cleared at the hardware level.

Because the handler jumps to done and re-enables the top-level interrupts,
could this cause an immediate IRQ storm that locks up the system since the
hardware will continually reassert the unhandled event?

[ ... ]

> +static int ltc4266_probe(struct i2c_client *client)
> +{
[ ... ]
> +	ret = devm_pse_controller_register(ltc4266->dev, &ltc4266->pcdev);
> +	if (ret)
> +		return dev_err_probe(&client->dev, ret,
> +						"Failed to register PSE controller\n");
> +
> +	if (client->irq) {
> +		struct pse_irq_desc irq_desc = {
> +			.name = "ltc4266-irq",
> +			.map_event = ltc4266_map_event,
> +		};
> +
> +		/* Enable the interrupt pin, and only report detect events on
> +		 * change (detchg) so idle ports continuously re-running
> +		 * detection in semi-auto mode don't flood the host with a
> +		 * detect event every cycle.
> +		 */
> +		regmap_update_bits(ltc4266->regmap, LTC4266_REG_MCONF,
> +				   LTC4266_MCONF_INTERRUPT_ENABLE | LTC4266_MCONF_DETCHG,
> +				   LTC4266_MCONF_INTERRUPT_ENABLE | LTC4266_MCONF_DETCHG);
> +
> +		ret = devm_pse_irq_helper(&ltc4266->pcdev, client->irq,
> +					  0, &irq_desc);

[Severity: High]
The controller is registered with devres before the IRQ helper. Since devres
tears down resources in reverse order (LIFO), wouldn't the IRQ be freed
before the controller is unregistered?

If pse_controller_unregister() tries to disable the IRQ using pcdev->irq
during teardown, it might operate on a freed IRQ descriptor. Could this lead
to a use-after-free or accidentally disable an unrelated IRQ?

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