[RFC] if_spi.c: enable host interrupts after host ISR is registered

[Andrey Yurovsky]

Hi George,

On Fri, Oct 9, 2009 at 3:37 AM, George Shore <George.Shore at imgtec.com> wrote:
> During the firmware loading process a number of interrupts are fired
> within this code. However, this happens when interrupts are being masked
> out. When the host interrupts are enabled after the download an
> interrupt is fired but left unhandled by the time the ISR is registered,
> causing timeouts on every command until a warm reset is made. This issue
> does not happen if the firmware code does not need to be downloaded.
>
> This moves the call to spu_set_interrupt_mode() to after the ISR is
> registered, so that the interrupt caused by the firmware load can be
> handled by the ISR (which currently does nothing with it anyway).
>
> Any thoughts upon this?

We haven't seen the command timeouts you're describing with the driver
the way it is.  What's your setup like?

With your patch applied, I see timeouts:

[17179586.484000] libertas_spi: Libertas SPI driver
[17179586.528000] libertas_spi spi0.0: firmware: requesting libertas/gspi8686_hn
[17179587.640000] libertas_spi spi0.0: firmware: requesting libertas/gspi8686.bn
[17179591.916000] libertas: command 0x0003 timed out
[17179591.920000] libertas: requeueing command 0x0003 due to timeout (#1)
[17179594.924000] libertas: command 0x0003 timed out
[17179594.928000] libertas: requeueing command 0x0003 due to timeout (#2)
[17179597.936000] libertas: command 0x0003 timed out
[17179597.940000] libertas: requeueing command 0x0003 due to timeout (#3)
[17179600.944000] libertas: command 0x0003 timed out
[17179600.948000] libertas: Excessive timeouts submitting command 0x0003
[17179600.952000] libertas: PREP_CMD: command 0x0003 failed: -110
[17179600.960000] libertas_spi: probe of spi0.0 failed with error -110

Also one minor issue with the patch,

> ---
> drivers/net/wireless/libertas/if_spi.c | 11 +++++++----
> 1 files changed, 7 insertions(+), 4 deletions(-)
>
> diff --git a/drivers/net/wireless/libertas/if_spi.c
> b/drivers/net/wireless/libertas/if_spi.c
> index 6564282..f95d9e8 100644
> --- a/drivers/net/wireless/libertas/if_spi.c
> +++ b/drivers/net/wireless/libertas/if_spi.c
> @@ -1103,10 +1103,6 @@ static int __devinit if_spi_probe(struct
> spi_device *spi)
> lbs_deb_spi("loaded FW for Marvell WLAN 802.11 adapter\n");
> }
>
> - err = spu_set_interrupt_mode(card, 0, 1);
> - if (err)
> - goto free_card;
> -
> /* Register our card with libertas.
> * This will call alloc_etherdev */
> priv = lbs_add_card(card, &spi->dev);
> @@ -1139,6 +1135,13 @@ static int __devinit if_spi_probe(struct
> spi_device *spi)
> goto terminate_thread;
> }
>
> + /* Enable the card->host interrupts now that the ISR has been
> + * registered. This will allow the driver to handle the interrupts
> + * that are fired when the firmware is loading. */
> + err = spu_set_interrupt_mode(card, 0, 1);
> + if (err)
> + goto free_card;
> +

If we fail here, we should "goto terminate_thread", "free_card" made
sense where that line was originally located but we need to undo more
if we fail later on as you have it now.

> /* Start the card.
> * This will call register_netdev, and we'll start
> * getting interrupts... */
>
> --
> 1.6.5.rc2
> -
> This message is subject to Imagination Technologies' e-mail terms: http://www.imgtec.com/e-mail.htm
>
> Imagination Technologies Ltd is a limited company registered in England No:  1306335
> Registered Office: Imagination House, Home Park Estate, Kings Langley, Hertfordshire, WD4 8LZ.
>
> Email to and from the company may be monitored for compliance and other administrative purposes.
> -
>
>
> _______________________________________________
> libertas-dev mailing list
> libertas-dev at lists.infradead.org
> http://lists.infradead.org/mailman/listinfo/libertas-dev
>