Groups | Search | Server Info | Keyboard shortcuts | Login | Register [http] [https] [nntp] [nntps]

Groups > linux.kernel > #1265271 > unrolled thread

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

Back to article view | Back to linux.kernel

This discussion starts older than the indexed window; earlier articles aren't shown. The article labeled Started by below is the oldest one visible, not the original post.

Contents

  [PATCH V2 2/3] scsi: fix compiler warning for sg Sinan Kaya <okaya@codeaurora.org> - 2015-11-09 03:00 +0100
    Re: [PATCH V2 2/3] scsi: fix compiler warning for sg Andy Shevchenko <andy.shevchenko@gmail.com> - 2015-11-09 15:20 +0100
      Re: [PATCH V2 2/3] scsi: fix compiler warning for sg Timur Tabi <timur@codeaurora.org> - 2015-11-10 04:30 +0100
        Re: [PATCH V2 2/3] scsi: fix compiler warning for sg Timur Tabi <timur@codeaurora.org> - 2015-11-10 06:00 +0100
          Re: [PATCH V2 2/3] scsi: fix compiler warning for sg Andy Shevchenko <andy.shevchenko@gmail.com> - 2015-11-10 10:30 +0100
          Re: [PATCH V2 2/3] scsi: fix compiler warning for sg Arnd Bergmann <arnd@arndb.de> - 2015-11-10 11:20 +0100
        Re: [PATCH V2 2/3] scsi: fix compiler warning for sg Sinan Kaya <okaya@codeaurora.org> - 2015-11-10 06:00 +0100
      Re: [PATCH V2 2/3] scsi: fix compiler warning for sg Sinan Kaya <okaya@codeaurora.org> - 2015-11-10 04:30 +0100

#1265271 — [PATCH V2 2/3] scsi: fix compiler warning for sg

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;
+}
 
-#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-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

[toc] | [next] | [standalone]

#1265710

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



-- 
With Best Regards,
Andy Shevchenko
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

[toc] | [prev] | [next] | [standalone]

#1266199

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?

-- 
Sent by an employee of the Qualcomm Innovation Center, Inc.
The Qualcomm Innovation Center, Inc. is a member of the
Code Aurora Forum, hosted by The Linux Foundation.
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

[toc] | [prev] | [next] | [standalone]

#1266239

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.

-- 
Sent by an employee of the Qualcomm Innovation Center, Inc.
The Qualcomm Innovation Center, Inc. is a member of the
Code Aurora Forum, hosted by The Linux Foundation.
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

[toc] | [prev] | [next] | [standalone]

#1266348

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.

-- 
With Best Regards,
Andy Shevchenko
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

[toc] | [prev] | [next] | [standalone]

#1266380

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-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

[toc] | [prev] | [next] | [standalone]

#1266247

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.

-- 
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
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

[toc] | [prev] | [next] | [standalone]

#1266204

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.

-- 
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
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

[toc] | [prev] | [standalone]

Back to top | Article view | linux.kernel

csiph-web