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

[PATCH v3 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 assumes 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 tidy 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 64K PCI address
   alignment constraint.

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   |  33 ++
 .../pci/controller/dwc/pcie-designware-ep.c   |  15 +
 drivers/pci/endpoint/functions/pci-epf-test.c | 372 +++++++++---------
 drivers/pci/endpoint/pci-epc-core.c           | 213 +++++++---
 drivers/pci/endpoint/pci-epc-mem.c            |   9 +-
 include/linux/pci-epc.h                       |  41 ++
 6 files changed, 453 insertions(+), 230 deletions(-)

-- 
2.46.2

[PATCH v3 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 v3 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 window
size. Add a check to skip bitmap_find_free_region() calls for such case.
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 v3 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>
---
 drivers/pci/endpoint/pci-epc-core.c | 56 +++++++++++++++++++++++++++++
 include/linux/pci-epc.h             | 37 +++++++++++++++++++
 2 files changed, 93 insertions(+)

diff --git a/drivers/pci/endpoint/pci-epc-core.c b/drivers/pci/endpoint/pci-epc-core.c
index b854f1bab26f..48dd3c28ac4c 100644
--- a/drivers/pci/endpoint/pci-epc-core.c
+++ b/drivers/pci/endpoint/pci-epc-core.c
@@ -435,6 +435,62 @@ 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;
+
+	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 */
+		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 v3 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>
---
 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 48dd3c28ac4c..5f3b0a86d6fe 100644
--- a/drivers/pci/endpoint/pci-epc-core.c
+++ b/drivers/pci/endpoint/pci-epc-core.c
@@ -522,6 +522,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 v3 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>
---
 Documentation/PCI/endpoint/pci-endpoint.rst | 33 +++++++++++++++++++++
 1 file changed, 33 insertions(+)

diff --git a/Documentation/PCI/endpoint/pci-endpoint.rst b/Documentation/PCI/endpoint/pci-endpoint.rst
index 21507e3cc238..80061110441d 100644
--- a/Documentation/PCI/endpoint/pci-endpoint.rst
+++ b/Documentation/PCI/endpoint/pci-endpoint.rst
@@ -117,6 +117,39 @@ 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()
+
+   The PCI endpoint controller may impose constraints on the RC PCI addresses
+   that can be mapped. The function pci_epc_map_align() allows endpoint drivers
+   to discover and handle such constraint 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 offset into the
+   allocated memory to use for accessing the target RC PCI address.
+
+* 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 endpoitn 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 v3 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 v3 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 struct dw_pcie->region_align
(generally 64K).

Handle this 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>
---
 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 v3 0/7] Improve PCI memory mapping API

[Niklas Cassel]

On Fri, Oct 04, 2024 at 02:07:35PM +0900, 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 assumes is defined for inbound ATU entries

s/assumes//


> (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 tidy up the epc core code

Introduces a small helper which cleans up the 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 64K PCI address
>    alignment constraint.

Why mention that it implements it for rk3588 ? (And why mention 64K?)
Patch 7 is not specific to rk3588. Better to mention that you implement
it for all DWC PCIe EP controllers, based on the iATU hardware requirements,
read from hardware registers (which is stored in pci->region_align), since
that is what the commit does.

You should probably also mention that as a result of this series, follow
up patches can remove alignment entries in drivers/misc/pci_endpoint_test.c


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

I think that you should have mentioned that this fixes quite a serious bug in
V2, that was reported here:
https://lore.kernel.org/linux-pci/eb580d64-1110-479a-9a0b-c2f1eacd23e7@kernel.org/


> 
> 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   |  33 ++
>  .../pci/controller/dwc/pcie-designware-ep.c   |  15 +
>  drivers/pci/endpoint/functions/pci-epf-test.c | 372 +++++++++---------
>  drivers/pci/endpoint/pci-epc-core.c           | 213 +++++++---
>  drivers/pci/endpoint/pci-epc-mem.c            |   9 +-
>  include/linux/pci-epc.h                       |  41 ++
>  6 files changed, 453 insertions(+), 230 deletions(-)
> 
> -- 
> 2.46.2
> 

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

[Niklas Cassel]

On Fri, Oct 04, 2024 at 02:07:37PM +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 window
> size. Add a check to skip bitmap_find_free_region() calls for such case.
> 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);

Perhaps mention in the commit message why it is safe to move the mutex_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 v3 3/7] PCI: endpoint: Introduce pci_epc_map_align()

