netdev - Re: [PATCH 2/2 v2] sierra_net: fix issues with SYNC/RESTART messages and interrupt pipe setup

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <1360256793.2382.27.camel@dcbw.foobar.com>
Date:	Thu, 07 Feb 2013 11:06:33 -0600
From:	Dan Williams <dcbw@...hat.com>
To:	Bjørn Mork <bjorn@...k.no>
Cc:	Oliver Neukum <oliver@...kum.org>,
	Elina Pasheva <epasheva@...rrawireless.com>,
	netdev@...r.kernel.org, linux-usb@...r.kernel.org,
	Rory Filer <rfiler@...rrawireless.com>,
	Phil Sutter <phil@....cc>
Subject: Re: [PATCH 2/2 v2] sierra_net: fix issues with SYNC/RESTART
 messages and interrupt pipe setup

On Wed, 2013-02-06 at 22:11 +0100, Bjørn Mork wrote:
> Dan Williams <dcbw@...hat.com> writes:
> 
> > As part of the initialization sequence, the driver sends a SYNC message
> > via the control pipe to the firmware, which appears to request a
> > firmware restart.  The firmware responds with an indication via the
> > interrupt pipe set up by usbnet.  If the driver does not receive a
> > RESTART indication within a certain amount of time, it will periodically
> > send additional SYNC messages until it receives the RESTART indication.
> >
> > Unfortunately, the interrupt URB is only submitted while the netdev
> > is open, which is usually not the case during initialization, and thus
> > the firmware's RESTART indication is lost.  So the driver continues
> > sending SYNC messages, and eventually the firmware crashes when it
> > receives too many.  This leads to a wedged netdev.
> >
> > To ensure the firmware's RESTART indications can be received by the
> > driver, request that usbnet keep the interrupt URB active via
> > FLAG_INTR_ALWAYS.
> >
> > Second, move the code that sends the SYNC message out of the
> > bind() hook and after usbnet_probe() to ensure the interrupt URB
> > is set up before trying to use it.
> 
> Given this description I am wondering if you couldn't just move the
> whole SYNC thing to a new reset() hook, using some private flag to make
> sure it only runs once?  Does it really need to start at probe time?

It doesn't need to run exactly at probe, but it appears to need to be
the first thing the driver does when communicating with the firmware to
ensure clear state and whatnot.  Possibly like the QMI SYNC message that
clears all the client IDs and resets the internal stack.  (the driver
also sends a "shutdown" message to the firmware when unbinding).

So I do think that somewhere around probe() is the best time to do this,
because it's best to initialize the device when the driver binds to it
and react to errors as soon as possible, rather than trying to set
everything up on open/IFF_UP and then fail right before you want to
actually use the device.  Late-fail is quite unhelpful for applications.

I don't really care if it happens in probe() or somewhere else right
after the driver is bound to the device, but it should be part of the
initialization process.

Dan

--
To unsubscribe from this list: send the line "unsubscribe netdev" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html