zd1211rw firmware loading timeout

zd1211rw firmware loading timeout

[Florian Fainelli]

Hello,

I am running Debian testing kernel 3.1.0-1-amd64, using a 079b:0062 Sagem 
XG-76NA 802.11bg stick.

Upon zd1211rw interface 
bringup (ifconfig wlan0 up) I get the following timeout:

[  950.330573] zd1211rw 1-3:1.0: phy2
[  955.108510] zd1211rw 1-3:1.0: firmware version 4725
[  955.148532] zd1211rw 1-3:1.0: zd1211b chip 079b:0062 v4810 high 00-19-70 
AL2230_RF pa0 g--NS
[snip]
[  955.204072] zd1211rw 1-3:1.0: error ioread32(CR_REG1): -110

A second ifconfig wlan0 up brings the interface up without problems.

After a bit more debugging, the call trace is the following:

[10241.028130] zd1211rw 1-3:1.0: zd_chip_lock_phy_regs: error 
ioread32(CR_REG1): -110
[10241.028140] zd1211rw 1-3:1.0: zd_switch_radio_on: failed to lock PHY regs
[10241.028148] zd1211rw 1-3:1.0: zd_op_start: failed to set radio on

The following small modification works around the issue:

diff --git a/zd_mac.c b/zd_mac.c
index cabfae1..6bfb673 100644
--- a/zd_mac.c
+++ b/zd_mac.c
@@ -306,9 +306,14 @@ int zd_op_start(struct ieee80211_hw *hw)
        r = set_mc_hash(mac);
        if (r)
                goto disable_int;
+
+       msleep(10);
+
        r = zd_chip_switch_radio_on(chip);
-       if (r < 0)
+       if (r < 0) {
+               dev_err(zd_chip_dev(chip), "%s: failed to set radio on\n", 
__func__);
                goto disable_int;
+       }
        r = zd_chip_enable_rxtx(chip);
        if (r < 0)
                goto disable_radio;

Is it possible that some chips require more time between a set_mc_hash and a 
radio on? Or in general that consequent writes to the PHY regs should be timed 
appropriately?

When I googled for this timeout issue, I could see quite some reports of other 
people having this error while the chip was already operating.

Thanks
-- 
Florian

Re: zd1211rw firmware loading timeout

[Jussi Kivilinna]

Quoting Florian Fainelli <florian@openwrt.org>:

>
> Hello,
>
> I am running Debian testing kernel 3.1.0-1-amd64, using a 079b:0062 Sagem
> XG-76NA 802.11bg stick.
>
> Upon zd1211rw interface
> bringup (ifconfig wlan0 up) I get the following timeout:
>
> [  950.330573] zd1211rw 1-3:1.0: phy2
> [  955.108510] zd1211rw 1-3:1.0: firmware version 4725
> [  955.148532] zd1211rw 1-3:1.0: zd1211b chip 079b:0062 v4810 high 00-19-70
> AL2230_RF pa0 g--NS
> [snip]
> [  955.204072] zd1211rw 1-3:1.0: error ioread32(CR_REG1): -110
>
> A second ifconfig wlan0 up brings the interface up without problems.
>
> After a bit more debugging, the call trace is the following:
>
> [10241.028130] zd1211rw 1-3:1.0: zd_chip_lock_phy_regs: error
> ioread32(CR_REG1): -110
> [10241.028140] zd1211rw 1-3:1.0: zd_switch_radio_on: failed to lock PHY regs
> [10241.028148] zd1211rw 1-3:1.0: zd_op_start: failed to set radio on
>
> The following small modification works around the issue:
>
> diff --git a/zd_mac.c b/zd_mac.c
> index cabfae1..6bfb673 100644
> --- a/zd_mac.c
> +++ b/zd_mac.c
> @@ -306,9 +306,14 @@ int zd_op_start(struct ieee80211_hw *hw)
>        r = set_mc_hash(mac);
>        if (r)
>                goto disable_int;
> +
> +       msleep(10);
> +
>        r = zd_chip_switch_radio_on(chip);
> -       if (r < 0)
> +       if (r < 0) {
> +               dev_err(zd_chip_dev(chip), "%s: failed to set radio on\n",
> __func__);
>                goto disable_int;
> +       }
>        r = zd_chip_enable_rxtx(chip);
>        if (r < 0)
>                goto disable_radio;
>
> Is it possible that some chips require more time between a set_mc_hash and a
> radio on? Or in general that consequent writes to the PHY regs  
> should be timed
> appropriately?

Yes, it is possible. Vendor driver appears to have quite alot sleeps  
spread around in RF-chip handling code. I might have seen this myself  
sometimes when zd1211 device has firmware freeze and has to be  
reseted. But since freeze is hard to reproduce and reset didn't fail  
always, I didn't bother with it.

Can you give your "Signed-off-by" line for the patch?

-Jussi

>
> When I googled for this timeout issue, I could see quite some  
> reports of other
> people having this error while the chip was already operating.
>
> Thanks
> --
> Florian
> --
> To unsubscribe from this list: send the line "unsubscribe linux-wireless" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
>
>

Re: zd1211rw firmware loading timeout

[Florian Fainelli]

Hello Jussi,

