From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from mx2.suse.de ([195.135.220.15]:51981 "EHLO mx2.suse.de" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1753352AbcGDMwT (ORCPT ); Mon, 4 Jul 2016 08:52:19 -0400 Message-ID: <1467636489.9978.9.camel@suse.com> Subject: Re: [PATCH v2] cdc-wdm: fix "out-of-sync" due to missing notifications From: Oliver Neukum To: =?ISO-8859-1?Q?Bj=F8rn?= Mork Cc: Greg Kroah-Hartman , linux-usb@vger.kernel.org, stable@vger.kernel.org Date: Mon, 04 Jul 2016 14:48:09 +0200 In-Reply-To: <87d1mt62kc.fsf@nemi.mork.no> References: <1467575977-26693-1-git-send-email-bjorn@mork.no> <1467619450.9978.3.camel@suse.com> <87d1mt62kc.fsf@nemi.mork.no> Content-Type: text/plain; charset="UTF-8" Mime-Version: 1.0 Content-Transfer-Encoding: 8bit Sender: stable-owner@vger.kernel.org List-ID: On Mon, 2016-07-04 at 13:54 +0200, Bjørn Mork wrote: > Oliver Neukum 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