linux-kernel.vger.kernel.org archive mirror
 help / color / mirror / Atom feed
* [PATCH V3] misc: pci_endpoint_test: simplify endpoint test read and write operations
@ 2022-01-20 10:11 Li Chen
  2022-01-20 11:57 ` Greg Kroah-Hartman
  2022-01-20 12:47 ` [PATCH V4] " Li Chen
  0 siblings, 2 replies; 14+ messages in thread
From: Li Chen @ 2022-01-20 10:11 UTC (permalink / raw)
  To: Kishon Vijay Abraham I, Lorenzo Pieralisi,
	"Krzysztof Wilczyński",
	Arnd Bergmann, Greg Kroah-Hartman, Bjorn Helgaas, linux-pci,
	linux-kernel

Introduce pci_endpoint_epf_transfer_data to simplify
read and write operations.

Signed-off-by: Li Chen <lchen@ambarella.com>
---
Changes in V2:
fix WARNING: line length of 108 exceeds 100 columns
#128: FILE: drivers/misc/pci_endpoint_test.c:243:
Changes in V3:
This patch context doesn't change but resend with my Zoho mail account in that previous 
company mail will contain un-removeable proprietary messages.

 drivers/misc/pci_endpoint_test.c | 289 ++++++++++++-------------------
 1 file changed, 109 insertions(+), 180 deletions(-)

diff --git a/drivers/misc/pci_endpoint_test.c b/drivers/misc/pci_endpoint_test.c
index 2ed7e3aaff3a8..b6b0b19b251b3 100644
--- a/drivers/misc/pci_endpoint_test.c
+++ b/drivers/misc/pci_endpoint_test.c
@@ -103,6 +103,11 @@ enum pci_barno {
 	BAR_5,
 };
 
