From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1162302AbeBOT6B (ORCPT ); Thu, 15 Feb 2018 14:58:01 -0500 Received: from mail.kernel.org ([198.145.29.99]:55564 "EHLO mail.kernel.org" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1161474AbeBOT57 (ORCPT ); Thu, 15 Feb 2018 14:57:59 -0500 DMARC-Filter: OpenDMARC Filter v1.3.2 mail.kernel.org F1035217A0 Authentication-Results: mail.kernel.org; dmarc=none (p=none dis=none) header.from=goodmis.org Authentication-Results: mail.kernel.org; spf=none smtp.mailfrom=rostedt@goodmis.org Date: Thu, 15 Feb 2018 14:57:55 -0500 From: Steven Rostedt To: Lina Iyer Cc: andy.gross@linaro.org, david.brown@linaro.org, linux-arm-msm@vger.kernel.org, linux-soc@vger.kernel.org, rnayak@codeaurora.org, bjorn.andersson@linaro.org, linux-kernel@vger.kernel.org Subject: Re: [PATCH v2 03/10] drivers: qcom: rpmh-rsc: log RPMH requests in FTRACE Message-ID: <20180215145755.31346aff@gandalf.local.home> In-Reply-To: <20180215173507.10989-4-ilina@codeaurora.org> References: <20180215173507.10989-1-ilina@codeaurora.org> <20180215173507.10989-4-ilina@codeaurora.org> X-Mailer: Claws Mail 3.16.0 (GTK+ 2.24.31; x86_64-pc-linux-gnu) MIME-Version: 1.0 Content-Type: text/plain; charset=US-ASCII Content-Transfer-Encoding: 7bit Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org On Thu, 15 Feb 2018 10:35:00 -0700 Lina Iyer 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. + } + + 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. -- Steve > + __entry->addr, __entry->data, __entry->complete) > +); > +