Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::1:18 as permitted sender) client-ip=2620:137:e000::1:18;
Message-ID: <1a5228c3-4ab0-0e2e-0d93-15ca9a90f1c2@intel.com>
Date:   Thu, 14 Apr 2022 17:39:27 -0700
User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:91.0) Gecko/20100101
 Firefox/91.0 Thunderbird/91.5.0
Subject: Re: [RESEND PATCH v1 3/8] firmware_loader: Split sysfs support from
 fallback
Content-Language: en-US
To:     Tom Rix <trix@redhat.com>, <mcgrof@kernel.org>,
        <gregkh@linuxfoundation.org>, <rafael@kernel.org>,
        <linux-kernel@vger.kernel.org>
CC:     <lgoncalv@redhat.com>, <yilun.xu@intel.com>, <hao.wu@intel.com>,
        <matthew.gerlach@intel.com>, <basheer.ahmed.muddebihal@intel.com>,
        <tianfei.zhang@intel.com>
References: <20220323233331.155121-1-russell.h.weight@intel.com>
 <20220323233331.155121-4-russell.h.weight@intel.com>
 <346561f8-df45-c3aa-a6b9-1328abe80e8f@redhat.com>
From:   Russ Weight <russell.h.weight@intel.com>
In-Reply-To: <346561f8-df45-c3aa-a6b9-1328abe80e8f@redhat.com>
Content-Type: text/plain; charset="UTF-8"
Content-Transfer-Encoding: 8bit
MIME-Version: 1.0
X-MS-Exchange-AntiSpam-MessageData-ChunkCount: 1
X-MS-Exchange-AntiSpam-MessageData-0: =?utf-8?B?MncwTDVBMVE2MVRYUG8vOVZ0ODJwZllTTExVbVhXZ1lZckQydkg3NmdDUHRl?=
 =?utf-8?B?ZURhQmJLR25sYVlsSlZNR2RrWWdGUFVqSEVob3JPUGIzU0RwUVJBaXVzc2Q0?=
 =?utf-8?B?SEJqeFpNK1hscm5JU3NlMExhczVobDJINjlZMFl5aU9IWFBjc3BnblV4QXJK?=
 =?utf-8?B?cHY3bzBWREp2SGVGVnF0L3JtM1VsWFhjRXlzNTI2RE9JVmRBM3pMM0N4NzBB?=
 =?utf-8?B?WDZSZzBrYWpsdEd0QVJrN1NhK2drSFFxTUJFVW9JbSsrTTQwc1ZiQTcwNFJM?=
 =?utf-8?B?SWFSMXJjRU9CenRISmJQMFhGdWJwam5ZaEJ4VGhuQi9kUEUxYmg5ZzhzRnBy?=
 =?utf-8?B?MFdKZWwwelo0MG16U1NUZThlM0NlQ1YvT3BBZytBVVYzaDYyUmtYNzA5cExy?=
 =?utf-8?B?VW90cnJHaVZSNmZqVS9uMHNweVlTZnJCYkozN0RVUVk3dDlNTWFCeUxKb0Zr?=
 =?utf-8?B?Wi9ZZnVySDJEM2k1YjY3N2g0UGhDSkUrQVQ4bi93eTFPKzFHMXB6RnRGNzNK?=
 =?utf-8?B?WWxYRXNZb1dQeExaZjhKNFdmaFYyWTNaVnhoMXhjZnYrWEhxSkRmNEVvbTFi?=
 =?utf-8?B?U0thN0RHVUlOZ2c5VVhJWVo5d3Bhc3pORmNWdGZFUzdacUhOeGNvOHdaYzFs?=
 =?utf-8?B?cVd1OGpRazdiY3IvVjlKNDNoWWU3VTBPK3pnTGh3d1o5dGpEZFVJaUtaeEho?=
 =?utf-8?B?Rm91SVNPNUFRMTQxQ2Z6Tjd3N2xLR2tTaWVYSkFoRWRQSUN2dXBhUERUdnBm?=
 =?utf-8?B?QkVmY0ZHLzFWMlpncWdiMDBFS0h1cXU3SWlmeG5aSXRacUdCSUJtaExtaDZT?=
 =?utf-8?B?dHdxMi9PWlhTUnNiQ3NiYkx6NlUxZjVnNGJZSEVFSDZqMEFUeUxoSmx2MXhH?=
 =?utf-8?B?RFpyZk5IbUhZWCt1QkNwd09IS2VSLzZYMkJZbEk2K1Q3WWo4dEtHMWNPc29M?=
 =?utf-8?B?UkJvcjZIMTJiVXZNMXpwbmZ0TlE0bW5CVlcrQ0FFajlVS3paUXR3SWN1d3lG?=
 =?utf-8?B?b3VRNHRaUEt1cS9lT0l1cDhjSHE3S1dHcmxJaWRDNFZ2RS84SjFnM01EU0RX?=
 =?utf-8?B?cXJIYk1CYklWTndjcVNHTEYrVGdmYWdJTVFtb2twV2xqTXhldk5IRDJKaUpP?=
 =?utf-8?B?YzNDOXYrbG9TUkpCVkJOOW5sZWVKcTRjQzlONFNMQWtLSlB0SUJvZkdJUDZh?=
 =?utf-8?B?cERPaHpWUmRuc3czMFhyZTlYRW1IYmtPWng1eEYxbmlVOGJ1cmxaeHprTVFn?=
 =?utf-8?B?Z0F1V3psRjdRekd2RDlDNmtTTGVtZFlicWp1UmpKWkhIMjhGSEJ3Y2xJcHg2?=
 =?utf-8?B?STJhc1M1Q296QXNZVE5SMzRJK01lTFRBMzYwUUNOcG1nNlRpMEs3bGRrVXFT?=
 =?utf-8?B?SUFONFBrSkszVVpJandDbUVYY0pWQjZuSzBKNmJsMnE4ZVduM0JJeGJCQVJs?=
 =?utf-8?B?L0xvTTZKc3RzWjhMMytDdjVaQk91YksxVmlqM1g3b1hLdDljS1c5RXZWTUdU?=
 =?utf-8?B?VWNCdEQvWkg1TEZsYzlYa1BhV0IrTEY0eElGbnlNRW1PVjBMblMvZEdFVlB3?=
 =?utf-8?B?RVFxZ3puREFzb2dMR2cybFB3MUg1VTlkZFFoZWtQcmtGeE12YUp0U1VZZHdC?=
 =?utf-8?B?L2lrY0Iwb0s5SWs5K0ZOTnlEZ2ZYdXltUFo1NE9lYlpLVXdiL2xUNFNOTDlG?=
 =?utf-8?B?Q25rQ2tWZTVxcFBiNU9McmVoMFRYR1VleUxDTnRJK3YxTGViTlk3aG9Cb0RJ?=
 =?utf-8?B?c1JiQVRIZ0ZHYWFRejJMUmpxSmRDOE5CK2RxREE5a3J3N00ra3ZlcTZBRnpI?=
 =?utf-8?B?a2crbjcxSEVuMi9lQ0Zaak5lRGhybHdOYS82N3l4S0RXbUZXNEk2M09jNTFt?=
 =?utf-8?B?bHR2aXZPaUdQYVJFZURhZzNsMERIM2FOR21Ka0ptTWliKzRJRG5NWWpxdTBY?=
 =?utf-8?B?cHVmL0ZUS0Q5RXlEcVFvYmQxVElMM3lWWTdKVEZvelJ0R2Uxem1RZVRiMXhI?=
 =?utf-8?B?NGIzK21CNFJkUjhVS2FyR09tWUVuWnUxWlVYVVNYck16bkthMzJ3Qk56K1d1?=
 =?utf-8?B?dm11UTRERjVNTzhvOEFtUkhhenp4YUpUc3FweDVoQlVFbXp6TUt5WjRUa1hy?=
 =?utf-8?B?RnYrcXlkR1BsME9HYWVFUDZUQzI1b3d1UXliOUljZkdrS2JEY1lZQjJOWHF3?=
 =?utf-8?B?SXhUUGRUd2ovZjFCTkduVkRXSVJIb2t3bDlGM2lkNklya0FvT0ZON0JtcG9C?=
 =?utf-8?B?NzlsVlY2dVRicFU5THBtMkxGNlkvRVhVVDRqZW1KM0ZrTy9QaUdCc2kzVW51?=
 =?utf-8?B?UGlvWVZNUXNxUWJuZERNNkkyWGhYUHlDR3pSdS9LTjFHeGY5RWVJdUNiR1pS?=
 =?utf-8?Q?f7OJSuHdsz/YScPs=3D?=
