[PATCH] staging: ft1000: replace timeval with ktime_t

[PATCH] staging: ft1000: replace timeval with ktime_t

[Deepa Dinamani]

Replace use of struct timeval with ktime_t to calculate connection
duration. This avoids y2038 issues. Use ktime_get monotonic clock
instead of gettimeofday realtime clock to guard against negative
durations. Fix pr_debug to use matching %ul instead of converting
value to int, so that debug does not provide misleading information.
Fix connection time conversion to use interface type unsigned long
instead of assuming it maps to u32.

Signed-off-by: Deepa Dinamani <deepa.kernel@gmail.com>
---
 drivers/staging/ft1000/ft1000-pcmcia/ft1000_hw.c | 8 +++-----
 drivers/staging/ft1000/ft1000-usb/ft1000_debug.c | 8 ++++----
 drivers/staging/ft1000/ft1000-usb/ft1000_hw.c    | 8 +++-----
 drivers/staging/ft1000/ft1000.h                  | 2 +-
 4 files changed, 11 insertions(+), 15 deletions(-)

diff --git a/drivers/staging/ft1000/ft1000-pcmcia/ft1000_hw.c b/drivers/staging/ft1000/ft1000-pcmcia/ft1000_hw.c
index eecfa37..540b896 100644
--- a/drivers/staging/ft1000/ft1000-pcmcia/ft1000_hw.c
+++ b/drivers/staging/ft1000/ft1000-pcmcia/ft1000_hw.c
@@ -906,7 +906,6 @@ static void ft1000_proc_drvmsg(struct net_device *dev)
 	struct prov_record *ptr;
 	struct pseudo_hdr *ppseudo_hdr;
 	u16 *pmsg;
-	struct timeval tv;
 	union {
 		u8 byte[2];
 		u16 wrd;
@@ -983,8 +982,7 @@ static void ft1000_proc_drvmsg(struct net_device *dev)
 						netif_carrier_on(dev);
 						netif_wake_queue(dev);
 						info->mediastate = 1;
-						do_gettimeofday(&tv);
-						info->ConTm = tv.tv_sec;
+						info->ConTm = ktime_get();
 					}
 				} else {
 					pr_debug("Media is down\n");
@@ -992,7 +990,7 @@ static void ft1000_proc_drvmsg(struct net_device *dev)
 						info->mediastate = 0;
 						netif_carrier_off(dev);
 						netif_stop_queue(dev);
-						info->ConTm = 0;
+						info->ConTm = ktime_set(0, 0);
 					}
 				}
 			} else {
@@ -1001,7 +999,7 @@ static void ft1000_proc_drvmsg(struct net_device *dev)
 					info->mediastate = 0;
 					netif_carrier_off(dev);
 					netif_stop_queue(dev);
-					info->ConTm = 0;
+					info->ConTm = ktime_set(0, 0);
 				}
 			}
 			break;
