On Thu, Mar 5, 2026 at 1:30 PM Maxime Ripard mripard@redhat.com wrote:

On Tue, Mar 03, 2026 at 03:47:14PM +0100, Albert Esteve wrote:

...

On Tue, Mar 3, 2026 at 2:20 PM Maxime Ripard mripard@redhat.com wrote:

...

On Tue, Mar 03, 2026 at 01:33:47PM +0100, Albert Esteve wrote:

...

Add a dma-buf heap for DT coherent reserved-memory (i.e., 'shared-dma-pool' without 'reusable' property), exposing one heap per region for userspace buffers.

The heap binds the heap device to each memory region so coherent allocations use the correct dev->dma_mem, and it defers registration until module_init when normal allocators are available.

Signed-off-by: Albert Esteve aesteve@redhat.com

drivers/dma-buf/dma-heap.c | 4 +- drivers/dma-buf/heaps/Kconfig | 9 + drivers/dma-buf/heaps/Makefile | 1 + drivers/dma-buf/heaps/coherent_heap.c | 426 ++++++++++++++++++++++++++++++++++ include/linux/dma-heap.h | 11 + include/linux/dma-map-ops.h | 7 + 6 files changed, 456 insertions(+), 2 deletions(-)

diff --git a/drivers/dma-buf/dma-heap.c b/drivers/dma-buf/dma-heap.c index 88189d4e48561..ba87e5ac16ae2 100644 --- a/drivers/dma-buf/dma-heap.c +++ b/drivers/dma-buf/dma-heap.c @@ -390,8 +390,8 @@ struct dma_heap *dma_heap_add(const struct dma_heap_export_info *exp_info)

  heap = dma_heap_create(exp_info);
  if (IS_ERR(heap)) {

•         pr_err("dma_heap: failed to create heap (%d)\n", PTR_ERR(heap));
  

•         return PTR_ERR(heap);
  

•         pr_err("dma_heap: failed to create heap (%ld)\n", PTR_ERR(heap));
  

•         return ERR_CAST(heap);
  

This looks unrelated and should possibly be squashed into the previous patch that introduces dma_heap_create()?

...

+static int coherent_heap_init_dma_mask(struct device *dev) +{

• int ret;
  

• ret = dma_coerce_mask_and_coherent(dev, DMA_BIT_MASK(64));
  

• if (!ret)
  

•         return 0;
  

• /* Fallback to 32-bit DMA mask */
  

• return dma_coerce_mask_and_coherent(dev, DMA_BIT_MASK(32));
  

+}

Why do you need to mess with the DMA mask? I'd expect that device to be able to access everything.

When I tested I was getting: "reserved memory is beyond device's set DMA address range", so I tested if it was fixed with dma_coerce_mask_and_coherent() and/or dma_set_mask_coherent(). I did not debug the value of coherent_dma_mask, but given the error I assume it was not set properly? Ultimately, using the 64 bit mask fixed it, and I added a 32-bit fallback to ensure support for 32-bit systems.

So you don't need to handle the fallback because dma_coerce_mask_and_coherent will truncate the generated mask to dma_addr_t, which is 64bits on 64 bits platforms, and 32 bits on 32 bits platforms.

https://elixir.bootlin.com/linux/v6.19.3/source/kernel/dma/mapping.c#L908

Good! I didn't realise that. I will remove it for the next revision.

But I think my point was more than there's nothing specific to the coherent heap itself: the device allocated for the heap should have the right mask for any heap, so it's something I'd rather put in dma_heap_add.

That was my first take too. But when I checked, I did not see dma_heap_add() doing anything to dev->coherent_dma_mask. So I assumed the problem relates to the rmem being bound, which triggers the check to ensure the memory pool is within boundaries. That's a specific issue with the coherent heap, so it sounds like it would be better handled here in the heap-specific code rather than in `dma_heap_add()`, which would affect all the dmabuf heaps.

That being said, setting the mask is probably(?) harmless for the other heaps anyway, so I would be fine with moving it -- to dma_heap_create() to be more specific.

BR, Albert.

...

...

...

+static int __coherent_heap_register(struct reserved_mem *rmem) +{

• struct dma_heap_export_info exp_info;
  

• struct coherent_heap *coh_heap;
  

• struct device *heap_dev;
  

• int ret;
  

• if (!rmem || !rmem->name)
  

•         return -EINVAL;
  

• coh_heap = kzalloc_obj(*coh_heap);
  

• if (!coh_heap)
  

•         return -ENOMEM;
  

• coh_heap->rmem = rmem;
  

• coh_heap->name = kstrdup(rmem->name, GFP_KERNEL);
  

• if (!coh_heap->name) {
  

•         ret = -ENOMEM;
  

•         goto free_coherent_heap;
  

• }
  

• exp_info.name = coh_heap->name;
  

• exp_info.ops = &coherent_heap_ops;
  

• exp_info.priv = coh_heap;
  

• coh_heap->heap = dma_heap_create(&exp_info);
  

• if (IS_ERR(coh_heap->heap)) {
  

•         ret = PTR_ERR(coh_heap->heap);
  

•         goto free_name;
  

• }
  

• heap_dev = dma_heap_get_dev(coh_heap->heap);
  

• ret = coherent_heap_init_dma_mask(heap_dev);
  

• if (ret) {
  

•         pr_err("coherent_heap: failed to set DMA mask (%d)\n", ret);
  

•         goto destroy_heap;
  

• }
  

• ret = of_reserved_mem_device_init_with_mem(heap_dev, rmem);
  

• if (ret) {
  

•         pr_err("coherent_heap: failed to initialize memory (%d)\n", ret);
  

•         goto destroy_heap;
  

• }
  

• ret = dma_heap_register(coh_heap->heap);
  

• if (ret) {
  

•         pr_err("coherent_heap: failed to register heap (%d)\n", ret);
  

•         goto destroy_heap;
  

• }
  

I guess it's more of a comment about your previous patch, but it's not clear to me why you needed to split dma_heap_add into dma_heap_create / _register. Can you expand a bit?

So first I tried to just use dma_heap_add() and then use the heap_dev afterward to call of_reserved_mem_device_init_with_mem(), but if that call failed, the error path required some kind dma_heap_remove() function as the heap was already registered by then.

In the CMA heap for example, dma_heap_add() is invoked at the end of the `init` function. Therefore, you do not have this issue, if it failed it means the heap was not added and you just need to clean everything else.

However, performing a remove() does not sound like something that can be done safely. I've spent some time thinking on alternatives, but splitting felt the best pattern.

This way I can:

1. Create the device
2. Call of_reserved_mem_device_init_with_mem
3. Register the heap

This places registration at the end, making every error path and cleanup easy to handle.

Also, the `dma_heap_add()` code already seemed to handle these two parts/phases implicitly with device_create(), so splitting felt architecturally sound.

That makes sense, thanks!

Maxime