[PATCH v2 0/5] firmware: doc revamp

[PATCH v2 0/5] firmware: doc revamp

[Luis R. Rodriguez]

This v2 has the small corrections on the revamp for the
documentation for the firmware API. I'll follow this up
with the drvdata API patches.

This is intended *after* the merge window, but posting
now as we'll soon be in end of the year holidays.

Luis R. Rodriguez (5):
  selftests: firmware: only modprobe if driver is missing
  selftests: firmware: send expected errors to /dev/null
  firmware: revamp firmware documentation
  firmware: add SmPL report for custom fallback mechanism
  firmware: add DECLARE_FW_CUSTOM_FALLBACK() annotation

 Documentation/driver-api/firmware/built-in-fw.rst  |  38 ++++
 Documentation/driver-api/firmware/core.rst         |  16 ++
 .../driver-api/firmware/direct-fs-lookup.rst       |  30 +++
 .../driver-api/firmware/fallback-mechanisms.rst    | 215 +++++++++++++++++++++
 .../driver-api/firmware/firmware_cache.rst         |  51 +++++
 .../driver-api/firmware/fw_search_path.rst         |  26 +++
 Documentation/driver-api/firmware/index.rst        |  16 ++
 Documentation/driver-api/firmware/introduction.rst |  27 +++
 Documentation/driver-api/firmware/lookup-order.rst |  18 ++
 .../driver-api/firmware/request_firmware.rst       |  56 ++++++
 Documentation/driver-api/index.rst                 |   1 +
 Documentation/firmware_class/README                | 128 ------------
 drivers/firmware/dell_rbu.c                        |   1 +
 drivers/leds/leds-lp55xx-common.c                  |   1 +
 include/linux/firmware.h                           |   7 +
 .../api/request_firmware-custom-fallback.cocci     |  44 +++++
 tools/testing/selftests/firmware/fw_filesystem.sh  |  25 ++-
 17 files changed, 567 insertions(+), 133 deletions(-)
 create mode 100644 Documentation/driver-api/firmware/built-in-fw.rst
 create mode 100644 Documentation/driver-api/firmware/core.rst
 create mode 100644 Documentation/driver-api/firmware/direct-fs-lookup.rst
 create mode 100644 Documentation/driver-api/firmware/fallback-mechanisms.rst
 create mode 100644 Documentation/driver-api/firmware/firmware_cache.rst
 create mode 100644 Documentation/driver-api/firmware/fw_search_path.rst
 create mode 100644 Documentation/driver-api/firmware/index.rst
 create mode 100644 Documentation/driver-api/firmware/introduction.rst
 create mode 100644 Documentation/driver-api/firmware/lookup-order.rst
 create mode 100644 Documentation/driver-api/firmware/request_firmware.rst
 delete mode 100644 Documentation/firmware_class/README
 create mode 100644 scripts/coccinelle/api/request_firmware-custom-fallback.cocci

-- 
2.10.1

[PATCH v2 1/5] selftests: firmware: only modprobe if driver is missing

[Luis R. Rodriguez]

No need to load test_firmware if its already there.
Also use a more generic form to recommend what is required
to be built.

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

diff --git a/tools/testing/selftests/firmware/fw_filesystem.sh b/tools/testing/selftests/firmware/fw_filesystem.sh
index 5c495ad7958a..c8ccdaa78479 100755
--- a/tools/testing/selftests/firmware/fw_filesystem.sh
+++ b/tools/testing/selftests/firmware/fw_filesystem.sh
@@ -5,9 +5,24 @@
 # 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
+TEST_DIR=$(dirname $0)
+
+test_modprobe()
+{
+	if [ ! -d $DIR ]; then
+		echo "$0: $DIR not present"
+		echo "You must have the following enabled in your kernel:"
+		cat $TEST_DIR/config
+		exit 1
+	fi
+}
+
+trap "test_modprobe" EXIT
+
+if [ ! -d $DIR ]; then
+	modprobe test_firmware
+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
-- 
2.10.1

[PATCH v2 2/5] 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 c8ccdaa78479..e35691239350 100755
--- a/tools/testing/selftests/firmware/fw_filesystem.sh
+++ b/tools/testing/selftests/firmware/fw_filesystem.sh
@@ -63,18 +63,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.10.1

[PATCH v2 3/5] firmware: revamp firmware documentation

[Luis R. Rodriguez]

Understanding this code is getting out of control without any
notes. Give the firmware_class driver a much needed documentation love,
and while at it convert it to the new sphinx documentation format.

v2: typos and small fixes

Signed-off-by: Luis R. Rodriguez <mcgrof@kernel.org>
---
 Documentation/driver-api/firmware/built-in-fw.rst  |  38 ++++
 Documentation/driver-api/firmware/core.rst         |  16 ++
 .../driver-api/firmware/direct-fs-lookup.rst       |  30 ++++
 .../driver-api/firmware/fallback-mechanisms.rst    | 195 +++++++++++++++++++++
 .../driver-api/firmware/firmware_cache.rst         |  51 ++++++
 .../driver-api/firmware/fw_search_path.rst         |  26 +++
 Documentation/driver-api/firmware/index.rst        |  16 ++
 Documentation/driver-api/firmware/introduction.rst |  27 +++
 Documentation/driver-api/firmware/lookup-order.rst |  18 ++
 .../driver-api/firmware/request_firmware.rst       |  56 ++++++
 Documentation/driver-api/index.rst                 |   1 +
 Documentation/firmware_class/README                | 128 --------------
 12 files changed, 474 insertions(+), 128 deletions(-)
 create mode 100644 Documentation/driver-api/firmware/built-in-fw.rst
 create mode 100644 Documentation/driver-api/firmware/core.rst
 create mode 100644 Documentation/driver-api/firmware/direct-fs-lookup.rst
 create mode 100644 Documentation/driver-api/firmware/fallback-mechanisms.rst
 create mode 100644 Documentation/driver-api/firmware/firmware_cache.rst
 create mode 100644 Documentation/driver-api/firmware/fw_search_path.rst
 create mode 100644 Documentation/driver-api/firmware/index.rst
 create mode 100644 Documentation/driver-api/firmware/introduction.rst
 create mode 100644 Documentation/driver-api/firmware/lookup-order.rst
 create mode 100644 Documentation/driver-api/firmware/request_firmware.rst
 delete mode 100644 Documentation/firmware_class/README

