From: Manivannan Sadhasivam <manivannan.sadhasivam@linaro.org>
To: Niklas Cassel <cassel@kernel.org>
Cc: "Krzysztof Wilczyński" <kw@linux.com>,
"Kishon Vijay Abraham I" <kishon@kernel.org>,
"Bjorn Helgaas" <bhelgaas@google.com>,
linux-pci@vger.kernel.org
Subject: Re: [PATCH] PCI: endpoint: pci-epf-test: Handle endianness properly
Date: Mon, 27 Jan 2025 11:56:47 +0530 [thread overview]
Message-ID: <20250127062647.ehadg6ji53hkbsbf@thinkpad> (raw)
In-Reply-To: <20250120115009.2748899-2-cassel@kernel.org>
On Mon, Jan 20, 2025 at 12:50:10PM +0100, Niklas Cassel wrote:
> The struct pci_epf_test_reg is the actual data in pci-epf-test's test_reg
> BAR (usually BAR0), which the host uses to send commands (etc.), and which
> pci-epf-test uses to send back status codes.
>
> pci-epf-test currently reads and writes this data without any endianness
> conversion functions, which means that pci-epf-test is completely broken
> on big-endian systems.
Not a big deal, but I'd like to mention 'big-endian endpoint systems' to clarify
the fact that the endianess issue is with the endpoint systems.
>
> PCI devices are inherently little-endian, and the data stored in the PCI
> BARs should be in little-endian.
>
> Use endianness conversion functions when reading and writing data to
> struct pci_epf_test_reg so that pci-epf-test will behave correctly on
> big-endian systems.
>
Same here.
> Signed-off-by: Niklas Cassel <cassel@kernel.org>
Fixes: 349e7a85b25f ("PCI: endpoint: functions: Add an EP function to test PCI")
Reviewed-by: Manivannan Sadhasivam <manivannan.sadhasivam@linaro.org>
No need to respin, these can be ammended while applying.
- Mani
> ---
> drivers/pci/endpoint/functions/pci-epf-test.c | 126 ++++++++++--------
> 1 file changed, 73 insertions(+), 53 deletions(-)
>
> diff --git a/drivers/pci/endpoint/functions/pci-epf-test.c b/drivers/pci/endpoint/functions/pci-epf-test.c
> index ffb534a8e50a..c1359f3662ae 100644
> --- a/drivers/pci/endpoint/functions/pci-epf-test.c
> +++ b/drivers/pci/endpoint/functions/pci-epf-test.c
> @@ -66,17 +66,17 @@ struct pci_epf_test {
> };
>
> struct pci_epf_test_reg {
> - u32 magic;
> - u32 command;
> - u32 status;
> - u64 src_addr;
> - u64 dst_addr;
> - u32 size;
> - u32 checksum;
> - u32 irq_type;
> - u32 irq_number;
> - u32 flags;
> - u32 caps;
> + __le32 magic;
> + __le32 command;
> + __le32 status;
> + __le64 src_addr;
> + __le64 dst_addr;
> + __le32 size;
> + __le32 checksum;
> + __le32 irq_type;
> + __le32 irq_number;
> + __le32 flags;
> + __le32 caps;
> } __packed;
>
> static struct pci_epf_header test_header = {
> @@ -324,13 +324,17 @@ static void pci_epf_test_copy(struct pci_epf_test *epf_test,
> struct pci_epc *epc = epf->epc;
> struct device *dev = &epf->dev;
> struct pci_epc_map src_map, dst_map;
> - u64 src_addr = reg->src_addr;
> - u64 dst_addr = reg->dst_addr;
> - size_t copy_size = reg->size;
> + u64 src_addr = le64_to_cpu(reg->src_addr);
> + u64 dst_addr = le64_to_cpu(reg->dst_addr);
> + size_t orig_size, copy_size;
> ssize_t map_size = 0;
> + u32 flags = le32_to_cpu(reg->flags);
> + u32 status = 0;
> void *copy_buf = NULL, *buf;
>
> - if (reg->flags & FLAG_USE_DMA) {
> + orig_size = copy_size = le32_to_cpu(reg->size);
> +
> + if (flags & FLAG_USE_DMA) {
> if (epf_test->dma_private) {
> dev_err(dev, "Cannot transfer data using DMA\n");
> ret = -EINVAL;
> @@ -350,7 +354,7 @@ static void pci_epf_test_copy(struct pci_epf_test *epf_test,
> src_addr, copy_size, &src_map);
> if (ret) {
> dev_err(dev, "Failed to map source address\n");
> - reg->status = STATUS_SRC_ADDR_INVALID;
> + status = STATUS_SRC_ADDR_INVALID;
> goto free_buf;
> }
>
> @@ -358,7 +362,7 @@ static void pci_epf_test_copy(struct pci_epf_test *epf_test,
> dst_addr, copy_size, &dst_map);
> if (ret) {
> dev_err(dev, "Failed to map destination address\n");
> - reg->status = STATUS_DST_ADDR_INVALID;
> + status = STATUS_DST_ADDR_INVALID;
> pci_epc_mem_unmap(epc, epf->func_no, epf->vfunc_no,
> &src_map);
> goto free_buf;
> @@ -367,7 +371,7 @@ static void pci_epf_test_copy(struct pci_epf_test *epf_test,
> map_size = min_t(size_t, dst_map.pci_size, src_map.pci_size);
>
> ktime_get_ts64(&start);
> - if (reg->flags & FLAG_USE_DMA) {
> + if (flags & FLAG_USE_DMA) {
> ret = pci_epf_test_data_transfer(epf_test,
> dst_map.phys_addr, src_map.phys_addr,
> map_size, 0, DMA_MEM_TO_MEM);
> @@ -391,8 +395,8 @@ static void pci_epf_test_copy(struct pci_epf_test *epf_test,
> map_size = 0;
> }
>
> - pci_epf_test_print_rate(epf_test, "COPY", reg->size, &start,
> - &end, reg->flags & FLAG_USE_DMA);
> + pci_epf_test_print_rate(epf_test, "COPY", orig_size, &start, &end,
> + flags & FLAG_USE_DMA);
>
> unmap:
> if (map_size) {
> @@ -405,9 +409,10 @@ static void pci_epf_test_copy(struct pci_epf_test *epf_test,
>
> set_status:
> if (!ret)
> - reg->status |= STATUS_COPY_SUCCESS;
> + status |= STATUS_COPY_SUCCESS;
> else
> - reg->status |= STATUS_COPY_FAIL;
> + status |= STATUS_COPY_FAIL;
> + reg->status = cpu_to_le32(status);
> }
>
> static void pci_epf_test_read(struct pci_epf_test *epf_test,
> @@ -423,9 +428,14 @@ static void pci_epf_test_read(struct pci_epf_test *epf_test,
> struct pci_epc *epc = epf->epc;
> struct device *dev = &epf->dev;
> struct device *dma_dev = epf->epc->dev.parent;
> - u64 src_addr = reg->src_addr;
> - size_t src_size = reg->size;
> + u64 src_addr = le64_to_cpu(reg->src_addr);
> + size_t orig_size, src_size;
> ssize_t map_size = 0;
> + u32 flags = le32_to_cpu(reg->flags);
> + u32 checksum = le32_to_cpu(reg->checksum);
> + u32 status = 0;
> +
> + orig_size = src_size = le32_to_cpu(reg->size);
>
> src_buf = kzalloc(src_size, GFP_KERNEL);
> if (!src_buf) {
> @@ -439,12 +449,12 @@ static void pci_epf_test_read(struct pci_epf_test *epf_test,
> src_addr, src_size, &map);
> if (ret) {
> dev_err(dev, "Failed to map address\n");
> - reg->status = STATUS_SRC_ADDR_INVALID;
> + status = STATUS_SRC_ADDR_INVALID;
> goto free_buf;
> }
>
> map_size = map.pci_size;
> - if (reg->flags & FLAG_USE_DMA) {
> + if (flags & FLAG_USE_DMA) {
> dst_phys_addr = dma_map_single(dma_dev, buf, map_size,
> DMA_FROM_DEVICE);
> if (dma_mapping_error(dma_dev, dst_phys_addr)) {
> @@ -481,11 +491,11 @@ static void pci_epf_test_read(struct pci_epf_test *epf_test,
> map_size = 0;
> }
>
> - pci_epf_test_print_rate(epf_test, "READ", reg->size, &start,
> - &end, reg->flags & FLAG_USE_DMA);
> + pci_epf_test_print_rate(epf_test, "READ", orig_size, &start, &end,
> + flags & FLAG_USE_DMA);
>
> - crc32 = crc32_le(~0, src_buf, reg->size);
> - if (crc32 != reg->checksum)
> + crc32 = crc32_le(~0, src_buf, orig_size);
> + if (crc32 != checksum)
> ret = -EIO;
>
> unmap:
> @@ -497,9 +507,10 @@ static void pci_epf_test_read(struct pci_epf_test *epf_test,
>
> set_status:
> if (!ret)
> - reg->status |= STATUS_READ_SUCCESS;
> + status |= STATUS_READ_SUCCESS;
> else
> - reg->status |= STATUS_READ_FAIL;
> + status |= STATUS_READ_FAIL;
> + reg->status = cpu_to_le32(status);
> }
>
> static void pci_epf_test_write(struct pci_epf_test *epf_test,
> @@ -514,9 +525,13 @@ static void pci_epf_test_write(struct pci_epf_test *epf_test,
> struct pci_epc *epc = epf->epc;
> struct device *dev = &epf->dev;
> struct device *dma_dev = epf->epc->dev.parent;
> - u64 dst_addr = reg->dst_addr;
> - size_t dst_size = reg->size;
> + u64 dst_addr = le64_to_cpu(reg->dst_addr);
> + size_t orig_size, dst_size;
> ssize_t map_size = 0;
> + u32 flags = le32_to_cpu(reg->flags);
> + u32 status = 0;
> +
> + orig_size = dst_size = le32_to_cpu(reg->size);
>
> dst_buf = kzalloc(dst_size, GFP_KERNEL);
> if (!dst_buf) {
> @@ -524,7 +539,7 @@ static void pci_epf_test_write(struct pci_epf_test *epf_test,
> goto set_status;
> }
> get_random_bytes(dst_buf, dst_size);
> - reg->checksum = crc32_le(~0, dst_buf, dst_size);
> + reg->checksum = cpu_to_le32(crc32_le(~0, dst_buf, dst_size));
> buf = dst_buf;
>
> while (dst_size) {
> @@ -532,12 +547,12 @@ static void pci_epf_test_write(struct pci_epf_test *epf_test,
> dst_addr, dst_size, &map);
> if (ret) {
> dev_err(dev, "Failed to map address\n");
> - reg->status = STATUS_DST_ADDR_INVALID;
> + status = STATUS_DST_ADDR_INVALID;
> goto free_buf;
> }
>
> map_size = map.pci_size;
> - if (reg->flags & FLAG_USE_DMA) {
> + if (flags & FLAG_USE_DMA) {
> src_phys_addr = dma_map_single(dma_dev, buf, map_size,
> DMA_TO_DEVICE);
> if (dma_mapping_error(dma_dev, src_phys_addr)) {
> @@ -576,8 +591,8 @@ static void pci_epf_test_write(struct pci_epf_test *epf_test,
> map_size = 0;
> }
>
> - pci_epf_test_print_rate(epf_test, "WRITE", reg->size, &start,
> - &end, reg->flags & FLAG_USE_DMA);
> + pci_epf_test_print_rate(epf_test, "WRITE", orig_size, &start, &end,
> + flags & FLAG_USE_DMA);
>
> /*
> * wait 1ms inorder for the write to complete. Without this delay L3
> @@ -594,9 +609,10 @@ static void pci_epf_test_write(struct pci_epf_test *epf_test,
>
> set_status:
> if (!ret)
> - reg->status |= STATUS_WRITE_SUCCESS;
> + status |= STATUS_WRITE_SUCCESS;
> else
> - reg->status |= STATUS_WRITE_FAIL;
> + status |= STATUS_WRITE_FAIL;
> + reg->status = cpu_to_le32(status);
> }
>
> static void pci_epf_test_raise_irq(struct pci_epf_test *epf_test,
> @@ -605,39 +621,42 @@ static void pci_epf_test_raise_irq(struct pci_epf_test *epf_test,
> struct pci_epf *epf = epf_test->epf;
> struct device *dev = &epf->dev;
> struct pci_epc *epc = epf->epc;
> - u32 status = reg->status | STATUS_IRQ_RAISED;
> + u32 status = le32_to_cpu(reg->status);
> + u32 irq_number = le32_to_cpu(reg->irq_number);
> + u32 irq_type = le32_to_cpu(reg->irq_type);
> int count;
>
> /*
> * Set the status before raising the IRQ to ensure that the host sees
> * the updated value when it gets the IRQ.
> */
> - WRITE_ONCE(reg->status, status);
> + status |= STATUS_IRQ_RAISED;
> + WRITE_ONCE(reg->status, cpu_to_le32(status));
>
> - switch (reg->irq_type) {
> + switch (irq_type) {
> case IRQ_TYPE_INTX:
> pci_epc_raise_irq(epc, epf->func_no, epf->vfunc_no,
> PCI_IRQ_INTX, 0);
> break;
> case IRQ_TYPE_MSI:
> count = pci_epc_get_msi(epc, epf->func_no, epf->vfunc_no);
> - if (reg->irq_number > count || count <= 0) {
> + if (irq_number > count || count <= 0) {
> dev_err(dev, "Invalid MSI IRQ number %d / %d\n",
> - reg->irq_number, count);
> + irq_number, count);
> return;
> }
> pci_epc_raise_irq(epc, epf->func_no, epf->vfunc_no,
> - PCI_IRQ_MSI, reg->irq_number);
> + PCI_IRQ_MSI, irq_number);
> break;
> case IRQ_TYPE_MSIX:
> count = pci_epc_get_msix(epc, epf->func_no, epf->vfunc_no);
> - if (reg->irq_number > count || count <= 0) {
> + if (irq_number > count || count <= 0) {
> dev_err(dev, "Invalid MSIX IRQ number %d / %d\n",
> - reg->irq_number, count);
> + irq_number, count);
> return;
> }
> pci_epc_raise_irq(epc, epf->func_no, epf->vfunc_no,
> - PCI_IRQ_MSIX, reg->irq_number);
> + PCI_IRQ_MSIX, irq_number);
> break;
> default:
> dev_err(dev, "Failed to raise IRQ, unknown type\n");
> @@ -654,21 +673,22 @@ static void pci_epf_test_cmd_handler(struct work_struct *work)
> struct device *dev = &epf->dev;
> enum pci_barno test_reg_bar = epf_test->test_reg_bar;
> struct pci_epf_test_reg *reg = epf_test->reg[test_reg_bar];
> + u32 irq_type = le32_to_cpu(reg->irq_type);
>
> - command = READ_ONCE(reg->command);
> + command = le32_to_cpu(READ_ONCE(reg->command));
> if (!command)
> goto reset_handler;
>
> WRITE_ONCE(reg->command, 0);
> WRITE_ONCE(reg->status, 0);
>
> - if ((READ_ONCE(reg->flags) & FLAG_USE_DMA) &&
> + if ((le32_to_cpu(READ_ONCE(reg->flags)) & FLAG_USE_DMA) &&
> !epf_test->dma_supported) {
> dev_err(dev, "Cannot transfer data using DMA\n");
> goto reset_handler;
> }
>
> - if (reg->irq_type > IRQ_TYPE_MSIX) {
> + if (irq_type > IRQ_TYPE_MSIX) {
> dev_err(dev, "Failed to detect IRQ type\n");
> goto reset_handler;
> }
> --
> 2.48.1
>
--
மணிவண்ணன் சதாசிவம்
next prev parent reply other threads:[~2025-01-27 6:26 UTC|newest]
Thread overview: 4+ messages / expand[flat|nested] mbox.gz Atom feed top
2025-01-20 11:50 [PATCH] PCI: endpoint: pci-epf-test: Handle endianness properly Niklas Cassel
2025-01-20 17:01 ` Frank Li
2025-01-27 6:26 ` Manivannan Sadhasivam [this message]
2025-01-27 15:55 ` Niklas Cassel
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=20250127062647.ehadg6ji53hkbsbf@thinkpad \
--to=manivannan.sadhasivam@linaro.org \
--cc=bhelgaas@google.com \
--cc=cassel@kernel.org \
--cc=kishon@kernel.org \
--cc=kw@linux.com \
--cc=linux-pci@vger.kernel.org \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox