lkml.org 
[lkml]   [2014]   [Jan]   [6]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
/
Date
From
SubjectRe: [PATCH] Input: cros_ec_keyb - avoid variable-length arrays on stack
On Mon, Jan 06, 2014 at 10:57:14AM -0800, Doug Anderson wrote:
> On Thu, Jan 2, 2014 at 4:25 PM, Luigi Semenzato <semenzato@chromium.org> wrote:
> > On Thu, Jan 2, 2014 at 11:48 AM, Dmitry Torokhov
> > <dmitry.torokhov@gmail.com> 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
> >>> <dmitry.torokhov@gmail.com> 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


\
 
 \ /
  Last update: 2014-01-06 23:41    [W:0.067 / U:0.252 seconds]
©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site