linux-kernel - Re: [PATCH] dmaengine: mmp_pdma: fix DMA mask handling

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives
Hash Suite for Android: free password hash cracker in your pocket
[<prev] [next>] [<thread-prev] [day] [month] [year] [list]
Message-ID: <20250930221001.GA66006@ax162>
Date: Tue, 30 Sep 2025 15:10:01 -0700
From: Nathan Chancellor <nathan@...nel.org>
To: Guodong Xu <guodong@...cstar.com>, Vinod Koul <vkoul@...nel.org>
Cc: Nick Desaulniers <nick.desaulniers+lkml@...il.com>,
	Bill Wendling <morbo@...gle.com>,
	Justin Stitt <justinstitt@...gle.com>, Yixun Lan <dlan@...too.org>,
	dmaengine@...r.kernel.org, linux-kernel@...r.kernel.org,
	llvm@...ts.linux.dev, linux-riscv@...ts.infradead.org,
	spacemit@...ts.linux.dev, elder@...cstar.com,
	Naresh Kamboju <naresh.kamboju@...aro.org>,
	Arnd Bergmann <arnd@...db.de>
Subject: Re: [PATCH] dmaengine: mmp_pdma: fix DMA mask handling

On Thu, Sep 18, 2025 at 10:27:27PM +0800, Guodong Xu wrote:
> The driver's existing logic for setting the DMA mask for "marvell,pdma-1.0"
> was flawed. It incorrectly relied on pdev->dev->coherent_dma_mask instead
> of declaring the hardware's fixed addressing capability. A cleaner and
> more correct approach is to define the mask directly based on the hardware
> limitations.
> 
> The MMP/PXA PDMA controller is a 32-bit DMA engine. This is supported by
> datasheets and various dtsi files for PXA25x, PXA27x, PXA3xx, and MMP2,
> all of which are 32-bit systems.
> 
> This patch simplifies the driver's logic by replacing the 'u64 dma_mask'
> field with a simpler 'u32 dma_width' to store the addressing capability
> in bits. The complex if/else block in probe() is then replaced with a
> single, clear call to dma_set_mask_and_coherent(). This sets a fixed
> 32-bit DMA mask for "marvell,pdma-1.0" and a 64-bit mask for
> "spacemit,k1-pdma," matching each device's hardware capabilities.
> 
> Finally, this change also works around a specific build error encountered
> with clang-20 on x86_64 allyesconfig. The shift-count-overflow error is
> caused by a known clang compiler issue where the DMA_BIT_MASK(n) macro's
> ternary operator is not correctly evaluated in static initializers. By
> moving the macro's evaluation into the probe() function, the driver avoids
> this compiler bug.
> 
> Fixes: 5cfe585d8624 ("dmaengine: mmp_pdma: Add SpacemiT K1 PDMA support with 64-bit addressing")
> Reported-by: Naresh Kamboju <naresh.kamboju@...aro.org>
> Closes: https://lore.kernel.org/lkml/CA+G9fYsPcMfW-e_0_TRqu4cnwqOqYF3aJOeKUYk6Z4qRStdFvg@mail.gmail.com
> Suggested-by: Arnd Bergmann <arnd@...db.de>
> Signed-off-by: Guodong Xu <guodong@...cstar.com>

Tested-by: Nathan Chancellor <nathan@...nel.org> # build

It would be great if this could be picked up before the 6.18 DMA pull
request so that I do not have to patch our CI to avoid this issue.

>  drivers/dma/mmp_pdma.c | 20 ++++++++------------
>  1 file changed, 8 insertions(+), 12 deletions(-)
> 
> diff --git a/drivers/dma/mmp_pdma.c b/drivers/dma/mmp_pdma.c
> index d07229a748868b8115892c63c54c16130d88e326..86661eb3cde1ff6d6d8f02b6f0d4142878b5a890 100644
> --- a/drivers/dma/mmp_pdma.c
> +++ b/drivers/dma/mmp_pdma.c
> @@ -152,8 +152,8 @@ struct mmp_pdma_phy {
>   *
>   * Controller Configuration:
>   * @run_bits:   Control bits in DCSR register for channel start/stop
> - * @dma_mask:   DMA addressing capability of controller. 0 to use OF/platform
> - *              settings, or explicit mask like DMA_BIT_MASK(32/64)
> + * @dma_width:  DMA addressing width in bits (32 or 64). Determines the
> + *              DMA mask capability of the controller hardware.
>   */
>  struct mmp_pdma_ops {
>  	/* Hardware Register Operations */
> @@ -173,7 +173,7 @@ struct mmp_pdma_ops {
>  
>  	/* Controller Configuration */
>  	u32 run_bits;
> -	u64 dma_mask;
> +	u32 dma_width;
>  };
>  
>  struct mmp_pdma_device {
> @@ -1172,7 +1172,7 @@ static const struct mmp_pdma_ops marvell_pdma_v1_ops = {
>  	.get_desc_src_addr = get_desc_src_addr_32,
>  	.get_desc_dst_addr = get_desc_dst_addr_32,
>  	.run_bits = (DCSR_RUN),
> -	.dma_mask = 0,			/* let OF/platform set DMA mask */
> +	.dma_width = 32,
>  };
>  
>  static const struct mmp_pdma_ops spacemit_k1_pdma_ops = {
> @@ -1185,7 +1185,7 @@ static const struct mmp_pdma_ops spacemit_k1_pdma_ops = {
>  	.get_desc_src_addr = get_desc_src_addr_64,
>  	.get_desc_dst_addr = get_desc_dst_addr_64,
>  	.run_bits = (DCSR_RUN | DCSR_LPAEEN),
> -	.dma_mask = DMA_BIT_MASK(64),	/* force 64-bit DMA addr capability */
> +	.dma_width = 64,
>  };
>  
>  static const struct of_device_id mmp_pdma_dt_ids[] = {
> @@ -1314,13 +1314,9 @@ static int mmp_pdma_probe(struct platform_device *op)
>  	pdev->device.directions = BIT(DMA_MEM_TO_DEV) | BIT(DMA_DEV_TO_MEM);
>  	pdev->device.residue_granularity = DMA_RESIDUE_GRANULARITY_DESCRIPTOR;
>  
> -	/* Set DMA mask based on ops->dma_mask, or OF/platform */
> -	if (pdev->ops->dma_mask)
> -		dma_set_mask(pdev->dev, pdev->ops->dma_mask);
> -	else if (pdev->dev->coherent_dma_mask)
> -		dma_set_mask(pdev->dev, pdev->dev->coherent_dma_mask);
> -	else
> -		dma_set_mask(pdev->dev, DMA_BIT_MASK(64));
> +	/* Set DMA mask based on controller hardware capabilities */
> +	dma_set_mask_and_coherent(pdev->dev,
> +				  DMA_BIT_MASK(pdev->ops->dma_width));
>  
>  	ret = dma_async_device_register(&pdev->device);
>  	if (ret) {
> 
> ---
> base-commit: cc0bacac6de7763a038550cf43cb94634d8be9cd
> change-id: 20250904-mmp-pdma-simplify-dma-addressing-f6aef03e07c3
> 
> Best regards,
> -- 
> Guodong Xu <guodong@...cstar.com>
>