linux-kernel.vger.kernel.org archive mirror
 help / color / mirror / Atom feed
* [Patch 1/1]: libertas/sdio: fix releasing memory twice.
@ 2013-10-12 16:02 Dr. H. Nikolaus Schaller
  2013-10-12 19:58 ` Dan Williams
  0 siblings, 1 reply; 4+ messages in thread
From: Dr. H. Nikolaus Schaller @ 2013-10-12 16:02 UTC (permalink / raw)
  To: John W. Linville, Bing Zhao, H. Nikolaus Schaller, Dan Williams,
	Harro Haan, libertas-dev, linux-wireless, netdev, LKML,
	Belisko Marek, NeilBrown Brown

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

While upgrading the GTA04 kernel to 3.12-rc4 we came across
an issue with libertas/sdio referencing stale memory on ifconfig up
when trying to load the firmware (for a second time).

I am not at all sure if the patch is how it should be done and the right
location, but it appears to work for us with resetting priv->helper_fw to NULL
before asynchronously loading the firmware again.




[-- Attachment #2: 0001-libertas-sdio-fix-releasing-memory-twice.patch --]
[-- Type: application/octet-stream, Size: 1850 bytes --]

From f6864491ea45d2bd877a37fbb4a618e42fe03fbe Mon Sep 17 00:00:00 2001
From: "H. Nikolaus Schaller" <hns@goldelico.com>
Date: Sat, 12 Oct 2013 17:49:31 +0200
Subject: [PATCH] libertas/sdio: fix releasing memory twice. We have connected
 a Wi2Wi W2CBW003 to an OMAP3 using SDIO. We have seen an
 issue (not related with this patch) that sometimes power is
 not turned off. This did lead to a kernel Oops if an
 ifconfig up / down / up when the chip was not powered down.
 This leads to a second call to lbs_get_firmware_async()
 with the same priv data - and that tries to
 release_firmware(priv->helper_fw); This appears to be
 wrong, since it was alredy released in the
 if_sdio_do_prog_firmware.

Signed-off-by: H. Nikolaus Schaller <hns@goldelico.com>
---
 drivers/net/wireless/libertas/if_sdio.c |   13 +++++++++++++
 1 files changed, 13 insertions(+), 0 deletions(-)

diff --git a/drivers/net/wireless/libertas/if_sdio.c b/drivers/net/wireless/libertas/if_sdio.c
index 4557833..a04eb41 100644
--- a/drivers/net/wireless/libertas/if_sdio.c
+++ b/drivers/net/wireless/libertas/if_sdio.c
@@ -769,6 +769,19 @@ static int if_sdio_prog_firmware(struct if_sdio_card *card)
 		return 0;
 	}
 
+	/* This is missing in lbs_get_firmware_async()
+	 * and therefore a second call using the same priv structure
+	 * may find a stale helper_fw entry that has already been
+	 * released by release_firmware(helper) in
+	 * if_sdio_do_prog_firmware().
+	 * Or doing that release in if_sdio_do_prog_firmware()
+	 * is a duplicate and should not be there.
+	 * Anyways, this can happen if a ifconfig up / down / up
+	 * sequence is issued.
+	 */
+
+	card->priv->helper_fw = NULL;
+
 	ret = lbs_get_firmware_async(card->priv, &card->func->dev, card->model,
 				     fw_table, if_sdio_do_prog_firmware);
 
-- 
1.7.7.4


^ permalink raw reply related	[flat|nested] 4+ messages in thread

* Re: [Patch 1/1]: libertas/sdio: fix releasing memory twice.
  2013-10-12 16:02 [Patch 1/1]: libertas/sdio: fix releasing memory twice Dr. H. Nikolaus Schaller
@ 2013-10-12 19:58 ` Dan Williams
  2013-10-13  7:09   ` Dr. H. Nikolaus Schaller
  0 siblings, 1 reply; 4+ messages in thread
From: Dan Williams @ 2013-10-12 19:58 UTC (permalink / raw)
  To: Dr. H. Nikolaus Schaller
  Cc: John W. Linville, Bing Zhao, Harro Haan, libertas-dev,
	linux-wireless, netdev, LKML, Belisko Marek, NeilBrown Brown

On Sat, 2013-10-12 at 18:02 +0200, Dr. H. Nikolaus Schaller wrote:
> While upgrading the GTA04 kernel to 3.12-rc4 we came across
> an issue with libertas/sdio referencing stale memory on ifconfig up
> when trying to load the firmware (for a second time).
> 
> I am not at all sure if the patch is how it should be done and the right
> location, but it appears to work for us with resetting priv->helper_fw to NULL
> before asynchronously loading the firmware again.

How about we go even simpler?  helper_fw is *only* used inside
firmware.c anyway, and that's probably where its lifetime should be
handled.  Same for the main firmware.  I have no idea why the bus code
is responsible for releasing the firmware anyway, when it originates
from firmware.c and control returns back there after the firmware loaded
callback is done.  Does the following patch fix your problem too?

Dan

---
diff --git a/drivers/net/wireless/libertas/firmware.c b/drivers/net/wireless/libertas/firmware.c
index c0f9e7e..51b92b5 100644
--- a/drivers/net/wireless/libertas/firmware.c
+++ b/drivers/net/wireless/libertas/firmware.c
@@ -49,14 +49,19 @@ static void main_firmware_cb(const struct firmware *firmware, void *context)
 		/* Failed to find firmware: try next table entry */
 		load_next_firmware_from_table(priv);
 		return;
 	}
 
 	/* Firmware found! */
 	lbs_fw_loaded(priv, 0, priv->helper_fw, firmware);
+	if (priv->helper_fw) {
+		release_firmware (priv->helper_fw);
+		priv->helper_fw = NULL;
+	}
+	release_firmware (firmware);
 }
 
 static void helper_firmware_cb(const struct firmware *firmware, void *context)
 {
 	struct lbs_private *priv = context;
 
 	if (!firmware) {
diff --git a/drivers/net/wireless/libertas/if_cs.c b/drivers/net/wireless/libertas/if_cs.c
index c94dd68..ef8c98e 100644
--- a/drivers/net/wireless/libertas/if_cs.c
+++ b/drivers/net/wireless/libertas/if_cs.c
@@ -750,22 +750,22 @@ static void if_cs_prog_firmware(struct lbs_private *priv, int ret,
 	}
 
 	/* Load the firmware */
 	ret = if_cs_prog_helper(card, helper);
 	if (ret == 0 && (card->model != MODEL_8305))
 		ret = if_cs_prog_real(card, mainfw);
 	if (ret)
-		goto out;
+		return;
 
 	/* Now actually get the IRQ */
 	ret = request_irq(card->p_dev->irq, if_cs_interrupt,
 		IRQF_SHARED, DRV_NAME, card);
 	if (ret) {
 		pr_err("error in request_irq\n");
-		goto out;
+		return;
 	}
 
 	/*
 	 * Clear any interrupt cause that happened while sending
 	 * firmware/initializing card
 	 */
 	if_cs_write16(card, IF_CS_CARD_INT_CAUSE, IF_CS_BIT_MASK);
@@ -773,18 +773,14 @@ static void if_cs_prog_firmware(struct lbs_private *priv, int ret,
 
 	/* And finally bring the card up */
 	priv->fw_ready = 1;
 	if (lbs_start_card(priv) != 0) {
 		pr_err("could not activate card\n");
 		free_irq(card->p_dev->irq, card);
 	}
-
-out:
-	release_firmware(helper);
-	release_firmware(mainfw);
 }
 
 
 /********************************************************************/
 /* Callback functions for libertas.ko                               */
 /********************************************************************/
 
diff --git a/drivers/net/wireless/libertas/if_sdio.c b/drivers/net/wireless/libertas/if_sdio.c
index 4557833..991238a 100644
--- a/drivers/net/wireless/libertas/if_sdio.c
+++ b/drivers/net/wireless/libertas/if_sdio.c
@@ -704,28 +704,24 @@ static void if_sdio_do_prog_firmware(struct lbs_private *priv, int ret,
 	if (ret) {
 		pr_err("failed to find firmware (%d)\n", ret);
 		return;
 	}
 
 	ret = if_sdio_prog_helper(card, helper);
 	if (ret)
-		goto out;
+		return;
 
 	lbs_deb_sdio("Helper firmware loaded\n");
 
 	ret = if_sdio_prog_real(card, mainfw);
 	if (ret)
-		goto out;
+		return;
 
 	lbs_deb_sdio("Firmware loaded\n");
 	if_sdio_finish_power_on(card);
-
-out:
-	release_firmware(helper);
-	release_firmware(mainfw);
 }
 
 static int if_sdio_prog_firmware(struct if_sdio_card *card)
 {
 	int ret;
 	u16 scratch;
 
diff --git a/drivers/net/wireless/libertas/if_spi.c b/drivers/net/wireless/libertas/if_spi.c
index 4bb6574..87ff0ca 100644
--- a/drivers/net/wireless/libertas/if_spi.c
+++ b/drivers/net/wireless/libertas/if_spi.c
@@ -1090,19 +1090,15 @@ static int if_spi_init_card(struct if_spi_card *card)
 	}
 
 	err = spu_set_interrupt_mode(card, 0, 1);
 	if (err)
 		goto out;
 
 out:
-	release_firmware(helper);
-	release_firmware(mainfw);
-
 	lbs_deb_leave_args(LBS_DEB_SPI, "err %d\n", err);
-
 	return err;
 }
 
 static void if_spi_resume_worker(struct work_struct *work)
 {
 	struct if_spi_card *card;
 
diff --git a/drivers/net/wireless/libertas/if_usb.c b/drivers/net/wireless/libertas/if_usb.c
index 2798077..dff08a2 100644
--- a/drivers/net/wireless/libertas/if_usb.c
+++ b/drivers/net/wireless/libertas/if_usb.c
@@ -840,15 +840,15 @@ static void if_usb_prog_firmware(struct lbs_private *priv, int ret,
 		pr_err("failed to find firmware (%d)\n", ret);
 		goto done;
 	}
 
 	cardp->fw = fw;
 	if (check_fwfile_format(cardp->fw->data, cardp->fw->size)) {
 		ret = -EINVAL;
-		goto release_fw;
+		goto done;
 	}
 
 	/* Cancel any pending usb business */
 	usb_kill_urb(cardp->rx_urb);
 	usb_kill_urb(cardp->tx_urb);
 
 	cardp->fwlastblksent = 0;
@@ -857,15 +857,15 @@ static void if_usb_prog_firmware(struct lbs_private *priv, int ret,
 	cardp->fwfinalblk = 0;
 	cardp->bootcmdresp = 0;
 
 restart:
 	if (if_usb_submit_rx_urb_fwload(cardp) < 0) {
 		lbs_deb_usbd(&cardp->udev->dev, "URB submission is failed\n");
 		ret = -EIO;
-		goto release_fw;
+		goto done;
 	}
 
 	cardp->bootcmdresp = 0;
 	do {
 		int j = 0;
 		i++;
 		if_usb_issue_boot_command(cardp, BOOT_CMD_FW_BY_USB);
@@ -879,22 +879,22 @@ restart:
 	if (cardp->bootcmdresp == BOOT_CMD_RESP_NOT_SUPPORTED) {
 		/* Return to normal operation */
 		ret = -EOPNOTSUPP;
 		usb_kill_urb(cardp->rx_urb);
 		usb_kill_urb(cardp->tx_urb);
 		if (if_usb_submit_rx_urb(cardp) < 0)
 			ret = -EIO;
-		goto release_fw;
+		goto done;
 	} else if (cardp->bootcmdresp <= 0) {
 		if (--reset_count >= 0) {
 			if_usb_reset_device(cardp);
 			goto restart;
 		}
 		ret = -EIO;
-		goto release_fw;
+		goto done;
 	}
 
 	i = 0;
 
 	cardp->totalbytes = 0;
 	cardp->fwlastblksent = 0;
 	cardp->CRC_OK = 1;
@@ -917,37 +917,34 @@ restart:
 		if (--reset_count >= 0) {
 			if_usb_reset_device(cardp);
 			goto restart;
 		}
 
 		pr_info("FW download failure, time = %d ms\n", i * 100);
 		ret = -EIO;
-		goto release_fw;
+		goto done;
 	}
 
 	cardp->priv->fw_ready = 1;
 	if_usb_submit_rx_urb(cardp);
 
 	if (lbs_start_card(priv))
-		goto release_fw;
+		goto done;
 
 	if_usb_setup_firmware(priv);
 
 	/*
 	 * EHS_REMOVE_WAKEUP is not supported on all versions of the firmware.
 	 */
 	priv->wol_criteria = EHS_REMOVE_WAKEUP;
 	if (lbs_host_sleep_cfg(priv, priv->wol_criteria, NULL))
 		priv->ehs_remove_supported = false;
 
- release_fw:
-	release_firmware(cardp->fw);
-	cardp->fw = NULL;
-
  done:
+	cardp->fw = NULL;
 	lbs_deb_leave(LBS_DEB_USB);
 }
 
 
 #ifdef CONFIG_PM
 static int if_usb_suspend(struct usb_interface *intf, pm_message_t message)
 {



^ permalink raw reply related	[flat|nested] 4+ messages in thread

* Re: [Patch 1/1]: libertas/sdio: fix releasing memory twice.
  2013-10-12 19:58 ` Dan Williams
