| Message ID | 20250123095906.3578241-2-cassel@kernel.org (mailing list archive) |
|---|---|
| State | Superseded |
| Delegated to: | Krzysztof Wilczyński |
| Headers | show |
| Series | misc: pci_endpoint_test: Handle BAR sizes larger than INT_MAX | expand |
On Thu, Jan 23, 2025 at 10:59:07AM +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. > > Co-developed-by: Hans Zhang <18255117159@163.com> > Signed-off-by: Hans Zhang <18255117159@163.com> > Signed-off-by: Niklas Cassel <cassel@kernel.org> > --- > Hans submitted a patch for this that was reverted because apparently some > gcc-7 arm32 compiler doesn't like div_u64(). In order to avoid debugging > gcc-7 arm32 compiler issues, simply replace the division with addition, > which arguably makes the code simpler as well. > > 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; > + } Actually, you change code logic although functionality is the same. I feel like you should mention at commit message or use origial code by just change variable type. #ifdef CONFIG_PHYS_ADDR_T_64BIT typedef u64 phys_addr_t; #else typedef u32 phys_addr_t; #endif typedef phys_addr_t resource_size_t; resource_size_t may 32bit at some configuration. But I don't know what happen when 8G bar pci device attached to such system. Frank > > return 0; > } > -- > 2.48.1 >
On Thu, Jan 23, 2025 at 10:54:19AM -0500, Frank Li wrote: > On Thu, Jan 23, 2025 at 10:59:07AM +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. > > > > Co-developed-by: Hans Zhang <18255117159@163.com> > > Signed-off-by: Hans Zhang <18255117159@163.com> > > Signed-off-by: Niklas Cassel <cassel@kernel.org> > > --- > > Hans submitted a patch for this that was reverted because apparently some > > gcc-7 arm32 compiler doesn't like div_u64(). In order to avoid debugging > > gcc-7 arm32 compiler issues, simply replace the division with addition, > > which arguably makes the code simpler as well. > > > > 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; > > + } > > Actually, you change code logic although functionality is the same. I feel > like you should mention at commit message or use origial code by just > change variable type. > > #ifdef CONFIG_PHYS_ADDR_T_64BIT > typedef u64 phys_addr_t; > #else > typedef u32 phys_addr_t; > #endif Hello Frank, I personally think that is a horrible idea :) We do not want to introduce ifdefs in the middle of the code, unless in exceptional circumstances, like architecture specific optimized code. Kind regards, Niklas
On Thu, Jan 23, 2025 at 07:35:06PM +0100, Niklas Cassel wrote: > On Thu, Jan 23, 2025 at 10:54:19AM -0500, Frank Li wrote: > > On Thu, Jan 23, 2025 at 10:59:07AM +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. > > > > > > Co-developed-by: Hans Zhang <18255117159@163.com> > > > Signed-off-by: Hans Zhang <18255117159@163.com> > > > Signed-off-by: Niklas Cassel <cassel@kernel.org> > > > --- > > > Hans submitted a patch for this that was reverted because apparently some > > > gcc-7 arm32 compiler doesn't like div_u64(). In order to avoid debugging > > > gcc-7 arm32 compiler issues, simply replace the division with addition, > > > which arguably makes the code simpler as well. > > > > > > 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; > > > + } > > > > Actually, you change code logic although functionality is the same. I feel > > like you should mention at commit message or use origial code by just > > change variable type. > > > > #ifdef CONFIG_PHYS_ADDR_T_64BIT > > typedef u64 phys_addr_t; > > #else > > typedef u32 phys_addr_t; > > #endif > > Hello Frank, > > I personally think that is a horrible idea :) > > We do not want to introduce ifdefs in the middle of the code, unless > in exceptional circumstances, like architecture specific optimized code. You miss understand what my means. I copy it from type.h to indicate resource_size_t is not 64bit at all platforms. Frank > > > Kind regards, > Niklas
On Thu, Jan 23, 2025 at 02:09:24PM -0500, Frank Li wrote: > On Thu, Jan 23, 2025 at 07:35:06PM +0100, Niklas Cassel wrote: > > > > > > Actually, you change code logic although functionality is the same. I feel > > > like you should mention at commit message or use origial code by just > > > change variable type. > > > > > > #ifdef CONFIG_PHYS_ADDR_T_64BIT > > > typedef u64 phys_addr_t; > > > #else > > > typedef u32 phys_addr_t; > > > #endif > > > > Hello Frank, > > > > I personally think that is a horrible idea :) > > > > We do not want to introduce ifdefs in the middle of the code, unless > > in exceptional circumstances, like architecture specific optimized code. > > You miss understand what my means. I copy it from type.h to indicate > resource_size_t is not 64bit at all platforms. I know that resource_size_t is typedefed to phys_addr_t, which can be 32-bit or 64-bit. (I compile tested this patch on 32-bit both with and without PAE.) resource_size_t is the type returned by pci_resource_len(). That is why the patch in subject changes the type to use resource_size_t. IMO, it does not make sense to use any other type (e.g. u64), since the value returned by pci_resource_len() will still be limited to what can be represented by resource_size_t. A BARs larger than 4GB, on systems with 32-bit resource_size_t, will get disabled by PCI core: https://github.com/torvalds/linux/blob/v6.13/drivers/pci/probe.c#L265-L270 So all good. As for your question why I don't keep the division, please read the comment section in this patch (where the changelog usually is), or read the thread: https://lore.kernel.org/linux-pci/20250109094556.1724663-1-18255117159@163.com/T/#t I guess I could have added: " 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. " Let me send a V2 with that senctence added to address your review comment. Kind regards, Niklas
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; }