Re: [PATCH v2] power: supply: pf1550: enable charging when battery profile exists

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

Pre-existing issues:
- [High] Unspecified `constant-charge-voltage-max-microvolt` causes driver probe to fail due to unsigned conversion of `-EINVAL`.
- [High] Delayed works are initialized before power supplies are registered, leading to a potential Use-After-Free during teardown.
--

--- Patch [1]: [PATCH v2] power: supply: pf1550: enable charging when battery profile exists ---
commit d674322929d0145285ab1259aa1b4032720d8bd7
Author: Xu Rao <[email protected]>

power: supply: pf1550: enable charging when battery profile exists

PF1550 starts in charger mode 1, where charging is disabled. The driver
comment says that mode 2 should be selected for applications using a
battery, but the condition is inverted: PF1550_CHG_BAT_ON is written only
when power_supply_get_battery_info() fails.

Cc: [email protected]

> diff --git a/drivers/power/supply/pf1550-charger.c b/drivers/power/supply/pf1550-charger.c
> index a457862ef4610..a826a938f52d7 100644
> --- a/drivers/power/supply/pf1550-charger.c
> +++ b/drivers/power/supply/pf1550-charger.c
> @@ -514,7 +514,7 @@ static int pf1550_reg_init(struct pf1550_charger *chg)
>  	 * a battery. The other supported mode is mode 2, the charger is turned
>  	 * on to charge a battery when present.
>  	 */
> -	if (power_supply_get_battery_info(chg->charger, &info)) {
> +	if (!power_supply_get_battery_info(chg->charger, &info)) {
>  		ret = regmap_write(chg->pf1550->regmap,
>  				   PF1550_CHARG_REG_CHG_OPER,
>  				   PF1550_CHG_BAT_ON);

[Severity: High]
This isn't a bug introduced by this patch, but if the device tree specifies
a simple-battery but omits the optional constant-charge-voltage-max-microvolt
property, power_supply_get_battery_info() leaves
info->constant_charge_voltage_max_uv initialized to -EINVAL.

This negative error code is assigned to chg->constant_volt (a u32) in
pf1550_dt_parse_dev_info():

drivers/power/supply/pf1550-charger.c: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;
    ...
}

Later, pf1550_set_constant_volt() checks this huge unsigned value and
fails its bounds check, returning -EINVAL:

drivers/power/supply/pf1550-charger.c:pf1550_set_constant_volt() {
    ...
    if (val < 3500000 || val > 4440000)
        return dev_err_probe(chg->dev, -EINVAL,
                             "Wrong value for constant voltage\n");
    ...
}

Could this cause the driver probe to unconditionally fail when the property
is missing?


[Severity: High]
This is a pre-existing issue, but I noticed the delayed works are
initialized before the power supplies in pf1550_charger_probe():

drivers/power/supply/pf1550-charger.c:pf1550_charger_probe() {
    ...
    ret = devm_delayed_work_autocancel(chg->dev, &chg->vbus_sense_work,
                                       pf1550_chg_vbus_work);
    ...
    chg->charger = devm_power_supply_register(&pdev->dev, ...);
    ...
}

Since devres framework cleans up resources in reverse order (LIFO), during
unbind or late probe failure, the power supplies will be unregistered and
freed first, and the delayed works cancelled afterward.

If pf1550_chg_vbus_work() executes in this window, it calls
power_supply_changed(chg->battery). Will this dereference a freed power
supply pointer and cause a use-after-free?

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