[PATCH v4 0/7] Improve PCI memory mapping API

[PATCH v4 0/7] Improve PCI memory mapping API

[Damien Le Moal]

This series introduces the new functions pci_epc_map_align(),
pci_epc_mem_map() and pci_epc_mem_unmap() to improve handling of the
PCI address mapping alignment constraints of endpoint controllers in a
controller independent manner.

The issue fixed is that the fixed alignment defined by the "align" field
of struct pci_epc_features is defined for inbound ATU entries (e.g.
BARs) and is a fixed value, whereas some controllers need a PCI address
alignment that depends on the PCI address itself. For instance, the
rk3399 SoC controller in endpoint mode uses the lower bits of the local
endpoint memory address as the lower bits for the PCI addresses for data
transfers. That is, when mapping local memory, one must take into
account the number of bits of the RC PCI address that change from the
start address of the mapping.

To fix this, the new endpoint controller method .map_align is introduced
and called from pci_epc_map_align(). This method is optional and for
controllers that do not define it, it is assumed that the controller has
no PCI address constraint.

The functions pci_epc_mem_map() is a helper function which obtains
mapping information, allocates endpoint controller memory according to
the mapping size obtained and maps the memory. pci_epc_mem_unmap()
unmaps and frees the endpoint memory.

This series is organized as follows:
 - Patch 1 introduces a small helper to clean up the epc core code
 - Patch 2 improves pci_epc_mem_alloc_addr()
 - Patch 3 and 4 introduce the new map_align endpoint controller method
   and the epc functions pci_epc_mem_map() and pci_epc_mem_unmap().
 - Patch 5 documents these new functions.
 - Patch 6 modifies the test endpoint function driver to use 
   pci_epc_mem_map() and pci_epc_mem_unmap() to illustrate the use of
   these functions.
 - Finally, patch 7 implements the rk3588 endpoint controller driver
   .map_align operation to satisfy that controller PCI address
   alignment constraint.

Changes from v3:
 - Addressed Niklas comments (improved patch 2 commit message, added
   comments in the pci_epc_map_align() function in patch 3, typos and
   improvements in patch 5, patch 7 commit message).
 - Added review tags

Changes from v2:
 - Dropped all patches for the rockchip-ep. These patches will be sent
   later as a separate series.
 - Added patch 2 and 5
 - Added review tags to patch 1

Changes from v1:
 - Changed pci_epc_check_func() to pci_epc_function_is_valid() in patch
   1.
 - Removed patch "PCI: endpoint: Improve pci_epc_mem_alloc_addr()"
   (former patch 2 of v1)
 - Various typos cleanups all over. Also fixed some blank space
   indentation.
 - Added review tags

Damien Le Moal (7):
  PCI: endpoint: Introduce pci_epc_function_is_valid()
  PCI: endpoint: Improve pci_epc_mem_alloc_addr()
  PCI: endpoint: Introduce pci_epc_map_align()
  PCI: endpoint: Introduce pci_epc_mem_map()/unmap()
  PCI: endpoint: Update documentation
  PCI: endpoint: test: Use pci_epc_mem_map/unmap()
  PCI: dwc: endpoint: Define the .map_align() controller operation

 Documentation/PCI/endpoint/pci-endpoint.rst   |  35 ++
 .../pci/controller/dwc/pcie-designware-ep.c   |  15 +
 drivers/pci/endpoint/functions/pci-epf-test.c | 372 +++++++++---------
 drivers/pci/endpoint/pci-epc-core.c           | 223 ++++++++---
 drivers/pci/endpoint/pci-epc-mem.c            |   9 +-
 include/linux/pci-epc.h                       |  41 ++
 6 files changed, 465 insertions(+), 230 deletions(-)

-- 
2.46.2

[PATCH v4 1/7] PCI: endpoint: Introduce pci_epc_function_is_valid()

[Damien Le Moal]

Introduce the epc core helper function pci_epc_function_is_valid() to
verify that an epc pointer, a physical function number and a virtual
function number are all valid. This avoids repeating the code pattern:

if (IS_ERR_OR_NULL(epc) || func_no >= epc->max_functions)
	return err;

if (vfunc_no > 0 && (!epc->max_vfs || vfunc_no > epc->max_vfs[func_no]))
	return err;

in many functions of the endpoint controller core code.

Signed-off-by: Damien Le Moal <dlemoal@kernel.org>
Reviewed-by: Manivannan Sadhasivam <manivannan.sadhasivam@linaro.org>
Reviewed-by: Niklas Cassel <cassel@kernel.org>
---
 drivers/pci/endpoint/pci-epc-core.c | 79 +++++++++++------------------
 1 file changed, 31 insertions(+), 48 deletions(-)

diff --git a/drivers/pci/endpoint/pci-epc-core.c b/drivers/pci/endpoint/pci-epc-core.c
index 17f007109255..b854f1bab26f 100644
--- a/drivers/pci/endpoint/pci-epc-core.c
+++ b/drivers/pci/endpoint/pci-epc-core.c
@@ -128,6 +128,18 @@ enum pci_barno pci_epc_get_next_free_bar(const struct pci_epc_features
 }
 EXPORT_SYMBOL_GPL(pci_epc_get_next_free_bar);
 
+static bool pci_epc_function_is_valid(struct pci_epc *epc,
+				      u8 func_no, u8 vfunc_no)
+{
+	if (IS_ERR_OR_NULL(epc) || func_no >= epc->max_functions)
+		return false;
+
+	if (vfunc_no > 0 && (!epc->max_vfs || vfunc_no > epc->max_vfs[func_no]))
+		return false;
+
+	return true;
+}
+
 /**
  * pci_epc_get_features() - get the features supported by EPC
  * @epc: the features supported by *this* EPC device will be returned
@@ -145,10 +157,7 @@ const struct pci_epc_features *pci_epc_get_features(struct pci_epc *epc,
 {
 	const struct pci_epc_features *epc_features;
 
-	if (IS_ERR_OR_NULL(epc) || func_no >= epc->max_functions)
-		return NULL;
-
-	if (vfunc_no > 0 && (!epc->max_vfs || vfunc_no > epc->max_vfs[func_no]))
+	if (!pci_epc_function_is_valid(epc, func_no, vfunc_no))
 		return NULL;
 
 	if (!epc->ops->get_features)
@@ -218,10 +227,7 @@ int pci_epc_raise_irq(struct pci_epc *epc, u8 func_no, u8 vfunc_no,
 {
 	int ret;
 
-	if (IS_ERR_OR_NULL(epc) || func_no >= epc->max_functions)
-		return -EINVAL;
-
-	if (vfunc_no > 0 && (!epc->max_vfs || vfunc_no > epc->max_vfs[func_no]))
+	if (!pci_epc_function_is_valid(epc, func_no, vfunc_no))
 		return -EINVAL;
 
 	if (!epc->ops->raise_irq)
@@ -262,10 +268,7 @@ int pci_epc_map_msi_irq(struct pci_epc *epc, u8 func_no, u8 vfunc_no,
 {
 	int ret;
 
-	if (IS_ERR_OR_NULL(epc))
-		return -EINVAL;
-
-	if (vfunc_no > 0 && (!epc->max_vfs || vfunc_no > epc->max_vfs[func_no]))
+	if (!pci_epc_function_is_valid(epc, func_no, vfunc_no))
 		return -EINVAL;
 
 	if (!epc->ops->map_msi_irq)
@@ -293,10 +296,7 @@ int pci_epc_get_msi(struct pci_epc *epc, u8 func_no, u8 vfunc_no)
 {
 	int interrupt;
 
-	if (IS_ERR_OR_NULL(epc) || func_no >= epc->max_functions)
-		return 0;
-
-	if (vfunc_no > 0 && (!epc->max_vfs || vfunc_no > epc->max_vfs[func_no]))
+	if (!pci_epc_function_is_valid(epc, func_no, vfunc_no))
 		return 0;
 
 	if (!epc->ops->get_msi)
@@ -329,11 +329,10 @@ int pci_epc_set_msi(struct pci_epc *epc, u8 func_no, u8 vfunc_no, u8 interrupts)
 	int ret;
 	u8 encode_int;
 
-	if (IS_ERR_OR_NULL(epc) || func_no >= epc->max_functions ||
-	    interrupts < 1 || interrupts > 32)
+	if (!pci_epc_function_is_valid(epc, func_no, vfunc_no))
 		return -EINVAL;
 
-	if (vfunc_no > 0 && (!epc->max_vfs || vfunc_no > epc->max_vfs[func_no]))
+	if (interrupts < 1 || interrupts > 32)
 		return -EINVAL;
 
 	if (!epc->ops->set_msi)
@@ -361,10 +360,7 @@ int pci_epc_get_msix(struct pci_epc *epc, u8 func_no, u8 vfunc_no)
 {
 	int interrupt;
 
-	if (IS_ERR_OR_NULL(epc) || func_no >= epc->max_functions)
-		return 0;
-
-	if (vfunc_no > 0 && (!epc->max_vfs || vfunc_no > epc->max_vfs[func_no]))
+	if (!pci_epc_function_is_valid(epc, func_no, vfunc_no))
 		return 0;
 
 	if (!epc->ops->get_msix)
@@ -397,11 +393,10 @@ int pci_epc_set_msix(struct pci_epc *epc, u8 func_no, u8 vfunc_no,
 {
 	int ret;
 
-	if (IS_ERR_OR_NULL(epc) || func_no >= epc->max_functions ||
-	    interrupts < 1 || interrupts > 2048)
+	if (!pci_epc_function_is_valid(epc, func_no, vfunc_no))
 		return -EINVAL;
 
-	if (vfunc_no > 0 && (!epc->max_vfs || vfunc_no > epc->max_vfs[func_no]))
+	if (interrupts < 1 || interrupts > 2048)
 		return -EINVAL;
 
 	if (!epc->ops->set_msix)
@@ -428,10 +423,7 @@ EXPORT_SYMBOL_GPL(pci_epc_set_msix);
 void pci_epc_unmap_addr(struct pci_epc *epc, u8 func_no, u8 vfunc_no,
 			phys_addr_t phys_addr)
 {
-	if (IS_ERR_OR_NULL(epc) || func_no >= epc->max_functions)
-		return;
-
-	if (vfunc_no > 0 && (!epc->max_vfs || vfunc_no > epc->max_vfs[func_no]))
+	if (!pci_epc_function_is_valid(epc, func_no, vfunc_no))
 		return;
 
 	if (!epc->ops->unmap_addr)
@@ -459,10 +451,7 @@ int pci_epc_map_addr(struct pci_epc *epc, u8 func_no, u8 vfunc_no,
 {
 	int ret;
 
-	if (IS_ERR_OR_NULL(epc) || func_no >= epc->max_functions)
-		return -EINVAL;
-
-	if (vfunc_no > 0 && (!epc->max_vfs || vfunc_no > epc->max_vfs[func_no]))
+	if (!pci_epc_function_is_valid(epc, func_no, vfunc_no))
 		return -EINVAL;
 
 	if (!epc->ops->map_addr)
@@ -489,12 +478,11 @@ EXPORT_SYMBOL_GPL(pci_epc_map_addr);
 void pci_epc_clear_bar(struct pci_epc *epc, u8 func_no, u8 vfunc_no,
 		       struct pci_epf_bar *epf_bar)
 {
-	if (IS_ERR_OR_NULL(epc) || func_no >= epc->max_functions ||
-	    (epf_bar->barno == BAR_5 &&
-	     epf_bar->flags & PCI_BASE_ADDRESS_MEM_TYPE_64))
+	if (!pci_epc_function_is_valid(epc, func_no, vfunc_no))
 		return;
 
-	if (vfunc_no > 0 && (!epc->max_vfs || vfunc_no > epc->max_vfs[func_no]))
+	if (epf_bar->barno == BAR_5 &&
+	    epf_bar->flags & PCI_BASE_ADDRESS_MEM_TYPE_64)
 		return;
 
 	if (!epc->ops->clear_bar)
@@ -521,18 +509,16 @@ int pci_epc_set_bar(struct pci_epc *epc, u8 func_no, u8 vfunc_no,
 	int ret;
 	int flags = epf_bar->flags;
 
-	if (IS_ERR_OR_NULL(epc) || func_no >= epc->max_functions ||
-	    (epf_bar->barno == BAR_5 &&
-	     flags & PCI_BASE_ADDRESS_MEM_TYPE_64) ||
+	if (!pci_epc_function_is_valid(epc, func_no, vfunc_no))
+		return -EINVAL;
+
+	if ((epf_bar->barno == BAR_5 && flags & PCI_BASE_ADDRESS_MEM_TYPE_64) ||
 	    (flags & PCI_BASE_ADDRESS_SPACE_IO &&
 	     flags & PCI_BASE_ADDRESS_IO_MASK) ||
 	    (upper_32_bits(epf_bar->size) &&
 	     !(flags & PCI_BASE_ADDRESS_MEM_TYPE_64)))
 		return -EINVAL;
 
-	if (vfunc_no > 0 && (!epc->max_vfs || vfunc_no > epc->max_vfs[func_no]))
-		return -EINVAL;
-
 	if (!epc->ops->set_bar)
 		return 0;
 
@@ -561,10 +547,7 @@ int pci_epc_write_header(struct pci_epc *epc, u8 func_no, u8 vfunc_no,
 {
 	int ret;
 
-	if (IS_ERR_OR_NULL(epc) || func_no >= epc->max_functions)
-		return -EINVAL;
-
-	if (vfunc_no > 0 && (!epc->max_vfs || vfunc_no > epc->max_vfs[func_no]))
+	if (!pci_epc_function_is_valid(epc, func_no, vfunc_no))
 		return -EINVAL;
 
 	/* Only Virtual Function #1 has deviceID */
-- 
2.46.2

[PATCH v4 2/7] PCI: endpoint: Improve pci_epc_mem_alloc_addr()

[Damien Le Moal]

There is no point in attempting to allocate memory from an endpoint
controller memory window if the requested size is larger than the memory
window size. Add a check to skip bitmap_find_free_region() calls for
such case. This check can be done without the mem->lock mutex held as
memory window sizes are constant and never modified at runtime.
Also change the final return to return NULL to simplify the code.

Signed-off-by: Damien Le Moal <dlemoal@kernel.org>
---
 drivers/pci/endpoint/pci-epc-mem.c | 9 ++++++---
 1 file changed, 6 insertions(+), 3 deletions(-)

