On Sat, Mar 06, 2010 at 09:59:14AM -0800, Linus Torvalds wrote:
>
>
> On Sat, 6 Mar 2010, Dmitry Torokhov wrote:
> >
> > Subject: USB: simplify /proc/bus/usb/devices handling
> > From: Dmitry Torokhov <dmitry.torokhov@xxxxxxxxx>
>
> Ack. The whole point of the poll_wait() logic is that you can add
> yourself to the wait-queue before doing the tests, so that you
> automatically get race-free behavior without having to hold any locks
> for the event in question over the whole thing.
>
> That said, I still do think conndiscevcnt should be atomic, or at a
> _minimum_ there should be an ACCESS_ONCE there:
>
> > + poll_wait(file, &deviceconndiscwq, wait);
> > + if (file->f_version != conndiscevcnt) {
> > + file->f_version = conndiscevcnt;
> > + return POLLIN | POLLRDNORM;
> > }
>
> iow it should probably look something like this:
>
> unsigned int count;
>
> poll_wait(file, &deviceconndiscwq, wait);
> count = ACCESS_ONCE(conndiscevcnt);
> if (file->f_version != conndiscevcnt) {
> file->f_version = conndiscevcnt;
> return POLLIN | POLLRDNORM;
> }
> return 0;
>
> because otherwise the compiler might do the two 'conndiscevcnt' accesses
> as two accesses (it probably won't, and will optimize it, but it certainly
> _can_), in which case we might write a _different_ conndiscevcnt count
> into the f_version than the one we just tested, and then lose the next
> event.
>
Which should still be fine. If a new device is added after we did the
test but before the assignment we may store the newest count in
f_version but it is OK - userpsace wehn woken up will read _both_ which
is what we want.
> Or something. It's quite possible that for poll we never care, because it
> doesn't matter if we lose the next event, because a poll user needs to
> read all the old events anyway.
>
> (Technically, I suspect that it's the "read the old events" case that
> _should_ reset the f_version thing, and not the poll routine)
That would be another option but we lived with the current behavior (we
wake up once and if userspace decides not to read don't wake it up again
till next event) for many years so I am not sure if it worth chnging it.
--
Dmitry
--
To unsubscribe from this list: send the line "unsubscribe linux-usb" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
