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 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:   Mon, 18 Sep 2017 17:15:01 +0200
From:   Greg KH <gregkh@...uxfoundation.org>
To:     "Luis R. Rodriguez" <mcgrof@...nel.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,
        mfuzzey@...keon.com, 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

On Thu, Sep 14, 2017 at 03:54:22PM -0700, Luis R. Rodriguez wrote:
> +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,
> +};

checkpatch.pl didn't complain about a lack of using BIT()?


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

Agreed that this should not be "priv_reqs" but some other better name.

> +	void *alloc_buf;
> +	size_t alloc_buf_size;
> +};
> +
> +#define fw_req_param_sync(priv_params)					\
> +	(priv_params->mode == FW_API_SYNC)
> +#define fw_req_param_async(priv_params)					\
> +	(priv_params->mode == FW_API_ASYNC)
> +
> +#define fw_param_use_fallback(params)					\
> +	(!!((params)->priv_reqs & FW_PRIV_REQ_FALLBACK))
> +#define fw_param_uevent(params)						\
> +	(!!((params)->priv_reqs & FW_PRIV_REQ_FALLBACK_UEVENT))
> +#define fw_param_nocache(params)					\
> +	(!!((params)->priv_reqs & FW_PRIV_REQ_NO_CACHE))
> +#define fw_param_optional(params)					\
> +	(!!((params)->priv_reqs & FW_PRIV_REQ_OPTIONAL))

static inline functions to get proper typechecking?

>  static bool fw_get_builtin_firmware(struct firmware *fw, const char *name,
> -				    void *buf, size_t size)
> +				    struct fw_priv_params *fw_priv_params)

Shouldn't the priv pointer hang off of 'struct firmware' in an opaque
type that can not be seen/accessed outside of this file?

That way you don't have to change the functions by adding new
parameters, what you did seems a lot more complex.

thanks,

greg k-h