Re: [RFC PATCH 1/5] firmware: Create firmware upload framework

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

On 11/17/21 10:54 AM, Greg KH wrote:
> On Wed, Nov 17, 2021 at 10:47:38AM -0800, Russ Weight wrote:
>>
>> On 11/17/21 10:18 AM, Greg KH wrote:
>>> On Wed, Nov 17, 2021 at 10:00:54AM -0800, Russ Weight wrote:
>>>> On 11/17/21 7:15 AM, Greg KH wrote:
>>>>> On Wed, Nov 10, 2021 at 05:13:41PM -0800, Russ Weight wrote:
>>>>>> The Firmware Upload class driver provides a common API for uploading
>>>>>> firmware files to devices.
>>>>> That is exactly what the existing firmware api in the kernel is supposed
>>>>> to be accomplishing.
>>>>>
>>>>> If it is not doing what you need it to do, then you need to document the
>>>>> heck out of why it is not, and why you need a different api for this.  I
>>>>> do not see that here in this changelog at all :(
>>>> This is part of the documentation included later in this patch. I can add
>>>> this to the changelog.
>>>>
>>>> +Some devices load firmware from on-board FLASH when the card initializes.
>>>> +These cards do not require the request_firmware framework to load the
>>>> +firmware when the card boots, but they to require a utility to allow
>>>> +users to update the FLASH contents.
>>> There's no requirement that request_firmware only be done at boot time,
>>> why not use it at any point in time?
>> Calling request_firmware() is not restricted to boot time. But it requires
>> a firmware filename under /lib/firmware
> Not really, there are many locations it can be in.  See the different
> configuration options we have.
>
> But why would you want firmware in another location?
My current use case is for a user to upload a new, signed FPGA image to an FPGA
card. The card BMC authenticates and stores the data in FLASH. From the
perspective of the card, the image in FLASH is analogous to a firmware file
for another device being stored in /lib/firmware. For the FPGA images, there
is no real value to also storing the files in /lib/firmware (or anywhere else on
the system).

>
>> , and the convention is to specify the
>> filename in the kernel config.
> That is not a requirement at all.
>
>> I don't see any support for a user to provide a filename at run time
>> to be uploaded to a device, and that is the use case that I want to
>> support.
> If that's the only difference here, please work with the existing
> framework to add that tiny thing (i.e. pass in a name) to the current
> framework.  Do not create a whole new one please.
I think another fundamental difference is that the request_firmware() API is
a driver API for the device driver to do a data "pull". The firmware-upload API
is a user API for doing a data "push" (prepare(), write(), poll_complete()) to
the lower-level driver.

I did look at the backup option of the request_firmware() framework for writing
image data via sysfs. That's a possibility, but I thought that we would be abusing
the intent. I can look more at that option...

>
>>>> When you say "existing firmware api", I'm thinking request_firmware, which
>>>> requires that driver names be specified in the kernel config and wants to
>>>> load firmware automatically during device initialization.
>>> It can be used at any time, why do you think it's restricted to init
>>> time?
>>>
>>> And I do not understand your issue with driver names.
>> Sorry - I meant so say "firmware file names"
>>
>> In an earlier iteration of this patchset, you pointed out that allowing a user
>> to provide a filename for request_firmware() to use was a security issue.
> It is crazy, don't you think?
>
>> The use case that I am targeting is to allow a user to provide an image file
>> to a device at run time.
> That's not a limitation of the existing firmware layer.
>
> thanks,
>
> greg k-h