+enum operation {
+	EPF_READ,
+	EPF_WRITE,
+};
+
 struct pci_endpoint_test {
 	struct pci_dev	*pdev;
 	void __iomem	*base;
@@ -142,6 +147,108 @@ static inline u32 pci_endpoint_test_bar_readl(struct pci_endpoint_test *test,
 {
 	return readl(test->bar[bar] + offset);
 }
+static bool pci_endpoint_test_transfer_data(struct pci_endpoint_test *test,
+				unsigned long arg, const enum operation operation)
+{
+	struct pci_endpoint_test_xfer_param param;
+	bool ret = false;
+	u32 flags = 0;
+	bool use_dma;
+	void *addr;
+	dma_addr_t phys_addr;
+	struct pci_dev *pdev = test->pdev;
+	struct device *dev = &pdev->dev;
+	void *orig_addr;
+	dma_addr_t orig_phys_addr;
+	size_t offset;
+	size_t alignment = test->alignment;
+	int irq_type = test->irq_type;
+	size_t size;
+	int err;
+
+	err = copy_from_user(&param, (void __user *)arg, sizeof(param));
+	if (err != 0) {
+		dev_err(dev, "Failed to get transfer param\n");
+		return false;
+	}
+
+	size = param.size;
+	if (size > SIZE_MAX - alignment)
+		goto err;
+
+	use_dma = !!(param.flags & PCITEST_FLAGS_USE_DMA);
+	if (use_dma)
+		flags |= FLAG_USE_DMA;
+
+	if (irq_type < IRQ_TYPE_LEGACY || irq_type > IRQ_TYPE_MSIX) {
+		dev_err(dev, "Invalid IRQ type option\n");
+		goto err;
+	}
+
+	orig_addr = kzalloc(size + alignment, GFP_KERNEL);
+	if (!orig_addr)
+		goto err;
+
+	get_random_bytes(orig_addr, size + alignment);
+
+	orig_phys_addr = dma_map_single(dev, orig_addr, size + alignment,
+					operation == EPF_WRITE ? DMA_TO_DEVICE : DMA_FROM_DEVICE);
+	if (dma_mapping_error(dev, orig_phys_addr)) {
+		dev_err(dev, "failed to map source buffer address\n");
+		goto err_phys_addr;
+	}
+
+	if (alignment && !IS_ALIGNED(orig_phys_addr, alignment)) {
+		phys_addr = PTR_ALIGN(orig_phys_addr, alignment);
+		offset = phys_addr - orig_phys_addr;
+		addr = orig_addr + offset;
+	} else {
+		phys_addr = orig_phys_addr;
+		addr = orig_addr;
+	}
+
+	if (operation == EPF_WRITE) {
+
+		pci_endpoint_test_writel(test, PCI_ENDPOINT_TEST_CHECKSUM,
+				 crc32_le(~0, addr, size));
+
+		pci_endpoint_test_writel(test, PCI_ENDPOINT_TEST_LOWER_SRC_ADDR,
+								lower_32_bits(phys_addr));
+		pci_endpoint_test_writel(test, PCI_ENDPOINT_TEST_UPPER_SRC_ADDR,
+								upper_32_bits(phys_addr));
+	} else {
+		pci_endpoint_test_writel(test, PCI_ENDPOINT_TEST_LOWER_DST_ADDR,
+								lower_32_bits(phys_addr));
+		pci_endpoint_test_writel(test, PCI_ENDPOINT_TEST_UPPER_DST_ADDR,
+								upper_32_bits(phys_addr));
+	}
+
+	pci_endpoint_test_writel(test, PCI_ENDPOINT_TEST_SIZE, size);
+	pci_endpoint_test_writel(test, PCI_ENDPOINT_TEST_FLAGS, flags);
+	pci_endpoint_test_writel(test, PCI_ENDPOINT_TEST_IRQ_TYPE, irq_type);
+	pci_endpoint_test_writel(test, PCI_ENDPOINT_TEST_IRQ_NUMBER, 1);
+
+	// if we ask rc to write to ep, then ep should do read operation, and vice versa.
+	pci_endpoint_test_writel(test, PCI_ENDPOINT_TEST_COMMAND,
+				 operation == EPF_WRITE ? COMMAND_READ : COMMAND_WRITE);
+
+	wait_for_completion(&test->irq_raised);
+
+	dma_unmap_single(dev, orig_phys_addr, size + alignment,
+					 operation == EPF_WRITE ? DMA_TO_DEVICE : DMA_FROM_DEVICE);
+
+	if (operation == WRITE)
+		ret = pci_endpoint_test_readl(test, PCI_ENDPOINT_TEST_STATUS) & STATUS_READ_SUCCESS;
+	else
+		ret = crc32_le(~0, addr, size) ==
+			pci_endpoint_test_readl(test, PCI_ENDPOINT_TEST_CHECKSUM);
+
+err_phys_addr:
+	kfree(orig_addr);
+
+err:
+	return ret;
+}
 
 static inline void pci_endpoint_test_bar_writel(struct pci_endpoint_test *test,
 						int bar, u32 offset, u32 value)
@@ -473,191 +580,13 @@ static bool pci_endpoint_test_copy(struct pci_endpoint_test *test,
 static bool pci_endpoint_test_write(struct pci_endpoint_test *test,
 				    unsigned long arg)
 {
-	struct pci_endpoint_test_xfer_param param;
-	bool ret = false;
-	u32 flags = 0;
-	bool use_dma;
-	u32 reg;
-	void *addr;
-	dma_addr_t phys_addr;
-	struct pci_dev *pdev = test->pdev;
-	struct device *dev = &pdev->dev;
-	void *orig_addr;
-	dma_addr_t orig_phys_addr;
-	size_t offset;
-	size_t alignment = test->alignment;
-	int irq_type = test->irq_type;
-	size_t size;
-	u32 crc32;
-	int err;
-
-	err = copy_from_user(&param, (void __user *)arg, sizeof(param));
-	if (err != 0) {
-		dev_err(dev, "Failed to get transfer param\n");
-		return false;
-	}
-
-	size = param.size;
-	if (size > SIZE_MAX - alignment)
-		goto err;
-
-	use_dma = !!(param.flags & PCITEST_FLAGS_USE_DMA);
-	if (use_dma)
-		flags |= FLAG_USE_DMA;
-
-	if (irq_type < IRQ_TYPE_LEGACY || irq_type > IRQ_TYPE_MSIX) {
-		dev_err(dev, "Invalid IRQ type option\n");
-		goto err;
-	}
-
-	orig_addr = kzalloc(size + alignment, GFP_KERNEL);
-	if (!orig_addr) {
-		dev_err(dev, "Failed to allocate address\n");
-		ret = false;
-		goto err;
-	}
-
-	get_random_bytes(orig_addr, size + alignment);
-
-	orig_phys_addr = dma_map_single(dev, orig_addr, size + alignment,
-					DMA_TO_DEVICE);
-	if (dma_mapping_error(dev, orig_phys_addr)) {
-		dev_err(dev, "failed to map source buffer address\n");
-		ret = false;
-		goto err_phys_addr;
-	}
-
-	if (alignment && !IS_ALIGNED(orig_phys_addr, alignment)) {
-		phys_addr =  PTR_ALIGN(orig_phys_addr, alignment);
-		offset = phys_addr - orig_phys_addr;
-		addr = orig_addr + offset;
-	} else {
-		phys_addr = orig_phys_addr;
-		addr = orig_addr;
-	}
-
-	crc32 = crc32_le(~0, addr, size);
-	pci_endpoint_test_writel(test, PCI_ENDPOINT_TEST_CHECKSUM,
-				 crc32);
-
-	pci_endpoint_test_writel(test, PCI_ENDPOINT_TEST_LOWER_SRC_ADDR,
-				 lower_32_bits(phys_addr));
-	pci_endpoint_test_writel(test, PCI_ENDPOINT_TEST_UPPER_SRC_ADDR,
-				 upper_32_bits(phys_addr));
-
-	pci_endpoint_test_writel(test, PCI_ENDPOINT_TEST_SIZE, size);
-
-	pci_endpoint_test_writel(test, PCI_ENDPOINT_TEST_FLAGS, flags);
-	pci_endpoint_test_writel(test, PCI_ENDPOINT_TEST_IRQ_TYPE, irq_type);
-	pci_endpoint_test_writel(test, PCI_ENDPOINT_TEST_IRQ_NUMBER, 1);
-	pci_endpoint_test_writel(test, PCI_ENDPOINT_TEST_COMMAND,
-				 COMMAND_READ);
-
-	wait_for_completion(&test->irq_raised);
-
-	reg = pci_endpoint_test_readl(test, PCI_ENDPOINT_TEST_STATUS);
-	if (reg & STATUS_READ_SUCCESS)
-		ret = true;
-
-	dma_unmap_single(dev, orig_phys_addr, size + alignment,
-			 DMA_TO_DEVICE);
-
-err_phys_addr:
-	kfree(orig_addr);
-
-err:
-	return ret;
+	return pci_endpoint_test_transfer_data(test, arg, EPF_WRITE);
 }
 
 static bool pci_endpoint_test_read(struct pci_endpoint_test *test,
 				   unsigned long arg)
 {
-	struct pci_endpoint_test_xfer_param param;
-	bool ret = false;
-	u32 flags = 0;
-	bool use_dma;
-	size_t size;
-	void *addr;
-	dma_addr_t phys_addr;
-	struct pci_dev *pdev = test->pdev;
-	struct device *dev = &pdev->dev;
-	void *orig_addr;
-	dma_addr_t orig_phys_addr;
-	size_t offset;
-	size_t alignment = test->alignment;
-	int irq_type = test->irq_type;
-	u32 crc32;
-	int err;
-
-	err = copy_from_user(&param, (void __user *)arg, sizeof(param));
-	if (err) {
-		dev_err(dev, "Failed to get transfer param\n");
-		return false;
-	}
-
-	size = param.size;
-	if (size > SIZE_MAX - alignment)
-		goto err;
-
-	use_dma = !!(param.flags & PCITEST_FLAGS_USE_DMA);
-	if (use_dma)
-		flags |= FLAG_USE_DMA;
-
-	if (irq_type < IRQ_TYPE_LEGACY || irq_type > IRQ_TYPE_MSIX) {
-		dev_err(dev, "Invalid IRQ type option\n");
-		goto err;
-	}
-
-	orig_addr = kzalloc(size + alignment, GFP_KERNEL);
-	if (!orig_addr) {
-		dev_err(dev, "Failed to allocate destination address\n");
-		ret = false;
-		goto err;
-	}
-
-	orig_phys_addr = dma_map_single(dev, orig_addr, size + alignment,
-					DMA_FROM_DEVICE);
-	if (dma_mapping_error(dev, orig_phys_addr)) {
-		dev_err(dev, "failed to map source buffer address\n");
-		ret = false;
-		goto err_phys_addr;
-	}
-
-	if (alignment && !IS_ALIGNED(orig_phys_addr, alignment)) {
-		phys_addr = PTR_ALIGN(orig_phys_addr, alignment);
-		offset = phys_addr - orig_phys_addr;
-		addr = orig_addr + offset;
-	} else {
-		phys_addr = orig_phys_addr;
-		addr = orig_addr;
-	}
-
-	pci_endpoint_test_writel(test, PCI_ENDPOINT_TEST_LOWER_DST_ADDR,
-				 lower_32_bits(phys_addr));
-	pci_endpoint_test_writel(test, PCI_ENDPOINT_TEST_UPPER_DST_ADDR,
-				 upper_32_bits(phys_addr));
-
-	pci_endpoint_test_writel(test, PCI_ENDPOINT_TEST_SIZE, size);
-
-	pci_endpoint_test_writel(test, PCI_ENDPOINT_TEST_FLAGS, flags);
-	pci_endpoint_test_writel(test, PCI_ENDPOINT_TEST_IRQ_TYPE, irq_type);
-	pci_endpoint_test_writel(test, PCI_ENDPOINT_TEST_IRQ_NUMBER, 1);
-	pci_endpoint_test_writel(test, PCI_ENDPOINT_TEST_COMMAND,
-				 COMMAND_WRITE);
-
-	wait_for_completion(&test->irq_raised);
-
-	dma_unmap_single(dev, orig_phys_addr, size + alignment,
-			 DMA_FROM_DEVICE);
-
-	crc32 = crc32_le(~0, addr, size);
-	if (crc32 == pci_endpoint_test_readl(test, PCI_ENDPOINT_TEST_CHECKSUM))
-		ret = true;
-
-err_phys_addr:
-	kfree(orig_addr);
-err:
-	return ret;
+	return pci_endpoint_test_transfer_data(test, arg, EPF_READ);
 }
 
 static bool pci_endpoint_test_clear_irq(struct pci_endpoint_test *test)
-- 
2.34.1


^ permalink raw reply related	[flat|nested] 14+ messages in thread

* Re: [PATCH V3] misc: pci_endpoint_test: simplify endpoint test read and write operations
  2022-01-20 10:11 [PATCH V3] misc: pci_endpoint_test: simplify endpoint test read and write operations Li Chen
@ 2022-01-20 11:57 ` Greg Kroah-Hartman
  2022-01-20 12:48   ` Li Chen
  2022-01-20 12:47 ` [PATCH V4] " Li Chen
  1 sibling, 1 reply; 14+ messages in thread
From: Greg Kroah-Hartman @ 2022-01-20 11:57 UTC (permalink / raw)
  To: Li Chen
  Cc: Kishon Vijay Abraham I, Lorenzo Pieralisi,
	"Krzysztof Wilczyński",
	Arnd Bergmann, Bjorn Helgaas, linux-pci, linux-kernel

On Thu, Jan 20, 2022 at 06:11:36PM +0800, Li Chen wrote:
> Introduce pci_endpoint_epf_transfer_data to simplify
> read and write operations.
> 
> Signed-off-by: Li Chen <lchen@ambarella.com>

Does not match the From: line.  Please read the documentation for how to
fix this up and add a From: line to the email body itself.

thanks,

greg k-h

^ permalink raw reply	[flat|nested] 14+ messages in thread

* [PATCH V4] misc: pci_endpoint_test: simplify endpoint test read and write operations
  2022-01-20 10:11 [PATCH V3] misc: pci_endpoint_test: simplify endpoint test read and write operations Li Chen
  2022-01-20 11:57 ` Greg Kroah-Hartman
@ 2022-01-20 12:47 ` Li Chen
  2022-01-26 18:07   ` Greg Kroah-Hartman
  1 sibling, 1 reply; 14+ messages in thread
From: Li Chen @ 2022-01-20 12:47 UTC (permalink / raw)
  To: Kishon Vijay Abraham I, Lorenzo Pieralisi,
	"Krzysztof Wilczyński",
	Arnd Bergmann, Greg Kroah-Hartman, Bjorn Helgaas, linux-pci,
	linux-kernel

From: Li Chen <lchen@ambarella.com>

Introduce pci_endpoint_epf_transfer_data to simplify
read and write operations.

Signed-off-by: Li Chen <lchen@ambarella.com>
---
Changes in V2:
fix WARNING: line length of 108 exceeds 100 columns
#128: FILE: drivers/misc/pci_endpoint_test.c:243:
Changes in V3:
This patch context doesn't change but resend with my Zoho mail account in that previous
company mail will contain un-removeable proprietary messages.
Changes in V4:
Add "From:" to the first line of the message body.

drivers/misc/pci_endpoint_test.c | 289 ++++++++++++-------------------
1 file changed, 109 insertions(+), 180 deletions(-)

diff --git a/drivers/misc/pci_endpoint_test.c b/drivers/misc/pci_endpoint_test.c
index 2ed7e3aaff3a8..b6b0b19b251b3 100644
--- a/drivers/misc/pci_endpoint_test.c
+++ b/drivers/misc/pci_endpoint_test.c
@@ -103,6 +103,11 @@ enum pci_barno {
    BAR_5,
};

+enum operation {
+    EPF_READ,
+    EPF_WRITE,
+};
+
struct pci_endpoint_test {
    struct pci_dev    *pdev;
    void __iomem    *base;
@@ -142,6 +147,108 @@ static inline u32 pci_endpoint_test_bar_readl(struct pci_endpoint_test *test,
{
    return readl(test->bar[bar] + offset);
}
+static bool pci_endpoint_test_transfer_data(struct pci_endpoint_test *test,
+                unsigned long arg, const enum operation operation)
+{
+    struct pci_endpoint_test_xfer_param param;
+    bool ret = false;
+    u32 flags = 0;
+    bool use_dma;
+    void *addr;
+    dma_addr_t phys_addr;
+    struct pci_dev *pdev = test->pdev;
+    struct device *dev = &pdev->dev;
+    void *orig_addr;
+    dma_addr_t orig_phys_addr;
+    size_t offset;
+    size_t alignment = test->alignment;
+    int irq_type = test->irq_type;
+    size_t size;
+    int err;
+
+    err = copy_from_user(&param, (void __user *)arg, sizeof(param));
+    if (err != 0) {
+        dev_err(dev, "Failed to get transfer param\n");
+        return false;
+    }
+
+    size = param.size;
+    if (size > SIZE_MAX - alignment)
+        goto err;
+
+    use_dma = !!(param.flags & PCITEST_FLAGS_USE_DMA);
+    if (use_dma)
+        flags |= FLAG_USE_DMA;
+
+    if (irq_type < IRQ_TYPE_LEGACY || irq_type > IRQ_TYPE_MSIX) {
+        dev_err(dev, "Invalid IRQ type option\n");
+        goto err;
+    }
+
+    orig_addr = kzalloc(size + alignment, GFP_KERNEL);
+    if (!orig_addr)
+        goto err;
+
+    get_random_bytes(orig_addr, size + alignment);
+
+    orig_phys_addr = dma_map_single(dev, orig_addr, size + alignment,
+                    operation == EPF_WRITE ? DMA_TO_DEVICE : DMA_FROM_DEVICE);
+    if (dma_mapping_error(dev, orig_phys_addr)) {
+        dev_err(dev, "failed to map source buffer address\n");
+        goto err_phys_addr;
+    }
+
+    if (alignment && !IS_ALIGNED(orig_phys_addr, alignment)) {
+        phys_addr = PTR_ALIGN(orig_phys_addr, alignment);
+        offset = phys_addr - orig_phys_addr;
+        addr = orig_addr + offset;
+    } else {
+        phys_addr = orig_phys_addr;
+        addr = orig_addr;
+    }
+
+    if (operation == EPF_WRITE) {
+
+        pci_endpoint_test_writel(test, PCI_ENDPOINT_TEST_CHECKSUM,
+                 crc32_le(~0, addr, size));
+
+        pci_endpoint_test_writel(test, PCI_ENDPOINT_TEST_LOWER_SRC_ADDR,
+                                lower_32_bits(phys_addr));
+        pci_endpoint_test_writel(test, PCI_ENDPOINT_TEST_UPPER_SRC_ADDR,
+                                upper_32_bits(phys_addr));
+    } else {
+        pci_endpoint_test_writel(test, PCI_ENDPOINT_TEST_LOWER_DST_ADDR,
+                                lower_32_bits(phys_addr));
+        pci_endpoint_test_writel(test, PCI_ENDPOINT_TEST_UPPER_DST_ADDR,
+                                upper_32_bits(phys_addr));
+    }
+
+    pci_endpoint_test_writel(test, PCI_ENDPOINT_TEST_SIZE, size);
+    pci_endpoint_test_writel(test, PCI_ENDPOINT_TEST_FLAGS, flags);
+    pci_endpoint_test_writel(test, PCI_ENDPOINT_TEST_IRQ_TYPE, irq_type);
+    pci_endpoint_test_writel(test, PCI_ENDPOINT_TEST_IRQ_NUMBER, 1);
+
+    // if we ask rc to write to ep, then ep should do read operation, and vice versa.
+    pci_endpoint_test_writel(test, PCI_ENDPOINT_TEST_COMMAND,
+                 operation == EPF_WRITE ? COMMAND_READ : COMMAND_WRITE);
+
+    wait_for_completion(&test->irq_raised);
+
+    dma_unmap_single(dev, orig_phys_addr, size + alignment,
+                     operation == EPF_WRITE ? DMA_TO_DEVICE : DMA_FROM_DEVICE);
+
+    if (operation == WRITE)
+        ret = pci_endpoint_test_readl(test, PCI_ENDPOINT_TEST_STATUS) & STATUS_READ_SUCCESS;
+    else
+        ret = crc32_le(~0, addr, size) ==
+            pci_endpoint_test_readl(test, PCI_ENDPOINT_TEST_CHECKSUM);
+
+err_phys_addr:
+    kfree(orig_addr);
+
+err:
+    return ret;
+}

static inline void pci_endpoint_test_bar_writel(struct pci_endpoint_test *test,
                        int bar, u32 offset, u32 value)
@@ -473,191 +580,13 @@ static bool pci_endpoint_test_copy(struct pci_endpoint_test *test,
static bool pci_endpoint_test_write(struct pci_endpoint_test *test,
                 unsigned long arg)
{
-    struct pci_endpoint_test_xfer_param param;
-    bool ret = false;
-    u32 flags = 0;
-    bool use_dma;
-    u32 reg;
-    void *addr;
-    dma_addr_t phys_addr;
-    struct pci_dev *pdev = test->pdev;
-    struct device *dev = &pdev->dev;
-    void *orig_addr;
-    dma_addr_t orig_phys_addr;
-    size_t offset;
-    size_t alignment = test->alignment;
-    int irq_type = test->irq_type;
-    size_t size;
-    u32 crc32;
-    int err;
-
-    err = copy_from_user(&param, (void __user *)arg, sizeof(param));
-    if (err != 0) {
-        dev_err(dev, "Failed to get transfer param\n");
-        return false;
-    }
-
-    size = param.size;
-    if (size > SIZE_MAX - alignment)
-        goto err;
-
-    use_dma = !!(param.flags & PCITEST_FLAGS_USE_DMA);
-    if (use_dma)
-        flags |= FLAG_USE_DMA;
-
-    if (irq_type < IRQ_TYPE_LEGACY || irq_type > IRQ_TYPE_MSIX) {
-        dev_err(dev, "Invalid IRQ type option\n");
-        goto err;
-    }
-
-    orig_addr = kzalloc(size + alignment, GFP_KERNEL);
-    if (!orig_addr) {
-        dev_err(dev, "Failed to allocate address\n");
-        ret = false;
-        goto err;
-    }
-
-    get_random_bytes(orig_addr, size + alignment);
-
-    orig_phys_addr = dma_map_single(dev, orig_addr, size + alignment,
-                    DMA_TO_DEVICE);
-    if (dma_mapping_error(dev, orig_phys_addr)) {
-        dev_err(dev, "failed to map source buffer address\n");
-        ret = false;
-        goto err_phys_addr;
-    }
-
-    if (alignment && !IS_ALIGNED(orig_phys_addr, alignment)) {
-        phys_addr = PTR_ALIGN(orig_phys_addr, alignment);
-        offset = phys_addr - orig_phys_addr;
-        addr = orig_addr + offset;
-    } else {
-        phys_addr = orig_phys_addr;
-        addr = orig_addr;
-    }
-
-    crc32 = crc32_le(~0, addr, size);
-    pci_endpoint_test_writel(test, PCI_ENDPOINT_TEST_CHECKSUM,
-                 crc32);
-
-    pci_endpoint_test_writel(test, PCI_ENDPOINT_TEST_LOWER_SRC_ADDR,
-                 lower_32_bits(phys_addr));
-    pci_endpoint_test_writel(test, PCI_ENDPOINT_TEST_UPPER_SRC_ADDR,
-                 upper_32_bits(phys_addr));
-
-    pci_endpoint_test_writel(test, PCI_ENDPOINT_TEST_SIZE, size);
-
-    pci_endpoint_test_writel(test, PCI_ENDPOINT_TEST_FLAGS, flags);
-    pci_endpoint_test_writel(test, PCI_ENDPOINT_TEST_IRQ_TYPE, irq_type);
-    pci_endpoint_test_writel(test, PCI_ENDPOINT_TEST_IRQ_NUMBER, 1);
-    pci_endpoint_test_writel(test, PCI_ENDPOINT_TEST_COMMAND,
-                 COMMAND_READ);
-
-    wait_for_completion(&test->irq_raised);
-
-    reg = pci_endpoint_test_readl(test, PCI_ENDPOINT_TEST_STATUS);
-    if (reg & STATUS_READ_SUCCESS)
-        ret = true;
-
-    dma_unmap_single(dev, orig_phys_addr, size + alignment,
-             DMA_TO_DEVICE);
-
-err_phys_addr:
-    kfree(orig_addr);
-
-err:
-    return ret;
+    return pci_endpoint_test_transfer_data(test, arg, EPF_WRITE);
}

static bool pci_endpoint_test_read(struct pci_endpoint_test *test,
                 unsigned long arg)
{
-    struct pci_endpoint_test_xfer_param param;
-    bool ret = false;
-    u32 flags = 0;
-    bool use_dma;
-    size_t size;
-    void *addr;
-    dma_addr_t phys_addr;
-    struct pci_dev *pdev = test->pdev;
-    struct device *dev = &pdev->dev;
-    void *orig_addr;
-    dma_addr_t orig_phys_addr;
-    size_t offset;
-    size_t alignment = test->alignment;
-    int irq_type = test->irq_type;
-    u32 crc32;
-    int err;
-
-    err = copy_from_user(&param, (void __user *)arg, sizeof(param));
-    if (err) {
-        dev_err(dev, "Failed to get transfer param\n");
-        return false;
-    }
-
-    size = param.size;
-    if (size > SIZE_MAX - alignment)
-        goto err;
-
-    use_dma = !!(param.flags & PCITEST_FLAGS_USE_DMA);
-    if (use_dma)
-        flags |= FLAG_USE_DMA;
-
-    if (irq_type < IRQ_TYPE_LEGACY || irq_type > IRQ_TYPE_MSIX) {
-        dev_err(dev, "Invalid IRQ type option\n");
-        goto err;
-    }
-
-    orig_addr = kzalloc(size + alignment, GFP_KERNEL);
-    if (!orig_addr) {
-        dev_err(dev, "Failed to allocate destination address\n");
-        ret = false;
-        goto err;
-    }
-
-    orig_phys_addr = dma_map_single(dev, orig_addr, size + alignment,
-                    DMA_FROM_DEVICE);
-    if (dma_mapping_error(dev, orig_phys_addr)) {
-        dev_err(dev, "failed to map source buffer address\n");
-        ret = false;
-        goto err_phys_addr;
-    }
-
-    if (alignment && !IS_ALIGNED(orig_phys_addr, alignment)) {
-        phys_addr = PTR_ALIGN(orig_phys_addr, alignment);
-        offset = phys_addr - orig_phys_addr;
-        addr = orig_addr + offset;
-    } else {
-        phys_addr = orig_phys_addr;
-        addr = orig_addr;
-    }
-
-    pci_endpoint_test_writel(test, PCI_ENDPOINT_TEST_LOWER_DST_ADDR,
-                 lower_32_bits(phys_addr));
-    pci_endpoint_test_writel(test, PCI_ENDPOINT_TEST_UPPER_DST_ADDR,
-                 upper_32_bits(phys_addr));
-
-    pci_endpoint_test_writel(test, PCI_ENDPOINT_TEST_SIZE, size);
-
-    pci_endpoint_test_writel(test, PCI_ENDPOINT_TEST_FLAGS, flags);
-    pci_endpoint_test_writel(test, PCI_ENDPOINT_TEST_IRQ_TYPE, irq_type);
-    pci_endpoint_test_writel(test, PCI_ENDPOINT_TEST_IRQ_NUMBER, 1);
-    pci_endpoint_test_writel(test, PCI_ENDPOINT_TEST_COMMAND,
-                 COMMAND_WRITE);
-
-    wait_for_completion(&test->irq_raised);
-
-    dma_unmap_single(dev, orig_phys_addr, size + alignment,
-             DMA_FROM_DEVICE);
-
-    crc32 = crc32_le(~0, addr, size);
-    if (crc32 == pci_endpoint_test_readl(test, PCI_ENDPOINT_TEST_CHECKSUM))
-        ret = true;
-
-err_phys_addr:
-    kfree(orig_addr);
-err:
-    return ret;
+    return pci_endpoint_test_transfer_data(test, arg, EPF_READ);
}

static bool pci_endpoint_test_clear_irq(struct pci_endpoint_test *test)
--
2.34.1

^ permalink raw reply related	[flat|nested] 14+ messages in thread

* Re: [PATCH V3] misc: pci_endpoint_test: simplify endpoint test read and write operations
  2022-01-20 11:57 ` Greg Kroah-Hartman
@ 2022-01-20 12:48   ` Li Chen
  0 siblings, 0 replies; 14+ messages in thread
From: Li Chen @ 2022-01-20 12:48 UTC (permalink / raw)
  To: Greg Kroah-Hartman
  Cc: Kishon Vijay Abraham I, Lorenzo Pieralisi,
	"Krzysztof Wilczyński",
	Arnd Bergmann, Bjorn Helgaas, linux-pci, linux-kernel

 ---- On Thu, 20 Jan 2022 19:57:30 +0800 Greg Kroah-Hartman <gregkh@linuxfoundation.org> wrote ----
 > On Thu, Jan 20, 2022 at 06:11:36PM +0800, Li Chen wrote:
 > > Introduce pci_endpoint_epf_transfer_data to simplify
 > > read and write operations.
 > > 
 > > Signed-off-by: Li Chen <lchen@ambarella.com>
 > 
 > Does not match the From: line.  Please read the documentation for how to
 > fix this up and add a From: line to the email body itself.
 > 
 > thanks,
 > 
 > greg k-h
 > 

Thanks! This is fixed in patch V4.

Regards,
Li

^ permalink raw reply	[flat|nested] 14+ messages in thread

* Re: [PATCH V4] misc: pci_endpoint_test: simplify endpoint test read and write operations
  2022-01-20 12:47 ` [PATCH V4] " Li Chen
@ 2022-01-26 18:07   ` Greg Kroah-Hartman
  2022-02-07  8:42     ` [PATCH V5] " Li Chen
  0 siblings, 1 reply; 14+ messages in thread
From: Greg Kroah-Hartman @ 2022-01-26 18:07 UTC (permalink / raw)
  To: Li Chen
  Cc: Kishon Vijay Abraham I, Lorenzo Pieralisi,
	"Krzysztof Wilczyński",
	Arnd Bergmann, Bjorn Helgaas, linux-pci, linux-kernel

On Thu, Jan 20, 2022 at 08:47:50PM +0800, Li Chen wrote:
> From: Li Chen <lchen@ambarella.com>
> 
> Introduce pci_endpoint_epf_transfer_data to simplify
> read and write operations.
> 
> Signed-off-by: Li Chen <lchen@ambarella.com>
> ---
> Changes in V2:
> fix WARNING: line length of 108 exceeds 100 columns
> #128: FILE: drivers/misc/pci_endpoint_test.c:243:
> Changes in V3:
> This patch context doesn't change but resend with my Zoho mail account in that previous
> company mail will contain un-removeable proprietary messages.
> Changes in V4:
> Add "From:" to the first line of the message body.
> 
> drivers/misc/pci_endpoint_test.c | 289 ++++++++++++-------------------
> 1 file changed, 109 insertions(+), 180 deletions(-)
> 
> diff --git a/drivers/misc/pci_endpoint_test.c b/drivers/misc/pci_endpoint_test.c
> index 2ed7e3aaff3a8..b6b0b19b251b3 100644
> --- a/drivers/misc/pci_endpoint_test.c
> +++ b/drivers/misc/pci_endpoint_test.c
> @@ -103,6 +103,11 @@ enum pci_barno {
>     BAR_5,
> };
> 
> +enum operation {
> +    EPF_READ,
> +    EPF_WRITE,
> +};
> +
> struct pci_endpoint_test {
>     struct pci_dev    *pdev;
>     void __iomem    *base;
> @@ -142,6 +147,108 @@ static inline u32 pci_endpoint_test_bar_readl(struct pci_endpoint_test *test,
> {
>     return readl(test->bar[bar] + offset);
> }
> +static bool pci_endpoint_test_transfer_data(struct pci_endpoint_test *test,
> +                unsigned long arg, const enum operation operation)
> +{
> +    struct pci_endpoint_test_xfer_param param;
> +    bool ret = false;
> +    u32 flags = 0;
> +    bool use_dma;
> +    void *addr;
> +    dma_addr_t phys_addr;
> +    struct pci_dev *pdev = test->pdev;
> +    struct device *dev = &pdev->dev;
> +    void *orig_addr;
> +    dma_addr_t orig_phys_addr;
> +    size_t offset;
> +    size_t alignment = test->alignment;
> +    int irq_type = test->irq_type;
> +    size_t size;
> +    int err;
> +
> +    err = copy_from_user(&param, (void __user *)arg, sizeof(param));
> +    if (err != 0) {
> +        dev_err(dev, "Failed to get transfer param\n");
> +        return false;
> +    }
> +
> +    size = param.size;
> +    if (size > SIZE_MAX - alignment)
> +        goto err;
> +
> +    use_dma = !!(param.flags & PCITEST_FLAGS_USE_DMA);
> +    if (use_dma)
> +        flags |= FLAG_USE_DMA;
> +
> +    if (irq_type < IRQ_TYPE_LEGACY || irq_type > IRQ_TYPE_MSIX) {
> +        dev_err(dev, "Invalid IRQ type option\n");
> +        goto err;
> +    }
> +
> +    orig_addr = kzalloc(size + alignment, GFP_KERNEL);
> +    if (!orig_addr)
> +        goto err;
> +
> +    get_random_bytes(orig_addr, size + alignment);
> +
> +    orig_phys_addr = dma_map_single(dev, orig_addr, size + alignment,
> +                    operation == EPF_WRITE ? DMA_TO_DEVICE : DMA_FROM_DEVICE);
> +    if (dma_mapping_error(dev, orig_phys_addr)) {
> +        dev_err(dev, "failed to map source buffer address\n");
> +        goto err_phys_addr;
> +    }
> +
> +    if (alignment && !IS_ALIGNED(orig_phys_addr, alignment)) {
> +        phys_addr = PTR_ALIGN(orig_phys_addr, alignment);
> +        offset = phys_addr - orig_phys_addr;
> +        addr = orig_addr + offset;
> +    } else {
> +        phys_addr = orig_phys_addr;
> +        addr = orig_addr;
> +    }
> +
> +    if (operation == EPF_WRITE) {
> +
> +        pci_endpoint_test_writel(test, PCI_ENDPOINT_TEST_CHECKSUM,
> +                 crc32_le(~0, addr, size));
> +
> +        pci_endpoint_test_writel(test, PCI_ENDPOINT_TEST_LOWER_SRC_ADDR,
> +                                lower_32_bits(phys_addr));
> +        pci_endpoint_test_writel(test, PCI_ENDPOINT_TEST_UPPER_SRC_ADDR,
> +                                upper_32_bits(phys_addr));
> +    } else {
> +        pci_endpoint_test_writel(test, PCI_ENDPOINT_TEST_LOWER_DST_ADDR,
> +                                lower_32_bits(phys_addr));
> +        pci_endpoint_test_writel(test, PCI_ENDPOINT_TEST_UPPER_DST_ADDR,
> +                                upper_32_bits(phys_addr));
> +    }
> +
> +    pci_endpoint_test_writel(test, PCI_ENDPOINT_TEST_SIZE, size);
> +    pci_endpoint_test_writel(test, PCI_ENDPOINT_TEST_FLAGS, flags);
> +    pci_endpoint_test_writel(test, PCI_ENDPOINT_TEST_IRQ_TYPE, irq_type);
> +    pci_endpoint_test_writel(test, PCI_ENDPOINT_TEST_IRQ_NUMBER, 1);
> +
> +    // if we ask rc to write to ep, then ep should do read operation, and vice versa.
> +    pci_endpoint_test_writel(test, PCI_ENDPOINT_TEST_COMMAND,
> +                 operation == EPF_WRITE ? COMMAND_READ : COMMAND_WRITE);
> +
> +    wait_for_completion(&test->irq_raised);
> +
> +    dma_unmap_single(dev, orig_phys_addr, size + alignment,
> +                     operation == EPF_WRITE ? DMA_TO_DEVICE : DMA_FROM_DEVICE);
> +
> +    if (operation == WRITE)
> +        ret = pci_endpoint_test_readl(test, PCI_ENDPOINT_TEST_STATUS) & STATUS_READ_SUCCESS;
> +    else
> +        ret = crc32_le(~0, addr, size) ==
> +            pci_endpoint_test_readl(test, PCI_ENDPOINT_TEST_CHECKSUM);
> +
> +err_phys_addr:
> +    kfree(orig_addr);
> +
> +err:
> +    return ret;
> +}
> 
> static inline void pci_endpoint_test_bar_writel(struct pci_endpoint_test *test,
>                         int bar, u32 offset, u32 value)
> @@ -473,191 +580,13 @@ static bool pci_endpoint_test_copy(struct pci_endpoint_test *test,
> static bool pci_endpoint_test_write(struct pci_endpoint_test *test,
>                  unsigned long arg)
> {
> -    struct pci_endpoint_test_xfer_param param;
> -    bool ret = false;
> -    u32 flags = 0;
> -    bool use_dma;
> -    u32 reg;
> -    void *addr;
> -    dma_addr_t phys_addr;
> -    struct pci_dev *pdev = test->pdev;
> -    struct device *dev = &pdev->dev;
> -    void *orig_addr;
> -    dma_addr_t orig_phys_addr;
> -    size_t offset;
> -    size_t alignment = test->alignment;
> -    int irq_type = test->irq_type;
> -    size_t size;
> -    u32 crc32;
> -    int err;
> -
> -    err = copy_from_user(&param, (void __user *)arg, sizeof(param));
> -    if (err != 0) {
> -        dev_err(dev, "Failed to get transfer param\n");
> -        return false;
> -    }
> -
> -    size = param.size;
> -    if (size > SIZE_MAX - alignment)
> -        goto err;
> -
> -    use_dma = !!(param.flags & PCITEST_FLAGS_USE_DMA);
> -    if (use_dma)
> -        flags |= FLAG_USE_DMA;
> -
> -    if (irq_type < IRQ_TYPE_LEGACY || irq_type > IRQ_TYPE_MSIX) {
> -        dev_err(dev, "Invalid IRQ type option\n");
> -        goto err;
> -    }
> -
> -    orig_addr = kzalloc(size + alignment, GFP_KERNEL);
> -    if (!orig_addr) {
> -        dev_err(dev, "Failed to allocate address\n");
> -        ret = false;
> -        goto err;
> -    }
> -
> -    get_random_bytes(orig_addr, size + alignment);
> -
> -    orig_phys_addr = dma_map_single(dev, orig_addr, size + alignment,
> -                    DMA_TO_DEVICE);
> -    if (dma_mapping_error(dev, orig_phys_addr)) {
> -        dev_err(dev, "failed to map source buffer address\n");
> -        ret = false;
> -        goto err_phys_addr;
> -    }
> -
> -    if (alignment && !IS_ALIGNED(orig_phys_addr, alignment)) {
> -        phys_addr = PTR_ALIGN(orig_phys_addr, alignment);
> -        offset = phys_addr - orig_phys_addr;
> -        addr = orig_addr + offset;
> -    } else {
> -        phys_addr = orig_phys_addr;
> -        addr = orig_addr;
> -    }
> -
> -    crc32 = crc32_le(~0, addr, size);
> -    pci_endpoint_test_writel(test, PCI_ENDPOINT_TEST_CHECKSUM,
> -                 crc32);
> -
> -    pci_endpoint_test_writel(test, PCI_ENDPOINT_TEST_LOWER_SRC_ADDR,
> -                 lower_32_bits(phys_addr));
> -    pci_endpoint_test_writel(test, PCI_ENDPOINT_TEST_UPPER_SRC_ADDR,
> -                 upper_32_bits(phys_addr));
> -
> -    pci_endpoint_test_writel(test, PCI_ENDPOINT_TEST_SIZE, size);
> -
> -    pci_endpoint_test_writel(test, PCI_ENDPOINT_TEST_FLAGS, flags);
> -    pci_endpoint_test_writel(test, PCI_ENDPOINT_TEST_IRQ_TYPE, irq_type);
> -    pci_endpoint_test_writel(test, PCI_ENDPOINT_TEST_IRQ_NUMBER, 1);
> -    pci_endpoint_test_writel(test, PCI_ENDPOINT_TEST_COMMAND,
> -                 COMMAND_READ);
> -
> -    wait_for_completion(&test->irq_raised);
> -
> -    reg = pci_endpoint_test_readl(test, PCI_ENDPOINT_TEST_STATUS);
> -    if (reg & STATUS_READ_SUCCESS)
> -        ret = true;
> -
> -    dma_unmap_single(dev, orig_phys_addr, size + alignment,
> -             DMA_TO_DEVICE);
> -
> -err_phys_addr:
> -    kfree(orig_addr);
> -
> -err:
> -    return ret;
> +    return pci_endpoint_test_transfer_data(test, arg, EPF_WRITE);
> }
> 
> static bool pci_endpoint_test_read(struct pci_endpoint_test *test,
>                  unsigned long arg)
> {
> -    struct pci_endpoint_test_xfer_param param;
> -    bool ret = false;
> -    u32 flags = 0;
> -    bool use_dma;
> -    size_t size;
> -    void *addr;
> -    dma_addr_t phys_addr;
> -    struct pci_dev *pdev = test->pdev;
> -    struct device *dev = &pdev->dev;
> -    void *orig_addr;
> -    dma_addr_t orig_phys_addr;
> -    size_t offset;
> -    size_t alignment = test->alignment;
> -    int irq_type = test->irq_type;
> -    u32 crc32;
> -    int err;
> -
> -    err = copy_from_user(&param, (void __user *)arg, sizeof(param));
> -    if (err) {
> -        dev_err(dev, "Failed to get transfer param\n");
> -        return false;
> -    }
> -
> -    size = param.size;
> -    if (size > SIZE_MAX - alignment)
> -        goto err;
> -
> -    use_dma = !!(param.flags & PCITEST_FLAGS_USE_DMA);
> -    if (use_dma)
> -        flags |= FLAG_USE_DMA;
> -
> -    if (irq_type < IRQ_TYPE_LEGACY || irq_type > IRQ_TYPE_MSIX) {
> -        dev_err(dev, "Invalid IRQ type option\n");
> -        goto err;
> -    }
> -
> -    orig_addr = kzalloc(size + alignment, GFP_KERNEL);
> -    if (!orig_addr) {
> -        dev_err(dev, "Failed to allocate destination address\n");
> -        ret = false;
> -        goto err;
> -    }
> -
> -    orig_phys_addr = dma_map_single(dev, orig_addr, size + alignment,
> -                    DMA_FROM_DEVICE);
> -    if (dma_mapping_error(dev, orig_phys_addr)) {
> -        dev_err(dev, "failed to map source buffer address\n");
> -        ret = false;
> -        goto err_phys_addr;
> -    }
> -
> -    if (alignment && !IS_ALIGNED(orig_phys_addr, alignment)) {
> -        phys_addr = PTR_ALIGN(orig_phys_addr, alignment);
> -        offset = phys_addr - orig_phys_addr;
> -        addr = orig_addr + offset;
> -    } else {
> -        phys_addr = orig_phys_addr;
> -        addr = orig_addr;
> -    }
> -
> -    pci_endpoint_test_writel(test, PCI_ENDPOINT_TEST_LOWER_DST_ADDR,
> -                 lower_32_bits(phys_addr));
> -    pci_endpoint_test_writel(test, PCI_ENDPOINT_TEST_UPPER_DST_ADDR,
> -                 upper_32_bits(phys_addr));
> -
> -    pci_endpoint_test_writel(test, PCI_ENDPOINT_TEST_SIZE, size);
> -
> -    pci_endpoint_test_writel(test, PCI_ENDPOINT_TEST_FLAGS, flags);
> -    pci_endpoint_test_writel(test, PCI_ENDPOINT_TEST_IRQ_TYPE, irq_type);
> -    pci_endpoint_test_writel(test, PCI_ENDPOINT_TEST_IRQ_NUMBER, 1);
> -    pci_endpoint_test_writel(test, PCI_ENDPOINT_TEST_COMMAND,
> -                 COMMAND_WRITE);
> -
> -    wait_for_completion(&test->irq_raised);
> -
> -    dma_unmap_single(dev, orig_phys_addr, size + alignment,
> -             DMA_FROM_DEVICE);
> -
> -    crc32 = crc32_le(~0, addr, size);
> -    if (crc32 == pci_endpoint_test_readl(test, PCI_ENDPOINT_TEST_CHECKSUM))
> -        ret = true;
> -
> -err_phys_addr:
> -    kfree(orig_addr);
> -err:
> -    return ret;
> +    return pci_endpoint_test_transfer_data(test, arg, EPF_READ);
> }
> 
> static bool pci_endpoint_test_clear_irq(struct pci_endpoint_test *test)
> --
> 2.34.1

Hi,

This is the friendly patch-bot of Greg Kroah-Hartman.  You have sent him
a patch that has triggered this response.  He used to manually respond
to these common problems, but in order to save his sanity (he kept
writing the same thing over and over, yet to different people), I was
created.  Hopefully you will not take offence and will fix the problem
in your patch and resubmit it so that it can be accepted into the Linux
kernel tree.

You are receiving this message because of the following common error(s)
as indicated below:

- Your patch is malformed (tabs converted to spaces, linewrapped, etc.)
  and can not be applied.  Please read the file,
  Documentation/email-clients.txt in order to fix this.
If you wish to discuss this problem further, or you have questions about
how to resolve this issue, please feel free to respond to this email and
Greg will reply once he has dug out from the pending patches received
from other developers.

thanks,

greg k-h's patch email bot

^ permalink raw reply	[flat|nested] 14+ messages in thread

* [PATCH V5] misc: pci_endpoint_test: simplify endpoint test read and write operations
  2022-01-26 18:07   ` Greg Kroah-Hartman
@ 2022-02-07  8:42     ` Li Chen
  2022-02-07  9:09       ` [PATCH V6] " Li Chen
  0 siblings, 1 reply; 14+ messages in thread
From: Li Chen @ 2022-02-07  8:42 UTC (permalink / raw)
  To: Greg Kroah-Hartman
  Cc: Kishon Vijay Abraham I, Lorenzo Pieralisi,
	"Krzysztof Wilczyński",
	Arnd Bergmann, Bjorn Helgaas, linux-pci, linux-kernel

From: Li Chen <lchen@ambarella.com>
Date: Thu, 20 Jan 2022 15:40:43 +0800
Subject: [PATCH v5] misc: pci_endpoint_test: simplify endpoint test read and
 write operations

Introduce pci_endpoint_epf_transfer_data to simplify
read and write operations.

Also tabify this file.

Signed-off-by: Li Chen <lchen@ambarella.com>
---
Changes in V2:
fix WARNING: line length of 108 exceeds 100 columns
#128: FILE: drivers/misc/pci_endpoint_test.c:243:
Changes in V3:
This patch context doesn't change but resend with my Zoho mail account in that previous
company mail will contain un-removeable proprietary messages.
Changes in V4:
Add "From:" to the first line of the message body. 
Changes in V5:
tabify file
replace enum EPF_WRITE/EPF_READ with WRITE/READ from linux/kernel.h
get_random_bytes only when WRITE.

 drivers/misc/pci_endpoint_test.c | 310 ++++++++++++-------------------
 1 file changed, 118 insertions(+), 192 deletions(-)

diff --git a/drivers/misc/pci_endpoint_test.c b/drivers/misc/pci_endpoint_test.c
index 2ed7e3aaff3a8..080239ae9601d 100644
--- a/drivers/misc/pci_endpoint_test.c
+++ b/drivers/misc/pci_endpoint_test.c
@@ -12,6 +12,7 @@
 #include <linux/io.h>
 #include <linux/interrupt.h>
 #include <linux/irq.h>
+#include <linux/kernel.h>
 #include <linux/miscdevice.h>
 #include <linux/module.h>
 #include <linux/mutex.h>
@@ -84,7 +85,7 @@
 static DEFINE_IDA(pci_endpoint_test_ida);
 
 #define to_endpoint_test(priv) container_of((priv), struct pci_endpoint_test, \
-					    miscdev)
+						miscdev)
 
 static bool no_msi;
 module_param(no_msi, bool, 0444);
@@ -132,16 +133,119 @@ static inline u32 pci_endpoint_test_readl(struct pci_endpoint_test *test,
 }
 
 static inline void pci_endpoint_test_writel(struct pci_endpoint_test *test,
-					    u32 offset, u32 value)
+						u32 offset, u32 value)
 {
 	writel(value, test->base + offset);
 }
 
 static inline u32 pci_endpoint_test_bar_readl(struct pci_endpoint_test *test,
-					      int bar, int offset)
+						  int bar, int offset)
 {
 	return readl(test->bar[bar] + offset);
 }
+static bool pci_endpoint_test_transfer_data(struct pci_endpoint_test *test,
+				unsigned long arg, const int operation)
+{
+	struct pci_endpoint_test_xfer_param param;
+	bool ret = false;
+	u32 flags = 0;
+	bool use_dma;
+	void *addr;
+	dma_addr_t phys_addr;
+	struct pci_dev *pdev = test->pdev;
+	struct device *dev = &pdev->dev;
+	void *orig_addr;
+	dma_addr_t orig_phys_addr;
+	size_t offset;
+	size_t alignment = test->alignment;
+	int irq_type = test->irq_type;
+	size_t size;
+	int err;
+
+	err = copy_from_user(&param, (void __user *)arg, sizeof(param));
+	if (err != 0) {
+		dev_err(dev, "Failed to get transfer param\n");
+		return false;
+	}
+
+	size = param.size;
+	if (size > SIZE_MAX - alignment)
+		goto err;
+
+	use_dma = !!(param.flags & PCITEST_FLAGS_USE_DMA);
+	if (use_dma)
+		flags |= FLAG_USE_DMA;
+
+	if (irq_type < IRQ_TYPE_LEGACY || irq_type > IRQ_TYPE_MSIX) {
+		dev_err(dev, "Invalid IRQ type option\n");
+		goto err;
+	}
+
+	orig_addr = kzalloc(size + alignment, GFP_KERNEL);
+	if (!orig_addr)
+		goto err;
+
+	if (operation == WRITE)
+		get_random_bytes(orig_addr, size + alignment);
+
+	orig_phys_addr = dma_map_single(dev, orig_addr, size + alignment,
+					operation == WRITE ? DMA_TO_DEVICE : DMA_FROM_DEVICE);
+	if (dma_mapping_error(dev, orig_phys_addr)) {
+		dev_err(dev, "failed to map source buffer address\n");
+		goto err_phys_addr;
+	}
+
+	if (alignment && !IS_ALIGNED(orig_phys_addr, alignment)) {
+		phys_addr = PTR_ALIGN(orig_phys_addr, alignment);
+		offset = phys_addr - orig_phys_addr;
+		addr = orig_addr + offset;
+	} else {
+		phys_addr = orig_phys_addr;
+		addr = orig_addr;
+	}
+
+	if (operation == WRITE) {
+
+		pci_endpoint_test_writel(test, PCI_ENDPOINT_TEST_CHECKSUM,
+				 crc32_le(~0, addr, size));
+
+		pci_endpoint_test_writel(test, PCI_ENDPOINT_TEST_LOWER_SRC_ADDR,
+								lower_32_bits(phys_addr));
+		pci_endpoint_test_writel(test, PCI_ENDPOINT_TEST_UPPER_SRC_ADDR,
+								upper_32_bits(phys_addr));
+	} else {
+		pci_endpoint_test_writel(test, PCI_ENDPOINT_TEST_LOWER_DST_ADDR,
+								lower_32_bits(phys_addr));
+		pci_endpoint_test_writel(test, PCI_ENDPOINT_TEST_UPPER_DST_ADDR,
+								upper_32_bits(phys_addr));
+	}
+
+	pci_endpoint_test_writel(test, PCI_ENDPOINT_TEST_SIZE, size);
+	pci_endpoint_test_writel(test, PCI_ENDPOINT_TEST_FLAGS, flags);
+	pci_endpoint_test_writel(test, PCI_ENDPOINT_TEST_IRQ_TYPE, irq_type);
+	pci_endpoint_test_writel(test, PCI_ENDPOINT_TEST_IRQ_NUMBER, 1);
+
+	// if we ask rc to write to ep, then ep should do read operation, and vice versa.
+	pci_endpoint_test_writel(test, PCI_ENDPOINT_TEST_COMMAND,
+				 operation == WRITE ? COMMAND_READ : COMMAND_WRITE);
+
+	wait_for_completion(&test->irq_raised);
+
+	dma_unmap_single(dev, orig_phys_addr, size + alignment,
+					 operation == WRITE ? DMA_TO_DEVICE : DMA_FROM_DEVICE);
+
+	if (operation == WRITE)
+		ret = pci_endpoint_test_readl(test, PCI_ENDPOINT_TEST_STATUS) & STATUS_READ_SUCCESS;
+	else
+		ret = crc32_le(~0, addr, size) ==
+			pci_endpoint_test_readl(test, PCI_ENDPOINT_TEST_CHECKSUM);
+
+err_phys_addr:
+	kfree(orig_addr);
+
+err:
+	return ret;
+}
 
 static inline void pci_endpoint_test_bar_writel(struct pci_endpoint_test *test,
 						int bar, u32 offset, u32 value)
@@ -234,8 +338,8 @@ static bool pci_endpoint_test_request_irq(struct pci_endpoint_test *test)
 
 	for (i = 0; i < test->num_irqs; i++) {
 		err = devm_request_irq(dev, pci_irq_vector(pdev, i),
-				       pci_endpoint_test_irqhandler,
-				       IRQF_SHARED, test->name, test);
+					   pci_endpoint_test_irqhandler,
+					   IRQF_SHARED, test->name, test);
 		if (err)
 			goto fail;
 	}
@@ -309,7 +413,7 @@ static bool pci_endpoint_test_legacy_irq(struct pci_endpoint_test *test)
 }
 
 static bool pci_endpoint_test_msi_irq(struct pci_endpoint_test *test,
-				       u16 msi_num, bool msix)
+					   u16 msi_num, bool msix)
 {
 	u32 val;
 	struct pci_dev *pdev = test->pdev;
@@ -385,7 +489,7 @@ static bool pci_endpoint_test_copy(struct pci_endpoint_test *test,
 
 	get_random_bytes(orig_src_addr, size + alignment);
 	orig_src_phys_addr = dma_map_single(dev, orig_src_addr,
-					    size + alignment, DMA_TO_DEVICE);
+						size + alignment, DMA_TO_DEVICE);
 	if (dma_mapping_error(dev, orig_src_phys_addr)) {
 		dev_err(dev, "failed to map source buffer address\n");
 		ret = false;
@@ -417,7 +521,7 @@ static bool pci_endpoint_test_copy(struct pci_endpoint_test *test,
 	}
 
 	orig_dst_phys_addr = dma_map_single(dev, orig_dst_addr,
-					    size + alignment, DMA_FROM_DEVICE);
+						size + alignment, DMA_FROM_DEVICE);
 	if (dma_mapping_error(dev, orig_dst_phys_addr)) {
 		dev_err(dev, "failed to map destination buffer address\n");
 		ret = false;
@@ -471,193 +575,15 @@ static bool pci_endpoint_test_copy(struct pci_endpoint_test *test,
 }
 
 static bool pci_endpoint_test_write(struct pci_endpoint_test *test,
-				    unsigned long arg)
+					unsigned long arg)
 {
-	struct pci_endpoint_test_xfer_param param;
-	bool ret = false;
-	u32 flags = 0;
-	bool use_dma;
-	u32 reg;
-	void *addr;
-	dma_addr_t phys_addr;
-	struct pci_dev *pdev = test->pdev;
-	struct device *dev = &pdev->dev;
-	void *orig_addr;
-	dma_addr_t orig_phys_addr;
-	size_t offset;
-	size_t alignment = test->alignment;
-	int irq_type = test->irq_type;
-	size_t size;
-	u32 crc32;
-	int err;
-
-	err = copy_from_user(&param, (void __user *)arg, sizeof(param));
-	if (err != 0) {
-		dev_err(dev, "Failed to get transfer param\n");
-		return false;
-	}
-
-	size = param.size;
-	if (size > SIZE_MAX - alignment)
-		goto err;
-
-	use_dma = !!(param.flags & PCITEST_FLAGS_USE_DMA);
-	if (use_dma)
-		flags |= FLAG_USE_DMA;
-
-	if (irq_type < IRQ_TYPE_LEGACY || irq_type > IRQ_TYPE_MSIX) {
-		dev_err(dev, "Invalid IRQ type option\n");
-		goto err;
-	}
-
-	orig_addr = kzalloc(size + alignment, GFP_KERNEL);
-	if (!orig_addr) {
-		dev_err(dev, "Failed to allocate address\n");
-		ret = false;
-		goto err;
-	}
-
-	get_random_bytes(orig_addr, size + alignment);
-
-	orig_phys_addr = dma_map_single(dev, orig_addr, size + alignment,
-					DMA_TO_DEVICE);
-	if (dma_mapping_error(dev, orig_phys_addr)) {
-		dev_err(dev, "failed to map source buffer address\n");
-		ret = false;
-		goto err_phys_addr;
-	}
-
-	if (alignment && !IS_ALIGNED(orig_phys_addr, alignment)) {
-		phys_addr =  PTR_ALIGN(orig_phys_addr, alignment);
-		offset = phys_addr - orig_phys_addr;
-		addr = orig_addr + offset;
-	} else {
-		phys_addr = orig_phys_addr;
-		addr = orig_addr;
-	}
-
-	crc32 = crc32_le(~0, addr, size);
-	pci_endpoint_test_writel(test, PCI_ENDPOINT_TEST_CHECKSUM,
-				 crc32);
-
-	pci_endpoint_test_writel(test, PCI_ENDPOINT_TEST_LOWER_SRC_ADDR,
-				 lower_32_bits(phys_addr));
-	pci_endpoint_test_writel(test, PCI_ENDPOINT_TEST_UPPER_SRC_ADDR,
-				 upper_32_bits(phys_addr));
-
-	pci_endpoint_test_writel(test, PCI_ENDPOINT_TEST_SIZE, size);
-
-	pci_endpoint_test_writel(test, PCI_ENDPOINT_TEST_FLAGS, flags);
-	pci_endpoint_test_writel(test, PCI_ENDPOINT_TEST_IRQ_TYPE, irq_type);
-	pci_endpoint_test_writel(test, PCI_ENDPOINT_TEST_IRQ_NUMBER, 1);
-	pci_endpoint_test_writel(test, PCI_ENDPOINT_TEST_COMMAND,
-				 COMMAND_READ);
-
-	wait_for_completion(&test->irq_raised);
-
-	reg = pci_endpoint_test_readl(test, PCI_ENDPOINT_TEST_STATUS);
-	if (reg & STATUS_READ_SUCCESS)
-		ret = true;
-
-	dma_unmap_single(dev, orig_phys_addr, size + alignment,
-			 DMA_TO_DEVICE);
-
-err_phys_addr:
-	kfree(orig_addr);
-
-err:
-	return ret;
+	return pci_endpoint_test_transfer_data(test, arg, WRITE);
 }
 
 static bool pci_endpoint_test_read(struct pci_endpoint_test *test,
 				   unsigned long arg)
 {
-	struct pci_endpoint_test_xfer_param param;
-	bool ret = false;
-	u32 flags = 0;
-	bool use_dma;
-	size_t size;
-	void *addr;
-	dma_addr_t phys_addr;
-	struct pci_dev *pdev = test->pdev;
-	struct device *dev = &pdev->dev;
-	void *orig_addr;
-	dma_addr_t orig_phys_addr;
-	size_t offset;
-	size_t alignment = test->alignment;
-	int irq_type = test->irq_type;
-	u32 crc32;
-	int err;
-
-	err = copy_from_user(&param, (void __user *)arg, sizeof(param));
-	if (err) {
-		dev_err(dev, "Failed to get transfer param\n");
-		return false;
-	}
-
-	size = param.size;
-	if (size > SIZE_MAX - alignment)
-		goto err;
-
-	use_dma = !!(param.flags & PCITEST_FLAGS_USE_DMA);
-	if (use_dma)
-		flags |= FLAG_USE_DMA;
-
-	if (irq_type < IRQ_TYPE_LEGACY || irq_type > IRQ_TYPE_MSIX) {
-		dev_err(dev, "Invalid IRQ type option\n");
-		goto err;
-	}
-
-	orig_addr = kzalloc(size + alignment, GFP_KERNEL);
-	if (!orig_addr) {
-		dev_err(dev, "Failed to allocate destination address\n");
-		ret = false;
-		goto err;
-	}
-
-	orig_phys_addr = dma_map_single(dev, orig_addr, size + alignment,
-					DMA_FROM_DEVICE);
-	if (dma_mapping_error(dev, orig_phys_addr)) {
-		dev_err(dev, "failed to map source buffer address\n");
-		ret = false;
-		goto err_phys_addr;
-	}
-
-	if (alignment && !IS_ALIGNED(orig_phys_addr, alignment)) {
-		phys_addr = PTR_ALIGN(orig_phys_addr, alignment);
-		offset = phys_addr - orig_phys_addr;
-		addr = orig_addr + offset;
-	} else {
-		phys_addr = orig_phys_addr;
-		addr = orig_addr;
-	}
-
-	pci_endpoint_test_writel(test, PCI_ENDPOINT_TEST_LOWER_DST_ADDR,
-				 lower_32_bits(phys_addr));
-	pci_endpoint_test_writel(test, PCI_ENDPOINT_TEST_UPPER_DST_ADDR,
-				 upper_32_bits(phys_addr));
-
-	pci_endpoint_test_writel(test, PCI_ENDPOINT_TEST_SIZE, size);
-
-	pci_endpoint_test_writel(test, PCI_ENDPOINT_TEST_FLAGS, flags);
-	pci_endpoint_test_writel(test, PCI_ENDPOINT_TEST_IRQ_TYPE, irq_type);
-	pci_endpoint_test_writel(test, PCI_ENDPOINT_TEST_IRQ_NUMBER, 1);
-	pci_endpoint_test_writel(test, PCI_ENDPOINT_TEST_COMMAND,
-				 COMMAND_WRITE);
-
-	wait_for_completion(&test->irq_raised);
-
-	dma_unmap_single(dev, orig_phys_addr, size + alignment,
-			 DMA_FROM_DEVICE);
-
-	crc32 = crc32_le(~0, addr, size);
-	if (crc32 == pci_endpoint_test_readl(test, PCI_ENDPOINT_TEST_CHECKSUM))
-		ret = true;
-
-err_phys_addr:
-	kfree(orig_addr);
-err:
-	return ret;
+	return pci_endpoint_test_transfer_data(test, arg, READ);
 }
 
 static bool pci_endpoint_test_clear_irq(struct pci_endpoint_test *test)
@@ -668,7 +594,7 @@ static bool pci_endpoint_test_clear_irq(struct pci_endpoint_test *test)
 }
 
 static bool pci_endpoint_test_set_irq(struct pci_endpoint_test *test,
-				      int req_irq_type)
+					  int req_irq_type)
 {
 	struct pci_dev *pdev = test->pdev;
 	struct device *dev = &pdev->dev;
@@ -698,7 +624,7 @@ static bool pci_endpoint_test_set_irq(struct pci_endpoint_test *test,
 }
 
 static long pci_endpoint_test_ioctl(struct file *file, unsigned int cmd,
-				    unsigned long arg)
+					unsigned long arg)
 {
 	int ret = -EINVAL;
 	enum pci_barno bar;
@@ -793,7 +719,7 @@ static int pci_endpoint_test_probe(struct pci_dev *pdev,
 	mutex_init(&test->mutex);
 
 	if ((dma_set_mask_and_coherent(&pdev->dev, DMA_BIT_MASK(48)) != 0) &&
-	    dma_set_mask_and_coherent(&pdev->dev, DMA_BIT_MASK(32)) != 0) {
+		dma_set_mask_and_coherent(&pdev->dev, DMA_BIT_MASK(32)) != 0) {
 		dev_err(dev, "Cannot set DMA mask\n");
 		return -EINVAL;
 	}
-- 
2.34.1



^ permalink raw reply related	[flat|nested] 14+ messages in thread

* [PATCH V6] misc: pci_endpoint_test: simplify endpoint test read and write operations
  2022-02-07  8:42     ` [PATCH V5] " Li Chen
@ 2022-02-07  9:09       ` Li Chen
  2022-02-07 17:56         ` Bjorn Helgaas
  0 siblings, 1 reply; 14+ messages in thread
From: Li Chen @ 2022-02-07  9:09 UTC (permalink / raw)
  To: Greg Kroah-Hartman
  Cc: Kishon Vijay Abraham I, Lorenzo Pieralisi,
	"Krzysztof Wilczyński",
	Arnd Bergmann, Bjorn Helgaas, linux-pci, linux-kernel

From: Li Chen <lchen@ambarella.com>

Introduce pci_endpoint_epf_transfer_data to simplify
read and write operations.

Also tabify this file.

Signed-off-by: Li Chen <lchen@ambarella.com>
---
Changes in V2:
fix WARNING: line length of 108 exceeds 100 columns
#128: FILE: drivers/misc/pci_endpoint_test.c:243:
Changes in V3:
This patch context doesn't change but resend with my Zoho mail account in that previous
company mail will contain un-removeable proprietary messages.
Changes in V4:
Add "From:" to the first line of the message body.
Changes in V5:
tabify file
replace enum EPF_WRITE/EPF_READ with WRITE/READ from linux/kernel.h
get_random_bytes only when WRITE.
Changes in V6:
remove useless "Date:" and "Subject:" in message body, only preserve "From:" tag.

drivers/misc/pci_endpoint_test.c | 310 ++++++++++++-------------------
1 file changed, 118 insertions(+), 192 deletions(-)

diff --git a/drivers/misc/pci_endpoint_test.c b/drivers/misc/pci_endpoint_test.c
index 2ed7e3aaff3a8..080239ae9601d 100644
--- a/drivers/misc/pci_endpoint_test.c
+++ b/drivers/misc/pci_endpoint_test.c
@@ -12,6 +12,7 @@
#include <linux/io.h>
#include <linux/interrupt.h>
#include <linux/irq.h>
+#include <linux/kernel.h>
#include <linux/miscdevice.h>
#include <linux/module.h>
#include <linux/mutex.h>
@@ -84,7 +85,7 @@
static DEFINE_IDA(pci_endpoint_test_ida);

#define to_endpoint_test(priv) container_of((priv), struct pci_endpoint_test, \
-                     miscdev)
+                        miscdev)

static bool no_msi;
module_param(no_msi, bool, 0444);
@@ -132,16 +133,119 @@ static inline u32 pci_endpoint_test_readl(struct pci_endpoint_test *test,
}

static inline void pci_endpoint_test_writel(struct pci_endpoint_test *test,
-                     u32 offset, u32 value)
+                        u32 offset, u32 value)
{
    writel(value, test->base + offset);
}

static inline u32 pci_endpoint_test_bar_readl(struct pci_endpoint_test *test,
-                     int bar, int offset)
+                         int bar, int offset)
{
    return readl(test->bar[bar] + offset);
}
+static bool pci_endpoint_test_transfer_data(struct pci_endpoint_test *test,
+                unsigned long arg, const int operation)
+{
+    struct pci_endpoint_test_xfer_param param;
+    bool ret = false;
+    u32 flags = 0;
+    bool use_dma;
+    void *addr;
+    dma_addr_t phys_addr;
+    struct pci_dev *pdev = test->pdev;
+    struct device *dev = &pdev->dev;
+    void *orig_addr;
+    dma_addr_t orig_phys_addr;
+    size_t offset;
+    size_t alignment = test->alignment;
+    int irq_type = test->irq_type;
+    size_t size;
+    int err;
+
+    err = copy_from_user(&param, (void __user *)arg, sizeof(param));
+    if (err != 0) {
+        dev_err(dev, "Failed to get transfer param\n");
+        return false;
+    }
+
+    size = param.size;
+    if (size > SIZE_MAX - alignment)
+        goto err;
+
+    use_dma = !!(param.flags & PCITEST_FLAGS_USE_DMA);
+    if (use_dma)
+        flags |= FLAG_USE_DMA;
+
+    if (irq_type < IRQ_TYPE_LEGACY || irq_type > IRQ_TYPE_MSIX) {
+        dev_err(dev, "Invalid IRQ type option\n");
+        goto err;
+    }
+
+    orig_addr = kzalloc(size + alignment, GFP_KERNEL);
+    if (!orig_addr)
+        goto err;
+
+    if (operation == WRITE)
+        get_random_bytes(orig_addr, size + alignment);
+
+    orig_phys_addr = dma_map_single(dev, orig_addr, size + alignment,
+                    operation == WRITE ? DMA_TO_DEVICE : DMA_FROM_DEVICE);
+    if (dma_mapping_error(dev, orig_phys_addr)) {
+        dev_err(dev, "failed to map source buffer address\n");
+        goto err_phys_addr;
+    }
+
+    if (alignment && !IS_ALIGNED(orig_phys_addr, alignment)) {
+        phys_addr = PTR_ALIGN(orig_phys_addr, alignment);
+        offset = phys_addr - orig_phys_addr;
+        addr = orig_addr + offset;
+    } else {
+        phys_addr = orig_phys_addr;
+        addr = orig_addr;
+    }
+
+    if (operation == WRITE) {
+
+        pci_endpoint_test_writel(test, PCI_ENDPOINT_TEST_CHECKSUM,
+                 crc32_le(~0, addr, size));
+
+        pci_endpoint_test_writel(test, PCI_ENDPOINT_TEST_LOWER_SRC_ADDR,
+                                lower_32_bits(phys_addr));
+        pci_endpoint_test_writel(test, PCI_ENDPOINT_TEST_UPPER_SRC_ADDR,
+                                upper_32_bits(phys_addr));
+    } else {
+        pci_endpoint_test_writel(test, PCI_ENDPOINT_TEST_LOWER_DST_ADDR,
+                                lower_32_bits(phys_addr));
+        pci_endpoint_test_writel(test, PCI_ENDPOINT_TEST_UPPER_DST_ADDR,
+                                upper_32_bits(phys_addr));
+    }
+
+    pci_endpoint_test_writel(test, PCI_ENDPOINT_TEST_SIZE, size);
+    pci_endpoint_test_writel(test, PCI_ENDPOINT_TEST_FLAGS, flags);
+    pci_endpoint_test_writel(test, PCI_ENDPOINT_TEST_IRQ_TYPE, irq_type);
+    pci_endpoint_test_writel(test, PCI_ENDPOINT_TEST_IRQ_NUMBER, 1);
+
+    // if we ask rc to write to ep, then ep should do read operation, and vice versa.
+    pci_endpoint_test_writel(test, PCI_ENDPOINT_TEST_COMMAND,
+                 operation == WRITE ? COMMAND_READ : COMMAND_WRITE);
+
+    wait_for_completion(&test->irq_raised);
+
+    dma_unmap_single(dev, orig_phys_addr, size + alignment,
+                     operation == WRITE ? DMA_TO_DEVICE : DMA_FROM_DEVICE);
+
+    if (operation == WRITE)
+        ret = pci_endpoint_test_readl(test, PCI_ENDPOINT_TEST_STATUS) & STATUS_READ_SUCCESS;
+    else
+        ret = crc32_le(~0, addr, size) ==
+            pci_endpoint_test_readl(test, PCI_ENDPOINT_TEST_CHECKSUM);
+
+err_phys_addr:
+    kfree(orig_addr);
+
+err:
+    return ret;
+}

static inline void pci_endpoint_test_bar_writel(struct pci_endpoint_test *test,
                        int bar, u32 offset, u32 value)
@@ -234,8 +338,8 @@ static bool pci_endpoint_test_request_irq(struct pci_endpoint_test *test)

    for (i = 0; i < test->num_irqs; i++) {
        err = devm_request_irq(dev, pci_irq_vector(pdev, i),
-                 pci_endpoint_test_irqhandler,
-                 IRQF_SHARED, test->name, test);
+                     pci_endpoint_test_irqhandler,
+                     IRQF_SHARED, test->name, test);
        if (err)
            goto fail;
    }
@@ -309,7 +413,7 @@ static bool pci_endpoint_test_legacy_irq(struct pci_endpoint_test *test)
}

static bool pci_endpoint_test_msi_irq(struct pci_endpoint_test *test,
-                 u16 msi_num, bool msix)
+                     u16 msi_num, bool msix)
{
    u32 val;
    struct pci_dev *pdev = test->pdev;
@@ -385,7 +489,7 @@ static bool pci_endpoint_test_copy(struct pci_endpoint_test *test,

    get_random_bytes(orig_src_addr, size + alignment);
    orig_src_phys_addr = dma_map_single(dev, orig_src_addr,
-                     size + alignment, DMA_TO_DEVICE);
+                        size + alignment, DMA_TO_DEVICE);
    if (dma_mapping_error(dev, orig_src_phys_addr)) {
        dev_err(dev, "failed to map source buffer address\n");
        ret = false;
@@ -417,7 +521,7 @@ static bool pci_endpoint_test_copy(struct pci_endpoint_test *test,
    }

    orig_dst_phys_addr = dma_map_single(dev, orig_dst_addr,
-                     size + alignment, DMA_FROM_DEVICE);
+                        size + alignment, DMA_FROM_DEVICE);
    if (dma_mapping_error(dev, orig_dst_phys_addr)) {
        dev_err(dev, "failed to map destination buffer address\n");
        ret = false;
@@ -471,193 +575,15 @@ static bool pci_endpoint_test_copy(struct pci_endpoint_test *test,
}

static bool pci_endpoint_test_write(struct pci_endpoint_test *test,
-                 unsigned long arg)
+                    unsigned long arg)
{
-    struct pci_endpoint_test_xfer_param param;
-    bool ret = false;
-    u32 flags = 0;
-    bool use_dma;
-    u32 reg;
-    void *addr;
-    dma_addr_t phys_addr;
-    struct pci_dev *pdev = test->pdev;
-    struct device *dev = &pdev->dev;
-    void *orig_addr;
-    dma_addr_t orig_phys_addr;
-    size_t offset;
-    size_t alignment = test->alignment;
-    int irq_type = test->irq_type;
-    size_t size;
-    u32 crc32;
-    int err;
-
-    err = copy_from_user(&param, (void __user *)arg, sizeof(param));
-    if (err != 0) {
-        dev_err(dev, "Failed to get transfer param\n");
-        return false;
-    }
-
-    size = param.size;
-    if (size > SIZE_MAX - alignment)
-        goto err;
-
-    use_dma = !!(param.flags & PCITEST_FLAGS_USE_DMA);
-    if (use_dma)
-        flags |= FLAG_USE_DMA;
-
-    if (irq_type < IRQ_TYPE_LEGACY || irq_type > IRQ_TYPE_MSIX) {
-        dev_err(dev, "Invalid IRQ type option\n");
-        goto err;
-    }
-
-    orig_addr = kzalloc(size + alignment, GFP_KERNEL);
-    if (!orig_addr) {
-        dev_err(dev, "Failed to allocate address\n");
-        ret = false;
-        goto err;
-    }
-
-    get_random_bytes(orig_addr, size + alignment);
-
-    orig_phys_addr = dma_map_single(dev, orig_addr, size + alignment,
-                    DMA_TO_DEVICE);
-    if (dma_mapping_error(dev, orig_phys_addr)) {
-        dev_err(dev, "failed to map source buffer address\n");
-        ret = false;
-        goto err_phys_addr;
-    }
-
-    if (alignment && !IS_ALIGNED(orig_phys_addr, alignment)) {
-        phys_addr = PTR_ALIGN(orig_phys_addr, alignment);
-        offset = phys_addr - orig_phys_addr;
-        addr = orig_addr + offset;
-    } else {
-        phys_addr = orig_phys_addr;
-        addr = orig_addr;
-    }
-
-    crc32 = crc32_le(~0, addr, size);
-    pci_endpoint_test_writel(test, PCI_ENDPOINT_TEST_CHECKSUM,
-                 crc32);
-
-    pci_endpoint_test_writel(test, PCI_ENDPOINT_TEST_LOWER_SRC_ADDR,
-                 lower_32_bits(phys_addr));
-    pci_endpoint_test_writel(test, PCI_ENDPOINT_TEST_UPPER_SRC_ADDR,
-                 upper_32_bits(phys_addr));
-
-    pci_endpoint_test_writel(test, PCI_ENDPOINT_TEST_SIZE, size);
-
-    pci_endpoint_test_writel(test, PCI_ENDPOINT_TEST_FLAGS, flags);
-    pci_endpoint_test_writel(test, PCI_ENDPOINT_TEST_IRQ_TYPE, irq_type);
-    pci_endpoint_test_writel(test, PCI_ENDPOINT_TEST_IRQ_NUMBER, 1);
-    pci_endpoint_test_writel(test, PCI_ENDPOINT_TEST_COMMAND,
-                 COMMAND_READ);
-
-    wait_for_completion(&test->irq_raised);
-
-    reg = pci_endpoint_test_readl(test, PCI_ENDPOINT_TEST_STATUS);
-    if (reg & STATUS_READ_SUCCESS)
-        ret = true;
-
-    dma_unmap_single(dev, orig_phys_addr, size + alignment,
-             DMA_TO_DEVICE);
-
-err_phys_addr:
-    kfree(orig_addr);
-
-err:
-    return ret;
+    return pci_endpoint_test_transfer_data(test, arg, WRITE);
}

static bool pci_endpoint_test_read(struct pci_endpoint_test *test,
                 unsigned long arg)
{
-    struct pci_endpoint_test_xfer_param param;
-    bool ret = false;
-    u32 flags = 0;
-    bool use_dma;
-    size_t size;
-    void *addr;
-    dma_addr_t phys_addr;
-    struct pci_dev *pdev = test->pdev;
-    struct device *dev = &pdev->dev;
-    void *orig_addr;
-    dma_addr_t orig_phys_addr;
-    size_t offset;
-    size_t alignment = test->alignment;
-    int irq_type = test->irq_type;
-    u32 crc32;
-    int err;
-
-    err = copy_from_user(&param, (void __user *)arg, sizeof(param));
-    if (err) {
-        dev_err(dev, "Failed to get transfer param\n");
-        return false;
-    }
-
-    size = param.size;
-    if (size > SIZE_MAX - alignment)
-        goto err;
-
-    use_dma = !!(param.flags & PCITEST_FLAGS_USE_DMA);
-    if (use_dma)
-        flags |= FLAG_USE_DMA;
-
-    if (irq_type < IRQ_TYPE_LEGACY || irq_type > IRQ_TYPE_MSIX) {
-        dev_err(dev, "Invalid IRQ type option\n");
-        goto err;
-    }
-
-    orig_addr = kzalloc(size + alignment, GFP_KERNEL);
-    if (!orig_addr) {
-        dev_err(dev, "Failed to allocate destination address\n");
-        ret = false;
-        goto err;
-    }
-
-    orig_phys_addr = dma_map_single(dev, orig_addr, size + alignment,
-                    DMA_FROM_DEVICE);
-    if (dma_mapping_error(dev, orig_phys_addr)) {
-        dev_err(dev, "failed to map source buffer address\n");
-        ret = false;
-        goto err_phys_addr;
-    }
-
-    if (alignment && !IS_ALIGNED(orig_phys_addr, alignment)) {
-        phys_addr = PTR_ALIGN(orig_phys_addr, alignment);
-        offset = phys_addr - orig_phys_addr;
-        addr = orig_addr + offset;
-    } else {
-        phys_addr = orig_phys_addr;
-        addr = orig_addr;
-    }
-
-    pci_endpoint_test_writel(test, PCI_ENDPOINT_TEST_LOWER_DST_ADDR,
-                 lower_32_bits(phys_addr));
-    pci_endpoint_test_writel(test, PCI_ENDPOINT_TEST_UPPER_DST_ADDR,
-                 upper_32_bits(phys_addr));
-
-    pci_endpoint_test_writel(test, PCI_ENDPOINT_TEST_SIZE, size);
-
-    pci_endpoint_test_writel(test, PCI_ENDPOINT_TEST_FLAGS, flags);
-    pci_endpoint_test_writel(test, PCI_ENDPOINT_TEST_IRQ_TYPE, irq_type);
-    pci_endpoint_test_writel(test, PCI_ENDPOINT_TEST_IRQ_NUMBER, 1);
-    pci_endpoint_test_writel(test, PCI_ENDPOINT_TEST_COMMAND,
-                 COMMAND_WRITE);
-
-    wait_for_completion(&test->irq_raised);
-
-    dma_unmap_single(dev, orig_phys_addr, size + alignment,
-             DMA_FROM_DEVICE);
-
-    crc32 = crc32_le(~0, addr, size);
-    if (crc32 == pci_endpoint_test_readl(test, PCI_ENDPOINT_TEST_CHECKSUM))
-        ret = true;
-
-err_phys_addr:
-    kfree(orig_addr);
-err:
-    return ret;
+    return pci_endpoint_test_transfer_data(test, arg, READ);
}

static bool pci_endpoint_test_clear_irq(struct pci_endpoint_test *test)
@@ -668,7 +594,7 @@ static bool pci_endpoint_test_clear_irq(struct pci_endpoint_test *test)
}

static bool pci_endpoint_test_set_irq(struct pci_endpoint_test *test,
-                 int req_irq_type)
+                     int req_irq_type)
{
    struct pci_dev *pdev = test->pdev;
    struct device *dev = &pdev->dev;
@@ -698,7 +624,7 @@ static bool pci_endpoint_test_set_irq(struct pci_endpoint_test *test,
}

static long pci_endpoint_test_ioctl(struct file *file, unsigned int cmd,
-                 unsigned long arg)
+                    unsigned long arg)
{
    int ret = -EINVAL;
    enum pci_barno bar;
@@ -793,7 +719,7 @@ static int pci_endpoint_test_probe(struct pci_dev *pdev,
    mutex_init(&test->mutex);

    if ((dma_set_mask_and_coherent(&pdev->dev, DMA_BIT_MASK(48)) != 0) &&
-     dma_set_mask_and_coherent(&pdev->dev, DMA_BIT_MASK(32)) != 0) {
+        dma_set_mask_and_coherent(&pdev->dev, DMA_BIT_MASK(32)) != 0) {
        dev_err(dev, "Cannot set DMA mask\n");
        return -EINVAL;
    }
-- 
2.34.1


^ permalink raw reply related	[flat|nested] 14+ messages in thread

* Re: [PATCH V6] misc: pci_endpoint_test: simplify endpoint test read and write operations
  2022-02-07  9:09       ` [PATCH V6] " Li Chen
@ 2022-02-07 17:56         ` Bjorn Helgaas
  2022-02-08  4:20           ` Li Chen
  0 siblings, 1 reply; 14+ messages in thread
From: Bjorn Helgaas @ 2022-02-07 17:56 UTC (permalink / raw)
  To: Li Chen
  Cc: Greg Kroah-Hartman, Kishon Vijay Abraham I, Lorenzo Pieralisi,
	"Krzysztof Wilczyński",
	Arnd Bergmann, Bjorn Helgaas, linux-pci, linux-kernel

On Mon, Feb 07, 2022 at 04:09:05AM -0500, Li Chen wrote:
> From: Li Chen <lchen@ambarella.com>
> 
> Introduce pci_endpoint_epf_transfer_data to simplify
> read and write operations.
> 
> Also tabify this file.

Thanks for the patch.

This doesn't apply cleanly on v5.17-rc1.  Please make it apply cleanly
there or at least mention where it *does* apply.

Please separate the whitespace tabification changes and the
pci_endpoint_epf_transfer_data() changes into two separate patches.
When they're mixed together, it's harder to review the patch.

> #define to_endpoint_test(priv) container_of((priv), struct pci_endpoint_test, \
> -                     miscdev)
> +                        miscdev)