diff --git a/Documentation/driver-api/firmware/built-in-fw.rst b/Documentation/driver-api/firmware/built-in-fw.rst
new file mode 100644
index 000000000000..7300e66857f8
--- /dev/null
+++ b/Documentation/driver-api/firmware/built-in-fw.rst
@@ -0,0 +1,38 @@
+=================
+Built-in firmware
+=================
+
+Firmware can be built-in to the kernel, this means building the firmware
+into vmlinux directly, to enable avoiding having to look for firmware from
+the filesystem. Instead, firmware can be looked for inside the kernel
+directly. You can enable built-in firmware using the kernel configuration
+options:
+
+  * CONFIG_EXTRA_FIRMWARE
+  * CONFIG_EXTRA_FIRMWARE_DIR
+
+This should not be confused with CONFIG_FIRMWARE_IN_KERNEL, this is for drivers
+which enables firmware to be built as part of the kernel build process. This
+option, CONFIG_FIRMWARE_IN_KERNEL, will build all firmware for all drivers
+enabled which ship its firmware inside the Linux kernel source tree.
+
+There are a few reasons why you might want to consider building your firmware
+into the kernel with CONFIG_EXTRA_FIRMWARE though:
+
+* Speed
+* Firmware is needed for accessing the boot device, and the user doesn't
+  want to stuff the firmware into the boot initramfs.
+
+Even if you have these needs there are a few reasons why you may not be
+able to make use of built-in firmware:
+
+* Legalese - firmware is non-GPL compatible
+* Some firmware may be optional
+* Firmware upgrades are possible, therefore a new firmware would implicate
+  a complete kernel rebuild.
+* Some firmware files may be really large in size. The remote-proc subsystem
+  is an example subsystem which deals with these sorts of firmware
+* The firmware may need to be scraped out from some device specific location
+  dynamically, an example is calibration data for for some WiFi chipsets. This
+  calibration data can be unique per sold device.
+
diff --git a/Documentation/driver-api/firmware/core.rst b/Documentation/driver-api/firmware/core.rst
new file mode 100644
index 000000000000..1d1688cbc078
--- /dev/null
+++ b/Documentation/driver-api/firmware/core.rst
@@ -0,0 +1,16 @@
+==========================
+Firmware API core features
+==========================
+
+The firmware API has a rich set of core features available. This section
+documents these features.
+
+.. toctree::
+
+   fw_search_path
+   built-in-fw
+   firmware_cache
+   direct-fs-lookup
+   fallback-mechanisms
+   lookup-order
+
diff --git a/Documentation/driver-api/firmware/direct-fs-lookup.rst b/Documentation/driver-api/firmware/direct-fs-lookup.rst
new file mode 100644
index 000000000000..82b4d585a213
--- /dev/null
+++ b/Documentation/driver-api/firmware/direct-fs-lookup.rst
@@ -0,0 +1,30 @@
+========================
+Direct filesystem lookup
+========================
+
+Direct filesystem lookup is the most common form of firmware lookup performed
+by the kernel. The kernel looks for the firmware directly on the root
+filesystem in the paths documented in the section 'Firmware search paths'.
+The filesystem lookup is implemented in fw_get_filesystem_firmware(), it
+uses common core kernel file loader facility kernel_read_file_from_path().
+The max path allowed is PATH_MAX -- currently this is 4096 characters.
+
+It is recommended you keep /lib/firmware paths on your root filesystem,
+avoid having a separate partition for them in order to avoid possible
+races with lookups and avoid uses of the custom fallback mechanisms
+documented below.
+
+Firmware and initramfs
+----------------------
+
+Drivers which are built-in to the kernel should have the firmware integrated
+also as part of the initramfs used to boot the kernel given that otherwise
+a race is possible with loading the driver and the real rootfs not yet being
+available. Stuffing the firmware into initramfs resolves this race issue,
+however note that using initrd does not suffice to address the same race.
+
+There are circumstances that justify not wanting to include firmware into
+initramfs, such as dealing with large firmware firmware files for the
+remote-proc subsystem. For such cases using a userspace fallback mechanism
+is currently the only viable solution as only userspace can know for sure
+when the real rootfs is ready and mounted.
diff --git a/Documentation/driver-api/firmware/fallback-mechanisms.rst b/Documentation/driver-api/firmware/fallback-mechanisms.rst
new file mode 100644
index 000000000000..d19354794e67
--- /dev/null
+++ b/Documentation/driver-api/firmware/fallback-mechanisms.rst
@@ -0,0 +1,195 @@
+===================
+Fallback mechanisms
+===================
+
+A fallback mechanism is supported to allow to overcome failures to do a direct
+filesystem lookup on the root filesystem or when the firmware simply cannot be
+installed for practical reasons on the root filesystem. The kernel
+configuration options related to supporting the firmware fallback mechanism are:
+
+  * CONFIG_FW_LOADER_USER_HELPER: enables building the firmware fallback
+    mechanism. Most distributions enable this option today. If enabled but
+    CONFIG_FW_LOADER_USER_HELPER_FALLBACK is disabled, only the custom fallback
+    mechanism is available and for the request_firmware_nowait() call.
+  * CONFIG_FW_LOADER_USER_HELPER_FALLBACK: force enables each request to
+    enable the kobject uevent fallback mechanism on all firmware API calls
+    except request_firmware_direct(). Most distributions disable this option
+    today. The call request_firmware_nowait() allows for one alternative
+    fallback mechanism: if this kconfig option is enabled and your second
+    argument to request_firmware_nowait(), uevent, is set to false you are
+    informing the kernel that you have a custom fallback mechanism and it will
+    manually load the firmware. Read below for more details.
+
+Note that this means when having this configuration:
+
+CONFIG_FW_LOADER_USER_HELPER=y
+CONFIG_FW_LOADER_USER_HELPER_FALLBACK=n
+
+the kobject uevent fallback mechanism will never take effect even
+for request_firmware_nowait() when uevent is set to true.
+
+Justifying the firmware fallback mechanism
+==========================================
+
+Direct filesystem lookups may fail for a variety of reasons. Known reasons for
+this are worth itemizing and documenting as it justifies the need for the
+fallback mechanism:
+
+* Race against access with the root filesystem upon bootup.
+
+* Races upon resume from suspend. This is resolved by the firmware cache, but
+  the firmware cache is only supported if you use uevents, and its not
+  supported for request_firmware_into_buf().
+
+* Firmware is not accessible through typical means:
+        * It cannot be installed into the root filesystem
+        * The firmware provides very unique device specific data tailored for
+          the unit gathered with local information. An example is calibration
+          data for WiFi chipsets for mobile devices. This calibration data is
+          not common to all units, but tailored per unit.  Such information may
+          be installed on a separate flash partition other than where the root
+          filesystem is provided.
+
+Types of fallback mechanisms
+============================
+
+There are really two fallback mechanisms available using one shared sysfs
+interface as a loading facility:
+
+* Kobject uevent fallback mechanism
+* Custom fallback mechanism
+
+First lets document the shared sysfs loading facility.
+
+Firmware sysfs loading facility
+===============================
+
+In order to help device drivers upload firmware using a fallback mechanism
+the firmware infrastructure creates a sysfs interface to enable userspace
+to load and indicate when firmware is ready. The sysfs directory is created
+via fw_create_instance(). This call creates a new struct device named after
+the firmware requested, and establishes it in the device hierarchy by
+associating the device used to make the request as the device's parent.
+The sysfs directory's file attributes are defined and controlled through
+the new device's class (firmare_class) and group (fw_dev_attr_groups).
+This is actually where the original firmware_class.c file name comes from,
+as originally the only firmware loading mechanism available was the
+mechanism we now use as a fallback mechanism.
+
+To load firmware using the sysfs interface we expose a loading indicator,
+and a file upload firmware into:
+
+  * /sys/$DEVPATH/loading
+  * /sys/$DEVPATH/data
+
+To upload firmware you will echo 1 onto the loading file to indicate
+you are loading firmware. You then cat the firmware into the data file,
+and you notify the kernel the firmware is ready by echo'ing 0 onto
+the loading file.
+
+The firmware device used to help load firmware using sysfs is only created if
+direct firmware loading fails and if the fallback mechanism is enabled for your
+firmware request, this is set up with fw_load_from_user_helper().  It is
+important to re-iterate that no device is created if a direct filesystem lookup
+succeeded.
+
+Using::
+
+        echo 1 > /sys/$DEVPATH/loading
+
+Will clean any previous partial load at once and make the firmware API
+return an error. When loading firmware the firmware_class grows a buffer
+for the firmware in PAGE_SIZE increments to hold the image as it comes in.
+
+firmware_data_read() and firmware_loading_show() are just provided for the
+test_firmware driver for testing, they are not called in normal use or
+expected to be used regularly by userspace.
+
+Firmware kobject uevent fallback mechanism
+==========================================
+
+Since a device is created for the sysfs interface to help load firmware as a
+fallback mechanism userspace can be informed of the addition of the device by
+relying on kobject uevents. The addition of the device into the device
+hierarchy means the fallback mechanism for firmware loading has been initiated.
+For details of implementation refer to _request_firmware_load(), in particular
+on the use of dev_set_uevent_suppress() and kobject_uevent().
+
+The kernel's kobject uevent mechanism is implemented in lib/kobject_uevent.c,
+it issues uevents to userspace. As a supplement to kobject uevents Linux
+distributions could also enable CONFIG_UEVENT_HELPER_PATH, which makes use of
+core kernel's usermode helper (UMH) functionality to call out to a userspace
+helper for kobject uevents. In practice though no standard distribution has
+ever used the CONFIG_UEVENT_HELPER_PATH. If CONFIG_UEVENT_HELPER_PATH is
+enabled this binary would be called each time kobject_uevent_env() gets called
+in the kernel for each kobject uevent triggered.
+
+Different implementations have been supported in userspace to take advantage of
+this fallback mechanism. When firmware loading was only possible using the
+sysfs mechanism the userspace component "hotplug" provided the functionality of
+monitoring for kobject events. Historically this was superseded be systemd's
+udev, however firmware loading support was removed from udev as of systemd
+commit be2ea723b1d0 ("udev: remove userspace firmware loading support")
+as of v217 on August, 2014. This means most Linux distributions today are
+not using or taking advantage of the firmware fallback mechanism provided
+by kobject uevents. This is specially exacerbated due to the fact that most
+distributions today disable CONFIG_FW_LOADER_USER_HELPER_FALLBACK.
+
+Refer to do_firmware_uevent() for details of the kobject event variables
+setup. Variables passwdd with a kobject add event:
+
+* FIRMWARE=firmware name
+* TIMEOUT=timeout value
+* ASYNC=whether or not the API request was asynchronous
+
+By default DEVPATH is set by the internal kernel kobject infrastructure.
+Below is an example simple kobject uevent script::
+
+        # Both $DEVPATH and $FIRMWARE are already provided in the environment.
+        MY_FW_DIR=/lib/firmware/
+        echo 1 > /sys/$DEVPATH/loading
+        cat $MY_FW_DIR/$FIRMWARE > /sys/$DEVPATH/data
+        echo 0 > /sys/$DEVPATH/loading
+
+Firmware custom fallback mechanism
+==================================
+
+Users of the request_firmware_nowait() call have yet another option available
+at their disposal: rely on the sysfs fallback mechanism but request that no
+kobject uevents be issued to userspace. The original logic behind this
+was that utilities other than udev might be required to lookup firmware
+in non-traditional paths -- paths outside of the listing documented in the
+section 'Direct filesystem lookup'. This option is not available to any of
+the other API calls as uevents are always forced for them.
+
+Since uevents are only meaningful if the fallback mechanism is enabled
+in your kernel it would seem odd to enable uevents with kernels that do not
+have the fallback mechanism enabled in their kernels. Unfortunately we also
+rely on the uevent flag which can be disabled by request_firmware_nowait() to
+also setup the firmware cache for firmware requests. As documented above,
+the firmware cache is only set up if uevent is enabled for an API call.
+Although this can disable the firmware cache for request_firmware_nowait()
+calls, users of this API should not use it for the purposes of disabling
+the cache as that was not the original purpose of the flag. Not setting
+the uevent flag means you want to opt-in for the firmware fallback mechanism
+but you want to suppress kobject uevents, as you have a custom solution which
+will monitor for your device addition into the device hierarchy somehow and
+load firmware for you through a custom path.
+
+Firmware fallback timeout
+=========================
+
+The firmware fallback mechanism has a timeout. If firmware is not loaded
+onto the sysfs interface by the timeout value an error is sent to the
+driver. By default the timeout is set to 60 seconds if uevents are
+desirable, otherwise MAX_JIFFY_OFFSET is used (max timeout possible).
+The logic behind using MAX_JIFFY_OFFSET for non-uevents is that a custom
+solution will have as much time as it needs to load firmware.
+
+You can customize the firmware timeout by echo'ing your desired timeout into
+the following file:
+
+* /sys/class/firmware/timeout
+
+If you echo 0 into it means MAX_JIFFY_OFFSET will be used. The data type
+for the timeout is an int.
diff --git a/Documentation/driver-api/firmware/firmware_cache.rst b/Documentation/driver-api/firmware/firmware_cache.rst
new file mode 100644
index 000000000000..2210e5bfb332
--- /dev/null
+++ b/Documentation/driver-api/firmware/firmware_cache.rst
@@ -0,0 +1,51 @@
+==============
+Firmware cache
+==============
+
+When Linux resumes from suspend some device drivers require firmware lookups to
+re-initialize devices. During resume there may be a period of time during which
+firmware lookups are not possible, during this short period of time firmware
+requests will fail. Time is of essence though, and delaying drivers to wait for
+the root filesystem for firmware delays user experience with device
+functionality. In order to support these requirements the firmware
+infrastructure implements a firmware cache for device drivers for most API
+calls, automatically behind the scenes.
+
+The firmware cache makes using certain firmware API calls safe during a device
+driver's suspend and resume callback.  Users of these API calls needn't cache
+the firmware by themselves for dealing with firmware loss during system resume.
+
+The firmware cache works by requesting for firmware prior to suspend and
+caching it in memory. Upon resume device drivers using the firmware API will
+have access to the firmware immediately, without having to wait for the root
+filesystem to mount or dealing with possible race issues with lookups as the
+root filesystem mounts.
+
+Some implementation details about the firmware cache setup:
+
+* The firmware cache is setup by adding a devres entry for each device that
+  uses all synchronous call except :c:func:`request_firmware_into_buf`.
+
+* If an asynchronous call is used the firmware cache is only set up for a
+  device if if the second argument (uevent) to request_firmware_nowait() is
+  true. When uevent is true it requests that a kobject uevent be sent to
+  userspace for the firmware request. For details refer to the Fackback
+  mechanism documented below.
+
+* If the firmware cache is determined to be needed as per the above two
+  criteria the firmware cache is setup by adding a devres entry for the
+  device making the firmware request.
+
+* The firmware devres entry is maintained throughout the lifetime of the
+  device. This means that even if you release_firmware() the firmware cache
+  will still be used on resume from suspend.
+
+* The timeout for the fallback mechanism is temporarily reduced to 10 seconds
+  as the firmware cache is set up during suspend, the timeout is set back to
+  the old value you had configured after the cache is set up.
+
+* Upon suspend any pending non-uevent firmware requests are killed to avoid
+  stalling the kernel, this is done with kill_requests_without_uevent(). Kernel
+  calls requiring the non-uevent therefore need to implement their own firmware
+  cache mechanism but must not use the firmware API on suspend.
+
diff --git a/Documentation/driver-api/firmware/fw_search_path.rst b/Documentation/driver-api/firmware/fw_search_path.rst
new file mode 100644
index 000000000000..a360f1009fa3
--- /dev/null
+++ b/Documentation/driver-api/firmware/fw_search_path.rst
@@ -0,0 +1,26 @@
+=====================
+Firmware search paths
+=====================
+
+The following search paths are used to look for firmware on your
+root filesystem.
+
+* fw_path_para - module parameter - default is empty so this is ignored
+* /lib/firmware/updates/UTS_RELEASE/
+* /lib/firmware/updates/
+* /lib/firmware/UTS_RELEASE/
+* /lib/firmware/
+
+The module parameter ''path'' can be passed to the firmware_class module
+to activate the first optional custom fw_path_para. The custom path can
+only be up to 256 characters long. The kernel parameter passed would be:
+
+* 'firmware_class.path=$CUSTOMIZED_PATH'
+
+There is an alternative to customize the path at run time after bootup, you
+can use the file:
+
+* /sys/module/firmware_class/parameters/path
+
+You would echo into it your custom path and firmware requested will be
+searched for there first.
diff --git a/Documentation/driver-api/firmware/index.rst b/Documentation/driver-api/firmware/index.rst
new file mode 100644
index 000000000000..1abe01793031
--- /dev/null
+++ b/Documentation/driver-api/firmware/index.rst
@@ -0,0 +1,16 @@
+==================
+Linux Firmware API
+==================
+
+.. toctree::
+
+   introduction
+   core
+   request_firmware
+
+.. only::  subproject and html
+
+   Indices
+   =======
+
+   * :ref:`genindex`
diff --git a/Documentation/driver-api/firmware/introduction.rst b/Documentation/driver-api/firmware/introduction.rst
new file mode 100644
index 000000000000..211cb44eb972
--- /dev/null
+++ b/Documentation/driver-api/firmware/introduction.rst
@@ -0,0 +1,27 @@
+============
+Introduction
+============
+
+The firmware API enables kernel code to request files required
+for functionality from userspace, the uses vary:
+
+* Microcode for CPU errata
+* Device driver firmware, required to be loaded onto device
+  microcontrollers
+* Device driver information data (calibration data, EEPROM overrides),
+  some of which can be completely optional.
+
+Types of firmware requests
+==========================
+
+There are two types of calls:
+
+* Synchronous
+* Asynchronous
+
+Which one you use vary depending on your requirements, the rule of thumb
+however is you should strive to use the asynchronous APIs unless you also
+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.
diff --git a/Documentation/driver-api/firmware/lookup-order.rst b/Documentation/driver-api/firmware/lookup-order.rst
new file mode 100644
index 000000000000..88c81739683c
--- /dev/null
+++ b/Documentation/driver-api/firmware/lookup-order.rst
@@ -0,0 +1,18 @@
+=====================
+Firmware lookup order
+=====================
+
+Different functionality is available to enable firmware to be found.
+Below is chronological order of how firmware will be looked for once
+a driver issues a firmware API call.
+
+* The ''Built-in firmware'' is checked first, if the firmware is present we
+  return it immediately
+* The ''Firmware cache'' is looked at next. If the firmware is found we
+  return it immediately
+* The ''Direct filesystem lookup'' is performed next, if found we
+  return it immediately
+* If no firmware has been found and the fallback mechanism was enabled
+  the sysfs interface is created. After this either a kobject uevent
+  is issued or the custom firmware loading is relied upon for firmware
+  loading up to the timeout value.
diff --git a/Documentation/driver-api/firmware/request_firmware.rst b/Documentation/driver-api/firmware/request_firmware.rst
new file mode 100644
index 000000000000..cc0aea880824
--- /dev/null
+++ b/Documentation/driver-api/firmware/request_firmware.rst
@@ -0,0 +1,56 @@
+====================
+request_firmware API
+====================
+
+You would typically load firmware and then load it into your device somehow.
+The typical firmware work flow is reflected below::
+
+	 if(request_firmware(&fw_entry, $FIRMWARE, device) == 0)
+                copy_fw_to_device(fw_entry->data, fw_entry->size);
+	 release_firmware(fw_entry);
+
+Synchronous firmware requests
+=============================
+
+Synchronous firmware requests will wait until the firmware is found or until
+an error is returned.
+
+request_firmware
+----------------
+.. kernel-doc:: drivers/base/firmware_class.c
+   :functions: request_firmware
+
+request_firmware_direct
+-----------------------
+.. kernel-doc:: drivers/base/firmware_class.c
+   :functions: request_firmware_direct
+
+request_firmware_into_buf
+-------------------------
+.. kernel-doc:: drivers/base/firmware_class.c
+   :functions: request_firmware_into_buf
+
+Asynchronous firmware requests
+==============================
+
+Asynchronous firmware requests allow driver code to not have to wait
+until the firmware or an error is returned. Function callbacks are
+provided so that when the firmware or an error is found the driver is
+informed through the callback. request_firmware_nowait() cannot be called
+in atomic contexts.
+
+request_firmware_nowait
+-----------------------
+.. kernel-doc:: drivers/base/firmware_class.c
+   :functions: request_firmware_nowait
+
+request firmware API expected driver use
+========================================
+
+Once an API call returns you process the firmware and then release the
+firmware. For example if you used request_firmware() and it returns,
+the driver has the firmware image accessible in fw_entry->{data,size}.
+If something went wrong request_firmware() returns non-zero and fw_entry
+is set to NULL. Once your driver is done with processing the firmware it
+can call call release_firmware(fw_entry) to release the firmware image
+and any related resource.
diff --git a/Documentation/driver-api/index.rst b/Documentation/driver-api/index.rst
index 5475a2807e7a..d6f4ad1a872d 100644
--- a/Documentation/driver-api/index.rst
+++ b/Documentation/driver-api/index.rst
@@ -30,6 +30,7 @@ available subsections can be seen below.
    miscellaneous
    vme
    80211/index
+   firmware/index
 
 .. only::  subproject and html
 
