Received-SPF: pass (google.com: best guess record for domain of linux-kernel-owner@vger.kernel.org designates 209.132.180.67 as permitted sender) client-ip=209.132.180.67;
Date:   Sat, 11 Nov 2017 02:26:09 +0100
From:   "Luis R. Rodriguez" <mcgrof@kernel.org>
To:     Martin Fuzzey <mfuzzey@parkeon.com>
Cc:     "Luis R. Rodriguez" <mcgrof@kernel.org>,
        gregkh@linuxfoundation.org, wagi@monom.org, yi1.li@linux.intel.com,
        takahiro.akashi@linaro.org, bjorn.andersson@linaro.org,
        luto@kernel.org, ebiederm@xmission.com, dmitry.torokhov@gmail.com,
        arend.vanspriel@broadcom.com, dwmw2@infradead.org,
        rjw@rjwysocki.net, atull@kernel.org, moritz.fischer@ettus.com,
        pmladek@suse.com, johannes.berg@intel.com,
        emmanuel.grumbach@intel.com, luciano.coelho@intel.com,
        kvalo@codeaurora.org, torvalds@linux-foundation.org,
        keescook@chromium.org, dhowells@redhat.com, pjones@redhat.com,
        hdegoede@redhat.com, alan@linux.intel.com, tytso@mit.edu,
        dave@stgolabs.net, mawilcox@microsoft.com, tglx@linutronix.de,
        peterz@infradead.org, jakub.kicinski@netronome.com,
        nbroeking@me.com, jewalt@lgsinnovations.com,
        Pali =?iso-8859-1?Q?Roh=E1r?= <pali.rohar@gmail.com>,
        linux-fsdevel@vger.kernel.org, linux-kernel@vger.kernel.org
Subject: Re: [PATCH] firmware: cleanup - group and document up private
 firmware parameters
Message-ID: <20171111012609.GY22894@wotan.suse.de>
References: <20170914225422.31034-1-mcgrof@kernel.org>
 <59BB8FB6.2040502@parkeon.com>
MIME-Version: 1.0
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
In-Reply-To: <59BB8FB6.2040502@parkeon.com>
User-Agent: Mutt/1.6.0 (2016-04-01)
Sender: linux-kernel-owner@vger.kernel.org
Precedence: bulk

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

From 1578892514428336720@xxx Mon Sep 18 15:40:39 +0000 2017
X-GM-THRID: 1578557473088358418
X-Gmail-Labels: Inbox,Category Forums