[PATCH 01/10] efi_loader: disk: add efi_disk_is_system_part()

[AKASHI Takahiro]

Heinrich,

On Mon, Apr 27, 2020 at 09:57:25PM +0200, Heinrich Schuchardt wrote:
> On 4/27/20 11:48 AM, AKASHI Takahiro wrote:
> > This function will check if a given handle to device is a EFI system
> > partition. It will be utilised in implementing capsule-on-disk feature.
> >
> > Signed-off-by: AKASHI Takahiro <takahiro.akashi at linaro.org>
> 
> Just a reminder for me:
> 
> This patch depends on
> "part: detect EFI system partition"
> https://lists.denx.de/pipermail/u-boot/2020-April/408150.html

Yes, and I explicitly mentioned it in "prerequisite patches"
of the cover letter.
(I objected to the patch 2/2 though.)

> > ---
> >  include/efi_loader.h      |  2 ++
> >  lib/efi_loader/efi_disk.c | 22 ++++++++++++++++++++++
> >  2 files changed, 24 insertions(+)
> >
> > diff --git a/include/efi_loader.h b/include/efi_loader.h
> > index b7bccf50b30c..d4510462d616 100644
> > --- a/include/efi_loader.h
> > +++ b/include/efi_loader.h
> > @@ -393,6 +393,8 @@ efi_status_t efi_disk_register(void);
> >  int efi_disk_create_partitions(efi_handle_t parent, struct blk_desc *desc,
> >  			       const char *if_typename, int diskid,
> >  			       const char *pdevname);
> > +/* Check if it is EFI system partition */
> > +bool efi_disk_is_system_part(efi_handle_t handle);
> >  /* Called by bootefi to make GOP (graphical) interface available */
> >  efi_status_t efi_gop_register(void);
> >  /* Called by bootefi to make the network interface available */
> > diff --git a/lib/efi_loader/efi_disk.c b/lib/efi_loader/efi_disk.c
> > index fd3df80b0b96..9f58a8642c85 100644
> > --- a/lib/efi_loader/efi_disk.c
> > +++ b/lib/efi_loader/efi_disk.c
> > @@ -588,3 +588,25 @@ efi_status_t efi_disk_register(void)
> >
> >  	return EFI_SUCCESS;
> >  }
> > +
> 
> Please, provide a function description, cf.
> https://www.kernel.org/doc/html/latest/doc-guide/kernel-doc.html#function-documentation

That is the thing that I've forgot to address in this version.

> > +bool efi_disk_is_system_part(efi_handle_t handle)
> > +{
> > +	struct efi_handler *handler;
> > +	struct efi_disk_obj *diskobj;
> > +	disk_partition_t info;
> > +	efi_status_t ret;
> > +	int r;
> > +
> > +	/* check if this is a block device */
> > +	ret = efi_search_protocol(handle, &efi_block_io_guid, &handler);
> > +	if (ret != EFI_SUCCESS)
> > +		return false;
> > +
> > +	diskobj = container_of(handle, struct efi_disk_obj, header);
> 
> This is just a complicated way of saying:
> 
> diskobj = (struct efi_disk_obj *)handle;
> 
> I would rather avoid container_of() here though it is not wrong.

My code doesn't rely on the fact that 'handle' is the first
element of 'struct efi_disk_obj'
So use of container_of has no disadvantage.

Thanks,
-Takahiro Akashi


> Best regards
> 
> Heinrich
> 
> > +
> > +	r = part_get_info(diskobj->desc, diskobj->part, &info);
> > +	if (r)
> > +		return false;
> > +
> > +	return info.bootable & PART_EFI_SYSTEM_PARTITION;
> > +}
> >