by Luis Chamberlain

[permalink] [raw]

Subject: [PATCH v3 06/20] firmware: use helpers for setting up a temporary cache timeout

We only use the timeout for the firmware fallback mechanism
except for trying to set the timeout during the cache setup
for resume/suspend. For those cases, setting the timeout should
be a no-op, so just reflect this in code by adding helpers for it.

This change introduces no functional changes.

Acked-by: Kees Cook <[email protected]>
Signed-off-by: Luis R. Rodriguez <[email protected]>
---
drivers/base/firmware_loader.c | 49 ++++++++++++++++++++++++++----------------
1 file changed, 30 insertions(+), 19 deletions(-)

diff --git a/drivers/base/firmware_loader.c b/drivers/base/firmware_loader.c
index 59dba794ce1a..2d819875348d 100644
--- a/drivers/base/firmware_loader.c
+++ b/drivers/base/firmware_loader.c
@@ -191,13 +191,6 @@ 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;
-}
-
static void fw_state_init(struct fw_priv *fw_priv)
{
struct fw_state *fw_st = &fw_priv->fw_st;
@@ -282,6 +275,32 @@ static const struct firmware_fallback_config fw_fallback_config = {
.force_sysfs_fallback = IS_ENABLED(CONFIG_FW_LOADER_USER_HELPER_FALLBACK),
};

+static int old_timeout;
+static int loading_timeout = 60; /* In seconds */
+
+static inline long firmware_loading_timeout(void)
+{
+ return loading_timeout > 0 ? loading_timeout * HZ : MAX_JIFFY_OFFSET;
+}
+
+/*
+ * 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.
+ */
+static void fw_fallback_set_cache_timeout(void)
+{
+ old_timeout = loading_timeout;
+ loading_timeout = 10;
+}
+
+/* Restores the timeout to the value last configured during normal operation */
+static void fw_fallback_set_default_timeout(void)
+{
+ loading_timeout = old_timeout;
+}
+
static inline bool fw_sysfs_done(struct fw_priv *fw_priv)
{
return __fw_state_check(fw_priv, FW_STATUS_DONE);
@@ -1206,6 +1225,8 @@ static int fw_sysfs_fallback(struct firmware *fw, const char *name,
}

static inline void kill_pending_fw_fallback_reqs(bool only_kill_custom) { }
+static inline void fw_fallback_set_cache_timeout(void) { }
+static inline void fw_fallback_set_default_timeout(void) { }

static inline int register_sysfs_loader(void)
{
@@ -1752,7 +1773,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__);
@@ -1760,16 +1780,7 @@ 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;
+ fw_fallback_set_cache_timeout();

mutex_lock(&fw_lock);
fwc->state = FW_LOADER_START_CACHE;
@@ -1779,7 +1790,7 @@ 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;
+ fw_fallback_set_default_timeout();
}

/**
--
2.16.2

2018-03-20 18:54:51

by Konstantin Ryabitsev

[permalink] [raw]

Subject: Re: [PATCH v3 19/20] firmware: add request_firmware_cache() to help with cache on reboot

On 03/20/18 14:24, Luis R. Rodriguez wrote:
> *Iff* this seems sensible, this would only mean kernel.org would have to start
> accepting git notes long term, for those who optionally want to push them or
> fetch them.

We don't disallow them. You just need to make sure you're fetching and
pushing them, because they aren't by default. You can set this in your
kernel.org origin section of .git/config:

[remote origin]
...
fetch = +refs/notes/*:refs/notes/*

And then push them separately as "git push origin refs/notes/*".

Since frontends clone with --mirror, the notes will be available on all
git.kernel.org nodes (e.g. see
https://git.kernel.org/mricon/hook-test/c/a8d310d4c13)

If you do start using notes, I strongly suggest you pick a dedicated
refspace for it instead of putting things into the default
refs/notes/commits, e.g.:

git notes --ref crosstree [...]

This will create refs/notes/crosstree that has less of a chance to be
clobbered by someone else's use of notes.

Best,
--
Konstantin Ryabitsev
Director, IT Infrastructure Security
The Linux Foundation

Attachments:

signature.asc (228.00 B)
OpenPGP digital signature

2018-03-10 14:15:34

by Luis Chamberlain

[permalink] [raw]

Subject: [PATCH v3 17/20] test_firmware: modify custom fallback tests to use unique files

Users of the custom firmware fallback interface is are not supposed to
use the firmware cache interface, this can happen if for instance the
one of the APIs which use the firmware cache is used first with one
firmware file and then the request_firmware_nowait(uevent=false) API
is used with the same file.

We'll soon become strict about this on the firmware interface to reject
such calls later, so correct the test scripts to avoid such uses as well.
We address this on the tests scripts by simply using unique names when
testing the custom fallback interface.

Signed-off-by: Luis R. Rodriguez <[email protected]>
---
tools/testing/selftests/firmware/fw_fallback.sh | 20 ++++++++++++++------
tools/testing/selftests/firmware/fw_filesystem.sh | 11 +++++++++--
tools/testing/selftests/firmware/fw_lib.sh | 23 +++++++++++++++++++++++
3 files changed, 46 insertions(+), 8 deletions(-)

diff --git a/tools/testing/selftests/firmware/fw_fallback.sh b/tools/testing/selftests/firmware/fw_fallback.sh
index 9337a0328627..8e2e34a2ca69 100755
--- a/tools/testing/selftests/firmware/fw_fallback.sh
+++ b/tools/testing/selftests/firmware/fw_fallback.sh
@@ -238,8 +238,10 @@ run_sysfs_main_tests()

run_sysfs_custom_load_tests()
{
- if load_fw_custom "$NAME" "$FW" ; then
- if ! diff -q "$FW" /dev/test_firmware >/dev/null ; then
+ RANDOM_FILE_PATH=$(setup_random_file)
+ RANDOM_FILE="$(basename $RANDOM_FILE_PATH)"
+ if load_fw_custom "$RANDOM_FILE" "$RANDOM_FILE_PATH" ; then
+ if ! diff -q "$RANDOM_FILE_PATH" /dev/test_firmware >/dev/null ; then
echo "$0: firmware was not loaded" >&2
exit 1
else
@@ -247,8 +249,10 @@ run_sysfs_custom_load_tests()
fi
fi

- if load_fw_custom "$NAME" "$FW" ; then
- if ! diff -q "$FW" /dev/test_firmware >/dev/null ; then
+ RANDOM_FILE_PATH=$(setup_random_file)
+ RANDOM_FILE="$(basename $RANDOM_FILE_PATH)"
+ if load_fw_custom "$RANDOM_FILE" "$RANDOM_FILE_PATH" ; then
+ if ! diff -q "$RANDOM_FILE_PATH" /dev/test_firmware >/dev/null ; then
echo "$0: firmware was not loaded" >&2
exit 1
else
@@ -256,8 +260,12 @@ run_sysfs_custom_load_tests()
fi
fi

- if load_fw_custom_cancel "nope-$NAME" "$FW" ; then
- if diff -q "$FW" /dev/test_firmware >/dev/null ; then
+ RANDOM_FILE_REAL="$RANDOM_FILE_PATH"
+ FAKE_RANDOM_FILE_PATH=$(setup_random_file_fake)
+ FAKE_RANDOM_FILE="$(basename $FAKE_RANDOM_FILE_PATH)"
+
+ if load_fw_custom_cancel "$FAKE_RANDOM_FILE" "$RANDOM_FILE_REAL" ; then
+ if diff -q "$RANDOM_FILE_PATH" /dev/test_firmware >/dev/null ; then
echo "$0: firmware was expected to be cancelled" >&2
exit 1
else
diff --git a/tools/testing/selftests/firmware/fw_filesystem.sh b/tools/testing/selftests/firmware/fw_filesystem.sh
index 7f47877fa7fa..6452d2129cd9 100755
--- a/tools/testing/selftests/firmware/fw_filesystem.sh
+++ b/tools/testing/selftests/firmware/fw_filesystem.sh
@@ -230,10 +230,13 @@ test_wait_and_cancel_custom_load()
test_request_firmware_nowait_custom_nofile()
{
echo -n "Batched request_firmware_nowait(uevent=false) nofile try #$1: "
+ config_reset
config_unset_uevent
- config_set_name nope-test-firmware.bin
+ RANDOM_FILE_PATH=$(setup_random_file_fake)
+ RANDOM_FILE="$(basename $RANDOM_FILE_PATH)"
+ config_set_name $RANDOM_FILE
config_trigger_async &
- test_wait_and_cancel_custom_load nope-test-firmware.bin
+ test_wait_and_cancel_custom_load $RANDOM_FILE
wait
release_all_firmware
echo "OK"
@@ -271,7 +274,11 @@ test_request_firmware_nowait_uevent()
test_request_firmware_nowait_custom()
{
echo -n "Batched request_firmware_nowait(uevent=false) try #$1: "
+ config_reset
config_unset_uevent
+ RANDOM_FILE_PATH=$(setup_random_file)
+ RANDOM_FILE="$(basename $RANDOM_FILE_PATH)"
+ config_set_name $RANDOM_FILE
config_trigger_async
release_all_firmware
echo "OK"
diff --git a/tools/testing/selftests/firmware/fw_lib.sh b/tools/testing/selftests/firmware/fw_lib.sh
index 98dceb847ba0..9ea31b57d71a 100755
--- a/tools/testing/selftests/firmware/fw_lib.sh
+++ b/tools/testing/selftests/firmware/fw_lib.sh
@@ -104,6 +104,29 @@ setup_tmp_file()
fi
}

+__setup_random_file()
+{
+ RANDOM_FILE_PATH="$(mktemp -p $FWPATH)"
+ # mktemp says dry-run -n is unsafe, so...
+ if [[ "$1" = "fake" ]]; then
+ rm -rf $RANDOM_FILE_PATH
+ sync
+ else
+ echo "ABCD0123" >"$RANDOM_FILE_PATH"
+ fi
+ echo $RANDOM_FILE_PATH
+}
+
+setup_random_file()
+{
+ echo $(__setup_random_file)
+}
+
+setup_random_file_fake()
+{
+ echo $(__setup_random_file fake)
+}
+
proc_set_force_sysfs_fallback()
{
if [ -f $FW_FORCE_SYSFS_FALLBACK ]; then
--
2.16.2