Re: [PATCH 34/36] usb: gadget: f_tcm: Free tags earlier

[Dmitry Bogdanov <d.bogdanov@yadro.com>]

On Thu, Jul 07, 2022 at 10:11:49AM +0000, Thinh Nguyen wrote:
> On 7/7/2022, Dmitriy Bogdanov wrote:
> > Hi Thinh,
> >
> >> Free up sbitmap index immediately once the command is completed rather
> >> than waiting for kref to call freeing tag. This keeps the sbitmap queue
> >> free quicker.
> >>
> > That is completely wrong. Sbitmap index is a place of usbg_cmd* in
> > a pre-allocated memory pool. It must not be freed until usbg_cmd*
> > is not used.
> 
> As noted in the commit message and as you can see in the code change, I
> only free it when the command is completed or no longer in used.

It IS used in the next line.
This patch is wrong.
NACK
> 
> > Signed-off-by: Thinh Nguyen <Thinh.Nguyen@synopsys.com>
> > ---
> >   drivers/usb/gadget/function/f_tcm.c | 6 ++++++
> >   1 file changed, 6 insertions(+)
> >
> > diff --git a/drivers/usb/gadget/function/f_tcm.c b/drivers/usb/gadget/function/f_tcm.c
> > index c13aa61d82aa..fa09999adda7 100644
> > --- a/drivers/usb/gadget/function/f_tcm.c
> > +++ b/drivers/usb/gadget/function/f_tcm.c
> > @@ -570,6 +570,7 @@ static void uasp_status_data_cmpl(struct usb_ep *ep, struct usb_request *req)
> >          struct usbg_cmd *cmd = req->context;
> >          struct uas_stream *stream = cmd->stream;
> >          struct f_uas *fu = cmd->fu;
> > +       struct se_session *se_sess = cmd->se_cmd.se_sess;
> >          int ret;
> >
> >          if (req->status == -ESHUTDOWN)
> > @@ -603,6 +604,8 @@ static void uasp_status_data_cmpl(struct usb_ep *ep, struct usb_request *req)
> >                  break;
> >
> >          case UASP_QUEUE_COMMAND:
> > +
> > +               target_free_tag(se_sess, &cmd->se_cmd);
> >                  transport_generic_free_cmd(&cmd->se_cmd, 0);
> >                  usb_ep_queue(fu->ep_cmd, cmd->req, GFP_ATOMIC);
> >
> > @@ -614,6 +617,7 @@ static void uasp_status_data_cmpl(struct usb_ep *ep, struct usb_request *req)
> >          return;
> >
> >   cleanup:
> > +       target_free_tag(se_sess, &cmd->se_cmd);
> >          transport_generic_free_cmd(&cmd->se_cmd, 0);
> >   }
> >
> > @@ -941,6 +945,7 @@ static void usbg_data_write_cmpl(struct usb_ep *ep, struct usb_request *req)
> >          struct se_cmd *se_cmd = &cmd->se_cmd;
> >
> >          if (req->status == -ESHUTDOWN) {
> > +               target_free_tag(se_cmd->se_sess, se_cmd);
> >                  transport_generic_free_cmd(&cmd->se_cmd, 0);
> >                  return;
> >          }
> > @@ -963,6 +968,7 @@ static void usbg_data_write_cmpl(struct usb_ep *ep, struct usb_request *req)
> >   cleanup:
> >          /* Command was aborted */
> >          if (cmd->state == UASP_QUEUE_COMMAND) {
> > +               target_free_tag(se_cmd->se_sess, se_cmd);
> >                  transport_generic_free_cmd(se_cmd, 0);
> >                  return;
> >          }
>