Re: [RFC PATCH 1/4] soc: qcom: pmic_glink: Hold back notifications over system sleep

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 3 potential issue(s) to consider:
- [High] soc: qcom: pmic_glink: deliver_work() dispatches messages before service UP
- [High] soc: qcom: pmic_glink: race between pmic_glink_prepare() and deliver_work()
- [Medium] soc: qcom: pmic_glink: pmic_glink_drop_deferred() leaks num_dropped count
--

commit 3dadaa06ca7997c9a5f8bc5601e0bf726bf560b1
Author: Vladyslav Tymoshchuk <tvlad744@gmail.com>

soc: qcom: pmic_glink: Hold back notifications over system sleep

This commit defers incoming PMIC GLINK notifications and service UP events
starting from the PM prepare callback and delivers them on
system_freezable_wq from the PM complete callback so client workers do not
access suspended devices during system sleep.

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

[ ... ]

> @@ -149,13 +166,77 @@ int pmic_glink_send(struct pmic_glink_client *client, void *data, size_t len)

[ ... ]

> +static void pmic_glink_drop_deferred(struct pmic_glink *pg)
> +{
> +	struct pmic_glink_deferred_msg *msg, *tmp;
> +
> +	guard(spinlock_irqsave)(&pg->defer_lock);
> +
> +	list_for_each_entry_safe(msg, tmp, &pg->deferred, node) {
> +		list_del(&msg->node);
> +		kfree(msg);
> +	}
> +	pg->num_deferred = 0;
> +}

[Severity: Medium]
Should pmic_glink_drop_deferred() also reset pg->num_dropped to 0?

If pmic_glink_defer() drops notifications while the queue is full (or on
kmalloc_flex() failure) and the remote service subsequently goes down,
pmic_glink_state_notify_clients() calls pmic_glink_drop_deferred() to discard
all queued messages from that remote session.

Because pg->num_dropped is left untouched here, pmic_glink_deliver_work() will
later read the stale pg->num_dropped count and log a warning for notifications
that belonged to the discarded session.

[ ... ]

> @@ -217,13 +305,18 @@ static void pmic_glink_state_notify_clients(struct pmic_glink *pg)
>  	unsigned long flags;
>  
>  	if (pg->client_state != SERVREG_SERVICE_STATE_UP) {
> -		if (pg->pdr_state == SERVREG_SERVICE_STATE_UP && pg->ept)
> +		/* On UP the clients use the remote, hold that back as well */
> +		if (pg->pdr_state == SERVREG_SERVICE_STATE_UP && pg->ept &&
> +		    !pmic_glink_deferring(pg))
>  			new_state = SERVREG_SERVICE_STATE_UP;
>  	} else {
>  		if (pg->pdr_state == SERVREG_SERVICE_STATE_DOWN || !pg->ept)
>  			new_state = SERVREG_SERVICE_STATE_DOWN;
>  	}
>  
> +	if (pg->pdr_state == SERVREG_SERVICE_STATE_DOWN || !pg->ept)
> +		pmic_glink_drop_deferred(pg);
> +
>  	if (new_state != pg->client_state) {
>  		spin_lock_irqsave(&pg->client_lock, flags);
>  		list_for_each_entry(client, &pg->clients, node)
> @@ -233,6 +326,40 @@ static void pmic_glink_state_notify_clients(struct pmic_glink *pg)
>  	}
>  }
>  
> +static void pmic_glink_deliver_work(struct work_struct *work)
> +{
> +	struct pmic_glink *pg = container_of(work, struct pmic_glink, deliver_work);
> +	struct pmic_glink_deferred_msg *msg;
> +	unsigned int dropped;
> +	unsigned long flags;
> +
> +	guard(mutex)(&pg->state_lock);
> +
> +	for (;;) {
> +		spin_lock_irqsave(&pg->defer_lock, flags);
> +		msg = list_first_entry_or_null(&pg->deferred, struct pmic_glink_deferred_msg, node);
> +		if (!msg) {
> +			pg->defer = false;
> +			dropped = pg->num_dropped;
> +			pg->num_dropped = 0;
> +			spin_unlock_irqrestore(&pg->defer_lock, flags);
> +			break;
> +		}
> +
> +		list_del(&msg->node);
> +		pg->num_deferred--;
> +		spin_unlock_irqrestore(&pg->defer_lock, flags);
> +
> +		pmic_glink_dispatch(pg, msg->data, msg->len);
> +		kfree(msg);
> +	}
> +
> +	pmic_glink_state_notify_clients(pg);
> +
> +	if (dropped)
> +		dev_warn(pg->dev, "%u notifications lost during system sleep\n", dropped);
> +}

