From mboxrd@z Thu Jan 1 00:00:00 1970 Return-path: Received: from mail-bk0-f46.google.com ([209.85.214.46]:41993 "EHLO mail-bk0-f46.google.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1754169Ab2BOKdx (ORCPT ); Wed, 15 Feb 2012 05:33:53 -0500 Received: by bkcjm19 with SMTP id jm19so772230bkc.19 for ; Wed, 15 Feb 2012 02:33:52 -0800 (PST) Message-ID: <4F3B89EA.8080501@openwrt.org> (sfid-20120215_113357_628444_2051148F) Date: Wed, 15 Feb 2012 11:33:14 +0100 From: Florian Fainelli MIME-Version: 1.0 To: Jussi Kivilinna CC: Julian Calaby , linville@tuxdriver.com, linux-wireless@vger.kernel.org, dsd@gentoo.org, kune@deine-taler.de Subject: Re: [PATCH] zd1211rw: wait between setting hash table and powering radio on References: <20120208180102.18346jfdlm9wakw8@www.81.fi> <1328783095-8330-1-git-send-email-florian@openwrt.org> <4F33A5DE.8020208@openwrt.org> <20120209134130.49211ca9upll7y00@www.81.fi> In-Reply-To: <20120209134130.49211ca9upll7y00@www.81.fi> Content-Type: text/plain; charset=UTF-8; format=flowed Sender: linux-wireless-owner@vger.kernel.org List-ID: On 02/09/12 12:41, Jussi Kivilinna wrote: > Quoting Julian Calaby : > >> Hi Florian, >> >> On Thu, Feb 9, 2012 at 21:54, Florian Fainelli >> 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 >>>> 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 >>>>> --- >>>>> 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