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

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

New issues:
- [High] Calling power_supply_put_battery_info() on a static battery profile triggers a devm_kfree() WARN_ON.

Pre-existing issues:
- [High] Missing device tree property leads to probe failure due to -EINVAL being cast to u32 and failing bounds checks.
--

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

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

This commit selects mode 2 for the PF1550 charger when a battery profile is
available, allowing it to charge instead of remaining in the default off mode.
It also adds calls to release the battery-info objects after use.

Cc: [email protected]

> diff --git a/drivers/power/supply/pf1550-charger.c b/drivers/power/supply/pf1550-charger.c
> --- a/drivers/power/supply/pf1550-charger.c
> +++ b/drivers/power/supply/pf1550-charger.c

[ ... ]

> @@ -540,10 +543,12 @@ static void pf1550_dt_parse_dev_info(struct pf1550_charger *chg)
>  				     &chg->thermal_regulation_temp))
>  		chg->thermal_regulation_temp = PF1550_DEFAULT_THERMAL_TEMP;
>  
> -	if (power_supply_get_battery_info(chg->charger, &info))
> +	if (power_supply_get_battery_info(chg->charger, &info)) {
>  		chg->constant_volt = PF1550_DEFAULT_CONSTANT_VOLT;
> -	else
> +	} else {
>  		chg->constant_volt = info->constant_charge_voltage_max_uv;

[Severity: High]
This is a pre-existing issue, but does assigning this value directly without 
validation cause the probe to fail if the device tree omits the optional 
constant-charge-voltage-max-microvolt property?

In power_supply_get_battery_info(), constant_charge_voltage_max_uv is 
initialized to -EINVAL. Since chg->constant_volt is a u32, the -EINVAL value 
underflows to a very large unsigned number.

When pf1550_set_constant_volt() later evaluates this out-of-bounds value 
against its allowed range, it looks like it will unconditionally reject it 
and abort the driver probe.

> +		power_supply_put_battery_info(chg->charger, info);

[Severity: High]
Will calling this on a static battery profile trigger a devm_kfree() warning?

If the device tree specifies a static battery profile (like a
samsung,eb425161la battery), power_supply_get_battery_info() returns a
pointer to the statically allocated samsung_sdi_batteries array. 

Because this array resides in the .data section rather than .rodata, when 
power_supply_put_battery_info() attempts to free it:

power_supply_core.c:power_supply_put_battery_info()
    devm_kfree(&psy->dev, info);

The is_kernel_rodata() check bypasses, and the devres lookup fails, resulting 
in a warning stack trace during probe.

> +	}
>  }

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