[PATCH v2 1/2] firmware_class: encapsulate firmware loading status

[Daniel Wagner <wagi@monom.org>]

From: Daniel Wagner <daniel.wagner@bmw-carit.de>

The firmware user helper code tracks the current state of the loading
process via an member of struct firmware_buf and a completion. Let's
encapsulate this simple state machine into struct fw_status.

We don't need the fw_lock to protect the state machine anymore. First of
all we don't do any RWM operations instead we do either a write or read
to the status variable. Reading the status variable via READ_ONCE
ensures we see the new state whenever we get woken. We are not allowed
to use the complete_all() more than once so we need to filter out all
state transition but the last. That is okay since only the final state
DONE|ABORTED is of interest.

Besides that we also safe a few bytes:

cris etrax-100lx_defconfig

$ bloat-o-meter firmware_class.o.old firmware_class.o.new
add/remove: 0/0 grow/shrink: 0/1 up/down: 0/-234 (-234)
function                                     old     new   delta
_request_firmware                           1228     994    -234
Total: Before=2179, After=1945, chg -10.74%

x86_64 FW_LOADER !FW_LOADER_USER_HELPER_FALLBACK

$ bloat-o-meter firmware_class.o.old firmware_class.o.new
add/remove: 0/0 grow/shrink: 0/1 up/down: 0/-194 (-194)
function                                     old     new   delta
_request_firmware                           2118    1924    -194
Total: Before=6811, After=6617, chg -2.85%

x86_64 FW_LOADER FW_LOADER_USER_HELPER_FALLBACK

$ bloat-o-meter firmware_class.o.old firmware_class.o.new
add/remove: 0/0 grow/shrink: 2/5 up/down: 4/-193 (-189)
function                                     old     new   delta
fw_shutdown_notify                            81      83      +2
firmware_data_read                           162     164      +2
fw_pm_notify                                 396     394      -2
firmware_loading_store                       458     441     -17
firmware_data_write                          504     475     -29
firmware_loading_show                         92      61     -31
_request_firmware                           2931    2817    -114
Total: Before=10328, After=10139, chg -1.83%

Note the variable loading_timeout and the inline function
firmware_loadint_timeout() are used also when no user helper is enabled.
So they need to be moved out side of the ifdef section.

Signed-off-by: Daniel Wagner <daniel.wagner@bmw-carit.de>
Cc: Ming Lei <ming.lei@canonical.com>
Cc: Luis R. Rodriguez <mcgrof@kernel.org>
Cc: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
---
 drivers/base/firmware_class.c | 158 +++++++++++++++++++++++++-----------------
 1 file changed, 93 insertions(+), 65 deletions(-)

diff --git a/drivers/base/firmware_class.c b/drivers/base/firmware_class.c
index 22d1760..d3dcf87 100644
--- a/drivers/base/firmware_class.c
+++ b/drivers/base/firmware_class.c
@@ -91,19 +91,88 @@ static inline bool fw_is_builtin_firmware(const struct firmware *fw)
 }
 #endif
 
+static int loading_timeout = 60;	/* In seconds */
+
+static inline long firmware_loading_timeout(void)
+{
+	return loading_timeout > 0 ? loading_timeout * HZ : MAX_JIFFY_OFFSET;
+}
+
+#ifdef CONFIG_FW_LOADER_USER_HELPER
+
 enum {
+	FW_STATUS_UNKNOWN,
 	FW_STATUS_LOADING,
 	FW_STATUS_DONE,
-	FW_STATUS_ABORT,
+	FW_STATUS_ABORTED,
 };
 
-static int loading_timeout = 60;	/* In seconds */
+struct fw_status {
+	unsigned long status;
+	struct completion completion;
+};
 
