diff mbox series

[v2,11/25] usb: gadget: f_tcm: Execute command on write completion

Message ID b030d10834c13aa09bbbba7b33b1957d5ba3664c.1658192351.git.Thinh.Nguyen@synopsys.com (mailing list archive)
State Not Applicable
Headers show
Series usb: gadget: f_tcm: Enhance UASP driver | expand

Commit Message

Thinh Nguyen July 19, 2022, 1:27 a.m. UTC
Don't just wait for the data write completion and execute the target
command. We need to verify if the request completed successfully and not
just sending invalid data. The verification is done in the write request
completion routine, so we can just run target_execute_cmd() there. The
wait for the data write is not needed.

Signed-off-by: Thinh Nguyen <Thinh.Nguyen@synopsys.com>
---
 Changes in v2:
 - None

 drivers/usb/gadget/function/f_tcm.c | 8 +-------
 drivers/usb/gadget/function/tcm.h   | 1 -
 2 files changed, 1 insertion(+), 8 deletions(-)

Comments

Sebastian Andrzej Siewior Aug. 26, 2022, 7 a.m. UTC | #1
On 2022-07-18 18:27:12 [-0700], Thinh Nguyen wrote:
> index 6fea80afe2d7..ec83f2f9a858 100644
> --- a/drivers/usb/gadget/function/f_tcm.c
> +++ b/drivers/usb/gadget/function/f_tcm.c
> @@ -955,7 +949,7 @@ static void usbg_data_write_cmpl(struct usb_ep *ep, struct usb_request *req)
>  				se_cmd->data_length);
>  	}
>  
> -	complete(&cmd->write_complete);
> +	target_execute_cmd(se_cmd);

usbg_data_write_cmpl() is invoked from interrupt service routing which
may run with disabled interrupts. From looking at target_execute_cmd():
| void target_execute_cmd(struct se_cmd *cmd)
| {
…
|         spin_lock_irq(&cmd->t_state_lock);
…
|         spin_unlock_irq(&cmd->t_state_lock);
…
| }

which means interrupts will remain open after leaving
target_execute_cmd(). Now, why didn't the WARN_ONCE() in
__handle_irq_event_percpu() trigger? Am I missing something?

>  	return;

Sebastian
Thinh Nguyen Aug. 26, 2022, 6:37 p.m. UTC | #2
On Fri, Aug 26, 2022, Sebastian Andrzej Siewior wrote:
> On 2022-07-18 18:27:12 [-0700], Thinh Nguyen wrote:
> > index 6fea80afe2d7..ec83f2f9a858 100644
> > --- a/drivers/usb/gadget/function/f_tcm.c
> > +++ b/drivers/usb/gadget/function/f_tcm.c
> > @@ -955,7 +949,7 @@ static void usbg_data_write_cmpl(struct usb_ep *ep, struct usb_request *req)
> >  				se_cmd->data_length);
> >  	}
> >  
> > -	complete(&cmd->write_complete);
> > +	target_execute_cmd(se_cmd);
> 
> usbg_data_write_cmpl() is invoked from interrupt service routing which
> may run with disabled interrupts. From looking at target_execute_cmd():

It will always be called with interrupts disabled as documented in
usb_request API.

> | void target_execute_cmd(struct se_cmd *cmd)
> | {
> …
> |         spin_lock_irq(&cmd->t_state_lock);
> …
> |         spin_unlock_irq(&cmd->t_state_lock);
> …
> | }
> 
> which means interrupts will remain open after leaving
> target_execute_cmd(). Now, why didn't the WARN_ONCE() in
> __handle_irq_event_percpu() trigger? Am I missing something?
> 
> >  	return;
> 

Since target_execute_cmd() is called in usbg_data_write_cmpl(),
interrupts are still disabled.

