Re: [RESEND PATCH v1 3/8] firmware_loader: Split sysfs support from fallback

[Russ Weight <russell.h.weight@intel.com>]

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