firmware: add drvdata API

This series is intended for considering *after* the merge window.

This v3 takes off from where we I last left off [0]. I've put aside patches not relating to this topic into its own separate small series [1]. Given the huge amount of confusion over the firmware API in general and also the firmware "usermode helper" in that series I also revamped the entire firmware documentation. Folks should stop calling it firmware "usermode helper" and simply referring to it as "fallback mechanisms" to avoid any further confusion.

I followed up with quite a few folks at Plumbers and over e-mail have been having quite a bit of productive exchanges for the outlook for the future of what a clean slate for fallback mechanisms can look like, but one thing is clear -- the current set of fallback mechanisms are quite a mess. The purpose of this series is to only add an extensible firmware API onto which we can rely on in the future for new features.

The fallback mechanism is not addressed yet given the old uevent fallback mechanism was ripped out form systemd a while ago, and so considering a clean solution means coordinating well with the systemd folks. This is in no way urgent given we have only 2 upstream drivers usign the custom fallback mechanism, and the purpose of the new extensible API is new features. Part of the new challenges we've faced (like the pivot root dillema) have been considered by systemd folks and others and in particular development lead by Tom Gundersen and Daniel Wagner are addressing the issues we had not been able to resolve yet in kernelspace by a userspace solution. For detalis check out their latest firmwared development [2] work, which is still being advanced. This is what we intend on relying on for a clean slate solution for a firmware fallback mechanism eventually.

I've also droped the SmPL patch to help developers convert drivers for two reasons:

1) We had some bikeshedding name changes and I haven't had time to port it and test it
2) It seems some folks were under the incorrect impression that the goal was to convert all drivers -- that's not the case, the goal is to add a new clean *extensible* API which lets us add new features without the mess we have with the current API.

As I had noted in the last series the reason for why using the drvdata API can incur a bit more code is we require a callback for the sync call so we can deal with freeing your firmware for you, as an option. The older API didn't do this.

This converts two drivers just as an example for now, more examples are available on the test_drvdata driver.

If you want to test pulling the code its up on my linux-next tree on the 20161216-drvdata-v3 branch.

[0] https://lkml.kernel.org/r/[email protected]
[1] https://lkml.kernel.org/r/[email protected]
[2] https://github.com/teg/firmwared
[3] https://git.kernel.org/cgit/linux/kernel/git/mcgrof/linux-next.git/log/?h=20161216-drvdata-v3

Luis R. Rodriguez (4):
firmware: add new extensible firmware API - drvdata
test: add new drvdata loader tester x86/microcode: convert to use sysdata API
p54: convert to sysdata API

 Documentation/driver-api/firmware/drvdata.rst      |   91 ++
 Documentation/driver-api/firmware/index.rst        |    1 +
 Documentation/driver-api/firmware/introduction.rst |   11 +
 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         |   80 +-
 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/drvdata.h                            |  245 +++++
 lib/Kconfig.debug                                  |   12 +
 lib/Makefile                                       |    1 +
 lib/test_drvdata.c                                 | 1033 ++++++++++++++++++++
 tools/testing/selftests/firmware/Makefile          |    2 +-
 tools/testing/selftests/firmware/config            |    1 +
 tools/testing/selftests/firmware/drvdata.sh        |  827 ++++++++++++++++
 25 files changed, 2679 insertions(+), 81 deletions(-)
 create mode 100644 Documentation/driver-api/firmware/drvdata.rst
 create mode 100644 include/linux/drvdata.h
 create mode 100644 lib/test_drvdata.c
 create mode 100755 tools/testing/selftests/firmware/drvdata.sh

2.10.1

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 and its clearly 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 (TM).

Another sticking point over the current firmware API is that some callers may need the firmware fallback mechanism when its enabled. There are two types of fallback mechanisms and both have shortcomings. This new API accepts the current status quo and ignore the fallback mechanism all together. When and if we add support for it, it will be well though out.

This new extensible firmware API enables new extensions to be added by avoiding future unnecessary collateral evolutions as this code / features get added. This new set of APIs leaves the old firmware API as-is, ignores all firmware fallback mechanism, labels the new API to reflect its broad use outside of the scope of firmware: driver data helpers, and builds on top of the original firmware core code. We purposely try to limit the scope of changes in this new API to simply enable a flexible API to start off with.

The new extensible "driver data" set of helpers accepts that there really are only two types of requests for accessing driver 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 struct drvdata_req_params to each type of request. This struct can be extended over time to support different requirements as the kernel evolves.

Using the new driver 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 old firmware fallback mechanism at all times, regardless of what kernel your driver might run in.

Developers with new uses should extend the new new struct drvdata_req_params and driver data code to provide support for new features.

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

- The firmware fallback mechanism is currenlty always ignored
- By default the kernel will free the driver data file for you after your callbacks are called, you however are allowed to request that you wish to keep the driver data file on the descriptor. The new drvdata API is able to free the drvdata file for you by requiring a consumer callback for the driver data file.
- You no longer need to declare and use your own completions, you can replace your completions with drvdata_synchronize_request() using the async_cookie set for you by drvdata_file_request_async(). When drvdata_file_request_async() completes you can rest assured all the work for both triggering, and processing the drvdata using any of your callbacks has completed.
- Allow both asynchronous and synchronous request to specify that driver 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 one of its goal was to also skip the fallback mechanisms. The driver data request APIs allow for you to annotate that a driver data file is optional for both synchronous or asynchronous requests through the same two basic set of APIs.
- The driver 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 driver data request APIs match the old asynchronous firmware API call and refcounts the caller's module.

v5 changes:

o Bike shedding:
o sysdata/drvdata
o sysdata_req_desc/drvdata_req_params
o Documentation conversion to Sphinx

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()).

---
 Documentation/driver-api/firmware/drvdata.rst      |  91 ++++++
 Documentation/driver-api/firmware/index.rst        |   1 +
 Documentation/driver-api/firmware/introduction.rst |  11 +
 MAINTAINERS                                        |   3 +-
 drivers/base/firmware_class.c                      | 327 +++++++++++++++++++++
 include/linux/drvdata.h                            | 245 +++++++++++++++
 6 files changed, 677 insertions(+), 1 deletion(-)
 create mode 100644 Documentation/driver-api/firmware/drvdata.rst
 create mode 100644 include/linux/drvdata.h

new file mode 100644
index 000000000000..0445d49cd974
@@ -0,0 +1,91 @@
+===========
+drvdata 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
+driver 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 driver data
+request set of APIs addresses rebranding of firmware as generic driver data
+files, and provides a way to enable these APIs to easily be extended without
+much collateral evolutions.
+
+Driver data modes of operation
+==============================
+
+There are only two types of modes of operation for system data requests:
+
+  * synchronous  - drvdata_request()
+  * asynchronous - drvdata_request_async()
+
+Synchronous requests expect requests to be done immediately, asynchronous
+requests enable requests to be scheduled for a later time.
+
+Driver data request parameters
+==============================
+
+Variations of types of driver data requests are specified by a driver data
+request parameter data structure. This data structure 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 "driver data file" is optional.  The driver data
+request API allows a caller to set the optional nature of the driver data
+on the request parameter data structure using the same synchronous API. Since
+this requirement is part of the request paramter data structure it also allows
+asynchronous requests to specify that the driver data 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 driver data requests. This prevents removal
+of the device and module making the driver data request call until the
+driver 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 driver 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 "driver data", consuming it after a request and finally
+freeing it. Typical asynchronous use of the old firmware APIs consist of
+the caller requesting for "driver data" and then finally freeing it on
+asynchronous callback.
+
+The driver 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 driver data around after your callbacks
+you must specify this through the driver data request paramter data structure.
+
+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 drvdata_synchronize_request() using the
+async_cookie set for you by drvdata_file_request_async(). When
+drvdata_file_request_async() completes you can rest assured all the work for
+both triggering, and processing the drvdata using any of your callbacks has
+completed.
+
+Fallback mechanisms on the driver data API
+==========================================
+
+The old firmware API provided support for a series of fallback mechanisms. The
+new driver data API abandons all current notions of the fallback mechanisms,
+it may soon add support for one though.
+
+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

