if_cs_poll_while_fw_download() returned the number of iterations
remaining on success, which in turn got returned as the value from
if_cs_prog_real() and if_cs_prog_helper(). But since if_cs_probe()
interprets non-zero return values from firmware load functions as an
error, this sometimes caused spurious firmware load failures.
Signed-off-by: Dan Williams <[email protected]>
---
diff --git a/drivers/net/wireless/libertas/if_cs.c b/drivers/net/wireless/libertas/if_cs.c
index 842a08d..8f8934a 100644
--- a/drivers/net/wireless/libertas/if_cs.c
+++ b/drivers/net/wireless/libertas/if_cs.c
@@ -151,7 +151,7 @@ static int if_cs_poll_while_fw_download(struct if_cs_card *card, uint addr, u8 r
for (i = 0; i < 100000; i++) {
u8 val = if_cs_read8(card, addr);
if (val == reg)
- return i;
+ return 0;
udelay(5);
}
return -ETIME;
On Fri, 2009-01-23 at 18:01 +0100, Michael Buesch wrote:
> On Friday 23 January 2009 17:55:33 Dan Williams wrote:
> > if_cs_poll_while_fw_download() returned the number of iterations
> > remaining on success, which in turn got returned as the value from
> > if_cs_prog_real() and if_cs_prog_helper(). But since if_cs_probe()
> > interprets non-zero return values from firmware load functions as an
> > error, this sometimes caused spurious firmware load failures.
> >
> > Signed-off-by: Dan Williams <[email protected]>
> > ---
> >
> > diff --git a/drivers/net/wireless/libertas/if_cs.c b/drivers/net/wireless/libertas/if_cs.c
> > index 842a08d..8f8934a 100644
> > --- a/drivers/net/wireless/libertas/if_cs.c
> > +++ b/drivers/net/wireless/libertas/if_cs.c
> > @@ -151,7 +151,7 @@ static int if_cs_poll_while_fw_download(struct if_cs_card *card, uint addr, u8 r
> > for (i = 0; i < 100000; i++) {
> > u8 val = if_cs_read8(card, addr);
> > if (val == reg)
> > - return i;
> > + return 0;
> > udelay(5);
> > }
> > return -ETIME;
>
> This is an incredibly expensive loop, btw. Even for init it's really painful to
> hog the CPU for more than half a second. Especially on UP.
> Any chance for msleep()?
Could be, Holger did the initial patch and I'm not sure if the udelay
was intentional. Note that this happens at PCMCIA/CF hardware probe
time (if that has any constraints that might apply here).
dan
On Sat, 2009-01-24 at 21:37 +1300, Ryan Mallon wrote:
> Dan Williams wrote:
> > if_cs_poll_while_fw_download() returned the number of iterations
> > remaining on success, which in turn got returned as the value from
> > if_cs_prog_real() and if_cs_prog_helper(). But since if_cs_probe()
> > interprets non-zero return values from firmware load functions as an
> > error, this sometimes caused spurious firmware load failures.
> >
> > Signed-off-by: Dan Williams <[email protected]>
> > ---
> >
> > diff --git a/drivers/net/wireless/libertas/if_cs.c b/drivers/net/wireless/libertas/if_cs.c
> > index 842a08d..8f8934a 100644
> > --- a/drivers/net/wireless/libertas/if_cs.c
> > +++ b/drivers/net/wireless/libertas/if_cs.c
> > @@ -151,7 +151,7 @@ static int if_cs_poll_while_fw_download(struct if_cs_card *card, uint addr, u8 r
> > for (i = 0; i < 100000; i++) {
> > u8 val = if_cs_read8(card, addr);
> > if (val == reg)
> > - return i;
> > + return 0;
> > udelay(5);
> > }
> > return -ETIME;
> >
> >
>
> Hmmm, I posted this same patch here a couple of months ago
> (http://lists.infradead.org/pipermail/libertas-dev/2008-October/002019.html),
> did I get something wrong in the submission process, or did it just get
> lost in the woodwork? Anyway:
Apparently I didn't read enough down that thread to find the second
patch; I only found the one that changed if_cs_prog_helper() and
if_cs_prog_real(). My mistake, this patch is yours.
John; when you apply, please credit Ryan.
Dan
> Could be, Holger did the initial patch and I'm not sure if the
> udelay was intentional. Note that this happens at PCMCIA/CF
> hardware probe time (if that has any constraints that might
> apply here).
I'm used the card on an ARM architecture, and there long mdelay
won't work as on i386, so I used udelay.
In practice, the "expensive loop" isn't that expensive, because
the real number of iterations is quite low (contrary to what the
manual states).
On Friday 23 January 2009 17:55:33 Dan Williams wrote:
> if_cs_poll_while_fw_download() returned the number of iterations
> remaining on success, which in turn got returned as the value from
> if_cs_prog_real() and if_cs_prog_helper(). But since if_cs_probe()
> interprets non-zero return values from firmware load functions as an
> error, this sometimes caused spurious firmware load failures.
>
> Signed-off-by: Dan Williams <[email protected]>
> ---
>
> diff --git a/drivers/net/wireless/libertas/if_cs.c b/drivers/net/wireless/libertas/if_cs.c
> index 842a08d..8f8934a 100644
> --- a/drivers/net/wireless/libertas/if_cs.c
> +++ b/drivers/net/wireless/libertas/if_cs.c
> @@ -151,7 +151,7 @@ static int if_cs_poll_while_fw_download(struct if_cs_card *card, uint addr, u8 r
> for (i = 0; i < 100000; i++) {
> u8 val = if_cs_read8(card, addr);
> if (val == reg)
> - return i;
> + return 0;
> udelay(5);
> }
> return -ETIME;
This is an incredibly expensive loop, btw. Even for init it's really painful to
hog the CPU for more than half a second. Especially on UP.
Any chance for msleep()?
--
Greetings, Michael.
Dan Williams wrote:
> if_cs_poll_while_fw_download() returned the number of iterations
> remaining on success, which in turn got returned as the value from
> if_cs_prog_real() and if_cs_prog_helper(). But since if_cs_probe()
> interprets non-zero return values from firmware load functions as an
> error, this sometimes caused spurious firmware load failures.
>
> Signed-off-by: Dan Williams <[email protected]>
> ---
>
> diff --git a/drivers/net/wireless/libertas/if_cs.c b/drivers/net/wireless/libertas/if_cs.c
> index 842a08d..8f8934a 100644
> --- a/drivers/net/wireless/libertas/if_cs.c
> +++ b/drivers/net/wireless/libertas/if_cs.c
> @@ -151,7 +151,7 @@ static int if_cs_poll_while_fw_download(struct if_cs_card *card, uint addr, u8 r
> for (i = 0; i < 100000; i++) {
> u8 val = if_cs_read8(card, addr);
> if (val == reg)
> - return i;
> + return 0;
> udelay(5);
> }
> return -ETIME;
>
>
Hmmm, I posted this same patch here a couple of months ago
(http://lists.infradead.org/pipermail/libertas-dev/2008-October/002019.html),
did I get something wrong in the submission process, or did it just get
lost in the woodwork? Anyway:
Tested-by: Ryan Mallon <[email protected]>
Acked-by: Ryan Mallon <[email protected]>
~Ryan