[PATCH 0/2] rpmsg: glink_native: fix leak and cleanup

mirror of https://lore.kernel.org/lkml/
 help / color / mirror / Atom feed

* [PATCH 0/2] rpmsg: glink_native: fix leak and cleanup
@ 2025-08-22 10:00 srinivas.kandagatla
  2025-08-22 10:00 ` [PATCH 1/2] rpmsg: glink_native: fix rpmsg device leak srinivas.kandagatla
                   ` (2 more replies)
  0 siblings, 3 replies; 7+ messages in thread
From: srinivas.kandagatla @ 2025-08-22 10:00 UTC (permalink / raw)
  To: andersson
  Cc: mathieu.poirier, srichara, linux-arm-msm, linux-remoteproc,
	linux-kernel, Srinivas Kandagatla

From: Srinivas Kandagatla <srinivas.kandagatla@oss.qualcomm.com>

This series fixes a rpmsg_device leak in glink_native, and also adds
helper function to remove some code duplication.

Am not 100% sure if this behaviour was intentional and not allow
rpmsg-char interface to work from glink_native, but by the looks
of the code it looks buggy, which is why am sending this series for
discussion.

Srinivas Kandagatla (2):
  rpmsg: glink_native: fix rpmsg device leak
  rpmsg: glink_native: remove duplicate code for rpmsg device remove

 drivers/rpmsg/qcom_glink_native.c | 35 ++++++++++++++-----------------
 1 file changed, 16 insertions(+), 19 deletions(-)

-- 
2.50.0


^ permalink raw reply	[flat|nested] 7+ messages in thread

* [PATCH 1/2] rpmsg: glink_native: fix rpmsg device leak
  2025-08-22 10:00 [PATCH 0/2] rpmsg: glink_native: fix leak and cleanup srinivas.kandagatla
@ 2025-08-22 10:00 ` srinivas.kandagatla
  2025-08-22 10:12   ` Dmitry Baryshkov
  2025-08-22 10:00 ` [PATCH 2/2] rpmsg: glink_native: remove duplicate code for rpmsg device remove srinivas.kandagatla
  2025-11-26 20:10 ` [PATCH 0/2] rpmsg: glink_native: fix leak and cleanup Bjorn Andersson
  2 siblings, 1 reply; 7+ messages in thread
From: srinivas.kandagatla @ 2025-08-22 10:00 UTC (permalink / raw)
  To: andersson
  Cc: mathieu.poirier, srichara, linux-arm-msm, linux-remoteproc,
	linux-kernel, Srinivas Kandagatla, Stable

From: Srinivas Kandagatla <srinivas.kandagatla@oss.qualcomm.com>

While testing rpmsg-char interface it was noticed that duplicate sysfs
entries are getting created and below warning is noticed.

Reason for this is that we are leaking rpmsg device pointer, setting it
null without actually unregistering device.
Any further attempts to unregister fail because rpdev is NULL,
resulting in a leak.

Fix this by unregistering rpmsg device before removing its reference
from rpmsg channel.

sysfs: cannot create duplicate filename '/devices/platform/soc@0/3700000.remot
eproc/remoteproc/remoteproc1/3700000.remoteproc:glink-edge/3700000.remoteproc:
glink-edge.adsp_apps.-1.-1'
[  114.115347] CPU: 0 UID: 0 PID: 9 Comm: kworker/0:0 Not
 tainted 6.16.0-rc4 #7 PREEMPT
[  114.115355] Hardware name: Qualcomm Technologies, Inc. Robotics RB3gen2 (DT)
[  114.115358] Workqueue: events qcom_glink_work
[  114.115371] Call trace:8
[  114.115374]  show_stack+0x18/0x24 (C)
[  114.115382]  dump_stack_lvl+0x60/0x80
[  114.115388]  dump_stack+0x18/0x24
[  114.115393]  sysfs_warn_dup+0x64/0x80
[  114.115402]  sysfs_create_dir_ns+0xf4/0x120
[  114.115409]  kobject_add_internal+0x98/0x260
[  114.115416]  kobject_add+0x9c/0x108
[  114.115421]  device_add+0xc4/0x7a0
[  114.115429]  rpmsg_register_device+0x5c/0xb0
[  114.115434]  qcom_glink_work+0x4bc/0x820
[  114.115438]  process_one_work+0x148/0x284
[  114.115446]  worker_thread+0x2c4/0x3e0
[  114.115452]  kthread+0x12c/0x204
[  114.115457]  ret_from_fork+0x10/0x20
[  114.115464] kobject: kobject_add_internal failed for 3700000.remoteproc:
glink-edge.adsp_apps.-1.-1 with -EEXIST, don't try to register things with
the same name in the same directory.
[  114.250045] rpmsg 3700000.remoteproc:glink-edge.adsp_apps.-1.-1:
device_add failed: -17

