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

From: Bean Huo <huobean@gmail.com>
To: Bart Van Assche <bvanassche@acm.org>,
	alim.akhtar@samsung.com, avri.altman@wdc.com,
	asutoshd@codeaurora.org, jejb@linux.ibm.com,
	martin.petersen@oracle.com, stanley.chu@mediatek.com,
	beanhuo@micron.com, tomas.winkler@intel.com, cang@codeaurora.org
Cc: linux-scsi@vger.kernel.org, linux-kernel@vger.kernel.org
Subject: Re: [PATCH v1 2/3] scsi: ufs: Let command trace only for the cmd != null case
Date: Tue, 25 May 2021 22:02:39 +0200	[thread overview]
Message-ID: <bd0c18c8ca48fedde3c273796c58307138cd4bd1.camel@gmail.com> (raw)
In-Reply-To: <1d06cc01-a642-e8e0-a251-1b392e4935c7@acm.org>

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.