On 02/08/12 14:15, Jussi Kivilinna wrote:
> Quoting Florian Fainelli <florian@openwrt.org>:
>
>>
>> Hello,
>>
>> I am running Debian testing kernel 3.1.0-1-amd64, using a 079b:0062 
>> Sagem
>> XG-76NA 802.11bg stick.
>>
>> Upon zd1211rw interface
>> bringup (ifconfig wlan0 up) I get the following timeout:
>>
>> [  950.330573] zd1211rw 1-3:1.0: phy2
>> [  955.108510] zd1211rw 1-3:1.0: firmware version 4725
>> [  955.148532] zd1211rw 1-3:1.0: zd1211b chip 079b:0062 v4810 high 
>> 00-19-70
>> AL2230_RF pa0 g--NS
>> [snip]
>> [  955.204072] zd1211rw 1-3:1.0: error ioread32(CR_REG1): -110
>>
>> A second ifconfig wlan0 up brings the interface up without problems.
>>
>> After a bit more debugging, the call trace is the following:
>>
>> [10241.028130] zd1211rw 1-3:1.0: zd_chip_lock_phy_regs: error
>> ioread32(CR_REG1): -110
>> [10241.028140] zd1211rw 1-3:1.0: zd_switch_radio_on: failed to lock 
>> PHY regs
>> [10241.028148] zd1211rw 1-3:1.0: zd_op_start: failed to set radio on
>>
>> The following small modification works around the issue:
>>
>> diff --git a/zd_mac.c b/zd_mac.c
>> index cabfae1..6bfb673 100644
>> --- a/zd_mac.c
>> +++ b/zd_mac.c
>> @@ -306,9 +306,14 @@ int zd_op_start(struct ieee80211_hw *hw)
>>        r = set_mc_hash(mac);
>>        if (r)
>>                goto disable_int;
>> +
>> +       msleep(10);
>> +
>>        r = zd_chip_switch_radio_on(chip);
>> -       if (r < 0)
>> +       if (r < 0) {
>> +               dev_err(zd_chip_dev(chip), "%s: failed to set radio 
>> on\n",
>> __func__);
>>                goto disable_int;
>> +       }
>>        r = zd_chip_enable_rxtx(chip);
>>        if (r < 0)
>>                goto disable_radio;
>>
>> Is it possible that some chips require more time between a 
>> set_mc_hash and a
>> radio on? Or in general that consequent writes to the PHY regs should 
>> be timed
>> appropriately?
>
> Yes, it is possible. Vendor driver appears to have quite alot sleeps 
> spread around in RF-chip handling code. I might have seen this myself 
> sometimes when zd1211 device has firmware freeze and has to be 
> reseted. But since freeze is hard to reproduce and reset didn't fail 
> always, I didn't bother with it.
>
> Can you give your "Signed-off-by" line for the patch?

Sure, I will resubmit this patch, do you also want me to keep the line 
showing the error?

>
> -Jussi
>
>>
>> When I googled for this timeout issue, I could see quite some reports 
>> of other
>> people having this error while the chip was already operating.
>>
>> Thanks
>> -- 
>> Florian
>> -- 
>> To unsubscribe from this list: send the line "unsubscribe 
>> linux-wireless" in
>> the body of a message to majordomo@vger.kernel.org
>> More majordomo info at  http://vger.kernel.org/majordomo-info.html
>>
>>
>
>
> -- 
> To unsubscribe from this list: send the line "unsubscribe 
> linux-wireless" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: zd1211rw firmware loading timeout

[Jussi Kivilinna]

Quoting Florian Fainelli <florian@openwrt.org>:

> Hello Jussi,
>
> On 02/08/12 14:15, Jussi Kivilinna wrote:
>> Quoting Florian Fainelli <florian@openwrt.org>:
>>
>>>
>>> Hello,
>>>
>>> I am running Debian testing kernel 3.1.0-1-amd64, using a 079b:0062 Sagem
>>> XG-76NA 802.11bg stick.
>>>
>>> Upon zd1211rw interface
>>> bringup (ifconfig wlan0 up) I get the following timeout:
>>>
>>> [  950.330573] zd1211rw 1-3:1.0: phy2
>>> [  955.108510] zd1211rw 1-3:1.0: firmware version 4725
>>> [  955.148532] zd1211rw 1-3:1.0: zd1211b chip 079b:0062 v4810 high 00-19-70
>>> AL2230_RF pa0 g--NS
>>> [snip]
>>> [  955.204072] zd1211rw 1-3:1.0: error ioread32(CR_REG1): -110
>>>
>>> A second ifconfig wlan0 up brings the interface up without problems.
>>>
>>> After a bit more debugging, the call trace is the following:
>>>
>>> [10241.028130] zd1211rw 1-3:1.0: zd_chip_lock_phy_regs: error
>>> ioread32(CR_REG1): -110
>>> [10241.028140] zd1211rw 1-3:1.0: zd_switch_radio_on: failed to  
>>> lock PHY regs
>>> [10241.028148] zd1211rw 1-3:1.0: zd_op_start: failed to set radio on
>>>
>>> The following small modification works around the issue:
>>>
>>> diff --git a/zd_mac.c b/zd_mac.c
>>> index cabfae1..6bfb673 100644
>>> --- a/zd_mac.c
>>> +++ b/zd_mac.c
>>> @@ -306,9 +306,14 @@ int zd_op_start(struct ieee80211_hw *hw)
>>>       r = set_mc_hash(mac);
>>>       if (r)
>>>               goto disable_int;
>>> +
>>> +       msleep(10);
>>> +
>>>       r = zd_chip_switch_radio_on(chip);
>>> -       if (r < 0)
>>> +       if (r < 0) {
>>> +               dev_err(zd_chip_dev(chip), "%s: failed to set radio on\n",
>>> __func__);
>>>               goto disable_int;
>>> +       }
>>>       r = zd_chip_enable_rxtx(chip);
>>>       if (r < 0)
>>>               goto disable_radio;
>>>
>>> Is it possible that some chips require more time between a  
>>> set_mc_hash and a
>>> radio on? Or in general that consequent writes to the PHY regs  
>>> should be timed
>>> appropriately?
>>
>> Yes, it is possible. Vendor driver appears to have quite alot  
>> sleeps spread around in RF-chip handling code. I might have seen  
>> this myself sometimes when zd1211 device has firmware freeze and  
>> has to be reseted. But since freeze is hard to reproduce and reset  
>> didn't fail always, I didn't bother with it.
>>
>> Can you give your "Signed-off-by" line for the patch?
>
> Sure, I will resubmit this patch, do you also want me to keep the  
> line showing the error?
>

Yes, do that or add URL to original mail. Thanks.

