Received-SPF: pass (google.com: domain of linux-kernel+bounces-35608-linux.lists.archive=gmail.com@vger.kernel.org designates 147.75.199.223 as permitted sender) client-ip=147.75.199.223;
Date: Tue, 23 Jan 2024 16:59:03 +0100
From: Johan Hovold <johan@kernel.org>
To: Konrad Dybcio <konrad.dybcio@linaro.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>
Subject: Re: [PATCH v2] power: supply: qcom_battmgr: Ignore notifications
 before initialization
Message-ID: <Za_iR0ctkgYO0W5L@hovoldconsulting.com>
References: <20240103-topic-battmgr2-v2-1-c07b9206a2a5@linaro.org>
Precedence: bulk
MIME-Version: 1.0
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
In-Reply-To: <20240103-topic-battmgr2-v2-1-c07b9206a2a5@linaro.org>

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?).

> Fixes: b43f7ddc2b7a ("power: supply: qcom_battmgr: Register the power supplies after PDR is up")
> Reported-by: Xilin Wu <wuxilin123@gmail.com>
> Signed-off-by: Konrad Dybcio <konrad.dybcio@linaro.org>
> ---
> Changes in v2:
> - Fix the commit title
> - Link to v1: https://lore.kernel.org/linux-arm-msm/d9cf7d9d-60d9-4637-97bf-c9840452899e@linaro.org/T/#t
> ---
>  drivers/power/supply/qcom_battmgr.c | 4 ++++
>  1 file changed, 4 insertions(+)
> 
> diff --git a/drivers/power/supply/qcom_battmgr.c b/drivers/power/supply/qcom_battmgr.c
> index a12e2a66d516..7d85292eb839 100644
> --- a/drivers/power/supply/qcom_battmgr.c
> +++ b/drivers/power/supply/qcom_battmgr.c
> @@ -1271,6 +1271,10 @@ static void qcom_battmgr_callback(const void *data, size_t len, void *priv)
>  	struct qcom_battmgr *battmgr = priv;
>  	unsigned int opcode = le32_to_cpu(hdr->opcode);
>  
> +	/* Ignore the pings that come before Linux cleanly initializes the battmgr stack */

Nit: I know you have a wide-screen monitor but please follow the coding
style and break your lines at 80 columns for readability. ;)

> +	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.

> +
>  	if (opcode == BATTMGR_NOTIFICATION)
>  		qcom_battmgr_notification(battmgr, data, len);
>  	else if (battmgr->variant == QCOM_BATTMGR_SC8280XP)

Johan