| 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 |
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
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
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
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
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 --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(-)