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
--
To unsubscribe from this list: send the line "unsubscribe linux-input" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
