Re: [PATCH v2] usb: dwc2: call dwc2_is_controller_alive() under spinlock

Felipe Balbi
2015-01-14T22:40:39Z

[-- Attachment #1: Type: text/plain, Size: 2762 bytes --]

On Wed, Jan 14, 2015 at 10:28:54PM +0000, Paul Zimmerman wrote:
> > From: Felipe Balbi [mailto:balbi@ti.com]
> > Sent: Wednesday, January 14, 2015 1:46 PM
> >
> > On Wed, Jan 14, 2015 at 04:41:23PM -0500, Alan Stern wrote:
> > > > > > This is really, really odd. Register accesses are atomic, so the lock
> > > > > > isn't really doing anything. Besides, you're calling
> > > > > > dwc2_is_controller_alive() from within the IRQ handler, so IRQs are
> > > > > > already disabled.
> > > > >
> > > > > Spinlocks sometimes do more than you think. For instance, here the
> > > > > lock prevents the register access from happening while some other CPU
> > > > > is holding the lock. If a silicon quirk causes the register access to
> > > > > interfere with other activities, this could be important.
> > > >
> > > > readl() (which is used by dwc2_is_controller_alive()) adds a memory
> > > > barrier to the register accesses, that should force all register
> > > > accesses the be correctly ordered.
> > >
> > > Memory barriers will order accesses that are all made on the same CPU
> > > with respect to each other. They do not order these accesses against
> > > accesses made from another CPU -- that's why we have spinlocks. :-)
> >
> > a fair point :-) The register is still read-only, so that shouldn't
> > matter either :-)
> >
> > > > I fail to see how a silicon quirk
> > > > could cause this and if, indeed, it does, I'd be more comfortable with a
> > > > proper STARS tickect number from synopsys :-s
> > >
> > > Maybe accessing this register somehow resets something else. I don't
> > > know. It seems unlikely, but at least it explains how adding a
> > > spinlock could fix the problem.
> >
> > I would really need Paul (or someone at Synopsys) to confirm this
> > somehow. Maybe it has something to do with how the register is
> > implemented, dunno.
> >
> > Paul, do you have any idea what could cause this ? Could the HW into
> > some weird state if we read GSNPSID at random locations or when data is
> > being transferred, or anything like that ?
>
> Only thing I can think of is that there is some silicon bug in Robert's
> platform. But I am not aware of any STARs that mention accesses to the
> GSNPSID register as being problematic.
>
> Funny thing is, this code has been basically the same since at least
> November 2013. So I think some other recent change must have modified
> the timing of the register accesses, or something like that. But that's
> just handwaving, really.

Alright, I'll apply this patch but for 3.20 with a stable tag as I have
already sent my last pull request to Greg. Unless someone has a really
big complaint about doing things as such.

--
balbi

[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 819 bytes --]