Re: [PATCH V2 1/2] Input: of_keymap: Introduce matrix_keypad_of_build_keymap()

2012-03-29 Thread Viresh Kumar 
On 3/29/2012 2:58 AM, Stephen Warren wrote:
> so the DT parsing can easily overflow this, and is driven by
> user-supplied data.

Got it. This check is must.

> I think you'll need to pass input_dev->keycodesize

you mean keycodemax here?

> into matrix_keypad_of_build_keymap() to achieve the overall
> range-checking.

Will do that.

> but also checking col against (1< needed to make sure the individual "fields" of the array index don't
> overflow too.

Ok. Please cross check V3 to verify, i got you comments correctly.

-- 
viresh
___
devicetree-discuss mailing list
devicetree-discuss@lists.ozlabs.org
https://lists.ozlabs.org/listinfo/devicetree-discuss

Re: [PATCH V2 1/2] Input: of_keymap: Introduce matrix_keypad_of_build_keymap()

2012-03-28 Thread Stephen Warren 
On 03/27/2012 10:55 PM, Viresh Kumar wrote:
> On 3/27/2012 9:15 PM, Stephen Warren wrote:
  static int __devinit tegra_kbc_probe(struct platform_device *pdev)
>> ...
 +  if (pdev->dev.of_node) {
 +  /* FIXME: Add handling of linux,fn-keymap here */
 +  err = matrix_keypad_of_build_keymap(&pdev->dev, KBC_ROW_SHIFT,
 +  input_dev->keycode, input_dev->keybit,
 +  "linux,keymap");
>>
>> Where do input_dev->keycode/keybit get allocated? As far as I can tell,
>> matrix_keypad_of_build_keymap() just writes to those and assumes they're
>> already allocated.
> 
> If i am not reading the code incorrectly, keycode is allocated memory with
> kbc. And then we do this:
> 
>   input_dev->keycode = kbc->keycode;
> 
> keybit is again present as part of struct input_dev.
> Am i missing something.

Ah right, I'd been looking inside input_allocate_device() rather than
the specific driver's probe(). So, no issue here.

 diff --git a/drivers/input/of_keymap.c b/drivers/input/of_keymap.c
>> ...
 +int matrix_keypad_of_build_keymap(struct device *dev, unsigned int 
 row_shift,
>> ...
 +  keymap[MATRIX_SCAN_CODE(row, col, row_shift)] = code;
 +  __set_bit(code, keybit);
>> How bit are keymap and keybit?
> 
> Couldn't get this one. :(
> Can you please elaborate the question a bit?

s/bit/big/ might make the question clearer:-)

>> I think we need range-checking here to
>> make sure that row/col/row_shift/code are valid and in-range.
> 
> I picked this directly from matrix_keypad_build_keymap() as is.
> Anyway there is no loss in improving it. :)
> 
> What kind of range-check you are looking for?
> Currently we do following
> 
>   unsigned int row = KEY_ROW(key);
>   unsigned int col = KEY_COL(key);
>   unsigned short code = KEY_VAL(key);
> 
> All these macros '&' 'key' with 0xFF, 0xFF and 0x.
> Which is also kind of range checking.

The size of the keycode array is much smaller though:

mach-tegra's kbc.h:

#define KBC_MAX_ROW 16
#define KBC_MAX_COL 8
#define KBC_MAX_KEY (KBC_MAX_ROW * KBC_MAX_COL)

tegra-kbc.c:

struct tegra_kbc {
...
unsigned short keycode[KBC_MAX_KEY * 2];

so the DT parsing can easily overflow this, and is driven by
user-supplied data. I think you'll need to pass input_dev->keycodesize
into matrix_keypad_of_build_keymap() to achieve the overall
range-checking, but also checking col against (1

Re: [PATCH V2 1/2] Input: of_keymap: Introduce matrix_keypad_of_build_keymap()

2012-03-27 Thread Viresh Kumar 
On 3/27/2012 9:15 PM, Stephen Warren wrote:
>> >  static int __devinit tegra_kbc_probe(struct platform_device *pdev)
> ...
>> > +  if (pdev->dev.of_node) {
>> > +  /* FIXME: Add handling of linux,fn-keymap here */
>> > +  err = matrix_keypad_of_build_keymap(&pdev->dev, KBC_ROW_SHIFT,
>> > +  input_dev->keycode, input_dev->keybit,
>> > +  "linux,keymap");
> Where do input_dev->keycode/keybit get allocated? As far as I can tell,
> matrix_keypad_of_build_keymap() just writes to those and assumes they're
> already allocated.

If i am not reading the code incorrectly, keycode is allocated memory with
kbc. And then we do this:

input_dev->keycode = kbc->keycode;

keybit is again present as part of struct input_dev.
Am i missing something.

>> > diff --git a/drivers/input/of_keymap.c b/drivers/input/of_keymap.c
> ...
>> > +int matrix_keypad_of_build_keymap(struct device *dev, unsigned int 
>> > row_shift,
> ...
>> > +  keymap[MATRIX_SCAN_CODE(row, col, row_shift)] = code;
>> > +  __set_bit(code, keybit);
> How bit are keymap and keybit?

Couldn't get this one. :(
Can you please elaborate the question a bit?

> I think we need range-checking here to
> make sure that row/col/row_shift/code are valid and in-range.

I picked this directly from matrix_keypad_build_keymap() as is.
Anyway there is no loss in improving it. :)

What kind of range-check you are looking for?
Currently we do following

unsigned int row = KEY_ROW(key);
unsigned int col = KEY_COL(key);
unsigned short code = KEY_VAL(key);

All these macros '&' 'key' with 0xFF, 0xFF and 0x.
Which is also kind of range checking.

-- 
viresh
___
devicetree-discuss mailing list
devicetree-discuss@lists.ozlabs.org
https://lists.ozlabs.org/listinfo/devicetree-discuss

Re: [PATCH V2 1/2] Input: of_keymap: Introduce matrix_keypad_of_build_keymap()

2012-03-27 Thread Stephen Warren 
On 03/26/2012 11:38 PM, Viresh Kumar wrote:
> We don't need to allocate memory for keymap in 
> matrix_keyboard_of_fill_keymap(),
> as this would only be used by matrix_keyboard_of_free_keymap(). Instead create
> another routine matrix_keypad_of_build_keymap() which reads directly the
> property from struct device_node and builds keymap.
> 
> With this eariler routines matrix_keyboard_of_fill_keymap() and
> matrix_keyboard_of_free_keymap() go away.
> 
> This patch also fixes tegra driver according to these changes.

> diff --git a/drivers/input/keyboard/tegra-kbc.c 
> b/drivers/input/keyboard/tegra-kbc.c
...
>  static int __devinit tegra_kbc_probe(struct platform_device *pdev)
...
> + if (pdev->dev.of_node) {
> + /* FIXME: Add handling of linux,fn-keymap here */
> + err = matrix_keypad_of_build_keymap(&pdev->dev, KBC_ROW_SHIFT,
> + input_dev->keycode, input_dev->keybit,
> + "linux,keymap");

Where do input_dev->keycode/keybit get allocated? As far as I can tell,
matrix_keypad_of_build_keymap() just writes to those and assumes they're
already allocated.

> diff --git a/drivers/input/of_keymap.c b/drivers/input/of_keymap.c
...
> +int matrix_keypad_of_build_keymap(struct device *dev, unsigned int row_shift,
...
> + keymap[MATRIX_SCAN_CODE(row, col, row_shift)] = code;
> + __set_bit(code, keybit);

How bit are keymap and keybit? I think we need range-checking here to
make sure that row/col/row_shift/code are valid and in-range.
___
devicetree-discuss mailing list
devicetree-discuss@lists.ozlabs.org
https://lists.ozlabs.org/listinfo/devicetree-discuss

[PATCH V2 1/2] Input: of_keymap: Introduce matrix_keypad_of_build_keymap()

2012-03-26 Thread Viresh Kumar 
We don't need to allocate memory for keymap in matrix_keyboard_of_fill_keymap(),
as this would only be used by matrix_keyboard_of_free_keymap(). Instead create
another routine matrix_keypad_of_build_keymap() which reads directly the
property from struct device_node and builds keymap.

With this eariler routines matrix_keyboard_of_fill_keymap() and
matrix_keyboard_of_free_keymap() go away.

This patch also fixes tegra driver according to these changes.

Signed-off-by: Viresh Kumar 
---
V2:
- Introduced matrix_keypad_of_build_keymap() and removed fill and free keymap
  routines.
- Updated tegra-kbc.

 drivers/input/keyboard/tegra-kbc.c  |   33 --
 drivers/input/of_keymap.c   |   82 +--
 include/linux/input/matrix_keypad.h |   18 +++-
 3 files changed, 66 insertions(+), 67 deletions(-)

diff --git a/drivers/input/keyboard/tegra-kbc.c 
b/drivers/input/keyboard/tegra-kbc.c
index 21c42f8..96ee31e 100644
--- a/drivers/input/keyboard/tegra-kbc.c
+++ b/drivers/input/keyboard/tegra-kbc.c
@@ -659,10 +659,6 @@ tegra_kbc_dt_parse_pdata(struct platform_device *pdev)
pdata->pin_cfg[KBC_MAX_ROW + i].type = PIN_CFG_COL;
}
 
-   pdata->keymap_data = matrix_keyboard_of_fill_keymap(np, "linux,keymap");
-
-   /* FIXME: Add handling of linux,fn-keymap here */
-
return pdata;
 }
 #else
@@ -676,7 +672,6 @@ static inline struct tegra_kbc_platform_data 
*tegra_kbc_dt_parse_pdata(
 static int __devinit tegra_kbc_probe(struct platform_device *pdev)
 {
const struct tegra_kbc_platform_data *pdata = pdev->dev.platform_data;
-   const struct matrix_keymap_data *keymap_data;
struct tegra_kbc *kbc;
struct input_dev *input_dev;
struct resource *res;
@@ -775,9 +770,24 @@ static int __devinit tegra_kbc_probe(struct 
platform_device *pdev)
 
kbc->use_fn_map = pdata->use_fn_map;
kbc->use_ghost_filter = pdata->use_ghost_filter;
-   keymap_data = pdata->keymap_data ?: &tegra_kbc_default_keymap_data;
-   matrix_keypad_build_keymap(keymap_data, KBC_ROW_SHIFT,
-  input_dev->keycode, input_dev->keybit);
+
+   if (pdev->dev.of_node) {
+   /* FIXME: Add handling of linux,fn-keymap here */
+   err = matrix_keypad_of_build_keymap(&pdev->dev, KBC_ROW_SHIFT,
+   input_dev->keycode, input_dev->keybit,
+   "linux,keymap");
+   if (err) {
+   dev_err(&pdev->dev, "OF: failed to build keymap\n");
+   goto err_put_clk;
+   }
+   } else {
+   const struct matrix_keymap_data *keymap_data =
+   pdata->keymap_data ?: &tegra_kbc_default_keymap_data;
+
+   matrix_keypad_build_keymap(keymap_data, KBC_ROW_SHIFT,
+   input_dev->keycode, input_dev->keybit);
+   }
+
kbc->wakeup_key = pdata->wakeup_key;
 
err = request_irq(kbc->irq, tegra_kbc_isr,
@@ -798,9 +808,6 @@ static int __devinit tegra_kbc_probe(struct platform_device 
*pdev)
platform_set_drvdata(pdev, kbc);
device_init_wakeup(&pdev->dev, pdata->wakeup);
 
-   if (!pdev->dev.platform_data)
-   matrix_keyboard_of_free_keymap(pdata->keymap_data);
-
return 0;
 
 err_free_irq:
@@ -815,10 +822,8 @@ err_free_mem:
input_free_device(input_dev);
kfree(kbc);
 err_free_pdata:
-   if (!pdev->dev.platform_data) {
-   matrix_keyboard_of_free_keymap(pdata->keymap_data);
+   if (!pdev->dev.platform_data)
kfree(pdata);
-   }
 
return err;
 }
diff --git a/drivers/input/of_keymap.c b/drivers/input/of_keymap.c
index 061493d..2837566 100644
--- a/drivers/input/of_keymap.c
+++ b/drivers/input/of_keymap.c
@@ -17,6 +17,7 @@
  *
  */
 
+#include 
 #include 
 #include 
 #include 
@@ -26,62 +27,61 @@
 #include 
 #include 
 
-struct matrix_keymap_data *
-matrix_keyboard_of_fill_keymap(struct device_node *np,
-  const char *propname)
+/**
+ * matrix_keypad_of_build_keymap - convert platform DT keymap into matrix 
keymap
+ * @dev: Pointer to struct device, will be used for getting struct device_node
+ * @row_shift: number of bits to shift row value by to advance to the next
+ * line in the keymap
+ * @keymap: expanded version of keymap that is suitable for use by
+ * matrix keyboad driver
+ * @keybit: pointer to bitmap of keys supported by input device
+ * @propname: Device Tree property name to be used for reading keymap. If 
passed
+ * as NULL, "linux,keymap" is used.
+ *
+ * This function creates an array of keycodes, by reading propname property 
from
+ * device tree passed, that is suitable for using in a standard matrix keyboard
+ * driver that uses row and col as indices.
+ */
+int matrix_keypad_of_build_keymap(struct device *dev, unsigned int row_shift,
+   unsigned short