Always indent with tabs when possible:

  https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/Documentation/process/coding-style.rst?id=v5.16#n18

Hmm, coding-style.rst is unfortunately not very explicit about that.

But it's obvious from the existing code in this file that things
should not be indented four spaces, as you did in
pci_endpoint_test_transfer_data().

Your patch should match the style of the existing code.

> +static bool pci_endpoint_test_transfer_data(struct pci_endpoint_test *test,
> +                unsigned long arg, const int operation)
> +{
> +    struct pci_endpoint_test_xfer_param param;
> +    bool ret = false;
> +    u32 flags = 0;


> +    // if we ask rc to write to ep, then ep should do read operation, and vice versa.

Please use /* */ comments to match the prevailing kernel comment
style:

  https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/Documentation/process/coding-style.rst?id=v5.16#n598

And spell out or at least capitalize "RC" and "EP" since they're not
real words.

Bjorn

^ permalink raw reply	[flat|nested] 14+ messages in thread

* Re: [PATCH V6] misc: pci_endpoint_test: simplify endpoint test read and write operations
  2022-02-07 17:56         ` Bjorn Helgaas
@ 2022-02-08  4:20           ` Li Chen
  2022-02-08 15:21             ` Bjorn Helgaas
  0 siblings, 1 reply; 14+ messages in thread
From: Li Chen @ 2022-02-08  4:20 UTC (permalink / raw)
  To: Bjorn Helgaas
  Cc: Greg Kroah-Hartman, Kishon Vijay Abraham I, Lorenzo Pieralisi,
	"Krzysztof Wilczyński",
	Arnd Bergmann, Bjorn Helgaas, linux-pci, linux-kernel

 ---- On Mon, 07 Feb 2022 12:56:13 -0500 Bjorn Helgaas <helgaas@kernel.org> wrote ----
 > On Mon, Feb 07, 2022 at 04:09:05AM -0500, Li Chen wrote:
 > > From: Li Chen <lchen@ambarella.com>
 > > 
 > > Introduce pci_endpoint_epf_transfer_data to simplify
 > > read and write operations.
 > > 
 > > Also tabify this file.
 > 
 > Thanks for the patch.
 > 
 > This doesn't apply cleanly on v5.17-rc1.  Please make it apply cleanly
 > there or at least mention where it *does* apply.

Ok, will send v7 which will be based on v5.17-rc3.

 > 
 > Please separate the whitespace tabification changes and the
 > pci_endpoint_epf_transfer_data() changes into two separate patches.
 > When they're mixed together, it's harder to review the patch.
 > 
 > > #define to_endpoint_test(priv) container_of((priv), struct pci_endpoint_test, \
 > > -                     miscdev)
 > > +                        miscdev)
 > 
 > Always indent with tabs when possible:
 > 
 >   https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/Documentation/process/coding-style.rst?id=v5.16#n18
 > 
 > Hmm, coding-style.rst is unfortunately not very explicit about that.
 > 
 > But it's obvious from the existing code in this file that things
 > should not be indented four spaces, as you did in
 > pci_endpoint_test_transfer_data().

So, can I say space is not allowed in indentation? If so, I should not use emacs's tabify, because it will not convert 4 space to 8-width tab. I'm also not sure is scripts/Lindent or clang-format is a good
choice here, they do too much changes.

 > 
 > Your patch should match the style of the existing code.
 > 
 > > +static bool pci_endpoint_test_transfer_data(struct pci_endpoint_test *test,
 > > +                unsigned long arg, const int operation)
 > > +{
 > > +    struct pci_endpoint_test_xfer_param param;
 > > +    bool ret = false;
 > > +    u32 flags = 0;
 > 
 > 
 > > +    // if we ask rc to write to ep, then ep should do read operation, and vice versa.
 > 
 > Please use /* */ comments to match the prevailing kernel comment
 > style:
 > 
 >   https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/Documentation/process/coding-style.rst?id=v5.16#n598
 > 
 > And spell out or at least capitalize "RC" and "EP" since they're not
 > real words.
 > 

Thanks, will do it in v7.

 > Bjorn
 > 

Regards,
Li

^ permalink raw reply	[flat|nested] 14+ messages in thread

* Re: [PATCH V6] misc: pci_endpoint_test: simplify endpoint test read and write operations
  2022-02-08  4:20           ` Li Chen
@ 2022-02-08 15:21             ` Bjorn Helgaas
  2022-02-09  2:42               ` Li Chen
  2022-02-09  2:49               ` [PATCH V7] " Li Chen
  0 siblings, 2 replies; 14+ messages in thread
From: Bjorn Helgaas @ 2022-02-08 15:21 UTC (permalink / raw)
  To: Li Chen
  Cc: Greg Kroah-Hartman, Kishon Vijay Abraham I, Lorenzo Pieralisi,
	"Krzysztof Wilczyński",
	Arnd Bergmann, Bjorn Helgaas, linux-pci, linux-kernel

On Mon, Feb 07, 2022 at 11:20:15PM -0500, Li Chen wrote:
>  ---- On Mon, 07 Feb 2022 12:56:13 -0500 Bjorn Helgaas <helgaas@kernel.org> wrote ----
>  > On Mon, Feb 07, 2022 at 04:09:05AM -0500, Li Chen wrote:
>  > > #define to_endpoint_test(priv) container_of((priv), struct pci_endpoint_test, \
>  > > -                     miscdev)
>  > > +                        miscdev)
>  > 
>  > Always indent with tabs when possible:
>  > 
>  >   https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/Documentation/process/coding-style.rst?id=v5.16#n18
>  > 
>  > Hmm, coding-style.rst is unfortunately not very explicit about that.
>  > 
>  > But it's obvious from the existing code in this file that things
>  > should not be indented four spaces, as you did in
>  > pci_endpoint_test_transfer_data().
> 
> So, can I say space is not allowed in indentation? If so, I should
> not use emacs's tabify, because it will not convert 4 space to
> 8-width tab. I'm also not sure is scripts/Lindent or clang-format is
> a good choice here, they do too much changes.

I don't know how emacs, Lindent, clang-format, etc work.  I *can* tell
you that in Linux code:

  - indents always use tabs (width 8) when possible,

  - up to 7 spaces are allowed after a tab when needed to align
    something with the previous line, and

  - a tab never directly follows a space.

Bjorn

^ permalink raw reply	[flat|nested] 14+ messages in thread

* Re: [PATCH V6] misc: pci_endpoint_test: simplify endpoint test read and write operations
  2022-02-08 15:21             ` Bjorn Helgaas
@ 2022-02-09  2:42               ` Li Chen
  2022-02-09  2:49               ` [PATCH V7] " Li Chen
  1 sibling, 0 replies; 14+ messages in thread
From: Li Chen @ 2022-02-09  2:42 UTC (permalink / raw)
  To: Bjorn Helgaas
  Cc: Greg Kroah-Hartman, Kishon Vijay Abraham I, Lorenzo Pieralisi,
	"Krzysztof Wilczyński",
	Arnd Bergmann, Bjorn Helgaas, linux-pci, linux-kernel

Sent using Zoho Mail

 ---- On Tue, 08 Feb 2022 10:21:06 -0500 Bjorn Helgaas <helgaas@kernel.org> wrote ----
 > On Mon, Feb 07, 2022 at 11:20:15PM -0500, Li Chen wrote:
 > >  ---- On Mon, 07 Feb 2022 12:56:13 -0500 Bjorn Helgaas <helgaas@kernel.org> wrote ----
 > >  > On Mon, Feb 07, 2022 at 04:09:05AM -0500, Li Chen wrote:
 > >  > > #define to_endpoint_test(priv) container_of((priv), struct pci_endpoint_test, \
 > >  > > -                     miscdev)
 > >  > > +                        miscdev)
 > >  > 
 > >  > Always indent with tabs when possible:
 > >  > 
 > >  >   https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/Documentation/process/coding-style.rst?id=v5.16#n18
 > >  > 
 > >  > Hmm, coding-style.rst is unfortunately not very explicit about that.
 > >  > 
 > >  > But it's obvious from the existing code in this file that things
 > >  > should not be indented four spaces, as you did in
 > >  > pci_endpoint_test_transfer_data().
 > > 
 > > So, can I say space is not allowed in indentation? If so, I should
 > > not use emacs's tabify, because it will not convert 4 space to
 > > 8-width tab. I'm also not sure is scripts/Lindent or clang-format is
 > > a good choice here, they do too much changes.
 > 
 > I don't know how emacs, Lindent, clang-format, etc work.  I *can* tell
 > you that in Linux code:
 > 
 >   - indents always use tabs (width 8) when possible,
 > 
 >   - up to 7 spaces are allowed after a tab when needed to align
 >     something with the previous line, and
 > 
 >   - a tab never directly follows a space.
 > 
 > Bjorn
 > 

