Groups | Search | Server Info | Keyboard shortcuts | Login | Register [http] [https] [nntp] [nntps]

Groups > linux.kernel > #1734920 > unrolled thread

Re: [PATCH v3] platform/chrome: Use proper protocol transfer function

Started byJon Hunter <jonathanh@nvidia.com>
First post2017-09-19 15:50 +0200
Last post2017-09-26 17:50 +0200
Articles 10 — 4 participants

Back to article view | Back to linux.kernel

This discussion starts older than the indexed window; earlier articles aren't shown. The article labeled Started by below is the oldest one visible, not the original post.

Contents

  Re: [PATCH v3] platform/chrome: Use proper protocol transfer function Jon Hunter <jonathanh@nvidia.com> - 2017-09-19 15:50 +0200
    Re: [PATCH v3] platform/chrome: Use proper protocol transfer function Shawn N <shawnn@chromium.org> - 2017-09-19 16:20 +0200
      Re: [PATCH v3] platform/chrome: Use proper protocol transfer function Jon Hunter <jonathanh@nvidia.com> - 2017-09-19 18:50 +0200
        Re: [PATCH v3] platform/chrome: Use proper protocol transfer function Shawn N <shawnn@chromium.org> - 2017-09-19 19:10 +0200
        Re: [PATCH v3] platform/chrome: Use proper protocol transfer function Brian Norris <briannorris@chromium.org> - 2017-09-19 19:20 +0200
          Re: [PATCH v3] platform/chrome: Use proper protocol transfer function Shawn N <shawnn@chromium.org> - 2017-09-20 08:10 +0200
            Re: [PATCH v3] platform/chrome: Use proper protocol transfer function Brian Norris <briannorris@chromium.org> - 2017-09-20 08:20 +0200
              Re: [PATCH v3] platform/chrome: Use proper protocol transfer function Shawn N <shawnn@google.com> - 2017-09-20 22:30 +0200
                Re: [PATCH v3] platform/chrome: Use proper protocol transfer function Shawn N <shawnn@chromium.org> - 2017-09-26 01:20 +0200
                  Re: [PATCH v3] platform/chrome: Use proper protocol transfer function Jon Hunter <jonathanh@nvidia.com> - 2017-09-26 17:50 +0200

#1734920 — Re: [PATCH v3] platform/chrome: Use proper protocol transfer function

FromJon Hunter <jonathanh@nvidia.com>
Date2017-09-19 15:50 +0200
SubjectRe: [PATCH v3] platform/chrome: Use proper protocol transfer function
Message-ID<urr4u-66a-23@gated-at.bofh.it>
Hi Brian,

