[PATCH v2 0/8] firmware: add new sysdata API

[PATCH v2 0/8] firmware: add new sysdata API

[Luis R. Rodriguez]

I started working on extending the firmware API to in the long
run help add firmware signing, that was the orignal purpose of
the sysdata API, now it also provides a good outlet for us for
how to compartamentalize the old usermode helper, since we cannot
remove it.

The core stuff that we wished to share is now mergd, which consisted
of adding a common core file loader. Mimi merged that work, so now its
time to respin this series. I decided to tackle adding a test driver
as well, which lets you muck at will with the API, even letting you
run things in parallel, so a lot of test can be written now in
userspace.

Perhaps the biggest change since last iteration is is both the use of
async_schedule*(), a test driver, and of course a large set of SmPL grammar
to help users convert if they so wish. Since we now have grammar to help hunt
down explicit usermode helper users [0] and annotations to whitelist
these callers and we've deteremined that we only have TWO drivers still
left explictly calling out for the usermode helper we may want to
consider if we can just sweep out the usermode helper underneath
all other calls, however that does a huge disservice to any Linux kernel
built with CONFIG_FW_LOADER_USER_HELPER_FALLBACK. Most distributions
disable this now though.

Converting drivers one by one is a large task, I don't recommend it.
I'd like to instead recommend we convert over users over that we know
we can see a benefit for and for which we know won't break old userspace.
One can know if one will not break userspace if one is certain old userspace
does not exist that requires a usermode helper for the driver. One example of
a benefit of using the sysdata API is for instance -- letting the sysdata API
deal with free'ing your sysdata; or if you're adding a new driver, you not
wanting to add your own completion / wait stuff.

The use of the old firmware API API varies, there is a good split between
drivers that need to keep the firmware and drivers that just request it for
an immediate use. For the driver that need to keep the firmware (in sysdata
lingo this is the ones that use the descriptors with SYSDATA_KEEP_SYNC() or
SYSDATA_KEEP_ASYNC()) I'd recommend instead we seriously consider extending
the sysdata API with using devm wrappers so that free'ing can also be skipped
there. Another example future extension to the API is daisy-chained requests,
there's a few drivers that do this, and having a simple API that manages
this would provide a a huge cleanup and probably fix quite a bit of odd bugs.

This series depends on 2 other series, the coccicheck enhancements [2] and
the firmware SmPL grammar extensions [3]. If you wish you can get the changes
from my linux-next tree as well [4]. Please do note that all these series
are based on linux-next tag next-20160616, and I noticed that Andrew Morton
had picked up a patch by Stephen Boyd and Vikram Mulukutla to add yet-another
new old firmware API. This then applies on top of that as its merged on
linux-next, however my recommendation would be to revert that patch and
re-write the patch under the sysdata API to help take advantage of a simple
API and stop creating new symbols for minor tweaks.

The SmPL transformation patches are provided as-is, but help is obviously
welcomed to enhance them, its why they are not in scripts/coccinelle/ -- what
I recommend is to leave it as is as it deals with the cases that use the
firmware locally and therefore do not require to keep it (SYSDATA_KEEP_SYNC()
or SYSDATA_KEEP_ASYNC()), we add devm support later, and then if user want
to help clean up their driver they can then use the form that does not
require releaesing the firmware at all on their end. The way we envision you'd
use the transformation patches is you'd use them against each driver one at
a time, not on the entire kernel, however using it against the entire kernel
is also possible but note that currently the async cookies are not added for
you when that is done.

FWIW running it against linux-next next-20160616 on a 32-core system takes
about 3 minutes using glimpse, also ~3 minutes with gitgrep and produces the
following diffstat:

 148 files changed, 5676 insertions(+), 4504 deletions(-)

A lot of this is because the sync case can now deal with freeing your firmware
for you, as such a callback is needed. Running it on my 4-core laptop using
gitgrep (default if you change my coccicheck series if you're on git) takes
~11 minutes on the entire kernel (not recommended), and about ~4 seconds
against a random 802.11 driver (drivers/net/wireless/ti/wl1251/). If you'd
like to be very careful with the transformations you could merge an series of
avoids declarations, by reverting a patch I dropped [5], we discareded this
given that we expect driver developers only to use this to later manually
inspect the code changes by hand carefully. Feedback on the results of the
transformations is welcomed and appreciated.

The two drivers I picked to transform here were simply random to help demo
the transformations.

[0] https://lkml.kernel.org/r/1466117661-22075-4-git-send-email-mcgrof@kernel.org
[1] https://lkml.kernel.org/r/1466117661-22075-5-git-send-email-mcgrof@kernel.org
[2] https://lkml.kernel.org/r/1466116292-21843-1-git-send-email-mcgrof@kernel.org
[3] https://lkml.kernel.org/r/1466117661-22075-1-git-send-email-mcgrof@kernel.org
[4] https://git.kernel.org/cgit/linux/kernel/git/mcgrof/linux-next.git/log/?h=20160616-sysdata-v2
[5] http://drvbp1.linux-foundation.org/~mcgrof/2016/06/16/remove-avoids.patch

Luis R. Rodriguez (8):
  firmware: add new extensible firmware API - sysdata_file_request*()
  lib/test_firmware.c: use late_initcall()
  selftests: firmware: only modprobe if driver is missing
  selftests: firmware: send expected errors to /dev/null
  test: add new sysdata_file_request*() loader tester
  Documentation/firmware_class: add sysdata API converter SmPL patch
  x86/microcode: convert to use sysdata API
  p54: convert to sysdata API

 .../firmware_class/0001-convert-sysdata-sync.cocci | 1154 ++++++++++++++++++++
 .../0002-convert-sysdata-async.cocci               |  259 +++++
 .../0003-convert-sysdata-generic.cocci             |   43 +
 Documentation/firmware_class/convert-sysdata.sh    |   13 +
 Documentation/firmware_class/system_data.txt       |  138 +++
 MAINTAINERS                                        |    3 +-
 arch/x86/kernel/cpu/microcode/amd.c                |   56 +-
 drivers/base/firmware_class.c                      |  327 ++++++
 drivers/net/wireless/intersil/p54/eeprom.c         |    2 +-
 drivers/net/wireless/intersil/p54/fwio.c           |    5 +-
 drivers/net/wireless/intersil/p54/led.c            |    2 +-
 drivers/net/wireless/intersil/p54/main.c           |    2 +-
 drivers/net/wireless/intersil/p54/p54.h            |    3 +-
 drivers/net/wireless/intersil/p54/p54pci.c         |   26 +-
 drivers/net/wireless/intersil/p54/p54pci.h         |    4 +-
 drivers/net/wireless/intersil/p54/p54spi.c         |   81 +-
 drivers/net/wireless/intersil/p54/p54spi.h         |    2 +-
 drivers/net/wireless/intersil/p54/p54usb.c         |   18 +-
 drivers/net/wireless/intersil/p54/p54usb.h         |    4 +-
 drivers/net/wireless/intersil/p54/txrx.c           |    2 +-
 include/linux/sysdata.h                            |  244 +++++
 lib/Kconfig.debug                                  |   12 +
 lib/Makefile                                       |    1 +
 lib/test_firmware.c                                |    2 +-
 lib/test_sysdata.c                                 | 1046 ++++++++++++++++++
 tools/testing/selftests/firmware/fw_filesystem.sh  |   17 +-
 tools/testing/selftests/firmware/sysdata.sh        |  633 +++++++++++
 27 files changed, 4013 insertions(+), 86 deletions(-)
 create mode 100644 Documentation/firmware_class/0001-convert-sysdata-sync.cocci
 create mode 100644 Documentation/firmware_class/0002-convert-sysdata-async.cocci
 create mode 100644 Documentation/firmware_class/0003-convert-sysdata-generic.cocci
 create mode 100755 Documentation/firmware_class/convert-sysdata.sh
 create mode 100644 Documentation/firmware_class/system_data.txt
 create mode 100644 include/linux/sysdata.h
 create mode 100644 lib/test_sysdata.c
 create mode 100755 tools/testing/selftests/firmware/sysdata.sh

-- 
2.8.2

[PATCH v2 1/8] firmware: add new extensible firmware API - sysdata_file_request*()

[Luis R. Rodriguez]

The firmware API has evolved over the years slowly, as it
grows we extend it by adding new routines or at times we extend
existing routines with more or less arguments. This doesn't scale
well, when new arguments are added to existing routines it means
we need to traverse the kernel with a slew of collateral
evolutions to adjust old driver users. The firmware API is also
now being used for things outside of the scope of what typically
would be considered "firmware", an example here is the p54 driver
enables users to provide a custom EEPROM through this interface.
Another example is optional CPU microcode updates. This list is
actually quite endless...

There are other subsystems which would like to make use of the
APIs for similar things (not firmware) but have different
requirements and criteria which they'd like to be met for the
requested file. If different requirements are needed it would
again mean adding more arguments and making a slew of collateral
evolutions, or adding yet-another-new-API-call.

Another sticking point over the current firmware API is that
some callers may need the usermode helper when its enabled.
No matter how much a lot of us now hate the usermode helper
(even Linus has called to deprecate it [0]), we've determined
we cannot get rid of it now [1]. There have even been hints
about further valid extended uses expected for the usermode
helper in the future... Although we cannot deprecate the usermode
helpers we can compartamentalize its uses to only valid uses then.

Even if we compartamentalize the usermode helpers uses, we still
need a flexible API for growing needs and requirements and new
features. This new extensible firmware API enables new extensions
to be added (an expected desirable feature for instance is firmware
signing support) by avoiding future unnecessary collateral
evolutions as this code / features get added and also provides
a clean way to enable folks who do wish to deprecate the usermode
helper to do so with certainty.

This new set of APIS leaves the old firmware API as-is, ignores all
consideration for usermode-helpers, labels the new API to reflect its
broad use outside of the scope of firmware: system data helpers, and
builds on top of the original firmware core code.

The new extensible "system data" set of helpers accepts that there
really are only two types of requests for accessing system data:

a) synchronous requests
b) asynchronous requests

Both of these requests may have a different set of requirements
which must be met. These requirements can simply be passed as a
descriptors to each type of request. The descriptor can be extended
over time to support different requirements as the kernel evolves.

Using the new system data helpers is only necessary if you have
requirements outside of what the existing old firmware API accepts
or alternatively if you want to ensure to avoid the usermode helper
at all times, regardless of what kernel your driver might run in.

Developers with new uses should extend the new new descriptors and system
data code to provide support for new features.

A few simple features added as part of the new set of system data
request APIs, other than making the new API easily extensible for
the future:

 - Usermode helpers is completely ignored, *always*
 - By default the kernel will free the system data file for you after
   your callbacks are called, you however are allowed to request that
   you wish to keep the system data file on the descriptor. The new
   sysdata API is able to free the sysdata file for you by requiring a
   consumer callback for the system data file.
 - You no longer need to declare and use your own completions, you
   can replace your completions with sysdata_synchronize_request() using
   the async_cookie set for you by sysdata_file_request_async(). When
   sysdata_file_request_async() completes you can rest assured all the
   work for both triggering, and processing the sysdata using any of
   your callbacks has completed.
 - Allow both asynchronous and synchronous request to specify that system data
   files are optional. With the old APIs we had added one full API call,
   request_firmware_direct() just for this purpose -- although it should be
   noted another of its goal was to also skip the usermode helper.
   The system data request APIs allow for you to annotate that a system
   data file is optional for both synchronous or asynchronous requests
   through the same two basic set of APIs.
 - The system data request APIs currently match the old synchronous firmware
   API calls to refcounted firmware_class module, but it should be easy
   to add support now to enable also refcounting the caller's module
   should it be be needed. Likewise the system data request APIs match the
   old asynchronous firmware API call and refcounts the caller's module.

v4 changes:

o Add SYSDATA_KEEP_SYNC() and SYSDATA_KEEP_ASYNC() macro helpers,
  drivers that want to keep the firmware are pretty common, however
  note that if we can figure out a way to avoid having drivers
  deal with releasing the firmware we're better off, that however
  can be an additional change to look forward to.

o 0-day-bot make htmldocs warning fixes

o When developing and testing the sysdata test driver I ended up
  running into tons of hairball code just to be able to come up
  with enough code to be able to tweak all possible knobs using
  a userspace test interface. This begged for a cleaner API and
  in testing found that async_schedule_domain() made life so much
  easier. This also added the sysdata_synchronize_request() helper
  which user can use to see if their async request completed. This
  should help users considerably as well. Updated code, commit log
  and documentation to reflect these changes.

o In testing found that to make semantics stronger we should
  require @optional to true on the descriptor if an optional
  callback is to be provided (with SYSDATA_SYNC_OPT_CB() or
  SYSDATA_ASYNC_OPT_CB()). Made notes to ensure to users
  that set @optional to true but are not providing a opt_fail_cb()
  should at the very least seriously consider using the returned
  using async_cookie to sysdata_synchronize_request() to ensure
  no lingering requests are kept out of bounds.

o Updated commit log to reflect how we can compartamentalize
  usermode helper code

o Adds SYSDATA_ASYNC_OPT_CB()

o Forces @optional on SYSDATA_SYNC_OPT_CB() to true

o Ensures sysdata_file_request() and sysdata_file_request_async()
  check for emptry string (name[0] == '\0') as follow up to
  Kees's check for empty string name 471b095dfe0d6 ("firmware_class:
  make sure fw requests contain a name") and later a fix by
  Brian through 715780ae4bb76d ("firmware: actually return NULL on
  failed request_firmware_nowait()).

[0] https://marc.info/?l=linux-kernel&m=144095832412928
[1] https://marc.info/?i=20151006090821.GB9030%40kroah.com

Signed-off-by: Luis R. Rodriguez <mcgrof@kernel.org>
---
 Documentation/firmware_class/system_data.txt |  89 ++++++++
 MAINTAINERS                                  |   3 +-
 drivers/base/firmware_class.c                | 327 +++++++++++++++++++++++++++
 include/linux/sysdata.h                      | 244 ++++++++++++++++++++
 4 files changed, 662 insertions(+), 1 deletion(-)
 create mode 100644 Documentation/firmware_class/system_data.txt
 create mode 100644 include/linux/sysdata.h

diff --git a/Documentation/firmware_class/system_data.txt b/Documentation/firmware_class/system_data.txt
new file mode 100644
index 000000000000..53378ce4fcd0
--- /dev/null
+++ b/Documentation/firmware_class/system_data.txt
@@ -0,0 +1,89 @@
+System data requests API
+========================
+
+As the kernel evolves we keep extending the firmware_class set of APIs
+with more or less arguments, this creates a slew of collateral evolutions.
+The set of users of firmware request APIs has also grown now to include
+users which are not looking for "firmware" per se, but instead general
+system data files which for one reason or another has been decided to be
+kept oustide of the kernel, and/or to allow dynamic updates. The system data
+request set of APIs addresses rebranding of firmware as generic system data
+files, and provides a way to enable these APIs to easily be extended without
+much collateral evolutions.
+
+System data modes of operation
+==============================
+
+There are only two types of modes of operation for system data requests:
+
+  * synchronous  - sysdata_file_request()
+  * asynchronous - sysdata_file_request_async()
+
+Synchronous requests expect requests to be done immediately, asynchronous
+requests enable requests to be scheduled for a later time.
+
+System data file descriptor
+===========================
+
+Variations of types of system data requests are specified by a system  data
+request descriptor. The system data request descriptor can grow as with new
+fields as requirements grow. The old firmware API provides two synchronous
+requests: request_firmware() and request_firmware_direct(), the later allowing
+the caller to specify that the "system data file" is optional. The system data
+request API allows a caller to set the optional nature of the system data file
+on the system data file descriptor using the same synchronous API. Since this
+requirement is part of the descriptor it also allows asynchronous requests
+to specify that the system data file is optional.
+
+Reference counting and releasing the system data file
+=====================================================
+
+As with the old firmware API both the device and module are bumped with
+reference counts during the system data requests. This prevents removal
+of the device and module making the system data request call until the
+system data request callbacks have completed, either synchronously or
+asynchronously.
+
+The old firmware APIs refcounted the firmware_class module for synchronous
+requests, meanwhile asynchronous requests refcounted the caller's module.
+The system data request API currently mimic this behaviour, for synchronous
+requests the firmware_class module is refcounted through the use of
+dfl_sync_reqs, although if in the future we may later enable use of
+also refcounting the caller's module as well. Likewise in the future we
+may extend asynchronous calls to refcount the firmware_class module.
+
+Typical use of the old synchronous firmware APIs consist of the caller
+requesting for "system data", consuming it after a request and finally
+freeing it. Typical asynchronous use of the old firmware APIs consist of
+the caller requesting for "system data" and then finally freeing it on
+asynchronous callback.
+
+The system data request API enables callers to provide a callback for both
+synchronous and asynchronous requests and since consumption can be expected
+in these callbacks it frees it for you by default after callback handlers
+are issued. If you wish to keep the system data around after your callbacks
+you must specify this through the system data request descriptor.
+
+Async cookies, replacing completions
+====================================
+
+With this new API you do not need to declare and use your own completions, you
+can replace your completions with sysdata_synchronize_request() using the
+async_cookie set for you by sysdata_file_request_async(). When
+sysdata_file_request_async() completes you can rest assured all the work for
+both triggering, and processing the sysdata using any of your callbacks has
+completed.
+
+User mode helper
+================
+
+The old firmware API provided support for an optional user mode helper. The
+new system data request API abandons all notions of the usermode helper.
+
+Tracking development enhancements and ideas
+===========================================
+
+To help track ongoing development for firmware_class and related items to
+firmware_class refer to the kernel newbies wiki page [0].
+
+[0] http://kernelnewbies.org/KernelProjects/firmware-class-enhancements
diff --git a/MAINTAINERS b/MAINTAINERS
index b030a95dcb97..4ebb7485a4d4 100644
--- a/MAINTAINERS
+++ b/MAINTAINERS
@@ -4778,7 +4778,7 @@ F:	include/linux/firewire.h
 F:	include/uapi/linux/firewire*.h
 F:	tools/firewire/
 
-FIRMWARE LOADER (request_firmware)
+FIRMWARE LOADER (request_firmware, sysdata_file_request)
 M:	Ming Lei <ming.lei@canonical.com>
 M:	Luis R. Rodriguez <mcgrof@kernel.org>
 L:	linux-kernel@vger.kernel.org
@@ -4786,6 +4786,7 @@ S:	Maintained
 F:	Documentation/firmware_class/
 F:	drivers/base/firmware*.c
 F:	include/linux/firmware.h
+F:	include/linux/sysdata.h
 
 FLASH ADAPTER DRIVER (IBM Flash Adapter 900GB Full Height PCI Flash Card)
 M:	Joshua Morris <josh.h.morris@us.ibm.com>
diff --git a/drivers/base/firmware_class.c b/drivers/base/firmware_class.c
index dca4f9cbf4db..913339fb844b 100644
--- a/drivers/base/firmware_class.c
+++ b/drivers/base/firmware_class.c
@@ -2,6 +2,7 @@
  * firmware_class.c - Multi purpose firmware loading support
  *
  * Copyright (c) 2003 Manuel Estrada Sainz
+ * Copyright (c) 2016 Luis R. Rodriguez <mcgrof@kernel.org>
  *
  * Please see Documentation/firmware_class/ for more information.
  *
@@ -18,6 +19,7 @@
 #include <linux/mutex.h>
 #include <linux/workqueue.h>
 #include <linux/highmem.h>
+#include <linux/sysdata.h>
 #include <linux/firmware.h>
 #include <linux/slab.h>
 #include <linux/sched.h>
@@ -39,6 +41,12 @@ MODULE_AUTHOR("Manuel Estrada Sainz");
 MODULE_DESCRIPTION("Multi purpose firmware loading support");
 MODULE_LICENSE("GPL");
 
+static const struct sysdata_file_sync_reqs dfl_sync_reqs = {
+	.mode = SYNCDATA_SYNC,
+	.module = THIS_MODULE,
+	.gfp = GFP_KERNEL,
+};
+
 /* Builtin firmware support */
 
 #ifdef CONFIG_FW_LOADER
@@ -1300,6 +1308,184 @@ void release_firmware(const struct firmware *fw)
 }
 EXPORT_SYMBOL(release_firmware);
 
