[PATCH] scsi: ips.c: use 64-bit time types

Hi Ebru,

I think you missed the location in which ffdc_time is initially
assigned, and wher it gets compared to the current time:

struct timeval tv;

do_gettimeofday(&tv);
ha->last_ffdc = tv.tv_sec;

This needs to be changed to timespec64 to actually avoid the
overflow problem. I think this one should also use monotonic time,
i.e. ktime_get_ts64 rather than getnstimeofday64.

Arnd

James Bottomley

2014-10-08 20:44:55 UTC

Post by Ebru Akagunduz
This patch changes 32-bit time types to 64-bit in
ips.c
time_t can only represent signed 32-bit dates but
the driver should represent dates that are after
January 2038.
Use time64_t type instead of time_t.
---
drivers/scsi/ips.c | 6 ++++--
drivers/scsi/ips.h | 2 +-
2 files changed, 5 insertions(+), 3 deletions(-)
diff --git a/drivers/scsi/ips.c b/drivers/scsi/ips.c
index 52a216f..8a2cf68 100644
--- a/drivers/scsi/ips.c
+++ b/drivers/scsi/ips.c
@@ -195,6 +195,8 @@
#include <linux/smp.h>
+#include <linux/time64.h>
+
#ifdef MODULE
static char *ips = NULL;
module_param(ips, charp, 0);
@@ -297,7 +299,7 @@ static void ips_freescb(ips_ha_t *, ips_scb_t *);
static void ips_setup_funclist(ips_ha_t *);
static void ips_statinit(ips_ha_t *);
static void ips_statinit_memio(ips_ha_t *);
-static void ips_fix_ffdc_time(ips_ha_t *, ips_scb_t *, time_t);
+static void ips_fix_ffdc_time(ips_ha_t *, ips_scb_t *, time64_t);
static void ips_ffdc_reset(ips_ha_t *, int);
static void ips_ffdc_time(ips_ha_t *);
static uint32_t ips_statupd_copperhead(ips_ha_t *);
@@ -6000,7 +6002,7 @@ ips_ffdc_time(ips_ha_t * ha)
/* */
/****************************************************************************/
static void
-ips_fix_ffdc_time(ips_ha_t * ha, ips_scb_t * scb, time_t current_time)
+ips_fix_ffdc_time(ips_ha_t * ha, ips_scb_t * scb, time64_t current_time)
{
long days;
long rem;
diff --git a/drivers/scsi/ips.h b/drivers/scsi/ips.h
index 45b9566..ff2a0b3 100644
--- a/drivers/scsi/ips.h
+++ b/drivers/scsi/ips.h
@@ -1054,7 +1054,7 @@ typedef struct ips_ha {
uint8_t active;
int ioctl_reset; /* IOCTL Requested Reset Flag */
uint16_t reset_count; /* number of resets */
- time_t last_ffdc; /* last time we sent ffdc info*/
+ time64_t last_ffdc; /* last time we sent ffdc info*/
uint8_t slot_num; /* PCI Slot Number */
int ioctl_len; /* size of ioctl buffer */
dma_addr_t ioctl_busaddr; /* dma address of ioctl buffer*/

This is completely pointless, isn't it? All the ips driver cares about
is that we send a FFDC time update every eight hours or so, so we can
happily truncate the number of seconds to 32 bits for that calculation
just keep the variable at 32 bits and do a time_after thing for the
comparison.

However, what the code *should* be doing is using jiffies and
time_before/after since the interval is so tiny rather than a
do_gettimeofday() call in the fast path.

James

Arnd Bergmann

2014-10-08 20:58:32 UTC

Post by Ebru Akagunduz
diff --git a/drivers/scsi/ips.h b/drivers/scsi/ips.h
index 45b9566..ff2a0b3 100644
--- a/drivers/scsi/ips.h
+++ b/drivers/scsi/ips.h
@@ -1054,7 +1054,7 @@ typedef struct ips_ha {
uint8_t active;
int ioctl_reset; /* IOCTL Requested Reset Flag */
uint16_t reset_count; /* number of resets */
- time_t last_ffdc; /* last time we sent ffdc info*/
+ time64_t last_ffdc; /* last time we sent ffdc info*/
uint8_t slot_num; /* PCI Slot Number */
int ioctl_len; /* size of ioctl buffer */
dma_addr_t ioctl_busaddr; /* dma address of ioctl buffer*/

Good point. The same has come up in a few other places, so I wonder if we
should introduce a proper way to do it that doesn't involve time_t.

While the current code works, we will have to audit 2000 other locations
in which time_t/timespec/timeval are used in the kernel, so we are going
to need some form of annotation to make sure we don't get everyone to
look at the driver again just to come to the same conclusion after working
on a patch first.

Post by James Bottomley
However, what the code *should* be doing is using jiffies and
time_before/after since the interval is so tiny rather than a
do_gettimeofday() call in the fast path.

Yes, this would probably be best for this particular driver, it also
means we end up with a monotonic clock source rather than a wall-clock.

Ebru, when I explained the various data types we have for timekeeping,
I failed to mention jiffies. That is one that is very fast to access
and has a resolution between 1 and 10 milliseconds but will overflow
within a few months, so it can only be used in places where overflow
is known to be handled safely, as time_before() does.

Arnd

James Bottomley

2014-10-09 13:40:26 UTC

Good point. The same has come up in a few other places, so I wonder if we
should introduce a proper way to do it that doesn't involve time_t.

We have, it's jiffies ... that's why I'm slightly non-plussed that this
driver is using gettimeofday for something like this ... it was clearly
a review failure when we put it in.

or are you thinking we need a time_t_time_before doing for time_t what
we do for jiffies?

Post by Arnd Bergmann
While the current code works, we will have to audit 2000 other locations
in which time_t/timespec/timeval are used in the kernel, so we are going
to need some form of annotation to make sure we don't get everyone to
look at the driver again just to come to the same conclusion after working
on a patch first.

Post by James Bottomley
However, what the code *should* be doing is using jiffies and
time_before/after since the interval is so tiny rather than a
do_gettimeofday() call in the fast path.

Yes, this would probably be best for this particular driver, it also
means we end up with a monotonic clock source rather than a wall-clock.

Right, and it's a 32 bit read instead of a system call every time the
thing dispatches a command ... to be honest the overhead of 64 bit
arithmetic is peanuts to making a syscall in the fast path.

James

Post by Arnd Bergmann
Ebru, when I explained the various data types we have for timekeeping,
I failed to mention jiffies. That is one that is very fast to access
and has a resolution between 1 and 10 milliseconds but will overflow
within a few months, so it can only be used in places where overflow
is known to be handled safely, as time_before() does.
Arnd
--
To unsubscribe from this list: send the line "unsubscribe linux-scsi" in
More majordomo info at http://vger.kernel.org/majordomo-info.html

--
To unsubscribe from this list: send the line "unsubscribe linux-scsi" in
the body of a message to ***@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html

Arnd Bergmann

2014-10-09 14:29:52 UTC

Good point. The same has come up in a few other places, so I wonder if we
should introduce a proper way to do it that doesn't involve time_t.

We have, it's jiffies ... that's why I'm slightly non-plussed that this
driver is using gettimeofday for something like this ... it was clearly
a review failure when we put it in.

Actually there is more to it, as I just found upon reading the code
again (I had noticed it before when I first looked at the driver but
then forgotten about it):

ips_fix_ffdc_time() needs the correct current wall-clock time, no overflow
allowed, to stick the year/month/day/hour/minute/second value into
the ffdc command.

My comment to Ebru about ktime_get_ts64 for monotonic time was unfortunately
completely wrong, since that would break whatever timekeeping it is
in the hardware that wants the correct year/month/day/hour/minute/second
values.

Post by James Bottomley
or are you thinking we need a time_t_time_before doing for time_t what
we do for jiffies?

The part I'm interested in is getting rid of any mention of time_t,
timespec and timeval in the kernel by replacing each use with something
that is known to be y2038-safe. Using jiffies correctly would solve
a number of them, but is not sufficient for this driver because of the
ffdc command.

We could use jiffies to test whether we need to send ffdc but then
we still need to read the correct time.

Post by James Bottomley
However, what the code *should* be doing is using jiffies and
time_before/after since the interval is so tiny rather than a
do_gettimeofday() call in the fast path.

Yes, this would probably be best for this particular driver, it also
means we end up with a monotonic clock source rather than a wall-clock.

Right, and it's a 32 bit read instead of a system call every time the
thing dispatches a command ... to be honest the overhead of 64 bit
arithmetic is peanuts to making a syscall in the fast path.

It's not a system call, all we need is a simple function call that reads
tk->xtime_sec. We can use get_seconds() today, but it returns an
'unsigned long', so that won't be enough on 32-bit architectures.

It's still slightly more expensive to do the function call and use a 64-bit
number on a 32-bit CPU, but it's not on the scale of doing a system call
here. You can probably judge best if it's worth the increase in complexity
to use jiffies for determining whether to send the update and then
use get_seconds64 (or similar) to read the wall-clock time, or whether
always using get_seconds64 would be good enough.

Arnd
--
To unsubscribe from this list: send the line "unsubscribe linux-scsi" in
the body of a message to ***@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html

James Bottomley

2014-10-09 15:13:14 UTC

Good point. The same has come up in a few other places, so I wonder if we
should introduce a proper way to do it that doesn't involve time_t.

We have, it's jiffies ... that's why I'm slightly non-plussed that this
driver is using gettimeofday for something like this ... it was clearly
a review failure when we put it in.

Actually there is more to it, as I just found upon reading the code
again (I had noticed it before when I first looked at the driver but
ips_fix_ffdc_time() needs the correct current wall-clock time, no overflow
allowed, to stick the year/month/day/hour/minute/second value into
the ffdc command.

true, but we could call do_gettimeofday() in the routine when we know
we're sending it. And it only does this once every 8 hours. My
complaint is the do_gettimeofday() sitting in the fast path to see if
the eight hours since the last time we sent the ffdc timestamp have
elapsed.

Actually, isn't there a version of the syscall that does return what
this firmware is looking for (the year, month, day, hour, seconds
values)?

Post by Arnd Bergmann
My comment to Ebru about ktime_get_ts64 for monotonic time was unfortunately
completely wrong, since that would break whatever timekeeping it is
in the hardware that wants the correct year/month/day/hour/minute/second
values.

Post by James Bottomley
or are you thinking we need a time_t_time_before doing for time_t what
we do for jiffies?

The part I'm interested in is getting rid of any mention of time_t,
timespec and timeval in the kernel by replacing each use with something
that is known to be y2038-safe. Using jiffies correctly would solve
a number of them, but is not sufficient for this driver because of the
ffdc command.
We could use jiffies to test whether we need to send ffdc but then
we still need to read the correct time.

Right, but it has its own wierd conversion formula, which is dictated by
the HW.

Post by James Bottomley
However, what the code *should* be doing is using jiffies and
time_before/after since the interval is so tiny rather than a
do_gettimeofday() call in the fast path.

Yes, this would probably be best for this particular driver, it also
means we end up with a monotonic clock source rather than a wall-clock.

Right, and it's a 32 bit read instead of a system call every time the
thing dispatches a command ... to be honest the overhead of 64 bit
arithmetic is peanuts to making a syscall in the fast path.

For an 8 hour interval it is provided we have the proper comparisons.

Post by Arnd Bergmann
It's still slightly more expensive to do the function call and use a 64-bit
number on a 32-bit CPU, but it's not on the scale of doing a system call
here. You can probably judge best if it's worth the increase in complexity
to use jiffies for determining whether to send the update and then
use get_seconds64 (or similar) to read the wall-clock time, or whether
always using get_seconds64 would be good enough.

heh, well we need to correct ips_fix_ffdc_time() somehow. I think
converting the trigger mechanism to jiffies makes sense because the
interval is so small and we already have the jiffies code overflow safe.

James

--
To unsubscribe from this list: send the line "unsubscribe linux-scsi" in
the body of a message to ***@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html

Arnd Bergmann

2014-10-09 16:13:51 UTC

Good point. The same has come up in a few other places, so I wonder if we
should introduce a proper way to do it that doesn't involve time_t.

We have, it's jiffies ... that's why I'm slightly non-plussed that this
driver is using gettimeofday for something like this ... it was clearly
a review failure when we put it in.

Ok, fair enough.

Post by James Bottomley
Actually, isn't there a version of the syscall that does return what
this firmware is looking for (the year, month, day, hour, seconds
values)?

Maybe rtc_ktime_to_tm()?

We would need a time64_t version of that anyway.