Bug 14751

Summary: RT2500 driver: cannot connect to network (regression, bisected)
Product: Networking Reporter: rafal
Component: WirelessAssignee: networking_wireless (networking_wireless)
Status: CLOSED CODE_FIX    
Severity: normal CC: akpm, gwingerde, IvDoorn, linville, rafal
Priority: P1    
Hardware: All   
OS: Linux   
Kernel Version: Subsystem:
Regression: Yes Bisected commit-id:
Attachments: patch based on pre e4ea1c403a code
Configure antenna during add_interface()
Proper fix for setting the initial antenna configuration.
Attempt 2 for a proper fix.

Description rafal 2009-12-06 16:54:06 UTC
I have laptop 
Benq Joybook A33E

with network card:
rt2500pci       : RaLink|RT2500 802.11g Cardbus/mini-PCI [NETWORK_OTHER] (vendor:1814 device:0201 subv:1814 subd:2560) (rev: 01)

My network card worked good till
2.6.28.
After that version i always have:
iwlist wlan0 scan:
no scan results

Bisection leaded to this commit:
e4ea1c403

After this commit, my wireless antena is not initialized anymore - kernel never calls function
rt2500pci_config_ant
in rt2500pci.c

I will attach dirty patch (against mainline) that cause my wireless card works again.
Comment 1 rafal 2009-12-06 17:03:17 UTC
Created attachment 24055 [details]
patch based on pre e4ea1c403a code
Comment 2 Andrew Morton 2009-12-07 22:07:06 UTC
Ivo, commit e4ea1c403 has your name on it ;)
Comment 3 Andrew Morton 2009-12-07 22:08:48 UTC
argh, I added IvDoorn@gmail.com to the wrong field.  Again:

Ivo, commit e4ea1c403 has your name on it ;)
Comment 4 John W. Linville 2009-12-08 15:20:56 UTC
Rafal, does the patch in comment 1 actually resolve the issue for you?
Comment 5 rafal 2009-12-08 15:31:34 UTC
Yes
After this patch I can list wireless networks again
and I can connect without problem.
Comment 6 John W. Linville 2009-12-08 15:41:48 UTC
Ivo & Gertjan, any comments on Rafal's patch?
Comment 7 Ivo van Doorn 2009-12-08 19:50:20 UTC
It seems to be a partial revert of e4ea1c403, which mixes the new code with some of the old code. What I am wondering however is this:

Does this rt2500pci device have fixed antenna's or is the link tuning triggered? If they have fixed antennas, then perhaps the more simpler fix is by at least calling rt2x00lib_config_antenna() once after the interface has been brought up. (Either when the mac80211 is using the start(), add_interface() or config() callback function).
Comment 8 Ivo van Doorn 2009-12-08 19:51:57 UTC
Created attachment 24093 [details]
Configure antenna during add_interface()

Compile-tested only, and I am not sure if the hardware accepts antenna configuration changes at this point (before the channel is configured).
Comment 9 rafal 2009-12-09 23:49:51 UTC
Thanks for this patch.
Unfortunately it doesnt resolve my problem.
I cannot list wireless networks.

I dont know if it's important, 
after patch from "comment 1"
i have many calls to 
rt2500pci_config_ant
but after Ivo's patch,
this function is called once 

