Re: [v3 3/6] PCI: endpoint: Add support to handle multiple base for mapping outbound memory

[Kishon Vijay Abraham I <kishon@ti.com>]

Hi Prabhakar,

On 10/01/20 11:38 PM, Lad, Prabhakar wrote:
> Hi Kishon,
> 
> Thank you for the review.
> 
> On Thu, Jan 9, 2020 at 6:25 AM Kishon Vijay Abraham I <kishon@ti.com> wrote:
>>
>> Hi Prabhakar,
>>
>> On 08/01/20 9:52 PM, Lad Prabhakar wrote:
>>> 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() now accepts multiple regions, also
>>> page_size for each memory region is passed during initialization so as
>>> to handle single allocation for each region by setting the page_size to
>>> window_size.
>>>
>>> Signed-off-by: Lad Prabhakar <prabhakar.mahadev-lad.rj@bp.renesas.com>
>>> ---
>>>  .../pci/controller/cadence/pcie-cadence-ep.c  |  12 +-
>>>  .../pci/controller/dwc/pcie-designware-ep.c   |  31 ++-
>>>  drivers/pci/controller/pcie-rockchip-ep.c     |  14 +-
>>>  drivers/pci/endpoint/functions/pci-epf-test.c |  29 +--
>>>  drivers/pci/endpoint/pci-epc-core.c           |   7 +-
>>>  drivers/pci/endpoint/pci-epc-mem.c            | 199 ++++++++++++++----
>>>  include/linux/pci-epc.h                       |  46 ++--
>>>  7 files changed, 245 insertions(+), 93 deletions(-)
>>>
>> .
>> .
>> <snip>
>> .
>> .
>>> diff --git a/drivers/pci/endpoint/pci-epc-core.c b/drivers/pci/endpoint/pci-epc-core.c
>>> index 2091508c1620..289c266c2d90 100644
>>> --- a/drivers/pci/endpoint/pci-epc-core.c
>>> +++ b/drivers/pci/endpoint/pci-epc-core.c
>>> @@ -358,13 +358,15 @@ EXPORT_SYMBOL_GPL(pci_epc_unmap_addr);
>>>   * @epc: the EPC device on which address is allocated
>>>   * @func_no: the endpoint function number in the EPC device
>>>   * @phys_addr: physical address of the local system
>>> + * @window: index to the window region where PCI address will be mapped
>>>   * @pci_addr: PCI address to which the physical address should be mapped
>>>   * @size: the size of the allocation
>>>   *
>>>   * Invoke to map CPU address with PCI address.
>>>   */
>>>  int pci_epc_map_addr(struct pci_epc *epc, u8 func_no,
>>> -                  phys_addr_t phys_addr, u64 pci_addr, size_t size)
>>> +                  phys_addr_t phys_addr, int window,
>>> +                  u64 pci_addr, size_t size)
>>>  {
>>>       int ret;
>>>       unsigned long flags;
>>> @@ -376,7 +378,8 @@ int pci_epc_map_addr(struct pci_epc *epc, u8 func_no,
>>>               return 0;
>>>
>>>       spin_lock_irqsave(&epc->lock, flags);
>>> -     ret = epc->ops->map_addr(epc, func_no, phys_addr, pci_addr, size);
>>> +     ret = epc->ops->map_addr(epc, func_no, phys_addr,
>>> +                              window, pci_addr, size);
>>>       spin_unlock_irqrestore(&epc->lock, flags);
>>>
>>>       return ret;
>>> diff --git a/drivers/pci/endpoint/pci-epc-mem.c b/drivers/pci/endpoint/pci-epc-mem.c
>>> index d2b174ce15de..f205f7819292 100644
>>> --- a/drivers/pci/endpoint/pci-epc-mem.c
>>> +++ b/drivers/pci/endpoint/pci-epc-mem.c
>>> @@ -38,57 +38,77 @@ 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,
>>> +                    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->mem_windows = 0;
>>>
>>> -     page_shift = ilog2(page_size);
>>> -     pages = size >> page_shift;
>>> -     bitmap_size = BITS_TO_LONGS(pages) * sizeof(long);
>>> +     if (!windows)
>>> +             return -EINVAL;
>>>
>>> -     mem = kzalloc(sizeof(*mem), GFP_KERNEL);
>>> -     if (!mem) {
>>> -             ret = -ENOMEM;
>>> -             goto err;
>>> -     }
>>> +     if (num_windows <= 0)
>>> +             return -EINVAL;
>>>
>>> -     bitmap = kzalloc(bitmap_size, GFP_KERNEL);
>>> -     if (!bitmap) {
>>> -             ret = -ENOMEM;
>>> -             goto err_mem;
>>> -     }
>>> +     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;
>>> +             }
>>>
>>> -     mem->bitmap = bitmap;
>>> -     mem->phys_base = phys_base;
>>> -     mem->page_size = page_size;
>>> -     mem->pages = pages;
>>> -     mem->size = size;
>>> +             bitmap = kzalloc(bitmap_size, GFP_KERNEL);
>>> +             if (!bitmap) {
>>> +                     ret = -ENOMEM;
>>> +                     goto err_mem;
>>> +             }
>>>
>>> -     epc->mem = mem;
>>> +             mem->bitmap = bitmap;
>>> +             mem->window.phys_base = windows[i].phys_base;
>>> +             mem->page_size = page_size;
>>> +             mem->pages = pages;
>>> +             mem->window.size = windows[i].size;
>>> +             mem->window.map_size = 0;
>>> +
>>> +             epc->mem[i] = mem;
>>> +     }
>>> +     epc->mem_windows = num_windows;
>>>
>>>       return 0;
>>>
>>>  err_mem:
>>> -     kfree(mem);
>>> +     for (; i >= 0; i--) {
>>
>> mem has to be reinitialized for every iteration of the loop.
> not sure what exactly you mean here, could you please elaborate.

You are invoking "kfree(mem->bitmap);" in a loop without re-initializing
mem. Refer pci_epc_mem_exit() where you are doing the free properly.

> 
>>> +             kfree(mem->bitmap);
>>> +             kfree(epc->mem[i]);
>>> +     }
>>> +     kfree(epc->mem);
>>>
>>> -err:
>>> -return ret;
>>> +     return ret;
>>>  }
>>>  EXPORT_SYMBOL_GPL(__pci_epc_mem_init);
>>>
>>> @@ -101,48 +121,127 @@ EXPORT_SYMBOL_GPL(__pci_epc_mem_init);
>>>   */
>>>  void pci_epc_mem_exit(struct pci_epc *epc)
>>>  {
>>> -     struct pci_epc_mem *mem = epc->mem;
>>> +     struct pci_epc_mem *mem;
>>> +     int i;
>>> +
>>> +     if (!epc->mem_windows)
>>> +             return;
>>> +
>>> +     for (i = 0; i <= epc->mem_windows; i++) {
>>> +             mem = epc->mem[i];

Missing the above line in the error handling above.


>>> +             kfree(mem->bitmap);
>>> +             kfree(epc->mem[i]);
>>> +     }

Thanks
Kishon