-static inline long firmware_loading_timeout(void)
+static void fw_status_init(struct fw_status *fw_st)
 {
-	return loading_timeout > 0 ? loading_timeout * HZ : MAX_JIFFY_OFFSET;
+	fw_st->status = FW_STATUS_UNKNOWN;
+	init_completion(&fw_st->completion);
+}
+
+static unsigned long __fw_status_get(struct fw_status *fw_st)
+{
+	return READ_ONCE(fw_st->status);
+}
+
+static int fw_status_wait_timeout(struct fw_status *fw_st, long timeout)
+{
+	unsigned long status;
+	int err;
+
+	err = wait_for_completion_interruptible_timeout(&fw_st->completion,
+							timeout);
+	status = READ_ONCE(fw_st->status);
+	if (err == 0 && status == FW_STATUS_ABORTED)
+		return -ENOENT;
+
+	return err;
+}
+
+static void __fw_status_set(struct fw_status *fw_st,
+			  unsigned long status)
+{
+	WRITE_ONCE(fw_st->status, status);
+
+	if (status == FW_STATUS_DONE ||
+			status == FW_STATUS_ABORTED)
+		complete_all(&fw_st->completion);
 }
 
+#define fw_status_start(fw_st)					\
+	__fw_status_set(fw_st, FW_STATUS_LOADING)
+#define fw_status_done(fw_st)					\
+	__fw_status_set(fw_st, FW_STATUS_DONE)
+#define fw_status_aborted(fw_st)				\
+	__fw_status_set(fw_st, FW_STATUS_ABORTED)
+#define fw_status_is_loading(fw_st)				\
+	(__fw_status_get(fw_st) == FW_STATUS_LOADING)
+#define fw_status_is_done(fw_st)				\
+	(__fw_status_get(fw_st) == FW_STATUS_DONE)
+#define fw_status_is_aborted(fw_st)				\
+	(__fw_status_get(fw_st) == FW_STATUS_ABORTED)
+
+#else /* CONFIG_FW_LOADER_USER_HELPER */
+
+#define fw_status_wait_timeout(fw_st, long)	0
+
+#define fw_status_start(fw_st)
+#define fw_status_done(fw_st)
+#define fw_status_aborted(fw_st)
+#define fw_status_is_loading(fw_st)
+#define fw_status_is_done(fw_st)
+#define fw_status_is_aborted(fw_st)		false
+
+#endif /* !CONFIG_FW_LOADER_USER_HELPER */
+
 /* firmware behavior options */
 #define FW_OPT_UEVENT	(1U << 0)
 #define FW_OPT_NOWAIT	(1U << 1)
