Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S934031AbbEMLKn (ORCPT ); Wed, 13 May 2015 07:10:43 -0400 Received: from mail-wg0-f44.google.com ([74.125.82.44]:33982 "EHLO mail-wg0-f44.google.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1754476AbbEMLKj (ORCPT ); Wed, 13 May 2015 07:10:39 -0400 Date: Wed, 13 May 2015 12:10:34 +0100 From: Lee Jones To: Javier Martinez Canillas Cc: Samuel Ortiz , Olof Johansson , Doug Anderson , Bill Richardson , Simon Glass , Gwendal Grignou , Stephen Barber , Filipe Brandenburger , Todd Broch , Alexandru M Stan , Heiko Stuebner , linux-samsung-soc@vger.kernel.org, linux-kernel@vger.kernel.org Subject: Re: [PATCH v2 04/10] mfd: cros_ec: Use a zero-length array for command data Message-ID: <20150513111034.GH3394@x1> References: <1431166241-15775-1-git-send-email-javier.martinez@collabora.co.uk> <1431166241-15775-5-git-send-email-javier.martinez@collabora.co.uk> MIME-Version: 1.0 Content-Type: text/plain; charset=utf-8 Content-Disposition: inline Content-Transfer-Encoding: 8bit In-Reply-To: <1431166241-15775-5-git-send-email-javier.martinez@collabora.co.uk> User-Agent: Mutt/1.5.21 (2010-09-15) Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org Content-Length: 5264 Lines: 122 On Sat, 09 May 2015, Javier Martinez Canillas wrote: > Commit 1b84f2a4cd4a ("mfd: cros_ec: Use fixed size arrays to transfer > data with the EC") modified the struct cros_ec_command fields to not > use pointers for the input and output buffers and use fixed length > arrays instead. > > This change was made because the cros_ec ioctl API uses that struct > cros_ec_command to allow user-space to send commands to the EC and > to get data from the EC. So using pointers made the API not 64-bit > safe. Unfortunately this approach was not flexible enough for all > the use-cases since there may be a need to send larger commands > on newer versions of the EC command protocol. > > So to avoid to choose a constant length that it may be too big for > most commands and thus wasting memory and CPU cycles on copy from > and to user-space or having a size that is too small for some big > commands, use a zero-length array that is both 64-bit safe and > flexible. The same buffer is used for both output and input data > so the maximum of these values should be used to allocate it. > > Suggested-by: Gwendal Grignou > Signed-off-by: Javier Martinez Canillas > Tested-by: Heiko Stuebner > --- > > Changes since v1: > - Add Heiko Stuebner Tested-by tag > - Removed a new blank line at EOF warning. Reported by Heiko Stuebner > - Use kmalloc instead of kzalloc when the message is later initialized > Suggested by Gwendal Grignou > - Pre-allocate struct cros_ec_command instead of dynamically allocate it > whenever is possible. Suggested by Gwendal Grignou > - Pre-allocate buffers for the usual cases and only allocate dynamically > in the heap for bigger sizes. Suggested by Gwendal Grignou > - Don't access the cros_ec_command received from user-space before doing > a copy_from_user. Suggested by Gwendal Grignou > - Only copy from user-space outsize bytes and copy_to_user insize bytes > Suggested by Gwendal Grignou > - ec_device_ioctl_xcmd() must return the numbers of bytes read and not 0 > on success. Suggested by Gwendal Grignou > - Rename alloc_cmd_msg to alloc_lightbar_cmd_msg. Suggested by Gwendal Grignou > --- > drivers/i2c/busses/i2c-cros-ec-tunnel.c | 59 ++++++++--- > drivers/input/keyboard/cros_ec_keyb.c | 19 ++-- > drivers/mfd/cros_ec.c | 18 ++-- > drivers/mfd/cros_ec_i2c.c | 4 +- > drivers/mfd/cros_ec_spi.c | 2 +- > drivers/platform/chrome/cros_ec_dev.c | 66 +++++++++---- > drivers/platform/chrome/cros_ec_lightbar.c | 152 +++++++++++++++++++---------- > drivers/platform/chrome/cros_ec_lpc.c | 8 +- > drivers/platform/chrome/cros_ec_sysfs.c | 92 +++++++++-------- > include/linux/mfd/cros_ec.h | 6 +- > 10 files changed, 273 insertions(+), 153 deletions(-) [...] > diff --git a/drivers/mfd/cros_ec.c b/drivers/mfd/cros_ec.c > index 1574a9352a6d..ee8aa8142932 100644 > --- a/drivers/mfd/cros_ec.c > +++ b/drivers/mfd/cros_ec.c > @@ -41,7 +41,7 @@ int cros_ec_prepare_tx(struct cros_ec_device *ec_dev, > out[2] = msg->outsize; > csum = out[0] + out[1] + out[2]; > for (i = 0; i < msg->outsize; i++) > - csum += out[EC_MSG_TX_HEADER_BYTES + i] = msg->outdata[i]; > + csum += out[EC_MSG_TX_HEADER_BYTES + i] = msg->data[i]; > out[EC_MSG_TX_HEADER_BYTES + msg->outsize] = (uint8_t)(csum & 0xff); > > return EC_MSG_TX_PROTO_BYTES + msg->outsize; > @@ -75,11 +75,13 @@ int cros_ec_cmd_xfer(struct cros_ec_device *ec_dev, > ret = ec_dev->cmd_xfer(ec_dev, msg); > if (msg->result == EC_RES_IN_PROGRESS) { > int i; > - struct cros_ec_command status_msg = { }; > + struct cros_ec_command *status_msg; > struct ec_response_get_comms_status *status; > + u8 buf[sizeof(*status_msg) + sizeof(*status)] = { }; This sort of thing is usually frowned upon. Can you allocate and free buf's memory using the normal kernel helpers please? [...] > diff --git a/include/linux/mfd/cros_ec.h b/include/linux/mfd/cros_ec.h > index 14cf522123dd..7eee38abd02a 100644 > --- a/include/linux/mfd/cros_ec.h > +++ b/include/linux/mfd/cros_ec.h > @@ -42,8 +42,7 @@ enum { > * @outsize: Outgoing length in bytes > * @insize: Max number of bytes to accept from EC > * @result: EC's response to the command (separate from communication failure) > - * @outdata: Outgoing data to EC > - * @indata: Where to put the incoming data from EC > + * @data: Where to put the incoming data from EC and outgoing data to EC > */ > struct cros_ec_command { > uint32_t version; > @@ -51,8 +50,7 @@ struct cros_ec_command { > uint32_t outsize; > uint32_t insize; > uint32_t result; > - uint8_t outdata[EC_PROTO2_MAX_PARAM_SIZE]; > - uint8_t indata[EC_PROTO2_MAX_PARAM_SIZE]; > + uint8_t data[0]; uint8_t *data? > }; > > /** -- Lee Jones Linaro STMicroelectronics Landing Team Lead Linaro.org │ Open source software for ARM SoCs Follow Linaro: Facebook | Twitter | Blog -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/