[Severity: High]
Isn't the ordering between deferred message delivery and
pmic_glink_state_notify_clients() in pmic_glink_deliver_work() inverted when
the remote service starts or restarts during system sleep?

In pmic_glink_state_notify_clients(), any SERVREG_SERVICE_STATE_DOWN event
discards all previously queued messages via pmic_glink_drop_deferred() and
notifies clients of SERVREG_SERVICE_STATE_DOWN. If the service then comes up
while pg->defer is true, SERVREG_SERVICE_STATE_UP is held back, so any
messages remaining in pg->deferred at resume arrived after the service came
up.

However, pmic_glink_deliver_work() dispatches all of pg->deferred (and clears
pg->defer = false) before calling pmic_glink_state_notify_clients(pg) to
deliver SERVREG_SERVICE_STATE_UP:

1. In drivers/power/supply/qcom_battmgr.c, qcom_battmgr_pdr_notify() only sets
   battmgr->service_up = true on SERVREG_SERVICE_STATE_UP:

   static void qcom_battmgr_pdr_notify(void *priv, int state)
   {
           struct qcom_battmgr *battmgr = priv;

           if (state == SERVREG_SERVICE_STATE_UP) {
                   battmgr->service_up = true;
                   schedule_work(&battmgr->enable_work);
           } else {
                   battmgr->service_up = false;
           }
   }

   Delivering post-UP notifications before SERVREG_SERVICE_STATE_UP causes
   qcom_battmgr_notification() -> power_supply_changed() to trigger property
   reads while battmgr->service_up is still false, failing with -EAGAIN.

2. In drivers/usb/typec/ucsi/ucsi_glink.c, pmic_glink_ucsi_pdr_notify()
   schedules ucsi->register_work on the non-freezable system_wq:

   static void pmic_glink_ucsi_pdr_notify(void *priv, int state)
   {
           struct pmic_glink_ucsi *ucsi = priv;
           unsigned long flags;

           spin_lock_irqsave(&ucsi->state_lock, flags);\
           ucsi->pd_running = (state == SERVREG_SERVICE_STATE_UP);
           spin_unlock_irqrestore(&ucsi->state_lock, flags);
           schedule_work(&ucsi->register_work);
   }

   When SERVREG_SERVICE_STATE_DOWN is passed on immediately during system
   sleep, ucsi->register_work runs ucsi_unregister() -> ucsi_unregister_port()
   -> ucsi_unregister_partner() -> typec_set_mode(con->port, TYPEC_STATE_SAFE)
   while Type-C mux and retimer devices are still suspended. Then, when the
   service comes back up during sleep and pmic_glink_deliver_work() dispatches
   post-UP notifications before SERVREG_SERVICE_STATE_UP,
   pmic_glink_ucsi_callback() schedules ucsi->notify_work before
   ucsi->register_work re-registers UCSI, accessing the freed ucsi->connector.

[ ... ]

> @@ -386,10 +519,35 @@ static void pmic_glink_remove(struct platform_device *pdev)
>  	if (pg->data->client_mask & BIT(PMIC_GLINK_CLIENT_UCSI))
>  		pmic_glink_del_aux_device(pg, &pg->ucsi_aux);
>  
> +	pmic_glink_drop_deferred(pg);
> +
>  	guard(mutex)(&__pmic_glink_lock);
>  	__pmic_glink = NULL;
>  }
>  
> +static int pmic_glink_prepare(struct device *dev)
> +{
> +	struct pmic_glink *pg = dev_get_drvdata(dev);
> +
> +	guard(spinlock_irqsave)(&pg->defer_lock);
> +	pg->defer = true;
> +
> +	return 0;
> +}

[Severity: High]
Can pmic_glink_deliver_work() race with pmic_glink_prepare() and clear
pg->defer = false while the system is entering sleep?

In kernel/power/hibernate.c, hibernation_snapshot() calls
dpm_prepare(PMSG_FREEZE) before freeze_kernel_threads() freezes
system_freezable_wq:

	error = dpm_prepare(PMSG_FREEZE);
	if (error)
		goto Complete;

	/* Preallocate image memory before freezing kernel threads and shutting down devices. */
	error = hibernate_preallocate_memory();
	if (error)
		goto Complete;

	error = freeze_kernel_threads();

A similar situation occurs if a second sleep transition or hybrid-sleep
suspend starts while pg->deliver_work queued by a previous
pmic_glink_complete() is still pending or running on system_freezable_wq.

Because pmic_glink_prepare() sets pg->defer = true without flushing or
canceling pg->deliver_work first, an in-flight pmic_glink_deliver_work() can
finish draining pg->deferred after pmic_glink_prepare() returns and set
pg->defer = false, leaving notification deferral disabled for the rest of the
sleep transition.

[ ... ]

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