netdev - Re: [RESEND][PATCH] Bluetooth: Make request workqueue freezable

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives
Hash Suite: Windows password security audit tool. GUI, reports in PDF.
[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]
Message-ID: <s5hbnhfifka.wl-tiwai@suse.de>
Date:	Wed, 20 May 2015 14:44:37 +0200
From:	Takashi Iwai <tiwai@...e.de>
To:	Marcel Holtmann <marcel@...tmann.org>
Cc:	Oliver Neukum <oneukum@...e.com>,
	Ming Lei <ming.lei@...onical.com>,
	"David S. Miller" <davem@...emloft.net>,
	Laura Abbott <labbott@...oraproject.org>,
	Johan Hedberg <johan.hedberg@...il.com>,
	"Rafael J. Wysocki" <rafael.j.wysocki@...el.com>,
	"Gustavo F. Padovan" <gustavo@...ovan.org>,
	Laura Abbott <labbott@...hat.com>,
	Alan Stern <stern@...land.harvard.edu>,
	"bluez mailin list (linux-bluetooth@...r.kernel.org)" 
	<linux-bluetooth@...r.kernel.org>,
	Linux Kernel Mailing List <linux-kernel@...r.kernel.org>,
	USB list <linux-usb@...r.kernel.org>,
	netdev <netdev@...r.kernel.org>
Subject: Re: [RESEND][PATCH] Bluetooth: Make request workqueue freezable

At Wed, 20 May 2015 11:46:31 +0200,
Marcel Holtmann wrote:
> 
> Hi Oliver,
> 
> >> The data is cached in RAM.  More specifically, the former loaded
> >> firmware files are reloaded and saved at suspend for each device
> >> object.  See fw_pm_notify() in firmware_class.c.
> > 
> > OK, this may be a stupid idea, but do we know the firmware
> > was successfully loaded in the first place?
> > Also btusb is in the habit of falling back to a generic
> > firmware in some places. It seems to me that caching
> > firmware is conceptually not enough, but we'd also need
> > to record the absence of firmware images.
> 
> in a lot of cases the firmware is optional. The device will operate fine without the firmware. There are a few devices where the firmware is required, but for many it just contains patches.
> 
> It would be nice if we could tell request_firmware() if it is optional or mandatory firmware. Or if it should just cache the status of a missing firmware as well.

OK, below is a quick hack to record the failed f/w files, too.
Not sure whether this helps, though.  Proper tests are appreciated.


Takashi

---
From: Takashi Iwai <tiwai@...e.de>
Subject: [PATCH] firmware: cache failed firmwares, too

Signed-off-by: Takashi Iwai <tiwai@...e.de>
---
 drivers/base/firmware_class.c | 33 ++++++++++++---------------------
 1 file changed, 12 insertions(+), 21 deletions(-)

diff --git a/drivers/base/firmware_class.c b/drivers/base/firmware_class.c
index 171841ad1008..a15af7289c94 100644
--- a/drivers/base/firmware_class.c
+++ b/drivers/base/firmware_class.c
@@ -1035,6 +1035,8 @@ _request_firmware_prepare(struct firmware **firmware_p, const char *name,
 	firmware->priv = buf;
 
 	if (ret > 0) {
+		if (buf->size == -1UL)
+			return -ENOENT; /* already recorded as failure */
 		ret = sync_cached_firmware_buf(buf);
 		if (!ret) {
 			fw_set_page_data(buf, firmware);
@@ -1047,17 +1049,12 @@ _request_firmware_prepare(struct firmware **firmware_p, const char *name,
 	return 1; /* need to load */
 }
 
-static int assign_firmware_buf(struct firmware *fw, struct device *device,
+static void assign_firmware_buf(struct firmware *fw, struct device *device,
 			       unsigned int opt_flags)
 {
 	struct firmware_buf *buf = fw->priv;
 
 	mutex_lock(&fw_lock);
-	if (!buf->size || is_fw_load_aborted(buf)) {
-		mutex_unlock(&fw_lock);
-		return -ENOENT;
-	}
-
 	/*
 	 * add firmware name into devres list so that we can auto cache
 	 * and uncache firmware for device.
@@ -1079,9 +1076,9 @@ static int assign_firmware_buf(struct firmware *fw, struct device *device,
 	}
 
 	/* pass the pages buffer to driver at the last minute */
-	fw_set_page_data(buf, fw);
+	if (buf->size != -1UL)
+		fw_set_page_data(buf, fw);
 	mutex_unlock(&fw_lock);
-	return 0;
 }
 
 /* called from request_firmware() and request_firmware_work_func() */
@@ -1124,6 +1121,9 @@ _request_firmware(const struct firmware **firmware_p, const char *name,
 
 	ret = fw_get_filesystem_firmware(device, fw->priv);
 	if (ret) {
+		struct firmware_buf *buf = fw->priv;
+
+		buf->size = -1UL; /* failed */
 		if (!(opt_flags & FW_OPT_NO_WARN))
 			dev_warn(device,
 				 "Direct firmware load for %s failed with error %d\n",
@@ -1132,12 +1132,12 @@ _request_firmware(const struct firmware **firmware_p, const char *name,
 			dev_warn(device, "Falling back to user helper\n");
 			ret = fw_load_from_user_helper(fw, name, device,
 						       opt_flags, timeout);
+			if (ret)
+				buf->size = -1UL; /* failed */
 		}
 	}
 
-	if (!ret)
-		ret = assign_firmware_buf(fw, device, opt_flags);
-
+	assign_firmware_buf(fw, device, opt_flags);
 	usermodehelper_read_unlock();
 
  out:
@@ -1435,17 +1435,8 @@ static void __async_dev_cache_fw_image(void *fw_entry,
 				       async_cookie_t cookie)
 {
 	struct fw_cache_entry *fce = fw_entry;
-	struct firmware_cache *fwc = &fw_cache;
-	int ret;
-
-	ret = cache_firmware(fce->name);
-	if (ret) {
-		spin_lock(&fwc->name_lock);
-		list_del(&fce->list);
-		spin_unlock(&fwc->name_lock);
 
-		free_fw_cache_entry(fce);
-	}
+	cache_firmware(fce->name);
 }
 
 /* called with dev->devres_lock held */
-- 
2.4.1

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