* Re: [PATCHES] HID: huion: Fix "doing dma on the stack"
From: Jiri Kosina @ 2014-08-12 9:27 UTC (permalink / raw)
To: Nikolai Kondrashov; +Cc: linux-input, DIGImend-devel
In-Reply-To: <1407779132-26749-1-git-send-email-spbnick@gmail.com>
On Mon, 11 Aug 2014, Nikolai Kondrashov wrote:
> Hi everyone,
>
> These two patches work towards fixing the "doing dma on the stack" reported by
> a static analyzer (I'm not sure which one) for my recent changes to hid-huion
> driver.
>
> The first one is easier to read with whitespace changes ingored (-w).
>
> Both patches were tested by me with a Huion H610N tablet.
>
> Nick
Nikolai,
thanks, I will be reviewing the patches soon and queuing for 3.17 still
eventually.
They seem to be missing your signoff though ... is that on purpose?
Thanks,
--
Jiri Kosina
SUSE Labs
^ permalink raw reply
* RE: [PATCH] Input: serio: make HYPERV_KEYBOARD depend on SERIO_I8042=y
From: Dexuan Cui @ 2014-08-12 7:15 UTC (permalink / raw)
To: Greg KH
Cc: olaf@aepfle.de, dmitry.torokhov@gmail.com, jasowang@redhat.com,
driverdev-devel@linuxdriverproject.org,
linux-kernel@vger.kernel.org, linux-input@vger.kernel.org,
apw@canonical.com, Haiyang Zhang
In-Reply-To: <20140812060108.GA13231@kroah.com>
> -----Original Message-----
> From: linux-kernel-owner@vger.kernel.org [mailto:linux-kernel-
> owner@vger.kernel.org] On Behalf Of Greg KH
> > > What exactly needs to be done to fix this "correctly" that is going to
> > > take too much work at the moment?
> >
> > To decouple the dependency between the hyperv-keyboard and i8042
> > modules, I suppose we probably have to re-implement hyperv-keyboard by
> > using input_allocate_device(), input_register_device(), and using
> > input_report_key() to pass the key strokes to the high level.
>
> Yes, that would be the best thing to do, and shouldn't be that hard to
> create an input driver, it's pretty simple code, right?
Hi Greg,
Thanks for the confirmation!
I didn't use the APIs before.
I think I need a couple of days to code, test and debug it while I have many
things at hand at present. :-(
> > I'll have to need some time for further investigation and a new
> > implementation. Before the new code is completely ready, IMHO the
> > patch can help to avoid a bad user experience like Arch Linux working
> > as a Generation 2 hyper-v guest.
>
> You are still preventing Arch from working here, as the driver can't be
> built at all, right?
The driver can build(compile) fine.
The issue is: the latest Arch Linux release doesn't have a working (virtual)
keyboard when it runs as Generation 2 hyper-v guest -- when it runs as
a "traditional" Generation 1 hyper-v guest, everything works fine.
I hope this patch can temporarily help Arch users if they find the issue
and if they can rebuild the kernel.
> > BTW, looks most of Linux distros (like RHEL, Ubuntu, SUSE) have
> > CONFIG_SERIO_I8042=y, probably because it's the result of
> > "make defconfig". So the patch actually doesn't affect these distros.
>
> Or maybe it is beause they use older kernels and like to turn on every
> single possible option :)
If these 3 distros had =m, we could have found the issue earlier.
Thanks,
-- Dexuan
^ permalink raw reply
* Re: [PATCH] Input: serio: make HYPERV_KEYBOARD depend on SERIO_I8042=y
From: Greg KH @ 2014-08-12 6:01 UTC (permalink / raw)
To: Dexuan Cui
Cc: olaf@aepfle.de, dmitry.torokhov@gmail.com, jasowang@redhat.com,
driverdev-devel@linuxdriverproject.org,
linux-kernel@vger.kernel.org, linux-input@vger.kernel.org,
apw@canonical.com, Haiyang Zhang
In-Reply-To: <EE124450C0AAF944A40DD71E61F878C9920A1C@SINEX14MBXC417.southpacific.corp.microsoft.com>
On Tue, Aug 12, 2014 at 05:51:28AM +0000, Dexuan Cui wrote:
> > -----Original Message-----
> > From: Greg KH [mailto:gregkh@linuxfoundation.org]
> > > Decoupling the dependency between hyperv_keyboard and i8042 needs
> > > non-trivial efforts and is hence a long term goal.
> > >
> > > For now, let's make the dependency explicit so people can beware of this.
> >
> > You didn't make anyone "aware" of this, you just prevented people from
> > being able to select the module unless they build the driver into the
> > kernel, which isn't very nice.
>
> Yes, exactly.
>
> > What exactly needs to be done to fix this "correctly" that is going to
> > take too much work at the moment?
> Hi Greg,
> The current implementation of hyperv-keyboard.c borrows
> serio_interrupt() to pass the key strokes to the high level component.
> serio_interrupt() actually depends on atkbd.c and i8042.c, so this doesn't
> work for a Generation 2 hyper-v guest because no i8042 keyboard controller
> is emulated: http://technet.microsoft.com/en-us/library/dn282285.aspx
>
> To decouple the dependency between the hyperv-keyboard and i8042
> modules, I suppose we probably have to re-implement hyperv-keyboard by
> using input_allocate_device(), input_register_device(), and using
> input_report_key() to pass the key strokes to the high level.
Yes, that would be the best thing to do, and shouldn't be that hard to
create an input driver, it's pretty simple code, right?
> I'll have to need some time for further investigation and a new
> implementation. Before the new code is completely ready, IMHO the
> patch can help to avoid a bad user experience like Arch Linux working
> as a Generation 2 hyper-v guest.
You are still preventing Arch from working here, as the driver can't be
built at all, right?
> BTW, looks most of Linux distros (like RHEL, Ubuntu, SUSE) have
> CONFIG_SERIO_I8042=y, probably because it's the result of
> "make defconfig". So the patch actually doesn't affect these distros.
Or maybe it is beause they use older kernels and like to turn on every
single possible option :)
thanks,
greg k-h
^ permalink raw reply
* RE: [PATCH] Input: serio: make HYPERV_KEYBOARD depend on SERIO_I8042=y
From: Dexuan Cui @ 2014-08-12 5:51 UTC (permalink / raw)
To: Greg KH
Cc: dmitry.torokhov@gmail.com, linux-input@vger.kernel.org,
linux-kernel@vger.kernel.org,
driverdev-devel@linuxdriverproject.org, olaf@aepfle.de,
apw@canonical.com, jasowang@redhat.com, KY Srinivasan,
Haiyang Zhang
In-Reply-To: <20140812032148.GB6867@kroah.com>
> -----Original Message-----
> From: Greg KH [mailto:gregkh@linuxfoundation.org]
> > Decoupling the dependency between hyperv_keyboard and i8042 needs
> > non-trivial efforts and is hence a long term goal.
> >
> > For now, let's make the dependency explicit so people can beware of this.
>
> You didn't make anyone "aware" of this, you just prevented people from
> being able to select the module unless they build the driver into the
> kernel, which isn't very nice.
Yes, exactly.
> What exactly needs to be done to fix this "correctly" that is going to
> take too much work at the moment?
Hi Greg,
The current implementation of hyperv-keyboard.c borrows
serio_interrupt() to pass the key strokes to the high level component.
serio_interrupt() actually depends on atkbd.c and i8042.c, so this doesn't
work for a Generation 2 hyper-v guest because no i8042 keyboard controller
is emulated: http://technet.microsoft.com/en-us/library/dn282285.aspx
To decouple the dependency between the hyperv-keyboard and i8042
modules, I suppose we probably have to re-implement hyperv-keyboard by
using input_allocate_device(), input_register_device(), and using
input_report_key() to pass the key strokes to the high level.
I'll have to need some time for further investigation and a new
implementation. Before the new code is completely ready, IMHO the
patch can help to avoid a bad user experience like Arch Linux working
as a Generation 2 hyper-v guest.
BTW, looks most of Linux distros (like RHEL, Ubuntu, SUSE) have
CONFIG_SERIO_I8042=y, probably because it's the result of
"make defconfig". So the patch actually doesn't affect these distros.
Thanks,
-- Dexuan
^ permalink raw reply
* Re: [PATCH] Input: serio: make HYPERV_KEYBOARD depend on SERIO_I8042=y
From: Greg KH @ 2014-08-12 3:21 UTC (permalink / raw)
To: Dexuan Cui
Cc: olaf, dmitry.torokhov, jasowang, driverdev-devel, linux-kernel,
linux-input, apw, haiyangz
In-Reply-To: <1407814240-4275-1-git-send-email-decui@microsoft.com>
On Mon, Aug 11, 2014 at 08:30:40PM -0700, Dexuan Cui wrote:
> hyperv_keyboard invokes serio_interrupt(), which needs a valid serio driver
> like atkbd.c.
> atkbd.c depends on libps2.c because it invokes ps2_command().
> libps2.c depends on i8042.c because it invokes i8042_check_port_owner().
> As a result, hyperv_keyboard actually depends on i8042.c.
>
> For a Generation 2 Hyper-V VM (meaning no i8042 device emulated), if a Linux
> VM (like Arch Linux) happens to configure CONFIG_SERIO_I8042=m rather than
> =y, atkbd.ko can't load because i8042.ko can't load(due to no i8042 device
> emulated) and finally hyperv_keyboard can't work and the user can't input:
> https://bugs.archlinux.org/task/39820
> (Ubuntu/RHEL/SUSE aren't affected since they use CONFIG_SERIO_I8042=y)
>
> Decoupling the dependency between hyperv_keyboard and i8042 needs
> non-trivial efforts and is hence a long term goal.
>
> For now, let's make the dependency explicit so people can beware of this.
You didn't make anyone "aware" of this, you just prevented people from
being able to select the module unless they build the driver into the
kernel, which isn't very nice.
What exactly needs to be done to fix this "correctly" that is going to
take too much work at the moment?
thanks,
greg k-h
^ permalink raw reply
* [PATCH] Input: serio: make HYPERV_KEYBOARD depend on SERIO_I8042=y
From: Dexuan Cui @ 2014-08-12 3:30 UTC (permalink / raw)
To: gregkh, dmitry.torokhov, linux-input, linux-kernel,
driverdev-devel, olaf, apw, jasowang
Cc: kys, haiyangz
hyperv_keyboard invokes serio_interrupt(), which needs a valid serio driver
like atkbd.c.
atkbd.c depends on libps2.c because it invokes ps2_command().
libps2.c depends on i8042.c because it invokes i8042_check_port_owner().
As a result, hyperv_keyboard actually depends on i8042.c.
For a Generation 2 Hyper-V VM (meaning no i8042 device emulated), if a Linux
VM (like Arch Linux) happens to configure CONFIG_SERIO_I8042=m rather than
=y, atkbd.ko can't load because i8042.ko can't load(due to no i8042 device
emulated) and finally hyperv_keyboard can't work and the user can't input:
https://bugs.archlinux.org/task/39820
(Ubuntu/RHEL/SUSE aren't affected since they use CONFIG_SERIO_I8042=y)
Decoupling the dependency between hyperv_keyboard and i8042 needs
non-trivial efforts and is hence a long term goal.
For now, let's make the dependency explicit so people can beware of this.
Thank Claudio for the initial reporting, investigation and suggesting the fix.
Signed-off-by: Dexuan Cui <decui@microsoft.com>
Reported-by: Claudio Latini <claudio.latini@live.com>
Cc: K. Y. Srinivasan <kys@microsoft.com>
---
drivers/input/serio/Kconfig | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/drivers/input/serio/Kconfig b/drivers/input/serio/Kconfig
index bc2d474..3277bff 100644
--- a/drivers/input/serio/Kconfig
+++ b/drivers/input/serio/Kconfig
@@ -273,7 +273,7 @@ config SERIO_OLPC_APSP
config HYPERV_KEYBOARD
tristate "Microsoft Synthetic Keyboard driver"
- depends on HYPERV
+ depends on HYPERV && SERIO_I8042=y
default HYPERV
help
Select this option to enable the Hyper-V Keyboard driver.
--
1.9.1
^ permalink raw reply related
* Re: HID: huion: Switch to generating report descriptor
From: Nikolai Kondrashov @ 2014-08-11 18:09 UTC (permalink / raw)
To: Dan Carpenter; +Cc: linux-input
In-Reply-To: <20140731094012.GH30276@mwanda>
Hi Dan,
On 07/31/2014 12:40 PM, Dan Carpenter wrote:
> The patch f8dd5cb2c6d0: "HID: huion: Switch to generating report
> descriptor" from Jul 23, 2014, leads to the following static checker
> warning:
>
> drivers/hid/hid-huion.c:126 huion_tablet_enable()
> error: doing dma on the stack (buf)
Thanks a lot for notifying about the problem.
I just submitted two patches fixing this under subject "HID: huion: Fix "doing
dma on the stack"".
Sincerely,
Nick
^ permalink raw reply
* [PATCH] Input: atmel_mxt_ts - fix a few issues reported by Coverity
From: Dmitry Torokhov @ 2014-08-11 18:03 UTC (permalink / raw)
To: linux-input
Cc: Nick Dyer, Yufeng Shen, Benson Leung, Stephen Warren,
linux-kernel
This should fix the following issues reported by Coverity:
*** CID 1230625: Logically dead code (DEADCODE)
/drivers/input/touchscreen/atmel_mxt_ts.c: 1692 in mxt_initialize()
*** CID 1230627: Missing break in switch (MISSING_BREAK)
/drivers/input/touchscreen/atmel_mxt_ts.c: 1436 in mxt_get_object_table()
*** CID 1230629: Out-of-bounds write (OVERRUN)
/drivers/input/touchscreen/atmel_mxt_ts.c: 1267 in mxt_update_cfg()
*** CID 1230632: Unused pointer value (UNUSED_VALUE)
/drivers/input/touchscreen/atmel_mxt_ts.c: 1211 in mxt_update_cfg()
Signed-off-by: Dmitry Torokhov <dmitry.torokhov@gmail.com>
---
drivers/input/touchscreen/atmel_mxt_ts.c | 17 +++++++++++------
1 file changed, 11 insertions(+), 6 deletions(-)
diff --git a/drivers/input/touchscreen/atmel_mxt_ts.c b/drivers/input/touchscreen/atmel_mxt_ts.c
index bbaf3ff..db178ed 100644
--- a/drivers/input/touchscreen/atmel_mxt_ts.c
+++ b/drivers/input/touchscreen/atmel_mxt_ts.c
@@ -1098,8 +1098,12 @@ static int mxt_prepare_cfg_mem(struct mxt_data *data,
/* Skip object */
for (i = 0; i < size; i++) {
ret = sscanf(cfg->data + data_pos, "%hhx%n",
- &val,
- &offset);
+ &val, &offset);
+ if (ret != 1) {
+ dev_err(dev, "Bad format in T%d at %d\n",
+ type, i);
+ return -EINVAL;
+ }
data_pos += offset;
}
continue;
@@ -1139,7 +1143,8 @@ static int mxt_prepare_cfg_mem(struct mxt_data *data,
&val,
&offset);
if (ret != 1) {
- dev_err(dev, "Bad format in T%d\n", type);
+ dev_err(dev, "Bad format in T%d at %d\n",
+ type, i);
return -EINVAL;
}
data_pos += offset;
@@ -1149,8 +1154,7 @@ static int mxt_prepare_cfg_mem(struct mxt_data *data,
byte_offset = reg + i - cfg_start_ofs;
- if (byte_offset >= 0 &&
- byte_offset <= config_mem_size) {
+ if (byte_offset >= 0 && byte_offset < config_mem_size) {
*(config_mem + byte_offset) = val;
} else {
dev_err(dev, "Bad object: reg:%d, T%d, ofs=%d\n",
@@ -1454,6 +1458,7 @@ static int mxt_get_object_table(struct mxt_data *data)
data->T5_msg_size = mxt_obj_size(object) - 1;
}
data->T5_address = object->start_address;
+ break;
case MXT_GEN_COMMAND_T6:
data->T6_reportid = min_id;
data->T6_address = object->start_address;
@@ -1707,7 +1712,7 @@ static int mxt_initialize(struct mxt_data *data)
return error;
}
- mxt_acquire_irq(data);
+ error = mxt_acquire_irq(data);
if (error)
goto err_free_object_table;
--
2.0.0.526.g5318336
--
Dmitry
^ permalink raw reply related
* [PATCH 2/2] HID: huion: Use allocated buffer for DMA
From: Nikolai Kondrashov @ 2014-08-11 17:45 UTC (permalink / raw)
To: Jiri Kosina; +Cc: linux-input, DIGImend-devel, Nikolai Kondrashov
In-Reply-To: <1407779132-26749-1-git-send-email-spbnick@gmail.com>
Allocate a buffer with kmalloc for receiving the parameters string
descriptor with usb_control_msg, instead of using a buffer on the stack,
as the latter is unsafe. Use an enum for indices into the buffer to
ensure the buffer size if sufficient.
This fixes the static checker error "doing dma on the stack (buf)".
---
drivers/hid/hid-huion.c | 50 +++++++++++++++++++++++++++++++++++++------------
1 file changed, 38 insertions(+), 12 deletions(-)
diff --git a/drivers/hid/hid-huion.c b/drivers/hid/hid-huion.c
index a683d4b..61b68ca 100644
--- a/drivers/hid/hid-huion.c
+++ b/drivers/hid/hid-huion.c
@@ -84,6 +84,15 @@ static const __u8 huion_tablet_rdesc_template[] = {
0xC0 /* End Collection */
};
+/* Parameter indices */
+enum huion_prm {
+ HUION_PRM_X_LM = 1,
+ HUION_PRM_Y_LM = 2,
+ HUION_PRM_PRESSURE_LM = 4,
+ HUION_PRM_RESOLUTION = 5,
+ HUION_PRM_NUM
+};
+
/* Driver data */
struct huion_drvdata {
__u8 *rdesc;
@@ -115,7 +124,8 @@ static int huion_tablet_enable(struct hid_device *hdev)
int rc;
struct usb_device *usb_dev = hid_to_usb_dev(hdev);
struct huion_drvdata *drvdata = hid_get_drvdata(hdev);
- __le16 buf[6];
+ __le16 *buf = NULL;
+ size_t len;
s32 params[HUION_PH_ID_NUM];
s32 resolution;
__u8 *p;
@@ -127,27 +137,38 @@ static int huion_tablet_enable(struct hid_device *hdev)
* driver traffic.
* NOTE: This enables fully-functional tablet mode.
*/
+ len = HUION_PRM_NUM * sizeof(*buf);
+ buf = kmalloc(len, GFP_KERNEL);
+ if (buf == NULL) {
+ hid_err(hdev, "failed to allocate parameter buffer\n");
+ rc = -ENOMEM;
+ goto cleanup;
+ }
rc = usb_control_msg(usb_dev, usb_rcvctrlpipe(usb_dev, 0),
USB_REQ_GET_DESCRIPTOR, USB_DIR_IN,
(USB_DT_STRING << 8) + 0x64,
- 0x0409, buf, sizeof(buf),
+ 0x0409, buf, len,
USB_CTRL_GET_TIMEOUT);
if (rc == -EPIPE) {
hid_err(hdev, "device parameters not found\n");
- return -ENODEV;
+ rc = -ENODEV;
+ goto cleanup;
} else if (rc < 0) {
hid_err(hdev, "failed to get device parameters: %d\n", rc);
- return -ENODEV;
- } else if (rc != sizeof(buf)) {
+ rc = -ENODEV;
+ goto cleanup;
+ } else if (rc != len) {
hid_err(hdev, "invalid device parameters\n");
- return -ENODEV;
+ rc = -ENODEV;
+ goto cleanup;
}
/* Extract device parameters */
- params[HUION_PH_ID_X_LM] = le16_to_cpu(buf[1]);
- params[HUION_PH_ID_Y_LM] = le16_to_cpu(buf[2]);
- params[HUION_PH_ID_PRESSURE_LM] = le16_to_cpu(buf[4]);
- resolution = le16_to_cpu(buf[5]);
+ params[HUION_PH_ID_X_LM] = le16_to_cpu(buf[HUION_PRM_X_LM]);
+ params[HUION_PH_ID_Y_LM] = le16_to_cpu(buf[HUION_PRM_Y_LM]);
+ params[HUION_PH_ID_PRESSURE_LM] =
+ le16_to_cpu(buf[HUION_PRM_PRESSURE_LM]);
+ resolution = le16_to_cpu(buf[HUION_PRM_RESOLUTION]);
if (resolution == 0) {
params[HUION_PH_ID_X_PM] = 0;
params[HUION_PH_ID_Y_PM] = 0;
@@ -164,7 +185,8 @@ static int huion_tablet_enable(struct hid_device *hdev)
GFP_KERNEL);
if (drvdata->rdesc == NULL) {
hid_err(hdev, "failed to allocate fixed rdesc\n");
- return -ENOMEM;
+ rc = -ENOMEM;
+ goto cleanup;
}
drvdata->rsize = sizeof(huion_tablet_rdesc_template);
@@ -183,7 +205,11 @@ static int huion_tablet_enable(struct hid_device *hdev)
}
}
- return 0;
+ rc = 0;
+
+cleanup:
+ kfree(buf);
+ return rc;
}
static int huion_probe(struct hid_device *hdev, const struct hid_device_id *id)
--
2.0.1
^ permalink raw reply related
* [PATCH 1/2] HID: huion: Fail on parameter retrieval errors
From: Nikolai Kondrashov @ 2014-08-11 17:45 UTC (permalink / raw)
To: Jiri Kosina; +Cc: linux-input, DIGImend-devel, Nikolai Kondrashov
In-Reply-To: <1407779132-26749-1-git-send-email-spbnick@gmail.com>
Fail Huion tablet interface enabling and probing, if parameter retrieval
fails. Move the main code path out of the else block accordingly.
This should prevent devices appearing in a half-working state due to
original report descriptor being used, simplifying diagnostics. This
also makes it easier to add cleanup in later commits, as error handling
is simplified.
---
drivers/hid/hid-huion.c | 96 +++++++++++++++++++++++++------------------------
1 file changed, 49 insertions(+), 47 deletions(-)
diff --git a/drivers/hid/hid-huion.c b/drivers/hid/hid-huion.c
index 60f44cd..a683d4b 100644
--- a/drivers/hid/hid-huion.c
+++ b/drivers/hid/hid-huion.c
@@ -116,6 +116,10 @@ static int huion_tablet_enable(struct hid_device *hdev)
struct usb_device *usb_dev = hid_to_usb_dev(hdev);
struct huion_drvdata *drvdata = hid_get_drvdata(hdev);
__le16 buf[6];
+ s32 params[HUION_PH_ID_NUM];
+ s32 resolution;
+ __u8 *p;
+ s32 v;
/*
* Read string descriptor containing tablet parameters. The specific
@@ -128,56 +132,54 @@ static int huion_tablet_enable(struct hid_device *hdev)
(USB_DT_STRING << 8) + 0x64,
0x0409, buf, sizeof(buf),
USB_CTRL_GET_TIMEOUT);
- if (rc == -EPIPE)
- hid_warn(hdev, "device parameters not found\n");
- else if (rc < 0)
- hid_warn(hdev, "failed to get device parameters: %d\n", rc);
- else if (rc != sizeof(buf))
- hid_warn(hdev, "invalid device parameters\n");
- else {
- s32 params[HUION_PH_ID_NUM];
- s32 resolution;
- __u8 *p;
- s32 v;
+ if (rc == -EPIPE) {
+ hid_err(hdev, "device parameters not found\n");
+ return -ENODEV;
+ } else if (rc < 0) {
+ hid_err(hdev, "failed to get device parameters: %d\n", rc);
+ return -ENODEV;
+ } else if (rc != sizeof(buf)) {
+ hid_err(hdev, "invalid device parameters\n");
+ return -ENODEV;
+ }
- /* Extract device parameters */
- params[HUION_PH_ID_X_LM] = le16_to_cpu(buf[1]);
- params[HUION_PH_ID_Y_LM] = le16_to_cpu(buf[2]);
- params[HUION_PH_ID_PRESSURE_LM] = le16_to_cpu(buf[4]);
- resolution = le16_to_cpu(buf[5]);
- if (resolution == 0) {
- params[HUION_PH_ID_X_PM] = 0;
- params[HUION_PH_ID_Y_PM] = 0;
- } else {
- params[HUION_PH_ID_X_PM] = params[HUION_PH_ID_X_LM] *
- 1000 / resolution;
- params[HUION_PH_ID_Y_PM] = params[HUION_PH_ID_Y_LM] *
- 1000 / resolution;
- }
+ /* Extract device parameters */
+ params[HUION_PH_ID_X_LM] = le16_to_cpu(buf[1]);
+ params[HUION_PH_ID_Y_LM] = le16_to_cpu(buf[2]);
+ params[HUION_PH_ID_PRESSURE_LM] = le16_to_cpu(buf[4]);
+ resolution = le16_to_cpu(buf[5]);
+ if (resolution == 0) {
+ params[HUION_PH_ID_X_PM] = 0;
+ params[HUION_PH_ID_Y_PM] = 0;
+ } else {
+ params[HUION_PH_ID_X_PM] = params[HUION_PH_ID_X_LM] *
+ 1000 / resolution;
+ params[HUION_PH_ID_Y_PM] = params[HUION_PH_ID_Y_LM] *
+ 1000 / resolution;
+ }
- /* Allocate fixed report descriptor */
- drvdata->rdesc = devm_kmalloc(&hdev->dev,
- sizeof(huion_tablet_rdesc_template),
- GFP_KERNEL);
- if (drvdata->rdesc == NULL) {
- hid_err(hdev, "failed to allocate fixed rdesc\n");
- return -ENOMEM;
- }
- drvdata->rsize = sizeof(huion_tablet_rdesc_template);
+ /* Allocate fixed report descriptor */
+ drvdata->rdesc = devm_kmalloc(&hdev->dev,
+ sizeof(huion_tablet_rdesc_template),
+ GFP_KERNEL);
+ if (drvdata->rdesc == NULL) {
+ hid_err(hdev, "failed to allocate fixed rdesc\n");
+ return -ENOMEM;
+ }
+ drvdata->rsize = sizeof(huion_tablet_rdesc_template);
- /* Format fixed report descriptor */
- memcpy(drvdata->rdesc, huion_tablet_rdesc_template,
- drvdata->rsize);
- for (p = drvdata->rdesc;
- p <= drvdata->rdesc + drvdata->rsize - 4;) {
- if (p[0] == 0xFE && p[1] == 0xED && p[2] == 0x1D &&
- p[3] < sizeof(params)) {
- v = params[p[3]];
- put_unaligned(cpu_to_le32(v), (s32 *)p);
- p += 4;
- } else {
- p++;
- }
+ /* Format fixed report descriptor */
+ memcpy(drvdata->rdesc, huion_tablet_rdesc_template,
+ drvdata->rsize);
+ for (p = drvdata->rdesc;
+ p <= drvdata->rdesc + drvdata->rsize - 4;) {
+ if (p[0] == 0xFE && p[1] == 0xED && p[2] == 0x1D &&
+ p[3] < sizeof(params)) {
+ v = params[p[3]];
+ put_unaligned(cpu_to_le32(v), (s32 *)p);
+ p += 4;
+ } else {
+ p++;
}
}
--
2.0.1
^ permalink raw reply related
* [PATCHES] HID: huion: Fix "doing dma on the stack"
From: Nikolai Kondrashov @ 2014-08-11 17:45 UTC (permalink / raw)
To: Jiri Kosina; +Cc: linux-input, DIGImend-devel
Hi everyone,
These two patches work towards fixing the "doing dma on the stack" reported by
a static analyzer (I'm not sure which one) for my recent changes to hid-huion
driver.
The first one is easier to read with whitespace changes ingored (-w).
Both patches were tested by me with a Huion H610N tablet.
Nick
drivers/hid/hid-huion.c | 128 +++++++++++++++++++++++++++++-------------------
1 file changed, 78 insertions(+), 50 deletions(-)
^ permalink raw reply
* [PATCH] input/joystick: use get_cycles on ARMv8
From: Mark Brown @ 2014-08-11 15:42 UTC (permalink / raw)
To: Dmitry Torokhov; +Cc: linux-input, linaro-kernel, Mark Brown
From: Mark Brown <broonie@linaro.org>
As with ARM the ARMv8 architecture provides a cycle counter which can be
used to provide a high resolution time for the joystick driver and silence
the build warning that results from not having a precise timer on ARMv8,
making allmodconfig and allyesconfig quieter.
Signed-off-by: Mark Brown <broonie@linaro.org>
---
drivers/input/joystick/analog.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/drivers/input/joystick/analog.c b/drivers/input/joystick/analog.c
index 9135606c8649..ab0fdcd36e18 100644
--- a/drivers/input/joystick/analog.c
+++ b/drivers/input/joystick/analog.c
@@ -158,7 +158,7 @@ static unsigned int get_time_pit(void)
#define GET_TIME(x) rdtscl(x)
#define DELTA(x,y) ((y)-(x))
#define TIME_NAME "TSC"
-#elif defined(__alpha__) || defined(CONFIG_MN10300) || defined(CONFIG_ARM) || defined(CONFIG_TILE)
+#elif defined(__alpha__) || defined(CONFIG_MN10300) || defined(CONFIG_ARM) || defined(CONFIG_ARM64) || defined(CONFIG_TILE)
#define GET_TIME(x) do { x = get_cycles(); } while (0)
#define DELTA(x,y) ((y)-(x))
#define TIME_NAME "get_cycles"
--
2.0.1
^ permalink raw reply related
* Re: [PATCH] Input: wacom - Fix compiler warning if !CONFIG_PM
From: Benjamin Tissoires @ 2014-08-11 14:42 UTC (permalink / raw)
To: Jiri Kosina
Cc: Geert Uytterhoeven, Dmitry Torokhov, linux-input, linux-kernel
In-Reply-To: <alpine.LNX.2.00.1408092238290.15220@pobox.suse.cz>
On Aug 09 2014 or thereabouts, Jiri Kosina wrote:
> On Sat, 9 Aug 2014, Geert Uytterhoeven wrote:
>
> > If CONFIG_PM is not set:
> >
> > drivers/hid/wacom_sys.c:1436: warning: ‘wacom_reset_resume’ defined but
> > not used
> >
> > Protect the unused functions by #ifdef CONFIG_PM to fix this.
> >
> > Signed-off-by: Geert Uytterhoeven <geert@linux-m68k.org>
>
> Thanks Geert.
>
> hid-wacom is in a sort of temporary mode of operation, being moved from
> Input to HID driver.
>
> Adding Benjamin and Dmitry to CC, as this will likely still go through
> Dmitry's queue.
>
Thanks for fixing this Geert.
Reviewed-by: Benjamin Tissoires <benjamin.tissoires@redhat.com>
Cheers,
Benjamin
--
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
* autoloading of modules failes
From: Christian Gmeiner @ 2014-08-11 12:42 UTC (permalink / raw)
To: linux-input; +Cc: devicetree
Hi all.
I finally want to mainline the ar1021 input driver but the only bit
which is missing is autoloading via device-tree.
I have sent an RFC [0] some long time ago for the driver and it looked okay.
Maybe someone has time to help me to get automatic loading working so
that I can send the final patch. My current state can be found here:
[1]
[0] http://www.spinics.net/lists/linux-input/msg29419.html
[1] http://dpaste.com/0V0XTR5
thanks
--
Christian Gmeiner, MSc
https://soundcloud.com/christian-gmeiner
^ permalink raw reply
* Re: [RFC RESEND 5/5] Input: evdev - add new EVIOCGABSRANGE ioctl
From: David Herrmann @ 2014-08-11 10:02 UTC (permalink / raw)
To: Peter Hutterer
Cc: Dmitry Torokhov, Benjamin Tissoires, open list:HID CORE LAYER,
Dmitry Torokhov
In-Reply-To: <20140811021316.GA26242@jelly.redhat.com>
Hi
On Mon, Aug 11, 2014 at 4:13 AM, Peter Hutterer
<peter.hutterer@who-t.net> wrote:
> On Sun, Aug 10, 2014 at 05:01:35PM -0700, Dmitry Torokhov wrote:
> [...]
>> > >
>> > > is about handling corner-cases. If we make SYN_DROPPED
>> > > handling cause SYN_DROPPED, we can just ignore it.
>> >
>> > yep, that too was my first thought. with a plain resync ioctl you're pretty
>> > much guaranteed to get SYN_DROPPED before the client manages to handle the
>> > resync. Even if you reduce the number of events as above because the most
>> > common occurance for SYN_DROPPED is in the ABS_MT range which we cannot
>> > meaningfully reduce.
>>
>> Hmm, that's a problem... But is it? We need to make sure that buffer is large
>> enough for the MT device to transmit all it's contacts properly. We can not
>> expect that we'll always be able to reduce number of events if a user actively
>> uses 10 contacts. IOW we need to solve this issue regardless of this proposed
>> sync ioctl.
>>
>> Maybe we need to review drivers and see if they need to supply their own hints
>> or update hinting logic in core?
>
> The buffer is already large enough for at least one full report from the
> device plus a few extra events [1]. for the devices we see SYN_REPORT most
> frequently dumping the state means filling up the buffer to the almost
> maximum. To give some room for movement, we need to increase the queue by at
> least a factor 2. That gives us with room for one whole sync report and at
> least one full extra event. Anything smaller we get the side-effect
> that a client that is too slow and gets a SYN_DROPPED is actually worse off
> because now the buffer is so full from the sync that a SYN_DROPPED is even
> more likely to occur than before.
>
> We also need to define the behaviour for the queue filling up while the
> client is in the middle of a sync. That means the client must be
> able to handle SYN_DROPPED as well as SYN_SYNC_DONE during a sync or the
> kernel protects the events up to SYN_SYNC_DONE in the queue in the case of
> a SYN_DROPPED.
>
> Either way it's IMO more complicated than having a separate buffer for the
> sync state.
Yepp, fully agree on both points you make (I summarized them way worse
than you did!).
Thanks
David
^ permalink raw reply
* Re: [RFC RESEND 5/5] Input: evdev - add new EVIOCGABSRANGE ioctl
From: David Herrmann @ 2014-08-11 10:00 UTC (permalink / raw)
To: Peter Hutterer
Cc: Dmitry Torokhov, Benjamin Tissoires, open list:HID CORE LAYER,
Dmitry Torokhov
In-Reply-To: <20140810231759.GA10731@jelly.redhat.com>
Hi
>> > No, it is more like EVIOCRESYNC(void) which makes input core to dump all
>> > existing state into the client's standard event queue so that here is no
>> > need to reconcile/reconstruct anything. We could give a new SYN marker
>> > to indicate end-of-state boundary.
>>
>> This doesn't make sense to me, for two reasons:
>> * Events received during re-sync are usually handled differently than
>> normal events. For instance, you *must* never handle key events
>> from re-syncs for shortcuts, because you don't know the order
>> they were pressed in.
>
> I think you're vastly overestimating what the majority of userspace does ;)
> both the xorg and the libinput-based stacks pretty much ignore this at the
> moment and by the time we handle shortcuts we're already one or more APIs
> away from the SYN_DROPPED handling.
I know what Xorg and weston do and, honestly, I don't care. If major
user-space components decide to ignore those side-effects, I'm fine.
They may have legitimate reason to do so. But I don't think we should
use this to argue against proper API design in the kernel.
> also, I haven't seen SYN_DROPPED issues from keyboard events. They usually
> happen only on absolute touch devices and ironically enough that's the one
> case where the kernel currently doesn't allow for a race-less resync.
Yes, obviously ABS and REL events are the culprits as they can occur
in vast amounts. However, any other events in between (like button
presses) may be dropped due to excessive ABS events. So I disagree
that this is an exclusive issue of ABS/REL, they just trigger the
SYN_DROPPED.
>> If we put sync events in the queue, users cannot know where an event
>> from.
>> Inserting a SYNC marker is ridiculous, because following normal
>> events may drop those (via SYN_DROPPED). We'd have to
>> protect it. Furthermore, it's awful to handle this from user-space,
>> because you have no idea how many events are sent as part
>> of the sync.
>
> I suspect Dmitry meant a new EV_SYN code. From a userspace POV, I'd know
> that after the ioctl() all events up until SYN_SYNC_DONE are the syncing
> events. That would work well and is coincidentally almost identical to the
> libevdev public API.
>
> In libevdev we already have code to calculate the maximum number of events
> by a device for the current SYN_DROPPED handling. And you can reduce the
> number of events by only sending those in a non-zero state for EV_KEY,
> EV_LED, etc.
Yes, I understand what Dmitry meant and I see the similarities to
libevdev. However, there are several issues:
1) *If* you flush all events into the queue, followed by a
SYN_SYNC_DONE marker, any following *real* events might cause the
queue to overflow and thus drop the SYN_SYNC_DONE marker. So if
user-space requested a sync and then reads a SYN_DROPPED *before*
reading a SYN_SYNC_DONE, it knows the sync didn't complete and has do
redo the sync all over. This is doable, but makes we wonder whether
it's the right design.
2) The evdev queue is not designed to hold this many events. Even if
it is, a SYN_DROPPED happens during excessive event reports. So if we
now also fill the queue with all sync-events, we increase the change
of overflowing the queue again, instead of clearing it and syncing
separately.
3) Flushing sync-events in the queue with special semantics (like only
sending non-zero state) sounds to me like a dirty hack, not like
proper API design. I mean, the queue was designed to send realtime
events, not to provide initial/SYN_DROPPED syncing. I don't see why
this is preferable to a separate user-provided buffer.
> IMO the real issue preventing this approach is:
>
>> * The receive-queue is usually too small to hold all those events.
>> I mean, evdev uses EVDEV_BUF_PACKETS as number of buffer
>> slots (defined as 8!). You cannot even hold a whole keyboard
>> state there. Yes, usually it should be enough, but re-syncing
>> is about handling corner-cases. If we make SYN_DROPPED
>> handling cause SYN_DROPPED, we can just ignore it.
>
> yep, that too was my first thought. with a plain resync ioctl you're pretty
> much guaranteed to get SYN_DROPPED before the client manages to handle the
> resync. Even if you reduce the number of events as above because the most
> common occurance for SYN_DROPPED is in the ABS_MT range which we cannot
> meaningfully reduce.
>
>> I understand why EVIORESYNC(void) is tempting. It avoids all the
>> complexity of my patch and makes all the other sync-ioctls we have so
>> far obsolete. But the reason we want it, is to avoid shortcomings of
>> the limited receive-queue. I don't think any solution involving the
>> receive-queue is going to work out. Even if we always make sure the
>> receive-queue is big enough, we might still get new events coming in
>> before user-space can drain it.
>>
>> How about this:
>>
>> struct input_resync {
>> __u64 count;
>> struct input_event buffer[];
>> };
>>
>> EVIOCSYNC(struct input_resync) copies the current state of all
>> available event-types of the device into the buffer. It returns the
>> number of events that it wants to write (so user-space can pass
>> count==0 the first time to figure out the required buffer-size). The
>> ioctl then flushed the _whole_ input queue.
>
> if you go with the suggestion above of only putting events in with a
> non-zero state then a count of 0 is guaranteed to return the wrong number
> since you may get events between the two ioctl calls. I don't think
> the double ioctl() call is that a case we need. as I said above the case of
> all events must already be handled in userspace anyway.
I think it's a bad idea to only send events with non-zero state. I can
live with it, but I don't understand why we do such optimizations for
an edge-case like SYN_DROPPED handling. It just complicates things and
no-one cares whether we copy 10 or 100 events, right?
> what's the reason you don't use a EVIOCSYNC(len) approach btw? quick check
> shows at 13 or 14 bits for len which should be enough for pretty much any
> device for a long time.
I think we can just make user-space read the event-bitmasks and
calculate the length themselves. This way, we avoid double-ioctl()
calls and it's easy enough to do from user-space.
>> Note that even if we used the client's receive buffer, we'd have to
>> copy huge amounts of events while holding the event lock. So both
>> ideas have to lock the buffers during the operation. The downside of
>> my solution is that we have to allocate temporary buffers as we cannot
>> copy to user-space while holding spin-locks. However, my code already
>> makes sure we don't do high-order allocations, so I think this is
>> fine.
>>
>> This ioctl would also allow us to get rid of all the other QUERY
>> ioctls. User-space can use it to initialize it's state and then update
>> it according to incoming events. This way, we simplify the API
>> considerably! I really like this idea. I know, we only need this to
>> handle corner-cases. But as Peter already said, it's really easy to
>> trigger and ignoring it causes awful bugs in user-space.
>
> summary from my side: I'd prefer EVIOCSYNC() with a buffer parameter, only
> sending nonzero state for key/led/sw/etc. and the full state for abs. no
> need for a new EV_SYN code, and provided it empties the whole queue we
> should be race-free.
Except for "only send non-zero state" I fully agree. But I would also
accept it with that limitation/optimization.
Thanks
David
^ permalink raw reply
* Re: [PATCH v2 1/1] Input: atmel_mxt_ts - Get IRQ edge/level flags on DT booting
From: Javier Martinez Canillas @ 2014-08-11 9:09 UTC (permalink / raw)
To: Linus Walleij
Cc: Doug Anderson, Dmitry Torokhov, Nick Dyer, Stephen Warren,
Yufeng Shen, Benson Leung, Olof Johansson, Tomasz Figa,
linux-input@vger.kernel.org, devicetree@vger.kernel.org,
linux-samsung-soc, linux-kernel@vger.kernel.org, Thomas Gleixner,
Jason Cooper, Benjamin Herrenschmidt, Thomas Abraham
In-Reply-To: <CACRpkdZ9-5Pw-SHFNuGtvxDDHrA13ShSSFN9uZRBLTfpe=m5ug@mail.gmail.com>
Hello Linus,
On 08/11/2014 10:32 AM, Linus Walleij wrote:
> I guess this discussion is about drivers/pinctrl/samsung/pinctrl-exynos.c?
>
> Or else I'm not really following this... $SUBJECT is a bit confusing.
>
Yes, the thing is that at the beginning I (wrongly) thought that the IRQ trigger
type flags defined in the DTS were discarded if someone called
request_threaded_irq() with a flags != 0. And since the atmel touchpad driver
was pass pdata->irqflags | IRQF_ONESHOT I posted this patch to get the trigger
type parsed from DT and pass it to request_threaded_irq().
But after digging further I found that the issue was in the pinctrl-exynos
driver and Tomasz explained to me that the real cause was that the pin was being
configured from GPIO IRQ to GPIO input (which on other platforms is just the
same mode but on Exynos are two different modes).
At the end passing the trigger type to request_threaded_irq() just had the side
effect to call the exynos .irq_set_type function handler that reconfigured the
pin as GPIO-IRQ and that is why it made it to work. So I posted a patch from
Tomasz that fixes the real cause [0].
I'm so sorry for the confusion.
> Yours,
> Linus Walleij
>
Best regards,
Javier
[0]: https://lkml.org/lkml/2014/8/8/962
^ permalink raw reply
* Re: [PATCH v2 1/1] Input: atmel_mxt_ts - Get IRQ edge/level flags on DT booting
From: Linus Walleij @ 2014-08-11 8:32 UTC (permalink / raw)
To: Javier Martinez Canillas
Cc: Doug Anderson, Dmitry Torokhov, Nick Dyer, Stephen Warren,
Yufeng Shen, Benson Leung, Olof Johansson, Tomasz Figa,
linux-input@vger.kernel.org, devicetree@vger.kernel.org,
linux-samsung-soc, linux-kernel@vger.kernel.org, Thomas Gleixner,
Jason Cooper, Benjamin Herrenschmidt, Thomas Abraham
In-Reply-To: <53E54E91.9060905@collabora.co.uk>
I guess this discussion is about drivers/pinctrl/samsung/pinctrl-exynos.c?
Or else I'm not really following this... $SUBJECT is a bit confusing.
On Sat, Aug 9, 2014 at 12:26 AM, Javier Martinez Canillas
<javier.martinez@collabora.co.uk> wrote:
> Regardless of this though I think that both the patch to move the IRQ
> pinmux setup from .irq_set_type to the .irq_request_resources and the patch to
> to prevent any pinmux reconfiguration are good improvements to avoid future
> issues like the one we found here.
I think someone should look into switching the Samsung/Exynos pinctrl
driver to the gpiolib irqchip helpers, I looked at it but was scared by
the special wkup chip and stuff I can't test.
The irqchip helpers will atleast help out in flagging GPIO lines as
used for IRQs so the core can keep track of stuff and show that
properly in debugfs.
The orthogonality compliance between GPIO and irqchip must
however be solved in the driver itself, the core only helps out
in blocking some abuse of the API.
Yours,
Linus Walleij
^ permalink raw reply
* Re: [RFC RESEND 5/5] Input: evdev - add new EVIOCGABSRANGE ioctl
From: Peter Hutterer @ 2014-08-11 2:13 UTC (permalink / raw)
To: Dmitry Torokhov
Cc: David Herrmann, Benjamin Tissoires, open list:HID CORE LAYER,
Dmitry Torokhov
In-Reply-To: <20140811000134.GA12650@core.coreip.homeip.net>
On Sun, Aug 10, 2014 at 05:01:35PM -0700, Dmitry Torokhov wrote:
[...]
> > >
> > > is about handling corner-cases. If we make SYN_DROPPED
> > > handling cause SYN_DROPPED, we can just ignore it.
> >
> > yep, that too was my first thought. with a plain resync ioctl you're pretty
> > much guaranteed to get SYN_DROPPED before the client manages to handle the
> > resync. Even if you reduce the number of events as above because the most
> > common occurance for SYN_DROPPED is in the ABS_MT range which we cannot
> > meaningfully reduce.
>
> Hmm, that's a problem... But is it? We need to make sure that buffer is large
> enough for the MT device to transmit all it's contacts properly. We can not
> expect that we'll always be able to reduce number of events if a user actively
> uses 10 contacts. IOW we need to solve this issue regardless of this proposed
> sync ioctl.
>
> Maybe we need to review drivers and see if they need to supply their own hints
> or update hinting logic in core?
The buffer is already large enough for at least one full report from the
device plus a few extra events [1]. for the devices we see SYN_REPORT most
frequently dumping the state means filling up the buffer to the almost
maximum. To give some room for movement, we need to increase the queue by at
least a factor 2. That gives us with room for one whole sync report and at
least one full extra event. Anything smaller we get the side-effect
that a client that is too slow and gets a SYN_DROPPED is actually worse off
because now the buffer is so full from the sync that a SYN_DROPPED is even
more likely to occur than before.
We also need to define the behaviour for the queue filling up while the
client is in the middle of a sync. That means the client must be
able to handle SYN_DROPPED as well as SYN_SYNC_DONE during a sync or the
kernel protects the events up to SYN_SYNC_DONE in the queue in the case of
a SYN_DROPPED.
Either way it's IMO more complicated than having a separate buffer for the
sync state.
Cheers,
Peter
[1] almost, it doesn't account for EV_SW for example
^ permalink raw reply
* Re: [RFC RESEND 5/5] Input: evdev - add new EVIOCGABSRANGE ioctl
From: Dmitry Torokhov @ 2014-08-11 0:01 UTC (permalink / raw)
To: Peter Hutterer
Cc: David Herrmann, Benjamin Tissoires, open list:HID CORE LAYER,
Dmitry Torokhov
In-Reply-To: <20140810231759.GA10731@jelly.redhat.com>
On Mon, Aug 11, 2014 at 09:17:59AM +1000, Peter Hutterer wrote:
> On Sun, Aug 10, 2014 at 05:21:47PM +0200, David Herrmann wrote:
> > Hi
> >
> > On Fri, Aug 8, 2014 at 7:47 PM, Dmitry Torokhov
> > <dmitry.torokhov@gmail.com> wrote:
> > > On Fri, Aug 08, 2014 at 03:26:56PM +0200, David Herrmann wrote:
> > >> Hi
> > >>
> > >> On Wed, Aug 6, 2014 at 3:35 AM, Peter Hutterer <peter.hutterer@who-t.net> wrote:
> > >> >> +
> > >> >> +/**
> > >> >> + * EVIOCGABSRANGE - Fetch range of ABS values
> > >> >> + *
> > >> >> + * This fetches the current values of a range of ABS codes atomically. The range
> > >> >> + * of codes to fetch and the buffer-types are passed as "struct input_absrange",
> > >> >> + * which has the following fields:
> > >> >> + * slots: Number of MT slots to fetch data for.
> > >> >> + * code: First ABS axis to query.
> > >> >> + * count: Number of ABS axes to query starting at @code.
> > >> >> + * buffer: Pointer to a receive buffer where to store the fetched ABS
> > >> >> + * values. This buffer must be an array of __s32 with at least
> > >> >> + * (@slots * @code) elements. The buffer is interpreted as two
> > >> >> + * dimensional __s32 array, declared as: __s32[slots][codes]
> > >> >
> > >> > tbh this seems more complicated than necessary. Have you thought about
> > >> > just dumping the events into the client buffer as if they came fresh in from
> > >> > the device? So to sync, the client calls the ioctl with a buffer and a
> > >> > buffer size, and the kernel simply writes a series of struct input_events
> > >> > into that buffer, with ABS_MT_SLOT as required for all slots, (optionally?)
> > >> > followed by a SYN_DROPPED. So the buffer afterwards could look like this:
> > >> > EV_ABS ABS_X 30
> > >> > EV_ABS ABS_X 1202
> > >> > EV_ABS ABS_MT_SLOT 0
> > >> > EV_ABS ABS_MT_POSITION_X 30
> > >> > EV_ABS ABS_MT_POSITION_Y 1202
> > >> > EV_ABS ABS_MT_SLOT 1
> > >> > EV_ABS ABS_MT_POSITION_X 80
> > >> > EV_ABS ABS_MT_POSITION_Y 1800
> > >> > EV_SYN SYN_REPORT 0
> > >> >
> > >> > the client can then go through and just process the events on-by-one as it
> > >> > would otherwise with real events.
> > >> >
> > >> > This approach could be even extended to include EV_KEY, etc. providing a
> > >> > single ioctl to sync the whole state of the device atomically.
> > >> >
> > >> > comments?
> > >
> > > I like it.
> > >
> > >>
> > >> So you mean instead of passing a __32 array we pass a "struct
> > >> input_event" array and write it there? So bypassing the receive-queue?
> > >> That does sound quite nice, indeed. We could replace all the other
> > >> "sync" ioctls and just provide a way to receive all the events
> > >> directly.
> > >>
> > >> Something like:
> > >>
> > >> EVIOCQUERY(struct input_query)
> > >>
> > >> struct input_query {
> > >> __u16 type;
> > >> __u16 start_code;
> > >> __u16 end_code;
> > >> __u16 slots;
> > >>
> > >> struct input_event buffer[];
> > >> };
> > >
> > > No, it is more like EVIOCRESYNC(void) which makes input core to dump all
> > > existing state into the client's standard event queue so that here is no
> > > need to reconcile/reconstruct anything. We could give a new SYN marker
> > > to indicate end-of-state boundary.
> >
> > This doesn't make sense to me, for two reasons:
> > * Events received during re-sync are usually handled differently than
> > normal events. For instance, you *must* never handle key events
> > from re-syncs for shortcuts, because you don't know the order
> > they were pressed in.
>
> I think you're vastly overestimating what the majority of userspace does ;)
> both the xorg and the libinput-based stacks pretty much ignore this at the
> moment and by the time we handle shortcuts we're already one or more APIs
> away from the SYN_DROPPED handling.
Also, client would not out of sudden get a bunch of events, it would have
requested them to be resent so it would know how to handle them properly.
>
> also, I haven't seen SYN_DROPPED issues from keyboard events. They usually
> happen only on absolute touch devices and ironically enough that's the one
> case where the kernel currently doesn't allow for a race-less resync.
>
> > If we put sync events in the queue, users cannot know where an event
> > from.
> > Inserting a SYNC marker is ridiculous, because following normal
> > events may drop those (via SYN_DROPPED). We'd have to
> > protect it. Furthermore, it's awful to handle this from user-space,
> > because you have no idea how many events are sent as part
> > of the sync.
>
> I suspect Dmitry meant a new EV_SYN code. From a userspace POV, I'd know
> that after the ioctl() all events up until SYN_SYNC_DONE are the syncing
> events. That would work well and is coincidentally almost identical to the
> libevdev public API.
Yes, that's what I ment.
>
> In libevdev we already have code to calculate the maximum number of events
> by a device for the current SYN_DROPPED handling. And you can reduce the
> number of events by only sending those in a non-zero state for EV_KEY,
> EV_LED, etc.
>
> IMO the real issue preventing this approach is:
>
> > * The receive-queue is usually too small to hold all those events.
> > I mean, evdev uses EVDEV_BUF_PACKETS as number of buffer
> > slots (defined as 8!). You cannot even hold a whole keyboard
> > state there. Yes, usually it should be enough, but re-syncing
Note that EVDEV_BUF_PACKETS is not raw number of events in the queue, but
number for full "packets". For plain keyboards we end up with buffer enough to
hold 8 * 8 = 64 events. Since we only need to transmit active keys that should
be enough for any keyboard and if there are keyboards reporting more then we'd
have to change their hint anyway.
> >
> > is about handling corner-cases. If we make SYN_DROPPED
> > handling cause SYN_DROPPED, we can just ignore it.
>
> yep, that too was my first thought. with a plain resync ioctl you're pretty
> much guaranteed to get SYN_DROPPED before the client manages to handle the
> resync. Even if you reduce the number of events as above because the most
> common occurance for SYN_DROPPED is in the ABS_MT range which we cannot
> meaningfully reduce.
Hmm, that's a problem... But is it? We need to make sure that buffer is large
enough for the MT device to transmit all it's contacts properly. We can not
expect that we'll always be able to reduce number of events if a user actively
uses 10 contacts. IOW we need to solve this issue regardless of this proposed
sync ioctl.
Maybe we need to review drivers and see if they need to supply their own hints
or update hinting logic in core?
Thanks.
--
Dmitry
^ permalink raw reply
* Re: [RFC RESEND 5/5] Input: evdev - add new EVIOCGABSRANGE ioctl
From: Peter Hutterer @ 2014-08-10 23:17 UTC (permalink / raw)
To: David Herrmann
Cc: Dmitry Torokhov, Benjamin Tissoires, open list:HID CORE LAYER,
Dmitry Torokhov
In-Reply-To: <CANq1E4R14eOiXQ9p7XOPiOBuOnNns+EwgQMqrwOqeVKn4y8vAQ@mail.gmail.com>
On Sun, Aug 10, 2014 at 05:21:47PM +0200, David Herrmann wrote:
> Hi
>
> On Fri, Aug 8, 2014 at 7:47 PM, Dmitry Torokhov
> <dmitry.torokhov@gmail.com> wrote:
> > On Fri, Aug 08, 2014 at 03:26:56PM +0200, David Herrmann wrote:
> >> Hi
> >>
> >> On Wed, Aug 6, 2014 at 3:35 AM, Peter Hutterer <peter.hutterer@who-t.net> wrote:
> >> >> +
> >> >> +/**
> >> >> + * EVIOCGABSRANGE - Fetch range of ABS values
> >> >> + *
> >> >> + * This fetches the current values of a range of ABS codes atomically. The range
> >> >> + * of codes to fetch and the buffer-types are passed as "struct input_absrange",
> >> >> + * which has the following fields:
> >> >> + * slots: Number of MT slots to fetch data for.
> >> >> + * code: First ABS axis to query.
> >> >> + * count: Number of ABS axes to query starting at @code.
> >> >> + * buffer: Pointer to a receive buffer where to store the fetched ABS
> >> >> + * values. This buffer must be an array of __s32 with at least
> >> >> + * (@slots * @code) elements. The buffer is interpreted as two
> >> >> + * dimensional __s32 array, declared as: __s32[slots][codes]
> >> >
> >> > tbh this seems more complicated than necessary. Have you thought about
> >> > just dumping the events into the client buffer as if they came fresh in from
> >> > the device? So to sync, the client calls the ioctl with a buffer and a
> >> > buffer size, and the kernel simply writes a series of struct input_events
> >> > into that buffer, with ABS_MT_SLOT as required for all slots, (optionally?)
> >> > followed by a SYN_DROPPED. So the buffer afterwards could look like this:
> >> > EV_ABS ABS_X 30
> >> > EV_ABS ABS_X 1202
> >> > EV_ABS ABS_MT_SLOT 0
> >> > EV_ABS ABS_MT_POSITION_X 30
> >> > EV_ABS ABS_MT_POSITION_Y 1202
> >> > EV_ABS ABS_MT_SLOT 1
> >> > EV_ABS ABS_MT_POSITION_X 80
> >> > EV_ABS ABS_MT_POSITION_Y 1800
> >> > EV_SYN SYN_REPORT 0
> >> >
> >> > the client can then go through and just process the events on-by-one as it
> >> > would otherwise with real events.
> >> >
> >> > This approach could be even extended to include EV_KEY, etc. providing a
> >> > single ioctl to sync the whole state of the device atomically.
> >> >
> >> > comments?
> >
> > I like it.
> >
> >>
> >> So you mean instead of passing a __32 array we pass a "struct
> >> input_event" array and write it there? So bypassing the receive-queue?
> >> That does sound quite nice, indeed. We could replace all the other
> >> "sync" ioctls and just provide a way to receive all the events
> >> directly.
> >>
> >> Something like:
> >>
> >> EVIOCQUERY(struct input_query)
> >>
> >> struct input_query {
> >> __u16 type;
> >> __u16 start_code;
> >> __u16 end_code;
> >> __u16 slots;
> >>
> >> struct input_event buffer[];
> >> };
> >
> > No, it is more like EVIOCRESYNC(void) which makes input core to dump all
> > existing state into the client's standard event queue so that here is no
> > need to reconcile/reconstruct anything. We could give a new SYN marker
> > to indicate end-of-state boundary.
>
> This doesn't make sense to me, for two reasons:
> * Events received during re-sync are usually handled differently than
> normal events. For instance, you *must* never handle key events
> from re-syncs for shortcuts, because you don't know the order
> they were pressed in.
I think you're vastly overestimating what the majority of userspace does ;)
both the xorg and the libinput-based stacks pretty much ignore this at the
moment and by the time we handle shortcuts we're already one or more APIs
away from the SYN_DROPPED handling.
also, I haven't seen SYN_DROPPED issues from keyboard events. They usually
happen only on absolute touch devices and ironically enough that's the one
case where the kernel currently doesn't allow for a race-less resync.
> If we put sync events in the queue, users cannot know where an event
> from.
> Inserting a SYNC marker is ridiculous, because following normal
> events may drop those (via SYN_DROPPED). We'd have to
> protect it. Furthermore, it's awful to handle this from user-space,
> because you have no idea how many events are sent as part
> of the sync.
I suspect Dmitry meant a new EV_SYN code. From a userspace POV, I'd know
that after the ioctl() all events up until SYN_SYNC_DONE are the syncing
events. That would work well and is coincidentally almost identical to the
libevdev public API.
In libevdev we already have code to calculate the maximum number of events
by a device for the current SYN_DROPPED handling. And you can reduce the
number of events by only sending those in a non-zero state for EV_KEY,
EV_LED, etc.
IMO the real issue preventing this approach is:
> * The receive-queue is usually too small to hold all those events.
> I mean, evdev uses EVDEV_BUF_PACKETS as number of buffer
> slots (defined as 8!). You cannot even hold a whole keyboard
> state there. Yes, usually it should be enough, but re-syncing
> is about handling corner-cases. If we make SYN_DROPPED
> handling cause SYN_DROPPED, we can just ignore it.
yep, that too was my first thought. with a plain resync ioctl you're pretty
much guaranteed to get SYN_DROPPED before the client manages to handle the
resync. Even if you reduce the number of events as above because the most
common occurance for SYN_DROPPED is in the ABS_MT range which we cannot
meaningfully reduce.
> I understand why EVIORESYNC(void) is tempting. It avoids all the
> complexity of my patch and makes all the other sync-ioctls we have so
> far obsolete. But the reason we want it, is to avoid shortcomings of
> the limited receive-queue. I don't think any solution involving the
> receive-queue is going to work out. Even if we always make sure the
> receive-queue is big enough, we might still get new events coming in
> before user-space can drain it.
>
> How about this:
>
> struct input_resync {
> __u64 count;
> struct input_event buffer[];
> };
>
> EVIOCSYNC(struct input_resync) copies the current state of all
> available event-types of the device into the buffer. It returns the
> number of events that it wants to write (so user-space can pass
> count==0 the first time to figure out the required buffer-size). The
> ioctl then flushed the _whole_ input queue.
if you go with the suggestion above of only putting events in with a
non-zero state then a count of 0 is guaranteed to return the wrong number
since you may get events between the two ioctl calls. I don't think
the double ioctl() call is that a case we need. as I said above the case of
all events must already be handled in userspace anyway.
what's the reason you don't use a EVIOCSYNC(len) approach btw? quick check
shows at 13 or 14 bits for len which should be enough for pretty much any
device for a long time.
> Note that even if we used the client's receive buffer, we'd have to
> copy huge amounts of events while holding the event lock. So both
> ideas have to lock the buffers during the operation. The downside of
> my solution is that we have to allocate temporary buffers as we cannot
> copy to user-space while holding spin-locks. However, my code already
> makes sure we don't do high-order allocations, so I think this is
> fine.
>
> This ioctl would also allow us to get rid of all the other QUERY
> ioctls. User-space can use it to initialize it's state and then update
> it according to incoming events. This way, we simplify the API
> considerably! I really like this idea. I know, we only need this to
> handle corner-cases. But as Peter already said, it's really easy to
> trigger and ignoring it causes awful bugs in user-space.
summary from my side: I'd prefer EVIOCSYNC() with a buffer parameter, only
sending nonzero state for key/led/sw/etc. and the full state for abs. no
need for a new EV_SYN code, and provided it empties the whole queue we
should be race-free.
Cheers,
Peter
^ permalink raw reply
* Re: [RFC RESEND 5/5] Input: evdev - add new EVIOCGABSRANGE ioctl
From: David Herrmann @ 2014-08-10 15:21 UTC (permalink / raw)
To: Dmitry Torokhov
Cc: Peter Hutterer, Benjamin Tissoires, open list:HID CORE LAYER,
Dmitry Torokhov
In-Reply-To: <20140808174742.GG26820@core.coreip.homeip.net>
Hi
On Fri, Aug 8, 2014 at 7:47 PM, Dmitry Torokhov
<dmitry.torokhov@gmail.com> wrote:
> On Fri, Aug 08, 2014 at 03:26:56PM +0200, David Herrmann wrote:
>> Hi
>>
>> On Wed, Aug 6, 2014 at 3:35 AM, Peter Hutterer <peter.hutterer@who-t.net> wrote:
>> >> +
>> >> +/**
>> >> + * EVIOCGABSRANGE - Fetch range of ABS values
>> >> + *
>> >> + * This fetches the current values of a range of ABS codes atomically. The range
>> >> + * of codes to fetch and the buffer-types are passed as "struct input_absrange",
>> >> + * which has the following fields:
>> >> + * slots: Number of MT slots to fetch data for.
>> >> + * code: First ABS axis to query.
>> >> + * count: Number of ABS axes to query starting at @code.
>> >> + * buffer: Pointer to a receive buffer where to store the fetched ABS
>> >> + * values. This buffer must be an array of __s32 with at least
>> >> + * (@slots * @code) elements. The buffer is interpreted as two
>> >> + * dimensional __s32 array, declared as: __s32[slots][codes]
>> >
>> > tbh this seems more complicated than necessary. Have you thought about
>> > just dumping the events into the client buffer as if they came fresh in from
>> > the device? So to sync, the client calls the ioctl with a buffer and a
>> > buffer size, and the kernel simply writes a series of struct input_events
>> > into that buffer, with ABS_MT_SLOT as required for all slots, (optionally?)
>> > followed by a SYN_DROPPED. So the buffer afterwards could look like this:
>> > EV_ABS ABS_X 30
>> > EV_ABS ABS_X 1202
>> > EV_ABS ABS_MT_SLOT 0
>> > EV_ABS ABS_MT_POSITION_X 30
>> > EV_ABS ABS_MT_POSITION_Y 1202
>> > EV_ABS ABS_MT_SLOT 1
>> > EV_ABS ABS_MT_POSITION_X 80
>> > EV_ABS ABS_MT_POSITION_Y 1800
>> > EV_SYN SYN_REPORT 0
>> >
>> > the client can then go through and just process the events on-by-one as it
>> > would otherwise with real events.
>> >
>> > This approach could be even extended to include EV_KEY, etc. providing a
>> > single ioctl to sync the whole state of the device atomically.
>> >
>> > comments?
>
> I like it.
>
>>
>> So you mean instead of passing a __32 array we pass a "struct
>> input_event" array and write it there? So bypassing the receive-queue?
>> That does sound quite nice, indeed. We could replace all the other
>> "sync" ioctls and just provide a way to receive all the events
>> directly.
>>
>> Something like:
>>
>> EVIOCQUERY(struct input_query)
>>
>> struct input_query {
>> __u16 type;
>> __u16 start_code;
>> __u16 end_code;
>> __u16 slots;
>>
>> struct input_event buffer[];
>> };
>
> No, it is more like EVIOCRESYNC(void) which makes input core to dump all
> existing state into the client's standard event queue so that here is no
> need to reconcile/reconstruct anything. We could give a new SYN marker
> to indicate end-of-state boundary.
This doesn't make sense to me, for two reasons:
* Events received during re-sync are usually handled differently than
normal events. For instance, you *must* never handle key events
from re-syncs for shortcuts, because you don't know the order
they were pressed in. If we put sync events in the queue, users
cannot know where an event came from.
Inserting a SYNC marker is ridiculous, because following normal
events may drop those (via SYN_DROPPED). We'd have to
protect it. Furthermore, it's awful to handle this from user-space,
because you have no idea how many events are sent as part
of the sync.
* The receive-queue is usually too small to hold all those events.
I mean, evdev uses EVDEV_BUF_PACKETS as number of buffer
slots (defined as 8!). You cannot even hold a whole keyboard
state there. Yes, usually it should be enough, but re-syncing
is about handling corner-cases. If we make SYN_DROPPED
handling cause SYN_DROPPED, we can just ignore it.
I understand why EVIORESYNC(void) is tempting. It avoids all the
complexity of my patch and makes all the other sync-ioctls we have so
far obsolete. But the reason we want it, is to avoid shortcomings of
the limited receive-queue. I don't think any solution involving the
receive-queue is going to work out. Even if we always make sure the
receive-queue is big enough, we might still get new events coming in
before user-space can drain it.
How about this:
struct input_resync {
__u64 count;
struct input_event buffer[];
};
EVIOCSYNC(struct input_resync) copies the current state of all
available event-types of the device into the buffer. It returns the
number of events that it wants to write (so user-space can pass
count==0 the first time to figure out the required buffer-size). The
ioctl then flushed the _whole_ input queue.
Note that even if we used the client's receive buffer, we'd have to
copy huge amounts of events while holding the event lock. So both
ideas have to lock the buffers during the operation. The downside of
my solution is that we have to allocate temporary buffers as we cannot
copy to user-space while holding spin-locks. However, my code already
makes sure we don't do high-order allocations, so I think this is
fine.
This ioctl would also allow us to get rid of all the other QUERY
ioctls. User-space can use it to initialize it's state and then update
it according to incoming events. This way, we simplify the API
considerably! I really like this idea. I know, we only need this to
handle corner-cases. But as Peter already said, it's really easy to
trigger and ignoring it causes awful bugs in user-space.
Comments?
David
^ permalink raw reply
* [PATCH] Handle spurious backslash key repeats on some keyboards
From: Fredrik Hallenberg @ 2014-08-10 9:56 UTC (permalink / raw)
To: Jiri Kosina, linux-input; +Cc: Fredrik Hallenberg
Here is my attempt on a fix for bug 70181, please review it. It is
tested on my nordic Corsair K70, if this solution is deemed acceptable
I can ask some people with other problematic keyboards to test it.
Signed-off-by: Fredrik Hallenberg <megahallon@gmail.com>
---
drivers/hid/hid-input.c | 14 ++++++++++++++
include/linux/hid.h | 1 +
2 files changed, 15 insertions(+)
diff --git a/drivers/hid/hid-input.c b/drivers/hid/hid-input.c
index 2619f7f..56429c0 100644
--- a/drivers/hid/hid-input.c
+++ b/drivers/hid/hid-input.c
@@ -1085,6 +1085,20 @@ void hidinput_hid_event(struct hid_device *hid, struct hid_field *field, struct
return;
}
+ /*
+ * Some keyboards will report both HID keys 0x31 (\ and |) and
+ * 0x32 (Non-US # and ~) which are both mapped to
+ * KEY_BACKSLASH. This will cause spurious events causing
+ * repeated backslash key presses. Handle this by tracking the
+ * active HID code and ignoring the other one.
+ */
+ if (usage->type == EV_KEY && usage->code == KEY_BACKSLASH) {
+ if (value)
+ field->hidinput->backslash_usage = usage->hid;
+ else if (field->hidinput->backslash_usage != usage->hid)
+ return;
+ }
+
/* report the usage code as scancode if the key status has changed */
if (usage->type == EV_KEY && !!test_bit(usage->code, input->key) != value)
input_event(input, EV_MSC, MSC_SCAN, usage->hid);
diff --git a/include/linux/hid.h b/include/linux/hid.h
index f53c4a9..1c59f8f 100644
--- a/include/linux/hid.h
+++ b/include/linux/hid.h
@@ -448,6 +448,7 @@ struct hid_input {
struct list_head list;
struct hid_report *report;
struct input_dev *input;
+ unsigned backslash_usage;
};
enum hid_type {
--
2.1.0.rc1
^ permalink raw reply related
* Re: [PATCH] Input: wacom - Fix compiler warning if !CONFIG_PM
From: Jiri Kosina @ 2014-08-09 20:39 UTC (permalink / raw)
To: Geert Uytterhoeven, Dmitry Torokhov
Cc: linux-input, linux-kernel, Benjamin Tissoires
In-Reply-To: <1407604023-7429-1-git-send-email-geert@linux-m68k.org>
On Sat, 9 Aug 2014, Geert Uytterhoeven wrote:
> If CONFIG_PM is not set:
>
> drivers/hid/wacom_sys.c:1436: warning: ‘wacom_reset_resume’ defined but
> not used
>
> Protect the unused functions by #ifdef CONFIG_PM to fix this.
>
> Signed-off-by: Geert Uytterhoeven <geert@linux-m68k.org>
Thanks Geert.
hid-wacom is in a sort of temporary mode of operation, being moved from
Input to HID driver.
Adding Benjamin and Dmitry to CC, as this will likely still go through
Dmitry's queue.
> ---
> drivers/hid/wacom_sys.c | 2 ++
> 1 file changed, 2 insertions(+)
>
> diff --git a/drivers/hid/wacom_sys.c b/drivers/hid/wacom_sys.c
> index 3e388ec31da8..f0db7eca9023 100644
> --- a/drivers/hid/wacom_sys.c
> +++ b/drivers/hid/wacom_sys.c
> @@ -1416,6 +1416,7 @@ static void wacom_remove(struct hid_device *hdev)
> kfree(wacom);
> }
>
> +#ifdef CONFIG_PM
> static int wacom_resume(struct hid_device *hdev)
> {
> struct wacom *wacom = hid_get_drvdata(hdev);
> @@ -1436,6 +1437,7 @@ static int wacom_reset_resume(struct hid_device *hdev)
> {
> return wacom_resume(hdev);
> }
> +#endif /* CONFIG_PM */
>
> static struct hid_driver wacom_driver = {
> .name = "wacom",
> --
> 1.9.1
>
--
Jiri Kosina
SUSE Labs
--
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
* [PATCH] Input: wacom - Fix compiler warning if !CONFIG_PM
From: Geert Uytterhoeven @ 2014-08-09 17:07 UTC (permalink / raw)
To: Jiri Kosina; +Cc: linux-input, linux-kernel, Geert Uytterhoeven
If CONFIG_PM is not set:
drivers/hid/wacom_sys.c:1436: warning: ‘wacom_reset_resume’ defined but
not used
Protect the unused functions by #ifdef CONFIG_PM to fix this.
Signed-off-by: Geert Uytterhoeven <geert@linux-m68k.org>
---
drivers/hid/wacom_sys.c | 2 ++
1 file changed, 2 insertions(+)
diff --git a/drivers/hid/wacom_sys.c b/drivers/hid/wacom_sys.c
index 3e388ec31da8..f0db7eca9023 100644
--- a/drivers/hid/wacom_sys.c
+++ b/drivers/hid/wacom_sys.c
@@ -1416,6 +1416,7 @@ static void wacom_remove(struct hid_device *hdev)
kfree(wacom);
}
+#ifdef CONFIG_PM
static int wacom_resume(struct hid_device *hdev)
{
struct wacom *wacom = hid_get_drvdata(hdev);
@@ -1436,6 +1437,7 @@ static int wacom_reset_resume(struct hid_device *hdev)
{
return wacom_resume(hdev);
}
+#endif /* CONFIG_PM */
static struct hid_driver wacom_driver = {
.name = "wacom",
--
1.9.1
^ permalink raw reply related
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