index 1abe01793031..8d275c4c165b 100644
@@ -7,6 +7,7 @@ Linux Firmware API
    introduction
    core
    request_firmware
+   drvdata
 
 .. only::  subproject and html
 

index 211cb44eb972..d7d5ef846ca0 100644
@@ -25,3 +25,14 @@ are already using asynchronous initialization mechanisms which will not
 stall or delay boot. Even if loading firmware does not take a lot of time
 processing firmware might, and this can still delay boot or initialization,
 as such mechanisms such as asynchronous probe can help supplement drivers.
+
+Two APIs
+========
+
+Two APIs are provided for firmware:
+
+* request_firmware API - old firmware API
+* drvdata API - new flexible API
+
+New features and development should be added through the new drvdata API to
+avoid unnecessary collateral evolutions when adding new features.

index b784e4216868..2190a5e2ea6a 100644
@@ -5038,7 +5038,7 @@ F:	include/linux/firewire.h
 F:	include/uapi/linux/firewire*.h
 F:	tools/firewire/
 
-FIRMWARE LOADER (request_firmware)
+FIRMWARE LOADER (request_firmware, drvdata_file_request)
 M:	Ming Lei <[email protected]>
 M:	Luis R. Rodriguez <[email protected]>
 L:	[email protected]

@@ -5046,6 +5046,7 @@ S:	Maintained
 F:	Documentation/firmware_class/
 F:	drivers/base/firmware*.c
 F:	include/linux/firmware.h
+F:	include/linux/drvdata.h
 
 FLASH ADAPTER DRIVER (IBM Flash Adapter 900GB Full Height PCI Flash Card)
 M:	Joshua Morris <[email protected]>

2.10.1

This adds a load tester driver test_drvdata a for the new extensible drvdata loader APIs, part of firmware_class. Since the fallback mechanisms are currenlty completely ignored by the drvdata 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 drvdata 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.

v5:
o expand testdrv.sh to support a series of different options to help with testing
o use snprintf() using PAGE_SIZE
o add a common helper config_test_show_str()
o drop test_dev_get_name() in favor for kasprintf()
o use vzalloc() instead of vmalloc()
o bike shedding sysdata/drvdata

---
 lib/Kconfig.debug                           |   12 +
 lib/Makefile                                |    1 +
 lib/test_drvdata.c                          | 1033 +++++++++++++++++++++++++++
 tools/testing/selftests/firmware/Makefile   |    2 +-
 tools/testing/selftests/firmware/config     |    1 +
 tools/testing/selftests/firmware/drvdata.sh |  827 +++++++++++++++++++++
 6 files changed, 1875 insertions(+), 1 deletion(-)
 create mode 100644 lib/test_drvdata.c
 create mode 100755 tools/testing/selftests/firmware/drvdata.sh

index 18ffa8084eb4..5b6830dc5915 100644
@@ -1955,6 +1955,18 @@ config TEST_FIRMWARE
 
 	  If unsure, say N.
 
+config TEST_DRVDATA
+	tristate "Test driver data loading via drvdata APIs"
+	default n
+	depends on FW_LOADER
+	help
+	  This builds the "test_drvdata" module that creates a userspace
+	  interface for testing driver data loading using the drvdata API.
+	  This can be used to control the triggering of driver data loading
+	  without needing an actual real device.
+
+	  If unsure, say N.
+
 config TEST_UDELAY
 	tristate "udelay test driver"
 	default n

index d15e235f72ea..ac266f3dbe4c 100644
@@ -44,6 +44,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_DRVDATA) += test_drvdata.o
 obj-$(CONFIG_TEST_HASH) += test_hash.o
 obj-$(CONFIG_TEST_KASAN) += test_kasan.o
 obj-$(CONFIG_TEST_KSTRTOX) += test-kstrtox.o

index 9bf82234855b..7d4f6808bf1c 100644
@@ -3,7 +3,7 @@
 # No binaries, but make sure arg-less "make" doesn't trigger "run_tests"
 all:
 
-TEST_PROGS := fw_filesystem.sh fw_userhelper.sh
+TEST_PROGS := fw_filesystem.sh fw_userhelper.sh drvdata.sh
 
 include ../lib.mk
 

index c8137f70e291..0a07fccd8850 100644
@@ -1 +1,2 @@
 CONFIG_TEST_FIRMWARE=y
+CONFIG_TEST_DRVDATA=y

2.10.1

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.

v2: was not present
v5: bike shed changes: sysdata/drvdata

---
 arch/x86/kernel/cpu/microcode/amd.c | 56 +++++++++++++++++++++++++------------
 1 file changed, 38 insertions(+), 18 deletions(-)

index 6f353bdb3a25..5d08cd7284e5 100644
@@ -24,7 +24,7 @@
 #define pr_fmt(fmt) "microcode: " fmt
 
 #include <linux/earlycpio.h>
-#include <linux/firmware.h>
+#include <linux/drvdata.h>
 #include <linux/uaccess.h>
 #include <linux/vmalloc.h>
 #include <linux/initrd.h>

@@ -877,6 +877,31 @@ load_microcode_amd(int cpu, u8 family, const u8 *data, size_t size)
 	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 drvdata *drvdata)
+{
+	struct amd_ucode_req *req = context;
+
+	if (*(u32 *)drvdata->data != UCODE_MAGIC) {
+		pr_err("invalid magic value (0x%08x)\n",
+		       *(u32 *)drvdata->data);
+		req->state = UCODE_ERROR;
+		return -EINVAL;
+	}
+
+	req->state = load_microcode_amd(req->cpu, req->c->x86,
+					drvdata->data, drvdata->size);
+
+	return 0;
+}
+
 /*
  * AMD microcode firmware naming convention, up to family 15h they are in
  * the legacy file:

@@ -896,10 +921,13 @@ load_microcode_amd(int cpu, u8 family, const u8 *data, size_t size)
 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 drvdata_req_params req_params = {
+			DRVDATA_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)

@@ -908,24 +936,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 (drvdata_request((const char *)fw_name, &req_params, 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.10.1

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.

v2: was not present
v3: initial release
v4: small cosmetic fixes
v5: bike shed changes

---
 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 | 80 +++++++++++++++++++-----------
 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, 89 insertions(+), 61 deletions(-)

index d4c73d39336f..b8184cbc6770 100644
@@ -16,7 +16,7 @@
  * published by the Free Software Foundation.
  */
 
-#include <linux/firmware.h>
+#include <linux/drvdata.h>
 #include <linux/etherdevice.h>
 #include <linux/sort.h>
 #include <linux/slab.h>

index 4ac6764f4897..dc27049e4533 100644
@@ -17,7 +17,7 @@
  */
 
 #include <linux/slab.h>
-#include <linux/firmware.h>
+#include <linux/drvdata.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 drvdata *fw)
 {
 	struct p54_common *priv = dev->priv;
 	struct exp_if *exp_if;

index 9a8fedd3c0f5..4d13598d3968 100644
@@ -16,7 +16,7 @@
  * published by the Free Software Foundation.
  */
 
-#include <linux/firmware.h>
+#include <linux/drvdata.h>
 #include <linux/etherdevice.h>
 
 #include <net/mac80211.h>

index d5a3bf91a03e..a1c546cd232c 100644
@@ -17,7 +17,7 @@
  */
 
 #include <linux/slab.h>
-#include <linux/firmware.h>
+#include <linux/drvdata.h>
 #include <linux/etherdevice.h>
 #include <linux/module.h>
 

index 529939e611cd..5bbe9d77e5fc 100644
@@ -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 drvdata *fw);
 int p54_parse_eeprom(struct ieee80211_hw *dev, void *eeprom, int len);
 int p54_read_eeprom(struct ieee80211_hw *dev);
 

