libertas: fix if_spi_prog_helper_firmware()
Subsystems (2)
RFC PATCH 1/1 libertas: fix if_spi_prog_helper_firmware()
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.
drivers/net/wireless/libertas/if_spi.c +4 -1
@@ -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);
Quoted 4 lines · 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.
[...]
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.
Signature · 2 lines
Ben Hutchings Once a job is fouled up, anything done to improve it makes it worse.
Quoted 5 lines · Ben Hutchings
Quoted 4 lines · 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.
Quoted 8 lines
[...]
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 [email protected]
More majordomo info at http://vger.kernel.org/majordomo-info.html
Quoted 11 lines · Colin McCabe
Quoted 10 lines · Ben Hutchings
Quoted 4 lines · 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.
[...]
The driver doesn't use or refer to the firmware image once it's copied into device RAM, so this just leaks the firmware.
Quoted 6 lines
Quoted 3 lines
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.
Signature · 2 lines
Ben Hutchings Once a job is fouled up, anything done to improve it makes it worse.
Quoted 16 lines · Ben Hutchings
Quoted 15 lines · Colin McCabe
Quoted 13 lines · Ben Hutchings
Quoted 4 lines · 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.
[...]
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).
Quoted 7 lines
Quoted 2 lines
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 [email protected]
More majordomo info at http://vger.kernel.org/majordomo-info.html
Johannes Berg
maintainerQuoted 8 lines · 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.
drivers/net/wireless/libertas/if_spi.c +0
-0goto release_firmware; err =spu_write_u16 (card,IF_SPI_CARD_INT_CAUSE_REG ,IF_SPI_CIC_CMD_DOWNLOAD_OVER );
drivers/net/wireless/libertas/if_spi.c static int if_spi_prog_helper_firmware(struct if_spi_card *card) · line 482
Quoted 10 lines
drivers/net/wireless/libertas/if_spi.c +4
-1+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
Quoted 6 lines · 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 <[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
Quoted 22 lines
drivers/net/wireless/libertas/if_spi.c +4
-2@@ -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
Quoted 6 lines · dan
Quoted 5 lines · 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.
Quoted 8 lines
Quoted 1 line
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
Quoted 21 lines
Dan
Review
Quoted 1 line
(quoted diff) +0
-0
b/drivers/net/wireless/libertas/if_spi.cdrivers/net/wireless/libertas/if_spi.c static int if_spi_prog_helper_firmware(struct
Quoted 4 lines
index fe3f080..123a541 100644
drivers/net/wireless/libertas/if_spi.c +0
-0@@ -471,9 +471,12 @@ static int if_spi_prog_helper_firmware(struct
if_spi_card*card)Quoted 11 lines
drivers/net/wireless/libertas/if_spi.c +4
-1goto 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 ;
Quoted 13 lines
Quoted 6 lines
drivers/net/wireless/libertas/if_spi.c +0
-1release_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]