[U-Boot] [PATCH v3 0/4] efi_loader: disk: install FILE_SYSTEM_PROTOCOL to whole disk
Changes in v3 (Oct 4, 2019) * add patch#1 * use newly-added fs_get_type() instead fo fs_get_type_name() * check for FS_TYPE_ANY to confirm if a file system exists or not * remove a redundant check on whether a device is a partition or a whole disk
Changes in v2 (Sept 12, 2019) * add patch#1 and #2 * install the protocol only if a file system does exist
AKASHI Takahiro (4): fs: export fs_close() fs: add fs_get_type() for current filesystem type efi_loader: disk: install FILE_SYSTEM_PROTOCOL only if available efi_loader: disk: install file system protocol to a whole disk
fs/fs.c | 15 ++++++++++++++- include/fs.h | 17 +++++++++++++++++ lib/efi_loader/efi_disk.c | 17 ++++++++++++++++- 3 files changed, 47 insertions(+), 2 deletions(-)
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 *
On 10/4/19 5:05 AM, AKASHI Takahiro wrote:
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()
This "paired" implies that one should call one of the name functions afterwards which may not be necessary. I would suggest:
"fs_close() closes the connection to a file system which opened with either fs_set_blk_dev() or fs_set_dev_with_part(). Many file functions implicitly call fs_close(), e.g. fs_closedir(), fs_exist(), fs_ln(), fs_ls(), fs_mkdir(), fs_read(), fs_size(), fs_write(), fs_unlink()."
Best regards
Heinrich Schuchardt
- */
+void fs_close(void);
/**
- fs_get_type_name() - Get type of current filesystem
On Fri, Oct 04, 2019 at 08:53:00PM +0200, Heinrich Schuchardt wrote:
On 10/4/19 5:05 AM, AKASHI Takahiro wrote:
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()
This "paired" implies that one should call one of the name functions afterwards which may not be necessary. I would suggest:
"fs_close() closes the connection to a file system which opened with either fs_set_blk_dev() or fs_set_dev_with_part(). Many file functions implicitly call fs_close(), e.g. fs_closedir(), fs_exist(), fs_ln(), fs_ls(), fs_mkdir(), fs_read(), fs_size(), fs_write(), fs_unlink()."
Okay.
-Takahiro Akashi
Best regards
Heinrich Schuchardt
- */
+void fs_close(void);
/**
- fs_get_type_name() - Get type of current filesystem
This function is a variant of fs_get_type_name() and returns a filesystem type with which the current device is associated. We don't want to export fs_type variable directly because we have to take care of it consistently within fs.c.
Signed-off-by: AKASHI Takahiro takahiro.akashi@linaro.org --- fs/fs.c | 13 +++++++++++++ include/fs.h | 10 ++++++++++ 2 files changed, 23 insertions(+)
diff --git a/fs/fs.c b/fs/fs.c index 64ba25fea8bf..e5307dbeaa37 100644 --- a/fs/fs.c +++ b/fs/fs.c @@ -307,6 +307,19 @@ static struct fstype_info *fs_get_info(int fstype) return info; }
+/** + * fs_get_type() - Get type of current filesystem + * + * Return: filesystem type + * + * Returns filesystem type representing the current filesystem, or + * FS_TYPE_ANY for any unrecognised filesystem. + */ +int fs_get_type(void) +{ + return fs_type; +} + /** * fs_get_type_name() - Get type of current filesystem * diff --git a/include/fs.h b/include/fs.h index 5a1244d57fd2..6dfdb5c5307a 100644 --- a/include/fs.h +++ b/include/fs.h @@ -44,6 +44,16 @@ int fs_set_blk_dev_with_part(struct blk_desc *desc, int part); */ void fs_close(void);
+/** + * fs_get_type() - Get type of current filesystem + * + * Return: filesystem type + * + * Returns filesystem type representing the current filesystem, or + * FS_TYPE_ANY for any unrecognised filesystem. + */ +int fs_get_type(void); + /** * fs_get_type_name() - Get type of current filesystem *
On 10/4/19 5:05 AM, AKASHI Takahiro wrote:
This function is a variant of fs_get_type_name() and returns a filesystem type with which the current device is associated. We don't want to export fs_type variable directly because we have to take care of it consistently within fs.c.
Signed-off-by: AKASHI Takahiro takahiro.akashi@linaro.org
Looking at fs/fs.c there seems to be a lot of inconsistency in the usage of FS_TYPE_ANY. Some of the file system functions set fs_type = FS_TYPE_ANY before calling fs_close(). Others don't. Shouldn't we move those assignments to fs_close() to get consistency?
FS_TYPE_ANY seems to be misnomer and could be replaced by FS_TYPE_NONE.
Please, use scripts/get_maintainer in future to determine the addressees of patches. I have put the missing ones on CC now.
Reviewed-by: Heinrich Schuchardt xypron.glpk@gmx.de
fs/fs.c | 13 +++++++++++++ include/fs.h | 10 ++++++++++ 2 files changed, 23 insertions(+)
diff --git a/fs/fs.c b/fs/fs.c index 64ba25fea8bf..e5307dbeaa37 100644 --- a/fs/fs.c +++ b/fs/fs.c @@ -307,6 +307,19 @@ static struct fstype_info *fs_get_info(int fstype) return info; }
+/**
- fs_get_type() - Get type of current filesystem
- Return: filesystem type
- Returns filesystem type representing the current filesystem, or
- FS_TYPE_ANY for any unrecognised filesystem.
- */
+int fs_get_type(void) +{
- return fs_type;
+}
/**
- fs_get_type_name() - Get type of current filesystem
diff --git a/include/fs.h b/include/fs.h index 5a1244d57fd2..6dfdb5c5307a 100644 --- a/include/fs.h +++ b/include/fs.h @@ -44,6 +44,16 @@ int fs_set_blk_dev_with_part(struct blk_desc *desc, int part); */ void fs_close(void);
+/**
- fs_get_type() - Get type of current filesystem
- Return: filesystem type
- Returns filesystem type representing the current filesystem, or
- FS_TYPE_ANY for any unrecognised filesystem.
- */
+int fs_get_type(void);
/**
- fs_get_type_name() - Get type of current filesystem
On Fri, Oct 04, 2019 at 09:04:59PM +0200, Heinrich Schuchardt wrote:
On 10/4/19 5:05 AM, AKASHI Takahiro wrote:
This function is a variant of fs_get_type_name() and returns a filesystem type with which the current device is associated. We don't want to export fs_type variable directly because we have to take care of it consistently within fs.c.
Signed-off-by: AKASHI Takahiro takahiro.akashi@linaro.org
Looking at fs/fs.c there seems to be a lot of inconsistency in the usage of FS_TYPE_ANY. Some of the file system functions set fs_type = FS_TYPE_ANY before calling fs_close(). Others don't. Shouldn't we move those assignments to fs_close() to get consistency?
Another patch.
FS_TYPE_ANY seems to be misnomer and could be replaced by FS_TYPE_NONE.
Please, use scripts/get_maintainer in future to determine the addressees of patches. I have put the missing ones on CC now.
There is no dedicated maintainer for "fs" sub-system, so Tom was included here after MAINTAINERS.
-Takahiro Akashi
Reviewed-by: Heinrich Schuchardt xypron.glpk@gmx.de
fs/fs.c | 13 +++++++++++++ include/fs.h | 10 ++++++++++ 2 files changed, 23 insertions(+)
diff --git a/fs/fs.c b/fs/fs.c index 64ba25fea8bf..e5307dbeaa37 100644 --- a/fs/fs.c +++ b/fs/fs.c @@ -307,6 +307,19 @@ static struct fstype_info *fs_get_info(int fstype) return info; }
+/**
- fs_get_type() - Get type of current filesystem
- Return: filesystem type
- Returns filesystem type representing the current filesystem, or
- FS_TYPE_ANY for any unrecognised filesystem.
- */
+int fs_get_type(void) +{
- return fs_type;
+}
/**
- fs_get_type_name() - Get type of current filesystem
diff --git a/include/fs.h b/include/fs.h index 5a1244d57fd2..6dfdb5c5307a 100644 --- a/include/fs.h +++ b/include/fs.h @@ -44,6 +44,16 @@ int fs_set_blk_dev_with_part(struct blk_desc *desc, int part); */ void fs_close(void);
+/**
- fs_get_type() - Get type of current filesystem
- Return: filesystem type
- Returns filesystem type representing the current filesystem, or
- FS_TYPE_ANY for any unrecognised filesystem.
- */
+int fs_get_type(void);
/**
- fs_get_type_name() - Get type of current filesystem
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 any 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 9007a5f77f3d..27329cadb6f1 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>
@@ -262,6 +263,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 (fs_get_type() == FS_TYPE_ANY) + return 0; + + fs_close(); + + return 1; +} + /* * Create a handle for a partition or disk * @@ -315,7 +329,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,
On 10/4/19 5:05 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 any file system exists.
I generally prefer if each individual patch also has a version history.
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 9007a5f77f3d..27329cadb6f1 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>
@@ -262,6 +263,19 @@ efi_fs_from_path(struct efi_device_path *full_path) return handler->protocol_interface; }
Please, add:
/** * efi_fs_exists() - check if a partition bears a file system * * @desc: block device descriptor * @part: partition number * Return: 1 if a file system exists on the partition * 0 otherwise */
Otherwise
Reviewed-by: Heinrich Schuchardt xypron.glpk@gmx.de
+static int efi_fs_exists(struct blk_desc *desc, int part) +{
- if (fs_set_blk_dev_with_part(desc, part))
return 0;- if (fs_get_type() == FS_TYPE_ANY)
return 0;- fs_close();
- return 1;
+}
/*
- Create a handle for a partition or disk
@@ -315,7 +329,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,
On Fri, Oct 04, 2019 at 09:13:08PM +0200, Heinrich Schuchardt wrote:
On 10/4/19 5:05 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 any file system exists.
I generally prefer if each individual patch also has a version history.
It's not my style.
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 9007a5f77f3d..27329cadb6f1 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>
@@ -262,6 +263,19 @@ efi_fs_from_path(struct efi_device_path *full_path) return handler->protocol_interface; }
Please, add:
/**
- efi_fs_exists() - check if a partition bears a file system
- @desc: block device descriptor
- @part: partition number
- Return: 1 if a file system exists on the partition
0 otherwise*/
Otherwise
Reviewed-by: Heinrich Schuchardt xypron.glpk@gmx.de
Okay, thanks -Takahiro Akashi
+static int efi_fs_exists(struct blk_desc *desc, int part) +{
- if (fs_set_blk_dev_with_part(desc, part))
return 0;- if (fs_get_type() == FS_TYPE_ANY)
return 0;- fs_close();
- return 1;
+}
/*
- Create a handle for a partition or disk
@@ -315,7 +329,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,
Currently, a whole disk without any partitions is not associated with EFI_SIMPLE_FILE_SYSTEM_PROTOCOL. So even if it houses some 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 no partition table installed on it.
Signed-off-by: AKASHI Takahiro takahiro.akashi@linaro.org --- lib/efi_loader/efi_disk.c | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-)
diff --git a/lib/efi_loader/efi_disk.c b/lib/efi_loader/efi_disk.c index 27329cadb6f1..4e1a1efed7ec 100644 --- a/lib/efi_loader/efi_disk.c +++ b/lib/efi_loader/efi_disk.c @@ -329,7 +329,8 @@ 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 (efi_fs_exists(desc, part)) { diskobj->volume = efi_simple_file_system(desc, part, diskobj->dp); ret = efi_add_protocol(&diskobj->header,
On 10/4/19 5:05 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 some 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 no partition table installed on it.
Signed-off-by: AKASHI Takahiro takahiro.akashi@linaro.org
lib/efi_loader/efi_disk.c | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-)
diff --git a/lib/efi_loader/efi_disk.c b/lib/efi_loader/efi_disk.c index 27329cadb6f1..4e1a1efed7ec 100644 --- a/lib/efi_loader/efi_disk.c +++ b/lib/efi_loader/efi_disk.c @@ -329,7 +329,8 @@ 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 (efi_fs_exists(desc, part)) {
As described in response to an earlier version of the patch series we should only install the EFI protocols if no partition table exits.
Best regards
Heinrich Schuchardt
diskobj->volume = efi_simple_file_system(desc, part, diskobj->dp); ret = efi_add_protocol(&diskobj->header,
On Fri, Oct 04, 2019 at 09:15:19PM +0200, Heinrich Schuchardt wrote:
On 10/4/19 5:05 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 some 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 no partition table installed on it.
Signed-off-by: AKASHI Takahiro takahiro.akashi@linaro.org
lib/efi_loader/efi_disk.c | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-)
diff --git a/lib/efi_loader/efi_disk.c b/lib/efi_loader/efi_disk.c index 27329cadb6f1..4e1a1efed7ec 100644 --- a/lib/efi_loader/efi_disk.c +++ b/lib/efi_loader/efi_disk.c @@ -329,7 +329,8 @@ 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 (efi_fs_exists(desc, part)) {
As described in response to an earlier version of the patch series we should only install the EFI protocols if no partition table exits.
That is why I removed "part >= 1" check here. efi_fs_exists() should work both for a whole disk (part == 0) or any partitions on a disk.
Thanks, -Takahiro Akashi
Best regards
Heinrich Schuchardt
diskobj->volume = efi_simple_file_system(desc, part, diskobj->dp); ret = efi_add_protocol(&diskobj->header,
participants (2)
-
AKASHI Takahiro -
Heinrich Schuchardt