On Wed, Jul 25, 2012 at 9:59 PM, Borislav Petkov <b...@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 -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