+static void sysdata_file_update(struct sysdata_file *sysdata)
+{
+	struct firmware *fw;
+	struct firmware_buf *buf;
+
+	if (!sysdata || !sysdata->priv)
+		return;
+
+	fw = sysdata->priv;
+	if (!fw->priv)
+		return;
+
+	buf = fw->priv;
+
+	sysdata->size = buf->size;
+	sysdata->data = buf->data;
+
+	pr_debug("%s: fw-%s buf=%p data=%p size=%u",
+		 __func__, buf->fw_id, buf, buf->data,
+		 (unsigned int)buf->size);
+}
+
+/*
+ * 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
+ */
+static int
+_request_sysdata_prepare(struct sysdata_file **sysdata_p, const char *name,
+			  struct device *device)
+{
+	struct sysdata_file *sysdata;
+	struct firmware *fw;
+	int ret;
+
+	*sysdata_p = sysdata = kzalloc(sizeof(*sysdata), GFP_KERNEL);
+	if (!sysdata) {
+		dev_err(device, "%s: kmalloc(struct sysdata) failed\n",
+			__func__);
+		return -ENOMEM;
+	}
+
+	ret = _request_firmware_prepare(&fw, name, device, NULL, 0);
+	if (ret >= 0)
+		sysdata->priv = fw;
+
+	return ret;
+}
+
+/**
+ * release_sysdata_file: - release the resource associated with the sysdata file
+ * @sysdata: sysdata file resource to release
+ **/
+void release_sysdata_file(const struct sysdata_file *sysdata)
+{
+	struct firmware *fw;
+
+	if (sysdata) {
+		if (sysdata->priv) {
+			fw = sysdata->priv;
+			release_firmware(fw);
+		}
+	}
+	kfree(sysdata);
+}
+EXPORT_SYMBOL_GPL(release_sysdata_file);
+
+/*
+ * sysdata_p is always set to be NULL unless a proper system
+ * data file was found.
+ */
+static int _sysdata_file_request(const struct sysdata_file **sysdata_p,
+				 const char *name,
+				 const struct sysdata_file_desc *desc,
+				 struct device *device)
+{
+	struct sysdata_file *sysdata = NULL;
+	struct firmware *fw = NULL;
+	int ret = -EINVAL;
+
+	if (!sysdata_p)
+		goto out;
+
+	if (!desc)
+		goto out;
+
+	if (!name || name[0] == '\0')
+		goto out;
+
+	ret = _request_sysdata_prepare(&sysdata, name, device);
+	if (ret <= 0) /* error or already assigned */
+		goto out;
+
+	fw = sysdata->priv;
+
+	ret = fw_get_filesystem_firmware(device, fw->priv);
+	if (ret && !desc->optional)
+		pr_err("Direct system data load for %s failed with error %d\n",
+		       name, ret);
+
+	if (!ret)
+		ret = assign_firmware_buf(fw, device, FW_OPT_UEVENT);
+
+ out:
+	if (ret < 0) {
+		release_sysdata_file(sysdata);
+		sysdata = NULL;
+	}
+
+	sysdata_file_update(sysdata);
+
+	*sysdata_p = sysdata;
+
+	return ret;
+}
+
+/**
+ * sysdata_file_request - synchronous request for a system data file
+ * @name: name of the system data file
+ * @desc: system data file descriptor, it provides all the requirements
+ * 	which must be met for the file being requested.
+ * @device: device for which firmware is being loaded
+ *
+ * This performs a synchronous system data file lookup with the requirements
+ * specified on @desc, if the file was found meeting the criteria requested
+ * 0 is returned. Access to the system data file data can be accessed through
+ * an optional callback set on the @desc. If the system data file is optional
+ * you must specify that on the @desc and if set you may provide an alternative
+ * callback which if set would be run if the system data file was not found.
+ *
+ * The system data file passed to the callbacks will be NULL unless it was
+ * found matching all the criteria on @desc. 0 is always returned if the file
+ * was found unless a callback was provided, in which case the callback's
+ * return value will be passed. Unless the desc->keep was set the kernel will
+ * release the system data file for you after your callbacks were processed.
+ *
+ * Reference counting is used during the duration of this call on both the
+ * device and module that made the request. This prevents any callers from
+ * freeing either the device or module prior to completion of this call.
+ */
+int sysdata_file_request(const char *name,
+			 const struct sysdata_file_desc *desc,
+			 struct device *device)
+{
+	const struct sysdata_file *sysdata;
+	const struct sysdata_file_sync_reqs *sync_reqs;
+	int ret;
+
+	if (!device || !desc || !name || name[0] == '\0')
+		return -EINVAL;
+
+	if (desc->sync_reqs.mode != SYNCDATA_SYNC)
+		return -EINVAL;
+
+	if (desc_sync_opt_cb(desc) && !desc->optional)
+		return -EINVAL;
+
+	sync_reqs = &dfl_sync_reqs;
+
+	__module_get(sync_reqs->module);
+	get_device(device);
+
+	ret = _sysdata_file_request(&sysdata, name, desc, device);
+	if (ret && desc->optional)
+		ret = desc_sync_opt_call_cb(desc);
+	else
+		ret = desc_sync_found_call_cb(desc, sysdata);
+
+	if (!desc->keep)
+		release_sysdata_file(sysdata);
+
+	put_device(device);
+	module_put(sync_reqs->module);
+
+	return ret;
+}
+EXPORT_SYMBOL_GPL(sysdata_file_request);
+
 /* Async support */
 struct firmware_work {
 	struct work_struct work;
@@ -1388,6 +1574,145 @@ request_firmware_nowait(
 }
 EXPORT_SYMBOL(request_firmware_nowait);
 
+struct sysdata_file_work {
+	const char *name;
+	struct sysdata_file_desc desc;
+	struct device *device;
+};
+
+static ASYNC_DOMAIN(sysdata_async_domain);
+
+static void request_sysdata_file_work_func(void *data, async_cookie_t cookie)
+{
+	struct sysdata_file_work *sys_work = data;
+	const struct sysdata_file_desc *desc;
+	const struct sysdata_file_sync_reqs *sync_reqs;
+	const struct sysdata_file *sysdata;
+	int ret;
+
+	desc = &sys_work->desc;
+	sync_reqs = &desc->sync_reqs;
+
+	ret = _sysdata_file_request(&sysdata, sys_work->name,
+				    desc, sys_work->device);
+	if (ret && desc->optional)
+		desc_async_opt_call_cb(desc);
+	else
+		desc_async_found_call_cb(sysdata, desc);
+
+	if (!desc->keep)
+		release_sysdata_file(sysdata);
+
+	put_device(sys_work->device);
+	module_put(sync_reqs->module);
+
+	kfree_const(sys_work->name);
+	kfree(sys_work);
+}
+
+/**
+ * sysdata_file_request_async - asynchronous request for a system data file
+ * @name: name of the system data file
+ * @desc: system data file descriptor, it provides all the requirements
+ * 	which must be met for the file being requested.
+ * @device: device for which firmware is being loaded
+ * @async_cookie: used for checkpointing your async request
+ *
+ * This performs an asynchronous system data file lookup with the requirements
+ * specified on @desc. The request for the actual system data file lookup will
+ * be scheduled with async_schedule_domain() to be run at a later time. 0 is
+ * returned if we were able to asynchronously schedlue your work to be run.
+ *
+ * Reference counting is used during the duration of this scheduled call on
+ * both the device and module that made the request. This prevents any callers
+ * from freeing either the device or module prior to completion of the
+ * scheduled work.
+ *
+ * Access to the system data file data can be accessed through an optional
+ * callback set on the @desc. If the system data file is optional you must
+ * specify that on the @desc and if set you may provide an alternative
+ * callback which if set would be run if the system data file was not found.
+ *
+ * The system data file passed to the callbacks will always be NULL unless
+ * it was found matching all the criteria on @desc. Unless the desc->keep
+ * was set the kernel will release the system data file for you after your
+ * callbacks were processed on the scheduled work.
+ *
+ * You should use rely on async_cookie to determine if your asynchronous work
+ * has been scheduled and completed. If you need to wait for completion of
+ * processing of your sysdata through your callbacks, or if you just want to
+ * know the hunt is over you can sysdata_synchronize_request() with the
+ * async_cookie.
+ */
+int sysdata_file_request_async(const char *name,
+			       const struct sysdata_file_desc *desc,
+			       struct device *device,
+			       async_cookie_t *async_cookie)
+{
+	struct sysdata_file_work *sys_work;
+	const struct sysdata_file_sync_reqs *sync_reqs;
+
+	if (!device || !desc || !name || name[0] == '\0')
+		return -EINVAL;
+
+	if (desc->sync_reqs.mode != SYNCDATA_ASYNC)
+		return -EINVAL;
+
+	if (desc_async_opt_cb(desc) && !desc->optional)
+		return -EINVAL;
+
+	sync_reqs = &desc->sync_reqs;
+
+	sys_work = kzalloc(sizeof(struct sysdata_file_work), sync_reqs->gfp);
+	if (!sys_work)
+		return -ENOMEM;
+
+	sys_work->device = device;
+	memcpy(&sys_work->desc, desc, sizeof(struct sysdata_file_desc));
+	sys_work->name = kstrdup_const(name, sync_reqs->gfp);
+	if (!sys_work->name) {
+		kfree(sys_work);
+		return -ENOMEM;
+	}
+
+	if (!try_module_get(sync_reqs->module)) {
+		kfree_const(sys_work->name);
+		kfree(sys_work);
+		return -EFAULT;
+	}
+
+	get_device(sys_work->device);
+
+	*async_cookie = async_schedule_domain(request_sysdata_file_work_func,
+					      sys_work,
+					      &sysdata_async_domain);
+
+	return 0;
+}
+EXPORT_SYMBOL_GPL(sysdata_file_request_async);
+
+/**
+ * sysdata_synchronize_request - wait until your async sysdata calls complete
+ * @async_cookie: async cookie
+ *
+ * Waits until all asynchronous sysdata calls prior to and up to @async_cookie
+ * have been completed. You can use this to wait for completion of your own
+ * async callback. Your wait will end after request_sysdata_file_work_func()
+ * is called for your cookie. At this point you can rest assured your
+ * series of async callbacks would have been called if supplied.
+ *
+ * async_cookie+1 is used as async_synchronize_cookie_domain() only waits
+ * until at least your own call is next in queue to be run, we want the
+ * next item after yours to be in queue, this tells us we have run already.
+ * Should there not be any other async scheduled item after yours this will
+ * simply wait until all async sysdata calls are complete.
+ */
+void sysdata_synchronize_request(async_cookie_t async_cookie)
+{
+	async_synchronize_cookie_domain(async_cookie+1, &sysdata_async_domain);
+}
+EXPORT_SYMBOL_GPL(sysdata_synchronize_request);
+
 #ifdef CONFIG_PM_SLEEP
 static ASYNC_DOMAIN_EXCLUSIVE(fw_cache_domain);
 
@@ -1761,6 +2086,7 @@ static int __init firmware_class_init(void)
 
 static void __exit firmware_class_exit(void)
 {
+	async_synchronize_full_domain(&sysdata_async_domain);
 #ifdef CONFIG_PM_SLEEP
 	unregister_syscore_ops(&fw_syscore_ops);
 	unregister_pm_notifier(&fw_cache.pm_notify);
@@ -1769,6 +2095,7 @@ static void __exit firmware_class_exit(void)
 	unregister_reboot_notifier(&fw_shutdown_nb);
 	class_unregister(&firmware_class);
 #endif
+	async_unregister_domain(&sysdata_async_domain);
 }
 
 fs_initcall(firmware_class_init);
diff --git a/include/linux/sysdata.h b/include/linux/sysdata.h
new file mode 100644
index 000000000000..6c16829981ce
--- /dev/null
+++ b/include/linux/sysdata.h
@@ -0,0 +1,244 @@
+#ifndef _LINUX_SYSDATA_H
+#define _LINUX_SYSDATA_H
+
+#include <linux/types.h>
+#include <linux/compiler.h>
+#include <linux/gfp.h>
+#include <linux/device.h>
+#include <linux/async.h>
+
+/*
+ * System Data internals
+ *
+ * Copyright (C) 2015 Luis R. Rodriguez <mcgrof@kernel.org>
+ *
+ * This program is free software; you can redistribute it and/or
+ * modify it under the terms of the GNU General Public Licence
+ * as published by the Free Software Foundation; either version
+ * 2 of the Licence, or (at your option) any later version.
+ */
+
+struct sysdata_file {
+	size_t size;
+	const u8 *data;
+
+	/* sysdata loader private fields */
+	void *priv;
+};
+
+/**
+ * enum sync_data_mode - system data mode of operation
+ *
+ * SYNCDATA_SYNC: your call to request system data is synchronous. We will
+ * 	look for the system data file you have requested immediatley.
+ * SYNCDATA_ASYNC: your call to request system data is asynchronous. We will
+ * 	schedule the search for your system data file to be run at a later
+ * 	time.
+ */
+enum sync_data_mode {
+	SYNCDATA_SYNC,
+	SYNCDATA_ASYNC,
+};
+
+/* one per sync_data_mode */
+union sysdata_file_cbs {
+	struct {
+		int __must_check (*found_cb)(void *, const struct sysdata_file *);
+		void *found_context;
+
+		int __must_check (*opt_fail_cb)(void *);
+		void *opt_fail_context;
+	} sync;
+	struct {
+		void (*found_cb)(const struct sysdata_file *, void *);
+		void *found_context;
+
+		void (*opt_fail_cb)(void *);
+		void *opt_fail_context;
+	} async;
+};
+
+struct sysdata_file_sync_reqs {
+	enum sync_data_mode mode;
+	struct module *module;
+	gfp_t gfp;
+};
+
+/**
+ * struct sysdata_file_desc - system data file descriptor
+ * @optional: if true it is not a hard requirement by the caller that this
+ *	file be present. An error will not be recorded if the file is not
+ *	found. You must set this to true if you have provided a opt_fail_cb
+ *	callback, SYSDATA_SYNC_OPT_CB() and SYSDATA_ASYNC_OPT_CB() ensures
+ *	this is done for you. If you set this to true and are using an
+ *	asynchronous request but not providing a opt_fail_cb() you should
+ *	seriously consider using at the very least using async_cookie provided
+ *	to you to sysdata_synchronize_request() to ensure no lingering
+ *	requests are kept out of bounds.
+ * @keep: if set the caller wants to claim ownership over the system data
+ *	through one of its callbacks, it must later free it with
+ *	release_sysdata_file(). By default this is set to false and the kernel
+ *	will release the system data file for you after callback processing
+ *	has completed.
+ * @sync_reqs: synchronization requirements, this will be taken care for you
+ *	by default if you are usingy sdata_file_request(), otherwise you
+ *	should provide your own requirements.
+ *
+ * This structure is set the by the driver and passed to the system data
+ * file helpers sysdata_file_request() or sysdata_file_request_async().
+ * It is intended to carry all requirements and specifications required
+ * to complete the task to get the requested system date file to the caller.
+ * If you wish to extend functionality of system data file requests you
+ * should extend this data structure and make use of the extensions on
+ * the callers to avoid unnecessary collateral evolutions.
+ *
+ * You are allowed to provide a callback to handle if a system data file was
+ * found or not. You do not need to provide a callback. You may also set
+ * an optional flag which would enable you to declare that the system data
+ * file is optional and that if it is not found an alternative callback be
+ * run for you.
+ *
+ * Refer to sysdata_file_request() and sysdata_file_request_async() for more
+ * details.
+ */
+struct sysdata_file_desc {
+	bool optional;
+	bool keep;
+	struct sysdata_file_sync_reqs sync_reqs;
+	const union sysdata_file_cbs cbs;
+};
+
+/*
+ * We keep these template definitions to a minimum for the most
+ * popular requests.
+ */
+
+/* Typical sync data case */
+#define SYSDATA_SYNC_FOUND(__found_cb, __context)			\
+	.cbs.sync.found_cb = __found_cb,				\
+	.cbs.sync.found_context = __context
+
+#define SYSDATA_DEFAULT_SYNC(__found_cb, __context)			\
+	SYSDATA_SYNC_FOUND(__found_cb, __context)
+
+#define SYSDATA_KEEP_SYNC(__found_cb, __context)			\
+	SYSDATA_DEFAULT_SYNC(__found_cb, __context),			\
+	.keep= true
+
+/* If you have one fallback routine */
+#define SYSDATA_SYNC_OPT_CB(__fail_cb, __context)			\
+	.optional = true,						\
+	.cbs.sync.opt_fail_cb = __fail_cb,				\
+	.cbs.sync.opt_fail_context = __context
+
+/*
+ * Used to define the default asynchronization requirements for
+ * sysdata_file_request_async(). Drivers can override.
+ */
+#define SYSDATA_DEFAULT_ASYNC(__found_cb, __context)			\
+	.sync_reqs = {							\
+		.mode = SYNCDATA_ASYNC,					\
+		.module = THIS_MODULE,					\
+		.gfp = GFP_KERNEL,					\
+	},								\
+	.cbs.async = {							\
+		.found_cb = __found_cb,					\
+		.found_context = __context,				\
+	}
+
+#define SYSDATA_KEEP_ASYNC(__found_cb, __context)			\
+	SYSDATA_DEFAULT_ASYNC(__found_cb, __context),			\
+	.keep = true
+
+#define SYSDATA_ASYNC_OPT_CB(__fail_cb, __context)			\
+	.optional = true,						\
+	.cbs.async.opt_fail_cb = __fail_cb,				\
+	.cbs.async.opt_fail_context = __context
+
+#define desc_sync_found_cb(desc)	((desc)->cbs.sync.found_cb)
+#define desc_sync_found_context(desc)	((desc)->cbs.sync.found_context)
+static inline int desc_sync_found_call_cb(const struct sysdata_file_desc *desc,
+					  const struct sysdata_file *sysdata)
+{
+	if (desc->sync_reqs.mode != SYNCDATA_SYNC)
+		return -EINVAL;
+	if (!desc_sync_found_cb(desc)) {
+		if (sysdata)
+			return 0;
+		return -ENOENT;
+	}
+	return desc_sync_found_cb(desc)(desc_sync_found_context(desc),
+					sysdata);
+}
+
+#define desc_sync_opt_cb(desc)		((desc)->cbs.sync.opt_fail_cb)
+#define desc_sync_opt_context(desc)	((desc)->cbs.sync.opt_fail_context)
+static inline int desc_sync_opt_call_cb(const struct sysdata_file_desc *desc)
+{
+	if (desc->sync_reqs.mode != SYNCDATA_SYNC)
+		return -EINVAL;
+	if (!desc_sync_opt_cb(desc))
+		return 0;
+	return desc_sync_opt_cb(desc)(desc_sync_opt_context(desc));
+}
+
+#define desc_async_found_cb(desc)	((desc)->cbs.async.found_cb)
+#define desc_async_found_context(desc)	((desc)->cbs.async.found_context)
+static inline void desc_async_found_call_cb(const struct sysdata_file *sysdata,
+					    const struct sysdata_file_desc *desc)
+{
+	if (desc->sync_reqs.mode != SYNCDATA_ASYNC)
+		return;
+	if (!desc_async_found_cb(desc))
+		return;
+	desc_async_found_cb(desc)(sysdata, desc_async_found_context(desc));
+}
+
+#define desc_async_opt_cb(desc)		((desc)->cbs.async.opt_fail_cb)
+#define desc_async_opt_context(desc)	((desc)->cbs.async.opt_fail_context)
+static inline void desc_async_opt_call_cb(const struct sysdata_file_desc *desc)
+{
+	if (desc->sync_reqs.mode != SYNCDATA_ASYNC)
+		return;
+	if (!desc_async_opt_cb(desc))
+		return;
+	desc_async_opt_cb(desc)(desc_async_opt_context(desc));
+}
+
+
+#if defined(CONFIG_FW_LOADER) || (defined(CONFIG_FW_LOADER_MODULE) && defined(MODULE))
+int sysdata_file_request(const char *name,
+			 const struct sysdata_file_desc *desc,
+			 struct device *device);
+int sysdata_file_request_async(const char *name,
+			       const struct sysdata_file_desc *desc,
+			       struct device *device,
+			       async_cookie_t *async_cookie);
+void release_sysdata_file(const struct sysdata_file *sysdata);
+void sysdata_synchronize_request(async_cookie_t async_cookie);
+#else
+static inline int sysdata_file_request(const char *name,
+				       const struct sysdata_file_desc *desc,
+				       struct device *device)
+{
+	return -EINVAL;
+}
+
+static inline int sysdata_file_request_async(const char *name,
+					     const struct sysdata_file_desc *desc,
+					     struct device *device,
+					     async_cookie_t *async_cookie);
+{
+	return -EINVAL;
+}
+
+static inline void release_sysdata_file(const struct sysdata_file *sysdata)
+{
+}
+
+void sysdata_synchronize_request(async_cookie_t async_cookie)
+{
+}
+#endif
+
+#endif /* _LINUX_SYSDATA_H */
-- 
2.8.2

[PATCH v2 2/8] lib/test_firmware.c: use late_initcall()

[Luis R. Rodriguez]

When expanding test coverage of firmware_class with the sysdata
API test driver we get a crash when CONFIG_TEST_FIRMWARE=y,
only a combination produces a panic however. This fixes it.

------------[ cut here ]------------
WARNING: CPU: 0 PID: 1 at lib/kobject.c:597 kobject_get+0x4d/0x80
kobject: '(null)' (ffffea0000722850): is not initialized, yet kobject_get() is being called.
Modules linked in:
CPU: 0 PID: 1 Comm: swapper/0 Not tainted 4.6.0-rc3-1-default+ #127
Hardware name: QEMU Standard PC (i440FX + PIIX, 1996), BIOS by qemu-project.org 04/01/2014
 0000000000000000 ffff88001e223c48 ffffffff813948c1 ffff88001e223c98
 0000000000000000 ffff88001e223c88 ffffffff8107eecb 00000255024080c0
 ffff88001f837810 ffffea0000722840 0000000000000000 ffff88001f837800
Call Trace:
 [<ffffffff813948c1>] dump_stack+0x63/0x82
 [<ffffffff8107eecb>] __warn+0xcb/0xf0
 [<ffffffff8107ef3f>] warn_slowpath_fmt+0x4f/0x60
 [<ffffffff814d6783>] ? device_private_init+0x23/0x70
 [<ffffffff81396cdd>] kobject_get+0x4d/0x80
 [<ffffffff814d68aa>] device_add+0xda/0x680
 [<ffffffff814d7040>] device_create_groups_vargs+0xe0/0xf0
 [<ffffffff81d82675>] ? test_firmware_init+0xb2/0xb2
 [<ffffffff814d70f6>] device_create_with_groups+0x36/0x40
 [<ffffffff813ae6d1>] ? test_dev_get_name+0x91/0xd0
 [<ffffffff814ad3d5>] misc_register+0x145/0x180
 [<ffffffff81d826a8>] test_sysdata_init+0x33/0xc8
 [<ffffffff81002123>] do_one_initcall+0xb3/0x200
 [<ffffffff8109d805>] ? parse_args+0x295/0x4b0
 [<ffffffff81d3e12b>] kernel_init_freeable+0x183/0x20e
 [<ffffffff816a1ace>] kernel_init+0xe/0x100
 [<ffffffff816af062>] ret_from_fork+0x22/0x40
 [<ffffffff816a1ac0>] ? rest_init+0x90/0x90
---[ end trace 3779de9087657326 ]---
------------[ cut here ]------------
WARNING: CPU: 0 PID: 1 at lib/kobject.c:597 kobject_get+0x4d/0x80
kobject: '(null)' (ffffea0000722850): is not initialized, yet kobject_get() is being called.
Modules linked in:
CPU: 0 PID: 1 Comm: swapper/0 Tainted: G        W       4.6.0-rc3-1-default+ #127
Hardware name: QEMU Standard PC (i440FX + PIIX, 1996), BIOS by qemu-project.org 04/01/2014
 0000000000000000 ffff88001e223b70 ffffffff813948c1 ffff88001e223bc0
 0000000000000000 ffff88001e223bb0 ffffffff8107eecb 00000255816af7cb
 ffff88001f80c400 0000000000000000 ffffea0000722850 ffff88001f836800
Call Trace:
 [<ffffffff813948c1>] dump_stack+0x63/0x82
 [<ffffffff8107eecb>] __warn+0xcb/0xf0
 [<ffffffff8107ef3f>] warn_slowpath_fmt+0x4f/0x60
 [<ffffffff813a90e5>] ? find_next_bit+0x15/0x20
 [<ffffffff81396cdd>] kobject_get+0x4d/0x80
 [<ffffffff81397629>] kobject_add_internal+0x39/0x340
 [<ffffffff811a9512>] ? kfree_const+0x22/0x30
 [<ffffffff81397998>] kobject_add+0x68/0xb0
 [<ffffffff8107ef3f>] ? warn_slowpath_fmt+0x4f/0x60
 [<ffffffff814d5b3d>] get_device_parent.isra.19+0x10d/0x1d0
 [<ffffffff814d68c1>] device_add+0xf1/0x680
 [<ffffffff814d7040>] device_create_groups_vargs+0xe0/0xf0
 [<ffffffff81d82675>] ? test_firmware_init+0xb2/0xb2
 [<ffffffff814d70f6>] device_create_with_groups+0x36/0x40
 [<ffffffff813ae6d1>] ? test_dev_get_name+0x91/0xd0
 [<ffffffff814ad3d5>] misc_register+0x145/0x180
 [<ffffffff81d826a8>] test_sysdata_init+0x33/0xc8
 [<ffffffff81002123>] do_one_initcall+0xb3/0x200
 [<ffffffff8109d805>] ? parse_args+0x295/0x4b0
 [<ffffffff81d3e12b>] kernel_init_freeable+0x183/0x20e
 [<ffffffff816a1ace>] kernel_init+0xe/0x100
 [<ffffffff816af062>] ret_from_fork+0x22/0x40
 [<ffffffff816a1ac0>] ? rest_init+0x90/0x90
---[ end trace 3779de9087657327 ]---
general protection fault: 0000 [#1] PREEMPT SMP

Signed-off-by: Luis R. Rodriguez <mcgrof@kernel.org>
---
 lib/test_firmware.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/lib/test_firmware.c b/lib/test_firmware.c
index a3e8ec3fb1c5..5f087a03e5f7 100644
--- a/lib/test_firmware.c
+++ b/lib/test_firmware.c
@@ -167,7 +167,7 @@ dereg:
 	return rc;
 }
 
-module_init(test_firmware_init);
+late_initcall(test_firmware_init);
 
 static void __exit test_firmware_exit(void)
 {
-- 
2.8.2

[PATCH v2 3/8] selftests: firmware: only modprobe if driver is missing

[Luis R. Rodriguez]

No need to load test_firmware if its already there.

Signed-off-by: Luis R. Rodriguez <mcgrof@kernel.org>
---
 tools/testing/selftests/firmware/fw_filesystem.sh | 11 +++++++++--
 1 file changed, 9 insertions(+), 2 deletions(-)

diff --git a/tools/testing/selftests/firmware/fw_filesystem.sh b/tools/testing/selftests/firmware/fw_filesystem.sh
index 5c495ad7958a..3ff573dc6009 100755
--- a/tools/testing/selftests/firmware/fw_filesystem.sh
+++ b/tools/testing/selftests/firmware/fw_filesystem.sh
@@ -5,10 +5,17 @@
 # know so we can be sure we're not accidentally testing the user helper.
 set -e
 
-modprobe test_firmware
-
 DIR=/sys/devices/virtual/misc/test_firmware
 
+if [ ! -d $DIR ]; then
+	modprobe test_firmware
+	if [ ! -d $DIR ]; then
+		echo "$0: $DIR not present"
+		echo "You must have CONFIG_TEST_FIRMWARE=m or CONFIG_TEST_FIRMWARE=y"
+		exit 1
+	fi
+fi
+
 # CONFIG_FW_LOADER_USER_HELPER has a sysfs class under /sys/class/firmware/
 # These days no one enables CONFIG_FW_LOADER_USER_HELPER so check for that
 # as an indicator for CONFIG_FW_LOADER_USER_HELPER.
-- 
2.8.2

[PATCH v2 4/8] selftests: firmware: send expected errors to /dev/null

[Luis R. Rodriguez]

Error that we expect should not be spilled to stdout.

Without this we get:

./fw_filesystem.sh: line 58: printf: write error: Invalid argument
./fw_filesystem.sh: line 63: printf: write error: No such device
./fw_filesystem.sh: line 69: echo: write error: No such file or directory
./fw_filesystem.sh: filesystem loading works
./fw_filesystem.sh: async filesystem loading works

With it:

./fw_filesystem.sh: filesystem loading works
./fw_filesystem.sh: async filesystem loading works

Signed-off-by: Luis R. Rodriguez <mcgrof@kernel.org>
---
 tools/testing/selftests/firmware/fw_filesystem.sh | 6 +++---
 1 file changed, 3 insertions(+), 3 deletions(-)

diff --git a/tools/testing/selftests/firmware/fw_filesystem.sh b/tools/testing/selftests/firmware/fw_filesystem.sh
index 3ff573dc6009..265a3f4badc6 100755
--- a/tools/testing/selftests/firmware/fw_filesystem.sh
+++ b/tools/testing/selftests/firmware/fw_filesystem.sh
@@ -55,18 +55,18 @@ echo "ABCD0123" >"$FW"
 
 NAME=$(basename "$FW")
 
-if printf '\000' >"$DIR"/trigger_request; then
+if printf '\000' >"$DIR"/trigger_request 2> /dev/null; then
 	echo "$0: empty filename should not succeed" >&2
 	exit 1
 fi
 
-if printf '\000' >"$DIR"/trigger_async_request; then
+if printf '\000' >"$DIR"/trigger_async_request 2> /dev/null; then
 	echo "$0: empty filename should not succeed (async)" >&2
 	exit 1
 fi
 
 # Request a firmware that doesn't exist, it should fail.
-if echo -n "nope-$NAME" >"$DIR"/trigger_request; then
+if echo -n "nope-$NAME" >"$DIR"/trigger_request 2> /dev/null; then
 	echo "$0: firmware shouldn't have loaded" >&2
 	exit 1
 fi
-- 
2.8.2

[PATCH v2 5/8] test: add new sysdata_file_request*() loader tester

[Luis R. Rodriguez]

This adds a load tester driver test_sysdata a for the new
extensible sysdata file loader APIs, part of firmware_class.
Since the usermode helper is completely ignored by the sysdata
API the testing is much easier to do.

Contrary to the firmware_class tester which adds in-kernel
code for each and every single test it can think of for each
type of request, this enables you to build your tests in userspace
by exposing knobs of the exported API to userspace of the
options available in the API and then lets the trigger kick a one
time kernel API use. This lets us build any possible test case
in userspace.

The test driver also enables multiple test triggers
to be created enabling further testing to be done through
separate threads in parallel.

Both these facts should should not only help testing the
sysdata API in as many ways as possible as efficiently
as possible, but it also paves the way to later strive to
see how it might be even possible to automatically generate
test API drivers for exported symbols in the future. The
exported symbols being the test cases and attributes exposed
in userspace consisting of device attributes, the target test
driver being the desired output driver.

Signed-off-by: Luis R. Rodriguez <mcgrof@kernel.org>
---
 lib/Kconfig.debug                           |   12 +
 lib/Makefile                                |    1 +
 lib/test_sysdata.c                          | 1046 +++++++++++++++++++++++++++
 tools/testing/selftests/firmware/sysdata.sh |  633 ++++++++++++++++
 4 files changed, 1692 insertions(+)
 create mode 100644 lib/test_sysdata.c
 create mode 100755 tools/testing/selftests/firmware/sysdata.sh

diff --git a/lib/Kconfig.debug b/lib/Kconfig.debug
index 0f9981999a27..ecd60a40e646 100644
--- a/lib/Kconfig.debug
+++ b/lib/Kconfig.debug
@@ -1979,6 +1979,18 @@ config TEST_FIRMWARE
 
 	  If unsure, say N.
 
+config TEST_SYSDATA
+	tristate "Test system data loading via sysdata APIs"
+	default n
+	depends on FW_LOADER
+	help
+	  This builds the "test_sysdata" module that creates a userspace
+	  interface for testing system data file loading using the sysdata API.
+	  This can be used to control the triggering of system data file
+	  loading without needing an actual real device.
+
+	  If unsure, say N.
+
 config TEST_UDELAY
 	tristate "udelay test driver"
 	default n
diff --git a/lib/Makefile b/lib/Makefile
index f6455db094e3..c0d0e096a947 100644
--- a/lib/Makefile
+++ b/lib/Makefile
@@ -45,6 +45,7 @@ obj-$(CONFIG_TEST_HEXDUMP) += test_hexdump.o
 obj-y += kstrtox.o
 obj-$(CONFIG_TEST_BPF) += test_bpf.o
 obj-$(CONFIG_TEST_FIRMWARE) += test_firmware.o
+obj-$(CONFIG_TEST_SYSDATA) += test_sysdata.o
 obj-$(CONFIG_TEST_HASH) += test_hash.o
 obj-$(CONFIG_TEST_KASAN) += test_kasan.o
 obj-$(CONFIG_TEST_KSTRTOX) += test-kstrtox.o
diff --git a/lib/test_sysdata.c b/lib/test_sysdata.c
new file mode 100644
index 000000000000..3ad0e0c0f6c3
--- /dev/null
+++ b/lib/test_sysdata.c
@@ -0,0 +1,1046 @@
+/*
+ * System Data test interface
+ *
+ * Copyright (C) 2016 Luis R. Rodriguez <mcgrof@kernel.org>
+ *
+ * This program is free software; you can redistribute it and/or
+ * modify it under the terms of the GNU General Public Licence
+ * as published by the Free Software Foundation; either version
+ * 2 of the Licence, or (at your option) any later version.
+ *
+ * This module provides an interface to trigger and test system data API
+ * through a series of configurations and a few triggers. This driver
+ * lacks any extra dependencies, and will not normally be loaded by the
+ * system unless explicitly requested by name. You can also build this
+ * driver into your kernel.
+ *
+ * Although all configurations are already written for and will be supported
+ * for this test driver, ideally we should strive to see what mechanisms we
+ * can put in place to instead automatically generate this sort of test
+ * interface, test cases, and infer results. Its a simple enough interface that
+ * should hopefully enable more exploring in this area.
+ */
+#define pr_fmt(fmt) KBUILD_MODNAME ": " fmt
+
+#include <linux/init.h>
+#include <linux/list.h>
+#include <linux/module.h>
+#include <linux/printk.h>
+#include <linux/completion.h>
+#include <linux/sysdata.h>
+#include <linux/device.h>
+#include <linux/fs.h>
+#include <linux/miscdevice.h>
+#include <linux/slab.h>
+#include <linux/uaccess.h>
+#include <linux/async.h>
+#include <linux/delay.h>
+#include <linux/vmalloc.h>
+
+/* Used for the fallback default to test against */
+#define TEST_SYSDATA "test-sysdata.bin"
+
+/*
+ * For device allocation / registration
+ */
+static DEFINE_MUTEX(reg_dev_mutex);
+static LIST_HEAD(reg_test_devs);
+
+/*
+ * num_test_devs actually represents the *next* ID of the next
+ * device we will allow to create.
+ */
+int num_test_devs;
+
+/**
+ * test_config - represents configuration for the sysdata API
+ *
+ * @name: the name of the primary sysdata file to look for
+ * @default_name: a fallback example, used to test the optional callback
+ * 	mechanism.
+ * @async: true if you want to trigger an async request. This will use
+ * 	sysdata_file_request_async(). If false the synchronous call will
+ * 	be used, sysdata_file_request().
+ * @optional: whether or not the sysdata is optional refer to the
+ *	struct sysdata_file_desc @optional field for more information.
+ * @keep: whether or not we wish to free the sysdata on our own, refer to
+ *	the struct sysdata_file_desc @keep field 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
+ *	sysdata_file_desc as this is implementation specific, and in
+ *	in sysdata API its explicit if you had defined an optional call
+ *	back for your descriptor with either SYSDATA_SYNC_OPT_CB() or
+ *	SYSDATA_ASYNC_OPT_CB(). Since the descriptor is a const we have
+ *	no option but to use a flag and two const structs to decide which
+ *	one we should use.
+ * @test_result: a test may use this to collect the result from the call
+ *	of the sysdata_file_request_async() or sysdata_file_request() calls
+ *	used in their tests. Note that for async calls this typically will
+ *	be a successful result (0) unless of course you've have sent in
+ *	a bogus descriptor, or the system is out of memory. Tests against
+ *	the callbacks can only be implementation specific, so we don't
+ *	test for that for now but it may make sense to build tests cases
+ *	against a series of semantically similar family of callbacks that
+ *	generally represents usage in the kernel. Synchronous calls return
+ *	bogus error checks against the descriptor as well, but also return
+ *	the result of the work from the callbacks. You can therefore rely
+ *	on sync calls if you really want to test for the callback results
+ *	as well. Errors you can expect:
+ *
+ *	API specific:
+ *
+ *	0:		success for sync, for async it means request was sent
+ *	-EINVAL:	invalid descriptor or request
+ *	-ENOENT:	files not found
+ *
+ *	System environment:
+ *
+ *	-ENOMEM:	memory pressure on system
+ *	-ENODEV:	out of number of devices to test
+ *
+ * The ordering of elements in this struct must match the exact order of the
+ * elements in the ATTRIBUTE_GROUPS(test_dev_config), this is done to know
+ * what corresponding field each device attribute configuration entry maps
+ * to what struct member on test_alloc_dev_attrs().
+ */
+struct test_config {
+	char *name;
+	char *default_name;
+	bool async;
+	bool optional;
+	bool keep;
+	bool enable_opt_cb;
+
+	int test_result;
+};
+
+/**
+ * test_sysdata_private - private device driver sysdata representation
+ *
+ * @size: size of the data copied, in bytes
+ * @data: the actual data we copied over from sysdata
+ * @written: true if a callback managed to copy data over to the device
+ *	successfully. Since different callbacks are used for this purpose
+ *	having the data written does not necessarily mean a test case
+ *	completed successfully. Each tests case has its own specific
+ *	goals.
+ *
+ * Private representation of buffer where we put the device system data */
+struct test_sysdata_private {
+	size_t size;
+	u8 *data;
+	bool written;
+};
+
+/**
+ * sysdata_test_device - test device to help test sysdata
+ *
+ * @dev_idx: unique ID for test device
+ * @config: this keeps the device's own configuration. Instead of creating
+ *	different triggers for all possible test cases we can think of in
+ *	kernel, we expose a set possible device attributes for tuning the
+ *	sysdata API and we to let you tune them in userspace. We then just
+ *	provide one trigger.
+ * @test_sysdata: internal private representation of a storage area
+ *	a driver might typically use to stuff firmware / sysdata.
+ * @misc_dev: we use a misc device under the hood
+ * @dev: pointer to misc_dev's own struct device
+ * @sysdata_mutex: for access into the @sysdata, the fake storage location for
+ * 	the system data we copy.
+ * @config_mutex:
+ * @trigger_mutex: all triggers are mutually exclusive when testing. To help
+ *	enable testing you can create a different device, each device has its
+ *	own set of protections, mimicking real devices.
+ * list: needed to be part of the reg_test_devs
+ */
+struct sysdata_test_device {
+	int dev_idx;
+	struct test_config config;
+	struct test_sysdata_private test_sysdata;
+	struct miscdevice misc_dev;
+	struct device *dev;
+
+	struct mutex sysdata_mutex;
+	struct mutex config_mutex;
+	struct mutex trigger_mutex;
+	struct list_head list;
+};
+
+static struct miscdevice *dev_to_misc_dev(struct device *dev)
+{
+	return dev_get_drvdata(dev);
+}
+
+static
+struct sysdata_test_device *misc_dev_to_test_dev(struct miscdevice *misc_dev)
+{
+	return container_of(misc_dev, struct sysdata_test_device, misc_dev);
+}
+
+static struct sysdata_test_device *dev_to_test_dev(struct device *dev)
+{
+	struct miscdevice *misc_dev;
+
+	misc_dev = dev_to_misc_dev(dev);
+
+	return misc_dev_to_test_dev(misc_dev);
+}
+
+static ssize_t test_fw_misc_read(struct file *f, char __user *buf,
+				 size_t size, loff_t *offset)
+{
+	struct miscdevice *misc_dev = f->private_data;
+	struct sysdata_test_device *test_dev = misc_dev_to_test_dev(misc_dev);
+	struct test_sysdata_private *test_sysdata = &test_dev->test_sysdata;
+	ssize_t rc = 0;
+
+	mutex_lock(&test_dev->sysdata_mutex);
+	if (test_sysdata->written)
+		rc = simple_read_from_buffer(buf, size, offset,
+					     test_sysdata->data,
+					     test_sysdata->size);
+	mutex_unlock(&test_dev->sysdata_mutex);
+
+	return rc;
+}
+
+static const struct file_operations test_fw_fops = {
+	.owner          = THIS_MODULE,
+	.read           = test_fw_misc_read,
+};
+
+static void free_test_sysdata(struct test_sysdata_private *test_sysdata)
+{
+	kfree(test_sysdata->data);
+	test_sysdata->data = NULL;
+	test_sysdata->size = 0;
+	test_sysdata->written = false;
+}
+
+static int test_load_sysdata(struct sysdata_test_device *test_dev,
+			     const struct sysdata_file *sysdata)
+{
+	struct test_sysdata_private *test_sysdata = &test_dev->test_sysdata;
+	int ret = 0;
+
+	if (!sysdata)
+		return -ENOENT;
+
+	mutex_lock(&test_dev->sysdata_mutex);
+
+	free_test_sysdata(test_sysdata);
+
+	test_sysdata->data = kzalloc(sysdata->size, GFP_KERNEL);
+	if (!test_sysdata->data) {
+		ret = -ENOMEM;
+		goto out;
+	}
+
+	memcpy(test_sysdata->data, sysdata->data, sysdata->size);
+	test_sysdata->size = sysdata->size;
+	test_sysdata->written = true;
+
+	dev_info(test_dev->dev, "loaded: %zu\n", test_sysdata->size);
+
+out:
+	mutex_unlock(&test_dev->sysdata_mutex);
+
+	return ret;
+}
+
+static int sync_found_cb(void *context, const struct sysdata_file *sysdata)
+{
+	struct sysdata_test_device *test_dev = context;
+	int ret;
+
+	ret = test_load_sysdata(test_dev, sysdata);
+	if (ret)
+		dev_info(test_dev->dev, "unable to write sysdata: %d\n", ret);
+	return ret;
+}
+
+static ssize_t config_show(struct device *dev,
+			   struct device_attribute *attr,
+			   char *buf)
+{
+	struct sysdata_test_device *test_dev = dev_to_test_dev(dev);
+	struct test_config *config = &test_dev->config;
+	int len = 0;
+
+	mutex_lock(&test_dev->config_mutex);
+
+	len += sprintf(buf, "Custom trigger configuration for: %s\n",
+		       dev_name(dev));
+
+	if (config->default_name)
+		len += sprintf(buf+len, "default name:\t%s\n",
+			       config->default_name);
+	else
+		len += sprintf(buf+len, "default name:\tEMTPY\n");
+
+	if (config->name)
+		len += sprintf(buf+len, "name:\t\t%s\n", config->name);
+	else
+		len += sprintf(buf+len, "name:\t\tEMPTY\n");
+
+	len += sprintf(buf+len, "type:\t\t%s\n",
+		       config->async ? "async" : "sync");
+	len += sprintf(buf+len, "optional:\t%s\n",
+		       config->optional ? "true" : "false");
+	len += sprintf(buf+len, "enable_opt_cb:\t%s\n",
+		       config->enable_opt_cb ? "true" : "false");
+	len += sprintf(buf+len, "keep:\t\t%s\n",
+		       config->keep ? "true" : "false");
+
+	mutex_unlock(&test_dev->config_mutex);
+
+	return len;
+}
+static DEVICE_ATTR_RO(config);
+
+static int config_load_data(struct sysdata_test_device *test_dev,
+			    const struct sysdata_file *sysdata)
+{
+	struct test_config *config = &test_dev->config;
+	int ret;
+
+	ret = test_load_sysdata(test_dev, sysdata);
+	if (ret) {
+		if (!config->optional)
+			dev_info(test_dev->dev, "unable to write sysdata\n");
+	}
+	if (config->keep) {
+		release_sysdata_file(sysdata);
+		sysdata = NULL;
+	}
+	return ret;
+}
+
+static int config_req_default(struct sysdata_test_device *test_dev)
+{
+	struct test_config *config = &test_dev->config;
+	int rc;
+	/*
+	 * Note: we don't chain config->optional here, we make this
+	 * fallback file a requirement. It doesn't make much sense to test
+	 * chaining further as the optional callback is implementation
+	 * specific, by testing it once we test it for any possible
+	 * chains. We provide this as an example of what people can do
+	 * and use a default non-optional fallback.
+	 */
+	const struct sysdata_file_desc sysdata_desc = {
+		SYSDATA_DEFAULT_SYNC(sync_found_cb, test_dev),
+	};
+
+	if (config->async)
+		dev_info(test_dev->dev,
+			 "loading default fallback '%s' using sync request now\n",
+			 config->default_name);
+	else
+		dev_info(test_dev->dev,
+			 "loading default fallback '%s'\n",
+			config->default_name);
+
+	rc = sysdata_file_request(config->default_name,
+				  &sysdata_desc, test_dev->dev);
+	if (rc)
+		dev_info(test_dev->dev,
+			 "load of default '%s' failed: %d\n",
+			 config->default_name, rc);
+
+	return rc;
+}
+
+/*
+ * This is the default sync fallback callback, as a fallback this
+ * then uses a sync request.
+ */
+static int config_sync_req_default_cb(void *context)
+{
+	struct sysdata_test_device *test_dev = context;
+	int rc;
+
+	rc = config_req_default(test_dev);
+
+	return rc;
+
+	/* Leave all the error checking for the main caller */
+}
+
+/*
+ * This is the default config->async fallback callback, as a fallback this
+ * then uses a sync request.
+ */
+static void config_async_req_default_cb(void *context)
+{
+	struct sysdata_test_device *test_dev = context;
+
+	config_req_default(test_dev);
+
+	/* Leave all the error checking for the main caller */
+}
+
+static int config_sync_req_cb(void *context,
+			      const struct sysdata_file *sysdata)
+{
+	struct sysdata_test_device *test_dev = context;
+
+	return config_load_data(test_dev, sysdata);
+}
+
+static int trigger_config_sync(struct sysdata_test_device *test_dev)
+{
+	struct test_config *config = &test_dev->config;
+	int rc;
+	const struct sysdata_file_desc sysdata_desc_default = {
+		SYSDATA_DEFAULT_SYNC(config_sync_req_cb, test_dev),
+		.optional = config->optional,
+		.keep = config->keep,
+	};
+	const struct sysdata_file_desc sysdata_desc_opt_cb = {
+		SYSDATA_DEFAULT_SYNC(config_sync_req_cb, test_dev),
+		SYSDATA_SYNC_OPT_CB(config_sync_req_default_cb, test_dev),
+		.optional = config->optional,
+		.keep = config->keep,
+	};
+	const struct sysdata_file_desc *sysdata_desc;
+
+	if (config->enable_opt_cb)
+		sysdata_desc = &sysdata_desc_opt_cb;
+	else
+		sysdata_desc = &sysdata_desc_default;
+
+	rc = sysdata_file_request(config->name, sysdata_desc, test_dev->dev);
+	if (rc)
+		dev_err(test_dev->dev, "sync load of '%s' failed: %d\n",
+			config->name, rc);
+
+	return rc;
+}
+
+static void config_async_req_cb(const struct sysdata_file *sysdata,
+				void *context)
+{
+	struct sysdata_test_device *test_dev = context;
+	config_load_data(test_dev, sysdata);
+}
+
+static int trigger_config_async(struct sysdata_test_device *test_dev)
+{
+	struct test_config *config = &test_dev->config;
+	int rc;
+	const struct sysdata_file_desc sysdata_desc_default = {
+		SYSDATA_DEFAULT_ASYNC(config_async_req_cb, test_dev),
+		.sync_reqs.mode = config->async ?
+			SYNCDATA_ASYNC : SYNCDATA_SYNC,
+		.optional = config->optional,
+		.keep = config->keep,
+	};
+	const struct sysdata_file_desc sysdata_desc_opt_cb = {
+		SYSDATA_DEFAULT_ASYNC(config_async_req_cb, test_dev),
+		SYSDATA_ASYNC_OPT_CB(config_async_req_default_cb, test_dev),
+		.sync_reqs.mode = config->async ?
+			SYNCDATA_ASYNC : SYNCDATA_SYNC,
+		.optional = config->optional,
+		.keep = config->keep,
+	};
+	const struct sysdata_file_desc *sysdata_desc;
+	async_cookie_t async_cookie;
+
+	if (config->enable_opt_cb)
+		sysdata_desc = &sysdata_desc_opt_cb;
+	else
+		sysdata_desc = &sysdata_desc_default;
+
+	rc = sysdata_file_request_async(config->name, sysdata_desc,
+					test_dev->dev,
+					&async_cookie);
+	if (rc) {
+		dev_err(test_dev->dev, "async load of '%s' failed: %d\n",
+			config->name, rc);
+		goto out;
+	}
+
+	sysdata_synchronize_request(async_cookie);
+out:
+	return rc;
+}
+
+static ssize_t
+trigger_config_store(struct device *dev,
+		     struct device_attribute *attr,
+		     const char *buf, size_t count)
+{
+	struct sysdata_test_device *test_dev = dev_to_test_dev(dev);
+	struct test_sysdata_private *test_sysdata = &test_dev->test_sysdata;
+	struct test_config *config = &test_dev->config;
+	int rc;
+
+	mutex_lock(&test_dev->trigger_mutex);
+	mutex_lock(&test_dev->config_mutex);
+
+	dev_info(dev, "loading '%s'\n", config->name);
+
+	if (config->async)
+		rc = trigger_config_async(test_dev);
+	else
+		rc = trigger_config_sync(test_dev);
+
+	config->test_result = rc;
+
+	if (rc)
+		goto out;
+
+	if (test_sysdata->written) {
+		dev_info(dev, "loaded: %zu\n", test_sysdata->size);
+		rc = count;
+	} else {
+		dev_err(dev, "failed to load firmware\n");
+		rc = -ENODEV;
+	}
+
+out:
+	mutex_unlock(&test_dev->config_mutex);
+	mutex_unlock(&test_dev->trigger_mutex);
+
+	return rc;
+}
+static DEVICE_ATTR_WO(trigger_config);
+
+/*
+ * XXX: move to kstrncpy() once merged.
+ *
+ * Users should use kfree_const() when freeing these.
+ */
+static int __kstrncpy(char **dst, const char *name, size_t count, gfp_t gfp)
+{
+	*dst = kstrndup(name, count, gfp);
+	if (!*dst)
+		return -ENOSPC;
+	return count;
+}
+
+static int config_copy_name(struct test_config *config,
+			    const char *name,
+			    size_t count)
+{
+	return __kstrncpy(&config->name, name, count, GFP_KERNEL);
+}
+
+static int config_copy_default_name(struct test_config *config,
+				    const char *name,
+				    size_t count)
+{
+	return __kstrncpy(&config->default_name, name, count, GFP_KERNEL);
+}
+
+static void __sysdata_config_free(struct test_config *config)
+{
+	kfree_const(config->name);
+	config->name = NULL;
+	kfree_const(config->default_name);
+	config->default_name = NULL;
+}
+
+static void sysdata_config_free(struct sysdata_test_device *test_dev)
+{
+	struct test_config *config = &test_dev->config;
+
+	mutex_lock(&test_dev->config_mutex);
+	__sysdata_config_free(config);
+	mutex_unlock(&test_dev->config_mutex);
+}
+
+static int __sysdata_config_init(struct test_config *config)
+{
+	int ret;
+
+	ret = config_copy_name(config, TEST_SYSDATA, strlen(TEST_SYSDATA));
+	if (ret < 0)
+		goto out;
+
+	ret = config_copy_default_name(config, TEST_SYSDATA,
+				       strlen(TEST_SYSDATA));
+	if (ret < 0)
+		goto out;
+
+	config->async = false;
+	config->optional = false;
+	config->keep = false;
+	config->enable_opt_cb = false;
+	config->test_result = 0;
+
+out:
+	return ret;
+}
+
+int sysdata_config_init(struct sysdata_test_device *test_dev)
+{
+	struct test_config *config = &test_dev->config;
+	int ret;
+
+	mutex_lock(&test_dev->config_mutex);
+	ret = __sysdata_config_init(config);
+	mutex_unlock(&test_dev->config_mutex);
+
+	return ret;
+}
+
+static ssize_t config_name_store(struct device *dev,
+				 struct device_attribute *attr,
+				 const char *buf, size_t count)
+{
+	struct sysdata_test_device *test_dev = dev_to_test_dev(dev);
+	struct test_config *config = &test_dev->config;
+	int rc;
+
+	mutex_lock(&test_dev->config_mutex);
+	rc = config_copy_name(config, buf, count);
+	mutex_unlock(&test_dev->config_mutex);
+
+	return rc;
+}
+
+static ssize_t config_name_show(struct device *dev,
+				struct device_attribute *attr,
+				char *buf)
+{
+	struct sysdata_test_device *test_dev = dev_to_test_dev(dev);
+	struct test_config *config = &test_dev->config;
+
+	mutex_lock(&test_dev->config_mutex);
+	strcpy(buf, config->name);
+	strcat(buf, "\n");
+	mutex_unlock(&test_dev->config_mutex);
+
+	return strlen(buf) + 1;
+}
+static DEVICE_ATTR(config_name, 0644, config_name_show, config_name_store);
+
+static ssize_t config_default_name_store(struct device *dev,
+					 struct device_attribute *attr,
+					 const char *buf, size_t count)
+{
+	struct sysdata_test_device *test_dev = dev_to_test_dev(dev);
+	struct test_config *config = &test_dev->config;
+	int rc;
+
+	mutex_lock(&test_dev->config_mutex);
+	rc = config_copy_default_name(config, buf, count);
+	mutex_unlock(&test_dev->config_mutex);
+
+	return rc;
+}
+
+static ssize_t config_default_name_show(struct device *dev,
+					struct device_attribute *attr,
+					char *buf)
+{
+	struct sysdata_test_device *test_dev = dev_to_test_dev(dev);
+	struct test_config *config = &test_dev->config;
+
+	mutex_lock(&test_dev->config_mutex);
+	strcpy(buf, config->default_name);
+	strcat(buf, "\n");
+	mutex_unlock(&test_dev->config_mutex);
+
+	return strlen(buf) + 1;
+}
+static DEVICE_ATTR(config_default_name, 0644, config_default_name_show,
+		   config_default_name_store);
+
+static ssize_t reset_store(struct device *dev,
+			   struct device_attribute *attr,
+			   const char *buf, size_t count)
+{
+	struct sysdata_test_device *test_dev = dev_to_test_dev(dev);
+	struct test_config *config = &test_dev->config;
+	int ret;
+
+	mutex_lock(&test_dev->trigger_mutex);
+
+	mutex_lock(&test_dev->sysdata_mutex);
+	free_test_sysdata(&test_dev->test_sysdata);
+	mutex_unlock(&test_dev->sysdata_mutex);
+
+	mutex_lock(&test_dev->config_mutex);
+
+	__sysdata_config_free(config);
+
+	ret = __sysdata_config_init(config);
+	if (ret < 0) {
+		ret = -ENOMEM;
+		dev_err(dev, "could not alloc settings for config trigger: %d\n",
+		       ret);
+		goto out;
+	}
+
+	dev_info(dev, "reset\n");
+	ret = count;
+
+out:
+	mutex_unlock(&test_dev->config_mutex);
+	mutex_unlock(&test_dev->trigger_mutex);
+
+	return ret;
+}
+static DEVICE_ATTR_WO(reset);
+
+/*
+ * XXX: consider a soluton to generalize drivers to specify their own
+ * mutex, adding it to dev core after this gets merged. This may not
+ * be important for once-in-a-while system tuning parameters, but if
+ * we want to enable fuzz testing, this is really important.
+ *
+ * It may make sense to just have a "struct device configuration mutex"
+ * for these sorts of things, although there is difficulty in that we'd
+ * need dynamically allocated attributes for that. Its the same reason
+ * why we ended up not using the provided standard device attribute
+ * bool, int interfaces.
+ */
+
+static int test_dev_config_update_bool(struct sysdata_test_device *test_dev,
+				       const char *buf, size_t size,
+				       bool *config)
+{
+	int ret;
+
+	mutex_lock(&test_dev->config_mutex);
+	if (strtobool(buf, config) < 0)
+		ret = -EINVAL;
+	else
+		ret = size;
+	mutex_unlock(&test_dev->config_mutex);
+
+	return ret;
+}
+
+static ssize_t test_dev_config_show_bool(struct sysdata_test_device *test_dev,
+					 char *buf,
+					 bool config)
+{
+	bool val;
+
+	mutex_lock(&test_dev->config_mutex);
+	val = config;
+	mutex_unlock(&test_dev->config_mutex);
+
+	return snprintf(buf, PAGE_SIZE, "%d\n", val);
+}
+
+static int test_dev_config_update_int(struct sysdata_test_device *test_dev,
+				      const char *buf, size_t size,
+				      int *config)
+{
+	char *end;
+	long new = simple_strtol(buf, &end, 0);
+	if (end == buf || new > INT_MAX || new < INT_MIN)
+		return -EINVAL;
+	mutex_lock(&test_dev->config_mutex);
+	*(int *)config = new;
+	mutex_unlock(&test_dev->config_mutex);
+	/* Always return full write size even if we didn't consume all */
+	return size;
+}
+
+static ssize_t test_dev_config_show_int(struct sysdata_test_device *test_dev,
+					char *buf,
+					int config)
+{
+	int val;
+
+	mutex_lock(&test_dev->config_mutex);
+	val = config;
+	mutex_unlock(&test_dev->config_mutex);
+
+	return snprintf(buf, PAGE_SIZE, "%d\n", val);
+}
+
+static ssize_t config_async_store(struct device *dev,
+				  struct device_attribute *attr,
+				  const char *buf, size_t count)
+{
+	struct sysdata_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->async);
+}
+
+static ssize_t config_async_show(struct device *dev,
+				 struct device_attribute *attr,
+				 char *buf)
+{
+	struct sysdata_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->async);
+}
+static DEVICE_ATTR(config_async, 0644, config_async_show, config_async_store);
+
+static ssize_t config_optional_store(struct device *dev,
+				     struct device_attribute *attr,
+				     const char *buf, size_t count)
+{
+	struct sysdata_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->optional);
+}
+
+static ssize_t config_optional_show(struct device *dev,
+				    struct device_attribute *attr,
+				    char *buf)
+{
+	struct sysdata_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->optional);
+}
+static DEVICE_ATTR(config_optional, 0644, config_optional_show,
+		   config_optional_store);
+
+static ssize_t config_keep_store(struct device *dev,
+				 struct device_attribute *attr,
+				 const char *buf, size_t count)
+{
+	struct sysdata_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->keep);
+}
+
+static ssize_t config_keep_show(struct device *dev,
+				struct device_attribute *attr,
+				char *buf)
+{
+	struct sysdata_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->keep);
+}
+static DEVICE_ATTR(config_keep, 0644, config_keep_show, config_keep_store);
+
+static ssize_t config_enable_opt_cb_store(struct device *dev,
+					  struct device_attribute *attr,
+					  const char *buf, size_t count)
+{
+	struct sysdata_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->enable_opt_cb);
+}
+
+static ssize_t config_enable_opt_cb_show(struct device *dev,
+					 struct device_attribute *attr,
+					 char *buf)
+{
+	struct sysdata_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->enable_opt_cb);
+}
+static DEVICE_ATTR(config_enable_opt_cb, 0644,
+		   config_enable_opt_cb_show,
+		   config_enable_opt_cb_store);
+
+static ssize_t test_result_store(struct device *dev,
+				 struct device_attribute *attr,
+				 const char *buf, size_t count)
+{
+	struct sysdata_test_device *test_dev = dev_to_test_dev(dev);
+	struct test_config *config = &test_dev->config;
+
+	return test_dev_config_update_int(test_dev, buf, count,
+					  &config->test_result);
+}
+
+static ssize_t test_result_show(struct device *dev,
+				struct device_attribute *attr,
+				char *buf)
+{
+	struct sysdata_test_device *test_dev = dev_to_test_dev(dev);
+	struct test_config *config = &test_dev->config;
+
+	return test_dev_config_show_int(test_dev, buf, config->test_result);
+}
+static DEVICE_ATTR(test_result, 0644, test_result_show, test_result_store);
+
+#define SYSDATA_DEV_ATTR(name)		&dev_attr_##name.attr
+
+static struct attribute *test_dev_attrs[] = {
+	SYSDATA_DEV_ATTR(trigger_config),
+	SYSDATA_DEV_ATTR(config),
+	SYSDATA_DEV_ATTR(reset),
+
+	SYSDATA_DEV_ATTR(config_name),
+	SYSDATA_DEV_ATTR(config_default_name),
+	SYSDATA_DEV_ATTR(config_async),
+	SYSDATA_DEV_ATTR(config_optional),
+	SYSDATA_DEV_ATTR(config_keep),
+	SYSDATA_DEV_ATTR(config_enable_opt_cb),
+	SYSDATA_DEV_ATTR(test_result),
+
+	NULL,
+};
+
+ATTRIBUTE_GROUPS(test_dev);
+
+/*
+ * XXX: this could perhaps be made generic already too, but a hunt
+ * for actual users would be needed first. It could be generic
+ * if other test drivers end up using a similar mechanism.
+ */
+const char *test_dev_get_name(const char *base, int idx, gfp_t gfp)
+{
+	const char *name_const;
+	char *name;
+
+	if (!base)
+		return NULL;
+	if (strlen(base) > 30)
+		return NULL;
+	name = kzalloc(1024, gfp);
+	if (!name)
+		return NULL;
+
+	strncat(name, base, strlen(base));
+	sprintf(name+(strlen(base)), "%d", idx);
+	name_const = kstrdup_const(name, gfp);
+
+	kfree(name);
+
+	return name_const;
+}
+
+void free_test_dev_sysdata(struct sysdata_test_device *test_dev)
+{
+	kfree_const(test_dev->misc_dev.name);
+	test_dev->misc_dev.name = NULL;
+	vfree(test_dev);
+	test_dev = NULL;
+	sysdata_config_free(test_dev);
+}
+
+void unregister_test_dev_sysdata(struct sysdata_test_device *test_dev)
+{
+	dev_info(test_dev->dev, "removing interface\n");
+	misc_deregister(&test_dev->misc_dev);
+	free_test_dev_sysdata(test_dev);
+}
+
+struct sysdata_test_device *alloc_test_dev_sysdata(int idx)
+{
+	int rc;
+	struct sysdata_test_device *test_dev;
+	struct miscdevice *misc_dev;
+
+	test_dev = vmalloc(sizeof(struct sysdata_test_device));
+	if (!test_dev) {
+		pr_err("Cannot alloc test_dev\n");
+		goto err_out;
+	}
+
+	mutex_init(&test_dev->sysdata_mutex);
+	mutex_init(&test_dev->config_mutex);
+	mutex_init(&test_dev->trigger_mutex);
+
+	rc = sysdata_config_init(test_dev);
+	if (rc < 0) {
+		pr_err("Cannot alloc sysdata_config_init()\n");
+		goto err_out_free;
+	}
+
+	test_dev->dev_idx = idx;
+	misc_dev = &test_dev->misc_dev;
+
+	misc_dev->minor = MISC_DYNAMIC_MINOR;
+	misc_dev->name = test_dev_get_name("test_sysdata", test_dev->dev_idx,
+					   GFP_KERNEL);
+	if (!misc_dev->name) {
+		pr_err("Cannot alloc misc_dev->name\n");
+		goto err_out_free_config;
+	}
+	misc_dev->fops = &test_fw_fops;
+	misc_dev->groups = test_dev_groups;
+
+	return test_dev;
+
+err_out_free_config:
+	__sysdata_config_free(&test_dev->config);
+err_out_free:
+	kfree(test_dev);
+err_out:
+	return NULL;
+}
+
+static int register_test_dev_sysdata(void)
+{
+	struct sysdata_test_device *test_dev = NULL;
+	int rc = -ENODEV;
+
+	mutex_lock(&reg_dev_mutex);
+
+	/* int should suffice for number of devices, test for wrap */
+	if (unlikely(num_test_devs + 1) < 0) {
+		pr_err("reached limit of number of test devices\n");
+		goto out;
+	}
+
+	test_dev = alloc_test_dev_sysdata(num_test_devs);
+	if (!test_dev) {
+		rc = -ENOMEM;
+		goto out;
+	}
+
+	rc = misc_register(&test_dev->misc_dev);
+	if (rc) {
+		pr_err("could not register misc device: %d\n", rc);
+		free_test_dev_sysdata(test_dev);
+		return rc;
+	}
+
+	test_dev->dev = test_dev->misc_dev.this_device;
+	list_add_tail(&test_dev->list, &reg_test_devs);
+	dev_info(test_dev->dev, "interface ready\n");
+
+	num_test_devs++;
+
+	mutex_unlock(&reg_dev_mutex);
+
+out:
+	return rc;
+}
+
+static int __init test_sysdata_init(void)
+{
+	int rc;
+
+	rc = register_test_dev_sysdata();
+	if (rc)
+		pr_err("Cannot add first test sysdata device\n");
+
+	return rc;
+}
+late_initcall(test_sysdata_init);
+
+static void __exit test_sysdata_exit(void)
+{
+	struct sysdata_test_device *test_dev, *tmp;
+
+	mutex_lock(&reg_dev_mutex);
+	list_for_each_entry_safe(test_dev, tmp, &reg_test_devs, list) {
+		list_del(&test_dev->list);
+		unregister_test_dev_sysdata(test_dev);
+	}
+	mutex_unlock(&reg_dev_mutex);
+}
+
+module_exit(test_sysdata_exit);
+
+MODULE_AUTHOR("Luis R. Rodriguez <mcgrof@kernel.org>");
+MODULE_LICENSE("GPL");
diff --git a/tools/testing/selftests/firmware/sysdata.sh b/tools/testing/selftests/firmware/sysdata.sh
new file mode 100755
index 000000000000..7bc8f9ff8f7d
--- /dev/null
+++ b/tools/testing/selftests/firmware/sysdata.sh
@@ -0,0 +1,633 @@
+#!/bin/bash
+
+# This performs a series tests against firmware_class to excercise the
+# firmware_class driver with focus only on the extensible system data API.
+#
+# To make this test self contained, and note pollute your distribution
+# firmware install paths, we reset the custom load directory to a
+# temporary location.
+
+set -e
+
+DIR=/sys/devices/virtual/misc/test_sysdata0/
+
+if [ ! -d $DIR ]; then
+	modprobe test_sysdata
+	if [ ! -d $DIR ]; then
+		echo "$0: $DIR not present"
+		echo "You must have CONFIG_TEST_FIRMWARE=m or CONFIG_TEST_FIRMWARE=y"
+		exit 1
+	fi
+fi
+
+OLD_FWPATH=$(cat /sys/module/firmware_class/parameters/path)
+
+FWPATH=$(mktemp -d)
+DEFAULT_SYSDATA="test-sysdata.bin"
+FW="$FWPATH/$DEFAULT_SYSDATA"
+
+test_reqs()
+{
+	if ! which diff 2> /dev/null > /dev/null; then
+		echo "$0: You need diff installed"
+		exit 1
+	fi
+}
+
+test_finish()
+{
+	echo -n "$OLD_PATH" >/sys/module/firmware_class/parameters/path
+	rm -f "$FW"
+	rmdir "$FWPATH"
+}
+
+trap "test_finish" EXIT
+
+# Set the kernel search path.
+echo -n "$FWPATH" >/sys/module/firmware_class/parameters/path
+
+# This is an unlikely real-world firmware content. :)
+echo "ABCD0123" >"$FW"
+
+NAME=$(basename "$FW")
+
+errno_name_to_val()
+{
+	case "$1" in
+	SUCCESS)
+		echo 0;;
+	-EPERM)
+		echo -1;;
+	-ENOENT)
+		echo -2;;
+	-EINVAL)
+		echo -22;;
+	-ERR_ANY)
+		echo -123456;;
+	*)
+		echo invalid;;
+	esac
+}
+
+errno_val_to_name()
+	case "$1" in
+	0)
+		echo SUCCESS;;
+	-1)
+		echo -EPERM;;
+	-2)
+		echo -ENOENT;;
+	-22)
+		echo -EINVAL;;
+	-123456)
+		echo -ERR_ANY;;
+	*)
+		echo invalid;;
+	esac
+
+config_set_async()
+{
+	if ! echo -n 1 >$DIR/config_async ; then
+		echo "$0: Unable to set to async" >&2
+		exit 1
+	fi
+}
+
+config_disable_async()
+{
+	if ! echo -n 0 >$DIR/config_async ; then
+		echo "$0: Unable to set to sync" >&2
+		exit 1
+	fi
+}
+
+config_set_optional()
+{
+	if ! echo -n 1 >$DIR/config_optional ; then
+		echo "$0: Unable to set to optional" >&2
+		exit 1
+	fi
+}
+
+config_disable_optional()
+{
+	if ! echo -n 0 >$DIR/config_optional ; then
+		echo "$0: Unable to disable optional" >&2
+		exit 1
+	fi
+}
+
+config_set_keep()
+{
+	if ! echo -n 1 >$DIR/config_keep; then
+		echo "$0: Unable to set to keep" >&2
+		exit 1
+	fi
+}
+
+config_disable_keep()
+{
+	if ! echo -n 0 >$DIR/config_keep; then
+		echo "$0: Unable to disable keep option" >&2
+		exit 1
+	fi
+}
+
+config_enable_opt_cb()
+{
+	if ! echo -n 1 >$DIR/config_enable_opt_cb; then
+		echo "$0: Unable to set to optional" >&2
+		exit 1
+	fi
+}
+
+config_disable_opt_cb()
+{
+	if ! echo -n 0 >$DIR/config_enable_opt_cb; then
+		echo "$0: Unable to disable keep option" >&2
+		exit 1
+	fi
+}
+
+
+# For special characters use printf directly,
+# refer to sysdata_test_0001
+config_set_name()
+{
+	if ! echo -n $1 >$DIR/config_name; then
+		echo "$0: Unable to set name" >&2
+		exit 1
+	fi
+}
+
+config_get_name()
+{
+	cat $DIR/config_name
+}
+
+# For special characters use printf directly,
+# refer to sysdata_test_0001
+config_set_default_name()
+{
+	if ! echo -n $1 >$DIR/config_default_name; then
+		echo "$0: Unable to set default_name" >&2
+		exit 1
+	fi
+}
+
+config_get_default_name()
+{
+	cat $DIR/config_default_name
+}
+
+config_get_test_result()
+{
+	cat $DIR/test_result
+}
+
+config_reset()
+{
+	if ! echo -n "1" >"$DIR"/reset; then
+		echo "$0: reset shuld have worked" >&2
+		exit 1
+	fi
+}
+
+config_show_config()
+{
+	echo "----------------------------------------------------"
+	cat "$DIR"/config
+	echo "----------------------------------------------------"
+}
+
+config_trigger()
+{
+	if ! echo -n "1" >"$DIR"/trigger_config 2>/dev/null; then
+		echo "$1: FAIL - loading should have worked"
+		config_show_config
+		exit 1
+	fi
+	echo "$1: OK! - loading sysdata"
+}
+
+config_trigger_want_fail()
+{
+	if echo "1" > $DIR/trigger_config 2>/dev/null; then
+		echo "$1: FAIL - loading was expected to fail"
+		config_show_config
+		exit 1
+	fi
+	echo "$1: OK! - loading failed as expected"
+}
+
+config_file_should_match()
+{
+	FILE=$(config_get_name)
+	# On this one we expect the file to exist so leave stderr in
+	if ! $(diff -q "$FWPATH"/"$FILE" /dev/test_sysdata0 > /dev/null) > /dev/null; then
+		echo "$1: FAIL - file $FILE did not match contents in /dev/test_sysdata0" >&2
+		config_show_config
+		exit 1
+	fi
+	echo "$1: OK! - $FILE == /dev/test_sysdata0"
+}
+
+config_file_should_match_default()
+{
+	FILE=$(config_get_default_name)
+	# On this one we expect the file to exist so leave stderr in
+	if ! $(diff -q "$FWPATH"/"$FILE" /dev/test_sysdata0 > /dev/null) > /dev/null; then
+		echo "$1: FAIL - file $FILE did not match contents in /dev/test_sysdata0" >&2
+		config_show_config
+		exit 1
+	fi
+	echo "$1: OK! - $FILE == /dev/test_sysdata0"
+}
+
+config_file_should_not_match()
+{
+	FILE=$(config_get_name)
+	# File may not exist, so skip those error messages as well
+	if $(diff -q $FWPATH/$FILE /dev/test_sysdata0 2> /dev/null) 2> /dev/null ; then
+		echo "$1: FAIL - file $FILE was not expected to match /dev/null" >&2
+		config_show_config
+		exit 1
+	fi
+	echo "$1: OK! - $FILE != /dev/test_sysdata0"
+}
+
+config_default_file_should_match()
+{
+	FILE=$(config_get_default_name)
+	diff -q $FWPATH/$FILE /dev/test_sysdata0 2> /dev/null
+	if ! $? ; then
+		echo "$1: FAIL - file $FILE expected to match /dev/test_sysdata0" >&2
+		config_show_config
+		exit 1
+	fi
+	echo "$1: OK! [file integrity matches]"
+}
+
+config_default_file_should_not_match()
+{
+	FILE=$(config_get_default_name)
+	diff -q FWPATH/$FILE /dev/test_sysdata0 2> /dev/null
+	if $? 2> /dev/null ; then
+		echo "$1: FAIL - file $FILE was not expected to match test_sysdata0" >&2
+		config_show_config
+		exit 1
+	fi
+	echo "$1: OK!"
+}
+
+config_expect_result()
+{
+	RC=$(config_get_test_result)
+	RC_NAME=$(errno_val_to_name $RC)
+
+	ERRNO_NAME=$2
+	ERRNO=$(errno_name_to_val $ERRNO_NAME)
+
+	if [[ $ERRNO_NAME = "-ERR_ANY" ]]; then
+		if [[ $RC -ge 0 ]]; then
+			echo "$1: FAIL, test expects $ERRNO_NAME - got $RC_NAME ($RC)" >&2
+			config_show_config
+			exit 1
+		fi
+	elif [[ $RC != $ERRNO ]]; then
+		echo "$1: FAIL, test expects $ERRNO_NAME ($ERRNO) - got $RC_NAME ($RC)" >&2
+		config_show_config
+		exit 1
+	fi
+	echo "$1: OK! - Return value: $RC ($RC_NAME), expected $ERRNO_NAME"
+}
+
+sysdata_set_sync_defaults()
+{
+	config_reset
+}
+
+sysdata_set_async_defaults()
+{
+	config_reset
+	config_set_async
+}
+
+sysdata_test_0001s()
+{
+	NAME='\000'
+
+	sysdata_set_sync_defaults
+	config_set_name $NAME
+	printf '\000' >"$DIR"/config_name
+	config_trigger_want_fail ${FUNCNAME[0]}
+	config_expect_result ${FUNCNAME[0]} -EINVAL
+}
+
+sysdata_test_0001a()
+{
+	NAME='\000'
+
+	sysdata_set_async_defaults
+	printf '\000' >"$DIR"/config_name
+	config_trigger_want_fail ${FUNCNAME[0]}
+	config_expect_result ${FUNCNAME[0]} -EINVAL
+}
+
+sysdata_test_0001()
+{
+	sysdata_test_0001s
+	sysdata_test_0001a
+}
+
+sysdata_test_0002s()
+{
+	NAME="nope-$DEFAULT_SYSDATA"
+
+	sysdata_set_sync_defaults
+	config_set_name ${FUNCNAME[0]}
+	config_trigger_want_fail ${FUNCNAME[0]}
+	config_expect_result ${FUNCNAME[0]} -ENOENT
+}
+
+sysdata_test_0002a()
+{
+	NAME="nope-$DEFAULT_SYSDATA"
+
+	sysdata_set_async_defaults
+	config_set_name $NAME
+	config_trigger_want_fail ${FUNCNAME[0]}
+	# This may seem odd to expect success on a bogus
+	# file but remember this is an async call, the actual
+	# error handling is managed by the async callbacks.
+	config_expect_result ${FUNCNAME[0]} SUCCESS
+}
+
+sysdata_test_0002()
+{
+	#sysdata_test_0002s
+	sysdata_test_0002a
+}
+
+sysdata_test_0003()
+{
+	config_reset
+	config_file_should_not_match ${FUNCNAME[0]}
+}
+
+sysdata_test_0004s()
+{
+	TEST="sysdata_test_0004s"
+
+	sysdata_set_sync_defaults
+	config_trigger ${FUNCNAME[0]}
+	config_file_should_match ${FUNCNAME[0]}
+	config_expect_result ${FUNCNAME[0]} SUCCESS
+}
+
+sysdata_test_0004a()
+{
+	sysdata_set_async_defaults
+	config_trigger ${FUNCNAME[0]}
+	config_file_should_match ${FUNCNAME[0]}
+	config_expect_result ${FUNCNAME[0]} SUCCESS
+}
+
+sysdata_test_0004()
+{
+	sysdata_test_0004s
+	sysdata_test_0004a
+}
+
+sysdata_test_0005s()
+{
+	NAME="nope-$DEFAULT_SYSDATA"
+
+	sysdata_set_sync_defaults
+	config_set_optional
+	config_set_name $NAME
+	config_trigger_want_fail ${FUNCNAME[0]}
+	# We do this to ensure the default backup callback hasn't
+	# been called yet
+	config_file_should_not_match ${FUNCNAME[0]}
+	config_expect_result ${FUNCNAME[0]} SUCCESS
+}
+
+sysdata_test_0005a()
+{
+	NAME="nope-$DEFAULT_SYSDATA"
+
+	sysdata_set_async_defaults
+	config_set_optional
+	config_set_name $NAME
+	config_trigger_want_fail ${FUNCNAME[0]}
+	# We do this to ensure the default backup callback hasn't
+	# been called yet
+	config_file_should_not_match ${FUNCNAME[0]}
+	config_expect_result ${FUNCNAME[0]} SUCCESS
+}
+
+sysdata_test_0005()
+{
+	sysdata_test_0005s
+	sysdata_test_0005a
+}
+
+sysdata_test_0006s()
+{
+	sysdata_set_sync_defaults
+	config_set_optional
+	config_trigger ${FUNCNAME[0]}
+	config_file_should_match ${FUNCNAME[0]}
+	config_expect_result ${FUNCNAME[0]} SUCCESS
+}
+
+sysdata_test_0006a()
+{
+	sysdata_set_async_defaults
+	config_set_optional
+	config_trigger ${FUNCNAME[0]}
+	config_file_should_match ${FUNCNAME[0]}
+	config_expect_result ${FUNCNAME[0]} SUCCESS
+}
+
+sysdata_test_0006()
+{
+	sysdata_test_0006s
+	sysdata_test_0006a
+}
+
+sysdata_test_0007s()
+{
+	sysdata_set_sync_defaults
+	config_set_keep
+	config_trigger ${FUNCNAME[0]}
+	config_file_should_match ${FUNCNAME[0]}
+	config_expect_result ${FUNCNAME[0]} SUCCESS
+}
+
+sysdata_test_0007a()
+{
+	sysdata_set_async_defaults
+	config_set_keep
+	config_trigger ${FUNCNAME[0]}
+	config_file_should_match ${FUNCNAME[0]}
+	config_expect_result ${FUNCNAME[0]} SUCCESS
+}
+
+sysdata_test_0007()
+{
+	sysdata_test_0007s
+	sysdata_test_0007a
+}
+
+sysdata_test_0008s()
+{
+	NAME="nope-$DEFAULT_SYSDATA"
+
+	sysdata_set_sync_defaults
+	config_set_name $NAME
+	config_set_optional
+	config_enable_opt_cb
+	config_trigger ${FUNCNAME[0]}
+	config_file_should_match_default ${FUNCNAME[0]}
+	config_expect_result ${FUNCNAME[0]} SUCCESS
+}
+
+sysdata_test_0008a()
+{
+	NAME="nope-$DEFAULT_SYSDATA"
+
+	sysdata_set_async_defaults
+	config_set_name $NAME
+	config_set_optional
+	config_enable_opt_cb
+	config_trigger ${FUNCNAME[0]}
+	config_file_should_match_default ${FUNCNAME[0]}
+	config_expect_result ${FUNCNAME[0]} SUCCESS
+}
+
+sysdata_test_0008()
+{
+	sysdata_test_0008s
+	sysdata_test_0008a
+}
+
+sysdata_test_0009s()
+{
+	NAME="nope-$DEFAULT_SYSDATA"
+
+	sysdata_set_sync_defaults
+	config_set_name $NAME
+	config_set_keep
+	config_set_optional
+	config_enable_opt_cb
+	config_trigger ${FUNCNAME[0]}
+	config_file_should_match_default ${FUNCNAME[0]}
+	config_expect_result ${FUNCNAME[0]} SUCCESS
+}
+
+sysdata_test_0009a()
+{
+	NAME="nope-$DEFAULT_SYSDATA"
+
+	sysdata_set_async_defaults
+	config_set_name $NAME
+	config_set_keep
+	config_set_optional
+	config_enable_opt_cb
+	config_trigger ${FUNCNAME[0]}
+	config_file_should_match_default ${FUNCNAME[0]}
+	config_expect_result ${FUNCNAME[0]} SUCCESS
+}
+
+sysdata_test_0009()
+{
+	sysdata_test_0009s
+	sysdata_test_0009a
+}
+
+sysdata_test_0010s()
+{
+	NAME="nope-$DEFAULT_SYSDATA"
+
+	sysdata_set_sync_defaults
+	config_set_name $NAME
+	config_set_default_name $NAME
+	config_set_keep
+	config_set_optional
+	config_enable_opt_cb
+	config_trigger_want_fail ${FUNCNAME[0]}
+	config_file_should_not_match ${FUNCNAME[0]}
+	config_expect_result ${FUNCNAME[0]} -ENOENT
+}
+
+sysdata_test_0010a()
+{
+	NAME="nope-$DEFAULT_SYSDATA"
+
+	sysdata_set_async_defaults
+	config_set_name $NAME
+	config_set_default_name $NAME
+	config_set_keep
+	config_set_optional
+	config_enable_opt_cb
+	config_trigger_want_fail ${FUNCNAME[0]}
+	config_file_should_not_match ${FUNCNAME[0]}
+	config_expect_result ${FUNCNAME[0]} SUCCESS
+}
+
+sysdata_test_0010()
+{
+	sysdata_test_0010s
+	sysdata_test_0010a
+}
+
+test_reqs
+
+usage()
+{
+	echo "Usage: $0 [ -t <4-number-digit> ]"
+	echo Valid tests: 0001-0005
+	echo
+	echo 0001 - Empty string should be ignored
+	echo 0002 - Files that do not exist should be ignored
+	echo 0003 - Verify test_sysdata0 has nothing loaded upon reset
+	echo 0004 - Simple sync and async loader
+	echo 0005 - Verify optional loading is not fatal
+	echo 0006 - Verify optional loading enables loading
+	echo 0007 - Verify keep works
+	echo 0008 - Verify optional callback works
+	echo 0009 - Verify optional callback works, keep
+	echo 0010 - Verify when fallback file is not present
+	exit 1
+}
+
+# You can ask for a specific test:
+if [[ $# > 0 ]] ; then
+	if [[ $1 != "-t" ]]; then
+		usage
+	fi
+
+	re='^[0-9]+$'
+	if ! [[ $2 =~ $re ]]; then
+		usage
+	fi
+
+	RUN_TEST=sysdata_test_$2
+	$RUN_TEST
+	exit 0
+fi
+
+sysdata_test_0001
+sysdata_test_0002
+sysdata_test_0003
+sysdata_test_0004
+sysdata_test_0005
+sysdata_test_0006
+sysdata_test_0007
+sysdata_test_0008
+sysdata_test_0009
+sysdata_test_0010
+
+exit 0
-- 
2.8.2

[PATCH v2 6/8] Documentation/firmware_class: add sysdata API converter SmPL patch

[Luis R. Rodriguez]

This adds an SmPL patch to let you help you convert
drivers over from the old firmware API to the new sysdata
API. Given the amount of changes for the sync case, and the
amount of manual revision needed these patches are kept out
of scripts/coccinelle/ to annotate required developer intervention
on the convertion.

3 Coccinelle patches are provided then in Documentation/firmware_class/:

0001-convert-sysdata-sync.cocci - sync work
0002-convert-sysdata-async.cocci - for async calls
0003-convert-sysdata-generic.cocci - generic work

To use Coccinelle to help convert your driver over using these helpers in
one shot you can use the provided script as follows:

./Documentation/firmware_class/convert-sysdata.sh path-to-driver/

Contrary to the scripts/coccinelle tradition to send changes to stdout,
this uses the spatch --in-place option to make the required changes
in your git tree, to see the resulting effects you can use 'git diff'.
You should revise the code yourself, and ensure that it is correct,
compile it and run time test it before submitting a patch to transform
it using this series of SmPL patches. Be aware that this currently only
address local variable uses of the firmware, so if you stuff your
struct firmware on a data structure this is not safe to use yet.

A few release notes about these Coccinelle SmPL transformation rules used:

a) If you see if (__true__) in your changes, the sync rule add_cb_sync
   was unable to complete the work, you should review this and do the
   change yourself

b) The add_desc_sync rule deals with two cases, one where the
   const struct sysdata_file_desc sysdata_desc sysdata_desc is
   properly initialized, and the other where its initialization
   occurs later in code. Since the uninitialized case can be identified
   through a compile error, to help simplify this rule and help with
   the transition the rule is kept, developers however should review
   the changes and ensure the descriptor is properly initialized.

