Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S965964AbcLMUoY (ORCPT ); Tue, 13 Dec 2016 15:44:24 -0500 Received: from smtp.codeaurora.org ([198.145.29.96]:54734 "EHLO smtp.codeaurora.org" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S965877AbcLMUoT (ORCPT ); Tue, 13 Dec 2016 15:44:19 -0500 MIME-Version: 1.0 Content-Type: text/plain; charset=US-ASCII; format=flowed Content-Transfer-Encoding: 7bit Date: Tue, 13 Dec 2016 12:44:17 -0800 From: Subhash Jadavani To: Steven Rostedt Cc: vinholikatti@gmail.com, jejb@linux.vnet.ibm.com, martin.petersen@oracle.com, linux-scsi@vger.kernel.org, Ingo Molnar , Sahitya Tummala , Venkat Gopalakrishnan , Lee Susman , linux-kernel@vger.kernel.org Subject: Re: [PATCH v2 02/12] scsi: ufs: add tracing support In-Reply-To: <20161213151021.217fef28@gandalf.local.home> References: <1481658529-31807-1-git-send-email-subhashj@codeaurora.org> <20161213151021.217fef28@gandalf.local.home> Message-ID: User-Agent: Roundcube Webmail/1.2.1 Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org Content-Length: 6361 Lines: 210 On 2016-12-13 12:10, Steven Rostedt wrote: > On Tue, 13 Dec 2016 11:48:45 -0800 > Subhash Jadavani wrote: > >> This change adds the ftrace support for following: >> 1. UFS initialization time >> 2. Clock gating states >> 3. Clock scaling states >> 4. Power management APIs latency >> 5. BKOPs enable/disable >> >> Usage: >> echo 1 > /sys/kernel/debug/tracing/events/ufs/enable >> cat /sys/kernel/debug/tracing/trace_pipe >> >> Reviewed-by: Sahitya Tummala >> Signed-off-by: Subhash Jadavani >> --- >> drivers/scsi/ufs/ufshcd.c | 133 >> ++++++++++++++++++++++++++++++++++++---- >> include/trace/events/ufs.h | 149 >> +++++++++++++++++++++++++++++++++++++++++++++ >> 2 files changed, 270 insertions(+), 12 deletions(-) >> create mode 100644 include/trace/events/ufs.h >> >> diff --git a/drivers/scsi/ufs/ufshcd.c b/drivers/scsi/ufs/ufshcd.c >> index fe516b2..73c5937 100644 >> --- a/drivers/scsi/ufs/ufshcd.c >> +++ b/drivers/scsi/ufs/ufshcd.c >> @@ -45,6 +45,9 @@ >> #include "ufs_quirks.h" >> #include "unipro.h" >> >> +#define CREATE_TRACE_POINTS >> +#include >> + >> #define UFSHCD_REQ_SENSE_SIZE 18 >> >> #define UFSHCD_ENABLE_INTRS (UTP_TRANSFER_REQ_COMPL |\ >> @@ -721,6 +724,40 @@ static void ufshcd_resume_clkscaling(struct >> ufs_hba *hba) >> devfreq_resume_device(hba->devfreq); >> } >> >> +static const char *ufschd_uic_link_state_to_string( >> + enum uic_link_state state) >> +{ >> + switch (state) { >> + case UIC_LINK_OFF_STATE: return "UIC_LINK_OFF_STATE"; >> + case UIC_LINK_ACTIVE_STATE: return "UIC_LINK_ACTIVE_STATE"; >> + case UIC_LINK_HIBERN8_STATE: return "UIC_LINK_HIBERN8_STATE"; >> + default: return "UNKNOWN_STATE"; >> + } >> +} >> + >> +static const char *ufschd_ufs_dev_pwr_mode_to_string( >> + enum ufs_dev_pwr_mode state) >> +{ >> + switch (state) { >> + case UFS_ACTIVE_PWR_MODE: return "UFS_ACTIVE_PWR_MODE"; >> + case UFS_SLEEP_PWR_MODE: return "UFS_SLEEP_PWR_MODE"; >> + case UFS_POWERDOWN_PWR_MODE: return "UFS_POWERDOWN_PWR_MODE"; >> + default: return "UNKNOWN_STATE"; >> + } >> +} >> + >> +static const char *ufschd_clk_gating_state_to_string( >> + enum clk_gating_state state) >> +{ >> + switch (state) { >> + case CLKS_OFF: return "CLKS_OFF"; >> + case CLKS_ON: return "CLKS_ON"; >> + case REQ_CLKS_OFF: return "REQ_CLKS_OFF"; >> + case REQ_CLKS_ON: return "REQ_CLKS_ON"; >> + default: return "UNKNOWN_STATE"; >> + } >> +} >> + > > A much better way than the above is to use the TRACE_DEFINE_ENUM() > macros in the include/trace/events/ufs.h header. In fact, that's what > it was made for. Not only will it be much faster to record, it wont > waste as much space in the ring buffer. > > .e.g. > > #define UFS_LINK_STATES \ > EM( UIC_LINK_OFF_STATE ) \ > EM( UIC_LINK_ACTIVE_STATE ) \ > EMe(UIC_LINK_HIBERN8_STATE ) > > #define UFS_PWR_MODES \ > EM( UFS_ACTIVE_PWR_MODE ) \ > EM( UFS_SLEEP_PWR_MODE ) \ > EM( UFS_POWERDOWN_PWR_MODE) \ > EMe(UFS_POWERDOWN_PWR_MODE) > > #define UFSCHD_CLK_GATING_STATES \ > EM( CLKS_OFF) \ > EM( CLKS_ON) \ > EM( REQ_CLKS_OFF) \ > EMe(REQ_CLKS_ON) > > #undef EM > #undef EMe > > #define EM(a) TRACE_DEFINE_ENUM(a); > #define EMe(a) TRACE_DEFINE_ENUM(a); > > UFS_LINK_STATES > UFS_PWR_MODES > UFSCHD_CLK_GATING_STATES > > #undef EM > #undef EMe > > #define EM(a) { a, #a }, > #define EMe(a) { a, #a } > > > DECLARE_EVENT_CLASS(ufshcd_template, > TP_PROTO(const char *dev_name int err, s64 usecs, > int state, int link_state); > > [..] > > TP_printk( > "%s: took %lld, dev_state: %s, link_state: %s, err %d", > get_str(dev_name), > __entry->usecs, > __print_symbolic(__entry->state, UFS_PWR_MODES), > __print_symbolic(__entry->link_state, UFS_LINK_STATES), > __entry->err > ) > > Not to mention, I think some of the strings may not be matching what > was passed in. Thanks for the suggestion. Let me check this. > > -- Steve > > > >> +DECLARE_EVENT_CLASS(ufshcd_template, >> + TP_PROTO(const char *dev_name, int err, s64 usecs, >> + const char *dev_state, const char *link_state), >> + >> + TP_ARGS(dev_name, err, usecs, dev_state, link_state), >> + >> + TP_STRUCT__entry( >> + __field(s64, usecs) >> + __field(int, err) >> + __string(dev_name, dev_name) >> + __string(dev_state, dev_state) >> + __string(link_state, link_state) >> + ), >> + >> + TP_fast_assign( >> + __entry->usecs = usecs; >> + __entry->err = err; >> + __assign_str(dev_name, dev_name); >> + __assign_str(dev_state, dev_state); >> + __assign_str(link_state, link_state); >> + ), >> + >> + TP_printk( >> + "%s: took %lld usecs, dev_state: %s, link_state: %s, err %d", >> + __get_str(dev_name), >> + __entry->usecs, >> + __get_str(dev_state), >> + __get_str(link_state), >> + __entry->err >> + ) >> +); >> + >> +DEFINE_EVENT(ufshcd_template, ufshcd_system_suspend, >> + TP_PROTO(const char *dev_name, int err, s64 usecs, >> + const char *dev_state, const char *link_state), >> + TP_ARGS(dev_name, err, usecs, dev_state, link_state)); >> + >> +DEFINE_EVENT(ufshcd_template, ufshcd_system_resume, >> + TP_PROTO(const char *dev_name, int err, s64 usecs, >> + const char *dev_state, const char *link_state), >> + TP_ARGS(dev_name, err, usecs, dev_state, link_state)); >> + >> +DEFINE_EVENT(ufshcd_template, ufshcd_runtime_suspend, >> + TP_PROTO(const char *dev_name, int err, s64 usecs, >> + const char *dev_state, const char *link_state), >> + TP_ARGS(dev_name, err, usecs, dev_state, link_state)); >> + >> +DEFINE_EVENT(ufshcd_template, ufshcd_runtime_resume, >> + TP_PROTO(const char *dev_name, int err, s64 usecs, >> + const char *dev_state, const char *link_state), >> + TP_ARGS(dev_name, err, usecs, dev_state, link_state)); >> + >> +DEFINE_EVENT(ufshcd_template, ufshcd_init, >> + TP_PROTO(const char *dev_name, int err, s64 usecs, >> + const char *dev_state, const char *link_state), >> + TP_ARGS(dev_name, err, usecs, dev_state, link_state)); >> +#endif /* if !defined(_TRACE_UFS_H) || >> defined(TRACE_HEADER_MULTI_READ) */ >> + >> +/* This part must be outside protection */ >> +#include -- The Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum, a Linux Foundation Collaborative Project