>>
>> -Jussi
>>
>>>
>>> When I googled for this timeout issue, I could see quite some  
>>> reports of other
>>> people having this error while the chip was already operating.
>>>
>>> Thanks
>>> -- 
>>> Florian
>>> -- 
>>> To unsubscribe from this list: send the line "unsubscribe  
>>> linux-wireless" in
>>> the body of a message to majordomo@vger.kernel.org
>>> More majordomo info at  http://vger.kernel.org/majordomo-info.html
>>>
>>>
>>
>>
>> -- 
>> To unsubscribe from this list: send the line "unsubscribe linux-wireless" in
>> the body of a message to majordomo@vger.kernel.org
>> More majordomo info at  http://vger.kernel.org/majordomo-info.html
>
>
>

[PATCH] zd1211rw: wait between setting hash table and powering radio on

[Florian Fainelli]

I am running Debian testing kernel 3.1.0-1-amd64, using a 079b:0062 Sagem
XG-76NA 802.11bg stick.

Upon zd1211rw interface
bringup (ifconfig wlan0 up) I get the following timeout:

[  950.330573] zd1211rw 1-3:1.0: phy2
[  955.108510] zd1211rw 1-3:1.0: firmware version 4725
[  955.148532] zd1211rw 1-3:1.0: zd1211b chip 079b:0062 v4810 high 00-19-70
AL2230_RF pa0 g--NS
[snip]
[  955.204072] zd1211rw 1-3:1.0: error ioread32(CR_REG1): -110

A second ifconfig wlan0 up brings the interface up without problems.

After a bit more debugging, the call trace is the following:

[10241.028130] zd1211rw 1-3:1.0: zd_chip_lock_phy_regs: error
ioread32(CR_REG1): -110
[10241.028140] zd1211rw 1-3:1.0: zd_switch_radio_on: failed to lock PHY regs
[10241.028148] zd1211rw 1-3:1.0: zd_op_start: failed to set radio on

Adding a 10 milliseconds delay between the call to set_mc_hash() and
zd_chip_switch_radio_on() allows successful interface bringups in all
cases and matches what the vendor driver did.

Signed-off-by: Florian Fainelli <florian@openwrt.org>
---
 drivers/net/wireless/zd1211rw/zd_mac.c |    7 ++++++-
 1 files changed, 6 insertions(+), 1 deletions(-)

diff --git a/drivers/net/wireless/zd1211rw/zd_mac.c b/drivers/net/wireless/zd1211rw/zd_mac.c
index 98a574a..bed634c 100644
--- a/drivers/net/wireless/zd1211rw/zd_mac.c
+++ b/drivers/net/wireless/zd1211rw/zd_mac.c
@@ -306,9 +306,14 @@ int zd_op_start(struct ieee80211_hw *hw)
 	r = set_mc_hash(mac);
 	if (r)
 		goto disable_int;
+	msleep(10);
+
 	r = zd_chip_switch_radio_on(chip);
-	if (r < 0)
+	if (r < 0) {
+		dev_err(zd_chip_dev(chip),
+			"%s: failed to set radio on\n", __func__);
 		goto disable_int;
+	}
 	r = zd_chip_enable_rxtx(chip);
 	if (r < 0)
 		goto disable_radio;
-- 
1.7.5.4

Re: [PATCH] zd1211rw: wait between setting hash table and powering radio on

[Julian Calaby]

Hi Florian,

A couple of minor points:

On Thu, Feb 9, 2012 at 21:24, Florian Fainelli <florian@openwrt.org> wrote:
> I am running Debian testing kernel 3.1.0-1-amd64, using a 079b:0062 Sagem
> XG-76NA 802.11bg stick.
>
> Upon zd1211rw interface
> bringup (ifconfig wlan0 up) I get the following timeout:
>
> [  950.330573] zd1211rw 1-3:1.0: phy2
> [  955.108510] zd1211rw 1-3:1.0: firmware version 4725
> [  955.148532] zd1211rw 1-3:1.0: zd1211b chip 079b:0062 v4810 high 00-19-70
> AL2230_RF pa0 g--NS
> [snip]
> [  955.204072] zd1211rw 1-3:1.0: error ioread32(CR_REG1): -110
>
> A second ifconfig wlan0 up brings the interface up without problems.
>
> After a bit more debugging, the call trace is the following:
>
> [10241.028130] zd1211rw 1-3:1.0: zd_chip_lock_phy_regs: error
> ioread32(CR_REG1): -110
> [10241.028140] zd1211rw 1-3:1.0: zd_switch_radio_on: failed to lock PHY regs
> [10241.028148] zd1211rw 1-3:1.0: zd_op_start: failed to set radio on
>
> Adding a 10 milliseconds delay between the call to set_mc_hash() and
> zd_chip_switch_radio_on() allows successful interface bringups in all
> cases and matches what the vendor driver did.
>
> Signed-off-by: Florian Fainelli <florian@openwrt.org>
> ---
>  drivers/net/wireless/zd1211rw/zd_mac.c |    7 ++++++-
>  1 files changed, 6 insertions(+), 1 deletions(-)
>
> diff --git a/drivers/net/wireless/zd1211rw/zd_mac.c b/drivers/net/wireless/zd1211rw/zd_mac.c
> index 98a574a..bed634c 100644
> --- a/drivers/net/wireless/zd1211rw/zd_mac.c
> +++ b/drivers/net/wireless/zd1211rw/zd_mac.c
> @@ -306,9 +306,14 @@ int zd_op_start(struct ieee80211_hw *hw)
>        r = set_mc_hash(mac);
>        if (r)
>                goto disable_int;
> +       msleep(10);

You might want to stick a comment in here to tell future developers
why we're doing this.

> +
>        r = zd_chip_switch_radio_on(chip);
> -       if (r < 0)
> +       if (r < 0) {
> +               dev_err(zd_chip_dev(chip),
> +                       "%s: failed to set radio on\n", __func__);
>                goto disable_int;
> +       }

It might also be an idea to have it re-try powering on the radio
after, say 100 msecs, just in case 10 msecs isn't enough for whatever
is causing this issue.

>        r = zd_chip_enable_rxtx(chip);
>        if (r < 0)
>                goto disable_radio;

Thanks,

-- 
Julian Calaby

Email: julian.calaby@gmail.com
Profile: http://www.google.com/profiles/julian.calaby/
.Plan: http://sites.google.com/site/juliancalaby/

