| Message ID | 6bffc2903d0cd1e7c7afca837053a48e883d8903.1733876548.git.Thinh.Nguyen@synopsys.com |
|---|---|
| State | New |
| Headers | show |
| Series | [v3,01/28] usb: gadget: f_tcm: Don't free command immediately | expand |
On Thu, Dec 19, 2024, Dan Carpenter wrote: > Hi Thinh, > > kernel test robot noticed the following build warnings: > > url: https://urldefense.com/v3/__https://github.com/intel-lab-lkp/linux/commits/Thinh-Nguyen/usb-gadget-f_tcm-Don-t-free-command-immediately/20241211-092317__;!!A4F2R9G_pg!YKeQa8JmJaKHAak1XzUO0sLWgipnVS9vCGr4PiZb8TEMwYAnaMG4XVSZ3aeoCV-54D_6YX6ylxj93-NYCMU04FiCU0xX$ > base: d8d936c51388442f769a81e512b505dcf87c6a51 > patch link: https://urldefense.com/v3/__https://lore.kernel.org/r/6bffc2903d0cd1e7c7afca837053a48e883d8903.1733876548.git.Thinh.Nguyen*40synopsys.com__;JQ!!A4F2R9G_pg!YKeQa8JmJaKHAak1XzUO0sLWgipnVS9vCGr4PiZb8TEMwYAnaMG4XVSZ3aeoCV-54D_6YX6ylxj93-NYCMU04BGCyVAg$ > patch subject: [PATCH v3 24/28] usb: gadget: f_tcm: Check overlapped command > config: nios2-randconfig-r071-20241219 (https://urldefense.com/v3/__https://download.01.org/0day-ci/archive/20241219/202412192132.XB16SilM-lkp@intel.com/config__;!!A4F2R9G_pg!YKeQa8JmJaKHAak1XzUO0sLWgipnVS9vCGr4PiZb8TEMwYAnaMG4XVSZ3aeoCV-54D_6YX6ylxj93-NYCMU04JujRj-r$ ) > compiler: nios2-linux-gcc (GCC) 14.2.0 > > If you fix the issue in a separate patch/commit (i.e. not just a new version of > the same patch/commit), kindly add following tags > | Reported-by: kernel test robot <lkp@intel.com> > | Reported-by: Dan Carpenter <dan.carpenter@linaro.org> > | Closes: https://urldefense.com/v3/__https://lore.kernel.org/r/202412192132.XB16SilM-lkp@intel.com/__;!!A4F2R9G_pg!YKeQa8JmJaKHAak1XzUO0sLWgipnVS9vCGr4PiZb8TEMwYAnaMG4XVSZ3aeoCV-54D_6YX6ylxj93-NYCMU04FWUkKjj$ > > smatch warnings: > drivers/usb/gadget/function/f_tcm.c:1308 usbg_cmd_work() error: we previously assumed 'active_cmd' could be null (see line 1265) > > vim +/active_cmd +1308 drivers/usb/gadget/function/f_tcm.c > > 287b3d115e5351 Thinh Nguyen 2024-12-11 1227 static void usbg_cmd_work(struct work_struct *work) > 287b3d115e5351 Thinh Nguyen 2024-12-11 1228 { > 287b3d115e5351 Thinh Nguyen 2024-12-11 1229 struct usbg_cmd *cmd = container_of(work, struct usbg_cmd, work); > 287b3d115e5351 Thinh Nguyen 2024-12-11 1230 > 287b3d115e5351 Thinh Nguyen 2024-12-11 1231 /* > 287b3d115e5351 Thinh Nguyen 2024-12-11 1232 * Failure is detected by f_tcm here. Skip submitting the command to the > 287b3d115e5351 Thinh Nguyen 2024-12-11 1233 * target core if we already know the failing response and send the usb > 287b3d115e5351 Thinh Nguyen 2024-12-11 1234 * response to the host directly. > 287b3d115e5351 Thinh Nguyen 2024-12-11 1235 */ > 287b3d115e5351 Thinh Nguyen 2024-12-11 1236 if (cmd->tmr_rsp != RC_RESPONSE_UNKNOWN) > 287b3d115e5351 Thinh Nguyen 2024-12-11 1237 goto skip; > 287b3d115e5351 Thinh Nguyen 2024-12-11 1238 > 287b3d115e5351 Thinh Nguyen 2024-12-11 1239 if (cmd->tmr_func) > 287b3d115e5351 Thinh Nguyen 2024-12-11 1240 usbg_submit_tmr(cmd); > 287b3d115e5351 Thinh Nguyen 2024-12-11 1241 else > 287b3d115e5351 Thinh Nguyen 2024-12-11 1242 usbg_submit_cmd(cmd); > 287b3d115e5351 Thinh Nguyen 2024-12-11 1243 > 287b3d115e5351 Thinh Nguyen 2024-12-11 1244 return; > 287b3d115e5351 Thinh Nguyen 2024-12-11 1245 > 287b3d115e5351 Thinh Nguyen 2024-12-11 1246 skip: > 7735c10c74d903 Thinh Nguyen 2024-12-11 1247 if (cmd->tmr_rsp == RC_OVERLAPPED_TAG) { > 7735c10c74d903 Thinh Nguyen 2024-12-11 1248 struct f_uas *fu = cmd->fu; > 7735c10c74d903 Thinh Nguyen 2024-12-11 1249 struct se_session *se_sess; > 7735c10c74d903 Thinh Nguyen 2024-12-11 1250 struct uas_stream *stream = NULL; > 7735c10c74d903 Thinh Nguyen 2024-12-11 1251 struct hlist_node *tmp; > 7735c10c74d903 Thinh Nguyen 2024-12-11 1252 struct usbg_cmd *active_cmd = NULL; > 7735c10c74d903 Thinh Nguyen 2024-12-11 1253 > 7735c10c74d903 Thinh Nguyen 2024-12-11 1254 se_sess = cmd->fu->tpg->tpg_nexus->tvn_se_sess; > 7735c10c74d903 Thinh Nguyen 2024-12-11 1255 > 7735c10c74d903 Thinh Nguyen 2024-12-11 1256 hash_for_each_possible_safe(fu->stream_hash, stream, tmp, node, cmd->tag) { > 7735c10c74d903 Thinh Nguyen 2024-12-11 1257 int i = stream - &fu->stream[0]; > 7735c10c74d903 Thinh Nguyen 2024-12-11 1258 > 7735c10c74d903 Thinh Nguyen 2024-12-11 1259 active_cmd = &((struct usbg_cmd *)se_sess->sess_cmd_map)[i]; > 7735c10c74d903 Thinh Nguyen 2024-12-11 1260 if (active_cmd->tag == cmd->tag) > 7735c10c74d903 Thinh Nguyen 2024-12-11 1261 break; > 7735c10c74d903 Thinh Nguyen 2024-12-11 1262 } > 7735c10c74d903 Thinh Nguyen 2024-12-11 1263 > 7735c10c74d903 Thinh Nguyen 2024-12-11 1264 /* Sanity check */ > 7735c10c74d903 Thinh Nguyen 2024-12-11 @1265 if (!stream || (active_cmd && active_cmd->tag != cmd->tag)) { > > Testing for !stream is sufficient. Another option would be to write this Just testing for !stream is sufficient to know whether active_cmd is NULL, but we still need to check for matching tag also. > as: > if (!stream || !active_cmd || active_cmd->tag != cmd->tag)) { Perhaps we can just do this: if (!active_cmd || active_cmd->tag != cmd->tag)) { If active_cmd is NULL, then the stream variable must also be NULL. This may not be obvious. > > 7735c10c74d903 Thinh Nguyen 2024-12-11 1266 usbg_submit_command(cmd->fu, cmd->req); > 7735c10c74d903 Thinh Nguyen 2024-12-11 1267 return; > 7735c10c74d903 Thinh Nguyen 2024-12-11 1268 } > 7735c10c74d903 Thinh Nguyen 2024-12-11 1269 > 7735c10c74d903 Thinh Nguyen 2024-12-11 1270 reinit_completion(&stream->cmd_completion); > 7735c10c74d903 Thinh Nguyen 2024-12-11 1271 > 7735c10c74d903 Thinh Nguyen 2024-12-11 1272 /* > 7735c10c74d903 Thinh Nguyen 2024-12-11 1273 * A UASP command consists of the command, data, and status > 7735c10c74d903 Thinh Nguyen 2024-12-11 1274 * stages, each operating sequentially from different endpoints. > 7735c10c74d903 Thinh Nguyen 2024-12-11 1275 * > 7735c10c74d903 Thinh Nguyen 2024-12-11 1276 * Each USB endpoint operates independently, and depending on > 7735c10c74d903 Thinh Nguyen 2024-12-11 1277 * hardware implementation, a completion callback for a transfer > 7735c10c74d903 Thinh Nguyen 2024-12-11 1278 * from one endpoint may not reflect the order of completion on > 7735c10c74d903 Thinh Nguyen 2024-12-11 1279 * the wire. This is particularly true for devices with > 7735c10c74d903 Thinh Nguyen 2024-12-11 1280 * endpoints that have independent interrupts and event buffers. > 7735c10c74d903 Thinh Nguyen 2024-12-11 1281 * > 7735c10c74d903 Thinh Nguyen 2024-12-11 1282 * The driver must still detect misbehaving hosts and respond > 7735c10c74d903 Thinh Nguyen 2024-12-11 1283 * with an overlap status. To reduce false overlap failures, > 7735c10c74d903 Thinh Nguyen 2024-12-11 1284 * allow the active and matching stream ID a brief 1ms to > 7735c10c74d903 Thinh Nguyen 2024-12-11 1285 * complete before responding with an overlap command failure. > 7735c10c74d903 Thinh Nguyen 2024-12-11 1286 * Overlap failure should be rare. > 7735c10c74d903 Thinh Nguyen 2024-12-11 1287 */ > 7735c10c74d903 Thinh Nguyen 2024-12-11 1288 wait_for_completion_timeout(&stream->cmd_completion, msecs_to_jiffies(1)); > 7735c10c74d903 Thinh Nguyen 2024-12-11 1289 > 7735c10c74d903 Thinh Nguyen 2024-12-11 1290 /* If the previous stream is completed, retry the command. */ > 7735c10c74d903 Thinh Nguyen 2024-12-11 1291 if (!hash_hashed(&stream->node)) { > 7735c10c74d903 Thinh Nguyen 2024-12-11 1292 usbg_submit_command(cmd->fu, cmd->req); > 7735c10c74d903 Thinh Nguyen 2024-12-11 1293 return; > 7735c10c74d903 Thinh Nguyen 2024-12-11 1294 } > 7735c10c74d903 Thinh Nguyen 2024-12-11 1295 > 7735c10c74d903 Thinh Nguyen 2024-12-11 1296 /* > 7735c10c74d903 Thinh Nguyen 2024-12-11 1297 * The command isn't submitted to the target core, so we're safe > 7735c10c74d903 Thinh Nguyen 2024-12-11 1298 * to remove the bitmap index from the session tag pool. > 7735c10c74d903 Thinh Nguyen 2024-12-11 1299 */ > 7735c10c74d903 Thinh Nguyen 2024-12-11 1300 sbitmap_queue_clear(&se_sess->sess_tag_pool, > 7735c10c74d903 Thinh Nguyen 2024-12-11 1301 cmd->se_cmd.map_tag, > 7735c10c74d903 Thinh Nguyen 2024-12-11 1302 cmd->se_cmd.map_cpu); > 7735c10c74d903 Thinh Nguyen 2024-12-11 1303 > 7735c10c74d903 Thinh Nguyen 2024-12-11 1304 /* > 7735c10c74d903 Thinh Nguyen 2024-12-11 1305 * Overlap command tag detected. Cancel any pending transfer of > 7735c10c74d903 Thinh Nguyen 2024-12-11 1306 * the command submitted to target core. > 7735c10c74d903 Thinh Nguyen 2024-12-11 1307 */ > 7735c10c74d903 Thinh Nguyen 2024-12-11 @1308 active_cmd->tmr_rsp = RC_OVERLAPPED_TAG; > > The inconsistent NULL check triggers a warning here. > We already check for !stream prior, so I didn't check for active_cmd here. This is more of a consistency issue. If possible and if needed, we can make this more consistent after the merge? Thanks, Thinh
diff --git a/drivers/usb/gadget/function/f_tcm.c b/drivers/usb/gadget/function/f_tcm.c index 3e04ce40a4a0..0c7a41568f40 100644 --- a/drivers/usb/gadget/function/f_tcm.c +++ b/drivers/usb/gadget/function/f_tcm.c @@ -685,12 +685,25 @@ static void uasp_status_data_cmpl(struct usb_ep *ep, struct usb_request *req) break; case UASP_QUEUE_COMMAND: + /* + * Overlapped command detected and cancelled. + * So send overlapped attempted status. + */ + if (cmd->tmr_rsp == RC_OVERLAPPED_TAG && + req->status == -ECONNRESET) { + uasp_send_tm_response(cmd); + return; + } + + hash_del(&stream->node); + /* * If no command submitted to target core here, just free the * bitmap index. This is for the cases where f_tcm handles * status response instead of the target core. */ - if (cmd->tmr_rsp != RC_RESPONSE_UNKNOWN) { + if (cmd->tmr_rsp != RC_OVERLAPPED_TAG && + cmd->tmr_rsp != RC_RESPONSE_UNKNOWN) { struct se_session *se_sess; se_sess = fu->tpg->tpg_nexus->tvn_se_sess; @@ -702,6 +715,7 @@ static void uasp_status_data_cmpl(struct usb_ep *ep, struct usb_request *req) } usb_ep_queue(fu->ep_cmd, cmd->req, GFP_ATOMIC); + complete(&stream->cmd_completion); break; default: @@ -710,6 +724,7 @@ static void uasp_status_data_cmpl(struct usb_ep *ep, struct usb_request *req) return; cleanup: + hash_del(&stream->node); transport_generic_free_cmd(&cmd->se_cmd, 0); } @@ -842,6 +857,8 @@ static void uasp_cmd_complete(struct usb_ep *ep, struct usb_request *req) static int uasp_alloc_stream_res(struct f_uas *fu, struct uas_stream *stream) { + init_completion(&stream->cmd_completion); + stream->req_in = usb_ep_alloc_request(fu->ep_in, GFP_KERNEL); if (!stream->req_in) goto out; @@ -1046,6 +1063,9 @@ static void usbg_data_write_cmpl(struct usb_ep *ep, struct usb_request *req) cmd->state = UASP_QUEUE_COMMAND; if (req->status == -ESHUTDOWN) { + struct uas_stream *stream = &cmd->fu->stream[se_cmd->map_tag]; + + hash_del(&stream->node); target_put_sess_cmd(se_cmd); transport_generic_free_cmd(&cmd->se_cmd, 0); return; @@ -1069,6 +1089,14 @@ static void usbg_data_write_cmpl(struct usb_ep *ep, struct usb_request *req) cleanup: target_put_sess_cmd(se_cmd); + + /* Command was aborted due to overlapped tag */ + if (cmd->state == UASP_QUEUE_COMMAND && + cmd->tmr_rsp == RC_OVERLAPPED_TAG) { + uasp_send_tm_response(cmd); + return; + } + transport_send_check_condition_and_sense(se_cmd, TCM_CHECK_CONDITION_ABORT_CMD, 0); } @@ -1137,6 +1165,8 @@ static int usbg_send_read_response(struct se_cmd *se_cmd) return uasp_send_read_response(cmd); } +static void usbg_aborted_task(struct se_cmd *se_cmd); + static void usbg_submit_tmr(struct usbg_cmd *cmd) { struct se_session *se_sess; @@ -1214,6 +1244,74 @@ static void usbg_cmd_work(struct work_struct *work) return; skip: + if (cmd->tmr_rsp == RC_OVERLAPPED_TAG) { + struct f_uas *fu = cmd->fu; + struct se_session *se_sess; + struct uas_stream *stream = NULL; + struct hlist_node *tmp; + struct usbg_cmd *active_cmd = NULL; + + se_sess = cmd->fu->tpg->tpg_nexus->tvn_se_sess; + + hash_for_each_possible_safe(fu->stream_hash, stream, tmp, node, cmd->tag) { + int i = stream - &fu->stream[0]; + + active_cmd = &((struct usbg_cmd *)se_sess->sess_cmd_map)[i]; + if (active_cmd->tag == cmd->tag) + break; + } + + /* Sanity check */ + if (!stream || (active_cmd && active_cmd->tag != cmd->tag)) { + usbg_submit_command(cmd->fu, cmd->req); + return; + } + + reinit_completion(&stream->cmd_completion); + + /* + * A UASP command consists of the command, data, and status + * stages, each operating sequentially from different endpoints. + * + * Each USB endpoint operates independently, and depending on + * hardware implementation, a completion callback for a transfer + * from one endpoint may not reflect the order of completion on + * the wire. This is particularly true for devices with + * endpoints that have independent interrupts and event buffers. + * + * The driver must still detect misbehaving hosts and respond + * with an overlap status. To reduce false overlap failures, + * allow the active and matching stream ID a brief 1ms to + * complete before responding with an overlap command failure. + * Overlap failure should be rare. + */ + wait_for_completion_timeout(&stream->cmd_completion, msecs_to_jiffies(1)); + + /* If the previous stream is completed, retry the command. */ + if (!hash_hashed(&stream->node)) { + usbg_submit_command(cmd->fu, cmd->req); + return; + } + + /* + * The command isn't submitted to the target core, so we're safe + * to remove the bitmap index from the session tag pool. + */ + sbitmap_queue_clear(&se_sess->sess_tag_pool, + cmd->se_cmd.map_tag, + cmd->se_cmd.map_cpu); + + /* + * Overlap command tag detected. Cancel any pending transfer of + * the command submitted to target core. + */ + active_cmd->tmr_rsp = RC_OVERLAPPED_TAG; + usbg_aborted_task(&active_cmd->se_cmd); + + /* Send the response after the transfer is aborted. */ + return; + } + uasp_send_tm_response(cmd); } @@ -1247,6 +1345,8 @@ static int usbg_submit_command(struct f_uas *fu, struct usb_request *req) struct usbg_cmd *cmd; struct usbg_tpg *tpg = fu->tpg; struct tcm_usbg_nexus *tv_nexus; + struct uas_stream *stream; + struct hlist_node *tmp; struct command_iu *cmd_iu; u32 cmd_len; u16 scsi_tag; @@ -1282,6 +1382,23 @@ static int usbg_submit_command(struct f_uas *fu, struct usb_request *req) goto skip; } + hash_for_each_possible_safe(fu->stream_hash, stream, tmp, node, scsi_tag) { + struct usbg_cmd *active_cmd; + struct se_session *se_sess; + int i = stream - &fu->stream[0]; + + se_sess = cmd->fu->tpg->tpg_nexus->tvn_se_sess; + active_cmd = &((struct usbg_cmd *)se_sess->sess_cmd_map)[i]; + + if (active_cmd->tag == scsi_tag) { + cmd->tmr_rsp = RC_OVERLAPPED_TAG; + goto skip; + } + } + + stream = &fu->stream[cmd->se_cmd.map_tag]; + hash_add(fu->stream_hash, &stream->node, scsi_tag); + if (iu->iu_id == IU_ID_TASK_MGMT) { struct task_mgmt_iu *tm_iu; @@ -1293,6 +1410,7 @@ static int usbg_submit_command(struct f_uas *fu, struct usb_request *req) cmd_len = (cmd_iu->len & ~0x3) + 16; if (cmd_len > USBG_MAX_CMD) { target_free_tag(tv_nexus->tvn_se_sess, &cmd->se_cmd); + hash_del(&stream->node); return -EINVAL; } memcpy(cmd->cmd_buf, cmd_iu->cdb, cmd_len); @@ -1443,6 +1561,7 @@ static void usbg_release_cmd(struct se_cmd *se_cmd) se_cmd); struct se_session *se_sess = se_cmd->se_sess; + cmd->tag = 0; kfree(cmd->data_buf); target_free_tag(se_sess, se_cmd); } @@ -2467,6 +2586,8 @@ static struct usb_function *tcm_alloc(struct usb_function_instance *fi) fu->function.disable = tcm_disable; fu->function.free_func = tcm_free; fu->tpg = tpg_instances[i].tpg; + + hash_init(fu->stream_hash); mutex_unlock(&tpg_instances_lock); return &fu->function; diff --git a/drivers/usb/gadget/function/tcm.h b/drivers/usb/gadget/function/tcm.h index d37358f09819..f6d6c86d10b3 100644 --- a/drivers/usb/gadget/function/tcm.h +++ b/drivers/usb/gadget/function/tcm.h @@ -4,6 +4,7 @@ #include <linux/kref.h> /* #include <linux/usb/uas.h> */ +#include <linux/hashtable.h> #include <linux/usb/composite.h> #include <linux/usb/uas.h> #include <linux/usb/storage.h> @@ -103,6 +104,9 @@ struct uas_stream { struct usb_request *req_in; struct usb_request *req_out; struct usb_request *req_status; + + struct completion cmd_completion; + struct hlist_node node; }; struct usbg_cdb { @@ -135,6 +139,7 @@ struct f_uas { struct usb_ep *ep_status; struct usb_ep *ep_cmd; struct uas_stream stream[USBG_NUM_CMDS]; + DECLARE_HASHTABLE(stream_hash, UASP_SS_EP_COMP_LOG_STREAMS); /* BOT */ struct bot_status bot_status;
If there's an overlapped command tag, cancel the command and respond with RC_OVERLAPPED_TAG to host. Signed-off-by: Thinh Nguyen <Thinh.Nguyen@synopsys.com> --- drivers/usb/gadget/function/f_tcm.c | 123 +++++++++++++++++++++++++++- drivers/usb/gadget/function/tcm.h | 5 ++ 2 files changed, 127 insertions(+), 1 deletion(-)