[PATCH] staging: gasket remove current_kernel_time usage

[PATCH] staging: gasket remove current_kernel_time usage

[Arnd Bergmann]

A new user of the deprecated current_kernel_time() function has appeared
here. This code won't work correct during leap seconds or a concurrent
settimeofday() call, and it probably doesn't do what the author intended
even for the normal case, as it passes a timeout in nanoseconds but
reads the time using a jiffies-granularity accessor.

I'm changing it to ktime_get_ns() here, which simplifies the logic,
and uses a high-res clocksource. This is a bit slower, but that
probably doesn't matter in a busy-wait loop.

Note: it also doesn't matter in the current version, as there are no
callers of this function.

Fixes: 9a69f5087ccc ("drivers/staging: Gasket driver framework + Apex driver")
Signed-off-by: Arnd Bergmann <arnd@arndb.de>
---
 drivers/staging/gasket/gasket_core.c | 11 +++--------
 1 file changed, 3 insertions(+), 8 deletions(-)

diff --git a/drivers/staging/gasket/gasket_core.c b/drivers/staging/gasket/gasket_core.c
index 45914ebc8f44..248c99a841a9 100644
--- a/drivers/staging/gasket/gasket_core.c
+++ b/drivers/staging/gasket/gasket_core.c
@@ -2090,19 +2090,14 @@ int gasket_wait_sync(
 	u64 timeout_ns)
 {
 	u64 reg;
-	struct timespec start_time, cur_time;
-	u64 diff_nanosec;
+	u64 start_time, diff_nanosec;
 	int count = 0;
 
 	reg = gasket_dev_read_64(gasket_dev, bar, offset);
-	start_time = current_kernel_time();
+	start_time = ktime_get_ns();
 	while ((reg & mask) != val) {
 		count++;
-		cur_time = current_kernel_time();
-		diff_nanosec = (u64)(cur_time.tv_sec - start_time.tv_sec) *
-				       1000000000LL +
-			       (u64)(cur_time.tv_nsec) -
-			       (u64)(start_time.tv_nsec);
+		diff_nanosec = ktime_get_ns() - start_time;
 		if (diff_nanosec > timeout_ns) {
 			gasket_log_error(
 				gasket_dev,
-- 
2.9.0

Re: [PATCH] staging: gasket remove current_kernel_time usage

[Greg Kroah-Hartman]

On Fri, Jul 13, 2018 at 05:06:37PM +0200, Arnd Bergmann wrote:
> A new user of the deprecated current_kernel_time() function has appeared
> here. This code won't work correct during leap seconds or a concurrent
> settimeofday() call, and it probably doesn't do what the author intended
> even for the normal case, as it passes a timeout in nanoseconds but
> reads the time using a jiffies-granularity accessor.
> 
> I'm changing it to ktime_get_ns() here, which simplifies the logic,
> and uses a high-res clocksource. This is a bit slower, but that
> probably doesn't matter in a busy-wait loop.
> 
> Note: it also doesn't matter in the current version, as there are no
> callers of this function.

Let's just rip the whole function out, I've been going through and
removing functions that no one calls and no one uses.  That would make
more sense here.  Want me to send a patch for that, or do you want to?

thanks,

greg k-h

Re: [PATCH] staging: gasket remove current_kernel_time usage

[Greg Kroah-Hartman]

On Fri, Jul 13, 2018 at 05:33:51PM +0200, Greg Kroah-Hartman wrote:
> On Fri, Jul 13, 2018 at 05:06:37PM +0200, Arnd Bergmann wrote:
> > A new user of the deprecated current_kernel_time() function has appeared
> > here. This code won't work correct during leap seconds or a concurrent
> > settimeofday() call, and it probably doesn't do what the author intended
> > even for the normal case, as it passes a timeout in nanoseconds but
> > reads the time using a jiffies-granularity accessor.
> > 
> > I'm changing it to ktime_get_ns() here, which simplifies the logic,
> > and uses a high-res clocksource. This is a bit slower, but that
> > probably doesn't matter in a busy-wait loop.
> > 
> > Note: it also doesn't matter in the current version, as there are no
> > callers of this function.
> 
> Let's just rip the whole function out, I've been going through and
> removing functions that no one calls and no one uses.  That would make
> more sense here.  Want me to send a patch for that, or do you want to?

Ah it was easy enough, here's the patch, I'll add it to the longer list
of gasket patches I have queued up to be applied tomorrow:

-------------

From: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
Subject: [PATCH] staging: gasket: remove gasket_wait_sync()

This function is not called anywhere, so just remove it.

Also, as an added benifit, Arnd points out that it doesn't even work
properly:
	This code won't work correct during leap seconds or a concurrent
	settimeofday() call, and it probably doesn't do what the author intended
	even for the normal case, as it passes a timeout in nanoseconds but
	reads the time using a jiffies-granularity accessor.

Reported-by: Arnd Bergmann <arnd@arndb.de>
Signed-off-by: Greg Kroah-Hartman <gregkh@linuxfoundation.org>


diff --git a/drivers/staging/gasket/gasket_core.c b/drivers/staging/gasket/gasket_core.c
index 4ca6e53116ea..df52e6e6bf13 100644
--- a/drivers/staging/gasket/gasket_core.c
+++ b/drivers/staging/gasket/gasket_core.c
@@ -2067,50 +2067,6 @@ struct device *gasket_get_device(struct gasket_dev *dev)
 	return NULL;
 }
 
-/**
- * Synchronously waits on device.
- * @gasket_dev: Device struct.
- * @bar: Bar
- * @offset: Register offset
- * @mask: Register mask
- * @val: Expected value
- * @timeout_ns: Timeout in nanoseconds
- *
- * Description: Busy waits for a specific combination of bits to be set
- * on a Gasket register.
- **/
-int gasket_wait_sync(
-	struct gasket_dev *gasket_dev, int bar, u64 offset, u64 mask, u64 val,
-	u64 timeout_ns)
-{
-	u64 reg;
-	struct timespec start_time, cur_time;
-	u64 diff_nanosec;
-	int count = 0;
-
-	reg = gasket_dev_read_64(gasket_dev, bar, offset);
-	start_time = current_kernel_time();
-	while ((reg & mask) != val) {
-		count++;
-		cur_time = current_kernel_time();
-		diff_nanosec = (u64)(cur_time.tv_sec - start_time.tv_sec) *
-				       1000000000LL +
-			       (u64)(cur_time.tv_nsec) -
-			       (u64)(start_time.tv_nsec);
-		if (diff_nanosec > timeout_ns) {
-			gasket_log_error(
-				gasket_dev,
-				"gasket_wait_sync timeout: reg %llx count %x "
-				"dma %lld ns\n",
-				offset, count, diff_nanosec);
-			return -1;
-		}
-		reg = gasket_dev_read_64(gasket_dev, bar, offset);
-	}
-	return 0;
-}
-EXPORT_SYMBOL(gasket_wait_sync);
-
 /**
  * Asynchronously waits on device.
  * @gasket_dev: Device struct.
diff --git a/drivers/staging/gasket/gasket_core.h b/drivers/staging/gasket/gasket_core.h
index 45013446b8c5..e51acadc0fc4 100644
--- a/drivers/staging/gasket/gasket_core.h
+++ b/drivers/staging/gasket/gasket_core.h
@@ -699,11 +699,6 @@ const struct gasket_driver_desc *gasket_get_driver_desc(struct gasket_dev *dev);
 /* Get the device structure for a given device. */
 struct device *gasket_get_device(struct gasket_dev *dev);
 
-/* Helper function, Synchronous waits on a given set of bits. */
-int gasket_wait_sync(
-	struct gasket_dev *gasket_dev, int bar, u64 offset, u64 mask, u64 val,
-	u64 timeout_ns);
-
 /* Helper function, Asynchronous waits on a given set of bits. */
 int gasket_wait_with_reschedule(
 	struct gasket_dev *gasket_dev, int bar, u64 offset, u64 mask, u64 val,