2022-02-08 19:34:06

by Bjorn Helgaas

[permalink] [raw]
Subject: Re: [PATCH V6] misc: pci_endpoint_test: simplify endpoint test read and write operations

On Mon, Feb 07, 2022 at 11:20:15PM -0500, Li Chen wrote:
> ---- On Mon, 07 Feb 2022 12:56:13 -0500 Bjorn Helgaas <[email protected]> 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


2022-02-09 10:12:20

by Li Chen

[permalink] [raw]
Subject: [PATCH V7] misc: pci_endpoint_test: simplify endpoint test read and write operations

From: Li Chen <[email protected]>

Introduce pci_endpoint_epf_transfer_data to simplify
read and write operations.

Signed-off-by: Li Chen <[email protected]>
---
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



2022-02-09 12:20:04

by Li Chen

[permalink] [raw]
Subject: Re: [PATCH V6] misc: pci_endpoint_test: simplify endpoint test read and write operations

Sent using Zoho Mail

---- On Tue, 08 Feb 2022 10:21:06 -0500 Bjorn Helgaas <[email protected]> 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 <[email protected]> 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

2022-05-13 13:05:44

by Kishon Vijay Abraham I

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

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 <[email protected]>
>>
>> Introduce pci_endpoint_epf_transfer_data to simplify
>> read and write operations.
>>
>> Signed-off-by: Li Chen <[email protected]>

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

2022-05-13 16:42:20

by Lorenzo Pieralisi

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

Hi Kishon,

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