On Mon, Jan 05, 2015 at 01:51:13PM +0000, Matt Fleming wrote:
> On Thu, 11 Dec, at 03:22:04PM, Peter Jones wrote:
> > Add sysfs files for EFI System Resource Table under
> > /sys/firmware/efi/esrt and for each EFI System Resource Entry under
> > entries/ as a subdir.
> >
> > v2 with suggestions from bpetkov.
> > v3 with me remembering checkpatch.
> > v4 without me typing struct decls completely wrong somehow.
> >
> > Signed-off-by: Peter Jones <pjones@xxxxxxxxxx>
> > ---
> > drivers/firmware/efi/Makefile | 2 +-
> > drivers/firmware/efi/efi.c | 46 ++++-
> > drivers/firmware/efi/esrt.c | 393 ++++++++++++++++++++++++++++++++++++++++++
> > include/linux/efi.h | 6 +
> > 4 files changed, 445 insertions(+), 2 deletions(-)
> > create mode 100644 drivers/firmware/efi/esrt.c
>
> When this patch moves out of RFC I'd ideally like to see a much more
> expansive commit message.
>
> Why do we want this functionality, what's it going to be used for? What
> does the patch actaully do? Where can we go looking for more
> information about ERST? You'll probably end up copying and pasting
> things from the top of erst.c, but that's fine.
Yeah, fair enough.
>
> > @@ -220,6 +223,46 @@ err_put:
> >
> > subsys_initcall(efisubsys_init);
> >
> > +/*
> > + * Given a physicall address, determine if it exists within an EFI Memory Map
> > + * entry, and if so, how much of that map exists at a higher address. That
> > + * is, if this is the address of something in an EFI map, what's the highest
> > + * address at which it's likely to end.
> > + */
> > +u64 efi_mem_max_reasonable_size(u64 phys_addr)
>
> Confused. I think I know what you're saying here, but I'm also doing a
> fair bit of a guesswork.
>
> Instead, how about,
>
> extern struct efi_memory_desc_t *efi_mem_desc_lookup(u64 phys_addr);
>
> /*
> * Return the end physical address of a EFI memory map region
> */
> static inline u64 efi_mem_desc_end(struct efi_memory_desc_t *md)
> {
> u64 size = md->num_pages << EFI_PAGE_SHIFT;
>
> return md->phys_addr + size;
> }
>
> The caller can then do,
>
> md = efi_mem_desc_lookup(phys_addr)
> if (!md)
> return -1;
>
> bytes = efi_mem_desc_end(md) - phys_addr;
> ioremap(phys_addr, bytes);
>
> I think leaving this "remaining bytes" logic in the ioremap() caller is
> the most robust solution because it's a quirky thing to want to do,
> whereas the other functions (efi_mem_desc_lookup() and
> efi_mem_desc_end()) are more general.
>
> Thoughts?
I can be okay with this.
>
> [...]
>
> > +static inline int esrt_table_exists(void)
> > +{
> > + if (!efi_enabled(EFI_CONFIG_TABLES))
> > + return 0;
> > + if (efi.esrt == EFI_INVALID_TABLE_ADDR)
> > + return 0;
> > + return 1;
> > +}
>
> Minor detail: this should be using 'bool'.
Thanks. I'll send a new patch soon.
--
Peter
--
To unsubscribe from this list: send the line "unsubscribe linux-efi" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