Re: [PATCH] zd1211rw: wait between setting hash table and powering radio on

[Florian Fainelli]

Hi Julian,

On 02/09/12 11:40, Julian Calaby wrote:
> Hi Florian,
>
> A couple of minor points:
>
> On Thu, Feb 9, 2012 at 21:24, Florian Fainelli<florian@openwrt.org>  wrote:
>> I am running Debian testing kernel 3.1.0-1-amd64, using a 079b:0062 Sagem
>> XG-76NA 802.11bg stick.
>>
>> Upon zd1211rw interface
>> bringup (ifconfig wlan0 up) I get the following timeout:
>>
>> [  950.330573] zd1211rw 1-3:1.0: phy2
>> [  955.108510] zd1211rw 1-3:1.0: firmware version 4725
>> [  955.148532] zd1211rw 1-3:1.0: zd1211b chip 079b:0062 v4810 high 00-19-70
>> AL2230_RF pa0 g--NS
>> [snip]
>> [  955.204072] zd1211rw 1-3:1.0: error ioread32(CR_REG1): -110
>>
>> A second ifconfig wlan0 up brings the interface up without problems.
>>
>> After a bit more debugging, the call trace is the following:
>>
>> [10241.028130] zd1211rw 1-3:1.0: zd_chip_lock_phy_regs: error
>> ioread32(CR_REG1): -110
>> [10241.028140] zd1211rw 1-3:1.0: zd_switch_radio_on: failed to lock PHY regs
>> [10241.028148] zd1211rw 1-3:1.0: zd_op_start: failed to set radio on
>>
>> Adding a 10 milliseconds delay between the call to set_mc_hash() and
>> zd_chip_switch_radio_on() allows successful interface bringups in all
>> cases and matches what the vendor driver did.
>>
>> Signed-off-by: Florian Fainelli<florian@openwrt.org>
>> ---
>>   drivers/net/wireless/zd1211rw/zd_mac.c |    7 ++++++-
>>   1 files changed, 6 insertions(+), 1 deletions(-)
>>
>> diff --git a/drivers/net/wireless/zd1211rw/zd_mac.c b/drivers/net/wireless/zd1211rw/zd_mac.c
>> index 98a574a..bed634c 100644
>> --- a/drivers/net/wireless/zd1211rw/zd_mac.c
>> +++ b/drivers/net/wireless/zd1211rw/zd_mac.c
>> @@ -306,9 +306,14 @@ int zd_op_start(struct ieee80211_hw *hw)
>>         r = set_mc_hash(mac);
>>         if (r)
>>                 goto disable_int;
>> +       msleep(10);
> You might want to stick a comment in here to tell future developers
> why we're doing this.

I was counting on the commit message to actually provide the detailed 
explanation, but you are right, some comment might be appropriate here 
as well.

>
>> +
>>         r = zd_chip_switch_radio_on(chip);
>> -       if (r<  0)
>> +       if (r<  0) {
>> +               dev_err(zd_chip_dev(chip),
>> +                       "%s: failed to set radio on\n", __func__);
>>                 goto disable_int;
>> +       }
> It might also be an idea to have it re-try powering on the radio
> after, say 100 msecs, just in case 10 msecs isn't enough for whatever
> is causing this issue.

Here I would rather let the error be printed in the kernel log for 
people to report an issue if 10 msecs proves not to be enough for some 
systems.

>
>>         r = zd_chip_enable_rxtx(chip);
>>         if (r<  0)
>>                 goto disable_radio;
> Thanks,
>

Re: [PATCH] zd1211rw: wait between setting hash table and powering radio on

[Julian Calaby]

Hi Florian,

On Thu, Feb 9, 2012 at 21:54, Florian Fainelli <florian@openwrt.org> wrote:
> Hi Julian,
>
>
> On 02/09/12 11:40, Julian Calaby wrote:
>>
>> Hi Florian,
>>
>> A couple of minor points:
>>
>> On Thu, Feb 9, 2012 at 21:24, Florian Fainelli<florian@openwrt.org>
>>  wrote:
>>>
>>> I am running Debian testing kernel 3.1.0-1-amd64, using a 079b:0062 Sagem
>>> XG-76NA 802.11bg stick.
>>>
>>> Upon zd1211rw interface
>>> bringup (ifconfig wlan0 up) I get the following timeout:
>>>
>>> [  950.330573] zd1211rw 1-3:1.0: phy2
>>> [  955.108510] zd1211rw 1-3:1.0: firmware version 4725
>>> [  955.148532] zd1211rw 1-3:1.0: zd1211b chip 079b:0062 v4810 high
>>> 00-19-70
>>> AL2230_RF pa0 g--NS
>>> [snip]
>>> [  955.204072] zd1211rw 1-3:1.0: error ioread32(CR_REG1): -110
>>>
>>> A second ifconfig wlan0 up brings the interface up without problems.
>>>
>>> After a bit more debugging, the call trace is the following:
>>>
>>> [10241.028130] zd1211rw 1-3:1.0: zd_chip_lock_phy_regs: error
>>> ioread32(CR_REG1): -110
>>> [10241.028140] zd1211rw 1-3:1.0: zd_switch_radio_on: failed to lock PHY
>>> regs
>>> [10241.028148] zd1211rw 1-3:1.0: zd_op_start: failed to set radio on
>>>
>>> Adding a 10 milliseconds delay between the call to set_mc_hash() and
>>> zd_chip_switch_radio_on() allows successful interface bringups in all
>>> cases and matches what the vendor driver did.
>>>
>>> Signed-off-by: Florian Fainelli<florian@openwrt.org>
>>> ---
>>>  drivers/net/wireless/zd1211rw/zd_mac.c |    7 ++++++-
>>>  1 files changed, 6 insertions(+), 1 deletions(-)
>>>
>>> diff --git a/drivers/net/wireless/zd1211rw/zd_mac.c
>>> b/drivers/net/wireless/zd1211rw/zd_mac.c
>>> index 98a574a..bed634c 100644
>>> --- a/drivers/net/wireless/zd1211rw/zd_mac.c
>>> +++ b/drivers/net/wireless/zd1211rw/zd_mac.c
>>> @@ -306,9 +306,14 @@ int zd_op_start(struct ieee80211_hw *hw)
>>>        r = set_mc_hash(mac);
>>>        if (r)
>>>                goto disable_int;
>>> +       msleep(10);
>>
>> You might want to stick a comment in here to tell future developers
>> why we're doing this.
>
>
> I was counting on the commit message to actually provide the detailed
> explanation, but you are right, some comment might be appropriate here as
> well.
>
>
>>
>>> +
>>>        r = zd_chip_switch_radio_on(chip);
>>> -       if (r<  0)
>>> +       if (r<  0) {
>>> +               dev_err(zd_chip_dev(chip),
>>> +                       "%s: failed to set radio on\n", __func__);
>>>                goto disable_int;
>>> +       }
>>
>> It might also be an idea to have it re-try powering on the radio
>> after, say 100 msecs, just in case 10 msecs isn't enough for whatever
>> is causing this issue.
>
>
> Here I would rather let the error be printed in the kernel log for people to
> report an issue if 10 msecs proves not to be enough for some systems.

