| Message ID | 3fbae6ecb8e9f31807635152a377b076e86fb12e.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:28:16 [-0700], Thinh Nguyen wrote: > diff --git a/drivers/usb/gadget/function/f_tcm.c b/drivers/usb/gadget/function/f_tcm.c > index 084143213176..a10e74290664 100644 > --- a/drivers/usb/gadget/function/f_tcm.c > +++ b/drivers/usb/gadget/function/f_tcm.c > @@ -506,6 +506,22 @@ static void uasp_cleanup_old_alt(struct f_uas *fu) > uasp_free_cmdreq(fu); > } > > +static struct uas_stream *uasp_get_stream_by_tag(struct f_uas *fu, u16 tag) > +{ > + /* > + * For simplicity, we use mod operation to quickly find an in-progress > + * matching command tag to check for overlapped command. The assumption > + * is that the UASP class driver will limit to using tag id from 1 to > + * USBG_NUM_CMDS. This is based on observation from the Windows and > + * Linux UASP storage class driver behavior. If an unusual UASP class > + * driver uses a tag greater than USBG_NUM_CMDS, then this method may no > + * longer work due to possible stream id collision. In that case, we > + * need to use a proper algorithm to fetch the stream (or simply walk > + * through all active streams to check for overlap). > + */ > + return &fu->stream[tag % USBG_NUM_CMDS]; Could you please avoid the assumption what tag actually is? Please take a look at hashtable.h, hash_add(), hash_del(), hash_for_each_possible_safe() is probably all you need. That % looks efficient but gcc will try and remove the div operation which is something the hash implementation (as of hash_min()) avoids. So the only additional costs here is the additional hashtable which worth the price given that you don't assume what tag can be. Sebastian
On Fri, Aug 26, 2022, Sebastian Andrzej Siewior wrote: > On 2022-07-18 18:28:16 [-0700], Thinh Nguyen wrote: > > diff --git a/drivers/usb/gadget/function/f_tcm.c b/drivers/usb/gadget/function/f_tcm.c > > index 084143213176..a10e74290664 100644 > > --- a/drivers/usb/gadget/function/f_tcm.c > > +++ b/drivers/usb/gadget/function/f_tcm.c > > @@ -506,6 +506,22 @@ static void uasp_cleanup_old_alt(struct f_uas *fu) > > uasp_free_cmdreq(fu); > > } > > > > +static struct uas_stream *uasp_get_stream_by_tag(struct f_uas *fu, u16 tag) > > +{ > > + /* > > + * For simplicity, we use mod operation to quickly find an in-progress > > + * matching command tag to check for overlapped command. The assumption > > + * is that the UASP class driver will limit to using tag id from 1 to > > + * USBG_NUM_CMDS. This is based on observation from the Windows and > > + * Linux UASP storage class driver behavior. If an unusual UASP class > > + * driver uses a tag greater than USBG_NUM_CMDS, then this method may no > > + * longer work due to possible stream id collision. In that case, we > > + * need to use a proper algorithm to fetch the stream (or simply walk > > + * through all active streams to check for overlap). > > + */ > > + return &fu->stream[tag % USBG_NUM_CMDS]; > > Could you please avoid the assumption what tag actually is? > Please take a look at hashtable.h, hash_add(), hash_del(), > hash_for_each_possible_safe() is probably all you need. > That % looks efficient but gcc will try and remove the div operation > which is something the hash implementation (as of hash_min()) avoids. So > the only additional costs here is the additional hashtable which worth > the price given that you don't assume what tag can be. > Sure, I can look into it. Thanks, Thinh
diff --git a/drivers/usb/gadget/function/f_tcm.c b/drivers/usb/gadget/function/f_tcm.c index 084143213176..a10e74290664 100644 --- a/drivers/usb/gadget/function/f_tcm.c +++ b/drivers/usb/gadget/function/f_tcm.c @@ -506,6 +506,22 @@ static void uasp_cleanup_old_alt(struct f_uas *fu) uasp_free_cmdreq(fu); } +static struct uas_stream *uasp_get_stream_by_tag(struct f_uas *fu, u16 tag) +{ + /* + * For simplicity, we use mod operation to quickly find an in-progress + * matching command tag to check for overlapped command. The assumption + * is that the UASP class driver will limit to using tag id from 1 to + * USBG_NUM_CMDS. This is based on observation from the Windows and + * Linux UASP storage class driver behavior. If an unusual UASP class + * driver uses a tag greater than USBG_NUM_CMDS, then this method may no + * longer work due to possible stream id collision. In that case, we + * need to use a proper algorithm to fetch the stream (or simply walk + * through all active streams to check for overlap). + */ + return &fu->stream[tag % USBG_NUM_CMDS]; +} + static void uasp_status_data_cmpl(struct usb_ep *ep, struct usb_request *req); static int uasp_prepare_r_request(struct usbg_cmd *cmd) @@ -513,7 +529,7 @@ static int uasp_prepare_r_request(struct usbg_cmd *cmd) struct se_cmd *se_cmd = &cmd->se_cmd; struct f_uas *fu = cmd->fu; struct usb_gadget *gadget = fuas_to_gadget(fu); - struct uas_stream *stream = cmd->stream; + struct uas_stream *stream = uasp_get_stream_by_tag(fu, cmd->tag); if (!gadget->sg_supported) { cmd->data_buf = kmalloc(se_cmd->data_length, GFP_ATOMIC); @@ -546,7 +562,7 @@ static void uasp_prepare_status(struct usbg_cmd *cmd) { struct se_cmd *se_cmd = &cmd->se_cmd; struct sense_iu *iu = &cmd->sense_iu; - struct uas_stream *stream = cmd->stream; + struct uas_stream *stream = uasp_get_stream_by_tag(cmd->fu, cmd->tag); cmd->state = UASP_QUEUE_COMMAND; iu->iu_id = IU_ID_STATUS; @@ -568,8 +584,8 @@ static void uasp_prepare_status(struct usbg_cmd *cmd) 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 uas_stream *stream = uasp_get_stream_by_tag(fu, cmd->tag); int ret; if (req->status == -ESHUTDOWN) @@ -620,7 +636,7 @@ static void uasp_status_data_cmpl(struct usb_ep *ep, struct usb_request *req) static int uasp_send_status_response(struct usbg_cmd *cmd) { struct f_uas *fu = cmd->fu; - struct uas_stream *stream = cmd->stream; + struct uas_stream *stream = uasp_get_stream_by_tag(fu, cmd->tag); struct sense_iu *iu = &cmd->sense_iu; iu->tag = cpu_to_be16(cmd->tag); @@ -632,7 +648,7 @@ static int uasp_send_status_response(struct usbg_cmd *cmd) static int uasp_send_read_response(struct usbg_cmd *cmd) { struct f_uas *fu = cmd->fu; - struct uas_stream *stream = cmd->stream; + struct uas_stream *stream = uasp_get_stream_by_tag(fu, cmd->tag); struct sense_iu *iu = &cmd->sense_iu; int ret; @@ -675,7 +691,7 @@ static int uasp_send_read_response(struct usbg_cmd *cmd) static int uasp_send_write_request(struct usbg_cmd *cmd) { struct f_uas *fu = cmd->fu; - struct uas_stream *stream = cmd->stream; + struct uas_stream *stream = uasp_get_stream_by_tag(fu, cmd->tag); struct sense_iu *iu = &cmd->sense_iu; int ret; @@ -1285,7 +1301,7 @@ static void usbg_aborted_task(struct se_cmd *se_cmd) { struct usbg_cmd *cmd = container_of(se_cmd, struct usbg_cmd, se_cmd); struct f_uas *fu = cmd->fu; - struct uas_stream *stream = cmd->stream; + struct uas_stream *stream = uasp_get_stream_by_tag(fu, cmd->tag); int ret = 0; if (stream->req_out->status == -EINPROGRESS)
In preparation for TASK MANAGEMENT handling of overlapped command, implement uasp_get_stream_by_tag() to find the active stream/command based on a given tag. For simplicity, we use mod operation to quickly find an in-progress matching command tag to check for overlapped command. The assumption is that the UASP class driver will limit to using tag id from 1 to USBG_NUM_CMDS. This is based on observation from the Windows and Linux UASP storage class driver behavior. If an unusual UASP class driver uses a tag greater than USBG_NUM_CMDS, then this method may no longer work due to possible stream id collision. In that case, we need to use a proper algorithm to fetch the stream (or simply walk through all active streams to check for overlap). Signed-off-by: Thinh Nguyen <Thinh.Nguyen@synopsys.com> --- Changes in v2: - Splitted from TASK MANAGEMENT change drivers/usb/gadget/function/f_tcm.c | 30 ++++++++++++++++++++++------- 1 file changed, 23 insertions(+), 7 deletions(-)