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

[Shawn N]

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@xxxxxxxxxxxx> 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@xxxxxxxxxx> 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