[PATCH v3 0/3] firmware_class: encapsulate firmware loading status

[PATCH v3 0/3] firmware_class: encapsulate firmware loading status

[Daniel Wagner]

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

This version should address all comments from Luis. In the last patch
the fw_lock dependency is dropped. This only works if we garantee not
to race between the reader and the writer side in combination in going
to sleep and waking up. So here that should now be a good argument for
swait :)

cheers,
daniel

changes since v2:
  - more splitting out
    - first patch factors out all the bit ops into fw_status
    - second patch gets rid of the bit ops
    - third get rid of fw_lock by using swait

changes since v1:
  - moved swait change into its own patch
  - added ifdef section for FW_LOADER_USER_HELPER_FALLBACK
  - updated commit message highlighting the mutex usage drop a bit

  https://lkml.org/lkml/2016/8/4/239

Cc: Ming Lei <ming.lei@canonical.com>
Cc: Luis R. Rodriguez <mcgrof@kernel.org>
Cc: Greg Kroah-Hartman <gregkh@linuxfoundation.org>

Daniel Wagner (3):
  firmware_class: encapsulate firmware loading status
  firmware_class: Drop bit ops in favor of simple state machine
  firmware_class: Do not use fw_lock for fw_status protection

 drivers/base/firmware_class.c | 153 ++++++++++++++++++++++++++----------------
 1 file changed, 94 insertions(+), 59 deletions(-)

-- 
2.7.4

[PATCH v3 1/3] firmware_class: encapsulate firmware loading status

[Daniel Wagner]

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

The firmware user helper code tracks the current state of the loading
process via unsigned long status and a complection in struct
firmware_buf. We only need this for the usermode helper as such we can
encapsulate all this data into its own data structure.

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 | 126 ++++++++++++++++++++++++++++++------------
 1 file changed, 91 insertions(+), 35 deletions(-)

diff --git a/drivers/base/firmware_class.c b/drivers/base/firmware_class.c
index 22d1760..f397026 100644
--- a/drivers/base/firmware_class.c
+++ b/drivers/base/firmware_class.c
@@ -91,10 +91,13 @@ static inline bool fw_is_builtin_firmware(const struct firmware *fw)
 }
 #endif
 
+#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 */
@@ -104,6 +107,72 @@ static inline long firmware_loading_timeout(void)
 	return loading_timeout > 0 ? loading_timeout * HZ : MAX_JIFFY_OFFSET;
 }
 
+struct fw_status {
+	unsigned long status;
+	struct completion completion;
+};
+
+static void fw_status_init(struct fw_status *fw_st)
+{
+	fw_st->status = FW_STATUS_UNKNOWN;
+	init_completion(&fw_st->completion);
+}
+
+static int __fw_status_check(struct fw_status *fw_st, unsigned long status)
+{
+	return test_bit(status, &fw_st->status);
+}
+
+static int fw_status_wait_timeout(struct fw_status *fw_st, long timeout)
+{
+	int ret;
+
+	ret = wait_for_completion_interruptible_timeout(&fw_st->completion,
+							timeout);
+	if (ret == 0 && test_bit(FW_STATUS_ABORTED, &fw_st->status))
+		return -ENOENT;
+
+	return ret;
+}
+
+static void __fw_status_set(struct fw_status *fw_st,
+			  unsigned long status)
+{
+	set_bit(status, &fw_st->status);
+
+	if (status == FW_STATUS_DONE ||
+			status == FW_STATUS_ABORTED) {
+		clear_bit(FW_STATUS_LOADING, &fw_st->status);
+		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_check(fw_st, FW_STATUS_LOADING)
+#define fw_status_is_done(fw_st)				\
+	__fw_status_check(fw_st, FW_STATUS_DONE)
+#define fw_status_is_aborted(fw_st)				\
+	__fw_status_check(fw_st, FW_STATUS_ABORTED)
+
+#else /* CONFIG_FW_LOADER_USER_HELPER */
+
+static int loading_timeout = 60;
+#define firmware_loading_timeout()		(loading_timeout * HZ)
+
+#define fw_status_wait_timeout(fw_st, long)	0
+
+#define fw_status_done(fw_st)
+#define fw_status_is_done(fw_st)		true
+#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
 
@@ -309,8 +377,7 @@ 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);
+	fw_status_done(&buf->fw_st);
 	mutex_unlock(&fw_lock);
 }
 
