| Message ID | cover.1733876548.git.Thinh.Nguyen@synopsys.com |
|---|---|
| State | New |
| Headers | show |
Hi Thinh, On 2024-12-11 0:31 UTC, Thinh Nguyen <Thinh.Nguyen@synopsys.com> wrote: > 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. I am trying to do some basic rw aging test with this serie on my gadget board. When it comes to target_core_iblock, the rw code is less similar to the one in target_core_file or ramdisk. Could you just spend some extra time explaining what cause the Data Corruption issue and how can this fix it ? So that I could perform similar fix in target_core_iblock on my own, so a full test could start soonner. B.R. H. Akemi
(Removed Cc invalid emails to Nicholas and Andrzej) Hi, On Fri, Dec 20, 2024, Homura Akemi wrote: > Hi Thinh, > > On 2024-12-11 0:31 UTC, Thinh Nguyen <Thinh.Nguyen@synopsys.com> wrote: > > 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. > > I am trying to do some basic rw aging test with this serie on my gadget board. > When it comes to target_core_iblock, the rw code is less similar to the one in > target_core_file or ramdisk. > Could you just spend some extra time explaining what cause the Data > Corruption issue and how can this fix it ? So that I could perform > similar fix in > target_core_iblock on my own, so a full test could start soonner. > When we prepare a new generic command for read/write, we call to target_alloc_sgl(). This will allocate PAGE_SIZE SG entries enough to handle the se_cmd read/write base on its length. The total length of all the SG entries combine will be at least se_cmd->data_length. The typical block size is 512 byte. A page size is typically 4KB. So, the se_cmd->data_length may not be a multiple of PAGE_SiZE. In target_core_file, it execute_rw() with this logic: 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; } // It sets the length to be the iter to be total length of the // allocated SG entries and not the requested command length: iov_iter_bvec(&iter, is_write, aio_cmd->bvecs, sgl_nents, len); aio_cmd->cmd = cmd; aio_cmd->len = len; aio_cmd->iocb.ki_pos = cmd->t_task_lba * dev->dev_attrib.block_size; aio_cmd->iocb.ki_filp = file; aio_cmd->iocb.ki_complete = cmd_rw_aio_complete; aio_cmd->iocb.ki_flags = IOCB_DIRECT; if (is_write && (cmd->se_cmd_flags & SCF_FUA)) aio_cmd->iocb.ki_flags |= IOCB_DSYNC; // So when we do f_op read/write, we may do more than needed and may // write bogus data from the extra SG entry length. if (is_write) ret = file->f_op->write_iter(&aio_cmd->iocb, &iter); else ret = file->f_op->read_iter(&aio_cmd->iocb, &iter); I did not review target_core_iblock. It may or may not do things properly. BR, Thinh
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
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(-)