Am Freitag, 20. Januar 2012, 12:23:11 schrieb Bjørn Mork:
> Oliver Neukum <oliver@xxxxxxxxxx> writes:
> > Am Freitag, 20. Januar 2012, 01:50:01 schrieb Bjørn Mork:
> > And what keeps this from overflowing?
>
> Time. Transferring 2^64 bytes over a DM interface is not going to
> happen. OK, understand that is not an argument and that the unlikely
> errors *will* happen to someone someday.... But as the effect of an
> overflow only is that readers will stop reading, I was prepared to
> ignore it.
>
> I guess your question means that I need to fix this?
Yes, simply treat it as signed like we handle overflows of jiffies.
> >> - if (desc->length == 0) {
> >> - desc->read = 0;
> >> -retry:
> >> + if (*ppos == desc->pos) {
> >> if (test_bit(WDM_DISCONNECTING, &desc->flags)) {
> >> rv = -ENODEV;
> >> goto err;
> >> }
> >> i++;
> >
> > What is the function of i?
>
>
> Beats me :-)
I believe IIRC it was used in a dev_dbg long removed. I declare open
season on it.
> >
> >> } else {
> >> rv = wait_event_interruptible(desc->wait,
> >> - test_bit(WDM_READ, &desc->flags));
> >> + (*ppos < desc->pos) ||
> >> + test_bit(WDM_DISCONNECTING, &desc->flags));
> >
> > Again, this is wrong.
> >
> > 1. You fail to test for an error condition as a reason for a wakeup
>
> Yes, needs fixing
OK
> > 2. As you test for a disconnection here, you must be able to handle it
>
> I do, by returning through the normal path.
This is exactly what you must not do. Or if you do you must alter
the normal path. Device removal is an error condition that must be
reported.
It would be best to add a second check for disconnection to the
general path, even in the case data is available.
> > 3. As you have removed the loop and you are rightly keeping the interruptible
> > sleep, you must be able to handle signals.
>
> Yes, needs fixing.
OK
>
> >> /* may have happened while we slept */
> >> @@ -397,50 +426,32 @@ retry:
> >> goto err;
> >> }
> >>
> >> - spin_lock_irq(&desc->iuspin);
> >> -
> >> - if (desc->rerr) { /* read completed, error happened */
> >> - desc->rerr = 0;
> >
> > No, you cannot simply remove unsetting the error.
>
> It's unset (or actually set) by the callback. The errors are shared
That callback may never come.
> among the readers and we cannot have one of them resetting it. They all
> need to see the error.
Oh well. First off all you need to get the single reader case right:
do {
data = read();
if ((error = errno) < 0)
handle_error();
else
process(data);
}
while ((!error && error != -EAGAIN) && !data)
must work even with O_NONBLOCK. That requires resetting the error.
If really, really all readers must see an error, you must record at what
position the error happened.
> > And you need to hold the spinlock to handle the race with the callback.
>
> Only if setting it, I presume?
Yes.
> So: Giving the same data to everyone is the goal of this patch.
Very well. So be it.
> > And what happens if the callback arrives here? You'll deliver
> > partially old and partially new data.
>
> True. I need to ensure that the part of the buffer being updated is
> locked against reading. Will fix that.
OK
> >> + /* move file position */
> >> + *ppos += cntr;
> >
> > And this races in case of multiple readers.
>
> It does? How?
A fool may use clone() and share the position.
You must use a lock.
Regards
Oliver
--
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