@@ -478,12 +545,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 +562,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 */
@@ -598,7 +661,7 @@ static ssize_t firmware_loading_show(struct device *dev,
 
 	mutex_lock(&fw_lock);
 	if (fw_priv->buf)
-		loading = test_bit(FW_STATUS_LOADING, &fw_priv->buf->status);
+		loading = fw_status_is_loading(&fw_priv->buf->fw_st);
 	mutex_unlock(&fw_lock);
 
 	return sprintf(buf, "%d\n", loading);
@@ -653,23 +716,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 +751,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 +763,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 +816,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 +903,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 +1016,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 +1025,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,29 +1075,25 @@ 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)) {
+	while (!fw_status_is_done(&buf->fw_st)) {
+		if (fw_status_is_aborted(&buf->fw_st)) {
 			ret = -ENOENT;
 			break;
 		}
 		mutex_unlock(&fw_lock);
-		ret = wait_for_completion_interruptible(&buf->completion);
+		ret = fw_status_wait_timeout(&buf->fw_st, 0);
 		mutex_lock(&fw_lock);
 	}
 	mutex_unlock(&fw_lock);
@@ -1095,7 +1151,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

Re: [PATCH v3 1/3] firmware_class: encapsulate firmware loading status

[Luis R. Rodriguez]

On Thu, Aug 25, 2016 at 11:52:01AM +0200, Daniel Wagner wrote:
> diff --git a/drivers/base/firmware_class.c b/drivers/base/firmware_class.c
> index 22d1760..f397026 100644
> --- a/drivers/base/firmware_class.c
> +++ b/drivers/base/firmware_class.c
> @@ -91,10 +91,13 @@ static inline bool fw_is_builtin_firmware(const struct firmware *fw)
>  }
>  #endif
>  
> +#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 */

<-- snip -->

> +#else /* CONFIG_FW_LOADER_USER_HELPER */
> +
> +static int loading_timeout = 60;
> +#define firmware_loading_timeout()		(loading_timeout * HZ)
> +
> +#define fw_status_wait_timeout(fw_st, long)	0

The timeout makes 0 sense for when !CONFIG_FW_LOADER_USER_HELPER so can
we do away with adding a silly 60 value to an int here and
the silly value of (loading_timeout * HZ) ? Its not used so its not
clear to me why this is here.

  Luis

Re: [PATCH v3 1/3] firmware_class: encapsulate firmware loading status

[Daniel Wagner]

On 08/25/2016 07:50 PM, Luis R. Rodriguez wrote:
>> +#else /* CONFIG_FW_LOADER_USER_HELPER */
>> +
>> +static int loading_timeout = 60;
>> +#define firmware_loading_timeout()		(loading_timeout * HZ)
>> +
>> +#define fw_status_wait_timeout(fw_st, long)	0
> 
> The timeout makes 0 sense for when !CONFIG_FW_LOADER_USER_HELPER so can
> we do away with adding a silly 60 value to an int here and
> the silly value of (loading_timeout * HZ) ? Its not used so its not
> clear to me why this is here.

So the main reason that silly timeout is needed is the usage of
it in device_cache_fw_images(). I suggest we add a timeout
argument to  _request_firmware() and use the right timeout value
at that level. 

That allows to move the loading_timeout into the
CONFIG_FW_LOADER_USER_HELPER section. 

What do you think?