diff --git a/Documentation/firmware_class/README b/Documentation/firmware_class/README
deleted file mode 100644
index cafdca8b3b15..000000000000
--- a/Documentation/firmware_class/README
+++ /dev/null
@@ -1,128 +0,0 @@
-
- request_firmware() hotplug interface:
- ------------------------------------
-	Copyright (C) 2003 Manuel Estrada Sainz
-
- Why:
- ---
-
- Today, the most extended way to use firmware in the Linux kernel is linking
- it statically in a header file. Which has political and technical issues:
-
-  1) Some firmware is not legal to redistribute.
-  2) The firmware occupies memory permanently, even though it often is just
-     used once.
-  3) Some people, like the Debian crowd, don't consider some firmware free
-     enough and remove entire drivers (e.g.: keyspan).
-
- High level behavior (mixed):
- ============================
-
- 1), kernel(driver):
-	- calls request_firmware(&fw_entry, $FIRMWARE, device)
-	- kernel searches the firmware image with name $FIRMWARE directly
-	in the below search path of root filesystem:
-		User customized search path by module parameter 'path'[1]
-		"/lib/firmware/updates/" UTS_RELEASE,
-		"/lib/firmware/updates",
-		"/lib/firmware/" UTS_RELEASE,
-		"/lib/firmware"
-	- If found, goto 7), else goto 2)
-
-	[1], the 'path' is a string parameter which length should be less
-	than 256, user should pass 'firmware_class.path=$CUSTOMIZED_PATH'
-	if firmware_class is built in kernel(the general situation)
-
- 2), userspace:
- 	- /sys/class/firmware/xxx/{loading,data} appear.
-	- hotplug gets called with a firmware identifier in $FIRMWARE
-	  and the usual hotplug environment.
-		- hotplug: echo 1 > /sys/class/firmware/xxx/loading
-
- 3), kernel: Discard any previous partial load.
-
- 4), userspace:
-		- hotplug: cat appropriate_firmware_image > \
-					/sys/class/firmware/xxx/data
-
- 5), kernel: grows a buffer in PAGE_SIZE increments to hold the image as it
-	 comes in.
-
- 6), userspace:
-		- hotplug: echo 0 > /sys/class/firmware/xxx/loading
-
- 7), kernel: request_firmware() returns and the driver has the firmware
-	 image in fw_entry->{data,size}. If something went wrong
-	 request_firmware() returns non-zero and fw_entry is set to
-	 NULL.
-
- 8), kernel(driver): Driver code calls release_firmware(fw_entry) releasing
-		 the firmware image and any related resource.
-
- High level behavior (driver code):
- ==================================
-
-	 if(request_firmware(&fw_entry, $FIRMWARE, device) == 0)
-	 	copy_fw_to_device(fw_entry->data, fw_entry->size);
-	 release_firmware(fw_entry);
-
- Sample/simple hotplug script:
- ============================
-
-	# Both $DEVPATH and $FIRMWARE are already provided in the environment.
-
-	HOTPLUG_FW_DIR=/usr/lib/hotplug/firmware/
-
-	echo 1 > /sys/$DEVPATH/loading
-	cat $HOTPLUG_FW_DIR/$FIRMWARE > /sys/$DEVPATH/data
-	echo 0 > /sys/$DEVPATH/loading
-
- Random notes:
- ============
-
- - "echo -1 > /sys/class/firmware/xxx/loading" will cancel the load at
-   once and make request_firmware() return with error.
-
- - firmware_data_read() and firmware_loading_show() are just provided
-   for testing and completeness, they are not called in normal use.
-
- - There is also /sys/class/firmware/timeout which holds a timeout in
-   seconds for the whole load operation.
-
- - request_firmware_nowait() is also provided for convenience in
-   user contexts to request firmware asynchronously, but can't be called
-   in atomic contexts.
-
-
- about in-kernel persistence:
- ---------------------------
- Under some circumstances, as explained below, it would be interesting to keep
- firmware images in non-swappable kernel memory or even in the kernel image
- (probably within initramfs).
-
- Note that this functionality has not been implemented.
-
- - Why OPTIONAL in-kernel persistence may be a good idea sometimes:
- 
-	- If the device that needs the firmware is needed to access the
-	  filesystem. When upon some error the device has to be reset and the
-	  firmware reloaded, it won't be possible to get it from userspace.
-	  e.g.:
-		- A diskless client with a network card that needs firmware.
-		- The filesystem is stored in a disk behind an scsi device
-		  that needs firmware.
-	- Replacing buggy DSDT/SSDT ACPI tables on boot.
-	  Note: this would require the persistent objects to be included
-	  within the kernel image, probably within initramfs.
-	  
-   And the same device can be needed to access the filesystem or not depending
-   on the setup, so I think that the choice on what firmware to make
-   persistent should be left to userspace.
-
- about firmware cache:
- --------------------
- After firmware cache mechanism is introduced during system sleep,
- request_firmware can be called safely inside device's suspend and
- resume callback, and callers needn't cache the firmware by
- themselves any more for dealing with firmware loss during system
- resume.
-- 
2.10.1

[PATCH v2 4/5] firmware: add SmPL report for custom fallback mechanism

[Luis R. Rodriguez]

Even though most distributions today disable the fallback mechanism
by default we've determined that we cannot remove them from the kernel.
This is not well understood so document the reason and logic behind that.

Recent discussions suggest some future userspace development prospects which
may enable fallback mechanisms to become more useful while avoiding some
historical issues. These discussions have made it clear though that there
is less value to the custom fallback mechanism and an alternative can be
provided in the future. Its also clear that some old users of the custom
fallback mechanism were using it as a copy and paste error. Because of
all this add a Coccinelle SmPL patch to help maintainers police for new
incorrect users of the custom fallback mechanism.

Best we can do for now then is police for new users of the custom
fallback mechanism and and fix incorrect users when they are spotted.
Drivers can only be transitioned out of the custom fallback mechanism
once we know old userspace cannot be not be broken by a kernel change.

The current SmPL patch reports:

$ export COCCI=scripts/coccinelle/api/request_firmware-custom-fallback.cocci
$ make coccicheck MODE=report

drivers/leds/leds-lp55xx-common.c:227:8-31: WARNING: please check if driver really needs a custom fallback mechanism
drivers/firmware/dell_rbu.c:622:17-40: WARNING: please check if driver really needs a custom fallback mechanism

Cc: Richard Purdie <rpurdie@rpsys.net>
Cc: Jacek Anaszewski <j.anaszewski@samsung.com>
Cc: linux-leds@vger.kernel.org
Cc: Abhay Salunke <Abhay_Salunke@dell.com>
Acked-by: Julia.Lawall@lip6.fr
Signed-off-by: Luis R. Rodriguez <mcgrof@kernel.org>
---
 .../driver-api/firmware/fallback-mechanisms.rst    | 17 ++++++++++
 .../api/request_firmware-custom-fallback.cocci     | 37 ++++++++++++++++++++++
 2 files changed, 54 insertions(+)
 create mode 100644 scripts/coccinelle/api/request_firmware-custom-fallback.cocci

diff --git a/Documentation/driver-api/firmware/fallback-mechanisms.rst b/Documentation/driver-api/firmware/fallback-mechanisms.rst
index d19354794e67..b87a292153c6 100644
--- a/Documentation/driver-api/firmware/fallback-mechanisms.rst
+++ b/Documentation/driver-api/firmware/fallback-mechanisms.rst
@@ -28,6 +28,12 @@ CONFIG_FW_LOADER_USER_HELPER_FALLBACK=n
 the kobject uevent fallback mechanism will never take effect even
 for request_firmware_nowait() when uevent is set to true.
 
+Although the fallback mechanisms are not used widely today they cannot be
+removed from the kernel since some old userspace may exist which could
+entirely depend on the fallback mechanism enabled with the kernel config option
+CONFIG_FW_LOADER_USER_HELPER_FALLBACK. In the future though drivers may opt
+to embrace a different API which provides alternative fallback mechanisms.
+
 Justifying the firmware fallback mechanism
 ==========================================
 
@@ -176,6 +182,17 @@ but you want to suppress kobject uevents, as you have a custom solution which
 will monitor for your device addition into the device hierarchy somehow and
 load firmware for you through a custom path.
 
+The custom fallback mechanism can often be enabled by mistake. We currently
+have only 2 users of it, and little justification to enable it for other users.
+Since it is a common driver developer mistake to enable it, help police for
+new users of the custom fallback mechanism with::
+
+        $ export COCCI=scripts/coccinelle/api/request_firmware-avoid-init-probe-init.cocci
+        $ make coccicheck MODE=report
+
+Drivers can only be transitioned out of the custom fallback mechanism
+once we know old userspace cannot be not be broken by a kernel change.
+
 Firmware fallback timeout
 =========================
 
diff --git a/scripts/coccinelle/api/request_firmware-custom-fallback.cocci b/scripts/coccinelle/api/request_firmware-custom-fallback.cocci
new file mode 100644
index 000000000000..c7598cfc4780
--- /dev/null
+++ b/scripts/coccinelle/api/request_firmware-custom-fallback.cocci
@@ -0,0 +1,37 @@
+// Avoid the firmware custom fallback mechanism at all costs
+//
+// request_firmware_nowait() API enables explicit request for use of the custom
+// fallback mechanism if firmware is not found. Chances are high its use is
+// just a copy and paste bug. Before you fix the driver be sure to *verify* no
+// custom firmware loading tool exists that would otherwise break if we replace
+// the driver to use the uevent fallback mechanism.
+//
+// Confidence: High
+//
+// Reason for low confidence:
+//
+// Copyright: (C) 2016 Luis R. Rodriguez <mcgrof@kernel.org> GPLv2.
+//
+// Options: --include-headers
+
+virtual report
+virtual context
+
+@ r1 depends on report || context @
+expression mod, name, dev, gfp, drv, cb;
+position p;
+@@
+
+(
+*request_firmware_nowait@p(mod, false, name, dev, gfp, drv, cb)
+|
+*request_firmware_nowait@p(mod, 0, name, dev, gfp, drv, cb)
+|
+*request_firmware_nowait@p(mod, FW_ACTION_NOHOTPLUG, name, dev, gfp, drv, cb)
+)
+
+@script:python depends on report@
+p << r1.p;
+@@
+
+coccilib.report.print_report(p[0], "WARNING: please check if driver really needs a custom fallback mechanism")
-- 
2.10.1

[PATCH v2 5/5] firmware: add DECLARE_FW_CUSTOM_FALLBACK() annotation

[Luis R. Rodriguez]

We need to ensure that when driver developers use the custom firmware
fallback mechanism it was not a copy and paste bug. These use cases on
upstream drivers are rare, we only have 2 upstream users and its for
really old drivers. Since valid uses are rare but possible enable a
white-list for its use, and use this same white-list annotation to refer
to the documentation covering the custom use case.

New faulty users can be reported via 0-day now.

Cc: Fengguang Wu <fengguang.wu@intel.com>
Cc: Richard Purdie <rpurdie@rpsys.net>
Cc: Jacek Anaszewski <j.anaszewski@samsung.com>
Cc: linux-leds@vger.kernel.org
Cc: Abhay Salunke <Abhay_Salunke@dell.com>
Acked-by: Jacek Anaszewski <j.anaszewski@samsung.com>
Signed-off-by: Luis R. Rodriguez <mcgrof@kernel.org>
---
 Documentation/driver-api/firmware/fallback-mechanisms.rst     | 7 +++++--
 drivers/firmware/dell_rbu.c                                   | 1 +
 drivers/leds/leds-lp55xx-common.c                             | 1 +
 include/linux/firmware.h                                      | 7 +++++++
 scripts/coccinelle/api/request_firmware-custom-fallback.cocci | 9 ++++++++-
 5 files changed, 22 insertions(+), 3 deletions(-)

diff --git a/Documentation/driver-api/firmware/fallback-mechanisms.rst b/Documentation/driver-api/firmware/fallback-mechanisms.rst
index b87a292153c6..73f509a8d2de 100644
--- a/Documentation/driver-api/firmware/fallback-mechanisms.rst
+++ b/Documentation/driver-api/firmware/fallback-mechanisms.rst
@@ -184,8 +184,11 @@ load firmware for you through a custom path.
 
 The custom fallback mechanism can often be enabled by mistake. We currently
 have only 2 users of it, and little justification to enable it for other users.
-Since it is a common driver developer mistake to enable it, help police for
-new users of the custom fallback mechanism with::
+Since it is a common driver developer mistake to enable it, driver developers
+should use DECLARE_FW_CUSTOM_FALLBACK() to both white-list and validate their
+use and also refer to the documentation for the custom loading solution.
+
+Invalid users of the custom fallback mechanism can be policed using::
 
         $ export COCCI=scripts/coccinelle/api/request_firmware-avoid-init-probe-init.cocci
         $ make coccicheck MODE=report
diff --git a/drivers/firmware/dell_rbu.c b/drivers/firmware/dell_rbu.c
index 2f452f1f7c8a..3f2aa35bc54d 100644
--- a/drivers/firmware/dell_rbu.c
+++ b/drivers/firmware/dell_rbu.c
@@ -586,6 +586,7 @@ static ssize_t read_rbu_image_type(struct file *filp, struct kobject *kobj,
 	return size;
 }
 
+DECLARE_FW_CUSTOM_FALLBACK("Documentation/dell_rbu.txt");
 static ssize_t write_rbu_image_type(struct file *filp, struct kobject *kobj,
 				    struct bin_attribute *bin_attr,
 				    char *buffer, loff_t pos, size_t count)
diff --git a/drivers/leds/leds-lp55xx-common.c b/drivers/leds/leds-lp55xx-common.c
index 5377f22ff994..04161428ee3b 100644
--- a/drivers/leds/leds-lp55xx-common.c
+++ b/drivers/leds/leds-lp55xx-common.c
@@ -219,6 +219,7 @@ static void lp55xx_firmware_loaded(const struct firmware *fw, void *context)
 	release_firmware(chip->fw);
 }
 
