Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1752402AbbB0Lfz (ORCPT ); Fri, 27 Feb 2015 06:35:55 -0500 Received: from mga02.intel.com ([134.134.136.20]:31812 "EHLO mga02.intel.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1751287AbbB0Lfx (ORCPT ); Fri, 27 Feb 2015 06:35:53 -0500 X-ExtLoop1: 1 X-IronPort-AV: E=Sophos;i="5.09,659,1418112000"; d="scan'208";a="684323985" From: "Kweh, Hock Leong" To: Roy Franz CC: "Fleming, Matt" , Ming Lei , Sam Protsenko , "Greg Kroah-Hartman" , LKML , "linux-efi@vger.kernel.org" , "Ong, Boon Leong" Subject: RE: [PATCH v2 3/3] efi: Capsule update with user helper interface Thread-Topic: [PATCH v2 3/3] efi: Capsule update with user helper interface Thread-Index: AQHQUks6p7XIEiKFpEO8Vl2BGyO+250EAxGw Date: Fri, 27 Feb 2015 11:35:47 +0000 Message-ID: References: <1414984030-13859-1-git-send-email-hock.leong.kweh@intel.com> <1414984030-13859-4-git-send-email-hock.leong.kweh@intel.com> In-Reply-To: Accept-Language: en-US Content-Language: en-US X-MS-Has-Attach: X-MS-TNEF-Correlator: x-originating-ip: [172.30.20.205] Content-Type: text/plain; charset="utf-8" MIME-Version: 1.0 Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org Content-Transfer-Encoding: 8bit X-MIME-Autoconverted: from base64 to 8bit by nfs id t1RBa0Av008525 Content-Length: 3803 Lines: 103 > -----Original Message----- > From: Roy Franz [mailto:roy.franz@linaro.org] > 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?+????{??G?????{ay?ʇڙ?,j??f???h?????????z_??(?階?ݢj"???m??????G????????????&???~???iO???z??v?^?m???? ????????I?