Re: [PATCH v2 07/23] dmaengine: sdxi: Allocate administrative context

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

sashiko-bot@kernel.org writes:

> Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
> - [High] Incorrect error check for `dma_pool_zalloc()` allocation using the DMA handle instead of the virtual address.
> --
>
> commit 67ceee55512586a3fbe30b9f642ed7c90819e736
> Author: Nathan Lynch <nathan.lynch@amd.com>
>
> dmaengine: sdxi: Allocate administrative context
>
> This commit creates the control structure hierarchy in memory for the
> per-function administrative context. It uses devres to queue the cleanup
> since the admin context is a device-scope resource.
>
>> diff --git a/drivers/dma/sdxi/context.c b/drivers/dma/sdxi/context.c
>> new file mode 100644
>> index 0000000000000..27821cfaf0311
>> --- /dev/null
>> +++ b/drivers/dma/sdxi/context.c
>
> [ ... ]
>
>> +/* Allocate a context and its control structure hierarchy in memory. */
>> +static struct sdxi_cxt *sdxi_alloc_cxt(struct sdxi_dev *sdxi)
>> +{
>
> [ ... ]
>
>> +	cxt->cxt_ctl = dma_pool_zalloc(sdxi->cxt_ctl_pool, GFP_KERNEL,
>> +				       &cxt->cxt_ctl_dma);
>> +	if (!cxt->cxt_ctl_dma)
>> +		return NULL;
>
> Could this incorrectly treat a valid allocation as a failure?
>
> A DMA handle of 0 is a valid DMA address on many hardware platforms and
> IOMMU configurations. If dma_pool_zalloc() succeeds and returns a DMA
> address of 0, checking the DMA handle might falsely trigger this error
> path and return NULL.
>
> Should this check the returned virtual address (!cxt->cxt_ctl) instead,
> similar to how the subsequent cxt_sts and write_index allocations are
> validated?

Yes, this is a legit bug likely caused by a code completion error on my
part.

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