Groups | Search | Server Info | Keyboard shortcuts | Login | Register [http] [https] [nntp] [nntps]
Groups > linux.kernel > #1468412 > unrolled thread
| Started by | Daniel Wagner <wagi@monom.org> |
|---|---|
| First post | 2016-08-23 11:10 +0200 |
| Last post | 2016-08-24 02:00 +0200 |
| Articles | 2 — 2 participants |
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.
[PATCH v2 1/2] firmware_class: encapsulate firmware loading status Daniel Wagner <wagi@monom.org> - 2016-08-23 11:10 +0200
Re: [PATCH v2 1/2] firmware_class: encapsulate firmware loading status "Luis R. Rodriguez" <mcgrof@kernel.org> - 2016-08-24 02:00 +0200
| From | Daniel Wagner <wagi@monom.org> |
|---|---|
| Date | 2016-08-23 11:10 +0200 |
| Subject | [PATCH v2 1/2] firmware_class: encapsulate firmware loading status |
| Message-ID | <s9fSx-1R4-7@gated-at.bofh.it> |
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
[toc] | [next] | [standalone]
| From | "Luis R. Rodriguez" <mcgrof@kernel.org> |
|---|---|
| Date | 2016-08-24 02:00 +0200 |
| Subject | Re: [PATCH v2 1/2] firmware_class: encapsulate firmware loading status |
| Message-ID | <s9tLQ-2xb-7@gated-at.bofh.it> |
| In reply to | #1468412 |
On Tue, Aug 23, 2016 at 11:00:19AM +0200, Daniel Wagner wrote:
> 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.
You can specify the members, the unsigned long status and the completion.
> Let's
> encapsulate this simple state machine into struct fw_status.
Please rephrase instead to:
"We only need this for the usermode helper as such we can encapsulate
all this data into its own data structure."
> We don't need the fw_lock to protect the state machine anymore.
You can the instead follow up explaining, "Doing this helps us compartamentalize
the usermode helper code further."
Can you cut that out into a separate patch?
Then another patch can be: "The usermode helper code only needs access to a
status variable, no RWM operations are done on it, so we can replace the
use of the global lock with READ_ONCE() and WRITE_ONCE()."
> 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;
> +}
All this code is being kept when CONFIG_FW_LOADER_USER_HELPER is not enabled, that
seems odd given firmware_loading_timeout() is used when CONFIG_FW_LOADER_USER_HELPER
is disabled in two places, in one case its not needed at all, in the second its in this
piece of code:
retval = fw_status_wait_timeout(&buf->fw_st, timeout);
if (retval == -ERESTARTSYS || !retval) {
...
}
I'd rather instead have a wrapper for a user mode helper timeout check and
have that return 0 for when CONFIG_FW_LOADER_USER_HELPER is not selected.
This way the global variable loading_timeout and anything that accesses it
is just left inside CONFIG_FW_LOADER_USER_HELPER.
> +
> +#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)
You don't need this, in fact if you had it as-is and the code for when
CONFIG_FW_LOADER_USER_HELPER is disabled would use it you may end up in
a compile error.
> +#define fw_status_done(fw_st)
You need this for both.
> +#define fw_status_aborted(fw_st)
> +#define fw_status_is_loading(fw_st)
> +#define fw_status_is_done(fw_st)
All these are no longer needed for when CONFIG_FW_LOADER_USER_HELPER is
disabled.
> +#define fw_status_is_aborted(fw_st) false
This is needed for both.
Given all this it seems to be best to rebrand these helpers as usermode helper
related. In fact the next step is to see if this looks best to just stuffing all
this user mode helper code into its own C file. But we can do that later.
Other than that looks good!
Luis
[toc] | [prev] | [standalone]
Back to top | Article view | linux.kernel
csiph-web