Fixes: 835764ddd9af ("rpmsg: glink: Move the common glink protocol implementation to glink_native.c")
Cc: <Stable@vger.kernel.org>
Signed-off-by: Srinivas Kandagatla <srinivas.kandagatla@oss.qualcomm.com>
---
 drivers/rpmsg/qcom_glink_native.c | 8 ++++++++
 1 file changed, 8 insertions(+)

diff --git a/drivers/rpmsg/qcom_glink_native.c b/drivers/rpmsg/qcom_glink_native.c
index 820a6ca5b1d7..3a15d9d10808 100644
--- a/drivers/rpmsg/qcom_glink_native.c
+++ b/drivers/rpmsg/qcom_glink_native.c
@@ -1399,6 +1399,7 @@ static void qcom_glink_destroy_ept(struct rpmsg_endpoint *ept)
 {
 	struct glink_channel *channel = to_glink_channel(ept);
 	struct qcom_glink *glink = channel->glink;
+	struct rpmsg_channel_info chinfo;
 	unsigned long flags;
 
 	spin_lock_irqsave(&channel->recv_lock, flags);
@@ -1406,6 +1407,13 @@ static void qcom_glink_destroy_ept(struct rpmsg_endpoint *ept)
 	spin_unlock_irqrestore(&channel->recv_lock, flags);
 
 	/* Decouple the potential rpdev from the channel */
+	if (channel->rpdev) {
+		strscpy_pad(chinfo.name, channel->name, sizeof(chinfo.name));
+		chinfo.src = RPMSG_ADDR_ANY;
+		chinfo.dst = RPMSG_ADDR_ANY;
+
+		rpmsg_unregister_device(glink->dev, &chinfo);
+	}
 	channel->rpdev = NULL;
 
 	qcom_glink_send_close_req(glink, channel);
-- 
2.50.0


^ permalink raw reply	[flat|nested] 7+ messages in thread

* Re: [PATCH 1/2] rpmsg: glink_native: fix rpmsg device leak
  2025-08-22 10:00 ` [PATCH 1/2] rpmsg: glink_native: fix rpmsg device leak srinivas.kandagatla
@ 2025-08-22 10:12   ` Dmitry Baryshkov
  0 siblings, 0 replies; 7+ messages in thread
From: Dmitry Baryshkov @ 2025-08-22 10:12 UTC (permalink / raw)
  To: srinivas.kandagatla
  Cc: andersson, mathieu.poirier, srichara, linux-arm-msm,
	linux-remoteproc, linux-kernel, Stable

On Fri, Aug 22, 2025 at 11:00:42AM +0100, srinivas.kandagatla@oss.qualcomm.com wrote:
> From: Srinivas Kandagatla <srinivas.kandagatla@oss.qualcomm.com>

Please adjust your git's sendemail.from line to include your name. Then
git-send-email will not generate extra From headers.

> 
> While testing rpmsg-char interface it was noticed that duplicate sysfs
> entries are getting created and below warning is noticed.
> 
> Reason for this is that we are leaking rpmsg device pointer, setting it
> null without actually unregistering device.
> Any further attempts to unregister fail because rpdev is NULL,
> resulting in a leak.
> 
> Fix this by unregistering rpmsg device before removing its reference
> from rpmsg channel.
> 
> sysfs: cannot create duplicate filename '/devices/platform/soc@0/3700000.remot
> eproc/remoteproc/remoteproc1/3700000.remoteproc:glink-edge/3700000.remoteproc:
> glink-edge.adsp_apps.-1.-1'
> [  114.115347] CPU: 0 UID: 0 PID: 9 Comm: kworker/0:0 Not
>  tainted 6.16.0-rc4 #7 PREEMPT
> [  114.115355] Hardware name: Qualcomm Technologies, Inc. Robotics RB3gen2 (DT)
> [  114.115358] Workqueue: events qcom_glink_work
> [  114.115371] Call trace:8
> [  114.115374]  show_stack+0x18/0x24 (C)
> [  114.115382]  dump_stack_lvl+0x60/0x80
> [  114.115388]  dump_stack+0x18/0x24
> [  114.115393]  sysfs_warn_dup+0x64/0x80
> [  114.115402]  sysfs_create_dir_ns+0xf4/0x120
> [  114.115409]  kobject_add_internal+0x98/0x260
> [  114.115416]  kobject_add+0x9c/0x108
> [  114.115421]  device_add+0xc4/0x7a0
> [  114.115429]  rpmsg_register_device+0x5c/0xb0
> [  114.115434]  qcom_glink_work+0x4bc/0x820
> [  114.115438]  process_one_work+0x148/0x284
> [  114.115446]  worker_thread+0x2c4/0x3e0
> [  114.115452]  kthread+0x12c/0x204
> [  114.115457]  ret_from_fork+0x10/0x20
> [  114.115464] kobject: kobject_add_internal failed for 3700000.remoteproc:
> glink-edge.adsp_apps.-1.-1 with -EEXIST, don't try to register things with
> the same name in the same directory.
> [  114.250045] rpmsg 3700000.remoteproc:glink-edge.adsp_apps.-1.-1:
> device_add failed: -17
> 
> Fixes: 835764ddd9af ("rpmsg: glink: Move the common glink protocol implementation to glink_native.c")
> Cc: <Stable@vger.kernel.org>
> Signed-off-by: Srinivas Kandagatla <srinivas.kandagatla@oss.qualcomm.com>
> ---
>  drivers/rpmsg/qcom_glink_native.c | 8 ++++++++
>  1 file changed, 8 insertions(+)
> 

Reviewed-by: Dmitry Baryshkov <dmitry.baryshkov@oss.qualcomm.com>


-- 
With best wishes
Dmitry

^ permalink raw reply	[flat|nested] 7+ messages in thread

* [PATCH 2/2] rpmsg: glink_native: remove duplicate code for rpmsg device remove
  2025-08-22 10:00 [PATCH 0/2] rpmsg: glink_native: fix leak and cleanup srinivas.kandagatla
  2025-08-22 10:00 ` [PATCH 1/2] rpmsg: glink_native: fix rpmsg device leak srinivas.kandagatla
@ 2025-08-22 10:00 ` srinivas.kandagatla
  2025-08-22 10:14   ` Dmitry Baryshkov
  2025-11-26 20:10 ` [PATCH 0/2] rpmsg: glink_native: fix leak and cleanup Bjorn Andersson
  2 siblings, 1 reply; 7+ messages in thread
From: srinivas.kandagatla @ 2025-08-22 10:00 UTC (permalink / raw)
  To: andersson
  Cc: mathieu.poirier, srichara, linux-arm-msm, linux-remoteproc,
	linux-kernel, Srinivas Kandagatla

From: Srinivas Kandagatla <srinivas.kandagatla@oss.qualcomm.com>

rpmsg device remove code is duplicated in at-least 2-3 places, add a
helper function to remove this duplicated code.

Signed-off-by: Srinivas Kandagatla <srinivas.kandagatla@oss.qualcomm.com>
---
 drivers/rpmsg/qcom_glink_native.c | 43 ++++++++++++-------------------
 1 file changed, 16 insertions(+), 27 deletions(-)

diff --git a/drivers/rpmsg/qcom_glink_native.c b/drivers/rpmsg/qcom_glink_native.c
index 3a15d9d10808..5ea096acc858 100644
--- a/drivers/rpmsg/qcom_glink_native.c
+++ b/drivers/rpmsg/qcom_glink_native.c
@@ -1395,11 +1395,23 @@ static int qcom_glink_announce_create(struct rpmsg_device *rpdev)
 	return 0;
 }
 
