diff mbox

[1/2] firmware: add more flexible request_firmware_async function

Message ID 20170215222948.21030-1-zajec5@gmail.com (mailing list archive)
State Changes Requested
Delegated to: Kalle Valo
Headers show

Commit Message

Rafał Miłecki Feb. 15, 2017, 10:29 p.m. UTC 
From: Rafał Miłecki <rafal@milecki.pl>

So far we got only one function for loading firmware asynchronously:
request_firmware_nowait. It didn't allow much customization of firmware
loading process - there is only one bool uevent argument. Moreover this
bool also controls user helper in an unclear way.

Resolve this problem by adding a one flexible function and making old
request_firmware_nowait a simple inline using new solution. This
implementation:
1) Modifies only single bits on existing code
2) Doesn't require adjusting / rewriting current drivers
3) Adds new function for drivers that need more control over loading a
   firmware. Thanks to using flags more features can be added later.

Signed-off-by: Rafał Miłecki <rafal@milecki.pl>
---
This patch is based on top of
[PATCH V2 RESEND] firmware: simplify defining and handling FW_OPT_FALLBACK
applied on top of Linux 4.10-rc8.

Ming/Luis/Greg: assuming this gets a positive review, could someone of you pick
this patchset? Second patch modifies brcmfmac, I'll try to get a proper Ack for
that one.
Unless you want this to go through wireless tree, then let me know please.
---
 drivers/base/firmware_class.c | 25 ++++++-------------------
 include/linux/firmware.h      | 34 +++++++++++++++++++++++++++++-----
 2 files changed, 35 insertions(+), 24 deletions(-)

Comments

Luis Chamberlain Feb. 21, 2017, 5:59 p.m. UTC | #1 
On Wed, Feb 15, 2017 at 11:29:47PM +0100, Rafał Miłecki wrote:
> From: Rafał Miłecki <rafal@milecki.pl>
> 
> So far we got only one function for loading firmware asynchronously:
> request_firmware_nowait. It didn't allow much customization of firmware
> loading process - there is only one bool uevent argument. Moreover this
> bool also controls user helper in an unclear way.
> 
> Resolve this problem by adding a one flexible function 

You've just taken under-the-hood flags and exposed them without considering and
enabling easy testing of any possible conflicts here.  Take for instance the
FW_OPT_NOCACHE feature -- not using caching functionality has implications for
driver developers -- they *must* resolve their own suspend/resume mishaps.
Another example, when we get firmware signing having just flags won't cut it
for optional parameters, we want a struct with the ability to specify custom
signing requirements. Exposing just flags will not cut it for us long term and
we'd have to then either add new export symbol or modify this one.

This is not as flexible nor as well documented as I want for future
functionality. Nor is there any series of battery of tests of all possible
options to ensure we do not regress. I've addressed this in the driver data
series.

> and making old
> request_firmware_nowait a simple inline using new solution. 

This I had not addressed in the driver data series but I will fold similar
strategy onto it.

> This
> implementation:
> 1) Modifies only single bits on existing code
> 2) Doesn't require adjusting / rewriting current drivers
> 3) Adds new function for drivers that need more control over loading a
>    firmware. Thanks to using flags more features can be added later.

As I noted in the driver data series -- we're passed using the firmware API for
non-firmware specific stuff, and as recent history shows regressions are are
easy, specially with the fallback mechanism. A new API which just gives us
2 calls: sync/async calls and shares a common set of optional parameters is
what we need, we need proper testing to ensure we also don't regress should
new features be introduced.

What I'll do is I'll integrate the feature you are asking for here and fold
this into the driver data series as what we need now is actual users of new
functionality, not just a test driver.

  Luis
diff mbox

Patch

diff --git a/drivers/base/firmware_class.c b/drivers/base/firmware_class.c
index d05be1732c8b..7b3f0a018dc3 100644
--- a/drivers/base/firmware_class.c
+++ b/drivers/base/firmware_class.c
@@ -182,18 +182,6 @@  static int __fw_state_check(struct fw_state *fw_st, enum fw_status status)
 
 #endif /* CONFIG_FW_LOADER_USER_HELPER */
 
