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

[Heinrich Schuchardt]

On 4/28/20 1:54 AM, AKASHI Takahiro wrote:
> 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'

It is efi_delete_handle() that relies on handle pointing to the start of
the EFI objects.

> So use of container_of has no disadvantage.

The generated assembler code is the same.

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

As you decided that the return value shall be boolean you have to use:

return !!(info.bootable & PART_EFI_SYSTEM_PARTITION);

Best regards

Heinrich

>>> +}
>>>