Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 23.128.96.18 as permitted sender) client-ip=23.128.96.18;
Message-ID: <bd0c18c8ca48fedde3c273796c58307138cd4bd1.camel@gmail.com>
Subject: 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
Date:   Tue, 25 May 2021 22:02:39 +0200
In-Reply-To: <1d06cc01-a642-e8e0-a251-1b392e4935c7@acm.org>
References: <20210523211409.210304-1-huobean@gmail.com>
         <20210523211409.210304-3-huobean@gmail.com>
         <1d06cc01-a642-e8e0-a251-1b392e4935c7@acm.org>
Content-Type: text/plain; charset="UTF-8"
User-Agent: Evolution 3.36.4-0ubuntu1 
MIME-Version: 1.0
Content-Transfer-Encoding: 7bit
Precedence: bulk

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.