Received-SPF: pass (google.com: domain of linux-kernel+bounces-71206-linux.lists.archive=gmail.com@vger.kernel.org designates 139.178.88.99 as permitted sender) client-ip=139.178.88.99;
Precedence: bulk
MIME-Version: 1.0
References: <20240208152808.3712149-1-howardyen@google.com> <ZcUXP14Ng8g5vw1j@smile.fi.intel.com>
In-Reply-To: <ZcUXP14Ng8g5vw1j@smile.fi.intel.com>
From: Howard Yen <howardyen@google.com>
Date: Mon, 19 Feb 2024 19:29:55 +0800
Message-ID: <CAJDAHvbYBVS=LiYHWd_KAtGtcQsjRsLCm2w3j9o4SvLBG_qiZw@mail.gmail.com>
Subject: Re: [PATCH v3] dma-coherent: add support for multi coherent rmems per dev
To: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
Cc: hch@lst.de, m.szyprowski@samsung.com, robin.murphy@arm.com, 
	gregkh@linuxfoundation.org, rafael@kernel.org, broonie@kernel.org, 
	james@equiv.tech, james.clark@arm.com, masahiroy@kernel.org, 
	linux-kernel@vger.kernel.org, iommu@lists.linux.dev
Content-Type: text/plain; charset="UTF-8"
Content-Transfer-Encoding: quoted-printable

On Fri, Feb 9, 2024 at 2:02=E2=80=AFAM Andy Shevchenko
<andriy.shevchenko@linux.intel.com> 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 multip=
le
> > 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 =3D <&reserved_mem0>, <&reserved_mem1>;
> >               ...
> >       };
> >
> > When driver tries to allocate memory from the rmems, looks for the firs=
t
> > 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 =3D dma_init_coherent_memory(phys_addr, device_addr, size, fa=
lse);
> >       if (IS_ERR(mem))
> >               return PTR_ERR(mem);
> >
> > -     ret =3D dma_assign_coherent_memory(dev, mem);
> > -     if (ret)
> > +     retval =3D 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 va=
lue.
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 =3D dev_get_coherent_memory(dev);
> > +     struct dma_coherent_mem *mem_tmp;
> >
> > -     if (!mem)
> > +     if (list_empty(&dev->dma_mems))
> >               return 0;
> >
> > -     *ret =3D __dma_alloc_from_coherent(dev, mem, size, dma_handle);
> > +     list_for_each_entry(mem_tmp, &dev->dma_mems, node) {
> > +             *ret =3D __dma_alloc_from_coherent(dev, mem_tmp, size, dm=
a_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 s=
hould
> 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 =3D dev_get_coherent_memory(dev);
> > +     struct dma_coherent_mem *mem_tmp;
> > +     int retval =3D 0;
> > +
> > +     list_for_each_entry(mem_tmp, &dev->dma_mems, node) {
> > +             retval =3D __dma_release_from_coherent(mem_tmp, order, va=
ddr);
> > +             if (retval =3D=3D 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_stru=
ct *vma,
> >                          void *vaddr, size_t size, int *ret)
> >  {
> > -     struct dma_coherent_mem *mem =3D dev_get_coherent_memory(dev);
> > +     struct dma_coherent_mem *mem_tmp;
> > +     int retval =3D 0;
> >
> > -     return __dma_mmap_from_coherent(mem, vma, vaddr, size, ret);
> > +     list_for_each_entry(mem_tmp, &dev->dma_mems, node) {
> > +             retval =3D __dma_mmap_from_coherent(mem_tmp, vma, vaddr, =
size, ret);
> > +             if (retval =3D=3D 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
>
>


--=20
Regards,

Howard