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

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

[Vladyslav Tymoshchuk]

The GLINK interrupt that delivers the PMIC GLINK messages is requested
with IRQF_NO_SUSPEND, so the clients get notifications during system
suspend and resume and in suspend-to-idle, and the work they schedule
for them can run while devices are suspended. Commit c7c56b6ec077
("soc: qcom: pmic_glink: Fix device access from worker during
suspend") moved the altmode worker to system_freezable_wq as a
stop-gap, and left the proper fix for later:

  The final shape of the rework of the pmic glink driver in order to
  fulfill both the filtering of the messages that need to be able to
  wake-up the system and the queueing of these messages until the
  system has properly resumed is still being discussed and it is
  planned as a future effort.

This series is a proposal for that shape, to have something concrete
to discuss:

- Patch 1 does the queueing: from ->prepare() on, pmic_glink holds the
  notifications back, and passes them on from system_freezable_wq once
  the system has resumed, in order, without duplicates, at most 64.
  Responses are passed on right away. The service UP is held back as
  well and passed on after the notifications, DOWN is passed on right
  away.

- Patch 2 does the filtering: a notification is a wakeup event if the
  client's device may wake up the system, see device_may_wakeup(), and
  the client's optional wakeup filter accepts it, which is the
  callback that Casey suggested [1]. pmic_glink reports the event when
  the notification arrives, and keeps the client's wakeup source
  active until the notification is delivered.

- Patch 3 enables wakeup for ucsi_glink, as Jishnu Prakash proposed
  [5], without the event in the callback.

- Patch 4 makes qcom_battmgr wakeup capable, with a filter for the
  charger notifications, but leaves wakeup disabled: Bjorn does not
  want a laptop to wake up in a backpack [9], and a battery charging
  state notification woke up Abel's laptop with some chargers [10].

The UCSI connector work running before the devices it uses have
resumed has been hit on three boards so far [3]: on X Elite with a
PS8830 retimer [2], on SM8850 with an FSA4480 switch, and on SM8750
with the dwc3-qcom role switch, where the register access with the
clocks off resets the SoC [4]. With patch 1, the clients don't have to
keep the work for their notifications off the system workqueue: Abel's
UCSI RFC [2] would not be needed, and the altmode stop-gap can stay or
go.

The earlier discussion is in the thread of the first version of the
altmode fix [1]. Krzysztof:

  Regardless whether we call pmic_glink a bus or some sort of
  transport mechanism, the users of its interface should not be
  called after they are suspended.

Bjorn asked whether a bus would mean "some mechanism for pmic_glink to
defer these messages until resume has happened". Abel found queueing
the messages fine, but noted that it does not decide when to wake up.
On the UCSI RFC, Dmitry asked to fix PMIC GLINK instead [2].

Why not in GLINK: dropping IRQF_NO_SUSPEND would break the other users
of the interrupt, as the description of c7c56b6ec077 says, and would
not close the race [6].

Why not a bus: patch 1 gives the guarantee Krzysztof asks for from the
pm callbacks of the pmic_glink device, and the wakeup policy stays
with the client devices, which reviewers preferred, according to [5],
over making the GLINK interrupt a wakeup interrupt [8]. cros_ec does
much the same: it passes the events that came during suspend on to its
clients in ->complete(), and decides whether an event wakes up the
system by its type and device_may_wakeup(). Its default wake mask
leaves out the battery events and the AC disconnect as "not actionable
wake events".

Open questions:

- ucsi_glink wakes the system up on a cable change by default, as in
  [5]. Should it only be wakeup capable, like qcom_battmgr?

- pmic_glink does not wait for the work that a client queues for a
  notification, so if a suspend starts right after a notification was
  passed on, the work can still overlap the suspend of devices. Should
  ucsi_glink cancel the UCSI work in its ->suspend() with
  ucsi_suspend(), like ucsi_acpi, or should a wakeup notification keep
  the system awake for a while, like the timeout in [5]?

- Only the latest of identical notifications is kept. That relies on
  the clients treating a notification as state, or as a hint to read
  the state, which holds for the in-tree ones. Should a client be able
  to opt out?

- qcom_battmgr notifications no longer reach power_supply_changed()
  during a suspend, so its wakeup source no longer aborts a suspend
  started through /sys/power/wakeup_count. Does anything rely on that?

- Patches 3 and 4 depend on the API added in patch 2. Should the whole
  series go through the Qualcomm SoC tree, with acks for those two?

Tested with suspend-to-idle on SM8750 (OnePlus Pad 3): the series on
top of the v7.3-rc5 based kernel that the device runs, with debug
prints added. That kernel does not have the dwc3-qcom fix [4].

- Awake, with qcom_battmgr wakeup enabled: of the 23 qcom_battmgr
  notifications for an unplug and replug, only the four USB property
  notifications were wakeup events.

- Cable plugged in (one run) or unplugged (two runs) during
  suspend-to-idle, with the default wakeup settings: the held-back UCSI
  notification woke the system up. 10 to 13 notifications were held
  back and 6 to 8 passed on after resume, none dropped. The wakeup
  source of ucsi_glink stayed active until they were passed on, about
  1.5 s. The UCSI work and the role switch ran only after that, and the
  SoC did not reset.

- With only qcom_battmgr wakeup enabled, a plug-in during
  suspend-to-idle woke the system up with the USB property
  notification. The UCSI notification before it was held back without
  a wakeup.

- With wakeup disabled for both clients, an unplug during
  suspend-to-idle did not wake the system up. The 11 notifications were
  held back until the RTC alarm 55 s later, and the SoC did not reset,
  so the queueing of patch 1 is enough to avoid the reset.

- 25 minutes of suspend-to-idle while charging: one qcom_battmgr
  notification came, was held back without a wakeup, and was passed on
  after resume, with the battery state updated right after.

Not tested: PDR during suspend, more than 64 notifications held back,
hibernation.

Kamal Wadhwa's PMIC GLINK RTC series [7] also changes pmic_glink.c, so
whichever goes in second needs a rebase. Its driver enables wakeup on
its device, so with patch 2 its notifications would become wakeup
events without further changes; today, the alarm only reports a soft
wakeup event through rtc_update_irq(), which does not end
suspend-to-idle.

I used an AI assistant (Claude) for the analysis and to draft these
patches and their descriptions; I reviewed the result and ran the
tests on my device.

[1] https://lore.kernel.org/all/20250110-soc-qcom-pmic-glink-fix-device-access-on-worker-while-suspended-v1-1-e32fd6bf322e@linaro.org/
[2] https://lore.kernel.org/all/20250205-ucsi-schedule-conn-worker-on-freezable-wq-v1-1-107d1356b77b@linaro.org/
[3] https://lore.kernel.org/all/20261020132432.998-1-26rote@gmail.com/
[4] https://lore.kernel.org/all/20261018143126.3978-1-tvlad744@gmail.com/
[5] https://lore.kernel.org/all/20260722-ucsi_glink_wakeup-v2-1-b43dee56df24@oss.qualcomm.com/
[6] https://lore.kernel.org/all/CAMVG2ssS4=Kb=OxuVXb_062fgvBfKz+X0q1N2rFob0TH6JypYg@mail.gmail.com/
[7] https://lore.kernel.org/all/20260807-rtc-glink-cleanup-v1-0-7801e4d69c4f@oss.qualcomm.com/
[8] https://lore.kernel.org/all/20240603073648.3475123-1-quic_deesin@quicinc.com/
[9] https://lore.kernel.org/all/ZnIe2hjPgWipDfgG@hu-bjorande-lv.qualcomm.com/
[10] https://lore.kernel.org/all/Z5taRZ0om5y%2FDJ69@linaro.org/

Vladyslav Tymoshchuk (4):
  soc: qcom: pmic_glink: Hold back notifications over system sleep
  soc: qcom: pmic_glink: Report wakeup events on behalf of clients
  usb: typec: ucsi: glink: Wake up the system on connector changes
  power: supply: qcom_battmgr: Allow charger changes to wake up the
    system

 drivers/power/supply/qcom_battmgr.c |  19 ++
 drivers/soc/qcom/pmic_glink.c       | 257 +++++++++++++++++++++++++++-
 drivers/usb/typec/ucsi/ucsi_glink.c |   6 +
 include/linux/soc/qcom/pmic_glink.h |   2 +
 4 files changed, 281 insertions(+), 3 deletions(-)


base-commit: 9a482184d910775cf648863148944945b74e2815
-- 
2.43.0

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

[Vladyslav Tymoshchuk]

The GLINK interrupt is not suspended, so the PMIC GLINK messages keep
reaching the clients during system suspend and resume, and while the
system is in suspend-to-idle. The clients handle most notifications in
work on the system workqueue, which is not freezable, so the work can
run before the devices it uses have resumed:

- ucsi_glink passes connector changes on to the UCSI core, whose
  connector work switches the USB role and sets the Type-C muxes and
  switches. On SM8750, unplugging the USB cable while the system is in
  suspend-to-idle makes dwc3-qcom access its registers with the clocks
  off, and the SoC resets. Other boards hit I2C transfers on suspended
  controllers from the same work, to a PS8830 retimer or an FSA4480
  switch.

- pmic_glink_altmode had the same problem with its worker, which
  commit c7c56b6ec077 ("soc: qcom: pmic_glink: Fix device access from
  worker during suspend") moved to system_freezable_wq as a stop-gap.

Rather than have each client defer its work, defer the notifications
in pmic_glink: queue them from ->prepare() on, and from ->complete()
schedule work on system_freezable_wq that passes them on to the
clients. The work runs once the tasks have been thawed, after all the
devices have resumed. Until it has run, new notifications are queued
behind the old ones, so the clients get them in order. Responses are
passed on right away, as the client that sent the request is waiting
for them.

The clients treat a notification as the latest state, or as a hint to
read the current state, so an identical notification adds nothing:
keep only the latest copy. Keep at most 64 notifications, drop the
oldest beyond that, and say how many were lost.

Hold back the service UP too, as on UP the clients start using the
remote, and pass it on after the deferred notifications. Pass DOWN on
right away, so that the clients stop using the remote, and drop the
deferred notifications with it, as they came from the remote that went
away.

ucsi_glink learns from a notification that a UCSI command has
completed, so a command still in flight at ->prepare() completes only
after the system has resumed. The timeout of the UCSI core does not
count the time spent in suspend, only the time the system takes to
suspend and resume.

qcom_battmgr calls power_supply_changed() for most notifications, which
keeps the system awake through the wakeup source of the power supply.
For a notification that is held back, this now happens after the
system has resumed, so it no longer aborts a suspend started through
/sys/power/wakeup_count.

Fixes: 58ef4ece1e41 ("soc: qcom: pmic_glink: Introduce base PMIC GLINK driver")
Signed-off-by: Vladyslav Tymoshchuk <tvlad744@gmail.com>
---
I used an AI assistant (Claude) for the analysis and to draft this
patch and its description; I reviewed the result. The testing is
described in the cover letter.

 drivers/soc/qcom/pmic_glink.c | 177 ++++++++++++++++++++++++++++++++--
 1 file changed, 168 insertions(+), 9 deletions(-)

diff --git a/drivers/soc/qcom/pmic_glink.c b/drivers/soc/qcom/pmic_glink.c
index 84fd0c82c0c4..748b335be6ef 100644
--- a/drivers/soc/qcom/pmic_glink.c
+++ b/drivers/soc/qcom/pmic_glink.c
@@ -14,8 +14,11 @@
 #include <linux/soc/qcom/pdr.h>
 #include <linux/soc/qcom/pmic_glink.h>
 #include <linux/spinlock.h>
+#include <linux/string.h>
+#include <linux/workqueue.h>
 
 #define PMIC_GLINK_SEND_TIMEOUT (5 * HZ)
+#define PMIC_GLINK_MAX_DEFERRED	64
 
 enum {
 	PMIC_GLINK_CLIENT_BATT = 0,
@@ -50,6 +53,14 @@ struct pmic_glink {
 	/* serializing clients list updates */
 	spinlock_t client_lock;
 	struct list_head clients;
+
+	/* notifications held back over system sleep */
+	spinlock_t defer_lock;
+	bool defer;
+	struct list_head deferred;
+	unsigned int num_deferred;
+	unsigned int num_dropped;
+	struct work_struct deliver_work;
 };
 
 static struct pmic_glink *__pmic_glink;
@@ -66,6 +77,12 @@ struct pmic_glink_client {
 	void *priv;
 };
 
+struct pmic_glink_deferred_msg {
+	struct list_head node;
+	size_t len;
+	u8 data[] __counted_by(len);
+};
+
 static void _devm_pmic_glink_release_client(struct device *dev, void *res)
 {
 	struct pmic_glink_client *client = (struct pmic_glink_client *)res;
@@ -149,13 +166,77 @@ int pmic_glink_send(struct pmic_glink_client *client, void *data, size_t len)
 }
 EXPORT_SYMBOL_GPL(pmic_glink_send);
 
+static void pmic_glink_dispatch(struct pmic_glink *pg, const void *data, size_t len)
+{
+	const struct pmic_glink_hdr *hdr = data;
+	struct pmic_glink_client *client;
+	unsigned long flags;
+
+	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);
+	}
+	spin_unlock_irqrestore(&pg->client_lock, flags);
+}
+
+static bool pmic_glink_defer(struct pmic_glink *pg, const void *data, size_t len)
+{
+	struct pmic_glink_deferred_msg *msg;
+
+	guard(spinlock_irqsave)(&pg->defer_lock);
+
+	if (!pg->defer)
+		return false;
+
+	/* An identical notification brings nothing new, keep the latest copy */
+	list_for_each_entry(msg, &pg->deferred, node) {
+		if (msg->len == len && !memcmp(msg->data, data, len)) {
+			list_move_tail(&msg->node, &pg->deferred);
+			return true;
+		}
+	}
+
+	if (pg->num_deferred == PMIC_GLINK_MAX_DEFERRED) {
+		msg = list_first_entry(&pg->deferred, struct pmic_glink_deferred_msg, node);
+		list_del(&msg->node);
+		kfree(msg);
+		pg->num_deferred--;
+		pg->num_dropped++;
+	}
+
+	msg = kmalloc_flex(*msg, data, len, GFP_ATOMIC);
+	if (!msg) {
+		pg->num_dropped++;
+		return true;
+	}
+
+	msg->len = len;
+	memcpy(msg->data, data, len);
+	list_add_tail(&msg->node, &pg->deferred);
+	pg->num_deferred++;
+
+	return true;
+}
+
+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;
+}
+
 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);
