Re: [PATCH] usb: dwc2: Fix shutdown callback in platform

[Alan Stern <stern@rowland.harvard.edu>]

On Fri, May 29, 2020 at 11:45:53AM -0700, Doug Anderson wrote:
> Hi,
> 
> On Fri, May 29, 2020 at 11:21 AM Frank Mori Hess <fmh6jj@gmail.com> wrote:
> >
> > On Fri, May 29, 2020 at 1:53 PM Doug Anderson <dianders@chromium.org> wrote:
> > > >
> > > > I don't get it.  A hypothetical machine could have literally anything
> > > > sharing the IRQ line, right?
> > >
> > > It's not a real physical line, though?  I don't think it's common to
> > > have a shared interrupt between different IP blocks in a given SoC.
> > > Even if it existed, all the drivers should disable their interrupts?
> >
> > I don't know, it's a hypothetical machine so it can be whatever you
> > want.  The driver requests shared irqs, if it doesn't actually support
> > irq sharing, it shouldn't request them.
> 
> I guess?  As I understood it drivers have to be very carefully coded
> up to support sharing their IRQ with someone else and I'm not
> convinced dwc2 does that anyway.  Certainly it doesn't hurt to keep
> dwc2 clean, but until I see someone that's actually sharing dwc2's
> interrupt and I can actually see an example I'm not sure I'm going to
> spend too much time thinking about it.

This is silly.  If the driver says it supports shared IRQs, then it 
should actually support them.

> > > > Anyways, my screaming interrupt occurs after a a new kernel has been
> > > > booted with kexec.  In this case, it doesn't matter if the old kernel
> > > > called disable_irq or not.  As soon as the new kernel re-enables the
> > > > interrupt line, the kernel immediately disables it again with a
> > > > backtrace due to the unhandled screaming interrupt.  That's why the
> > > > dwc2 hardware needs to have its interrupts turned off when the old
> > > > kernel is shutdown.
> > >
> > > Isn't that a bug with your new kernel?  I've seen plenty of bugs where
> > > drivers enable their interrupt before their interrupt handler is set
> > > to handle it.  You never know what state the bootloader (or previous
> > > kernel) might have left things in and if an interrupt was pending it
> > > shouldn't kill you.
> >
> > It wouldn't hurt to add disabling of the dwc2 irq early in dwc2
> > initialization,
> 
> More than it not hurting, I'd consider it a bug in the driver (and a
> much more serious one than shutdown not disabling the interrupt).

Normally the first thing a driver would do is reset the hardware, and 
that reset should disable any interrupt source.

> > but why leave the irq screaming after shutdown?
> 
> Sure.  So I guess the answer is to just do both disable the interrupt
> and make sure that the interrupt handler has finished.
> 
> dwc2_disable_global_interrupts(hsotg);
> disable_irq(hsotg->irq);

Drivers with shared IRQs don't call disable_irq(); they call 
synchronize_irq().  It will do what you want (wait until all running 
handlers have returned).

Alan Stern