Date:   Thu, 14 Sep 2017 15:54:22 -0700
From:   "Luis R. Rodriguez" <mcgrof@...nel.org>
To:     gregkh@...uxfoundation.org
Cc:     wagi@...om.org, yi1.li@...ux.intel.com, takahiro.akashi@...aro.org,
        bjorn.andersson@...aro.org, luto@...nel.org, ebiederm@...ssion.com,
        dmitry.torokhov@...il.com, arend.vanspriel@...adcom.com,
        dwmw2@...radead.org, rjw@...ysocki.net, atull@...nel.org,
        moritz.fischer@...us.com, pmladek@...e.com,
        johannes.berg@...el.com, emmanuel.grumbach@...el.com,
        luciano.coelho@...el.com, kvalo@...eaurora.org,
        torvalds@...ux-foundation.org, keescook@...omium.org,
        dhowells@...hat.com, pjones@...hat.com, hdegoede@...hat.com,
        alan@...ux.intel.com, tytso@....edu, dave@...olabs.net,
        mawilcox@...rosoft.com, tglx@...utronix.de, peterz@...radead.org,
        mfuzzey@...keon.com, jakub.kicinski@...ronome.com,
        nbroeking@...com, jewalt@...innovations.com,
        linux-fsdevel@...r.kernel.org, linux-kernel@...r.kernel.org,
        "Luis R. Rodriguez" <mcgrof@...nel.org>
Subject: [PATCH] firmware: cleanup - group and document up private firmware parameters

The firmware API has a slew of private options available, which can
sometimes be hard to understand. When new functionality is introduced
we also tend to have modify a slew of internal helpers.

Just stuff all common private requirements into its own data structure
and move features into properly defined flags which can then be carefully
documented. This:

  o reduces the amount of changes we have make as we advance functionality
  o helps remove the #ifdef mess we had created for private features

The above benefits makes the code much easier to understand and maintain.

This cleanup introduces no functional changes. It has been tested using
the firmware selftests [0] [1] against the three main kernel configurations
which we care about:

0)
  CONFIG_FW_LOADER=y
1)
  o CONFIG_FW_LOADER=y
  o CONFIG_FW_LOADER_USER_HELPER=y
2)
  o CONFIG_FW_LOADER=y
  o CONFIG_FW_LOADER_USER_HELPER=y
  o CONFIG_FW_LOADER_USER_HELPER_FALLBACK=y

[0] tools/testing/selftests/firmware/fw_fallback.sh
[1] tools/testing/selftests/firmware/fw_filesystem.sh

Signed-off-by: Luis R. Rodriguez <mcgrof@...nel.org>
---
 drivers/base/firmware_class.c | 249 +++++++++++++++++++++++++++++++-----------
 1 file changed, 184 insertions(+), 65 deletions(-)

diff --git a/drivers/base/firmware_class.c b/drivers/base/firmware_class.c
index 4b57cf5bc81d..99a859e9ddc6 100644
--- a/drivers/base/firmware_class.c
+++ b/drivers/base/firmware_class.c
@@ -41,6 +41,81 @@ MODULE_AUTHOR("Manuel Estrada Sainz");
 MODULE_DESCRIPTION("Multi purpose firmware loading support");
 MODULE_LICENSE("GPL");
 
