2.6.20-rc6 pb_fnmode regression

2.6.20-rc6 pb_fnmode regression

[Soeren Sonnenburg]

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.

Re: 2.6.20-rc6 pb_fnmode regression

[Jiri Kosina]

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

Re: [linux-usb-devel] 2.6.20-rc6 pb_fnmode regression

[Sergey Vlasov]

[-- Attachment #1: Type: text/plain, Size: 610 bytes --]

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);

[-- Attachment #2: Type: application/pgp-signature, Size: 189 bytes --]

Re: 2.6.20-rc6 pb_fnmode regression

[Soeren Sonnenburg]

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.

Re: 2.6.20-rc6 pb_fnmode regression

[Jiri Kosina]

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

Re: 2.6.20-rc6 pb_fnmode regression

[Jiri Kosina]

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

Re: 2.6.20-rc6 pb_fnmode regression

[Soeren Sonnenburg]

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.

Re: 2.6.20-rc6 pb_fnmode regression

[Jiri Kosina]

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 <jkosina@suse.cz>

---
commit f5d9972112d5a78233cae97d7b162d5e33389026
tree cf8140e884ad2fe11212579e1ba91a925da87e58
parent 99abfeafb5f2eea1bb481330ff37343e1133c924
author Jiri Kosina <jkosina@suse.cz> Mon, 29 Jan 2007 12:44:41 +0100
committer Jiri Kosina <jkosina@suse.cz> 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 <linux/hid.h>
 
+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 unk	KEY_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

Re: 2.6.20-rc6 pb_fnmode regression

[Soeren Sonnenburg]

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.