| Message ID | 1387295334-1744-3-git-send-email-dh.herrmann@gmail.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
Hi David, thanks for the update. On Tue, 17 Dec 2013 16:48:52 +0100 David Herrmann <dh.herrmann@gmail.com> 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. > Just a question: did you consider the possibility of not exposing _MAX2 and _CNT2 in a header file at all but let those be queried? Or maybe this is not necessary if we assume that userspace programs are supposed to know what is the MAX _they_ support? BTW I was thinking for instance to a program compiled with ABS_MAX2 = 0x4f but working with future kernels with an increased value, it still won't be able to support values greater than 0x4f. > 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. > To my inexperienced eye it's not obvious why ABS_MT_SLOT must be handled as a special case. Maybe because I don't even know what code=0,cnt=ABS_CNT2 is used for. > 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. > I'll check how to use that in evtest. Just one more comment below. > 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/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 [...] > > /* > + * 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) > + Maybe it's just my English, but when you say "between ABS_MAX and ABS_MAX2" it sounds like the old protocol still _must_ be used for values <= ABS_MAX. IIUC this is true "only" for compatibility with older kernels right? If a program decides to support only newer kernels it can check the protocol version and use only the new ioctls, right? Maybe you can be more explicit about that in the comment? Thanks, Antonio
Hi On Wed, Dec 18, 2013 at 3:27 PM, Antonio Ospite <ospite@studenti.unina.it> wrote: > Hi David, > > thanks for the update. > > On Tue, 17 Dec 2013 16:48:52 +0100 > David Herrmann <dh.herrmann@gmail.com> 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. >> > > Just a question: did you consider the possibility of not exposing _MAX2 > and _CNT2 in a header file at all but let those be queried? Or maybe > this is not necessary if we assume that userspace programs are supposed > to know what is the MAX _they_ support? > > BTW I was thinking for instance to a program compiled with ABS_MAX2 = > 0x4f but working with future kernels with an increased value, it still > won't be able to support values greater than 0x4f. Why would a program that was compiled with ABS_MAX2=0x4f want to use ABS_* values greater than 0x4f? It doesn't know of any new values so even if it could query ABS_MAX2, it could never know anything about the new constants. The only place where it is useful is for generic programs that just foward ABS_* codes. But these can just ignore ABS_MAX2 entirely and use the whole int32_t range for codes. If there is a specific use-case that'd require a dynamic way to retrieve ABS_MAX2, I can consider changing the API. But unless there's such a use-case, I'd like to keep it the same as for KEY_*, SW_*, ... >> 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. >> > > To my inexperienced eye it's not obvious why ABS_MT_SLOT must be handled > as a special case. Maybe because I don't even know what > code=0,cnt=ABS_CNT2 is used for. ABS_MT_SLOT describes the current MT-slot that is reported via ABS_MT_* bits. It is read-only so we cannot let user-space write to it. >> 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. >> > > I'll check how to use that in evtest. > > Just one more comment below. > >> 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/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 > [...] >> >> /* >> + * 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) >> + > > Maybe it's just my English, but when you say "between ABS_MAX and > ABS_MAX2" it sounds like the old protocol still _must_ be used for > values <= ABS_MAX. > > IIUC this is true "only" for compatibility with older kernels right? > If a program decides to support only newer kernels it can check the > protocol version and use only the new ioctls, right? > > Maybe you can be more explicit about that in the comment? See the comment on "struct input_absinfo2". It describes the API, this comment just describes the ABS_* values. But if anyone has a better phrase to use here, I will gladly adjust it. Thanks David -- 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
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]; I wonder if kittens will die if we do: in include/uapi/linux/input: #define ABS_MAX1 0x3f #define ABS_MAX2 0xYY /* * Keep old value of ABS_MAX exported to userspace. Users ready to * handle bigger range will have to use ABS_MAX2 explicitly. */ #define ABS_MAX ABS_MAX1 in include/linux/input.h ... /* Kernel should use the new ABS range by default */ #undef ABS_MAX #define ABS_MAX ABS_MAX2 Thanks.
On Wed, Dec 18, 2013 at 03:44:48PM +0100, David Herrmann wrote: > >> > >> /* > >> + * 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) > >> + > > > > Maybe it's just my English, but when you say "between ABS_MAX and > > ABS_MAX2" it sounds like the old protocol still _must_ be used for > > values <= ABS_MAX. > > > > IIUC this is true "only" for compatibility with older kernels right? > > If a program decides to support only newer kernels it can check the > > protocol version and use only the new ioctls, right? > > > > Maybe you can be more explicit about that in the comment? > > See the comment on "struct input_absinfo2". It describes the API, this > comment just describes the ABS_* values. But if anyone has a better > phrase to use here, I will gladly adjust it. "Use the extended *ABS2 ioctls to access the full range of ABS values supported by the kernel." ?
On Wed, 18 Dec 2013 15:44:48 +0100 David Herrmann <dh.herrmann@gmail.com> wrote: > Hi > > On Wed, Dec 18, 2013 at 3:27 PM, Antonio Ospite > <ospite@studenti.unina.it> wrote: > > Hi David, > > > > thanks for the update. > > > > On Tue, 17 Dec 2013 16:48:52 +0100 > > David Herrmann <dh.herrmann@gmail.com> 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. > >> > > > > Just a question: did you consider the possibility of not exposing _MAX2 > > and _CNT2 in a header file at all but let those be queried? Or maybe > > this is not necessary if we assume that userspace programs are supposed > > to know what is the MAX _they_ support? > > > > BTW I was thinking for instance to a program compiled with ABS_MAX2 = > > 0x4f but working with future kernels with an increased value, it still > > won't be able to support values greater than 0x4f. > > Why would a program that was compiled with ABS_MAX2=0x4f want to use > ABS_* values greater than 0x4f? It doesn't know of any new values so > even if it could query ABS_MAX2, it could never know anything about > the new constants. The only place where it is useful is for generic > programs that just foward ABS_* codes. But these can just ignore > ABS_MAX2 entirely and use the whole int32_t range for codes. > OK, that makes sense, thanks. > If there is a specific use-case that'd require a dynamic way to > retrieve ABS_MAX2, I can consider changing the API. But unless there's > such a use-case, I'd like to keep it the same as for KEY_*, SW_*, ... > > >> 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. > >> > > > > To my inexperienced eye it's not obvious why ABS_MT_SLOT must be handled > > as a special case. Maybe because I don't even know what > > code=0,cnt=ABS_CNT2 is used for. > > ABS_MT_SLOT describes the current MT-slot that is reported via > ABS_MT_* bits. It is read-only so we cannot let user-space write to > it. > I see. > >> 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. > >> > > > > I'll check how to use that in evtest. > > > > Just one more comment below. > > > >> 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/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 > > [...] > >> > >> /* > >> + * 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) > >> + > > > > Maybe it's just my English, but when you say "between ABS_MAX and > > ABS_MAX2" it sounds like the old protocol still _must_ be used for > > values <= ABS_MAX. > > > > IIUC this is true "only" for compatibility with older kernels right? > > If a program decides to support only newer kernels it can check the > > protocol version and use only the new ioctls, right? > > > > Maybe you can be more explicit about that in the comment? > > See the comment on "struct input_absinfo2". It describes the API, this > comment just describes the ABS_* values. But if anyone has a better > phrase to use here, I will gladly adjust it. > That comment says: EVIOC[G/S]ABS2 ioctls [...] do the same as the old EVIOC [G/S]ABS ioctls but... My bad, I was confused by the fact that the new ioctls are presented as a replacement for the old ones, but then I interpreted the later comment as suggesting to use them only for values between ABS_MAX and ABS_MAX2. Looking at how you use both mechanisms combined in libevdev it is clear they can be used together for backward compatibility. I like keywords, so I'd stress in the commit message that the new protocol can either _replace_ or _extend_ the old one. Ciao, Antonio
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 > -- 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
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.
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 -- 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
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.
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 -- 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
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.
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; +} + +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/input.c b/drivers/input/input.c index 846ccdd..042157c 100644 --- a/drivers/input/input.c +++ b/drivers/input/input.c @@ -305,7 +305,7 @@ static int input_get_disposition(struct input_dev *dev, break; case EV_ABS: - if (is_event_supported(code, dev->absbit, ABS_MAX)) + if (is_event_supported(code, dev->absbit, ABS_MAX2)) disposition = input_handle_abs_event(dev, code, &value); break; @@ -474,7 +474,7 @@ EXPORT_SYMBOL(input_inject_event); void input_alloc_absinfo(struct input_dev *dev) { if (!dev->absinfo) - dev->absinfo = kcalloc(ABS_CNT, sizeof(struct input_absinfo), + dev->absinfo = kcalloc(ABS_CNT2, sizeof(struct input_absinfo), GFP_KERNEL); WARN(!dev->absinfo, "%s(): kcalloc() failed?\n", __func__); @@ -954,7 +954,7 @@ static const struct input_device_id *input_match_device(struct input_handler *ha if (!bitmap_subset(id->relbit, dev->relbit, REL_MAX)) continue; - if (!bitmap_subset(id->absbit, dev->absbit, ABS_MAX)) + if (!bitmap_subset(id->absbit, dev->absbit, ABS_MAX2)) continue; if (!bitmap_subset(id->mscbit, dev->mscbit, MSC_MAX)) @@ -1147,7 +1147,7 @@ static int input_devices_seq_show(struct seq_file *seq, void *v) if (test_bit(EV_REL, dev->evbit)) input_seq_print_bitmap(seq, "REL", dev->relbit, REL_MAX); if (test_bit(EV_ABS, dev->evbit)) - input_seq_print_bitmap(seq, "ABS", dev->absbit, ABS_MAX); + input_seq_print_bitmap(seq, "ABS", dev->absbit, ABS_MAX2); if (test_bit(EV_MSC, dev->evbit)) input_seq_print_bitmap(seq, "MSC", dev->mscbit, MSC_MAX); if (test_bit(EV_LED, dev->evbit)) @@ -1333,7 +1333,7 @@ static int input_print_modalias(char *buf, int size, struct input_dev *id, len += input_print_modalias_bits(buf + len, size - len, 'r', id->relbit, 0, REL_MAX); len += input_print_modalias_bits(buf + len, size - len, - 'a', id->absbit, 0, ABS_MAX); + 'a', id->absbit, 0, ABS_MAX2); len += input_print_modalias_bits(buf + len, size - len, 'm', id->mscbit, 0, MSC_MAX); len += input_print_modalias_bits(buf + len, size - len, @@ -1592,7 +1592,7 @@ static int input_dev_uevent(struct device *device, struct kobj_uevent_env *env) if (test_bit(EV_REL, dev->evbit)) INPUT_ADD_HOTPLUG_BM_VAR("REL=", dev->relbit, REL_MAX); if (test_bit(EV_ABS, dev->evbit)) - INPUT_ADD_HOTPLUG_BM_VAR("ABS=", dev->absbit, ABS_MAX); + INPUT_ADD_HOTPLUG_BM_VAR("ABS=", dev->absbit, ABS_MAX2); if (test_bit(EV_MSC, dev->evbit)) INPUT_ADD_HOTPLUG_BM_VAR("MSC=", dev->mscbit, MSC_MAX); if (test_bit(EV_LED, dev->evbit)) @@ -1929,7 +1929,7 @@ static unsigned int input_estimate_events_per_packet(struct input_dev *dev) events = mt_slots + 1; /* count SYN_MT_REPORT and SYN_REPORT */ - for (i = 0; i < ABS_CNT; i++) { + for (i = 0; i < ABS_CNT2; i++) { if (test_bit(i, dev->absbit)) { if (input_is_mt_axis(i)) events += mt_slots; diff --git a/drivers/input/keyboard/goldfish_events.c b/drivers/input/keyboard/goldfish_events.c index 9f60a2e..9999cea 100644 --- a/drivers/input/keyboard/goldfish_events.c +++ b/drivers/input/keyboard/goldfish_events.c @@ -90,8 +90,8 @@ static void events_import_abs_params(struct event_dev *edev) __raw_writel(PAGE_ABSDATA, addr + REG_SET_PAGE); count = __raw_readl(addr + REG_LEN) / sizeof(val); - if (count > ABS_MAX) - count = ABS_MAX; + if (count > ABS_MAX2) + count = ABS_MAX2; for (i = 0; i < count; i++) { if (!test_bit(i, input_dev->absbit)) @@ -158,7 +158,7 @@ static int events_probe(struct platform_device *pdev) events_import_bits(edev, input_dev->evbit, EV_SYN, EV_MAX); events_import_bits(edev, input_dev->keybit, EV_KEY, KEY_MAX); events_import_bits(edev, input_dev->relbit, EV_REL, REL_MAX); - events_import_bits(edev, input_dev->absbit, EV_ABS, ABS_MAX); + events_import_bits(edev, input_dev->absbit, EV_ABS, ABS_MAX2); events_import_bits(edev, input_dev->mscbit, EV_MSC, MSC_MAX); events_import_bits(edev, input_dev->ledbit, EV_LED, LED_MAX); events_import_bits(edev, input_dev->sndbit, EV_SND, SND_MAX); diff --git a/drivers/input/keyboard/hil_kbd.c b/drivers/input/keyboard/hil_kbd.c index 589e3c2..4e4e010 100644 --- a/drivers/input/keyboard/hil_kbd.c +++ b/drivers/input/keyboard/hil_kbd.c @@ -387,7 +387,7 @@ static void hil_dev_pointer_setup(struct hil_dev *ptr) 0, HIL_IDD_AXIS_MAX(idd, i - 3), 0, 0); #ifdef TABLET_AUTOADJUST - for (i = 0; i < ABS_MAX; i++) { + for (i = 0; i < ABS_MAX2; i++) { int diff = input_abs_get_max(input_dev, ABS_X + i) / 10; input_abs_set_min(input_dev, ABS_X + i, input_abs_get_min(input_dev, ABS_X + i) + diff); 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); 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 /* * IOCTLs (0x00 - 0x7f) @@ -74,6 +74,30 @@ struct input_absinfo { }; /** + * struct input_absinfo2 - used by EVIOC[G/S]ABS2 ioctls + * @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. + * + * 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. + */ +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_ */
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(-)