Re: [PATCH v1 2/3] scsi: ufs: Let command trace only for the cmd != null case

[Bean Huo]

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.