Thanks! I'll remove tabify operation in v7, the original code have no indentation code style issue.

Regards,
Li

^ permalink raw reply	[flat|nested] 14+ messages in thread

* [PATCH V7] misc: pci_endpoint_test: simplify endpoint test read and write operations
  2022-02-08 15:21             ` Bjorn Helgaas
  2022-02-09  2:42               ` Li Chen
@ 2022-02-09  2:49               ` Li Chen
  2022-05-11 16:52                 ` Lorenzo Pieralisi
  1 sibling, 1 reply; 14+ messages in thread
From: Li Chen @ 2022-02-09  2:49 UTC (permalink / raw)
  To: Bjorn Helgaas
  Cc: Greg Kroah-Hartman, Kishon Vijay Abraham I, Lorenzo Pieralisi,
	"Krzysztof Wilczyński",
	Arnd Bergmann, Bjorn Helgaas, linux-pci, linux-kernel

From: Li Chen <lchen@ambarella.com>

Introduce pci_endpoint_epf_transfer_data to simplify
read and write operations.

Signed-off-by: Li Chen <lchen@ambarella.com>
---
Changes in V2:
fix WARNING: line length of 108 exceeds 100 columns
#128: FILE: drivers/misc/pci_endpoint_test.c:243:
Changes in V3:
This patch context doesn't change but resend with my Zoho mail account in that previous
company mail will contain un-removeable proprietary messages.
Changes in V4:
Add "From:" to the first line of the message body.
Changes in V5:
tabify file
replace enum EPF_WRITE/EPF_READ with WRITE/READ from linux/kernel.h
get_random_bytes only when WRITE.
Changes in V6:
remove useless "Date:" and "Subject:" in message body, only preserve "From:" tag. 
Changes in V7:
use /* */ comments to match the prevailing kernel comment style
capitalize "RC" and "EP" since they're not real words.
remove tabify in that the original code have no style issue if tab is 8-width.

 drivers/misc/pci_endpoint_test.c | 286 ++++++++++++-------------------
 1 file changed, 106 insertions(+), 180 deletions(-)