-/* firmware behavior options */
-#define FW_OPT_UEVENT	(1U << 0)
-#define FW_OPT_NOWAIT	(1U << 1)
-#ifdef CONFIG_FW_LOADER_USER_HELPER
-#define FW_OPT_USERHELPER	(1U << 2)
-#else
-#define FW_OPT_USERHELPER	0
-#endif
-#define FW_OPT_NO_WARN	(1U << 3)
-#define FW_OPT_NOCACHE	(1U << 4)
-#define FW_OPT_FALLBACK	(1U << 5)
-
 struct firmware_cache {
 	/* firmware_buf instance will be added into the below list */
 	spinlock_t lock;
@@ -1356,7 +1344,7 @@  static void request_firmware_work_func(struct work_struct *work)
 	_request_firmware(&fw, fw_work->name, fw_work->device, NULL, 0,
 			  fw_work->opt_flags);
 	fw_work->cont(fw, fw_work->context);
-	put_device(fw_work->device); /* taken in request_firmware_nowait() */
+	put_device(fw_work->device); /* taken in request_firmware_async() */
 
 	module_put(fw_work->module);
 	kfree_const(fw_work->name);
@@ -1364,7 +1352,7 @@  static void request_firmware_work_func(struct work_struct *work)
 }
 
 /**
- * request_firmware_nowait - asynchronous version of request_firmware
+ * request_firmware_async - asynchronous version of request_firmware
  * @module: module requesting the firmware
  * @uevent: sends uevent to copy the firmware image if this flag
  *	is non-zero else the firmware copy must be done manually.
@@ -1387,8 +1375,8 @@  static void request_firmware_work_func(struct work_struct *work)
  *		- can't sleep at all if @gfp is GFP_ATOMIC.
  **/
 int
-request_firmware_nowait(
-	struct module *module, bool uevent,
+request_firmware_async(
+	struct module *module, unsigned int opt_flags,
 	const char *name, struct device *device, gfp_t gfp, void *context,
 	void (*cont)(const struct firmware *fw, void *context))
 {
@@ -1407,8 +1395,7 @@  request_firmware_nowait(
 	fw_work->device = device;
 	fw_work->context = context;
 	fw_work->cont = cont;
-	fw_work->opt_flags = FW_OPT_NOWAIT | FW_OPT_FALLBACK |
-		(uevent ? FW_OPT_UEVENT : FW_OPT_USERHELPER);
+	fw_work->opt_flags = FW_OPT_NOWAIT | opt_flags;
 
 	if (!try_module_get(module)) {
 		kfree_const(fw_work->name);
@@ -1421,7 +1408,7 @@  request_firmware_nowait(
 	schedule_work(&fw_work->work);
 	return 0;
 }
-EXPORT_SYMBOL(request_firmware_nowait);
+EXPORT_SYMBOL(request_firmware_async);
 
 #ifdef CONFIG_PM_SLEEP
 static ASYNC_DOMAIN_EXCLUSIVE(fw_cache_domain);
diff --git a/include/linux/firmware.h b/include/linux/firmware.h
index b1f9f0ccb8ac..1f2bf14aa441 100644
--- a/include/linux/firmware.h
+++ b/include/linux/firmware.h
@@ -8,6 +8,18 @@ 
 #define FW_ACTION_NOHOTPLUG 0
 #define FW_ACTION_HOTPLUG 1
 
+/* firmware behavior options */
+#define FW_OPT_UEVENT	(1U << 0)
+#define FW_OPT_NOWAIT	(1U << 1)
+#ifdef CONFIG_FW_LOADER_USER_HELPER
+#define FW_OPT_USERHELPER	(1U << 2)
+#else
+#define FW_OPT_USERHELPER	0
+#endif
+#define FW_OPT_NO_WARN	(1U << 3)
+#define FW_OPT_NOCACHE	(1U << 4)
+#define FW_OPT_FALLBACK	(1U << 5)
+
 struct firmware {
 	size_t size;
 	const u8 *data;
@@ -41,8 +53,8 @@  struct builtin_fw {
 #if defined(CONFIG_FW_LOADER) || (defined(CONFIG_FW_LOADER_MODULE) && defined(MODULE))
 int request_firmware(const struct firmware **fw, const char *name,
 		     struct device *device);
-int request_firmware_nowait(
-	struct module *module, bool uevent,
+int request_firmware_async(
+	struct module *module, unsigned int opt_flags,
 	const char *name, struct device *device, gfp_t gfp, void *context,
 	void (*cont)(const struct firmware *fw, void *context));
 int request_firmware_direct(const struct firmware **fw, const char *name,
@@ -58,8 +70,8 @@  static inline int request_firmware(const struct firmware **fw,
 {
 	return -EINVAL;
 }
-static inline int request_firmware_nowait(
-	struct module *module, bool uevent,
+static inline int request_firmware_async(
+	struct module *module, unsigned int opt_flags,
 	const char *name, struct device *device, gfp_t gfp, void *context,
 	void (*cont)(const struct firmware *fw, void *context))
 {
@@ -82,6 +94,18 @@  static inline int request_firmware_into_buf(const struct firmware **firmware_p,
 {
 	return -EINVAL;
 }
-
 #endif
+
+static inline int request_firmware_nowait(
+	struct module *module, bool uevent,
+	const char *name, struct device *device, gfp_t gfp, void *context,
+	void (*cont)(const struct firmware *fw, void *context))
+{
+	unsigned int opt_flags = FW_OPT_FALLBACK |
+		(uevent ? FW_OPT_UEVENT : FW_OPT_USERHELPER);
+
+	return request_firmware_async(module, opt_flags, name, device, gfp,
+				      context, cont);
+}
+
 #endif