Re: [PATCH v2] cdc-wdm: fix "out-of-sync" due to missing notifications

[Oliver Neukum <oneukum@suse.com>]

On Mon, 2016-07-04 at 13:54 +0200, Bjørn Mork wrote:
> Oliver Neukum <oneukum@suse.com> writes:
> 
> > On Sun, 2016-07-03 at 21:59 +0200, Bjørn Mork wrote:
> >> 		default:
> >> @@ -200,10 +200,29 @@ static void wdm_in_callback(struct urb *urb)
> >>  			desc->reslength = length;
> >>  		}
> >>  	}
> >> +
> >> +	/*
> >> +	 * If desc->resp_count is unset, then the urb was submitted
> >> +	 * without a prior notification.  If the device returned any
> >> +	 * data, then this implies that it had messages queued without
> >> +	 * notifying us.  Continue reading until that queue is flushed.
> >> +	 */
> >> +	if (!desc->resp_count) {
> >> +		if (!length) {
> >> +			/* do not propagate the expected -EPIPE */
> >> +			desc->rerr = 0;
> >> +			goto unlock;
> >> +		}
> >> +		dev_dbg(&desc->intf->dev, "got %d bytes without notification\n", length);
> >> +		set_bit(WDM_RESPONDING, &desc->flags);
> >> +		usb_submit_urb(desc->response, GFP_ATOMIC);
> >
> > You must check for being in an overflow condition.
> 
> No, I don't think that will give the wanted/expected result.  The main
> point here is to flush whatever the data the device has queued up, but

That raises the point why we store the data at all.

> haven't notified us about (or for which we've lost the notification for
> some reason). We want to continue flushing whether or not we can store
> the received data.  The goal is to get back into a syncronouos state,
> not necessarily to save everything the device has queued.  If we
> overflow, then the lines just before this hunk will handle that just
> fine by setting the WDM_OVERFLOW bit and dropping the data.  That's what
> we want.

OK, you know the protocol better than I.

> >> +	}
> >> +
> >>  skip_error:
> >>  	wake_up(&desc->wait);
> >>  
> >>  	set_bit(WDM_READ, &desc->flags);
> >> +unlock:
> >>  	spin_unlock(&desc->iuspin);
> >>  }
> >>  
> >> @@ -647,6 +666,16 @@ static int wdm_open(struct inode *inode, struct file *file)
> >
> > If you go for that approack you also need to do it
> > in resume()
> 
> Yes, I wondered about that...  But I didn't add it because I am unable
> to provoke the problem there, so I cannot really test if it has any
> positive effect.  Do you still want it?

Yes. I think the window is small, but we don't want strange
irreproducible flukes.

	Regards
		Oliver