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@xxxxxxxxx/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@xxxxxxxxx> > | Reported-by: Dan Carpenter <dan.carpenter@xxxxxxxxxx> > | Closes: https://urldefense.com/v3/__https://lore.kernel.org/r/202412192132.XB16SilM-lkp@xxxxxxxxx/__;!!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