Thanks,
Thinh
Sebastian Andrzej Siewior Aug. 29, 2022, 7:49 p.m. UTC | #3
On 2022-08-26 18:37:36 [+0000], Thinh Nguyen wrote:
> On Fri, Aug 26, 2022, Sebastian Andrzej Siewior wrote:
> > On 2022-07-18 18:27:12 [-0700], Thinh Nguyen wrote:
> > > index 6fea80afe2d7..ec83f2f9a858 100644
> > > --- a/drivers/usb/gadget/function/f_tcm.c
> > > +++ b/drivers/usb/gadget/function/f_tcm.c
> > > @@ -955,7 +949,7 @@ static void usbg_data_write_cmpl(struct usb_ep *ep, struct usb_request *req)
> > >  				se_cmd->data_length);
> > >  	}
> > >  
> > > -	complete(&cmd->write_complete);
> > > +	target_execute_cmd(se_cmd);
> > 
> > usbg_data_write_cmpl() is invoked from interrupt service routing which
> > may run with disabled interrupts. From looking at target_execute_cmd():
> 
> It will always be called with interrupts disabled as documented in
> usb_request API.
> 
> > | void target_execute_cmd(struct se_cmd *cmd)
> > | {
> > …
> > |         spin_lock_irq(&cmd->t_state_lock);
> > …
> > |         spin_unlock_irq(&cmd->t_state_lock);
> > …
> > | }
> > 
> > which means interrupts will remain open after leaving
> > target_execute_cmd(). Now, why didn't the WARN_ONCE() in
> > __handle_irq_event_percpu() trigger? Am I missing something?
> > 
> > >  	return;
> > 
> 
> Since target_execute_cmd() is called in usbg_data_write_cmpl(),
> interrupts are still disabled.

but you do realize that target_execute_cmd() will leave with enabled
interrupts and this is not desired? I _think_ this was the reason why I
ended up with the wait+complete construct instead of invoking this
function directly.
An _irqsave() in target_execute_cmd() would probably be all you need
here.

> Thanks,
> Thinh

Sebastian
Thinh Nguyen Aug. 29, 2022, 9:47 p.m. UTC | #4
On Mon, Aug 29, 2022, Sebastian Andrzej Siewior wrote:
> On 2022-08-26 18:37:36 [+0000], Thinh Nguyen wrote:
> > On Fri, Aug 26, 2022, Sebastian Andrzej Siewior wrote:
> > > On 2022-07-18 18:27:12 [-0700], Thinh Nguyen wrote:
> > > > index 6fea80afe2d7..ec83f2f9a858 100644
> > > > --- a/drivers/usb/gadget/function/f_tcm.c
> > > > +++ b/drivers/usb/gadget/function/f_tcm.c
> > > > @@ -955,7 +949,7 @@ static void usbg_data_write_cmpl(struct usb_ep *ep, struct usb_request *req)
> > > >  				se_cmd->data_length);
> > > >  	}
> > > >  
> > > > -	complete(&cmd->write_complete);
> > > > +	target_execute_cmd(se_cmd);
> > > 
> > > usbg_data_write_cmpl() is invoked from interrupt service routing which
> > > may run with disabled interrupts. From looking at target_execute_cmd():
> > 
> > It will always be called with interrupts disabled as documented in
> > usb_request API.
> > 
> > > | void target_execute_cmd(struct se_cmd *cmd)
> > > | {
> > > …
> > > |         spin_lock_irq(&cmd->t_state_lock);
> > > …
> > > |         spin_unlock_irq(&cmd->t_state_lock);
> > > …
> > > | }
> > > 
> > > which means interrupts will remain open after leaving
> > > target_execute_cmd(). Now, why didn't the WARN_ONCE() in
> > > __handle_irq_event_percpu() trigger? Am I missing something?
> > > 
> > > >  	return;
> > > 
> > 
> > Since target_execute_cmd() is called in usbg_data_write_cmpl(),
> > interrupts are still disabled.
> 
> but you do realize that target_execute_cmd() will leave with enabled
> interrupts and this is not desired? I _think_ this was the reason why I
> ended up with the wait+complete construct instead of invoking this
> function directly.
> An _irqsave() in target_execute_cmd() would probably be all you need
> here.
> 

