From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from mx2.suse.de ([195.135.220.15]:49603 "EHLO mx2.suse.de" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1751191AbcEQVwh (ORCPT ); Tue, 17 May 2016 17:52:37 -0400 Message-ID: <1463521746.21262.6.camel@suse.com> Subject: Re: [PATCH] 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: Tue, 17 May 2016 23:49:06 +0200 In-Reply-To: <87k2is1o8x.fsf@nemi.mork.no> References: <1463153977-19771-1-git-send-email-bjorn@mork.no> <87mvntevwy.fsf@nemi.mork.no> <1463476438.19237.3.camel@suse.com> <87k2is1o8x.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 Tue, 2016-05-17 at 21:24 +0200, Bjørn Mork wrote: > Oliver Neukum writes: > > > On Fri, 2016-05-13 at 18:59 +0200, Bjørn Mork wrote: > >> Bjørn Mork writes: > >> > >> > The driver enforces a strict one-to-one relationship between the > >> > received RESPONSE_AVAILABLE notifications and messages read from > >> > the device. At the same time, it will cancel the interrupt URB > >> > when there is no client holding the character device open. > >> > >> Never mind. Forget it. > >> > >> This patch breaks other devices again. The immediate and unconditional > >> reading make them barf. I guess it can be worked around by delaying the > >> flushing until at least one notification is received, but I obviously > >> have to test this theory thoroughly on all devices I have. > > > > Hi, > > > > I think the best approach would be to keep the interrupt URB always > > active. I didn't do this to conserve bandwidth, but if it makes devices > > work, it certainly would be the best option. > > Yes, I considered that. But this implies purging the device message > queue without telling userspace that we did so. At least with the > current driver design, which is based on a single limited size > buffer. If the device queues a number of unsolictied messages between > two userspace requests, then we really want all those unsolicted > messages delivered to the userspace program on the second request. You might argue that if user space wants the data it should open the device. > And I do think the original bandwidth (and power) conservative approach > is worth keeping too. There is no point in waking up these devices > unless there actually is an interested userspace application. They can sleep just fine. I did not imply that runtime PM should be disabled. > FWIW, my initial analysis of the problem with the patch was too quick > imprecise. The problem is simply the -EPIPE status we inevitably will > hit when the queue is empty, as I should have anticipated. It will be > returned to userspace translated to -EIO. I am currently testing a > version taking care of that, and it seems to behave well so far. I'll > submit it as soon as I am absoltely sure that it works on all WDM, QMI > and MBIM devices I have. Might take some time, since I am running out > of mini-PCIe and m.2 adapters.. That looks a bit risky. Firstly, if you get -EPIPE after a notification it is an error and must be reported as such, so you need an additional state. And what do you do after -EPIPE? Do you clean up the stall or not? And the fun really starts if you get a notification while you clean the stall. And are you sure all devices can cope with an unsolicited request? Regards Oliver