[PATCH v2 0/2] media: aspeed: order probe cleanup and last close

[PATCH v2 0/2] media: aspeed: order probe cleanup and last close

[Myeonghun Pak]

This revision requests the IRQ after its dependencies and synchronizes
IRQ/work before manual probe cleanup. The second patch fixes last-close
ordering and keeps the engine available until VB2 release completes.

Compile-tested on ARM64 with LLVM, W=1.
No hardware testing.

Changes in v2:
- Rebase onto managed reserved-memory initialization.
- Remove the research sentence and Assisted-by tag.
- Join the IRQ and delayed work before manual probe-failure cleanup.
- Keep VB2 release before normal last-close hardware shutdown.
- Drain the IRQ producer before cancelling resolution work.
- Balance IRQ disable/enable within stop and check STOPPED before power-on.

Myeonghun Pak (2):
  media: aspeed: join IRQ and work before probe cleanup
  media: aspeed: drain IRQ work before stopping the engine

 drivers/media/platform/aspeed/aspeed-video.c | 38 +++++++++++++++++++++++++-------------
 1 file changed, 25 insertions(+), 13 deletions(-)

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

[Myeonghun Pak]

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.

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@gmail.com>
Signed-off-by: Ijae Kim <ae878000@gmail.com>
Signed-off-by: Myeonghun Pak <mhun512@gmail.com>
---
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;
 	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);
 		clk_unprepare(video->vclk);
 		clk_unprepare(video->eclk);

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@gmail.com>
> Signed-off-by: Ijae Kim <ae878000@gmail.com>
> Signed-off-by: Myeonghun Pak <mhun512@gmail.com>
> ---
> 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);

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

[Myeonghun Pak]

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.

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@gmail.com>
Signed-off-by: Ijae Kim <ae878000@gmail.com>
Signed-off-by: Myeonghun Pak <mhun512@gmail.com>
---
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);
 
 	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);
 
 	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);
 
 	mutex_unlock(&video->video_lock);
 

Re: [PATCH v2 0/2] media: aspeed: order probe cleanup and last close

[Hans Verkuil]

Eddie or Jammy,

On 10/11/2026 05:58, Myeonghun Pak wrote:
> This revision requests the IRQ after its dependencies and synchronizes
> IRQ/work before manual probe cleanup. The second patch fixes last-close
> ordering and keeps the engine available until VB2 release completes.
> 
> Compile-tested on ARM64 with LLVM, W=1.
> No hardware testing.

Can you review/test this series?

I don't feel comfortable merging this without your OK.

Regards,

	Hans

> 
> Changes in v2:
> - Rebase onto managed reserved-memory initialization.
> - Remove the research sentence and Assisted-by tag.
> - Join the IRQ and delayed work before manual probe-failure cleanup.
> - Keep VB2 release before normal last-close hardware shutdown.
> - Drain the IRQ producer before cancelling resolution work.
> - Balance IRQ disable/enable within stop and check STOPPED before power-on.
> 
> Myeonghun Pak (2):
>   media: aspeed: join IRQ and work before probe cleanup
>   media: aspeed: drain IRQ work before stopping the engine
> 
>  drivers/media/platform/aspeed/aspeed-video.c | 38 +++++++++++++++++++++++++-------------
>  1 file changed, 25 insertions(+), 13 deletions(-)
> 

RE: [PATCH v2 0/2] media: aspeed: order probe cleanup and last close

[Jammy Huang]

Hi Hans,

I'll test this series on our ASPEED platforms first. If everything looks good, I'll follow up with a Reviewed-by tag afterward.

Regards,
Jammy

> Eddie or Jammy,
> 
> On 10/11/2026 05:58, Myeonghun Pak wrote:
> > This revision requests the IRQ after its dependencies and synchronizes
> > IRQ/work before manual probe cleanup. The second patch fixes
> > last-close ordering and keeps the engine available until VB2 release
> completes.
> >
> > Compile-tested on ARM64 with LLVM, W=1.
> > No hardware testing.
> 
> Can you review/test this series?
> 
> I don't feel comfortable merging this without your OK.
> 
> Regards,
> 
> 	Hans
> 
> >
> > Changes in v2:
> > - Rebase onto managed reserved-memory initialization.
> > - Remove the research sentence and Assisted-by tag.
> > - Join the IRQ and delayed work before manual probe-failure cleanup.
> > - Keep VB2 release before normal last-close hardware shutdown.
> > - Drain the IRQ producer before cancelling resolution work.
> > - Balance IRQ disable/enable within stop and check STOPPED before
> power-on.
> >
> > Myeonghun Pak (2):
> >   media: aspeed: join IRQ and work before probe cleanup
> >   media: aspeed: drain IRQ work before stopping the engine
> >
> >  drivers/media/platform/aspeed/aspeed-video.c | 38
> > +++++++++++++++++++++++++-------------
> >  1 file changed, 25 insertions(+), 13 deletions(-)
> >