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

[Heinrich Schuchardt]

On 5/1/20 9:06 AM, Heinrich Schuchardt wrote:
> 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
>
>>>> +}
>>>>
>

This patch is merged adding function description and adjusting return to
be bool. Thanks Heinrich