Re: [PATCH V7] misc: pci_endpoint_test: simplify endpoint test read and write operations

From: Lorenzo Pieralisi
Date: Wed May 11 2022 - 12:52:57 EST

Hi Kishon,

On Tue, Feb 08, 2022 at 09:49:46PM -0500, Li Chen wrote:
> From: Li Chen <lchen@xxxxxxxxxxxxx>
>
> Introduce pci_endpoint_epf_transfer_data to simplify
> read and write operations.
>
> Signed-off-by: Li Chen <lchen@xxxxxxxxxxxxx>
> ---
> 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
>
>