Re: [PATCH 3/9] firmware: add kernel-doc for enum fw_opt

[Luis R. Rodriguez]

On Tue, Apr 17, 2018 at 11:33:01AM -0400, Andres Rodriguez wrote:
> Some basic definitions for the FW_OPT_* values
> 
> Signed-off-by: Andres Rodriguez <andresx7@xxxxxxxxx>
> ---
>  drivers/base/firmware_loader/firmware.h | 11 +++++++++++
>  1 file changed, 11 insertions(+)
> 
> diff --git a/drivers/base/firmware_loader/firmware.h b/drivers/base/firmware_loader/firmware.h
> index 957396293b92..8ef23c334135 100644
> --- a/drivers/base/firmware_loader/firmware.h
> +++ b/drivers/base/firmware_loader/firmware.h
> @@ -10,6 +10,17 @@
>  
>  #include <generated/utsrelease.h>
>  
> +/**
> + * enum fw_opt - options to control firmware loading behaviour
> + *
> + * @FW_OPT_UEVENT: Enable the uevent fallback path.

No, that is not what this does. This sends a kobject uevent to userspace when the
fallback mechanism is used. So please use:

"Enables the fallback mechanism to send a kobject uevent when the firmware
is not found. Userspace is in charge to load the firmware using the sysfs
loading facility."

> + * @FW_OPT_NOWAIT: Executing inside an asynchronous firmware request.

Please use "Used to describe the firmware request is asynchronous.".

> + * @FW_OPT_USERHELPER: Enable the usermode fallback path.

The notion of "userhelper" only causes confusion since the kernel has its
own usermode helper (kernel/umh.c), and the only use case that the firmware loader
has for it is for when CONFIG_UEVENT_HELPER_PATH is set to call  out to
userspace helper for kobject uevents. In practice *no one* is setting  or using
this these days.

So I have been going on a small nomenclature crusade to clean this mess up bon
on kernel/umh.c and the firmware loader. This is why the entire fallback mechanism
is now stuffed into a file called fallback.c. I don't want to confuse people
further so please do not refer to "usermode" anymore for the fallback mechanism,
it is completely misleading. We should eventually just rename this flag to
fallback or something.

Therefore please be consistent with the documentation and use:

"Enable the fallback mechanism, in case the direct filesystem lookup
fails at finding the firmware. For details refer to fw_sysfs_fallback()."

Since fw_sysfs_fallback() is used in documentation and we don't want to clash
with other documentation names, then rename fw_sysfs_fallback() to use the
firmware_ prefix. That would be a separate patch.

So Hans -- same goes for your call which can be added after Andres' patch series.

> + * @FW_OPT_NO_WARN: Quiet, avoid printing warning messages.
> + * @FW_OPT_NOCACHE: Firmware caching will be disabled for this request.

Although we have good documentation about this on
Documentation/driver-api/firmware/firmware_cache.rst best to describe this
a bit more here.

Please use (modulo adjust lengths accordingly):

* @FW_OPT_NOCACHE: Disables firmware caching. Firmware caching is used to
	           cache the firmware upon suspend, so that upon resume
		   races against the firmware file lookup on storage is
		   avoided. Used for calls where the file may be too
                   big, or where the driver takes charge of its own firmware
		   caching mechanism.

Note that an example driver which does its own firmware caching mechanism is
iwlwifi, IIRC.

> + * @FW_OPT_NOFALLBACK: Disable the fallback path. Takes precedence over
> + *                     &FW_OPT_UEVENT and &FW_OPT_USERHELPER.

Please replace "fallback path" with "fallback mechanism" to be consistent
with the documentation.

  Luis

> + */
>  enum fw_opt {
>  	FW_OPT_UEVENT =         (1U << 0),
>  	FW_OPT_NOWAIT =         (1U << 1),
> -- 
> 2.14.1
> 
> 

-- 
Do not panic