Re: [RFC PATCH 2/4] soc: qcom: pmic_glink: Report wakeup events on behalf of clients

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
- [Medium] soc: qcom: pmic_glink: premature wakeup relax in pmic_glink_dispatch()
- [Medium] soc: qcom: pmic_glink: client wakeup source clobbered on overlapping events
--

commit 8f440750750801ffa1527cda60138e403b071b3e
Author: Vladyslav Tymoshchuk <tvlad744@gmail.com>

soc: qcom: pmic_glink: Report wakeup events on behalf of clients

This commit adds wakeup event reporting to pmic_glink on behalf of client
devices when notifications arrive, with optional per-client wakeup filtering.
When notifications are deferred during system sleep, it keeps the client
device's wakeup source active until the deferred notifications are dispatched
or dropped.

> diff --git a/drivers/soc/qcom/pmic_glink.c b/drivers/soc/qcom/pmic_glink.c
> index 748b335be6ef7..c629df8009e68 100644
> --- a/drivers/soc/qcom/pmic_glink.c
> +++ b/drivers/soc/qcom/pmic_glink.c

[ ... ]

> @@ -166,6 +216,15 @@ int pmic_glink_send(struct pmic_glink_client *client, void *data, size_t len)
>  }
>  EXPORT_SYMBOL_GPL(pmic_glink_send);
>  
> +static bool pmic_glink_is_wakeup_event(struct pmic_glink_client *client,
> +				       const void *data, size_t len)
> +{
> +	if (!device_may_wakeup(client->dev))
> +		return false;
> +
> +	return !client->wakeup_filter || client->wakeup_filter(data, len, client->priv);
> +}
> +
>  static void pmic_glink_dispatch(struct pmic_glink *pg, const void *data, size_t len)
>  {
>  	const struct pmic_glink_hdr *hdr = data;
> @@ -174,8 +233,12 @@ static void pmic_glink_dispatch(struct pmic_glink *pg, const void *data, size_t
>  
>  	spin_lock_irqsave(&pg->client_lock, flags);
>  	list_for_each_entry(client, &pg->clients, node) {
> -		if (client->id == le32_to_cpu(hdr->owner))
> -			client->cb(data, len, client->priv);
> +		if (client->id != le32_to_cpu(hdr->owner))
> +			continue;
> +
> +		/* From here on, the client keeps the system awake if it needs to */
> +		pmic_glink_client_relax(client);
> +		client->cb(data, len, client->priv);
>  	}
>  	spin_unlock_irqrestore(&pg->client_lock, flags);
>  }

[Severity: Medium]
Can pmic_glink_dispatch() in drivers/soc/qcom/pmic_glink.c prematurely
deactivate client->dev's wakeup source while a wakeup notification for that
client is still queued in pg->deferred?

Because deferred wakeup retention is tracked with a single per-client boolean
client->stay_awake in pmic_glink_rpmsg_callback(), pmic_glink_dispatch()
unconditionally clears client->stay_awake and calls pm_relax(client->dev) on
the first message dispatched to that client.

If a client uses pmic_glink_client_set_wakeup_filter() and receives a
non-wakeup notification during system sleep followed by a wakeup notification
before resume, both are queued in FIFO order in pg->deferred and
client->stay_awake is set to true. When pmic_glink_deliver_work() drains
pg->deferred:

drivers/soc/qcom/pmic_glink.c:pmic_glink_deliver_work() {
	for (;;) {
		spin_lock_irqsave(&pg->defer_lock, flags);
		msg = list_first_entry_or_null(&pg->deferred, ...);
		...
		list_del(&msg->node);
		pg->num_deferred--;
		spin_unlock_irqrestore(&pg->defer_lock, flags);

		pmic_glink_dispatch(pg, msg->data, msg->len);
		kfree(msg);
	}
}

the earlier non-wakeup notification is dispatched first. That call runs
pmic_glink_client_relax(client) -> pm_relax(client->dev) before invoking
client->cb(). Since the first message is not a wakeup event, the client
callback will not re-activate client->dev's wakeup source, leaving the wakeup
source inactive while the wakeup notification is still waiting in
pg->deferred.

The same premature relax happens if two wakeup notifications for the same
client are queued in pg->deferred, or if a new wakeup notification arrives in
pmic_glink_rpmsg_callback() after pmic_glink_deliver_work() drops
pg->defer_lock for an earlier message (while pg->defer is still true) and
before pmic_glink_dispatch() acquires pg->client_lock. Wouldn't this allow
autosleep or a suspend started via /sys/power/wakeup_count to put the system
back to sleep before the client has seen the pending wakeup notification?

> @@ -235,8 +298,11 @@ static void pmic_glink_drop_deferred(struct pmic_glink *pg)
>  static int pmic_glink_rpmsg_callback(struct rpmsg_device *rpdev, void *data,
>  				     int len, void *priv, u32 addr)
>  {
> +	struct pmic_glink_client *client;
>  	struct pmic_glink_hdr *hdr;
>  	struct pmic_glink *pg = dev_get_drvdata(&rpdev->dev);
> +	bool notification, deferred;
> +	unsigned long flags;
>  
>  	if (len < sizeof(*hdr)) {
>  		dev_warn(pg->dev, "ignoring truncated message\n");
> @@ -246,11 +312,30 @@ static int pmic_glink_rpmsg_callback(struct rpmsg_device *rpdev, void *data,
>  	hdr = data;
>  
>  	/* The sender of a request is waiting for the response */
> -	if (le32_to_cpu(hdr->type) != PMIC_GLINK_REQ_RESP &&
> -	    pmic_glink_defer(pg, data, len))
> -		return 0;
> +	notification = le32_to_cpu(hdr->type) != PMIC_GLINK_REQ_RESP;
> +
> +	/* Defer under client_lock, to set stay_awake before it is delivered */
> +	spin_lock_irqsave(&pg->client_lock, flags);
> +	deferred = notification && pmic_glink_defer(pg, data, len);
> +	list_for_each_entry(client, &pg->clients, node) {
> +		if (client->id != le32_to_cpu(hdr->owner))
> +			continue;
> +
> +		if (deferred) {
> +			if (pmic_glink_is_wakeup_event(client, data, len)) {
> +				pm_stay_awake(client->dev);
> +				pm_system_wakeup();
> +				client->stay_awake = true;
> +			}
> +			continue;
> +		}
> +
> +		if (notification && pmic_glink_is_wakeup_event(client, data, len))
> +			pm_wakeup_hard_event(client->dev);
>  
> -	pmic_glink_dispatch(pg, data, len);
> +		client->cb(data, len, client->priv);
> +	}
> +	spin_unlock_irqrestore(&pg->client_lock, flags);
>  
>  	return 0;
>  }

[Severity: Medium]
What happens if a client keeps client->dev's wakeup source active from
client->cb(), as described in the commit message, and another notification
arrives for the same client before the client relaxes its wakeup source?

In drivers/base/power/wakeup.c, struct wakeup_source is not reference-counted.
When pmic_glink_rpmsg_callback() in drivers/soc/qcom/pmic_glink.c calls
pm_wakeup_hard_event(client->dev) for a non-deferred wakeup notification, it
invokes pm_wakeup_dev_event(client->dev, 0, true) ->
pm_wakeup_ws_event(ws, 0, true):

drivers/base/power/wakeup.c:pm_wakeup_ws_event() {
	...
	wakeup_source_report_event(ws, hard);

	if (!msec) {
		wakeup_source_deactivate(ws);
		goto unlock;
	}
	...
}

Because msec is 0, pm_wakeup_ws_event() unconditionally calls
wakeup_source_deactivate(ws) before client->cb() is called, which deactivates
client->dev->power.wakeup and cancels its timer if the client was already
holding it active from an earlier callback (for example via
pm_stay_awake(client->dev) or pm_wakeup_event(client->dev, msec)).

Similarly, in pmic_glink_deliver_work() in drivers/soc/qcom/pmic_glink.c:

drivers/soc/qcom/pmic_glink.c:pmic_glink_deliver_work() {
	for (;;) {
		spin_lock_irqsave(&pg->defer_lock, flags);
		msg = list_first_entry_or_null(&pg->deferred, ...);
		if (!msg) {
			pg->defer = false;
			dropped = pg->num_dropped;
			pg->num_dropped = 0;
			spin_unlock_irqrestore(&pg->defer_lock, flags);
			break;
		}
		...
		spin_unlock_irqrestore(&pg->defer_lock, flags);

		pmic_glink_dispatch(pg, msg->data, msg->len);
		kfree(msg);
	}

	/* Release the clients whose notifications were dropped */
	spin_lock_irqsave(&pg->client_lock, flags);
	list_for_each_entry(client, &pg->clients, node)
		pmic_glink_client_relax(client);
	spin_unlock_irqrestore(&pg->client_lock, flags);
}

if client->cb() activates client->dev's wakeup source during
pmic_glink_dispatch(), and a new wakeup notification arrives while pg->defer
is still true, pmic_glink_rpmsg_callback() calls pm_stay_awake(client->dev)
and sets client->stay_awake = true; the subsequent pmic_glink_client_relax()
then calls pm_relax(client->dev) and deactivates the wakeup source held by the
client. Also, between setting pg->defer = false and acquiring pg->client_lock
for the post-loop pmic_glink_client_relax() pass, a client with
client->stay_awake set from a dropped notification can receive a non-deferred
notification in pmic_glink_rpmsg_callback(), activate its wakeup source in
client->cb(), and then immediately have it relaxed by the post-loop
pmic_glink_client_relax() call.

Could pmic_glink use its own wakeup_source (or otherwise avoid calling
pm_wakeup_hard_event() and pm_relax() on client->dev while the client owns
it) so that pmic_glink and the client driver do not clobber the same
non-refcounted wakeup source?

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