| Message ID | 20200522214658.12722-1-scott.branden@broadcom.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | firmware_loader: change enum fw_opt to u32 | expand |
On Fri, May 22, 2020 at 02:46:58PM -0700, Scott Branden wrote: > > /** > - * enum fw_opt - options to control firmware loading behaviour > + * fw_opt - options to control firmware loading behaviour > * > * @FW_OPT_UEVENT: Enables the fallback mechanism to send a kobject uevent > * when the firmware is not found. Userspace is in charge to load the > @@ -33,15 +33,13 @@ > * the platform's main firmware. If both this fallback and the sysfs > * fallback are enabled, then this fallback will be tried first. > */ > -enum fw_opt { > - FW_OPT_UEVENT = BIT(0), > - FW_OPT_NOWAIT = BIT(1), > - FW_OPT_USERHELPER = BIT(2), > - FW_OPT_NO_WARN = BIT(3), > - FW_OPT_NOCACHE = BIT(4), > - FW_OPT_NOFALLBACK_SYSFS = BIT(5), > - FW_OPT_FALLBACK_PLATFORM = BIT(6), > -}; > +#define FW_OPT_UEVENT BIT(0) > +#define FW_OPT_NOWAIT BIT(1) > +#define FW_OPT_USERHELPER BIT(2) > +#define FW_OPT_NO_WARN BIT(3) > +#define FW_OPT_NOCACHE BIT(4) > +#define FW_OPT_NOFALLBACK_SYSFS BIT(5) > +#define FW_OPT_FALLBACK_PLATFORM BIT(6) Everything looked good up to here. The enum defines each flag. We just want to use an enum for *one* flag represetnation, not a bundle. Luis
Hi Luis, On 2020-05-22 3:45 p.m., Luis Chamberlain wrote: > On Fri, May 22, 2020 at 02:46:58PM -0700, Scott Branden wrote: >> >> /** >> - * enum fw_opt - options to control firmware loading behaviour >> + * fw_opt - options to control firmware loading behaviour >> * >> * @FW_OPT_UEVENT: Enables the fallback mechanism to send a kobject uevent >> * when the firmware is not found. Userspace is in charge to load the >> @@ -33,15 +33,13 @@ >> * the platform's main firmware. If both this fallback and the sysfs >> * fallback are enabled, then this fallback will be tried first. >> */ >> -enum fw_opt { >> - FW_OPT_UEVENT = BIT(0), >> - FW_OPT_NOWAIT = BIT(1), >> - FW_OPT_USERHELPER = BIT(2), >> - FW_OPT_NO_WARN = BIT(3), >> - FW_OPT_NOCACHE = BIT(4), >> - FW_OPT_NOFALLBACK_SYSFS = BIT(5), >> - FW_OPT_FALLBACK_PLATFORM = BIT(6), >> -}; >> +#define FW_OPT_UEVENT BIT(0) >> +#define FW_OPT_NOWAIT BIT(1) >> +#define FW_OPT_USERHELPER BIT(2) >> +#define FW_OPT_NO_WARN BIT(3) >> +#define FW_OPT_NOCACHE BIT(4) >> +#define FW_OPT_NOFALLBACK_SYSFS BIT(5) >> +#define FW_OPT_FALLBACK_PLATFORM BIT(6) > Everything looked good up to here. The enum defines each flag. > We just want to use an enum for *one* flag represetnation, not > a bundle. I do not know exactly what you are looking for then. The FW_OPT_* values are OR'd together in the code. You still want the fw_opt enum above left in place entirely and then the values used in OR'd together? > Luis
On Fri, May 22, 2020 at 03:49:05PM -0700, Scott Branden wrote: > Hi Luis, > > On 2020-05-22 3:45 p.m., Luis Chamberlain wrote: > > On Fri, May 22, 2020 at 02:46:58PM -0700, Scott Branden wrote: > > > /** > > > - * enum fw_opt - options to control firmware loading behaviour > > > + * fw_opt - options to control firmware loading behaviour > > > * > > > * @FW_OPT_UEVENT: Enables the fallback mechanism to send a kobject uevent > > > * when the firmware is not found. Userspace is in charge to load the > > > @@ -33,15 +33,13 @@ > > > * the platform's main firmware. If both this fallback and the sysfs > > > * fallback are enabled, then this fallback will be tried first. > > > */ > > > -enum fw_opt { > > > - FW_OPT_UEVENT = BIT(0), > > > - FW_OPT_NOWAIT = BIT(1), > > > - FW_OPT_USERHELPER = BIT(2), > > > - FW_OPT_NO_WARN = BIT(3), > > > - FW_OPT_NOCACHE = BIT(4), > > > - FW_OPT_NOFALLBACK_SYSFS = BIT(5), > > > - FW_OPT_FALLBACK_PLATFORM = BIT(6), > > > -}; > > > +#define FW_OPT_UEVENT BIT(0) > > > +#define FW_OPT_NOWAIT BIT(1) > > > +#define FW_OPT_USERHELPER BIT(2) > > > +#define FW_OPT_NO_WARN BIT(3) > > > +#define FW_OPT_NOCACHE BIT(4) > > > +#define FW_OPT_NOFALLBACK_SYSFS BIT(5) > > > +#define FW_OPT_FALLBACK_PLATFORM BIT(6) > > Everything looked good up to here. The enum defines each flag. > > We just want to use an enum for *one* flag represetnation, not > > a bundle. > > I do not know exactly what you are looking for then. The FW_OPT_* > values are OR'd together in the code. You still want the fw_opt enum > above left in place entirely and then the values used in OR'd > together? Yes, exactly. If they are combined then we just use a u32 to represent these are then flags combined. An enum represents just *one* flag, not a set which are OR'd together. Let me know if this is still not clear. Luis
On Fri, May 22, 2020 at 11:00:25PM +0000, Luis Chamberlain wrote: > On Fri, May 22, 2020 at 03:49:05PM -0700, Scott Branden wrote: > > Hi Luis, > > > > On 2020-05-22 3:45 p.m., Luis Chamberlain wrote: > > > On Fri, May 22, 2020 at 02:46:58PM -0700, Scott Branden wrote: > > > > /** > > > > - * enum fw_opt - options to control firmware loading behaviour > > > > + * fw_opt - options to control firmware loading behaviour > > > > * > > > > * @FW_OPT_UEVENT: Enables the fallback mechanism to send a kobject uevent > > > > * when the firmware is not found. Userspace is in charge to load the > > > > @@ -33,15 +33,13 @@ > > > > * the platform's main firmware. If both this fallback and the sysfs > > > > * fallback are enabled, then this fallback will be tried first. > > > > */ > > > > -enum fw_opt { > > > > - FW_OPT_UEVENT = BIT(0), > > > > - FW_OPT_NOWAIT = BIT(1), > > > > - FW_OPT_USERHELPER = BIT(2), > > > > - FW_OPT_NO_WARN = BIT(3), > > > > - FW_OPT_NOCACHE = BIT(4), > > > > - FW_OPT_NOFALLBACK_SYSFS = BIT(5), > > > > - FW_OPT_FALLBACK_PLATFORM = BIT(6), > > > > -}; > > > > +#define FW_OPT_UEVENT BIT(0) > > > > +#define FW_OPT_NOWAIT BIT(1) > > > > +#define FW_OPT_USERHELPER BIT(2) > > > > +#define FW_OPT_NO_WARN BIT(3) > > > > +#define FW_OPT_NOCACHE BIT(4) > > > > +#define FW_OPT_NOFALLBACK_SYSFS BIT(5) > > > > +#define FW_OPT_FALLBACK_PLATFORM BIT(6) > > > Everything looked good up to here. The enum defines each flag. > > > We just want to use an enum for *one* flag represetnation, not > > > a bundle. > > > > I do not know exactly what you are looking for then. The FW_OPT_* > > values are OR'd together in the code. You still want the fw_opt enum > > above left in place entirely and then the values used in OR'd > > together? > > Yes, exactly. If they are combined then we just use a u32 to represent > these are then flags combined. An enum represents just *one* flag, not > a set which are OR'd together. Let me know if this is still not clear. If this is the case, why have an enum at all? What difference is it from a #define like was done here? thanks, greg k-h
On Sat, May 23, 2020 at 08:05:48AM +0200, Greg Kroah-Hartman wrote: > On Fri, May 22, 2020 at 11:00:25PM +0000, Luis Chamberlain wrote: > > On Fri, May 22, 2020 at 03:49:05PM -0700, Scott Branden wrote: > > > Hi Luis, > > > > > > On 2020-05-22 3:45 p.m., Luis Chamberlain wrote: > > > > On Fri, May 22, 2020 at 02:46:58PM -0700, Scott Branden wrote: > > > > > /** > > > > > - * enum fw_opt - options to control firmware loading behaviour > > > > > + * fw_opt - options to control firmware loading behaviour > > > > > * > > > > > * @FW_OPT_UEVENT: Enables the fallback mechanism to send a kobject uevent > > > > > * when the firmware is not found. Userspace is in charge to load the > > > > > @@ -33,15 +33,13 @@ > > > > > * the platform's main firmware. If both this fallback and the sysfs > > > > > * fallback are enabled, then this fallback will be tried first. > > > > > */ > > > > > -enum fw_opt { > > > > > - FW_OPT_UEVENT = BIT(0), > > > > > - FW_OPT_NOWAIT = BIT(1), > > > > > - FW_OPT_USERHELPER = BIT(2), > > > > > - FW_OPT_NO_WARN = BIT(3), > > > > > - FW_OPT_NOCACHE = BIT(4), > > > > > - FW_OPT_NOFALLBACK_SYSFS = BIT(5), > > > > > - FW_OPT_FALLBACK_PLATFORM = BIT(6), > > > > > -}; > > > > > +#define FW_OPT_UEVENT BIT(0) > > > > > +#define FW_OPT_NOWAIT BIT(1) > > > > > +#define FW_OPT_USERHELPER BIT(2) > > > > > +#define FW_OPT_NO_WARN BIT(3) > > > > > +#define FW_OPT_NOCACHE BIT(4) > > > > > +#define FW_OPT_NOFALLBACK_SYSFS BIT(5) > > > > > +#define FW_OPT_FALLBACK_PLATFORM BIT(6) > > > > Everything looked good up to here. The enum defines each flag. > > > > We just want to use an enum for *one* flag represetnation, not > > > > a bundle. > > > > > > I do not know exactly what you are looking for then. The FW_OPT_* > > > values are OR'd together in the code. You still want the fw_opt enum > > > above left in place entirely and then the values used in OR'd > > > together? > > > > Yes, exactly. If they are combined then we just use a u32 to represent > > these are then flags combined. An enum represents just *one* flag, not > > a set which are OR'd together. Let me know if this is still not clear. > > If this is the case, why have an enum at all? What difference is it > from a #define like was done here? If used on a switch to test for cases the compiler will error if you miss one, and it is easier to use kdoc for them. Luis
diff --git a/drivers/base/firmware_loader/fallback.c b/drivers/base/firmware_loader/fallback.c index 1e9c96e3ed63..af1aa3d69920 100644 --- a/drivers/base/firmware_loader/fallback.c +++ b/drivers/base/firmware_loader/fallback.c @@ -460,7 +460,7 @@ static const struct attribute_group *fw_dev_attr_groups[] = { static struct fw_sysfs * fw_create_instance(struct firmware *firmware, const char *fw_name, - struct device *device, enum fw_opt opt_flags) + struct device *device, u32 opt_flags) { struct fw_sysfs *fw_sysfs; struct device *f_dev; @@ -493,7 +493,7 @@ fw_create_instance(struct firmware *firmware, const char *fw_name, * In charge of constructing a sysfs fallback interface for firmware loading. **/ static int fw_load_sysfs_fallback(struct fw_sysfs *fw_sysfs, - enum fw_opt opt_flags, long timeout) + u32 opt_flags, long timeout) { int retval = 0; struct device *f_dev = &fw_sysfs->dev; @@ -547,7 +547,7 @@ static int fw_load_sysfs_fallback(struct fw_sysfs *fw_sysfs, static int fw_load_from_user_helper(struct firmware *firmware, const char *name, struct device *device, - enum fw_opt opt_flags) + u32 opt_flags) { struct fw_sysfs *fw_sysfs; long timeout; @@ -588,7 +588,7 @@ static int fw_load_from_user_helper(struct firmware *firmware, return ret; } -static bool fw_force_sysfs_fallback(enum fw_opt opt_flags) +static bool fw_force_sysfs_fallback(u32 opt_flags) { if (fw_fallback_config.force_sysfs_fallback) return true; @@ -597,7 +597,7 @@ static bool fw_force_sysfs_fallback(enum fw_opt opt_flags) return true; } -static bool fw_run_sysfs_fallback(enum fw_opt opt_flags) +static bool fw_run_sysfs_fallback(u32 opt_flags) { int ret; @@ -640,7 +640,7 @@ static bool fw_run_sysfs_fallback(enum fw_opt opt_flags) **/ int firmware_fallback_sysfs(struct firmware *fw, const char *name, struct device *device, - enum fw_opt opt_flags, + u32 opt_flags, int ret) { if (!fw_run_sysfs_fallback(opt_flags)) diff --git a/drivers/base/firmware_loader/fallback.h b/drivers/base/firmware_loader/fallback.h index 06f4577733a8..2afdb6adb23f 100644 --- a/drivers/base/firmware_loader/fallback.h +++ b/drivers/base/firmware_loader/fallback.h @@ -33,7 +33,7 @@ struct firmware_fallback_config { #ifdef CONFIG_FW_LOADER_USER_HELPER int firmware_fallback_sysfs(struct firmware *fw, const char *name, struct device *device, - enum fw_opt opt_flags, + u32 opt_flags, int ret); void kill_pending_fw_fallback_reqs(bool only_kill_custom); @@ -45,7 +45,7 @@ void unregister_sysfs_loader(void); #else /* CONFIG_FW_LOADER_USER_HELPER */ static inline int firmware_fallback_sysfs(struct firmware *fw, const char *name, struct device *device, - enum fw_opt opt_flags, + u32 opt_flags, int ret) { /* Keep carrying over the same error */ @@ -67,10 +67,10 @@ static inline void unregister_sysfs_loader(void) #endif /* CONFIG_FW_LOADER_USER_HELPER */ #ifdef CONFIG_EFI_EMBEDDED_FIRMWARE -int firmware_fallback_platform(struct fw_priv *fw_priv, enum fw_opt opt_flags); +int firmware_fallback_platform(struct fw_priv *fw_priv, u32 opt_flags); #else static inline int firmware_fallback_platform(struct fw_priv *fw_priv, - enum fw_opt opt_flags) + u32 opt_flags) { return -ENOENT; } diff --git a/drivers/base/firmware_loader/fallback_platform.c b/drivers/base/firmware_loader/fallback_platform.c index c88c745590fe..cdd2c9a9f38a 100644 --- a/drivers/base/firmware_loader/fallback_platform.c +++ b/drivers/base/firmware_loader/fallback_platform.c @@ -8,7 +8,7 @@ #include "fallback.h" #include "firmware.h" -int firmware_fallback_platform(struct fw_priv *fw_priv, enum fw_opt opt_flags) +int firmware_fallback_platform(struct fw_priv *fw_priv, u32 opt_flags) { const u8 *data; size_t size; diff --git a/drivers/base/firmware_loader/firmware.h b/drivers/base/firmware_loader/firmware.h index 25836a6afc9f..cf6dc96a84a2 100644 --- a/drivers/base/firmware_loader/firmware.h +++ b/drivers/base/firmware_loader/firmware.h @@ -12,7 +12,7 @@ #include <generated/utsrelease.h> /** - * enum fw_opt - options to control firmware loading behaviour + * fw_opt - options to control firmware loading behaviour * * @FW_OPT_UEVENT: Enables the fallback mechanism to send a kobject uevent * when the firmware is not found. Userspace is in charge to load the @@ -33,15 +33,13 @@ * the platform's main firmware. If both this fallback and the sysfs * fallback are enabled, then this fallback will be tried first. */ -enum fw_opt { - FW_OPT_UEVENT = BIT(0), - FW_OPT_NOWAIT = BIT(1), - FW_OPT_USERHELPER = BIT(2), - FW_OPT_NO_WARN = BIT(3), - FW_OPT_NOCACHE = BIT(4), - FW_OPT_NOFALLBACK_SYSFS = BIT(5), - FW_OPT_FALLBACK_PLATFORM = BIT(6), -}; +#define FW_OPT_UEVENT BIT(0) +#define FW_OPT_NOWAIT BIT(1) +#define FW_OPT_USERHELPER BIT(2) +#define FW_OPT_NO_WARN BIT(3) +#define FW_OPT_NOCACHE BIT(4) +#define FW_OPT_NOFALLBACK_SYSFS BIT(5) +#define FW_OPT_FALLBACK_PLATFORM BIT(6) enum fw_status { FW_STATUS_UNKNOWN, @@ -136,8 +134,7 @@ static inline void fw_state_done(struct fw_priv *fw_priv) __fw_state_set(fw_priv, FW_STATUS_DONE); } -int assign_fw(struct firmware *fw, struct device *device, - enum fw_opt opt_flags); +int assign_fw(struct firmware *fw, struct device *device, u32 opt_flags); #ifdef CONFIG_FW_LOADER_PAGED_BUF void fw_free_paged_buf(struct fw_priv *fw_priv); diff --git a/drivers/base/firmware_loader/main.c b/drivers/base/firmware_loader/main.c index 76f79913916d..2ca6b4735979 100644 --- a/drivers/base/firmware_loader/main.c +++ b/drivers/base/firmware_loader/main.c @@ -210,7 +210,7 @@ static struct fw_priv *__lookup_fw_priv(const char *fw_name) static int alloc_lookup_fw_priv(const char *fw_name, struct firmware_cache *fwc, struct fw_priv **fw_priv, void *dbuf, - size_t size, enum fw_opt opt_flags) + size_t size, u32 opt_flags) { struct fw_priv *tmp; @@ -635,8 +635,7 @@ static int fw_add_devm_name(struct device *dev, const char *name) } #endif -int assign_fw(struct firmware *fw, struct device *device, - enum fw_opt opt_flags) +int assign_fw(struct firmware *fw, struct device *device, u32 opt_flags) { struct fw_priv *fw_priv = fw->priv; int ret; @@ -687,7 +686,7 @@ int assign_fw(struct firmware *fw, struct device *device, static int _request_firmware_prepare(struct firmware **firmware_p, const char *name, struct device *device, void *dbuf, size_t size, - enum fw_opt opt_flags) + u32 opt_flags) { struct firmware *firmware; struct fw_priv *fw_priv; @@ -753,7 +752,7 @@ static void fw_abort_batch_reqs(struct firmware *fw) static int _request_firmware(const struct firmware **firmware_p, const char *name, struct device *device, void *buf, size_t size, - enum fw_opt opt_flags) + u32 opt_flags) { struct firmware *fw = NULL; int ret; @@ -990,7 +989,7 @@ struct firmware_work { struct device *device; void *context; void (*cont)(const struct firmware *fw, void *context); - enum fw_opt opt_flags; + u32 opt_flags; }; static void request_firmware_work_func(struct work_struct *work)
"enum fw_opt" is not used as an enum. Change fw_opt to a u32 and the FW_OPT_* values inside the enum to defines. Signed-off-by: Scott Branden <scott.branden@broadcom.com> --- drivers/base/firmware_loader/fallback.c | 12 +++++------ drivers/base/firmware_loader/fallback.h | 8 +++---- .../base/firmware_loader/fallback_platform.c | 2 +- drivers/base/firmware_loader/firmware.h | 21 ++++++++----------- drivers/base/firmware_loader/main.c | 11 +++++----- 5 files changed, 25 insertions(+), 29 deletions(-)