diff --git a/drivers/staging/ft1000/ft1000-usb/ft1000_debug.c b/drivers/staging/ft1000/ft1000-usb/ft1000_debug.c
index f241a3a..d535c13 100644
--- a/drivers/staging/ft1000/ft1000-usb/ft1000_debug.c
+++ b/drivers/staging/ft1000/ft1000-usb/ft1000_debug.c
@@ -413,7 +413,7 @@ static long ft1000_ioctl(struct file *file, unsigned int command,
 	int i;
 	u16 tempword;
 	unsigned long flags;
-	struct timeval tv;
+	ktime_t con_tm;
 	struct IOCTL_GET_VER get_ver_data;
 	struct IOCTL_GET_DSP_STAT get_stat_data;
 	u8 ConnectionMsg[] = {
@@ -520,9 +520,9 @@ static long ft1000_ioctl(struct file *file, unsigned int command,
 		get_stat_data.nRxPkts = info->stats.rx_packets;
 		get_stat_data.nTxBytes = info->stats.tx_bytes;
 		get_stat_data.nRxBytes = info->stats.rx_bytes;
-		do_gettimeofday(&tv);
-		get_stat_data.ConTm = (u32)(tv.tv_sec - info->ConTm);
-		pr_debug("Connection Time = %d\n", (int)get_stat_data.ConTm);
+		con_tm = ktime_sub(ktime_get(), info->ConTm);
+		get_stat_data.ConTm = (unsigned long)ktime_divns(con_tm, NSEC_PER_SEC);
+		pr_debug("Connection Time = %lu\n", get_stat_data.ConTm);
 		if (copy_to_user(argp, &get_stat_data, sizeof(get_stat_data))) {
 			pr_debug("copy fault occurred\n");
 			result = -EFAULT;
diff --git a/drivers/staging/ft1000/ft1000-usb/ft1000_hw.c b/drivers/staging/ft1000/ft1000-usb/ft1000_hw.c
index 9ea32ce..633f525 100644
--- a/drivers/staging/ft1000/ft1000-usb/ft1000_hw.c
+++ b/drivers/staging/ft1000/ft1000-usb/ft1000_hw.c
@@ -619,7 +619,6 @@ static int ft1000_open(struct net_device *dev)
 {
 	struct ft1000_info *pInfo = netdev_priv(dev);
 	struct ft1000_usb *pFt1000Dev = pInfo->priv;
-	struct timeval tv;
 
 	pr_debug("ft1000_open is called for card %d\n", pFt1000Dev->CardNumber);
 
@@ -627,8 +626,7 @@ static int ft1000_open(struct net_device *dev)
 	pInfo->stats.tx_bytes = 0;
 	pInfo->stats.rx_packets = 0;
 	pInfo->stats.tx_packets = 0;
-	do_gettimeofday(&tv);
-	pInfo->ConTm = tv.tv_sec;
+	pInfo->ConTm = ktime_get();
 	pInfo->ProgConStat = 0;
 
 	netif_start_queue(dev);
@@ -1163,14 +1161,14 @@ static int ft1000_proc_drvmsg(struct ft1000_usb *dev, u16 size)
 				if (info->mediastate == 1) {
 					info->mediastate = 0;
 					if (dev->NetDevRegDone)
-						info->ConTm = 0;
+						info->ConTm = ktime_set(0, 0);
 				}
 			}
 		} else {
 			pr_debug("Media is down\n");
 			if (info->mediastate == 1) {
 				info->mediastate = 0;
-				info->ConTm = 0;
+				info->ConTm = ktime_set(0, 0);
 			}
 		}
 		break;
diff --git a/drivers/staging/ft1000/ft1000.h b/drivers/staging/ft1000/ft1000.h
index 8a2e4ca..77b3f6e 100644
--- a/drivers/staging/ft1000/ft1000.h
+++ b/drivers/staging/ft1000/ft1000.h
@@ -347,7 +347,7 @@ struct ft1000_info {
 	u8 HwSerNum[HWSERNUMSZ];	/* Hardware Serial Number */
 	u8 Sku[SKUSZ];			/* SKU */
 	u8 eui64[EUISZ];		/* EUI64 */
-	time_t ConTm;			/* Connection Time */
+	ktime_t ConTm;			/* Connection Time */
 	u8 ProductMode[MODESZ];
 	u8 RfCalVer[CALVERSZ];
 	u8 RfCalDate[CALDATESZ];
-- 
1.9.1

Re: [Y2038] [PATCH] staging: ft1000: replace timeval with ktime_t

[Arnd Bergmann]

On Wednesday 14 October 2015 17:43:00 Deepa Dinamani wrote:
> Replace use of struct timeval with ktime_t to calculate connection
> duration. This avoids y2038 issues. Use ktime_get monotonic clock
> instead of gettimeofday realtime clock to guard against negative
> durations. Fix pr_debug to use matching %ul instead of converting
> value to int, so that debug does not provide misleading information.
> Fix connection time conversion to use interface type unsigned long
> instead of assuming it maps to u32.
> 
> Signed-off-by: Deepa Dinamani <deepa.kernel@gmail.com>

The change looks correct, and your changelog also looks reasonable.

However, I notice that the code only ever looks at the second
portion of the time stamp. Therefore it would be nicer to use
ktime_get_seconds() to avoid the expensive ktime_divns(), and to
slightly simplify the code further.

You can use either time64_t or a shorter type like 'u32' or 'long'
to store monotonic seconds, so pick one of them and explain why
you have that one.

	Arnd

Re: [Outreachy kernel] Re: [Y2038] [PATCH] staging: ft1000: replace timeval with ktime_t

[Deepa Dinamani]

[-- Attachment #1: Type: text/plain, Size: 2220 bytes --]

Forgot to do reply-all earlier.

Resending.

I considered using the ktime_get_seconds() earlier.
However, I'm not convinced that the driver actually needs time in seconds.
It would be hard to guess given that I'm not actually running this on a
platform.
So I used ktime_get() instead so that when the driver gets cleaned up
later, it can be updated to use correct time granularity.

Given that sometimes they could end up calculating time from epoch seems to
hint that there is more cleanup required on the driver.

Do you think this is reasonable?

Thanks,
Deepa

On Fri, Oct 16, 2015 at 3:14 PM, Arnd Bergmann <arnd@arndb.de> wrote:

> On Wednesday 14 October 2015 17:43:00 Deepa Dinamani wrote:
> > Replace use of struct timeval with ktime_t to calculate connection
> > duration. This avoids y2038 issues. Use ktime_get monotonic clock
> > instead of gettimeofday realtime clock to guard against negative
> > durations. Fix pr_debug to use matching %ul instead of converting
> > value to int, so that debug does not provide misleading information.
> > Fix connection time conversion to use interface type unsigned long
> > instead of assuming it maps to u32.
> >
> > Signed-off-by: Deepa Dinamani <deepa.kernel@gmail.com>
>
> The change looks correct, and your changelog also looks reasonable.
>
> However, I notice that the code only ever looks at the second
> portion of the time stamp. Therefore it would be nicer to use
> ktime_get_seconds() to avoid the expensive ktime_divns(), and to
> slightly simplify the code further.
>
> You can use either time64_t or a shorter type like 'u32' or 'long'
> to store monotonic seconds, so pick one of them and explain why
> you have that one.
>
>         Arnd
>
> --
> You received this message because you are subscribed to the Google Groups
> "outreachy-kernel" group.
> To unsubscribe from this group and stop receiving emails from it, send an
> email to outreachy-kernel+unsubscribe@googlegroups.com.
> To post to this group, send email to outreachy-kernel@googlegroups.com.
> To view this discussion on the web visit
> https://groups.google.com/d/msgid/outreachy-kernel/30046181.iodvR1OlJa%40wuerfel
> .
> For more options, visit https://groups.google.com/d/optout.
>

[-- Attachment #2: Type: text/html, Size: 3405 bytes --]

Re: [Outreachy kernel] Re: [Y2038] [PATCH] staging: ft1000: replace timeval with ktime_t

[Arnd Bergmann]

On Friday 16 October 2015 15:53:04 Deepa Dinamani wrote:
> Forgot to do reply-all earlier.
> 
> Resending.
> 
> I considered using the ktime_get_seconds() earlier.
> However, I'm not convinced that the driver actually needs time in seconds.
> It would be hard to guess given that I'm not actually running this on a
> platform.
> So I used ktime_get() instead so that when the driver gets cleaned up
> later, it can be updated to use correct time granularity.
> 
> Given that sometimes they could end up calculating time from epoch seems to
> hint that there is more cleanup required on the driver.
> 
> Do you think this is reasonable?
> 

The driver only uses the time in one place, which is the ioctl function
returning the connection time in seconds, and this is a debugging
interface.

From this, we know that there is little value in using a more accurate
time representation. Your approach gives us better rounding, but it
won't really matter as the connection time for a wireless connection
is not interesting in the low seconds anyway.

Regarding future cleanups, you should not introduce features just
because you think they might be used later: Code is easy to change,
so if someone needs the higher resolution, they can change it then.
In this case, it's particularly unlikely to change, as the only
user is in an ioctl. Making a change to that interface would break
existing binaries, so we don't want that.

A more likely cleanup would be the removal of the debug interface,
replacing it with something completely different.

	Arnd

Re: [Y2038] [Outreachy kernel] Re: [PATCH] staging: ft1000: replace timeval with ktime_t

[Arnd Bergmann]

On Saturday 17 October 2015 00:57:45 Arnd Bergmann wrote:

> A more likely cleanup would be the removal of the debug interface,
> replacing it with something completely different.

A little more research shows that we can probably remove the driver
from the kernel really soon, as the only remaining operator is
shutting down its service:

http://www.gtigroup.org/news/ind/2015-08-18/6996.html

They just went from a nationwide service to one that is only
available in the two largest cities of the country, and they
already offer much faster LTE service there.

	Arnd

Re: [Y2038] [Outreachy kernel] Re: [PATCH] staging: ft1000: replace timeval with ktime_t

[Deepa Dinamani]

Should I then fix this patch as per your suggestion or let it be?

-Deepa

> On Oct 16, 2015, at 4:30 PM, Arnd Bergmann <arnd@arndb.de> wrote:
> 
>> On Saturday 17 October 2015 00:57:45 Arnd Bergmann wrote:
>> 
>> A more likely cleanup would be the removal of the debug interface,
>> replacing it with something completely different.
> 
> A little more research shows that we can probably remove the driver
> from the kernel really soon, as the only remaining operator is
> shutting down its service:
> 
> http://www.gtigroup.org/news/ind/2015-08-18/6996.html
> 
> They just went from a nationwide service to one that is only
> available in the two largest cities of the country, and they
> already offer much faster LTE service there.
> 
>    Arnd

Re: [Y2038] [Outreachy kernel] Re: [PATCH] staging: ft1000: replace timeval with ktime_t

[Greg KH]

On Fri, Oct 16, 2015 at 05:32:25PM -0700, Deepa Dinamani wrote:
> Should I then fix this patch as per your suggestion or let it be?

Let's just delete the driver from the tree.

thanks,

greg k-h

Re: [Y2038] [Outreachy kernel] Re: [PATCH] staging: ft1000: replace timeval with ktime_t

[Arnd Bergmann]

On Friday 16 October 2015 22:55:11 Greg KH wrote:
> On Fri, Oct 16, 2015 at 05:32:25PM -0700, Deepa Dinamani wrote:
> > Should I then fix this patch as per your suggestion or let it be?
> 
> Let's just delete the driver from the tree.

Ok.

Deepa, can you send a patch to remove the driver with an explanation
of why it's no longer used?

	Arnd

Re: [Y2038] [Outreachy kernel] Re: [PATCH] staging: ft1000: replace timeval with ktime_t

[Deepa Dinamani]

> On Oct 17, 2015, at 04:29, Arnd Bergmann <arnd@arndb.de> wrote:
> 
>> On Friday 16 October 2015 22:55:11 Greg KH wrote:
>>> On Fri, Oct 16, 2015 at 05:32:25PM -0700, Deepa Dinamani wrote:
>>> Should I then fix this patch as per your suggestion or let it be?
>> 
>> Let's just delete the driver from the tree.
> 
> Ok.
> 
> Deepa, can you send a patch to remove the driver with an explanation
> of why it's no longer used?

Sure. Will do.

-Deepa