| Message ID | 20200427094829.1140-2-takahiro.akashi@linaro.org |
|---|---|
| State | Accepted, archived |
| Delegated to: | Heinrich Schuchardt |
| Headers | show |
| Series | efi_loader: add capsule update support | expand |
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@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; > +} >
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@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; > > +} > >
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@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 >>> +} >>>
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@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 --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; +}
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@linaro.org> --- include/efi_loader.h | 2 ++ lib/efi_loader/efi_disk.c | 22 ++++++++++++++++++++++ 2 files changed, 24 insertions(+)