[Niklas Cassel]

On Fri, Oct 04, 2024 at 02:07:38PM +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).
> 
> 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>
> ---
>  drivers/pci/endpoint/pci-epc-core.c | 56 +++++++++++++++++++++++++++++
>  include/linux/pci-epc.h             | 37 +++++++++++++++++++
>  2 files changed, 93 insertions(+)
> 
> diff --git a/drivers/pci/endpoint/pci-epc-core.c b/drivers/pci/endpoint/pci-epc-core.c
> index b854f1bab26f..48dd3c28ac4c 100644
> --- a/drivers/pci/endpoint/pci-epc-core.c
> +++ b/drivers/pci/endpoint/pci-epc-core.c
> @@ -435,6 +435,62 @@ 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;
> +
> +	memset(map, 0, sizeof(*map));
> +	map->pci_addr = pci_addr;

Since the kdoc says that @map->pci_size might be smaller than the requested size,
I assume that you are only temporarily storing pci_size in map here, and that
map_align() might overwrite this value. Perhaps add a comment here that explains
that map_align() might/will overwrite this value.


> +	map->pci_size = pci_size;
> +
> +	if (!epc->ops->map_align) {
> +		/* Assume that the EP controller has no alignment constraint */
> +		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
> 

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

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

[Niklas Cassel]

On Fri, Oct 04, 2024 at 02:07:39PM +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>
> ---
>  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 48dd3c28ac4c..5f3b0a86d6fe 100644
> --- a/drivers/pci/endpoint/pci-epc-core.c
> +++ b/drivers/pci/endpoint/pci-epc-core.c
> @@ -522,6 +522,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
> 

Naming is one of the hardest problems in computer science :)

Perhaps:
s/pci_epc_mem_map()/pci_epc_mem_alloc_map()/
s/pci_epc_mem_unmap()/pci_epc_mem_free_unmap()/

is slightly more clear that this both allocates and maps.


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

Re: [PATCH v3 5/7] PCI: endpoint: Update documentation

[Niklas Cassel]

On Fri, Oct 04, 2024 at 02:07:40PM +0900, Damien Le Moal wrote:
> 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>
> ---
>  Documentation/PCI/endpoint/pci-endpoint.rst | 33 +++++++++++++++++++++
>  1 file changed, 33 insertions(+)
> 
> diff --git a/Documentation/PCI/endpoint/pci-endpoint.rst b/Documentation/PCI/endpoint/pci-endpoint.rst
> index 21507e3cc238..80061110441d 100644
> --- a/Documentation/PCI/endpoint/pci-endpoint.rst
> +++ b/Documentation/PCI/endpoint/pci-endpoint.rst
> @@ -117,6 +117,39 @@ 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()
> +
> +   The PCI endpoint controller may impose constraints on the RC PCI addresses
> +   that can be mapped. The function pci_epc_map_align() allows endpoint drivers
> +   to discover and handle such constraint by providing the size of the memory

Nit: s/constraint/constraints/


> +   that must be allocated with pci_epc_mem_alloc_addr() for successfully mapping
> +   any RC PCI address. This function will also indicate the offset into the
> +   allocated memory to use for accessing the target RC PCI address.

Should probably also mention that it might map less memory than requested.


> +
> +* 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 endpoitn function drivers.

s/endpoitn/endpoint/


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

