Re: [PATCH v3] dma-coherent: add support for multi coherent rmems per dev

[Howard Yen]

On Fri, Feb 9, 2024 at 2:02 AM Andy Shevchenko
<andriy.shevchenko@xxxxxxxxxxxxxxx> wrote:
>
> On Thu, Feb 08, 2024 at 03:28:05PM +0000, Howard Yen wrote:
> > Add support for multiple coherent rmems per device. This patch replaces
> > original dma_mem with dma_mems list in device structure to store multiple
> > rmems.
> >
> > These multiple rmems can be assigned to the device one by one by
> > of_reserved_mem_device_init_by_idx() with the memory-region
> > declaration in device tree as below and store the rmem to the dma_mems
> > list.
> >
> >       device1@0 {
> >               ...
> >               memory-region = <&reserved_mem0>, <&reserved_mem1>;
> >               ...
> >       };
> >
> > When driver tries to allocate memory from the rmems, looks for the first
> > available rmem and allocates the memory from this rmem.
> >
> > Then if driver removed, of_reserved_mem_device_release() needs to be
> > invoked to release all the rmems assigned to the device.
>
> ...
>
> >       struct dma_coherent_mem *mem;
> > -     int ret;
> > +     int retval;
> >
> >       mem = dma_init_coherent_memory(phys_addr, device_addr, size, false);
> >       if (IS_ERR(mem))
> >               return PTR_ERR(mem);
> >
> > -     ret = dma_assign_coherent_memory(dev, mem);
> > -     if (ret)
> > +     retval = dma_assign_coherent_memory(dev, mem);
> > +     if (retval)
> >               _dma_release_coherent_memory(mem);
> > -     return ret;
> > +     return retval;
>
> This is unrelated change.
>
> But why? Do you have retval in the _existing_ code elsewhere?

Rename the 'ret' variable to 'retval' because, in the
dma_mmap_from_dev_coherent(),
there is an argument named 'ret', and also I add 'retval' for the return value.
So I try to rename it here to be consistent.

>
>
> ...
>
> >  int dma_alloc_from_dev_coherent(struct device *dev, ssize_t size,
> >               dma_addr_t *dma_handle, void **ret)
> >  {
> > -     struct dma_coherent_mem *mem = dev_get_coherent_memory(dev);
> > +     struct dma_coherent_mem *mem_tmp;
> >
> > -     if (!mem)
> > +     if (list_empty(&dev->dma_mems))
> >               return 0;
> >
> > -     *ret = __dma_alloc_from_coherent(dev, mem, size, dma_handle);
> > +     list_for_each_entry(mem_tmp, &dev->dma_mems, node) {
> > +             *ret = __dma_alloc_from_coherent(dev, mem_tmp, size, dma_handle);
> > +             if (*ret)
> > +                     break;
>
> This bails out on the first success. Moreover, if one calls this function
> again, it will rewrite the existing allocation. Is this all expected?
>
> OTOH, if you add multiple entries and bailing out on error condition it should
> be clear if the previous allocations have to be released.

If it's not able to allocate required memory from the first entry,
then tries to allocate
from the following entry, and so on.

>
> > +     }
>
> >       return 1;
>
> >  }
>
> ...
>
> >  int dma_release_from_dev_coherent(struct device *dev, int order, void *vaddr)
> >  {
> > -     struct dma_coherent_mem *mem = dev_get_coherent_memory(dev);
> > +     struct dma_coherent_mem *mem_tmp;
> > +     int retval = 0;
> > +
> > +     list_for_each_entry(mem_tmp, &dev->dma_mems, node) {
> > +             retval = __dma_release_from_coherent(mem_tmp, order, vaddr);
> > +             if (retval == 1)
> > +                     break;
>
> Same Q here.
>
> > +     }
> >
> > -     return __dma_release_from_coherent(mem, order, vaddr);
> > +     return retval;
> >  }
>
> ...
>
> >  int dma_mmap_from_dev_coherent(struct device *dev, struct vm_area_struct *vma,
> >                          void *vaddr, size_t size, int *ret)
> >  {
> > -     struct dma_coherent_mem *mem = dev_get_coherent_memory(dev);
> > +     struct dma_coherent_mem *mem_tmp;
> > +     int retval = 0;
> >
> > -     return __dma_mmap_from_coherent(mem, vma, vaddr, size, ret);
> > +     list_for_each_entry(mem_tmp, &dev->dma_mems, node) {
> > +             retval = __dma_mmap_from_coherent(mem_tmp, vma, vaddr, size, ret);
> > +             if (retval == 1)
> > +                     break;
>
> And here.
>
> > +     }
> > +
> > +     return retval;
> >  }
>
> ...
>
> With the above Q in mind, here is another one: Why can't we allocate all at once?

Not sure if I misunderstand your meaning, It tries to allocate the
memory from the first
entry that satisfies the allocation requirement, but not separately.

>
> --
> With Best Regards,
> Andy Shevchenko
>
>


-- 
Regards,

Howard