[PATCH v2 0/2] rtc: Fix broken RTC alarm support on Qualcomm Glymur and Hamoa/X1E80100 boards

[PATCH v2 0/2] rtc: Fix broken RTC alarm support on Qualcomm Glymur and Hamoa/X1E80100 boards

[Kamal Wadhwa]

On some Qualcomm targets like Glymur & Hamoa, the SPMI based rtc-pm8xxx.c
driver is unable to receive alarm IRQs. This happens because the SPMI IRQ
permission for receiving the RTC alarms is routed to remote firmware that
implements the TAD feature [1].

TAD feature is a standard feature on Microsoft Windows OS based devices.
Since Glymur and Hamoa were originally launched on Windows OS, unlike the other
Qualcomm targets launched with Android/Linux OS, rtc-pm8xxx.c can not work on
these targets to provide alarm support.

So in order to make the RTC alarm IRQ available on the Linux kernel, the PMIC
RTC Alarm IRQ must be relayed to APPS from the remote firmware via Glink
interface and on the APPS side a new driver is needed to register an RTC device, and
provide time and alarm functionality.

To support this, add a new auxiliary driver rtc-qcom-glink.c under the
existing pmic_glink base driver, which will register an RTC device and will
handle the communication with the remote firmware for RTC related operations via
the pmic_glink interface driver.

[1] See "Control method time and alarm device" in the specification:
Link: https://learn.microsoft.com/en-us/windows-hardware/drivers/bringup/acpi-defined-devices

Signed-off-by: Kamal Wadhwa <kamal.wadhwa@oss.qualcomm.com>
---
Changes in v2:
- Extended this series to include Hamoa RTC support (along with Glymur)
- Split the single driver patch into two: the pmic_glink client plumbing is now
  its own patch, separate from the RTC driver.
- Fixed a UAF on driver unbind/probe failure by registering the
  GLINK client only after the RTC device is allocated, and by
  explicitly unregistering the client before cancel_work_sync() in
  remove().
- Fixed a timeout in qcom_rtc_glink_request() that could leave the
  request/response state out of sync, letting a late firmware response get
mistaken for a subsequent request's.
- Fixed 32-bit wraparound and sign-extension bugs in the time/alarm
  math, and removed the ticks-based time fallback entirely along
  with the offset tracking it relied on.
- Renamed the GET_PROPERTY/SET_PROPERTY opcodes (and the structs and
  helpers built around them) to GET_TIMER_VALUE/SET_TIMER_VALUE to
  match the firmware's own wire protocol naming.
- Dropped the "allow-set-time" DT property: the firmware does not
  gate RTC writes on it, so it was host-side policy with no
  hardware/firmware fact to document, not something devicetree
  should express. RTC writes are now always allowed.
- Link to v1:
  https://patch.msgid.link/20260807-rtc-glink-cleanup-v1-0-7801e4d69c4f@oss.qualcomm.com

To: Bjorn Andersson <andersson@kernel.org>
To: Konrad Dybcio <konradybcio@kernel.org>
To: Abel Vesa <abelvesa@kernel.org>
To: Alexandre Belloni <alexandre.belloni@bootlin.com>
To: Kamal Wadhwa <kamal.wadhwa@oss.qualcomm.com>
Cc: linux-arm-msm@vger.kernel.org
Cc: linux-kernel@vger.kernel.org
Cc: linux-rtc@vger.kernel.org

---
Kamal Wadhwa (2):
      soc: qcom: pmic_glink: Add RTC client support
      rtc: qcom: rtc-glink: Add new driver to support alarms on Glymur & Hamoa boards

 MAINTAINERS                         |   7 +
 drivers/rtc/Kconfig                 |  10 +
 drivers/rtc/Makefile                |   1 +
 drivers/rtc/rtc-qcom-glink.c        | 590 ++++++++++++++++++++++++++++++++++++
 drivers/soc/qcom/pmic_glink.c       |  43 ++-
 include/linux/soc/qcom/pmic_glink.h |   1 +
 6 files changed, 651 insertions(+), 1 deletion(-)
---
base-commit: 7649ba73f9e7e1e04d0b46213e39c79ae09dd55c
change-id: 20260806-rtc-glink-cleanup-038163045afd

Best regards,
--  
Kamal Wadhwa <kamalw@qti.qualcomm.com>

[PATCH v2 1/2] soc: qcom: pmic_glink: Add RTC client support

[Kamal Wadhwa]

On some Qualcomm targets like Hamoa and Glymur, PMIC RTC is only reachable over
the GLINK interface, so add a new RTC auxiliary device ("rtc-glink") to let the
new RTC glink driver bind against it, mirroring the existing
altmode/UCSI/power-supply clients.

Also add pmic_glink_client_unregister() so the RTC aux client can detach from the
clients list before unregistering the RTC in order to avoid use-after-free issue
in a scenario when an alarm IRQ is received during teardown.

Enable the RTC client on Glymur and Hamoa/X1E80100 variants, as only those
currently support this RTC client from the firmware side.

Assisted-by: Claude:claude-sonnet-5
Signed-off-by: Raj Aryan <raryan@qti.qualcomm.com>
Signed-off-by: Kamal Wadhwa <kamal.wadhwa@oss.qualcomm.com>
---
 drivers/soc/qcom/pmic_glink.c       | 43 ++++++++++++++++++++++++++++++++++++-
 include/linux/soc/qcom/pmic_glink.h |  1 +
 2 files changed, 43 insertions(+), 1 deletion(-)

