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?
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
<[email protected]> wrote:
> Signed-off-by: Dmitry Torokhov <[email protected]>
> ---
> 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.
> }
>
> 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.
> + 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.
To put it in concrete terms, I _think_ this is what happens (please
correct me if I'm wrong):
Example A: imagine no keys were pressed when we suspended. Then, we
press and hold the "Shift" key to wake up. After waking up, we also
press the "A" key.
The sequence of events before your patch would be:
1. System wakes up and ignores the shift key being pressed.
2. System sees the "A" pressed and notices shift is still pressed.
3. We'll add a keydown for Shift and A at the same time.
4. Everyone thinks both keys down.
After your patch:
1. System wakes up and ignores the shift key being pressed.
2. System sees the "A" pressed; thinks it already told about shift.
3. We'll add a keydown for A.
4. cros_ec thinks both down; OS thinks A down.
Example B: imagine no keys were pressed when we suspended. Then, we
press and hold the "Shift" key to wake up. After waking, we release
Shift.
Before:
1. System wakes up and ignores the shift key being pressed.
2. When shift is released we scan and don't enqueue anything.
After:
1. System wakes up and ignores the shift key being pressed.
2. When shift is released we scan and process a release of the Shift
key (though we never sent a press).
-Doug
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
> <[email protected]> wrote:
> > Signed-off-by: Dmitry Torokhov <[email protected]>
> > ---
> > 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
On Thu, Jan 2, 2014 at 11:48 AM, Dmitry Torokhov
<[email protected]> wrote:
> 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
>> <[email protected]> wrote:
>> > Signed-off-by: Dmitry Torokhov <[email protected]>
>> > ---
>> > 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?
This is almost funny. The keyboard is connected to the EC, which
stays awake during sleep. The case of some laptops has enough flex
that keys can be pressed when the laptop is squished in a backpack.
On resume the kernel gets interrupts and loads the key events from the
EC, but we really don't want to see those keys, so we ignore them and
clear the state.
I suppose we could have made the EC smarter about suspend/resume, but
it's got a tiny memory and we don't want to field-update it, so we
keep that code as simple as possible.
Thanks!
>
> Thanks.
>
> --
> Dmitry
On Thu, Jan 2, 2014 at 4:25 PM, Luigi Semenzato <[email protected]> wrote:
> On Thu, Jan 2, 2014 at 11:48 AM, Dmitry Torokhov
> <[email protected]> wrote:
>> 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
>>> <[email protected]> wrote:
...
>>> > @@ -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.
OK, I will leave this as your call. To me, piggybacking like this
make sense if you've got a single chunk of dynamic memory that you
just want to cram onto the end of the structure. It just gets more
complicated when you have two nearly identical chunks of memory and
one of them is using this piggybacking technique while the other
isn't.
What about a compromise and declaring as:
u8 *kb_state;
u8 *old_kb_state;
u8 buffers[];
You still have the same number of memory allocations but (to me) it's
much clearer what's going on here. You do pay a penalty of an extra
memory dereference and an extra 4 bytes of memory, but clarity should
trump that.
-Doug
On Mon, Jan 06, 2014 at 10:57:14AM -0800, Doug Anderson wrote:
> On Thu, Jan 2, 2014 at 4:25 PM, Luigi Semenzato <[email protected]> wrote:
> > On Thu, Jan 2, 2014 at 11:48 AM, Dmitry Torokhov
> > <[email protected]> wrote:
> >> 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
> >>> <[email protected]> wrote:
> ...
> >>> > @@ -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.
>
> OK, I will leave this as your call. To me, piggybacking like this
> make sense if you've got a single chunk of dynamic memory that you
> just want to cram onto the end of the structure. It just gets more
> complicated when you have two nearly identical chunks of memory and
> one of them is using this piggybacking technique while the other
> isn't.
>
> What about a compromise and declaring as:
>
> u8 *kb_state;
> u8 *old_kb_state;
> u8 buffers[];
>
> You still have the same number of memory allocations but (to me) it's
> much clearer what's going on here. You do pay a penalty of an extra
> memory dereference and an extra 4 bytes of memory, but clarity should
> trump that.
OK, I can do that.
Thanks.
--
Dmitry