On 08/09/17 21:50, Brian Norris wrote:
> From: Shawn Nematbakhsh <shawnn@chromium.org>
> 
> pkt_xfer should be used for protocol v3, and cmd_xfer otherwise. We had
> one instance of these functions correct, but not the second, fall-back
> case. We use the fall-back only when the first command returns an
> IN_PROGRESS status, which is only used on some EC firmwares where we
> don't want to constantly poll the bus, but instead back off and
> sleep/retry for a little while.
> 
> Fixes: 2c7589af3c4d ("mfd: cros_ec: add proto v3 skeleton")
> Signed-off-by: Shawn Nematbakhsh <shawnn@chromium.org>
> Signed-off-by: Brian Norris <briannorris@chromium.org>
> Reviewed-by: Javier Martinez Canillas <javier@osg.samsung.com>
> ---
> v3:
>  * Added Javier's reviewed tag
>  * It's been > 8 months since [1], so why not? And hey, Benson's officially in
>    MAINTAINERS now! Too bad no one told me.
> 
>  [1] https://patchwork.kernel.org/patch/9450633/
> 
> 
> v2:
>  * Add Benson in 'To:'
>  * make subject prefix more obvious
> 
>  drivers/platform/chrome/cros_ec_proto.c | 8 +++++---
>  1 file changed, 5 insertions(+), 3 deletions(-)
> 
> diff --git a/drivers/platform/chrome/cros_ec_proto.c b/drivers/platform/chrome/cros_ec_proto.c
> index 8dfa7fcb1248..e7bbdf947bbc 100644
> --- a/drivers/platform/chrome/cros_ec_proto.c
> +++ b/drivers/platform/chrome/cros_ec_proto.c
> @@ -60,12 +60,14 @@ static int send_command(struct cros_ec_device *ec_dev,
>  			struct cros_ec_command *msg)
>  {
>  	int ret;
> +	int (*xfer_fxn)(struct cros_ec_device *ec, struct cros_ec_command *msg);
>  
>  	if (ec_dev->proto_version > 2)
> -		ret = ec_dev->pkt_xfer(ec_dev, msg);
> +		xfer_fxn = ec_dev->pkt_xfer;
>  	else
> -		ret = ec_dev->cmd_xfer(ec_dev, msg);
> +		xfer_fxn = ec_dev->cmd_xfer;
>  
> +	ret = (*xfer_fxn)(ec_dev, msg);
>  	if (msg->result == EC_RES_IN_PROGRESS) {
>  		int i;
>  		struct cros_ec_command *status_msg;
> @@ -88,7 +90,7 @@ static int send_command(struct cros_ec_device *ec_dev,
>  		for (i = 0; i < EC_COMMAND_RETRIES; i++) {
>  			usleep_range(10000, 11000);
>  
> -			ret = ec_dev->cmd_xfer(ec_dev, status_msg);
> +			ret = (*xfer_fxn)(ec_dev, status_msg);
>  			if (ret < 0)
>  				break;
>  

Tegra124 Nyan-Big is currently crashing during boot with -next [0] and
bisect is pointing to this commit. Reverting the above on top of -next
does allow the board to boot successfully. Looks like this board is
proto_version 3 but I have not looked into this any further. Let me know
if you have any thoughts.

Cheers
Jon

[0]
https://nvtb.github.io//linux-next/test_next-20170919/20170918213034/boot/tegra124-nyan-big/tegra124-nyan-big/tegra_defconfig_log.txt


-- 
nvpublic

[toc] | [next] | [standalone]

#1734945

FromShawn N <shawnn@chromium.org>
Date2017-09-19 16:20 +0200
Message-ID<urrxw-6uO-9@gated-at.bofh.it>
In reply to#1734920
On Tue, Sep 19, 2017 at 6:44 AM, Jon Hunter <jonathanh@nvidia.com> wrote:
>
> Hi Brian,
>
> On 08/09/17 21:50, Brian Norris wrote:
> > From: Shawn Nematbakhsh <shawnn@chromium.org>
> >
> > pkt_xfer should be used for protocol v3, and cmd_xfer otherwise. We had
> > one instance of these functions correct, but not the second, fall-back
> > case. We use the fall-back only when the first command returns an
> > IN_PROGRESS status, which is only used on some EC firmwares where we
> > don't want to constantly poll the bus, but instead back off and
> > sleep/retry for a little while.
> >
> > Fixes: 2c7589af3c4d ("mfd: cros_ec: add proto v3 skeleton")
> > Signed-off-by: Shawn Nematbakhsh <shawnn@chromium.org>
> > Signed-off-by: Brian Norris <briannorris@chromium.org>
> > Reviewed-by: Javier Martinez Canillas <javier@osg.samsung.com>
> > ---
> > v3:
> >  * Added Javier's reviewed tag
> >  * It's been > 8 months since [1], so why not? And hey, Benson's officially in
> >    MAINTAINERS now! Too bad no one told me.
> >
> >  [1] https://patchwork.kernel.org/patch/9450633/
> >
> >
> > v2:
> >  * Add Benson in 'To:'
> >  * make subject prefix more obvious
> >
> >  drivers/platform/chrome/cros_ec_proto.c | 8 +++++---
> >  1 file changed, 5 insertions(+), 3 deletions(-)
> >
> > diff --git a/drivers/platform/chrome/cros_ec_proto.c b/drivers/platform/chrome/cros_ec_proto.c
> > index 8dfa7fcb1248..e7bbdf947bbc 100644
> > --- a/drivers/platform/chrome/cros_ec_proto.c
> > +++ b/drivers/platform/chrome/cros_ec_proto.c
> > @@ -60,12 +60,14 @@ static int send_command(struct cros_ec_device *ec_dev,
> >                       struct cros_ec_command *msg)
> >  {
> >       int ret;
> > +     int (*xfer_fxn)(struct cros_ec_device *ec, struct cros_ec_command *msg);
> >
> >       if (ec_dev->proto_version > 2)
> > -             ret = ec_dev->pkt_xfer(ec_dev, msg);
> > +             xfer_fxn = ec_dev->pkt_xfer;
> >       else
> > -             ret = ec_dev->cmd_xfer(ec_dev, msg);
> > +             xfer_fxn = ec_dev->cmd_xfer;
> >
> > +     ret = (*xfer_fxn)(ec_dev, msg);
> >       if (msg->result == EC_RES_IN_PROGRESS) {
> >               int i;
> >               struct cros_ec_command *status_msg;
> > @@ -88,7 +90,7 @@ static int send_command(struct cros_ec_device *ec_dev,
> >               for (i = 0; i < EC_COMMAND_RETRIES; i++) {
> >                       usleep_range(10000, 11000);
> >
> > -                     ret = ec_dev->cmd_xfer(ec_dev, status_msg);
> > +                     ret = (*xfer_fxn)(ec_dev, status_msg);
> >                       if (ret < 0)
> >                               break;
> >
>
> Tegra124 Nyan-Big is currently crashing during boot with -next [0] and
> bisect is pointing to this commit. Reverting the above on top of -next
> does allow the board to boot successfully. Looks like this board is
> proto_version 3 but I have not looked into this any further. Let me know
> if you have any thoughts.


Thanks for the bug report, I'll look into this today.

> [    1.502497] kernel BUG at drivers/platform/chrome/cros_ec_proto.c:34!
> 34 BUG_ON(ec_dev->proto_version != EC_HOST_REQUEST_VERSION);

So, ec_dev->proto_version > 3? That doesn't seem right.

>
>
> Cheers
> Jon
>
> [0]
> https://nvtb.github.io//linux-next/test_next-20170919/20170918213034/boot/tegra124-nyan-big/tegra124-nyan-big/tegra_defconfig_log.txt
>
>
> --
> nvpublic

[toc] | [prev] | [next] | [standalone]

#1735101

FromJon Hunter <jonathanh@nvidia.com>
Date2017-09-19 18:50 +0200
Message-ID<urtSG-7OZ-7@gated-at.bofh.it>
In reply to#1734945

On 19/09/17 15:09, Shawn N wrote:
> On Tue, Sep 19, 2017 at 6:44 AM, Jon Hunter <jonathanh@nvidia.com> wrote:
>>
>> Hi Brian,
>>
>> On 08/09/17 21:50, Brian Norris wrote:
>>> From: Shawn Nematbakhsh <shawnn@chromium.org>
>>>
>>> pkt_xfer should be used for protocol v3, and cmd_xfer otherwise. We had
>>> one instance of these functions correct, but not the second, fall-back
>>> case. We use the fall-back only when the first command returns an
>>> IN_PROGRESS status, which is only used on some EC firmwares where we
>>> don't want to constantly poll the bus, but instead back off and
>>> sleep/retry for a little while.
>>>
>>> Fixes: 2c7589af3c4d ("mfd: cros_ec: add proto v3 skeleton")
>>> Signed-off-by: Shawn Nematbakhsh <shawnn@chromium.org>
>>> Signed-off-by: Brian Norris <briannorris@chromium.org>
>>> Reviewed-by: Javier Martinez Canillas <javier@osg.samsung.com>
>>> ---
>>> v3:
>>>  * Added Javier's reviewed tag
>>>  * It's been > 8 months since [1], so why not? And hey, Benson's officially in
>>>    MAINTAINERS now! Too bad no one told me.
>>>
>>>  [1] https://patchwork.kernel.org/patch/9450633/
>>>
>>>
>>> v2:
>>>  * Add Benson in 'To:'
>>>  * make subject prefix more obvious
>>>
>>>  drivers/platform/chrome/cros_ec_proto.c | 8 +++++---
>>>  1 file changed, 5 insertions(+), 3 deletions(-)
>>>
>>> diff --git a/drivers/platform/chrome/cros_ec_proto.c b/drivers/platform/chrome/cros_ec_proto.c
>>> index 8dfa7fcb1248..e7bbdf947bbc 100644
>>> --- a/drivers/platform/chrome/cros_ec_proto.c
>>> +++ b/drivers/platform/chrome/cros_ec_proto.c
>>> @@ -60,12 +60,14 @@ static int send_command(struct cros_ec_device *ec_dev,
>>>                       struct cros_ec_command *msg)
>>>  {
>>>       int ret;
>>> +     int (*xfer_fxn)(struct cros_ec_device *ec, struct cros_ec_command *msg);
>>>
>>>       if (ec_dev->proto_version > 2)
>>> -             ret = ec_dev->pkt_xfer(ec_dev, msg);
>>> +             xfer_fxn = ec_dev->pkt_xfer;
>>>       else
>>> -             ret = ec_dev->cmd_xfer(ec_dev, msg);
>>> +             xfer_fxn = ec_dev->cmd_xfer;u
>>>
>>> +     ret = (*xfer_fxn)(ec_dev, msg);
>>>       if (msg->result == EC_RES_IN_PROGRESS) {
>>>               int i;
>>>               struct cros_ec_command *status_msg;
>>> @@ -88,7 +90,7 @@ static int send_command(struct cros_ec_device *ec_dev,
>>>               for (i = 0; i < EC_COMMAND_RETRIES; i++) {
>>>                       usleep_range(10000, 11000);
>>>
>>> -                     ret = ec_dev->cmd_xfer(ec_dev, status_msg);
>>> +                     ret = (*xfer_fxn)(ec_dev, status_msg);
>>>                       if (ret < 0)
>>>                               break;
>>>
>>
>> Tegra124 Nyan-Big is currently crashing during boot with -next [0] and
>> bisect is pointing to this commit. Reverting the above on top of -next
>> does allow the board to boot successfully. Looks like this board is
>> proto_version 3 but I have not looked into this any further. Let me know
>> if you have any thoughts.
> 
> 
> Thanks for the bug report, I'll look into this today.
> 
>> [    1.502497] kernel BUG at drivers/platform/chrome/cros_ec_proto.c:34!
>> 34 BUG_ON(ec_dev->proto_version != EC_HOST_REQUEST_VERSION);
> 
> So, ec_dev->proto_version > 3? That doesn't seem right.

You mean != 3, but yes. Looks like an initialisation problem, because if I 
add the following WARNING ...

diff --git a/drivers/platform/chrome/cros_ec_proto.c b/drivers/platform/chrome/cros_ec_proto.c
index e7bbdf947bbc..ad3b3a1e8d54 100644
--- a/drivers/platform/chrome/cros_ec_proto.c
+++ b/drivers/platform/chrome/cros_ec_proto.c
@@ -31,6 +31,7 @@ static int prepare_packet(struct cros_ec_device *ec_dev,
        int i;
        u8 csum = 0;
 
+       WARN(ec_dev->proto_version != EC_HOST_REQUEST_VERSION, "%d != %d", ec_dev->proto_version, EC_HOST_REQUEST_VERSION);
        BUG_ON(ec_dev->proto_version != EC_HOST_REQUEST_VERSION);
        BUG_ON(msg->outsize + sizeof(*request) > ec_dev->dout_size);
 
... then I see ...

[    1.502495] WARNING: CPU: 0 PID: 1 at drivers/platform/chrome/cros_ec_proto.c:35 cros_ec_prepare_tx+0x190/0x1a8
[    1.512566] 65535 != 3

Any chance this is being called before the version is initialised?

Cheers
Jon

-- 
nvpublic

[toc] | [prev] | [next] | [standalone]

#1735107

FromShawn N <shawnn@chromium.org>
Date2017-09-19 19:10 +0200
Message-ID<uruc1-8cm-5@gated-at.bofh.it>
In reply to#1735101
On Tue, Sep 19, 2017 at 9:39 AM, Jon Hunter <jonathanh@nvidia.com> wrote:
>
>
> On 19/09/17 15:09, Shawn N wrote:
>> On Tue, Sep 19, 2017 at 6:44 AM, Jon Hunter <jonathanh@nvidia.com> wrote:
>>>
>>> Hi Brian,
>>>
>>> On 08/09/17 21:50, Brian Norris wrote:
>>>> From: Shawn Nematbakhsh <shawnn@chromium.org>
>>>>
>>>> pkt_xfer should be used for protocol v3, and cmd_xfer otherwise. We had
>>>> one instance of these functions correct, but not the second, fall-back
>>>> case. We use the fall-back only when the first command returns an
>>>> IN_PROGRESS status, which is only used on some EC firmwares where we
>>>> don't want to constantly poll the bus, but instead back off and
>>>> sleep/retry for a little while.
>>>>
>>>> Fixes: 2c7589af3c4d ("mfd: cros_ec: add proto v3 skeleton")
>>>> Signed-off-by: Shawn Nematbakhsh <shawnn@chromium.org>
>>>> Signed-off-by: Brian Norris <briannorris@chromium.org>
>>>> Reviewed-by: Javier Martinez Canillas <javier@osg.samsung.com>
>>>> ---
>>>> v3:
>>>>  * Added Javier's reviewed tag
>>>>  * It's been > 8 months since [1], so why not? And hey, Benson's officially in
>>>>    MAINTAINERS now! Too bad no one told me.
>>>>
>>>>  [1] https://patchwork.kernel.org/patch/9450633/
>>>>
>>>>
>>>> v2:
>>>>  * Add Benson in 'To:'
>>>>  * make subject prefix more obvious
>>>>
>>>>  drivers/platform/chrome/cros_ec_proto.c | 8 +++++---
>>>>  1 file changed, 5 insertions(+), 3 deletions(-)
>>>>
>>>> diff --git a/drivers/platform/chrome/cros_ec_proto.c b/drivers/platform/chrome/cros_ec_proto.c
>>>> index 8dfa7fcb1248..e7bbdf947bbc 100644
>>>> --- a/drivers/platform/chrome/cros_ec_proto.c
>>>> +++ b/drivers/platform/chrome/cros_ec_proto.c
>>>> @@ -60,12 +60,14 @@ static int send_command(struct cros_ec_device *ec_dev,
>>>>                       struct cros_ec_command *msg)
>>>>  {
>>>>       int ret;
>>>> +     int (*xfer_fxn)(struct cros_ec_device *ec, struct cros_ec_command *msg);
>>>>
>>>>       if (ec_dev->proto_version > 2)
>>>> -             ret = ec_dev->pkt_xfer(ec_dev, msg);
>>>> +             xfer_fxn = ec_dev->pkt_xfer;
>>>>       else
>>>> -             ret = ec_dev->cmd_xfer(ec_dev, msg);
>>>> +             xfer_fxn = ec_dev->cmd_xfer;u
>>>>
>>>> +     ret = (*xfer_fxn)(ec_dev, msg);
>>>>       if (msg->result == EC_RES_IN_PROGRESS) {
>>>>               int i;
>>>>               struct cros_ec_command *status_msg;
>>>> @@ -88,7 +90,7 @@ static int send_command(struct cros_ec_device *ec_dev,
>>>>               for (i = 0; i < EC_COMMAND_RETRIES; i++) {
>>>>                       usleep_range(10000, 11000);
>>>>
>>>> -                     ret = ec_dev->cmd_xfer(ec_dev, status_msg);
>>>> +                     ret = (*xfer_fxn)(ec_dev, status_msg);
>>>>                       if (ret < 0)
>>>>                               break;
>>>>
>>>
>>> Tegra124 Nyan-Big is currently crashing during boot with -next [0] and
>>> bisect is pointing to this commit. Reverting the above on top of -next
>>> does allow the board to boot successfully. Looks like this board is
>>> proto_version 3 but I have not looked into this any further. Let me know
>>> if you have any thoughts.
>>
>>
>> Thanks for the bug report, I'll look into this today.
>>
>>> [    1.502497] kernel BUG at drivers/platform/chrome/cros_ec_proto.c:34!
>>> 34 BUG_ON(ec_dev->proto_version != EC_HOST_REQUEST_VERSION);
>>
>> So, ec_dev->proto_version > 3? That doesn't seem right.
>
> You mean != 3, but yes. Looks like an initialisation problem, because if I
> add the following WARNING ...

I meant > 3 because we check for > 2 in send_command().

>
> diff --git a/drivers/platform/chrome/cros_ec_proto.c b/drivers/platform/chrome/cros_ec_proto.c
> index e7bbdf947bbc..ad3b3a1e8d54 100644
> --- a/drivers/platform/chrome/cros_ec_proto.c
> +++ b/drivers/platform/chrome/cros_ec_proto.c
> @@ -31,6 +31,7 @@ static int prepare_packet(struct cros_ec_device *ec_dev,
>         int i;
>         u8 csum = 0;
>
> +       WARN(ec_dev->proto_version != EC_HOST_REQUEST_VERSION, "%d != %d", ec_dev->proto_version, EC_HOST_REQUEST_VERSION);
>         BUG_ON(ec_dev->proto_version != EC_HOST_REQUEST_VERSION);
>         BUG_ON(msg->outsize + sizeof(*request) > ec_dev->dout_size);
>
> ... then I see ...
>
> [    1.502495] WARNING: CPU: 0 PID: 1 at drivers/platform/chrome/cros_ec_proto.c:35 cros_ec_prepare_tx+0x190/0x1a8
> [    1.512566] 65535 != 3
>
> Any chance this is being called before the version is initialised?

It's initialized in cros_ec_query_all().

Considering your trace shows (send_command+0x20/0xd8) as a caller, I'm
guessing that we die on the first call to (*xfer_fxn):

+     ret = (*xfer_fxn)(ec_dev, msg);

That part of the change should be a NOP, I only added a function
pointer so we wouldn't have to re-check protocol_version later. The
syntax looks fine to me even after re-checking, but maybe I missed
something. Let me test on my side.

>
> Cheers
> Jon
>
> --
> nvpublic

[toc] | [prev] | [next] | [standalone]

#1735109

FromBrian Norris <briannorris@chromium.org>
Date2017-09-19 19:20 +0200
Message-ID<urulI-8fP-1@gated-at.bofh.it>
In reply to#1735101
Hi Jon,

On Tue, Sep 19, 2017 at 05:39:56PM +0100, Jon Hunter wrote:
> On 19/09/17 15:09, Shawn N wrote:
> > On Tue, Sep 19, 2017 at 6:44 AM, Jon Hunter <jonathanh@nvidia.com> wrote:
> >> Tegra124 Nyan-Big is currently crashing during boot with -next [0] and
> >> bisect is pointing to this commit. Reverting the above on top of -next
> >> does allow the board to boot successfully. Looks like this board is
> >> proto_version 3 but I have not looked into this any further. Let me know
> >> if you have any thoughts.
> > 
> > 
> > Thanks for the bug report, I'll look into this today.

Yes, thanks!

> >> [    1.502497] kernel BUG at drivers/platform/chrome/cros_ec_proto.c:34!
> >> 34 BUG_ON(ec_dev->proto_version != EC_HOST_REQUEST_VERSION);
> > 
> > So, ec_dev->proto_version > 3? That doesn't seem right.
> 
> You mean != 3, but yes. Looks like an initialisation problem, because if I 
> add the following WARNING ...
> 
> diff --git a/drivers/platform/chrome/cros_ec_proto.c b/drivers/platform/chrome/cros_ec_proto.c
> index e7bbdf947bbc..ad3b3a1e8d54 100644
> --- a/drivers/platform/chrome/cros_ec_proto.c
> +++ b/drivers/platform/chrome/cros_ec_proto.c
> @@ -31,6 +31,7 @@ static int prepare_packet(struct cros_ec_device *ec_dev,
>         int i;
>         u8 csum = 0;
>  
> +       WARN(ec_dev->proto_version != EC_HOST_REQUEST_VERSION, "%d != %d", ec_dev->proto_version, EC_HOST_REQUEST_VERSION);
>         BUG_ON(ec_dev->proto_version != EC_HOST_REQUEST_VERSION);
>         BUG_ON(msg->outsize + sizeof(*request) > ec_dev->dout_size);
>  
> ... then I see ...
> 
> [    1.502495] WARNING: CPU: 0 PID: 1 at drivers/platform/chrome/cros_ec_proto.c:35 cros_ec_prepare_tx+0x190/0x1a8
> [    1.512566] 65535 != 3
> 
> Any chance this is being called before the version is initialised?

If it's uninitialized, it should be 0 (the structure is kzalloc'd, and
the call stack you point to clearly shows it's at least been allocated
already). Also, if it's uninitialized, then you should be BUG'ing even
without this patch; the patch you've bisected to is only modifying the
*second* (or later) attempt to send the command, and it's using the same
'ec_dev' structure.

Furthermore, the only assignments to this 'proto_version' field look
like they're only writing one of 0, 2, 3, or

   min(EC_HOST_REQUEST_VERSION, fls(proto_info->protocol_versions) - 1)

. I don't see where 0xffff comes from.

So...is there any chance we've got a heap corruption somewhere?
Somebody's overwriting 'ec_dev->proto_version' accidentally?

Brian

[toc] | [prev] | [next] | [standalone]

#1735539

FromShawn N <shawnn@chromium.org>
Date2017-09-20 08:10 +0200
Message-ID<urGmR-7Am-3@gated-at.bofh.it>
In reply to#1735109
This is failing because our EC_CMD_GET_PROTOCOL_INFO host command is
getting messed up, or the reply buffer is getting corrupted somehow.

               ec_dev->proto_version =
                        min(EC_HOST_REQUEST_VERSION,
                                        fls(proto_info->protocol_versions) - 1);

If proto_info->protocol_versions == 0 then ec_dev->proto_version will
be assigned 0xffff. The logic here seems strange to me, if the EC is
successfully replying to our v3 command then obviously it supports v3
(maybe it will be useful someday if EC_HOST_REQUEST_VERSION is rev'd).
Anyway, we need to figure out what is happening with our
EC_HOST_REQUEST_VERSION host command.

On Tue, Sep 19, 2017 at 10:14 AM, Brian Norris <briannorris@chromium.org> wrote:
> Hi Jon,
>
> On Tue, Sep 19, 2017 at 05:39:56PM +0100, Jon Hunter wrote:
>> On 19/09/17 15:09, Shawn N wrote:
>> > On Tue, Sep 19, 2017 at 6:44 AM, Jon Hunter <jonathanh@nvidia.com> wrote:
>> >> Tegra124 Nyan-Big is currently crashing during boot with -next [0] and
>> >> bisect is pointing to this commit. Reverting the above on top of -next
>> >> does allow the board to boot successfully. Looks like this board is
>> >> proto_version 3 but I have not looked into this any further. Let me know
>> >> if you have any thoughts.
>> >
>> >
>> > Thanks for the bug report, I'll look into this today.
>
> Yes, thanks!
>
>> >> [    1.502497] kernel BUG at drivers/platform/chrome/cros_ec_proto.c:34!
>> >> 34 BUG_ON(ec_dev->proto_version != EC_HOST_REQUEST_VERSION);
>> >
>> > So, ec_dev->proto_version > 3? That doesn't seem right.
>>
>> You mean != 3, but yes. Looks like an initialisation problem, because if I
>> add the following WARNING ...
>>
>> diff --git a/drivers/platform/chrome/cros_ec_proto.c b/drivers/platform/chrome/cros_ec_proto.c
>> index e7bbdf947bbc..ad3b3a1e8d54 100644
>> --- a/drivers/platform/chrome/cros_ec_proto.c
>> +++ b/drivers/platform/chrome/cros_ec_proto.c
>> @@ -31,6 +31,7 @@ static int prepare_packet(struct cros_ec_device *ec_dev,
>>         int i;
>>         u8 csum = 0;
>>
>> +       WARN(ec_dev->proto_version != EC_HOST_REQUEST_VERSION, "%d != %d", ec_dev->proto_version, EC_HOST_REQUEST_VERSION);
>>         BUG_ON(ec_dev->proto_version != EC_HOST_REQUEST_VERSION);
>>         BUG_ON(msg->outsize + sizeof(*request) > ec_dev->dout_size);
>>
>> ... then I see ...
>>
>> [    1.502495] WARNING: CPU: 0 PID: 1 at drivers/platform/chrome/cros_ec_proto.c:35 cros_ec_prepare_tx+0x190/0x1a8
>> [    1.512566] 65535 != 3
>>
>> Any chance this is being called before the version is initialised?
>
> If it's uninitialized, it should be 0 (the structure is kzalloc'd, and
> the call stack you point to clearly shows it's at least been allocated
> already). Also, if it's uninitialized, then you should be BUG'ing even
> without this patch; the patch you've bisected to is only modifying the
> *second* (or later) attempt to send the command, and it's using the same
> 'ec_dev' structure.
>
> Furthermore, the only assignments to this 'proto_version' field look
> like they're only writing one of 0, 2, 3, or
>
>    min(EC_HOST_REQUEST_VERSION, fls(proto_info->protocol_versions) - 1)
>
> . I don't see where 0xffff comes from.
>
> So...is there any chance we've got a heap corruption somewhere?
> Somebody's overwriting 'ec_dev->proto_version' accidentally?
>
> Brian

[toc] | [prev] | [next] | [standalone]

#1735546

FromBrian Norris <briannorris@chromium.org>
Date2017-09-20 08:20 +0200
Message-ID<urGwy-7DX-7@gated-at.bofh.it>
In reply to#1735539
Hi,

On Tue, Sep 19, 2017 at 11:05:38PM -0700, Shawn N wrote:
> This is failing because our EC_CMD_GET_PROTOCOL_INFO host command is
> getting messed up, or the reply buffer is getting corrupted somehow.
> 
>                ec_dev->proto_version =
>                         min(EC_HOST_REQUEST_VERSION,
>                                         fls(proto_info->protocol_versions) - 1);
> 
> If proto_info->protocol_versions == 0 then ec_dev->proto_version will
> be assigned 0xffff. The logic here seems strange to me, if the EC is

Whoops...

> successfully replying to our v3 command then obviously it supports v3
> (maybe it will be useful someday if EC_HOST_REQUEST_VERSION is rev'd).
> Anyway, we need to figure out what is happening with our
> EC_HOST_REQUEST_VERSION host command.
> 
> On Tue, Sep 19, 2017 at 10:14 AM, Brian Norris <briannorris@chromium.org> wrote:
> > Hi Jon,
> >
> > On Tue, Sep 19, 2017 at 05:39:56PM +0100, Jon Hunter wrote:
> >> On 19/09/17 15:09, Shawn N wrote:
...
> > Furthermore, the only assignments to this 'proto_version' field look
> > like they're only writing one of 0, 2, 3, or
> >
> >    min(EC_HOST_REQUEST_VERSION, fls(proto_info->protocol_versions) - 1)
> >
> > . I don't see where 0xffff comes from.

...I'm an idiot. While the rvalue (the expression above) is an int (e.g,
-1), it's getting cast into a uint16_t (ec_dev->proto_version). So
that's where the 0xffff can come from.

Sorry if I misled you Shawn :(

Brian

[toc] | [prev] | [next] | [standalone]

#1736086

FromShawn N <shawnn@google.com>
Date2017-09-20 22:30 +0200
Message-ID<urTN8-7TQ-27@gated-at.bofh.it>
In reply to#1735546
On Tue, Sep 19, 2017 at 11:13 PM, Brian Norris <briannorris@chromium.org> wrote:
> Hi,
>
> On Tue, Sep 19, 2017 at 11:05:38PM -0700, Shawn N wrote:
>> This is failing because our EC_CMD_GET_PROTOCOL_INFO host command is
>> getting messed up, or the reply buffer is getting corrupted somehow.
>>
>>                ec_dev->proto_version =
>>                         min(EC_HOST_REQUEST_VERSION,
>>                                         fls(proto_info->protocol_versions) - 1);
>>

Checking this closer, the first host command we send after we boot the
kernel (EC_CMD_GET_PROTOCOL_INFO) is failing due to protocol error
(see 'SPI rx bad data' / 'SPI not ready' on the EC console). Since
this doesn't seem to happen on the Chromium OS nyan_big release
kernel, I suggest to hook up a logic analyzer and see if the SPI
master is doing something bad.

The error handling in cros_ec_cmd_xfer_spi() is completely wrong and
we return -EAGAIN / EC_RES_IN_PROGRESS, which the caller interprets
"the host command was received by the EC and is currently being
handled, poll status until completion". So the caller polls status
with EC_CMD_GET_COMMS_STATUS, sees no host command is in progress
(which is interpreted to mean "the host command I sent previously has
now successfully completed"), and returns success. The problem here is
that the initial host command was never received at all, and no reply
was ever received, so our reply data is all zero.

Two things need to be fixed here:

1) Find out why the first host command after boot is failing. Probe
SPI pins and see what's going on.
2) Fix error handling so we properly return an error (or properly
retry the entire command) when a protocol error occurs (I made some
attempt in https://chromium-review.googlesource.com/385080/, probably
I should revisit that).

>> If proto_info->protocol_versions == 0 then ec_dev->proto_version will
>> be assigned 0xffff. The logic here seems strange to me, if the EC is
>
> Whoops...
>
>> successfully replying to our v3 command then obviously it supports v3
>> (maybe it will be useful someday if EC_HOST_REQUEST_VERSION is rev'd).
>> Anyway, we need to figure out what is happening with our
>> EC_HOST_REQUEST_VERSION host command.
>>
>> On Tue, Sep 19, 2017 at 10:14 AM, Brian Norris <briannorris@chromium.org> wrote:
>> > Hi Jon,
>> >
>> > On Tue, Sep 19, 2017 at 05:39:56PM +0100, Jon Hunter wrote:
>> >> On 19/09/17 15:09, Shawn N wrote:
> ...
>> > Furthermore, the only assignments to this 'proto_version' field look
>> > like they're only writing one of 0, 2, 3, or
>> >
>> >    min(EC_HOST_REQUEST_VERSION, fls(proto_info->protocol_versions) - 1)
>> >
>> > . I don't see where 0xffff comes from.
>
> ...I'm an idiot. While the rvalue (the expression above) is an int (e.g,
> -1), it's getting cast into a uint16_t (ec_dev->proto_version). So
> that's where the 0xffff can come from.

I saw that before and overlooked it too, so we're both idiots.

>
> Sorry if I misled you Shawn :(
>
> Brian

[toc] | [prev] | [next] | [standalone]

#1739338

FromShawn N <shawnn@chromium.org>
Date2017-09-26 01:20 +0200
Message-ID<utKPp-4Lc-15@gated-at.bofh.it>
In reply to#1736086

[Multipart message — attachments visible in raw view] — view raw

On Wed, Sep 20, 2017 at 1:22 PM, Shawn N <shawnn@google.com> wrote:
> On Tue, Sep 19, 2017 at 11:13 PM, Brian Norris <briannorris@chromium.org> wrote:
>> Hi,
>>
>> On Tue, Sep 19, 2017 at 11:05:38PM -0700, Shawn N wrote:
>>> This is failing because our EC_CMD_GET_PROTOCOL_INFO host command is
>>> getting messed up, or the reply buffer is getting corrupted somehow.
>>>
>>>                ec_dev->proto_version =
>>>                         min(EC_HOST_REQUEST_VERSION,
>>>                                         fls(proto_info->protocol_versions) - 1);
>>>
>
> Checking this closer, the first host command we send after we boot the
> kernel (EC_CMD_GET_PROTOCOL_INFO) is failing due to protocol error
> (see 'SPI rx bad data' / 'SPI not ready' on the EC console). Since
> this doesn't seem to happen on the Chromium OS nyan_big release
> kernel, I suggest to hook up a logic analyzer and see if the SPI
> master is doing something bad.
>
> The error handling in cros_ec_cmd_xfer_spi() is completely wrong and
> we return -EAGAIN / EC_RES_IN_PROGRESS, which the caller interprets
> "the host command was received by the EC and is currently being
> handled, poll status until completion". So the caller polls status
> with EC_CMD_GET_COMMS_STATUS, sees no host command is in progress
> (which is interpreted to mean "the host command I sent previously has
> now successfully completed"), and returns success. The problem here is
> that the initial host command was never received at all, and no reply
> was ever received, so our reply data is all zero.
>
> Two things need to be fixed here:
>
> 1) Find out why the first host command after boot is failing. Probe
> SPI pins and see what's going on.
> 2) Fix error handling so we properly return an error (or properly
> retry the entire command) when a protocol error occurs (I made some
> attempt in https://chromium-review.googlesource.com/385080/, probably
> I should revisit that).

The below patch will fix error handling and will make things mostly
work on nyan_big, because we'll fall back to V2 protocol after the
initial failure. But we should still investigate why we're getting
errors on the first host command. We aren't seeing these errors when
we send commands from firmware, so I suspect something is wrong in
kernel SPI HW initialization that causes the first command to fail.

From: Shawn Nematbakhsh <shawnn@chromium.org>
Date: Mon, 25 Sep 2017 14:32:38 -0700
Subject: [PATCH] mfd: cros ec: spi: Fix "in progress" error signaling

For host commands that take a long time to process, cros ec can return
early by signaling a EC_RES_IN_PROGRESS result. The host must then poll
status with EC_CMD_GET_COMMS_STATUS until completion of the command.

None of the above applies when data link errors are encountered. When
errors such as EC_SPI_PAST_END are encountered during command
transmission, it usually means the command was not received by the EC.
Treating such errors as if they were 'EC_RES_IN_PROGRESS' results is
almost always the wrong decision, and can result in host commands
silently being lost.

Signed-off-by: Shawn Nematbakhsh <shawnn@chromium.org>
---
 drivers/mfd/cros_ec_spi.c | 26 ++++++++++++--------------
 1 file changed, 12 insertions(+), 14 deletions(-)

diff --git a/drivers/mfd/cros_ec_spi.c b/drivers/mfd/cros_ec_spi.c
index c9714072e224..d33e3847e11e 100644
--- a/drivers/mfd/cros_ec_spi.c
+++ b/drivers/mfd/cros_ec_spi.c
@@ -377,6 +377,7 @@ static int cros_ec_pkt_xfer_spi(struct
cros_ec_device *ec_dev,
        u8 *ptr;
        u8 *rx_buf;
        u8 sum;
+       u8 rx_byte;
        int ret = 0, final_ret;

        len = cros_ec_prepare_tx(ec_dev, ec_msg);
@@ -421,25 +422,22 @@ static int cros_ec_pkt_xfer_spi(struct
cros_ec_device *ec_dev,
        if (!ret) {
                /* Verify that EC can process command */
                for (i = 0; i < len; i++) {
-                       switch (rx_buf[i]) {
-                       case EC_SPI_PAST_END:
-                       case EC_SPI_RX_BAD_DATA:
-                       case EC_SPI_NOT_READY:
-                               ret = -EAGAIN;
-                               ec_msg->result = EC_RES_IN_PROGRESS;
-                       default:
+                       rx_byte = rx_buf[i];
+                       if (rx_byte == EC_SPI_PAST_END  ||
+                           rx_byte == EC_SPI_RX_BAD_DATA ||
+                           rx_byte == EC_SPI_NOT_READY) {
+                               ret = -EREMOTEIO;
                                break;
                        }
-                       if (ret)
-                               break;
                }
-               if (!ret)
-                       ret = cros_ec_spi_receive_packet(ec_dev,
-                                       ec_msg->insize + sizeof(*response));
-       } else {
-               dev_err(ec_dev->dev, "spi transfer failed: %d\n", ret);
        }

+       if (!ret)
+               ret = cros_ec_spi_receive_packet(ec_dev,
+                               ec_msg->insize + sizeof(*response));
+       else
+               dev_err(ec_dev->dev, "spi transfer failed: %d\n", ret);
+
        final_ret = terminate_request(ec_dev);

        spi_bus_unlock(ec_spi->spi->master);
-- 
2.12.2

>
>>> If proto_info->protocol_versions == 0 then ec_dev->proto_version will
>>> be assigned 0xffff. The logic here seems strange to me, if the EC is
>>
>> Whoops...
>>
>>> successfully replying to our v3 command then obviously it supports v3
>>> (maybe it will be useful someday if EC_HOST_REQUEST_VERSION is rev'd).
>>> Anyway, we need to figure out what is happening with our
>>> EC_HOST_REQUEST_VERSION host command.
>>>
>>> On Tue, Sep 19, 2017 at 10:14 AM, Brian Norris <briannorris@chromium.org> wrote:
>>> > Hi Jon,
>>> >
>>> > On Tue, Sep 19, 2017 at 05:39:56PM +0100, Jon Hunter wrote:
>>> >> On 19/09/17 15:09, Shawn N wrote:
>> ...
>>> > Furthermore, the only assignments to this 'proto_version' field look
>>> > like they're only writing one of 0, 2, 3, or
>>> >
>>> >    min(EC_HOST_REQUEST_VERSION, fls(proto_info->protocol_versions) - 1)
>>> >
>>> > . I don't see where 0xffff comes from.
>>
>> ...I'm an idiot. While the rvalue (the expression above) is an int (e.g,
>> -1), it's getting cast into a uint16_t (ec_dev->proto_version). So
>> that's where the 0xffff can come from.
>
> I saw that before and overlooked it too, so we're both idiots.
>
>>
>> Sorry if I misled you Shawn :(
>>
>> Brian

[toc] | [prev] | [next] | [standalone]

#1739996

FromJon Hunter <jonathanh@nvidia.com>
Date2017-09-26 17:50 +0200
Message-ID<uu0hs-6CT-17@gated-at.bofh.it>
In reply to#1739338
On 26/09/17 00:15, Shawn N wrote:
> On Wed, Sep 20, 2017 at 1:22 PM, Shawn N <shawnn@google.com> wrote:
>> On Tue, Sep 19, 2017 at 11:13 PM, Brian Norris <briannorris@chromium.org> wrote:
>>> Hi,
>>>
>>> On Tue, Sep 19, 2017 at 11:05:38PM -0700, Shawn N wrote:
>>>> This is failing because our EC_CMD_GET_PROTOCOL_INFO host command is
>>>> getting messed up, or the reply buffer is getting corrupted somehow.
>>>>
>>>>                ec_dev->proto_version =
>>>>                         min(EC_HOST_REQUEST_VERSION,
>>>>                                         fls(proto_info->protocol_versions) - 1);
>>>>
>>
>> Checking this closer, the first host command we send after we boot the
>> kernel (EC_CMD_GET_PROTOCOL_INFO) is failing due to protocol error
>> (see 'SPI rx bad data' / 'SPI not ready' on the EC console). Since
>> this doesn't seem to happen on the Chromium OS nyan_big release
>> kernel, I suggest to hook up a logic analyzer and see if the SPI
>> master is doing something bad.
>>
>> The error handling in cros_ec_cmd_xfer_spi() is completely wrong and
>> we return -EAGAIN / EC_RES_IN_PROGRESS, which the caller interprets
>> "the host command was received by the EC and is currently being
>> handled, poll status until completion". So the caller polls status
>> with EC_CMD_GET_COMMS_STATUS, sees no host command is in progress
>> (which is interpreted to mean "the host command I sent previously has
>> now successfully completed"), and returns success. The problem here is
>> that the initial host command was never received at all, and no reply
>> was ever received, so our reply data is all zero.
>>
>> Two things need to be fixed here:
>>
>> 1) Find out why the first host command after boot is failing. Probe
>> SPI pins and see what's going on.

Yes, I will see if I can look into this.

>> 2) Fix error handling so we properly return an error (or properly
>> retry the entire command) when a protocol error occurs (I made some
>> attempt in https://chromium-review.googlesource.com/385080/, probably
>> I should revisit that).
> 
> The below patch will fix error handling and will make things mostly
> work on nyan_big, because we'll fall back to V2 protocol after the
> initial failure. But we should still investigate why we're getting
> errors on the first host command. We aren't seeing these errors when
> we send commands from firmware, so I suspect something is wrong in
> kernel SPI HW initialization that causes the first command to fail.
> 
> From: Shawn Nematbakhsh <shawnn@chromium.org>
> Date: Mon, 25 Sep 2017 14:32:38 -0700
> Subject: [PATCH] mfd: cros ec: spi: Fix "in progress" error signaling
> 
> For host commands that take a long time to process, cros ec can return
> early by signaling a EC_RES_IN_PROGRESS result. The host must then poll
> status with EC_CMD_GET_COMMS_STATUS until completion of the command.
> 
> None of the above applies when data link errors are encountered. When
> errors such as EC_SPI_PAST_END are encountered during command
> transmission, it usually means the command was not received by the EC.
> Treating such errors as if they were 'EC_RES_IN_PROGRESS' results is
> almost always the wrong decision, and can result in host commands
> silently being lost.
> 
> Signed-off-by: Shawn Nematbakhsh <shawnn@chromium.org>
> ---
>  drivers/mfd/cros_ec_spi.c | 26 ++++++++++++--------------
>  1 file changed, 12 insertions(+), 14 deletions(-)
> 
> diff --git a/drivers/mfd/cros_ec_spi.c b/drivers/mfd/cros_ec_spi.c
> index c9714072e224..d33e3847e11e 100644
> --- a/drivers/mfd/cros_ec_spi.c
> +++ b/drivers/mfd/cros_ec_spi.c
> @@ -377,6 +377,7 @@ static int cros_ec_pkt_xfer_spi(struct
> cros_ec_device *ec_dev,
>         u8 *ptr;
>         u8 *rx_buf;
>         u8 sum;
> +       u8 rx_byte;
>         int ret = 0, final_ret;
> 
>         len = cros_ec_prepare_tx(ec_dev, ec_msg);
> @@ -421,25 +422,22 @@ static int cros_ec_pkt_xfer_spi(struct
> cros_ec_device *ec_dev,
>         if (!ret) {
>                 /* Verify that EC can process command */
>                 for (i = 0; i < len; i++) {
> -                       switch (rx_buf[i]) {
> -                       case EC_SPI_PAST_END:
> -                       case EC_SPI_RX_BAD_DATA:
> -                       case EC_SPI_NOT_READY:
> -                               ret = -EAGAIN;
> -                               ec_msg->result = EC_RES_IN_PROGRESS;
> -                       default:
> +                       rx_byte = rx_buf[i];
> +                       if (rx_byte == EC_SPI_PAST_END  ||
> +                           rx_byte == EC_SPI_RX_BAD_DATA ||
> +                           rx_byte == EC_SPI_NOT_READY) {
> +                               ret = -EREMOTEIO;
>                                 break;
>                         }
> -                       if (ret)
> -                               break;
>                 }
> -               if (!ret)
> -                       ret = cros_ec_spi_receive_packet(ec_dev,
> -                                       ec_msg->insize + sizeof(*response));
> -       } else {
> -               dev_err(ec_dev->dev, "spi transfer failed: %d\n", ret);
>         }
> 
> +       if (!ret)
> +               ret = cros_ec_spi_receive_packet(ec_dev,
> +                               ec_msg->insize + sizeof(*response));
> +       else
> +               dev_err(ec_dev->dev, "spi transfer failed: %d\n", ret);
> +
>         final_ret = terminate_request(ec_dev);
> 
>         spi_bus_unlock(ec_spi->spi->master);
> 

Thanks! Works for me ...

Tested-by: Jon Hunter <jonathanh@nvidia.com>

Cheers
Jon

-- 
nvpublic

[toc] | [prev] | [standalone]

Back to top | Article view | linux.kernel

csiph-web