linux-kernel - Re: [PATCH] firmware: cleanup - group and document up private firmware parameters

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Date:   Fri, 15 Sep 2017 10:30:46 +0200
From:   Martin Fuzzey <mfuzzey@...keon.com>
To:     "Luis R. Rodriguez" <mcgrof@...nel.org>, gregkh@...uxfoundation.org
Cc:     wagi@...om.org, yi1.li@...ux.intel.com, takahiro.akashi@...aro.org,
        bjorn.andersson@...aro.org, luto@...nel.org, ebiederm@...ssion.com,
        dmitry.torokhov@...il.com, arend.vanspriel@...adcom.com,
        dwmw2@...radead.org, rjw@...ysocki.net, atull@...nel.org,
        moritz.fischer@...us.com, pmladek@...e.com,
        johannes.berg@...el.com, emmanuel.grumbach@...el.com,
        luciano.coelho@...el.com, kvalo@...eaurora.org,
        torvalds@...ux-foundation.org, keescook@...omium.org,
        dhowells@...hat.com, pjones@...hat.com, hdegoede@...hat.com,
        alan@...ux.intel.com, tytso@....edu, dave@...olabs.net,
        mawilcox@...rosoft.com, tglx@...utronix.de, peterz@...radead.org,
        jakub.kicinski@...ronome.com, nbroeking@...com,
        jewalt@...innovations.com, linux-fsdevel@...r.kernel.org,
        linux-kernel@...r.kernel.org
Subject: Re: [PATCH] firmware: cleanup - group and document up private
 firmware parameters

Hi Luis,

On 15/09/17 00:54, Luis R. Rodriguez wrote:
> The firmware API has a slew of private options available, which can
> sometimes be hard to understand. When new functionality is introduced
> we also tend to have modify a slew of internal helpers.
>
> Just stuff all common private requirements into its own data structure
> and move features into properly defined flags which can then be carefully
> documented. This:
>
>    o reduces the amount of changes we have make as we advance functionality
>    o helps remove the #ifdef mess we had created for private features
>
> The above benefits makes the code much easier to understand and maintain.

Yes I agree it is much cleaner that way.

A couple of nitpicks below.

> +/**
> + * enum fw_priv_reqs - private features only used internally
> + *
> + * @FW_PRIV_REQ_FALLBACK: specifies that the firmware request
> + *	will use a fallback mechanism if the kernel's direct filesystem
> + *	lookup failed to find the requested firmware. If the flag
> + *	%FW_PRIV_REQ_FALLBACK is set but the flag
> + *	%FW_PRIV_REQ_FALLBACK_UEVENT is not set, it means the caller
> + *	is relying on a custom fallback mechanism for firmwarwe lookup as a
> + *	fallback mechanism. The custom fallback mechanism is expected to find
> + *	any found firmware using the exposed sysfs interface of the
> + *	firmware_class.  Since the custom fallback mechanism is not compatible
> + *	with the internal caching mechanism for firmware lookups at resume,
> + *	caching will be disabled when the custom fallback mechanism is used.
> + * @FW_PRIV_REQ_FALLBACK_UEVENT: indicates that the fallback mechanism
> + *	this firmware request will rely on will be that of having the kernel
> + *	issue a uevent to userspace. Userspace in turn is expected to be
> + *	monitoring for uevents for the firmware_class and will use the
> + *	exposted sysfs interface to upload the firmware for the caller.
> + * @FW_PRIV_REQ_NO_CACHE: indicates that the firmware request
> + *	should not set up and use the internal caching mechanism to assist
> + *	drivers from fetching firmware at resume time after suspend.
> + * @FW_PRIV_REQ_OPTIONAL: if set it is not a hard requirement by the
> + *	caller that the file requested be present. An error will not be recorded
> + *	if the file is not found.
> + */
> +enum fw_priv_reqs {
> +	FW_PRIV_REQ_FALLBACK			= 1 << 0,
> +	FW_PRIV_REQ_FALLBACK_UEVENT		= 1 << 1,
> +	FW_PRIV_REQ_NO_CACHE			= 1 << 2,
> +	FW_PRIV_REQ_OPTIONAL			= 1 << 3,
> +};
> +

Why REQ ?
Looks more like a set of flags to me.
Wouldn't FW_PRIV_FLAG_XXX be better?

> +/**
> + * struct fw_priv_params - private firmware parameters
> + * @mode: mode of operation
> + * @priv_reqs: private set of &enum fw_priv_reqs, private requirements for
> + *	the firmware request
> + * @alloc_buf: buffer area allocated by the caller so we can place the
> + *	respective firmware
> + * @alloc_buf_size: size of the @alloc_buf
> + */
> +struct fw_priv_params {
> +	enum fw_api_mode mode;
> +	u64 priv_reqs;

Not sure the priv_ prefix in the priv_reqs is necessary since the whole 
structure is private.
I'd have named it just flags.


Regards,

Martin