Messages in this thread | | | From | Jeffrey Brown <> | Date | Mon, 4 Apr 2011 17:34:09 -0700 | Subject | Re: [PATCH v2 4/4] input: evdev: Make device readable only when it contains a complete packet. |
| |
Dmitry,
On Mon, Apr 4, 2011 at 3:46 PM, Dmitry Torokhov <dmitry.torokhov@gmail.com> wrote: > On Mon, Apr 04, 2011 at 03:16:13PM -0700, Jeffrey Brown wrote: >> bool full_sync = (type == EV_SYN && code == SYN_REPORT); > > I am not sure what is cheaper - 2 conditionals or stack manipulation > needed to push another argument if we happed to be register-starved.
Not a question of the computational cost. It was mostly done to avoid repeating the same predicate in multiple places. This was one of the suggested improvements to my earlier patch.
>> 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. > > Hm, yes, comment is incorrect. Given this fact I do not like the name > anymore either (nor do I like 'end'). Need to think about something > better.
Heh, I faced this very same dilemma. I tried 'last_sync', 'readable_tail', 'read_end', and others before settling on 'end' and a descriptive comment.
>> Should use client->head here so that the SYN_DROPPED is readable. > > It is readable, but we do not want to signal on it.
I think we do want to signal on it. We should signal whenever the device becomes readable.
Signaling on dropped is useful in the case where a misbehaving device driver fails to ever call input_sync. If that happens, we might enqueue a dropped event and then never wake up the client which makes the issue hard to diagnose.
>> I don't think it's safe to modify last_syn outside of the spin lock. >> This should be done above. > > This is the only writer, plus we are running under event_lock with > interrupts off, so it is safe.
The value will be read concurrently by evdev_fetch_next_event. So if this were safe, then we wouldn't need the spin lock at all.
At the very least for the sake of consistency, I think we should keep the buffer manipulations within the guarded region.
Jeff. -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
| |