I think that it would make more sense if you squashed the doc updates for
each function into the respective commit that add the new function(s).

Sure, you would still need a commit that adds the documentation for the
functions that was missing, but that is expected.


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

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

[Niklas Cassel]

On Fri, Oct 04, 2024 at 02:07:41PM +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>
> ---
>  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);

You are introducing an API where we need to do a while() loop around all
pci_epc_mem_map calls, because the API allows you to return a
map->pci_size that is smaller than the requested size.

What I don't understand is that the only driver that implements this API
is DWC PCIe EP, and that function currently never returns a map->pci_size
smaller than requested, so from just looking at this series by itself,
this API seems way too complicated for the only single implementer.

I think that you need to also include the patch that implements map_align()
for rk3399 in this series (without any other rk3399 patches), such that the
API actually makes sense.

Because without that patch, the API seems to be way more complicated than
it needs to be.



With this new API, it is a bit unfortunate that all EPF drivers will now
need do a while() loop around their map() calls, just because of rk3399.

The current map functions already return an -EINVAL if you try to map
something that is larger than the window size. Isn't the problem for rk3399
that the window size is just 1 MB. Can't you simply return -EINVAL if the
allocation (including the extra bytes needed for alignment) exceeds 1 MB?


By default, pcitest.sh uses these sizes for read write transfers:

echo "Read Tests"
echo

pcitest -r -s 1
pcitest -r -s 1024
pcitest -r -s 1025
pcitest -r -s 1024000
pcitest -r -s 1024001
echo

echo "Write Tests"
echo

pcitest -w -s 1
pcitest -w -s 1024
pcitest -w -s 1025
pcitest -w -s 1024000
pcitest -w -s 1024001

All that should be way smaller than 1 MB, including alignment.
Specifying something larger will (for many platforms) fail.


I understand that certain EPF drivers will need to map buffers larger
than that. But perhaps we can keep pci-epf-test.c simple, and introduce
the more complex logic in EPF drivers that actually need it?

E.g. introduce a PCI EP API, e.g. pci_epc_max_mapping_for_address(),
that given a PCI address, returns the largest buffer the EPC can map.

Simple drivers like pci-epf-test can then choose to only support the simple
case (no looping case), and return error (e.g. -EINVAL) for buffers larger
than pci_epc_max_mapping_for_address().

More complicated EPF drivers can then choose if they want to support only
the simple case (no looping case), but can also choose to support buffers
larger than pci_epc_max_mapping_for_address(), using looping (I assume that
each loop iteration will be able to map (at least) the same amount as the
first loop iteration).
The looping case and the non-looping case can even be implemented in their
separate function.

I personally, think that there is value in keeping pci-epf-test.c as simple
as possible, so that people can get familiar with the PCI endpoint framework.
More complicated logic can be found in other EPF drivers, e.g. pci-epf-ntb.c
and pci-epf-vntb.c.

Thoughts?


If we implement pci_epc_max_mapping_for_address(), and then the looping and
non-looping case in separate functions, perhaps we could even implement it
in pci-epf-test.c, as having more functions will help with the readability,
but with the patch as it looks right, I do feel like the readability gets
quite worse.


> +	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 v3 7/7] PCI: dwc: endpoint: Define the .map_align() controller operation

[Niklas Cassel]

On Fri, Oct 04, 2024 at 02:07:42PM +0900, Damien Le Moal wrote:
> 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 struct dw_pcie->region_align
> (generally 64K).

I think that you should just remove the "generally 64K". This is totally
dependent on the hardware configuration set when synthesizing the DWC PCIe
controller.

See e.g. drivers/misc/pci_endpoint_test.c:
.alignment = SZ_4K,
.alignment = SZ_64K,
.alignment = 256,

In fact, the most common one, from looking at what the different PCI device
and vendor IDs, seems to be 4k.

