* Re: [PATCH] usb: dwc2: Revert "usb: dwc2: Disable all EP's on disconnect"
2018-12-04 12:34 ` Minas Harutyunyan
@ 2018-12-04 13:29 ` Dan Carpenter
2018-12-05 12:52 ` Minas Harutyunyan
2018-12-06 15:03 ` Marek Szyprowski
2018-12-06 23:09 ` Maynard CABIENTE
2 siblings, 1 reply; 20+ messages in thread
From: Dan Carpenter @ 2018-12-04 13:29 UTC (permalink / raw)
To: Minas Harutyunyan
Cc: Marek Szyprowski, Maynard CABIENTE, linux-kernel, linux-usb,
Greg Kroah-Hartman, Felipe Balbi, Geert Uytterhoeven,
Bartlomiej Zolnierkiewicz
On Tue, Dec 04, 2018 at 12:34:08PM +0000, Minas Harutyunyan wrote:
> @@ -3185,12 +3183,13 @@ void dwc2_hsotg_disconnect(struct dwc2_hsotg *hsotg)
> hsotg->connected = 0;
> hsotg->test_mode = 0;
>
> - /* all endpoints should be shutdown */
> for (ep = 0; ep < hsotg->num_of_eps; ep++) {
> if (hsotg->eps_in[ep])
> - dwc2_hsotg_ep_disable(&hsotg->eps_in[ep]->ep);
> + kill_all_requests(hsotg, hsotg->eps_in[ep],
> + -ESHUTDOWN);
> if (hsotg->eps_out[ep])
> - dwc2_hsotg_ep_disable(&hsotg->eps_out[ep]->ep);
> + kill_all_requests(hsotg, hsotg->eps_out[ep],
> + -ESHUTDOWN);
Should this part be in a separate patch?
I'm not trying to be rhetorical at all. I literally don't know the
code very well. Hopefully the full commit message will explain it.
> }
>
> call_gadget(hsotg, disconnect);
> @@ -3234,6 +3233,8 @@ static void dwc2_hsotg_irq_fifoempty(struct
> dwc2_hsotg *hsotg, bool periodic)
> GINTSTS_PTXFEMP | \
> GINTSTS_RXFLVL)
>
> +static int dwc2_hsotg_ep_disable(struct usb_ep *ep);
> +
> /**
> * dwc2_hsotg_core_init - issue softreset to the core
> * @hsotg: The device state
> @@ -3258,12 +3259,14 @@ void dwc2_hsotg_core_init_disconnected(struct
> dwc2_hsotg *hsotg,
> return;
> } else {
> /* all endpoints should be shutdown */
> + spin_unlock(&hsotg->lock);
> for (ep = 1; ep < hsotg->num_of_eps; ep++) {
> if (hsotg->eps_in[ep])
>
> dwc2_hsotg_ep_disable(&hsotg->eps_in[ep]->ep);
> if (hsotg->eps_out[ep])
>
> dwc2_hsotg_ep_disable(&hsotg->eps_out[ep]->ep);
> }
> + spin_lock(&hsotg->lock);
> }
>
> /*
The idea here is that this is the only caller which is holding the
lock and we drop it here and take it again inside dwc2_hsotg_ep_disable().
I don't know the code very well and can't totally swear that this
doesn't introduce a small race condition...
Another option would be to introduce a new function which takes the lock
and change all the other callers instead. To me that would be easier to
review... See below for how it might look:
regards,
dan carpenter
diff --git a/drivers/usb/dwc2/gadget.c b/drivers/usb/dwc2/gadget.c
index 94f3ba995580..b17a5dbefd5f 100644
--- a/drivers/usb/dwc2/gadget.c
+++ b/drivers/usb/dwc2/gadget.c
@@ -3166,6 +3166,7 @@ static void kill_all_requests(struct dwc2_hsotg *hsotg,
}
static int dwc2_hsotg_ep_disable(struct usb_ep *ep);
+static int dwc2_hsotg_ep_disable_lock(struct usb_ep *ep);
/**
* dwc2_hsotg_disconnect - disconnect service
@@ -3188,9 +3189,9 @@ void dwc2_hsotg_disconnect(struct dwc2_hsotg *hsotg)
/* all endpoints should be shutdown */
for (ep = 0; ep < hsotg->num_of_eps; ep++) {
if (hsotg->eps_in[ep])
- dwc2_hsotg_ep_disable(&hsotg->eps_in[ep]->ep);
+ dwc2_hsotg_ep_disable_lock(&hsotg->eps_in[ep]->ep);
if (hsotg->eps_out[ep])
- dwc2_hsotg_ep_disable(&hsotg->eps_out[ep]->ep);
+ dwc2_hsotg_ep_disable_lock(&hsotg->eps_out[ep]->ep);
}
call_gadget(hsotg, disconnect);
@@ -4069,10 +4070,8 @@ static int dwc2_hsotg_ep_disable(struct usb_ep *ep)
struct dwc2_hsotg *hsotg = hs_ep->parent;
int dir_in = hs_ep->dir_in;
int index = hs_ep->index;
- unsigned long flags;
u32 epctrl_reg;
u32 ctrl;
- int locked;
dev_dbg(hsotg->dev, "%s(ep %p)\n", __func__, ep);
@@ -4088,10 +4087,6 @@ static int dwc2_hsotg_ep_disable(struct usb_ep *ep)
epctrl_reg = dir_in ? DIEPCTL(index) : DOEPCTL(index);
- locked = spin_is_locked(&hsotg->lock);
- if (!locked)
- spin_lock_irqsave(&hsotg->lock, flags);
-
ctrl = dwc2_readl(hsotg, epctrl_reg);
if (ctrl & DXEPCTL_EPENA)
@@ -4114,12 +4109,23 @@ static int dwc2_hsotg_ep_disable(struct usb_ep *ep)
hs_ep->fifo_index = 0;
hs_ep->fifo_size = 0;
- if (!locked)
- spin_unlock_irqrestore(&hsotg->lock, flags);
-
return 0;
}
+static int dwc2_hsotg_ep_disable_lock(struct usb_ep *ep)
+{
+ struct dwc2_hsotg_ep *hs_ep = our_ep(ep);
+ struct dwc2_hsotg *hsotg = hs_ep->parent;
+ unsigned long flags;
+ int ret;
+
+ spin_lock_irqsave(&hsotg->lock, flags);
+ ret = dwc2_hsotg_ep_disable(ep);
+ spin_unlock_irqrestore(&hsotg->lock, flags);
+
+ return ret;
+}
+
/**
* on_list - check request is on the given endpoint
* @ep: The endpoint to check.
@@ -4267,7 +4273,7 @@ static int dwc2_hsotg_ep_sethalt_lock(struct usb_ep *ep, int value)
static const struct usb_ep_ops dwc2_hsotg_ep_ops = {
.enable = dwc2_hsotg_ep_enable,
- .disable = dwc2_hsotg_ep_disable,
+ .disable = dwc2_hsotg_ep_disable_lock,
.alloc_request = dwc2_hsotg_ep_alloc_request,
.free_request = dwc2_hsotg_ep_free_request,
.queue = dwc2_hsotg_ep_queue_lock,
@@ -4407,9 +4413,9 @@ static int dwc2_hsotg_udc_stop(struct usb_gadget *gadget)
/* all endpoints should be shutdown */
for (ep = 1; ep < hsotg->num_of_eps; ep++) {
if (hsotg->eps_in[ep])
- dwc2_hsotg_ep_disable(&hsotg->eps_in[ep]->ep);
+ dwc2_hsotg_ep_disable_lock(&hsotg->eps_in[ep]->ep);
if (hsotg->eps_out[ep])
- dwc2_hsotg_ep_disable(&hsotg->eps_out[ep]->ep);
+ dwc2_hsotg_ep_disable_lock(&hsotg->eps_out[ep]->ep);
}
spin_lock_irqsave(&hsotg->lock, flags);
@@ -4857,9 +4863,9 @@ int dwc2_hsotg_suspend(struct dwc2_hsotg *hsotg)
for (ep = 0; ep < hsotg->num_of_eps; ep++) {
if (hsotg->eps_in[ep])
- dwc2_hsotg_ep_disable(&hsotg->eps_in[ep]->ep);
+ dwc2_hsotg_ep_disable_lock(&hsotg->eps_in[ep]->ep);
if (hsotg->eps_out[ep])
- dwc2_hsotg_ep_disable(&hsotg->eps_out[ep]->ep);
+ dwc2_hsotg_ep_disable_lock(&hsotg->eps_out[ep]->ep);
}
}
^ permalink raw reply related [flat|nested] 20+ messages in thread* Re: [PATCH] usb: dwc2: Revert "usb: dwc2: Disable all EP's on disconnect"
2018-12-04 13:29 ` Dan Carpenter
@ 2018-12-05 12:52 ` Minas Harutyunyan
2018-12-07 10:15 ` Dan Carpenter
0 siblings, 1 reply; 20+ messages in thread
From: Minas Harutyunyan @ 2018-12-05 12:52 UTC (permalink / raw)
To: Dan Carpenter, Minas Harutyunyan
Cc: Marek Szyprowski, Maynard CABIENTE, linux-kernel, linux-usb,
Greg Kroah-Hartman, Felipe Balbi, Geert Uytterhoeven,
Bartlomiej Zolnierkiewicz
Hi,
On 12/4/2018 5:29 PM, Dan Carpenter wrote:
> On Tue, Dec 04, 2018 at 12:34:08PM +0000, Minas Harutyunyan wrote:
>> @@ -3185,12 +3183,13 @@ void dwc2_hsotg_disconnect(struct dwc2_hsotg *hsotg)
>> hsotg->connected = 0;
>> hsotg->test_mode = 0;
>>
>> - /* all endpoints should be shutdown */
>> for (ep = 0; ep < hsotg->num_of_eps; ep++) {
>> if (hsotg->eps_in[ep])
>> - dwc2_hsotg_ep_disable(&hsotg->eps_in[ep]->ep);
>> + kill_all_requests(hsotg, hsotg->eps_in[ep],
>> + -ESHUTDOWN);
>> if (hsotg->eps_out[ep])
>> - dwc2_hsotg_ep_disable(&hsotg->eps_out[ep]->ep);
>> + kill_all_requests(hsotg, hsotg->eps_out[ep],
>> + -ESHUTDOWN);
>
>
> Should this part be in a separate patch?
>
> I'm not trying to be rhetorical at all. I literally don't know the
> code very well. Hopefully the full commit message will explain it.
>
Actually, this fragment of patch revert changes from V2 and keep
untouched dwc2_hsotg_disconnect() function.
>> }
>>
>> call_gadget(hsotg, disconnect);
>> @@ -3234,6 +3233,8 @@ static void dwc2_hsotg_irq_fifoempty(struct
>> dwc2_hsotg *hsotg, bool periodic)
>> GINTSTS_PTXFEMP | \
>> GINTSTS_RXFLVL)
>>
>> +static int dwc2_hsotg_ep_disable(struct usb_ep *ep);
>> +
>> /**
>> * dwc2_hsotg_core_init - issue softreset to the core
>> * @hsotg: The device state
>> @@ -3258,12 +3259,14 @@ void dwc2_hsotg_core_init_disconnected(struct
>> dwc2_hsotg *hsotg,
>> return;
>> } else {
>> /* all endpoints should be shutdown */
>> + spin_unlock(&hsotg->lock);
>> for (ep = 1; ep < hsotg->num_of_eps; ep++) {
>> if (hsotg->eps_in[ep])
>>
>> dwc2_hsotg_ep_disable(&hsotg->eps_in[ep]->ep);
>> if (hsotg->eps_out[ep])
>>
>> dwc2_hsotg_ep_disable(&hsotg->eps_out[ep]->ep);
>> }
>> + spin_lock(&hsotg->lock);
>> }
>>
>> /*
>
> The idea here is that this is the only caller which is holding the
> lock and we drop it here and take it again inside dwc2_hsotg_ep_disable().
> I don't know the code very well and can't totally swear that this
> doesn't introduce a small race condition...
>
Above fragment of patch allow to keep untouched dwc2_hsotg_ep_disble()
function also, without changing spin_lock/_unlock stuff inside function.
My approach here minimally update code to add any races. Just in
dwc2_hsotg_core_init_disconnected() function on USB reset interrupt
perform disabling all EP's. Because on USB reset interrupt, called from interrupt
handler with acquired lock and dwc2_hsotg_ep_disble() function (without
changes) acquire lock, just need to unlock lock to avoid any troubles.
> Another option would be to introduce a new function which takes the lock
> and change all the other callers instead. To me that would be easier to
> review... See below for how it might look:
>
> regards,
> dan carpenter
>
> diff --git a/drivers/usb/dwc2/gadget.c b/drivers/usb/dwc2/gadget.c
> index 94f3ba995580..b17a5dbefd5f 100644
> --- a/drivers/usb/dwc2/gadget.c
> +++ b/drivers/usb/dwc2/gadget.c
> @@ -3166,6 +3166,7 @@ static void kill_all_requests(struct dwc2_hsotg *hsotg,
> }
>
> static int dwc2_hsotg_ep_disable(struct usb_ep *ep);
> +static int dwc2_hsotg_ep_disable_lock(struct usb_ep *ep);
>
> /**
> * dwc2_hsotg_disconnect - disconnect service
> @@ -3188,9 +3189,9 @@ void dwc2_hsotg_disconnect(struct dwc2_hsotg *hsotg)
> /* all endpoints should be shutdown */
> for (ep = 0; ep < hsotg->num_of_eps; ep++) {
> if (hsotg->eps_in[ep])
> - dwc2_hsotg_ep_disable(&hsotg->eps_in[ep]->ep);
> + dwc2_hsotg_ep_disable_lock(&hsotg->eps_in[ep]->ep);
> if (hsotg->eps_out[ep])
> - dwc2_hsotg_ep_disable(&hsotg->eps_out[ep]->ep);
> + dwc2_hsotg_ep_disable_lock(&hsotg->eps_out[ep]->ep);
> }
>
> call_gadget(hsotg, disconnect);
> @@ -4069,10 +4070,8 @@ static int dwc2_hsotg_ep_disable(struct usb_ep *ep)
> struct dwc2_hsotg *hsotg = hs_ep->parent;
> int dir_in = hs_ep->dir_in;
> int index = hs_ep->index;
> - unsigned long flags;
> u32 epctrl_reg;
> u32 ctrl;
> - int locked;
>
> dev_dbg(hsotg->dev, "%s(ep %p)\n", __func__, ep);
>
> @@ -4088,10 +4087,6 @@ static int dwc2_hsotg_ep_disable(struct usb_ep *ep)
>
> epctrl_reg = dir_in ? DIEPCTL(index) : DOEPCTL(index);
>
> - locked = spin_is_locked(&hsotg->lock);
> - if (!locked)
> - spin_lock_irqsave(&hsotg->lock, flags);
> -
> ctrl = dwc2_readl(hsotg, epctrl_reg);
>
> if (ctrl & DXEPCTL_EPENA)
> @@ -4114,12 +4109,23 @@ static int dwc2_hsotg_ep_disable(struct usb_ep *ep)
> hs_ep->fifo_index = 0;
> hs_ep->fifo_size = 0;
>
> - if (!locked)
> - spin_unlock_irqrestore(&hsotg->lock, flags);
> -
> return 0;
> }
>
> +static int dwc2_hsotg_ep_disable_lock(struct usb_ep *ep)
> +{
> + struct dwc2_hsotg_ep *hs_ep = our_ep(ep);
> + struct dwc2_hsotg *hsotg = hs_ep->parent;
> + unsigned long flags;
> + int ret;
> +
> + spin_lock_irqsave(&hsotg->lock, flags);
> + ret = dwc2_hsotg_ep_disable(ep);
> + spin_unlock_irqrestore(&hsotg->lock, flags);
> +
> + return ret;
> +}
> +
> /**
> * on_list - check request is on the given endpoint
> * @ep: The endpoint to check.
> @@ -4267,7 +4273,7 @@ static int dwc2_hsotg_ep_sethalt_lock(struct usb_ep *ep, int value)
>
> static const struct usb_ep_ops dwc2_hsotg_ep_ops = {
> .enable = dwc2_hsotg_ep_enable,
> - .disable = dwc2_hsotg_ep_disable,
> + .disable = dwc2_hsotg_ep_disable_lock,
> .alloc_request = dwc2_hsotg_ep_alloc_request,
> .free_request = dwc2_hsotg_ep_free_request,
> .queue = dwc2_hsotg_ep_queue_lock,
> @@ -4407,9 +4413,9 @@ static int dwc2_hsotg_udc_stop(struct usb_gadget *gadget)
> /* all endpoints should be shutdown */
> for (ep = 1; ep < hsotg->num_of_eps; ep++) {
> if (hsotg->eps_in[ep])
> - dwc2_hsotg_ep_disable(&hsotg->eps_in[ep]->ep);
> + dwc2_hsotg_ep_disable_lock(&hsotg->eps_in[ep]->ep);
> if (hsotg->eps_out[ep])
> - dwc2_hsotg_ep_disable(&hsotg->eps_out[ep]->ep);
> + dwc2_hsotg_ep_disable_lock(&hsotg->eps_out[ep]->ep);
> }
>
> spin_lock_irqsave(&hsotg->lock, flags);
> @@ -4857,9 +4863,9 @@ int dwc2_hsotg_suspend(struct dwc2_hsotg *hsotg)
>
> for (ep = 0; ep < hsotg->num_of_eps; ep++) {
> if (hsotg->eps_in[ep])
> - dwc2_hsotg_ep_disable(&hsotg->eps_in[ep]->ep);
> + dwc2_hsotg_ep_disable_lock(&hsotg->eps_in[ep]->ep);
> if (hsotg->eps_out[ep])
> - dwc2_hsotg_ep_disable(&hsotg->eps_out[ep]->ep);
> + dwc2_hsotg_ep_disable_lock(&hsotg->eps_out[ep]->ep);
> }
> }
>
>
Your code doesn't take care about fifo_map warnings from
dwc2_hsotg_init_fifo() function. Before calling dwc2_hsotg_init_fifo()
from dwc2_hsotg_core_init_disconnected() function all Ep's should
disabled and fifo bitmap should be cleared.
Thanks,
Minas
^ permalink raw reply [flat|nested] 20+ messages in thread* Re: [PATCH] usb: dwc2: Revert "usb: dwc2: Disable all EP's on disconnect"
2018-12-05 12:52 ` Minas Harutyunyan
@ 2018-12-07 10:15 ` Dan Carpenter
2018-12-07 11:20 ` Minas Harutyunyan
0 siblings, 1 reply; 20+ messages in thread
From: Dan Carpenter @ 2018-12-07 10:15 UTC (permalink / raw)
To: Minas Harutyunyan
Cc: Marek Szyprowski, Maynard CABIENTE, linux-kernel, linux-usb,
Greg Kroah-Hartman, Felipe Balbi, Geert Uytterhoeven,
Bartlomiej Zolnierkiewicz
On Wed, Dec 05, 2018 at 12:52:22PM +0000, Minas Harutyunyan wrote:
> Hi,
>
> On 12/4/2018 5:29 PM, Dan Carpenter wrote:
> > On Tue, Dec 04, 2018 at 12:34:08PM +0000, Minas Harutyunyan wrote:
> >> @@ -3185,12 +3183,13 @@ void dwc2_hsotg_disconnect(struct dwc2_hsotg *hsotg)
> >> hsotg->connected = 0;
> >> hsotg->test_mode = 0;
> >>
> >> - /* all endpoints should be shutdown */
> >> for (ep = 0; ep < hsotg->num_of_eps; ep++) {
> >> if (hsotg->eps_in[ep])
> >> - dwc2_hsotg_ep_disable(&hsotg->eps_in[ep]->ep);
> >> + kill_all_requests(hsotg, hsotg->eps_in[ep],
> >> + -ESHUTDOWN);
> >> if (hsotg->eps_out[ep])
> >> - dwc2_hsotg_ep_disable(&hsotg->eps_out[ep]->ep);
> >> + kill_all_requests(hsotg, hsotg->eps_out[ep],
> >> + -ESHUTDOWN);
> >
> >
> > Should this part be in a separate patch?
> >
> > I'm not trying to be rhetorical at all. I literally don't know the
> > code very well. Hopefully the full commit message will explain it.
> >
>
> Actually, this fragment of patch revert changes from V2 and keep
> untouched dwc2_hsotg_disconnect() function.
>
To me it feels like there are two issues. The first is this change, and
the second is fixing the lockdep warning.
> >> }
> >>
> >> call_gadget(hsotg, disconnect);
> >> @@ -3234,6 +3233,8 @@ static void dwc2_hsotg_irq_fifoempty(struct
> >> dwc2_hsotg *hsotg, bool periodic)
> >> GINTSTS_PTXFEMP | \
> >> GINTSTS_RXFLVL)
> >>
> >> +static int dwc2_hsotg_ep_disable(struct usb_ep *ep);
> >> +
> >> /**
> >> * dwc2_hsotg_core_init - issue softreset to the core
> >> * @hsotg: The device state
> >> @@ -3258,12 +3259,14 @@ void dwc2_hsotg_core_init_disconnected(struct
> >> dwc2_hsotg *hsotg,
> >> return;
> >> } else {
> >> /* all endpoints should be shutdown */
> >> + spin_unlock(&hsotg->lock);
> >> for (ep = 1; ep < hsotg->num_of_eps; ep++) {
> >> if (hsotg->eps_in[ep])
> >>
> >> dwc2_hsotg_ep_disable(&hsotg->eps_in[ep]->ep);
> >> if (hsotg->eps_out[ep])
> >>
> >> dwc2_hsotg_ep_disable(&hsotg->eps_out[ep]->ep);
> >> }
> >> + spin_lock(&hsotg->lock);
> >> }
> >>
> >> /*
> >
> > The idea here is that this is the only caller which is holding the
> > lock and we drop it here and take it again inside dwc2_hsotg_ep_disable().
> > I don't know the code very well and can't totally swear that this
> > doesn't introduce a small race condition...
> >
> Above fragment of patch allow to keep untouched dwc2_hsotg_ep_disble()
> function also, without changing spin_lock/_unlock stuff inside function.
>
> My approach here minimally update code to add any races. Just in
> dwc2_hsotg_core_init_disconnected() function on USB reset interrupt
> perform disabling all EP's. Because on USB reset interrupt, called from interrupt
> handler with acquired lock and dwc2_hsotg_ep_disble() function (without
> changes) acquire lock, just need to unlock lock to avoid any troubles.
>
Yes. I understand that. I just don't like it.
Although your patch is more "minimal" in that it touches fewer lines of
code it's actually more complicated because we have to verify that it's
safe to drop the lock.
> > Another option would be to introduce a new function which takes the lock
> > and change all the other callers instead. To me that would be easier to
> > review... See below for how it might look:
> >
> > regards,
> > dan carpenter
> >
> > diff --git a/drivers/usb/dwc2/gadget.c b/drivers/usb/dwc2/gadget.c
> > index 94f3ba995580..b17a5dbefd5f 100644
> > --- a/drivers/usb/dwc2/gadget.c
> > +++ b/drivers/usb/dwc2/gadget.c
> > @@ -3166,6 +3166,7 @@ static void kill_all_requests(struct dwc2_hsotg *hsotg,
> > }
> >
> > static int dwc2_hsotg_ep_disable(struct usb_ep *ep);
> > +static int dwc2_hsotg_ep_disable_lock(struct usb_ep *ep);
> >
> > /**
> > * dwc2_hsotg_disconnect - disconnect service
> > @@ -3188,9 +3189,9 @@ void dwc2_hsotg_disconnect(struct dwc2_hsotg *hsotg)
> > /* all endpoints should be shutdown */
> > for (ep = 0; ep < hsotg->num_of_eps; ep++) {
> > if (hsotg->eps_in[ep])
> > - dwc2_hsotg_ep_disable(&hsotg->eps_in[ep]->ep);
> > + dwc2_hsotg_ep_disable_lock(&hsotg->eps_in[ep]->ep);
> > if (hsotg->eps_out[ep])
> > - dwc2_hsotg_ep_disable(&hsotg->eps_out[ep]->ep);
> > + dwc2_hsotg_ep_disable_lock(&hsotg->eps_out[ep]->ep);
> > }
> >
> > call_gadget(hsotg, disconnect);
> > @@ -4069,10 +4070,8 @@ static int dwc2_hsotg_ep_disable(struct usb_ep *ep)
> > struct dwc2_hsotg *hsotg = hs_ep->parent;
> > int dir_in = hs_ep->dir_in;
> > int index = hs_ep->index;
> > - unsigned long flags;
> > u32 epctrl_reg;
> > u32 ctrl;
> > - int locked;
> >
> > dev_dbg(hsotg->dev, "%s(ep %p)\n", __func__, ep);
> >
> > @@ -4088,10 +4087,6 @@ static int dwc2_hsotg_ep_disable(struct usb_ep *ep)
> >
> > epctrl_reg = dir_in ? DIEPCTL(index) : DOEPCTL(index);
> >
> > - locked = spin_is_locked(&hsotg->lock);
> > - if (!locked)
> > - spin_lock_irqsave(&hsotg->lock, flags);
> > -
> > ctrl = dwc2_readl(hsotg, epctrl_reg);
> >
> > if (ctrl & DXEPCTL_EPENA)
> > @@ -4114,12 +4109,23 @@ static int dwc2_hsotg_ep_disable(struct usb_ep *ep)
> > hs_ep->fifo_index = 0;
> > hs_ep->fifo_size = 0;
> >
> > - if (!locked)
> > - spin_unlock_irqrestore(&hsotg->lock, flags);
> > -
> > return 0;
> > }
> >
> > +static int dwc2_hsotg_ep_disable_lock(struct usb_ep *ep)
> > +{
> > + struct dwc2_hsotg_ep *hs_ep = our_ep(ep);
> > + struct dwc2_hsotg *hsotg = hs_ep->parent;
> > + unsigned long flags;
> > + int ret;
> > +
> > + spin_lock_irqsave(&hsotg->lock, flags);
> > + ret = dwc2_hsotg_ep_disable(ep);
> > + spin_unlock_irqrestore(&hsotg->lock, flags);
> > +
> > + return ret;
> > +}
> > +
> > /**
> > * on_list - check request is on the given endpoint
> > * @ep: The endpoint to check.
> > @@ -4267,7 +4273,7 @@ static int dwc2_hsotg_ep_sethalt_lock(struct usb_ep *ep, int value)
> >
> > static const struct usb_ep_ops dwc2_hsotg_ep_ops = {
> > .enable = dwc2_hsotg_ep_enable,
> > - .disable = dwc2_hsotg_ep_disable,
> > + .disable = dwc2_hsotg_ep_disable_lock,
> > .alloc_request = dwc2_hsotg_ep_alloc_request,
> > .free_request = dwc2_hsotg_ep_free_request,
> > .queue = dwc2_hsotg_ep_queue_lock,
> > @@ -4407,9 +4413,9 @@ static int dwc2_hsotg_udc_stop(struct usb_gadget *gadget)
> > /* all endpoints should be shutdown */
> > for (ep = 1; ep < hsotg->num_of_eps; ep++) {
> > if (hsotg->eps_in[ep])
> > - dwc2_hsotg_ep_disable(&hsotg->eps_in[ep]->ep);
> > + dwc2_hsotg_ep_disable_lock(&hsotg->eps_in[ep]->ep);
> > if (hsotg->eps_out[ep])
> > - dwc2_hsotg_ep_disable(&hsotg->eps_out[ep]->ep);
> > + dwc2_hsotg_ep_disable_lock(&hsotg->eps_out[ep]->ep);
> > }
> >
> > spin_lock_irqsave(&hsotg->lock, flags);
> > @@ -4857,9 +4863,9 @@ int dwc2_hsotg_suspend(struct dwc2_hsotg *hsotg)
> >
> > for (ep = 0; ep < hsotg->num_of_eps; ep++) {
> > if (hsotg->eps_in[ep])
> > - dwc2_hsotg_ep_disable(&hsotg->eps_in[ep]->ep);
> > + dwc2_hsotg_ep_disable_lock(&hsotg->eps_in[ep]->ep);
> > if (hsotg->eps_out[ep])
> > - dwc2_hsotg_ep_disable(&hsotg->eps_out[ep]->ep);
> > + dwc2_hsotg_ep_disable_lock(&hsotg->eps_out[ep]->ep);
> > }
> > }
> >
> >
>
> Your code doesn't take care about fifo_map warnings from
> dwc2_hsotg_init_fifo() function. Before calling dwc2_hsotg_init_fifo()
> from dwc2_hsotg_core_init_disconnected() function all Ep's should
> disabled and fifo bitmap should be cleared.
>
Correct. I am only trying to fix the locking. I hope you can fix the
rest in a separate patch.
regards,
dan carpenter
^ permalink raw reply [flat|nested] 20+ messages in thread* Re: [PATCH] usb: dwc2: Revert "usb: dwc2: Disable all EP's on disconnect"
2018-12-07 10:15 ` Dan Carpenter
@ 2018-12-07 11:20 ` Minas Harutyunyan
2018-12-07 14:13 ` Minas Harutyunyan
0 siblings, 1 reply; 20+ messages in thread
From: Minas Harutyunyan @ 2018-12-07 11:20 UTC (permalink / raw)
To: Dan Carpenter, Minas Harutyunyan
Cc: Marek Szyprowski, Maynard CABIENTE, linux-kernel, linux-usb,
Greg Kroah-Hartman, Felipe Balbi, Geert Uytterhoeven,
Bartlomiej Zolnierkiewicz
Hi Dan,
On 12/7/2018 2:16 PM, Dan Carpenter wrote:
> On Wed, Dec 05, 2018 at 12:52:22PM +0000, Minas Harutyunyan wrote:
>> Hi,
>>
>> On 12/4/2018 5:29 PM, Dan Carpenter wrote:
>>> On Tue, Dec 04, 2018 at 12:34:08PM +0000, Minas Harutyunyan wrote:
>>>> @@ -3185,12 +3183,13 @@ void dwc2_hsotg_disconnect(struct dwc2_hsotg *hsotg)
>>>> hsotg->connected = 0;
>>>> hsotg->test_mode = 0;
>>>>
>>>> - /* all endpoints should be shutdown */
>>>> for (ep = 0; ep < hsotg->num_of_eps; ep++) {
>>>> if (hsotg->eps_in[ep])
>>>> - dwc2_hsotg_ep_disable(&hsotg->eps_in[ep]->ep);
>>>> + kill_all_requests(hsotg, hsotg->eps_in[ep],
>>>> + -ESHUTDOWN);
>>>> if (hsotg->eps_out[ep])
>>>> - dwc2_hsotg_ep_disable(&hsotg->eps_out[ep]->ep);
>>>> + kill_all_requests(hsotg, hsotg->eps_out[ep],
>>>> + -ESHUTDOWN);
>>>
>>>
>>> Should this part be in a separate patch?
>>>
>>> I'm not trying to be rhetorical at all. I literally don't know the
>>> code very well. Hopefully the full commit message will explain it.
>>>
>>
>> Actually, this fragment of patch revert changes from V2 and keep
>> untouched dwc2_hsotg_disconnect() function.
>>
>
> To me it feels like there are two issues. The first is this change, and
> the second is fixing the lockdep warning.
>
>
>>>> }
>>>>
>>>> call_gadget(hsotg, disconnect);
>>>> @@ -3234,6 +3233,8 @@ static void dwc2_hsotg_irq_fifoempty(struct
>>>> dwc2_hsotg *hsotg, bool periodic)
>>>> GINTSTS_PTXFEMP | \
>>>> GINTSTS_RXFLVL)
>>>>
>>>> +static int dwc2_hsotg_ep_disable(struct usb_ep *ep);
>>>> +
>>>> /**
>>>> * dwc2_hsotg_core_init - issue softreset to the core
>>>> * @hsotg: The device state
>>>> @@ -3258,12 +3259,14 @@ void dwc2_hsotg_core_init_disconnected(struct
>>>> dwc2_hsotg *hsotg,
>>>> return;
>>>> } else {
>>>> /* all endpoints should be shutdown */
>>>> + spin_unlock(&hsotg->lock);
>>>> for (ep = 1; ep < hsotg->num_of_eps; ep++) {
>>>> if (hsotg->eps_in[ep])
>>>>
>>>> dwc2_hsotg_ep_disable(&hsotg->eps_in[ep]->ep);
>>>> if (hsotg->eps_out[ep])
>>>>
>>>> dwc2_hsotg_ep_disable(&hsotg->eps_out[ep]->ep);
>>>> }
>>>> + spin_lock(&hsotg->lock);
>>>> }
>>>>
>>>> /*
>>>
>>> The idea here is that this is the only caller which is holding the
>>> lock and we drop it here and take it again inside dwc2_hsotg_ep_disable().
>>> I don't know the code very well and can't totally swear that this
>>> doesn't introduce a small race condition...
>>>
>> Above fragment of patch allow to keep untouched dwc2_hsotg_ep_disble()
>> function also, without changing spin_lock/_unlock stuff inside function.
>>
>> My approach here minimally update code to add any races. Just in
>> dwc2_hsotg_core_init_disconnected() function on USB reset interrupt
>> perform disabling all EP's. Because on USB reset interrupt, called from interrupt
>> handler with acquired lock and dwc2_hsotg_ep_disble() function (without
>> changes) acquire lock, just need to unlock lock to avoid any troubles.
>>
>
> Yes. I understand that. I just don't like it.
>
> Although your patch is more "minimal" in that it touches fewer lines of
> code it's actually more complicated because we have to verify that it's
> safe to drop the lock.
>
>
>>> Another option would be to introduce a new function which takes the lock
>>> and change all the other callers instead. To me that would be easier to
>>> review... See below for how it might look:
>>>
>>> regards,
>>> dan carpenter
>>>
>>> diff --git a/drivers/usb/dwc2/gadget.c b/drivers/usb/dwc2/gadget.c
>>> index 94f3ba995580..b17a5dbefd5f 100644
>>> --- a/drivers/usb/dwc2/gadget.c
>>> +++ b/drivers/usb/dwc2/gadget.c
>>> @@ -3166,6 +3166,7 @@ static void kill_all_requests(struct dwc2_hsotg *hsotg,
>>> }
>>>
>>> static int dwc2_hsotg_ep_disable(struct usb_ep *ep);
>>> +static int dwc2_hsotg_ep_disable_lock(struct usb_ep *ep);
>>>
>>> /**
>>> * dwc2_hsotg_disconnect - disconnect service
>>> @@ -3188,9 +3189,9 @@ void dwc2_hsotg_disconnect(struct dwc2_hsotg *hsotg)
>>> /* all endpoints should be shutdown */
>>> for (ep = 0; ep < hsotg->num_of_eps; ep++) {
>>> if (hsotg->eps_in[ep])
>>> - dwc2_hsotg_ep_disable(&hsotg->eps_in[ep]->ep);
>>> + dwc2_hsotg_ep_disable_lock(&hsotg->eps_in[ep]->ep);
>>> if (hsotg->eps_out[ep])
>>> - dwc2_hsotg_ep_disable(&hsotg->eps_out[ep]->ep);
>>> + dwc2_hsotg_ep_disable_lock(&hsotg->eps_out[ep]->ep);
>>> }
>>>
>>> call_gadget(hsotg, disconnect);
>>> @@ -4069,10 +4070,8 @@ static int dwc2_hsotg_ep_disable(struct usb_ep *ep)
>>> struct dwc2_hsotg *hsotg = hs_ep->parent;
>>> int dir_in = hs_ep->dir_in;
>>> int index = hs_ep->index;
>>> - unsigned long flags;
>>> u32 epctrl_reg;
>>> u32 ctrl;
>>> - int locked;
>>>
>>> dev_dbg(hsotg->dev, "%s(ep %p)\n", __func__, ep);
>>>
>>> @@ -4088,10 +4087,6 @@ static int dwc2_hsotg_ep_disable(struct usb_ep *ep)
>>>
>>> epctrl_reg = dir_in ? DIEPCTL(index) : DOEPCTL(index);
>>>
>>> - locked = spin_is_locked(&hsotg->lock);
>>> - if (!locked)
>>> - spin_lock_irqsave(&hsotg->lock, flags);
>>> -
>>> ctrl = dwc2_readl(hsotg, epctrl_reg);
>>>
>>> if (ctrl & DXEPCTL_EPENA)
>>> @@ -4114,12 +4109,23 @@ static int dwc2_hsotg_ep_disable(struct usb_ep *ep)
>>> hs_ep->fifo_index = 0;
>>> hs_ep->fifo_size = 0;
>>>
>>> - if (!locked)
>>> - spin_unlock_irqrestore(&hsotg->lock, flags);
>>> -
>>> return 0;
>>> }
>>>
>>> +static int dwc2_hsotg_ep_disable_lock(struct usb_ep *ep)
>>> +{
>>> + struct dwc2_hsotg_ep *hs_ep = our_ep(ep);
>>> + struct dwc2_hsotg *hsotg = hs_ep->parent;
>>> + unsigned long flags;
>>> + int ret;
>>> +
>>> + spin_lock_irqsave(&hsotg->lock, flags);
>>> + ret = dwc2_hsotg_ep_disable(ep);
>>> + spin_unlock_irqrestore(&hsotg->lock, flags);
>>> +
>>> + return ret;
>>> +}
>>> +
>>> /**
>>> * on_list - check request is on the given endpoint
>>> * @ep: The endpoint to check.
>>> @@ -4267,7 +4273,7 @@ static int dwc2_hsotg_ep_sethalt_lock(struct usb_ep *ep, int value)
>>>
>>> static const struct usb_ep_ops dwc2_hsotg_ep_ops = {
>>> .enable = dwc2_hsotg_ep_enable,
>>> - .disable = dwc2_hsotg_ep_disable,
>>> + .disable = dwc2_hsotg_ep_disable_lock,
>>> .alloc_request = dwc2_hsotg_ep_alloc_request,
>>> .free_request = dwc2_hsotg_ep_free_request,
>>> .queue = dwc2_hsotg_ep_queue_lock,
>>> @@ -4407,9 +4413,9 @@ static int dwc2_hsotg_udc_stop(struct usb_gadget *gadget)
>>> /* all endpoints should be shutdown */
>>> for (ep = 1; ep < hsotg->num_of_eps; ep++) {
>>> if (hsotg->eps_in[ep])
>>> - dwc2_hsotg_ep_disable(&hsotg->eps_in[ep]->ep);
>>> + dwc2_hsotg_ep_disable_lock(&hsotg->eps_in[ep]->ep);
>>> if (hsotg->eps_out[ep])
>>> - dwc2_hsotg_ep_disable(&hsotg->eps_out[ep]->ep);
>>> + dwc2_hsotg_ep_disable_lock(&hsotg->eps_out[ep]->ep);
>>> }
>>>
>>> spin_lock_irqsave(&hsotg->lock, flags);
>>> @@ -4857,9 +4863,9 @@ int dwc2_hsotg_suspend(struct dwc2_hsotg *hsotg)
>>>
>>> for (ep = 0; ep < hsotg->num_of_eps; ep++) {
>>> if (hsotg->eps_in[ep])
>>> - dwc2_hsotg_ep_disable(&hsotg->eps_in[ep]->ep);
>>> + dwc2_hsotg_ep_disable_lock(&hsotg->eps_in[ep]->ep);
>>> if (hsotg->eps_out[ep])
>>> - dwc2_hsotg_ep_disable(&hsotg->eps_out[ep]->ep);
>>> + dwc2_hsotg_ep_disable_lock(&hsotg->eps_out[ep]->ep);
>>> }
>>> }
>>>
>>>
>>
>> Your code doesn't take care about fifo_map warnings from
>> dwc2_hsotg_init_fifo() function. Before calling dwc2_hsotg_init_fifo()
>> from dwc2_hsotg_core_init_disconnected() function all Ep's should
>> disabled and fifo bitmap should be cleared.
>>
>
> Correct. I am only trying to fix the locking. I hope you can fix the
> rest in a separate patch.
>
Yeah. I'll try deeper investigate driver locking flow and fix it later.
Actually, I like your idea with introducing dwc2_hsotg_ep_disable_lock()
function. Maybe you yourself will submit new patch for safe locking
fixes? But please just after my patch will applied :-)
Currently there are 2-3 high priority issues reported by community and I
should find solutions/fixes.
Thank you very much for your time and useful feedback.
Thanks,
Minas
> regards,
> dan carpenter
>
>
^ permalink raw reply [flat|nested] 20+ messages in thread* Re: [PATCH] usb: dwc2: Revert "usb: dwc2: Disable all EP's on disconnect"
2018-12-07 11:20 ` Minas Harutyunyan
@ 2018-12-07 14:13 ` Minas Harutyunyan
2018-12-07 14:40 ` Dan Carpenter
0 siblings, 1 reply; 20+ messages in thread
From: Minas Harutyunyan @ 2018-12-07 14:13 UTC (permalink / raw)
To: Dan Carpenter, Minas Harutyunyan
Cc: Marek Szyprowski, Maynard CABIENTE, linux-kernel, linux-usb,
Greg Kroah-Hartman, Felipe Balbi, Geert Uytterhoeven,
Bartlomiej Zolnierkiewicz
Hi Dan,
On 12/7/2018 3:20 PM, Minas Harutyunyan wrote:
> Hi Dan,
>
> On 12/7/2018 2:16 PM, Dan Carpenter wrote:
>> On Wed, Dec 05, 2018 at 12:52:22PM +0000, Minas Harutyunyan wrote:
>>> Hi,
>>>
>>> On 12/4/2018 5:29 PM, Dan Carpenter wrote:
>>>> On Tue, Dec 04, 2018 at 12:34:08PM +0000, Minas Harutyunyan wrote:
>>>>> @@ -3185,12 +3183,13 @@ void dwc2_hsotg_disconnect(struct dwc2_hsotg *hsotg)
>>>>> hsotg->connected = 0;
>>>>> hsotg->test_mode = 0;
>>>>>
>>>>> - /* all endpoints should be shutdown */
>>>>> for (ep = 0; ep < hsotg->num_of_eps; ep++) {
>>>>> if (hsotg->eps_in[ep])
>>>>> - dwc2_hsotg_ep_disable(&hsotg->eps_in[ep]->ep);
>>>>> + kill_all_requests(hsotg, hsotg->eps_in[ep],
>>>>> + -ESHUTDOWN);
>>>>> if (hsotg->eps_out[ep])
>>>>> - dwc2_hsotg_ep_disable(&hsotg->eps_out[ep]->ep);
>>>>> + kill_all_requests(hsotg, hsotg->eps_out[ep],
>>>>> + -ESHUTDOWN);
>>>>
>>>>
>>>> Should this part be in a separate patch?
>>>>
>>>> I'm not trying to be rhetorical at all. I literally don't know the
>>>> code very well. Hopefully the full commit message will explain it.
>>>>
>>>
>>> Actually, this fragment of patch revert changes from V2 and keep
>>> untouched dwc2_hsotg_disconnect() function.
>>>
>>
>> To me it feels like there are two issues. The first is this change, and
>> the second is fixing the lockdep warning.
>>
>>
>>>>> }
>>>>>
>>>>> call_gadget(hsotg, disconnect);
>>>>> @@ -3234,6 +3233,8 @@ static void dwc2_hsotg_irq_fifoempty(struct
>>>>> dwc2_hsotg *hsotg, bool periodic)
>>>>> GINTSTS_PTXFEMP | \
>>>>> GINTSTS_RXFLVL)
>>>>>
>>>>> +static int dwc2_hsotg_ep_disable(struct usb_ep *ep);
>>>>> +
>>>>> /**
>>>>> * dwc2_hsotg_core_init - issue softreset to the core
>>>>> * @hsotg: The device state
>>>>> @@ -3258,12 +3259,14 @@ void dwc2_hsotg_core_init_disconnected(struct
>>>>> dwc2_hsotg *hsotg,
>>>>> return;
>>>>> } else {
>>>>> /* all endpoints should be shutdown */
>>>>> + spin_unlock(&hsotg->lock);
>>>>> for (ep = 1; ep < hsotg->num_of_eps; ep++) {
>>>>> if (hsotg->eps_in[ep])
>>>>>
>>>>> dwc2_hsotg_ep_disable(&hsotg->eps_in[ep]->ep);
>>>>> if (hsotg->eps_out[ep])
>>>>>
>>>>> dwc2_hsotg_ep_disable(&hsotg->eps_out[ep]->ep);
>>>>> }
>>>>> + spin_lock(&hsotg->lock);
>>>>> }
>>>>>
>>>>> /*
>>>>
>>>> The idea here is that this is the only caller which is holding the
>>>> lock and we drop it here and take it again inside dwc2_hsotg_ep_disable().
>>>> I don't know the code very well and can't totally swear that this
>>>> doesn't introduce a small race condition...
>>>>
>>> Above fragment of patch allow to keep untouched dwc2_hsotg_ep_disble()
>>> function also, without changing spin_lock/_unlock stuff inside function.
>>>
>>> My approach here minimally update code to add any races. Just in
>>> dwc2_hsotg_core_init_disconnected() function on USB reset interrupt
>>> perform disabling all EP's. Because on USB reset interrupt, called from interrupt
>>> handler with acquired lock and dwc2_hsotg_ep_disble() function (without
>>> changes) acquire lock, just need to unlock lock to avoid any troubles.
>>>
>>
>> Yes. I understand that. I just don't like it.
>>
>> Although your patch is more "minimal" in that it touches fewer lines of
>> code it's actually more complicated because we have to verify that it's
>> safe to drop the lock.
>>
>>
>>>> Another option would be to introduce a new function which takes the lock
>>>> and change all the other callers instead. To me that would be easier to
>>>> review... See below for how it might look:
>>>>
>>>> regards,
>>>> dan carpenter
>>>>
>>>> diff --git a/drivers/usb/dwc2/gadget.c b/drivers/usb/dwc2/gadget.c
>>>> index 94f3ba995580..b17a5dbefd5f 100644
>>>> --- a/drivers/usb/dwc2/gadget.c
>>>> +++ b/drivers/usb/dwc2/gadget.c
>>>> @@ -3166,6 +3166,7 @@ static void kill_all_requests(struct dwc2_hsotg *hsotg,
>>>> }
>>>>
>>>> static int dwc2_hsotg_ep_disable(struct usb_ep *ep);
>>>> +static int dwc2_hsotg_ep_disable_lock(struct usb_ep *ep);
>>>>
>>>> /**
>>>> * dwc2_hsotg_disconnect - disconnect service
>>>> @@ -3188,9 +3189,9 @@ void dwc2_hsotg_disconnect(struct dwc2_hsotg *hsotg)
>>>> /* all endpoints should be shutdown */
>>>> for (ep = 0; ep < hsotg->num_of_eps; ep++) {
>>>> if (hsotg->eps_in[ep])
>>>> - dwc2_hsotg_ep_disable(&hsotg->eps_in[ep]->ep);
>>>> + dwc2_hsotg_ep_disable_lock(&hsotg->eps_in[ep]->ep);
>>>> if (hsotg->eps_out[ep])
>>>> - dwc2_hsotg_ep_disable(&hsotg->eps_out[ep]->ep);
>>>> + dwc2_hsotg_ep_disable_lock(&hsotg->eps_out[ep]->ep);
>>>> }
>>>>
>>>> call_gadget(hsotg, disconnect);
>>>> @@ -4069,10 +4070,8 @@ static int dwc2_hsotg_ep_disable(struct usb_ep *ep)
>>>> struct dwc2_hsotg *hsotg = hs_ep->parent;
>>>> int dir_in = hs_ep->dir_in;
>>>> int index = hs_ep->index;
>>>> - unsigned long flags;
>>>> u32 epctrl_reg;
>>>> u32 ctrl;
>>>> - int locked;
>>>>
>>>> dev_dbg(hsotg->dev, "%s(ep %p)\n", __func__, ep);
>>>>
>>>> @@ -4088,10 +4087,6 @@ static int dwc2_hsotg_ep_disable(struct usb_ep *ep)
>>>>
>>>> epctrl_reg = dir_in ? DIEPCTL(index) : DOEPCTL(index);
>>>>
>>>> - locked = spin_is_locked(&hsotg->lock);
>>>> - if (!locked)
>>>> - spin_lock_irqsave(&hsotg->lock, flags);
>>>> -
>>>> ctrl = dwc2_readl(hsotg, epctrl_reg);
>>>>
>>>> if (ctrl & DXEPCTL_EPENA)
>>>> @@ -4114,12 +4109,23 @@ static int dwc2_hsotg_ep_disable(struct usb_ep *ep)
>>>> hs_ep->fifo_index = 0;
>>>> hs_ep->fifo_size = 0;
>>>>
>>>> - if (!locked)
>>>> - spin_unlock_irqrestore(&hsotg->lock, flags);
>>>> -
>>>> return 0;
>>>> }
>>>>
>>>> +static int dwc2_hsotg_ep_disable_lock(struct usb_ep *ep)
>>>> +{
>>>> + struct dwc2_hsotg_ep *hs_ep = our_ep(ep);
>>>> + struct dwc2_hsotg *hsotg = hs_ep->parent;
>>>> + unsigned long flags;
>>>> + int ret;
>>>> +
>>>> + spin_lock_irqsave(&hsotg->lock, flags);
>>>> + ret = dwc2_hsotg_ep_disable(ep);
>>>> + spin_unlock_irqrestore(&hsotg->lock, flags);
>>>> +
>>>> + return ret;
>>>> +}
>>>> +
>>>> /**
>>>> * on_list - check request is on the given endpoint
>>>> * @ep: The endpoint to check.
>>>> @@ -4267,7 +4273,7 @@ static int dwc2_hsotg_ep_sethalt_lock(struct usb_ep *ep, int value)
>>>>
>>>> static const struct usb_ep_ops dwc2_hsotg_ep_ops = {
>>>> .enable = dwc2_hsotg_ep_enable,
>>>> - .disable = dwc2_hsotg_ep_disable,
>>>> + .disable = dwc2_hsotg_ep_disable_lock,
>>>> .alloc_request = dwc2_hsotg_ep_alloc_request,
>>>> .free_request = dwc2_hsotg_ep_free_request,
>>>> .queue = dwc2_hsotg_ep_queue_lock,
>>>> @@ -4407,9 +4413,9 @@ static int dwc2_hsotg_udc_stop(struct usb_gadget *gadget)
>>>> /* all endpoints should be shutdown */
>>>> for (ep = 1; ep < hsotg->num_of_eps; ep++) {
>>>> if (hsotg->eps_in[ep])
>>>> - dwc2_hsotg_ep_disable(&hsotg->eps_in[ep]->ep);
>>>> + dwc2_hsotg_ep_disable_lock(&hsotg->eps_in[ep]->ep);
>>>> if (hsotg->eps_out[ep])
>>>> - dwc2_hsotg_ep_disable(&hsotg->eps_out[ep]->ep);
>>>> + dwc2_hsotg_ep_disable_lock(&hsotg->eps_out[ep]->ep);
>>>> }
>>>>
>>>> spin_lock_irqsave(&hsotg->lock, flags);
>>>> @@ -4857,9 +4863,9 @@ int dwc2_hsotg_suspend(struct dwc2_hsotg *hsotg)
>>>>
>>>> for (ep = 0; ep < hsotg->num_of_eps; ep++) {
>>>> if (hsotg->eps_in[ep])
>>>> - dwc2_hsotg_ep_disable(&hsotg->eps_in[ep]->ep);
>>>> + dwc2_hsotg_ep_disable_lock(&hsotg->eps_in[ep]->ep);
>>>> if (hsotg->eps_out[ep])
>>>> - dwc2_hsotg_ep_disable(&hsotg->eps_out[ep]->ep);
>>>> + dwc2_hsotg_ep_disable_lock(&hsotg->eps_out[ep]->ep);
>>>> }
>>>> }
>>>>
>>>>
>>>
>>> Your code doesn't take care about fifo_map warnings from
>>> dwc2_hsotg_init_fifo() function. Before calling dwc2_hsotg_init_fifo()
>>> from dwc2_hsotg_core_init_disconnected() function all Ep's should
>>> disabled and fifo bitmap should be cleared.
>>>
>>
>> Correct. I am only trying to fix the locking. I hope you can fix the
>> rest in a separate patch.
>>
> Yeah. I'll try deeper investigate driver locking flow and fix it later.
> Actually, I like your idea with introducing dwc2_hsotg_ep_disable_lock()
> function. Maybe you yourself will submit new patch for safe locking
> fixes? But please just after my patch will applied :-)
> Currently there are 2-3 high priority issues reported by community and I
> should find solutions/fixes.
> Thank you very much for your time and useful feedback.
>
> Thanks,
> Minas
>
>
>> regards,
>> dan carpenter
>>
>>
>
>
My patch doesn't pass sparse checking: "warning: context imbalance in
'dwc2_hsotg_core_init_disconnected' - unexpected unlock". Sparse persist!
So, I need to re-work patch. Can I use your idea with
dwc2_hsotg_ep_disable_lock() function to prepare new one?
Thanks,
Minas
^ permalink raw reply [flat|nested] 20+ messages in thread* Re: [PATCH] usb: dwc2: Revert "usb: dwc2: Disable all EP's on disconnect"
2018-12-07 14:13 ` Minas Harutyunyan
@ 2018-12-07 14:40 ` Dan Carpenter
0 siblings, 0 replies; 20+ messages in thread
From: Dan Carpenter @ 2018-12-07 14:40 UTC (permalink / raw)
To: Minas Harutyunyan
Cc: Marek Szyprowski, Maynard CABIENTE, linux-kernel, linux-usb,
Greg Kroah-Hartman, Felipe Balbi, Geert Uytterhoeven,
Bartlomiej Zolnierkiewicz
On Fri, Dec 07, 2018 at 02:13:18PM +0000, Minas Harutyunyan wrote:
>
> My patch doesn't pass sparse checking: "warning: context imbalance in
> 'dwc2_hsotg_core_init_disconnected' - unexpected unlock". Sparse persist!
> So, I need to re-work patch. Can I use your idea with
> dwc2_hsotg_ep_disable_lock() function to prepare new one?
>
Sparse lock checking is pretty crap, and I wouldn't go out of my way
normally to make it happy... But of course you can use my idea.
regards,
dan carpenter
^ permalink raw reply [flat|nested] 20+ messages in thread
* Re: [PATCH] usb: dwc2: Revert "usb: dwc2: Disable all EP's on disconnect"
2018-12-04 12:34 ` Minas Harutyunyan
2018-12-04 13:29 ` Dan Carpenter
@ 2018-12-06 15:03 ` Marek Szyprowski
2018-12-07 9:06 ` Minas Harutyunyan
2018-12-06 23:09 ` Maynard CABIENTE
2 siblings, 1 reply; 20+ messages in thread
From: Marek Szyprowski @ 2018-12-06 15:03 UTC (permalink / raw)
To: Minas Harutyunyan, Dan Carpenter, Maynard CABIENTE
Cc: linux-kernel, linux-usb, Greg Kroah-Hartman, Felipe Balbi,
Geert Uytterhoeven, Bartlomiej Zolnierkiewicz
Dear Minas,
On 2018-12-04 13:34, Minas Harutyunyan wrote:
> On 11/23/2018 6:43 PM, Dan Carpenter wrote:
>> Ugh... We also had a long thread about the v2 patch but it turns out
>> the list was not CC'd. I should have checked for that.
>>
>> You have to pass a flag to say if the caller holds the lock or not...
>>
>> regards,
>> dan carpenter
>>
> Hi Dan, Marek, Maynard,
>
> Could you please apply bellow patch over follow patches:
>
> dccf1bad4be7 usb: dwc2: Disable all EP's on disconnect
> 6f774b501928 usb: dwc2: Fix ep disable spinlock flow.
>
> Please review and test. Feedback is appreciated :-)
Okay, I finally managed to find some time to check this. Your diff is
mangled, so I had to manually apply it. Frankly, it is very similar to
the revert I proposed. I've checked it on my test machines and it fixes
the issues. I'm not very happy about the unlock/lock design, but it
should be safe in this case and doesn't make the code even more complex.
Feel free to add a following tag to the final patch:
Tested-by: Marek Szyprowski <m.szyprowski@samsung.com>
> diff --git a/drivers/usb/dwc2/gadget.c b/drivers/usb/dwc2/gadget.c
> index 8eb25e3597b0..db438d13c36a 100644
> --- a/drivers/usb/dwc2/gadget.c
> +++ b/drivers/usb/dwc2/gadget.c
> @@ -3165,8 +3165,6 @@ static void kill_all_requests(struct dwc2_hsotg
> *hsotg,
> dwc2_hsotg_txfifo_flush(hsotg, ep->fifo_index);
> }
>
> -static int dwc2_hsotg_ep_disable(struct usb_ep *ep);
> -
> /**
> * dwc2_hsotg_disconnect - disconnect service
> * @hsotg: The device state.
> @@ -3185,12 +3183,13 @@ void dwc2_hsotg_disconnect(struct dwc2_hsotg *hsotg)
> hsotg->connected = 0;
> hsotg->test_mode = 0;
>
> - /* all endpoints should be shutdown */
> for (ep = 0; ep < hsotg->num_of_eps; ep++) {
> if (hsotg->eps_in[ep])
> - dwc2_hsotg_ep_disable(&hsotg->eps_in[ep]->ep);
> + kill_all_requests(hsotg, hsotg->eps_in[ep],
> + -ESHUTDOWN);
> if (hsotg->eps_out[ep])
> - dwc2_hsotg_ep_disable(&hsotg->eps_out[ep]->ep);
> + kill_all_requests(hsotg, hsotg->eps_out[ep],
> + -ESHUTDOWN);
> }
>
> call_gadget(hsotg, disconnect);
> @@ -3234,6 +3233,8 @@ static void dwc2_hsotg_irq_fifoempty(struct
> dwc2_hsotg *hsotg, bool periodic)
> GINTSTS_PTXFEMP | \
> GINTSTS_RXFLVL)
>
> +static int dwc2_hsotg_ep_disable(struct usb_ep *ep);
> +
> /**
> * dwc2_hsotg_core_init - issue softreset to the core
> * @hsotg: The device state
> @@ -3258,12 +3259,14 @@ void dwc2_hsotg_core_init_disconnected(struct
> dwc2_hsotg *hsotg,
> return;
> } else {
> /* all endpoints should be shutdown */
> + spin_unlock(&hsotg->lock);
> for (ep = 1; ep < hsotg->num_of_eps; ep++) {
> if (hsotg->eps_in[ep])
>
> dwc2_hsotg_ep_disable(&hsotg->eps_in[ep]->ep);
> if (hsotg->eps_out[ep])
>
> dwc2_hsotg_ep_disable(&hsotg->eps_out[ep]->ep);
> }
> + spin_lock(&hsotg->lock);
> }
>
> /*
> @@ -4072,7 +4075,6 @@ 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);
>
> @@ -4088,7 +4090,7 @@ static int dwc2_hsotg_ep_disable(struct usb_ep *ep)
>
> epctrl_reg = dir_in ? DIEPCTL(index) : DOEPCTL(index);
>
> - locked = spin_trylock_irqsave(&hsotg->lock, flags);
> + spin_lock_irqsave(&hsotg->lock, flags);
>
> ctrl = dwc2_readl(hsotg, epctrl_reg);
>
> @@ -4112,8 +4114,7 @@ static int dwc2_hsotg_ep_disable(struct usb_ep *ep)
> hs_ep->fifo_index = 0;
> hs_ep->fifo_size = 0;
>
> - if (locked)
> - spin_unlock_irqrestore(&hsotg->lock, flags);
> + spin_unlock_irqrestore(&hsotg->lock, flags);
>
> return 0;
> }
>
> Thanks,
> Minas
>
>
Best regards
--
Marek Szyprowski, PhD
Samsung R&D Institute Poland
^ permalink raw reply [flat|nested] 20+ messages in thread* Re: [PATCH] usb: dwc2: Revert "usb: dwc2: Disable all EP's on disconnect"
2018-12-06 15:03 ` Marek Szyprowski
@ 2018-12-07 9:06 ` Minas Harutyunyan
2018-12-07 9:50 ` Felipe Balbi
2018-12-07 9:58 ` Minas Harutyunyan
0 siblings, 2 replies; 20+ messages in thread
From: Minas Harutyunyan @ 2018-12-07 9:06 UTC (permalink / raw)
To: Marek Szyprowski, Minas Harutyunyan, Dan Carpenter, Maynard CABIENTE
Cc: linux-kernel, linux-usb, Greg Kroah-Hartman, Felipe Balbi,
Geert Uytterhoeven, Bartlomiej Zolnierkiewicz
Hi Marek,
On 12/6/2018 7:04 PM, Marek Szyprowski wrote:
> Dear Minas,
>
> On 2018-12-04 13:34, Minas Harutyunyan wrote:
>> On 11/23/2018 6:43 PM, Dan Carpenter wrote:
>>> Ugh... We also had a long thread about the v2 patch but it turns out
>>> the list was not CC'd. I should have checked for that.
>>>
>>> You have to pass a flag to say if the caller holds the lock or not...
>>>
>>> regards,
>>> dan carpenter
>>>
>> Hi Dan, Marek, Maynard,
>>
>> Could you please apply bellow patch over follow patches:
>>
>> dccf1bad4be7 usb: dwc2: Disable all EP's on disconnect
>> 6f774b501928 usb: dwc2: Fix ep disable spinlock flow.
>>
>> Please review and test. Feedback is appreciated :-)
>
> Okay, I finally managed to find some time to check this. Your diff is
> mangled, so I had to manually apply it. Frankly, it is very similar to
> the revert I proposed. I've checked it on my test machines and it fixes
> the issues. I'm not very happy about the unlock/lock design, but it
> should be safe in this case and doesn't make the code even more complex.
> Feel free to add a following tag to the final patch:
>
> Tested-by: Marek Szyprowski <m.szyprowski@samsung.com>
Thanks for testing.
>
>> diff --git a/drivers/usb/dwc2/gadget.c b/drivers/usb/dwc2/gadget.c
>> index 8eb25e3597b0..db438d13c36a 100644
>> --- a/drivers/usb/dwc2/gadget.c
>> +++ b/drivers/usb/dwc2/gadget.c
>> @@ -3165,8 +3165,6 @@ static void kill_all_requests(struct dwc2_hsotg
>> *hsotg,
>> dwc2_hsotg_txfifo_flush(hsotg, ep->fifo_index);
>> }
>>
>> -static int dwc2_hsotg_ep_disable(struct usb_ep *ep);
>> -
>> /**
>> * dwc2_hsotg_disconnect - disconnect service
>> * @hsotg: The device state.
>> @@ -3185,12 +3183,13 @@ void dwc2_hsotg_disconnect(struct dwc2_hsotg *hsotg)
>> hsotg->connected = 0;
>> hsotg->test_mode = 0;
>>
>> - /* all endpoints should be shutdown */
>> for (ep = 0; ep < hsotg->num_of_eps; ep++) {
>> if (hsotg->eps_in[ep])
>> - dwc2_hsotg_ep_disable(&hsotg->eps_in[ep]->ep);
>> + kill_all_requests(hsotg, hsotg->eps_in[ep],
>> + -ESHUTDOWN);
>> if (hsotg->eps_out[ep])
>> - dwc2_hsotg_ep_disable(&hsotg->eps_out[ep]->ep);
>> + kill_all_requests(hsotg, hsotg->eps_out[ep],
>> + -ESHUTDOWN);
>> }
>>
>> call_gadget(hsotg, disconnect);
>> @@ -3234,6 +3233,8 @@ static void dwc2_hsotg_irq_fifoempty(struct
>> dwc2_hsotg *hsotg, bool periodic)
>> GINTSTS_PTXFEMP | \
>> GINTSTS_RXFLVL)
>>
>> +static int dwc2_hsotg_ep_disable(struct usb_ep *ep);
>> +
>> /**
>> * dwc2_hsotg_core_init - issue softreset to the core
>> * @hsotg: The device state
>> @@ -3258,12 +3259,14 @@ void dwc2_hsotg_core_init_disconnected(struct
>> dwc2_hsotg *hsotg,
>> return;
>> } else {
>> /* all endpoints should be shutdown */
>> + spin_unlock(&hsotg->lock);
>> for (ep = 1; ep < hsotg->num_of_eps; ep++) {
>> if (hsotg->eps_in[ep])
>>
>> dwc2_hsotg_ep_disable(&hsotg->eps_in[ep]->ep);
>> if (hsotg->eps_out[ep])
>>
>> dwc2_hsotg_ep_disable(&hsotg->eps_out[ep]->ep);
>> }
>> + spin_lock(&hsotg->lock);
>> }
>>
>> /*
>> @@ -4072,7 +4075,6 @@ 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);
>>
>> @@ -4088,7 +4090,7 @@ static int dwc2_hsotg_ep_disable(struct usb_ep *ep)
>>
>> epctrl_reg = dir_in ? DIEPCTL(index) : DOEPCTL(index);
>>
>> - locked = spin_trylock_irqsave(&hsotg->lock, flags);
>> + spin_lock_irqsave(&hsotg->lock, flags);
>>
>> ctrl = dwc2_readl(hsotg, epctrl_reg);
>>
>> @@ -4112,8 +4114,7 @@ static int dwc2_hsotg_ep_disable(struct usb_ep *ep)
>> hs_ep->fifo_index = 0;
>> hs_ep->fifo_size = 0;
>>
>> - if (locked)
>> - spin_unlock_irqrestore(&hsotg->lock, flags);
>> + spin_unlock_irqrestore(&hsotg->lock, flags);
>>
>> return 0;
>> }
>>
>> Thanks,
>> Minas
>>
>>
> Best regards
>
Thanks,
Minas
^ permalink raw reply [flat|nested] 20+ messages in thread* Re: [PATCH] usb: dwc2: Revert "usb: dwc2: Disable all EP's on disconnect"
2018-12-07 9:06 ` Minas Harutyunyan
@ 2018-12-07 9:50 ` Felipe Balbi
2018-12-07 9:58 ` Minas Harutyunyan
1 sibling, 0 replies; 20+ messages in thread
From: Felipe Balbi @ 2018-12-07 9:50 UTC (permalink / raw)
To: Minas Harutyunyan, Marek Szyprowski, Minas Harutyunyan,
Dan Carpenter, Maynard CABIENTE
Cc: linux-kernel, linux-usb, Greg Kroah-Hartman, Geert Uytterhoeven,
Bartlomiej Zolnierkiewicz
Hi,
Minas Harutyunyan <minas.harutyunyan@synopsys.com> writes:
> Hi Marek,
>
> On 12/6/2018 7:04 PM, Marek Szyprowski wrote:
>> Dear Minas,
>>
>> On 2018-12-04 13:34, Minas Harutyunyan wrote:
>>> On 11/23/2018 6:43 PM, Dan Carpenter wrote:
>>>> Ugh... We also had a long thread about the v2 patch but it turns out
>>>> the list was not CC'd. I should have checked for that.
>>>>
>>>> You have to pass a flag to say if the caller holds the lock or not...
>>>>
>>>> regards,
>>>> dan carpenter
>>>>
>>> Hi Dan, Marek, Maynard,
>>>
>>> Could you please apply bellow patch over follow patches:
>>>
>>> dccf1bad4be7 usb: dwc2: Disable all EP's on disconnect
>>> 6f774b501928 usb: dwc2: Fix ep disable spinlock flow.
>>>
>>> Please review and test. Feedback is appreciated :-)
>>
>> Okay, I finally managed to find some time to check this. Your diff is
>> mangled, so I had to manually apply it. Frankly, it is very similar to
>> the revert I proposed. I've checked it on my test machines and it fixes
>> the issues. I'm not very happy about the unlock/lock design, but it
>> should be safe in this case and doesn't make the code even more complex.
>> Feel free to add a following tag to the final patch:
>>
>> Tested-by: Marek Szyprowski <m.szyprowski@samsung.com>
> Thanks for testing.
should $Subject be applied? Can you resend rebased on testing/next with
Tested-by tags collected if you're happy with it?
thanks
--
balbi
^ permalink raw reply [flat|nested] 20+ messages in thread
* Re: [PATCH] usb: dwc2: Revert "usb: dwc2: Disable all EP's on disconnect"
2018-12-07 9:06 ` Minas Harutyunyan
2018-12-07 9:50 ` Felipe Balbi
@ 2018-12-07 9:58 ` Minas Harutyunyan
2018-12-07 10:11 ` Felipe Balbi
1 sibling, 1 reply; 20+ messages in thread
From: Minas Harutyunyan @ 2018-12-07 9:58 UTC (permalink / raw)
To: Minas Harutyunyan, Felipe Balbi
Cc: Marek Szyprowski, Dan Carpenter, Maynard CABIENTE, linux-kernel,
linux-usb, Greg Kroah-Hartman, Geert Uytterhoeven,
Bartlomiej Zolnierkiewicz
Hi Filipe,
My patch dccf1bad4be7eaa096c1f3697bd37883f9a08ecb "usb: dwc2: Disable
all EP's on disconnect" applied to 4.20-rc1.
I need to update this patch. What I should do. There are 2 options:
1. Ack Marek Szyprowski <m.szyprowski@samsung.com> [PATCH] usb: dwc2:
Revert "usb: dwc2: Disable all EP's on disconnect" then submit final
fixed version of patch?
2. Or submit new patch to fix existing "usb: dwc2: Disable all EP's on
disconnect".
Please advise.
Thanks,
Minas
^ permalink raw reply [flat|nested] 20+ messages in thread
* Re: [PATCH] usb: dwc2: Revert "usb: dwc2: Disable all EP's on disconnect"
2018-12-07 9:58 ` Minas Harutyunyan
@ 2018-12-07 10:11 ` Felipe Balbi
0 siblings, 0 replies; 20+ messages in thread
From: Felipe Balbi @ 2018-12-07 10:11 UTC (permalink / raw)
To: Minas Harutyunyan, Minas Harutyunyan
Cc: Marek Szyprowski, Dan Carpenter, Maynard CABIENTE, linux-kernel,
linux-usb, Greg Kroah-Hartman, Geert Uytterhoeven,
Bartlomiej Zolnierkiewicz
Hi,
Minas Harutyunyan <minas.harutyunyan@synopsys.com> writes:
> Hi Filipe,
>
> My patch dccf1bad4be7eaa096c1f3697bd37883f9a08ecb "usb: dwc2: Disable
> all EP's on disconnect" applied to 4.20-rc1.
>
> I need to update this patch. What I should do. There are 2 options:
>
> 1. Ack Marek Szyprowski <m.szyprowski@samsung.com> [PATCH] usb: dwc2:
> Revert "usb: dwc2: Disable all EP's on disconnect" then submit final
> fixed version of patch?
>
> 2. Or submit new patch to fix existing "usb: dwc2: Disable all EP's on
> disconnect".
I would say it's best to go with option 2. Just send a fix on top :-)
--
balbi
^ permalink raw reply [flat|nested] 20+ messages in thread
* RE: [PATCH] usb: dwc2: Revert "usb: dwc2: Disable all EP's on disconnect"
2018-12-04 12:34 ` Minas Harutyunyan
2018-12-04 13:29 ` Dan Carpenter
2018-12-06 15:03 ` Marek Szyprowski
@ 2018-12-06 23:09 ` Maynard CABIENTE
2018-12-07 9:06 ` Minas Harutyunyan
2 siblings, 1 reply; 20+ messages in thread
From: Maynard CABIENTE @ 2018-12-06 23:09 UTC (permalink / raw)
To: Minas Harutyunyan, Dan Carpenter, Marek Szyprowski
Cc: linux-kernel, linux-usb, Greg Kroah-Hartman, Felipe Balbi,
Geert Uytterhoeven, Bartlomiej Zolnierkiewicz
Hi Minas,
I tried your new patch on top of the other 2 patches for a couple of days now and I do not see the issue that Marek encountered. Of course, I did not see it also on the original two patches you created. I also do not see the original FIFO map warning issue that I have with all 3 patches intact.
However, I do have 2 issues that I'm not certain if this was created by these patches or already there in the beginning (prior to applying all your 3 patches). Again my system is an Altera Cyclone V SoC FPGA running on linux kernel 4.14.44. We are using both USB HID and mass storage gadget. The 2 issues may occur when USB mass storage is in use.
First issue I am encountering is (sometimes continuous) multiple USB resets occurring when using the USB mass storage gadget. When I used a USB sniffer (Ellisys) to see the cause of the USB reset, I am seeing an out-of-order USB mass storage transfer. That is, the transfer is supposed to follow (1) Command Block Wrapper (CBW), (2) Data, (3) Command Status Wrapper (CSW). When the USB reset occur, I see either one of the following prior to the reset:
- Data comes in first before the CBW
- Request Sense CBW, Data then no CSW followed by a Test Unit Ready CBW and Data with no CSW again
- CBW, CSW and then Data
Second issue that I see is a deadlock then my system will reboot. This happens when I re-enable the USB mass storage gadget after disabling it before.
The two issues may not be related to your changes. I will revert back all your patches to see if the issue is present already. However, I will hit my original problem when I don't apply at least 2 of your patches, which is the WARNING: CPU: 0 PID: 0 at ../drivers/usb/dwc2/gadget.c:300 dwc2_hsotg_init_fifo+0x34/0x1b4.
I will get back to you once I test it without your patches.
Regards,
Maynard
On Tuesday, December 04, 2018 7:34 AM, Minas Harutyunyan wrote:
> On 11/23/2018 6:43 PM, Dan Carpenter wrote:
> > Ugh... We also had a long thread about the v2 patch but it turns out
> > the list was not CC'd. I should have checked for that.
> >
> > You have to pass a flag to say if the caller holds the lock or not...
> >
> > regards,
> > dan carpenter
> >
>
> Hi Dan, Marek, Maynard,
>
> Could you please apply bellow patch over follow patches:
>
> dccf1bad4be7 usb: dwc2: Disable all EP's on disconnect
> 6f774b501928 usb: dwc2: Fix ep disable spinlock flow.
>
> Please review and test. Feedback is appreciated :-)
>
> diff --git a/drivers/usb/dwc2/gadget.c b/drivers/usb/dwc2/gadget.c index
> 8eb25e3597b0..db438d13c36a 100644
> --- a/drivers/usb/dwc2/gadget.c
> +++ b/drivers/usb/dwc2/gadget.c
> @@ -3165,8 +3165,6 @@ static void kill_all_requests(struct dwc2_hsotg
> *hsotg,
> dwc2_hsotg_txfifo_flush(hsotg, ep->fifo_index);
> }
>
> -static int dwc2_hsotg_ep_disable(struct usb_ep *ep);
> -
> /**
> * dwc2_hsotg_disconnect - disconnect service
> * @hsotg: The device state.
> @@ -3185,12 +3183,13 @@ void dwc2_hsotg_disconnect(struct dwc2_hsotg
> *hsotg)
> hsotg->connected = 0;
> hsotg->test_mode = 0;
>
> - /* all endpoints should be shutdown */
> for (ep = 0; ep < hsotg->num_of_eps; ep++) {
> if (hsotg->eps_in[ep])
> - dwc2_hsotg_ep_disable(&hsotg->eps_in[ep]->ep);
> + kill_all_requests(hsotg, hsotg->eps_in[ep],
> + -ESHUTDOWN);
> if (hsotg->eps_out[ep])
> - dwc2_hsotg_ep_disable(&hsotg->eps_out[ep]->ep);
> + kill_all_requests(hsotg, hsotg->eps_out[ep],
> + -ESHUTDOWN);
> }
>
> call_gadget(hsotg, disconnect); @@ -3234,6 +3233,8 @@ static void
> dwc2_hsotg_irq_fifoempty(struct dwc2_hsotg *hsotg, bool periodic)
> GINTSTS_PTXFEMP | \
> GINTSTS_RXFLVL)
>
> +static int dwc2_hsotg_ep_disable(struct usb_ep *ep);
> +
> /**
> * dwc2_hsotg_core_init - issue softreset to the core
> * @hsotg: The device state
> @@ -3258,12 +3259,14 @@ void dwc2_hsotg_core_init_disconnected(struct
> dwc2_hsotg *hsotg,
> return;
> } else {
> /* all endpoints should be shutdown */
> + spin_unlock(&hsotg->lock);
> for (ep = 1; ep < hsotg->num_of_eps; ep++) {
> if (hsotg->eps_in[ep])
>
> dwc2_hsotg_ep_disable(&hsotg->eps_in[ep]->ep);
> if (hsotg->eps_out[ep])
>
> dwc2_hsotg_ep_disable(&hsotg->eps_out[ep]->ep);
> }
> + spin_lock(&hsotg->lock);
> }
>
> /*
> @@ -4072,7 +4075,6 @@ 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);
>
> @@ -4088,7 +4090,7 @@ static int dwc2_hsotg_ep_disable(struct usb_ep
> *ep)
>
> epctrl_reg = dir_in ? DIEPCTL(index) : DOEPCTL(index);
>
> - locked = spin_trylock_irqsave(&hsotg->lock, flags);
> + spin_lock_irqsave(&hsotg->lock, flags);
>
> ctrl = dwc2_readl(hsotg, epctrl_reg);
>
> @@ -4112,8 +4114,7 @@ static int dwc2_hsotg_ep_disable(struct usb_ep
> *ep)
> hs_ep->fifo_index = 0;
> hs_ep->fifo_size = 0;
>
> - if (locked)
> - spin_unlock_irqrestore(&hsotg->lock, flags);
> + spin_unlock_irqrestore(&hsotg->lock, flags);
>
> return 0;
> }
>
> Thanks,
> Minas
________________________________
Ce message, ainsi que tous les fichiers joints à ce message, peuvent contenir des informations sensibles et/ ou confidentielles ne devant pas être divulguées. Si vous n'êtes pas le destinataire de ce message (ou que vous recevez ce message par erreur), nous vous remercions de le notifier immédiatement à son expéditeur, et de détruire ce message. Toute copie, divulgation, modification, utilisation ou diffusion, non autorisée, directe ou indirecte, de tout ou partie de ce message, est strictement interdite.
This e-mail, and any document attached hereby, may contain confidential and/or privileged information. If you are not the intended recipient (or have received this e-mail in error) please notify the sender immediately and destroy this e-mail. Any unauthorized, direct or indirect, copying, disclosure, distribution or other use of the material or parts thereof is strictly forbidden.
^ permalink raw reply [flat|nested] 20+ messages in thread* Re: [PATCH] usb: dwc2: Revert "usb: dwc2: Disable all EP's on disconnect"
2018-12-06 23:09 ` Maynard CABIENTE
@ 2018-12-07 9:06 ` Minas Harutyunyan
0 siblings, 0 replies; 20+ messages in thread
From: Minas Harutyunyan @ 2018-12-07 9:06 UTC (permalink / raw)
To: Maynard CABIENTE, Minas Harutyunyan, Dan Carpenter, Marek Szyprowski
Cc: linux-kernel, linux-usb, Greg Kroah-Hartman, Felipe Balbi,
Geert Uytterhoeven, Bartlomiej Zolnierkiewicz
Hi Maynard,
On 12/7/2018 3:09 AM, Maynard CABIENTE wrote:
> Hi Minas,
>
> I tried your new patch on top of the other 2 patches for a couple of days now and I do not see the issue that Marek encountered. Of course, I did not see it also on the original two patches you created. I also do not see the original FIFO map warning issue that I have with all 3 patches intact.
>
> However, I do have 2 issues that I'm not certain if this was created by these patches or already there in the beginning (prior to applying all your 3 patches). Again my system is an Altera Cyclone V SoC FPGA running on linux kernel 4.14.44. We are using both USB HID and mass storage gadget. The 2 issues may occur when USB mass storage is in use.
>
> First issue I am encountering is (sometimes continuous) multiple USB resets occurring when using the USB mass storage gadget. When I used a USB sniffer (Ellisys) to see the cause of the USB reset, I am seeing an out-of-order USB mass storage transfer. That is, the transfer is supposed to follow (1) Command Block Wrapper (CBW), (2) Data, (3) Command Status Wrapper (CSW). When the USB reset occur, I see either one of the following prior to the reset:
>
> - Data comes in first before the CBW
> - Request Sense CBW, Data then no CSW followed by a Test Unit Ready CBW and Data with no CSW again
> - CBW, CSW and then Data
>
> Second issue that I see is a deadlock then my system will reboot. This happens when I re-enable the USB mass storage gadget after disabling it before.
>
> The two issues may not be related to your changes. I will revert back all your patches to see if the issue is present already. However, I will hit my original problem when I don't apply at least 2 of your patches, which is the WARNING: CPU: 0 PID: 0 at ../drivers/usb/dwc2/gadget.c:300 dwc2_hsotg_init_fifo+0x34/0x1b4.
>
> I will get back to you once I test it without your patches.
>
> Regards,
> Maynard
>
Thank you for testing.
Issue which described looks like not related to these patches. For that
issue please open another mail thread with more details. We will work on it.
Thanks,
Minas
> On Tuesday, December 04, 2018 7:34 AM, Minas Harutyunyan wrote:
>> On 11/23/2018 6:43 PM, Dan Carpenter wrote:
>>> Ugh... We also had a long thread about the v2 patch but it turns out
>>> the list was not CC'd. I should have checked for that.
>>>
>>> You have to pass a flag to say if the caller holds the lock or not...
>>>
>>> regards,
>>> dan carpenter
>>>
>>
>> Hi Dan, Marek, Maynard,
>>
>> Could you please apply bellow patch over follow patches:
>>
>> dccf1bad4be7 usb: dwc2: Disable all EP's on disconnect
>> 6f774b501928 usb: dwc2: Fix ep disable spinlock flow.
>>
>> Please review and test. Feedback is appreciated :-)
>>
>> diff --git a/drivers/usb/dwc2/gadget.c b/drivers/usb/dwc2/gadget.c index
>> 8eb25e3597b0..db438d13c36a 100644
>> --- a/drivers/usb/dwc2/gadget.c
>> +++ b/drivers/usb/dwc2/gadget.c
>> @@ -3165,8 +3165,6 @@ static void kill_all_requests(struct dwc2_hsotg
>> *hsotg,
>> dwc2_hsotg_txfifo_flush(hsotg, ep->fifo_index);
>> }
>>
>> -static int dwc2_hsotg_ep_disable(struct usb_ep *ep);
>> -
>> /**
>> * dwc2_hsotg_disconnect - disconnect service
>> * @hsotg: The device state.
>> @@ -3185,12 +3183,13 @@ void dwc2_hsotg_disconnect(struct dwc2_hsotg
>> *hsotg)
>> hsotg->connected = 0;
>> hsotg->test_mode = 0;
>>
>> - /* all endpoints should be shutdown */
>> for (ep = 0; ep < hsotg->num_of_eps; ep++) {
>> if (hsotg->eps_in[ep])
>> - dwc2_hsotg_ep_disable(&hsotg->eps_in[ep]->ep);
>> + kill_all_requests(hsotg, hsotg->eps_in[ep],
>> + -ESHUTDOWN);
>> if (hsotg->eps_out[ep])
>> - dwc2_hsotg_ep_disable(&hsotg->eps_out[ep]->ep);
>> + kill_all_requests(hsotg, hsotg->eps_out[ep],
>> + -ESHUTDOWN);
>> }
>>
>> call_gadget(hsotg, disconnect); @@ -3234,6 +3233,8 @@ static void
>> dwc2_hsotg_irq_fifoempty(struct dwc2_hsotg *hsotg, bool periodic)
>> GINTSTS_PTXFEMP | \
>> GINTSTS_RXFLVL)
>>
>> +static int dwc2_hsotg_ep_disable(struct usb_ep *ep);
>> +
>> /**
>> * dwc2_hsotg_core_init - issue softreset to the core
>> * @hsotg: The device state
>> @@ -3258,12 +3259,14 @@ void dwc2_hsotg_core_init_disconnected(struct
>> dwc2_hsotg *hsotg,
>> return;
>> } else {
>> /* all endpoints should be shutdown */
>> + spin_unlock(&hsotg->lock);
>> for (ep = 1; ep < hsotg->num_of_eps; ep++) {
>> if (hsotg->eps_in[ep])
>>
>> dwc2_hsotg_ep_disable(&hsotg->eps_in[ep]->ep);
>> if (hsotg->eps_out[ep])
>>
>> dwc2_hsotg_ep_disable(&hsotg->eps_out[ep]->ep);
>> }
>> + spin_lock(&hsotg->lock);
>> }
>>
>> /*
>> @@ -4072,7 +4075,6 @@ 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);
>>
>> @@ -4088,7 +4090,7 @@ static int dwc2_hsotg_ep_disable(struct usb_ep
>> *ep)
>>
>> epctrl_reg = dir_in ? DIEPCTL(index) : DOEPCTL(index);
>>
>> - locked = spin_trylock_irqsave(&hsotg->lock, flags);
>> + spin_lock_irqsave(&hsotg->lock, flags);
>>
>> ctrl = dwc2_readl(hsotg, epctrl_reg);
>>
>> @@ -4112,8 +4114,7 @@ static int dwc2_hsotg_ep_disable(struct usb_ep
>> *ep)
>> hs_ep->fifo_index = 0;
>> hs_ep->fifo_size = 0;
>>
>> - if (locked)
>> - spin_unlock_irqrestore(&hsotg->lock, flags);
>> + spin_unlock_irqrestore(&hsotg->lock, flags);
>>
>> return 0;
>> }
>>
>> Thanks,
>> Minas
>
> ________________________________
>
> Ce message, ainsi que tous les fichiers joints à ce message, peuvent contenir des informations sensibles et/ ou confidentielles ne devant pas être divulguées. Si vous n'êtes pas le destinataire de ce message (ou que vous recevez ce message par erreur), nous vous remercions de le notifier immédiatement à son expéditeur, et de détruire ce message. Toute copie, divulgation, modification, utilisation ou diffusion, non autorisée, directe ou indirecte, de tout ou partie de ce message, est strictement interdite.
>
>
> This e-mail, and any document attached hereby, may contain confidential and/or privileged information. If you are not the intended recipient (or have received this e-mail in error) please notify the sender immediately and destroy this e-mail. Any unauthorized, direct or indirect, copying, disclosure, distribution or other use of the material or parts thereof is strictly forbidden.
>
^ permalink raw reply [flat|nested] 20+ messages in thread