| Message ID | 20250104151652.1652181-1-18255117159@163.com (mailing list archive) |
|---|---|
| State | New |
| Delegated to: | Krzysztof Wilczyński |
| Headers | show |
| Series | [v8] misc: pci_endpoint_test: Fix overflow of bar_size | expand |
On Sat, Jan 04, 2025 at 11:16:52PM +0800, Hans Zhang wrote: > With 8GB BAR2, running pcitest -b 2 fails with "TEST FAILED". > > The return value of the `pci_resource_len` interface is not an integer. > Using `pcitest` with an 8GB BAR2, the bar_size of integer type will > overflow. > > Change the data type of bar_size from integer to resource_size_t, to fix > the above issue. > > Signed-off-by: Hans Zhang <18255117159@163.com> > Reviewed-by: Niklas Cassel <cassel@kernel.org> > Reviewed-by: Manivannan Sadhasivam <manivannan.sadhasivam@linaro.org> When significantly changing the patch from one version to another, (as in this case), you are supposed to drop the Reviewed-by tags. Doing a: $ git grep -A 10 "IS_ENABLED(CONFIG_PHYS_ADDR_T_64BIT" does not show very many hits, which suggests that this is not the proper way to solve this. I don't know the proper solution to this. How is resource_size_t handled in other PCI driver when being built on with 32-bit PHYS_ADDR_T ? Can't you just cast the resource_size_t to u64 before doing the division? > --- > Changes since v8: > > - Add reviewer. > > Changes since v4-v7: > https://lore.kernel.org/linux-pci/20250102120222.1403906-1-18255117159@163.com/ > https://lore.kernel.org/linux-pci/20250101151509.570341-1-18255117159@163.com/ > > - Fix 32-bit OS warnings and errors. > - Fix undefined reference to `__udivmoddi4` > > Changes since v3: > https://lore.kernel.org/linux-pci/20241221141009.27317-1-18255117159@163.com/ > > - The patch subject were modified. > > Changes since v2: > https://lore.kernel.org/linux-pci/20241220075253.16791-1-18255117159@163.com/ > > - Fix "changes" part goes below the --- line > - The patch commit message were modified. > > Changes since v1: > https://lore.kernel.org/linux-pci/20241217121220.19676-1-18255117159@163.com/ > > - The patch subject and commit message were modified. > --- > drivers/misc/pci_endpoint_test.c | 12 +++++++++--- > 1 file changed, 9 insertions(+), 3 deletions(-) > > diff --git a/drivers/misc/pci_endpoint_test.c b/drivers/misc/pci_endpoint_test.c > index 3aaaf47fa4ee..50d4616119af 100644 > --- a/drivers/misc/pci_endpoint_test.c > +++ b/drivers/misc/pci_endpoint_test.c > @@ -280,10 +280,11 @@ static int pci_endpoint_test_bar_memcmp(struct pci_endpoint_test *test, > static bool pci_endpoint_test_bar(struct pci_endpoint_test *test, > enum pci_barno barno) > { > - int j, bar_size, buf_size, iters, remain; > void *write_buf __free(kfree) = NULL; > void *read_buf __free(kfree) = NULL; > struct pci_dev *pdev = test->pdev; > + int j, buf_size, iters, remain; > + resource_size_t bar_size; > > if (!test->bar[barno]) > return false; > @@ -307,13 +308,18 @@ static bool pci_endpoint_test_bar(struct pci_endpoint_test *test, > if (!read_buf) > return false; > > - iters = bar_size / buf_size; > + if (IS_ENABLED(CONFIG_PHYS_ADDR_T_64BIT)) { > + remain = do_div(bar_size, buf_size); > + iters = bar_size; > + } else { > + iters = bar_size / buf_size; > + remain = 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)) > return false; > > - remain = bar_size % buf_size; > if (remain) > if (pci_endpoint_test_bar_memcmp(test, barno, buf_size * iters, > write_buf, read_buf, remain)) > > base-commit: ccb98ccef0e543c2bd4ef1a72270461957f3d8d0 > -- > 2.25.1 >
On 2025/1/6 19:49, Niklas Cassel wrote: >> >> Signed-off-by: Hans Zhang <18255117159@163.com> >> Reviewed-by: Niklas Cassel <cassel@kernel.org> >> Reviewed-by: Manivannan Sadhasivam <manivannan.sadhasivam@linaro.org> > > When significantly changing the patch from one version to another, > (as in this case), you are supposed to drop the Reviewed-by tags. Okay, i will remove the reviewer. > > Doing a: > $ git grep -A 10 "IS_ENABLED(CONFIG_PHYS_ADDR_T_64BIT" > does not show very many hits, which suggests that this is not the proper > way to solve this. > > I don't know the proper solution to this. How is resource_size_t handled > in other PCI driver when being built on with 32-bit PHYS_ADDR_T ? > > Can't you just cast the resource_size_t to u64 before doing the division? > Thank you Niklas. I'll try it. Regards Hans
On 2025/1/6 19:49, Niklas Cassel wrote: > Doing a: > $ git grep -A 10 "IS_ENABLED(CONFIG_PHYS_ADDR_T_64BIT" > does not show very many hits, which suggests that this is not the proper > way to solve this. > > I don't know the proper solution to this. How is resource_size_t handled > in other PCI driver when being built on with 32-bit PHYS_ADDR_T ? > > Can't you just cast the resource_size_t to u64 before doing the division? Hi Niklas, Modify as follows, if you have no opinion, I will fix the next version. >> --- >> drivers/misc/pci_endpoint_test.c | 12 +++++++++--- >> 1 file changed, 9 insertions(+), 3 deletions(-) >> >> diff --git a/drivers/misc/pci_endpoint_test.c b/drivers/misc/pci_endpoint_test.c >> index 3aaaf47fa4ee..50d4616119af 100644 >> --- a/drivers/misc/pci_endpoint_test.c >> +++ b/drivers/misc/pci_endpoint_test.c >> @@ -280,10 +280,11 @@ static int pci_endpoint_test_bar_memcmp(struct pci_endpoint_test *test, >> static bool pci_endpoint_test_bar(struct pci_endpoint_test *test, >> enum pci_barno barno) >> { >> - int j, bar_size, buf_size, iters, remain; >> void *write_buf __free(kfree) = NULL; >> void *read_buf __free(kfree) = NULL; >> struct pci_dev *pdev = test->pdev; >> + int j, buf_size, iters, remain; >> + resource_size_t bar_size; Fix resource_size_t to u64 bar_size. u64 bar_size; >> >> if (!test->bar[barno]) >> return false; >> @@ -307,13 +308,18 @@ static bool pci_endpoint_test_bar(struct pci_endpoint_test *test, >> if (!read_buf) >> return false; >> >> - iters = bar_size / buf_size; >> + if (IS_ENABLED(CONFIG_PHYS_ADDR_T_64BIT)) { >> + remain = do_div(bar_size, buf_size); >> + iters = bar_size; >> + } else { >> + iters = bar_size / buf_size; >> + remain = bar_size % buf_size; >> + } Removed IS_ENABLED(CONFIG_PHYS_ADDR_T_64BIT), Execute the following code. remain = do_div(bar_size, buf_size); iters = bar_size; >> for (j = 0; j < iters; j++) >> if (pci_endpoint_test_bar_memcmp(test, barno, buf_size * j, >> write_buf, read_buf, buf_size)) >> return false; >> >> - remain = bar_size % buf_size; >> if (remain) >> if (pci_endpoint_test_bar_memcmp(test, barno, buf_size * iters, >> write_buf, read_buf, remain)) >> >> base-commit: ccb98ccef0e543c2bd4ef1a72270461957f3d8d0 >> -- >> 2.25.1 >> Best regards Hans
Hello Hans, On Mon, Jan 06, 2025 at 11:32:33PM +0800, Hans Zhang wrote: > > > On 2025/1/6 19:49, Niklas Cassel wrote: > > Doing a: > > $ git grep -A 10 "IS_ENABLED(CONFIG_PHYS_ADDR_T_64BIT" > > does not show very many hits, which suggests that this is not the proper > > way to solve this. > > > > I don't know the proper solution to this. How is resource_size_t handled > > in other PCI driver when being built on with 32-bit PHYS_ADDR_T ? > > > > Can't you just cast the resource_size_t to u64 before doing the division? > > Hi Niklas, > > Modify as follows, if you have no opinion, I will fix the next version. > > > > --- > > > drivers/misc/pci_endpoint_test.c | 12 +++++++++--- > > > 1 file changed, 9 insertions(+), 3 deletions(-) > > > > > > diff --git a/drivers/misc/pci_endpoint_test.c b/drivers/misc/pci_endpoint_test.c > > > index 3aaaf47fa4ee..50d4616119af 100644 > > > --- a/drivers/misc/pci_endpoint_test.c > > > +++ b/drivers/misc/pci_endpoint_test.c > > > @@ -280,10 +280,11 @@ static int pci_endpoint_test_bar_memcmp(struct pci_endpoint_test *test, > > > static bool pci_endpoint_test_bar(struct pci_endpoint_test *test, > > > enum pci_barno barno) > > > { > > > - int j, bar_size, buf_size, iters, remain; > > > void *write_buf __free(kfree) = NULL; > > > void *read_buf __free(kfree) = NULL; > > > struct pci_dev *pdev = test->pdev; > > > + int j, buf_size, iters, remain; > > > + resource_size_t bar_size; > > Fix resource_size_t to u64 bar_size. > u64 bar_size; > > > > if (!test->bar[barno]) > > > return false; > > > @@ -307,13 +308,18 @@ static bool pci_endpoint_test_bar(struct pci_endpoint_test *test, > > > if (!read_buf) > > > return false; > > > - iters = bar_size / buf_size; > > > + if (IS_ENABLED(CONFIG_PHYS_ADDR_T_64BIT)) { > > > + remain = do_div(bar_size, buf_size); > > > + iters = bar_size; > > > + } else { > > > + iters = bar_size / buf_size; > > > + remain = bar_size % buf_size; > > > + } > > Removed IS_ENABLED(CONFIG_PHYS_ADDR_T_64BIT), Execute the following code. > > remain = do_div(bar_size, buf_size); > iters = bar_size; Perhaps keep it as resource_size_t and then cast it to u64 in the do_div() call? Kind regards, Niklas
On 2025/1/7 18:32, Niklas Cassel wrote: >>>> --- >>>> drivers/misc/pci_endpoint_test.c | 12 +++++++++--- >>>> 1 file changed, 9 insertions(+), 3 deletions(-) >>>> >>>> diff --git a/drivers/misc/pci_endpoint_test.c b/drivers/misc/pci_endpoint_test.c >>>> index 3aaaf47fa4ee..50d4616119af 100644 >>>> --- a/drivers/misc/pci_endpoint_test.c >>>> +++ b/drivers/misc/pci_endpoint_test.c >>>> @@ -280,10 +280,11 @@ static int pci_endpoint_test_bar_memcmp(struct pci_endpoint_test *test, >>>> static bool pci_endpoint_test_bar(struct pci_endpoint_test *test, >>>> enum pci_barno barno) >>>> { >>>> - int j, bar_size, buf_size, iters, remain; >>>> void *write_buf __free(kfree) = NULL; >>>> void *read_buf __free(kfree) = NULL; >>>> struct pci_dev *pdev = test->pdev; >>>> + int j, buf_size, iters, remain; >>>> + resource_size_t bar_size; >> >> Fix resource_size_t to u64 bar_size. >> u64 bar_size; >> >>>> if (!test->bar[barno]) >>>> return false; >>>> @@ -307,13 +308,18 @@ static bool pci_endpoint_test_bar(struct pci_endpoint_test *test, >>>> if (!read_buf) >>>> return false; >>>> - iters = bar_size / buf_size; >>>> + if (IS_ENABLED(CONFIG_PHYS_ADDR_T_64BIT)) { >>>> + remain = do_div(bar_size, buf_size); >>>> + iters = bar_size; >>>> + } else { >>>> + iters = bar_size / buf_size; >>>> + remain = bar_size % buf_size; >>>> + } >> >> Removed IS_ENABLED(CONFIG_PHYS_ADDR_T_64BIT), Execute the following code. >> >> remain = do_div(bar_size, buf_size); >> iters = bar_size; > > Perhaps keep it as resource_size_t and then cast it to u64 in the do_div() > call? Hi Niklas, resource_size_t bar_size; remain = do_div((u64)bar_size, buf_size); It works for the arm platform. arch/arm/include/asm/div64.h static inline uint32_t __div64_32(uint64_t *n, uint32_t base) { register unsigned int __base asm("r4") = base; register unsigned long long __n asm("r0") = *n; register unsigned long long __res asm("r2"); unsigned int __rem; asm( __asmeq("%0", "r0") __asmeq("%1", "r2") __asmeq("%2", "r4") "bl __do_div64" : "+r" (__n), "=r" (__res) : "r" (__base) : "ip", "lr", "cc"); __rem = __n >> 32; *n = __res; return __rem; } #define __div64_32 __div64_32 #define do_div(n, base) __div64_32(&(n), base) For X86 platforms, do_div is a macro definition, and the first parameter does not define its type. If the macro definition is replaced directly, an error will be reported in the ubuntu20.04 release. resource_size_t bar_size; remain = do_div((u64)bar_size, buf_size); arch/x86/include/asm/div64.h #define do_div(n, base) \ ({ \ unsigned long __upper, __low, __high, __mod, __base; \ __base = (base); \ if (__builtin_constant_p(__base) && is_power_of_2(__base)) { \ __mod = n & (__base - 1); \ n >>= ilog2(__base); \ } else { \ asm("" : "=a" (__low), "=d" (__high) : "A" (n));\ __upper = __high; \ if (__high) { \ __upper = __high % (__base); \ __high = __high / (__base); \ } \ asm("divl %2" : "=a" (__low), "=d" (__mod) \ : "rm" (__base), "0" (__low), "1" (__upper)); \ asm("" : "=A" (n) : "a" (__low), "d" (__high)); \ } \ __mod; \ }) Best regards Hans
On Tue, Jan 07, 2025 at 07:27:24PM +0800, Hans Zhang wrote: > > > On 2025/1/7 18:32, Niklas Cassel wrote: > > > > > --- > > > > > drivers/misc/pci_endpoint_test.c | 12 +++++++++--- > > > > > 1 file changed, 9 insertions(+), 3 deletions(-) > > > > > > > > > > diff --git a/drivers/misc/pci_endpoint_test.c b/drivers/misc/pci_endpoint_test.c > > > > > index 3aaaf47fa4ee..50d4616119af 100644 > > > > > --- a/drivers/misc/pci_endpoint_test.c > > > > > +++ b/drivers/misc/pci_endpoint_test.c > > > > > @@ -280,10 +280,11 @@ static int pci_endpoint_test_bar_memcmp(struct pci_endpoint_test *test, > > > > > static bool pci_endpoint_test_bar(struct pci_endpoint_test *test, > > > > > enum pci_barno barno) > > > > > { > > > > > - int j, bar_size, buf_size, iters, remain; > > > > > void *write_buf __free(kfree) = NULL; > > > > > void *read_buf __free(kfree) = NULL; > > > > > struct pci_dev *pdev = test->pdev; > > > > > + int j, buf_size, iters, remain; > > > > > + resource_size_t bar_size; > > > > > > Fix resource_size_t to u64 bar_size. > > > u64 bar_size; > > > > > > > > if (!test->bar[barno]) > > > > > return false; > > > > > @@ -307,13 +308,18 @@ static bool pci_endpoint_test_bar(struct pci_endpoint_test *test, > > > > > if (!read_buf) > > > > > return false; > > > > > - iters = bar_size / buf_size; > > > > > + if (IS_ENABLED(CONFIG_PHYS_ADDR_T_64BIT)) { > > > > > + remain = do_div(bar_size, buf_size); > > > > > + iters = bar_size; > > > > > + } else { > > > > > + iters = bar_size / buf_size; > > > > > + remain = bar_size % buf_size; > > > > > + } > > > > > > Removed IS_ENABLED(CONFIG_PHYS_ADDR_T_64BIT), Execute the following code. > > > > > > remain = do_div(bar_size, buf_size); > > > iters = bar_size; > > > > Perhaps keep it as resource_size_t and then cast it to u64 in the do_div() > > call? > > > Hi Niklas, > > resource_size_t bar_size; > remain = do_div((u64)bar_size, buf_size); > > It works for the arm platform. > > arch/arm/include/asm/div64.h > static inline uint32_t __div64_32(uint64_t *n, uint32_t base) > { > register unsigned int __base asm("r4") = base; > register unsigned long long __n asm("r0") = *n; > register unsigned long long __res asm("r2"); > unsigned int __rem; > asm( __asmeq("%0", "r0") > __asmeq("%1", "r2") > __asmeq("%2", "r4") > "bl __do_div64" > : "+r" (__n), "=r" (__res) > : "r" (__base) > : "ip", "lr", "cc"); > __rem = __n >> 32; > *n = __res; > return __rem; > } > #define __div64_32 __div64_32 > > #define do_div(n, base) __div64_32(&(n), base) > > > For X86 platforms, do_div is a macro definition, and the first parameter > does not define its type. If the macro definition is replaced directly, an > error will be reported in the ubuntu20.04 release. What is the error? We don't need to use do_div(). The current code that does normal / and % works fine on both 32-bit and 64-bit if you just do: static bool pci_endpoint_test_bar(struct pci_endpoint_test *test, enum pci_barno barno) { - int j, bar_size, buf_size, iters, remain; + int j, buf_size, iters, remain; void *write_buf __free(kfree) = NULL; void *read_buf __free(kfree) = NULL; struct pci_dev *pdev = test->pdev; + u64 bar_size; No? Kind regards, Niklas
On 2025/1/7 19:33, Niklas Cassel wrote: >> Hi Niklas, >> >> resource_size_t bar_size; >> remain = do_div((u64)bar_size, buf_size); >> >> It works for the arm platform. >> >> arch/arm/include/asm/div64.h >> static inline uint32_t __div64_32(uint64_t *n, uint32_t base) >> { >> register unsigned int __base asm("r4") = base; >> register unsigned long long __n asm("r0") = *n; >> register unsigned long long __res asm("r2"); >> unsigned int __rem; >> asm( __asmeq("%0", "r0") >> __asmeq("%1", "r2") >> __asmeq("%2", "r4") >> "bl __do_div64" >> : "+r" (__n), "=r" (__res) >> : "r" (__base) >> : "ip", "lr", "cc"); >> __rem = __n >> 32; >> *n = __res; >> return __rem; >> } >> #define __div64_32 __div64_32 >> >> #define do_div(n, base) __div64_32(&(n), base) >> >> >> For X86 platforms, do_div is a macro definition, and the first parameter >> does not define its type. If the macro definition is replaced directly, an >> error will be reported in the ubuntu20.04 release. > > What is the error? > > We don't need to use do_div(). > The current code that does normal / and % works fine on both > 32-bit and 64-bit if you just do: > > static bool pci_endpoint_test_bar(struct pci_endpoint_test *test, > enum pci_barno barno) > { > - int j, bar_size, buf_size, iters, remain; > + int j, buf_size, iters, remain; > void *write_buf __free(kfree) = NULL; > void *read_buf __free(kfree) = NULL; > struct pci_dev *pdev = test->pdev; > + u64 bar_size; > > No? Hi Niklas, Please look at the robot compilation error. https://patchwork.kernel.org/project/linux-pci/patch/20241231065500.168799-1-18255117159@163.com/ drivers/misc/pci_endpoint_test.c:315: undefined reference to `__udivmoddi4' Best regards Hans
On Tue, Jan 07, 2025 at 07:43:26PM +0800, Hans Zhang wrote: > > > On 2025/1/7 19:33, Niklas Cassel wrote: > > > Hi Niklas, > > > > > > resource_size_t bar_size; > > > remain = do_div((u64)bar_size, buf_size); > > > > > > It works for the arm platform. > > > > > > arch/arm/include/asm/div64.h > > > static inline uint32_t __div64_32(uint64_t *n, uint32_t base) > > > { > > > register unsigned int __base asm("r4") = base; > > > register unsigned long long __n asm("r0") = *n; > > > register unsigned long long __res asm("r2"); > > > unsigned int __rem; > > > asm( __asmeq("%0", "r0") > > > __asmeq("%1", "r2") > > > __asmeq("%2", "r4") > > > "bl __do_div64" > > > : "+r" (__n), "=r" (__res) > > > : "r" (__base) > > > : "ip", "lr", "cc"); > > > __rem = __n >> 32; > > > *n = __res; > > > return __rem; > > > } > > > #define __div64_32 __div64_32 > > > > > > #define do_div(n, base) __div64_32(&(n), base) > > > > > > > > > For X86 platforms, do_div is a macro definition, and the first parameter > > > does not define its type. If the macro definition is replaced directly, an > > > error will be reported in the ubuntu20.04 release. > > > > What is the error? > > > > We don't need to use do_div(). > > The current code that does normal / and % works fine on both > > 32-bit and 64-bit if you just do: > > > > static bool pci_endpoint_test_bar(struct pci_endpoint_test *test, > > enum pci_barno barno) > > { > > - int j, bar_size, buf_size, iters, remain; > > + int j, buf_size, iters, remain; > > void *write_buf __free(kfree) = NULL; > > void *read_buf __free(kfree) = NULL; > > struct pci_dev *pdev = test->pdev; > > + u64 bar_size; > > > > No? > > > Hi Niklas, > > Please look at the robot compilation error. > > https://patchwork.kernel.org/project/linux-pci/patch/20241231065500.168799-1-18255117159@163.com/ > > drivers/misc/pci_endpoint_test.c:315: undefined reference to `__udivmoddi4' That error was for your patch that changed bar_size to resource_size_t, which is typedefed to phys_addr_t, which can be either 32-bits or 64-bits on 32-bit systems (depending on CONFIG_X86_PAE). I was suggesting to just change it to u64, so it will unconditionally be 64-bits. Kind regards, Niklas
On 2025/1/7 19:47, Niklas Cassel wrote: > On Tue, Jan 07, 2025 at 07:43:26PM +0800, Hans Zhang wrote: >> >> >> On 2025/1/7 19:33, Niklas Cassel wrote: >>>> Hi Niklas, >>>> >>>> resource_size_t bar_size; >>>> remain = do_div((u64)bar_size, buf_size); >>>> >>>> It works for the arm platform. >>>> >>>> arch/arm/include/asm/div64.h >>>> static inline uint32_t __div64_32(uint64_t *n, uint32_t base) >>>> { >>>> register unsigned int __base asm("r4") = base; >>>> register unsigned long long __n asm("r0") = *n; >>>> register unsigned long long __res asm("r2"); >>>> unsigned int __rem; >>>> asm( __asmeq("%0", "r0") >>>> __asmeq("%1", "r2") >>>> __asmeq("%2", "r4") >>>> "bl __do_div64" >>>> : "+r" (__n), "=r" (__res) >>>> : "r" (__base) >>>> : "ip", "lr", "cc"); >>>> __rem = __n >> 32; >>>> *n = __res; >>>> return __rem; >>>> } >>>> #define __div64_32 __div64_32 >>>> >>>> #define do_div(n, base) __div64_32(&(n), base) >>>> >>>> >>>> For X86 platforms, do_div is a macro definition, and the first parameter >>>> does not define its type. If the macro definition is replaced directly, an >>>> error will be reported in the ubuntu20.04 release. >>> >>> What is the error? >>> >>> We don't need to use do_div(). >>> The current code that does normal / and % works fine on both >>> 32-bit and 64-bit if you just do: >>> >>> static bool pci_endpoint_test_bar(struct pci_endpoint_test *test, >>> enum pci_barno barno) >>> { >>> - int j, bar_size, buf_size, iters, remain; >>> + int j, buf_size, iters, remain; >>> void *write_buf __free(kfree) = NULL; >>> void *read_buf __free(kfree) = NULL; >>> struct pci_dev *pdev = test->pdev; >>> + u64 bar_size; >>> >>> No? >> >> >> Hi Niklas, >> >> Please look at the robot compilation error. >> >> https://patchwork.kernel.org/project/linux-pci/patch/20241231065500.168799-1-18255117159@163.com/ >> >> drivers/misc/pci_endpoint_test.c:315: undefined reference to `__udivmoddi4' > > That error was for your patch that changed bar_size to resource_size_t, > which is typedefed to phys_addr_t, which can be either 32-bits or 64-bits > on 32-bit systems (depending on CONFIG_X86_PAE). > > I was suggesting to just change it to u64, so it will unconditionally be > 64-bits. Hi Niklas, The robot has been compiled with CONFIG_PHYS_ADDR_T_64BIT=y, so resource_size_t=u64 include/linux/types.h #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; Is my understanding wrong? Could you correct me, please? Thank you very much. config: i386-randconfig-003-20250101 (https://download.01.org/0day-ci/archive/20250101/202501011917.ugP1ywJV-lkp@intel.com/config) I compiled it as a KO module for an experiment. __umoddi3 and __udivdi3 is similar to __udivmoddi4. u64 bar_size; iters = bar_size / buf_size; remain = bar_size % buf_size; zhb@zhb:/media/zhb/hans/code/kernel_org/hans$ make make -C /media/zhb/hans/code/kernel_org/linux/ M=/media/zhb/hans/code/kernel_org/hans modules make[1]: Entering directory '/media/zhb/hans/code/kernel_org/linux' make[2]: Entering directory '/media/zhb/hans/code/kernel_org/hans' CC [M] pci_endpoint_test.o MODPOST Module.symvers ERROR: modpost: "__umoddi3" [pci_endpoint_test.ko] undefined! ERROR: modpost: "__udivdi3" [pci_endpoint_test.ko] undefined! make[4]: *** [/media/zhb/hans/code/kernel_org/linux/scripts/Makefile.modpost:145: Module.symvers] Error 1 make[3]: *** [/media/zhb/hans/code/kernel_org/linux/Makefile:1939: modpost] Error 2 make[2]: *** [/media/zhb/hans/code/kernel_org/linux/Makefile:251: __sub-make] Error 2 make[2]: Leaving directory '/media/zhb/hans/code/kernel_org/hans' make[1]: *** [Makefile:251: __sub-make] Error 2 make[1]: Leaving directory '/media/zhb/hans/code/kernel_org/linux' make: *** [Makefile:10: kernel_modules] Error 2 Best regards Hans
On Tue, Jan 07, 2025 at 08:09:48PM +0800, Hans Zhang wrote: > Hi Niklas, > > The robot has been compiled with CONFIG_PHYS_ADDR_T_64BIT=y, so > resource_size_t=u64 > > > include/linux/types.h > > #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; > > > Is my understanding wrong? Could you correct me, please? Thank you very > much. I see. That is correct. > > config: i386-randconfig-003-20250101 (https://download.01.org/0day-ci/archive/20250101/202501011917.ugP1ywJV-lkp@intel.com/config) > > > I compiled it as a KO module for an experiment. > __umoddi3 and __udivdi3 is similar to __udivmoddi4. > > u64 bar_size; > > iters = bar_size / buf_size; > remain = bar_size % buf_size; I think that I am an idiot (I'm the one who wrote this code). A BAR size is always a power of two. So I don't see how there can ever be a remainer... buf_size = min(SZ_1M, bar_size); If the BAR size is <= 1MB, there will be 1 iteration, no remainder. If the BAR size is > 1MB, there will be more than one iteration, but the size will always be evenly divisible by 1MB, so no remainder. This should probably be two patches: patch 1/2: @@ -316,12 +317,6 @@ static bool pci_endpoint_test_bar(struct pci_endpoint_test *test, write_buf, read_buf, buf_size)) return false; - remain = bar_size % buf_size; - if (remain) - if (pci_endpoint_test_bar_memcmp(test, barno, buf_size * iters, - write_buf, read_buf, remain)) - return false; - return true; } patch 2/2: @@ -283,10 +283,11 @@ static int pci_endpoint_test_bar_memcmp(struct pci_endpoint_test *test, static bool pci_endpoint_test_bar(struct pci_endpoint_test *test, enum pci_barno barno) { - int j, bar_size, buf_size, iters, remain; + int j, buf_size, iters; void *write_buf __free(kfree) = NULL; void *read_buf __free(kfree) = NULL; struct pci_dev *pdev = test->pdev; + resource_size_t bar_size; if (!test->bar[barno]) return false; The error: drivers/misc/pci_endpoint_test.c:315: undefined reference to `__udivmoddi4' sounds like the compiler is using a specialized instruction to do both div and mod in one. By removing the mod in patch 1/2, I expect that patch 2/2 will no longer get this error. Kind regards, Niklas
On 2025/1/7 19:47, Niklas Cassel wrote: Hi Niklas, > The error: > drivers/misc/pci_endpoint_test.c:315: undefined reference to `__udivmoddi4' > sounds like the compiler is using a specialized instruction to do both div > and mod in one. By removing the mod in patch 1/2, I expect that patch 2/2 > will no longer get this error. The __udivmoddi4 may be the way div and mod are combined. Delete remain's patch 1/2 according to your suggestion. I compiled it as a KO module for an experiment. There are still __udivdi3 errors, so the do_div API must be used. zhb@zhb:/media/zhb/hans/code/kernel_org/hans$ make make -C /media/zhb/hans/code/kernel_org/linux/ M=/media/zhb/hans/code/kernel_org/hans modules make[1]: Entering directory '/media/zhb/hans/code/kernel_org/linux' make[2]: Entering directory '/media/zhb/hans/code/kernel_org/hans' CC [M] pci_endpoint_test.o MODPOST Module.symvers ERROR: modpost: "__udivdi3" [pci_endpoint_test.ko] undefined! make[4]: *** [/media/zhb/hans/code/kernel_org/linux/scripts/Makefile.modpost:145: Module.symvers] Error 1 make[3]: *** [/media/zhb/hans/code/kernel_org/linux/Makefile:1939: modpost] Error 2 make[2]: *** [/media/zhb/hans/code/kernel_org/linux/Makefile:251: __sub-make] Error 2 make[2]: Leaving directory '/media/zhb/hans/code/kernel_org/hans' make[1]: *** [Makefile:251: __sub-make] Error 2 make[1]: Leaving directory '/media/zhb/hans/code/kernel_org/linux' make: *** [Makefile:10: kernel_modules] Error 2 Best regards Hans
On Tue, Jan 07, 2025 at 11:44:21PM +0800, Hans Zhang wrote: > > > On 2025/1/7 19:47, Niklas Cassel wrote: > > Hi Niklas, > > > The error: > > drivers/misc/pci_endpoint_test.c:315: undefined reference to `__udivmoddi4' > > sounds like the compiler is using a specialized instruction to do both div > > and mod in one. By removing the mod in patch 1/2, I expect that patch 2/2 > > will no longer get this error. > > The __udivmoddi4 may be the way div and mod are combined. > > Delete remain's patch 1/2 according to your suggestion. I compiled it as a > KO module for an experiment. > > There are still __udivdi3 errors, so the do_div API must be used. Ok. Looking at do_div(), it seems to be the correct API to use for this problem. Just change bar_size type to u64 (instead of casting) and use do_div() ? That is how it is seems to be used in other drivers. I still think that a patch that removes the "remainder" code is a good cleanup, so please send it as patch 1/2, you can be the author, just add: Suggested-by: Niklas Cassel <cassel@kernel.org> Kind regards, Niklas
On 2025/1/7 23:57, Niklas Cassel wrote:>>> The error: >>> drivers/misc/pci_endpoint_test.c:315: undefined reference to `__udivmoddi4' >>> sounds like the compiler is using a specialized instruction to do both div >>> and mod in one. By removing the mod in patch 1/2, I expect that patch 2/2 >>> will no longer get this error. >> >> The __udivmoddi4 may be the way div and mod are combined. >> >> Delete remain's patch 1/2 according to your suggestion. I compiled it as a >> KO module for an experiment. >> >> There are still __udivdi3 errors, so the do_div API must be used. > > Ok. Looking at do_div(), it seems to be the correct API to use > for this problem. Just change bar_size type to u64 (instead of casting) > and use do_div() ? That is how it is seems to be used in other drivers. > > I still think that a patch that removes the "remainder" code is a good > cleanup, so please send it as patch 1/2, you can be the author, just add: > Suggested-by: Niklas Cassel <cassel@kernel.org> > Thank you very much for Niklas' discussion on this patch. I will resubmit two patches in the future. Best regards Hans
diff --git a/drivers/misc/pci_endpoint_test.c b/drivers/misc/pci_endpoint_test.c index 3aaaf47fa4ee..50d4616119af 100644 --- a/drivers/misc/pci_endpoint_test.c +++ b/drivers/misc/pci_endpoint_test.c @@ -280,10 +280,11 @@ static int pci_endpoint_test_bar_memcmp(struct pci_endpoint_test *test, static bool pci_endpoint_test_bar(struct pci_endpoint_test *test, enum pci_barno barno) { - int j, bar_size, buf_size, iters, remain; void *write_buf __free(kfree) = NULL; void *read_buf __free(kfree) = NULL; struct pci_dev *pdev = test->pdev; + int j, buf_size, iters, remain; + resource_size_t bar_size; if (!test->bar[barno]) return false; @@ -307,13 +308,18 @@ static bool pci_endpoint_test_bar(struct pci_endpoint_test *test, if (!read_buf) return false; - iters = bar_size / buf_size; + if (IS_ENABLED(CONFIG_PHYS_ADDR_T_64BIT)) { + remain = do_div(bar_size, buf_size); + iters = bar_size; + } else { + iters = bar_size / buf_size; + remain = 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)) return false; - remain = bar_size % buf_size; if (remain) if (pci_endpoint_test_bar_memcmp(test, barno, buf_size * iters, write_buf, read_buf, remain))