diff --git a/drivers/base/firmware_class.c b/drivers/base/firmware_class.c
index 22d1760..afb6d93 100644
--- a/drivers/base/firmware_class.c
+++ b/drivers/base/firmware_class.c
@@ -1132,10 +1132,9 @@ static int assign_firmware_buf(struct firmware *fw, struct device *device,
 static int
 _request_firmware(const struct firmware **firmware_p, const char *name,
 		  struct device *device, void *buf, size_t size,
-		  unsigned int opt_flags)
+		  unsigned int opt_flags, long timeout)
 {
 	struct firmware *fw = NULL;
-	long timeout;
 	int ret;
 
 	if (!firmware_p)
@@ -1151,7 +1150,6 @@ _request_firmware(const struct firmware **firmware_p, const char *name,
 		goto out;
 
 	ret = 0;
-	timeout = firmware_loading_timeout();
 	if (opt_flags & FW_OPT_NOWAIT) {
 		timeout = usermodehelper_read_lock_wait(timeout);
 		if (!timeout) {
@@ -1226,7 +1224,8 @@ request_firmware(const struct firmware **firmware_p, const char *name,
 	/* 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);
+				FW_OPT_UEVENT | FW_OPT_FALLBACK,
+				firmware_loading_timeout());
 	module_put(THIS_MODULE);
 	return ret;
 }
@@ -1250,7 +1249,8 @@ int request_firmware_direct(const struct firmware **firmware_p,
 
 	__module_get(THIS_MODULE);
 	ret = _request_firmware(firmware_p, name, device, NULL, 0,
-				FW_OPT_UEVENT | FW_OPT_NO_WARN);
+				FW_OPT_UEVENT | FW_OPT_NO_WARN,
+				firmware_loading_timeout());
 	module_put(THIS_MODULE);
 	return ret;
 }
@@ -1280,7 +1280,8 @@ request_firmware_into_buf(const struct firmware **firmware_p, const char *name,
 	__module_get(THIS_MODULE);
 	ret = _request_firmware(firmware_p, name, device, buf, size,
 				FW_OPT_UEVENT | FW_OPT_FALLBACK |
-				FW_OPT_NOCACHE);
+				FW_OPT_NOCACHE,
+				firmware_loading_timeout());
 	module_put(THIS_MODULE);
 	return ret;
 }
@@ -1319,7 +1320,7 @@ static void request_firmware_work_func(struct work_struct *work)
 	fw_work = container_of(work, struct firmware_work, work);
 
 	_request_firmware(&fw, fw_work->name, fw_work->device, NULL, 0,
-			  fw_work->opt_flags);
+			  fw_work->opt_flags, firmware_loading_timeout());
 	fw_work->cont(fw, fw_work->context);
 	put_device(fw_work->device); /* taken in request_firmware_nowait() */
 
@@ -1391,6 +1392,16 @@ EXPORT_SYMBOL(request_firmware_nowait);
 #ifdef CONFIG_PM_SLEEP
 static ASYNC_DOMAIN_EXCLUSIVE(fw_cache_domain);
 
+/*
+ * use small loading timeout for caching devices' firmware
+ * because all these firmware images have been loaded
+ * successfully at lease once, also system is ready for
+ * completing firmware loading now. The maximum size of
+ * firmware in current distributions is about 2M bytes,
+ * so 10 secs should be enough.
+ */
+#define FW_LOAD_CACHED_TIMEOUT (10 * HZ)
+
 /**
  * cache_firmware - cache one firmware image in kernel memory space
  * @fw_name: the firmware image name
@@ -1412,7 +1423,11 @@ static int cache_firmware(const char *fw_name)
 
 	pr_debug("%s: %s\n", __func__, fw_name);
 
-	ret = request_firmware(&fw, fw_name, NULL);
+	__module_get(THIS_MODULE);
+	ret = _request_firmware(&fw, fw_name, NULL, NULL, 0,
+				FW_OPT_UEVENT | FW_OPT_FALLBACK,
+				FW_LOAD_CACHED_TIMEOUT);
+	module_put(THIS_MODULE);
 	if (!ret)
 		kfree(fw);
 
@@ -1622,7 +1637,6 @@ static void __device_uncache_fw_images(void)
 static void device_cache_fw_images(void)
 {
 	struct firmware_cache *fwc = &fw_cache;
-	int old_timeout;
 	DEFINE_WAIT(wait);
 
 	pr_debug("%s\n", __func__);
@@ -1630,17 +1644,6 @@ static void device_cache_fw_images(void)
 	/* cancel uncache work */
 	cancel_delayed_work_sync(&fwc->work);
 
-	/*
-	 * use small loading timeout for caching devices' firmware
-	 * because all these firmware images have been loaded
-	 * successfully at lease once, also system is ready for
-	 * completing firmware loading now. The maximum size of
-	 * firmware in current distributions is about 2M bytes,
-	 * so 10 secs should be enough.
-	 */
-	old_timeout = loading_timeout;
-	loading_timeout = 10;
-
 	mutex_lock(&fw_lock);
 	fwc->state = FW_LOADER_START_CACHE;
 	dpm_for_each_dev(NULL, dev_cache_fw_image);
@@ -1648,8 +1651,6 @@ static void device_cache_fw_images(void)
 
 	/* wait for completion of caching firmware for all devices */
 	async_synchronize_full_domain(&fw_cache_domain);
-
-	loading_timeout = old_timeout;
 }
 
 /**

Re: [PATCH v3 1/3] firmware_class: encapsulate firmware loading status

[Daniel Wagner]

On 08/29/2016 11:50 AM, Daniel Wagner wrote:
> On 08/25/2016 07:50 PM, Luis R. Rodriguez wrote:
>>> +#else /* CONFIG_FW_LOADER_USER_HELPER */
>>> +
>>> +static int loading_timeout = 60;
>>> +#define firmware_loading_timeout()		(loading_timeout * HZ)
>>> +
>>> +#define fw_status_wait_timeout(fw_st, long)	0
>>
>> The timeout makes 0 sense for when !CONFIG_FW_LOADER_USER_HELPER so can
>> we do away with adding a silly 60 value to an int here and
>> the silly value of (loading_timeout * HZ) ? Its not used so its not
>> clear to me why this is here.
>
> So the main reason that silly timeout is needed is the usage of
> it in device_cache_fw_images(). I suggest we add a timeout
> argument to  _request_firmware() and use the right timeout value
> at that level.
>
> That allows to move the loading_timeout into the
> CONFIG_FW_LOADER_USER_HELPER section.

I forgot to answer your question. So we have the dependency to 
loading_timeout/firmware_loading_timeout from the firmware caching path. 
The patch added in the previous email removes that dependency.

We still need the 60 second even in the !CONFIG_FW_LOADER_USER_HELPER 
case. I think it would be a regression if we change that value, no?

cheers,
daniel

Re: [PATCH v3 1/3] firmware_class: encapsulate firmware loading status

[Luis R. Rodriguez]

On Mon, Aug 29, 2016 at 04:18:33PM +0200, Daniel Wagner wrote:
> On 08/29/2016 11:50 AM, Daniel Wagner wrote:
> >On 08/25/2016 07:50 PM, Luis R. Rodriguez wrote:
> >>>+#else /* CONFIG_FW_LOADER_USER_HELPER */
> >>>+
> >>>+static int loading_timeout = 60;
> >>>+#define firmware_loading_timeout()		(loading_timeout * HZ)
> >>>+
> >>>+#define fw_status_wait_timeout(fw_st, long)	0
> >>
> >>The timeout makes 0 sense for when !CONFIG_FW_LOADER_USER_HELPER so can
> >>we do away with adding a silly 60 value to an int here and
> >>the silly value of (loading_timeout * HZ) ? Its not used so its not
> >>clear to me why this is here.
> >
> >So the main reason that silly timeout is needed is the usage of
> >it in device_cache_fw_images(). I suggest we add a timeout
> >argument to  _request_firmware() and use the right timeout value
> >at that level.
> >
> >That allows to move the loading_timeout into the
> >CONFIG_FW_LOADER_USER_HELPER section.
> 
> I forgot to answer your question. So we have the dependency to
> loading_timeout/firmware_loading_timeout from the firmware caching
> path. The patch added in the previous email removes that dependency.
> 
> We still need the 60 second even in the
> !CONFIG_FW_LOADER_USER_HELPER case. I think it would be a regression
> if we change that value, no?

Oh that might be the disconnect, see my series of pending patches, I did away
with the cache stuff using the usermode helper, the cache stuff should not use
the usermode helper as the cache stuff kills off the pending usermode helper
requests right before suspend.

  Luis

Re: [PATCH v3 1/3] firmware_class: encapsulate firmware loading status

[Daniel Wagner]

Hi Luis,

On 08/30/2016 09:34 PM, Luis R. Rodriguez wrote:
> On Mon, Aug 29, 2016 at 04:18:33PM +0200, Daniel Wagner wrote:
>> On 08/29/2016 11:50 AM, Daniel Wagner wrote:
>> I forgot to answer your question. So we have the dependency to
>> loading_timeout/firmware_loading_timeout from the firmware caching
>> path. The patch added in the previous email removes that dependency.
>>
>> We still need the 60 second even in the
>> !CONFIG_FW_LOADER_USER_HELPER case. I think it would be a regression
>> if we change that value, no?
>
> Oh that might be the disconnect, see my series of pending patches, I did away
> with the cache stuff using the usermode helper, the cache stuff should not use
> the usermode helper as the cache stuff kills off the pending usermode helper
> requests right before suspend.

The question is how do we proceed from here. I suggest that I don't 
touch the fw cache path in my patches. Basically, leave it as it now. 
After your series in, we cleanup this bit here, maybe even move the user 
helper stuff into its own file.

I think this code is a big interleaved puzzle. Best thing is to split it 
up and figure out what interacts with what. Moving this bits here out is 
definitely the right direction.

cheers,
daniel

Re: [PATCH v3 1/3] firmware_class: encapsulate firmware loading status

[Luis R. Rodriguez]

On Wed, Aug 31, 2016 at 09:13:45AM +0200, Daniel Wagner wrote:
> Hi Luis,
> 
> On 08/30/2016 09:34 PM, Luis R. Rodriguez wrote:
> >On Mon, Aug 29, 2016 at 04:18:33PM +0200, Daniel Wagner wrote:
> >>On 08/29/2016 11:50 AM, Daniel Wagner wrote:
> >>I forgot to answer your question. So we have the dependency to
> >>loading_timeout/firmware_loading_timeout from the firmware caching
> >>path. The patch added in the previous email removes that dependency.
> >>
> >>We still need the 60 second even in the
> >>!CONFIG_FW_LOADER_USER_HELPER case. I think it would be a regression
> >>if we change that value, no?
> >
> >Oh that might be the disconnect, see my series of pending patches, I did away
> >with the cache stuff using the usermode helper, the cache stuff should not use
> >the usermode helper as the cache stuff kills off the pending usermode helper
> >requests right before suspend.
> 
> The question is how do we proceed from here. I suggest that I don't
> touch the fw cache path in my patches. Basically, leave it as it
> now. After your series in, we cleanup this bit here, maybe even move
> the user helper stuff into its own file.
> 
> I think this code is a big interleaved puzzle. Best thing is to
> split it up and figure out what interacts with what. Moving this
> bits here out is definitely the right direction.

How about I remove the timeout crap form the cache stuff in my patch
as you noted and fold then your changes on top of that pending series?

  Luis

[PATCH v3 2/3] firmware_class: Drop bit ops in favor of simple state machine

[Daniel Wagner]

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

We track the state of the loading with bit ops. Since the state machine
has only a couple of states and there are only a few simple state
transition we can model this simplify.

	   UNKNOWN -> LOADING -> DONE | ABORTED

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 | 12 ++++++------
 1 file changed, 6 insertions(+), 6 deletions(-)

diff --git a/drivers/base/firmware_class.c b/drivers/base/firmware_class.c
index f397026..d6cb33a 100644
--- a/drivers/base/firmware_class.c
+++ b/drivers/base/firmware_class.c
@@ -120,16 +120,18 @@ static void fw_status_init(struct fw_status *fw_st)
 
 static int __fw_status_check(struct fw_status *fw_st, unsigned long status)
 {
-	return test_bit(status, &fw_st->status);
+	return fw_st->status == status;
 }
 
 static int fw_status_wait_timeout(struct fw_status *fw_st, long timeout)
 {
+	unsigned long status;
 	int ret;
 
 	ret = wait_for_completion_interruptible_timeout(&fw_st->completion,
 							timeout);
-	if (ret == 0 && test_bit(FW_STATUS_ABORTED, &fw_st->status))
+	status = READ_ONCE(fw_st->status);
+	if (ret == 0 && status == FW_STATUS_ABORTED)
 		return -ENOENT;
 
 	return ret;
@@ -138,13 +140,11 @@ static int fw_status_wait_timeout(struct fw_status *fw_st, long timeout)
 static void __fw_status_set(struct fw_status *fw_st,
 			  unsigned long status)
 {
-	set_bit(status, &fw_st->status);
+	WRITE_ONCE(fw_st->status, status);
 
 	if (status == FW_STATUS_DONE ||
-			status == FW_STATUS_ABORTED) {
-		clear_bit(FW_STATUS_LOADING, &fw_st->status);
+			status == FW_STATUS_ABORTED)
 		complete_all(&fw_st->completion);
-	}
 }
 
 #define fw_status_start(fw_st)					\
-- 
2.7.4

[PATCH v3 3/3] firmware_class: Do not use fw_lock for fw_status protection

[Daniel Wagner]

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

fw_lock is to use to protect 'corner cases' inside firmware_class. It
is not exactly clear what those corner cases are nor what it exactly
protects. fw_status can be used without needing the fw_lock to protect
its state transition and wake ups.

fw_status is holds the state in status and the completion is used to
wake up all waiters (in this case that is the user land helper so only
one). This operation has to be 'atomic' to avoid races.  We can do this
by using swait which takes care we don't miss any wake up.

We use also swait instead of wait because don't need all the additional
features wait provides.

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 | 53 +++++++++++++------------------------------
 1 file changed, 16 insertions(+), 37 deletions(-)

diff --git a/drivers/base/firmware_class.c b/drivers/base/firmware_class.c
index d6cb33a..514aec3 100644
--- a/drivers/base/firmware_class.c
+++ b/drivers/base/firmware_class.c
@@ -30,6 +30,7 @@
 #include <linux/syscore_ops.h>
 #include <linux/reboot.h>
 #include <linux/security.h>
+#include <linux/swait.h>
 
 #include <generated/utsrelease.h>
 
@@ -109,13 +110,13 @@ static inline long firmware_loading_timeout(void)
 
 struct fw_status {
 	unsigned long status;
-	struct completion completion;
+	struct swait_queue_head wq;
 };
 
 static void fw_status_init(struct fw_status *fw_st)
 {
 	fw_st->status = FW_STATUS_UNKNOWN;
-	init_completion(&fw_st->completion);
+	init_swait_queue_head(&fw_st->wq);
 }
 
 static int __fw_status_check(struct fw_status *fw_st, unsigned long status)
@@ -123,15 +124,19 @@ static int __fw_status_check(struct fw_status *fw_st, unsigned long status)
 	return fw_st->status == status;
 }
 
+static inline bool __fw_status_is_done(unsigned long status)
+{
+	return status == FW_STATUS_DONE ||
+	       status == FW_STATUS_ABORTED;
+}
+
 static int fw_status_wait_timeout(struct fw_status *fw_st, long timeout)
 {
-	unsigned long status;
 	int ret;
-
-	ret = wait_for_completion_interruptible_timeout(&fw_st->completion,
-							timeout);
-	status = READ_ONCE(fw_st->status);
-	if (ret == 0 && status == FW_STATUS_ABORTED)
+	ret = swait_event_interruptible_timeout(fw_st->wq,
+				__fw_status_is_done(READ_ONCE(fw_st->status)),
+				timeout);
+	if (ret == 0 && fw_st->status == FW_STATUS_ABORTED)
 		return -ENOENT;
 
 	return ret;
@@ -144,7 +149,7 @@ static void __fw_status_set(struct fw_status *fw_st,
 
 	if (status == FW_STATUS_DONE ||
 			status == FW_STATUS_ABORTED)
-		complete_all(&fw_st->completion);
+		swake_up(&fw_st->wq);
 }
 
 #define fw_status_start(fw_st)					\
@@ -373,14 +378,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);
-	fw_status_done(&buf->fw_st);
-	mutex_unlock(&fw_lock);
-}
-
 static int
 fw_get_filesystem_firmware(struct device *device, struct firmware_buf *buf)
 {
@@ -427,7 +424,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);
@@ -1081,24 +1078,6 @@ static inline void kill_requests_without_uevent(void) { }
 
 #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 (!fw_status_is_done(&buf->fw_st)) {
-		if (fw_status_is_aborted(&buf->fw_st)) {
-			ret = -ENOENT;
-			break;
-		}
-		mutex_unlock(&fw_lock);
-		ret = fw_status_wait_timeout(&buf->fw_st, 0);
-		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,
@@ -1133,7 +1112,7 @@ _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, 0);
 		if (!ret) {
 			fw_set_page_data(buf, firmware);
 			return 0; /* assigned */
-- 
2.7.4