index 27a49068d32d..0e7fd9ba7186 100644
@@ -15,7 +15,7 @@
 
 #include <linux/pci.h>
 #include <linux/slab.h>
-#include <linux/firmware.h>
+#include <linux/drvdata.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 drvdata *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 @@ static void p54p_firmware_step2(const struct firmware *fw,
 	pci_dev_put(pdev);
 }
 
+static int p54p_load_firmware(struct p54p_priv *priv)
+{
+	const struct drvdata_req_params req_params = {
+		DRVDATA_KEEP_ASYNC(p54p_firmware_step2, priv),
+	};
+
+	return drvdata_request_async("isl3886pci", &req_params,
+				     &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);
+	drvdata_synchronize_request(priv->fw_async_cookie);
 	p54_unregister_common(dev);
-	release_firmware(priv->firmware);
+	release_drvdata(priv->firmware);
 	pci_free_consistent(pdev, sizeof(*priv->ring_control),
 			    priv->ring_control, priv->ring_control_dma);
 	iounmap(priv->map);

index 68405c142f97..00c30e1fc60b 100644
@@ -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 drvdata *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 */

index dfaa62aaeb07..c484536be900 100644
@@ -119,7 +119,7 @@ struct p54s_priv {
 	struct list_head tx_pending;
 
 	enum fw_state fw_state;
-	const struct firmware *firmware;
+	const struct drvdata *firmware;
 };
 
 #endif /* P54SPI_H */

index 043bd1c23c19..8870d177b7d5 100644
@@ -15,7 +15,7 @@
 #include <linux/usb.h>
 #include <linux/pci.h>
 #include <linux/slab.h>
-#include <linux/firmware.h>
+#include <linux/drvdata.h>
 #include <linux/etherdevice.h>
 #include <linux/delay.h>
 #include <linux/crc32.h>

@@ -916,14 +916,13 @@ static int p54u_start_ops(struct p54u_priv *priv)
 	return ret;
 }
 
-static void p54u_load_firmware_cb(const struct firmware *firmware,
+static void p54u_load_firmware_cb(const struct drvdata *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 drvdata_req_params req_params = {
+		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 = drvdata_request_async(p54u_fwlist[i].fw, &req_params,
+				    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);
+	drvdata_synchronize_request(priv->fw_async_cookie);
 	p54_unregister_common(dev);
 
 	usb_put_dev(interface_to_usbdev(intf));
-	release_firmware(priv->fw);
+	release_drvdata(priv->fw);
 	p54_free_common(dev);
 }
 

index a5f5f0fea3bd..56b58a1c41c1 100644
@@ -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 drvdata *fw;
 
 	/* asynchronous firmware callback */
-	struct completion fw_wait_load;
+	async_cookie_t fw_async_cookie;
 };
 
 #endif /* P54USB_H */

index 1af7da0b386e..fd52af4735e7 100644
@@ -17,7 +17,7 @@
  */
 
 #include <linux/export.h>
-#include <linux/firmware.h>
+#include <linux/drvdata.h>
 #include <linux/etherdevice.h>
 #include <asm/div64.h>
 

2.10.1

This v4 fixes a small typo on the p54 driver conversion after the sysdata/drvdata rename caught by 0-day. It also drops the microcode firmware conversion as we only need one driver to demo the API for now.

Once we get more bells and whistles we might see more drivers convert over, if they want, specially once we get firmware signing -- but that may take a bit more time.

The drvdata goes without any fallback mechanism as that is a royal mess right now, we *do* plan on merging support for it once we iron out a clean solution on the old API which addresses all known shortcomings. A clean fallback mechanism will then be available to both old an new the new API.

*New* features however will be encouraged using the new drvdata API moving forward. This series is intended to apply after the two documentation patch updates in the series: "[PATCH v4 0/2] firmware: fw doc revamp follow up" [0].

[0] https://lkml.kernel.org/r/[email protected]

Luis R. Rodriguez (3):
firmware: add new extensible firmware API - drvdata
test: add new drvdata loader tester
p54: convert to sysdata API

 Documentation/driver-api/firmware/drvdata.rst      |   91 ++
 Documentation/driver-api/firmware/index.rst        |    1 +
 Documentation/driver-api/firmware/introduction.rst |   11 +
 MAINTAINERS                                        |    3 +-
 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         |   80 +-
 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/drvdata.h                            |  245 +++++
 lib/Kconfig.debug                                  |   12 +
 lib/Makefile                                       |    1 +
 lib/test_drvdata.c                                 | 1033 ++++++++++++++++++++
 tools/testing/selftests/firmware/Makefile          |    2 +-
 tools/testing/selftests/firmware/config            |    1 +
 tools/testing/selftests/firmware/drvdata.sh        |  827 ++++++++++++++++
 24 files changed, 2641 insertions(+), 63 deletions(-)
 create mode 100644 Documentation/driver-api/firmware/drvdata.rst
 create mode 100644 include/linux/drvdata.h
 create mode 100644 lib/test_drvdata.c
 create mode 100755 tools/testing/selftests/firmware/drvdata.sh

2.11.0

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 and its clearly 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 (TM).

Another sticking point over the current firmware API is that some callers may need the firmware fallback mechanism when its enabled. There are two types of fallback mechanisms and both have shortcomings. This new API accepts the current status quo and ignore the fallback mechanism all together. When and if we add support for it, it will be well though out.

This new extensible firmware API enables new extensions to be added by avoiding future unnecessary collateral evolutions as this code / features get added. This new set of APIs leaves the old firmware API as-is, ignores all firmware fallback mechanism, labels the new API to reflect its broad use outside of the scope of firmware: driver data helpers, and builds on top of the original firmware core code. We purposely try to limit the scope of changes in this new API to simply enable a flexible API to start off with.

The new extensible "driver data" set of helpers accepts that there really are only two types of requests for accessing driver 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 struct drvdata_req_params to each type of request. This struct can be extended over time to support different requirements as the kernel evolves.

Using the new driver 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 old firmware fallback mechanism at all times, regardless of what kernel your driver might run in.

Developers with new uses should extend the new new struct drvdata_req_params and driver data code to provide support for new features.

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

- The firmware fallback mechanism is currenlty always ignored
- By default the kernel will free the driver data file for you after your callbacks are called, you however are allowed to request that you wish to keep the driver data file on the descriptor. The new drvdata API is able to free the drvdata file for you by requiring a consumer callback for the driver data file.
- You no longer need to declare and use your own completions, you can replace your completions with drvdata_synchronize_request() using the async_cookie set for you by drvdata_file_request_async(). When drvdata_file_request_async() completes you can rest assured all the work for both triggering, and processing the drvdata using any of your callbacks has completed.
- Allow both asynchronous and synchronous request to specify that driver 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 one of its goal was to also skip the fallback mechanisms. The driver data request APIs allow for you to annotate that a driver data file is optional for both synchronous or asynchronous requests through the same two basic set of APIs.
- The driver 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 driver data request APIs match the old asynchronous firmware API call and refcounts the caller's module.

v5 changes:

o Bike shedding:
o sysdata/drvdata
o sysdata_req_desc/drvdata_req_params
o Documentation conversion to Sphinx

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()).

