Groups | Search | Server Info | Keyboard shortcuts | Login | Register [http] [https] [nntp] [nntps]

Groups > linux.kernel > #1470025 > unrolled thread

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

Back to article view | Back to linux.kernel

This discussion starts older than the indexed window; earlier articles aren't shown. The article labeled Started by below is the oldest one visible, not the original post.

Contents

  [PATCH v3 1/3] firmware_class: encapsulate firmware loading status Daniel Wagner <wagi@monom.org> - 2016-08-25 12:00 +0200
    Re: [PATCH v3 1/3] firmware_class: encapsulate firmware loading  status "Luis R. Rodriguez" <mcgrof@kernel.org> - 2016-08-25 20:00 +0200
      Re: [PATCH v3 1/3] firmware_class: encapsulate firmware loading  status Daniel Wagner <daniel.wagner@bmw-carit.de> - 2016-08-29 12:00 +0200
        Re: [PATCH v3 1/3] firmware_class: encapsulate firmware loading  status Daniel Wagner <daniel.wagner@bmw-carit.de> - 2016-08-29 16:20 +0200
          Re: [PATCH v3 1/3] firmware_class: encapsulate firmware loading  status "Luis R. Rodriguez" <mcgrof@kernel.org> - 2016-08-30 21:40 +0200
            Re: [PATCH v3 1/3] firmware_class: encapsulate firmware loading  status Daniel Wagner <wagi@monom.org> - 2016-08-31 09:20 +0200
              Re: [PATCH v3 1/3] firmware_class: encapsulate firmware loading  status "Luis R. Rodriguez" <mcgrof@kernel.org> - 2016-09-07 02:40 +0200

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

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

[toc] | [next] | [standalone]

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

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

[toc] | [prev] | [next] | [standalone]

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

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;
 }
 
 /**

[toc] | [prev] | [next] | [standalone]

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

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

[toc] | [prev] | [next] | [standalone]

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

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

[toc] | [prev] | [next] | [standalone]

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

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

[toc] | [prev] | [next] | [standalone]

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

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

[toc] | [prev] | [standalone]

Back to top | Article view | linux.kernel

csiph-web