Re: [PATCH] firmware: cleanup - group and document up private firmware parameters
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
Re: [PATCH] firmware: cleanup - group and document up private firmware parameters
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
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 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
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 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
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 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
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 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
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 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
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 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
