linux-kernel.vger.kernel.org archive mirror
 help / color / mirror / Atom feed
* Wireless IBSS on Linux-2.6.34 broken by commit 3bbb9ec946428b96657126768f65487a48dd090c
@ 2010-05-23  4:58 Jeff Chua
  2010-05-23  6:21 ` Arjan van de Ven
  2010-05-23  9:49 ` Johannes Berg
  0 siblings, 2 replies; 9+ messages in thread
From: Jeff Chua @ 2010-05-23  4:58 UTC (permalink / raw)
  To: Arjan van de Ven, johnstul, Ingo Molnar, Andrew Morton,
	Thomas Gleixner, Intel Linux Wireless, John W. Linville,
	Frans Pop, Johannes Berg, Reinette Chatre, Linus Torvalds, lkml

Wireless IBSS on Linux-2.6.34 is broken. Reverting commit
3bbb9ec946428b96657126768f65487a48dd090c makes it work again. This was
tested by bisecting the kernel.

I'm using 6200 AGN ...

cfg80211: Calling CRDA to update world regulatory domain
iwlagn: Intel(R) Wireless WiFi Link AGN driver for Linux, in-tree:
iwlagn: Copyright(c) 2003-2010 Intel Corporation
iwlagn 0000:02:00.0: PCI INT A -> GSI 16 (level, low) -> IRQ 16
iwlagn 0000:02:00.0: setting latency timer to 64
iwlagn 0000:02:00.0: Detected Intel(R) Centrino(R) Advanced-N 6200 AGN, REV=0x74
iwlagn 0000:02:00.0: Tunable channels: 13 802.11bg, 24 802.11a channels
iwlagn 0000:02:00.0: irq 46 for MSI/MSI-X
iwlagn 0000:02:00.0: loaded firmware version 9.193.4.1 build 19710



So, where should the fix be?


Here's the commit ...

3bbb9ec946428b96657126768f65487a48dd090c is the first bad commit
commit 3bbb9ec946428b96657126768f65487a48dd090c
Author: Arjan van de Ven <arjan@linux.intel.com>
Date:   Thu Mar 11 14:04:36 2010 -0800

    timers: Introduce the concept of timer slack for legacy timers

    While HR timers have had the concept of timer slack for quite some time
    now, the legacy timers lacked this concept, and had to make do with
    round_jiffies() and friends.

    Timer slack is important for power management; grouping timers reduces the
    number of wakeups which in turn reduces power consumption.

    This patch introduces timer slack to the legacy timers using the following
    pieces:
    * A slack field in the timer struct
    * An api (set_timer_slack) that callers can use to set explicit timer slack
    * A default slack of 0.4% of the requested delay for callers that do not set
      any explicit slack
    * Rounding code that is part of mod_timer() that tries to
      group timers around jiffies values every 'power of two'
      (so quick timers will group around every 2, but longer timers
      will group around every 4, 8, 16, 32 etc)

    Signed-off-by: Arjan van de Ven <arjan@linux.intel.com>
    Cc: johnstul@us.ibm.com
    Cc: Ingo Molnar <mingo@elte.hu>
    Signed-off-by: Andrew Morton <akpm@linux-foundation.org>
    Signed-off-by: Thomas Gleixner <tglx@linutronix.de>




Thanks,
Jeff.

^ permalink raw reply	[flat|nested] 9+ messages in thread
* Re: Wireless IBSS on Linux-2.6.34 broken by commit 3bbb9ec946428b96657126768f65487a48dd090c
@ 2010-05-23 17:07 Jeff Chua
  0 siblings, 0 replies; 9+ messages in thread
From: Jeff Chua @ 2010-05-23 17:07 UTC (permalink / raw)
  To: Johannes Berg, Arjan van de Ven, johnstul@us.ibm.com, Ingo Molnar,
	Andrew Morton, Thomas Gleixner, Intel Linux Wireless,
	John W. Linville, Frans Pop, , Chatre, Reinette, Linus Torvalds,
	lkml


On Sun, May 23, 2010 at 5:49 PM, Johannes Berg <johannes.berg@intel.com> 
wrote:
> On Sun, 2010-05-23 at 05:58 +0100, Jeff Chua wrote:
>> Wireless IBSS on Linux-2.6.34 is broken. Reverting commit
>> 3bbb9ec946428b96657126768f65487a48dd090c makes it work again. This was
>> tested by bisecting the kernel.


Arjan, Johannes,

Here's the demsg with debug enabled ...


1) with bad commit reverted ...

