RE: [PATCH v5 4/7] PCI: endpoint: Add support to handle multiple base for mapping outbound memory

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

Hi Prabhakar-san,

Just in my opinion though, automatically adding new line of email client
should be disabled or setting larger characters.
# In my side, I change the setting as 132 characters on Outlook :)

> From: Prabhakar Mahadev Lad, Sent: Tuesday, March 17, 2020 7:04 PM
<snip>
> > > -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,
> > > +		       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;
> > > -
> > > -	if (page_size < PAGE_SIZE)
> > > -		page_size = PAGE_SIZE;
> > > -
> > > -	page_shift = ilog2(page_size);
> > > -	pages = size >> page_shift;
> > > -	bitmap_size = BITS_TO_LONGS(pages) * sizeof(long);
> > > -
> > > -	mem = kzalloc(sizeof(*mem), GFP_KERNEL);
> > > -	if (!mem) {
> > > -		ret = -ENOMEM;
> > > -		goto err;
> > > -	}
> > > -
> > > -	bitmap = kzalloc(bitmap_size, GFP_KERNEL);
> > > -	if (!bitmap) {
> > > -		ret = -ENOMEM;
> > > -		goto err_mem;
> > > +	int pages;
> > > +	int ret;
> > > +	int i;
> > > +
> > > +	epc->mem_windows = 0;
> > > +
> > > +	if (!windows)
> > > +		return -EINVAL;
> > > +
> > > +	if (num_windows <= 0)
> > > +		return -EINVAL;
> > > +
> > > +	epc->mem = kcalloc(num_windows, sizeof(*mem), GFP_KERNEL);
> > > +	if (!epc->mem)
> > > +		return -EINVAL;
> > > +
> > > +	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;
> > > +			goto err_mem;
> > > +		}
> > > +
> > > +		bitmap = kzalloc(bitmap_size, GFP_KERNEL);
> > > +		if (!bitmap) {
> > > +			ret = -ENOMEM;
> > > +			goto err_mem;
> > > +		}
> > > +
> > > +		mem->bitmap = bitmap;
> > > +		mem->window.phys_base = windows[i].phys_base;
> >
> > I could not understand why the window member is needed.
> > I think original members (just phys_base and size) are enough.
> > Also, this function doesn't store the page_size to mem->window.page_size.

I'm sorry, but I meant the window member is in the left side (mem->window.phys_base).
In other words, this patch changes the struct pci_epc_mem like below, but
I'm thinking this change is not needed because struct pci_epc will have
multiple windows as "array of address space of the endpoint controller".
---
struct pci_epc_mem {
-	phys_addr_t	phys_base;
-	size_t		size;
+	struct pci_epc_mem_window window;
---

> Because,  for example on RZ/Gx platforms, following are the windows on endpoint device
> where the root's address can be mapped, but where as on other platforms atm there
> exists just single window to map. Also on RZ/Gx platforms if a window is allocated say of
> size 1K, rest of the window cannot be used for other allocations.
> 
> 1: 0xfe000000 0 0x80000
> 2: 0xfe100000 0 0x100000
> 3: 0xfe200000 0 0x200000
> 4: 0x30000000 0 0x8000000
> 5: 0x38000000 0 0x8000000
> 
> Struct pci_epc_mem_window, represents the above windows.

Yes, I understood it.

> window.page_size is set by endpoint controller drivers as done in this patch.

I meant the left side. No one change the mem->window.page_size so that
the value seems to be 0. Of course, for now, this is no problem because
no one uses this value though.

<snip>
> > >  /**
> > >   * 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
> > > + * @pages: number of bits representing the address region
> > >   */
> > >  struct pci_epc_mem {
> > > -	phys_addr_t	phys_base;
> > > -	size_t		size;
> > > +	struct pci_epc_mem_window window;
> > >  	unsigned long	*bitmap;
> > >  	size_t		page_size;
> > >  	int		pages;
> > > @@ -85,7 +97,8 @@ struct pci_epc_mem {
> > >   * @dev: PCI EPC device
> > >   * @pci_epf: list of endpoint functions present in this EPC device
> > >   * @ops: function pointers for performing endpoint operations
> > > - * @mem: address space of the endpoint controller
> >
> > If my idea is acceptable, this should be "default address space ...".
> >
> Could you please elaborate more on how you would like the structures to be organized.

 * @mem: default address space of the endpoint controller.

And, if I assumed the "array of address space of the endpoint controller"
is renamed as struct pci_epc_mem **windows and when __pci_epc_mem_init() is succeeded,
the function should set the mem value right before return as the first window like below.

+	epc->mem = epc->windows[0];
+	epc->num_windows = num_windows;

	return 0;

Best regards,
Yoshihiro Shimoda