Dmitry Torokhov wrote:
> Hi Henrik,
>
> On Sun, Jun 20, 2010 at 08:48:54PM +0200, Henrik Rydberg wrote:
>> @@ -149,11 +148,29 @@ static int evdev_grab(struct evdev *evdev, struct evdev_client *client)
>>
>> static int evdev_ungrab(struct evdev *evdev, struct evdev_client *client)
>> {
>> + struct input_dev *dev = evdev->handle.dev;
>> + int head, tail;
>> +
>> if (evdev->grab != client)
>> return -EINVAL;
>>
>> + spin_lock_irq(&dev->event_lock);
>> +
>> + head = client->head;
>> + tail = client->tail;
>> +
>> + rcu_read_lock();
>> + list_for_each_entry_rcu(client, &evdev->client_list, node) {
>> + client->head = head;
>> + client->tail = tail;
>> + }
>> + rcu_read_unlock();
>> +
>> rcu_assign_pointer(evdev->grab, NULL);
>> synchronize_rcu();
>
> You may not call synchronize_rcu() while holding spinlock and with
> interrupts off.
Thanks.
>> +
>> + spin_unlock_irq(&dev->event_lock);
>> +
>> input_release_device(&evdev->handle);
>>
>> return 0;
>> @@ -162,6 +179,7 @@ static int evdev_ungrab(struct evdev *evdev, struct evdev_client *client)
>> static void evdev_attach_client(struct evdev *evdev,
>> struct evdev_client *client)
>> {
>> + client->head = client->tail = evdev->head;
>> spin_lock(&evdev->client_lock);
>
> So what happens if we advance evdev->head here but before we added the
> new client to the list? I guess we'll have to wait till next event and
> then client will read both of them...
Right...
> Overall I am starting getting concerned about proper isolation between
> clients. Right now, if one client stops reading events and another one
> issues grab then the first client will only get events that were
> accumulated before grab tookm place. With the new shared buffer the
> first client may get "grabbed" events if it stop for long enough for
> buffer to wrap around.
Doing some research, the semantics of ioctl have obviously been discussed
before, and I believe this points to another such issue. When grabbing a device,
are we guaranteeing that the device no longer sends events to other clients, or
are we guaranteeing that other clients can no longer read the device? If the
latter, clearing all client buffers in conjunction with a grab would be
appropriate, and would solve this issue.
> Do we really same that much memory here? We normally do not have that
> many users connected to event devices at once...
Ok, let's scratch this. Although I think the idea of multi-reader buffers is
sound, it is obviously sufficiently incompatible with the current approach to
produce distastefully complex patches. I will return with a new set which only
fixes the buffer resize problem, and leaves the rest for later.
Thanks,
Henrik
--
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
