Re: [PATCH v2 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:
- [High] Unnecessary use of `__packed` attribute on naturally aligned DMA structures reduces their assumed alignment to 1 byte, risking torn reads/writes.
- [Medium] `cxt_ctl_pool` has a hardcoded alignment of 64 bytes instead of `max(size, SMP_CACHE_BYTES)`, leading to potential cacheline sharing.
--

commit 12aa7b8ec291c34d27a87b91722b70b67d211aa3
Author: Nathan Lynch <nathan.lynch@amd.com>

dmaengine: sdxi: Allocate DMA pools

This commit introduces C structures for SDXI context control, context status,
and completion status blocks. It also creates a DMA pool for each of these
objects as well as write indexes, aiming to align them to avoid cacheline
sharing.

> diff --git a/drivers/dma/sdxi/device.c b/drivers/dma/sdxi/device.c
> index 6a2204ff7fdee..851e73597c22b 100644
> --- a/drivers/dma/sdxi/device.c
> +++ b/drivers/dma/sdxi/device.c

[ ... ]

> @@ -211,6 +214,43 @@ static int sdxi_fn_activate(struct sdxi_dev *sdxi)
>  	return 0;
>  }
>  
> +static int sdxi_device_init(struct sdxi_dev *sdxi)
> +{
> +	struct device *dev = sdxi->dev;
> +	size_t size, align;
> +	int err;
> +
> +	size = sizeof(__le64);
> +	align = max(size, SMP_CACHE_BYTES);
> +	sdxi->write_index_pool = dmam_pool_create("Write_Index", dev, size,
> +						  align, 0);
> +	if (!sdxi->write_index_pool)
> +		return -ENOMEM;
> +
> +	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);

The commit message states that frequently-updated objects are aligned to avoid
cacheline sharing. Does hardcoding the alignment to sizeof(struct sdxi_cxt_ctl)
risk cacheline sharing on architectures where SMP_CACHE_BYTES is 128?

Should this use max(size, SMP_CACHE_BYTES) like the other pools?

> +	if (!sdxi->cxt_ctl_pool)
> +		return -ENOMEM;

[ ... ]

> diff --git a/drivers/dma/sdxi/hw.h b/drivers/dma/sdxi/hw.h
> index df520ca7792bf..846c671c423f5 100644
> --- a/drivers/dma/sdxi/hw.h
> +++ b/drivers/dma/sdxi/hw.h

[ ... ]

> +/* SDXI 1.0 Table 3-4: Context Control (CXT_CTL) */
> +struct sdxi_cxt_ctl {
> +	__le64 ds_ring_ptr;
> +	__le32 ds_ring_sz;
> +	__u8 rsvd_0[4];
> +	__le64 cxt_sts_ptr;
> +	__le64 write_index_ptr;
> +	__u8 rsvd_1[32];
> +} __packed;

These structures appear to be naturally aligned and padded properly. Does
applying the __packed attribute without an explicit __aligned() attribute
force the compiler to assume 1-byte alignment?

Could this lead to the compiler synthesizing multiple byte-wise or narrower
memory accesses for 32-bit or 64-bit fields when they are read or written,
potentially causing torn reads or writes if the hardware lacks native
unaligned access support?

> +static_assert(sizeof(struct sdxi_cxt_ctl) == 64);
> +
> +/* SDXI 1.0 Table 3-5: Context Status (CXT_STS) */
> +struct sdxi_cxt_sts {
> +	__u8 state;
> +	__u8 misc0;
> +	__u8 rsvd_0[6];
> +	__le64 read_index;
> +} __packed;
> +static_assert(sizeof(struct sdxi_cxt_sts) == 16);
> +
> +/* SDXI 1.0 Table 6-4: CST_BLK (Completion Status Block) */
> +struct sdxi_cst_blk {
> +	__le64 signal;
> +	__le32 flags;
> +	__u8 rsvd_0[20];
> +} __packed;
> +static_assert(sizeof(struct sdxi_cst_blk) == 32);

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