Re: 2.6.20-rc6 pb_fnmode regression
On Mon, 2007-01-29 at 12:45 +0100, Jiri Kosina wrote: > On Mon, 29 Jan 2007, Soeren Sonnenburg wrote: > > > That sounds good for me. Breaking with what was there is not a problem > > as long as this feature is still there, it can be done in a more clean > > way this way, and the new /sys/foo/bar path is documented (basically > > people nowadays expect slight user interface changes between kernel > > versions). > > So, does the patch below look OK to powerbook people? The only difference > is that the module taking care of pb_fnmode parameter is now hid, instead For me yes ... I just rebooted and checked fn_modes ... it works nicely. So I guess this should be applied ?! Soeren -- Sometimes, there's a moment as you're waking, when you become aware of the real world around you, but you're still dreaming. - To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: 2.6.20-rc6 pb_fnmode regression
On Mon, 29 Jan 2007, Soeren Sonnenburg wrote: > That sounds good for me. Breaking with what was there is not a problem > as long as this feature is still there, it can be done in a more clean > way this way, and the new /sys/foo/bar path is documented (basically > people nowadays expect slight user interface changes between kernel > versions). So, does the patch below look OK to powerbook people? The only difference is that the module taking care of pb_fnmode parameter is now hid, instead of usbhid. If it is OK I will probably queue it as a bugfix for 2.6.20-rc6, as it seems that quite a lot of users got used to be able to change pb_fnmode value through sysfs. [PATCH] HID: pb_fnmode fix and move it to generic HID The apple powerbook people are used to switch the pb_fnmode setting at runtime through writing to sysfs, altering the module parameter value. This was broken for them in 2.6.20-rc1 when generic HID layer was introduced, as the pb_fnmode flag was made per-hiddevice, instead of global variable. This patch moves the pb_fnmode module parameter from usbhid module to hid module, but apart from that retains backward compatibility with respect to changing the mode through sysfs. Signed-off-by: Jiri Kosina <[EMAIL PROTECTED]> --- commit f5d9972112d5a78233cae97d7b162d5e33389026 tree cf8140e884ad2fe11212579e1ba91a925da87e58 parent 99abfeafb5f2eea1bb481330ff37343e1133c924 author Jiri Kosina <[EMAIL PROTECTED]> Mon, 29 Jan 2007 12:44:41 +0100 committer Jiri Kosina <[EMAIL PROTECTED]> Mon, 29 Jan 2007 12:44:41 +0100 drivers/hid/hid-input.c | 11 --- drivers/usb/input/hid-core.c |9 - include/linux/hid.h |1 - 3 files changed, 8 insertions(+), 13 deletions(-) diff --git a/drivers/hid/hid-input.c b/drivers/hid/hid-input.c index 9cf591a..54b0b95 100644 --- a/drivers/hid/hid-input.c +++ b/drivers/hid/hid-input.c @@ -35,6 +35,11 @@ #include +static int hid_pb_fnmode = 1; +module_param_named(pb_fnmode, hid_pb_fnmode, int, 0644); +MODULE_PARM_DESC(pb_fnmode, + "Mode of fn key on PowerBooks (0 = disabled, 1 = fkeyslast, 2 = fkeysfirst)"); + #define unkKEY_UNKNOWN static const unsigned char hid_keyboard[256] = { @@ -154,7 +159,7 @@ static int hidinput_pb_event(struct hid_device *hid, struct input_dev *input, return 1; } - if (hid->pb_fnmode) { + if (hid_pb_fnmode) { int do_translate; trans = find_translation(powerbook_fn_keys, usage->code); @@ -163,8 +168,8 @@ static int hidinput_pb_event(struct hid_device *hid, struct input_dev *input, do_translate = 1; else if (trans->flags & POWERBOOK_FLAG_FKEY) do_translate = - (hid->pb_fnmode == 2 && (hid->quirks & HID_QUIRK_POWERBOOK_FN_ON)) || - (hid->pb_fnmode == 1 && !(hid->quirks & HID_QUIRK_POWERBOOK_FN_ON)); + (hid_pb_fnmode == 2 && (hid->quirks & HID_QUIRK_POWERBOOK_FN_ON)) || + (hid_pb_fnmode == 1 && !(hid->quirks & HID_QUIRK_POWERBOOK_FN_ON)); else do_translate = (hid->quirks & HID_QUIRK_POWERBOOK_FN_ON); diff --git a/drivers/usb/input/hid-core.c b/drivers/usb/input/hid-core.c index ea3636d..a2ee707 100644 --- a/drivers/usb/input/hid-core.c +++ b/drivers/usb/input/hid-core.c @@ -56,11 +56,6 @@ static unsigned int hid_mousepoll_interval; module_param_named(mousepoll, hid_mousepoll_interval, uint, 0644); MODULE_PARM_DESC(mousepoll, "Polling interval of mice"); -static int usbhid_pb_fnmode = 1; -module_param_named(pb_fnmode, usbhid_pb_fnmode, int, 0644); -MODULE_PARM_DESC(pb_fnmode, - "Mode of fn key on PowerBooks (0 = disabled, 1 = fkeyslast, 2 = fkeysfirst)"); - /* * Input submission and I/O error handler. */ @@ -1249,10 +1244,6 @@ static struct hid_device *usb_hid_configure(struct usb_interface *intf) hid->hiddev_hid_event = hiddev_hid_event; hid->hiddev_report_event = hiddev_report_event; #endif -#ifdef CONFIG_USB_HIDINPUT_POWERBOOK - hid->pb_fnmode = usbhid_pb_fnmode; -#endif - return hid; fail: diff --git a/include/linux/hid.h b/include/linux/hid.h index 770120a..342b4e6 100644 --- a/include/linux/hid.h +++ b/include/linux/hid.h @@ -438,7 +438,6 @@ struct hid_device { /* device report descriptor */ struct hid_usage *, __s32); void (*hiddev_report_event) (struct hid_device *, struct hid_report *); #ifdef CONFIG_USB_HIDINPUT_POWERBOOK - unsigned int pb_fnmode; unsigned long pb_pressed_fn[NBITS(KEY_MAX)]; unsigned long pb_pressed_numlock[NBITS(KEY_MAX)]; #endif - To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the bo
Re: 2.6.20-rc6 pb_fnmode regression
On Mon, 2007-01-29 at 12:13 +0100, Jiri Kosina wrote: > On Mon, 29 Jan 2007, Jiri Kosina wrote: > > > Ah, now I see. The problem is that in pre-2.6.20-rc1 the pb_fnmode was > > setting global variable, but after the HID layer rework, this is a > > per-hid variable, which is of course not updated when write to sysfs > > triggers. I will try to fix this before I send 2.6.20-rc6 updates to > > Linus, thanks for pointing this out. > > Actually the cleanest solution would be when I change the code in such a > way that pb_fnmode parameter would be passed to hid instead of usbhid > module, as this is where the input mapping is being done (you could > potentially have a keyboard which needs the very same handling of fn mode > as usb powerbook keyboards currently have, but on different transport > - input mapping is logically transport independent). > > But I guess you will be not OK with breaking the backward compatibility in > such way, because all the already existing tutorials, etc. right? That sounds good for me. Breaking with what was there is not a problem as long as this feature is still there, it can be done in a more clean way this way, and the new /sys/foo/bar path is documented (basically people nowadays expect slight user interface changes between kernel versions). > Would warning that would trigger when the module parameter is passed to > usbhid and would instruct user to pass the parameter to hid module > instead, be acceptable? (and then changing the parameter of hid module > through sysfs would work as expected again). I guess this warning is not too useful, except if it is triggered on echo >/sys/*/pb_fnmode too (which I suspect is what most people do). Soeren -- Sometimes, there's a moment as you're waking, when you become aware of the real world around you, but you're still dreaming. - To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: 2.6.20-rc6 pb_fnmode regression
On Mon, 29 Jan 2007, Jiri Kosina wrote: > Ah, now I see. The problem is that in pre-2.6.20-rc1 the pb_fnmode was > setting global variable, but after the HID layer rework, this is a > per-hid variable, which is of course not updated when write to sysfs > triggers. I will try to fix this before I send 2.6.20-rc6 updates to > Linus, thanks for pointing this out. Actually the cleanest solution would be when I change the code in such a way that pb_fnmode parameter would be passed to hid instead of usbhid module, as this is where the input mapping is being done (you could potentially have a keyboard which needs the very same handling of fn mode as usb powerbook keyboards currently have, but on different transport - input mapping is logically transport independent). But I guess you will be not OK with breaking the backward compatibility in such way, because all the already existing tutorials, etc. right? Would warning that would trigger when the module parameter is passed to usbhid and would instruct user to pass the parameter to hid module instead, be acceptable? (and then changing the parameter of hid module through sysfs would work as expected again). Thanks, -- Jiri Kosina - To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [linux-usb-devel] 2.6.20-rc6 pb_fnmode regression
On Mon, Jan 29, 2007 at 10:55:48AM +0100, Jiri Kosina wrote: > Changing module parameter values through sysfs is not a very nice idea, > because the change of the value is indeed silent - the driver is not > notified in any way, that the value has changed. So the driver should take > care of it by itself, which is not a nice thing. There is module_param_call() - used at least by drivers/md/md.c: static int get_ro(char *buffer, struct kernel_param *kp) ... static int set_ro(const char *val, struct kernel_param *kp) ... module_param_call(start_ro, set_ro, get_ro, NULL, S_IRUSR|S_IWUSR); pgpRrgp6hqJW1.pgp Description: PGP signature
Re: 2.6.20-rc6 pb_fnmode regression
On Mon, 29 Jan 2007, Soeren Sonnenburg wrote: > Well I need in-kernel usbhid and the way this was implemented in 2.6.19 > (and before) one could change pb_fnmode on-the-fly. This is mentioned in > all the power/i/mac/book tutorials and everyone is used to switching > modes this way. I can happily patch the kernel to use the pb_fnmode but > nonetheless this is a regression to pre 2.6.20* and will confuse others > too... Ah, now I see. The problem is that in pre-2.6.20-rc1 the pb_fnmode was setting global variable, but after the HID layer rework, this is a per-hid variable, which is of course not updated when write to sysfs triggers. I will try to fix this before I send 2.6.20-rc6 updates to Linus, thanks for pointing this out. -- Jiri Kosina - To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: 2.6.20-rc6 pb_fnmode regression
On Mon, 2007-01-29 at 10:55 +0100, Jiri Kosina wrote: > On Sat, 27 Jan 2007, Soeren Sonnenburg wrote: > > > I realized that any setting to /sys/module/usbhid/parameters/pb_fnmode > > is just ignored until the machine does a suspend-resume cycle. [...] > I would rather be inclined to just make the > /sys/module/usbhid/parameters/pb_fnmode read-only (which is what most of > the drivers do anyway), to avoid this kind of confusion. > > Do you have really any strong use-case, when setting the parameter during > runtime would be much more useful than just do it during modprobe or > rmmod/modprobe cycle? Well I need in-kernel usbhid and the way this was implemented in 2.6.19 (and before) one could change pb_fnmode on-the-fly. This is mentioned in all the power/i/mac/book tutorials and everyone is used to switching modes this way. I can happily patch the kernel to use the pb_fnmode but nonetheless this is a regression to pre 2.6.20* and will confuse others too... Soeren -- Sometimes, there's a moment as you're waking, when you become aware of the real world around you, but you're still dreaming. - To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: 2.6.20-rc6 pb_fnmode regression
On Sat, 27 Jan 2007, Soeren Sonnenburg wrote: > I realized that any setting to /sys/module/usbhid/parameters/pb_fnmode > is just ignored until the machine does a suspend-resume cycle. Hi Soeren, I would probably not call this a regression, as this has been always there, as far as I can see. > I've added a printk in drivers/usb/input/hid-core.c (which is the only > place where hid->pb_fnmode is set) and indeed only on module load ( in > my case usbhid is compiled into the kernel - so on kernel boot) any > change to hid>pb_fnmode is done. Adding a printk to hidinput_pb_event() > in drivers/hid/hid-input.c says the same: hid->pb_fnmode cannot be > changed on the fly anymore... HOWEVER: If I s2ram the machine > hid->pb_fnmode is initialized with the value I put into > /sys/module/usbhid/parameters/pb_fnmode . As I have no idea how/whether > sysfs works/is possible I now hope someone more knowledgable than me can > resolve this issue! Changing module parameter values through sysfs is not a very nice idea, because the change of the value is indeed silent - the driver is not notified in any way, that the value has changed. So the driver should take care of it by itself, which is not a nice thing. The fact that during suspend/resume cycle it works is caused by the fact that all the hid devices are reinitialized, and therefore the hid->pb_fnmode is reassigned a new value, which has eventually been changed through sysfs. I would rather be inclined to just make the /sys/module/usbhid/parameters/pb_fnmode read-only (which is what most of the drivers do anyway), to avoid this kind of confusion. Do you have really any strong use-case, when setting the parameter during runtime would be much more useful than just do it during modprobe or rmmod/modprobe cycle? Thanks, -- Jiri Kosina - To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
2.6.20-rc6 pb_fnmode regression
Dear all, I realized that any setting to /sys/module/usbhid/parameters/pb_fnmode is just ignored until the machine does a suspend-resume cycle. I've added a printk in drivers/usb/input/hid-core.c (which is the only place where hid->pb_fnmode is set) and indeed only on module load ( in my case usbhid is compiled into the kernel - so on kernel boot) any change to hid>pb_fnmode is done. Adding a printk to hidinput_pb_event() in drivers/hid/hid-input.c says the same: hid->pb_fnmode cannot be changed on the fly anymore... HOWEVER: If I s2ram the machine hid->pb_fnmode is initialized with the value I put into /sys/module/usbhid/parameters/pb_fnmode . As I have no idea how/whether sysfs works/is possible I now hope someone more knowledgable than me can resolve this issue! Soeren -- Sometimes, there's a moment as you're waking, when you become aware of the real world around you, but you're still dreaming. - To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/