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;
Sender: cang=codeaurora.org@mg.codeaurora.org
MIME-Version: 1.0
Content-Type: text/plain; charset=UTF-8;
 format=flowed
Content-Transfer-Encoding: 8bit
Date:   Wed, 02 Jun 2021 10:32:26 +0800
From:   Can Guo <cang@codeaurora.org>
To:     Bean Huo <huobean@gmail.com>
Cc:     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, bvanassche@acm.org, tomas.winkler@intel.com,
        linux-scsi@vger.kernel.org, linux-kernel@vger.kernel.org
Subject: Re: [PATCH v2 3/4] scsi: ufs: Let command trace only for the cmd !=
 null case
In-Reply-To: <20210531104308.391842-4-huobean@gmail.com>
References: <20210531104308.391842-1-huobean@gmail.com>
 <20210531104308.391842-4-huobean@gmail.com>
Message-ID: <309e9bacdc0a6ab78913036f4c95f142@codeaurora.org>
User-Agent: Roundcube Webmail/1.3.9
Precedence: bulk

On 2021-05-31 18:43, Bean Huo wrote:
> From: Bean Huo <beanhuo@micron.com>
> 
> For the query request, we already have query_trace, but in
> ufshcd_send_command (), there will add two more redundant
> traces. Since lrbp->cmd is null in the query request, the
> below these two trace events provide nothing except the tag
> and DB. Instead of letting them take up the limited trace
> ring buffer, it’s better not to print these traces in case
> of cmd == null.
> 
> ufshcd_command: send_req: ff3b0000.ufs: tag: 28, DB: 0x0, size: -1,
> IS: 0, LBA: 18446744073709551615, opcode: 0x0 (0x0), group_id: 0x0
> ufshcd_command: dev_complete: ff3b0000.ufs: tag: 28, DB: 0x0, size:
> -1, IS: 0, LBA: 18446744073709551615, opcode: 0x0 (0x0), group_id: 0x0
> 
> Signed-off-by: Bean Huo <beanhuo@micron.com>
> ---
>  drivers/scsi/ufs/ufshcd.c | 44 +++++++++++++++++++--------------------
>  1 file changed, 21 insertions(+), 23 deletions(-)
> 
> diff --git a/drivers/scsi/ufs/ufshcd.c b/drivers/scsi/ufs/ufshcd.c
> index c5754d5486c9..c84bd8e045f6 100644
> --- a/drivers/scsi/ufs/ufshcd.c
> +++ b/drivers/scsi/ufs/ufshcd.c
> @@ -378,35 +378,33 @@ static void ufshcd_add_command_trace(struct
> ufs_hba *hba, unsigned int tag,
>  	struct scsi_cmnd *cmd = lrbp->cmd;
>  	int transfer_len = -1;
> 
> +	if (!cmd)
> +		return;
> +
>  	if (!trace_ufshcd_command_enabled()) {
>  		/* trace UPIU W/O tracing command */
> -		if (cmd)
> -			ufshcd_add_cmd_upiu_trace(hba, tag, str_t);
> +		ufshcd_add_cmd_upiu_trace(hba, tag, str_t);
>  		return;
>  	}
> 
> -	if (cmd) { /* data phase exists */
> -		/* trace UPIU also */
> -		ufshcd_add_cmd_upiu_trace(hba, tag, str_t);
> -		opcode = cmd->cmnd[0];
> -		lba = sectors_to_logical(cmd->device, blk_rq_pos(cmd->request));
> +	/* trace UPIU also */
> +	ufshcd_add_cmd_upiu_trace(hba, tag, str_t);
> +	opcode = cmd->cmnd[0];
> +	lba = sectors_to_logical(cmd->device, blk_rq_pos(cmd->request));
> 
> -		if ((opcode == READ_10) || (opcode == WRITE_10)) {
> -			/*
> -			 * Currently we only fully trace read(10) and write(10)
> -			 * commands
> -			 */
> -			transfer_len = be32_to_cpu(
> -				lrbp->ucd_req_ptr->sc.exp_data_transfer_len);
> -			if (opcode == WRITE_10)
> -				group_id = lrbp->cmd->cmnd[6];
> -		} else if (opcode == UNMAP) {
> -			/*
> -			 * The number of Bytes to be unmapped beginning with the
> -			 * lba.
> -			 */
> -			transfer_len = blk_rq_bytes(cmd->request);
> -		}
> +	if (opcode == READ_10 || opcode == WRITE_10) {
> +		/*
> +		 * Currently we only fully trace read(10) and write(10) commands
> +		 */
> +		transfer_len =
> +		       be32_to_cpu(lrbp->ucd_req_ptr->sc.exp_data_transfer_len);
> +		if (opcode == WRITE_10)
> +			group_id = lrbp->cmd->cmnd[6];
> +	} else if (opcode == UNMAP) {
> +		/*
> +		 * The number of Bytes to be unmapped beginning with the lba.
> +		 */
> +		transfer_len = blk_rq_bytes(cmd->request);
>  	}
> 
>  	intr = ufshcd_readl(hba, REG_INTERRUPT_STATUS);

Reviewed-by: Can Guo <cang@codeaurora.org>