All of lore.kernel.org
 help / color / mirror / Atom feed
From: Leon Romanovsky <leon@kernel.org>
To: Jianpeng Chang <jianpeng.chang.cn@windriver.com>
Cc: m.szyprowski@samsung.com, robin.murphy@arm.com,
	kbusch@kernel.org, jgg@ziepe.ca, iommu@lists.linux.dev,
	linux-kernel@vger.kernel.org, stable@vger.kernel.org
Subject: Re: [PATCH v2] dma-mapping: move dma_map_resource() sanity check into debug code
Date: Mon, 11 May 2026 20:56:41 +0300	[thread overview]
Message-ID: <20260511175641.GL15586@unreal> (raw)
In-Reply-To: <20260511083133.1096171-1-jianpeng.chang.cn@windriver.com>

On Mon, May 11, 2026 at 04:31:33PM +0800, Jianpeng Chang wrote:
> dma_map_resource() uses pfn_valid() to ensure the range is not RAM.
> However, pfn_valid() only checks for availability of the memory map for
> a PFN but it does not ensure that the PFN is actually backed by RAM. On
> ARM64 with SPARSEMEM (128MB section granularity), MMIO addresses that
> share a section with RAM will falsely trigger the WARN_ON_ONCE and cause
> dma_map_resource() to return DMA_MAPPING_ERROR.
> 
> This causes a WARNING on Raspberry Pi 4 during spi_bcm2835 probe because
> the SPI FIFO register (0xfe204004) falls in the same sparsemem section
> as the end of RAM (0xf8000000-0xfbffffff), both in section 31
> (0xf8000000-0xffffffff).
> 
> Move the sanity check from dma_map_resource() into debug_dma_map_phys()
> and replace the unreliable pfn_valid() with pfn_valid() &&
> !PageReserved(), which correctly identifies actual usable RAM without
> false positives for MMIO regions that happen to have struct pages.
> 
> Since dma_map_resource() is dma_map_phys(DMA_ATTR_MMIO), the check
> applies equally to both APIs. Any non-reserved page represents kernel
> memory to a sufficient degree that using DMA_ATTR_MMIO on it is almost
> certainly wrong and risks breaking coherency on non-coherent platforms.
> ZONE_DEVICE pages used for PCI P2P DMA (MEMORY_DEVICE_PCI_P2PDMA) have
> PageReserved set, so they will not trigger a false positive.
> 
> The check is now a WARN_ONCE that no longer blocks the mapping, since
> being unobtrusive is more important than being exhaustive for what is
> merely a debug sanity check.
> 
> Fixes: f7326196a781 ("dma-mapping: export new dma_*map_phys() interface")
> Signed-off-by: Jianpeng Chang <jianpeng.chang.cn@windriver.com>
> ---
> v2:
>   - move check to debug_dma_map_phys and replace pfn_valid() with
>     pfn_valid() && !PageReserved() as Robin suggested.
>   - update commit message to explain why PageReserved is safe for
>     ZONE_DEVICE PCI_P2PDMA pages
> v1: https://lore.kernel.org/all/20260507032120.4072283-1-jianpeng.chang.cn@windriver.com/
>  kernel/dma/debug.c   | 9 +++++++++
>  kernel/dma/mapping.c | 4 ----
>  2 files changed, 9 insertions(+), 4 deletions(-)
> 
> diff --git a/kernel/dma/debug.c b/kernel/dma/debug.c
> index 1a725edbbbf6..180aa2c930b5 100644
> --- a/kernel/dma/debug.c
> +++ b/kernel/dma/debug.c
> @@ -1239,6 +1239,15 @@ void debug_dma_map_phys(struct device *dev, phys_addr_t phys, size_t size,
>  	if (dma_mapping_error(dev, dma_addr))
>  		return;
>  
> +	if (attrs & DMA_ATTR_MMIO) {
> +		unsigned long pfn = PHYS_PFN(phys);
> +
> +		WARN_ONCE(pfn_valid(pfn) && !PageReserved(pfn_to_page(pfn)),
> +			  "dma_map_resource called for RAM address %pa\n",
> +			  &phys);
> +		return;

I’m not comfortable with this return statement. It effectively disables  
DMA debugging for any caller that uses dma_map_phys(..., DMA_ATTR_MMIO),
like dma-buf, block layer, HMM, e.t.c

Thanks

> +	}
> +
>  	entry = dma_entry_alloc();
>  	if (!entry)
>  		return;
> diff --git a/kernel/dma/mapping.c b/kernel/dma/mapping.c
> index 23ed8eb9233e..e6b07f160d20 100644
> --- a/kernel/dma/mapping.c
> +++ b/kernel/dma/mapping.c
> @@ -365,10 +365,6 @@ EXPORT_SYMBOL(dma_unmap_sg_attrs);
>  dma_addr_t dma_map_resource(struct device *dev, phys_addr_t phys_addr,
>  		size_t size, enum dma_data_direction dir, unsigned long attrs)
>  {
> -	if (IS_ENABLED(CONFIG_DMA_API_DEBUG) &&
> -	    WARN_ON_ONCE(pfn_valid(PHYS_PFN(phys_addr))))
> -		return DMA_MAPPING_ERROR;
> -
>  	return dma_map_phys(dev, phys_addr, size, dir, attrs | DMA_ATTR_MMIO);
>  }
>  EXPORT_SYMBOL(dma_map_resource);
> -- 
> 2.54.0
> 

      parent reply	other threads:[~2026-05-11 17:56 UTC|newest]

Thread overview: 3+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2026-05-11  8:31 [PATCH v2] dma-mapping: move dma_map_resource() sanity check into debug code Jianpeng Chang
2026-05-11 15:32 ` Robin Murphy
2026-05-11 17:56 ` Leon Romanovsky [this message]

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=20260511175641.GL15586@unreal \
    --to=leon@kernel.org \
    --cc=iommu@lists.linux.dev \
    --cc=jgg@ziepe.ca \
    --cc=jianpeng.chang.cn@windriver.com \
    --cc=kbusch@kernel.org \
    --cc=linux-kernel@vger.kernel.org \
    --cc=m.szyprowski@samsung.com \
    --cc=robin.murphy@arm.com \
    --cc=stable@vger.kernel.org \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line before the message body.
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.