Received-SPF: pass (google.com: domain of linux-kernel+bounces-35824-linux.lists.archive=gmail.com@vger.kernel.org designates 147.75.199.223 as permitted sender) client-ip=147.75.199.223;
Message-ID: <b7e91951-e554-4baf-9b8d-fca4a2f0d412@linaro.org>
Date: Tue, 23 Jan 2024 18:53:46 +0100
Precedence: bulk
MIME-Version: 1.0
User-Agent: Mozilla Thunderbird
Subject: Re: [PATCH v2] power: supply: qcom_battmgr: Ignore notifications
 before initialization
Content-Language: en-US
To: Johan Hovold <johan@kernel.org>
Cc: Bjorn Andersson <andersson@kernel.org>, Sebastian Reichel
 <sre@kernel.org>, Marijn Suijten <marijn.suijten@somainline.org>,
 Sebastian Reichel <sebastian.reichel@collabora.com>,
 linux-arm-msm@vger.kernel.org, linux-pm@vger.kernel.org,
 linux-kernel@vger.kernel.org, Xilin Wu <wuxilin123@gmail.com>
References: <20240103-topic-battmgr2-v2-1-c07b9206a2a5@linaro.org>
 <Za_iR0ctkgYO0W5L@hovoldconsulting.com>
From: Konrad Dybcio <konrad.dybcio@linaro.org>
In-Reply-To: <Za_iR0ctkgYO0W5L@hovoldconsulting.com>
Content-Type: text/plain; charset=UTF-8; format=flowed
Content-Transfer-Encoding: 7bit


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