Re: [RFC PATCH] Add esrt support.

[Peter Jones <pjones-H+wXaHxf7aLQT0dZR+AlfA@public.gmane.org>]

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-H+wXaHxf7aLQT0dZR+AlfA@public.gmane.org>
> > ---
> >  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