From: Javier Martinez Canillas <javier@dowhile0.org>
To: "Emilio López" <emilio.lopez@collabora.co.uk>
Cc: "Greg Kroah-Hartman" <gregkh@linuxfoundation.org>,
"Olof Johansson" <olof@lixom.net>,
"Kukjin Kim" <kgene@kernel.org>,
"Krzysztof Kozłowski" <k.kozlowski@samsung.com>,
"Guenter Roeck" <linux@roeck-us.net>,
"Linux Kernel" <linux-kernel@vger.kernel.org>,
"devicetree@vger.kernel.org" <devicetree@vger.kernel.org>,
"linux-arm-kernel@lists.infradead.org"
<linux-arm-kernel@lists.infradead.org>,
"linux-samsung-soc@vger.kernel.org"
<linux-samsung-soc@vger.kernel.org>
Subject: Re: [PATCH v2 2/3] platform/chrome: Support reading/writing the vboot context
Date: Wed, 16 Sep 2015 00:26:09 +0200 [thread overview]
Message-ID: <CABxcv=kqigrfX3ruOUW=wJU1cAs1DmN41AmrUjYRB5f0phhy7g@mail.gmail.com> (raw)
In-Reply-To: <55F87E92.8000609@collabora.co.uk>
Hello Emilio,
On Tue, Sep 15, 2015 at 10:24 PM, Emilio López
<emilio.lopez@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@osg.samsung.com>
>
>
> Ok, thanks!
>
> Emilio
Best regards,
Javier
WARNING: multiple messages have this Message-ID (diff)
From: javier@dowhile0.org (Javier Martinez Canillas)
To: linux-arm-kernel@lists.infradead.org
Subject: [PATCH v2 2/3] platform/chrome: Support reading/writing the vboot context
Date: Wed, 16 Sep 2015 00:26:09 +0200 [thread overview]
Message-ID: <CABxcv=kqigrfX3ruOUW=wJU1cAs1DmN41AmrUjYRB5f0phhy7g@mail.gmail.com> (raw)
In-Reply-To: <55F87E92.8000609@collabora.co.uk>
Hello Emilio,
On Tue, Sep 15, 2015 at 10:24 PM, Emilio L?pez
<emilio.lopez@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@osg.samsung.com>
>
>
> Ok, thanks!
>
> Emilio
Best regards,
Javier
next prev parent reply other threads:[~2015-09-15 22:26 UTC|newest]
Thread overview: 28+ messages / expand[flat|nested] mbox.gz Atom feed top
2015-09-14 12:34 [PATCH v2 0/3] platform/chrome: vboot context support Emilio López
2015-09-14 12:34 ` Emilio López
2015-09-14 12:34 ` Emilio López
2015-09-14 12:34 ` [PATCH v2 1/3] sysfs: Support is_visible() on binary attributes Emilio López
2015-09-14 12:34 ` Emilio López
2015-09-14 15:33 ` Guenter Roeck
2015-09-14 15:33 ` Guenter Roeck
2015-09-14 12:34 ` [PATCH v2 2/3] platform/chrome: Support reading/writing the vboot context Emilio López
2015-09-14 12:34 ` Emilio López
2015-09-15 13:47 ` Javier Martinez Canillas
2015-09-15 13:47 ` Javier Martinez Canillas
2015-09-15 19:16 ` Emilio López
2015-09-15 19:16 ` Emilio López
2015-09-15 19:43 ` Javier Martinez Canillas
2015-09-15 19:43 ` Javier Martinez Canillas
2015-09-15 20:22 ` Greg Kroah-Hartman
2015-09-15 20:22 ` Greg Kroah-Hartman
2015-09-15 20:24 ` Emilio López
2015-09-15 20:24 ` Emilio López
2015-09-15 20:24 ` Emilio López
2015-09-15 22:26 ` Javier Martinez Canillas [this message]
2015-09-15 22:26 ` Javier Martinez Canillas
2015-09-14 12:34 ` [PATCH v2 3/3] ARM: dts: Enable EC vboot context support on Peach boards Emilio López
2015-09-14 12:34 ` Emilio López
2015-09-15 0:00 ` Krzysztof Kozlowski
2015-09-15 0:00 ` Krzysztof Kozlowski
2015-09-15 13:49 ` Javier Martinez Canillas
2015-09-15 13:49 ` Javier Martinez Canillas
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to='CABxcv=kqigrfX3ruOUW=wJU1cAs1DmN41AmrUjYRB5f0phhy7g@mail.gmail.com' \
--to=javier@dowhile0.org \
--cc=devicetree@vger.kernel.org \
--cc=emilio.lopez@collabora.co.uk \
--cc=gregkh@linuxfoundation.org \
--cc=k.kozlowski@samsung.com \
--cc=kgene@kernel.org \
--cc=linux-arm-kernel@lists.infradead.org \
--cc=linux-kernel@vger.kernel.org \
--cc=linux-samsung-soc@vger.kernel.org \
--cc=linux@roeck-us.net \
--cc=olof@lixom.net \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.