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

[Doug Anderson <dianders@chromium.org>]

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