drivers/misc/pci_endpoint_test.c | 12 +++++++++--- 1 file changed, 9 insertions(+), 3 deletions(-)
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>
---
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.1On 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:
> 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: 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 Tue, Jan 7, 2025, at 16:57, Niklas Cassel wrote:
> 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 think using div_u64_rem() instead of do_div() would make this
more readable as this is always an inline function, so the type can
remain resource_size_t, and the division gets optimized well when
that is a 32-bit type.
Arnd
On Wed, Jan 08, 2025 at 03:10:03PM +0100, Arnd Bergmann wrote: > On Tue, Jan 7, 2025, at 16:57, Niklas Cassel wrote: > > 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 think using div_u64_rem() instead of do_div() would make this > more readable as this is always an inline function, so the type can > remain resource_size_t, and the division gets optimized well when > that is a 32-bit type. After patch 1/2, we no longer care about the remainder, so I guess div64_u64() is the correct function to use then? Kind regards, Niklas
On 2025/1/8 22:13, Niklas Cassel wrote:
Hi Niklas,
>>> 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 think using div_u64_rem() instead of do_div() would make this
>> more readable as this is always an inline function, so the type can
>> remain resource_size_t, and the division gets optimized well when
>> that is a 32-bit type.
>
> After patch 1/2, we no longer care about the remainder, so I guess
> div64_u64() is the correct function to use then?
>
include/asm-generic/bitsperlong.h
#ifdef CONFIG_64BIT
#define BITS_PER_LONG 64
#else
#define BITS_PER_LONG 32
#endif /* CONFIG_64BIT */
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;
include/linux/math64.h
#if BITS_PER_LONG == 64
......
static inline u64 div64_u64(u64 dividend, u64 divisor)
{
return dividend / divisor;
}
......
#elif BITS_PER_LONG == 32
#ifndef div64_u64
extern u64 div64_u64(u64 dividend, u64 divisor);
#endif
......
#endif /* BITS_PER_LONG */
lib/math/div64.c
u64 div64_u64(u64 dividend, u64 divisor)
{
u32 high = divisor >> 32;
u64 quot;
if (high == 0) {
quot = div_u64(dividend, divisor);
} else {
int n = fls(high);
quot = div_u64(dividend >> n, divisor >> n);
if (quot != 0)
quot--;
if ((dividend - quot * divisor) >= divisor)
quot++;
}
return quot;
}
EXPORT_SYMBOL(div64_u64);
If CONFIG_64BIT and CONFIG_PHYS_ADDR_T_64BIT are not configured. The
resource_size_t=u32, and the first parameter of div64_u64 is u64.The
same question is as follows:
https://patchwork.kernel.org/project/linux-pci/patch/20250102120222.1403906-1-18255117159@163.com/
config: arm-defconfig
(https://download.01.org/0day-ci/archive/20250103/202501030414.p0DE9xNK-lkp@intel.com/config)
All error/warnings (new ones prefixed by >>):
>> drivers/misc/pci_endpoint_test.c:311:11: warning: comparison of
distinct pointer types ('typeof ((bar_size)) *' (aka 'unsigned int *')
and 'uint64_t *' (aka 'unsigned long long *'))
[-Wcompare-distinct-pointer-types]
311 | remain = do_div(bar_size, buf_size);
| ^~~~~~~~~~~~~~~~~~~~~~~~~~
include/asm-generic/div64.h:183:28: note: expanded from macro 'do_div'
183 | (void)(((typeof((n)) *)0) == ((uint64_t *)0)); \
| ~~~~~~~~~~~~~~~~~~ ^ ~~~~~~~~~~~~~~~
>> drivers/misc/pci_endpoint_test.c:311:11: error: incompatible pointer
types passing 'resource_size_t *' (aka 'unsigned int *') to parameter of
type 'uint64_t *' (aka 'unsigned long long *')
[-Werror,-Wincompatible-pointer-types]
311 | remain = do_div(bar_size, buf_size);
| ^~~~~~~~~~~~~~~~~~~~~~~~~~
include/asm-generic/div64.h:199:22: note: expanded from macro 'do_div'
199 | __rem = __div64_32(&(n), __base); \
| ^~~~
arch/arm/include/asm/div64.h:24:45: note: passing argument to
parameter 'n' here
24 | static inline uint32_t __div64_32(uint64_t *n, uint32_t base)
| ^
>> drivers/misc/pci_endpoint_test.c:311:11: warning: shift count >=
width of type [-Wshift-count-overflow]
311 | remain = do_div(bar_size, buf_size);
| ^~~~~~~~~~~~~~~~~~~~~~~~~~
include/asm-generic/div64.h:195:25: note: expanded from macro 'do_div'
195 | } else if (likely(((n) >> 32) == 0)) { \
| ^ ~~
include/linux/compiler.h:76:40: note: expanded from macro 'likely'
76 | # define likely(x) __builtin_expect(!!(x), 1)
| ^
2 warnings and 1 error generated.
Best regards
Hans
On Thu, Jan 9, 2025, at 03:59, Hans Zhang wrote:
> On 2025/1/8 22:13, Niklas Cassel wrote:
>>>> 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 think using div_u64_rem() instead of do_div() would make this
>>> more readable as this is always an inline function, so the type can
>>> remain resource_size_t, and the division gets optimized well when
>>> that is a 32-bit type.
>>
>> After patch 1/2, we no longer care about the remainder, so I guess
>> div64_u64() is the correct function to use then?
div_u64() is the correct interface here, div64_u64() is the
even slower version where both arguments are 64-bit wide.
> >> drivers/misc/pci_endpoint_test.c:311:11: warning: comparison of
> distinct pointer types ('typeof ((bar_size)) *' (aka 'unsigned int *')
> and 'uint64_t *' (aka 'unsigned long long *'))
> [-Wcompare-distinct-pointer-types]
> 311 | remain = do_div(bar_size, buf_size);
> | ^~~~~~~~~~~~~~~~~~~~~~~~~~
You don't use div_u64() or div64_u64() here, do_div() is the macro
version that must be called with a 64-bit argument.
Arnd
On 2025/1/9 14:29, Arnd Bergmann wrote:
>> On 2025/1/8 22:13, Niklas Cassel wrote:
>>>>> 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 think using div_u64_rem() instead of do_div() would make this
>>>> more readable as this is always an inline function, so the type can
>>>> remain resource_size_t, and the division gets optimized well when
>>>> that is a 32-bit type.
>>>
>>> After patch 1/2, we no longer care about the remainder, so I guess
>>> div64_u64() is the correct function to use then?
>
> div_u64() is the correct interface here, div64_u64() is the
> even slower version where both arguments are 64-bit wide.
>
>> >> drivers/misc/pci_endpoint_test.c:311:11: warning: comparison of
>> distinct pointer types ('typeof ((bar_size)) *' (aka 'unsigned int *')
>> and 'uint64_t *' (aka 'unsigned long long *'))
>> [-Wcompare-distinct-pointer-types]
>> 311 | remain = do_div(bar_size, buf_size);
>> | ^~~~~~~~~~~~~~~~~~~~~~~~~~
>
> You don't use div_u64() or div64_u64() here, do_div() is the macro
> version that must be called with a 64-bit argument.
>
> Arnd
Thank you so much Niklas and Arnd.
Best regards
Hans
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
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/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
© 2016 - 2026 Red Hat, Inc.