rfkill-input doesn't work until 5 minutes after boot

PATCH Superseded Matthew Garrett 6 participants 16 messages Oct 4, 2008 – Oct 6, 2008 Rfkill

lore

Share & tools

lore

Matthew Garrett

author

2008-10-04 20:44 UTC #0 lore raw

PATCH 1/1 rfkill-input doesn't work until 5 minutes after boot

rfkill-input implements debounce as follows:

        if (time_after(jiffies, task->last + msecs_to_jiffies(200))) {

However, task->last is initialised to 0 while jiffies starts at -300*HZ. Any input within 5 minutes of kernel start is therefore ignored. Fix by initialising task->last correctly.

Signed-off-by Matthew Garrett <[email protected]>

---

.27 material?

net/rfkill/rfkill-input.c +1 -0

index e5b6955..de75934 100644
@@ -101,6 +101,7 @@ static void rfkill_schedule_toggle(struct rfkill_task *task)
 		.mutex = __MUTEX_INITIALIZER(n.mutex),		\
 		.lock = __SPIN_LOCK_UNLOCKED(n.lock),		\
 		.desired_state = RFKILL_STATE_UNBLOCKED,	\
+		.last = INITIAL_JIFFIES,			\
 	}
 
 static DEFINE_RFKILL_TASK(rfkill_wlan, RFKILL_TYPE_WLAN);

Signature · 1 line

Matthew Garrett | [email protected]

Sitsofe Wheeler

Tested-by on v1 1/1

2008-10-04 22:03 UTC #1 lore raw

Quoted 7 lines · Matthew Garrett

