Re: [PATCH v2 4/4] input: evdev: Make device readable only when it contains a complete packet.
From: Jeffrey Brown <hidden>
Date: 2011-04-04 22:16:55
Also in:
lkml
Hi Dmitry, I don't think the new patch is completely correct. On Mon, Apr 4, 2011 at 2:36 PM, Dmitry Torokhov [off-list ref] wrote:
I think we should target SYN_REPORT directly. SYN_CONFIG is unused and SYN_DROPPED is not interesting till next SYN_REPORT anyway. Given the changes to the previous patch I have the following:
Explicitly checking for SYN_REPORT makes sense. I wasn't sure to do with SYN_CONFIG before so I tried to keep the condition somewhat conservative. Per previous comments on an older iteration of this patch, it probably makes sense to calculate this flag once in evdev_event and pass it to evdev_pass_event. bool full_sync = (type == EV_SYN && code == SYN_REPORT);
quoted hunk ↗ jump to hunk
@@ -41,6 +41,7 @@ struct evdev {struct evdev_client { unsigned int head; unsigned int tail; + unsigned int last_syn; /* position of the last EV_SYN/SYN_REPORT */
This comment for last_syn is not quite right. We need last_syn to refer to the position just beyond the last sync. Otherwise the device will not become readable until another event is written there. The invariants for last_syn should be similar to those for head. Whereas tail != head means buffer non-empty, tail != last_syn should mean buffer is readable. It looks like we almost maintain those invariants here, except for SYN_DROPPED.
quoted hunk ↗ jump to hunk
spinlock_t buffer_lock; /* protects access to buffer, head and tail */ struct fasync_struct *fasync; struct evdev *evdev;@@ -72,12 +73,16 @@ static void evdev_pass_event(struct evdev_client *client,client->buffer[client->tail].type = EV_SYN; client->buffer[client->tail].code = SYN_DROPPED; client->buffer[client->tail].value = 0; +
Should use client->head here so that the SYN_DROPPED is readable.
+ client->last_syn = client->tail; } spin_unlock(&client->buffer_lock);
Can use full_sync or something equivalent instead of repeating the condition on EV_SYN / SYN_REPORT here.
- if (event->type == EV_SYN)
+ if (event->type == EV_SYN && event->code == SYN_REPORT) {I don't think it's safe to modify last_syn outside of the spin lock. This should be done above.
+ client->last_syn = client->head; kill_fasync(&client->fasync, SIGIO, POLL_IN); + } }
MISSING: We need to also modify evdev_event to only call
wake_up_interruptible when enqueuing a sync. It does not make sense
to wake up waiters unless the device is about to become readable
again.
This also means we should wake after having written SYN_DROPPED. We
might need to make evdev_pass_event return (or take by reference) a
boolean that indicates whether at least one client has become
readable.
Pseudo-code:
if (full_sync || evdev_became_readable_for_a_client_due_to_syn_dropped)
wake_up_interruptible(&evdev->wait);
quoted hunk ↗ jump to hunk
/*@@ -387,12 +392,12 @@ static ssize_t evdev_read(struct file *file, char __user *buffer,if (count < input_event_size()) return -EINVAL; - if (client->head == client->tail && evdev->exist && + if (client->last_syn == client->tail && evdev->exist && (file->f_flags & O_NONBLOCK)) return -EAGAIN; retval = wait_event_interruptible(evdev->wait, - client->head != client->tail || !evdev->exist); + client->last_syn != client->tail || !evdev->exist); if (retval) return retval;@@ -421,7 +426,7 @@ static unsigned int evdev_poll(struct file *file, poll_table *wait)poll_wait(file, &evdev->wait, wait); mask = evdev->exist ? POLLOUT | POLLWRNORM : POLLHUP | POLLERR; - if (client->head != client->tail) + if (client->last_syn != client->tail) mask |= POLLIN | POLLRDNORM; return mask;
It looks to me like this patch isn't based on top of your previous patch for SYN_DROPPED. Specifically, the SYN_DROPPED should be inserted before the newly enqueued event but I don't see that above. Jeff. -- To unsubscribe from this list: send the line "unsubscribe linux-input" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html