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

[Dmitry Torokhov <dmitry.torokhov@gmail.com>]

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