Re: [PATCH 2/3] dmaengine: imx-sdma: Use non-atomic __set_bit() for event_mask setup

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:

Pre-existing issues:
- [High] Event mask configuration for event_id >= 32 is broken on 64-bit architectures, causing single-event DMA channels to fail.
- [High] Lockless iteration of `sdmac->terminated` in worker thread races with list modification in `sdma_terminate_all()`, causing list corruption.
--

commit 0310ee402ce9c50c678bc6f3ddf91b00a310f763
Author: Peng Fan <peng.fan@nxp.com>

dmaengine: imx-sdma: Use non-atomic __set_bit() for event_mask setup

In sdma_set_watermarklevel_for_p2p(), the event_mask[] fields are zeroed
in sdma_config_channel() right before this function is called, and the
channel has already been disabled. There is no concurrent access to
event_mask at this point, so replace set_bit() with __set_bit().

> diff --git a/drivers/dma/imx-sdma.c b/drivers/dma/imx-sdma.c
> index 36368835a845c..63261ab6402b9 100644
> --- a/drivers/dma/imx-sdma.c
> +++ b/drivers/dma/imx-sdma.c
> @@ -1241,8 +1241,8 @@ static void sdma_set_watermarklevel_for_p2p(struct sdma_channel *sdmac)
>  	int lwml = sdmac->watermark_level & SDMA_WATERMARK_LEVEL_LWML;
>  	int hwml = (sdmac->watermark_level & SDMA_WATERMARK_LEVEL_HWML) >> 16;
>  
> -	set_bit(sdmac->event_id0 % 32, &sdmac->event_mask[1]);
> -	set_bit(sdmac->event_id1 % 32, &sdmac->event_mask[0]);
> +	__set_bit(sdmac->event_id0 % 32, &sdmac->event_mask[1]);
> +	__set_bit(sdmac->event_id1 % 32, &sdmac->event_mask[0]);
>  
>  	if (sdmac->event_id0 > 31)
>  		sdmac->watermark_level |= SDMA_WATERMARK_LEVEL_LWE;

[Severity: High]
This isn't a bug introduced by this patch, but while reviewing the event
mask configuration, I noticed a potential issue for single-event channels
on 64-bit architectures.

For single-event channels, sdma_config_channel() sets the bit directly:

drivers/dma/imx-sdma.c:sdma_config_channel() {
    ...
    __set_bit(sdmac->event_id0, sdmac->event_mask);
    ...
}

On 64-bit architectures, event_mask is an array of 64-bit words, so an
event_id of 32 sets bit 32 in event_mask[0]. Later in sdma_load_context(),
this value is assigned to context->gReg[1]:

drivers/dma/imx-sdma.c:sdma_load_context() {
    ...
    context->gReg[1] = sdmac->event_mask[0];
    ...
}

Since gReg elements are 32-bit (u32), the 64-bit value is truncated,
silently dropping the upper 32 bits and leaving gReg[1] as 0.

Will this cause single-event DMA channels with an event ID >= 32 to fail
completely on 64-bit platforms like i.MX8?

[Severity: High]
This is also a pre-existing issue, but I noticed a potential list corruption
in the terminate worker.

When a client calls dmaengine_terminate_all(), the driver clears sdmac->desc
and schedules the asynchronous worker sdma_channel_terminate_work().

If the client immediately submits a new descriptor, starts it, and terminates
again before the worker finishes, sdma_terminate_all() will acquire the
channel lock, execute vchan_get_all_descriptors(), and splice the new
descriptors into sdmac->terminated:

drivers/dma/imx-sdma.c:sdma_terminate_all() {
    ...
    vchan_get_all_descriptors(&sdmac->vc, &sdmac->terminated);
    ...
}

Concurrently, the worker thread locklessly iterates over and removes items
from sdmac->terminated:

drivers/dma/imx-sdma.c:sdma_channel_terminate_work() {
    ...
    vchan_dma_desc_free_list(&sdmac->vc, &sdmac->terminated);
    ...
}

Could this data race cause list corruption and lead to a kernel panic or
use-after-free?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260903-dma-mask-v1-0-211ffd12c236@nxp.com?part=2