+DECLARE_FW_CUSTOM_FALLBACK("Documentation/leds/leds-lp55xx.txt");
 static int lp55xx_request_firmware(struct lp55xx_chip *chip)
 {
 	const char *name = chip->cl->name;
diff --git a/include/linux/firmware.h b/include/linux/firmware.h
index b1f9f0ccb8ac..e6ca19c03dcc 100644
--- a/include/linux/firmware.h
+++ b/include/linux/firmware.h
@@ -8,6 +8,13 @@
 #define FW_ACTION_NOHOTPLUG 0
 #define FW_ACTION_HOTPLUG 1
 
+/*
+ * Helper for scripts/coccinelle/api/request_firmware-custom-fallback.cocci
+ * and so users can also easily search for the documentation for the
+ * respectively needed custom fallback mechanism.
+ */
+#define DECLARE_FW_CUSTOM_FALLBACK(__usermode_helper)
+
 struct firmware {
 	size_t size;
 	const u8 *data;
diff --git a/scripts/coccinelle/api/request_firmware-custom-fallback.cocci b/scripts/coccinelle/api/request_firmware-custom-fallback.cocci
index c7598cfc4780..68cacab35b76 100644
--- a/scripts/coccinelle/api/request_firmware-custom-fallback.cocci
+++ b/scripts/coccinelle/api/request_firmware-custom-fallback.cocci
@@ -17,6 +17,13 @@
 virtual report
 virtual context
 
+@ r0 depends on report || context @
+declarer name DECLARE_FW_CUSTOM_FALLBACK;
+expression E;
+@@
+
+DECLARE_FW_CUSTOM_FALLBACK(E);
+
 @ r1 depends on report || context @
 expression mod, name, dev, gfp, drv, cb;
 position p;
@@ -30,7 +37,7 @@ position p;
 *request_firmware_nowait@p(mod, FW_ACTION_NOHOTPLUG, name, dev, gfp, drv, cb)
 )
 
-@script:python depends on report@
+@script:python depends on report && !r0 @
 p << r1.p;
 @@
 
-- 
2.10.1

Re: [PATCH v2 5/5] firmware: add DECLARE_FW_CUSTOM_FALLBACK() annotation

[Julia Lawall]

On Fri, 16 Dec 2016, Luis R. Rodriguez wrote:

> We need to ensure that when driver developers use the custom firmware
> fallback mechanism it was not a copy and paste bug. These use cases on
> upstream drivers are rare, we only have 2 upstream users and its for
> really old drivers. Since valid uses are rare but possible enable a
> white-list for its use, and use this same white-list annotation to refer
> to the documentation covering the custom use case.
>
> New faulty users can be reported via 0-day now.
>
> Cc: Fengguang Wu <fengguang.wu@intel.com>
> Cc: Richard Purdie <rpurdie@rpsys.net>
> Cc: Jacek Anaszewski <j.anaszewski@samsung.com>
> Cc: linux-leds@vger.kernel.org
> Cc: Abhay Salunke <Abhay_Salunke@dell.com>
> Acked-by: Jacek Anaszewski <j.anaszewski@samsung.com>
> Signed-off-by: Luis R. Rodriguez <mcgrof@kernel.org>
> ---
>  Documentation/driver-api/firmware/fallback-mechanisms.rst     | 7 +++++--
>  drivers/firmware/dell_rbu.c                                   | 1 +
>  drivers/leds/leds-lp55xx-common.c                             | 1 +
>  include/linux/firmware.h                                      | 7 +++++++
>  scripts/coccinelle/api/request_firmware-custom-fallback.cocci | 9 ++++++++-
>  5 files changed, 22 insertions(+), 3 deletions(-)
>
> diff --git a/Documentation/driver-api/firmware/fallback-mechanisms.rst b/Documentation/driver-api/firmware/fallback-mechanisms.rst
> index b87a292153c6..73f509a8d2de 100644
> --- a/Documentation/driver-api/firmware/fallback-mechanisms.rst
> +++ b/Documentation/driver-api/firmware/fallback-mechanisms.rst
> @@ -184,8 +184,11 @@ load firmware for you through a custom path.
>
>  The custom fallback mechanism can often be enabled by mistake. We currently
>  have only 2 users of it, and little justification to enable it for other users.
> -Since it is a common driver developer mistake to enable it, help police for
> -new users of the custom fallback mechanism with::
> +Since it is a common driver developer mistake to enable it, driver developers
> +should use DECLARE_FW_CUSTOM_FALLBACK() to both white-list and validate their
> +use and also refer to the documentation for the custom loading solution.
> +
> +Invalid users of the custom fallback mechanism can be policed using::
>
>          $ export COCCI=scripts/coccinelle/api/request_firmware-avoid-init-probe-init.cocci
>          $ make coccicheck MODE=report
> diff --git a/drivers/firmware/dell_rbu.c b/drivers/firmware/dell_rbu.c
> index 2f452f1f7c8a..3f2aa35bc54d 100644
> --- a/drivers/firmware/dell_rbu.c
> +++ b/drivers/firmware/dell_rbu.c
> @@ -586,6 +586,7 @@ static ssize_t read_rbu_image_type(struct file *filp, struct kobject *kobj,
>  	return size;
>  }
>
> +DECLARE_FW_CUSTOM_FALLBACK("Documentation/dell_rbu.txt");
>  static ssize_t write_rbu_image_type(struct file *filp, struct kobject *kobj,
>  				    struct bin_attribute *bin_attr,
>  				    char *buffer, loff_t pos, size_t count)
> diff --git a/drivers/leds/leds-lp55xx-common.c b/drivers/leds/leds-lp55xx-common.c
> index 5377f22ff994..04161428ee3b 100644
> --- a/drivers/leds/leds-lp55xx-common.c
> +++ b/drivers/leds/leds-lp55xx-common.c
> @@ -219,6 +219,7 @@ static void lp55xx_firmware_loaded(const struct firmware *fw, void *context)
>  	release_firmware(chip->fw);
>  }
>
> +DECLARE_FW_CUSTOM_FALLBACK("Documentation/leds/leds-lp55xx.txt");
>  static int lp55xx_request_firmware(struct lp55xx_chip *chip)
>  {
>  	const char *name = chip->cl->name;
> diff --git a/include/linux/firmware.h b/include/linux/firmware.h
> index b1f9f0ccb8ac..e6ca19c03dcc 100644
> --- a/include/linux/firmware.h
> +++ b/include/linux/firmware.h
> @@ -8,6 +8,13 @@
>  #define FW_ACTION_NOHOTPLUG 0
>  #define FW_ACTION_HOTPLUG 1
>
> +/*
> + * Helper for scripts/coccinelle/api/request_firmware-custom-fallback.cocci
> + * and so users can also easily search for the documentation for the
> + * respectively needed custom fallback mechanism.
> + */
> +#define DECLARE_FW_CUSTOM_FALLBACK(__usermode_helper)
> +
>  struct firmware {
>  	size_t size;
>  	const u8 *data;
> diff --git a/scripts/coccinelle/api/request_firmware-custom-fallback.cocci b/scripts/coccinelle/api/request_firmware-custom-fallback.cocci
> index c7598cfc4780..68cacab35b76 100644
> --- a/scripts/coccinelle/api/request_firmware-custom-fallback.cocci
> +++ b/scripts/coccinelle/api/request_firmware-custom-fallback.cocci
> @@ -17,6 +17,13 @@
>  virtual report
>  virtual context
>
> +@ r0 depends on report || context @
> +declarer name DECLARE_FW_CUSTOM_FALLBACK;
> +expression E;
> +@@
> +
> +DECLARE_FW_CUSTOM_FALLBACK(E);
> +
>  @ r1 depends on report || context @
>  expression mod, name, dev, gfp, drv, cb;
>  position p;
> @@ -30,7 +37,7 @@ position p;
>  *request_firmware_nowait@p(mod, FW_ACTION_NOHOTPLUG, name, dev, gfp, drv, cb)

It looks suspicious that your added a new rule r0, that the python rule
below depends on r0 failing, and that the rule with the * (context mode)
does not depend on r0 in any way.

julia

>  )
>
> -@script:python depends on report@
> +@script:python depends on report && !r0 @
>  p << r1.p;
>  @@
>
> --
> 2.10.1
>
>

Re: [PATCH v2 5/5] firmware: add DECLARE_FW_CUSTOM_FALLBACK() annotation

[Luis R. Rodriguez]

On Mon, Dec 19, 2016 at 11:23:17AM +0100, Julia Lawall wrote:
> 
> 
> On Fri, 16 Dec 2016, Luis R. Rodriguez wrote:
> 
> > We need to ensure that when driver developers use the custom firmware
> > fallback mechanism it was not a copy and paste bug. These use cases on
> > upstream drivers are rare, we only have 2 upstream users and its for
> > really old drivers. Since valid uses are rare but possible enable a
> > white-list for its use, and use this same white-list annotation to refer
> > to the documentation covering the custom use case.
> >
> > New faulty users can be reported via 0-day now.
> >
> > Cc: Fengguang Wu <fengguang.wu@intel.com>
> > Cc: Richard Purdie <rpurdie@rpsys.net>
> > Cc: Jacek Anaszewski <j.anaszewski@samsung.com>
> > Cc: linux-leds@vger.kernel.org
> > Cc: Abhay Salunke <Abhay_Salunke@dell.com>
> > Acked-by: Jacek Anaszewski <j.anaszewski@samsung.com>
> > Signed-off-by: Luis R. Rodriguez <mcgrof@kernel.org>
> > ---
> >  Documentation/driver-api/firmware/fallback-mechanisms.rst     | 7 +++++--
> >  drivers/firmware/dell_rbu.c                                   | 1 +
> >  drivers/leds/leds-lp55xx-common.c                             | 1 +
> >  include/linux/firmware.h                                      | 7 +++++++
> >  scripts/coccinelle/api/request_firmware-custom-fallback.cocci | 9 ++++++++-
> >  5 files changed, 22 insertions(+), 3 deletions(-)
> >
> > diff --git a/Documentation/driver-api/firmware/fallback-mechanisms.rst b/Documentation/driver-api/firmware/fallback-mechanisms.rst
> > index b87a292153c6..73f509a8d2de 100644
> > --- a/Documentation/driver-api/firmware/fallback-mechanisms.rst
> > +++ b/Documentation/driver-api/firmware/fallback-mechanisms.rst
> > @@ -184,8 +184,11 @@ load firmware for you through a custom path.
> >
> >  The custom fallback mechanism can often be enabled by mistake. We currently
> >  have only 2 users of it, and little justification to enable it for other users.
> > -Since it is a common driver developer mistake to enable it, help police for
> > -new users of the custom fallback mechanism with::
> > +Since it is a common driver developer mistake to enable it, driver developers
> > +should use DECLARE_FW_CUSTOM_FALLBACK() to both white-list and validate their
> > +use and also refer to the documentation for the custom loading solution.
> > +
> > +Invalid users of the custom fallback mechanism can be policed using::
> >
> >          $ export COCCI=scripts/coccinelle/api/request_firmware-avoid-init-probe-init.cocci
> >          $ make coccicheck MODE=report
> > diff --git a/drivers/firmware/dell_rbu.c b/drivers/firmware/dell_rbu.c
> > index 2f452f1f7c8a..3f2aa35bc54d 100644
> > --- a/drivers/firmware/dell_rbu.c
> > +++ b/drivers/firmware/dell_rbu.c
> > @@ -586,6 +586,7 @@ static ssize_t read_rbu_image_type(struct file *filp, struct kobject *kobj,
> >  	return size;
> >  }
> >
> > +DECLARE_FW_CUSTOM_FALLBACK("Documentation/dell_rbu.txt");
> >  static ssize_t write_rbu_image_type(struct file *filp, struct kobject *kobj,
> >  				    struct bin_attribute *bin_attr,
> >  				    char *buffer, loff_t pos, size_t count)
> > diff --git a/drivers/leds/leds-lp55xx-common.c b/drivers/leds/leds-lp55xx-common.c
> > index 5377f22ff994..04161428ee3b 100644
> > --- a/drivers/leds/leds-lp55xx-common.c
> > +++ b/drivers/leds/leds-lp55xx-common.c
> > @@ -219,6 +219,7 @@ static void lp55xx_firmware_loaded(const struct firmware *fw, void *context)
> >  	release_firmware(chip->fw);
> >  }
> >
> > +DECLARE_FW_CUSTOM_FALLBACK("Documentation/leds/leds-lp55xx.txt");
> >  static int lp55xx_request_firmware(struct lp55xx_chip *chip)
> >  {
> >  	const char *name = chip->cl->name;
> > diff --git a/include/linux/firmware.h b/include/linux/firmware.h
> > index b1f9f0ccb8ac..e6ca19c03dcc 100644
> > --- a/include/linux/firmware.h
> > +++ b/include/linux/firmware.h
> > @@ -8,6 +8,13 @@
> >  #define FW_ACTION_NOHOTPLUG 0
> >  #define FW_ACTION_HOTPLUG 1
> >
> > +/*
> > + * Helper for scripts/coccinelle/api/request_firmware-custom-fallback.cocci
> > + * and so users can also easily search for the documentation for the
> > + * respectively needed custom fallback mechanism.
> > + */
> > +#define DECLARE_FW_CUSTOM_FALLBACK(__usermode_helper)
> > +
> >  struct firmware {
> >  	size_t size;
> >  	const u8 *data;
> > diff --git a/scripts/coccinelle/api/request_firmware-custom-fallback.cocci b/scripts/coccinelle/api/request_firmware-custom-fallback.cocci
> > index c7598cfc4780..68cacab35b76 100644
> > --- a/scripts/coccinelle/api/request_firmware-custom-fallback.cocci
> > +++ b/scripts/coccinelle/api/request_firmware-custom-fallback.cocci
> > @@ -17,6 +17,13 @@
> >  virtual report
> >  virtual context
> >
> > +@ r0 depends on report || context @
> > +declarer name DECLARE_FW_CUSTOM_FALLBACK;
> > +expression E;
> > +@@
> > +
> > +DECLARE_FW_CUSTOM_FALLBACK(E);
> > +
> >  @ r1 depends on report || context @
> >  expression mod, name, dev, gfp, drv, cb;
> >  position p;
> > @@ -30,7 +37,7 @@ position p;
> >  *request_firmware_nowait@p(mod, FW_ACTION_NOHOTPLUG, name, dev, gfp, drv, cb)
> 
> It looks suspicious that your added a new rule r0, that the python rule
> below depends on r0 failing, and that the rule with the * (context mode)
> does not depend on r0 in any way.

You're right, the context mode would report all cases, I've changed it as follows:

diff --git a/scripts/coccinelle/api/request_firmware-custom-fallback.cocci b/scripts/coccinelle/api/request_firmware-custom-fallback.cocci
index 68cacab35b76..9548e5be9c0e 100644
--- a/scripts/coccinelle/api/request_firmware-custom-fallback.cocci
+++ b/scripts/coccinelle/api/request_firmware-custom-fallback.cocci
@@ -24,7 +24,7 @@ expression E;
 
 DECLARE_FW_CUSTOM_FALLBACK(E);
 
-@ r1 depends on report || context @
+@ r1 depends on !r0 && report || context @
 expression mod, name, dev, gfp, drv, cb;
 position p;
 @@
@@ -37,7 +37,7 @@ position p;
 *request_firmware_nowait@p(mod, FW_ACTION_NOHOTPLUG, name, dev, gfp, drv, cb)
 )
 
-@script:python depends on report && !r0 @
+@script:python depends on report && r1 @
 p << r1.p;
 @@
 

Re: [PATCH v2 5/5] firmware: add DECLARE_FW_CUSTOM_FALLBACK() annotation

[Luis R. Rodriguez]

On Mon, Dec 19, 2016 at 11:23:17AM +0100, Julia Lawall wrote:
> 
> 
> On Fri, 16 Dec 2016, Luis R. Rodriguez wrote:
> 
> > We need to ensure that when driver developers use the custom firmware
> > fallback mechanism it was not a copy and paste bug. These use cases on
> > upstream drivers are rare, we only have 2 upstream users and its for
> > really old drivers. Since valid uses are rare but possible enable a
> > white-list for its use, and use this same white-list annotation to refer
> > to the documentation covering the custom use case.
> >
> > New faulty users can be reported via 0-day now.
> >
> > Cc: Fengguang Wu <fengguang.wu@intel.com>
> > Cc: Richard Purdie <rpurdie@rpsys.net>
> > Cc: Jacek Anaszewski <j.anaszewski@samsung.com>
> > Cc: linux-leds@vger.kernel.org
> > Cc: Abhay Salunke <Abhay_Salunke@dell.com>
> > Acked-by: Jacek Anaszewski <j.anaszewski@samsung.com>
> > Signed-off-by: Luis R. Rodriguez <mcgrof@kernel.org>
> > ---
> >  Documentation/driver-api/firmware/fallback-mechanisms.rst     | 7 +++++--
> >  drivers/firmware/dell_rbu.c                                   | 1 +
> >  drivers/leds/leds-lp55xx-common.c                             | 1 +
> >  include/linux/firmware.h                                      | 7 +++++++
> >  scripts/coccinelle/api/request_firmware-custom-fallback.cocci | 9 ++++++++-
> >  5 files changed, 22 insertions(+), 3 deletions(-)
> >
> > diff --git a/Documentation/driver-api/firmware/fallback-mechanisms.rst b/Documentation/driver-api/firmware/fallback-mechanisms.rst
> > index b87a292153c6..73f509a8d2de 100644
> > --- a/Documentation/driver-api/firmware/fallback-mechanisms.rst
> > +++ b/Documentation/driver-api/firmware/fallback-mechanisms.rst
> > @@ -184,8 +184,11 @@ load firmware for you through a custom path.
> >
> >  The custom fallback mechanism can often be enabled by mistake. We currently
> >  have only 2 users of it, and little justification to enable it for other users.
> > -Since it is a common driver developer mistake to enable it, help police for
> > -new users of the custom fallback mechanism with::
> > +Since it is a common driver developer mistake to enable it, driver developers
> > +should use DECLARE_FW_CUSTOM_FALLBACK() to both white-list and validate their
> > +use and also refer to the documentation for the custom loading solution.
> > +
> > +Invalid users of the custom fallback mechanism can be policed using::
> >
> >          $ export COCCI=scripts/coccinelle/api/request_firmware-avoid-init-probe-init.cocci
> >          $ make coccicheck MODE=report
> > diff --git a/drivers/firmware/dell_rbu.c b/drivers/firmware/dell_rbu.c
> > index 2f452f1f7c8a..3f2aa35bc54d 100644
> > --- a/drivers/firmware/dell_rbu.c
> > +++ b/drivers/firmware/dell_rbu.c
> > @@ -586,6 +586,7 @@ static ssize_t read_rbu_image_type(struct file *filp, struct kobject *kobj,
> >  	return size;
> >  }
> >
> > +DECLARE_FW_CUSTOM_FALLBACK("Documentation/dell_rbu.txt");
> >  static ssize_t write_rbu_image_type(struct file *filp, struct kobject *kobj,
> >  				    struct bin_attribute *bin_attr,
> >  				    char *buffer, loff_t pos, size_t count)
> > diff --git a/drivers/leds/leds-lp55xx-common.c b/drivers/leds/leds-lp55xx-common.c
> > index 5377f22ff994..04161428ee3b 100644
> > --- a/drivers/leds/leds-lp55xx-common.c
> > +++ b/drivers/leds/leds-lp55xx-common.c
> > @@ -219,6 +219,7 @@ static void lp55xx_firmware_loaded(const struct firmware *fw, void *context)
> >  	release_firmware(chip->fw);
> >  }
> >
> > +DECLARE_FW_CUSTOM_FALLBACK("Documentation/leds/leds-lp55xx.txt");
> >  static int lp55xx_request_firmware(struct lp55xx_chip *chip)
> >  {
> >  	const char *name = chip->cl->name;
> > diff --git a/include/linux/firmware.h b/include/linux/firmware.h
> > index b1f9f0ccb8ac..e6ca19c03dcc 100644
> > --- a/include/linux/firmware.h
> > +++ b/include/linux/firmware.h
> > @@ -8,6 +8,13 @@
> >  #define FW_ACTION_NOHOTPLUG 0
> >  #define FW_ACTION_HOTPLUG 1
> >
> > +/*
> > + * Helper for scripts/coccinelle/api/request_firmware-custom-fallback.cocci
> > + * and so users can also easily search for the documentation for the
> > + * respectively needed custom fallback mechanism.
> > + */
> > +#define DECLARE_FW_CUSTOM_FALLBACK(__usermode_helper)
> > +
> >  struct firmware {
> >  	size_t size;
> >  	const u8 *data;
> > diff --git a/scripts/coccinelle/api/request_firmware-custom-fallback.cocci b/scripts/coccinelle/api/request_firmware-custom-fallback.cocci
> > index c7598cfc4780..68cacab35b76 100644
> > --- a/scripts/coccinelle/api/request_firmware-custom-fallback.cocci
> > +++ b/scripts/coccinelle/api/request_firmware-custom-fallback.cocci
> > @@ -17,6 +17,13 @@
> >  virtual report
> >  virtual context
> >
> > +@ r0 depends on report || context @
> > +declarer name DECLARE_FW_CUSTOM_FALLBACK;
> > +expression E;
> > +@@
> > +
> > +DECLARE_FW_CUSTOM_FALLBACK(E);
> > +
> >  @ r1 depends on report || context @
> >  expression mod, name, dev, gfp, drv, cb;
> >  position p;
> > @@ -30,7 +37,7 @@ position p;
> >  *request_firmware_nowait@p(mod, FW_ACTION_NOHOTPLUG, name, dev, gfp, drv, cb)
> 
> It looks suspicious that your added a new rule r0, that the python rule
> below depends on r0 failing, and that the rule with the * (context mode)
> does not depend on r0 in any way.

You're right, the context mode would report all cases, I've changed it as follows:

diff --git a/scripts/coccinelle/api/request_firmware-custom-fallback.cocci b/scripts/coccinelle/api/request_firmware-custom-fallback.cocci
index 68cacab35b76..9548e5be9c0e 100644
--- a/scripts/coccinelle/api/request_firmware-custom-fallback.cocci
+++ b/scripts/coccinelle/api/request_firmware-custom-fallback.cocci
@@ -24,7 +24,7 @@ expression E;
 
 DECLARE_FW_CUSTOM_FALLBACK(E);
 
-@ r1 depends on report || context @
+@ r1 depends on !r0 && report || context @
 expression mod, name, dev, gfp, drv, cb;
 position p;
 @@
@@ -37,7 +37,7 @@ position p;
 *request_firmware_nowait@p(mod, FW_ACTION_NOHOTPLUG, name, dev, gfp, drv, cb)
 )
 
-@script:python depends on report && !r0 @
+@script:python depends on report && r1 @
 p << r1.p;
 @@
 

Re: [PATCH v2 5/5] firmware: add DECLARE_FW_CUSTOM_FALLBACK() annotation

[Luis R. Rodriguez]

On Mon, Jan 9, 2017 at 2:46 PM, Luis R. Rodriguez <mcgrof@kernel.org> wrote:
> On Mon, Dec 19, 2016 at 11:23:17AM +0100, Julia Lawall wrote:
>> It looks suspicious that your added a new rule r0, that the python rule
>> below depends on r0 failing, and that the rule with the * (context mode)
>> does not depend on r0 in any way.
>
> You're right, the context mode would report all cases, I've changed it as follows:

That still didn't cut it.. will dig further.

 Luis

Re: [PATCH v2 5/5] firmware: add DECLARE_FW_CUSTOM_FALLBACK() annotation

[Luis R. Rodriguez]

On Mon, Jan 9, 2017 at 2:46 PM, Luis R. Rodriguez <mcgrof@kernel.org> wrote:
> On Mon, Dec 19, 2016 at 11:23:17AM +0100, Julia Lawall wrote:
>> It looks suspicious that your added a new rule r0, that the python rule
>> below depends on r0 failing, and that the rule with the * (context mode)
>> does not depend on r0 in any way.
>
> You're right, the context mode would report all cases, I've changed it as follows:

That still didn't cut it.. will dig further.

 Luis

Re: [PATCH v2 5/5] firmware: add DECLARE_FW_CUSTOM_FALLBACK() annotation

[Julia Lawall]

On Mon, 9 Jan 2017, Luis R. Rodriguez wrote:

> On Mon, Dec 19, 2016 at 11:23:17AM +0100, Julia Lawall wrote:
> >
> >
> > On Fri, 16 Dec 2016, Luis R. Rodriguez wrote:
> >
> > > We need to ensure that when driver developers use the custom firmware
> > > fallback mechanism it was not a copy and paste bug. These use cases on
> > > upstream drivers are rare, we only have 2 upstream users and its for
> > > really old drivers. Since valid uses are rare but possible enable a
> > > white-list for its use, and use this same white-list annotation to refer
> > > to the documentation covering the custom use case.
> > >
> > > New faulty users can be reported via 0-day now.
> > >
> > > Cc: Fengguang Wu <fengguang.wu@intel.com>
> > > Cc: Richard Purdie <rpurdie@rpsys.net>
> > > Cc: Jacek Anaszewski <j.anaszewski@samsung.com>
> > > Cc: linux-leds@vger.kernel.org
> > > Cc: Abhay Salunke <Abhay_Salunke@dell.com>
> > > Acked-by: Jacek Anaszewski <j.anaszewski@samsung.com>
> > > Signed-off-by: Luis R. Rodriguez <mcgrof@kernel.org>
> > > ---
> > >  Documentation/driver-api/firmware/fallback-mechanisms.rst     | 7 +++++--
> > >  drivers/firmware/dell_rbu.c                                   | 1 +
> > >  drivers/leds/leds-lp55xx-common.c                             | 1 +
> > >  include/linux/firmware.h                                      | 7 +++++++
> > >  scripts/coccinelle/api/request_firmware-custom-fallback.cocci | 9 ++++++++-
> > >  5 files changed, 22 insertions(+), 3 deletions(-)
> > >
> > > diff --git a/Documentation/driver-api/firmware/fallback-mechanisms.rst b/Documentation/driver-api/firmware/fallback-mechanisms.rst
> > > index b87a292153c6..73f509a8d2de 100644
> > > --- a/Documentation/driver-api/firmware/fallback-mechanisms.rst
> > > +++ b/Documentation/driver-api/firmware/fallback-mechanisms.rst
> > > @@ -184,8 +184,11 @@ load firmware for you through a custom path.
> > >
> > >  The custom fallback mechanism can often be enabled by mistake. We currently
> > >  have only 2 users of it, and little justification to enable it for other users.
> > > -Since it is a common driver developer mistake to enable it, help police for
> > > -new users of the custom fallback mechanism with::
> > > +Since it is a common driver developer mistake to enable it, driver developers
> > > +should use DECLARE_FW_CUSTOM_FALLBACK() to both white-list and validate their
> > > +use and also refer to the documentation for the custom loading solution.
> > > +
> > > +Invalid users of the custom fallback mechanism can be policed using::
> > >
> > >          $ export COCCI=scripts/coccinelle/api/request_firmware-avoid-init-probe-init.cocci
> > >          $ make coccicheck MODE=report
> > > diff --git a/drivers/firmware/dell_rbu.c b/drivers/firmware/dell_rbu.c
> > > index 2f452f1f7c8a..3f2aa35bc54d 100644
> > > --- a/drivers/firmware/dell_rbu.c
> > > +++ b/drivers/firmware/dell_rbu.c
> > > @@ -586,6 +586,7 @@ static ssize_t read_rbu_image_type(struct file *filp, struct kobject *kobj,
> > >  	return size;
> > >  }
> > >
> > > +DECLARE_FW_CUSTOM_FALLBACK("Documentation/dell_rbu.txt");
> > >  static ssize_t write_rbu_image_type(struct file *filp, struct kobject *kobj,
> > >  				    struct bin_attribute *bin_attr,
> > >  				    char *buffer, loff_t pos, size_t count)
> > > diff --git a/drivers/leds/leds-lp55xx-common.c b/drivers/leds/leds-lp55xx-common.c
> > > index 5377f22ff994..04161428ee3b 100644
> > > --- a/drivers/leds/leds-lp55xx-common.c
> > > +++ b/drivers/leds/leds-lp55xx-common.c
> > > @@ -219,6 +219,7 @@ static void lp55xx_firmware_loaded(const struct firmware *fw, void *context)
> > >  	release_firmware(chip->fw);
> > >  }
> > >
> > > +DECLARE_FW_CUSTOM_FALLBACK("Documentation/leds/leds-lp55xx.txt");
> > >  static int lp55xx_request_firmware(struct lp55xx_chip *chip)
> > >  {
> > >  	const char *name = chip->cl->name;
> > > diff --git a/include/linux/firmware.h b/include/linux/firmware.h
> > > index b1f9f0ccb8ac..e6ca19c03dcc 100644
> > > --- a/include/linux/firmware.h
> > > +++ b/include/linux/firmware.h
> > > @@ -8,6 +8,13 @@
> > >  #define FW_ACTION_NOHOTPLUG 0
> > >  #define FW_ACTION_HOTPLUG 1
> > >
> > > +/*
> > > + * Helper for scripts/coccinelle/api/request_firmware-custom-fallback.cocci
> > > + * and so users can also easily search for the documentation for the
> > > + * respectively needed custom fallback mechanism.
> > > + */
> > > +#define DECLARE_FW_CUSTOM_FALLBACK(__usermode_helper)
> > > +
> > >  struct firmware {
> > >  	size_t size;
> > >  	const u8 *data;
> > > diff --git a/scripts/coccinelle/api/request_firmware-custom-fallback.cocci b/scripts/coccinelle/api/request_firmware-custom-fallback.cocci
> > > index c7598cfc4780..68cacab35b76 100644
> > > --- a/scripts/coccinelle/api/request_firmware-custom-fallback.cocci
> > > +++ b/scripts/coccinelle/api/request_firmware-custom-fallback.cocci
> > > @@ -17,6 +17,13 @@
> > >  virtual report
> > >  virtual context
> > >
> > > +@ r0 depends on report || context @
> > > +declarer name DECLARE_FW_CUSTOM_FALLBACK;
> > > +expression E;
> > > +@@
> > > +
> > > +DECLARE_FW_CUSTOM_FALLBACK(E);
> > > +
> > >  @ r1 depends on report || context @
> > >  expression mod, name, dev, gfp, drv, cb;
> > >  position p;
> > > @@ -30,7 +37,7 @@ position p;
> > >  *request_firmware_nowait@p(mod, FW_ACTION_NOHOTPLUG, name, dev, gfp, drv, cb)
> >
> > It looks suspicious that your added a new rule r0, that the python rule
> > below depends on r0 failing, and that the rule with the * (context mode)
> > does not depend on r0 in any way.
>
> You're right, the context mode would report all cases, I've changed it as follows:
>
> diff --git a/scripts/coccinelle/api/request_firmware-custom-fallback.cocci b/scripts/coccinelle/api/request_firmware-custom-fallback.cocci
> index 68cacab35b76..9548e5be9c0e 100644
> --- a/scripts/coccinelle/api/request_firmware-custom-fallback.cocci
> +++ b/scripts/coccinelle/api/request_firmware-custom-fallback.cocci
> @@ -24,7 +24,7 @@ expression E;
>
>  DECLARE_FW_CUSTOM_FALLBACK(E);
>
> -@ r1 depends on report || context @
> +@ r1 depends on !r0 && report || context @
>  expression mod, name, dev, gfp, drv, cb;
>  position p;
>  @@
> @@ -37,7 +37,7 @@ position p;
>  *request_firmware_nowait@p(mod, FW_ACTION_NOHOTPLUG, name, dev, gfp, drv, cb)
>  )
>
> -@script:python depends on report && !r0 @
> +@script:python depends on report && r1 @
>  p << r1.p;
>  @@

It is never useful in a python rule to mention an inherited rule in the
depends line that is also mentioned in the metavariable list.  A python
rule is only applied if all the metavariables are bound.  Thus, the use of
r1.p means that r1 has to be satisfied.

If you need further suggestions, just send the whole thing again, and I
will take a look.

julia

Re: [PATCH v2 5/5] firmware: add DECLARE_FW_CUSTOM_FALLBACK() annotation

[Julia Lawall]

On Mon, 9 Jan 2017, Luis R. Rodriguez wrote:

> On Mon, Dec 19, 2016 at 11:23:17AM +0100, Julia Lawall wrote:
> >
> >
> > On Fri, 16 Dec 2016, Luis R. Rodriguez wrote:
> >
> > > We need to ensure that when driver developers use the custom firmware
> > > fallback mechanism it was not a copy and paste bug. These use cases on
> > > upstream drivers are rare, we only have 2 upstream users and its for
> > > really old drivers. Since valid uses are rare but possible enable a
> > > white-list for its use, and use this same white-list annotation to refer
> > > to the documentation covering the custom use case.
> > >
> > > New faulty users can be reported via 0-day now.
> > >
> > > Cc: Fengguang Wu <fengguang.wu@intel.com>
> > > Cc: Richard Purdie <rpurdie@rpsys.net>
> > > Cc: Jacek Anaszewski <j.anaszewski@samsung.com>
> > > Cc: linux-leds@vger.kernel.org
> > > Cc: Abhay Salunke <Abhay_Salunke@dell.com>
> > > Acked-by: Jacek Anaszewski <j.anaszewski@samsung.com>
> > > Signed-off-by: Luis R. Rodriguez <mcgrof@kernel.org>
> > > ---
> > >  Documentation/driver-api/firmware/fallback-mechanisms.rst     | 7 +++++--
> > >  drivers/firmware/dell_rbu.c                                   | 1 +
> > >  drivers/leds/leds-lp55xx-common.c                             | 1 +
> > >  include/linux/firmware.h                                      | 7 +++++++
> > >  scripts/coccinelle/api/request_firmware-custom-fallback.cocci | 9 ++++++++-
> > >  5 files changed, 22 insertions(+), 3 deletions(-)
> > >
> > > diff --git a/Documentation/driver-api/firmware/fallback-mechanisms.rst b/Documentation/driver-api/firmware/fallback-mechanisms.rst
> > > index b87a292153c6..73f509a8d2de 100644
> > > --- a/Documentation/driver-api/firmware/fallback-mechanisms.rst
> > > +++ b/Documentation/driver-api/firmware/fallback-mechanisms.rst
> > > @@ -184,8 +184,11 @@ load firmware for you through a custom path.
> > >
> > >  The custom fallback mechanism can often be enabled by mistake. We currently
> > >  have only 2 users of it, and little justification to enable it for other users.
> > > -Since it is a common driver developer mistake to enable it, help police for
> > > -new users of the custom fallback mechanism with::
> > > +Since it is a common driver developer mistake to enable it, driver developers
> > > +should use DECLARE_FW_CUSTOM_FALLBACK() to both white-list and validate their
> > > +use and also refer to the documentation for the custom loading solution.
> > > +
> > > +Invalid users of the custom fallback mechanism can be policed using::
> > >
> > >          $ export COCCI=scripts/coccinelle/api/request_firmware-avoid-init-probe-init.cocci
> > >          $ make coccicheck MODE=report
> > > diff --git a/drivers/firmware/dell_rbu.c b/drivers/firmware/dell_rbu.c
> > > index 2f452f1f7c8a..3f2aa35bc54d 100644
> > > --- a/drivers/firmware/dell_rbu.c
> > > +++ b/drivers/firmware/dell_rbu.c
> > > @@ -586,6 +586,7 @@ static ssize_t read_rbu_image_type(struct file *filp, struct kobject *kobj,
> > >  	return size;
> > >  }
> > >
> > > +DECLARE_FW_CUSTOM_FALLBACK("Documentation/dell_rbu.txt");
> > >  static ssize_t write_rbu_image_type(struct file *filp, struct kobject *kobj,
> > >  				    struct bin_attribute *bin_attr,
> > >  				    char *buffer, loff_t pos, size_t count)
> > > diff --git a/drivers/leds/leds-lp55xx-common.c b/drivers/leds/leds-lp55xx-common.c
> > > index 5377f22ff994..04161428ee3b 100644
> > > --- a/drivers/leds/leds-lp55xx-common.c
> > > +++ b/drivers/leds/leds-lp55xx-common.c
> > > @@ -219,6 +219,7 @@ static void lp55xx_firmware_loaded(const struct firmware *fw, void *context)
> > >  	release_firmware(chip->fw);
> > >  }
> > >
> > > +DECLARE_FW_CUSTOM_FALLBACK("Documentation/leds/leds-lp55xx.txt");
> > >  static int lp55xx_request_firmware(struct lp55xx_chip *chip)
> > >  {
> > >  	const char *name = chip->cl->name;
> > > diff --git a/include/linux/firmware.h b/include/linux/firmware.h
> > > index b1f9f0ccb8ac..e6ca19c03dcc 100644
> > > --- a/include/linux/firmware.h
> > > +++ b/include/linux/firmware.h
> > > @@ -8,6 +8,13 @@
> > >  #define FW_ACTION_NOHOTPLUG 0
> > >  #define FW_ACTION_HOTPLUG 1
> > >
> > > +/*
> > > + * Helper for scripts/coccinelle/api/request_firmware-custom-fallback.cocci
> > > + * and so users can also easily search for the documentation for the
> > > + * respectively needed custom fallback mechanism.
> > > + */
> > > +#define DECLARE_FW_CUSTOM_FALLBACK(__usermode_helper)
> > > +
> > >  struct firmware {
> > >  	size_t size;
> > >  	const u8 *data;
> > > diff --git a/scripts/coccinelle/api/request_firmware-custom-fallback.cocci b/scripts/coccinelle/api/request_firmware-custom-fallback.cocci
> > > index c7598cfc4780..68cacab35b76 100644
> > > --- a/scripts/coccinelle/api/request_firmware-custom-fallback.cocci
> > > +++ b/scripts/coccinelle/api/request_firmware-custom-fallback.cocci
> > > @@ -17,6 +17,13 @@
> > >  virtual report
> > >  virtual context
> > >
> > > +@ r0 depends on report || context @
> > > +declarer name DECLARE_FW_CUSTOM_FALLBACK;
> > > +expression E;
> > > +@@
> > > +
> > > +DECLARE_FW_CUSTOM_FALLBACK(E);
> > > +
> > >  @ r1 depends on report || context @
> > >  expression mod, name, dev, gfp, drv, cb;
> > >  position p;
> > > @@ -30,7 +37,7 @@ position p;
> > >  *request_firmware_nowait@p(mod, FW_ACTION_NOHOTPLUG, name, dev, gfp, drv, cb)
> >
> > It looks suspicious that your added a new rule r0, that the python rule
> > below depends on r0 failing, and that the rule with the * (context mode)
> > does not depend on r0 in any way.
>
> You're right, the context mode would report all cases, I've changed it as follows:
>
> diff --git a/scripts/coccinelle/api/request_firmware-custom-fallback.cocci b/scripts/coccinelle/api/request_firmware-custom-fallback.cocci
> index 68cacab35b76..9548e5be9c0e 100644
> --- a/scripts/coccinelle/api/request_firmware-custom-fallback.cocci
> +++ b/scripts/coccinelle/api/request_firmware-custom-fallback.cocci
> @@ -24,7 +24,7 @@ expression E;
>
>  DECLARE_FW_CUSTOM_FALLBACK(E);
>
> -@ r1 depends on report || context @
> +@ r1 depends on !r0 && report || context @
>  expression mod, name, dev, gfp, drv, cb;
>  position p;
>  @@
> @@ -37,7 +37,7 @@ position p;
>  *request_firmware_nowait@p(mod, FW_ACTION_NOHOTPLUG, name, dev, gfp, drv, cb)
>  )
>
> -@script:python depends on report && !r0 @
> +@script:python depends on report && r1 @
>  p << r1.p;
>  @@

It is never useful in a python rule to mention an inherited rule in the
depends line that is also mentioned in the metavariable list.  A python
rule is only applied if all the metavariables are bound.  Thus, the use of
r1.p means that r1 has to be satisfied.

If you need further suggestions, just send the whole thing again, and I
will take a look.

julia

Re: [PATCH v2 5/5] firmware: add DECLARE_FW_CUSTOM_FALLBACK() annotation

[Luis R. Rodriguez]

On Mon, Jan 09, 2017 at 10:09:51PM +0100, Julia Lawall wrote:
> 
> 
> On Mon, 9 Jan 2017, Luis R. Rodriguez wrote:
> 
> > On Mon, Dec 19, 2016 at 11:23:17AM +0100, Julia Lawall wrote:
> > >
> > >
> > > On Fri, 16 Dec 2016, Luis R. Rodriguez wrote:
> > >
> > > > We need to ensure that when driver developers use the custom firmware
> > > > fallback mechanism it was not a copy and paste bug. These use cases on
> > > > upstream drivers are rare, we only have 2 upstream users and its for
> > > > really old drivers. Since valid uses are rare but possible enable a
> > > > white-list for its use, and use this same white-list annotation to refer
> > > > to the documentation covering the custom use case.
> > > >
> > > > New faulty users can be reported via 0-day now.
> > > >
> > > > Cc: Fengguang Wu <fengguang.wu@intel.com>
> > > > Cc: Richard Purdie <rpurdie@rpsys.net>
> > > > Cc: Jacek Anaszewski <j.anaszewski@samsung.com>
> > > > Cc: linux-leds@vger.kernel.org
> > > > Cc: Abhay Salunke <Abhay_Salunke@dell.com>
> > > > Acked-by: Jacek Anaszewski <j.anaszewski@samsung.com>
> > > > Signed-off-by: Luis R. Rodriguez <mcgrof@kernel.org>
> > > > ---
> > > >  Documentation/driver-api/firmware/fallback-mechanisms.rst     | 7 +++++--
> > > >  drivers/firmware/dell_rbu.c                                   | 1 +
> > > >  drivers/leds/leds-lp55xx-common.c                             | 1 +
> > > >  include/linux/firmware.h                                      | 7 +++++++
> > > >  scripts/coccinelle/api/request_firmware-custom-fallback.cocci | 9 ++++++++-
> > > >  5 files changed, 22 insertions(+), 3 deletions(-)
> > > >
> > > > diff --git a/Documentation/driver-api/firmware/fallback-mechanisms.rst b/Documentation/driver-api/firmware/fallback-mechanisms.rst
> > > > index b87a292153c6..73f509a8d2de 100644
> > > > --- a/Documentation/driver-api/firmware/fallback-mechanisms.rst
> > > > +++ b/Documentation/driver-api/firmware/fallback-mechanisms.rst
> > > > @@ -184,8 +184,11 @@ load firmware for you through a custom path.
> > > >
> > > >  The custom fallback mechanism can often be enabled by mistake. We currently
> > > >  have only 2 users of it, and little justification to enable it for other users.
> > > > -Since it is a common driver developer mistake to enable it, help police for
> > > > -new users of the custom fallback mechanism with::
> > > > +Since it is a common driver developer mistake to enable it, driver developers
> > > > +should use DECLARE_FW_CUSTOM_FALLBACK() to both white-list and validate their
> > > > +use and also refer to the documentation for the custom loading solution.
> > > > +
> > > > +Invalid users of the custom fallback mechanism can be policed using::
> > > >
> > > >          $ export COCCI=scripts/coccinelle/api/request_firmware-avoid-init-probe-init.cocci
> > > >          $ make coccicheck MODE=report
> > > > diff --git a/drivers/firmware/dell_rbu.c b/drivers/firmware/dell_rbu.c
> > > > index 2f452f1f7c8a..3f2aa35bc54d 100644
> > > > --- a/drivers/firmware/dell_rbu.c
> > > > +++ b/drivers/firmware/dell_rbu.c
> > > > @@ -586,6 +586,7 @@ static ssize_t read_rbu_image_type(struct file *filp, struct kobject *kobj,
> > > >  	return size;
> > > >  }
> > > >
> > > > +DECLARE_FW_CUSTOM_FALLBACK("Documentation/dell_rbu.txt");
> > > >  static ssize_t write_rbu_image_type(struct file *filp, struct kobject *kobj,
> > > >  				    struct bin_attribute *bin_attr,
> > > >  				    char *buffer, loff_t pos, size_t count)
> > > > diff --git a/drivers/leds/leds-lp55xx-common.c b/drivers/leds/leds-lp55xx-common.c
> > > > index 5377f22ff994..04161428ee3b 100644
> > > > --- a/drivers/leds/leds-lp55xx-common.c
> > > > +++ b/drivers/leds/leds-lp55xx-common.c
> > > > @@ -219,6 +219,7 @@ static void lp55xx_firmware_loaded(const struct firmware *fw, void *context)
> > > >  	release_firmware(chip->fw);
> > > >  }
> > > >
> > > > +DECLARE_FW_CUSTOM_FALLBACK("Documentation/leds/leds-lp55xx.txt");
> > > >  static int lp55xx_request_firmware(struct lp55xx_chip *chip)
> > > >  {
> > > >  	const char *name = chip->cl->name;
> > > > diff --git a/include/linux/firmware.h b/include/linux/firmware.h
> > > > index b1f9f0ccb8ac..e6ca19c03dcc 100644
> > > > --- a/include/linux/firmware.h
> > > > +++ b/include/linux/firmware.h
> > > > @@ -8,6 +8,13 @@
> > > >  #define FW_ACTION_NOHOTPLUG 0
> > > >  #define FW_ACTION_HOTPLUG 1
> > > >
> > > > +/*
> > > > + * Helper for scripts/coccinelle/api/request_firmware-custom-fallback.cocci
> > > > + * and so users can also easily search for the documentation for the
> > > > + * respectively needed custom fallback mechanism.
> > > > + */
> > > > +#define DECLARE_FW_CUSTOM_FALLBACK(__usermode_helper)
> > > > +
> > > >  struct firmware {
> > > >  	size_t size;
> > > >  	const u8 *data;
> > > > diff --git a/scripts/coccinelle/api/request_firmware-custom-fallback.cocci b/scripts/coccinelle/api/request_firmware-custom-fallback.cocci
> > > > index c7598cfc4780..68cacab35b76 100644
> > > > --- a/scripts/coccinelle/api/request_firmware-custom-fallback.cocci
> > > > +++ b/scripts/coccinelle/api/request_firmware-custom-fallback.cocci
> > > > @@ -17,6 +17,13 @@
> > > >  virtual report
> > > >  virtual context
> > > >
> > > > +@ r0 depends on report || context @
> > > > +declarer name DECLARE_FW_CUSTOM_FALLBACK;
> > > > +expression E;
> > > > +@@
> > > > +
> > > > +DECLARE_FW_CUSTOM_FALLBACK(E);
> > > > +
> > > >  @ r1 depends on report || context @
> > > >  expression mod, name, dev, gfp, drv, cb;
> > > >  position p;
> > > > @@ -30,7 +37,7 @@ position p;
> > > >  *request_firmware_nowait@p(mod, FW_ACTION_NOHOTPLUG, name, dev, gfp, drv, cb)
> > >
> > > It looks suspicious that your added a new rule r0, that the python rule
> > > below depends on r0 failing, and that the rule with the * (context mode)
> > > does not depend on r0 in any way.
> >
> > You're right, the context mode would report all cases, I've changed it as follows:
> >
> > diff --git a/scripts/coccinelle/api/request_firmware-custom-fallback.cocci b/scripts/coccinelle/api/request_firmware-custom-fallback.cocci
> > index 68cacab35b76..9548e5be9c0e 100644
> > --- a/scripts/coccinelle/api/request_firmware-custom-fallback.cocci
> > +++ b/scripts/coccinelle/api/request_firmware-custom-fallback.cocci
> > @@ -24,7 +24,7 @@ expression E;
> >
> >  DECLARE_FW_CUSTOM_FALLBACK(E);
> >
> > -@ r1 depends on report || context @
> > +@ r1 depends on !r0 && report || context @
> >  expression mod, name, dev, gfp, drv, cb;
> >  position p;
> >  @@
> > @@ -37,7 +37,7 @@ position p;
> >  *request_firmware_nowait@p(mod, FW_ACTION_NOHOTPLUG, name, dev, gfp, drv, cb)
> >  )
> >
> > -@script:python depends on report && !r0 @
> > +@script:python depends on report && r1 @
> >  p << r1.p;
> >  @@
> 
> It is never useful in a python rule to mention an inherited rule in the
> depends line that is also mentioned in the metavariable list.  A python
> rule is only applied if all the metavariables are bound.  Thus, the use of
> r1.p means that r1 has to be satisfied.
> 
> If you need further suggestions, just send the whole thing again, and I
> will take a look.

Sure, I can just also drop context support for now as what we really are after
are the reports, and that works well.

  Luis

Re: [PATCH v2 5/5] firmware: add DECLARE_FW_CUSTOM_FALLBACK() annotation

[Luis R. Rodriguez]

On Mon, Jan 09, 2017 at 10:09:51PM +0100, Julia Lawall wrote:
> 
> 
> On Mon, 9 Jan 2017, Luis R. Rodriguez wrote:
> 
> > On Mon, Dec 19, 2016 at 11:23:17AM +0100, Julia Lawall wrote:
> > >
> > >
> > > On Fri, 16 Dec 2016, Luis R. Rodriguez wrote:
> > >
> > > > We need to ensure that when driver developers use the custom firmware
> > > > fallback mechanism it was not a copy and paste bug. These use cases on
> > > > upstream drivers are rare, we only have 2 upstream users and its for
> > > > really old drivers. Since valid uses are rare but possible enable a
> > > > white-list for its use, and use this same white-list annotation to refer
> > > > to the documentation covering the custom use case.
> > > >
> > > > New faulty users can be reported via 0-day now.
> > > >
> > > > Cc: Fengguang Wu <fengguang.wu@intel.com>
> > > > Cc: Richard Purdie <rpurdie@rpsys.net>
> > > > Cc: Jacek Anaszewski <j.anaszewski@samsung.com>
> > > > Cc: linux-leds@vger.kernel.org
> > > > Cc: Abhay Salunke <Abhay_Salunke@dell.com>
> > > > Acked-by: Jacek Anaszewski <j.anaszewski@samsung.com>
> > > > Signed-off-by: Luis R. Rodriguez <mcgrof@kernel.org>
> > > > ---
> > > >  Documentation/driver-api/firmware/fallback-mechanisms.rst     | 7 +++++--
> > > >  drivers/firmware/dell_rbu.c                                   | 1 +
> > > >  drivers/leds/leds-lp55xx-common.c                             | 1 +
> > > >  include/linux/firmware.h                                      | 7 +++++++
> > > >  scripts/coccinelle/api/request_firmware-custom-fallback.cocci | 9 ++++++++-
> > > >  5 files changed, 22 insertions(+), 3 deletions(-)
> > > >
> > > > diff --git a/Documentation/driver-api/firmware/fallback-mechanisms.rst b/Documentation/driver-api/firmware/fallback-mechanisms.rst
> > > > index b87a292153c6..73f509a8d2de 100644
> > > > --- a/Documentation/driver-api/firmware/fallback-mechanisms.rst
> > > > +++ b/Documentation/driver-api/firmware/fallback-mechanisms.rst
> > > > @@ -184,8 +184,11 @@ load firmware for you through a custom path.
> > > >
> > > >  The custom fallback mechanism can often be enabled by mistake. We currently
> > > >  have only 2 users of it, and little justification to enable it for other users.
> > > > -Since it is a common driver developer mistake to enable it, help police for
> > > > -new users of the custom fallback mechanism with::
> > > > +Since it is a common driver developer mistake to enable it, driver developers
> > > > +should use DECLARE_FW_CUSTOM_FALLBACK() to both white-list and validate their
> > > > +use and also refer to the documentation for the custom loading solution.
> > > > +
> > > > +Invalid users of the custom fallback mechanism can be policed using::
> > > >
> > > >          $ export COCCI=scripts/coccinelle/api/request_firmware-avoid-init-probe-init.cocci
> > > >          $ make coccicheck MODE=report
> > > > diff --git a/drivers/firmware/dell_rbu.c b/drivers/firmware/dell_rbu.c
> > > > index 2f452f1f7c8a..3f2aa35bc54d 100644
> > > > --- a/drivers/firmware/dell_rbu.c
> > > > +++ b/drivers/firmware/dell_rbu.c
> > > > @@ -586,6 +586,7 @@ static ssize_t read_rbu_image_type(struct file *filp, struct kobject *kobj,
> > > >  	return size;
> > > >  }
> > > >
> > > > +DECLARE_FW_CUSTOM_FALLBACK("Documentation/dell_rbu.txt");
> > > >  static ssize_t write_rbu_image_type(struct file *filp, struct kobject *kobj,
> > > >  				    struct bin_attribute *bin_attr,
> > > >  				    char *buffer, loff_t pos, size_t count)
> > > > diff --git a/drivers/leds/leds-lp55xx-common.c b/drivers/leds/leds-lp55xx-common.c
> > > > index 5377f22ff994..04161428ee3b 100644
> > > > --- a/drivers/leds/leds-lp55xx-common.c
> > > > +++ b/drivers/leds/leds-lp55xx-common.c
> > > > @@ -219,6 +219,7 @@ static void lp55xx_firmware_loaded(const struct firmware *fw, void *context)
> > > >  	release_firmware(chip->fw);
> > > >  }
> > > >
> > > > +DECLARE_FW_CUSTOM_FALLBACK("Documentation/leds/leds-lp55xx.txt");
> > > >  static int lp55xx_request_firmware(struct lp55xx_chip *chip)
> > > >  {
> > > >  	const char *name = chip->cl->name;
> > > > diff --git a/include/linux/firmware.h b/include/linux/firmware.h
> > > > index b1f9f0ccb8ac..e6ca19c03dcc 100644
> > > > --- a/include/linux/firmware.h
> > > > +++ b/include/linux/firmware.h
> > > > @@ -8,6 +8,13 @@
> > > >  #define FW_ACTION_NOHOTPLUG 0
> > > >  #define FW_ACTION_HOTPLUG 1
> > > >
> > > > +/*
> > > > + * Helper for scripts/coccinelle/api/request_firmware-custom-fallback.cocci
> > > > + * and so users can also easily search for the documentation for the
> > > > + * respectively needed custom fallback mechanism.
> > > > + */
> > > > +#define DECLARE_FW_CUSTOM_FALLBACK(__usermode_helper)
> > > > +
> > > >  struct firmware {
> > > >  	size_t size;
> > > >  	const u8 *data;
> > > > diff --git a/scripts/coccinelle/api/request_firmware-custom-fallback.cocci b/scripts/coccinelle/api/request_firmware-custom-fallback.cocci
> > > > index c7598cfc4780..68cacab35b76 100644
> > > > --- a/scripts/coccinelle/api/request_firmware-custom-fallback.cocci
> > > > +++ b/scripts/coccinelle/api/request_firmware-custom-fallback.cocci
> > > > @@ -17,6 +17,13 @@
> > > >  virtual report
> > > >  virtual context
> > > >
> > > > +@ r0 depends on report || context @
> > > > +declarer name DECLARE_FW_CUSTOM_FALLBACK;
> > > > +expression E;
> > > > +@@
> > > > +
> > > > +DECLARE_FW_CUSTOM_FALLBACK(E);
> > > > +
> > > >  @ r1 depends on report || context @
> > > >  expression mod, name, dev, gfp, drv, cb;
> > > >  position p;
> > > > @@ -30,7 +37,7 @@ position p;
> > > >  *request_firmware_nowait@p(mod, FW_ACTION_NOHOTPLUG, name, dev, gfp, drv, cb)
> > >
> > > It looks suspicious that your added a new rule r0, that the python rule
> > > below depends on r0 failing, and that the rule with the * (context mode)
> > > does not depend on r0 in any way.
> >
> > You're right, the context mode would report all cases, I've changed it as follows:
> >
> > diff --git a/scripts/coccinelle/api/request_firmware-custom-fallback.cocci b/scripts/coccinelle/api/request_firmware-custom-fallback.cocci
> > index 68cacab35b76..9548e5be9c0e 100644
> > --- a/scripts/coccinelle/api/request_firmware-custom-fallback.cocci
> > +++ b/scripts/coccinelle/api/request_firmware-custom-fallback.cocci
> > @@ -24,7 +24,7 @@ expression E;
> >
> >  DECLARE_FW_CUSTOM_FALLBACK(E);
> >
> > -@ r1 depends on report || context @
> > +@ r1 depends on !r0 && report || context @
> >  expression mod, name, dev, gfp, drv, cb;
> >  position p;
> >  @@
> > @@ -37,7 +37,7 @@ position p;
> >  *request_firmware_nowait@p(mod, FW_ACTION_NOHOTPLUG, name, dev, gfp, drv, cb)
> >  )
> >
> > -@script:python depends on report && !r0 @
> > +@script:python depends on report && r1 @
> >  p << r1.p;
> >  @@
> 
> It is never useful in a python rule to mention an inherited rule in the
> depends line that is also mentioned in the metavariable list.  A python
> rule is only applied if all the metavariables are bound.  Thus, the use of
> r1.p means that r1 has to be satisfied.
> 
> If you need further suggestions, just send the whole thing again, and I
> will take a look.

Sure, I can just also drop context support for now as what we really are after
are the reports, and that works well.

  Luis

Re: [PATCH v2 4/5] firmware: add SmPL report for custom fallback mechanism

[Greg KH]

On Fri, Dec 16, 2016 at 03:10:37AM -0800, Luis R. Rodriguez wrote:
> Even though most distributions today disable the fallback mechanism
> by default we've determined that we cannot remove them from the kernel.
> This is not well understood so document the reason and logic behind that.

Well, the biggest reason is that some distros still rely on this.  I've
seen new products being made that rely on it, so let's please stop
treating it like it is somehow a "deprecated" interface.  We can't get
rid of it, so we just have to live with it, for forever, sorry.

greg k-h

Re: [PATCH v2 4/5] firmware: add SmPL report for custom fallback mechanism

[Luis R. Rodriguez]

On Wed, Jan 11, 2017 at 09:32:26AM +0100, Greg KH wrote:
> On Fri, Dec 16, 2016 at 03:10:37AM -0800, Luis R. Rodriguez wrote:
> > Even though most distributions today disable the fallback mechanism
> > by default we've determined that we cannot remove them from the kernel.
> > This is not well understood so document the reason and logic behind that.
> 
> Well, the biggest reason is that some distros still rely on this.  I've
> seen new products being made that rely on it,

Let's be a bit more precise: upstream there are only two driver relying on this
and I've learned about the non-upstream uses which folks have been calling for
ensuring this functionality is kept for: a) non-upstream mobile 802.11 drivers or
upstream 802.11 drivers with slight out-of-tree customizations with a requirements to
get calibration data using custom mechanisms b) remote-proc users with huge firmware
requirements for which initramfs is not well suited for.

