lkml.org 
[lkml]   [2011]   [Apr]   [4]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
/
From
Date
SubjectRe: [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/

\
 
 \ /
  Last update: 2011-04-05 02:37    [W:0.095 / U:0.104 seconds]
©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site