Messages in this thread | | | Subject | Re: [RESEND PATCH v1 1/8] firmware_loader: Clear data and size in fw_free_paged_buf | From | Tom Rix <> | Date | Mon, 28 Mar 2022 11:52:25 -0700 |
| |
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.
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)
| |