I've taken these requirements seriously in this series. Perhaps this is not well
reflected in the series enough so let me elaborate.

> so let's please stop
> treating it like it is somehow a "deprecated" interface.

In this series I no longer treats it as a deprecated interface.  This series
however does acknowledge a bit of the drawbacks and cautions folks should take
when using these interfaces though. These issues are real.

> We can't get rid of it,

I am way past that point. I've had to personally deal with both pushes now: the
misplaced crusade against the both the mislabeled "firmware usermode helper" which
was originally actually caused by the "firmware timeout issue" and now properly
diagnosed, and later those out of a) and b) users. I've listened to both carefully
and given this much thought and discussion at Plumbers through hallway tracks
and private exchanges including with systemd folks and have tried to itemize
the current drawbacks and also finally address them. One thing is clear: the
out of tree custom fallback users simply need a custom way to load firmware,
the use of the kobject driven uevent mechanism should suffice, its just we
never had a clear documented way to enable custom solutions. In discussions
with Tom Gundersen, and Johannes Berg it seems we can enable these users easily
with the kobject uevent fallback mechanism, we just need to address the existing
drawback issues. This means if taking into consideration upstream and
non-upstream drivers -- the custom fallback mechanism becomes more of a legacy thing.
So sure -- we cannot remove it, but we should avoid more propagation of it by
mistake upstream, and hence this patch.

To this end my new goal is to first properly label and document the interfaces first, 
then to itemize the drawbacks of the "custom firmware fallback mechanism", avoid further
copy and paste bugs which implicated the "custom firmware fallback mechanism" and were
frequent before (its the purpose of this patch). Then work with kernel/systemd folks
to provide a proper resolution to the drawbacks as best as we can for the general
uevent kobject fallback mechanism. This solution will work for the existing
request_firmware_nowait() API, so it will benefit from it, but only once that is
properly hashed out will I plan to add equivalent a fallback mechanism to the
drvdata API.

> so we just have to live with it, for forever, sorry.

This documentation revamp acknowledges this, but paves the way for what we
need to do for the old users of the custom fallback mechanism and of the
users which I've heard from which need a type of fallback mechanism. The
next patch white-lists though the few old upstream uses of the custom
fallback mechanism.

The plan is to never remove the old custom fallback mechanism then. The
drvdata API will start without any fallback mechanism to start with but
the plan is to incorporate support for one once we iron out all the kinks
for a clean solution for a general fallback mechanism we are happy with.

The old custom fallback mechanism will be kept forever.

  Luis

Re: [PATCH v2 4/5] firmware: add SmPL report for custom fallback mechanism

[Luis R. Rodriguez]

On Wed, Jan 11, 2017 at 09:32:26AM +0100, Greg KH wrote:
> On Fri, Dec 16, 2016 at 03:10:37AM -0800, Luis R. Rodriguez wrote:
> > Even though most distributions today disable the fallback mechanism
> > by default we've determined that we cannot remove them from the kernel.
> > This is not well understood so document the reason and logic behind that.
> 
> Well, the biggest reason is that some distros still rely on this.  I've
> seen new products being made that rely on it,

