Re: [PATCH v5 06/17] dmaengine: sh: rz-dmac: Save the start LM descriptor

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 4 potential issue(s) to consider:
- [High] Uninitialized pointer dereference causing a kernel panic in `rz_dmac_calculate_residue_bytes_in_vd`.
- [High] Pre-existing bug: `rz_dmac_prepare_desc_for_memcpy` does not advance the hardware ring buffer tail, causing active descriptor corruption.
- [High] Pre-existing bug: Fundamentally broken residue calculation in `rz_dmac_calculate_residue_bytes_in_vd()`.
- [Medium] The patch fails to prevent the unnecessary loop iterations it claims to fix (Placebo Fix).
--

commit 4d414b3c13389eee6de9985a5483dae47daf52b4
Author: Claudiu Beznea <claudiu.beznea.uj@bp.renesas.com>

dmaengine: sh: rz-dmac: Save the start LM descriptor

Save the start LM descriptor to avoid looping through the entire
channel's LM descriptor list when computing the residue. This avoids
unnecessary iterations.

> diff --git a/drivers/dma/sh/rz-dmac.c b/drivers/dma/sh/rz-dmac.c
> index c48858b68dee6..d3926ecd63ac1 100644
> --- a/drivers/dma/sh/rz-dmac.c
> +++ b/drivers/dma/sh/rz-dmac.c

[ ... ]

> @@ -343,6 +344,8 @@ static void rz_dmac_prepare_desc_for_memcpy(struct rz_dmac_chan *channel)
>  	struct rz_dmac_desc *d = channel->desc;
>  	u32 chcfg = CHCFG_MEM_COPY;
>  
> +	d->start_lmdesc = lmdesc;
> +
>  	/* prepare descriptor */
>  	lmdesc->sa = d->src;
>  	lmdesc->da = d->dest;

This isn't a bug introduced by this commit, but I noticed
rz_dmac_prepare_desc_for_memcpy() does not appear to advance the hardware ring
buffer tail.

If multiple memcpy transfers are queued, will each new preparation overwrite
the exact same slot in the DMA hardware's ring buffer memory?

If a new memcpy transfer is prepared while a previous one is currently
executing from that slot, could this physically overwrite the active hardware
descriptor and lead to unpredictable hardware behavior and memory corruption?

[ ... ]

> @@ -693,9 +697,10 @@ rz_dmac_get_next_lmdesc(struct rz_lmdesc *base, struct rz_lmdesc *lmdesc)
>  	return next;
>  }
>  
> -static u32 rz_dmac_calculate_residue_bytes_in_vd(struct rz_dmac_chan *channel, u32 crla)
> +static u32 rz_dmac_calculate_residue_bytes_in_vd(struct rz_dmac_chan *channel,
> +						 struct rz_dmac_desc *desc, u32 crla)
>  {
> -	struct rz_lmdesc *lmdesc = channel->lmdesc.head;
> +	struct rz_lmdesc *lmdesc = desc->start_lmdesc;

Can desc->start_lmdesc be uninitialized here and cause a panic?

If a client driver prepares and submits a descriptor but delays calling
dma_async_issue_pending(), the descriptor enters ld_queue but not desc_issued.
If a prior transfer completes, rz_dmac_irq_handler_thread() attempts to
process ld_queue by calling rz_dmac_xfer_desc().

Because the new descriptor is not in desc_issued, vchan_next_desc() returns
NULL, and rz_dmac_xfer_desc() returns 0 without executing the prepare
functions, leaving start_lmdesc as NULL.

The IRQ handler then interprets the 0 return as success, moving this
unprepared descriptor to ld_active. A subsequent call to
dmaengine_tx_status() triggers rz_dmac_calculate_residue_bytes_in_vd(), which
would dereference the NULL start_lmdesc.

I see this is fixed later in the patch series by commit "dmaengine: sh:
rz-dmac: Use virt-dma APIs for channel descriptor processing" which correctly
handles vchan_next_desc() returning NULL, but could this cause bisectability
issues here?

>  	struct dma_chan *chan = &channel->vc.chan;
>  	struct rz_dmac *dmac = to_rz_dmac(chan->device);
>  	u32 residue = 0, i = 0;

Does the while (lmdesc->nxla != crla) loop below this still iterate through
the whole list, missing the intended optimization mentioned in the commit
message?

If the hardware is actively processing the first descriptor, crla is the
address of that first descriptor. Since lmdesc is initialized to
desc->start_lmdesc (which is also the first descriptor), lmdesc->nxla will
point to the second descriptor.

This will not match crla, so it appears the loop will still iterate through
all 64 elements of the ring buffer to find the descriptor preceding the
first one.

Also, a pre-existing issue unrelated to this patch: is the underlying residue
arithmetic correct for multi-segment transfers?

When processing a multi-segment scatter-gather list, the function attempts to
find the descriptor preceding the currently executing one. Once found, it
iterates forward using:

    while (lmdesc->chcfg & CHCFG_DEM)

Because it starts from the descriptor preceding the current one, if crla is
the second descriptor (L2), the loop starts at L1, adds L1->tb, and terminates
because L2's CHCFG_DEM is 0.

It then adds crtb (the bytes of the current segment). Does the final returned
value double-count the already transferred L1 and completely miss the sizes of
all future segments in the virtual transfer?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260512121219.216159-1-claudiu.beznea.uj@bp.renesas.com?part=6