@ 2013-10-13  7:09   ` Dr. H. Nikolaus Schaller
  2013-10-14 22:51     ` [PATCH] libertas: move firmware lifetime handling to firmware.c Dan Williams
  0 siblings, 1 reply; 4+ messages in thread
From: Dr. H. Nikolaus Schaller @ 2013-10-13  7:09 UTC (permalink / raw)
  To: Dan Williams
  Cc: John W. Linville, Bing Zhao, Harro Haan, libertas-dev,
	linux-wireless, netdev, LKML, Belisko Marek, NeilBrown Brown

Hi Dan,

Am 12.10.2013 um 21:58 schrieb Dan Williams:

> On Sat, 2013-10-12 at 18:02 +0200, Dr. H. Nikolaus Schaller wrote:
>> While upgrading the GTA04 kernel to 3.12-rc4 we came across
>> an issue with libertas/sdio referencing stale memory on ifconfig up
>> when trying to load the firmware (for a second time).
>> 
>> I am not at all sure if the patch is how it should be done and the right
>> location, but it appears to work for us with resetting priv->helper_fw to NULL
>> before asynchronously loading the firmware again.
> 
> How about we go even simpler?  helper_fw is *only* used inside
> firmware.c anyway, and that's probably where its lifetime should be
> handled.  Same for the main firmware.  I have no idea why the bus code
> is responsible for releasing the firmware anyway, when it originates
> from firmware.c and control returns back there after the firmware loaded
> callback is done.  Does the following patch fix your problem too?