Perhaps simply mention that the pci->region_align contains the value that was
read/determined from iATU hardware registers during detection time of the iATU
(done by dw_pcie_iatu_detect()), so this code is actually generic for all DWC
PCIe controllers, and should thus work regardless of the hardware configuration
used when synthesizing the DWC PCIe controller.


> 
> Handle this 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>
> ---
>  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
> 

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

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

[Niklas Cassel]

On Fri, Oct 04, 2024 at 02:07:35PM +0900, 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 assumes 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 tidy 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 64K PCI address
>    alignment constraint.
> 
> 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


I think the cover letter is missing some text on how this series has been
tested.

In V2 I suggested adding a new option to pcitest.c, so that it doesn't
ensure that buffers are aligned. pci_test will currently use a 4k alignment
by default, and for some PCI device IDs and vendor IDs, it will ensure that
the buffers are aligned to something else. (E.g. for the PCI device ID used
by rk3588, buffers will be aligned to 64K.)

By adding an --no-alignment option to pci_test, we can ensure that this new
API is actually working.

Did you perhaps ifdef out all the alignment from pci_endpoint_test.c when
testing?

I think that having a --no-alignment option included as part of the series
would give us higher confidence that the API is working as intended.

(pci_test would still align buffers by default, and the long term plan is
to remove these eventually, but it would be nice to already have an option
to disable it.)


Kind regards,
Niklas

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

[Damien Le Moal]

On 10/4/24 22:13, Niklas Cassel wrote:
> On Fri, Oct 04, 2024 at 02:07:35PM +0900, 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 assumes 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 tidy 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 64K PCI address
>>    alignment constraint.
>>
>> 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
> 
> 
> I think the cover letter is missing some text on how this series has been
> tested.
> 
> In V2 I suggested adding a new option to pcitest.c, so that it doesn't
> ensure that buffers are aligned. pci_test will currently use a 4k alignment
> by default, and for some PCI device IDs and vendor IDs, it will ensure that
> the buffers are aligned to something else. (E.g. for the PCI device ID used
> by rk3588, buffers will be aligned to 64K.)
> 
> By adding an --no-alignment option to pci_test, we can ensure that this new
> API is actually working.
> 
> Did you perhaps ifdef out all the alignment from pci_endpoint_test.c when
> testing?

Yes I did. And I also extensively tested using the nvme epf function driver
(coming soon !) which has very random PCI addresses for data buffers (e.g.
BIOSes and GRUB are happy using on-stack totally unaligned buffers...).

> I think that having a --no-alignment option included as part of the series
> would give us higher confidence that the API is working as intended.

Sure, we can add that as a followup patch. I really would like that series to
not be hold up by this though as more stuff depend on it (the nvme epf function
driver is one).

> 
> (pci_test would still align buffers by default, and the long term plan is
> to remove these eventually, but it would be nice to already have an option
> to disable it.)
> 
> 
> Kind regards,
> Niklas


-- 
Damien Le Moal
Western Digital Research

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

[Damien Le Moal]

On 10/4/24 20:47, Niklas Cassel wrote:
> Naming is one of the hardest problems in computer science :)

Yep.

> Perhaps:
> s/pci_epc_mem_map()/pci_epc_mem_alloc_map()/
> s/pci_epc_mem_unmap()/pci_epc_mem_free_unmap()/
> 
> is slightly more clear that this both allocates and maps.

Sure, but I consider the allocation an implementation detail of the function.
And I really prefer the shorter function names :)

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

Thanks.

-- 
Damien Le Moal
Western Digital Research

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

[Damien Le Moal]

On 10/4/24 21:11, Niklas Cassel wrote:
> You are introducing an API where we need to do a while() loop around all
> pci_epc_mem_map calls, because the API allows you to return a
> map->pci_size that is smaller than the requested size.

Yes.