+/**
+ * enum fw_api_mode - firmware API mode of operation
+ * @FW_API_SYNC: used to determine if we should look for the firmware file
+ *	immediatley.
+ * @FW_API_ASYNC: used to determine if we should schedule the search for
+ *	your firmware file to be run at a later time.
+ */
+enum fw_api_mode {
+	FW_API_SYNC = 0,
+	FW_API_ASYNC,
+};
+
+/**
+ * enum fw_priv_reqs - private features only used internally
+ *
+ * @FW_PRIV_REQ_FALLBACK: specifies that the firmware request
+ *	will use a fallback mechanism if the kernel's direct filesystem
+ *	lookup failed to find the requested firmware. If the flag
+ *	%FW_PRIV_REQ_FALLBACK is set but the flag
+ *	%FW_PRIV_REQ_FALLBACK_UEVENT is not set, it means the caller
+ *	is relying on a custom fallback mechanism for firmwarwe lookup as a
+ *	fallback mechanism. The custom fallback mechanism is expected to find
+ *	any found firmware using the exposed sysfs interface of the
+ *	firmware_class.  Since the custom fallback mechanism is not compatible
+ *	with the internal caching mechanism for firmware lookups at resume,
+ *	caching will be disabled when the custom fallback mechanism is used.
+ * @FW_PRIV_REQ_FALLBACK_UEVENT: indicates that the fallback mechanism
+ *	this firmware request will rely on will be that of having the kernel
+ *	issue a uevent to userspace. Userspace in turn is expected to be
+ *	monitoring for uevents for the firmware_class and will use the
+ *	exposted sysfs interface to upload the firmware for the caller.
+ * @FW_PRIV_REQ_NO_CACHE: indicates that the firmware request
+ *	should not set up and use the internal caching mechanism to assist
+ *	drivers from fetching firmware at resume time after suspend.
+ * @FW_PRIV_REQ_OPTIONAL: if set it is not a hard requirement by the
+ *	caller that the file requested be present. An error will not be recorded
+ *	if the file is not found.
+ */
+enum fw_priv_reqs {
+	FW_PRIV_REQ_FALLBACK			= 1 << 0,
+	FW_PRIV_REQ_FALLBACK_UEVENT		= 1 << 1,
+	FW_PRIV_REQ_NO_CACHE			= 1 << 2,
+	FW_PRIV_REQ_OPTIONAL			= 1 << 3,
+};
+
+/**
+ * struct fw_priv_params - private firmware parameters
+ * @mode: mode of operation
+ * @priv_reqs: private set of &enum fw_priv_reqs, private requirements for
+ *	the firmware request
+ * @alloc_buf: buffer area allocated by the caller so we can place the
+ *	respective firmware
+ * @alloc_buf_size: size of the @alloc_buf
+ */
+struct fw_priv_params {
+	enum fw_api_mode mode;
+	u64 priv_reqs;
+	void *alloc_buf;
+	size_t alloc_buf_size;
+};
+
+#define fw_req_param_sync(priv_params)					\
+	(priv_params->mode == FW_API_SYNC)
+#define fw_req_param_async(priv_params)					\
+	(priv_params->mode == FW_API_ASYNC)
+
+#define fw_param_use_fallback(params)					\
+	(!!((params)->priv_reqs & FW_PRIV_REQ_FALLBACK))
+#define fw_param_uevent(params)						\
+	(!!((params)->priv_reqs & FW_PRIV_REQ_FALLBACK_UEVENT))
+#define fw_param_nocache(params)					\
+	(!!((params)->priv_reqs & FW_PRIV_REQ_NO_CACHE))
+#define fw_param_optional(params)					\
+	(!!((params)->priv_reqs & FW_PRIV_REQ_OPTIONAL))
+
 /* Builtin firmware support */
 
 #ifdef CONFIG_FW_LOADER
