* [PATCH] PCI: endpoint: pci-epf-test: Handle endianness properly
@ 2025-01-20 11:50 Niklas Cassel
2025-01-20 17:01 ` Frank Li
2025-01-27 6:26 ` Manivannan Sadhasivam
0 siblings, 2 replies; 4+ messages in thread
From: Niklas Cassel @ 2025-01-20 11:50 UTC (permalink / raw)
To: Manivannan Sadhasivam, Krzysztof Wilczyński,
Kishon Vijay Abraham I, Bjorn Helgaas
Cc: Niklas Cassel, linux-pci
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.
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.
Signed-off-by: Niklas Cassel <cassel@kernel.org>
---
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
^ permalink raw reply related [flat|nested] 4+ messages in thread
* Re: [PATCH] PCI: endpoint: pci-epf-test: Handle endianness properly
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
1 sibling, 0 replies; 4+ messages in thread
From: Frank Li @ 2025-01-20 17:01 UTC (permalink / raw)
To: Niklas Cassel
Cc: Manivannan Sadhasivam, Krzysztof Wilczyński,
Kishon Vijay Abraham I, Bjorn Helgaas, linux-pci
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.
>
> 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.
>
> Signed-off-by: Niklas Cassel <cassel@kernel.org>
Reviewed-by: Frank Li <Frank.Li@nxp.com>
> ---
> 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
>
^ permalink raw reply [flat|nested] 4+ messages in thread
* Re: [PATCH] PCI: endpoint: pci-epf-test: Handle endianness properly
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
2025-01-27 15:55 ` Niklas Cassel
1 sibling, 1 reply; 4+ messages in thread
From: Manivannan Sadhasivam @ 2025-01-27 6:26 UTC (permalink / raw)
To: Niklas Cassel
Cc: Krzysztof Wilczyński, Kishon Vijay Abraham I, Bjorn Helgaas,
linux-pci
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
>
--
மணிவண்ணன் சதாசிவம்
^ permalink raw reply [flat|nested] 4+ messages in thread
* Re: [PATCH] PCI: endpoint: pci-epf-test: Handle endianness properly
2025-01-27 6:26 ` Manivannan Sadhasivam
@ 2025-01-27 15:55 ` Niklas Cassel
0 siblings, 0 replies; 4+ messages in thread
From: Niklas Cassel @ 2025-01-27 15:55 UTC (permalink / raw)
To: Manivannan Sadhasivam
Cc: Krzysztof Wilczyński, Kishon Vijay Abraham I, Bjorn Helgaas,
linux-pci
On Mon, Jan 27, 2025 at 11:56:47AM +0530, Manivannan Sadhasivam wrote:
> 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.
Thank you for the review and review comments.
I'll send a V2 regardless, to make the life easier for the PCI maintainers.
(And update V1 as superseeded in patchwork.)
Kind regards,
Niklas
^ permalink raw reply [flat|nested] 4+ messages in thread
end of thread, other threads:[~2025-01-27 15:55 UTC | newest]
Thread overview: 4+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
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
2025-01-27 15:55 ` Niklas Cassel
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox