Re: [PATCH v8 01/23] dma-direct: return struct page from dma_direct_alloc_from_pool()

Leon Romanovsky <[email protected]> writes:

> On Sat, Jul 25, 2026 at 11:34:11AM -0300, Jason Gunthorpe wrote:
>> On Thu, Jul 23, 2026 at 10:57:04AM +0300, Leon Romanovsky wrote:
>> > On Wed, Jul 22, 2026 at 04:59:12PM -0300, Jason Gunthorpe wrote:
>> > > On Tue, Jul 21, 2026 at 06:33:21PM +0300, Leon Romanovsky wrote:
>> > > 
>> > > > Something like, but probably void* needs to be phys_addr_t:
>> > > > 
>> > > > static inline struct page *dma_phys_to_page(void *addr)
>> > > > {
>> > > > #ifdef CONFIG_DMA_DIRECT_REMAP
>> > > > 	return vmalloc_to_page(addr);
>> > > > #else
>> > > > 	return virt_to_page(addr);
>> > > > #endif
>> > > > }
>> > > 
>> > > I don't like this, we don't know for sure the addr will be in a vmap
>> > > and this will fail if it isn't.
>> > 
>> > Of course we know. The existing "#ifdef CONFIG_DMA_DIRECT_REMAP" is
>> > relevant for addresses acquired from pool.
>> 
>> Yeah, but I still don't like it :) It is hard to follow if you make
>> those kinds of leaps, someone will call this new helper on something
>> they shouldn't
>
> This issue is straightforward to address today. Limit the scope, use
> an appropriate function name, and add a comment indicating that the
> function is local to and specific to the pool. The latter helps
> AI-based review tools flag incorrect usage outside the intended scope.
>
>> 
>> > dma_phys_to_page() is a bad name for some low-level conversion function.
>> > It needs to be internal to DMA logic, in the level when we convert from
>> > phys to page.
>> 
>> I think we should not convert from phys to page, that's also easy to
>> do wrong
>
> Perhaps I'll reiterate my complaint. DMA internals are already hard to
> understand. Part of that complexity is necessary to support every
> possible use case, but another part comes from a maze of types that is
> entirely self-inflicted. I think this code only makes the latter worse.
>
> Currently, the phys type universally describes memory and can be
> reliably translated into any required representation. It is the most
> fundamental type we have.
>

I think we can address the suggested changes as a follow-up patch series
on top of this.

How about we get this picked up for the next merge window first, so that
we can get the CC_SHARED related changes in? The follow-up changes
should be easier once we have agreement on the overall design, and can
then be done as incremental cleanups.

-aneesh