diff mbox

[V2,2/3] scsi: fix compiler warning for sg

Message ID 1447034266-28003-3-git-send-email-okaya@codeaurora.org (mailing list archive)
State New, archived
Headers show

Commit Message

Sinan Kaya Nov. 9, 2015, 1:57 a.m. UTC
The MULDIV macro has been designed for small numbers.
Compiler emits an overflow warning on 64 bit systems.
This patch uses 64 bit numbers in order to suppress
warning.

Signed-off-by: Sinan Kaya <okaya@codeaurora.org>
---
 drivers/scsi/sg.c | 20 +++++++++++++-------
 1 file changed, 13 insertions(+), 7 deletions(-)

Comments

Andy Shevchenko Nov. 9, 2015, 2:14 p.m. UTC | #1
On Mon, Nov 9, 2015 at 3:57 AM, Sinan Kaya <okaya@codeaurora.org> wrote:
> The MULDIV macro has been designed for small numbers.
> Compiler emits an overflow warning on 64 bit systems.
> This patch uses 64 bit numbers in order to suppress
> warning.
>
> Signed-off-by: Sinan Kaya <okaya@codeaurora.org>


> ---
>  drivers/scsi/sg.c | 20 +++++++++++++-------
>  1 file changed, 13 insertions(+), 7 deletions(-)
>
> diff --git a/drivers/scsi/sg.c b/drivers/scsi/sg.c
> index 9d7b7db..112d8974 100644
> --- a/drivers/scsi/sg.c
> +++ b/drivers/scsi/sg.c
> @@ -51,6 +51,7 @@ static int sg_version_num = 30536;    /* 2 digits for each component */
>  #include <linux/atomic.h>
>  #include <linux/ratelimit.h>
>  #include <linux/uio.h>
> +#include <asm/div64.h>
>
>  #include "scsi.h"
>  #include <scsi/scsi_dbg.h>
> @@ -85,12 +86,17 @@ static void sg_proc_cleanup(void);
>   * Replacing muldiv(x) by muldiv(x)=((x % d) * m) / d + int(x / d) * m
>   * calculates the same, but prevents the overflow when both m and d
>   * are "small" numbers (like HZ and USER_HZ).
> - * 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 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;

Parens are useless, noticed later, sorry.

Isn't mult_frac() enough here?

Btw, can you mention explicitly what is the warning you get
(copy'n'paste of the line would be okay)?

> +}
>
> -#define SG_DEFAULT_TIMEOUT MULDIV(SG_DEFAULT_TIMEOUT_USER, HZ, USER_HZ)
> +#define SG_DEFAULT_TIMEOUT mult_frac64(SG_DEFAULT_TIMEOUT_USER, HZ, USER_HZ)
>
>  int sg_big_buff = SG_DEF_RESERVED_SIZE;
>  /* N.B. This variable is readable and writeable via
> @@ -877,10 +883,10 @@ sg_ioctl(struct file *filp, unsigned int cmd_in, unsigned long arg)
>                         return result;
>                 if (val < 0)
>                         return -EIO;
> -               if (val >= MULDIV (INT_MAX, USER_HZ, HZ))
> -                   val = MULDIV (INT_MAX, USER_HZ, HZ);
> +               if (val >= mult_frac64(INT_MAX, USER_HZ, HZ))
> +                       val = mult_frac64(INT_MAX, USER_HZ, HZ);


>                 sfp->timeout_user = val;
> -               sfp->timeout = MULDIV (val, HZ, USER_HZ);
> +               sfp->timeout = mult_frac64(val, HZ, USER_HZ);
>
>                 return 0;
>         case SG_GET_TIMEOUT:    /* N.B. User receives timeout as return value */
> --
> 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 Nov. 10, 2015, 3:21 a.m. UTC | #2
On 11/9/2015 9:14 AM, Andy Shevchenko wrote:
> Parens are useless, noticed later, sorry.
>
> Isn't mult_frac() enough here?
>
> Btw, can you mention explicitly what is the warning you get
> (copy'n'paste of the line would be okay)?

I created this patch back in March with an older version of the compiler 
and older kernel (3.19). I'm no longer able to reproduce this with this 
compiler and linux-next.

Thread model: posix
gcc version 4.8.3 20140401 (prerelease) (crosstool-NG 
linaro-1.13.1-4.8-2014.04 - Linaro GCC 4.8-2014.04)

I'll drop this patch.
Timur Tabi Nov. 10, 2015, 3:26 a.m. UTC | #3
Sinan Kaya wrote:
>
> I created this patch back in March with an older version of the compiler
> and older kernel (3.19). I'm no longer able to reproduce this with this
> compiler and linux-next.
>
> Thread model: posix
> gcc version 4.8.3 20140401 (prerelease) (crosstool-NG
> linaro-1.13.1-4.8-2014.04 - Linaro GCC 4.8-2014.04)
>
> I'll drop this patch.