rfkill-input implements debounce as follows:
        if (time_after(jiffies, task->last + msecs_to_jiffies(200))) {
However, task->last is initialised to 0 while jiffies starts at -300*HZ. Any input within 5 minutes of kernel start is therefore ignored. Fix by initialising task->last correctly.
Signed-off-by Matthew Garrett <[email protected]>

Works for me (saves a huge wait to be able to do wifi toggling via a hotkey).

Tested-by Sitsofe Wheeler <[email protected]>

Signature · 1 line

Sitsofe | http://sucs.org/~sits/

Andrew Morton

2008-10-04 22:40 UTC #2 lore raw

In reply to Matthew Garrett

Quoted 17 lines · On Sat, 4 Oct 2008 21:43:43 +0100 Matthew Garrett <[email protected]> wrote:

rfkill-input implements debounce as follows:
        if (time_after(jiffies, task->last + msecs_to_jiffies(200))) {
However, task->last is initialised to 0 while jiffies starts at -300*HZ. Any input within 5 minutes of kernel start is therefore ignored. Fix by initialising task->last correctly.
Signed-off-by Matthew Garrett <[email protected]>
net/rfkill/rfkill-input.c +0 -0
index e5b6955..de75934 100644

net/rfkill/rfkill-input.c static void rfkill_schedule_toggle(struct rfkill_task *task) · line 107

Quoted 7 lines

net/rfkill/rfkill-input.c +1 -0

@@ -101,6 +101,7 @@ static void rfkill_schedule_toggle(struct rfkill_task *task)
 		.mutex = __MUTEX_INITIALIZER(n.mutex),		\
 		.lock = __SPIN_LOCK_UNLOCKED(n.lock),		\
 		.desired_state = RFKILL_STATE_UNBLOCKED,	\
+		.last = INITIAL_JIFFIES,			\
 	}
 
 static DEFINE_RFKILL_TASK(rfkill_wlan, RFKILL_TYPE_WLAN);

That'll only work as intended if CONFIG_RFKILL_INPUT=y? If the module is loaded 10 minutes after boot, the timestamp is still wrong. It might happily happen to work, but will still fail after 2^31 jiffies (or something like that).

Generally speaking, INITIAL_JIFFIES is a secret internal debugging detail and its use out in general kernel code is a red flag.

I think this initialisation should be done at runtime somehow.

Quoted 1 line

.27 material?

yup.

Matthew Garrett

author

2008-10-04 22:50 UTC #3 lore raw

Quoted 4 lines · Andrew Morton

That'll only work as intended if CONFIG_RFKILL_INPUT=y? If the module is loaded 10 minutes after boot, the timestamp is still wrong. It might happily happen to work, but will still fail after 2^31 jiffies (or something like that).

Mm. True. I'll fix it up to do it at load time.

Signature · 1 line

Matthew Garrett | [email protected]

Matthew Garrett

author

2008-10-05 00:43 UTC #4 lore raw

PATCH v2 1/1 rfkill-input doesn't work until 5 minutes after boot

In reply to Matthew Garrett

rfkill-input implements debounce as follows:

        if (time_after(jiffies, task->last + msecs_to_jiffies(200))) {

However, task->last is initialised to 0 while jiffies starts at -300*HZ. Any input within 5 minutes of kernel start is therefore ignored. Fix by initialising task->last correctly.

Signed-off-by Matthew Garrett <[email protected]>

---

Set the last event value at module load time, since otherwise we'll have a window of failure if someone loads the module in a few hundred million years. I look forward to being rewarded by the post-humans for caring so much about them.

net/rfkill/rfkill-input.c +5 -0

index e5b6955..86197bb 100644
@@ -255,6 +255,11 @@ static struct input_handler rfkill_handler = {
 
 static int __init rfkill_handler_init(void)
 {
+	rfkill_wlan.last = jiffies - HZ/5;
+	rfkill_bt.last = jiffies - HZ/5;
+	rfkill_uwb.last = jiffies - HZ/5;
+	rfkill_wimax.last = jiffies - HZ/5;
+	rfkill_wwan.last = jiffies - HZ/5;
 	return input_register_handler(&rfkill_handler);
 }

Signature · 1 line

Matthew Garrett | [email protected]

Andrew Morton

2008-10-05 04:44 UTC #5 lore raw

Quoted 8 lines · On Sun, 5 Oct 2008 01:43:34 +0100 Matthew Garrett <[email protected]> wrote:

rfkill-input implements debounce as follows:
        if (time_after(jiffies, task->last + msecs_to_jiffies(200))) {
However, task->last is initialised to 0 while jiffies starts at -300*HZ. Any input within 5 minutes of kernel start is therefore ignored. Fix by initialising task->last correctly.

Quoted 8 lines

Signed-off-by Matthew Garrett <[email protected]>
---
Set the last event value at module load time, since otherwise we'll have a window of failure if someone loads the module in a few hundred million years. I look forward to being rewarded by the post-humans for caring so much about them.

Jiffies wraparound is 49.7 days at HZ=1000.

Quoted 5 lines

net/rfkill/rfkill-input.c +0 -0
index e5b6955..86197bb 100644

net/rfkill/rfkill-input.c static struct input_handler rfkill_handler = { · line 264

Quoted 10 lines

net/rfkill/rfkill-input.c +5 -0

@@ -255,6 +255,11 @@ static struct input_handler rfkill_handler = {
 
 static int __init rfkill_handler_init(void)
 {
+	rfkill_wlan.last = jiffies - HZ/5;
+	rfkill_bt.last = jiffies - HZ/5;
+	rfkill_uwb.last = jiffies - HZ/5;
+	rfkill_wimax.last = jiffies - HZ/5;
+	rfkill_wwan.last = jiffies - HZ/5;
 	return input_register_handler(&rfkill_handler);
 }

If someone adds a new rfkill_foo there's a risk that they'll forget to make the corresponding change here. A comment at the definition sites would help.

Or, better, do something like

static struct rfkill_task rfkill_tasks[] = {
	DEFINE_RFKILL_TASK(RFKILL_TYPE_WLAN),
	...
};

#define rfkill_wlan rfkill_tasks[0]
...

	for (i = 0; i < ARRAY_SIZE(rfkill_tasks); i++)
		...

but the new definition of DEFINE_RFKILL_TASK gets tricky.

Matthew Garrett

author

2008-10-05 10:59 UTC #6 lore raw

Quoted 1 line · Andrew Morton

Jiffies wraparound is 49.7 days at HZ=1000.

Damn those pesky 32-bit systems.

Quoted 3 lines

If someone adds a new rfkill_foo there's a risk that they'll forget to make the corresponding change here. A comment at the definition sites would help.

Sure.

Signature · 1 line

Matthew Garrett | [email protected]

Matthew Garrett

author

2008-10-05 11:02 UTC #7 lore raw

PATCH v3 1/1 rfkill-input doesn't work until 5 minutes after boot

In reply to Matthew Garrett

rfkill-input implements debounce as follows:

        if (time_after(jiffies, task->last + msecs_to_jiffies(200))) {

However, task->last is initialised to 0 while jiffies starts at -300*HZ. Any input within 5 minutes of kernel start is therefore ignored. Fix by initialising task->last correctly.

Signed-off-by Matthew Garrett <[email protected]>

---

Document the requirement to initialise the struct.

net/rfkill/rfkill-input.c +7 -0

index e5b6955..6c7baa0 100644
@@ -103,6 +103,8 @@ static void rfkill_schedule_toggle(struct rfkill_task *task)
 		.desired_state = RFKILL_STATE_UNBLOCKED,	\
 	}
 
+/* Remember to initialise these in rfkill_handler_init if you add any */
+
 static DEFINE_RFKILL_TASK(rfkill_wlan, RFKILL_TYPE_WLAN);
 static DEFINE_RFKILL_TASK(rfkill_bt, RFKILL_TYPE_BLUETOOTH);
 static DEFINE_RFKILL_TASK(rfkill_uwb, RFKILL_TYPE_UWB);

@@ -255,6 +257,11 @@ static struct input_handler rfkill_handler = {
 
 static int __init rfkill_handler_init(void)
 {
+	rfkill_wlan.last = jiffies - HZ/5;
+	rfkill_bt.last = jiffies - HZ/5;
+	rfkill_uwb.last = jiffies - HZ/5;
+	rfkill_wimax.last = jiffies - HZ/5;
+	rfkill_wwan.last = jiffies - HZ/5;
 	return input_register_handler(&rfkill_handler);
 }

Signature · 1 line

Matthew Garrett | [email protected]

Henrique de Moraes Holschuh

Acked-by on v3 1/1

2008-10-05 13:51 UTC #8 lore raw

Quoted 9 lines · Matthew Garrett

rfkill-input implements debounce as follows:
        if (time_after(jiffies, task->last + msecs_to_jiffies(200))) {
However, task->last is initialised to 0 while jiffies starts at -300*HZ. Any input within 5 minutes of kernel start is therefore ignored. Fix by initialising task->last correctly.
Signed-off-by Matthew Garrett <[email protected]>

FWIW:

Acked-by Henrique de Moraes Holschuh <[email protected]>

Signature · 4 lines

  "One disk to rule them all, One disk to find them. One disk to bring
  them all and in the darkness grind them. In the Land of Redmond
  where the shadows lie." -- The Silicon Valley Tarot
  Henrique Holschuh

Ivo van Doorn

Acked-by on v3 1/1

2008-10-05 14:31 UTC #9 lore raw

In reply to Matthew Garrett

Quoted 9 lines · Matthew Garrett

rfkill-input implements debounce as follows:
        if (time_after(jiffies, task->last + msecs_to_jiffies(200))) {
However, task->last is initialised to 0 while jiffies starts at -300*HZ. Any input within 5 minutes of kernel start is therefore ignored. Fix by initialising task->last correctly.
Signed-off-by Matthew Garrett <[email protected]>

I am not too happy about the multiple jiffies - HZ/5 statements regardless of the comment that it should be updated when new tasks are defined.

But on the other hand I haven't seen any real alternatives yet either, so I have no objections against this patch either.

Acked-by Ivo van Doorn <[email protected]>

Quoted 30 lines

---

Document the requirement to initialise the struct.

net/rfkill/rfkill-input.c +7 -0

index e5b6955..6c7baa0 100644
@@ -103,6 +103,8 @@ static void rfkill_schedule_toggle(struct rfkill_task *task)
 		.desired_state = RFKILL_STATE_UNBLOCKED,	\
 	}
 
+/* Remember to initialise these in rfkill_handler_init if you add any */
+
 static DEFINE_RFKILL_TASK(rfkill_wlan, RFKILL_TYPE_WLAN);
 static DEFINE_RFKILL_TASK(rfkill_bt, RFKILL_TYPE_BLUETOOTH);
 static DEFINE_RFKILL_TASK(rfkill_uwb, RFKILL_TYPE_UWB);

@@ -255,6 +257,11 @@ static struct input_handler rfkill_handler = {
 
 static int __init rfkill_handler_init(void)
 {
+	rfkill_wlan.last = jiffies - HZ/5;
+	rfkill_bt.last = jiffies - HZ/5;
+	rfkill_uwb.last = jiffies - HZ/5;
+	rfkill_wimax.last = jiffies - HZ/5;
+	rfkill_wwan.last = jiffies - HZ/5;
 	return input_register_handler(&rfkill_handler);
 }

Henrique de Moraes Holschuh

2008-10-06 02:16 UTC #10 lore raw

Quoted 6 lines · Ivo van Doorn

I am not too happy about the multiple jiffies - HZ/5 statements regardless of the comment that it should be updated when new tasks are defined.
But on the other hand I haven't seen any real alternatives yet either, so I have no objections against this patch either.

It doesn't matter much. The next rfkill patchset will remove most of that code, anyway (and yes, I *will* make sure it doesn't have the same issue this patch is fixing). But that's 2.6.28/2.6.29 stuff, probably 2.6.29.

I didn't send them yet because I was still changing other parts of the code too much. I may send them soon as RFC.

In other words, don't worry too much about that maintenance annoyance in rfkill-input. It will not be around for long.

Signature · 4 lines

  "One disk to rule them all, One disk to find them. One disk to bring
  them all and in the darkness grind them. In the Land of Redmond
  where the shadows lie." -- The Silicon Valley Tarot
  Henrique Holschuh

Sitsofe Wheeler

Tested-by on v3 1/1

2008-10-05 19:04 UTC #11 lore raw

In reply to Matthew Garrett

Quoted 5 lines · Matthew Garrett

rfkill-input implements debounce as follows:
        if (time_after(jiffies, task->last + msecs_to_jiffies(200))) {
However, task->last is initialised to 0 while jiffies starts at -300*HZ. Any input within 5 minutes of kernel start is therefore ignored. Fix by initialising task->last correctly.

This patch also works for me.

Tested-by Sitsofe Wheeler <[email protected]>

Signature · 1 line

Sitsofe | http://sucs.org/~sits/

Dmitry Torokhov

2008-10-05 21:24 UTC #12 lore raw

In reply to Matthew Garrett

Hi Matthew,

Quoted 10 lines · Matthew Garrett

rfkill-input implements debounce as follows:
        if (time_after(jiffies, task->last + msecs_to_jiffies(200))) {
However, task->last is initialised to 0 while jiffies starts at -300*HZ. Any input within 5 minutes of kernel start is therefore ignored. Fix by initialising task->last correctly.
Signed-off-by Matthew Garrett <[email protected]>

I'd rather have something like below. It should probably go through David's (networking) tree. since it is in net/.

Signature · 20 lines

Dmitry

Input: rfkill-input doesn't work until 5 minutes after boot

From: Matthew Garrett <[email protected]>

rfkill-input implements debounce as follows:

        if (time_after(jiffies, task->last + msecs_to_jiffies(200))) {

However, task->last is initialised to 0 while jiffies starts at -300*HZ.
Any input within 5 minutes of kernel start is therefore ignored. Fix by
initialising task->last at module load.

Signed-off-by: Matthew Garrett <[email protected]>
Signed-off-by: Dmitry Torokhov <[email protected]>
---

 net/rfkill/rfkill-input.c |   28 ++++++++++++++++++----------
 1 files changed, 18 insertions(+), 10 deletions(-)

net/rfkill/rfkill-input.c +18 ~~-10~~

index e5b6955..775c6ba 100644
@@ -109,21 +109,24 @@ static DEFINE_RFKILL_TASK(rfkill_uwb, RFKILL_TYPE_UWB);
 static DEFINE_RFKILL_TASK(rfkill_wimax, RFKILL_TYPE_WIMAX);
 static DEFINE_RFKILL_TASK(rfkill_wwan, RFKILL_TYPE_WWAN);
 
+static struct rfkill_task *rfkill_tasks[] = {
+	&rfkill_wlan,
+	&rfkill_bt,
+	&rfkill_uwb,
+	&rfkill_wimax,
+	&rfkill_wwan,
+};
+
 static void rfkill_schedule_evsw_rfkillall(int state)
 {
+	int i;
+
 	/* EVERY radio type. state != 0 means radios ON */
 	/* handle EPO (emergency power off) through shortcut */
 	if (state) {
-		rfkill_schedule_set(&rfkill_wwan,
-				    RFKILL_STATE_UNBLOCKED);
-		rfkill_schedule_set(&rfkill_wimax,
-				    RFKILL_STATE_UNBLOCKED);
-		rfkill_schedule_set(&rfkill_uwb,
-				    RFKILL_STATE_UNBLOCKED);
-		rfkill_schedule_set(&rfkill_bt,
-				    RFKILL_STATE_UNBLOCKED);
-		rfkill_schedule_set(&rfkill_wlan,
-				    RFKILL_STATE_UNBLOCKED);
+		for (i = 0; i < ARRAY_SIZE(rfkill_tasks); i++)
+			rfkill_schedule_set(rfkill_tasks[i],
+					    RFKILL_STATE_UNBLOCKED);
 	} else
 		rfkill_schedule_epo();
 }

@@ -255,6 +258,11 @@ static struct input_handler rfkill_handler = {
 
 static int __init rfkill_handler_init(void)
 {
+	int i;
+
+	for (i = 0; i < ARRAY_SIZE(rfkill_tasks); i++)
+		rfkill_tasks[i]->last = jiffies - HZ / 5;
+
 	return input_register_handler(&rfkill_handler);
 }

Matthew Garrett

author

2008-10-05 21:33 UTC #13 lore raw

Quoted 2 lines · Dmitry Torokhov

I'd rather have something like below. It should probably go through David's (networking) tree. since it is in net/.

Fine with me, as long as Sitsofe can confirm it works?

Signature · 1 line

Matthew Garrett | [email protected]

Sitsofe Wheeler

Tested-by

2008-10-06 05:51 UTC #14 lore raw

Quoted 6 lines · Matthew Garrett

Quoted 2 lines · Dmitry Torokhov
I'd rather have something like below. It should probably go through David's (networking) tree. since it is in net/.
Fine with me, as long as Sitsofe can confirm it works?

Dmitry's patch also works for me.

Tested-by Sitsofe Wheeler <[email protected]>

Signature · 1 line

Sitsofe | http://sucs.org/~sits/

Ivo van Doorn

Acked-by

2008-10-06 16:31 UTC #15 lore raw

Quoted 11 lines · Sitsofe Wheeler

Quoted 6 lines · Matthew Garrett
Quoted 2 lines · Dmitry Torokhov
I'd rather have something like below. It should probably go through David's (networking) tree. since it is in net/.
Fine with me, as long as Sitsofe can confirm it works?
Dmitry's patch also works for me.
Tested-by Sitsofe Wheeler <[email protected]>

Dmitry's patch has my personal preference, because it decreases the change of forgetting the initialization.

Acked-by Ivo van Doorn <[email protected]>