[RESEND PATCH v3] scsi: stex: Remove use of struct timeval

[RESEND PATCH v3] scsi: stex: Remove use of struct timeval

[Tina Ruchandani]

Function stex_gettime uses 'struct timeval' whose tv_sec value
will overflow on 32-bit systems in year 2038 and beyond. This patch
replaces the use of struct timeval and do_gettimeofday with
ktime_get_real_seconds, which returns a 64-bit seconds value.

Suggested-by: Arnd Bergmann <arnd@arndb.de>
Signed-off-by: Tina Ruchandani <ruchandani.tina@gmail.com>
--
Changes in v3:
- Remove stex_gettime altogether, directly assign the timestamp.
Changes in v2:
- Change subject line to indicate that the patch is restricted to stex driver.
---
---
 drivers/scsi/stex.c | 13 +++----------
 1 file changed, 3 insertions(+), 10 deletions(-)

diff --git a/drivers/scsi/stex.c b/drivers/scsi/stex.c
index 98a62bc..84e196e 100644
--- a/drivers/scsi/stex.c
+++ b/drivers/scsi/stex.c
@@ -25,6 +25,7 @@
 #include <linux/types.h>
 #include <linux/module.h>
 #include <linux/spinlock.h>
+#include <linux/ktime.h>
 #include <asm/io.h>
 #include <asm/irq.h>
 #include <asm/byteorder.h>
@@ -362,14 +363,6 @@ MODULE_DESCRIPTION("Promise Technology SuperTrak EX Controllers");
 MODULE_LICENSE("GPL");
 MODULE_VERSION(ST_DRIVER_VERSION);
 
-static void stex_gettime(__le64 *time)
-{
-	struct timeval tv;
-
-	do_gettimeofday(&tv);
-	*time = cpu_to_le64(tv.tv_sec);
-}
-
 static struct status_msg *stex_get_status(struct st_hba *hba)
 {
 	struct status_msg *status = hba->status_buffer + hba->status_tail;
@@ -1002,7 +995,7 @@ static int stex_common_handshake(struct st_hba *hba)
 	h->req_cnt = cpu_to_le16(hba->rq_count+1);
 	h->status_sz = cpu_to_le16(sizeof(struct status_msg));
 	h->status_cnt = cpu_to_le16(hba->sts_count+1);
-	stex_gettime(&h->hosttime);
+	h->hosttime = cpu_to_le64(ktime_get_real_seconds());
 	h->partner_type = HMU_PARTNER_TYPE;
 	if (hba->extra_offset) {
 		h->extra_offset = cpu_to_le32(hba->extra_offset);
@@ -1076,7 +1069,7 @@ static int stex_ss_handshake(struct st_hba *hba)
 	h->req_cnt = cpu_to_le16(hba->rq_count+1);
 	h->status_sz = cpu_to_le16(sizeof(struct status_msg));
 	h->status_cnt = cpu_to_le16(hba->sts_count+1);
-	stex_gettime(&h->hosttime);
+	h->hosttime = cpu_to_le64(ktime_get_real_seconds());
 	h->partner_type = HMU_PARTNER_TYPE;
 	h->extra_offset = h->extra_size = 0;
 	scratch_size = (hba->sts_count+1)*sizeof(u32);
-- 
2.2.0.rc0.207.ga3a616c

Re: [RESEND PATCH v3] scsi: stex: Remove use of struct timeval

[Johannes Thumshirn]

Hi,

On Fri, 2015-10-30 at 01:30 -0700, Tina Ruchandani wrote:
> Function stex_gettime uses 'struct timeval' whose tv_sec value
> will overflow on 32-bit systems in year 2038 and beyond. This patch
> replaces the use of struct timeval and do_gettimeofday with
> ktime_get_real_seconds, which returns a 64-bit seconds value.

Thanks for the conversion. Can you please check if other (scsi) drivers
have the same y2038 issues? A quick "git grep do_gettimeofday
drivers/scsi/  | wc -l" reveals 30 occurrences (of cause not all are
problematic).


Other than that
Reviewed-by: Johannes Thumshirn <jthumshirn@suse.de>

Re: [RESEND PATCH v3] scsi: stex: Remove use of struct timeval

[Tina Ruchandani]

>
> Thanks for the conversion. Can you please check if other (scsi) drivers
> have the same y2038 issues? A quick "git grep do_gettimeofday
> drivers/scsi/  | wc -l" reveals 30 occurrences (of cause not all are
> problematic).
>

Hi Johannes,
Yes, there are quite a few occurrences of timeval within scsi. I had
sent some of the trivial back in the Feb-May 2015 period, and they
were ack-ed by my then mentor and a couple of other people, but not
merged or ack-ed by someone from linux-scsi. Until today, I thought
using "RESEND" would be impolite, but now I will resend the other ones
as well. Arnd Bergmann is leading this effort and may have more
insightful comments.
Thanks,
Tina

