Re: [PATCH v3 00/28] usb: gadget: f_tcm: Enhance UASP driver

[Thinh Nguyen <Thinh.Nguyen@synopsys.com>]

(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