[PATCH] platform/raspberrypi: vchiq: Stop kthreads on vchiq_platform_init() failure

[PATCH] platform/raspberrypi: vchiq: Stop kthreads on vchiq_platform_init() failure

[Tomasz Unger]

vchiq_init_state() creates and wakes the slot handler, recycle and sync
kthreads. If a later step in vchiq_platform_init() fails, the function
returns without stopping them and vchiq_probe() only propagates the
error. devres then frees what probe allocated, including the driver
data and the slot memory the kthreads operate on.

Forcing devm_request_irq() to fail on a Raspberry Pi 4 Model B makes
all three kthreads fault right after the failed probe:

  bcm2835_vchiq fe00b840.mailbox: failed to register irq=34
  bcm2835_vchiq fe00b840.mailbox: arm: Could not initialize vchiq platform
  Unable to handle kernel paging request at virtual address ffff800080ba402c
  Internal error: Oops: 0000000096000007 [#1]  SMP
  CPU: 1 UID: 0 PID: 370 Comm: vchiq-sync/0 Not tainted 7.4.0-rc1-trace #1
  pc : sync_func+0x44/0x3a8 [vchiq]
  Call trace:
   sync_func+0x44/0x3a8 [vchiq] (P)
   kthread+0x128/0x150
   ret_from_fork+0x10/0x20

If the kthreads are asleep at that point they are left behind instead,
three per failed probe, still pointing at the freed memory.

Map the registers and look up the interrupt before vchiq_init_state().
Neither depends on the VCHIQ state, so these two failures need no
unwinding. On the remaining failures stop the kthreads with
vchiq_platform_uninit(), as is already done when registering the
character device fails. Move vchiq_platform_uninit() and
vchiq_platform_get_arm_state() above vchiq_platform_init() for that,
without changing them.

The missing unwind is older than the commit named below, but the
kthreads could not be stopped before it, so it is the earliest point
this fix applies to.

The bug was found and the fix drafted with the help of an AI coding
assistant (Claude).

Fixes: 863a756aaf49 ("staging: vc04_services: vchiq_core: Stop kthreads on vchiq module unload")
Assisted-by: Claude:claude-fable-5-2
Signed-off-by: Tomasz Unger <tomasz.unger@yahoo.pl>
---
Verified on a Raspberry Pi 4 Model B with a test-only one-line change
that forces devm_request_irq() to fail. Without this patch the three
kthreads oops as shown above. With it probe fails cleanly, no vchiq
kthreads remain after four failed attempts and the module unloads.
Without the forced error the driver probes normally and audio playback
over VCHIQ works.
---
 .../raspberrypi/vchiq-interface/vchiq_arm.c        | 57 ++++++++++++----------
 1 file changed, 31 insertions(+), 26 deletions(-)

diff --git a/drivers/platform/raspberrypi/vchiq-interface/vchiq_arm.c b/drivers/platform/raspberrypi/vchiq-interface/vchiq_arm.c
index e78641703252..11764a2590e4 100644
--- a/drivers/platform/raspberrypi/vchiq-interface/vchiq_arm.c
+++ b/drivers/platform/raspberrypi/vchiq-interface/vchiq_arm.c
@@ -187,6 +187,25 @@ void vchiq_add_connected_callback(struct vchiq_device *device, void (*callback)(
 }
 EXPORT_SYMBOL(vchiq_add_connected_callback);
 
+static struct vchiq_arm_state *vchiq_platform_get_arm_state(struct vchiq_state *state)
+{
+	return (struct vchiq_arm_state *)state->platform_state;
+}
+
+static void
+vchiq_platform_uninit(struct vchiq_drv_mgmt *mgmt)
+{
+	struct vchiq_arm_state *arm_state;
+
+	kthread_stop(mgmt->state.sync_thread);
+	kthread_stop(mgmt->state.recycle_thread);
+	kthread_stop(mgmt->state.slot_handler_thread);
+
+	arm_state = vchiq_platform_get_arm_state(&mgmt->state);
+	if (!IS_ERR_OR_NULL(arm_state->ka_thread))
+		kthread_stop(arm_state->ka_thread);
+}
+
 static int vchiq_platform_init(struct platform_device *pdev, struct vchiq_state *state)
 {
 	struct device *dev = &pdev->dev;
@@ -243,10 +262,6 @@ static int vchiq_platform_init(struct platform_device *pdev, struct vchiq_state
 	sema_init(&drv_mgmt->free_fragments_sema, MAX_FRAGMENTS);
 	sema_init(&drv_mgmt->free_fragments_mutex, 1);
 
-	err = vchiq_init_state(state, vchiq_slot_zero, dev);
-	if (err)
-		return err;
-
 	drv_mgmt->regs = devm_platform_ioremap_resource(pdev, 0);
 	if (IS_ERR(drv_mgmt->regs))
 		return PTR_ERR(drv_mgmt->regs);
@@ -255,11 +270,15 @@ static int vchiq_platform_init(struct platform_device *pdev, struct vchiq_state
 	if (irq <= 0)
 		return irq;
 
+	err = vchiq_init_state(state, vchiq_slot_zero, dev);
+	if (err)
+		return err;
+
 	err = devm_request_irq(dev, irq, vchiq_doorbell_irq, IRQF_IRQPOLL,
 			       "VCHIQ doorbell", state);
 	if (err) {
 		dev_err(dev, "failed to register irq=%d\n", irq);
-		return err;
+		goto err_stop_threads;
 	}
 
 	/* Send the base address of the slots to VideoCore */
@@ -268,13 +287,14 @@ static int vchiq_platform_init(struct platform_device *pdev, struct vchiq_state
 				    &channelbase, sizeof(channelbase));
 	if (err) {
 		dev_err(dev, "failed to send firmware property: %d\n", err);
-		return err;
+		goto err_stop_threads;
 	}
 
 	if (channelbase) {
 		dev_err(dev, "failed to set channelbase (response: %x)\n",
 			channelbase);
-		return -ENXIO;
+		err = -ENXIO;
+		goto err_stop_threads;
 	}
 
 	dev_dbg(&pdev->dev, "arm: vchiq_init - done (slots %p, phys %pad)\n",
@@ -284,6 +304,10 @@ static int vchiq_platform_init(struct platform_device *pdev, struct vchiq_state
 	vchiq_call_connected_callbacks(drv_mgmt);
 
 	return 0;
+
+err_stop_threads:
+	vchiq_platform_uninit(drv_mgmt);
+	return err;
 }
 
 int
@@ -309,25 +333,6 @@ vchiq_platform_init_state(struct vchiq_state *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;
-}
-
-static void
-vchiq_platform_uninit(struct vchiq_drv_mgmt *mgmt)
-{
-	struct vchiq_arm_state *arm_state;
-
-	kthread_stop(mgmt->state.sync_thread);
-	kthread_stop(mgmt->state.recycle_thread);
-	kthread_stop(mgmt->state.slot_handler_thread);
-
-	arm_state = vchiq_platform_get_arm_state(&mgmt->state);
-	if (!IS_ERR_OR_NULL(arm_state->ka_thread))
-		kthread_stop(arm_state->ka_thread);
-}
-
 void vchiq_dump_platform_state(struct seq_file *f)
 {
 	seq_puts(f, "  Platform: 2835 (VC master)\n");

---
base-commit: 6628e7d4c6b25c8d18f1345e3a20d21829aae7e4
change-id: 20261114-vchiq-probe-kthread-leak-8f6178695a33

Best regards,
--  
Tomasz Unger <tomasz.unger@yahoo.pl>

Re: [PATCH] platform/raspberrypi: vchiq: Stop kthreads on vchiq_platform_init() failure

[Stefan Wahren]

Hi Tomasz,

Am 14.11.26 um 01:01 schrieb Tomasz Unger:
> vchiq_init_state() creates and wakes the slot handler, recycle and sync
> kthreads. If a later step in vchiq_platform_init() fails, the function
> returns without stopping them and vchiq_probe() only propagates the
> error. devres then frees what probe allocated, including the driver
> data and the slot memory the kthreads operate on.
>
> Forcing devm_request_irq() to fail on a Raspberry Pi 4 Model B makes
> all three kthreads fault right after the failed probe:
>
>    bcm2835_vchiq fe00b840.mailbox: failed to register irq=34
>    bcm2835_vchiq fe00b840.mailbox: arm: Could not initialize vchiq platform
>    Unable to handle kernel paging request at virtual address ffff800080ba402c
>    Internal error: Oops: 0000000096000007 [#1]  SMP
>    CPU: 1 UID: 0 PID: 370 Comm: vchiq-sync/0 Not tainted 7.4.0-rc1-trace #1
>    pc : sync_func+0x44/0x3a8 [vchiq]
>    Call trace:
>     sync_func+0x44/0x3a8 [vchiq] (P)
>     kthread+0x128/0x150
>     ret_from_fork+0x10/0x20
>
> If the kthreads are asleep at that point they are left behind instead,
> three per failed probe, still pointing at the freed memory.
>
> Map the registers and look up the interrupt before vchiq_init_state().
> Neither depends on the VCHIQ state, so these two failures need no
> unwinding. On the remaining failures stop the kthreads with
> vchiq_platform_uninit(), as is already done when registering the
> character device fails. Move vchiq_platform_uninit() and
> vchiq_platform_get_arm_state() above vchiq_platform_init() for that,
> without changing them.
>
> The missing unwind is older than the commit named below, but the
> kthreads could not be stopped before it, so it is the earliest point
> this fix applies to.
>
> The bug was found and the fix drafted with the help of an AI coding
> assistant (Claude).
>
> Fixes: 863a756aaf49 ("staging: vc04_services: vchiq_core: Stop kthreads on vchiq module unload")
> Assisted-by: Claude:claude-fable-5-2
> Signed-off-by: Tomasz Unger <tomasz.unger@yahoo.pl>
> ---
> Verified on a Raspberry Pi 4 Model B with a test-only one-line change
> that forces devm_request_irq() to fail. Without this patch the three
> kthreads oops as shown above. With it probe fails cleanly, no vchiq
> kthreads remain after four failed attempts and the module unloads.
> Without the forced error the driver probes normally and audio playback
> over VCHIQ works.
> ---
>   .../raspberrypi/vchiq-interface/vchiq_arm.c        | 57 ++++++++++++----------
>   1 file changed, 31 insertions(+), 26 deletions(-)
>
> diff --git a/drivers/platform/raspberrypi/vchiq-interface/vchiq_arm.c b/drivers/platform/raspberrypi/vchiq-interface/vchiq_arm.c
> index e78641703252..11764a2590e4 100644
> --- a/drivers/platform/raspberrypi/vchiq-interface/vchiq_arm.c
> +++ b/drivers/platform/raspberrypi/vchiq-interface/vchiq_arm.c
> @@ -187,6 +187,25 @@ void vchiq_add_connected_callback(struct vchiq_device *device, void (*callback)(
>   }
>   EXPORT_SYMBOL(vchiq_add_connected_callback);
>   
> +static struct vchiq_arm_state *vchiq_platform_get_arm_state(struct vchiq_state *state)
> +{
> +	return (struct vchiq_arm_state *)state->platform_state;
> +}
> +
> +static void
> +vchiq_platform_uninit(struct vchiq_drv_mgmt *mgmt)
> +{
> +	struct vchiq_arm_state *arm_state;
> +
> +	kthread_stop(mgmt->state.sync_thread);
> +	kthread_stop(mgmt->state.recycle_thread);
> +	kthread_stop(mgmt->state.slot_handler_thread);
> +
> +	arm_state = vchiq_platform_get_arm_state(&mgmt->state);
> +	if (!IS_ERR_OR_NULL(arm_state->ka_thread))
> +		kthread_stop(arm_state->ka_thread);
> +}
> +

instead of moving the defintion of vchiq_platform_get_arm_state() and 
vchiq_platform_uninit() upwards, I would suggest to make forward 
declarations of them. This should avoid backporting conflicts and makes 
the patch easier to review.

Beside of this, looks good to me.

Thanks

>   static int vchiq_platform_init(struct platform_device *pdev, struct vchiq_state *state)
>   {
>   	struct device *dev = &pdev->dev;
> @@ -243,10 +262,6 @@ static int vchiq_platform_init(struct platform_device *pdev, struct vchiq_state
>   	sema_init(&drv_mgmt->free_fragments_sema, MAX_FRAGMENTS);
>   	sema_init(&drv_mgmt->free_fragments_mutex, 1);
>   
> -	err = vchiq_init_state(state, vchiq_slot_zero, dev);
> -	if (err)
> -		return err;
> -
>   	drv_mgmt->regs = devm_platform_ioremap_resource(pdev, 0);
>   	if (IS_ERR(drv_mgmt->regs))
>   		return PTR_ERR(drv_mgmt->regs);
> @@ -255,11 +270,15 @@ static int vchiq_platform_init(struct platform_device *pdev, struct vchiq_state
>   	if (irq <= 0)
>   		return irq;
>   
> +	err = vchiq_init_state(state, vchiq_slot_zero, dev);
> +	if (err)
> +		return err;
> +
>   	err = devm_request_irq(dev, irq, vchiq_doorbell_irq, IRQF_IRQPOLL,
>   			       "VCHIQ doorbell", state);
>   	if (err) {
>   		dev_err(dev, "failed to register irq=%d\n", irq);
> -		return err;
> +		goto err_stop_threads;
>   	}
>   
>   	/* Send the base address of the slots to VideoCore */
> @@ -268,13 +287,14 @@ static int vchiq_platform_init(struct platform_device *pdev, struct vchiq_state
>   				    &channelbase, sizeof(channelbase));
>   	if (err) {
>   		dev_err(dev, "failed to send firmware property: %d\n", err);
> -		return err;
> +		goto err_stop_threads;
>   	}
>   
>   	if (channelbase) {
>   		dev_err(dev, "failed to set channelbase (response: %x)\n",
>   			channelbase);
> -		return -ENXIO;
> +		err = -ENXIO;
> +		goto err_stop_threads;
>   	}
>   
>   	dev_dbg(&pdev->dev, "arm: vchiq_init - done (slots %p, phys %pad)\n",
> @@ -284,6 +304,10 @@ static int vchiq_platform_init(struct platform_device *pdev, struct vchiq_state
>   	vchiq_call_connected_callbacks(drv_mgmt);
>   
>   	return 0;
> +
> +err_stop_threads:
> +	vchiq_platform_uninit(drv_mgmt);
> +	return err;
>   }
>   
>   int
> @@ -309,25 +333,6 @@ vchiq_platform_init_state(struct vchiq_state *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;
> -}
> -
> -static void
> -vchiq_platform_uninit(struct vchiq_drv_mgmt *mgmt)
> -{
> -	struct vchiq_arm_state *arm_state;
> -
> -	kthread_stop(mgmt->state.sync_thread);
> -	kthread_stop(mgmt->state.recycle_thread);
> -	kthread_stop(mgmt->state.slot_handler_thread);
> -
> -	arm_state = vchiq_platform_get_arm_state(&mgmt->state);
> -	if (!IS_ERR_OR_NULL(arm_state->ka_thread))
> -		kthread_stop(arm_state->ka_thread);
> -}
> -
>   void vchiq_dump_platform_state(struct seq_file *f)
>   {
>   	seq_puts(f, "  Platform: 2835 (VC master)\n");
>
> ---
> base-commit: 6628e7d4c6b25c8d18f1345e3a20d21829aae7e4
> change-id: 20261114-vchiq-probe-kthread-leak-8f6178695a33
>
> Best regards,
> --
> Tomasz Unger <tomasz.unger@yahoo.pl>