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;
Subject: Re: [PATCH v1 2/3] scsi: ufs: Let command trace only for the cmd !=
 null case
To:     Bean Huo <huobean@gmail.com>, 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
References: <20210523211409.210304-1-huobean@gmail.com>
 <20210523211409.210304-3-huobean@gmail.com>
From:   Bart Van Assche <bvanassche@acm.org>
Message-ID: <1d06cc01-a642-e8e0-a251-1b392e4935c7@acm.org>
Date:   Sun, 23 May 2021 18:32:08 -0700
User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:78.0) Gecko/20100101
 Thunderbird/78.10.2
MIME-Version: 1.0
In-Reply-To: <20210523211409.210304-3-huobean@gmail.com>
Content-Type: text/plain; charset=utf-8
Content-Language: en-US
Content-Transfer-Encoding: 7bit
Precedence: bulk

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?

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().

Why are READ_16 and WRITE_16 ignored?

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.

>  /**
> - * 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?

Thanks,

Bart.