RE: [PATCH v6 06/11] PCI: endpoint: Add support to handle multiple base for mapping outbound memory

[Prabhakar Mahadev Lad]

Hi Shimoda-san,

Thank you for the review.

> -----Original Message-----
> From: Yoshihiro Shimoda <yoshihiro.shimoda.uh@xxxxxxxxxxx>
> Sent: 03 April 2020 09:23
> To: Prabhakar Mahadev Lad <prabhakar.mahadev-lad.rj@xxxxxxxxxxxxxx>; Bjorn Helgaas <bhelgaas@xxxxxxxxxx>; Rob Herring
> <robh+dt@xxxxxxxxxx>; Mark Rutland <mark.rutland@xxxxxxx>; Geert Uytterhoeven <geert+renesas@xxxxxxxxx>; Magnus Damm
> <magnus.damm@xxxxxxxxx>; Kishon Vijay Abraham I <kishon@xxxxxx>; Lorenzo Pieralisi <lorenzo.pieralisi@xxxxxxx>; Marek Vasut
> <marek.vasut+renesas@xxxxxxxxx>; linux-pci@xxxxxxxxxxxxxxx
> Cc: Catalin Marinas <catalin.marinas@xxxxxxx>; Will Deacon <will@xxxxxxxxxx>; Arnd Bergmann <arnd@xxxxxxxx>; Greg Kroah-Hartman
> <gregkh@xxxxxxxxxxxxxxxxxxx>; Andrew Murray <andrew.murray@xxxxxxx>; devicetree@xxxxxxxxxxxxxxx; linux-kernel@xxxxxxxxxxxxxxx;
> linux-arm-kernel@xxxxxxxxxxxxxxxxxxx; linux-renesas-soc@xxxxxxxxxxxxxxx; Chris Paterson <Chris.Paterson2@xxxxxxxxxxx>; Frank Rowand
> <frowand.list@xxxxxxxxx>; Gustavo Pimentel <gustavo.pimentel@xxxxxxxxxxxx>; Jingoo Han <jingoohan1@xxxxxxxxx>; Simon Horman
> <horms@xxxxxxxxxxxx>; Shawn Lin <shawn.lin@xxxxxxxxxxxxxx>; Tom Joseph <tjoseph@xxxxxxxxxxx>; Heiko Stuebner
> <heiko@xxxxxxxxx>; linux-rockchip@xxxxxxxxxxxxxxxxxxx; Lad Prabhakar <prabhakar.csengg@xxxxxxxxx>; Prabhakar Mahadev Lad
> <prabhakar.mahadev-lad.rj@xxxxxxxxxxxxxx>
> Subject: RE: [PATCH v6 06/11] PCI: endpoint: Add support to handle multiple base for mapping outbound memory
>
> Hi Prabhakar-san,
>
> Thank you for the patch!
>
> > From: Lad Prabhakar, Sent: Friday, April 3, 2020 4:39 AM
> >
> > R-Car PCIe controller has support to map multiple memory regions for
> > mapping the outbound memory in local system also the controller limits
> > single allocation for each region (that is, once a chunk is used from the
> > region it cannot be used to allocate a new one). This features inspires to
> > add support for handling multiple memory bases in endpoint framework.
> >
> > With this patch pci_epc_mem_init() initializes address space for endpoint
> > controller which support single window and whereas __pci_epc_mem_init()
> > now accepts pointer to multiple windows supported by endpoint controller.
> >
> > Signed-off-by: Lad Prabhakar <prabhakar.mahadev-lad.rj@xxxxxxxxxxxxxx>
> > ---
> > @@ -38,61 +38,95 @@ static int pci_epc_mem_get_order(struct pci_epc_mem *mem, size_t size)
> >  /**
> >   * __pci_epc_mem_init() - initialize the pci_epc_mem structure
> >   * @epc: the EPC device that invoked pci_epc_mem_init
> > - * @phys_base: the physical address of the base
> > - * @size: the size of the address space
> > - * @page_size: size of each page
> > + * @windows: pointer to windows supported by the device
> > + * @num_windows: number of windows device supports
> >   *
> >   * Invoke to initialize the pci_epc_mem structure used by the
> >   * endpoint functions to allocate mapped PCI address.
> >   */
> > -int __pci_epc_mem_init(struct pci_epc *epc, phys_addr_t phys_base, size_t size,
> > -       size_t page_size)
> > +int __pci_epc_mem_init(struct pci_epc *epc, struct pci_epc_mem_window *windows,
> > +       unsigned int num_windows)
> >  {
> > -int ret;
> > -struct pci_epc_mem *mem;
> > -unsigned long *bitmap;
> > +struct pci_epc_mem *mem = NULL;
> > +unsigned long *bitmap = NULL;
> >  unsigned int page_shift;
> > -int pages;
> > +size_t page_size;
> >  int bitmap_size;
> > +int pages;
> > +int ret;
> > +int i;
> >
> > -if (page_size < PAGE_SIZE)
> > -page_size = PAGE_SIZE;
> > +epc->num_windows = 0;
> >
> > -page_shift = ilog2(page_size);
> > -pages = size >> page_shift;
> > -bitmap_size = BITS_TO_LONGS(pages) * sizeof(long);
> > +if (!windows || !num_windows)
> > +return -EINVAL;
> >
> > -mem = kzalloc(sizeof(*mem), GFP_KERNEL);
> > -if (!mem) {
> > -ret = -ENOMEM;
> > -goto err;
> > -}
> > +epc->windows = kcalloc(num_windows, sizeof(*mem), GFP_KERNEL);
> > +if (!epc->windows)
> > +return -ENOMEM;
> >
> > -bitmap = kzalloc(bitmap_size, GFP_KERNEL);
> > -if (!bitmap) {
> > -ret = -ENOMEM;
> > -goto err_mem;
> > -}
> > +for (i = 0; i < num_windows; i++) {
> > +page_size = windows[i].page_size;
> > +if (page_size < PAGE_SIZE)
> > +page_size = PAGE_SIZE;
> > +page_shift = ilog2(page_size);
> > +pages = windows[i].size >> page_shift;
> > +bitmap_size = BITS_TO_LONGS(pages) * sizeof(long);
> > +
> > +mem = kzalloc(sizeof(*mem), GFP_KERNEL);
> > +if (!mem) {
> > +ret = -ENOMEM;
> > +i -= 1;
>
> nit: We can use i--;
>
Will change it.

> > +goto err_mem;
> > +}
> > +
> > +bitmap = kzalloc(bitmap_size, GFP_KERNEL);
> > +if (!bitmap) {
> > +ret = -ENOMEM;
> > +kfree(mem);
> > +i -= 1;
>
> nit: We can use i--;
>
As above.

> <snip>
> > @@ -122,31 +167,56 @@ EXPORT_SYMBOL_GPL(pci_epc_mem_exit);
> >  void __iomem *pci_epc_mem_alloc_addr(struct pci_epc *epc,
> >       phys_addr_t *phys_addr, size_t size)
> >  {
> > -int pageno;
> >  void __iomem *virt_addr = NULL;
> > -struct pci_epc_mem *mem = epc->mem;
> > -unsigned int page_shift = ilog2(mem->page_size);
> > +struct pci_epc_mem *mem;
> > +unsigned int page_shift;
> > +int pageno = -EINVAL;
> >  int order;
> > +int i;
> >
> > -size = ALIGN(size, mem->page_size);
> > -order = pci_epc_mem_get_order(mem, size);
> > -
> > -mutex_lock(&mem->lock);
> > -pageno = bitmap_find_free_region(mem->bitmap, mem->pages, order);
> > -if (pageno < 0)
> > -goto ret;
> > +for (i = 0; i < epc->num_windows; i++) {
> > +mem = epc->windows[i];
> > +mutex_lock(&mem->lock);
>
> This is my feeling though, calling mutex_lock() in the loop seems
> to cause overhead. And, if we call mutex_lock() at out-of the loop,
> I think we can write single mutex_unlock() calling.
>
But the mutex is for each window, are you suggesting to add a global mutex ?

> > +size = ALIGN(size, mem->window.page_size);
>
> I'm sorry I should have realized this in the previous review,
> but overwriting this size is possible to cause an issue at second time or more loops.
> So, the first argument of ALIGN should be kept for the loop.
>
Could you please elaborate on this.

> > +order = pci_epc_mem_get_order(mem, size);
> >
> > -*phys_addr = mem->phys_base + ((phys_addr_t)pageno << page_shift);
> > -virt_addr = ioremap(*phys_addr, size);
> > -if (!virt_addr)
> > -bitmap_release_region(mem->bitmap, pageno, order);
> > +pageno = bitmap_find_free_region(mem->bitmap, mem->pages,
> > + order);
> > +if (pageno >= 0) {
> > +page_shift = ilog2(mem->window.page_size);
> > +*phys_addr = mem->window.phys_base +
> > +((phys_addr_t)pageno << page_shift);
> > +virt_addr = ioremap(*phys_addr, size);
> > +if (!virt_addr)
> > +bitmap_release_region(mem->bitmap,
> > +      pageno, order);
> > +mutex_unlock(&mem->lock);
> > +return virt_addr;
>
> As I mentioned above, if mutex_lock() is called at out-of-loop,
> we can use "goto ret;" here like the original code,
>
> > +}
> > +mutex_unlock(&mem->lock);
>
> and we can remove this.
>
> > +}
> >
> > -ret:
> > -mutex_unlock(&mem->lock);
> >  return virt_addr;
> >  }
> >  EXPORT_SYMBOL_GPL(pci_epc_mem_alloc_addr);
> >
> > +struct pci_epc_mem *pci_epc_get_matching_window(struct pci_epc *epc,
> > +phys_addr_t phys_addr)
> > +{
> > +struct pci_epc_mem *mem;
> > +int i;
> > +
> > +for (i = 0; i < epc->num_windows; i++) {
> > +mem = epc->windows[i];
> > +
> > +if (phys_addr >= mem->window.phys_base &&
> > +    phys_addr < (mem->window.phys_base + mem->window.size))
> > +return mem;
> > +}
> > +
> > +return NULL;
> > +}
> > +
> >  /**
> >   * pci_epc_mem_free_addr() - free the allocated memory address
> >   * @epc: the EPC device on which memory was allocated
> > @@ -159,14 +229,23 @@ EXPORT_SYMBOL_GPL(pci_epc_mem_alloc_addr);
> >  void pci_epc_mem_free_addr(struct pci_epc *epc, phys_addr_t phys_addr,
> >     void __iomem *virt_addr, size_t size)
> >  {
> > +struct pci_epc_mem *mem;
> > +unsigned int page_shift;
> > +size_t page_size;
> >  int pageno;
> > -struct pci_epc_mem *mem = epc->mem;
> > -unsigned int page_shift = ilog2(mem->page_size);
> >  int order;
> >
> > +mem = pci_epc_get_matching_window(epc, phys_addr);
> > +if (!mem) {
> > +pr_err("failed to get matching window\n");
> > +return;
> > +}
> > +
> > +page_size = mem->window.page_size;
> > +page_shift = ilog2(page_size);
> >  iounmap(virt_addr);
> > -pageno = (phys_addr - mem->phys_base) >> page_shift;
> > -size = ALIGN(size, mem->page_size);
> > +pageno = (phys_addr - mem->window.phys_base) >> page_shift;
> > +size = ALIGN(size, page_size);
> >  order = pci_epc_mem_get_order(mem, size);
> >  mutex_lock(&mem->lock);
> >  bitmap_release_region(mem->bitmap, pageno, order);
> > diff --git a/include/linux/pci-epc.h b/include/linux/pci-epc.h
> > index e0ed9d01f6e5..d5da11cf0f2a 100644
> > --- a/include/linux/pci-epc.h
> > +++ b/include/linux/pci-epc.h
> > @@ -65,20 +65,28 @@ struct pci_epc_ops {
> >  struct module *owner;
> >  };
> >
> > +/**
> > + * struct pci_epc_mem_window - address window of the endpoint controller
> > + * @phys_base: physical base address of the PCI address window
> > + * @size: the size of the PCI address window
> > + * @page_size: size of each page
> > + */
> > +struct pci_epc_mem_window {
> > +phys_addr_tphys_base;
> > +size_tsize;
> > +size_tpage_size;
> > +};
> > +
> >  /**
> >   * struct pci_epc_mem - address space of the endpoint controller
> > - * @phys_base: physical base address of the PCI address space
> > - * @size: the size of the PCI address space
> > + * @window: address window of the endpoint controller
> >   * @bitmap: bitmap to manage the PCI address space
> > - * @pages: number of bits representing the address region
> > - * @page_size: size of each page
> >   * @lock: mutex to protect bitmap
> > + * @pages: number of bits representing the address region
>
> Perhaps, we should not change the "@pages" line.
>
OK will drop this change.

Cheers,
--Prabhakar

> Best regards,
> Yoshihiro Shimoda



Renesas Electronics Europe GmbH, Geschaeftsfuehrer/President: Carsten Jauch, Sitz der Gesellschaft/Registered office: Duesseldorf, Arcadiastrasse 10, 40472 Duesseldorf, Germany, Handelsregister/Commercial Register: Duesseldorf, HRB 3708 USt-IDNr./Tax identification no.: DE 119353406 WEEE-Reg.-Nr./WEEE reg. no.: DE 14978647