On Thursday 29 October 2015 22:40:03 Alison Schofield wrote:

Replace the use of struct timeval and do_gettimeofday() with 64 bit ktime_get_real_seconds. Prevents 32-bit type overflow in year 2038 on 32-bit systems.

Signed-off-by: Alison Schofield amsfield22@gmail.com

In the subject line, please be more specific and name the driver, not just the subsystem.

The patch looks correct, but it would be nice to explain in the changelog how the time is used here (it gets passed to the firmware as 64-bit milliseconds).

@@ -45,6 +45,7 @@ #include <asm/processor.h> #include <linux/libata.h> #include <linux/mutex.h> +#include <linux/ktime.h> #include <scsi/scsi.h> #include <scsi/scsi_host.h> #include <scsi/scsi_device.h> @@ -5563,11 +5564,9 @@ static void pmcraid_set_timestamp(struct pmcraid_cmd *cmd) __be32 time_stamp_len = cpu_to_be32(PMCRAID_TIMESTAMP_LEN); struct pmcraid_ioadl_desc *ioadl = ioarcb->add_data.u.ioadl;

• struct timeval tv; __le64 timestamp;

• do_gettimeofday(&tv);
• timestamp = tv.tv_sec * 1000;

• timestamp = ktime_get_real_seconds() * 1000;

This is a nice simplification on top of the fix.

I see that the driver for some reason leaves the sub-second portion of the 64-bit milliseconds as zero, which is a bit odd. Did you notice that too?

What I'd do in a case like this is to list in the changelog an alternative approach, so the maintainer can decide whether to apply the patch as-is or to ask for the alternative. The other approach would be to fix the milliseconds as well and do:

struct timespec64 ts;

ktime_get_real_ts64(&ts); timestamp = ts.tv_sec * MSEC_PER_SEC + ts.tv_nsec / NSEC_PER_MSEC;

Arnd

Replace the use of struct timeval and do_gettimeofday() with 64 bit ktime_get_real_seconds. Prevents 32-bit type overflow in year 2038 on 32-bit systems.

Driver was using the seconds portion of struct timeval (.tv_secs) to pass a millseconds timestamp to the firmware. This change maintains that same behavior using ktime_get_real_seconds.

The structure used to pass the timestamp to firmware is 48 bits and works fine as long as the top 16 bits are zero and they will be zero for a long time..ie. thousands of years.

Signed-off-by: Alison Schofield amsfield22@gmail.com ---

Changes in v2: * add pmcraid to subject line * update commit msg w additional detail * add alternative change proposal per reviewer feedback

Alternative Change: Add sub second granularity to timestamp

As noted above, the driver only used the seconds portion of timeval, ignores the microseconds portion, and by multiplying by 1000 effectively does a <<10 and always writes zero into timestamp[0].

The alternative change would pass all the bits to the firmware:

struct timespec64 ts;

ktime_get_real_ts64(&ts); timestamp = ts.tv_sec * MSEC_PER_SEC + ts.tv_nsec / NSEC_PER_MSEC;

drivers/scsi/pmcraid.c | 5 ++--- 1 file changed, 2 insertions(+), 3 deletions(-)

diff --git a/drivers/scsi/pmcraid.c b/drivers/scsi/pmcraid.c index ed31d8c..3f64275 100644 --- a/drivers/scsi/pmcraid.c +++ b/drivers/scsi/pmcraid.c @@ -45,6 +45,7 @@ #include <asm/processor.h> #include <linux/libata.h> #include <linux/mutex.h> +#include <linux/ktime.h> #include <scsi/scsi.h> #include <scsi/scsi_host.h> #include <scsi/scsi_device.h> @@ -5563,11 +5564,9 @@ static void pmcraid_set_timestamp(struct pmcraid_cmd *cmd) __be32 time_stamp_len = cpu_to_be32(PMCRAID_TIMESTAMP_LEN); struct pmcraid_ioadl_desc *ioadl = ioarcb->add_data.u.ioadl;

- struct timeval tv; __le64 timestamp;

- do_gettimeofday(&tv); - timestamp = tv.tv_sec * 1000; + timestamp = ktime_get_real_seconds() * 1000;

pinstance->timestamp_data->timestamp[0] = (__u8)(timestamp); pinstance->timestamp_data->timestamp[1] = (__u8)((timestamp) >> 8);