Re: [PATCH] PCI: endpoint: Migrate to Genalloc framework for outbound window memory allocation

[Kishon Vijay Abraham I]

Hi Mani,

On 3/17/2024 11:39 AM, Manivannan Sadhasivam wrote:
> As proposed during the last year 'PCI Endpoint Subsystem Open Items
> Discussion' of Linux Plumbers conference [1], let's migrate to Genalloc
> framework for managing the endpoint outbound window memory allocation.
>
> PCI Endpoint subsystem is using a custom memory allocator in pci-epc-mem
> driver from the start for managing the memory required to map the host
> address space (outbound) in endpoint. Even though it works well, it
> completely defeats the purpose of the 'Genalloc framework', a general
> purpose memory allocator framework created to avoid various custom memory
> allocators in the kernel.
>
> The migration to Genalloc framework is done is such a way that the existing
> API semantics are preserved. So that the callers of the EPC mem APIs do not
> need any modification (apart from the pcie-designware-epc driver that
> queries page size).
>
> Internally, the EPC mem driver now uses Genalloc framework's
> 'gen_pool_first_fit_order_align' algorithm that aligns the allocated memory
> based on the requested size as like the previous allocator. And the
> page size passed during pci_epc_mem_init() API is used as the minimum order
> for the memory allocations.
>
> During the migration, 'struct pci_epc_mem' is removed as it is seems
> redundant and the existing 'struct pci_epc_mem_window' in 'struct pci_epc'
> is now used to hold the address windows of the endpoint controller.
>
> [1] https://lpc.events/event/17/contributions/1419/

Thank you for working on this!
> Signed-off-by: Manivannan Sadhasivam <manivannan.sadhasivam@xxxxxxxxxx>
> ---
>   drivers/pci/controller/dwc/pcie-designware-ep.c |  14 +-
>   drivers/pci/endpoint/pci-epc-mem.c              | 182 +++++++++---------------
>   include/linux/pci-epc.h                         |  25 +---
>   3 files changed, 81 insertions(+), 140 deletions(-)
.
.
> diff --git a/drivers/pci/endpoint/pci-epc-mem.c b/drivers/pci/endpoint/pci-epc-mem.c
> index a9c028f58da1..f9e6e1a6aeaa 100644
> --- a/drivers/pci/endpoint/pci-epc-mem.c
> +++ b/drivers/pci/endpoint/pci-epc-mem.c
> @@ -4,37 +4,18 @@
>    *
>    * Copyright (C) 2017 Texas Instruments
>    * Author: Kishon Vijay Abraham I <kishon@xxxxxx>
> + *
> + * Copyright (C) 2024 Linaro Ltd.
> + * Author: Manivannan Sadhasivam <manivannan.sadhasivam@xxxxxxxxxx>
>    */
>   
> +#include <linux/genalloc.h>
>   #include <linux/io.h>
>   #include <linux/module.h>
>   #include <linux/slab.h>
>   
>   #include <linux/pci-epc.h>
>   
> -/**
> - * pci_epc_mem_get_order() - determine the allocation order of a memory size
> - * @mem: address space of the endpoint controller
> - * @size: the size for which to get the order
> - *
> - * Reimplement get_order() for mem->page_size since the generic get_order
> - * always gets order with a constant PAGE_SIZE.
> - */
> -static int pci_epc_mem_get_order(struct pci_epc_mem *mem, size_t size)
> -{
> -	int order;
> -	unsigned int page_shift = ilog2(mem->window.page_size);
> -
> -	size--;
> -	size >>= page_shift;
> -#if BITS_PER_LONG == 32
> -	order = fls(size);
> -#else
> -	order = fls64(size);
> -#endif
> -	return order;
> -}
> -
>   /**
>    * pci_epc_multi_mem_init() - initialize the pci_epc_mem structure
>    * @epc: the EPC device that invoked pci_epc_mem_init
> @@ -48,17 +29,11 @@ int pci_epc_multi_mem_init(struct pci_epc *epc,
>   			   struct pci_epc_mem_window *windows,
>   			   unsigned int num_windows)
>   {
> -	struct pci_epc_mem *mem = NULL;
> -	unsigned long *bitmap = NULL;
> -	unsigned int page_shift;
> +	struct pci_epc_mem_window *window = NULL;
>   	size_t page_size;
> -	int bitmap_size;
> -	int pages;
>   	int ret;
>   	int i;
>   
> -	epc->num_windows = 0;
> -
>   	if (!windows || !num_windows)
>   		return -EINVAL;
>   
> @@ -70,45 +45,51 @@ int pci_epc_multi_mem_init(struct pci_epc *epc,
>   		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) {
> +		windows[i].pool = gen_pool_create(ilog2(page_size), -1);
> +		if (!windows[i].pool) {
>   			ret = -ENOMEM;
> -			i--;
> -			goto err_mem;
> +			goto err_free_mem;
> +		}
> +
> +		gen_pool_set_algo(windows[i].pool, gen_pool_first_fit_order_align,
> +				  NULL);
> +
> +		windows[i].virt_base = ioremap(windows[i].phys_base, windows[i].size);

Do you have to ioremap upfront the entire window? This could be a 
problem in 32-bit systems which has limited vmalloc space. I have faced 
issues before when trying to map the entire memory window and had to 
manipulate vmalloc boot parameter.

I'd prefer we find a way to do ioremap per allocation as before.

Thanks,
Kishon