Hi Florian,
On Thu, Feb 9, 2012 at 21:54, Florian Fainelli <florian@xxxxxxxxxxx> 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@xxxxxxxxxxx>
>> 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@xxxxxxxxxxx>
>>> ---
>>> 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@xxxxxxxxx
Profile: http://www.google.com/profiles/julian.calaby/
.Plan: http://sites.google.com/site/juliancalaby/
--
To unsubscribe from this list: send the line "unsubscribe linux-wireless" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
