* [PATCH v3 24/28] usb: gadget: f_tcm: Check overlapped command
2024-12-11 0:31 [PATCH v3 00/28] usb: gadget: f_tcm: Enhance UASP driver Thinh Nguyen
@ 2024-12-11 0:33 ` Thinh Nguyen
2024-12-19 13:47 ` Dan Carpenter
0 siblings, 1 reply; 5+ messages in thread
From: Thinh Nguyen @ 2024-12-11 0:33 UTC (permalink / raw)
To: Greg Kroah-Hartman, Thinh Nguyen, Sebastian Andrzej Siewior
Cc: linux-usb@vger.kernel.org, Homura Akemi
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(-)
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;
--
2.28.0
^ permalink raw reply related [flat|nested] 5+ messages in thread
* Re: [PATCH v3 24/28] usb: gadget: f_tcm: Check overlapped command
@ 2024-12-19 13:27 kernel test robot
0 siblings, 0 replies; 5+ messages in thread
From: kernel test robot @ 2024-12-19 13:27 UTC (permalink / raw)
To: oe-kbuild; +Cc: lkp, Dan Carpenter
BCC: lkp@intel.com
CC: oe-kbuild-all@lists.linux.dev
In-Reply-To: <6bffc2903d0cd1e7c7afca837053a48e883d8903.1733876548.git.Thinh.Nguyen@synopsys.com>
References: <6bffc2903d0cd1e7c7afca837053a48e883d8903.1733876548.git.Thinh.Nguyen@synopsys.com>
TO: Thinh Nguyen <Thinh.Nguyen@synopsys.com>
TO: "Greg Kroah-Hartman" <gregkh@linuxfoundation.org>
TO: Thinh Nguyen <Thinh.Nguyen@synopsys.com>
TO: Sebastian Andrzej Siewior <bigeasy@linutronix.de>
CC: "linux-usb@vger.kernel.org" <linux-usb@vger.kernel.org>
CC: Homura Akemi <a1134123566@gmail.com>
Hi Thinh,
kernel test robot noticed the following build warnings:
[auto build test WARNING on d8d936c51388442f769a81e512b505dcf87c6a51]
url: https://github.com/intel-lab-lkp/linux/commits/Thinh-Nguyen/usb-gadget-f_tcm-Don-t-free-command-immediately/20241211-092317
base: d8d936c51388442f769a81e512b505dcf87c6a51
patch link: https://lore.kernel.org/r/6bffc2903d0cd1e7c7afca837053a48e883d8903.1733876548.git.Thinh.Nguyen%40synopsys.com
patch subject: [PATCH v3 24/28] usb: gadget: f_tcm: Check overlapped command
:::::: branch date: 9 days ago
:::::: commit date: 9 days ago
config: nios2-randconfig-r071-20241219 (https://download.01.org/0day-ci/archive/20241219/202412192132.XB16SilM-lkp@intel.com/config)
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 <error27@gmail.com>
| Closes: https://lore.kernel.org/r/202412192132.XB16SilM-lkp@intel.com/
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
08a1cb0f65fde6 Andrzej Pietrasiewicz 2015-12-11 1226
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)) {
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;
7735c10c74d903 Thinh Nguyen 2024-12-11 1309 usbg_aborted_task(&active_cmd->se_cmd);
7735c10c74d903 Thinh Nguyen 2024-12-11 1310
7735c10c74d903 Thinh Nguyen 2024-12-11 1311 /* Send the response after the transfer is aborted. */
7735c10c74d903 Thinh Nguyen 2024-12-11 1312 return;
7735c10c74d903 Thinh Nguyen 2024-12-11 1313 }
7735c10c74d903 Thinh Nguyen 2024-12-11 1314
287b3d115e5351 Thinh Nguyen 2024-12-11 1315 uasp_send_tm_response(cmd);
287b3d115e5351 Thinh Nguyen 2024-12-11 1316 }
287b3d115e5351 Thinh Nguyen 2024-12-11 1317
--
0-DAY CI Kernel Test Service
https://github.com/intel/lkp-tests/wiki
^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: [PATCH v3 24/28] usb: gadget: f_tcm: Check overlapped command
2024-12-11 0:33 ` [PATCH v3 24/28] usb: gadget: f_tcm: Check overlapped command Thinh Nguyen
@ 2024-12-19 13:47 ` Dan Carpenter
2024-12-20 2:31 ` Thinh Nguyen
0 siblings, 1 reply; 5+ messages in thread
From: Dan Carpenter @ 2024-12-19 13:47 UTC (permalink / raw)
To: oe-kbuild, Thinh Nguyen, Greg Kroah-Hartman,
Sebastian Andrzej Siewior
Cc: lkp, oe-kbuild-all, linux-usb@vger.kernel.org, Homura Akemi
Hi Thinh,
kernel test robot noticed the following build warnings:
url: https://github.com/intel-lab-lkp/linux/commits/Thinh-Nguyen/usb-gadget-f_tcm-Don-t-free-command-immediately/20241211-092317
base: d8d936c51388442f769a81e512b505dcf87c6a51
patch link: https://lore.kernel.org/r/6bffc2903d0cd1e7c7afca837053a48e883d8903.1733876548.git.Thinh.Nguyen%40synopsys.com
patch subject: [PATCH v3 24/28] usb: gadget: f_tcm: Check overlapped command
config: nios2-randconfig-r071-20241219 (https://download.01.org/0day-ci/archive/20241219/202412192132.XB16SilM-lkp@intel.com/config)
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://lore.kernel.org/r/202412192132.XB16SilM-lkp@intel.com/
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
as:
if (!stream || !active_cmd || active_cmd->tag != cmd->tag)) {
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.
7735c10c74d903 Thinh Nguyen 2024-12-11 1309 usbg_aborted_task(&active_cmd->se_cmd);
7735c10c74d903 Thinh Nguyen 2024-12-11 1310
7735c10c74d903 Thinh Nguyen 2024-12-11 1311 /* Send the response after the transfer is aborted. */
7735c10c74d903 Thinh Nguyen 2024-12-11 1312 return;
7735c10c74d903 Thinh Nguyen 2024-12-11 1313 }
7735c10c74d903 Thinh Nguyen 2024-12-11 1314
287b3d115e5351 Thinh Nguyen 2024-12-11 1315 uasp_send_tm_response(cmd);
287b3d115e5351 Thinh Nguyen 2024-12-11 1316 }
--
0-DAY CI Kernel Test Service
https://github.com/intel/lkp-tests/wiki
^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: [PATCH v3 24/28] usb: gadget: f_tcm: Check overlapped command
2024-12-19 13:47 ` Dan Carpenter
@ 2024-12-20 2:31 ` Thinh Nguyen
2025-01-06 8:01 ` Dan Carpenter
0 siblings, 1 reply; 5+ messages in thread
From: Thinh Nguyen @ 2024-12-20 2:31 UTC (permalink / raw)
To: Dan Carpenter
Cc: oe-kbuild@lists.linux.dev, Thinh Nguyen, Greg Kroah-Hartman,
Sebastian Andrzej Siewior, lkp@intel.com,
oe-kbuild-all@lists.linux.dev, linux-usb@vger.kernel.org,
Homura Akemi
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
^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: [PATCH v3 24/28] usb: gadget: f_tcm: Check overlapped command
2024-12-20 2:31 ` Thinh Nguyen
@ 2025-01-06 8:01 ` Dan Carpenter
0 siblings, 0 replies; 5+ messages in thread
From: Dan Carpenter @ 2025-01-06 8:01 UTC (permalink / raw)
To: Thinh Nguyen
Cc: oe-kbuild@lists.linux.dev, Greg Kroah-Hartman,
Sebastian Andrzej Siewior, lkp@intel.com,
oe-kbuild-all@lists.linux.dev, linux-usb@vger.kernel.org,
Homura Akemi
Sorry for the delayed response. I was on vacation.
On Fri, Dec 20, 2024 at 02:31:20AM +0000, Thinh Nguyen wrote:
> On Thu, Dec 19, 2024, Dan Carpenter wrote:
> > Hi Thinh,
> >
> > kernel test robot noticed the following build warnings:
> >
> > url: https://github.com/intel-lab-lkp/linux/commits/Thinh-Nguyen/usb-gadget-f_tcm-Don-t-free-command-immediately/20241211-092317
> > base: d8d936c51388442f769a81e512b505dcf87c6a51
> > patch link: https://lore.kernel.org/r/6bffc2903d0cd1e7c7afca837053a48e883d8903.1733876548.git.Thinh.Nguyen%40synopsys.com
> > patch subject: [PATCH v3 24/28] usb: gadget: f_tcm: Check overlapped command
> > config: nios2-randconfig-r071-20241219 (https://download.01.org/0day-ci/archive/20241219/202412192132.XB16SilM-lkp@intel.com/config )
> > 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://lore.kernel.org/r/202412192132.XB16SilM-lkp@intel.com/
> >
> > 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.
Yes. Sorry, I was unclear. That's what I meant. We could write it
as:
if (!stream || active_cmd->tag != cmd->tag) {
There is no need to check if active_cmd is non-NULL since we know that
stream is non-NULL. However, if we DO check it, then we should check
it consistently everywhere.
>
> > 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?
This is not a run time bug, yes. It's just an inconsistent NULL check,
but the NULL check is not necessary so that's not a problem. I don't
have a vote on how you merge it. ;) You can do that however you want.
regards,
dan carpenter
^ permalink raw reply [flat|nested] 5+ messages in thread
end of thread, other threads:[~2025-01-06 8:01 UTC | newest]
Thread overview: 5+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2024-12-19 13:27 [PATCH v3 24/28] usb: gadget: f_tcm: Check overlapped command kernel test robot
-- strict thread matches above, loose matches on Subject: below --
2024-12-11 0:31 [PATCH v3 00/28] usb: gadget: f_tcm: Enhance UASP driver Thinh Nguyen
2024-12-11 0:33 ` [PATCH v3 24/28] usb: gadget: f_tcm: Check overlapped command Thinh Nguyen
2024-12-19 13:47 ` Dan Carpenter
2024-12-20 2:31 ` Thinh Nguyen
2025-01-06 8:01 ` Dan Carpenter
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.