Are you sure the compiler handles the old macro correctly?  Maybe it's 
just quiescing the error message, but it's still broken?
Sinan Kaya Nov. 10, 2015, 4:51 a.m. UTC | #4
On 11/9/2015 10:26 PM, Timur Tabi wrote:
> Sinan Kaya wrote:
>>
>> I created this patch back in March with an older version of the compiler
>> and older kernel (3.19). I'm no longer able to reproduce this with this
>> compiler and linux-next.
>>
>> Thread model: posix
>> gcc version 4.8.3 20140401 (prerelease) (crosstool-NG
>> linaro-1.13.1-4.8-2014.04 - Linaro GCC 4.8-2014.04)
>>
>> I'll drop this patch.
>
> Are you sure the compiler handles the old macro correctly?  Maybe it's
> just quiescing the error message, but it's still broken?
>

The code says it is using these macros for small integers only which 
can't overflow. I was trying to get rid of compiler warning and it seems 
to have disappeared.
Timur Tabi Nov. 10, 2015, 4:53 a.m. UTC | #5
Sinan Kaya wrote:
>
> The code says it is using these macros for small integers only which
> can't overflow. I was trying to get rid of compiler warning and it seems
> to have disappeared.

I would double-check the assembly code, if I were you.  I don't like it 
when warnings just go away like that.

Besides, we *should* be using do_div() for 64-bit division.
Andy Shevchenko Nov. 10, 2015, 9:23 a.m. UTC | #6
On Tue, Nov 10, 2015 at 6:53 AM, Timur Tabi <timur@codeaurora.org> wrote:
> Sinan Kaya wrote:
>>
>>
>> The code says it is using these macros for small integers only which
>> can't overflow. I was trying to get rid of compiler warning and it seems
>> to have disappeared.
>
>
> I would double-check the assembly code, if I were you.  I don't like it when
> warnings just go away like that.

+1 to that.

>
> Besides, we *should* be using do_div() for 64-bit division.

But here looks like all numbers are guaranteed to be less than or
equal to INT_MAX.
Thus, the matter is only to replace MULDIV() by mult_frac() which is
already in kernel.
Arnd Bergmann Nov. 10, 2015, 10:09 a.m. UTC | #7
On Monday 09 November 2015 22:53:17 Timur Tabi wrote:
> Sinan Kaya wrote:
> >
> > The code says it is using these macros for small integers only which
> > can't overflow. I was trying to get rid of compiler warning and it seems
> > to have disappeared.
> 
> I would double-check the assembly code, if I were you.  I don't like it 
> when warnings just go away like that.
> 
> Besides, we *should* be using do_div() for 64-bit division.

I stared at this code for some time and couldn't figure out whether it
is actually safe or not. The point here is that it doesn't actually do
a 64-bit division here:

	MULDIV(INT_MAX, USER_HZ, HZ)

where all arguments are 32bit and it tries to figure out whether the
ioctl argument is too big to fit into a 32-bit number

but it does a 'long' division that happens to be 64-bit long on
architectures with the respective register size when it then does

	sfp->timeout = MULDIV (val, HZ, USER_HZ);

to scale up the argument from USER_HZ to the possibly larger in-kernel
HZ value. So I think it's safe as is, but I'm still not entirely sure.

	Arnd
--
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
diff mbox

Patch

diff --git a/drivers/scsi/sg.c b/drivers/scsi/sg.c
index 9d7b7db..112d8974 100644
--- a/drivers/scsi/sg.c
+++ b/drivers/scsi/sg.c
@@ -51,6 +51,7 @@  static int sg_version_num = 30536;	/* 2 digits for each component */
 #include <linux/atomic.h>
 #include <linux/ratelimit.h>
 #include <linux/uio.h>
+#include <asm/div64.h>
 
 #include "scsi.h"
 #include <scsi/scsi_dbg.h>
@@ -85,12 +86,17 @@  static void sg_proc_cleanup(void);
  * Replacing muldiv(x) by muldiv(x)=((x % d) * m) / d + int(x / d) * m
  * calculates the same, but prevents the overflow when both m and d
  * are "small" numbers (like HZ and USER_HZ).
- * 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 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;
+}
 
-#define SG_DEFAULT_TIMEOUT MULDIV(SG_DEFAULT_TIMEOUT_USER, HZ, USER_HZ)
+#define SG_DEFAULT_TIMEOUT mult_frac64(SG_DEFAULT_TIMEOUT_USER, HZ, USER_HZ)
 
 int sg_big_buff = SG_DEF_RESERVED_SIZE;
 /* N.B. This variable is readable and writeable via
@@ -877,10 +883,10 @@  sg_ioctl(struct file *filp, unsigned int cmd_in, unsigned long arg)
 			return result;
 		if (val < 0)
 			return -EIO;
-		if (val >= MULDIV (INT_MAX, USER_HZ, HZ))
-		    val = MULDIV (INT_MAX, USER_HZ, HZ);
+		if (val >= mult_frac64(INT_MAX, USER_HZ, HZ))
+			val = mult_frac64(INT_MAX, USER_HZ, HZ);
 		sfp->timeout_user = val;
-		sfp->timeout = MULDIV (val, HZ, USER_HZ);
+		sfp->timeout = mult_frac64(val, HZ, USER_HZ);
 
 		return 0;
 	case SG_GET_TIMEOUT:	/* N.B. User receives timeout as return value */