diff mbox series

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

Message ID 20200427094829.1140-2-takahiro.akashi@linaro.org
State Accepted
Commit 41fd506842c2d9385d940cffe8ceeb8456c29fc5
Headers show
Series efi_loader: add capsule update support | expand

Commit Message

AKASHI Takahiro April 27, 2020, 9:48 a.m. UTC
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>
---
 include/efi_loader.h      |  2 ++
 lib/efi_loader/efi_disk.c | 22 ++++++++++++++++++++++
 2 files changed, 24 insertions(+)

Comments

Heinrich Schuchardt April 27, 2020, 7:57 p.m. UTC | #1
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

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

> +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.

Best regards

Heinrich

> +
> +	r = part_get_info(diskobj->desc, diskobj->part, &info);
> +	if (r)
> +		return false;
> +
> +	return info.bootable & PART_EFI_SYSTEM_PARTITION;
> +}
>
AKASHI Takahiro April 27, 2020, 11:54 p.m. UTC | #2
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;
> > +}
> >
Heinrich Schuchardt May 1, 2020, 7:06 a.m. UTC | #3
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

>>> +}
>>>
Heinrich Schuchardt May 5, 2020, 10:33 p.m. UTC | #4
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
diff mbox series

Patch

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;
 }
+
+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);
+
+	r = part_get_info(diskobj->desc, diskobj->part, &info);
+	if (r)
+		return false;
+
+	return info.bootable & PART_EFI_SYSTEM_PARTITION;
+}