@@ -145,13 +214,12 @@ struct firmware_cache {
 struct firmware_buf {
 	struct kref ref;
 	struct list_head list;
-	struct completion completion;
 	struct firmware_cache *fwc;
-	unsigned long status;
 	void *data;
 	size_t size;
 	size_t allocated_size;
 #ifdef CONFIG_FW_LOADER_USER_HELPER
+	struct fw_status fw_st;
 	bool is_paged_buf;
 	bool need_uevent;
 	struct page **pages;
@@ -205,8 +273,8 @@ static struct firmware_buf *__allocate_fw_buf(const char *fw_name,
 	buf->fwc = fwc;
 	buf->data = dbuf;
 	buf->allocated_size = size;
-	init_completion(&buf->completion);
 #ifdef CONFIG_FW_LOADER_USER_HELPER
+	fw_status_init(&buf->fw_st);
 	INIT_LIST_HEAD(&buf->pending_list);
 #endif
 
@@ -305,15 +373,6 @@ static const char * const fw_path[] = {
 module_param_string(path, fw_path_para, sizeof(fw_path_para), 0644);
 MODULE_PARM_DESC(path, "customized firmware image search path with a higher priority than default path");
 
-static void fw_finish_direct_load(struct device *device,
-				  struct firmware_buf *buf)
-{
-	mutex_lock(&fw_lock);
-	set_bit(FW_STATUS_DONE, &buf->status);
-	complete_all(&buf->completion);
-	mutex_unlock(&fw_lock);
-}
-
 static int
 fw_get_filesystem_firmware(struct device *device, struct firmware_buf *buf)
 {
@@ -360,7 +419,7 @@ fw_get_filesystem_firmware(struct device *device, struct firmware_buf *buf)
 		}
 		dev_dbg(device, "direct-loading %s\n", buf->fw_id);
 		buf->size = size;
-		fw_finish_direct_load(device, buf);
+		fw_status_done(&buf->fw_st);
 		break;
 	}
 	__putname(path);
@@ -478,12 +537,11 @@ static void __fw_load_abort(struct firmware_buf *buf)
 	 * There is a small window in which user can write to 'loading'
 	 * between loading done and disappearance of 'loading'
 	 */
-	if (test_bit(FW_STATUS_DONE, &buf->status))
+	if (fw_status_is_done(&buf->fw_st))
 		return;
 
 	list_del_init(&buf->pending_list);
-	set_bit(FW_STATUS_ABORT, &buf->status);
-	complete_all(&buf->completion);
+	fw_status_aborted(&buf->fw_st);
 }
 
 static void fw_load_abort(struct firmware_priv *fw_priv)
@@ -496,9 +554,6 @@ static void fw_load_abort(struct firmware_priv *fw_priv)
 	fw_priv->buf = NULL;
 }
 
-#define is_fw_load_aborted(buf)	\
-	test_bit(FW_STATUS_ABORT, &(buf)->status)
-
 static LIST_HEAD(pending_fw_head);
 
 /* reboot notifier for avoid deadlock with usermode_lock */
@@ -596,10 +651,8 @@ static ssize_t firmware_loading_show(struct device *dev,
 	struct firmware_priv *fw_priv = to_firmware_priv(dev);
 	int loading = 0;
 
-	mutex_lock(&fw_lock);
 	if (fw_priv->buf)
-		loading = test_bit(FW_STATUS_LOADING, &fw_priv->buf->status);
-	mutex_unlock(&fw_lock);
+		loading = fw_status_is_loading(&fw_priv->buf->fw_st);
 
 	return sprintf(buf, "%d\n", loading);
 }
@@ -653,23 +706,20 @@ static ssize_t firmware_loading_store(struct device *dev,
 	switch (loading) {
 	case 1:
 		/* discarding any previous partial load */
-		if (!test_bit(FW_STATUS_DONE, &fw_buf->status)) {
+		if (!fw_status_is_done(&fw_buf->fw_st)) {
 			for (i = 0; i < fw_buf->nr_pages; i++)
 				__free_page(fw_buf->pages[i]);
 			vfree(fw_buf->pages);
 			fw_buf->pages = NULL;
 			fw_buf->page_array_size = 0;
 			fw_buf->nr_pages = 0;
-			set_bit(FW_STATUS_LOADING, &fw_buf->status);
+			fw_status_start(&fw_buf->fw_st);
 		}
 		break;
 	case 0:
-		if (test_bit(FW_STATUS_LOADING, &fw_buf->status)) {
+		if (fw_status_is_loading(&fw_buf->fw_st)) {
 			int rc;
 
-			set_bit(FW_STATUS_DONE, &fw_buf->status);
-			clear_bit(FW_STATUS_LOADING, &fw_buf->status);
-
 			/*
 			 * Several loading requests may be pending on
 			 * one same firmware buf, so let all requests
@@ -691,10 +741,11 @@ static ssize_t firmware_loading_store(struct device *dev,
 			 */
 			list_del_init(&fw_buf->pending_list);
 			if (rc) {
-				set_bit(FW_STATUS_ABORT, &fw_buf->status);
+				fw_status_aborted(&fw_buf->fw_st);
 				written = rc;
+			} else {
+				fw_status_done(&fw_buf->fw_st);
 			}
-			complete_all(&fw_buf->completion);
 			break;
 		}
 		/* fallthrough */
@@ -702,7 +753,7 @@ static ssize_t firmware_loading_store(struct device *dev,
 		dev_err(dev, "%s: unexpected value (%d)\n", __func__, loading);
 		/* fallthrough */
 	case -1:
-		fw_load_abort(fw_priv);
+		fw_status_aborted(&fw_buf->fw_st);
 		break;
 	}
 out:
@@ -755,7 +806,7 @@ static ssize_t firmware_data_read(struct file *filp, struct kobject *kobj,
 
 	mutex_lock(&fw_lock);
 	buf = fw_priv->buf;
-	if (!buf || test_bit(FW_STATUS_DONE, &buf->status)) {
+	if (!buf || fw_status_is_done(&buf->fw_st)) {
 		ret_count = -ENODEV;
 		goto out;
 	}
@@ -842,7 +893,7 @@ static ssize_t firmware_data_write(struct file *filp, struct kobject *kobj,
 
 	mutex_lock(&fw_lock);
 	buf = fw_priv->buf;
-	if (!buf || test_bit(FW_STATUS_DONE, &buf->status)) {
+	if (!buf || fw_status_is_done(&buf->fw_st)) {
 		retval = -ENODEV;
 		goto out;
 	}
@@ -955,8 +1006,7 @@ static int _request_firmware_load(struct firmware_priv *fw_priv,
 		timeout = MAX_JIFFY_OFFSET;
 	}
 
-	retval = wait_for_completion_interruptible_timeout(&buf->completion,
-			timeout);
+	retval = fw_status_wait_timeout(&buf->fw_st, timeout);
 	if (retval == -ERESTARTSYS || !retval) {
 		mutex_lock(&fw_lock);
 		fw_load_abort(fw_priv);
@@ -965,7 +1015,7 @@ static int _request_firmware_load(struct firmware_priv *fw_priv,
 		retval = 0;
 	}
 
-	if (is_fw_load_aborted(buf))
+	if (fw_status_is_aborted(&buf->fw_st))
 		retval = -EAGAIN;
 	else if (buf->is_paged_buf && !buf->data)
 		retval = -ENOMEM;
@@ -1015,35 +1065,12 @@ fw_load_from_user_helper(struct firmware *firmware, const char *name,
 	return -ENOENT;
 }
 
-/* No abort during direct loading */
-#define is_fw_load_aborted(buf) false
-
 #ifdef CONFIG_PM_SLEEP
 static inline void kill_requests_without_uevent(void) { }
 #endif
 
 #endif /* CONFIG_FW_LOADER_USER_HELPER */
 
-
-/* wait until the shared firmware_buf becomes ready (or error) */
-static int sync_cached_firmware_buf(struct firmware_buf *buf)
-{
-	int ret = 0;
-
-	mutex_lock(&fw_lock);
-	while (!test_bit(FW_STATUS_DONE, &buf->status)) {
-		if (is_fw_load_aborted(buf)) {
-			ret = -ENOENT;
-			break;
-		}
-		mutex_unlock(&fw_lock);
-		ret = wait_for_completion_interruptible(&buf->completion);
-		mutex_lock(&fw_lock);
-	}
-	mutex_unlock(&fw_lock);
-	return ret;
-}
-
 /* prepare firmware and firmware_buf structs;
  * return 0 if a firmware is already assigned, 1 if need to load one,
  * or a negative error code
@@ -1077,7 +1104,8 @@ _request_firmware_prepare(struct firmware **firmware_p, const char *name,
 	firmware->priv = buf;
 
 	if (ret > 0) {
-		ret = sync_cached_firmware_buf(buf);
+		ret = fw_status_wait_timeout(&buf->fw_st,
+					     firmware_loading_timeout());
 		if (!ret) {
 			fw_set_page_data(buf, firmware);
 			return 0; /* assigned */
@@ -1095,7 +1123,7 @@ static int assign_firmware_buf(struct firmware *fw, struct device *device,
 	struct firmware_buf *buf = fw->priv;
 
 	mutex_lock(&fw_lock);
-	if (!buf->size || is_fw_load_aborted(buf)) {
+	if (!buf->size || fw_status_is_aborted(&buf->fw_st)) {
 		mutex_unlock(&fw_lock);
 		return -ENOENT;
 	}
-- 
2.7.4