2010-05-23T21:24:23.656671+08:00 boston kernel: cfg80211: Calling CRDA to 
update world regulatory domain
2010-05-23T21:24:23.720182+08:00 boston kernel: iwlagn: Intel(R) Wireless 
WiFi Link AGN driver for Linux, in-tree:
2010-05-23T21:24:23.720211+08:00 boston kernel: iwlagn: Copyright(c) 
2003-2010 Intel Corporation
2010-05-23T21:24:23.720215+08:00 boston kernel: iwlagn 0000:02:00.0: PCI 
INT A -> GSI 16 (level, low) -> IRQ 16
2010-05-23T21:24:23.720218+08:00 boston kernel: iwlagn 0000:02:00.0: 
setting latency timer to 64
2010-05-23T21:24:23.720222+08:00 boston kernel: iwlagn 0000:02:00.0: 
Detected Intel(R) Centrino(R) Advanced-N 6200 AGN, REV=0x74
2010-05-23T21:24:23.732171+08:00 boston kernel: iwlagn 0000:02:00.0: 
Tunable channels: 13 802.11bg, 24 802.11a channels
2010-05-23T21:24:23.732181+08:00 boston kernel: iwlagn 0000:02:00.0: irq 
46 for MSI/MSI-X
2010-05-23T21:24:23.820182+08:00 boston kernel: iwlagn 0000:02:00.0: 
loaded firmware version 9.193.4.1 build 19710
2010-05-23T21:24:23.848169+08:00 boston kernel: phy0: Selected rate 
control algorithm 'iwl-agn-rs'
2010-05-23T21:24:24.335667+08:00 boston kernel: phy0: device now idle
2010-05-23T21:24:24.340162+08:00 boston kernel: phy0: device no longer 
idle - in use
2010-05-23T21:24:24.384668+08:00 boston kernel: wlan0: sta_find_ibss 
(active_ibss=0)
2010-05-23T21:24:24.384678+08:00 boston kernel:   did not try to join ibss
2010-05-23T21:24:24.384682+08:00 boston kernel: wlan0: Trigger new scan to 
find an IBSS to join
2010-05-23T21:24:24.400170+08:00 boston kernel: wlan0: sta_find_ibss 
(active_ibss=0)
2010-05-23T21:24:24.400187+08:00 boston kernel:   did not try to join ibss
2010-05-23T21:24:26.687669+08:00 boston kernel: wlan0: sta_find_ibss 
(active_ibss=0)
2010-05-23T21:24:26.687691+08:00 boston kernel:   did not try to join ibss
2010-05-23T21:24:26.687695+08:00 boston kernel: wlan0: Trigger new scan to 
find an IBSS to join
2010-05-23T21:24:26.707658+08:00 boston kernel: wlan0: sta_find_ibss 
(active_ibss=0)
2010-05-23T21:24:26.707666+08:00 boston kernel:   did not try to join ibss
2010-05-23T21:24:28.703660+08:00 boston kernel: wlan0: sta_find_ibss 
(active_ibss=0)
2010-05-23T21:24:28.703668+08:00 boston kernel:   did not try to join ibss
2010-05-23T21:24:30.703668+08:00 boston kernel: wlan0: sta_find_ibss 
(active_ibss=0)
2010-05-23T21:24:30.703687+08:00 boston kernel:   did not try to join ibss
2010-05-23T21:24:30.703691+08:00 boston kernel: wlan0: Trigger new scan to 
find an IBSS to join
2010-05-23T21:24:30.723659+08:00 boston kernel: wlan0: sta_find_ibss 
(active_ibss=0)
2010-05-23T21:24:30.723666+08:00 boston kernel:   did not try to join ibss
2010-05-23T21:24:32.703664+08:00 boston kernel: wlan0: sta_find_ibss 
(active_ibss=0)
2010-05-23T21:24:32.703675+08:00 boston kernel:   did not try to join ibss
2010-05-23T21:24:32.703679+08:00 boston kernel: wlan0: Creating new IBSS 
network, BSSID 06:a5:c6:42:20:80
2010-05-23T21:24:32.712163+08:00 boston kernel: iwlagn 0000:02:00.0: 
Unable to find TIM Element in beacon
2010-05-23T21:24:32.712172+08:00 boston kernel: iwlagn 0000:02:00.0: 
Unable to find TIM Element in beacon



2) Still with bad-commit in the kernel

