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

[Stefan Wahren <wahrenst@gmx.net>]

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>