Re: [PATCH v3 06/23] dmaengine: sdxi: Allocate DMA pools

[sashiko-bot@kernel.org]

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

New issues:
- [Medium] The DMA pool for Context Control (CXT_CTL) blocks is hardcoded to a 64-byte alignment, causing cacheline false sharing on architectures with larger cache lines.

Pre-existing issues:
- [High] The driver programs hardware registers with physical memory addresses but lacks a hardware quiesce/cleanup path on probe failure, leading to a structural DMA Use-After-Free (UAF).
--

commit 9cf4c3500101cb5a3bcbf3a8b8a42a1772787cc2
Author: Nathan Lynch <nathan.lynch@amd.com>

dmaengine: sdxi: Allocate DMA pools

This commit introduces C structures for context control, context status,
and completion status blocks. It creates DMA pools for each during SDXI
function initialization to ensure proper alignment and avoid cacheline
sharing.

> diff --git a/drivers/dma/sdxi/device.c b/drivers/dma/sdxi/device.c
> --- a/drivers/dma/sdxi/device.c
> +++ b/drivers/dma/sdxi/device.c
[ ... ]
> @@ -218,6 +221,43 @@ static int sdxi_fn_activate(struct sdxi_dev *sdxi)
>  	return 0;
>  }
>  
> +static int sdxi_device_init(struct sdxi_dev *sdxi)
> +{
[ ... ]
> +	size = sizeof(struct sdxi_cxt_sts);
> +	align = max(size, SMP_CACHE_BYTES);
> +	sdxi->cxt_sts_pool = dmam_pool_create("CXT_STS", dev, size, align, 0);
> +	if (!sdxi->cxt_sts_pool)
> +		return -ENOMEM;
> +
> +	size = align = sizeof(struct sdxi_cxt_ctl);
> +	sdxi->cxt_ctl_pool = dmam_pool_create("CXT_CTL", dev, size, align, 0);

[Severity: Medium]
Does this fixed alignment introduce false sharing on architectures with
larger cache lines?

The commit message states the goal is to avoid cacheline sharing for
frequently-updated objects, but setting alignment strictly to the struct
size (64 bytes) means the allocator could pack two CXT_CTL objects into
a single 128-byte cache line on platforms where SMP_CACHE_BYTES > 64.

Should this use max(size, SMP_CACHE_BYTES) similar to how the other
pools in this function are aligned?

> +	if (!sdxi->cxt_ctl_pool)
> +		return -ENOMEM;
[ ... ]
> +	err = sdxi_fn_activate(sdxi);
> +	if (err)
> +		return err;
> +
> +	return 0;
> +}
> +
>  int sdxi_register(struct device *dev, const struct sdxi_bus_ops *ops)
>  {
>  	struct sdxi_dev *sdxi;
> @@ -235,7 +275,7 @@ int sdxi_register(struct device *dev, const struct sdxi_bus_ops *ops)
>  	if (err)
>  		return err;
>  
> -	return sdxi_fn_activate(sdxi);
> +	return sdxi_device_init(sdxi);
>  }

[Severity: High]
This isn't a bug introduced by this patch, but does the probe sequence
leave the hardware active if an error occurs here?

Looking at sdxi_fn_activate(), it programs hardware registers with the
physical addresses of devres-managed memory:

sdxi_fn_activate() {
    ...
    cxt_l2 = FIELD_PREP(SDXI_MMIO_CXT_L2_PTR, sdxi->L2_dma >> ilog2(SZ_4K));
    sdxi_write64(sdxi, SDXI_MMIO_CXT_L2, cxt_l2);
    ...
}

If a subsequent step fails (like sdxi_dev_start timing out), the function
returns an error and sdxi_device_init() propagates it back up to
sdxi_register().

Because devres actions execute in reverse order, the dmam_alloc_coherent
memory is freed before the PCI bus master bit is cleared. Can the active
device perform a DMA into freed memory during this window?

Additionally, if the driver is re-probed, pci_set_master() is called
before sdxi_fn_activate() can stop the device, which might cause the
hardware to immediately resume DMA to the old freed addresses.

Is there a need for a cleanup path on failure that explicitly stops the
device?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260605-sdxi-base-v3-0-4d38ca2bdffe@amd.com?part=6