* Re: [PATCH 2/4] Input: introduce ABS_MAX2/CNT2 and friends
From: Peter Hutterer @ 2013-12-18 23:40 UTC (permalink / raw)
To: David Herrmann
Cc: linux-input, Dmitry Torokhov, Jiri Kosina, Benjamin Tissoires,
Antonio Ospite, linux-kernel, input-tools
In-Reply-To: <1387295334-1744-3-git-send-email-dh.herrmann@gmail.com>
On Tue, Dec 17, 2013 at 04:48:52PM +0100, David Herrmann wrote:
> As we painfully noticed during the 3.12 merge-window our
> EVIOCGABS/EVIOCSABS API is limited to ABS_MAX<=0x3f. We tried several
> hacks to work around it but if we ever decide to increase ABS_MAX, the
> EVIOCSABS ioctl ABI might overflow into the next byte causing horrible
> misinterpretations in the kernel that we cannot catch.
>
> Therefore, we decided to go with ABS_MAX2/CNT2 and introduce two new
> ioctls to get/set abs-params. They no longer encode the ABS code in the
> ioctl number and thus allow up to 4 billion ABS codes.
>
> The new API also allows to query multiple ABS values with one call. To
> allow EVIOCSABS2(code = 0, cnt = ABS_CNT2) we need to silently ignore
> writes to ABS_MT_SLOT. Furthermore, for better compatibility with
> newer user-space, we ignore writes to unknown codes. Hence, if we ever
> increase ABS_MAX2, new user-space will work with code=0,cnt=ABS_CNT2 just
> fine even on old kernels.
>
> Note that we also need to increase EV_VERSION so user-space can reliably
> know whether ABS2 is supported. Unfortunately, we return EINVAL instead of
> ENOSYS for unknown evdev ioctls so it's nearly impossible to catch
> reliably without EVIOCGVERSION.
>
> Signed-off-by: David Herrmann <dh.herrmann@gmail.com>
> ---
> drivers/hid/hid-debug.c | 2 +-
> drivers/hid/hid-input.c | 2 +-
> drivers/input/evdev.c | 95 +++++++++++++++++++++++++++++++-
> drivers/input/input.c | 14 ++---
> drivers/input/keyboard/goldfish_events.c | 6 +-
> drivers/input/keyboard/hil_kbd.c | 2 +-
> drivers/input/misc/uinput.c | 6 +-
> include/linux/hid.h | 2 +-
> include/linux/input.h | 6 +-
> include/uapi/linux/input.h | 42 +++++++++++++-
> include/uapi/linux/uinput.h | 2 +-
> 11 files changed, 155 insertions(+), 24 deletions(-)
>
> diff --git a/drivers/hid/hid-debug.c b/drivers/hid/hid-debug.c
> index 8453214..d32fa30 100644
> --- a/drivers/hid/hid-debug.c
> +++ b/drivers/hid/hid-debug.c
> @@ -862,7 +862,7 @@ static const char *relatives[REL_MAX + 1] = {
> [REL_WHEEL] = "Wheel", [REL_MISC] = "Misc",
> };
>
> -static const char *absolutes[ABS_CNT] = {
> +static const char *absolutes[ABS_CNT2] = {
> [ABS_X] = "X", [ABS_Y] = "Y",
> [ABS_Z] = "Z", [ABS_RX] = "Rx",
> [ABS_RY] = "Ry", [ABS_RZ] = "Rz",
> diff --git a/drivers/hid/hid-input.c b/drivers/hid/hid-input.c
> index d97f232..a02721c 100644
> --- a/drivers/hid/hid-input.c
> +++ b/drivers/hid/hid-input.c
> @@ -1300,7 +1300,7 @@ static bool hidinput_has_been_populated(struct hid_input *hidinput)
> for (i = 0; i < BITS_TO_LONGS(REL_CNT); i++)
> r |= hidinput->input->relbit[i];
>
> - for (i = 0; i < BITS_TO_LONGS(ABS_CNT); i++)
> + for (i = 0; i < BITS_TO_LONGS(ABS_CNT2); i++)
> r |= hidinput->input->absbit[i];
>
> for (i = 0; i < BITS_TO_LONGS(MSC_CNT); i++)
> diff --git a/drivers/input/evdev.c b/drivers/input/evdev.c
> index a06e125..32b74e5 100644
> --- a/drivers/input/evdev.c
> +++ b/drivers/input/evdev.c
> @@ -643,7 +643,7 @@ static int handle_eviocgbit(struct input_dev *dev,
> case 0: bits = dev->evbit; len = EV_MAX; break;
> case EV_KEY: bits = dev->keybit; len = KEY_MAX; break;
> case EV_REL: bits = dev->relbit; len = REL_MAX; break;
> - case EV_ABS: bits = dev->absbit; len = ABS_MAX; break;
> + case EV_ABS: bits = dev->absbit; len = ABS_MAX2; break;
> case EV_MSC: bits = dev->mscbit; len = MSC_MAX; break;
> case EV_LED: bits = dev->ledbit; len = LED_MAX; break;
> case EV_SND: bits = dev->sndbit; len = SND_MAX; break;
> @@ -671,6 +671,93 @@ static int handle_eviocgbit(struct input_dev *dev,
> }
> #undef OLD_KEY_MAX
>
> +static int evdev_handle_get_abs2(struct input_dev *dev, void __user *p)
> +{
> + u32 code, cnt, valid_cnt, i;
> + struct input_absinfo2 __user *pinfo = p;
> + struct input_absinfo abs;
> +
> + if (copy_from_user(&code, &pinfo->code, sizeof(code)))
> + return -EFAULT;
> + if (copy_from_user(&cnt, &pinfo->cnt, sizeof(cnt)))
> + return -EFAULT;
> + if (!cnt)
> + return 0;
> +
> + if (!dev->absinfo)
> + valid_cnt = 0;
> + else if (code > ABS_MAX2)
> + valid_cnt = 0;
> + else if (code + cnt <= code || code + cnt > ABS_MAX2)
> + valid_cnt = ABS_MAX2 - code + 1;
> + else
> + valid_cnt = cnt;
> +
> + for (i = 0; i < valid_cnt; ++i) {
> + /*
> + * Take event lock to ensure that we are not
> + * copying data while EVIOCSABS2 changes it.
> + * Might be inconsistent, otherwise.
> + */
> + spin_lock_irq(&dev->event_lock);
> + abs = dev->absinfo[code + i];
> + spin_unlock_irq(&dev->event_lock);
> +
> + if (copy_to_user(&pinfo->info[i], &abs, sizeof(abs)))
> + return -EFAULT;
> + }
> +
> + memset(&abs, 0, sizeof(abs));
> + for (i = valid_cnt; i < cnt; ++i)
> + if (copy_to_user(&pinfo->info[i], &abs, sizeof(abs)))
> + return -EFAULT;
> +
> + return 0;
why don't you return the number of valid copied axes to the user?
that seems better even than forcing the remainder to 0.
> +}
> +
> +static int evdev_handle_set_abs2(struct input_dev *dev, void __user *p)
> +{
> + struct input_absinfo2 __user *pinfo = p;
> + struct input_absinfo *abs;
> + u32 code, cnt, i;
> + size_t size;
> +
> + if (!dev->absinfo)
> + return 0;
> + if (copy_from_user(&code, &pinfo->code, sizeof(code)))
> + return -EFAULT;
> + if (copy_from_user(&cnt, &pinfo->cnt, sizeof(cnt)))
> + return -EFAULT;
> + if (!cnt || code > ABS_MAX2)
> + return 0;
> +
> + if (code + cnt <= code || code + cnt > ABS_MAX2)
> + cnt = ABS_MAX2 - code + 1;
> +
> + size = cnt * sizeof(*abs);
> + abs = memdup_user(pinfo->info, size);
> + if (IS_ERR(abs))
> + return PTR_ERR(abs);
> +
> + /*
> + * Take event lock to ensure that we are not
> + * changing device parameters in the middle
> + * of event.
> + */
> + spin_lock_irq(&dev->event_lock);
> + for (i = 0; i < cnt; ++i) {
> + /* silently drop ABS_MT_SLOT */
> + if (code + i == ABS_MT_SLOT)
> + continue;
> +
> + dev->absinfo[code + i] = abs[i];
> + }
> + spin_unlock_irq(&dev->event_lock);
> +
> + kfree(abs);
> + return 0;
> +}
> +
> static int evdev_handle_get_keycode(struct input_dev *dev, void __user *p)
> {
> struct input_keymap_entry ke = {
> @@ -898,6 +985,12 @@ static long evdev_do_ioctl(struct file *file, unsigned int cmd,
> client->clkid = i;
> return 0;
>
> + case EVIOCGABS2:
> + return evdev_handle_get_abs2(dev, p);
> +
> + case EVIOCSABS2:
> + return evdev_handle_set_abs2(dev, p);
> +
> case EVIOCGKEYCODE:
> return evdev_handle_get_keycode(dev, p);
>
[...]
> diff --git a/drivers/input/misc/uinput.c b/drivers/input/misc/uinput.c
> index 927ad9a..4660ed1 100644
> --- a/drivers/input/misc/uinput.c
> +++ b/drivers/input/misc/uinput.c
> @@ -311,7 +311,7 @@ static int uinput_validate_absbits(struct input_dev *dev)
> unsigned int cnt;
> int retval = 0;
>
> - for (cnt = 0; cnt < ABS_CNT; cnt++) {
> + for (cnt = 0; cnt < ABS_CNT2; cnt++) {
> int min, max;
> if (!test_bit(cnt, dev->absbit))
> continue;
> @@ -474,7 +474,7 @@ static int uinput_setup_device2(struct uinput_device *udev,
> return -EINVAL;
>
> /* rough check to avoid huge kernel space allocations */
> - max = ABS_CNT * sizeof(*user_dev2->abs) + sizeof(*user_dev2);
> + max = ABS_CNT2 * sizeof(*user_dev2->abs) + sizeof(*user_dev2);
hmm, if you ever up the value of ABS_CNT2 and you don't have it query-able,
userspace won't be able to create a uinput device on a kernel with a smaller
ABS_CNT2. worst of all, the only error you get is EINVAL, which is also
used for invalid axis ranges, etc.
> if (count > max)
> return -EINVAL;
>
> @@ -780,7 +780,7 @@ static long uinput_ioctl_handler(struct file *file, unsigned int cmd,
> break;
>
> case UI_SET_ABSBIT:
> - retval = uinput_set_bit(arg, absbit, ABS_MAX);
> + retval = uinput_set_bit(arg, absbit, ABS_MAX2);
> break;
>
> case UI_SET_MSCBIT:
> diff --git a/include/linux/hid.h b/include/linux/hid.h
> index 31b9d29..c21d8bb 100644
> --- a/include/linux/hid.h
> +++ b/include/linux/hid.h
> @@ -828,7 +828,7 @@ static inline void hid_map_usage(struct hid_input *hidinput,
> switch (type) {
> case EV_ABS:
> *bit = input->absbit;
> - *max = ABS_MAX;
> + *max = ABS_MAX2;
> break;
> case EV_REL:
> *bit = input->relbit;
> diff --git a/include/linux/input.h b/include/linux/input.h
> index 82ce323..c6add6f 100644
> --- a/include/linux/input.h
> +++ b/include/linux/input.h
> @@ -129,7 +129,7 @@ struct input_dev {
> unsigned long evbit[BITS_TO_LONGS(EV_CNT)];
> unsigned long keybit[BITS_TO_LONGS(KEY_CNT)];
> unsigned long relbit[BITS_TO_LONGS(REL_CNT)];
> - unsigned long absbit[BITS_TO_LONGS(ABS_CNT)];
> + unsigned long absbit[BITS_TO_LONGS(ABS_CNT2)];
> unsigned long mscbit[BITS_TO_LONGS(MSC_CNT)];
> unsigned long ledbit[BITS_TO_LONGS(LED_CNT)];
> unsigned long sndbit[BITS_TO_LONGS(SND_CNT)];
> @@ -210,8 +210,8 @@ struct input_dev {
> #error "REL_MAX and INPUT_DEVICE_ID_REL_MAX do not match"
> #endif
>
> -#if ABS_MAX != INPUT_DEVICE_ID_ABS_MAX
> -#error "ABS_MAX and INPUT_DEVICE_ID_ABS_MAX do not match"
> +#if ABS_MAX2 != INPUT_DEVICE_ID_ABS_MAX
> +#error "ABS_MAX2 and INPUT_DEVICE_ID_ABS_MAX do not match"
> #endif
>
> #if MSC_MAX != INPUT_DEVICE_ID_MSC_MAX
> diff --git a/include/uapi/linux/input.h b/include/uapi/linux/input.h
> index bd24470..1856461 100644
> --- a/include/uapi/linux/input.h
> +++ b/include/uapi/linux/input.h
> @@ -32,7 +32,7 @@ struct input_event {
> * Protocol version.
> */
>
> -#define EV_VERSION 0x010001
> +#define EV_VERSION 0x010002
A history in the comments would be nice. something like
/**
* 0x010000: original version
* 0x010001: support for long scancodes
* 0x010002: added ABS_CNT2/ABS_MAX2, EVIOCSABS2, EVIOCGABS2
*/
> /*
> * IOCTLs (0x00 - 0x7f)
> @@ -74,6 +74,30 @@ struct input_absinfo {
> };
>
> /**
> + * struct input_absinfo2 - used by EVIOC[G/S]ABS2 ioctls
please spell the two out (at least once in this paragraph), it makes it grep-able.
> + * @code: First ABS code to query
> + * @cnt: Number of ABS codes to query starting at @code
> + * @info: #@cnt absinfo structures to get/set abs parameters for all codes
> + *
> + * This structure is used by the new EVIOC[G/S]ABS2 ioctls which
> + * do the same as the old EVIOC[G/S]ABS ioctls but avoid encoding
> + * the ABS code in the ioctl number. This allows a much wider
> + * range of ABS codes. Furthermore, it allows to query multiple codes with a
> + * single call.
"new" and "old" have a tendency to become "old" and "before old" quickly,
just skip both. A simple "use of EVIOCGABS/EVIOCSABS is discouraged except on kernels
without EVIOCGABS2/EVIOCSABS2 support" is enough to signal which one is preferred.
> + *
> + * Note that this silently drops any requests to set ABS_MT_SLOT. Hence, it is
> + * allowed to call this with code=0 cnt=ABS_CNT2. Furthermore, retrieving
> + * invalid codes returns all 0, setting them does nothing. So you must check
> + * with EVIOCGBIT first if you want reliable results. This behavior is needed
> + * to allow forward compatibility to new ABS codes.
I think this needs rewording, I was quite confused reading this. How about:
"For axes not present on the device and for axes exceeding the kernel's
built-in ABS_CNT2 maximum, EVIOCGABS2 sets all values in the struct absinfo
to 0. EVIOCGSABS2 silently ignores write requests to these axes.
ABS_MT_SlOT is an immutable axis and cannot be modified by EVIOCSABS2,
the respective value is silently ignored."
also, please document the return value of the ioctl.
Cheers,
Peter
> + */
> +struct input_absinfo2 {
> + __u32 code;
> + __u32 cnt;
> + struct input_absinfo info[1];
> +};
> +
> +/**
> * struct input_keymap_entry - used by EVIOCGKEYCODE/EVIOCSKEYCODE ioctls
> * @scancode: scancode represented in machine-endian form.
> * @len: length of the scancode that resides in @scancode buffer.
> @@ -153,6 +177,8 @@ struct input_keymap_entry {
>
> #define EVIOCGRAB _IOW('E', 0x90, int) /* Grab/Release device */
> #define EVIOCREVOKE _IOW('E', 0x91, int) /* Revoke device access */
> +#define EVIOCGABS2 _IOR('E', 0x92, struct input_absinfo2) /* get abs value/limits */
> +#define EVIOCSABS2 _IOW('E', 0x93, struct input_absinfo2) /* set abs value/limits */
>
> #define EVIOCSCLOCKID _IOW('E', 0xa0, int) /* Set clockid to be used for timestamps */
>
> @@ -835,11 +861,23 @@ struct input_keymap_entry {
> #define ABS_MT_TOOL_X 0x3c /* Center X tool position */
> #define ABS_MT_TOOL_Y 0x3d /* Center Y tool position */
>
> -
> +/*
> + * ABS_MAX/CNT is limited to a maximum of 0x3f due to the design of EVIOCGABS
> + * and EVIOCSABS ioctls. Other kernel APIs like uinput also hardcoded it. Do
> + * not modify this value and instead use the extended ABS_MAX2/CNT2 API.
> + */
> #define ABS_MAX 0x3f
> #define ABS_CNT (ABS_MAX+1)
>
> /*
> + * Due to API restrictions the legacy evdev API only supports ABS values up to
> + * ABS_MAX/CNT. Use the extended *ABS2 ioctls to operate on any ABS values in
> + * between ABS_MAX and ABS_MAX2.
> + */
> +#define ABS_MAX2 0x3f
> +#define ABS_CNT2 (ABS_MAX2+1)
> +
> +/*
> * Switch events
> */
>
> diff --git a/include/uapi/linux/uinput.h b/include/uapi/linux/uinput.h
> index c2e8710..27ee521 100644
> --- a/include/uapi/linux/uinput.h
> +++ b/include/uapi/linux/uinput.h
> @@ -140,7 +140,7 @@ struct uinput_user_dev2 {
> char name[UINPUT_MAX_NAME_SIZE];
> struct input_id id;
> __u32 ff_effects_max;
> - struct input_absinfo abs[ABS_CNT];
> + struct input_absinfo abs[ABS_CNT2];
> };
>
> #endif /* _UAPI__UINPUT_H_ */
> --
> 1.8.5.1
>
^ permalink raw reply
* Re: [PATCH 2/4] Input: introduce ABS_MAX2/CNT2 and friends
From: Dmitry Torokhov @ 2013-12-18 23:48 UTC (permalink / raw)
To: Peter Hutterer
Cc: David Herrmann, linux-input, Jiri Kosina, Benjamin Tissoires,
Antonio Ospite, linux-kernel, input-tools
In-Reply-To: <20131218234009.GA9360@yabbi.redhat.com>
On Thursday, December 19, 2013 09:40:09 AM Peter Hutterer wrote:
> > + memset(&abs, 0, sizeof(abs));
> > + for (i = valid_cnt; i < cnt; ++i)
> > + if (copy_to_user(&pinfo->info[i], &abs, sizeof(abs)))
> > + return -EFAULT;
> > +
> > + return 0;
>
> why don't you return the number of valid copied axes to the user?
> that seems better even than forcing the remainder to 0.
Well, if your program messed up buffers that it faulted we do not know
for sure if data that did not cause fault ended up where it should have
or if it smashed something else. This condition I think should be
signaled early.
--
Dmitry
^ permalink raw reply
* Re: [PATCH 2/4] Input: introduce ABS_MAX2/CNT2 and friends
From: Peter Hutterer @ 2013-12-18 23:55 UTC (permalink / raw)
To: Dmitry Torokhov
Cc: David Herrmann, linux-input, Jiri Kosina, Benjamin Tissoires,
Antonio Ospite, linux-kernel, input-tools
In-Reply-To: <1950536.BXYYX85NB4@dtor-d630.eng.vmware.com>
On Wed, Dec 18, 2013 at 03:48:37PM -0800, Dmitry Torokhov wrote:
> On Thursday, December 19, 2013 09:40:09 AM Peter Hutterer wrote:
> > > + memset(&abs, 0, sizeof(abs));
> > > + for (i = valid_cnt; i < cnt; ++i)
> > > + if (copy_to_user(&pinfo->info[i], &abs, sizeof(abs)))
> > > + return -EFAULT;
> > > +
> > > + return 0;
> >
> > why don't you return the number of valid copied axes to the user?
> > that seems better even than forcing the remainder to 0.
>
> Well, if your program messed up buffers that it faulted we do not know
> for sure if data that did not cause fault ended up where it should have
> or if it smashed something else. This condition I think should be
> signaled early.
not 100% sure I understand but I wasn't proposing to remove the -EFAULT, i
was proposing to replace "return 0" with "return valid_cnt".
Cheers,
Peter
^ permalink raw reply
* Re: [PATCH 2/4] Input: introduce ABS_MAX2/CNT2 and friends
From: Dmitry Torokhov @ 2013-12-19 0:05 UTC (permalink / raw)
To: Peter Hutterer
Cc: David Herrmann, linux-input, Jiri Kosina, Benjamin Tissoires,
Antonio Ospite, linux-kernel, input-tools
In-Reply-To: <20131218235504.GA17958@yabbi.redhat.com>
On Thu, Dec 19, 2013 at 09:55:04AM +1000, Peter Hutterer wrote:
> On Wed, Dec 18, 2013 at 03:48:37PM -0800, Dmitry Torokhov wrote:
> > On Thursday, December 19, 2013 09:40:09 AM Peter Hutterer wrote:
> > > > + memset(&abs, 0, sizeof(abs));
> > > > + for (i = valid_cnt; i < cnt; ++i)
> > > > + if (copy_to_user(&pinfo->info[i], &abs, sizeof(abs)))
> > > > + return -EFAULT;
> > > > +
> > > > + return 0;
> > >
> > > why don't you return the number of valid copied axes to the user?
> > > that seems better even than forcing the remainder to 0.
> >
> > Well, if your program messed up buffers that it faulted we do not know
> > for sure if data that did not cause fault ended up where it should have
> > or if it smashed something else. This condition I think should be
> > signaled early.
>
> not 100% sure I understand but I wasn't proposing to remove the -EFAULT, i
> was proposing to replace "return 0" with "return valid_cnt".
I understand what you were saying. Now consider: your program supplied
buffer that is actually smaller than what it said to the kernel.
Depending on the exact placement we may or may not fault when we get
pass the buffer boundary, most likely not. We are likely to fault when
we go way past the buffer boundary and wracked process' memory. If we
return -EFAULT the program will at least notice that something wrong. If
we return count it will try to resubmit the remainder of operation and
not even know that there was something very bad happening.
IOW we should not treat fault condition as other partial read/write
conditions.
Thanks.
--
Dmitry
^ permalink raw reply
* Re: [PATCH 2/4] Input: introduce ABS_MAX2/CNT2 and friends
From: Peter Hutterer @ 2013-12-19 0:25 UTC (permalink / raw)
To: Dmitry Torokhov
Cc: David Herrmann, linux-input, Jiri Kosina, Benjamin Tissoires,
Antonio Ospite, linux-kernel, input-tools
In-Reply-To: <20131219000537.GA838@core.coreip.homeip.net>
On Wed, Dec 18, 2013 at 04:05:37PM -0800, Dmitry Torokhov wrote:
> On Thu, Dec 19, 2013 at 09:55:04AM +1000, Peter Hutterer wrote:
> > On Wed, Dec 18, 2013 at 03:48:37PM -0800, Dmitry Torokhov wrote:
> > > On Thursday, December 19, 2013 09:40:09 AM Peter Hutterer wrote:
> > > > > + memset(&abs, 0, sizeof(abs));
> > > > > + for (i = valid_cnt; i < cnt; ++i)
> > > > > + if (copy_to_user(&pinfo->info[i], &abs, sizeof(abs)))
> > > > > + return -EFAULT;
> > > > > +
> > > > > + return 0;
> > > >
> > > > why don't you return the number of valid copied axes to the user?
> > > > that seems better even than forcing the remainder to 0.
> > >
> > > Well, if your program messed up buffers that it faulted we do not know
> > > for sure if data that did not cause fault ended up where it should have
> > > or if it smashed something else. This condition I think should be
> > > signaled early.
> >
> > not 100% sure I understand but I wasn't proposing to remove the -EFAULT, i
> > was proposing to replace "return 0" with "return valid_cnt".
>
> I understand what you were saying. Now consider: your program supplied
> buffer that is actually smaller than what it said to the kernel.
> Depending on the exact placement we may or may not fault when we get
> pass the buffer boundary, most likely not. We are likely to fault when
> we go way past the buffer boundary and wracked process' memory. If we
> return -EFAULT the program will at least notice that something wrong. If
> we return count it will try to resubmit the remainder of operation and
> not even know that there was something very bad happening.
>
> IOW we should not treat fault condition as other partial read/write
> conditions.
I'm still not sure we're talking about the same thing :)
let me rephrase: why can't we use the behaviour bits_to_user() provides?
it limits the output to maxlen and returns that value (or -EFAULT), it's
only a small step from that to limit the output to min(maxbit, ABS_CNT2).
Cheers,
Peter
^ permalink raw reply
* Re: [PATCH 2/4] Input: introduce ABS_MAX2/CNT2 and friends
From: Dmitry Torokhov @ 2013-12-19 0:34 UTC (permalink / raw)
To: Peter Hutterer
Cc: David Herrmann, linux-input, Jiri Kosina, Benjamin Tissoires,
Antonio Ospite, linux-kernel, input-tools
In-Reply-To: <20131219002542.GB6315@yabbi.redhat.com>
On Thu, Dec 19, 2013 at 10:25:42AM +1000, Peter Hutterer wrote:
> On Wed, Dec 18, 2013 at 04:05:37PM -0800, Dmitry Torokhov wrote:
> > On Thu, Dec 19, 2013 at 09:55:04AM +1000, Peter Hutterer wrote:
> > > On Wed, Dec 18, 2013 at 03:48:37PM -0800, Dmitry Torokhov wrote:
> > > > On Thursday, December 19, 2013 09:40:09 AM Peter Hutterer wrote:
> > > > > > + memset(&abs, 0, sizeof(abs));
> > > > > > + for (i = valid_cnt; i < cnt; ++i)
> > > > > > + if (copy_to_user(&pinfo->info[i], &abs, sizeof(abs)))
> > > > > > + return -EFAULT;
> > > > > > +
> > > > > > + return 0;
> > > > >
> > > > > why don't you return the number of valid copied axes to the user?
> > > > > that seems better even than forcing the remainder to 0.
> > > >
> > > > Well, if your program messed up buffers that it faulted we do not know
> > > > for sure if data that did not cause fault ended up where it should have
> > > > or if it smashed something else. This condition I think should be
> > > > signaled early.
> > >
> > > not 100% sure I understand but I wasn't proposing to remove the -EFAULT, i
> > > was proposing to replace "return 0" with "return valid_cnt".
> >
> > I understand what you were saying. Now consider: your program supplied
> > buffer that is actually smaller than what it said to the kernel.
> > Depending on the exact placement we may or may not fault when we get
> > pass the buffer boundary, most likely not. We are likely to fault when
> > we go way past the buffer boundary and wracked process' memory. If we
> > return -EFAULT the program will at least notice that something wrong. If
> > we return count it will try to resubmit the remainder of operation and
> > not even know that there was something very bad happening.
> >
> > IOW we should not treat fault condition as other partial read/write
> > conditions.
>
> I'm still not sure we're talking about the same thing :)
Hmm, it appears you are right ;)
> let me rephrase: why can't we use the behaviour bits_to_user() provides?
> it limits the output to maxlen and returns that value (or -EFAULT), it's
> only a small step from that to limit the output to min(maxbit, ABS_CNT2).
OK, makes sense.
--
Dmitry
^ permalink raw reply
* Re: [PATCH V2] input synaptics-rmi4: Bug fixes to ATTN GPIO handling.
From: Christopher Heiny @ 2013-12-19 1:23 UTC (permalink / raw)
To: Dmitry Torokhov
Cc: Linux Input, Andrew Duggan, Vincent Huang, Vivian Ly,
Daniel Rosenberg, Jean Delvare, Joerie de Gram, Linus Walleij,
Benjamin Tissoires
In-Reply-To: <20131218143944.GG28504@core.coreip.homeip.net>
On 12/18/2013 06:39 AM, Dmitry Torokhov wrote:
> Hi Chris,
>
> On Tue, Dec 17, 2013 at 12:16:01PM -0800, Christopher Heiny wrote:
>> This patch fixes two bugs in handling of the RMI4 attention line GPIO.
>>
>> 1) in enable_sensor(), make sure the attn_gpio is defined before attempting to
>> get its value.
>>
>> 2) in rmi_driver_probe(), declare the name of the attn_gpio, then
>> request the attn_gpio before attempting to export it.
>>
>> Also introduces a GPIO_LABEL constant for identifying the attention GPIO.
>>
>
> I was looking at the patch some more and I have some concerns with it.
>
>> Signed-off-by: Christopher Heiny <cheiny@synaptics.com>
>> Cc: Dmitry Torokhov <dmitry.torokhov@gmail.com>
>> Cc: Benjamin Tissoires <benjamin.tissoires@redhat.com>
>>
>> ---
>>
>> drivers/input/rmi4/rmi_driver.c | 28 +++++++++++++++++-----------
>> 1 file changed, 17 insertions(+), 11 deletions(-)
>>
>> diff --git a/drivers/input/rmi4/rmi_driver.c b/drivers/input/rmi4/rmi_driver.c
>> index a30c7d3..33fb8f8 100644
>> --- a/drivers/input/rmi4/rmi_driver.c
>> +++ b/drivers/input/rmi4/rmi_driver.c
>> @@ -169,7 +169,7 @@ static int enable_sensor(struct rmi_device *rmi_dev)
>>
>> data->enabled = true;
>>
>> - if (!pdata->level_triggered &&
>> + if (pdata->attn_gpio && !pdata->level_triggered &&
>
> O is perfectly fine GPIO number, you want to use gpio_is_valid() hete. I
> also wonder why do you need such elaborate check. Can we simply "flush"
> device before enabling interrupts?
Hmmm. gpio_is_valid() is a good suggestion. However, I think we can do
away with the whole check on the ATTN gpio, and just call
process_interrupt_requests(). That will both flush the state and handle
any important pending events. In the typical use case for
enable_sensor(), the RMI4 device will either be just coming up or else
coming out of a diagnostic mode, and there will at least be a status
event to handle. In the off-case where there is nothing pending (that
is, ATTN not asserted), the overhead is pretty low - just a quick read
of the interrupt status register.
>
>> gpio_get_value(pdata->attn_gpio) == pdata->attn_polarity)
>> retval = process_interrupt_requests(rmi_dev);
>>
>> @@ -807,6 +807,8 @@ static int rmi_driver_remove(struct device *dev)
>> return 0;
>> }
>>
>> +static const char GPIO_LABEL[] = "attn";
>> +
>> static int rmi_driver_probe(struct device *dev)
>> {
>> struct rmi_driver *rmi_driver;
>> @@ -959,20 +961,24 @@ static int rmi_driver_probe(struct device *dev)
>> }
>>
>> if (IS_ENABLED(CONFIG_RMI4_DEV) && pdata->attn_gpio) {
>> - retval = gpio_export(pdata->attn_gpio, false);
>> + retval = gpio_request(pdata->attn_gpio, GPIO_LABEL);
>
> Here it is too late to request GPIO. You have been converting it to IRQ,
> enabling that IRQ and calling gpio_get_value() so GPIO should have
> already been requested by now.
>
> So you need to move this code up.
I'll give that a try.
> You may also consider using
> gpio_request_one() and use GPIOF_EXPORT flag if you want to export it.
> It would also be nice to set the direction (GPIOF_DIR_IN).
Both of these are good ideas.
> I also do not see matching call to gpio_free() in remove().
Neither do I :-(.
We'll update.
>
>> if (retval) {
>> - dev_warn(dev, "WARNING: Failed to export ATTN gpio!\n");
>> - retval = 0;
>> + dev_warn(dev, "WARNING: Failed to request ATTN gpio %d, code: %d.\n",
>> + pdata->attn_gpio, retval);
>> } else {
>> - retval = gpio_export_link(dev,
>> - "attn", pdata->attn_gpio);
>> + retval = gpio_export(pdata->attn_gpio, false);
>> if (retval) {
>> - dev_warn(dev,
>> - "WARNING: Failed to symlink ATTN gpio!\n");
>> - retval = 0;
>> + dev_warn(dev, "WARNING: Failed to export ATTN gpio %d, code: %d.\n",
>> + pdata->attn_gpio, retval);
>> } else {
>> - dev_info(dev, "Exported ATTN GPIO %d.",
>> - pdata->attn_gpio);
>> + retval = gpio_export_link(dev, GPIO_LABEL,
>> + pdata->attn_gpio);
>> + if (retval)
>> + dev_warn(dev,
>> + "WARNING: Failed to symlink ATTN gpio!\n");
>> + else
>> + dev_info(dev, "Exported ATTN GPIO %d.",
>> + pdata->attn_gpio);
>> }
>> }
>> }
>
> Thanks.
>
^ permalink raw reply
* Re: [PATCH 7/9] Input: pixcir_i2c_ts: Implement Type B Multi Touch reporting
From: Roger Quadros @ 2013-12-19 5:49 UTC (permalink / raw)
To: Dmitry Torokhov
Cc: rydberg-Hk7bIW8heu4wFerOooGFRg, jcbian-mY6CKx1T+M6Pt1CcHtbs0g,
linux-input-u79uwXL29TY76Z2rM5mHXA,
linux-kernel-u79uwXL29TY76Z2rM5mHXA,
devicetree-u79uwXL29TY76Z2rM5mHXA
In-Reply-To: <20131218141841.GE28504-WlK9ik9hQGAhIp7JRqBPierSzoNAToWh@public.gmane.org>
Hi Dmitry,
On 12/18/2013 07:48 PM, Dmitry Torokhov wrote:
> On Wed, Dec 18, 2013 at 02:51:18PM +0530, Roger Quadros wrote:
>> Some pixcir controllers e.g. tangoC family report finger IDs with
>> the co-ordinates and are more suitable for Type-B MT protocol.
>>
>> Signed-off-by: Roger Quadros <rogerq-l0cyMroinI0@public.gmane.org>
>> Acked-by: Mugunthan V N <mugunthanvnm-l0cyMroinI0@public.gmane.org>
>> ---
>> drivers/input/touchscreen/pixcir_i2c_ts.c | 202 +++++++++++++++++++++++-------
>> 1 file changed, 155 insertions(+), 47 deletions(-)
>>
>> diff --git a/drivers/input/touchscreen/pixcir_i2c_ts.c b/drivers/input/touchscreen/pixcir_i2c_ts.c
>> index ff68246..9e14415 100644
>> --- a/drivers/input/touchscreen/pixcir_i2c_ts.c
>> +++ b/drivers/input/touchscreen/pixcir_i2c_ts.c
>> @@ -23,84 +23,173 @@
>> #include <linux/slab.h>
>> #include <linux/i2c.h>
>> #include <linux/input.h>
>> +#include <linux/input/mt.h>
>> #include <linux/input/pixcir_ts.h>
>> #include <linux/gpio.h>
>> #include <linux/of.h>
>> #include <linux/of_gpio.h>
>> #include <linux/of_device.h>
>>
>> +#define MAX_FINGERS 5 /* Maximum supported by the driver */
>> +
>> struct pixcir_i2c_ts_data {
>> struct i2c_client *client;
>> struct input_dev *input;
>> const struct pixcir_ts_platform_data *pdata;
>> bool exiting;
>> + u8 max_fingers; /* Maximum supported by the chip */
>> };
>>
>> -static void pixcir_ts_poscheck(struct pixcir_i2c_ts_data *data)
>> +static void pixcir_ts_typea_report(struct pixcir_i2c_ts_data *tsdata)
>
> Hmm, I do not think we should keep Type A reports if we can do Type B.
> The protocols are not new and userspace should be able to handle MT-B by
> now.
>
It seems the controller the original driver was written for does not report
touch ID and just reports 2 touch co-ordinates. I'm not sure how this fits into
Type B reporting model.
cheers,
-roger
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
^ permalink raw reply
* Re: [PATCH 5/9] Input: pixcir_i2c_ts: Get rid of pdata->attb_read_val()
From: Roger Quadros @ 2013-12-19 5:54 UTC (permalink / raw)
To: Dmitry Torokhov; +Cc: rydberg, jcbian, linux-input, linux-kernel, devicetree
In-Reply-To: <20131218142056.GF28504@core.coreip.homeip.net>
On 12/18/2013 07:50 PM, Dmitry Torokhov wrote:
> On Wed, Dec 18, 2013 at 02:51:16PM +0530, Roger Quadros wrote:
>> Get rid of the attb_read_val() platform hook. Instead,
>> read the ATTB gpio directly from the driver.
>>
>> Fail if valid ATTB gpio is not provided by patform data.
>
> Do you also need to define polarity?
>
In patch 3, in pixcir_start(), we explicitly configure the GPIO output
polarity as /* LEVEL_TOUCH interrupt with active low polarity */
So I don't think we need to define polarity.
cheers,
-roger
>>
>> Signed-off-by: Roger Quadros <rogerq@ti.com>
>> Acked-by: Mugunthan V N <mugunthanvnm@ti.com>
>> ---
>> drivers/input/touchscreen/pixcir_i2c_ts.c | 19 +++++++++++++++++--
>> include/linux/input/pixcir_ts.h | 1 -
>> 2 files changed, 17 insertions(+), 3 deletions(-)
>>
>> diff --git a/drivers/input/touchscreen/pixcir_i2c_ts.c b/drivers/input/touchscreen/pixcir_i2c_ts.c
>> index 3370fd9..a783d94 100644
>> --- a/drivers/input/touchscreen/pixcir_i2c_ts.c
>> +++ b/drivers/input/touchscreen/pixcir_i2c_ts.c
>> @@ -91,11 +91,12 @@ static void pixcir_ts_poscheck(struct pixcir_i2c_ts_data *data)
>> static irqreturn_t pixcir_ts_isr(int irq, void *dev_id)
>> {
>> struct pixcir_i2c_ts_data *tsdata = dev_id;
>> + const struct pixcir_ts_platform_data *pdata = tsdata->chip;
>>
>> while (!tsdata->exiting) {
>> pixcir_ts_poscheck(tsdata);
>>
>> - if (tsdata->chip->attb_read_val())
>> + if (gpio_get_value(pdata->gpio_attb))
>> break;
>>
>> msleep(20);
>> @@ -301,8 +302,10 @@ static struct pixcir_ts_platform_data *pixcir_parse_dt(struct device *dev)
>> return ERR_PTR(-ENOMEM);
>>
>> pdata->gpio_attb = of_get_named_gpio(np, "attb-gpio", 0);
>> - if (!gpio_is_valid(pdata->gpio_attb))
>> + if (!gpio_is_valid(pdata->gpio_attb)) {
>> dev_err(dev, "Failed to get ATTB GPIO\n");
>> + return ERR_PTR(-EINVAL);
>> + }
>>
>> if (of_property_read_u32(np, "x-size", &pdata->x_size)) {
>> dev_err(dev, "Failed to get x-size property\n");
>> @@ -344,6 +347,11 @@ static int pixcir_i2c_ts_probe(struct i2c_client *client,
>> } else if (!pdata) {
>> dev_err(&client->dev, "platform data not defined\n");
>> return -EINVAL;
>> + } else {
>> + if (!gpio_is_valid(pdata->gpio_attb)) {
>> + dev_err(dev, "Invalid gpio_attb in pdata\n");
>> + return -EINVAL;
>> + }
>> }
>>
>> tsdata = devm_kzalloc(dev, sizeof(*tsdata), GFP_KERNEL);
>> @@ -380,6 +388,13 @@ static int pixcir_i2c_ts_probe(struct i2c_client *client,
>>
>> input_set_drvdata(input, tsdata);
>>
>> + error = devm_gpio_request_one(dev, pdata->gpio_attb,
>> + GPIOF_DIR_IN, "pixcir_i2c_attb");
>> + if (error) {
>> + dev_err(dev, "Failed to request ATTB gpio\n");
>> + return error;
>> + }
>> +
>> error = devm_request_threaded_irq(dev, client->irq, NULL, pixcir_ts_isr,
>> IRQF_TRIGGER_FALLING | IRQF_ONESHOT,
>> client->name, tsdata);
>> diff --git a/include/linux/input/pixcir_ts.h b/include/linux/input/pixcir_ts.h
>> index f17c192..88ffdb50 100644
>> --- a/include/linux/input/pixcir_ts.h
>> +++ b/include/linux/input/pixcir_ts.h
>> @@ -44,7 +44,6 @@ enum pixcir_int_mode {
>> #define PIXCIR_INT_POL_HIGH (1UL << 2)
>>
>> struct pixcir_ts_platform_data {
>> - int (*attb_read_val)(void);
>> unsigned int x_size; /* X axis resolution */
>> unsigned int y_size; /* Y axis resolution */
>> int gpio_attb; /* GPIO connected to ATTB line */
>> --
>> 1.8.3.2
>>
>
^ permalink raw reply
* Re: [PATCH 3/9] Input: pixcir_i2c_ts: Initialize interrupt mode and power mode
From: Roger Quadros @ 2013-12-19 5:57 UTC (permalink / raw)
To: Dmitry Torokhov; +Cc: rydberg, jcbian, linux-input, linux-kernel, devicetree
In-Reply-To: <20131218141408.GC28504@core.coreip.homeip.net>
On 12/18/2013 07:44 PM, Dmitry Torokhov wrote:
> On Wed, Dec 18, 2013 at 02:51:14PM +0530, Roger Quadros wrote:
>> +
>> +static int pixcir_stop(struct pixcir_i2c_ts_data *ts)
>> +{
>> + struct device *dev = &ts->client->dev;
>> + int ret;
>> +
>> + /* disable interrupt generation */
>> + ret = pixcir_int_enable(ts, 0);
>> + if (ret) {
>> + dev_err(dev, "Failed to disable interrupt generation\n");
>> + return ret;
>> + }
>> +
>> + disable_irq(ts->client->irq);
>
> Why do you need to disable IRQ? If you disable interrupt generation in
> the chip I think you only need to call synchronize_irq() to make sure
> it's completed if it happens to be running. Also you need to move the
> code:
Agreed, no need to call disable_irq().
>
> tsdata->exiting = true;
> mb();
>
> here from pixcir_i2c_ts_remove() to make sure handler exits promptly.
>
> You will also need to reset tsdata->exiting in your start method.
>
OK.
cheers,
-roger
^ permalink raw reply
* Re: [PATCH 1/9] Input: pixcir_i2c_ts: Add device tree support
From: Roger Quadros @ 2013-12-19 6:12 UTC (permalink / raw)
To: Dmitry Torokhov; +Cc: rydberg, jcbian, linux-input, linux-kernel, devicetree
In-Reply-To: <20131218140918.GA28504@core.coreip.homeip.net>
On 12/18/2013 07:39 PM, Dmitry Torokhov wrote:
> Hi Roger,
>
> On Wed, Dec 18, 2013 at 02:51:12PM +0530, Roger Quadros wrote:
>> Provide device tree support and binding information.
>> Change platform data parameters from x/y_max to x/y_size..
>
> I'd rather keep them as they were.
OK.
>
>>
>> Signed-off-by: Roger Quadros <rogerq@ti.com>
>> Acked-by: Mugunthan V N <mugunthanvnm@ti.com>
>> ---
>> .../bindings/input/touchscreen/pixcir_i2c_ts.txt | 26 ++++++++
>> drivers/input/touchscreen/pixcir_i2c_ts.c | 77 ++++++++++++++++++++--
>> include/linux/input/pixcir_ts.h | 5 +-
>> 3 files changed, 101 insertions(+), 7 deletions(-)
>> create mode 100644 Documentation/devicetree/bindings/input/touchscreen/pixcir_i2c_ts.txt
>>
>> diff --git a/Documentation/devicetree/bindings/input/touchscreen/pixcir_i2c_ts.txt b/Documentation/devicetree/bindings/input/touchscreen/pixcir_i2c_ts.txt
>> new file mode 100644
>> index 0000000..c0b0b270
>> --- /dev/null
>> +++ b/Documentation/devicetree/bindings/input/touchscreen/pixcir_i2c_ts.txt
>> @@ -0,0 +1,26 @@
>> +* Pixcir I2C touchscreen controllers
>> +
>> +Required properties:
>> +- compatible: must be "pixcir,pixcir_ts"
>> +- reg: I2C address of the chip
>> +- interrupts: interrupt to which the chip is connected
>> +- attb-gpio: GPIO connected to the ATTB line of the chip
>> +- x-size: horizontal resolution of touchscreen
>> +- y-size: vertical resolution of touchscreen
>> +
>> +Example:
>> +
>> + i2c@00000000 {
>> + /* ... */
>> +
>> + pixcir_ts@5c {
>> + compatible = "pixcir,pixcir_ts";
>> + reg = <0x5c>;
>> + interrupts = <2 0>;
>> + attb-gpio = <&gpf 2 0 2>;
>> + x-size = <800>;
>> + y-size = <600>;
>> + };
>> +
>> + /* ... */
>> + };
>> diff --git a/drivers/input/touchscreen/pixcir_i2c_ts.c b/drivers/input/touchscreen/pixcir_i2c_ts.c
>> index 6cc6b36..3a447c9 100644
>> --- a/drivers/input/touchscreen/pixcir_i2c_ts.c
>> +++ b/drivers/input/touchscreen/pixcir_i2c_ts.c
>> @@ -24,6 +24,10 @@
>> #include <linux/i2c.h>
>> #include <linux/input.h>
>> #include <linux/input/pixcir_ts.h>
>> +#include <linux/gpio.h>
>> +#include <linux/of.h>
>> +#include <linux/of_gpio.h>
>> +#include <linux/of_device.h>
>>
>> struct pixcir_i2c_ts_data {
>> struct i2c_client *client;
>> @@ -125,15 +129,65 @@ static int pixcir_i2c_ts_resume(struct device *dev)
>> static SIMPLE_DEV_PM_OPS(pixcir_dev_pm_ops,
>> pixcir_i2c_ts_suspend, pixcir_i2c_ts_resume);
>>
>> +#if defined(CONFIG_OF)
>
> #ifdef is preferred for simple conditions.
>
>> +static const struct of_device_id pixcir_of_match[];
>> +
>> +static struct pixcir_ts_platform_data *pixcir_parse_dt(struct device *dev)
>> +{
>> + struct pixcir_ts_platform_data *pdata;
>> + struct device_node *np = dev->of_node;
>> + const struct of_device_id *match;
>> +
>> + match = of_match_device(of_match_ptr(pixcir_of_match), dev);
>> + if (!match)
>> + return ERR_PTR(-EINVAL);
>
> Why do you need an explicit match here?
>
Right, it is not needed here for this patch but used later in patch 6
to get chip specific data. I'll fix this while re-arranging the patches.
>> +
>> + pdata = devm_kzalloc(dev, sizeof(*pdata), GFP_KERNEL);
>> + if (!pdata)
>> + return ERR_PTR(-ENOMEM);
>> +
>> + pdata->gpio_attb = of_get_named_gpio(np, "attb-gpio", 0);
>> + if (!gpio_is_valid(pdata->gpio_attb))
>> + dev_err(dev, "Failed to get ATTB GPIO\n");
>> +
>> + if (of_property_read_u32(np, "x-size", &pdata->x_size)) {
>> + dev_err(dev, "Failed to get x-size property\n");
>> + return ERR_PTR(-EINVAL);
>> + }
>> +
>> + if (of_property_read_u32(np, "y-size", &pdata->y_size)) {
>> + dev_err(dev, "Failed to get y-size property\n");
>> + return ERR_PTR(-EINVAL);
>> + }
>> +
>> + dev_dbg(dev, "%s: x %d, y %d, gpio %d\n", __func__,
>> + pdata->x_size, pdata->y_size, pdata->gpio_attb);
>> +
>> + return pdata;
>> +}
>> +#else
>> +static struct pixcir_ts_platform_data *pixcir_parse_dt(struct device *dev)
>> +{
>> + return NULL;
>
> This should be
>
> return ERR_PTR(-EINVAL);
>
> since you test it with IS_ERR() later.
OK.
>
>> +}
>> +#endif
>> +
>> static int pixcir_i2c_ts_probe(struct i2c_client *client,
>> const struct i2c_device_id *id)
>> {
>> const struct pixcir_ts_platform_data *pdata = client->dev.platform_data;
>> + struct device *dev = &client->dev;
>> + struct device_node *np = dev->of_node;
>> struct pixcir_i2c_ts_data *tsdata;
>> struct input_dev *input;
>> int error;
>>
>> - if (!pdata) {
>> + if (np) {
>> + pdata = pixcir_parse_dt(dev);
>> + if (IS_ERR(pdata))
>> + return PTR_ERR(pdata);
>
> We should be favoring kernel-provided platform data if it is pesent even
> if there is dt-data available. This way user can override firmware,m if
> needed.
>
OK. But how can the user override kernel provided platform data? Can't device
tree overlays be used to patch DT data in the same fashion.
I've not tried either so I don't know which one is more user friendly.
>> +
>> + } else if (!pdata) {
>> dev_err(&client->dev, "platform data not defined\n");
>> return -EINVAL;
>> }
>> @@ -157,10 +211,14 @@ static int pixcir_i2c_ts_probe(struct i2c_client *client,
>> __set_bit(EV_KEY, input->evbit);
>> __set_bit(EV_ABS, input->evbit);
>> __set_bit(BTN_TOUCH, input->keybit);
>> - input_set_abs_params(input, ABS_X, 0, pdata->x_max, 0, 0);
>> - input_set_abs_params(input, ABS_Y, 0, pdata->y_max, 0, 0);
>> - input_set_abs_params(input, ABS_MT_POSITION_X, 0, pdata->x_max, 0, 0);
>> - input_set_abs_params(input, ABS_MT_POSITION_Y, 0, pdata->y_max, 0, 0);
>> + input_set_abs_params(input, ABS_X,
>> + 0, pdata->x_size - 1, 0, 0);
>> + input_set_abs_params(input, ABS_Y,
>> + 0, pdata->y_size - 1, 0, 0);
>> + input_set_abs_params(input, ABS_MT_POSITION_X,
>> + 0, pdata->x_size - 1, 0, 0);
>> + input_set_abs_params(input, ABS_MT_POSITION_Y,
>> + 0, pdata->y_size - 1, 0, 0);
>>
>> input_set_drvdata(input, tsdata);
>>
>> @@ -211,11 +269,20 @@ static const struct i2c_device_id pixcir_i2c_ts_id[] = {
>> };
>> MODULE_DEVICE_TABLE(i2c, pixcir_i2c_ts_id);
>>
>> +#if defined(CONFIG_OF)
>> +static const struct of_device_id pixcir_of_match[] = {
>> + { .compatible = "pixcir,pixcir_ts", },
>> + { }
>> +};
>> +MODULE_DEVICE_TABLE(of, pixcir_of_match);
>> +#endif
>
> #ifdef is preferred for simple conditions.
>
OK.
>> +
>> static struct i2c_driver pixcir_i2c_ts_driver = {
>> .driver = {
>> .owner = THIS_MODULE,
>> .name = "pixcir_ts",
>> .pm = &pixcir_dev_pm_ops,
>> + .of_match_table = of_match_ptr(pixcir_of_match),
>> },
>> .probe = pixcir_i2c_ts_probe,
>> .remove = pixcir_i2c_ts_remove,
>> diff --git a/include/linux/input/pixcir_ts.h b/include/linux/input/pixcir_ts.h
>> index 7163d91..b34ff7e 100644
>> --- a/include/linux/input/pixcir_ts.h
>> +++ b/include/linux/input/pixcir_ts.h
>> @@ -3,8 +3,9 @@
>>
>> struct pixcir_ts_platform_data {
>> int (*attb_read_val)(void);
>> - int x_max;
>> - int y_max;
>> + unsigned int x_size; /* X axis resolution */
>> + unsigned int y_size; /* Y axis resolution */
>> + int gpio_attb; /* GPIO connected to ATTB line */
>> };
>
> OK, it looks like the series were split awkwardly: you are defining data
> that is used by follow-up patches and its purpose is unclear when
> reviewing patch-by-patch. I'd recommend rearranging so that DT support
> patch is the very last in the series.
>
Got it.
>>
>> #endif
>> --
>> 1.8.3.2
cheers,
-roger
^ permalink raw reply
* Re: [PATCH 5/5] mfd: input: iio: ti_amm335x: rework TSC/ADC synchronisation
From: Lee Jones @ 2013-12-19 8:42 UTC (permalink / raw)
To: Sebastian Andrzej Siewior
Cc: Jonathan Cameron, Dmitry Torokhov, Samuel Ortiz, Zubair Lutfullah,
Felipe Balbi, linux-iio-u79uwXL29TY76Z2rM5mHXA,
linux-input-u79uwXL29TY76Z2rM5mHXA,
linux-kernel-u79uwXL29TY76Z2rM5mHXA
In-Reply-To: <1387385577-24447-6-git-send-email-bigeasy-hfZtesqFncYOwBW4kG4KsQ@public.gmane.org>
> The ADC driver always programms all possible ADC values and discards
> them except for the value IIO asked for. On the am335x-evm the driver
> programs four values and it takes 500us to gather them. Reducing the number
> of coversations down to (required) one also reduces the busy loop down to
> 125us.
> This leads to another error, namely the FIFOCOUNT register is sometimes
> (like one out of 10 attempts) not updated in time leading to timeouts.
> The next read has the fifocount register updated.
> Checking for the ADCStatus status register for beeing idle isn't a good
> choice either. The problem is that it often times out if the TSC is used
> at the same time. The problem is that the HW compelted the conversaton for
> the ADC *and* before the driver noticed it, the HW began to perfom a
> TSC conversation and so the driver never seen the HW idle. The next time
> we would have two values in the fifo but since the driver reads
> everything we always see the current one.
> So instead of polling for the IDLE bit, we check for the fifocount. It
> should be one instead of zero because we request one value.
> This change in turn lead to another error :) Sometimes if TSC & ADC are
> used together the TSC starts becomming interrupts even if nobody
> actually touched the touchscreen. The interrupts are valid because TSC's
> fifo is filled with values. This condition stops after a few ADC reads but
> will occur once TSC & ADC are used at the same time. So not good.
>
> On top of this (even without the changes I just mentioned) there is a ADC
> & TSC lockup condition which was reported to my Jeff Lance including a test
> case:
> A busyloop of loop of "cat /sys/bus/iio/devices/iio\:device0/in_voltage4_raw"
> and a mug on touch screen. Whith this setup, the hardware will lockup after
> something between 20min and it could take up to a couple of hours. During that
> lockup, the ADCSTAT says 0x30 (or 0x70) which means STEP_ID = IDLE and
> FSM_BUSY = yes. That means it says that it is idle and busy at the same
> time which is an invalid condition.
>
> For all this reasons I decided to rework this TSC/ADC part and add a
> handshake / synchronisation here:
> First the ADC signals that it needs the HW and writes a 0 mask into the
> SE register. The HW (if active) will complete the current conversation
> and become idle. The TSC driver will gather the values from the FIFO
> (woken up by an interrupt) and won't "enable" another conversation.
> Instead it will wake up the ADC driver which is already waiting. The ADC
> driver will start "its" converstation and once it is done, it will
> enable the TSC events so the TSC will work again.
Spell check this entire block.
Smileys in commit messages are generally a bad idea.
Please insert '\n's between paragraphs.
Proof read, as some of the sentences are not comprehensible.
/* MFD Parts */
> --- a/drivers/mfd/ti_am335x_tscadc.c
> +++ b/drivers/mfd/ti_am335x_tscadc.c
> @@ -24,6 +24,7 @@
> -static void am335x_tsc_se_update(struct ti_tscadc_dev *tsadc)
> +void am335x_tsc_se_set(struct ti_tscadc_dev *tsadc, u32 val)
> {
> + unsigned long flags;
> +
> + spin_lock_irqsave(&tsadc->reg_lock, flags);
> + tsadc->reg_se_cache |= val;
> tscadc_writel(tsadc, REG_SE, tsadc->reg_se_cache);
> + spin_unlock_irqrestore(&tsadc->reg_lock, flags);
> }
> +EXPORT_SYMBOL_GPL(am335x_tsc_se_set);
>
> -void am335x_tsc_se_set(struct ti_tscadc_dev *tsadc, u32 val)
> +void am335x_tsc_se_tsc_set(struct ti_tscadc_dev *tsadc, u32 val)
What's the difference between:
am335x_tsc_se_set()
and
am335x_tsc_se_tsc_set()
Why don't you have a am335x_tsc_se_tsc_set_tsc_se_tsc_set() ;)
Perhaps a better function name might be in order.
> {
> unsigned long flags;
>
> spin_lock_irqsave(&tsadc->reg_lock, flags);
> - tsadc->reg_se_cache |= val;
> - am335x_tsc_se_update(tsadc);
> + tsadc->reg_se_cache = val;
> + if (tsadc->adc_in_use || tsadc->adc_waiting) {
> + if (tsadc->adc_waiting)
> + wake_up(&tsadc->reg_se_wait);
So if we're either in-use or waiting, the step mask is never set?
But I guess that the touchscreen driver assumes it has been set.
Is this okay?
> + } else {
> + tscadc_writel(tsadc, REG_SE, val);
> + }
I think this would be better represented as:
if (tsadc->adc_waiting)
wake_up(&tsadc->reg_se_wait);
else if (!tsadc->adc_in_use)
tscadc_writel(tsadc, REG_SE, val);
> spin_unlock_irqrestore(&tsadc->reg_lock, flags);
> }
> -EXPORT_SYMBOL_GPL(am335x_tsc_se_set);
> +EXPORT_SYMBOL_GPL(am335x_tsc_se_tsc_set);
<snip>
--
Lee Jones
Linaro STMicroelectronics Landing Team Lead
Linaro.org │ Open source software for ARM SoCs
Follow Linaro: Facebook | Twitter | Blog
^ permalink raw reply
* Re: am335x: IIO/ADC fixes if used together with TSC
From: Lee Jones @ 2013-12-19 8:48 UTC (permalink / raw)
To: Jonathan Cameron
Cc: Sebastian Andrzej Siewior, Dmitry Torokhov, Samuel Ortiz,
Zubair Lutfullah, Felipe Balbi, linux-iio, linux-input,
linux-kernel
In-Reply-To: <a3513779-5ede-4e33-b29d-0cc8e73f8282@email.android.com>
> >> Given timing I suspect that by the we have reviewed these will be too
> >late in this cycle so will hit in the merge window plus stable.
> >>
> >> Also some of these are cleanups rather than fixes so should normally
> >be separate from fixes and applied after them as they are not
> >> Stable material.
> >
> >I tried to keep 5/5 as small as possible so I factor individual and
> >separate changes as good as possible as long as the drivers remained
> >functional. That means 1-4 are mostly pre-work for 5/5 and that makes
> >5/5 depends on them.
> >
> >Tagging 5/5 stable I would prefer to take 1-4, too to keep the backport
> >effort small. The clash due to the continues mode which is v3.13 is
> >probably small. This said, I think it is the best to backport it stable
> >if someone asks for it (unless the majority here thinks otherwise).
> >
> >Are you saying this is too late for v3.13 or v3.14-rc1?
> >
> >Sebastian
>
> Too late for 3.13. Probably fine for next merge window if reviews are all good or as a
> fix post 3.14-rc1 of it misses the cutoff for the merge window.
I think this set should go in via -next. So it will hit the v3.14
merge window.
--
Lee Jones
Linaro STMicroelectronics Landing Team Lead
Linaro.org │ Open source software for ARM SoCs
Follow Linaro: Facebook | Twitter | Blog
^ permalink raw reply
* Re: [PATCH 1/5] iio: ti_am335x_adc: adjust the closing bracket in tiadc_read_raw()
From: Lee Jones @ 2013-12-19 8:49 UTC (permalink / raw)
To: Sebastian Andrzej Siewior
Cc: Jonathan Cameron, Dmitry Torokhov, Samuel Ortiz, Zubair Lutfullah,
Felipe Balbi, linux-iio, linux-input, linux-kernel
In-Reply-To: <1387385577-24447-2-git-send-email-bigeasy@linutronix.de>
On Wed, 18 Dec 2013, Sebastian Andrzej Siewior wrote:
> It somehow looks like the ending bracket belongs to the if statement but
> it does belong to the while loop. This patch moves the bracket where it
> belongs.
>
> Signed-off-by: Sebastian Andrzej Siewior <bigeasy@linutronix.de>
> ---
> drivers/iio/adc/ti_am335x_adc.c | 2 +-
> 1 file changed, 1 insertion(+), 1 deletion(-)
Reviewed-by: Lee Jones <lee.jones@linaro.org>
--
Lee Jones
Linaro STMicroelectronics Landing Team Lead
Linaro.org │ Open source software for ARM SoCs
Follow Linaro: Facebook | Twitter | Blog
--
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 2/5] mfd: ti_am335x_tscadc: make am335x_tsc_se_update() local
From: Lee Jones @ 2013-12-19 8:53 UTC (permalink / raw)
To: Sebastian Andrzej Siewior
Cc: Jonathan Cameron, Dmitry Torokhov, Samuel Ortiz, Zubair Lutfullah,
Felipe Balbi, linux-iio, linux-input, linux-kernel
In-Reply-To: <1387385577-24447-3-git-send-email-bigeasy@linutronix.de>
> since the "recent" changes, am335x_tsc_se_update() has no longer any
> users outside of this file so make it local.
Please use formatting dictated by the subsystem when providing
patches. For MFD we capitalise the first word after 'mfd: <dev>: ' and
the same for the first word in the commit body.
Patch looks okay, I'd prefer to take the set in one go.
Acked-by: Lee Jones <lee.jones@linaro.org>
--
Lee Jones
Linaro STMicroelectronics Landing Team Lead
Linaro.org │ Open source software for ARM SoCs
Follow Linaro: Facebook | Twitter | Blog
^ permalink raw reply
* Re: [PATCH 3/5] mfd: ti_am335x_tscadc: don't read back REG_SE
From: Lee Jones @ 2013-12-19 8:56 UTC (permalink / raw)
To: Sebastian Andrzej Siewior
Cc: Jonathan Cameron, Dmitry Torokhov, Samuel Ortiz, Zubair Lutfullah,
Felipe Balbi, linux-iio, linux-input, linux-kernel
In-Reply-To: <1387385577-24447-4-git-send-email-bigeasy@linutronix.de>
On Wed, 18 Dec 2013, Sebastian Andrzej Siewior wrote:
> The purpose of reg_se_cache has been defeated. It should avoid the
> read-back of the register to avoid the latency and the fact that the
> bits are reset to 0 after the individual conversation took place.
>
> The reason why this is required like this to work, is that read-back of
> the register removes the bits of the ADC so they do not start another
> conversation after the register is re-written from the TSC side for the
> update.
> To avoid the unrequired read-back I introduce a "set adc" variant which
> does not update the cache mask. After the conversation completes, the
> register bit is removed from the SE reister anyway and we don't plan a
> new conversation "any time soon".
> This is a small preparation for a larger sync-rework.
>
> Signed-off-by: Sebastian Andrzej Siewior <bigeasy@linutronix.de>
> ---
> drivers/iio/adc/ti_am335x_adc.c | 2 +-
> drivers/mfd/ti_am335x_tscadc.c | 12 ++++++++++--
> include/linux/mfd/ti_am335x_tscadc.h | 1 +
> 3 files changed, 12 insertions(+), 3 deletions(-)
Looks about right: Acked-by: Lee Jones <lee.jones@linaro.org>
--
Lee Jones
Linaro STMicroelectronics Landing Team Lead
Linaro.org │ Open source software for ARM SoCs
Follow Linaro: Facebook | Twitter | Blog
^ permalink raw reply
* Re: [PATCH 4/5] mfd: ti_am335x: drop am335x_tsc_se_update() from resume path
From: Lee Jones @ 2013-12-19 9:00 UTC (permalink / raw)
To: Sebastian Andrzej Siewior
Cc: Jonathan Cameron, Dmitry Torokhov, Samuel Ortiz, Zubair Lutfullah,
Felipe Balbi, linux-iio-u79uwXL29TY76Z2rM5mHXA,
linux-input-u79uwXL29TY76Z2rM5mHXA,
linux-kernel-u79uwXL29TY76Z2rM5mHXA
In-Reply-To: <1387385577-24447-5-git-send-email-bigeasy-hfZtesqFncYOwBW4kG4KsQ@public.gmane.org>
> The update of the SE register in MFD doesn't look right as it has
> nothing to do with it. The better place to do it is in TSC driver (which
> is already doint it) and in the ADC driver which needs this only in the
> continues mode.
When you resubmit the patch-set complete with Acks, please ensure you
spell check and proof read _all_ of your commit logs.
> Signed-off-by: Sebastian Andrzej Siewior <bigeasy-hfZtesqFncYOwBW4kG4KsQ@public.gmane.org>
> ---
> drivers/iio/adc/ti_am335x_adc.c | 2 ++
> drivers/mfd/ti_am335x_tscadc.c | 1 -
> 2 files changed, 2 insertions(+), 1 deletion(-)
For the MFD part:
Acked-by: Lee Jones <lee.jones-QSEj5FYQhm4dnm+yROfE0A@public.gmane.org>
It's probably best that we suck these changes up through the MFD tree,
so once the review comments have been satisfied and we have all
relevant Acks I can take them in and create an immutable branch for
you guys to pull from if you like.
--
Lee Jones
Linaro STMicroelectronics Landing Team Lead
Linaro.org │ Open source software for ARM SoCs
Follow Linaro: Facebook | Twitter | Blog
^ permalink raw reply
* Re: Xpad Driver Replacement
From: David Herrmann @ 2013-12-19 9:24 UTC (permalink / raw)
To: Zachary Lund; +Cc: open list:HID CORE LAYER
In-Reply-To: <52A68A23.7050707@computerquip.com>
Hi
On Tue, Dec 10, 2013 at 4:27 AM, Zachary Lund <admin@computerquip.com> wrote:
>
> On 12/08/13 00:18, Zachary Lund wrote:
>>
>> Secondly, the Xbox 360 controllers claim to be HID compliant... this is
>> not an HID driver. That's because the report descriptor is missing and I,
>> unfortunately, do not know what to do about that. Some drivers like XBCD and
>> the driver found at tattiebogle.net both provide their own report descriptor
>> and work from there. While I'd like to do the same eventually, it will take
>> me longer than a week to do that as I'd have to educate myself on HID and
>> figure out what to do about the missing descriptors.
>
> I've run into an instant road block. The controller claims bInterfaceClass
> to be 0xFF (Vendor-specific) so usbhid won't probe it. I didn't think it
> would be so difficult to work around that but I've spent the better part of
> today trying to figure out just that. usbhid is a usb_driver that has only
> one requirement to be probed: bInterfaceClass be 0x03. Unfortunately, the
> device fails this requirement.
>
> Does anyone know of a way around this mechanism? Or perhaps I should take a
> different approach?
Try adding it to:
static const struct usb_device_id hid_usb_ids[]
in drivers/hid/usb-hid/hid-core.c
You should provide a fairly narrow match-line, though. You don't want
to match on all vendor-specific-class devices.
Cheers
David
^ permalink raw reply
* Re: Why does adding HID USB ID allow a keyboard to work?
From: David Herrmann @ 2013-12-19 9:42 UTC (permalink / raw)
To: Reyad Attiyat; +Cc: open list:HID CORE LAYER
In-Reply-To: <CA+BWVUT_v4nQTY7DHfdwegJ8rKOEraodReAkWt4y7Ck93_-PBQ@mail.gmail.com>
Hi
On Sat, Dec 14, 2013 at 8:14 AM, Reyad Attiyat <reyad.attiyat@gmail.com> wrote:
> I have a basic question about the way the linux kernel handles human
> input devices. I bought a Surface Pro 2 with a Type cover keyboard.
> This did not work out of the box with stock linux kernel 3.11. When I
> added the hardware id of the device to the hid-microsoft.c driver it
> worked.
>
> Does this work simply because when I add the hardware id to the
> driver, the kernel knows to use usb-hid generic driver with the
> device? I'm trying to understand the hid-microsoft driver and it seems
> to apply quirks/hacks that enable Microsoft devices to work with the
> hid driver. Such as remapping keys. I don't need any of these quirks
> as all the media keys on the keyboard work all by adding the hardware
> id.
>
> Why doesn't the device work out of the box? Is the device not
> advertising itself as a hid device over usb? I'm little confused as to
> how this should be fixed. Does the kernel fail in noticing this hid
> device or does the device fail in telling the kernel it is a
> keyboard/mouse?
There are lots of HID devices out there that either don't follow the
HID specification or are not covered by the specification. Such
devices cannot be supported out-of-the-box. How should we know how
they work? That's why we provide lots of HID quirks for such devices.
The hid-microsoft driver is one of them. It applies a bunch of fixups
so HID core knows how it works.
We could use some broader heuristics to detect such devices, however,
the conservative approach is the safest. That means, only device-IDs
that have been reported to work are added to the hid-quirk lists. This
way, we don't break well-defined working devices via false positives,
but on the other hand, every new device needs to be added manually.
If you have suggestions how to improve this situation, please provide
patches. But input-development (or generally kernel development) must
be conservative in that regard. A regression is never acceptable and
we'll revert such patches. So someone has yet to come up with better
heuristics or we'll keep our ID lists.
Cheers
David
^ permalink raw reply
* Re: [v3.11][Regression] HID: hyperv: convert alloc+memcpy to memdup
From: David Herrmann @ 2013-12-19 9:55 UTC (permalink / raw)
To: Jiri Kosina
Cc: Joseph Salisbury, Dan Carpenter, thomas, list, Haiyang Zhang,
LKML, open, HID CORE LAYER,
driverdev-devel@linuxdriverproject.org
In-Reply-To: <alpine.LNX.2.00.1312161401100.12882@pobox.suse.cz>
Hi
On Mon, Dec 16, 2013 at 2:01 PM, Jiri Kosina <jkosina@suse.cz> wrote:
> On Fri, 27 Sep 2013, Joseph Salisbury wrote:
>
>> >> commit b1a1442a23776756b254b69786848a94d92445ba
>> >> Author: Jiri Kosina <jkosina@suse.cz>
>> >> Date: Mon Jun 3 11:27:48 2013 +0200
>> >>
>> >> HID: core: fix reporting of raw events
>> >>
>> >> Reverting this commit in v3.12-rc2 prevents the system from locking up,
>> >> which happens when connecting a bluetooth trackpad.
>> >>
>> >> Jiri, do you think we should revert this patch, or is there some further
>> >> debugging/data collecting you would like to do?
>> > Hi Joseph,
>> >
>> > in this mail:
>> >
>> > Message-ID: <5241992E.3090805@canonical.com>
>> > Date: Tue, 24 Sep 2013 09:52:46 -0400
>> >
>> > you said that reverting this commit doesn't prevent the lockups, so I am
>> > rather confused ... ?
>> >
>> > Thanks,
>> >
>> The testing was invalid. Reverting commit b1a1442 does resolve the bug
>> and stop the lockups.
>
> Okay, I finally got some sense of this, sorry for the delay.
>
> Could you please test with the patch below, instead of reverting
> b1a1442a23? Thanks a lot.
>
>
>
> diff --git a/drivers/hid/hid-core.c b/drivers/hid/hid-core.c
> index 253fe23..81eacd3 100644
> --- a/drivers/hid/hid-core.c
> +++ b/drivers/hid/hid-core.c
> @@ -1334,7 +1334,7 @@ int hid_report_raw_event(struct hid_device *hid, int type, u8 *data, int size,
> csize--;
> }
>
> - rsize = ((report->size - 1) >> 3) + 1;
> + rsize = ((report->size - 1) >> 3) + 1 + (report->id > 0) + 7;
Isn't "report->id" already covered by "if (report_enum->numbered)"
above? The test for "id > 0" won't work here as in this case
"report_enum->numbered" must already be set to true by the hid-desc
parser, doesn't it?
Where exactly did you get the +7 from?
What worries me here is that we write over the @data buffer from the
hid-ll-driver if the report is too short. I don't think the BT driver
accounts for that, mhh.
David
> if (rsize > HID_MAX_BUFFER_SIZE)
> rsize = HID_MAX_BUFFER_SIZE;
>
> --
> 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
* Re: [v3.11][Regression] HID: hyperv: convert alloc+memcpy to memdup
From: Jiri Kosina @ 2013-12-19 9:59 UTC (permalink / raw)
To: David Herrmann
Cc: Joseph Salisbury, Dan Carpenter, thomas, list, Haiyang Zhang,
LKML, open, HID CORE LAYER,
driverdev-devel@linuxdriverproject.org
In-Reply-To: <CANq1E4QriATWbfruwj7oVnEBHvtLe_s2yhSSU5COXFjomt5R_A@mail.gmail.com>
On Thu, 19 Dec 2013, David Herrmann wrote:
> > diff --git a/drivers/hid/hid-core.c b/drivers/hid/hid-core.c
> > index 253fe23..81eacd3 100644
> > --- a/drivers/hid/hid-core.c
> > +++ b/drivers/hid/hid-core.c
> > @@ -1334,7 +1334,7 @@ int hid_report_raw_event(struct hid_device *hid, int type, u8 *data, int size,
> > csize--;
> > }
> >
> > - rsize = ((report->size - 1) >> 3) + 1;
> > + rsize = ((report->size - 1) >> 3) + 1 + (report->id > 0) + 7;
>
> Isn't "report->id" already covered by "if (report_enum->numbered)"
> above? The test for "id > 0" won't work here as in this case
> "report_enum->numbered" must already be set to true by the hid-desc
> parser, doesn't it?
Right, that one is not correct here, thanks.
> Where exactly did you get the +7 from?
Please see commit (the one I am not really proud of) 27ce405039bfe6d3.
--
Jiri Kosina
SUSE Labs
^ permalink raw reply
* Re: [v3.11][Regression] HID: hyperv: convert alloc+memcpy to memdup
From: David Herrmann @ 2013-12-19 10:08 UTC (permalink / raw)
To: Jiri Kosina
Cc: list, Joseph Salisbury, Haiyang Zhang, LKML, open, Dan Carpenter,
HID CORE LAYER, driverdev-devel@linuxdriverproject.org, thomas
In-Reply-To: <alpine.LNX.2.00.1312191058100.28730@pobox.suse.cz>
Hi
On Thu, Dec 19, 2013 at 10:59 AM, Jiri Kosina <jkosina@suse.cz> wrote:
> On Thu, 19 Dec 2013, David Herrmann wrote:
>
>> > diff --git a/drivers/hid/hid-core.c b/drivers/hid/hid-core.c
>> > index 253fe23..81eacd3 100644
>> > --- a/drivers/hid/hid-core.c
>> > +++ b/drivers/hid/hid-core.c
>> > @@ -1334,7 +1334,7 @@ int hid_report_raw_event(struct hid_device *hid, int type, u8 *data, int size,
>> > csize--;
>> > }
>> >
>> > - rsize = ((report->size - 1) >> 3) + 1;
>> > + rsize = ((report->size - 1) >> 3) + 1 + (report->id > 0) + 7;
>>
>> Isn't "report->id" already covered by "if (report_enum->numbered)"
>> above? The test for "id > 0" won't work here as in this case
>> "report_enum->numbered" must already be set to true by the hid-desc
>> parser, doesn't it?
>
> Right, that one is not correct here, thanks.
>
>> Where exactly did you get the +7 from?
>
> Please see commit (the one I am not really proud of) 27ce405039bfe6d3.
Eh, I remember.. Ok, but the magic-mouse is BT right? So this commit
really breaks BT drivers:
commit b1a1442a23776756b254b69786848a94d92445ba
Author: Jiri Kosina <jkosina@suse.cz>
Date: Mon Jun 3 11:27:48 2013 +0200
HID: core: fix reporting of raw events
Problem is, if the raw_event() callback returned 0 earlier, we just
skipped raw input reports. However, we now always call the
hid_report_raw_event() helper. Which is normally fine, but the helper
expects the input buffer to be of size HID_MAX_REPORT_SIZE, which is
*not* true for HIDP. So the memset() writes over some random memory.
I don't know exactly how to fix it. HID_MAX_BUFFER_SIZE is too big to
be allocated on the stack, but we're in atomic-context here so a
kzalloc(rsize, GFP_ATOMIC) seems overkill. So I guess we'd have to
look into HIDP to make the skb big enough, but I'm not sure how we can
achieve that.
So off the top of my head, the best idea is to add "char
inbuf[HID_MAX_BUFFER_SIZE];" to the hidp_session object in HIDP and
copy every input-report into the buffer before passing to
hid_input_report().
Ideas?
David
^ permalink raw reply
* Re: [PATCH 5/5] mfd: input: iio: ti_amm335x: rework TSC/ADC synchronisation
From: Sebastian Andrzej Siewior @ 2013-12-19 10:36 UTC (permalink / raw)
To: Lee Jones
Cc: Jonathan Cameron, Dmitry Torokhov, Samuel Ortiz, Zubair Lutfullah,
Felipe Balbi, linux-iio, linux-input, linux-kernel
In-Reply-To: <20131219084206.GC2621@lee--X1>
On 12/19/2013 09:42 AM, Lee Jones wrote:
> Spell check this entire block.
will do.
> Smileys in commit messages are generally a bad idea.
will drop.
> Please insert '\n's between paragraphs.
Okay.
> Proof read, as some of the sentences are not comprehensible.
I am going to retry.
> /* MFD Parts */
>
>> --- a/drivers/mfd/ti_am335x_tscadc.c
>> +++ b/drivers/mfd/ti_am335x_tscadc.c
>> @@ -24,6 +24,7 @@
>
>> -static void am335x_tsc_se_update(struct ti_tscadc_dev *tsadc)
>> +void am335x_tsc_se_set(struct ti_tscadc_dev *tsadc, u32 val)
>> {
>> + unsigned long flags;
>> +
>> + spin_lock_irqsave(&tsadc->reg_lock, flags);
>> + tsadc->reg_se_cache |= val;
>> tscadc_writel(tsadc, REG_SE, tsadc->reg_se_cache);
>> + spin_unlock_irqrestore(&tsadc->reg_lock, flags);
>> }
>> +EXPORT_SYMBOL_GPL(am335x_tsc_se_set);
>>
>> -void am335x_tsc_se_set(struct ti_tscadc_dev *tsadc, u32 val)
>> +void am335x_tsc_se_tsc_set(struct ti_tscadc_dev *tsadc, u32 val)
>
> What's the difference between:
>
> am335x_tsc_se_set()
> and
> am335x_tsc_se_tsc_set()
The code is the same. I tried to avoid to call the function with the
tsc in its name from the adc part. The continuous mode is still using
this interface because its content has to remain while the TSC is
updating the register. Only the write of the ADC in single-shot-mode is
used as a "one-time-thing" and not required later.
> Why don't you have a am335x_tsc_se_tsc_set_tsc_se_tsc_set() ;)
>
> Perhaps a better function name might be in order.
the am335x_tsc is the namespace. se the register followed by the user
which is tsc or adc but I know what you mean. I will try to replace it
with _cached and _once so we get rid of the part where is twice in the
name of the function.
>
>> {
>> unsigned long flags;
>>
>> spin_lock_irqsave(&tsadc->reg_lock, flags);
>> - tsadc->reg_se_cache |= val;
>> - am335x_tsc_se_update(tsadc);
>> + tsadc->reg_se_cache = val;
>> + if (tsadc->adc_in_use || tsadc->adc_waiting) {
>> + if (tsadc->adc_waiting)
>> + wake_up(&tsadc->reg_se_wait);
>
> So if we're either in-use or waiting, the step mask is never set?
>
> But I guess that the touchscreen driver assumes it has been set.
>
> Is this okay?
Yes this is okay because the ADC driver is waiting to use it
exclusively. The ADC driver invokes am335x_tsc_se_adc_done() once it is
done so TSC's mask which is saved here will then be written to the
register.
>
>> + } else {
>> + tscadc_writel(tsadc, REG_SE, val);
>> + }
>
> I think this would be better represented as:
>
> if (tsadc->adc_waiting)
> wake_up(&tsadc->reg_se_wait);
> else if (!tsadc->adc_in_use)
> tscadc_writel(tsadc, REG_SE, val);
I think that looks fine. I will double check it and take your proposal
if nothing goes wrong.
Sebastian
^ permalink raw reply
* [PATCH] Bluetooth: hidp: make sure input buffers are big enough
From: David Herrmann @ 2013-12-19 11:09 UTC (permalink / raw)
To: linux-input-u79uwXL29TY76Z2rM5mHXA
Cc: Jiri Kosina, linux-bluetooth-u79uwXL29TY76Z2rM5mHXA,
Marcel Holtmann, Gustavo Padovan, David Herrmann
HID core expects the input buffers to be at least of size 4096
(HID_MAX_BUFFER_SIZE). Other sizes will result in buffer-overflows if an
input-report is smaller than advertised. We could, like i2c, compute the
biggest report-size instead of using HID_MAX_BUFFER_SIZE, but this will
blow up if report-descriptors are changed after ->start() has been called.
So lets be safe and just use the biggest buffer we have.
Note that this adds an additional copy to the HIDP input path. If there is
a way to make sure the skb-buf is big enough, we should use that instead.
The best way would be to make hid-core honor the @size argument, though,
that sounds easier than it is. So lets just fix the buffer-overflows for
now and afterwards look for a faster way for all transport drivers.
Signed-off-by: David Herrmann <dh.herrmann-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org>
---
Hi
Any ideas how to improve this patch? I'd like to avoid the extra copy but I have
no clue how the skb stuff works exactly.
I also haven't figured out a nice way to make HID-core honor the "size"
parameter. hid-input depends on getting the whole input-report.
Comments welcome!
David
net/bluetooth/hidp/core.c | 16 ++++++++++++++--
net/bluetooth/hidp/hidp.h | 4 ++++
2 files changed, 18 insertions(+), 2 deletions(-)
diff --git a/net/bluetooth/hidp/core.c b/net/bluetooth/hidp/core.c
index 292e619..d9fb934 100644
--- a/net/bluetooth/hidp/core.c
+++ b/net/bluetooth/hidp/core.c
@@ -430,6 +430,16 @@ static void hidp_del_timer(struct hidp_session *session)
del_timer(&session->timer);
}
+static void hidp_process_report(struct hidp_session *session,
+ int type, const u8 *data, int len, int intr)
+{
+ if (len > HID_MAX_BUFFER_SIZE)
+ len = HID_MAX_BUFFER_SIZE;
+
+ memcpy(session->input_buf, data, len);
+ hid_input_report(session->hid, type, session->input_buf, len, intr);
+}
+
static void hidp_process_handshake(struct hidp_session *session,
unsigned char param)
{
@@ -502,7 +512,8 @@ static int hidp_process_data(struct hidp_session *session, struct sk_buff *skb,
hidp_input_report(session, skb);
if (session->hid)
- hid_input_report(session->hid, HID_INPUT_REPORT, skb->data, skb->len, 0);
+ hidp_process_report(session, HID_INPUT_REPORT,
+ skb->data, skb->len, 0);
break;
case HIDP_DATA_RTYPE_OTHER:
@@ -584,7 +595,8 @@ static void hidp_recv_intr_frame(struct hidp_session *session,
hidp_input_report(session, skb);
if (session->hid) {
- hid_input_report(session->hid, HID_INPUT_REPORT, skb->data, skb->len, 1);
+ hidp_process_report(session, HID_INPUT_REPORT,
+ skb->data, skb->len, 1);
BT_DBG("report len %d", skb->len);
}
} else {
diff --git a/net/bluetooth/hidp/hidp.h b/net/bluetooth/hidp/hidp.h
index ab52414..8798492 100644
--- a/net/bluetooth/hidp/hidp.h
+++ b/net/bluetooth/hidp/hidp.h
@@ -24,6 +24,7 @@
#define __HIDP_H
#include <linux/types.h>
+#include <linux/hid.h>
#include <linux/kref.h>
#include <net/bluetooth/bluetooth.h>
#include <net/bluetooth/l2cap.h>
@@ -179,6 +180,9 @@ struct hidp_session {
/* Used in hidp_output_raw_report() */
int output_report_success; /* boolean */
+
+ /* temporary input buffer */
+ u8 input_buf[HID_MAX_BUFFER_SIZE];
};
/* HIDP init defines */
--
1.8.5.1
^ permalink raw reply related
* Re: [v3.11][Regression] HID: hyperv: convert alloc+memcpy to memdup
From: David Herrmann @ 2013-12-19 11:22 UTC (permalink / raw)
To: Jiri Kosina
Cc: Joseph Salisbury, Haiyang Zhang, LKML, open, Dan Carpenter,
HID CORE LAYER, driverdev-devel@linuxdriverproject.org, thomas
In-Reply-To: <CANq1E4RuteNfo23DQ5SvGXpsrpTmotLoJ6Wv1L8hAbE1ict6aA@mail.gmail.com>
Hi
On Thu, Dec 19, 2013 at 11:08 AM, David Herrmann <dh.herrmann@gmail.com> wrote:
> Hi
>
> On Thu, Dec 19, 2013 at 10:59 AM, Jiri Kosina <jkosina@suse.cz> wrote:
>> On Thu, 19 Dec 2013, David Herrmann wrote:
>>
>>> > diff --git a/drivers/hid/hid-core.c b/drivers/hid/hid-core.c
>>> > index 253fe23..81eacd3 100644
>>> > --- a/drivers/hid/hid-core.c
>>> > +++ b/drivers/hid/hid-core.c
>>> > @@ -1334,7 +1334,7 @@ int hid_report_raw_event(struct hid_device *hid, int type, u8 *data, int size,
>>> > csize--;
>>> > }
>>> >
>>> > - rsize = ((report->size - 1) >> 3) + 1;
>>> > + rsize = ((report->size - 1) >> 3) + 1 + (report->id > 0) + 7;
>>>
>>> Isn't "report->id" already covered by "if (report_enum->numbered)"
>>> above? The test for "id > 0" won't work here as in this case
>>> "report_enum->numbered" must already be set to true by the hid-desc
>>> parser, doesn't it?
>>
>> Right, that one is not correct here, thanks.
>>
>>> Where exactly did you get the +7 from?
>>
>> Please see commit (the one I am not really proud of) 27ce405039bfe6d3.
>
> Eh, I remember.. Ok, but the magic-mouse is BT right? So this commit
> really breaks BT drivers:
>
> commit b1a1442a23776756b254b69786848a94d92445ba
> Author: Jiri Kosina <jkosina@suse.cz>
> Date: Mon Jun 3 11:27:48 2013 +0200
>
> HID: core: fix reporting of raw events
>
> Problem is, if the raw_event() callback returned 0 earlier, we just
> skipped raw input reports. However, we now always call the
> hid_report_raw_event() helper. Which is normally fine, but the helper
> expects the input buffer to be of size HID_MAX_REPORT_SIZE, which is
> *not* true for HIDP. So the memset() writes over some random memory.
>
> I don't know exactly how to fix it. HID_MAX_BUFFER_SIZE is too big to
> be allocated on the stack, but we're in atomic-context here so a
> kzalloc(rsize, GFP_ATOMIC) seems overkill. So I guess we'd have to
> look into HIDP to make the skb big enough, but I'm not sure how we can
> achieve that.
>
> So off the top of my head, the best idea is to add "char
> inbuf[HID_MAX_BUFFER_SIZE];" to the hidp_session object in HIDP and
> copy every input-report into the buffer before passing to
> hid_input_report().
As this thread doesn't really contain any oops message nor the exact
driver name (except mentioning hyperv and magicmouse), I fixed a
related bug for HIDP:
http://thread.gmane.org/gmane.linux.bluez.kernel/41969
A similar patch is *really* required for hid-hyperv.c as it also does
not provide a properly sized buffer to hid_input_report().
David
^ 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