lkml.org 
[lkml]   [2015]   [Feb]   [27]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
/
From
SubjectRE: [PATCH v2 3/3] efi: Capsule update with user helper interface
Date
> -----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
\
 
 \ /
  Last update: 2015-02-27 12:41    [W:0.144 / U:0.168 seconds]
©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site