diff mbox series

[v2,16/25] usb: gadget: f_tcm: Update state on data write

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

Commit Message

Thinh Nguyen July 19, 2022, 1:27 a.m. UTC
When preparing request for data write state, the next state is
UASP_SEND_STATUS. When data write completes, the next state is
UASP_QUEUE_COMMAND. Without this change, the command will transition to
the wrong state.

Signed-off-by: Thinh Nguyen <Thinh.Nguyen@synopsys.com>
---
 Changes in v2:
 - Move a related change from TASK MANAGEMENT updating cmd state to
   UASP_QUEUE_COMMAND to here.

 drivers/usb/gadget/function/f_tcm.c | 4 ++++
 1 file changed, 4 insertions(+)

Comments

Sebastian Andrzej Siewior Aug. 26, 2022, 7:22 a.m. UTC | #1
On 2022-07-18 18:27:45 [-0700], Thinh Nguyen wrote:
> When preparing request for data write state, the next state is
> UASP_SEND_STATUS. When data write completes, the next state is
> UASP_QUEUE_COMMAND. Without this change, the command will transition to
> the wrong state.

Why is this needed now, what is the outcome of not having it?
My point is, was this always broken, worked by chance and broke over
time while code was changed?

> Signed-off-by: Thinh Nguyen <Thinh.Nguyen@synopsys.com>
> ---
>  Changes in v2:
>  - Move a related change from TASK MANAGEMENT updating cmd state to
>    UASP_QUEUE_COMMAND to here.
> 
>  drivers/usb/gadget/function/f_tcm.c | 4 ++++
>  1 file changed, 4 insertions(+)
> 
> diff --git a/drivers/usb/gadget/function/f_tcm.c b/drivers/usb/gadget/function/f_tcm.c
> index 1e7d29f8aecb..d7318c84af98 100644
> --- a/drivers/usb/gadget/function/f_tcm.c
> +++ b/drivers/usb/gadget/function/f_tcm.c
> @@ -934,6 +934,8 @@ static void usbg_data_write_cmpl(struct usb_ep *ep, struct usb_request *req)
>  	struct usbg_cmd *cmd = req->context;
>  	struct se_cmd *se_cmd = &cmd->se_cmd;
>  
> +	cmd->state = UASP_QUEUE_COMMAND;
> +
>  	if (req->status < 0) {
>  		pr_err("%s() state %d transfer failed\n", __func__, cmd->state);
>  		goto cleanup;
> @@ -976,6 +978,8 @@ static int usbg_prepare_w_request(struct usbg_cmd *cmd, struct usb_request *req)
>  	req->complete = usbg_data_write_cmpl;
>  	req->length = se_cmd->data_length;
>  	req->context = cmd;
> +
> +	cmd->state = UASP_SEND_STATUS;
>  	return 0;
>  }
>  

Sebastian
Thinh Nguyen Aug. 26, 2022, 7:01 p.m. UTC | #2
On Fri, Aug 26, 2022, Sebastian Andrzej Siewior wrote:
> On 2022-07-18 18:27:45 [-0700], Thinh Nguyen wrote:
> > When preparing request for data write state, the next state is
> > UASP_SEND_STATUS. When data write completes, the next state is
> > UASP_QUEUE_COMMAND. Without this change, the command will transition to
> > the wrong state.
> 
> Why is this needed now, what is the outcome of not having it?
> My point is, was this always broken, worked by chance and broke over
> time while code was changed?

This patch should've been part of the patch "usb: gadget: f_tcm: Execute
command on write completion", and maybe I should've been more clear in
the commmit message.

Previously, the f_tcm doesn't take care of the case where the queued
data could fail/aborted. It attempts to execute the command anyway.

This change here takes care of both scenarios. If the request fails, the
next state is to send status, and if the request completes successfully,
the next state is to queue a new command.

Thanks,
Thinh


> 
> > Signed-off-by: Thinh Nguyen <Thinh.Nguyen@synopsys.com>
> > ---
> >  Changes in v2:
> >  - Move a related change from TASK MANAGEMENT updating cmd state to
> >    UASP_QUEUE_COMMAND to here.
> > 
> >  drivers/usb/gadget/function/f_tcm.c | 4 ++++
> >  1 file changed, 4 insertions(+)
> > 
> > diff --git a/drivers/usb/gadget/function/f_tcm.c b/drivers/usb/gadget/function/f_tcm.c
> > index 1e7d29f8aecb..d7318c84af98 100644
> > --- a/drivers/usb/gadget/function/f_tcm.c
> > +++ b/drivers/usb/gadget/function/f_tcm.c
> > @@ -934,6 +934,8 @@ static void usbg_data_write_cmpl(struct usb_ep *ep, struct usb_request *req)
> >  	struct usbg_cmd *cmd = req->context;
> >  	struct se_cmd *se_cmd = &cmd->se_cmd;
> >  
> > +	cmd->state = UASP_QUEUE_COMMAND;
> > +
> >  	if (req->status < 0) {
> >  		pr_err("%s() state %d transfer failed\n", __func__, cmd->state);
> >  		goto cleanup;
> > @@ -976,6 +978,8 @@ static int usbg_prepare_w_request(struct usbg_cmd *cmd, struct usb_request *req)
> >  	req->complete = usbg_data_write_cmpl;
> >  	req->length = se_cmd->data_length;
> >  	req->context = cmd;
> > +
> > +	cmd->state = UASP_SEND_STATUS;
> >  	return 0;
> >  }
> >  
> 
> Sebastian
diff mbox series

Patch

diff --git a/drivers/usb/gadget/function/f_tcm.c b/drivers/usb/gadget/function/f_tcm.c
index 1e7d29f8aecb..d7318c84af98 100644
--- a/drivers/usb/gadget/function/f_tcm.c
+++ b/drivers/usb/gadget/function/f_tcm.c
@@ -934,6 +934,8 @@  static void usbg_data_write_cmpl(struct usb_ep *ep, struct usb_request *req)
 	struct usbg_cmd *cmd = req->context;
 	struct se_cmd *se_cmd = &cmd->se_cmd;
 
+	cmd->state = UASP_QUEUE_COMMAND;
+
 	if (req->status < 0) {
 		pr_err("%s() state %d transfer failed\n", __func__, cmd->state);
 		goto cleanup;
@@ -976,6 +978,8 @@  static int usbg_prepare_w_request(struct usbg_cmd *cmd, struct usb_request *req)
 	req->complete = usbg_data_write_cmpl;
 	req->length = se_cmd->data_length;
 	req->context = cmd;
+
+	cmd->state = UASP_SEND_STATUS;
 	return 0;
 }