Re: [PATCH 1/2] media: amd: isp4: hold buf_list_lock while sending buffers at stream start

[Nirujogi, Pratap]

On 10/26/2026 2:24 PM, Seifelden Abdelmonem wrote:
> [You don't often get email from saif.elden.hatim@xxxxxxxxx. Learn why this is important at https://aka.ms/LearnAboutSenderIdentification ]
>
> Caution: This message originated from an External Source. Use proper caution when opening attachments, clicking links, or responding.
>
>
> isp4vid_qops_start_streaming() walks isp_vdev->buf_list without
> buf_list_lock and passes each buffer to isp4sd_ioc_send_img_buf().
> Every other access to buf_list holds that lock.
>
> vb2 serialises the walk against buf_queue(), but not against the
> firmware response thread. Once ISP4SD_MIN_BUF_CNT_BEF_START_STREAM
> buffers have been sent, isp4sd_ioc_send_img_buf() also sends
> START_STREAM, so the firmware can complete buffers while the walk is
> still running. isp4vid_handle_frame_done() then list_del()s the
> completed buffer from buf_list. If that buffer is the current entry of
> the walk, list_for_each_entry() follows the poisoned next pointer:
>
>    Oops: general protection fault, maybe for address 0xffff8e6944d97060
>    RIP: 0010:isp4if_alloc_buffer_node+0x37/0x50 [amd_isp4_capture]
>    RBX: dead0000000000b8 ... RSI: dead0000000000b8
>    Call Trace:
>     isp4sd_ioc_send_img_buf
>     isp4vid_qops_start_streaming
>     vb2_start_streaming
>     vb2_core_streamon
>
> The oopsing task holds the queue lock, so the video device can no
> longer be closed and the module cannot be unloaded until reboot.
>
> This was hit on a board that is not supported upstream yet, with an
> out-of-tree change that made one isp4sd_ioc_send_img_buf() call take
> over a second. With the stock driver the window is much shorter, but
> the list is still modified concurrently without the lock.
Reviewed-by: Pratap Nirujogi <pratap.nirujogi@xxxxxxx>

Thanks for submitting the fix. It appears we were fortunate to have 
passed the tests, as isp4sd_ioc_send_img_buf() never took this long to 
reproduce the issue while the stream is running. I’m curious about the 
sequence you are using to reproduce this issue. Would you mind sharing 
the details of the sequence and steps involved?

Thanks,
Pratap

> Hold buf_list_lock across the walk. This adds the lock order
> buf_list_lock -> ops_mutex. No path takes them in the reverse order:
> isp4vid_handle_frame_done() runs in the response thread without
> ops_mutex. isp4sd_ioc_send_img_buf() only queues commands and does not
> wait for a firmware response, so the response thread is at most
> delayed until the initial buffers have been sent.
>
> Fixes: 2ccf48af2270 ("media: platform: amd: isp4 video node and buffers handling added")
> Signed-off-by: Seifelden Abdelmonem <saif.elden.hatim@xxxxxxxxx>
> ---
> Testing and AI use: see the cover letter.
>
>   drivers/media/platform/amd/isp4/isp4_video.c | 8 +++++---
>   1 file changed, 5 insertions(+), 3 deletions(-)
>
> diff --git a/drivers/media/platform/amd/isp4/isp4_video.c b/drivers/media/platform/amd/isp4/isp4_video.c
> index 856a2a0b4..58ea8d6ac 100644
> --- a/drivers/media/platform/amd/isp4/isp4_video.c
> +++ b/drivers/media/platform/amd/isp4/isp4_video.c
> @@ -567,9 +567,11 @@ static int isp4vid_qops_start_streaming(struct vb2_queue *vq,
>                  }
>          }
>
> -       list_for_each_entry(isp4vid_buf, &isp_vdev->buf_list, list)
> -               isp4sd_ioc_send_img_buf(isp_vdev->isp_sdev,
> -                                       &isp4vid_buf->img_buf);
> +       scoped_guard(mutex, &isp_vdev->buf_list_lock) {
> +               list_for_each_entry(isp4vid_buf, &isp_vdev->buf_list, list)
> +                       isp4sd_ioc_send_img_buf(isp_vdev->isp_sdev,
> +                                               &isp4vid_buf->img_buf);
> +       }
>
>          isp_vdev->stream_started = true;
>
> --
> 2.56.0