Yes, it works!

I think your approach is much better from software architecture point
of view than our hack.

Thank you,
Nikolaus

> 
> Dan
> 
> ---
> diff --git a/drivers/net/wireless/libertas/firmware.c b/drivers/net/wireless/libertas/firmware.c
> index c0f9e7e..51b92b5 100644
> --- a/drivers/net/wireless/libertas/firmware.c
> +++ b/drivers/net/wireless/libertas/firmware.c
> @@ -49,14 +49,19 @@ static void main_firmware_cb(const struct firmware *firmware, void *context)
> 		/* Failed to find firmware: try next table entry */
> 		load_next_firmware_from_table(priv);
> 		return;
> 	}
> 
> 	/* Firmware found! */
> 	lbs_fw_loaded(priv, 0, priv->helper_fw, firmware);
> +	if (priv->helper_fw) {
> +		release_firmware (priv->helper_fw);
> +		priv->helper_fw = NULL;
> +	}
> +	release_firmware (firmware);
> }
> 
> static void helper_firmware_cb(const struct firmware *firmware, void *context)
> {
> 	struct lbs_private *priv = context;
> 
> 	if (!firmware) {
> diff --git a/drivers/net/wireless/libertas/if_cs.c b/drivers/net/wireless/libertas/if_cs.c
> index c94dd68..ef8c98e 100644
> --- a/drivers/net/wireless/libertas/if_cs.c
> +++ b/drivers/net/wireless/libertas/if_cs.c
> @@ -750,22 +750,22 @@ static void if_cs_prog_firmware(struct lbs_private *priv, int ret,
> 	}
> 
> 	/* Load the firmware */
> 	ret = if_cs_prog_helper(card, helper);
> 	if (ret == 0 && (card->model != MODEL_8305))
> 		ret = if_cs_prog_real(card, mainfw);
> 	if (ret)
> -		goto out;
> +		return;
> 
> 	/* Now actually get the IRQ */
> 	ret = request_irq(card->p_dev->irq, if_cs_interrupt,
> 		IRQF_SHARED, DRV_NAME, card);
> 	if (ret) {
> 		pr_err("error in request_irq\n");
> -		goto out;
> +		return;
> 	}
> 
> 	/*
> 	 * Clear any interrupt cause that happened while sending
> 	 * firmware/initializing card
> 	 */
> 	if_cs_write16(card, IF_CS_CARD_INT_CAUSE, IF_CS_BIT_MASK);
> @@ -773,18 +773,14 @@ static void if_cs_prog_firmware(struct lbs_private *priv, int ret,
> 
> 	/* And finally bring the card up */
> 	priv->fw_ready = 1;
> 	if (lbs_start_card(priv) != 0) {
> 		pr_err("could not activate card\n");
> 		free_irq(card->p_dev->irq, card);
> 	}
> -
> -out:
> -	release_firmware(helper);
> -	release_firmware(mainfw);
> }
> 
> 
> /********************************************************************/
> /* Callback functions for libertas.ko                               */
> /********************************************************************/
> 
> diff --git a/drivers/net/wireless/libertas/if_sdio.c b/drivers/net/wireless/libertas/if_sdio.c
> index 4557833..991238a 100644
> --- a/drivers/net/wireless/libertas/if_sdio.c
> +++ b/drivers/net/wireless/libertas/if_sdio.c
> @@ -704,28 +704,24 @@ static void if_sdio_do_prog_firmware(struct lbs_private *priv, int ret,
> 	if (ret) {
> 		pr_err("failed to find firmware (%d)\n", ret);
> 		return;
> 	}
> 
> 	ret = if_sdio_prog_helper(card, helper);
> 	if (ret)
> -		goto out;
> +		return;
> 
> 	lbs_deb_sdio("Helper firmware loaded\n");
> 
> 	ret = if_sdio_prog_real(card, mainfw);
> 	if (ret)
> -		goto out;
> +		return;
> 
> 	lbs_deb_sdio("Firmware loaded\n");
> 	if_sdio_finish_power_on(card);
> -
> -out:
> -	release_firmware(helper);
> -	release_firmware(mainfw);
> }
> 
> static int if_sdio_prog_firmware(struct if_sdio_card *card)
> {
> 	int ret;
> 	u16 scratch;
> 
> diff --git a/drivers/net/wireless/libertas/if_spi.c b/drivers/net/wireless/libertas/if_spi.c
> index 4bb6574..87ff0ca 100644
> --- a/drivers/net/wireless/libertas/if_spi.c
> +++ b/drivers/net/wireless/libertas/if_spi.c
> @@ -1090,19 +1090,15 @@ static int if_spi_init_card(struct if_spi_card *card)
> 	}
> 
> 	err = spu_set_interrupt_mode(card, 0, 1);
> 	if (err)
> 		goto out;
> 
> out:
> -	release_firmware(helper);
> -	release_firmware(mainfw);
> -
> 	lbs_deb_leave_args(LBS_DEB_SPI, "err %d\n", err);
> -
> 	return err;
> }
> 
> static void if_spi_resume_worker(struct work_struct *work)
> {
> 	struct if_spi_card *card;
> 
> diff --git a/drivers/net/wireless/libertas/if_usb.c b/drivers/net/wireless/libertas/if_usb.c
> index 2798077..dff08a2 100644
> --- a/drivers/net/wireless/libertas/if_usb.c
> +++ b/drivers/net/wireless/libertas/if_usb.c
> @@ -840,15 +840,15 @@ static void if_usb_prog_firmware(struct lbs_private *priv, int ret,
> 		pr_err("failed to find firmware (%d)\n", ret);
> 		goto done;
> 	}
> 
> 	cardp->fw = fw;
> 	if (check_fwfile_format(cardp->fw->data, cardp->fw->size)) {
> 		ret = -EINVAL;
> -		goto release_fw;
> +		goto done;
> 	}
> 
> 	/* Cancel any pending usb business */
> 	usb_kill_urb(cardp->rx_urb);
> 	usb_kill_urb(cardp->tx_urb);
> 
> 	cardp->fwlastblksent = 0;
> @@ -857,15 +857,15 @@ static void if_usb_prog_firmware(struct lbs_private *priv, int ret,
> 	cardp->fwfinalblk = 0;
> 	cardp->bootcmdresp = 0;
> 
> restart:
> 	if (if_usb_submit_rx_urb_fwload(cardp) < 0) {
> 		lbs_deb_usbd(&cardp->udev->dev, "URB submission is failed\n");
> 		ret = -EIO;
> -		goto release_fw;
> +		goto done;
> 	}
> 
> 	cardp->bootcmdresp = 0;
> 	do {
> 		int j = 0;
> 		i++;
> 		if_usb_issue_boot_command(cardp, BOOT_CMD_FW_BY_USB);
> @@ -879,22 +879,22 @@ restart:
> 	if (cardp->bootcmdresp == BOOT_CMD_RESP_NOT_SUPPORTED) {
> 		/* Return to normal operation */
> 		ret = -EOPNOTSUPP;
> 		usb_kill_urb(cardp->rx_urb);
> 		usb_kill_urb(cardp->tx_urb);
> 		if (if_usb_submit_rx_urb(cardp) < 0)
> 			ret = -EIO;
> -		goto release_fw;
> +		goto done;
> 	} else if (cardp->bootcmdresp <= 0) {
> 		if (--reset_count >= 0) {
> 			if_usb_reset_device(cardp);
> 			goto restart;
> 		}
> 		ret = -EIO;
> -		goto release_fw;
> +		goto done;
> 	}
> 
> 	i = 0;
> 
> 	cardp->totalbytes = 0;
> 	cardp->fwlastblksent = 0;
> 	cardp->CRC_OK = 1;
> @@ -917,37 +917,34 @@ restart:
> 		if (--reset_count >= 0) {
> 			if_usb_reset_device(cardp);
> 			goto restart;
> 		}
> 
> 		pr_info("FW download failure, time = %d ms\n", i * 100);
> 		ret = -EIO;
> -		goto release_fw;
> +		goto done;
> 	}
> 
> 	cardp->priv->fw_ready = 1;
> 	if_usb_submit_rx_urb(cardp);
> 
> 	if (lbs_start_card(priv))
> -		goto release_fw;
> +		goto done;
> 
> 	if_usb_setup_firmware(priv);
> 
> 	/*
> 	 * EHS_REMOVE_WAKEUP is not supported on all versions of the firmware.
> 	 */
> 	priv->wol_criteria = EHS_REMOVE_WAKEUP;
> 	if (lbs_host_sleep_cfg(priv, priv->wol_criteria, NULL))
> 		priv->ehs_remove_supported = false;
> 
> - release_fw:
> -	release_firmware(cardp->fw);
> -	cardp->fw = NULL;
> -
>  done:
> +	cardp->fw = NULL;
> 	lbs_deb_leave(LBS_DEB_USB);
> }
> 
> 
> #ifdef CONFIG_PM
> static int if_usb_suspend(struct usb_interface *intf, pm_message_t message)
> {
> 
> 


^ permalink raw reply	[flat|nested] 4+ messages in thread

* [PATCH] libertas: move firmware lifetime handling to firmware.c
  2013-10-13  7:09   ` Dr. H. Nikolaus Schaller
@ 2013-10-14 22:51     ` Dan Williams
  0 siblings, 0 replies; 4+ messages in thread
From: Dan Williams @ 2013-10-14 22:51 UTC (permalink / raw)
  To: Dr. H. Nikolaus Schaller
  Cc: libertas-dev, NeilBrown Brown, netdev, linux-wireless,
	Harro Haan, John W. Linville, Belisko Marek, LKML

Previously, each bus type was responsible for freeing the firmware
structure, but some did that badly.  Move responsibility for freeing
firmware into firmware.c so that it's done once and correctly, instead
of happening in multiple places in bus-specific code.

This fixes a use-after-free bug found by Dr. H. Nikolaus Schaller where
the SDIO code forgot to NULL priv->helper_fw after freeing it.

Signed-off-by: Dan Williams <dcbw@redhat.com>
---
Tested firmware loading on USB (8388), CS (8385), and SDIO (8686).

diff --git a/drivers/net/wireless/libertas/firmware.c b/drivers/net/wireless/libertas/firmware.c
index c0f9e7e..51b92b5 100644
--- a/drivers/net/wireless/libertas/firmware.c
+++ b/drivers/net/wireless/libertas/firmware.c
@@ -49,14 +49,19 @@ static void main_firmware_cb(const struct firmware *firmware, void *context)
 		/* Failed to find firmware: try next table entry */
 		load_next_firmware_from_table(priv);
 		return;
 	}
 
 	/* Firmware found! */
 	lbs_fw_loaded(priv, 0, priv->helper_fw, firmware);
+	if (priv->helper_fw) {
+		release_firmware (priv->helper_fw);
+		priv->helper_fw = NULL;
+	}
+	release_firmware (firmware);
 }
 
 static void helper_firmware_cb(const struct firmware *firmware, void *context)
 {
 	struct lbs_private *priv = context;
 
 	if (!firmware) {
diff --git a/drivers/net/wireless/libertas/if_cs.c b/drivers/net/wireless/libertas/if_cs.c
index c94dd68..ef8c98e 100644
--- a/drivers/net/wireless/libertas/if_cs.c
+++ b/drivers/net/wireless/libertas/if_cs.c
@@ -750,22 +750,22 @@ static void if_cs_prog_firmware(struct lbs_private *priv, int ret,
 	}
 
 	/* Load the firmware */
 	ret = if_cs_prog_helper(card, helper);
 	if (ret == 0 && (card->model != MODEL_8305))
 		ret = if_cs_prog_real(card, mainfw);
 	if (ret)
-		goto out;
+		return;
 
 	/* Now actually get the IRQ */
 	ret = request_irq(card->p_dev->irq, if_cs_interrupt,
 		IRQF_SHARED, DRV_NAME, card);
 	if (ret) {
 		pr_err("error in request_irq\n");
-		goto out;
+		return;
 	}
 
 	/*
 	 * Clear any interrupt cause that happened while sending
 	 * firmware/initializing card
 	 */
 	if_cs_write16(card, IF_CS_CARD_INT_CAUSE, IF_CS_BIT_MASK);
@@ -773,18 +773,14 @@ static void if_cs_prog_firmware(struct lbs_private *priv, int ret,
 
 	/* And finally bring the card up */
 	priv->fw_ready = 1;
 	if (lbs_start_card(priv) != 0) {
 		pr_err("could not activate card\n");
 		free_irq(card->p_dev->irq, card);
 	}
-
-out:
-	release_firmware(helper);
-	release_firmware(mainfw);
 }
 
 
 /********************************************************************/
 /* Callback functions for libertas.ko                               */
 /********************************************************************/
 
diff --git a/drivers/net/wireless/libertas/if_sdio.c b/drivers/net/wireless/libertas/if_sdio.c
index 4557833..991238a 100644
--- a/drivers/net/wireless/libertas/if_sdio.c
+++ b/drivers/net/wireless/libertas/if_sdio.c
@@ -704,28 +704,24 @@ static void if_sdio_do_prog_firmware(struct lbs_private *priv, int ret,
 	if (ret) {
 		pr_err("failed to find firmware (%d)\n", ret);
 		return;
 	}
 
 	ret = if_sdio_prog_helper(card, helper);
 	if (ret)
-		goto out;
+		return;
 
 	lbs_deb_sdio("Helper firmware loaded\n");
 
 	ret = if_sdio_prog_real(card, mainfw);
 	if (ret)
-		goto out;
+		return;
 
 	lbs_deb_sdio("Firmware loaded\n");
 	if_sdio_finish_power_on(card);
-
-out:
-	release_firmware(helper);
-	release_firmware(mainfw);
 }
 
 static int if_sdio_prog_firmware(struct if_sdio_card *card)
 {
 	int ret;
 	u16 scratch;
 
diff --git a/drivers/net/wireless/libertas/if_spi.c b/drivers/net/wireless/libertas/if_spi.c
index 4bb6574..87ff0ca 100644
--- a/drivers/net/wireless/libertas/if_spi.c
+++ b/drivers/net/wireless/libertas/if_spi.c
@@ -1090,19 +1090,15 @@ static int if_spi_init_card(struct if_spi_card *card)
 	}
 
 	err = spu_set_interrupt_mode(card, 0, 1);
 	if (err)
 		goto out;
 
 out:
-	release_firmware(helper);
-	release_firmware(mainfw);
-
 	lbs_deb_leave_args(LBS_DEB_SPI, "err %d\n", err);
-
 	return err;
 }
 
 static void if_spi_resume_worker(struct work_struct *work)
 {
 	struct if_spi_card *card;
 
diff --git a/drivers/net/wireless/libertas/if_usb.c b/drivers/net/wireless/libertas/if_usb.c
index 2798077..dff08a2 100644
--- a/drivers/net/wireless/libertas/if_usb.c
+++ b/drivers/net/wireless/libertas/if_usb.c
@@ -840,15 +840,15 @@ static void if_usb_prog_firmware(struct lbs_private *priv, int ret,
 		pr_err("failed to find firmware (%d)\n", ret);
 		goto done;
 	}
 
 	cardp->fw = fw;
 	if (check_fwfile_format(cardp->fw->data, cardp->fw->size)) {
 		ret = -EINVAL;
-		goto release_fw;
+		goto done;
 	}
 
 	/* Cancel any pending usb business */
 	usb_kill_urb(cardp->rx_urb);
 	usb_kill_urb(cardp->tx_urb);
 
 	cardp->fwlastblksent = 0;
@@ -857,15 +857,15 @@ static void if_usb_prog_firmware(struct lbs_private *priv, int ret,
 	cardp->fwfinalblk = 0;
 	cardp->bootcmdresp = 0;
 
 restart:
 	if (if_usb_submit_rx_urb_fwload(cardp) < 0) {
 		lbs_deb_usbd(&cardp->udev->dev, "URB submission is failed\n");
 		ret = -EIO;
-		goto release_fw;
+		goto done;
 	}
 
 	cardp->bootcmdresp = 0;
 	do {
 		int j = 0;
 		i++;
 		if_usb_issue_boot_command(cardp, BOOT_CMD_FW_BY_USB);
@@ -879,22 +879,22 @@ restart:
 	if (cardp->bootcmdresp == BOOT_CMD_RESP_NOT_SUPPORTED) {
 		/* Return to normal operation */
 		ret = -EOPNOTSUPP;
 		usb_kill_urb(cardp->rx_urb);
 		usb_kill_urb(cardp->tx_urb);
 		if (if_usb_submit_rx_urb(cardp) < 0)
 			ret = -EIO;
-		goto release_fw;
+		goto done;
 	} else if (cardp->bootcmdresp <= 0) {
 		if (--reset_count >= 0) {
 			if_usb_reset_device(cardp);
 			goto restart;
 		}
 		ret = -EIO;
-		goto release_fw;
+		goto done;
 	}
 
 	i = 0;
 
 	cardp->totalbytes = 0;
 	cardp->fwlastblksent = 0;
 	cardp->CRC_OK = 1;
@@ -917,37 +917,34 @@ restart:
 		if (--reset_count >= 0) {
 			if_usb_reset_device(cardp);
 			goto restart;
 		}
 
 		pr_info("FW download failure, time = %d ms\n", i * 100);
 		ret = -EIO;
-		goto release_fw;
+		goto done;
 	}
 
 	cardp->priv->fw_ready = 1;
 	if_usb_submit_rx_urb(cardp);
 
 	if (lbs_start_card(priv))
-		goto release_fw;
+		goto done;
 
 	if_usb_setup_firmware(priv);
 
 	/*
 	 * EHS_REMOVE_WAKEUP is not supported on all versions of the firmware.
 	 */
 	priv->wol_criteria = EHS_REMOVE_WAKEUP;
 	if (lbs_host_sleep_cfg(priv, priv->wol_criteria, NULL))
 		priv->ehs_remove_supported = false;
 
- release_fw:
-	release_firmware(cardp->fw);
-	cardp->fw = NULL;
-
  done:
+	cardp->fw = NULL;
 	lbs_deb_leave(LBS_DEB_USB);
 }
 
 
 #ifdef CONFIG_PM
 static int if_usb_suspend(struct usb_interface *intf, pm_message_t message)
 {



^ permalink raw reply related	[flat|nested] 4+ messages in thread

end of thread, other threads:[~2013-10-14 22:46 UTC | newest]

Thread overview: 4+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2013-10-12 16:02 [Patch 1/1]: libertas/sdio: fix releasing memory twice Dr. H. Nikolaus Schaller
2013-10-12 19:58 ` Dan Williams
2013-10-13  7:09   ` Dr. H. Nikolaus Schaller
2013-10-14 22:51     ` [PATCH] libertas: move firmware lifetime handling to firmware.c Dan Williams

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).