From: Avri Altman <Avri.Altman@wdc.com>
To: Stanley Chu <stanley.chu@mediatek.com>,
"linux-scsi@vger.kernel.org" <linux-scsi@vger.kernel.org>,
"martin.petersen@oracle.com" <martin.petersen@oracle.com>,
"alim.akhtar@samsung.com" <alim.akhtar@samsung.com>,
"jejb@linux.ibm.com" <jejb@linux.ibm.com>,
"asutoshd@codeaurora.org" <asutoshd@codeaurora.org>
Cc: "beanhuo@micron.com" <beanhuo@micron.com>,
"cang@codeaurora.org" <cang@codeaurora.org>,
"matthias.bgg@gmail.com" <matthias.bgg@gmail.com>,
"bvanassche@acm.org" <bvanassche@acm.org>,
"linux-mediatek@lists.infradead.org"
<linux-mediatek@lists.infradead.org>,
"linux-arm-kernel@lists.infradead.org"
<linux-arm-kernel@lists.infradead.org>,
"linux-kernel@vger.kernel.org" <linux-kernel@vger.kernel.org>,
"kuohong.wang@mediatek.com" <kuohong.wang@mediatek.com>,
"peter.wang@mediatek.com" <peter.wang@mediatek.com>,
"chun-hung.wu@mediatek.com" <chun-hung.wu@mediatek.com>,
"andy.teng@mediatek.com" <andy.teng@mediatek.com>,
"chaotian.jing@mediatek.com" <chaotian.jing@mediatek.com>,
"cc.chou@mediatek.com" <cc.chou@mediatek.com>
Subject: RE: [PATCH v1 2/2] scsi: ufs: Add trace event for UIC commands
Date: Sat, 13 Jun 2020 10:48:21 +0000 [thread overview]
Message-ID: <SN6PR04MB4640968DCD865651AFA8925DFC9E0@SN6PR04MB4640.namprd04.prod.outlook.com> (raw)
In-Reply-To: <20200612151000.27639-3-stanley.chu@mediatek.com>
> +static void ufshcd_add_uic_command_trace(struct ufs_hba *hba,
> + struct uic_command *ucmd,
> + const char *str)
> +{
> + u32 cmd;
> +
> + if (!trace_ufshcd_uic_command_enabled())
> + return;
> +
> + if (!strcmp(str, "uic_send"))
> + cmd = ucmd->command;
> + else
> + cmd = ufshcd_readl(hba, REG_UIC_COMMAND);
Why on complete you can't just use ucmd->command as well?
> +
> + trace_ufshcd_uic_command(dev_name(hba->dev), str, cmd,
> + ucmd->result,
> + ufshcd_readl(hba, REG_UIC_COMMAND_ARG_1),
> + ufshcd_readl(hba, REG_UIC_COMMAND_ARG_2),
> + ufshcd_readl(hba, REG_UIC_COMMAND_ARG_3));
Why can't you just use the ucmd members?
Why need to read those?
> +}
> +
> static void ufshcd_add_command_trace(struct ufs_hba *hba,
> unsigned int tag, const char *str)
> {
> @@ -2054,6 +2075,8 @@ ufshcd_dispatch_uic_cmd(struct ufs_hba *hba,
> struct uic_command *uic_cmd)
> /* Write UIC Cmd */
> ufshcd_writel(hba, uic_cmd->command & COMMAND_OPCODE_MASK,
> REG_UIC_COMMAND);
> +
> + ufshcd_add_uic_command_trace(hba, uic_cmd, "uic_send");
> }
>
> /**
> @@ -2080,6 +2103,9 @@ ufshcd_wait_for_uic_cmd(struct ufs_hba *hba,
> struct uic_command *uic_cmd)
> hba->active_uic_cmd = NULL;
> spin_unlock_irqrestore(hba->host->host_lock, flags);
>
> + uic_cmd->result = ret;
> + ufshcd_add_uic_command_trace(hba, uic_cmd, "uic_complete");
> +
> return ret;
> }
Can't you just call the "send" and "complete" from ufshcd_send_uic_cmd?
>
> @@ -3760,6 +3786,9 @@ static int ufshcd_uic_pwr_ctrl(struct ufs_hba *hba,
> struct uic_command *cmd)
> ret = (status != PWR_OK) ? status : -1;
> }
> out:
> + cmd->result = ret;
> + ufshcd_add_uic_command_trace(hba, cmd, "uic_complete");
> +
> if (ret) {
> ufshcd_print_host_state(hba);
> ufshcd_print_pwr_info(hba);
> diff --git a/include/trace/events/ufs.h b/include/trace/events/ufs.h
> index 5f300739240d..cf8d568d5a13 100644
> --- a/include/trace/events/ufs.h
> +++ b/include/trace/events/ufs.h
> @@ -249,6 +249,39 @@ TRACE_EVENT(ufshcd_command,
> )
> );
>
> +TRACE_EVENT(ufshcd_uic_command,
> + TP_PROTO(const char *dev_name, const char *str, u32 cmd, int result,
> + u32 arg1, u32 arg2, u32 arg3),
> +
> + TP_ARGS(dev_name, str, cmd, result, arg1, arg2, arg3),
> +
> + TP_STRUCT__entry(
> + __string(dev_name, dev_name)
> + __string(str, str)
> + __field(u32, cmd)
> + __field(int, result)
> + __field(u32, arg1)
> + __field(u32, arg2)
> + __field(u32, arg3)
> + ),
> +
> + TP_fast_assign(
> + __assign_str(dev_name, dev_name);
> + __assign_str(str, str);
> + __entry->cmd = cmd;
> + __entry->result = result;
> + __entry->arg1 = arg1;
> + __entry->arg2 = arg2;
> + __entry->arg3 = arg3;
> + ),
> +
> + TP_printk(
> + "%s: %s: cmd: 0x%x, arg1: 0x%x, arg2: 0x%x, arg3: 0x%x, result: %d",
> + __get_str(str), __get_str(dev_name), __entry->cmd,
> + __entry->arg1, __entry->arg2, __entry->arg3, __entry->result
> + )
Personally, as those trace events aren't very human readable anyway, I would just dump the uic command,
And let the parsers do their job.
And if this is the case, result is redundant as it is part of arg2
Thanks,
Avri
> +);
> +
> TRACE_EVENT(ufshcd_upiu,
> TP_PROTO(const char *dev_name, const char *str, void *hdr, void *tsf),
>
> --
> 2.18.0
next prev parent reply other threads:[~2020-06-13 10:48 UTC|newest]
Thread overview: 7+ messages / expand[flat|nested] mbox.gz Atom feed top
2020-06-12 15:09 [PATCH v1 0/2] scsi: ufs: Add trace event for UIC commands Stanley Chu
2020-06-12 15:09 ` [PATCH v1 1/2] scsi: ufs: Remove unused field in struct uic_command Stanley Chu
2020-06-12 16:22 ` Alim Akhtar
2020-06-12 15:10 ` [PATCH v1 2/2] scsi: ufs: Add trace event for UIC commands Stanley Chu
2020-06-13 10:48 ` Avri Altman [this message]
2020-06-13 14:18 ` Stanley Chu
2020-06-14 9:27 ` Avri Altman
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=SN6PR04MB4640968DCD865651AFA8925DFC9E0@SN6PR04MB4640.namprd04.prod.outlook.com \
--to=avri.altman@wdc.com \
--cc=alim.akhtar@samsung.com \
--cc=andy.teng@mediatek.com \
--cc=asutoshd@codeaurora.org \
--cc=beanhuo@micron.com \
--cc=bvanassche@acm.org \
--cc=cang@codeaurora.org \
--cc=cc.chou@mediatek.com \
--cc=chaotian.jing@mediatek.com \
--cc=chun-hung.wu@mediatek.com \
--cc=jejb@linux.ibm.com \
--cc=kuohong.wang@mediatek.com \
--cc=linux-arm-kernel@lists.infradead.org \
--cc=linux-kernel@vger.kernel.org \
--cc=linux-mediatek@lists.infradead.org \
--cc=linux-scsi@vger.kernel.org \
--cc=martin.petersen@oracle.com \
--cc=matthias.bgg@gmail.com \
--cc=peter.wang@mediatek.com \
--cc=stanley.chu@mediatek.com \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).