[PATCH V2 0/3] staging: vchiq_arm: Fix regression & resource leaks

[PATCH V2 0/3] staging: vchiq_arm: Fix regression & resource leaks

[Stefan Wahren]

This small series fixes a regression introduced by recent changes and
possible resource leaks in the VCHIQ driver.

Changes in V2:
- Add Maíra's Reviewed-bys
- Fix Reported address in patch #2

Stefan Wahren (3):
  Revert "staging: vchiq_arm: Improve initial VCHIQ connect"
  Revert "staging: vchiq_arm: Create keep-alive thread during probe"
  staging: vchiq_arm: Make vchiq_shutdown never fail

 .../interface/vchiq_arm/vchiq_arm.c           | 98 +++++++++++--------
 .../interface/vchiq_arm/vchiq_core.c          |  1 -
 .../interface/vchiq_arm/vchiq_core.h          |  2 -
 3 files changed, 56 insertions(+), 45 deletions(-)

-- 
2.34.1

[PATCH V2 1/3] Revert "staging: vchiq_arm: Improve initial VCHIQ connect"

[Stefan Wahren]

The commit 3e5def4249b9 ("staging: vchiq_arm: Improve initial VCHIQ connect")
based on the assumption that in good case the VCHIQ connect always happen and
therefore the keep-alive thread is guaranteed to be woken up. This is wrong,
because in certain configurations there are no VCHIQ users and so the VCHIQ
connect never happen. So revert it.

Fixes: 3e5def4249b9 ("staging: vchiq_arm: Improve initial VCHIQ connect")
Reported-by: Maíra Canal <mcanal@igalia.com>
Closes: https://lore.kernel.org/linux-staging/ba35b960-a981-4671-9f7f-060da10feaa1@usp.br/
Cc: <stable@kernel.org>
Signed-off-by: Stefan Wahren <wahrenst@gmx.net>
Reviewed-by: Maíra Canal <mcanal@igalia.com>
---
 .../interface/vchiq_arm/vchiq_arm.c           | 28 ++++++++++++++-----
 .../interface/vchiq_arm/vchiq_core.c          |  1 -
 .../interface/vchiq_arm/vchiq_core.h          |  2 --
 3 files changed, 21 insertions(+), 10 deletions(-)

diff --git a/drivers/staging/vc04_services/interface/vchiq_arm/vchiq_arm.c b/drivers/staging/vc04_services/interface/vchiq_arm/vchiq_arm.c
index 5dbf8d53db09..cdf5687ad4f0 100644
--- a/drivers/staging/vc04_services/interface/vchiq_arm/vchiq_arm.c
+++ b/drivers/staging/vc04_services/interface/vchiq_arm/vchiq_arm.c
@@ -97,6 +97,13 @@ struct vchiq_arm_state {
 	 * tracked separately with the state.
 	 */
 	int peer_use_count;
+
+	/*
+	 * Flag to indicate that the first vchiq connect has made it through.
+	 * This means that both sides should be fully ready, and we should
+	 * be able to suspend after this point.
+	 */
+	int first_connect;
 };
 
 static int
@@ -1329,19 +1336,26 @@ vchiq_check_service(struct vchiq_service *service)
 	return ret;
 }
 
