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.
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)
Linus
--
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
