All of lore.kernel.org
 help / color / mirror / Atom feed
From: Oliver Neukum <oneukum@suse.com>
To: "Bjørn Mork" <bjorn@mork.no>
Cc: Greg Kroah-Hartman <gregkh@linuxfoundation.org>,
	linux-usb@vger.kernel.org, stable@vger.kernel.org
Subject: Re: [PATCH v2] cdc-wdm: fix "out-of-sync" due to missing notifications
Date: Tue, 05 Jul 2016 14:52:50 +0200	[thread overview]
Message-ID: <1467723170.1909.11.camel@suse.com> (raw)
In-Reply-To: <87shvpcp7w.fsf@nemi.mork.no>

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



  reply	other threads:[~2016-07-05 12:57 UTC|newest]

Thread overview: 9+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2016-07-03 19:59 [PATCH v2] cdc-wdm: fix "out-of-sync" due to missing notifications Bjørn Mork
2016-07-04  8:04 ` Oliver Neukum
2016-07-04 11:54   ` Bjørn Mork
2016-07-04 12:48     ` Oliver Neukum
2016-07-04 13:09       ` Bjørn Mork
2016-07-04 17:01         ` Bjørn Mork
2016-07-05 12:52           ` Oliver Neukum [this message]
2016-07-09 18:31             ` Bjørn Mork
2016-07-10 12:47               ` Oliver Neukum

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=1467723170.1909.11.camel@suse.com \
    --to=oneukum@suse.com \
    --cc=bjorn@mork.no \
    --cc=gregkh@linuxfoundation.org \
    --cc=linux-usb@vger.kernel.org \
    --cc=stable@vger.kernel.org \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line before the message body.
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.