From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from mx2.suse.de ([195.135.220.15]:56709 "EHLO mx2.suse.de" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1751762AbcGEM5B (ORCPT ); Tue, 5 Jul 2016 08:57:01 -0400 Message-ID: <1467723170.1909.11.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: Tue, 05 Jul 2016 14:52:50 +0200 In-Reply-To: <87shvpcp7w.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> <1467636489.9978.9.camel@suse.com> <871t39eehv.fsf@nemi.mork.no> <87shvpcp7w.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 19:01 +0200, Bjørn Mork wrote: > The proposed driver solution is a hack, allowing us to resyncronize > without resetting the firmware. We have no reliable way to reset the > firmware and doing so will have large side effects anyway. The hack > allows us to continue, with a possibly lost message or two as the only > consequences. We don't have to break any existing session etc. OK, then I propose it is time to bring out the sledge hammer. How about changing usb_cdc_wdm_register() to include a flag about the necessity to drain the buffers? > But it turns out that it is a real problem for the very same MBIM > firmwares which caused us to need the hack in the first place: They > stall when you try to read and there is no data. Which means that a > lost race can end up with an EPIPE being returned to _read(), where it > is translated and returned to userspace. This sucks and it tells me even more that CDC MBIM should tell us when to apply this work around at startup. > This is a mess. I fully agree that it looks like this either should go > into both open() and resume(), or none. The real answer is 'none'. > > But we need the hack for the yucky MBIM firmwares. Putting it in open() > is defining open() as a syncronization point for something which should > not need syncronization in a perfect world. I can expand the comment > there a bit to make this clear. Making resume() a syncronization point > as well is only making things worse. I'd say add a flag, let CDC MBIM set it unconditionally (we can switch to a black list if we absolutely need to) and include a big, fat commentary. There is no good solution for this problem. Regards Oliver