c) GFP_KERNEL is assumed, through an easy change the script can enable use of
   ATOMIC cases as well, however these haven't been reviewed yet.

d) FW_ACTION_NOHOTPLUG is not handled -- these require the usermode helper
   and the sysdata API purposely ignores this.

e) Coccinelle 1.0.5 will produce "return0 instead of return 0" in a few
   cases, this will be fixed in a future Coccinelle release.

f) Coccinelle 1.0.5 is required

g) The output will complain about:
    warning: line 501: should sysdata be a metavariable?
   This can safely be ignored for now.

If you use this SmPL patch to help transform your driver please use the tag:

   Generated-by: Coccinelle SmPL

Signed-off-by: Luis R. Rodriguez <mcgrof@kernel.org>
---
 .../firmware_class/0001-convert-sysdata-sync.cocci | 1154 ++++++++++++++++++++
 .../0002-convert-sysdata-async.cocci               |  259 +++++
 .../0003-convert-sysdata-generic.cocci             |   43 +
 Documentation/firmware_class/convert-sysdata.sh    |   13 +
 Documentation/firmware_class/system_data.txt       |   49 +
 5 files changed, 1518 insertions(+)
 create mode 100644 Documentation/firmware_class/0001-convert-sysdata-sync.cocci
 create mode 100644 Documentation/firmware_class/0002-convert-sysdata-async.cocci
 create mode 100644 Documentation/firmware_class/0003-convert-sysdata-generic.cocci
 create mode 100755 Documentation/firmware_class/convert-sysdata.sh

