Re: Handling of large keycodes
On Sat, Jul 31, 2010 at 02:19:36AM -0700, Dmitry Torokhov wrote:
+/**
+ * struct keymap_entry - used by EVIOCGKEYCODE/EVIOCSKEYCODE ioctls
+ * @scancode: scancode represented in machine-endian form.
+ * @len: length of the scancode that resides in @scancode buffer.
+ * @index: index in the keymap, may be used instead of scancode
+ * @by_index: boolean value indicating that kernel should perform
+ * lookup in keymap by @index instead of @scancode
+ * @keycode: key code assigned to this scancode
+ *
+ * The structure is used to retrieve and modify keymap data. Users have
missing the option here?
+ * of performing lookup either by @scancode itself or by @index in
+ * keymap entry. EVIOCGKEYCODE will also return scancode or index
+ * (depending on which element was used to perform lookup).
+ */
+struct keymap_entry {
+ __u8 len;
+ __u8 by_index;
+ __u16 index;
+ __u32 keycode;
+ __u8 scancode[32];
};
Perhaps it would be a good idea to add a flags member to the struct,
either as an additional member or by replacing:
__u8 by_index;
with:
__u32 flags;
to help with any future extensions/changes/additions to the interface?
--
David Härdeman
--
To unsubscribe from this list: send the line unsubscribe linux-media in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: Handling of large keycodes
On Sat, Jul 31, 2010 at 10:23:45AM -0300, Mauro Carvalho Chehab wrote: Em 31-07-2010 06:19, Dmitry Torokhov escreveu: The usefulnes of reserved data elements in the structure is doubtful, since we do not seem to require them being set to a particular value and so we'll be unable to distinguish betwee legacy and newer users. David proposed some parameters that we rejected on our discussions. As we might need to add something similar, I decided to keep it on my approach, since a set of reserved fields wouldn't hurt (and removing it on our discussions would be easy), but I'm ok on removing them. The parameters that we'll need for the ir subsystem can be implemented in a different way in the future (with no changes necessary to the input subsystem) so I'm also ok with removing them. -- David Härdeman -- To unsubscribe from this list: send the line unsubscribe linux-media in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: Handling of large keycodes
On Sat, Jul 31, 2010 at 10:23:45AM -0300, Mauro Carvalho Chehab wrote: Hi Dmitry, Em 31-07-2010 06:19, Dmitry Torokhov escreveu: Hi Mauro, I finally got a chance to review the patches adding handling of large scancodes to input core and there are a few things with this approach that I do not like. Thanks for the review! First of all I do not think that we should be working with scancode via a pointer as it requires additional compat handling when running 32-bit userspace on 64-bit kernel. We can use a static buffer of sufficient size (lets say 32 bytes) to move scancode around and simply increase its size if we come upon device that uses even bigger scancodes. As long as buffer is at the end we can handle this in a compatible way. Yes, this is the downside of using a pointer. I'm not aware of a Remote Controller protocol using more than 256 bits for scancode, so 32 bits should be ok. The other issue is that interface is notsymmetrical, setting is done by scancode but retrieval is done by index. I think we should be able to use both scancode and index in both operations. Yes, this also bothered me. I was thinking to do something similar to your approach of having a bool to select between them. This change is welcome. The usefulnes of reserved data elements in the structure is doubtful, since we do not seem to require them being set to a particular value and so we'll be unable to distinguish betwee legacy and newer users. David proposed some parameters that we rejected on our discussions. As we might need to add something similar, I decided to keep it on my approach, since a set of reserved fields wouldn't hurt (and removing it on our discussions would be easy), but I'm ok on removing them. I also concerned about the code very messy with regard to using old/new style interfaces instea dof converting old ones to use new insfrastructure, Good cleanup at the code! I below is something that addresses these issues and seems to be working for me. It is on top of your patches and it also depends on a few changes in my tree that I have not publushed yet but plan on doing that tomorrow. I am also attaching patches converting sparse keymap and hid to the new style of getkeycode and setkeycode as examples. Please take a look and let me know if I missed something important. It seems to work for me. After you add the patches on your git tree, I'll work on porting the RC core to the new approach, and change the ir-keycode userspace program to work with, in order to be 100% sure that it will work, but I can't foresee any missing part on it. Currently, I'm not using my input patches, as I was waiting for your review. I just patched the userspace application, in order to test the legacy mode. OK, great. I want to fold all the patches from your tree plus this one into one and apply in one shpt (mentioning Jarod and Dan as authors of fixup patches in the changelog) - I do not believe we shoudl expose intermediate versions in the code that will go to Linus. Are you OK with this? Thanks. -- Dmitry -- To unsubscribe from this list: send the line unsubscribe linux-media in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: Handling of large keycodes
On Sat, Jul 31, 2010 at 01:03:26PM +0200, Stefan Richter wrote:
Dmitry Torokhov wrote:
--- a/include/linux/input.h
+++ b/include/linux/input.h
@@ -56,22 +56,35 @@ struct input_absinfo {
__s32 resolution;
};
-struct keycode_table_entry {
- __u32 keycode; /* e.g. KEY_A */
- __u32 index;/* Index for the given scan/key table, on
EVIOCGKEYCODEBIG */
- __u32 len; /* Length of the scancode */
- __u32 reserved[2]; /* Reserved for future usage */
- char *scancode; /* scancode, in machine-endian */
+/**
+ * struct keymap_entry - used by EVIOCGKEYCODE/EVIOCSKEYCODE ioctls
+ * @scancode: scancode represented in machine-endian form.
+ * @len: length of the scancode that resides in @scancode buffer.
+ * @index: index in the keymap, may be used instead of scancode
+ * @by_index: boolean value indicating that kernel should perform
+ * lookup in keymap by @index instead of @scancode
+ * @keycode: key code assigned to this scancode
+ *
+ * The structure is used to retrieve and modify keymap data. Users have
+ * of performing lookup either by @scancode itself or by @index in
+ * keymap entry. EVIOCGKEYCODE will also return scancode or index
+ * (depending on which element was used to perform lookup).
+ */
+struct keymap_entry {
+ __u8 len;
+ __u8 by_index;
+ __u16 index;
+ __u32 keycode;
+ __u8 scancode[32];
};
I agree with Dimitry; don't put a pointer typed member into a userspace
ABI struct.
Two remarks:
- Presently, linux/input.h defines three structs named input_... for
userspace, two structs named input_... for kernelspace, and a few
structs named ff_... specially for force-feedback stuff. How about
calling struct keymap_entry perhaps struct input_keymap_entry
instead, to keep namespaces tidy?
Good idea, changed.
- I take it from your description that scan codes are fundamentally
variable-length data. How about defining it as __u8 scancode[0]?
In addition to difficulty in adding members (as you mentioned) it also
makes it more difficult for users to allocate such variables on stack or
make them global (anything but malloc with additional memory buffer)...
--
Dmitry
--
To unsubscribe from this list: send the line unsubscribe linux-media in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: Handling of large keycodes
Em 02-08-2010 05:02, Dmitry Torokhov escreveu: On Sat, Jul 31, 2010 at 10:23:45AM -0300, Mauro Carvalho Chehab wrote: Hi Dmitry, Em 31-07-2010 06:19, Dmitry Torokhov escreveu: Hi Mauro, I finally got a chance to review the patches adding handling of large scancodes to input core and there are a few things with this approach that I do not like. Thanks for the review! First of all I do not think that we should be working with scancode via a pointer as it requires additional compat handling when running 32-bit userspace on 64-bit kernel. We can use a static buffer of sufficient size (lets say 32 bytes) to move scancode around and simply increase its size if we come upon device that uses even bigger scancodes. As long as buffer is at the end we can handle this in a compatible way. Yes, this is the downside of using a pointer. I'm not aware of a Remote Controller protocol using more than 256 bits for scancode, so 32 bits should be ok. The other issue is that interface is notsymmetrical, setting is done by scancode but retrieval is done by index. I think we should be able to use both scancode and index in both operations. Yes, this also bothered me. I was thinking to do something similar to your approach of having a bool to select between them. This change is welcome. The usefulnes of reserved data elements in the structure is doubtful, since we do not seem to require them being set to a particular value and so we'll be unable to distinguish betwee legacy and newer users. David proposed some parameters that we rejected on our discussions. As we might need to add something similar, I decided to keep it on my approach, since a set of reserved fields wouldn't hurt (and removing it on our discussions would be easy), but I'm ok on removing them. I also concerned about the code very messy with regard to using old/new style interfaces instea dof converting old ones to use new insfrastructure, Good cleanup at the code! I below is something that addresses these issues and seems to be working for me. It is on top of your patches and it also depends on a few changes in my tree that I have not publushed yet but plan on doing that tomorrow. I am also attaching patches converting sparse keymap and hid to the new style of getkeycode and setkeycode as examples. Please take a look and let me know if I missed something important. It seems to work for me. After you add the patches on your git tree, I'll work on porting the RC core to the new approach, and change the ir-keycode userspace program to work with, in order to be 100% sure that it will work, but I can't foresee any missing part on it. Currently, I'm not using my input patches, as I was waiting for your review. I just patched the userspace application, in order to test the legacy mode. OK, great. I want to fold all the patches from your tree plus this one into one and apply in one shpt (mentioning Jarod and Dan as authors of fixup patches in the changelog) - I do not believe we shoudl expose intermediate versions in the code that will go to Linus. Are you OK with this? I'm OK. If you want, you can add my ack on your patch: Acked-by: Mauro Carvalho Chehab mche...@redhat.com Cheers, Mauro -- To unsubscribe from this list: send the line unsubscribe linux-media in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: Handling of large keycodes
On Mon, Aug 02, 2010 at 08:35:37AM -0300, Mauro Carvalho Chehab wrote: Em 02-08-2010 05:02, Dmitry Torokhov escreveu: On Sat, Jul 31, 2010 at 10:23:45AM -0300, Mauro Carvalho Chehab wrote: Hi Dmitry, Em 31-07-2010 06:19, Dmitry Torokhov escreveu: Hi Mauro, I finally got a chance to review the patches adding handling of large scancodes to input core and there are a few things with this approach that I do not like. Thanks for the review! First of all I do not think that we should be working with scancode via a pointer as it requires additional compat handling when running 32-bit userspace on 64-bit kernel. We can use a static buffer of sufficient size (lets say 32 bytes) to move scancode around and simply increase its size if we come upon device that uses even bigger scancodes. As long as buffer is at the end we can handle this in a compatible way. Yes, this is the downside of using a pointer. I'm not aware of a Remote Controller protocol using more than 256 bits for scancode, so 32 bits should be ok. The other issue is that interface is notsymmetrical, setting is done by scancode but retrieval is done by index. I think we should be able to use both scancode and index in both operations. Yes, this also bothered me. I was thinking to do something similar to your approach of having a bool to select between them. This change is welcome. The usefulnes of reserved data elements in the structure is doubtful, since we do not seem to require them being set to a particular value and so we'll be unable to distinguish betwee legacy and newer users. David proposed some parameters that we rejected on our discussions. As we might need to add something similar, I decided to keep it on my approach, since a set of reserved fields wouldn't hurt (and removing it on our discussions would be easy), but I'm ok on removing them. I also concerned about the code very messy with regard to using old/new style interfaces instea dof converting old ones to use new insfrastructure, Good cleanup at the code! I below is something that addresses these issues and seems to be working for me. It is on top of your patches and it also depends on a few changes in my tree that I have not publushed yet but plan on doing that tomorrow. I am also attaching patches converting sparse keymap and hid to the new style of getkeycode and setkeycode as examples. Please take a look and let me know if I missed something important. It seems to work for me. After you add the patches on your git tree, I'll work on porting the RC core to the new approach, and change the ir-keycode userspace program to work with, in order to be 100% sure that it will work, but I can't foresee any missing part on it. Currently, I'm not using my input patches, as I was waiting for your review. I just patched the userspace application, in order to test the legacy mode. OK, great. I want to fold all the patches from your tree plus this one into one and apply in one shpt (mentioning Jarod and Dan as authors of fixup patches in the changelog) - I do not believe we shoudl expose intermediate versions in the code that will go to Linus. Are you OK with this? I'm OK. If you want, you can add my ack on your patch: Acked-by: Mauro Carvalho Chehab mche...@redhat.com Yeah, works for me too. -- Jarod Wilson ja...@redhat.com -- To unsubscribe from this list: send the line unsubscribe linux-media in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Handling of large keycodes
Hi Mauro,
I finally got a chance to review the patches adding handling of large
scancodes to input core and there are a few things with this approach
that I do not like.
First of all I do not think that we should be working with scancode via
a pointer as it requires additional compat handling when running 32-bit
userspace on 64-bit kernel. We can use a static buffer of sufficient
size (lets say 32 bytes) to move scancode around and simply increase its
size if we come upon device that uses even bigger scancodes. As long as
buffer is at the end we can handle this in a compatible way.
The other issue is that interface is notsymmetrical, setting is done by
scancode but retrieval is done by index. I think we should be able to
use both scancode and index in both operations.
The usefulnes of reserved data elements in the structure is doubtful,
since we do not seem to require them being set to a particular value and
so we'll be unable to distinguish betwee legacy and newer users.
I also concerned about the code very messy with regard to using old/new
style interfaces instea dof converting old ones to use new
insfrastructure,
I below is something that addresses these issues and seems to be working
for me. It is on top of your patches and it also depends on a few
changes in my tree that I have not publushed yet but plan on doing that
tomorrow. I am also attaching patches converting sparse keymap and hid
to the new style of getkeycode and setkeycode as examples.
Please take a look and let me know if I missed something important.
Thank you.
--
Dmitry
Signed-off-by: Dmitry Torokhov d...@mail.ru
---
drivers/char/keyboard.c | 31 +++-
drivers/input/evdev.c | 139 +++
drivers/input/input.c | 351 +++
include/linux/input.h | 72 +-
4 files changed, 255 insertions(+), 338 deletions(-)
diff --git a/drivers/char/keyboard.c b/drivers/char/keyboard.c
index 54109dc..4dd9fb0 100644
--- a/drivers/char/keyboard.c
+++ b/drivers/char/keyboard.c
@@ -175,8 +175,7 @@ EXPORT_SYMBOL_GPL(unregister_keyboard_notifier);
*/
struct getset_keycode_data {
- unsigned int scancode;
- unsigned int keycode;
+ struct keymap_entry ke;
int error;
};
@@ -184,32 +183,50 @@ static int getkeycode_helper(struct input_handle *handle,
void *data)
{
struct getset_keycode_data *d = data;
- d-error = input_get_keycode(handle-dev, d-scancode, d-keycode);
+ d-error = input_get_keycode(handle-dev, d-ke);
return d-error == 0; /* stop as soon as we successfully get one */
}
int getkeycode(unsigned int scancode)
{
- struct getset_keycode_data d = { scancode, 0, -ENODEV };
+ struct getset_keycode_data d = {
+ .ke = {
+ .by_index = false,
+ .len= sizeof(scancode),
+ .keycode= 0,
+ },
+ .error = -ENODEV,
+ };
+
+ memcpy(d.ke.scancode, scancode, sizeof(scancode));
input_handler_for_each_handle(kbd_handler, d, getkeycode_helper);
- return d.error ?: d.keycode;
+ return d.error ?: d.ke.keycode;
}
static int setkeycode_helper(struct input_handle *handle, void *data)
{
struct getset_keycode_data *d = data;
- d-error = input_set_keycode(handle-dev, d-scancode, d-keycode);
+ d-error = input_set_keycode(handle-dev, d-ke);
return d-error == 0; /* stop as soon as we successfully set one */
}
int setkeycode(unsigned int scancode, unsigned int keycode)
{
- struct getset_keycode_data d = { scancode, keycode, -ENODEV };
+ struct getset_keycode_data d = {
+ .ke = {
+ .by_index = false,
+ .len= sizeof(scancode),
+ .keycode= keycode,
+ },
+ .error = -ENODEV,
+ };
+
+ memcpy(d.ke.scancode, scancode, sizeof(scancode));
input_handler_for_each_handle(kbd_handler, d, setkeycode_helper);
diff --git a/drivers/input/evdev.c b/drivers/input/evdev.c
index 783cdd3..9c7a97b 100644
--- a/drivers/input/evdev.c
+++ b/drivers/input/evdev.c
@@ -534,6 +534,80 @@ static int handle_eviocgbit(struct input_dev *dev,
}
#undef OLD_KEY_MAX
+static int evdev_handle_get_keycode(struct input_dev *dev,
+ void __user *p, size_t size)
+{
+ struct keymap_entry ke;
+ int error;
+
+ memset(ke, 0, sizeof(ke));
+
+ if (size == sizeof(unsigned int[2])) {
+ /* legacy case */
+ int __user *ip = (int __user *)p;
+
+ if (copy_from_user(ke.scancode, p, sizeof(unsigned int)))
+ return -EFAULT;
+
+ ke.len = sizeof(unsigned int);
+ ke.by_index = false;
+
+ error = input_get_keycode(dev, ke);
+
Re: Handling of large keycodes
Hi Dmitry, Em 31-07-2010 06:19, Dmitry Torokhov escreveu: Hi Mauro, I finally got a chance to review the patches adding handling of large scancodes to input core and there are a few things with this approach that I do not like. Thanks for the review! First of all I do not think that we should be working with scancode via a pointer as it requires additional compat handling when running 32-bit userspace on 64-bit kernel. We can use a static buffer of sufficient size (lets say 32 bytes) to move scancode around and simply increase its size if we come upon device that uses even bigger scancodes. As long as buffer is at the end we can handle this in a compatible way. Yes, this is the downside of using a pointer. I'm not aware of a Remote Controller protocol using more than 256 bits for scancode, so 32 bits should be ok. The other issue is that interface is notsymmetrical, setting is done by scancode but retrieval is done by index. I think we should be able to use both scancode and index in both operations. Yes, this also bothered me. I was thinking to do something similar to your approach of having a bool to select between them. This change is welcome. The usefulnes of reserved data elements in the structure is doubtful, since we do not seem to require them being set to a particular value and so we'll be unable to distinguish betwee legacy and newer users. David proposed some parameters that we rejected on our discussions. As we might need to add something similar, I decided to keep it on my approach, since a set of reserved fields wouldn't hurt (and removing it on our discussions would be easy), but I'm ok on removing them. I also concerned about the code very messy with regard to using old/new style interfaces instea dof converting old ones to use new insfrastructure, Good cleanup at the code! I below is something that addresses these issues and seems to be working for me. It is on top of your patches and it also depends on a few changes in my tree that I have not publushed yet but plan on doing that tomorrow. I am also attaching patches converting sparse keymap and hid to the new style of getkeycode and setkeycode as examples. Please take a look and let me know if I missed something important. It seems to work for me. After you add the patches on your git tree, I'll work on porting the RC core to the new approach, and change the ir-keycode userspace program to work with, in order to be 100% sure that it will work, but I can't foresee any missing part on it. Currently, I'm not using my input patches, as I was waiting for your review. I just patched the userspace application, in order to test the legacy mode. Cheers, Mauro -- To unsubscribe from this list: send the line unsubscribe linux-media in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
