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;
Date:   Mon, 7 Dec 2020 10:34:03 -0500
From:   Steven Rostedt <rostedt@goodmis.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,
        cang@codeaurora.org, linux-scsi@vger.kernel.org,
        linux-kernel@vger.kernel.org
Subject: Re: [PATCH v1 3/3] scsi: ufs: Make UPIU trace easier differentiate
 among CDB, OSF, and TM
Message-ID: <20201207103403.1c9c1045@gandalf.local.home>
In-Reply-To: <20201206164226.6595-4-huobean@gmail.com>
References: <20201206164226.6595-1-huobean@gmail.com>
        <20201206164226.6595-4-huobean@gmail.com>
MIME-Version: 1.0
Content-Type: text/plain; charset=US-ASCII
Content-Transfer-Encoding: 7bit
Precedence: bulk

On Sun,  6 Dec 2020 17:42:26 +0100
Bean Huo <huobean@gmail.com> wrote:

> From: Bean Huo <beanhuo@micron.com>
> 
> Transaction Specific Fields (TSF) in the UPIU package could be CDB
> (SCSI/UFS Command Descriptor Block), OSF (Opcode Specific Field), and
> TM I/O parameter (Task Management Input/Output Parameter). But, currently,
> we take all of these as CDB  in the UPIU trace. Thus makes user confuse
> among CDB, OSF, and TM message. So fix it with this patch.
> 
> Signed-off-by: Bean Huo <beanhuo@micron.com>
> ---
>  drivers/scsi/ufs/ufshcd.c  |  9 +++++----
>  include/trace/events/ufs.h | 10 +++++++---
>  2 files changed, 12 insertions(+), 7 deletions(-)
> 
> diff --git a/drivers/scsi/ufs/ufshcd.c b/drivers/scsi/ufs/ufshcd.c
> index 29d7240a61bf..5b2219e44743 100644
> --- a/drivers/scsi/ufs/ufshcd.c
> +++ b/drivers/scsi/ufs/ufshcd.c
> @@ -315,7 +315,8 @@ static void ufshcd_add_cmd_upiu_trace(struct ufs_hba *hba, unsigned int tag,
>  {
>  	struct utp_upiu_req *rq = hba->lrb[tag].ucd_req_ptr;
>  
> -	trace_ufshcd_upiu(dev_name(hba->dev), str, &rq->header, &rq->sc.cdb);
> +	trace_ufshcd_upiu(dev_name(hba->dev), str, &rq->header, &rq->sc.cdb,
> +			  "CDB");
>  }
>  
>  static void ufshcd_add_query_upiu_trace(struct ufs_hba *hba, unsigned int tag,
> @@ -329,7 +330,7 @@ static void ufshcd_add_query_upiu_trace(struct ufs_hba *hba, unsigned int tag,
>  		rq_rsp = (struct utp_upiu_req *)hba->lrb[tag].ucd_rsp_ptr;
>  
>  	trace_ufshcd_upiu(dev_name(hba->dev), str, &rq_rsp->header,
> -			  &rq_rsp->qr);
> +			  &rq_rsp->qr, "OSF");
>  }
>  
>  static void ufshcd_add_tm_upiu_trace(struct ufs_hba *hba, unsigned int tag,
> @@ -340,10 +341,10 @@ static void ufshcd_add_tm_upiu_trace(struct ufs_hba *hba, unsigned int tag,
>  
>  	if (!strcmp("tm_send", str))
>  		trace_ufshcd_upiu(dev_name(hba->dev), str, &descp->req_header,
> -				  &descp->input_param1);
> +				  &descp->input_param1, "TM_INPUT");
>  	else
>  		trace_ufshcd_upiu(dev_name(hba->dev), str, &descp->rsp_header,
> -				  &descp->output_param1);
> +				  &descp->output_param1, "TM_OUTPUT");

You could save some space on the ring buffer, if you made the above into an
enum, and then used print_symbolic().

>  }
>  
>  static void ufshcd_add_uic_command_trace(struct ufs_hba *hba,
> diff --git a/include/trace/events/ufs.h b/include/trace/events/ufs.h
> index 0bd54a184391..68e8e97a9b47 100644
> --- a/include/trace/events/ufs.h
> +++ b/include/trace/events/ufs.h
> @@ -295,15 +295,17 @@ TRACE_EVENT(ufshcd_uic_command,
>  );


You could make this:

#define TRACE_TSF_TYPES		\
	EM(CDB)			\
	EM(OSF)			\
	EM(TM_INPUT)		\
	EMe(TM_OUTPUT)

#ifndef TRACE_TSF_TYPES_ENUMS
#define TRACE_TSF_TYPES_ENUMS
#undef EM
#undef EMe

#define EM(x)	TRACE_TSF_##x,
#define EMe(x)	TRACE_TSF_##x

enum {
	TRACE_TSF_TYPES
}
#endif /* TRACE_TSF_TYPES_ENUMS */

#undef EM
#undef EMe

/* These export the enum names to user space */
#define EM(x)	TRACE_DEFINE_ENUM(TRACE_TSF_##x)
#define EMe(x)	TRACE_DEFINE_ENUM(TRACE_TSF_##x)

TRACE_TSF_TYPES

#undef EM
#undef EMe

/* These are used in the print_symbolic */
#define EM(x) { TRACE_TSF_##x, #x },
#define EMe(x) { TRACE_TSF_##x, #x }


>  
>  TRACE_EVENT(ufshcd_upiu,
> -	TP_PROTO(const char *dev_name, const char *str, void *hdr, void *tsf),
> +	TP_PROTO(const char *dev_name, const char *str, void *hdr, void *tsf,
> +		 const char *tsf_type),

		int tsf_type;

>  
> -	TP_ARGS(dev_name, str, hdr, tsf),
> +	TP_ARGS(dev_name, str, hdr, tsf, tsf_type),
>  
>  	TP_STRUCT__entry(
>  		__string(dev_name, dev_name)
>  		__string(str, str)
>  		__array(unsigned char, hdr, 12)
>  		__array(unsigned char, tsf, 16)
> +		__string(tsf_type, tsf_type)

		__field(int, tsf_type)

>  	),
>  
>  	TP_fast_assign(
> @@ -311,12 +313,14 @@ TRACE_EVENT(ufshcd_upiu,
>  		__assign_str(str, str);
>  		memcpy(__entry->hdr, hdr, sizeof(__entry->hdr));
>  		memcpy(__entry->tsf, tsf, sizeof(__entry->tsf));
> +		__assign_str(tsf_type, tsf_type);


		__entry->tsf_type = tsf_type;


>  	),
>  
>  	TP_printk(
> -		"%s: %s: HDR:%s, CDB:%s",
> +		"%s: %s: HDR:%s, %s:%s",
>  		__get_str(str), __get_str(dev_name),
>  		__print_hex(__entry->hdr, sizeof(__entry->hdr)),
> +		__get_str(tsf_type),

		print_symbolic(tsf_type, TRACE_TSF_TYPES),

-- Steve


>  		__print_hex(__entry->tsf, sizeof(__entry->tsf))
>  	)
>  );