linux-kernel - Re: [PATCH v2 3/4] scsi: ufs: Let command trace only for the cmd != null case

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <309e9bacdc0a6ab78913036f4c95f142@codeaurora.org>
Date:   Wed, 02 Jun 2021 10:32:26 +0800
From:   Can Guo <cang@...eaurora.org>
To:     Bean Huo <huobean@...il.com>
Cc:     alim.akhtar@...sung.com, avri.altman@....com,
        asutoshd@...eaurora.org, jejb@...ux.ibm.com,
        martin.petersen@...cle.com, stanley.chu@...iatek.com,
        beanhuo@...ron.com, bvanassche@....org, tomas.winkler@...el.com,
        linux-scsi@...r.kernel.org, linux-kernel@...r.kernel.org
Subject: Re: [PATCH v2 3/4] scsi: ufs: Let command trace only for the cmd !=
 null case

On 2021-05-31 18:43, Bean Huo wrote:
> From: Bean Huo <beanhuo@...ron.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@...ron.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@...eaurora.org>