Ok. Maybe we should make a change in the target_execute_cmd() then. It
seems unreasonable to force the caller to workaround this such as the
wait+complete construct you did (and I don't recall we have changes in
place to know/guarantee that interrupts are enabled before executing
target_execute_cmd() previously either).

For the dwc3, we masked the interrupt at this point, so interrupt won't
be asserted here.

Thanks,
Thinh
Sebastian Andrzej Siewior Aug. 30, 2022, 10:03 a.m. UTC | #5
On 2022-08-29 21:47:41 [+0000], Thinh Nguyen wrote:
> Ok. Maybe we should make a change in the target_execute_cmd() then. It
> seems unreasonable to force the caller to workaround this such as the
> wait+complete construct you did (and I don't recall we have changes in
> place to know/guarantee that interrupts are enabled before executing
> target_execute_cmd() previously either).

Sounds reasonable. Back then I wasn't sure if I'm putting all the puzzle
pieces correctly together so I preferred this over a target change I
wasn't sure was really needed. Anyway.

> For the dwc3, we masked the interrupt at this point, so interrupt won't
> be asserted here.

dwc3 has a irqrestore() after calling the routine so that will avoid the
splat. But lockdep should yell here.
Anyway, other interrupts on that CPU (timer for instance) could trigger.

> Thanks,
> Thinh

Sebastian
diff mbox series

Patch

diff --git a/drivers/usb/gadget/function/f_tcm.c b/drivers/usb/gadget/function/f_tcm.c
index 6fea80afe2d7..ec83f2f9a858 100644
--- a/drivers/usb/gadget/function/f_tcm.c
+++ b/drivers/usb/gadget/function/f_tcm.c
@@ -248,7 +248,6 @@  static int bot_send_write_request(struct usbg_cmd *cmd)
 	struct usb_gadget *gadget = fuas_to_gadget(fu);
 	int ret;
 
-	init_completion(&cmd->write_complete);
 	cmd->fu = fu;
 
 	if (!cmd->data_len) {
@@ -279,8 +278,6 @@  static int bot_send_write_request(struct usbg_cmd *cmd)
 	if (ret)
 		pr_err("%s(%d)\n", __func__, __LINE__);
 
-	wait_for_completion(&cmd->write_complete);
-	target_execute_cmd(se_cmd);
 cleanup:
 	return ret;
 }
@@ -685,7 +682,6 @@  static int uasp_send_write_request(struct usbg_cmd *cmd)
 	struct sense_iu *iu = &cmd->sense_iu;
 	int ret;
 
-	init_completion(&cmd->write_complete);
 	cmd->fu = fu;
 
 	iu->tag = cpu_to_be16(cmd->tag);
@@ -717,8 +713,6 @@  static int uasp_send_write_request(struct usbg_cmd *cmd)
 			pr_err("%s(%d)\n", __func__, __LINE__);
 	}
 
-	wait_for_completion(&cmd->write_complete);
-	target_execute_cmd(se_cmd);
 cleanup:
 	return ret;
 }
@@ -955,7 +949,7 @@  static void usbg_data_write_cmpl(struct usb_ep *ep, struct usb_request *req)
 				se_cmd->data_length);
 	}
 
-	complete(&cmd->write_complete);
+	target_execute_cmd(se_cmd);
 	return;
 
 cleanup:
diff --git a/drivers/usb/gadget/function/tcm.h b/drivers/usb/gadget/function/tcm.h
index c7e6d36afd3a..5157af1b166b 100644
--- a/drivers/usb/gadget/function/tcm.h
+++ b/drivers/usb/gadget/function/tcm.h
@@ -74,7 +74,6 @@  struct usbg_cmd {
 	struct se_cmd se_cmd;
 	void *data_buf; /* used if no sg support available */
 	struct f_uas *fu;
-	struct completion write_complete;
 	struct kref ref;
 
 	struct usb_request *req;