staging: ft1000: replace timeval with ktime_t

PATCH In review Deepa Dinamani 3 participants 9 messages Oct 15, 2015 – Oct 17, 2015 Staging

Share & tools

Deepa Dinamani

author

2015-10-15 00:44 UTC #0 lore raw

PATCH 1/1 staging: ft1000: replace timeval with ktime_t

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

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

drivers/staging/ft1000/ft1000-pcmcia/ft1000_hw.c +3 -5

index eecfa37..540b896 100644
@@ -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;

drivers/staging/ft1000/ft1000-usb/ft1000_debug.c +4 -4

index f241a3a..d535c13 100644
@@ -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;

drivers/staging/ft1000/ft1000-usb/ft1000_hw.c +3 -5

index 9ea32ce..633f525 100644
@@ -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;

drivers/staging/ft1000/ft1000.h +1 -1

index 8a2e4ca..77b3f6e 100644
@@ -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];

Signature · 1 line

1.9.1

Arnd Bergmann

2015-10-16 22:14 UTC #1 lore raw

Quoted 9 lines · 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 <[email protected]>

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

Deepa Dinamani

author

2015-10-16 22:53 UTC #2 lore raw

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

Quoted 35 lines · Arnd Bergmann

Quoted 9 lines · 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 <[email protected]>
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 [email protected].
To post to this group, send email to [email protected]. 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.

Arnd Bergmann

2015-10-16 22:57 UTC #3 lore raw

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

Quoted 5 lines

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

Arnd Bergmann

2015-10-16 23:31 UTC #4 lore raw

Quoted 2 lines · 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

Deepa Dinamani

author

2015-10-17 00:32 UTC #5 lore raw

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

-Deepa

Quoted 18 lines · Arnd Bergmann

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

Greg KH

maintainer

2015-10-17 05:55 UTC #6 lore raw

Quoted 1 line · Deepa Dinamani

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

Arnd Bergmann

2015-10-17 11:29 UTC #7 lore raw

Quoted 4 lines · On Friday 16 October 2015 22:55:11 Greg KH wrote:

Quoted 1 line · Deepa Dinamani
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

Deepa Dinamani

author

2015-10-17 18:23 UTC #8 lore raw

Quoted 8 lines · Arnd Bergmann

Quoted 5 lines · On Friday 16 October 2015 22:55:11 Greg KH wrote:
Quoted 2 lines · Deepa Dinamani
Should I then fix this patch as per your suggestion or let it be?

Quoted 6 lines

Quoted 1 line
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