| Message ID | b030d10834c13aa09bbbba7b33b1957d5ba3664c.1658192351.git.Thinh.Nguyen@synopsys.com |
|---|---|
| State | New |
| Headers | show |
| Series | usb: gadget: f_tcm: Enhance UASP driver | expand |
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
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;
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(-)