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

["Bjørn Mork" <bjorn@mork.no>]

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



>> +	}
>> +
>>  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?


Bjørn