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?
diff --git a/net/rfkill/rfkill-input.c b/net/rfkill/rfkill-input.c
index e5b6955..de75934 100644
--- a/net/rfkill/rfkill-input.c
+++ b/net/rfkill/rfkill-input.c
@@ -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);
--
Matthew Garrett | [email protected]
Matthew Garrett 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]>
Works for me (saves a huge wait to be able to do wifi toggling via a
hotkey).
Tested-by: Sitsofe Wheeler <[email protected]>
--
Sitsofe | http://sucs.org/~sits/
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]>
>
>
> diff --git a/net/rfkill/rfkill-input.c b/net/rfkill/rfkill-input.c
> index e5b6955..de75934 100644
> --- a/net/rfkill/rfkill-input.c
> +++ b/net/rfkill/rfkill-input.c
> @@ -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.
> .27 material?
yup.
On Sat, Oct 04, 2008 at 03:39:29PM -0700, Andrew Morton wrote:
> 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.
--
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 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.
diff --git a/net/rfkill/rfkill-input.c b/net/rfkill/rfkill-input.c
index e5b6955..86197bb 100644
--- a/net/rfkill/rfkill-input.c
+++ b/net/rfkill/rfkill-input.c
@@ -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);
}
--
Matthew Garrett | [email protected]
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.
>
> 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.
> diff --git a/net/rfkill/rfkill-input.c b/net/rfkill/rfkill-input.c
> index e5b6955..86197bb 100644
> --- a/net/rfkill/rfkill-input.c
> +++ b/net/rfkill/rfkill-input.c
> @@ -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.
On Sat, Oct 04, 2008 at 09:43:41PM -0700, Andrew Morton wrote:
> Jiffies wraparound is 49.7 days at HZ=1000.
Damn those pesky 32-bit systems.
> 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.
--
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 correctly.
Signed-off-by: Matthew Garrett <[email protected]>
---
Document the requirement to initialise the struct.
diff --git a/net/rfkill/rfkill-input.c b/net/rfkill/rfkill-input.c
index e5b6955..6c7baa0 100644
--- a/net/rfkill/rfkill-input.c
+++ b/net/rfkill/rfkill-input.c
@@ -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);
}
--
Matthew Garrett | [email protected]
On Sun, 05 Oct 2008, Matthew Garrett 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]>
FWIW:
Acked-by: Henrique de Moraes Holschuh <[email protected]>
--
"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
On Sunday 05 October 2008, Matthew Garrett 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]>
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]>
> ---
>
> Document the requirement to initialise the struct.
>
> diff --git a/net/rfkill/rfkill-input.c b/net/rfkill/rfkill-input.c
> index e5b6955..6c7baa0 100644
> --- a/net/rfkill/rfkill-input.c
> +++ b/net/rfkill/rfkill-input.c
> @@ -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);
> }
>
>
Matthew Garrett 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.
This patch also works for me.
Tested-by: Sitsofe Wheeler <[email protected]>
--
Sitsofe | http://sucs.org/~sits/
Hi Matthew,
On Sun, Oct 05, 2008 at 12:02:26PM +0100, Matthew Garrett 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]>
>
I'd rather have something like below. It should probably go through
David's (networking) tree. since it is in net/.
--
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(-)
diff --git a/net/rfkill/rfkill-input.c b/net/rfkill/rfkill-input.c
index e5b6955..775c6ba 100644
--- a/net/rfkill/rfkill-input.c
+++ b/net/rfkill/rfkill-input.c
@@ -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);
}
On Sun, Oct 05, 2008 at 05:24:39PM -0400, Dmitry Torokhov wrote:
> 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?
--
Matthew Garrett | [email protected]
On Sun, 05 Oct 2008, Ivo van Doorn wrote:
> 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.
--
"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
Matthew Garrett wrote:
> On Sun, Oct 05, 2008 at 05:24:39PM -0400, Dmitry Torokhov wrote:
>
>> 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]>
--
Sitsofe | http://sucs.org/~sits/
On Monday 06 October 2008, Sitsofe Wheeler wrote:
> Matthew Garrett wrote:
> > On Sun, Oct 05, 2008 at 05:24:39PM -0400, Dmitry Torokhov wrote:
> >
> >> 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]>