* [PATCH] scsi: srp_transport: Fix 'always false comparison' in srp_tmo_valid()
@ 2017-01-26 11:17 Augusto Mecking Caringi
2017-01-26 15:11 ` Bart Van Assche
0 siblings, 1 reply; 4+ messages in thread
From: Augusto Mecking Caringi @ 2017-01-26 11:17 UTC (permalink / raw)
To: James E . J . Bottomley
Cc: Augusto Mecking Caringi, Martin K. Petersen, Ewan D. Milne,
Bart Van Assche, Sagi Grimberg, Andy Shevchenko, linux-scsi,
linux-kernel
In a 64bit system (where long is 64bit wide), even dividing LONG_MAX by
HZ will always be bigger than the max value that an int variable can
hold.
This has been detected by building the driver with W=1:
drivers/scsi/scsi_transport_srp.c: In function ‘srp_tmo_valid’:
drivers/scsi/scsi_transport_srp.c:92:19: warning: comparison is always
false due to limited range of data type [-Wtype-limits]
if (dev_loss_tmo >= LONG_MAX / HZ)
^
Signed-off-by: Augusto Mecking Caringi <augustocaringi@gmail.com>
---
drivers/scsi/scsi_transport_srp.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/drivers/scsi/scsi_transport_srp.c b/drivers/scsi/scsi_transport_srp.c
index b87a786..d8c83f4 100644
--- a/drivers/scsi/scsi_transport_srp.c
+++ b/drivers/scsi/scsi_transport_srp.c
@@ -89,7 +89,7 @@ int srp_tmo_valid(int reconnect_delay, int fast_io_fail_tmo, int dev_loss_tmo)
if (fast_io_fail_tmo < 0 &&
dev_loss_tmo > SCSI_DEVICE_BLOCK_MAX_TIMEOUT)
return -EINVAL;
- if (dev_loss_tmo >= LONG_MAX / HZ)
+ if (dev_loss_tmo >= INT_MAX / HZ)
return -EINVAL;
if (fast_io_fail_tmo >= 0 && dev_loss_tmo >= 0 &&
fast_io_fail_tmo >= dev_loss_tmo)
--
2.7.4
^ permalink raw reply related [flat|nested] 4+ messages in thread* Re: [PATCH] scsi: srp_transport: Fix 'always false comparison' in srp_tmo_valid()
2017-01-26 11:17 [PATCH] scsi: srp_transport: Fix 'always false comparison' in srp_tmo_valid() Augusto Mecking Caringi
@ 2017-01-26 15:11 ` Bart Van Assche
2017-01-27 10:06 ` Augusto Mecking Caringi
0 siblings, 1 reply; 4+ messages in thread
From: Bart Van Assche @ 2017-01-26 15:11 UTC (permalink / raw)
To: augustocaringi@gmail.com, jejb@linux.vnet.ibm.com
Cc: linux-scsi@vger.kernel.org, emilne@redhat.com,
andriy.shevchenko@linux.intel.com, linux-kernel@vger.kernel.org,
martin.petersen@oracle.com, sagi@grimberg.me
On Thu, 2017-01-26 at 11:17 +0000, Augusto Mecking Caringi wrote:
> In a 64bit system (where long is 64bit wide), even dividing LONG_MAX by
> HZ will always be bigger than the max value that an int variable can
> hold.
>
> This has been detected by building the driver with W=1:
>
> drivers/scsi/scsi_transport_srp.c: In function ‘srp_tmo_valid’:
> drivers/scsi/scsi_transport_srp.c:92:19: warning: comparison is always
> false due to limited range of data type [-Wtype-limits]
> if (dev_loss_tmo >= LONG_MAX / HZ)
> ^
>
> Signed-off-by: Augusto Mecking Caringi <augustocaringi@gmail.com>
> ---
> drivers/scsi/scsi_transport_srp.c | 2 +-
> 1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/drivers/scsi/scsi_transport_srp.c b/drivers/scsi/scsi_transport_srp.c
> index b87a786..d8c83f4 100644
> --- a/drivers/scsi/scsi_transport_srp.c
> +++ b/drivers/scsi/scsi_transport_srp.c
> @@ -89,7 +89,7 @@ int srp_tmo_valid(int reconnect_delay, int fast_io_fail_tmo, int dev_loss_tmo)
> if (fast_io_fail_tmo < 0 &&
> dev_loss_tmo > SCSI_DEVICE_BLOCK_MAX_TIMEOUT)
> return -EINVAL;
> - if (dev_loss_tmo >= LONG_MAX / HZ)
> + if (dev_loss_tmo >= INT_MAX / HZ)
> return -EINVAL;
> if (fast_io_fail_tmo >= 0 && dev_loss_tmo >= 0 &&
> fast_io_fail_tmo >= dev_loss_tmo)
This patch is wrong. The purpose of the dev_loss_tmo >= LONG_MAX / HZ check
is to avoid that the expression 1UL * dev_loss_tmo * HZ further down
overflows. Can you check whether changing the if-statement into if (1UL *
dev_loss_tmo >= LONG_MAX / HZ) also suppresses the compiler warning?
Bart.
^ permalink raw reply [flat|nested] 4+ messages in thread
* Re: [PATCH] scsi: srp_transport: Fix 'always false comparison' in srp_tmo_valid()
2017-01-26 15:11 ` Bart Van Assche
@ 2017-01-27 10:06 ` Augusto Mecking Caringi
2017-01-27 19:42 ` Bart Van Assche
0 siblings, 1 reply; 4+ messages in thread
From: Augusto Mecking Caringi @ 2017-01-27 10:06 UTC (permalink / raw)
To: Bart Van Assche
Cc: jejb@linux.vnet.ibm.com, linux-scsi@vger.kernel.org,
emilne@redhat.com, andriy.shevchenko@linux.intel.com,
linux-kernel@vger.kernel.org, martin.petersen@oracle.com,
sagi@grimberg.me
On Thu, Jan 26, 2017 at 3:11 PM, Bart Van Assche
<Bart.VanAssche@sandisk.com> wrote:
> This patch is wrong. The purpose of the dev_loss_tmo >= LONG_MAX / HZ check
> is to avoid that the expression 1UL * dev_loss_tmo * HZ further down
> overflows. Can you check whether changing the if-statement into if (1UL *
> dev_loss_tmo >= LONG_MAX / HZ) also suppresses the compiler warning?
Hi Bart,
Right, now a I see...
Doing your proposed change the warning go away...
Do you want me to send a new patch for that?
--
Augusto Mecking Caringi
^ permalink raw reply [flat|nested] 4+ messages in thread* Re: [PATCH] scsi: srp_transport: Fix 'always false comparison' in srp_tmo_valid()
2017-01-27 10:06 ` Augusto Mecking Caringi
@ 2017-01-27 19:42 ` Bart Van Assche
0 siblings, 0 replies; 4+ messages in thread
From: Bart Van Assche @ 2017-01-27 19:42 UTC (permalink / raw)
To: augustocaringi@gmail.com
Cc: jejb@linux.vnet.ibm.com, linux-scsi@vger.kernel.org,
emilne@redhat.com, andriy.shevchenko@linux.intel.com,
linux-kernel@vger.kernel.org, martin.petersen@oracle.com,
sagi@grimberg.me
On Fri, 2017-01-27 at 10:06 +0000, Augusto Mecking Caringi wrote:
> On Thu, Jan 26, 2017 at 3:11 PM, Bart Van Assche
> <Bart.VanAssche@sandisk.com> wrote:
> > This patch is wrong. The purpose of the dev_loss_tmo >= LONG_MAX / HZ check
> > is to avoid that the expression 1UL * dev_loss_tmo * HZ further down
> > overflows. Can you check whether changing the if-statement into if (1UL *
> > dev_loss_tmo >= LONG_MAX / HZ) also suppresses the compiler warning?
>
> Hi Bart,
>
> Right, now a I see...
>
> Doing your proposed change the warning go away...
>
> Do you want me to send a new patch for that?
Hello Augusto,
If you want your patch to go upstream you will have to repost it.
Bart.
^ permalink raw reply [flat|nested] 4+ messages in thread
end of thread, other threads:[~2017-01-27 19:46 UTC | newest]
Thread overview: 4+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2017-01-26 11:17 [PATCH] scsi: srp_transport: Fix 'always false comparison' in srp_tmo_valid() Augusto Mecking Caringi
2017-01-26 15:11 ` Bart Van Assche
2017-01-27 10:06 ` Augusto Mecking Caringi
2017-01-27 19:42 ` Bart Van Assche
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox