> -----Original Message----- > From: Roy Franz [mailto:roy.franz@xxxxxxxxxx] > Sent: Friday, February 27, 2015 1:07 PM > > On Sun, Nov 2, 2014 at 7:07 PM, Kweh Hock Leong > > +/* > > + * This function will store the capsule binary and pass it to > > + * efi_capsule_update() API in capsule.c */ static int > > +efi_capsule_store(const struct firmware *fw) { > > + int i; > > + int ret; > > + int count = fw->size; > > + int copy_size = (fw->size > PAGE_SIZE) ? PAGE_SIZE : fw->size; > > + int pages_needed = ALIGN(fw->size, PAGE_SIZE) >> PAGE_SHIFT; > > + struct page **pages; > > + void *page_data; > > + efi_capsule_header_t *capsule = NULL; > > + > > + pages = kmalloc_array(pages_needed, sizeof(void *), GFP_KERNEL); > > + if (!pages) { > > + pr_err("%s: kmalloc_array() failed\n", __func__); > > + return -ENOMEM; > > + } > > + > > + for (i = 0; i < pages_needed; i++) { > > + pages[i] = alloc_page(GFP_KERNEL); > > + if (!pages[i]) { > > + pr_err("%s: alloc_page() failed\n", __func__); > > + --i; > > + ret = -ENOMEM; > > + goto failed; > > + } > > + } > > + > > + for (i = 0; i < pages_needed; i++) { > > + page_data = kmap(pages[i]); > > + memcpy(page_data, fw->data + (i * PAGE_SIZE), > > + copy_size); > > + > > + if (i == 0) > > + capsule = (efi_capsule_header_t *)page_data; > > + else > > + kunmap(pages[i]); > > + > > + count -= copy_size; > > + if (count < PAGE_SIZE) > > + copy_size = count; > > + } > > + > > + ret = efi_capsule_update(capsule, pages); > > + if (ret) { > > + pr_err("%s: efi_capsule_update() failed\n", __func__); > > + --i; > > Hi Hock, > > What does the decrement of i do here? I looked at > efi_capsule_update() and didn't see anything that would account for this. It > looks like in this failure case one page won't get freed. > > As an aside, when I was looking at efi_update_capsule, I see that Matt is > doing very similar operations (array of struct page pointers), but does it like > this (snipped from his patch): > > + struct page **block_pgs; > ... > + block_pgs = kzalloc(nr_block_pgs * sizeof(*block_pgs), GFP_KERNEL); > + if (!block_pgs) > + return -ENOMEM; > + > + for (i = 0; i < nr_block_pgs; i++) { > + block_pgs[i] = alloc_page(GFP_KERNEL); > + if (!block_pgs[i]) > + goto fail; > + } > > and then can simply free the pages that are not NULL: > +fail: > + for (i = 0; i < nr_block_pgs; i++) { > + if (block_pgs[i]) > + __free_page(block_pgs[i]); > + } > > I think this way is preferable since it doesn't rely on 'i' being unchanged at the > end of the function. I also think it would be nice if the capsule code stuck > with one idiom for dealing with arrays of page pointers. > > Roy > Hi Roy, The reason "i" there have to perform a decrement is because a full for loop will end with one extra incremented value if it does not break out in the middle. And the efi_capsule_store()'s alloc page is to store the binary content and the efi_capsule_update() from Matt is to store the efi capsule block descriptor which will point to the binary blocks content. So, they are different. Regards, Wilson ��.n��������+%������w��{.n�����{����* jg��� �����ݢj����G���� ���j:+v���w�m������w�������h�����٥