---
 Documentation/driver-api/firmware/drvdata.rst      |  91 ++++++
 Documentation/driver-api/firmware/index.rst        |   1 +
 Documentation/driver-api/firmware/introduction.rst |  11 +
 MAINTAINERS                                        |   3 +-
 drivers/base/firmware_class.c                      | 327 +++++++++++++++++++++
 include/linux/drvdata.h                            | 245 +++++++++++++++
 6 files changed, 677 insertions(+), 1 deletion(-)
 create mode 100644 Documentation/driver-api/firmware/drvdata.rst
 create mode 100644 include/linux/drvdata.h

new file mode 100644
index 000000000000..0445d49cd974
@@ -0,0 +1,91 @@
+===========
+drvdata 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
+driver 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 driver data
+request set of APIs addresses rebranding of firmware as generic driver data
+files, and provides a way to enable these APIs to easily be extended without
+much collateral evolutions.
+
+Driver data modes of operation
+==============================
+
+There are only two types of modes of operation for system data requests:
+
+  * synchronous  - drvdata_request()
+  * asynchronous - drvdata_request_async()
+
+Synchronous requests expect requests to be done immediately, asynchronous
+requests enable requests to be scheduled for a later time.
+
+Driver data request parameters
+==============================
+
+Variations of types of driver data requests are specified by a driver data
+request parameter data structure. This data structure 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 "driver data file" is optional.  The driver data
+request API allows a caller to set the optional nature of the driver data
+on the request parameter data structure using the same synchronous API. Since
+this requirement is part of the request paramter data structure it also allows
+asynchronous requests to specify that the driver data 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 driver data requests. This prevents removal
+of the device and module making the driver data request call until the
+driver 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 driver 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 "driver data", consuming it after a request and finally
+freeing it. Typical asynchronous use of the old firmware APIs consist of
+the caller requesting for "driver data" and then finally freeing it on
+asynchronous callback.
+
+The driver 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 driver data around after your callbacks
+you must specify this through the driver data request paramter data structure.
+
+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 drvdata_synchronize_request() using the
+async_cookie set for you by drvdata_file_request_async(). When
+drvdata_file_request_async() completes you can rest assured all the work for
+both triggering, and processing the drvdata using any of your callbacks has
+completed.
+
+Fallback mechanisms on the driver data API
+==========================================
+
+The old firmware API provided support for a series of fallback mechanisms. The
+new driver data API abandons all current notions of the fallback mechanisms,
+it may soon add support for one though.
+
+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

index 1abe01793031..8d275c4c165b 100644
@@ -7,6 +7,7 @@ Linux Firmware API
    introduction
    core
    request_firmware
+   drvdata
 
 .. only::  subproject and html
 

index 211cb44eb972..d7d5ef846ca0 100644
@@ -25,3 +25,14 @@ are already using asynchronous initialization mechanisms which will not
 stall or delay boot. Even if loading firmware does not take a lot of time
 processing firmware might, and this can still delay boot or initialization,
 as such mechanisms such as asynchronous probe can help supplement drivers.
+
+Two APIs
+========
+
+Two APIs are provided for firmware:
+
+* request_firmware API - old firmware API
+* drvdata API - new flexible API
+
+New features and development should be added through the new drvdata API to
+avoid unnecessary collateral evolutions when adding new features.

index 5f0420a0da5b..f7752e4a2de8 100644
@@ -5028,7 +5028,7 @@ F:	include/linux/firewire.h
 F:	include/uapi/linux/firewire*.h
 F:	tools/firewire/
 
-FIRMWARE LOADER (request_firmware)
+FIRMWARE LOADER (request_firmware, drvdata_file_request)
 M:	Ming Lei <[email protected]>
 M:	Luis R. Rodriguez <[email protected]>
 L:	[email protected]

@@ -5036,6 +5036,7 @@ S:	Maintained
 F:	Documentation/firmware_class/
 F:	drivers/base/firmware*.c
 F:	include/linux/firmware.h
+F:	include/linux/drvdata.h
 
 FLASH ADAPTER DRIVER (IBM Flash Adapter 900GB Full Height PCI Flash Card)
 M:	Joshua Morris <[email protected]>

2.11.0