@@ -49,9 +124,19 @@ extern struct builtin_fw __start_builtin_fw[];
 extern struct builtin_fw __end_builtin_fw[];
 
 static bool fw_get_builtin_firmware(struct firmware *fw, const char *name,
-				    void *buf, size_t size)
+				    struct fw_priv_params *fw_priv_params)
 {
 	struct builtin_fw *b_fw;
+	void *buf;
+	size_t size;
+
+	if (fw_priv_params) {
+		buf = fw_priv_params->alloc_buf;
+		size = fw_priv_params->alloc_buf_size;
+	} else {
+		buf = NULL;
+		size = 0;
+	}
 
 	for (b_fw = __start_builtin_fw; b_fw != __end_builtin_fw; b_fw++) {
 		if (strcmp(name, b_fw->name) == 0) {
@@ -81,8 +166,8 @@ static bool fw_is_builtin_firmware(const struct firmware *fw)
 #else /* Module case - no builtin firmware support */
 
 static inline bool fw_get_builtin_firmware(struct firmware *fw,
-					   const char *name, void *buf,
-					   size_t size)
+					   const char *name,
+					   struct fw_params *fw_priv_params);
 {
 	return false;
 }
@@ -180,22 +265,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
-#ifdef CONFIG_FW_LOADER_USER_HELPER_FALLBACK
-#define FW_OPT_FALLBACK		FW_OPT_USERHELPER
-#else
-#define FW_OPT_FALLBACK		0
-#endif
-#define FW_OPT_NO_WARN	(1U << 3)
-#define FW_OPT_NOCACHE	(1U << 4)
-
 struct firmware_cache {
 	/* firmware_buf instance will be added into the below list */
 	spinlock_t lock;
@@ -260,11 +329,22 @@ static DEFINE_MUTEX(fw_lock);
 
 static struct firmware_cache fw_cache;
 
-static struct firmware_buf *__allocate_fw_buf(const char *fw_name,
-					      struct firmware_cache *fwc,
-					      void *dbuf, size_t size)
+static
+struct firmware_buf *__allocate_fw_buf(const char *fw_name,
+				       struct firmware_cache *fwc,
+				       struct fw_priv_params *fw_priv_params)
 {
 	struct firmware_buf *buf;
+	void *dbuf;
+	size_t size;
+
+	if (fw_priv_params) {
+		dbuf = fw_priv_params->alloc_buf;
+		size = fw_priv_params->alloc_buf_size;
+	} else {
+		dbuf = NULL;
+		size = 0;
+	}
 
 	buf = kzalloc(sizeof(*buf), GFP_ATOMIC);
 	if (!buf)
@@ -304,8 +384,8 @@ static struct firmware_buf *__fw_lookup_buf(const char *fw_name)
 /* Returns 1 for batching firmware requests with the same name */
 static int fw_lookup_and_allocate_buf(const char *fw_name,
 				      struct firmware_cache *fwc,
-				      struct firmware_buf **buf, void *dbuf,
-				      size_t size)
+				      struct firmware_buf **buf,
+				      struct fw_priv_params *fw_priv_params)
 {
 	struct firmware_buf *tmp;
 
@@ -318,7 +398,7 @@ static int fw_lookup_and_allocate_buf(const char *fw_name,
 		pr_debug("batched request - sharing the same struct firmware_buf and lookup for multiple requests\n");
 		return 1;
 	}
-	tmp = __allocate_fw_buf(fw_name, fwc, dbuf, size);
+	tmp = __allocate_fw_buf(fw_name, fwc, fw_priv_params);
 	if (tmp)
 		list_add(&tmp->list, &fwc->head);
 	spin_unlock(&fwc->lock);
@@ -524,7 +604,7 @@ static int fw_add_devm_name(struct device *dev, const char *name)
 #endif
 
 static int assign_firmware_buf(struct firmware *fw, struct device *device,
-			       unsigned int opt_flags)
+			       struct fw_priv_params *fw_priv_params)
 {
 	struct firmware_buf *buf = fw->priv;
 
@@ -542,15 +622,16 @@ static int assign_firmware_buf(struct firmware *fw, struct device *device,
 	 * should be fixed in devres or driver core.
 	 */
 	/* don't cache firmware handled without uevent */
-	if (device && (opt_flags & FW_OPT_UEVENT) &&
-	    !(opt_flags & FW_OPT_NOCACHE))
+	if (device &&
+	    fw_param_uevent(fw_priv_params) &&
+	    !fw_param_nocache(fw_priv_params))
 		fw_add_devm_name(device, buf->fw_id);
 
 	/*
 	 * After caching firmware image is started, let it piggyback
 	 * on request firmware.
 	 */
-	if (!(opt_flags & FW_OPT_NOCACHE) &&
+	if (!fw_param_nocache(fw_priv_params) &&
 	    buf->fwc->state == FW_LOADER_START_CACHE) {
 		if (fw_cache_piggyback_on_request(buf->fw_id))
 			kref_get(&buf->ref);
@@ -993,7 +1074,8 @@ static const struct attribute_group *fw_dev_attr_groups[] = {
 
 static struct firmware_priv *
 fw_create_instance(struct firmware *firmware, const char *fw_name,
-		   struct device *device, unsigned int opt_flags)
+		   struct device *device,
+		   struct fw_priv_params *fw_priv_params)
 {
 	struct firmware_priv *fw_priv;
 	struct device *f_dev;
@@ -1004,7 +1086,7 @@ fw_create_instance(struct firmware *firmware, const char *fw_name,
 		goto exit;
 	}
 
-	fw_priv->nowait = !!(opt_flags & FW_OPT_NOWAIT);
+	fw_priv->nowait = fw_req_param_async(fw_priv_params);
 	fw_priv->fw = firmware;
 	f_dev = &fw_priv->dev;
 
@@ -1019,7 +1101,8 @@ fw_create_instance(struct firmware *firmware, const char *fw_name,
 
 /* load a firmware via user helper */
 static int _request_firmware_load(struct firmware_priv *fw_priv,
-				  unsigned int opt_flags, long timeout)
+				  struct fw_priv_params *fw_priv_params,
+				  long timeout)
 {
 	int retval = 0;
 	struct device *f_dev = &fw_priv->dev;
@@ -1041,7 +1124,7 @@ static int _request_firmware_load(struct firmware_priv *fw_priv,
 	list_add(&buf->pending_list, &pending_fw_head);
 	mutex_unlock(&fw_lock);
 
-	if (opt_flags & FW_OPT_UEVENT) {
+	if (fw_param_uevent(fw_priv_params)) {
 		buf->need_uevent = true;
 		dev_set_uevent_suppress(f_dev, false);
 		dev_dbg(f_dev, "firmware: requesting %s\n", buf->fw_id);
@@ -1073,14 +1156,14 @@ static int _request_firmware_load(struct firmware_priv *fw_priv,
 
 static int fw_load_from_user_helper(struct firmware *firmware,
 				    const char *name, struct device *device,
-				    unsigned int opt_flags)
+				    struct fw_priv_params *fw_priv_params)
 {
 	struct firmware_priv *fw_priv;
 	long timeout;
 	int ret;
 
 	timeout = firmware_loading_timeout();
-	if (opt_flags & FW_OPT_NOWAIT) {
+	if (fw_req_param_async(fw_priv_params)) {
 		timeout = usermodehelper_read_lock_wait(timeout);
 		if (!timeout) {
 			dev_dbg(device, "firmware: %s loading timed out\n",
@@ -1096,17 +1179,17 @@ static int fw_load_from_user_helper(struct firmware *firmware,
 		}
 	}
 
-	fw_priv = fw_create_instance(firmware, name, device, opt_flags);
+	fw_priv = fw_create_instance(firmware, name, device, fw_priv_params);
 	if (IS_ERR(fw_priv)) {
 		ret = PTR_ERR(fw_priv);
 		goto out_unlock;
 	}
 
 	fw_priv->buf = firmware->priv;
-	ret = _request_firmware_load(fw_priv, opt_flags, timeout);
+	ret = _request_firmware_load(fw_priv, fw_priv_params, timeout);
 
 	if (!ret)
-		ret = assign_firmware_buf(firmware, device, opt_flags);
+		ret = assign_firmware_buf(firmware, device, fw_priv_params);
 
 out_unlock:
 	usermodehelper_read_unlock();
@@ -1117,7 +1200,8 @@ static int fw_load_from_user_helper(struct firmware *firmware,
 #else /* CONFIG_FW_LOADER_USER_HELPER */
 static inline int
 fw_load_from_user_helper(struct firmware *firmware, const char *name,
-			 struct device *device, unsigned int opt_flags)
+			 struct device *device,
+			 struct fw_priv_params *fw_priv_params)
 {
 	return -ENOENT;
 }
@@ -1132,7 +1216,8 @@ static inline void kill_pending_fw_fallback_reqs(bool only_kill_custom) { }
  */
 static int
 _request_firmware_prepare(struct firmware **firmware_p, const char *name,
-			  struct device *device, void *dbuf, size_t size)
+			  struct device *device,
+			  struct fw_priv_params *fw_priv_params)
 {
 	struct firmware *firmware;
 	struct firmware_buf *buf;
@@ -1145,12 +1230,12 @@ _request_firmware_prepare(struct firmware **firmware_p, const char *name,
 		return -ENOMEM;
 	}
 
-	if (fw_get_builtin_firmware(firmware, name, dbuf, size)) {
+	if (fw_get_builtin_firmware(firmware, name, fw_priv_params)) {
 		dev_dbg(device, "using built-in %s\n", name);
 		return 0; /* assigned */
 	}
 
-	ret = fw_lookup_and_allocate_buf(name, &fw_cache, &buf, dbuf, size);
+	ret = fw_lookup_and_allocate_buf(name, &fw_cache, &buf, fw_priv_params);
 
 	/*
 	 * bind with 'buf' now to avoid warning in failure path
@@ -1193,11 +1278,33 @@ static void fw_abort_batch_reqs(struct firmware *fw)
 		fw_state_aborted(&buf->fw_st);
 }
 
+#ifdef CONFIG_FW_LOADER_USER_HELPER
+static int fw_fallback(struct firmware *fw, const char *name,
+		       struct device *device,
+		       struct fw_priv_params *fw_priv_params,
+		       int ret)
+{
+	if (!fw_param_use_fallback(fw_priv_params))
+		return ret;
+
+	dev_warn(device, "Falling back to user helper\n");
+	return fw_load_from_user_helper(fw, name, device, fw_priv_params);
+}
+#else
+static int fw_fallback(struct firmware *fw, const char *name,
+		       struct device *device,
+		       struct fw_priv_params *fw_priv_params,
+		       int ret)
+{
+	return -ENOENT;
+}
+#endif
+
 /* called from request_firmware() and request_firmware_work_func() */
 static int
 _request_firmware(const struct firmware **firmware_p, const char *name,
-		  struct device *device, void *buf, size_t size,
-		  unsigned int opt_flags)
+		  struct fw_priv_params *fw_priv_params,
+		  struct device *device)
 {
 	struct firmware *fw = NULL;
 	int ret;
@@ -1210,23 +1317,19 @@ _request_firmware(const struct firmware **firmware_p, const char *name,
 		goto out;
 	}
 
-	ret = _request_firmware_prepare(&fw, name, device, buf, size);
+	ret = _request_firmware_prepare(&fw, name, device, fw_priv_params);
 	if (ret <= 0) /* error or already assigned */
 		goto out;
 
 	ret = fw_get_filesystem_firmware(device, fw->priv);
 	if (ret) {
-		if (!(opt_flags & FW_OPT_NO_WARN))
+		if (!fw_param_optional(fw_priv_params))
 			dev_warn(device,
 				 "Direct firmware load for %s failed with error %d\n",
 				 name, ret);
-		if (opt_flags & FW_OPT_USERHELPER) {
-			dev_warn(device, "Falling back to user helper\n");
-			ret = fw_load_from_user_helper(fw, name, device,
-						       opt_flags);
-		}
+		ret = fw_fallback(fw, name, device, fw_priv_params, ret);
 	} else
-		ret = assign_firmware_buf(fw, device, opt_flags);
+		ret = assign_firmware_buf(fw, device, fw_priv_params);
 
  out:
 	if (ret < 0) {
@@ -1264,11 +1367,14 @@ request_firmware(const struct firmware **firmware_p, const char *name,
 		 struct device *device)
 {
 	int ret;
+	struct fw_priv_params fw_priv_params = {
+		.priv_reqs = FW_PRIV_REQ_FALLBACK |
+			     FW_PRIV_REQ_FALLBACK_UEVENT
+	};
 
 	/* Need to pin this module until return */
 	__module_get(THIS_MODULE);
-	ret = _request_firmware(firmware_p, name, device, NULL, 0,
-				FW_OPT_UEVENT | FW_OPT_FALLBACK);
+	ret = _request_firmware(firmware_p, name, &fw_priv_params, device);
 	module_put(THIS_MODULE);
 	return ret;
 }
@@ -1289,10 +1395,12 @@ int request_firmware_direct(const struct firmware **firmware_p,
 			    const char *name, struct device *device)
 {
 	int ret;
+	struct fw_priv_params fw_priv_params = {
+		.priv_reqs = FW_PRIV_REQ_FALLBACK_UEVENT
+	};
 
 	__module_get(THIS_MODULE);
-	ret = _request_firmware(firmware_p, name, device, NULL, 0,
-				FW_OPT_UEVENT | FW_OPT_NO_WARN);
+	ret = _request_firmware(firmware_p, name, &fw_priv_params, device);
 	module_put(THIS_MODULE);
 	return ret;
 }
@@ -1318,11 +1426,16 @@ request_firmware_into_buf(const struct firmware **firmware_p, const char *name,
 			  struct device *device, void *buf, size_t size)
 {
 	int ret;
+	struct fw_priv_params fw_priv_params = {
+		.priv_reqs = FW_PRIV_REQ_FALLBACK |
+			     FW_PRIV_REQ_FALLBACK_UEVENT |
+			     FW_PRIV_REQ_NO_CACHE,
+		.alloc_buf = buf,
+		.alloc_buf_size = size
+	};
 
 	__module_get(THIS_MODULE);
-	ret = _request_firmware(firmware_p, name, device, buf, size,
-				FW_OPT_UEVENT | FW_OPT_FALLBACK |
-				FW_OPT_NOCACHE);
+	ret = _request_firmware(firmware_p, name, &fw_priv_params, device);
 	module_put(THIS_MODULE);
 	return ret;
 }
@@ -1347,21 +1460,22 @@ struct firmware_work {
 	struct work_struct work;
 	struct module *module;
 	const char *name;
+	struct fw_priv_params fw_priv_params;
 	struct device *device;
 	void *context;
 	void (*cont)(const struct firmware *fw, void *context);
-	unsigned int opt_flags;
 };
 
 static void request_firmware_work_func(struct work_struct *work)
 {
 	struct firmware_work *fw_work;
 	const struct firmware *fw;
+	struct fw_priv_params *fw_priv_params;
 
 	fw_work = container_of(work, struct firmware_work, work);
+	fw_priv_params = &fw_work->fw_priv_params;
 
-	_request_firmware(&fw, fw_work->name, fw_work->device, NULL, 0,
-			  fw_work->opt_flags);
+	_request_firmware(&fw, fw_work->name, fw_priv_params, fw_work->device);
 	fw_work->cont(fw, fw_work->context);
 	put_device(fw_work->device); /* taken in request_firmware_nowait() */
 
@@ -1400,6 +1514,11 @@ request_firmware_nowait(
 	void (*cont)(const struct firmware *fw, void *context))
 {
 	struct firmware_work *fw_work;
+	struct fw_priv_params fw_priv_params = {
+		.mode = FW_API_ASYNC,
+		.priv_reqs = FW_PRIV_REQ_FALLBACK |
+			     (uevent ? FW_PRIV_REQ_FALLBACK_UEVENT : 0)
+	};
 
 	fw_work = kzalloc(sizeof(struct firmware_work), gfp);
 	if (!fw_work)
@@ -1414,8 +1533,8 @@ 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);
+	memcpy(&fw_work->fw_priv_params, &fw_priv_params,
+	       sizeof(struct fw_priv_params));
 
 	if (!try_module_get(module)) {
 		kfree_const(fw_work->name);
@@ -1493,7 +1612,7 @@ static int uncache_firmware(const char *fw_name)
 
 	pr_debug("%s: %s\n", __func__, fw_name);
 
-	if (fw_get_builtin_firmware(&fw, fw_name, NULL, 0))
+	if (fw_get_builtin_firmware(&fw, fw_name, NULL))
 		return 0;
 
 	buf = fw_lookup_buf(fw_name);
-- 
2.14.0

Powered by blists - more mailing lists