diff --git a/Documentation/firmware_class/0001-convert-sysdata-sync.cocci b/Documentation/firmware_class/0001-convert-sysdata-sync.cocci
new file mode 100644
index 000000000000..9fe607436af6
--- /dev/null
+++ b/Documentation/firmware_class/0001-convert-sysdata-sync.cocci
@@ -0,0 +1,1154 @@
+// Copyright: (C) 2016 Julia Lawall, Inria/LIP6.  GPLv2
+//
+// Options: --allow-inconsistent-paths --in-place  --force-diff -D index="" -D sysdata=sysdata
+// Requires: 1.0.5
+//
+// Identify the functions that we can treat.
+// Request_firmware must be called in the main execution path, not under an if
+// Unless all if branches call request_firmware
+// This is enforced by the ...s and their lack of when any annotation
+// This requires the first argument of request_firmware to have the form &e
+// There are a couple of calls in the kernel where this property does not hold
+
+virtual patch
+
+// identifies as function as a combination of its name and parameter list,
+// in hopes that this is unique
+
+@initialize:ocaml@
+@@
+
+let redo index =
+  let it = new iteration() in
+  let file = List.hd (Coccilib.files()) in
+  it#set_files [file];
+  let index = if index = "" then 1 else (1+(int_of_string index)) in
+  it#add_virtual_identifier Index (string_of_int index);
+  it#add_virtual_identifier Sysdata (Printf.sprintf "sysdata%d" index);
+  it#register()
+
+@exists@
+expression x;
+@@
+
+request_firmware(...)
+<...
+- return
++ SRETURN_(
+x
++)
+ ;
+...>
+
+@inverted@
+position p;
+@@
+
+request_firmware@p(...) == 0
+
+@start@
+identifier f, ret, ret1;
+local idexpression fw;
+expression name, dev;
+fresh identifier sync_found_cb = f ## "_found_cb";
+fresh identifier reqtype = f ## "_req";
+fresh identifier sreq = "sreq" ## virtual.index;
+fresh identifier context = "context" ## virtual.index;
+statement S;
+position p1 != inverted.p;
+parameter list ps;
+@@
+
+int f(ps) {
++struct reqtype { void *nothing; };
++struct reqtype sreq;
++const struct sysdata_file_desc sysdata_desc = {
++	SYSDATA_DEFAULT_SYNC(sync_found_cb, &sreq),
++};
+... when != request_firmware(...)
+(
+- ret = request_firmware@p1(&fw, name, dev);
++ ret = sysdata_file_request(name, &sysdata_desc, dev);
+if (<+...ret...+>) S
++ if(__true__) {
++ struct reqtype *sreq = context;
+  ... when any
++ return ret1;
++}
+SRETURN_(
+- ret1
++ ret
+  );
+|
+if (<+...
+-        request_firmware@p1(&fw, name, dev)
++        sysdata_file_request(name, &sysdata_desc, dev)
+    ...+>) S
++ if(__true__) {
++ struct reqtype *sreq = context;
+  ... when any
+      when != request_firmware(...)
++ return ret1;
++}
+SRETURN_(
+- ret1
++ 0
+  );
+)
+}
+
+@redoable depends on start@
+@@
+request_firmware(...)
+
+@script:ocaml depends on redoable@
+index << virtual.index;
+@@
+redo index
+
+// Rename firmware structure and type
+@exists@
+identifier start.f,virtual.sysdata;
+local idexpression start.fw;
+symbol __true__;
+parameter list start.ps;
+@@
+
+f (ps) {
+<...
+ if(__true__) {
+<...
+- fw
++ sysdata
+...>
+}
+...>
+}
+
+@@
+identifier start.f, i;
+parameter list start.ps;
+@@
+
+f (ps) {
+<...
+  struct
+- firmware
++ sysdata_file
+  i;
+...>
+}
+
+@depends on start@
+identifier l,l1;
+@@
+
+if (...) {
+   ...
+   goto l;
+}
+... when != if (...) { ... goto l1; }
+if(__true__) {
+  ...
++ if (__false__) {
+l:
+...
++}
+}
+
+// Propagate renames in hopes of reducing the number of variables that
+// have to be stored in the context structure
+// Needs to come first because the propagated thing is not a decl, so decls
+// have to be added before it.
+@depends on start@
+identifier b, start.reqtype, start.sreq, context;
+expression x,e1,e2,a;
+@@
+
+x = a->b;
+...  when != x = e1
+     when != a = e2
+if(__true__) {
+    struct reqtype *sreq = context;
+++  x = a->b;
+    ... when exists
+    x
+    ... when any
+}
+
+// Push downwards unused initializers as well as calls on parameter (risky)
+// requires if on ehc
+
+@@
+identifier start.f, x, start.reqtype, start.sreq, context;
+constant C;
+type T;
+symbol __false__;
+parameter list start.ps;
+@@
+
+f (ps) {
+ ... when any
+     when strict
+-T x = C;
+ ... when != x
+     when any
+ if(__true__) {
+    struct reqtype *sreq = context;
+++  T x = C;
+    <+...x...+>
+    if (__false__) { ... when != x
+    }
+ }
+... when != x
+    when any
+}
+
+@@ // structs are like constants
+identifier start.f, x, i, start.reqtype, start.sreq, context;
+symbol __false__;
+parameter list start.ps;
+@@
+
+f (ps) {
+ ... when any
+     when strict
+-struct i x;
+ ... when != x
+     when any
+ if(__true__) {
+    struct reqtype *sreq = context;
+++  struct i x;
+    <+...x...+>
+    if (__false__) { ... when != x
+    }
+ }
+... when != x
+    when any
+}
+
+@@
+identifier start.f, h, x, i, j, start.reqtype, start.sreq, context;
+expression e,e1;
+type T,T1;
+@@
+
+f (...,T1 i,...) {
+ ... when any
+     when strict
+-T x = \(i->@e j\|h(i)@e\);
+ ... when != x
+     when != i = e1
+     when any
+ if(__true__) {
+    struct reqtype *sreq = context;
+++  T x = e;
+    <+...x...+>
+    if (__false__) { ... when != x
+    }
+ }
+... when != x
+    when any
+}
+
+@nofalse@
+identifier start.f;
+parameter list start.ps;
+statement S;
+@@
+
+f(ps) {
+  ... when != if (__false__) S
+}
+
+@depends on nofalse@
+identifier start.f, x, start.reqtype, start.sreq, context;
+constant C;
+type T;
+parameter list start.ps;
+@@
+
+f (ps) {
+ ... when any
+     when strict
+-T x = C;
+ ... when != x
+     when any
+ if(__true__) {
+    struct reqtype *sreq = context;
+++  T x = C;
+    ... when exists
+    x
+    ... when any
+ }
+... when != x
+    when any
+}
+
+@depends on nofalse@ // structs are like constants
+identifier start.f, x, i != start.reqtype, start.reqtype, start.sreq, context;
+parameter list start.ps;
+@@
+
+f (ps) {
+ ... when any
+     when strict
+-struct i x;
+ ... when != x
+     when any
+ if(__true__) {
+    struct reqtype *sreq = context;
+++  struct i x;
+    ... when exists
+    x
+    ... when any
+ }
+... when != x
+    when any
+}
+
+@depends on nofalse@
+identifier start.f, h, x, i, j, start.reqtype, start.sreq, context;
+expression e,e1;
+type T,T1;
+@@
+
+f (...,T1 i,...) {
+ ... when any
+     when strict
+-T x = \(i->@e j\|h(i)@e\);
+ ... when != x
+     when != i = e1
+     when any
+ if(__true__) {
+    struct reqtype *sreq = context;
+++  T x = e;
+    ... when exists
+    x
+    ... when any
+ }
+... when != x
+    when any
+}
+
+// Initialize return vaue if needed
+
+@updret depends on start exists@
+identifier ret,r;
+type T;
+expression e1,e2;
+statement S;
+position p;
+@@
+
+T ret;
+...
+ret = sysdata_file_request(...);
+if (...) S
+if@p(__true__) {
+  ... when != ret = e1
+      when != T ret;
+(
+  ret = e2
+|
+  ret@r
+)
+  ... when any
+}
+
+@depends on start exists@
+identifier updret.r, start.reqtype, start.sreq, context;
+type updret.T;
+expression e1;
+position updret.p;
+@@
+
+if@p(__true__) {
+  struct reqtype *sreq = context;
+++ T r = 0;
+  ... when != r = e1
+      when != T r;
+  r
+  ... when any
+}
+
+
+// Duplicates decl
+@@
+identifier start.f, x, i, start.reqtype, start.sreq, context;
+constant C;
+type T,T1;
+expression e;
+@@
+
+f (...,T1 i,...) {
+ ... when any
+T x = C;
+ ... when != x = e
+     when any
+ if(__true__) {
+    struct reqtype *sreq = context;
+++  T x = C;
+    ... when exists
+    x
+    ... when any
+ }
+... when any
+}
+
+// Remove if on ehc
+
+@ehc depends on start exists@
+statement list sl;
+@@
+
+if(__true__) {
+  ... when any
+- if (__false__) {
+ sl
+-}
+ ... when any
+}
+
+@@
+expression x;
+@@
+
+- SRETURN_(
++ return
+  x
+- )
+  ;
+
+// Try to construct the context: working on the written variables
+
+@write1 depends on start exists@
+position pa1,px1;
+type T;
+identifier i;
+expression e;
+@@
+
+if (__true__) { ... when any
+  T i@pa1@px1 = e;
+  ... when any
+}
+
+@write2 depends on start exists@
+type T;
+T v;
+identifier i;
+expression e;
+position pa2,px2;
+@@
+
+if (__true__) { ... when any
+  v@i@pa2@px2 = e
+  ... when any
+}
+
+@write3 depends on start exists@
+type T;
+T v;
+identifier i,g;
+position pa3,px3;
+@@
+
+if (__true__) { ... when any
+  g(...,&v@i@pa3@px3,...)
+  ... when any
+}
+
+@writen@
+identifier i;
+position any write1.pa1;
+position any write2.pa2;
+position any write3.pa3;
+@@
+
+(
+i@pa1
+|
+i@pa2
+|
+i@pa3
+)
+
+// var is read on some path before it was written
+@read depends on start exists@
+local idexpression v;
+identifier virtual.sysdata;
+identifier writen.i;
+expression e,e1;
+position p != {write1.px1,write2.px2,write3.px3};
+position any write1.pa1;
+position any write2.pa2;
+position any write3.pa3;
+statement S;
+type T;
+@@
+
+if (__true__) {
+  ... when any
+      when != T i@pa1 = e1;
+      when != i@pa2
+      when != i@pa3
+(
+  for (<+...i = e...+>; ...; ...) S
+|
+  sizeof(<+...i...+>) // not a reference
+|
+  sysdata
+|
+  v@i@p
+)
+  ... when any
+}
+
+// other occurrences of vars that were somewhere first read
+@firstread depends on start exists@
+local idexpression read.v;
+identifier writen.i;
+position p;
+@@
+
+if (__true__) {
+  <...
+  v@i@p
+  ...>
+}
+
+@r depends on start exists@
+type T;
+local idexpression T v;
+identifier writen.i,reqtype,start.f,start.sreq, context;
+position p != firstread.p;
+parameter list start.ps;
+@@
+
+f(ps) {
+...
+if (__true__) {
+   struct reqtype *sreq = context;
+  ... when != T i;
+  v@i@p
+  ... when any
+}
+... when any
+}
+
+@depends on start@
+type r.T;
+identifier writen.i,start.f,start.sreq, context;
+identifier reqtype;
+parameter list start.ps;
+@@
+
+f(ps) {
+... when any
+if (__true__) {
+   struct reqtype *sreq = context;
+++ T i;
+  ...
+}
+... when any
+}
+
+// Try to construct the context: working on the read variables
+
+@assignfor depends on start exists@
+identifier i;
+expression e;
+position p,p1,p2;
+statement S;
+@@
+
+if (__true__) { ... when any
+  for(<+...i@p = e...+>; <+...i@p1...+>; <+...i@p2...+>) S
+  ... when any
+}
+
+@assignaddr depends on start exists@
+identifier i,g;
+position p;
+@@
+
+if (__true__) { ... when any
+  g(...,&i@p,...)
+  ... when any
+}
+
+// The following overlaps with assignaddr, so has to be a separate rule
+@assign depends on start exists@
+identifier i;
+expression e;
+position p;
+type T;
+@@
+
+if (__true__) { ... when any
+(
+  T i@p = e;
+|
+  T i@p;
+|
+  i@p = e
+)
+  ... when any
+}
+
+@doesread depends on start exists@
+position p != {assignfor.p,assignfor.p1,assignfor.p2,assignaddr.p,assign.p};
+identifier i,reqtype,start.f,start.sreq,context;
+type T,T1;
+local idexpression T v;
+parameter list start.ps;
+@@
+
+f(ps) {
+...
+if (__true__) {
+  struct reqtype *sreq = context;
+  ... when any
+      when != i // matches exp
+      when != T1 i;
+(
+  sizeof(<+...v...+>)
+|
+  v@i@p
+)
+  ... when any
+}
+... when any
+}
+
+@cc depends on start exists@
+type doesread.T,T1;
+identifier doesread.i,start.f,start.sreq,context;
+identifier reqtype, ret;
+statement S;
+parameter list start.ps;
+@@
+
+f(ps) {
+struct reqtype {
+- void *nothing;
+  ...
+++ T i;
+};
+... when any
+(
+++sreq.i = i;
+ret = sysdata_file_request(...);
+if (<+...ret...+>) S
+|
+++sreq.i = i;
+if (<+...sysdata_file_request(...)...+>) S
+)
+if (__true__) {
+  struct reqtype *sreq = context;
+++ T i = sreq->i;
+  ... when any
+      when != i // matches exp
+      when != T1 i;
+  i
+  ... when any
+}
+  ...  when any
+}
+
+// Convert arrays to pointers
+@@
+identifier cc.reqtype;
+type T;
+identifier i;
+@@
+
+struct reqtype {
+  ...
+  T
++ *
+  i
+- [...]
+  ;
+  ...
+};
+
+// Hack to deal with array types and with a weakness in the pretty printer,
+// part 2.
+@depends on start exists@
+type T;
+identifier i,start.sreq;
+@@
+
+if (__true__) { ... when any
+  T
++ *
+  i
+- [...]
+  = sreq->i;
+  ... when any
+}
+
+@sz depends on start exists@ // vars only used in sizeof
+type T1,T;
+identifier reqtype, i, start.sreq, context;
+local idexpression T v;
+@@
+
+if (__true__) {
+  struct reqtype *sreq = context;
+++ T i;
+  ... when != T1 i;
+  sizeof(v@i)
+  ... when any
+}
+
+// Drop the context structure if it contains only one element
+
+@dropstructparam exists@
+identifier start.f,start.reqtype,x,start.sreq,context;
+type T;
+expression e;
+symbol sysdata_desc;
+@@
+
+f(...,T x,...) {
+-struct reqtype { T x; };
+-struct reqtype sreq;
+const struct sysdata_file_desc sysdata_desc = {
+	SYSDATA_DEFAULT_SYNC(e,
+-       &sreq
++       (void *)x
+        ),
+};
+...
+-sreq.x = x;
+...
+if (__true__) {
+-  struct reqtype *sreq = context;
+-  T x = sreq->x;
++  T x = context;
+   ...
+}
+... when any
+}
+
+@dropstructlocal exists@
+identifier start.f,start.reqtype,x,start.sreq,context;
+statement S;
+expression e;
+type T;
+parameter list start.ps;
+@@
+
+f(ps) {
+-struct reqtype { T x; };
+-struct reqtype sreq;
+-const struct sysdata_file_desc sysdata_desc = {
+-	SYSDATA_DEFAULT_SYNC(e, &sreq),
+-};
+... when != S
+T x = ...;
++const struct sysdata_file_desc sysdata_desc = {
++	SYSDATA_DEFAULT_SYNC(e, (void *)x),
++};
+...
+-sreq.x = x;
+...
+if (__true__) {
+-  struct reqtype *sreq = context;
+-  T x = sreq->x;
++  T x = context;
+   ...
+}
+... when any
+}
+
+@dropstruct exists@
+identifier start.f,start.reqtype,start.sreq,context;
+expression e;
+parameter list start.ps;
+@@
+
+f(ps) {
+-struct reqtype { void *nothing; };
+-struct reqtype sreq;
+const struct sysdata_file_desc sysdata_desc = {
+	SYSDATA_DEFAULT_SYNC(e,
+- &sreq
++ NULL
+  ),
+};
+...
+if (__true__) {
+-  struct reqtype *sreq = context;
+   ...
+}
+... when any
+}
+
+// Move the context structure declaration out to top level
+
+@depends on !dropstructparam && !dropstructlocal && !dropstruct@
+identifier start.f;
+type T;
+parameter list start.ps;
+@@
+
++ T;
+
+f(ps) {
+- T;
+  ...
+}
+
+// Construct the callback function
+
+// The conjunction ( & ) is for efficiency: the first case is easy to
+// remove, while the second allows transporting the code without the braces.
+@ add_cb_sync1 depends on ehc@
+identifier start.f,virtual.sysdata;
+fresh identifier sync_found_cb = f ## "_found_cb";
+fresh identifier context = "context" ## virtual.index;
+statement S;
+statement list S1, ehc.sl;
+expression ret;
+parameter list start.ps;
+@@
+
++static int sync_found_cb(void *context, const struct sysdata_file *sysdata)
++{
++ S1
++}
+
+f (ps) {
+... when any
+(
+- if (__true__) S
+&
+if (__true__) { S1 }
+)
+-return ret;
++sl
+}
+
+@@
+expression x;
+@@
+
+- SRETURN_(x);
++ return x;
+
+@ add_cb_sync2 depends on !ehc @
+identifier start.f,virtual.sysdata;
+fresh identifier sync_found_cb = f ## "_found_cb";
+fresh identifier context = "context" ## virtual.index;
+statement S;
+statement list S1;
+expression ret;
+parameter list start.ps;
+@@
+
++static int sync_found_cb(void *context, const struct sysdata_file *sysdata)
++{
++ S1
++}
+
+f (ps) {
+... when any
+(
+- if (__true__) S
+&
+if (__true__) { S1 }
+)
+return ret;
+}
+
+// Check for a single retry
+// Not safe.  We just hope that the fw that is consistent with f, name, and dev
+// is the right one...
+
+@@
+identifier start.f,l,ret;
+expression name, dev, ret1, name1, dev1;
+local idexpression start.fw;
+statement S;
+parameter list start.ps;
+@@
+
+f(ps) { <...
+(
+ret = sysdata_file_request(name, &sysdata_desc, dev);
+if (...) {
+  ... when != goto l;
+(
+- ret1 = request_firmware(&fw, name1, dev1);
++ ret1 = sysdata_file_request(name1, &sysdata_desc, dev);
+if (<+...ret1...+>) S
+|
+if (<+...
+-        request_firmware(&fw, name1, dev1)
++        sysdata_file_request(name1, &sysdata_desc, dev)
+    ...+>) S
+)
+}
+|
+if (<+...sysdata_file_request(name, &sysdata_desc, dev)...+>) {
+  ... when != goto l;
+(
+- ret1 = request_firmware(&fw, name1, dev1);
++ ret1 = sysdata_file_request(name1, &sysdata_desc, dev);
+if (<+...ret1...+>) S
+|
+if (<+...
+-        request_firmware(&fw, name1, dev1)
++        sysdata_file_request(name1, &sysdata_desc, dev)
+    ...+>) S
+)
+}
+)
+...> }
+
+// drop trivial labels
+
+@@
+identifier start.f,out;
+local idexpression x;
+parameter list start.ps;
+@@
+
+f(ps) {
+... when != goto out;
+- x =
++ return
+   sysdata_file_request(...);
+-if (<+...x...+>) goto out;
+-out:
+-return x;
+}
+
+@@
+identifier start.f,out;
+local idexpression x;
+parameter list start.ps;
+statement S, S1;
+@@
+
+f(ps) {
+...
+ x =
+   sysdata_file_request(...);
+(
+ if (<+...x...+>)
+ {
+  if (...) S else S1
+- goto out;
+ }
+|
+ if (<+...x...+>)
+- {
+  S
+- goto out;
+- }
+)
+out:
+...
+}
+
+// drop unused labels
+
+@l1 exists@
+identifier f = {start.sync_found_cb,start.f};
+identifier l;
+position p,p1;
+@@
+
+f@p1(...) {
+  <... when any
+  l@p:
+  ...>
+}
+
+@l2@
+identifier l1.f, l1.l;
+position l1.p1;
+@@
+
+f@p1(...) {
+  ... when != goto l;
+      when strict
+}
+
+@l3 depends on l2@
+identifier l1.f, l1.l;
+position l1.p,l1.p1;
+@@
+
+f@p1(...) {
+  <...
+- l@p:
+  ...>
+}
+
+@@
+identifier start.f;
+local idexpression x;
+parameter list start.ps;
+@@
+
+f(ps) {
+...
+- x =
++ return
+   sysdata_file_request(...);
+-if (<+...x...+>) return x;
+-return x;
+...
+}
+
+@@
+identifier start.f;
+local idexpression x;
+statement S,S1;
+parameter list start.ps;
+@@
+
+f(ps) {
+...
+ x =
+   sysdata_file_request(...);
+(
+if (...) { if (...) S else S1
+- return x;
+ }
+|
+if (<+...x...+>)
+- {
+  S
+- return x;}
+)
+return x;
+}
+
+// drop any now (or previously...) unused variables
+@decld exists@
+identifier start.f;
+type T;
+identifier x;
+constant C;
+parameter list start.ps;
+position p;
+@@
+
+f(ps) {
+  ... when any
+      when strict
+(
+ T x@p = C;
+|
+ T x@p;
+)
+  ... when any
+}
+
+@unused@
+identifier start.f;
+identifier decld.x;
+parameter list start.ps;
+@@
+
+f(ps) {
+  ... when != x
+}
+
+@depends on unused@
+position decld.p;
+type T;
+identifier x;
+constant C;
+@@
+
+(
+- T x@p = C;
+|
+- T x@p;
+)
+
+// common cleanup: we can't include files at the end of a cocci file yet
+// so we make this highly dependent on these set of rules.
+// Despite the care some of these changes may still be too aggressive
+// given we do not ensure the const struct firmware *data was the one
+// used on the start rule
+
+@ use_new_struct depends on start @
+identifier consumer, data;
+@@
+
+consumer(...,
+-	const struct firmware *data
++	const struct sysdata_file *data
+	,...)
+{
+...
+}
+
+@ modify_decl depends on start @
+type T;
+identifier consumer, data;
+@@
+
+T consumer(...,
+-	const struct firmware *data
++	const struct sysdata_file *data
+	,...);
+
+@ replace_struct_on_types depends on start @
+type T;
+identifier data;
+@@
+
+T {
+	...
+-	const struct firmware *data;
++	const struct sysdata_file *data;
+	...
+};
+
+@ drop_fw_release_goto depends on start @
+identifier out, some_fn;
+@@
+
+void some_fn (...) {
+<+...
+- goto out;
++ return;
+...+>
+-out:
+-release_firmware(...);
+}
+
+@ drop_fw_release_fn depends on start @
+identifier fn;
+@@
+
+-fn (...) {
+-release_firmware(...);
+-}
+
+@ drop_fw_release_fn_uses depends on start @
+identifier drop_fw_release_fn.fn;
+@@
+
+-fn(...);
+
+@ drop_fw_release_branch depends on start @
+@@
+
+-if (...)
+-release_firmware(...);
+
+@ drop_fw_release depends on start @
+@@
+
+-release_firmware(...);
diff --git a/Documentation/firmware_class/0002-convert-sysdata-async.cocci b/Documentation/firmware_class/0002-convert-sysdata-async.cocci
new file mode 100644
index 000000000000..c781bdc5da04
--- /dev/null
+++ b/Documentation/firmware_class/0002-convert-sysdata-async.cocci
@@ -0,0 +1,259 @@
+// You can use this to help convert a device driver from the old firmware
+// request_firmware_nowait() API the new flexible sysdata API for async
+// requests.
+//
+// Confidence: Medium
+//
+// Reason for low confidence:
+//
+// Copyright: (C) 2016 Luis R. Rodriguez <mcgrof@kernel.org> GPLv2.
+// Copyright: (C) 2016 Julia Lawall, Inria/LIP6.  GPLv2
+//
+// Options: --include-headers --in-place --no-show-diff
+// Requires: 1.0.5
+
+virtual patch
+
+@ async_get_t @
+expression name, dev;
+bool true;
+identifier drv_callback;
+type T;
+T *drv;
+@@
+
+(
+request_firmware_nowait(THIS_MODULE, true, name, dev, GFP_KERNEL, drv, drv_callback)
+|
+request_firmware_nowait(THIS_MODULE, 1, name, dev, GFP_KERNEL, drv, drv_callback)
+|
+request_firmware_nowait(THIS_MODULE, FW_ACTION_HOTPLUG, name, dev, GFP_KERNEL, drv, drv_callback)
+)
+
+@ find_request_async depends on async_get_t @
+expression name, dev, uevent;
+identifier drv_callback, drv, f;
+@@
+
+f (...)
+{
+<+...
+-request_firmware_nowait(THIS_MODULE, uevent, name, dev, GFP_KERNEL, drv, drv_callback)
++sysdata_file_request_async(name, &sysdata_desc, dev, &drv->sysdata_async_cookie)
+...+>
+}
+
+@ add_desc_async @
+type T;
+identifier find_request_async.f;
+identifier find_request_async.drv;
+identifier find_request_async.drv_callback;
+@@
+
+f (...)
+{
+...
+T *drv;
++const struct sysdata_file_desc sysdata_desc = {
++	SYSDATA_DEFAULT_ASYNC(drv_callback, drv),
++};
+...
+}
+
+@ add_desc_direct_async @
+type T;
+identifier find_request_async.f;
+identifier find_request_async.drv;
+identifier find_request_async.drv_callback;
+@@
+
+f (...,
+   T *drv
+   ,...)
+{
++const struct sysdata_file_desc sysdata_desc = {
++	SYSDATA_DEFAULT_ASYNC(drv_callback, drv),
++};
+...
+}
+
+@ found_callback depends on async_get_t @
+identifier find_request_async.drv_callback;
+identifier find_request_async.drv;
+identifier data, arg;
+type T1;
+@@
+
+ drv_callback(
+-const struct firmware *data,
++const struct sysdata_file *data,
+ void *arg)
+ {
+	...
+	T1 *drv = arg;
+	...
+ }
+
+@ found_completion depends on found_callback @
+identifier find_request_async.drv_callback;
+type found_callback.T1;
+T1 *drv;
+identifier cmpl;
+@@
+
+ drv_callback(...)
+ {
+	<+...
+(
+-	complete(&drv->cmpl);
+|
+-	complete_all(&drv->cmpl);
+)
+	...+>
+ }
+
+@ drop_init_completion @
+identifier find_request_async.drv;
+identifier found_completion.cmpl;
+@@
+
+-init_completion(&drv->cmpl);
+
+@ replace_completion_wait @
+identifier find_request_async.drv;
+identifier found_completion.cmpl;
+@@
+
+-wait_for_completion(&drv->cmpl);
++sysdata_synchronize_request(drv->sysdata_async_cookie);
+
+@ async_cookie exists @
+typedef async_cookie_t;
+type T;
+@@
+
+T {
+	...
+	async_cookie_t sysdata_async_cookie;
+	...
+};
+
+@ modify_drv depends on !async_cookie @
+type async_get_t.T;
+identifier found_completion.cmpl;
+@@
+
+T {
+	...
+-	struct completion cmpl;
++	async_cookie_t sysdata_async_cookie;
+	...
+};
+
+@ modify_drv2 depends on !modify_drv @
+type found_callback.T1;
+identifier found_completion.cmpl;
+@@
+
+T1 {
+	...
+-	struct completion cmpl;
++	async_cookie_t sysdata_async_cookie;
+	...
+};
+
+@ modify_drv3 depends on !(modify_drv || modify_drv2)@
+type add_desc_async.T;
+@@
+
+T {
+	...
++	async_cookie_t sysdata_async_cookie;
+};
+
+@ modify_drv_direct depends on !(modify_drv || modify_drv2 || modify_drv3) @
+type add_desc_direct_async.T;
+@@
+
+T {
+	...
++	async_cookie_t sysdata_async_cookie;
+};
+
+// common cleanup: we can't include files at the end of a cocci file yet
+// so we make this highly dependent on these set of rules.
+// Despite the care some of these changes may still be too aggressive
+// given we do not ensure the const struct firmware *data was the one
+// used on the start rule
+
+@ use_new_struct depends on add_desc_async || add_desc_direct_async @
+identifier consumer, data;
+@@
+
+consumer(...,
+-	const struct firmware *data
++	const struct sysdata_file *data
+	,...)
+{
+...
+}
+
+@ modify_decl depends on add_desc_async || add_desc_direct_async @
+type T;
+identifier consumer, data;
+@@
+
+T consumer(...,
+-	const struct firmware *data
++	const struct sysdata_file *data
+	,...);
+
+@ replace_struct_on_types depends on add_desc_async || add_desc_direct_async @
+type T;
+identifier data;
+@@
+
+T {
+	...
+-	const struct firmware *data;
++	const struct sysdata_file *data;
+	...
+};
+
+@ drop_fw_release_goto depends on add_desc_async || add_desc_direct_async @
+identifier out, some_fn;
+@@
+
+void some_fn (...) {
+<+...
+- goto out;
++ return;
+...+>
+-out:
+-release_firmware(...);
+}
+
+@ drop_fw_release_fn depends on add_desc_async || add_desc_direct_async @
+identifier fn;
+@@
+
+-fn (...) {
+-release_firmware(...);
+-}
+
+@ drop_fw_release_fn_uses depends on add_desc_async || add_desc_direct_async @
+identifier drop_fw_release_fn.fn;
+@@
+
+-fn(...);
+
+@ drop_fw_release_branch depends on add_desc_async || add_desc_direct_async @
+@@
+
+-if (...)
+-release_firmware(...);
+
+@ drop_fw_release depends on add_desc_async || add_desc_direct_async @
+@@
+
+-release_firmware(...);
diff --git a/Documentation/firmware_class/0003-convert-sysdata-generic.cocci b/Documentation/firmware_class/0003-convert-sysdata-generic.cocci
new file mode 100644
index 000000000000..de476f44a5b3
--- /dev/null
+++ b/Documentation/firmware_class/0003-convert-sysdata-generic.cocci
@@ -0,0 +1,43 @@
+// You can use this to help convert a device driver from the old firmware
+// request_firmware*() API the new flexible sysdata API, this covers the
+// generic conversions for both sync and async cases.
+//
+// Confidence: High
+//
+// Copyright: (C) 2016 Luis R. Rodriguez <mcgrof@kernel.org> GPLv2.
+// Copyright: (C) 2016 Julia Lawall, INRIA/LIP6.  GPLv2
+//
+// Options: --include-headers --in-place --no-show-diff
+// Requires: 1.0.5
+
+virtual patch
+
+@ uses_fw_api @
+@@
+
+(
+request_firmware(...)
+|
+request_firmware_nowait(...)
+)
+
+@ uses_sysdata @
+@@
+
+(
+sysdata_file_request(...)
+|
+sysdata_file_request_async(...)
+)
+
+@ replace_header depends on uses_sysdata && !uses_fw_api @
+@@
+
+-#include <linux/firmware.h>
++#include <linux/sysdata.h>
+
+@ add_header depends on uses_sysdata && uses_fw_api @
+@@
+
+#include <linux/firmware.h>
++#include <linux/sysdata.h>
diff --git a/Documentation/firmware_class/convert-sysdata.sh b/Documentation/firmware_class/convert-sysdata.sh
new file mode 100755
index 000000000000..a6a01ba4c6fa
--- /dev/null
+++ b/Documentation/firmware_class/convert-sysdata.sh
@@ -0,0 +1,13 @@
+#!/bin/bash
+
+set -e
+
+if [[ $# -ne 1 ]]; then
+	echo Usage: $0 ./path/to-driver/
+	exit
+fi
+
+for i in Documentation/firmware_class/*.cocci; do
+	export COCCI=$i
+	make coccicheck MODE=patch M=$1
+done
diff --git a/Documentation/firmware_class/system_data.txt b/Documentation/firmware_class/system_data.txt
index 53378ce4fcd0..495a5ef6a24b 100644
--- a/Documentation/firmware_class/system_data.txt
+++ b/Documentation/firmware_class/system_data.txt
@@ -86,4 +86,53 @@ Tracking development enhancements and ideas
 To help track ongoing development for firmware_class and related items to
 firmware_class refer to the kernel newbies wiki page [0].
 
+Converting old firmware API users to sysdata API
+================================================
+
+To help developers convert drivers over to the sysdata API
+a series of Coccinelle SmPL patches is provided to help with
+the transition if and when needed:
+
+Documentation/firmware_class/convert-sysdata-async.cocci - for async calls
+Documentation/firmware_class/convert-sysdata-generic.cocci - for sync calls
+Documentation/firmware_class/convert-sysdata-sync.cocci - generic work
+
+3 files are used to limit each conversion's scope and increase the speed
+for conversion. For instance the sync conversion uses the flags (--no-loops
+--no-gotos). To use Coccinelle to help convert your driver over using
+these helpers in one shot you can use the provided script as follows:
+
+./Documentation/firmware_class/convert-sysdata.sh path-to-driver/
+
+Contrary to the scripts/coccinelle tradition to send changes to stdout,
+this uses the spatch --in-place option to make the required changes
+in your git tree, to see the resulting effects you can use 'git diff'.
+You should revise the code yourself, and ensure that it is correct,
+compile it and run time test it if possible before submitting a patch
+to transform it using this SmPL patch.
+
+A few notes about these Coccinelle changes:
+
+a) If you see if (__true__) in your changes, the sync rule add_cb_sync
+   was unable to complete the work, you should review this and do the
+   change yourself
+
+b) The add_desc_sync rule deals with two cases, one where the
+   const struct sysdata_file_desc sysdata_desc sysdata_desc is properly
+   initialized, and the other where its initialization occurs later in
+   code. Since the uninitialized case can be identified through a compile
+   error, to help simplify this rule and help with the transition the rule
+   is kept, developers however should review the changes and ensure the
+   descriptor is properly initialized.
+
+c) GFP_KERNEL is assumed, an easy change the script can enable use on
+   ATOMIC cases as well, however these haven't been reviewed yet.
+
+d) FW_ACTION_NOHOTPLUG is not handled -- these require the usermode helper
+   and the sysdata API purposely ignores this.
+
+If you use this SmPL patch to transform your driver please use the tag:
+
+Generated-by: Coccinelle SmPL
+
 [0] http://kernelnewbies.org/KernelProjects/firmware-class-enhancements
-- 
2.8.2

[PATCH v2 7/8] x86/microcode: convert to use sysdata API

[Luis R. Rodriguez]

This uses the new flexible firmware API, since we don't
have to keep the firmware around the sysdata API does the
freeing for us safely.

Signed-off-by: Luis R. Rodriguez <mcgrof@kernel.org>
---
 arch/x86/kernel/cpu/microcode/amd.c | 56 +++++++++++++++++++++++++------------
 1 file changed, 38 insertions(+), 18 deletions(-)

diff --git a/arch/x86/kernel/cpu/microcode/amd.c b/arch/x86/kernel/cpu/microcode/amd.c
index 27a0228c9cae..df4351ce4256 100644
--- a/arch/x86/kernel/cpu/microcode/amd.c
+++ b/arch/x86/kernel/cpu/microcode/amd.c
@@ -23,7 +23,7 @@
 #define pr_fmt(fmt) "microcode: " fmt
 
 #include <linux/earlycpio.h>
-#include <linux/firmware.h>
+#include <linux/sysdata.h>
 #include <linux/uaccess.h>
 #include <linux/vmalloc.h>
 #include <linux/initrd.h>
@@ -872,6 +872,31 @@ enum ucode_state load_microcode_amd(int cpu, u8 family, const u8 *data, size_t s
 	return ret;
 }
 
+struct amd_ucode_req {
+	int cpu;
+	struct cpuinfo_x86 *c;
+	enum ucode_state state;
+	const char *name;
+};
+
+static int request_microcode_amd_cb(void *context,
+				    const struct sysdata_file *sysdata)
+{
+	struct amd_ucode_req *req = context;
+
+	if (*(u32 *)sysdata->data != UCODE_MAGIC) {
+		pr_err("invalid magic value (0x%08x)\n",
+		       *(u32 *)sysdata->data);
+		req->state = UCODE_ERROR;
+		return -EINVAL;
+	}
+
+	req->state = load_microcode_amd(req->cpu, req->c->x86,
+					sysdata->data, sysdata->size);
+
+	return 0;
+}
+
 /*
  * AMD microcode firmware naming convention, up to family 15h they are in
  * the legacy file:
@@ -891,10 +916,13 @@ enum ucode_state load_microcode_amd(int cpu, u8 family, const u8 *data, size_t s
 static enum ucode_state request_microcode_amd(int cpu, struct device *device,
 					      bool refresh_fw)
 {
+	struct amd_ucode_req req;
 	char fw_name[36] = "amd-ucode/microcode_amd.bin";
 	struct cpuinfo_x86 *c = &cpu_data(cpu);
-	enum ucode_state ret = UCODE_NFOUND;
-	const struct firmware *fw;
+	const struct sysdata_file_desc sysdata_desc = {
+			SYSDATA_DEFAULT_SYNC(request_microcode_amd_cb, &req),
+			.optional = true,
+	};
 
 	/* reload ucode container only on the boot cpu */
 	if (!refresh_fw || c->cpu_index != boot_cpu_data.cpu_index)
@@ -903,24 +931,16 @@ static enum ucode_state request_microcode_amd(int cpu, struct device *device,
 	if (c->x86 >= 0x15)
 		snprintf(fw_name, sizeof(fw_name), "amd-ucode/microcode_amd_fam%.2xh.bin", c->x86);
 
-	if (request_firmware_direct(&fw, (const char *)fw_name, device)) {
-		pr_debug("failed to load file %s\n", fw_name);
-		goto out;
-	}
+	req.cpu = cpu;
+	req.c = c;
+	req.name = fw_name;
+	req.state = UCODE_NFOUND;
 
-	ret = UCODE_ERROR;
-	if (*(u32 *)fw->data != UCODE_MAGIC) {
-		pr_err("invalid magic value (0x%08x)\n", *(u32 *)fw->data);
-		goto fw_release;
+	if (sysdata_file_request((const char *)fw_name, &sysdata_desc, device)) {
+		pr_debug("failed to load file %s\n", fw_name);
 	}
 
-	ret = load_microcode_amd(cpu, c->x86, fw->data, fw->size);
-
- fw_release:
-	release_firmware(fw);
-
- out:
-	return ret;
+	return req.state;
 }
 
 static enum ucode_state
-- 
2.8.2

[PATCH v2 8/8] p54: convert to sysdata API

[Luis R. Rodriguez]

The Coccinelle sysdata patches were used to help with
this transition. The changes have been carefully manually
vetted for. With the conversion we modify the cases that do
not need the firmware to be kept so that the sysdata API
can release it for us. Using the new sysdata API also means
we can get rid of our own completions.

Generated-by: Coccinelle SmPL
Signed-off-by: Luis R. Rodriguez <mcgrof@kernel.org>
---
 drivers/net/wireless/intersil/p54/eeprom.c |  2 +-
 drivers/net/wireless/intersil/p54/fwio.c   |  5 +-
 drivers/net/wireless/intersil/p54/led.c    |  2 +-
 drivers/net/wireless/intersil/p54/main.c   |  2 +-
 drivers/net/wireless/intersil/p54/p54.h    |  3 +-
 drivers/net/wireless/intersil/p54/p54pci.c | 26 ++++++----
 drivers/net/wireless/intersil/p54/p54pci.h |  4 +-
 drivers/net/wireless/intersil/p54/p54spi.c | 81 +++++++++++++++++++-----------
 drivers/net/wireless/intersil/p54/p54spi.h |  2 +-
 drivers/net/wireless/intersil/p54/p54usb.c | 18 +++----
 drivers/net/wireless/intersil/p54/p54usb.h |  4 +-
 drivers/net/wireless/intersil/p54/txrx.c   |  2 +-
 12 files changed, 90 insertions(+), 61 deletions(-)

diff --git a/drivers/net/wireless/intersil/p54/eeprom.c b/drivers/net/wireless/intersil/p54/eeprom.c
index d4c73d39336f..9d048fa1f07f 100644
--- a/drivers/net/wireless/intersil/p54/eeprom.c
+++ b/drivers/net/wireless/intersil/p54/eeprom.c
@@ -16,7 +16,7 @@
  * published by the Free Software Foundation.
  */
 
-#include <linux/firmware.h>
+#include <linux/sysdata.h>
 #include <linux/etherdevice.h>
 #include <linux/sort.h>
 #include <linux/slab.h>
diff --git a/drivers/net/wireless/intersil/p54/fwio.c b/drivers/net/wireless/intersil/p54/fwio.c
index 257a9eadd595..6bc8463ccf5e 100644
--- a/drivers/net/wireless/intersil/p54/fwio.c
+++ b/drivers/net/wireless/intersil/p54/fwio.c
@@ -17,7 +17,7 @@
  */
 
 #include <linux/slab.h>
-#include <linux/firmware.h>
+#include <linux/sysdata.h>
 #include <linux/etherdevice.h>
 #include <linux/export.h>
 
@@ -27,7 +27,8 @@
 #include "eeprom.h"
 #include "lmac.h"
 
-int p54_parse_firmware(struct ieee80211_hw *dev, const struct firmware *fw)
+int p54_parse_firmware(struct ieee80211_hw *dev,
+		       const struct sysdata_file *fw)
 {
 	struct p54_common *priv = dev->priv;
 	struct exp_if *exp_if;
diff --git a/drivers/net/wireless/intersil/p54/led.c b/drivers/net/wireless/intersil/p54/led.c
index 9a8fedd3c0f5..b9f18082b1ff 100644
--- a/drivers/net/wireless/intersil/p54/led.c
+++ b/drivers/net/wireless/intersil/p54/led.c
@@ -16,7 +16,7 @@
  * published by the Free Software Foundation.
  */
 
-#include <linux/firmware.h>
+#include <linux/sysdata.h>
 #include <linux/etherdevice.h>
 
 #include <net/mac80211.h>
diff --git a/drivers/net/wireless/intersil/p54/main.c b/drivers/net/wireless/intersil/p54/main.c
index d5a3bf91a03e..f000e39f677a 100644
--- a/drivers/net/wireless/intersil/p54/main.c
+++ b/drivers/net/wireless/intersil/p54/main.c
@@ -17,7 +17,7 @@
  */
 
 #include <linux/slab.h>
-#include <linux/firmware.h>
+#include <linux/sysdata.h>
 #include <linux/etherdevice.h>
 #include <linux/module.h>
 
diff --git a/drivers/net/wireless/intersil/p54/p54.h b/drivers/net/wireless/intersil/p54/p54.h
index 529939e611cd..1747119921d6 100644
--- a/drivers/net/wireless/intersil/p54/p54.h
+++ b/drivers/net/wireless/intersil/p54/p54.h
@@ -268,7 +268,8 @@ struct p54_common {
 /* interfaces for the drivers */
 int p54_rx(struct ieee80211_hw *dev, struct sk_buff *skb);
 void p54_free_skb(struct ieee80211_hw *dev, struct sk_buff *skb);
-int p54_parse_firmware(struct ieee80211_hw *dev, const struct firmware *fw);
+int p54_parse_firmware(struct ieee80211_hw *dev,
+		       const struct sysdata_file *fw);
 int p54_parse_eeprom(struct ieee80211_hw *dev, void *eeprom, int len);
 int p54_read_eeprom(struct ieee80211_hw *dev);
 
diff --git a/drivers/net/wireless/intersil/p54/p54pci.c b/drivers/net/wireless/intersil/p54/p54pci.c
index 27a49068d32d..e65497de5454 100644
--- a/drivers/net/wireless/intersil/p54/p54pci.c
+++ b/drivers/net/wireless/intersil/p54/p54pci.c
@@ -15,7 +15,7 @@
 
 #include <linux/pci.h>
 #include <linux/slab.h>
-#include <linux/firmware.h>
+#include <linux/sysdata.h>
 #include <linux/etherdevice.h>
 #include <linux/delay.h>
 #include <linux/completion.h>
@@ -490,7 +490,7 @@ static int p54p_open(struct ieee80211_hw *dev)
 	return 0;
 }
 
-static void p54p_firmware_step2(const struct firmware *fw,
+static void p54p_firmware_step2(const struct sysdata_file *fw,
 				void *context)
 {
 	struct p54p_priv *priv = context;
@@ -520,8 +520,6 @@ static void p54p_firmware_step2(const struct firmware *fw,
 
 out:
 
-	complete(&priv->fw_loaded);
-
 	if (err) {
 		struct device *parent = pdev->dev.parent;
 
@@ -542,6 +540,17 @@ out:
 	pci_dev_put(pdev);
 }
 
+static int p54p_load_firmware(struct p54p_priv *priv)
+{
+	const struct sysdata_file_desc sysdata_desc = {
+		SYSDATA_KEEP_ASYNC(p54p_firmware_step2, priv),
+	};
+
+	return sysdata_file_request_async("isl3886pci", &sysdata_desc,
+					  &priv->pdev->dev,
+					  &priv->fw_async_cookie);
+}
+
 static int p54p_probe(struct pci_dev *pdev,
 				const struct pci_device_id *id)
 {
@@ -595,7 +604,6 @@ static int p54p_probe(struct pci_dev *pdev,
 	priv = dev->priv;
 	priv->pdev = pdev;
 
-	init_completion(&priv->fw_loaded);
 	SET_IEEE80211_DEV(dev, &pdev->dev);
 	pci_set_drvdata(pdev, dev);
 
@@ -620,9 +628,7 @@ static int p54p_probe(struct pci_dev *pdev,
 	spin_lock_init(&priv->lock);
 	tasklet_init(&priv->tasklet, p54p_tasklet, (unsigned long)dev);
 
-	err = request_firmware_nowait(THIS_MODULE, 1, "isl3886pci",
-				      &priv->pdev->dev, GFP_KERNEL,
-				      priv, p54p_firmware_step2);
+	err = p54p_load_firmware(priv);
 	if (!err)
 		return 0;
 
@@ -652,9 +658,9 @@ static void p54p_remove(struct pci_dev *pdev)
 		return;
 
 	priv = dev->priv;
-	wait_for_completion(&priv->fw_loaded);
+	sysdata_synchronize_request(priv->fw_async_cookie);
 	p54_unregister_common(dev);
-	release_firmware(priv->firmware);
+	release_sysdata_file(priv->firmware);
 	pci_free_consistent(pdev, sizeof(*priv->ring_control),
 			    priv->ring_control, priv->ring_control_dma);
 	iounmap(priv->map);
diff --git a/drivers/net/wireless/intersil/p54/p54pci.h b/drivers/net/wireless/intersil/p54/p54pci.h
index 68405c142f97..4477f6b943de 100644
--- a/drivers/net/wireless/intersil/p54/p54pci.h
+++ b/drivers/net/wireless/intersil/p54/p54pci.h
@@ -94,7 +94,7 @@ struct p54p_priv {
 	struct pci_dev *pdev;
 	struct p54p_csr __iomem *map;
 	struct tasklet_struct tasklet;
-	const struct firmware *firmware;
+	const struct sysdata_file *firmware;
 	spinlock_t lock;
 	struct p54p_ring_control *ring_control;
 	dma_addr_t ring_control_dma;
@@ -105,7 +105,7 @@ struct p54p_priv {
 	struct sk_buff *tx_buf_data[32];
 	struct sk_buff *tx_buf_mgmt[4];
 	struct completion boot_comp;
-	struct completion fw_loaded;
+	async_cookie_t fw_async_cookie;
 };
 
 #endif /* P54USB_H */
diff --git a/drivers/net/wireless/intersil/p54/p54spi.c b/drivers/net/wireless/intersil/p54/p54spi.c
index 7ab2f43ab425..b38ac2a0b83b 100644
--- a/drivers/net/wireless/intersil/p54/p54spi.c
+++ b/drivers/net/wireless/intersil/p54/p54spi.c
@@ -23,7 +23,7 @@
 #include <linux/module.h>
 #include <linux/platform_device.h>
 #include <linux/interrupt.h>
-#include <linux/firmware.h>
+#include <linux/sysdata.h>
 #include <linux/delay.h>
 #include <linux/irq.h>
 #include <linux/spi/spi.h>
@@ -162,53 +162,74 @@ static int p54spi_spi_write_dma(struct p54s_priv *priv, __le32 base,
 	return 0;
 }
 
+static int p54spi_request_firmware_found_cb(void *context,
+					    const struct sysdata_file *sysdata)
+{
+	int ret;
+	struct p54s_priv *priv = context;
+
+	priv->firmware = sysdata;
+	ret = p54_parse_firmware(priv->hw, priv->firmware);
+	if (ret)
+		release_sysdata_file(priv->firmware);
+
+	return ret;
+}
+
 static int p54spi_request_firmware(struct ieee80211_hw *dev)
 {
 	struct p54s_priv *priv = dev->priv;
+	const struct sysdata_file_desc sysdata_desc = {
+		SYSDATA_KEEP_SYNC(p54spi_request_firmware_found_cb, priv),
+	};
 	int ret;
 
 	/* FIXME: should driver use it's own struct device? */
-	ret = request_firmware(&priv->firmware, "3826.arm", &priv->spi->dev);
-
+	ret = sysdata_file_request("3826.arm", &sysdata_desc, &priv->spi->dev);
 	if (ret < 0) {
-		dev_err(&priv->spi->dev, "request_firmware() failed: %d", ret);
-		return ret;
+		dev_err(&priv->spi->dev,
+			"request_firmware() failed: %d", ret);
 	}
+	return ret;
+}
 
-	ret = p54_parse_firmware(dev, priv->firmware);
-	if (ret) {
-		release_firmware(priv->firmware);
-		return ret;
-	}
+#ifdef CONFIG_P54_SPI_DEFAULT_EEPROM
+static int p54spi_load_eeprom_default(void *context)
+{
+	struct p54s_priv *priv = context;
+	struct ieee80211_hw *dev = priv->hw;
 
-	return 0;
+	dev_info(&priv->spi->dev, "loading default eeprom...\n");
+	return p54_parse_eeprom(dev, (void *) p54spi_eeprom,
+				sizeof(p54spi_eeprom));
 }
+#endif
+
+static int p54spi_load_eeprom_cb(void *context,
+				 const struct sysdata_file *sysdata)
+{
+	struct p54s_priv *priv = context;
+	struct ieee80211_hw *dev = priv->hw;
 
+	dev_info(&priv->spi->dev, "loading user eeprom...\n");
+	return p54_parse_eeprom(dev, (void *) sysdata->data,
+			        (int)sysdata->size);
+}
 static int p54spi_request_eeprom(struct ieee80211_hw *dev)
 {
 	struct p54s_priv *priv = dev->priv;
-	const struct firmware *eeprom;
-	int ret;
+	const struct sysdata_file_desc sysdata_desc = {
+		SYSDATA_DEFAULT_SYNC(p54spi_load_eeprom_cb, priv),
+#ifdef CONFIG_P54_SPI_DEFAULT_EEPROM
+		SYSDATA_SYNC_OPT_CB(p54spi_load_eeprom_default, priv),
+#endif
+	};
 
 	/* allow users to customize their eeprom.
 	 */
 
-	ret = request_firmware_direct(&eeprom, "3826.eeprom", &priv->spi->dev);
-	if (ret < 0) {
-#ifdef CONFIG_P54_SPI_DEFAULT_EEPROM
-		dev_info(&priv->spi->dev, "loading default eeprom...\n");
-		ret = p54_parse_eeprom(dev, (void *) p54spi_eeprom,
-				       sizeof(p54spi_eeprom));
-#else
-		dev_err(&priv->spi->dev, "Failed to request user eeprom\n");
-#endif /* CONFIG_P54_SPI_DEFAULT_EEPROM */
-	} else {
-		dev_info(&priv->spi->dev, "loading user eeprom...\n");
-		ret = p54_parse_eeprom(dev, (void *) eeprom->data,
-				       (int)eeprom->size);
-		release_firmware(eeprom);
-	}
-	return ret;
+	return sysdata_file_request("3826.eeprom", &sysdata_desc,
+				    &priv->spi->dev);
 }
 
 static int p54spi_upload_firmware(struct ieee80211_hw *dev)
@@ -692,7 +713,7 @@ static int p54spi_remove(struct spi_device *spi)
 
 	gpio_free(p54spi_gpio_power);
 	gpio_free(p54spi_gpio_irq);
-	release_firmware(priv->firmware);
+	release_sysdata_file(priv->firmware);
 
 	mutex_destroy(&priv->mutex);
 
diff --git a/drivers/net/wireless/intersil/p54/p54spi.h b/drivers/net/wireless/intersil/p54/p54spi.h
index dfaa62aaeb07..f4f20ff7df79 100644
--- a/drivers/net/wireless/intersil/p54/p54spi.h
+++ b/drivers/net/wireless/intersil/p54/p54spi.h
@@ -119,7 +119,7 @@ struct p54s_priv {
 	struct list_head tx_pending;
 
 	enum fw_state fw_state;
-	const struct firmware *firmware;
+	const struct sysdata_file *firmware;
 };
 
 #endif /* P54SPI_H */
diff --git a/drivers/net/wireless/intersil/p54/p54usb.c b/drivers/net/wireless/intersil/p54/p54usb.c
index 043bd1c23c19..a46d8ff6c3a9 100644
--- a/drivers/net/wireless/intersil/p54/p54usb.c
+++ b/drivers/net/wireless/intersil/p54/p54usb.c
@@ -15,7 +15,7 @@
 #include <linux/usb.h>
 #include <linux/pci.h>
 #include <linux/slab.h>
-#include <linux/firmware.h>
+#include <linux/sysdata.h>
 #include <linux/etherdevice.h>
 #include <linux/delay.h>
 #include <linux/crc32.h>
@@ -916,14 +916,13 @@ err_out:
 	return ret;
 }
 
-static void p54u_load_firmware_cb(const struct firmware *firmware,
+static void p54u_load_firmware_cb(const struct sysdata_file *firmware,
 				  void *context)
 {
 	struct p54u_priv *priv = context;
 	struct usb_device *udev = priv->udev;
 	int err;
 
-	complete(&priv->fw_wait_load);
 	if (firmware) {
 		priv->fw = firmware;
 		err = p54u_start_ops(priv);
@@ -959,12 +958,14 @@ static int p54u_load_firmware(struct ieee80211_hw *dev,
 {
 	struct usb_device *udev = interface_to_usbdev(intf);
 	struct p54u_priv *priv = dev->priv;
+	const struct sysdata_file_desc sysdata_desc = {
+		SYSDATA_KEEP_ASYNC(p54u_load_firmware_cb, priv),
+	};
 	struct device *device = &udev->dev;
 	int err, i;
 
 	BUILD_BUG_ON(ARRAY_SIZE(p54u_fwlist) != __NUM_P54U_HWTYPES);
 
-	init_completion(&priv->fw_wait_load);
 	i = p54_find_type(priv);
 	if (i < 0)
 		return i;
@@ -973,9 +974,8 @@ static int p54u_load_firmware(struct ieee80211_hw *dev,
 	       p54u_fwlist[i].fw);
 
 	usb_get_dev(udev);
-	err = request_firmware_nowait(THIS_MODULE, 1, p54u_fwlist[i].fw,
-				      device, GFP_KERNEL, priv,
-				      p54u_load_firmware_cb);
+	err = sysdata_file_request_async(p54u_fwlist[i].fw, &sysdata_desc,
+					 device, &priv->fw_async_cookie);
 	if (err) {
 		dev_err(&priv->udev->dev, "(p54usb) cannot load firmware %s "
 					  "(%d)!\n", p54u_fwlist[i].fw, err);
@@ -1069,11 +1069,11 @@ static void p54u_disconnect(struct usb_interface *intf)
 		return;
 
 	priv = dev->priv;
-	wait_for_completion(&priv->fw_wait_load);
+	sysdata_synchronize_request(priv->fw_async_cookie);
 	p54_unregister_common(dev);
 
 	usb_put_dev(interface_to_usbdev(intf));
-	release_firmware(priv->fw);
+	release_sysdata_file(priv->fw);
 	p54_free_common(dev);
 }
 
diff --git a/drivers/net/wireless/intersil/p54/p54usb.h b/drivers/net/wireless/intersil/p54/p54usb.h
index a5f5f0fea3bd..089c62a3c2e7 100644
--- a/drivers/net/wireless/intersil/p54/p54usb.h
+++ b/drivers/net/wireless/intersil/p54/p54usb.h
@@ -153,10 +153,10 @@ struct p54u_priv {
 	spinlock_t lock;
 	struct sk_buff_head rx_queue;
 	struct usb_anchor submitted;
-	const struct firmware *fw;
+	const struct sysdata_file *fw;
 
 	/* asynchronous firmware callback */
-	struct completion fw_wait_load;
+	async_cookie_t fw_async_cookie;
 };
 
 #endif /* P54USB_H */
diff --git a/drivers/net/wireless/intersil/p54/txrx.c b/drivers/net/wireless/intersil/p54/txrx.c
index 1af7da0b386e..eed08ca886d5 100644
--- a/drivers/net/wireless/intersil/p54/txrx.c
+++ b/drivers/net/wireless/intersil/p54/txrx.c
@@ -17,7 +17,7 @@
  */
 
 #include <linux/export.h>
-#include <linux/firmware.h>
+#include <linux/sysdata.h>
 #include <linux/etherdevice.h>
 #include <asm/div64.h>
 
-- 
2.8.2

Re: [PATCH v2 8/8] p54: convert to sysdata API

[Luis R. Rodriguez]

On Thu, Jun 16, 2016 at 02:21:02PM -1000, Linus Torvalds wrote:
> So what is the advantage of this, since it needs to add more lines of code
> than it removes?
>
> It doesn't seem to be a simplification. Quite the reverse.
> 
> Your diffstat of the whole automated conversion did that too.

A lot of the diff stat of automated changes is space changes which consists of
not wanted new lines in one case caused by Coccinelle.  To get an idea by
comparison one would have to cleanup the output. Typically its on par,
sometimes you save, sometimes a bit more code due to the syn case needing a
callback. In this case its more given the non-keep cases which need a callback,
and also some changes around an #ifdef to make code cleaner.

> Why would we ever want to apply a patch like this?

Well, it depends, in terms of API it helps us extend it without having to do
more collateral evolutions. The p54 driver happens to have both sync and async
calls, and also has a few "KEEP" cases, the SmPL grammar does not deal with the
keep cases -- this change to p54 was just a demo, but what I'd much prefer is
to deal with the keep cases by folding the fw into devm to also avoid the
free'ing in drivers just as with the non-keep cases. If that's desirable it
needs discussion given its a significant change. The grammar still produces
more changes than before for the sync cases given a callback is needed, its
just that simple.

Reason this could not wait is folks seem to want to keep extending the API,
which is another reason for this, do we want to put an end to an unflexible
API now or should we wait ?

If we want to wait for devm integration -- that's cool with me, its however
better to release often and release early. So the point of this particular
series is to show where development is at on the front of a new flexible API that
also avoids the usermode helper. I posted this also *now* given I saw the old
API being extended further. So another option is to evaluate a merge now, and
evolve the devm integration later.

Alternatively, since its only 2 drivers that explicitly require the usermode
helper, another option is to just compartamentalize the usermode helper
explicit callers with a its own API now and save the others with a strategy
devised by the current sysdata API, without requiring any changes at all.
This is a significant change though, so still requires discussion. As I
see it this is worth considering just because we get the same end result
if we transform drivers to sysdata or we just compartamentalize the usermode
helper now to the 2 callers.

Ideas, patches, feedback, rants welcomed.

  Luis

Re: [PATCH v2 8/8] p54: convert to sysdata API

[Linus Torvalds]

On Thu, Jun 16, 2016 at 3:36 PM, Luis R. Rodriguez <mcgrof@kernel.org> wrote:
>
> Reason this could not wait is folks seem to want to keep extending the API,
> which is another reason for this, do we want to put an end to an unflexible
> API now or should we wait ?

So I absolutely abhor "changes for changes sake".

If the existing code works for existing drivers, let them keep it.

And if a new interface is truly more flexible, then it should be able
to implement the old interface with no changes, so that drivers
shouldn't need to be changed/upgraded.

Then, drivers that actually _want_ new features, or that can take
advantage of new interfaces to actually make things *simpler*, can
choose to make those changes. But those changes should have real
advantages.

Having to have a callback, or a magical "sysdata_desc" descriptor, and
having a new name ("sysdata") that is less descriptive than the old
one ("firmware") are all in my opinion making the example patch be a
step _backwards_ rather than an improvement. It does not look like a
simpler or more natural interface for a driver.

               Linus

Re: [PATCH v2 8/8] p54: convert to sysdata API

[Luis R. Rodriguez]

On Thu, Jun 16, 2016 at 05:09:30PM -1000, Linus Torvalds wrote:
> On Thu, Jun 16, 2016 at 3:36 PM, Luis R. Rodriguez <mcgrof@kernel.org> wrote:
> >
> > Reason this could not wait is folks seem to want to keep extending the API,
> > which is another reason for this, do we want to put an end to an unflexible
> > API now or should we wait ?
> 
> So I absolutely abhor "changes for changes sake".
> 
> If the existing code works for existing drivers, let them keep it.
> 
> And if a new interface is truly more flexible, then it should be able
> to implement the old interface with no changes, so that drivers
> shouldn't need to be changed/upgraded.

Are we OK to leave only the usermode helper support in place for the
2 drivers I've noted that explicitly require the usermode helper in their
firmware request [0] ?

If so then I can do what you recommend below.

> Then, drivers that actually _want_ new features, or that can take
> advantage of new interfaces to actually make things *simpler*, can
> choose to make those changes. But those changes should have real
> advantages.

Sure agreed.

> Having to have a callback, 

This is because devm is not used, as such the consumer is needed prior to
freeing. I can give adding devm support a shot if folks seem to be generally 
agree its the right approach. I do expect this will mean tons of code
deletions and helping with bugs.

> or a magical "sysdata_desc" descriptor,

This is one way to make a flexible and extensible API without affecting drivers
with further collateral evolutions as it gets extended. Its no different than
the "flags" lesson learned from writing system calls, for instance.

Descriptor seemed, fitting, let me know if you have any other preference.

> and having a new name ("sysdata") that is less descriptive than the old one
> ("firmware")

Well no, the APIs are used in *a lot* of cases for things that are not firmware
already, and let's recall I originally started working on this to replace CRDA
from userspace to be able to just fetch the signed regulatory database file
from the kernel. Calling it firmware simply makes no sense anymore.

> are all in my opinion making the example patch be a
> step _backwards_ rather than an improvement. It does not look like a
> simpler or more natural interface for a driver.

Hope the above explains the current state. Feedback on desirable changes
welcomed.

[0] https://lkml.kernel.org/r/1466117661-22075-5-git-send-email-mcgrof@kernel.org

  Luis

Re: [PATCH v2 0/8] firmware: add new sysdata API

[Luis R. Rodriguez]

On Thu, Jun 16, 2016 at 04:59:11PM -0700, Luis R. Rodriguez wrote:
> FWIW running it against linux-next next-20160616 on a 32-core system takes
> about 3 minutes using glimpse, also ~3 minutes with gitgrep and produces the
> following diffstat:
> 
>  148 files changed, 5676 insertions(+), 4504 deletions(-)

FWIW by using idutils the amount it takes is reduced considerably to 1 minute
for the entire kernel as well.

  Luis

Re: [PATCH v2 0/8] firmware: add new sysdata API

[Luis R. Rodriguez]

On Sat, Jun 18, 2016 at 01:40:43AM +0200, Luis R. Rodriguez wrote:
> On Thu, Jun 16, 2016 at 04:59:11PM -0700, Luis R. Rodriguez wrote:
> > FWIW running it against linux-next next-20160616 on a 32-core system takes
> > about 3 minutes using glimpse, also ~3 minutes with gitgrep and produces the
> > following diffstat:
> > 
> >  148 files changed, 5676 insertions(+), 4504 deletions(-)
> 
> FWIW by using idutils the amount it takes is reduced considerably to 1 minute
> for the entire kernel as well.

Actually... even though idutils seems to be performing better Coccinelle 1.0.5
and even github version as of right now have an issue with idutils when these
SmPL patches are used, so for now please use only gitgrep (default if you merge
the pendign coccicheck enhancements if you are on a git tree with no ID or
.id-utils.index from idutils).

  Luis

Re: [PATCH v2 8/8] p54: convert to sysdata API

[Luis R. Rodriguez]

On Fri, Jun 17, 2016 at 08:35:03PM +0200, Luis R. Rodriguez wrote:
> On Thu, Jun 16, 2016 at 05:09:30PM -1000, Linus Torvalds wrote:
> > On Thu, Jun 16, 2016 at 3:36 PM, Luis R. Rodriguez <mcgrof@kernel.org> wrote:
> > >
> > > Reason this could not wait is folks seem to want to keep extending the API,
> > > which is another reason for this, do we want to put an end to an unflexible
> > > API now or should we wait ?
> > 
> > So I absolutely abhor "changes for changes sake".
> > 
> > If the existing code works for existing drivers, let them keep it.
> > 
> > And if a new interface is truly more flexible, then it should be able
> > to implement the old interface with no changes, so that drivers
> > shouldn't need to be changed/upgraded.
> 
> Are we OK to leave only the usermode helper support in place for the
> 2 drivers I've noted that explicitly require the usermode helper in their
> firmware request [0] ?
> 
> If so then I can do what you recommend below.
> 
> > Then, drivers that actually _want_ new features, or that can take
> > advantage of new interfaces to actually make things *simpler*, can
> > choose to make those changes. But those changes should have real
> > advantages.

As I noted above -- this still needs to be decided.

We only have 2 more drivers using the usermode helper explicitly now. Other
than this we have the old CONFIG_FW_LOADER_USER_HELPER_FALLBACK -- which
explicitly forced the usermode helper on every call. This later option is no
longer widely used by distributions, and distros these days just enable
CONFIG_FW_LOADER_USER_HELPER, with this only 2 drivers are using the usermode
helper. This still should mean we should not break users of
CONFIG_FW_LOADER_USER_HELPER_FALLBACK, as stupid as it may have been.

My preferred approach is as follows (and this is what I'll follow unless
I hear otherwise):

Obviously let's not break CONFIG_FW_LOADER_USER_HELPER_FALLBACK -- even if it
logically means long term most drivers if not all will convert to the new API,
there is no need for a full swing change. Lets leave drivers as-is, given most
distros are sensible and only enabling CONFIG_FW_LOADER_USER_HELPER.  Given
most distros disable CONFIG_FW_LOADER_USER_HELPER_FALLBACK, and we have now
verified only 2 explicit user mode helper scallers exists this means we have
mostly put away most of the sore of the user mode helper. With Daniel Wagner's
change to change the firmware API to use the new swait stuff it further pushes
some other user mode helper crap away [0]. Then, only drivers *that care and
want to change* to the new API will do so but we put a stop gap measure so that
new features only go through the new API. This means we mark
CONFIG_FW_LOADER_USER_HELPER_FALLBACK as deprecated.

We can strive to mark CONFIG_FW_LOADER_USER_HELPER as deprecated but to be fair
this requires more work:

Despite my best efforts to call out for valid new uses of the user mode helper
the only valid use I've heard so far over CONFIG_FW_LOADER_USER_HELPER was for
huge files for remoteproc as explained by Bjorn Andersson given the
deterministic aspect issue of ensuring a file is ready and also that they
cannot use initramfs to stuff the firmware [1]. As recently discussed in that
same thread with Bjorn though we can easily just add this upstream either as
a simple file sentinel or better yet -- a simple event from userspace [2] to
be used either to indicate the rootfs is ready or if we wanted farther
granularity per enum kernel_read_file_id (READING_FIRMWARE, READING_MODULE,
READING_KEXEC_IMAGE, READING_KEXEC_INITRAMFS, READING_POLICY), and that would
seem to put everyone's concerns over direct file loading at ease, including
Bjorn's [1]. This needs to be implemented. When that is done -- unless we hear
otherwise over requirements for the usermode helper -- we can finally mark
CONFIG_FW_LOADER_USER_HELPER as deprecated.

[0] https://lkml.kernel.org/r/1470313636-670-1-git-send-email-wagi@monom.org
[1] https://lkml.kernel.org/r/20160803173955.GD13516@tuxbot
[2] https://lkml.kernel.org/r/20160803184058.GS3296@wotan.suse.de

> Sure agreed.
> 
> > Having to have a callback, 
> 
> This is because devm is not used, as such the consumer is needed prior to
> freeing. I can give adding devm support a shot if folks seem to be generally 
> agree its the right approach. I do expect this will mean tons of code
> deletions and helping with bugs.

Regarding this -- Dmitry recenlty noted devm only works if used on the probe
path, and as we now determined, we don't want firmware loading on probe [3], unless
async probe is used, so this would make a devm solution here not ideal for
freeing the firmware. Even if you use async probe -- that seems very special
use case and adding devm support for the firmware API just for that seems silly.

As such the current devised solution in the sysdata API is called for, given
if you indicated that keep = false, you are explicitly telling the firmware
API that your firmware will certainly not be needed after the callback is
called.

So for the sync case, a new callback is needed, and that explains the extra
bit of code if someone conerts from the old API to the new one.

[3] https://lkml.kernel.org/r/20160803161821.GB32965@dtor-ws

> > or a magical "sysdata_desc" descriptor,
> 
> This is one way to make a flexible and extensible API without affecting drivers
> with further collateral evolutions as it gets extended. Its no different than
> the "flags" lesson learned from writing system calls, for instance.
> 
> Descriptor seemed, fitting, let me know if you have any other preference.

I haven't heard otherwise so will be sticking to that.

> > and having a new name ("sysdata") that is less descriptive than the old one
> > ("firmware")
> 
> Well no, the APIs are used in *a lot* of cases for things that are not firmware
> already, and let's recall I originally started working on this to replace CRDA
> from userspace to be able to just fetch the signed regulatory database file
> from the kernel. Calling it firmware simply makes no sense anymore.

So help me bike shed. This seems to be sticking point and I frankly don't
care what we call it. I've asked others for name suggestions and here are
a few suggestions:

 o driver_data
 o dsd: device specific data
 o xfw - eXtensible firmware API
 o bbl - binary blob loader

Can someone just pick something? I really, really do not care.

If I don't hear anything concrete I will go with driver_data.

> > are all in my opinion making the example patch be a
> > step _backwards_ rather than an improvement. It does not look like a
> > simpler or more natural interface for a driver.
> 
> Hope the above explains the current state. Feedback on desirable changes
> welcomed.
> 
> [0] https://lkml.kernel.org/r/1466117661-22075-5-git-send-email-mcgrof@kernel.org

All this said, this series is still justified, the extra code only comes in
place when porting the sync requests due to the callback stuff described above
and the inability to use devm there. As far as I can tell, just the bike
shedding is left.

As it stands then, unless I hear back, I'll roll Daniel Wagner's changes into
my series to be applied first, then rename sysdata driver_data, rebase all this
and shoot it out again.

Only a few drivers will be converted over as demos. The SmPL grammar can be used
by those who do want a change due to the few features added. Long term we'll
add more features to the new API:

 o the whole ihex conversion is crap, we should do this within the API and
   this can just be specified as a descriptor preference, then drivers
   don't have to deal with the ihex crap themselves.

 o firmware singing - this lets us kill CRDA as a requirement

  Luis

Re: [PATCH v2 8/8] p54: convert to sysdata API

[Arend Van Spriel]

On 10-8-2016 20:32, Luis R. Rodriguez wrote:
> On Fri, Jun 17, 2016 at 08:35:03PM +0200, Luis R. Rodriguez wrote:
>> On Thu, Jun 16, 2016 at 05:09:30PM -1000, Linus Torvalds wrote:
>>> On Thu, Jun 16, 2016 at 3:36 PM, Luis R. Rodriguez <mcgrof@kernel.org> wrote:
>>>>
>>>> Reason this could not wait is folks seem to want to keep extending the API,
>>>> which is another reason for this, do we want to put an end to an unflexible
>>>> API now or should we wait ?

[big snip]

> 
> Regarding this -- Dmitry recenlty noted devm only works if used on the probe
> path, and as we now determined, we don't want firmware loading on probe [3], unless
> async probe is used, so this would make a devm solution here not ideal for
> freeing the firmware. Even if you use async probe -- that seems very special
> use case and adding devm support for the firmware API just for that seems silly.

So why would drivers want the devm solution anyway. Once firmware is
loaded in the device it can be freed or do you expect device drivers to
keep the firmware/sysdata for suspend/resume scenario as some do because
of firmware cache behaviour. Does the "rootfs ready" event cover
suspend/resume?

> As such the current devised solution in the sysdata API is called for, given
> if you indicated that keep = false, you are explicitly telling the firmware
> API that your firmware will certainly not be needed after the callback is
> called.
> 
> So for the sync case, a new callback is needed, and that explains the extra
> bit of code if someone conerts from the old API to the new one.
> 
> [3] https://lkml.kernel.org/r/20160803161821.GB32965@dtor-ws
> 
>>> or a magical "sysdata_desc" descriptor,
>>
>> This is one way to make a flexible and extensible API without affecting drivers
>> with further collateral evolutions as it gets extended. Its no different than
>> the "flags" lesson learned from writing system calls, for instance.
>>
>> Descriptor seemed, fitting, let me know if you have any other preference.
> 
> I haven't heard otherwise so will be sticking to that.

How about sysdata_req{,uest}_params?

>>> and having a new name ("sysdata") that is less descriptive than the old one
>>> ("firmware")
>>
>> Well no, the APIs are used in *a lot* of cases for things that are not firmware
>> already, and let's recall I originally started working on this to replace CRDA
>> from userspace to be able to just fetch the signed regulatory database file
>> from the kernel. Calling it firmware simply makes no sense anymore.
> 
> So help me bike shed. This seems to be sticking point and I frankly don't
> care what we call it. I've asked others for name suggestions and here are
> a few suggestions:
> 
>  o driver_data
>  o dsd: device specific data
>  o xfw - eXtensible firmware API
>  o bbl - binary blob loader
> 
> Can someone just pick something? I really, really do not care.
> 
> If I don't hear anything concrete I will go with driver_data.

bit of skin crawling here, but not enough to care.

>>> are all in my opinion making the example patch be a
>>> step _backwards_ rather than an improvement. It does not look like a
>>> simpler or more natural interface for a driver.
>>
>> Hope the above explains the current state. Feedback on desirable changes
>> welcomed.
>>
>> [0] https://lkml.kernel.org/r/1466117661-22075-5-git-send-email-mcgrof@kernel.org
> 
> All this said, this series is still justified, the extra code only comes in
> place when porting the sync requests due to the callback stuff described above
> and the inability to use devm there. As far as I can tell, just the bike
> shedding is left.
> 
> As it stands then, unless I hear back, I'll roll Daniel Wagner's changes into
> my series to be applied first, then rename sysdata driver_data, rebase all this
> and shoot it out again.
> 
> Only a few drivers will be converted over as demos. The SmPL grammar can be used
> by those who do want a change due to the few features added. Long term we'll
> add more features to the new API:
> 
>  o the whole ihex conversion is crap, we should do this within the API and
>    this can just be specified as a descriptor preference, then drivers
>    don't have to deal with the ihex crap themselves.
> 
>  o firmware singing - this lets us kill CRDA as a requirement

Strongly suspect a typo here :-p

Regards,
Arend

Re: [PATCH v2 8/8] p54: convert to sysdata API

[Mark Brown]

[-- Attachment #1: Type: text/plain, Size: 811 bytes --]

On Wed, Aug 10, 2016 at 09:04:38PM +0200, Arend Van Spriel wrote:

> So why would drivers want the devm solution anyway. Once firmware is
> loaded in the device it can be freed or do you expect device drivers to
> keep the firmware/sysdata for suspend/resume scenario as some do because
> of firmware cache behaviour. Does the "rootfs ready" event cover
> suspend/resume?

There are classes of devices that spend a large proportion of their time
powered off and are only powered up and have firmware loaded when they
are actively in use.  DSPs used in audio systems are one example of
this, I'd not be surprised to learn that similar things are done with
video.  It's too expensive to keep the device powered up and you may be
swapping between firmwares depending on use case anyway for a lot of
these devices.

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 473 bytes --]

Re: [PATCH v2 8/8] p54: convert to sysdata API

[Luis R. Rodriguez]

On Wed, Aug 10, 2016 at 09:04:38PM +0200, Arend Van Spriel wrote:
> On 10-8-2016 20:32, Luis R. Rodriguez wrote:
> > On Fri, Jun 17, 2016 at 08:35:03PM +0200, Luis R. Rodriguez wrote:
> >> On Thu, Jun 16, 2016 at 05:09:30PM -1000, Linus Torvalds wrote:
> >>> On Thu, Jun 16, 2016 at 3:36 PM, Luis R. Rodriguez <mcgrof@kernel.org> wrote:
> >>>>
> >>>> Reason this could not wait is folks seem to want to keep extending the API,
> >>>> which is another reason for this, do we want to put an end to an unflexible
> >>>> API now or should we wait ?
> 
> [big snip]
> 
> > 
> > Regarding this -- Dmitry recenlty noted devm only works if used on the probe
> > path, and as we now determined, we don't want firmware loading on probe [3], unless
> > async probe is used, so this would make a devm solution here not ideal for
> > freeing the firmware. Even if you use async probe -- that seems very special
> > use case and adding devm support for the firmware API just for that seems silly.
> 
> So why would drivers want the devm solution anyway.

It depends on the use case, some may want to keep the firmware around,
some may not, but by default the new API ssumes you will not (keep = false)
and we free it. My point above was that using devm will not typically
be the most fruitful solution to free the firmware givne that there are only
a few drivers that should need it upon probe, and drivers using firmware
APIs on probe shoudld be using async probe anyway to delay avoid delaying
boot as processing the firmware can take time.

> Once firmware is
> loaded in the device it can be freed or do you expect device drivers to
> keep the firmware/sysdata for suspend/resume scenario as some do because
> of firmware cache behaviour.

You would think! Upon careful inspection there are tons of odd things drivers
do, some modify the firmware and therefore have their own reasons to keep
it. Whatever the reasons are -- I recall seeing a few well justified uses.

> Does the "rootfs ready" event cover
> suspend/resume?

The "rootfs ready" is orthogonal to suspend/resume case given the firmware_class
cache firmware feature.

The firmware API as-is upstream already has a cache firmware solution added
a long time ago, as reflected by a resent patch set (before this one) I
updated documentation for this given its not clearly well known and people
keep adding their own caching solutions, the firmware API requests firmware
prior to suspend so that upon resume the firmware is present, precisely to
avoid race issues. I will note that this feature is only for non-usermode helper
firmware API, upon suspend we *kill* any pending user mode helper requests
given that this can delay suspend, as such drivers using or relying (only
2) on the user mode helper for firmware have no solution built in for
the cache stuff and I can't say I care given there seems to be no valid
modern use case given as a requirement for it yet. In fact there was a bug
in the firmware_class code that *allows* the cache call to request the
usermode helper, obviously that's buggy if we are trying to kill the
usermode helper upon suspend...  so a pending patch fixes that. That's been
a long standing bug and surprised no one ever picked up on it.

> > As such the current devised solution in the sysdata API is called for, given
> > if you indicated that keep = false, you are explicitly telling the firmware
> > API that your firmware will certainly not be needed after the callback is
> > called.
> > 
> > So for the sync case, a new callback is needed, and that explains the extra
> > bit of code if someone conerts from the old API to the new one.
> > 
> > [3] https://lkml.kernel.org/r/20160803161821.GB32965@dtor-ws
> > 
> >>> or a magical "sysdata_desc" descriptor,
> >>
> >> This is one way to make a flexible and extensible API without affecting drivers
> >> with further collateral evolutions as it gets extended. Its no different than
> >> the "flags" lesson learned from writing system calls, for instance.
> >>
> >> Descriptor seemed, fitting, let me know if you have any other preference.
> > 
> > I haven't heard otherwise so will be sticking to that.
> 
> How about sysdata_req{,uest}_params?

Thanks will go with that.

> >>> and having a new name ("sysdata") that is less descriptive than the old one
> >>> ("firmware")
> >>
> >> Well no, the APIs are used in *a lot* of cases for things that are not firmware
> >> already, and let's recall I originally started working on this to replace CRDA
> >> from userspace to be able to just fetch the signed regulatory database file
> >> from the kernel. Calling it firmware simply makes no sense anymore.
> > 
> > So help me bike shed. This seems to be sticking point and I frankly don't
> > care what we call it. I've asked others for name suggestions and here are
> > a few suggestions:
> > 
> >  o driver_data
> >  o dsd: device specific data
> >  o xfw - eXtensible firmware API
> >  o bbl - binary blob loader
> > 
> > Can someone just pick something? I really, really do not care.
> > 
> > If I don't hear anything concrete I will go with driver_data.
> 
> bit of skin crawling here, but not enough to care.

Bike shedding is so much fun. Not.

> >>> are all in my opinion making the example patch be a
> >>> step _backwards_ rather than an improvement. It does not look like a
> >>> simpler or more natural interface for a driver.
> >>
> >> Hope the above explains the current state. Feedback on desirable changes
> >> welcomed.
> >>
> >> [0] https://lkml.kernel.org/r/1466117661-22075-5-git-send-email-mcgrof@kernel.org
> > 
> > All this said, this series is still justified, the extra code only comes in
> > place when porting the sync requests due to the callback stuff described above
> > and the inability to use devm there. As far as I can tell, just the bike
> > shedding is left.
> > 
> > As it stands then, unless I hear back, I'll roll Daniel Wagner's changes into
> > my series to be applied first, then rename sysdata driver_data, rebase all this
> > and shoot it out again.
> > 
> > Only a few drivers will be converted over as demos. The SmPL grammar can be used
> > by those who do want a change due to the few features added. Long term we'll
> > add more features to the new API:
> > 
> >  o the whole ihex conversion is crap, we should do this within the API and
> >    this can just be specified as a descriptor preference, then drivers
> >    don't have to deal with the ihex crap themselves.
> > 
> >  o firmware singing - this lets us kill CRDA as a requirement
> 
> Strongly suspect a typo here :-p

Indeed, thanks :)

  Luis

Re: [PATCH v2 8/8] p54: convert to sysdata API

[Luis R. Rodriguez]

On Wed, Aug 10, 2016 at 08:17:08PM +0100, Mark Brown wrote:
> On Wed, Aug 10, 2016 at 09:04:38PM +0200, Arend Van Spriel wrote:
> 
> > So why would drivers want the devm solution anyway. Once firmware is
> > loaded in the device it can be freed or do you expect device drivers to
> > keep the firmware/sysdata for suspend/resume scenario as some do because
> > of firmware cache behaviour. Does the "rootfs ready" event cover
> > suspend/resume?
> 
> There are classes of devices that spend a large proportion of their time
> powered off and are only powered up and have firmware loaded when they
> are actively in use.  DSPs used in audio systems are one example of
> this, I'd not be surprised to learn that similar things are done with
> video.  It's too expensive to keep the device powered up and you may be
> swapping between firmwares depending on use case anyway for a lot of
> these devices.

devm use and probe is orthogonal to this use case and suspend/resume.
In any case the proposed new API simply allows the driver caller to
either free the firmware after the consumer callback or to keep it.
It does this without devm, so allows this feature without the API
being used on probe. The point about devm was that it would only
beneficial to us if most users for the API were on probe. Clearly
that's not the case and in fact unless async probe is used that use
is likely buggy as it would delay boot further.

If having a local cache of firmware *beyond* the suspend / resume
case is an optimization that can help we should look into that, but
my preference would be to peg this onto the API through an optional
setting in the request passed.

  Luis

Re: [PATCH v2 1/8] firmware: add new extensible firmware API - sysdata_file_request*()

[Bjorn Andersson]

On Thu, Jun 16, 2016 at 4:59 PM, Luis R. Rodriguez <mcgrof@kernel.org> wrote:
> The firmware API has evolved over the years slowly, as it
> grows we extend it by adding new routines or at times we extend
> existing routines with more or less arguments. This doesn't scale
> well, when new arguments are added to existing routines it means
> we need to traverse the kernel with a slew of collateral
> evolutions to adjust old driver users. The firmware API is also
> now being used for things outside of the scope of what typically
> would be considered "firmware", an example here is the p54 driver
> enables users to provide a custom EEPROM through this interface.
> Another example is optional CPU microcode updates. This list is
> actually quite endless...
>

Why can't this done in an incremental fashion, like other frameworks
has done, by transitioning the existing APIs to take a argument
structure?

How are these cases of "misuse" going to go away with the introduction
of another non-firmware-loading interface?

> There are other subsystems which would like to make use of the
> APIs for similar things (not firmware) but have different
> requirements and criteria which they'd like to be met for the
> requested file. If different requirements are needed it would
> again mean adding more arguments and making a slew of collateral
> evolutions, or adding yet-another-new-API-call.
>

Is the main problem here that it's named "firmware" or that there are
potential requirements that are inconsistent with something loading
"firmware"?

[..]
>
>  - Usermode helpers is completely ignored, *always*

What technical benefit does this give us?


As discussed elsewhere, having a mechanism for postponing firmware
loading until the appropriate file systems are mounted would remove my
dependency on the usermode helper. But the direction discussed would
be unrelated to firmware vs sysdata.

Regards,
Bjorn

Re: [PATCH v2 1/8] firmware: add new extensible firmware API - sysdata_file_request*()

[Luis R. Rodriguez]

On Thu, Aug 11, 2016 at 02:15:31PM -0700, Bjorn Andersson wrote:
> On Thu, Jun 16, 2016 at 4:59 PM, Luis R. Rodriguez <mcgrof@kernel.org> wrote:
> > The firmware API has evolved over the years slowly, as it
> > grows we extend it by adding new routines or at times we extend
> > existing routines with more or less arguments. This doesn't scale
> > well, when new arguments are added to existing routines it means
> > we need to traverse the kernel with a slew of collateral
> > evolutions to adjust old driver users. The firmware API is also
> > now being used for things outside of the scope of what typically
> > would be considered "firmware", an example here is the p54 driver
> > enables users to provide a custom EEPROM through this interface.
> > Another example is optional CPU microcode updates. This list is
> > actually quite endless...
> >
> 
> Why can't this done in an incremental fashion, like other frameworks
> has done, by transitioning the existing APIs to take a argument
> structure?

This is proposed as incremental, the grammar helpers are there to help,
not to force a full sweep change.

> How are these cases of "misuse" going to go away with the introduction
> of another non-firmware-loading interface?

For starters maintainers would ask developer to extend functionality
via the new descriptor (or whatever we end up calling it).

> > There are other subsystems which would like to make use of the
> > APIs for similar things (not firmware) but have different
> > requirements and criteria which they'd like to be met for the
> > requested file. If different requirements are needed it would
> > again mean adding more arguments and making a slew of collateral
> > evolutions, or adding yet-another-new-API-call.
> >
> 
> Is the main problem here that it's named "firmware" or that there are
> potential requirements that are inconsistent with something loading
> "firmware"?

We will replace CRDA with an in-kernel API call to fetch the regulatory.bin, it
makes no sense to be referring to regulatory.bin as firmware. There are other
uses already, device configuration, EEPROM default data, etc. In the case to
replace CRDA we will want signature data, and we may want to have some key
specified. To add support for firmware singing we will want to extend yet again
the firmware API with another argument, with the flexible API this will just
be an added entry into the descriptor.

> [..]
> >
> >  - Usermode helpers is completely ignored, *always*
> 
> What technical benefit does this give us?

It means we can compartamentalize the user mode helper. Because...

> 
> As discussed elsewhere, having a mechanism for postponing firmware
> loading until the appropriate file systems are mounted would remove my
> dependency on the usermode helper.

This is the only reason stated so far as why some folks want to use it.
That and some legacy crap (2 drivers). As I have noted elsewhere though
too, this sort of deterministic loading is a generic core fs/exec.c
enhancement for the kernel, not just a firmware thing any more as we
share the same core APIs for reading files now.

> But the direction discussed would
> be unrelated to firmware vs sysdata.

Right, the name chosen for the API is orthogonal. Think of it as two circles,
device data or sysdata and then firmware. The new flexible API just acknowledges
that the first exists and is going to be used, so for instance 802.11 will use it
eventually for regulatory.bin.

  Luis