I think this changelog novel is longer than the documentation you added to the kernel :(

Heh, OK will work on that.

Sure thing'o.

Hm yeah it was not clear I was trying to explain *why* we ended up implementing a flexible API. Will work on that.

Will fix.

Sure.

As recent fixes for the fallback mechanism show the fallback mechanism is pretty hairy. I want to clean up all that mess, add respective test units for it. There are also some more longer term things to consider which I'd like to address as well, for instance the firmware code is the only code using the general UMH lock, although it was originally added to help warn for firmware APi uses on suspend/resume the firmware cache mechanism now helps resolve the issues -- the only case which cannot take advantage of this is the custom fallback mechanism. One also needs to consider if the UMH lock or a replacement should be considered for other kernel UMH. A related effort here which can also help is a kernel functionality to help address races with block devices freeze_super() can be used to queue superblock filesystem operations. Getting this resolved (as discussed at kernel summit by Jiri) could help resolve some of the corner case suspend/resume race concerns for to help replace the lock for the old API (custom fallback mechanism).

There are *real* more practical races on init which we will be able to soon fix with the new firmwared effort by Daniel Wagner and Tom Gunderson. Once most of this is lined up I'd feel much more comfortable with a fallback mechanism.

I really am trying hard to make that fallback mechanism *work* fine, right now its just hairballs. More patches to follow up to help with that..

Will fix.

Luis

On Thu 12 Jan 07:02 PST 2017, Luis R. Rodriguez wrote:
[..]

I think its important to strive towards this as we get to fix up the old stuff as much as possible that way. As you could tell from the surge of fixes on the fallback mechanism it is rather fragile and has not had much love. With time some of us have been giving it some love, just recently we added test interfaces for it. We need to address some few more cobwebs before being 100% certain the precise old interface is the ideal solution for a fallback mechanism. I haven't personally found any roadblocks from keeping it compatible on the newer API.

We need flexibility, that's where the new API comes from, the old API will remain but new features should go in through the new API.

The idea is to keep the old API as-is, users requiring new features would use the newer API. That's all.

Luis

This adds a load tester driver test_drvdata a for the new extensible drvdata loader APIs, part of firmware_class. Since the fallback mechanisms are currenlty completely ignored by the drvdata 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 drvdata 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.

v5:
o 0-day build complaint lock fix on lib/test_drvdata.c:9991
o expand testdrv.sh to support a series of different options to help with testing
o use snprintf() using PAGE_SIZE
o add a common helper config_test_show_str()
o drop test_dev_get_name() in favor for kasprintf()
o use vzalloc() instead of vmalloc()
o bike shedding sysdata/drvdata

---
 lib/Kconfig.debug                           |   12 +
 lib/Makefile                                |    1 +
 lib/test_drvdata.c                          | 1033 +++++++++++++++++++++++++++
 tools/testing/selftests/firmware/Makefile   |    2 +-
 tools/testing/selftests/firmware/config     |    1 +
 tools/testing/selftests/firmware/drvdata.sh |  827 +++++++++++++++++++++
 6 files changed, 1875 insertions(+), 1 deletion(-)
 create mode 100644 lib/test_drvdata.c
 create mode 100755 tools/testing/selftests/firmware/drvdata.sh

index b06848a104e6..88d807eb1efe 100644
@@ -1930,6 +1930,18 @@ config TEST_FIRMWARE
 
 	  If unsure, say N.
 
+config TEST_DRVDATA
+	tristate "Test driver data loading via drvdata APIs"
+	default n
+	depends on FW_LOADER
+	help
+	  This builds the "test_drvdata" module that creates a userspace
+	  interface for testing driver data loading using the drvdata API.
+	  This can be used to control the triggering of driver data loading
+	  without needing an actual real device.
+
+	  If unsure, say N.
+
 config TEST_UDELAY
 	tristate "udelay test driver"
 	default n

index bc4073a8cd08..d456f59e6ae0 100644
@@ -45,6 +45,7 @@ obj-y += kstrtox.o
 obj-$(CONFIG_TEST_BPF) += test_bpf.o
 obj-$(CONFIG_TEST_FIRMWARE) += test_firmware.o
 obj-$(CONFIG_TEST_HASH) += test_hash.o
+obj-$(CONFIG_TEST_DRVDATA) += test_drvdata.o
 obj-$(CONFIG_TEST_KASAN) += test_kasan.o
 obj-$(CONFIG_TEST_KSTRTOX) += test-kstrtox.o
 obj-$(CONFIG_TEST_LKM) += test_module.o

index 9bf82234855b..7d4f6808bf1c 100644
@@ -3,7 +3,7 @@
 # No binaries, but make sure arg-less "make" doesn't trigger "run_tests"
 all:
 
-TEST_PROGS := fw_filesystem.sh fw_userhelper.sh
+TEST_PROGS := fw_filesystem.sh fw_userhelper.sh drvdata.sh
 
 include ../lib.mk
 

index c8137f70e291..0a07fccd8850 100644
@@ -1 +1,2 @@
 CONFIG_TEST_FIRMWARE=y
+CONFIG_TEST_DRVDATA=y

2.11.0

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.

v2: was not present
v3: initial release
v4: small cosmetic fixes
v5: bike shed changes
v6: forgot to change one piece of code during the bikeshed name change

---
 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 | 80 +++++++++++++++++++-----------
 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, 89 insertions(+), 61 deletions(-)

index d4c73d39336f..b8184cbc6770 100644
@@ -16,7 +16,7 @@
  * published by the Free Software Foundation.
  */
 
-#include <linux/firmware.h>
+#include <linux/drvdata.h>
 #include <linux/etherdevice.h>
 #include <linux/sort.h>
 #include <linux/slab.h>

index 4ac6764f4897..dc27049e4533 100644
@@ -17,7 +17,7 @@
  */
 
 #include <linux/slab.h>
-#include <linux/firmware.h>
+#include <linux/drvdata.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 drvdata *fw)
 {
 	struct p54_common *priv = dev->priv;
 	struct exp_if *exp_if;

index 9a8fedd3c0f5..4d13598d3968 100644
@@ -16,7 +16,7 @@
  * published by the Free Software Foundation.
  */
 
-#include <linux/firmware.h>
+#include <linux/drvdata.h>
 #include <linux/etherdevice.h>
 
 #include <net/mac80211.h>

index d5a3bf91a03e..a1c546cd232c 100644
@@ -17,7 +17,7 @@
  */
 
 #include <linux/slab.h>
-#include <linux/firmware.h>
+#include <linux/drvdata.h>
 #include <linux/etherdevice.h>
 #include <linux/module.h>
 

index 529939e611cd..5bbe9d77e5fc 100644
@@ -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 drvdata *fw);
 int p54_parse_eeprom(struct ieee80211_hw *dev, void *eeprom, int len);
 int p54_read_eeprom(struct ieee80211_hw *dev);
 

index 27a49068d32d..0e7fd9ba7186 100644
@@ -15,7 +15,7 @@
 
 #include <linux/pci.h>
 #include <linux/slab.h>
-#include <linux/firmware.h>
+#include <linux/drvdata.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 drvdata *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 @@ static void p54p_firmware_step2(const struct firmware *fw,
 	pci_dev_put(pdev);
 }
 
+static int p54p_load_firmware(struct p54p_priv *priv)
+{
+	const struct drvdata_req_params req_params = {
+		DRVDATA_KEEP_ASYNC(p54p_firmware_step2, priv),
+	};
+
+	return drvdata_request_async("isl3886pci", &req_params,
+				     &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);
+	drvdata_synchronize_request(priv->fw_async_cookie);
 	p54_unregister_common(dev);
-	release_firmware(priv->firmware);
+	release_drvdata(priv->firmware);
 	pci_free_consistent(pdev, sizeof(*priv->ring_control),
 			    priv->ring_control, priv->ring_control_dma);
 	iounmap(priv->map);

index 68405c142f97..00c30e1fc60b 100644
@@ -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 drvdata *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 */

index dfaa62aaeb07..c484536be900 100644
@@ -119,7 +119,7 @@ struct p54s_priv {
 	struct list_head tx_pending;
 
 	enum fw_state fw_state;
-	const struct firmware *firmware;
+	const struct drvdata *firmware;
 };
 
 #endif /* P54SPI_H */

index 043bd1c23c19..f698c4acfb49 100644
@@ -15,7 +15,7 @@
 #include <linux/usb.h>
 #include <linux/pci.h>
 #include <linux/slab.h>
-#include <linux/firmware.h>
+#include <linux/drvdata.h>
 #include <linux/etherdevice.h>
 #include <linux/delay.h>
 #include <linux/crc32.h>

@@ -916,14 +916,13 @@ static int p54u_start_ops(struct p54u_priv *priv)
 	return ret;
 }
 
-static void p54u_load_firmware_cb(const struct firmware *firmware,
+static void p54u_load_firmware_cb(const struct drvdata *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 drvdata_req_params req_params = {
+		DRVDATA_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 = drvdata_request_async(p54u_fwlist[i].fw, &req_params,
+				    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);
+	drvdata_synchronize_request(priv->fw_async_cookie);
 	p54_unregister_common(dev);
 
 	usb_put_dev(interface_to_usbdev(intf));
-	release_firmware(priv->fw);
+	release_drvdata(priv->fw);
 	p54_free_common(dev);
 }
 

index a5f5f0fea3bd..56b58a1c41c1 100644
@@ -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 drvdata *fw;
 
 	/* asynchronous firmware callback */
-	struct completion fw_wait_load;
+	async_cookie_t fw_async_cookie;
 };
 
 #endif /* P54USB_H */

index 1af7da0b386e..fd52af4735e7 100644
@@ -17,7 +17,7 @@
  */
 
 #include <linux/export.h>
-#include <linux/firmware.h>
+#include <linux/drvdata.h>
 #include <linux/etherdevice.h>
 #include <asm/div64.h>
 

2.11.0

I've also tested it with the ISL3887USB and a ISL3880PCI. I hope the p54spi will work as well. But I don't have the HW to test it.

Regards,
Christian

What is this tag for?

Also, meta-comment, put your vN: lines below the --- line like the kernel documentation says to do.

why does the "new" api require more lines?

Every no wand then some tool scrapes for commit logs to see if Coccinelle was used to help with a kernel commit. There are different heuristics, this tag is to help make a more unique search more easily identifiable as I used Coccinelle to do the original port.

Oh, OK sounds good.

This is a bare bones flexible API with only a few new tiny features to start with, one of them was to enable the API do the freeing of the driver data for you. In the kernel we have devres to help with this but devres only helps if you would use the API call on probe. We want to support the ability to let the API free the driver data for you even if your call is outside of probe, for this to work we need a callback. For async calls this is rather trivial given we already have a callback, for sync calls this means a new routine is needed. Freeing the data for you is an option, but I decided to keep the callback requirement even if you didn't want the free'ing to be done for you. The addition of a callback is what accounts for the slight increase on this driver.

I could try avoiding the callback if no freeing is needed.

Great point. The API already has:

static int _drvdata_request(const struct drvdata **drvdata_p,
const char *name,
const struct drvdata_req_params *params, struct device *device)
{
...

       if (ret && !params->optional)
               pr_err("Direct driver data load for %s failed with error %d\n",
                      name, ret);

...
}

So it is not needed for driver to moan about failures here.

Luis

OK I've added a respective helper call which would map 1-1 with the old sync mechanism to enable a 1-1 change, this will be called driver_data_request_simple(), but let me know if there is a preference for something else.

With this the only visible delta now is from taking advantage of new features. In p54's case this would re-organize the mess in drivers/net/wireless/intersil/p54/p54spi.c, the diff stat is a bit larger for that file just because of this but I think in this case its very much worth the small additions. In this case two routines are added for handling the work through callbacks on a sync call.

 1 file changed, 38 insertions(+), 30 deletions(-)

index 7ab2f43ab425..6183a8bfa149 100644
@@ -23,7 +23,7 @@
 #include <linux/module.h>
 #include <linux/platform_device.h>
 #include <linux/interrupt.h>
-#include <linux/firmware.h>
+#include <linux/driver_data.h>
 #include <linux/delay.h>
 #include <linux/irq.h>
 #include <linux/spi/spi.h>

@@ -168,47 +168,55 @@ static int p54spi_request_firmware(struct ieee80211_hw *dev)
 	int ret;
 
 	/* FIXME: should driver use it's own struct device? */
-	ret = request_firmware(&priv->firmware, "3826.arm", &priv->spi->dev);
-
-	if (ret < 0) {
-		dev_err(&priv->spi->dev, "request_firmware() failed: %d", ret);
+	ret = driver_data_request_simple("3826.arm", &priv->spi->dev,
+					 &priv->firmware);
+	if (ret < 0)
 		return ret;
-	}
 
 	ret = p54_parse_firmware(dev, priv->firmware);
 	if (ret) {
-		release_firmware(priv->firmware);
+		release_driver_data(priv->firmware);
 		return ret;
 	}
-
 	return 0;
 }
 
