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

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

[Minas Harutyunyan]

To avoid lot of interrupts from dwc2 core, which can be asserted in
specific conditions need to disable interrupts on HW level instead of
disable IRQs on Kernel level, because of IRQ can be shared between
drivers.

Cc: stable@vger.kernel.org
Fixes: a40a00318c7fc ("usb: dwc2: add shutdown callback to platform variant")
Tested-by: Frank Mori Hess <fmh6jj@gmail.com>
Signed-off-by: Minas Harutyunyan <hminas@synopsys.com>
---
 drivers/usb/dwc2/platform.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/usb/dwc2/platform.c b/drivers/usb/dwc2/platform.c
index e571c8ae65ec..ada5b66b948e 100644
--- a/drivers/usb/dwc2/platform.c
+++ b/drivers/usb/dwc2/platform.c
@@ -342,7 +342,7 @@ static void dwc2_driver_shutdown(struct platform_device *dev)
 {
 	struct dwc2_hsotg *hsotg = platform_get_drvdata(dev);
 
-	disable_irq(hsotg->irq);
+	dwc2_disable_global_interrupts(hsotg);
 }
 
 /**
-- 
2.11.0

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

[Doug Anderson]

Hi,

On Fri, May 29, 2020 at 4:51 AM Minas Harutyunyan
<Minas.Harutyunyan@synopsys.com> wrote:
>
> To avoid lot of interrupts from dwc2 core, which can be asserted in
> specific conditions need to disable interrupts on HW level instead of
> disable IRQs on Kernel level, because of IRQ can be shared between
> drivers.
>
> Cc: stable@vger.kernel.org
> Fixes: a40a00318c7fc ("usb: dwc2: add shutdown callback to platform variant")
> Tested-by: Frank Mori Hess <fmh6jj@gmail.com>
> Signed-off-by: Minas Harutyunyan <hminas@synopsys.com>
> ---
>  drivers/usb/dwc2/platform.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/drivers/usb/dwc2/platform.c b/drivers/usb/dwc2/platform.c
> index e571c8ae65ec..ada5b66b948e 100644
> --- a/drivers/usb/dwc2/platform.c
> +++ b/drivers/usb/dwc2/platform.c
> @@ -342,7 +342,7 @@ static void dwc2_driver_shutdown(struct platform_device *dev)
>  {
>         struct dwc2_hsotg *hsotg = platform_get_drvdata(dev);
>
> -       disable_irq(hsotg->irq);
> +       dwc2_disable_global_interrupts(hsotg);
>  }

I could be wrong, but I think it would be better to instead end up
with both calls, like:

dwc2_disable_global_interrupts(hsotg);
disable_irq(hsotg->irq);

To some extent it's slightly overkill, but the disable_irq() function
has the nice "and wait for completion" bit.  Your new call doesn't do
this.

That being said, though, you still won't wait for the completion of
the IRQ handler for the "other drivers" you reference, right.  Maybe a
better fix would be to add a shutdown callback for those other drivers
and just keep relying on disable_irq()?


-Doug

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

[Doug Anderson]

Hi,


On Fri, May 29, 2020 at 9:30 AM Minas Harutyunyan
<Minas.Harutyunyan@synopsys.com> wrote:
>
> Hi Doug,
>
> On 5/29/2020 6:49 PM, Doug Anderson wrote:
> > Hi,
> >
> > On Fri, May 29, 2020 at 4:51 AM Minas Harutyunyan
> > <Minas.Harutyunyan@synopsys.com> wrote:
> >>
> >> To avoid lot of interrupts from dwc2 core, which can be asserted in
> >> specific conditions need to disable interrupts on HW level instead of
> >> disable IRQs on Kernel level, because of IRQ can be shared between
> >> drivers.
> >>
> >> Cc: stable@vger.kernel.org
> >> Fixes: a40a00318c7fc ("usb: dwc2: add shutdown callback to platform variant")
> >> Tested-by: Frank Mori Hess <fmh6jj@gmail.com>
> >> Signed-off-by: Minas Harutyunyan <hminas@synopsys.com>
> >> ---
> >>   drivers/usb/dwc2/platform.c | 2 +-
> >>   1 file changed, 1 insertion(+), 1 deletion(-)
> >>
> >> diff --git a/drivers/usb/dwc2/platform.c b/drivers/usb/dwc2/platform.c
> >> index e571c8ae65ec..ada5b66b948e 100644
> >> --- a/drivers/usb/dwc2/platform.c
> >> +++ b/drivers/usb/dwc2/platform.c
> >> @@ -342,7 +342,7 @@ static void dwc2_driver_shutdown(struct platform_device *dev)
> >>   {
> >>          struct dwc2_hsotg *hsotg = platform_get_drvdata(dev);
> >>
> >> -       disable_irq(hsotg->irq);
> >> +       dwc2_disable_global_interrupts(hsotg);
> >>   }
> >
> > I could be wrong, but I think it would be better to instead end up
> > with both calls, like:
> >
> > dwc2_disable_global_interrupts(hsotg);
> > disable_irq(hsotg->irq);
> >
> > To some extent it's slightly overkill, but the disable_irq() function
> > has the nice "and wait for completion" bit.  Your new call doesn't do
> > this.
> >
> If dwc2 currently handling some interrupt then below patch can allow to
> wait until interrupt will be handled:
>
> spin_lock(&hsotg->lock);
> dwc2_disable_global_interrupts(hsotg);
> spin_unlock(&hsotg->lock);

Would that really work?  If you've got a two core system and the
interrupt is just firing on a different core but hasn't acquired the
spinlock then your code might get the spinlock, disable the
interrupts, and then release the spinlock.  The interrupt handler will
still be running on the other CPU and now will get the spinlock.


> but on other hand dwc2 have 3 subsequent interrupt handlers - core,
> gadget, host and not clear which of handler completed.
>
> > That being said, though, you still won't wait for the completion of
> > the IRQ handler for the "other drivers" you reference, right.  Maybe a
> > better fix would be to add a shutdown callback for those other drivers
> > and just keep relying on disable_irq()?
> >
> I have look to other drivers where used disable_irq() - no any driver
> care about SHARED irq's. In that case your suggestion to use both
> disabling is looks Ok.

I'm not sure I understand.  Are you saying that you'll just add
shutdown callbacks to all the drivers using this IRQ and call
disable_irq() there?  That seems like the best solution to me.

-Doug

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

[Frank Mori Hess]

On Fri, May 29, 2020 at 12:37 PM Doug Anderson <dianders@chromium.org> wrote:
>
> I'm not sure I understand.  Are you saying that you'll just add
> shutdown callbacks to all the drivers using this IRQ and call
> disable_irq() there?  That seems like the best solution to me.

I don't get it.  A hypothetical machine could have literally anything
sharing the IRQ line, right?  If it is important to call disable_irq
during shutdown (I have no idea if it is) then shouldn't the kernel
core just disable all irqs after calling all the driver shutdown
callbacks?

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.


-- 
Frank

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

[Doug Anderson]

Hi,

On Fri, May 29, 2020 at 10:45 AM Frank Mori Hess <fmh6jj@gmail.com> wrote:
>
> On Fri, May 29, 2020 at 12:37 PM Doug Anderson <dianders@chromium.org> wrote:
> >
> > I'm not sure I understand.  Are you saying that you'll just add
> > shutdown callbacks to all the drivers using this IRQ and call
> > disable_irq() there?  That seems like the best solution to me.
>
> 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?


> If it is important to call disable_irq
> during shutdown (I have no idea if it is) then shouldn't the kernel
> core just disable all irqs after calling all the driver shutdown
> callbacks?

It seems like a reasonable idea for this to be in the core but I
haven't dug into whether the core has enough knowledge or if there
would be other problems.


> 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.

-Doug

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

[Frank Mori Hess]

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.

> > 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, but why leave the irq screaming after shutdown?  If
there is another device using the same irq, it will generate unhandled
interrupt backtraces and get its irq disabled when the new kernel
requests its irq, if the device's driver is loaded before the dwc2
driver (assuming the new kernel even has a dwc2 driver).  The dwc2
driver in its current state will generate unhandled interrupt
backtraces by itself until it registers the right handler.

-- 
Frank

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

[Doug Anderson]

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.


> > > 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).


> 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);


-Doug

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

[Alan Stern]

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

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

[Doug Anderson]

Hi,

On Fri, May 29, 2020 at 12:00 PM Alan Stern <stern@rowland.harvard.edu> wrote:
>
> 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.

Ah.  The IRQ here is "shared" because of the way that the dwc2 driver
is architected.  The "gadget" mode and "host" mode driver "share" the
IRQ.  ...but there is no non-dwc2 device sharing.  In other words,
it's not like there's a UART or and i2c device that would share it.


> > > > > 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.

Yup.


> > > 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).

OK.

-Doug

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

[Frank Mori Hess]

On Fri, May 29, 2020 at 3:33 PM Minas Harutyunyan
<Minas.Harutyunyan@synopsys.com> wrote:
> So, can we conclude on this solution?
>
> dwc2_disable_global_interrupts(hsotg);
> synchronize_irq(hsotg->irq)

That solution is fine with me.  Disabling the dwc2 interrupt sources
in initialization before the handlers are registered is also worth
doing, but it doesn't have to be in the same patch.

-- 
Frank

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

[Doug Anderson]

Hi,

On Fri, May 29, 2020 at 12:45 PM Frank Mori Hess <fmh6jj@gmail.com> wrote:
>
> On Fri, May 29, 2020 at 3:33 PM Minas Harutyunyan
> <Minas.Harutyunyan@synopsys.com> wrote:
> > So, can we conclude on this solution?
> >
> > dwc2_disable_global_interrupts(hsotg);
> > synchronize_irq(hsotg->irq)
>
> That solution is fine with me.  Disabling the dwc2 interrupt sources
> in initialization before the handlers are registered is also worth
> doing, but it doesn't have to be in the same patch.

Sounds fine to me.

-Doug

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

[Frank Mori Hess]

Hi Minas,

On Fri, May 29, 2020 at 3:50 PM Minas Harutyunyan
<Minas.Harutyunyan@synopsys.com> wrote:
>
> Can you test it on your setup and confirm (to keep "Tested-by: Frank.."
> tag).
>

I just tested the

dwc2_disable_global_interrupts(hsotg);
synchronize_irq(hsotg->irq);

version of dwc2 shutdown, and booting a new kernel with kexec worked
fine for me.

-- 
Frank