Re: [PATCH v2 05/23] dmaengine: sdxi: Configure context tables

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
- [Medium] Premature programming of the L2 table base address in the hardware register, leading to a dangling DMA pointer on error paths.
- [Medium] Unnecessary and potentially harmful `__packed` attribute on naturally aligned DMA structures.
--

commit 0a4d9e9c9ca24875b4f7b4512a3bf669c25c0595
Author: Nathan Lynch <nathan.lynch@amd.com>

dmaengine: sdxi: Configure context tables

This commit allocates and configures the two-level context tracking tables (L1
and L2) for the SDXI driver. It allocates enough space for context IDs 0-127
and caps the maximum contexts accordingly, using devres-managed allocations to
simplify cleanup.

> 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

[ ... ]

> @@ -172,6 +182,32 @@ static int sdxi_fn_activate(struct sdxi_dev *sdxi)
>  			   FIELD_GET(SDXI_MMIO_CAP1_OPB_000_CAP, cap1));
>  	sdxi_write64(sdxi, SDXI_MMIO_CTL2, ctl2);
>  
> +	/* SDXI 1.0 4.1.8.2 Context Level 2 Table Setup */
> +	sdxi->L2_table = dmam_alloc_coherent(sdxi->dev,
> +					     sizeof(*sdxi->L2_table),
> +					     &sdxi->L2_dma, GFP_KERNEL);
> +	if (!sdxi->L2_table)
> +		return -ENOMEM;
> +
> +	cxt_l2 = FIELD_PREP(SDXI_MMIO_CXT_L2_PTR, sdxi->L2_dma >> ilog2(SZ_4K));
> +	sdxi_write64(sdxi, SDXI_MMIO_CXT_L2, cxt_l2);
> +
> +	/* SDXI 1.0 4.1.8.3 Context Level 1 Table Setup */
> +	sdxi->L1_table = dmam_alloc_coherent(sdxi->dev,
> +					     sizeof(*sdxi->L1_table),
> +					     &sdxi->L1_dma, GFP_KERNEL);
> +	if (!sdxi->L1_table)
> +		return -ENOMEM;

If dmam_alloc_coherent() fails for L1_table, we return -ENOMEM. The devres
framework will then free L2_table, but does this leave a dangling physical
pointer in the hardware SDXI_MMIO_CXT_L2 register?

Could this lead to a DMA use-after-free regression if the hardware incorrectly
resumes or is later manipulated? Should we defer writing to SDXI_MMIO_CXT_L2
until both tables are fully allocated and linked?

> diff --git a/drivers/dma/sdxi/hw.h b/drivers/dma/sdxi/hw.h
> --- /dev/null
> +++ b/drivers/dma/sdxi/hw.h

[ ... ]

> +/* SDXI 1.0 Table 3-2: Context Level 2 Table Entry (CXT_L2_ENT) */
> +struct sdxi_cxt_L2_ent {
> +	__le64 lv01_ptr;
> +#define SDXI_CXT_L2_ENT_VL       BIT_ULL(0)
> +#define SDXI_CXT_L2_ENT_LV01_PTR GENMASK_ULL(63, 12)
> +} __packed;

Are the __packed attributes necessary here? Since these structures contain
naturally aligned fields and their sizes are multiples of their largest
members, they shouldn't require compiler padding.

Could packing them cause the compiler to assume pointers are unaligned and
generate byte-by-byte accesses instead of native loads and stores?

For structures in DMA-coherent memory, this might break hardware atomicity
or trigger architectural alignment faults on some platforms.

[ ... ]

> +/* SDXI 1.0 Table 3-3: Context Level 1 Table Entry (CXT_L1_ENT) */
> +struct sdxi_cxt_L1_ent {
> +	__le64 cxt_ctl_ptr;
> +	__le64 akey_ptr;
> +	__le32 misc0;
> +	__le32 opb_000_enb;
> +	__u8 rsvd_0[8];
> +} __packed;

Does this struct have the same __packed regression as sdxi_cxt_L2_ent?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260511-sdxi-base-v2-0-889cfed17e3f@amd.com?part=5