-static int p54spi_request_eeprom(struct ieee80211_hw *dev)
+#ifdef CONFIG_P54_SPI_DEFAULT_EEPROM
+static int p54spi_load_eeprom_default(void *context)
 {
-	struct p54s_priv *priv = dev->priv;
-	const struct firmware *eeprom;
-	int ret;
+	struct p54s_priv *priv = context;
+	struct ieee80211_hw *dev = priv->hw;
 
-	/* allow users to customize their eeprom.
-	 */
+	dev_info(&priv->spi->dev, "loading default eeprom...\n");
+	return p54_parse_eeprom(dev, (void *) p54spi_eeprom,
+				sizeof(p54spi_eeprom));
+}
+#endif
 
-	ret = request_firmware_direct(&eeprom, "3826.eeprom", &priv->spi->dev);
-	if (ret < 0) {
+static int p54spi_load_eeprom_cb(void *context,
+				 const struct driver_data *driver_data)
+{
+	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 *) driver_data->data,
+				(int)driver_data->size);
+}
+static int p54spi_request_eeprom(struct ieee80211_hw *dev)
+{
+	struct p54s_priv *priv = dev->priv;
+	const struct driver_data_req_params req_params = {
+		DRIVER_DATA_DEFAULT_SYNC(p54spi_load_eeprom_cb, priv),
 #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;
+		DRIVER_DATA_SYNC_OPT_CB(p54spi_load_eeprom_default, priv),
+#endif
+	};
+	/*
+	 * allow users to customize their eeprom.
+	 */
+	return driver_data_request("3826.eeprom", &req_params,
+				   &priv->spi->dev);
 }
 
 static int p54spi_upload_firmware(struct ieee80211_hw *dev)

@@ -692,7 +700,7 @@ static int p54spi_remove(struct spi_device *spi)
 
 	gpio_free(p54spi_gpio_power);
 	gpio_free(p54spi_gpio_irq);
-	release_firmware(priv->firmware);
+	release_driver_data(priv->firmware);
 
 	mutex_destroy(&priv->mutex);
 

So just looking at this patch, what's the *advantage* to the driver writer?

Apart from the actual new feature, this patch seems to actively make the driver uglier.

I mentioned this before, but replacing "request_firmware()" with "driver_data_request_simple()" is SIMPLY NOT AN IMPROVEMENT.

The new name is longer and _less_ descriptive.

So I'm really not seeing why you want to make these conversions that just make code worse.

Linus

So for the driver writer it provides a clean way to logically split up what is to be done for certain situations if the firmware is not present or is present. Without this the code is a bit unruly, and this is actually a mild case. There are much crazier chained conditionals (iwlwifi is one that has a long chain of firmwares) but a goal here was to just provide only the most basic bump in logic so that further enhancements/functionality is added later.

I strongly agree with this.

The real goal here was first to actually provide a flexible API to enable more advanced features to be added without having to affect existing callers, as has been done before. So hence the const struct driver_data_req_params approach and only two basic calls -- a sync and async call. This was after long ago we had revised how we would go about adding firmware signing support to the kernel.

My first approach in addressing firmware signing was to mimic how we handle have module signing: everyone gets it (even those on the old API), using one default key. The flexible API was then a secondary step, to enable users to customize signature requirements. As we discussed things it was clear that we wanted the ability to support firmware signing with the ability to provide alternative key requirements from the very start. Having an extensible firmware API in place first would enable the flexibility to let us decide what requirements we want to put in place for firmware signing without concern for making a slew of collateral evolutions as requirements change.

The flexible API then would be, as is in this series, completely optional. Only if you want to reap benefit of some of the new features would you use it.

So unless the flexible API is reproachable in and of itself perhaps the thing to do is leave all drivers as-is (without no conversion) and only convert once we have a full gain value-add. For instance later adding support to easily chain a series of firmware requests (not just 2), or once we have firmware signing support and a driver want to reap benefit from it.

Thoughts ?

Luis

So I've said this before, and I'll say this one more time:

It's fine if we make the internal implementation of some generic "load data from the filesystem or user" be this kind of new flexible API that is internally called "driver_data_request()" or whatever.

But dammit, that is NOT AN EXCUSE for then making crap patches that just replace the existing firmware users.

If the new interface cannot be wrapped in the old names (and the old semantics) the new interface is shit and should never ever go anywhere.

So leave the existing users alone. Concentrate on _only_ the parts where there is actual and real need of new features. Don't try to rename or extend current drivers. Don't send out these patches that make drivers actively uglier. Really.

I really question the whole thing when there are things like this going on.

Linus

Works with me.

There's a few questionable things part of the old API which (UMH lock is one used even if no UMH is used, the fallback mechanism another) so I've taken out what I can truly vouch for and its all being shared on the driver_data API. Extending the old API with yet-more flags is a big concern on my part so will also recommend new functionality to be focused on the newer API.

Right on.

Luis

I agree with Linus, as well as, look, it's still bigger, so you are making driver developers do more work :(

Maybe we could try cleaning up existing firmware API and see if we really hit something that can't be solved in any sane way? What do you think?

I'd love to help with that, I started with a trivial cleaning patch:
[PATCH V2] firmware: simplify defining and handling FW_OPT_FALLBACK
https://patchwork.kernel.org/patch/9469875/

It didn't receive any real negative comments but I also have no idea how could pick it up for me and send in some pull request. Any suggestions?

Rafał

Does that patch really "simplify" anything? Anyway, resend it if the maintainer of the subsystem ignores it (you did cc: the correct people, right?)

And yes, I always like seeing things that simplify apis, and the firmware interface could really use that, which I thought is what Luis was trying to do here, which is why I keep pushing back on this patchset as it doesn't seem to be happening.

thanks,

greg k-h

According to the MAINTAINERS there isn't firmware API tree / maintainer. Also this is just a cleanup so I don't know if I should expect some random maintainer (e.g. wireless tree) to pick it.