When I see timing like this added to established drivers it makes me
think that maybe something has changed since the code was originally
written, maybe there was a 10msec delay somewhere deep down in the USB
code that's now been eliminated, or something else equally random, and
maybe we'll need a longer wait in the future. Writing the code to try
it, say, 5 times will probably never completely time out, but it will
probably work until the driver is removed from the kernel, rather than
having to change this to a larger value some time in the future. On
the other hand, this fixes the bug you've found and maybe something
like that can wait.

Thanks,

-- 
Julian Calaby

Email: julian.calaby@gmail.com
Profile: http://www.google.com/profiles/julian.calaby/
.Plan: http://sites.google.com/site/juliancalaby/

Re: [PATCH] zd1211rw: wait between setting hash table and powering radio on

[Jussi Kivilinna]

Quoting Julian Calaby <julian.calaby@gmail.com>:

> Hi Florian,
>
> On Thu, Feb 9, 2012 at 21:54, Florian Fainelli <florian@openwrt.org> wrote:
>> Hi Julian,
>>
>>
>> On 02/09/12 11:40, Julian Calaby wrote:
>>>
>>> Hi Florian,
>>>
>>> A couple of minor points:
>>>
>>> On Thu, Feb 9, 2012 at 21:24, Florian Fainelli<florian@openwrt.org>
>>>  wrote:
>>>>
>>>> I am running Debian testing kernel 3.1.0-1-amd64, using a 079b:0062 Sagem
>>>> XG-76NA 802.11bg stick.
>>>>
>>>> Upon zd1211rw interface
>>>> bringup (ifconfig wlan0 up) I get the following timeout:
>>>>
>>>> [  950.330573] zd1211rw 1-3:1.0: phy2
>>>> [  955.108510] zd1211rw 1-3:1.0: firmware version 4725
>>>> [  955.148532] zd1211rw 1-3:1.0: zd1211b chip 079b:0062 v4810 high
>>>> 00-19-70
>>>> AL2230_RF pa0 g--NS
>>>> [snip]
>>>> [  955.204072] zd1211rw 1-3:1.0: error ioread32(CR_REG1): -110
>>>>
>>>> A second ifconfig wlan0 up brings the interface up without problems.
>>>>
>>>> After a bit more debugging, the call trace is the following:
>>>>
>>>> [10241.028130] zd1211rw 1-3:1.0: zd_chip_lock_phy_regs: error
>>>> ioread32(CR_REG1): -110
>>>> [10241.028140] zd1211rw 1-3:1.0: zd_switch_radio_on: failed to lock PHY
>>>> regs
>>>> [10241.028148] zd1211rw 1-3:1.0: zd_op_start: failed to set radio on
>>>>
>>>> Adding a 10 milliseconds delay between the call to set_mc_hash() and
>>>> zd_chip_switch_radio_on() allows successful interface bringups in all
>>>> cases and matches what the vendor driver did.
>>>>
>>>> Signed-off-by: Florian Fainelli<florian@openwrt.org>
>>>> ---
>>>>  drivers/net/wireless/zd1211rw/zd_mac.c |    7 ++++++-
>>>>  1 files changed, 6 insertions(+), 1 deletions(-)
>>>>
>>>> diff --git a/drivers/net/wireless/zd1211rw/zd_mac.c
>>>> b/drivers/net/wireless/zd1211rw/zd_mac.c
>>>> index 98a574a..bed634c 100644
>>>> --- a/drivers/net/wireless/zd1211rw/zd_mac.c
>>>> +++ b/drivers/net/wireless/zd1211rw/zd_mac.c
>>>> @@ -306,9 +306,14 @@ int zd_op_start(struct ieee80211_hw *hw)
>>>>        r = set_mc_hash(mac);
>>>>        if (r)
>>>>                goto disable_int;
>>>> +       msleep(10);
>>>
>>> You might want to stick a comment in here to tell future developers
>>> why we're doing this.
>>
>>
>> I was counting on the commit message to actually provide the detailed
>> explanation, but you are right, some comment might be appropriate here as
>> well.
>>
>>
>>>
>>>> +
>>>>        r = zd_chip_switch_radio_on(chip);
>>>> -       if (r<  0)
>>>> +       if (r<  0) {
>>>> +               dev_err(zd_chip_dev(chip),
>>>> +                       "%s: failed to set radio on\n", __func__);
>>>>                goto disable_int;
>>>> +       }
>>>
>>> It might also be an idea to have it re-try powering on the radio
>>> after, say 100 msecs, just in case 10 msecs isn't enough for whatever
>>> is causing this issue.
>>
>>
>> Here I would rather let the error be printed in the kernel log for people to
>> report an issue if 10 msecs proves not to be enough for some systems.
>
> When I see timing like this added to established drivers it makes me
> think that maybe something has changed since the code was originally
> written, maybe there was a 10msec delay somewhere deep down in the USB
> code that's now been eliminated, or something else equally random, and
> maybe we'll need a longer wait in the future. Writing the code to try
> it, say, 5 times will probably never completely time out, but it will
> probably work until the driver is removed from the kernel, rather than
> having to change this to a larger value some time in the future. On
> the other hand, this fixes the bug you've found and maybe something
> like that can wait.

