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

[Yoshihiro Shimoda <yoshihiro.shimoda.uh@renesas.com>]

Hi Prabhakar-san,

> From: Prabhakar Mahadev Lad, Sent: Friday, April 3, 2020 6:12 PM
<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 ?

Oops, that's right. So, I'd like to recall.

> > > +		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.

My concern is the following.

For example, the size of argument of pci_epc_mem_alloc_addr() is 4096.
epc->windows[0].window.page_size = 8192
 --> then the size will be changed to 0.

epc->windows[1].window.page_size = 4096
 --> since the size was changed to 0 on the first loop, the result is 0.
     But, this should be 4096.

Does such a case never happen?
(Or, is my understanding incorrect?)

Best regards,
Yoshihiro Shimoda


> > > +		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_t	phys_base;
> > > +	size_t		size;
> > > +	size_t		page_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