scsi: stex: Remove use of struct timeval

PATCH RESEND In review Tina Ruchandani 4 participants 8 messages Oct 30, 2015 – Nov 5, 2015 SCSI

Share & tools

Tina Ruchandani

author

2015-10-30 08:30 UTC #0 lore raw

PATCH RESEND v3 1/1 scsi: stex: Remove use of struct timeval

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.

Signed-off-by Tina Ruchandani <[email protected]>

--
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(-)

drivers/scsi/stex.c +3 ~~-10~~

index 98a62bc..84e196e 100644
@@ -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);

Signature · 5 lines

2.2.0.rc0.207.ga3a616c
_______________________________________________
Y2038 mailing list
[email protected]
https://lists.linaro.org/mailman/listinfo/y2038

Johannes Thumshirn

Reviewed-by on v3 1/1

2015-10-30 08:50 UTC #1 lore raw

Hi,

Quoted 4 lines · 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.

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 <[email protected]>

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

Tina Ruchandani

author

2015-10-30 08:54 UTC #2 lore raw

Quoted 6 lines

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

Arnd Bergmann

2015-10-30 23:37 UTC #3 lore raw

Quoted 5 lines · On Friday 30 October 2015 01:54:10 Tina Ruchandani wrote:

Quoted 5 lines
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.

Quoted 9 lines

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

Hannes Reinecke

2015-10-30 11:58 UTC #4 lore raw

In reply to Tina Ruchandani

Quoted 55 lines · Tina Ruchandani

Signed-off-by Tina Ruchandani <[email protected]>

--
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(-)

drivers/scsi/stex.c +2 -9

index 98a62bc..84e196e 100644
@@ -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);

drivers/scsi/stex.c static int stex_ss_handshake(struct st_hba *hba) · line 1075

Quoted 8 lines

drivers/scsi/stex.c +1 -1

@@ -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

Signature · 8 lines

Dr. Hannes Reinecke		      zSeries & Storage
[email protected]			      +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)
_______________________________________________
Y2038 mailing list
[email protected]
https://lists.linaro.org/mailman/listinfo/y2038

Arnd Bergmann

2015-10-30 12:45 UTC #5 lore raw

Quoted 13 lines · On Friday 30 October 2015 12:58:33 Hannes Reinecke wrote:

drivers/scsi/stex.c static int stex_ss_handshake(struct st_hba *hba) · line 1075

Quoted 10 lines

drivers/scsi/stex.c +1 -1

@@ -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

Hannes Reinecke

2015-11-02 07:49 UTC #6 lore raw

Quoted 15 lines · Arnd Bergmann

Quoted 13 lines · On Friday 30 October 2015 12:58:33 Hannes Reinecke wrote:
drivers/scsi/stex.c static int stex_ss_handshake(struct st_hba *hba) · line 1075
Quoted 10 lines
drivers/scsi/stex.c +1 -1
@@ -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.

Quoted 4 lines

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

Arnd Bergmann

Reviewed-by on v3 1/1

2015-11-05 16:23 UTC #7 lore raw

In reply to Tina Ruchandani

Quoted 8 lines · 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 <[email protected]>
Signed-off-by Tina Ruchandani <[email protected]>

For reference

Reviewed-by Arnd Bergmann <[email protected]>

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