Re: [RESEND PATCH v3] scsi: stex: Remove use of struct timeval

[Hannes Reinecke]

On 10/30/2015 09:30 AM, Tina Ruchandani wrote:
> Function stex_gettime uses 'struct timeval' whose tv_sec value
> will overflow on 32-bit systems in year 2038 and beyond. This patch
> replaces the use of struct timeval and do_gettimeofday with
> ktime_get_real_seconds, which returns a 64-bit seconds value.
> 
> Suggested-by: Arnd Bergmann <arnd@arndb.de>
> Signed-off-by: Tina Ruchandani <ruchandani.tina@gmail.com>
> --
> Changes in v3:
> - Remove stex_gettime altogether, directly assign the timestamp.
> Changes in v2:
> - Change subject line to indicate that the patch is restricted to stex driver.
> ---
> ---
>  drivers/scsi/stex.c | 13 +++----------
>  1 file changed, 3 insertions(+), 10 deletions(-)
> 
> diff --git a/drivers/scsi/stex.c b/drivers/scsi/stex.c
> index 98a62bc..84e196e 100644
> --- a/drivers/scsi/stex.c
> +++ b/drivers/scsi/stex.c
> @@ -25,6 +25,7 @@
>  #include <linux/types.h>
>  #include <linux/module.h>
>  #include <linux/spinlock.h>
> +#include <linux/ktime.h>
>  #include <asm/io.h>
>  #include <asm/irq.h>
>  #include <asm/byteorder.h>
> @@ -362,14 +363,6 @@ MODULE_DESCRIPTION("Promise Technology SuperTrak EX Controllers");
>  MODULE_LICENSE("GPL");
>  MODULE_VERSION(ST_DRIVER_VERSION);
>  
> -static void stex_gettime(__le64 *time)
> -{
> -	struct timeval tv;
> -
> -	do_gettimeofday(&tv);
> -	*time = cpu_to_le64(tv.tv_sec);
> -}
> -
>  static struct status_msg *stex_get_status(struct st_hba *hba)
>  {
>  	struct status_msg *status = hba->status_buffer + hba->status_tail;
> @@ -1002,7 +995,7 @@ static int stex_common_handshake(struct st_hba *hba)
>  	h->req_cnt = cpu_to_le16(hba->rq_count+1);
>  	h->status_sz = cpu_to_le16(sizeof(struct status_msg));
>  	h->status_cnt = cpu_to_le16(hba->sts_count+1);
> -	stex_gettime(&h->hosttime);
> +	h->hosttime = cpu_to_le64(ktime_get_real_seconds());
>  	h->partner_type = HMU_PARTNER_TYPE;
>  	if (hba->extra_offset) {
>  		h->extra_offset = cpu_to_le32(hba->extra_offset);
> @@ -1076,7 +1069,7 @@ static int stex_ss_handshake(struct st_hba *hba)
>  	h->req_cnt = cpu_to_le16(hba->rq_count+1);
>  	h->status_sz = cpu_to_le16(sizeof(struct status_msg));
>  	h->status_cnt = cpu_to_le16(hba->sts_count+1);
> -	stex_gettime(&h->hosttime);
> +	h->hosttime = cpu_to_le64(ktime_get_real_seconds());
>  	h->partner_type = HMU_PARTNER_TYPE;
>  	h->extra_offset = h->extra_size = 0;
>  	scratch_size = (hba->sts_count+1)*sizeof(u32);
> 
Just remove 'hosttime' altogether. It serves no purpose whatsoever.

Cheers,

Hannes
-- 
Dr. Hannes Reinecke		      zSeries & Storage
hare@suse.de			      +49 911 74053 688
SUSE LINUX Products GmbH, Maxfeldstr. 5, 90409 Nürnberg
GF: J. Hawn, J. Guild, F. Imendörffer, HRB 16746 (AG Nürnberg)

Re: [RESEND PATCH v3] scsi: stex: Remove use of struct timeval

[Arnd Bergmann]

On Friday 30 October 2015 12:58:33 Hannes Reinecke wrote:
> > @@ -1076,7 +1069,7 @@ static int stex_ss_handshake(struct st_hba *hba)
> >       h->req_cnt = cpu_to_le16(hba->rq_count+1);
> >       h->status_sz = cpu_to_le16(sizeof(struct status_msg));
> >       h->status_cnt = cpu_to_le16(hba->sts_count+1);
> > -     stex_gettime(&h->hosttime);
> > +     h->hosttime = cpu_to_le64(ktime_get_real_seconds());
> >       h->partner_type = HMU_PARTNER_TYPE;
> >       h->extra_offset = h->extra_size = 0;
> >       scratch_size = (hba->sts_count+1)*sizeof(u32);
> > 
> Just remove 'hosttime' altogether. It serves no purpose whatsoever.
> 
> 

Are you sure? It is defined in a struct that is shared with the HBA
as part of hba->dma_mem, so unless you have access to the firmware,
it's hard to guess what it might be used for inside of the firmware.

	Arnd

Re: [Y2038] [RESEND PATCH v3] scsi: stex: Remove use of struct timeval

[Arnd Bergmann]

On Friday 30 October 2015 01:54:10 Tina Ruchandani wrote:
> >
> > Thanks for the conversion. Can you please check if other (scsi) drivers
> > have the same y2038 issues? A quick "git grep do_gettimeofday
> > drivers/scsi/  | wc -l" reveals 30 occurrences (of cause not all are
> > problematic).

In fact all of them are problematic, just for different reasons.

* Some drivers actually overflow in 2038 in a way that causes problems
  in those drivers. These obviously need to be fixed right away.

* A second class of drivers pass time_t/timeval/timespec values to
  or from user space. Even in cases where the absolute numbers are
  small (monotonic times, or time intervals), we have to change them
  to be able to deal with 32-bit user space that will be compiled
  against a modified libc using 64-bit time_t.

* All other driver are likely not broken, but we want to change them
  anyway, to annotate the fact that we have looked at them. My goal
  is to remove the definition of time_t (and all derived structures)
  from the kernel once all drivers have been converted, to ensure that
  we are not adding new broken users, and to have a reasonable
  confidence that we have in actually found the ones that were wrong.

> Hi Johannes,
> Yes, there are quite a few occurrences of timeval within scsi. I had
> sent some of the trivial back in the Feb-May 2015 period, and they
> were ack-ed by my then mentor and a couple of other people, but not
> merged or ack-ed by someone from linux-scsi. Until today, I thought
> using "RESEND" would be impolite, but now I will resend the other ones
> as well. Arnd Bergmann is leading this effort and may have more
> insightful comments.
> 

I provided a "Reviewed-by" tag in https://lkml.org/lkml/2015/5/5/201 .
Normally, when patches get picked up directly from the list, the
person who merges it should add the tags directly.

However, if you have to re-send the patch (with or without
small modifications), you should add that tag after your
Signed-off-by, so it does not get lost when the new patch is
applied.

	Arnd

Re: [RESEND PATCH v3] scsi: stex: Remove use of struct timeval

[Hannes Reinecke]

On 10/30/2015 01:45 PM, Arnd Bergmann wrote:
> On Friday 30 October 2015 12:58:33 Hannes Reinecke wrote:
>>> @@ -1076,7 +1069,7 @@ static int stex_ss_handshake(struct st_hba *hba)
>>>       h->req_cnt = cpu_to_le16(hba->rq_count+1);
>>>       h->status_sz = cpu_to_le16(sizeof(struct status_msg));
>>>       h->status_cnt = cpu_to_le16(hba->sts_count+1);
>>> -     stex_gettime(&h->hosttime);
>>> +     h->hosttime = cpu_to_le64(ktime_get_real_seconds());
>>>       h->partner_type = HMU_PARTNER_TYPE;
>>>       h->extra_offset = h->extra_size = 0;
>>>       scratch_size = (hba->sts_count+1)*sizeof(u32);
>>>
>> Just remove 'hosttime' altogether. It serves no purpose whatsoever.
>>
>>
> 
> Are you sure? It is defined in a struct that is shared with the HBA
> as part of hba->dma_mem, so unless you have access to the firmware,
> it's hard to guess what it might be used for inside of the firmware.
> 
Ah, you're right. Okay, so we'll need to convert it.

Cheers,

Hannes

Re: [Y2038] [RESEND PATCH v3] scsi: stex: Remove use of struct timeval

[Arnd Bergmann]

On Friday 30 October 2015 01:30:40 Tina Ruchandani wrote:
> Function stex_gettime uses 'struct timeval' whose tv_sec value
> will overflow on 32-bit systems in year 2038 and beyond. This patch
> replaces the use of struct timeval and do_gettimeofday with
> ktime_get_real_seconds, which returns a 64-bit seconds value.
> 
> Suggested-by: Arnd Bergmann <arnd@arndb.de>
> Signed-off-by: Tina Ruchandani <ruchandani.tina@gmail.com>
> 

For reference

Reviewed-by: Arnd Bergmann <arnd@arndb.de>

I'm putting this into my y2038 tree so I don't forget it, but I'd hope
that James can apply it into scsi and I'll drop it again.

	Arnd