| Message ID | 1446698789-19308-3-git-send-email-okaya@codeaurora.org (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
Hi Sinan,
[auto build test ERROR on: scsi/for-next]
[also build test ERROR on: v4.3 next-20151104]
url: https://github.com/0day-ci/linux/commits/Sinan-Kaya/scsi-mpt2sas-try-64-bit-DMA-when-32-bit-DMA-fails/20151105-125248
base: https://git.kernel.org/pub/scm/linux/kernel/git/jejb/scsi.git for-next
config: i386-defconfig (attached as .config)
reproduce:
# save the attached .config to linux build tree
make ARCH=i386
All errors (new ones prefixed by >>):
drivers/built-in.o: In function `sg_ioctl':
>> sg.c:(.text+0x1b680b): undefined reference to `__umoddi3'
>> sg.c:(.text+0x1b6829): undefined reference to `__udivdi3'
sg.c:(.text+0x1b6849): undefined reference to `__udivdi3'
---
0-DAY kernel test infrastructure Open Source Technology Center
https://lists.01.org/pipermail/kbuild-all Intel Corporation
Hi Sinan,
[auto build test ERROR on: scsi/for-next]
[also build test ERROR on: v4.3 next-20151105]
url: https://github.com/0day-ci/linux/commits/Sinan-Kaya/scsi-mpt2sas-try-64-bit-DMA-when-32-bit-DMA-fails/20151105-125248
base: https://git.kernel.org/pub/scm/linux/kernel/git/jejb/scsi.git for-next
config: arm-mv78xx0_defconfig (attached as .config)
reproduce:
wget https://git.kernel.org/cgit/linux/kernel/git/wfg/lkp-tests.git/plain/sbin/make.cross -O ~/bin/make.cross
chmod +x ~/bin/make.cross
# save the attached .config to linux build tree
make.cross ARCH=arm
All errors (new ones prefixed by >>):
>> ERROR: "__aeabi_uldivmod" [drivers/scsi/sg.ko] undefined!
---
0-DAY kernel test infrastructure Open Source Technology Center
https://lists.01.org/pipermail/kbuild-all Intel Corporation
Hi Sinan,
[auto build test ERROR on: scsi/for-next]
[also build test ERROR on: v4.3 next-20151105]
url: https://github.com/0day-ci/linux/commits/Sinan-Kaya/scsi-mpt2sas-try-64-bit-DMA-when-32-bit-DMA-fails/20151105-125248
base: https://git.kernel.org/pub/scm/linux/kernel/git/jejb/scsi.git for-next
config: powerpc-mpc8610_hpcd_defconfig (attached as .config)
reproduce:
wget https://git.kernel.org/cgit/linux/kernel/git/wfg/lkp-tests.git/plain/sbin/make.cross -O ~/bin/make.cross
chmod +x ~/bin/make.cross
# save the attached .config to linux build tree
make.cross ARCH=powerpc
All errors (new ones prefixed by >>):
drivers/built-in.o: In function `sg_ioctl':
>> drivers/scsi/sg.c:897: undefined reference to `__umoddi3'
>> drivers/scsi/sg.c:897: undefined reference to `__udivdi3'
>> drivers/scsi/sg.c:897: undefined reference to `__udivdi3'
vim +897 drivers/scsi/sg.c
^1da177e Linus Torvalds 2005-04-16 891 return sfp->timeout_user;
^1da177e Linus Torvalds 2005-04-16 892 case SG_SET_FORCE_LOW_DMA:
^1da177e Linus Torvalds 2005-04-16 893 result = get_user(val, ip);
^1da177e Linus Torvalds 2005-04-16 894 if (result)
^1da177e Linus Torvalds 2005-04-16 895 return result;
^1da177e Linus Torvalds 2005-04-16 896 if (val) {
^1da177e Linus Torvalds 2005-04-16 @897 sfp->low_dma = 1;
^1da177e Linus Torvalds 2005-04-16 898 if ((0 == sfp->low_dma) && (0 == sg_res_in_use(sfp))) {
^1da177e Linus Torvalds 2005-04-16 899 val = (int) sfp->reserve.bufflen;
95e159d6 Hannes Reinecke 2014-06-25 900 sg_remove_scat(sfp, &sfp->reserve);
:::::: The code at line 897 was first introduced by commit
:::::: 1da177e4c3f41524e886b7f1b8a0c1fc7321cac2 Linux-2.6.12-rc2
:::::: TO: Linus Torvalds <torvalds@ppc970.osdl.org>
:::::: CC: Linus Torvalds <torvalds@ppc970.osdl.org>
---
0-DAY kernel test infrastructure Open Source Technology Center
https://lists.01.org/pipermail/kbuild-all Intel Corporation
On Thu, Nov 5, 2015 at 6:46 AM, Sinan Kaya <okaya@codeaurora.org> wrote: > The MULDIV macro has been designed for small > numbers. It emits an overflow warning on 64 bit > systems. This patch places type casts in the > parameters to fix the compiler warning. > > Signed-off-by: Sinan Kaya <okaya@codeaurora.org> > --- > drivers/scsi/sg.c | 5 ++++- > 1 file changed, 4 insertions(+), 1 deletion(-) > > diff --git a/drivers/scsi/sg.c b/drivers/scsi/sg.c > index 9d7b7db..eb2739d 100644 > --- a/drivers/scsi/sg.c > +++ b/drivers/scsi/sg.c > @@ -88,7 +88,10 @@ static void sg_proc_cleanup(void); > * Of course an overflow is inavoidable if the result of muldiv doesn't fit > * in 32 bits. > */ > -#define MULDIV(X,MUL,DIV) ((((X % DIV) * MUL) / DIV) + ((X / DIV) * MUL)) > +static inline u64 MULDIV(u64 X, u32 MUL, u32 DIV) > +{ > + return ((((X % DIV) * MUL) / DIV) + ((X / DIV) * MUL)); > +} Like kbuild bot already told you it would be nice to think of 32-bit architectures. Moreover we have mult_frac() macro already for 32-bit numbers. For 64 bit numbers you need to do do_div(). Like: static inline u64 mult_frac64(u64 x, u32 m, u32 n) { u64 ret; ret = do_div(x, n); return ret * m; } > > #define SG_DEFAULT_TIMEOUT MULDIV(SG_DEFAULT_TIMEOUT_USER, HZ, USER_HZ) > > -- > Qualcomm Technologies, Inc. on behalf of Qualcomm Innovation Center, Inc. > Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum, a Linux Foundation Collaborative Project > > -- > To unsubscribe from this list: send the line "unsubscribe linux-scsi" in > the body of a message to majordomo@vger.kernel.org > More majordomo info at http://vger.kernel.org/majordomo-info.html
On 11/5/2015 3:48 AM, Andy Shevchenko wrote: > On Thu, Nov 5, 2015 at 6:46 AM, Sinan Kaya <okaya@codeaurora.org> wrote: >> The MULDIV macro has been designed for small >> numbers. It emits an overflow warning on 64 bit >> systems. This patch places type casts in the >> parameters to fix the compiler warning. >> >> Signed-off-by: Sinan Kaya <okaya@codeaurora.org> >> --- >> drivers/scsi/sg.c | 5 ++++- >> 1 file changed, 4 insertions(+), 1 deletion(-) >> >> diff --git a/drivers/scsi/sg.c b/drivers/scsi/sg.c >> index 9d7b7db..eb2739d 100644 >> --- a/drivers/scsi/sg.c >> +++ b/drivers/scsi/sg.c >> @@ -88,7 +88,10 @@ static void sg_proc_cleanup(void); >> * Of course an overflow is inavoidable if the result of muldiv doesn't fit >> * in 32 bits. >> */ >> -#define MULDIV(X,MUL,DIV) ((((X % DIV) * MUL) / DIV) + ((X / DIV) * MUL)) >> +static inline u64 MULDIV(u64 X, u32 MUL, u32 DIV) >> +{ >> + return ((((X % DIV) * MUL) / DIV) + ((X / DIV) * MUL)); >> +} > > Like kbuild bot already told you it would be nice to think of 32-bit > architectures. > > Moreover we have mult_frac() macro already for 32-bit numbers. > > For 64 bit numbers you need to do do_div(). > > Like: > > static inline u64 mult_frac64(u64 x, u32 m, u32 n) > { > u64 ret; > > ret = do_div(x, n); > return ret * m; > } > OK, I didn't know that we had such a macro. To make this look like the other macro, I can do this. static inline u64 mult_frac64(u64 x, u32 numer, u32 denom) { u64 quot; u64 rem = x % denom; u64 rem2; quot = x; do_div(quot, denom); rem2 = rem * numer; do_div(rem2, denom); return (quot * numer) + rem2; } #define MULDIV(X,MUL,DIV) mult_frac64(X, MUL, DIV) > >> >> #define SG_DEFAULT_TIMEOUT MULDIV(SG_DEFAULT_TIMEOUT_USER, HZ, USER_HZ) >> >> -- >> Qualcomm Technologies, Inc. on behalf of Qualcomm Innovation Center, Inc. >> Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum, a Linux Foundation Collaborative Project >> >> -- >> To unsubscribe from this list: send the line "unsubscribe linux-scsi" in >> the body of a message to majordomo@vger.kernel.org >> More majordomo info at http://vger.kernel.org/majordomo-info.html > > >
Sinan Kaya wrote: > > > #define MULDIV(X,MUL,DIV) mult_frac64(X, MUL, DIV) Why bother with the macro at all? Just change the code to use do_div() directly. It's possible that the original code was written before do_div() became standard, or the developer didn't know about, which is why we have this macro in the first place.
On Thu, Nov 5, 2015 at 5:10 PM, Sinan Kaya <okaya@codeaurora.org> wrote: > > > On 11/5/2015 3:48 AM, Andy Shevchenko wrote: >> >> On Thu, Nov 5, 2015 at 6:46 AM, Sinan Kaya <okaya@codeaurora.org> wrote: >>> >>> The MULDIV macro has been designed for small >>> numbers. It emits an overflow warning on 64 bit >>> systems. This patch places type casts in the >>> parameters to fix the compiler warning. >>> >>> Signed-off-by: Sinan Kaya <okaya@codeaurora.org> >>> --- >>> drivers/scsi/sg.c | 5 ++++- >>> 1 file changed, 4 insertions(+), 1 deletion(-) >>> >>> diff --git a/drivers/scsi/sg.c b/drivers/scsi/sg.c >>> index 9d7b7db..eb2739d 100644 >>> --- a/drivers/scsi/sg.c >>> +++ b/drivers/scsi/sg.c >>> @@ -88,7 +88,10 @@ static void sg_proc_cleanup(void); >>> * Of course an overflow is inavoidable if the result of muldiv doesn't >>> fit >>> * in 32 bits. >>> */ >>> -#define MULDIV(X,MUL,DIV) ((((X % DIV) * MUL) / DIV) + ((X / DIV) * >>> MUL)) >>> +static inline u64 MULDIV(u64 X, u32 MUL, u32 DIV) >>> +{ >>> + return ((((X % DIV) * MUL) / DIV) + ((X / DIV) * MUL)); >>> +} >> >> >> Like kbuild bot already told you it would be nice to think of 32-bit >> architectures. >> >> Moreover we have mult_frac() macro already for 32-bit numbers. >> >> For 64 bit numbers you need to do do_div(). >> >> Like: >> >> static inline u64 mult_frac64(u64 x, u32 m, u32 n) >> { >> u64 ret; >> >> ret = do_div(x, n); >> return ret * m; >> } >> > > OK, I didn't know that we had such a macro. To make this look like the other > macro, I can do this. > > static inline u64 mult_frac64(u64 x, u32 numer, u32 denom) > { > u64 quot; > u64 rem = x % denom; > u64 rem2; > > quot = x; > do_div(quot, denom); > > rem2 = rem * numer; > do_div(rem2, denom); > > return (quot * numer) + rem2; > } Might be I did a wrong smaple, but do_div() returns two values actually. You perhaps overlooked it and thus wrote something redundant above. > > #define MULDIV(X,MUL,DIV) mult_frac64(X, MUL, DIV) > >> >>> >>> #define SG_DEFAULT_TIMEOUT MULDIV(SG_DEFAULT_TIMEOUT_USER, HZ, USER_HZ) >>> >>> -- >>> Qualcomm Technologies, Inc. on behalf of Qualcomm Innovation Center, Inc. >>> Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum, a >>> Linux Foundation Collaborative Project >>> >>> -- >>> To unsubscribe from this list: send the line "unsubscribe linux-scsi" in >>> the body of a message to majordomo@vger.kernel.org >>> More majordomo info at http://vger.kernel.org/majordomo-info.html >> >> >> >> > > -- > Sinan Kaya > Qualcomm Technologies, Inc. on behalf of Qualcomm Innovation Center, Inc. > Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum, a Linux > Foundation Collaborative Project
On 11/5/2015 1:07 PM, Andy Shevchenko wrote: >> OK, I didn't know that we had such a macro. To make this look like the other >> >macro, I can do this. >> > >> >static inline u64 mult_frac64(u64 x, u32 numer, u32 denom) >> >{ >> > u64 quot; >> > u64 rem = x % denom; >> > u64 rem2; >> > >> > quot = x; >> > do_div(quot, denom); >> > >> > rem2 = rem * numer; >> > do_div(rem2, denom); >> > >> > return (quot * numer) + rem2; >> >} > Might be I did a wrong smaple, but do_div() returns two values actually. > You perhaps overlooked it and thus wrote something redundant above. > OK, I was looking at example usages in the kernel. The ones I looked always used the first argument as an input & output parameter. I got nervous about overwriting something. void __ndelay(unsigned long long nsecs) { u64 end; nsecs <<= 9; do_div(nsecs, 125); ... } Let's try again. static inline u64 mult_frac64(u64 x, u32 numer, u32 denom) { u64 rem = x % denom; u64 quot = do_div(x, denom); u64 mul = rem * numer; return (quot * numer) + do_div(mul, denom); } I'll do a s/MULDIV/mult_frac64/g to address Timur's concern.
On Thu, Nov 5, 2015 at 8:32 PM, Sinan Kaya <okaya@codeaurora.org> wrote: > On 11/5/2015 1:07 PM, Andy Shevchenko wrote: > Let's try again. > > static inline u64 mult_frac64(u64 x, u32 numer, u32 denom) { > u64 rem = x % denom; > u64 quot = do_div(x, denom); > u64 mul = rem * numer; > > return (quot * numer) + do_div(mul, denom); > } First of all why not to put this to generic header? We have math64.h and kernel.h. Might be a good idea (needs to check current users) to move mult_frac to math64.h. Then, x % y is already a problem. After all, you seems messed quot and remainder. What about something like #if BITS_PER_LONG == 64 #define mult_frac64(x,n,d) mult_frac(x,n,d) #else static inline u64 mult_frac64(u64 x, u32 numer, u32 denom) { u64 r1 = do_div(x, denom); u64 r2 = r1 * numer; do_div(r2, denom); return (x * numer) + r2; } #endif ?
On Thu, Nov 5, 2015 at 9:31 PM, Andy Shevchenko <andy.shevchenko@gmail.com> wrote: > On Thu, Nov 5, 2015 at 8:32 PM, Sinan Kaya <okaya@codeaurora.org> wrote: >> On 11/5/2015 1:07 PM, Andy Shevchenko wrote: > >> Let's try again. >> >> static inline u64 mult_frac64(u64 x, u32 numer, u32 denom) { >> u64 rem = x % denom; >> u64 quot = do_div(x, denom); >> u64 mul = rem * numer; >> >> return (quot * numer) + do_div(mul, denom); >> } > > First of all why not to put this to generic header? We have math64.h > and kernel.h. > Might be a good idea (needs to check current users) to move mult_frac > to math64.h. > > Then, x % y is already a problem. After all, you seems messed quot and > remainder. > > What about something like > > #if BITS_PER_LONG == 64 > > #define mult_frac64(x,n,d) mult_frac(x,n,d) > > #else > > static inline u64 mult_frac64(u64 x, u32 numer, u32 denom) { > u64 r1 = do_div(x, denom); > u64 r2 = r1 * numer; > > do_div(r2, denom); > return (x * numer) + r2; > } > > #endif > > ? One more look to the users of MULDIV. They all seems 32 bit for x. It means you don't need two do_div()s at all. Just do something like: u64 d = x * numer; do_div(d, denom); return d;
On 11/5/2015 2:56 PM, Andy Shevchenko wrote: > One more look to the users of MULDIV. > > They all seems 32 bit for x. > It means you don't need two do_div()s at all. > > Just do something like: > > u64 d = x * numer; > do_div(d, denom); > return d; OK. I assume you want a change only in this file.
On 11/5/2015 2:56 PM, Andy Shevchenko wrote: > On Thu, Nov 5, 2015 at 9:31 PM, Andy Shevchenko > <andy.shevchenko@gmail.com> wrote: >> On Thu, Nov 5, 2015 at 8:32 PM, Sinan Kaya <okaya@codeaurora.org> wrote: >>> On 11/5/2015 1:07 PM, Andy Shevchenko wrote: >> >>> Let's try again. >>> >>> static inline u64 mult_frac64(u64 x, u32 numer, u32 denom) { >>> u64 rem = x % denom; >>> u64 quot = do_div(x, denom); >>> u64 mul = rem * numer; >>> >>> return (quot * numer) + do_div(mul, denom); >>> } >> >> First of all why not to put this to generic header? We have math64.h >> and kernel.h. >> Might be a good idea (needs to check current users) to move mult_frac >> to math64.h. >> >> Then, x % y is already a problem. After all, you seems messed quot and >> remainder. >> >> What about something like >> >> #if BITS_PER_LONG == 64 >> >> #define mult_frac64(x,n,d) mult_frac(x,n,d) >> >> #else >> >> static inline u64 mult_frac64(u64 x, u32 numer, u32 denom) { >> u64 r1 = do_div(x, denom); >> u64 r2 = r1 * numer; >> >> do_div(r2, denom); >> return (x * numer) + r2; >> } I'll use this instead. This is cleaner, scalable and functionally correct to the original code. I'll post a patch with this soon. >> >> #endif >> >> ? > > One more look to the users of MULDIV. > > They all seems 32 bit for x. > It means you don't need two do_div()s at all. > > Just do something like: > > u64 d = x * numer; > do_div(d, denom); > return d; >
diff --git a/drivers/scsi/sg.c b/drivers/scsi/sg.c index 9d7b7db..eb2739d 100644 --- a/drivers/scsi/sg.c +++ b/drivers/scsi/sg.c @@ -88,7 +88,10 @@ static void sg_proc_cleanup(void); * Of course an overflow is inavoidable if the result of muldiv doesn't fit * in 32 bits. */ -#define MULDIV(X,MUL,DIV) ((((X % DIV) * MUL) / DIV) + ((X / DIV) * MUL)) +static inline u64 MULDIV(u64 X, u32 MUL, u32 DIV) +{ + return ((((X % DIV) * MUL) / DIV) + ((X / DIV) * MUL)); +} #define SG_DEFAULT_TIMEOUT MULDIV(SG_DEFAULT_TIMEOUT_USER, HZ, USER_HZ)
The MULDIV macro has been designed for small numbers. It emits an overflow warning on 64 bit systems. This patch places type casts in the parameters to fix the compiler warning. Signed-off-by: Sinan Kaya <okaya@codeaurora.org> --- drivers/scsi/sg.c | 5 ++++- 1 file changed, 4 insertions(+), 1 deletion(-)