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

[Nathan Lynch <nathan.lynch@amd.com>]

sashiko-bot@kernel.org writes:
> 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?

No, sdxi_cxt_ctl is not one of the control structures we expect to be
frequently updated, but I suppose this could be made more clear in the
commit message or comments.

>
>> +	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?

Valid -- I'll add the aligned attributes throughout (I want to continue
using packed to prevent inadvertent padding).


>> +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