diff --git a/drivers/misc/pci_endpoint_test.c b/drivers/misc/pci_endpoint_test.c
index 8f786a225dcf8..30adf0e4c36a0 100644
--- a/drivers/misc/pci_endpoint_test.c
+++ b/drivers/misc/pci_endpoint_test.c
@@ -12,6 +12,7 @@
 #include <linux/io.h>
 #include <linux/interrupt.h>
 #include <linux/irq.h>
+#include <linux/kernel.h>
 #include <linux/miscdevice.h>
 #include <linux/module.h>
 #include <linux/mutex.h>
@@ -142,6 +143,109 @@ static inline u32 pci_endpoint_test_bar_readl(struct pci_endpoint_test *test,
 {
 	return readl(test->bar[bar] + offset);
 }
+static bool pci_endpoint_test_transfer_data(struct pci_endpoint_test *test,
+				unsigned long arg, const int operation)
+{
+	struct pci_endpoint_test_xfer_param param;
+	bool ret = false;
+	u32 flags = 0;
+	bool use_dma;
+	void *addr;
+	dma_addr_t phys_addr;
+	struct pci_dev *pdev = test->pdev;
+	struct device *dev = &pdev->dev;
+	void *orig_addr;
+	dma_addr_t orig_phys_addr;
+	size_t offset;
+	size_t alignment = test->alignment;
+	int irq_type = test->irq_type;
+	size_t size;
+	int err;
+
+	err = copy_from_user(&param, (void __user *)arg, sizeof(param));
+	if (err != 0) {
+		dev_err(dev, "Failed to get transfer param\n");
+		return false;
+	}
+
+	size = param.size;
+	if (size > SIZE_MAX - alignment)
+		goto err;
+
+	use_dma = !!(param.flags & PCITEST_FLAGS_USE_DMA);
+	if (use_dma)
+		flags |= FLAG_USE_DMA;
+
+	if (irq_type < IRQ_TYPE_LEGACY || irq_type > IRQ_TYPE_MSIX) {
+		dev_err(dev, "Invalid IRQ type option\n");
+		goto err;
+	}
+
+	orig_addr = kzalloc(size + alignment, GFP_KERNEL);
+	if (!orig_addr)
+		goto err;
+
+	if (operation == WRITE)
+		get_random_bytes(orig_addr, size + alignment);
+
+	orig_phys_addr = dma_map_single(dev, orig_addr, size + alignment,
+					operation == WRITE ? DMA_TO_DEVICE : DMA_FROM_DEVICE);
+	if (dma_mapping_error(dev, orig_phys_addr)) {
+		dev_err(dev, "failed to map source buffer address\n");
+		goto err_phys_addr;
+	}
+
+	if (alignment && !IS_ALIGNED(orig_phys_addr, alignment)) {
+		phys_addr = PTR_ALIGN(orig_phys_addr, alignment);
+		offset = phys_addr - orig_phys_addr;
+		addr = orig_addr + offset;
+	} else {
+		phys_addr = orig_phys_addr;
+		addr = orig_addr;
+	}
+
+	if (operation == WRITE) {
+
+		pci_endpoint_test_writel(test, PCI_ENDPOINT_TEST_CHECKSUM,
+				 crc32_le(~0, addr, size));
+
+		pci_endpoint_test_writel(test, PCI_ENDPOINT_TEST_LOWER_SRC_ADDR,
+								lower_32_bits(phys_addr));
+		pci_endpoint_test_writel(test, PCI_ENDPOINT_TEST_UPPER_SRC_ADDR,
+								upper_32_bits(phys_addr));
+	} else {
+		pci_endpoint_test_writel(test, PCI_ENDPOINT_TEST_LOWER_DST_ADDR,
+								lower_32_bits(phys_addr));
+		pci_endpoint_test_writel(test, PCI_ENDPOINT_TEST_UPPER_DST_ADDR,
+								upper_32_bits(phys_addr));
+	}
+
+	pci_endpoint_test_writel(test, PCI_ENDPOINT_TEST_SIZE, size);
+	pci_endpoint_test_writel(test, PCI_ENDPOINT_TEST_FLAGS, flags);
+	pci_endpoint_test_writel(test, PCI_ENDPOINT_TEST_IRQ_TYPE, irq_type);
+	pci_endpoint_test_writel(test, PCI_ENDPOINT_TEST_IRQ_NUMBER, 1);
+
+	/* if we ask RC to write to EP, then EP should do read operation, and vice versa. */
+	pci_endpoint_test_writel(test, PCI_ENDPOINT_TEST_COMMAND,
+				 operation == WRITE ? COMMAND_READ : COMMAND_WRITE);
+
+	wait_for_completion(&test->irq_raised);
+
+	dma_unmap_single(dev, orig_phys_addr, size + alignment,
+					 operation == WRITE ? DMA_TO_DEVICE : DMA_FROM_DEVICE);
+
+	if (operation == WRITE)
+		ret = pci_endpoint_test_readl(test, PCI_ENDPOINT_TEST_STATUS) & STATUS_READ_SUCCESS;
+	else
+		ret = crc32_le(~0, addr, size) ==
+			pci_endpoint_test_readl(test, PCI_ENDPOINT_TEST_CHECKSUM);
+
+err_phys_addr:
+	kfree(orig_addr);
+
+err:
+	return ret;
+}
 
 static inline void pci_endpoint_test_bar_writel(struct pci_endpoint_test *test,
 						int bar, u32 offset, u32 value)
@@ -473,191 +577,13 @@ static bool pci_endpoint_test_copy(struct pci_endpoint_test *test,
 static bool pci_endpoint_test_write(struct pci_endpoint_test *test,
 				    unsigned long arg)
 {
-	struct pci_endpoint_test_xfer_param param;
-	bool ret = false;
-	u32 flags = 0;
-	bool use_dma;
-	u32 reg;
-	void *addr;
-	dma_addr_t phys_addr;
-	struct pci_dev *pdev = test->pdev;
-	struct device *dev = &pdev->dev;
-	void *orig_addr;
-	dma_addr_t orig_phys_addr;
-	size_t offset;
-	size_t alignment = test->alignment;
-	int irq_type = test->irq_type;
-	size_t size;
-	u32 crc32;
-	int err;
-
-	err = copy_from_user(&param, (void __user *)arg, sizeof(param));
-	if (err != 0) {
-		dev_err(dev, "Failed to get transfer param\n");
-		return false;
-	}
-
-	size = param.size;
-	if (size > SIZE_MAX - alignment)
-		goto err;
-
-	use_dma = !!(param.flags & PCITEST_FLAGS_USE_DMA);
-	if (use_dma)
-		flags |= FLAG_USE_DMA;
-
-	if (irq_type < IRQ_TYPE_LEGACY || irq_type > IRQ_TYPE_MSIX) {
-		dev_err(dev, "Invalid IRQ type option\n");
-		goto err;
-	}
-
-	orig_addr = kzalloc(size + alignment, GFP_KERNEL);
-	if (!orig_addr) {
-		dev_err(dev, "Failed to allocate address\n");
-		ret = false;
-		goto err;
-	}
-
-	get_random_bytes(orig_addr, size + alignment);
-
-	orig_phys_addr = dma_map_single(dev, orig_addr, size + alignment,
-					DMA_TO_DEVICE);
-	if (dma_mapping_error(dev, orig_phys_addr)) {
-		dev_err(dev, "failed to map source buffer address\n");
-		ret = false;
-		goto err_phys_addr;
-	}
-
-	if (alignment && !IS_ALIGNED(orig_phys_addr, alignment)) {
-		phys_addr =  PTR_ALIGN(orig_phys_addr, alignment);
-		offset = phys_addr - orig_phys_addr;
-		addr = orig_addr + offset;
-	} else {
-		phys_addr = orig_phys_addr;
-		addr = orig_addr;
-	}
-
-	crc32 = crc32_le(~0, addr, size);
-	pci_endpoint_test_writel(test, PCI_ENDPOINT_TEST_CHECKSUM,
-				 crc32);
-
-	pci_endpoint_test_writel(test, PCI_ENDPOINT_TEST_LOWER_SRC_ADDR,
-				 lower_32_bits(phys_addr));
-	pci_endpoint_test_writel(test, PCI_ENDPOINT_TEST_UPPER_SRC_ADDR,
-				 upper_32_bits(phys_addr));
-
-	pci_endpoint_test_writel(test, PCI_ENDPOINT_TEST_SIZE, size);
-
-	pci_endpoint_test_writel(test, PCI_ENDPOINT_TEST_FLAGS, flags);
-	pci_endpoint_test_writel(test, PCI_ENDPOINT_TEST_IRQ_TYPE, irq_type);
-	pci_endpoint_test_writel(test, PCI_ENDPOINT_TEST_IRQ_NUMBER, 1);
-	pci_endpoint_test_writel(test, PCI_ENDPOINT_TEST_COMMAND,
-				 COMMAND_READ);
-
-	wait_for_completion(&test->irq_raised);
-
-	reg = pci_endpoint_test_readl(test, PCI_ENDPOINT_TEST_STATUS);
-	if (reg & STATUS_READ_SUCCESS)
-		ret = true;
-
-	dma_unmap_single(dev, orig_phys_addr, size + alignment,
-			 DMA_TO_DEVICE);
-
-err_phys_addr:
-	kfree(orig_addr);
-
-err:
-	return ret;
+	return pci_endpoint_test_transfer_data(test, arg, WRITE);
 }
 
 static bool pci_endpoint_test_read(struct pci_endpoint_test *test,
 				   unsigned long arg)
 {
-	struct pci_endpoint_test_xfer_param param;
-	bool ret = false;
-	u32 flags = 0;
-	bool use_dma;
-	size_t size;
-	void *addr;
-	dma_addr_t phys_addr;
-	struct pci_dev *pdev = test->pdev;
-	struct device *dev = &pdev->dev;
-	void *orig_addr;
-	dma_addr_t orig_phys_addr;
-	size_t offset;
-	size_t alignment = test->alignment;
-	int irq_type = test->irq_type;
-	u32 crc32;
-	int err;
-
-	err = copy_from_user(&param, (void __user *)arg, sizeof(param));
-	if (err) {
-		dev_err(dev, "Failed to get transfer param\n");
-		return false;
-	}
-
-	size = param.size;
-	if (size > SIZE_MAX - alignment)
-		goto err;
-
-	use_dma = !!(param.flags & PCITEST_FLAGS_USE_DMA);
-	if (use_dma)
-		flags |= FLAG_USE_DMA;
-
-	if (irq_type < IRQ_TYPE_LEGACY || irq_type > IRQ_TYPE_MSIX) {
-		dev_err(dev, "Invalid IRQ type option\n");
-		goto err;
-	}
-
-	orig_addr = kzalloc(size + alignment, GFP_KERNEL);
-	if (!orig_addr) {
-		dev_err(dev, "Failed to allocate destination address\n");
-		ret = false;
-		goto err;
-	}
-
-	orig_phys_addr = dma_map_single(dev, orig_addr, size + alignment,
-					DMA_FROM_DEVICE);
-	if (dma_mapping_error(dev, orig_phys_addr)) {
-		dev_err(dev, "failed to map source buffer address\n");
-		ret = false;
-		goto err_phys_addr;
-	}
-
-	if (alignment && !IS_ALIGNED(orig_phys_addr, alignment)) {
-		phys_addr = PTR_ALIGN(orig_phys_addr, alignment);
-		offset = phys_addr - orig_phys_addr;
-		addr = orig_addr + offset;
-	} else {
-		phys_addr = orig_phys_addr;
-		addr = orig_addr;
-	}
-
-	pci_endpoint_test_writel(test, PCI_ENDPOINT_TEST_LOWER_DST_ADDR,
-				 lower_32_bits(phys_addr));
-	pci_endpoint_test_writel(test, PCI_ENDPOINT_TEST_UPPER_DST_ADDR,
-				 upper_32_bits(phys_addr));
-
-	pci_endpoint_test_writel(test, PCI_ENDPOINT_TEST_SIZE, size);
-
-	pci_endpoint_test_writel(test, PCI_ENDPOINT_TEST_FLAGS, flags);
-	pci_endpoint_test_writel(test, PCI_ENDPOINT_TEST_IRQ_TYPE, irq_type);
-	pci_endpoint_test_writel(test, PCI_ENDPOINT_TEST_IRQ_NUMBER, 1);
-	pci_endpoint_test_writel(test, PCI_ENDPOINT_TEST_COMMAND,
-				 COMMAND_WRITE);
-
-	wait_for_completion(&test->irq_raised);
-
-	dma_unmap_single(dev, orig_phys_addr, size + alignment,
-			 DMA_FROM_DEVICE);
-
-	crc32 = crc32_le(~0, addr, size);
-	if (crc32 == pci_endpoint_test_readl(test, PCI_ENDPOINT_TEST_CHECKSUM))
-		ret = true;
-
-err_phys_addr:
-	kfree(orig_addr);
-err:
-	return ret;
+	return pci_endpoint_test_transfer_data(test, arg, READ);
 }
 
 static bool pci_endpoint_test_clear_irq(struct pci_endpoint_test *test)
-- 
2.34.1



^ permalink raw reply related	[flat|nested] 14+ messages in thread

* Re: [PATCH V7] misc: pci_endpoint_test: simplify endpoint test read and write operations
  2022-02-09  2:49               ` [PATCH V7] " Li Chen
@ 2022-05-11 16:52                 ` Lorenzo Pieralisi
  2022-05-12 14:07                   ` Kishon Vijay Abraham I
  0 siblings, 1 reply; 14+ messages in thread
From: Lorenzo Pieralisi @ 2022-05-11 16:52 UTC (permalink / raw)
  To: Li Chen, kishon
  Cc: Bjorn Helgaas, Greg Kroah-Hartman,
	"Krzysztof Wilczyński",
	Arnd Bergmann, Bjorn Helgaas, linux-pci, linux-kernel

Hi Kishon,

On Tue, Feb 08, 2022 at 09:49:46PM -0500, Li Chen wrote:
> From: Li Chen <lchen@ambarella.com>
> 
> Introduce pci_endpoint_epf_transfer_data to simplify
> read and write operations.
> 
> Signed-off-by: Li Chen <lchen@ambarella.com>
> ---
> Changes in V2:
> fix WARNING: line length of 108 exceeds 100 columns
> #128: FILE: drivers/misc/pci_endpoint_test.c:243:
> Changes in V3:
> This patch context doesn't change but resend with my Zoho mail account in that previous
> company mail will contain un-removeable proprietary messages.
> Changes in V4:
> Add "From:" to the first line of the message body.
> Changes in V5:
> tabify file
> replace enum EPF_WRITE/EPF_READ with WRITE/READ from linux/kernel.h
> get_random_bytes only when WRITE.
> Changes in V6:
> remove useless "Date:" and "Subject:" in message body, only preserve "From:" tag. 
> Changes in V7:
> use /* */ comments to match the prevailing kernel comment style
> capitalize "RC" and "EP" since they're not real words.
> remove tabify in that the original code have no style issue if tab is 8-width.
> 
>  drivers/misc/pci_endpoint_test.c | 286 ++++++++++++-------------------
>  1 file changed, 106 insertions(+), 180 deletions(-)

Please review these changes when you can, thanks.

Lorenzo

> diff --git a/drivers/misc/pci_endpoint_test.c b/drivers/misc/pci_endpoint_test.c
> index 8f786a225dcf8..30adf0e4c36a0 100644
> --- a/drivers/misc/pci_endpoint_test.c
> +++ b/drivers/misc/pci_endpoint_test.c
> @@ -12,6 +12,7 @@
>  #include <linux/io.h>
>  #include <linux/interrupt.h>
>  #include <linux/irq.h>
> +#include <linux/kernel.h>
>  #include <linux/miscdevice.h>
>  #include <linux/module.h>
>  #include <linux/mutex.h>
> @@ -142,6 +143,109 @@ static inline u32 pci_endpoint_test_bar_readl(struct pci_endpoint_test *test,
>  {
>  	return readl(test->bar[bar] + offset);
>  }
> +static bool pci_endpoint_test_transfer_data(struct pci_endpoint_test *test,
> +				unsigned long arg, const int operation)
> +{
> +	struct pci_endpoint_test_xfer_param param;
> +	bool ret = false;
> +	u32 flags = 0;
> +	bool use_dma;
> +	void *addr;
> +	dma_addr_t phys_addr;
> +	struct pci_dev *pdev = test->pdev;
> +	struct device *dev = &pdev->dev;
> +	void *orig_addr;
> +	dma_addr_t orig_phys_addr;
> +	size_t offset;
> +	size_t alignment = test->alignment;
> +	int irq_type = test->irq_type;
> +	size_t size;
> +	int err;
> +
> +	err = copy_from_user(&param, (void __user *)arg, sizeof(param));
> +	if (err != 0) {
> +		dev_err(dev, "Failed to get transfer param\n");
> +		return false;
> +	}
> +
> +	size = param.size;
> +	if (size > SIZE_MAX - alignment)
> +		goto err;
> +
> +	use_dma = !!(param.flags & PCITEST_FLAGS_USE_DMA);
> +	if (use_dma)
> +		flags |= FLAG_USE_DMA;
> +
> +	if (irq_type < IRQ_TYPE_LEGACY || irq_type > IRQ_TYPE_MSIX) {
> +		dev_err(dev, "Invalid IRQ type option\n");
> +		goto err;
> +	}
> +
> +	orig_addr = kzalloc(size + alignment, GFP_KERNEL);
> +	if (!orig_addr)
> +		goto err;
> +
> +	if (operation == WRITE)
> +		get_random_bytes(orig_addr, size + alignment);
> +
> +	orig_phys_addr = dma_map_single(dev, orig_addr, size + alignment,
> +					operation == WRITE ? DMA_TO_DEVICE : DMA_FROM_DEVICE);
> +	if (dma_mapping_error(dev, orig_phys_addr)) {
> +		dev_err(dev, "failed to map source buffer address\n");
> +		goto err_phys_addr;
> +	}
> +
> +	if (alignment && !IS_ALIGNED(orig_phys_addr, alignment)) {
> +		phys_addr = PTR_ALIGN(orig_phys_addr, alignment);
> +		offset = phys_addr - orig_phys_addr;
> +		addr = orig_addr + offset;
> +	} else {
> +		phys_addr = orig_phys_addr;
> +		addr = orig_addr;
> +	}
> +
> +	if (operation == WRITE) {
> +
> +		pci_endpoint_test_writel(test, PCI_ENDPOINT_TEST_CHECKSUM,
> +				 crc32_le(~0, addr, size));
> +
> +		pci_endpoint_test_writel(test, PCI_ENDPOINT_TEST_LOWER_SRC_ADDR,
> +								lower_32_bits(phys_addr));
> +		pci_endpoint_test_writel(test, PCI_ENDPOINT_TEST_UPPER_SRC_ADDR,
> +								upper_32_bits(phys_addr));
> +	} else {
> +		pci_endpoint_test_writel(test, PCI_ENDPOINT_TEST_LOWER_DST_ADDR,
> +								lower_32_bits(phys_addr));
> +		pci_endpoint_test_writel(test, PCI_ENDPOINT_TEST_UPPER_DST_ADDR,
> +								upper_32_bits(phys_addr));
> +	}
> +
> +	pci_endpoint_test_writel(test, PCI_ENDPOINT_TEST_SIZE, size);
> +	pci_endpoint_test_writel(test, PCI_ENDPOINT_TEST_FLAGS, flags);
> +	pci_endpoint_test_writel(test, PCI_ENDPOINT_TEST_IRQ_TYPE, irq_type);
> +	pci_endpoint_test_writel(test, PCI_ENDPOINT_TEST_IRQ_NUMBER, 1);
> +
> +	/* if we ask RC to write to EP, then EP should do read operation, and vice versa. */
> +	pci_endpoint_test_writel(test, PCI_ENDPOINT_TEST_COMMAND,
> +				 operation == WRITE ? COMMAND_READ : COMMAND_WRITE);
> +
> +	wait_for_completion(&test->irq_raised);
> +
> +	dma_unmap_single(dev, orig_phys_addr, size + alignment,
> +					 operation == WRITE ? DMA_TO_DEVICE : DMA_FROM_DEVICE);
> +
> +	if (operation == WRITE)
> +		ret = pci_endpoint_test_readl(test, PCI_ENDPOINT_TEST_STATUS) & STATUS_READ_SUCCESS;
> +	else
> +		ret = crc32_le(~0, addr, size) ==
> +			pci_endpoint_test_readl(test, PCI_ENDPOINT_TEST_CHECKSUM);
> +
> +err_phys_addr:
> +	kfree(orig_addr);
> +
> +err:
> +	return ret;
> +}
>  
>  static inline void pci_endpoint_test_bar_writel(struct pci_endpoint_test *test,
>  						int bar, u32 offset, u32 value)
> @@ -473,191 +577,13 @@ static bool pci_endpoint_test_copy(struct pci_endpoint_test *test,
>  static bool pci_endpoint_test_write(struct pci_endpoint_test *test,
>  				    unsigned long arg)
>  {
> -	struct pci_endpoint_test_xfer_param param;
> -	bool ret = false;
> -	u32 flags = 0;
> -	bool use_dma;
> -	u32 reg;
> -	void *addr;
> -	dma_addr_t phys_addr;
> -	struct pci_dev *pdev = test->pdev;
> -	struct device *dev = &pdev->dev;
> -	void *orig_addr;
> -	dma_addr_t orig_phys_addr;
> -	size_t offset;
> -	size_t alignment = test->alignment;
> -	int irq_type = test->irq_type;
> -	size_t size;
> -	u32 crc32;
> -	int err;
> -
> -	err = copy_from_user(&param, (void __user *)arg, sizeof(param));
> -	if (err != 0) {
> -		dev_err(dev, "Failed to get transfer param\n");
> -		return false;
> -	}
> -
> -	size = param.size;
> -	if (size > SIZE_MAX - alignment)
> -		goto err;
> -
> -	use_dma = !!(param.flags & PCITEST_FLAGS_USE_DMA);
> -	if (use_dma)
> -		flags |= FLAG_USE_DMA;
> -
> -	if (irq_type < IRQ_TYPE_LEGACY || irq_type > IRQ_TYPE_MSIX) {
> -		dev_err(dev, "Invalid IRQ type option\n");
> -		goto err;
> -	}
> -
> -	orig_addr = kzalloc(size + alignment, GFP_KERNEL);
> -	if (!orig_addr) {
> -		dev_err(dev, "Failed to allocate address\n");
> -		ret = false;
> -		goto err;
> -	}
> -
> -	get_random_bytes(orig_addr, size + alignment);
> -
> -	orig_phys_addr = dma_map_single(dev, orig_addr, size + alignment,
> -					DMA_TO_DEVICE);
> -	if (dma_mapping_error(dev, orig_phys_addr)) {
> -		dev_err(dev, "failed to map source buffer address\n");
> -		ret = false;
> -		goto err_phys_addr;
> -	}
> -
> -	if (alignment && !IS_ALIGNED(orig_phys_addr, alignment)) {
> -		phys_addr =  PTR_ALIGN(orig_phys_addr, alignment);
> -		offset = phys_addr - orig_phys_addr;
> -		addr = orig_addr + offset;
> -	} else {
> -		phys_addr = orig_phys_addr;
> -		addr = orig_addr;
> -	}
> -
> -	crc32 = crc32_le(~0, addr, size);
> -	pci_endpoint_test_writel(test, PCI_ENDPOINT_TEST_CHECKSUM,
> -				 crc32);
> -
> -	pci_endpoint_test_writel(test, PCI_ENDPOINT_TEST_LOWER_SRC_ADDR,
> -				 lower_32_bits(phys_addr));
> -	pci_endpoint_test_writel(test, PCI_ENDPOINT_TEST_UPPER_SRC_ADDR,
> -				 upper_32_bits(phys_addr));
> -
> -	pci_endpoint_test_writel(test, PCI_ENDPOINT_TEST_SIZE, size);
> -
> -	pci_endpoint_test_writel(test, PCI_ENDPOINT_TEST_FLAGS, flags);
> -	pci_endpoint_test_writel(test, PCI_ENDPOINT_TEST_IRQ_TYPE, irq_type);
> -	pci_endpoint_test_writel(test, PCI_ENDPOINT_TEST_IRQ_NUMBER, 1);
> -	pci_endpoint_test_writel(test, PCI_ENDPOINT_TEST_COMMAND,
> -				 COMMAND_READ);
> -
> -	wait_for_completion(&test->irq_raised);
> -
> -	reg = pci_endpoint_test_readl(test, PCI_ENDPOINT_TEST_STATUS);
> -	if (reg & STATUS_READ_SUCCESS)
> -		ret = true;
> -
> -	dma_unmap_single(dev, orig_phys_addr, size + alignment,
> -			 DMA_TO_DEVICE);
> -
> -err_phys_addr:
> -	kfree(orig_addr);
> -
> -err:
> -	return ret;
> +	return pci_endpoint_test_transfer_data(test, arg, WRITE);
>  }
>  
>  static bool pci_endpoint_test_read(struct pci_endpoint_test *test,
>  				   unsigned long arg)
>  {
> -	struct pci_endpoint_test_xfer_param param;
> -	bool ret = false;
> -	u32 flags = 0;
> -	bool use_dma;
> -	size_t size;
> -	void *addr;
> -	dma_addr_t phys_addr;
> -	struct pci_dev *pdev = test->pdev;
> -	struct device *dev = &pdev->dev;
> -	void *orig_addr;
> -	dma_addr_t orig_phys_addr;
> -	size_t offset;
> -	size_t alignment = test->alignment;
> -	int irq_type = test->irq_type;
> -	u32 crc32;
> -	int err;
> -
> -	err = copy_from_user(&param, (void __user *)arg, sizeof(param));
> -	if (err) {
> -		dev_err(dev, "Failed to get transfer param\n");
> -		return false;
> -	}
> -
> -	size = param.size;
> -	if (size > SIZE_MAX - alignment)
> -		goto err;
> -
> -	use_dma = !!(param.flags & PCITEST_FLAGS_USE_DMA);
> -	if (use_dma)
> -		flags |= FLAG_USE_DMA;
> -
> -	if (irq_type < IRQ_TYPE_LEGACY || irq_type > IRQ_TYPE_MSIX) {
> -		dev_err(dev, "Invalid IRQ type option\n");
> -		goto err;
> -	}
> -
> -	orig_addr = kzalloc(size + alignment, GFP_KERNEL);
> -	if (!orig_addr) {
> -		dev_err(dev, "Failed to allocate destination address\n");
> -		ret = false;
> -		goto err;
> -	}
> -
> -	orig_phys_addr = dma_map_single(dev, orig_addr, size + alignment,
> -					DMA_FROM_DEVICE);
> -	if (dma_mapping_error(dev, orig_phys_addr)) {
> -		dev_err(dev, "failed to map source buffer address\n");
> -		ret = false;
> -		goto err_phys_addr;
> -	}
> -
> -	if (alignment && !IS_ALIGNED(orig_phys_addr, alignment)) {
> -		phys_addr = PTR_ALIGN(orig_phys_addr, alignment);
> -		offset = phys_addr - orig_phys_addr;
> -		addr = orig_addr + offset;
> -	} else {
> -		phys_addr = orig_phys_addr;
> -		addr = orig_addr;
> -	}
> -
> -	pci_endpoint_test_writel(test, PCI_ENDPOINT_TEST_LOWER_DST_ADDR,
> -				 lower_32_bits(phys_addr));
> -	pci_endpoint_test_writel(test, PCI_ENDPOINT_TEST_UPPER_DST_ADDR,
> -				 upper_32_bits(phys_addr));
> -
> -	pci_endpoint_test_writel(test, PCI_ENDPOINT_TEST_SIZE, size);
> -
> -	pci_endpoint_test_writel(test, PCI_ENDPOINT_TEST_FLAGS, flags);
> -	pci_endpoint_test_writel(test, PCI_ENDPOINT_TEST_IRQ_TYPE, irq_type);
> -	pci_endpoint_test_writel(test, PCI_ENDPOINT_TEST_IRQ_NUMBER, 1);
> -	pci_endpoint_test_writel(test, PCI_ENDPOINT_TEST_COMMAND,
> -				 COMMAND_WRITE);
> -
> -	wait_for_completion(&test->irq_raised);
> -
> -	dma_unmap_single(dev, orig_phys_addr, size + alignment,
> -			 DMA_FROM_DEVICE);
> -
> -	crc32 = crc32_le(~0, addr, size);
> -	if (crc32 == pci_endpoint_test_readl(test, PCI_ENDPOINT_TEST_CHECKSUM))
> -		ret = true;
> -
> -err_phys_addr:
> -	kfree(orig_addr);
> -err:
> -	return ret;
> +	return pci_endpoint_test_transfer_data(test, arg, READ);
>  }
>  
>  static bool pci_endpoint_test_clear_irq(struct pci_endpoint_test *test)
> -- 
> 2.34.1
> 
> 

^ permalink raw reply	[flat|nested] 14+ messages in thread

* Re: [PATCH V7] misc: pci_endpoint_test: simplify endpoint test read and write operations
  2022-05-11 16:52                 ` Lorenzo Pieralisi
@ 2022-05-12 14:07                   ` Kishon Vijay Abraham I
  0 siblings, 0 replies; 14+ messages in thread
From: Kishon Vijay Abraham I @ 2022-05-12 14:07 UTC (permalink / raw)
  To: Lorenzo Pieralisi, Li Chen
  Cc: Bjorn Helgaas, Greg Kroah-Hartman, Krzysztof Wilczyński,
	Arnd Bergmann, Bjorn Helgaas, linux-pci, linux-kernel

Hi Chen,

On 11/05/22 22:22, Lorenzo Pieralisi wrote:
> Hi Kishon,
> 
> On Tue, Feb 08, 2022 at 09:49:46PM -0500, Li Chen wrote:
>> From: Li Chen <lchen@ambarella.com>
>>
>> Introduce pci_endpoint_epf_transfer_data to simplify
>> read and write operations.
>>
>> Signed-off-by: Li Chen <lchen@ambarella.com>

Sorry for the delay in looking into this.
The read and write in it's current form is simple and easier to read.
Don't think it has to be disturbed IMO.

Thanks,
Kishon
>> ---
>> Changes in V2:
>> fix WARNING: line length of 108 exceeds 100 columns
>> #128: FILE: drivers/misc/pci_endpoint_test.c:243:
>> Changes in V3:
>> This patch context doesn't change but resend with my Zoho mail account in that previous
>> company mail will contain un-removeable proprietary messages.
>> Changes in V4:
>> Add "From:" to the first line of the message body.
>> Changes in V5:
>> tabify file
>> replace enum EPF_WRITE/EPF_READ with WRITE/READ from linux/kernel.h
>> get_random_bytes only when WRITE.
>> Changes in V6:
>> remove useless "Date:" and "Subject:" in message body, only preserve "From:" tag. 
>> Changes in V7:
>> use /* */ comments to match the prevailing kernel comment style
>> capitalize "RC" and "EP" since they're not real words.
>> remove tabify in that the original code have no style issue if tab is 8-width.
>>
>>  drivers/misc/pci_endpoint_test.c | 286 ++++++++++++-------------------
>>  1 file changed, 106 insertions(+), 180 deletions(-)
> 
> Please review these changes when you can, thanks.
> 
> Lorenzo
> 
>> diff --git a/drivers/misc/pci_endpoint_test.c b/drivers/misc/pci_endpoint_test.c
>> index 8f786a225dcf8..30adf0e4c36a0 100644
>> --- a/drivers/misc/pci_endpoint_test.c
>> +++ b/drivers/misc/pci_endpoint_test.c
>> @@ -12,6 +12,7 @@
>>  #include <linux/io.h>
>>  #include <linux/interrupt.h>
>>  #include <linux/irq.h>
>> +#include <linux/kernel.h>
>>  #include <linux/miscdevice.h>
>>  #include <linux/module.h>
>>  #include <linux/mutex.h>
>> @@ -142,6 +143,109 @@ static inline u32 pci_endpoint_test_bar_readl(struct pci_endpoint_test *test,
>>  {
>>  	return readl(test->bar[bar] + offset);
>>  }
>> +static bool pci_endpoint_test_transfer_data(struct pci_endpoint_test *test,
>> +				unsigned long arg, const int operation)
>> +{
>> +	struct pci_endpoint_test_xfer_param param;
>> +	bool ret = false;
>> +	u32 flags = 0;
>> +	bool use_dma;
>> +	void *addr;
>> +	dma_addr_t phys_addr;
>> +	struct pci_dev *pdev = test->pdev;
>> +	struct device *dev = &pdev->dev;
>> +	void *orig_addr;
>> +	dma_addr_t orig_phys_addr;
>> +	size_t offset;
>> +	size_t alignment = test->alignment;
>> +	int irq_type = test->irq_type;
>> +	size_t size;
>> +	int err;
>> +
>> +	err = copy_from_user(&param, (void __user *)arg, sizeof(param));
>> +	if (err != 0) {
>> +		dev_err(dev, "Failed to get transfer param\n");
>> +		return false;
>> +	}
>> +
>> +	size = param.size;
>> +	if (size > SIZE_MAX - alignment)
>> +		goto err;
>> +
>> +	use_dma = !!(param.flags & PCITEST_FLAGS_USE_DMA);
>> +	if (use_dma)
>> +		flags |= FLAG_USE_DMA;
>> +
>> +	if (irq_type < IRQ_TYPE_LEGACY || irq_type > IRQ_TYPE_MSIX) {
>> +		dev_err(dev, "Invalid IRQ type option\n");
>> +		goto err;
>> +	}
>> +
>> +	orig_addr = kzalloc(size + alignment, GFP_KERNEL);
>> +	if (!orig_addr)
>> +		goto err;
>> +
>> +	if (operation == WRITE)
>> +		get_random_bytes(orig_addr, size + alignment);
>> +
>> +	orig_phys_addr = dma_map_single(dev, orig_addr, size + alignment,
>> +					operation == WRITE ? DMA_TO_DEVICE : DMA_FROM_DEVICE);
>> +	if (dma_mapping_error(dev, orig_phys_addr)) {
>> +		dev_err(dev, "failed to map source buffer address\n");
>> +		goto err_phys_addr;
>> +	}
>> +
>> +	if (alignment && !IS_ALIGNED(orig_phys_addr, alignment)) {
>> +		phys_addr = PTR_ALIGN(orig_phys_addr, alignment);
>> +		offset = phys_addr - orig_phys_addr;
>> +		addr = orig_addr + offset;
>> +	} else {
>> +		phys_addr = orig_phys_addr;
>> +		addr = orig_addr;
>> +	}
>> +
>> +	if (operation == WRITE) {
>> +
>> +		pci_endpoint_test_writel(test, PCI_ENDPOINT_TEST_CHECKSUM,
>> +				 crc32_le(~0, addr, size));
>> +
>> +		pci_endpoint_test_writel(test, PCI_ENDPOINT_TEST_LOWER_SRC_ADDR,
>> +								lower_32_bits(phys_addr));
>> +		pci_endpoint_test_writel(test, PCI_ENDPOINT_TEST_UPPER_SRC_ADDR,
>> +								upper_32_bits(phys_addr));
>> +	} else {
>> +		pci_endpoint_test_writel(test, PCI_ENDPOINT_TEST_LOWER_DST_ADDR,
>> +								lower_32_bits(phys_addr));
>> +		pci_endpoint_test_writel(test, PCI_ENDPOINT_TEST_UPPER_DST_ADDR,
>> +								upper_32_bits(phys_addr));
>> +	}
>> +
>> +	pci_endpoint_test_writel(test, PCI_ENDPOINT_TEST_SIZE, size);
>> +	pci_endpoint_test_writel(test, PCI_ENDPOINT_TEST_FLAGS, flags);
>> +	pci_endpoint_test_writel(test, PCI_ENDPOINT_TEST_IRQ_TYPE, irq_type);
>> +	pci_endpoint_test_writel(test, PCI_ENDPOINT_TEST_IRQ_NUMBER, 1);
>> +
>> +	/* if we ask RC to write to EP, then EP should do read operation, and vice versa. */
>> +	pci_endpoint_test_writel(test, PCI_ENDPOINT_TEST_COMMAND,
>> +				 operation == WRITE ? COMMAND_READ : COMMAND_WRITE);
>> +
>> +	wait_for_completion(&test->irq_raised);
>> +
>> +	dma_unmap_single(dev, orig_phys_addr, size + alignment,
>> +					 operation == WRITE ? DMA_TO_DEVICE : DMA_FROM_DEVICE);
>> +
>> +	if (operation == WRITE)
>> +		ret = pci_endpoint_test_readl(test, PCI_ENDPOINT_TEST_STATUS) & STATUS_READ_SUCCESS;
>> +	else
>> +		ret = crc32_le(~0, addr, size) ==
>> +			pci_endpoint_test_readl(test, PCI_ENDPOINT_TEST_CHECKSUM);
>> +
>> +err_phys_addr:
>> +	kfree(orig_addr);
>> +
>> +err:
>> +	return ret;
>> +}
>>  
>>  static inline void pci_endpoint_test_bar_writel(struct pci_endpoint_test *test,
>>  						int bar, u32 offset, u32 value)
>> @@ -473,191 +577,13 @@ static bool pci_endpoint_test_copy(struct pci_endpoint_test *test,
>>  static bool pci_endpoint_test_write(struct pci_endpoint_test *test,
>>  				    unsigned long arg)
>>  {
>> -	struct pci_endpoint_test_xfer_param param;
>> -	bool ret = false;
>> -	u32 flags = 0;
>> -	bool use_dma;
>> -	u32 reg;
>> -	void *addr;
>> -	dma_addr_t phys_addr;
>> -	struct pci_dev *pdev = test->pdev;
>> -	struct device *dev = &pdev->dev;
>> -	void *orig_addr;
>> -	dma_addr_t orig_phys_addr;
>> -	size_t offset;
>> -	size_t alignment = test->alignment;
>> -	int irq_type = test->irq_type;
>> -	size_t size;
>> -	u32 crc32;
>> -	int err;
>> -
>> -	err = copy_from_user(&param, (void __user *)arg, sizeof(param));
>> -	if (err != 0) {
>> -		dev_err(dev, "Failed to get transfer param\n");
>> -		return false;
>> -	}
>> -
>> -	size = param.size;
>> -	if (size > SIZE_MAX - alignment)
>> -		goto err;
>> -
>> -	use_dma = !!(param.flags & PCITEST_FLAGS_USE_DMA);
>> -	if (use_dma)
>> -		flags |= FLAG_USE_DMA;
>> -
>> -	if (irq_type < IRQ_TYPE_LEGACY || irq_type > IRQ_TYPE_MSIX) {
>> -		dev_err(dev, "Invalid IRQ type option\n");
>> -		goto err;
>> -	}
>> -
>> -	orig_addr = kzalloc(size + alignment, GFP_KERNEL);
>> -	if (!orig_addr) {
>> -		dev_err(dev, "Failed to allocate address\n");
>> -		ret = false;
>> -		goto err;
>> -	}
>> -
>> -	get_random_bytes(orig_addr, size + alignment);
>> -
>> -	orig_phys_addr = dma_map_single(dev, orig_addr, size + alignment,
>> -					DMA_TO_DEVICE);
>> -	if (dma_mapping_error(dev, orig_phys_addr)) {
>> -		dev_err(dev, "failed to map source buffer address\n");
>> -		ret = false;
>> -		goto err_phys_addr;
>> -	}
>> -
>> -	if (alignment && !IS_ALIGNED(orig_phys_addr, alignment)) {
>> -		phys_addr =  PTR_ALIGN(orig_phys_addr, alignment);
>> -		offset = phys_addr - orig_phys_addr;
>> -		addr = orig_addr + offset;
>> -	} else {
>> -		phys_addr = orig_phys_addr;
>> -		addr = orig_addr;
>> -	}
>> -
>> -	crc32 = crc32_le(~0, addr, size);
>> -	pci_endpoint_test_writel(test, PCI_ENDPOINT_TEST_CHECKSUM,
>> -				 crc32);
>> -
>> -	pci_endpoint_test_writel(test, PCI_ENDPOINT_TEST_LOWER_SRC_ADDR,
>> -				 lower_32_bits(phys_addr));
>> -	pci_endpoint_test_writel(test, PCI_ENDPOINT_TEST_UPPER_SRC_ADDR,
>> -				 upper_32_bits(phys_addr));
>> -
>> -	pci_endpoint_test_writel(test, PCI_ENDPOINT_TEST_SIZE, size);
>> -
>> -	pci_endpoint_test_writel(test, PCI_ENDPOINT_TEST_FLAGS, flags);
>> -	pci_endpoint_test_writel(test, PCI_ENDPOINT_TEST_IRQ_TYPE, irq_type);
>> -	pci_endpoint_test_writel(test, PCI_ENDPOINT_TEST_IRQ_NUMBER, 1);
>> -	pci_endpoint_test_writel(test, PCI_ENDPOINT_TEST_COMMAND,
>> -				 COMMAND_READ);
>> -
>> -	wait_for_completion(&test->irq_raised);
>> -
>> -	reg = pci_endpoint_test_readl(test, PCI_ENDPOINT_TEST_STATUS);
>> -	if (reg & STATUS_READ_SUCCESS)
>> -		ret = true;
>> -
>> -	dma_unmap_single(dev, orig_phys_addr, size + alignment,
>> -			 DMA_TO_DEVICE);
>> -
>> -err_phys_addr:
>> -	kfree(orig_addr);
>> -
>> -err:
>> -	return ret;
>> +	return pci_endpoint_test_transfer_data(test, arg, WRITE);
>>  }
>>  
>>  static bool pci_endpoint_test_read(struct pci_endpoint_test *test,
>>  				   unsigned long arg)
>>  {
>> -	struct pci_endpoint_test_xfer_param param;
>> -	bool ret = false;
>> -	u32 flags = 0;
>> -	bool use_dma;
>> -	size_t size;
>> -	void *addr;
>> -	dma_addr_t phys_addr;
>> -	struct pci_dev *pdev = test->pdev;
>> -	struct device *dev = &pdev->dev;
>> -	void *orig_addr;
>> -	dma_addr_t orig_phys_addr;
>> -	size_t offset;
>> -	size_t alignment = test->alignment;
>> -	int irq_type = test->irq_type;
>> -	u32 crc32;
>> -	int err;
>> -
>> -	err = copy_from_user(&param, (void __user *)arg, sizeof(param));
>> -	if (err) {
>> -		dev_err(dev, "Failed to get transfer param\n");
>> -		return false;
>> -	}
>> -
>> -	size = param.size;
>> -	if (size > SIZE_MAX - alignment)
>> -		goto err;
>> -
>> -	use_dma = !!(param.flags & PCITEST_FLAGS_USE_DMA);
>> -	if (use_dma)
>> -		flags |= FLAG_USE_DMA;
>> -
>> -	if (irq_type < IRQ_TYPE_LEGACY || irq_type > IRQ_TYPE_MSIX) {
>> -		dev_err(dev, "Invalid IRQ type option\n");
>> -		goto err;
>> -	}
>> -
>> -	orig_addr = kzalloc(size + alignment, GFP_KERNEL);
>> -	if (!orig_addr) {
>> -		dev_err(dev, "Failed to allocate destination address\n");
>> -		ret = false;
>> -		goto err;
>> -	}
>> -
>> -	orig_phys_addr = dma_map_single(dev, orig_addr, size + alignment,
>> -					DMA_FROM_DEVICE);
>> -	if (dma_mapping_error(dev, orig_phys_addr)) {
>> -		dev_err(dev, "failed to map source buffer address\n");
>> -		ret = false;
>> -		goto err_phys_addr;
>> -	}
>> -
>> -	if (alignment && !IS_ALIGNED(orig_phys_addr, alignment)) {
>> -		phys_addr = PTR_ALIGN(orig_phys_addr, alignment);
>> -		offset = phys_addr - orig_phys_addr;
>> -		addr = orig_addr + offset;
>> -	} else {
>> -		phys_addr = orig_phys_addr;
>> -		addr = orig_addr;
>> -	}
>> -
>> -	pci_endpoint_test_writel(test, PCI_ENDPOINT_TEST_LOWER_DST_ADDR,
>> -				 lower_32_bits(phys_addr));
>> -	pci_endpoint_test_writel(test, PCI_ENDPOINT_TEST_UPPER_DST_ADDR,
>> -				 upper_32_bits(phys_addr));
>> -
>> -	pci_endpoint_test_writel(test, PCI_ENDPOINT_TEST_SIZE, size);
>> -
>> -	pci_endpoint_test_writel(test, PCI_ENDPOINT_TEST_FLAGS, flags);
>> -	pci_endpoint_test_writel(test, PCI_ENDPOINT_TEST_IRQ_TYPE, irq_type);
>> -	pci_endpoint_test_writel(test, PCI_ENDPOINT_TEST_IRQ_NUMBER, 1);
>> -	pci_endpoint_test_writel(test, PCI_ENDPOINT_TEST_COMMAND,
>> -				 COMMAND_WRITE);
>> -
>> -	wait_for_completion(&test->irq_raised);
>> -
>> -	dma_unmap_single(dev, orig_phys_addr, size + alignment,
>> -			 DMA_FROM_DEVICE);
>> -
>> -	crc32 = crc32_le(~0, addr, size);
>> -	if (crc32 == pci_endpoint_test_readl(test, PCI_ENDPOINT_TEST_CHECKSUM))
>> -		ret = true;
>> -
>> -err_phys_addr:
>> -	kfree(orig_addr);
>> -err:
>> -	return ret;
>> +	return pci_endpoint_test_transfer_data(test, arg, READ);
>>  }
>>  
>>  static bool pci_endpoint_test_clear_irq(struct pci_endpoint_test *test)
>> -- 
>> 2.34.1
>>
>>

^ permalink raw reply	[flat|nested] 14+ messages in thread

end of thread, other threads:[~2022-05-12 14:08 UTC | newest]

Thread overview: 14+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2022-01-20 10:11 [PATCH V3] misc: pci_endpoint_test: simplify endpoint test read and write operations Li Chen
2022-01-20 11:57 ` Greg Kroah-Hartman
2022-01-20 12:48   ` Li Chen
2022-01-20 12:47 ` [PATCH V4] " Li Chen
2022-01-26 18:07   ` Greg Kroah-Hartman
2022-02-07  8:42     ` [PATCH V5] " Li Chen
2022-02-07  9:09       ` [PATCH V6] " Li Chen
2022-02-07 17:56         ` Bjorn Helgaas
2022-02-08  4:20           ` Li Chen
2022-02-08 15:21             ` Bjorn Helgaas
2022-02-09  2:42               ` Li Chen
2022-02-09  2:49               ` [PATCH V7] " Li Chen
2022-05-11 16:52                 ` Lorenzo Pieralisi
2022-05-12 14:07                   ` Kishon Vijay Abraham I

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).