diff --git a/drivers/pci/endpoint/pci-epc-mem.c b/drivers/pci/endpoint/pci-epc-mem.c
index a9c028f58da1..218a60e945db 100644
--- a/drivers/pci/endpoint/pci-epc-mem.c
+++ b/drivers/pci/endpoint/pci-epc-mem.c
@@ -178,7 +178,7 @@ 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)
 {
-	void __iomem *virt_addr = NULL;
+	void __iomem *virt_addr;
 	struct pci_epc_mem *mem;
 	unsigned int page_shift;
 	size_t align_size;
@@ -188,10 +188,13 @@ void __iomem *pci_epc_mem_alloc_addr(struct pci_epc *epc,
 
 	for (i = 0; i < epc->num_windows; i++) {
 		mem = epc->windows[i];
-		mutex_lock(&mem->lock);
+		if (size > mem->window.size)
+			continue;
+
 		align_size = ALIGN(size, mem->window.page_size);
 		order = pci_epc_mem_get_order(mem, align_size);
 
+		mutex_lock(&mem->lock);
 		pageno = bitmap_find_free_region(mem->bitmap, mem->pages,
 						 order);
 		if (pageno >= 0) {
@@ -211,7 +214,7 @@ void __iomem *pci_epc_mem_alloc_addr(struct pci_epc *epc,
 		mutex_unlock(&mem->lock);
 	}
 
-	return virt_addr;
+	return NULL;
 }
 EXPORT_SYMBOL_GPL(pci_epc_mem_alloc_addr);
 
-- 
2.46.2

[PATCH v4 3/7] PCI: endpoint: Introduce pci_epc_map_align()

[Damien Le Moal]

Some endpoint controllers have requirements on the alignment of the
controller physical memory address that must be used to map a RC PCI
address region. For instance, the rockchip endpoint controller uses
at most the lower 20 bits of a physical memory address region as the
lower bits of an RC PCI address. For mapping a PCI address region of
size bytes starting from pci_addr, the exact number of address bits
used is the number of address bits changing in the address range
[pci_addr..pci_addr + size - 1].

For this example, this creates the following constraints:
1) The offset into the controller physical memory allocated for a
   mapping depends on the mapping size *and* the starting PCI address
   for the mapping.
2) A mapping size cannot exceed the controller windows size (1MB) minus
   the offset needed into the allocated physical memory, which can end
   up being a smaller size than the desired mapping size.

Handling these constraints independently of the controller being used
in an endpoint function driver is not possible with the current EPC
API as only the ->align field in struct pci_epc_features is provided
and used for BAR (inbound ATU mappings) mapping. A new API is needed
for function drivers to discover mapping constraints and handle
non-static requirements based on the RC PCI address range to access.

Introduce the function pci_epc_map_align() and the endpoint controller
operation ->map_align to allow endpoint function drivers to obtain the
size and the offset into a controller address region that must be
allocated and mapped to access an RC PCI address region. The size
of the mapping provided by pci_epc_map_align() can then be used as the
size argument for the function pci_epc_mem_alloc_addr().
The offset into the allocated controller memory provided can be used to
correctly handle data transfers.

For endpoint controllers that have PCI address alignment constraints,
pci_epc_map_align() may indicate upon return an effective PCI address
region mapping size that is smaller (but not 0) than the requested PCI
address region size. For such case, an endpoint function driver must
handle data accesses over the desired PCI address range in fragments,
by repeatedly using pci_epc_map_align() over the PCI address range.

The controller operation ->map_align is optional: controllers that do
not have any alignment constraints for mapping a RC PCI address region
do not need to implement this operation. For such controllers,
pci_epc_map_align() always returns the mapping size as equal to the
requested size of the PCI region and an offset equal to 0.

The new structure struct pci_epc_map is introduced to represent a
mapping start PCI address, mapping effective size, the size and offset
into the controller memory needed for mapping the PCI address region as
well as the physical and virtual CPU addresses of the mapping (phys_base
and virt_base fields). For convenience, the physical and virtual CPU
addresses within that mapping to access the target RC PCI address region
are also provided (phys_addr and virt_addr fields).

Co-developed-by: Rick Wertenbroek <rick.wertenbroek@gmail.com>
Signed-off-by: Rick Wertenbroek <rick.wertenbroek@gmail.com>
Signed-off-by: Damien Le Moal <dlemoal@kernel.org>
Reviewed-by: Niklas Cassel <cassel@kernel.org>
---
 drivers/pci/endpoint/pci-epc-core.c | 66 +++++++++++++++++++++++++++++
 include/linux/pci-epc.h             | 37 ++++++++++++++++
 2 files changed, 103 insertions(+)

diff --git a/drivers/pci/endpoint/pci-epc-core.c b/drivers/pci/endpoint/pci-epc-core.c
index b854f1bab26f..1adccf07c33e 100644
--- a/drivers/pci/endpoint/pci-epc-core.c
+++ b/drivers/pci/endpoint/pci-epc-core.c
@@ -435,6 +435,72 @@ void pci_epc_unmap_addr(struct pci_epc *epc, u8 func_no, u8 vfunc_no,
 }
 EXPORT_SYMBOL_GPL(pci_epc_unmap_addr);
 
+/**
+ * pci_epc_map_align() - Get the offset into and the size of a controller memory
+ *			 address region needed to map a RC PCI address region
+ * @epc: the EPC device on which address is allocated
+ * @func_no: the physical endpoint function number in the EPC device
+ * @vfunc_no: the virtual endpoint function number in the physical function
+ * @pci_addr: The RC PCI address to map
+ * @pci_size: the number of bytes to map starting from @pci_addr
+ * @map: populate here the actual size and offset into the controller memory
+ *       that must be allocated for the mapping
+ *
+ * If defined, invoke the controller map_align operation to obtain the size and
+ * the offset into a controller address region that must be allocated to map
+ * @pci_size bytes of the RC PCI address space starting from @pci_addr.
+ *
+ * On return, @map->pci_size indicates the effective size of the mapping that
+ * can be handled by the controller. This size may be smaller than the requested
+ * @pci_size. In such case, the endpoint function driver must handle the mapping
+ * using several fragments. The offset into the controller memory for the
+ * effective mapping of the RC PCI address range
+ * @pci_addr..@pci_addr+@map->pci_size is indicated by @map->map_ofst.
+ *
+ * If the target controller does not define a map_align operation, it is assumed
+ * that the controller has no PCI address mapping alignment constraint.
+ */
+int pci_epc_map_align(struct pci_epc *epc, u8 func_no, u8 vfunc_no,
+		      u64 pci_addr, size_t pci_size, struct pci_epc_map *map)
+{
+	int ret;
+
+	if (!pci_epc_function_is_valid(epc, func_no, vfunc_no))
+		return -EINVAL;
+
+	if (!pci_size || !map)
+		return -EINVAL;
+
+	/*
+	 * Initialize and remember the PCI address region to be mapped. The
+	 * controller ->map_align() operation may change the map->pci_size to a
+	 * smaller value.
+	 */
+	memset(map, 0, sizeof(*map));
+	map->pci_addr = pci_addr;
+	map->pci_size = pci_size;
+
+	if (!epc->ops->map_align) {
+		/*
+		 * Assume that the EP controller has no alignment constraint,
+		 * that is, that the PCI address to map and the size of the
+		 * controller memory needed for the mapping are the same as
+		 * specified by the caller.
+		 */
+		map->map_pci_addr = pci_addr;
+		map->map_size = pci_size;
+		map->map_ofst = 0;
+		return 0;
+	}
+
+	mutex_lock(&epc->lock);
+	ret = epc->ops->map_align(epc, func_no, vfunc_no, map);
+	mutex_unlock(&epc->lock);
+
+	return ret;
+}
+EXPORT_SYMBOL_GPL(pci_epc_map_align);
+
 /**
  * pci_epc_map_addr() - map CPU address to PCI address
  * @epc: the EPC device on which address is allocated
diff --git a/include/linux/pci-epc.h b/include/linux/pci-epc.h
index 42ef06136bd1..9df8a83e8d10 100644
--- a/include/linux/pci-epc.h
+++ b/include/linux/pci-epc.h
@@ -32,11 +32,44 @@ pci_epc_interface_string(enum pci_epc_interface_type type)
 	}
 }
 
+/**
+ * struct pci_epc_map - information about EPC memory for mapping a RC PCI
+ *                      address range
+ * @pci_addr: start address of the RC PCI address range to map
+ * @pci_size: size of the RC PCI address range mapped from @pci_addr
+ * @map_pci_addr: RC PCI address used as the first address mapped (may be lower
+ *                than @pci_addr)
+ * @map_size: size of the controller memory needed for mapping the RC PCI address
+ *            range @pci_addr..@pci_addr+@pci_size
+ * @map_ofst: offset into the mapped controller memory to access @pci_addr
+ * @phys_base: base physical address of the allocated EPC memory for mapping the
+ *             RC PCI address range
+ * @phys_addr: physical address at which @pci_addr is mapped
+ * @virt_base: base virtual address of the allocated EPC memory for mapping the
+ *             RC PCI address range
+ * @virt_addr: virtual address at which @pci_addr is mapped
+ */
+struct pci_epc_map {
+	phys_addr_t	pci_addr;
+	size_t		pci_size;
+
+	phys_addr_t	map_pci_addr;
+	size_t		map_size;
+	phys_addr_t	map_ofst;
+
+	phys_addr_t	phys_base;
+	phys_addr_t	phys_addr;
+	void __iomem	*virt_base;
+	void __iomem	*virt_addr;
+};
+
 /**
  * struct pci_epc_ops - set of function pointers for performing EPC operations
  * @write_header: ops to populate configuration space header
  * @set_bar: ops to configure the BAR
  * @clear_bar: ops to reset the BAR
+ * @map_align: operation to get the size and offset into a controller memory
+ *             window needed to map an RC PCI address region
  * @map_addr: ops to map CPU address to PCI address
  * @unmap_addr: ops to unmap CPU address and PCI address
  * @set_msi: ops to set the requested number of MSI interrupts in the MSI
@@ -61,6 +94,8 @@ struct pci_epc_ops {
 			   struct pci_epf_bar *epf_bar);
 	void	(*clear_bar)(struct pci_epc *epc, u8 func_no, u8 vfunc_no,
 			     struct pci_epf_bar *epf_bar);
+	int	(*map_align)(struct pci_epc *epc, u8 func_no, u8 vfunc_no,
+			    struct pci_epc_map *map);
 	int	(*map_addr)(struct pci_epc *epc, u8 func_no, u8 vfunc_no,
 			    phys_addr_t addr, u64 pci_addr, size_t size);
 	void	(*unmap_addr)(struct pci_epc *epc, u8 func_no, u8 vfunc_no,
@@ -241,6 +276,8 @@ int pci_epc_set_bar(struct pci_epc *epc, u8 func_no, u8 vfunc_no,
 		    struct pci_epf_bar *epf_bar);
 void pci_epc_clear_bar(struct pci_epc *epc, u8 func_no, u8 vfunc_no,
 		       struct pci_epf_bar *epf_bar);
+int pci_epc_map_align(struct pci_epc *epc, u8 func_no, u8 vfunc_no,
+		      u64 pci_addr, size_t size, struct pci_epc_map *map);
 int pci_epc_map_addr(struct pci_epc *epc, u8 func_no, u8 vfunc_no,
 		     phys_addr_t phys_addr,
 		     u64 pci_addr, size_t size);
-- 
2.46.2

[PATCH v4 4/7] PCI: endpoint: Introduce pci_epc_mem_map()/unmap()

[Damien Le Moal]

Introduce the function pci_epc_mem_map() to facilitate controller memory
address allocation and mapping to a RC PCI address region in endpoint
function drivers.

This function first uses pci_epc_map_align() to determine the controller
memory address size (and offset into) depending on the controller
address alignment constraints. The result of this function is used to
allocate a controller physical memory region using
pci_epc_mem_alloc_addr() and map that memory to the RC PCI address
space with pci_epc_map_addr().

Since pci_epc_map_align() may indicate that the effective mapping
of a PCI address region is smaller than the user requested size,
pci_epc_mem_map() may only partially map the RC PCI address region
specified. It is the responsibility of the caller (an endpoint function
driver) to handle such smaller mapping.

The counterpart of pci_epc_mem_map() to unmap and free the controller
memory address region is pci_epc_mem_unmap().

Both functions operate using a struct pci_epc_map data structure
Endpoint function drivers can use struct pci_epc_map to access the
mapped RC PCI address region using the ->virt_addr and ->pci_size
fields.

Co-developed-by: Rick Wertenbroek <rick.wertenbroek@gmail.com>
Signed-off-by: Rick Wertenbroek <rick.wertenbroek@gmail.com>
Signed-off-by: Damien Le Moal <dlemoal@kernel.org>
Reviewed-by: Niklas Cassel <cassel@kernel.org>
---
 drivers/pci/endpoint/pci-epc-core.c | 78 +++++++++++++++++++++++++++++
 include/linux/pci-epc.h             |  4 ++
 2 files changed, 82 insertions(+)

diff --git a/drivers/pci/endpoint/pci-epc-core.c b/drivers/pci/endpoint/pci-epc-core.c
index 1adccf07c33e..d03c753d0a53 100644
--- a/drivers/pci/endpoint/pci-epc-core.c
+++ b/drivers/pci/endpoint/pci-epc-core.c
@@ -532,6 +532,84 @@ int pci_epc_map_addr(struct pci_epc *epc, u8 func_no, u8 vfunc_no,
 }
 EXPORT_SYMBOL_GPL(pci_epc_map_addr);
 
+/**
+ * pci_epc_mem_map() - allocate and map a PCI address to a CPU address
+ * @epc: the EPC device on which the CPU address is to be allocated and mapped
+ * @func_no: the physical endpoint function number in the EPC device
+ * @vfunc_no: the virtual endpoint function number in the physical function
+ * @pci_addr: PCI address to which the CPU address should be mapped
+ * @pci_size: the number of bytes to map starting from @pci_addr
+ * @map: where to return the mapping information
+ *
+ * Allocate a controller memory address region and map it to a RC PCI address
+ * region, taking into account the controller physical address mapping
+ * constraints using pci_epc_map_align().
+ * The effective size of the PCI address range mapped from @pci_addr is
+ * indicated by @map->pci_size. This size may be less than the requested
+ * @pci_size. The local virtual CPU address for the mapping is indicated by
+ * @map->virt_addr (@map->phys_addr indicates the physical address).
+ * The size and CPU address of the controller memory allocated and mapped are
+ * respectively indicated by @map->map_size and @map->virt_base (and
+ * @map->phys_base).
+ *
+ * Returns 0 on success and a negative error code in case of error.
+ */
+int pci_epc_mem_map(struct pci_epc *epc, u8 func_no, u8 vfunc_no,
+		    u64 pci_addr, size_t pci_size, struct pci_epc_map *map)
+{
+	int ret;
+
+	ret = pci_epc_map_align(epc, func_no, vfunc_no, pci_addr, pci_size, map);
+	if (ret)
+		return ret;
+
+	map->virt_base = pci_epc_mem_alloc_addr(epc, &map->phys_base,
+						map->map_size);
+	if (!map->virt_base)
+		return -ENOMEM;
+
+	map->phys_addr = map->phys_base + map->map_ofst;
+	map->virt_addr = map->virt_base + map->map_ofst;
+
+	ret = pci_epc_map_addr(epc, func_no, vfunc_no, map->phys_base,
+			       map->map_pci_addr, map->map_size);
+	if (ret) {
+		pci_epc_mem_free_addr(epc, map->phys_base, map->virt_base,
+				      map->map_size);
+		map->virt_base = 0;
+		return ret;
+	}
+
+	return 0;
+}
+EXPORT_SYMBOL_GPL(pci_epc_mem_map);
+
+/**
+ * pci_epc_mem_unmap() - unmap and free a CPU address region
+ * @epc: the EPC device on which the CPU address is allocated and mapped
+ * @func_no: the physical endpoint function number in the EPC device
+ * @vfunc_no: the virtual endpoint function number in the physical function
+ * @map: the mapping information
+ *
+ * Unmap and free a CPU address region that was allocated and mapped with
+ * pci_epc_mem_map().
+ */
+void pci_epc_mem_unmap(struct pci_epc *epc, u8 func_no, u8 vfunc_no,
+		       struct pci_epc_map *map)
+{
+	if (!pci_epc_function_is_valid(epc, func_no, vfunc_no))
+		return;
+
+	if (!map || !map->virt_base)
+		return;
+
+	pci_epc_unmap_addr(epc, func_no, vfunc_no, map->phys_base);
+	pci_epc_mem_free_addr(epc, map->phys_base, map->virt_base,
+			      map->map_size);
+	map->map_size = 0;
+}
+EXPORT_SYMBOL_GPL(pci_epc_mem_unmap);
+
 /**
  * pci_epc_clear_bar() - reset the BAR
  * @epc: the EPC device for which the BAR has to be cleared
diff --git a/include/linux/pci-epc.h b/include/linux/pci-epc.h
index 9df8a83e8d10..97d2fbb740fd 100644
--- a/include/linux/pci-epc.h
+++ b/include/linux/pci-epc.h
@@ -315,6 +315,10 @@ void __iomem *pci_epc_mem_alloc_addr(struct pci_epc *epc,
 				     phys_addr_t *phys_addr, size_t size);
 void pci_epc_mem_free_addr(struct pci_epc *epc, phys_addr_t phys_addr,
 			   void __iomem *virt_addr, size_t size);
+int pci_epc_mem_map(struct pci_epc *epc, u8 func_no, u8 vfunc_no,
+		    u64 pci_addr, size_t pci_size, struct pci_epc_map *map);
+void pci_epc_mem_unmap(struct pci_epc *epc, u8 func_no, u8 vfunc_no,
+		       struct pci_epc_map *map);
 
 #else
 static inline void pci_epc_init_notify(struct pci_epc *epc)
-- 
2.46.2

[PATCH v4 5/7] PCI: endpoint: Update documentation

[Damien Le Moal]

Document the new functions pci_epc_map_align(), pci_epc_mem_map() and
pci_epc_mem_unmap(). Also add the documentation for the functions
pci_epc_map_addr() and pci_epc_unmap_addr() that were missing.

Signed-off-by: Damien Le Moal <dlemoal@kernel.org>
Reviewed-by: Niklas Cassel <cassel@kernel.org>
---
 Documentation/PCI/endpoint/pci-endpoint.rst | 35 +++++++++++++++++++++
 1 file changed, 35 insertions(+)

diff --git a/Documentation/PCI/endpoint/pci-endpoint.rst b/Documentation/PCI/endpoint/pci-endpoint.rst
index 21507e3cc238..8e86cd209f9a 100644
--- a/Documentation/PCI/endpoint/pci-endpoint.rst
+++ b/Documentation/PCI/endpoint/pci-endpoint.rst
@@ -117,6 +117,41 @@ by the PCI endpoint function driver.
    The PCI endpoint function driver should use pci_epc_mem_free_addr() to
    free the memory space allocated using pci_epc_mem_alloc_addr().
 
+* pci_epc_map_align()
+
+   A PCI endpoint controller may impose constraints on the RC PCI addresses
+   that can be mapped. The function pci_epc_map_align() allows endpoint
+   function drivers to discover and handle such constraints by providing the
+   size of the memory that must be allocated with pci_epc_mem_alloc_addr()
+   for successfully mapping any RC PCI address. This function will also
+   indicate the size of the PCI address range that can be mapped, which can be
+   less than the requested size, as well as the offset into the allocated
+   memory to use for accessing the RC PCI address range that can be mapped.
+
+* pci_epc_map_addr()
+
+  A PCI endpoint function driver should use pci_epc_map_addr() to map to a RC
+  PCI address the CPU address of local memory obtained with
+  pci_epc_mem_alloc_addr().
+
+* pci_epc_unmap_addr()
+
+  A PCI endpoint function driver should use pci_epc_unmap_addr() to unmap the
+  CPU address of local memory mapped to a RC address with pci_epc_map_addr().
+
+* pci_epc_mem_map()
+
+  A PCI endpoint function driver can use pci_epc_mem_map() to allocate and map
+  a RC PCI address range. This function combines calls to pci_epc_map_align(),
+  pci_epc_mem_alloc_addr() and pci_epc_mem_alloc_addr() into a single function
+  to simplify the PCI address mapping handling in endpoint function drivers.
+
+* pci_epc_mem_unmap()
+
+  A PCI endpoint function driver can use pci_epc_mem_unmap() to unmap and free
+  memory that was allocated and mapped using pci_epc_mem_map().
+
+
 Other EPC APIs
 ~~~~~~~~~~~~~~
 
-- 
2.46.2

[PATCH v4 6/7] PCI: endpoint: test: Use pci_epc_mem_map/unmap()

[Damien Le Moal]

Modify the endpoint test driver to use the functions pci_epc_mem_map()
and pci_epc_mem_unmap() for the read, write and copy tests. For each
test case, the transfer (dma or mmio) are executed in a loop to ensure
that potentially partial mappings returned by pci_epc_mem_map() are
correctly handled.

Signed-off-by: Damien Le Moal <dlemoal@kernel.org>
---
 drivers/pci/endpoint/functions/pci-epf-test.c | 372 +++++++++---------
 1 file changed, 193 insertions(+), 179 deletions(-)

diff --git a/drivers/pci/endpoint/functions/pci-epf-test.c b/drivers/pci/endpoint/functions/pci-epf-test.c
index 7c2ed6eae53a..a73bc0771d35 100644
--- a/drivers/pci/endpoint/functions/pci-epf-test.c
+++ b/drivers/pci/endpoint/functions/pci-epf-test.c
@@ -317,91 +317,92 @@ static void pci_epf_test_print_rate(struct pci_epf_test *epf_test,
 static void pci_epf_test_copy(struct pci_epf_test *epf_test,
 			      struct pci_epf_test_reg *reg)
 {
-	int ret;
-	void __iomem *src_addr;
-	void __iomem *dst_addr;
-	phys_addr_t src_phys_addr;
-	phys_addr_t dst_phys_addr;
+	int ret = 0;
 	struct timespec64 start, end;
 	struct pci_epf *epf = epf_test->epf;
-	struct device *dev = &epf->dev;
 	struct pci_epc *epc = epf->epc;
+	struct device *dev = &epf->dev;
+	struct pci_epc_map src_map, dst_map;
+	u64 src_addr = reg->src_addr;
+	u64 dst_addr = reg->dst_addr;
+	size_t copy_size = reg->size;
+	ssize_t map_size = 0;
+	void *copy_buf = NULL, *buf;
 
-	src_addr = pci_epc_mem_alloc_addr(epc, &src_phys_addr, reg->size);
-	if (!src_addr) {
-		dev_err(dev, "Failed to allocate source address\n");
-		reg->status = STATUS_SRC_ADDR_INVALID;
-		ret = -ENOMEM;
-		goto err;
-	}
-
-	ret = pci_epc_map_addr(epc, epf->func_no, epf->vfunc_no, src_phys_addr,
-			       reg->src_addr, reg->size);
-	if (ret) {
-		dev_err(dev, "Failed to map source address\n");
-		reg->status = STATUS_SRC_ADDR_INVALID;
-		goto err_src_addr;
-	}
-
-	dst_addr = pci_epc_mem_alloc_addr(epc, &dst_phys_addr, reg->size);
-	if (!dst_addr) {
-		dev_err(dev, "Failed to allocate destination address\n");
-		reg->status = STATUS_DST_ADDR_INVALID;
-		ret = -ENOMEM;
-		goto err_src_map_addr;
-	}
-
-	ret = pci_epc_map_addr(epc, epf->func_no, epf->vfunc_no, dst_phys_addr,
-			       reg->dst_addr, reg->size);
-	if (ret) {
-		dev_err(dev, "Failed to map destination address\n");
-		reg->status = STATUS_DST_ADDR_INVALID;
-		goto err_dst_addr;
-	}
-
-	ktime_get_ts64(&start);
 	if (reg->flags & FLAG_USE_DMA) {
 		if (epf_test->dma_private) {
 			dev_err(dev, "Cannot transfer data using DMA\n");
 			ret = -EINVAL;
-			goto err_map_addr;
+			goto set_status;
 		}
-
-		ret = pci_epf_test_data_transfer(epf_test, dst_phys_addr,
-						 src_phys_addr, reg->size, 0,
-						 DMA_MEM_TO_MEM);
-		if (ret)
-			dev_err(dev, "Data transfer failed\n");
 	} else {
-		void *buf;
-
-		buf = kzalloc(reg->size, GFP_KERNEL);
-		if (!buf) {
+		copy_buf = kzalloc(copy_size, GFP_KERNEL);
+		if (!copy_buf) {
 			ret = -ENOMEM;
-			goto err_map_addr;
+			goto set_status;
 		}
-
-		memcpy_fromio(buf, src_addr, reg->size);
-		memcpy_toio(dst_addr, buf, reg->size);
-		kfree(buf);
+		buf = copy_buf;
 	}
-	ktime_get_ts64(&end);
-	pci_epf_test_print_rate(epf_test, "COPY", reg->size, &start, &end,
-				reg->flags & FLAG_USE_DMA);
 
-err_map_addr:
-	pci_epc_unmap_addr(epc, epf->func_no, epf->vfunc_no, dst_phys_addr);
+	while (copy_size) {
+		ret = pci_epc_mem_map(epc, epf->func_no, epf->vfunc_no,
+				      src_addr, copy_size, &src_map);
+		if (ret) {
+			dev_err(dev, "Failed to map source address\n");
+			reg->status = STATUS_SRC_ADDR_INVALID;
+			goto free_buf;
+		}
+
+		ret = pci_epc_mem_map(epf->epc, epf->func_no, epf->vfunc_no,
+					   dst_addr, copy_size, &dst_map);
+		if (ret) {
+			dev_err(dev, "Failed to map destination address\n");
+			reg->status = STATUS_DST_ADDR_INVALID;
+			pci_epc_mem_unmap(epc, epf->func_no, epf->vfunc_no,
+					  &src_map);
+			goto free_buf;
+		}
+
+		map_size = min_t(size_t, dst_map.pci_size, src_map.pci_size);
+
+		ktime_get_ts64(&start);
+		if (reg->flags & FLAG_USE_DMA) {
+			ret = pci_epf_test_data_transfer(epf_test,
+					dst_map.phys_addr, src_map.phys_addr,
+					map_size, 0, DMA_MEM_TO_MEM);
+			if (ret) {
+				dev_err(dev, "Data transfer failed\n");
+				goto unmap;
+			}
+		} else {
+			memcpy_fromio(buf, src_map.virt_addr, map_size);
+			memcpy_toio(dst_map.virt_addr, buf, map_size);
+			buf += map_size;
+		}
+		ktime_get_ts64(&end);
 
-err_dst_addr:
-	pci_epc_mem_free_addr(epc, dst_phys_addr, dst_addr, reg->size);
+		copy_size -= map_size;
+		src_addr += map_size;
+		dst_addr += map_size;
 
-err_src_map_addr:
-	pci_epc_unmap_addr(epc, epf->func_no, epf->vfunc_no, src_phys_addr);
+		pci_epc_mem_unmap(epc, epf->func_no, epf->vfunc_no, &dst_map);
+		pci_epc_mem_unmap(epc, epf->func_no, epf->vfunc_no, &src_map);
+		map_size = 0;
+	}
 
-err_src_addr:
-	pci_epc_mem_free_addr(epc, src_phys_addr, src_addr, reg->size);
+	pci_epf_test_print_rate(epf_test, "COPY", reg->size, &start,
+				&end, reg->flags & FLAG_USE_DMA);
 
-err:
+unmap:
+	if (map_size) {
+		pci_epc_mem_unmap(epc, epf->func_no, epf->vfunc_no, &dst_map);
+		pci_epc_mem_unmap(epc, epf->func_no, epf->vfunc_no, &src_map);
+	}
+
+free_buf:
+	kfree(copy_buf);
+
+set_status:
 	if (!ret)
 		reg->status |= STATUS_COPY_SUCCESS;
 	else
@@ -411,82 +412,89 @@ static void pci_epf_test_copy(struct pci_epf_test *epf_test,
 static void pci_epf_test_read(struct pci_epf_test *epf_test,
 			      struct pci_epf_test_reg *reg)
 {
-	int ret;
-	void __iomem *src_addr;
-	void *buf;
+	int ret = 0;
+	void *src_buf, *buf;
 	u32 crc32;
-	phys_addr_t phys_addr;
+	struct pci_epc_map map;
 	phys_addr_t dst_phys_addr;
 	struct timespec64 start, end;
 	struct pci_epf *epf = epf_test->epf;
-	struct device *dev = &epf->dev;
 	struct pci_epc *epc = epf->epc;
+	struct device *dev = &epf->dev;
 	struct device *dma_dev = epf->epc->dev.parent;
+	u64 src_addr = reg->src_addr;
+	size_t src_size = reg->size;
+	ssize_t map_size = 0;
 
-	src_addr = pci_epc_mem_alloc_addr(epc, &phys_addr, reg->size);
-	if (!src_addr) {
-		dev_err(dev, "Failed to allocate address\n");
-		reg->status = STATUS_SRC_ADDR_INVALID;
+	src_buf = kzalloc(src_size, GFP_KERNEL);
+	if (!src_buf) {
 		ret = -ENOMEM;
-		goto err;
+		goto set_status;
 	}
+	buf = src_buf;
 
-	ret = pci_epc_map_addr(epc, epf->func_no, epf->vfunc_no, phys_addr,
-			       reg->src_addr, reg->size);
-	if (ret) {
-		dev_err(dev, "Failed to map address\n");
-		reg->status = STATUS_SRC_ADDR_INVALID;
-		goto err_addr;
-	}
-
-	buf = kzalloc(reg->size, GFP_KERNEL);
-	if (!buf) {
-		ret = -ENOMEM;
-		goto err_map_addr;
-	}
+	while (src_size) {
+		ret = pci_epc_mem_map(epc, epf->func_no, epf->vfunc_no,
+					   src_addr, src_size, &map);
+		if (ret) {
+			dev_err(dev, "Failed to map address\n");
+			reg->status = STATUS_SRC_ADDR_INVALID;
+			goto free_buf;
+		}
 
-	if (reg->flags & FLAG_USE_DMA) {
-		dst_phys_addr = dma_map_single(dma_dev, buf, reg->size,
-					       DMA_FROM_DEVICE);
-		if (dma_mapping_error(dma_dev, dst_phys_addr)) {
-			dev_err(dev, "Failed to map destination buffer addr\n");
-			ret = -ENOMEM;
-			goto err_dma_map;
+		map_size = map.pci_size;
+		if (reg->flags & FLAG_USE_DMA) {
+			dst_phys_addr = dma_map_single(dma_dev, buf, map_size,
+						       DMA_FROM_DEVICE);
+			if (dma_mapping_error(dma_dev, dst_phys_addr)) {
+				dev_err(dev,
+					"Failed to map destination buffer addr\n");
+				ret = -ENOMEM;
+				goto unmap;
+			}
+
+			ktime_get_ts64(&start);
+			ret = pci_epf_test_data_transfer(epf_test,
+					dst_phys_addr, map.phys_addr,
+					map_size, src_addr, DMA_DEV_TO_MEM);
+			if (ret)
+				dev_err(dev, "Data transfer failed\n");
+			ktime_get_ts64(&end);
+
+			dma_unmap_single(dma_dev, dst_phys_addr, map_size,
+					 DMA_FROM_DEVICE);
+
+			if (ret)
+				goto unmap;
+		} else {
+			ktime_get_ts64(&start);
+			memcpy_fromio(buf, map.virt_addr, map_size);
+			ktime_get_ts64(&end);
 		}
 
-		ktime_get_ts64(&start);
-		ret = pci_epf_test_data_transfer(epf_test, dst_phys_addr,
-						 phys_addr, reg->size,
-						 reg->src_addr, DMA_DEV_TO_MEM);
-		if (ret)
-			dev_err(dev, "Data transfer failed\n");
-		ktime_get_ts64(&end);
+		src_size -= map_size;
+		src_addr += map_size;
+		buf += map_size;
 
-		dma_unmap_single(dma_dev, dst_phys_addr, reg->size,
-				 DMA_FROM_DEVICE);
-	} else {
-		ktime_get_ts64(&start);
-		memcpy_fromio(buf, src_addr, reg->size);
-		ktime_get_ts64(&end);
+		pci_epc_mem_unmap(epc, epf->func_no, epf->vfunc_no, &map);
+		map_size = 0;
 	}
 
-	pci_epf_test_print_rate(epf_test, "READ", reg->size, &start, &end,
-				reg->flags & FLAG_USE_DMA);
+	pci_epf_test_print_rate(epf_test, "READ", reg->size, &start,
+				&end, reg->flags & FLAG_USE_DMA);
 
-	crc32 = crc32_le(~0, buf, reg->size);
+	crc32 = crc32_le(~0, src_buf, reg->size);
 	if (crc32 != reg->checksum)
 		ret = -EIO;
 
-err_dma_map:
-	kfree(buf);
-
-err_map_addr:
-	pci_epc_unmap_addr(epc, epf->func_no, epf->vfunc_no, phys_addr);
+unmap:
+	if (map_size)
+		pci_epc_mem_unmap(epc, epf->func_no, epf->vfunc_no, &map);
 
-err_addr:
-	pci_epc_mem_free_addr(epc, phys_addr, src_addr, reg->size);
+free_buf:
+	kfree(src_buf);
 
-err:
+set_status:
 	if (!ret)
 		reg->status |= STATUS_READ_SUCCESS;
 	else
@@ -496,71 +504,79 @@ static void pci_epf_test_read(struct pci_epf_test *epf_test,
 static void pci_epf_test_write(struct pci_epf_test *epf_test,
 			       struct pci_epf_test_reg *reg)
 {
-	int ret;
-	void __iomem *dst_addr;
-	void *buf;
-	phys_addr_t phys_addr;
+	int ret = 0;
+	void *dst_buf, *buf;
+	struct pci_epc_map map;
 	phys_addr_t src_phys_addr;
 	struct timespec64 start, end;
 	struct pci_epf *epf = epf_test->epf;
-	struct device *dev = &epf->dev;
 	struct pci_epc *epc = epf->epc;
+	struct device *dev = &epf->dev;
 	struct device *dma_dev = epf->epc->dev.parent;
+	u64 dst_addr = reg->dst_addr;
+	size_t dst_size = reg->size;
+	ssize_t map_size = 0;
 
-	dst_addr = pci_epc_mem_alloc_addr(epc, &phys_addr, reg->size);
-	if (!dst_addr) {
-		dev_err(dev, "Failed to allocate address\n");
-		reg->status = STATUS_DST_ADDR_INVALID;
+	dst_buf = kzalloc(dst_size, GFP_KERNEL);
+	if (!dst_buf) {
 		ret = -ENOMEM;
-		goto err;
+		goto set_status;
 	}
+	get_random_bytes(dst_buf, dst_size);
+	reg->checksum = crc32_le(~0, dst_buf, dst_size);
+	buf = dst_buf;
 
-	ret = pci_epc_map_addr(epc, epf->func_no, epf->vfunc_no, phys_addr,
-			       reg->dst_addr, reg->size);
-	if (ret) {
-		dev_err(dev, "Failed to map address\n");
-		reg->status = STATUS_DST_ADDR_INVALID;
-		goto err_addr;
-	}
-
-	buf = kzalloc(reg->size, GFP_KERNEL);
-	if (!buf) {
-		ret = -ENOMEM;
-		goto err_map_addr;
-	}
-
-	get_random_bytes(buf, reg->size);
-	reg->checksum = crc32_le(~0, buf, reg->size);
-
-	if (reg->flags & FLAG_USE_DMA) {
-		src_phys_addr = dma_map_single(dma_dev, buf, reg->size,
-					       DMA_TO_DEVICE);
-		if (dma_mapping_error(dma_dev, src_phys_addr)) {
-			dev_err(dev, "Failed to map source buffer addr\n");
-			ret = -ENOMEM;
-			goto err_dma_map;
+	while (dst_size) {
+		ret = pci_epc_mem_map(epc, epf->func_no, epf->vfunc_no,
+					   dst_addr, dst_size, &map);
+		if (ret) {
+			dev_err(dev, "Failed to map address\n");
+			reg->status = STATUS_DST_ADDR_INVALID;
+			goto free_buf;
 		}
 
-		ktime_get_ts64(&start);
+		map_size = map.pci_size;
+		if (reg->flags & FLAG_USE_DMA) {
+			src_phys_addr = dma_map_single(dma_dev, buf, map_size,
+						       DMA_TO_DEVICE);
+			if (dma_mapping_error(dma_dev, src_phys_addr)) {
+				dev_err(dev,
+					"Failed to map source buffer addr\n");
+				ret = -ENOMEM;
+				goto unmap;
+			}
+
+			ktime_get_ts64(&start);
+
+			ret = pci_epf_test_data_transfer(epf_test,
+						map.phys_addr, src_phys_addr,
+						map_size, dst_addr,
+						DMA_MEM_TO_DEV);
+			if (ret)
+				dev_err(dev, "Data transfer failed\n");
+			ktime_get_ts64(&end);
+
+			dma_unmap_single(dma_dev, src_phys_addr, map_size,
+					 DMA_TO_DEVICE);
+
+			if (ret)
+				goto unmap;
+		} else {
+			ktime_get_ts64(&start);
+			memcpy_toio(map.virt_addr, buf, map_size);
+			ktime_get_ts64(&end);
+		}
 
-		ret = pci_epf_test_data_transfer(epf_test, phys_addr,
-						 src_phys_addr, reg->size,
-						 reg->dst_addr,
-						 DMA_MEM_TO_DEV);
-		if (ret)
-			dev_err(dev, "Data transfer failed\n");
-		ktime_get_ts64(&end);
+		dst_size -= map_size;
+		dst_addr += map_size;
+		buf += map_size;
 
-		dma_unmap_single(dma_dev, src_phys_addr, reg->size,
-				 DMA_TO_DEVICE);
-	} else {
-		ktime_get_ts64(&start);
-		memcpy_toio(dst_addr, buf, reg->size);
-		ktime_get_ts64(&end);
+		pci_epc_mem_unmap(epc, epf->func_no, epf->vfunc_no, &map);
+		map_size = 0;
 	}
 
-	pci_epf_test_print_rate(epf_test, "WRITE", reg->size, &start, &end,
-				reg->flags & FLAG_USE_DMA);
+	pci_epf_test_print_rate(epf_test, "WRITE", reg->size, &start,
+				&end, reg->flags & FLAG_USE_DMA);
 
 	/*
 	 * wait 1ms inorder for the write to complete. Without this delay L3
@@ -568,16 +584,14 @@ static void pci_epf_test_write(struct pci_epf_test *epf_test,
 	 */
 	usleep_range(1000, 2000);
 
-err_dma_map:
-	kfree(buf);
-
-err_map_addr:
-	pci_epc_unmap_addr(epc, epf->func_no, epf->vfunc_no, phys_addr);
+unmap:
+	if (map_size)
+		pci_epc_mem_unmap(epc, epf->func_no, epf->vfunc_no, &map);
 
-err_addr:
-	pci_epc_mem_free_addr(epc, phys_addr, dst_addr, reg->size);
+free_buf:
+	kfree(dst_buf);
 
-err:
+set_status:
 	if (!ret)
 		reg->status |= STATUS_WRITE_SUCCESS;
 	else
-- 
2.46.2

[PATCH v4 7/7] PCI: dwc: endpoint: Define the .map_align() controller operation

[Damien Le Moal]

The function dw_pcie_prog_outbound_atu() used to program outbound ATU
entries for mapping RC PCI addresses to local CPU addresses does not
allow PCI addresses that are not aligned to the value of region_align
of struct dw_pcie. This value is determined from the iATU hardware
registers during probing of the iATU (done by dw_pcie_iatu_detect()).
This value is thus valid for all DWC PCIe controllers, and valid
regardless of the hardware configuration used when synthesizing the
DWC PCIe controller.

Handle this PCI address alignment constraint by defining the endpoint
controller .map_align() operation to calculate a mapping size and the
offset into the mapping based on the requested RC PCI address and size
to map.

Signed-off-by: Damien Le Moal <dlemoal@kernel.org>
Reviewed-by: Niklas Cassel <cassel@kernel.org>
---
 drivers/pci/controller/dwc/pcie-designware-ep.c | 15 +++++++++++++++
 1 file changed, 15 insertions(+)

diff --git a/drivers/pci/controller/dwc/pcie-designware-ep.c b/drivers/pci/controller/dwc/pcie-designware-ep.c
index 43ba5c6738df..501e527c188e 100644
--- a/drivers/pci/controller/dwc/pcie-designware-ep.c
+++ b/drivers/pci/controller/dwc/pcie-designware-ep.c
@@ -268,6 +268,20 @@ static int dw_pcie_find_index(struct dw_pcie_ep *ep, phys_addr_t addr,
 	return -EINVAL;
 }
 
+static int dw_pcie_ep_map_align(struct pci_epc *epc, u8 func_no, u8 vfunc_no,
+				struct pci_epc_map *map)
+{
+	struct dw_pcie_ep *ep = epc_get_drvdata(epc);
+	struct dw_pcie *pci = to_dw_pcie_from_ep(ep);
+	size_t mask = pci->region_align - 1;
+
+	map->map_pci_addr = map->pci_addr & ~mask;
+	map->map_ofst = map->pci_addr & mask;
+	map->map_size = ALIGN(map->map_ofst + map->pci_size, ep->page_size);
+
+	return 0;
+}
+
 static void dw_pcie_ep_unmap_addr(struct pci_epc *epc, u8 func_no, u8 vfunc_no,
 				  phys_addr_t addr)
 {
@@ -444,6 +458,7 @@ static const struct pci_epc_ops epc_ops = {
 	.write_header		= dw_pcie_ep_write_header,
 	.set_bar		= dw_pcie_ep_set_bar,
 	.clear_bar		= dw_pcie_ep_clear_bar,
+	.map_align		= dw_pcie_ep_map_align,
 	.map_addr		= dw_pcie_ep_map_addr,
 	.unmap_addr		= dw_pcie_ep_unmap_addr,
 	.set_msi		= dw_pcie_ep_set_msi,
-- 
2.46.2

Re: [PATCH v4 0/7] Improve PCI memory mapping API

[Damien Le Moal]

On 10/7/24 13:03, Damien Le Moal wrote:
> This series introduces the new functions pci_epc_map_align(),
> pci_epc_mem_map() and pci_epc_mem_unmap() to improve handling of the
> PCI address mapping alignment constraints of endpoint controllers in a
> controller independent manner.
> 
> The issue fixed is that the fixed alignment defined by the "align" field
> of struct pci_epc_features is defined for inbound ATU entries (e.g.
> BARs) and is a fixed value, whereas some controllers need a PCI address
> alignment that depends on the PCI address itself. For instance, the
> rk3399 SoC controller in endpoint mode uses the lower bits of the local
> endpoint memory address as the lower bits for the PCI addresses for data
> transfers. That is, when mapping local memory, one must take into
> account the number of bits of the RC PCI address that change from the
> start address of the mapping.
> 
> To fix this, the new endpoint controller method .map_align is introduced
> and called from pci_epc_map_align(). This method is optional and for
> controllers that do not define it, it is assumed that the controller has
> no PCI address constraint.
> 
> The functions pci_epc_mem_map() is a helper function which obtains
> mapping information, allocates endpoint controller memory according to
> the mapping size obtained and maps the memory. pci_epc_mem_unmap()
> unmaps and frees the endpoint memory.
> 
> This series is organized as follows:
>  - Patch 1 introduces a small helper to clean up the epc core code
>  - Patch 2 improves pci_epc_mem_alloc_addr()
>  - Patch 3 and 4 introduce the new map_align endpoint controller method
>    and the epc functions pci_epc_mem_map() and pci_epc_mem_unmap().
>  - Patch 5 documents these new functions.
>  - Patch 6 modifies the test endpoint function driver to use 
>    pci_epc_mem_map() and pci_epc_mem_unmap() to illustrate the use of
>    these functions.
>  - Finally, patch 7 implements the rk3588 endpoint controller driver
>    .map_align operation to satisfy that controller PCI address
>    alignment constraint.

I forgot to add that the series was extensively tested using the rk3588 endpoint
controller with the test endpoint function driver (as-is and modified to remove
the forced host buffer alignment) as well as using the NVMe endpoint function
driver (v1 patches just posted).

> 
> Changes from v3:
>  - Addressed Niklas comments (improved patch 2 commit message, added
>    comments in the pci_epc_map_align() function in patch 3, typos and
>    improvements in patch 5, patch 7 commit message).
>  - Added review tags
> 
> Changes from v2:
>  - Dropped all patches for the rockchip-ep. These patches will be sent
>    later as a separate series.
>  - Added patch 2 and 5
>  - Added review tags to patch 1
> 
> Changes from v1:
>  - Changed pci_epc_check_func() to pci_epc_function_is_valid() in patch
>    1.
>  - Removed patch "PCI: endpoint: Improve pci_epc_mem_alloc_addr()"
>    (former patch 2 of v1)
>  - Various typos cleanups all over. Also fixed some blank space
>    indentation.
>  - Added review tags
> 
> Damien Le Moal (7):
>   PCI: endpoint: Introduce pci_epc_function_is_valid()
>   PCI: endpoint: Improve pci_epc_mem_alloc_addr()
>   PCI: endpoint: Introduce pci_epc_map_align()
>   PCI: endpoint: Introduce pci_epc_mem_map()/unmap()
>   PCI: endpoint: Update documentation
>   PCI: endpoint: test: Use pci_epc_mem_map/unmap()
>   PCI: dwc: endpoint: Define the .map_align() controller operation
> 
>  Documentation/PCI/endpoint/pci-endpoint.rst   |  35 ++
>  .../pci/controller/dwc/pcie-designware-ep.c   |  15 +
>  drivers/pci/endpoint/functions/pci-epf-test.c | 372 +++++++++---------
>  drivers/pci/endpoint/pci-epc-core.c           | 223 ++++++++---
>  drivers/pci/endpoint/pci-epc-mem.c            |   9 +-
>  include/linux/pci-epc.h                       |  41 ++
>  6 files changed, 465 insertions(+), 230 deletions(-)
> 


-- 
Damien Le Moal
Western Digital Research

Re: [PATCH v4 2/7] PCI: endpoint: Improve pci_epc_mem_alloc_addr()

[Niklas Cassel]

On Mon, Oct 07, 2024 at 01:03:14PM +0900, Damien Le Moal wrote:
> There is no point in attempting to allocate memory from an endpoint
> controller memory window if the requested size is larger than the memory
> window size. Add a check to skip bitmap_find_free_region() calls for
> such case. This check can be done without the mem->lock mutex held as
> memory window sizes are constant and never modified at runtime.
> Also change the final return to return NULL to simplify the code.
> 
> Signed-off-by: Damien Le Moal <dlemoal@kernel.org>
> ---
>  drivers/pci/endpoint/pci-epc-mem.c | 9 ++++++---
>  1 file changed, 6 insertions(+), 3 deletions(-)
> 
> diff --git a/drivers/pci/endpoint/pci-epc-mem.c b/drivers/pci/endpoint/pci-epc-mem.c
> index a9c028f58da1..218a60e945db 100644
> --- a/drivers/pci/endpoint/pci-epc-mem.c
> +++ b/drivers/pci/endpoint/pci-epc-mem.c
> @@ -178,7 +178,7 @@ 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)
>  {
> -	void __iomem *virt_addr = NULL;
> +	void __iomem *virt_addr;
>  	struct pci_epc_mem *mem;
>  	unsigned int page_shift;
>  	size_t align_size;
> @@ -188,10 +188,13 @@ void __iomem *pci_epc_mem_alloc_addr(struct pci_epc *epc,
>  
>  	for (i = 0; i < epc->num_windows; i++) {
>  		mem = epc->windows[i];
> -		mutex_lock(&mem->lock);
> +		if (size > mem->window.size)
> +			continue;
> +
>  		align_size = ALIGN(size, mem->window.page_size);
>  		order = pci_epc_mem_get_order(mem, align_size);
>  
> +		mutex_lock(&mem->lock);
>  		pageno = bitmap_find_free_region(mem->bitmap, mem->pages,
>  						 order);
>  		if (pageno >= 0) {
> @@ -211,7 +214,7 @@ void __iomem *pci_epc_mem_alloc_addr(struct pci_epc *epc,
>  		mutex_unlock(&mem->lock);
>  	}
>  
> -	return virt_addr;
> +	return NULL;
>  }
>  EXPORT_SYMBOL_GPL(pci_epc_mem_alloc_addr);
>  
> -- 
> 2.46.2
> 

Reviewed-by: Niklas Cassel <cassel@kernel.org>

Re: [PATCH v4 6/7] PCI: endpoint: test: Use pci_epc_mem_map/unmap()

[Niklas Cassel]

On Mon, Oct 07, 2024 at 01:03:18PM +0900, Damien Le Moal wrote:
> Modify the endpoint test driver to use the functions pci_epc_mem_map()
> and pci_epc_mem_unmap() for the read, write and copy tests. For each
> test case, the transfer (dma or mmio) are executed in a loop to ensure
> that potentially partial mappings returned by pci_epc_mem_map() are
> correctly handled.

I still think that this commit message could have been even clearer to
state that the pci-epf-test driver is currently not handling buffers
larger than the window size, so this loop is not only needed because of
the new API (which can return a size smaller than requested), it was
actually needed even without this patch.


Regardless, since this loop is definitely needed:
Reviewed-by: Niklas Cassel <cassel@kernel.org>


> 
> Signed-off-by: Damien Le Moal <dlemoal@kernel.org>
> ---
>  drivers/pci/endpoint/functions/pci-epf-test.c | 372 +++++++++---------
>  1 file changed, 193 insertions(+), 179 deletions(-)
> 
> diff --git a/drivers/pci/endpoint/functions/pci-epf-test.c b/drivers/pci/endpoint/functions/pci-epf-test.c
> index 7c2ed6eae53a..a73bc0771d35 100644
> --- a/drivers/pci/endpoint/functions/pci-epf-test.c
> +++ b/drivers/pci/endpoint/functions/pci-epf-test.c
> @@ -317,91 +317,92 @@ static void pci_epf_test_print_rate(struct pci_epf_test *epf_test,
>  static void pci_epf_test_copy(struct pci_epf_test *epf_test,
>  			      struct pci_epf_test_reg *reg)
>  {
> -	int ret;
> -	void __iomem *src_addr;
> -	void __iomem *dst_addr;
> -	phys_addr_t src_phys_addr;
> -	phys_addr_t dst_phys_addr;
> +	int ret = 0;
>  	struct timespec64 start, end;
>  	struct pci_epf *epf = epf_test->epf;
> -	struct device *dev = &epf->dev;
>  	struct pci_epc *epc = epf->epc;
> +	struct device *dev = &epf->dev;
> +	struct pci_epc_map src_map, dst_map;
> +	u64 src_addr = reg->src_addr;
> +	u64 dst_addr = reg->dst_addr;
> +	size_t copy_size = reg->size;
> +	ssize_t map_size = 0;
> +	void *copy_buf = NULL, *buf;
>  
> -	src_addr = pci_epc_mem_alloc_addr(epc, &src_phys_addr, reg->size);
> -	if (!src_addr) {
> -		dev_err(dev, "Failed to allocate source address\n");
> -		reg->status = STATUS_SRC_ADDR_INVALID;
> -		ret = -ENOMEM;
> -		goto err;
> -	}
> -
> -	ret = pci_epc_map_addr(epc, epf->func_no, epf->vfunc_no, src_phys_addr,
> -			       reg->src_addr, reg->size);
> -	if (ret) {
> -		dev_err(dev, "Failed to map source address\n");
> -		reg->status = STATUS_SRC_ADDR_INVALID;
> -		goto err_src_addr;
> -	}
> -
> -	dst_addr = pci_epc_mem_alloc_addr(epc, &dst_phys_addr, reg->size);
> -	if (!dst_addr) {
> -		dev_err(dev, "Failed to allocate destination address\n");
> -		reg->status = STATUS_DST_ADDR_INVALID;
> -		ret = -ENOMEM;
> -		goto err_src_map_addr;
> -	}
> -
> -	ret = pci_epc_map_addr(epc, epf->func_no, epf->vfunc_no, dst_phys_addr,
> -			       reg->dst_addr, reg->size);
> -	if (ret) {
> -		dev_err(dev, "Failed to map destination address\n");
> -		reg->status = STATUS_DST_ADDR_INVALID;
> -		goto err_dst_addr;
> -	}
> -
> -	ktime_get_ts64(&start);
>  	if (reg->flags & FLAG_USE_DMA) {
>  		if (epf_test->dma_private) {
>  			dev_err(dev, "Cannot transfer data using DMA\n");
>  			ret = -EINVAL;
> -			goto err_map_addr;
> +			goto set_status;
>  		}
> -
> -		ret = pci_epf_test_data_transfer(epf_test, dst_phys_addr,
> -						 src_phys_addr, reg->size, 0,
> -						 DMA_MEM_TO_MEM);
> -		if (ret)
> -			dev_err(dev, "Data transfer failed\n");
>  	} else {
> -		void *buf;
> -
> -		buf = kzalloc(reg->size, GFP_KERNEL);
> -		if (!buf) {
> +		copy_buf = kzalloc(copy_size, GFP_KERNEL);
> +		if (!copy_buf) {
>  			ret = -ENOMEM;
> -			goto err_map_addr;
> +			goto set_status;
>  		}
> -
> -		memcpy_fromio(buf, src_addr, reg->size);
> -		memcpy_toio(dst_addr, buf, reg->size);
> -		kfree(buf);
> +		buf = copy_buf;
>  	}
> -	ktime_get_ts64(&end);
> -	pci_epf_test_print_rate(epf_test, "COPY", reg->size, &start, &end,
> -				reg->flags & FLAG_USE_DMA);
>  
> -err_map_addr:
> -	pci_epc_unmap_addr(epc, epf->func_no, epf->vfunc_no, dst_phys_addr);
> +	while (copy_size) {
> +		ret = pci_epc_mem_map(epc, epf->func_no, epf->vfunc_no,
> +				      src_addr, copy_size, &src_map);
> +		if (ret) {
> +			dev_err(dev, "Failed to map source address\n");
> +			reg->status = STATUS_SRC_ADDR_INVALID;
> +			goto free_buf;
> +		}
> +
> +		ret = pci_epc_mem_map(epf->epc, epf->func_no, epf->vfunc_no,
> +					   dst_addr, copy_size, &dst_map);
> +		if (ret) {
> +			dev_err(dev, "Failed to map destination address\n");
> +			reg->status = STATUS_DST_ADDR_INVALID;
> +			pci_epc_mem_unmap(epc, epf->func_no, epf->vfunc_no,
> +					  &src_map);
> +			goto free_buf;
> +		}
> +
> +		map_size = min_t(size_t, dst_map.pci_size, src_map.pci_size);
> +
> +		ktime_get_ts64(&start);
> +		if (reg->flags & FLAG_USE_DMA) {
> +			ret = pci_epf_test_data_transfer(epf_test,
> +					dst_map.phys_addr, src_map.phys_addr,
> +					map_size, 0, DMA_MEM_TO_MEM);
> +			if (ret) {
> +				dev_err(dev, "Data transfer failed\n");
> +				goto unmap;
> +			}
> +		} else {
> +			memcpy_fromio(buf, src_map.virt_addr, map_size);
> +			memcpy_toio(dst_map.virt_addr, buf, map_size);
> +			buf += map_size;
> +		}
> +		ktime_get_ts64(&end);
>  
> -err_dst_addr:
> -	pci_epc_mem_free_addr(epc, dst_phys_addr, dst_addr, reg->size);
> +		copy_size -= map_size;
> +		src_addr += map_size;
> +		dst_addr += map_size;
>  
> -err_src_map_addr:
> -	pci_epc_unmap_addr(epc, epf->func_no, epf->vfunc_no, src_phys_addr);
> +		pci_epc_mem_unmap(epc, epf->func_no, epf->vfunc_no, &dst_map);
> +		pci_epc_mem_unmap(epc, epf->func_no, epf->vfunc_no, &src_map);
> +		map_size = 0;
> +	}
>  
> -err_src_addr:
> -	pci_epc_mem_free_addr(epc, src_phys_addr, src_addr, reg->size);
> +	pci_epf_test_print_rate(epf_test, "COPY", reg->size, &start,
> +				&end, reg->flags & FLAG_USE_DMA);
>  
> -err:
> +unmap:
> +	if (map_size) {
> +		pci_epc_mem_unmap(epc, epf->func_no, epf->vfunc_no, &dst_map);
> +		pci_epc_mem_unmap(epc, epf->func_no, epf->vfunc_no, &src_map);
> +	}
> +
> +free_buf:
> +	kfree(copy_buf);
> +
> +set_status:
>  	if (!ret)
>  		reg->status |= STATUS_COPY_SUCCESS;
>  	else
> @@ -411,82 +412,89 @@ static void pci_epf_test_copy(struct pci_epf_test *epf_test,
>  static void pci_epf_test_read(struct pci_epf_test *epf_test,
>  			      struct pci_epf_test_reg *reg)
>  {
> -	int ret;
> -	void __iomem *src_addr;
> -	void *buf;
> +	int ret = 0;
> +	void *src_buf, *buf;
>  	u32 crc32;
> -	phys_addr_t phys_addr;
> +	struct pci_epc_map map;
>  	phys_addr_t dst_phys_addr;
>  	struct timespec64 start, end;
>  	struct pci_epf *epf = epf_test->epf;
> -	struct device *dev = &epf->dev;
>  	struct pci_epc *epc = epf->epc;
> +	struct device *dev = &epf->dev;
>  	struct device *dma_dev = epf->epc->dev.parent;
> +	u64 src_addr = reg->src_addr;
> +	size_t src_size = reg->size;
> +	ssize_t map_size = 0;
>  
> -	src_addr = pci_epc_mem_alloc_addr(epc, &phys_addr, reg->size);
> -	if (!src_addr) {
> -		dev_err(dev, "Failed to allocate address\n");
> -		reg->status = STATUS_SRC_ADDR_INVALID;
> +	src_buf = kzalloc(src_size, GFP_KERNEL);
> +	if (!src_buf) {
>  		ret = -ENOMEM;
> -		goto err;
> +		goto set_status;
>  	}
> +	buf = src_buf;
>  
> -	ret = pci_epc_map_addr(epc, epf->func_no, epf->vfunc_no, phys_addr,
> -			       reg->src_addr, reg->size);
> -	if (ret) {
> -		dev_err(dev, "Failed to map address\n");
> -		reg->status = STATUS_SRC_ADDR_INVALID;
> -		goto err_addr;
> -	}
> -
> -	buf = kzalloc(reg->size, GFP_KERNEL);
> -	if (!buf) {
> -		ret = -ENOMEM;
> -		goto err_map_addr;
> -	}
> +	while (src_size) {
> +		ret = pci_epc_mem_map(epc, epf->func_no, epf->vfunc_no,
> +					   src_addr, src_size, &map);
> +		if (ret) {
> +			dev_err(dev, "Failed to map address\n");
> +			reg->status = STATUS_SRC_ADDR_INVALID;
> +			goto free_buf;
> +		}
>  
> -	if (reg->flags & FLAG_USE_DMA) {
> -		dst_phys_addr = dma_map_single(dma_dev, buf, reg->size,
> -					       DMA_FROM_DEVICE);
> -		if (dma_mapping_error(dma_dev, dst_phys_addr)) {
> -			dev_err(dev, "Failed to map destination buffer addr\n");
> -			ret = -ENOMEM;
> -			goto err_dma_map;
> +		map_size = map.pci_size;
> +		if (reg->flags & FLAG_USE_DMA) {
> +			dst_phys_addr = dma_map_single(dma_dev, buf, map_size,
> +						       DMA_FROM_DEVICE);
> +			if (dma_mapping_error(dma_dev, dst_phys_addr)) {
> +				dev_err(dev,
> +					"Failed to map destination buffer addr\n");
> +				ret = -ENOMEM;
> +				goto unmap;
> +			}
> +
> +			ktime_get_ts64(&start);
> +			ret = pci_epf_test_data_transfer(epf_test,
> +					dst_phys_addr, map.phys_addr,
> +					map_size, src_addr, DMA_DEV_TO_MEM);
> +			if (ret)
> +				dev_err(dev, "Data transfer failed\n");
> +			ktime_get_ts64(&end);
> +
> +			dma_unmap_single(dma_dev, dst_phys_addr, map_size,
> +					 DMA_FROM_DEVICE);
> +
> +			if (ret)
> +				goto unmap;
> +		} else {
> +			ktime_get_ts64(&start);
> +			memcpy_fromio(buf, map.virt_addr, map_size);
> +			ktime_get_ts64(&end);
>  		}
>  
> -		ktime_get_ts64(&start);
> -		ret = pci_epf_test_data_transfer(epf_test, dst_phys_addr,
> -						 phys_addr, reg->size,
> -						 reg->src_addr, DMA_DEV_TO_MEM);
> -		if (ret)
> -			dev_err(dev, "Data transfer failed\n");
> -		ktime_get_ts64(&end);
> +		src_size -= map_size;
> +		src_addr += map_size;
> +		buf += map_size;
>  
> -		dma_unmap_single(dma_dev, dst_phys_addr, reg->size,
> -				 DMA_FROM_DEVICE);
> -	} else {
> -		ktime_get_ts64(&start);
> -		memcpy_fromio(buf, src_addr, reg->size);
> -		ktime_get_ts64(&end);
> +		pci_epc_mem_unmap(epc, epf->func_no, epf->vfunc_no, &map);
> +		map_size = 0;
>  	}
>  
> -	pci_epf_test_print_rate(epf_test, "READ", reg->size, &start, &end,
> -				reg->flags & FLAG_USE_DMA);
> +	pci_epf_test_print_rate(epf_test, "READ", reg->size, &start,
> +				&end, reg->flags & FLAG_USE_DMA);
>  
> -	crc32 = crc32_le(~0, buf, reg->size);
> +	crc32 = crc32_le(~0, src_buf, reg->size);
>  	if (crc32 != reg->checksum)
>  		ret = -EIO;
>  
> -err_dma_map:
> -	kfree(buf);
> -
> -err_map_addr:
> -	pci_epc_unmap_addr(epc, epf->func_no, epf->vfunc_no, phys_addr);
> +unmap:
> +	if (map_size)
> +		pci_epc_mem_unmap(epc, epf->func_no, epf->vfunc_no, &map);
>  
> -err_addr:
> -	pci_epc_mem_free_addr(epc, phys_addr, src_addr, reg->size);
> +free_buf:
> +	kfree(src_buf);
>  
> -err:
> +set_status:
>  	if (!ret)
>  		reg->status |= STATUS_READ_SUCCESS;
>  	else
> @@ -496,71 +504,79 @@ static void pci_epf_test_read(struct pci_epf_test *epf_test,
>  static void pci_epf_test_write(struct pci_epf_test *epf_test,
>  			       struct pci_epf_test_reg *reg)
>  {
> -	int ret;
> -	void __iomem *dst_addr;
> -	void *buf;
> -	phys_addr_t phys_addr;
> +	int ret = 0;
> +	void *dst_buf, *buf;
> +	struct pci_epc_map map;
>  	phys_addr_t src_phys_addr;
>  	struct timespec64 start, end;
>  	struct pci_epf *epf = epf_test->epf;
> -	struct device *dev = &epf->dev;
>  	struct pci_epc *epc = epf->epc;
> +	struct device *dev = &epf->dev;
>  	struct device *dma_dev = epf->epc->dev.parent;
> +	u64 dst_addr = reg->dst_addr;
> +	size_t dst_size = reg->size;
> +	ssize_t map_size = 0;
>  
> -	dst_addr = pci_epc_mem_alloc_addr(epc, &phys_addr, reg->size);
> -	if (!dst_addr) {
> -		dev_err(dev, "Failed to allocate address\n");
> -		reg->status = STATUS_DST_ADDR_INVALID;
> +	dst_buf = kzalloc(dst_size, GFP_KERNEL);
> +	if (!dst_buf) {
>  		ret = -ENOMEM;
> -		goto err;
> +		goto set_status;
>  	}
> +	get_random_bytes(dst_buf, dst_size);
> +	reg->checksum = crc32_le(~0, dst_buf, dst_size);
> +	buf = dst_buf;
>  
> -	ret = pci_epc_map_addr(epc, epf->func_no, epf->vfunc_no, phys_addr,
> -			       reg->dst_addr, reg->size);
> -	if (ret) {
> -		dev_err(dev, "Failed to map address\n");
> -		reg->status = STATUS_DST_ADDR_INVALID;
> -		goto err_addr;
> -	}
> -
> -	buf = kzalloc(reg->size, GFP_KERNEL);
> -	if (!buf) {
> -		ret = -ENOMEM;
> -		goto err_map_addr;
> -	}
> -
> -	get_random_bytes(buf, reg->size);
> -	reg->checksum = crc32_le(~0, buf, reg->size);
> -
> -	if (reg->flags & FLAG_USE_DMA) {
> -		src_phys_addr = dma_map_single(dma_dev, buf, reg->size,
> -					       DMA_TO_DEVICE);
> -		if (dma_mapping_error(dma_dev, src_phys_addr)) {
> -			dev_err(dev, "Failed to map source buffer addr\n");
> -			ret = -ENOMEM;
> -			goto err_dma_map;
> +	while (dst_size) {
> +		ret = pci_epc_mem_map(epc, epf->func_no, epf->vfunc_no,
> +					   dst_addr, dst_size, &map);
> +		if (ret) {
> +			dev_err(dev, "Failed to map address\n");
> +			reg->status = STATUS_DST_ADDR_INVALID;
> +			goto free_buf;
>  		}
>  
> -		ktime_get_ts64(&start);
> +		map_size = map.pci_size;
> +		if (reg->flags & FLAG_USE_DMA) {
> +			src_phys_addr = dma_map_single(dma_dev, buf, map_size,
> +						       DMA_TO_DEVICE);
> +			if (dma_mapping_error(dma_dev, src_phys_addr)) {
> +				dev_err(dev,
> +					"Failed to map source buffer addr\n");
> +				ret = -ENOMEM;
> +				goto unmap;
> +			}
> +
> +			ktime_get_ts64(&start);
> +
> +			ret = pci_epf_test_data_transfer(epf_test,
> +						map.phys_addr, src_phys_addr,
> +						map_size, dst_addr,
> +						DMA_MEM_TO_DEV);
> +			if (ret)
> +				dev_err(dev, "Data transfer failed\n");
> +			ktime_get_ts64(&end);
> +
> +			dma_unmap_single(dma_dev, src_phys_addr, map_size,
> +					 DMA_TO_DEVICE);
> +
> +			if (ret)
> +				goto unmap;
> +		} else {
> +			ktime_get_ts64(&start);
> +			memcpy_toio(map.virt_addr, buf, map_size);
> +			ktime_get_ts64(&end);
> +		}
>  
> -		ret = pci_epf_test_data_transfer(epf_test, phys_addr,
> -						 src_phys_addr, reg->size,
> -						 reg->dst_addr,
> -						 DMA_MEM_TO_DEV);
> -		if (ret)
> -			dev_err(dev, "Data transfer failed\n");
> -		ktime_get_ts64(&end);
> +		dst_size -= map_size;
> +		dst_addr += map_size;
> +		buf += map_size;
>  
> -		dma_unmap_single(dma_dev, src_phys_addr, reg->size,
> -				 DMA_TO_DEVICE);
> -	} else {
> -		ktime_get_ts64(&start);
> -		memcpy_toio(dst_addr, buf, reg->size);
> -		ktime_get_ts64(&end);
> +		pci_epc_mem_unmap(epc, epf->func_no, epf->vfunc_no, &map);
> +		map_size = 0;
>  	}
>  
> -	pci_epf_test_print_rate(epf_test, "WRITE", reg->size, &start, &end,
> -				reg->flags & FLAG_USE_DMA);
> +	pci_epf_test_print_rate(epf_test, "WRITE", reg->size, &start,
> +				&end, reg->flags & FLAG_USE_DMA);
>  
>  	/*
>  	 * wait 1ms inorder for the write to complete. Without this delay L3
> @@ -568,16 +584,14 @@ static void pci_epf_test_write(struct pci_epf_test *epf_test,
>  	 */
>  	usleep_range(1000, 2000);
>  
> -err_dma_map:
> -	kfree(buf);
> -
> -err_map_addr:
> -	pci_epc_unmap_addr(epc, epf->func_no, epf->vfunc_no, phys_addr);
> +unmap:
> +	if (map_size)
> +		pci_epc_mem_unmap(epc, epf->func_no, epf->vfunc_no, &map);
>  
> -err_addr:
> -	pci_epc_mem_free_addr(epc, phys_addr, dst_addr, reg->size);
> +free_buf:
> +	kfree(dst_buf);
>  
> -err:
> +set_status:
>  	if (!ret)
>  		reg->status |= STATUS_WRITE_SUCCESS;
>  	else
> -- 
> 2.46.2
> 

Re: [PATCH v4 2/7] PCI: endpoint: Improve pci_epc_mem_alloc_addr()

[Manivannan Sadhasivam]

On Mon, Oct 07, 2024 at 01:03:14PM +0900, Damien Le Moal wrote:
> There is no point in attempting to allocate memory from an endpoint
> controller memory window if the requested size is larger than the memory
> window size. Add a check to skip bitmap_find_free_region() calls for
> such case. This check can be done without the mem->lock mutex held as
> memory window sizes are constant and never modified at runtime.
> Also change the final return to return NULL to simplify the code.
> 
> Signed-off-by: Damien Le Moal <dlemoal@kernel.org>

Reviewed-by: Manivannan Sadhasivam <manivannan.sadhasivam@linaro.org>

- Mani

> ---
>  drivers/pci/endpoint/pci-epc-mem.c | 9 ++++++---
>  1 file changed, 6 insertions(+), 3 deletions(-)
> 
> diff --git a/drivers/pci/endpoint/pci-epc-mem.c b/drivers/pci/endpoint/pci-epc-mem.c
> index a9c028f58da1..218a60e945db 100644
> --- a/drivers/pci/endpoint/pci-epc-mem.c
> +++ b/drivers/pci/endpoint/pci-epc-mem.c
> @@ -178,7 +178,7 @@ 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)
>  {
> -	void __iomem *virt_addr = NULL;
> +	void __iomem *virt_addr;
>  	struct pci_epc_mem *mem;
>  	unsigned int page_shift;
>  	size_t align_size;
> @@ -188,10 +188,13 @@ void __iomem *pci_epc_mem_alloc_addr(struct pci_epc *epc,
>  
>  	for (i = 0; i < epc->num_windows; i++) {
>  		mem = epc->windows[i];
> -		mutex_lock(&mem->lock);
> +		if (size > mem->window.size)
> +			continue;
> +
>  		align_size = ALIGN(size, mem->window.page_size);
>  		order = pci_epc_mem_get_order(mem, align_size);
>  
> +		mutex_lock(&mem->lock);
>  		pageno = bitmap_find_free_region(mem->bitmap, mem->pages,
>  						 order);
>  		if (pageno >= 0) {
> @@ -211,7 +214,7 @@ void __iomem *pci_epc_mem_alloc_addr(struct pci_epc *epc,
>  		mutex_unlock(&mem->lock);
>  	}
>  
> -	return virt_addr;
> +	return NULL;
>  }
>  EXPORT_SYMBOL_GPL(pci_epc_mem_alloc_addr);
>  
> -- 
> 2.46.2
> 

-- 
மணிவண்ணன் சதாசிவம்

Re: [PATCH v4 3/7] PCI: endpoint: Introduce pci_epc_map_align()

[Manivannan Sadhasivam]

On Mon, Oct 07, 2024 at 01:03:15PM +0900, Damien Le Moal wrote:
> Some endpoint controllers have requirements on the alignment of the
> controller physical memory address that must be used to map a RC PCI
> address region. For instance, the rockchip endpoint controller uses
> at most the lower 20 bits of a physical memory address region as the
> lower bits of an RC PCI address. For mapping a PCI address region of
> size bytes starting from pci_addr, the exact number of address bits
> used is the number of address bits changing in the address range
> [pci_addr..pci_addr + size - 1].
> 
> For this example, this creates the following constraints:
> 1) The offset into the controller physical memory allocated for a
>    mapping depends on the mapping size *and* the starting PCI address
>    for the mapping.
> 2) A mapping size cannot exceed the controller windows size (1MB) minus
>    the offset needed into the allocated physical memory, which can end
>    up being a smaller size than the desired mapping size.
> 
> Handling these constraints independently of the controller being used
> in an endpoint function driver is not possible with the current EPC
> API as only the ->align field in struct pci_epc_features is provided
> and used for BAR (inbound ATU mappings) mapping. A new API is needed
> for function drivers to discover mapping constraints and handle
> non-static requirements based on the RC PCI address range to access.
> 
> Introduce the function pci_epc_map_align() and the endpoint controller
> operation ->map_align to allow endpoint function drivers to obtain the
> size and the offset into a controller address region that must be
> allocated and mapped to access an RC PCI address region. The size
> of the mapping provided by pci_epc_map_align() can then be used as the
> size argument for the function pci_epc_mem_alloc_addr().
> The offset into the allocated controller memory provided can be used to
> correctly handle data transfers.
> 
> For endpoint controllers that have PCI address alignment constraints,
> pci_epc_map_align() may indicate upon return an effective PCI address
> region mapping size that is smaller (but not 0) than the requested PCI
> address region size. For such case, an endpoint function driver must
> handle data accesses over the desired PCI address range in fragments,
> by repeatedly using pci_epc_map_align() over the PCI address range.
> 
> The controller operation ->map_align is optional: controllers that do
> not have any alignment constraints for mapping a RC PCI address region
> do not need to implement this operation. For such controllers,
> pci_epc_map_align() always returns the mapping size as equal to the
> requested size of the PCI region and an offset equal to 0.
> 
> The new structure struct pci_epc_map is introduced to represent a
> mapping start PCI address, mapping effective size, the size and offset
> into the controller memory needed for mapping the PCI address region as
> well as the physical and virtual CPU addresses of the mapping (phys_base
> and virt_base fields). For convenience, the physical and virtual CPU
> addresses within that mapping to access the target RC PCI address region
> are also provided (phys_addr and virt_addr fields).
> 

I'm fine with the concept of this patch, but I don't get why you need an API for
this and not just a callback to be used in the pci_epc_mem_{map/unmap} APIs.
Furthermore, I don't see an user of this API (in 3 series you've sent out so
far). Let me know if I failed to spot it.

Also, the API name pci_epc_map_align() sounds like it does the mapping, but it
doesn't. So I'd not have it exposed as an API at all.

- Mani

> Co-developed-by: Rick Wertenbroek <rick.wertenbroek@gmail.com>
> Signed-off-by: Rick Wertenbroek <rick.wertenbroek@gmail.com>
> Signed-off-by: Damien Le Moal <dlemoal@kernel.org>
> Reviewed-by: Niklas Cassel <cassel@kernel.org>
> ---
>  drivers/pci/endpoint/pci-epc-core.c | 66 +++++++++++++++++++++++++++++
>  include/linux/pci-epc.h             | 37 ++++++++++++++++
>  2 files changed, 103 insertions(+)
> 
> diff --git a/drivers/pci/endpoint/pci-epc-core.c b/drivers/pci/endpoint/pci-epc-core.c
> index b854f1bab26f..1adccf07c33e 100644
> --- a/drivers/pci/endpoint/pci-epc-core.c
> +++ b/drivers/pci/endpoint/pci-epc-core.c
> @@ -435,6 +435,72 @@ void pci_epc_unmap_addr(struct pci_epc *epc, u8 func_no, u8 vfunc_no,
>  }
>  EXPORT_SYMBOL_GPL(pci_epc_unmap_addr);
>  
> +/**
> + * pci_epc_map_align() - Get the offset into and the size of a controller memory
> + *			 address region needed to map a RC PCI address region
> + * @epc: the EPC device on which address is allocated
> + * @func_no: the physical endpoint function number in the EPC device
> + * @vfunc_no: the virtual endpoint function number in the physical function
> + * @pci_addr: The RC PCI address to map
> + * @pci_size: the number of bytes to map starting from @pci_addr
> + * @map: populate here the actual size and offset into the controller memory
> + *       that must be allocated for the mapping
> + *
> + * If defined, invoke the controller map_align operation to obtain the size and
> + * the offset into a controller address region that must be allocated to map
> + * @pci_size bytes of the RC PCI address space starting from @pci_addr.
> + *
> + * On return, @map->pci_size indicates the effective size of the mapping that
> + * can be handled by the controller. This size may be smaller than the requested
> + * @pci_size. In such case, the endpoint function driver must handle the mapping
> + * using several fragments. The offset into the controller memory for the
> + * effective mapping of the RC PCI address range
> + * @pci_addr..@pci_addr+@map->pci_size is indicated by @map->map_ofst.
> + *
> + * If the target controller does not define a map_align operation, it is assumed
> + * that the controller has no PCI address mapping alignment constraint.
> + */
> +int pci_epc_map_align(struct pci_epc *epc, u8 func_no, u8 vfunc_no,
> +		      u64 pci_addr, size_t pci_size, struct pci_epc_map *map)
> +{
> +	int ret;
> +
> +	if (!pci_epc_function_is_valid(epc, func_no, vfunc_no))
> +		return -EINVAL;
> +
> +	if (!pci_size || !map)
> +		return -EINVAL;
> +
> +	/*
> +	 * Initialize and remember the PCI address region to be mapped. The
> +	 * controller ->map_align() operation may change the map->pci_size to a
> +	 * smaller value.
> +	 */
> +	memset(map, 0, sizeof(*map));
> +	map->pci_addr = pci_addr;
> +	map->pci_size = pci_size;
> +
> +	if (!epc->ops->map_align) {
> +		/*
> +		 * Assume that the EP controller has no alignment constraint,
> +		 * that is, that the PCI address to map and the size of the
> +		 * controller memory needed for the mapping are the same as
> +		 * specified by the caller.
> +		 */
> +		map->map_pci_addr = pci_addr;
> +		map->map_size = pci_size;
> +		map->map_ofst = 0;
> +		return 0;
> +	}
> +
> +	mutex_lock(&epc->lock);
> +	ret = epc->ops->map_align(epc, func_no, vfunc_no, map);
> +	mutex_unlock(&epc->lock);
> +
> +	return ret;
> +}
> +EXPORT_SYMBOL_GPL(pci_epc_map_align);
> +
>  /**
>   * pci_epc_map_addr() - map CPU address to PCI address
>   * @epc: the EPC device on which address is allocated
> diff --git a/include/linux/pci-epc.h b/include/linux/pci-epc.h
> index 42ef06136bd1..9df8a83e8d10 100644
> --- a/include/linux/pci-epc.h
> +++ b/include/linux/pci-epc.h
> @@ -32,11 +32,44 @@ pci_epc_interface_string(enum pci_epc_interface_type type)
>  	}
>  }
>  
> +/**
> + * struct pci_epc_map - information about EPC memory for mapping a RC PCI
> + *                      address range
> + * @pci_addr: start address of the RC PCI address range to map
> + * @pci_size: size of the RC PCI address range mapped from @pci_addr
> + * @map_pci_addr: RC PCI address used as the first address mapped (may be lower
> + *                than @pci_addr)
> + * @map_size: size of the controller memory needed for mapping the RC PCI address
> + *            range @pci_addr..@pci_addr+@pci_size
> + * @map_ofst: offset into the mapped controller memory to access @pci_addr
> + * @phys_base: base physical address of the allocated EPC memory for mapping the
> + *             RC PCI address range
> + * @phys_addr: physical address at which @pci_addr is mapped
> + * @virt_base: base virtual address of the allocated EPC memory for mapping the
> + *             RC PCI address range
> + * @virt_addr: virtual address at which @pci_addr is mapped
> + */
> +struct pci_epc_map {
> +	phys_addr_t	pci_addr;
> +	size_t		pci_size;
> +
> +	phys_addr_t	map_pci_addr;
> +	size_t		map_size;
> +	phys_addr_t	map_ofst;
> +
> +	phys_addr_t	phys_base;
> +	phys_addr_t	phys_addr;
> +	void __iomem	*virt_base;
> +	void __iomem	*virt_addr;
> +};
> +
>  /**
>   * struct pci_epc_ops - set of function pointers for performing EPC operations
>   * @write_header: ops to populate configuration space header
>   * @set_bar: ops to configure the BAR
>   * @clear_bar: ops to reset the BAR
> + * @map_align: operation to get the size and offset into a controller memory
> + *             window needed to map an RC PCI address region
>   * @map_addr: ops to map CPU address to PCI address
>   * @unmap_addr: ops to unmap CPU address and PCI address
>   * @set_msi: ops to set the requested number of MSI interrupts in the MSI
> @@ -61,6 +94,8 @@ struct pci_epc_ops {
>  			   struct pci_epf_bar *epf_bar);
>  	void	(*clear_bar)(struct pci_epc *epc, u8 func_no, u8 vfunc_no,
>  			     struct pci_epf_bar *epf_bar);
> +	int	(*map_align)(struct pci_epc *epc, u8 func_no, u8 vfunc_no,
> +			    struct pci_epc_map *map);
>  	int	(*map_addr)(struct pci_epc *epc, u8 func_no, u8 vfunc_no,
>  			    phys_addr_t addr, u64 pci_addr, size_t size);
>  	void	(*unmap_addr)(struct pci_epc *epc, u8 func_no, u8 vfunc_no,
> @@ -241,6 +276,8 @@ int pci_epc_set_bar(struct pci_epc *epc, u8 func_no, u8 vfunc_no,
>  		    struct pci_epf_bar *epf_bar);
>  void pci_epc_clear_bar(struct pci_epc *epc, u8 func_no, u8 vfunc_no,
>  		       struct pci_epf_bar *epf_bar);
> +int pci_epc_map_align(struct pci_epc *epc, u8 func_no, u8 vfunc_no,
> +		      u64 pci_addr, size_t size, struct pci_epc_map *map);
>  int pci_epc_map_addr(struct pci_epc *epc, u8 func_no, u8 vfunc_no,
>  		     phys_addr_t phys_addr,
>  		     u64 pci_addr, size_t size);
> -- 
> 2.46.2
> 

-- 
மணிவண்ணன் சதாசிவம்

Re: [PATCH v4 4/7] PCI: endpoint: Introduce pci_epc_mem_map()/unmap()

[Manivannan Sadhasivam]

On Mon, Oct 07, 2024 at 01:03:16PM +0900, Damien Le Moal wrote:
> Introduce the function pci_epc_mem_map() to facilitate controller memory
> address allocation and mapping to a RC PCI address region in endpoint
> function drivers.
> 
> This function first uses pci_epc_map_align() to determine the controller
> memory address size (and offset into) depending on the controller
> address alignment constraints. The result of this function is used to
> allocate a controller physical memory region using
> pci_epc_mem_alloc_addr() and map that memory to the RC PCI address
> space with pci_epc_map_addr().
> 
> Since pci_epc_map_align() may indicate that the effective mapping
> of a PCI address region is smaller than the user requested size,
> pci_epc_mem_map() may only partially map the RC PCI address region
> specified. It is the responsibility of the caller (an endpoint function
> driver) to handle such smaller mapping.
> 
> The counterpart of pci_epc_mem_map() to unmap and free the controller
> memory address region is pci_epc_mem_unmap().
> 
> Both functions operate using a struct pci_epc_map data structure
> Endpoint function drivers can use struct pci_epc_map to access the
> mapped RC PCI address region using the ->virt_addr and ->pci_size
> fields.
> 
> Co-developed-by: Rick Wertenbroek <rick.wertenbroek@gmail.com>
> Signed-off-by: Rick Wertenbroek <rick.wertenbroek@gmail.com>
> Signed-off-by: Damien Le Moal <dlemoal@kernel.org>

Looks good to me. Just one comment below.

> Reviewed-by: Niklas Cassel <cassel@kernel.org>
> ---
>  drivers/pci/endpoint/pci-epc-core.c | 78 +++++++++++++++++++++++++++++
>  include/linux/pci-epc.h             |  4 ++
>  2 files changed, 82 insertions(+)
> 
> diff --git a/drivers/pci/endpoint/pci-epc-core.c b/drivers/pci/endpoint/pci-epc-core.c
> index 1adccf07c33e..d03c753d0a53 100644
> --- a/drivers/pci/endpoint/pci-epc-core.c
> +++ b/drivers/pci/endpoint/pci-epc-core.c
> @@ -532,6 +532,84 @@ int pci_epc_map_addr(struct pci_epc *epc, u8 func_no, u8 vfunc_no,
>  }
>  EXPORT_SYMBOL_GPL(pci_epc_map_addr);
>  
> +/**
> + * pci_epc_mem_map() - allocate and map a PCI address to a CPU address
> + * @epc: the EPC device on which the CPU address is to be allocated and mapped
> + * @func_no: the physical endpoint function number in the EPC device
> + * @vfunc_no: the virtual endpoint function number in the physical function
> + * @pci_addr: PCI address to which the CPU address should be mapped
> + * @pci_size: the number of bytes to map starting from @pci_addr
> + * @map: where to return the mapping information
> + *
> + * Allocate a controller memory address region and map it to a RC PCI address
> + * region, taking into account the controller physical address mapping
> + * constraints using pci_epc_map_align().
> + * The effective size of the PCI address range mapped from @pci_addr is
> + * indicated by @map->pci_size. This size may be less than the requested
> + * @pci_size. The local virtual CPU address for the mapping is indicated by
> + * @map->virt_addr (@map->phys_addr indicates the physical address).
> + * The size and CPU address of the controller memory allocated and mapped are
> + * respectively indicated by @map->map_size and @map->virt_base (and
> + * @map->phys_base).
> + *
> + * Returns 0 on success and a negative error code in case of error.
> + */
> +int pci_epc_mem_map(struct pci_epc *epc, u8 func_no, u8 vfunc_no,
> +		    u64 pci_addr, size_t pci_size, struct pci_epc_map *map)
> +{
> +	int ret;
> +
> +	ret = pci_epc_map_align(epc, func_no, vfunc_no, pci_addr, pci_size, map);

I don't like the fact that one structure is passed to two functions and both
modify some members. If you get rid of the pci_epc_map_align() API and just use
the callback, then the arguments could be passed on their own without the 'map'
struct.

- Mani

-- 
மணிவண்ணன் சதாசிவம்

Re: [PATCH v4 6/7] PCI: endpoint: test: Use pci_epc_mem_map/unmap()

[Manivannan Sadhasivam]

On Mon, Oct 07, 2024 at 01:03:18PM +0900, Damien Le Moal wrote:
> Modify the endpoint test driver to use the functions pci_epc_mem_map()
> and pci_epc_mem_unmap() for the read, write and copy tests. For each
> test case, the transfer (dma or mmio) are executed in a loop to ensure
> that potentially partial mappings returned by pci_epc_mem_map() are
> correctly handled.
> 
> Signed-off-by: Damien Le Moal <dlemoal@kernel.org>

Reviewed-by: Manivannan Sadhasivam <manivannan.sadhasivam@linaro.org>

- Mani

> ---
>  drivers/pci/endpoint/functions/pci-epf-test.c | 372 +++++++++---------
>  1 file changed, 193 insertions(+), 179 deletions(-)
> 
> diff --git a/drivers/pci/endpoint/functions/pci-epf-test.c b/drivers/pci/endpoint/functions/pci-epf-test.c
> index 7c2ed6eae53a..a73bc0771d35 100644
> --- a/drivers/pci/endpoint/functions/pci-epf-test.c
> +++ b/drivers/pci/endpoint/functions/pci-epf-test.c
> @@ -317,91 +317,92 @@ static void pci_epf_test_print_rate(struct pci_epf_test *epf_test,
>  static void pci_epf_test_copy(struct pci_epf_test *epf_test,
>  			      struct pci_epf_test_reg *reg)
>  {
> -	int ret;
> -	void __iomem *src_addr;
> -	void __iomem *dst_addr;
> -	phys_addr_t src_phys_addr;
> -	phys_addr_t dst_phys_addr;
> +	int ret = 0;
>  	struct timespec64 start, end;
>  	struct pci_epf *epf = epf_test->epf;
> -	struct device *dev = &epf->dev;
>  	struct pci_epc *epc = epf->epc;
> +	struct device *dev = &epf->dev;
> +	struct pci_epc_map src_map, dst_map;
> +	u64 src_addr = reg->src_addr;
> +	u64 dst_addr = reg->dst_addr;
> +	size_t copy_size = reg->size;
> +	ssize_t map_size = 0;
> +	void *copy_buf = NULL, *buf;
>  
> -	src_addr = pci_epc_mem_alloc_addr(epc, &src_phys_addr, reg->size);
> -	if (!src_addr) {
> -		dev_err(dev, "Failed to allocate source address\n");
> -		reg->status = STATUS_SRC_ADDR_INVALID;
> -		ret = -ENOMEM;
> -		goto err;
> -	}
> -
> -	ret = pci_epc_map_addr(epc, epf->func_no, epf->vfunc_no, src_phys_addr,
> -			       reg->src_addr, reg->size);
> -	if (ret) {
> -		dev_err(dev, "Failed to map source address\n");
> -		reg->status = STATUS_SRC_ADDR_INVALID;
> -		goto err_src_addr;
> -	}
> -
> -	dst_addr = pci_epc_mem_alloc_addr(epc, &dst_phys_addr, reg->size);
> -	if (!dst_addr) {
> -		dev_err(dev, "Failed to allocate destination address\n");
> -		reg->status = STATUS_DST_ADDR_INVALID;
> -		ret = -ENOMEM;
> -		goto err_src_map_addr;
> -	}
> -
> -	ret = pci_epc_map_addr(epc, epf->func_no, epf->vfunc_no, dst_phys_addr,
> -			       reg->dst_addr, reg->size);
> -	if (ret) {
> -		dev_err(dev, "Failed to map destination address\n");
> -		reg->status = STATUS_DST_ADDR_INVALID;
> -		goto err_dst_addr;
> -	}
> -
> -	ktime_get_ts64(&start);
>  	if (reg->flags & FLAG_USE_DMA) {
>  		if (epf_test->dma_private) {
>  			dev_err(dev, "Cannot transfer data using DMA\n");
>  			ret = -EINVAL;
> -			goto err_map_addr;
> +			goto set_status;
>  		}
> -
> -		ret = pci_epf_test_data_transfer(epf_test, dst_phys_addr,
> -						 src_phys_addr, reg->size, 0,
> -						 DMA_MEM_TO_MEM);
> -		if (ret)
> -			dev_err(dev, "Data transfer failed\n");
>  	} else {
> -		void *buf;
> -
> -		buf = kzalloc(reg->size, GFP_KERNEL);
> -		if (!buf) {
> +		copy_buf = kzalloc(copy_size, GFP_KERNEL);
> +		if (!copy_buf) {
>  			ret = -ENOMEM;
> -			goto err_map_addr;
> +			goto set_status;
>  		}
> -
> -		memcpy_fromio(buf, src_addr, reg->size);
> -		memcpy_toio(dst_addr, buf, reg->size);
> -		kfree(buf);
> +		buf = copy_buf;
>  	}
> -	ktime_get_ts64(&end);
> -	pci_epf_test_print_rate(epf_test, "COPY", reg->size, &start, &end,
> -				reg->flags & FLAG_USE_DMA);
>  
> -err_map_addr:
> -	pci_epc_unmap_addr(epc, epf->func_no, epf->vfunc_no, dst_phys_addr);
> +	while (copy_size) {
> +		ret = pci_epc_mem_map(epc, epf->func_no, epf->vfunc_no,
> +				      src_addr, copy_size, &src_map);
> +		if (ret) {
> +			dev_err(dev, "Failed to map source address\n");
> +			reg->status = STATUS_SRC_ADDR_INVALID;
> +			goto free_buf;
> +		}
> +
> +		ret = pci_epc_mem_map(epf->epc, epf->func_no, epf->vfunc_no,
> +					   dst_addr, copy_size, &dst_map);
> +		if (ret) {
> +			dev_err(dev, "Failed to map destination address\n");
> +			reg->status = STATUS_DST_ADDR_INVALID;
> +			pci_epc_mem_unmap(epc, epf->func_no, epf->vfunc_no,
> +					  &src_map);
> +			goto free_buf;
> +		}
> +
> +		map_size = min_t(size_t, dst_map.pci_size, src_map.pci_size);
> +
> +		ktime_get_ts64(&start);
> +		if (reg->flags & FLAG_USE_DMA) {
> +			ret = pci_epf_test_data_transfer(epf_test,
> +					dst_map.phys_addr, src_map.phys_addr,
> +					map_size, 0, DMA_MEM_TO_MEM);
> +			if (ret) {
> +				dev_err(dev, "Data transfer failed\n");
> +				goto unmap;
> +			}
> +		} else {
> +			memcpy_fromio(buf, src_map.virt_addr, map_size);
> +			memcpy_toio(dst_map.virt_addr, buf, map_size);
> +			buf += map_size;
> +		}
> +		ktime_get_ts64(&end);
>  
> -err_dst_addr:
> -	pci_epc_mem_free_addr(epc, dst_phys_addr, dst_addr, reg->size);
> +		copy_size -= map_size;
> +		src_addr += map_size;
> +		dst_addr += map_size;
>  
> -err_src_map_addr:
> -	pci_epc_unmap_addr(epc, epf->func_no, epf->vfunc_no, src_phys_addr);
> +		pci_epc_mem_unmap(epc, epf->func_no, epf->vfunc_no, &dst_map);
> +		pci_epc_mem_unmap(epc, epf->func_no, epf->vfunc_no, &src_map);
> +		map_size = 0;
> +	}
>  
> -err_src_addr:
> -	pci_epc_mem_free_addr(epc, src_phys_addr, src_addr, reg->size);
> +	pci_epf_test_print_rate(epf_test, "COPY", reg->size, &start,
> +				&end, reg->flags & FLAG_USE_DMA);
>  
> -err:
> +unmap:
> +	if (map_size) {
> +		pci_epc_mem_unmap(epc, epf->func_no, epf->vfunc_no, &dst_map);
> +		pci_epc_mem_unmap(epc, epf->func_no, epf->vfunc_no, &src_map);
> +	}
> +
> +free_buf:
> +	kfree(copy_buf);
> +
> +set_status:
>  	if (!ret)
>  		reg->status |= STATUS_COPY_SUCCESS;
>  	else
> @@ -411,82 +412,89 @@ static void pci_epf_test_copy(struct pci_epf_test *epf_test,
>  static void pci_epf_test_read(struct pci_epf_test *epf_test,
>  			      struct pci_epf_test_reg *reg)
>  {
> -	int ret;
> -	void __iomem *src_addr;
> -	void *buf;
> +	int ret = 0;
> +	void *src_buf, *buf;
>  	u32 crc32;
> -	phys_addr_t phys_addr;
> +	struct pci_epc_map map;
>  	phys_addr_t dst_phys_addr;
>  	struct timespec64 start, end;
>  	struct pci_epf *epf = epf_test->epf;
> -	struct device *dev = &epf->dev;
>  	struct pci_epc *epc = epf->epc;
> +	struct device *dev = &epf->dev;
>  	struct device *dma_dev = epf->epc->dev.parent;
> +	u64 src_addr = reg->src_addr;
> +	size_t src_size = reg->size;
> +	ssize_t map_size = 0;
>  
> -	src_addr = pci_epc_mem_alloc_addr(epc, &phys_addr, reg->size);
> -	if (!src_addr) {
> -		dev_err(dev, "Failed to allocate address\n");
> -		reg->status = STATUS_SRC_ADDR_INVALID;
> +	src_buf = kzalloc(src_size, GFP_KERNEL);
> +	if (!src_buf) {
>  		ret = -ENOMEM;
> -		goto err;
> +		goto set_status;
>  	}
> +	buf = src_buf;
>  
> -	ret = pci_epc_map_addr(epc, epf->func_no, epf->vfunc_no, phys_addr,
> -			       reg->src_addr, reg->size);
> -	if (ret) {
> -		dev_err(dev, "Failed to map address\n");
> -		reg->status = STATUS_SRC_ADDR_INVALID;
> -		goto err_addr;
> -	}
> -
> -	buf = kzalloc(reg->size, GFP_KERNEL);
> -	if (!buf) {
> -		ret = -ENOMEM;
> -		goto err_map_addr;
> -	}
> +	while (src_size) {
> +		ret = pci_epc_mem_map(epc, epf->func_no, epf->vfunc_no,
> +					   src_addr, src_size, &map);
> +		if (ret) {
> +			dev_err(dev, "Failed to map address\n");
> +			reg->status = STATUS_SRC_ADDR_INVALID;
> +			goto free_buf;
> +		}
>  
> -	if (reg->flags & FLAG_USE_DMA) {
> -		dst_phys_addr = dma_map_single(dma_dev, buf, reg->size,
> -					       DMA_FROM_DEVICE);
> -		if (dma_mapping_error(dma_dev, dst_phys_addr)) {
> -			dev_err(dev, "Failed to map destination buffer addr\n");
> -			ret = -ENOMEM;
> -			goto err_dma_map;
> +		map_size = map.pci_size;
> +		if (reg->flags & FLAG_USE_DMA) {
> +			dst_phys_addr = dma_map_single(dma_dev, buf, map_size,
> +						       DMA_FROM_DEVICE);
> +			if (dma_mapping_error(dma_dev, dst_phys_addr)) {
> +				dev_err(dev,
> +					"Failed to map destination buffer addr\n");
> +				ret = -ENOMEM;
> +				goto unmap;
> +			}
> +
> +			ktime_get_ts64(&start);
> +			ret = pci_epf_test_data_transfer(epf_test,
> +					dst_phys_addr, map.phys_addr,
> +					map_size, src_addr, DMA_DEV_TO_MEM);
> +			if (ret)
> +				dev_err(dev, "Data transfer failed\n");
> +			ktime_get_ts64(&end);
> +
> +			dma_unmap_single(dma_dev, dst_phys_addr, map_size,
> +					 DMA_FROM_DEVICE);
> +
> +			if (ret)
> +				goto unmap;
> +		} else {
> +			ktime_get_ts64(&start);
> +			memcpy_fromio(buf, map.virt_addr, map_size);
> +			ktime_get_ts64(&end);
>  		}
>  
> -		ktime_get_ts64(&start);
> -		ret = pci_epf_test_data_transfer(epf_test, dst_phys_addr,
> -						 phys_addr, reg->size,
> -						 reg->src_addr, DMA_DEV_TO_MEM);
> -		if (ret)
> -			dev_err(dev, "Data transfer failed\n");
> -		ktime_get_ts64(&end);
> +		src_size -= map_size;
> +		src_addr += map_size;
> +		buf += map_size;
>  
> -		dma_unmap_single(dma_dev, dst_phys_addr, reg->size,
> -				 DMA_FROM_DEVICE);
> -	} else {
> -		ktime_get_ts64(&start);
> -		memcpy_fromio(buf, src_addr, reg->size);
> -		ktime_get_ts64(&end);
> +		pci_epc_mem_unmap(epc, epf->func_no, epf->vfunc_no, &map);
> +		map_size = 0;
>  	}
>  
> -	pci_epf_test_print_rate(epf_test, "READ", reg->size, &start, &end,
> -				reg->flags & FLAG_USE_DMA);
> +	pci_epf_test_print_rate(epf_test, "READ", reg->size, &start,
> +				&end, reg->flags & FLAG_USE_DMA);
>  
> -	crc32 = crc32_le(~0, buf, reg->size);
> +	crc32 = crc32_le(~0, src_buf, reg->size);
>  	if (crc32 != reg->checksum)
>  		ret = -EIO;
>  
> -err_dma_map:
> -	kfree(buf);
> -
> -err_map_addr:
> -	pci_epc_unmap_addr(epc, epf->func_no, epf->vfunc_no, phys_addr);
> +unmap:
> +	if (map_size)
> +		pci_epc_mem_unmap(epc, epf->func_no, epf->vfunc_no, &map);
>  
> -err_addr:
> -	pci_epc_mem_free_addr(epc, phys_addr, src_addr, reg->size);
> +free_buf:
> +	kfree(src_buf);
>  
> -err:
> +set_status:
>  	if (!ret)
>  		reg->status |= STATUS_READ_SUCCESS;
>  	else
> @@ -496,71 +504,79 @@ static void pci_epf_test_read(struct pci_epf_test *epf_test,
>  static void pci_epf_test_write(struct pci_epf_test *epf_test,
>  			       struct pci_epf_test_reg *reg)
>  {
> -	int ret;
> -	void __iomem *dst_addr;
> -	void *buf;
> -	phys_addr_t phys_addr;
> +	int ret = 0;
> +	void *dst_buf, *buf;
> +	struct pci_epc_map map;
>  	phys_addr_t src_phys_addr;
>  	struct timespec64 start, end;
>  	struct pci_epf *epf = epf_test->epf;
> -	struct device *dev = &epf->dev;
>  	struct pci_epc *epc = epf->epc;
> +	struct device *dev = &epf->dev;
>  	struct device *dma_dev = epf->epc->dev.parent;
> +	u64 dst_addr = reg->dst_addr;
> +	size_t dst_size = reg->size;
> +	ssize_t map_size = 0;
>  
> -	dst_addr = pci_epc_mem_alloc_addr(epc, &phys_addr, reg->size);
> -	if (!dst_addr) {
> -		dev_err(dev, "Failed to allocate address\n");
> -		reg->status = STATUS_DST_ADDR_INVALID;
> +	dst_buf = kzalloc(dst_size, GFP_KERNEL);
> +	if (!dst_buf) {
>  		ret = -ENOMEM;
> -		goto err;
> +		goto set_status;
>  	}
> +	get_random_bytes(dst_buf, dst_size);
> +	reg->checksum = crc32_le(~0, dst_buf, dst_size);
> +	buf = dst_buf;
>  
> -	ret = pci_epc_map_addr(epc, epf->func_no, epf->vfunc_no, phys_addr,
> -			       reg->dst_addr, reg->size);
> -	if (ret) {
> -		dev_err(dev, "Failed to map address\n");
> -		reg->status = STATUS_DST_ADDR_INVALID;
> -		goto err_addr;
> -	}
> -
> -	buf = kzalloc(reg->size, GFP_KERNEL);
> -	if (!buf) {
> -		ret = -ENOMEM;
> -		goto err_map_addr;
> -	}
> -
> -	get_random_bytes(buf, reg->size);
> -	reg->checksum = crc32_le(~0, buf, reg->size);
> -
> -	if (reg->flags & FLAG_USE_DMA) {
> -		src_phys_addr = dma_map_single(dma_dev, buf, reg->size,
> -					       DMA_TO_DEVICE);
> -		if (dma_mapping_error(dma_dev, src_phys_addr)) {
> -			dev_err(dev, "Failed to map source buffer addr\n");
> -			ret = -ENOMEM;
> -			goto err_dma_map;
> +	while (dst_size) {
> +		ret = pci_epc_mem_map(epc, epf->func_no, epf->vfunc_no,
> +					   dst_addr, dst_size, &map);
> +		if (ret) {
> +			dev_err(dev, "Failed to map address\n");
> +			reg->status = STATUS_DST_ADDR_INVALID;
> +			goto free_buf;
>  		}
>  
> -		ktime_get_ts64(&start);
> +		map_size = map.pci_size;
> +		if (reg->flags & FLAG_USE_DMA) {
> +			src_phys_addr = dma_map_single(dma_dev, buf, map_size,
> +						       DMA_TO_DEVICE);
> +			if (dma_mapping_error(dma_dev, src_phys_addr)) {
> +				dev_err(dev,
> +					"Failed to map source buffer addr\n");
> +				ret = -ENOMEM;
> +				goto unmap;
> +			}
> +
> +			ktime_get_ts64(&start);
> +
> +			ret = pci_epf_test_data_transfer(epf_test,
> +						map.phys_addr, src_phys_addr,
> +						map_size, dst_addr,
> +						DMA_MEM_TO_DEV);
> +			if (ret)
> +				dev_err(dev, "Data transfer failed\n");
> +			ktime_get_ts64(&end);
> +
> +			dma_unmap_single(dma_dev, src_phys_addr, map_size,
> +					 DMA_TO_DEVICE);
> +
> +			if (ret)
> +				goto unmap;
> +		} else {
> +			ktime_get_ts64(&start);
> +			memcpy_toio(map.virt_addr, buf, map_size);
> +			ktime_get_ts64(&end);
> +		}
>  
> -		ret = pci_epf_test_data_transfer(epf_test, phys_addr,
> -						 src_phys_addr, reg->size,
> -						 reg->dst_addr,
> -						 DMA_MEM_TO_DEV);
> -		if (ret)
> -			dev_err(dev, "Data transfer failed\n");
> -		ktime_get_ts64(&end);
> +		dst_size -= map_size;
> +		dst_addr += map_size;
> +		buf += map_size;
>  
> -		dma_unmap_single(dma_dev, src_phys_addr, reg->size,
> -				 DMA_TO_DEVICE);
> -	} else {
> -		ktime_get_ts64(&start);
> -		memcpy_toio(dst_addr, buf, reg->size);
> -		ktime_get_ts64(&end);
> +		pci_epc_mem_unmap(epc, epf->func_no, epf->vfunc_no, &map);
> +		map_size = 0;
>  	}
>  
> -	pci_epf_test_print_rate(epf_test, "WRITE", reg->size, &start, &end,
> -				reg->flags & FLAG_USE_DMA);
> +	pci_epf_test_print_rate(epf_test, "WRITE", reg->size, &start,
> +				&end, reg->flags & FLAG_USE_DMA);
>  
>  	/*
>  	 * wait 1ms inorder for the write to complete. Without this delay L3
> @@ -568,16 +584,14 @@ static void pci_epf_test_write(struct pci_epf_test *epf_test,
>  	 */
>  	usleep_range(1000, 2000);
>  
> -err_dma_map:
> -	kfree(buf);
> -
> -err_map_addr:
> -	pci_epc_unmap_addr(epc, epf->func_no, epf->vfunc_no, phys_addr);
> +unmap:
> +	if (map_size)
> +		pci_epc_mem_unmap(epc, epf->func_no, epf->vfunc_no, &map);
>  
> -err_addr:
> -	pci_epc_mem_free_addr(epc, phys_addr, dst_addr, reg->size);
> +free_buf:
> +	kfree(dst_buf);
>  
> -err:
> +set_status:
>  	if (!ret)
>  		reg->status |= STATUS_WRITE_SUCCESS;
>  	else
> -- 
> 2.46.2
> 

-- 
மணிவண்ணன் சதாசிவம்

Re: [PATCH v4 3/7] PCI: endpoint: Introduce pci_epc_map_align()

[Damien Le Moal]

On 10/10/24 23:36, Manivannan Sadhasivam wrote:
> On Mon, Oct 07, 2024 at 01:03:15PM +0900, Damien Le Moal wrote:
>> Some endpoint controllers have requirements on the alignment of the
>> controller physical memory address that must be used to map a RC PCI
>> address region. For instance, the rockchip endpoint controller uses
>> at most the lower 20 bits of a physical memory address region as the
>> lower bits of an RC PCI address. For mapping a PCI address region of
>> size bytes starting from pci_addr, the exact number of address bits
>> used is the number of address bits changing in the address range
>> [pci_addr..pci_addr + size - 1].
>>
>> For this example, this creates the following constraints:
>> 1) The offset into the controller physical memory allocated for a
>>    mapping depends on the mapping size *and* the starting PCI address
>>    for the mapping.
>> 2) A mapping size cannot exceed the controller windows size (1MB) minus
>>    the offset needed into the allocated physical memory, which can end
>>    up being a smaller size than the desired mapping size.
>>
>> Handling these constraints independently of the controller being used
>> in an endpoint function driver is not possible with the current EPC
>> API as only the ->align field in struct pci_epc_features is provided
>> and used for BAR (inbound ATU mappings) mapping. A new API is needed
>> for function drivers to discover mapping constraints and handle
>> non-static requirements based on the RC PCI address range to access.
>>
>> Introduce the function pci_epc_map_align() and the endpoint controller
>> operation ->map_align to allow endpoint function drivers to obtain the
>> size and the offset into a controller address region that must be
>> allocated and mapped to access an RC PCI address region. The size
>> of the mapping provided by pci_epc_map_align() can then be used as the
>> size argument for the function pci_epc_mem_alloc_addr().
>> The offset into the allocated controller memory provided can be used to
>> correctly handle data transfers.
>>
>> For endpoint controllers that have PCI address alignment constraints,
>> pci_epc_map_align() may indicate upon return an effective PCI address
>> region mapping size that is smaller (but not 0) than the requested PCI
>> address region size. For such case, an endpoint function driver must
>> handle data accesses over the desired PCI address range in fragments,
>> by repeatedly using pci_epc_map_align() over the PCI address range.
>>
>> The controller operation ->map_align is optional: controllers that do
>> not have any alignment constraints for mapping a RC PCI address region
>> do not need to implement this operation. For such controllers,
>> pci_epc_map_align() always returns the mapping size as equal to the
>> requested size of the PCI region and an offset equal to 0.
>>
>> The new structure struct pci_epc_map is introduced to represent a
>> mapping start PCI address, mapping effective size, the size and offset
>> into the controller memory needed for mapping the PCI address region as
>> well as the physical and virtual CPU addresses of the mapping (phys_base
>> and virt_base fields). For convenience, the physical and virtual CPU
>> addresses within that mapping to access the target RC PCI address region
>> are also provided (phys_addr and virt_addr fields).
>>
> 
> I'm fine with the concept of this patch, but I don't get why you need an API for
> this and not just a callback to be used in the pci_epc_mem_{map/unmap} APIs.
> Furthermore, I don't see an user of this API (in 3 series you've sent out so
> far). Let me know if I failed to spot it.
> 
> Also, the API name pci_epc_map_align() sounds like it does the mapping, but it
> doesn't. So I'd not have it exposed as an API at all.

OK. Fine with me. I will move this inside pci_epc_mem_map(). But note that
without this function, pci_epc_mem_alloc_addr() and pci_epc_map_addr() are
totally useless for EP controllers that have a mapping alignment requirement,
which without the pci_epc_map_align() function, an endpoint function driver
cannot discover *at all* currently. That does not fix the overall API of EPC...

By not having pci_epc_map_align(), pci_epc_mem_alloc_addr() and
pci_epc_map_addr() remain broken, but the introduction of pci_epc_mem_map() does
provide a working solution for the general case.

So I think we will still need to do something about this bad state of the API later.


-- 
Damien Le Moal
Western Digital Research

Re: [PATCH v4 4/7] PCI: endpoint: Introduce pci_epc_mem_map()/unmap()

[Damien Le Moal]

On 10/11/24 01:43, Manivannan Sadhasivam wrote:
> On Mon, Oct 07, 2024 at 01:03:16PM +0900, Damien Le Moal wrote:
>> Introduce the function pci_epc_mem_map() to facilitate controller memory
>> address allocation and mapping to a RC PCI address region in endpoint
>> function drivers.
>>
>> This function first uses pci_epc_map_align() to determine the controller
>> memory address size (and offset into) depending on the controller
>> address alignment constraints. The result of this function is used to
>> allocate a controller physical memory region using
>> pci_epc_mem_alloc_addr() and map that memory to the RC PCI address
>> space with pci_epc_map_addr().
>>
>> Since pci_epc_map_align() may indicate that the effective mapping
>> of a PCI address region is smaller than the user requested size,
>> pci_epc_mem_map() may only partially map the RC PCI address region
>> specified. It is the responsibility of the caller (an endpoint function
>> driver) to handle such smaller mapping.
>>
>> The counterpart of pci_epc_mem_map() to unmap and free the controller
>> memory address region is pci_epc_mem_unmap().
>>
>> Both functions operate using a struct pci_epc_map data structure
>> Endpoint function drivers can use struct pci_epc_map to access the
>> mapped RC PCI address region using the ->virt_addr and ->pci_size
>> fields.
>>
>> Co-developed-by: Rick Wertenbroek <rick.wertenbroek@gmail.com>
>> Signed-off-by: Rick Wertenbroek <rick.wertenbroek@gmail.com>
>> Signed-off-by: Damien Le Moal <dlemoal@kernel.org>
> 
> Looks good to me. Just one comment below.
> 
>> Reviewed-by: Niklas Cassel <cassel@kernel.org>
>> ---
>>  drivers/pci/endpoint/pci-epc-core.c | 78 +++++++++++++++++++++++++++++
>>  include/linux/pci-epc.h             |  4 ++
>>  2 files changed, 82 insertions(+)
>>
>> diff --git a/drivers/pci/endpoint/pci-epc-core.c b/drivers/pci/endpoint/pci-epc-core.c
>> index 1adccf07c33e..d03c753d0a53 100644
>> --- a/drivers/pci/endpoint/pci-epc-core.c
>> +++ b/drivers/pci/endpoint/pci-epc-core.c
>> @@ -532,6 +532,84 @@ int pci_epc_map_addr(struct pci_epc *epc, u8 func_no, u8 vfunc_no,
>>  }
>>  EXPORT_SYMBOL_GPL(pci_epc_map_addr);
>>  
>> +/**
>> + * pci_epc_mem_map() - allocate and map a PCI address to a CPU address
>> + * @epc: the EPC device on which the CPU address is to be allocated and mapped
>> + * @func_no: the physical endpoint function number in the EPC device
>> + * @vfunc_no: the virtual endpoint function number in the physical function
>> + * @pci_addr: PCI address to which the CPU address should be mapped
>> + * @pci_size: the number of bytes to map starting from @pci_addr
>> + * @map: where to return the mapping information
>> + *
>> + * Allocate a controller memory address region and map it to a RC PCI address
>> + * region, taking into account the controller physical address mapping
>> + * constraints using pci_epc_map_align().
>> + * The effective size of the PCI address range mapped from @pci_addr is
>> + * indicated by @map->pci_size. This size may be less than the requested
>> + * @pci_size. The local virtual CPU address for the mapping is indicated by
>> + * @map->virt_addr (@map->phys_addr indicates the physical address).
>> + * The size and CPU address of the controller memory allocated and mapped are
>> + * respectively indicated by @map->map_size and @map->virt_base (and
>> + * @map->phys_base).
>> + *
>> + * Returns 0 on success and a negative error code in case of error.
>> + */
>> +int pci_epc_mem_map(struct pci_epc *epc, u8 func_no, u8 vfunc_no,
>> +		    u64 pci_addr, size_t pci_size, struct pci_epc_map *map)
>> +{
>> +	int ret;
>> +
>> +	ret = pci_epc_map_align(epc, func_no, vfunc_no, pci_addr, pci_size, map);
> 
> I don't like the fact that one structure is passed to two functions and both
> modify some members. If you get rid of the pci_epc_map_align() API and just use
> the callback, then the arguments could be passed on their own without the 'map'
> struct.

That would be far too many arguments. The pci_epc functions already have many
(minimum of 3 for epc, func and vfunc). So I prefer trying to minimize that.

I removed clearing map->map_size in the unmap function. I had added that to make
that function a nop if it is called twice with for the same map. But given that
pci_epc_unmap_addr() and pci_epc_mem_free_addr() will do nothing for memory that
is not mapped/allocated, it is not super useful. Doing such double call would be
a bug in the endpoint function anyway.

-- 
Damien Le Moal
Western Digital Research

Re: [PATCH v4 3/7] PCI: endpoint: Introduce pci_epc_map_align()

[Manivannan Sadhasivam]

On Fri, Oct 11, 2024 at 10:07:30AM +0900, Damien Le Moal wrote:
> On 10/10/24 23:36, Manivannan Sadhasivam wrote:
> > On Mon, Oct 07, 2024 at 01:03:15PM +0900, Damien Le Moal wrote:
> >> Some endpoint controllers have requirements on the alignment of the
> >> controller physical memory address that must be used to map a RC PCI
> >> address region. For instance, the rockchip endpoint controller uses
> >> at most the lower 20 bits of a physical memory address region as the
> >> lower bits of an RC PCI address. For mapping a PCI address region of
> >> size bytes starting from pci_addr, the exact number of address bits
> >> used is the number of address bits changing in the address range
> >> [pci_addr..pci_addr + size - 1].
> >>
> >> For this example, this creates the following constraints:
> >> 1) The offset into the controller physical memory allocated for a
> >>    mapping depends on the mapping size *and* the starting PCI address
> >>    for the mapping.
> >> 2) A mapping size cannot exceed the controller windows size (1MB) minus
> >>    the offset needed into the allocated physical memory, which can end
> >>    up being a smaller size than the desired mapping size.
> >>
> >> Handling these constraints independently of the controller being used
> >> in an endpoint function driver is not possible with the current EPC
> >> API as only the ->align field in struct pci_epc_features is provided
> >> and used for BAR (inbound ATU mappings) mapping. A new API is needed
> >> for function drivers to discover mapping constraints and handle
> >> non-static requirements based on the RC PCI address range to access.
> >>
> >> Introduce the function pci_epc_map_align() and the endpoint controller
> >> operation ->map_align to allow endpoint function drivers to obtain the
> >> size and the offset into a controller address region that must be
> >> allocated and mapped to access an RC PCI address region. The size
> >> of the mapping provided by pci_epc_map_align() can then be used as the
> >> size argument for the function pci_epc_mem_alloc_addr().
> >> The offset into the allocated controller memory provided can be used to
> >> correctly handle data transfers.
> >>
> >> For endpoint controllers that have PCI address alignment constraints,
> >> pci_epc_map_align() may indicate upon return an effective PCI address
> >> region mapping size that is smaller (but not 0) than the requested PCI
> >> address region size. For such case, an endpoint function driver must
> >> handle data accesses over the desired PCI address range in fragments,
> >> by repeatedly using pci_epc_map_align() over the PCI address range.
> >>
> >> The controller operation ->map_align is optional: controllers that do
> >> not have any alignment constraints for mapping a RC PCI address region
> >> do not need to implement this operation. For such controllers,
> >> pci_epc_map_align() always returns the mapping size as equal to the
> >> requested size of the PCI region and an offset equal to 0.
> >>
> >> The new structure struct pci_epc_map is introduced to represent a
> >> mapping start PCI address, mapping effective size, the size and offset
> >> into the controller memory needed for mapping the PCI address region as
> >> well as the physical and virtual CPU addresses of the mapping (phys_base
> >> and virt_base fields). For convenience, the physical and virtual CPU
> >> addresses within that mapping to access the target RC PCI address region
> >> are also provided (phys_addr and virt_addr fields).
> >>
> > 
> > I'm fine with the concept of this patch, but I don't get why you need an API for
> > this and not just a callback to be used in the pci_epc_mem_{map/unmap} APIs.
> > Furthermore, I don't see an user of this API (in 3 series you've sent out so
> > far). Let me know if I failed to spot it.
> > 
> > Also, the API name pci_epc_map_align() sounds like it does the mapping, but it
> > doesn't. So I'd not have it exposed as an API at all.
> 
> OK. Fine with me. I will move this inside pci_epc_mem_map(). But note that
> without this function, pci_epc_mem_alloc_addr() and pci_epc_map_addr() are
> totally useless for EP controllers that have a mapping alignment requirement,
> which without the pci_epc_map_align() function, an endpoint function driver
> cannot discover *at all* currently. That does not fix the overall API of EPC...
> 

Not at all. EPF drivers still can use "epf_mhi->epc_features->align" to discover
the alignment requirement and calculate the offset on their own (please see
pci-epf-mhi). But I'm not in favor of that approach since the APIs need to do
that job and that's why I like your pci_epc_mem_map() API.

> By not having pci_epc_map_align(), pci_epc_mem_alloc_addr() and
> pci_epc_map_addr() remain broken, but the introduction of pci_epc_mem_map() does
> provide a working solution for the general case.
> 
> So I think we will still need to do something about this bad state of the API later.
> 

We can always rework the APIs to incorporate the alignment requirement.

- Mani

-- 
மணிவண்ணன் சதாசிவம்

Re: [PATCH v4 4/7] PCI: endpoint: Introduce pci_epc_mem_map()/unmap()

[Manivannan Sadhasivam]

On Fri, Oct 11, 2024 at 11:01:09AM +0900, Damien Le Moal wrote:
> On 10/11/24 01:43, Manivannan Sadhasivam wrote:
> > On Mon, Oct 07, 2024 at 01:03:16PM +0900, Damien Le Moal wrote:
> >> Introduce the function pci_epc_mem_map() to facilitate controller memory
> >> address allocation and mapping to a RC PCI address region in endpoint
> >> function drivers.
> >>
> >> This function first uses pci_epc_map_align() to determine the controller
> >> memory address size (and offset into) depending on the controller
> >> address alignment constraints. The result of this function is used to
> >> allocate a controller physical memory region using
> >> pci_epc_mem_alloc_addr() and map that memory to the RC PCI address
> >> space with pci_epc_map_addr().
> >>
> >> Since pci_epc_map_align() may indicate that the effective mapping
> >> of a PCI address region is smaller than the user requested size,
> >> pci_epc_mem_map() may only partially map the RC PCI address region
> >> specified. It is the responsibility of the caller (an endpoint function
> >> driver) to handle such smaller mapping.
> >>
> >> The counterpart of pci_epc_mem_map() to unmap and free the controller
> >> memory address region is pci_epc_mem_unmap().
> >>
> >> Both functions operate using a struct pci_epc_map data structure
> >> Endpoint function drivers can use struct pci_epc_map to access the
> >> mapped RC PCI address region using the ->virt_addr and ->pci_size
> >> fields.
> >>
> >> Co-developed-by: Rick Wertenbroek <rick.wertenbroek@gmail.com>
> >> Signed-off-by: Rick Wertenbroek <rick.wertenbroek@gmail.com>
> >> Signed-off-by: Damien Le Moal <dlemoal@kernel.org>
> > 
> > Looks good to me. Just one comment below.
> > 
> >> Reviewed-by: Niklas Cassel <cassel@kernel.org>
> >> ---
> >>  drivers/pci/endpoint/pci-epc-core.c | 78 +++++++++++++++++++++++++++++
> >>  include/linux/pci-epc.h             |  4 ++
> >>  2 files changed, 82 insertions(+)
> >>
> >> diff --git a/drivers/pci/endpoint/pci-epc-core.c b/drivers/pci/endpoint/pci-epc-core.c
> >> index 1adccf07c33e..d03c753d0a53 100644
> >> --- a/drivers/pci/endpoint/pci-epc-core.c
> >> +++ b/drivers/pci/endpoint/pci-epc-core.c
> >> @@ -532,6 +532,84 @@ int pci_epc_map_addr(struct pci_epc *epc, u8 func_no, u8 vfunc_no,
> >>  }
> >>  EXPORT_SYMBOL_GPL(pci_epc_map_addr);
> >>  
> >> +/**
> >> + * pci_epc_mem_map() - allocate and map a PCI address to a CPU address
> >> + * @epc: the EPC device on which the CPU address is to be allocated and mapped
> >> + * @func_no: the physical endpoint function number in the EPC device
> >> + * @vfunc_no: the virtual endpoint function number in the physical function
> >> + * @pci_addr: PCI address to which the CPU address should be mapped
> >> + * @pci_size: the number of bytes to map starting from @pci_addr
> >> + * @map: where to return the mapping information
> >> + *
> >> + * Allocate a controller memory address region and map it to a RC PCI address
> >> + * region, taking into account the controller physical address mapping
> >> + * constraints using pci_epc_map_align().
> >> + * The effective size of the PCI address range mapped from @pci_addr is
> >> + * indicated by @map->pci_size. This size may be less than the requested
> >> + * @pci_size. The local virtual CPU address for the mapping is indicated by
> >> + * @map->virt_addr (@map->phys_addr indicates the physical address).
> >> + * The size and CPU address of the controller memory allocated and mapped are
> >> + * respectively indicated by @map->map_size and @map->virt_base (and
> >> + * @map->phys_base).
> >> + *
> >> + * Returns 0 on success and a negative error code in case of error.
> >> + */
> >> +int pci_epc_mem_map(struct pci_epc *epc, u8 func_no, u8 vfunc_no,
> >> +		    u64 pci_addr, size_t pci_size, struct pci_epc_map *map)
> >> +{
> >> +	int ret;
> >> +
> >> +	ret = pci_epc_map_align(epc, func_no, vfunc_no, pci_addr, pci_size, map);
> > 
> > I don't like the fact that one structure is passed to two functions and both
> > modify some members. If you get rid of the pci_epc_map_align() API and just use
> > the callback, then the arguments could be passed on their own without the 'map'
> > struct.
> 
> That would be far too many arguments. The pci_epc functions already have many
> (minimum of 3 for epc, func and vfunc). So I prefer trying to minimize that.
> 

Actually, there is no need to pass 'func, vfunc' as I don't think the controller
can have different alignment requirements for each functions.

So I'm envisioning a callback like this:

	u64 (*align_addr)(struct pci_epc *epc, u64 addr, size_t *offset, size_t *size);

And there is no need to check the error return also. Also you can avoid passing
'offset', as the caller can derive the offset using the mapped and unmapped
addresses. This also avoids the extra local function and allows the callers to
just use the callback directly.

NOTE: Please do not respin the patches without concluding the comments on
previous revisions. I understand that you want to get the series merged asap and
I do have the same adjective.

- Mani

-- 
மணிவண்ணன் சதாசிவம்

Re: [PATCH v4 3/7] PCI: endpoint: Introduce pci_epc_map_align()

[Damien Le Moal]

On 10/12/24 15:32, Manivannan Sadhasivam wrote:
> On Fri, Oct 11, 2024 at 10:07:30AM +0900, Damien Le Moal wrote:
>> On 10/10/24 23:36, Manivannan Sadhasivam wrote:
>>> On Mon, Oct 07, 2024 at 01:03:15PM +0900, Damien Le Moal wrote:
>>>> Some endpoint controllers have requirements on the alignment of the
>>>> controller physical memory address that must be used to map a RC PCI
>>>> address region. For instance, the rockchip endpoint controller uses
>>>> at most the lower 20 bits of a physical memory address region as the
>>>> lower bits of an RC PCI address. For mapping a PCI address region of
>>>> size bytes starting from pci_addr, the exact number of address bits
>>>> used is the number of address bits changing in the address range
>>>> [pci_addr..pci_addr + size - 1].
>>>>
>>>> For this example, this creates the following constraints:
>>>> 1) The offset into the controller physical memory allocated for a
>>>>    mapping depends on the mapping size *and* the starting PCI address
>>>>    for the mapping.
>>>> 2) A mapping size cannot exceed the controller windows size (1MB) minus
>>>>    the offset needed into the allocated physical memory, which can end
>>>>    up being a smaller size than the desired mapping size.
>>>>
>>>> Handling these constraints independently of the controller being used
>>>> in an endpoint function driver is not possible with the current EPC
>>>> API as only the ->align field in struct pci_epc_features is provided
>>>> and used for BAR (inbound ATU mappings) mapping. A new API is needed
>>>> for function drivers to discover mapping constraints and handle
>>>> non-static requirements based on the RC PCI address range to access.
>>>>
>>>> Introduce the function pci_epc_map_align() and the endpoint controller
>>>> operation ->map_align to allow endpoint function drivers to obtain the
>>>> size and the offset into a controller address region that must be
>>>> allocated and mapped to access an RC PCI address region. The size
>>>> of the mapping provided by pci_epc_map_align() can then be used as the
>>>> size argument for the function pci_epc_mem_alloc_addr().
>>>> The offset into the allocated controller memory provided can be used to
>>>> correctly handle data transfers.
>>>>
>>>> For endpoint controllers that have PCI address alignment constraints,
>>>> pci_epc_map_align() may indicate upon return an effective PCI address
>>>> region mapping size that is smaller (but not 0) than the requested PCI
>>>> address region size. For such case, an endpoint function driver must
>>>> handle data accesses over the desired PCI address range in fragments,
>>>> by repeatedly using pci_epc_map_align() over the PCI address range.
>>>>
>>>> The controller operation ->map_align is optional: controllers that do
>>>> not have any alignment constraints for mapping a RC PCI address region
>>>> do not need to implement this operation. For such controllers,
>>>> pci_epc_map_align() always returns the mapping size as equal to the
>>>> requested size of the PCI region and an offset equal to 0.
>>>>
>>>> The new structure struct pci_epc_map is introduced to represent a
>>>> mapping start PCI address, mapping effective size, the size and offset
>>>> into the controller memory needed for mapping the PCI address region as
>>>> well as the physical and virtual CPU addresses of the mapping (phys_base
>>>> and virt_base fields). For convenience, the physical and virtual CPU
>>>> addresses within that mapping to access the target RC PCI address region
>>>> are also provided (phys_addr and virt_addr fields).
>>>>
>>>
>>> I'm fine with the concept of this patch, but I don't get why you need an API for
>>> this and not just a callback to be used in the pci_epc_mem_{map/unmap} APIs.
>>> Furthermore, I don't see an user of this API (in 3 series you've sent out so
>>> far). Let me know if I failed to spot it.
>>>
>>> Also, the API name pci_epc_map_align() sounds like it does the mapping, but it
>>> doesn't. So I'd not have it exposed as an API at all.
>>
>> OK. Fine with me. I will move this inside pci_epc_mem_map(). But note that
>> without this function, pci_epc_mem_alloc_addr() and pci_epc_map_addr() are
>> totally useless for EP controllers that have a mapping alignment requirement,
>> which without the pci_epc_map_align() function, an endpoint function driver
>> cannot discover *at all* currently. That does not fix the overall API of EPC...
>>
> 
> Not at all. EPF drivers still can use "epf_mhi->epc_features->align" to discover
> the alignment requirement and calculate the offset on their own (please see
> pci-epf-mhi). But I'm not in favor of that approach since the APIs need to do
> that job and that's why I like your pci_epc_mem_map() API.

That is *not* correct, at least in general. For two reasons:
1) epc_features->align defines alignment for BARs, that is, inbound windows
memory. It is not supposed to be about the outbound windows for mapping PCI
address space for doing mmio or DMA. Some controllers may have the same
alignment constraint for both ib and ob, in which case things will work, but
that is "just being lucky". I spent weeks with the RK3399 understanding that I
was not lucky with that one :)
2) A static alignment constraint does not work for all controllers. C.f. my
series fixing the RK3399 were I think I clearly explain that alignment of a
mapping depends on the PCI address AND the size being mapped, as both determine
the number of bits of address changing within the PCI address range to access.
Using a fixed boundary alignment for the RK3399 simply does not work at all. An
epf cannot know that simply looking at a fixed value...

What you said may be true for the mhi epf, because it requires special hardware
that has a simple fixed alignment constraint. ntb and vntb are also coded
assuming such constraint. So If I try to run ntb or vntg on the RK3399 it will
likely not work (actually it may, but out of sheer luck given that the addresses
that will be mapped will likely be aligned to 1MB, that is, the memory window size).

Developping the nvme epf driver where I was seeing completely random PCI
addresses for command buffers, I could make things work only after developping
the pci_epc_mem_map() with the controller operation telling the mapping
(.get_mem_map()) for every address to map.

> 
>> By not having pci_epc_map_align(), pci_epc_mem_alloc_addr() and
>> pci_epc_map_addr() remain broken, but the introduction of pci_epc_mem_map() does
>> provide a working solution for the general case.
>>
>> So I think we will still need to do something about this bad state of the API later.
>>
> 
> We can always rework the APIs to incorporate the alignment requirement.

See above. An API that advertise a simple alignment requirement will not work
for all controllers... But anyway, given that we are not getting any problem
report, people using the EP framework likely have setups that combine
controllers and endpoint drivers playing well together. So I do not think there
is any urgency about the API. I really do need this series for the nvme endpoint
driver though, as a first step for the API improvement.

-- 
Damien Le Moal
Western Digital Research

Re: [PATCH v4 4/7] PCI: endpoint: Introduce pci_epc_mem_map()/unmap()

[Damien Le Moal]

On 10/12/24 16:56, Manivannan Sadhasivam wrote:
> On Fri, Oct 11, 2024 at 11:01:09AM +0900, Damien Le Moal wrote:
>> On 10/11/24 01:43, Manivannan Sadhasivam wrote:
>>> On Mon, Oct 07, 2024 at 01:03:16PM +0900, Damien Le Moal wrote:
>>>> Introduce the function pci_epc_mem_map() to facilitate controller memory
>>>> address allocation and mapping to a RC PCI address region in endpoint
>>>> function drivers.
>>>>
>>>> This function first uses pci_epc_map_align() to determine the controller
>>>> memory address size (and offset into) depending on the controller
>>>> address alignment constraints. The result of this function is used to
>>>> allocate a controller physical memory region using
>>>> pci_epc_mem_alloc_addr() and map that memory to the RC PCI address
>>>> space with pci_epc_map_addr().
>>>>
>>>> Since pci_epc_map_align() may indicate that the effective mapping
>>>> of a PCI address region is smaller than the user requested size,
>>>> pci_epc_mem_map() may only partially map the RC PCI address region
>>>> specified. It is the responsibility of the caller (an endpoint function
>>>> driver) to handle such smaller mapping.
>>>>
>>>> The counterpart of pci_epc_mem_map() to unmap and free the controller
>>>> memory address region is pci_epc_mem_unmap().
>>>>
>>>> Both functions operate using a struct pci_epc_map data structure
>>>> Endpoint function drivers can use struct pci_epc_map to access the
>>>> mapped RC PCI address region using the ->virt_addr and ->pci_size
>>>> fields.
>>>>
>>>> Co-developed-by: Rick Wertenbroek <rick.wertenbroek@gmail.com>
>>>> Signed-off-by: Rick Wertenbroek <rick.wertenbroek@gmail.com>
>>>> Signed-off-by: Damien Le Moal <dlemoal@kernel.org>
>>>
>>> Looks good to me. Just one comment below.
>>>
>>>> Reviewed-by: Niklas Cassel <cassel@kernel.org>
>>>> ---
>>>>  drivers/pci/endpoint/pci-epc-core.c | 78 +++++++++++++++++++++++++++++
>>>>  include/linux/pci-epc.h             |  4 ++
>>>>  2 files changed, 82 insertions(+)
>>>>
>>>> diff --git a/drivers/pci/endpoint/pci-epc-core.c b/drivers/pci/endpoint/pci-epc-core.c
>>>> index 1adccf07c33e..d03c753d0a53 100644
>>>> --- a/drivers/pci/endpoint/pci-epc-core.c
>>>> +++ b/drivers/pci/endpoint/pci-epc-core.c
>>>> @@ -532,6 +532,84 @@ int pci_epc_map_addr(struct pci_epc *epc, u8 func_no, u8 vfunc_no,
>>>>  }
>>>>  EXPORT_SYMBOL_GPL(pci_epc_map_addr);
>>>>  
>>>> +/**
>>>> + * pci_epc_mem_map() - allocate and map a PCI address to a CPU address
>>>> + * @epc: the EPC device on which the CPU address is to be allocated and mapped
>>>> + * @func_no: the physical endpoint function number in the EPC device
>>>> + * @vfunc_no: the virtual endpoint function number in the physical function
>>>> + * @pci_addr: PCI address to which the CPU address should be mapped
>>>> + * @pci_size: the number of bytes to map starting from @pci_addr
>>>> + * @map: where to return the mapping information
>>>> + *
>>>> + * Allocate a controller memory address region and map it to a RC PCI address
>>>> + * region, taking into account the controller physical address mapping
>>>> + * constraints using pci_epc_map_align().
>>>> + * The effective size of the PCI address range mapped from @pci_addr is
>>>> + * indicated by @map->pci_size. This size may be less than the requested
>>>> + * @pci_size. The local virtual CPU address for the mapping is indicated by
>>>> + * @map->virt_addr (@map->phys_addr indicates the physical address).
>>>> + * The size and CPU address of the controller memory allocated and mapped are
>>>> + * respectively indicated by @map->map_size and @map->virt_base (and
>>>> + * @map->phys_base).
>>>> + *
>>>> + * Returns 0 on success and a negative error code in case of error.
>>>> + */
>>>> +int pci_epc_mem_map(struct pci_epc *epc, u8 func_no, u8 vfunc_no,
>>>> +		    u64 pci_addr, size_t pci_size, struct pci_epc_map *map)
>>>> +{
>>>> +	int ret;
>>>> +
>>>> +	ret = pci_epc_map_align(epc, func_no, vfunc_no, pci_addr, pci_size, map);
>>>
>>> I don't like the fact that one structure is passed to two functions and both
>>> modify some members. If you get rid of the pci_epc_map_align() API and just use
>>> the callback, then the arguments could be passed on their own without the 'map'
>>> struct.
>>
>> That would be far too many arguments. The pci_epc functions already have many
>> (minimum of 3 for epc, func and vfunc). So I prefer trying to minimize that.
>>
> 
> Actually, there is no need to pass 'func, vfunc' as I don't think the controller
> can have different alignment requirements for each functions.
> 
> So I'm envisioning a callback like this:
> 
> 	u64 (*align_addr)(struct pci_epc *epc, u64 addr, size_t *offset, size_t *size);
> 
> And there is no need to check the error return also. Also you can avoid passing
> 'offset', as the caller can derive the offset using the mapped and unmapped
> addresses. This also avoids the extra local function and allows the callers to
> just use the callback directly.
> 
> NOTE: Please do not respin the patches without concluding the comments on
> previous revisions. I understand that you want to get the series merged asap and
> I do have the same adjective.

v5 that I posted yesterday addressed all your comment, except the one above.
The controller operation (renamed get_mem_map) still uses the pci_mem_map
structure as argument.

I need to respin a v6. Do you want me to change the controller op as you suggest
above ?

> 
> - Mani
> 


-- 
Damien Le Moal
Western Digital Research

Re: [PATCH v4 3/7] PCI: endpoint: Introduce pci_epc_map_align()

[Manivannan Sadhasivam]

On Sat, Oct 12, 2024 at 05:30:29PM +0900, Damien Le Moal wrote:
> On 10/12/24 15:32, Manivannan Sadhasivam wrote:
> > On Fri, Oct 11, 2024 at 10:07:30AM +0900, Damien Le Moal wrote:
> >> On 10/10/24 23:36, Manivannan Sadhasivam wrote:
> >>> On Mon, Oct 07, 2024 at 01:03:15PM +0900, Damien Le Moal wrote:
> >>>> Some endpoint controllers have requirements on the alignment of the
> >>>> controller physical memory address that must be used to map a RC PCI
> >>>> address region. For instance, the rockchip endpoint controller uses
> >>>> at most the lower 20 bits of a physical memory address region as the
> >>>> lower bits of an RC PCI address. For mapping a PCI address region of
> >>>> size bytes starting from pci_addr, the exact number of address bits
> >>>> used is the number of address bits changing in the address range
> >>>> [pci_addr..pci_addr + size - 1].
> >>>>
> >>>> For this example, this creates the following constraints:
> >>>> 1) The offset into the controller physical memory allocated for a
> >>>>    mapping depends on the mapping size *and* the starting PCI address
> >>>>    for the mapping.
> >>>> 2) A mapping size cannot exceed the controller windows size (1MB) minus
> >>>>    the offset needed into the allocated physical memory, which can end
> >>>>    up being a smaller size than the desired mapping size.
> >>>>
> >>>> Handling these constraints independently of the controller being used
> >>>> in an endpoint function driver is not possible with the current EPC
> >>>> API as only the ->align field in struct pci_epc_features is provided
> >>>> and used for BAR (inbound ATU mappings) mapping. A new API is needed
> >>>> for function drivers to discover mapping constraints and handle
> >>>> non-static requirements based on the RC PCI address range to access.
> >>>>
> >>>> Introduce the function pci_epc_map_align() and the endpoint controller
> >>>> operation ->map_align to allow endpoint function drivers to obtain the
> >>>> size and the offset into a controller address region that must be
> >>>> allocated and mapped to access an RC PCI address region. The size
> >>>> of the mapping provided by pci_epc_map_align() can then be used as the
> >>>> size argument for the function pci_epc_mem_alloc_addr().
> >>>> The offset into the allocated controller memory provided can be used to
> >>>> correctly handle data transfers.
> >>>>
> >>>> For endpoint controllers that have PCI address alignment constraints,
> >>>> pci_epc_map_align() may indicate upon return an effective PCI address
> >>>> region mapping size that is smaller (but not 0) than the requested PCI
> >>>> address region size. For such case, an endpoint function driver must
> >>>> handle data accesses over the desired PCI address range in fragments,
> >>>> by repeatedly using pci_epc_map_align() over the PCI address range.
> >>>>
> >>>> The controller operation ->map_align is optional: controllers that do
> >>>> not have any alignment constraints for mapping a RC PCI address region
> >>>> do not need to implement this operation. For such controllers,
> >>>> pci_epc_map_align() always returns the mapping size as equal to the
> >>>> requested size of the PCI region and an offset equal to 0.
> >>>>
> >>>> The new structure struct pci_epc_map is introduced to represent a
> >>>> mapping start PCI address, mapping effective size, the size and offset
> >>>> into the controller memory needed for mapping the PCI address region as
> >>>> well as the physical and virtual CPU addresses of the mapping (phys_base
> >>>> and virt_base fields). For convenience, the physical and virtual CPU
> >>>> addresses within that mapping to access the target RC PCI address region
> >>>> are also provided (phys_addr and virt_addr fields).
> >>>>
> >>>
> >>> I'm fine with the concept of this patch, but I don't get why you need an API for
> >>> this and not just a callback to be used in the pci_epc_mem_{map/unmap} APIs.
> >>> Furthermore, I don't see an user of this API (in 3 series you've sent out so
> >>> far). Let me know if I failed to spot it.
> >>>
> >>> Also, the API name pci_epc_map_align() sounds like it does the mapping, but it
> >>> doesn't. So I'd not have it exposed as an API at all.
> >>
> >> OK. Fine with me. I will move this inside pci_epc_mem_map(). But note that
> >> without this function, pci_epc_mem_alloc_addr() and pci_epc_map_addr() are
> >> totally useless for EP controllers that have a mapping alignment requirement,
> >> which without the pci_epc_map_align() function, an endpoint function driver
> >> cannot discover *at all* currently. That does not fix the overall API of EPC...
> >>
> > 
> > Not at all. EPF drivers still can use "epf_mhi->epc_features->align" to discover
> > the alignment requirement and calculate the offset on their own (please see
> > pci-epf-mhi). But I'm not in favor of that approach since the APIs need to do
> > that job and that's why I like your pci_epc_mem_map() API.
> 
> That is *not* correct, at least in general. For two reasons:
> 1) epc_features->align defines alignment for BARs, that is, inbound windows
> memory. It is not supposed to be about the outbound windows for mapping PCI
> address space for doing mmio or DMA. Some controllers may have the same
> alignment constraint for both ib and ob, in which case things will work, but
> that is "just being lucky". I spent weeks with the RK3399 understanding that I
> was not lucky with that one :)
> 2) A static alignment constraint does not work for all controllers. C.f. my
> series fixing the RK3399 were I think I clearly explain that alignment of a
> mapping depends on the PCI address AND the size being mapped, as both determine
> the number of bits of address changing within the PCI address range to access.
> Using a fixed boundary alignment for the RK3399 simply does not work at all. An
> epf cannot know that simply looking at a fixed value...
> 
> What you said may be true for the mhi epf, because it requires special hardware
> that has a simple fixed alignment constraint. ntb and vntb are also coded
> assuming such constraint. So If I try to run ntb or vntg on the RK3399 it will
> likely not work (actually it may, but out of sheer luck given that the addresses
> that will be mapped will likely be aligned to 1MB, that is, the memory window size).
> 
> Developping the nvme epf driver where I was seeing completely random PCI
> addresses for command buffers, I could make things work only after developping
> the pci_epc_mem_map() with the controller operation telling the mapping
> (.get_mem_map()) for every address to map.
> 

Fair enough...

> > 
> >> By not having pci_epc_map_align(), pci_epc_mem_alloc_addr() and
> >> pci_epc_map_addr() remain broken, but the introduction of pci_epc_mem_map() does
> >> provide a working solution for the general case.
> >>
> >> So I think we will still need to do something about this bad state of the API later.
> >>
> > 
> > We can always rework the APIs to incorporate the alignment requirement.
> 
> See above. An API that advertise a simple alignment requirement will not work
> for all controllers... But anyway, given that we are not getting any problem
> report, people using the EP framework likely have setups that combine
> controllers and endpoint drivers playing well together. So I do not think there
> is any urgency about the API. I really do need this series for the nvme endpoint
> driver though, as a first step for the API improvement.
> 

No, what I meant was that you can use the new alignment callback (that takes
care of the complex alignment restrictions) in the existing map API to properly
map the addresses for all controllers in the future.

- Mani

-- 
மணிவண்ணன் சதாசிவம்

Re: [PATCH v4 4/7] PCI: endpoint: Introduce pci_epc_mem_map()/unmap()

[Manivannan Sadhasivam]

On Sat, Oct 12, 2024 at 05:33:34PM +0900, Damien Le Moal wrote:
> On 10/12/24 16:56, Manivannan Sadhasivam wrote:
> > On Fri, Oct 11, 2024 at 11:01:09AM +0900, Damien Le Moal wrote:
> >> On 10/11/24 01:43, Manivannan Sadhasivam wrote:
> >>> On Mon, Oct 07, 2024 at 01:03:16PM +0900, Damien Le Moal wrote:
> >>>> Introduce the function pci_epc_mem_map() to facilitate controller memory
> >>>> address allocation and mapping to a RC PCI address region in endpoint
> >>>> function drivers.
> >>>>
> >>>> This function first uses pci_epc_map_align() to determine the controller
> >>>> memory address size (and offset into) depending on the controller
> >>>> address alignment constraints. The result of this function is used to
> >>>> allocate a controller physical memory region using
> >>>> pci_epc_mem_alloc_addr() and map that memory to the RC PCI address
> >>>> space with pci_epc_map_addr().
> >>>>
> >>>> Since pci_epc_map_align() may indicate that the effective mapping
> >>>> of a PCI address region is smaller than the user requested size,
> >>>> pci_epc_mem_map() may only partially map the RC PCI address region
> >>>> specified. It is the responsibility of the caller (an endpoint function
> >>>> driver) to handle such smaller mapping.
> >>>>
> >>>> The counterpart of pci_epc_mem_map() to unmap and free the controller
> >>>> memory address region is pci_epc_mem_unmap().
> >>>>
> >>>> Both functions operate using a struct pci_epc_map data structure
> >>>> Endpoint function drivers can use struct pci_epc_map to access the
> >>>> mapped RC PCI address region using the ->virt_addr and ->pci_size
> >>>> fields.
> >>>>
> >>>> Co-developed-by: Rick Wertenbroek <rick.wertenbroek@gmail.com>
> >>>> Signed-off-by: Rick Wertenbroek <rick.wertenbroek@gmail.com>
> >>>> Signed-off-by: Damien Le Moal <dlemoal@kernel.org>
> >>>
> >>> Looks good to me. Just one comment below.
> >>>
> >>>> Reviewed-by: Niklas Cassel <cassel@kernel.org>
> >>>> ---
> >>>>  drivers/pci/endpoint/pci-epc-core.c | 78 +++++++++++++++++++++++++++++
> >>>>  include/linux/pci-epc.h             |  4 ++
> >>>>  2 files changed, 82 insertions(+)
> >>>>
> >>>> diff --git a/drivers/pci/endpoint/pci-epc-core.c b/drivers/pci/endpoint/pci-epc-core.c
> >>>> index 1adccf07c33e..d03c753d0a53 100644
> >>>> --- a/drivers/pci/endpoint/pci-epc-core.c
> >>>> +++ b/drivers/pci/endpoint/pci-epc-core.c
> >>>> @@ -532,6 +532,84 @@ int pci_epc_map_addr(struct pci_epc *epc, u8 func_no, u8 vfunc_no,
> >>>>  }
> >>>>  EXPORT_SYMBOL_GPL(pci_epc_map_addr);
> >>>>  
> >>>> +/**
> >>>> + * pci_epc_mem_map() - allocate and map a PCI address to a CPU address
> >>>> + * @epc: the EPC device on which the CPU address is to be allocated and mapped
> >>>> + * @func_no: the physical endpoint function number in the EPC device
> >>>> + * @vfunc_no: the virtual endpoint function number in the physical function
> >>>> + * @pci_addr: PCI address to which the CPU address should be mapped
> >>>> + * @pci_size: the number of bytes to map starting from @pci_addr
> >>>> + * @map: where to return the mapping information
> >>>> + *
> >>>> + * Allocate a controller memory address region and map it to a RC PCI address
> >>>> + * region, taking into account the controller physical address mapping
> >>>> + * constraints using pci_epc_map_align().
> >>>> + * The effective size of the PCI address range mapped from @pci_addr is
> >>>> + * indicated by @map->pci_size. This size may be less than the requested
> >>>> + * @pci_size. The local virtual CPU address for the mapping is indicated by
> >>>> + * @map->virt_addr (@map->phys_addr indicates the physical address).
> >>>> + * The size and CPU address of the controller memory allocated and mapped are
> >>>> + * respectively indicated by @map->map_size and @map->virt_base (and
> >>>> + * @map->phys_base).
> >>>> + *
> >>>> + * Returns 0 on success and a negative error code in case of error.
> >>>> + */
> >>>> +int pci_epc_mem_map(struct pci_epc *epc, u8 func_no, u8 vfunc_no,
> >>>> +		    u64 pci_addr, size_t pci_size, struct pci_epc_map *map)
> >>>> +{
> >>>> +	int ret;
> >>>> +
> >>>> +	ret = pci_epc_map_align(epc, func_no, vfunc_no, pci_addr, pci_size, map);
> >>>
> >>> I don't like the fact that one structure is passed to two functions and both
> >>> modify some members. If you get rid of the pci_epc_map_align() API and just use
> >>> the callback, then the arguments could be passed on their own without the 'map'
> >>> struct.
> >>
> >> That would be far too many arguments. The pci_epc functions already have many
> >> (minimum of 3 for epc, func and vfunc). So I prefer trying to minimize that.
> >>
> > 
> > Actually, there is no need to pass 'func, vfunc' as I don't think the controller
> > can have different alignment requirements for each functions.
> > 
> > So I'm envisioning a callback like this:
> > 
> > 	u64 (*align_addr)(struct pci_epc *epc, u64 addr, size_t *offset, size_t *size);
> > 
> > And there is no need to check the error return also. Also you can avoid passing
> > 'offset', as the caller can derive the offset using the mapped and unmapped
> > addresses. This also avoids the extra local function and allows the callers to
> > just use the callback directly.
> > 
> > NOTE: Please do not respin the patches without concluding the comments on
> > previous revisions. I understand that you want to get the series merged asap and
> > I do have the same adjective.
> 
> v5 that I posted yesterday addressed all your comment, except the one above.
> The controller operation (renamed get_mem_map) still uses the pci_mem_map
> structure as argument.
> 
> I need to respin a v6. Do you want me to change the controller op as you suggest
> above ?
> 

Please do so. I will apply it right away as everything else looks good.

- Mani

-- 
மணிவண்ணன் சதாசிவம்

Re: [PATCH v4 3/7] PCI: endpoint: Introduce pci_epc_map_align()

[Damien Le Moal]

On 10/12/24 18:40, Manivannan Sadhasivam wrote:
> On Sat, Oct 12, 2024 at 05:30:29PM +0900, Damien Le Moal wrote:
>> On 10/12/24 15:32, Manivannan Sadhasivam wrote:
>>> On Fri, Oct 11, 2024 at 10:07:30AM +0900, Damien Le Moal wrote:
>>>> On 10/10/24 23:36, Manivannan Sadhasivam wrote:
>>>>> On Mon, Oct 07, 2024 at 01:03:15PM +0900, Damien Le Moal wrote:
>>>>>> Some endpoint controllers have requirements on the alignment of the
>>>>>> controller physical memory address that must be used to map a RC PCI
>>>>>> address region. For instance, the rockchip endpoint controller uses
>>>>>> at most the lower 20 bits of a physical memory address region as the
>>>>>> lower bits of an RC PCI address. For mapping a PCI address region of
>>>>>> size bytes starting from pci_addr, the exact number of address bits
>>>>>> used is the number of address bits changing in the address range
>>>>>> [pci_addr..pci_addr + size - 1].
>>>>>>
>>>>>> For this example, this creates the following constraints:
>>>>>> 1) The offset into the controller physical memory allocated for a
>>>>>>    mapping depends on the mapping size *and* the starting PCI address
>>>>>>    for the mapping.
>>>>>> 2) A mapping size cannot exceed the controller windows size (1MB) minus
>>>>>>    the offset needed into the allocated physical memory, which can end
>>>>>>    up being a smaller size than the desired mapping size.
>>>>>>
>>>>>> Handling these constraints independently of the controller being used
>>>>>> in an endpoint function driver is not possible with the current EPC
>>>>>> API as only the ->align field in struct pci_epc_features is provided
>>>>>> and used for BAR (inbound ATU mappings) mapping. A new API is needed
>>>>>> for function drivers to discover mapping constraints and handle
>>>>>> non-static requirements based on the RC PCI address range to access.
>>>>>>
>>>>>> Introduce the function pci_epc_map_align() and the endpoint controller
>>>>>> operation ->map_align to allow endpoint function drivers to obtain the
>>>>>> size and the offset into a controller address region that must be
>>>>>> allocated and mapped to access an RC PCI address region. The size
>>>>>> of the mapping provided by pci_epc_map_align() can then be used as the
>>>>>> size argument for the function pci_epc_mem_alloc_addr().
>>>>>> The offset into the allocated controller memory provided can be used to
>>>>>> correctly handle data transfers.
>>>>>>
>>>>>> For endpoint controllers that have PCI address alignment constraints,
>>>>>> pci_epc_map_align() may indicate upon return an effective PCI address
>>>>>> region mapping size that is smaller (but not 0) than the requested PCI
>>>>>> address region size. For such case, an endpoint function driver must
>>>>>> handle data accesses over the desired PCI address range in fragments,
>>>>>> by repeatedly using pci_epc_map_align() over the PCI address range.
>>>>>>
>>>>>> The controller operation ->map_align is optional: controllers that do
>>>>>> not have any alignment constraints for mapping a RC PCI address region
>>>>>> do not need to implement this operation. For such controllers,
>>>>>> pci_epc_map_align() always returns the mapping size as equal to the
>>>>>> requested size of the PCI region and an offset equal to 0.
>>>>>>
>>>>>> The new structure struct pci_epc_map is introduced to represent a
>>>>>> mapping start PCI address, mapping effective size, the size and offset
>>>>>> into the controller memory needed for mapping the PCI address region as
>>>>>> well as the physical and virtual CPU addresses of the mapping (phys_base
>>>>>> and virt_base fields). For convenience, the physical and virtual CPU
>>>>>> addresses within that mapping to access the target RC PCI address region
>>>>>> are also provided (phys_addr and virt_addr fields).
>>>>>>
>>>>>
>>>>> I'm fine with the concept of this patch, but I don't get why you need an API for
>>>>> this and not just a callback to be used in the pci_epc_mem_{map/unmap} APIs.
>>>>> Furthermore, I don't see an user of this API (in 3 series you've sent out so
>>>>> far). Let me know if I failed to spot it.
>>>>>
>>>>> Also, the API name pci_epc_map_align() sounds like it does the mapping, but it
>>>>> doesn't. So I'd not have it exposed as an API at all.
>>>>
>>>> OK. Fine with me. I will move this inside pci_epc_mem_map(). But note that
>>>> without this function, pci_epc_mem_alloc_addr() and pci_epc_map_addr() are
>>>> totally useless for EP controllers that have a mapping alignment requirement,
>>>> which without the pci_epc_map_align() function, an endpoint function driver
>>>> cannot discover *at all* currently. That does not fix the overall API of EPC...
>>>>
>>>
>>> Not at all. EPF drivers still can use "epf_mhi->epc_features->align" to discover
>>> the alignment requirement and calculate the offset on their own (please see
>>> pci-epf-mhi). But I'm not in favor of that approach since the APIs need to do
>>> that job and that's why I like your pci_epc_mem_map() API.
>>
>> That is *not* correct, at least in general. For two reasons:
>> 1) epc_features->align defines alignment for BARs, that is, inbound windows
>> memory. It is not supposed to be about the outbound windows for mapping PCI
>> address space for doing mmio or DMA. Some controllers may have the same
>> alignment constraint for both ib and ob, in which case things will work, but
>> that is "just being lucky". I spent weeks with the RK3399 understanding that I
>> was not lucky with that one :)
>> 2) A static alignment constraint does not work for all controllers. C.f. my
>> series fixing the RK3399 were I think I clearly explain that alignment of a
>> mapping depends on the PCI address AND the size being mapped, as both determine
>> the number of bits of address changing within the PCI address range to access.
>> Using a fixed boundary alignment for the RK3399 simply does not work at all. An
>> epf cannot know that simply looking at a fixed value...
>>
>> What you said may be true for the mhi epf, because it requires special hardware
>> that has a simple fixed alignment constraint. ntb and vntb are also coded
>> assuming such constraint. So If I try to run ntb or vntg on the RK3399 it will
>> likely not work (actually it may, but out of sheer luck given that the addresses
>> that will be mapped will likely be aligned to 1MB, that is, the memory window size).
>>
>> Developping the nvme epf driver where I was seeing completely random PCI
>> addresses for command buffers, I could make things work only after developping
>> the pci_epc_mem_map() with the controller operation telling the mapping
>> (.get_mem_map()) for every address to map.
>>
> 
> Fair enough...
> 
>>>
>>>> By not having pci_epc_map_align(), pci_epc_mem_alloc_addr() and
>>>> pci_epc_map_addr() remain broken, but the introduction of pci_epc_mem_map() does
>>>> provide a working solution for the general case.
>>>>
>>>> So I think we will still need to do something about this bad state of the API later.
>>>>
>>>
>>> We can always rework the APIs to incorporate the alignment requirement.
>>
>> See above. An API that advertise a simple alignment requirement will not work
>> for all controllers... But anyway, given that we are not getting any problem
>> report, people using the EP framework likely have setups that combine
>> controllers and endpoint drivers playing well together. So I do not think there
>> is any urgency about the API. I really do need this series for the nvme endpoint
>> driver though, as a first step for the API improvement.
>>
> 
> No, what I meant was that you can use the new alignment callback (that takes
> care of the complex alignment restrictions) in the existing map API to properly
> map the addresses for all controllers in the future.

The existing map API cannot alone use ->align_addr() to get the correct mapping.
It is because the memory needed to handle a mapping may be larger than the PCI
address range to map. In fact, it almost always is larger for any controller
that has a constraint. As a result, the memory allocation side
(pci_epc_alloc_addr()) must also be aware of the mapping constraint and
resulting size of the memory to allocate... Hence pci_epc_mem_map() using both
functions.

-- 
Damien Le Moal
Western Digital Research

Re: [PATCH v4 4/7] PCI: endpoint: Introduce pci_epc_mem_map()/unmap()

[Damien Le Moal]

On 10/12/24 18:41, Manivannan Sadhasivam wrote:
> On Sat, Oct 12, 2024 at 05:33:34PM +0900, Damien Le Moal wrote:
>> On 10/12/24 16:56, Manivannan Sadhasivam wrote:
>>> On Fri, Oct 11, 2024 at 11:01:09AM +0900, Damien Le Moal wrote:
>>>> On 10/11/24 01:43, Manivannan Sadhasivam wrote:
>>>>> On Mon, Oct 07, 2024 at 01:03:16PM +0900, Damien Le Moal wrote:
>>>>>> Introduce the function pci_epc_mem_map() to facilitate controller memory
>>>>>> address allocation and mapping to a RC PCI address region in endpoint
>>>>>> function drivers.
>>>>>>
>>>>>> This function first uses pci_epc_map_align() to determine the controller
>>>>>> memory address size (and offset into) depending on the controller
>>>>>> address alignment constraints. The result of this function is used to
>>>>>> allocate a controller physical memory region using
>>>>>> pci_epc_mem_alloc_addr() and map that memory to the RC PCI address
>>>>>> space with pci_epc_map_addr().
>>>>>>
>>>>>> Since pci_epc_map_align() may indicate that the effective mapping
>>>>>> of a PCI address region is smaller than the user requested size,
>>>>>> pci_epc_mem_map() may only partially map the RC PCI address region
>>>>>> specified. It is the responsibility of the caller (an endpoint function
>>>>>> driver) to handle such smaller mapping.
>>>>>>
>>>>>> The counterpart of pci_epc_mem_map() to unmap and free the controller
>>>>>> memory address region is pci_epc_mem_unmap().
>>>>>>
>>>>>> Both functions operate using a struct pci_epc_map data structure
>>>>>> Endpoint function drivers can use struct pci_epc_map to access the
>>>>>> mapped RC PCI address region using the ->virt_addr and ->pci_size
>>>>>> fields.
>>>>>>
>>>>>> Co-developed-by: Rick Wertenbroek <rick.wertenbroek@gmail.com>
>>>>>> Signed-off-by: Rick Wertenbroek <rick.wertenbroek@gmail.com>
>>>>>> Signed-off-by: Damien Le Moal <dlemoal@kernel.org>
>>>>>
>>>>> Looks good to me. Just one comment below.
>>>>>
>>>>>> Reviewed-by: Niklas Cassel <cassel@kernel.org>
>>>>>> ---
>>>>>>  drivers/pci/endpoint/pci-epc-core.c | 78 +++++++++++++++++++++++++++++
>>>>>>  include/linux/pci-epc.h             |  4 ++
>>>>>>  2 files changed, 82 insertions(+)
>>>>>>
>>>>>> diff --git a/drivers/pci/endpoint/pci-epc-core.c b/drivers/pci/endpoint/pci-epc-core.c
>>>>>> index 1adccf07c33e..d03c753d0a53 100644
>>>>>> --- a/drivers/pci/endpoint/pci-epc-core.c
>>>>>> +++ b/drivers/pci/endpoint/pci-epc-core.c
>>>>>> @@ -532,6 +532,84 @@ int pci_epc_map_addr(struct pci_epc *epc, u8 func_no, u8 vfunc_no,
>>>>>>  }
>>>>>>  EXPORT_SYMBOL_GPL(pci_epc_map_addr);
>>>>>>  
>>>>>> +/**
>>>>>> + * pci_epc_mem_map() - allocate and map a PCI address to a CPU address
>>>>>> + * @epc: the EPC device on which the CPU address is to be allocated and mapped
>>>>>> + * @func_no: the physical endpoint function number in the EPC device
>>>>>> + * @vfunc_no: the virtual endpoint function number in the physical function
>>>>>> + * @pci_addr: PCI address to which the CPU address should be mapped
>>>>>> + * @pci_size: the number of bytes to map starting from @pci_addr
>>>>>> + * @map: where to return the mapping information
>>>>>> + *
>>>>>> + * Allocate a controller memory address region and map it to a RC PCI address
>>>>>> + * region, taking into account the controller physical address mapping
>>>>>> + * constraints using pci_epc_map_align().
>>>>>> + * The effective size of the PCI address range mapped from @pci_addr is
>>>>>> + * indicated by @map->pci_size. This size may be less than the requested
>>>>>> + * @pci_size. The local virtual CPU address for the mapping is indicated by
>>>>>> + * @map->virt_addr (@map->phys_addr indicates the physical address).
>>>>>> + * The size and CPU address of the controller memory allocated and mapped are
>>>>>> + * respectively indicated by @map->map_size and @map->virt_base (and
>>>>>> + * @map->phys_base).
>>>>>> + *
>>>>>> + * Returns 0 on success and a negative error code in case of error.
>>>>>> + */
>>>>>> +int pci_epc_mem_map(struct pci_epc *epc, u8 func_no, u8 vfunc_no,
>>>>>> +		    u64 pci_addr, size_t pci_size, struct pci_epc_map *map)
>>>>>> +{
>>>>>> +	int ret;
>>>>>> +
>>>>>> +	ret = pci_epc_map_align(epc, func_no, vfunc_no, pci_addr, pci_size, map);
>>>>>
>>>>> I don't like the fact that one structure is passed to two functions and both
>>>>> modify some members. If you get rid of the pci_epc_map_align() API and just use
>>>>> the callback, then the arguments could be passed on their own without the 'map'
>>>>> struct.
>>>>
>>>> That would be far too many arguments. The pci_epc functions already have many
>>>> (minimum of 3 for epc, func and vfunc). So I prefer trying to minimize that.
>>>>
>>>
>>> Actually, there is no need to pass 'func, vfunc' as I don't think the controller
>>> can have different alignment requirements for each functions.
>>>
>>> So I'm envisioning a callback like this:
>>>
>>> 	u64 (*align_addr)(struct pci_epc *epc, u64 addr, size_t *offset, size_t *size);
>>>
>>> And there is no need to check the error return also. Also you can avoid passing
>>> 'offset', as the caller can derive the offset using the mapped and unmapped
>>> addresses. This also avoids the extra local function and allows the callers to
>>> just use the callback directly.
>>>
>>> NOTE: Please do not respin the patches without concluding the comments on
>>> previous revisions. I understand that you want to get the series merged asap and
>>> I do have the same adjective.
>>
>> v5 that I posted yesterday addressed all your comment, except the one above.
>> The controller operation (renamed get_mem_map) still uses the pci_mem_map
>> structure as argument.
>>
>> I need to respin a v6. Do you want me to change the controller op as you suggest
>> above ?
>>
> 
> Please do so. I will apply it right away as everything else looks good.

Done. Under test now but everything is looking good. Note that I kept the offset
argument as otherwise pci_epc_mem_map() needs to re-calculate it but the
controller ->align_addr() may already have calculated it (or can calculate it
more efficiently than with a substraction).

So keeping offset as an argument is cleaner I think.

I also remove the mutex lock/unlock around ->align_addr() call as I really do
not think it is necessary at all (and if needed a controller implementation of
align_addr can always take that mutex).


-- 
Damien Le Moal
Western Digital Research

Re: [PATCH v4 3/7] PCI: endpoint: Introduce pci_epc_map_align()

[Manivannan Sadhasivam]

On Sat, Oct 12, 2024 at 08:06:46PM +0900, Damien Le Moal wrote:
> On 10/12/24 18:40, Manivannan Sadhasivam wrote:
> > On Sat, Oct 12, 2024 at 05:30:29PM +0900, Damien Le Moal wrote:
> >> On 10/12/24 15:32, Manivannan Sadhasivam wrote:
> >>> On Fri, Oct 11, 2024 at 10:07:30AM +0900, Damien Le Moal wrote:
> >>>> On 10/10/24 23:36, Manivannan Sadhasivam wrote:
> >>>>> On Mon, Oct 07, 2024 at 01:03:15PM +0900, Damien Le Moal wrote:
> >>>>>> Some endpoint controllers have requirements on the alignment of the
> >>>>>> controller physical memory address that must be used to map a RC PCI
> >>>>>> address region. For instance, the rockchip endpoint controller uses
> >>>>>> at most the lower 20 bits of a physical memory address region as the
> >>>>>> lower bits of an RC PCI address. For mapping a PCI address region of
> >>>>>> size bytes starting from pci_addr, the exact number of address bits
> >>>>>> used is the number of address bits changing in the address range
> >>>>>> [pci_addr..pci_addr + size - 1].
> >>>>>>
> >>>>>> For this example, this creates the following constraints:
> >>>>>> 1) The offset into the controller physical memory allocated for a
> >>>>>>    mapping depends on the mapping size *and* the starting PCI address
> >>>>>>    for the mapping.
> >>>>>> 2) A mapping size cannot exceed the controller windows size (1MB) minus
> >>>>>>    the offset needed into the allocated physical memory, which can end
> >>>>>>    up being a smaller size than the desired mapping size.
> >>>>>>
> >>>>>> Handling these constraints independently of the controller being used
> >>>>>> in an endpoint function driver is not possible with the current EPC
> >>>>>> API as only the ->align field in struct pci_epc_features is provided
> >>>>>> and used for BAR (inbound ATU mappings) mapping. A new API is needed
> >>>>>> for function drivers to discover mapping constraints and handle
> >>>>>> non-static requirements based on the RC PCI address range to access.
> >>>>>>
> >>>>>> Introduce the function pci_epc_map_align() and the endpoint controller
> >>>>>> operation ->map_align to allow endpoint function drivers to obtain the
> >>>>>> size and the offset into a controller address region that must be
> >>>>>> allocated and mapped to access an RC PCI address region. The size
> >>>>>> of the mapping provided by pci_epc_map_align() can then be used as the
> >>>>>> size argument for the function pci_epc_mem_alloc_addr().
> >>>>>> The offset into the allocated controller memory provided can be used to
> >>>>>> correctly handle data transfers.
> >>>>>>
> >>>>>> For endpoint controllers that have PCI address alignment constraints,
> >>>>>> pci_epc_map_align() may indicate upon return an effective PCI address
> >>>>>> region mapping size that is smaller (but not 0) than the requested PCI
> >>>>>> address region size. For such case, an endpoint function driver must
> >>>>>> handle data accesses over the desired PCI address range in fragments,
> >>>>>> by repeatedly using pci_epc_map_align() over the PCI address range.
> >>>>>>
> >>>>>> The controller operation ->map_align is optional: controllers that do
> >>>>>> not have any alignment constraints for mapping a RC PCI address region
> >>>>>> do not need to implement this operation. For such controllers,
> >>>>>> pci_epc_map_align() always returns the mapping size as equal to the
> >>>>>> requested size of the PCI region and an offset equal to 0.
> >>>>>>
> >>>>>> The new structure struct pci_epc_map is introduced to represent a
> >>>>>> mapping start PCI address, mapping effective size, the size and offset
> >>>>>> into the controller memory needed for mapping the PCI address region as
> >>>>>> well as the physical and virtual CPU addresses of the mapping (phys_base
> >>>>>> and virt_base fields). For convenience, the physical and virtual CPU
> >>>>>> addresses within that mapping to access the target RC PCI address region
> >>>>>> are also provided (phys_addr and virt_addr fields).
> >>>>>>
> >>>>>
> >>>>> I'm fine with the concept of this patch, but I don't get why you need an API for
> >>>>> this and not just a callback to be used in the pci_epc_mem_{map/unmap} APIs.
> >>>>> Furthermore, I don't see an user of this API (in 3 series you've sent out so
> >>>>> far). Let me know if I failed to spot it.
> >>>>>
> >>>>> Also, the API name pci_epc_map_align() sounds like it does the mapping, but it
> >>>>> doesn't. So I'd not have it exposed as an API at all.
> >>>>
> >>>> OK. Fine with me. I will move this inside pci_epc_mem_map(). But note that
> >>>> without this function, pci_epc_mem_alloc_addr() and pci_epc_map_addr() are
> >>>> totally useless for EP controllers that have a mapping alignment requirement,
> >>>> which without the pci_epc_map_align() function, an endpoint function driver
> >>>> cannot discover *at all* currently. That does not fix the overall API of EPC...
> >>>>
> >>>
> >>> Not at all. EPF drivers still can use "epf_mhi->epc_features->align" to discover
> >>> the alignment requirement and calculate the offset on their own (please see
> >>> pci-epf-mhi). But I'm not in favor of that approach since the APIs need to do
> >>> that job and that's why I like your pci_epc_mem_map() API.
> >>
> >> That is *not* correct, at least in general. For two reasons:
> >> 1) epc_features->align defines alignment for BARs, that is, inbound windows
> >> memory. It is not supposed to be about the outbound windows for mapping PCI
> >> address space for doing mmio or DMA. Some controllers may have the same
> >> alignment constraint for both ib and ob, in which case things will work, but
> >> that is "just being lucky". I spent weeks with the RK3399 understanding that I
> >> was not lucky with that one :)
> >> 2) A static alignment constraint does not work for all controllers. C.f. my
> >> series fixing the RK3399 were I think I clearly explain that alignment of a
> >> mapping depends on the PCI address AND the size being mapped, as both determine
> >> the number of bits of address changing within the PCI address range to access.
> >> Using a fixed boundary alignment for the RK3399 simply does not work at all. An
> >> epf cannot know that simply looking at a fixed value...
> >>
> >> What you said may be true for the mhi epf, because it requires special hardware
> >> that has a simple fixed alignment constraint. ntb and vntb are also coded
> >> assuming such constraint. So If I try to run ntb or vntg on the RK3399 it will
> >> likely not work (actually it may, but out of sheer luck given that the addresses
> >> that will be mapped will likely be aligned to 1MB, that is, the memory window size).
> >>
> >> Developping the nvme epf driver where I was seeing completely random PCI
> >> addresses for command buffers, I could make things work only after developping
> >> the pci_epc_mem_map() with the controller operation telling the mapping
> >> (.get_mem_map()) for every address to map.
> >>
> > 
> > Fair enough...
> > 
> >>>
> >>>> By not having pci_epc_map_align(), pci_epc_mem_alloc_addr() and
> >>>> pci_epc_map_addr() remain broken, but the introduction of pci_epc_mem_map() does
> >>>> provide a working solution for the general case.
> >>>>
> >>>> So I think we will still need to do something about this bad state of the API later.
> >>>>
> >>>
> >>> We can always rework the APIs to incorporate the alignment requirement.
> >>
> >> See above. An API that advertise a simple alignment requirement will not work
> >> for all controllers... But anyway, given that we are not getting any problem
> >> report, people using the EP framework likely have setups that combine
> >> controllers and endpoint drivers playing well together. So I do not think there
> >> is any urgency about the API. I really do need this series for the nvme endpoint
> >> driver though, as a first step for the API improvement.
> >>
> > 
> > No, what I meant was that you can use the new alignment callback (that takes
> > care of the complex alignment restrictions) in the existing map API to properly
> > map the addresses for all controllers in the future.
> 
> The existing map API cannot alone use ->align_addr() to get the correct mapping.
> It is because the memory needed to handle a mapping may be larger than the PCI
> address range to map. In fact, it almost always is larger for any controller
> that has a constraint. As a result, the memory allocation side
> (pci_epc_alloc_addr()) must also be aware of the mapping constraint and
> resulting size of the memory to allocate... Hence pci_epc_mem_map() using both
> functions.
> 

Ah, I missed that. Thanks for clarifying!

- Mani

-- 
மணிவண்ணன் சதாசிவம்