RE: [PATCH v2 3/3] efi: Capsule update with user helper interface

[Kweh, Hock Leong]

> -----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