Re: [PATCH] Input: cros_ec_keyb - avoid variable-length arrays onstack
From: Dmitry Torokhov
Date: Thu Jan 02 2014 - 19:10:54 EST
Hi Doug,
On Thu, Jan 02, 2014 at 09:40:44AM -0800, Doug Anderson wrote:
> Dmitry,
>
> Thanks for cleaning up cros_eckeyb. :) I'm a little curious about
> the motivation here. I can't imagine a keyboard with all that many
> columns (ours has 13), so it's really not taking a whole lot off of
> the stack. Are you trying to make some sort of automated checker
> happy, or just generally trying to keep the kernel consistent?
I compile most of the code with sparse so I prefer to keep it happy.
>
> In any case, I'm not opposed to moving these bytes off the stack.
> Comments below, though...
>
> ---
>
> On Tue, Dec 31, 2013 at 11:35 AM, Dmitry Torokhov
> <dmitry.torokhov@xxxxxxxxx> wrote:
> > Signed-off-by: Dmitry Torokhov <dmitry.torokhov@xxxxxxxxx>
> > ---
> > drivers/input/keyboard/cros_ec_keyb.c | 80 ++++++++++++++++++++---------------
> > 1 file changed, 45 insertions(+), 35 deletions(-)
> >
> > diff --git a/drivers/input/keyboard/cros_ec_keyb.c b/drivers/input/keyboard/cros_ec_keyb.c
> > index d44c5d4..03cb960 100644
> > --- a/drivers/input/keyboard/cros_ec_keyb.c
> > +++ b/drivers/input/keyboard/cros_ec_keyb.c
> > @@ -39,6 +39,7 @@
> > * @keymap_data: Matrix keymap data used to convert to keyscan values
> > * @ghost_filter: true to enable the matrix key-ghosting filter
> > * @old_kb_state: bitmap of keys pressed last scan
> > + * @kb_state: bitmap of keys currently pressed
> > * @dev: Device pointer
> > * @idev: Input device
> > * @ec: Top level ChromeOS device to use to talk to EC
> > @@ -50,17 +51,17 @@ struct cros_ec_keyb {
> > int row_shift;
> > const struct matrix_keymap_data *keymap_data;
> > bool ghost_filter;
> > - u8 *old_kb_state;
> > -
> > struct device *dev;
> > struct input_dev *idev;
> > struct cros_ec_device *ec;
> > struct notifier_block notifier;
> > +
> > + u8 *old_kb_state;
> > + u8 kb_state[];
>
> nit: you've moved old_kb_state to the end of the structure but you
> haven't moved the description in the comments above. I'd expect the
> ordering in the comment and the ordering in the comment to match.
>
> > };
> >
> >
> > -static bool cros_ec_keyb_row_has_ghosting(struct cros_ec_keyb *ckdev,
> > - u8 *buf, int row)
> > +static bool cros_ec_keyb_row_has_ghosting(struct cros_ec_keyb *ckdev, int row)
> > {
> > int pressed_in_row = 0;
> > int row_has_teeth = 0;
> > @@ -68,9 +69,9 @@ static bool cros_ec_keyb_row_has_ghosting(struct cros_ec_keyb *ckdev,
> >
> > mask = 1 << row;
> > for (col = 0; col < ckdev->cols; col++) {
> > - if (buf[col] & mask) {
> > + if (ckdev->kb_state[col] & mask) {
> > pressed_in_row++;
> > - row_has_teeth |= buf[col] & ~mask;
> > + row_has_teeth |= ckdev->kb_state[col] & ~mask;
> > if (pressed_in_row > 1 && row_has_teeth) {
> > /* ghosting */
> > dev_dbg(ckdev->dev,
> > @@ -89,7 +90,7 @@ static bool cros_ec_keyb_row_has_ghosting(struct cros_ec_keyb *ckdev,
> > * Returns true when there is at least one combination of pressed keys that
> > * results in ghosting.
> > */
> > -static bool cros_ec_keyb_has_ghosting(struct cros_ec_keyb *ckdev, u8 *buf)
> > +static bool cros_ec_keyb_has_ghosting(struct cros_ec_keyb *ckdev)
> > {
> > int row;
> >
> > @@ -120,7 +121,7 @@ static bool cros_ec_keyb_has_ghosting(struct cros_ec_keyb *ckdev, u8 *buf)
> > * cheat because the number of rows is small.
> > */
> > for (row = 0; row < ckdev->rows; row++)
> > - if (cros_ec_keyb_row_has_ghosting(ckdev, buf, row))
> > + if (cros_ec_keyb_row_has_ghosting(ckdev, row))
> > return true;
> >
> > return false;
> > @@ -131,8 +132,7 @@ static bool cros_ec_keyb_has_ghosting(struct cros_ec_keyb *ckdev, u8 *buf)
> > * press/release events accordingly. The keyboard state is 13 bytes (one byte
> > * per column)
> > */
> > -static void cros_ec_keyb_process(struct cros_ec_keyb *ckdev,
> > - u8 *kb_state, int len)
> > +static void cros_ec_keyb_process(struct cros_ec_keyb *ckdev, int len)
> > {
> > struct input_dev *idev = ckdev->idev;
> > int col, row;
> > @@ -142,7 +142,7 @@ static void cros_ec_keyb_process(struct cros_ec_keyb *ckdev,
> >
> > num_cols = len;
> >
> > - if (ckdev->ghost_filter && cros_ec_keyb_has_ghosting(ckdev, kb_state)) {
> > + if (ckdev->ghost_filter && cros_ec_keyb_has_ghosting(ckdev)) {
> > /*
> > * Simple-minded solution: ignore this state. The obvious
> > * improvement is to only ignore changes to keys involved in
> > @@ -157,7 +157,7 @@ static void cros_ec_keyb_process(struct cros_ec_keyb *ckdev,
> > int pos = MATRIX_SCAN_CODE(row, col, ckdev->row_shift);
> > const unsigned short *keycodes = idev->keycode;
> >
> > - new_state = kb_state[col] & (1 << row);
> > + new_state = ckdev->kb_state[col] & (1 << row);
> > old_state = ckdev->old_kb_state[col] & (1 << row);
> > if (new_state != old_state) {
> > dev_dbg(ckdev->dev,
> > @@ -168,9 +168,12 @@ static void cros_ec_keyb_process(struct cros_ec_keyb *ckdev,
> > new_state);
> > }
> > }
> > - ckdev->old_kb_state[col] = kb_state[col];
> > }
> > +
> > input_sync(ckdev->idev);
> > +
> > + memcpy(ckdev->old_kb_state, ckdev->kb_state,
> > + ckdev->cols * sizeof(*ckdev->kb_state));
>
> Any motivation for why you've moved this to a memcpy? In my mind the
> old code is easier to understand and I'm not convinced about the speed
> / code space gains (introducing a function call to save 13 assignment
> operations). Again this is not something I'll NAK, but it seems like
> a bit of code churn.
Logically you are saving entire state of keyboard so to me it makes
sense to do it at once.
>
>
> > }
> >
> > static int cros_ec_keyb_open(struct input_dev *dev)
> > @@ -189,10 +192,10 @@ static void cros_ec_keyb_close(struct input_dev *dev)
> > &ckdev->notifier);
> > }
> >
> > -static int cros_ec_keyb_get_state(struct cros_ec_keyb *ckdev, u8 *kb_state)
> > +static int cros_ec_keyb_get_state(struct cros_ec_keyb *ckdev)
> > {
> > return ckdev->ec->command_recv(ckdev->ec, EC_CMD_MKBP_STATE,
> > - kb_state, ckdev->cols);
> > + ckdev->kb_state, ckdev->cols);
> > }
> >
> > static int cros_ec_keyb_work(struct notifier_block *nb,
> > @@ -201,11 +204,10 @@ static int cros_ec_keyb_work(struct notifier_block *nb,
> > int ret;
> > struct cros_ec_keyb *ckdev = container_of(nb, struct cros_ec_keyb,
> > notifier);
> > - u8 kb_state[ckdev->cols];
> >
> > - ret = cros_ec_keyb_get_state(ckdev, kb_state);
> > + ret = cros_ec_keyb_get_state(ckdev);
> > if (ret >= 0)
> > - cros_ec_keyb_process(ckdev, kb_state, ret);
> > + cros_ec_keyb_process(ckdev, ret);
> >
> > return NOTIFY_DONE;
> > }
> > @@ -217,32 +219,40 @@ static int cros_ec_keyb_probe(struct platform_device *pdev)
> > struct cros_ec_keyb *ckdev;
> > struct input_dev *idev;
> > struct device_node *np;
> > + unsigned int rows, cols;
> > + size_t size;
> > int err;
> >
> > np = pdev->dev.of_node;
> > if (!np)
> > return -ENODEV;
> >
> > - ckdev = devm_kzalloc(&pdev->dev, sizeof(*ckdev), GFP_KERNEL);
> > - if (!ckdev)
> > - return -ENOMEM;
> > - err = matrix_keypad_parse_of_params(&pdev->dev, &ckdev->rows,
> > - &ckdev->cols);
> > + err = matrix_keypad_parse_of_params(&pdev->dev, &rows, &cols);
> > if (err)
> > return err;
> > - ckdev->old_kb_state = devm_kzalloc(&pdev->dev, ckdev->cols, GFP_KERNEL);
> > - if (!ckdev->old_kb_state)
> > - return -ENOMEM;
> >
> > - idev = devm_input_allocate_device(&pdev->dev);
> > - if (!idev)
> > + /*
> > + * Double memory for keyboard state so we have space for storing
> > + * current and previous state.
> > + */
> > + size = sizeof(*ckdev) + 2 * cols * sizeof(*ckdev->kb_state);
> > + ckdev = devm_kzalloc(&pdev->dev, size, GFP_KERNEL);
> > + if (!ckdev)
> > return -ENOMEM;
>
> This change seems like a lot of complexity to save one memory
> allocation. If you insist, I'd be OK with having one allocation for
> both buffers (kb_state and old_kb_state) but trying to jam this onto
> the end of the structure is non-obvious. It certainly took me a
> minute to understand what you were doing and why.
It is not one additional allocation but more as you need to allocate
devres data structures and add them there. I think we have quite a few
drivers piggy-backing key tables at the end of data structures.
>
>
> > + ckdev->rows = rows;
> > + ckdev->cols = cols;
> > + ckdev->old_kb_state = &ckdev->kb_state[cols];
> > +
> > ckdev->ec = ec;
> > - ckdev->notifier.notifier_call = cros_ec_keyb_work;
> > ckdev->dev = dev;
> > + ckdev->notifier.notifier_call = cros_ec_keyb_work;
> > dev_set_drvdata(&pdev->dev, ckdev);
> >
> > + idev = devm_input_allocate_device(&pdev->dev);
> > + if (!idev)
> > + return -ENOMEM;
> > +
> > idev->name = ec->ec_name;
> > idev->phys = ec->phys_name;
> > __set_bit(EV_REP, idev->evbit);
> > @@ -282,8 +292,6 @@ static int cros_ec_keyb_probe(struct platform_device *pdev)
> > /* Clear any keys in the buffer */
> > static void cros_ec_keyb_clear_keyboard(struct cros_ec_keyb *ckdev)
> > {
> > - u8 old_state[ckdev->cols];
> > - u8 new_state[ckdev->cols];
> > unsigned long duration;
> > int i, ret;
> >
> > @@ -294,11 +302,13 @@ static void cros_ec_keyb_clear_keyboard(struct cros_ec_keyb *ckdev)
> > * Assume that the EC keyscan buffer is at most 32 deep.
> > */
> > duration = jiffies;
> > - ret = cros_ec_keyb_get_state(ckdev, new_state);
> > + ret = cros_ec_keyb_get_state(ckdev);
> > for (i = 1; !ret && i < 32; i++) {
> > - memcpy(old_state, new_state, sizeof(old_state));
> > - ret = cros_ec_keyb_get_state(ckdev, new_state);
> > - if (0 == memcmp(old_state, new_state, sizeof(old_state)))
> > + memcpy(ckdev->old_kb_state, ckdev->kb_state,
> > + ckdev->cols * sizeof(*ckdev->kb_state));
> > + ret = cros_ec_keyb_get_state(ckdev);
> > + if (!memcmp(ckdev->old_kb_state, ckdev->kb_state,
> > + ckdev->cols * sizeof(*ckdev->kb_state)))
>
> I'm not necessarily convinced that reusing ckdev->old_kb_state is wise
> in cros_ec_keyb_clear_keyboard(). It is a behavior change (though it
> might be a benign one).
>
> Before your patch old_kb_state represented the last state that was
> reported to the keyboard subsystem. After your patch, old_kb_state
> may contain something different than what was reported to the
> subsystem, at least after a suspend/resume cycle.
Hmm, I think you are right... By the way, why do we have to clear the
buffer?
Thanks.
--
Dmitry
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/