> What I don't understand is that the only driver that implements this API
> is DWC PCIe EP, and that function currently never returns a map->pci_size
> smaller than requested, so from just looking at this series by itself,
> this API seems way too complicated for the only single implementer.
> 
> I think that you need to also include the patch that implements map_align()
> for rk3399 in this series (without any other rk3399 patches), such that the
> API actually makes sense.

This is coming in a followup series. Note that the Cadence controller also look
suspiciously similar to the rk3399, so it may need the same treatment. As for
the need for the loop, let's not kid ourselves here: it was already needed
because some controllers have limited window sizes and do not allow allocating
large chunks of memory to map. That was never handled...

I consider this loop the least of our problems with the epc API. This series
fixes one issue (transparent handling of alignment), but does not address the
fact that there is absolutely NOTHING that allows an epf driver to manage the
number of mappings that can be done simultaneously. That is, if the epf driver
does not in purpose serialize things to minimize the number of mappings used, it
will get errors.

And coming from block layer where large request splitting is normal, I really do
not have any problem with handling large things in smaller chunks with a loop :)

> Because without that patch, the API seems to be way more complicated than
> it needs to be.
> 
> 
> 
> With this new API, it is a bit unfortunate that all EPF drivers will now
> need do a while() loop around their map() calls, just because of rk3399.

As I said, the Cadence controller likely needs the same treatment as well. But I
do not have that hardware and specs to check that.

> The current map functions already return an -EINVAL if you try to map
> something that is larger than the window size. Isn't the problem for rk3399
> that the window size is just 1 MB. Can't you simply return -EINVAL if the
> allocation (including the extra bytes needed for alignment) exceeds 1 MB?

Sure. But then the epf will still need to loop using smaller mappings to get
work done. So at least this API simplifies that task.

> By default, pcitest.sh uses these sizes for read write transfers:
> 
> echo "Read Tests"
> echo
> 
> pcitest -r -s 1
> pcitest -r -s 1024
> pcitest -r -s 1025
> pcitest -r -s 1024000
> pcitest -r -s 1024001
> echo
> 
> echo "Write Tests"
> echo
> 
> pcitest -w -s 1
> pcitest -w -s 1024
> pcitest -w -s 1025
> pcitest -w -s 1024000
> pcitest -w -s 1024001
> 
> All that should be way smaller than 1 MB, including alignment.
> Specifying something larger will (for many platforms) fail.

To improve the test, we can add larger sizes. However, we should do that only
when all controllers have been audited and eventually modified to have a
.map_align() (or not if not needed) operation. Given the alignment use in the
test function driver, I suspect that all ep controllers need a .map_align()
operation.

> I understand that certain EPF drivers will need to map buffers larger
> than that. But perhaps we can keep pci-epf-test.c simple, and introduce
> the more complex logic in EPF drivers that actually need it?

But then it would not be much of test driver... We need to exercise corner cases
as well.

> E.g. introduce a PCI EP API, e.g. pci_epc_max_mapping_for_address(),
> that given a PCI address, returns the largest buffer the EPC can map.

That is what pci_epc_map_align() tells you... You are only changing the name.

> Simple drivers like pci-epf-test can then choose to only support the simple
> case (no looping case), and return error (e.g. -EINVAL) for buffers larger
> than pci_epc_max_mapping_for_address().

I really do not see any issue with that loop...

> More complicated EPF drivers can then choose if they want to support only
> the simple case (no looping case), but can also choose to support buffers
> larger than pci_epc_max_mapping_for_address(), using looping (I assume that
> each loop iteration will be able to map (at least) the same amount as the
> first loop iteration).

How can they "choose" if the controller being used gives you a max mapping size
that completely depend on the PCI address used by the RC ? Unless the protocol
used by that function driver imposes constraint on the host on buffer alignment,
the epf cannot choose anything here. E.g. NVMe epf needs to be able to handle
anything.