Rafał

I don't think you looked very hard:

 $ ./scripts/get_maintainer.pl --file drivers/base/firmware_class.c
 Ming Lei <[email protected]> (maintainer:FIRMWARE LOADER (request_firmware))
 "Luis R. Rodriguez" <[email protected]> (maintainer:FIRMWARE LOADER (request_firmware))
 Greg Kroah-Hartman <[email protected]> (supporter:DRIVER CORE, KOBJECTS, DEBUGFS, KERNFS AND SYSFS)
 [email protected] (open list:FIRMWARE LOADER (request_firmware))

Please try again...

greg k-h

My memory totally failed me :( Sorry

Rafał

I was eventually copied on the patch but there was also a side discussion on you wanting FW_OPT_NO_WARN. Can you review if the proposed driver_data API does what I think you wanted. Although request_firmware_direct() only avoids warning on a sync all the diver_data API also supports this for async calls. If its something else I'd prefer we evaluate extending the driver_data API before loosely adding yet another API call using the old API.

Luis

Greg,

This v5 drops the driver porting examples in light of the fact simple cases do not yet benefit from use of the newer API. The API is purposely introduced bare bones so new features are easier to review.

This v5 embraces the proposed "driver data" name, extends documentation to cover the driver data API stress test driver, and simplifies documentation a bit more as requested. I've taken the time to also review all checkpatch annoyances.

On the v4 series Linus clarified he's of the position the driver data API could be a generic API for a generic "load data from the filesystem or user" requests and that it should be possible to wrap the new interface in the old firmware API. There's a few questionable things part of the old firmware API which upon review I could not stomache in keeping though as either they are very likely not needed anymore or require more thought to properly implement. As such the new API only carries on what is sensible from the old API. The questionable things from the old API are currently not carried forward are:

o The usermode helper lock: is currently used on the older firmware API even if the kernel UMH is not used. The UMH lock was added to help avoid a suspend and resume face issue, which the firmware cache fixed.

o The fallback mechanism: the fallback mechanism is rather complex and supports a custom fallback mechanism. As recent patches reveal this was not well tested for a while. We also know systemd udev has long had the fallback mechanism ripped out, and some distributions simply ignore these requestions all together (Debian). A few new users of the fallback mechanism have come to light however and upon review it would seem we can address support for these users using the default fallback mechanism with kobject uevents even if a custom path is required, and a new userspace solution which replaces the old systemd udev fallback helper. Long term then the custom fallback mechanism proves pointless to extend.

Lastly, we now have the new common kernel_read_file_from_fd() and kernel_read_file_from_path() with respective LSM hooks and identifiers for LSMs for reading files from the kernel for:

o READING_UNKNOWN
o READING_FIRMWARE
o READING_FIRMWARE_PREALLOC_BUFFER
o READING_MODULE
o READING_KEXEC_IMAGE
o READING_KEXEC_INITRAMFS
o READING_POLICY

If we want to support a fallback mechanism and/or custom paths on the driver data API to generalize it more we'd need to revise how we'd want to expand on context to feed LSMs. For now though the driver data API only supports and uses kernel_read_file_from_path() and uses the READING_FIRMWARE id only as this is where the code grew from.

Luis R. Rodriguez (2):
firmware: add extensible driver data API
test: add new driver_data load tester

 Documentation/driver-api/firmware/driver_data.rst  |  109 ++
 Documentation/driver-api/firmware/index.rst        |    1 +
 Documentation/driver-api/firmware/introduction.rst |   16 +
 MAINTAINERS                                        |    4 +-
 drivers/base/firmware_class.c                      |  333 +++++++
 include/linux/driver_data.h                        |  253 +++++
 lib/Kconfig.debug                                  |   12 +
 lib/Makefile                                       |    1 +
 lib/test_driver_data.c                             | 1039 ++++++++++++++++++++
 tools/testing/selftests/firmware/Makefile          |    2 +-
 tools/testing/selftests/firmware/config            |    1 +
 tools/testing/selftests/firmware/driver_data.sh    |  826 ++++++++++++++++
 12 files changed, 2595 insertions(+), 2 deletions(-)
 create mode 100644 Documentation/driver-api/firmware/driver_data.rst
 create mode 100644 include/linux/driver_data.h
 create mode 100644 lib/test_driver_data.c
 create mode 100755 tools/testing/selftests/firmware/driver_data.sh

2.11.0

The firmware API does not scale well: when new features are added we either add a new exported symbol or extend the arguments of existing routines. For the later case this 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". There are other subsystems which would like to make use of the firmware APIs for similar things and its clearly not firmware, but have different requirements and criteria which they'd like to be met for the requested file.

An extensible API is in order. While reviewing the existing API the firmware fallback mechansism stands out as requiring more review before folding forward as such it has been kept out of the new API.

We purposely try to limit the scope of changes in this new API to enable a flexible API to start off with as base. The driver data API accepts that there are only two types of requests:

a) synchronous requests
b) asynchronous requests

Both requests may have a different requirements which must be met. These requirements can be described in the struct driver_data_req_params. This struct is expected to be extended over time to support different requirements as the kernel evolves. Other than providing a flexible API a few minor features are are introduced:

- By default the kernel will free the driver data file for you after your callbacks are called, you however are allowed to request that you wish to keep the driver data file on the requirements params. The new driver data API is able to free the driver data file for you by requiring a consumer callback for the driver data file.
- Allows both asynchronous and synchronous request to specify that driver data files are optional. With the old APIs we had added one full API call, request_firmware_direct() just for this purpose -- the driver data request APIs allow for you to annotate that a driver data file is optional for both synchronous or asynchronous requests through the same two basic set of APIs.
- You no longer need to declare and use your own completions, you can replace your completions with driver_data_synchronize_request() using the async_cookie given back by the async driver data call.

---
 Documentation/driver-api/firmware/driver_data.rst  |  77 +++++
 Documentation/driver-api/firmware/index.rst        |   1 +
 Documentation/driver-api/firmware/introduction.rst |  16 +
 MAINTAINERS                                        |   3 +-
 drivers/base/firmware_class.c                      | 333 +++++++++++++++++++++
 include/linux/driver_data.h                        | 253 ++++++++++++++++
 6 files changed, 682 insertions(+), 1 deletion(-)
 create mode 100644 Documentation/driver-api/firmware/driver_data.rst
 create mode 100644 include/linux/driver_data.h

new file mode 100644
index 000000000000..08407b7568fe
@@ -0,0 +1,77 @@
+===============
+driver_data API
+===============
+
+Users of firmware request APIs has grown to include users which are not
+looking for "firmware", but instead general driver data files which have
+been kept oustide of the kernel. The driver data APIs addresses rebranding
+of firmware as generic driver data files, and provides a flexible API which
+mitigates collateral evolutions on the kernel as new functionality is
+introduced.
+
+Driver data modes of operation
+==============================
+
+There are only two types of modes of operation for driver data requests:
+
+  * synchronous  - driver_data_request()
+  * asynchronous - driver_data_request_async()
+
+Synchronous requests expect requests to be done immediately, asynchronous
+requests enable requests to be scheduled for a later time.
+
+Driver data request parameters
+==============================
+
+Variations of types of driver data requests are specified by a driver data
+request parameter data structure. This data structure is expected to grow as
+new requirements grow.
+
+Reference counting and releasing the driver data file
+=====================================================
+
+As with the old firmware API both the device and module are bumped with
+reference counts during the driver data requests. This prevents removal
+of the device and module making the driver data request call until the
+driver 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 driver data request API currently mimics this behaviour, for synchronous
+requests the firmware_class module is refcounted through the use of
+dfl_sync_reqs. In the future we may enable the ability to also refcount the
+caller's module as well. Likewise in the future we may enable asynchronous
+calls to refcount the firmware_class module.
+
+Typical use of the old synchronous firmware APIs consist of the caller
+requesting for "driver data", consuming it after a request and finally
+freeing it. Typical asynchronous use of the old firmware APIs consist of
+the caller requesting for "driver data" and then finally freeing it on
+asynchronous callback.
+
+The driver 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 driver data around after your callbacks
+you must specify this through the driver data request parameter data structure.
+
+Synchronizing with async cookies
+================================
+
+The driver data API relies on async cookies to enable users to synchronize
+for any pending async work. The async cookie obtained through an async
+call using driver_data_file_request_async() can be used to synchronize and
+wait for pending work with driver_data_synchronize_request().
+
+When driver_data_file_request_async() completes you can rest assured all the
+work for both triggering, and processing the driver data using any of your
+callbacks has completed.
+
+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

