On 02/05/2016 06:24 PM, Gwendal Grignou wrote:
> We should not used kmalloc when get events, these functions are called
> quite often.
> Gwendal.
>
> On Fri, Feb 5, 2016 at 5:32 AM, Tomeu Vizoso <tomeu.viz...@collabora.com
> <mailto:tomeu.viz...@collabora.com>> wrote:
>
> +static int cros_ec_get_host_command_version_mask(struct
> cros_ec_device *ec_dev,
> + u16 cmd, u32 *mask)
> +{
> + struct ec_params_get_cmd_versions *pver;
> + struct ec_response_get_cmd_versions *rver;
> + struct cros_ec_command *msg;
> + int ret;
> +
> + msg = kmalloc(sizeof(*msg) + max(sizeof(rver), sizeof(pver)),
> + GFP_KERNEL);
> + if (!msg)
> + return -ENOMEM;
>
> Victor's version in https://chromium-review.googlesource.com/272954
> looks cleaner: no malloc, no need to cast rver.
I agree that it looks cleaner, but how would you allocate the payload at
build time if it has to be max(sizeof(*pver), sizeof(*rver))?
> +static int get_next_event(struct cros_ec_device *ec_dev)
> +{
> + struct cros_ec_command *msg;
> + int ret;
> +
> + msg = kmalloc(sizeof(*msg) + sizeof(ec_dev->event_data),
> GFP_KERNEL);
> + if (!msg)
> + return -ENOMEM;
>
> Same grip about malloc(). This function will be called very often when
> sensors are used.
Ok.
> +static int get_keyboard_state_event(struct cros_ec_device *ec_dev)
> +{
> + struct cros_ec_command *msg;
> +
> + msg = kmalloc(sizeof(*msg) + sizeof(ec_dev->event_data.data),
> + GFP_KERNEL);
> + if (!msg)
> + return -ENOMEM;
>
> Given this command will be used on every key press, it is better to
> pre-allocate data (like
> in https://chromium-review.googlesource.com/276768) and use msg on the
> stack like Victor's changes.
Ok.
Thanks,
Tomeu
