[PATCH 0/3] soc: qcom: apr: improve RX processing

[PATCH 0/3] soc: qcom: apr: improve RX processing

[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

[PATCH 1/3] soc: qcom: apr: Remove service from IDR before registration failure

[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

[PATCH 2/3] soc: qcom: apr: Check response packet length by router type

[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

Re: [PATCH 2/3] soc: qcom: apr: Check response packet length by router type

[Konrad Dybcio]

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

Re: [PATCH 2/3] soc: qcom: apr: Check response packet length by router type

[Srinivas Kandagatla]

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

[PATCH 3/3] soc: qcom: apr: Process RX messages using per-service work items

[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

Re: [PATCH 3/3] soc: qcom: apr: Process RX messages using per-service work items

[Bjorn Andersson]

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
> 

Re: [PATCH 3/3] soc: qcom: apr: Process RX messages using per-service work items

[Srinivas Kandagatla]

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
>>

Re: [PATCH 0/3] soc: qcom: apr: improve RX processing

[Val Packett]

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