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

[Luis R. Rodriguez]

On Mon, Sep 18, 2017 at 05:15:01PM +0200, Greg KH wrote:
> 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()?

Fixed.

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

Went with pri_-flags.

> > +	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?

Sure!

> >  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.

Excellent point!

Note that we already have and use a priv opaque pointer, its however only a buf
pointer really though, I could extend this while looking at doing just that
I noticed that we also already have a "struct firmware_priv" but this is
really only used for the sysfs interface loading functionality. Adding
yet another private pointer is only going to make things more confusing,
so I'll go rename that accordingly and try to consolidate what I can so that
this patch itself is much smaller and easier to read.

Thanks for the feedback!

  Luis