Re: [PATCH v7 net] octeontx2-af: switch qmem from coherent DMA alloc to streaming DMA mapping

[Ratheesh Kannoth <rkannoth@marvell.com>]

On 2026-09-07 at 12:50:55, Leon Romanovsky (leon@kernel.org) wrote:
> On Mon, Sep 07, 2026 at 09:56:16AM +0530, Ratheesh Kannoth wrote:
> > qmem_alloc() uses dma_alloc_attrs() with DMA_ATTR_FORCE_CONTIGUOUS, which
> > allocates CPU-cache-coherent DMA memory and, with CMA enabled, draws from
> > the CMA pool. qmem backs NIX/NPA queue contexts, admin queues, and LMTST
> > regions (including CN10K LMTST areas that span page boundaries), so
> > consumption grows with enabled interfaces and is hard to provision in CMA.
> >
> >
> > v1 -> v2: Rewrote patch as per sashiko comment
>
> Your changelog says nothing. It should contain bullet points describing
> what was actually changed. A generic "Addressed Sashiko comments" is not
> sufficient.

Thanks for pointing this out. I relied on the Sashiko link to avoid
over-cluttering the notes, as sashiko coments are many.
I will make sure to explicitly list all notable changes in the
changelog in future revisions.

> >
> > +static inline bool otx2_dma_phys_in_mask(struct device *dev, phys_addr_t paddr,
> > +					 size_t size)
> > +{
> > +	dma_addr_t dma_addr = phys_to_dma(dev, paddr);
> > +
> > +	return dma_capable(dev, dma_addr, size, true, 0);
>
> This line makes no sense in the driver code.

ACK. will remove internal APIs

>
> > +}
> > +
> > +static inline void *otx2_dma_alloc_coherent(struct device *dev, size_t size,
> > +					    dma_addr_t *dma_handle)
> > +{
> > +	dma_addr_t dma_addr;
> > +	unsigned int order;
> > +	gfp_t alloc_gfp;
> > +	void *vaddr;
> > +
> > +	if (!dev || !dma_handle || !size)
> > +		return NULL;
>
> Please remove defensive programming style, how can you call to DMA API
> without device, dma_handle or size?
ACK.

>
> > +
> > +	if (!dev_is_dma_coherent(dev))
> > +		return NULL;
> > +
> > +	size = PAGE_ALIGN(size);
> > +	order = get_order(size);
> > +
> > +	/* Octeontx2 qmem call sites size their allocations within
> > +	 * MAX_PAGE_ORDER; mailbox, queue context, and ring memory
> > +	 * requirements stay below the buddy allocator's limit.
> > +	 */
> > +	if (order > MAX_PAGE_ORDER) {
>
> Size is coming from the kernel, how can it be with order more than MAX_PAGE_ORDER?
There is contigious memory allocation request from driver for PF-to-VF mail box memory.
It is crossing max page order in newer platforms as number of VFs per PF increased.

>
> > +		dev_err(dev,
> > +			"CONFIG_ARCH_FORCE_MAX_ORDER is set to %u, minimum needed is %u\n",
> > +			MAX_PAGE_ORDER, order);
> > +		return NULL;
> > +	}
> > +
> > +	if (size > dma_max_mapping_size(dev))
> > +		return NULL;
>
> Same comment.
Will remove this code and depend on dma_map_page_attrs().

>
> > +
> > +	alloc_gfp = GFP_KERNEL | __GFP_ZERO | __GFP_COMP;
>
> __GFP_COMP???
Will remove this flag.

>
> > +
> > +	vaddr = (void *)__get_free_pages(alloc_gfp, order);
>
> You should use plain ksmalloc(). There was ongoing effort to remove
> useless __get_free_pages().
> https://lore.kernel.org/all/20260713-b4-rdma-v2-0-65d2a1a5180c@kernel.org/
will replace get_free_pages with kmalloc.

>
> > +	while (vaddr &&
> > +	       !otx2_dma_phys_in_mask(dev, virt_to_phys(vaddr), size)) {
> > +		free_pages((unsigned long)vaddr, order);
> > +		if (alloc_gfp & GFP_DMA32)
> > +			return NULL;
> > +		alloc_gfp |= GFP_DMA32;
> > +		vaddr = (void *)__get_free_pages(alloc_gfp, order);
> > +	}
>
> If you want to use streamline API, please use it like any other driver
> without these DMA32 hacks.
Will kmalloc() with GFP_ZERO | GFP_KERNEL

>
> > +	if (!vaddr)
> > +		return NULL;
> > +
> > +	/* dev_is_dma_coherent() only guarantees cache coherency, not that the
> > +	 * mapped DMA address aliases qmem->base.  Require a coherent mapping
> > +	 * so the DMA API rejects SWIOTLB bounce buffers.
> > +	 */
> > +	dma_addr = dma_map_page_attrs(dev, virt_to_page(vaddr), 0, size,
> > +				      DMA_BIDIRECTIONAL, DMA_ATTR_REQUIRE_COHERENT);
>
> I don't understand why you insist on DMA_ATTR_REQUIRE_COHERENT. It is
> clearly documented as being required for UAPI-visible memory.
ACK.
I overlooked this uAPI part, as  AI tool recommended adding
this logic to reject allocations with SWIOTLB or cache management
support.