> The looping case and the non-looping case can even be implemented in their
> separate function.
> 
> I personally, think that there is value in keeping pci-epf-test.c as simple
> as possible, so that people can get familiar with the PCI endpoint framework.
> More complicated logic can be found in other EPF drivers, e.g. pci-epf-ntb.c
> and pci-epf-vntb.c.
> 
> Thoughts?

The loop is really simple. It is clear that it iterates over the buffer to
process until it is fully accessed. I really do not see the problem. EPC/EPF
stuff in itself is nor simle at all already. The map_align API and the potential
for needing a loop to process PCI transfers does not make it more complicated
than it already is in my opinion.

> If we implement pci_epc_max_mapping_for_address(), and then the looping and
> non-looping case in separate functions, perhaps we could even implement it
> in pci-epf-test.c, as having more functions will help with the readability,
> but with the patch as it looks right, I do feel like the readability gets
> quite worse.

If you really insist, then I will drop this patch, but then the test driver will
not be of much a test at all... The nice thing about this patch is that it
provides a *simple* example of how to handle PCI transfers using mmio or DMA,
and in chunks based on the PCI address alignment. That is something that all epf
drivers will do and that devleopers of epf drivers will be happy to see.

-- 
Damien Le Moal
Western Digital Research

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

[Niklas Cassel]

On Fri, Oct 04, 2024 at 10:25:54PM +0900, Damien Le Moal wrote:
> On 10/4/24 22:13, Niklas Cassel wrote:
> > On Fri, Oct 04, 2024 at 02:07:35PM +0900, Damien Le Moal wrote:

(snip)

> > I think the cover letter is missing some text on how this series has been
> > tested.
> > 
> > In V2 I suggested adding a new option to pcitest.c, so that it doesn't
> > ensure that buffers are aligned. pci_test will currently use a 4k alignment
> > by default, and for some PCI device IDs and vendor IDs, it will ensure that
> > the buffers are aligned to something else. (E.g. for the PCI device ID used
> > by rk3588, buffers will be aligned to 64K.)
> > 
> > By adding an --no-alignment option to pci_test, we can ensure that this new
> > API is actually working.
> > 
> > Did you perhaps ifdef out all the alignment from pci_endpoint_test.c when
> > testing?
> 
> Yes I did. And I also extensively tested using the nvme epf function driver
> (coming soon !) which has very random PCI addresses for data buffers (e.g.
> BIOSes and GRUB are happy using on-stack totally unaligned buffers...).

I know that you did test using a nvme EPF :)

But for anyone reading the cover letter, it wasn't clear how this series
was tested, so it would have been nice if the information which you
provided above would have been part of the cover letter.


Kind regards,
Niklas

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

[Niklas Cassel]

On Fri, Oct 04, 2024 at 10:47:01PM +0900, Damien Le Moal wrote:
> On 10/4/24 21:11, Niklas Cassel wrote:

(snip)

> > I think that you need to also include the patch that implements map_align()
> > for rk3399 in this series (without any other rk3399 patches), such that the
> > API actually makes sense.
> 
> This is coming in a followup series. Note that the Cadence controller also look
> suspiciously similar to the rk3399, so it may need the same treatment.

I strongly think that you should continue to include patch:
"PCI: rockchip-ep: Implement the map_align endpoint controller operation"
(that was part of your V2) in this series.

(Just do not include any of the other random fixes for rockchip-ep that
were part of your V2.)

Because without an implementer of the API that can actually return a size
smaller than requested, the current API makes no sense.

We do send out a series that provides a complex API if there is no implementer
(in that same series) which require the complexity. The single implementer in
this series (DWC PCIe EP), does not require the complexity of the API.

(The reason for this is that it is possible (for whatever reason) that the
intended follow up series which adds an implementer which actually requires
this complex API, may never materialize/land, and in that case we are left
with technical debt/an overcomplicated API for no reason.)


> As for the need for the loop, let's not kid ourselves here: it was already
> needed because some controllers have limited window sizes and do not allow
> allocating large chunks of memory to map. That was never handled...