Let's be a bit more precise: upstream there are only two driver relying on this
and I've learned about the non-upstream uses which folks have been calling for
ensuring this functionality is kept for: a) non-upstream mobile 802.11 drivers or
upstream 802.11 drivers with slight out-of-tree customizations with a requirements to
get calibration data using custom mechanisms b) remote-proc users with huge firmware
requirements for which initramfs is not well suited for.

I've taken these requirements seriously in this series. Perhaps this is not well
reflected in the series enough so let me elaborate.

> so let's please stop
> treating it like it is somehow a "deprecated" interface.

In this series I no longer treats it as a deprecated interface.  This series
however does acknowledge a bit of the drawbacks and cautions folks should take
when using these interfaces though. These issues are real.

> We can't get rid of it,

I am way past that point. I've had to personally deal with both pushes now: the
misplaced crusade against the both the mislabeled "firmware usermode helper" which
was originally actually caused by the "firmware timeout issue" and now properly
diagnosed, and later those out of a) and b) users. I've listened to both carefully
and given this much thought and discussion at Plumbers through hallway tracks
and private exchanges including with systemd folks and have tried to itemize
the current drawbacks and also finally address them. One thing is clear: the
out of tree custom fallback users simply need a custom way to load firmware,
the use of the kobject driven uevent mechanism should suffice, its just we
never had a clear documented way to enable custom solutions. In discussions
with Tom Gundersen, and Johannes Berg it seems we can enable these users easily
with the kobject uevent fallback mechanism, we just need to address the existing
drawback issues. This means if taking into consideration upstream and
non-upstream drivers -- the custom fallback mechanism becomes more of a legacy thing.
So sure -- we cannot remove it, but we should avoid more propagation of it by
mistake upstream, and hence this patch.