2010-05-23T21:47:28.498041+08:00 boston kernel: cfg80211: Calling CRDA to 
update world regulatory domain
2010-05-23T21:47:28.535052+08:00 boston kernel: iwlagn: Intel(R) Wireless 
WiFi Link AGN driver for Linux, in-tree:
2010-05-23T21:47:28.535078+08:00 boston kernel: iwlagn: Copyright(c) 
2003-2010 Intel Corporation
2010-05-23T21:47:28.535082+08:00 boston kernel: iwlagn 0000:02:00.0: PCI 
INT A -> GSI 16 (level, low) -> IRQ 16
2010-05-23T21:47:28.535085+08:00 boston kernel: iwlagn 0000:02:00.0: 
setting latency timer to 64
2010-05-23T21:47:28.535089+08:00 boston kernel: iwlagn 0000:02:00.0: 
Detected Intel(R) Centrino(R) Advanced-N 6200 AGN, REV=0x74
2010-05-23T21:47:28.551036+08:00 boston kernel: iwlagn 0000:02:00.0: 
Tunable channels: 13 802.11bg, 24 802.11a channels
2010-05-23T21:47:28.551048+08:00 boston kernel: iwlagn 0000:02:00.0: irq 
46 for MSI/MSI-X
2010-05-23T21:47:28.571054+08:00 boston kernel: iwlagn 0000:02:00.0: 
loaded firmware version 9.193.4.1 build 19710
2010-05-23T21:47:28.574286+08:00 boston kernel: phy0: Selected rate 
control algorithm 'iwl-agn-rs'
2010-05-23T21:47:29.406045+08:00 boston kernel: phy0: device now idle
2010-05-23T21:47:29.413564+08:00 boston kernel: phy0: device no longer 
idle - in use
2010-05-23T21:47:29.486081+08:00 boston kernel: wlan0: sta_find_ibss 
(active_ibss=0)
2010-05-23T21:47:29.486099+08:00 boston kernel:   did not try to join ibss
2010-05-23T21:47:29.486103+08:00 boston kernel: wlan0: Trigger new scan to 
find an IBSS to join



So, the difference are these messages when it's working ...

wlan0: sta_find_ibss (active_ibss=0)
     did not try to join ibss
wlan0: Trigger new scan to find an IBSS to join
wlan0: sta_find_ibss (active_ibss=0)
     did not try to join ibss
wlan0: sta_find_ibss (active_ibss=0)
     did not try to join ibss
wlan0: Trigger new scan to find an IBSS to join
wlan0: sta_find_ibss (active_ibss=0)
     did not try to join ibss
wlan0: sta_find_ibss (active_ibss=0)
     did not try to join ibss
wlan0: sta_find_ibss (active_ibss=0)
     did not try to join ibss
wlan0: Trigger new scan to find an IBSS to join
wlan0: sta_find_ibss (active_ibss=0)
     did not try to join ibss
wlan0: sta_find_ibss (active_ibss=0)
     did not try to join ibss
wlan0: Creating new IBSS network, BSSID 62:c8:0e:ff:7e:69
iwlagn 0000:02:00.0: Unable to find TIM Element in beacon
iwlagn 0000:02:00.0: Unable to find TIM Element in beacon



I modified one line in the bad commit to set the default "timer->slack = 
0" and that fixed the problem of IBSS not working. Below is the patch.

I don't know how this impacts others, but it seems better to default to 
the previous behavior rather than introducing "slacks" that other 
subsystems are not just ready to handle.


Thanks,
Jeff

