[PATCH v2] Input: joystick - Use ktime for measuring timing

[PATCH v2] Input: joystick - Use ktime for measuring timing

[Takashi Iwai]

The current codes in gameport and analog joystick drivers for the time
accounting have a long-standing problem when the system is running
with CPU freq; since the timing is measured via TSC or sample counter,
the calculation isn't reliable.

In this patch, as a simple fix, use the standard ktime to measure the
timing.  In case where no high resolution timer is available,
use_ktime bool option is provided to both modules.  Setting
use_ktime=false switches to the old methods.

Tested-by: Clemens Ladisch <clemens@ladisch.de>
Signed-off-by: Takashi Iwai <tiwai@suse.de>
---
v1->v2: Add paramter descritions, use t3 in gameport speed measurement

 drivers/input/gameport/gameport.c | 41 +++++++++++++++++++++-
 drivers/input/joystick/analog.c   | 71 ++++++++++++++++++++++++++++-----------
 2 files changed, 92 insertions(+), 20 deletions(-)

diff --git a/drivers/input/gameport/gameport.c b/drivers/input/gameport/gameport.c
index 24c41ba7d4e0..e29c04e2aff4 100644
--- a/drivers/input/gameport/gameport.c
+++ b/drivers/input/gameport/gameport.c
@@ -23,6 +23,7 @@
 #include <linux/workqueue.h>
 #include <linux/sched.h>	/* HZ */
 #include <linux/mutex.h>
+#include <linux/timekeeping.h>
 
 /*#include <asm/io.h>*/
 
@@ -30,6 +31,10 @@ MODULE_AUTHOR("Vojtech Pavlik <vojtech@ucw.cz>");
 MODULE_DESCRIPTION("Generic gameport layer");
 MODULE_LICENSE("GPL");
 
+static bool use_ktime = true;
+module_param(use_ktime, bool, 0400);
+MODULE_PARM_DESC(use_ktime, "Use ktime for measuring I/O speed");
+
 /*
  * gameport_mutex protects entire gameport subsystem and is taken
  * every time gameport port or driver registrered or unregistered.
@@ -76,6 +81,38 @@ static unsigned int get_time_pit(void)
 
 static int gameport_measure_speed(struct gameport *gameport)
 {
+	unsigned int i, t, tx;
+	u64 t1, t2, t3;
+	unsigned long flags;
+
+	if (gameport_open(gameport, NULL, GAMEPORT_MODE_RAW))
+		return 0;
+
+	tx = ~0;
+
+	for (i = 0; i < 50; i++) {
+		local_irq_save(flags);
+		t1 = ktime_get_ns();
+		for (t = 0; t < 50; t++)
+			gameport_read(gameport);
+		t2 = ktime_get_ns();
+		t3 = ktime_get_ns();
+		local_irq_restore(flags);
+		udelay(i * 10);
+		t = (t2 - t1) - (t3 - t2);
+		if (t < tx)
+			tx = t;
+	}
+
+	gameport_close(gameport);
+	t = 1000000 * 50;
+	if (tx)
+		t /= tx;
+	return t;
+}
+
+static int old_gameport_measure_speed(struct gameport *gameport)
+{
 #if defined(__i386__)
 
 	unsigned int i, t, t1, t2, t3, tx;
@@ -521,7 +558,9 @@ static void gameport_add_port(struct gameport *gameport)
 	if (gameport->parent)
 		gameport->parent->child = gameport;
 
-	gameport->speed = gameport_measure_speed(gameport);
+	gameport->speed = use_ktime ?
+		gameport_measure_speed(gameport) :
+		old_gameport_measure_speed(gameport);
 
 	list_add_tail(&gameport->node, &gameport_list);
 
diff --git a/drivers/input/joystick/analog.c b/drivers/input/joystick/analog.c
index ab0fdcd36e18..4284080e481d 100644
--- a/drivers/input/joystick/analog.c
+++ b/drivers/input/joystick/analog.c
@@ -36,6 +36,7 @@
 #include <linux/gameport.h>
 #include <linux/jiffies.h>
 #include <linux/timex.h>
+#include <linux/timekeeping.h>
 
 #define DRIVER_DESC	"Analog joystick and gamepad driver"
 
@@ -43,6 +44,10 @@ MODULE_AUTHOR("Vojtech Pavlik <vojtech@ucw.cz>");
 MODULE_DESCRIPTION(DRIVER_DESC);
 MODULE_LICENSE("GPL");
 
+static bool use_ktime = true;
+module_param(use_ktime, bool, 0400);
+MODULE_PARM_DESC(use_ktime, "Use ktime for measuring I/O speed");
+
 /*
  * Option parsing.
  */
