RE: [PATCH v2 1/2] media: aspeed: join IRQ and work before probe cleanup

[Jammy Huang]

Hi Myeonghun,

Thanks for the patch.

> The setup failure path unprepares the clocks before devres frees the IRQ.
> An IRQ handler that has sampled an enabled status can still queue resolution
> work. Masking the source does not synchronize that handler.
Before aspeed_video_setup_video() registers the video node, nothing
calls aspeed_video_on(). The engine stays clock-gated and
VE_INTERRUPT_CTRL keeps its reset value, so I don't see how the handler
can sample an enabled status during probe. You say as much below the
'---'. Could you reword the commit message so it reads as hardening of
the cleanup order, not as a fix for an observed race? Then please
reconsider the Fixes: tag as well.

> 
> Save the IRQ number after a successful request, then free the action and drain
> resolution work before the manual JPEG and clock cleanup. Turn the engine off
> after the work has finished. Request the IRQ after reset, clock and JPEG
> initialization so earlier init failures have no live IRQ.
> 
> Fixes: 3536169f8531 ("media: aspeed: fix clock handling logic")
> Co-developed-by: Ijae Kim <ae878000@xxxxxxxxx>
> Signed-off-by: Ijae Kim <ae878000@xxxxxxxxx>
> Signed-off-by: Myeonghun Pak <mhun512@xxxxxxxxx>
> ---
> No hardware testing.
> 
> With reset-default masking, no probe IRQ failure has been demonstrated; this
> part hardens cleanup ordering.
> 
> Changes in v2:
> - Join the IRQ and resolution work before manual probe-failure cleanup.
> - Rebase onto managed reserved-memory initialization.
> - Clean up JPEG and clocks if the late IRQ request fails.
> - Split normal last-close ordering into patch 2.
> 
>  drivers/media/platform/aspeed/aspeed-video.c | 24
> ++++++++++++++++--------
>  1 file changed, 16 insertions(+), 8 deletions(-)
> 
> diff --git a/drivers/media/platform/aspeed/aspeed-video.c
> b/drivers/media/platform/aspeed/aspeed-video.c
> --- a/drivers/media/platform/aspeed/aspeed-video.c
> +++ b/drivers/media/platform/aspeed/aspeed-video.c
> @@ -308,6 +308,7 @@
>   * perf:		holds the statistics primary for debugfs
>   */
>  struct aspeed_video {
> +	int irq;
>  	void __iomem *base;
This struct has a kernel-doc comment, so the new member needs a
description there, or W=1 builds will warn. Please also move it next
to the other resources rather than making it the first member.

>  	struct clk *eclk;
>  	struct clk *vclk;
> @@ -2232,14 +2233,6 @@
>  		return -ENODEV;
>  	}
> 
> -	rc = devm_request_threaded_irq(dev, irq, NULL, aspeed_video_irq,
> -				       IRQF_ONESHOT, DEVICE_NAME, video);
> -	if (rc < 0) {
> -		dev_err(dev, "Unable to request IRQ %d\n", irq);
> -		return rc;
> -	}
> -	dev_info(video->dev, "irq %d\n", irq);
> -
>  	video->reset = devm_reset_control_get(dev, NULL);
>  	if (IS_ERR(video->reset)) {
>  		dev_err(dev, "Unable to get reset\n"); @@ -2286,8 +2279,19 @@
> 
>  	aspeed_video_init_jpeg_table(video->jpeg.virt, video->yuv420);
> 
> +	rc = devm_request_threaded_irq(dev, irq, NULL, aspeed_video_irq,
> +				       IRQF_ONESHOT, DEVICE_NAME, video);
> +	if (rc < 0) {
> +		dev_err(dev, "Unable to request IRQ %d\n", irq);
> +		goto err_free_jpeg;
> +	}
> +	video->irq = irq;
> +	dev_info(video->dev, "irq %d\n", irq);
> +
>  	return 0;
> 
> +err_free_jpeg:
> +	aspeed_video_free_buf(video, &video->jpeg);
>  err_unprepare_vclk:
>  	clk_unprepare(video->vclk);
>  err_unprepare_eclk:
> @@ -2341,6 +2345,10 @@
> 
>  	rc = aspeed_video_setup_video(video);
>  	if (rc) {
> +		set_bit(VIDEO_STOPPED, &video->flags);
> +		devm_free_irq(video->dev, video->irq, video);
> +		cancel_delayed_work_sync(&video->res_work);
> +		aspeed_video_off(video);
>  		aspeed_video_free_buf(video, &video->jpeg);
I don't think this hunk does anything. Until setup_video() has
registered the video node, nothing calls aspeed_video_on(), so
VIDEO_CLOCKS_ON is clear and VE_INTERRUPT_CTRL keeps its reset value
of 0. The handler masks status with VE_INTERRUPT_CTRL and returns
IRQ_NONE, so res_work can never be queued. cancel_delayed_work_sync()
then has nothing to cancel, and aspeed_video_off() returns early.

>  		clk_unprepare(video->vclk);
>  		clk_unprepare(video->eclk);