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

[Can Guo]

On 2021-05-31 18:43, Bean Huo wrote:
> From: Bean Huo <beanhuo@xxxxxxxxxx>
>
> 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@xxxxxxxxxx>
> ---
>  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@xxxxxxxxxxxxxx>