When the CMA allocation succeeds but isn't addressable, its
buffer has already been released and the page is set to NULL.
So later when the normal page allocation succeeds but isn't
addressable, __free_pages() should be used to free that normal
page rather than freeing continuous page(CMA).

Fixes: 90ae409f9eb3 ("dma-direct: fix zone selection after an unaddressable CMA allocation")
Signed-off-by: Chen Yu <yu.c.chen@intel.com>
---
 kernel/dma/direct.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/kernel/dma/direct.c b/kernel/dma/direct.c
index 4480a3cd92e0..51f07bf235c0 100644
--- a/kernel/dma/direct.c
+++ b/kernel/dma/direct.c
@@ -140,7 +140,7 @@ static struct page *__dma_direct_alloc_pages(struct device *dev, size_t size,
 	if (!page)
 		page = alloc_pages_node(node, gfp, get_order(size));
 	if (page && !dma_coherent_ok(dev, page_to_phys(page), size)) {
-		dma_free_contiguous(dev, page, size);
+		__free_pages(page, get_order(size));
 		page = NULL;
 
 		if (IS_ENABLED(CONFIG_ZONE_DMA32) &&
-- 
2.25.1

On Sat, Aug 31, 2024 at 07:01:19PM +0800, Chen Yu wrote:
> When the CMA allocation succeeds but isn't addressable, its
> buffer has already been released and the page is set to NULL.
> So later when the normal page allocation succeeds but isn't
> addressable, __free_pages() should be used to free that normal
> page rather than freeing continuous page(CMA).

So the patch is obviously correct and probably useful, but I don't think
the existing code is buggy.

dma_free_contiguous calls into cma_release, which uses cma_pages_valid to
check if the page belongs to the CMA pool and retuns early if not,
letting dma_free_contiguous fall back to __free_pages eventually.

What am I missing here?

Hi Christoph,

On 2024-09-03 at 09:35:21 +0200, Christoph Hellwig wrote:
> On Sat, Aug 31, 2024 at 07:01:19PM +0800, Chen Yu wrote:
> > When the CMA allocation succeeds but isn't addressable, its
> > buffer has already been released and the page is set to NULL.
> > So later when the normal page allocation succeeds but isn't
> > addressable, __free_pages() should be used to free that normal
> > page rather than freeing continuous page(CMA).
> 
> So the patch is obviously correct and probably useful, but I don't think
> the existing code is buggy.
> 
> dma_free_contiguous calls into cma_release, which uses cma_pages_valid to
> check if the page belongs to the CMA pool and retuns early if not,
> letting dma_free_contiguous fall back to __free_pages eventually.
>
> What am I missing here?
> 
Thanks for taking a look. Your are right, the pfn will be checked in
cma_pages_valid() to see it is within the CMA range, if not, it prints
some messages and return false, finally falls into __free_pages(). From
the functional point of view, there is no bug. From the efficiency point
of view(extra checks/printed message), and from the code readability to
avoid confusing, maybe we can refine it?

thanks,
Chenyu

On Tue, Sep 03, 2024 at 04:00:46PM +0800, Chen Yu wrote:
> Thanks for taking a look. Your are right, the pfn will be checked in
> cma_pages_valid() to see it is within the CMA range, if not, it prints
> some messages and return false, finally falls into __free_pages(). From
> the functional point of view, there is no bug. From the efficiency point
> of view(extra checks/printed message), and from the code readability to
> avoid confusing, maybe we can refine it?

I'll apply the patch with an updated commit log, thanks.