All of lore.kernel.org
 help / color / mirror / Atom feed
* 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
* [PATCH v3 00/28] usb: gadget: f_tcm: Enhance UASP driver
@ 2024-12-11  0:31 Thinh Nguyen
  2024-12-11  0:33 ` [PATCH v3 24/28] usb: gadget: f_tcm: Check overlapped command Thinh Nguyen
  0 siblings, 1 reply; 5+ messages in thread
From: Thinh Nguyen @ 2024-12-11  0:31 UTC (permalink / raw)
  To: Greg Kroah-Hartman, Thinh Nguyen, Nicholas Bellinger,
	Sebastian Andrzej Siewior
  Cc: linux-usb@vger.kernel.org, stable@vger.kernel.org, Homura Akemi,
	Alan Stern, Andrzej Pietrasiewicz, Christoph Hellwig

Apologies for the delay; after two years and multiple requests to resume this
series, I squeezed some time to push an update. This series applies on top of
Greg's usb-testing branch.

If possible, please help test this series and get this merged as my resources
are nil for this work.

Example Bringup Steps
=====================
To test UASP, here's an example perl script snippet to bring it up.

Note: the script was cut down and quickly rewritten, so sorry if I make
mistakes.

    my $MY_UAS_VID = xxxx;
    my $MY_UAS_PID = yyyy;
    my $SERIAL = "1234";
    my $VENDOR = "VENDOR";
    my $MY_VER = "VER";

    my $vendor_id = "my_vid";
    my $product_id = "my_pid";
    my $revision = "my_rev";

    # Must update:
    my $backing_storage = "/tmp/some_file";
    my $backing_storage_size = 1024*1024*16;
    my $use_ramdisk = 0;

    my $g = "/sys/kernel/config/usb_gadget/g1";

    system("modprobe libcomposite");
    system("modprobe usb_f_tcm");
    system("mkdir -p $g");
    system("mkdir -p $g/configs/c.1");
    system("mkdir -p $g/functions/tcm.0");
    system("mkdir -p $g/strings/0x409");
    system("mkdir -p $g/configs/c.1/strings/0x409");

    my $tp = "/sys/kernel/config/target/usb_gadget/naa.0/tpgt_1";

    my $tf;
    my $ctrl;

    if ($use_ramdisk) {
        $tf = "/sys/kernel/config/target/core/rd_mcp_0/ramdisk";
        $ctrl = 'rd_pages=524288';
    } else {
        $tf = "/sys/kernel/config/target/core/fileio_0/fileio";
        $ctrl = 'fd_dev_name=$backing_storage,fd_dev_size=$backing_storage_size,fd_async_io=1';
    }

    system("mkdir -p /etc/target");

    system("mkdir -p $tp");
    system("mkdir -p $tf");
    system("mkdir -p $tp/lun/lun_0");

    system("echo naa.0         > $tp/nexus");
    system("echo $ctrl         > $tf/control");
    system("echo 1             > $tf/attrib/emulate_ua_intlck_ctrl");
    system("echo 123           > $tf/wwn/vpd_unit_serial");
    system("echo $vendor_id    > $tf/wwn/vendor_id");
    system("echo $product_id   > $tf/wwn/product_id");
    system("echo $revision     > $tf/wwn/revision");
    system("echo 1             > $tf/enable");

    system("ln -s $tf $tp/lun/lun_0/virtual_scsi_port");
    system("echo 1             > $tp/enable");

    system("echo $MY_UAS_PID   > $g/idProduct");

    system("ln -s $g/functions/tcm.0 $g/configs/c.1");

    system("echo $MY_UAS_VID   > $g/idVendor");
    system("echo $SERIAL       > $g/strings/0x409/serialnumber");
    system("echo $VENDOR       > $g/strings/0x409/manufacturer");
    system("echo \"$MY_VER\"   > $g/strings/0x409/product");
    system("echo \"Conf 1\"    > $g/configs/c.1/strings/0x409/configuration");
    system("echo super-speed-plus > $g/max_speed");

    # Make sure the UDC is available
    system("echo $my_udc       > $g/UDC");


Target Subsystem Fixes
======================
I have eliminated unnecessary changes related to the Target subsystem and
reworked f_tcm to minimize the modifications required in the Target subsystem.
There are unimplemented Task Management Requests in the Target subsystem, but
the basic flow should still work.

Regardless, you should still need to apply at least these 2 fixes:

1) Fix Data Corruption
----------------------

Properly increment the "len" base on the command requested length instead of
the SG entry length.

If you're using File backend, then you need to fix target_core_file. If you're
using other backend such as Ramdisk, then you need a similar fix there.

---
 drivers/target/target_core_file.c | 14 ++++++++++++--
 1 file changed, 12 insertions(+), 2 deletions(-)

diff --git a/drivers/target/target_core_file.c b/drivers/target/target_core_file.c
index 2d78ef74633c..d9fc048c1734 100644
--- a/drivers/target/target_core_file.c
+++ b/drivers/target/target_core_file.c
@@ -283,7 +283,12 @@ fd_execute_rw_aio(struct se_cmd *cmd, struct scatterlist *sgl, u32 sgl_nents,
 	for_each_sg(sgl, sg, sgl_nents, i) {
 		bvec_set_page(&aio_cmd->bvecs[i], sg_page(sg), sg->length,
 			      sg->offset);
-		len += sg->length;
+		if (len + sg->length >= cmd->data_length) {
+			len = cmd->data_length;
+			break;
+		} else {
+			len += sg->length;
+		}
 	}
 
 	iov_iter_bvec(&iter, is_write, aio_cmd->bvecs, sgl_nents, len);
@@ -328,7 +333,12 @@ static int fd_do_rw(struct se_cmd *cmd, struct file *fd,
 
 	for_each_sg(sgl, sg, sgl_nents, i) {
 		bvec_set_page(&bvec[i], sg_page(sg), sg->length, sg->offset);
-		len += sg->length;
+		if (len + sg->length >= data_length) {
+			len = data_length;
+			break;
+		} else {
+			len += sg->length;
+		}
 	}
 
 	iov_iter_bvec(&iter, is_write, bvec, sgl_nents, len);
-- 


2) Fix Sense Data Length
------------------------

The transport_get_sense_buffer() and transport_copy_sense_to_cmd() take
sense data length to be the allocated sense buffer length
TRANSPORT_SENSE_BUFFER. However, the sense data length is depending on
the sense data description. Check the sense data to set the proper
cmd->scsi_sense_length.

See SPC4-r37 section 4.5.2.1.
---
 drivers/target/target_core_transport.c | 19 +++++++++++++++----
 1 file changed, 15 insertions(+), 4 deletions(-)

diff --git a/drivers/target/target_core_transport.c b/drivers/target/target_core_transport.c
index 8d8f4ad4f59e..da75d6873ab5 100644
--- a/drivers/target/target_core_transport.c
+++ b/drivers/target/target_core_transport.c
@@ -804,8 +804,6 @@ static unsigned char *transport_get_sense_buffer(struct se_cmd *cmd)
 	if (cmd->se_cmd_flags & SCF_SENT_CHECK_CONDITION)
 		return NULL;
 
-	cmd->scsi_sense_length = TRANSPORT_SENSE_BUFFER;
-
 	pr_debug("HBA_[%u]_PLUG[%s]: Requesting sense for SAM STATUS: 0x%02x\n",
 		dev->se_hba->hba_id, dev->transport->name, cmd->scsi_status);
 	return cmd->sense_buffer;
@@ -824,7 +822,13 @@ void transport_copy_sense_to_cmd(struct se_cmd *cmd, unsigned char *sense)
 	}
 
 	cmd->se_cmd_flags |= SCF_TRANSPORT_TASK_SENSE;
+
+	/* Sense data length = min sense data + additional sense data length */
+	cmd->scsi_sense_length = min_t(u16, cmd_sense_buf[7] + 8,
+				       TRANSPORT_SENSE_BUFFER);
+
 	memcpy(cmd_sense_buf, sense, cmd->scsi_sense_length);
+
 	spin_unlock_irqrestore(&cmd->t_state_lock, flags);
 }
 EXPORT_SYMBOL(transport_copy_sense_to_cmd);
@@ -3521,12 +3525,19 @@ static void translate_sense_reason(struct se_cmd *cmd, sense_reason_t reason)
 
 	cmd->se_cmd_flags |= SCF_EMULATED_TASK_SENSE;
 	cmd->scsi_status = SAM_STAT_CHECK_CONDITION;
-	cmd->scsi_sense_length  = TRANSPORT_SENSE_BUFFER;
+
 	scsi_build_sense_buffer(desc_format, buffer, key, asc, ascq);
 	if (sd->add_sense_info)
 		WARN_ON_ONCE(scsi_set_sense_information(buffer,
-							cmd->scsi_sense_length,
+							TRANSPORT_SENSE_BUFFER,
 							cmd->sense_info) < 0);
+	/*
+	 * CHECK CONDITION returns sense data, and sense data is minimum 8
+	 * bytes long plus additional Sense Data Length.
+	 * See SPC4-r37 section 4.5.2.1.
+	 */
+	cmd->scsi_sense_length = min_t(u16, buffer[7] + 8,
+				       TRANSPORT_SENSE_BUFFER);
 }
 
 int
-- 


Changes in v3:
 - v2: https://lore.kernel.org/linux-usb/cover.1658192351.git.Thinh.Nguyen@synopsys.com/
 - Moved patches around so fixes patches go first
 - Use hashtable to map tag to uas stream
 - Move target_execute_cmd() out of interrupt context
 - Various cleanup
 - Additional fixes over the 2 years


Thinh Nguyen (28):
  usb: gadget: f_tcm: Don't free command immediately
  usb: gadget: f_tcm: Translate error to sense
  usb: gadget: f_tcm: Decrement command ref count on cleanup
  usb: gadget: f_tcm: Fix Get/SetInterface return value
  usb: gadget: f_tcm: ep_autoconfig with fullspeed endpoint
  usb: gadget: f_tcm: Don't prepare BOT write request twice
  usb: gadget: f_tcm: Increase stream count
  usb: gadget: f_tcm: Increase bMaxBurst
  usb: gadget: f_tcm: Limit number of sessions
  usb: gadget: f_tcm: Get stream by sbitmap number
  usb: gadget: f_tcm: Don't set static stream_id
  usb: gadget: f_tcm: Allocate matching number of commands to streams
  usb: gadget: f_tcm: Handle multiple commands in parallel
  usb: gadget: f_tcm: Use extra number of commands
  usb: gadget: f_tcm: Return ATA cmd direction
  usb: gadget: f_tcm: Execute command on write completion
  usb: gadget: f_tcm: Minor cleanup redundant code
  usb: gadget: f_tcm: Handle abort command
  usb: gadget: f_tcm: Cleanup requests on ep disable
  usb: gadget: f_tcm: Stop proceeding further on -ESHUTDOWN
  usb: gadget: f_tcm: Save CPU ID per command
  usb: gadget: f_tcm: Send sense on cancelled transfer
  usb: gadget: f_tcm: Handle TASK_MANAGEMENT commands
  usb: gadget: f_tcm: Check overlapped command
  usb: gadget: f_tcm: Stall on invalid CBW
  usb: gadget: f_tcm: Requeue command request on error
  usb: gadget: f_tcm: Track BOT command kref
  usb: gadget: f_tcm: Refactor goto check_condition

 drivers/usb/gadget/function/f_tcm.c | 711 ++++++++++++++++++++--------
 drivers/usb/gadget/function/tcm.h   |  28 +-
 2 files changed, 547 insertions(+), 192 deletions(-)


base-commit: d8d936c51388442f769a81e512b505dcf87c6a51
-- 
2.28.0

^ permalink raw reply related	[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.