-	unsigned long flags;
 
 	if (len < sizeof(*hdr)) {
 		dev_warn(pg->dev, "ignoring truncated message\n");
@@ -164,12 +245,12 @@ static int pmic_glink_rpmsg_callback(struct rpmsg_device *rpdev, void *data,
 
 	hdr = data;
 
-	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);
-	}
-	spin_unlock_irqrestore(&pg->client_lock, flags);
+	/* 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;
+
+	pmic_glink_dispatch(pg, data, len);
 
 	return 0;
 }
@@ -210,6 +291,13 @@ static void pmic_glink_del_aux_device(struct pmic_glink *pg,
 	auxiliary_device_uninit(aux);
 }
 
+static bool pmic_glink_deferring(struct pmic_glink *pg)
+{
+	guard(spinlock_irqsave)(&pg->defer_lock);
+
+	return pg->defer;
+}
+
 static void pmic_glink_state_notify_clients(struct pmic_glink *pg)
 {
 	struct pmic_glink_client *client;
@@ -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);
+}
+
 static void pmic_glink_pdr_callback(int state, char *svc_path, void *priv)
 {
 	struct pmic_glink *pg = priv;
@@ -315,6 +442,10 @@ static int pmic_glink_probe(struct platform_device *pdev)
 	spin_lock_init(&pg->client_lock);
 	mutex_init(&pg->state_lock);
 
+	INIT_LIST_HEAD(&pg->deferred);
+	spin_lock_init(&pg->defer_lock);
+	INIT_WORK(&pg->deliver_work, pmic_glink_deliver_work);
+
 	pg->data = of_device_get_match_data(&pdev->dev);
 	if (!pg->data)
 		return -EINVAL;
@@ -377,6 +508,8 @@ static void pmic_glink_remove(struct platform_device *pdev)
 {
 	struct pmic_glink *pg = dev_get_drvdata(&pdev->dev);
 
+	cancel_work_sync(&pg->deliver_work);
+
 	pdr_handle_release(pg->pdr);
 
 	if (pg->data->client_mask & BIT(PMIC_GLINK_CLIENT_BATT))
@@ -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;
+}
+
+static void pmic_glink_complete(struct device *dev)
+{
+	struct pmic_glink *pg = dev_get_drvdata(dev);
+
+	/* Runs once the tasks are thawed, all devices have resumed by then */
+	queue_work(system_freezable_wq, &pg->deliver_work);
+}
+
+static const struct dev_pm_ops pmic_glink_pm_ops = {
+	.prepare = pmic_glink_prepare,
+	.complete = pmic_glink_complete,
+};
+
 static const struct pmic_glink_data pmic_glink_adsp_data = {
 	.client_mask = BIT(PMIC_GLINK_CLIENT_BATT) |
 		       BIT(PMIC_GLINK_CLIENT_ALTMODE) |
@@ -418,6 +576,7 @@ static struct platform_driver pmic_glink_driver = {
 	.driver = {
 		.name = "qcom_pmic_glink",
 		.of_match_table = pmic_glink_of_match,
+		.pm = pm_sleep_ptr(&pmic_glink_pm_ops),
 	},
 };
 
-- 
2.43.0

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

[Vladyslav Tymoshchuk]

The GLINK interrupt is requested with IRQF_NO_SUSPEND, but it is not a
wakeup interrupt: most PMIC GLINK messages, battery status updates for
one, must not wake up the system. A notification that arrives during
system sleep is now held back until the system has resumed, so the
client sees it too late to report it as a wakeup event.

Decide in pmic_glink, when the notification arrives: it is a wakeup
event if the client's device may wake up the system and the client's
wakeup filter, if the client has set one, accepts it. The client
driver picks the default with device_init_wakeup() or
device_set_wakeup_capable(), and user space can change it through
power/wakeup, like for any other device. Report a hard wakeup event,
so that it also brings the system out of suspend-to-idle.

While a wakeup notification is held back, keep the wakeup source of
the client's device active, so that autosleep, or a suspend started
through /sys/power/wakeup_count, does not put the system back to sleep
before the client has seen it. Release it right before the callback
rather than after it, as the client may keep the same wakeup source
active from there.

Responses are not wakeup events, the client is waiting for them.

Keep the device passed to devm_pmic_glink_client_alloc() in the client
for this, and document the function. No in-tree client enables wakeup
on its device yet, so nothing changes for them.

Signed-off-by: Vladyslav Tymoshchuk <tvlad744@gmail.com>
---
I used an AI assistant (Claude) for the analysis and to draft this
patch and its description; I reviewed the result. The testing is
described in the cover letter.

 drivers/soc/qcom/pmic_glink.c       | 104 ++++++++++++++++++++++++++--
 include/linux/soc/qcom/pmic_glink.h |   2 +
 2 files changed, 100 insertions(+), 6 deletions(-)

diff --git a/drivers/soc/qcom/pmic_glink.c b/drivers/soc/qcom/pmic_glink.c
index 748b335be6ef..c629df8009e6 100644
--- a/drivers/soc/qcom/pmic_glink.c
+++ b/drivers/soc/qcom/pmic_glink.c
@@ -9,12 +9,14 @@
 #include <linux/module.h>
 #include <linux/of.h>
 #include <linux/platform_device.h>
+#include <linux/pm_wakeup.h>
 #include <linux/rpmsg.h>
 #include <linux/slab.h>
 #include <linux/soc/qcom/pdr.h>
 #include <linux/soc/qcom/pmic_glink.h>
 #include <linux/spinlock.h>
 #include <linux/string.h>
+#include <linux/suspend.h>
 #include <linux/workqueue.h>
 
 #define PMIC_GLINK_SEND_TIMEOUT (5 * HZ)
@@ -70,11 +72,15 @@ struct pmic_glink_client {
 	struct list_head node;
 
 	struct pmic_glink *pg;
+	struct device *dev;
 	unsigned int id;
 
 	void (*cb)(const void *data, size_t len, void *priv);
 	void (*pdr_notify)(void *priv, int state);
+	bool (*wakeup_filter)(const void *data, size_t len, void *priv);
 	void *priv;
+
+	bool stay_awake;
 };
 
 struct pmic_glink_deferred_msg {
@@ -83,6 +89,16 @@ struct pmic_glink_deferred_msg {
 	u8 data[] __counted_by(len);
 };
 
+static void pmic_glink_client_relax(struct pmic_glink_client *client)
+{
+	lockdep_assert_held(&client->pg->client_lock);
+
+	if (client->stay_awake) {
+		client->stay_awake = false;
+		pm_relax(client->dev);
+	}
+}
+
 static void _devm_pmic_glink_release_client(struct device *dev, void *res)
 {
 	struct pmic_glink_client *client = (struct pmic_glink_client *)res;
@@ -91,9 +107,26 @@ static void _devm_pmic_glink_release_client(struct device *dev, void *res)
 
 	spin_lock_irqsave(&pg->client_lock, flags);
 	list_del(&client->node);
+	pmic_glink_client_relax(client);
 	spin_unlock_irqrestore(&pg->client_lock, flags);
 }
 
+/**
+ * devm_pmic_glink_client_alloc() - allocate a client of the PMIC GLINK service
+ * @dev:	client device, a child of the pmic_glink device
+ * @id:		owner of the messages for the client
+ * @cb:		called, in atomic context, with each message for the client
+ * @pdr:	called, in atomic context, with the state of the remote service
+ * @priv:	passed to @cb, @pdr and the wakeup filter
+ *
+ * A notification for a client whose @dev may wake up the system, see
+ * device_may_wakeup(), is a wakeup event, unless the wakeup filter of the
+ * client rejects it. Notifications that arrive during system sleep reach @cb
+ * once the system has resumed.
+ *
+ * Return: the client, to be registered with pmic_glink_client_register(), or
+ * an ERR_PTR() on failure.
+ */
 struct pmic_glink_client *devm_pmic_glink_client_alloc(struct device *dev,
 						       unsigned int id,
 						       void (*cb)(const void *, size_t, void *),
@@ -108,6 +141,7 @@ struct pmic_glink_client *devm_pmic_glink_client_alloc(struct device *dev,
 		return ERR_PTR(-ENOMEM);
 
 	client->pg = pg;
+	client->dev = dev;
 	client->id = id;
 	client->cb = cb;
 	client->pdr_notify = pdr;
@@ -120,6 +154,22 @@ struct pmic_glink_client *devm_pmic_glink_client_alloc(struct device *dev,
 }
 EXPORT_SYMBOL_GPL(devm_pmic_glink_client_alloc);
 
+/**
+ * pmic_glink_client_set_wakeup_filter() - select the wakeup notifications
+ * @client:	client from devm_pmic_glink_client_alloc(), not registered yet
+ * @filter:	called, in atomic context, with each notification for the client
+ *		while its device may wake up the system; returns true to wake
+ *		the system up
+ *
+ * Without a filter, every notification for the client is a wakeup event.
+ */
+void pmic_glink_client_set_wakeup_filter(struct pmic_glink_client *client,
+					 bool (*filter)(const void *, size_t, void *))
+{
+	client->wakeup_filter = filter;
+}
+EXPORT_SYMBOL_GPL(pmic_glink_client_set_wakeup_filter);
+
 void pmic_glink_client_register(struct pmic_glink_client *client)
 {
 	struct pmic_glink *pg = client->pg;
@@ -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);
 }
@@ -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;
 }
@@ -330,6 +415,7 @@ 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;
+	struct pmic_glink_client *client;
 	unsigned int dropped;
 	unsigned long flags;
 
@@ -354,6 +440,12 @@ static void pmic_glink_deliver_work(struct work_struct *work)
 		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);
+
 	pmic_glink_state_notify_clients(pg);
 
 	if (dropped)
diff --git a/include/linux/soc/qcom/pmic_glink.h b/include/linux/soc/qcom/pmic_glink.h
index 7cddf1027752..786d2f35a9a9 100644
--- a/include/linux/soc/qcom/pmic_glink.h
+++ b/include/linux/soc/qcom/pmic_glink.h
@@ -28,6 +28,8 @@ struct pmic_glink_client *devm_pmic_glink_client_alloc(struct device *dev,
 						       void (*cb)(const void *, size_t, void *),
 						       void (*pdr)(void *, int),
 						       void *priv);
+void pmic_glink_client_set_wakeup_filter(struct pmic_glink_client *client,
+					 bool (*filter)(const void *, size_t, void *));
 void pmic_glink_client_register(struct pmic_glink_client *client);
 
 #endif
-- 
2.43.0

[RFC PATCH 3/4] usb: typec: ucsi: glink: Wake up the system on connector changes

[Vladyslav Tymoshchuk]

The UCSI notifications from PMIC GLINK report connector changes, a USB
cable being plugged in or out for one, and the completion of UCSI
commands. Enable wakeup on the device, so that pmic_glink reports them
as wakeup events: a cable change brings the system out of
suspend-to-idle and aborts a suspend in progress.

The completion of a command arrives only while the command is in
flight, so it can wake up the system only when a suspend has started
in the middle of one.

The same was proposed in [1], with the wakeup event reported from the
callback. pmic_glink reports it now, also for the notifications that
it holds back over system sleep.

Link: https://lore.kernel.org/all/20260722-ucsi_glink_wakeup-v2-1-b43dee56df24@oss.qualcomm.com/ [1]
Signed-off-by: Vladyslav Tymoshchuk <tvlad744@gmail.com>
---
I used an AI assistant (Claude) for the analysis and to draft this
patch and its description; I reviewed the result. The testing is
described in the cover letter.

 drivers/usb/typec/ucsi/ucsi_glink.c | 6 ++++++
 1 file changed, 6 insertions(+)

diff --git a/drivers/usb/typec/ucsi/ucsi_glink.c b/drivers/usb/typec/ucsi/ucsi_glink.c
index 1db0c88d817e..b03320f5a6bb 100644
--- a/drivers/usb/typec/ucsi/ucsi_glink.c
+++ b/drivers/usb/typec/ucsi/ucsi_glink.c
@@ -7,6 +7,7 @@
 #include <linux/module.h>
 #include <linux/mutex.h>
 #include <linux/of_device.h>
+#include <linux/pm_wakeup.h>
 #include <linux/property.h>
 #include <linux/soc/qcom/pdr.h>
 #include <linux/usb/typec_mux.h>
@@ -455,6 +456,11 @@ static int pmic_glink_ucsi_probe(struct auxiliary_device *adev,
 		ucsi->port_orientation[port] = desc;
 	}
 
+	/* pmic_glink reports the notifications as wakeup events */
+	ret = devm_device_init_wakeup(dev);
+	if (ret)
+		return ret;
+
 	ucsi->client = devm_pmic_glink_client_alloc(dev, PMIC_GLINK_OWNER_USBC,
 						    pmic_glink_ucsi_callback,
 						    pmic_glink_ucsi_pdr_notify,
-- 
2.43.0

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

[Vladyslav Tymoshchuk]

The battery manager notifies about changes of the battery state, many
of which arrive while the system is asleep, and about changes of the
USB and wireless charger properties, a charger being plugged in or out
for one. Only the latter can be worth a wakeup, to show the new
charging state for example.

Make the device wakeup capable, and set a wakeup filter that lets
pmic_glink report only the charger notifications as wakeup events.
Leave wakeup disabled, so that nothing changes by default: whether a
charger change should wake up the system is up to user space, through
power/wakeup.

Signed-off-by: Vladyslav Tymoshchuk <tvlad744@gmail.com>
---
I used an AI assistant (Claude) for the analysis and to draft this
patch and its description; I reviewed the result. The testing is
described in the cover letter.

 drivers/power/supply/qcom_battmgr.c | 19 +++++++++++++++++++
 1 file changed, 19 insertions(+)

diff --git a/drivers/power/supply/qcom_battmgr.c b/drivers/power/supply/qcom_battmgr.c
index 636d6f6acecc..7a783a3a320d 100644
--- a/drivers/power/supply/qcom_battmgr.c
+++ b/drivers/power/supply/qcom_battmgr.c
@@ -10,6 +10,7 @@
 #include <linux/mutex.h>
 #include <linux/nvmem-consumer.h>
 #include <linux/of_device.h>
+#include <linux/pm_wakeup.h>
 #include <linux/power_supply.h>
 #include <linux/property.h>
 #include <linux/soc/qcom/pdr.h>
@@ -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;
-- 
2.43.0

Re: [RFC PATCH 3/4] usb: typec: ucsi: glink: Wake up the system on connector changes

[sashiko-bot]

> The UCSI notifications from PMIC GLINK report connector changes, a USB
> cable being plugged in or out for one, and the completion of UCSI
> commands. Enable wakeup on the device, so that pmic_glink reports them
> as wakeup events: a cable change brings the system out of
> suspend-to-idle and aborts a suspend in progress.
> 
> The completion of a command arrives only while the command is in
> flight, so it can wake up the system only when a suspend has started
> in the middle of one.
> 
> The same was proposed in [1], with the wakeup event reported from the
> callback. pmic_glink reports it now, also for the notifications that
> it holds back over system sleep.
> 
> Link: https://lore.kernel.org/all/20260722-ucsi_glink_wakeup-v2-1-b43dee56df24@oss.qualcomm.com/ [1]
> Signed-off-by: Vladyslav Tymoshchuk <tvlad744@gmail.com>

Sashiko has reviewed this patch and found no issues.

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

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

[sashiko-bot]

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

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

[sashiko-bot]

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

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

[sashiko-bot]

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