linux-kernel - Re: [PATCH v5 14/14] media: ti: j721e-csi2rx: Wait for the last drain completion

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <175707067154.8095.10777597561482124941@freya>
Date: Fri, 05 Sep 2025 16:41:11 +0530
From: Jai Luthra <jai.luthra@...asonboard.com>
To: Rishikesh Donadkar <r-donadkar@...com>, jai.luthra@...ux.dev, laurent.pinchart@...asonboard.com, mripard@...nel.org
Cc: r-donadkar@...com, y-abhilashchandra@...com, devarsht@...com, vaishnav.a@...com, s-jain1@...com, vigneshr@...com, mchehab@...nel.org, robh@...nel.org, krzk+dt@...nel.org, conor+dt@...nel.org, sakari.ailus@...ux.intel.com, hverkuil-cisco@...all.nl, tomi.valkeinen@...asonboard.com, changhuang.liang@...rfivetech.com, jack.zhu@...rfivetech.com, linux-kernel@...r.kernel.org, linux-media@...r.kernel.org, devicetree@...r.kernel.org
Subject: Re: [PATCH v5 14/14] media: ti: j721e-csi2rx: Wait for the last drain completion

Quoting Rishikesh Donadkar (2025-08-25 19:55:22)
> dmaengine_terminate_sync() causes all activity for the DMA channel to be
> stopped, and may discard data in the DMA FIFO which hasn't been fully
> transferred. No callback functions will be called for any
> incomplete transfers[1].
> 
> In multistream use case, calling dmaengine_terminate_sync() immediately
> after issuing the last drain transaction will result in no callback
> for the last drain cycle.
> 
> Implement complete callback for the last drain cycle to make sure that
> the last drain has completed properly, this will ensure that stale data
> is not left out in the HW FIFO.
> 
> [1] : https://docs.kernel.org/driver-api/dmaengine/client.html
> 
> Signed-off-by: Rishikesh Donadkar <r-donadkar@...com>
> ---
>  drivers/media/platform/ti/j721e-csi2rx/j721e-csi2rx.c | 11 +++++++++++
>  1 file changed, 11 insertions(+)
> 
> diff --git a/drivers/media/platform/ti/j721e-csi2rx/j721e-csi2rx.c b/drivers/media/platform/ti/j721e-csi2rx/j721e-csi2rx.c
> index 4ac6a76b9409..520ee05eb5b4 100644
> --- a/drivers/media/platform/ti/j721e-csi2rx/j721e-csi2rx.c
> +++ b/drivers/media/platform/ti/j721e-csi2rx/j721e-csi2rx.c
> @@ -62,6 +62,7 @@
>  #define TI_CSI2RX_MAX_PADS             (1 + TI_CSI2RX_MAX_SOURCE_PADS)
>  
>  #define DRAIN_BUFFER_SIZE              SZ_32K
> +#define DRAIN_TIMEOUT_MS               50

This was dropped in the previous patch, and now reintroduce.

IIUC this patch is fixing a bug introduced by the previous one, so it's
better to squash them together, and have a combined commit description that
goes over this end-of-stream case, as well as why continuous drain was
needed for mid-stream scenario.

>  
>  #define CSI2RX_BRIDGE_SOURCE_PAD       1
>  
> @@ -137,6 +138,7 @@ struct ti_csi2rx_dev {
>                 size_t                  len;
>         } drain;
>         bool                            vc_cached;
> +       struct completion drain_complete;

Why is the struct completion shared amongst all contexts in the
ti_csi2rx_dev structure?

What happens when two streams are stopped together?

>  };
>  
>  static inline struct ti_csi2rx_dev *to_csi2rx_dev(struct v4l2_subdev *sd)
> @@ -624,12 +626,14 @@ static void ti_csi2rx_setup_shim(struct ti_csi2rx_ctx *ctx)
>  static void ti_csi2rx_drain_callback(void *param)
>  {
>         struct ti_csi2rx_ctx *ctx = param;
> +       struct ti_csi2rx_dev *csi = ctx->csi;
>         struct ti_csi2rx_dma *dma = &ctx->dma;
>         unsigned long flags;
>  
>         spin_lock_irqsave(&dma->lock, flags);
>  
>         if (dma->state == TI_CSI2RX_DMA_STOPPED) {
> +               complete(&csi->drain_complete);

Please also add comment above this if case explaining why we need to wait
for the drain to complete when dma->state == STOPPED, which is set by the
driver elsewhere when streamoff was requested, and no more data will be
coming in from the source.

>                 spin_unlock_irqrestore(&dma->lock, flags);
>                 return;
>         }
> @@ -774,6 +778,7 @@ static int ti_csi2rx_start_dma(struct ti_csi2rx_ctx *ctx,
>  static void ti_csi2rx_stop_dma(struct ti_csi2rx_ctx *ctx)
>  {
>         struct ti_csi2rx_dma *dma = &ctx->dma;
> +       struct ti_csi2rx_dev *csi = ctx->csi;
>         enum ti_csi2rx_dma_state state;
>         unsigned long flags;
>         int ret;
> @@ -783,6 +788,8 @@ static void ti_csi2rx_stop_dma(struct ti_csi2rx_ctx *ctx)
>         dma->state = TI_CSI2RX_DMA_STOPPED;
>         spin_unlock_irqrestore(&dma->lock, flags);
>  
> +       init_completion(&csi->drain_complete);
> +
>         if (state != TI_CSI2RX_DMA_STOPPED) {
>                 /*
>                  * Normal DMA termination does not clean up pending data on
> @@ -796,6 +803,10 @@ static void ti_csi2rx_stop_dma(struct ti_csi2rx_ctx *ctx)
>                                  "Failed to drain DMA. Next frame might be bogus\n");
>         }
>  
> +       if (!wait_for_completion_timeout(&csi->drain_complete,
> +                                        msecs_to_jiffies(DRAIN_TIMEOUT_MS)))
> +               dev_dbg(csi->dev, "DMA transfer timed out for drain buffer\n");
> +
>         ret = dmaengine_terminate_sync(ctx->dma.chan);
>         if (ret)
>                 dev_err(ctx->csi->dev, "Failed to stop DMA: %d\n", ret);
> -- 
> 2.34.1
>

Thanks,
    Jai