diff --git a/drivers/soc/qcom/pmic_glink.c b/drivers/soc/qcom/pmic_glink.c
index 84fd0c82c0c4..8af6f4a65e61 100644
--- a/drivers/soc/qcom/pmic_glink.c
+++ b/drivers/soc/qcom/pmic_glink.c
@@ -21,6 +21,7 @@ enum {
 	PMIC_GLINK_CLIENT_BATT = 0,
 	PMIC_GLINK_CLIENT_ALTMODE,
 	PMIC_GLINK_CLIENT_UCSI,
+	PMIC_GLINK_CLIENT_RTC,
 };
 
 struct pmic_glink_data {
@@ -40,6 +41,7 @@ struct pmic_glink {
 	struct auxiliary_device altmode_aux;
 	struct auxiliary_device ps_aux;
 	struct auxiliary_device ucsi_aux;
+	struct auxiliary_device rtc_aux;
 
 	/* serializing client_state and pdr_state updates */
 	struct mutex state_lock;
@@ -118,6 +120,17 @@ void pmic_glink_client_register(struct pmic_glink_client *client)
 }
 EXPORT_SYMBOL_GPL(pmic_glink_client_register);
 
+void pmic_glink_client_unregister(struct pmic_glink_client *client)
+{
+	struct pmic_glink *pg = client->pg;
+	unsigned long flags;
+
+	spin_lock_irqsave(&pg->client_lock, flags);
+	list_del_init(&client->node);
+	spin_unlock_irqrestore(&pg->client_lock, flags);
+}
+EXPORT_SYMBOL_GPL(pmic_glink_client_unregister);
+
 int pmic_glink_send(struct pmic_glink_client *client, void *data, size_t len)
 {
 	struct pmic_glink *pg = client->pg;
@@ -342,6 +355,12 @@ static int pmic_glink_probe(struct platform_device *pdev)
 			goto out_release_altmode_aux;
 	}
 
+	if (pg->data->client_mask & BIT(PMIC_GLINK_CLIENT_RTC)) {
+		ret = pmic_glink_add_aux_device(pg, &pg->rtc_aux, "rtc-glink");
+		if (ret)
+			goto out_release_ps_aux;
+	}
+
 	if (pg->data->charger_pdr_service_name && pg->data->charger_pdr_service_path) {
 		service = pdr_add_lookup(pg->pdr, pg->data->charger_pdr_service_name,
 					 pg->data->charger_pdr_service_path);
@@ -359,6 +378,9 @@ static int pmic_glink_probe(struct platform_device *pdev)
 	return 0;
 
 out_release_aux_devices:
+	if (pg->data->client_mask & BIT(PMIC_GLINK_CLIENT_RTC))
+		pmic_glink_del_aux_device(pg, &pg->rtc_aux);
+out_release_ps_aux:
 	if (pg->data->client_mask & BIT(PMIC_GLINK_CLIENT_BATT))
 		pmic_glink_del_aux_device(pg, &pg->ps_aux);
 out_release_altmode_aux:
@@ -385,11 +407,20 @@ static void pmic_glink_remove(struct platform_device *pdev)
 		pmic_glink_del_aux_device(pg, &pg->altmode_aux);
 	if (pg->data->client_mask & BIT(PMIC_GLINK_CLIENT_UCSI))
 		pmic_glink_del_aux_device(pg, &pg->ucsi_aux);
+	if (pg->data->client_mask & BIT(PMIC_GLINK_CLIENT_RTC))
+		pmic_glink_del_aux_device(pg, &pg->rtc_aux);
 
 	guard(mutex)(&__pmic_glink_lock);
 	__pmic_glink = NULL;
 }
 
+static const struct pmic_glink_data pmic_glink_glymur_data = {
+	.client_mask = BIT(PMIC_GLINK_CLIENT_BATT) |
+		       BIT(PMIC_GLINK_CLIENT_ALTMODE) |
+		       BIT(PMIC_GLINK_CLIENT_UCSI) |
+		       BIT(PMIC_GLINK_CLIENT_RTC),
+};
+
 static const struct pmic_glink_data pmic_glink_adsp_data = {
 	.client_mask = BIT(PMIC_GLINK_CLIENT_BATT) |
 		       BIT(PMIC_GLINK_CLIENT_ALTMODE) |
@@ -404,10 +435,20 @@ static const struct pmic_glink_data pmic_glink_soccp_data = {
 		       BIT(PMIC_GLINK_CLIENT_UCSI),
 };
 
+static const struct pmic_glink_data pmic_glink_x1e80100_data = {
+	.client_mask = BIT(PMIC_GLINK_CLIENT_BATT) |
+		       BIT(PMIC_GLINK_CLIENT_ALTMODE) |
+		       BIT(PMIC_GLINK_CLIENT_UCSI) |
+		       BIT(PMIC_GLINK_CLIENT_RTC),
+	.charger_pdr_service_name = "tms/servreg",
+	.charger_pdr_service_path = "msm/adsp/charger_pd",
+};
+
 static const struct of_device_id pmic_glink_of_match[] = {
-	{ .compatible = "qcom,glymur-pmic-glink", .data = &pmic_glink_soccp_data },
+	{ .compatible = "qcom,glymur-pmic-glink", .data = &pmic_glink_glymur_data },
 	{ .compatible = "qcom,kaanapali-pmic-glink", .data = &pmic_glink_soccp_data },
 	{ .compatible = "qcom,pmic-glink", .data = &pmic_glink_adsp_data },
+	{ .compatible = "qcom,x1e80100-pmic-glink", .data = &pmic_glink_x1e80100_data },
 	{}
 };
 MODULE_DEVICE_TABLE(of, pmic_glink_of_match);
diff --git a/include/linux/soc/qcom/pmic_glink.h b/include/linux/soc/qcom/pmic_glink.h
index 7cddf1027752..c78dc7a8c99e 100644
--- a/include/linux/soc/qcom/pmic_glink.h
+++ b/include/linux/soc/qcom/pmic_glink.h
@@ -29,5 +29,6 @@ struct pmic_glink_client *devm_pmic_glink_client_alloc(struct device *dev,
 						       void (*pdr)(void *, int),
 						       void *priv);
 void pmic_glink_client_register(struct pmic_glink_client *client);
+void pmic_glink_client_unregister(struct pmic_glink_client *client);
 
 #endif

-- 
2.43.0

[PATCH v2 2/2] rtc: qcom: rtc-glink: Add new driver to support alarms on Glymur & Hamoa boards

[Kamal Wadhwa]

On some Qualcomm targets like Glymur & Hamoa, the SPMI based rtc-pm8xxx.c
driver is unable to receive alarm IRQs. This happens because the SPMI IRQ
permission for receiving the RTC alarms is routed to remote firmware that
implements the TAD feature [1].

TAD feature is a standard feature on Microsoft Windows OS based devices.
Since Glymur and Hamoa were originally launched on Windows OS, unlike the other
Qualcomm targets which were launched with Android/Linux OS, rtc-pm8xxx.c can
not provide alarm support on these targets due to the design change in the
RTC's SPMI IRQ routing.

In order to make the RTC alarm IRQ available on the Linux kernel, the PMIC RTC
Alarm IRQ is also relayed to APPS by the remote firmware via Glink interface.

To receive the relayed alarms on APPS and also to support normal time
read/write operations for RTC, add a new auxiliary driver `rtc-qcom-glink.c`
under the existing `pmic_glink` base driver, which will register an RTC device
and under the hood will handle the communication with the remote firmware for
RTC related operations via the pmic_glink interface driver.

Note - On the Qualcomm targets, pmic_glink driver is currently used for
communicating with the remote firmware by 3 clients i.e. qcom_battmgr,
ucsi_glink, altmode_glink aux client drivers. RTC glink will be the 4th aux
client to this pmic_glink driver.

[1] See "Control method time and alarm device" in the specification:
Link: https://learn.microsoft.com/en-us/windows-hardware/drivers/bringup/acpi-defined-devices

Assisted-by: Claude:claude-sonnet-5
Signed-off-by: Raj Aryan <raryan@qti.qualcomm.com>
Signed-off-by: Kamal Wadhwa <kamal.wadhwa@oss.qualcomm.com>
---
 MAINTAINERS                  |   7 +
 drivers/rtc/Kconfig          |  10 +
 drivers/rtc/Makefile         |   1 +
 drivers/rtc/rtc-qcom-glink.c | 590 +++++++++++++++++++++++++++++++++++++++++++
 4 files changed, 608 insertions(+)

diff --git a/MAINTAINERS b/MAINTAINERS
index aaf2825ac945..b8f500581da2 100644
--- a/MAINTAINERS
+++ b/MAINTAINERS
@@ -23152,6 +23152,13 @@ S:	Maintained
 F:	Documentation/devicetree/bindings/power/supply/qcom,pmi8998-charger.yaml
 F:	drivers/power/supply/qcom_smbx.c
 
+QUALCOMM PMIC GLINK RTC DRIVER
+M:	Kamal Wadhwa <kamal.wadhwa@oss.qualcomm.com>
+L:	linux-rtc@vger.kernel.org
+L:	linux-arm-msm@vger.kernel.org
+S:	Maintained
+F:	drivers/rtc/rtc-qcom-glink.c
+
 QUALCOMM PPE DRIVER
 M:	Luo Jie <jie.luo@oss.qualcomm.com>
 L:	netdev@vger.kernel.org
diff --git a/drivers/rtc/Kconfig b/drivers/rtc/Kconfig
index 73ec9230edfd..41ace94550ed 100644
--- a/drivers/rtc/Kconfig
+++ b/drivers/rtc/Kconfig
@@ -1863,6 +1863,16 @@ config RTC_DRV_PM8XXX
 	  To compile this driver as a module, choose M here: the
 	  module will be called rtc-pm8xxx.
 
+config RTC_DRV_QCOM_GLINK
+	tristate "Qualcomm PMIC GLINK RTC"
+	depends on QCOM_PMIC_GLINK
+	help
+	  Say Y here to enable support for the RTC exposed by the PMIC
+	  over the GLINK transport on Qualcomm platforms.
+
+	  This driver can also be built as a module. If so, the module
+	  will be called rtc-qcom-glink.
+
 config RTC_DRV_TEGRA
 	tristate "NVIDIA Tegra Internal RTC driver"
 	depends on ARCH_TEGRA || COMPILE_TEST
diff --git a/drivers/rtc/Makefile b/drivers/rtc/Makefile
index 46f1a0fc2416..a01803a69ad7 100644
--- a/drivers/rtc/Makefile
+++ b/drivers/rtc/Makefile
@@ -73,6 +73,7 @@ obj-$(CONFIG_RTC_DRV_FM3130)	+= rtc-fm3130.o
 obj-$(CONFIG_RTC_DRV_FSL_FTM_ALARM)	+= rtc-fsl-ftm-alarm.o
 obj-$(CONFIG_RTC_DRV_FTRTC010)	+= rtc-ftrtc010.o
 obj-$(CONFIG_RTC_DRV_GENERIC)	+= rtc-generic.o
+obj-$(CONFIG_RTC_DRV_QCOM_GLINK) += rtc-qcom-glink.o
 obj-$(CONFIG_RTC_DRV_GOLDFISH)	+= rtc-goldfish.o
 obj-$(CONFIG_RTC_DRV_HID_SENSOR_TIME) += rtc-hid-sensor-time.o
 obj-$(CONFIG_RTC_DRV_HYM8563)	+= rtc-hym8563.o
diff --git a/drivers/rtc/rtc-qcom-glink.c b/drivers/rtc/rtc-qcom-glink.c
new file mode 100644
index 000000000000..b1394b08e28d
--- /dev/null
+++ b/drivers/rtc/rtc-qcom-glink.c
@@ -0,0 +1,590 @@
+// SPDX-License-Identifier: GPL-2.0-only
+/*
+ * Copyright (c) Qualcomm Technologies, Inc. and/or its subsidiaries.
+ */
+#include <linux/auxiliary_bus.h>
+#include <linux/bitfield.h>
+#include <linux/completion.h>
+#include <linux/device.h>
+#include <linux/module.h>
+#include <linux/rtc.h>
+#include <linux/slab.h>
+#include <linux/soc/qcom/pdr.h>
+#include <linux/soc/qcom/pmic_glink.h>
+#include <linux/timekeeping.h>
+#include <linux/workqueue.h>
+
+#define PMIC_GLINK_OWNER_RTC		32784
+
+#define RTC_GLINK_SET_TIMER_VALUE	0x63
+#define RTC_GLINK_GET_TIMER_VALUE	0x65
+#define RTC_GLINK_GET_REAL_TIME		0x68
+#define RTC_GLINK_SET_REAL_TIME		0x69
+#define RTC_GLINK_ALARM_EXPIRED		0x6A
+
+#define QCOM_RTC_GLINK_TIME		0
+#define QCOM_RTC_GLINK_ALARM_TIME	1
+#define QCOM_RTC_GLINK_ALARM_ENABLE	2
+
+#define RTC_GLINK_W0_YEAR		GENMASK(15, 0)
+#define RTC_GLINK_W0_MONTH		GENMASK(23, 16)
+#define RTC_GLINK_W0_DAY		GENMASK(31, 24)
+#define RTC_GLINK_W1_HOUR		GENMASK(7, 0)
+#define RTC_GLINK_W1_MIN		GENMASK(15, 8)
+#define RTC_GLINK_W1_SEC		GENMASK(23, 16)
+#define RTC_GLINK_W1_VALID		GENMASK(31, 24)
+
+struct qcom_rtc_glink_get_timer_req {
+	struct pmic_glink_hdr	hdr;
+	__le32			timer_id;
+};
+
+struct qcom_rtc_glink_set_timer_req {
+	struct pmic_glink_hdr	hdr;
+	__le32			timer_id;
+	__le32			timer_value;
+};
+
+struct qcom_rtc_glink_generic_req {
+	struct pmic_glink_hdr	hdr;
+};
+
+struct qcom_rtc_glink_status_resp {
+	struct pmic_glink_hdr	hdr;
+	__le32			return_status;
+};
+
+/*
+ * Shared by SET_TIMER_VALUE (value = pass/fail status) and
+ * GET_TIMER_VALUE (value = the timer's current value) responses --
+ * firmware never echoes the request's timer_id back.
+ */
+struct qcom_rtc_glink_timer_resp {
+	struct pmic_glink_hdr	hdr;
+	__le32			value;
+};
+
+struct qcom_rtc_glink_real_time_resp {
+	struct pmic_glink_hdr	hdr;
+	__le32			return_status;
+	__le32			real_time_data[4];
+};
+
+struct qcom_rtc_glink_set_real_time_req {
+	struct pmic_glink_hdr	hdr;
+	__le32			real_time_data[4];
+};
+
+struct qcom_rtc_glink {
+	struct device		*dev;
+	struct pmic_glink_client *client;
+	struct rtc_device	*rtc;
+
+	struct completion	ack;
+	struct work_struct	alarm_work;
+
+	spinlock_t		state_lock;
+	bool			service_up;
+	bool			request_pending;
+	bool			timed_out;
+
+	int			error;
+	u32			resp_value;
+	struct rtc_time		resp_tm;
+	bool			resp_valid;
+};
+
+static int qcom_rtc_glink_request(struct qcom_rtc_glink *rtc_glink,
+				  void *data, size_t len)
+{
+	unsigned long flags;
+	bool timed_out = false;
+	int ret;
+
+	/*
+	 * Reinit the completion before request_pending becomes visible to
+	 * qcom_rtc_glink_pdr_notify(), so a concurrent SSR down-transition
+	 * can't complete() a stale completion that gets wiped out by
+	 * reinit_completion() right after.
+	 */
+	reinit_completion(&rtc_glink->ack);
+	rtc_glink->error = 0;
+	rtc_glink->resp_valid = false;
+
+	spin_lock_irqsave(&rtc_glink->state_lock, flags);
+	if (!rtc_glink->service_up || rtc_glink->timed_out) {
+		spin_unlock_irqrestore(&rtc_glink->state_lock, flags);
+		return !rtc_glink->service_up ? -ECONNRESET : -EAGAIN;
+	}
+	rtc_glink->request_pending = true;
+	spin_unlock_irqrestore(&rtc_glink->state_lock, flags);
+
+	ret = pmic_glink_send(rtc_glink->client, data, len);
+	if (!ret && !wait_for_completion_timeout(&rtc_glink->ack, HZ)) {
+		ret = -ETIMEDOUT;
+		timed_out = true;
+	}
+
+	spin_lock_irqsave(&rtc_glink->state_lock, flags);
+	rtc_glink->request_pending = false;
+	rtc_glink->timed_out |= timed_out;
+	spin_unlock_irqrestore(&rtc_glink->state_lock, flags);
+
+	return ret < 0 ? ret : rtc_glink->error;
+}
+
+static int qcom_rtc_glink_set_timer_value(struct qcom_rtc_glink *rtc_glink,
+					  u32 timer_id, u32 timer_value)
+{
+	struct qcom_rtc_glink_set_timer_req msg = {
+		.hdr.owner = cpu_to_le32(PMIC_GLINK_OWNER_RTC),
+		.hdr.type = cpu_to_le32(PMIC_GLINK_REQ_RESP),
+		.hdr.opcode = cpu_to_le32(RTC_GLINK_SET_TIMER_VALUE),
+		.timer_id = cpu_to_le32(timer_id),
+		.timer_value = cpu_to_le32(timer_value),
+	};
+
+	dev_dbg(rtc_glink->dev,
+		"TX opcode=0x%x timer_id=%u timer_value=%u\n",
+		RTC_GLINK_SET_TIMER_VALUE, timer_id, timer_value);
+	return qcom_rtc_glink_request(rtc_glink, &msg, sizeof(msg));
+}
+
+static int qcom_rtc_glink_get_timer_value(struct qcom_rtc_glink *rtc_glink,
+					  u32 timer_id, u32 *timer_value)
+{
+	struct qcom_rtc_glink_get_timer_req msg = {
+		.hdr.owner = cpu_to_le32(PMIC_GLINK_OWNER_RTC),
+		.hdr.type = cpu_to_le32(PMIC_GLINK_REQ_RESP),
+		.hdr.opcode = cpu_to_le32(RTC_GLINK_GET_TIMER_VALUE),
+		.timer_id = cpu_to_le32(timer_id),
+	};
+	int ret;
+
+	dev_dbg(rtc_glink->dev,
+		"TX opcode=0x%x timer_id=%u\n",
+		RTC_GLINK_GET_TIMER_VALUE, timer_id);
+	ret = qcom_rtc_glink_request(rtc_glink, &msg, sizeof(msg));
+	if (ret)
+		return ret;
+	if (!rtc_glink->resp_valid)
+		return -EIO;
+
+	*timer_value = rtc_glink->resp_value;
+	return 0;
+}
+
+static int qcom_rtc_glink_get_real_time(struct qcom_rtc_glink *rtc_glink, struct rtc_time *tm)
+{
+	struct qcom_rtc_glink_generic_req msg = {
+		.hdr.owner = cpu_to_le32(PMIC_GLINK_OWNER_RTC),
+		.hdr.type = cpu_to_le32(PMIC_GLINK_REQ_RESP),
+		.hdr.opcode = cpu_to_le32(RTC_GLINK_GET_REAL_TIME),
+	};
+	int ret;
+
+	dev_dbg(rtc_glink->dev, "TX opcode=0x%x\n", RTC_GLINK_GET_REAL_TIME);
+	ret = qcom_rtc_glink_request(rtc_glink, &msg, sizeof(msg));
+	if (ret)
+		return ret;
+	if (!rtc_glink->resp_valid)
+		return -EIO;
+
+	*tm = rtc_glink->resp_tm;
+	return 0;
+}
+
+static int qcom_rtc_glink_set_real_time(struct qcom_rtc_glink *rtc_glink,
+					struct rtc_time *tm)
+{
+	struct qcom_rtc_glink_set_real_time_req msg = {};
+	u32 w0, w1;
+
+	w0 = FIELD_PREP(RTC_GLINK_W0_YEAR, tm->tm_year + 1900) |
+	     FIELD_PREP(RTC_GLINK_W0_MONTH, tm->tm_mon + 1) |
+	     FIELD_PREP(RTC_GLINK_W0_DAY, tm->tm_mday);
+	w1 = FIELD_PREP(RTC_GLINK_W1_HOUR, tm->tm_hour) |
+	     FIELD_PREP(RTC_GLINK_W1_MIN, tm->tm_min) |
+	     FIELD_PREP(RTC_GLINK_W1_SEC, tm->tm_sec) |
+	     FIELD_PREP(RTC_GLINK_W1_VALID, 1);
+
+	msg.hdr.owner = cpu_to_le32(PMIC_GLINK_OWNER_RTC);
+	msg.hdr.type = cpu_to_le32(PMIC_GLINK_REQ_RESP);
+	msg.hdr.opcode = cpu_to_le32(RTC_GLINK_SET_REAL_TIME);
+	msg.real_time_data[0] = cpu_to_le32(w0);
+	msg.real_time_data[1] = cpu_to_le32(w1);
+	msg.real_time_data[2] = 0;
+	msg.real_time_data[3] = 0;
+
+	dev_dbg(rtc_glink->dev, "TX opcode=0x%x data=%08x %08x\n", RTC_GLINK_SET_REAL_TIME, w0, w1);
+	return qcom_rtc_glink_request(rtc_glink, &msg, sizeof(msg));
+}
+
+static int qcom_rtc_glink_get_time(struct qcom_rtc_glink *rtc_glink,
+				   struct rtc_time *time)
+{
+	return qcom_rtc_glink_get_real_time(rtc_glink, time);
+}
+
+static int qcom_rtc_glink_set_time(struct qcom_rtc_glink *rtc_glink,
+				   struct rtc_time *time)
+{
+	return qcom_rtc_glink_set_real_time(rtc_glink, time);
+}
+
+static int qcom_rtc_glink_set_alarm_en(struct qcom_rtc_glink *rtc_glink,
+				       int enabled)
+{
+	return qcom_rtc_glink_set_timer_value(rtc_glink,
+					      QCOM_RTC_GLINK_ALARM_ENABLE,
+					      enabled);
+}
+
+static int qcom_rtc_glink_set_alarm(struct qcom_rtc_glink *rtc_glink,
+				    struct rtc_wkalrm *alarm)
+{
+	time64_t alarm_t = rtc_tm_to_time64(&alarm->time);
+	struct rtc_time now_tm;
+	time64_t now_real, secs_until_alarm;
+	u32 fw_current, fw_alarm;
+	int ret;
+
+	ret = qcom_rtc_glink_get_time(rtc_glink, &now_tm);
+	if (ret)
+		return ret;
+
+	now_real = rtc_tm_to_time64(&now_tm);
+	secs_until_alarm = alarm_t - now_real;
+	dev_dbg(rtc_glink->dev, "set_alarm: now_real=%lld alarm_t=%lld delta=%lld\n",
+		now_real, alarm_t, secs_until_alarm);
+
+	if (secs_until_alarm < 0)
+		return -EINVAL;
+	if (secs_until_alarm > U32_MAX)
+		return -ERANGE;
+
+	ret = qcom_rtc_glink_get_timer_value(rtc_glink, QCOM_RTC_GLINK_TIME, &fw_current);
+	if (ret)
+		return ret;
+	if (fw_current == U32_MAX)
+		fw_current = 0;
+	fw_alarm = fw_current + (u32)secs_until_alarm;
+	dev_dbg(rtc_glink->dev, "set_alarm: fw_current=%u fw_alarm=%u\n", fw_current, fw_alarm);
+
+	ret = qcom_rtc_glink_set_alarm_en(rtc_glink, 0);
+	if (ret)
+		return ret;
+
+	ret = qcom_rtc_glink_set_timer_value(rtc_glink, QCOM_RTC_GLINK_ALARM_TIME, fw_alarm);
+	if (ret)
+		return ret;
+
+	return qcom_rtc_glink_set_alarm_en(rtc_glink, alarm->enabled);
+}
+
+static int qcom_rtc_glink_get_alarm(struct qcom_rtc_glink *rtc_glink,
+				    struct rtc_wkalrm *alarm)
+{
+	u32 fw_alarm_time, fw_current, alarm_en;
+	struct rtc_time now_tm;
+	time64_t now_real, secs_until_alarm, alarm_real;
+	int ret;
+
+	ret = qcom_rtc_glink_get_timer_value(rtc_glink, QCOM_RTC_GLINK_ALARM_TIME, &fw_alarm_time);
+	if (ret)
+		return ret;
+
+	ret = qcom_rtc_glink_get_timer_value(rtc_glink, QCOM_RTC_GLINK_TIME, &fw_current);
+	if (ret)
+		return ret;
+
+	ret = qcom_rtc_glink_get_timer_value(rtc_glink, QCOM_RTC_GLINK_ALARM_ENABLE, &alarm_en);
+	if (ret)
+		return ret;
+	if (fw_current == U32_MAX)
+		fw_current = 0;
+
+	ret = qcom_rtc_glink_get_time(rtc_glink, &now_tm);
+	if (ret)
+		return ret;
+
+	now_real = rtc_tm_to_time64(&now_tm);
+	secs_until_alarm = (u32)(fw_alarm_time - fw_current);
+	alarm_real = now_real + secs_until_alarm;
+	dev_dbg(rtc_glink->dev, "get_alarm: now_real=%lld fw_current=%u fw_alarm=%u delta=%lld\n",
+		now_real, fw_current, fw_alarm_time, secs_until_alarm);
+
+	rtc_time64_to_tm(alarm_real, &alarm->time);
+	alarm->enabled = !!alarm_en;
+	return 0;
+}
+
+static void qcom_rtc_glink_alarm_work(struct work_struct *work)
+{
+	struct qcom_rtc_glink *rtc_glink =
+		container_of(work, struct qcom_rtc_glink, alarm_work);
+
+	/*
+	 * The glink client is registered, and can receive an unsolicited
+	 * ALARM_EXPIRED, before the RTC device exists - e.g. if the board
+	 * booted because of a previously armed alarm.
+	 */
+	if (!rtc_glink->rtc)
+		return;
+
+	rtc_lock(rtc_glink->rtc);
+	rtc_update_irq(rtc_glink->rtc, 1, RTC_IRQF | RTC_AF);
+	rtc_unlock(rtc_glink->rtc);
+}
+
+static void qcom_rtc_glink_callback(const void *data, size_t len, void *priv)
+{
+	struct qcom_rtc_glink *rtc_glink = priv;
+	const struct pmic_glink_hdr *hdr = data;
+	unsigned long flags;
+	bool pending;
+
+	if (len < sizeof(*hdr))
+		return;
+
+	dev_dbg(rtc_glink->dev,
+		"RX opcode=0x%x type=0x%x owner=0x%x len=%zu\n",
+		le32_to_cpu(hdr->opcode), le32_to_cpu(hdr->type),
+		le32_to_cpu(hdr->owner), len);
+
+	if (le32_to_cpu(hdr->opcode) == RTC_GLINK_ALARM_EXPIRED) {
+		schedule_work(&rtc_glink->alarm_work);
+		return;
+	}
+
+	/*
+	 * Hold state_lock across the pending check and the response fields
+	 * below: qcom_rtc_glink_pdr_notify() writes the same error/resp_*
+	 * fields under this lock when it force-completes a request on an
+	 * SSR down-transition, and the two must not race. Dropping the
+	 * response here if !pending also covers requests that already
+	 * timed out or were aborted; qcom_rtc_glink_request() will call
+	 * reinit_completion() again before this response could be mistaken
+	 * for a later request's.
+	 */
+	spin_lock_irqsave(&rtc_glink->state_lock, flags);
+	pending = rtc_glink->request_pending;
+
+	if (!pending) {
+		if (rtc_glink->timed_out)
+			rtc_glink->timed_out = false;
+		spin_unlock_irqrestore(&rtc_glink->state_lock, flags);
+		dev_dbg(rtc_glink->dev,
+			"dropping stale response opcode=0x%x\n",
+			le32_to_cpu(hdr->opcode));
+		return;
+	}
+
+	switch (le32_to_cpu(hdr->opcode)) {
+	case RTC_GLINK_GET_TIMER_VALUE:
+		fallthrough;
+	case RTC_GLINK_SET_TIMER_VALUE: {
+		const struct qcom_rtc_glink_timer_resp *resp = data;
+
+		if (len < sizeof(*resp)) {
+			rtc_glink->error = -EINVAL;
+			break;
+		}
+		rtc_glink->resp_value = le32_to_cpu(resp->value);
+		rtc_glink->resp_valid = true;
+		rtc_glink->error = 0;
+		dev_dbg(rtc_glink->dev, "ACK opcode=0x%x resp_value=%u\n",
+			le32_to_cpu(hdr->opcode), rtc_glink->resp_value);
+		break;
+	}
+
+	case RTC_GLINK_GET_REAL_TIME: {
+		const struct qcom_rtc_glink_real_time_resp *resp = data;
+		u32 w0, w1;
+
+		if (len < sizeof(*resp)) {
+			rtc_glink->error = -EINVAL;
+			break;
+		}
+		if (le32_to_cpu(resp->return_status)) {
+			rtc_glink->error = -EIO;
+			break;
+		}
+		w0 = le32_to_cpu(resp->real_time_data[0]);
+		w1 = le32_to_cpu(resp->real_time_data[1]);
+		rtc_glink->resp_tm.tm_year = FIELD_GET(RTC_GLINK_W0_YEAR, w0) - 1900;
+		rtc_glink->resp_tm.tm_mon = FIELD_GET(RTC_GLINK_W0_MONTH, w0) - 1;
+		rtc_glink->resp_tm.tm_mday = FIELD_GET(RTC_GLINK_W0_DAY, w0);
+		rtc_glink->resp_tm.tm_hour = FIELD_GET(RTC_GLINK_W1_HOUR, w1);
+		rtc_glink->resp_tm.tm_min = FIELD_GET(RTC_GLINK_W1_MIN, w1);
+		rtc_glink->resp_tm.tm_sec = FIELD_GET(RTC_GLINK_W1_SEC, w1);
+		rtc_glink->resp_valid = true;
+		rtc_glink->error = 0;
+		dev_dbg(rtc_glink->dev,
+			"ACK opcode=0x%x %04d-%02d-%02d %02d:%02d:%02d\n",
+			RTC_GLINK_GET_REAL_TIME,
+			rtc_glink->resp_tm.tm_year + 1900,
+			rtc_glink->resp_tm.tm_mon + 1,
+			rtc_glink->resp_tm.tm_mday,
+			rtc_glink->resp_tm.tm_hour,
+			rtc_glink->resp_tm.tm_min,
+			rtc_glink->resp_tm.tm_sec);
+		break;
+	}
+
+	case RTC_GLINK_SET_REAL_TIME: {
+		const struct qcom_rtc_glink_status_resp *resp = data;
+
+		if (len < sizeof(*resp)) {
+			rtc_glink->error = -EINVAL;
+			break;
+		}
+		rtc_glink->error = le32_to_cpu(resp->return_status) ? -EIO : 0;
+		dev_dbg(rtc_glink->dev, "ACK opcode=0x%x status=%u\n",
+			RTC_GLINK_SET_REAL_TIME,
+			le32_to_cpu(resp->return_status));
+		break;
+	}
+
+	default:
+		spin_unlock_irqrestore(&rtc_glink->state_lock, flags);
+		return;
+	}
+
+	spin_unlock_irqrestore(&rtc_glink->state_lock, flags);
+	complete(&rtc_glink->ack);
+}
+
+static void qcom_rtc_glink_pdr_notify(void *priv, int state)
+{
+	struct qcom_rtc_glink *rtc_glink = priv;
+	unsigned long flags;
+	bool up = (state == SERVREG_SERVICE_STATE_UP);
+
+	spin_lock_irqsave(&rtc_glink->state_lock, flags);
+	rtc_glink->service_up = up;
+	if (!up && rtc_glink->request_pending) {
+		rtc_glink->error = -ECONNRESET;
+		rtc_glink->resp_valid = false;
+		complete(&rtc_glink->ack);
+	}
+	if (!up)
+		rtc_glink->timed_out = false;
+	spin_unlock_irqrestore(&rtc_glink->state_lock, flags);
+}
+
+static int qcom_rtc_glink_read_time(struct device *dev, struct rtc_time *tm)
+{
+	return qcom_rtc_glink_get_time(dev_get_drvdata(dev), tm);
+}
+
+static int qcom_rtc_glink_set_time_dev(struct device *dev, struct rtc_time *tm)
+{
+	return qcom_rtc_glink_set_time(dev_get_drvdata(dev), tm);
+}
+
+static int qcom_rtc_glink_read_alarm(struct device *dev,
+				     struct rtc_wkalrm *alrm)
+{
+	return qcom_rtc_glink_get_alarm(dev_get_drvdata(dev), alrm);
+}
+
+static int qcom_rtc_glink_set_alarm_dev(struct device *dev,
+					struct rtc_wkalrm *alrm)
+{
+	return qcom_rtc_glink_set_alarm(dev_get_drvdata(dev), alrm);
+}
+
+static int qcom_rtc_glink_alarm_irq_enable(struct device *dev,
+					   unsigned int enabled)
+{
+	return qcom_rtc_glink_set_alarm_en(dev_get_drvdata(dev), enabled);
+}
+
+static const struct rtc_class_ops qcom_rtc_glink_rtc_ops = {
+	.read_time = qcom_rtc_glink_read_time,
+	.set_time = qcom_rtc_glink_set_time_dev,
+	.read_alarm = qcom_rtc_glink_read_alarm,
+	.set_alarm = qcom_rtc_glink_set_alarm_dev,
+	.alarm_irq_enable = qcom_rtc_glink_alarm_irq_enable,
+};
+
+static int qcom_rtc_glink_probe(struct auxiliary_device *adev,
+				const struct auxiliary_device_id *id)
+{
+	struct device *dev = &adev->dev;
+	struct qcom_rtc_glink *rtc_glink;
+	struct rtc_time tm;
+	int ret;
+
+	rtc_glink = devm_kzalloc(dev, sizeof(*rtc_glink), GFP_KERNEL);
+	if (!rtc_glink)
+		return -ENOMEM;
+
+	rtc_glink->dev = dev;
+
+	spin_lock_init(&rtc_glink->state_lock);
+	init_completion(&rtc_glink->ack);
+	INIT_WORK(&rtc_glink->alarm_work, qcom_rtc_glink_alarm_work);
+	dev_set_drvdata(dev, rtc_glink);
+
+	rtc_glink->client = devm_pmic_glink_client_alloc(dev,
+							 PMIC_GLINK_OWNER_RTC,
+							 qcom_rtc_glink_callback,
+							 qcom_rtc_glink_pdr_notify,
+							 rtc_glink);
+	if (IS_ERR(rtc_glink->client))
+		return dev_err_probe(dev, PTR_ERR(rtc_glink->client),
+				     "failed to allocate glink client\n");
+
+	pmic_glink_client_register(rtc_glink->client);
+
+	ret = qcom_rtc_glink_get_time(rtc_glink, &tm);
+	if (ret) {
+		pmic_glink_client_unregister(rtc_glink->client);
+		if (ret == -ECONNRESET)
+			return -EPROBE_DEFER;
+		return dev_err_probe(dev, -ENODEV,
+				     "RTC GLINK firmware is not available\n");
+	}
+
+	device_init_wakeup(dev, true);
+
+	rtc_glink->rtc = devm_rtc_allocate_device(dev);
+	if (IS_ERR(rtc_glink->rtc)) {
+		pmic_glink_client_unregister(rtc_glink->client);
+		return PTR_ERR(rtc_glink->rtc);
+	}
+
+	rtc_glink->rtc->ops = &qcom_rtc_glink_rtc_ops;
+	rtc_glink->rtc->range_min = 0;
+	rtc_glink->rtc->range_max = U32_MAX;
+
+	return devm_rtc_register_device(rtc_glink->rtc);
+}
+
+static void qcom_rtc_glink_remove(struct auxiliary_device *adev)
+{
+	struct qcom_rtc_glink *rtc_glink = dev_get_drvdata(&adev->dev);
+
+	pmic_glink_client_unregister(rtc_glink->client);
+	cancel_work_sync(&rtc_glink->alarm_work);
+}
+
+static const struct auxiliary_device_id qcom_rtc_glink_id_table[] = {
+	{ .name = "pmic_glink.rtc-glink" },
+	{}
+};
+
+MODULE_DEVICE_TABLE(auxiliary, qcom_rtc_glink_id_table);
+
+static struct auxiliary_driver qcom_rtc_glink_driver = {
+	.name = "qcom_pmic_rtc_glink",
+	.id_table = qcom_rtc_glink_id_table,
+	.probe = qcom_rtc_glink_probe,
+	.remove = qcom_rtc_glink_remove,
+};
+
+module_auxiliary_driver(qcom_rtc_glink_driver);
+MODULE_DESCRIPTION("Qualcomm PMIC GLINK RTC driver");
+MODULE_LICENSE("GPL");

-- 
2.43.0

Re: [PATCH v2 1/2] soc: qcom: pmic_glink: Add RTC client support

[sashiko-bot]

> On some Qualcomm targets like Hamoa and Glymur, PMIC RTC is only reachable over
> the GLINK interface, so add a new RTC auxiliary device ("rtc-glink") to let the
> new RTC glink driver bind against it, mirroring the existing
> altmode/UCSI/power-supply clients.
> 
> Also add pmic_glink_client_unregister() so the RTC aux client can detach from the
> clients list before unregistering the RTC in order to avoid use-after-free issue
> in a scenario when an alarm IRQ is received during teardown.
> 
> Enable the RTC client on Glymur and Hamoa/X1E80100 variants, as only those
> currently support this RTC client from the firmware side.
> 
> Assisted-by: Claude:claude-sonnet-5
> Signed-off-by: Raj Aryan <raryan@qti.qualcomm.com>
> Signed-off-by: Kamal Wadhwa <kamal.wadhwa@oss.qualcomm.com>

Sashiko has reviewed this patch and found no issues.

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20261103-rtc-glink-cleanup-v2-0-895eb6e8d890@qti.qualcomm.com?part=1

Re: [PATCH v2 2/2] rtc: qcom: rtc-glink: Add new driver to support alarms on Glymur & Hamoa boards

[sashiko-bot]

Thank you for your contribution! Sashiko AI review found 9 potential issue(s) to consider:
- [High] rtc: qcom-glink: permanent -EAGAIN wedge and races in qcom_rtc_glink_request()
- [High] rtc: qcom-glink: UAF and ERR_PTR dereference on qcom_rtc_glink_probe() failure
- [High] rtc: qcom-glink: u32 wrap-around in qcom_rtc_glink_get_alarm() and set_alarm()
- [High] rtc: qcom-glink: indefinite probe deferral in qcom_rtc_glink_probe() with PDR
- [Medium] rtc: qcom-glink: ignored status in qcom_rtc_glink_set_timer_value()
- [Medium] rtc: qcom-glink: unbalanced device_init_wakeup() on probe error and remove
- [Medium] rtc: qcom-glink: returning -EINVAL instead of -ETIME in qcom_rtc_glink_set_alarm()
- [Medium] rtc: qcom-glink: premature client unregister in qcom_rtc_glink_remove()
- [Medium] rtc: qcom-glink: wakeup race from deferring rtc_update_irq() to workqueue
--

commit 3cad1ec82d81d9d80a30f2fc1c26ef5d59d6afbe
Author: Kamal Wadhwa <kamal.wadhwa@oss.qualcomm.com>

rtc: qcom: rtc-glink: Add new driver to support alarms on Glymur & Hamoa boards

This commit adds the rtc-qcom-glink auxiliary driver under pmic_glink to
support RTC time read/write operations and alarm notifications relayed by
remote firmware on Qualcomm Glymur and Hamoa platforms.

Link: https://learn.microsoft.com/en-us/windows-hardware/drivers/bringup/acpi-defined-devices

> diff --git a/drivers/rtc/rtc-qcom-glink.c b/drivers/rtc/rtc-qcom-glink.c
> new file mode 100644
> index 0000000000000..b1394b08e28d2
> --- /dev/null
> +++ b/drivers/rtc/rtc-qcom-glink.c

[ ... ]

> +static int qcom_rtc_glink_request(struct qcom_rtc_glink *rtc_glink,
> +				  void *data, size_t len)
> +{
> +	unsigned long flags;
> +	bool timed_out = false;
> +	int ret;
> +
> +	/*
> +	 * Reinit the completion before request_pending becomes visible to
> +	 * qcom_rtc_glink_pdr_notify(), so a concurrent SSR down-transition
> +	 * can't complete() a stale completion that gets wiped out by
> +	 * reinit_completion() right after.
> +	 */
> +	reinit_completion(&rtc_glink->ack);
> +	rtc_glink->error = 0;
> +	rtc_glink->resp_valid = false;
> +
> +	spin_lock_irqsave(&rtc_glink->state_lock, flags);
> +	if (!rtc_glink->service_up || rtc_glink->timed_out) {
> +		spin_unlock_irqrestore(&rtc_glink->state_lock, flags);
> +		return !rtc_glink->service_up ? -ECONNRESET : -EAGAIN;
> +	}
> +	rtc_glink->request_pending = true;
> +	spin_unlock_irqrestore(&rtc_glink->state_lock, flags);
> +
> +	ret = pmic_glink_send(rtc_glink->client, data, len);
> +	if (!ret && !wait_for_completion_timeout(&rtc_glink->ack, HZ)) {
> +		ret = -ETIMEDOUT;
> +		timed_out = true;
> +	}
> +
> +	spin_lock_irqsave(&rtc_glink->state_lock, flags);
> +	rtc_glink->request_pending = false;
> +	rtc_glink->timed_out |= timed_out;
> +	spin_unlock_irqrestore(&rtc_glink->state_lock, flags);
> +
> +	return ret < 0 ? ret : rtc_glink->error;
> +}

[Severity: High]
As reported by Sashiko on v1 [1], can a timeout in qcom_rtc_glink_request()
permanently wedge the driver into returning -EAGAIN for all subsequent RTC
operations?

When wait_for_completion_timeout() expires, qcom_rtc_glink_request() sets
rtc_glink->timed_out = true, and every later call to qcom_rtc_glink_request()
immediately returns -EAGAIN while rtc_glink->timed_out remains set.
rtc_glink->timed_out is only cleared in qcom_rtc_glink_callback() when a
response arrives while !rtc_glink->request_pending, or in
qcom_rtc_glink_pdr_notify() on a down-transition. If a request times out
because the message was dropped by firmware and no late reply or SSR
down-transition ever occurs (noting that Glymur sets pdr_available = false),
won't rtc_glink->timed_out stay true forever?

In addition, neither qcom_rtc_glink_callback() nor qcom_rtc_glink_pdr_notify()
clears rtc_glink->request_pending under state_lock. If the response or a PDR
down-notification arrives on another CPU right after
wait_for_completion_timeout() returns 0 and before qcom_rtc_glink_request()
re-acquires state_lock:

CPU0                                  CPU1
qcom_rtc_glink_request()
  wait_for_completion_timeout() -> 0
  timed_out = true;
                                      qcom_rtc_glink_callback()
                                        spin_lock_irqsave(&state_lock)
                                        pending = rtc_glink->request_pending;
                                        /* pending is true, consumes reply */
                                        spin_unlock_irqrestore(&state_lock)
  spin_lock_irqsave(&state_lock)
  rtc_glink->request_pending = false;
  rtc_glink->timed_out |= timed_out;
  spin_unlock_irqrestore(&state_lock)

Because CPU1 already consumed the response while request_pending was still
true, no further response will arrive to clear rtc_glink->timed_out. The same
happens if qcom_rtc_glink_pdr_notify() runs with !up in that window and clears
rtc_glink->timed_out before CPU0 sets rtc_glink->timed_out |= true.

Also, in the early exit path of qcom_rtc_glink_request():

	if (!rtc_glink->service_up || rtc_glink->timed_out) {
		spin_unlock_irqrestore(&rtc_glink->state_lock, flags);
		return !rtc_glink->service_up ? -ECONNRESET : -EAGAIN;
	}

rtc_glink->service_up is re-read after dropping state_lock. If service_up was
false under the lock and qcom_rtc_glink_pdr_notify() sets service_up = true
before the return expression is evaluated, qcom_rtc_glink_request() returns
-EAGAIN instead of -ECONNRESET, causing qcom_rtc_glink_probe() to fail with
-ENODEV instead of -EPROBE_DEFER.

> +
> +static int qcom_rtc_glink_set_timer_value(struct qcom_rtc_glink *rtc_glink,
> +					  u32 timer_id, u32 timer_value)
> +{
> +	struct qcom_rtc_glink_set_timer_req msg = {
> +		.hdr.owner = cpu_to_le32(PMIC_GLINK_OWNER_RTC),
> +		.hdr.type = cpu_to_le32(PMIC_GLINK_REQ_RESP),
> +		.hdr.opcode = cpu_to_le32(RTC_GLINK_SET_TIMER_VALUE),
> +		.timer_id = cpu_to_le32(timer_id),
> +		.timer_value = cpu_to_le32(timer_value),
> +	};
> +
> +	dev_dbg(rtc_glink->dev,
> +		"TX opcode=0x%x timer_id=%u timer_value=%u\n",
> +		RTC_GLINK_SET_TIMER_VALUE, timer_id, timer_value);
> +	return qcom_rtc_glink_request(rtc_glink, &msg, sizeof(msg));
> +}

[Severity: Medium]
Does qcom_rtc_glink_set_timer_value() ignore firmware failure statuses
returned in struct qcom_rtc_glink_timer_resp?

The comment above struct qcom_rtc_glink_timer_resp notes that for
RTC_GLINK_SET_TIMER_VALUE, resp->value holds the pass/fail status. However, in
qcom_rtc_glink_callback(), case RTC_GLINK_GET_TIMER_VALUE falls through to
case RTC_GLINK_SET_TIMER_VALUE and unconditionally sets rtc_glink->error = 0:

	case RTC_GLINK_GET_TIMER_VALUE:
		fallthrough;
	case RTC_GLINK_SET_TIMER_VALUE: {
		const struct qcom_rtc_glink_timer_resp *resp = data;
		...
		rtc_glink->resp_value = le32_to_cpu(resp->value);
		rtc_glink->resp_valid = true;
		rtc_glink->error = 0;

And qcom_rtc_glink_set_timer_value() returns qcom_rtc_glink_request() directly
without inspecting rtc_glink->resp_value or verifying that the response opcode
matched the pending request. Should RTC_GLINK_SET_TIMER_VALUE check
le32_to_cpu(resp->value) and set rtc_glink->error = -EIO when non-zero, similar
to RTC_GLINK_SET_REAL_TIME?

[ ... ]

> +static int qcom_rtc_glink_set_alarm(struct qcom_rtc_glink *rtc_glink,
> +				    struct rtc_wkalrm *alarm)
> +{
> +	time64_t alarm_t = rtc_tm_to_time64(&alarm->time);
> +	struct rtc_time now_tm;
> +	time64_t now_real, secs_until_alarm;
> +	u32 fw_current, fw_alarm;
> +	int ret;
> +
> +	ret = qcom_rtc_glink_get_time(rtc_glink, &now_tm);
> +	if (ret)
> +		return ret;
> +
> +	now_real = rtc_tm_to_time64(&now_tm);
> +	secs_until_alarm = alarm_t - now_real;
> +	dev_dbg(rtc_glink->dev, "set_alarm: now_real=%lld alarm_t=%lld delta=%lld\n",
> +		now_real, alarm_t, secs_until_alarm);
> +
> +	if (secs_until_alarm < 0)
> +		return -EINVAL;

[Severity: Medium]
Should qcom_rtc_glink_set_alarm() return -ETIME instead of -EINVAL when the
alarm time has already passed?

In drivers/rtc/interface.c, __rtc_set_alarm() reads the current time, checks
scheduled <= now, and then calls rtc->ops->set_alarm(). Because
qcom_rtc_glink_set_alarm() performs another GLINK round-trip via
qcom_rtc_glink_get_time(), the RTC second can tick past alarm_t during that
IPC call.

The callers of __rtc_set_alarm() in drivers/rtc/interface.c
(rtc_timer_enqueue(), rtc_timer_remove(), and rtc_timer_do_work()) check
specifically for -ETIME to expire the timer immediately via rtc->irqwork.
Returning -EINVAL causes rtc_timer_do_work() to drop the timer after retries
without firing it, leaves the next timer unscheduled in rtc_timer_remove(),
and fails 1-second UIE timers in rtc_timer_enqueue(). Also, if
secs_until_alarm == 0 when alarm->enabled is true, programming fw_alarm =
fw_current + 0 sets an alarm timestamp that expires before the remaining GLINK
requests finish.

> +	if (secs_until_alarm > U32_MAX)
> +		return -ERANGE;
> +
> +	ret = qcom_rtc_glink_get_timer_value(rtc_glink, QCOM_RTC_GLINK_TIME, &fw_current);
> +	if (ret)
> +		return ret;
> +	if (fw_current == U32_MAX)
> +		fw_current = 0;
> +	fw_alarm = fw_current + (u32)secs_until_alarm;
> +	dev_dbg(rtc_glink->dev, "set_alarm: fw_current=%u fw_alarm=%u\n", fw_current, fw_alarm);
> +
> +	ret = qcom_rtc_glink_set_alarm_en(rtc_glink, 0);
> +	if (ret)
> +		return ret;
> +
> +	ret = qcom_rtc_glink_set_timer_value(rtc_glink, QCOM_RTC_GLINK_ALARM_TIME, fw_alarm);
> +	if (ret)
> +		return ret;
> +
> +	return qcom_rtc_glink_set_alarm_en(rtc_glink, alarm->enabled);
> +}
> +
> +static int qcom_rtc_glink_get_alarm(struct qcom_rtc_glink *rtc_glink,
> +				    struct rtc_wkalrm *alarm)
> +{
> +	u32 fw_alarm_time, fw_current, alarm_en;
> +	struct rtc_time now_tm;
> +	time64_t now_real, secs_until_alarm, alarm_real;
> +	int ret;
> +
> +	ret = qcom_rtc_glink_get_timer_value(rtc_glink, QCOM_RTC_GLINK_ALARM_TIME, &fw_alarm_time);
> +	if (ret)
> +		return ret;
> +
> +	ret = qcom_rtc_glink_get_timer_value(rtc_glink, QCOM_RTC_GLINK_TIME, &fw_current);
> +	if (ret)
> +		return ret;
> +
> +	ret = qcom_rtc_glink_get_timer_value(rtc_glink, QCOM_RTC_GLINK_ALARM_ENABLE, &alarm_en);
> +	if (ret)
> +		return ret;
> +	if (fw_current == U32_MAX)
> +		fw_current = 0;
> +
> +	ret = qcom_rtc_glink_get_time(rtc_glink, &now_tm);
> +	if (ret)
> +		return ret;
> +
> +	now_real = rtc_tm_to_time64(&now_tm);
> +	secs_until_alarm = (u32)(fw_alarm_time - fw_current);
> +	alarm_real = now_real + secs_until_alarm;
> +	dev_dbg(rtc_glink->dev, "get_alarm: now_real=%lld fw_current=%u fw_alarm=%u delta=%lld\n",
> +		now_real, fw_current, fw_alarm_time, secs_until_alarm);
> +
> +	rtc_time64_to_tm(alarm_real, &alarm->time);
> +	alarm->enabled = !!alarm_en;
> +	return 0;
> +}

[Severity: High]
What happens in qcom_rtc_glink_get_alarm() when fw_alarm_time is less than
fw_current, such as at boot when no alarm has been programmed (fw_alarm_time
is 0) or after an alarm has already expired?

Because secs_until_alarm is computed as:

	secs_until_alarm = (u32)(fw_alarm_time - fw_current);
	alarm_real = now_real + secs_until_alarm;

any negative delta underflows in 32-bit unsigned arithmetic and wraps to a
timestamp roughly 136 years in the future (exceeding rtc->range_max). During
devm_rtc_register_device(), __devm_rtc_register_device() calls
__rtc_read_alarm() and rtc_initialize_alarm(), which relies on comparing the
alarm time against now to ignore already-expired alarms. Wrapping expired
alarms into the future defeats that check and enqueues stale alarms into
rtc->timerqueue.

Conversely, in qcom_rtc_glink_set_alarm(), fw_alarm = fw_current +
(u32)secs_until_alarm can overflow 32 bits or land on the U32_MAX sentinel
when secs_until_alarm >= U32_MAX - fw_current.

> +
> +static void qcom_rtc_glink_alarm_work(struct work_struct *work)
> +{
> +	struct qcom_rtc_glink *rtc_glink =
> +		container_of(work, struct qcom_rtc_glink, alarm_work);
> +
> +	/*
> +	 * The glink client is registered, and can receive an unsolicited
> +	 * ALARM_EXPIRED, before the RTC device exists - e.g. if the board
> +	 * booted because of a previously armed alarm.
> +	 */
> +	if (!rtc_glink->rtc)
> +		return;
> +
> +	rtc_lock(rtc_glink->rtc);
> +	rtc_update_irq(rtc_glink->rtc, 1, RTC_IRQF | RTC_AF);
> +	rtc_unlock(rtc_glink->rtc);
> +}
> +
> +static void qcom_rtc_glink_callback(const void *data, size_t len, void *priv)
> +{
> +	struct qcom_rtc_glink *rtc_glink = priv;
> +	const struct pmic_glink_hdr *hdr = data;
> +	unsigned long flags;
> +	bool pending;
> +
> +	if (len < sizeof(*hdr))
> +		return;
> +
> +	dev_dbg(rtc_glink->dev,
> +		"RX opcode=0x%x type=0x%x owner=0x%x len=%zu\n",
> +		le32_to_cpu(hdr->opcode), le32_to_cpu(hdr->type),
> +		le32_to_cpu(hdr->owner), len);
> +
> +	if (le32_to_cpu(hdr->opcode) == RTC_GLINK_ALARM_EXPIRED) {
> +		schedule_work(&rtc_glink->alarm_work);
> +		return;
> +	}

[Severity: Medium]
Can deferring rtc_update_irq() to qcom_rtc_glink_alarm_work() race with system
suspend when RTC_GLINK_ALARM_EXPIRED arrives during suspend entry or
suspend-to-idle?

The GLINK RX callback runs from the IPCC interrupt handler
(qcom_ipcc_irq_fn() -> qcom_glink_smem_intr() -> qcom_glink_native_rx() ->
pmic_glink_rpmsg_callback() -> qcom_rtc_glink_callback()), and none of the
underlying IPCC, GLINK, or pmic_glink layers activate a wakeup source. When
qcom_rtc_glink_callback() only calls schedule_work(&rtc_glink->alarm_work) and
returns from the interrupt handler, pm_stay_awake(rtc->dev.parent) inside
rtc_update_irq() is not called until alarm_work gets scheduled and acquires
rtc_lock(rtc_glink->rtc) (which is also held by rtc_suspend() during
dpm_suspend()).

Since rtc_update_irq() is safe to call from hardirq context, checks
IS_ERR_OR_NULL(rtc), calls pm_stay_awake(rtc->dev.parent) immediately, and
schedules rtc->irqwork (which takes rtc->ops_lock itself), could
qcom_rtc_glink_callback() call rtc_update_irq() directly instead of bouncing
through alarm_work?

[ ... ]

> +static int qcom_rtc_glink_probe(struct auxiliary_device *adev,
> +				const struct auxiliary_device_id *id)
> +{

[ ... ]

> +	pmic_glink_client_register(rtc_glink->client);
> +
> +	ret = qcom_rtc_glink_get_time(rtc_glink, &tm);
> +	if (ret) {
> +		pmic_glink_client_unregister(rtc_glink->client);
> +		if (ret == -ECONNRESET)
> +			return -EPROBE_DEFER;
> +		return dev_err_probe(dev, -ENODEV,
> +				     "RTC GLINK firmware is not available\n");
> +	}

[Severity: High]
On PDR-enabled platforms such as Hamoa (X1E80100), can returning -EPROBE_DEFER
here when ret == -ECONNRESET leave the driver permanently on the deferred
probe list?

In drivers/soc/qcom/pmic_glink.c, pmic_glink_probe() registers the rtc-glink
auxiliary device before pdr_add_lookup() completes and before __pmic_glink is
set, so pg->client_state is initially 0 and qcom_rtc_glink_get_time() returns
-ECONNRESET. On Hamoa (PMIC_RTR_ADSP_APPS), pg->pdr_available is true, so
pg->pdr_state only transitions to SERVREG_SERVICE_STATE_UP asynchronously in
pmic_glink_pdr_callback() from a QMI worker thread. Because
pmic_glink_pdr_callback() only notifies registered clients and does not bind a
device, it does not trigger driver_deferred_probe_trigger(). If
pmic_glink_pdr_callback() runs after device probing has settled, what will
re-trigger probing of qcom_rtc_glink_probe()?

> +
> +	device_init_wakeup(dev, true);

[Severity: Medium]
Does calling unmanaged device_init_wakeup(dev, true) here leak the wakeup
source on probe failure or when the driver is unbound in
qcom_rtc_glink_remove()?

Neither the error paths below nor qcom_rtc_glink_remove() call
device_init_wakeup(dev, false). Should this use devm_device_init_wakeup(dev)
instead?

> +
> +	rtc_glink->rtc = devm_rtc_allocate_device(dev);
> +	if (IS_ERR(rtc_glink->rtc)) {
> +		pmic_glink_client_unregister(rtc_glink->client);
> +		return PTR_ERR(rtc_glink->rtc);
> +	}
> +
> +	rtc_glink->rtc->ops = &qcom_rtc_glink_rtc_ops;
> +	rtc_glink->rtc->range_min = 0;
> +	rtc_glink->rtc->range_max = U32_MAX;
> +
> +	return devm_rtc_register_device(rtc_glink->rtc);
> +}

[Severity: High]
As reported by Sashiko on v1 [2], can a probe failure after
pmic_glink_client_register() cause a use-after-free or an ERR_PTR dereference
in qcom_rtc_glink_alarm_work()?

Once pmic_glink_client_register() has been called, qcom_rtc_glink_callback()
can receive an unsolicited RTC_GLINK_ALARM_EXPIRED message and schedule
rtc_glink->alarm_work:

1. None of the error paths after pmic_glink_client_register()
   (qcom_rtc_glink_get_time(), devm_rtc_allocate_device(), or
   devm_rtc_register_device()) cancel rtc_glink->alarm_work before devres
   frees rtc_glink.

2. If devm_rtc_allocate_device() fails, rtc_glink->rtc is assigned the ERR_PTR
   before the IS_ERR() check and before pmic_glink_client_unregister() is
   called. Because qcom_rtc_glink_alarm_work() only checks if
   (!rtc_glink->rtc), a concurrent work execution will pass that check and
   dereference the ERR_PTR in rtc_lock(rtc_glink->rtc).

3. If devm_rtc_register_device() fails, pmic_glink_client_unregister() is not
   called at all. Because devm_pmic_glink_client_alloc() was registered before
   devm_rtc_allocate_device(), devres releases rtc_glink->rtc before removing
   the GLINK client, leaving rtc_glink->rtc as a non-NULL dangling pointer
   while callbacks and alarm_work can still run.

> +
> +static void qcom_rtc_glink_remove(struct auxiliary_device *adev)
> +{
> +	struct qcom_rtc_glink *rtc_glink = dev_get_drvdata(&adev->dev);
> +
> +	pmic_glink_client_unregister(rtc_glink->client);
> +	cancel_work_sync(&rtc_glink->alarm_work);
> +}

[Severity: Medium]
In qcom_rtc_glink_remove(), pmic_glink_client_unregister() is called before
devres unregisters rtc_glink->rtc, and without clearing rtc_glink->service_up.

If an RTC operation or rtc->irqwork (scheduled by qcom_rtc_glink_alarm_work()
via rtc_update_irq()) runs while the RTC device is still registered, won't it
see rtc_glink->service_up == true, send a GLINK request whose response can
never arrive, block for HZ until timeout, and cause rtc_timer_do_work() to
bail out on -ETIMEDOUT without calling pm_relax(rtc->dev.parent)?

[1] https://lore.kernel.org/r/20260806210934.485441F000E9@smtp.kernel.org
[2] https://lore.kernel.org/r/20260806210934.485441F000E9@smtp.kernel.org

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20261103-rtc-glink-cleanup-v2-0-895eb6e8d890@qti.qualcomm.com?part=2

Re: [PATCH v2 1/2] soc: qcom: pmic_glink: Add RTC client support

[Abel Vesa]

On 26-11-03 04:12:17, Kamal Wadhwa wrote:
> On some Qualcomm targets like Hamoa and Glymur, PMIC RTC is only reachable over
> the GLINK interface, so add a new RTC auxiliary device ("rtc-glink") to let the
> new RTC glink driver bind against it, mirroring the existing
> altmode/UCSI/power-supply clients.
> 
> Also add pmic_glink_client_unregister() so the RTC aux client can detach from the
> clients list before unregistering the RTC in order to avoid use-after-free issue
> in a scenario when an alarm IRQ is received during teardown.
> 
> Enable the RTC client on Glymur and Hamoa/X1E80100 variants, as only those
> currently support this RTC client from the firmware side.
> 
> Assisted-by: Claude:claude-sonnet-5
> Signed-off-by: Raj Aryan <raryan@qti.qualcomm.com>
> Signed-off-by: Kamal Wadhwa <kamal.wadhwa@oss.qualcomm.com>

Reviewed-by: Abel Vesa <abel.vesa@oss.qualcomm.com>