Received-SPF: pass (google.com: best guess record for domain of linux-kernel-owner@vger.kernel.org designates 209.132.180.67 as permitted sender) client-ip=209.132.180.67;
MIME-Version: 1.0
References: <20191121115542.1.Iaf98f0ab455b626537e77cfa71cef6ff2ab6f37b@changeid>
 <4eef47ad-8d42-1ab4-0c99-028a121cc27c@collabora.com>
In-Reply-To: <4eef47ad-8d42-1ab4-0c99-028a121cc27c@collabora.com>
From:   Raul Rangel <rrangel@chromium.org>
Date:   Mon, 25 Nov 2019 09:49:50 -0700
Message-ID: <CAHQZ30BOmBBHnDR2fTWA1_uoTd6c5+66PbpA57MpiWZcnU5y+A@mail.gmail.com>
Subject: Re: [PATCH] platform/chrome: cros_ec_proto: Add response tracing
To:     Enric Balletbo i Serra <enric.balletbo@collabora.com>
Cc:     Akshu Agrawal <akshu.agrawal@amd.com>,
        Guenter Roeck <groeck@chromium.org>,
        linux-kernel <linux-kernel@vger.kernel.org>,
        Benson Leung <bleung@chromium.org>
Content-Type: text/plain; charset="UTF-8"
Sender: linux-kernel-owner@vger.kernel.org
Precedence: bulk

On Mon, Nov 25, 2019 at 8:45 AM Enric Balletbo i Serra
<enric.balletbo@collabora.com> wrote:
>
> Hi Raul,
>
> Many tanks for sending this patch upstream, some few comments below.
>
> On 21/11/19 19:55, Raul E Rangel wrote:
> > Add the ability to view response codes as well.
> >
> > I renamed the trace event from cros_ec_cmd to cros_ec_request and added
> > a cros_ec_response.
> >
> > Example:
> > $ echo 1 > /sys/kernel/debug/tracing/events/cros_ec/enable
> > $ cat /sys/kernel/debug/tracing/trace
> >
> > cros_ec_request: version: 1, command: EC_CMD_CONSOLE_READ
> > cros_ec_response: version: 1, command: EC_CMD_CONSOLE_READ, result: EC_RES_SUCCESS, rc: 0
>
> I don't see the advantage of have two traces, one for the request and another
> one for the response. Do you expect get stuck between them?

It might if there is a bug in xfer_fxn, we miss an interrupt, or some
kind of communication problem on the bus (early bring up issues). It's
also possible to compute the latency of a transaction by having both.
The mmc subsystem has both mmc_request_start and mmc_request_done. If
you feel strongly, I can record just the response.
>
> What about just move the trace_cros_ec_cmd after the xfer_fnx call and add the
> results?
>
> Thanks,
>  Enric
>
> > cros_ec_request: version: 0, command: EC_CMD_USB_PD_POWER_INFO
> > cros_ec_response: version: 0, command: EC_CMD_USB_PD_POWER_INFO, result: EC_RES_SUCCESS, rc: 16
> >
> > Signed-off-by: Raul E Rangel <rrangel@chromium.org>
> > ---
> >
> >  drivers/platform/chrome/cros_ec_proto.c |  7 +++++-
> >  drivers/platform/chrome/cros_ec_trace.c | 24 +++++++++++++++++++
> >  drivers/platform/chrome/cros_ec_trace.h | 32 +++++++++++++++++++++++--
> >  3 files changed, 60 insertions(+), 3 deletions(-)
> >
> > diff --git a/drivers/platform/chrome/cros_ec_proto.c b/drivers/platform/chrome/cros_ec_proto.c
> > index bd485ce98a42..ef2229047e0f 100644
> > --- a/drivers/platform/chrome/cros_ec_proto.c
> > +++ b/drivers/platform/chrome/cros_ec_proto.c
> > @@ -54,7 +54,7 @@ static int send_command(struct cros_ec_device *ec_dev,
> >       int ret;
> >       int (*xfer_fxn)(struct cros_ec_device *ec, struct cros_ec_command *msg);
> >
> > -     trace_cros_ec_cmd(msg);
> > +     trace_cros_ec_request(msg);
> >
> >       if (ec_dev->proto_version > 2)
> >               xfer_fxn = ec_dev->pkt_xfer;
> > @@ -73,6 +73,8 @@ static int send_command(struct cros_ec_device *ec_dev,
> >       }
> >
> >       ret = (*xfer_fxn)(ec_dev, msg);
> > +
> > +     trace_cros_ec_response(msg, ret);
> >       if (msg->result == EC_RES_IN_PROGRESS) {
> >               int i;
> >               struct cros_ec_command *status_msg;
> > @@ -95,7 +97,10 @@ static int send_command(struct cros_ec_device *ec_dev,
> >               for (i = 0; i < EC_COMMAND_RETRIES; i++) {
> >                       usleep_range(10000, 11000);
> >
> > +                     trace_cros_ec_request(status_msg);
> >                       ret = (*xfer_fxn)(ec_dev, status_msg);
> > +                     trace_cros_ec_response(status_msg, ret);
> > +
> >                       if (ret == -EAGAIN)
> >                               continue;
> >                       if (ret < 0)
> > diff --git a/drivers/platform/chrome/cros_ec_trace.c b/drivers/platform/chrome/cros_ec_trace.c
> > index 6f80ff4532ae..28eb94d99ba2 100644
> > --- a/drivers/platform/chrome/cros_ec_trace.c
> > +++ b/drivers/platform/chrome/cros_ec_trace.c
> > @@ -120,5 +120,29 @@
> >       TRACE_SYMBOL(EC_CMD_PD_GET_LOG_ENTRY), \
> >       TRACE_SYMBOL(EC_CMD_USB_PD_MUX_INFO)
> >
> > +// See enum ec_status
>
> Use the C comment style, please.
I used // because that was the comment format used above `#define
EC_CMDS`. Do you want me to change that as well?

Thanks