RE: [PATCH] media: aspeed: acquire IRQ after reset and clock resources

[Jammy Huang]

Hi Myeonghun,

Thanks for the patch. A few comments:
1. This doesn’t apply to current mainline. aspeed_video_init() now uses 
devm_of_reserved_mem_device_init() and err_release_reserved_mem no longer exists; 
please rebase and fall through to err_unprepare_vclk. The reserved-memory rollback 
mention in the commit message should go as well.

2. The analysis is correct, but I don’t think the window is reachable in practice: 
The handler masks the status with VE_INTERRUPT_CTRL. aspeed_video_remove() →
 aspeed_video_off() zeroes the control register and clears the status before devres runs. 
On a probe failure the register is still at its reset default of 0.

2. Please drop the “ongoing research” sentence from the commit message, and name 
the actual tool in Assisted-by, or just remove it.
> 
> The threaded IRQ uses the reset and clock resources, but it is registered before
> those managed handles are acquired. Reverse devres cleanup can release the
> handles before the IRQ action has been removed and joined.
> 
> On a resolution-change interrupt, aspeed_video_irq() calls
> aspeed_video_irq_res_change(), which invokes aspeed_video_reset() and
> accesses video->reset through reset_control_assert() and
> reset_control_deassert().
> 
> Request the IRQ after reset, clock and JPEG-table initialization so managed IRQ
> release precedes release of those handles. Add rollback for the JPEG allocation,
> reserved-memory association and prepared clocks if the later IRQ request fails.
> 
> This issue was identified as part of our ongoing research while reviewing
> kernel code.
> 
> Fixes: d2b4387f3bdf ("media: platform: Add Aspeed Video Engine driver")
> Assisted-by: LLM
> Co-developed-by: Ijae Kim <ae878000@xxxxxxxxx>
> Signed-off-by: Ijae Kim <ae878000@xxxxxxxxx>
> Signed-off-by: Myeonghun Pak <mhun512@xxxxxxxxx>
> ---
> This fixes managed handle lifetime only.
> 
> Compile-tested with Clang 22.1.2, W=1. No runtime or hardware testing.
> 
> Base: 0fcb05b72344f40806cbe810de8872f1698bf053
> 
>  drivers/media/platform/aspeed/aspeed-video.c | 19 +++++++++++--------
>  1 file changed, 11 insertions(+), 8 deletions(-)
> 
> diff --git a/drivers/media/platform/aspeed/aspeed-video.c
> b/drivers/media/platform/aspeed/aspeed-video.c
> index a292275f6b7b..234001a93ad4 100644
> --- a/drivers/media/platform/aspeed/aspeed-video.c
> +++ b/drivers/media/platform/aspeed/aspeed-video.c
> @@ -2232,14 +2232,6 @@ static int aspeed_video_init(struct aspeed_video
> *video)
>  		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 +2278,19 @@
> static int aspeed_video_init(struct aspeed_video *video)
> 
>  	aspeed_video_init_jpeg_table(video->jpeg.virt, video->yuv420);
> 
> +	/* Release the IRQ before the reset and clock handles on devres unwind.
> */
> +	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;
> +	}
> +	dev_info(video->dev, "irq %d\n", irq);
> +
>  	return 0;
> 
> +err_free_jpeg:
> +	aspeed_video_free_buf(video, &video->jpeg);
>  err_release_reserved_mem:
>  	of_reserved_mem_device_release(dev);
>  	clk_unprepare(video->vclk);