Groups | Search | Server Info | Keyboard shortcuts | Login | Register [http] [https] [nntp] [nntps]
Groups > linux.kernel > #1688010 > unrolled thread
| Started by | "Luis R. Rodriguez" <mcgrof@kernel.org> |
|---|---|
| First post | 2017-07-15 23:20 +0200 |
| Last post | 2017-07-15 23:30 +0200 |
| Articles | 3 — 1 participant |
Back to article view | Back to linux.kernel
[PATCH 0/4] firmware: few more fixes - fix batched requests "Luis R. Rodriguez" <mcgrof@kernel.org> - 2017-07-15 23:20 +0200
[PATCH 4/4] firmware: fix batched requests: send wake up on failure on direct lookups "Luis R. Rodriguez" <mcgrof@kernel.org> - 2017-07-15 23:20 +0200
[PATCH 1/4] firmware: define pr_fmt "Luis R. Rodriguez" <mcgrof@kernel.org> - 2017-07-15 23:30 +0200
| From | "Luis R. Rodriguez" <mcgrof@kernel.org> |
|---|---|
| Date | 2017-07-15 23:20 +0200 |
| Subject | [PATCH 0/4] firmware: few more fixes - fix batched requests |
| Message-ID | <u3CDL-6wr-7@gated-at.bofh.it> |
Greg,
As I hinted as I worked on the last firmware fixes, the batched requests
mechanism of the firmware API still had one remaining issue which has been
broken for a long time. This series fixes the last issue I am aware of.
I've written test cases for it using a smiller test interface which we
can later port the other tests for it. I haven't moved the old tests to
it given a) kselftests are being tested on older kernels with older
test_firmware drivers so moving the tests would nullify the old tests. We
could later address this by checking for both triggers for compatibility
purposes but we would also have to test for everything we tested for the
old tests. The new tests only first prioritize batched requets for now.
With time we could mesh them all together and still remain compatible with
the old test_firmware on older kernels.
I've pushed testsing on all kernel configuration possible and provide
report of the issue on all configurations as well, before and after for
all the APIs.
This series depends on the last series of fixes I had posted before. As usual
I have a tree with this and all other pending fixes on my linux-next tree on the
20170714-fw-fix-batched-req branch [0] based on linux-next tag next-20170714.
If there any questions please let me know.
[0] https://git.kernel.org/pub/scm/linux/kernel/git/mcgrof/linux-next.git/log/?h=20170714-fw-fix-batched-req
Luis R. Rodriguez (4):
firmware: define pr_fmt
firwmare: enable a debug print for batched requests
test_firmware: add batched firmware tests
firmware: fix batched requests: send wake up on failure on direct
lookups
drivers/base/firmware_class.c | 42 +-
lib/test_firmware.c | 594 ++++++++++++++++++++++
tools/testing/selftests/firmware/fw_filesystem.sh | 105 +++-
3 files changed, 731 insertions(+), 10 deletions(-)
--
2.11.0
[toc] | [next] | [standalone]
| From | "Luis R. Rodriguez" <mcgrof@kernel.org> |
|---|---|
| Date | 2017-07-15 23:20 +0200 |
| Subject | [PATCH 4/4] firmware: fix batched requests: send wake up on failure on direct lookups |
| Message-ID | <u3CDM-6wr-13@gated-at.bofh.it> |
| In reply to | #1688010 |
Fix batched requests from waiting forever on failure.
The firmware API batched requests feature has been broken since the API call
request_firmware_direct() was introduced on commit bba3a87e982ad ("firmware:
Introduce request_firmware_direct()"), added on v3.14 *iff* the firmware
being requested was not present in *certail kernel builds* [0].
When no firmware is found the worker which goes on to finish never informs
waiters queued up of this, so any batched request will stall in what seems
to be forever (MAX_SCHEDULE_TIMEOUT). Sadly, a reboot will also stall, as
the reboot notifier was only designed to kill custom fallback workers. The
issue seems to the user as a type of soft lockup, what *actually* happens
underneath the hood is a wait call which never completes as we failed to
issue a completion on error.
For device drivers with optional firmware schemes (ie, Intel iwlwifi, or
Netronome -- even though it uses request_firmware() and not
request_firmware_direct()), this could mean that when you boot a system with
multiple cards the firmware will seem to never load on the system, or that
the card is just not responsive even the driver initialization. Due to
differences in scheduling possible this should not always trigger --
one would need to to ensure that multiple requests are in place at the
right time for this to work, also release_firmware() must not be called
prior to any other incoming request. The complexity may not be worth
supporting batched requests in the future given the wait mechanism is
only used also for the fallback mechanism. We'll keep it for now and
just fix it.
Its reported that at least with the Intel WiFi cards on one system this
issue was creeping up 50% of the boots [0].
Ater this commit batched testing results:
============================================================================
CONFIG_FW_LOADER_USER_HELPER_FALLBACK=n
CONFIG_FW_LOADER_USER_HELPER=y
Most common Linux distribution setup.
API-type no-firmware-found firmware-found
----------------------------------------------------------------------
request_firmware() OK OK
request_firmware_direct() OK OK
request_firmware_nowait(uevent=true) OK OK
request_firmware_nowait(uevent=false) OK OK
============================================================================
CONFIG_FW_LOADER_USER_HELPER_FALLBACK=n
CONFIG_FW_LOADER_USER_HELPER=n
Only possible if CONFIG_DELL_RBU=n and CONFIG_LEDS_LP55XX_COMMON=n, rare.
API-type no-firmware-found firmware-found
----------------------------------------------------------------------
request_firmware() OK OK
request_firmware_direct() OK OK
request_firmware_nowait(uevent=true) OK OK
request_firmware_nowait(uevent=false) OK OK
============================================================================
CONFIG_FW_LOADER_USER_HELPER_FALLBACK=y
CONFIG_FW_LOADER_USER_HELPER=y
Google Android setup.
API-type no-firmware-found firmware-found
----------------------------------------------------------------------
request_firmware() OK OK
request_firmware_direct() OK OK
request_firmware_nowait(uevent=true) OK OK
request_firmware_nowait(uevent=false) OK OK
============================================================================
[0] https://bugzilla.kernel.org/show_bug.cgi?id=195477
Cc: stable <stable@vger.kernel.org> # v3.14
Fixes: bba3a87e982ad ("firmware: Introduce request_firmware_direct()"
Reported-by: Nicolas <nbroeking@me.com>
Reported-by: John Ewalt <jewalt@lgsinnovations.com>
Reported-by: Jakub Kicinski <jakub.kicinski@netronome.com>
Signed-off-by: Luis R. Rodriguez <mcgrof@kernel.org>
---
drivers/base/firmware_class.c | 38 ++++++++++++++++++++++++++++++--------
1 file changed, 30 insertions(+), 8 deletions(-)
diff --git a/drivers/base/firmware_class.c b/drivers/base/firmware_class.c
index a85dabeb1e20..a5fb884a136d 100644
--- a/drivers/base/firmware_class.c
+++ b/drivers/base/firmware_class.c
@@ -154,28 +154,27 @@ static void __fw_state_set(struct fw_state *fw_st,
__fw_state_set(fw_st, FW_STATUS_LOADING)
#define fw_state_done(fw_st) \
__fw_state_set(fw_st, FW_STATUS_DONE)
+#define fw_state_aborted(fw_st) \
+ __fw_state_set(fw_st, FW_STATUS_ABORTED)
#define fw_state_wait(fw_st) \
__fw_state_wait_common(fw_st, MAX_SCHEDULE_TIMEOUT)
-#ifndef CONFIG_FW_LOADER_USER_HELPER
-
-#define fw_state_is_aborted(fw_st) false
-
-#else /* CONFIG_FW_LOADER_USER_HELPER */
-
static int __fw_state_check(struct fw_state *fw_st, enum fw_status status)
{
return fw_st->status == status;
}
+#define fw_state_is_aborted(fw_st) \
+ __fw_state_check(fw_st, FW_STATUS_ABORTED)
+
+#ifdef CONFIG_FW_LOADER_USER_HELPER
+
#define fw_state_aborted(fw_st) \
__fw_state_set(fw_st, FW_STATUS_ABORTED)
#define fw_state_is_done(fw_st) \
__fw_state_check(fw_st, FW_STATUS_DONE)
#define fw_state_is_loading(fw_st) \
__fw_state_check(fw_st, FW_STATUS_LOADING)
-#define fw_state_is_aborted(fw_st) \
- __fw_state_check(fw_st, FW_STATUS_ABORTED)
#define fw_state_wait_timeout(fw_st, timeout) \
__fw_state_wait_common(fw_st, timeout)
@@ -1204,6 +1203,28 @@ _request_firmware_prepare(struct firmware **firmware_p, const char *name,
return 1; /* need to load */
}
+/*
+ * Batched requests need only one wake, we need to do this step last due to the
+ * fallback mechanism. The buf is protected with kref_get(), and it won't be
+ * released until the last user calls release_firmware().
+ *
+ * Failed batched requests are possible as well, in such cases we just share
+ * the struct firmware_buf and won't release it until all requests are woken
+ * and have gone through this same path.
+ */
+static void fw_abort_batch_reqs(struct firmware *fw)
+{
+ struct firmware_buf *buf;
+
+ /* Loaded directly? */
+ if (!fw || !fw->priv)
+ return;
+
+ buf = fw->priv;
+ if (!fw_state_is_aborted(&buf->fw_st))
+ fw_state_aborted(&buf->fw_st);
+}
+
/* called from request_firmware() and request_firmware_work_func() */
static int
_request_firmware(const struct firmware **firmware_p, const char *name,
@@ -1247,6 +1268,7 @@ _request_firmware(const struct firmware **firmware_p, const char *name,
out:
if (ret < 0) {
+ fw_abort_batch_reqs(fw);
release_firmware(fw);
fw = NULL;
}
--
2.11.0
[toc] | [prev] | [next] | [standalone]
| From | "Luis R. Rodriguez" <mcgrof@kernel.org> |
|---|---|
| Date | 2017-07-15 23:30 +0200 |
| Subject | [PATCH 1/4] firmware: define pr_fmt |
| Message-ID | <u3CNr-6zC-3@gated-at.bofh.it> |
| In reply to | #1688010 |
For some reason we have always forgotten this. Without this we don't get a nice prefix on our pr_debug() / pr_*() messages. Signed-off-by: Luis R. Rodriguez <mcgrof@kernel.org> --- drivers/base/firmware_class.c | 2 ++ 1 file changed, 2 insertions(+) diff --git a/drivers/base/firmware_class.c b/drivers/base/firmware_class.c index d3d071dbd2a5..6f58a15efa2b 100644 --- a/drivers/base/firmware_class.c +++ b/drivers/base/firmware_class.c @@ -7,6 +7,8 @@ * */ +#define pr_fmt(fmt) KBUILD_MODNAME ": " fmt + #include <linux/capability.h> #include <linux/device.h> #include <linux/module.h> -- 2.11.0
[toc] | [prev] | [standalone]
Back to top | Article view | linux.kernel
csiph-web