Re: [PATCH v2] power: supply: qcom_battmgr: Ignore notifications before initialization

[Konrad Dybcio]

On 1/23/24 16:59, Johan Hovold wrote:
> On Wed, Jan 03, 2024 at 01:36:08PM +0100, Konrad Dybcio wrote:
> > Commit b43f7ddc2b7a ("power: supply: qcom_battmgr: Register the power
> > supplies after PDR is up") moved the devm_power_supply_register() calls
> > so that the power supply devices are not registered before we go through
> > the entire initialization sequence (power up the ADSP remote processor,
> > wait for it to come online, coordinate with userspace..).
> >
> > Some firmware versions (e.g. on SM8550) seem to leave battmgr at least
> > partly initialized when exiting the bootloader and loading Linux. Check
> > if the power supply devices are registered before consuming the battmgr
> > notifications.
>
> So this clearly was not tested properly as the offending commit breaks
> both the Lenovo ThinkPad X13s and the SC8280XP CRD.
>
> I spent some time this afternoon tracking down and considering the best
> way to address this before I checked lore and found this proposed fix
> (why was I not CCed?).

I didn't give the offending commit a spin on the laptops, as I simply
assumed the interface is generic enough to behave similarly across the
platforms. With this, I didn't imagine the DSP firmwares aren't unloaded
on these..

[...]

> > +	if (!battmgr->bat_psy)
> > +		return;
>
> This is not a proper fix. You register 3-4 class devices and only check
> one. Even if your checked the last one, there's no locking or barriers
> in place to prevent this from breaking.
>
> Deferred registration of the class devices also risks missing
> notifications as you'll be spending time on registration after the
> service has gone live.
>
> I'm sure all of this can be handled but as it is non-trivial and the
> motivation for the offending commit is questionable to begin with, I
> suggest reverting for now.
>
> I'll send a revert for Sebastian to consider.

What you're saying is valid, but a "battery" device is always expected
to be present. If devm_power_supply_register fails, things would go very
south very fast anyway. I personally don't see this being a terribly bad
fix, but I'm open to different propositions.

Konrad