+static void qcom_glink_remove_rpmsg_device(struct qcom_glink *glink, struct glink_channel *channel)
+{
+	struct rpmsg_channel_info chinfo;
+
+	if (channel->rpdev) {
+		strscpy_pad(chinfo.name, channel->name, sizeof(chinfo.name));
+		chinfo.src = RPMSG_ADDR_ANY;
+		chinfo.dst = RPMSG_ADDR_ANY;
+		rpmsg_unregister_device(glink->dev, &chinfo);
+	}
+	channel->rpdev = NULL;
+}
+
 static void qcom_glink_destroy_ept(struct rpmsg_endpoint *ept)
 {
 	struct glink_channel *channel = to_glink_channel(ept);
 	struct qcom_glink *glink = channel->glink;
-	struct rpmsg_channel_info chinfo;
 	unsigned long flags;
 
 	spin_lock_irqsave(&channel->recv_lock, flags);
@@ -1407,14 +1419,7 @@ static void qcom_glink_destroy_ept(struct rpmsg_endpoint *ept)
 	spin_unlock_irqrestore(&channel->recv_lock, flags);
 
 	/* Decouple the potential rpdev from the channel */
-	if (channel->rpdev) {
-		strscpy_pad(chinfo.name, channel->name, sizeof(chinfo.name));
-		chinfo.src = RPMSG_ADDR_ANY;
-		chinfo.dst = RPMSG_ADDR_ANY;
-
-		rpmsg_unregister_device(glink->dev, &chinfo);
-	}
-	channel->rpdev = NULL;
+	qcom_glink_remove_rpmsg_device(glink, channel);
 
 	qcom_glink_send_close_req(glink, channel);
 }
