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

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

New issues:
- [Low] Typo in the hardware data structure name for the Apple M1 (t8103) SoC.

Pre-existing issues:
- [High] Tasklets are not killed on device unbind, leading to a use-after-free when scheduled tasklets run after driver data is freed.
--

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

dmaengine: apple-admac: Add M3 generation ADMACs

This commit adds support for the ADMACs found on Apple t8122 and t603x SoCs.
It introduces additional register writes required for correct operation on
these platforms, and updates the device match table accordingly.

> diff --git a/drivers/dma/apple-admac.c b/drivers/dma/apple-admac.c
> index 14a5ee14a4815..7ee9a59081c1a 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 is a pre-existing issue, but does this code leave scheduled tasklets
running after the driver is unbound?

If the device is unbound via sysfs, admac_remove() is called and frees the
IRQ, but it does not appear to stop scheduled tasklets for its channels.
If an interrupt fires just before free_irq() and schedules a tasklet,
the software tasklet won't be canceled.

Since the driver uses devm-allocated memory for struct admac_data, that
memory is automatically freed after admac_remove() returns. If the tasklet
runs after this, could it lead to a use-after-free? Should we be calling
tasklet_kill() for all active channels before returning?

> +static const struct admac_hw admac_t8013_hw = {

[Severity: Low]
This isn't a bug, but should this be named admac_t8103_hw to match the
apple,t8103-admac compatible string it targets?

> +	.set_unk28 = false,
> +};
> +
> +static const struct admac_hw admac_t8122_hw = {
> +	.set_unk28 = true,
> +};
> +
>  static const struct of_device_id admac_of_match[] = {
> -	{ .compatible = "apple,t8103-admac", },
> -	{ .compatible = "apple,admac", },
> +	{ .compatible = "apple,t8122-admac", .data = &admac_t8122_hw },
> +	{ .compatible = "apple,t8103-admac", .data = &admac_t8013_hw },
> +	{ .compatible = "apple,admac", .data = &admac_t8013_hw },
>  	{ }
>  };
>  MODULE_DEVICE_TABLE(of, admac_of_match);

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