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

Mon Oct 12 12:21:46 EDT 2009

On Mon, Oct 12, 2009 at 5:57 AM, George Shore <George.Shore at imgtec.com> wrote:
> Hi Andrey,
>
>> -----Original Message-----
>> From: Andrey Yurovsky [mailto:andrey at cozybit.com]
>> Sent: 09 October 2009 17:17
>> To: George Shore
>> Cc: libertas-dev at lists.infradead.org; Dan Williams
>> Subject: Re: [RFC] if_spi.c: enable host interrupts after host ISR is
>> registered
>>
>> 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?
>>
>
> I'm using the 8686 via gspi with on in-house hardware with an in-house CPU. I'm beginning to think the problem is more on our side of the fence...
>
>> 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
>>
>
> Yeah, I thought it was a little too good to be true to be changing bits of a device driver that works for other people to make it work for me :)
>
> Without the patch, I was receiving:
> [  329.087773] libertas_spi: Libertas SPI driver
> [  330.154280] libertas_spi spi0.1: firmware: using built-in firmware libertas/gspi8686_hlp.bin
> [  468.726590] libertas_spi spi0.1: firmware: using built-in firmware libertas/gspi8686.bin
> [  526.401588] libertas: command 0x0003 timed out
> [  526.426667] libertas: requeueing command 0x0003 due to timeout (#1)
> [  526.466897] libertas: Received result 0 to command 3 after 1 retries
> [  526.494199] libertas: 00:13:e0:b8:61:46, fw 9.70.3p37, cap 0x00000303
> [  526.506927] libertas: unidentified region code; using the default (USA)
> [  526.546953] libertas: Received CMD_RESP with invalid sequence 1 (expected 2)
> [  529.531525] libertas: command 0x001e timed out
> [  529.557658] libertas: requeueing command 0x001e due to timeout (#1)
> [  529.595596] libertas: Received result 0 to command 1e after 1 retries
> [  529.687728] libertas: Received CMD_RESP with invalid sequence 2 (expected 3)
> [  532.671527] libertas: command 0x0028 timed out
> [  532.698173] libertas: requeueing command 0x0028 due to timeout (#1)
> [  532.736723] libertas: Received result 0 to command 28 after 1 retries
> [  532.775889] libertas: Received CMD_RESP with invalid sequence 3 (expected 4)
> [  535.761536] libertas: command 0x001d timed out
> [  535.788422] libertas: requeueing command 0x001d due to timeout (#1)
> [  535.829696] libertas: Received result 0 to command 1d after 1 retries
> [  535.855839] libertas: wlan0: Marvell WLAN 802.11 adapter

Interesting.  It looks like you missed the first command response
entirely (probably due to missing the interrupt associated with it)
and, thereafter, you're getting sequence number errors because the
host and device are out of sync.

> This goes on until a warm reset (where the firmware is still loaded) and then everything is fine. I'm sure it's an interrupt issue, and the more I think about it the more I tend towards the interrupt code we have for the board being the cause...

Yeah, please take a look at your interrupt-related code and see if you
can be more confident about it.  There's certainly a possibility that
we have a real driver bug that we happen to not hit on other systems,
but there could of course be a reason for the lost interrupt on your
side.  You might want to check if your interrupt source configured for
level or edge trigger, and of course if there's any noise or abnormal
behavior when you check it with a scope.

>> 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.
>>
>
> Eeek... I knew there was something I'd forgotten to change. I blame the rush of adrenaline from submitting something to a mailing list ;)
>
>> > /* 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
>> >
>
> Thanks for the feedback - I've think I should've asked questions first, THEN posted a patch suggestion! :)
>
> Regards,
> George
> -
> 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.
> -
>
>