index 1abe01793031..c2be92e2628c 100644
@@ -7,6 +7,7 @@ Linux Firmware API
    introduction
    core
    request_firmware
+   driver_data
 
 .. only::  subproject and html
 

index 211cb44eb972..a0f6a3fa1d5d 100644
@@ -25,3 +25,19 @@ are already using asynchronous initialization mechanisms which will not
 stall or delay boot. Even if loading firmware does not take a lot of time
 processing firmware might, and this can still delay boot or initialization,
 as such mechanisms such as asynchronous probe can help supplement drivers.
+
+Two APIs
+========
+
+Two APIs are provided for firmware:
+
+* request_firmware API - old firmware API
+* driver_data API - flexible API
+
+We have historically extended the firmware API by adding new routines or at
+times extending 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 driver data API is an extensible API enabling extensions to
+be added by avoiding unnecessary collateral evolutions as features get added.
+New features and development should be added through the driver_data API.

index 2c171ad9ea07..f1dd84bb21ed 100644
@@ -5065,7 +5065,7 @@ F:	include/linux/firewire.h
 F:	include/uapi/linux/firewire*.h
 F:	tools/firewire/
 
-FIRMWARE LOADER (request_firmware)
+FIRMWARE LOADER (request_firmware, driver_data_request)
 M:	Ming Lei <[email protected]>
 M:	Luis R. Rodriguez <[email protected]>
 L:	[email protected]

@@ -5073,6 +5073,7 @@ S:	Maintained
 F:	Documentation/firmware_class/
 F:	drivers/base/firmware*.c
 F:	include/linux/firmware.h
+F:	include/linux/driver_data.h
 
 FLASH ADAPTER DRIVER (IBM Flash Adapter 900GB Full Height PCI Flash Card)
 M:	Joshua Morris <[email protected]>

2.11.0

This adds a load tester driver test_driver_data a for the new extensible driver_data loader API, part of firmware_class. This test driver enables you to build your tests in userspace by exposing knobs of the exported API to userspace and enables a trigger action to mimic a one time use of the kernel API. This gives us the flexibility to build test case from userspace with less kernel changes.

---
 Documentation/driver-api/firmware/driver_data.rst |   32 +
 MAINTAINERS                                       |    1 +
 lib/Kconfig.debug                                 |   12 +
 lib/Makefile                                      |    1 +
 lib/test_driver_data.c                            | 1039 +++++++++++++++++++++
 tools/testing/selftests/firmware/Makefile         |    2 +-
 tools/testing/selftests/firmware/config           |    1 +
 tools/testing/selftests/firmware/driver_data.sh   |  826 ++++++++++++++++
 8 files changed, 1913 insertions(+), 1 deletion(-)
 create mode 100644 lib/test_driver_data.c
 create mode 100755 tools/testing/selftests/firmware/driver_data.sh

index 08407b7568fe..757c2ffa4ba6 100644
@@ -68,6 +68,38 @@ When driver_data_file_request_async() completes you can rest assured all the
 work for both triggering, and processing the driver data using any of your
 callbacks has completed.
 
+Testing the driver_data API
+===========================
+
+The driver data API has a selftest driver: lib/test_driver_data.c. The
+test_driver_data enables you to build your tests in userspace by exposing knobs
+of the exported API in userspace and enabling userspace to configure and
+trigger a kernel call. This lets us build most possible test cases of
+the kernel APIs from userspace.
+
+The test_driver_data also enables multiple test triggers to be created
+enabling testing to be done in parallel, one test interface per test case.
+
+To test an async call one could do::
+
+        echo anything > /lib/firmware/test-driver_data.bin
+        echo -n 1 >  /sys/devices/virtual/misc/test_driver_data0/config_async
+        echo -n 1 >  /sys/devices/virtual/misc/test_driver_data0/trigger_config
+
+A series of tests have been written to test the driver data API thoroughly.
+A respective test case is expected to bet written as new features get added.
+For details of existing tests run::
+
+        tools/testing/selftests/firmware/driver_data.sh -l
+
+To see all available options::
+
+        tools/testing/selftests/firmware/driver_data.sh --help
+
+To run a test 0010 case 40 times::
+
+        tools/testing/selftests/firmware/driver_data.sh -c 0010 40
+
 Tracking development enhancements and ideas
 ===========================================
 

index f1dd84bb21ed..96449804ec9d 100644
@@ -5072,6 +5072,7 @@ L:	[email protected]
 S:	Maintained
 F:	Documentation/firmware_class/
 F:	drivers/base/firmware*.c
+F:	lib/test_driver_data.c
 F:	include/linux/firmware.h
 F:	include/linux/driver_data.h
 

index 64c03b07ad2f..68495327ca49 100644
@@ -1975,6 +1975,18 @@ config TEST_FIRMWARE
 
 	  If unsure, say N.
 
+config TEST_DRIVER_DATA
+	tristate "Test driver data loading via driver_data APIs"
+	default n
+	depends on FW_LOADER
+	help
+	  This builds the "test_driver_data" module that creates a userspace
+	  interface for testing driver data loading using the driver_data API.
+	  This can be used to control the triggering of driver data loading
+	  without needing an actual real device.
+
+	  If unsure, say N.
+
 config TEST_UDELAY
 	tristate "udelay test driver"
 	default n

index fbbe2e2dba8f..79eecd0ce6fb 100644
@@ -46,6 +46,7 @@ obj-y += kstrtox.o
 obj-$(CONFIG_TEST_BPF) += test_bpf.o
 obj-$(CONFIG_TEST_FIRMWARE) += test_firmware.o
 obj-$(CONFIG_TEST_HASH) += test_hash.o test_siphash.o
+obj-$(CONFIG_TEST_DRIVER_DATA) += test_driver_data.o
 obj-$(CONFIG_TEST_KASAN) += test_kasan.o
 obj-$(CONFIG_TEST_KSTRTOX) += test-kstrtox.o
 obj-$(CONFIG_TEST_LKM) += test_module.o

index 1894d625af2d..c9bf6c44435f 100644
@@ -3,7 +3,7 @@
 # No binaries, but make sure arg-less "make" doesn't trigger "run_tests"
 all:
 
-TEST_PROGS := fw_filesystem.sh fw_fallback.sh
+TEST_PROGS := fw_filesystem.sh fw_fallback.sh driver_data.sh
 
 include ../lib.mk
 

index c8137f70e291..0f1a299f9270 100644
@@ -1 +1,2 @@
 CONFIG_TEST_FIRMWARE=y
+CONFIG_TEST_DRIVER_DATA=y

2.11.0

I am not going to review this until you actually convert some existing code, or do something with it. A test-module isn't proof that this is a good api or not. As it is, I can't add new apis with no users.

sorry,

greg k-h