[PATCH v2 2/3] platform/chrome: Support reading/writing the vboot context

[Javier Martinez Canillas]

Hello Emilio,

On Tue, Sep 15, 2015 at 10:24 PM, Emilio López
<emilio.lopez at collabora.co.uk> wrote:

[snip]

>>>>> +
>>>>> +       params = (struct ec_params_vbnvcontext *)msg->data;
>>>>> +       params->op = EC_VBNV_CONTEXT_OP_READ;
>>>>> +
>>>>> +       msg->version = EC_VER_VBNV_CONTEXT;
>>>>> +       msg->command = EC_CMD_VBNV_CONTEXT;
>>>>> +       msg->outsize = sizeof(params->op);
>>>>
>>>>
>>>>
>>>> Shouldn't this be para_sz ? Since you are sending to the EC the whole
>>>> struct ec_params_vbnvcontext and not only the op field.
>>>>
>>>> Or if the EC only expects to get the u32 op field, then I think your
>>>> max payload calculation is not correct.
>>>
>>>
>>>
>>> The params struct is the same for both read and write ops, so it has the
>>> op
>>
>>
>> That's not true, struct ec_response_vbnvcontext has only the block
>> field while struct ec_param_vbnvcontext has both the op and block
>> fields.
>
>
> The former is a response struct, not a params struct.
>

I misread read/write as request/response in the previous email, sorry
about that.

>>> flag and a buffer for the write op. During the read op I believe there's
>>> no
>>> need to send this potentially-garbage-filled buffer to the EC, so outsize
>>> is
>>> set accordingly here.
>>
>>
>> Yes, I agree with you but then as I mentioned I think your payload
>> calculation is wrong since you want instead max(sizeof(struct
>> ec_response_vbnvcontext), sizeof(param->op)). Otherwise you are
>> allocating 4 bytes more than needed.
>
>
> Yeah, I can see that. If I do that though, max(...) would be less than the
> size of the full params struct, and casting data to it could lead to subtle
> bugs in the future. I can change it and add a comment mentioning this, deal?
>

But by setting outsize to sizeof(params->op) you are allocating less
than the params struct anyways in the transport driver. Take a look
for example to cros_ec_cmd_xfer_i2c():

http://lxr.free-electrons.com/source/drivers/mfd/cros_ec_i2c.c#L187

But I don't have a strong opinion on this tbh, I was just pointing out
that it's strange that max(insize,outsize) does not match
msg->{insize,outsize}.

> (...)
>
>> with the needed changes, feel free to add my:
>>
>> Reviewed-by: Javier Martinez Canillas <javier at osg.samsung.com>
>
>
> Ok, thanks!
>
> Emilio

Best regards,
Javier