@@ -171,6 +176,25 @@ static unsigned long analog_faketime = 0;
 #warning Precise timer not defined for this architecture.
 #endif
 
+static inline u64 get_time(void)
+{
+	if (use_ktime) {
+		return ktime_get_ns();
+	} else {
+		unsigned int x;
+		GET_TIME(x);
+		return x;
+	}
+}
+
+static inline unsigned int delta(u64 x, u64 y)
+{
+	if (use_ktime)
+		return y - x;
+	else
+		return DELTA((unsigned int)x, (unsigned int)y);
+}
+
 /*
  * analog_decode() decodes analog joystick data and reports input events.
  */
@@ -226,7 +250,8 @@ static void analog_decode(struct analog *analog, int *axes, int *initial, int bu
 static int analog_cooked_read(struct analog_port *port)
 {
 	struct gameport *gameport = port->gameport;
-	unsigned int time[4], start, loop, now, loopout, timeout;
+	u64 time[4], start, loop, now;
+	unsigned int loopout, timeout;
 	unsigned char data[4], this, last;
 	unsigned long flags;
 	int i, j;
@@ -236,7 +261,7 @@ static int analog_cooked_read(struct analog_port *port)
 
 	local_irq_save(flags);
 	gameport_trigger(gameport);
-	GET_TIME(now);
+	now = get_time();
 	local_irq_restore(flags);
 
 	start = now;
@@ -249,16 +274,16 @@ static int analog_cooked_read(struct analog_port *port)
 
 		local_irq_disable();
 		this = gameport_read(gameport) & port->mask;
-		GET_TIME(now);
+		now = get_time();
 		local_irq_restore(flags);
 
-		if ((last ^ this) && (DELTA(loop, now) < loopout)) {
+		if ((last ^ this) && (delta(loop, now) < loopout)) {
 			data[i] = last ^ this;
 			time[i] = now;
 			i++;
 		}
 
-	} while (this && (i < 4) && (DELTA(start, now) < timeout));
+	} while (this && (i < 4) && (delta(start, now) < timeout));
 
 	this <<= 4;
 
@@ -266,7 +291,7 @@ static int analog_cooked_read(struct analog_port *port)
 		this |= data[i];
 		for (j = 0; j < 4; j++)
 			if (data[i] & (1 << j))
-				port->axes[j] = (DELTA(start, time[i]) << ANALOG_FUZZ_BITS) / port->loop;
+				port->axes[j] = (delta(start, time[i]) << ANALOG_FUZZ_BITS) / port->loop;
 	}
 
 	return -(this != port->mask);