diff --git a/kernel/timer.c b/kernel/timer.c
--- a/kernel/timer.c
+++ b/kernel/timer.c
@@ -549,7 +567,7 @@ static void __init_timer(struct timer_list *timer,
   {
   	timer->entry.next = NULL;
   	timer->base = __raw_get_cpu_var(tvec_bases);
-	timer->slack = -1;
+	timer->slack = 0;
   #ifdef CONFIG_TIMER_STATS
   	timer->start_site = NULL;
   	timer->start_pid = -1;

^ permalink raw reply	[flat|nested] 9+ messages in thread
* Re: Wireless IBSS on Linux-2.6.34 broken by commit 3bbb9ec946428b96657126768f65487a48dd090c
@ 2010-05-23 23:16 Jeff Chua
  2010-05-24 14:57 ` Linus Torvalds
  0 siblings, 1 reply; 9+ messages in thread
From: Jeff Chua @ 2010-05-23 23:16 UTC (permalink / raw)
  To: Johannes Berg, Arjan van de Ven, johnstul@us.ibm.com, Ingo Molnar,
	Andrew Morton, Thomas Gleixner, Intel Linux Wireless,
	John W. Linville, Frans Pop, Chatre, Reinette, Linus Torvalds,
	lkml


Mon, May 24, 2010 at 5:25 AM, Arjan van de Ven <arjan@linux.intel.com> 
wrote:
>>> can you try, instead do the following, in the apply_slack() function:
>>>
>>> just before the return expires_limit; do
>>>
>>> if (expires_limit<  expires)
>>> # # #expires_limit = expires;
>>
>> This doesn't work.
>>
>>
>>> if that does not fix it, a second thought:
>>> add, after the first if (timer->slack<  0)
>>>
>>> if (timer->slack<  0&&  expires<  jiffies)
>>> # # # #expires_limit = expires;
>>
>> This works.
>
> hmm ok so the wireless stack sets a timer way back in the past
> ok that's technically legal.
>
> how about
>
> expires_limit = expires;
>
> if (timer->slack > -1)
>        expires_limit = expires + timer->slack;
> else if (time_after(expires, jiffies))
>        expires_limit = expires + (expires - jiffies)/256;
>
> can you test such a thing and send a patch?
> (it has my Acked-By: either way)


Arjan,

Tested and working. Here's the patch. Thanks for the fix!

Jeff



--- a/kernel/timer.c.org	2010-05-24 07:09:04.000000000 +0800
+++ a/kernel/timer.c	2010-05-24 07:05:22.000000000 +0800
@@ -750,9 +750,11 @@
  	unsigned long expires_limit, mask;
  	int bit;

-	expires_limit = expires + timer->slack;
+	expires_limit = expires;

-	if (timer->slack < 0) /* auto slack: use 0.4% */
+	if (timer->slack > -1)
+		expires_limit = expires + timer->slack;
+	else if (time_after(expires, jiffies)) /* auto slack: use 0.4% */
  		expires_limit = expires + (expires - jiffies)/256;

  	mask = expires ^ expires_limit;



^ permalink raw reply	[flat|nested] 9+ messages in thread
* Re: Wireless IBSS on Linux-2.6.34 broken by commit 3bbb9ec946428b96657126768f65487a48dd090c
@ 2010-05-24 22:48 Jeff Chua
  0 siblings, 0 replies; 9+ messages in thread
From: Jeff Chua @ 2010-05-24 22:48 UTC (permalink / raw)
  To: Linus Torvalds
  Cc: Johannes Berg, Arjan van de Ven, johnstul@us.ibm.com, Ingo Molnar,
	Andrew Morton, Thomas Gleixner, Intel Linux Wireless,
	John W. Linville, Frans Pop, Chatre, Reinette, lkml



On Mon, May 24, 2010 at 10:57 PM, Linus Torvalds <torvalds@linux-foundation.org> wrote:>
> On Mon, 24 May 2010, Jeff Chua wrote:
>> +     if (timer->slack > -1)
>> +     else if (time_after(expires, jiffies)) /* auto slack: use 0.4% */
>>               expires_limit = expires + (expires - jiffies)/256;
>
> Please don't reload 'jiffies' twice. It's volatile, and the compiler will
> do a crap job of it.

Linus,

Sorry, didn't know what a jiffies was before, so didn't know about the 
crap it caused. Now I learned something new.

> Also, '0' is a normal number, but '-1' is a rather odd value. Please just
> test for non-negative by doing ">= 0" rather than "> -1"

Fixed.

Below is the patch to fix jiffies and checking for non-negative.


Thanks,
Jeff

--- a/kernel/timer.c.org	2010-05-25 06:05:46.000000000 +0800
+++ a/kernel/timer.c	2010-05-25 06:17:44.000000000 +0800
@@ -748,14 +748,15 @@
  unsigned long apply_slack(struct timer_list *timer, unsigned long expires)
  {
  	unsigned long expires_limit, mask;
+	unsigned long now = jiffies;
  	int bit;

  	expires_limit = expires;

-	if (timer->slack > -1)
+	if (timer->slack >= 0)
  		expires_limit = expires + timer->slack;
-	else if (time_after(expires, jiffies)) /* auto slack: use 0.4% */
-		expires_limit = expires + (expires - jiffies)/256;
+	else if (time_after(expires, now)) /* auto slack: use 0.4% */
+		expires_limit = expires + (expires - now)/256;

  	mask = expires ^ expires_limit;
  	if (mask == 0)


^ permalink raw reply	[flat|nested] 9+ messages in thread
end of thread, other threads:[~2010-05-25 18:39 UTC | newest]

Thread overview: 9+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2010-05-23  4:58 Wireless IBSS on Linux-2.6.34 broken by commit 3bbb9ec946428b96657126768f65487a48dd090c Jeff Chua
2010-05-23  6:21 ` Arjan van de Ven
2010-05-23  9:49 ` Johannes Berg
2010-05-23 13:19   ` Jeff Chua
  -- strict thread matches above, loose matches on Subject: below --
2010-05-23 17:07 Jeff Chua
2010-05-23 23:16 Jeff Chua
2010-05-24 14:57 ` Linus Torvalds
2010-05-25 18:37   ` Thomas Gleixner
2010-05-24 22:48 Jeff Chua

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).