From: Russ Weight <russell.h.weight@intel.com>
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>
Subject: Re: [RESEND PATCH v1 1/8] firmware_loader: Clear data and size in fw_free_paged_buf
Date: Mon, 28 Mar 2022 14:08:50 -0600 [thread overview]
Message-ID: <83f24772-3fa5-c775-a3a5-77d57731d56e@intel.com> (raw)
In-Reply-To: <16f93247-3af2-e9cb-b7f6-c8305fa1db33@redhat.com>
On 3/28/22 11:52, Tom Rix wrote:
>
> On 3/28/22 11:09 AM, Russ Weight wrote:
>> Hi Tom,
>>
>> On 3/28/22 06:27, Tom Rix wrote:
>>> On 3/23/22 4:33 PM, Russ Weight wrote:
>>>> The fw_free_paged_buf() function resets the paged buffer information in
>>>> the fw_priv data structure. Additionally, clear the data and size members
>>>> of fw_priv in order to facilitate the reuse of fw_priv. This is being
>>>> done in preparation for enabling userspace to initiate multiple firmware
>>>> uploads using this sysfs interface.
>>>>
>>>> Signed-off-by: Russ Weight <russell.h.weight@intel.com>
>>>> ---
>>>> v1:
>>>> - No change from RFC patch
>>>> ---
>>>> drivers/base/firmware_loader/main.c | 2 ++
>>>> 1 file changed, 2 insertions(+)
>>>>
>>>> diff --git a/drivers/base/firmware_loader/main.c b/drivers/base/firmware_loader/main.c
>>>> index 94d1789a233e..2cc11d93753a 100644
>>>> --- a/drivers/base/firmware_loader/main.c
>>>> +++ b/drivers/base/firmware_loader/main.c
>>>> @@ -253,6 +253,8 @@ void fw_free_paged_buf(struct fw_priv *fw_priv)
>>> Why isn't a vfree needed or realloc done?
>
> I am looking at the use of this function in __free_fw_priv
>
> if (fw_is_paged_buf(fw_priv))
> fw_free_paged_buf(fw_priv);
> else if (!fw_priv->allocated_size)
> vfree(fw_priv->data);
>
> Where it seems like there is another way to set data, so it needs another way to unset.
>
> The vfree here looks suspect because the pointer comes in from request_firmware_info_buf with a hope that it was allocated by vmalloc.
There are places in the code where, if fw_priv->data is set, it is assumed that it
was allocated by the caller. In the sysfs-upload path, vmalloc is never used - only
paged buffers.
As I have revisited the code to answer your questions, I see that in the firmware
fallback path, after it checks for (!fw_priv->data), it sets "fw_priv->is_paged_buf = true".
I realize now that I should be setting "fw_priv->is_paged_buf = true" in firmware_upload_register(). I'll make that change and rebase on 5.18-rc1 when it is
available.
Thanks for the comments!
- Russ
>
> Tom
>
>> The free and realloc support was present prior to my changes. The page
>> buffer support was designed such that if a firmware write was cancelled, the
>> existing fw_priv structure could be re-used for another write in the context
>> of the same firmware upload. However, there was no prior case for completing
>> a write and then reusing the fw_priv structure for subsequent firmware writes;
>> fw_priv previously had a one-time use. The changes I have made are to enable
>> the re-use of the fw_priv structure.
>>
>> Initially, fw_priv->data is NULL. The "realloc" functionality happens during
>> the write of the data binary attribute here:
>>
>> https://github.com/torvalds/linux/blob/ae085d7f9365de7da27ab5c0d16b12d51ea7fca9/drivers/base/firmware_loader/fallback.c#L426
>>
>> The fw_priv->data pointer remains NULL until all data is written and the
>> user writes '0' to the loading attribute. The fw_priv->data pointer is set in
>> fw_map_paged_buf() which is called here:
>>
>> https://github.com/torvalds/linux/blob/ae085d7f9365de7da27ab5c0d16b12d51ea7fca9/drivers/base/firmware_loader/fallback.c#L274
>>
>> In the unmodified code, the fw_priv->data pointer is never cleared. My changes
>> reset the pointer to NULL after the memory is released so that the fw_priv can
>> be resused.
>>
>> The new firmware-upload happens in the context of a kernel worker thread and the work
>> function is fw_upload_main(). At the end of fw_upload_main(), fw_free_paged_buf()
>> is called to do the free. This is the function that is being modified by the lines
>> below. This function calls "__free_page(fw_priv->pages[i])" in a loop to free the
>> memory pages. It also calls "vunmap(fw_priv->data)" to free the virtual mapping.
>> You can see the unmodified implementation of this function here:
>>
>> https://github.com/torvalds/linux/blob/ae085d7f9365de7da27ab5c0d16b12d51ea7fca9/drivers/base/firmware_loader/main.c#L241
>>
>> - Russ
>>
>>> Tom
>>>
>>>> fw_priv->pages = NULL;
>>>> fw_priv->page_array_size = 0;
>>>> fw_priv->nr_pages = 0;
>>>> + fw_priv->data = NULL;
>>>> + fw_priv->size = 0;
>>>> }
>>>> int fw_grow_paged_buf(struct fw_priv *fw_priv, int pages_needed)
>
next prev parent reply other threads:[~2022-03-28 20:09 UTC|newest]
Thread overview: 17+ messages / expand[flat|nested] mbox.gz Atom feed top
2022-03-23 23:33 [RESEND PATCH v1 0/8] Extend FW framework for user FW uploads Russ Weight
2022-03-23 23:33 ` [RESEND PATCH v1 1/8] firmware_loader: Clear data and size in fw_free_paged_buf Russ Weight
2022-03-28 13:27 ` Tom Rix
2022-03-28 18:09 ` Russ Weight
2022-03-28 18:52 ` Tom Rix
2022-03-28 20:08 ` Russ Weight [this message]
2022-03-23 23:33 ` [RESEND PATCH v1 2/8] firmware_loader: Check fw_state_is_done in loading_store Russ Weight
2022-04-02 14:47 ` Tom Rix
2022-04-14 23:41 ` Russ Weight
2022-03-23 23:33 ` [RESEND PATCH v1 3/8] firmware_loader: Split sysfs support from fallback Russ Weight
2022-04-02 15:15 ` Tom Rix
2022-04-15 0:39 ` Russ Weight
2022-03-23 23:33 ` [RESEND PATCH v1 4/8] firmware_loader: Add firmware-upload support Russ Weight
2022-03-23 23:33 ` [RESEND PATCH v1 5/8] firmware_loader: Add sysfs nodes to monitor fw_upload Russ Weight
2022-03-23 23:33 ` [RESEND PATCH v1 6/8] test_firmware: Add test support for firmware upload Russ Weight
2022-03-23 23:33 ` [RESEND PATCH v1 7/8] test_firmware: Error injection " Russ Weight
2022-03-23 23:33 ` [RESEND PATCH v1 8/8] selftests: firmware: Add firmware upload selftests Russ Weight
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=83f24772-3fa5-c775-a3a5-77d57731d56e@intel.com \
--to=russell.h.weight@intel.com \
--cc=basheer.ahmed.muddebihal@intel.com \
--cc=gregkh@linuxfoundation.org \
--cc=hao.wu@intel.com \
--cc=lgoncalv@redhat.com \
--cc=linux-kernel@vger.kernel.org \
--cc=matthew.gerlach@intel.com \
--cc=mcgrof@kernel.org \
--cc=rafael@kernel.org \
--cc=tianfei.zhang@intel.com \
--cc=trix@redhat.com \
--cc=yilun.xu@intel.com \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).