-void vchiq_platform_connected(struct vchiq_state *state)
-{
-	struct vchiq_arm_state *arm_state = vchiq_platform_get_arm_state(state);
-
-	wake_up_process(arm_state->ka_thread);
-}
-
 void vchiq_platform_conn_state_changed(struct vchiq_state *state,
 				       enum vchiq_connstate oldstate,
 				       enum vchiq_connstate newstate)
 {
+	struct vchiq_arm_state *arm_state = vchiq_platform_get_arm_state(state);
+
 	dev_dbg(state->dev, "suspend: %d: %s->%s\n",
 		state->id, get_conn_state_name(oldstate), get_conn_state_name(newstate));
+	if (state->conn_state != VCHIQ_CONNSTATE_CONNECTED)
+		return;
+
+	write_lock_bh(&arm_state->susp_res_lock);
+	if (arm_state->first_connect) {
+		write_unlock_bh(&arm_state->susp_res_lock);
+		return;
+	}
+
+	arm_state->first_connect = 1;
+	write_unlock_bh(&arm_state->susp_res_lock);
+	wake_up_process(arm_state->ka_thread);
 }
 
 static const struct of_device_id vchiq_of_match[] = {
diff --git a/drivers/staging/vc04_services/interface/vchiq_arm/vchiq_core.c b/drivers/staging/vc04_services/interface/vchiq_arm/vchiq_core.c
index e7b0c800a205..e2cac0898b8f 100644
--- a/drivers/staging/vc04_services/interface/vchiq_arm/vchiq_core.c
+++ b/drivers/staging/vc04_services/interface/vchiq_arm/vchiq_core.c
@@ -3343,7 +3343,6 @@ vchiq_connect_internal(struct vchiq_state *state, struct vchiq_instance *instanc
 			return -EAGAIN;
 
 		vchiq_set_conn_state(state, VCHIQ_CONNSTATE_CONNECTED);
-		vchiq_platform_connected(state);
 		complete(&state->connect);
 	}
 
diff --git a/drivers/staging/vc04_services/interface/vchiq_arm/vchiq_core.h b/drivers/staging/vc04_services/interface/vchiq_arm/vchiq_core.h
index 3b5c0618e567..9b4e766990a4 100644
--- a/drivers/staging/vc04_services/interface/vchiq_arm/vchiq_core.h
+++ b/drivers/staging/vc04_services/interface/vchiq_arm/vchiq_core.h
@@ -575,8 +575,6 @@ int vchiq_send_remote_use(struct vchiq_state *state);
 
 int vchiq_send_remote_use_active(struct vchiq_state *state);
 
-void vchiq_platform_connected(struct vchiq_state *state);
-
 void vchiq_platform_conn_state_changed(struct vchiq_state *state,
 				       enum vchiq_connstate oldstate,
 				  enum vchiq_connstate newstate);
-- 
2.34.1

[PATCH V2 2/3] Revert "staging: vchiq_arm: Create keep-alive thread during probe"

[Stefan Wahren]

The commit 86bc88217006 ("staging: vchiq_arm: Create keep-alive thread
during probe") introduced a regression for certain configurations,
which doesn't have a VCHIQ user. This results in a unused and hanging
keep-alive thread:

  INFO: task vchiq-keep/0:85 blocked for more than 120 seconds.
        Not tainted 6.12.34-v8-+ #13
  "echo 0 > /proc/sys/kernel/hung_task_timeout_secs" disables this message.
  task:vchiq-keep/0    state:D stack:0 pid:85    tgid:85    ppid:2
  Call trace:
   __switch_to+0x188/0x230
   __schedule+0xa54/0xb28
   schedule+0x80/0x120
   schedule_preempt_disabled+0x30/0x50
   kthread+0xd4/0x1a0
   ret_from_fork+0x10/0x20

Fixes: 86bc88217006 ("staging: vchiq_arm: Create keep-alive thread during probe")
Reported-by: Maíra Canal <mcanal@igalia.com>
Closes: https://lore.kernel.org/linux-staging/ba35b960-a981-4671-9f7f-060da10feaa1@usp.br/
Cc: <stable@kernel.org>
Signed-off-by: Stefan Wahren <wahrenst@gmx.net>
Reviewed-by: Maíra Canal <mcanal@igalia.com>
---
 .../interface/vchiq_arm/vchiq_arm.c           | 69 ++++++++++---------
 1 file changed, 35 insertions(+), 34 deletions(-)

diff --git a/drivers/staging/vc04_services/interface/vchiq_arm/vchiq_arm.c b/drivers/staging/vc04_services/interface/vchiq_arm/vchiq_arm.c
index cdf5687ad4f0..6434cbdc1a6e 100644
--- a/drivers/staging/vc04_services/interface/vchiq_arm/vchiq_arm.c
+++ b/drivers/staging/vc04_services/interface/vchiq_arm/vchiq_arm.c
@@ -280,6 +280,29 @@ static int vchiq_platform_init(struct platform_device *pdev, struct vchiq_state
 	return 0;
 }
 
+int
+vchiq_platform_init_state(struct vchiq_state *state)
+{
+	struct vchiq_arm_state *platform_state;
+
+	platform_state = devm_kzalloc(state->dev, sizeof(*platform_state), GFP_KERNEL);
+	if (!platform_state)
+		return -ENOMEM;
+
+	rwlock_init(&platform_state->susp_res_lock);
+
+	init_completion(&platform_state->ka_evt);
+	atomic_set(&platform_state->ka_use_count, 0);
+	atomic_set(&platform_state->ka_use_ack_count, 0);
+	atomic_set(&platform_state->ka_release_count, 0);
+
+	platform_state->state = state;
+
+	state->platform_state = (struct opaque_platform_state *)platform_state;
+
+	return 0;
+}
+
 static struct vchiq_arm_state *vchiq_platform_get_arm_state(struct vchiq_state *state)
 {
 	return (struct vchiq_arm_state *)state->platform_state;
@@ -988,39 +1011,6 @@ vchiq_keepalive_thread_func(void *v)
 	return 0;
 }
 
-int
-vchiq_platform_init_state(struct vchiq_state *state)
-{
-	struct vchiq_arm_state *platform_state;
-	char threadname[16];
-
-	platform_state = devm_kzalloc(state->dev, sizeof(*platform_state), GFP_KERNEL);
-	if (!platform_state)
-		return -ENOMEM;
-
-	snprintf(threadname, sizeof(threadname), "vchiq-keep/%d",
-		 state->id);
-	platform_state->ka_thread = kthread_create(&vchiq_keepalive_thread_func,
-						   (void *)state, threadname);
-	if (IS_ERR(platform_state->ka_thread)) {
-		dev_err(state->dev, "couldn't create thread %s\n", threadname);
-		return PTR_ERR(platform_state->ka_thread);
-	}
-
-	rwlock_init(&platform_state->susp_res_lock);
-
-	init_completion(&platform_state->ka_evt);
-	atomic_set(&platform_state->ka_use_count, 0);
-	atomic_set(&platform_state->ka_use_ack_count, 0);
-	atomic_set(&platform_state->ka_release_count, 0);
-
-	platform_state->state = state;
-
-	state->platform_state = (struct opaque_platform_state *)platform_state;
-
-	return 0;
-}
-
 int
 vchiq_use_internal(struct vchiq_state *state, struct vchiq_service *service,
 		   enum USE_TYPE_E use_type)
@@ -1341,6 +1331,7 @@ void vchiq_platform_conn_state_changed(struct vchiq_state *state,
 				       enum vchiq_connstate newstate)
 {
 	struct vchiq_arm_state *arm_state = vchiq_platform_get_arm_state(state);
+	char threadname[16];
 
 	dev_dbg(state->dev, "suspend: %d: %s->%s\n",
 		state->id, get_conn_state_name(oldstate), get_conn_state_name(newstate));
@@ -1355,7 +1346,17 @@ void vchiq_platform_conn_state_changed(struct vchiq_state *state,
 
 	arm_state->first_connect = 1;
 	write_unlock_bh(&arm_state->susp_res_lock);
-	wake_up_process(arm_state->ka_thread);
+	snprintf(threadname, sizeof(threadname), "vchiq-keep/%d",
+		 state->id);
+	arm_state->ka_thread = kthread_create(&vchiq_keepalive_thread_func,
+					      (void *)state,
+					      threadname);
+	if (IS_ERR(arm_state->ka_thread)) {
+		dev_err(state->dev, "suspend: Couldn't create thread %s\n",
+			threadname);
+	} else {
+		wake_up_process(arm_state->ka_thread);
+	}
 }
 
 static const struct of_device_id vchiq_of_match[] = {
-- 
2.34.1

[PATCH V2 3/3] staging: vchiq_arm: Make vchiq_shutdown never fail

[Stefan Wahren]

Most of the users of vchiq_shutdown ignore the return value,
which is bad because this could lead to resource leaks.
So instead of changing all calls to vchiq_shutdown, it's easier
to make vchiq_shutdown never fail.

Fixes: 71bad7f08641 ("staging: add bcm2708 vchiq driver")
Signed-off-by: Stefan Wahren <wahrenst@gmx.net>
---
 drivers/staging/vc04_services/interface/vchiq_arm/vchiq_arm.c | 3 +--
 1 file changed, 1 insertion(+), 2 deletions(-)

diff --git a/drivers/staging/vc04_services/interface/vchiq_arm/vchiq_arm.c b/drivers/staging/vc04_services/interface/vchiq_arm/vchiq_arm.c
index 6434cbdc1a6e..721b15b7e13b 100644
--- a/drivers/staging/vc04_services/interface/vchiq_arm/vchiq_arm.c
+++ b/drivers/staging/vc04_services/interface/vchiq_arm/vchiq_arm.c
@@ -393,8 +393,7 @@ int vchiq_shutdown(struct vchiq_instance *instance)
 	struct vchiq_state *state = instance->state;
 	int ret = 0;
 
-	if (mutex_lock_killable(&state->mutex))
-		return -EAGAIN;
+	mutex_lock(&state->mutex);
 
 	/* Remove all services */
 	vchiq_shutdown_internal(state, instance);
-- 
2.34.1

Re: [PATCH V2 0/3] staging: vchiq_arm: Fix regression & resource leaks

[Greg Kroah-Hartman]

On Tue, Jul 15, 2025 at 06:11:05PM +0200, Stefan Wahren wrote:
> This small series fixes a regression introduced by recent changes and
> possible resource leaks in the VCHIQ driver.
> 
> Changes in V2:
> - Add Maíra's Reviewed-bys
> - Fix Reported address in patch #2
> 
> Stefan Wahren (3):
>   Revert "staging: vchiq_arm: Improve initial VCHIQ connect"
>   Revert "staging: vchiq_arm: Create keep-alive thread during probe"
>   staging: vchiq_arm: Make vchiq_shutdown never fail
> 
>  .../interface/vchiq_arm/vchiq_arm.c           | 98 +++++++++++--------
>  .../interface/vchiq_arm/vchiq_core.c          |  1 -
>  .../interface/vchiq_arm/vchiq_core.h          |  2 -
>  3 files changed, 56 insertions(+), 45 deletions(-)

I already applied v1, is there any code changes that are different here?

thanks,

greg k-h

Re: [PATCH V2 0/3] staging: vchiq_arm: Fix regression & resource leaks

[Stefan Wahren]

Am 15.07.25 um 19:44 schrieb Greg Kroah-Hartman:
> On Tue, Jul 15, 2025 at 06:11:05PM +0200, Stefan Wahren wrote:
>> This small series fixes a regression introduced by recent changes and
>> possible resource leaks in the VCHIQ driver.
>>
>> Changes in V2:
>> - Add Maíra's Reviewed-bys
>> - Fix Reported address in patch #2
>>
>> Stefan Wahren (3):
>>    Revert "staging: vchiq_arm: Improve initial VCHIQ connect"
>>    Revert "staging: vchiq_arm: Create keep-alive thread during probe"
>>    staging: vchiq_arm: Make vchiq_shutdown never fail
>>
>>   .../interface/vchiq_arm/vchiq_arm.c           | 98 +++++++++++--------
>>   .../interface/vchiq_arm/vchiq_core.c          |  1 -
>>   .../interface/vchiq_arm/vchiq_core.h          |  2 -
>>   3 files changed, 56 insertions(+), 45 deletions(-)
> I already applied v1, is there any code changes that are different here?
Strange, I didn't received a mail that the patches has been applied. No, 
there wasn't any code changes.

Best regards
>
> thanks,
>
> greg k-h

Re: [PATCH V2 0/3] staging: vchiq_arm: Fix regression & resource leaks

[Greg Kroah-Hartman]

On Tue, Jul 15, 2025 at 09:59:45PM +0200, Stefan Wahren wrote:
> Am 15.07.25 um 19:44 schrieb Greg Kroah-Hartman:
> > On Tue, Jul 15, 2025 at 06:11:05PM +0200, Stefan Wahren wrote:
> > > This small series fixes a regression introduced by recent changes and
> > > possible resource leaks in the VCHIQ driver.
> > > 
> > > Changes in V2:
> > > - Add Maíra's Reviewed-bys
> > > - Fix Reported address in patch #2
> > > 
> > > Stefan Wahren (3):
> > >    Revert "staging: vchiq_arm: Improve initial VCHIQ connect"
> > >    Revert "staging: vchiq_arm: Create keep-alive thread during probe"
> > >    staging: vchiq_arm: Make vchiq_shutdown never fail
> > > 
> > >   .../interface/vchiq_arm/vchiq_arm.c           | 98 +++++++++++--------
> > >   .../interface/vchiq_arm/vchiq_core.c          |  1 -
> > >   .../interface/vchiq_arm/vchiq_core.h          |  2 -
> > >   3 files changed, 56 insertions(+), 45 deletions(-)
> > I already applied v1, is there any code changes that are different here?
> Strange, I didn't received a mail that the patches has been applied. No,
> there wasn't any code changes.

Ah, I hadn't pushed it publiclly yet, that's why!  I'll go drop my
internal tree and queue up this one now instead, thanks.

greg k-h