RE: [PATCH v2 2/2] media: aspeed: drain IRQ work before stopping the engine

[Jammy Huang]

Hi Myeonghun,

> 
> Last close cancels resolution work while its IRQ producer is still live.
> A handler can queue work after cancellation and buffer cleanup. The worker
> can then turn clocks back on after stop has cleared VIDEO_STOPPED.
The stop() and worker changes look right to me. The release reorder
doesn't seem needed; details below.

Series structure: this patch fixes a real race, while 1/2 is hardening
for a probe path that can't currently hit the problem. Please add the
irq field here and put this patch first, so it can be backported on
its own with the Fixes: tag, independent of how 1/2 changes.

> 
> Release the VB2 queue before shutting down the engine, since streamoff can
> need capture and mode-detect interrupts. Disable and join the IRQ, drain work,
> stop the engine and free its buffers before re-enabling the IRQ. Check
> VIDEO_STOPPED before enabling clocks in the worker.
> 
> Fixes: d2b4387f3bdf ("media: platform: Add Aspeed Video Engine driver")
> Co-developed-by: Ijae Kim <ae878000@xxxxxxxxx>
> Signed-off-by: Ijae Kim <ae878000@xxxxxxxxx>
> Signed-off-by: Myeonghun Pak <mhun512@xxxxxxxxx>
> ---
> No hardware testing.
> 
> Changes in v2:
> - Split normal last-close ordering into a separate patch.
> - Balance IRQ disable/enable within stop; add no reopen state.
> 
>  drivers/media/platform/aspeed/aspeed-video.c | 14 +++++++++-----
>  1 file changed, 9 insertions(+), 5 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
> @@ -1423,6 +1423,7 @@
>  static void aspeed_video_stop(struct aspeed_video *video)  {
>  	set_bit(VIDEO_STOPPED, &video->flags);
> +	disable_irq(video->irq);
>  	cancel_delayed_work_sync(&video->res_work);
OK. One side effect: if res_work is already in
aspeed_video_get_resolution(), it waits for a mode-detect IRQ that
can't arrive, so close may block for about MODE_DETECT_TIMEOUT while
holding video_lock. That's acceptable around a mode change, but please
add a comment here and mention it in the commit message.

> 
>  	aspeed_video_off(video);
> @@ -1438,6 +1439,7 @@
> 
>  	video->v4l2_input_status = V4L2_IN_ST_NO_SIGNAL;
>  	video->flags = 0;
> +	enable_irq(video->irq);
>  }

> 
>  static int aspeed_video_querycap(struct file *file, void *fh, @@ -1847,11
> +1849,11 @@
>  	struct aspeed_video *video = container_of(dwork, struct aspeed_video,
>  						  res_work);
> 
> -	aspeed_video_on(video);
> -
>  	/* Exit early in case no clients remain */
>  	if (test_bit(VIDEO_STOPPED, &video->flags))
>  		goto done;
> +
> +	aspeed_video_on(video);
Looks fine for me.

> 
>  	aspeed_video_init_regs(video);
> 
> @@ -1901,15 +1903,17 @@
> 
>  static int aspeed_video_release(struct file *file)  {
> +	bool last;
>  	int rc;
>  	struct aspeed_video *video = video_drvdata(file);
> 
>  	mutex_lock(&video->video_lock);
> 
> -	if (v4l2_fh_is_singular_file(file))
> +	last = v4l2_fh_is_singular_file(file);
> +	rc = _vb2_fop_release(file, NULL);
> +
> +	if (last)
>  		aspeed_video_stop(video);
> -
> -	rc = _vb2_fop_release(file, NULL);
I don't see what this fixes. In the original order, stop() gates the
clocks, which halts the engine, and clears VIDEO_FRAME_INPRG, so
stop_streaming() returns the buffers right away without needing any
interrupt.

With the new order, closing while streaming can wait up to
STOP_TIMEOUT, then reset and re-run mode detect (another
MODE_DETECT_TIMEOUT), only for stop() to shut everything down.

Please drop this hunk and update the commit message and changelog to
match.

Thanks,
Jammy> 
>  	mutex_unlock(&video->video_lock);
>