Quick search of "error ioread32(CR_REG1): -110" shows that first  
reports are from 2008/2.6.24. People have found following workarounds  
are "rmmod zd1211rw;modprobe zd1211rw", unplug/replug USB device, use  
motherboard USB connector instead of hub, etc.

-Jussi

>
> Thanks,
>
> --
> Julian Calaby
>
> Email: julian.calaby@gmail.com
> Profile: http://www.google.com/profiles/julian.calaby/
> .Plan: http://sites.google.com/site/juliancalaby/
>
>

Re: [PATCH] zd1211rw: wait between setting hash table and powering radio on

[Florian Fainelli]

On 02/09/12 12:41, Jussi Kivilinna wrote:
> Quoting Julian Calaby <julian.calaby@gmail.com>:
>
>> Hi Florian,
>>
>> On Thu, Feb 9, 2012 at 21:54, Florian Fainelli <florian@openwrt.org> 
>> wrote:
>>> Hi Julian,
>>>
>>>
>>> On 02/09/12 11:40, Julian Calaby wrote:
>>>>
>>>> Hi Florian,
>>>>
>>>> A couple of minor points:
>>>>
>>>> On Thu, Feb 9, 2012 at 21:24, Florian Fainelli<florian@openwrt.org>
>>>>  wrote:
>>>>>
>>>>> I am running Debian testing kernel 3.1.0-1-amd64, using a 
>>>>> 079b:0062 Sagem
>>>>> XG-76NA 802.11bg stick.
>>>>>
>>>>> Upon zd1211rw interface
>>>>> bringup (ifconfig wlan0 up) I get the following timeout:
>>>>>
>>>>> [  950.330573] zd1211rw 1-3:1.0: phy2
>>>>> [  955.108510] zd1211rw 1-3:1.0: firmware version 4725
>>>>> [  955.148532] zd1211rw 1-3:1.0: zd1211b chip 079b:0062 v4810 high
>>>>> 00-19-70
>>>>> AL2230_RF pa0 g--NS
>>>>> [snip]
>>>>> [  955.204072] zd1211rw 1-3:1.0: error ioread32(CR_REG1): -110
>>>>>
>>>>> A second ifconfig wlan0 up brings the interface up without problems.
>>>>>
>>>>> After a bit more debugging, the call trace is the following:
>>>>>
>>>>> [10241.028130] zd1211rw 1-3:1.0: zd_chip_lock_phy_regs: error
>>>>> ioread32(CR_REG1): -110
>>>>> [10241.028140] zd1211rw 1-3:1.0: zd_switch_radio_on: failed to 
>>>>> lock PHY
>>>>> regs
>>>>> [10241.028148] zd1211rw 1-3:1.0: zd_op_start: failed to set radio on
>>>>>
>>>>> Adding a 10 milliseconds delay between the call to set_mc_hash() and
>>>>> zd_chip_switch_radio_on() allows successful interface bringups in all
>>>>> cases and matches what the vendor driver did.
>>>>>
>>>>> Signed-off-by: Florian Fainelli<florian@openwrt.org>
>>>>> ---
>>>>>  drivers/net/wireless/zd1211rw/zd_mac.c |    7 ++++++-
>>>>>  1 files changed, 6 insertions(+), 1 deletions(-)
>>>>>
>>>>> diff --git a/drivers/net/wireless/zd1211rw/zd_mac.c
>>>>> b/drivers/net/wireless/zd1211rw/zd_mac.c
>>>>> index 98a574a..bed634c 100644
>>>>> --- a/drivers/net/wireless/zd1211rw/zd_mac.c
>>>>> +++ b/drivers/net/wireless/zd1211rw/zd_mac.c
>>>>> @@ -306,9 +306,14 @@ int zd_op_start(struct ieee80211_hw *hw)
>>>>>        r = set_mc_hash(mac);
>>>>>        if (r)
>>>>>                goto disable_int;
>>>>> +       msleep(10);
>>>>
>>>> You might want to stick a comment in here to tell future developers
>>>> why we're doing this.
>>>
>>>
>>> I was counting on the commit message to actually provide the detailed
>>> explanation, but you are right, some comment might be appropriate 
>>> here as
>>> well.
>>>
>>>
>>>>
>>>>> +
>>>>>        r = zd_chip_switch_radio_on(chip);
>>>>> -       if (r<  0)
>>>>> +       if (r<  0) {
>>>>> +               dev_err(zd_chip_dev(chip),
>>>>> +                       "%s: failed to set radio on\n", __func__);
>>>>>                goto disable_int;
>>>>> +       }
>>>>
>>>> It might also be an idea to have it re-try powering on the radio
>>>> after, say 100 msecs, just in case 10 msecs isn't enough for whatever
>>>> is causing this issue.
>>>
>>>
>>> Here I would rather let the error be printed in the kernel log for 
>>> people to
>>> report an issue if 10 msecs proves not to be enough for some systems.
>>
>> When I see timing like this added to established drivers it makes me
>> think that maybe something has changed since the code was originally
>> written, maybe there was a 10msec delay somewhere deep down in the USB
>> code that's now been eliminated, or something else equally random, and
>> maybe we'll need a longer wait in the future. Writing the code to try
>> it, say, 5 times will probably never completely time out, but it will
>> probably work until the driver is removed from the kernel, rather than
>> having to change this to a larger value some time in the future. On
>> the other hand, this fixes the bug you've found and maybe something
>> like that can wait.
>
> Quick search of "error ioread32(CR_REG1): -110" shows that first 
> reports are from 2008/2.6.24. People have found following workarounds 
> are "rmmod zd1211rw;modprobe zd1211rw", unplug/replug USB device, use 
> motherboard USB connector instead of hub, etc.

So, is the patch as-is acceptable, or do you want me to resubmit with 
more comments around the msleep()?

