2017-12-09 00:34:22

by Colin King

Subject: [PATCH][next] scsi: arcmsr: remove redundant check for secs < 0

From: Colin Ian King <[email protected]>

The check for secs being less than zero is redundant for two reasons.
Firstly, secs is unsigned so the check is always going to be false.
Secondly, if secs was signed the proceeding calculation of secs is
never going to be negative. Hence we can remove this redundant check
and day and secs re-adjustment.

Detected by static analysis with smatch:
arcmsr_set_iop_datetime() warn: unsigned 'secs' is never less than zero.

Signed-off-by: Colin Ian King <[email protected]>
---
drivers/scsi/arcmsr/arcmsr_hba.c | 4 ----
1 file changed, 4 deletions(-)

diff --git a/drivers/scsi/arcmsr/arcmsr_hba.c b/drivers/scsi/arcmsr/arcmsr_hba.c
index 0707a60bf5c0..e4258b69f4be 100644
--- a/drivers/scsi/arcmsr/arcmsr_hba.c
+++ b/drivers/scsi/arcmsr/arcmsr_hba.c
@@ -3679,10 +3679,6 @@ static void arcmsr_set_iop_datetime(struct timer_list *t)
secs = (u32)(tv.tv_sec - (sys_tz.tz_minuteswest * 60));
days = secs / 86400;
secs = secs - 86400 * days;
- if (secs < 0) {
- days = days - 1;
- secs = secs + 86400;
- }
j = days / 146097;
i = days - 146097 * j;
a = i + 719468;
--
2.14.1

2017-12-11 02:41:10

by Ching Huang

Subject: Re: [PATCH][next] scsi: arcmsr: remove redundant check for secs < 0

Colin,

You are right. That's checking is redundant. secs is never be negative.
Thanks for your correction and patch.

Regards,
Ching

2017-12-09 8:34 GMT+08:00 Colin King <[email protected]>:
> From: Colin Ian King <[email protected]>
>
> The check for secs being less than zero is redundant for two reasons.
> Firstly, secs is unsigned so the check is always going to be false.
> Secondly, if secs was signed the proceeding calculation of secs is
> never going to be negative. Hence we can remove this redundant check
> and day and secs re-adjustment.
>
> Detected by static analysis with smatch:
> arcmsr_set_iop_datetime() warn: unsigned 'secs' is never less than zero.
>
> Signed-off-by: Colin Ian King <[email protected]>
> ---
> drivers/scsi/arcmsr/arcmsr_hba.c | 4 ----
> 1 file changed, 4 deletions(-)
>
> diff --git a/drivers/scsi/arcmsr/arcmsr_hba.c b/drivers/scsi/arcmsr/arcmsr_hba.c
> index 0707a60bf5c0..e4258b69f4be 100644
> --- a/drivers/scsi/arcmsr/arcmsr_hba.c
> +++ b/drivers/scsi/arcmsr/arcmsr_hba.c
> @@ -3679,10 +3679,6 @@ static void arcmsr_set_iop_datetime(struct timer_list *t)
> secs = (u32)(tv.tv_sec - (sys_tz.tz_minuteswest * 60));
> days = secs / 86400;
> secs = secs - 86400 * days;
> - if (secs < 0) {
> - days = days - 1;
> - secs = secs + 86400;
> - }
> j = days / 146097;
> i = days - 146097 * j;
> a = i + 719468;
> --
> 2.14.1
>

2017-12-12 02:38:05

by Martin K. Petersen

Subject: Re: [PATCH][next] scsi: arcmsr: remove redundant check for secs < 0

Colin,

> The check for secs being less than zero is redundant for two reasons.
> Firstly, secs is unsigned so the check is always going to be false.
> Secondly, if secs was signed the proceeding calculation of secs is
> never going to be negative. Hence we can remove this redundant check
> and day and secs re-adjustment.

Applied to 4.16/scsi-queue, thanks!

--
Martin K. Petersen Oracle Linux Engineering

2017-12-15 17:52:50

by walter harms

Subject: Re: [PATCH][next] scsi: arcmsr: remove redundant check for secs < 0

Am 09.12.2017 01:34, schrieb Colin King:
> From: Colin Ian King <[email protected]>
>
> The check for secs being less than zero is redundant for two reasons.
> Firstly, secs is unsigned so the check is always going to be false.
> Secondly, if secs was signed the proceeding calculation of secs is
> never going to be negative. Hence we can remove this redundant check
> and day and secs re-adjustment.
>
> Detected by static analysis with smatch:
> arcmsr_set_iop_datetime() warn: unsigned 'secs' is never less than zero.
>
> Signed-off-by: Colin Ian King <[email protected]>
> ---
> drivers/scsi/arcmsr/arcmsr_hba.c | 4 ----
> 1 file changed, 4 deletions(-)
>
> diff --git a/drivers/scsi/arcmsr/arcmsr_hba.c b/drivers/scsi/arcmsr/arcmsr_hba.c
> index 0707a60bf5c0..e4258b69f4be 100644
> --- a/drivers/scsi/arcmsr/arcmsr_hba.c
> +++ b/drivers/scsi/arcmsr/arcmsr_hba.c
> @@ -3679,10 +3679,6 @@ static void arcmsr_set_iop_datetime(struct timer_list *t)
> secs = (u32)(tv.tv_sec - (sys_tz.tz_minuteswest * 60));
> days = secs / 86400;
> secs = secs - 86400 * days;

mmh, what they want to do is:
secs = secs % 86400 ;

> - if (secs < 0) {
> - days = days - 1;
> - secs = secs + 86400;
> - }
> j = days / 146097;
> i = days - 146097 * j;
> a = i + 719468;

see above.
btw: are the numbers documented ?

re,
wh