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 <[email protected]>
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);
On Tue, Aug 24, 2010 at 5:15 AM, Ben Hutchings <[email protected]> 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
On Wed, 2010-08-25 at 20:26 -0700, Colin McCabe wrote:
> On Tue, Aug 24, 2010 at 5:15 AM, Ben Hutchings <[email protected]> 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.
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
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 <[email protected]>
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 [email protected]
> More majordomo info at http://vger.kernel.org/majordomo-info.html
On Wed, Aug 25, 2010 at 8:37 PM, Ben Hutchings <[email protected]> wrote:
> On Wed, 2010-08-25 at 20:26 -0700, Colin McCabe wrote:
>> On Tue, Aug 24, 2010 at 5:15 AM, Ben Hutchings <[email protected]> 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
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.
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 <[email protected]>
>
> 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 [email protected]
> > More majordomo info at http://vger.kernel.org/majordomo-info.html
>
>
>
> _______________________________________________
> libertas-dev mailing list
> [email protected]
> http://lists.infradead.org/mailman/listinfo/libertas-dev
=---------------------
paul fox, [email protected]