* [PATCH 1/5] selftests: firmware: only modprobe if driver is missing
2016-12-13 3:08 [PATCH 0/5] firmware: doc revamp Luis R. Rodriguez
@ 2016-12-13 3:08 ` Luis R. Rodriguez
2016-12-13 3:08 ` [PATCH 2/5] selftests: firmware: send expected errors to /dev/null Luis R. Rodriguez
` (4 subsequent siblings)
5 siblings, 0 replies; 43+ messages in thread
From: Luis R. Rodriguez @ 2016-12-13 3:08 UTC (permalink / raw)
To: gregkh, ming.lei
Cc: daniel.wagner, teg, mchehab, zajec5, linux-kernel, markivx,
stephen.boyd, broonie, zohar, tiwai, johannes, chunkeey, hauke,
jwboyer, dmitry.torokhov, dwmw2, jslaby, torvalds, luto,
fengguang.wu, rpurdie, j.anaszewski, Abhay_Salunke, Julia.Lawall,
Gilles.Muller, nicolas.palix, dhowells, bjorn.andersson,
arend.vanspriel, kvalo, 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
^ permalink raw reply related [flat|nested] 43+ messages in thread* [PATCH 2/5] selftests: firmware: send expected errors to /dev/null
2016-12-13 3:08 [PATCH 0/5] firmware: doc revamp Luis R. Rodriguez
2016-12-13 3:08 ` [PATCH 1/5] selftests: firmware: only modprobe if driver is missing Luis R. Rodriguez
@ 2016-12-13 3:08 ` Luis R. Rodriguez
2016-12-13 3:08 ` [PATCH 3/5] firmware: revamp firmware documentation Luis R. Rodriguez
` (3 subsequent siblings)
5 siblings, 0 replies; 43+ messages in thread
From: Luis R. Rodriguez @ 2016-12-13 3:08 UTC (permalink / raw)
To: gregkh, ming.lei
Cc: daniel.wagner, teg, mchehab, zajec5, linux-kernel, markivx,
stephen.boyd, broonie, zohar, tiwai, johannes, chunkeey, hauke,
jwboyer, dmitry.torokhov, dwmw2, jslaby, torvalds, luto,
fengguang.wu, rpurdie, j.anaszewski, Abhay_Salunke, Julia.Lawall,
Gilles.Muller, nicolas.palix, dhowells, bjorn.andersson,
arend.vanspriel, kvalo, 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
^ permalink raw reply related [flat|nested] 43+ messages in thread* [PATCH 3/5] firmware: revamp firmware documentation
2016-12-13 3:08 [PATCH 0/5] firmware: doc revamp Luis R. Rodriguez
2016-12-13 3:08 ` [PATCH 1/5] selftests: firmware: only modprobe if driver is missing Luis R. Rodriguez
2016-12-13 3:08 ` [PATCH 2/5] selftests: firmware: send expected errors to /dev/null Luis R. Rodriguez
@ 2016-12-13 3:08 ` Luis R. Rodriguez
2016-12-13 7:26 ` Rafał Miłecki
` (2 more replies)
2016-12-13 3:08 ` [PATCH 4/5] firmware: add SmPL report for custom fallback mechanism Luis R. Rodriguez
` (2 subsequent siblings)
5 siblings, 3 replies; 43+ messages in thread
From: Luis R. Rodriguez @ 2016-12-13 3:08 UTC (permalink / raw)
To: gregkh, ming.lei
Cc: daniel.wagner, teg, mchehab, zajec5, linux-kernel, markivx,
stephen.boyd, broonie, zohar, tiwai, johannes, chunkeey, hauke,
jwboyer, dmitry.torokhov, dwmw2, jslaby, torvalds, luto,
fengguang.wu, rpurdie, j.anaszewski, Abhay_Salunke, Julia.Lawall,
Gilles.Muller, nicolas.palix, dhowells, bjorn.andersson,
arend.vanspriel, kvalo, 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.
Signed-off-by: Luis R. Rodriguez <mcgrof@kernel.org>
---
Documentation/driver-api/firmware/built-in-fw.rst | 36 ++++
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, 472 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..382e1578a693
--- /dev/null
+++ b/Documentation/driver-api/firmware/built-in-fw.rst
@@ -0,0 +1,36 @@
+=================
+Built-in firmware
+=================
+
+Firmware can be built-in to the kernel, that is built-in to vmlinux,
+to enable firmware lookups to avoid having to look for firmware from
+the filesystem. You can enable built-in firmware using the kernel
+configuration options:
+
+ * CONFIG_EXTRA_FIRMWARE
+ * CONFIG_EXTRA_FIRMWARE_DIR
+
+The should not be confused with CONFIG_FIRMWARE_IN_KERNEL, this is for drivers
+which enable 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 firmware 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.
+
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..edce1d76ce29
--- /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 firmare 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
^ permalink raw reply related [flat|nested] 43+ messages in thread* Re: [PATCH 3/5] firmware: revamp firmware documentation
2016-12-13 3:08 ` [PATCH 3/5] firmware: revamp firmware documentation Luis R. Rodriguez
@ 2016-12-13 7:26 ` Rafał Miłecki
2016-12-16 9:09 ` Luis R. Rodriguez
2016-12-13 13:26 ` Daniel Wagner
2017-01-12 14:42 ` [PATCH v4 0/2] firmware: fw doc revamp follow up Luis R. Rodriguez
2 siblings, 1 reply; 43+ messages in thread
From: Rafał Miłecki @ 2016-12-13 7:26 UTC (permalink / raw)
To: Luis R. Rodriguez, gregkh, ming.lei
Cc: daniel.wagner, teg, mchehab, linux-kernel, markivx, stephen.boyd,
broonie, zohar, tiwai, johannes, chunkeey, hauke, jwboyer,
dmitry.torokhov, dwmw2, jslaby, torvalds, luto, fengguang.wu,
rpurdie, j.anaszewski, Abhay_Salunke, Julia.Lawall,
Gilles.Muller, nicolas.palix, dhowells, bjorn.andersson,
arend.vanspriel, kvalo
On 12/13/2016 04:08 AM, Luis R. Rodriguez wrote:
> 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.
It does help to understand the class/API, thank you!
> +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 firmware rebuild.
Could it be you meant "kernel rebuild" or "vmlinux rebuild" here?
> diff --git a/Documentation/driver-api/firmware/fallback-mechanisms.rst b/Documentation/driver-api/firmware/fallback-mechanisms.rst
> new file mode 100644
> index 000000000000..edce1d76ce29
> --- /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 firmare 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.
Yeah, it really asks for API simplification ;)
^ permalink raw reply [flat|nested] 43+ messages in thread
* Re: [PATCH 3/5] firmware: revamp firmware documentation
2016-12-13 7:26 ` Rafał Miłecki
@ 2016-12-16 9:09 ` Luis R. Rodriguez
0 siblings, 0 replies; 43+ messages in thread
From: Luis R. Rodriguez @ 2016-12-16 9:09 UTC (permalink / raw)
To: Rafał Miłecki
Cc: Luis R. Rodriguez, gregkh, ming.lei, daniel.wagner, teg, mchehab,
linux-kernel, markivx, stephen.boyd, broonie, zohar, tiwai,
johannes, chunkeey, hauke, jwboyer, dmitry.torokhov, dwmw2,
jslaby, torvalds, luto, fengguang.wu, rpurdie, j.anaszewski,
Abhay_Salunke, Julia.Lawall, Gilles.Muller, nicolas.palix,
dhowells, bjorn.andersson, arend.vanspriel, kvalo
On Tue, Dec 13, 2016 at 08:26:41AM +0100, Rafał Miłecki wrote:
> On 12/13/2016 04:08 AM, Luis R. Rodriguez wrote:
> > 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.
>
> It does help to understand the class/API, thank you!
Glad it is helping!
> > +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 firmware rebuild.
>
> Could it be you meant "kernel rebuild" or "vmlinux rebuild" here?
Heh, yes... thanks, fixed!
> > diff --git a/Documentation/driver-api/firmware/fallback-mechanisms.rst b/Documentation/driver-api/firmware/fallback-mechanisms.rst
> > new file mode 100644
> > index 000000000000..edce1d76ce29
> > --- /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 firmare 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.
>
> Yeah, it really asks for API simplification ;)
So up next once this is done is the new revamp with an extensible firmware API.
Luis
^ permalink raw reply [flat|nested] 43+ messages in thread
* Re: [PATCH 3/5] firmware: revamp firmware documentation
2016-12-13 3:08 ` [PATCH 3/5] firmware: revamp firmware documentation Luis R. Rodriguez
2016-12-13 7:26 ` Rafał Miłecki
@ 2016-12-13 13:26 ` Daniel Wagner
2016-12-13 13:30 ` Rafał Miłecki
2016-12-16 9:16 ` Luis R. Rodriguez
2017-01-12 14:42 ` [PATCH v4 0/2] firmware: fw doc revamp follow up Luis R. Rodriguez
2 siblings, 2 replies; 43+ messages in thread
From: Daniel Wagner @ 2016-12-13 13:26 UTC (permalink / raw)
To: Luis R. Rodriguez, gregkh, ming.lei
Cc: teg, mchehab, zajec5, linux-kernel, markivx, stephen.boyd,
broonie, zohar, tiwai, johannes, chunkeey, hauke, jwboyer,
dmitry.torokhov, dwmw2, jslaby, torvalds, luto, fengguang.wu,
rpurdie, j.anaszewski, Abhay_Salunke, Julia.Lawall,
Gilles.Muller, nicolas.palix, dhowells, bjorn.andersson,
arend.vanspriel, kvalo
> +++ b/Documentation/driver-api/firmware/built-in-fw.rst
> @@ -0,0 +1,36 @@
> +=================
> +Built-in firmware
> +=================
> +
> +Firmware can be built-in to the kernel, that is built-in to vmlinux,
> +to enable firmware lookups to avoid having to look for firmware from
> +the filesystem.
I find the second part of the sentence a bit confusing in the wording.
> You can enable built-in firmware using the kernel
> +configuration options:
> +
> + * CONFIG_EXTRA_FIRMWARE
> + * CONFIG_EXTRA_FIRMWARE_DIR
> +
> +The should not be confused with CONFIG_FIRMWARE_IN_KERNEL, this is for drivers
s/The/This/ ?
> +which enable firmware to be built as part of the kernel build process. This
enables?
> +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 firmware rebuild.
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.
Maybe it is worth to mention, that the calibration data is unique to a
given chip, so it is individual. That is you would need to built for
each device you sell its own kernel.
[...]
> +++ 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 firmare API calls
s/firmare/firmware/
> + 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.
Ah, her is the bit about the calibration information.
cheers,
daniel
^ permalink raw reply [flat|nested] 43+ messages in thread
* Re: [PATCH 3/5] firmware: revamp firmware documentation
2016-12-13 13:26 ` Daniel Wagner
@ 2016-12-13 13:30 ` Rafał Miłecki
2016-12-16 9:18 ` Luis R. Rodriguez
2016-12-16 9:16 ` Luis R. Rodriguez
1 sibling, 1 reply; 43+ messages in thread
From: Rafał Miłecki @ 2016-12-13 13:30 UTC (permalink / raw)
To: Daniel Wagner
Cc: Luis R. Rodriguez, Greg Kroah-Hartman, Ming Lei, Tom Gundersen,
Mauro Carvalho Chehab, Linux Kernel Mailing List,
Vikram Mulukutla, Stephen Boyd, Mark Brown, zohar, Takashi Iwai,
Johannes Berg, Christian Lamparter, Hauke Mehrtens, Josh Boyer,
Dmitry Torokhov, David Woodhouse, jslaby, Linus Torvalds, luto,
Fengguang Wu, Richard Purdie, Jacek Anaszewski, Abhay_Salunke,
Julia Lawall, Gilles.Muller, nicolas.palix, dhowells,
bjorn.andersson, Arend Van Spriel, Kalle Valo
On 13 December 2016 at 14:26, Daniel Wagner <daniel.wagner@bmw-carit.de> wrote:
>> +* 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.
>
>
> Maybe it is worth to mention, that the calibration data is unique to a given
> chip, so it is individual. That is you would need to built for each device
> you sell its own kernel.
It's commonly unique to the device model, not a chip. The same chip
can be used with different power amplifiers or different antennas.
That's why you need model (board) specific calibration data.
^ permalink raw reply [flat|nested] 43+ messages in thread
* Re: [PATCH 3/5] firmware: revamp firmware documentation
2016-12-13 13:30 ` Rafał Miłecki
@ 2016-12-16 9:18 ` Luis R. Rodriguez
2016-12-16 9:34 ` Johannes Berg
0 siblings, 1 reply; 43+ messages in thread
From: Luis R. Rodriguez @ 2016-12-16 9:18 UTC (permalink / raw)
To: Rafał Miłecki
Cc: Daniel Wagner, Luis R. Rodriguez, Greg Kroah-Hartman, Ming Lei,
Tom Gundersen, Mauro Carvalho Chehab, Linux Kernel Mailing List,
Vikram Mulukutla, Stephen Boyd, Mark Brown, zohar, Takashi Iwai,
Johannes Berg, Christian Lamparter, Hauke Mehrtens, Josh Boyer,
Dmitry Torokhov, David Woodhouse, jslaby, Linus Torvalds, luto,
Fengguang Wu, Richard Purdie, Jacek Anaszewski, Abhay_Salunke,
Julia Lawall, Gilles.Muller, nicolas.palix, dhowells,
bjorn.andersson, Arend Van Spriel, Kalle Valo
On Tue, Dec 13, 2016 at 02:30:22PM +0100, Rafał Miłecki wrote:
> On 13 December 2016 at 14:26, Daniel Wagner <daniel.wagner@bmw-carit.de> wrote:
> >> +* 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.
> >
> >
> > Maybe it is worth to mention, that the calibration data is unique to a given
> > chip, so it is individual. That is you would need to built for each device
> > you sell its own kernel.
>
> It's commonly unique to the device model, not a chip. The same chip
> can be used with different power amplifiers or different antennas.
> That's why you need model (board) specific calibration data.
>From what I recall in my 802.11 days this can be often very specific
per *batch* of cards not just chip/device model, so this depends on the
manufacturing process and date.
Luis
^ permalink raw reply [flat|nested] 43+ messages in thread
* Re: [PATCH 3/5] firmware: revamp firmware documentation
2016-12-16 9:18 ` Luis R. Rodriguez
@ 2016-12-16 9:34 ` Johannes Berg
0 siblings, 0 replies; 43+ messages in thread
From: Johannes Berg @ 2016-12-16 9:34 UTC (permalink / raw)
To: Luis R. Rodriguez, Rafał Miłecki
Cc: Daniel Wagner, Greg Kroah-Hartman, Ming Lei, Tom Gundersen,
Mauro Carvalho Chehab, Linux Kernel Mailing List,
Vikram Mulukutla, Stephen Boyd, Mark Brown, zohar, Takashi Iwai,
Christian Lamparter, Hauke Mehrtens, Josh Boyer, Dmitry Torokhov,
David Woodhouse, jslaby, Linus Torvalds, luto, Fengguang Wu,
Richard Purdie, Jacek Anaszewski, Abhay_Salunke, Julia Lawall,
Gilles.Muller, nicolas.palix, dhowells, bjorn.andersson,
Arend Van Spriel, Kalle Valo
> > > Maybe it is worth to mention, that the calibration data is unique
> > > to a given chip, so it is individual. That is you would need to
> > > built for each device you sell its own kernel.
> >
> > It's commonly unique to the device model, not a chip. The same chip
> > can be used with different power amplifiers or different antennas.
> > That's why you need model (board) specific calibration data.
>
> From what I recall in my 802.11 days this can be often very specific
> per *batch* of cards not just chip/device model, so this depends on
> the manufacturing process and date.
Yes, it can be a per-device calibration done in the factory, and then
the data is programmed into the device after that calibration - but if
the device is something like a router (not a standalone NIC) then the
data might still go through request_firmware() or similar in the end.
johannes
^ permalink raw reply [flat|nested] 43+ messages in thread
* Re: [PATCH 3/5] firmware: revamp firmware documentation
2016-12-13 13:26 ` Daniel Wagner
2016-12-13 13:30 ` Rafał Miłecki
@ 2016-12-16 9:16 ` Luis R. Rodriguez
1 sibling, 0 replies; 43+ messages in thread
From: Luis R. Rodriguez @ 2016-12-16 9:16 UTC (permalink / raw)
To: Daniel Wagner
Cc: Luis R. Rodriguez, gregkh, ming.lei, teg, mchehab, zajec5,
linux-kernel, markivx, stephen.boyd, broonie, zohar, tiwai,
johannes, chunkeey, hauke, jwboyer, dmitry.torokhov, dwmw2,
jslaby, torvalds, luto, fengguang.wu, rpurdie, j.anaszewski,
Abhay_Salunke, Julia.Lawall, Gilles.Muller, nicolas.palix,
dhowells, bjorn.andersson, arend.vanspriel, kvalo
On Tue, Dec 13, 2016 at 02:26:06PM +0100, Daniel Wagner wrote:
> > +++ b/Documentation/driver-api/firmware/built-in-fw.rst
> > @@ -0,0 +1,36 @@
> > +=================
> > +Built-in firmware
> > +=================
> > +
> > +Firmware can be built-in to the kernel, that is built-in to vmlinux,
> > +to enable firmware lookups to avoid having to look for firmware from
> > +the filesystem.
>
> I find the second part of the sentence a bit confusing in the wording.
OK how about:
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
> > You can enable built-in firmware using the kernel
> > +configuration options:
> > +
> > + * CONFIG_EXTRA_FIRMWARE
> > + * CONFIG_EXTRA_FIRMWARE_DIR
> > +
> > +The should not be confused with CONFIG_FIRMWARE_IN_KERNEL, this is for drivers
>
> s/The/This/ ?
Fixed.
> > +which enable firmware to be built as part of the kernel build process. This
>
> enables?
Fixed.
> > +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 firmware rebuild.
>
> kernel rebuild?
Fixed.
> > +* 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.
>
> Maybe it is worth to mention, that the calibration data is unique to a given
> chip, so it is individual. That is you would need to built for each device
> you sell its own kernel.
Adjusted. It now says:
* 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.
> [...]
>
> > +++ 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 firmare API calls
>
> s/firmare/firmware/
Fixed.
Luis
^ permalink raw reply [flat|nested] 43+ messages in thread
* [PATCH v4 0/2] firmware: fw doc revamp follow up
2016-12-13 3:08 ` [PATCH 3/5] firmware: revamp firmware documentation Luis R. Rodriguez
2016-12-13 7:26 ` Rafał Miłecki
2016-12-13 13:26 ` Daniel Wagner
@ 2017-01-12 14:42 ` Luis R. Rodriguez
2017-01-12 14:42 ` [PATCH v4 1/2] firmware: add SmPL report for custom fallback mechanism Luis R. Rodriguez
2017-01-12 14:42 ` [PATCH v4 2/2] firmware: add DECLARE_FW_CUSTOM_FALLBACK() annotation Luis R. Rodriguez
2 siblings, 2 replies; 43+ messages in thread
From: Luis R. Rodriguez @ 2017-01-12 14:42 UTC (permalink / raw)
To: gregkh, ming.lei
Cc: bp, wagi, teg, mchehab, zajec5, linux-kernel, markivx,
stephen.boyd, broonie, zohar, tiwai, johannes, chunkeey, hauke,
jwboyer, dmitry.torokhov, dwmw2, jslaby, torvalds, luto,
fengguang.wu, rpurdie, j.anaszewski, Abhay_Salunke, Julia.Lawall,
Gilles.Muller, nicolas.palix, dhowells, bjorn.andersson,
arend.vanspriel, kvalo, Luis R. Rodriguez
This v4 addresses the last 2 remaining patches from the last doc revamp series,
which relate only to the firmware custom fallback mechanism. One of the
patches just needed minor adjustments to account for a conjunction on SmPL
grammar. We also keep the leds-lp55xx-common white-list annotation of
DECLARE_FW_CUSTOM_FALLBACK() given despite the fact that it is acknowledged
that we could do better, old userspace already exists and relies on using
request firmware instead of using sysfs to enable custom control of LEDs.
As clarified in the last series, most users of the custom fallback mechanism
have been typo issues on part of the driver developer. The custom fallback
mechanism is needed today to help with custom fetching of firmware outside of
the typical standard firmware path, and for out of tree drivers for remote-proc
firmware for really large firmware files for which initramfs is not suitable. A
proper upstream solution for this need has been discussed and we can address
this using the *standard fallback mechanism* using *kobject uevents*, what we
need is a proper design paired with corresponding userspace component. That is
underway. Given this, there is no longer any need *upstream* for these uses. To
avoid further issues and to help understand the reason and logic behind old
custom uses this adds a white-list upstream to help report if further faulty
uses are added upstream.
Luis R. Rodriguez (2):
firmware: add SmPL report for custom fallback mechanism
firmware: add DECLARE_FW_CUSTOM_FALLBACK() annotation
.../driver-api/firmware/fallback-mechanisms.rst | 20 +++++++++++
drivers/firmware/dell_rbu.c | 1 +
drivers/leds/leds-lp55xx-common.c | 1 +
include/linux/firmware.h | 7 ++++
.../api/request_firmware-custom-fallback.cocci | 42 ++++++++++++++++++++++
5 files changed, 71 insertions(+)
create mode 100644 scripts/coccinelle/api/request_firmware-custom-fallback.cocci
--
2.11.0
^ permalink raw reply [flat|nested] 43+ messages in thread
* [PATCH v4 1/2] firmware: add SmPL report for custom fallback mechanism
2017-01-12 14:42 ` [PATCH v4 0/2] firmware: fw doc revamp follow up Luis R. Rodriguez
@ 2017-01-12 14:42 ` Luis R. Rodriguez
2017-01-12 14:42 ` [PATCH v4 2/2] firmware: add DECLARE_FW_CUSTOM_FALLBACK() annotation Luis R. Rodriguez
1 sibling, 0 replies; 43+ messages in thread
From: Luis R. Rodriguez @ 2017-01-12 14:42 UTC (permalink / raw)
To: gregkh, ming.lei
Cc: bp, wagi, teg, mchehab, zajec5, linux-kernel, markivx,
stephen.boyd, broonie, zohar, tiwai, johannes, chunkeey, hauke,
jwboyer, dmitry.torokhov, dwmw2, jslaby, torvalds, luto,
fengguang.wu, rpurdie, j.anaszewski, Abhay_Salunke, Julia.Lawall,
Gilles.Muller, nicolas.palix, dhowells, bjorn.andersson,
arend.vanspriel, kvalo, Luis R. Rodriguez, linux-leds
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 | 35 ++++++++++++++++++++++
2 files changed, 52 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..0188d446b611
--- /dev/null
+++ b/scripts/coccinelle/api/request_firmware-custom-fallback.cocci
@@ -0,0 +1,35 @@
+// 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
+//
+// Copyright: (C) 2017 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.11.0
^ permalink raw reply related [flat|nested] 43+ messages in thread
* [PATCH v4 2/2] firmware: add DECLARE_FW_CUSTOM_FALLBACK() annotation
2017-01-12 14:42 ` [PATCH v4 0/2] firmware: fw doc revamp follow up Luis R. Rodriguez
2017-01-12 14:42 ` [PATCH v4 1/2] firmware: add SmPL report for custom fallback mechanism Luis R. Rodriguez
@ 2017-01-12 14:42 ` Luis R. Rodriguez
2017-01-19 11:31 ` Greg KH
1 sibling, 1 reply; 43+ messages in thread
From: Luis R. Rodriguez @ 2017-01-12 14:42 UTC (permalink / raw)
To: gregkh, ming.lei
Cc: bp, wagi, teg, mchehab, zajec5, linux-kernel, markivx,
stephen.boyd, broonie, zohar, tiwai, johannes, chunkeey, hauke,
jwboyer, dmitry.torokhov, dwmw2, jslaby, torvalds, luto,
fengguang.wu, rpurdie, j.anaszewski, Abhay_Salunke, Julia.Lawall,
Gilles.Muller, nicolas.palix, dhowells, bjorn.andersson,
arend.vanspriel, kvalo, Luis R. Rodriguez, linux-leds
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.
v2: change dependencies on rules make more sense, and fixed
context mode
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 0188d446b611..a1ed9d633441 100644
--- a/scripts/coccinelle/api/request_firmware-custom-fallback.cocci
+++ b/scripts/coccinelle/api/request_firmware-custom-fallback.cocci
@@ -15,7 +15,14 @@
virtual report
virtual context
-@ r1 depends on report || context @
+@ r0 depends on report || context @
+declarer name DECLARE_FW_CUSTOM_FALLBACK;
+expression E;
+@@
+
+DECLARE_FW_CUSTOM_FALLBACK(E);
+
+@ r1 depends on !r0 && (report || context) @
expression mod, name, dev, gfp, drv, cb;
position p;
@@
--
2.11.0
^ permalink raw reply related [flat|nested] 43+ messages in thread* Re: [PATCH v4 2/2] firmware: add DECLARE_FW_CUSTOM_FALLBACK() annotation
2017-01-12 14:42 ` [PATCH v4 2/2] firmware: add DECLARE_FW_CUSTOM_FALLBACK() annotation Luis R. Rodriguez
@ 2017-01-19 11:31 ` Greg KH
2017-01-19 16:08 ` Luis R. Rodriguez
0 siblings, 1 reply; 43+ messages in thread
From: Greg KH @ 2017-01-19 11:31 UTC (permalink / raw)
To: Luis R. Rodriguez
Cc: ming.lei, bp, wagi, teg, mchehab, zajec5, linux-kernel, markivx,
stephen.boyd, broonie, zohar, tiwai, johannes, chunkeey, hauke,
jwboyer, dmitry.torokhov, dwmw2, jslaby, torvalds, luto,
fengguang.wu, rpurdie, j.anaszewski, Abhay_Salunke, Julia.Lawall,
Gilles.Muller, nicolas.palix, dhowells, bjorn.andersson,
arend.vanspriel, kvalo, linux-leds
On Thu, Jan 12, 2017 at 06:42:50AM -0800, 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.
>
> v2: change dependencies on rules make more sense, and fixed
> context mode
>
> 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::
Ick, no, why? Why not just add a checkpatch rule instead?
>
> $ 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");
That's a pain.
> 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");
Same here.
> 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)
So you really don't need to put anything "valid" in the define argument?
This feels like such a horrid hack, I really don't like it, especially
as we don't do it anywhere else in the kernel, right? Why start now?
thanks,
greg k-h
^ permalink raw reply [flat|nested] 43+ messages in thread* Re: [PATCH v4 2/2] firmware: add DECLARE_FW_CUSTOM_FALLBACK() annotation
2017-01-19 11:31 ` Greg KH
@ 2017-01-19 16:08 ` Luis R. Rodriguez
2017-01-19 16:14 ` Greg KH
0 siblings, 1 reply; 43+ messages in thread
From: Luis R. Rodriguez @ 2017-01-19 16:08 UTC (permalink / raw)
To: Greg KH
Cc: Luis R. Rodriguez, ming.lei, bp, wagi, teg, mchehab, zajec5,
linux-kernel, markivx, stephen.boyd, broonie, zohar, tiwai,
johannes, chunkeey, hauke, jwboyer, dmitry.torokhov, dwmw2,
jslaby, torvalds, luto, fengguang.wu, rpurdie, j.anaszewski,
Abhay_Salunke, Julia.Lawall, Gilles.Muller, nicolas.palix,
dhowells, bjorn.andersson, arend.vanspriel, kvalo, linux-leds
On Thu, Jan 19, 2017 at 12:31:11PM +0100, Greg KH wrote:
> On Thu, Jan 12, 2017 at 06:42:50AM -0800, Luis R. Rodriguez wrote:
> > +Invalid users of the custom fallback mechanism can be policed using::
>
> Ick, no, why? Why not just add a checkpatch rule instead?
If its easy to do, how would we do that?
> >
> > $ 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");
>
> That's a pain.
It is easier with checkpatch?
> > 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)
>
> So you really don't need to put anything "valid" in the define argument?
> This feels like such a horrid hack, I really don't like it, especially
> as we don't do it anywhere else in the kernel, right? Why start now?
Correct me if I'm wrong but AFAICT we may not have had previous grammatical
policing done before so I think this is a question of how we would want to
handle such type of strategies. Indeed this is just one approach. Using
checkpatch is certainly possible as well, I however think using checkpatch
is a bit more hacky.
I could also just drop this completely but figured its worth discussion.
Luis
^ permalink raw reply [flat|nested] 43+ messages in thread* Re: [PATCH v4 2/2] firmware: add DECLARE_FW_CUSTOM_FALLBACK() annotation
2017-01-19 16:08 ` Luis R. Rodriguez
@ 2017-01-19 16:14 ` Greg KH
2017-01-19 21:38 ` Luis R. Rodriguez
0 siblings, 1 reply; 43+ messages in thread
From: Greg KH @ 2017-01-19 16:14 UTC (permalink / raw)
To: Luis R. Rodriguez
Cc: ming.lei, bp, wagi, teg, mchehab, zajec5, linux-kernel, markivx,
stephen.boyd, broonie, zohar, tiwai, johannes, chunkeey, hauke,
jwboyer, dmitry.torokhov, dwmw2, jslaby, torvalds, luto,
fengguang.wu, rpurdie, j.anaszewski, Abhay_Salunke, Julia.Lawall,
Gilles.Muller, nicolas.palix, dhowells, bjorn.andersson,
arend.vanspriel, kvalo, linux-leds
On Thu, Jan 19, 2017 at 05:08:25PM +0100, Luis R. Rodriguez wrote:
> On Thu, Jan 19, 2017 at 12:31:11PM +0100, Greg KH wrote:
> > On Thu, Jan 12, 2017 at 06:42:50AM -0800, Luis R. Rodriguez wrote:
> > > +Invalid users of the custom fallback mechanism can be policed using::
> >
> > Ick, no, why? Why not just add a checkpatch rule instead?
>
> If its easy to do, how would we do that?
You just patch checkpatch.pl to test for the apis you don't want used
anymore.
> > > $ 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");
> >
> > That's a pain.
>
> It is easier with checkpatch?
Yes. You are modifying the .c code just to "whitelist" an api you want
to check for. What's the odds that someone who wants to use that api
just cargo-cult-copies this line as well, to prevent the strange warning
they are getting from the build system? :)
In short, don't worry about this, it's not a big deal nor really worth
the effort.
thanks,
greg k-h
^ permalink raw reply [flat|nested] 43+ messages in thread* Re: [PATCH v4 2/2] firmware: add DECLARE_FW_CUSTOM_FALLBACK() annotation
2017-01-19 16:14 ` Greg KH
@ 2017-01-19 21:38 ` Luis R. Rodriguez
0 siblings, 0 replies; 43+ messages in thread
From: Luis R. Rodriguez @ 2017-01-19 21:38 UTC (permalink / raw)
To: Greg KH
Cc: Luis R. Rodriguez, ming.lei, bp, wagi, teg, mchehab, zajec5,
linux-kernel, markivx, stephen.boyd, broonie, zohar, tiwai,
johannes, chunkeey, hauke, jwboyer, dmitry.torokhov, dwmw2,
jslaby, torvalds, luto, fengguang.wu, rpurdie, j.anaszewski,
Abhay_Salunke, Julia.Lawall, Gilles.Muller, nicolas.palix,
dhowells, bjorn.andersson, arend.vanspriel, kvalo, linux-leds
On Thu, Jan 19, 2017 at 05:14:36PM +0100, Greg KH wrote:
> In short, don't worry about this, it's not a big deal nor really worth
> the effort.
Fine by me, I'll drop these two patches.
Luis
^ permalink raw reply [flat|nested] 43+ messages in thread
* [PATCH 4/5] firmware: add SmPL report for custom fallback mechanism
2016-12-13 3:08 [PATCH 0/5] firmware: doc revamp Luis R. Rodriguez
` (2 preceding siblings ...)
2016-12-13 3:08 ` [PATCH 3/5] firmware: revamp firmware documentation Luis R. Rodriguez
@ 2016-12-13 3:08 ` Luis R. Rodriguez
2016-12-13 6:13 ` Julia Lawall
2016-12-13 9:44 ` Jacek Anaszewski
2016-12-13 3:08 ` [PATCH 5/5] firmware: add DECLARE_FW_CUSTOM_FALLBACK() annotation Luis R. Rodriguez
2016-12-13 12:58 ` [PATCH 0/5] firmware: doc revamp Daniel Wagner
5 siblings, 2 replies; 43+ messages in thread
From: Luis R. Rodriguez @ 2016-12-13 3:08 UTC (permalink / raw)
To: gregkh, ming.lei
Cc: daniel.wagner, teg, mchehab, zajec5, linux-kernel, markivx,
stephen.boyd, broonie, zohar, tiwai, johannes, chunkeey, hauke,
jwboyer, dmitry.torokhov, dwmw2, jslaby, torvalds, luto,
fengguang.wu, rpurdie, j.anaszewski, Abhay_Salunke, Julia.Lawall,
Gilles.Muller, nicolas.palix, dhowells, bjorn.andersson,
arend.vanspriel, kvalo, Luis R. Rodriguez, linux-leds
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>
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 edce1d76ce29..955c11d6ff9d 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
^ permalink raw reply related [flat|nested] 43+ messages in thread* Re: [PATCH 4/5] firmware: add SmPL report for custom fallback mechanism
2016-12-13 3:08 ` [PATCH 4/5] firmware: add SmPL report for custom fallback mechanism Luis R. Rodriguez
@ 2016-12-13 6:13 ` Julia Lawall
2016-12-13 9:44 ` Jacek Anaszewski
1 sibling, 0 replies; 43+ messages in thread
From: Julia Lawall @ 2016-12-13 6:13 UTC (permalink / raw)
To: Luis R. Rodriguez
Cc: gregkh, ming.lei, daniel.wagner, teg, mchehab, zajec5,
linux-kernel, markivx, stephen.boyd, broonie, zohar, tiwai,
johannes, chunkeey, hauke, jwboyer, dmitry.torokhov, dwmw2,
jslaby, torvalds, luto, fengguang.wu, rpurdie, j.anaszewski,
Abhay_Salunke, Gilles Muller, nicolas.palix, dhowells,
bjorn.andersson, arend.vanspriel, kvalo, linux-leds
On Mon, 12 Dec 2016, 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.
>
> 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>
> Signed-off-by: Luis R. Rodriguez <mcgrof@kernel.org>
Acked-by: Julia.Lawall@lip6.fr
> ---
> .../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 edce1d76ce29..955c11d6ff9d 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
>
>
^ permalink raw reply [flat|nested] 43+ messages in thread
* Re: [PATCH 4/5] firmware: add SmPL report for custom fallback mechanism
2016-12-13 3:08 ` [PATCH 4/5] firmware: add SmPL report for custom fallback mechanism Luis R. Rodriguez
2016-12-13 6:13 ` Julia Lawall
@ 2016-12-13 9:44 ` Jacek Anaszewski
2016-12-14 1:48 ` Milo Kim
1 sibling, 1 reply; 43+ messages in thread
From: Jacek Anaszewski @ 2016-12-13 9:44 UTC (permalink / raw)
To: woogyom.kim
Cc: Luis R. Rodriguez, gregkh, ming.lei, daniel.wagner, teg, mchehab,
zajec5, linux-kernel, markivx, stephen.boyd, broonie, zohar,
tiwai, johannes, chunkeey, hauke, jwboyer, dmitry.torokhov,
dwmw2, jslaby, torvalds, luto, fengguang.wu, rpurdie,
Abhay_Salunke, Julia.Lawall, Gilles.Muller, nicolas.palix,
dhowells, bjorn.andersson, arend.vanspriel, kvalo, linux-leds
Hi Milo,
Could you please verify if leds-lp55xx-common.c driver
really needs a custom firmware loading fallback mechanism?
See [0] to gain some background knowledge, especially patch 3/5 seems
to provide a big amount of information.
[0] https://lkml.org/lkml/2016/12/12/717
Thanks,
Jacek Anaszewski
On 12/13/2016 04:08 AM, 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.
>
> 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>
> 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 edce1d76ce29..955c11d6ff9d 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")
>
^ permalink raw reply [flat|nested] 43+ messages in thread
* Re: [PATCH 4/5] firmware: add SmPL report for custom fallback mechanism
2016-12-13 9:44 ` Jacek Anaszewski
@ 2016-12-14 1:48 ` Milo Kim
2016-12-16 9:29 ` Luis R. Rodriguez
0 siblings, 1 reply; 43+ messages in thread
From: Milo Kim @ 2016-12-14 1:48 UTC (permalink / raw)
To: Jacek Anaszewski
Cc: Luis R. Rodriguez, gregkh, ming.lei, daniel.wagner, teg, mchehab,
zajec5, linux-kernel, markivx, stephen.boyd, broonie, zohar,
tiwai, johannes, chunkeey, hauke, jwboyer, dmitry.torokhov,
dwmw2, jslaby, torvalds, luto, fengguang.wu, rpurdie,
Abhay_Salunke, Julia.Lawall, Gilles.Muller, nicolas.palix,
dhowells, bjorn.andersson, arend.vanspriel, kvalo, linux-leds
Hi Jacek,
On 12/13/2016 06:44 PM, Jacek Anaszewski wrote:
>
> Could you please verify if leds-lp55xx-common.c driver
> really needs a custom firmware loading fallback mechanism?
Thanks for sharing this. The lp55xx-common uses this mechanism to load
and run LED effect manually, so this could be a misuse case.
I think the right solution is providing device attributes.
At this moment, four drivers use lp55xx-common code.
- lp5521, lp5523: OK if we do not support FW loading fallback mechanism
- lp5562, lp8501: need to create additional sysfs alternatively.
However, we should be careful because I'm not sure this modification
will generate the regression (breaking the user-space) or not.
Best regards,
Milo
^ permalink raw reply [flat|nested] 43+ messages in thread
* Re: [PATCH 4/5] firmware: add SmPL report for custom fallback mechanism
2016-12-14 1:48 ` Milo Kim
@ 2016-12-16 9:29 ` Luis R. Rodriguez
2017-01-11 18:51 ` Luis R. Rodriguez
0 siblings, 1 reply; 43+ messages in thread
From: Luis R. Rodriguez @ 2016-12-16 9:29 UTC (permalink / raw)
To: Milo Kim
Cc: Jacek Anaszewski, Luis R. Rodriguez, gregkh, ming.lei,
daniel.wagner, teg, mchehab, zajec5, linux-kernel, markivx,
stephen.boyd, broonie, zohar, tiwai, johannes, chunkeey, hauke,
jwboyer, dmitry.torokhov, dwmw2, jslaby, torvalds, luto,
fengguang.wu, rpurdie, Abhay_Salunke, Julia.Lawall,
Gilles.Muller, nicolas.palix, dhowells, bjorn.andersson,
arend.vanspriel, kvalo, linux-leds
On Wed, Dec 14, 2016 at 10:48:37AM +0900, Milo Kim wrote:
> Hi Jacek,
>
> On 12/13/2016 06:44 PM, Jacek Anaszewski wrote:
> >
> > Could you please verify if leds-lp55xx-common.c driver
> > really needs a custom firmware loading fallback mechanism?
>
> Thanks for sharing this. The lp55xx-common uses this mechanism to load and
> run LED effect manually, so this could be a misuse case.
> I think the right solution is providing device attributes.
Run LED manually using request_firmware()? What the.. yes this sounds odd.
> At this moment, four drivers use lp55xx-common code.
>
> - lp5521, lp5523: OK if we do not support FW loading fallback mechanism
> - lp5562, lp8501: need to create additional sysfs alternatively.
Phew!
> However, we should be careful because I'm not sure this modification will
> generate the regression (breaking the user-space) or not.
Right so not breaking old userspace is critical.
Luis
^ permalink raw reply [flat|nested] 43+ messages in thread
* Re: [PATCH 4/5] firmware: add SmPL report for custom fallback mechanism
2016-12-16 9:29 ` Luis R. Rodriguez
@ 2017-01-11 18:51 ` Luis R. Rodriguez
0 siblings, 0 replies; 43+ messages in thread
From: Luis R. Rodriguez @ 2017-01-11 18:51 UTC (permalink / raw)
To: Pavel Machek, Milo Kim, Jacek Anaszewski
Cc: mcgrof, gregkh, ming.lei, daniel.wagner, teg, mchehab, zajec5,
linux-kernel, markivx, stephen.boyd, broonie, zohar, tiwai,
johannes, chunkeey, hauke, jwboyer, dmitry.torokhov, dwmw2,
jslaby, torvalds, luto, fengguang.wu, rpurdie, Abhay_Salunke,
Julia.Lawall, Gilles.Muller, nicolas.palix, dhowells,
bjorn.andersson, arend.vanspriel, kvalo, linux-leds
On Fri, Dec 16, 2016 at 10:29:52AM +0100, Luis R. Rodriguez wrote:
> On Wed, Dec 14, 2016 at 10:48:37AM +0900, Milo Kim wrote:
> > Hi Jacek,
> >
> > On 12/13/2016 06:44 PM, Jacek Anaszewski wrote:
> > >
> > > Could you please verify if leds-lp55xx-common.c driver
> > > really needs a custom firmware loading fallback mechanism?
> >
> > Thanks for sharing this. The lp55xx-common uses this mechanism to load and
> > run LED effect manually, so this could be a misuse case.
> > I think the right solution is providing device attributes.
>
> Run LED manually using request_firmware()? What the.. yes this sounds odd.
>
> > At this moment, four drivers use lp55xx-common code.
> >
> > - lp5521, lp5523: OK if we do not support FW loading fallback mechanism
> > - lp5562, lp8501: need to create additional sysfs alternatively.
>
> Phew!
>
> > However, we should be careful because I'm not sure this modification will
> > generate the regression (breaking the user-space) or not.
>
> Right so not breaking old userspace is critical.
So to recap:
What you can do is add support for the new interface and recommend folks
to use it. We just cannot break old userspace relying on the same module.
So the currently proposed DECLARE_FW_CUSTOM_FALLBACK() is still justified
for this driver. As stupid as the original implementation may have been
its there.. and we just have to deal with it. I would seriously appreciate
a revamp update on the Documentation/leds/leds-lp55xx.txt documentation though
with everything that we have discussed.
Luis
^ permalink raw reply [flat|nested] 43+ messages in thread
* [PATCH 5/5] firmware: add DECLARE_FW_CUSTOM_FALLBACK() annotation
2016-12-13 3:08 [PATCH 0/5] firmware: doc revamp Luis R. Rodriguez
` (3 preceding siblings ...)
2016-12-13 3:08 ` [PATCH 4/5] firmware: add SmPL report for custom fallback mechanism Luis R. Rodriguez
@ 2016-12-13 3:08 ` Luis R. Rodriguez
2016-12-13 19:04 ` Pavel Machek
2016-12-15 9:32 ` Jacek Anaszewski
2016-12-13 12:58 ` [PATCH 0/5] firmware: doc revamp Daniel Wagner
5 siblings, 2 replies; 43+ messages in thread
From: Luis R. Rodriguez @ 2016-12-13 3:08 UTC (permalink / raw)
To: gregkh, ming.lei
Cc: daniel.wagner, teg, mchehab, zajec5, linux-kernel, markivx,
stephen.boyd, broonie, zohar, tiwai, johannes, chunkeey, hauke,
jwboyer, dmitry.torokhov, dwmw2, jslaby, torvalds, luto,
fengguang.wu, rpurdie, j.anaszewski, Abhay_Salunke, Julia.Lawall,
Gilles.Muller, nicolas.palix, dhowells, bjorn.andersson,
arend.vanspriel, kvalo, Luis R. Rodriguez, linux-leds
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>
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 955c11d6ff9d..b51673e40439 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
^ permalink raw reply related [flat|nested] 43+ messages in thread* Re: [PATCH 5/5] firmware: add DECLARE_FW_CUSTOM_FALLBACK() annotation
2016-12-13 3:08 ` [PATCH 5/5] firmware: add DECLARE_FW_CUSTOM_FALLBACK() annotation Luis R. Rodriguez
@ 2016-12-13 19:04 ` Pavel Machek
2016-12-16 9:22 ` Luis R. Rodriguez
2016-12-15 9:32 ` Jacek Anaszewski
1 sibling, 1 reply; 43+ messages in thread
From: Pavel Machek @ 2016-12-13 19:04 UTC (permalink / raw)
To: Luis R. Rodriguez
Cc: gregkh, ming.lei, daniel.wagner, teg, mchehab, zajec5,
linux-kernel, markivx, stephen.boyd, broonie, zohar, tiwai,
johannes, chunkeey, hauke, jwboyer, dmitry.torokhov, dwmw2,
jslaby, torvalds, luto, fengguang.wu, rpurdie, j.anaszewski,
Abhay_Salunke, Julia.Lawall, Gilles.Muller, nicolas.palix,
dhowells, bjorn.andersson, arend.vanspriel, kvalo, linux-leds
[-- Attachment #1: Type: text/plain, Size: 1542 bytes --]
Hi!
> 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.
> --- 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;
The driver does:
static void lp55xx_firmware_loaded(const struct firmware *fw, void
*context)
{
struct lp55xx_chip *chip = context;
struct device *dev = &chip->cl->dev;
enum lp55xx_engine_index idx =
chip->engine_idx;
if (!fw) {
dev_err(dev, "firmware request failed\n");
goto out;
}
...
out:
/* firmware should be released for other channel use */
release_firmware(chip->fw);
}
Does that match the "custom fallback" definition?
--
(english) http://www.livejournal.com/~pavelmachek
(cesky, pictures) http://atrey.karlin.mff.cuni.cz/~pavel/picture/horses/blog.html
[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 181 bytes --]
^ permalink raw reply [flat|nested] 43+ messages in thread* Re: [PATCH 5/5] firmware: add DECLARE_FW_CUSTOM_FALLBACK() annotation
2016-12-13 19:04 ` Pavel Machek
@ 2016-12-16 9:22 ` Luis R. Rodriguez
2016-12-16 9:29 ` Pavel Machek
0 siblings, 1 reply; 43+ messages in thread
From: Luis R. Rodriguez @ 2016-12-16 9:22 UTC (permalink / raw)
To: Pavel Machek
Cc: Luis R. Rodriguez, gregkh, ming.lei, daniel.wagner, teg, mchehab,
zajec5, linux-kernel, markivx, stephen.boyd, broonie, zohar,
tiwai, johannes, chunkeey, hauke, jwboyer, dmitry.torokhov,
dwmw2, jslaby, torvalds, luto, fengguang.wu, rpurdie,
j.anaszewski, Abhay_Salunke, Julia.Lawall, Gilles.Muller,
nicolas.palix, dhowells, bjorn.andersson, arend.vanspriel, kvalo,
linux-leds
On Tue, Dec 13, 2016 at 08:04:29PM +0100, Pavel Machek wrote:
> Hi!
>
> > 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.
>
> > --- 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;
>
> The driver does:
>
> static void lp55xx_firmware_loaded(const struct firmware *fw, void
> *context)
> {
> struct lp55xx_chip *chip = context;
> struct device *dev = &chip->cl->dev;
> enum lp55xx_engine_index idx =
> chip->engine_idx;
>
> if (!fw) {
> dev_err(dev, "firmware request failed\n");
> goto out;
> }
> ...
> out:
> /* firmware should be released for other channel use */
> release_firmware(chip->fw);
> }
>
>
> Does that match the "custom fallback" definition?
Refer to the documentation I supplied, and also to the grammar rule, in
particular the patch "firmware: add SmPL report for custom fallback mechanism",
it captures the SmPL form for the custom fallback mechanism as:
@ 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)
)
^ permalink raw reply [flat|nested] 43+ messages in thread* Re: [PATCH 5/5] firmware: add DECLARE_FW_CUSTOM_FALLBACK() annotation
2016-12-16 9:22 ` Luis R. Rodriguez
@ 2016-12-16 9:29 ` Pavel Machek
2016-12-16 9:59 ` Luis R. Rodriguez
0 siblings, 1 reply; 43+ messages in thread
From: Pavel Machek @ 2016-12-16 9:29 UTC (permalink / raw)
To: Luis R. Rodriguez
Cc: gregkh, ming.lei, daniel.wagner, teg, mchehab, zajec5,
linux-kernel, markivx, stephen.boyd, broonie, zohar, tiwai,
johannes, chunkeey, hauke, jwboyer, dmitry.torokhov, dwmw2,
jslaby, torvalds, luto, fengguang.wu, rpurdie, j.anaszewski,
Abhay_Salunke, Julia.Lawall, Gilles.Muller, nicolas.palix,
dhowells, bjorn.andersson, arend.vanspriel, kvalo, linux-leds
[-- Attachment #1: Type: text/plain, Size: 2148 bytes --]
On Fri 2016-12-16 10:22:41, Luis R. Rodriguez wrote:
> On Tue, Dec 13, 2016 at 08:04:29PM +0100, Pavel Machek wrote:
> > Hi!
> >
> > > 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.
> >
> > > --- 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;
> >
> > The driver does:
> >
> > static void lp55xx_firmware_loaded(const struct firmware *fw, void
> > *context)
> > {
> > struct lp55xx_chip *chip = context;
> > struct device *dev = &chip->cl->dev;
> > enum lp55xx_engine_index idx =
> > chip->engine_idx;
> >
> > if (!fw) {
> > dev_err(dev, "firmware request failed\n");
> > goto out;
> > }
> > ...
> > out:
> > /* firmware should be released for other channel use */
> > release_firmware(chip->fw);
> > }
> >
> >
> > Does that match the "custom fallback" definition?
>
> Refer to the documentation I supplied, and also to the grammar rule, in
> particular the patch "firmware: add SmPL report for custom fallback mechanism",
> it captures the SmPL form for the custom fallback mechanism as:
I don't much care what the rule says. If you believe the code is
buggy, submit a patch.
--
(english) http://www.livejournal.com/~pavelmachek
(cesky, pictures) http://atrey.karlin.mff.cuni.cz/~pavel/picture/horses/blog.html
[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 181 bytes --]
^ permalink raw reply [flat|nested] 43+ messages in thread* Re: [PATCH 5/5] firmware: add DECLARE_FW_CUSTOM_FALLBACK() annotation
2016-12-16 9:29 ` Pavel Machek
@ 2016-12-16 9:59 ` Luis R. Rodriguez
2016-12-16 10:14 ` Pavel Machek
0 siblings, 1 reply; 43+ messages in thread
From: Luis R. Rodriguez @ 2016-12-16 9:59 UTC (permalink / raw)
To: Pavel Machek
Cc: Luis R. Rodriguez, gregkh, ming.lei, daniel.wagner, teg, mchehab,
zajec5, linux-kernel, markivx, stephen.boyd, broonie, zohar,
tiwai, johannes, chunkeey, hauke, jwboyer, dmitry.torokhov,
dwmw2, jslaby, torvalds, luto, fengguang.wu, rpurdie,
j.anaszewski, Abhay_Salunke, Julia.Lawall, Gilles.Muller,
nicolas.palix, dhowells, bjorn.andersson, arend.vanspriel, kvalo,
linux-leds
On Fri, Dec 16, 2016 at 10:29:20AM +0100, Pavel Machek wrote:
> On Fri 2016-12-16 10:22:41, Luis R. Rodriguez wrote:
> > On Tue, Dec 13, 2016 at 08:04:29PM +0100, Pavel Machek wrote:
> > > Hi!
> > >
> > > > 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.
> > >
> > > > --- 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;
> > >
> > > The driver does:
> > >
> > > static void lp55xx_firmware_loaded(const struct firmware *fw, void
> > > *context)
> > > {
> > > struct lp55xx_chip *chip = context;
> > > struct device *dev = &chip->cl->dev;
> > > enum lp55xx_engine_index idx =
> > > chip->engine_idx;
> > >
> > > if (!fw) {
> > > dev_err(dev, "firmware request failed\n");
> > > goto out;
> > > }
> > > ...
> > > out:
> > > /* firmware should be released for other channel use */
> > > release_firmware(chip->fw);
> > > }
> > >
> > >
> > > Does that match the "custom fallback" definition?
> >
> > Refer to the documentation I supplied, and also to the grammar rule, in
> > particular the patch "firmware: add SmPL report for custom fallback mechanism",
> > it captures the SmPL form for the custom fallback mechanism as:
>
> I don't much care what the rule says. If you believe the code is
> buggy, submit a patch.
Huh? No, its an old API and valid uses are scarce. The point is to avoid folks
adding yet other users by mistake by using grammar to help white-list actual
valid users.
Luis
^ permalink raw reply [flat|nested] 43+ messages in thread* Re: [PATCH 5/5] firmware: add DECLARE_FW_CUSTOM_FALLBACK() annotation
2016-12-16 9:59 ` Luis R. Rodriguez
@ 2016-12-16 10:14 ` Pavel Machek
2016-12-16 10:56 ` Luis R. Rodriguez
0 siblings, 1 reply; 43+ messages in thread
From: Pavel Machek @ 2016-12-16 10:14 UTC (permalink / raw)
To: Luis R. Rodriguez
Cc: gregkh, ming.lei, daniel.wagner, teg, mchehab, zajec5,
linux-kernel, markivx, stephen.boyd, broonie, zohar, tiwai,
johannes, chunkeey, hauke, jwboyer, dmitry.torokhov, dwmw2,
jslaby, torvalds, luto, fengguang.wu, rpurdie, j.anaszewski,
Abhay_Salunke, Julia.Lawall, Gilles.Muller, nicolas.palix,
dhowells, bjorn.andersson, arend.vanspriel, kvalo, linux-leds
[-- Attachment #1: Type: text/plain, Size: 2838 bytes --]
On Fri 2016-12-16 10:59:06, Luis R. Rodriguez wrote:
> On Fri, Dec 16, 2016 at 10:29:20AM +0100, Pavel Machek wrote:
> > On Fri 2016-12-16 10:22:41, Luis R. Rodriguez wrote:
> > > On Tue, Dec 13, 2016 at 08:04:29PM +0100, Pavel Machek wrote:
> > > > Hi!
> > > >
> > > > > 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.
> > > >
> > > > > --- 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;
> > > >
> > > > The driver does:
> > > >
> > > > static void lp55xx_firmware_loaded(const struct firmware *fw, void
> > > > *context)
> > > > {
> > > > struct lp55xx_chip *chip = context;
> > > > struct device *dev = &chip->cl->dev;
> > > > enum lp55xx_engine_index idx =
> > > > chip->engine_idx;
> > > >
> > > > if (!fw) {
> > > > dev_err(dev, "firmware request failed\n");
> > > > goto out;
> > > > }
> > > > ...
> > > > out:
> > > > /* firmware should be released for other channel use */
> > > > release_firmware(chip->fw);
> > > > }
> > > >
> > > >
> > > > Does that match the "custom fallback" definition?
> > >
> > > Refer to the documentation I supplied, and also to the grammar rule, in
> > > particular the patch "firmware: add SmPL report for custom fallback mechanism",
> > > it captures the SmPL form for the custom fallback mechanism as:
> >
> > I don't much care what the rule says. If you believe the code is
> > buggy, submit a patch.
>
> Huh? No, its an old API and valid uses are scarce. The point is to avoid folks
> adding yet other users by mistake by using grammar to help white-list actual
> valid users.
Well, I was asking if the above snipped looks like valid use. Because
AFAICT, the "custom fallback" is just dev_err(), see above. Coccinelle
rules don't help me...
Pavel
--
(english) http://www.livejournal.com/~pavelmachek
(cesky, pictures) http://atrey.karlin.mff.cuni.cz/~pavel/picture/horses/blog.html
[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 181 bytes --]
^ permalink raw reply [flat|nested] 43+ messages in thread* Re: [PATCH 5/5] firmware: add DECLARE_FW_CUSTOM_FALLBACK() annotation
2016-12-16 10:14 ` Pavel Machek
@ 2016-12-16 10:56 ` Luis R. Rodriguez
2016-12-16 11:27 ` Pavel Machek
0 siblings, 1 reply; 43+ messages in thread
From: Luis R. Rodriguez @ 2016-12-16 10:56 UTC (permalink / raw)
To: Pavel Machek
Cc: Luis R. Rodriguez, gregkh, ming.lei, daniel.wagner, teg, mchehab,
zajec5, linux-kernel, markivx, stephen.boyd, broonie, zohar,
tiwai, johannes, chunkeey, hauke, jwboyer, dmitry.torokhov,
dwmw2, jslaby, torvalds, luto, fengguang.wu, rpurdie,
j.anaszewski, Abhay_Salunke, Julia.Lawall, Gilles.Muller,
nicolas.palix, dhowells, bjorn.andersson, arend.vanspriel, kvalo,
linux-leds
On Fri, Dec 16, 2016 at 11:14:05AM +0100, Pavel Machek wrote:
>
> Well, I was asking if the above snipped looks like valid use. Because
> AFAICT, the "custom fallback" is just dev_err(), see above. Coccinelle
> rules don't help me...
Its not. Its when you ask for no uevent. Only 2 drivers do this.
Luis
^ permalink raw reply [flat|nested] 43+ messages in thread
* Re: [PATCH 5/5] firmware: add DECLARE_FW_CUSTOM_FALLBACK() annotation
2016-12-16 10:56 ` Luis R. Rodriguez
@ 2016-12-16 11:27 ` Pavel Machek
2016-12-16 15:19 ` Luis R. Rodriguez
2016-12-16 16:10 ` Luis R. Rodriguez
0 siblings, 2 replies; 43+ messages in thread
From: Pavel Machek @ 2016-12-16 11:27 UTC (permalink / raw)
To: Luis R. Rodriguez
Cc: gregkh, ming.lei, daniel.wagner, teg, mchehab, zajec5,
linux-kernel, markivx, stephen.boyd, broonie, zohar, tiwai,
johannes, chunkeey, hauke, jwboyer, dmitry.torokhov, dwmw2,
jslaby, torvalds, luto, fengguang.wu, rpurdie, j.anaszewski,
Abhay_Salunke, Julia.Lawall, Gilles.Muller, nicolas.palix,
dhowells, bjorn.andersson, arend.vanspriel, kvalo, linux-leds
[-- Attachment #1: Type: text/plain, Size: 640 bytes --]
On Fri 2016-12-16 11:56:48, Luis R. Rodriguez wrote:
> On Fri, Dec 16, 2016 at 11:14:05AM +0100, Pavel Machek wrote:
> >
> > Well, I was asking if the above snipped looks like valid use. Because
> > AFAICT, the "custom fallback" is just dev_err(), see above. Coccinelle
> > rules don't help me...
>
> Its not. Its when you ask for no uevent. Only 2 drivers do this.
That was one of two you listed. If that is not valid use, perhaps it
should be removed, not annotated?
Pavel
--
(english) http://www.livejournal.com/~pavelmachek
(cesky, pictures) http://atrey.karlin.mff.cuni.cz/~pavel/picture/horses/blog.html
[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 181 bytes --]
^ permalink raw reply [flat|nested] 43+ messages in thread
* Re: [PATCH 5/5] firmware: add DECLARE_FW_CUSTOM_FALLBACK() annotation
2016-12-16 11:27 ` Pavel Machek
@ 2016-12-16 15:19 ` Luis R. Rodriguez
2016-12-16 16:10 ` Luis R. Rodriguez
1 sibling, 0 replies; 43+ messages in thread
From: Luis R. Rodriguez @ 2016-12-16 15:19 UTC (permalink / raw)
To: Pavel Machek
Cc: Greg Kroah-Hartman, Ming Lei, Daniel Wagner, Tom Gundersen,
Mauro Carvalho Chehab, Rafał Miłecki, linux-kernel,
Vikram Mulukutla, Stephen Boyd, Mark Brown, Mimi Zohar,
Takashi Iwai, Johannes Berg, Christian Lamparter, Hauke Mehrtens,
Josh Boyer, Dmitry Torokhov, David Woodhouse, Jiri Slaby,
Linus Torvalds, Andy Lutomirski, Fengguang Wu, Richard Purdie,
Jacek Anaszewski, Abhay Salunke, Julia Lawall, Gilles Muller,
Nicolas Palix, David Howells, Bjorn Andersson, Arend Van Spriel,
Kalle Valo, linux-leds
On Fri, Dec 16, 2016 at 5:27 AM, Pavel Machek <pavel@ucw.cz> wrote:
> On Fri 2016-12-16 11:56:48, Luis R. Rodriguez wrote:
>> On Fri, Dec 16, 2016 at 11:14:05AM +0100, Pavel Machek wrote:
>> >
>> > Well, I was asking if the above snipped looks like valid use. Because
>> > AFAICT, the "custom fallback" is just dev_err(), see above. Coccinelle
>> > rules don't help me...
>>
>> Its not. Its when you ask for no uevent. Only 2 drivers do this.
>
> That was one of two you listed. If that is not valid use, perhaps it
> should be removed, not annotated?
Pavel, the annotation was added on top of:
static int lp55xx_request_firmware(struct lp55xx_chip *chip)
{
const char *name = chip->cl->name;
struct device *dev = &chip->cl->dev;
return request_firmware_nowait(THIS_MODULE, false, name, dev,
GFP_KERNEL, chip, lp55xx_firmware_loaded);
}
Note the second argument is false. This matches the grammar and the
definition for a custom fallback mechanism since uevents are not used.
What am I missing?
Luis
^ permalink raw reply [flat|nested] 43+ messages in thread* Re: [PATCH 5/5] firmware: add DECLARE_FW_CUSTOM_FALLBACK() annotation
2016-12-16 11:27 ` Pavel Machek
2016-12-16 15:19 ` Luis R. Rodriguez
@ 2016-12-16 16:10 ` Luis R. Rodriguez
2016-12-16 16:14 ` Luis R. Rodriguez
1 sibling, 1 reply; 43+ messages in thread
From: Luis R. Rodriguez @ 2016-12-16 16:10 UTC (permalink / raw)
To: Pavel Machek, Milo Kim
Cc: Luis R. Rodriguez, gregkh, ming.lei, daniel.wagner, teg, mchehab,
zajec5, linux-kernel, markivx, stephen.boyd, broonie, zohar,
tiwai, johannes, chunkeey, hauke, jwboyer, dmitry.torokhov,
dwmw2, jslaby, torvalds, luto, fengguang.wu, rpurdie,
j.anaszewski, Abhay_Salunke, Julia.Lawall, Gilles.Muller,
nicolas.palix, dhowells, bjorn.andersson, arend.vanspriel, kvalo,
linux-leds
On Fri, Dec 16, 2016 at 12:27:00PM +0100, Pavel Machek wrote:
> On Fri 2016-12-16 11:56:48, Luis R. Rodriguez wrote:
> > On Fri, Dec 16, 2016 at 11:14:05AM +0100, Pavel Machek wrote:
> > >
> > > Well, I was asking if the above snipped looks like valid use. Because
> > > AFAICT, the "custom fallback" is just dev_err(), see above. Coccinelle
> > > rules don't help me...
> >
> > Its not. Its when you ask for no uevent. Only 2 drivers do this.
>
> That was one of two you listed. If that is not valid use, perhaps it
> should be removed, not annotated?
Ah, well Milo Kim replied and described that the custom fallback is used as to
help load LED effect manually, and suggested a sysfs interface is more ideal [0]. I
agree however its also may be too late, and it depends how wide spread this "userspace"
that relies on this is, we just can't break it. Granted the custom fallback
mechanism was broken since v4.0 (see the fix "firmware: fix usermode helper
fallback loading") so one may argue no one seems to care...
So this is a judgement call, and the declaration is to point to documentation
to white list uses, as terrible as this one is userspace exists for it. but
more importantly to also help the SmPL grammar report to avoid reporting
already vetted cases. The alarm / cases for the 2 drivers has been issueed,
moving forward the lack of declaration with the custom fallback should trigger
a rant through 0-day so we don't run into the same stupid situation.
[0] https://marc.info/?l=linux-kernel&m=148168024112445
Luis
^ permalink raw reply [flat|nested] 43+ messages in thread
* Re: [PATCH 5/5] firmware: add DECLARE_FW_CUSTOM_FALLBACK() annotation
2016-12-16 16:10 ` Luis R. Rodriguez
@ 2016-12-16 16:14 ` Luis R. Rodriguez
2016-12-18 3:50 ` Milo Kim
0 siblings, 1 reply; 43+ messages in thread
From: Luis R. Rodriguez @ 2016-12-16 16:14 UTC (permalink / raw)
To: Luis R. Rodriguez
Cc: Pavel Machek, Milo Kim, gregkh, ming.lei, daniel.wagner, teg,
mchehab, zajec5, linux-kernel, markivx, stephen.boyd, broonie,
zohar, tiwai, johannes, chunkeey, hauke, jwboyer,
dmitry.torokhov, dwmw2, jslaby, torvalds, luto, fengguang.wu,
rpurdie, j.anaszewski, Abhay_Salunke, Julia.Lawall,
Gilles.Muller, nicolas.palix, dhowells, bjorn.andersson,
arend.vanspriel, kvalo, linux-leds
On Fri, Dec 16, 2016 at 05:10:18PM +0100, Luis R. Rodriguez wrote:
> Ah, well Milo Kim replied and described that the custom fallback is used as to
> help load LED effect manually, and suggested a sysfs interface is more ideal [0]. I
> agree however its also may be too late, and it depends how wide spread this "userspace"
> that relies on this is, we just can't break it. Granted the custom fallback
> mechanism was broken since v4.0 (see the fix "firmware: fix usermode helper
> fallback loading") so one may argue no one seems to care...
>
> So this is a judgement call, and the declaration is to point to documentation
> to white list uses, as terrible as this one is userspace exists for it. but
> more importantly to also help the SmPL grammar report to avoid reporting
> already vetted cases. The alarm / cases for the 2 drivers has been issueed,
> moving forward the lack of declaration with the custom fallback should trigger
> a rant through 0-day so we don't run into the same stupid situation.
>
> [0] https://marc.info/?l=linux-kernel&m=148168024112445
Milo if sysfs is used can't the old userspace be mapped to use the new
sysfs interface through a wrapper of some sort ? What exactly would be
needed to ensure old userspace will not break? Why has no one cried
after the v4.0 custom fallback mechanism breaking ? How wide spread is
this custom userspace ?
Luis
^ permalink raw reply [flat|nested] 43+ messages in thread
* Re: [PATCH 5/5] firmware: add DECLARE_FW_CUSTOM_FALLBACK() annotation
2016-12-16 16:14 ` Luis R. Rodriguez
@ 2016-12-18 3:50 ` Milo Kim
2016-12-19 20:08 ` Pavel Machek
0 siblings, 1 reply; 43+ messages in thread
From: Milo Kim @ 2016-12-18 3:50 UTC (permalink / raw)
To: Luis R. Rodriguez
Cc: Pavel Machek, gregkh, ming.lei, daniel.wagner, teg, mchehab,
zajec5, linux-kernel, markivx, stephen.boyd, broonie, zohar,
tiwai, johannes, chunkeey, hauke, jwboyer, dmitry.torokhov,
dwmw2, jslaby, torvalds, luto, fengguang.wu, rpurdie,
j.anaszewski, Abhay_Salunke, Julia.Lawall, Gilles.Muller,
nicolas.palix, dhowells, bjorn.andersson, arend.vanspriel, kvalo,
linux-leds
Hi Luis,
On 12/17/2016 01:14 AM, Luis R. Rodriguez wrote:
> Milo if sysfs is used can't the old userspace be mapped to use the new
> sysfs interface through a wrapper of some sort ? What exactly would be
> needed to ensure old userspace will not break?
LP5521 and LP5523 have two ways to load hex code from the userspace -
the sysfs and firmware I/F. So user program supports both interfaces.
Even if the firmware I/F is not available, user can still run LED effect
through the sysfs.
However, LP5562 and LP8501 support only single way which is the firmware
I/F. So user-space program for LP5562/8501 should be modified if lp55xx
removes the interface. My idea is
Phase 1)
- create sysfs in LP5562 and LP8501
- use new sysfs inside the firmware I/F loading callback
- mark the firmware callback as a deprecated interface
Phase 2)
- remove the firmware I/F after all user program fixes the interface
(but the problem is how can we get to know when this is done?)
> Why has no one cried
> after the v4.0 custom fallback mechanism breaking ?
Well, I don't know the reason exactly but my guess is they maybe still
using old kernel.
> How wide spread is this custom userspace ?
Device manufactures in Asia & North America requested lp55xx drivers,
but I don't know how many vendors uses the firmware I/F. Some vendors
embeds the binary code inside the driver instead of using user-program.
I understood it's a kind of troublesome work in terms of the
maintenance. Sorry for that. I hope we have a consensus to resolve it.
Thanks!
Best regards,
Milo
^ permalink raw reply [flat|nested] 43+ messages in thread* Re: [PATCH 5/5] firmware: add DECLARE_FW_CUSTOM_FALLBACK() annotation
2016-12-18 3:50 ` Milo Kim
@ 2016-12-19 20:08 ` Pavel Machek
2016-12-19 20:46 ` Jacek Anaszewski
0 siblings, 1 reply; 43+ messages in thread
From: Pavel Machek @ 2016-12-19 20:08 UTC (permalink / raw)
To: Milo Kim
Cc: Luis R. Rodriguez, gregkh, ming.lei, daniel.wagner, teg, mchehab,
zajec5, linux-kernel, markivx, stephen.boyd, broonie, zohar,
tiwai, johannes, chunkeey, hauke, jwboyer, dmitry.torokhov,
dwmw2, jslaby, torvalds, luto, fengguang.wu, rpurdie,
j.anaszewski, Abhay_Salunke, Julia.Lawall, Gilles.Muller,
nicolas.palix, dhowells, bjorn.andersson, arend.vanspriel, kvalo,
linux-leds
[-- Attachment #1: Type: text/plain, Size: 2139 bytes --]
Hi!
> On 12/17/2016 01:14 AM, Luis R. Rodriguez wrote:
> >Milo if sysfs is used can't the old userspace be mapped to use the new
> >sysfs interface through a wrapper of some sort ? What exactly would be
> >needed to ensure old userspace will not break?
>
> LP5521 and LP5523 have two ways to load hex code from the userspace - the
> sysfs and firmware I/F. So user program supports both interfaces. Even if
> the firmware I/F is not available, user can still run LED effect through the
> sysfs.
>
> However, LP5562 and LP8501 support only single way which is the firmware
> I/F. So user-space program for LP5562/8501 should be modified if lp55xx
> removes the interface. My idea is
Actually... it would be good to have some reasonable interface for RGB
LEDs. This way, we need separate "firmware" for each LED
controller. It would be good to have common format for LED effects.
> Phase 1)
> - create sysfs in LP5562 and LP8501
> - use new sysfs inside the firmware I/F loading callback
> - mark the firmware callback as a deprecated interface
Phase 1a)
stick WARN_ON() in the firmware callback.
> Phase 2)
> - remove the firmware I/F after all user program fixes the interface
> (but the problem is how can we get to know when this is done?)
>
> > Why has no one cried
> > after the v4.0 custom fallback mechanism breaking ?
>
> Well, I don't know the reason exactly but my guess is they maybe still using
> old kernel.
>
> > How wide spread is this custom userspace ?
>
> Device manufactures in Asia & North America requested lp55xx drivers, but I
> don't know how many vendors uses the firmware I/F. Some vendors embeds the
> binary code inside the driver instead of using user-program.
Nokia N900 uses lp55xx, and I have custom scripts interfacing sysfs.
Maemo uses the LEDs, too, but maemo is not open source.
So no, I don't think there's anything important that could be broken.
Best regards,
Pavel
--
(english) http://www.livejournal.com/~pavelmachek
(cesky, pictures) http://atrey.karlin.mff.cuni.cz/~pavel/picture/horses/blog.html
[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 181 bytes --]
^ permalink raw reply [flat|nested] 43+ messages in thread
* Re: [PATCH 5/5] firmware: add DECLARE_FW_CUSTOM_FALLBACK() annotation
2016-12-19 20:08 ` Pavel Machek
@ 2016-12-19 20:46 ` Jacek Anaszewski
2016-12-21 18:49 ` Pavel Machek
0 siblings, 1 reply; 43+ messages in thread
From: Jacek Anaszewski @ 2016-12-19 20:46 UTC (permalink / raw)
To: Pavel Machek, Milo Kim
Cc: Luis R. Rodriguez, gregkh, ming.lei, daniel.wagner, teg, mchehab,
zajec5, linux-kernel, markivx, stephen.boyd, broonie, zohar,
tiwai, johannes, chunkeey, hauke, jwboyer, dmitry.torokhov,
dwmw2, jslaby, torvalds, luto, fengguang.wu, rpurdie,
j.anaszewski, Abhay_Salunke, Julia.Lawall, Gilles.Muller,
nicolas.palix, dhowells, bjorn.andersson, arend.vanspriel, kvalo,
linux-leds
On 12/19/2016 09:08 PM, Pavel Machek wrote:
> Hi!
>
>> On 12/17/2016 01:14 AM, Luis R. Rodriguez wrote:
>>> Milo if sysfs is used can't the old userspace be mapped to use the new
>>> sysfs interface through a wrapper of some sort ? What exactly would be
>>> needed to ensure old userspace will not break?
>>
>> LP5521 and LP5523 have two ways to load hex code from the userspace - the
>> sysfs and firmware I/F. So user program supports both interfaces. Even if
>> the firmware I/F is not available, user can still run LED effect through the
>> sysfs.
>>
>> However, LP5562 and LP8501 support only single way which is the firmware
>> I/F. So user-space program for LP5562/8501 should be modified if lp55xx
>> removes the interface. My idea is
>
> Actually... it would be good to have some reasonable interface for RGB
> LEDs. This way, we need separate "firmware" for each LED
> controller. It would be good to have common format for LED effects.
We still haven't tried trigger approach discussed over half a year ago.
If we used firmware approach we would still have to overcome the problem
of defining the LED class drivers affected by the firmware program.
>> Phase 1)
>> - create sysfs in LP5562 and LP8501
>> - use new sysfs inside the firmware I/F loading callback
>> - mark the firmware callback as a deprecated interface
>
> Phase 1a)
>
> stick WARN_ON() in the firmware callback.
>
>> Phase 2)
>> - remove the firmware I/F after all user program fixes the interface
>> (but the problem is how can we get to know when this is done?)
>>
>>> Why has no one cried
>>> after the v4.0 custom fallback mechanism breaking ?
>>
>> Well, I don't know the reason exactly but my guess is they maybe still using
>> old kernel.
>>
>>> How wide spread is this custom userspace ?
>>
>> Device manufactures in Asia & North America requested lp55xx drivers, but I
>> don't know how many vendors uses the firmware I/F. Some vendors embeds the
>> binary code inside the driver instead of using user-program.
>
> Nokia N900 uses lp55xx, and I have custom scripts interfacing sysfs.
>
> Maemo uses the LEDs, too, but maemo is not open source.
>
> So no, I don't think there's anything important that could be broken.
We can't guarantee that. Is there any problem in just using the
currently introduced DECLARE_FW_CUSTOM_FALLBACK() in
drivers/leds/leds-lp55xx-common.c?
--
Best regards,
Jacek Anaszewski
^ permalink raw reply [flat|nested] 43+ messages in thread
* Re: [PATCH 5/5] firmware: add DECLARE_FW_CUSTOM_FALLBACK() annotation
2016-12-19 20:46 ` Jacek Anaszewski
@ 2016-12-21 18:49 ` Pavel Machek
2016-12-21 20:33 ` Jacek Anaszewski
0 siblings, 1 reply; 43+ messages in thread
From: Pavel Machek @ 2016-12-21 18:49 UTC (permalink / raw)
To: Jacek Anaszewski
Cc: Milo Kim, Luis R. Rodriguez, gregkh, ming.lei, daniel.wagner,
teg, mchehab, zajec5, linux-kernel, markivx, stephen.boyd,
broonie, zohar, tiwai, johannes, chunkeey, hauke, jwboyer,
dmitry.torokhov, dwmw2, jslaby, torvalds, luto, fengguang.wu,
rpurdie, j.anaszewski, Abhay_Salunke, Julia.Lawall,
Gilles.Muller, nicolas.palix, dhowells, bjorn.andersson,
arend.vanspriel, kvalo, linux-leds
[-- Attachment #1: Type: text/plain, Size: 2168 bytes --]
Hi!
> >>> Milo if sysfs is used can't the old userspace be mapped to use the new
> >>> sysfs interface through a wrapper of some sort ? What exactly would be
> >>> needed to ensure old userspace will not break?
> >>
> >> LP5521 and LP5523 have two ways to load hex code from the userspace - the
> >> sysfs and firmware I/F. So user program supports both interfaces. Even if
> >> the firmware I/F is not available, user can still run LED effect through the
> >> sysfs.
> >>
> >> However, LP5562 and LP8501 support only single way which is the firmware
> >> I/F. So user-space program for LP5562/8501 should be modified if lp55xx
> >> removes the interface. My idea is
> >
> > Actually... it would be good to have some reasonable interface for RGB
> > LEDs. This way, we need separate "firmware" for each LED
> > controller. It would be good to have common format for LED effects.
>
> We still haven't tried trigger approach discussed over half a year ago.
> If we used firmware approach we would still have to overcome the problem
> of defining the LED class drivers affected by the firmware program.
The firmware approach is in the tree today :-(.
> >> Device manufactures in Asia & North America requested lp55xx drivers, but I
> >> don't know how many vendors uses the firmware I/F. Some vendors embeds the
> >> binary code inside the driver instead of using user-program.
> >
> > Nokia N900 uses lp55xx, and I have custom scripts interfacing sysfs.
> >
> > Maemo uses the LEDs, too, but maemo is not open source.
> >
> > So no, I don't think there's anything important that could be broken.
>
> We can't guarantee that. Is there any problem in just using the
> currently introduced DECLARE_FW_CUSTOM_FALLBACK() in
> drivers/leds/leds-lp55xx-common.c?
Well, it would be good to get rid of the custom fallback
functionality. And no, we don't need to "guarantee" that. Removing
obscure functionality noone uses is far game... providing noone
complains ;-).
Pavel
--
(english) http://www.livejournal.com/~pavelmachek
(cesky, pictures) http://atrey.karlin.mff.cuni.cz/~pavel/picture/horses/blog.html
[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 181 bytes --]
^ permalink raw reply [flat|nested] 43+ messages in thread
* Re: [PATCH 5/5] firmware: add DECLARE_FW_CUSTOM_FALLBACK() annotation
2016-12-21 18:49 ` Pavel Machek
@ 2016-12-21 20:33 ` Jacek Anaszewski
0 siblings, 0 replies; 43+ messages in thread
From: Jacek Anaszewski @ 2016-12-21 20:33 UTC (permalink / raw)
To: Pavel Machek
Cc: Milo Kim, Luis R. Rodriguez, gregkh, ming.lei, daniel.wagner,
teg, mchehab, zajec5, linux-kernel, markivx, stephen.boyd,
broonie, zohar, tiwai, johannes, chunkeey, hauke, jwboyer,
dmitry.torokhov, dwmw2, jslaby, torvalds, luto, fengguang.wu,
rpurdie, j.anaszewski, Abhay_Salunke, Julia.Lawall,
Gilles.Muller, nicolas.palix, dhowells, bjorn.andersson,
arend.vanspriel, kvalo, linux-leds
Hi,
On 12/21/2016 07:49 PM, Pavel Machek wrote:
> Hi!
>
>>>>> Milo if sysfs is used can't the old userspace be mapped to use the new
>>>>> sysfs interface through a wrapper of some sort ? What exactly would be
>>>>> needed to ensure old userspace will not break?
>>>>
>>>> LP5521 and LP5523 have two ways to load hex code from the userspace - the
>>>> sysfs and firmware I/F. So user program supports both interfaces. Even if
>>>> the firmware I/F is not available, user can still run LED effect through the
>>>> sysfs.
>>>>
>>>> However, LP5562 and LP8501 support only single way which is the firmware
>>>> I/F. So user-space program for LP5562/8501 should be modified if lp55xx
>>>> removes the interface. My idea is
>>>
>>> Actually... it would be good to have some reasonable interface for RGB
>>> LEDs. This way, we need separate "firmware" for each LED
>>> controller. It would be good to have common format for LED effects.
>>
>> We still haven't tried trigger approach discussed over half a year ago.
>> If we used firmware approach we would still have to overcome the problem
>> of defining the LED class drivers affected by the firmware program.
>
> The firmware approach is in the tree today :-(.
to RGB LEDs? What exactly do you have on mind?
>
>>>> Device manufactures in Asia & North America requested lp55xx drivers, but I
>>>> don't know how many vendors uses the firmware I/F. Some vendors embeds the
>>>> binary code inside the driver instead of using user-program.
>>>
>>> Nokia N900 uses lp55xx, and I have custom scripts interfacing sysfs.
>>>
>>> Maemo uses the LEDs, too, but maemo is not open source.
>>>
>>> So no, I don't think there's anything important that could be broken.
>>
>> We can't guarantee that. Is there any problem in just using the
>> currently introduced DECLARE_FW_CUSTOM_FALLBACK() in
>> drivers/leds/leds-lp55xx-common.c?
>
> Well, it would be good to get rid of the custom fallback
> functionality. And no, we don't need to "guarantee" that. Removing
> obscure functionality noone uses is far game... providing noone
> complains ;-).
As Milo explained:
> Why has no one cried
> after the v4.0 custom fallback mechanism breaking ?
"Well, I don't know the reason exactly but my guess is they maybe still
using old kernel."
and after that:
"Device manufactures in Asia & North America requested lp55xx drivers"
These should be sufficient arguments for us for keeping the API
unchanged. If the users decided to upgrade their kernel then they
would be surprised by the API change.
DECLARE_FW_CUSTOM_FALLBACK macro seems to have been designed for
handling exactly this type of cases.
--
Best regards,
Jacek Anaszewski
^ permalink raw reply [flat|nested] 43+ messages in thread
* Re: [PATCH 5/5] firmware: add DECLARE_FW_CUSTOM_FALLBACK() annotation
2016-12-13 3:08 ` [PATCH 5/5] firmware: add DECLARE_FW_CUSTOM_FALLBACK() annotation Luis R. Rodriguez
2016-12-13 19:04 ` Pavel Machek
@ 2016-12-15 9:32 ` Jacek Anaszewski
2016-12-16 9:26 ` Luis R. Rodriguez
1 sibling, 1 reply; 43+ messages in thread
From: Jacek Anaszewski @ 2016-12-15 9:32 UTC (permalink / raw)
To: Luis R. Rodriguez, gregkh, ming.lei
Cc: daniel.wagner, teg, mchehab, zajec5, linux-kernel, markivx,
stephen.boyd, broonie, zohar, tiwai, johannes, chunkeey, hauke,
jwboyer, dmitry.torokhov, dwmw2, jslaby, torvalds, luto,
fengguang.wu, rpurdie, j.anaszewski, Abhay_Salunke, Julia.Lawall,
Gilles.Muller, nicolas.palix, dhowells, bjorn.andersson,
arend.vanspriel, kvalo, linux-leds, woogyom.kim
Hi Luis,
Thanks for the patch.
On 12/13/2016 04:08 AM, 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>
> 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 955c11d6ff9d..b51673e40439 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::
double colon at the end of line
>
> $ 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)
> {
For this LED class driver:
Acked-by: Jacek Anaszewski <j.anaszewski@samsung.com>
> 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;
> @@
>
>
--
Best regards,
Jacek Anaszewski
^ permalink raw reply [flat|nested] 43+ messages in thread* Re: [PATCH 5/5] firmware: add DECLARE_FW_CUSTOM_FALLBACK() annotation
2016-12-15 9:32 ` Jacek Anaszewski
@ 2016-12-16 9:26 ` Luis R. Rodriguez
0 siblings, 0 replies; 43+ messages in thread
From: Luis R. Rodriguez @ 2016-12-16 9:26 UTC (permalink / raw)
To: Jacek Anaszewski
Cc: Luis R. Rodriguez, gregkh, ming.lei, daniel.wagner, teg, mchehab,
zajec5, linux-kernel, markivx, stephen.boyd, broonie, zohar,
tiwai, johannes, chunkeey, hauke, jwboyer, dmitry.torokhov,
dwmw2, jslaby, torvalds, luto, fengguang.wu, rpurdie,
j.anaszewski, Abhay_Salunke, Julia.Lawall, Gilles.Muller,
nicolas.palix, dhowells, bjorn.andersson, arend.vanspriel, kvalo,
linux-leds, woogyom.kim
On Thu, Dec 15, 2016 at 10:32:12AM +0100, Jacek Anaszewski wrote:
> > diff --git a/Documentation/driver-api/firmware/fallback-mechanisms.rst b/Documentation/driver-api/firmware/fallback-mechanisms.rst
> > index 955c11d6ff9d..b51673e40439 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::
>
> double colon at the end of line
That is on purpose for rst files, for use with the new trendy hipster
Sphinx documentation format.
> >
> > $ export COCCI=scripts/coccinelle/api/request_firmware-avoid-init-probe-init.cocci
> > $ make coccicheck MODE=report
It will kind of blockquote the above.
> > 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)
> > {
>
> For this LED class driver:
>
> Acked-by: Jacek Anaszewski <j.anaszewski@samsung.com>
Thanks, amended!
Luis
^ permalink raw reply [flat|nested] 43+ messages in thread
* Re: [PATCH 0/5] firmware: doc revamp
2016-12-13 3:08 [PATCH 0/5] firmware: doc revamp Luis R. Rodriguez
` (4 preceding siblings ...)
2016-12-13 3:08 ` [PATCH 5/5] firmware: add DECLARE_FW_CUSTOM_FALLBACK() annotation Luis R. Rodriguez
@ 2016-12-13 12:58 ` Daniel Wagner
5 siblings, 0 replies; 43+ messages in thread
From: Daniel Wagner @ 2016-12-13 12:58 UTC (permalink / raw)
To: Luis R. Rodriguez, gregkh, ming.lei
Cc: teg, mchehab, zajec5, linux-kernel, markivx, stephen.boyd,
broonie, zohar, tiwai, johannes, chunkeey, hauke, jwboyer,
dmitry.torokhov, dwmw2, jslaby, torvalds, luto, fengguang.wu,
rpurdie, j.anaszewski, Abhay_Salunke, Julia.Lawall,
Gilles.Muller, nicolas.palix, dhowells, bjorn.andersson,
arend.vanspriel, kvalo, Daniel Wagner
Hi Luis,
On 12/13/2016 04:08 AM, Luis R. Rodriguez wrote:
> You may notice I've dropped the SmPL patches which complain on use of the
> API on init and probe -- although valid the context was off given the only
> valid use case was if you don't use initramfs, and that's a corner case.
> Fortunatley Daniel Wagner and Tom Gundersen have come up with some ideas
> that should help correct these issues, so I've dropped that grammar patch.
My BMW email address will soon be shutdown. I guess best to use my
private instead: wagi@monom.org
cheers,
daniel
^ permalink raw reply [flat|nested] 43+ messages in thread