On Thu, Jan 2, 2014 at 4:25 PM, Luigi Semenzato <semenzato@xxxxxxxxxxxx> wrote: > On Thu, Jan 2, 2014 at 11:48 AM, Dmitry Torokhov > <dmitry.torokhov@xxxxxxxxx> 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@xxxxxxxxx> 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 -- To unsubscribe from this list: send the line "unsubscribe linux-input" in the body of a message to majordomo@xxxxxxxxxxxxxxx More majordomo info at http://vger.kernel.org/majordomo-info.html
