Re: [PATCH 28/43] power: supply: pf1550-charger: Remove redundant dev_err()/dev_err_probe()

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 4 potential issue(s) to consider:

Pre-existing issues:
- [High] Inverted logic when turning on the charger based on battery presence in `pf1550_reg_init()`.
- [High] Use-after-free vulnerability during driver unbind or probe failure due to incorrect `devm` registration order.
- [High] Guaranteed probe failure for device trees omitting the optional `constant-charge-voltage-max-microvolt` property due to unhandled `-EINVAL`.
- [Medium] Resource leak of `struct power_supply_battery_info` due to uncalled `power_supply_put_battery_info()`.
--

--- Patch [28]: [PATCH 28/43] power: supply: pf1550-charger: Remove redundant dev_err()/dev_err_probe() ---
commit 7f7745c9eb293f3e6b24f5df79b35be592a07130
Author: Pan Chuang <panchuang@vivo.com>

power: supply: pf1550-charger: Remove redundant dev_err()/dev_err_probe()
    
The devm_request_threaded_irq() and devm_request_irq() now automatically
log detailed error messages on failure. This eliminates the need for
driver-specific dev_err() and dev_err_probe() calls.

> diff --git a/drivers/power/supply/pf1550-charger.c b/drivers/power/supply/pf1550-charger.c
> index 41036f4cb64a0..94fe81bab5a21 100644
> --- a/drivers/power/supply/pf1550-charger.c
> +++ b/drivers/power/supply/pf1550-charger.c
> @@ -612,8 +612,7 @@ static int pf1550_charger_probe(struct platform_device *pdev)
>  						IRQF_NO_SUSPEND,
>  						"pf1550-charger", chg);
>  		if (ret)
> -			return dev_err_probe(&pdev->dev, ret,
> -					     "failed irq request\n");
> +			return ret;
>  	}
>  
>  	pf1550_dt_parse_dev_info(chg);

[Severity: High]
This is a pre-existing issue, but does this devm registration order leave 
a use-after-free window during device unbind?

In drivers/power/supply/pf1550-charger.c:pf1550_charger_probe(), the delayed
works are registered before the power supplies:

	ret = devm_delayed_work_autocancel(chg->dev, &chg->vbus_sense_work,
					   pf1550_chg_vbus_work);
	...
	chg->charger = devm_power_supply_register(&pdev->dev,
						  &pf1550_charger_desc,
						  &psy_cfg);

Since devres cleans up in reverse order, the power supplies will be freed
first. If pf1550_chg_vbus_work executes before the work is canceled, it
will call power_supply_changed(chg->battery) on a freed power supply
pointer.


[Severity: Medium]
This is a pre-existing issue, but does this code leak the battery info
structure?

Looking at pf1550_dt_parse_dev_info(), it calls power_supply_get_battery_info():

	if (power_supply_get_battery_info(chg->charger, &info))
		chg->constant_volt = PF1550_DEFAULT_CONSTANT_VOLT;
	else
		chg->constant_volt = info->constant_charge_voltage_max_uv;

The same call also occurs in pf1550_reg_init(). However, there are no
matching calls to power_supply_put_battery_info() in the driver to free the
memory allocated by these lookups.


[Severity: High]
This is a pre-existing issue, but does this code handle missing devicetree
properties correctly?

In pf1550_dt_parse_dev_info():

	if (power_supply_get_battery_info(chg->charger, &info))
		chg->constant_volt = PF1550_DEFAULT_CONSTANT_VOLT;
	else
		chg->constant_volt = info->constant_charge_voltage_max_uv;

If the devicetree lacks the optional constant-charge-voltage-max-microvolt
property, info->constant_charge_voltage_max_uv is set to -EINVAL. This
negative error code is then cast to the u32 chg->constant_volt, which
becomes 4294967274 and guarantees a failure in later upper-bound checks.


[Severity: High]
This is a pre-existing issue, but is the logic for enabling the charger
inverted here?

Looking at pf1550_reg_init():

	if (power_supply_get_battery_info(chg->charger, &info)) {
		ret = regmap_write(chg->pf1550->regmap,
				   PF1550_CHARG_REG_CHG_OPER,
				   PF1550_CHG_BAT_ON);

Since power_supply_get_battery_info() returns 0 on success and an error code
like -ENODEV on failure, this condition evaluates to true only when the
battery info lookup fails. This appears to turn on the charger exactly when
the battery is missing or misconfigured.

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260709033428.362970-1-panchuang@vivo.com?part=28