@@ -365,31 +390,39 @@ static void analog_close(struct input_dev *dev)
 static void analog_calibrate_timer(struct analog_port *port)
 {
 	struct gameport *gameport = port->gameport;
-	unsigned int i, t, tx, t1, t2, t3;
+	unsigned int i, t, tx;
+	u64 t1, t2, t3;
 	unsigned long flags;
 
-	local_irq_save(flags);
-	GET_TIME(t1);
+	if (use_ktime) {
+		port->speed = 1000000;
+	} else {
+		local_irq_save(flags);
+		t1 = get_time();
 #ifdef FAKE_TIME
-	analog_faketime += 830;
+		analog_faketime += 830;
 #endif
-	mdelay(1);
-	GET_TIME(t2);
-	GET_TIME(t3);
-	local_irq_restore(flags);
+		mdelay(1);
+		t2 = get_time();
+		t3 = get_time();
+		local_irq_restore(flags);
 
-	port->speed = DELTA(t1, t2) - DELTA(t2, t3);
+		port->speed = delta(t1, t2) - delta(t2, t3);
+	}
 
 	tx = ~0;
 
 	for (i = 0; i < 50; i++) {
 		local_irq_save(flags);
-		GET_TIME(t1);
-		for (t = 0; t < 50; t++) { gameport_read(gameport); GET_TIME(t2); }
-		GET_TIME(t3);
+		t1 = get_time();
+		for (t = 0; t < 50; t++) {
+			gameport_read(gameport);
+			t2 = get_time();
+		}
+		t3 = get_time();
 		local_irq_restore(flags);
 		udelay(i);
-		t = DELTA(t1, t2) - DELTA(t2, t3);
+		t = delta(t1, t2) - delta(t2, t3);
 		if (t < tx) tx = t;
 	}
 
-- 
2.1.0

Re: [PATCH v2] Input: joystick - Use ktime for measuring timing

[Andreas Mohr]

Hi,

On Wed, Sep 10, 2014 at 05:57:17PM +0200, Takashi Iwai wrote:
> The current codes in gameport and analog joystick drivers for the time
> accounting have a long-standing problem when the system is running
> with CPU freq; since the timing is measured via TSC or sample counter,
> the calculation isn't reliable.

Thank you very much for having followed through on this!
(somehow you never seem to disappoint me :)

While working on testing this and doing various gameport/soundcard modifications,
I'm afraid I have seen the following checkpatch.pl (of v3.16) warnings:

WARNING: Missing a blank line after declarations
#164: FILE: drivers/input/joystick/analog.c:192:
+               unsigned int x;
+               GET_TIME(x);

WARNING: line over 80 characters
#224: FILE: drivers/input/joystick/analog.c:301:
+                               port->axes[j] = (delta(start, time[i]) << ANALOG_FUZZ_BITS) / port->loop;


BTW the commit as-is will not be compatible with v3.16
since there's no ktime_get_ns() there yet
and (to add insult to injury) even the #include header files have changed, too.

That's a bit of a pity since I just had intended to say
that it's a very good idea
to release a quick(&dirty) initial timing hotfix for gameport handling
*prior* to possibly doing any subsequent less-related gameport cleanup commits,
since the quick initial timing hotfix would be very prominent -stable material,
except it... ain't so (--> life sucks :-).

So, for -stable reasons it might be very worthwhile
to add some compat code to the analog.c patch -
I am currently using the following compat fix (on v3.16):

--- a/drivers/input/joystick/analog.c
+++ b/drivers/input/joystick/analog.c
@@ -36,7 +36,14 @@
 #include <linux/gameport.h>
 #include <linux/jiffies.h>
 #include <linux/timex.h>
-#include <linux/timekeeping.h>
+//#include <linux/timekeeping.h>
+#include <linux/hrtimer.h>
+
+static inline u64 ktime_get_ns(void)
+{
+       return ktime_to_ns(ktime_get());
+}
+


(which obviously isn't fit for purpose yet
since you'd need a versioned #include conditional,
and the name of this function when unconditionally added
is in direct conflict with the same-name > v3.16 one)


So, should that commit be improved
to have a simple versioning check
in order to be fully -stable deployable,
or would -stable be handled differently anyway?
(I'll now have these updates and checkpatch.pl stuff
maintained in a FIXUP commit locally)

So much for a quick status update
(I'll be rebooting into 3.16 now - my last kernel build was as low as 3.11.x even, WOW).

Thanks,

Andreas Mohr

Re: [PATCH v2] Input: joystick - Use ktime for measuring timing

[Takashi Iwai]

At Wed, 17 Sep 2014 21:23:37 +0200,
Andreas Mohr wrote:
> 
> Hi,
> 
> On Wed, Sep 10, 2014 at 05:57:17PM +0200, Takashi Iwai wrote:
> > The current codes in gameport and analog joystick drivers for the time
> > accounting have a long-standing problem when the system is running
> > with CPU freq; since the timing is measured via TSC or sample counter,
> > the calculation isn't reliable.
> 
> Thank you very much for having followed through on this!
> (somehow you never seem to disappoint me :)
> 
> While working on testing this and doing various gameport/soundcard modifications,
> I'm afraid I have seen the following checkpatch.pl (of v3.16) warnings:
> 
> WARNING: Missing a blank line after declarations
> #164: FILE: drivers/input/joystick/analog.c:192:
> +               unsigned int x;
> +               GET_TIME(x);
> 
> WARNING: line over 80 characters
> #224: FILE: drivers/input/joystick/analog.c:301:
> +                               port->axes[j] = (delta(start, time[i]) << ANALOG_FUZZ_BITS) / port->loop;

Both are cosmetic and I won't be bothered if it were sent to my tree,
for example ;)  It depends on the maintainer, so if Dmitry wants these
to be fixed, I'm willing to resubmit, of course.

> BTW the commit as-is will not be compatible with v3.16
> since there's no ktime_get_ns() there yet
> and (to add insult to injury) even the #include header files have changed, too.
> 
> That's a bit of a pity since I just had intended to say
> that it's a very good idea
> to release a quick(&dirty) initial timing hotfix for gameport handling
> *prior* to possibly doing any subsequent less-related gameport cleanup commits,
> since the quick initial timing hotfix would be very prominent -stable material,
> except it... ain't so (--> life sucks :-).

Hey, why not using 3.17 kernel?
If the fix is *seriously* demanded, one can backport easily with the
equivalent function and the header change, and send to stable kernel,
too.  The important point is to track the upstream commit ID (once if
it's really merged).


Takashi

> So, for -stable reasons it might be very worthwhile
> to add some compat code to the analog.c patch -
> I am currently using the following compat fix (on v3.16):
> 
> --- a/drivers/input/joystick/analog.c
> +++ b/drivers/input/joystick/analog.c
> @@ -36,7 +36,14 @@
>  #include <linux/gameport.h>
>  #include <linux/jiffies.h>
>  #include <linux/timex.h>
> -#include <linux/timekeeping.h>
> +//#include <linux/timekeeping.h>
> +#include <linux/hrtimer.h>
> +
> +static inline u64 ktime_get_ns(void)
> +{
> +       return ktime_to_ns(ktime_get());
> +}
> +
> 
> 
> (which obviously isn't fit for purpose yet
> since you'd need a versioned #include conditional,
> and the name of this function when unconditionally added
> is in direct conflict with the same-name > v3.16 one)
> 
> 
> So, should that commit be improved
> to have a simple versioning check
> in order to be fully -stable deployable,
> or would -stable be handled differently anyway?
> (I'll now have these updates and checkpatch.pl stuff
> maintained in a FIXUP commit locally)
> 
> So much for a quick status update
> (I'll be rebooting into 3.16 now - my last kernel build was as low as 3.11.x even, WOW).
> 
> Thanks,
> 
> Andreas Mohr
> 

Re: [PATCH v2] Input: joystick - Use ktime for measuring timing

[Dmitry Torokhov]

On Thu, Sep 18, 2014 at 09:15:44AM +0200, Takashi Iwai wrote:
> At Wed, 17 Sep 2014 21:23:37 +0200,
> Andreas Mohr wrote:
> > 
> > Hi,
> > 
> > On Wed, Sep 10, 2014 at 05:57:17PM +0200, Takashi Iwai wrote:
> > > The current codes in gameport and analog joystick drivers for the time
> > > accounting have a long-standing problem when the system is running
> > > with CPU freq; since the timing is measured via TSC or sample counter,
> > > the calculation isn't reliable.
> > 
> > Thank you very much for having followed through on this!
> > (somehow you never seem to disappoint me :)
> > 
> > While working on testing this and doing various gameport/soundcard modifications,
> > I'm afraid I have seen the following checkpatch.pl (of v3.16) warnings:
> > 
> > WARNING: Missing a blank line after declarations
> > #164: FILE: drivers/input/joystick/analog.c:192:
> > +               unsigned int x;
> > +               GET_TIME(x);
> > 
> > WARNING: line over 80 characters
> > #224: FILE: drivers/input/joystick/analog.c:301:
> > +                               port->axes[j] = (delta(start, time[i]) << ANALOG_FUZZ_BITS) / port->loop;
> 
> Both are cosmetic and I won't be bothered if it were sent to my tree,
> for example ;)  It depends on the maintainer, so if Dmitry wants these
> to be fixed, I'm willing to resubmit, of course.

No, I do not. If there is a large rework we might want to reformat it,
otherwise let's leave it as is.

Thanks.

-- 
Dmitry