Evdev: Fix bug in checking duplicate clock change request

PATCH In review Aniroop Mathur 3 participants 4 messages Oct 28, 2015 – Oct 30, 2015

Subsystems (2)

HID Input drivers

lore

Share & tools

lore

Aniroop Mathur

author

2015-10-28 20:08 UTC #0 lore raw

PATCH 1/1 Evdev: Fix bug in checking duplicate clock change request

From: Aniroop Mathur <[email protected]>

clk_type and clkid stores different predefined clock identification values so they cannot be compared for checking duplicate clock change request. Therefore, lets fix it to avoid unexpected results.

Signed-off-by Aniroop Mathur <[email protected]>

---
 drivers/input/evdev.c | 10 ++++++----
 1 file changed, 6 insertions(+), 4 deletions(-)

drivers/input/evdev.c +6 -4

index 08d4964..0d40f6f 100644
@@ -56,7 +56,7 @@ struct evdev_client {
 	struct fasync_struct *fasync;
 	struct evdev *evdev;
 	struct list_head node;
-	int clk_type;
+	unsigned int clk_type;
 	bool revoked;
 	unsigned int bufsize;
 	struct input_event buffer[];

@@ -146,9 +146,7 @@ static void evdev_queue_syn_dropped(struct evdev_client *client)
 static int evdev_set_clk_type(struct evdev_client *client, unsigned int clkid)
 {
 	unsigned long flags;
-
-	if (client->clk_type == clkid)
-		return 0;
+	unsigned int prev_clk_type = client->clk_type;
 
 	switch (clkid) {

@@ -165,6 +163,10 @@ static int evdev_set_clk_type(struct evdev_client *client, unsigned int clkid)
 		return -EINVAL;
 	}
 
+	/* No need to flush if clk_type is same as before */
+	if (client->clk_type == prev_clk_type)
+		return 0;
+
 	/*
 	 * Flush pending events and queue SYN_DROPPED event,
 	 * but only if the queue is not empty.

Signature · 1 line

2.6.2

Dmitry Torokhov

maintainer

2015-10-30 11:15 UTC #1 lore raw

Hi Aniroop,

Quoted 5 lines · Aniroop Mathur

From: Aniroop Mathur <[email protected]>
clk_type and clkid stores different predefined clock identification values so they cannot be compared for checking duplicate clock change request. Therefore, lets fix it to avoid unexpected results.

Good catch!

Quoted 3 lines

Signed-off-by Aniroop Mathur <[email protected]>
Signed-off-by Aniroop Mathur <[email protected]>

You only need one signoff, which one should I keep? The Samsung one?

Quoted 18 lines

---
 drivers/input/evdev.c | 10 ++++++----
 1 file changed, 6 insertions(+), 4 deletions(-)

drivers/input/evdev.c +1 -1

index 08d4964..0d40f6f 100644
@@ -56,7 +56,7 @@ struct evdev_client {
 	struct fasync_struct *fasync;
 	struct evdev *evdev;
 	struct list_head node;
-	int clk_type;
+	unsigned int clk_type;
 	bool revoked;
 	unsigned int bufsize;
 	struct input_event buffer[];

drivers/input/evdev.c static void evdev_queue_syn_dropped(struct evdev_client *client) · line 149

Quoted 7 lines

drivers/input/evdev.c +1 -3

@@ -146,9 +146,7 @@ static void evdev_queue_syn_dropped(struct evdev_client *client)
 static int evdev_set_clk_type(struct evdev_client *client, unsigned int clkid)
 {
 	unsigned long flags;
-
-	if (client->clk_type == clkid)
-		return 0;
+	unsigned int prev_clk_type = client->clk_type;

I prefer the other way around: convert to a temp and then compare. I;ll fix it up on my side.

Quoted 17 lines

drivers/input/evdev.c +4 -0

 	switch (clkid) {

@@ -165,6 +163,10 @@ static int evdev_set_clk_type(struct evdev_client *client, unsigned int clkid)
 		return -EINVAL;
 	}
 
+	/* No need to flush if clk_type is same as before */
+	if (client->clk_type == prev_clk_type)
+		return 0;
+
 	/*
 	 * Flush pending events and queue SYN_DROPPED event,
 	 * but only if the queue is not empty.

Signature · 1 line

2.6.2

Thanks.

Signature · 1 line

Dmitry

Aniroop Mathur

2015-10-30 13:04 UTC #2 lore raw

Hello Mr. Torokhov,

Quoted 11 lines · Dmitry Torokhov

Hi Aniroop,
Quoted 5 lines · Aniroop Mathur
From: Aniroop Mathur <[email protected]>
clk_type and clkid stores different predefined clock identification values so they cannot be compared for checking duplicate clock change request. Therefore, lets fix it to avoid unexpected results.
Good catch!

Thank you :)

Quoted 6 lines

Quoted 3 lines
Signed-off-by Aniroop Mathur <[email protected]>
Signed-off-by Aniroop Mathur <[email protected]>
You only need one signoff, which one should I keep? The Samsung one?

Yes, Samsung one.

Quoted 20 lines

---
 drivers/input/evdev.c | 10 ++++++----
 1 file changed, 6 insertions(+), 4 deletions(-)

drivers/input/evdev.c +1 -1

index 08d4964..0d40f6f 100644
@@ -56,7 +56,7 @@ struct evdev_client {
      struct fasync_struct *fasync;
      struct evdev *evdev;
      struct list_head node;
-     int clk_type;
+     unsigned int clk_type;
      bool revoked;
      unsigned int bufsize;
      struct input_event buffer[];

@@ -146,9 +146,7 @@ static void evdev_queue_syn_dropped(struct evdev_client *client)
 static int evdev_set_clk_type(struct evdev_client *client, unsigned int clkid)
 {

Quoted 9 lines

drivers/input/evdev.c static void evdev_queue_syn_dropped(struct evdev_client *client) · line 149
Quoted 5 lines
drivers/input/evdev.c +1 -3
      unsigned long flags;
-
-     if (client->clk_type == clkid)
-             return 0;
+     unsigned int prev_clk_type = client->clk_type;
I prefer the other way around: convert to a temp and then compare. I;ll fix it up on my side.

I thought that was the only better way of all.
I am not sure what exactly you're referring. I guess I need little more elaboration here or anyways, I'll check it up up once you apply it.

Quoted 20 lines

Quoted 15 lines

drivers/input/evdev.c +4 -1

      switch (clkid) {

@@ -165,6 +163,10 @@ static int evdev_set_clk_type(struct evdev_client *client, unsigned int clkid)
              return -EINVAL;
      }
 
+     /* No need to flush if clk_type is same as before */
+     if (client->clk_type == prev_clk_type)
+             return 0;
+
      /*
       * Flush pending events and queue SYN_DROPPED event,
       * but only if the queue is not empty.
--

Quoted 2 lines

2.6.2

Thanks.

Quoted 2 lines

--
Dmitry

Regards,
Aniroop

Aniroop Mathur

2015-10-30 17:47 UTC #3 lore raw

Quoted 58 lines · Aniroop Mathur

Hello Mr. Torokhov,
Quoted 11 lines · Dmitry Torokhov
Hi Aniroop,
Quoted 5 lines · Aniroop Mathur
From: Aniroop Mathur <[email protected]>
clk_type and clkid stores different predefined clock identification values so they cannot be compared for checking duplicate clock change request. Therefore, lets fix it to avoid unexpected results.
Good catch!
Thank you :)
Quoted 6 lines
Quoted 3 lines
Signed-off-by Aniroop Mathur <[email protected]>
Signed-off-by Aniroop Mathur <[email protected]>
You only need one signoff, which one should I keep? The Samsung one?
Yes, Samsung one.
Quoted 20 lines
Quoted 20 lines
---
 drivers/input/evdev.c | 10 ++++++----
 1 file changed, 6 insertions(+), 4 deletions(-)
drivers/input/evdev.c +1 -1
index 08d4964..0d40f6f 100644
@@ -56,7 +56,7 @@ struct evdev_client {
      struct fasync_struct *fasync;
      struct evdev *evdev;
      struct list_head node;
-     int clk_type;
+     unsigned int clk_type;
      bool revoked;
      unsigned int bufsize;
      struct input_event buffer[];
@@ -146,9 +146,7 @@ static void evdev_queue_syn_dropped(struct evdev_client *client)
 static int evdev_set_clk_type(struct evdev_client *client, unsigned int clkid)
 {
Quoted 9 lines
drivers/input/evdev.c static void evdev_queue_syn_dropped(struct evdev_client *client) · line 149
Quoted 5 lines
drivers/input/evdev.c +1 -3
      unsigned long flags;
-
-     if (client->clk_type == clkid)
-             return 0;
+     unsigned int prev_clk_type = client->clk_type;
I prefer the other way around: convert to a temp and then compare. I;ll fix it up on my side.

Quoted 4 lines

I thought that was the only better way of all.
I am not sure what exactly you're referring. I guess I need little more elaboration here or anyways, I'll check it up up once you apply it.

Ahhh okay... I got the other way around.
(Filling client->clk_type in the end instead)

Quoted 28 lines

Quoted 23 lines

Quoted 18 lines

drivers/input/evdev.c +4 -1

      switch (clkid) {

@@ -165,6 +163,10 @@ static int evdev_set_clk_type(struct evdev_client *client, unsigned int clkid)
              return -EINVAL;
      }
 
+     /* No need to flush if clk_type is same as before */
+     if (client->clk_type == prev_clk_type)
+             return 0;
+
      /*
       * Flush pending events and queue SYN_DROPPED event,
       * but only if the queue is not empty.
--

Quoted 2 lines

2.6.2

Thanks.

Quoted 2 lines

--
Dmitry

Regards,
Aniroop