Re: [PATCH] dma-mapping: remove bogus test for pfn_valid from dma_map_resource

[Robin Murphy <robin.murphy@arm.com>]

On 2026-05-08 6:36 pm, Jason Gunthorpe wrote:
> On Fri, May 08, 2026 at 05:04:31PM +0100, Robin Murphy wrote:
>> On 2026-05-08 4:18 pm, Jason Gunthorpe wrote:
>>> On Fri, May 08, 2026 at 01:16:25PM +0100, Robin Murphy wrote:
>>>> On 2026-05-08 12:31 pm, Jason Gunthorpe wrote:
>>>>> On Fri, May 08, 2026 at 06:01:01PM +0800, Jianpeng Chang wrote:
>>>>>>> As I said last time, I think pfn_valid() && !PageReserved(pfn_to_page())
>>>>>>> would be enough for what we want here, although now it's strictly under
>>>>>>> CONFIG_DMA_API_DEBUG, perhaps the overhead of memblock_is_map_memory()
>>>>>>> might be less of an issue. Either way though, now that it's all
>>>>>>> channelled through the single dma_map_phys() path, it would probably
>>>>>>> make sense to consolidate any MMIO sanity-checking into
>>>>>>> dma_debug_map_phys() anyway :/
>>>>>
>>>>>> Thanks for the suggestion. Move the check into debug_dma_map_phys() is
>>>>>> indeed better, and I will replace pfn_valid() with pfn_valid() &&
>>>>>> !PageReserved() as you suggested.
>>>>>
>>>>> I'm not sure that is right. IIRC pfn_valid() is true for ZONE_DEVICE
>>>>> P2P pages that are used with map_phys but never with map_resource.
>>>>>
>>>>> PageReserved isn't enough to fix it.
>>>>
>>>> It fixes the false-positive on non-reserved pages, which is the important
>>>> thing. Yes, we'll get false-negatives on reserved ZONE_DEVICE pages and
>>>> similar, but that's still an improvement over getting false-negatives on
>>>> _everything_ by not checking at all. Realistically, dma-debug can never be
>>>> exhaustive and 100% accurate, but there's still value in catching as much
>>>> obvious misuse as is straightforward to do.
>>>
>>> I'm saying I think the new expression still has a false positive for
>>> the common case of map_phys with ZONE_DEVICE P2P, and I don't want to
>>> see debugging logging for normal as-designed scenarios in map_phys.
>>>
>>> So we either need to narrow the expression further somehow, or leave
>>> it in map_resource which has fewer users and doesn't accept
>>> ZONE_DEVICE anyhow.
>>
>> But surely anything with a ZONE_DEVICE page is "memory" to the degree that
>> mapping it with DMA_ATTR_MMIO would be wrong, no?
> 
> If the ZONE_DEVICE subtype is MEMORY_DEVICE_PCI_P2PDMA it is mapped as
> MMIO and must be used with DMA_ATTR_MMIO.
> 
>> However, IIRC ZONE_DEVICE pages _are_ reserved, so still wouldn't
>> warn whether we'd like it or not.
> 
> I didn't think that was the case for PCI_P2PDMA, but yes it does look
> like the reserved flag remains set.
> 
>> I'm confused as to what you're objecting to...
> 
> I don't want to see a warning, if it turns out it doesn't then it's
> fine, but it certainly isn't obvious that it was going to be OK for
> phys and I explained what we were worried about when we had left this
> behind in map resource.
> 
> So this should all be summarized in the commit message moving the
> check

Indeed. The general rule here is that DMA_ATTR_MMIO must not be used for 
anything which could have a cacheable CPU mapping because that could 
break coherency, while conversely any !DMA_ATTR_MMIO mappings must be 
valid for phys_to_virt()/kmap_atomic() so that a DMA API backend *can* 
perform cache maintenance by VA internally. Anything that is invalid for 
dma_map_resource() is inherently invalid for dma_map_phys(DMA_ATTR_MMIO) 
for the same reasons, because dma_map_resource() *is* 
dma_map_phys(DMA_ATTR_MMIO), and it doesn't make any sense to check the 
wrapper differently from the thing it wraps when they are both equally 
public APIs. The point of checking is not to enforce some 
arbitrarily-decided API policy, it is to flag up "you must not do this 
because it risks going badly wrong on non-coherent platforms" if a 
driver does try to do something obviously inappropriate.

Robin.