[rfc patch] libertas: fix if_spi_prog_helper_firmware()

[rfc patch] libertas: fix if_spi_prog_helper_firmware()

[Dan Carpenter]

The indenting is not correct here.  I don't have this hardware and I'm
just guessing as to what was intended.  I think that if there is an
error we should return an error code, but if there isn't an error we
should return success directly without releasing the firmware.

Signed-off-by: Dan Carpenter <error27@gmail.com>

diff --git a/drivers/net/wireless/libertas/if_spi.c b/drivers/net/wireless/libertas/if_spi.c
index fe3f080..123a541 100644
--- a/drivers/net/wireless/libertas/if_spi.c
+++ b/drivers/net/wireless/libertas/if_spi.c
@@ -471,9 +471,12 @@ static int if_spi_prog_helper_firmware(struct if_spi_card *card)
 		goto release_firmware;
 	err = spu_write_u16(card, IF_SPI_CARD_INT_CAUSE_REG,
 				IF_SPI_CIC_CMD_DOWNLOAD_OVER);
+	if (err)
 		goto release_firmware;
 
-	lbs_deb_spi("waiting for helper to boot...\n");
+	lbs_deb_spi("helper firmware loaded...\n");
+
+	return 0;
 
 release_firmware:
 	release_firmware(firmware);

Re: [rfc patch] libertas: fix if_spi_prog_helper_firmware()

[Ben Hutchings]

[-- Attachment #1: Type: text/plain, Size: 723 bytes --]

On Tue, 2010-08-24 at 14:07 +0200, Dan Carpenter wrote:
> The indenting is not correct here.  I don't have this hardware and I'm
> just guessing as to what was intended.  I think that if there is an
> error we should return an error code, but if there isn't an error we
> should return success directly without releasing the firmware.
[...]

The driver doesn't use or refer to the firmware image once it's copied
into device RAM, so this just leaks the firmware.

The driver *should* keep a reference so it can restore the firmware
after suspend/resume without filesystem access (which is likely to
deadlock).

Ben.

-- 
Ben Hutchings
Once a job is fouled up, anything done to improve it makes it worse.

[-- Attachment #2: This is a digitally signed message part --]
[-- Type: application/pgp-signature, Size: 828 bytes --]

Re: [rfc patch] libertas: fix if_spi_prog_helper_firmware()

[Johannes Berg]

On Tue, 2010-08-24 at 14:07 +0200, Dan Carpenter wrote:
> The indenting is not correct here.  I don't have this hardware and I'm
> just guessing as to what was intended.  I think that if there is an
> error we should return an error code, but if there isn't an error we
> should return success directly without releasing the firmware.

>  		goto release_firmware;
>  	err = spu_write_u16(card, IF_SPI_CARD_INT_CAUSE_REG,
>  				IF_SPI_CIC_CMD_DOWNLOAD_OVER);
> +	if (err)
>  		goto release_firmware;
>  
> -	lbs_deb_spi("waiting for helper to boot...\n");
> +	lbs_deb_spi("helper firmware loaded...\n");
> +
> +	return 0;
>  
>  release_firmware:
>  	release_firmware(firmware);

This doesn't look correct, the caller of this function also sometimes
releases the firmware, but it looks like it could lead to a double-free?

johannes

Re: [rfc patch] libertas: fix if_spi_prog_helper_firmware()

[Colin McCabe]

On Tue, Aug 24, 2010 at 5:15 AM, Ben Hutchings <ben@decadent.org.uk> wrote:
> On Tue, 2010-08-24 at 14:07 +0200, Dan Carpenter wrote:
>> The indenting is not correct here.  I don't have this hardware and I'm
>> just guessing as to what was intended.  I think that if there is an
>> error we should return an error code, but if there isn't an error we
>> should return success directly without releasing the firmware.
> [...]
>
> The driver doesn't use or refer to the firmware image once it's copied
> into device RAM, so this just leaks the firmware.
>
> The driver *should* keep a reference so it can restore the firmware
> after suspend/resume without filesystem access (which is likely to
> deadlock).

Well, if all you want to do is put the device into "deep sleep" mode,
you will not need a firmware reload after waking it up.

P.S. I notice that if_spi.c is still setting priv->enter_deep_sleep = NULL
Too bad. This feature IS available with the SPI interface.

regards,
Colin McCabe
--
To unsubscribe from this list: send the line "unsubscribe kernel-janitors" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [rfc patch] libertas: fix if_spi_prog_helper_firmware()

[Ben Hutchings]

[-- Attachment #1: Type: text/plain, Size: 1138 bytes --]

On Wed, 2010-08-25 at 20:26 -0700, Colin McCabe wrote:
> On Tue, Aug 24, 2010 at 5:15 AM, Ben Hutchings <ben@decadent.org.uk> wrote:
> > On Tue, 2010-08-24 at 14:07 +0200, Dan Carpenter wrote:
> >> The indenting is not correct here.  I don't have this hardware and I'm
> >> just guessing as to what was intended.  I think that if there is an
> >> error we should return an error code, but if there isn't an error we
> >> should return success directly without releasing the firmware.
> > [...]
> >
> > The driver doesn't use or refer to the firmware image once it's copied
> > into device RAM, so this just leaks the firmware.
> >
> > The driver *should* keep a reference so it can restore the firmware
> > after suspend/resume without filesystem access (which is likely to
> > deadlock).
> 
> Well, if all you want to do is put the device into "deep sleep" mode,
> you will not need a firmware reload after waking it up.
[...]

I was thinking of system suspend/resume, which could remove all power
from the device.

Ben.

-- 
Ben Hutchings
Once a job is fouled up, anything done to improve it makes it worse.

[-- Attachment #2: This is a digitally signed message part --]
[-- Type: application/pgp-signature, Size: 828 bytes --]

Re: [rfc patch] libertas: fix if_spi_prog_helper_firmware()

[Dan Williams]

On Tue, 2010-08-24 at 14:07 +0200, Dan Carpenter wrote:
> The indenting is not correct here.  I don't have this hardware and I'm
> just guessing as to what was intended.  I think that if there is an
> error we should return an error code, but if there isn't an error we
> should return success directly without releasing the firmware.
> 
> Signed-off-by: Dan Carpenter <error27@gmail.com>

I've significantly changed firmware loading in wireless-testing which
should hit the next merge window.  It won't have this problem, and it
does correctly release the firmware later on.  It does preserve the
existing behavior of releasing the firmware after load, instead of
keeping it around for resume.  If there are fixes I think they should be
against wireless-testing actually since that's what's "next".

Dan

> diff --git a/drivers/net/wireless/libertas/if_spi.c b/drivers/net/wireless/libertas/if_spi.c
> index fe3f080..123a541 100644
> --- a/drivers/net/wireless/libertas/if_spi.c
> +++ b/drivers/net/wireless/libertas/if_spi.c
> @@ -471,9 +471,12 @@ static int if_spi_prog_helper_firmware(struct if_spi_card *card)
>  		goto release_firmware;
>  	err = spu_write_u16(card, IF_SPI_CARD_INT_CAUSE_REG,
>  				IF_SPI_CIC_CMD_DOWNLOAD_OVER);
> +	if (err)
>  		goto release_firmware;
>  
> -	lbs_deb_spi("waiting for helper to boot...\n");
> +	lbs_deb_spi("helper firmware loaded...\n");
> +
> +	return 0;
>  
>  release_firmware:
>  	release_firmware(firmware);
> --
> To unsubscribe from this list: send the line "unsubscribe linux-wireless" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [rfc patch] libertas: fix if_spi_prog_helper_firmware()

[Colin McCabe]

On Wed, Aug 25, 2010 at 8:37 PM, Ben Hutchings <ben@decadent.org.uk> wrote:
> On Wed, 2010-08-25 at 20:26 -0700, Colin McCabe wrote:
>> On Tue, Aug 24, 2010 at 5:15 AM, Ben Hutchings <ben@decadent.org.uk> wrote:
>> > On Tue, 2010-08-24 at 14:07 +0200, Dan Carpenter wrote:
>> >> The indenting is not correct here.  I don't have this hardware and I'm
>> >> just guessing as to what was intended.  I think that if there is an
>> >> error we should return an error code, but if there isn't an error we
>> >> should return success directly without releasing the firmware.
>> > [...]
>> >
>> > The driver doesn't use or refer to the firmware image once it's copied
>> > into device RAM, so this just leaks the firmware.
>> >
>> > The driver *should* keep a reference so it can restore the firmware
>> > after suspend/resume without filesystem access (which is likely to
>> > deadlock).
>>
>> Well, if all you want to do is put the device into "deep sleep" mode,
>> you will not need a firmware reload after waking it up.
> [...]
>
> I was thinking of system suspend/resume, which could remove all power
> from the device.
>

Ok, that makes sense.

Colin
--
To unsubscribe from this list: send the line "unsubscribe kernel-janitors" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [rfc patch] libertas: fix if_spi_prog_helper_firmware()

[Paul Fox]

dan wrote:
 > On Tue, 2010-08-24 at 14:07 +0200, Dan Carpenter wrote:
 > > The indenting is not correct here.  I don't have this hardware and I'm
 > > just guessing as to what was intended.  I think that if there is an
 > > error we should return an error code, but if there isn't an error we
 > > should return success directly without releasing the firmware.
 > > 
 > > Signed-off-by: Dan Carpenter <error27@gmail.com>
 > 
 > I've significantly changed firmware loading in wireless-testing which
 > should hit the next merge window.  It won't have this problem, and it
 > does correctly release the firmware later on.  It does preserve the
 > existing behavior of releasing the firmware after load, instead of
 > keeping it around for resume.  If there are fixes I think they should be
 > against wireless-testing actually since that's what's "next".

as part of the firmware and suspend/resume topic:  on the XO 1.5
laptop, running 2.6.31, we see an apparent leak of the firmware on
every suspend/resume where the card is powered down.  (our driver
keeps the wlan module powered acros s/r if there are wakeup events
configured, otherwise it powers down -- i can't remember if that change
has been upstreamed or not.) our trac ticket is here: 
http://dev.laptop.org/ticket/9928

i got as far as deciding that the leak wasn't in the libertas driver
itself before we decided we had more important fish to fry, so our
investigation is/was incomplete.  from what i found, it seemed like
perhaps the leak would go away if the driver cached a copy of the
firmware, as other drivers do.  but that still leaves the suspicion
that there's another copy hanging around in the download path still.

paul

 > 
 > Dan
 > 
 > > diff --git a/drivers/net/wireless/libertas/if_spi.c 
 > b/drivers/net/wireless/libertas/if_spi.c
 > > index fe3f080..123a541 100644
 > > --- a/drivers/net/wireless/libertas/if_spi.c
 > > +++ b/drivers/net/wireless/libertas/if_spi.c
 > > @@ -471,9 +471,12 @@ static int if_spi_prog_helper_firmware(struct 
 > if_spi_card *card)
 > >  		goto release_firmware;
 > >  	err = spu_write_u16(card, IF_SPI_CARD_INT_CAUSE_REG,
 > >  				IF_SPI_CIC_CMD_DOWNLOAD_OVER);
 > > +	if (err)
 > >  		goto release_firmware;
 > >  
 > > -	lbs_deb_spi("waiting for helper to boot...\n");
 > > +	lbs_deb_spi("helper firmware loaded...\n");
 > > +
 > > +	return 0;
 > >  
 > >  release_firmware:
 > >  	release_firmware(firmware);
 > > --
 > > To unsubscribe from this list: send the line "unsubscribe linux-wireless" in
 > > the body of a message to majordomo@vger.kernel.org
 > > More majordomo info at  http://vger.kernel.org/majordomo-info.html
 > 
 > 
 > 
 > _______________________________________________
 > libertas-dev mailing list
 > libertas-dev@lists.infradead.org
 > http://lists.infradead.org/mailman/listinfo/libertas-dev

=---------------------
 paul fox, pgf@laptop.org