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

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [day] [month] [year] [list]

Message-ID: <1a5228c3-4ab0-0e2e-0d93-15ca9a90f1c2@intel.com>
Date:   Thu, 14 Apr 2022 17:39:27 -0700
From:   Russ Weight <russell.h.weight@...el.com>
To:     Tom Rix <trix@...hat.com>, <mcgrof@...nel.org>,
        <gregkh@...uxfoundation.org>, <rafael@...nel.org>,
        <linux-kernel@...r.kernel.org>
CC:     <lgoncalv@...hat.com>, <yilun.xu@...el.com>, <hao.wu@...el.com>,
        <matthew.gerlach@...el.com>, <basheer.ahmed.muddebihal@...el.com>,
        <tianfei.zhang@...el.com>
Subject: Re: [RESEND PATCH v1 3/8] firmware_loader: Split sysfs support from
 fallback



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