Re: [RFC PATCH 4/4] power: supply: qcom_battmgr: Allow charger changes to wake up the system

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [Medium] power: supply: qcom_battmgr: broken wakeup filter on SC8280XP and X1E80100
--

commit 40893dd1782d4c214a14dd6e1c7eba74c6b54afb
Author: Vladyslav Tymoshchuk <tvlad744@gmail.com>

power: supply: qcom_battmgr: Allow charger changes to wake up the system

This commit marks the qcom_battmgr device as wakeup-capable and registers a
wakeup filter with pmic_glink so that USB and wireless charger property
notifications can wake the system when wakeup is enabled in sysfs.

> diff --git a/drivers/power/supply/qcom_battmgr.c b/drivers/power/supply/qcom_battmgr.c
> index 636d6f6acecc5..7a783a3a320d5 100644
> --- a/drivers/power/supply/qcom_battmgr.c
> +++ b/drivers/power/supply/qcom_battmgr.c
[ ... ]
> @@ -1596,6 +1597,20 @@ static void qcom_battmgr_sm8350_callback(struct qcom_battmgr *battmgr,
>  	complete(&battmgr->ack);
>  }
>  
> +static bool qcom_battmgr_is_charger_notification(const void *data, size_t len, void *priv)
> +{
> +	const struct qcom_battmgr_message *msg = data;
> +	unsigned int notification;
> +
> +	if (le32_to_cpu(msg->hdr.opcode) != BATTMGR_NOTIFICATION ||
> +	    len != sizeof(msg->hdr) + sizeof(msg->notification))
> +		return false;
> +
> +	notification = le32_to_cpu(msg->notification) & 0xff;
> +
> +	return notification == NOTIF_USB_PROPERTY || notification == NOTIF_WLS_PROPERTY;
> +}
> +
>  static void qcom_battmgr_callback(const void *data, size_t len, void *priv)
>  {
>  	const struct pmic_glink_hdr *hdr = data;
> @@ -1750,6 +1765,10 @@ static int qcom_battmgr_probe(struct auxiliary_device *adev,
>  	if (IS_ERR(battmgr->client))
>  		return PTR_ERR(battmgr->client);
>  
> +	device_set_wakeup_capable(dev, true);
> +	pmic_glink_client_set_wakeup_filter(battmgr->client,
> +					    qcom_battmgr_is_charger_notification);
> +
>  	pmic_glink_client_register(battmgr->client);
>  
>  	return 0;

[Severity: Medium]
Does wakeup work on sc8280xp and x1e80100 variants with this filter?

In drivers/power/supply/qcom_battmgr.c, qcom_battmgr_probe() unconditionally
marks the device as wakeup-capable and sets the wakeup filter to
qcom_battmgr_is_charger_notification() for all variants. However,
qcom_battmgr_is_charger_notification() only accepts:

	return notification == NOTIF_USB_PROPERTY || notification == NOTIF_WLS_PROPERTY;

which belong exclusively to the sm8350 and sm8550 property-get protocol.

As noted in the comment above qcom_battmgr_sc8280xp_set_online():

/*
 * The charger supplies' online state comes from the battery status response,
 * and the firmware sends no notification of its own when it changes, so
 * announce the change here. A supply that is not registered yet announces its
 * state when it is.
 */

On sc8280xp and x1e80100 platforms, the firmware only sends battery
notifications rather than dedicated charger property notifications when AC,
USB, or wireless charger online state changes. When userspace enables
power/wakeup on those platforms and a charger is plugged in or unplugged
while suspended, the call chain:

pmic_glink_rpmsg_callback() ->
  pmic_glink_is_wakeup_event() ->
    qcom_battmgr_is_charger_notification()

will reject every incoming notification, making wakeup a no-op despite being
advertised in sysfs.

Should device_set_wakeup_capable() and pmic_glink_client_set_wakeup_filter()
only be called on the sm8350 and sm8550 variants, or is a different wakeup
condition needed for sc8280xp and x1e80100?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20261022164151.592-1-tvlad744@gmail.com?part=4