* Re: [PATCH v2] misc: pci_endpoint_test: Handle BAR sizes larger than INT_MAX
2025-01-24 9:33 [PATCH v2] misc: pci_endpoint_test: Handle BAR sizes larger than INT_MAX Niklas Cassel
@ 2025-01-24 15:43 ` Frank Li
2025-01-27 13:37 ` Jon Hunter
` (2 subsequent siblings)
3 siblings, 0 replies; 7+ messages in thread
From: Frank Li @ 2025-01-24 15:43 UTC (permalink / raw)
To: Niklas Cassel
Cc: Manivannan Sadhasivam, Krzysztof Wilczyński,
Kishon Vijay Abraham I, Arnd Bergmann, Greg Kroah-Hartman,
Jon Hunter, Hans Zhang, linux-pci
On Fri, Jan 24, 2025 at 10:33:01AM +0100, Niklas Cassel wrote:
> Running 'pcitest -b 0' fails with "TEST FAILED" when the BAR0 size
> is e.g. 8 GB.
>
> The return value of the pci_resource_len() macro can be larger than that
> of a signed integer type. Thus, when using 'pcitest' with an 8 GB BAR,
> the bar_size of the integer type will overflow.
>
> Change bar_size from integer to resource_size_t to prevent integer
> overflow for large BAR sizes with 32-bit compilers.
>
> In order to handle 64-bit resource_type_t on 32-bit platforms, we would
> have needed to use a function like div_u64() or similar. Instead, change
> the code to use addition instead of division. This avoids the need for
> div_u64() or similar, while also simplifying the code.
>
> Co-developed-by: Hans Zhang <18255117159@163.com>
> Signed-off-by: Hans Zhang <18255117159@163.com>
> Signed-off-by: Niklas Cassel <cassel@kernel.org>
Reviewed-by: Frank Li <Frank.Li@nxp.com>
> ---
> Changes since v1:
> -Add reason for why division was changed to addition in commit log.
>
> drivers/misc/pci_endpoint_test.c | 18 ++++++++++--------
> 1 file changed, 10 insertions(+), 8 deletions(-)
>
> diff --git a/drivers/misc/pci_endpoint_test.c b/drivers/misc/pci_endpoint_test.c
> index d5ac71a49386..8e48a15100f1 100644
> --- a/drivers/misc/pci_endpoint_test.c
> +++ b/drivers/misc/pci_endpoint_test.c
> @@ -272,9 +272,9 @@ static const u32 bar_test_pattern[] = {
> };
>
> static int pci_endpoint_test_bar_memcmp(struct pci_endpoint_test *test,
> - enum pci_barno barno, int offset,
> - void *write_buf, void *read_buf,
> - int size)
> + enum pci_barno barno,
> + resource_size_t offset, void *write_buf,
> + void *read_buf, int size)
> {
> memset(write_buf, bar_test_pattern[barno], size);
> memcpy_toio(test->bar[barno] + offset, write_buf, size);
> @@ -287,10 +287,11 @@ static int pci_endpoint_test_bar_memcmp(struct pci_endpoint_test *test,
> static int pci_endpoint_test_bar(struct pci_endpoint_test *test,
> enum pci_barno barno)
> {
> - int j, bar_size, buf_size, iters;
> + resource_size_t bar_size, offset = 0;
> void *write_buf __free(kfree) = NULL;
> void *read_buf __free(kfree) = NULL;
> struct pci_dev *pdev = test->pdev;
> + int buf_size;
>
> if (!test->bar[barno])
> return -ENOMEM;
> @@ -314,11 +315,12 @@ static int pci_endpoint_test_bar(struct pci_endpoint_test *test,
> if (!read_buf)
> return -ENOMEM;
>
> - iters = bar_size / buf_size;
> - for (j = 0; j < iters; j++)
> - if (pci_endpoint_test_bar_memcmp(test, barno, buf_size * j,
> - write_buf, read_buf, buf_size))
> + while (offset < bar_size) {
> + if (pci_endpoint_test_bar_memcmp(test, barno, offset, write_buf,
> + read_buf, buf_size))
> return -EIO;
> + offset += buf_size;
> + }
>
> return 0;
> }
> --
> 2.48.1
>
^ permalink raw reply [flat|nested] 7+ messages in thread* Re: [PATCH v2] misc: pci_endpoint_test: Handle BAR sizes larger than INT_MAX
2025-01-24 9:33 [PATCH v2] misc: pci_endpoint_test: Handle BAR sizes larger than INT_MAX Niklas Cassel
2025-01-24 15:43 ` Frank Li
@ 2025-01-27 13:37 ` Jon Hunter
2025-02-01 16:01 ` Manivannan Sadhasivam
2025-02-14 17:48 ` Manivannan Sadhasivam
3 siblings, 0 replies; 7+ messages in thread
From: Jon Hunter @ 2025-01-27 13:37 UTC (permalink / raw)
To: Niklas Cassel, Manivannan Sadhasivam, Krzysztof Wilczyński,
Kishon Vijay Abraham I, Arnd Bergmann, Greg Kroah-Hartman
Cc: Frank Li, Hans Zhang, linux-pci
On 24/01/2025 09:33, Niklas Cassel wrote:
> Running 'pcitest -b 0' fails with "TEST FAILED" when the BAR0 size
> is e.g. 8 GB.
>
> The return value of the pci_resource_len() macro can be larger than that
> of a signed integer type. Thus, when using 'pcitest' with an 8 GB BAR,
> the bar_size of the integer type will overflow.
>
> Change bar_size from integer to resource_size_t to prevent integer
> overflow for large BAR sizes with 32-bit compilers.
>
> In order to handle 64-bit resource_type_t on 32-bit platforms, we would
> have needed to use a function like div_u64() or similar. Instead, change
> the code to use addition instead of division. This avoids the need for
> div_u64() or similar, while also simplifying the code.
>
> Co-developed-by: Hans Zhang <18255117159@163.com>
> Signed-off-by: Hans Zhang <18255117159@163.com>
> Signed-off-by: Niklas Cassel <cassel@kernel.org>
> ---
> Changes since v1:
> -Add reason for why division was changed to addition in commit log.
>
> drivers/misc/pci_endpoint_test.c | 18 ++++++++++--------
> 1 file changed, 10 insertions(+), 8 deletions(-)
>
> diff --git a/drivers/misc/pci_endpoint_test.c b/drivers/misc/pci_endpoint_test.c
> index d5ac71a49386..8e48a15100f1 100644
> --- a/drivers/misc/pci_endpoint_test.c
> +++ b/drivers/misc/pci_endpoint_test.c
> @@ -272,9 +272,9 @@ static const u32 bar_test_pattern[] = {
> };
>
> static int pci_endpoint_test_bar_memcmp(struct pci_endpoint_test *test,
> - enum pci_barno barno, int offset,
> - void *write_buf, void *read_buf,
> - int size)
> + enum pci_barno barno,
> + resource_size_t offset, void *write_buf,
> + void *read_buf, int size)
> {
> memset(write_buf, bar_test_pattern[barno], size);
> memcpy_toio(test->bar[barno] + offset, write_buf, size);
> @@ -287,10 +287,11 @@ static int pci_endpoint_test_bar_memcmp(struct pci_endpoint_test *test,
> static int pci_endpoint_test_bar(struct pci_endpoint_test *test,
> enum pci_barno barno)
> {
> - int j, bar_size, buf_size, iters;
> + resource_size_t bar_size, offset = 0;
> void *write_buf __free(kfree) = NULL;
> void *read_buf __free(kfree) = NULL;
> struct pci_dev *pdev = test->pdev;
> + int buf_size;
>
> if (!test->bar[barno])
> return -ENOMEM;
> @@ -314,11 +315,12 @@ static int pci_endpoint_test_bar(struct pci_endpoint_test *test,
> if (!read_buf)
> return -ENOMEM;
>
> - iters = bar_size / buf_size;
> - for (j = 0; j < iters; j++)
> - if (pci_endpoint_test_bar_memcmp(test, barno, buf_size * j,
> - write_buf, read_buf, buf_size))
> + while (offset < bar_size) {
> + if (pci_endpoint_test_bar_memcmp(test, barno, offset, write_buf,
> + read_buf, buf_size))
> return -EIO;
> + offset += buf_size;
> + }
>
> return 0;
> }
This builds fine for me! I have ran through our testsuite and so ...
Tested-by: Jon Hunter <jonathanh@nvidia.com>
Jon
--
nvpublic
^ permalink raw reply [flat|nested] 7+ messages in thread* Re: [PATCH v2] misc: pci_endpoint_test: Handle BAR sizes larger than INT_MAX
2025-01-24 9:33 [PATCH v2] misc: pci_endpoint_test: Handle BAR sizes larger than INT_MAX Niklas Cassel
2025-01-24 15:43 ` Frank Li
2025-01-27 13:37 ` Jon Hunter
@ 2025-02-01 16:01 ` Manivannan Sadhasivam
2025-02-01 17:39 ` Niklas Cassel
2025-02-14 17:48 ` Manivannan Sadhasivam
3 siblings, 1 reply; 7+ messages in thread
From: Manivannan Sadhasivam @ 2025-02-01 16:01 UTC (permalink / raw)
To: Niklas Cassel
Cc: Krzysztof Wilczyński, Kishon Vijay Abraham I, Arnd Bergmann,
Greg Kroah-Hartman, Jon Hunter, Frank Li, Hans Zhang, linux-pci
On Fri, Jan 24, 2025 at 10:33:01AM +0100, Niklas Cassel wrote:
> Running 'pcitest -b 0' fails with "TEST FAILED" when the BAR0 size
> is e.g. 8 GB.
>
> The return value of the pci_resource_len() macro can be larger than that
> of a signed integer type. Thus, when using 'pcitest' with an 8 GB BAR,
> the bar_size of the integer type will overflow.
>
> Change bar_size from integer to resource_size_t to prevent integer
> overflow for large BAR sizes with 32-bit compilers.
>
> In order to handle 64-bit resource_type_t on 32-bit platforms, we would
> have needed to use a function like div_u64() or similar. Instead, change
> the code to use addition instead of division. This avoids the need for
> div_u64() or similar, while also simplifying the code.
>
Fixes tag?
> Co-developed-by: Hans Zhang <18255117159@163.com>
> Signed-off-by: Hans Zhang <18255117159@163.com>
> Signed-off-by: Niklas Cassel <cassel@kernel.org>
Reviewed-by: Manivannan Sadhasivam <manivannan.sadhasivam@linaro.org>
> ---
> Changes since v1:
> -Add reason for why division was changed to addition in commit log.
You removed the history of this patch that was present in v1. Since there is
no link to v1, you could've kept it here for the sake of completeness.
- Mani
--
மணிவண்ணன் சதாசிவம்
^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [PATCH v2] misc: pci_endpoint_test: Handle BAR sizes larger than INT_MAX
2025-02-01 16:01 ` Manivannan Sadhasivam
@ 2025-02-01 17:39 ` Niklas Cassel
2025-02-07 17:42 ` Manivannan Sadhasivam
0 siblings, 1 reply; 7+ messages in thread
From: Niklas Cassel @ 2025-02-01 17:39 UTC (permalink / raw)
To: Manivannan Sadhasivam
Cc: Krzysztof Wilczyński, Kishon Vijay Abraham I, Arnd Bergmann,
Greg Kroah-Hartman, Jon Hunter, Frank Li, Hans Zhang, linux-pci
On Sat, Feb 01, 2025 at 09:31:54PM +0530, Manivannan Sadhasivam wrote:
> On Fri, Jan 24, 2025 at 10:33:01AM +0100, Niklas Cassel wrote:
> > Running 'pcitest -b 0' fails with "TEST FAILED" when the BAR0 size
> > is e.g. 8 GB.
> >
> > The return value of the pci_resource_len() macro can be larger than that
> > of a signed integer type. Thus, when using 'pcitest' with an 8 GB BAR,
> > the bar_size of the integer type will overflow.
> >
> > Change bar_size from integer to resource_size_t to prevent integer
> > overflow for large BAR sizes with 32-bit compilers.
> >
> > In order to handle 64-bit resource_type_t on 32-bit platforms, we would
> > have needed to use a function like div_u64() or similar. Instead, change
> > the code to use addition instead of division. This avoids the need for
> > div_u64() or similar, while also simplifying the code.
> >
>
> Fixes tag?
size has been of type int since:
2c156ac71c6b ("misc: Add host side PCI driver for PCI test function device")
But I think a better SHA1 to use as the Fixes tag would be:
cda370ec6d1f ("misc: pci_endpoint_test: Avoid using hard-coded BAR sizes")
Which has the one that started using pci_resource_len()
(while still keeping size as type int).
Perhaps this can be amended while applying?
>
> > Co-developed-by: Hans Zhang <18255117159@163.com>
> > Signed-off-by: Hans Zhang <18255117159@163.com>
> > Signed-off-by: Niklas Cassel <cassel@kernel.org>
>
> Reviewed-by: Manivannan Sadhasivam <manivannan.sadhasivam@linaro.org>
>
> > ---
> > Changes since v1:
> > -Add reason for why division was changed to addition in commit log.
>
> You removed the history of this patch that was present in v1. Since there is
> no link to v1, you could've kept it here for the sake of completeness.
Yes, because I made the commit log way more verbose, as requested by Frank,
so it now includes references to div_u64() even though we are not using it.
Kind regards,
Niklas
^ permalink raw reply [flat|nested] 7+ messages in thread* Re: [PATCH v2] misc: pci_endpoint_test: Handle BAR sizes larger than INT_MAX
2025-02-01 17:39 ` Niklas Cassel
@ 2025-02-07 17:42 ` Manivannan Sadhasivam
0 siblings, 0 replies; 7+ messages in thread
From: Manivannan Sadhasivam @ 2025-02-07 17:42 UTC (permalink / raw)
To: Niklas Cassel
Cc: Krzysztof Wilczyński, Kishon Vijay Abraham I, Arnd Bergmann,
Greg Kroah-Hartman, Jon Hunter, Frank Li, Hans Zhang, linux-pci
On Sat, Feb 01, 2025 at 06:39:01PM +0100, Niklas Cassel wrote:
> On Sat, Feb 01, 2025 at 09:31:54PM +0530, Manivannan Sadhasivam wrote:
> > On Fri, Jan 24, 2025 at 10:33:01AM +0100, Niklas Cassel wrote:
> > > Running 'pcitest -b 0' fails with "TEST FAILED" when the BAR0 size
> > > is e.g. 8 GB.
> > >
> > > The return value of the pci_resource_len() macro can be larger than that
> > > of a signed integer type. Thus, when using 'pcitest' with an 8 GB BAR,
> > > the bar_size of the integer type will overflow.
> > >
> > > Change bar_size from integer to resource_size_t to prevent integer
> > > overflow for large BAR sizes with 32-bit compilers.
> > >
> > > In order to handle 64-bit resource_type_t on 32-bit platforms, we would
> > > have needed to use a function like div_u64() or similar. Instead, change
> > > the code to use addition instead of division. This avoids the need for
> > > div_u64() or similar, while also simplifying the code.
> > >
> >
> > Fixes tag?
>
> size has been of type int since:
> 2c156ac71c6b ("misc: Add host side PCI driver for PCI test function device")
>
> But I think a better SHA1 to use as the Fixes tag would be:
> cda370ec6d1f ("misc: pci_endpoint_test: Avoid using hard-coded BAR sizes")
>
> Which has the one that started using pci_resource_len()
> (while still keeping size as type int).
>
> Perhaps this can be amended while applying?
>
Sure.
- Mani
--
மணிவண்ணன் சதாசிவம்
^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [PATCH v2] misc: pci_endpoint_test: Handle BAR sizes larger than INT_MAX
2025-01-24 9:33 [PATCH v2] misc: pci_endpoint_test: Handle BAR sizes larger than INT_MAX Niklas Cassel
` (2 preceding siblings ...)
2025-02-01 16:01 ` Manivannan Sadhasivam
@ 2025-02-14 17:48 ` Manivannan Sadhasivam
3 siblings, 0 replies; 7+ messages in thread
From: Manivannan Sadhasivam @ 2025-02-14 17:48 UTC (permalink / raw)
To: Niklas Cassel
Cc: Krzysztof Wilczyński, Kishon Vijay Abraham I, Arnd Bergmann,
Greg Kroah-Hartman, Jon Hunter, Frank Li, Hans Zhang, linux-pci
On Fri, Jan 24, 2025 at 10:33:01AM +0100, Niklas Cassel wrote:
> Running 'pcitest -b 0' fails with "TEST FAILED" when the BAR0 size
> is e.g. 8 GB.
>
> The return value of the pci_resource_len() macro can be larger than that
> of a signed integer type. Thus, when using 'pcitest' with an 8 GB BAR,
> the bar_size of the integer type will overflow.
>
> Change bar_size from integer to resource_size_t to prevent integer
> overflow for large BAR sizes with 32-bit compilers.
>
> In order to handle 64-bit resource_type_t on 32-bit platforms, we would
> have needed to use a function like div_u64() or similar. Instead, change
> the code to use addition instead of division. This avoids the need for
> div_u64() or similar, while also simplifying the code.
>
> Co-developed-by: Hans Zhang <18255117159@163.com>
> Signed-off-by: Hans Zhang <18255117159@163.com>
> Signed-off-by: Niklas Cassel <cassel@kernel.org>
Added fixes tag and applied to pci/endpoint!
- Mani
> ---
> Changes since v1:
> -Add reason for why division was changed to addition in commit log.
>
> drivers/misc/pci_endpoint_test.c | 18 ++++++++++--------
> 1 file changed, 10 insertions(+), 8 deletions(-)
>
> diff --git a/drivers/misc/pci_endpoint_test.c b/drivers/misc/pci_endpoint_test.c
> index d5ac71a49386..8e48a15100f1 100644
> --- a/drivers/misc/pci_endpoint_test.c
> +++ b/drivers/misc/pci_endpoint_test.c
> @@ -272,9 +272,9 @@ static const u32 bar_test_pattern[] = {
> };
>
> static int pci_endpoint_test_bar_memcmp(struct pci_endpoint_test *test,
> - enum pci_barno barno, int offset,
> - void *write_buf, void *read_buf,
> - int size)
> + enum pci_barno barno,
> + resource_size_t offset, void *write_buf,
> + void *read_buf, int size)
> {
> memset(write_buf, bar_test_pattern[barno], size);
> memcpy_toio(test->bar[barno] + offset, write_buf, size);
> @@ -287,10 +287,11 @@ static int pci_endpoint_test_bar_memcmp(struct pci_endpoint_test *test,
> static int pci_endpoint_test_bar(struct pci_endpoint_test *test,
> enum pci_barno barno)
> {
> - int j, bar_size, buf_size, iters;
> + resource_size_t bar_size, offset = 0;
> void *write_buf __free(kfree) = NULL;
> void *read_buf __free(kfree) = NULL;
> struct pci_dev *pdev = test->pdev;
> + int buf_size;
>
> if (!test->bar[barno])
> return -ENOMEM;
> @@ -314,11 +315,12 @@ static int pci_endpoint_test_bar(struct pci_endpoint_test *test,
> if (!read_buf)
> return -ENOMEM;
>
> - iters = bar_size / buf_size;
> - for (j = 0; j < iters; j++)
> - if (pci_endpoint_test_bar_memcmp(test, barno, buf_size * j,
> - write_buf, read_buf, buf_size))
> + while (offset < bar_size) {
> + if (pci_endpoint_test_bar_memcmp(test, barno, offset, write_buf,
> + read_buf, buf_size))
> return -EIO;
> + offset += buf_size;
> + }
>
> return 0;
> }
> --
> 2.48.1
>
>
--
மணிவண்ணன் சதாசிவம்
^ permalink raw reply [flat|nested] 7+ messages in thread