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

On Thu, Jan 08, 2015 at 07:27:59PM +0530, Anshul Garg wrote:

Dear Mr. Dmitry ,

Thanks a lot for your suggestions.

quoted

+ 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.

quoted

+
+ /* 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