Messages in this thread | | | Date | Thu, 26 Jul 2012 10:51:55 +0800 | Subject | Re: [RFC PATCH 05/13] driver core: firmware loader: introduce firmware_buf | From | Ming Lei <> |
| |
On Wed, Jul 25, 2012 at 9:59 PM, Borislav Petkov <bp@amd64.org> wrote:
> This is subtle: the caller of fw_free_buf might forget to assign NULL to > the buf ptr.
Who is the caller? Since it is always called inside firmware loader, we should make sure that.
> Why not pass struct firmware_priv *fw_priv to the function instead and ...
No, it shouldn't. The lifetime of fw_priv is just same with request_firmware or its work_func pair, but firmware_buf may live much longer than fw_priv. You will see that fw_free_buf is the release function of kref in firmware_buf.
> >> + >> + for (i = 0; i < buf->nr_pages; i++) >> + __free_page(buf->pages[i]); >> + kfree(buf->pages); > > assign NULL to the ptr as a last step, when all is done: > > fw_priv->buf = NULL; > > This way you're making sure ->buf is NULL after all pages are freed and > your check above is always correct.
It has been done in _request_firmware_load
>> - kunmap(fw_priv->pages[page_nr]); >> + kunmap(buf->pages[page_nr]); >> buffer += page_cnt; >> offset += page_cnt; >> count -= page_cnt; >> @@ -320,12 +334,13 @@ out: > > While you're at it, you can indent this "out:" label one space to the > right so that the diff can pick up the function name in the hunk tag > above instead of the label.
Suppose you are right, it shouldn't be done in this patch since this patch just converts to firmware_buf.
Thanks, -- Ming Lei
| |