Messages in this thread | | | Date | Thu, 14 Apr 2022 17:39:27 -0700 | Subject | Re: [RESEND PATCH v1 3/8] firmware_loader: Split sysfs support from fallback | From | Russ Weight <> |
| |
On 4/2/22 08:15, Tom Rix wrote: > > On 3/23/22 4:33 PM, Russ Weight wrote: >> In preparation for sharing the "loading" and "data" sysfs nodes with the >> new firmware upload support, split out sysfs functionality from fallback.c >> and fallback.h into sysfs.c and sysfs.h. This includes the firmware >> class driver code that is associated with the sysfs files and the >> fw_fallback_config support for the timeout sysfs node. >> >> CONFIG_FW_LOADER_SYSFS is created and is selected by >> CONFIG_FW_LOADER_USER_HELPER in order to include sysfs.o in >> firmware_class-objs. >> >> This is mostly just a code reorganization. There are a few symbols that >> change in scope, and these can be identified by looking at the header >> file changes. A few white-space warnings from checkpatch are also >> addressed in this patch. >> >> Signed-off-by: Russ Weight <russell.h.weight@intel.com> >> --- >> v1: >> - Renamed files fw_sysfs.c and fw_sysfs.h to sysfs.c and sysfs.h >> - Moved "MODULE_IMPORT_NS(FIRMWARE_LOADER_PRIVATE);" from sysfs.c to >> sysfs.h to address an error identified by the kernel test robot >> <lkp@intel.com> >> --- >> drivers/base/firmware_loader/Kconfig | 4 + >> drivers/base/firmware_loader/Makefile | 1 + >> drivers/base/firmware_loader/fallback.c | 430 ------------------------ >> drivers/base/firmware_loader/fallback.h | 46 +-- >> drivers/base/firmware_loader/sysfs.c | 411 ++++++++++++++++++++++ >> drivers/base/firmware_loader/sysfs.h | 96 ++++++ >> 6 files changed, 513 insertions(+), 475 deletions(-) >> create mode 100644 drivers/base/firmware_loader/sysfs.c >> create mode 100644 drivers/base/firmware_loader/sysfs.h >> >> diff --git a/drivers/base/firmware_loader/Kconfig b/drivers/base/firmware_loader/Kconfig >> index 38f3b66bf52b..9e03178eee00 100644 >> --- a/drivers/base/firmware_loader/Kconfig >> +++ b/drivers/base/firmware_loader/Kconfig >> @@ -29,6 +29,9 @@ if FW_LOADER >> config FW_LOADER_PAGED_BUF >> bool >> +config FW_LOADER_SYSFS >> + bool >> + >> config EXTRA_FIRMWARE >> string "Build named firmware blobs into the kernel binary" >> help >> @@ -72,6 +75,7 @@ config EXTRA_FIRMWARE_DIR >> config FW_LOADER_USER_HELPER >> bool "Enable the firmware sysfs fallback mechanism" >> + select FW_LOADER_SYSFS > > Is this code reordering necessary ? > > This config is not removed or renamed later and has the same configs are the later FW_UPLOAD.
FW_UPLOAD and FW_LOADER_USER_HELPER both select FW_LOADER_SYSFS and FW_LOADER_PAGED_BUF, but they aren't exactly the same. With the split, you can have fallback.c without compiling sysfs_upload.c and you can have sysfs_upload.c without compiling fallback.c.
> > Maybe leave fallback.c as-is and rename FW_LOADER_USER_HELPER to FW_LOADER_SYSFS because the name is more descriptive. > > The 'sorry we suck' help message replaced with a shorter message to indicate this is now a more capable config. > > The later FW_UPLOAD would have a 'depends on FW_LOADER_SYSFS'
I could, of course, leave fallback.c as is and add to it, but then you couldn't compile in the firmware-upload support without also including the firmware-fallback support, and vice versa. That seems like a disadvantage to me. Isn't it better to allow a user to select these features separately but have them share code if they are both selected?
> > If you end up needing to do the reorder, move it to patch 1 because bisecting-wise it should not depend on improvements in the current 1,2 patches.
Patches 1 & 2 are standalone patches. They could be accepted independently without implementing firmware-upload. They don't depend on the code reorganization, and in my opinion, they are easier to understand when viewed before the code reorganization.
What do other's think? Should I keep all the code in fallback.c? Or is it better to split out the code? If I keep the split, should I move patches 1 & 2 to after the split?
- Russ
> > Tom > >> select FW_LOADER_PAGED_BUF >> help >> This option enables a sysfs loading facility to enable firmware
| |