* [PATCH 0/3] soc: qcom: apr: improve RX processing
@ 2026-05-14 15:50 Srinivas Kandagatla
2026-05-14 15:50 ` [PATCH 1/3] soc: qcom: apr: Remove service from IDR before registration failure Srinivas Kandagatla
` (3 more replies)
0 siblings, 4 replies; 9+ messages in thread
From: Srinivas Kandagatla @ 2026-05-14 15:50 UTC (permalink / raw)
To: andersson
Cc: srini, konradybcio, linux-sound, linux-arm-msm, linux-kernel,
Srinivas Kandagatla
Hi all,
This series contains a couple of APR enhancements and a rework of the RX
processing path.
The first two patches makes code more robust for trival issues in the APR core.
Remove stale service entries from the IDR when apr_add_device()
fails before the device is registered.
Validate incoming packet lengths using the header size appropriate
for the active packet router type (APR or GPR).
The final patch moves RX buffering and work items from the packet
router to individual services.
Today all incoming packets are serialized through a single work item and
a global RX queue. If one service callback blocks or takes a long time
to complete, packet processing for all other services is delayed. This
can lead to command timeouts when responses are stuck behind unrelated
work.
With this change, packets are queued and processed on a per-service
basis, preserving message ordering within each service while allowing
unrelated services to make progress independently. Since queued packets
may outlive service removal, a reference count is added to keep the
service object alive until all queued packets have been processed.
The shared APR workqueue is also converted to an unbound reclaim
workqueue so multiple services can process packets in parallel.
Thanks,
Srini
Srinivas Kandagatla (3):
soc: qcom: apr: Remove service from IDR before registration failure
soc: qcom: apr: Check response packet length by router type
soc: qcom: apr: Process RX messages using per-service work items
drivers/soc/qcom/apr.c | 283 ++++++++++++++++++++++++-----------
include/linux/soc/qcom/apr.h | 6 +
2 files changed, 200 insertions(+), 89 deletions(-)
--
2.47.3
^ permalink raw reply [flat|nested] 9+ messages in thread
* [PATCH 1/3] soc: qcom: apr: Remove service from IDR before registration failure
2026-05-14 15:50 [PATCH 0/3] soc: qcom: apr: improve RX processing Srinivas Kandagatla
@ 2026-05-14 15:50 ` Srinivas Kandagatla
2026-05-14 15:50 ` [PATCH 2/3] soc: qcom: apr: Check response packet length by router type Srinivas Kandagatla
` (2 subsequent siblings)
3 siblings, 0 replies; 9+ messages in thread
From: Srinivas Kandagatla @ 2026-05-14 15:50 UTC (permalink / raw)
To: andersson
Cc: srini, konradybcio, linux-sound, linux-arm-msm, linux-kernel,
Srinivas Kandagatla
apr_add_device() may fail before device register, do cleanup of idr from
global apr->svcs_idr list so that we are not leaving any stale id in the
list.
Signed-off-by: Srinivas Kandagatla <srinivas.kandagatla@oss.qualcomm.com>
---
drivers/soc/qcom/apr.c | 1 +
1 file changed, 1 insertion(+)
diff --git a/drivers/soc/qcom/apr.c b/drivers/soc/qcom/apr.c
index ea7f83916d8d..127204c195ea 100644
--- a/drivers/soc/qcom/apr.c
+++ b/drivers/soc/qcom/apr.c
@@ -466,6 +466,7 @@ static int apr_add_device(struct device *dev, struct device_node *np,
1, &adev->service_path);
if (ret < 0 && ret != -EINVAL) {
dev_err(dev, "Failed to read second value of qcom,protection-domain\n");
+ idr_remove(&apr->svcs_idr, svc_id);
goto out;
}
--
2.47.3
^ permalink raw reply related [flat|nested] 9+ messages in thread
* [PATCH 2/3] soc: qcom: apr: Check response packet length by router type
2026-05-14 15:50 [PATCH 0/3] soc: qcom: apr: improve RX processing Srinivas Kandagatla
2026-05-14 15:50 ` [PATCH 1/3] soc: qcom: apr: Remove service from IDR before registration failure Srinivas Kandagatla
@ 2026-05-14 15:50 ` Srinivas Kandagatla
2026-05-21 9:45 ` Konrad Dybcio
2026-05-14 15:50 ` [PATCH 3/3] soc: qcom: apr: Process RX messages using per-service work items Srinivas Kandagatla
2026-05-26 22:26 ` [PATCH 0/3] soc: qcom: apr: improve RX processing Val Packett
3 siblings, 1 reply; 9+ messages in thread
From: Srinivas Kandagatla @ 2026-05-14 15:50 UTC (permalink / raw)
To: andersson
Cc: srini, konradybcio, linux-sound, linux-arm-msm, linux-kernel,
Srinivas Kandagatla
apr_callback() currently validates all received packets against
APR_HDR_SIZE before queueing them for the RX worker. This is not correct
for GPR packets, which use a different header size.
Validate the received packet length against the header size matching the
packet router type before copying the packet.
Signed-off-by: Srinivas Kandagatla <srinivas.kandagatla@oss.qualcomm.com>
---
drivers/soc/qcom/apr.c | 17 ++++++++++++++---
1 file changed, 14 insertions(+), 3 deletions(-)
diff --git a/drivers/soc/qcom/apr.c b/drivers/soc/qcom/apr.c
index 127204c195ea..68b357462438 100644
--- a/drivers/soc/qcom/apr.c
+++ b/drivers/soc/qcom/apr.c
@@ -165,9 +165,20 @@ static int apr_callback(struct rpmsg_device *rpdev, void *buf,
struct apr_rx_buf *abuf;
unsigned long flags;
- if (len <= APR_HDR_SIZE) {
- dev_err(apr->dev, "APR: Improper apr pkt received:%p %d\n",
- buf, len);
+ switch (apr->type) {
+ case PR_TYPE_APR:
+ if (len <= APR_HDR_SIZE) {
+ dev_err(apr->dev, "APR: Improper apr pkt received:%p %d\n", buf, len);
+ return -EINVAL;
+ }
+ break;
+ case PR_TYPE_GPR:
+ if (len <= GPR_HDR_SIZE) {
+ dev_err(apr->dev, "APR: Improper gpr pkt received:%p %d\n", buf, len);
+ return -EINVAL;
+ }
+ break;
+ default:
return -EINVAL;
}
--
2.47.3
^ permalink raw reply related [flat|nested] 9+ messages in thread
* [PATCH 3/3] soc: qcom: apr: Process RX messages using per-service work items
2026-05-14 15:50 [PATCH 0/3] soc: qcom: apr: improve RX processing Srinivas Kandagatla
2026-05-14 15:50 ` [PATCH 1/3] soc: qcom: apr: Remove service from IDR before registration failure Srinivas Kandagatla
2026-05-14 15:50 ` [PATCH 2/3] soc: qcom: apr: Check response packet length by router type Srinivas Kandagatla
@ 2026-05-14 15:50 ` Srinivas Kandagatla
2026-05-21 22:28 ` Bjorn Andersson
2026-05-26 22:26 ` [PATCH 0/3] soc: qcom: apr: improve RX processing Val Packett
3 siblings, 1 reply; 9+ messages in thread
From: Srinivas Kandagatla @ 2026-05-14 15:50 UTC (permalink / raw)
To: andersson
Cc: srini, konradybcio, linux-sound, linux-arm-msm, linux-kernel,
Srinivas Kandagatla
The APR transport currently serializes all incoming packets through a
single work item and a global RX queue. If one service callback blocks
or takes a long time to complete, packet processing for all other
services is delayed.
Move RX buffering and work items from the packet router to each
individual service. Incoming packets are queued on a per-service list
and processed by that service's work item, allowing unrelated services
to make progress independently while preserving message ordering within
each service.
Since queued packets may outlive service removal, add a reference count
to keep the service object alive until all queued packets have been
processed, and reject new packets once the service begins shutting down.
Switch the shared APR workqueue to an unbound reclaim workqueue so that
multiple services can process packets in parallel.
This also addresses the random CMD timeouts seen with audio commands
that are sent to DSP which timeout on the response as they are waiting in
the queue for other commands to finish.
Signed-off-by: Srinivas Kandagatla <srinivas.kandagatla@oss.qualcomm.com>
---
drivers/soc/qcom/apr.c | 265 +++++++++++++++++++++++------------
include/linux/soc/qcom/apr.h | 6 +
2 files changed, 185 insertions(+), 86 deletions(-)
diff --git a/drivers/soc/qcom/apr.c b/drivers/soc/qcom/apr.c
index 68b357462438..f25219b48cdc 100644
--- a/drivers/soc/qcom/apr.c
+++ b/drivers/soc/qcom/apr.c
@@ -28,14 +28,11 @@ struct packet_router {
struct rpmsg_endpoint *ch;
struct device *dev;
spinlock_t svcs_lock;
- spinlock_t rx_lock;
struct idr svcs_idr;
int dest_domain_id;
int type;
struct pdr_handle *pdr;
struct workqueue_struct *rxwq;
- struct work_struct rx_work;
- struct list_head rx_list;
};
struct apr_rx_buf {
@@ -74,54 +71,58 @@ int apr_send_pkt(struct apr_device *adev, struct apr_pkt *pkt)
}
EXPORT_SYMBOL_GPL(apr_send_pkt);
-void gpr_free_port(gpr_port_t *port)
+static void apr_svc_release(struct kref *ref)
{
- struct packet_router *gpr = port->pr;
- unsigned long flags;
+ struct pkt_router_svc *svc;
- spin_lock_irqsave(&gpr->svcs_lock, flags);
- idr_remove(&gpr->svcs_idr, port->id);
- spin_unlock_irqrestore(&gpr->svcs_lock, flags);
+ svc = container_of(ref, struct pkt_router_svc, refcount);
- kfree(port);
+ if (svc->dynamic_svc)
+ kfree(svc);
}
-EXPORT_SYMBOL_GPL(gpr_free_port);
-gpr_port_t *gpr_alloc_port(struct apr_device *gdev, struct device *dev,
- gpr_port_cb cb, void *priv)
+static void apr_svc_get(struct pkt_router_svc *svc)
{
- struct packet_router *pr = dev_get_drvdata(gdev->dev.parent);
- gpr_port_t *port;
- struct pkt_router_svc *svc;
- int id;
+ kref_get(&svc->refcount);
+}
- port = kzalloc_obj(*port);
- if (!port)
- return ERR_PTR(-ENOMEM);
+static void apr_svc_put(struct pkt_router_svc *svc)
+{
+ kref_put(&svc->refcount, apr_svc_release);
+}
- svc = port;
- svc->callback = cb;
- svc->pr = pr;
- svc->priv = priv;
- svc->dev = dev;
- spin_lock_init(&svc->lock);
+static void apr_svc_purge_rx(struct pkt_router_svc *svc)
+{
+ struct apr_rx_buf *abuf, *tmp;
+ unsigned long flags;
- spin_lock(&pr->svcs_lock);
- id = idr_alloc_cyclic(&pr->svcs_idr, svc, GPR_DYNAMIC_PORT_START,
- GPR_DYNAMIC_PORT_END, GFP_ATOMIC);
- if (id < 0) {
- dev_err(dev, "Unable to allocate dynamic GPR src port\n");
- kfree(port);
- spin_unlock(&pr->svcs_lock);
- return ERR_PTR(id);
+ spin_lock_irqsave(&svc->lock, flags);
+ list_for_each_entry_safe(abuf, tmp, &svc->rx_list, node) {
+ list_del(&abuf->node);
+ kfree(abuf);
+ apr_svc_put(svc);
}
+ spin_unlock_irqrestore(&svc->lock, flags);
+}
- svc->id = id;
- spin_unlock(&pr->svcs_lock);
+void gpr_free_port(gpr_port_t *port)
+{
+ struct packet_router *gpr = port->pr;
+ unsigned long flags;
- return port;
+ spin_lock_irqsave(&port->lock, flags);
+ port->dying = true;
+ spin_unlock_irqrestore(&port->lock, flags);
+
+ spin_lock_irqsave(&gpr->svcs_lock, flags);
+ idr_remove(&gpr->svcs_idr, port->id);
+ spin_unlock_irqrestore(&gpr->svcs_lock, flags);
+
+ cancel_work_sync(&port->rx_work);
+ apr_svc_purge_rx(port);
+ apr_svc_put(port);
}
-EXPORT_SYMBOL_GPL(gpr_alloc_port);
+EXPORT_SYMBOL_GPL(gpr_free_port);
static int pkt_router_send_svc_pkt(struct pkt_router_svc *svc, const struct gpr_pkt *pkt)
{
@@ -155,15 +156,53 @@ static void apr_dev_release(struct device *dev)
{
struct apr_device *adev = to_apr_device(dev);
+ cancel_work_sync(&adev->svc.rx_work);
+ apr_svc_purge_rx(&adev->svc);
+ apr_svc_put(&adev->svc);
kfree(adev);
}
+static struct pkt_router_svc *apr_find_svc(struct packet_router *apr, void *buf)
+{
+ struct pkt_router_svc *svc;
+ unsigned long flags;
+ uint32_t svc_id;
+ struct apr_hdr *ahdr;
+ struct gpr_hdr *ghdr;
+
+ switch (apr->type) {
+ case PR_TYPE_APR:
+ ahdr = buf;
+ svc_id = ahdr->dest_svc;
+ break;
+ case PR_TYPE_GPR:
+ ghdr = buf;
+ svc_id = ghdr->dest_port;
+ break;
+ default:
+ dev_err(apr->dev, "Invalid Packet Router\n");
+ return NULL;
+ }
+
+ spin_lock_irqsave(&apr->svcs_lock, flags);
+ svc = idr_find(&apr->svcs_idr, svc_id);
+ if (svc)
+ apr_svc_get(svc);
+ spin_unlock_irqrestore(&apr->svcs_lock, flags);
+
+ if (!svc)
+ dev_err(apr->dev, "APR: service is not registered (%d)\n", svc_id);
+
+ return svc;
+}
+
static int apr_callback(struct rpmsg_device *rpdev, void *buf,
int len, void *priv, u32 addr)
{
struct packet_router *apr = dev_get_drvdata(&rpdev->dev);
struct apr_rx_buf *abuf;
unsigned long flags;
+ struct pkt_router_svc *svc;
switch (apr->type) {
case PR_TYPE_APR:
@@ -189,24 +228,35 @@ static int apr_callback(struct rpmsg_device *rpdev, void *buf,
abuf->len = len;
memcpy(abuf->buf, buf, len);
- spin_lock_irqsave(&apr->rx_lock, flags);
- list_add_tail(&abuf->node, &apr->rx_list);
- spin_unlock_irqrestore(&apr->rx_lock, flags);
+ svc = apr_find_svc(apr, buf);
+ if (!svc) {
+ kfree(abuf);
+ return 0;
+ }
+
+ spin_lock_irqsave(&svc->lock, flags);
+ if (svc->dying) {
+ spin_unlock_irqrestore(&svc->lock, flags);
+ kfree(abuf);
+ apr_svc_put(svc);
+ return 0;
+ }
+
+ list_add_tail(&abuf->node, &svc->rx_list);
+ spin_unlock_irqrestore(&svc->lock, flags);
- queue_work(apr->rxwq, &apr->rx_work);
+ queue_work(apr->rxwq, &svc->rx_work);
return 0;
}
-static int apr_do_rx_callback(struct packet_router *apr, struct apr_rx_buf *abuf)
+static int apr_do_rx_callback(struct pkt_router_svc *svc, struct apr_rx_buf *abuf)
{
- uint16_t hdr_size, msg_type, ver, svc_id;
- struct pkt_router_svc *svc;
- struct apr_device *adev;
+ uint16_t hdr_size, msg_type, ver;
+ struct apr_device *adev = NULL;
struct apr_driver *adrv = NULL;
struct apr_resp_pkt resp;
struct apr_hdr *hdr;
- unsigned long flags;
void *buf = abuf->buf;
int len = abuf->len;
@@ -217,18 +267,18 @@ static int apr_do_rx_callback(struct packet_router *apr, struct apr_rx_buf *abuf
hdr_size = APR_HDR_FIELD_SIZE_BYTES(hdr->hdr_field);
if (hdr_size < APR_HDR_SIZE) {
- dev_err(apr->dev, "APR: Wrong hdr size:%d\n", hdr_size);
+ dev_err(svc->dev, "APR: Wrong hdr size:%d\n", hdr_size);
return -EINVAL;
}
if (hdr->pkt_size < APR_HDR_SIZE || hdr->pkt_size != len) {
- dev_err(apr->dev, "APR: Wrong packet size\n");
+ dev_err(svc->dev, "APR: Wrong packet size\n");
return -EINVAL;
}
msg_type = APR_HDR_FIELD_MT(hdr->hdr_field);
if (msg_type >= APR_MSG_TYPE_MAX) {
- dev_err(apr->dev, "APR: Wrong message type: %d\n", msg_type);
+ dev_err(svc->dev, "APR: Wrong message type: %d\n", msg_type);
return -EINVAL;
}
@@ -236,22 +286,17 @@ static int apr_do_rx_callback(struct packet_router *apr, struct apr_rx_buf *abuf
hdr->dest_domain >= APR_DOMAIN_MAX ||
hdr->src_svc >= APR_SVC_MAX ||
hdr->dest_svc >= APR_SVC_MAX) {
- dev_err(apr->dev, "APR: Wrong APR header\n");
+ dev_err(svc->dev, "APR: Wrong APR header\n");
return -EINVAL;
}
- svc_id = hdr->dest_svc;
- spin_lock_irqsave(&apr->svcs_lock, flags);
- svc = idr_find(&apr->svcs_idr, svc_id);
if (svc && svc->dev->driver) {
adev = svc_to_apr_device(svc);
adrv = to_apr_driver(adev->dev.driver);
}
- spin_unlock_irqrestore(&apr->svcs_lock, flags);
if (!adrv || !adev) {
- dev_err(apr->dev, "APR: service is not registered (%d)\n",
- svc_id);
+ dev_err(svc->dev, "APR: service device not found\n");
return -EINVAL;
}
@@ -270,13 +315,11 @@ static int apr_do_rx_callback(struct packet_router *apr, struct apr_rx_buf *abuf
return 0;
}
-static int gpr_do_rx_callback(struct packet_router *gpr, struct apr_rx_buf *abuf)
+static int gpr_do_rx_callback(struct pkt_router_svc *svc, struct apr_rx_buf *abuf)
{
uint16_t hdr_size, ver;
- struct pkt_router_svc *svc = NULL;
struct gpr_resp_pkt resp;
struct gpr_hdr *hdr;
- unsigned long flags;
void *buf = abuf->buf;
int len = abuf->len;
@@ -287,12 +330,12 @@ static int gpr_do_rx_callback(struct packet_router *gpr, struct apr_rx_buf *abuf
hdr_size = hdr->hdr_size;
if (hdr_size < GPR_PKT_HEADER_WORD_SIZE) {
- dev_err(gpr->dev, "GPR: Wrong hdr size:%d\n", hdr_size);
+ dev_err(svc->dev, "GPR: Wrong hdr size:%d\n", hdr_size);
return -EINVAL;
}
if (hdr->pkt_size < GPR_PKT_HEADER_BYTE_SIZE || hdr->pkt_size != len) {
- dev_err(gpr->dev, "GPR: Wrong packet size\n");
+ dev_err(svc->dev, "GPR: Wrong packet size\n");
return -EINVAL;
}
@@ -306,49 +349,91 @@ static int gpr_do_rx_callback(struct packet_router *gpr, struct apr_rx_buf *abuf
if (resp.payload_size > 0)
resp.payload = buf + (hdr_size * 4);
-
- spin_lock_irqsave(&gpr->svcs_lock, flags);
- svc = idr_find(&gpr->svcs_idr, hdr->dest_port);
- spin_unlock_irqrestore(&gpr->svcs_lock, flags);
-
- if (!svc) {
- dev_err(gpr->dev, "GPR: Port(%x) is not registered\n",
- hdr->dest_port);
- return -EINVAL;
- }
-
if (svc->callback)
svc->callback(&resp, svc->priv, 0);
return 0;
}
-static void apr_rxwq(struct work_struct *work)
+static void apr_service_rxwq(struct work_struct *work)
{
- struct packet_router *apr = container_of(work, struct packet_router, rx_work);
+ struct pkt_router_svc *svc = container_of(work, struct pkt_router_svc, rx_work);
+ struct packet_router *apr = svc->pr;
struct apr_rx_buf *abuf, *b;
unsigned long flags;
- if (!list_empty(&apr->rx_list)) {
- list_for_each_entry_safe(abuf, b, &apr->rx_list, node) {
+ for (;;) {
+ LIST_HEAD(local);
+
+ spin_lock_irqsave(&svc->lock, flags);
+ if (list_empty(&svc->rx_list)) {
+ spin_unlock_irqrestore(&svc->lock, flags);
+ break;
+ }
+
+ list_splice_init(&svc->rx_list, &local);
+ spin_unlock_irqrestore(&svc->lock, flags);
+
+ list_for_each_entry_safe(abuf, b, &local, node) {
switch (apr->type) {
case PR_TYPE_APR:
- apr_do_rx_callback(apr, abuf);
+ apr_do_rx_callback(svc, abuf);
break;
case PR_TYPE_GPR:
- gpr_do_rx_callback(apr, abuf);
+ gpr_do_rx_callback(svc, abuf);
break;
default:
break;
}
- spin_lock_irqsave(&apr->rx_lock, flags);
+ apr_svc_put(svc);
list_del(&abuf->node);
- spin_unlock_irqrestore(&apr->rx_lock, flags);
kfree(abuf);
}
}
}
+gpr_port_t *gpr_alloc_port(struct apr_device *gdev, struct device *dev,
+ gpr_port_cb cb, void *priv)
+{
+ struct packet_router *pr = dev_get_drvdata(gdev->dev.parent);
+ gpr_port_t *port;
+ struct pkt_router_svc *svc;
+ int id;
+
+ port = kzalloc_obj(*port);
+ if (!port)
+ return ERR_PTR(-ENOMEM);
+
+ svc = port;
+ svc->callback = cb;
+ svc->pr = pr;
+ svc->priv = priv;
+ svc->dying = false;
+ svc->dynamic_svc = true;
+ svc->dev = dev;
+ spin_lock_init(&svc->lock);
+
+ INIT_WORK(&svc->rx_work, apr_service_rxwq);
+ INIT_LIST_HEAD(&svc->rx_list);
+ kref_init(&svc->refcount);
+
+ spin_lock(&pr->svcs_lock);
+ id = idr_alloc_cyclic(&pr->svcs_idr, svc, GPR_DYNAMIC_PORT_START,
+ GPR_DYNAMIC_PORT_END, GFP_ATOMIC);
+ if (id < 0) {
+ dev_err(dev, "Unable to allocate dynamic GPR src port\n");
+ kfree(port);
+ spin_unlock(&pr->svcs_lock);
+ return ERR_PTR(id);
+ }
+
+ svc->id = id;
+ spin_unlock(&pr->svcs_lock);
+
+ return port;
+}
+EXPORT_SYMBOL_GPL(gpr_alloc_port);
+
static int apr_device_match(struct device *dev, const struct device_driver *drv)
{
struct apr_device *adev = to_apr_device(dev);
@@ -390,6 +475,11 @@ static void apr_device_remove(struct device *dev)
struct apr_device *adev = to_apr_device(dev);
struct apr_driver *adrv = to_apr_driver(dev->driver);
struct packet_router *apr = dev_get_drvdata(adev->dev.parent);
+ unsigned long flags;
+
+ spin_lock_irqsave(&adev->svc.lock, flags);
+ adev->svc.dying = true;
+ spin_unlock_irqrestore(&adev->svc.lock, flags);
if (adrv->remove)
adrv->remove(adev);
@@ -437,9 +527,15 @@ static int apr_add_device(struct device *dev, struct device_node *np,
svc->id = svc_id;
svc->pr = apr;
svc->priv = adev;
+ svc->dying = false;
svc->dev = dev;
+ svc->dynamic_svc = false;
spin_lock_init(&svc->lock);
+ INIT_WORK(&svc->rx_work, apr_service_rxwq);
+ INIT_LIST_HEAD(&svc->rx_list);
+ kref_init(&svc->refcount);
+
adev->domain_id = domain_id;
if (np)
@@ -488,7 +584,6 @@ static int apr_add_device(struct device *dev, struct device_node *np,
dev_err(dev, "device_register failed: %d\n", ret);
put_device(&adev->dev);
}
-
out:
return ret;
}
@@ -629,12 +724,12 @@ static int apr_probe(struct rpmsg_device *rpdev)
dev_set_drvdata(dev, apr);
apr->ch = rpdev->ept;
apr->dev = dev;
- apr->rxwq = create_singlethread_workqueue("qcom_apr_rx");
+
+ apr->rxwq = alloc_workqueue("qcom_apr_rx", WQ_UNBOUND | WQ_MEM_RECLAIM, 0);
if (!apr->rxwq) {
dev_err(apr->dev, "Failed to start Rx WQ\n");
return -ENOMEM;
}
- INIT_WORK(&apr->rx_work, apr_rxwq);
apr->pdr = pdr_handle_alloc(apr_pd_status, apr);
if (IS_ERR(apr->pdr)) {
@@ -643,8 +738,6 @@ static int apr_probe(struct rpmsg_device *rpdev)
goto destroy_wq;
}
- INIT_LIST_HEAD(&apr->rx_list);
- spin_lock_init(&apr->rx_lock);
spin_lock_init(&apr->svcs_lock);
idr_init(&apr->svcs_idr);
diff --git a/include/linux/soc/qcom/apr.h b/include/linux/soc/qcom/apr.h
index 58fa1df96347..f5bc55c3d025 100644
--- a/include/linux/soc/qcom/apr.h
+++ b/include/linux/soc/qcom/apr.h
@@ -3,6 +3,7 @@
#ifndef __QCOM_APR_H_
#define __QCOM_APR_H_
+#include <linux/kref.h>
#include <linux/spinlock.h>
#include <linux/device.h>
#include <linux/mod_devicetable.h>
@@ -129,6 +130,11 @@ struct pkt_router_svc {
gpr_port_cb callback;
struct packet_router *pr;
spinlock_t lock;
+ struct work_struct rx_work;
+ struct list_head rx_list;
+ struct kref refcount;
+ bool dying;
+ bool dynamic_svc;
int id;
void *priv;
};
--
2.47.3
^ permalink raw reply related [flat|nested] 9+ messages in thread
* Re: [PATCH 2/3] soc: qcom: apr: Check response packet length by router type
2026-05-14 15:50 ` [PATCH 2/3] soc: qcom: apr: Check response packet length by router type Srinivas Kandagatla
@ 2026-05-21 9:45 ` Konrad Dybcio
2026-05-21 17:19 ` Srinivas Kandagatla
0 siblings, 1 reply; 9+ messages in thread
From: Konrad Dybcio @ 2026-05-21 9:45 UTC (permalink / raw)
To: Srinivas Kandagatla, andersson
Cc: srini, konradybcio, linux-sound, linux-arm-msm, linux-kernel
On 5/14/26 5:50 PM, Srinivas Kandagatla wrote:
> apr_callback() currently validates all received packets against
> APR_HDR_SIZE before queueing them for the RX worker. This is not correct
> for GPR packets, which use a different header size.
>
> Validate the received packet length against the header size matching the
> packet router type before copying the packet.
>
> Signed-off-by: Srinivas Kandagatla <srinivas.kandagatla@oss.qualcomm.com>
> ---
> drivers/soc/qcom/apr.c | 17 ++++++++++++++---
> 1 file changed, 14 insertions(+), 3 deletions(-)
>
> diff --git a/drivers/soc/qcom/apr.c b/drivers/soc/qcom/apr.c
> index 127204c195ea..68b357462438 100644
> --- a/drivers/soc/qcom/apr.c
> +++ b/drivers/soc/qcom/apr.c
> @@ -165,9 +165,20 @@ static int apr_callback(struct rpmsg_device *rpdev, void *buf,
> struct apr_rx_buf *abuf;
> unsigned long flags;
>
> - if (len <= APR_HDR_SIZE) {
> - dev_err(apr->dev, "APR: Improper apr pkt received:%p %d\n",
> - buf, len);
> + switch (apr->type) {
> + case PR_TYPE_APR:
> + if (len <= APR_HDR_SIZE) {
> + dev_err(apr->dev, "APR: Improper apr pkt received:%p %d\n", buf, len);
> + return -EINVAL;
> + }
> + break;
> + case PR_TYPE_GPR:
> + if (len <= GPR_HDR_SIZE) {
> + dev_err(apr->dev, "APR: Improper gpr pkt received:%p %d\n", buf, len);
It first says "APR:" and then "[ag]pr" later, please at least make the
case consistent
> + return -EINVAL;
> + }
> + break;
Switch seems a little far-fetched, unless there'll be more packet types
Konrad
^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH 2/3] soc: qcom: apr: Check response packet length by router type
2026-05-21 9:45 ` Konrad Dybcio
@ 2026-05-21 17:19 ` Srinivas Kandagatla
0 siblings, 0 replies; 9+ messages in thread
From: Srinivas Kandagatla @ 2026-05-21 17:19 UTC (permalink / raw)
To: Konrad Dybcio, Srinivas Kandagatla, andersson
Cc: srini, konradybcio, linux-sound, linux-arm-msm, linux-kernel
On 5/21/26 9:45 AM, Konrad Dybcio wrote:
> On 5/14/26 5:50 PM, Srinivas Kandagatla wrote:
>> apr_callback() currently validates all received packets against
>> APR_HDR_SIZE before queueing them for the RX worker. This is not correct
>> for GPR packets, which use a different header size.
>>
>> Validate the received packet length against the header size matching the
>> packet router type before copying the packet.
>>
>> Signed-off-by: Srinivas Kandagatla <srinivas.kandagatla@oss.qualcomm.com>
>> ---
>> drivers/soc/qcom/apr.c | 17 ++++++++++++++---
>> 1 file changed, 14 insertions(+), 3 deletions(-)
>>
>> diff --git a/drivers/soc/qcom/apr.c b/drivers/soc/qcom/apr.c
>> index 127204c195ea..68b357462438 100644
>> --- a/drivers/soc/qcom/apr.c
>> +++ b/drivers/soc/qcom/apr.c
>> @@ -165,9 +165,20 @@ static int apr_callback(struct rpmsg_device *rpdev, void *buf,
>> struct apr_rx_buf *abuf;
>> unsigned long flags;
>>
>> - if (len <= APR_HDR_SIZE) {
>> - dev_err(apr->dev, "APR: Improper apr pkt received:%p %d\n",
>> - buf, len);
>> + switch (apr->type) {
>> + case PR_TYPE_APR:
>> + if (len <= APR_HDR_SIZE) {
>> + dev_err(apr->dev, "APR: Improper apr pkt received:%p %d\n", buf, len);
>> + return -EINVAL;
>> + }
>> + break;
>> + case PR_TYPE_GPR:
>> + if (len <= GPR_HDR_SIZE) {
>> + dev_err(apr->dev, "APR: Improper gpr pkt received:%p %d\n", buf, len);
>
> It first says "APR:" and then "[ag]pr" later, please at least make the
> case consistent
Ah, seems like an overlook, thanks for spotting it, will fix this in
next spin.
>
>> + return -EINVAL;
>> + }
>> + break;
>
> Switch seems a little far-fetched, unless there'll be more packet types
never know... we have similar checks in the driver, atleast its
consistent with that.
--srini
>
> Konrad
^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH 3/3] soc: qcom: apr: Process RX messages using per-service work items
2026-05-14 15:50 ` [PATCH 3/3] soc: qcom: apr: Process RX messages using per-service work items Srinivas Kandagatla
@ 2026-05-21 22:28 ` Bjorn Andersson
2026-05-28 13:50 ` Srinivas Kandagatla
0 siblings, 1 reply; 9+ messages in thread
From: Bjorn Andersson @ 2026-05-21 22:28 UTC (permalink / raw)
To: Srinivas Kandagatla
Cc: srini, konradybcio, linux-sound, linux-arm-msm, linux-kernel
On Thu, May 14, 2026 at 03:50:51PM +0000, Srinivas Kandagatla wrote:
> The APR transport currently serializes all incoming packets through a
> single work item and a global RX queue. If one service callback blocks
> or takes a long time to complete, packet processing for all other
> services is delayed.
>
Can you please elaborate on why that's bad? Also, can you confirm that
there are no issues downstream of this with the packets now potentially
being handled out-of-order?
Regards,
Bjorn
> Move RX buffering and work items from the packet router to each
> individual service. Incoming packets are queued on a per-service list
> and processed by that service's work item, allowing unrelated services
> to make progress independently while preserving message ordering within
> each service.
>
> Since queued packets may outlive service removal, add a reference count
> to keep the service object alive until all queued packets have been
> processed, and reject new packets once the service begins shutting down.
>
> Switch the shared APR workqueue to an unbound reclaim workqueue so that
> multiple services can process packets in parallel.
>
> This also addresses the random CMD timeouts seen with audio commands
> that are sent to DSP which timeout on the response as they are waiting in
> the queue for other commands to finish.
>
> Signed-off-by: Srinivas Kandagatla <srinivas.kandagatla@oss.qualcomm.com>
> ---
> drivers/soc/qcom/apr.c | 265 +++++++++++++++++++++++------------
> include/linux/soc/qcom/apr.h | 6 +
> 2 files changed, 185 insertions(+), 86 deletions(-)
>
> diff --git a/drivers/soc/qcom/apr.c b/drivers/soc/qcom/apr.c
> index 68b357462438..f25219b48cdc 100644
> --- a/drivers/soc/qcom/apr.c
> +++ b/drivers/soc/qcom/apr.c
> @@ -28,14 +28,11 @@ struct packet_router {
> struct rpmsg_endpoint *ch;
> struct device *dev;
> spinlock_t svcs_lock;
> - spinlock_t rx_lock;
> struct idr svcs_idr;
> int dest_domain_id;
> int type;
> struct pdr_handle *pdr;
> struct workqueue_struct *rxwq;
> - struct work_struct rx_work;
> - struct list_head rx_list;
> };
>
> struct apr_rx_buf {
> @@ -74,54 +71,58 @@ int apr_send_pkt(struct apr_device *adev, struct apr_pkt *pkt)
> }
> EXPORT_SYMBOL_GPL(apr_send_pkt);
>
> -void gpr_free_port(gpr_port_t *port)
> +static void apr_svc_release(struct kref *ref)
> {
> - struct packet_router *gpr = port->pr;
> - unsigned long flags;
> + struct pkt_router_svc *svc;
>
> - spin_lock_irqsave(&gpr->svcs_lock, flags);
> - idr_remove(&gpr->svcs_idr, port->id);
> - spin_unlock_irqrestore(&gpr->svcs_lock, flags);
> + svc = container_of(ref, struct pkt_router_svc, refcount);
>
> - kfree(port);
> + if (svc->dynamic_svc)
> + kfree(svc);
> }
> -EXPORT_SYMBOL_GPL(gpr_free_port);
>
> -gpr_port_t *gpr_alloc_port(struct apr_device *gdev, struct device *dev,
> - gpr_port_cb cb, void *priv)
> +static void apr_svc_get(struct pkt_router_svc *svc)
> {
> - struct packet_router *pr = dev_get_drvdata(gdev->dev.parent);
> - gpr_port_t *port;
> - struct pkt_router_svc *svc;
> - int id;
> + kref_get(&svc->refcount);
> +}
>
> - port = kzalloc_obj(*port);
> - if (!port)
> - return ERR_PTR(-ENOMEM);
> +static void apr_svc_put(struct pkt_router_svc *svc)
> +{
> + kref_put(&svc->refcount, apr_svc_release);
> +}
>
> - svc = port;
> - svc->callback = cb;
> - svc->pr = pr;
> - svc->priv = priv;
> - svc->dev = dev;
> - spin_lock_init(&svc->lock);
> +static void apr_svc_purge_rx(struct pkt_router_svc *svc)
> +{
> + struct apr_rx_buf *abuf, *tmp;
> + unsigned long flags;
>
> - spin_lock(&pr->svcs_lock);
> - id = idr_alloc_cyclic(&pr->svcs_idr, svc, GPR_DYNAMIC_PORT_START,
> - GPR_DYNAMIC_PORT_END, GFP_ATOMIC);
> - if (id < 0) {
> - dev_err(dev, "Unable to allocate dynamic GPR src port\n");
> - kfree(port);
> - spin_unlock(&pr->svcs_lock);
> - return ERR_PTR(id);
> + spin_lock_irqsave(&svc->lock, flags);
> + list_for_each_entry_safe(abuf, tmp, &svc->rx_list, node) {
> + list_del(&abuf->node);
> + kfree(abuf);
> + apr_svc_put(svc);
> }
> + spin_unlock_irqrestore(&svc->lock, flags);
> +}
>
> - svc->id = id;
> - spin_unlock(&pr->svcs_lock);
> +void gpr_free_port(gpr_port_t *port)
> +{
> + struct packet_router *gpr = port->pr;
> + unsigned long flags;
>
> - return port;
> + spin_lock_irqsave(&port->lock, flags);
> + port->dying = true;
> + spin_unlock_irqrestore(&port->lock, flags);
> +
> + spin_lock_irqsave(&gpr->svcs_lock, flags);
> + idr_remove(&gpr->svcs_idr, port->id);
> + spin_unlock_irqrestore(&gpr->svcs_lock, flags);
> +
> + cancel_work_sync(&port->rx_work);
> + apr_svc_purge_rx(port);
> + apr_svc_put(port);
> }
> -EXPORT_SYMBOL_GPL(gpr_alloc_port);
> +EXPORT_SYMBOL_GPL(gpr_free_port);
>
> static int pkt_router_send_svc_pkt(struct pkt_router_svc *svc, const struct gpr_pkt *pkt)
> {
> @@ -155,15 +156,53 @@ static void apr_dev_release(struct device *dev)
> {
> struct apr_device *adev = to_apr_device(dev);
>
> + cancel_work_sync(&adev->svc.rx_work);
> + apr_svc_purge_rx(&adev->svc);
> + apr_svc_put(&adev->svc);
> kfree(adev);
> }
>
> +static struct pkt_router_svc *apr_find_svc(struct packet_router *apr, void *buf)
> +{
> + struct pkt_router_svc *svc;
> + unsigned long flags;
> + uint32_t svc_id;
> + struct apr_hdr *ahdr;
> + struct gpr_hdr *ghdr;
> +
> + switch (apr->type) {
> + case PR_TYPE_APR:
> + ahdr = buf;
> + svc_id = ahdr->dest_svc;
> + break;
> + case PR_TYPE_GPR:
> + ghdr = buf;
> + svc_id = ghdr->dest_port;
> + break;
> + default:
> + dev_err(apr->dev, "Invalid Packet Router\n");
> + return NULL;
> + }
> +
> + spin_lock_irqsave(&apr->svcs_lock, flags);
> + svc = idr_find(&apr->svcs_idr, svc_id);
> + if (svc)
> + apr_svc_get(svc);
> + spin_unlock_irqrestore(&apr->svcs_lock, flags);
> +
> + if (!svc)
> + dev_err(apr->dev, "APR: service is not registered (%d)\n", svc_id);
> +
> + return svc;
> +}
> +
> static int apr_callback(struct rpmsg_device *rpdev, void *buf,
> int len, void *priv, u32 addr)
> {
> struct packet_router *apr = dev_get_drvdata(&rpdev->dev);
> struct apr_rx_buf *abuf;
> unsigned long flags;
> + struct pkt_router_svc *svc;
>
> switch (apr->type) {
> case PR_TYPE_APR:
> @@ -189,24 +228,35 @@ static int apr_callback(struct rpmsg_device *rpdev, void *buf,
> abuf->len = len;
> memcpy(abuf->buf, buf, len);
>
> - spin_lock_irqsave(&apr->rx_lock, flags);
> - list_add_tail(&abuf->node, &apr->rx_list);
> - spin_unlock_irqrestore(&apr->rx_lock, flags);
> + svc = apr_find_svc(apr, buf);
> + if (!svc) {
> + kfree(abuf);
> + return 0;
> + }
> +
> + spin_lock_irqsave(&svc->lock, flags);
> + if (svc->dying) {
> + spin_unlock_irqrestore(&svc->lock, flags);
> + kfree(abuf);
> + apr_svc_put(svc);
> + return 0;
> + }
> +
> + list_add_tail(&abuf->node, &svc->rx_list);
> + spin_unlock_irqrestore(&svc->lock, flags);
>
> - queue_work(apr->rxwq, &apr->rx_work);
> + queue_work(apr->rxwq, &svc->rx_work);
>
> return 0;
> }
>
> -static int apr_do_rx_callback(struct packet_router *apr, struct apr_rx_buf *abuf)
> +static int apr_do_rx_callback(struct pkt_router_svc *svc, struct apr_rx_buf *abuf)
> {
> - uint16_t hdr_size, msg_type, ver, svc_id;
> - struct pkt_router_svc *svc;
> - struct apr_device *adev;
> + uint16_t hdr_size, msg_type, ver;
> + struct apr_device *adev = NULL;
> struct apr_driver *adrv = NULL;
> struct apr_resp_pkt resp;
> struct apr_hdr *hdr;
> - unsigned long flags;
> void *buf = abuf->buf;
> int len = abuf->len;
>
> @@ -217,18 +267,18 @@ static int apr_do_rx_callback(struct packet_router *apr, struct apr_rx_buf *abuf
>
> hdr_size = APR_HDR_FIELD_SIZE_BYTES(hdr->hdr_field);
> if (hdr_size < APR_HDR_SIZE) {
> - dev_err(apr->dev, "APR: Wrong hdr size:%d\n", hdr_size);
> + dev_err(svc->dev, "APR: Wrong hdr size:%d\n", hdr_size);
> return -EINVAL;
> }
>
> if (hdr->pkt_size < APR_HDR_SIZE || hdr->pkt_size != len) {
> - dev_err(apr->dev, "APR: Wrong packet size\n");
> + dev_err(svc->dev, "APR: Wrong packet size\n");
> return -EINVAL;
> }
>
> msg_type = APR_HDR_FIELD_MT(hdr->hdr_field);
> if (msg_type >= APR_MSG_TYPE_MAX) {
> - dev_err(apr->dev, "APR: Wrong message type: %d\n", msg_type);
> + dev_err(svc->dev, "APR: Wrong message type: %d\n", msg_type);
> return -EINVAL;
> }
>
> @@ -236,22 +286,17 @@ static int apr_do_rx_callback(struct packet_router *apr, struct apr_rx_buf *abuf
> hdr->dest_domain >= APR_DOMAIN_MAX ||
> hdr->src_svc >= APR_SVC_MAX ||
> hdr->dest_svc >= APR_SVC_MAX) {
> - dev_err(apr->dev, "APR: Wrong APR header\n");
> + dev_err(svc->dev, "APR: Wrong APR header\n");
> return -EINVAL;
> }
>
> - svc_id = hdr->dest_svc;
> - spin_lock_irqsave(&apr->svcs_lock, flags);
> - svc = idr_find(&apr->svcs_idr, svc_id);
> if (svc && svc->dev->driver) {
> adev = svc_to_apr_device(svc);
> adrv = to_apr_driver(adev->dev.driver);
> }
> - spin_unlock_irqrestore(&apr->svcs_lock, flags);
>
> if (!adrv || !adev) {
> - dev_err(apr->dev, "APR: service is not registered (%d)\n",
> - svc_id);
> + dev_err(svc->dev, "APR: service device not found\n");
> return -EINVAL;
> }
>
> @@ -270,13 +315,11 @@ static int apr_do_rx_callback(struct packet_router *apr, struct apr_rx_buf *abuf
> return 0;
> }
>
> -static int gpr_do_rx_callback(struct packet_router *gpr, struct apr_rx_buf *abuf)
> +static int gpr_do_rx_callback(struct pkt_router_svc *svc, struct apr_rx_buf *abuf)
> {
> uint16_t hdr_size, ver;
> - struct pkt_router_svc *svc = NULL;
> struct gpr_resp_pkt resp;
> struct gpr_hdr *hdr;
> - unsigned long flags;
> void *buf = abuf->buf;
> int len = abuf->len;
>
> @@ -287,12 +330,12 @@ static int gpr_do_rx_callback(struct packet_router *gpr, struct apr_rx_buf *abuf
>
> hdr_size = hdr->hdr_size;
> if (hdr_size < GPR_PKT_HEADER_WORD_SIZE) {
> - dev_err(gpr->dev, "GPR: Wrong hdr size:%d\n", hdr_size);
> + dev_err(svc->dev, "GPR: Wrong hdr size:%d\n", hdr_size);
> return -EINVAL;
> }
>
> if (hdr->pkt_size < GPR_PKT_HEADER_BYTE_SIZE || hdr->pkt_size != len) {
> - dev_err(gpr->dev, "GPR: Wrong packet size\n");
> + dev_err(svc->dev, "GPR: Wrong packet size\n");
> return -EINVAL;
> }
>
> @@ -306,49 +349,91 @@ static int gpr_do_rx_callback(struct packet_router *gpr, struct apr_rx_buf *abuf
> if (resp.payload_size > 0)
> resp.payload = buf + (hdr_size * 4);
>
> -
> - spin_lock_irqsave(&gpr->svcs_lock, flags);
> - svc = idr_find(&gpr->svcs_idr, hdr->dest_port);
> - spin_unlock_irqrestore(&gpr->svcs_lock, flags);
> -
> - if (!svc) {
> - dev_err(gpr->dev, "GPR: Port(%x) is not registered\n",
> - hdr->dest_port);
> - return -EINVAL;
> - }
> -
> if (svc->callback)
> svc->callback(&resp, svc->priv, 0);
>
> return 0;
> }
>
> -static void apr_rxwq(struct work_struct *work)
> +static void apr_service_rxwq(struct work_struct *work)
> {
> - struct packet_router *apr = container_of(work, struct packet_router, rx_work);
> + struct pkt_router_svc *svc = container_of(work, struct pkt_router_svc, rx_work);
> + struct packet_router *apr = svc->pr;
> struct apr_rx_buf *abuf, *b;
> unsigned long flags;
>
> - if (!list_empty(&apr->rx_list)) {
> - list_for_each_entry_safe(abuf, b, &apr->rx_list, node) {
> + for (;;) {
> + LIST_HEAD(local);
> +
> + spin_lock_irqsave(&svc->lock, flags);
> + if (list_empty(&svc->rx_list)) {
> + spin_unlock_irqrestore(&svc->lock, flags);
> + break;
> + }
> +
> + list_splice_init(&svc->rx_list, &local);
> + spin_unlock_irqrestore(&svc->lock, flags);
> +
> + list_for_each_entry_safe(abuf, b, &local, node) {
> switch (apr->type) {
> case PR_TYPE_APR:
> - apr_do_rx_callback(apr, abuf);
> + apr_do_rx_callback(svc, abuf);
> break;
> case PR_TYPE_GPR:
> - gpr_do_rx_callback(apr, abuf);
> + gpr_do_rx_callback(svc, abuf);
> break;
> default:
> break;
> }
> - spin_lock_irqsave(&apr->rx_lock, flags);
> + apr_svc_put(svc);
> list_del(&abuf->node);
> - spin_unlock_irqrestore(&apr->rx_lock, flags);
> kfree(abuf);
> }
> }
> }
>
> +gpr_port_t *gpr_alloc_port(struct apr_device *gdev, struct device *dev,
> + gpr_port_cb cb, void *priv)
> +{
> + struct packet_router *pr = dev_get_drvdata(gdev->dev.parent);
> + gpr_port_t *port;
> + struct pkt_router_svc *svc;
> + int id;
> +
> + port = kzalloc_obj(*port);
> + if (!port)
> + return ERR_PTR(-ENOMEM);
> +
> + svc = port;
> + svc->callback = cb;
> + svc->pr = pr;
> + svc->priv = priv;
> + svc->dying = false;
> + svc->dynamic_svc = true;
> + svc->dev = dev;
> + spin_lock_init(&svc->lock);
> +
> + INIT_WORK(&svc->rx_work, apr_service_rxwq);
> + INIT_LIST_HEAD(&svc->rx_list);
> + kref_init(&svc->refcount);
> +
> + spin_lock(&pr->svcs_lock);
> + id = idr_alloc_cyclic(&pr->svcs_idr, svc, GPR_DYNAMIC_PORT_START,
> + GPR_DYNAMIC_PORT_END, GFP_ATOMIC);
> + if (id < 0) {
> + dev_err(dev, "Unable to allocate dynamic GPR src port\n");
> + kfree(port);
> + spin_unlock(&pr->svcs_lock);
> + return ERR_PTR(id);
> + }
> +
> + svc->id = id;
> + spin_unlock(&pr->svcs_lock);
> +
> + return port;
> +}
> +EXPORT_SYMBOL_GPL(gpr_alloc_port);
> +
> static int apr_device_match(struct device *dev, const struct device_driver *drv)
> {
> struct apr_device *adev = to_apr_device(dev);
> @@ -390,6 +475,11 @@ static void apr_device_remove(struct device *dev)
> struct apr_device *adev = to_apr_device(dev);
> struct apr_driver *adrv = to_apr_driver(dev->driver);
> struct packet_router *apr = dev_get_drvdata(adev->dev.parent);
> + unsigned long flags;
> +
> + spin_lock_irqsave(&adev->svc.lock, flags);
> + adev->svc.dying = true;
> + spin_unlock_irqrestore(&adev->svc.lock, flags);
>
> if (adrv->remove)
> adrv->remove(adev);
> @@ -437,9 +527,15 @@ static int apr_add_device(struct device *dev, struct device_node *np,
> svc->id = svc_id;
> svc->pr = apr;
> svc->priv = adev;
> + svc->dying = false;
> svc->dev = dev;
> + svc->dynamic_svc = false;
> spin_lock_init(&svc->lock);
>
> + INIT_WORK(&svc->rx_work, apr_service_rxwq);
> + INIT_LIST_HEAD(&svc->rx_list);
> + kref_init(&svc->refcount);
> +
> adev->domain_id = domain_id;
>
> if (np)
> @@ -488,7 +584,6 @@ static int apr_add_device(struct device *dev, struct device_node *np,
> dev_err(dev, "device_register failed: %d\n", ret);
> put_device(&adev->dev);
> }
> -
> out:
> return ret;
> }
> @@ -629,12 +724,12 @@ static int apr_probe(struct rpmsg_device *rpdev)
> dev_set_drvdata(dev, apr);
> apr->ch = rpdev->ept;
> apr->dev = dev;
> - apr->rxwq = create_singlethread_workqueue("qcom_apr_rx");
> +
> + apr->rxwq = alloc_workqueue("qcom_apr_rx", WQ_UNBOUND | WQ_MEM_RECLAIM, 0);
> if (!apr->rxwq) {
> dev_err(apr->dev, "Failed to start Rx WQ\n");
> return -ENOMEM;
> }
> - INIT_WORK(&apr->rx_work, apr_rxwq);
>
> apr->pdr = pdr_handle_alloc(apr_pd_status, apr);
> if (IS_ERR(apr->pdr)) {
> @@ -643,8 +738,6 @@ static int apr_probe(struct rpmsg_device *rpdev)
> goto destroy_wq;
> }
>
> - INIT_LIST_HEAD(&apr->rx_list);
> - spin_lock_init(&apr->rx_lock);
> spin_lock_init(&apr->svcs_lock);
> idr_init(&apr->svcs_idr);
>
> diff --git a/include/linux/soc/qcom/apr.h b/include/linux/soc/qcom/apr.h
> index 58fa1df96347..f5bc55c3d025 100644
> --- a/include/linux/soc/qcom/apr.h
> +++ b/include/linux/soc/qcom/apr.h
> @@ -3,6 +3,7 @@
> #ifndef __QCOM_APR_H_
> #define __QCOM_APR_H_
>
> +#include <linux/kref.h>
> #include <linux/spinlock.h>
> #include <linux/device.h>
> #include <linux/mod_devicetable.h>
> @@ -129,6 +130,11 @@ struct pkt_router_svc {
> gpr_port_cb callback;
> struct packet_router *pr;
> spinlock_t lock;
> + struct work_struct rx_work;
> + struct list_head rx_list;
> + struct kref refcount;
> + bool dying;
> + bool dynamic_svc;
> int id;
> void *priv;
> };
> --
> 2.47.3
>
^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH 0/3] soc: qcom: apr: improve RX processing
2026-05-14 15:50 [PATCH 0/3] soc: qcom: apr: improve RX processing Srinivas Kandagatla
` (2 preceding siblings ...)
2026-05-14 15:50 ` [PATCH 3/3] soc: qcom: apr: Process RX messages using per-service work items Srinivas Kandagatla
@ 2026-05-26 22:26 ` Val Packett
3 siblings, 0 replies; 9+ messages in thread
From: Val Packett @ 2026-05-26 22:26 UTC (permalink / raw)
To: Srinivas Kandagatla, andersson
Cc: srini, konradybcio, linux-sound, linux-arm-msm, linux-kernel
Hi,
On 5/14/26 12:50 PM, Srinivas Kandagatla wrote:
> Hi all,
> [..]
> Srinivas Kandagatla (3):
> soc: qcom: apr: Remove service from IDR before registration failure
> soc: qcom: apr: Check response packet length by router type
> soc: qcom: apr: Process RX messages using per-service work items
>
> drivers/soc/qcom/apr.c | 283 ++++++++++++++++++++++++-----------
> include/linux/soc/qcom/apr.h | 6 +
> 2 files changed, 200 insertions(+), 89 deletions(-)
just saw a crash under apr_service_rxwq -> graph_callback with this
series applied. Can't say it really is to blame, but still replying here
because it was applied..
hamoa, quad WSA speakers, pipewire, audio playing from
https://flathub.org/en/apps/com.rafaelmardojai.Blanket, crashed when
opening pavucontrol (display froze with the pavucontrol UI already loaded)
<1>[577090.453413] Unable to handle kernel NULL pointer dereference at
virtual address 0000000000000050
[..]
<4>[577090.453924] CPU: 0 UID: 0 PID: 946838 Comm: kworker/u49:1
Tainted: G W 7.1.0-rc4-next-20260519-uwu+ #116 PREEMPT(full)
<4>[577090.453933] Tainted: [W]=WARN
<4>[577090.453936] Hardware name: Dell Inc. Latitude 7455/0FK7MX, BIOS
2.13.0 09/19/2025
<4>[577090.453941] Workqueue: qcom_apr_rx apr_service_rxwq [apr]
(../drivers/soc/qcom/apr.c:359)
<4>[577090.453957] pstate: 61400009 (nZCv daif +PAN -UAO -TCO +DIT -SSBS
BTYPE=--)
<4>[577090.453963] pc : graph_callback+0x1a8/0x280 [snd_q6apm]
(../sound/soc/qcom/qdsp6/q6apm.c:577)
<4>[577090.453977] lr : graph_callback+0x194/0x280 [snd_q6apm]
(../sound/soc/qcom/qdsp6/q6apm.c:572)
[..]
<4>[577090.454086] Call trace:
<4>[577090.454090] graph_callback+0x1a8/0x280 [snd_q6apm]
(../sound/soc/qcom/qdsp6/q6apm.c:577) (P)
<4>[577090.454100] apr_service_rxwq+0x274/0x3b8 [apr]
(../drivers/soc/qcom/apr.c:535)
<4>[577090.454109] process_scheduled_works+0x190/0x500
(../kernel/workqueue.c:3323)
<4>[577090.454124] worker_thread+0x230/0x330 (../include/linux/list.h:404)
<4>[577090.454133] kthread+0x120/0x158 (../kernel/kthread.c:438)
<4>[577090.454141] ret_from_fork+0x10/0x20
(../arch/arm64/kernel/entry.S:842)
<0>[577090.454153] Code: f94026e9 910242e0 f9400e95 d37ced08 (f8686936)
<4>[577090.454158] ---[ end trace 0000000000000000 ]---
Line 577 is atomic_set(&graph->tx_data.hw_ptr, hdr->token + 1);
weird, both graph and hdr have been dereferenced before this line..
Thanks,
~val
^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH 3/3] soc: qcom: apr: Process RX messages using per-service work items
2026-05-21 22:28 ` Bjorn Andersson
@ 2026-05-28 13:50 ` Srinivas Kandagatla
0 siblings, 0 replies; 9+ messages in thread
From: Srinivas Kandagatla @ 2026-05-28 13:50 UTC (permalink / raw)
To: Bjorn Andersson, Srinivas Kandagatla
Cc: srini, konradybcio, linux-sound, linux-arm-msm, linux-kernel
On 5/21/26 11:28 PM, Bjorn Andersson wrote:
> On Thu, May 14, 2026 at 03:50:51PM +0000, Srinivas Kandagatla wrote:
>> The APR transport currently serializes all incoming packets through a
>> single work item and a global RX queue. If one service callback blocks
>> or takes a long time to complete, packet processing for all other
>> services is delayed.
>>
>
> Can you please elaborate on why that's bad? Also, can you confirm that
We started to notice that some of the commands are timing out mainly
commands that are sent while streaming is in progress, example, graph
stop/start. The underlying reason for this timeout is that there are
read/write acks that are in the queue.
And other potential issue could be when we have both playback and
capture or multiple streaming in progress, we do not want to delay dsp
events to particular stream because other stream events are in queue.
Events for each stream or service are independent of each other, I will
be really surprised if there is a inter dependency of this.
> there are no issues downstream of this with the packets now potentially
> being handled out-of-order?
When you mean downstream, you mean adsp or downstream kernel?
>
> Regards,
> Bjorn
>
>> Move RX buffering and work items from the packet router to each
>> individual service. Incoming packets are queued on a per-service list
>> and processed by that service's work item, allowing unrelated services
>> to make progress independently while preserving message ordering within
>> each service.
>>
>> Since queued packets may outlive service removal, add a reference count
>> to keep the service object alive until all queued packets have been
>> processed, and reject new packets once the service begins shutting down.
>>
>> Switch the shared APR workqueue to an unbound reclaim workqueue so that
>> multiple services can process packets in parallel.
>>
>> This also addresses the random CMD timeouts seen with audio commands
>> that are sent to DSP which timeout on the response as they are waiting in
>> the queue for other commands to finish.
>>
>> Signed-off-by: Srinivas Kandagatla <srinivas.kandagatla@oss.qualcomm.com>
>> ---
>> drivers/soc/qcom/apr.c | 265 +++++++++++++++++++++++------------
>> include/linux/soc/qcom/apr.h | 6 +
>> 2 files changed, 185 insertions(+), 86 deletions(-)
>>
>> diff --git a/drivers/soc/qcom/apr.c b/drivers/soc/qcom/apr.c
>> index 68b357462438..f25219b48cdc 100644
>> --- a/drivers/soc/qcom/apr.c
>> +++ b/drivers/soc/qcom/apr.c
>> @@ -28,14 +28,11 @@ struct packet_router {
>> struct rpmsg_endpoint *ch;
>> struct device *dev;
>> spinlock_t svcs_lock;
>> - spinlock_t rx_lock;
>> struct idr svcs_idr;
>> int dest_domain_id;
>> int type;
>> struct pdr_handle *pdr;
>> struct workqueue_struct *rxwq;
>> - struct work_struct rx_work;
>> - struct list_head rx_list;
>> };
>>
>> struct apr_rx_buf {
>> @@ -74,54 +71,58 @@ int apr_send_pkt(struct apr_device *adev, struct apr_pkt *pkt)
>> }
>> EXPORT_SYMBOL_GPL(apr_send_pkt);
>>
>> -void gpr_free_port(gpr_port_t *port)
>> +static void apr_svc_release(struct kref *ref)
>> {
>> - struct packet_router *gpr = port->pr;
>> - unsigned long flags;
>> + struct pkt_router_svc *svc;
>>
>> - spin_lock_irqsave(&gpr->svcs_lock, flags);
>> - idr_remove(&gpr->svcs_idr, port->id);
>> - spin_unlock_irqrestore(&gpr->svcs_lock, flags);
>> + svc = container_of(ref, struct pkt_router_svc, refcount);
>>
>> - kfree(port);
>> + if (svc->dynamic_svc)
>> + kfree(svc);
>> }
>> -EXPORT_SYMBOL_GPL(gpr_free_port);
>>
>> -gpr_port_t *gpr_alloc_port(struct apr_device *gdev, struct device *dev,
>> - gpr_port_cb cb, void *priv)
>> +static void apr_svc_get(struct pkt_router_svc *svc)
>> {
>> - struct packet_router *pr = dev_get_drvdata(gdev->dev.parent);
>> - gpr_port_t *port;
>> - struct pkt_router_svc *svc;
>> - int id;
>> + kref_get(&svc->refcount);
>> +}
>>
>> - port = kzalloc_obj(*port);
>> - if (!port)
>> - return ERR_PTR(-ENOMEM);
>> +static void apr_svc_put(struct pkt_router_svc *svc)
>> +{
>> + kref_put(&svc->refcount, apr_svc_release);
>> +}
>>
>> - svc = port;
>> - svc->callback = cb;
>> - svc->pr = pr;
>> - svc->priv = priv;
>> - svc->dev = dev;
>> - spin_lock_init(&svc->lock);
>> +static void apr_svc_purge_rx(struct pkt_router_svc *svc)
>> +{
>> + struct apr_rx_buf *abuf, *tmp;
>> + unsigned long flags;
>>
>> - spin_lock(&pr->svcs_lock);
>> - id = idr_alloc_cyclic(&pr->svcs_idr, svc, GPR_DYNAMIC_PORT_START,
>> - GPR_DYNAMIC_PORT_END, GFP_ATOMIC);
>> - if (id < 0) {
>> - dev_err(dev, "Unable to allocate dynamic GPR src port\n");
>> - kfree(port);
>> - spin_unlock(&pr->svcs_lock);
>> - return ERR_PTR(id);
>> + spin_lock_irqsave(&svc->lock, flags);
>> + list_for_each_entry_safe(abuf, tmp, &svc->rx_list, node) {
>> + list_del(&abuf->node);
>> + kfree(abuf);
>> + apr_svc_put(svc);
>> }
>> + spin_unlock_irqrestore(&svc->lock, flags);
>> +}
>>
>> - svc->id = id;
>> - spin_unlock(&pr->svcs_lock);
>> +void gpr_free_port(gpr_port_t *port)
>> +{
>> + struct packet_router *gpr = port->pr;
>> + unsigned long flags;
>>
>> - return port;
>> + spin_lock_irqsave(&port->lock, flags);
>> + port->dying = true;
>> + spin_unlock_irqrestore(&port->lock, flags);
>> +
>> + spin_lock_irqsave(&gpr->svcs_lock, flags);
>> + idr_remove(&gpr->svcs_idr, port->id);
>> + spin_unlock_irqrestore(&gpr->svcs_lock, flags);
>> +
>> + cancel_work_sync(&port->rx_work);
>> + apr_svc_purge_rx(port);
>> + apr_svc_put(port);
>> }
>> -EXPORT_SYMBOL_GPL(gpr_alloc_port);
>> +EXPORT_SYMBOL_GPL(gpr_free_port);
>>
>> static int pkt_router_send_svc_pkt(struct pkt_router_svc *svc, const struct gpr_pkt *pkt)
>> {
>> @@ -155,15 +156,53 @@ static void apr_dev_release(struct device *dev)
>> {
>> struct apr_device *adev = to_apr_device(dev);
>>
>> + cancel_work_sync(&adev->svc.rx_work);
>> + apr_svc_purge_rx(&adev->svc);
>> + apr_svc_put(&adev->svc);
>> kfree(adev);
>> }
>>
>> +static struct pkt_router_svc *apr_find_svc(struct packet_router *apr, void *buf)
>> +{
>> + struct pkt_router_svc *svc;
>> + unsigned long flags;
>> + uint32_t svc_id;
>> + struct apr_hdr *ahdr;
>> + struct gpr_hdr *ghdr;
>> +
>> + switch (apr->type) {
>> + case PR_TYPE_APR:
>> + ahdr = buf;
>> + svc_id = ahdr->dest_svc;
>> + break;
>> + case PR_TYPE_GPR:
>> + ghdr = buf;
>> + svc_id = ghdr->dest_port;
>> + break;
>> + default:
>> + dev_err(apr->dev, "Invalid Packet Router\n");
>> + return NULL;
>> + }
>> +
>> + spin_lock_irqsave(&apr->svcs_lock, flags);
>> + svc = idr_find(&apr->svcs_idr, svc_id);
>> + if (svc)
>> + apr_svc_get(svc);
>> + spin_unlock_irqrestore(&apr->svcs_lock, flags);
>> +
>> + if (!svc)
>> + dev_err(apr->dev, "APR: service is not registered (%d)\n", svc_id);
>> +
>> + return svc;
>> +}
>> +
>> static int apr_callback(struct rpmsg_device *rpdev, void *buf,
>> int len, void *priv, u32 addr)
>> {
>> struct packet_router *apr = dev_get_drvdata(&rpdev->dev);
>> struct apr_rx_buf *abuf;
>> unsigned long flags;
>> + struct pkt_router_svc *svc;
>>
>> switch (apr->type) {
>> case PR_TYPE_APR:
>> @@ -189,24 +228,35 @@ static int apr_callback(struct rpmsg_device *rpdev, void *buf,
>> abuf->len = len;
>> memcpy(abuf->buf, buf, len);
>>
>> - spin_lock_irqsave(&apr->rx_lock, flags);
>> - list_add_tail(&abuf->node, &apr->rx_list);
>> - spin_unlock_irqrestore(&apr->rx_lock, flags);
>> + svc = apr_find_svc(apr, buf);
>> + if (!svc) {
>> + kfree(abuf);
>> + return 0;
>> + }
>> +
>> + spin_lock_irqsave(&svc->lock, flags);
>> + if (svc->dying) {
>> + spin_unlock_irqrestore(&svc->lock, flags);
>> + kfree(abuf);
>> + apr_svc_put(svc);
>> + return 0;
>> + }
>> +
>> + list_add_tail(&abuf->node, &svc->rx_list);
>> + spin_unlock_irqrestore(&svc->lock, flags);
>>
>> - queue_work(apr->rxwq, &apr->rx_work);
>> + queue_work(apr->rxwq, &svc->rx_work);
>>
>> return 0;
>> }
>>
>> -static int apr_do_rx_callback(struct packet_router *apr, struct apr_rx_buf *abuf)
>> +static int apr_do_rx_callback(struct pkt_router_svc *svc, struct apr_rx_buf *abuf)
>> {
>> - uint16_t hdr_size, msg_type, ver, svc_id;
>> - struct pkt_router_svc *svc;
>> - struct apr_device *adev;
>> + uint16_t hdr_size, msg_type, ver;
>> + struct apr_device *adev = NULL;
>> struct apr_driver *adrv = NULL;
>> struct apr_resp_pkt resp;
>> struct apr_hdr *hdr;
>> - unsigned long flags;
>> void *buf = abuf->buf;
>> int len = abuf->len;
>>
>> @@ -217,18 +267,18 @@ static int apr_do_rx_callback(struct packet_router *apr, struct apr_rx_buf *abuf
>>
>> hdr_size = APR_HDR_FIELD_SIZE_BYTES(hdr->hdr_field);
>> if (hdr_size < APR_HDR_SIZE) {
>> - dev_err(apr->dev, "APR: Wrong hdr size:%d\n", hdr_size);
>> + dev_err(svc->dev, "APR: Wrong hdr size:%d\n", hdr_size);
>> return -EINVAL;
>> }
>>
>> if (hdr->pkt_size < APR_HDR_SIZE || hdr->pkt_size != len) {
>> - dev_err(apr->dev, "APR: Wrong packet size\n");
>> + dev_err(svc->dev, "APR: Wrong packet size\n");
>> return -EINVAL;
>> }
>>
>> msg_type = APR_HDR_FIELD_MT(hdr->hdr_field);
>> if (msg_type >= APR_MSG_TYPE_MAX) {
>> - dev_err(apr->dev, "APR: Wrong message type: %d\n", msg_type);
>> + dev_err(svc->dev, "APR: Wrong message type: %d\n", msg_type);
>> return -EINVAL;
>> }
>>
>> @@ -236,22 +286,17 @@ static int apr_do_rx_callback(struct packet_router *apr, struct apr_rx_buf *abuf
>> hdr->dest_domain >= APR_DOMAIN_MAX ||
>> hdr->src_svc >= APR_SVC_MAX ||
>> hdr->dest_svc >= APR_SVC_MAX) {
>> - dev_err(apr->dev, "APR: Wrong APR header\n");
>> + dev_err(svc->dev, "APR: Wrong APR header\n");
>> return -EINVAL;
>> }
>>
>> - svc_id = hdr->dest_svc;
>> - spin_lock_irqsave(&apr->svcs_lock, flags);
>> - svc = idr_find(&apr->svcs_idr, svc_id);
>> if (svc && svc->dev->driver) {
>> adev = svc_to_apr_device(svc);
>> adrv = to_apr_driver(adev->dev.driver);
>> }
>> - spin_unlock_irqrestore(&apr->svcs_lock, flags);
>>
>> if (!adrv || !adev) {
>> - dev_err(apr->dev, "APR: service is not registered (%d)\n",
>> - svc_id);
>> + dev_err(svc->dev, "APR: service device not found\n");
>> return -EINVAL;
>> }
>>
>> @@ -270,13 +315,11 @@ static int apr_do_rx_callback(struct packet_router *apr, struct apr_rx_buf *abuf
>> return 0;
>> }
>>
>> -static int gpr_do_rx_callback(struct packet_router *gpr, struct apr_rx_buf *abuf)
>> +static int gpr_do_rx_callback(struct pkt_router_svc *svc, struct apr_rx_buf *abuf)
>> {
>> uint16_t hdr_size, ver;
>> - struct pkt_router_svc *svc = NULL;
>> struct gpr_resp_pkt resp;
>> struct gpr_hdr *hdr;
>> - unsigned long flags;
>> void *buf = abuf->buf;
>> int len = abuf->len;
>>
>> @@ -287,12 +330,12 @@ static int gpr_do_rx_callback(struct packet_router *gpr, struct apr_rx_buf *abuf
>>
>> hdr_size = hdr->hdr_size;
>> if (hdr_size < GPR_PKT_HEADER_WORD_SIZE) {
>> - dev_err(gpr->dev, "GPR: Wrong hdr size:%d\n", hdr_size);
>> + dev_err(svc->dev, "GPR: Wrong hdr size:%d\n", hdr_size);
>> return -EINVAL;
>> }
>>
>> if (hdr->pkt_size < GPR_PKT_HEADER_BYTE_SIZE || hdr->pkt_size != len) {
>> - dev_err(gpr->dev, "GPR: Wrong packet size\n");
>> + dev_err(svc->dev, "GPR: Wrong packet size\n");
>> return -EINVAL;
>> }
>>
>> @@ -306,49 +349,91 @@ static int gpr_do_rx_callback(struct packet_router *gpr, struct apr_rx_buf *abuf
>> if (resp.payload_size > 0)
>> resp.payload = buf + (hdr_size * 4);
>>
>> -
>> - spin_lock_irqsave(&gpr->svcs_lock, flags);
>> - svc = idr_find(&gpr->svcs_idr, hdr->dest_port);
>> - spin_unlock_irqrestore(&gpr->svcs_lock, flags);
>> -
>> - if (!svc) {
>> - dev_err(gpr->dev, "GPR: Port(%x) is not registered\n",
>> - hdr->dest_port);
>> - return -EINVAL;
>> - }
>> -
>> if (svc->callback)
>> svc->callback(&resp, svc->priv, 0);
>>
>> return 0;
>> }
>>
>> -static void apr_rxwq(struct work_struct *work)
>> +static void apr_service_rxwq(struct work_struct *work)
>> {
>> - struct packet_router *apr = container_of(work, struct packet_router, rx_work);
>> + struct pkt_router_svc *svc = container_of(work, struct pkt_router_svc, rx_work);
>> + struct packet_router *apr = svc->pr;
>> struct apr_rx_buf *abuf, *b;
>> unsigned long flags;
>>
>> - if (!list_empty(&apr->rx_list)) {
>> - list_for_each_entry_safe(abuf, b, &apr->rx_list, node) {
>> + for (;;) {
>> + LIST_HEAD(local);
>> +
>> + spin_lock_irqsave(&svc->lock, flags);
>> + if (list_empty(&svc->rx_list)) {
>> + spin_unlock_irqrestore(&svc->lock, flags);
>> + break;
>> + }
>> +
>> + list_splice_init(&svc->rx_list, &local);
>> + spin_unlock_irqrestore(&svc->lock, flags);
>> +
>> + list_for_each_entry_safe(abuf, b, &local, node) {
>> switch (apr->type) {
>> case PR_TYPE_APR:
>> - apr_do_rx_callback(apr, abuf);
>> + apr_do_rx_callback(svc, abuf);
>> break;
>> case PR_TYPE_GPR:
>> - gpr_do_rx_callback(apr, abuf);
>> + gpr_do_rx_callback(svc, abuf);
>> break;
>> default:
>> break;
>> }
>> - spin_lock_irqsave(&apr->rx_lock, flags);
>> + apr_svc_put(svc);
>> list_del(&abuf->node);
>> - spin_unlock_irqrestore(&apr->rx_lock, flags);
>> kfree(abuf);
>> }
>> }
>> }
>>
>> +gpr_port_t *gpr_alloc_port(struct apr_device *gdev, struct device *dev,
>> + gpr_port_cb cb, void *priv)
>> +{
>> + struct packet_router *pr = dev_get_drvdata(gdev->dev.parent);
>> + gpr_port_t *port;
>> + struct pkt_router_svc *svc;
>> + int id;
>> +
>> + port = kzalloc_obj(*port);
>> + if (!port)
>> + return ERR_PTR(-ENOMEM);
>> +
>> + svc = port;
>> + svc->callback = cb;
>> + svc->pr = pr;
>> + svc->priv = priv;
>> + svc->dying = false;
>> + svc->dynamic_svc = true;
>> + svc->dev = dev;
>> + spin_lock_init(&svc->lock);
>> +
>> + INIT_WORK(&svc->rx_work, apr_service_rxwq);
>> + INIT_LIST_HEAD(&svc->rx_list);
>> + kref_init(&svc->refcount);
>> +
>> + spin_lock(&pr->svcs_lock);
>> + id = idr_alloc_cyclic(&pr->svcs_idr, svc, GPR_DYNAMIC_PORT_START,
>> + GPR_DYNAMIC_PORT_END, GFP_ATOMIC);
>> + if (id < 0) {
>> + dev_err(dev, "Unable to allocate dynamic GPR src port\n");
>> + kfree(port);
>> + spin_unlock(&pr->svcs_lock);
>> + return ERR_PTR(id);
>> + }
>> +
>> + svc->id = id;
>> + spin_unlock(&pr->svcs_lock);
>> +
>> + return port;
>> +}
>> +EXPORT_SYMBOL_GPL(gpr_alloc_port);
>> +
>> static int apr_device_match(struct device *dev, const struct device_driver *drv)
>> {
>> struct apr_device *adev = to_apr_device(dev);
>> @@ -390,6 +475,11 @@ static void apr_device_remove(struct device *dev)
>> struct apr_device *adev = to_apr_device(dev);
>> struct apr_driver *adrv = to_apr_driver(dev->driver);
>> struct packet_router *apr = dev_get_drvdata(adev->dev.parent);
>> + unsigned long flags;
>> +
>> + spin_lock_irqsave(&adev->svc.lock, flags);
>> + adev->svc.dying = true;
>> + spin_unlock_irqrestore(&adev->svc.lock, flags);
>>
>> if (adrv->remove)
>> adrv->remove(adev);
>> @@ -437,9 +527,15 @@ static int apr_add_device(struct device *dev, struct device_node *np,
>> svc->id = svc_id;
>> svc->pr = apr;
>> svc->priv = adev;
>> + svc->dying = false;
>> svc->dev = dev;
>> + svc->dynamic_svc = false;
>> spin_lock_init(&svc->lock);
>>
>> + INIT_WORK(&svc->rx_work, apr_service_rxwq);
>> + INIT_LIST_HEAD(&svc->rx_list);
>> + kref_init(&svc->refcount);
>> +
>> adev->domain_id = domain_id;
>>
>> if (np)
>> @@ -488,7 +584,6 @@ static int apr_add_device(struct device *dev, struct device_node *np,
>> dev_err(dev, "device_register failed: %d\n", ret);
>> put_device(&adev->dev);
>> }
>> -
>> out:
>> return ret;
>> }
>> @@ -629,12 +724,12 @@ static int apr_probe(struct rpmsg_device *rpdev)
>> dev_set_drvdata(dev, apr);
>> apr->ch = rpdev->ept;
>> apr->dev = dev;
>> - apr->rxwq = create_singlethread_workqueue("qcom_apr_rx");
>> +
>> + apr->rxwq = alloc_workqueue("qcom_apr_rx", WQ_UNBOUND | WQ_MEM_RECLAIM, 0);
>> if (!apr->rxwq) {
>> dev_err(apr->dev, "Failed to start Rx WQ\n");
>> return -ENOMEM;
>> }
>> - INIT_WORK(&apr->rx_work, apr_rxwq);
>>
>> apr->pdr = pdr_handle_alloc(apr_pd_status, apr);
>> if (IS_ERR(apr->pdr)) {
>> @@ -643,8 +738,6 @@ static int apr_probe(struct rpmsg_device *rpdev)
>> goto destroy_wq;
>> }
>>
>> - INIT_LIST_HEAD(&apr->rx_list);
>> - spin_lock_init(&apr->rx_lock);
>> spin_lock_init(&apr->svcs_lock);
>> idr_init(&apr->svcs_idr);
>>
>> diff --git a/include/linux/soc/qcom/apr.h b/include/linux/soc/qcom/apr.h
>> index 58fa1df96347..f5bc55c3d025 100644
>> --- a/include/linux/soc/qcom/apr.h
>> +++ b/include/linux/soc/qcom/apr.h
>> @@ -3,6 +3,7 @@
>> #ifndef __QCOM_APR_H_
>> #define __QCOM_APR_H_
>>
>> +#include <linux/kref.h>
>> #include <linux/spinlock.h>
>> #include <linux/device.h>
>> #include <linux/mod_devicetable.h>
>> @@ -129,6 +130,11 @@ struct pkt_router_svc {
>> gpr_port_cb callback;
>> struct packet_router *pr;
>> spinlock_t lock;
>> + struct work_struct rx_work;
>> + struct list_head rx_list;
>> + struct kref refcount;
>> + bool dying;
>> + bool dynamic_svc;
>> int id;
>> void *priv;
>> };
>> --
>> 2.47.3
>>
^ permalink raw reply [flat|nested] 9+ messages in thread
end of thread, other threads:[~2026-05-28 13:50 UTC | newest]
Thread overview: 9+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2026-05-14 15:50 [PATCH 0/3] soc: qcom: apr: improve RX processing Srinivas Kandagatla
2026-05-14 15:50 ` [PATCH 1/3] soc: qcom: apr: Remove service from IDR before registration failure Srinivas Kandagatla
2026-05-14 15:50 ` [PATCH 2/3] soc: qcom: apr: Check response packet length by router type Srinivas Kandagatla
2026-05-21 9:45 ` Konrad Dybcio
2026-05-21 17:19 ` Srinivas Kandagatla
2026-05-14 15:50 ` [PATCH 3/3] soc: qcom: apr: Process RX messages using per-service work items Srinivas Kandagatla
2026-05-21 22:28 ` Bjorn Andersson
2026-05-28 13:50 ` Srinivas Kandagatla
2026-05-26 22:26 ` [PATCH 0/3] soc: qcom: apr: improve RX processing Val Packett
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox