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:   Sat, 11 Nov 2017 02:26:09 +0100
From:   "Luis R. Rodriguez" <mcgrof@...nel.org>
To:     Martin Fuzzey <mfuzzey@...keon.com>
Cc:     "Luis R. Rodriguez" <mcgrof@...nel.org>,
        gregkh@...uxfoundation.org, 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,
        Pali Rohár <pali.rohar@...il.com>,
        linux-fsdevel@...r.kernel.org, linux-kernel@...r.kernel.org
Subject: Re: [PATCH] firmware: cleanup - group and document up private
 firmware parameters

On Fri, Sep 15, 2017 at 10:30:46AM +0200, Martin Fuzzey wrote:
> On 15/09/17 00:54, Luis R. Rodriguez wrote:
> > 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?

Sure, its much better without anything so will just go with FW_PRIV_ as the
prefix.

> > +/**
> > + * 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.

Went with priv_flags.

Thanks for the feedback!

  Luis