@@ -1705,7 +1710,6 @@ static int qcom_glink_rx_open(struct qcom_glink *glink, unsigned int rcid,
 
 static void qcom_glink_rx_close(struct qcom_glink *glink, unsigned int rcid)
 {
-	struct rpmsg_channel_info chinfo;
 	struct glink_channel *channel;
 	unsigned long flags;
 
@@ -1721,14 +1725,7 @@ static void qcom_glink_rx_close(struct qcom_glink *glink, unsigned int rcid)
 	/* cancel pending rx_done work */
 	cancel_work_sync(&channel->intent_work);
 
-	if (channel->rpdev) {
-		strscpy_pad(chinfo.name, channel->name, sizeof(chinfo.name));
-		chinfo.src = RPMSG_ADDR_ANY;
-		chinfo.dst = RPMSG_ADDR_ANY;
-
-		rpmsg_unregister_device(glink->dev, &chinfo);
-	}
-	channel->rpdev = NULL;
+	qcom_glink_remove_rpmsg_device(glink, channel);
 
 	qcom_glink_send_close_ack(glink, channel);
 
@@ -1742,7 +1739,6 @@ static void qcom_glink_rx_close(struct qcom_glink *glink, unsigned int rcid)
 
 static void qcom_glink_rx_close_ack(struct qcom_glink *glink, unsigned int lcid)
 {
-	struct rpmsg_channel_info chinfo;
 	struct glink_channel *channel;
 	unsigned long flags;
 
@@ -1764,14 +1760,7 @@ static void qcom_glink_rx_close_ack(struct qcom_glink *glink, unsigned int lcid)
 	spin_unlock_irqrestore(&glink->idr_lock, flags);
 
 	/* Decouple the potential rpdev from the channel */
-	if (channel->rpdev) {
-		strscpy(chinfo.name, channel->name, sizeof(chinfo.name));
-		chinfo.src = RPMSG_ADDR_ANY;
-		chinfo.dst = RPMSG_ADDR_ANY;
-
-		rpmsg_unregister_device(glink->dev, &chinfo);
-	}
-	channel->rpdev = NULL;
+	qcom_glink_remove_rpmsg_device(glink, channel);
 
 	kref_put(&channel->refcount, qcom_glink_channel_release);
 }
-- 
2.50.0


^ permalink raw reply	[flat|nested] 7+ messages in thread

* Re: [PATCH 2/2] rpmsg: glink_native: remove duplicate code for rpmsg device remove
  2025-08-22 10:00 ` [PATCH 2/2] rpmsg: glink_native: remove duplicate code for rpmsg device remove srinivas.kandagatla
@ 2025-08-22 10:14   ` Dmitry Baryshkov
  2025-08-22 10:20     ` Srinivas Kandagatla
  0 siblings, 1 reply; 7+ messages in thread
From: Dmitry Baryshkov @ 2025-08-22 10:14 UTC (permalink / raw)
  To: srinivas.kandagatla
  Cc: andersson, mathieu.poirier, srichara, linux-arm-msm,
	linux-remoteproc, linux-kernel

On Fri, Aug 22, 2025 at 11:00:43AM +0100, srinivas.kandagatla@oss.qualcomm.com wrote:
> From: Srinivas Kandagatla <srinivas.kandagatla@oss.qualcomm.com>
> 
> rpmsg device remove code is duplicated in at-least 2-3 places, add a
> helper function to remove this duplicated code.

I think it's better to sqiash this into the previous patch. Otherwise
you are fixing the code that you've just added.

> 
> Signed-off-by: Srinivas Kandagatla <srinivas.kandagatla@oss.qualcomm.com>
> ---
>  drivers/rpmsg/qcom_glink_native.c | 43 ++++++++++++-------------------
>  1 file changed, 16 insertions(+), 27 deletions(-)
> 
-- 
With best wishes
Dmitry

^ permalink raw reply	[flat|nested] 7+ messages in thread

* Re: [PATCH 2/2] rpmsg: glink_native: remove duplicate code for rpmsg device remove
  2025-08-22 10:14   ` Dmitry Baryshkov
@ 2025-08-22 10:20     ` Srinivas Kandagatla
  0 siblings, 0 replies; 7+ messages in thread
From: Srinivas Kandagatla @ 2025-08-22 10:20 UTC (permalink / raw)
  To: Dmitry Baryshkov
  Cc: andersson, mathieu.poirier, linux-arm-msm, linux-remoteproc,
	linux-kernel

On 8/22/25 11:14 AM, Dmitry Baryshkov wrote:
> On Fri, Aug 22, 2025 at 11:00:43AM +0100, srinivas.kandagatla@oss.qualcomm.com wrote:
>> From: Srinivas Kandagatla <srinivas.kandagatla@oss.qualcomm.com>
>>
>> rpmsg device remove code is duplicated in at-least 2-3 places, add a
>> helper function to remove this duplicated code.
> 
> I think it's better to sqiash this into the previous patch. Otherwise
> you are fixing the code that you've just added.

I did not wanted to add new cleanup code for backports, which is why I
split this up as fix and cleanup.

Am happy to merge these two if Bjorn prefers that way.

--srini

> 
>>
>> Signed-off-by: Srinivas Kandagatla <srinivas.kandagatla@oss.qualcomm.com>
>> ---
>>  drivers/rpmsg/qcom_glink_native.c | 43 ++++++++++++-------------------
>>  1 file changed, 16 insertions(+), 27 deletions(-)
>>


^ permalink raw reply	[flat|nested] 7+ messages in thread

* Re: [PATCH 0/2] rpmsg: glink_native: fix leak and cleanup
  2025-08-22 10:00 [PATCH 0/2] rpmsg: glink_native: fix leak and cleanup srinivas.kandagatla
  2025-08-22 10:00 ` [PATCH 1/2] rpmsg: glink_native: fix rpmsg device leak srinivas.kandagatla
  2025-08-22 10:00 ` [PATCH 2/2] rpmsg: glink_native: remove duplicate code for rpmsg device remove srinivas.kandagatla
@ 2025-11-26 20:10 ` Bjorn Andersson
  2 siblings, 0 replies; 7+ messages in thread
From: Bjorn Andersson @ 2025-11-26 20:10 UTC (permalink / raw)
  To: srinivas.kandagatla
  Cc: mathieu.poirier, srichara, linux-arm-msm, linux-remoteproc, linux-kernel


On Fri, 22 Aug 2025 11:00:41 +0100, srinivas.kandagatla@oss.qualcomm.com wrote:
> From: Srinivas Kandagatla <srinivas.kandagatla@oss.qualcomm.com>
> 
> This series fixes a rpmsg_device leak in glink_native, and also adds
> helper function to remove some code duplication.
> 
> Am not 100% sure if this behaviour was intentional and not allow
> rpmsg-char interface to work from glink_native, but by the looks
> of the code it looks buggy, which is why am sending this series for
> discussion.
> 
> [...]

Applied, thanks!

[1/2] rpmsg: glink_native: fix rpmsg device leak
      commit: a53e356df548f6b0e82529ef3cc6070f42622189
[2/2] rpmsg: glink_native: remove duplicate code for rpmsg device remove
      commit: 112766cdf2e5ea0a0f72b0304d57a6f74c066670

Best regards,
-- 
Bjorn Andersson <andersson@kernel.org>

^ permalink raw reply	[flat|nested] 7+ messages in thread

end of thread, other threads:[~2025-11-26 20:05 UTC | newest]

Thread overview: 7+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2025-08-22 10:00 [PATCH 0/2] rpmsg: glink_native: fix leak and cleanup srinivas.kandagatla
2025-08-22 10:00 ` [PATCH 1/2] rpmsg: glink_native: fix rpmsg device leak srinivas.kandagatla
2025-08-22 10:12   ` Dmitry Baryshkov
2025-08-22 10:00 ` [PATCH 2/2] rpmsg: glink_native: remove duplicate code for rpmsg device remove srinivas.kandagatla
2025-08-22 10:14   ` Dmitry Baryshkov
2025-08-22 10:20     ` Srinivas Kandagatla
2025-11-26 20:10 ` [PATCH 0/2] rpmsg: glink_native: fix leak and cleanup Bjorn Andersson

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox

all inboxes | Powered by JetHome®