From: Yi Li <[email protected]>
Changes in v3:
- Add a platform device to enable PM support in test driver
- Add a debugfs device to enable driver data cache testing
- Fix a logic error so driver_data can be able to cache
- Add two test cases for caching and non-caching
- Rebase to Luis R. Rodriguez's 20170605-driver-data branch
Changes in v2:
- Rebase to Luis R. Rodriguez's 20170501-driver-data-try2
branch
- Expose DRIVER_DATA_REQ_NO_CACHE flag to public
driver_data_req_params structure, so upper drivers can ask
driver_data driver to bypass the internal caching mechanism.
This will be used for streaming and other drivers maintains
their own caching like iwlwifi.
- Add self test cases.
Yi Li (3):
firmware_class: move NO_CACHE from private to driver_data_req_params
iwlwifi: use DRIVER_DATA_REQ_NO_CACHE for driver_data
test: add no_cache to driver_data load tester
drivers/base/firmware_class-dbg.c | 108 ++++++++++++++++++++++++
drivers/base/firmware_class.c | 26 +++---
drivers/net/wireless/intel/iwlwifi/iwl-drv.c | 2 +
include/linux/driver_data.h | 4 +
lib/test_driver_data.c | 97 ++++++++++++++++++---
tools/testing/selftests/firmware/driver_data.sh | 69 +++++++++++++++
6 files changed, 285 insertions(+), 21 deletions(-)
create mode 100644 drivers/base/firmware_class-dbg.c
--
2.7.4
From: Yi Li <[email protected]>
Add a platform device to enable PM cache and add cache/no_cache test.
Signed-off-by: Yi Li <[email protected]>
---
lib/test_driver_data.c | 97 ++++++++++++++++++++++---
tools/testing/selftests/firmware/driver_data.sh | 67 +++++++++++++++++
2 files changed, 154 insertions(+), 10 deletions(-)
diff --git a/lib/test_driver_data.c b/lib/test_driver_data.c
index c176527..cadd122 100644
--- a/lib/test_driver_data.c
+++ b/lib/test_driver_data.c
@@ -44,6 +44,8 @@
#include <linux/async.h>
#include <linux/delay.h>
#include <linux/vmalloc.h>
+#include <linux/pm.h>
+#include <linux/platform_device.h>
/* Used for the fallback default to test against */
#define TEST_DRIVER_DATA "test-driver_data.bin"
@@ -73,6 +75,10 @@ int num_test_devs;
* struct driver_data_reg_params @optional field for more information.
* @keep: whether or not we wish to free the driver_data on our own, refer to
* the struct driver_data_req_params @keep field for more information.
+ * @no_cache: whether or not we wish to use the internal caching mechanism
+ * to assist drivers from fetching driver data at resume time after
+ * suspend, refer to the struct driver_data_req_params .req
+ * DRIVER_DATA_REQ_NO_CACHE for more information.
* @enable_opt_cb: whether or not the optional callback should be set
* on a trigger. There is no equivalent setting on the struct
* driver_data_req_params as this is implementation specific, and in
@@ -121,6 +127,7 @@ struct test_config {
bool async;
bool optional;
bool keep;
+ bool no_cache;
bool enable_opt_cb;
bool use_api_versioning;
u8 api_min;
@@ -163,6 +170,7 @@ struct test_driver_data_private {
* a driver might typically use to stuff firmware / driver_data.
* @misc_dev: we use a misc device under the hood
* @dev: pointer to misc_dev's own struct device
+ * @pdev: pointer to platform device's struct device
* @api_found_calls: number of calls a fetch for a driver was found. We use
* for internal use on the api callback.
* @driver_data_mutex: for access into the @driver_data, the fake storage
@@ -181,6 +189,7 @@ struct driver_data_test_device {
struct test_driver_data_private test_driver_data;
struct miscdevice misc_dev;
struct device *dev;
+ struct device *pdev;
u8 api_found_calls;
@@ -346,6 +355,9 @@ static ssize_t config_show(struct device *dev,
len += snprintf(buf+len, PAGE_SIZE,
"keep:\t\t%s\n",
config->keep ? "true" : "false");
+ len += snprintf(buf + len, PAGE_SIZE,
+ "no_cache:\t\t%s\n",
+ config->no_cache ? "true" : "false");
mutex_unlock(&test_dev->config_mutex);
@@ -399,9 +411,9 @@ static int config_req_default(struct driver_data_test_device *test_dev)
config->default_name);
ret = driver_data_request_sync(config->default_name,
- &req_params, test_dev->dev);
+ &req_params, test_dev->pdev);
if (ret)
- dev_info(test_dev->dev,
+ dev_info(test_dev->pdev,
"load of default '%s' failed: %d\n",
config->default_name, ret);
@@ -456,14 +468,17 @@ static int trigger_config_sync(struct driver_data_test_device *test_dev)
(config->optional ?
DRIVER_DATA_REQ_OPTIONAL : 0) |
(config->keep ?
- DRIVER_DATA_REQ_KEEP : 0)),
+ DRIVER_DATA_REQ_KEEP : 0) |
+ (config->no_cache ?
+ DRIVER_DATA_REQ_NO_CACHE : 0)),
};
const struct driver_data_req_params req_params_opt_cb = {
DRIVER_DATA_DEFAULT_SYNC(config_sync_req_cb, test_dev),
DRIVER_DATA_SYNC_OPT_CB(config_sync_req_default_cb,
test_dev),
.reqs = (config->optional ? DRIVER_DATA_REQ_OPTIONAL : 0) |
- (config->keep ? DRIVER_DATA_REQ_KEEP : 0),
+ (config->keep ? DRIVER_DATA_REQ_KEEP : 0) |
+ (config->no_cache ? DRIVER_DATA_REQ_NO_CACHE : 0),
};
const struct driver_data_req_params *req_params;
@@ -472,9 +487,10 @@ static int trigger_config_sync(struct driver_data_test_device *test_dev)
else
req_params = &req_params_default;
- ret = driver_data_request_sync(config->name, req_params, test_dev->dev);
+ ret = driver_data_request_sync(config->name, req_params,
+ test_dev->pdev);
if (ret)
- dev_err(test_dev->dev, "sync load of '%s' failed: %d\n",
+ dev_err(test_dev->pdev, "sync load of '%s' failed: %d\n",
config->name, ret);
return ret;
@@ -529,19 +545,22 @@ static int trigger_config_async(struct driver_data_test_device *test_dev)
const struct driver_data_req_params req_params_default = {
DRIVER_DATA_DEFAULT_ASYNC(config_async_req_cb, test_dev),
.reqs = (config->optional ? DRIVER_DATA_REQ_OPTIONAL : 0) |
- (config->keep ? DRIVER_DATA_REQ_KEEP : 0),
+ (config->keep ? DRIVER_DATA_REQ_KEEP : 0) |
+ (config->no_cache ? DRIVER_DATA_REQ_NO_CACHE : 0),
};
const struct driver_data_req_params req_params_opt_cb = {
DRIVER_DATA_DEFAULT_ASYNC(config_async_req_cb, test_dev),
DRIVER_DATA_ASYNC_OPT_CB(config_async_req_default_cb, test_dev),
.reqs = (config->optional ? DRIVER_DATA_REQ_OPTIONAL : 0) |
- (config->keep ? DRIVER_DATA_REQ_KEEP : 0),
+ (config->keep ? DRIVER_DATA_REQ_KEEP : 0) |
+ (config->no_cache ? DRIVER_DATA_REQ_NO_CACHE : 0),
};
const struct driver_data_req_params req_params_api = {
DRIVER_DATA_API_CB(config_async_req_api_cb, test_dev),
DRIVER_DATA_API(config->api_min, config->api_max, config->api_name_postfix),
.reqs = (config->optional ? DRIVER_DATA_REQ_OPTIONAL : 0) |
(config->keep ? DRIVER_DATA_REQ_KEEP : 0) |
+ (config->no_cache ? DRIVER_DATA_REQ_NO_CACHE : 0) |
(config->use_api_versioning ? DRIVER_DATA_REQ_USE_API_VERSIONING : 0),
};
const struct driver_data_req_params *req_params;
@@ -555,9 +574,9 @@ static int trigger_config_async(struct driver_data_test_device *test_dev)
test_dev->api_found_calls = 0;
ret = driver_data_request_async(config->name, req_params,
- test_dev->dev);
+ test_dev->pdev);
if (ret) {
- dev_err(test_dev->dev, "async load of '%s' failed: %d\n",
+ dev_err(test_dev->pdev, "async load of '%s' failed: %d\n",
config->name, ret);
goto out;
}
@@ -667,6 +686,7 @@ static int __driver_data_config_init(struct test_config *config)
config->async = false;
config->optional = false;
config->keep = false;
+ config->no_cache = false;
config->enable_opt_cb = false;
config->use_api_versioning = false;
config->api_min = 0;
@@ -986,6 +1006,29 @@ static ssize_t config_keep_show(struct device *dev,
}
static DEVICE_ATTR(config_keep, 0644, config_keep_show, config_keep_store);
+static ssize_t config_no_cache_store(struct device *dev,
+ struct device_attribute *attr,
+ const char *buf, size_t count)
+{
+ struct driver_data_test_device *test_dev = dev_to_test_dev(dev);
+ struct test_config *config = &test_dev->config;
+
+ return test_dev_config_update_bool(test_dev, buf, count,
+ &config->no_cache);
+}
+
+static ssize_t config_no_cache_show(struct device *dev,
+ struct device_attribute *attr,
+ char *buf)
+{
+ struct driver_data_test_device *test_dev = dev_to_test_dev(dev);
+ struct test_config *config = &test_dev->config;
+
+ return test_dev_config_show_bool(test_dev, buf, config->no_cache);
+}
+static DEVICE_ATTR(config_no_cache, 0644, config_no_cache_show,
+ config_no_cache_store);
+
static ssize_t config_enable_opt_cb_store(struct device *dev,
struct device_attribute *attr,
const char *buf, size_t count)
@@ -1143,6 +1186,7 @@ static struct attribute *test_dev_attrs[] = {
TEST_DRIVER_DATA_DEV_ATTR(config_async),
TEST_DRIVER_DATA_DEV_ATTR(config_optional),
TEST_DRIVER_DATA_DEV_ATTR(config_keep),
+ TEST_DRIVER_DATA_DEV_ATTR(config_no_cache),
TEST_DRIVER_DATA_DEV_ATTR(config_use_api_versioning),
TEST_DRIVER_DATA_DEV_ATTR(config_enable_opt_cb),
TEST_DRIVER_DATA_DEV_ATTR(config_api_min),
@@ -1163,10 +1207,32 @@ void free_test_dev_driver_data(struct driver_data_test_device *test_dev)
vfree(test_dev);
}
+static struct platform_device *driver_data_pdev;
+static int driver_data_test_suspend(struct device *dev)
+{
+ return 0;
+}
+
+static int driver_data_test_resume(struct device *dev)
+{
+ return 0;
+}
+
+static SIMPLE_DEV_PM_OPS(driver_test_platform_pm_ops, driver_data_test_suspend,
+ driver_data_test_resume);
+
+static struct platform_driver driver_data_test_driver = {
+ .driver = {
+ .name = "driver_data_test",
+ .pm = &driver_test_platform_pm_ops,
+ }
+};
+
void unregister_test_dev_driver_data(struct driver_data_test_device *test_dev)
{
wait_for_completion_timeout(&test_dev->request_complete, 5 * HZ);
dev_info(test_dev->dev, "removing interface\n");
+ platform_device_unregister(driver_data_pdev);
misc_deregister(&test_dev->misc_dev);
free_test_dev_driver_data(test_dev);
}
@@ -1211,6 +1277,11 @@ struct driver_data_test_device *alloc_test_dev_driver_data(int idx)
return NULL;
}
+static int driver_data_probe(struct platform_device *pdev)
+{
+ return 0;
+}
+
static int register_test_dev_driver_data(void)
{
struct driver_data_test_device *test_dev = NULL;
@@ -1230,6 +1301,12 @@ static int register_test_dev_driver_data(void)
goto out;
}
+ driver_data_pdev = platform_create_bundle(
+ &driver_data_test_driver, driver_data_probe, NULL, 0, NULL, 0);
+ if (IS_ERR(driver_data_pdev))
+ return PTR_ERR(driver_data_pdev);
+ test_dev->pdev = &driver_data_pdev->dev;
+
ret = misc_register(&test_dev->misc_dev);
if (ret) {
pr_err("could not register misc device: %d\n", ret);
diff --git a/tools/testing/selftests/firmware/driver_data.sh b/tools/testing/selftests/firmware/driver_data.sh
index dc823ae..15af500 100755
--- a/tools/testing/selftests/firmware/driver_data.sh
+++ b/tools/testing/selftests/firmware/driver_data.sh
@@ -48,6 +48,8 @@ ALL_TESTS="$ALL_TESTS 0010:10:1"
ALL_TESTS="$ALL_TESTS 0011:10:1"
ALL_TESTS="$ALL_TESTS 0012:1:1"
ALL_TESTS="$ALL_TESTS 0013:1:1"
+ALL_TESTS="$ALL_TESTS 0015:1:1"
+ALL_TESTS="$ALL_TESTS 0016:1:1"
# Not yet sure how to automate suspend test well yet. For now we expect a
# manual run. If using qemu you can resume a guest using something like the
@@ -219,6 +221,22 @@ config_disable_keep()
fi
}
+config_set_no_cache()
+{
+ if ! echo -n 1 >$DIR/config_no_cache; then
+ echo "$0: Unable to set to no_cache" >&2
+ exit 1
+ fi
+}
+
+config_disable_no_cache()
+{
+ if ! echo -n 0 >$DIR/config_no_cache; then
+ echo "$0: Unable to disable no_cache option" >&2
+ exit 1
+ fi
+}
+
config_enable_opt_cb()
{
if ! echo -n 1 >$DIR/config_enable_opt_cb; then
@@ -412,6 +430,30 @@ config_default_file_should_not_match()
echo "$1: OK!"
}
+config_expect_cached()
+{
+ echo -n 1 > /sys/kernel/debug/firmware/cache
+ if ! cat /sys/kernel/debug/firmware/cache | grep "cached test-driver_data.bin"; then
+ echo "$1: FAIL to cache firmware" >&2
+ echo -n 0 > /sys/kernel/debug/firmware/cache
+ exit 1
+ fi
+ echo "$1: OK! cache success"
+ echo -n 0 > /sys/kernel/debug/firmware/cache
+}
+
+config_expect_non_cached()
+{
+ echo -n 1 > /sys/kernel/debug/firmware/cache
+ if cat /sys/kernel/debug/firmware/cache | grep "cached driver_data_test_no_cache.bin"; then
+ echo "$1: FAIL, should not cache firmware" >&2
+ echo -n 0 > /sys/kernel/debug/firmware/cache
+ exit 1
+ fi
+ echo "$1: OK! expect non cache success"
+ echo -n 0 > /sys/kernel/debug/firmware/cache
+}
+
config_expect_result()
{
RC=$(config_get_test_result)
@@ -821,6 +863,29 @@ driver_data_test_0014()
driver_data_test_0014a
}
+driver_data_test_0015()
+{
+ driver_data_set_sync_defaults
+ config_disable_no_cache
+ config_trigger ${FUNCNAME[0]}
+ config_expect_cached ${FUNCNAME[0]}
+}
+
+driver_data_test_0016()
+{
+ NAME_PREFIX="driver_data_test_no_cache"
+ NAME_POSTFIX=".bin"
+ NAME="${NAME_PREFIX}${NAME_POSTFIX}"
+
+ config_add_api_file $NAME
+
+ driver_data_set_sync_defaults
+ config_set_name $NAME
+ config_set_no_cache
+ config_trigger ${FUNCNAME[0]}
+ config_expect_non_cached ${FUNCNAME[0]}
+ config_rm_api_file $NAME
+}
list_tests()
{
echo "Test ID list:"
@@ -843,6 +908,8 @@ list_tests()
echo "0012 x $(get_test_count 0012) - Verify api call wills will hunt for files, ignore file"
echo "0013 x $(get_test_count 0013) - Verify api call works"
echo "0014 x $(get_test_count 0013) - Verify api call works with suspend + resume"
+ echo "0015 x $(get_test_count 0016) - Verify cache works"
+ echo "0016 x $(get_test_count 0017) - Verify non-cache works"
}
test_reqs
--
2.7.4
From: Yi Li <[email protected]>
This adds DRIVER_DATA_REQ_NO_CACHE flag with .req flag under struct
driver_data_req_params. When this flag is set, the driver_data driver
will not cache the firmware during PM cycle, which is expensive. It
will be used by streaming case and other drivers which implement
their own cache thing. Also added the debugfs interface to selftest.
Signed-off-by: Yi Li <[email protected]>
---
drivers/base/firmware_class-dbg.c | 108 ++++++++++++++++++++++++++++++++++++++
drivers/base/firmware_class.c | 26 +++++----
include/linux/driver_data.h | 4 ++
3 files changed, 127 insertions(+), 11 deletions(-)
create mode 100644 drivers/base/firmware_class-dbg.c
diff --git a/drivers/base/firmware_class-dbg.c b/drivers/base/firmware_class-dbg.c
new file mode 100644
index 0000000..102a4cd
--- /dev/null
+++ b/drivers/base/firmware_class-dbg.c
@@ -0,0 +1,108 @@
+/*
+ * Copyright (c) 2017 by Yi Li <[email protected]>
+ *
+ */
+/* This is part of firmware_class.c for testing firmware cache */
+
+#ifndef CONFIG_TEST_DRIVER_DATA
+static inline void create_debug_files(struct firmware_cache *cache) { }
+static inline void remove_debug_files(struct firmware_cache *cache) { }
+#else
+#include <linux/debugfs.h>
+#include <linux/seq_file.h>
+
+static int debug_cache_show(struct seq_file *s, void *v)
+{
+ struct firmware_cache *cache = s->private;
+ unsigned long flags;
+ struct fw_cache_entry *cache_entry;
+
+ spin_lock_irqsave(&cache->lock, flags);
+
+ list_for_each_entry(cache_entry, &cache->fw_names, list)
+ seq_printf(s, "cached %s\n", cache_entry->name);
+
+ spin_unlock_irqrestore(&cache->lock, flags);
+
+ return 0;
+}
+
+static int debug_cache_open(struct inode *inode, struct file *file)
+{
+ return single_open(file, debug_cache_show, inode->i_private);
+}
+
+#define MAX_LEN 16
+/**
+ * test_cache - set value in the 'cache' control file
+ *
+ * The relevant values are:
+ *
+ * 1: Test the suspend and start the cache
+ * 0: Test the resume and clear the cache.
+ **/
+static ssize_t test_cache(struct file *fp, const char __user *user_buffer,
+ size_t size, loff_t *ppos)
+{
+ char buf[MAX_LEN];
+ size_t len;
+ long cmd;
+
+ len = min(size, (size_t)(MAX_LEN - 1));
+ if (copy_from_user(buf, user_buffer, len))
+ return -EFAULT;
+ buf[len] = 0;
+ if (kstrtol(buf, 10, &cmd))
+ return -EFAULT;
+
+#ifdef CONFIG_PM_SLEEP
+ switch (cmd) {
+ /* Simulate PM suspend prepare and start to cache */
+ case 1:
+ kill_pending_fw_fallback_reqs(true);
+ device_cache_fw_images();
+ disable_firmware();
+ break;
+ /* Simulate PM resume and un-cache */
+ case 0:
+ mutex_lock(&fw_lock);
+ fw_cache.state = FW_LOADER_NO_CACHE;
+ mutex_unlock(&fw_lock);
+ enable_firmware();
+ device_uncache_fw_images_delay(10);
+ break;
+ default:
+ pr_err("unexpected cmd\n");
+ }
+#endif
+ return size;
+}
+
+static const struct file_operations debug_cache_fops = {
+ .open = debug_cache_open,
+ .read = seq_read,
+ .write = test_cache,
+ .llseek = seq_lseek,
+ .release = single_release,
+};
+
+static void create_debug_files(struct firmware_cache *cache)
+{
+ cache->debug = debugfs_create_dir("firmware", NULL);
+ if (!cache->debug)
+ return;
+ if (!debugfs_create_file("cache", 0644, cache->debug,
+ cache, &debug_cache_fops))
+ goto failed_create;
+ return;
+
+failed_create:
+ debugfs_remove_recursive(cache->debug);
+}
+
+static void remove_debug_files(struct firmware_cache *cache)
+{
+ debugfs_remove_recursive(cache->debug);
+ cache->debug = NULL;
+}
+#endif
diff --git a/drivers/base/firmware_class.c b/drivers/base/firmware_class.c
index 7af430a..a70a2a7 100644
--- a/drivers/base/firmware_class.c
+++ b/drivers/base/firmware_class.c
@@ -72,14 +72,10 @@ enum driver_data_mode {
* issue a uevent to userspace. Userspace in turn is expected to be
* monitoring for uevents for the firmware_class and will use the
* exposted sysfs interface to upload the driver data for the caller.
- * @DRIVER_DATA_PRIV_REQ_NO_CACHE: indicates that the driver data request
- * should not set up and use the internal caching mechanism to assist
- * drivers from fetching driver data at resume time after suspend.
*/
enum driver_data_priv_reqs {
DRIVER_DATA_PRIV_REQ_FALLBACK = 1 << 0,
DRIVER_DATA_PRIV_REQ_FALLBACK_UEVENT = 1 << 1,
- DRIVER_DATA_PRIV_REQ_NO_CACHE = 1 << 2,
};
/**
@@ -151,10 +147,12 @@ struct driver_data_params {
}
#define __DATA_REQ_FIRMWARE_BUF(buf, size) \
+ .req_params = { \
+ .reqs = DRIVER_DATA_REQ_NO_CACHE, \
+ }, \
.priv_params = { \
.priv_reqs = DRIVER_DATA_PRIV_REQ_FALLBACK | \
- DRIVER_DATA_PRIV_REQ_FALLBACK_UEVENT | \
- DRIVER_DATA_PRIV_REQ_NO_CACHE, \
+ DRIVER_DATA_PRIV_REQ_FALLBACK_UEVENT, \
.alloc_buf = buf, \
.alloc_buf_size = size, \
}
@@ -186,7 +184,7 @@ struct driver_data_params {
#define driver_data_param_uevent(params) \
(!!((params)->priv_reqs & DRIVER_DATA_PRIV_REQ_FALLBACK_UEVENT))
#define driver_data_param_nocache(params) \
- (!!((params)->priv_reqs & DRIVER_DATA_PRIV_REQ_NO_CACHE))
+ (!!((params)->reqs & DRIVER_DATA_REQ_NO_CACHE))
#define driver_data_param_optional(params) \
(!!((params)->reqs & DRIVER_DATA_REQ_OPTIONAL))
@@ -424,6 +422,8 @@ struct firmware_cache {
struct delayed_work work;
struct notifier_block pm_notify;
+
+ struct dentry *debug;
#endif
};
@@ -790,17 +790,17 @@ static int assign_firmware_buf(struct firmware *fw, struct device *device,
* device may has been deleted already, but the problem
* should be fixed in devres or driver core.
*/
- /* don't cache firmware handled without uevent */
+ /* don't cache without uevent for legacy API */
if (device &&
- driver_data_param_uevent(&data_params->priv_params) &&
- !driver_data_param_nocache(&data_params->priv_params))
+ (driver_data_param_uevent(&data_params->priv_params) ||
+ !driver_data_param_nocache(&data_params->req_params)))
fw_add_devm_name(device, buf->fw_id);
/*
* After caching firmware image is started, let it piggyback
* on request firmware.
*/
- if (!driver_data_param_nocache(&data_params->priv_params) &&
+ if (!driver_data_param_nocache(&data_params->req_params) &&
buf->fwc->state == FW_LOADER_START_CACHE) {
if (fw_cache_piggyback_on_request(buf->fw_id))
kref_get(&buf->ref);
@@ -2437,6 +2437,8 @@ static int fw_cache_piggyback_on_request(const char *name)
}
#endif
+#include "firmware_class-dbg.c"
+
static void __init fw_cache_init(void)
{
spin_lock_init(&fw_cache.lock);
@@ -2454,6 +2456,7 @@ static void __init fw_cache_init(void)
register_pm_notifier(&fw_cache.pm_notify);
register_syscore_ops(&fw_syscore_ops);
+ create_debug_files(&fw_cache);
#endif
}
@@ -2492,6 +2495,7 @@ static void __exit firmware_class_exit(void)
#ifdef CONFIG_PM_SLEEP
unregister_syscore_ops(&fw_syscore_ops);
unregister_pm_notifier(&fw_cache.pm_notify);
+ remove_debug_files(&fw_cache);
#endif
unregister_reboot_notifier(&fw_shutdown_nb);
#ifdef CONFIG_FW_LOADER_USER_HELPER
diff --git a/include/linux/driver_data.h b/include/linux/driver_data.h
index 2cb999e..4d5f9d1 100644
--- a/include/linux/driver_data.h
+++ b/include/linux/driver_data.h
@@ -124,11 +124,15 @@ union driver_data_cbs {
* file to be present given the API range, it is only required for one
* file in the API range to be present. If the %DRIVER_DATA_REQ_OPTIONAL
* flag is also enabled then all files are treated as optional.
+ * @DRIVER_DATA_REQ_NO_CACHE: indicates that the driver data request
+ * should not set up and use the internal caching mechanism to assist
+ * drivers from fetching driver data at resume time after suspend.
*/
enum driver_data_reqs {
DRIVER_DATA_REQ_OPTIONAL = 1 << 0,
DRIVER_DATA_REQ_KEEP = 1 << 1,
DRIVER_DATA_REQ_USE_API_VERSIONING = 1 << 2,
+ DRIVER_DATA_REQ_NO_CACHE = 1 << 3,
};
/**
--
2.7.4
From: Yi Li <[email protected]>
Set DRIVER_DATA_REQ_NO_CACHE flag to disable driver_data driver caching
mechanism, iwlwifi has its own firmware cache management.
Signed-off-by: Yi Li <[email protected]>
---
drivers/net/wireless/intel/iwlwifi/iwl-drv.c | 2 ++
1 file changed, 2 insertions(+)
diff --git a/drivers/net/wireless/intel/iwlwifi/iwl-drv.c b/drivers/net/wireless/intel/iwlwifi/iwl-drv.c
index 028854d3..db4d6fc 100644
--- a/drivers/net/wireless/intel/iwlwifi/iwl-drv.c
+++ b/drivers/net/wireless/intel/iwlwifi/iwl-drv.c
@@ -229,6 +229,8 @@ static int iwl_request_firmware(struct iwl_drv *drv)
const struct driver_data_req_params req_params = {
DRIVER_DATA_API_CB(iwl_req_fw_callback, drv),
DRIVER_DATA_API(cfg->ucode_api_min, cfg->ucode_api_max, ".ucode"),
+ .reqs = DRIVER_DATA_REQ_NO_CACHE |
+ DRIVER_DATA_REQ_USE_API_VERSIONING,
};
return driver_data_request_async(name_pre,
--
2.7.4
On Fri, Jun 16, 2017 at 05:58:29PM -0500, [email protected] wrote:
> From: Yi Li <[email protected]>
>
> This adds DRIVER_DATA_REQ_NO_CACHE flag with .req flag under struct
> driver_data_req_params. When this flag is set, the driver_data driver
> will not cache the firmware during PM cycle, which is expensive.
Why is it "expensive"? What do you mean by this? Caching was added to
help things out, why would you not want it?
> It will be used by streaming case and other drivers which implement
> their own cache thing.
Why would a driver implement their own cache? And what do you mean by
"streaming case"?
Again, as I said to Luis's patches, you need to show that the added code
is actually needed. You only convert one driver to this new interface
in this series, is that the only user of it? If so, that seems
excessive don't you think?
> Also added the debugfs interface to selftest.
In this patch? Why not have it be in a separate patch like it should
be?
> diff --git a/drivers/base/firmware_class-dbg.c b/drivers/base/firmware_class-dbg.c
> new file mode 100644
> index 0000000..102a4cd
> --- /dev/null
> +++ b/drivers/base/firmware_class-dbg.c
> @@ -0,0 +1,108 @@
> +/*
> + * Copyright (c) 2017 by Yi Li <[email protected]>
> + *
> + */
> +/* This is part of firmware_class.c for testing firmware cache */
> +
> +#ifndef CONFIG_TEST_DRIVER_DATA
> +static inline void create_debug_files(struct firmware_cache *cache) { }
> +static inline void remove_debug_files(struct firmware_cache *cache) { }
> +#else
> +#include <linux/debugfs.h>
> +#include <linux/seq_file.h>
This implies you are including a .c file into another one. Please never
do that.
> - /* don't cache firmware handled without uevent */
Um, I thought we didn't have this feature already? But this implies
that we do have it, so what exactly does this patch do?
thanks,
greg k-h
hi Greg
On 6/23/2017 10:41 AM, Greg KH wrote:
> On Fri, Jun 16, 2017 at 05:58:29PM -0500, [email protected] wrote:
>> From: Yi Li <[email protected]>
>>
>> This adds DRIVER_DATA_REQ_NO_CACHE flag with .req flag under struct
>> driver_data_req_params. When this flag is set, the driver_data driver
>> will not cache the firmware during PM cycle, which is expensive.
>
> Why is it "expensive"? What do you mean by this? Caching was added to
> help things out, why would you not want it?
>
Say we have couple drivers (GPU, wifi, FPGA, etc) loading firmwares
during init, with firmware cache feature on, each time the system going
to PM suspend, the firmware framework will cache all the firmware files
(which have been loaded before) from fs to RAM, just in case during PM
resume the drivers need to re-program firmware. If say FPGA driver does
not need to re-program firmware to hardware for suspend/resume, it's
"expensive/wasted" to load those in suspend phase and do not use them.
Or I am missing something?
>> It will be used by streaming case and other drivers which implement
>> their own cache thing.
>
> Why would a driver implement their own cache? And what do you mean by
> "streaming case"?
When the firmware file size is too big to fit into the RAM, we try to
streaming it (load a page and program the page to the hardware and
repeat till the end of file). For this case, the firmware was not loaded
to RAM as a whole piece, there is no point/way to cache it.
>
> Again, as I said to Luis's patches, you need to show that the added code
> is actually needed. You only convert one driver to this new interface
> in this series, is that the only user of it? If so, that seems
> excessive don't you think?
>
I started adding the new streaming use case with current firmware code
back on March, saw Luis is working on the new driver data, which suppose
to make expending new features cleaner, that's the reason I switched to
driver data vehicle. New feature/requirement normally come one at a
time, with this logic it's always excessive to add new interface. :) But
if you all decide to keep the current firmware framework, I can switch
back.
>> Also added the debugfs interface to selftest.
>
> In this patch? Why not have it be in a separate patch like it should
> be?
Sure, I can do that.
>
>> diff --git a/drivers/base/firmware_class-dbg.c b/drivers/base/firmware_class-dbg.c
>> new file mode 100644
>> index 0000000..102a4cd
>> --- /dev/null
>> +++ b/drivers/base/firmware_class-dbg.c
>> @@ -0,0 +1,108 @@
>> +/*
>> + * Copyright (c) 2017 by Yi Li <[email protected]>
>> + *
>> + */
>> +/* This is part of firmware_class.c for testing firmware cache */
>> +
>> +#ifndef CONFIG_TEST_DRIVER_DATA
>> +static inline void create_debug_files(struct firmware_cache *cache) { }
>> +static inline void remove_debug_files(struct firmware_cache *cache) { }
>> +#else
>> +#include <linux/debugfs.h>
>> +#include <linux/seq_file.h>
>
> This implies you are including a .c file into another one. Please never
> do that.
>
>> - /* don't cache firmware handled without uevent */
>
> Um, I thought we didn't have this feature already? But this implies
> that we do have it, so what exactly does this patch do?
>
> thanks,
>
> greg k-h
> --
> To unsubscribe from this list: send the line "unsubscribe linux-fpga" in
> the body of a message to [email protected]
> More majordomo info at http://vger.kernel.org/majordomo-info.html
>
On Mon, Jun 26, 2017 at 10:28 AM, Li, Yi <[email protected]> wrote:
Hi Greg,
> hi Greg
>
> On 6/23/2017 10:41 AM, Greg KH wrote:
>>
>> On Fri, Jun 16, 2017 at 05:58:29PM -0500, [email protected] wrote:
>>>
>>> From: Yi Li <[email protected]>
>>>
>>> This adds DRIVER_DATA_REQ_NO_CACHE flag with .req flag under struct
>>> driver_data_req_params. When this flag is set, the driver_data driver
>>> will not cache the firmware during PM cycle, which is expensive.
>>
>>
>> Why is it "expensive"? What do you mean by this? Caching was added to
>> help things out, why would you not want it?
>>
>
> Say we have couple drivers (GPU, wifi, FPGA, etc) loading firmwares during
> init, with firmware cache feature on, each time the system going to PM
> suspend, the firmware framework will cache all the firmware files (which
> have been loaded before) from fs to RAM, just in case during PM resume the
> drivers need to re-program firmware. If say FPGA driver does not need to
> re-program firmware to hardware for suspend/resume, it's "expensive/wasted"
> to load those in suspend phase and do not use them. Or I am missing
> something?
>
>>> It will be used by streaming case and other drivers which implement
>>> their own cache thing.
>>
>>
>> Why would a driver implement their own cache? And what do you mean by
>> "streaming case"?
>
> When the firmware file size is too big to fit into the RAM, we try to
> streaming it (load a page and program the page to the hardware and repeat
> till the end of file). For this case, the firmware was not loaded to RAM as
> a whole piece, there is no point/way to cache it.
Streaming for FPGA programming would mean a FPGA driver could load the
firmware a page at a time and write each page to the FPGA. In most
cases there is no need to keep the FPGA image in memory after
programming is done or even keep the whole image in memory while
programming.
As we're starting to look at larger devices with 100Meg FPGA images,
we want to avoid contiguous buffers. An alternative would be firmware
files loaded to scatter gather tables since Jason Gunthorpe added sg
support to fpga-mgr.
Alan Tull