Re: usb: dwc2: Disable all EP's on disconnect

[Geert Uytterhoeven]

Hi Minas,

On Fri, Oct 26, 2018 at 6:59 PM Linux Kernel Mailing List
<linux-kernel@xxxxxxxxxxxxxxx> wrote:
> Commit:     dccf1bad4be7eaa096c1f3697bd37883f9a08ecb
> Parent:     4c19cc14064d99ef0a20fb5ba0d45c94dbedb13c
> Refname:    refs/heads/master
> Web:        https://git.kernel.org/torvalds/c/dccf1bad4be7eaa096c1f3697bd37883f9a08ecb
> Author:     Minas Harutyunyan <Minas.Harutyunyan@xxxxxxxxxxxx>
> AuthorDate: Wed Sep 19 18:13:52 2018 +0400
> Committer:  Felipe Balbi <felipe.balbi@xxxxxxxxxxxxxxx>
> CommitDate: Tue Oct 2 10:33:15 2018 +0300
>
>     usb: dwc2: Disable all EP's on disconnect
>
>     Disabling all EP's allow to reset EP's to initial state.
>     On disconnect disable all EP's instead of just killing
>     all requests. Because of some platform didn't catch
>     disconnect event, same stuff added to
>     dwc2_hsotg_core_init_disconnected() function when USB
>     reset detected on the bus.
>
>     Changed from version 1:
>     Changed lock acquire flow in dwc2_hsotg_ep_disable()
>     function.

Dan had some comments on v1, and requested to document why the locking
games you're playing are safe, cfr.
https://lists.01.org/pipermail/kbuild/2018-September/002945.html

Unfortunately you didn't CC him for v2...

> --- a/drivers/usb/dwc2/gadget.c
> +++ b/drivers/usb/dwc2/gadget.c

> @@ -3993,6 +4004,7 @@ static int dwc2_hsotg_ep_disable(struct usb_ep *ep)
>         unsigned long flags;
>         u32 epctrl_reg;
>         u32 ctrl;
> +       int locked;
>
>         dev_dbg(hsotg->dev, "%s(ep %p)\n", __func__, ep);
>
> @@ -4008,7 +4020,9 @@ static int dwc2_hsotg_ep_disable(struct usb_ep *ep)
>
>         epctrl_reg = dir_in ? DIEPCTL(index) : DOEPCTL(index);
>
> -       spin_lock_irqsave(&hsotg->lock, flags);
> +       locked = spin_is_locked(&hsotg->lock);
> +       if (!locked)
> +               spin_lock_irqsave(&hsotg->lock, flags);

This looks really fishy to me.
What if another CPU takes the lock in between?

Can you please explain, and fix if necessary?

Thanks!

>
>         ctrl = dwc2_readl(hsotg, epctrl_reg);
>
> @@ -4032,7 +4046,9 @@ static int dwc2_hsotg_ep_disable(struct usb_ep *ep)
>         hs_ep->fifo_index = 0;
>         hs_ep->fifo_size = 0;
>
> -       spin_unlock_irqrestore(&hsotg->lock, flags);
> +       if (!locked)
> +               spin_unlock_irqrestore(&hsotg->lock, flags);
> +
>         return 0;
>  }

Gr{oetje,eeting}s,

                        Geert

-- 
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@xxxxxxxxxxxxxx

In personal conversations with technical people, I call myself a hacker. But
when I'm talking to journalists I just say "programmer" or something like that.
                                -- Linus Torvalds