* Re: usb: dwc2: Disable all EP's on disconnect
[not found] <git-mailbomb-linux-master-dccf1bad4be7eaa096c1f3697bd37883f9a08ecb@kernel.org>
@ 2018-10-28 16:23 ` Geert Uytterhoeven
0 siblings, 0 replies; only message in thread
From: Geert Uytterhoeven @ 2018-10-28 16:23 UTC (permalink / raw)
To: Minas.Harutyunyan
Cc: Felipe Balbi, Dan Carpenter, USB list, Linux Kernel Mailing List
Hi Minas,
On Fri, Oct 26, 2018 at 6:59 PM Linux Kernel Mailing List
<linux-kernel@vger.kernel.org> wrote:
> Commit: dccf1bad4be7eaa096c1f3697bd37883f9a08ecb
> Parent: 4c19cc14064d99ef0a20fb5ba0d45c94dbedb13c
> Refname: refs/heads/master
> Web: https://git.kernel.org/torvalds/c/dccf1bad4be7eaa096c1f3697bd37883f9a08ecb
> Author: Minas Harutyunyan <Minas.Harutyunyan@synopsys.com>
> AuthorDate: Wed Sep 19 18:13:52 2018 +0400
> Committer: Felipe Balbi <felipe.balbi@linux.intel.com>
> 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@linux-m68k.org
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
^ permalink raw reply [flat|nested] only message in thread
only message in thread, other threads:[~2018-10-28 16:23 UTC | newest]
Thread overview: (only message) (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
[not found] <git-mailbomb-linux-master-dccf1bad4be7eaa096c1f3697bd37883f9a08ecb@kernel.org>
2018-10-28 16:23 ` usb: dwc2: Disable all EP's on disconnect Geert Uytterhoeven
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).