Re: [PATCH v2 2/2] dmaengine: apple-admac: Add M3 generation ADMACs

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

Pre-existing issues:
- [High] The driver schedules a tasklet for DMA channel completion but fails to kill it upon driver unbind, leading to a Use-After-Free.
- [High] Missing memory barrier between CPU writes to the DMA buffer and triggering the hardware via MMIO.
--

--- Patch [2]: [PATCH v2 2/2] dmaengine: apple-admac: Add M3 generation ADMACs ---
commit 3e4665bdfbbc14988e79c929d83bff445def0885
Author: Sasha Finkelstein <[email protected]>

dmaengine: apple-admac: Add M3 generation ADMACs
    
The admacs present on t8122 and t603x SoCs need additional writes in
order to operate correctly. The exact purpose of this register
is unknown

> diff --git a/drivers/dma/apple-admac.c b/drivers/dma/apple-admac.c
> index 14a5ee14a4815..72ff677c8ce54 100644
> --- a/drivers/dma/apple-admac.c
> +++ b/drivers/dma/apple-admac.c
[ ... ]
> @@ -935,9 +954,18 @@ static void admac_remove(struct platform_device *pdev)
>  	reset_control_rearm(ad->rstc);
>  }

[Severity: High]
This isn't a bug introduced by this patch, but I noticed the driver schedules
a tasklet for DMA channel completion but fails to kill it upon driver unbind.

Does this leave the code open to a use-after-free? If an interrupt fires just
before free_irq() and schedules the tasklet, the tasklet may run after
admac_remove() returns and the devm_kzalloc managed memory (struct admac_data)
is freed.

Could we add a call to tasklet_kill() during the teardown sequence?

[Severity: High]
This is another pre-existing issue not touched by this patch, but I noticed
missing memory barriers when the hardware is started via MMIO.

Looking at admac_start_chan() and admac_cyclic_write_one_desc():

drivers/dma/apple-admac.c:admac_start_chan() {
    ...
	switch (admac_chan_direction(adchan->no)) {
	case DMA_MEM_TO_DEV:
		writel_relaxed(startbit, ad->base + REG_TX_START);
		break;
    ...
}

drivers/dma/apple-admac.c:admac_cyclic_write_one_desc() {
    ...
	writel_relaxed(lower_32_bits(addr), ad->base + REG_DESC_WRITE(channo));
	writel_relaxed(upper_32_bits(addr), ad->base + REG_DESC_WRITE(channo));
    ...
}

Could CPU writes to the DMA buffer be delayed and become visible to the DMA
controller only after it starts fetching data? 

Since writel_relaxed() does not include a memory barrier on ARM64 architectures
and spin_lock only provides ACQUIRE semantics, prior memory stores to the DMA
buffer might be reordered after the MMIO write. Does this require a memory
barrier to prevent transmitting stale data?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/[email protected]?part=2