X-MS-Exchange-CrossTenant-Network-Message-Id: 5fe9b8bf-482a-420d-826d-08da1e786759
X-MS-Exchange-CrossTenant-AuthSource: DM5PR11MB1899.namprd11.prod.outlook.com
X-MS-Exchange-CrossTenant-AuthAs: Internal
X-MS-Exchange-CrossTenant-OriginalArrivalTime: 15 Apr 2022 00:39:30.7049
 (UTC)
X-MS-Exchange-CrossTenant-FromEntityHeader: Hosted
X-MS-Exchange-CrossTenant-Id: 46c98d88-e344-4ed4-8496-4ed7712e255d
X-MS-Exchange-CrossTenant-MailboxType: HOSTED
X-MS-Exchange-CrossTenant-UserPrincipalName: iDWXdM1+CRYdWo+Ex1jPBy3IDlcBr515L0XyUZOxMBZQ9mdJTgTuvnIoayRUv0s1eepnnr4cScPDxTrEMo7zwvcvpoa4BkyejIEBH8H2s9s=
X-MS-Exchange-Transport-CrossTenantHeadersStamped: BY5PR11MB4209
X-OriginatorOrg: intel.com
X-Spam-Status: No, score=-5.8 required=5.0 tests=BAYES_00,DKIMWL_WL_HIGH,
	DKIM_SIGNED,DKIM_VALID,DKIM_VALID_AU,HEADER_FROM_DIFFERENT_DOMAINS,
	MAILING_LIST_MULTI,NICE_REPLY_A,RDNS_NONE,SPF_HELO_NONE,
	T_SCC_BODY_TEXT_LINE autolearn=unavailable autolearn_force=no
	version=3.4.6
X-Spam-Checker-Version: SpamAssassin 3.4.6 (2021-04-09) on
	lindbergh.monkeyblade.net
Precedence: bulk
List-ID: <linux-kernel.vger.kernel.org>
X-Mailing-List: linux-kernel@vger.kernel.org


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