linux-kernel - Re: [PATCH] firmware: add a function to load optional firmware v2

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] [thread-next>] [day] [month] [year] [list]

Date:   Wed, 14 Mar 2018 10:48:46 +0200
From:   Kalle Valo <kvalo@...eaurora.org>
To:     Arend van Spriel <arend.vanspriel@...adcom.com>
Cc:     "Luis R. Rodriguez" <mcgrof@...nel.org>,
        Andres Rodriguez <andresx7@...il.com>,
        "linux-kernel\@vger.kernel.org" <linux-kernel@...r.kernel.org>,
        Greg Kroah-Hartman <gregkh@...uxfoundation.org>,
        linux-wireless <linux-wireless@...r.kernel.org>
Subject: Re: [PATCH] firmware: add a function to load optional firmware v2

Arend van Spriel <arend.vanspriel@...adcom.com> writes:

> On 3/13/2018 5:46 PM, Kalle Valo wrote:
>> "Luis R. Rodriguez" <mcgrof@...nel.org> writes:
>>
>>> On Tue, Mar 13, 2018 at 03:16:34PM +0200, Kalle Valo wrote:
>>>> "Luis R. Rodriguez" <mcgrof@...nel.org> writes:
>>>>
>>>>>> +/**
>>>>>> + * request_firmware_optional: - request for an optional fw module
>>>>>> + * @firmware_p: pointer to firmware image
>>>>>> + * @name: name of firmware file
>>>>>> + * @device: device for which firmware is being loaded
>>>>>> + *
>>>>>> + * This function is similar in behaviour to request_firmware(), except
>>>>>> + * it doesn't produce warning messages when the file is not found.
>>>>>> + **/
>>>>>> +int
>>>>>> +request_firmware_optional(const struct firmware **firmware_p, const char *name,
>>>>>> +                         struct device *device)
>>>>>> +{
>>>>>> +       int ret;
>>>>>> +
>>>>>> +       /* Need to pin this module until return */
>>>>>> +       __module_get(THIS_MODULE);
>>>>>> +               ret = _request_firmware(firmware_p, name, device, NULL, 0,
>>>>>> +                               FW_OPT_UEVENT | FW_OPT_NO_WARN );
>>>>>> +       module_put(THIS_MODULE);
>>>>>> +       return ret;
>>>>>> +}
>>>>>> +EXPORT_SYMBOL(request_firmware_optional);
>>>>>
>>>>> New exported symbols for the firmware API should be EXPORT_SYMBOL_GPL().
>>>>
>>>> To me the word optional feels weird to me. For example, in ath10k I
>>>> suspect we would be only calling request_firmware_optional() with all
>>>> firmware and not request_firmware() at all.
>>>>
>>>> How about request_firmware_nowarn()? That would even match the
>>>> documentation above.
>>>
>>> _nowarn() works with me. Do you at least want the return value to give
>>> an error value if no file was found? This way the driver can decide
>>> when to issue an error if it wants to.
>>
>> Yes, it would be very good to return the error value to ath10k. That way
>> we can give a proper error message to the user if we can't find a
>> suitable firmware image.
>
> I fully agree with the _nowarn() and returning an error. However, the
> firmware_p parameter (btw. do we really want the _p postfix?)

Oh yeah, that _p is ugly. Please get rid of it, hungarian notation is
awful.

> is an output parameter which will be null in case of an error so do
> you really need a specific error code for the proper error message.

Sometimes the error code helps with debugging. But let's ask it this
way: why would we NOT return an error code? What would we gain from
that? I don't see any advantage from dropping the error code, on the
contrary better to be consistent with request_firmware() to avoid any
confusion.

-- 
Kalle Valo