Thank you.
--
Florian

Re: [PATCH] zd1211rw: wait between setting hash table and powering radio on

[Jussi Kivilinna]

Quoting Florian Fainelli <florian@openwrt.org>:

> On 02/09/12 12:41, Jussi Kivilinna wrote:
>> Quoting Julian Calaby <julian.calaby@gmail.com>:
>>
>>> Hi Florian,
>>>
>>> On Thu, Feb 9, 2012 at 21:54, Florian Fainelli <florian@openwrt.org> wrote:
>>>> Hi Julian,
>>>>
>>>>
>>>> On 02/09/12 11:40, Julian Calaby wrote:
>>>>>
>>>>> Hi Florian,
>>>>>
>>>>> A couple of minor points:
>>>>>
>>>>> On Thu, Feb 9, 2012 at 21:24, Florian Fainelli<florian@openwrt.org>
>>>>> wrote:
>>>>>>
>>>>>> I am running Debian testing kernel 3.1.0-1-amd64, using a  
>>>>>> 079b:0062 Sagem
>>>>>> XG-76NA 802.11bg stick.
>>>>>>
>>>>>> Upon zd1211rw interface
>>>>>> bringup (ifconfig wlan0 up) I get the following timeout:
>>>>>>
>>>>>> [  950.330573] zd1211rw 1-3:1.0: phy2
>>>>>> [  955.108510] zd1211rw 1-3:1.0: firmware version 4725
>>>>>> [  955.148532] zd1211rw 1-3:1.0: zd1211b chip 079b:0062 v4810 high
>>>>>> 00-19-70
>>>>>> AL2230_RF pa0 g--NS
>>>>>> [snip]
>>>>>> [  955.204072] zd1211rw 1-3:1.0: error ioread32(CR_REG1): -110
>>>>>>
>>>>>> A second ifconfig wlan0 up brings the interface up without problems.
>>>>>>
>>>>>> After a bit more debugging, the call trace is the following:
>>>>>>
>>>>>> [10241.028130] zd1211rw 1-3:1.0: zd_chip_lock_phy_regs: error
>>>>>> ioread32(CR_REG1): -110
>>>>>> [10241.028140] zd1211rw 1-3:1.0: zd_switch_radio_on: failed to lock PHY
>>>>>> regs
>>>>>> [10241.028148] zd1211rw 1-3:1.0: zd_op_start: failed to set radio on
>>>>>>
>>>>>> Adding a 10 milliseconds delay between the call to set_mc_hash() and
>>>>>> zd_chip_switch_radio_on() allows successful interface bringups in all
>>>>>> cases and matches what the vendor driver did.
>>>>>>
>>>>>> Signed-off-by: Florian Fainelli<florian@openwrt.org>
>>>>>> ---
>>>>>> drivers/net/wireless/zd1211rw/zd_mac.c |    7 ++++++-
>>>>>> 1 files changed, 6 insertions(+), 1 deletions(-)
>>>>>>
>>>>>> diff --git a/drivers/net/wireless/zd1211rw/zd_mac.c
>>>>>> b/drivers/net/wireless/zd1211rw/zd_mac.c
>>>>>> index 98a574a..bed634c 100644
>>>>>> --- a/drivers/net/wireless/zd1211rw/zd_mac.c
>>>>>> +++ b/drivers/net/wireless/zd1211rw/zd_mac.c
>>>>>> @@ -306,9 +306,14 @@ int zd_op_start(struct ieee80211_hw *hw)
>>>>>>       r = set_mc_hash(mac);
>>>>>>       if (r)
>>>>>>               goto disable_int;
>>>>>> +       msleep(10);
>>>>>
>>>>> You might want to stick a comment in here to tell future developers
>>>>> why we're doing this.
>>>>
>>>>
>>>> I was counting on the commit message to actually provide the detailed
>>>> explanation, but you are right, some comment might be appropriate here as
>>>> well.
>>>>
>>>>
>>>>>
>>>>>> +
>>>>>>       r = zd_chip_switch_radio_on(chip);
>>>>>> -       if (r<  0)
>>>>>> +       if (r<  0) {
>>>>>> +               dev_err(zd_chip_dev(chip),
>>>>>> +                       "%s: failed to set radio on\n", __func__);
>>>>>>               goto disable_int;
>>>>>> +       }
>>>>>
>>>>> It might also be an idea to have it re-try powering on the radio
>>>>> after, say 100 msecs, just in case 10 msecs isn't enough for whatever
>>>>> is causing this issue.
>>>>
>>>>
>>>> Here I would rather let the error be printed in the kernel log  
>>>> for people to
>>>> report an issue if 10 msecs proves not to be enough for some systems.
>>>
>>> When I see timing like this added to established drivers it makes me
>>> think that maybe something has changed since the code was originally
>>> written, maybe there was a 10msec delay somewhere deep down in the USB
>>> code that's now been eliminated, or something else equally random, and
>>> maybe we'll need a longer wait in the future. Writing the code to try
>>> it, say, 5 times will probably never completely time out, but it will
>>> probably work until the driver is removed from the kernel, rather than
>>> having to change this to a larger value some time in the future. On
>>> the other hand, this fixes the bug you've found and maybe something
>>> like that can wait.
>>
>> Quick search of "error ioread32(CR_REG1): -110" shows that first  
>> reports are from 2008/2.6.24. People have found following  
>> workarounds are "rmmod zd1211rw;modprobe zd1211rw", unplug/replug  
>> USB device, use motherboard USB connector instead of hub, etc.
>
> So, is the patch as-is acceptable, or do you want me to resubmit  
> with more comments around the msleep()?

Comment would be good to have. You can add my acked-by to patch message..

Acked-by: Jussi Kivilinna <jussi.kivilinna@mbnet.fi>

>
> Thank you.
> --
> Florian
>
>

[PATCH v2] zd1211rw: wait between setting hash table and powering radio on

[Florian Fainelli]

I am running Debian testing kernel 3.1.0-1-amd64, using a 079b:0062 Sagem
XG-76NA 802.11bg stick.

