* [U-Boot] [PATCH v2 0/3] efi_loader: disk: install FILE_SYSTEM_PROTOCOL to whole disk
@ 2019-09-12 4:51 AKASHI Takahiro
2019-09-12 4:51 ` [U-Boot] [PATCH v2 1/3] fs: export fs_close() AKASHI Takahiro
` (2 more replies)
0 siblings, 3 replies; 13+ messages in thread
From: AKASHI Takahiro @ 2019-09-12 4:51 UTC (permalink / raw)
To: u-boot
Changes in v2 (Sept 12, 2019)
* add patch#1 and #2
* install the protocol only if a file system does exist
AKASHI Takahiro (3):
fs: export fs_close()
efi_loader: disk: install FILE_SYSTEM_PROTOCOL only if available
efi_loader: disk: install file system protocol to a whole disk
fs/fs.c | 2 +-
include/fs.h | 7 +++++++
lib/efi_loader/efi_disk.c | 19 ++++++++++++++++++-
3 files changed, 26 insertions(+), 2 deletions(-)
--
2.21.0
^ permalink raw reply [flat|nested] 13+ messages in thread* [U-Boot] [PATCH v2 1/3] fs: export fs_close() 2019-09-12 4:51 [U-Boot] [PATCH v2 0/3] efi_loader: disk: install FILE_SYSTEM_PROTOCOL to whole disk AKASHI Takahiro @ 2019-09-12 4:51 ` AKASHI Takahiro 2019-09-12 4:51 ` [U-Boot] [PATCH v2 2/3] efi_loader: disk: install FILE_SYSTEM_PROTOCOL only if available AKASHI Takahiro 2019-09-12 4:51 ` [U-Boot] [PATCH v2 3/3] efi_loader: disk: install file system protocol to a whole disk AKASHI Takahiro 2 siblings, 0 replies; 13+ messages in thread From: AKASHI Takahiro @ 2019-09-12 4:51 UTC (permalink / raw) To: u-boot This function is always paired with either fs_set_blk_desc() or fs_set_blk_desc_with_part(). So just export it. Signed-off-by: AKASHI Takahiro <takahiro.akashi@linaro.org> --- fs/fs.c | 2 +- include/fs.h | 7 +++++++ 2 files changed, 8 insertions(+), 1 deletion(-) diff --git a/fs/fs.c b/fs/fs.c index d8a4ced4698e..64ba25fea8bf 100644 --- a/fs/fs.c +++ b/fs/fs.c @@ -389,7 +389,7 @@ int fs_set_blk_dev_with_part(struct blk_desc *desc, int part) return -1; } -static void fs_close(void) +void fs_close(void) { struct fstype_info *info = fs_get_info(fs_type); diff --git a/include/fs.h b/include/fs.h index 7601b0343bcd..5a1244d57fd2 100644 --- a/include/fs.h +++ b/include/fs.h @@ -37,6 +37,13 @@ int fs_set_blk_dev(const char *ifname, const char *dev_part_str, int fstype); */ int fs_set_blk_dev_with_part(struct blk_desc *desc, int part); +/** + * fs_close() - Unset current block device and partition + * + * Should be paired with either fs_set_blk_dev() or fs_set_dev_with_part() + */ +void fs_close(void); + /** * fs_get_type_name() - Get type of current filesystem * -- 2.21.0 ^ permalink raw reply related [flat|nested] 13+ messages in thread
* [U-Boot] [PATCH v2 2/3] efi_loader: disk: install FILE_SYSTEM_PROTOCOL only if available 2019-09-12 4:51 [U-Boot] [PATCH v2 0/3] efi_loader: disk: install FILE_SYSTEM_PROTOCOL to whole disk AKASHI Takahiro 2019-09-12 4:51 ` [U-Boot] [PATCH v2 1/3] fs: export fs_close() AKASHI Takahiro @ 2019-09-12 4:51 ` AKASHI Takahiro 2019-09-12 8:57 ` Heinrich Schuchardt 2019-09-12 4:51 ` [U-Boot] [PATCH v2 3/3] efi_loader: disk: install file system protocol to a whole disk AKASHI Takahiro 2 siblings, 1 reply; 13+ messages in thread From: AKASHI Takahiro @ 2019-09-12 4:51 UTC (permalink / raw) To: u-boot In the current implementation, EFI_SIMPLEFILE_SYSTEM_PROTOCOL is always installed to all the partitions even if some of them may house no file system. With this patch, that protocol will be installed only if FAT file system exists. Signed-off-by: AKASHI Takahiro <takahiro.akashi@linaro.org> --- lib/efi_loader/efi_disk.c | 16 +++++++++++++++- 1 file changed, 15 insertions(+), 1 deletion(-) diff --git a/lib/efi_loader/efi_disk.c b/lib/efi_loader/efi_disk.c index 7a6b06821a47..d72f455901f2 100644 --- a/lib/efi_loader/efi_disk.c +++ b/lib/efi_loader/efi_disk.c @@ -9,6 +9,7 @@ #include <blk.h> #include <dm.h> #include <efi_loader.h> +#include <fs.h> #include <part.h> #include <malloc.h> @@ -217,6 +218,19 @@ efi_fs_from_path(struct efi_device_path *full_path) return handler->protocol_interface; } +static int efi_fs_exists(struct blk_desc *desc, int part) +{ + if (fs_set_blk_dev_with_part(desc, part)) + return 0; + + if (strcmp(fs_get_type_name(), "fat")) + return 0; + + fs_close(); + + return 1; +} + /* * Create a handle for a partition or disk * @@ -270,7 +284,7 @@ static efi_status_t efi_disk_add_dev( diskobj->dp); if (ret != EFI_SUCCESS) return ret; - if (part >= 1) { + if (part >= 1 && efi_fs_exists(desc, part)) { diskobj->volume = efi_simple_file_system(desc, part, diskobj->dp); ret = efi_add_protocol(&diskobj->header, -- 2.21.0 ^ permalink raw reply related [flat|nested] 13+ messages in thread
* [U-Boot] [PATCH v2 2/3] efi_loader: disk: install FILE_SYSTEM_PROTOCOL only if available 2019-09-12 4:51 ` [U-Boot] [PATCH v2 2/3] efi_loader: disk: install FILE_SYSTEM_PROTOCOL only if available AKASHI Takahiro @ 2019-09-12 8:57 ` Heinrich Schuchardt 2019-09-12 9:17 ` AKASHI Takahiro 0 siblings, 1 reply; 13+ messages in thread From: Heinrich Schuchardt @ 2019-09-12 8:57 UTC (permalink / raw) To: u-boot On 9/12/19 6:51 AM, AKASHI Takahiro wrote: > In the current implementation, EFI_SIMPLEFILE_SYSTEM_PROTOCOL is always > installed to all the partitions even if some of them may house no file > system. > > With this patch, that protocol will be installed only if FAT file system > exists. > > Signed-off-by: AKASHI Takahiro <takahiro.akashi@linaro.org> > --- > lib/efi_loader/efi_disk.c | 16 +++++++++++++++- > 1 file changed, 15 insertions(+), 1 deletion(-) > > diff --git a/lib/efi_loader/efi_disk.c b/lib/efi_loader/efi_disk.c > index 7a6b06821a47..d72f455901f2 100644 > --- a/lib/efi_loader/efi_disk.c > +++ b/lib/efi_loader/efi_disk.c > @@ -9,6 +9,7 @@ > #include <blk.h> > #include <dm.h> > #include <efi_loader.h> > +#include <fs.h> > #include <part.h> > #include <malloc.h> > > @@ -217,6 +218,19 @@ efi_fs_from_path(struct efi_device_path *full_path) > return handler->protocol_interface; > } > > +static int efi_fs_exists(struct blk_desc *desc, int part) > +{ > + if (fs_set_blk_dev_with_part(desc, part)) > + return 0; > + > + if (strcmp(fs_get_type_name(), "fat")) Before your patch we could use any supported file system (e.g. EXT2). I see no need for a restriction to FAT. You could compare the string to "unsupported": if (!strcmp(fs_get_type_name(), "unsupported")) return 0; But wouldn't it be preferable to have a function to access fs_type (in fs/fs.c) directly instead of a string representation? Otherwise we should convert the string "unsupported" of fstypes[] into a constant in fs.h so that we can be sure we are using the same value. Best regards Heinrich > + return 0; > + > + fs_close(); > + > + return 1; > +} > + > /* > * Create a handle for a partition or disk > * > @@ -270,7 +284,7 @@ static efi_status_t efi_disk_add_dev( > diskobj->dp); > if (ret != EFI_SUCCESS) > return ret; > - if (part >= 1) { > + if (part >= 1 && efi_fs_exists(desc, part)) { > diskobj->volume = efi_simple_file_system(desc, part, > diskobj->dp); > ret = efi_add_protocol(&diskobj->header, > ^ permalink raw reply [flat|nested] 13+ messages in thread
* [U-Boot] [PATCH v2 2/3] efi_loader: disk: install FILE_SYSTEM_PROTOCOL only if available 2019-09-12 8:57 ` Heinrich Schuchardt @ 2019-09-12 9:17 ` AKASHI Takahiro 2019-09-12 9:43 ` Heinrich Schuchardt 0 siblings, 1 reply; 13+ messages in thread From: AKASHI Takahiro @ 2019-09-12 9:17 UTC (permalink / raw) To: u-boot On Thu, Sep 12, 2019 at 10:57:20AM +0200, Heinrich Schuchardt wrote: > On 9/12/19 6:51 AM, AKASHI Takahiro wrote: > > In the current implementation, EFI_SIMPLEFILE_SYSTEM_PROTOCOL is always > > installed to all the partitions even if some of them may house no file > > system. > > > > With this patch, that protocol will be installed only if FAT file system > > exists. > > > > Signed-off-by: AKASHI Takahiro <takahiro.akashi@linaro.org> > > --- > > lib/efi_loader/efi_disk.c | 16 +++++++++++++++- > > 1 file changed, 15 insertions(+), 1 deletion(-) > > > > diff --git a/lib/efi_loader/efi_disk.c b/lib/efi_loader/efi_disk.c > > index 7a6b06821a47..d72f455901f2 100644 > > --- a/lib/efi_loader/efi_disk.c > > +++ b/lib/efi_loader/efi_disk.c > > @@ -9,6 +9,7 @@ > > #include <blk.h> > > #include <dm.h> > > #include <efi_loader.h> > > +#include <fs.h> > > #include <part.h> > > #include <malloc.h> > > > > @@ -217,6 +218,19 @@ efi_fs_from_path(struct efi_device_path *full_path) > > return handler->protocol_interface; > > } > > > > +static int efi_fs_exists(struct blk_desc *desc, int part) > > +{ > > + if (fs_set_blk_dev_with_part(desc, part)) > > + return 0; > > + > > + if (strcmp(fs_get_type_name(), "fat")) > > Before your patch we could use any supported file system (e.g. EXT2). I > see no need for a restriction to FAT. You could compare the string to > "unsupported": No. As far as you want to stick to compliance to UEFI specification, "fat" is the only file system supported by UEFI. > if (!strcmp(fs_get_type_name(), "unsupported")) > return 0; > > But wouldn't it be preferable to have a function to access fs_type (in > fs/fs.c) directly instead of a string representation? Agree, but there is no direct function in fs/fs.c. I'm reluctant to invent a new function just for this purpose. -Takahiro Akashi > Otherwise we should convert the string "unsupported" of fstypes[] into a > constant in fs.h so that we can be sure we are using the same value. > > Best regards > > Heinrich > > > + return 0; > > + > > + fs_close(); > > + > > + return 1; > > +} > > + > > /* > > * Create a handle for a partition or disk > > * > > @@ -270,7 +284,7 @@ static efi_status_t efi_disk_add_dev( > > diskobj->dp); > > if (ret != EFI_SUCCESS) > > return ret; > > - if (part >= 1) { > > + if (part >= 1 && efi_fs_exists(desc, part)) { > > diskobj->volume = efi_simple_file_system(desc, part, > > diskobj->dp); > > ret = efi_add_protocol(&diskobj->header, > > > ^ permalink raw reply [flat|nested] 13+ messages in thread
* [U-Boot] [PATCH v2 2/3] efi_loader: disk: install FILE_SYSTEM_PROTOCOL only if available 2019-09-12 9:17 ` AKASHI Takahiro @ 2019-09-12 9:43 ` Heinrich Schuchardt 2019-09-12 9:51 ` AKASHI Takahiro 0 siblings, 1 reply; 13+ messages in thread From: Heinrich Schuchardt @ 2019-09-12 9:43 UTC (permalink / raw) To: u-boot On 9/12/19 11:17 AM, AKASHI Takahiro wrote: > On Thu, Sep 12, 2019 at 10:57:20AM +0200, Heinrich Schuchardt wrote: >> On 9/12/19 6:51 AM, AKASHI Takahiro wrote: >>> In the current implementation, EFI_SIMPLEFILE_SYSTEM_PROTOCOL is always >>> installed to all the partitions even if some of them may house no file >>> system. >>> >>> With this patch, that protocol will be installed only if FAT file system >>> exists. >>> >>> Signed-off-by: AKASHI Takahiro <takahiro.akashi@linaro.org> >>> --- >>> lib/efi_loader/efi_disk.c | 16 +++++++++++++++- >>> 1 file changed, 15 insertions(+), 1 deletion(-) >>> >>> diff --git a/lib/efi_loader/efi_disk.c b/lib/efi_loader/efi_disk.c >>> index 7a6b06821a47..d72f455901f2 100644 >>> --- a/lib/efi_loader/efi_disk.c >>> +++ b/lib/efi_loader/efi_disk.c >>> @@ -9,6 +9,7 @@ >>> #include <blk.h> >>> #include <dm.h> >>> #include <efi_loader.h> >>> +#include <fs.h> >>> #include <part.h> >>> #include <malloc.h> >>> >>> @@ -217,6 +218,19 @@ efi_fs_from_path(struct efi_device_path *full_path) >>> return handler->protocol_interface; >>> } >>> >>> +static int efi_fs_exists(struct blk_desc *desc, int part) >>> +{ >>> + if (fs_set_blk_dev_with_part(desc, part)) >>> + return 0; >>> + >>> + if (strcmp(fs_get_type_name(), "fat")) >> >> Before your patch we could use any supported file system (e.g. EXT2). I >> see no need for a restriction to FAT. You could compare the string to >> "unsupported": > > No. As far as you want to stick to compliance to UEFI specification, > "fat" is the only file system supported by UEFI. In the case of device path node VenHw() there is a direct rule in the spec indicating how it should be rendered. I have not seen anything in the UEFI spec saying that you should not support file systems besides FAT. So there is no compliance issue. I would be reluctant to remove an existing capability of U-Boot. > >> if (!strcmp(fs_get_type_name(), "unsupported")) >> return 0; >> >> But wouldn't it be preferable to have a function to access fs_type (in >> fs/fs.c) directly instead of a string representation? > > Agree, but there is no direct function in fs/fs.c. > I'm reluctant to invent a new function just for this purpose. In that case we should compare to a string that is defined as constant in fs.h. Best regards Heinrich > > -Takahiro Akashi > > >> Otherwise we should convert the string "unsupported" of fstypes[] into a >> constant in fs.h so that we can be sure we are using the same value. >> >> Best regards >> >> Heinrich >> >>> + return 0; >>> + >>> + fs_close(); >>> + >>> + return 1; >>> +} >>> + >>> /* >>> * Create a handle for a partition or disk >>> * >>> @@ -270,7 +284,7 @@ static efi_status_t efi_disk_add_dev( >>> diskobj->dp); >>> if (ret != EFI_SUCCESS) >>> return ret; >>> - if (part >= 1) { >>> + if (part >= 1 && efi_fs_exists(desc, part)) { >>> diskobj->volume = efi_simple_file_system(desc, part, >>> diskobj->dp); >>> ret = efi_add_protocol(&diskobj->header, >>> >> > ^ permalink raw reply [flat|nested] 13+ messages in thread
* [U-Boot] [PATCH v2 2/3] efi_loader: disk: install FILE_SYSTEM_PROTOCOL only if available 2019-09-12 9:43 ` Heinrich Schuchardt @ 2019-09-12 9:51 ` AKASHI Takahiro 2019-10-03 7:09 ` AKASHI Takahiro 0 siblings, 1 reply; 13+ messages in thread From: AKASHI Takahiro @ 2019-09-12 9:51 UTC (permalink / raw) To: u-boot On Thu, Sep 12, 2019 at 11:43:07AM +0200, Heinrich Schuchardt wrote: > On 9/12/19 11:17 AM, AKASHI Takahiro wrote: > > On Thu, Sep 12, 2019 at 10:57:20AM +0200, Heinrich Schuchardt wrote: > >> On 9/12/19 6:51 AM, AKASHI Takahiro wrote: > >>> In the current implementation, EFI_SIMPLEFILE_SYSTEM_PROTOCOL is always > >>> installed to all the partitions even if some of them may house no file > >>> system. > >>> > >>> With this patch, that protocol will be installed only if FAT file system > >>> exists. > >>> > >>> Signed-off-by: AKASHI Takahiro <takahiro.akashi@linaro.org> > >>> --- > >>> lib/efi_loader/efi_disk.c | 16 +++++++++++++++- > >>> 1 file changed, 15 insertions(+), 1 deletion(-) > >>> > >>> diff --git a/lib/efi_loader/efi_disk.c b/lib/efi_loader/efi_disk.c > >>> index 7a6b06821a47..d72f455901f2 100644 > >>> --- a/lib/efi_loader/efi_disk.c > >>> +++ b/lib/efi_loader/efi_disk.c > >>> @@ -9,6 +9,7 @@ > >>> #include <blk.h> > >>> #include <dm.h> > >>> #include <efi_loader.h> > >>> +#include <fs.h> > >>> #include <part.h> > >>> #include <malloc.h> > >>> > >>> @@ -217,6 +218,19 @@ efi_fs_from_path(struct efi_device_path *full_path) > >>> return handler->protocol_interface; > >>> } > >>> > >>> +static int efi_fs_exists(struct blk_desc *desc, int part) > >>> +{ > >>> + if (fs_set_blk_dev_with_part(desc, part)) > >>> + return 0; > >>> + > >>> + if (strcmp(fs_get_type_name(), "fat")) > >> > >> Before your patch we could use any supported file system (e.g. EXT2). I > >> see no need for a restriction to FAT. You could compare the string to > >> "unsupported": > > > > No. As far as you want to stick to compliance to UEFI specification, > > "fat" is the only file system supported by UEFI. > > In the case of device path node VenHw() there is a direct rule in the > spec indicating how it should be rendered. I have not seen anything in > the UEFI spec saying that you should not support file systems besides > FAT. So there is no compliance issue. I would be reluctant to remove an > existing capability of U-Boot. See section 13.3. It says, The file system supported by the Extensible Firmware Interface is based on the FAT file system. -Takahiro Akashi > > > >> if (!strcmp(fs_get_type_name(), "unsupported")) > >> return 0; > >> > >> But wouldn't it be preferable to have a function to access fs_type (in > >> fs/fs.c) directly instead of a string representation? > > > > Agree, but there is no direct function in fs/fs.c. > > I'm reluctant to invent a new function just for this purpose. > > In that case we should compare to a string that is defined as constant > in fs.h. > > Best regards > > Heinrich > > > > > -Takahiro Akashi > > > > > >> Otherwise we should convert the string "unsupported" of fstypes[] into a > >> constant in fs.h so that we can be sure we are using the same value. > >> > >> Best regards > >> > >> Heinrich > >> > >>> + return 0; > >>> + > >>> + fs_close(); > >>> + > >>> + return 1; > >>> +} > >>> + > >>> /* > >>> * Create a handle for a partition or disk > >>> * > >>> @@ -270,7 +284,7 @@ static efi_status_t efi_disk_add_dev( > >>> diskobj->dp); > >>> if (ret != EFI_SUCCESS) > >>> return ret; > >>> - if (part >= 1) { > >>> + if (part >= 1 && efi_fs_exists(desc, part)) { > >>> diskobj->volume = efi_simple_file_system(desc, part, > >>> diskobj->dp); > >>> ret = efi_add_protocol(&diskobj->header, > >>> > >> > > > ^ permalink raw reply [flat|nested] 13+ messages in thread
* [U-Boot] [PATCH v2 2/3] efi_loader: disk: install FILE_SYSTEM_PROTOCOL only if available 2019-09-12 9:51 ` AKASHI Takahiro @ 2019-10-03 7:09 ` AKASHI Takahiro 2019-10-03 13:44 ` Heinrich Schuchardt 0 siblings, 1 reply; 13+ messages in thread From: AKASHI Takahiro @ 2019-10-03 7:09 UTC (permalink / raw) To: u-boot Heinrich, On Thu, Sep 12, 2019 at 06:51:35PM +0900, AKASHI Takahiro wrote: > On Thu, Sep 12, 2019 at 11:43:07AM +0200, Heinrich Schuchardt wrote: > > On 9/12/19 11:17 AM, AKASHI Takahiro wrote: > > > On Thu, Sep 12, 2019 at 10:57:20AM +0200, Heinrich Schuchardt wrote: > > >> On 9/12/19 6:51 AM, AKASHI Takahiro wrote: > > >>> In the current implementation, EFI_SIMPLEFILE_SYSTEM_PROTOCOL is always > > >>> installed to all the partitions even if some of them may house no file > > >>> system. > > >>> > > >>> With this patch, that protocol will be installed only if FAT file system > > >>> exists. > > >>> > > >>> Signed-off-by: AKASHI Takahiro <takahiro.akashi@linaro.org> > > >>> --- > > >>> lib/efi_loader/efi_disk.c | 16 +++++++++++++++- > > >>> 1 file changed, 15 insertions(+), 1 deletion(-) > > >>> > > >>> diff --git a/lib/efi_loader/efi_disk.c b/lib/efi_loader/efi_disk.c > > >>> index 7a6b06821a47..d72f455901f2 100644 > > >>> --- a/lib/efi_loader/efi_disk.c > > >>> +++ b/lib/efi_loader/efi_disk.c > > >>> @@ -9,6 +9,7 @@ > > >>> #include <blk.h> > > >>> #include <dm.h> > > >>> #include <efi_loader.h> > > >>> +#include <fs.h> > > >>> #include <part.h> > > >>> #include <malloc.h> > > >>> > > >>> @@ -217,6 +218,19 @@ efi_fs_from_path(struct efi_device_path *full_path) > > >>> return handler->protocol_interface; > > >>> } > > >>> > > >>> +static int efi_fs_exists(struct blk_desc *desc, int part) > > >>> +{ > > >>> + if (fs_set_blk_dev_with_part(desc, part)) > > >>> + return 0; > > >>> + > > >>> + if (strcmp(fs_get_type_name(), "fat")) > > >> > > >> Before your patch we could use any supported file system (e.g. EXT2). I > > >> see no need for a restriction to FAT. You could compare the string to > > >> "unsupported": > > > > > > No. As far as you want to stick to compliance to UEFI specification, > > > "fat" is the only file system supported by UEFI. > > > > In the case of device path node VenHw() there is a direct rule in the > > spec indicating how it should be rendered. I have not seen anything in > > the UEFI spec saying that you should not support file systems besides > > FAT. So there is no compliance issue. I would be reluctant to remove an > > existing capability of U-Boot. > > See section 13.3. It says, > The file system supported by the Extensible Firmware Interface is > based on the FAT file system. Any comments here? > > > > > >> if (!strcmp(fs_get_type_name(), "unsupported")) > > >> return 0; > > >> > > >> But wouldn't it be preferable to have a function to access fs_type (in > > >> fs/fs.c) directly instead of a string representation? > > > > > > Agree, but there is no direct function in fs/fs.c. > > > I'm reluctant to invent a new function just for this purpose. > > > > In that case we should compare to a string that is defined as constant > > in fs.h. Are you saying that we should add new macros in fs.h? All the file system names are initialized in fs/fs.c now. > -Takahiro Akashi > > > > > Best regards > > > > Heinrich > > > > > > > > -Takahiro Akashi > > > > > > > > >> Otherwise we should convert the string "unsupported" of fstypes[] into a > > >> constant in fs.h so that we can be sure we are using the same value. > > >> > > >> Best regards > > >> > > >> Heinrich > > >> > > >>> + return 0; > > >>> + > > >>> + fs_close(); > > >>> + > > >>> + return 1; > > >>> +} > > >>> + > > >>> /* > > >>> * Create a handle for a partition or disk > > >>> * > > >>> @@ -270,7 +284,7 @@ static efi_status_t efi_disk_add_dev( > > >>> diskobj->dp); > > >>> if (ret != EFI_SUCCESS) > > >>> return ret; > > >>> - if (part >= 1) { > > >>> + if (part >= 1 && efi_fs_exists(desc, part)) { > > >>> diskobj->volume = efi_simple_file_system(desc, part, > > >>> diskobj->dp); > > >>> ret = efi_add_protocol(&diskobj->header, > > >>> > > >> > > > > > ^ permalink raw reply [flat|nested] 13+ messages in thread
* [U-Boot] [PATCH v2 2/3] efi_loader: disk: install FILE_SYSTEM_PROTOCOL only if available 2019-10-03 7:09 ` AKASHI Takahiro @ 2019-10-03 13:44 ` Heinrich Schuchardt 2019-10-04 0:22 ` AKASHI Takahiro 0 siblings, 1 reply; 13+ messages in thread From: Heinrich Schuchardt @ 2019-10-03 13:44 UTC (permalink / raw) To: u-boot On 10/3/19 9:09 AM, AKASHI Takahiro wrote: > Heinrich, > > On Thu, Sep 12, 2019 at 06:51:35PM +0900, AKASHI Takahiro wrote: >> On Thu, Sep 12, 2019 at 11:43:07AM +0200, Heinrich Schuchardt wrote: >>> On 9/12/19 11:17 AM, AKASHI Takahiro wrote: >>>> On Thu, Sep 12, 2019 at 10:57:20AM +0200, Heinrich Schuchardt wrote: >>>>> On 9/12/19 6:51 AM, AKASHI Takahiro wrote: >>>>>> In the current implementation, EFI_SIMPLEFILE_SYSTEM_PROTOCOL is always >>>>>> installed to all the partitions even if some of them may house no file >>>>>> system. >>>>>> >>>>>> With this patch, that protocol will be installed only if FAT file system >>>>>> exists. >>>>>> >>>>>> Signed-off-by: AKASHI Takahiro <takahiro.akashi@linaro.org> >>>>>> --- >>>>>> lib/efi_loader/efi_disk.c | 16 +++++++++++++++- >>>>>> 1 file changed, 15 insertions(+), 1 deletion(-) >>>>>> >>>>>> diff --git a/lib/efi_loader/efi_disk.c b/lib/efi_loader/efi_disk.c >>>>>> index 7a6b06821a47..d72f455901f2 100644 >>>>>> --- a/lib/efi_loader/efi_disk.c >>>>>> +++ b/lib/efi_loader/efi_disk.c >>>>>> @@ -9,6 +9,7 @@ >>>>>> #include <blk.h> >>>>>> #include <dm.h> >>>>>> #include <efi_loader.h> >>>>>> +#include <fs.h> >>>>>> #include <part.h> >>>>>> #include <malloc.h> >>>>>> >>>>>> @@ -217,6 +218,19 @@ efi_fs_from_path(struct efi_device_path *full_path) >>>>>> return handler->protocol_interface; >>>>>> } >>>>>> >>>>>> +static int efi_fs_exists(struct blk_desc *desc, int part) >>>>>> +{ >>>>>> + if (fs_set_blk_dev_with_part(desc, part)) >>>>>> + return 0; >>>>>> + >>>>>> + if (strcmp(fs_get_type_name(), "fat")) >>>>> >>>>> Before your patch we could use any supported file system (e.g. EXT2). I >>>>> see no need for a restriction to FAT. You could compare the string to >>>>> "unsupported": >>>> >>>> No. As far as you want to stick to compliance to UEFI specification, >>>> "fat" is the only file system supported by UEFI. >>> >>> In the case of device path node VenHw() there is a direct rule in the >>> spec indicating how it should be rendered. I have not seen anything in >>> the UEFI spec saying that you should not support file systems besides >>> FAT. So there is no compliance issue. I would be reluctant to remove an >>> existing capability of U-Boot. >> >> See section 13.3. It says, >> The file system supported by the Extensible Firmware Interface is >> based on the FAT file system. > > Any comments here? The spec does not say: Thou shalt not support other file systems. > >>>> >>>>> if (!strcmp(fs_get_type_name(), "unsupported")) >>>>> return 0; >>>>> >>>>> But wouldn't it be preferable to have a function to access fs_type (in >>>>> fs/fs.c) directly instead of a string representation? >>>> >>>> Agree, but there is no direct function in fs/fs.c. >>>> I'm reluctant to invent a new function just for this purpose. >>> >>> In that case we should compare to a string that is defined as constant >>> in fs.h. > > Are you saying that we should add new macros in fs.h? > All the file system names are initialized in fs/fs.c now. > >> -Takahiro Akashi >> >>> >>> Best regards >>> >>> Heinrich >>> >>>> >>>> -Takahiro Akashi >>>> >>>> >>>>> Otherwise we should convert the string "unsupported" of fstypes[] into a >>>>> constant in fs.h so that we can be sure we are using the same value. >>>>> >>>>> Best regards >>>>> >>>>> Heinrich >>>>> >>>>>> + return 0; >>>>>> + >>>>>> + fs_close(); >>>>>> + >>>>>> + return 1; >>>>>> +} >>>>>> + >>>>>> /* >>>>>> * Create a handle for a partition or disk >>>>>> * >>>>>> @@ -270,7 +284,7 @@ static efi_status_t efi_disk_add_dev( >>>>>> diskobj->dp); >>>>>> if (ret != EFI_SUCCESS) >>>>>> return ret; >>>>>> - if (part >= 1) { >>>>>> + if (part >= 1 && efi_fs_exists(desc, part)) { >>>>>> diskobj->volume = efi_simple_file_system(desc, part, >>>>>> diskobj->dp); >>>>>> ret = efi_add_protocol(&diskobj->header, >>>>>> >>>>> >>>> >>> > ^ permalink raw reply [flat|nested] 13+ messages in thread
* [U-Boot] [PATCH v2 2/3] efi_loader: disk: install FILE_SYSTEM_PROTOCOL only if available 2019-10-03 13:44 ` Heinrich Schuchardt @ 2019-10-04 0:22 ` AKASHI Takahiro 0 siblings, 0 replies; 13+ messages in thread From: AKASHI Takahiro @ 2019-10-04 0:22 UTC (permalink / raw) To: u-boot On Thu, Oct 03, 2019 at 03:44:54PM +0200, Heinrich Schuchardt wrote: > On 10/3/19 9:09 AM, AKASHI Takahiro wrote: > >Heinrich, > > > >On Thu, Sep 12, 2019 at 06:51:35PM +0900, AKASHI Takahiro wrote: > >>On Thu, Sep 12, 2019 at 11:43:07AM +0200, Heinrich Schuchardt wrote: > >>>On 9/12/19 11:17 AM, AKASHI Takahiro wrote: > >>>>On Thu, Sep 12, 2019 at 10:57:20AM +0200, Heinrich Schuchardt wrote: > >>>>>On 9/12/19 6:51 AM, AKASHI Takahiro wrote: > >>>>>>In the current implementation, EFI_SIMPLEFILE_SYSTEM_PROTOCOL is always > >>>>>>installed to all the partitions even if some of them may house no file > >>>>>>system. > >>>>>> > >>>>>>With this patch, that protocol will be installed only if FAT file system > >>>>>>exists. > >>>>>> > >>>>>>Signed-off-by: AKASHI Takahiro <takahiro.akashi@linaro.org> > >>>>>>--- > >>>>>> lib/efi_loader/efi_disk.c | 16 +++++++++++++++- > >>>>>> 1 file changed, 15 insertions(+), 1 deletion(-) > >>>>>> > >>>>>>diff --git a/lib/efi_loader/efi_disk.c b/lib/efi_loader/efi_disk.c > >>>>>>index 7a6b06821a47..d72f455901f2 100644 > >>>>>>--- a/lib/efi_loader/efi_disk.c > >>>>>>+++ b/lib/efi_loader/efi_disk.c > >>>>>>@@ -9,6 +9,7 @@ > >>>>>> #include <blk.h> > >>>>>> #include <dm.h> > >>>>>> #include <efi_loader.h> > >>>>>>+#include <fs.h> > >>>>>> #include <part.h> > >>>>>> #include <malloc.h> > >>>>>> > >>>>>>@@ -217,6 +218,19 @@ efi_fs_from_path(struct efi_device_path *full_path) > >>>>>> return handler->protocol_interface; > >>>>>> } > >>>>>> > >>>>>>+static int efi_fs_exists(struct blk_desc *desc, int part) > >>>>>>+{ > >>>>>>+ if (fs_set_blk_dev_with_part(desc, part)) > >>>>>>+ return 0; > >>>>>>+ > >>>>>>+ if (strcmp(fs_get_type_name(), "fat")) > >>>>> > >>>>>Before your patch we could use any supported file system (e.g. EXT2). I > >>>>>see no need for a restriction to FAT. You could compare the string to > >>>>>"unsupported": > >>>> > >>>>No. As far as you want to stick to compliance to UEFI specification, > >>>>"fat" is the only file system supported by UEFI. > >>> > >>>In the case of device path node VenHw() there is a direct rule in the > >>>spec indicating how it should be rendered. I have not seen anything in > >>>the UEFI spec saying that you should not support file systems besides > >>>FAT. So there is no compliance issue. I would be reluctant to remove an > >>>existing capability of U-Boot. > >> > >>See section 13.3. It says, > >> The file system supported by the Extensible Firmware Interface is > >> based on the FAT file system. > > > >Any comments here? > > The spec does not say: Thou shalt not support other file systems. If we admit such an excuse for arbitrary extension, it will easily break compatibility in general. -Takahiro Akashi > > > >>>> > >>>>>if (!strcmp(fs_get_type_name(), "unsupported")) > >>>>> return 0; > >>>>> > >>>>>But wouldn't it be preferable to have a function to access fs_type (in > >>>>>fs/fs.c) directly instead of a string representation? > >>>> > >>>>Agree, but there is no direct function in fs/fs.c. > >>>>I'm reluctant to invent a new function just for this purpose. > >>> > >>>In that case we should compare to a string that is defined as constant > >>>in fs.h. > > > >Are you saying that we should add new macros in fs.h? > >All the file system names are initialized in fs/fs.c now. > > > >>-Takahiro Akashi > >> > >>> > >>>Best regards > >>> > >>>Heinrich > >>> > >>>> > >>>>-Takahiro Akashi > >>>> > >>>> > >>>>>Otherwise we should convert the string "unsupported" of fstypes[] into a > >>>>>constant in fs.h so that we can be sure we are using the same value. > >>>>> > >>>>>Best regards > >>>>> > >>>>>Heinrich > >>>>> > >>>>>>+ return 0; > >>>>>>+ > >>>>>>+ fs_close(); > >>>>>>+ > >>>>>>+ return 1; > >>>>>>+} > >>>>>>+ > >>>>>> /* > >>>>>> * Create a handle for a partition or disk > >>>>>> * > >>>>>>@@ -270,7 +284,7 @@ static efi_status_t efi_disk_add_dev( > >>>>>> diskobj->dp); > >>>>>> if (ret != EFI_SUCCESS) > >>>>>> return ret; > >>>>>>- if (part >= 1) { > >>>>>>+ if (part >= 1 && efi_fs_exists(desc, part)) { > >>>>>> diskobj->volume = efi_simple_file_system(desc, part, > >>>>>> diskobj->dp); > >>>>>> ret = efi_add_protocol(&diskobj->header, > >>>>>> > >>>>> > >>>> > >>> > > > ^ permalink raw reply [flat|nested] 13+ messages in thread
* [U-Boot] [PATCH v2 3/3] efi_loader: disk: install file system protocol to a whole disk 2019-09-12 4:51 [U-Boot] [PATCH v2 0/3] efi_loader: disk: install FILE_SYSTEM_PROTOCOL to whole disk AKASHI Takahiro 2019-09-12 4:51 ` [U-Boot] [PATCH v2 1/3] fs: export fs_close() AKASHI Takahiro 2019-09-12 4:51 ` [U-Boot] [PATCH v2 2/3] efi_loader: disk: install FILE_SYSTEM_PROTOCOL only if available AKASHI Takahiro @ 2019-09-12 4:51 ` AKASHI Takahiro 2019-09-12 9:21 ` Heinrich Schuchardt 2 siblings, 1 reply; 13+ messages in thread From: AKASHI Takahiro @ 2019-09-12 4:51 UTC (permalink / raw) To: u-boot Currently, a whole disk without any partitions is not associated with EFI_SIMPLE_FILE_SYSTEM_PROTOCOL. So even if it houses FAT file system, there is a chance that we may not be able to access it, particularly, when accesses are to be attempted after searching that protocol against a device handle. With this patch, EFI_SIMPLE_FILE_SYSTEM_PROTOCOL is installed to such a disk if part_get_info() shows there is not partition table installed on it. Signed-off-by: AKASHI Takahiro <takahiro.akashi@linaro.org> --- lib/efi_loader/efi_disk.c | 5 ++++- 1 file changed, 4 insertions(+), 1 deletion(-) diff --git a/lib/efi_loader/efi_disk.c b/lib/efi_loader/efi_disk.c index d72f455901f2..d36f22cedc52 100644 --- a/lib/efi_loader/efi_disk.c +++ b/lib/efi_loader/efi_disk.c @@ -253,6 +253,7 @@ static efi_status_t efi_disk_add_dev( struct efi_disk_obj **disk) { struct efi_disk_obj *diskobj; + disk_partition_t info; efi_status_t ret; /* Don't add empty devices */ @@ -284,7 +285,9 @@ static efi_status_t efi_disk_add_dev( diskobj->dp); if (ret != EFI_SUCCESS) return ret; - if (part >= 1 && efi_fs_exists(desc, part)) { + /* partitions or whole disk without partitions */ + if ((part >= 1 || part_get_info(desc, part, &info)) && + efi_fs_exists(desc, part)) { diskobj->volume = efi_simple_file_system(desc, part, diskobj->dp); ret = efi_add_protocol(&diskobj->header, -- 2.21.0 ^ permalink raw reply related [flat|nested] 13+ messages in thread
* [U-Boot] [PATCH v2 3/3] efi_loader: disk: install file system protocol to a whole disk 2019-09-12 4:51 ` [U-Boot] [PATCH v2 3/3] efi_loader: disk: install file system protocol to a whole disk AKASHI Takahiro @ 2019-09-12 9:21 ` Heinrich Schuchardt 2019-10-03 7:21 ` AKASHI Takahiro 0 siblings, 1 reply; 13+ messages in thread From: Heinrich Schuchardt @ 2019-09-12 9:21 UTC (permalink / raw) To: u-boot On 9/12/19 6:51 AM, AKASHI Takahiro wrote: > Currently, a whole disk without any partitions is not associated > with EFI_SIMPLE_FILE_SYSTEM_PROTOCOL. So even if it houses FAT > file system, there is a chance that we may not be able to access > it, particularly, when accesses are to be attempted after searching > that protocol against a device handle. > > With this patch, EFI_SIMPLE_FILE_SYSTEM_PROTOCOL is installed > to such a disk if part_get_info() shows there is not partition > table installed on it. > > Signed-off-by: AKASHI Takahiro <takahiro.akashi@linaro.org> > --- > lib/efi_loader/efi_disk.c | 5 ++++- > 1 file changed, 4 insertions(+), 1 deletion(-) > > diff --git a/lib/efi_loader/efi_disk.c b/lib/efi_loader/efi_disk.c > index d72f455901f2..d36f22cedc52 100644 > --- a/lib/efi_loader/efi_disk.c > +++ b/lib/efi_loader/efi_disk.c > @@ -253,6 +253,7 @@ static efi_status_t efi_disk_add_dev( > struct efi_disk_obj **disk) > { > struct efi_disk_obj *diskobj; > + disk_partition_t info; > efi_status_t ret; > > /* Don't add empty devices */ > @@ -284,7 +285,9 @@ static efi_status_t efi_disk_add_dev( > diskobj->dp); > if (ret != EFI_SUCCESS) > return ret; > - if (part >= 1 && efi_fs_exists(desc, part)) { > + /* partitions or whole disk without partitions */ > + if ((part >= 1 || part_get_info(desc, part, &info)) && part_get_info() returns -1 for part = 0 on a DOS partioned disk. So this check does not work. Best regards Heinrich > + efi_fs_exists(desc, part)) { > diskobj->volume = efi_simple_file_system(desc, part, > diskobj->dp); > ret = efi_add_protocol(&diskobj->header, > ^ permalink raw reply [flat|nested] 13+ messages in thread
* [U-Boot] [PATCH v2 3/3] efi_loader: disk: install file system protocol to a whole disk 2019-09-12 9:21 ` Heinrich Schuchardt @ 2019-10-03 7:21 ` AKASHI Takahiro 0 siblings, 0 replies; 13+ messages in thread From: AKASHI Takahiro @ 2019-10-03 7:21 UTC (permalink / raw) To: u-boot Heinrich, On Thu, Sep 12, 2019 at 11:21:51AM +0200, Heinrich Schuchardt wrote: > On 9/12/19 6:51 AM, AKASHI Takahiro wrote: > > Currently, a whole disk without any partitions is not associated > > with EFI_SIMPLE_FILE_SYSTEM_PROTOCOL. So even if it houses FAT > > file system, there is a chance that we may not be able to access > > it, particularly, when accesses are to be attempted after searching > > that protocol against a device handle. > > > > With this patch, EFI_SIMPLE_FILE_SYSTEM_PROTOCOL is installed > > to such a disk if part_get_info() shows there is not partition > > table installed on it. > > > > Signed-off-by: AKASHI Takahiro <takahiro.akashi@linaro.org> > > --- > > lib/efi_loader/efi_disk.c | 5 ++++- > > 1 file changed, 4 insertions(+), 1 deletion(-) > > > > diff --git a/lib/efi_loader/efi_disk.c b/lib/efi_loader/efi_disk.c > > index d72f455901f2..d36f22cedc52 100644 > > --- a/lib/efi_loader/efi_disk.c > > +++ b/lib/efi_loader/efi_disk.c > > @@ -253,6 +253,7 @@ static efi_status_t efi_disk_add_dev( > > struct efi_disk_obj **disk) > > { > > struct efi_disk_obj *diskobj; > > + disk_partition_t info; > > efi_status_t ret; > > > > /* Don't add empty devices */ > > @@ -284,7 +285,9 @@ static efi_status_t efi_disk_add_dev( > > diskobj->dp); > > if (ret != EFI_SUCCESS) > > return ret; > > - if (part >= 1 && efi_fs_exists(desc, part)) { > > + /* partitions or whole disk without partitions */ > > + if ((part >= 1 || part_get_info(desc, part, &info)) && > > part_get_info() returns -1 for part = 0 on a DOS partioned disk. So this > check does not work. Right. So this statement should be simplified like: if (efi_fs_exists(desc, part)) { diskobj->volume = efi_simple_file_system(desc, part, diskobj->dp); ret = efi_add_protocol(&diskobj->header, ... } -Takahiro Akashi > Best regards > > Heinrich > > > + efi_fs_exists(desc, part)) { > > diskobj->volume = efi_simple_file_system(desc, part, > > diskobj->dp); > > ret = efi_add_protocol(&diskobj->header, > > > ^ permalink raw reply [flat|nested] 13+ messages in thread
end of thread, other threads:[~2019-10-04 0:22 UTC | newest] Thread overview: 13+ messages (download: mbox.gz follow: Atom feed -- links below jump to the message on this page -- 2019-09-12 4:51 [U-Boot] [PATCH v2 0/3] efi_loader: disk: install FILE_SYSTEM_PROTOCOL to whole disk AKASHI Takahiro 2019-09-12 4:51 ` [U-Boot] [PATCH v2 1/3] fs: export fs_close() AKASHI Takahiro 2019-09-12 4:51 ` [U-Boot] [PATCH v2 2/3] efi_loader: disk: install FILE_SYSTEM_PROTOCOL only if available AKASHI Takahiro 2019-09-12 8:57 ` Heinrich Schuchardt 2019-09-12 9:17 ` AKASHI Takahiro 2019-09-12 9:43 ` Heinrich Schuchardt 2019-09-12 9:51 ` AKASHI Takahiro 2019-10-03 7:09 ` AKASHI Takahiro 2019-10-03 13:44 ` Heinrich Schuchardt 2019-10-04 0:22 ` AKASHI Takahiro 2019-09-12 4:51 ` [U-Boot] [PATCH v2 3/3] efi_loader: disk: install file system protocol to a whole disk AKASHI Takahiro 2019-09-12 9:21 ` Heinrich Schuchardt 2019-10-03 7:21 ` AKASHI Takahiro
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox