| lists.openwall.net | lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC | |
|
Open Source and information security mailing list archives
| ||
|
Message-ID: <CAPnjgZ2-QV36Ro_G32kcHStpj0SCC_gDoeCPqjN-h6bsBTSGvQ@mail.gmail.com>
Date: Wed, 13 Feb 2013 22:45:07 -0800
From: Simon Glass <sjg@...omium.org>
To: Dmitry Torokhov <dmitry.torokhov@...il.com>
Cc: LKML <linux-kernel@...r.kernel.org>,
Samuel Ortiz <sameo@...ux.intel.com>,
Luigi Semenzato <semenzato@...omium.org>,
Vincent Palatin <vpalatin@...omium.org>,
Grant Likely <grant.likely@...retlab.ca>,
Rob Herring <rob.herring@...xeda.com>,
Rob Landley <rob@...dley.net>, Felipe Balbi <balbi@...com>,
Sourav Poddar <sourav.poddar@...com>,
Tony Lindgren <tony@...mide.com>,
Greg Kroah-Hartman <gregkh@...uxfoundation.org>,
"Mike A. Chan" <mikechan@...gle.com>,
Jun Nakajima <jun.nakajima@...el.com>,
Tom Keel <thomas.keel@...el.com>,
devicetree-discuss@...ts.ozlabs.org, linux-doc@...r.kernel.org,
linux-input@...r.kernel.org
Subject: Re: [PATCH v2 6/6] Input: Add ChromeOS EC keyboard driver
Hi Dmitry,
On Wed, Feb 13, 2013 at 12:02 PM, Dmitry Torokhov
<dmitry.torokhov@...il.com> wrote:
> Hi SImon,
>
> On Tue, Feb 12, 2013 at 06:42:26PM -0800, Simon Glass wrote:
>> Use the key-matrix layer to interpret key scan information from the EC
>> and inject input based on the FDT-supplied key map. This driver registers
>> itself with the ChromeOS EC driver to perform communications.
>>
>> Additional FDT bindings are provided to specify rows/columns and the
>> auto-repeat information.
>>
>> Signed-off-by: Simon Glass <sjg@...omium.org>
>> Signed-off-by: Luigi Semenzato <semenzato@...omium.org>
>> Signed-off-by: Vincent Palatin <vpalatin@...omium.org>
>> ---
>> Changes in v2:
>> - Remove use of __devinit/__devexit
>> - Use function to read matrix-keypad parameters from DT
>> - Remove key autorepeat parameters from DT binding and driver
>> - Use unsigned int for rows/cols
Thanks for all the review comments.
>>
>> .../devicetree/bindings/input/cros-ec-keyb.txt | 72 ++++
>> drivers/input/keyboard/Kconfig | 12 +
>> drivers/input/keyboard/Makefile | 1 +
>> drivers/input/keyboard/cros_ec_keyb.c | 394 +++++++++++++++++++++
>> 4 files changed, 479 insertions(+)
>> create mode 100644 Documentation/devicetree/bindings/input/cros-ec-keyb.txt
>> create mode 100644 drivers/input/keyboard/cros_ec_keyb.c
>>
>> diff --git a/Documentation/devicetree/bindings/input/cros-ec-keyb.txt b/Documentation/devicetree/bindings/input/cros-ec-keyb.txt
>> new file mode 100644
>> index 0000000..0f6355c
>> --- /dev/null
>> +++ b/Documentation/devicetree/bindings/input/cros-ec-keyb.txt
>> @@ -0,0 +1,72 @@
>> +ChromeOS EC Keyboard
>> +
>> +Google's ChromeOS EC Keyboard is a simple matrix keyboard implemented on
>> +a separate EC (Embedded Controller) device. It provides a message for reading
>> +key scans from the EC. These are then converted into keycodes for processing
>> +by the kernel.
>> +
>> +This binding is based on matrix-keymap.txt and extends/modifies it as follows:
>> +
>> +Required properties:
>> +- compatible: "google,cros-ec-keyb"
>> +
>> +Optional properties:
>> +- google,needs-ghost-filter: True to enable a ghost filter for the matrix
>> +keyboard. This is recommended if the EC does not have its own logic or
>> +hardware for this.
>> +
>> +
>> +Example:
>> +
>> +cros-ec-keyb {
>> + compatible = "google,cros-ec-keyb";
>> + keypad,num-rows = <8>;
>> + keypad,num-columns = <13>;
>> + google,needs-ghost-filter;
>> + /*
>> + * Keymap entries take the form of 0xRRCCKKKK where
>> + * RR=Row CC=Column KKKK=Key Code
>> + * The values below are for a US keyboard layout and
>> + * are taken from the Linux driver. Note that the
>> + * 102ND key is not used for US keyboards.
>> + */
>> + linux,keymap = <
>> + /* CAPSLCK F1 B F10 */
>> + 0x0001003a 0x0002003b 0x00030030 0x00040044
>> + /* N = R_ALT ESC */
>> + 0x00060031 0x0008000d 0x000a0064 0x01010001
>> + /* F4 G F7 H */
>> + 0x0102003e 0x01030022 0x01040041 0x01060023
>> + /* ' F9 BKSPACE L_CTRL */
>> + 0x01080028 0x01090043 0x010b000e 0x0200001d
>> + /* TAB F3 T F6 */
>> + 0x0201000f 0x0202003d 0x02030014 0x02040040
>> + /* ] Y 102ND [ */
>> + 0x0205001b 0x02060015 0x02070056 0x0208001a
>> + /* F8 GRAVE F2 5 */
>> + 0x02090042 0x03010029 0x0302003c 0x03030006
>> + /* F5 6 - \ */
>> + 0x0304003f 0x03060007 0x0308000c 0x030b002b
>> + /* R_CTRL A D F */
>> + 0x04000061 0x0401001e 0x04020020 0x04030021
>> + /* S K J ; */
>> + 0x0404001f 0x04050025 0x04060024 0x04080027
>> + /* L ENTER Z C */
>> + 0x04090026 0x040b001c 0x0501002c 0x0502002e
>> + /* V X , M */
>> + 0x0503002f 0x0504002d 0x05050033 0x05060032
>> + /* L_SHIFT / . SPACE */
>> + 0x0507002a 0x05080035 0x05090034 0x050B0039
>> + /* 1 3 4 2 */
>> + 0x06010002 0x06020004 0x06030005 0x06040003
>> + /* 8 7 0 9 */
>> + 0x06050009 0x06060008 0x0608000b 0x0609000a
>> + /* L_ALT DOWN RIGHT Q */
>> + 0x060a0038 0x060b006c 0x060c006a 0x07010010
>> + /* E R W I */
>> + 0x07020012 0x07030013 0x07040011 0x07050017
>> + /* U R_SHIFT P O */
>> + 0x07060016 0x07070036 0x07080019 0x07090018
>> + /* UP LEFT */
>> + 0x070b0067 0x070c0069>;
>> +};
>> diff --git a/drivers/input/keyboard/Kconfig b/drivers/input/keyboard/Kconfig
>> index 078305e..3a70be7 100644
>> --- a/drivers/input/keyboard/Kconfig
>> +++ b/drivers/input/keyboard/Kconfig
>> @@ -628,4 +628,16 @@ config KEYBOARD_W90P910
>> To compile this driver as a module, choose M here: the
>> module will be called w90p910_keypad.
>>
>> +config KEYBOARD_CROS_EC
>> + tristate "ChromeOS EC keyboard"
>> + select INPUT_MATRIXKMAP
>> + select MFD_CROS_EC
>
> Is this select safe? I.e. does MFD_CROS_EC depend on anything else?
I'll remove it, since it isn't required, and it's true that it does
need other things.
>
>> + help
>> + Say Y here to enable the matrix keyboard used by ChromeOS devices
>> + and implemented on the ChromeOS EC. You must enable one bus option
>> + (MFD_CROS_EC_I2C or MFD_CROS_EC_SPI) to use this.
>> +
>> + To compile this driver as a module, choose M here: the
>> + module will be called cros_ec_keyb.
>> +
>> endif
>> diff --git a/drivers/input/keyboard/Makefile b/drivers/input/keyboard/Makefile
>> index 49b1645..0c43e8c 100644
>> --- a/drivers/input/keyboard/Makefile
>> +++ b/drivers/input/keyboard/Makefile
>> @@ -11,6 +11,7 @@ obj-$(CONFIG_KEYBOARD_AMIGA) += amikbd.o
>> obj-$(CONFIG_KEYBOARD_ATARI) += atakbd.o
>> obj-$(CONFIG_KEYBOARD_ATKBD) += atkbd.o
>> obj-$(CONFIG_KEYBOARD_BFIN) += bf54x-keys.o
>> +obj-$(CONFIG_KEYBOARD_CROS_EC) += cros_ec_keyb.o
>> obj-$(CONFIG_KEYBOARD_DAVINCI) += davinci_keyscan.o
>> obj-$(CONFIG_KEYBOARD_EP93XX) += ep93xx_keypad.o
>> obj-$(CONFIG_KEYBOARD_GOLDFISH_EVENTS) += goldfish_events.o
>> diff --git a/drivers/input/keyboard/cros_ec_keyb.c b/drivers/input/keyboard/cros_ec_keyb.c
>> new file mode 100644
>> index 0000000..43e5be2
>> --- /dev/null
>> +++ b/drivers/input/keyboard/cros_ec_keyb.c
>> @@ -0,0 +1,394 @@
>> +/*
>> + * ChromeOS EC keyboard driver
>> + *
>> + * Copyright (C) 2012 Google, Inc
>> + *
>> + * This software is licensed under the terms of the GNU General Public
>> + * License version 2, as published by the Free Software Foundation, and
>> + * may be copied, distributed, and modified under those terms.
>> + *
>> + * This program is distributed in the hope that it will be useful,
>> + * but WITHOUT ANY WARRANTY; without even the implied warranty of
>> + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the
>> + * GNU General Public License for more details.
>> + *
>> + * This driver uses the Chrome OS EC byte-level message-based protocol for
>> + * communicating the keyboard state (which keys are pressed) from a keyboard EC
>> + * to the AP over some bus (such as i2c, lpc, spi). The EC does debouncing,
>> + * but everything else (including deghosting) is done here. The main
>> + * motivation for this is to keep the EC firmware as simple as possible, since
>> + * it cannot be easily upgraded and EC flash/IRAM space is relatively
>> + * expensive.
>> + */
>> +
>> +#include <linux/module.h>
>> +#include <linux/i2c.h>
>> +#include <linux/input.h>
>> +#include <linux/kernel.h>
>> +#include <linux/notifier.h>
>> +#include <linux/platform_device.h>
>> +#include <linux/slab.h>
>> +#include <linux/input/matrix_keypad.h>
>> +#include <linux/mfd/cros_ec.h>
>> +#include <linux/mfd/cros_ec_commands.h>
>> +
>> +/*
>> + * @rows: Number of rows in the keypad
>> + * @cols: Number of columns in the keypad
>> + * @row_shift: log2 or number of rows, rounded up
>> + * @keymap_data: Matrix keymap data used to convert to keyscan values
>> + * @ghost_filter: true to enable the matrix key-ghosting filter
>> + * @old_state: Previous state of the keyboard matrix (used to calc deltas)
>> + * @dev: Device pointer
>> + * @idev: Input device
>> + * @ec: Top level ChromeOS device to use to talk to EC
>> + * @event_notifier: interrupt event notifier for transport devices
>> + * @wake_notifier: wake notfier for client devices (e.g. keyboard). This
>> + * indicates to sub-drivers that we have woken up from resume but we
>> + * were not a wakeup source.
>> + */
>> +struct cros_ec_keyb {
>> + unsigned int rows;
>> + unsigned int cols;
>> + int row_shift;
>> + const struct matrix_keymap_data *keymap_data;
>> + bool ghost_filter;
>> + /*
>> + * old_state[matrix code] is 1 when the most recent (valid)
>> + * communication with the keyboard indicated that the key at row/col
>> + * was in the pressed state.
>> + */
>> + uint8_t *old_state;
>> +
>> + struct device *dev;
>> + struct input_dev *idev;
>> + struct cros_ec_device *ec;
>> + struct notifier_block notifier;
>> + struct notifier_block wake_notifier;
>> +};
>> +
>> +
>> +/*
>> + * Sends a single key event to the input layer.
>> + */
>> +static inline void cros_ec_keyb_send_key_event(struct cros_ec_keyb *ckdev,
>> + int row, int col, int pressed)
>> +{
>> + struct input_dev *idev = ckdev->idev;
>> + int code = MATRIX_SCAN_CODE(row, col, ckdev->row_shift);
>> + const unsigned short *keycodes = idev->keycode;
>> +
>> + input_report_key(idev, keycodes[code], pressed);
>> +}
>> +
>> +/*
>> + * 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, uint8_t *buf)
>> +{
>> + int col, row;
>> + int mask;
>> + int pressed_in_row[ckdev->rows];
>> + int row_has_teeth[ckdev->rows];
>> +
>> + memset(pressed_in_row, '\0', sizeof(pressed_in_row));
>> + memset(row_has_teeth, '\0', sizeof(row_has_teeth));
>> + /*
>> + * Ghosting happens if for any pressed key X there are other keys
>> + * pressed both in the same row and column of X as, for instance,
>> + * in the following diagram:
>> + *
>> + * . . Y . g .
>> + * . . . . . .
>> + * . . . . . .
>> + * . . X . Z .
>> + *
>> + * In this case only X, Y, and Z are pressed, but g appears to be
>> + * pressed too (see Wikipedia).
>> + *
>> + * We can detect ghosting in a single pass (*) over the keyboard state
>> + * by maintaining two arrays. pressed_in_row counts how many pressed
>> + * keys we have found in a row. row_has_teeth is true if any of the
>> + * pressed keys for this row has other pressed keys in its column. If
>> + * at any point of the scan we find that a row has multiple pressed
>> + * keys, and at least one of them is at the intersection with a column
>> + * with multiple pressed keys, we're sure there is ghosting.
>> + * Conversely, if there is ghosting, we will detect such situation for
>> + * at least one key during the pass.
>> + *
>> + * (*) This looks linear in the number of keys, but it's not. We can
>> + * cheat because the number of rows is small.
>> + */
>> + for (row = 0; row < ckdev->rows; row++) {
>> + mask = 1 << row;
>> + for (col = 0; col < ckdev->cols; col++) {
>> + if (buf[col] & mask) {
>> + pressed_in_row[row] += 1;
>
> Just ++ please.
Done
>
>> + row_has_teeth[row] |= buf[col] & ~mask;
>> + if (pressed_in_row[row] > 1 &&
>> + row_has_teeth[row]) {
>> + /* ghosting */
>> + dev_dbg(ckdev->dev,
>> + "ghost found at: r%d c%d,"
>> + " pressed %d, teeth 0x%x\n",
>
> Please do not break message strings even if they push you over 80 columns.
Done
>
>> + row, col, pressed_in_row[row],
>> + row_has_teeth[row]);
>> + return true;
>> + }
>
> I am confused why you need pressed_in_row and row_has_teeth arrays as
> you are working with one row at a time.
Hmmm I never did quite grok that code. I can't see why we need an
array, so have changed it.
>
> Also, can we move inner loop into a separate function?
Done
>
>> + }
>> + }
>> + }
>> + return false;
>> +}
>> +
>> +/*
>> + * Compares the new keyboard state to the old one and produces key
>> + * 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,
>> + uint8_t *kb_state, int len)
>> +{
>> + int col, row;
>> + int new_state;
>> + int num_cols;
>> +
>> + num_cols = len;
>> +
>> + if (ckdev->ghost_filter && cros_ec_keyb_has_ghosting(ckdev, kb_state)) {
>> + /*
>> + * Simple-minded solution: ignore this state. The obvious
>> + * improvement is to only ignore changes to keys involved in
>> + * the ghosting, but process the other changes.
>> + */
>> + dev_dbg(ckdev->dev, "ghosting found\n");
>> + return;
>> + }
>> +
>> + for (col = 0; col < ckdev->cols; col++) {
>> + for (row = 0; row < ckdev->rows; row++) {
>> + int code = MATRIX_SCAN_CODE(row, col, ckdev->row_shift);
>> +
>> + new_state = kb_state[col] & (1 << row);
>> + if (!!new_state != ckdev->old_state[code]) {
>> + dev_dbg(ckdev->dev,
>> + "changed: [r%d c%d]: byte %02x\n",
>> + row, col, new_state);
>> + }
>> + if (new_state && !ckdev->old_state[code]) {
>> + /* key press */
>> + cros_ec_keyb_send_key_event(ckdev, row, col, 1);
>> + ckdev->old_state[code] = 1;
>> + } else if (!new_state && ckdev->old_state[code]) {
>> + /* key release */
>> + cros_ec_keyb_send_key_event(ckdev, row, col, 0);
>> + ckdev->old_state[code] = 0;
>> + }
>
> Should not all of the above be:
>
> if (!!new_state != test_bit(code, dev->key)) {
> dev_dbg(ckdev->dev,
> "changed: [r%d c%d]: byte %02x\n",
> row, col, new_state);
>
> input_report_key(idev, keycodes[code], new_state);
> }
>
> and yo can get rid of old_state altogether?
That's cool. Done.
>
>> + }
>> + }
>> + input_sync(ckdev->idev);
>> +}
>> +
>> +static int cros_ec_keyb_open(struct input_dev *dev)
>> +{
>> + struct cros_ec_keyb *ckdev = input_get_drvdata(dev);
>> + int ret;
>> +
>> + ret = blocking_notifier_chain_register(&ckdev->ec->event_notifier,
>> + &ckdev->notifier);
>> + if (ret)
>> + return ret;
>> + ret = blocking_notifier_chain_register(&ckdev->ec->wake_notifier,
>> + &ckdev->wake_notifier);
>> + if (ret) {
>> + blocking_notifier_chain_unregister(
>> + &ckdev->ec->event_notifier, &ckdev->notifier);
>> + return ret;
>> + }
>> +
>> + return 0;
>> +}
>> +
>> +static void cros_ec_keyb_close(struct input_dev *dev)
>> +{
>> + struct cros_ec_keyb *ckdev = input_get_drvdata(dev);
>> +
>> + blocking_notifier_chain_unregister(&ckdev->ec->event_notifier,
>> + &ckdev->notifier);
>> + blocking_notifier_chain_unregister(&ckdev->ec->wake_notifier,
>> + &ckdev->wake_notifier);
>
> Why is this done via a notifier instead of regular resume method?
Because we only call the notifer in resume when we were not waking on
a keyboard event. We use it to flush the keyboard. It was a late
change so there might be a better way, but this driver does not have a
resume handler.
>
>> +}
>> +
>> +static int cros_ec_keyb_get_state(struct cros_ec_keyb *ckdev, uint8_t *kb_state)
>> +{
>> + return ckdev->ec->command_recv(ckdev->ec, EC_CMD_MKBP_STATE,
>> + kb_state, ckdev->cols);
>> +}
>> +
>> +static int cros_ec_keyb_work(struct notifier_block *nb,
>> + unsigned long state, void *_notify)
>> +{
>> + int ret;
>> + struct cros_ec_keyb *ckdev = container_of(nb, struct cros_ec_keyb,
>> + notifier);
>> + uint8_t kb_state[ckdev->cols];
>> +
>> + ret = cros_ec_keyb_get_state(ckdev, kb_state);
>> + if (ret >= 0)
>> + cros_ec_keyb_process(ckdev, kb_state, ret);
>> +
>> + return NOTIFY_DONE;
>> +}
>> +
>> +/* On resume, clear any keys in the buffer */
>> +static int cros_ec_keyb_clear_keyboard(struct notifier_block *nb,
>> + unsigned long state, void *_notify)
>> +{
>> + struct cros_ec_keyb *ckdev = container_of(nb, struct cros_ec_keyb,
>> + wake_notifier);
>> + uint8_t old_state[ckdev->cols];
>> + uint8_t new_state[ckdev->cols];
>> + unsigned long duration;
>> + int i, ret;
>> +
>> + /*
>> + * Keep reading until we see that the scan state does not change.
>> + * That indicates that we are done.
>> + *
>> + * Assume that the EC keyscan buffer is at most 32 deep.
>> + */
>> + duration = jiffies;
>> + ret = cros_ec_keyb_get_state(ckdev, new_state);
>> + 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)))
>> + break;
>> + }
>> + duration = jiffies - duration;
>> + dev_info(ckdev->dev, "Discarded %d keyscan(s) in %dus\n", i,
>> + jiffies_to_usecs(duration));
>> +
>> + return 0;
>> +}
>> +
>> +static const struct of_device_id cros_ec_kbc_of_match[] = {
>> + { .compatible = "google,cros-ec-keyb", },
>> + { },
>> +};
>> +
>> +static int cros_ec_keyb_probe(struct platform_device *pdev)
>> +{
>> + struct cros_ec_device *ec = dev_get_drvdata(pdev->dev.parent);
>> + struct device *dev = ec->dev;
>> + struct cros_ec_keyb *ckdev = NULL;
>> + struct input_dev *idev = NULL;
>> + struct device_node *np;
>> + int err;
>> +
>> + np = of_find_matching_node(NULL, cros_ec_kbc_of_match);
>
> And if we don't find it?
Added error checking.
>
>> +
>> + ckdev = kzalloc(sizeof(*ckdev), GFP_KERNEL);
>> + if (!ckdev) {
>> + dev_err(dev, "cannot allocate memory for ckdev\n");
>> + return -ENOMEM;
>> + }
>> + pdev->dev.of_node = np;
>
> Huh? I'd expect the platform device be fully set up (including DT data)
> before the driver is called.
This is a child of the mfd driver cros_ec, so I don't think that
works. Or maybe I'm just not sure how to plumb it in so it is
automatic. Or maybe I just need to add the id to the device info
below?
>
>> + err = matrix_keypad_parse_of_params(&pdev->dev, &ckdev->rows,
>> + &ckdev->cols);
>> + if (err)
>> + goto fail_alloc_dev;
>> +
>> + idev = input_allocate_device();
>> + if (!idev) {
>> + err = -ENOMEM;
>> + dev_err(dev, "cannot allocate memory for input device\n");
>> + goto fail_alloc_dev;
>> + }
>> +
>> + ckdev->ec = ec;
>> + ckdev->notifier.notifier_call = cros_ec_keyb_work;
>> + ckdev->wake_notifier.notifier_call = cros_ec_keyb_clear_keyboard;
>> + ckdev->dev = dev;
>> + dev_set_drvdata(&pdev->dev, ckdev);
>> +
>> + idev->name = ec->get_name(ec);
>> + idev->phys = ec->get_phys_name(ec);
>> + __set_bit(EV_REP, idev->evbit);
>> +
>> + idev->id.bustype = BUS_VIRTUAL;
>> + idev->id.version = 1;
>> + idev->id.product = 0;
>> + idev->dev.parent = &pdev->dev;
>> + idev->open = cros_ec_keyb_open;
>> + idev->close = cros_ec_keyb_close;
>> +
>> + ckdev->ghost_filter = of_property_read_bool(np,
>> + "google,needs-ghost-filter");
>> +
>> + err = matrix_keypad_build_keymap(NULL, NULL, ckdev->rows, ckdev->cols,
>> + NULL, idev);
>> + if (err) {
>> + dev_err(dev, "cannot build key matrix\n");
>> + goto fail_matrix;
>> + }
>> +
>> + ckdev->row_shift = get_count_order(ckdev->cols);
>> + ckdev->old_state = kzalloc(idev->keycodemax, GFP_KERNEL);
>> + if (!ckdev->old_state) {
>> + dev_err(dev, "Cannot allocate memory for old_state\n");
>> + err = -ENOMEM;
>> + goto fail_old_state;
>> + }
>
> Not needed I believe.
Dropped.
>
>> +
>> + input_set_capability(idev, EV_MSC, MSC_SCAN);
>> + input_set_drvdata(idev, ckdev);
>> + ckdev->idev = idev;
>> + err = input_register_device(ckdev->idev);
>> + if (err) {
>> + dev_err(dev, "cannot register input device\n");
>> + goto fail_register;
>> + }
>> +
>> + return 0;
>> +
>> +fail_register:
>> + kfree(ckdev->old_state);
>> +fail_old_state:
>> + kfree(idev->keycode);
>> +fail_matrix:
>> + input_free_device(idev);
>> +fail_alloc_dev:
>> + kfree(ckdev);
>> + return err;
>> +}
>> +
>> +static int cros_ec_keyb_remove(struct platform_device *pdev)
>> +{
>> + struct cros_ec_keyb *ckdev = dev_get_drvdata(&pdev->dev);
>
> platform_get_drvdata() please.
Done
>
>> + struct input_dev *idev = ckdev->idev;
>> +
>> + /* I believe we leak a matrix_keymap here */
>
> How? It is devm-managed.
Original code might pre-date that. Removed comment.
>
>> + input_unregister_device(idev);
>> + kfree(ckdev->old_state);
>> + kfree(idev->keycode);
>
> And since it is devm-managed you should not free it yourself. Actually
> idev is most likely gone at this point already.
Yes, done.
>
>> + input_free_device(idev);
>
> Do not call input_free_device() after input_unregister_device().
Done
>
>> + kfree(ckdev);
>> +
>> + return 0;
>> +}
>> +
>> +static struct platform_driver cros_ec_keyb_driver = {
>> + .probe = cros_ec_keyb_probe,
>> + .remove = cros_ec_keyb_remove,
>> + .driver = {
>> + .name = "cros-ec-keyb",
>> + },
>> +};
>> +
>> +module_platform_driver(cros_ec_keyb_driver);
>> +
>> +MODULE_LICENSE("GPL");
>> +MODULE_DESCRIPTION("ChromeOS EC keyboard driver");
>> +MODULE_ALIAS("platform:cros-ec-keyb");
>> --
>> 1.8.1
>>
>
> Thanks.
>
> --
> Dmitry
I'll send a new version so that the above is done, at least.
Regards,
Simon
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Powered by blists - more mailing lists