Upon zd1211rw interface
bringup (ifconfig wlan0 up) I get the following timeout:

[  950.330573] zd1211rw 1-3:1.0: phy2
[  955.108510] zd1211rw 1-3:1.0: firmware version 4725
[  955.148532] zd1211rw 1-3:1.0: zd1211b chip 079b:0062 v4810 high 00-19-70
AL2230_RF pa0 g--NS
[snip]
[  955.204072] zd1211rw 1-3:1.0: error ioread32(CR_REG1): -110

A second ifconfig wlan0 up brings the interface up without problems.

After a bit more debugging, the call trace is the following:

[10241.028130] zd1211rw 1-3:1.0: zd_chip_lock_phy_regs: error
ioread32(CR_REG1): -110
[10241.028140] zd1211rw 1-3:1.0: zd_switch_radio_on: failed to lock PHY regs
[10241.028148] zd1211rw 1-3:1.0: zd_op_start: failed to set radio on

Adding a 10 milliseconds delay between the call to set_mc_hash() and
zd_chip_switch_radio_on() allows successful interface bringups in all
cases and matches what the vendor driver did.

Acked-by: Jussi Kivilinna <jussi.kivilinna@mbnet.fi>
Signed-off-by: Florian Fainelli <florian@openwrt.org>
---
Changes since v1:
- added comment in the code as to why this delay is required
- added Jussi's acked-by tag

 drivers/net/wireless/zd1211rw/zd_mac.c |   12 +++++++++++-
 1 files changed, 11 insertions(+), 1 deletions(-)

diff --git a/drivers/net/wireless/zd1211rw/zd_mac.c b/drivers/net/wireless/zd1211rw/zd_mac.c
index 98a574a..2d43e08 100644
--- a/drivers/net/wireless/zd1211rw/zd_mac.c
+++ b/drivers/net/wireless/zd1211rw/zd_mac.c
@@ -306,9 +306,19 @@ int zd_op_start(struct ieee80211_hw *hw)
 	r = set_mc_hash(mac);
 	if (r)
 		goto disable_int;
+
+	/* Wait after setting the mutlicast hash table and powering on
+	 * the radio otherwise interface bring up will fail. This matches
+	 * what the vendor driver did.
+	 */
+	msleep(10);
+
 	r = zd_chip_switch_radio_on(chip);
-	if (r < 0)
+	if (r < 0) {
+		dev_err(zd_chip_dev(chip),
+			"%s: failed to set radio on\n", __func__);
 		goto disable_int;
+	}
 	r = zd_chip_enable_rxtx(chip);
 	if (r < 0)
 		goto disable_radio;
-- 
1.7.5.4

Re: [PATCH v2] zd1211rw: wait between setting hash table and powering radio on

[Julian Calaby]

Hi Florian,

On Wed, Feb 29, 2012 at 20:36, Florian Fainelli <florian@openwrt.org> wrote:
> +       /* Wait after setting the mutlicast hash table and powering on

I hate to, but you've spelled multicast mutlicast.

Thanks,

-- 
Julian Calaby

Email: julian.calaby@gmail.com
Profile: http://www.google.com/profiles/julian.calaby/
.Plan: http://sites.google.com/site/juliancalaby/

[PATCH v3] zd1211rw: wait between setting hash table and powering radio on

[Florian Fainelli]

I am running Debian testing kernel 3.1.0-1-amd64, using a 079b:0062 Sagem
XG-76NA 802.11bg stick.

Upon zd1211rw interface
bringup (ifconfig wlan0 up) I get the following timeout:

[  950.330573] zd1211rw 1-3:1.0: phy2
[  955.108510] zd1211rw 1-3:1.0: firmware version 4725
[  955.148532] zd1211rw 1-3:1.0: zd1211b chip 079b:0062 v4810 high 00-19-70
AL2230_RF pa0 g--NS
[snip]
[  955.204072] zd1211rw 1-3:1.0: error ioread32(CR_REG1): -110

A second ifconfig wlan0 up brings the interface up without problems.

After a bit more debugging, the call trace is the following:

[10241.028130] zd1211rw 1-3:1.0: zd_chip_lock_phy_regs: error
ioread32(CR_REG1): -110
[10241.028140] zd1211rw 1-3:1.0: zd_switch_radio_on: failed to lock PHY regs
[10241.028148] zd1211rw 1-3:1.0: zd_op_start: failed to set radio on

Adding a 10 milliseconds delay between the call to set_mc_hash() and
zd_chip_switch_radio_on() allows successful interface bringups in all
cases and matches what the vendor driver did.

Acked-by: Jussi Kivilinna <jussi.kivilinna@mbnet.fi>
Signed-off-by: Florian Fainelli <florian@openwrt.org>
---
Changes since v2:
- fixed typo mutlicast instead of multicast in comment

Changes since v1:
- added comment in the code as to why this delay is required
- added Jussi's acked-by tag

 drivers/net/wireless/zd1211rw/zd_mac.c |   12 +++++++++++-
 1 files changed, 11 insertions(+), 1 deletions(-)

diff --git a/drivers/net/wireless/zd1211rw/zd_mac.c b/drivers/net/wireless/zd1211rw/zd_mac.c
index 98a574a..9fcde36 100644
--- a/drivers/net/wireless/zd1211rw/zd_mac.c
+++ b/drivers/net/wireless/zd1211rw/zd_mac.c
@@ -306,9 +306,19 @@ int zd_op_start(struct ieee80211_hw *hw)
 	r = set_mc_hash(mac);
 	if (r)
 		goto disable_int;
+
+	/* Wait after setting the multicast hash table and powering on
+	 * the radio otherwise interface bring up will fail. This matches
+	 * what the vendor driver did.
+	 */
+	msleep(10);
+
 	r = zd_chip_switch_radio_on(chip);
-	if (r < 0)
+	if (r < 0) {
+		dev_err(zd_chip_dev(chip),
+			"%s: failed to set radio on\n", __func__);
 		goto disable_int;
+	}
 	r = zd_chip_enable_rxtx(chip);
 	if (r < 0)
 		goto disable_radio;
-- 
1.7.5.4