* [PATCH 01/18] common: partitions: decouple from EFI GUID definition
2023-05-31 14:59 [PATCH 00/18] state: allow lookup of barebox state partition by Type GUID Ahmad Fatoum
@ 2023-05-31 14:59 ` Ahmad Fatoum
2023-05-31 14:59 ` [PATCH 02/18] efi: define efi_guid_t as 32-bit aligned guid_t Ahmad Fatoum
` (16 subsequent siblings)
17 siblings, 0 replies; 62+ messages in thread
From: Ahmad Fatoum @ 2023-05-31 14:59 UTC (permalink / raw)
To: barebox; +Cc: Ahmad Fatoum
We have three UUID/GUID definitions in barebox:
- efi_guid_t: for EFI GUIDs
- uuid_t: for RFC 4122/DCE 1.1 (Variant 1) UUIDs
- guid_t: Apparently UUIDs stored in little-endian
In preparation for switching efi_guid_t to be a special case of guid_t
like in Linux, let's replace non-EFI uses of efi_guid_t with guid_t.
This allows us to drop the efi.h header outside of EFI code.
This also involves two replacements of efi_char16_t with wchar_t.
This is ok, as we always build with -fshort-wchar.
Signed-off-by: Ahmad Fatoum <a.fatoum@pengutronix.de>
---
common/partitions/efi.c | 4 ++--
common/partitions/parser.h | 1 +
include/efi/partition.h | 27 ++++++++++++++-------------
lib/Makefile | 1 +
lib/uuid.c | 15 +++++++++++++++
5 files changed, 33 insertions(+), 15 deletions(-)
create mode 100644 lib/uuid.c
diff --git a/common/partitions/efi.c b/common/partitions/efi.c
index ffdbd9a56f85..a6c95f3969d1 100644
--- a/common/partitions/efi.c
+++ b/common/partitions/efi.c
@@ -232,7 +232,7 @@ static int is_gpt_valid(struct block_device *blk, u64 lba,
static inline int
is_pte_valid(const gpt_entry *pte, const u64 lastlba)
{
- if ((!efi_guidcmp(pte->partition_type_guid, EFI_NULL_GUID)) ||
+ if (guid_is_null(&pte->partition_type_guid) ||
le64_to_cpu(pte->starting_lba) > lastlba ||
le64_to_cpu(pte->ending_lba) > lastlba)
return 0;
@@ -287,7 +287,7 @@ compare_gpts(struct device *dev, gpt_header *pgpt, gpt_header *agpt,
(unsigned long long)le64_to_cpu(agpt->last_usable_lba));
error_found++;
}
- if (efi_guidcmp(pgpt->disk_guid, agpt->disk_guid)) {
+ if (!guid_equal(&pgpt->disk_guid, &agpt->disk_guid)) {
dev_warn(dev, "GPT:disk_guids don't match.\n");
error_found++;
}
diff --git a/common/partitions/parser.h b/common/partitions/parser.h
index d67f8e1d6a09..f2f692f7903b 100644
--- a/common/partitions/parser.h
+++ b/common/partitions/parser.h
@@ -9,6 +9,7 @@
#include <block.h>
#include <filetype.h>
+#include <linux/uuid.h>
#include <linux/list.h>
#define MAX_PARTITION 128
diff --git a/include/efi/partition.h b/include/efi/partition.h
index a9b10c126654..0ca2a72eb9b7 100644
--- a/include/efi/partition.h
+++ b/include/efi/partition.h
@@ -21,7 +21,8 @@
#ifndef FS_PART_EFI_H_INCLUDED
#define FS_PART_EFI_H_INCLUDED
-#include <efi.h>
+#include <linux/types.h>
+#include <linux/uuid.h>
#define MSDOS_MBR_SIGNATURE 0xaa55
#define EFI_PMBR_OSTYPE_EFI 0xEF
@@ -33,25 +34,25 @@
#define GPT_PRIMARY_PARTITION_TABLE_LBA 1
#define PARTITION_SYSTEM_GUID \
- EFI_GUID( 0xC12A7328, 0xF81F, 0x11d2, \
+ GUID_INIT( 0xC12A7328, 0xF81F, 0x11d2, \
0xBA, 0x4B, 0x00, 0xA0, 0xC9, 0x3E, 0xC9, 0x3B)
#define LEGACY_MBR_PARTITION_GUID \
- EFI_GUID( 0x024DEE41, 0x33E7, 0x11d3, \
+ GUID_INIT( 0x024DEE41, 0x33E7, 0x11d3, \
0x9D, 0x69, 0x00, 0x08, 0xC7, 0x81, 0xF3, 0x9F)
#define PARTITION_MSFT_RESERVED_GUID \
- EFI_GUID( 0xE3C9E316, 0x0B5C, 0x4DB8, \
+ GUID_INIT( 0xE3C9E316, 0x0B5C, 0x4DB8, \
0x81, 0x7D, 0xF9, 0x2D, 0xF0, 0x02, 0x15, 0xAE)
#define PARTITION_BASIC_DATA_GUID \
- EFI_GUID( 0xEBD0A0A2, 0xB9E5, 0x4433, \
+ GUID_INIT( 0xEBD0A0A2, 0xB9E5, 0x4433, \
0x87, 0xC0, 0x68, 0xB6, 0xB7, 0x26, 0x99, 0xC7)
#define PARTITION_LINUX_RAID_GUID \
- EFI_GUID( 0xa19d880f, 0x05fc, 0x4d3b, \
+ GUID_INIT( 0xa19d880f, 0x05fc, 0x4d3b, \
0xa0, 0x06, 0x74, 0x3f, 0x0f, 0x84, 0x91, 0x1e)
#define PARTITION_LINUX_SWAP_GUID \
- EFI_GUID( 0x0657fd6d, 0xa4ab, 0x43c4, \
+ GUID_INIT( 0x0657fd6d, 0xa4ab, 0x43c4, \
0x84, 0xe5, 0x09, 0x33, 0xc8, 0x4b, 0x4f, 0x4f)
#define PARTITION_LINUX_LVM_GUID \
- EFI_GUID( 0xe6d6d379, 0xf507, 0x44c2, \
+ GUID_INIT( 0xe6d6d379, 0xf507, 0x44c2, \
0xa2, 0x3c, 0x23, 0x8f, 0x2a, 0x3d, 0xf9, 0x28)
/* based on linux/include/genhd.h */
@@ -79,7 +80,7 @@ typedef struct _gpt_header {
__le64 alternate_lba;
__le64 first_usable_lba;
__le64 last_usable_lba;
- efi_guid_t disk_guid;
+ guid_t disk_guid;
__le64 partition_entry_lba;
__le32 num_partition_entries;
__le32 sizeof_partition_entry;
@@ -98,14 +99,14 @@ typedef struct _gpt_entry_attributes {
u64 type_guid_specific:16;
} __attribute__ ((packed)) gpt_entry_attributes;
-#define GPT_PARTNAME_MAX_SIZE (72 / sizeof (efi_char16_t))
+#define GPT_PARTNAME_MAX_SIZE (72 / sizeof (wchar_t))
typedef struct _gpt_entry {
- efi_guid_t partition_type_guid;
- efi_guid_t unique_partition_guid;
+ guid_t partition_type_guid;
+ guid_t unique_partition_guid;
__le64 starting_lba;
__le64 ending_lba;
gpt_entry_attributes attributes;
- efi_char16_t partition_name[GPT_PARTNAME_MAX_SIZE];
+ wchar_t partition_name[GPT_PARTNAME_MAX_SIZE];
} __attribute__ ((packed)) gpt_entry;
typedef struct _legacy_mbr {
diff --git a/lib/Makefile b/lib/Makefile
index 38478625423b..764bcbf911aa 100644
--- a/lib/Makefile
+++ b/lib/Makefile
@@ -27,6 +27,7 @@ obj-y += recursive_action.o
obj-y += make_directory.o
obj-y += math.o
obj-y += math/
+obj-y += uuid.o
obj-$(CONFIG_XXHASH) += xxhash.o
obj-$(CONFIG_BZLIB) += decompress_bunzip2.o
obj-$(CONFIG_ZLIB) += decompress_inflate.o zlib_inflate/
diff --git a/lib/uuid.c b/lib/uuid.c
new file mode 100644
index 000000000000..db6464e354b1
--- /dev/null
+++ b/lib/uuid.c
@@ -0,0 +1,15 @@
+// SPDX-License-Identifier: GPL-2.0-only
+/*
+ * Unified UUID/GUID definition
+ *
+ * Copyright (C) 2009, 2016 Intel Corp.
+ * Huang Ying <ying.huang@intel.com>
+ */
+
+#include <linux/uuid.h>
+#include <linux/export.h>
+
+const guid_t guid_null;
+EXPORT_SYMBOL(guid_null);
+const uuid_t uuid_null;
+EXPORT_SYMBOL(uuid_null);
--
2.39.2
^ permalink raw reply [flat|nested] 62+ messages in thread* [PATCH 02/18] efi: define efi_guid_t as 32-bit aligned guid_t
2023-05-31 14:59 [PATCH 00/18] state: allow lookup of barebox state partition by Type GUID Ahmad Fatoum
2023-05-31 14:59 ` [PATCH 01/18] common: partitions: decouple from EFI GUID definition Ahmad Fatoum
@ 2023-05-31 14:59 ` Ahmad Fatoum
2023-05-31 14:59 ` [PATCH 03/18] cdev: fix for_each_cdev macro Ahmad Fatoum
` (15 subsequent siblings)
17 siblings, 0 replies; 62+ messages in thread
From: Ahmad Fatoum @ 2023-05-31 14:59 UTC (permalink / raw)
To: barebox; +Cc: Ahmad Fatoum
Let's sync definition with Linux, so we are able to pass efi_guid_t
types to function accepting guid_t. This has the added benefit of us
starting to observe alignment, which may become relevant with barebox
EFI on non-x86.
Signed-off-by: Ahmad Fatoum <a.fatoum@pengutronix.de>
---
include/efi.h | 19 +++++++++++++++----
1 file changed, 15 insertions(+), 4 deletions(-)
diff --git a/include/efi.h b/include/efi.h
index 3595cf05ccb7..1904caf3a4b6 100644
--- a/include/efi.h
+++ b/include/efi.h
@@ -14,6 +14,7 @@
*/
#include <linux/string.h>
#include <linux/types.h>
+#include <linux/uuid.h>
#ifdef CONFIG_EFI_BOOTUP
#define EFIAPI __attribute__((ms_abi))
@@ -66,10 +67,20 @@ typedef u16 efi_char16_t; /* UNICODE character */
typedef u64 efi_physical_addr_t;
typedef void *efi_handle_t;
-
-typedef struct {
- u8 b[16];
-} efi_guid_t;
+/*
+ * The UEFI spec and EDK2 reference implementation both define EFI_GUID as
+ * struct { u32 a; u16; b; u16 c; u8 d[8]; }; and so the implied alignment
+ * is 32 bits not 8 bits like our guid_t. In some cases (i.e., on 32-bit ARM),
+ * this means that firmware services invoked by the kernel may assume that
+ * efi_guid_t* arguments are 32-bit aligned, and use memory accessors that
+ * do not tolerate misalignment. So let's set the minimum alignment to 32 bits.
+ *
+ * Note that the UEFI spec as well as some comments in the EDK2 code base
+ * suggest that EFI_GUID should be 64-bit aligned, but this appears to be
+ * a mistake, given that no code seems to exist that actually enforces that
+ * or relies on it.
+ */
+typedef guid_t efi_guid_t __aligned(__alignof__(u32));
#define EFI_GUID(a, b, c, d0, d1, d2, d3, d4, d5, d6, d7) \
((efi_guid_t) \
--
2.39.2
^ permalink raw reply [flat|nested] 62+ messages in thread* [PATCH 03/18] cdev: fix for_each_cdev macro
2023-05-31 14:59 [PATCH 00/18] state: allow lookup of barebox state partition by Type GUID Ahmad Fatoum
2023-05-31 14:59 ` [PATCH 01/18] common: partitions: decouple from EFI GUID definition Ahmad Fatoum
2023-05-31 14:59 ` [PATCH 02/18] efi: define efi_guid_t as 32-bit aligned guid_t Ahmad Fatoum
@ 2023-05-31 14:59 ` Ahmad Fatoum
2023-05-31 15:37 ` Marco Felsch
2023-05-31 14:59 ` [PATCH 04/18] of: partition: support of_partition_ensure_probed on parent device Ahmad Fatoum
` (14 subsequent siblings)
17 siblings, 1 reply; 62+ messages in thread
From: Ahmad Fatoum @ 2023-05-31 14:59 UTC (permalink / raw)
To: barebox; +Cc: Ahmad Fatoum
The macro parameter 'c' was never used, instead hardcoding cdev.
It worked so far anyway, because all users of for_each_cdev used cdev
as the argument. Fix this.
Signed-off-by: Ahmad Fatoum <a.fatoum@pengutronix.de>
---
include/driver.h | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/include/driver.h b/include/driver.h
index d33e0fcbccc9..e1ee3dc2dd7c 100644
--- a/include/driver.h
+++ b/include/driver.h
@@ -566,7 +566,7 @@ int cdev_truncate(struct cdev*, size_t size);
loff_t cdev_unallocated_space(struct cdev *cdev);
extern struct list_head cdev_list;
-#define for_each_cdev(c) \
+#define for_each_cdev(cdev) \
list_for_each_entry(cdev, &cdev_list, list)
#define DEVFS_PARTITION_FIXED (1U << 0)
--
2.39.2
^ permalink raw reply [flat|nested] 62+ messages in thread* Re: [PATCH 03/18] cdev: fix for_each_cdev macro
2023-05-31 14:59 ` [PATCH 03/18] cdev: fix for_each_cdev macro Ahmad Fatoum
@ 2023-05-31 15:37 ` Marco Felsch
0 siblings, 0 replies; 62+ messages in thread
From: Marco Felsch @ 2023-05-31 15:37 UTC (permalink / raw)
To: Ahmad Fatoum; +Cc: barebox
On 23-05-31, Ahmad Fatoum wrote:
> The macro parameter 'c' was never used, instead hardcoding cdev.
> It worked so far anyway, because all users of for_each_cdev used cdev
> as the argument. Fix this.
>
> Signed-off-by: Ahmad Fatoum <a.fatoum@pengutronix.de>
Reviewed-by: Marco Felsch <m.felsch@pengutronix.de>
> ---
> include/driver.h | 2 +-
> 1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/include/driver.h b/include/driver.h
> index d33e0fcbccc9..e1ee3dc2dd7c 100644
> --- a/include/driver.h
> +++ b/include/driver.h
> @@ -566,7 +566,7 @@ int cdev_truncate(struct cdev*, size_t size);
> loff_t cdev_unallocated_space(struct cdev *cdev);
>
> extern struct list_head cdev_list;
> -#define for_each_cdev(c) \
> +#define for_each_cdev(cdev) \
> list_for_each_entry(cdev, &cdev_list, list)
>
> #define DEVFS_PARTITION_FIXED (1U << 0)
> --
> 2.39.2
>
>
>
^ permalink raw reply [flat|nested] 62+ messages in thread
* [PATCH 04/18] of: partition: support of_partition_ensure_probed on parent device
2023-05-31 14:59 [PATCH 00/18] state: allow lookup of barebox state partition by Type GUID Ahmad Fatoum
` (2 preceding siblings ...)
2023-05-31 14:59 ` [PATCH 03/18] cdev: fix for_each_cdev macro Ahmad Fatoum
@ 2023-05-31 14:59 ` Ahmad Fatoum
2023-05-31 16:30 ` Marco Felsch
2023-05-31 14:59 ` [PATCH 05/18] of: of_path: always call of_partition_ensure_probed before resolving Ahmad Fatoum
` (13 subsequent siblings)
17 siblings, 1 reply; 62+ messages in thread
From: Ahmad Fatoum @ 2023-05-31 14:59 UTC (permalink / raw)
To: barebox; +Cc: Ahmad Fatoum
barebox-state code uses of_partition_ensure_probed to resolve the
backend property. We want to allow backend to point directly at a
storage device instead of a partition. We can't determine whether a DT
device is a storage device though before it's probed, so let's have
of_partition_ensure_probed support either case.
Signed-off-by: Ahmad Fatoum <a.fatoum@pengutronix.de>
---
drivers/of/partition.c | 26 ++++++++++++++++++++++----
drivers/of/platform.c | 2 +-
2 files changed, 23 insertions(+), 5 deletions(-)
diff --git a/drivers/of/partition.c b/drivers/of/partition.c
index 40c47f554ad2..a70e503cec9e 100644
--- a/drivers/of/partition.c
+++ b/drivers/of/partition.c
@@ -110,14 +110,32 @@ int of_parse_partitions(struct cdev *cdev, struct device_node *node)
return 0;
}
+/**
+ * of_partition_ensure_probed - ensure a parition is probed
+ * @np: pointer to a partition or to a partitionable device
+ * Unfortunately, there is no completely reliable way
+ * to differentiate partitions from devices prior to
+ * probing, because partitions may also have compatibles.
+ * We only handle nvmem-cells, so anything besides that
+ * is assumed to be a device that should be probed directly.
+ *
+ * Returns zero on success or a negative error code otherwise
+ */
int of_partition_ensure_probed(struct device_node *np)
{
- np = of_get_parent(np);
+ struct device_node *parent = of_get_parent(np);
- if (of_device_is_compatible(np, "fixed-partitions"))
- np = of_get_parent(np);
+ if (parent && of_device_is_compatible(parent, "fixed-partitions"))
+ return of_device_ensure_probed(of_get_parent(np));
- return np ? of_device_ensure_probed(np) : -EINVAL;
+ if (of_get_compatible_child(np, "fixed-partitions"))
+ return of_device_ensure_probed(np);
+
+ if (!of_property_present(np, "compatible") ||
+ of_device_is_compatible(np, "nvmem-cells"))
+ return of_device_ensure_probed(parent);
+
+ return of_device_ensure_probed(np);
}
EXPORT_SYMBOL_GPL(of_partition_ensure_probed);
diff --git a/drivers/of/platform.c b/drivers/of/platform.c
index ab737629325a..78b8a31331db 100644
--- a/drivers/of/platform.c
+++ b/drivers/of/platform.c
@@ -484,7 +484,7 @@ int of_device_ensure_probed(struct device_node *np)
{
struct device *dev;
- if (!deep_probe_is_supported())
+ if (!np || !deep_probe_is_supported())
return 0;
dev = of_device_create_on_demand(np);
--
2.39.2
^ permalink raw reply [flat|nested] 62+ messages in thread* Re: [PATCH 04/18] of: partition: support of_partition_ensure_probed on parent device
2023-05-31 14:59 ` [PATCH 04/18] of: partition: support of_partition_ensure_probed on parent device Ahmad Fatoum
@ 2023-05-31 16:30 ` Marco Felsch
2023-06-01 4:48 ` Ahmad Fatoum
0 siblings, 1 reply; 62+ messages in thread
From: Marco Felsch @ 2023-05-31 16:30 UTC (permalink / raw)
To: Ahmad Fatoum; +Cc: barebox
Hi Ahmad,
On 23-05-31, Ahmad Fatoum wrote:
> barebox-state code uses of_partition_ensure_probed to resolve the
> backend property. We want to allow backend to point directly at a
> storage device instead of a partition. We can't determine whether a DT
> device is a storage device though before it's probed, so let's have
> of_partition_ensure_probed support either case.
>
> Signed-off-by: Ahmad Fatoum <a.fatoum@pengutronix.de>
> ---
> drivers/of/partition.c | 26 ++++++++++++++++++++++----
> drivers/of/platform.c | 2 +-
> 2 files changed, 23 insertions(+), 5 deletions(-)
>
> diff --git a/drivers/of/partition.c b/drivers/of/partition.c
> index 40c47f554ad2..a70e503cec9e 100644
> --- a/drivers/of/partition.c
> +++ b/drivers/of/partition.c
> @@ -110,14 +110,32 @@ int of_parse_partitions(struct cdev *cdev, struct device_node *node)
> return 0;
> }
>
> +/**
> + * of_partition_ensure_probed - ensure a parition is probed
> + * @np: pointer to a partition or to a partitionable device
> + * Unfortunately, there is no completely reliable way
> + * to differentiate partitions from devices prior to
> + * probing, because partitions may also have compatibles.
> + * We only handle nvmem-cells, so anything besides that
> + * is assumed to be a device that should be probed directly.
> + *
> + * Returns zero on success or a negative error code otherwise
> + */
> int of_partition_ensure_probed(struct device_node *np)
> {
> - np = of_get_parent(np);
> + struct device_node *parent = of_get_parent(np);
>
> - if (of_device_is_compatible(np, "fixed-partitions"))
> - np = of_get_parent(np);
> + if (parent && of_device_is_compatible(parent, "fixed-partitions"))
When is the parent not present? This should only be the case when 'np'
points to the root_node. So in case of !parent I would return -EINVAL
early.
> + return of_device_ensure_probed(of_get_parent(np));
^
parent?
Not related to this patch but the logic would become easier if would
have devices for each mtd-part, like the kernel does. In such case we
could avoid these special handlings and just use
of_device_ensure_probed() at least for the mtd-parts, nvmem-cells still
need a special handling.
Regards,
Marco
> - return np ? of_device_ensure_probed(np) : -EINVAL;
> + if (of_get_compatible_child(np, "fixed-partitions"))
> + return of_device_ensure_probed(np);
> +
> + if (!of_property_present(np, "compatible") ||
> + of_device_is_compatible(np, "nvmem-cells"))
> + return of_device_ensure_probed(parent);
> +
> + return of_device_ensure_probed(np);
> }
> EXPORT_SYMBOL_GPL(of_partition_ensure_probed);
>
> diff --git a/drivers/of/platform.c b/drivers/of/platform.c
> index ab737629325a..78b8a31331db 100644
> --- a/drivers/of/platform.c
> +++ b/drivers/of/platform.c
> @@ -484,7 +484,7 @@ int of_device_ensure_probed(struct device_node *np)
> {
> struct device *dev;
>
> - if (!deep_probe_is_supported())
> + if (!np || !deep_probe_is_supported())
> return 0;
>
> dev = of_device_create_on_demand(np);
> --
> 2.39.2
>
>
>
^ permalink raw reply [flat|nested] 62+ messages in thread* Re: [PATCH 04/18] of: partition: support of_partition_ensure_probed on parent device
2023-05-31 16:30 ` Marco Felsch
@ 2023-06-01 4:48 ` Ahmad Fatoum
0 siblings, 0 replies; 62+ messages in thread
From: Ahmad Fatoum @ 2023-06-01 4:48 UTC (permalink / raw)
To: Marco Felsch; +Cc: barebox
Hello Marco,
On 31.05.23 18:30, Marco Felsch wrote:
> Hi Ahmad,
>
> On 23-05-31, Ahmad Fatoum wrote:
>> barebox-state code uses of_partition_ensure_probed to resolve the
>> backend property. We want to allow backend to point directly at a
>> storage device instead of a partition. We can't determine whether a DT
>> device is a storage device though before it's probed, so let's have
>> of_partition_ensure_probed support either case.
>>
>> Signed-off-by: Ahmad Fatoum <a.fatoum@pengutronix.de>
>> ---
>> drivers/of/partition.c | 26 ++++++++++++++++++++++----
>> drivers/of/platform.c | 2 +-
>> 2 files changed, 23 insertions(+), 5 deletions(-)
>>
>> diff --git a/drivers/of/partition.c b/drivers/of/partition.c
>> index 40c47f554ad2..a70e503cec9e 100644
>> --- a/drivers/of/partition.c
>> +++ b/drivers/of/partition.c
>> @@ -110,14 +110,32 @@ int of_parse_partitions(struct cdev *cdev, struct device_node *node)
>> return 0;
>> }
>>
>> +/**
>> + * of_partition_ensure_probed - ensure a parition is probed
>> + * @np: pointer to a partition or to a partitionable device
>> + * Unfortunately, there is no completely reliable way
>> + * to differentiate partitions from devices prior to
>> + * probing, because partitions may also have compatibles.
>> + * We only handle nvmem-cells, so anything besides that
>> + * is assumed to be a device that should be probed directly.
>> + *
>> + * Returns zero on success or a negative error code otherwise
>> + */
>> int of_partition_ensure_probed(struct device_node *np)
>> {
>> - np = of_get_parent(np);
>> + struct device_node *parent = of_get_parent(np);
>>
>> - if (of_device_is_compatible(np, "fixed-partitions"))
>> - np = of_get_parent(np);
>> + if (parent && of_device_is_compatible(parent, "fixed-partitions"))
>
> When is the parent not present? This should only be the case when 'np'
> points to the root_node. So in case of !parent I would return -EINVAL
> early.
will do.
>
>> + return of_device_ensure_probed(of_get_parent(np));
> ^
> parent?
Yes, You're right.
> Not related to this patch but the logic would become easier if would
> have devices for each mtd-part, like the kernel does. In such case we
> could avoid these special handlings and just use
> of_device_ensure_probed() at least for the mtd-parts, nvmem-cells still
> need a special handling.
How you mean? of_partition_ensure_probed can be called before there can
be any devices at all.
>
> Regards,
> Marco
>
>> - return np ? of_device_ensure_probed(np) : -EINVAL;
>> + if (of_get_compatible_child(np, "fixed-partitions"))
>> + return of_device_ensure_probed(np);
I think this can be dropped, so the case falls through to the final
of_device_ensure_probed.
>> +
>> + if (!of_property_present(np, "compatible") ||
>> + of_device_is_compatible(np, "nvmem-cells"))
>> + return of_device_ensure_probed(parent);
>> +
>> + return of_device_ensure_probed(np);
>> }
>> EXPORT_SYMBOL_GPL(of_partition_ensure_probed);
Thanks,
Ahmad
>>
>> diff --git a/drivers/of/platform.c b/drivers/of/platform.c
>> index ab737629325a..78b8a31331db 100644
>> --- a/drivers/of/platform.c
>> +++ b/drivers/of/platform.c
>> @@ -484,7 +484,7 @@ int of_device_ensure_probed(struct device_node *np)
>> {
>> struct device *dev;
>>
>> - if (!deep_probe_is_supported())
>> + if (!np || !deep_probe_is_supported())
>> return 0;
>>
>> dev = of_device_create_on_demand(np);
>> --
>> 2.39.2
>>
>>
>>
>
--
Pengutronix e.K. | |
Steuerwalder Str. 21 | http://www.pengutronix.de/ |
31137 Hildesheim, Germany | Phone: +49-5121-206917-0 |
Amtsgericht Hildesheim, HRA 2686 | Fax: +49-5121-206917-5555 |
^ permalink raw reply [flat|nested] 62+ messages in thread
* [PATCH 05/18] of: of_path: always call of_partition_ensure_probed before resolving
2023-05-31 14:59 [PATCH 00/18] state: allow lookup of barebox state partition by Type GUID Ahmad Fatoum
` (3 preceding siblings ...)
2023-05-31 14:59 ` [PATCH 04/18] of: partition: support of_partition_ensure_probed on parent device Ahmad Fatoum
@ 2023-05-31 14:59 ` Ahmad Fatoum
2023-05-31 16:34 ` Marco Felsch
2023-06-01 7:00 ` Ulrich Ölmann
2023-05-31 14:59 ` [PATCH 06/18] driver: add new cdev_is_partition helper Ahmad Fatoum
` (12 subsequent siblings)
17 siblings, 2 replies; 62+ messages in thread
From: Ahmad Fatoum @ 2023-05-31 14:59 UTC (permalink / raw)
To: barebox; +Cc: Ahmad Fatoum
of_find_path may be called on a partition, whose parent device is not
yet probed. state code solves that by calling of_partition_ensure_probed
before of_find_path_by_nde, but really we should be doing that for all
calls to of_find_path. Do so.
Signed-off-by: Ahmad Fatoum <a.fatoum@pengutronix.de>
---
common/state/state.c | 4 ----
drivers/of/of_path.c | 2 ++
2 files changed, 2 insertions(+), 4 deletions(-)
diff --git a/common/state/state.c b/common/state/state.c
index 6b4acbb32bcc..11cc86ff73be 100644
--- a/common/state/state.c
+++ b/common/state/state.c
@@ -618,10 +618,6 @@ struct state *state_new_from_node(struct device_node *node, bool readonly)
}
#ifdef __BAREBOX__
- ret = of_partition_ensure_probed(partition_node);
- if (ret)
- goto out_release_state;
-
ret = of_find_path_by_node(partition_node, &state->backend_path, 0);
#else
ret = of_get_devicepath(partition_node, &state->backend_path, &offset, &size);
diff --git a/drivers/of/of_path.c b/drivers/of/of_path.c
index 1268cf36ee5b..059690e9b8e8 100644
--- a/drivers/of/of_path.c
+++ b/drivers/of/of_path.c
@@ -43,6 +43,8 @@ static int __of_find_path(struct device_node *node, const char *part, char **out
struct cdev *cdev;
bool add_bb = false;
+ of_partition_ensure_probed(node);
+
dev = of_find_device_by_node_path(node->full_name);
if (!dev) {
int ret;
--
2.39.2
^ permalink raw reply [flat|nested] 62+ messages in thread* Re: [PATCH 05/18] of: of_path: always call of_partition_ensure_probed before resolving
2023-05-31 14:59 ` [PATCH 05/18] of: of_path: always call of_partition_ensure_probed before resolving Ahmad Fatoum
@ 2023-05-31 16:34 ` Marco Felsch
2023-06-01 7:00 ` Ulrich Ölmann
1 sibling, 0 replies; 62+ messages in thread
From: Marco Felsch @ 2023-05-31 16:34 UTC (permalink / raw)
To: Ahmad Fatoum; +Cc: barebox
Hi Ahmad,
On 23-05-31, Ahmad Fatoum wrote:
> of_find_path may be called on a partition, whose parent device is not
> yet probed. state code solves that by calling of_partition_ensure_probed
> before of_find_path_by_nde, but really we should be doing that for all
nit: of_find_path_by_node
> calls to of_find_path. Do so.
>
> Signed-off-by: Ahmad Fatoum <a.fatoum@pengutronix.de>
Reviewed-by: Marco Felsch <m.felsch@pengutronix.de>
> ---
> common/state/state.c | 4 ----
> drivers/of/of_path.c | 2 ++
> 2 files changed, 2 insertions(+), 4 deletions(-)
>
> diff --git a/common/state/state.c b/common/state/state.c
> index 6b4acbb32bcc..11cc86ff73be 100644
> --- a/common/state/state.c
> +++ b/common/state/state.c
> @@ -618,10 +618,6 @@ struct state *state_new_from_node(struct device_node *node, bool readonly)
> }
>
> #ifdef __BAREBOX__
> - ret = of_partition_ensure_probed(partition_node);
> - if (ret)
> - goto out_release_state;
> -
> ret = of_find_path_by_node(partition_node, &state->backend_path, 0);
> #else
> ret = of_get_devicepath(partition_node, &state->backend_path, &offset, &size);
> diff --git a/drivers/of/of_path.c b/drivers/of/of_path.c
> index 1268cf36ee5b..059690e9b8e8 100644
> --- a/drivers/of/of_path.c
> +++ b/drivers/of/of_path.c
> @@ -43,6 +43,8 @@ static int __of_find_path(struct device_node *node, const char *part, char **out
> struct cdev *cdev;
> bool add_bb = false;
>
> + of_partition_ensure_probed(node);
> +
> dev = of_find_device_by_node_path(node->full_name);
> if (!dev) {
> int ret;
> --
> 2.39.2
>
>
>
^ permalink raw reply [flat|nested] 62+ messages in thread* Re: [PATCH 05/18] of: of_path: always call of_partition_ensure_probed before resolving
2023-05-31 14:59 ` [PATCH 05/18] of: of_path: always call of_partition_ensure_probed before resolving Ahmad Fatoum
2023-05-31 16:34 ` Marco Felsch
@ 2023-06-01 7:00 ` Ulrich Ölmann
1 sibling, 0 replies; 62+ messages in thread
From: Ulrich Ölmann @ 2023-06-01 7:00 UTC (permalink / raw)
To: Ahmad Fatoum; +Cc: barebox
Hi Ahmad,
as it looks that you are going to create a v2 anyway, just a small typo
fix.
On Wed, May 31 2023 at 16:59 +0200, Ahmad Fatoum <a.fatoum@pengutronix.de> wrote:
> of_find_path may be called on a partition, whose parent device is not
> yet probed. state code solves that by calling of_partition_ensure_probed
> before of_find_path_by_nde, but really we should be doing that for all
s/of_find_path_by_nde/of_find_path_by_node/
Best regards
Ulrich
> calls to of_find_path. Do so.
>
> Signed-off-by: Ahmad Fatoum <a.fatoum@pengutronix.de>
> ---
> common/state/state.c | 4 ----
> drivers/of/of_path.c | 2 ++
> 2 files changed, 2 insertions(+), 4 deletions(-)
>
> diff --git a/common/state/state.c b/common/state/state.c
> index 6b4acbb32bcc..11cc86ff73be 100644
> --- a/common/state/state.c
> +++ b/common/state/state.c
> @@ -618,10 +618,6 @@ struct state *state_new_from_node(struct device_node *node, bool readonly)
> }
>
> #ifdef __BAREBOX__
> - ret = of_partition_ensure_probed(partition_node);
> - if (ret)
> - goto out_release_state;
> -
> ret = of_find_path_by_node(partition_node, &state->backend_path, 0);
> #else
> ret = of_get_devicepath(partition_node, &state->backend_path, &offset, &size);
> diff --git a/drivers/of/of_path.c b/drivers/of/of_path.c
> index 1268cf36ee5b..059690e9b8e8 100644
> --- a/drivers/of/of_path.c
> +++ b/drivers/of/of_path.c
> @@ -43,6 +43,8 @@ static int __of_find_path(struct device_node *node, const char *part, char **out
> struct cdev *cdev;
> bool add_bb = false;
>
> + of_partition_ensure_probed(node);
> +
> dev = of_find_device_by_node_path(node->full_name);
> if (!dev) {
> int ret;
--
Pengutronix e.K. | Ulrich Ölmann |
Industrial Linux Solutions | http://www.pengutronix.de/ |
Peiner Str. 6-8, 31137 Hildesheim, Germany | Phone: +49-5121-206917-0 |
Amtsgericht Hildesheim, HRA 2686 | Fax: +49-5121-206917-5555 |
^ permalink raw reply [flat|nested] 62+ messages in thread
* [PATCH 06/18] driver: add new cdev_is_partition helper
2023-05-31 14:59 [PATCH 00/18] state: allow lookup of barebox state partition by Type GUID Ahmad Fatoum
` (4 preceding siblings ...)
2023-05-31 14:59 ` [PATCH 05/18] of: of_path: always call of_partition_ensure_probed before resolving Ahmad Fatoum
@ 2023-05-31 14:59 ` Ahmad Fatoum
2023-05-31 16:36 ` Marco Felsch
2023-05-31 14:59 ` [PATCH 07/18] commands: stat: remove code duplication for type info Ahmad Fatoum
` (11 subsequent siblings)
17 siblings, 1 reply; 62+ messages in thread
From: Ahmad Fatoum @ 2023-05-31 14:59 UTC (permalink / raw)
To: barebox; +Cc: Ahmad Fatoum
Partitions will have cdev->master != NULL, so often code will just do
if (cdev->master) to check if a cdev is a partition. This is suboptimal
as it may be misinterpreted by readers as meaning that the cdev is the
master device, while it's the other way round.
Let's define cdev_is_partition instead and use it everywhere, where
cdev->master is only checked, but not dereferenced.
Signed-off-by: Ahmad Fatoum <a.fatoum@pengutronix.de>
---
drivers/base/driver.c | 2 +-
fs/devfs-core.c | 10 +++++-----
include/driver.h | 4 ++++
3 files changed, 10 insertions(+), 6 deletions(-)
diff --git a/drivers/base/driver.c b/drivers/base/driver.c
index f00be99cdcbf..10d765e1a213 100644
--- a/drivers/base/driver.c
+++ b/drivers/base/driver.c
@@ -277,7 +277,7 @@ int unregister_device(struct device *old_dev)
}
list_for_each_entry_safe(cdev, ct, &old_dev->cdevs, devices_list) {
- if (cdev->master) {
+ if (cdev_is_partition(cdev)) {
dev_dbg(old_dev, "unregister part %s\n", cdev->name);
devfs_del_partition(cdev->name);
}
diff --git a/fs/devfs-core.c b/fs/devfs-core.c
index fbcf68e81597..bb66993b90f9 100644
--- a/fs/devfs-core.c
+++ b/fs/devfs-core.c
@@ -38,7 +38,7 @@ int devfs_partition_complete(struct string_list *sl, char *instr)
len = strlen(instr);
for_each_cdev(cdev) {
- if (cdev->master &&
+ if (cdev_is_partition(cdev) &&
!strncmp(instr, cdev->name, len)) {
string_list_add_asprintf(sl, "%s ", cdev->name);
}
@@ -101,7 +101,7 @@ struct cdev *cdev_by_partuuid(const char *partuuid)
return NULL;
for_each_cdev(cdev) {
- if (cdev->master && !strcasecmp(cdev->uuid, partuuid))
+ if (cdev_is_partition(cdev) && !strcasecmp(cdev->uuid, partuuid))
return cdev;
}
return NULL;
@@ -115,7 +115,7 @@ struct cdev *cdev_by_diskuuid(const char *diskuuid)
return NULL;
for_each_cdev(cdev) {
- if (!cdev->master && !strcasecmp(cdev->uuid, diskuuid))
+ if (!cdev_is_partition(cdev) && !strcasecmp(cdev->uuid, diskuuid))
return cdev;
}
return NULL;
@@ -393,7 +393,7 @@ int devfs_remove(struct cdev *cdev)
list_for_each_entry_safe(c, tmp, &cdev->links, link_entry)
devfs_remove(c);
- if (cdev->master)
+ if (cdev_is_partition(cdev))
list_del(&cdev->partition_entry);
if (cdev->link)
@@ -549,7 +549,7 @@ int devfs_del_partition(const char *name)
return ret;
}
- if (!cdev->master)
+ if (!cdev_is_partition(cdev))
return -EINVAL;
if (cdev->flags & DEVFS_PARTITION_FIXED)
return -EPERM;
diff --git a/include/driver.h b/include/driver.h
index e1ee3dc2dd7c..00b4a0e4af75 100644
--- a/include/driver.h
+++ b/include/driver.h
@@ -564,6 +564,10 @@ int cdev_discard_range(struct cdev*, loff_t count, loff_t offset);
int cdev_memmap(struct cdev*, void **map, int flags);
int cdev_truncate(struct cdev*, size_t size);
loff_t cdev_unallocated_space(struct cdev *cdev);
+static inline bool cdev_is_partition(const struct cdev *cdev)
+{
+ return cdev->master != NULL;
+}
extern struct list_head cdev_list;
#define for_each_cdev(cdev) \
--
2.39.2
^ permalink raw reply [flat|nested] 62+ messages in thread* Re: [PATCH 06/18] driver: add new cdev_is_partition helper
2023-05-31 14:59 ` [PATCH 06/18] driver: add new cdev_is_partition helper Ahmad Fatoum
@ 2023-05-31 16:36 ` Marco Felsch
0 siblings, 0 replies; 62+ messages in thread
From: Marco Felsch @ 2023-05-31 16:36 UTC (permalink / raw)
To: Ahmad Fatoum; +Cc: barebox
On 23-05-31, Ahmad Fatoum wrote:
> Partitions will have cdev->master != NULL, so often code will just do
> if (cdev->master) to check if a cdev is a partition. This is suboptimal
> as it may be misinterpreted by readers as meaning that the cdev is the
> master device, while it's the other way round.
>
> Let's define cdev_is_partition instead and use it everywhere, where
> cdev->master is only checked, but not dereferenced.
>
> Signed-off-by: Ahmad Fatoum <a.fatoum@pengutronix.de>
Reviewed-by: Marco Felsch <m.felsch@pengutronix.de>
> ---
> drivers/base/driver.c | 2 +-
> fs/devfs-core.c | 10 +++++-----
> include/driver.h | 4 ++++
> 3 files changed, 10 insertions(+), 6 deletions(-)
>
> diff --git a/drivers/base/driver.c b/drivers/base/driver.c
> index f00be99cdcbf..10d765e1a213 100644
> --- a/drivers/base/driver.c
> +++ b/drivers/base/driver.c
> @@ -277,7 +277,7 @@ int unregister_device(struct device *old_dev)
> }
>
> list_for_each_entry_safe(cdev, ct, &old_dev->cdevs, devices_list) {
> - if (cdev->master) {
> + if (cdev_is_partition(cdev)) {
> dev_dbg(old_dev, "unregister part %s\n", cdev->name);
> devfs_del_partition(cdev->name);
> }
> diff --git a/fs/devfs-core.c b/fs/devfs-core.c
> index fbcf68e81597..bb66993b90f9 100644
> --- a/fs/devfs-core.c
> +++ b/fs/devfs-core.c
> @@ -38,7 +38,7 @@ int devfs_partition_complete(struct string_list *sl, char *instr)
> len = strlen(instr);
>
> for_each_cdev(cdev) {
> - if (cdev->master &&
> + if (cdev_is_partition(cdev) &&
> !strncmp(instr, cdev->name, len)) {
> string_list_add_asprintf(sl, "%s ", cdev->name);
> }
> @@ -101,7 +101,7 @@ struct cdev *cdev_by_partuuid(const char *partuuid)
> return NULL;
>
> for_each_cdev(cdev) {
> - if (cdev->master && !strcasecmp(cdev->uuid, partuuid))
> + if (cdev_is_partition(cdev) && !strcasecmp(cdev->uuid, partuuid))
> return cdev;
> }
> return NULL;
> @@ -115,7 +115,7 @@ struct cdev *cdev_by_diskuuid(const char *diskuuid)
> return NULL;
>
> for_each_cdev(cdev) {
> - if (!cdev->master && !strcasecmp(cdev->uuid, diskuuid))
> + if (!cdev_is_partition(cdev) && !strcasecmp(cdev->uuid, diskuuid))
> return cdev;
> }
> return NULL;
> @@ -393,7 +393,7 @@ int devfs_remove(struct cdev *cdev)
> list_for_each_entry_safe(c, tmp, &cdev->links, link_entry)
> devfs_remove(c);
>
> - if (cdev->master)
> + if (cdev_is_partition(cdev))
> list_del(&cdev->partition_entry);
>
> if (cdev->link)
> @@ -549,7 +549,7 @@ int devfs_del_partition(const char *name)
> return ret;
> }
>
> - if (!cdev->master)
> + if (!cdev_is_partition(cdev))
> return -EINVAL;
> if (cdev->flags & DEVFS_PARTITION_FIXED)
> return -EPERM;
> diff --git a/include/driver.h b/include/driver.h
> index e1ee3dc2dd7c..00b4a0e4af75 100644
> --- a/include/driver.h
> +++ b/include/driver.h
> @@ -564,6 +564,10 @@ int cdev_discard_range(struct cdev*, loff_t count, loff_t offset);
> int cdev_memmap(struct cdev*, void **map, int flags);
> int cdev_truncate(struct cdev*, size_t size);
> loff_t cdev_unallocated_space(struct cdev *cdev);
> +static inline bool cdev_is_partition(const struct cdev *cdev)
> +{
> + return cdev->master != NULL;
> +}
>
> extern struct list_head cdev_list;
> #define for_each_cdev(cdev) \
> --
> 2.39.2
>
>
>
^ permalink raw reply [flat|nested] 62+ messages in thread
* [PATCH 07/18] commands: stat: remove code duplication for type info
2023-05-31 14:59 [PATCH 00/18] state: allow lookup of barebox state partition by Type GUID Ahmad Fatoum
` (5 preceding siblings ...)
2023-05-31 14:59 ` [PATCH 06/18] driver: add new cdev_is_partition helper Ahmad Fatoum
@ 2023-05-31 14:59 ` Ahmad Fatoum
2023-05-31 16:44 ` Marco Felsch
2023-05-31 14:59 ` [PATCH 08/18] cdev: use more descriptive struct cdev::diskuuid/partuuid Ahmad Fatoum
` (10 subsequent siblings)
17 siblings, 1 reply; 62+ messages in thread
From: Ahmad Fatoum @ 2023-05-31 14:59 UTC (permalink / raw)
To: barebox; +Cc: Ahmad Fatoum
stat prints a line with partitioning/type info for cdevs, but not all
cdevs have these, so we want to skip printing when it's empty.
Instead of duplicating the check, just utilize printf returning the
number of characters written.
Signed-off-by: Ahmad Fatoum <a.fatoum@pengutronix.de>
---
fs/fs.c | 20 ++++++++++++--------
1 file changed, 12 insertions(+), 8 deletions(-)
diff --git a/fs/fs.c b/fs/fs.c
index 368458cc54f8..ba60766a065a 100644
--- a/fs/fs.c
+++ b/fs/fs.c
@@ -69,6 +69,8 @@ EXPORT_SYMBOL(mkmodestr);
void cdev_print(const struct cdev *cdev)
{
+ int nbytes;
+
if (cdev->dev || cdev->master || cdev->partname) {
printf("Origin: %s", dev_name(cdev->dev) ?: "None");
if (cdev->master)
@@ -96,15 +98,17 @@ void cdev_print(const struct cdev *cdev)
}
printf("\n");
- if (cdev->filetype || cdev->dos_partition_type || *cdev->uuid) {
- if (cdev->filetype)
- printf("Filetype: %s\t", file_type_to_string(cdev->filetype));
- if (cdev->dos_partition_type)
- printf("DOS parttype: 0x%02x\t", cdev->dos_partition_type);
- if (*cdev->uuid)
- printf("UUID: %s", cdev->uuid);
+ nbytes = 0;
+
+ if (cdev->filetype)
+ nbytes += printf("Filetype: %s\t", file_type_to_string(cdev->filetype));
+ if (cdev->dos_partition_type)
+ nbytes += printf("DOS parttype: 0x%02x\t", cdev->dos_partition_type);
+ if (*cdev->uuid)
+ nbytes += printf("UUID: %s", cdev->uuid);
+
+ if (nbytes)
printf("\n");
- }
}
EXPORT_SYMBOL(cdev_print);
--
2.39.2
^ permalink raw reply [flat|nested] 62+ messages in thread* Re: [PATCH 07/18] commands: stat: remove code duplication for type info
2023-05-31 14:59 ` [PATCH 07/18] commands: stat: remove code duplication for type info Ahmad Fatoum
@ 2023-05-31 16:44 ` Marco Felsch
0 siblings, 0 replies; 62+ messages in thread
From: Marco Felsch @ 2023-05-31 16:44 UTC (permalink / raw)
To: Ahmad Fatoum; +Cc: barebox
On 23-05-31, Ahmad Fatoum wrote:
> stat prints a line with partitioning/type info for cdevs, but not all
> cdevs have these, so we want to skip printing when it's empty.
> Instead of duplicating the check, just utilize printf returning the
> number of characters written.
>
> Signed-off-by: Ahmad Fatoum <a.fatoum@pengutronix.de>
Reviewed-by: Marco Felsch <m.felsch@pengutronix.de>
> ---
> fs/fs.c | 20 ++++++++++++--------
> 1 file changed, 12 insertions(+), 8 deletions(-)
>
> diff --git a/fs/fs.c b/fs/fs.c
> index 368458cc54f8..ba60766a065a 100644
> --- a/fs/fs.c
> +++ b/fs/fs.c
> @@ -69,6 +69,8 @@ EXPORT_SYMBOL(mkmodestr);
>
> void cdev_print(const struct cdev *cdev)
> {
> + int nbytes;
> +
> if (cdev->dev || cdev->master || cdev->partname) {
> printf("Origin: %s", dev_name(cdev->dev) ?: "None");
> if (cdev->master)
> @@ -96,15 +98,17 @@ void cdev_print(const struct cdev *cdev)
> }
> printf("\n");
>
> - if (cdev->filetype || cdev->dos_partition_type || *cdev->uuid) {
> - if (cdev->filetype)
> - printf("Filetype: %s\t", file_type_to_string(cdev->filetype));
> - if (cdev->dos_partition_type)
> - printf("DOS parttype: 0x%02x\t", cdev->dos_partition_type);
> - if (*cdev->uuid)
> - printf("UUID: %s", cdev->uuid);
> + nbytes = 0;
> +
> + if (cdev->filetype)
> + nbytes += printf("Filetype: %s\t", file_type_to_string(cdev->filetype));
> + if (cdev->dos_partition_type)
> + nbytes += printf("DOS parttype: 0x%02x\t", cdev->dos_partition_type);
> + if (*cdev->uuid)
> + nbytes += printf("UUID: %s", cdev->uuid);
> +
> + if (nbytes)
> printf("\n");
> - }
> }
> EXPORT_SYMBOL(cdev_print);
>
> --
> 2.39.2
>
>
>
^ permalink raw reply [flat|nested] 62+ messages in thread
* [PATCH 08/18] cdev: use more descriptive struct cdev::diskuuid/partuuid
2023-05-31 14:59 [PATCH 00/18] state: allow lookup of barebox state partition by Type GUID Ahmad Fatoum
` (6 preceding siblings ...)
2023-05-31 14:59 ` [PATCH 07/18] commands: stat: remove code duplication for type info Ahmad Fatoum
@ 2023-05-31 14:59 ` Ahmad Fatoum
2023-05-31 16:56 ` Marco Felsch
2023-05-31 14:59 ` [PATCH 09/18] cdev: record whether partition is parsed from OF Ahmad Fatoum
` (9 subsequent siblings)
17 siblings, 1 reply; 62+ messages in thread
From: Ahmad Fatoum @ 2023-05-31 14:59 UTC (permalink / raw)
To: barebox; +Cc: Ahmad Fatoum
The UUID field has different meanings:
For a master cdev:
- GPT Header DiskGUID if GPT-formatted
- MBR Header NT Disk Signature if MBR-formatted
For a partition cdev:
- GPT UniquePartitionGUID
- MBR Header NT Disk Signature followed by "-${partititon_number}"
Later code will add yet another UUID (Partition Type GUID), so let's
make existing code more readable by using either diskuuid or partuuid as
appropriate.
No functional change.
Signed-off-by: Ahmad Fatoum <a.fatoum@pengutronix.de>
---
common/bootm.c | 2 +-
common/partitions.c | 2 +-
common/partitions/dos.c | 2 +-
common/partitions/efi.c | 4 ++--
drivers/misc/storage-by-uuid.c | 4 ++--
fs/devfs-core.c | 4 ++--
fs/fs.c | 9 +++++----
include/driver.h | 10 +++++++++-
8 files changed, 23 insertions(+), 14 deletions(-)
diff --git a/common/bootm.c b/common/bootm.c
index 91a6e1688674..791d6b8fbbf1 100644
--- a/common/bootm.c
+++ b/common/bootm.c
@@ -734,7 +734,7 @@ int bootm_boot(struct bootm_data *bootm_data)
if (!root_cdev)
pr_err("no cdev found for %s, cannot set root= option\n",
root_dev_name);
- else if (!root_cdev->uuid[0])
+ else if (!root_cdev->partuuid[0])
pr_err("%s doesn't have a PARTUUID, cannot set root= option\n",
root_dev_name);
}
diff --git a/common/partitions.c b/common/partitions.c
index 9cca5c4a1546..b579559672a0 100644
--- a/common/partitions.c
+++ b/common/partitions.c
@@ -51,7 +51,7 @@ static int register_one_partition(struct block_device *blk,
cdev->flags |= DEVFS_PARTITION_FROM_TABLE;
cdev->dos_partition_type = part->dos_partition_type;
- strcpy(cdev->uuid, part->partuuid);
+ strcpy(cdev->partuuid, part->partuuid);
free(partition_name);
diff --git a/common/partitions/dos.c b/common/partitions/dos.c
index 566c8dd949b4..ad60c0b27b46 100644
--- a/common/partitions/dos.c
+++ b/common/partitions/dos.c
@@ -183,7 +183,7 @@ static void dos_partition(void *buf, struct block_device *blk,
uint32_t signature = get_unaligned_le32(buf + 0x1b8);
if (signature)
- sprintf(blk->cdev.uuid, "%08x", signature);
+ sprintf(blk->cdev.diskuuid, "%08x", signature);
table = (struct partition_entry *)&buffer[446];
diff --git a/common/partitions/efi.c b/common/partitions/efi.c
index a6c95f3969d1..780a8695e8a8 100644
--- a/common/partitions/efi.c
+++ b/common/partitions/efi.c
@@ -446,8 +446,8 @@ static void efi_partition(void *buf, struct block_device *blk,
return;
}
- snprintf(blk->cdev.uuid, sizeof(blk->cdev.uuid), "%pUl", &gpt->disk_guid);
- dev_add_param_string_fixed(blk->dev, "guid", blk->cdev.uuid);
+ snprintf(blk->cdev.diskuuid, sizeof(blk->cdev.diskuuid), "%pUl", &gpt->disk_guid);
+ dev_add_param_string_fixed(blk->dev, "guid", blk->cdev.diskuuid);
nb_part = le32_to_cpu(gpt->num_partition_entries);
diff --git a/drivers/misc/storage-by-uuid.c b/drivers/misc/storage-by-uuid.c
index a938bfaaa2c4..a7a66a1421a3 100644
--- a/drivers/misc/storage-by-uuid.c
+++ b/drivers/misc/storage-by-uuid.c
@@ -140,8 +140,8 @@ static void check_exist(struct sbu *sbu)
struct cdev *cdev;
for_each_cdev(cdev) {
- if (!strcmp(cdev->uuid, sbu->uuid)) {
- dev_dbg(sbu->dev, "Found %s %s\n", cdev->name, cdev->uuid);
+ if (!strcmp(cdev->diskuuid, sbu->uuid)) {
+ dev_dbg(sbu->dev, "Found %s %s\n", cdev->name, cdev->diskuuid);
storage_by_uuid_add_partitions(sbu, cdev);
}
}
diff --git a/fs/devfs-core.c b/fs/devfs-core.c
index bb66993b90f9..a0732dafca42 100644
--- a/fs/devfs-core.c
+++ b/fs/devfs-core.c
@@ -101,7 +101,7 @@ struct cdev *cdev_by_partuuid(const char *partuuid)
return NULL;
for_each_cdev(cdev) {
- if (cdev_is_partition(cdev) && !strcasecmp(cdev->uuid, partuuid))
+ if (cdev_is_partition(cdev) && !strcasecmp(cdev->partuuid, partuuid))
return cdev;
}
return NULL;
@@ -115,7 +115,7 @@ struct cdev *cdev_by_diskuuid(const char *diskuuid)
return NULL;
for_each_cdev(cdev) {
- if (!cdev_is_partition(cdev) && !strcasecmp(cdev->uuid, diskuuid))
+ if (!cdev_is_partition(cdev) && !strcasecmp(cdev->diskuuid, diskuuid))
return cdev;
}
return NULL;
diff --git a/fs/fs.c b/fs/fs.c
index ba60766a065a..1820e48393af 100644
--- a/fs/fs.c
+++ b/fs/fs.c
@@ -104,8 +104,9 @@ void cdev_print(const struct cdev *cdev)
nbytes += printf("Filetype: %s\t", file_type_to_string(cdev->filetype));
if (cdev->dos_partition_type)
nbytes += printf("DOS parttype: 0x%02x\t", cdev->dos_partition_type);
- if (*cdev->uuid)
- nbytes += printf("UUID: %s", cdev->uuid);
+ if (*cdev->partuuid || *cdev->diskuuid)
+ nbytes += printf("%sUUID: %s", cdev_is_partition(cdev) ? "PART" : "DISK",
+ cdev_is_partition(cdev) ? cdev->partuuid : cdev->diskuuid);
if (nbytes)
printf("\n");
@@ -3061,8 +3062,8 @@ char *cdev_get_linux_rootarg(const struct cdev *cdev)
if (str)
return str;
- if (cdev->uuid[0] != 0)
- return basprintf("root=PARTUUID=%s", cdev->uuid);
+ if (cdev->partuuid[0] != 0)
+ return basprintf("root=PARTUUID=%s", cdev->partuuid);
return NULL;
}
diff --git a/include/driver.h b/include/driver.h
index 00b4a0e4af75..42e513a15603 100644
--- a/include/driver.h
+++ b/include/driver.h
@@ -522,7 +522,15 @@ struct cdev {
char *partname; /* the partition name, usually the above without the
* device part, i.e. name = "nand0.barebox" -> partname = "barebox"
*/
- char uuid[MAX_UUID_STR];
+ union {
+ char diskuuid[MAX_UUID_STR]; /* GPT Header DiskGUID or
+ * MBR Header NT Disk Signature
+ */
+ char partuuid[MAX_UUID_STR]; /* GPT Partition Entry UniquePartitionGUID or
+ * MBR Partition Entry "${nt_signature}-${partno}"
+ */
+ };
+
loff_t offset;
loff_t size;
unsigned int flags;
--
2.39.2
^ permalink raw reply [flat|nested] 62+ messages in thread* Re: [PATCH 08/18] cdev: use more descriptive struct cdev::diskuuid/partuuid
2023-05-31 14:59 ` [PATCH 08/18] cdev: use more descriptive struct cdev::diskuuid/partuuid Ahmad Fatoum
@ 2023-05-31 16:56 ` Marco Felsch
0 siblings, 0 replies; 62+ messages in thread
From: Marco Felsch @ 2023-05-31 16:56 UTC (permalink / raw)
To: Ahmad Fatoum; +Cc: barebox
On 23-05-31, Ahmad Fatoum wrote:
> The UUID field has different meanings:
>
> For a master cdev:
>
> - GPT Header DiskGUID if GPT-formatted
> - MBR Header NT Disk Signature if MBR-formatted
>
> For a partition cdev:
>
> - GPT UniquePartitionGUID
> - MBR Header NT Disk Signature followed by "-${partititon_number}"
>
> Later code will add yet another UUID (Partition Type GUID), so let's
> make existing code more readable by using either diskuuid or partuuid as
> appropriate.
>
> No functional change.
>
> Signed-off-by: Ahmad Fatoum <a.fatoum@pengutronix.de>
Reviewed-by: Marco Felsch <m.felsch@pengutronix.de>
> ---
> common/bootm.c | 2 +-
> common/partitions.c | 2 +-
> common/partitions/dos.c | 2 +-
> common/partitions/efi.c | 4 ++--
> drivers/misc/storage-by-uuid.c | 4 ++--
> fs/devfs-core.c | 4 ++--
> fs/fs.c | 9 +++++----
> include/driver.h | 10 +++++++++-
> 8 files changed, 23 insertions(+), 14 deletions(-)
>
> diff --git a/common/bootm.c b/common/bootm.c
> index 91a6e1688674..791d6b8fbbf1 100644
> --- a/common/bootm.c
> +++ b/common/bootm.c
> @@ -734,7 +734,7 @@ int bootm_boot(struct bootm_data *bootm_data)
> if (!root_cdev)
> pr_err("no cdev found for %s, cannot set root= option\n",
> root_dev_name);
> - else if (!root_cdev->uuid[0])
> + else if (!root_cdev->partuuid[0])
> pr_err("%s doesn't have a PARTUUID, cannot set root= option\n",
> root_dev_name);
> }
> diff --git a/common/partitions.c b/common/partitions.c
> index 9cca5c4a1546..b579559672a0 100644
> --- a/common/partitions.c
> +++ b/common/partitions.c
> @@ -51,7 +51,7 @@ static int register_one_partition(struct block_device *blk,
> cdev->flags |= DEVFS_PARTITION_FROM_TABLE;
>
> cdev->dos_partition_type = part->dos_partition_type;
> - strcpy(cdev->uuid, part->partuuid);
> + strcpy(cdev->partuuid, part->partuuid);
>
> free(partition_name);
>
> diff --git a/common/partitions/dos.c b/common/partitions/dos.c
> index 566c8dd949b4..ad60c0b27b46 100644
> --- a/common/partitions/dos.c
> +++ b/common/partitions/dos.c
> @@ -183,7 +183,7 @@ static void dos_partition(void *buf, struct block_device *blk,
> uint32_t signature = get_unaligned_le32(buf + 0x1b8);
>
> if (signature)
> - sprintf(blk->cdev.uuid, "%08x", signature);
> + sprintf(blk->cdev.diskuuid, "%08x", signature);
>
> table = (struct partition_entry *)&buffer[446];
>
> diff --git a/common/partitions/efi.c b/common/partitions/efi.c
> index a6c95f3969d1..780a8695e8a8 100644
> --- a/common/partitions/efi.c
> +++ b/common/partitions/efi.c
> @@ -446,8 +446,8 @@ static void efi_partition(void *buf, struct block_device *blk,
> return;
> }
>
> - snprintf(blk->cdev.uuid, sizeof(blk->cdev.uuid), "%pUl", &gpt->disk_guid);
> - dev_add_param_string_fixed(blk->dev, "guid", blk->cdev.uuid);
> + snprintf(blk->cdev.diskuuid, sizeof(blk->cdev.diskuuid), "%pUl", &gpt->disk_guid);
> + dev_add_param_string_fixed(blk->dev, "guid", blk->cdev.diskuuid);
>
> nb_part = le32_to_cpu(gpt->num_partition_entries);
>
> diff --git a/drivers/misc/storage-by-uuid.c b/drivers/misc/storage-by-uuid.c
> index a938bfaaa2c4..a7a66a1421a3 100644
> --- a/drivers/misc/storage-by-uuid.c
> +++ b/drivers/misc/storage-by-uuid.c
> @@ -140,8 +140,8 @@ static void check_exist(struct sbu *sbu)
> struct cdev *cdev;
>
> for_each_cdev(cdev) {
> - if (!strcmp(cdev->uuid, sbu->uuid)) {
> - dev_dbg(sbu->dev, "Found %s %s\n", cdev->name, cdev->uuid);
> + if (!strcmp(cdev->diskuuid, sbu->uuid)) {
> + dev_dbg(sbu->dev, "Found %s %s\n", cdev->name, cdev->diskuuid);
> storage_by_uuid_add_partitions(sbu, cdev);
> }
> }
> diff --git a/fs/devfs-core.c b/fs/devfs-core.c
> index bb66993b90f9..a0732dafca42 100644
> --- a/fs/devfs-core.c
> +++ b/fs/devfs-core.c
> @@ -101,7 +101,7 @@ struct cdev *cdev_by_partuuid(const char *partuuid)
> return NULL;
>
> for_each_cdev(cdev) {
> - if (cdev_is_partition(cdev) && !strcasecmp(cdev->uuid, partuuid))
> + if (cdev_is_partition(cdev) && !strcasecmp(cdev->partuuid, partuuid))
> return cdev;
> }
> return NULL;
> @@ -115,7 +115,7 @@ struct cdev *cdev_by_diskuuid(const char *diskuuid)
> return NULL;
>
> for_each_cdev(cdev) {
> - if (!cdev_is_partition(cdev) && !strcasecmp(cdev->uuid, diskuuid))
> + if (!cdev_is_partition(cdev) && !strcasecmp(cdev->diskuuid, diskuuid))
> return cdev;
> }
> return NULL;
> diff --git a/fs/fs.c b/fs/fs.c
> index ba60766a065a..1820e48393af 100644
> --- a/fs/fs.c
> +++ b/fs/fs.c
> @@ -104,8 +104,9 @@ void cdev_print(const struct cdev *cdev)
> nbytes += printf("Filetype: %s\t", file_type_to_string(cdev->filetype));
> if (cdev->dos_partition_type)
> nbytes += printf("DOS parttype: 0x%02x\t", cdev->dos_partition_type);
> - if (*cdev->uuid)
> - nbytes += printf("UUID: %s", cdev->uuid);
> + if (*cdev->partuuid || *cdev->diskuuid)
> + nbytes += printf("%sUUID: %s", cdev_is_partition(cdev) ? "PART" : "DISK",
> + cdev_is_partition(cdev) ? cdev->partuuid : cdev->diskuuid);
>
> if (nbytes)
> printf("\n");
> @@ -3061,8 +3062,8 @@ char *cdev_get_linux_rootarg(const struct cdev *cdev)
> if (str)
> return str;
>
> - if (cdev->uuid[0] != 0)
> - return basprintf("root=PARTUUID=%s", cdev->uuid);
> + if (cdev->partuuid[0] != 0)
> + return basprintf("root=PARTUUID=%s", cdev->partuuid);
>
> return NULL;
> }
> diff --git a/include/driver.h b/include/driver.h
> index 00b4a0e4af75..42e513a15603 100644
> --- a/include/driver.h
> +++ b/include/driver.h
> @@ -522,7 +522,15 @@ struct cdev {
> char *partname; /* the partition name, usually the above without the
> * device part, i.e. name = "nand0.barebox" -> partname = "barebox"
> */
> - char uuid[MAX_UUID_STR];
> + union {
> + char diskuuid[MAX_UUID_STR]; /* GPT Header DiskGUID or
> + * MBR Header NT Disk Signature
> + */
> + char partuuid[MAX_UUID_STR]; /* GPT Partition Entry UniquePartitionGUID or
> + * MBR Partition Entry "${nt_signature}-${partno}"
> + */
> + };
> +
> loff_t offset;
> loff_t size;
> unsigned int flags;
> --
> 2.39.2
>
>
>
^ permalink raw reply [flat|nested] 62+ messages in thread
* [PATCH 09/18] cdev: record whether partition is parsed from OF
2023-05-31 14:59 [PATCH 00/18] state: allow lookup of barebox state partition by Type GUID Ahmad Fatoum
` (7 preceding siblings ...)
2023-05-31 14:59 ` [PATCH 08/18] cdev: use more descriptive struct cdev::diskuuid/partuuid Ahmad Fatoum
@ 2023-05-31 14:59 ` Ahmad Fatoum
2023-05-31 17:04 ` Marco Felsch
2023-06-01 8:03 ` Ulrich Ölmann
2023-05-31 14:59 ` [PATCH 10/18] cdev: have devfs_add_partition return existing identical partition, not NULL Ahmad Fatoum
` (8 subsequent siblings)
17 siblings, 2 replies; 62+ messages in thread
From: Ahmad Fatoum @ 2023-05-31 14:59 UTC (permalink / raw)
To: barebox; +Cc: Ahmad Fatoum
Later code will make it possible to define a on-disk-described partition
in the DT as well. For this reason, we can't assumed
DEVFS_PARTITION_FROM_TABLE to mean !DT, so let's add a dedicated flag
for that.
Signed-off-by: Ahmad Fatoum <a.fatoum@pengutronix.de>
---
drivers/of/partition.c | 5 +++--
fs/fs.c | 2 ++
include/driver.h | 5 +++--
3 files changed, 8 insertions(+), 4 deletions(-)
diff --git a/drivers/of/partition.c b/drivers/of/partition.c
index a70e503cec9e..15943502ce17 100644
--- a/drivers/of/partition.c
+++ b/drivers/of/partition.c
@@ -74,6 +74,7 @@ struct cdev *of_parse_partition(struct cdev *cdev, struct device_node *node)
}
new->device_node = node;
+ new->flags |= DEVFS_PARTITION_FROM_OF;
if (IS_ENABLED(CONFIG_NVMEM) && of_device_is_compatible(node, "nvmem-cells")) {
struct nvmem_device *nvmem = nvmem_partition_register(new);
@@ -162,7 +163,7 @@ int of_fixup_partitions(struct device_node *np, struct cdev *cdev)
return 0;
list_for_each_entry(partcdev, &cdev->partitions, partition_entry) {
- if (partcdev->flags & DEVFS_PARTITION_FROM_TABLE)
+ if (!(partcdev->flags & DEVFS_PARTITION_FROM_OF))
continue;
n_parts++;
}
@@ -213,7 +214,7 @@ int of_fixup_partitions(struct device_node *np, struct cdev *cdev)
u8 tmp[16 * 16]; /* Up to 64-bit address + 64-bit size */
loff_t partoffset;
- if (partcdev->flags & DEVFS_PARTITION_FROM_TABLE)
+ if (!(partcdev->flags & DEVFS_PARTITION_FROM_OF))
continue;
if (partcdev->mtd)
diff --git a/fs/fs.c b/fs/fs.c
index 1820e48393af..9d8aab268ca4 100644
--- a/fs/fs.c
+++ b/fs/fs.c
@@ -88,6 +88,8 @@ void cdev_print(const struct cdev *cdev)
printf(" fixed-partition");
if (cdev->flags & DEVFS_PARTITION_READONLY)
printf(" readonly-partition");
+ if (cdev->flags & DEVFS_PARTITION_FROM_OF)
+ printf(" of-partition");
if (cdev->flags & DEVFS_PARTITION_FROM_TABLE)
printf(" table-partition");
if (cdev->flags & DEVFS_IS_MCI_MAIN_PART_DEV)
diff --git a/include/driver.h b/include/driver.h
index 42e513a15603..118d2adb6750 100644
--- a/include/driver.h
+++ b/include/driver.h
@@ -584,8 +584,9 @@ extern struct list_head cdev_list;
#define DEVFS_PARTITION_FIXED (1U << 0)
#define DEVFS_PARTITION_READONLY (1U << 1)
#define DEVFS_IS_CHARACTER_DEV (1U << 3)
-#define DEVFS_PARTITION_FROM_TABLE (1U << 4)
-#define DEVFS_IS_MCI_MAIN_PART_DEV (1U << 5)
+#define DEVFS_IS_MCI_MAIN_PART_DEV (1U << 4)
+#define DEVFS_PARTITION_FROM_OF (1U << 5)
+#define DEVFS_PARTITION_FROM_TABLE (1U << 6)
struct cdev *devfs_add_partition(const char *devname, loff_t offset,
loff_t size, unsigned int flags, const char *name);
--
2.39.2
^ permalink raw reply [flat|nested] 62+ messages in thread* Re: [PATCH 09/18] cdev: record whether partition is parsed from OF
2023-05-31 14:59 ` [PATCH 09/18] cdev: record whether partition is parsed from OF Ahmad Fatoum
@ 2023-05-31 17:04 ` Marco Felsch
2023-06-06 19:31 ` Ahmad Fatoum
2023-06-01 8:03 ` Ulrich Ölmann
1 sibling, 1 reply; 62+ messages in thread
From: Marco Felsch @ 2023-05-31 17:04 UTC (permalink / raw)
To: Ahmad Fatoum; +Cc: barebox
Hi Ahmad,
On 23-05-31, Ahmad Fatoum wrote:
> Later code will make it possible to define a on-disk-described partition
> in the DT as well. For this reason, we can't assumed
> DEVFS_PARTITION_FROM_TABLE to mean !DT, so let's add a dedicated flag
> for that.
>
> Signed-off-by: Ahmad Fatoum <a.fatoum@pengutronix.de>
> ---
> drivers/of/partition.c | 5 +++--
> fs/fs.c | 2 ++
> include/driver.h | 5 +++--
> 3 files changed, 8 insertions(+), 4 deletions(-)
>
> diff --git a/drivers/of/partition.c b/drivers/of/partition.c
> index a70e503cec9e..15943502ce17 100644
> --- a/drivers/of/partition.c
> +++ b/drivers/of/partition.c
> @@ -74,6 +74,7 @@ struct cdev *of_parse_partition(struct cdev *cdev, struct device_node *node)
> }
>
> new->device_node = node;
> + new->flags |= DEVFS_PARTITION_FROM_OF;
>
> if (IS_ENABLED(CONFIG_NVMEM) && of_device_is_compatible(node, "nvmem-cells")) {
> struct nvmem_device *nvmem = nvmem_partition_register(new);
> @@ -162,7 +163,7 @@ int of_fixup_partitions(struct device_node *np, struct cdev *cdev)
> return 0;
>
> list_for_each_entry(partcdev, &cdev->partitions, partition_entry) {
> - if (partcdev->flags & DEVFS_PARTITION_FROM_TABLE)
> + if (!(partcdev->flags & DEVFS_PARTITION_FROM_OF))
Even though the code is already 'open-coded' I would suggest a macro like:
is_of_partition_cdev() or cdev_is_of_partition().
Reviewed-by: Marco Felsch <m.felsch@pengutronix.de>
> continue;
> n_parts++;
> }
> @@ -213,7 +214,7 @@ int of_fixup_partitions(struct device_node *np, struct cdev *cdev)
> u8 tmp[16 * 16]; /* Up to 64-bit address + 64-bit size */
> loff_t partoffset;
>
> - if (partcdev->flags & DEVFS_PARTITION_FROM_TABLE)
> + if (!(partcdev->flags & DEVFS_PARTITION_FROM_OF))
> continue;
>
> if (partcdev->mtd)
> diff --git a/fs/fs.c b/fs/fs.c
> index 1820e48393af..9d8aab268ca4 100644
> --- a/fs/fs.c
> +++ b/fs/fs.c
> @@ -88,6 +88,8 @@ void cdev_print(const struct cdev *cdev)
> printf(" fixed-partition");
> if (cdev->flags & DEVFS_PARTITION_READONLY)
> printf(" readonly-partition");
> + if (cdev->flags & DEVFS_PARTITION_FROM_OF)
> + printf(" of-partition");
> if (cdev->flags & DEVFS_PARTITION_FROM_TABLE)
> printf(" table-partition");
> if (cdev->flags & DEVFS_IS_MCI_MAIN_PART_DEV)
> diff --git a/include/driver.h b/include/driver.h
> index 42e513a15603..118d2adb6750 100644
> --- a/include/driver.h
> +++ b/include/driver.h
> @@ -584,8 +584,9 @@ extern struct list_head cdev_list;
> #define DEVFS_PARTITION_FIXED (1U << 0)
> #define DEVFS_PARTITION_READONLY (1U << 1)
> #define DEVFS_IS_CHARACTER_DEV (1U << 3)
> -#define DEVFS_PARTITION_FROM_TABLE (1U << 4)
> -#define DEVFS_IS_MCI_MAIN_PART_DEV (1U << 5)
> +#define DEVFS_IS_MCI_MAIN_PART_DEV (1U << 4)
> +#define DEVFS_PARTITION_FROM_OF (1U << 5)
> +#define DEVFS_PARTITION_FROM_TABLE (1U << 6)
>
> struct cdev *devfs_add_partition(const char *devname, loff_t offset,
> loff_t size, unsigned int flags, const char *name);
> --
> 2.39.2
>
>
>
^ permalink raw reply [flat|nested] 62+ messages in thread* Re: [PATCH 09/18] cdev: record whether partition is parsed from OF
2023-05-31 17:04 ` Marco Felsch
@ 2023-06-06 19:31 ` Ahmad Fatoum
0 siblings, 0 replies; 62+ messages in thread
From: Ahmad Fatoum @ 2023-06-06 19:31 UTC (permalink / raw)
To: Marco Felsch; +Cc: barebox
On 31.05.23 19:04, Marco Felsch wrote:
> Hi Ahmad,
>
> On 23-05-31, Ahmad Fatoum wrote:
>> Later code will make it possible to define a on-disk-described partition
>> in the DT as well. For this reason, we can't assumed
>> DEVFS_PARTITION_FROM_TABLE to mean !DT, so let's add a dedicated flag
>> for that.
>>
>> Signed-off-by: Ahmad Fatoum <a.fatoum@pengutronix.de>
>> ---
>> drivers/of/partition.c | 5 +++--
>> fs/fs.c | 2 ++
>> include/driver.h | 5 +++--
>> 3 files changed, 8 insertions(+), 4 deletions(-)
>>
>> diff --git a/drivers/of/partition.c b/drivers/of/partition.c
>> index a70e503cec9e..15943502ce17 100644
>> --- a/drivers/of/partition.c
>> +++ b/drivers/of/partition.c
>> @@ -74,6 +74,7 @@ struct cdev *of_parse_partition(struct cdev *cdev, struct device_node *node)
>> }
>>
>> new->device_node = node;
>> + new->flags |= DEVFS_PARTITION_FROM_OF;
>>
>> if (IS_ENABLED(CONFIG_NVMEM) && of_device_is_compatible(node, "nvmem-cells")) {
>> struct nvmem_device *nvmem = nvmem_partition_register(new);
>> @@ -162,7 +163,7 @@ int of_fixup_partitions(struct device_node *np, struct cdev *cdev)
>> return 0;
>>
>> list_for_each_entry(partcdev, &cdev->partitions, partition_entry) {
>> - if (partcdev->flags & DEVFS_PARTITION_FROM_TABLE)
>> + if (!(partcdev->flags & DEVFS_PARTITION_FROM_OF))
>
> Even though the code is already 'open-coded' I would suggest a macro like:
>
> is_of_partition_cdev() or cdev_is_of_partition().
DEVFS_PARTITION_FROM_OF is set in this file and it's read in this file.
I don't think having an indirection through a helper would bring much
benefit.
>
> Reviewed-by: Marco Felsch <m.felsch@pengutronix.de>
>
>> continue;
>> n_parts++;
>> }
>> @@ -213,7 +214,7 @@ int of_fixup_partitions(struct device_node *np, struct cdev *cdev)
>> u8 tmp[16 * 16]; /* Up to 64-bit address + 64-bit size */
>> loff_t partoffset;
>>
>> - if (partcdev->flags & DEVFS_PARTITION_FROM_TABLE)
>> + if (!(partcdev->flags & DEVFS_PARTITION_FROM_OF))
>> continue;
>>
>> if (partcdev->mtd)
>> diff --git a/fs/fs.c b/fs/fs.c
>> index 1820e48393af..9d8aab268ca4 100644
>> --- a/fs/fs.c
>> +++ b/fs/fs.c
>> @@ -88,6 +88,8 @@ void cdev_print(const struct cdev *cdev)
>> printf(" fixed-partition");
>> if (cdev->flags & DEVFS_PARTITION_READONLY)
>> printf(" readonly-partition");
>> + if (cdev->flags & DEVFS_PARTITION_FROM_OF)
>> + printf(" of-partition");
>> if (cdev->flags & DEVFS_PARTITION_FROM_TABLE)
>> printf(" table-partition");
>> if (cdev->flags & DEVFS_IS_MCI_MAIN_PART_DEV)
>> diff --git a/include/driver.h b/include/driver.h
>> index 42e513a15603..118d2adb6750 100644
>> --- a/include/driver.h
>> +++ b/include/driver.h
>> @@ -584,8 +584,9 @@ extern struct list_head cdev_list;
>> #define DEVFS_PARTITION_FIXED (1U << 0)
>> #define DEVFS_PARTITION_READONLY (1U << 1)
>> #define DEVFS_IS_CHARACTER_DEV (1U << 3)
>> -#define DEVFS_PARTITION_FROM_TABLE (1U << 4)
>> -#define DEVFS_IS_MCI_MAIN_PART_DEV (1U << 5)
>> +#define DEVFS_IS_MCI_MAIN_PART_DEV (1U << 4)
>> +#define DEVFS_PARTITION_FROM_OF (1U << 5)
>> +#define DEVFS_PARTITION_FROM_TABLE (1U << 6)
>>
>> struct cdev *devfs_add_partition(const char *devname, loff_t offset,
>> loff_t size, unsigned int flags, const char *name);
>> --
>> 2.39.2
>>
>>
>>
>
--
Pengutronix e.K. | |
Steuerwalder Str. 21 | http://www.pengutronix.de/ |
31137 Hildesheim, Germany | Phone: +49-5121-206917-0 |
Amtsgericht Hildesheim, HRA 2686 | Fax: +49-5121-206917-5555 |
^ permalink raw reply [flat|nested] 62+ messages in thread
* Re: [PATCH 09/18] cdev: record whether partition is parsed from OF
2023-05-31 14:59 ` [PATCH 09/18] cdev: record whether partition is parsed from OF Ahmad Fatoum
2023-05-31 17:04 ` Marco Felsch
@ 2023-06-01 8:03 ` Ulrich Ölmann
1 sibling, 0 replies; 62+ messages in thread
From: Ulrich Ölmann @ 2023-06-01 8:03 UTC (permalink / raw)
To: Ahmad Fatoum; +Cc: barebox
Hi Ahmad,
again some nitpicky typo fixes.
On Wed, May 31 2023 at 16:59 +0200, Ahmad Fatoum <a.fatoum@pengutronix.de> wrote:
> Later code will make it possible to define a on-disk-described partition
s/a on-disk-described/an on-disk-described/
> in the DT as well. For this reason, we can't assumed
s/assumed/assume/
Best regards
Ulrich
> DEVFS_PARTITION_FROM_TABLE to mean !DT, so let's add a dedicated flag
> for that.
>
> Signed-off-by: Ahmad Fatoum <a.fatoum@pengutronix.de>
> ---
> drivers/of/partition.c | 5 +++--
> fs/fs.c | 2 ++
> include/driver.h | 5 +++--
> 3 files changed, 8 insertions(+), 4 deletions(-)
>
> diff --git a/drivers/of/partition.c b/drivers/of/partition.c
> index a70e503cec9e..15943502ce17 100644
> --- a/drivers/of/partition.c
> +++ b/drivers/of/partition.c
> @@ -74,6 +74,7 @@ struct cdev *of_parse_partition(struct cdev *cdev, struct device_node *node)
> }
>
> new->device_node = node;
> + new->flags |= DEVFS_PARTITION_FROM_OF;
>
> if (IS_ENABLED(CONFIG_NVMEM) && of_device_is_compatible(node, "nvmem-cells")) {
> struct nvmem_device *nvmem = nvmem_partition_register(new);
> @@ -162,7 +163,7 @@ int of_fixup_partitions(struct device_node *np, struct cdev *cdev)
> return 0;
>
> list_for_each_entry(partcdev, &cdev->partitions, partition_entry) {
> - if (partcdev->flags & DEVFS_PARTITION_FROM_TABLE)
> + if (!(partcdev->flags & DEVFS_PARTITION_FROM_OF))
> continue;
> n_parts++;
> }
> @@ -213,7 +214,7 @@ int of_fixup_partitions(struct device_node *np, struct cdev *cdev)
> u8 tmp[16 * 16]; /* Up to 64-bit address + 64-bit size */
> loff_t partoffset;
>
> - if (partcdev->flags & DEVFS_PARTITION_FROM_TABLE)
> + if (!(partcdev->flags & DEVFS_PARTITION_FROM_OF))
> continue;
>
> if (partcdev->mtd)
> diff --git a/fs/fs.c b/fs/fs.c
> index 1820e48393af..9d8aab268ca4 100644
> --- a/fs/fs.c
> +++ b/fs/fs.c
> @@ -88,6 +88,8 @@ void cdev_print(const struct cdev *cdev)
> printf(" fixed-partition");
> if (cdev->flags & DEVFS_PARTITION_READONLY)
> printf(" readonly-partition");
> + if (cdev->flags & DEVFS_PARTITION_FROM_OF)
> + printf(" of-partition");
> if (cdev->flags & DEVFS_PARTITION_FROM_TABLE)
> printf(" table-partition");
> if (cdev->flags & DEVFS_IS_MCI_MAIN_PART_DEV)
> diff --git a/include/driver.h b/include/driver.h
> index 42e513a15603..118d2adb6750 100644
> --- a/include/driver.h
> +++ b/include/driver.h
> @@ -584,8 +584,9 @@ extern struct list_head cdev_list;
> #define DEVFS_PARTITION_FIXED (1U << 0)
> #define DEVFS_PARTITION_READONLY (1U << 1)
> #define DEVFS_IS_CHARACTER_DEV (1U << 3)
> -#define DEVFS_PARTITION_FROM_TABLE (1U << 4)
> -#define DEVFS_IS_MCI_MAIN_PART_DEV (1U << 5)
> +#define DEVFS_IS_MCI_MAIN_PART_DEV (1U << 4)
> +#define DEVFS_PARTITION_FROM_OF (1U << 5)
> +#define DEVFS_PARTITION_FROM_TABLE (1U << 6)
>
> struct cdev *devfs_add_partition(const char *devname, loff_t offset,
> loff_t size, unsigned int flags, const char *name);
--
Pengutronix e.K. | Ulrich Ölmann |
Industrial Linux Solutions | http://www.pengutronix.de/ |
Peiner Str. 6-8, 31137 Hildesheim, Germany | Phone: +49-5121-206917-0 |
Amtsgericht Hildesheim, HRA 2686 | Fax: +49-5121-206917-5555 |
^ permalink raw reply [flat|nested] 62+ messages in thread
* [PATCH 10/18] cdev: have devfs_add_partition return existing identical partition, not NULL
2023-05-31 14:59 [PATCH 00/18] state: allow lookup of barebox state partition by Type GUID Ahmad Fatoum
` (8 preceding siblings ...)
2023-05-31 14:59 ` [PATCH 09/18] cdev: record whether partition is parsed from OF Ahmad Fatoum
@ 2023-05-31 14:59 ` Ahmad Fatoum
2023-05-31 17:23 ` Marco Felsch
2023-06-01 7:36 ` Sascha Hauer
2023-05-31 14:59 ` [PATCH 11/18] block: parse partition table on block device registration Ahmad Fatoum
` (7 subsequent siblings)
17 siblings, 2 replies; 62+ messages in thread
From: Ahmad Fatoum @ 2023-05-31 14:59 UTC (permalink / raw)
To: barebox; +Cc: Ahmad Fatoum
Starting with commit 7f9f45b9bfef ("devfs: Do not create overlapping
partitions"), any overlapping is disallowed. Overlapping can be useful
though to bridge the gap between partition described in DT and via
on-disk partition tables. Let's handle the case of identical partitions
specially and have it neither be an error or a duplicate partition, but
instead just return the existing partition. This existing partition will
be given a device tree node and thus enabling schemes like:
&{/state} {
backend = <&state_part>;
};
&mmc1 {
partitions {
compatible = "fixed-partitions";
#address-cells = <2>;
#size-cells = <2>;
state_part: partition@5300000 {
label = "barebox-state";
/* will be folded with overlapping GPT partition if found */
reg = <0x0 0x5300000 0x0 0x100000>;
};
};
};
Signed-off-by: Ahmad Fatoum <a.fatoum@pengutronix.de>
---
fs/devfs-core.c | 50 ++++++++++++++++++++++++++++++++++++++-----------
1 file changed, 39 insertions(+), 11 deletions(-)
diff --git a/fs/devfs-core.c b/fs/devfs-core.c
index a0732dafca42..b3a274d01ee0 100644
--- a/fs/devfs-core.c
+++ b/fs/devfs-core.c
@@ -402,6 +402,12 @@ int devfs_remove(struct cdev *cdev)
return 0;
}
+static bool region_identical(loff_t starta, loff_t lena,
+ loff_t startb, loff_t lenb)
+{
+ return starta == startb && lena == lenb;
+}
+
static bool region_overlap(loff_t starta, loff_t lena,
loff_t startb, loff_t lenb)
{
@@ -412,10 +418,22 @@ static bool region_overlap(loff_t starta, loff_t lena,
return 1;
}
-static int check_overlap(struct cdev *cdev, const char *name, loff_t offset, loff_t size)
+/**
+ * check_overlap() - check overlap with existing partitions
+ * @cdev: parent cdev
+ * @name: partition name for informational purposes on conflict
+ * @offset: offset of new partition to be added
+ * @size: size of new partition to be added
+ *
+ * Return: NULL if no overlapping partition found or overlapping
+ * partition if and only if it's identical in offset and size
+ * to an existing partition. Otherwise, PTR_ERR(-EINVAL).
+ */
+static struct cdev *check_overlap(struct cdev *cdev, const char *name, loff_t offset, loff_t size)
{
struct cdev *cpart;
loff_t cpart_offset;
+ int ret;
list_for_each_entry(cpart, &cdev->partitions, partition_entry) {
cpart_offset = cpart->offset;
@@ -428,20 +446,28 @@ static int check_overlap(struct cdev *cdev, const char *name, loff_t offset, lof
if (cpart->mtd)
cpart_offset = cpart->mtd->master_offset;
- if (region_overlap(cpart_offset, cpart->size,
- offset, size))
+ if (region_identical(cpart_offset, cpart->size, offset, size)) {
+ ret = 0;
goto conflict;
+ }
+
+ if (region_overlap(cpart_offset, cpart->size, offset, size)) {
+ ret = -EINVAL;
+ goto conflict;
+ }
}
- return 0;
+ return NULL;
conflict:
- pr_err("New partition %s (0x%08llx-0x%08llx) on %s "
- "overlaps with partition %s (0x%08llx-0x%08llx), not creating it\n",
- name, offset, offset + size - 1, cdev->name,
- cpart->name, cpart_offset, cpart_offset + cpart->size - 1);
+ __pr_printk(ret ? MSG_WARNING : MSG_DEBUG,
+ "New partition %s (0x%08llx-0x%08llx) on %s "
+ "%s with partition %s (0x%08llx-0x%08llx), not creating it\n",
+ name, offset, offset + size - 1, cdev->name,
+ ret ? "conflicts" : "identical",
+ cpart->name, cpart_offset, cpart_offset + cpart->size - 1);
- return -EINVAL;
+ return ret ? ERR_PTR(ret) : cpart;
}
static struct cdev *__devfs_add_partition(struct cdev *cdev,
@@ -449,6 +475,7 @@ static struct cdev *__devfs_add_partition(struct cdev *cdev,
{
loff_t offset, size;
static struct cdev *new;
+ struct cdev *overlap;
if (cdev_by_name(partinfo->name))
return ERR_PTR(-EEXIST);
@@ -479,8 +506,9 @@ static struct cdev *__devfs_add_partition(struct cdev *cdev,
return ERR_PTR(-EINVAL);
}
- if (check_overlap(cdev, partinfo->name, offset, size))
- return ERR_PTR(-EINVAL);
+ overlap = check_overlap(cdev, partinfo->name, offset, size);
+ if (overlap)
+ return overlap;
if (IS_ENABLED(CONFIG_MTD) && cdev->mtd) {
struct mtd_info *mtd;
--
2.39.2
^ permalink raw reply [flat|nested] 62+ messages in thread* Re: [PATCH 10/18] cdev: have devfs_add_partition return existing identical partition, not NULL
2023-05-31 14:59 ` [PATCH 10/18] cdev: have devfs_add_partition return existing identical partition, not NULL Ahmad Fatoum
@ 2023-05-31 17:23 ` Marco Felsch
2023-06-01 4:56 ` Ahmad Fatoum
2023-06-01 7:36 ` Sascha Hauer
1 sibling, 1 reply; 62+ messages in thread
From: Marco Felsch @ 2023-05-31 17:23 UTC (permalink / raw)
To: Ahmad Fatoum; +Cc: barebox
Hi Ahmad,
On 23-05-31, Ahmad Fatoum wrote:
> Starting with commit 7f9f45b9bfef ("devfs: Do not create overlapping
> partitions"), any overlapping is disallowed. Overlapping can be useful
> though to bridge the gap between partition described in DT and via
> on-disk partition tables. Let's handle the case of identical partitions
> specially and have it neither be an error or a duplicate partition, but
> instead just return the existing partition. This existing partition will
> be given a device tree node and thus enabling schemes like:
>
> &{/state} {
> backend = <&state_part>;
> };
>
> &mmc1 {
> partitions {
> compatible = "fixed-partitions";
> #address-cells = <2>;
> #size-cells = <2>;
>
> state_part: partition@5300000 {
> label = "barebox-state";
> /* will be folded with overlapping GPT partition if found */
> reg = <0x0 0x5300000 0x0 0x100000>;
> };
> };
> };
>
> Signed-off-by: Ahmad Fatoum <a.fatoum@pengutronix.de>
> ---
> fs/devfs-core.c | 50 ++++++++++++++++++++++++++++++++++++++-----------
> 1 file changed, 39 insertions(+), 11 deletions(-)
>
> diff --git a/fs/devfs-core.c b/fs/devfs-core.c
> index a0732dafca42..b3a274d01ee0 100644
> --- a/fs/devfs-core.c
> +++ b/fs/devfs-core.c
> @@ -402,6 +402,12 @@ int devfs_remove(struct cdev *cdev)
> return 0;
> }
>
> +static bool region_identical(loff_t starta, loff_t lena,
> + loff_t startb, loff_t lenb)
> +{
> + return starta == startb && lena == lenb;
> +}
> +
> static bool region_overlap(loff_t starta, loff_t lena,
> loff_t startb, loff_t lenb)
> {
> @@ -412,10 +418,22 @@ static bool region_overlap(loff_t starta, loff_t lena,
> return 1;
> }
>
> -static int check_overlap(struct cdev *cdev, const char *name, loff_t offset, loff_t size)
> +/**
> + * check_overlap() - check overlap with existing partitions
> + * @cdev: parent cdev
> + * @name: partition name for informational purposes on conflict
> + * @offset: offset of new partition to be added
> + * @size: size of new partition to be added
> + *
> + * Return: NULL if no overlapping partition found or overlapping
> + * partition if and only if it's identical in offset and size
> + * to an existing partition. Otherwise, PTR_ERR(-EINVAL).
> + */
> +static struct cdev *check_overlap(struct cdev *cdev, const char *name, loff_t offset, loff_t size)
> {
> struct cdev *cpart;
> loff_t cpart_offset;
> + int ret;
>
> list_for_each_entry(cpart, &cdev->partitions, partition_entry) {
> cpart_offset = cpart->offset;
> @@ -428,20 +446,28 @@ static int check_overlap(struct cdev *cdev, const char *name, loff_t offset, lof
> if (cpart->mtd)
> cpart_offset = cpart->mtd->master_offset;
>
> - if (region_overlap(cpart_offset, cpart->size,
> - offset, size))
> + if (region_identical(cpart_offset, cpart->size, offset, size)) {
> + ret = 0;
> goto conflict;
> + }
The 'goto conflict' is a bit misleading here since this is no conflict
as you described within the commit message. I would rather do:
if (region_identical(cpart_offset, cpart->size, offset, size))
goto out_identical;
and replace the __pr_printk() by pr_debug(). This way you split
__pr_printk() and drop the ternary operator. The rest lgtm.
Regards,
Marco
> +
> + if (region_overlap(cpart_offset, cpart->size, offset, size)) {
> + ret = -EINVAL;
> + goto conflict;
> + }
> }
>
> - return 0;
> + return NULL;
>
> conflict:
> - pr_err("New partition %s (0x%08llx-0x%08llx) on %s "
> - "overlaps with partition %s (0x%08llx-0x%08llx), not creating it\n",
> - name, offset, offset + size - 1, cdev->name,
> - cpart->name, cpart_offset, cpart_offset + cpart->size - 1);
> + __pr_printk(ret ? MSG_WARNING : MSG_DEBUG,
> + "New partition %s (0x%08llx-0x%08llx) on %s "
> + "%s with partition %s (0x%08llx-0x%08llx), not creating it\n",
> + name, offset, offset + size - 1, cdev->name,
> + ret ? "conflicts" : "identical",
> + cpart->name, cpart_offset, cpart_offset + cpart->size - 1);
>
> - return -EINVAL;
> + return ret ? ERR_PTR(ret) : cpart;
> }
>
> static struct cdev *__devfs_add_partition(struct cdev *cdev,
> @@ -449,6 +475,7 @@ static struct cdev *__devfs_add_partition(struct cdev *cdev,
> {
> loff_t offset, size;
> static struct cdev *new;
> + struct cdev *overlap;
>
> if (cdev_by_name(partinfo->name))
> return ERR_PTR(-EEXIST);
> @@ -479,8 +506,9 @@ static struct cdev *__devfs_add_partition(struct cdev *cdev,
> return ERR_PTR(-EINVAL);
> }
>
> - if (check_overlap(cdev, partinfo->name, offset, size))
> - return ERR_PTR(-EINVAL);
> + overlap = check_overlap(cdev, partinfo->name, offset, size);
> + if (overlap)
> + return overlap;
>
> if (IS_ENABLED(CONFIG_MTD) && cdev->mtd) {
> struct mtd_info *mtd;
> --
> 2.39.2
>
>
>
^ permalink raw reply [flat|nested] 62+ messages in thread* Re: [PATCH 10/18] cdev: have devfs_add_partition return existing identical partition, not NULL
2023-05-31 17:23 ` Marco Felsch
@ 2023-06-01 4:56 ` Ahmad Fatoum
2023-06-01 7:32 ` Sascha Hauer
2023-06-01 8:26 ` Marco Felsch
0 siblings, 2 replies; 62+ messages in thread
From: Ahmad Fatoum @ 2023-06-01 4:56 UTC (permalink / raw)
To: Marco Felsch; +Cc: barebox
Hello Marco,
On 31.05.23 19:23, Marco Felsch wrote:
> Hi Ahmad,
>
> On 23-05-31, Ahmad Fatoum wrote:
>> Starting with commit 7f9f45b9bfef ("devfs: Do not create overlapping
>> partitions"), any overlapping is disallowed. Overlapping can be useful
>> though to bridge the gap between partition described in DT and via
>> on-disk partition tables. Let's handle the case of identical partitions
>> specially and have it neither be an error or a duplicate partition, but
>> instead just return the existing partition. This existing partition will
>> be given a device tree node and thus enabling schemes like:
>>
>> &{/state} {
>> backend = <&state_part>;
>> };
>>
>> &mmc1 {
>> partitions {
>> compatible = "fixed-partitions";
>> #address-cells = <2>;
>> #size-cells = <2>;
>>
>> state_part: partition@5300000 {
>> label = "barebox-state";
>> /* will be folded with overlapping GPT partition if found */
>> reg = <0x0 0x5300000 0x0 0x100000>;
>> };
>> };
>> };
>>
>> Signed-off-by: Ahmad Fatoum <a.fatoum@pengutronix.de>
>> ---
>> fs/devfs-core.c | 50 ++++++++++++++++++++++++++++++++++++++-----------
>> 1 file changed, 39 insertions(+), 11 deletions(-)
>>
>> diff --git a/fs/devfs-core.c b/fs/devfs-core.c
>> index a0732dafca42..b3a274d01ee0 100644
>> --- a/fs/devfs-core.c
>> +++ b/fs/devfs-core.c
>> @@ -402,6 +402,12 @@ int devfs_remove(struct cdev *cdev)
>> return 0;
>> }
>>
>> +static bool region_identical(loff_t starta, loff_t lena,
>> + loff_t startb, loff_t lenb)
>> +{
>> + return starta == startb && lena == lenb;
>> +}
>> +
>> static bool region_overlap(loff_t starta, loff_t lena,
>> loff_t startb, loff_t lenb)
>> {
>> @@ -412,10 +418,22 @@ static bool region_overlap(loff_t starta, loff_t lena,
>> return 1;
>> }
>>
>> -static int check_overlap(struct cdev *cdev, const char *name, loff_t offset, loff_t size)
>> +/**
>> + * check_overlap() - check overlap with existing partitions
>> + * @cdev: parent cdev
>> + * @name: partition name for informational purposes on conflict
>> + * @offset: offset of new partition to be added
>> + * @size: size of new partition to be added
>> + *
>> + * Return: NULL if no overlapping partition found or overlapping
>> + * partition if and only if it's identical in offset and size
>> + * to an existing partition. Otherwise, PTR_ERR(-EINVAL).
>> + */
>> +static struct cdev *check_overlap(struct cdev *cdev, const char *name, loff_t offset, loff_t size)
>> {
>> struct cdev *cpart;
>> loff_t cpart_offset;
>> + int ret;
>>
>> list_for_each_entry(cpart, &cdev->partitions, partition_entry) {
>> cpart_offset = cpart->offset;
>> @@ -428,20 +446,28 @@ static int check_overlap(struct cdev *cdev, const char *name, loff_t offset, lof
>> if (cpart->mtd)
>> cpart_offset = cpart->mtd->master_offset;
>>
>> - if (region_overlap(cpart_offset, cpart->size,
>> - offset, size))
>> + if (region_identical(cpart_offset, cpart->size, offset, size)) {
>> + ret = 0;
>> goto conflict;
>> + }
>
> The 'goto conflict' is a bit misleading here since this is no conflict
> as you described within the commit message.
It's still a conflict, but one that can be resolved by returning the existing
partition.
> I would rather do:
>
> if (region_identical(cpart_offset, cpart->size, offset, size))
> goto out_identical;
>
> and replace the __pr_printk() by pr_debug(). This way you split
> __pr_printk() and drop the ternary operator. The rest lgtm.
The print line is going to be long anyway, so why not share it between
the two cases?
>
> Regards,
> Marco
>
>> +
>> + if (region_overlap(cpart_offset, cpart->size, offset, size)) {
>> + ret = -EINVAL;
>> + goto conflict;
>> + }
>> }
>>
>> - return 0;
>> + return NULL;
>>
>> conflict:
>> - pr_err("New partition %s (0x%08llx-0x%08llx) on %s "
>> - "overlaps with partition %s (0x%08llx-0x%08llx), not creating it\n",
>> - name, offset, offset + size - 1, cdev->name,
>> - cpart->name, cpart_offset, cpart_offset + cpart->size - 1);
>> + __pr_printk(ret ? MSG_WARNING : MSG_DEBUG,
>> + "New partition %s (0x%08llx-0x%08llx) on %s "
>> + "%s with partition %s (0x%08llx-0x%08llx), not creating it\n",
>> + name, offset, offset + size - 1, cdev->name,
>> + ret ? "conflicts" : "identical",
>> + cpart->name, cpart_offset, cpart_offset + cpart->size - 1);
>>
>> - return -EINVAL;
>> + return ret ? ERR_PTR(ret) : cpart;
>> }
>>
>> static struct cdev *__devfs_add_partition(struct cdev *cdev,
>> @@ -449,6 +475,7 @@ static struct cdev *__devfs_add_partition(struct cdev *cdev,
>> {
>> loff_t offset, size;
>> static struct cdev *new;
>> + struct cdev *overlap;
>>
>> if (cdev_by_name(partinfo->name))
>> return ERR_PTR(-EEXIST);
>> @@ -479,8 +506,9 @@ static struct cdev *__devfs_add_partition(struct cdev *cdev,
>> return ERR_PTR(-EINVAL);
>> }
>>
>> - if (check_overlap(cdev, partinfo->name, offset, size))
>> - return ERR_PTR(-EINVAL);
>> + overlap = check_overlap(cdev, partinfo->name, offset, size);
>> + if (overlap)
>> + return overlap;
>>
>> if (IS_ENABLED(CONFIG_MTD) && cdev->mtd) {
>> struct mtd_info *mtd;
>> --
>> 2.39.2
>>
>>
>>
>
--
Pengutronix e.K. | |
Steuerwalder Str. 21 | http://www.pengutronix.de/ |
31137 Hildesheim, Germany | Phone: +49-5121-206917-0 |
Amtsgericht Hildesheim, HRA 2686 | Fax: +49-5121-206917-5555 |
^ permalink raw reply [flat|nested] 62+ messages in thread* Re: [PATCH 10/18] cdev: have devfs_add_partition return existing identical partition, not NULL
2023-06-01 4:56 ` Ahmad Fatoum
@ 2023-06-01 7:32 ` Sascha Hauer
2023-06-01 8:26 ` Marco Felsch
1 sibling, 0 replies; 62+ messages in thread
From: Sascha Hauer @ 2023-06-01 7:32 UTC (permalink / raw)
To: Ahmad Fatoum; +Cc: Marco Felsch, barebox
On Thu, Jun 01, 2023 at 06:56:47AM +0200, Ahmad Fatoum wrote:
> Hello Marco,
>
> On 31.05.23 19:23, Marco Felsch wrote:
> > Hi Ahmad,
> >
> > On 23-05-31, Ahmad Fatoum wrote:
> >> Starting with commit 7f9f45b9bfef ("devfs: Do not create overlapping
> >> partitions"), any overlapping is disallowed. Overlapping can be useful
> >> though to bridge the gap between partition described in DT and via
> >> on-disk partition tables. Let's handle the case of identical partitions
> >> specially and have it neither be an error or a duplicate partition, but
> >> instead just return the existing partition. This existing partition will
> >> be given a device tree node and thus enabling schemes like:
> >>
> >> &{/state} {
> >> backend = <&state_part>;
> >> };
> >>
> >> &mmc1 {
> >> partitions {
> >> compatible = "fixed-partitions";
> >> #address-cells = <2>;
> >> #size-cells = <2>;
> >>
> >> state_part: partition@5300000 {
> >> label = "barebox-state";
> >> /* will be folded with overlapping GPT partition if found */
> >> reg = <0x0 0x5300000 0x0 0x100000>;
> >> };
> >> };
> >> };
> >>
> >> Signed-off-by: Ahmad Fatoum <a.fatoum@pengutronix.de>
> >> ---
> >> fs/devfs-core.c | 50 ++++++++++++++++++++++++++++++++++++++-----------
> >> 1 file changed, 39 insertions(+), 11 deletions(-)
> >>
> >> diff --git a/fs/devfs-core.c b/fs/devfs-core.c
> >> index a0732dafca42..b3a274d01ee0 100644
> >> --- a/fs/devfs-core.c
> >> +++ b/fs/devfs-core.c
> >> @@ -402,6 +402,12 @@ int devfs_remove(struct cdev *cdev)
> >> return 0;
> >> }
> >>
> >> +static bool region_identical(loff_t starta, loff_t lena,
> >> + loff_t startb, loff_t lenb)
> >> +{
> >> + return starta == startb && lena == lenb;
> >> +}
> >> +
> >> static bool region_overlap(loff_t starta, loff_t lena,
> >> loff_t startb, loff_t lenb)
> >> {
> >> @@ -412,10 +418,22 @@ static bool region_overlap(loff_t starta, loff_t lena,
> >> return 1;
> >> }
> >>
> >> -static int check_overlap(struct cdev *cdev, const char *name, loff_t offset, loff_t size)
> >> +/**
> >> + * check_overlap() - check overlap with existing partitions
> >> + * @cdev: parent cdev
> >> + * @name: partition name for informational purposes on conflict
> >> + * @offset: offset of new partition to be added
> >> + * @size: size of new partition to be added
> >> + *
> >> + * Return: NULL if no overlapping partition found or overlapping
> >> + * partition if and only if it's identical in offset and size
> >> + * to an existing partition. Otherwise, PTR_ERR(-EINVAL).
> >> + */
> >> +static struct cdev *check_overlap(struct cdev *cdev, const char *name, loff_t offset, loff_t size)
> >> {
> >> struct cdev *cpart;
> >> loff_t cpart_offset;
> >> + int ret;
> >>
> >> list_for_each_entry(cpart, &cdev->partitions, partition_entry) {
> >> cpart_offset = cpart->offset;
> >> @@ -428,20 +446,28 @@ static int check_overlap(struct cdev *cdev, const char *name, loff_t offset, lof
> >> if (cpart->mtd)
> >> cpart_offset = cpart->mtd->master_offset;
> >>
> >> - if (region_overlap(cpart_offset, cpart->size,
> >> - offset, size))
> >> + if (region_identical(cpart_offset, cpart->size, offset, size)) {
> >> + ret = 0;
> >> goto conflict;
> >> + }
> >
> > The 'goto conflict' is a bit misleading here since this is no conflict
> > as you described within the commit message.
>
> It's still a conflict, but one that can be resolved by returning the existing
> partition.
As a compromise you could use a 'conflict' and 'identical' label and
place them directly under each other. No functional change, but makes
the intention clearer.
Sascha
--
Pengutronix e.K. | |
Steuerwalder Str. 21 | http://www.pengutronix.de/ |
31137 Hildesheim, Germany | Phone: +49-5121-206917-0 |
Amtsgericht Hildesheim, HRA 2686 | Fax: +49-5121-206917-5555 |
^ permalink raw reply [flat|nested] 62+ messages in thread* Re: [PATCH 10/18] cdev: have devfs_add_partition return existing identical partition, not NULL
2023-06-01 4:56 ` Ahmad Fatoum
2023-06-01 7:32 ` Sascha Hauer
@ 2023-06-01 8:26 ` Marco Felsch
1 sibling, 0 replies; 62+ messages in thread
From: Marco Felsch @ 2023-06-01 8:26 UTC (permalink / raw)
To: Ahmad Fatoum; +Cc: barebox
On 23-06-01, Ahmad Fatoum wrote:
> Hello Marco,
>
> On 31.05.23 19:23, Marco Felsch wrote:
> > Hi Ahmad,
> >
> > On 23-05-31, Ahmad Fatoum wrote:
> >> Starting with commit 7f9f45b9bfef ("devfs: Do not create overlapping
> >> partitions"), any overlapping is disallowed. Overlapping can be useful
> >> though to bridge the gap between partition described in DT and via
> >> on-disk partition tables. Let's handle the case of identical partitions
> >> specially and have it neither be an error or a duplicate partition, but
> >> instead just return the existing partition. This existing partition will
> >> be given a device tree node and thus enabling schemes like:
> >>
> >> &{/state} {
> >> backend = <&state_part>;
> >> };
> >>
> >> &mmc1 {
> >> partitions {
> >> compatible = "fixed-partitions";
> >> #address-cells = <2>;
> >> #size-cells = <2>;
> >>
> >> state_part: partition@5300000 {
> >> label = "barebox-state";
> >> /* will be folded with overlapping GPT partition if found */
> >> reg = <0x0 0x5300000 0x0 0x100000>;
> >> };
> >> };
> >> };
> >>
> >> Signed-off-by: Ahmad Fatoum <a.fatoum@pengutronix.de>
> >> ---
> >> fs/devfs-core.c | 50 ++++++++++++++++++++++++++++++++++++++-----------
> >> 1 file changed, 39 insertions(+), 11 deletions(-)
> >>
> >> diff --git a/fs/devfs-core.c b/fs/devfs-core.c
> >> index a0732dafca42..b3a274d01ee0 100644
> >> --- a/fs/devfs-core.c
> >> +++ b/fs/devfs-core.c
> >> @@ -402,6 +402,12 @@ int devfs_remove(struct cdev *cdev)
> >> return 0;
> >> }
> >>
> >> +static bool region_identical(loff_t starta, loff_t lena,
> >> + loff_t startb, loff_t lenb)
> >> +{
> >> + return starta == startb && lena == lenb;
> >> +}
> >> +
> >> static bool region_overlap(loff_t starta, loff_t lena,
> >> loff_t startb, loff_t lenb)
> >> {
> >> @@ -412,10 +418,22 @@ static bool region_overlap(loff_t starta, loff_t lena,
> >> return 1;
> >> }
> >>
> >> -static int check_overlap(struct cdev *cdev, const char *name, loff_t offset, loff_t size)
> >> +/**
> >> + * check_overlap() - check overlap with existing partitions
> >> + * @cdev: parent cdev
> >> + * @name: partition name for informational purposes on conflict
> >> + * @offset: offset of new partition to be added
> >> + * @size: size of new partition to be added
> >> + *
> >> + * Return: NULL if no overlapping partition found or overlapping
> >> + * partition if and only if it's identical in offset and size
> >> + * to an existing partition. Otherwise, PTR_ERR(-EINVAL).
> >> + */
> >> +static struct cdev *check_overlap(struct cdev *cdev, const char *name, loff_t offset, loff_t size)
> >> {
> >> struct cdev *cpart;
> >> loff_t cpart_offset;
> >> + int ret;
> >>
> >> list_for_each_entry(cpart, &cdev->partitions, partition_entry) {
> >> cpart_offset = cpart->offset;
> >> @@ -428,20 +446,28 @@ static int check_overlap(struct cdev *cdev, const char *name, loff_t offset, lof
> >> if (cpart->mtd)
> >> cpart_offset = cpart->mtd->master_offset;
> >>
> >> - if (region_overlap(cpart_offset, cpart->size,
> >> - offset, size))
> >> + if (region_identical(cpart_offset, cpart->size, offset, size)) {
> >> + ret = 0;
> >> goto conflict;
> >> + }
> >
> > The 'goto conflict' is a bit misleading here since this is no conflict
> > as you described within the commit message.
>
> It's still a conflict, but one that can be resolved by returning the existing
> partition.
>
> > I would rather do:
> >
> > if (region_identical(cpart_offset, cpart->size, offset, size))
> > goto out_identical;
> >
> > and replace the __pr_printk() by pr_debug(). This way you split
> > __pr_printk() and drop the ternary operator. The rest lgtm.
>
> The print line is going to be long anyway, so why not share it between
> the two cases?
It's long but at least to me it is easier to read and later on to grep
for albeit it already has many parameters. Anyway this is just a nit and
at least to me easier to read since you don't need the additional ret
parameter which you need to re-evaluate later on.
Regards,
Marco
>
> >
> > Regards,
> > Marco
> >
> >> +
> >> + if (region_overlap(cpart_offset, cpart->size, offset, size)) {
> >> + ret = -EINVAL;
> >> + goto conflict;
> >> + }
> >> }
> >>
> >> - return 0;
> >> + return NULL;
> >>
> >> conflict:
> >> - pr_err("New partition %s (0x%08llx-0x%08llx) on %s "
> >> - "overlaps with partition %s (0x%08llx-0x%08llx), not creating it\n",
> >> - name, offset, offset + size - 1, cdev->name,
> >> - cpart->name, cpart_offset, cpart_offset + cpart->size - 1);
> >> + __pr_printk(ret ? MSG_WARNING : MSG_DEBUG,
> >> + "New partition %s (0x%08llx-0x%08llx) on %s "
> >> + "%s with partition %s (0x%08llx-0x%08llx), not creating it\n",
> >> + name, offset, offset + size - 1, cdev->name,
> >> + ret ? "conflicts" : "identical",
> >> + cpart->name, cpart_offset, cpart_offset + cpart->size - 1);
> >>
> >> - return -EINVAL;
> >> + return ret ? ERR_PTR(ret) : cpart;
> >> }
> >>
> >> static struct cdev *__devfs_add_partition(struct cdev *cdev,
> >> @@ -449,6 +475,7 @@ static struct cdev *__devfs_add_partition(struct cdev *cdev,
> >> {
> >> loff_t offset, size;
> >> static struct cdev *new;
> >> + struct cdev *overlap;
> >>
> >> if (cdev_by_name(partinfo->name))
> >> return ERR_PTR(-EEXIST);
> >> @@ -479,8 +506,9 @@ static struct cdev *__devfs_add_partition(struct cdev *cdev,
> >> return ERR_PTR(-EINVAL);
> >> }
> >>
> >> - if (check_overlap(cdev, partinfo->name, offset, size))
> >> - return ERR_PTR(-EINVAL);
> >> + overlap = check_overlap(cdev, partinfo->name, offset, size);
> >> + if (overlap)
> >> + return overlap;
> >>
> >> if (IS_ENABLED(CONFIG_MTD) && cdev->mtd) {
> >> struct mtd_info *mtd;
> >> --
> >> 2.39.2
> >>
> >>
> >>
> >
>
> --
> Pengutronix e.K. | |
> Steuerwalder Str. 21 | http://www.pengutronix.de/ |
> 31137 Hildesheim, Germany | Phone: +49-5121-206917-0 |
> Amtsgericht Hildesheim, HRA 2686 | Fax: +49-5121-206917-5555 |
>
>
^ permalink raw reply [flat|nested] 62+ messages in thread
* Re: [PATCH 10/18] cdev: have devfs_add_partition return existing identical partition, not NULL
2023-05-31 14:59 ` [PATCH 10/18] cdev: have devfs_add_partition return existing identical partition, not NULL Ahmad Fatoum
2023-05-31 17:23 ` Marco Felsch
@ 2023-06-01 7:36 ` Sascha Hauer
2023-06-07 8:06 ` Ahmad Fatoum
1 sibling, 1 reply; 62+ messages in thread
From: Sascha Hauer @ 2023-06-01 7:36 UTC (permalink / raw)
To: Ahmad Fatoum; +Cc: barebox
On Wed, May 31, 2023 at 04:59:19PM +0200, Ahmad Fatoum wrote:
> Starting with commit 7f9f45b9bfef ("devfs: Do not create overlapping
> partitions"), any overlapping is disallowed. Overlapping can be useful
> though to bridge the gap between partition described in DT and via
> on-disk partition tables. Let's handle the case of identical partitions
> specially and have it neither be an error or a duplicate partition, but
> instead just return the existing partition. This existing partition will
> be given a device tree node and thus enabling schemes like:
>
> &{/state} {
> backend = <&state_part>;
> };
>
> &mmc1 {
> partitions {
> compatible = "fixed-partitions";
> #address-cells = <2>;
> #size-cells = <2>;
>
> state_part: partition@5300000 {
> label = "barebox-state";
> /* will be folded with overlapping GPT partition if found */
> reg = <0x0 0x5300000 0x0 0x100000>;
> };
> };
> };
You introduced the DEVFS_PARTITION_FROM_OF earlier this series.
Depending on the order the code runs you end up with either the
partition from the partition table or the one created from OF with
DEVFS_PARTITION_FROM_OF. I am not sure about the implications of
this inconsistency. Can it lead to problems later?
Sascha
--
Pengutronix e.K. | |
Steuerwalder Str. 21 | http://www.pengutronix.de/ |
31137 Hildesheim, Germany | Phone: +49-5121-206917-0 |
Amtsgericht Hildesheim, HRA 2686 | Fax: +49-5121-206917-5555 |
^ permalink raw reply [flat|nested] 62+ messages in thread* Re: [PATCH 10/18] cdev: have devfs_add_partition return existing identical partition, not NULL
2023-06-01 7:36 ` Sascha Hauer
@ 2023-06-07 8:06 ` Ahmad Fatoum
0 siblings, 0 replies; 62+ messages in thread
From: Ahmad Fatoum @ 2023-06-07 8:06 UTC (permalink / raw)
To: Sascha Hauer; +Cc: barebox
On 01.06.23 09:36, Sascha Hauer wrote:
> On Wed, May 31, 2023 at 04:59:19PM +0200, Ahmad Fatoum wrote:
>> Starting with commit 7f9f45b9bfef ("devfs: Do not create overlapping
>> partitions"), any overlapping is disallowed. Overlapping can be useful
>> though to bridge the gap between partition described in DT and via
>> on-disk partition tables. Let's handle the case of identical partitions
>> specially and have it neither be an error or a duplicate partition, but
>> instead just return the existing partition. This existing partition will
>> be given a device tree node and thus enabling schemes like:
>>
>> &{/state} {
>> backend = <&state_part>;
>> };
>>
>> &mmc1 {
>> partitions {
>> compatible = "fixed-partitions";
>> #address-cells = <2>;
>> #size-cells = <2>;
>>
>> state_part: partition@5300000 {
>> label = "barebox-state";
>> /* will be folded with overlapping GPT partition if found */
>> reg = <0x0 0x5300000 0x0 0x100000>;
>> };
>> };
>> };
>
> You introduced the DEVFS_PARTITION_FROM_OF earlier this series.
> Depending on the order the code runs you end up with either the
> partition from the partition table or the one created from OF with
> DEVFS_PARTITION_FROM_OF. I am not sure about the implications of
> this inconsistency. Can it lead to problems later?
Order is always the same: parse_partition_table is called before
of_parse_partitions. The latter is called on the cdev populated
by blockdevice_register at all call site.
Nevertheless, I have tested that you can reorder parse_partition_table
and of_parse_partitions, but there's a slight change in behavior: The first
partition that's registered is the namesake. I have adapted the code
to call devfs_create_link(overlap, partinfo->name); in that case. That way
we will always have all partitions in /dev regardless of order. The only
thing that will change, is which is a real cdev and which is a link.
Thanks,
Ahmad
>
> Sascha
>
--
Pengutronix e.K. | |
Steuerwalder Str. 21 | http://www.pengutronix.de/ |
31137 Hildesheim, Germany | Phone: +49-5121-206917-0 |
Amtsgericht Hildesheim, HRA 2686 | Fax: +49-5121-206917-5555 |
^ permalink raw reply [flat|nested] 62+ messages in thread
* [PATCH 11/18] block: parse partition table on block device registration
2023-05-31 14:59 [PATCH 00/18] state: allow lookup of barebox state partition by Type GUID Ahmad Fatoum
` (9 preceding siblings ...)
2023-05-31 14:59 ` [PATCH 10/18] cdev: have devfs_add_partition return existing identical partition, not NULL Ahmad Fatoum
@ 2023-05-31 14:59 ` Ahmad Fatoum
2023-05-31 17:25 ` Marco Felsch
` (2 more replies)
2023-05-31 14:59 ` [PATCH 12/18] common: partitions: record whether disk is GPT or MBR partitioned Ahmad Fatoum
` (6 subsequent siblings)
17 siblings, 3 replies; 62+ messages in thread
From: Ahmad Fatoum @ 2023-05-31 14:59 UTC (permalink / raw)
To: barebox; +Cc: Ahmad Fatoum
Every instance where we register a block device, it's followed by an
attempt to parse the partition table, most often with a warning when
it fails. Thus let's move partition table parsing into
blockdevice_register.
Signed-off-by: Ahmad Fatoum <a.fatoum@pengutronix.de>
---
arch/sandbox/board/hostfile.c | 4 ----
common/block.c | 6 ++++++
drivers/ata/disk_ata_drive.c | 5 -----
drivers/block/efi-block-io.c | 9 +--------
drivers/block/virtio_blk.c | 8 +-------
drivers/mci/mci-core.c | 6 ------
drivers/nvme/host/core.c | 5 -----
drivers/usb/storage/usb.c | 5 -----
8 files changed, 8 insertions(+), 40 deletions(-)
diff --git a/arch/sandbox/board/hostfile.c b/arch/sandbox/board/hostfile.c
index d0f400787d7a..a1ab06b87770 100644
--- a/arch/sandbox/board/hostfile.c
+++ b/arch/sandbox/board/hostfile.c
@@ -166,10 +166,6 @@ static int hf_probe(struct device *dev)
if (err)
return err;
- err = parse_partition_table(&priv->blk);
- if (err)
- dev_warn(dev, "No partition table found\n");
-
dev_info(dev, "registered as block device\n");
} else {
cdev->name = np->name;
diff --git a/common/block.c b/common/block.c
index c39269d3a692..98adcfdf3dab 100644
--- a/common/block.c
+++ b/common/block.c
@@ -6,6 +6,7 @@
*/
#include <common.h>
#include <block.h>
+#include <disks.h>
#include <malloc.h>
#include <linux/err.h>
#include <linux/list.h>
@@ -408,6 +409,11 @@ int blockdevice_register(struct block_device *blk)
cdev_create_default_automount(&blk->cdev);
+ /* Lack of partition table is unusual, but not a failure */
+ ret = parse_partition_table(blk);
+ if (ret)
+ dev_warn(blk->dev, "No partition table found\n");
+
return 0;
}
diff --git a/drivers/ata/disk_ata_drive.c b/drivers/ata/disk_ata_drive.c
index c1c736a0a88a..2d97710b827a 100644
--- a/drivers/ata/disk_ata_drive.c
+++ b/drivers/ata/disk_ata_drive.c
@@ -254,11 +254,6 @@ static int ata_port_init(struct ata_port *port)
dev_info(dev, "registered /dev/%s\n", port->blk.cdev.name);
- /* create partitions on demand */
- rc = parse_partition_table(&port->blk);
- if (rc != 0)
- dev_warn(dev, "No partition table found\n");
-
return 0;
on_error:
diff --git a/drivers/block/efi-block-io.c b/drivers/block/efi-block-io.c
index eb4981e86298..7162106ab8ea 100644
--- a/drivers/block/efi-block-io.c
+++ b/drivers/block/efi-block-io.c
@@ -12,7 +12,6 @@
#include <fcntl.h>
#include <efi.h>
#include <block.h>
-#include <disks.h>
#include <efi/efi-payload.h>
#include <efi/efi-device.h>
#include <bootsource.h>
@@ -184,16 +183,10 @@ static int efi_bio_probe(struct efi_device *efidev)
priv->media_id = media->media_id;
- ret = blockdevice_register(&priv->blk);
- if (ret)
- return ret;
-
if (efi_get_bootsource() == efidev)
bootsource_set_raw_instance(instance);
- parse_partition_table(&priv->blk);
-
- return 0;
+ return blockdevice_register(&priv->blk);
}
static struct efi_driver efi_bio_driver = {
diff --git a/drivers/block/virtio_blk.c b/drivers/block/virtio_blk.c
index 660f3a7b6b9b..11e52d9e6457 100644
--- a/drivers/block/virtio_blk.c
+++ b/drivers/block/virtio_blk.c
@@ -105,13 +105,7 @@ static int virtio_blk_probe(struct virtio_device *vdev)
priv->blk.num_blocks = cap;
priv->blk.ops = &virtio_blk_ops;
- ret = blockdevice_register(&priv->blk);
- if (ret)
- return ret;
-
- parse_partition_table(&priv->blk);
-
- return 0;
+ return blockdevice_register(&priv->blk);
}
static void virtio_blk_remove(struct virtio_device *vdev)
diff --git a/drivers/mci/mci-core.c b/drivers/mci/mci-core.c
index 6d0d6473770c..32edd5382386 100644
--- a/drivers/mci/mci-core.c
+++ b/drivers/mci/mci-core.c
@@ -1900,12 +1900,6 @@ static int mci_register_partition(struct mci_part *part)
return 0;
}
- rc = parse_partition_table(&part->blk);
- if (rc != 0) {
- /* Lack of partition table is unusual, but not a failure */
- dev_warn(&mci->dev, "No partition table found\n");
- }
-
if (np) {
of_parse_partitions(&part->blk.cdev, np);
diff --git a/drivers/nvme/host/core.c b/drivers/nvme/host/core.c
index bf9176ce0922..79a5f9325ef8 100644
--- a/drivers/nvme/host/core.c
+++ b/drivers/nvme/host/core.c
@@ -1,6 +1,5 @@
// SPDX-License-Identifier: GPL-2.0-only
#include <common.h>
-#include <disks.h>
#include "nvme.h"
@@ -373,10 +372,6 @@ static void nvme_alloc_ns(struct nvme_ctrl *ctrl, unsigned nsid)
goto out_free_id;
}
- ret = parse_partition_table(&ns->blk);
- if (ret)
- dev_warn(ctrl->dev, "No partition table found\n");
-
return;
out_free_id:
kfree(id);
diff --git a/drivers/usb/storage/usb.c b/drivers/usb/storage/usb.c
index 103ae293a3a4..dda713196071 100644
--- a/drivers/usb/storage/usb.c
+++ b/drivers/usb/storage/usb.c
@@ -420,11 +420,6 @@ static int usb_stor_add_blkdev(struct us_data *us, unsigned char lun)
goto BadDevice;
}
- /* create partitions on demand */
- result = parse_partition_table(&pblk_dev->blk);
- if (result != 0)
- dev_warn(dev, "No partition table found\n");
-
list_add_tail(&pblk_dev->list, &us->blk_dev_list);
dev_dbg(dev, "USB disk device successfully added\n");
--
2.39.2
^ permalink raw reply [flat|nested] 62+ messages in thread* Re: [PATCH 11/18] block: parse partition table on block device registration
2023-05-31 14:59 ` [PATCH 11/18] block: parse partition table on block device registration Ahmad Fatoum
@ 2023-05-31 17:25 ` Marco Felsch
2023-06-01 7:42 ` Sascha Hauer
2023-06-01 8:24 ` Ulrich Ölmann
2 siblings, 0 replies; 62+ messages in thread
From: Marco Felsch @ 2023-05-31 17:25 UTC (permalink / raw)
To: Ahmad Fatoum; +Cc: barebox
On 23-05-31, Ahmad Fatoum wrote:
> Every instance where we register a block device, it's followed by an
> attempt to parse the partition table, most often with a warning when
> it fails. Thus let's move partition table parsing into
> blockdevice_register.
>
> Signed-off-by: Ahmad Fatoum <a.fatoum@pengutronix.de>
Reviewed-by: Marco Felsch <m.felsch@pengutronix.de>
> ---
> arch/sandbox/board/hostfile.c | 4 ----
> common/block.c | 6 ++++++
> drivers/ata/disk_ata_drive.c | 5 -----
> drivers/block/efi-block-io.c | 9 +--------
> drivers/block/virtio_blk.c | 8 +-------
> drivers/mci/mci-core.c | 6 ------
> drivers/nvme/host/core.c | 5 -----
> drivers/usb/storage/usb.c | 5 -----
> 8 files changed, 8 insertions(+), 40 deletions(-)
>
> diff --git a/arch/sandbox/board/hostfile.c b/arch/sandbox/board/hostfile.c
> index d0f400787d7a..a1ab06b87770 100644
> --- a/arch/sandbox/board/hostfile.c
> +++ b/arch/sandbox/board/hostfile.c
> @@ -166,10 +166,6 @@ static int hf_probe(struct device *dev)
> if (err)
> return err;
>
> - err = parse_partition_table(&priv->blk);
> - if (err)
> - dev_warn(dev, "No partition table found\n");
> -
> dev_info(dev, "registered as block device\n");
> } else {
> cdev->name = np->name;
> diff --git a/common/block.c b/common/block.c
> index c39269d3a692..98adcfdf3dab 100644
> --- a/common/block.c
> +++ b/common/block.c
> @@ -6,6 +6,7 @@
> */
> #include <common.h>
> #include <block.h>
> +#include <disks.h>
> #include <malloc.h>
> #include <linux/err.h>
> #include <linux/list.h>
> @@ -408,6 +409,11 @@ int blockdevice_register(struct block_device *blk)
>
> cdev_create_default_automount(&blk->cdev);
>
> + /* Lack of partition table is unusual, but not a failure */
> + ret = parse_partition_table(blk);
> + if (ret)
> + dev_warn(blk->dev, "No partition table found\n");
> +
> return 0;
> }
>
> diff --git a/drivers/ata/disk_ata_drive.c b/drivers/ata/disk_ata_drive.c
> index c1c736a0a88a..2d97710b827a 100644
> --- a/drivers/ata/disk_ata_drive.c
> +++ b/drivers/ata/disk_ata_drive.c
> @@ -254,11 +254,6 @@ static int ata_port_init(struct ata_port *port)
>
> dev_info(dev, "registered /dev/%s\n", port->blk.cdev.name);
>
> - /* create partitions on demand */
> - rc = parse_partition_table(&port->blk);
> - if (rc != 0)
> - dev_warn(dev, "No partition table found\n");
> -
> return 0;
>
> on_error:
> diff --git a/drivers/block/efi-block-io.c b/drivers/block/efi-block-io.c
> index eb4981e86298..7162106ab8ea 100644
> --- a/drivers/block/efi-block-io.c
> +++ b/drivers/block/efi-block-io.c
> @@ -12,7 +12,6 @@
> #include <fcntl.h>
> #include <efi.h>
> #include <block.h>
> -#include <disks.h>
> #include <efi/efi-payload.h>
> #include <efi/efi-device.h>
> #include <bootsource.h>
> @@ -184,16 +183,10 @@ static int efi_bio_probe(struct efi_device *efidev)
>
> priv->media_id = media->media_id;
>
> - ret = blockdevice_register(&priv->blk);
> - if (ret)
> - return ret;
> -
> if (efi_get_bootsource() == efidev)
> bootsource_set_raw_instance(instance);
>
> - parse_partition_table(&priv->blk);
> -
> - return 0;
> + return blockdevice_register(&priv->blk);
> }
>
> static struct efi_driver efi_bio_driver = {
> diff --git a/drivers/block/virtio_blk.c b/drivers/block/virtio_blk.c
> index 660f3a7b6b9b..11e52d9e6457 100644
> --- a/drivers/block/virtio_blk.c
> +++ b/drivers/block/virtio_blk.c
> @@ -105,13 +105,7 @@ static int virtio_blk_probe(struct virtio_device *vdev)
> priv->blk.num_blocks = cap;
> priv->blk.ops = &virtio_blk_ops;
>
> - ret = blockdevice_register(&priv->blk);
> - if (ret)
> - return ret;
> -
> - parse_partition_table(&priv->blk);
> -
> - return 0;
> + return blockdevice_register(&priv->blk);
> }
>
> static void virtio_blk_remove(struct virtio_device *vdev)
> diff --git a/drivers/mci/mci-core.c b/drivers/mci/mci-core.c
> index 6d0d6473770c..32edd5382386 100644
> --- a/drivers/mci/mci-core.c
> +++ b/drivers/mci/mci-core.c
> @@ -1900,12 +1900,6 @@ static int mci_register_partition(struct mci_part *part)
> return 0;
> }
>
> - rc = parse_partition_table(&part->blk);
> - if (rc != 0) {
> - /* Lack of partition table is unusual, but not a failure */
> - dev_warn(&mci->dev, "No partition table found\n");
> - }
> -
> if (np) {
> of_parse_partitions(&part->blk.cdev, np);
>
> diff --git a/drivers/nvme/host/core.c b/drivers/nvme/host/core.c
> index bf9176ce0922..79a5f9325ef8 100644
> --- a/drivers/nvme/host/core.c
> +++ b/drivers/nvme/host/core.c
> @@ -1,6 +1,5 @@
> // SPDX-License-Identifier: GPL-2.0-only
> #include <common.h>
> -#include <disks.h>
>
> #include "nvme.h"
>
> @@ -373,10 +372,6 @@ static void nvme_alloc_ns(struct nvme_ctrl *ctrl, unsigned nsid)
> goto out_free_id;
> }
>
> - ret = parse_partition_table(&ns->blk);
> - if (ret)
> - dev_warn(ctrl->dev, "No partition table found\n");
> -
> return;
> out_free_id:
> kfree(id);
> diff --git a/drivers/usb/storage/usb.c b/drivers/usb/storage/usb.c
> index 103ae293a3a4..dda713196071 100644
> --- a/drivers/usb/storage/usb.c
> +++ b/drivers/usb/storage/usb.c
> @@ -420,11 +420,6 @@ static int usb_stor_add_blkdev(struct us_data *us, unsigned char lun)
> goto BadDevice;
> }
>
> - /* create partitions on demand */
> - result = parse_partition_table(&pblk_dev->blk);
> - if (result != 0)
> - dev_warn(dev, "No partition table found\n");
> -
> list_add_tail(&pblk_dev->list, &us->blk_dev_list);
> dev_dbg(dev, "USB disk device successfully added\n");
>
> --
> 2.39.2
>
>
>
^ permalink raw reply [flat|nested] 62+ messages in thread* Re: [PATCH 11/18] block: parse partition table on block device registration
2023-05-31 14:59 ` [PATCH 11/18] block: parse partition table on block device registration Ahmad Fatoum
2023-05-31 17:25 ` Marco Felsch
@ 2023-06-01 7:42 ` Sascha Hauer
2023-06-01 8:33 ` Marco Felsch
2023-06-06 19:30 ` Ahmad Fatoum
2023-06-01 8:24 ` Ulrich Ölmann
2 siblings, 2 replies; 62+ messages in thread
From: Sascha Hauer @ 2023-06-01 7:42 UTC (permalink / raw)
To: Ahmad Fatoum; +Cc: barebox
On Wed, May 31, 2023 at 04:59:20PM +0200, Ahmad Fatoum wrote:
> Every instance where we register a block device, it's followed by an
> attempt to parse the partition table, most often with a warning when
> it fails. Thus let's move partition table parsing into
> blockdevice_register.
>
> Signed-off-by: Ahmad Fatoum <a.fatoum@pengutronix.de>
> ---
> arch/sandbox/board/hostfile.c | 4 ----
> common/block.c | 6 ++++++
> drivers/ata/disk_ata_drive.c | 5 -----
> drivers/block/efi-block-io.c | 9 +--------
> drivers/block/virtio_blk.c | 8 +-------
> drivers/mci/mci-core.c | 6 ------
> drivers/nvme/host/core.c | 5 -----
> drivers/usb/storage/usb.c | 5 -----
> 8 files changed, 8 insertions(+), 40 deletions(-)
>
> diff --git a/arch/sandbox/board/hostfile.c b/arch/sandbox/board/hostfile.c
> index d0f400787d7a..a1ab06b87770 100644
> --- a/arch/sandbox/board/hostfile.c
> +++ b/arch/sandbox/board/hostfile.c
> @@ -166,10 +166,6 @@ static int hf_probe(struct device *dev)
> if (err)
> return err;
>
> - err = parse_partition_table(&priv->blk);
> - if (err)
> - dev_warn(dev, "No partition table found\n");
> -
> dev_info(dev, "registered as block device\n");
> } else {
> cdev->name = np->name;
> diff --git a/common/block.c b/common/block.c
> index c39269d3a692..98adcfdf3dab 100644
> --- a/common/block.c
> +++ b/common/block.c
> @@ -6,6 +6,7 @@
> */
> #include <common.h>
> #include <block.h>
> +#include <disks.h>
> #include <malloc.h>
> #include <linux/err.h>
> #include <linux/list.h>
> @@ -408,6 +409,11 @@ int blockdevice_register(struct block_device *blk)
>
> cdev_create_default_automount(&blk->cdev);
>
> + /* Lack of partition table is unusual, but not a failure */
> + ret = parse_partition_table(blk);
> + if (ret)
> + dev_warn(blk->dev, "No partition table found\n");
This is not changed in this series, so it's ok like this, but should
this really be a warning? Using a raw device without a partition table
seems like a legitimate usecase.
Sascha
--
Pengutronix e.K. | |
Steuerwalder Str. 21 | http://www.pengutronix.de/ |
31137 Hildesheim, Germany | Phone: +49-5121-206917-0 |
Amtsgericht Hildesheim, HRA 2686 | Fax: +49-5121-206917-5555 |
^ permalink raw reply [flat|nested] 62+ messages in thread* Re: [PATCH 11/18] block: parse partition table on block device registration
2023-06-01 7:42 ` Sascha Hauer
@ 2023-06-01 8:33 ` Marco Felsch
2023-06-06 19:30 ` Ahmad Fatoum
1 sibling, 0 replies; 62+ messages in thread
From: Marco Felsch @ 2023-06-01 8:33 UTC (permalink / raw)
To: Sascha Hauer; +Cc: Ahmad Fatoum, barebox
On 23-06-01, Sascha Hauer wrote:
> On Wed, May 31, 2023 at 04:59:20PM +0200, Ahmad Fatoum wrote:
> > Every instance where we register a block device, it's followed by an
> > attempt to parse the partition table, most often with a warning when
> > it fails. Thus let's move partition table parsing into
> > blockdevice_register.
> >
> > Signed-off-by: Ahmad Fatoum <a.fatoum@pengutronix.de>
> > ---
> > arch/sandbox/board/hostfile.c | 4 ----
> > common/block.c | 6 ++++++
> > drivers/ata/disk_ata_drive.c | 5 -----
> > drivers/block/efi-block-io.c | 9 +--------
> > drivers/block/virtio_blk.c | 8 +-------
> > drivers/mci/mci-core.c | 6 ------
> > drivers/nvme/host/core.c | 5 -----
> > drivers/usb/storage/usb.c | 5 -----
> > 8 files changed, 8 insertions(+), 40 deletions(-)
> >
> > diff --git a/arch/sandbox/board/hostfile.c b/arch/sandbox/board/hostfile.c
> > index d0f400787d7a..a1ab06b87770 100644
> > --- a/arch/sandbox/board/hostfile.c
> > +++ b/arch/sandbox/board/hostfile.c
> > @@ -166,10 +166,6 @@ static int hf_probe(struct device *dev)
> > if (err)
> > return err;
> >
> > - err = parse_partition_table(&priv->blk);
> > - if (err)
> > - dev_warn(dev, "No partition table found\n");
> > -
> > dev_info(dev, "registered as block device\n");
> > } else {
> > cdev->name = np->name;
> > diff --git a/common/block.c b/common/block.c
> > index c39269d3a692..98adcfdf3dab 100644
> > --- a/common/block.c
> > +++ b/common/block.c
> > @@ -6,6 +6,7 @@
> > */
> > #include <common.h>
> > #include <block.h>
> > +#include <disks.h>
> > #include <malloc.h>
> > #include <linux/err.h>
> > #include <linux/list.h>
> > @@ -408,6 +409,11 @@ int blockdevice_register(struct block_device *blk)
> >
> > cdev_create_default_automount(&blk->cdev);
> >
> > + /* Lack of partition table is unusual, but not a failure */
> > + ret = parse_partition_table(blk);
> > + if (ret)
> > + dev_warn(blk->dev, "No partition table found\n");
>
> This is not changed in this series, so it's ok like this, but should
> this really be a warning? Using a raw device without a partition table
> seems like a legitimate usecase.
Good point, maybe we should use dev_notice() as compromise?
Regards,
Marco
> Sascha
>
>
> --
> Pengutronix e.K. | |
> Steuerwalder Str. 21 | http://www.pengutronix.de/ |
> 31137 Hildesheim, Germany | Phone: +49-5121-206917-0 |
> Amtsgericht Hildesheim, HRA 2686 | Fax: +49-5121-206917-5555 |
>
>
^ permalink raw reply [flat|nested] 62+ messages in thread* Re: [PATCH 11/18] block: parse partition table on block device registration
2023-06-01 7:42 ` Sascha Hauer
2023-06-01 8:33 ` Marco Felsch
@ 2023-06-06 19:30 ` Ahmad Fatoum
1 sibling, 0 replies; 62+ messages in thread
From: Ahmad Fatoum @ 2023-06-06 19:30 UTC (permalink / raw)
To: Sascha Hauer; +Cc: barebox
On 01.06.23 09:42, Sascha Hauer wrote:
> On Wed, May 31, 2023 at 04:59:20PM +0200, Ahmad Fatoum wrote:
>> Every instance where we register a block device, it's followed by an
>> attempt to parse the partition table, most often with a warning when
>> it fails. Thus let's move partition table parsing into
>> blockdevice_register.
>>
>> Signed-off-by: Ahmad Fatoum <a.fatoum@pengutronix.de>
>> ---
>> arch/sandbox/board/hostfile.c | 4 ----
>> common/block.c | 6 ++++++
>> drivers/ata/disk_ata_drive.c | 5 -----
>> drivers/block/efi-block-io.c | 9 +--------
>> drivers/block/virtio_blk.c | 8 +-------
>> drivers/mci/mci-core.c | 6 ------
>> drivers/nvme/host/core.c | 5 -----
>> drivers/usb/storage/usb.c | 5 -----
>> 8 files changed, 8 insertions(+), 40 deletions(-)
>>
>> diff --git a/arch/sandbox/board/hostfile.c b/arch/sandbox/board/hostfile.c
>> index d0f400787d7a..a1ab06b87770 100644
>> --- a/arch/sandbox/board/hostfile.c
>> +++ b/arch/sandbox/board/hostfile.c
>> @@ -166,10 +166,6 @@ static int hf_probe(struct device *dev)
>> if (err)
>> return err;
>>
>> - err = parse_partition_table(&priv->blk);
>> - if (err)
>> - dev_warn(dev, "No partition table found\n");
>> -
>> dev_info(dev, "registered as block device\n");
>> } else {
>> cdev->name = np->name;
>> diff --git a/common/block.c b/common/block.c
>> index c39269d3a692..98adcfdf3dab 100644
>> --- a/common/block.c
>> +++ b/common/block.c
>> @@ -6,6 +6,7 @@
>> */
>> #include <common.h>
>> #include <block.h>
>> +#include <disks.h>
>> #include <malloc.h>
>> #include <linux/err.h>
>> #include <linux/list.h>
>> @@ -408,6 +409,11 @@ int blockdevice_register(struct block_device *blk)
>>
>> cdev_create_default_automount(&blk->cdev);
>>
>> + /* Lack of partition table is unusual, but not a failure */
>> + ret = parse_partition_table(blk);
>> + if (ret)
>> + dev_warn(blk->dev, "No partition table found\n");
>
> This is not changed in this series, so it's ok like this, but should
> this really be a warning? Using a raw device without a partition table
> seems like a legitimate usecase.
parse_partition_table returns 0 when no partition table exists. Now that
I looked into it, all errors already have an error print, so duplicating
it is unnecessary. I will drop the dev_warn altogether for v2.
>
> Sascha
>
>
--
Pengutronix e.K. | |
Steuerwalder Str. 21 | http://www.pengutronix.de/ |
31137 Hildesheim, Germany | Phone: +49-5121-206917-0 |
Amtsgericht Hildesheim, HRA 2686 | Fax: +49-5121-206917-5555 |
^ permalink raw reply [flat|nested] 62+ messages in thread
* Re: [PATCH 11/18] block: parse partition table on block device registration
2023-05-31 14:59 ` [PATCH 11/18] block: parse partition table on block device registration Ahmad Fatoum
2023-05-31 17:25 ` Marco Felsch
2023-06-01 7:42 ` Sascha Hauer
@ 2023-06-01 8:24 ` Ulrich Ölmann
2023-06-01 8:31 ` Ahmad Fatoum
2 siblings, 1 reply; 62+ messages in thread
From: Ulrich Ölmann @ 2023-06-01 8:24 UTC (permalink / raw)
To: Ahmad Fatoum; +Cc: barebox
Hi Ahmad,
again a small typo:
On Wed, May 31 2023 at 16:59 +0200, Ahmad Fatoum <a.fatoum@pengutronix.de> wrote:
> Every instance where we register a block device, it's followed by an
s/device, it's/device is/
Best regards
Ulrich
> attempt to parse the partition table, most often with a warning when
> it fails. Thus let's move partition table parsing into
> blockdevice_register.
>
> Signed-off-by: Ahmad Fatoum <a.fatoum@pengutronix.de>
> ---
> arch/sandbox/board/hostfile.c | 4 ----
> common/block.c | 6 ++++++
> drivers/ata/disk_ata_drive.c | 5 -----
> drivers/block/efi-block-io.c | 9 +--------
> drivers/block/virtio_blk.c | 8 +-------
> drivers/mci/mci-core.c | 6 ------
> drivers/nvme/host/core.c | 5 -----
> drivers/usb/storage/usb.c | 5 -----
> 8 files changed, 8 insertions(+), 40 deletions(-)
>
> diff --git a/arch/sandbox/board/hostfile.c b/arch/sandbox/board/hostfile.c
> index d0f400787d7a..a1ab06b87770 100644
> --- a/arch/sandbox/board/hostfile.c
> +++ b/arch/sandbox/board/hostfile.c
> @@ -166,10 +166,6 @@ static int hf_probe(struct device *dev)
> if (err)
> return err;
>
> - err = parse_partition_table(&priv->blk);
> - if (err)
> - dev_warn(dev, "No partition table found\n");
> -
> dev_info(dev, "registered as block device\n");
> } else {
> cdev->name = np->name;
> diff --git a/common/block.c b/common/block.c
> index c39269d3a692..98adcfdf3dab 100644
> --- a/common/block.c
> +++ b/common/block.c
> @@ -6,6 +6,7 @@
> */
> #include <common.h>
> #include <block.h>
> +#include <disks.h>
> #include <malloc.h>
> #include <linux/err.h>
> #include <linux/list.h>
> @@ -408,6 +409,11 @@ int blockdevice_register(struct block_device *blk)
>
> cdev_create_default_automount(&blk->cdev);
>
> + /* Lack of partition table is unusual, but not a failure */
> + ret = parse_partition_table(blk);
> + if (ret)
> + dev_warn(blk->dev, "No partition table found\n");
> +
> return 0;
> }
>
> diff --git a/drivers/ata/disk_ata_drive.c b/drivers/ata/disk_ata_drive.c
> index c1c736a0a88a..2d97710b827a 100644
> --- a/drivers/ata/disk_ata_drive.c
> +++ b/drivers/ata/disk_ata_drive.c
> @@ -254,11 +254,6 @@ static int ata_port_init(struct ata_port *port)
>
> dev_info(dev, "registered /dev/%s\n", port->blk.cdev.name);
>
> - /* create partitions on demand */
> - rc = parse_partition_table(&port->blk);
> - if (rc != 0)
> - dev_warn(dev, "No partition table found\n");
> -
> return 0;
>
> on_error:
> diff --git a/drivers/block/efi-block-io.c b/drivers/block/efi-block-io.c
> index eb4981e86298..7162106ab8ea 100644
> --- a/drivers/block/efi-block-io.c
> +++ b/drivers/block/efi-block-io.c
> @@ -12,7 +12,6 @@
> #include <fcntl.h>
> #include <efi.h>
> #include <block.h>
> -#include <disks.h>
> #include <efi/efi-payload.h>
> #include <efi/efi-device.h>
> #include <bootsource.h>
> @@ -184,16 +183,10 @@ static int efi_bio_probe(struct efi_device *efidev)
>
> priv->media_id = media->media_id;
>
> - ret = blockdevice_register(&priv->blk);
> - if (ret)
> - return ret;
> -
> if (efi_get_bootsource() == efidev)
> bootsource_set_raw_instance(instance);
>
> - parse_partition_table(&priv->blk);
> -
> - return 0;
> + return blockdevice_register(&priv->blk);
> }
>
> static struct efi_driver efi_bio_driver = {
> diff --git a/drivers/block/virtio_blk.c b/drivers/block/virtio_blk.c
> index 660f3a7b6b9b..11e52d9e6457 100644
> --- a/drivers/block/virtio_blk.c
> +++ b/drivers/block/virtio_blk.c
> @@ -105,13 +105,7 @@ static int virtio_blk_probe(struct virtio_device *vdev)
> priv->blk.num_blocks = cap;
> priv->blk.ops = &virtio_blk_ops;
>
> - ret = blockdevice_register(&priv->blk);
> - if (ret)
> - return ret;
> -
> - parse_partition_table(&priv->blk);
> -
> - return 0;
> + return blockdevice_register(&priv->blk);
> }
>
> static void virtio_blk_remove(struct virtio_device *vdev)
> diff --git a/drivers/mci/mci-core.c b/drivers/mci/mci-core.c
> index 6d0d6473770c..32edd5382386 100644
> --- a/drivers/mci/mci-core.c
> +++ b/drivers/mci/mci-core.c
> @@ -1900,12 +1900,6 @@ static int mci_register_partition(struct mci_part *part)
> return 0;
> }
>
> - rc = parse_partition_table(&part->blk);
> - if (rc != 0) {
> - /* Lack of partition table is unusual, but not a failure */
> - dev_warn(&mci->dev, "No partition table found\n");
> - }
> -
> if (np) {
> of_parse_partitions(&part->blk.cdev, np);
>
> diff --git a/drivers/nvme/host/core.c b/drivers/nvme/host/core.c
> index bf9176ce0922..79a5f9325ef8 100644
> --- a/drivers/nvme/host/core.c
> +++ b/drivers/nvme/host/core.c
> @@ -1,6 +1,5 @@
> // SPDX-License-Identifier: GPL-2.0-only
> #include <common.h>
> -#include <disks.h>
>
> #include "nvme.h"
>
> @@ -373,10 +372,6 @@ static void nvme_alloc_ns(struct nvme_ctrl *ctrl, unsigned nsid)
> goto out_free_id;
> }
>
> - ret = parse_partition_table(&ns->blk);
> - if (ret)
> - dev_warn(ctrl->dev, "No partition table found\n");
> -
> return;
> out_free_id:
> kfree(id);
> diff --git a/drivers/usb/storage/usb.c b/drivers/usb/storage/usb.c
> index 103ae293a3a4..dda713196071 100644
> --- a/drivers/usb/storage/usb.c
> +++ b/drivers/usb/storage/usb.c
> @@ -420,11 +420,6 @@ static int usb_stor_add_blkdev(struct us_data *us, unsigned char lun)
> goto BadDevice;
> }
>
> - /* create partitions on demand */
> - result = parse_partition_table(&pblk_dev->blk);
> - if (result != 0)
> - dev_warn(dev, "No partition table found\n");
> -
> list_add_tail(&pblk_dev->list, &us->blk_dev_list);
> dev_dbg(dev, "USB disk device successfully added\n");
--
Pengutronix e.K. | Ulrich Ölmann |
Industrial Linux Solutions | http://www.pengutronix.de/ |
Peiner Str. 6-8, 31137 Hildesheim, Germany | Phone: +49-5121-206917-0 |
Amtsgericht Hildesheim, HRA 2686 | Fax: +49-5121-206917-5555 |
^ permalink raw reply [flat|nested] 62+ messages in thread* Re: [PATCH 11/18] block: parse partition table on block device registration
2023-06-01 8:24 ` Ulrich Ölmann
@ 2023-06-01 8:31 ` Ahmad Fatoum
2023-06-01 10:33 ` Ahmad Fatoum
0 siblings, 1 reply; 62+ messages in thread
From: Ahmad Fatoum @ 2023-06-01 8:31 UTC (permalink / raw)
To: Ulrich Ölmann; +Cc: barebox
On 01.06.23 10:24, Ulrich Ölmann wrote:
> Hi Ahmad,
>
> again a small typo:
>
> On Wed, May 31 2023 at 16:59 +0200, Ahmad Fatoum <a.fatoum@pengutronix.de> wrote:
>> Every instance where we register a block device, it's followed by an
>
> s/device, it's/device is/
I don't think that's correct. Should I change it to be On every instance [...]?
Thanks,
Ahmad
>
> Best regards
> Ulrich
>
>
>> attempt to parse the partition table, most often with a warning when
>> it fails. Thus let's move partition table parsing into
>> blockdevice_register.
>>
>> Signed-off-by: Ahmad Fatoum <a.fatoum@pengutronix.de>
>> ---
>> arch/sandbox/board/hostfile.c | 4 ----
>> common/block.c | 6 ++++++
>> drivers/ata/disk_ata_drive.c | 5 -----
>> drivers/block/efi-block-io.c | 9 +--------
>> drivers/block/virtio_blk.c | 8 +-------
>> drivers/mci/mci-core.c | 6 ------
>> drivers/nvme/host/core.c | 5 -----
>> drivers/usb/storage/usb.c | 5 -----
>> 8 files changed, 8 insertions(+), 40 deletions(-)
>>
>> diff --git a/arch/sandbox/board/hostfile.c b/arch/sandbox/board/hostfile.c
>> index d0f400787d7a..a1ab06b87770 100644
>> --- a/arch/sandbox/board/hostfile.c
>> +++ b/arch/sandbox/board/hostfile.c
>> @@ -166,10 +166,6 @@ static int hf_probe(struct device *dev)
>> if (err)
>> return err;
>>
>> - err = parse_partition_table(&priv->blk);
>> - if (err)
>> - dev_warn(dev, "No partition table found\n");
>> -
>> dev_info(dev, "registered as block device\n");
>> } else {
>> cdev->name = np->name;
>> diff --git a/common/block.c b/common/block.c
>> index c39269d3a692..98adcfdf3dab 100644
>> --- a/common/block.c
>> +++ b/common/block.c
>> @@ -6,6 +6,7 @@
>> */
>> #include <common.h>
>> #include <block.h>
>> +#include <disks.h>
>> #include <malloc.h>
>> #include <linux/err.h>
>> #include <linux/list.h>
>> @@ -408,6 +409,11 @@ int blockdevice_register(struct block_device *blk)
>>
>> cdev_create_default_automount(&blk->cdev);
>>
>> + /* Lack of partition table is unusual, but not a failure */
>> + ret = parse_partition_table(blk);
>> + if (ret)
>> + dev_warn(blk->dev, "No partition table found\n");
>> +
>> return 0;
>> }
>>
>> diff --git a/drivers/ata/disk_ata_drive.c b/drivers/ata/disk_ata_drive.c
>> index c1c736a0a88a..2d97710b827a 100644
>> --- a/drivers/ata/disk_ata_drive.c
>> +++ b/drivers/ata/disk_ata_drive.c
>> @@ -254,11 +254,6 @@ static int ata_port_init(struct ata_port *port)
>>
>> dev_info(dev, "registered /dev/%s\n", port->blk.cdev.name);
>>
>> - /* create partitions on demand */
>> - rc = parse_partition_table(&port->blk);
>> - if (rc != 0)
>> - dev_warn(dev, "No partition table found\n");
>> -
>> return 0;
>>
>> on_error:
>> diff --git a/drivers/block/efi-block-io.c b/drivers/block/efi-block-io.c
>> index eb4981e86298..7162106ab8ea 100644
>> --- a/drivers/block/efi-block-io.c
>> +++ b/drivers/block/efi-block-io.c
>> @@ -12,7 +12,6 @@
>> #include <fcntl.h>
>> #include <efi.h>
>> #include <block.h>
>> -#include <disks.h>
>> #include <efi/efi-payload.h>
>> #include <efi/efi-device.h>
>> #include <bootsource.h>
>> @@ -184,16 +183,10 @@ static int efi_bio_probe(struct efi_device *efidev)
>>
>> priv->media_id = media->media_id;
>>
>> - ret = blockdevice_register(&priv->blk);
>> - if (ret)
>> - return ret;
>> -
>> if (efi_get_bootsource() == efidev)
>> bootsource_set_raw_instance(instance);
>>
>> - parse_partition_table(&priv->blk);
>> -
>> - return 0;
>> + return blockdevice_register(&priv->blk);
>> }
>>
>> static struct efi_driver efi_bio_driver = {
>> diff --git a/drivers/block/virtio_blk.c b/drivers/block/virtio_blk.c
>> index 660f3a7b6b9b..11e52d9e6457 100644
>> --- a/drivers/block/virtio_blk.c
>> +++ b/drivers/block/virtio_blk.c
>> @@ -105,13 +105,7 @@ static int virtio_blk_probe(struct virtio_device *vdev)
>> priv->blk.num_blocks = cap;
>> priv->blk.ops = &virtio_blk_ops;
>>
>> - ret = blockdevice_register(&priv->blk);
>> - if (ret)
>> - return ret;
>> -
>> - parse_partition_table(&priv->blk);
>> -
>> - return 0;
>> + return blockdevice_register(&priv->blk);
>> }
>>
>> static void virtio_blk_remove(struct virtio_device *vdev)
>> diff --git a/drivers/mci/mci-core.c b/drivers/mci/mci-core.c
>> index 6d0d6473770c..32edd5382386 100644
>> --- a/drivers/mci/mci-core.c
>> +++ b/drivers/mci/mci-core.c
>> @@ -1900,12 +1900,6 @@ static int mci_register_partition(struct mci_part *part)
>> return 0;
>> }
>>
>> - rc = parse_partition_table(&part->blk);
>> - if (rc != 0) {
>> - /* Lack of partition table is unusual, but not a failure */
>> - dev_warn(&mci->dev, "No partition table found\n");
>> - }
>> -
>> if (np) {
>> of_parse_partitions(&part->blk.cdev, np);
>>
>> diff --git a/drivers/nvme/host/core.c b/drivers/nvme/host/core.c
>> index bf9176ce0922..79a5f9325ef8 100644
>> --- a/drivers/nvme/host/core.c
>> +++ b/drivers/nvme/host/core.c
>> @@ -1,6 +1,5 @@
>> // SPDX-License-Identifier: GPL-2.0-only
>> #include <common.h>
>> -#include <disks.h>
>>
>> #include "nvme.h"
>>
>> @@ -373,10 +372,6 @@ static void nvme_alloc_ns(struct nvme_ctrl *ctrl, unsigned nsid)
>> goto out_free_id;
>> }
>>
>> - ret = parse_partition_table(&ns->blk);
>> - if (ret)
>> - dev_warn(ctrl->dev, "No partition table found\n");
>> -
>> return;
>> out_free_id:
>> kfree(id);
>> diff --git a/drivers/usb/storage/usb.c b/drivers/usb/storage/usb.c
>> index 103ae293a3a4..dda713196071 100644
>> --- a/drivers/usb/storage/usb.c
>> +++ b/drivers/usb/storage/usb.c
>> @@ -420,11 +420,6 @@ static int usb_stor_add_blkdev(struct us_data *us, unsigned char lun)
>> goto BadDevice;
>> }
>>
>> - /* create partitions on demand */
>> - result = parse_partition_table(&pblk_dev->blk);
>> - if (result != 0)
>> - dev_warn(dev, "No partition table found\n");
>> -
>> list_add_tail(&pblk_dev->list, &us->blk_dev_list);
>> dev_dbg(dev, "USB disk device successfully added\n");
--
Pengutronix e.K. | |
Steuerwalder Str. 21 | http://www.pengutronix.de/ |
31137 Hildesheim, Germany | Phone: +49-5121-206917-0 |
Amtsgericht Hildesheim, HRA 2686 | Fax: +49-5121-206917-5555 |
^ permalink raw reply [flat|nested] 62+ messages in thread* Re: [PATCH 11/18] block: parse partition table on block device registration
2023-06-01 8:31 ` Ahmad Fatoum
@ 2023-06-01 10:33 ` Ahmad Fatoum
0 siblings, 0 replies; 62+ messages in thread
From: Ahmad Fatoum @ 2023-06-01 10:33 UTC (permalink / raw)
To: Ulrich Ölmann; +Cc: barebox
On 01.06.23 10:31, Ahmad Fatoum wrote:
> On 01.06.23 10:24, Ulrich Ölmann wrote:
>> Hi Ahmad,
>>
>> again a small typo:
>>
>> On Wed, May 31 2023 at 16:59 +0200, Ahmad Fatoum <a.fatoum@pengutronix.de> wrote:
>>> Every instance where we register a block device, it's followed by an
>>
>> s/device, it's/device is/
>
> I don't think that's correct. Should I change it to be On every instance [...]?
I see what you mean now. "Every instance [ ... ] is" is what I meant to say.
I will change it and add a comma before, where.
Thanks!
Ahmad
>
> Thanks,
> Ahmad
>
>>
>> Best regards
>> Ulrich
>>
>>
>>> attempt to parse the partition table, most often with a warning when
>>> it fails. Thus let's move partition table parsing into
>>> blockdevice_register.
>>>
>>> Signed-off-by: Ahmad Fatoum <a.fatoum@pengutronix.de>
>>> ---
>>> arch/sandbox/board/hostfile.c | 4 ----
>>> common/block.c | 6 ++++++
>>> drivers/ata/disk_ata_drive.c | 5 -----
>>> drivers/block/efi-block-io.c | 9 +--------
>>> drivers/block/virtio_blk.c | 8 +-------
>>> drivers/mci/mci-core.c | 6 ------
>>> drivers/nvme/host/core.c | 5 -----
>>> drivers/usb/storage/usb.c | 5 -----
>>> 8 files changed, 8 insertions(+), 40 deletions(-)
>>>
>>> diff --git a/arch/sandbox/board/hostfile.c b/arch/sandbox/board/hostfile.c
>>> index d0f400787d7a..a1ab06b87770 100644
>>> --- a/arch/sandbox/board/hostfile.c
>>> +++ b/arch/sandbox/board/hostfile.c
>>> @@ -166,10 +166,6 @@ static int hf_probe(struct device *dev)
>>> if (err)
>>> return err;
>>>
>>> - err = parse_partition_table(&priv->blk);
>>> - if (err)
>>> - dev_warn(dev, "No partition table found\n");
>>> -
>>> dev_info(dev, "registered as block device\n");
>>> } else {
>>> cdev->name = np->name;
>>> diff --git a/common/block.c b/common/block.c
>>> index c39269d3a692..98adcfdf3dab 100644
>>> --- a/common/block.c
>>> +++ b/common/block.c
>>> @@ -6,6 +6,7 @@
>>> */
>>> #include <common.h>
>>> #include <block.h>
>>> +#include <disks.h>
>>> #include <malloc.h>
>>> #include <linux/err.h>
>>> #include <linux/list.h>
>>> @@ -408,6 +409,11 @@ int blockdevice_register(struct block_device *blk)
>>>
>>> cdev_create_default_automount(&blk->cdev);
>>>
>>> + /* Lack of partition table is unusual, but not a failure */
>>> + ret = parse_partition_table(blk);
>>> + if (ret)
>>> + dev_warn(blk->dev, "No partition table found\n");
>>> +
>>> return 0;
>>> }
>>>
>>> diff --git a/drivers/ata/disk_ata_drive.c b/drivers/ata/disk_ata_drive.c
>>> index c1c736a0a88a..2d97710b827a 100644
>>> --- a/drivers/ata/disk_ata_drive.c
>>> +++ b/drivers/ata/disk_ata_drive.c
>>> @@ -254,11 +254,6 @@ static int ata_port_init(struct ata_port *port)
>>>
>>> dev_info(dev, "registered /dev/%s\n", port->blk.cdev.name);
>>>
>>> - /* create partitions on demand */
>>> - rc = parse_partition_table(&port->blk);
>>> - if (rc != 0)
>>> - dev_warn(dev, "No partition table found\n");
>>> -
>>> return 0;
>>>
>>> on_error:
>>> diff --git a/drivers/block/efi-block-io.c b/drivers/block/efi-block-io.c
>>> index eb4981e86298..7162106ab8ea 100644
>>> --- a/drivers/block/efi-block-io.c
>>> +++ b/drivers/block/efi-block-io.c
>>> @@ -12,7 +12,6 @@
>>> #include <fcntl.h>
>>> #include <efi.h>
>>> #include <block.h>
>>> -#include <disks.h>
>>> #include <efi/efi-payload.h>
>>> #include <efi/efi-device.h>
>>> #include <bootsource.h>
>>> @@ -184,16 +183,10 @@ static int efi_bio_probe(struct efi_device *efidev)
>>>
>>> priv->media_id = media->media_id;
>>>
>>> - ret = blockdevice_register(&priv->blk);
>>> - if (ret)
>>> - return ret;
>>> -
>>> if (efi_get_bootsource() == efidev)
>>> bootsource_set_raw_instance(instance);
>>>
>>> - parse_partition_table(&priv->blk);
>>> -
>>> - return 0;
>>> + return blockdevice_register(&priv->blk);
>>> }
>>>
>>> static struct efi_driver efi_bio_driver = {
>>> diff --git a/drivers/block/virtio_blk.c b/drivers/block/virtio_blk.c
>>> index 660f3a7b6b9b..11e52d9e6457 100644
>>> --- a/drivers/block/virtio_blk.c
>>> +++ b/drivers/block/virtio_blk.c
>>> @@ -105,13 +105,7 @@ static int virtio_blk_probe(struct virtio_device *vdev)
>>> priv->blk.num_blocks = cap;
>>> priv->blk.ops = &virtio_blk_ops;
>>>
>>> - ret = blockdevice_register(&priv->blk);
>>> - if (ret)
>>> - return ret;
>>> -
>>> - parse_partition_table(&priv->blk);
>>> -
>>> - return 0;
>>> + return blockdevice_register(&priv->blk);
>>> }
>>>
>>> static void virtio_blk_remove(struct virtio_device *vdev)
>>> diff --git a/drivers/mci/mci-core.c b/drivers/mci/mci-core.c
>>> index 6d0d6473770c..32edd5382386 100644
>>> --- a/drivers/mci/mci-core.c
>>> +++ b/drivers/mci/mci-core.c
>>> @@ -1900,12 +1900,6 @@ static int mci_register_partition(struct mci_part *part)
>>> return 0;
>>> }
>>>
>>> - rc = parse_partition_table(&part->blk);
>>> - if (rc != 0) {
>>> - /* Lack of partition table is unusual, but not a failure */
>>> - dev_warn(&mci->dev, "No partition table found\n");
>>> - }
>>> -
>>> if (np) {
>>> of_parse_partitions(&part->blk.cdev, np);
>>>
>>> diff --git a/drivers/nvme/host/core.c b/drivers/nvme/host/core.c
>>> index bf9176ce0922..79a5f9325ef8 100644
>>> --- a/drivers/nvme/host/core.c
>>> +++ b/drivers/nvme/host/core.c
>>> @@ -1,6 +1,5 @@
>>> // SPDX-License-Identifier: GPL-2.0-only
>>> #include <common.h>
>>> -#include <disks.h>
>>>
>>> #include "nvme.h"
>>>
>>> @@ -373,10 +372,6 @@ static void nvme_alloc_ns(struct nvme_ctrl *ctrl, unsigned nsid)
>>> goto out_free_id;
>>> }
>>>
>>> - ret = parse_partition_table(&ns->blk);
>>> - if (ret)
>>> - dev_warn(ctrl->dev, "No partition table found\n");
>>> -
>>> return;
>>> out_free_id:
>>> kfree(id);
>>> diff --git a/drivers/usb/storage/usb.c b/drivers/usb/storage/usb.c
>>> index 103ae293a3a4..dda713196071 100644
>>> --- a/drivers/usb/storage/usb.c
>>> +++ b/drivers/usb/storage/usb.c
>>> @@ -420,11 +420,6 @@ static int usb_stor_add_blkdev(struct us_data *us, unsigned char lun)
>>> goto BadDevice;
>>> }
>>>
>>> - /* create partitions on demand */
>>> - result = parse_partition_table(&pblk_dev->blk);
>>> - if (result != 0)
>>> - dev_warn(dev, "No partition table found\n");
>>> -
>>> list_add_tail(&pblk_dev->list, &us->blk_dev_list);
>>> dev_dbg(dev, "USB disk device successfully added\n");
>
--
Pengutronix e.K. | |
Steuerwalder Str. 21 | http://www.pengutronix.de/ |
31137 Hildesheim, Germany | Phone: +49-5121-206917-0 |
Amtsgericht Hildesheim, HRA 2686 | Fax: +49-5121-206917-5555 |
^ permalink raw reply [flat|nested] 62+ messages in thread
* [PATCH 12/18] common: partitions: record whether disk is GPT or MBR partitioned
2023-05-31 14:59 [PATCH 00/18] state: allow lookup of barebox state partition by Type GUID Ahmad Fatoum
` (10 preceding siblings ...)
2023-05-31 14:59 ` [PATCH 11/18] block: parse partition table on block device registration Ahmad Fatoum
@ 2023-05-31 14:59 ` Ahmad Fatoum
2023-05-31 17:33 ` Marco Felsch
2023-05-31 14:59 ` [PATCH 13/18] block: add cdev_is_block_(device,partition,disk) helpers Ahmad Fatoum
` (5 subsequent siblings)
17 siblings, 1 reply; 62+ messages in thread
From: Ahmad Fatoum @ 2023-05-31 14:59 UTC (permalink / raw)
To: barebox; +Cc: Ahmad Fatoum
Currently, the only way to differentiate between a GPT disk and a MBR
one is to check whether the cdev's device has a guid (=> GPT) or a
nt_signature (=> MBR) device parameter. We already have a flag parameter
though, so let's record this info there for easy retrieval.
We intentionally don't use the struct cdev::filetype member, because
we don't want to change behavior of file_detect_type().
Signed-off-by: Ahmad Fatoum <a.fatoum@pengutronix.de>
---
common/partitions/dos.c | 2 ++
common/partitions/efi.c | 2 ++
fs/fs.c | 4 ++++
include/driver.h | 20 +++++++++++++++++---
4 files changed, 25 insertions(+), 3 deletions(-)
diff --git a/common/partitions/dos.c b/common/partitions/dos.c
index ad60c0b27b46..7472824b00b9 100644
--- a/common/partitions/dos.c
+++ b/common/partitions/dos.c
@@ -185,6 +185,8 @@ static void dos_partition(void *buf, struct block_device *blk,
if (signature)
sprintf(blk->cdev.diskuuid, "%08x", signature);
+ blk->cdev.flags |= DEVFS_IS_MBR_PARTITIONED;
+
table = (struct partition_entry *)&buffer[446];
for (i = 0; i < 4; i++) {
diff --git a/common/partitions/efi.c b/common/partitions/efi.c
index 780a8695e8a8..df63b82afe24 100644
--- a/common/partitions/efi.c
+++ b/common/partitions/efi.c
@@ -449,6 +449,8 @@ static void efi_partition(void *buf, struct block_device *blk,
snprintf(blk->cdev.diskuuid, sizeof(blk->cdev.diskuuid), "%pUl", &gpt->disk_guid);
dev_add_param_string_fixed(blk->dev, "guid", blk->cdev.diskuuid);
+ blk->cdev.flags |= DEVFS_IS_GPT_PARTITIONED;
+
nb_part = le32_to_cpu(gpt->num_partition_entries);
if (nb_part > MAX_PARTITION) {
diff --git a/fs/fs.c b/fs/fs.c
index 9d8aab268ca4..2d2d327c5fbc 100644
--- a/fs/fs.c
+++ b/fs/fs.c
@@ -94,6 +94,10 @@ void cdev_print(const struct cdev *cdev)
printf(" table-partition");
if (cdev->flags & DEVFS_IS_MCI_MAIN_PART_DEV)
printf(" mci-main-partition");
+ if (cdev->flags & DEVFS_IS_GPT_PARTITIONED)
+ printf(" gpt-partitioned");
+ if (cdev->flags & DEVFS_IS_MBR_PARTITIONED)
+ printf(" mbr-partitioned");
if (cdev->mtd)
printf(" mtd");
printf(" )");
diff --git a/include/driver.h b/include/driver.h
index 118d2adb6750..5f2eae65466f 100644
--- a/include/driver.h
+++ b/include/driver.h
@@ -584,9 +584,23 @@ extern struct list_head cdev_list;
#define DEVFS_PARTITION_FIXED (1U << 0)
#define DEVFS_PARTITION_READONLY (1U << 1)
#define DEVFS_IS_CHARACTER_DEV (1U << 3)
-#define DEVFS_IS_MCI_MAIN_PART_DEV (1U << 4)
-#define DEVFS_PARTITION_FROM_OF (1U << 5)
-#define DEVFS_PARTITION_FROM_TABLE (1U << 6)
+#define DEVFS_PARTITION_FROM_OF (1U << 4)
+#define DEVFS_PARTITION_FROM_TABLE (1U << 5)
+#define DEVFS_IS_GPT_PARTITIONED (1U << 6)
+#define DEVFS_IS_MBR_PARTITIONED (1U << 7)
+#define DEVFS_IS_MCI_MAIN_PART_DEV (1U << 8)
+
+static inline bool cdev_is_gpt_partitioned(const struct cdev *master)
+{
+ return master && (master->flags & DEVFS_IS_GPT_PARTITIONED)
+ == DEVFS_IS_GPT_PARTITIONED;
+}
+
+static inline bool cdev_is_mbr_partitioned(const struct cdev *master)
+{
+ return master && (master->flags & DEVFS_IS_MBR_PARTITIONED)
+ == DEVFS_IS_MBR_PARTITIONED;
+}
struct cdev *devfs_add_partition(const char *devname, loff_t offset,
loff_t size, unsigned int flags, const char *name);
--
2.39.2
^ permalink raw reply [flat|nested] 62+ messages in thread* Re: [PATCH 12/18] common: partitions: record whether disk is GPT or MBR partitioned
2023-05-31 14:59 ` [PATCH 12/18] common: partitions: record whether disk is GPT or MBR partitioned Ahmad Fatoum
@ 2023-05-31 17:33 ` Marco Felsch
2023-06-01 5:08 ` Ahmad Fatoum
0 siblings, 1 reply; 62+ messages in thread
From: Marco Felsch @ 2023-05-31 17:33 UTC (permalink / raw)
To: Ahmad Fatoum; +Cc: barebox
On 23-05-31, Ahmad Fatoum wrote:
> Currently, the only way to differentiate between a GPT disk and a MBR
> one is to check whether the cdev's device has a guid (=> GPT) or a
> nt_signature (=> MBR) device parameter. We already have a flag parameter
> though, so let's record this info there for easy retrieval.
>
> We intentionally don't use the struct cdev::filetype member, because
> we don't want to change behavior of file_detect_type().
>
> Signed-off-by: Ahmad Fatoum <a.fatoum@pengutronix.de>
> ---
> common/partitions/dos.c | 2 ++
> common/partitions/efi.c | 2 ++
> fs/fs.c | 4 ++++
> include/driver.h | 20 +++++++++++++++++---
> 4 files changed, 25 insertions(+), 3 deletions(-)
>
> diff --git a/common/partitions/dos.c b/common/partitions/dos.c
> index ad60c0b27b46..7472824b00b9 100644
> --- a/common/partitions/dos.c
> +++ b/common/partitions/dos.c
> @@ -185,6 +185,8 @@ static void dos_partition(void *buf, struct block_device *blk,
> if (signature)
> sprintf(blk->cdev.diskuuid, "%08x", signature);
>
> + blk->cdev.flags |= DEVFS_IS_MBR_PARTITIONED;
> +
> table = (struct partition_entry *)&buffer[446];
>
> for (i = 0; i < 4; i++) {
> diff --git a/common/partitions/efi.c b/common/partitions/efi.c
> index 780a8695e8a8..df63b82afe24 100644
> --- a/common/partitions/efi.c
> +++ b/common/partitions/efi.c
> @@ -449,6 +449,8 @@ static void efi_partition(void *buf, struct block_device *blk,
> snprintf(blk->cdev.diskuuid, sizeof(blk->cdev.diskuuid), "%pUl", &gpt->disk_guid);
> dev_add_param_string_fixed(blk->dev, "guid", blk->cdev.diskuuid);
>
> + blk->cdev.flags |= DEVFS_IS_GPT_PARTITIONED;
> +
> nb_part = le32_to_cpu(gpt->num_partition_entries);
>
> if (nb_part > MAX_PARTITION) {
> diff --git a/fs/fs.c b/fs/fs.c
> index 9d8aab268ca4..2d2d327c5fbc 100644
> --- a/fs/fs.c
> +++ b/fs/fs.c
> @@ -94,6 +94,10 @@ void cdev_print(const struct cdev *cdev)
> printf(" table-partition");
> if (cdev->flags & DEVFS_IS_MCI_MAIN_PART_DEV)
> printf(" mci-main-partition");
> + if (cdev->flags & DEVFS_IS_GPT_PARTITIONED)
> + printf(" gpt-partitioned");
> + if (cdev->flags & DEVFS_IS_MBR_PARTITIONED)
> + printf(" mbr-partitioned");
> if (cdev->mtd)
> printf(" mtd");
> printf(" )");
> diff --git a/include/driver.h b/include/driver.h
> index 118d2adb6750..5f2eae65466f 100644
> --- a/include/driver.h
> +++ b/include/driver.h
> @@ -584,9 +584,23 @@ extern struct list_head cdev_list;
> #define DEVFS_PARTITION_FIXED (1U << 0)
> #define DEVFS_PARTITION_READONLY (1U << 1)
> #define DEVFS_IS_CHARACTER_DEV (1U << 3)
> -#define DEVFS_IS_MCI_MAIN_PART_DEV (1U << 4)
> -#define DEVFS_PARTITION_FROM_OF (1U << 5)
> -#define DEVFS_PARTITION_FROM_TABLE (1U << 6)
> +#define DEVFS_PARTITION_FROM_OF (1U << 4)
> +#define DEVFS_PARTITION_FROM_TABLE (1U << 5)
> +#define DEVFS_IS_GPT_PARTITIONED (1U << 6)
> +#define DEVFS_IS_MBR_PARTITIONED (1U << 7)
> +#define DEVFS_IS_MCI_MAIN_PART_DEV (1U << 8)
Why do you reorder the bits here again?
> +
> +static inline bool cdev_is_gpt_partitioned(const struct cdev *master)
> +{
> + return master && (master->flags & DEVFS_IS_GPT_PARTITIONED)
> + == DEVFS_IS_GPT_PARTITIONED;
Why not just: 'return !!(master->flags & DEVFS_IS_GPT_PARTITIONED)' ?
Regards,
Marco
> +}
> +
> +static inline bool cdev_is_mbr_partitioned(const struct cdev *master)
> +{
> + return master && (master->flags & DEVFS_IS_MBR_PARTITIONED)
> + == DEVFS_IS_MBR_PARTITIONED;
> +}
>
> struct cdev *devfs_add_partition(const char *devname, loff_t offset,
> loff_t size, unsigned int flags, const char *name);
> --
> 2.39.2
>
>
>
^ permalink raw reply [flat|nested] 62+ messages in thread* Re: [PATCH 12/18] common: partitions: record whether disk is GPT or MBR partitioned
2023-05-31 17:33 ` Marco Felsch
@ 2023-06-01 5:08 ` Ahmad Fatoum
2023-06-01 5:58 ` Ahmad Fatoum
2023-06-01 8:19 ` Marco Felsch
0 siblings, 2 replies; 62+ messages in thread
From: Ahmad Fatoum @ 2023-06-01 5:08 UTC (permalink / raw)
To: Marco Felsch; +Cc: barebox
On 31.05.23 19:33, Marco Felsch wrote:
> On 23-05-31, Ahmad Fatoum wrote:
>> Currently, the only way to differentiate between a GPT disk and a MBR
>> one is to check whether the cdev's device has a guid (=> GPT) or a
>> nt_signature (=> MBR) device parameter. We already have a flag parameter
>> though, so let's record this info there for easy retrieval.
>>
>> We intentionally don't use the struct cdev::filetype member, because
>> we don't want to change behavior of file_detect_type().
>>
>> Signed-off-by: Ahmad Fatoum <a.fatoum@pengutronix.de>
>> ---
>> common/partitions/dos.c | 2 ++
>> common/partitions/efi.c | 2 ++
>> fs/fs.c | 4 ++++
>> include/driver.h | 20 +++++++++++++++++---
>> 4 files changed, 25 insertions(+), 3 deletions(-)
>>
>> diff --git a/common/partitions/dos.c b/common/partitions/dos.c
>> index ad60c0b27b46..7472824b00b9 100644
>> --- a/common/partitions/dos.c
>> +++ b/common/partitions/dos.c
>> @@ -185,6 +185,8 @@ static void dos_partition(void *buf, struct block_device *blk,
>> if (signature)
>> sprintf(blk->cdev.diskuuid, "%08x", signature);
>>
>> + blk->cdev.flags |= DEVFS_IS_MBR_PARTITIONED;
>> +
>> table = (struct partition_entry *)&buffer[446];
>>
>> for (i = 0; i < 4; i++) {
>> diff --git a/common/partitions/efi.c b/common/partitions/efi.c
>> index 780a8695e8a8..df63b82afe24 100644
>> --- a/common/partitions/efi.c
>> +++ b/common/partitions/efi.c
>> @@ -449,6 +449,8 @@ static void efi_partition(void *buf, struct block_device *blk,
>> snprintf(blk->cdev.diskuuid, sizeof(blk->cdev.diskuuid), "%pUl", &gpt->disk_guid);
>> dev_add_param_string_fixed(blk->dev, "guid", blk->cdev.diskuuid);
>>
>> + blk->cdev.flags |= DEVFS_IS_GPT_PARTITIONED;
>> +
>> nb_part = le32_to_cpu(gpt->num_partition_entries);
>>
>> if (nb_part > MAX_PARTITION) {
>> diff --git a/fs/fs.c b/fs/fs.c
>> index 9d8aab268ca4..2d2d327c5fbc 100644
>> --- a/fs/fs.c
>> +++ b/fs/fs.c
>> @@ -94,6 +94,10 @@ void cdev_print(const struct cdev *cdev)
>> printf(" table-partition");
>> if (cdev->flags & DEVFS_IS_MCI_MAIN_PART_DEV)
>> printf(" mci-main-partition");
>> + if (cdev->flags & DEVFS_IS_GPT_PARTITIONED)
>> + printf(" gpt-partitioned");
>> + if (cdev->flags & DEVFS_IS_MBR_PARTITIONED)
>> + printf(" mbr-partitioned");
>> if (cdev->mtd)
>> printf(" mtd");
>> printf(" )");
>> diff --git a/include/driver.h b/include/driver.h
>> index 118d2adb6750..5f2eae65466f 100644
>> --- a/include/driver.h
>> +++ b/include/driver.h
>> @@ -584,9 +584,23 @@ extern struct list_head cdev_list;
>> #define DEVFS_PARTITION_FIXED (1U << 0)
>> #define DEVFS_PARTITION_READONLY (1U << 1)
>> #define DEVFS_IS_CHARACTER_DEV (1U << 3)
>> -#define DEVFS_IS_MCI_MAIN_PART_DEV (1U << 4)
>> -#define DEVFS_PARTITION_FROM_OF (1U << 5)
>> -#define DEVFS_PARTITION_FROM_TABLE (1U << 6)
>> +#define DEVFS_PARTITION_FROM_OF (1U << 4)
>> +#define DEVFS_PARTITION_FROM_TABLE (1U << 5)
>> +#define DEVFS_IS_GPT_PARTITIONED (1U << 6)
>> +#define DEVFS_IS_MBR_PARTITIONED (1U << 7)
>> +#define DEVFS_IS_MCI_MAIN_PART_DEV (1U << 8)
>
> Why do you reorder the bits here again?
I wanted the partition bits to be together.
>> +
>> +static inline bool cdev_is_gpt_partitioned(const struct cdev *master)
>> +{
>> + return master && (master->flags & DEVFS_IS_GPT_PARTITIONED)
>> + == DEVFS_IS_GPT_PARTITIONED;
>
> Why not just: 'return !!(master->flags & DEVFS_IS_GPT_PARTITIONED)' ?
Left over from when this was more than one bit. I will change to:
return master && (master->flags & DEVFS_IS_GPT_PARTITIONED)
I want to keep the NULL check, as we only set this for the master device.
Cheers,
Ahmad
>
> Regards,
> Marco
>
>> +}
>> +
>> +static inline bool cdev_is_mbr_partitioned(const struct cdev *master)
>> +{
>> + return master && (master->flags & DEVFS_IS_MBR_PARTITIONED)
>> + == DEVFS_IS_MBR_PARTITIONED;
>> +}
>>
>> struct cdev *devfs_add_partition(const char *devname, loff_t offset,
>> loff_t size, unsigned int flags, const char *name);
>> --
>> 2.39.2
>>
>>
>>
>
--
Pengutronix e.K. | |
Steuerwalder Str. 21 | http://www.pengutronix.de/ |
31137 Hildesheim, Germany | Phone: +49-5121-206917-0 |
Amtsgericht Hildesheim, HRA 2686 | Fax: +49-5121-206917-5555 |
^ permalink raw reply [flat|nested] 62+ messages in thread* Re: [PATCH 12/18] common: partitions: record whether disk is GPT or MBR partitioned
2023-06-01 5:08 ` Ahmad Fatoum
@ 2023-06-01 5:58 ` Ahmad Fatoum
2023-06-01 8:19 ` Marco Felsch
1 sibling, 0 replies; 62+ messages in thread
From: Ahmad Fatoum @ 2023-06-01 5:58 UTC (permalink / raw)
To: Marco Felsch; +Cc: barebox
On 01.06.23 07:08, Ahmad Fatoum wrote:
> On 31.05.23 19:33, Marco Felsch wrote:
>> On 23-05-31, Ahmad Fatoum wrote:
>>> Currently, the only way to differentiate between a GPT disk and a MBR
>>> one is to check whether the cdev's device has a guid (=> GPT) or a
>>> nt_signature (=> MBR) device parameter. We already have a flag parameter
>>> though, so let's record this info there for easy retrieval.
>>>
>>> We intentionally don't use the struct cdev::filetype member, because
>>> we don't want to change behavior of file_detect_type().
>>>
>>> Signed-off-by: Ahmad Fatoum <a.fatoum@pengutronix.de>
>>> ---
>>> common/partitions/dos.c | 2 ++
>>> common/partitions/efi.c | 2 ++
>>> fs/fs.c | 4 ++++
>>> include/driver.h | 20 +++++++++++++++++---
>>> 4 files changed, 25 insertions(+), 3 deletions(-)
>>>
>>> diff --git a/common/partitions/dos.c b/common/partitions/dos.c
>>> index ad60c0b27b46..7472824b00b9 100644
>>> --- a/common/partitions/dos.c
>>> +++ b/common/partitions/dos.c
>>> @@ -185,6 +185,8 @@ static void dos_partition(void *buf, struct block_device *blk,
>>> if (signature)
>>> sprintf(blk->cdev.diskuuid, "%08x", signature);
>>>
>>> + blk->cdev.flags |= DEVFS_IS_MBR_PARTITIONED;
>>> +
>>> table = (struct partition_entry *)&buffer[446];
>>>
>>> for (i = 0; i < 4; i++) {
>>> diff --git a/common/partitions/efi.c b/common/partitions/efi.c
>>> index 780a8695e8a8..df63b82afe24 100644
>>> --- a/common/partitions/efi.c
>>> +++ b/common/partitions/efi.c
>>> @@ -449,6 +449,8 @@ static void efi_partition(void *buf, struct block_device *blk,
>>> snprintf(blk->cdev.diskuuid, sizeof(blk->cdev.diskuuid), "%pUl", &gpt->disk_guid);
>>> dev_add_param_string_fixed(blk->dev, "guid", blk->cdev.diskuuid);
>>>
>>> + blk->cdev.flags |= DEVFS_IS_GPT_PARTITIONED;
>>> +
>>> nb_part = le32_to_cpu(gpt->num_partition_entries);
>>>
>>> if (nb_part > MAX_PARTITION) {
>>> diff --git a/fs/fs.c b/fs/fs.c
>>> index 9d8aab268ca4..2d2d327c5fbc 100644
>>> --- a/fs/fs.c
>>> +++ b/fs/fs.c
>>> @@ -94,6 +94,10 @@ void cdev_print(const struct cdev *cdev)
>>> printf(" table-partition");
>>> if (cdev->flags & DEVFS_IS_MCI_MAIN_PART_DEV)
>>> printf(" mci-main-partition");
>>> + if (cdev->flags & DEVFS_IS_GPT_PARTITIONED)
>>> + printf(" gpt-partitioned");
>>> + if (cdev->flags & DEVFS_IS_MBR_PARTITIONED)
>>> + printf(" mbr-partitioned");
>>> if (cdev->mtd)
>>> printf(" mtd");
>>> printf(" )");
>>> diff --git a/include/driver.h b/include/driver.h
>>> index 118d2adb6750..5f2eae65466f 100644
>>> --- a/include/driver.h
>>> +++ b/include/driver.h
>>> @@ -584,9 +584,23 @@ extern struct list_head cdev_list;
>>> #define DEVFS_PARTITION_FIXED (1U << 0)
>>> #define DEVFS_PARTITION_READONLY (1U << 1)
>>> #define DEVFS_IS_CHARACTER_DEV (1U << 3)
>>> -#define DEVFS_IS_MCI_MAIN_PART_DEV (1U << 4)
>>> -#define DEVFS_PARTITION_FROM_OF (1U << 5)
>>> -#define DEVFS_PARTITION_FROM_TABLE (1U << 6)
>>> +#define DEVFS_PARTITION_FROM_OF (1U << 4)
>>> +#define DEVFS_PARTITION_FROM_TABLE (1U << 5)
>>> +#define DEVFS_IS_GPT_PARTITIONED (1U << 6)
>>> +#define DEVFS_IS_MBR_PARTITIONED (1U << 7)
>>> +#define DEVFS_IS_MCI_MAIN_PART_DEV (1U << 8)
>>
>> Why do you reorder the bits here again?
>
> I wanted the partition bits to be together.
I see what you mean now. I'll leave MCI_MAIN_PART as bit 4
and add everything after that. Thanks.
>
>>> +
>>> +static inline bool cdev_is_gpt_partitioned(const struct cdev *master)
>>> +{
>>> + return master && (master->flags & DEVFS_IS_GPT_PARTITIONED)
>>> + == DEVFS_IS_GPT_PARTITIONED;
>>
>> Why not just: 'return !!(master->flags & DEVFS_IS_GPT_PARTITIONED)' ?
>
> Left over from when this was more than one bit. I will change to:
>
> return master && (master->flags & DEVFS_IS_GPT_PARTITIONED)
>
> I want to keep the NULL check, as we only set this for the master device.
>
> Cheers,
> Ahmad
>
>
>>
>> Regards,
>> Marco
>>
>>> +}
>>> +
>>> +static inline bool cdev_is_mbr_partitioned(const struct cdev *master)
>>> +{
>>> + return master && (master->flags & DEVFS_IS_MBR_PARTITIONED)
>>> + == DEVFS_IS_MBR_PARTITIONED;
>>> +}
>>>
>>> struct cdev *devfs_add_partition(const char *devname, loff_t offset,
>>> loff_t size, unsigned int flags, const char *name);
>>> --
>>> 2.39.2
>>>
>>>
>>>
>>
>
--
Pengutronix e.K. | |
Steuerwalder Str. 21 | http://www.pengutronix.de/ |
31137 Hildesheim, Germany | Phone: +49-5121-206917-0 |
Amtsgericht Hildesheim, HRA 2686 | Fax: +49-5121-206917-5555 |
^ permalink raw reply [flat|nested] 62+ messages in thread* Re: [PATCH 12/18] common: partitions: record whether disk is GPT or MBR partitioned
2023-06-01 5:08 ` Ahmad Fatoum
2023-06-01 5:58 ` Ahmad Fatoum
@ 2023-06-01 8:19 ` Marco Felsch
2023-06-01 10:40 ` Ahmad Fatoum
1 sibling, 1 reply; 62+ messages in thread
From: Marco Felsch @ 2023-06-01 8:19 UTC (permalink / raw)
To: Ahmad Fatoum; +Cc: barebox
On 23-06-01, Ahmad Fatoum wrote:
> On 31.05.23 19:33, Marco Felsch wrote:
> > On 23-05-31, Ahmad Fatoum wrote:
> >> Currently, the only way to differentiate between a GPT disk and a MBR
> >> one is to check whether the cdev's device has a guid (=> GPT) or a
> >> nt_signature (=> MBR) device parameter. We already have a flag parameter
> >> though, so let's record this info there for easy retrieval.
> >>
> >> We intentionally don't use the struct cdev::filetype member, because
> >> we don't want to change behavior of file_detect_type().
> >>
> >> Signed-off-by: Ahmad Fatoum <a.fatoum@pengutronix.de>
> >> ---
> >> common/partitions/dos.c | 2 ++
> >> common/partitions/efi.c | 2 ++
> >> fs/fs.c | 4 ++++
> >> include/driver.h | 20 +++++++++++++++++---
> >> 4 files changed, 25 insertions(+), 3 deletions(-)
> >>
> >> diff --git a/common/partitions/dos.c b/common/partitions/dos.c
> >> index ad60c0b27b46..7472824b00b9 100644
> >> --- a/common/partitions/dos.c
> >> +++ b/common/partitions/dos.c
> >> @@ -185,6 +185,8 @@ static void dos_partition(void *buf, struct block_device *blk,
> >> if (signature)
> >> sprintf(blk->cdev.diskuuid, "%08x", signature);
> >>
> >> + blk->cdev.flags |= DEVFS_IS_MBR_PARTITIONED;
> >> +
> >> table = (struct partition_entry *)&buffer[446];
> >>
> >> for (i = 0; i < 4; i++) {
> >> diff --git a/common/partitions/efi.c b/common/partitions/efi.c
> >> index 780a8695e8a8..df63b82afe24 100644
> >> --- a/common/partitions/efi.c
> >> +++ b/common/partitions/efi.c
> >> @@ -449,6 +449,8 @@ static void efi_partition(void *buf, struct block_device *blk,
> >> snprintf(blk->cdev.diskuuid, sizeof(blk->cdev.diskuuid), "%pUl", &gpt->disk_guid);
> >> dev_add_param_string_fixed(blk->dev, "guid", blk->cdev.diskuuid);
> >>
> >> + blk->cdev.flags |= DEVFS_IS_GPT_PARTITIONED;
> >> +
> >> nb_part = le32_to_cpu(gpt->num_partition_entries);
> >>
> >> if (nb_part > MAX_PARTITION) {
> >> diff --git a/fs/fs.c b/fs/fs.c
> >> index 9d8aab268ca4..2d2d327c5fbc 100644
> >> --- a/fs/fs.c
> >> +++ b/fs/fs.c
> >> @@ -94,6 +94,10 @@ void cdev_print(const struct cdev *cdev)
> >> printf(" table-partition");
> >> if (cdev->flags & DEVFS_IS_MCI_MAIN_PART_DEV)
> >> printf(" mci-main-partition");
> >> + if (cdev->flags & DEVFS_IS_GPT_PARTITIONED)
> >> + printf(" gpt-partitioned");
> >> + if (cdev->flags & DEVFS_IS_MBR_PARTITIONED)
> >> + printf(" mbr-partitioned");
> >> if (cdev->mtd)
> >> printf(" mtd");
> >> printf(" )");
> >> diff --git a/include/driver.h b/include/driver.h
> >> index 118d2adb6750..5f2eae65466f 100644
> >> --- a/include/driver.h
> >> +++ b/include/driver.h
> >> @@ -584,9 +584,23 @@ extern struct list_head cdev_list;
> >> #define DEVFS_PARTITION_FIXED (1U << 0)
> >> #define DEVFS_PARTITION_READONLY (1U << 1)
> >> #define DEVFS_IS_CHARACTER_DEV (1U << 3)
> >> -#define DEVFS_IS_MCI_MAIN_PART_DEV (1U << 4)
> >> -#define DEVFS_PARTITION_FROM_OF (1U << 5)
> >> -#define DEVFS_PARTITION_FROM_TABLE (1U << 6)
> >> +#define DEVFS_PARTITION_FROM_OF (1U << 4)
> >> +#define DEVFS_PARTITION_FROM_TABLE (1U << 5)
> >> +#define DEVFS_IS_GPT_PARTITIONED (1U << 6)
> >> +#define DEVFS_IS_MBR_PARTITIONED (1U << 7)
> >> +#define DEVFS_IS_MCI_MAIN_PART_DEV (1U << 8)
> >
> > Why do you reorder the bits here again?
>
> I wanted the partition bits to be together.
>
> >> +
> >> +static inline bool cdev_is_gpt_partitioned(const struct cdev *master)
> >> +{
> >> + return master && (master->flags & DEVFS_IS_GPT_PARTITIONED)
> >> + == DEVFS_IS_GPT_PARTITIONED;
> >
> > Why not just: 'return !!(master->flags & DEVFS_IS_GPT_PARTITIONED)' ?
>
> Left over from when this was more than one bit. I will change to:
>
> return master && (master->flags & DEVFS_IS_GPT_PARTITIONED)
>
> I want to keep the NULL check, as we only set this for the master device.
By 'as we only set this for the master device' you mean the flags right?
We don't expect the 'struct cdev*' parameter to be NULL.
Regards,
Marco
>
> Cheers,
> Ahmad
>
>
> >
> > Regards,
> > Marco
> >
> >> +}
> >> +
> >> +static inline bool cdev_is_mbr_partitioned(const struct cdev *master)
> >> +{
> >> + return master && (master->flags & DEVFS_IS_MBR_PARTITIONED)
> >> + == DEVFS_IS_MBR_PARTITIONED;
> >> +}
> >>
> >> struct cdev *devfs_add_partition(const char *devname, loff_t offset,
> >> loff_t size, unsigned int flags, const char *name);
> >> --
> >> 2.39.2
> >>
> >>
> >>
> >
>
> --
> Pengutronix e.K. | |
> Steuerwalder Str. 21 | http://www.pengutronix.de/ |
> 31137 Hildesheim, Germany | Phone: +49-5121-206917-0 |
> Amtsgericht Hildesheim, HRA 2686 | Fax: +49-5121-206917-5555 |
>
>
^ permalink raw reply [flat|nested] 62+ messages in thread* Re: [PATCH 12/18] common: partitions: record whether disk is GPT or MBR partitioned
2023-06-01 8:19 ` Marco Felsch
@ 2023-06-01 10:40 ` Ahmad Fatoum
0 siblings, 0 replies; 62+ messages in thread
From: Ahmad Fatoum @ 2023-06-01 10:40 UTC (permalink / raw)
To: Marco Felsch; +Cc: barebox
On 01.06.23 10:19, Marco Felsch wrote:
> On 23-06-01, Ahmad Fatoum wrote:
>> On 31.05.23 19:33, Marco Felsch wrote:
>>> On 23-05-31, Ahmad Fatoum wrote:
>>>> Currently, the only way to differentiate between a GPT disk and a MBR
>>>> one is to check whether the cdev's device has a guid (=> GPT) or a
>>>> nt_signature (=> MBR) device parameter. We already have a flag parameter
>>>> though, so let's record this info there for easy retrieval.
>>>>
>>>> We intentionally don't use the struct cdev::filetype member, because
>>>> we don't want to change behavior of file_detect_type().
>>>>
>>>> Signed-off-by: Ahmad Fatoum <a.fatoum@pengutronix.de>
>>>> ---
>>>> common/partitions/dos.c | 2 ++
>>>> common/partitions/efi.c | 2 ++
>>>> fs/fs.c | 4 ++++
>>>> include/driver.h | 20 +++++++++++++++++---
>>>> 4 files changed, 25 insertions(+), 3 deletions(-)
>>>>
>>>> diff --git a/common/partitions/dos.c b/common/partitions/dos.c
>>>> index ad60c0b27b46..7472824b00b9 100644
>>>> --- a/common/partitions/dos.c
>>>> +++ b/common/partitions/dos.c
>>>> @@ -185,6 +185,8 @@ static void dos_partition(void *buf, struct block_device *blk,
>>>> if (signature)
>>>> sprintf(blk->cdev.diskuuid, "%08x", signature);
>>>>
>>>> + blk->cdev.flags |= DEVFS_IS_MBR_PARTITIONED;
>>>> +
>>>> table = (struct partition_entry *)&buffer[446];
>>>>
>>>> for (i = 0; i < 4; i++) {
>>>> diff --git a/common/partitions/efi.c b/common/partitions/efi.c
>>>> index 780a8695e8a8..df63b82afe24 100644
>>>> --- a/common/partitions/efi.c
>>>> +++ b/common/partitions/efi.c
>>>> @@ -449,6 +449,8 @@ static void efi_partition(void *buf, struct block_device *blk,
>>>> snprintf(blk->cdev.diskuuid, sizeof(blk->cdev.diskuuid), "%pUl", &gpt->disk_guid);
>>>> dev_add_param_string_fixed(blk->dev, "guid", blk->cdev.diskuuid);
>>>>
>>>> + blk->cdev.flags |= DEVFS_IS_GPT_PARTITIONED;
>>>> +
>>>> nb_part = le32_to_cpu(gpt->num_partition_entries);
>>>>
>>>> if (nb_part > MAX_PARTITION) {
>>>> diff --git a/fs/fs.c b/fs/fs.c
>>>> index 9d8aab268ca4..2d2d327c5fbc 100644
>>>> --- a/fs/fs.c
>>>> +++ b/fs/fs.c
>>>> @@ -94,6 +94,10 @@ void cdev_print(const struct cdev *cdev)
>>>> printf(" table-partition");
>>>> if (cdev->flags & DEVFS_IS_MCI_MAIN_PART_DEV)
>>>> printf(" mci-main-partition");
>>>> + if (cdev->flags & DEVFS_IS_GPT_PARTITIONED)
>>>> + printf(" gpt-partitioned");
>>>> + if (cdev->flags & DEVFS_IS_MBR_PARTITIONED)
>>>> + printf(" mbr-partitioned");
>>>> if (cdev->mtd)
>>>> printf(" mtd");
>>>> printf(" )");
>>>> diff --git a/include/driver.h b/include/driver.h
>>>> index 118d2adb6750..5f2eae65466f 100644
>>>> --- a/include/driver.h
>>>> +++ b/include/driver.h
>>>> @@ -584,9 +584,23 @@ extern struct list_head cdev_list;
>>>> #define DEVFS_PARTITION_FIXED (1U << 0)
>>>> #define DEVFS_PARTITION_READONLY (1U << 1)
>>>> #define DEVFS_IS_CHARACTER_DEV (1U << 3)
>>>> -#define DEVFS_IS_MCI_MAIN_PART_DEV (1U << 4)
>>>> -#define DEVFS_PARTITION_FROM_OF (1U << 5)
>>>> -#define DEVFS_PARTITION_FROM_TABLE (1U << 6)
>>>> +#define DEVFS_PARTITION_FROM_OF (1U << 4)
>>>> +#define DEVFS_PARTITION_FROM_TABLE (1U << 5)
>>>> +#define DEVFS_IS_GPT_PARTITIONED (1U << 6)
>>>> +#define DEVFS_IS_MBR_PARTITIONED (1U << 7)
>>>> +#define DEVFS_IS_MCI_MAIN_PART_DEV (1U << 8)
>>>
>>> Why do you reorder the bits here again?
>>
>> I wanted the partition bits to be together.
>>
>>>> +
>>>> +static inline bool cdev_is_gpt_partitioned(const struct cdev *master)
>>>> +{
>>>> + return master && (master->flags & DEVFS_IS_GPT_PARTITIONED)
>>>> + == DEVFS_IS_GPT_PARTITIONED;
>>>
>>> Why not just: 'return !!(master->flags & DEVFS_IS_GPT_PARTITIONED)' ?
>>
>> Left over from when this was more than one bit. I will change to:
>>
>> return master && (master->flags & DEVFS_IS_GPT_PARTITIONED)
>>
>> I want to keep the NULL check, as we only set this for the master device.
>
> By 'as we only set this for the master device' you mean the flags right?
> We don't expect the 'struct cdev*' parameter to be NULL.
stat has this use:
if (cdev_is_mbr_partitioned(cdev->master))
nbytes += printf("DOS parttype: 0x%02x\t", cdev->dos_partition_type);
The same line is called for all cdevs and disks are handled gracefully
by cdev->master being NULL and cdev_is_mbr_partitioned having a NULL
check.
>
> Regards,
> Marco
>
>>
>> Cheers,
>> Ahmad
>>
>>
>>>
>>> Regards,
>>> Marco
>>>
>>>> +}
>>>> +
>>>> +static inline bool cdev_is_mbr_partitioned(const struct cdev *master)
>>>> +{
>>>> + return master && (master->flags & DEVFS_IS_MBR_PARTITIONED)
>>>> + == DEVFS_IS_MBR_PARTITIONED;
>>>> +}
>>>>
>>>> struct cdev *devfs_add_partition(const char *devname, loff_t offset,
>>>> loff_t size, unsigned int flags, const char *name);
>>>> --
>>>> 2.39.2
>>>>
>>>>
>>>>
>>>
>>
>> --
>> Pengutronix e.K. | |
>> Steuerwalder Str. 21 | http://www.pengutronix.de/ |
>> 31137 Hildesheim, Germany | Phone: +49-5121-206917-0 |
>> Amtsgericht Hildesheim, HRA 2686 | Fax: +49-5121-206917-5555 |
>>
>>
>
--
Pengutronix e.K. | |
Steuerwalder Str. 21 | http://www.pengutronix.de/ |
31137 Hildesheim, Germany | Phone: +49-5121-206917-0 |
Amtsgericht Hildesheim, HRA 2686 | Fax: +49-5121-206917-5555 |
^ permalink raw reply [flat|nested] 62+ messages in thread
* [PATCH 13/18] block: add cdev_is_block_(device,partition,disk) helpers
2023-05-31 14:59 [PATCH 00/18] state: allow lookup of barebox state partition by Type GUID Ahmad Fatoum
` (11 preceding siblings ...)
2023-05-31 14:59 ` [PATCH 12/18] common: partitions: record whether disk is GPT or MBR partitioned Ahmad Fatoum
@ 2023-05-31 14:59 ` Ahmad Fatoum
2023-05-31 17:35 ` Marco Felsch
2023-05-31 14:59 ` [PATCH 14/18] of: export new of_cdev_find helper Ahmad Fatoum
` (4 subsequent siblings)
17 siblings, 1 reply; 62+ messages in thread
From: Ahmad Fatoum @ 2023-05-31 14:59 UTC (permalink / raw)
To: barebox; +Cc: Ahmad Fatoum
We look too much into struct cdev's guts. Let's add helpers to make
operating on block device cdevs more concise.
Signed-off-by: Ahmad Fatoum <a.fatoum@pengutronix.de>
---
include/block.h | 15 +++++++++++++++
1 file changed, 15 insertions(+)
diff --git a/include/block.h b/include/block.h
index 4dd2aa1f80ef..da258f509b41 100644
--- a/include/block.h
+++ b/include/block.h
@@ -58,4 +58,19 @@ static inline struct block_device *cdev_get_block_device(const struct cdev *cdev
}
#endif
+static inline bool cdev_is_block_device(const struct cdev *cdev)
+{
+ return cdev_get_block_device(cdev) != NULL;
+}
+
+static inline bool cdev_is_block_partition(const struct cdev *cdev)
+{
+ return cdev_is_block_device(cdev) && cdev_is_partition(cdev);
+}
+
+static inline bool cdev_is_block_disk(const struct cdev *cdev)
+{
+ return cdev_is_block_device(cdev) && !cdev_is_partition(cdev);
+}
+
#endif /* __BLOCK_H */
--
2.39.2
^ permalink raw reply [flat|nested] 62+ messages in thread* Re: [PATCH 13/18] block: add cdev_is_block_(device,partition,disk) helpers
2023-05-31 14:59 ` [PATCH 13/18] block: add cdev_is_block_(device,partition,disk) helpers Ahmad Fatoum
@ 2023-05-31 17:35 ` Marco Felsch
0 siblings, 0 replies; 62+ messages in thread
From: Marco Felsch @ 2023-05-31 17:35 UTC (permalink / raw)
To: Ahmad Fatoum; +Cc: barebox
On 23-05-31, Ahmad Fatoum wrote:
> We look too much into struct cdev's guts. Let's add helpers to make
> operating on block device cdevs more concise.
>
> Signed-off-by: Ahmad Fatoum <a.fatoum@pengutronix.de>
Reviewed-by: Marco Felsch <m.felsch@pengutronix.de>
> ---
> include/block.h | 15 +++++++++++++++
> 1 file changed, 15 insertions(+)
>
> diff --git a/include/block.h b/include/block.h
> index 4dd2aa1f80ef..da258f509b41 100644
> --- a/include/block.h
> +++ b/include/block.h
> @@ -58,4 +58,19 @@ static inline struct block_device *cdev_get_block_device(const struct cdev *cdev
> }
> #endif
>
> +static inline bool cdev_is_block_device(const struct cdev *cdev)
> +{
> + return cdev_get_block_device(cdev) != NULL;
> +}
> +
> +static inline bool cdev_is_block_partition(const struct cdev *cdev)
> +{
> + return cdev_is_block_device(cdev) && cdev_is_partition(cdev);
> +}
> +
> +static inline bool cdev_is_block_disk(const struct cdev *cdev)
> +{
> + return cdev_is_block_device(cdev) && !cdev_is_partition(cdev);
> +}
> +
> #endif /* __BLOCK_H */
> --
> 2.39.2
>
>
>
^ permalink raw reply [flat|nested] 62+ messages in thread
* [PATCH 14/18] of: export new of_cdev_find helper
2023-05-31 14:59 [PATCH 00/18] state: allow lookup of barebox state partition by Type GUID Ahmad Fatoum
` (12 preceding siblings ...)
2023-05-31 14:59 ` [PATCH 13/18] block: add cdev_is_block_(device,partition,disk) helpers Ahmad Fatoum
@ 2023-05-31 14:59 ` Ahmad Fatoum
2023-05-31 17:41 ` Marco Felsch
2023-06-01 8:41 ` Ulrich Ölmann
2023-05-31 14:59 ` [PATCH 15/18] state: factor device path lookup into helper function Ahmad Fatoum
` (3 subsequent siblings)
17 siblings, 2 replies; 62+ messages in thread
From: Ahmad Fatoum @ 2023-05-31 14:59 UTC (permalink / raw)
To: barebox; +Cc: Ahmad Fatoum
__of_find_path goes throught the hassle of determining the cdev, only to
discard it again and return either zero or an error code.
Follow up commits will need to get the cdev corresponding to a path in
the DT. So let's make that easier by exporting a suitable helper function.
Signed-off-by: Ahmad Fatoum <a.fatoum@pengutronix.de>
---
drivers/of/of_path.c | 59 ++++++++++++++++++++++++++++++--------------
include/of.h | 1 +
2 files changed, 41 insertions(+), 19 deletions(-)
diff --git a/drivers/of/of_path.c b/drivers/of/of_path.c
index 059690e9b8e8..4d9f7b6005af 100644
--- a/drivers/of/of_path.c
+++ b/drivers/of/of_path.c
@@ -27,21 +27,17 @@ struct device *of_find_device_by_node_path(const char *path)
}
/**
- * __of_find_path
+ * __of_cdev_find
*
* @node: The node to find the cdev for, can be the device or a
* partition in the device
* @part: Optionally, a description of a partition of @node. See of_find_path
- * @outpath: if this function returns 0 outpath will contain the path belonging
- * to the input path description. Must be freed with free().
- * @flags: use OF_FIND_PATH_FLAGS_BB to return the .bb device if available
*
*/
-static int __of_find_path(struct device_node *node, const char *part, char **outpath, unsigned flags)
+static struct cdev *__of_cdev_find(struct device_node *node, const char *part)
{
struct device *dev;
struct cdev *cdev;
- bool add_bb = false;
of_partition_ensure_probed(node);
@@ -56,24 +52,17 @@ static int __of_find_path(struct device_node *node, const char *part, char **out
/* when partuuid is specified short-circuit the search for the cdev */
ret = of_property_read_string(node, "partuuid", &uuid);
- if (!ret) {
- cdev = cdev_by_partuuid(uuid);
- if (!cdev)
- return -ENODEV;
-
- *outpath = basprintf("/dev/%s", cdev->name);
-
- return 0;
- }
+ if (!ret)
+ return cdev_by_partuuid(uuid) ?: ERR_PTR(-ENODEV);
}
dev = of_find_device_by_node_path(devnode->full_name);
if (!dev)
- return -ENODEV;
+ return ERR_PTR(-ENODEV);
}
if (dev->bus && !dev->driver)
- return -EPROBE_DEFER;
+ return ERR_PTR(-EPROBE_DEFER);
device_detect(dev);
@@ -82,8 +71,40 @@ static int __of_find_path(struct device_node *node, const char *part, char **out
else
cdev = cdev_by_device_node(node);
- if (!cdev)
- return -ENOENT;
+ return cdev ?: ERR_PTR(-ENOENT);
+}
+
+/**
+ * of_cdev_find
+ *
+ * @node: The node to find the cdev for, can be the device or a
+ * partition in the device
+ *
+ */
+struct cdev *of_cdev_find(struct device_node *node)
+{
+ return __of_cdev_find(node, NULL);
+}
+
+/**
+ * __of_find_path
+ *
+ * @node: The node to find the cdev for, can be the device or a
+ * partition in the device
+ * @part: Optionally, a description of a partition of @node. See of_find_path
+ * @outpath: if this function returns 0 outpath will contain the path belonging
+ * to the input path description. Must be freed with free().
+ * @flags: use OF_FIND_PATH_FLAGS_BB to return the .bb device if available
+ *
+ */
+static int __of_find_path(struct device_node *node, const char *part, char **outpath, unsigned flags)
+{
+ bool add_bb = false;
+ struct cdev *cdev;
+
+ cdev = __of_cdev_find(node, part);
+ if (IS_ERR(cdev))
+ return PTR_ERR(cdev);
if ((flags & OF_FIND_PATH_FLAGS_BB) && cdev->mtd &&
mtd_can_have_bb(cdev->mtd))
diff --git a/include/of.h b/include/of.h
index c716f9283316..2b75ce63e185 100644
--- a/include/of.h
+++ b/include/of.h
@@ -331,6 +331,7 @@ int of_add_memory_bank(struct device_node *node, bool dump, int r,
struct device *of_find_device_by_node_path(const char *path);
#define OF_FIND_PATH_FLAGS_BB 1 /* return .bb device if available */
int of_find_path(struct device_node *node, const char *propname, char **outpath, unsigned flags);
+struct cdev *of_cdev_find(struct device_node *node);
int of_find_path_by_node(struct device_node *node, char **outpath, unsigned flags);
struct device_node *of_find_node_by_devpath(struct device_node *root, const char *path);
int of_register_fixup(int (*fixup)(struct device_node *, void *), void *context);
--
2.39.2
^ permalink raw reply [flat|nested] 62+ messages in thread* Re: [PATCH 14/18] of: export new of_cdev_find helper
2023-05-31 14:59 ` [PATCH 14/18] of: export new of_cdev_find helper Ahmad Fatoum
@ 2023-05-31 17:41 ` Marco Felsch
2023-06-01 8:41 ` Ulrich Ölmann
1 sibling, 0 replies; 62+ messages in thread
From: Marco Felsch @ 2023-05-31 17:41 UTC (permalink / raw)
To: Ahmad Fatoum; +Cc: barebox
On 23-05-31, Ahmad Fatoum wrote:
> __of_find_path goes throught the hassle of determining the cdev, only to
> discard it again and return either zero or an error code.
>
> Follow up commits will need to get the cdev corresponding to a path in
> the DT. So let's make that easier by exporting a suitable helper function.
>
> Signed-off-by: Ahmad Fatoum <a.fatoum@pengutronix.de>
Reviewed-by: Marco Felsch <m.felsch@pengutronix.de>
> ---
> drivers/of/of_path.c | 59 ++++++++++++++++++++++++++++++--------------
> include/of.h | 1 +
> 2 files changed, 41 insertions(+), 19 deletions(-)
>
> diff --git a/drivers/of/of_path.c b/drivers/of/of_path.c
> index 059690e9b8e8..4d9f7b6005af 100644
> --- a/drivers/of/of_path.c
> +++ b/drivers/of/of_path.c
> @@ -27,21 +27,17 @@ struct device *of_find_device_by_node_path(const char *path)
> }
>
> /**
> - * __of_find_path
> + * __of_cdev_find
> *
> * @node: The node to find the cdev for, can be the device or a
> * partition in the device
> * @part: Optionally, a description of a partition of @node. See of_find_path
> - * @outpath: if this function returns 0 outpath will contain the path belonging
> - * to the input path description. Must be freed with free().
> - * @flags: use OF_FIND_PATH_FLAGS_BB to return the .bb device if available
> *
> */
> -static int __of_find_path(struct device_node *node, const char *part, char **outpath, unsigned flags)
> +static struct cdev *__of_cdev_find(struct device_node *node, const char *part)
> {
> struct device *dev;
> struct cdev *cdev;
> - bool add_bb = false;
>
> of_partition_ensure_probed(node);
>
> @@ -56,24 +52,17 @@ static int __of_find_path(struct device_node *node, const char *part, char **out
>
> /* when partuuid is specified short-circuit the search for the cdev */
> ret = of_property_read_string(node, "partuuid", &uuid);
> - if (!ret) {
> - cdev = cdev_by_partuuid(uuid);
> - if (!cdev)
> - return -ENODEV;
> -
> - *outpath = basprintf("/dev/%s", cdev->name);
> -
> - return 0;
> - }
> + if (!ret)
> + return cdev_by_partuuid(uuid) ?: ERR_PTR(-ENODEV);
> }
>
> dev = of_find_device_by_node_path(devnode->full_name);
> if (!dev)
> - return -ENODEV;
> + return ERR_PTR(-ENODEV);
> }
>
> if (dev->bus && !dev->driver)
> - return -EPROBE_DEFER;
> + return ERR_PTR(-EPROBE_DEFER);
>
> device_detect(dev);
>
> @@ -82,8 +71,40 @@ static int __of_find_path(struct device_node *node, const char *part, char **out
> else
> cdev = cdev_by_device_node(node);
>
> - if (!cdev)
> - return -ENOENT;
> + return cdev ?: ERR_PTR(-ENOENT);
> +}
> +
> +/**
> + * of_cdev_find
> + *
> + * @node: The node to find the cdev for, can be the device or a
> + * partition in the device
> + *
> + */
> +struct cdev *of_cdev_find(struct device_node *node)
> +{
> + return __of_cdev_find(node, NULL);
> +}
> +
> +/**
> + * __of_find_path
> + *
> + * @node: The node to find the cdev for, can be the device or a
> + * partition in the device
> + * @part: Optionally, a description of a partition of @node. See of_find_path
> + * @outpath: if this function returns 0 outpath will contain the path belonging
> + * to the input path description. Must be freed with free().
> + * @flags: use OF_FIND_PATH_FLAGS_BB to return the .bb device if available
> + *
> + */
> +static int __of_find_path(struct device_node *node, const char *part, char **outpath, unsigned flags)
> +{
> + bool add_bb = false;
> + struct cdev *cdev;
> +
> + cdev = __of_cdev_find(node, part);
> + if (IS_ERR(cdev))
> + return PTR_ERR(cdev);
>
> if ((flags & OF_FIND_PATH_FLAGS_BB) && cdev->mtd &&
> mtd_can_have_bb(cdev->mtd))
> diff --git a/include/of.h b/include/of.h
> index c716f9283316..2b75ce63e185 100644
> --- a/include/of.h
> +++ b/include/of.h
> @@ -331,6 +331,7 @@ int of_add_memory_bank(struct device_node *node, bool dump, int r,
> struct device *of_find_device_by_node_path(const char *path);
> #define OF_FIND_PATH_FLAGS_BB 1 /* return .bb device if available */
> int of_find_path(struct device_node *node, const char *propname, char **outpath, unsigned flags);
> +struct cdev *of_cdev_find(struct device_node *node);
> int of_find_path_by_node(struct device_node *node, char **outpath, unsigned flags);
> struct device_node *of_find_node_by_devpath(struct device_node *root, const char *path);
> int of_register_fixup(int (*fixup)(struct device_node *, void *), void *context);
> --
> 2.39.2
>
>
>
^ permalink raw reply [flat|nested] 62+ messages in thread* Re: [PATCH 14/18] of: export new of_cdev_find helper
2023-05-31 14:59 ` [PATCH 14/18] of: export new of_cdev_find helper Ahmad Fatoum
2023-05-31 17:41 ` Marco Felsch
@ 2023-06-01 8:41 ` Ulrich Ölmann
1 sibling, 0 replies; 62+ messages in thread
From: Ulrich Ölmann @ 2023-06-01 8:41 UTC (permalink / raw)
To: Ahmad Fatoum; +Cc: barebox
Hi Ahmad,
another small typo.
On Wed, May 31 2023 at 16:59 +0200, Ahmad Fatoum <a.fatoum@pengutronix.de> wrote:
> __of_find_path goes throught the hassle of determining the cdev, only to
s/throught/through/
Best regards
Ulrich
> discard it again and return either zero or an error code.
>
> Follow up commits will need to get the cdev corresponding to a path in
> the DT. So let's make that easier by exporting a suitable helper function.
>
> Signed-off-by: Ahmad Fatoum <a.fatoum@pengutronix.de>
> ---
> drivers/of/of_path.c | 59 ++++++++++++++++++++++++++++++--------------
> include/of.h | 1 +
> 2 files changed, 41 insertions(+), 19 deletions(-)
>
> diff --git a/drivers/of/of_path.c b/drivers/of/of_path.c
> index 059690e9b8e8..4d9f7b6005af 100644
> --- a/drivers/of/of_path.c
> +++ b/drivers/of/of_path.c
> @@ -27,21 +27,17 @@ struct device *of_find_device_by_node_path(const char *path)
> }
>
> /**
> - * __of_find_path
> + * __of_cdev_find
> *
> * @node: The node to find the cdev for, can be the device or a
> * partition in the device
> * @part: Optionally, a description of a partition of @node. See of_find_path
> - * @outpath: if this function returns 0 outpath will contain the path belonging
> - * to the input path description. Must be freed with free().
> - * @flags: use OF_FIND_PATH_FLAGS_BB to return the .bb device if available
> *
> */
> -static int __of_find_path(struct device_node *node, const char *part, char **outpath, unsigned flags)
> +static struct cdev *__of_cdev_find(struct device_node *node, const char *part)
> {
> struct device *dev;
> struct cdev *cdev;
> - bool add_bb = false;
>
> of_partition_ensure_probed(node);
>
> @@ -56,24 +52,17 @@ static int __of_find_path(struct device_node *node, const char *part, char **out
>
> /* when partuuid is specified short-circuit the search for the cdev */
> ret = of_property_read_string(node, "partuuid", &uuid);
> - if (!ret) {
> - cdev = cdev_by_partuuid(uuid);
> - if (!cdev)
> - return -ENODEV;
> -
> - *outpath = basprintf("/dev/%s", cdev->name);
> -
> - return 0;
> - }
> + if (!ret)
> + return cdev_by_partuuid(uuid) ?: ERR_PTR(-ENODEV);
> }
>
> dev = of_find_device_by_node_path(devnode->full_name);
> if (!dev)
> - return -ENODEV;
> + return ERR_PTR(-ENODEV);
> }
>
> if (dev->bus && !dev->driver)
> - return -EPROBE_DEFER;
> + return ERR_PTR(-EPROBE_DEFER);
>
> device_detect(dev);
>
> @@ -82,8 +71,40 @@ static int __of_find_path(struct device_node *node, const char *part, char **out
> else
> cdev = cdev_by_device_node(node);
>
> - if (!cdev)
> - return -ENOENT;
> + return cdev ?: ERR_PTR(-ENOENT);
> +}
> +
> +/**
> + * of_cdev_find
> + *
> + * @node: The node to find the cdev for, can be the device or a
> + * partition in the device
> + *
> + */
> +struct cdev *of_cdev_find(struct device_node *node)
> +{
> + return __of_cdev_find(node, NULL);
> +}
> +
> +/**
> + * __of_find_path
> + *
> + * @node: The node to find the cdev for, can be the device or a
> + * partition in the device
> + * @part: Optionally, a description of a partition of @node. See of_find_path
> + * @outpath: if this function returns 0 outpath will contain the path belonging
> + * to the input path description. Must be freed with free().
> + * @flags: use OF_FIND_PATH_FLAGS_BB to return the .bb device if available
> + *
> + */
> +static int __of_find_path(struct device_node *node, const char *part, char **outpath, unsigned flags)
> +{
> + bool add_bb = false;
> + struct cdev *cdev;
> +
> + cdev = __of_cdev_find(node, part);
> + if (IS_ERR(cdev))
> + return PTR_ERR(cdev);
>
> if ((flags & OF_FIND_PATH_FLAGS_BB) && cdev->mtd &&
> mtd_can_have_bb(cdev->mtd))
> diff --git a/include/of.h b/include/of.h
> index c716f9283316..2b75ce63e185 100644
> --- a/include/of.h
> +++ b/include/of.h
> @@ -331,6 +331,7 @@ int of_add_memory_bank(struct device_node *node, bool dump, int r,
> struct device *of_find_device_by_node_path(const char *path);
> #define OF_FIND_PATH_FLAGS_BB 1 /* return .bb device if available */
> int of_find_path(struct device_node *node, const char *propname, char **outpath, unsigned flags);
> +struct cdev *of_cdev_find(struct device_node *node);
> int of_find_path_by_node(struct device_node *node, char **outpath, unsigned flags);
> struct device_node *of_find_node_by_devpath(struct device_node *root, const char *path);
> int of_register_fixup(int (*fixup)(struct device_node *, void *), void *context);
--
Pengutronix e.K. | Ulrich Ölmann |
Industrial Linux Solutions | http://www.pengutronix.de/ |
Peiner Str. 6-8, 31137 Hildesheim, Germany | Phone: +49-5121-206917-0 |
Amtsgericht Hildesheim, HRA 2686 | Fax: +49-5121-206917-5555 |
^ permalink raw reply [flat|nested] 62+ messages in thread
* [PATCH 15/18] state: factor device path lookup into helper function
2023-05-31 14:59 [PATCH 00/18] state: allow lookup of barebox state partition by Type GUID Ahmad Fatoum
` (13 preceding siblings ...)
2023-05-31 14:59 ` [PATCH 14/18] of: export new of_cdev_find helper Ahmad Fatoum
@ 2023-05-31 14:59 ` Ahmad Fatoum
2023-05-31 17:54 ` Marco Felsch
2023-05-31 14:59 ` [PATCH 16/18] cdev: use cdev::dos_partition_type only if cdev_is_mbr_partitioned Ahmad Fatoum
` (2 subsequent siblings)
17 siblings, 1 reply; 62+ messages in thread
From: Ahmad Fatoum @ 2023-05-31 14:59 UTC (permalink / raw)
To: barebox; +Cc: Ahmad Fatoum
The #ifdef __BAREBOX__ is meant for easier synchronization with
dt-utils. We'll keep that intact, but move it out of the function to not
break reading flow. After sync, dt-utils would now need to implement
of_cdev_find
cdev_to_devpath
Signed-off-by: Ahmad Fatoum <a.fatoum@pengutronix.de>
---
common/state/state.c | 30 +++++++++++++++++++++++-------
1 file changed, 23 insertions(+), 7 deletions(-)
diff --git a/common/state/state.c b/common/state/state.c
index 11cc86ff73be..88e246198fb8 100644
--- a/common/state/state.c
+++ b/common/state/state.c
@@ -581,6 +581,20 @@ void state_release(struct state *state)
free(state);
}
+#ifdef __BAREBOX__
+static char *cdev_to_devpath(struct cdev *cdev, off_t *offset, size_t *size)
+{
+ /*
+ * We only accept partitions exactly mapping the barebox-state,
+ * but dt-utils may need to set non-zero values here
+ */
+ *offset = 0;
+ *size = 0;
+
+ return basprintf("/dev/%s", cdev->name);
+}
+#endif
+
/*
* state_new_from_node - create a new state instance from a device_node
*
@@ -597,8 +611,9 @@ struct state *state_new_from_node(struct device_node *node, bool readonly)
const char *alias;
uint32_t stridesize;
struct device_node *partition_node;
- off_t offset = 0;
- size_t size = 0;
+ struct cdev *cdev;
+ off_t offset;
+ size_t size;
alias = of_alias_get(node);
if (!alias) {
@@ -617,11 +632,8 @@ struct state *state_new_from_node(struct device_node *node, bool readonly)
goto out_release_state;
}
-#ifdef __BAREBOX__
- ret = of_find_path_by_node(partition_node, &state->backend_path, 0);
-#else
- ret = of_get_devicepath(partition_node, &state->backend_path, &offset, &size);
-#endif
+ cdev = of_cdev_find(partition_node);
+ ret = PTR_ERR_OR_ZERO(cdev);
if (ret) {
if (ret != -EPROBE_DEFER)
dev_err(&state->dev, "state failed to parse path to backend: %s\n",
@@ -629,6 +641,10 @@ struct state *state_new_from_node(struct device_node *node, bool readonly)
goto out_release_state;
}
+ state->backend_path = cdev_to_devpath(cdev, &offset, &size);
+
+ pr_debug("%s: backend resolved to %s\n", node->full_name, state->backend_path);
+
state->backend_reproducible_name = of_get_reproducible_name(partition_node);
ret = of_property_read_string(node, "backend-type", &backend_type);
--
2.39.2
^ permalink raw reply [flat|nested] 62+ messages in thread* Re: [PATCH 15/18] state: factor device path lookup into helper function
2023-05-31 14:59 ` [PATCH 15/18] state: factor device path lookup into helper function Ahmad Fatoum
@ 2023-05-31 17:54 ` Marco Felsch
2023-06-01 5:14 ` Ahmad Fatoum
0 siblings, 1 reply; 62+ messages in thread
From: Marco Felsch @ 2023-05-31 17:54 UTC (permalink / raw)
To: Ahmad Fatoum; +Cc: barebox
Hi Ahmad,
On 23-05-31, Ahmad Fatoum wrote:
> The #ifdef __BAREBOX__ is meant for easier synchronization with
> dt-utils. We'll keep that intact, but move it out of the function to not
> break reading flow. After sync, dt-utils would now need to implement
>
> of_cdev_find
> cdev_to_devpath
>
> Signed-off-by: Ahmad Fatoum <a.fatoum@pengutronix.de>
> ---
> common/state/state.c | 30 +++++++++++++++++++++++-------
> 1 file changed, 23 insertions(+), 7 deletions(-)
>
> diff --git a/common/state/state.c b/common/state/state.c
> index 11cc86ff73be..88e246198fb8 100644
> --- a/common/state/state.c
> +++ b/common/state/state.c
> @@ -581,6 +581,20 @@ void state_release(struct state *state)
> free(state);
> }
>
> +#ifdef __BAREBOX__
> +static char *cdev_to_devpath(struct cdev *cdev, off_t *offset, size_t *size)
> +{
> + /*
> + * We only accept partitions exactly mapping the barebox-state,
> + * but dt-utils may need to set non-zero values here
> + */
> + *offset = 0;
> + *size = 0;
> +
> + return basprintf("/dev/%s", cdev->name);
> +}
> +#endif
We could get rid of the #ifdef if we move this function to some barebox
internal code not shared with dt-utils.
Regards,
Marco
> +
> /*
> * state_new_from_node - create a new state instance from a device_node
> *
> @@ -597,8 +611,9 @@ struct state *state_new_from_node(struct device_node *node, bool readonly)
> const char *alias;
> uint32_t stridesize;
> struct device_node *partition_node;
> - off_t offset = 0;
> - size_t size = 0;
> + struct cdev *cdev;
> + off_t offset;
> + size_t size;
>
> alias = of_alias_get(node);
> if (!alias) {
> @@ -617,11 +632,8 @@ struct state *state_new_from_node(struct device_node *node, bool readonly)
> goto out_release_state;
> }
>
> -#ifdef __BAREBOX__
> - ret = of_find_path_by_node(partition_node, &state->backend_path, 0);
> -#else
> - ret = of_get_devicepath(partition_node, &state->backend_path, &offset, &size);
> -#endif
> + cdev = of_cdev_find(partition_node);
> + ret = PTR_ERR_OR_ZERO(cdev);
> if (ret) {
> if (ret != -EPROBE_DEFER)
> dev_err(&state->dev, "state failed to parse path to backend: %s\n",
> @@ -629,6 +641,10 @@ struct state *state_new_from_node(struct device_node *node, bool readonly)
> goto out_release_state;
> }
>
> + state->backend_path = cdev_to_devpath(cdev, &offset, &size);
> +
> + pr_debug("%s: backend resolved to %s\n", node->full_name, state->backend_path);
> +
> state->backend_reproducible_name = of_get_reproducible_name(partition_node);
>
> ret = of_property_read_string(node, "backend-type", &backend_type);
> --
> 2.39.2
>
>
>
^ permalink raw reply [flat|nested] 62+ messages in thread* Re: [PATCH 15/18] state: factor device path lookup into helper function
2023-05-31 17:54 ` Marco Felsch
@ 2023-06-01 5:14 ` Ahmad Fatoum
0 siblings, 0 replies; 62+ messages in thread
From: Ahmad Fatoum @ 2023-06-01 5:14 UTC (permalink / raw)
To: Marco Felsch; +Cc: barebox
On 31.05.23 19:54, Marco Felsch wrote:
> Hi Ahmad,
>
> On 23-05-31, Ahmad Fatoum wrote:
>> The #ifdef __BAREBOX__ is meant for easier synchronization with
>> dt-utils. We'll keep that intact, but move it out of the function to not
>> break reading flow. After sync, dt-utils would now need to implement
>>
>> of_cdev_find
>> cdev_to_devpath
>>
>> Signed-off-by: Ahmad Fatoum <a.fatoum@pengutronix.de>
>> ---
>> common/state/state.c | 30 +++++++++++++++++++++++-------
>> 1 file changed, 23 insertions(+), 7 deletions(-)
>>
>> diff --git a/common/state/state.c b/common/state/state.c
>> index 11cc86ff73be..88e246198fb8 100644
>> --- a/common/state/state.c
>> +++ b/common/state/state.c
>> @@ -581,6 +581,20 @@ void state_release(struct state *state)
>> free(state);
>> }
>>
>> +#ifdef __BAREBOX__
>> +static char *cdev_to_devpath(struct cdev *cdev, off_t *offset, size_t *size)
>> +{
>> + /*
>> + * We only accept partitions exactly mapping the barebox-state,
>> + * but dt-utils may need to set non-zero values here
>> + */
>> + *offset = 0;
>> + *size = 0;
>> +
>> + return basprintf("/dev/%s", cdev->name);
>> +}
>> +#endif
>
> We could get rid of the #ifdef if we move this function to some barebox
> internal code not shared with dt-utils.
Setting offset and size to zero makes no sense elsewhere, that's why I left it here.
> Regards,
> Marco
>
>> +
>> /*
>> * state_new_from_node - create a new state instance from a device_node
>> *
>> @@ -597,8 +611,9 @@ struct state *state_new_from_node(struct device_node *node, bool readonly)
>> const char *alias;
>> uint32_t stridesize;
>> struct device_node *partition_node;
>> - off_t offset = 0;
>> - size_t size = 0;
>> + struct cdev *cdev;
>> + off_t offset;
>> + size_t size;
>>
>> alias = of_alias_get(node);
>> if (!alias) {
>> @@ -617,11 +632,8 @@ struct state *state_new_from_node(struct device_node *node, bool readonly)
>> goto out_release_state;
>> }
>>
>> -#ifdef __BAREBOX__
>> - ret = of_find_path_by_node(partition_node, &state->backend_path, 0);
>> -#else
>> - ret = of_get_devicepath(partition_node, &state->backend_path, &offset, &size);
>> -#endif
>> + cdev = of_cdev_find(partition_node);
>> + ret = PTR_ERR_OR_ZERO(cdev);
>> if (ret) {
>> if (ret != -EPROBE_DEFER)
>> dev_err(&state->dev, "state failed to parse path to backend: %s\n",
>> @@ -629,6 +641,10 @@ struct state *state_new_from_node(struct device_node *node, bool readonly)
>> goto out_release_state;
>> }
>>
>> + state->backend_path = cdev_to_devpath(cdev, &offset, &size);
>> +
>> + pr_debug("%s: backend resolved to %s\n", node->full_name, state->backend_path);
>> +
>> state->backend_reproducible_name = of_get_reproducible_name(partition_node);
>>
>> ret = of_property_read_string(node, "backend-type", &backend_type);
>> --
>> 2.39.2
>>
>>
>>
>
--
Pengutronix e.K. | |
Steuerwalder Str. 21 | http://www.pengutronix.de/ |
31137 Hildesheim, Germany | Phone: +49-5121-206917-0 |
Amtsgericht Hildesheim, HRA 2686 | Fax: +49-5121-206917-5555 |
^ permalink raw reply [flat|nested] 62+ messages in thread
* [PATCH 16/18] cdev: use cdev::dos_partition_type only if cdev_is_mbr_partitioned
2023-05-31 14:59 [PATCH 00/18] state: allow lookup of barebox state partition by Type GUID Ahmad Fatoum
` (14 preceding siblings ...)
2023-05-31 14:59 ` [PATCH 15/18] state: factor device path lookup into helper function Ahmad Fatoum
@ 2023-05-31 14:59 ` Ahmad Fatoum
2023-05-31 18:54 ` Marco Felsch
2023-05-31 14:59 ` [PATCH 17/18] common: partitions: efi: record type UUID in cdev Ahmad Fatoum
2023-05-31 14:59 ` [PATCH 18/18] state: allow lookup of barebox state partition by Type GUID Ahmad Fatoum
17 siblings, 1 reply; 62+ messages in thread
From: Ahmad Fatoum @ 2023-05-31 14:59 UTC (permalink / raw)
To: barebox; +Cc: Ahmad Fatoum
dos_partition_type == 0 can mean that either a partition is not
a MBR partition or that it indeed has a partition type of 0x00.
In preparation for using that field in a union, explicitly check if we
have a MBR partition.
Signed-off-by: Ahmad Fatoum <a.fatoum@pengutronix.de>
---
common/blspec.c | 2 +-
fs/fs.c | 2 +-
2 files changed, 2 insertions(+), 2 deletions(-)
diff --git a/common/blspec.c b/common/blspec.c
index e95a8dba8d76..8c7970da8915 100644
--- a/common/blspec.c
+++ b/common/blspec.c
@@ -729,7 +729,7 @@ int blspec_scan_device(struct bootentries *bootentries, struct device *dev)
* partition with the MBR type id of 0xEA already exists it
* should be used as $BOOT
*/
- if (cdev->dos_partition_type == 0xea) {
+ if (cdev_is_mbr_partitioned(cdev->master) && cdev->dos_partition_type == 0xea) {
ret = blspec_scan_cdev(bootentries, cdev);
if (ret == 0)
ret = -ENOENT;
diff --git a/fs/fs.c b/fs/fs.c
index 2d2d327c5fbc..9a92e6e251e5 100644
--- a/fs/fs.c
+++ b/fs/fs.c
@@ -108,7 +108,7 @@ void cdev_print(const struct cdev *cdev)
if (cdev->filetype)
nbytes += printf("Filetype: %s\t", file_type_to_string(cdev->filetype));
- if (cdev->dos_partition_type)
+ if (cdev_is_mbr_partitioned(cdev->master))
nbytes += printf("DOS parttype: 0x%02x\t", cdev->dos_partition_type);
if (*cdev->partuuid || *cdev->diskuuid)
nbytes += printf("%sUUID: %s", cdev_is_partition(cdev) ? "PART" : "DISK",
--
2.39.2
^ permalink raw reply [flat|nested] 62+ messages in thread* Re: [PATCH 16/18] cdev: use cdev::dos_partition_type only if cdev_is_mbr_partitioned
2023-05-31 14:59 ` [PATCH 16/18] cdev: use cdev::dos_partition_type only if cdev_is_mbr_partitioned Ahmad Fatoum
@ 2023-05-31 18:54 ` Marco Felsch
2023-06-01 5:30 ` Ahmad Fatoum
0 siblings, 1 reply; 62+ messages in thread
From: Marco Felsch @ 2023-05-31 18:54 UTC (permalink / raw)
To: Ahmad Fatoum; +Cc: barebox
On 23-05-31, Ahmad Fatoum wrote:
> dos_partition_type == 0 can mean that either a partition is not
> a MBR partition or that it indeed has a partition type of 0x00.
>
> In preparation for using that field in a union, explicitly check if we
> have a MBR partition.
>
> Signed-off-by: Ahmad Fatoum <a.fatoum@pengutronix.de>
> ---
> common/blspec.c | 2 +-
> fs/fs.c | 2 +-
> 2 files changed, 2 insertions(+), 2 deletions(-)
>
> diff --git a/common/blspec.c b/common/blspec.c
> index e95a8dba8d76..8c7970da8915 100644
> --- a/common/blspec.c
> +++ b/common/blspec.c
> @@ -729,7 +729,7 @@ int blspec_scan_device(struct bootentries *bootentries, struct device *dev)
> * partition with the MBR type id of 0xEA already exists it
> * should be used as $BOOT
> */
> - if (cdev->dos_partition_type == 0xea) {
> + if (cdev_is_mbr_partitioned(cdev->master) && cdev->dos_partition_type == 0xea) {
Since you already various helpers to drop priv direct access, what
about:
if (cdev_dos_partition_type(cdev) == 0xea)
Within the helper you can check for the cdev_is_mbr_partitioned().
?
Regards,
Marco
> ret = blspec_scan_cdev(bootentries, cdev);
> if (ret == 0)
> ret = -ENOENT;
> diff --git a/fs/fs.c b/fs/fs.c
> index 2d2d327c5fbc..9a92e6e251e5 100644
> --- a/fs/fs.c
> +++ b/fs/fs.c
> @@ -108,7 +108,7 @@ void cdev_print(const struct cdev *cdev)
>
> if (cdev->filetype)
> nbytes += printf("Filetype: %s\t", file_type_to_string(cdev->filetype));
> - if (cdev->dos_partition_type)
> + if (cdev_is_mbr_partitioned(cdev->master))
> nbytes += printf("DOS parttype: 0x%02x\t", cdev->dos_partition_type);
> if (*cdev->partuuid || *cdev->diskuuid)
> nbytes += printf("%sUUID: %s", cdev_is_partition(cdev) ? "PART" : "DISK",
> --
> 2.39.2
>
>
>
^ permalink raw reply [flat|nested] 62+ messages in thread* Re: [PATCH 16/18] cdev: use cdev::dos_partition_type only if cdev_is_mbr_partitioned
2023-05-31 18:54 ` Marco Felsch
@ 2023-06-01 5:30 ` Ahmad Fatoum
0 siblings, 0 replies; 62+ messages in thread
From: Ahmad Fatoum @ 2023-06-01 5:30 UTC (permalink / raw)
To: Marco Felsch; +Cc: barebox
On 31.05.23 20:54, Marco Felsch wrote:
> On 23-05-31, Ahmad Fatoum wrote:
>> dos_partition_type == 0 can mean that either a partition is not
>> a MBR partition or that it indeed has a partition type of 0x00.
>>
>> In preparation for using that field in a union, explicitly check if we
>> have a MBR partition.
>>
>> Signed-off-by: Ahmad Fatoum <a.fatoum@pengutronix.de>
>> ---
>> common/blspec.c | 2 +-
>> fs/fs.c | 2 +-
>> 2 files changed, 2 insertions(+), 2 deletions(-)
>>
>> diff --git a/common/blspec.c b/common/blspec.c
>> index e95a8dba8d76..8c7970da8915 100644
>> --- a/common/blspec.c
>> +++ b/common/blspec.c
>> @@ -729,7 +729,7 @@ int blspec_scan_device(struct bootentries *bootentries, struct device *dev)
>> * partition with the MBR type id of 0xEA already exists it
>> * should be used as $BOOT
>> */
>> - if (cdev->dos_partition_type == 0xea) {
>> + if (cdev_is_mbr_partitioned(cdev->master) && cdev->dos_partition_type == 0xea) {
>
> Since you already various helpers to drop priv direct access, what
> about:
>
> if (cdev_dos_partition_type(cdev) == 0xea)
>
> Within the helper you can check for the cdev_is_mbr_partitioned().
We only have a single call site and there'll be a separate series that adds:
if (cdev_is_gpt_partitioned(cdev->master) && guid_equal(&partcdev->typeuuid, blspec_xbootldr_guid))
/* use that */;
after this line. So I'd rather leave it as is.
Thanks,
Ahmad
>
> ?
>
> Regards,
> Marco
>
>> ret = blspec_scan_cdev(bootentries, cdev);
>> if (ret == 0)
>> ret = -ENOENT;
>> diff --git a/fs/fs.c b/fs/fs.c
>> index 2d2d327c5fbc..9a92e6e251e5 100644
>> --- a/fs/fs.c
>> +++ b/fs/fs.c
>> @@ -108,7 +108,7 @@ void cdev_print(const struct cdev *cdev)
>>
>> if (cdev->filetype)
>> nbytes += printf("Filetype: %s\t", file_type_to_string(cdev->filetype));
>> - if (cdev->dos_partition_type)
>> + if (cdev_is_mbr_partitioned(cdev->master))
>> nbytes += printf("DOS parttype: 0x%02x\t", cdev->dos_partition_type);
>> if (*cdev->partuuid || *cdev->diskuuid)
>> nbytes += printf("%sUUID: %s", cdev_is_partition(cdev) ? "PART" : "DISK",
>> --
>> 2.39.2
>>
>>
>>
>
--
Pengutronix e.K. | |
Steuerwalder Str. 21 | http://www.pengutronix.de/ |
31137 Hildesheim, Germany | Phone: +49-5121-206917-0 |
Amtsgericht Hildesheim, HRA 2686 | Fax: +49-5121-206917-5555 |
^ permalink raw reply [flat|nested] 62+ messages in thread
* [PATCH 17/18] common: partitions: efi: record type UUID in cdev
2023-05-31 14:59 [PATCH 00/18] state: allow lookup of barebox state partition by Type GUID Ahmad Fatoum
` (15 preceding siblings ...)
2023-05-31 14:59 ` [PATCH 16/18] cdev: use cdev::dos_partition_type only if cdev_is_mbr_partitioned Ahmad Fatoum
@ 2023-05-31 14:59 ` Ahmad Fatoum
[not found] ` <20230531193130.fgmvxm27dh3gbvhh@pengutronix.de>
2023-05-31 14:59 ` [PATCH 18/18] state: allow lookup of barebox state partition by Type GUID Ahmad Fatoum
17 siblings, 1 reply; 62+ messages in thread
From: Ahmad Fatoum @ 2023-05-31 14:59 UTC (permalink / raw)
To: barebox; +Cc: Ahmad Fatoum
We already record DOS partition type in cdev, so let's do the same for
GPT Type UUID. This will be used in a later commit to identify
barebox-state partitions.
Signed-off-by: Ahmad Fatoum <a.fatoum@pengutronix.de>
---
common/partitions.c | 2 +-
common/partitions/efi.c | 1 +
common/partitions/parser.h | 5 ++++-
fs/fs.c | 2 ++
include/driver.h | 6 +++++-
5 files changed, 13 insertions(+), 3 deletions(-)
diff --git a/common/partitions.c b/common/partitions.c
index b579559672a0..e3e8a9f3044d 100644
--- a/common/partitions.c
+++ b/common/partitions.c
@@ -50,7 +50,7 @@ static int register_one_partition(struct block_device *blk,
cdev->flags |= DEVFS_PARTITION_FROM_TABLE;
- cdev->dos_partition_type = part->dos_partition_type;
+ cdev->typeuuid = part->typeuuid;
strcpy(cdev->partuuid, part->partuuid);
free(partition_name);
diff --git a/common/partitions/efi.c b/common/partitions/efi.c
index df63b82afe24..2756337ab284 100644
--- a/common/partitions/efi.c
+++ b/common/partitions/efi.c
@@ -471,6 +471,7 @@ static void efi_partition(void *buf, struct block_device *blk,
pentry->size++;
part_set_efi_name(&ptes[i], pentry->name);
snprintf(pentry->partuuid, sizeof(pentry->partuuid), "%pUl", &ptes[i].unique_partition_guid);
+ pentry->typeuuid = ptes[i].partition_type_guid;
pd->used_entries++;
}
}
diff --git a/common/partitions/parser.h b/common/partitions/parser.h
index f2f692f7903b..9cc41a7573fe 100644
--- a/common/partitions/parser.h
+++ b/common/partitions/parser.h
@@ -17,10 +17,13 @@
struct partition {
char name[MAX_PARTITION_NAME];
- u8 dos_partition_type;
char partuuid[MAX_UUID_STR];
uint64_t first_sec;
uint64_t size;
+ union {
+ u8 dos_partition_type;
+ guid_t typeuuid;
+ };
};
struct partition_desc {
diff --git a/fs/fs.c b/fs/fs.c
index 9a92e6e251e5..16cc072adfaf 100644
--- a/fs/fs.c
+++ b/fs/fs.c
@@ -110,6 +110,8 @@ void cdev_print(const struct cdev *cdev)
nbytes += printf("Filetype: %s\t", file_type_to_string(cdev->filetype));
if (cdev_is_mbr_partitioned(cdev->master))
nbytes += printf("DOS parttype: 0x%02x\t", cdev->dos_partition_type);
+ else if (cdev_is_gpt_partitioned(cdev->master))
+ nbytes += printf("GPT typeuuid: %pUl\t", &cdev->typeuuid);
if (*cdev->partuuid || *cdev->diskuuid)
nbytes += printf("%sUUID: %s", cdev_is_partition(cdev) ? "PART" : "DISK",
cdev_is_partition(cdev) ? cdev->partuuid : cdev->diskuuid);
diff --git a/include/driver.h b/include/driver.h
index 5f2eae65466f..6407f7d6ba36 100644
--- a/include/driver.h
+++ b/include/driver.h
@@ -8,6 +8,7 @@
#include <linux/list.h>
#include <linux/ioport.h>
+#include <linux/uuid.h>
#include <of.h>
#include <filetype.h>
@@ -536,12 +537,15 @@ struct cdev {
unsigned int flags;
int open;
struct mtd_info *mtd;
- u8 dos_partition_type;
struct cdev *link;
struct list_head link_entry, links;
struct list_head partition_entry, partitions;
struct cdev *master;
enum filetype filetype;
+ union {
+ u8 dos_partition_type;
+ guid_t typeuuid;
+ };
};
int devfs_create(struct cdev *);
--
2.39.2
^ permalink raw reply [flat|nested] 62+ messages in thread* [PATCH 18/18] state: allow lookup of barebox state partition by Type GUID
2023-05-31 14:59 [PATCH 00/18] state: allow lookup of barebox state partition by Type GUID Ahmad Fatoum
` (16 preceding siblings ...)
2023-05-31 14:59 ` [PATCH 17/18] common: partitions: efi: record type UUID in cdev Ahmad Fatoum
@ 2023-05-31 14:59 ` Ahmad Fatoum
2023-05-31 20:01 ` Marco Felsch
2023-06-01 8:05 ` Sascha Hauer
17 siblings, 2 replies; 62+ messages in thread
From: Ahmad Fatoum @ 2023-05-31 14:59 UTC (permalink / raw)
To: barebox; +Cc: Ahmad Fatoum
The backend device tree property so far always pointed at a partition.
Let's allow pointing it at GPT storage devices directly and lookup
the correct barebox state partition by the well-known type GUID:
4778ed65-bf42-45fa-9c5b-287a1dc4aab1
Signed-off-by: Ahmad Fatoum <a.fatoum@pengutronix.de>
---
common/state/state.c | 22 ++++++++++++++++++++++
include/driver.h | 17 +++++++++++++++++
include/state.h | 4 ++++
3 files changed, 43 insertions(+)
diff --git a/common/state/state.c b/common/state/state.c
index 88e246198fb8..8f56c60b0e82 100644
--- a/common/state/state.c
+++ b/common/state/state.c
@@ -21,8 +21,10 @@
#include <fs.h>
#include <crc.h>
#include <init.h>
+#include <block.h>
#include <linux/err.h>
#include <linux/list.h>
+#include <linux/uuid.h>
#include <linux/mtd/mtd-abi.h>
#include <malloc.h>
@@ -595,6 +597,8 @@ static char *cdev_to_devpath(struct cdev *cdev, off_t *offset, size_t *size)
}
#endif
+static guid_t barebox_state_partition_guid = BAREBOX_STATE_PARTITION_GUID;
+
/*
* state_new_from_node - create a new state instance from a device_node
*
@@ -641,6 +645,24 @@ struct state *state_new_from_node(struct device_node *node, bool readonly)
goto out_release_state;
}
+ /* Is the backend referencing an on-disk partitonable block device? */
+ if (cdev_is_block_disk(cdev)) {
+ struct cdev *partcdev = NULL;
+
+ if (cdev_is_gpt_partitioned(cdev))
+ partcdev = cdev_find_child_by_typeuuid(cdev, &barebox_state_partition_guid);
+
+ if (!partcdev) {
+ ret = -EINVAL;
+ goto out_release_state;
+ }
+
+ pr_debug("%s: backend GPT partition looked up via PartitionTypeGUID\n",
+ node->full_name);
+
+ cdev = partcdev;
+ }
+
state->backend_path = cdev_to_devpath(cdev, &offset, &size);
pr_debug("%s: backend resolved to %s\n", node->full_name, state->backend_path);
diff --git a/include/driver.h b/include/driver.h
index 6407f7d6ba36..579b03fbac34 100644
--- a/include/driver.h
+++ b/include/driver.h
@@ -585,6 +585,23 @@ extern struct list_head cdev_list;
#define for_each_cdev(cdev) \
list_for_each_entry(cdev, &cdev_list, list)
+#define for_each_cdev_partition(partcdev, cdev) \
+ list_for_each_entry((partcdev), &(cdev)->partitions, partition_entry)
+
+
+static inline struct cdev *cdev_find_child_by_typeuuid(struct cdev *cdev,
+ guid_t *typeuuid)
+{
+ struct cdev *partcdev;
+
+ for_each_cdev_partition(partcdev, cdev) {
+ if (guid_equal(&partcdev->typeuuid, typeuuid))
+ return partcdev;
+ }
+
+ return NULL;
+}
+
#define DEVFS_PARTITION_FIXED (1U << 0)
#define DEVFS_PARTITION_READONLY (1U << 1)
#define DEVFS_IS_CHARACTER_DEV (1U << 3)
diff --git a/include/state.h b/include/state.h
index bffcd5a9007f..3daf82c0735f 100644
--- a/include/state.h
+++ b/include/state.h
@@ -62,4 +62,8 @@ static inline int state_read_mac(struct state *state, const char *name, u8 *buf)
#endif /* #if IS_ENABLED(CONFIG_STATE) / #else */
+#define BAREBOX_STATE_PARTITION_GUID \
+ GUID_INIT(0x4778ed65, 0xbf42, 0x45fa, 0x9c, 0x5b, \
+ 0x28, 0x7a, 0x1d, 0xc4, 0xaa, 0xb1)
+
#endif /* __STATE_H */
--
2.39.2
^ permalink raw reply [flat|nested] 62+ messages in thread* Re: [PATCH 18/18] state: allow lookup of barebox state partition by Type GUID
2023-05-31 14:59 ` [PATCH 18/18] state: allow lookup of barebox state partition by Type GUID Ahmad Fatoum
@ 2023-05-31 20:01 ` Marco Felsch
2023-06-01 5:49 ` Ahmad Fatoum
2023-06-01 8:05 ` Sascha Hauer
1 sibling, 1 reply; 62+ messages in thread
From: Marco Felsch @ 2023-05-31 20:01 UTC (permalink / raw)
To: Ahmad Fatoum; +Cc: barebox
Hi Ahmad,
On 23-05-31, Ahmad Fatoum wrote:
> The backend device tree property so far always pointed at a partition.
> Let's allow pointing it at GPT storage devices directly and lookup
> the correct barebox state partition by the well-known type GUID:
>
> 4778ed65-bf42-45fa-9c5b-287a1dc4aab1
we should add an example within the Documetation/ too.
> Signed-off-by: Ahmad Fatoum <a.fatoum@pengutronix.de>
> ---
> common/state/state.c | 22 ++++++++++++++++++++++
> include/driver.h | 17 +++++++++++++++++
> include/state.h | 4 ++++
> 3 files changed, 43 insertions(+)
>
> diff --git a/common/state/state.c b/common/state/state.c
> index 88e246198fb8..8f56c60b0e82 100644
> --- a/common/state/state.c
> +++ b/common/state/state.c
> @@ -21,8 +21,10 @@
> #include <fs.h>
> #include <crc.h>
> #include <init.h>
> +#include <block.h>
> #include <linux/err.h>
> #include <linux/list.h>
> +#include <linux/uuid.h>
>
> #include <linux/mtd/mtd-abi.h>
> #include <malloc.h>
> @@ -595,6 +597,8 @@ static char *cdev_to_devpath(struct cdev *cdev, off_t *offset, size_t *size)
> }
> #endif
>
> +static guid_t barebox_state_partition_guid = BAREBOX_STATE_PARTITION_GUID;
> +
> /*
> * state_new_from_node - create a new state instance from a device_node
> *
> @@ -641,6 +645,24 @@ struct state *state_new_from_node(struct device_node *node, bool readonly)
> goto out_release_state;
> }
>
> + /* Is the backend referencing an on-disk partitonable block device? */
> + if (cdev_is_block_disk(cdev)) {
> + struct cdev *partcdev = NULL;
> +
> + if (cdev_is_gpt_partitioned(cdev))
> + partcdev = cdev_find_child_by_typeuuid(cdev, &barebox_state_partition_guid);
> +
> + if (!partcdev) {
> + ret = -EINVAL;
> + goto out_release_state;
> + }
> +
> + pr_debug("%s: backend GPT partition looked up via PartitionTypeGUID\n",
> + node->full_name);
> +
> + cdev = partcdev;
> + }
What about having the above logic within a seperate function and the
above code would be:
if (cdev_is_block_disk(cdev))
cdev = get_cdev_by_typeuuid(cdev, &barebox_state_partition_guid)
if (!cdev) {
ret = -EINVAL;
goto out_release_state;
}
This way we would have everything in place to re-use the same logic for
the barebox-environmnet too. What do you think?
Regards,
Marco
> +
> state->backend_path = cdev_to_devpath(cdev, &offset, &size);
>
> pr_debug("%s: backend resolved to %s\n", node->full_name, state->backend_path);
> diff --git a/include/driver.h b/include/driver.h
> index 6407f7d6ba36..579b03fbac34 100644
> --- a/include/driver.h
> +++ b/include/driver.h
> @@ -585,6 +585,23 @@ extern struct list_head cdev_list;
> #define for_each_cdev(cdev) \
> list_for_each_entry(cdev, &cdev_list, list)
>
> +#define for_each_cdev_partition(partcdev, cdev) \
> + list_for_each_entry((partcdev), &(cdev)->partitions, partition_entry)
> +
> +
> +static inline struct cdev *cdev_find_child_by_typeuuid(struct cdev *cdev,
> + guid_t *typeuuid)
> +{
> + struct cdev *partcdev;
> +
> + for_each_cdev_partition(partcdev, cdev) {
> + if (guid_equal(&partcdev->typeuuid, typeuuid))
> + return partcdev;
> + }
> +
> + return NULL;
> +}
> +
> #define DEVFS_PARTITION_FIXED (1U << 0)
> #define DEVFS_PARTITION_READONLY (1U << 1)
> #define DEVFS_IS_CHARACTER_DEV (1U << 3)
> diff --git a/include/state.h b/include/state.h
> index bffcd5a9007f..3daf82c0735f 100644
> --- a/include/state.h
> +++ b/include/state.h
> @@ -62,4 +62,8 @@ static inline int state_read_mac(struct state *state, const char *name, u8 *buf)
>
> #endif /* #if IS_ENABLED(CONFIG_STATE) / #else */
>
> +#define BAREBOX_STATE_PARTITION_GUID \
> + GUID_INIT(0x4778ed65, 0xbf42, 0x45fa, 0x9c, 0x5b, \
> + 0x28, 0x7a, 0x1d, 0xc4, 0xaa, 0xb1)
> +
> #endif /* __STATE_H */
> --
> 2.39.2
>
>
>
^ permalink raw reply [flat|nested] 62+ messages in thread* Re: [PATCH 18/18] state: allow lookup of barebox state partition by Type GUID
2023-05-31 20:01 ` Marco Felsch
@ 2023-06-01 5:49 ` Ahmad Fatoum
2023-06-01 8:11 ` Marco Felsch
0 siblings, 1 reply; 62+ messages in thread
From: Ahmad Fatoum @ 2023-06-01 5:49 UTC (permalink / raw)
To: Marco Felsch; +Cc: barebox
On 31.05.23 22:01, Marco Felsch wrote:
> Hi Ahmad,
>
> On 23-05-31, Ahmad Fatoum wrote:
>> The backend device tree property so far always pointed at a partition.
>> Let's allow pointing it at GPT storage devices directly and lookup
>> the correct barebox state partition by the well-known type GUID:
>>
>> 4778ed65-bf42-45fa-9c5b-287a1dc4aab1
>
> we should add an example within the Documetation/ too.
I'd wait until we have a new dt-utils release with the same
binding. Anything else would be confusing.
>
>> Signed-off-by: Ahmad Fatoum <a.fatoum@pengutronix.de>
>> ---
>> common/state/state.c | 22 ++++++++++++++++++++++
>> include/driver.h | 17 +++++++++++++++++
>> include/state.h | 4 ++++
>> 3 files changed, 43 insertions(+)
>>
>> diff --git a/common/state/state.c b/common/state/state.c
>> index 88e246198fb8..8f56c60b0e82 100644
>> --- a/common/state/state.c
>> +++ b/common/state/state.c
>> @@ -21,8 +21,10 @@
>> #include <fs.h>
>> #include <crc.h>
>> #include <init.h>
>> +#include <block.h>
>> #include <linux/err.h>
>> #include <linux/list.h>
>> +#include <linux/uuid.h>
>>
>> #include <linux/mtd/mtd-abi.h>
>> #include <malloc.h>
>> @@ -595,6 +597,8 @@ static char *cdev_to_devpath(struct cdev *cdev, off_t *offset, size_t *size)
>> }
>> #endif
>>
>> +static guid_t barebox_state_partition_guid = BAREBOX_STATE_PARTITION_GUID;
>> +
>> /*
>> * state_new_from_node - create a new state instance from a device_node
>> *
>> @@ -641,6 +645,24 @@ struct state *state_new_from_node(struct device_node *node, bool readonly)
>> goto out_release_state;
>> }
>>
>> + /* Is the backend referencing an on-disk partitonable block device? */
>> + if (cdev_is_block_disk(cdev)) {
>> + struct cdev *partcdev = NULL;
>> +
>> + if (cdev_is_gpt_partitioned(cdev))
>> + partcdev = cdev_find_child_by_typeuuid(cdev, &barebox_state_partition_guid);
>> +
>> + if (!partcdev) {
>> + ret = -EINVAL;
>> + goto out_release_state;
>> + }
>> +
>> + pr_debug("%s: backend GPT partition looked up via PartitionTypeGUID\n",
>> + node->full_name);
>> +
>> + cdev = partcdev;
>> + }
>
> What about having the above logic within a seperate function and the
> above code would be:
>
> if (cdev_is_block_disk(cdev))
> cdev = get_cdev_by_typeuuid(cdev, &barebox_state_partition_guid)
>
> if (!cdev) {
> ret = -EINVAL;
> goto out_release_state;
> }
>
> This way we would have everything in place to re-use the same logic for
> the barebox-environmnet too. What do you think?
cdev from of_cdev_find not being available -> -EPROBE_DEFER
cdev GPT partition not existing,despite backend pointing at root device -> -EINVAL
So the cdev check needs to be within if (cdev_is_block_disk(cdev)).
I don't object to combining cdev_is_gpt_partitioned and cdev_find_child_by_typeuuid into
cdev_find_child_by_gpt_typeuuid though.
Thanks for all the review!
Ahmad
>
> Regards,
> Marco
>
>> +
>> state->backend_path = cdev_to_devpath(cdev, &offset, &size);
>>
>> pr_debug("%s: backend resolved to %s\n", node->full_name, state->backend_path);
>> diff --git a/include/driver.h b/include/driver.h
>> index 6407f7d6ba36..579b03fbac34 100644
>> --- a/include/driver.h
>> +++ b/include/driver.h
>> @@ -585,6 +585,23 @@ extern struct list_head cdev_list;
>> #define for_each_cdev(cdev) \
>> list_for_each_entry(cdev, &cdev_list, list)
>>
>> +#define for_each_cdev_partition(partcdev, cdev) \
>> + list_for_each_entry((partcdev), &(cdev)->partitions, partition_entry)
>> +
>> +
>> +static inline struct cdev *cdev_find_child_by_typeuuid(struct cdev *cdev,
>> + guid_t *typeuuid)
>> +{
>> + struct cdev *partcdev;
>> +
>> + for_each_cdev_partition(partcdev, cdev) {
>> + if (guid_equal(&partcdev->typeuuid, typeuuid))
>> + return partcdev;
>> + }
>> +
>> + return NULL;
>> +}
>> +
>> #define DEVFS_PARTITION_FIXED (1U << 0)
>> #define DEVFS_PARTITION_READONLY (1U << 1)
>> #define DEVFS_IS_CHARACTER_DEV (1U << 3)
>> diff --git a/include/state.h b/include/state.h
>> index bffcd5a9007f..3daf82c0735f 100644
>> --- a/include/state.h
>> +++ b/include/state.h
>> @@ -62,4 +62,8 @@ static inline int state_read_mac(struct state *state, const char *name, u8 *buf)
>>
>> #endif /* #if IS_ENABLED(CONFIG_STATE) / #else */
>>
>> +#define BAREBOX_STATE_PARTITION_GUID \
>> + GUID_INIT(0x4778ed65, 0xbf42, 0x45fa, 0x9c, 0x5b, \
>> + 0x28, 0x7a, 0x1d, 0xc4, 0xaa, 0xb1)
>> +
>> #endif /* __STATE_H */
>> --
>> 2.39.2
>>
>>
>>
>
--
Pengutronix e.K. | |
Steuerwalder Str. 21 | http://www.pengutronix.de/ |
31137 Hildesheim, Germany | Phone: +49-5121-206917-0 |
Amtsgericht Hildesheim, HRA 2686 | Fax: +49-5121-206917-5555 |
^ permalink raw reply [flat|nested] 62+ messages in thread* Re: [PATCH 18/18] state: allow lookup of barebox state partition by Type GUID
2023-06-01 5:49 ` Ahmad Fatoum
@ 2023-06-01 8:11 ` Marco Felsch
2023-06-01 10:44 ` Ahmad Fatoum
0 siblings, 1 reply; 62+ messages in thread
From: Marco Felsch @ 2023-06-01 8:11 UTC (permalink / raw)
To: Ahmad Fatoum; +Cc: barebox
On 23-06-01, Ahmad Fatoum wrote:
> On 31.05.23 22:01, Marco Felsch wrote:
> > Hi Ahmad,
> >
> > On 23-05-31, Ahmad Fatoum wrote:
> >> The backend device tree property so far always pointed at a partition.
> >> Let's allow pointing it at GPT storage devices directly and lookup
> >> the correct barebox state partition by the well-known type GUID:
> >>
> >> 4778ed65-bf42-45fa-9c5b-287a1dc4aab1
> >
> > we should add an example within the Documetation/ too.
>
> I'd wait until we have a new dt-utils release with the same
> binding. Anything else would be confusing.
Sure.
> >> Signed-off-by: Ahmad Fatoum <a.fatoum@pengutronix.de>
> >> ---
> >> common/state/state.c | 22 ++++++++++++++++++++++
> >> include/driver.h | 17 +++++++++++++++++
> >> include/state.h | 4 ++++
> >> 3 files changed, 43 insertions(+)
> >>
> >> diff --git a/common/state/state.c b/common/state/state.c
> >> index 88e246198fb8..8f56c60b0e82 100644
> >> --- a/common/state/state.c
> >> +++ b/common/state/state.c
> >> @@ -21,8 +21,10 @@
> >> #include <fs.h>
> >> #include <crc.h>
> >> #include <init.h>
> >> +#include <block.h>
> >> #include <linux/err.h>
> >> #include <linux/list.h>
> >> +#include <linux/uuid.h>
> >>
> >> #include <linux/mtd/mtd-abi.h>
> >> #include <malloc.h>
> >> @@ -595,6 +597,8 @@ static char *cdev_to_devpath(struct cdev *cdev, off_t *offset, size_t *size)
> >> }
> >> #endif
> >>
> >> +static guid_t barebox_state_partition_guid = BAREBOX_STATE_PARTITION_GUID;
> >> +
> >> /*
> >> * state_new_from_node - create a new state instance from a device_node
> >> *
> >> @@ -641,6 +645,24 @@ struct state *state_new_from_node(struct device_node *node, bool readonly)
> >> goto out_release_state;
> >> }
> >>
> >> + /* Is the backend referencing an on-disk partitonable block device? */
> >> + if (cdev_is_block_disk(cdev)) {
> >> + struct cdev *partcdev = NULL;
> >> +
> >> + if (cdev_is_gpt_partitioned(cdev))
> >> + partcdev = cdev_find_child_by_typeuuid(cdev, &barebox_state_partition_guid);
> >> +
> >> + if (!partcdev) {
> >> + ret = -EINVAL;
> >> + goto out_release_state;
> >> + }
> >> +
> >> + pr_debug("%s: backend GPT partition looked up via PartitionTypeGUID\n",
> >> + node->full_name);
> >> +
> >> + cdev = partcdev;
> >> + }
> >
> > What about having the above logic within a seperate function and the
> > above code would be:
> >
> > if (cdev_is_block_disk(cdev))
> > cdev = get_cdev_by_typeuuid(cdev, &barebox_state_partition_guid)
> >
> > if (!cdev) {
> > ret = -EINVAL;
> > goto out_release_state;
> > }
> >
> > This way we would have everything in place to re-use the same logic for
> > the barebox-environmnet too. What do you think?
>
> cdev from of_cdev_find not being available -> -EPROBE_DEFER
This should be checked earlier or do I miss something?
> cdev GPT partition not existing,despite backend pointing at root device -> -EINVAL
So this could follow without..
> So the cdev check needs to be within if (cdev_is_block_disk(cdev)).
having it within the if. The cdev_find_child_by_gpt_typeuuid() could
return NULL in case nothing is found. But that's just a minor nit.
Regards,
Marco
> I don't object to combining cdev_is_gpt_partitioned and cdev_find_child_by_typeuuid into
> cdev_find_child_by_gpt_typeuuid though.
>
>
> Thanks for all the review!
> Ahmad
>
> >
> > Regards,
> > Marco
> >
> >> +
> >> state->backend_path = cdev_to_devpath(cdev, &offset, &size);
> >>
> >> pr_debug("%s: backend resolved to %s\n", node->full_name, state->backend_path);
> >> diff --git a/include/driver.h b/include/driver.h
> >> index 6407f7d6ba36..579b03fbac34 100644
> >> --- a/include/driver.h
> >> +++ b/include/driver.h
> >> @@ -585,6 +585,23 @@ extern struct list_head cdev_list;
> >> #define for_each_cdev(cdev) \
> >> list_for_each_entry(cdev, &cdev_list, list)
> >>
> >> +#define for_each_cdev_partition(partcdev, cdev) \
> >> + list_for_each_entry((partcdev), &(cdev)->partitions, partition_entry)
> >> +
> >> +
> >> +static inline struct cdev *cdev_find_child_by_typeuuid(struct cdev *cdev,
> >> + guid_t *typeuuid)
> >> +{
> >> + struct cdev *partcdev;
> >> +
> >> + for_each_cdev_partition(partcdev, cdev) {
> >> + if (guid_equal(&partcdev->typeuuid, typeuuid))
> >> + return partcdev;
> >> + }
> >> +
> >> + return NULL;
> >> +}
> >> +
> >> #define DEVFS_PARTITION_FIXED (1U << 0)
> >> #define DEVFS_PARTITION_READONLY (1U << 1)
> >> #define DEVFS_IS_CHARACTER_DEV (1U << 3)
> >> diff --git a/include/state.h b/include/state.h
> >> index bffcd5a9007f..3daf82c0735f 100644
> >> --- a/include/state.h
> >> +++ b/include/state.h
> >> @@ -62,4 +62,8 @@ static inline int state_read_mac(struct state *state, const char *name, u8 *buf)
> >>
> >> #endif /* #if IS_ENABLED(CONFIG_STATE) / #else */
> >>
> >> +#define BAREBOX_STATE_PARTITION_GUID \
> >> + GUID_INIT(0x4778ed65, 0xbf42, 0x45fa, 0x9c, 0x5b, \
> >> + 0x28, 0x7a, 0x1d, 0xc4, 0xaa, 0xb1)
> >> +
> >> #endif /* __STATE_H */
> >> --
> >> 2.39.2
> >>
> >>
> >>
> >
>
> --
> Pengutronix e.K. | |
> Steuerwalder Str. 21 | http://www.pengutronix.de/ |
> 31137 Hildesheim, Germany | Phone: +49-5121-206917-0 |
> Amtsgericht Hildesheim, HRA 2686 | Fax: +49-5121-206917-5555 |
>
>
^ permalink raw reply [flat|nested] 62+ messages in thread* Re: [PATCH 18/18] state: allow lookup of barebox state partition by Type GUID
2023-06-01 8:11 ` Marco Felsch
@ 2023-06-01 10:44 ` Ahmad Fatoum
0 siblings, 0 replies; 62+ messages in thread
From: Ahmad Fatoum @ 2023-06-01 10:44 UTC (permalink / raw)
To: Marco Felsch; +Cc: barebox
On 01.06.23 10:11, Marco Felsch wrote:
> On 23-06-01, Ahmad Fatoum wrote:
>> On 31.05.23 22:01, Marco Felsch wrote:
>>> Hi Ahmad,
>>>
>>> On 23-05-31, Ahmad Fatoum wrote:
>>>> The backend device tree property so far always pointed at a partition.
>>>> Let's allow pointing it at GPT storage devices directly and lookup
>>>> the correct barebox state partition by the well-known type GUID:
>>>>
>>>> 4778ed65-bf42-45fa-9c5b-287a1dc4aab1
>>>
>>> we should add an example within the Documetation/ too.
>>
>> I'd wait until we have a new dt-utils release with the same
>> binding. Anything else would be confusing.
>
> Sure.
>
>>>> Signed-off-by: Ahmad Fatoum <a.fatoum@pengutronix.de>
>>>> ---
>>>> common/state/state.c | 22 ++++++++++++++++++++++
>>>> include/driver.h | 17 +++++++++++++++++
>>>> include/state.h | 4 ++++
>>>> 3 files changed, 43 insertions(+)
>>>>
>>>> diff --git a/common/state/state.c b/common/state/state.c
>>>> index 88e246198fb8..8f56c60b0e82 100644
>>>> --- a/common/state/state.c
>>>> +++ b/common/state/state.c
>>>> @@ -21,8 +21,10 @@
>>>> #include <fs.h>
>>>> #include <crc.h>
>>>> #include <init.h>
>>>> +#include <block.h>
>>>> #include <linux/err.h>
>>>> #include <linux/list.h>
>>>> +#include <linux/uuid.h>
>>>>
>>>> #include <linux/mtd/mtd-abi.h>
>>>> #include <malloc.h>
>>>> @@ -595,6 +597,8 @@ static char *cdev_to_devpath(struct cdev *cdev, off_t *offset, size_t *size)
>>>> }
>>>> #endif
>>>>
>>>> +static guid_t barebox_state_partition_guid = BAREBOX_STATE_PARTITION_GUID;
>>>> +
>>>> /*
>>>> * state_new_from_node - create a new state instance from a device_node
>>>> *
>>>> @@ -641,6 +645,24 @@ struct state *state_new_from_node(struct device_node *node, bool readonly)
>>>> goto out_release_state;
>>>> }
>>>>
>>>> + /* Is the backend referencing an on-disk partitonable block device? */
>>>> + if (cdev_is_block_disk(cdev)) {
>>>> + struct cdev *partcdev = NULL;
>>>> +
>>>> + if (cdev_is_gpt_partitioned(cdev))
>>>> + partcdev = cdev_find_child_by_typeuuid(cdev, &barebox_state_partition_guid);
>>>> +
>>>> + if (!partcdev) {
>>>> + ret = -EINVAL;
>>>> + goto out_release_state;
>>>> + }
>>>> +
>>>> + pr_debug("%s: backend GPT partition looked up via PartitionTypeGUID\n",
>>>> + node->full_name);
>>>> +
>>>> + cdev = partcdev;
>>>> + }
>>>
>>> What about having the above logic within a seperate function and the
>>> above code would be:
>>>
>>> if (cdev_is_block_disk(cdev))
>>> cdev = get_cdev_by_typeuuid(cdev, &barebox_state_partition_guid)
>>>
>>> if (!cdev) {
>>> ret = -EINVAL;
>>> goto out_release_state;
>>> }
>>>
>>> This way we would have everything in place to re-use the same logic for
>>> the barebox-environmnet too. What do you think?
>>
>> cdev from of_cdev_find not being available -> -EPROBE_DEFER
>
> This should be checked earlier or do I miss something?
I don't think it aids readability to have constructs like:
if (ret)
return ret;
if (check())
ret = do_foo();
if (ret)
return ret;
I want to print a debug message anyway, so I'd just keep if (ret) inside.
>
>> cdev GPT partition not existing,despite backend pointing at root device -> -EINVAL
>
> So this could follow without..
>
>> So the cdev check needs to be within if (cdev_is_block_disk(cdev)).
>
> having it within the if. The cdev_find_child_by_gpt_typeuuid() could
> return NULL in case nothing is found. But that's just a minor nit.
I want it to return errors codes. barebox-state doesn't care and would
return -EINVAL on IS_ERR unconditionally, but it may be useful to future
users to differentiate between disk is not gpt and disk is gpt but device
doesn't exist.
>
> Regards,
> Marco
>
>> I don't object to combining cdev_is_gpt_partitioned and cdev_find_child_by_typeuuid into
>> cdev_find_child_by_gpt_typeuuid though.
>>
>>
>> Thanks for all the review!
>> Ahmad
>>
>>>
>>> Regards,
>>> Marco
>>>
>>>> +
>>>> state->backend_path = cdev_to_devpath(cdev, &offset, &size);
>>>>
>>>> pr_debug("%s: backend resolved to %s\n", node->full_name, state->backend_path);
>>>> diff --git a/include/driver.h b/include/driver.h
>>>> index 6407f7d6ba36..579b03fbac34 100644
>>>> --- a/include/driver.h
>>>> +++ b/include/driver.h
>>>> @@ -585,6 +585,23 @@ extern struct list_head cdev_list;
>>>> #define for_each_cdev(cdev) \
>>>> list_for_each_entry(cdev, &cdev_list, list)
>>>>
>>>> +#define for_each_cdev_partition(partcdev, cdev) \
>>>> + list_for_each_entry((partcdev), &(cdev)->partitions, partition_entry)
>>>> +
>>>> +
>>>> +static inline struct cdev *cdev_find_child_by_typeuuid(struct cdev *cdev,
>>>> + guid_t *typeuuid)
>>>> +{
>>>> + struct cdev *partcdev;
>>>> +
>>>> + for_each_cdev_partition(partcdev, cdev) {
>>>> + if (guid_equal(&partcdev->typeuuid, typeuuid))
>>>> + return partcdev;
>>>> + }
>>>> +
>>>> + return NULL;
>>>> +}
>>>> +
>>>> #define DEVFS_PARTITION_FIXED (1U << 0)
>>>> #define DEVFS_PARTITION_READONLY (1U << 1)
>>>> #define DEVFS_IS_CHARACTER_DEV (1U << 3)
>>>> diff --git a/include/state.h b/include/state.h
>>>> index bffcd5a9007f..3daf82c0735f 100644
>>>> --- a/include/state.h
>>>> +++ b/include/state.h
>>>> @@ -62,4 +62,8 @@ static inline int state_read_mac(struct state *state, const char *name, u8 *buf)
>>>>
>>>> #endif /* #if IS_ENABLED(CONFIG_STATE) / #else */
>>>>
>>>> +#define BAREBOX_STATE_PARTITION_GUID \
>>>> + GUID_INIT(0x4778ed65, 0xbf42, 0x45fa, 0x9c, 0x5b, \
>>>> + 0x28, 0x7a, 0x1d, 0xc4, 0xaa, 0xb1)
>>>> +
>>>> #endif /* __STATE_H */
>>>> --
>>>> 2.39.2
>>>>
>>>>
>>>>
>>>
>>
>> --
>> Pengutronix e.K. | |
>> Steuerwalder Str. 21 | http://www.pengutronix.de/ |
>> 31137 Hildesheim, Germany | Phone: +49-5121-206917-0 |
>> Amtsgericht Hildesheim, HRA 2686 | Fax: +49-5121-206917-5555 |
>>
>>
>
--
Pengutronix e.K. | |
Steuerwalder Str. 21 | http://www.pengutronix.de/ |
31137 Hildesheim, Germany | Phone: +49-5121-206917-0 |
Amtsgericht Hildesheim, HRA 2686 | Fax: +49-5121-206917-5555 |
^ permalink raw reply [flat|nested] 62+ messages in thread
* Re: [PATCH 18/18] state: allow lookup of barebox state partition by Type GUID
2023-05-31 14:59 ` [PATCH 18/18] state: allow lookup of barebox state partition by Type GUID Ahmad Fatoum
2023-05-31 20:01 ` Marco Felsch
@ 2023-06-01 8:05 ` Sascha Hauer
1 sibling, 0 replies; 62+ messages in thread
From: Sascha Hauer @ 2023-06-01 8:05 UTC (permalink / raw)
To: Ahmad Fatoum; +Cc: barebox
On Wed, May 31, 2023 at 04:59:27PM +0200, Ahmad Fatoum wrote:
> The backend device tree property so far always pointed at a partition.
> Let's allow pointing it at GPT storage devices directly and lookup
> the correct barebox state partition by the well-known type GUID:
>
> 4778ed65-bf42-45fa-9c5b-287a1dc4aab1
>
> Signed-off-by: Ahmad Fatoum <a.fatoum@pengutronix.de>
> ---
> common/state/state.c | 22 ++++++++++++++++++++++
> include/driver.h | 17 +++++++++++++++++
> include/state.h | 4 ++++
> 3 files changed, 43 insertions(+)
>
> diff --git a/common/state/state.c b/common/state/state.c
> index 88e246198fb8..8f56c60b0e82 100644
> --- a/common/state/state.c
> +++ b/common/state/state.c
> @@ -21,8 +21,10 @@
> #include <fs.h>
> #include <crc.h>
> #include <init.h>
> +#include <block.h>
> #include <linux/err.h>
> #include <linux/list.h>
> +#include <linux/uuid.h>
>
> #include <linux/mtd/mtd-abi.h>
> #include <malloc.h>
> @@ -595,6 +597,8 @@ static char *cdev_to_devpath(struct cdev *cdev, off_t *offset, size_t *size)
> }
> #endif
>
> +static guid_t barebox_state_partition_guid = BAREBOX_STATE_PARTITION_GUID;
> +
> /*
> * state_new_from_node - create a new state instance from a device_node
> *
> @@ -641,6 +645,24 @@ struct state *state_new_from_node(struct device_node *node, bool readonly)
> goto out_release_state;
> }
>
> + /* Is the backend referencing an on-disk partitonable block device? */
s/partitonable/partitionable/
Sascha
--
Pengutronix e.K. | |
Steuerwalder Str. 21 | http://www.pengutronix.de/ |
31137 Hildesheim, Germany | Phone: +49-5121-206917-0 |
Amtsgericht Hildesheim, HRA 2686 | Fax: +49-5121-206917-5555 |
^ permalink raw reply [flat|nested] 62+ messages in thread