I agree.

For controllers which have a window size that is very small, the pci-epf-test
driver will currently not handle buffer sizes larger than the window size.
Adding a loop would solve that limitation.

But this information is currently completely missing for the commit log.

May I suggest that you:
1) Include a preparatory patch in this series, which adds the loop using the
   existing map functions. With the proper motivation in the commit log.
2) Add this patch which converts the pci-epf-test driver to use the new map
   API. The number of changed lines in this patch will thus be much smaller
   than it currently is, and it will be easier to review.


> > I understand that certain EPF drivers will need to map buffers larger
> > than that. But perhaps we can keep pci-epf-test.c simple, and introduce
> > the more complex logic in EPF drivers that actually need it?
> 
> But then it would not be much of test driver... We need to exercise corner cases
> as well.

Yes, your argument does make a lot of sense.


> > More complicated EPF drivers can then choose if they want to support only
> > the simple case (no looping case), but can also choose to support buffers
> > larger than pci_epc_max_mapping_for_address(), using looping (I assume that
> > each loop iteration will be able to map (at least) the same amount as the
> > first loop iteration).
> 
> How can they "choose" if the controller being used gives you a max mapping size
> that completely depend on the PCI address used by the RC ? Unless the protocol
> used by that function driver imposes constraint on the host on buffer alignment,
> the epf cannot choose anything here. E.g. NVMe epf needs to be able to handle
> anything.

What I meant was that PCI EPF drivers could simply (continue) to return error
if the buffer size was larger than the window size. But you have successfully
changed my mind, let's just add the loop.

(As my previous suggestion of letting an EPF driver call
pci_epc_max_mapping_for_address()/pci_epc_map_align(), and then based on if
the returned size being either smaller or larger than the window size, call
either a function that does no looping, or a function that does looping, might
actually be more error prone and more confusing compared to just having a
single transfer function which includes a loop.)


Kind regards,
Niklas

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

[Damien Le Moal]

On 10/6/24 20:48, Niklas Cassel wrote:
> On Fri, Oct 04, 2024 at 10:47:01PM +0900, Damien Le Moal wrote:
>> On 10/4/24 21:11, Niklas Cassel wrote:
> 
> (snip)
> 
>>> I think that you need to also include the patch that implements map_align()
>>> for rk3399 in this series (without any other rk3399 patches), such that the
>>> API actually makes sense.
>>
>> This is coming in a followup series. Note that the Cadence controller also look
>> suspiciously similar to the rk3399, so it may need the same treatment.
> 
> I strongly think that you should continue to include patch:
> "PCI: rockchip-ep: Implement the map_align endpoint controller operation"
> (that was part of your V2) in this series.

I would rather not because this patch needs a prep patch that fixes the
currently broken rockchip_pcie_prog_ep_ob_atu() function. So I will post this
patch as part of a series for the rk3399 today.



-- 
Damien Le Moal
Western Digital Research

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

[kernel test robot]

Hi Damien,

kernel test robot noticed the following build warnings:

[auto build test WARNING on pci/next]
[also build test WARNING on pci/for-linus mani-mhi/mhi-next linus/master v6.12-rc2 next-20241004]
[If your patch is applied to the wrong git tree, kindly drop us a note.
And when submitting patch, we suggest to use '--base' as documented in
https://git-scm.com/docs/git-format-patch#_base_tree_information]

url:    https://github.com/intel-lab-lkp/linux/commits/Damien-Le-Moal/PCI-endpoint-Introduce-pci_epc_function_is_valid/20241004-130947
base:   https://git.kernel.org/pub/scm/linux/kernel/git/pci/pci.git next
patch link:    https://lore.kernel.org/r/20241004050742.140664-5-dlemoal%40kernel.org
patch subject: [PATCH v3 4/7] PCI: endpoint: Introduce pci_epc_mem_map()/unmap()
config: x86_64-randconfig-122-20241007 (https://download.01.org/0day-ci/archive/20241007/202410070929.jEKAJxjG-lkp@intel.com/config)
compiler: clang version 18.1.8 (https://github.com/llvm/llvm-project 3b5b5c1ec4a3095ab096dd780e84d7ab81f3d7ff)
reproduce (this is a W=1 build): (https://download.01.org/0day-ci/archive/20241007/202410070929.jEKAJxjG-lkp@intel.com/reproduce)

If you fix the issue in a separate patch/commit (i.e. not just a new version of
the same patch/commit), kindly add following tags
| Reported-by: kernel test robot <lkp@intel.com>
| Closes: https://lore.kernel.org/oe-kbuild-all/202410070929.jEKAJxjG-lkp@intel.com/

sparse warnings: (new ones prefixed by >>)
>> drivers/pci/endpoint/pci-epc-core.c:569:34: sparse: sparse: Using plain integer as NULL pointer
   drivers/pci/endpoint/pci-epc-core.c: note: in included file (through include/linux/smp.h, include/linux/lockdep.h, include/linux/spinlock.h, ...):
   include/linux/list.h:83:21: sparse: sparse: self-comparison always evaluates to true

vim +569 drivers/pci/endpoint/pci-epc-core.c

   524	
   525	/**
   526	 * pci_epc_mem_map() - allocate and map a PCI address to a CPU address
   527	 * @epc: the EPC device on which the CPU address is to be allocated and mapped
   528	 * @func_no: the physical endpoint function number in the EPC device
   529	 * @vfunc_no: the virtual endpoint function number in the physical function
   530	 * @pci_addr: PCI address to which the CPU address should be mapped
   531	 * @pci_size: the number of bytes to map starting from @pci_addr
   532	 * @map: where to return the mapping information
   533	 *
   534	 * Allocate a controller memory address region and map it to a RC PCI address
   535	 * region, taking into account the controller physical address mapping
   536	 * constraints using pci_epc_map_align().
   537	 * The effective size of the PCI address range mapped from @pci_addr is
   538	 * indicated by @map->pci_size. This size may be less than the requested
   539	 * @pci_size. The local virtual CPU address for the mapping is indicated by
   540	 * @map->virt_addr (@map->phys_addr indicates the physical address).
   541	 * The size and CPU address of the controller memory allocated and mapped are
   542	 * respectively indicated by @map->map_size and @map->virt_base (and
   543	 * @map->phys_base).
   544	 *
   545	 * Returns 0 on success and a negative error code in case of error.
   546	 */
   547	int pci_epc_mem_map(struct pci_epc *epc, u8 func_no, u8 vfunc_no,
   548			    u64 pci_addr, size_t pci_size, struct pci_epc_map *map)
   549	{
   550		int ret;
   551	
   552		ret = pci_epc_map_align(epc, func_no, vfunc_no, pci_addr, pci_size, map);
   553		if (ret)
   554			return ret;
   555	
   556		map->virt_base = pci_epc_mem_alloc_addr(epc, &map->phys_base,
   557							map->map_size);
   558		if (!map->virt_base)
   559			return -ENOMEM;
   560	
   561		map->phys_addr = map->phys_base + map->map_ofst;
   562		map->virt_addr = map->virt_base + map->map_ofst;
   563	
   564		ret = pci_epc_map_addr(epc, func_no, vfunc_no, map->phys_base,
   565				       map->map_pci_addr, map->map_size);
   566		if (ret) {
   567			pci_epc_mem_free_addr(epc, map->phys_base, map->virt_base,
   568					      map->map_size);
 > 569			map->virt_base = 0;
   570			return ret;
   571		}
   572	
   573		return 0;
   574	}
   575	EXPORT_SYMBOL_GPL(pci_epc_mem_map);
   576	

-- 
0-DAY CI Kernel Test Service
https://github.com/intel/lkp-tests/wiki