One more thing.
Ivo's patch and this:
--- a/drivers/net/wireless/rt2x00/rt2x00config.c
+++ b/drivers/net/wireless/rt2x00/rt2x00config.c
@@ -140,8 +140,8 @@ void rt2x00lib_config_antenna(struct rt2x00_dev *rt2x00dev,
        else
                config.tx = active->tx;

-       if (config.rx == active->rx && config.tx == active->tx)
-               return;

resolves my problem

Hope it helps.
Comment 10 Ivo van Doorn 2009-12-10 19:47:42 UTC
> I dont know if it's important, 
> after patch from "comment 1"
> i have many calls to 
> rt2500pci_config_ant
> but after Ivo's patch,
> this function is called once 

Yeah, this is actually important. The antenna configuration should not be done
too often because you can loose frames while the antenna is being tuned.

> One more thing.
> Ivo's patch and this:
> --- a/drivers/net/wireless/rt2x00/rt2x00config.c
> +++ b/drivers/net/wireless/rt2x00/rt2x00config.c
> @@ -140,8 +140,8 @@ void rt2x00lib_config_antenna(struct rt2x00_dev
> *rt2x00dev,
>         else
>                 config.tx = active->tx;
> 
> -       if (config.rx == active->rx && config.tx == active->tx)
> -               return;

Ah right, now I see the big problem. When antenna diversity is disabled
the driver never attempts to configure the card. But even worse, the rt2x00lib_config_antenna() only works for software diversity. I'll try to comeup with another test patch in a day or so.
Comment 11 John W. Linville 2010-03-01 18:56:48 UTC
Ivo, any word on a new test patch?
Comment 12 Ivo van Doorn 2010-03-05 18:14:17 UTC
Actually I think I already submitted a patch some time ago. Is this bug still present in the latest kernel version?
Comment 13 rafal 2010-04-30 10:02:48 UTC
I have checked current kernel tree and its working fine.
Thank you very much.
Comment 14 rafal 2010-05-04 11:13:40 UTC
I have checked wrong kernel version.
In current git, it still desnt work.

Solution is the same:


> --- a/drivers/net/wireless/rt2x00/rt2x00config.c
> +++ b/drivers/net/wireless/rt2x00/rt2x00config.c
> @@ -140,8 +140,8 @@ void rt2x00lib_config_antenna(struct rt2x00_dev
> *rt2x00dev,
>         else
>                 config.tx = active->tx;
> 
> -       if (config.rx == active->rx && config.tx == active->tx)
> -               return;


Sorry for the confusion.
Comment 15 John W. Linville 2010-05-25 17:55:14 UTC
Ivo, ping?  Should I revert bdfa500b8b8ca87dfe7a311f569fe8b39746c299?
Comment 16 Ivo van Doorn 2010-05-25 20:57:36 UTC
No reverting bdfa500b8b8ca87dfe7a311f569fe8b39746c299 won't be the best solution.
Lets go for the change rafal suggested, and apply the change as proposed in #14

It is not the most desirable solution, since there might be some problems in combination with powersaving, but this should be quite limited. Reverting
bdfa500b8b8ca87dfe7a311f569fe8b39746c299 will only cause a lot more cards to break due to problems with antenna diversity.
Comment 17 Gertjan van Wingerde 2010-05-27 16:57:29 UTC
Created attachment 26561 [details]
Proper fix for setting the initial antenna configuration.

Sorry, have been away for a few days.
Please check the newly attached patch, which is a refinement of rafal's patch. With this patch the antenna configuration will only be done at start-up, and after that only when the antenna really changes.
Comment 18 Ivo van Doorn 2010-05-27 18:25:18 UTC
Still not sure about this patch, I think this partially solves the problem.
The antenna is still configured each time the configuration changes (i.e. channel switch).
I tested the patch from Rafal on an rt2800usb device today, and it actually caused that no results where produced during scanning. Without the patch I could scan and associate correctly.
Comment 19 Gertjan van Wingerde 2010-05-27 19:28:25 UTC
Did you test with my patch?

Basically my patch keeps the behavior of rt2x00lib_config_antenna exactly as it is now, except at initialization time. So, my patch should not have the drawbacks of rafal's patch.
Comment 20 Ivo van Doorn 2010-05-27 19:52:38 UTC
No I haven't with your patch yet, but rt2800usb doesn't have diversity for its antenna's. So that means only the config_antenna call from rt2x00mac_config() is used. And that is the one which sets the initial argument unconditionally to true.

As a result, there won't be a difference between rafals your patch in that respect. Perhaps in rt2x00mac_config it should determine if it really is in initialize() state or not (aka was the radio disabled, and is it being enabled by this configuration option).
Comment 21 Gertjan van Wingerde 2010-05-27 20:18:17 UTC
Hmm. OK. But that means we should not go ahead with rafal's patch as well, as it breaks drivers other than rt2500pci.
Comment 22 Ivo van Doorn 2010-05-28 07:14:55 UTC
Well we can't revert bdfa500b8b8ca87dfe7a311f569fe8b39746c299 either since that would break software diversity. But could you extend your patch to check if this is the first call to config since the start() callback? That way we know if we are in initialization state or not.
Comment 23 Gertjan van Wingerde 2010-06-03 04:08:47 UTC
Created attachment 26631 [details]
Attempt 2 for a proper fix.

OK. Took me a while to full understand the code flow.

V2 of the patch is now uploaded, where the default antenna is programmed at initialization time, instead of from the config callback.
Comment 24 Ivo van Doorn 2010-06-03 15:46:32 UTC
Did you test this patch?
I am not sure if the antenna can be configured before the radio is enabled, or if no channel has been configured yet
Comment 25 Gertjan van Wingerde 2010-06-03 18:45:27 UTC
I tested it on a rt2800pci device, as I currently have no machine available with a rt2500pci in it.

Seemed to work fine for that device, although I must say that that device works perfectly without the patch as well.

However, I don't think the dependency on the radio or the channel is there, as in the registers you simply indicate which antenna to use.

I suggest we first get confirmation from rafal that this patch fixes his problem.

Rafal, can you test if this fixes your problem?
Comment 26 John W. Linville 2010-07-14 17:42:11 UTC
Rafal, ping?
Comment 27 rafal 2010-07-14 17:49:09 UTC
I'm sorry
I was on vacation for few weeks.
Tomorrow I will do tests.
Comment 28 rafal 2010-07-21 22:55:04 UTC
Unfortunately this patch doesnt work :(
Maybe my wifi card is somehow broken ?
Can somebody reproduce this bug ?
Comment 29 John W. Linville 2010-11-23 16:27:29 UTC
What kernel did you try?

Comment 14 indicates that this patch (which is in 2.6.36) should have fixed the issue for you?

commit 4d7ede7f5ad58c5316335b9018ddef58bd687def
Author: John W. Linville <linville@tuxdriver.com>
Date:   Wed May 26 13:33:31 2010 -0400

    rt2x00: do not shortcut rt2x00lib_config_antenna
    
    This function was exiting early if the existing diversity settings
    were unchanged.  Unfortunately, in some cases the antenna configuration
    is not initialized at all.
    
    https://bugzilla.kernel.org/show_bug.cgi?id=14751
    
    Signed-off-by: John W. Linville <linville@tuxdriver.com>
    Acked-by: Ivo van Doorn <IvDoorn@gmail.com>
    Cc: Gertjan van Wingerde <gwingerde@gmail.com>
Comment 30 rafal 2010-11-23 16:39:28 UTC
I think tests were performed before 2.6.36.
I will do it again with 2.6.36 ASAP.
Comment 31 rafal 2010-12-15 12:20:05 UTC
Its working now.
Thank you all for your work.