To this end my new goal is to first properly label and document the interfaces first, 
then to itemize the drawbacks of the "custom firmware fallback mechanism", avoid further
copy and paste bugs which implicated the "custom firmware fallback mechanism" and were
frequent before (its the purpose of this patch). Then work with kernel/systemd folks
to provide a proper resolution to the drawbacks as best as we can for the general
uevent kobject fallback mechanism. This solution will work for the existing
request_firmware_nowait() API, so it will benefit from it, but only once that is
properly hashed out will I plan to add equivalent a fallback mechanism to the
drvdata API.

> so we just have to live with it, for forever, sorry.

This documentation revamp acknowledges this, but paves the way for what we
need to do for the old users of the custom fallback mechanism and of the
users which I've heard from which need a type of fallback mechanism. The
next patch white-lists though the few old upstream uses of the custom
fallback mechanism.

The plan is to never remove the old custom fallback mechanism then. The
drvdata API will start without any fallback mechanism to start with but
the plan is to incorporate support for one once we iron out all the kinks
for a clean solution for a general fallback mechanism we are happy with.

The old custom fallback mechanism will be kept forever.

  Luis

Re: [PATCH v2 4/5] firmware: add SmPL report for custom fallback mechanism

[Greg KH]

On Wed, Jan 11, 2017 at 03:02:22PM +0100, Luis R. Rodriguez wrote:
> On Wed, Jan 11, 2017 at 09:32:26AM +0100, Greg KH wrote:
> > On Fri, Dec 16, 2016 at 03:10:37AM -0800, Luis R. Rodriguez wrote:
> > > Even though most distributions today disable the fallback mechanism
> > > by default we've determined that we cannot remove them from the kernel.
> > > This is not well understood so document the reason and logic behind that.
> > 
> > Well, the biggest reason is that some distros still rely on this.  I've
> > seen new products being made that rely on it,
> 
> Let's be a bit more precise: upstream there are only two driver relying on this
> and I've learned about the non-upstream uses which folks have been calling for
> ensuring this functionality is kept for: a) non-upstream mobile 802.11 drivers or
> upstream 802.11 drivers with slight out-of-tree customizations with a requirements to
> get calibration data using custom mechanisms b) remote-proc users with huge firmware
> requirements for which initramfs is not well suited for.

That b) is a lot of devices, I know of a few million phones in the wild
right now that rely on it.  And millions is a pretty big number :)

Anyway, thanks for addressing my concerns, I'm guessing you will respin
these remaining patches and resend them as I think there were still some
comments on them?  I took the first 3 here.

Is the "drvdata" code ready in your opinion to be merged / reviewed yet?

thanks,

greg k-h

Re: [PATCH v2 4/5] firmware: add SmPL report for custom fallback mechanism

[Luis R. Rodriguez]

On Wed, Jan 11, 2017 at 9:49 AM, Greg KH <gregkh@linuxfoundation.org> wrote:
> On Wed, Jan 11, 2017 at 03:02:22PM +0100, Luis R. Rodriguez wrote:
>> On Wed, Jan 11, 2017 at 09:32:26AM +0100, Greg KH wrote:
>> > On Fri, Dec 16, 2016 at 03:10:37AM -0800, Luis R. Rodriguez wrote:
>> > > Even though most distributions today disable the fallback mechanism
>> > > by default we've determined that we cannot remove them from the kernel.
>> > > This is not well understood so document the reason and logic behind that.
>> >
>> > Well, the biggest reason is that some distros still rely on this.  I've
>> > seen new products being made that rely on it,
>>
>> Let's be a bit more precise: upstream there are only two driver relying on this
>> and I've learned about the non-upstream uses which folks have been calling for
>> ensuring this functionality is kept for: a) non-upstream mobile 802.11 drivers or
>> upstream 802.11 drivers with slight out-of-tree customizations with a requirements to
>> get calibration data using custom mechanisms b) remote-proc users with huge firmware
>> requirements for which initramfs is not well suited for.
>
> That b) is a lot of devices, I know of a few million phones in the wild
> right now that rely on it.  And millions is a pretty big number :)
>
> Anyway, thanks for addressing my concerns, I'm guessing you will respin
> these remaining patches and resend them as I think there were still some
> comments on them?  I took the first 3 here.

Yeah sure, I will address these comments.

> Is the "drvdata" code ready in your opinion to be merged / reviewed yet?

drvdata stuff is ready as can be but after the sysdata/drvdata rename
change I failed to change one of the p54 files, I also forgot to Cc
Boris on the microcode conversion so I can just respin the drvdata
series again as a separate series right after I address the concerns
for this series.

 Luis

Re: [PATCH v2 4/5] firmware: add SmPL report for custom fallback mechanism

[Luis R. Rodriguez]

On Wed, Jan 11, 2017 at 9:49 AM, Greg KH <gregkh@linuxfoundation.org> wrote:
> On Wed, Jan 11, 2017 at 03:02:22PM +0100, Luis R. Rodriguez wrote:
>> On Wed, Jan 11, 2017 at 09:32:26AM +0100, Greg KH wrote:
>> > On Fri, Dec 16, 2016 at 03:10:37AM -0800, Luis R. Rodriguez wrote:
>> > > Even though most distributions today disable the fallback mechanism
>> > > by default we've determined that we cannot remove them from the kernel.
>> > > This is not well understood so document the reason and logic behind that.
>> >
>> > Well, the biggest reason is that some distros still rely on this.  I've
>> > seen new products being made that rely on it,
>>
>> Let's be a bit more precise: upstream there are only two driver relying on this
>> and I've learned about the non-upstream uses which folks have been calling for
>> ensuring this functionality is kept for: a) non-upstream mobile 802.11 drivers or
>> upstream 802.11 drivers with slight out-of-tree customizations with a requirements to
>> get calibration data using custom mechanisms b) remote-proc users with huge firmware
>> requirements for which initramfs is not well suited for.
>
> That b) is a lot of devices, I know of a few million phones in the wild
> right now that rely on it.  And millions is a pretty big number :)
>
> Anyway, thanks for addressing my concerns, I'm guessing you will respin
> these remaining patches and resend them as I think there were still some
> comments on them?  I took the first 3 here.

Yeah sure, I will address these comments.

> Is the "drvdata" code ready in your opinion to be merged / reviewed yet?

drvdata stuff is ready as can be but after the sysdata/drvdata rename
change I failed to change one of the p54 files, I also forgot to Cc
Boris on the microcode conversion so I can just respin the drvdata
series again as a separate series right after I address the concerns
for this series.

 Luis

Re: [PATCH v2 3/5] firmware: revamp firmware documentation

[Kalle Valo]

"Luis R. Rodriguez" <mcgrof@kernel.org> writes:

> Understanding this code is getting out of control without any
> notes. Give the firmware_class driver a much needed documentation love,
> and while at it convert it to the new sphinx documentation format.
>
> v2: typos and small fixes
>
> Signed-off-by: Luis R. Rodriguez <mcgrof@kernel.org>

[...]

> +To upload firmware you will echo 1 onto the loading file to indicate
> +you are loading firmware. You then cat the firmware into the data file,

s/cat/write/?

> +Refer to do_firmware_uevent() for details of the kobject event variables
> +setup. Variables passwdd with a kobject add event:

"passed" or what? I'm not quite understanding the last sentence anyway.

> +* If an asynchronous call is used the firmware cache is only set up for a
> +  device if if the second argument (uevent) to request_firmware_nowait() is
> +  true. When uevent is true it requests that a kobject uevent be sent to
> +  userspace for the firmware request. For details refer to the Fackback
> +  mechanism documented below.

Is that "fackback" typo intentional? Pretty funny at least :)

But this was really good, thanks. I learned a lot.

-- 
Kalle Valo