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;
IronPort-SDR: A4uP8KPI/Uho8AJqq/h27cUJEGfAPMj0hZSlApwiYVI+n5ud3E6RVENCvdwKBExu5kdkzmvs+9
 f4ARB0mLjeSfx5MuwjlQ5sQuHLg6mG7MWFYU1qu/ZtT+AhGYz1SMwwM1PgDX2VQtW5I+vfLu2O
 vAwpLmyxzM2Ess/NSpm/j/84aVDJRv9fDGRHtyQSyxVMS69BszHTIZ0V57gF0v5WdCuuJ5XvuD
 IrQTWXadVMKfEkiJxYuhywPTgqXxXVYAhT29GVu4+kHpjLnHS2vKo2t7tKeXVp+2QHwp+0A34N
 C6g=
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
Thread-Topic: [PATCH v1 2/2] scsi: ufs: Add trace event for UIC commands
Thread-Index: AQHWQMuVYKwzY3Iyf0mpsLlh28O4SKjWW/nQ
Date:   Sat, 13 Jun 2020 10:48:21 +0000
Message-ID: <SN6PR04MB4640968DCD865651AFA8925DFC9E0@SN6PR04MB4640.namprd04.prod.outlook.com>
References: <20200612151000.27639-1-stanley.chu@mediatek.com>
 <20200612151000.27639-3-stanley.chu@mediatek.com>
In-Reply-To: <20200612151000.27639-3-stanley.chu@mediatek.com>
Accept-Language: en-US
Content-Language: en-US
wdcipoutbound: EOP-TRUE
Content-Type: text/plain; charset="us-ascii"
Content-Transfer-Encoding: quoted-printable
MIME-Version: 1.0
X-MS-Exchange-CrossTenant-Network-Message-Id: 38c49df9-2682-4548-bc75-08d80f874a9d
X-MS-Exchange-CrossTenant-originalarrivaltime: 13 Jun 2020 10:48:21.7929
 (UTC)
X-MS-Exchange-CrossTenant-fromentityheader: Hosted
X-MS-Exchange-CrossTenant-id: b61c8803-16f3-4c35-9b17-6f65f441df86
X-MS-Exchange-CrossTenant-mailboxtype: HOSTED
X-MS-Exchange-CrossTenant-userprincipalname: kB2K77rChAkj2gpARUuGEtt4Ka9wCKkd0YzTqG9dXebrPKgHcw/Z7IEHQiapeJAngHBKe3Hy5/IFCkGYRJ/tzA==
X-MS-Exchange-Transport-CrossTenantHeadersStamped: SN2PR04MB2350
Sender: linux-kernel-owner@vger.kernel.org
Precedence: bulk
List-ID: <linux-kernel.vger.kernel.org>
X-Mailing-List: linux-kernel@vger.kernel.org

> +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 =3D ucmd->command;
> +       else
> +               cmd =3D 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");
>  }
>=20
>  /**
> @@ -2080,6 +2103,9 @@ ufshcd_wait_for_uic_cmd(struct ufs_hba *hba,
> struct uic_command *uic_cmd)
>         hba->active_uic_cmd =3D NULL;
>         spin_unlock_irqrestore(hba->host->host_lock, flags);
>=20
> +       uic_cmd->result =3D 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?


>=20
> @@ -3760,6 +3786,9 @@ static int ufshcd_uic_pwr_ctrl(struct ufs_hba *hba,
> struct uic_command *cmd)
>                 ret =3D (status !=3D PWR_OK) ? status : -1;
>         }
>  out:
> +       cmd->result =3D 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,
>         )
>  );
>=20
> +TRACE_EVENT(ufshcd_uic_command,
> +       TP_PROTO(const char *dev_name, const char *str, u32 cmd, int resu=
lt,
> +                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 =3D cmd;
> +               __entry->result =3D result;
> +               __entry->arg1 =3D arg1;
> +               __entry->arg2 =3D arg2;
> +               __entry->arg3 =3D arg3;
> +       ),
> +
> +       TP_printk(
> +               "%s: %s: cmd: 0x%x, arg1: 0x%x, arg2: 0x%x, arg3: 0x%x, r=
esult: %d",
> +               __get_str(str), __get_str(dev_name), __entry->cmd,
> +               __entry->arg1, __entry->arg2, __entry->arg3, __entry->res=
ult
> +       )
Personally, as those trace events aren't very human readable anyway, I woul=
d 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),
>=20
> --
> 2.18.0