Groups | Search | Server Info | Keyboard shortcuts | Login | Register [http] [https] [nntp] [nntps]

Groups > linux.kernel > #1259318 > unrolled thread

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

Back to article view | Back to linux.kernel

Contents

  [RESEND PATCH v3] scsi: stex: Remove use of struct timeval Tina Ruchandani <ruchandani.tina@gmail.com> - 2015-10-30 09:40 +0100
    Re: [RESEND PATCH v3] scsi: stex: Remove use of struct timeval Johannes Thumshirn <jthumshirn@suse.de> - 2015-10-30 10:00 +0100
      Re: [RESEND PATCH v3] scsi: stex: Remove use of struct timeval Tina Ruchandani <ruchandani.tina@gmail.com> - 2015-10-30 10:00 +0100
        Re: [Y2038] [RESEND PATCH v3] scsi: stex: Remove use of struct timeval Arnd Bergmann <arnd@arndb.de> - 2015-10-31 00:40 +0100
    Re: [RESEND PATCH v3] scsi: stex: Remove use of struct timeval Hannes Reinecke <hare@suse.de> - 2015-10-30 13:00 +0100
      Re: [RESEND PATCH v3] scsi: stex: Remove use of struct timeval Arnd Bergmann <arnd@arndb.de> - 2015-10-30 13:50 +0100

#1259318 — [RESEND PATCH v3] 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.

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
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

[toc] | [next] | [standalone]

#1259330

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>


--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

[toc] | [prev] | [next] | [standalone]

#1259335

>
> 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
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

[toc] | [prev] | [next] | [standalone]

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

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
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

[toc] | [prev] | [next] | [standalone]

#1259436

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)
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

[toc] | [prev] | [next] | [standalone]

#1259480

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
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

[toc] | [prev] | [standalone]

Back to top | Article view | linux.kernel

csiph-web