* Re: [PATCH 5/6] Staging/iio/adc/touchscreen/MXS: add interrupt driven touch detection
From: Jürgen Beisert @ 2013-09-16 14:34 UTC (permalink / raw)
To: Marek Vasut
Cc: linux-arm-kernel-IAPFreCvJWM7uuMidbF8XUB+6BGkLq7r,
Jonathan Cameron, linux-iio-u79uwXL29TY76Z2rM5mHXA,
devel-gWbeCf7V1WCQmaza687I9mD2FQJk+8+b,
fabio.estevam-KZfg59tc24xl57MIdRCFDg,
jic23-KWPb1pKIrIJaa/9Udqfwiw, Torokhov,
linux-input-u79uwXL29TY76Z2rM5mHXA@public.gmane.org,
Otavio Salvador
In-Reply-To: <201309161623.48247.marex-ynQEQJNshbs@public.gmane.org>
Hi Marek,
On Monday 16 September 2013 16:23:48 Marek Vasut wrote:
> > On Sunday 15 September 2013 12:56:25 Jonathan Cameron wrote:
> > > On 09/11/13 09:18, Juergen Beisert wrote:
> > > > For battery driven systems it is a very bad idea to collect the
> > > > touchscreen data within a kernel busy loop.
> > > >
> > > > This change uses the features of the hardware to delay and accumulate
> > > > samples in hardware to avoid a high interrupt and CPU load.
> > > >
> > > > Note: this is only tested on an i.MX23 SoC yet.
> > > >
> > > > Signed-off-by: Juergen Beisert <jbe-bIcnvbaLZ9MEGnE8C9+IrQ@public.gmane.org>
> > > > CC: linux-arm-kernel-IAPFreCvJWM7uuMidbF8XUB+6BGkLq7r@public.gmane.org
> > > > CC: devel-gWbeCf7V1WCQmaza687I9mD2FQJk+8+b@public.gmane.org
> > > > CC: Marek Vasut <marex-ynQEQJNshbs@public.gmane.org>
> > > > CC: Fabio Estevam <fabio.estevam-KZfg59tc24xl57MIdRCFDg@public.gmane.org>
> > > > CC: Jonathan Cameron <jic23-KWPb1pKIrIJaa/9Udqfwiw@public.gmane.org>
> > >
> > > While this driver is placed in IIO within staging at the moment, these
> > > changes are definitely input related. Hence I have cc'd Dmitry and the
> > > input list.
> > >
> > > I am personaly a little uncomfortable that we have such a complex bit
> > > of input code sat within an IIO driver but such is life.
> >
> > Maybe an MFD for this ADC unit would be a better way to go? Currently I
> > have a different problem with this driver, because the ADC unit monitors
> > the battery as well. And the charging driver from the power subsystem
> > needs these values to charge the battery in a correct manner.
>
> Are you planning to post the power block patches too ?
When they will work: yes. Currently it crashes all the time. The PMIC is a
beast...
Regards,
Juergen
--
Pengutronix e.K. | Juergen Beisert |
Linux Solutions for Science and Industry | http://www.pengutronix.de/ |
^ permalink raw reply
* RE: [PATCH 1/1] Drivers: input: serio: New driver to support Hyper-V synthetic keyboard
From: KY Srinivasan @ 2013-09-16 14:46 UTC (permalink / raw)
To: Dan Carpenter
Cc: olaf@aepfle.de, gregkh@linuxfoundation.org, jasowang@redhat.com,
dmitry.torokhov@gmail.com, linux-kernel@vger.kernel.org,
vojtech@suse.cz, linux-input@vger.kernel.org, apw@canonical.com,
devel@linuxdriverproject.org
In-Reply-To: <20130916082110.GN25896@mwanda>
> -----Original Message-----
> From: Dan Carpenter [mailto:dan.carpenter@oracle.com]
> Sent: Monday, September 16, 2013 1:21 AM
> To: KY Srinivasan
> Cc: gregkh@linuxfoundation.org; linux-kernel@vger.kernel.org;
> devel@linuxdriverproject.org; linux-input@vger.kernel.org;
> dmitry.torokhov@gmail.com; vojtech@suse.cz; olaf@aepfle.de;
> apw@canonical.com; jasowang@redhat.com
> Subject: Re: [PATCH 1/1] Drivers: input: serio: New driver to support Hyper-V
> synthetic keyboard
>
> The main thing is that could you improve the error handling in
> hv_kbd_on_channel_callback() explained inline.
Thanks Dan. I will address all the relevant issues you have pointed out.
I have answered/responded to your comments in-line.
>
> On Sun, Sep 15, 2013 at 10:28:54PM -0700, K. Y. Srinivasan wrote:
> > Add a new driver to support synthetic keyboard. On the next generation
> > Hyper-V guest firmware, many legacy devices will not be emulated and this
> > driver will be required.
> >
> > I would like to thank Vojtech Pavlik <vojtech@suse.cz> for helping me with the
> > details of the AT keyboard driver.
> >
> > Signed-off-by: K. Y. Srinivasan <kys@microsoft.com>
> > ---
> > drivers/input/serio/Kconfig | 7 +
> > drivers/input/serio/Makefile | 1 +
> > drivers/input/serio/hyperv-keyboard.c | 379
> +++++++++++++++++++++++++++++++++
> > 3 files changed, 387 insertions(+), 0 deletions(-)
> > create mode 100644 drivers/input/serio/hyperv-keyboard.c
> >
> > diff --git a/drivers/input/serio/Kconfig b/drivers/input/serio/Kconfig
> > index 1e691a3..f3996e7 100644
> > --- a/drivers/input/serio/Kconfig
> > +++ b/drivers/input/serio/Kconfig
> > @@ -267,4 +267,11 @@ config SERIO_OLPC_APSP
> > To compile this driver as a module, choose M here: the module will
> > be called olpc_apsp.
> >
> > +config HYPERV_KEYBOARD
> > + tristate "Microsoft Synthetic Keyboard driver"
> > + depends on HYPERV
> > + default HYPERV
> > + help
> > + Select this option to enable the Hyper-V Keyboard driver.
> > +
> > endif
> > diff --git a/drivers/input/serio/Makefile b/drivers/input/serio/Makefile
> > index 12298b1..815d874 100644
> > --- a/drivers/input/serio/Makefile
> > +++ b/drivers/input/serio/Makefile
> > @@ -28,3 +28,4 @@ obj-$(CONFIG_SERIO_ALTERA_PS2) += altera_ps2.o
> > obj-$(CONFIG_SERIO_ARC_PS2) += arc_ps2.o
> > obj-$(CONFIG_SERIO_APBPS2) += apbps2.o
> > obj-$(CONFIG_SERIO_OLPC_APSP) += olpc_apsp.o
> > +obj-$(CONFIG_HYPERV_KEYBOARD) += hyperv-keyboard.o
> > diff --git a/drivers/input/serio/hyperv-keyboard.c
> b/drivers/input/serio/hyperv-keyboard.c
> > new file mode 100644
> > index 0000000..0d4625f
> > --- /dev/null
> > +++ b/drivers/input/serio/hyperv-keyboard.c
> > @@ -0,0 +1,379 @@
> > +/*
> > + * Copyright (c) 2013, Microsoft Corporation.
> > + *
> > + * This program is free software; you can redistribute it and/or modify it
> > + * under the terms and conditions of the GNU General Public License,
> > + * version 2, as published by the Free Software Foundation.
> > + *
> > + * This program is distributed in the hope 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.
> > + */
> > +#include <linux/init.h>
> > +#include <linux/module.h>
> > +#include <linux/device.h>
> > +#include <linux/completion.h>
> > +#include <linux/hyperv.h>
> > +#include <linux/serio.h>
> > +#include <linux/slab.h>
> > +
> > +
>
> Extra blank line.
>
> > +/*
> > + * Current version 1.0
> > + *
> > + */
> > +#define SYNTH_KBD_VERSION_MAJOR 1
> > +#define SYNTH_KBD_VERSION_MINOR 0
> > +#define SYNTH_KBD_VERSION (SYNTH_KBD_VERSION_MINOR |
> \
> > + (SYNTH_KBD_VERSION_MAJOR << 16))
> > +
> > +
> > +/*
> > + * Message types in the synthetic input protocol
> > + */
> > +enum synth_kbd_msg_type {
> > + SYNTH_KBD_PROTOCOL_REQUEST = 1,
> > + SYNTH_KBD_PROTOCOL_RESPONSE = 2,
> > + SYNTH_KBD_EVENT = 3,
> > + SYNTH_KBD_LED_INDICATORS = 4,
> > +};
> > +
> > +/*
> > + * Basic message structures.
> > + */
> > +struct synth_kbd_msg_hdr {
> > + enum synth_kbd_msg_type type;
> > +};
>
> Enum type is wrong here because sizeof(enum) is up to the compiler to
> decide.
>
> > +
> > +struct synth_kbd_msg {
> > + struct synth_kbd_msg_hdr header;
> > + char data[1]; /* Enclosed message */
> > +};
>
> You could use a zero size array instead.
>
> > +
> > +union synth_kbd_version {
> > + struct {
> > + u16 minor_version;
> > + u16 major_version;
> > + };
> > + u32 version;
> > +};
> > +
> > +/*
> > + * Protocol messages
> > + */
> > +struct synth_kbd_protocol_request {
> > + struct synth_kbd_msg_hdr header;
> > + union synth_kbd_version version_requested;
> > +};
> > +
> > +struct synth_kbd_protocol_response {
> > + struct synth_kbd_msg_hdr header;
> > + u32 accepted:1;
> > + u32 reserved:31;
> > +};
> > +
> > +struct synth_kbd_keystroke {
> > + struct synth_kbd_msg_hdr header;
> > + u16 make_code;
> > + u16 reserved0;
> > + u32 is_unicode:1;
> > + u32 is_break:1;
> > + u32 is_e0:1;
> > + u32 is_e1:1;
> > + u32 reserved:28;
> > +};
> > +
> > +
>
> Extra blank line.
>
> > +#define HK_MAXIMUM_MESSAGE_SIZE 256
> > +
> > +#define KBD_VSC_SEND_RING_BUFFER_SIZE (10 * PAGE_SIZE)
> > +#define KBD_VSC_RECV_RING_BUFFER_SIZE (10 * PAGE_SIZE)
> > +
> > +#define XTKBD_EMUL0 0xe0
> > +#define XTKBD_EMUL1 0xe1
> > +#define XTKBD_RELEASE 0x80
> > +
> > +
>
> Extra blank.
>
> > +/*
> > + * Represents a keyboard device
> > + */
> > +struct hv_kbd_dev {
> > + unsigned char keycode[256];
> > + struct hv_device *device;
> > + struct synth_kbd_protocol_request protocol_req;
> > + struct synth_kbd_protocol_response protocol_resp;
> > + /* Synchronize the request/response if needed */
> > + struct completion wait_event;
> > + struct serio *hv_serio;
> > +};
> > +
> > +
>
> Extra blank.
>
> > +static struct hv_kbd_dev *hv_kbd_alloc_device(struct hv_device *device)
> > +{
> > + struct hv_kbd_dev *kbd_dev;
> > + struct serio *hv_serio;
> > +
> > + kbd_dev = kzalloc(sizeof(struct hv_kbd_dev), GFP_KERNEL);
> > +
>
> Spurious blank line.
>
> > + if (!kbd_dev)
> > + return NULL;
> > +
> > + hv_serio = kzalloc(sizeof(struct serio), GFP_KERNEL);
> > +
>
> Spurious blank.
>
> > + if (hv_serio == NULL) {
> > + kfree(kbd_dev);
> > + return NULL;
> > + }
> > +
> > + hv_serio->id.type = SERIO_8042_XL;
>
> Pointless tab before the '='.
>
> > + strlcpy(hv_serio->name, dev_name(&device->device),
> > + sizeof(hv_serio->name));
> > + strlcpy(hv_serio->phys, dev_name(&device->device),
> > + sizeof(hv_serio->phys));
> > + hv_serio->dev.parent = &device->device;
>
> Why are there two spaces before the '='?
>
> > +
> > +
>
> Extra blank line.
>
> > + kbd_dev->device = device;
> > + kbd_dev->hv_serio = hv_serio;
> > + hv_set_drvdata(device, kbd_dev);
> > + init_completion(&kbd_dev->wait_event);
> > +
> > + return kbd_dev;
> > +}
> > +
> > +static void hv_kbd_free_device(struct hv_kbd_dev *device)
> > +{
> > + serio_unregister_port(device->hv_serio);
> > + kfree(device->hv_serio);
> > + hv_set_drvdata(device->device, NULL);
> > + kfree(device);
> > +}
> > +
> > +static void hv_kbd_on_receive(struct hv_device *device,
> > + struct vmpacket_descriptor *packet)
> > +{
> > + struct synth_kbd_msg *msg;
> > + struct hv_kbd_dev *kbd_dev = hv_get_drvdata(device);
> > + struct synth_kbd_keystroke *ks_msg;
> > + u16 scan_code;
> > +
> > + msg = (struct synth_kbd_msg *)((unsigned long)packet +
> > + (packet->offset8 << 3));
> > +
> > + switch (msg->header.type) {
> > + case SYNTH_KBD_PROTOCOL_RESPONSE:
> > + memcpy(&kbd_dev->protocol_resp, msg,
> > + sizeof(struct synth_kbd_protocol_response));
> > + complete(&kbd_dev->wait_event);
> > + break;
> > + case SYNTH_KBD_EVENT:
> > + ks_msg = (struct synth_kbd_keystroke *)msg;
> > + scan_code = ks_msg->make_code;
> > +
> > + /*
> > + * Inject the information through the serio interrupt.
> > + */
> > + if (ks_msg->is_e0)
> > + serio_interrupt(kbd_dev->hv_serio, XTKBD_EMUL0, 0);
> > + serio_interrupt(kbd_dev->hv_serio,
> > + scan_code | (ks_msg->is_break ?
> > + XTKBD_RELEASE : 0),
> > + 0);
> > +
>
> It would be more readable to do:
>
> if (ks_msg->is_break)
> scan_code |= XTKBD_RELEASE;
> serio_interrupt(kbd_dev->hv_serio, scan_code, 0);
>
>
> > + break;
> > +
> > + default:
> > + pr_err("unhandled message type %d\n", msg->header.type);
>
> Just a question. This can only be triggered by the hyperviser, right?
Yes.
>
> > + }
> > +}
> > +
> > +static void hv_kbd_on_channel_callback(void *context)
> > +{
> > + const int packet_size = 0x100;
> > + int ret;
> > + struct hv_device *device = context;
> > + u32 bytes_recvd;
> > + u64 req_id;
> > + struct vmpacket_descriptor *desc;
> > + unsigned char *buffer;
> > + int bufferlen = packet_size;
> > +
> > + buffer = kmalloc(bufferlen, GFP_ATOMIC);
> > + if (!buffer)
> > + return;
> > +
> > + do {
>
>
> Forever loops should be while (1) { instead of do { } while (1).
>
>
> > + ret = vmbus_recvpacket_raw(device->channel, buffer,
> > + bufferlen, &bytes_recvd, &req_id);
> > +
> > + switch (ret) {
> > + case 0:
> > + if (bytes_recvd <= 0) {
>
> There can never be a negative number of bytes_recvd.
>
> > + kfree(buffer);
> > + return;
> > + }
> > + desc = (struct vmpacket_descriptor *)buffer;
> > +
> > + switch (desc->type) {
> > + case VM_PKT_COMP:
> > + break;
> > +
> > + case VM_PKT_DATA_INBAND:
> > + hv_kbd_on_receive(device, desc);
>
> This is the error handling I mentioned at the top. hv_kbd_on_receive()
> doesn't take into consideration the amount of data we recieved, it
> trusts the offset we recieved from the user. There is an out of bounds
> read.
What user are you referring to. The message is sent by the host - the user keystroke
is normalized into a fixed size packet by the host and sent to the guest. We will parse this
packet, based on the host specified layout here.
>
> > + break;
> > +
> > + default:
> > + pr_err("unhandled packet type %d, tid %llx len
> %d\n",
> > + desc->type, req_id, bytes_recvd);
> > + break;
> > + }
> > +
> > + break;
> > +
> > + case -ENOBUFS:
> > + kfree(buffer);
> > + /* Handle large packet */
> > + bufferlen = bytes_recvd;
> > + buffer = kmalloc(bytes_recvd, GFP_ATOMIC);
> > +
> > + if (!buffer)
> > + return;
> > +
> > + break;
> > + }
> > + } while (1);
> > +
> > +}
> > +
> > +static int hv_kbd_connect_to_vsp(struct hv_device *device)
> > +{
> > + int ret = 0;
>
> Don't initialize this.
>
> > + int t;
> > + struct hv_kbd_dev *kbd_dev = hv_get_drvdata(device);
> > + struct synth_kbd_protocol_request *request;
> > + struct synth_kbd_protocol_response *response;
> > +
> > + request = &kbd_dev->protocol_req;
> > + memset(request, 0, sizeof(struct synth_kbd_protocol_request));
> > +
> > + request->header.type = SYNTH_KBD_PROTOCOL_REQUEST;
> > + request->version_requested.version = SYNTH_KBD_VERSION;
> > +
> > + ret = vmbus_sendpacket(device->channel, request,
> > + sizeof(struct synth_kbd_protocol_request),
> > + (unsigned long)request,
> > + VM_PKT_DATA_INBAND,
> > +
> VMBUS_DATA_PACKET_FLAG_COMPLETION_REQUESTED);
> > + if (ret)
> > + goto cleanup;
>
> There is no cleanup. Just return directly.
>
> > +
> > + t = wait_for_completion_timeout(&kbd_dev->wait_event, 10 * HZ);
> > + if (!t) {
> > + ret = -ETIMEDOUT;
> > + goto cleanup;
>
> return -ETIMEOUT;
>
> > + }
> > +
> > + response = &kbd_dev->protocol_resp;
> > +
> > + if (!response->accepted) {
> > + pr_err("synth_kbd protocol request failed (version %d)\n",
> > + SYNTH_KBD_VERSION);
> > + ret = -ENODEV;
> > + goto cleanup;
>
> Just return -ENODEV;
>
> > + }
> > +
>
> return 0;
>
> > +
> > +cleanup:
> > + return ret;
> > +}
> > +
> > +static int hv_kbd_probe(struct hv_device *device,
> > + const struct hv_vmbus_device_id *dev_id)
> > +{
> > + int ret;
> > + struct hv_kbd_dev *kbd_dev;
> > +
> > + kbd_dev = hv_kbd_alloc_device(device);
> > +
>
> Delete the blank line.
>
> > + if (!kbd_dev)
> > + return -ENOMEM;
> > +
> > + ret = vmbus_open(device->channel,
> > + KBD_VSC_SEND_RING_BUFFER_SIZE,
> > + KBD_VSC_RECV_RING_BUFFER_SIZE,
> > + NULL,
> > + 0,
> > + hv_kbd_on_channel_callback,
> > + device
> > + );
> > +
>
> Delete the blank line.
>
> > + if (ret)
> > + goto probe_err0;
> > +
> > + ret = hv_kbd_connect_to_vsp(device);
> > +
>
> Delete the blank line.
>
> > + if (ret)
> > + goto probe_err1;
> > +
> > + serio_register_port(kbd_dev->hv_serio);
> > +
> > + return ret;
>
> return 0;
>
> > +
> > +probe_err1:
> > + vmbus_close(device->channel);
>
> The label here should be "err_close:". The number is more GW-Basic
> style than C.
>
> > +
> > +probe_err0:
>
> The label should be "err_free_dev:".
>
> > + hv_kbd_free_device(kbd_dev);
> > +
> > + return ret;
> > +}
> > +
> > +
>
> Extra blank line.
>
> > +static int hv_kbd_remove(struct hv_device *dev)
>
> regards,
> dan carpenter
Regards,
K. Y
^ permalink raw reply
* [PATCH] Input: atmel_mxt_ts - update to devm_* API
From: Manish Badarkhe @ 2013-09-16 14:50 UTC (permalink / raw)
To: linux-input, linux-kernel; +Cc: josh.wu, dmitry.torokhov, badarkhe.manish
Update the code to use devm_* API so that driver core will manage
resources.
Signed-off-by: Manish Badarkhe <badarkhe.manish@gmail.com>
---
This is just clean up of code to manage resources using "devm_"
funtions. Not tested on hardware.
:100644 100644 59aa240... 73c5ad0... M drivers/input/touchscreen/atmel_mxt_ts.c
drivers/input/touchscreen/atmel_mxt_ts.c | 27 +++++++++++----------------
1 file changed, 11 insertions(+), 16 deletions(-)
diff --git a/drivers/input/touchscreen/atmel_mxt_ts.c b/drivers/input/touchscreen/atmel_mxt_ts.c
index 59aa240..73c5ad0 100644
--- a/drivers/input/touchscreen/atmel_mxt_ts.c
+++ b/drivers/input/touchscreen/atmel_mxt_ts.c
@@ -1139,12 +1139,12 @@ static int mxt_probe(struct i2c_client *client,
if (!pdata)
return -EINVAL;
- data = kzalloc(sizeof(struct mxt_data), GFP_KERNEL);
- input_dev = input_allocate_device();
+ data = devm_kzalloc(&client->dev, sizeof(struct mxt_data), GFP_KERNEL);
+ input_dev = devm_input_allocate_device(&client->dev);
if (!data || !input_dev) {
dev_err(&client->dev, "Failed to allocate memory\n");
error = -ENOMEM;
- goto err_free_mem;
+ goto err_out;
}
data->is_tp = pdata && pdata->is_tp;
@@ -1170,7 +1170,7 @@ static int mxt_probe(struct i2c_client *client,
error = mxt_initialize(data);
if (error)
- goto err_free_mem;
+ goto err_out;
__set_bit(EV_ABS, input_dev->evbit);
__set_bit(EV_KEY, input_dev->evbit);
@@ -1224,9 +1224,10 @@ static int mxt_probe(struct i2c_client *client,
input_set_drvdata(input_dev, data);
i2c_set_clientdata(client, data);
- error = request_threaded_irq(client->irq, NULL, mxt_interrupt,
- pdata->irqflags | IRQF_ONESHOT,
- client->name, data);
+ error = devm_request_threaded_irq(&client->dev,
+ client->irq, NULL, mxt_interrupt,
+ pdata->irqflags | IRQF_ONESHOT,
+ client->name, data);
if (error) {
dev_err(&client->dev, "Failed to register interrupt\n");
goto err_free_object;
@@ -1234,11 +1235,11 @@ static int mxt_probe(struct i2c_client *client,
error = mxt_make_highchg(data);
if (error)
- goto err_free_irq;
+ goto err_free_object;
error = input_register_device(input_dev);
if (error)
- goto err_free_irq;
+ goto err_free_object;
error = sysfs_create_group(&client->dev.kobj, &mxt_attr_group);
if (error)
@@ -1249,13 +1250,9 @@ static int mxt_probe(struct i2c_client *client,
err_unregister_device:
input_unregister_device(input_dev);
input_dev = NULL;
-err_free_irq:
- free_irq(client->irq, data);
err_free_object:
kfree(data->object_table);
-err_free_mem:
- input_free_device(input_dev);
- kfree(data);
+err_out:
return error;
}
@@ -1264,10 +1261,8 @@ static int mxt_remove(struct i2c_client *client)
struct mxt_data *data = i2c_get_clientdata(client);
sysfs_remove_group(&client->dev.kobj, &mxt_attr_group);
- free_irq(data->irq, data);
input_unregister_device(data->input_dev);
kfree(data->object_table);
- kfree(data);
return 0;
}
--
1.7.10.4
^ permalink raw reply related
* [PATCH] Input: atmel_tscadcc - update to devm_* API
From: Manish Badarkhe @ 2013-09-16 14:52 UTC (permalink / raw)
To: linux-input, linux-kernel; +Cc: josh.wu, dmitry.torokhov, badarkhe.manish
Update the code to use devm_* API so that driver core will manage
resources.
Signed-off-by: Manish Badarkhe <badarkhe.manish@gmail.com>
---
This is just clean up of code to manage resources using "devm_"
funtions. Not tested on hardware.
:100644 100644 bddabc5... 4ddf97c... M drivers/input/touchscreen/atmel_tsadcc.c
drivers/input/touchscreen/atmel_tsadcc.c | 53 ++++++++----------------------
1 file changed, 13 insertions(+), 40 deletions(-)
diff --git a/drivers/input/touchscreen/atmel_tsadcc.c b/drivers/input/touchscreen/atmel_tsadcc.c
index bddabc5..4ddf97c 100644
--- a/drivers/input/touchscreen/atmel_tsadcc.c
+++ b/drivers/input/touchscreen/atmel_tsadcc.c
@@ -197,53 +197,47 @@ static int atmel_tsadcc_probe(struct platform_device *pdev)
}
/* Allocate memory for device */
- ts_dev = kzalloc(sizeof(struct atmel_tsadcc), GFP_KERNEL);
+ ts_dev = devm_kzalloc(&pdev->dev, sizeof(struct atmel_tsadcc),
+ GFP_KERNEL);
if (!ts_dev) {
dev_err(&pdev->dev, "failed to allocate memory.\n");
return -ENOMEM;
}
platform_set_drvdata(pdev, ts_dev);
- input_dev = input_allocate_device();
+ input_dev = devm_input_allocate_device(&pdev->dev);
if (!input_dev) {
dev_err(&pdev->dev, "failed to allocate input device.\n");
err = -EBUSY;
- goto err_free_mem;
+ goto err_out;
}
ts_dev->irq = platform_get_irq(pdev, 0);
if (ts_dev->irq < 0) {
dev_err(&pdev->dev, "no irq ID is designated.\n");
err = -ENODEV;
- goto err_free_dev;
+ goto err_out;
}
- if (!request_mem_region(res->start, resource_size(res),
- "atmel tsadcc regs")) {
- dev_err(&pdev->dev, "resources is unavailable.\n");
- err = -EBUSY;
- goto err_free_dev;
- }
-
- tsc_base = ioremap(res->start, resource_size(res));
+ tsc_base = devm_ioremap_resource(&pdev->dev, res);
if (!tsc_base) {
dev_err(&pdev->dev, "failed to map registers.\n");
err = -ENOMEM;
- goto err_release_mem;
+ goto err_out;
}
- err = request_irq(ts_dev->irq, atmel_tsadcc_interrupt, 0,
- pdev->dev.driver->name, ts_dev);
+ err = devm_request_irq(&pdev->dev, ts_dev->irq, atmel_tsadcc_interrupt,
+ 0, pdev->dev.driver->name, ts_dev);
if (err) {
dev_err(&pdev->dev, "failed to allocate irq.\n");
- goto err_unmap_regs;
+ goto err_out;
}
- ts_dev->clk = clk_get(&pdev->dev, "tsc_clk");
+ ts_dev->clk = devm_clk_get(&pdev->dev, "tsc_clk");
if (IS_ERR(ts_dev->clk)) {
dev_err(&pdev->dev, "failed to get ts_clk\n");
err = PTR_ERR(ts_dev->clk);
- goto err_free_irq;
+ goto err_out;
}
ts_dev->input = input_dev;
@@ -309,37 +303,17 @@ static int atmel_tsadcc_probe(struct platform_device *pdev)
err_fail:
clk_disable(ts_dev->clk);
- clk_put(ts_dev->clk);
-err_free_irq:
- free_irq(ts_dev->irq, ts_dev);
-err_unmap_regs:
- iounmap(tsc_base);
-err_release_mem:
- release_mem_region(res->start, resource_size(res));
-err_free_dev:
- input_free_device(input_dev);
-err_free_mem:
- kfree(ts_dev);
+err_out:
return err;
}
static int atmel_tsadcc_remove(struct platform_device *pdev)
{
struct atmel_tsadcc *ts_dev = platform_get_drvdata(pdev);
- struct resource *res;
-
- free_irq(ts_dev->irq, ts_dev);
input_unregister_device(ts_dev->input);
- res = platform_get_resource(pdev, IORESOURCE_MEM, 0);
- iounmap(tsc_base);
- release_mem_region(res->start, resource_size(res));
-
clk_disable(ts_dev->clk);
- clk_put(ts_dev->clk);
-
- kfree(ts_dev);
return 0;
}
@@ -356,4 +330,3 @@ module_platform_driver(atmel_tsadcc_driver);
MODULE_LICENSE("GPL");
MODULE_DESCRIPTION("Atmel TouchScreen Driver");
MODULE_AUTHOR("Dan Liang <dan.liang@atmel.com>");
-
--
1.7.10.4
^ permalink raw reply related
* Re: [PATCH 1/1] Drivers: input: serio: New driver to support Hyper-V synthetic keyboard
From: Dan Carpenter @ 2013-09-16 15:05 UTC (permalink / raw)
To: KY Srinivasan
Cc: olaf@aepfle.de, gregkh@linuxfoundation.org, jasowang@redhat.com,
dmitry.torokhov@gmail.com, linux-kernel@vger.kernel.org,
vojtech@suse.cz, linux-input@vger.kernel.org, apw@canonical.com,
devel@linuxdriverproject.org
In-Reply-To: <3b5096d0190b4440a8e25afbd22ab72d@SN2PR03MB061.namprd03.prod.outlook.com>
On Mon, Sep 16, 2013 at 02:46:24PM +0000, KY Srinivasan wrote:
> > > + case VM_PKT_DATA_INBAND:
> > > + hv_kbd_on_receive(device, desc);
> >
> > This is the error handling I mentioned at the top. hv_kbd_on_receive()
> > doesn't take into consideration the amount of data we recieved, it
> > trusts the offset we recieved from the user. There is an out of bounds
> > read.
>
> What user are you referring to. The message is sent by the host - the user keystroke
> is normalized into a fixed size packet by the host and sent to the guest. We will parse this
> packet, based on the host specified layout here.
>
The user means the hypervisor, yes.
I don't want the hypervisor accessing outside of the buffer. It is
robustness issue. Just check the offset against "bytes_recvd". It's
not complicated.
If you have a different place where the guest does this then tell me
which function to look at.
regards,
dan carpenter
^ permalink raw reply
* Re: [PATCH] Input: atmel_mxt_ts - update to devm_* API
From: Dmitry Torokhov @ 2013-09-16 15:06 UTC (permalink / raw)
To: Manish Badarkhe; +Cc: linux-input, linux-kernel, josh.wu
In-Reply-To: <1379343016-6354-1-git-send-email-badarkhe.manish@gmail.com>
Hi Manish,
On Mon, Sep 16, 2013 at 08:20:16PM +0530, Manish Badarkhe wrote:
> @@ -1264,10 +1261,8 @@ static int mxt_remove(struct i2c_client *client)
> struct mxt_data *data = i2c_get_clientdata(client);
>
> sysfs_remove_group(&client->dev.kobj, &mxt_attr_group);
> - free_irq(data->irq, data);
> input_unregister_device(data->input_dev);
> kfree(data->object_table);
> - kfree(data);
YOu need to be _very_ careful when mixing devm_* and non-devm-managed
resources, especially when interrupt is involved. In this particular
case you are offloading freeing of the interrupt (and disabling it) to
devm, which will happen later, bu you are freeing object table
immediately. This may cause driver crash if interrupt comes while device
is being unbound.
That said, there is a large outstanding series to the driver from Nick
Dryer that I am trying to merge, you may want to coordinate this change
with him.
Thanks.
--
Dmitry
^ permalink raw reply
* Re: [PATCH 5/6] Staging/iio/adc/touchscreen/MXS: add interrupt driven touch detection
From: Marek Vasut @ 2013-09-16 15:10 UTC (permalink / raw)
To: Jürgen Beisert
Cc: linux-arm-kernel, Jonathan Cameron, linux-iio, devel,
fabio.estevam, jic23, Torokhov, linux-input@vger.kernel.org,
Otavio Salvador
In-Reply-To: <201309161634.54603.jbe@pengutronix.de>
Dear Jürgen Beisert,
> Hi Marek,
>
> On Monday 16 September 2013 16:23:48 Marek Vasut wrote:
> > > On Sunday 15 September 2013 12:56:25 Jonathan Cameron wrote:
> > > > On 09/11/13 09:18, Juergen Beisert wrote:
> > > > > For battery driven systems it is a very bad idea to collect the
> > > > > touchscreen data within a kernel busy loop.
> > > > >
> > > > > This change uses the features of the hardware to delay and
> > > > > accumulate samples in hardware to avoid a high interrupt and CPU
> > > > > load.
> > > > >
> > > > > Note: this is only tested on an i.MX23 SoC yet.
> > > > >
> > > > > Signed-off-by: Juergen Beisert <jbe@pengutronix.de>
> > > > > CC: linux-arm-kernel@lists.infradead.org
> > > > > CC: devel@driverdev.osuosl.org
> > > > > CC: Marek Vasut <marex@denx.de>
> > > > > CC: Fabio Estevam <fabio.estevam@freescale.com>
> > > > > CC: Jonathan Cameron <jic23@cam.ac.uk>
> > > >
> > > > While this driver is placed in IIO within staging at the moment,
> > > > these changes are definitely input related. Hence I have cc'd
> > > > Dmitry and the input list.
> > > >
> > > > I am personaly a little uncomfortable that we have such a complex bit
> > > > of input code sat within an IIO driver but such is life.
> > >
> > > Maybe an MFD for this ADC unit would be a better way to go? Currently I
> > > have a different problem with this driver, because the ADC unit
> > > monitors the battery as well. And the charging driver from the power
> > > subsystem needs these values to charge the battery in a correct
> > > manner.
> >
> > Are you planning to post the power block patches too ?
>
> When they will work: yes. Currently it crashes all the time. The PMIC is a
> beast...
Full ACK
Best regards,
Marek Vasut
--
To unsubscribe from this list: send the line "unsubscribe linux-input" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
^ permalink raw reply
* Re: [PATCH 1/1] Drivers: input: serio: New driver to support Hyper-V synthetic keyboard
From: Dmitry Torokhov @ 2013-09-16 15:20 UTC (permalink / raw)
To: K. Y. Srinivasan
Cc: olaf, gregkh, jasowang, linux-kernel, vojtech, linux-input, apw,
devel
In-Reply-To: <1379309334-25042-1-git-send-email-kys@microsoft.com>
Hi K. Y.
On Sun, Sep 15, 2013 at 10:28:54PM -0700, K. Y. Srinivasan wrote:
> Add a new driver to support synthetic keyboard. On the next generation
> Hyper-V guest firmware, many legacy devices will not be emulated and this
> driver will be required.
>
> I would like to thank Vojtech Pavlik <vojtech@suse.cz> for helping me with the
> details of the AT keyboard driver.
>
In addition to what Dan said:
> +
> +struct synth_kbd_protocol_response {
> + struct synth_kbd_msg_hdr header;
> + u32 accepted:1;
> + u32 reserved:31;
> +};
Use of bitfields for on the wire structures makes me uneasy. I know that
currently you only going to run LE on LE, but still, maybe using
explicit shifts and masks would be better,
> +
> +struct synth_kbd_keystroke {
> + struct synth_kbd_msg_hdr header;
> + u16 make_code;
> + u16 reserved0;
> + u32 is_unicode:1;
> + u32 is_break:1;
> + u32 is_e0:1;
> + u32 is_e1:1;
> + u32 reserved:28;
> +};
Same here.
> +
> +
> +#define HK_MAXIMUM_MESSAGE_SIZE 256
> +
> +#define KBD_VSC_SEND_RING_BUFFER_SIZE (10 * PAGE_SIZE)
> +#define KBD_VSC_RECV_RING_BUFFER_SIZE (10 * PAGE_SIZE)
> +
> +#define XTKBD_EMUL0 0xe0
> +#define XTKBD_EMUL1 0xe1
> +#define XTKBD_RELEASE 0x80
> +
> +
> +/*
> + * Represents a keyboard device
> + */
> +struct hv_kbd_dev {
> + unsigned char keycode[256];
I do not see anything using keycode table? This is wrong level for it
regardless.
> + struct hv_device *device;
> + struct synth_kbd_protocol_request protocol_req;
> + struct synth_kbd_protocol_response protocol_resp;
> + /* Synchronize the request/response if needed */
> + struct completion wait_event;
> + struct serio *hv_serio;
> +};
> +
> +
> +static struct hv_kbd_dev *hv_kbd_alloc_device(struct hv_device *device)
> +{
> + struct hv_kbd_dev *kbd_dev;
> + struct serio *hv_serio;
You are aligning with tabs half of declarations, leaving the others. Can
we not align at all?
> +
> + kbd_dev = kzalloc(sizeof(struct hv_kbd_dev), GFP_KERNEL);
> +
> + if (!kbd_dev)
> + return NULL;
> +
> + hv_serio = kzalloc(sizeof(struct serio), GFP_KERNEL);
> +
> + if (hv_serio == NULL) {
> + kfree(kbd_dev);
> + return NULL;
> + }
> +
> + hv_serio->id.type = SERIO_8042_XL;
> + strlcpy(hv_serio->name, dev_name(&device->device),
> + sizeof(hv_serio->name));
> + strlcpy(hv_serio->phys, dev_name(&device->device),
> + sizeof(hv_serio->phys));
> + hv_serio->dev.parent = &device->device;
Do you also want to set up serio's parent device to point to hv device?
> +
> +
> + kbd_dev->device = device;
> + kbd_dev->hv_serio = hv_serio;
> + hv_set_drvdata(device, kbd_dev);
> + init_completion(&kbd_dev->wait_event);
> +
> + return kbd_dev;
> +}
> +
> +static void hv_kbd_free_device(struct hv_kbd_dev *device)
> +{
> + serio_unregister_port(device->hv_serio);
> + kfree(device->hv_serio);
Serio ports are refcounted, do not free them explicitly after they have
been registered.
> + hv_set_drvdata(device->device, NULL);
> + kfree(device);
> +}
Thanks.
--
Dmitry
^ permalink raw reply
* Re: [PATCH 5/6] Staging/iio/adc/touchscreen/MXS: add interrupt driven touch detection
From: Dmitry Torokhov @ 2013-09-16 15:28 UTC (permalink / raw)
To: Jonathan Cameron
Cc: Juergen Beisert, linux-iio-u79uwXL29TY76Z2rM5mHXA,
linux-arm-kernel-IAPFreCvJWM7uuMidbF8XUB+6BGkLq7r,
devel-gWbeCf7V1WCQmaza687I9mD2FQJk+8+b, marex-ynQEQJNshbs,
fabio.estevam-KZfg59tc24xl57MIdRCFDg,
jic23-KWPb1pKIrIJaa/9Udqfwiw,
linux-input-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
In-Reply-To: <52359259.8010202-DgEjT+Ai2ygdnm+yROfE0A@public.gmane.org>
On Sun, Sep 15, 2013 at 11:56:25AM +0100, Jonathan Cameron wrote:
> On 09/11/13 09:18, Juergen Beisert wrote:
> > For battery driven systems it is a very bad idea to collect the touchscreen
> > data within a kernel busy loop.
> >
> > This change uses the features of the hardware to delay and accumulate samples in
> > hardware to avoid a high interrupt and CPU load.
> >
> > Note: this is only tested on an i.MX23 SoC yet.
> >
> > Signed-off-by: Juergen Beisert <jbe-bIcnvbaLZ9MEGnE8C9+IrQ@public.gmane.org>
> > CC: linux-arm-kernel-IAPFreCvJWM7uuMidbF8XUB+6BGkLq7r@public.gmane.org
> > CC: devel-gWbeCf7V1WCQmaza687I9mD2FQJk+8+b@public.gmane.org
> > CC: Marek Vasut <marex-ynQEQJNshbs@public.gmane.org>
> > CC: Fabio Estevam <fabio.estevam-KZfg59tc24xl57MIdRCFDg@public.gmane.org>
> > CC: Jonathan Cameron <jic23-KWPb1pKIrIJaa/9Udqfwiw@public.gmane.org>
> While this driver is placed in IIO within staging at the moment, these changes are definitely
> input related. Hence I have cc'd Dmitry and the input list.
>
> I am personaly a little uncomfortable that we have such a complex bit of input code sat
> within an IIO driver but such is life.
>
...
> >
> > static int mxs_lradc_ts_register(struct mxs_lradc *lradc)
> > @@ -641,6 +1056,7 @@ static void mxs_lradc_ts_unregister(struct mxs_lradc *lradc)
> >
> > cancel_work_sync(&lradc->ts_work);
> >
> > + mxs_lradc_disable_ts(lradc);
> > input_unregister_device(lradc->ts_input);
> > }
This looks iffy... Normally you disable the device so that it does not
generate more interrupts, and then cancel outstanding work(s), otherwise
newly generated interrupts may cause more work to be scheduled. Or I
missed some of the context and this is not a concern here?
Thanks.
--
Dmitry
^ permalink raw reply
* Re: [PATCH 5/6] Staging/iio/adc/touchscreen/MXS: add interrupt driven touch detection
From: Jonathan Cameron @ 2013-09-16 15:30 UTC (permalink / raw)
To: Jürgen Beisert,
linux-arm-kernel-IAPFreCvJWM7uuMidbF8XUB+6BGkLq7r
Cc: linux-iio-u79uwXL29TY76Z2rM5mHXA,
devel-gWbeCf7V1WCQmaza687I9mD2FQJk+8+b, marex-ynQEQJNshbs,
fabio.estevam-KZfg59tc24xl57MIdRCFDg,
jic23-KWPb1pKIrIJaa/9Udqfwiw, Torokhov,
linux-input-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
In-Reply-To: <201309161010.22151.jbe-bIcnvbaLZ9MEGnE8C9+IrQ@public.gmane.org>
"Jürgen Beisert" <jbe-bIcnvbaLZ9MEGnE8C9+IrQ@public.gmane.org> wrote:
>Hi Jonathan,
>
>On Sunday 15 September 2013 12:56:25 Jonathan Cameron wrote:
>> On 09/11/13 09:18, Juergen Beisert wrote:
>> > For battery driven systems it is a very bad idea to collect the
>> > touchscreen data within a kernel busy loop.
>> >
>> > This change uses the features of the hardware to delay and
>accumulate
>> > samples in hardware to avoid a high interrupt and CPU load.
>> >
>> > Note: this is only tested on an i.MX23 SoC yet.
>> >
>> > Signed-off-by: Juergen Beisert <jbe-bIcnvbaLZ9MEGnE8C9+IrQ@public.gmane.org>
>> > CC: linux-arm-kernel-IAPFreCvJWM7uuMidbF8XUB+6BGkLq7r@public.gmane.org
>> > CC: devel-gWbeCf7V1WCQmaza687I9mD2FQJk+8+b@public.gmane.org
>> > CC: Marek Vasut <marex-ynQEQJNshbs@public.gmane.org>
>> > CC: Fabio Estevam <fabio.estevam-KZfg59tc24xl57MIdRCFDg@public.gmane.org>
>> > CC: Jonathan Cameron <jic23-KWPb1pKIrIJaa/9Udqfwiw@public.gmane.org>
>>
>> While this driver is placed in IIO within staging at the moment,
>these
>> changes are definitely input related. Hence I have cc'd Dmitry and
>the
>> input list.
>>
>> I am personaly a little uncomfortable that we have such a complex bit
>of
>> input code sat within an IIO driver but such is life.
>
>Maybe an MFD for this ADC unit would be a better way to go?
That would be great and is definitely the preferred method.
Currently I
>have a
>different problem with this driver, because the ADC unit monitors the
>battery
>as well. And the charging driver from the power subsystem needs these
>values
>to charge the battery in a correct manner.
There is an iio client battery driver as generic_ADC_battery.c but it is currently only doing polled access. Not too hard to add buffered access though ideally we would want a generic way of combining consumers doing polled and interrupt driven accesses.
>
>> [...]
>
>Regards,
>Juergen
--
Sent from my Android phone with K-9 Mail. Please excuse my brevity.
^ permalink raw reply
* RE: [PATCH 1/1] Drivers: input: serio: New driver to support Hyper-V synthetic keyboard
From: KY Srinivasan @ 2013-09-16 15:52 UTC (permalink / raw)
To: Dmitry Torokhov
Cc: gregkh@linuxfoundation.org, linux-kernel@vger.kernel.org,
devel@linuxdriverproject.org, linux-input@vger.kernel.org,
vojtech@suse.cz, olaf@aepfle.de, apw@canonical.com,
jasowang@redhat.com
In-Reply-To: <20130916152019.GB12105@core.coreip.homeip.net>
> -----Original Message-----
> From: Dmitry Torokhov [mailto:dmitry.torokhov@gmail.com]
> Sent: Monday, September 16, 2013 8:20 AM
> To: KY Srinivasan
> Cc: gregkh@linuxfoundation.org; linux-kernel@vger.kernel.org;
> devel@linuxdriverproject.org; linux-input@vger.kernel.org; vojtech@suse.cz;
> olaf@aepfle.de; apw@canonical.com; jasowang@redhat.com
> Subject: Re: [PATCH 1/1] Drivers: input: serio: New driver to support Hyper-V
> synthetic keyboard
>
> Hi K. Y.
>
> On Sun, Sep 15, 2013 at 10:28:54PM -0700, K. Y. Srinivasan wrote:
> > Add a new driver to support synthetic keyboard. On the next generation
> > Hyper-V guest firmware, many legacy devices will not be emulated and this
> > driver will be required.
> >
> > I would like to thank Vojtech Pavlik <vojtech@suse.cz> for helping me with the
> > details of the AT keyboard driver.
> >
>
> In addition to what Dan said:
>
> > +
> > +struct synth_kbd_protocol_response {
> > + struct synth_kbd_msg_hdr header;
> > + u32 accepted:1;
> > + u32 reserved:31;
> > +};
>
> Use of bitfields for on the wire structures makes me uneasy. I know that
> currently you only going to run LE on LE, but still, maybe using
> explicit shifts and masks would be better,
This definition of the data structure is defined by the host. I will see what I
can do here.
>
> > +
> > +struct synth_kbd_keystroke {
> > + struct synth_kbd_msg_hdr header;
> > + u16 make_code;
> > + u16 reserved0;
> > + u32 is_unicode:1;
> > + u32 is_break:1;
> > + u32 is_e0:1;
> > + u32 is_e1:1;
> > + u32 reserved:28;
> > +};
>
> Same here.
>
> > +
> > +
> > +#define HK_MAXIMUM_MESSAGE_SIZE 256
> > +
> > +#define KBD_VSC_SEND_RING_BUFFER_SIZE (10 * PAGE_SIZE)
> > +#define KBD_VSC_RECV_RING_BUFFER_SIZE (10 * PAGE_SIZE)
> > +
> > +#define XTKBD_EMUL0 0xe0
> > +#define XTKBD_EMUL1 0xe1
> > +#define XTKBD_RELEASE 0x80
> > +
> > +
> > +/*
> > + * Represents a keyboard device
> > + */
> > +struct hv_kbd_dev {
> > + unsigned char keycode[256];
>
> I do not see anything using keycode table? This is wrong level for it
> regardless.
>
> > + struct hv_device *device;
> > + struct synth_kbd_protocol_request protocol_req;
> > + struct synth_kbd_protocol_response protocol_resp;
> > + /* Synchronize the request/response if needed */
> > + struct completion wait_event;
> > + struct serio *hv_serio;
> > +};
> > +
> > +
> > +static struct hv_kbd_dev *hv_kbd_alloc_device(struct hv_device *device)
> > +{
> > + struct hv_kbd_dev *kbd_dev;
> > + struct serio *hv_serio;
>
> You are aligning with tabs half of declarations, leaving the others. Can
> we not align at all?
>
> > +
> > + kbd_dev = kzalloc(sizeof(struct hv_kbd_dev), GFP_KERNEL);
> > +
> > + if (!kbd_dev)
> > + return NULL;
> > +
> > + hv_serio = kzalloc(sizeof(struct serio), GFP_KERNEL);
> > +
> > + if (hv_serio == NULL) {
> > + kfree(kbd_dev);
> > + return NULL;
> > + }
> > +
> > + hv_serio->id.type = SERIO_8042_XL;
> > + strlcpy(hv_serio->name, dev_name(&device->device),
> > + sizeof(hv_serio->name));
> > + strlcpy(hv_serio->phys, dev_name(&device->device),
> > + sizeof(hv_serio->phys));
> > + hv_serio->dev.parent = &device->device;
>
> Do you also want to set up serio's parent device to point to hv device?
Is there an issue here; I could setup the parent as the vmbus device.
>
> > +
> > +
> > + kbd_dev->device = device;
> > + kbd_dev->hv_serio = hv_serio;
> > + hv_set_drvdata(device, kbd_dev);
> > + init_completion(&kbd_dev->wait_event);
> > +
> > + return kbd_dev;
> > +}
> > +
> > +static void hv_kbd_free_device(struct hv_kbd_dev *device)
> > +{
> > + serio_unregister_port(device->hv_serio);
> > + kfree(device->hv_serio);
>
> Serio ports are refcounted, do not free them explicitly after they have
> been registered.
Thank you. I will fix this.
>
> > + hv_set_drvdata(device->device, NULL);
> > + kfree(device);
> > +}
>
>
> Thanks.
Thank you!.
Regards,
K. Y
^ permalink raw reply
* Re: [PATCH] input: pxa27x_keypad: fix NULL pointer dereference
From: Marek Vasut @ 2013-09-16 16:49 UTC (permalink / raw)
To: Mike Dunn; +Cc: linux-input, Dmitry Torokhov, Chao Xie, Robert Jarzmik
In-Reply-To: <1379349842-13089-1-git-send-email-mikedunn@newsguy.com>
Dear Mike Dunn,
> A NULL pointer dereference exception occurs in the driver probe function
> when device tree is used. The pdata pointer will be NULL in this case,
> but the code dereferences it in all cases. When device tree is used, a
> platform data structure is allocated and initialized, and in all cases
> this pointer is copied to the driver's private data, so the variable being
> tested should be accessed through the driver's private data structure.
>
> Signed-off-by: Mike Dunn <mikedunn@newsguy.com>
> ---
> drivers/input/keyboard/pxa27x_keypad.c | 6 ++++--
> 1 file changed, 4 insertions(+), 2 deletions(-)
>
> diff --git a/drivers/input/keyboard/pxa27x_keypad.c
> b/drivers/input/keyboard/pxa27x_keypad.c index 134c3b4..3b2a614 100644
> --- a/drivers/input/keyboard/pxa27x_keypad.c
> +++ b/drivers/input/keyboard/pxa27x_keypad.c
> @@ -795,8 +795,10 @@ static int pxa27x_keypad_probe(struct platform_device
> *pdev) goto failed_put_clk;
> }
>
> - if ((pdata->enable_rotary0 && keypad->rotary_rel_code[0] != -1) ||
> - (pdata->enable_rotary1 && keypad->rotary_rel_code[1] != -1)) {
> + if ((keypad->pdata->enable_rotary0 &&
> + keypad->rotary_rel_code[0] != -1) ||
> + (keypad->pdata->enable_rotary1 &&
> + keypad->rotary_rel_code[1] != -1)) {
> input_dev->evbit[0] |= BIT_MASK(EV_REL);
> }
Nice find. Acked-by: Marek Vasut <marex@denx.de>
Best regards,
Marek Vasut
^ permalink raw reply
* RE: [PATCH 1/1] Drivers: input: serio: New driver to support Hyper-V synthetic keyboard
From: KY Srinivasan @ 2013-09-16 16:56 UTC (permalink / raw)
To: Dan Carpenter
Cc: olaf@aepfle.de, gregkh@linuxfoundation.org, jasowang@redhat.com,
dmitry.torokhov@gmail.com, linux-kernel@vger.kernel.org,
vojtech@suse.cz, linux-input@vger.kernel.org, apw@canonical.com,
devel@linuxdriverproject.org
In-Reply-To: <20130916150548.GO25896@mwanda>
> -----Original Message-----
> From: Dan Carpenter [mailto:dan.carpenter@oracle.com]
> Sent: Monday, September 16, 2013 8:06 AM
> To: KY Srinivasan
> Cc: olaf@aepfle.de; gregkh@linuxfoundation.org; jasowang@redhat.com;
> dmitry.torokhov@gmail.com; linux-kernel@vger.kernel.org; vojtech@suse.cz;
> linux-input@vger.kernel.org; apw@canonical.com; devel@linuxdriverproject.org
> Subject: Re: [PATCH 1/1] Drivers: input: serio: New driver to support Hyper-V
> synthetic keyboard
>
> On Mon, Sep 16, 2013 at 02:46:24PM +0000, KY Srinivasan wrote:
> > > > + case VM_PKT_DATA_INBAND:
> > > > + hv_kbd_on_receive(device, desc);
> > >
> > > This is the error handling I mentioned at the top. hv_kbd_on_receive()
> > > doesn't take into consideration the amount of data we recieved, it
> > > trusts the offset we recieved from the user. There is an out of bounds
> > > read.
> >
> > What user are you referring to. The message is sent by the host - the user
> keystroke
> > is normalized into a fixed size packet by the host and sent to the guest. We will
> parse this
> > packet, based on the host specified layout here.
> >
>
> The user means the hypervisor, yes.
>
> I don't want the hypervisor accessing outside of the buffer. It is
> robustness issue. Just check the offset against "bytes_recvd". It's
> not complicated.
At the outset, let me apologize for not understanding your concern.
You say: " I don't want the hypervisor accessing outside of the buffer"
Where did you see the hypervisor accessing anything outside the buffer?
The buffer is allocated by this driver and a packet from vmbus is read into this
buffer - this is the call to vmbus_recvpacket(). If the specified buffer is smaller
than the packet that needs to be read, then nothing will be read. Once the read
completes, we can be sure we have read a valid packet and can proceed to parse it in
this driver.
I don't want to be argumentative, I am just not seeing the issue.
Regards,
K. Y
^ permalink raw reply
* Re: [PATCH] input: pxa27x_keypad: fix NULL pointer dereference
From: Dmitry Torokhov @ 2013-09-16 17:06 UTC (permalink / raw)
To: Marek Vasut; +Cc: Mike Dunn, linux-input, Chao Xie, Robert Jarzmik
In-Reply-To: <201309161849.53556.marex@denx.de>
On Mon, Sep 16, 2013 at 06:49:53PM +0200, Marek Vasut wrote:
> Dear Mike Dunn,
>
> > A NULL pointer dereference exception occurs in the driver probe function
> > when device tree is used. The pdata pointer will be NULL in this case,
> > but the code dereferences it in all cases. When device tree is used, a
> > platform data structure is allocated and initialized, and in all cases
> > this pointer is copied to the driver's private data, so the variable being
> > tested should be accessed through the driver's private data structure.
> >
> > Signed-off-by: Mike Dunn <mikedunn@newsguy.com>
> > ---
> > drivers/input/keyboard/pxa27x_keypad.c | 6 ++++--
> > 1 file changed, 4 insertions(+), 2 deletions(-)
> >
> > diff --git a/drivers/input/keyboard/pxa27x_keypad.c
> > b/drivers/input/keyboard/pxa27x_keypad.c index 134c3b4..3b2a614 100644
> > --- a/drivers/input/keyboard/pxa27x_keypad.c
> > +++ b/drivers/input/keyboard/pxa27x_keypad.c
> > @@ -795,8 +795,10 @@ static int pxa27x_keypad_probe(struct platform_device
> > *pdev) goto failed_put_clk;
> > }
> >
> > - if ((pdata->enable_rotary0 && keypad->rotary_rel_code[0] != -1) ||
> > - (pdata->enable_rotary1 && keypad->rotary_rel_code[1] != -1)) {
> > + if ((keypad->pdata->enable_rotary0 &&
> > + keypad->rotary_rel_code[0] != -1) ||
> > + (keypad->pdata->enable_rotary1 &&
> > + keypad->rotary_rel_code[1] != -1)) {
> > input_dev->evbit[0] |= BIT_MASK(EV_REL);
> > }
>
> Nice find. Acked-by: Marek Vasut <marex@denx.de>
Excellent booby trap. I would prefer if we explicitly did
pdata = keypad->pdata;
after calling the parse DT fucntion with a nice comment, because we
somebody might want to rearrange the code and accidentially revert the
checks to the original state.
Thanks.
--
Dmitry
^ permalink raw reply
* [PATCH] input: pxa27x_keypad: fix NULL pointer dereference
From: Mike Dunn @ 2013-09-16 16:44 UTC (permalink / raw)
To: linux-input
Cc: Mike Dunn, Dmitry Torokhov, Chao Xie, Robert Jarzmik, Marek Vasut
A NULL pointer dereference exception occurs in the driver probe function when
device tree is used. The pdata pointer will be NULL in this case, but the code
dereferences it in all cases. When device tree is used, a platform data
structure is allocated and initialized, and in all cases this pointer is copied
to the driver's private data, so the variable being tested should be accessed
through the driver's private data structure.
Signed-off-by: Mike Dunn <mikedunn@newsguy.com>
---
drivers/input/keyboard/pxa27x_keypad.c | 6 ++++--
1 file changed, 4 insertions(+), 2 deletions(-)
diff --git a/drivers/input/keyboard/pxa27x_keypad.c b/drivers/input/keyboard/pxa27x_keypad.c
index 134c3b4..3b2a614 100644
--- a/drivers/input/keyboard/pxa27x_keypad.c
+++ b/drivers/input/keyboard/pxa27x_keypad.c
@@ -795,8 +795,10 @@ static int pxa27x_keypad_probe(struct platform_device *pdev)
goto failed_put_clk;
}
- if ((pdata->enable_rotary0 && keypad->rotary_rel_code[0] != -1) ||
- (pdata->enable_rotary1 && keypad->rotary_rel_code[1] != -1)) {
+ if ((keypad->pdata->enable_rotary0 &&
+ keypad->rotary_rel_code[0] != -1) ||
+ (keypad->pdata->enable_rotary1 &&
+ keypad->rotary_rel_code[1] != -1)) {
input_dev->evbit[0] |= BIT_MASK(EV_REL);
}
--
1.8.1.5
^ permalink raw reply related
* Re: [PATCH 1/1] Drivers: input: serio: New driver to support Hyper-V synthetic keyboard
From: Dmitry Torokhov @ 2013-09-16 17:09 UTC (permalink / raw)
To: KY Srinivasan
Cc: Dan Carpenter, olaf@aepfle.de, gregkh@linuxfoundation.org,
jasowang@redhat.com, linux-kernel@vger.kernel.org,
vojtech@suse.cz, linux-input@vger.kernel.org, apw@canonical.com,
devel@linuxdriverproject.org
In-Reply-To: <baabf219b12b40de8e17b029e621e079@SN2PR03MB061.namprd03.prod.outlook.com>
On Mon, Sep 16, 2013 at 04:56:03PM +0000, KY Srinivasan wrote:
>
>
> > -----Original Message-----
> > From: Dan Carpenter [mailto:dan.carpenter@oracle.com]
> > Sent: Monday, September 16, 2013 8:06 AM
> > To: KY Srinivasan
> > Cc: olaf@aepfle.de; gregkh@linuxfoundation.org; jasowang@redhat.com;
> > dmitry.torokhov@gmail.com; linux-kernel@vger.kernel.org; vojtech@suse.cz;
> > linux-input@vger.kernel.org; apw@canonical.com; devel@linuxdriverproject.org
> > Subject: Re: [PATCH 1/1] Drivers: input: serio: New driver to support Hyper-V
> > synthetic keyboard
> >
> > On Mon, Sep 16, 2013 at 02:46:24PM +0000, KY Srinivasan wrote:
> > > > > + case VM_PKT_DATA_INBAND:
> > > > > + hv_kbd_on_receive(device, desc);
> > > >
> > > > This is the error handling I mentioned at the top. hv_kbd_on_receive()
> > > > doesn't take into consideration the amount of data we recieved, it
> > > > trusts the offset we recieved from the user. There is an out of bounds
> > > > read.
> > >
> > > What user are you referring to. The message is sent by the host - the user
> > keystroke
> > > is normalized into a fixed size packet by the host and sent to the guest. We will
> > parse this
> > > packet, based on the host specified layout here.
> > >
> >
> > The user means the hypervisor, yes.
> >
> > I don't want the hypervisor accessing outside of the buffer. It is
> > robustness issue. Just check the offset against "bytes_recvd". It's
> > not complicated.
>
> At the outset, let me apologize for not understanding your concern.
> You say: " I don't want the hypervisor accessing outside of the buffer"
> Where did you see the hypervisor accessing anything outside the buffer?
> The buffer is allocated by this driver and a packet from vmbus is read into this
> buffer - this is the call to vmbus_recvpacket(). If the specified buffer is smaller
> than the packet that needs to be read, then nothing will be read. Once the read
> completes, we can be sure we have read a valid packet and can proceed to parse it in
> this driver.
The concern is that number of bytes received and contents of a packet
are not in sync. Imagine if we were told that 16 butes was received but
in the packet offset is 78. Then we'll try reading well past the buffer
boundary that we allocated for the packets.
Thanks.
--
Dmitry
^ permalink raw reply
* Re: [PATCH 1/1] Drivers: input: serio: New driver to support Hyper-V synthetic keyboard
From: Dmitry Torokhov @ 2013-09-16 17:13 UTC (permalink / raw)
To: KY Srinivasan
Cc: olaf@aepfle.de, gregkh@linuxfoundation.org, jasowang@redhat.com,
linux-kernel@vger.kernel.org, vojtech@suse.cz,
linux-input@vger.kernel.org, apw@canonical.com,
devel@linuxdriverproject.org
In-Reply-To: <076938490f5141ef957947dc10b82c65@SN2PR03MB061.namprd03.prod.outlook.com>
On Mon, Sep 16, 2013 at 03:52:18PM +0000, KY Srinivasan wrote:
>
>
> > -----Original Message-----
> > From: Dmitry Torokhov [mailto:dmitry.torokhov@gmail.com]
> > Sent: Monday, September 16, 2013 8:20 AM
> > To: KY Srinivasan
> > Cc: gregkh@linuxfoundation.org; linux-kernel@vger.kernel.org;
> > devel@linuxdriverproject.org; linux-input@vger.kernel.org; vojtech@suse.cz;
> > olaf@aepfle.de; apw@canonical.com; jasowang@redhat.com
> > Subject: Re: [PATCH 1/1] Drivers: input: serio: New driver to support Hyper-V
> > synthetic keyboard
> >
> > Hi K. Y.
> >
> > On Sun, Sep 15, 2013 at 10:28:54PM -0700, K. Y. Srinivasan wrote:
> > > Add a new driver to support synthetic keyboard. On the next generation
> > > Hyper-V guest firmware, many legacy devices will not be emulated and this
> > > driver will be required.
> > >
> > > I would like to thank Vojtech Pavlik <vojtech@suse.cz> for helping me with the
> > > details of the AT keyboard driver.
> > >
> >
> > In addition to what Dan said:
> >
> > > +
> > > +struct synth_kbd_protocol_response {
> > > + struct synth_kbd_msg_hdr header;
> > > + u32 accepted:1;
> > > + u32 reserved:31;
> > > +};
> >
> > Use of bitfields for on the wire structures makes me uneasy. I know that
> > currently you only going to run LE on LE, but still, maybe using
> > explicit shifts and masks would be better,
>
> This definition of the data structure is defined by the host. I will see what I
> can do here.
You do not really need to change protocol, you just sat that accepted is
the bit 0 of the word and define endianness (LE in your case). Then you
do:
struct synth_kbd_protocol_response {
struct synth_kbd_msg_hdr header;
__le32 status;
}
#define KBD_PROTOCOL_ACCEPTED BIT(0)
...
status = _le32_to_cpu(response->status);
accepted = status & KBD_PROTOCOL_ACCEPTED;
Thanks.
--
Dmitry
^ permalink raw reply
* [v3.11][Regression] HID: hyperv: convert alloc+memcpy to memdup
From: Joseph Salisbury @ 2013-09-16 17:42 UTC (permalink / raw)
To: thomas, Jiri Kosina
Cc: list, Haiyang Zhang, LKML, open, HID CORE LAYER, devel
Hi Thomas,
A bug was opened against the Ubuntu kernel[0]. After a kernel bisect,
it was found that reverting the following commit resolved
this bug:
commit a4a23f6d68ad2c86ee8df6a6f89c9d315c0a761c
Author: Thomas Meyer <thomas@m3y3r.de>
Date: Sat Jun 1 11:40:31 2013 +0200
HID: hyperv: convert alloc+memcpy to memdup
The regression was introduced as of v3.11-rc1. The bug causes a system
to lockup when connecting a bluetooth trackpad and has been reported on
multiple machines.
I wanted to ping you since you are the author of this patch. I was
thinking of requesting a revert for v3.12 and v3.11, but I wanted to get
your feedback first.
Reverting the patch changes the driver back to useing kzalloc() and
memcpy() instead of kmemdup. Doing so has uncovered another bug, which
causes an oops on memcpy()[1]. We are in the process of bisecting that
one now and will provide the results.
Thanks,
Joe
[0] pad.lv/1218004
[1]
https://bugs.launchpad.net/ubuntu/+source/linux/+bug/1218004/+attachment/3818671/+files/kern.log
^ permalink raw reply
* RE: [PATCH 1/1] Drivers: input: serio: New driver to support Hyper-V synthetic keyboard
From: KY Srinivasan @ 2013-09-16 18:29 UTC (permalink / raw)
To: Dmitry Torokhov
Cc: Dan Carpenter, olaf@aepfle.de, gregkh@linuxfoundation.org,
jasowang@redhat.com, linux-kernel@vger.kernel.org,
vojtech@suse.cz, linux-input@vger.kernel.org, apw@canonical.com,
devel@linuxdriverproject.org
In-Reply-To: <20130916170951.GB20734@core.coreip.homeip.net>
> -----Original Message-----
> From: Dmitry Torokhov [mailto:dmitry.torokhov@gmail.com]
> Sent: Monday, September 16, 2013 10:10 AM
> To: KY Srinivasan
> Cc: Dan Carpenter; olaf@aepfle.de; gregkh@linuxfoundation.org;
> jasowang@redhat.com; linux-kernel@vger.kernel.org; vojtech@suse.cz; linux-
> input@vger.kernel.org; apw@canonical.com; devel@linuxdriverproject.org
> Subject: Re: [PATCH 1/1] Drivers: input: serio: New driver to support Hyper-V
> synthetic keyboard
>
> On Mon, Sep 16, 2013 at 04:56:03PM +0000, KY Srinivasan wrote:
> >
> >
> > > -----Original Message-----
> > > From: Dan Carpenter [mailto:dan.carpenter@oracle.com]
> > > Sent: Monday, September 16, 2013 8:06 AM
> > > To: KY Srinivasan
> > > Cc: olaf@aepfle.de; gregkh@linuxfoundation.org; jasowang@redhat.com;
> > > dmitry.torokhov@gmail.com; linux-kernel@vger.kernel.org;
> vojtech@suse.cz;
> > > linux-input@vger.kernel.org; apw@canonical.com;
> devel@linuxdriverproject.org
> > > Subject: Re: [PATCH 1/1] Drivers: input: serio: New driver to support Hyper-V
> > > synthetic keyboard
> > >
> > > On Mon, Sep 16, 2013 at 02:46:24PM +0000, KY Srinivasan wrote:
> > > > > > + case VM_PKT_DATA_INBAND:
> > > > > > + hv_kbd_on_receive(device, desc);
> > > > >
> > > > > This is the error handling I mentioned at the top. hv_kbd_on_receive()
> > > > > doesn't take into consideration the amount of data we recieved, it
> > > > > trusts the offset we recieved from the user. There is an out of bounds
> > > > > read.
> > > >
> > > > What user are you referring to. The message is sent by the host - the user
> > > keystroke
> > > > is normalized into a fixed size packet by the host and sent to the guest. We
> will
> > > parse this
> > > > packet, based on the host specified layout here.
> > > >
> > >
> > > The user means the hypervisor, yes.
> > >
> > > I don't want the hypervisor accessing outside of the buffer. It is
> > > robustness issue. Just check the offset against "bytes_recvd". It's
> > > not complicated.
> >
> > At the outset, let me apologize for not understanding your concern.
> > You say: " I don't want the hypervisor accessing outside of the buffer"
> > Where did you see the hypervisor accessing anything outside the buffer?
> > The buffer is allocated by this driver and a packet from vmbus is read into this
> > buffer - this is the call to vmbus_recvpacket(). If the specified buffer is smaller
> > than the packet that needs to be read, then nothing will be read. Once the read
> > completes, we can be sure we have read a valid packet and can proceed to
> parse it in
> > this driver.
>
> The concern is that number of bytes received and contents of a packet
> are not in sync. Imagine if we were told that 16 butes was received but
> in the packet offset is 78. Then we'll try reading well past the buffer
> boundary that we allocated for the packets.
I am not sure how this would be the case. Following are the semantics of the function
vmbus_recvpacket_raw():
If the packet to be read is larger than the buffer specified; nothing will be read and
appropriate error is returned. If a packet is read, the complete packet is read and
so we can safely peek into this packet based on the information in the header.
Regards,
K. Y
^ permalink raw reply
* Re: [PATCH 1/1] Drivers: input: serio: New driver to support Hyper-V synthetic keyboard
From: Dan Carpenter @ 2013-09-16 18:33 UTC (permalink / raw)
To: KY Srinivasan
Cc: Dmitry Torokhov, olaf@aepfle.de, gregkh@linuxfoundation.org,
jasowang@redhat.com, linux-kernel@vger.kernel.org,
vojtech@suse.cz, linux-input@vger.kernel.org, apw@canonical.com,
devel@linuxdriverproject.org
In-Reply-To: <3e45f054c6e243eabf2896ab1dcf38d4@SN2PR03MB061.namprd03.prod.outlook.com>
Just roll something like the following into your patch.
regards,
dan carpenter
diff --git a/drivers/input/serio/hyperv-keyboard.c b/drivers/input/serio/hyperv-keyboard.c
index 0d4625f..262721b 100644
--- a/drivers/input/serio/hyperv-keyboard.c
+++ b/drivers/input/serio/hyperv-keyboard.c
@@ -151,15 +151,18 @@ static void hv_kbd_free_device(struct hv_kbd_dev *device)
}
static void hv_kbd_on_receive(struct hv_device *device,
- struct vmpacket_descriptor *packet)
+ struct vmpacket_descriptor *packet, size_t size)
{
struct synth_kbd_msg *msg;
struct hv_kbd_dev *kbd_dev = hv_get_drvdata(device);
struct synth_kbd_keystroke *ks_msg;
+ int offset;
u16 scan_code;
- msg = (struct synth_kbd_msg *)((unsigned long)packet +
- (packet->offset8 << 3));
+ offset = packet->offset8 << 3;
+ if (offset + sizeof(struct synth_kbd_protocol_response) > size)
+ return;
+ msg = (void *)packet + offset;
switch (msg->header.type) {
case SYNTH_KBD_PROTOCOL_RESPONSE:
@@ -220,7 +223,7 @@ static void hv_kbd_on_channel_callback(void *context)
break;
case VM_PKT_DATA_INBAND:
- hv_kbd_on_receive(device, desc);
+ hv_kbd_on_receive(device, desc, bytes_recvd);
break;
default:
^ permalink raw reply related
* RE: [PATCH 1/1] Drivers: input: serio: New driver to support Hyper-V synthetic keyboard
From: KY Srinivasan @ 2013-09-16 18:42 UTC (permalink / raw)
To: Dan Carpenter
Cc: olaf@aepfle.de, gregkh@linuxfoundation.org, jasowang@redhat.com,
Dmitry Torokhov, linux-kernel@vger.kernel.org, vojtech@suse.cz,
linux-input@vger.kernel.org, apw@canonical.com,
devel@linuxdriverproject.org
In-Reply-To: <20130916183319.GM19256@mwanda>
> -----Original Message-----
> From: Dan Carpenter [mailto:dan.carpenter@oracle.com]
> Sent: Monday, September 16, 2013 11:33 AM
> To: KY Srinivasan
> Cc: Dmitry Torokhov; olaf@aepfle.de; gregkh@linuxfoundation.org;
> jasowang@redhat.com; linux-kernel@vger.kernel.org; vojtech@suse.cz; linux-
> input@vger.kernel.org; apw@canonical.com; devel@linuxdriverproject.org
> Subject: Re: [PATCH 1/1] Drivers: input: serio: New driver to support Hyper-V
> synthetic keyboard
>
> Just roll something like the following into your patch.
>
> regards,
> dan carpenter
>
> diff --git a/drivers/input/serio/hyperv-keyboard.c b/drivers/input/serio/hyperv-
> keyboard.c
> index 0d4625f..262721b 100644
> --- a/drivers/input/serio/hyperv-keyboard.c
> +++ b/drivers/input/serio/hyperv-keyboard.c
> @@ -151,15 +151,18 @@ static void hv_kbd_free_device(struct hv_kbd_dev
> *device)
> }
>
> static void hv_kbd_on_receive(struct hv_device *device,
> - struct vmpacket_descriptor *packet)
> + struct vmpacket_descriptor *packet, size_t size)
> {
> struct synth_kbd_msg *msg;
> struct hv_kbd_dev *kbd_dev = hv_get_drvdata(device);
> struct synth_kbd_keystroke *ks_msg;
> + int offset;
> u16 scan_code;
>
> - msg = (struct synth_kbd_msg *)((unsigned long)packet +
> - (packet->offset8 << 3));
> + offset = packet->offset8 << 3;
> + if (offset + sizeof(struct synth_kbd_protocol_response) > size)
> + return;
> + msg = (void *)packet + offset;
>
> switch (msg->header.type) {
> case SYNTH_KBD_PROTOCOL_RESPONSE:
> @@ -220,7 +223,7 @@ static void hv_kbd_on_channel_callback(void *context)
> break;
>
> case VM_PKT_DATA_INBAND:
> - hv_kbd_on_receive(device, desc);
> + hv_kbd_on_receive(device, desc, bytes_recvd);
> break;
>
> default:
Dan,
Rolling the changes you have indicated is not the issue; this can trivially be done.
My contention is that it is not needed given that the underlying function is already
doing that. Look at the function vmbus_recvpacket_raw() in drivers/hv/channel.c.
Regards,
K. Y
^ permalink raw reply
* Re: [PATCH 1/1] Drivers: input: serio: New driver to support Hyper-V synthetic keyboard
From: Dan Carpenter @ 2013-09-16 20:13 UTC (permalink / raw)
To: KY Srinivasan
Cc: olaf@aepfle.de, gregkh@linuxfoundation.org, jasowang@redhat.com,
Dmitry Torokhov, linux-kernel@vger.kernel.org, vojtech@suse.cz,
linux-input@vger.kernel.org, apw@canonical.com,
devel@linuxdriverproject.org
In-Reply-To: <117fbf407a554684b4d72ab06b033eeb@SN2PR03MB061.namprd03.prod.outlook.com>
On Mon, Sep 16, 2013 at 06:42:25PM +0000, KY Srinivasan wrote:
> Dan,
>
> Rolling the changes you have indicated is not the issue; this can trivially be done.
> My contention is that it is not needed given that the underlying function is already
> doing that. Look at the function vmbus_recvpacket_raw() in drivers/hv/channel.c.
>
I'm confused.
There is no mention of ->offset8 in vmbus_recvpacket_raw().
It's a good idea to add a check there but the lower levels don't know
about the sizeof(synth_kbd_protocol_response) so we would still need
something like my check.
regards,
dan carpenter
^ permalink raw reply
* Re: [v3.11][Regression] HID: hyperv: convert alloc+memcpy to memdup
From: Dan Carpenter @ 2013-09-16 20:38 UTC (permalink / raw)
To: Joseph Salisbury
Cc: list, Jiri Kosina, Haiyang Zhang, LKML, open, HID CORE LAYER,
devel, thomas
In-Reply-To: <5237430B.5040009@canonical.com>
On Mon, Sep 16, 2013 at 01:42:35PM -0400, Joseph Salisbury wrote:
> Reverting the patch changes the driver back to useing kzalloc() and
> memcpy() instead of kmemdup. Doing so has uncovered another bug, which
> causes an oops on memcpy()[1]. We are in the process of bisecting that
> one now and will provide the results.
The two bugs are the same it's that the code has shifted a little. Mark
the commit as buggy and continue with the git bisect.
regards,
dan carpenter
^ permalink raw reply
* Re: [v3.11][Regression] HID: hyperv: convert alloc+memcpy to memdup
From: Joseph Salisbury @ 2013-09-16 20:49 UTC (permalink / raw)
To: Dan Carpenter
Cc: list, Jiri Kosina, Haiyang Zhang, LKML, open, HID CORE LAYER,
devel, thomas
In-Reply-To: <20130916203824.GP25896@mwanda>
On 09/16/2013 04:38 PM, Dan Carpenter wrote:
> On Mon, Sep 16, 2013 at 01:42:35PM -0400, Joseph Salisbury wrote:
>> Reverting the patch changes the driver back to useing kzalloc() and
>> memcpy() instead of kmemdup. Doing so has uncovered another bug, which
>> causes an oops on memcpy()[1]. We are in the process of bisecting that
>> one now and will provide the results.
> The two bugs are the same it's that the code has shifted a little. Mark
> the commit as buggy and continue with the git bisect.
>
> regards,
> dan carpenter
Can you explain a little further? Mark commit a4a23f6 as bad? An
initial bisect already reported that was the first bad commit, so it
can't be marked bad. The oops on memcpy() happens after commit a4a23f6
is reverted. The oops on memcpy() did not happen before a4a23f6 was
committed, so I assume this new oops was introduced by a change later.
Right now I'm bisecting down the oops on memcpy() by updating the bisect
with good or bad, depending if the test kernel hit the oops. I then
revert a4a23f6, so that revert is the HEAD of the tree each time before
building the kernel again(As long as the commit spit out by bisect is
after when a4a23f6 was introduced).
Thanks,
Joe
^ permalink raw reply
* Re: [v3.11][Regression] HID: hyperv: convert alloc+memcpy to memdup
From: Dan Carpenter @ 2013-09-16 21:05 UTC (permalink / raw)
To: Joseph Salisbury
Cc: list, Jiri Kosina, Haiyang Zhang, LKML, open, HID CORE LAYER,
devel, thomas
In-Reply-To: <52376ED9.5080208@canonical.com>
On Mon, Sep 16, 2013 at 04:49:29PM -0400, Joseph Salisbury wrote:
> On 09/16/2013 04:38 PM, Dan Carpenter wrote:
> > On Mon, Sep 16, 2013 at 01:42:35PM -0400, Joseph Salisbury wrote:
> >> Reverting the patch changes the driver back to useing kzalloc() and
> >> memcpy() instead of kmemdup. Doing so has uncovered another bug, which
> >> causes an oops on memcpy()[1]. We are in the process of bisecting that
> >> one now and will provide the results.
> > The two bugs are the same it's that the code has shifted a little. Mark
> > the commit as buggy and continue with the git bisect.
> >
> > regards,
> > dan carpenter
> Can you explain a little further? Mark commit a4a23f6 as bad? An
> initial bisect already reported that was the first bad commit, so it
> can't be marked bad. The oops on memcpy() happens after commit a4a23f6
> is reverted. The oops on memcpy() did not happen before a4a23f6 was
> committed, so I assume this new oops was introduced by a change later.
>
> Right now I'm bisecting down the oops on memcpy() by updating the bisect
> with good or bad, depending if the test kernel hit the oops. I then
> revert a4a23f6, so that revert is the HEAD of the tree each time before
> building the kernel again(As long as the commit spit out by bisect is
> after when a4a23f6 was introduced).
Yep. Please continue bisecting the memcpy() oops.
kmemdup() is just a kzalloc() followed by a memcpy(). When we split it
apart by reverting the patch then we would expect the oops to move to
the memcpy() part. Somehow "desc" is a bogus pointer, but I don't
immediately see how that is possible.
regards,
dan carpenter
^ permalink raw reply
page: next (older) | prev (newer) | latest
- recent:[subjects (threaded)|topics (new)|topics (active)]
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox