Bug 14751

Created attachment 24055 [details]
patch based on pre e4ea1c403a code

Ivo, commit e4ea1c403 has your name on it ;)

argh, I added IvDoorn@gmail.com to the wrong field.  Again:

Ivo, commit e4ea1c403 has your name on it ;)

Rafal, does the patch in comment 1 actually resolve the issue for you?

Yes
After this patch I can list wireless networks again
and I can connect without problem.

Ivo & Gertjan, any comments on Rafal's patch?

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

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

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.

> 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.

Ivo, any word on a new test patch?

Actually I think I already submitted a patch some time ago. Is this bug still present in the latest kernel version?

I have checked current kernel tree and its working fine.
Thank you very much.

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.

Ivo, ping?  Should I revert bdfa500b8b8ca87dfe7a311f569fe8b39746c299?

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.

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.

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.

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.

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

Hmm. OK. But that means we should not go ahead with rafal's patch as well, as it breaks drivers other than rt2500pci.

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.

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.

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

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?

Rafal, ping?

I'm sorry
I was on vacation for few weeks.
Tomorrow I will do tests.

Unfortunately this patch doesnt work :(
Maybe my wifi card is somehow broken ?
Can somebody reproduce this bug ?

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>

I think tests were performed before 2.6.36.
I will do it again with 2.6.36 ASAP.

Its working now.
Thank you all for your work.