[PATCH] Input:Flush client events on clk_type change

[PATCH] Input:Flush client events on clk_type change

[Anshul Garg]

From: Anshul Garg <aksgarg1989@gmail.com>

Since the client clk_type is changed , flush pending
events from client buffer and queue SYN_DROPPED event.
Added check for duplicate clk_type change request.

Signed-off-by: Anshul Garg <anshul.g@samsung.com>
---
 drivers/input/evdev.c |   18 ++++++++++++++++--
 1 file changed, 16 insertions(+), 2 deletions(-)

diff --git a/drivers/input/evdev.c b/drivers/input/evdev.c
index b1a52ab..d024fc6 100644
--- a/drivers/input/evdev.c
+++ b/drivers/input/evdev.c
@@ -62,8 +62,12 @@ struct evdev_client {
 	struct input_event buffer[];
 };
 
-static int evdev_set_clk_type(struct evdev_client *client, unsigned int clkid)
+static int evdev_set_clk_type(struct evdev_client *client,
+			struct input_dev *dev, unsigned int clkid)
 {
+	if (client->clk_type == clkid)
+		return 0;
+
 	switch (clkid) {
 
 	case CLOCK_REALTIME:
@@ -78,7 +82,17 @@ static int evdev_set_clk_type(struct evdev_client *client, unsigned int clkid)
 	default:
 		return -EINVAL;
 	}
+
+	spin_lock_irq(&dev->event_lock);
+	spin_lock(&client->buffer_lock);
+	spin_unlock(&dev->event_lock);
 
+	/* Flush clients events after clk_type is changed
+	 * and queue SYN_DROPPED event.*/
+	client->packet_head = client->head = client->tail;
+	spin_unlock_irq(&client->buffer_lock);
+
+	evdev_queue_syn_dropped(client);
 	return 0;
 }
 
@@ -908,7 +922,7 @@ static long evdev_do_ioctl(struct file *file, unsigned int cmd,
 		if (copy_from_user(&i, p, sizeof(unsigned int)))
 			return -EFAULT;
 
-		return evdev_set_clk_type(client, i);
+		return evdev_set_clk_type(client, dev, i);
 
 	case EVIOCGKEYCODE:
 		return evdev_handle_get_keycode(dev, p);
-- 
1.7.9.5


---
This email has been checked for viruses by Avast antivirus software.
http://www.avast.com

[PATCH] Input:Flush client events on clk_type change

[Anshul Garg]

From: Anshul Garg <aksgarg1989@gmail.com>

Since the client clk_type is changed , flush pending
events from client buffer and queue SYN_DROPPED event.
Added check for duplicate clk_type change request.

Signed-off-by: Anshul Garg <anshul.g@samsung.com>
---
 drivers/input/evdev.c |   58 ++++++++++++++++++++++++++++++-------------------
 1 file changed, 36 insertions(+), 22 deletions(-)

diff --git a/drivers/input/evdev.c b/drivers/input/evdev.c
index b1a52ab..223367e 100644
--- a/drivers/input/evdev.c
+++ b/drivers/input/evdev.c
@@ -62,27 +62,6 @@ struct evdev_client {
 	struct input_event buffer[];
 };
 
-static int evdev_set_clk_type(struct evdev_client *client, unsigned int clkid)
-{
-	switch (clkid) {
-
-	case CLOCK_REALTIME:
-		client->clk_type = EV_CLK_REAL;
-		break;
-	case CLOCK_MONOTONIC:
-		client->clk_type = EV_CLK_MONO;
-		break;
-	case CLOCK_BOOTTIME:
-		client->clk_type = EV_CLK_BOOT;
-		break;
-	default:
-		return -EINVAL;
-	}
-
-	return 0;
-}
-
-/* flush queued events of type @type, caller must hold client->buffer_lock */
 static void __evdev_flush_queue(struct evdev_client *client, unsigned int type)
 {
 	unsigned int i, head, num;
@@ -160,6 +139,41 @@ static void evdev_queue_syn_dropped(struct evdev_client *client)
 	spin_unlock_irqrestore(&client->buffer_lock, flags);
 }
 
+static int evdev_set_clk_type(struct evdev_client *client,
+			struct input_dev *dev, unsigned int clkid)
+{
+	if (client->clk_type == clkid)
+		return 0;
+
+	switch (clkid) {
+
+	case CLOCK_REALTIME:
+		client->clk_type = EV_CLK_REAL;
+		break;
+	case CLOCK_MONOTONIC:
+		client->clk_type = EV_CLK_MONO;
+		break;
+	case CLOCK_BOOTTIME:
+		client->clk_type = EV_CLK_BOOT;
+		break;
+	default:
+		return -EINVAL;
+	}
+
+	spin_lock_irq(&dev->event_lock);
+	spin_lock(&client->buffer_lock);
+	spin_unlock(&dev->event_lock);
+
+	/* Flush clients events after clk_type is changed
+	 * and queue SYN_DROPPED event.*/
+	client->packet_head = client->head = client->tail;
+	spin_unlock_irq(&client->buffer_lock);
+
+	evdev_queue_syn_dropped(client);
+	return 0;
+}
+
+/* flush queued events of type @type, caller must hold client->buffer_lock */
 static void __pass_event(struct evdev_client *client,
 			 const struct input_event *event)
 {
@@ -908,7 +922,7 @@ static long evdev_do_ioctl(struct file *file, unsigned int cmd,
 		if (copy_from_user(&i, p, sizeof(unsigned int)))
 			return -EFAULT;
 
-		return evdev_set_clk_type(client, i);
+		return evdev_set_clk_type(client, dev, i);
 
 	case EVIOCGKEYCODE:
 		return evdev_handle_get_keycode(dev, p);
-- 
1.7.9.5


---
This email has been checked for viruses by Avast antivirus software.
http://www.avast.com

Re: [PATCH] Input:Flush client events on clk_type change

[Dmitry Torokhov]

On Wed, Jan 07, 2015 at 10:56:03AM -0800, Anshul Garg wrote:
> From: Anshul Garg <aksgarg1989@gmail.com>
> 
> Since the client clk_type is changed , flush pending
> events from client buffer and queue SYN_DROPPED event.
> Added check for duplicate clk_type change request.
> 
> Signed-off-by: Anshul Garg <anshul.g@samsung.com>
> ---
>  drivers/input/evdev.c |   58 ++++++++++++++++++++++++++++++-------------------
>  1 file changed, 36 insertions(+), 22 deletions(-)
> 
> diff --git a/drivers/input/evdev.c b/drivers/input/evdev.c
> index b1a52ab..223367e 100644
> --- a/drivers/input/evdev.c
> +++ b/drivers/input/evdev.c
> @@ -62,27 +62,6 @@ struct evdev_client {
>  	struct input_event buffer[];
>  };
>  
> -static int evdev_set_clk_type(struct evdev_client *client, unsigned int clkid)
> -{
> -	switch (clkid) {
> -
> -	case CLOCK_REALTIME:
> -		client->clk_type = EV_CLK_REAL;
> -		break;
> -	case CLOCK_MONOTONIC:
> -		client->clk_type = EV_CLK_MONO;
> -		break;
> -	case CLOCK_BOOTTIME:
> -		client->clk_type = EV_CLK_BOOT;
> -		break;
> -	default:
> -		return -EINVAL;
> -	}
> -
> -	return 0;
> -}
> -
> -/* flush queued events of type @type, caller must hold client->buffer_lock */
>  static void __evdev_flush_queue(struct evdev_client *client, unsigned int type)
>  {
>  	unsigned int i, head, num;
> @@ -160,6 +139,41 @@ static void evdev_queue_syn_dropped(struct evdev_client *client)
>  	spin_unlock_irqrestore(&client->buffer_lock, flags);
>  }
>  
> +static int evdev_set_clk_type(struct evdev_client *client,
> +			struct input_dev *dev, unsigned int clkid)
> +{
> +	if (client->clk_type == clkid)
> +		return 0;
> +
> +	switch (clkid) {
> +
> +	case CLOCK_REALTIME:
> +		client->clk_type = EV_CLK_REAL;
> +		break;
> +	case CLOCK_MONOTONIC:
> +		client->clk_type = EV_CLK_MONO;
> +		break;
> +	case CLOCK_BOOTTIME:
> +		client->clk_type = EV_CLK_BOOT;
> +		break;
> +	default:
> +		return -EINVAL;
> +	}
> +
> +	spin_lock_irq(&dev->event_lock);
> +	spin_lock(&client->buffer_lock);
> +	spin_unlock(&dev->event_lock);

Umm, why?

> +
> +	/* Flush clients events after clk_type is changed
> +	 * and queue SYN_DROPPED event.*/
> +	client->packet_head = client->head = client->tail;
> +	spin_unlock_irq(&client->buffer_lock);
> +
> +	evdev_queue_syn_dropped(client);

This is still racy. I'd rather we passed a flag to
evdev_queue_syn_dropped() to indicate it should also clear queue.

Thanks.

-- 
Dmitry

Re: [PATCH] Input:Flush client events on clk_type change

[Anshul Garg]

Dear Mr. Dmitry ,

Thanks a lot for your suggestions.

> + spin_lock_irq(&dev->event_lock);
> + spin_lock(&client->buffer_lock);
> + spin_unlock(&dev->event_lock);

Umm, why?

Yes, there is no need of event_lock as we are modifying client
specific data structure only.

Hence only buffer_lock will guarantee atomicity for flushing of client
pending events buffer.

So i will modify locking mechanism to use buffer_lock only.

> +
> + /* Flush clients events after clk_type is changed
> + * and queue SYN_DROPPED event.*/
> + client->packet_head = client->head = client->tail;
> + spin_unlock_irq(&client->buffer_lock);
> +
> + evdev_queue_syn_dropped(client);

This is still racy. I'd rather we passed a flag to
evdev_queue_syn_dropped() to indicate it should also clear queue.

Can you please tell me in which scenario's this patch is prone to race
condition's?
As i think we are modifying the client's buffer indexes so buffer_lock
would be sufficient.


Yes by adding one more parameter in evdev_queue_syn_dropped function
on the basis
of which we can flush the buffer.

Example ::

static void evdev_queue_syn_dropped(struct evdev_client *client)

It can be changed to

static void evdev_queue_syn_dropped(struct evdev_client *client , bool flush)
{
spin_lock(buffer_lock);

if(flush)
    client->packet_head = client->head = client->tail;


.........

spin_unlock(buffer_lock);
}

OR
Similarly we can extend__evdev_flush_queue function to support
flushing of client event queue.
As currently this function flushes single type of events only.

I think 2nd way is better.

Please give your insignt on above suggested changes.

Thanks
Anshul Garg

Re: [PATCH] Input:Flush client events on clk_type change

[Dmitry Torokhov]

On Thu, Jan 08, 2015 at 07:27:59PM +0530, Anshul Garg wrote:
> Dear Mr. Dmitry ,
> 
> Thanks a lot for your suggestions.
> 
> > + spin_lock_irq(&dev->event_lock);
> > + spin_lock(&client->buffer_lock);
> > + spin_unlock(&dev->event_lock);
> 
> Umm, why?
> 
> Yes, there is no need of event_lock as we are modifying client
> specific data structure only.
> 
> Hence only buffer_lock will guarantee atomicity for flushing of client
> pending events buffer.
> 
> So i will modify locking mechanism to use buffer_lock only.
> 
> > +
> > + /* Flush clients events after clk_type is changed
> > + * and queue SYN_DROPPED event.*/
> > + client->packet_head = client->head = client->tail;
> > + spin_unlock_irq(&client->buffer_lock);
> > +
> > + evdev_queue_syn_dropped(client);
> 
> This is still racy. I'd rather we passed a flag to
> evdev_queue_syn_dropped() to indicate it should also clear queue.
> 
> Can you please tell me in which scenario's this patch is prone to race
> condition's?
> As i think we are modifying the client's buffer indexes so buffer_lock
> would be sufficient.

New events may come up between resetting the queue and queuing EV_SYN
and client would not really know if they contain valid time or not.

> 
> 
> Yes by adding one more parameter in evdev_queue_syn_dropped function
> on the basis
> of which we can flush the buffer.
> 
> Example ::
> 
> static void evdev_queue_syn_dropped(struct evdev_client *client)
> 
> It can be changed to
> 
> static void evdev_queue_syn_dropped(struct evdev_client *client , bool flush)
> {
> spin_lock(buffer_lock);
> 
> if(flush)
>     client->packet_head = client->head = client->tail;
> 
> 
> .........
> 
> spin_unlock(buffer_lock);
> }
> 
> OR
> Similarly we can extend__evdev_flush_queue function to support
> flushing of client event queue.
> As currently this function flushes single type of events only.
> 
> I think 2nd way is better.
> 
> Please give your insignt on above suggested changes.

I do not see how make evdev_flush_queue() to flush all types of events
without adding another parameter that would "override" type, which is
ugly.

It looks like we can make evdev_queue_syn_dropped() zap the old events
unconditionally, so I'd rather do that.

Thanks.

-- 
Dmitry