From patchwork Sat Oct 12 19:58:31 2013 Content-Type: text/plain; charset="utf-8" MIME-Version: 1.0 Content-Transfer-Encoding: 7bit X-Patchwork-Submitter: Dan Williams X-Patchwork-Id: 3032111 Return-Path: X-Original-To: patchwork-linux-wireless@patchwork.kernel.org Delivered-To: patchwork-parsemail@patchwork1.web.kernel.org Received: from mail.kernel.org (mail.kernel.org [198.145.19.201]) by patchwork1.web.kernel.org (Postfix) with ESMTP id 6CCAF9F243 for ; Sat, 12 Oct 2013 19:54:01 +0000 (UTC) Received: from mail.kernel.org (localhost [127.0.0.1]) by mail.kernel.org (Postfix) with ESMTP id 57CE820374 for ; Sat, 12 Oct 2013 19:54:00 +0000 (UTC) Received: from vger.kernel.org (vger.kernel.org [209.132.180.67]) by mail.kernel.org (Postfix) with ESMTP id 553092025B for ; Sat, 12 Oct 2013 19:53:59 +0000 (UTC) Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1752769Ab3JLTxk (ORCPT ); Sat, 12 Oct 2013 15:53:40 -0400 Received: from mx1.redhat.com ([209.132.183.28]:34551 "EHLO mx1.redhat.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1752701Ab3JLTxj (ORCPT ); Sat, 12 Oct 2013 15:53:39 -0400 Received: from int-mx10.intmail.prod.int.phx2.redhat.com (int-mx10.intmail.prod.int.phx2.redhat.com [10.5.11.23]) by mx1.redhat.com (8.14.4/8.14.4) with ESMTP id r9CJqviY026738 (version=TLSv1/SSLv3 cipher=DHE-RSA-AES256-SHA bits=256 verify=OK); Sat, 12 Oct 2013 15:52:57 -0400 Received: from [10.3.239.218] (vpn-239-218.phx2.redhat.com [10.3.239.218]) by int-mx10.intmail.prod.int.phx2.redhat.com (8.14.4/8.14.4) with ESMTP id r9CJqspN010653; Sat, 12 Oct 2013 15:52:55 -0400 Message-ID: <1381607911.21655.7.camel@dcbw.foobar.com> Subject: Re: [Patch 1/1]: libertas/sdio: fix releasing memory twice. From: Dan Williams To: "Dr. H. Nikolaus Schaller" Cc: "John W. Linville" , Bing Zhao , Harro Haan , libertas-dev@lists.infradead.org, linux-wireless@vger.kernel.org, netdev@vger.kernel.org, LKML , Belisko Marek , NeilBrown Brown Date: Sat, 12 Oct 2013 14:58:31 -0500 In-Reply-To: References: Mime-Version: 1.0 X-Scanned-By: MIMEDefang 2.68 on 10.5.11.23 Sender: linux-wireless-owner@vger.kernel.org Precedence: bulk List-ID: X-Mailing-List: linux-wireless@vger.kernel.org X-Spam-Status: No, score=-7.1 required=5.0 tests=BAYES_00, RCVD_IN_DNSWL_HI, RP_MATCHES_RCVD, UNPARSEABLE_RELAY autolearn=unavailable version=3.3.1 X-Spam-Checker-Version: SpamAssassin 3.3.1 (2010-03-16) on mail.kernel.org X-Virus-Scanned: ClamAV using ClamSMTP 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 --- -- 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 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) {