Re: [Y2038] [PATCH 1/2] scsi: 3w-9xxx: Remove use of struct timeval

On Wednesday 28 October 2015 09:10:58 Amitoj Kaur Chawla wrote:

This driver uses 'struct timeval' which we are trying to remove since 32 bit time types will break in the year 2038.

This patch uses ktime_get() instead of do_gettimeofday() which returns ktime_t type.

This patch also uses ktime_to_ms() to find current time in milliseconds.

It looks like you are doing two or three things in this patch, so it would be better to send multiple patches. This driver is a little harder to do than some of the other 'simple' ones I have listed in my table.

@@ -384,8 +383,8 @@ static void twa_aen_queue_event(TW_Device_Extension *tw_dev, TW_Command_Apache_H memset(event, 0, sizeof(TW_Event)); event->severity = TW_SEV_OUT(header->status_block.severity__reserved);

• do_gettimeofday(&time);
• local_time = (u32)(time.tv_sec - (sys_tz.tz_minuteswest * 60));

• local_time = (u32)(ktime_get_real_seconds() -

• 				(sys_tz.tz_minuteswest * 60));
  

  event->time_stamp_sec = local_time; event->aen_code = aen; event->retrieved = TW_AEN_NOT_RETRIEVED;

Unlike what you write in the description, you use ktime_get_real_seconds() rather than ktime_get().

time_stamp_sec is limited to 32 bits, and you handle this correctly, but you do not describe in the changelog what causes the limitation and what this means for the overflow.

@@ -491,8 +489,8 @@ static void twa_aen_sync_time(TW_Device_Extension *tw_dev, int request_id) /* Convert system time in UTC to local time seconds since last Sunday 12:00AM */

• do_gettimeofday(&utc);
• local_time = (u32)(utc.tv_sec - (sys_tz.tz_minuteswest * 60));

• local_time = (u32)(ktime_get_real_seconds() -

• 				(sys_tz.tz_minuteswest * 60));
  

  schedulertime = local_time - (3 * 86400); schedulertime = cpu_to_le32(schedulertime % 604800);

I think here you have the math wrong for the case that the 'u32' overflows. Either explain why that is harmless (if so), or use do_div() to get the modulo from the 64-bit seconds count.

@@ -832,8 +829,7 @@ static long twa_chrdev_ioctl(struct file *file, unsigned int cmd, unsigned long break; case TW_IOCTL_GET_LOCK: tw_lock = (TW_Lock *)tw_ioctl->data_buffer;

• do_gettimeofday(&current_time);
  

• current_time_ms = (current_time.tv_sec * 1000) + (current_time.tv_usec / 1000);
  

• current_time_ms = ktime_to_ms(ktime_get());
  

if ((tw_lock->force_flag == 1) || (tw_dev->ioctl_sem_lock == 0) || (current_time_ms >= tw_dev->ioctl_msec)) { tw_dev->ioctl_sem_lock = 1;

The conversion looks good, but it seems there is a preexisting bug that happens every time the current_time_ms variable overflows, so the current time ends up being less than ioctl_msec. You could try to fix this using the time_before() function, when converting all the ms variables into 'unsigned long'.

Arnd