On Thu, Feb 15 2018 at 19:57 +0000, Steven Rostedt wrote:
On Thu, 15 Feb 2018 10:35:00 -0700 Lina Iyer <[email protected]> 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
