Bart,
Thanks for your review, appreciated it.
On Sun, 2021-05-23 at 18:32 -0700, Bart Van Assche wrote:
> On 5/23/21 2:14 PM, Bean Huo wrote:
> > + opcode = cmd->cmnd[0];
> > + if ((opcode == READ_10) || (opcode == WRITE_10)) {
> > + /*
> > + * Currently we only fully trace read(10) and write(10)
> > + * commands
> > + */
> > + if (cmd->request && cmd->request->bio)
> > + lba = cmd->request->bio->bi_iter.bi_sector;
>
> Why does the lba assignment occur inside the if-statement for the
> READ_10 and WRITE_10 cases? Has it been considered to move that
> assignment before this if-statement?
yes, this lba assignment can be moved before if-statement:
if (cmd->request && cmd->request->bio)
lba
= cmd->request->bio->bi_iter.bi_sector;
if ((opcode == READ_10) || (opcode == WRITE_10)) {
/*
* Currently we only fully trace read(10) and write(10)
* commands
*/
>
> Does 'lba' represent an offset in units of 512 bytes (sector_t) or an
> LBA (logical block address)? In the former case, please rename the
> variable 'lba' into 'sector' or 'start_sector'. In the latter case,
> please use sectors_to_logical().
apparently it is in 512 bytes. ok, sector is more readable.
>
> Why are READ_16 and WRITE_16 ignored?
READ_16 and WRITE_16 are optimal for the UFS. not mandatory.
>
> Please remove the 'if (cmd->request)' checks since these are not
> necessary.
>
> > + } else if (opcode == UNMAP) {
> > + if (cmd->request) {
> > + lba = scsi_get_lba(cmd);
> > + transfer_len = blk_rq_bytes(cmd->request);
> > }
> > }
>
> The name of the variable 'transfer_len' is wrong since blk_rq_bytes()
> returns the number of bytes affected on the storage medium instead of
> the number of bytes transferred from the host to the storage
> controller.
>
ok, I will remove them, and they will be a additional cleanup patch.
> > /**
> > - * Describes the ufs rpmb wlun.
> > - * Used only to send uac.
> > + * Describes the ufs rpmb wlun. Used only to send uac.
> > */
>
> Is this change related to the rest of this patch?
>
It might be a cleanup.
Bean
> Thanks,
>
> Bart.
