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@xxxxxxxxxxxx>
> > ---
> > 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;
> > }
>
