From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1751953AbbIOW0M (ORCPT ); Tue, 15 Sep 2015 18:26:12 -0400 Received: from mail-yk0-f175.google.com ([209.85.160.175]:34890 "EHLO mail-yk0-f175.google.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1751810AbbIOW0K convert rfc822-to-8bit (ORCPT ); Tue, 15 Sep 2015 18:26:10 -0400 MIME-Version: 1.0 X-Originating-IP: [95.23.51.209] In-Reply-To: <55F87E92.8000609@collabora.co.uk> References: <1442234049-18637-1-git-send-email-emilio.lopez@collabora.co.uk> <1442234049-18637-3-git-send-email-emilio.lopez@collabora.co.uk> <55F86E74.9010907@collabora.co.uk> <55F87E92.8000609@collabora.co.uk> Date: Wed, 16 Sep 2015 00:26:09 +0200 Message-ID: Subject: Re: [PATCH v2 2/3] platform/chrome: Support reading/writing the vboot context From: Javier Martinez Canillas To: =?UTF-8?Q?Emilio_L=C3=B3pez?= Cc: Greg Kroah-Hartman , Olof Johansson , Kukjin Kim , =?UTF-8?Q?Krzysztof_Koz=C5=82owski?= , Guenter Roeck , Linux Kernel , "devicetree@vger.kernel.org" , "linux-arm-kernel@lists.infradead.org" , "linux-samsung-soc@vger.kernel.org" Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8BIT Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org Hello Emilio, On Tue, Sep 15, 2015 at 10:24 PM, Emilio López 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 > > > Ok, thanks! > > Emilio Best regards, Javier From mboxrd@z Thu Jan 1 00:00:00 1970 From: javier@dowhile0.org (Javier Martinez Canillas) Date: Wed, 16 Sep 2015 00:26:09 +0200 Subject: [PATCH v2 2/3] platform/chrome: Support reading/writing the vboot context In-Reply-To: <55F87E92.8000609@collabora.co.uk> References: <1442234049-18637-1-git-send-email-emilio.lopez@collabora.co.uk> <1442234049-18637-3-git-send-email-emilio.lopez@collabora.co.uk> <55F86E74.9010907@collabora.co.uk> <55F87E92.8000609@collabora.co.uk> Message-ID: To: linux-arm-kernel@lists.infradead.org List-Id: linux-arm-kernel.lists.infradead.org Hello Emilio, On Tue, Sep 15, 2015 at 10:24 PM, Emilio L?pez 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 > > > Ok, thanks! > > Emilio Best regards, Javier