lkml.org 
[lkml]   [2018]   [Feb]   [15]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
Patch in this message
/
Date
From
SubjectRe: [PATCH v2 03/10] drivers: qcom: rpmh-rsc: log RPMH requests in FTRACE
On Thu, Feb 15 2018 at 19:57 +0000, Steven Rostedt wrote:
>On Thu, 15 Feb 2018 10:35:00 -0700
>Lina Iyer <ilina@codeaurora.org> wrote:
>
>> @@ -298,6 +303,7 @@ static void __tcs_buffer_write(struct rsc_drv *drv, int m, int n,
>> write_tcs_reg(drv, RSC_DRV_CMD_MSGID, m, n + i, msgid);
>> write_tcs_reg(drv, RSC_DRV_CMD_ADDR, m, n + i, cmd->addr);
>> write_tcs_reg(drv, RSC_DRV_CMD_DATA, m, n + i, cmd->data);
>> + trace_rpmh_send_msg(drv, m, n + i, msgid, cmd);
>
>No biggy, but I'm curious to why you didn't do something this:
>
>+static void __tcs_buffer_write(struct rsc_drv *drv, int m, int n,
>+ struct tcs_request *msg)
>+{
>+ u32 msgid, cmd_msgid = 0;
>+ u32 cmd_enable = 0;
>+ u32 cmd_complete;
>+ struct tcs_cmd *cmd;
>+ int i;
>+
>+ cmd_msgid = CMD_MSGID_LEN;
>+ cmd_msgid |= (msg->is_complete) ? CMD_MSGID_RESP_REQ : 0;
>+ cmd_msgid |= CMD_MSGID_WRITE;
>+
>+ cmd_complete = read_tcs_reg(drv, RSC_DRV_CMD_WAIT_FOR_CMPL, m, 0);
>+
>+ for (i = 0; i < msg->num_payload; i++) {
>
> int bit = n + i;
>
>+ cmd = &msg->payload[i];
>+ cmd_enable |= BIT(bit);
>+ cmd_complete |= cmd->complete << (n + i);
>+ msgid = cmd_msgid;
>+ msgid |= (cmd->complete) ? CMD_MSGID_RESP_REQ : 0;
>+ write_tcs_reg(drv, RSC_DRV_CMD_MSGID, m, bit, msgid);
>+ write_tcs_reg(drv, RSC_DRV_CMD_ADDR, m, bit, cmd->addr);
>+ write_tcs_reg(drv, RSC_DRV_CMD_DATA, m, bit, cmd->data);
>
> trace_rpmh_send_msg(drv, m, bit, msgid, cmd);
>
>The compiler should optimize that, so this isn't really a big deal, but
>I was just curious.
>
>
No particular reason. Think I just went with the logic at that time and
didn't look back deeply again on the code to tidy it up. Thanks for the
suggestion.

>+ }
>+
>+ write_tcs_reg(drv, RSC_DRV_CMD_WAIT_FOR_CMPL, m, 0, cmd_complete);
>+ cmd_enable |= read_tcs_reg(drv, RSC_DRV_CMD_ENABLE, m, 0);
>+ write_tcs_reg(drv, RSC_DRV_CMD_ENABLE, m, 0, cmd_enable);
>+}
>
>> }
>>
>> write_tcs_reg(drv, RSC_DRV_CMD_WAIT_FOR_CMPL, m, 0, cmd_complete);
>
>[..]
>
>> +TRACE_EVENT(rpmh_send_msg,
>> +
>> + TP_PROTO(struct rsc_drv *d, int m, int n, u32 h, struct tcs_cmd *c),
>> +
>> + TP_ARGS(d, m, n, h, c),
>> +
>> + TP_STRUCT__entry(
>> + __field(const char*, d->name)
>> + __field(int, m)
>> + __field(int, n)
>> + __field(u32, hdr)
>> + __field(u32, addr)
>> + __field(u32, data)
>> + __field(bool, complete)
>> + ),
>> +
>> + TP_fast_assign(
>> + __entry->name = s;
>> + __entry->m = m;
>> + __entry->n = n;
>> + __entry->hdr = h;
>> + __entry->addr = c->addr;
>> + __entry->data = c->data;
>> + __entry->complete = c->complete;
>> + ),
>> +
>> + TP_printk("%s: send-msg: tcs(m): %d cmd(n): %d msgid: 0x%08x addr: 0x%08x data: 0x%08x complete: %d",
>> + __entry->name, __entry->m, __entry->n, __entry->hdr,
>
>I'm sorry I didn't catch this in my other reviews, but please don't use
>direct strings in TP_printk(). In trace-cmd and perf, it has no access
>to that information when reading this trace event. Not to mention, if
>drv is freed between the time it is recorded, and the time it is read
>in the trace buffer, you are now referencing random memory.
>
>The way to do this in a trace event is to use the string functionality:
>
> TP_STRUCT__entry(
> __string(name, d->name)
> [..]
> TP_fast_assign(
> __assign_string(name, d->name)
> [..]
> TP_printk("%s: ...",
> __get_str(name), ...
>
>Then the name is recorded in the ring buffer at the time of execution
>of the trace event, and trace-cmd and perf can read it, and there's no
>worries about it being freed between recording and reading the tracing
>buffer.
>
The drv would not be freed. But that said, I will use this in my
patches.

Thanks Steve.

-- Lina

\
 
 \ /
  Last update: 2018-02-15 21:39    [W:0.172 / U:0.248 seconds]
©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site