[PATCH 0/8] Barebox-State on-disk partition support

[PATCH 0/8] Barebox-State on-disk partition support

[Marco Felsch]

Hi,

this small series adds the support to store the barebox-state on a
on-disk partition like mbr/gpt. Note for testing you need a very recent
dt-utils package. I will send the patches soon as well and will add a
link to the patches here later.

Regards,
  Marco

Marco Felsch (8):
  of: of_node_name_eq: correct alignment
  state: select the STATE_DRV when STATE is selected
  state: rename partition_node to backend_node
  state: cosmetic fix reverse christmas tree order
  state: rename backend members
  state: factor out the backend property parsing
  of: partition: add a helper to determin if a node is a of-partition
  state: add support for new backend format

 .../bindings/barebox/barebox,state.rst        |  30 +++-
 Documentation/user/state.rst                  |  12 +-
 common/Kconfig                                |   1 +
 common/state/state.c                          | 170 +++++++++++++-----
 common/state/state.h                          |   6 +-
 drivers/of/base.c                             |   2 +-
 drivers/of/partition.c                        |  12 ++
 include/of.h                                  |   6 +
 8 files changed, 181 insertions(+), 58 deletions(-)

-- 
2.30.2

[PATCH 1/8] of: of_node_name_eq: correct alignment

[Marco Felsch]

Just a cleanup nothing special.

Signed-off-by: Marco Felsch <m.felsch@pengutronix.de>
---
 drivers/of/base.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/of/base.c b/drivers/of/base.c
index ea2a88764b..2eee1279a9 100644
--- a/drivers/of/base.c
+++ b/drivers/of/base.c
@@ -32,7 +32,7 @@ bool of_node_name_eq(const struct device_node *np, const char *name)
 		return false;
 
 	node_name = kbasename(np->full_name);
-		len = strchrnul(node_name, '@') - node_name;
+	len = strchrnul(node_name, '@') - node_name;
 
 	return (strlen(name) == len) && (strncmp(node_name, name, len) == 0);
 }
-- 
2.30.2

Re: [PATCH 1/8] of: of_node_name_eq: correct alignment

[Sascha Hauer]

On Fri, Oct 14, 2022 at 06:35:27PM +0200, Marco Felsch wrote:
> Just a cleanup nothing special.
> 
> Signed-off-by: Marco Felsch <m.felsch@pengutronix.de>
> ---
>  drivers/of/base.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/drivers/of/base.c b/drivers/of/base.c
> index ea2a88764b..2eee1279a9 100644
> --- a/drivers/of/base.c
> +++ b/drivers/of/base.c
> @@ -32,7 +32,7 @@ bool of_node_name_eq(const struct device_node *np, const char *name)
>  		return false;
>  
>  	node_name = kbasename(np->full_name);
> -		len = strchrnul(node_name, '@') - node_name;
> +	len = strchrnul(node_name, '@') - node_name;
>  
>  	return (strlen(name) == len) && (strncmp(node_name, name, len) == 0);

Applied (only this patch), thanks

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 |

[PATCH 2/8] state: select the STATE_DRV when STATE is selected

[Marco Felsch]

As written in the state.rst documentation, state will silently fail if
the STATE_DRV is not selected. So enabling state without the state
driver is useless. Fix this by selecting the STATE_DRV if STATE is
selected.

Signed-off-by: Marco Felsch <m.felsch@pengutronix.de>
---
 common/Kconfig | 1 +
 1 file changed, 1 insertion(+)

diff --git a/common/Kconfig b/common/Kconfig
index fb2bf49683..4654a5cfaf 100644
--- a/common/Kconfig
+++ b/common/Kconfig
@@ -1016,6 +1016,7 @@ config STATE
 	select ENVIRONMENT_VARIABLES
 	select OFTREE
 	select PARAMETER
+	select STATE_DRV
 	help
 	  barebox state is a generic framework for atomic power fail-safe
 	  variable storage and retrieval. It can be used to safely maintain
-- 
2.30.2

Re: [PATCH 2/8] state: select the STATE_DRV when STATE is selected

[Sascha Hauer]

On Fri, Oct 14, 2022 at 06:35:28PM +0200, Marco Felsch wrote:
> As written in the state.rst documentation, state will silently fail if
> the STATE_DRV is not selected. So enabling state without the state
> driver is useless. Fix this by selecting the STATE_DRV if STATE is
> selected.
> 
> Signed-off-by: Marco Felsch <m.felsch@pengutronix.de>
> ---
>  common/Kconfig | 1 +
>  1 file changed, 1 insertion(+)
> 
> diff --git a/common/Kconfig b/common/Kconfig
> index fb2bf49683..4654a5cfaf 100644
> --- a/common/Kconfig
> +++ b/common/Kconfig
> @@ -1016,6 +1016,7 @@ config STATE
>  	select ENVIRONMENT_VARIABLES
>  	select OFTREE
>  	select PARAMETER
> +	select STATE_DRV

STATE_DRV depends on OFDEVICE, so that should be selected here as well.

I am thinking about moving the state driver and the common state stuff
together into a single directory like drivers/state/. That would make it
more obvious that these belong together for both developers and users.

What do you think?

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 |

Re: [PATCH 2/8] state: select the STATE_DRV when STATE is selected

[Marco Felsch]

On 22-10-19, Sascha Hauer wrote:
> On Fri, Oct 14, 2022 at 06:35:28PM +0200, Marco Felsch wrote:
> > As written in the state.rst documentation, state will silently fail if
> > the STATE_DRV is not selected. So enabling state without the state
> > driver is useless. Fix this by selecting the STATE_DRV if STATE is
> > selected.
> > 
> > Signed-off-by: Marco Felsch <m.felsch@pengutronix.de>
> > ---
> >  common/Kconfig | 1 +
> >  1 file changed, 1 insertion(+)
> > 
> > diff --git a/common/Kconfig b/common/Kconfig
> > index fb2bf49683..4654a5cfaf 100644
> > --- a/common/Kconfig
> > +++ b/common/Kconfig
> > @@ -1016,6 +1016,7 @@ config STATE
> >  	select ENVIRONMENT_VARIABLES
> >  	select OFTREE
> >  	select PARAMETER
> > +	select STATE_DRV
> 
> STATE_DRV depends on OFDEVICE, so that should be selected here as well.

Arg.. didn't checked that.

> I am thinking about moving the state driver and the common state stuff
> together into a single directory like drivers/state/. That would make it
> more obvious that these belong together for both developers and users.
> 
> What do you think?

+1 from my side.

Regards,
  Marco

[PATCH 3/8] state: rename partition_node to backend_node

[Marco Felsch]

Rename the local variable in preparation of addding support for backends
stored within a partition table. This also aligns the name with the name
used by of_state_fixup().

No functional change.

Signed-off-by: Marco Felsch <m.felsch@pengutronix.de>
---
 common/state/state.c | 14 +++++++-------
 1 file changed, 7 insertions(+), 7 deletions(-)

diff --git a/common/state/state.c b/common/state/state.c
index a614c849c7..d954f0d453 100644
--- a/common/state/state.c
+++ b/common/state/state.c
@@ -593,7 +593,7 @@ struct state *state_new_from_node(struct device_node *node, bool readonly)
 	const char *storage_type = NULL;
 	const char *alias;
 	uint32_t stridesize;
-	struct device_node *partition_node;
+	struct device_node *backend_node;
 	off_t offset = 0;
 	size_t size = 0;
 
@@ -607,21 +607,21 @@ struct state *state_new_from_node(struct device_node *node, bool readonly)
 	if (IS_ERR(state))
 		return state;
 
-	partition_node = of_parse_phandle(node, "backend", 0);
-	if (!partition_node) {
+	backend_node = of_parse_phandle(node, "backend", 0);
+	if (!backend_node) {
 		dev_err(&state->dev, "Cannot resolve \"backend\" phandle\n");
 		ret = -EINVAL;
 		goto out_release_state;
 	}
 
 #ifdef __BAREBOX__
-	ret = of_partition_ensure_probed(partition_node);
+	ret = of_partition_ensure_probed(backend_node);
 	if (ret)
 		goto out_release_state;
 
-	ret = of_find_path_by_node(partition_node, &state->backend_path, 0);
+	ret = of_find_path_by_node(backend_node, &state->backend_path, 0);
 #else
-	ret = of_get_devicepath(partition_node, &state->backend_path, &offset, &size);
+	ret = of_get_devicepath(backend_node, &state->backend_path, &offset, &size);
 #endif
 	if (ret) {
 		if (ret != -EPROBE_DEFER)
@@ -630,7 +630,7 @@ struct state *state_new_from_node(struct device_node *node, bool readonly)
 		goto out_release_state;
 	}
 
-	state->backend_reproducible_name = of_get_reproducible_name(partition_node);
+	state->backend_reproducible_name = of_get_reproducible_name(backend_node);
 
 	ret = of_property_read_string(node, "backend-type", &backend_type);
 	if (ret) {
-- 
2.30.2

[PATCH 4/8] state: cosmetic fix reverse christmas tree order

[Marco Felsch]

No functional change, just apply the common code standards.

Signed-off-by: Marco Felsch <m.felsch@pengutronix.de>
---
 common/state/state.c | 32 ++++++++++++++++----------------
 1 file changed, 16 insertions(+), 16 deletions(-)

diff --git a/common/state/state.c b/common/state/state.c
index d954f0d453..65e47524a3 100644
--- a/common/state/state.c
+++ b/common/state/state.c
@@ -41,11 +41,11 @@ static LIST_HEAD(state_list);
  */
 int state_save(struct state *state)
 {
-	void *buf;
-	ssize_t len;
-	int ret;
 	struct state_backend_storage_bucket *bucket;
 	struct state_backend_storage *storage;
+	ssize_t len;
+	void *buf;
+	int ret;
 
 	if (!state->dirty)
 		return 0;
@@ -212,13 +212,13 @@ static int state_convert_node_variable(struct state *state,
 				       const char *parent_name,
 				       enum state_convert conv)
 {
+	struct device_node *new_node = NULL;
 	const struct variable_type *vtype;
+	char *short_name, *name, *indexs;
+	unsigned int start_size[2];
 	struct device_node *child;
-	struct device_node *new_node = NULL;
 	struct state_variable *sv;
 	const char *type_name;
-	char *short_name, *name, *indexs;
-	unsigned int start_size[2];
 	int ret;
 
 	/* strip trailing @<ADDRESS> */
@@ -341,8 +341,8 @@ struct device_node *state_to_node(struct state *state,
 				  struct device_node *parent,
 				  enum state_convert conv)
 {
-	struct device_node *child;
 	struct device_node *root, *state_root;
+	struct device_node *child;
 	int ret;
 
 	state_root = of_find_node_by_path(state->of_path);
@@ -369,8 +369,8 @@ int state_from_node(struct state *state, struct device_node *node, bool create)
 {
 	struct device_node *child;
 	enum state_convert conv;
-	int ret;
 	uint32_t magic;
+	int ret;
 
 	ret = of_property_read_u32(node, "magic", &magic);
 	if (ret)
@@ -428,12 +428,12 @@ int state_from_node(struct state *state, struct device_node *node, bool create)
 
 static int of_state_fixup(struct device_node *root, void *ctx)
 {
-	struct state *state = ctx;
-	const char *compatible = "barebox,state";
 	struct device_node *new_node, *node, *parent, *backend_node, *aliases;
+	const char *compatible = "barebox,state";
+	struct state *state = ctx;
 	struct property *p;
-	int ret;
 	phandle phandle;
+	int ret;
 
 	node = of_find_node_by_path_from(root, state->of_path);
 	if (node) {
@@ -587,15 +587,15 @@ void state_release(struct state *state)
  */
 struct state *state_new_from_node(struct device_node *node, bool readonly)
 {
-	struct state *state;
-	int ret = 0;
-	const char *backend_type;
+	struct device_node *backend_node;
 	const char *storage_type = NULL;
-	const char *alias;
+	const char *backend_type;
+	struct state *state;
 	uint32_t stridesize;
-	struct device_node *backend_node;
+	const char *alias;
 	off_t offset = 0;
 	size_t size = 0;
+	int ret = 0;
 
 	alias = of_alias_get(node);
 	if (!alias) {
-- 
2.30.2

Re: [PATCH 4/8] state: cosmetic fix reverse christmas tree order

[Sascha Hauer]

On Fri, Oct 14, 2022 at 06:35:30PM +0200, Marco Felsch wrote:
> No functional change, just apply the common code standards.

I didn't know these are common code standards for barebox ;)

Also I really don't like such things very much. The following forces you
to violate this rule:

	struct priv *priv = to_priv(common_type);
	struct myfoo = &priv->some_special_thingy_inside_priv;

>  		return 0;
> @@ -212,13 +212,13 @@ static int state_convert_node_variable(struct state *state,
>  				       const char *parent_name,
>  				       enum state_convert conv)
>  {
> +	struct device_node *new_node = NULL;
>  	const struct variable_type *vtype;
> +	char *short_name, *name, *indexs;

If you add another char * type variable or remove one of those you
either violate the ordering or introduce another useless churn.

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 |

Re: [PATCH 4/8] state: cosmetic fix reverse christmas tree order

[Marco Felsch]

On 22-10-19, Sascha Hauer wrote:
> On Fri, Oct 14, 2022 at 06:35:30PM +0200, Marco Felsch wrote:
> > No functional change, just apply the common code standards.
> 
> I didn't know these are common code standards for barebox ;)

I thought that we are following the Linux way. At least there it is
common to follow this.

> Also I really don't like such things very much. The following forces you
> to violate this rule:
> 
> 	struct priv *priv = to_priv(common_type);
> 	struct myfoo = &priv->some_special_thingy_inside_priv;

Of course, in such cases we can't follow them.

> >  		return 0;
> > @@ -212,13 +212,13 @@ static int state_convert_node_variable(struct state *state,
> >  				       const char *parent_name,
> >  				       enum state_convert conv)
> >  {
> > +	struct device_node *new_node = NULL;
> >  	const struct variable_type *vtype;
> > +	char *short_name, *name, *indexs;
> 
> If you add another char * type variable or remove one of those you
> either violate the ordering or introduce another useless churn.

Got it, I will drop it if you don't like it.

Regards,
  Marco

[PATCH 5/8] state: rename backend members

[Marco Felsch]

Rename backend members to drop the assumption that the backend always
points to a partition. This is preperation work for the upcoming
state partition table support which allows us to store the state within
a MBR/GPT partition.

Signed-off-by: Marco Felsch <m.felsch@pengutronix.de>
---
 common/state/state.c | 17 +++++++++--------
 common/state/state.h |  4 ++--
 2 files changed, 11 insertions(+), 10 deletions(-)

diff --git a/common/state/state.c b/common/state/state.c
index 65e47524a3..6b8b701ba0 100644
--- a/common/state/state.c
+++ b/common/state/state.c
@@ -493,7 +493,7 @@ static int of_state_fixup(struct device_node *root, void *ctx)
 
 	/* backend phandle */
 	backend_node = of_find_node_by_reproducible_name(root,
-						state->backend_reproducible_name);
+						state->backend_dts_dev_or_part);
 	if (!backend_node) {
 		ret = -ENODEV;
 		goto out;
@@ -572,8 +572,8 @@ void state_release(struct state *state)
 	unregister_device(&state->dev);
 	state_storage_free(&state->storage);
 	state_format_free(state->format);
-	free(state->backend_path);
-	free(state->backend_reproducible_name);
+	free(state->backend_dev_path);
+	free(state->backend_dts_dev_or_part);
 	free(state->of_path);
 	free(state);
 }
@@ -619,10 +619,11 @@ struct state *state_new_from_node(struct device_node *node, bool readonly)
 	if (ret)
 		goto out_release_state;
 
-	ret = of_find_path_by_node(backend_node, &state->backend_path, 0);
+	ret = of_find_path_by_node(backend_node, &state->backend_dev_path, 0);
 #else
-	ret = of_get_devicepath(backend_node, &state->backend_path, &offset, &size);
+	ret = of_get_devicepath(backend_node, &state->backend_dev_path, &offset, &size);
 #endif
+
 	if (ret) {
 		if (ret != -EPROBE_DEFER)
 			dev_err(&state->dev, "state failed to parse path to backend: %s\n",
@@ -630,7 +631,7 @@ struct state *state_new_from_node(struct device_node *node, bool readonly)
 		goto out_release_state;
 	}
 
-	state->backend_reproducible_name = of_get_reproducible_name(backend_node);
+	state->backend_dts_dev_or_part = of_get_reproducible_name(backend_node);
 
 	ret = of_property_read_string(node, "backend-type", &backend_type);
 	if (ret) {
@@ -653,7 +654,7 @@ struct state *state_new_from_node(struct device_node *node, bool readonly)
 	if (ret)
 		goto out_release_state;
 
-	ret = state_storage_init(state, state->backend_path, offset,
+	ret = state_storage_init(state, state->backend_dev_path, offset,
 				 size, stridesize, storage_type);
 	if (ret)
 		goto out_release_state;
@@ -766,7 +767,7 @@ void state_info(void)
 		if (state->format)
 			printf("(backend: %s, path: %s)\n",
 			       state->format->name,
-			       state->backend_path);
+			       state->backend_dev_path);
 		else
 			printf("(no backend)\n");
 	}
diff --git a/common/state/state.h b/common/state/state.h
index 0545cf6ac1..83a7fae4a5 100644
--- a/common/state/state.h
+++ b/common/state/state.h
@@ -119,8 +119,8 @@ struct state {
 
 	struct state_backend_format *format;
 	struct state_backend_storage storage;
-	char *backend_path;
-	char *backend_reproducible_name;
+	char *backend_dev_path;
+	char *backend_dts_dev_or_part;
 };
 
 enum state_convert {
-- 
2.30.2

[PATCH 6/8] state: factor out the backend property parsing

[Marco Felsch]

Move the backend parsing into a own subfunction. This is in preperation
of addding support for a new format which differs from the old one.

While on it introduce the state_of_find_path_by_node() wrapper define
which handles the different compile options: barebox build vs. dt-utils
build which makes the code easier to read. Also remove the unnecessary
ret initialization.

Signed-off-by: Marco Felsch <m.felsch@pengutronix.de>
---
 common/state/state.c | 55 ++++++++++++++++++++++++++++++--------------
 1 file changed, 38 insertions(+), 17 deletions(-)

diff --git a/common/state/state.c b/common/state/state.c
index 6b8b701ba0..cee83ebde4 100644
--- a/common/state/state.c
+++ b/common/state/state.c
@@ -578,6 +578,42 @@ void state_release(struct state *state)
 	free(state);
 }
 
+#ifdef __BAREBOX__
+
+#define state_of_find_path_by_node(node, path, flags, offset, size) \
+	of_find_path_by_node(node, path, flags)
+
+#else
+
+#define state_of_find_path_by_node(node, path, flags, offset, size) \
+	of_get_devicepath(node, path, offset, size)
+
+#endif /* __BAREBOX__ */
+
+static int
+state_parse_old_backend_format(struct device_node *backend_node,
+			       struct state *state, off_t *offset, size_t *size)
+{
+	int ret;
+
+	ret = of_partition_ensure_probed(backend_node);
+	if (ret)
+		return ret;
+
+	ret = state_of_find_path_by_node(backend_node, &state->backend_dev_path,
+					 0, offset, size);
+	if (ret) {
+		if (ret != -EPROBE_DEFER)
+			dev_err(&state->dev, "state failed to parse path to backend: %s\n",
+			       strerror(-ret));
+		return ret;
+	}
+
+	state->backend_dts_dev_or_part = of_get_reproducible_name(backend_node);
+
+	return 0;
+}
+
 /*
  * state_new_from_node - create a new state instance from a device_node
  *
@@ -595,7 +631,7 @@ struct state *state_new_from_node(struct device_node *node, bool readonly)
 	const char *alias;
 	off_t offset = 0;
 	size_t size = 0;
-	int ret = 0;
+	int ret;
 
 	alias = of_alias_get(node);
 	if (!alias) {
@@ -614,25 +650,10 @@ struct state *state_new_from_node(struct device_node *node, bool readonly)
 		goto out_release_state;
 	}
 
-#ifdef __BAREBOX__
-	ret = of_partition_ensure_probed(backend_node);
+	ret = state_parse_old_backend_format(backend_node, state, &offset, &size);
 	if (ret)
 		goto out_release_state;
 
-	ret = of_find_path_by_node(backend_node, &state->backend_dev_path, 0);
-#else
-	ret = of_get_devicepath(backend_node, &state->backend_dev_path, &offset, &size);
-#endif
-
-	if (ret) {
-		if (ret != -EPROBE_DEFER)
-			dev_err(&state->dev, "state failed to parse path to backend: %s\n",
-			       strerror(-ret));
-		goto out_release_state;
-	}
-
-	state->backend_dts_dev_or_part = of_get_reproducible_name(backend_node);
-
 	ret = of_property_read_string(node, "backend-type", &backend_type);
 	if (ret) {
 		dev_dbg(&state->dev, "Missing 'backend-type' property\n");
-- 
2.30.2

[PATCH 7/8] of: partition: add a helper to determin if a node is a of-partition

[Marco Felsch]

This helper checks if the given device_node is a of-partition. The check
is based on the node name which should something like partition@xxx. If
that fails the we try to check if the parent node contains a
"fixed-partitions" compatible.

Signed-off-by: Marco Felsch <m.felsch@pengutronix.de>
---
 drivers/of/partition.c | 12 ++++++++++++
 include/of.h           |  6 ++++++
 2 files changed, 18 insertions(+)

diff --git a/drivers/of/partition.c b/drivers/of/partition.c
index abbda674d6..494fd92660 100644
--- a/drivers/of/partition.c
+++ b/drivers/of/partition.c
@@ -107,6 +107,18 @@ int of_parse_partitions(struct cdev *cdev, struct device_node *node)
 	return 0;
 }
 
+int is_of_partition(struct device_node *np)
+{
+	if (of_node_name_eq(np, "partition"))
+		return 1;
+
+	np = of_get_parent(np);
+	if (of_device_is_compatible(np, "fixed-partitions"))
+		return 1;
+
+	return 0;
+}
+
 int of_partition_ensure_probed(struct device_node *np)
 {
 	np = of_get_parent(np);
diff --git a/include/of.h b/include/of.h
index 052d5fcad8..d6a3e75f86 100644
--- a/include/of.h
+++ b/include/of.h
@@ -309,6 +309,7 @@ struct cdev *of_parse_partition(struct cdev *cdev, struct device_node *node);
 int of_parse_partitions(struct cdev *cdev, struct device_node *node);
 int of_fixup_partitions(struct device_node *np, struct cdev *cdev);
 int of_partitions_register_fixup(struct cdev *cdev);
+int is_of_partition(struct device_node *np);
 struct device_node *of_get_stdoutpath(unsigned int *);
 int of_device_is_stdout_path(struct device_d *dev, unsigned int *baudrate);
 const char *of_get_model(void);
@@ -359,6 +360,11 @@ static inline int of_partitions_register_fixup(struct cdev *cdev)
 	return -ENOSYS;
 }
 
+static inline int is_of_partition(struct device_node *np)
+{
+	return -ENOSYS;
+}
+
 static inline struct device_node *of_get_stdoutpath(unsigned int *rate)
 {
 	return NULL;
-- 
2.30.2

[PATCH 8/8] state: add support for new backend format

[Marco Felsch]

The backend can now be specified by either:

  backend = <&of_partition>;

or by using the path binding:

  backend = &mmc2, "partname:2";

  backend = &of_partition;

This allows us to store the state within a partition table like GPT or
MBR. So the days of magic offsets within the partitions table are gone.
For this feature a recent dt-utils package must be used, else you can't
manipulate the state from user-space anymore.

Signed-off-by: Marco Felsch <m.felsch@pengutronix.de>
---
 .../bindings/barebox/barebox,state.rst        | 30 ++++++-
 Documentation/user/state.rst                  | 12 ++-
 common/state/state.c                          | 78 ++++++++++++++++---
 common/state/state.h                          |  2 +
 4 files changed, 102 insertions(+), 20 deletions(-)

diff --git a/Documentation/devicetree/bindings/barebox/barebox,state.rst b/Documentation/devicetree/bindings/barebox/barebox,state.rst
index 390e148a28..ef1c0b418c 100644
--- a/Documentation/devicetree/bindings/barebox/barebox,state.rst
+++ b/Documentation/devicetree/bindings/barebox/barebox,state.rst
@@ -18,7 +18,7 @@ Required Properties
 
 * ``compatible``: should be ``barebox,state``
 * ``magic``: a 32bit number
-* ``backend``: phandle to persistent memory
+* ``backend``: points to the storage backend please see below
 * ``backend-type``: defines the *state* variable set storage format
 * additionally a *state* node must have an alias in the ``/aliases`` node pointing
   to it.
@@ -37,9 +37,31 @@ with the new *state* variable set's content.
    magic value. They're already reserved by the ``direct`` and ``circular``
    storage backends.
 
-The ``backend`` property uses the *phandle* mechanism to link the *state* to
-a real persistent memory. Refer :ref:`Backend <state_framework,backends>` for
-supported persistent memories.
+The ``backend`` property link the *state* to a  real persistent memory. Refer
+:ref:`Backend <state_framework,backends>` for supported persistent memories.
+The property can be specified in two ways:
+
+1. Specified by using the *handle* mechanism
+
+	.. code-block:: c
+
+		backend = <&of_partition>;
+
+2. Specified by using string references
+
+	.. code-block:: c
+
+		backend = &mmc2, "partname:1";
+		backend = &mmc2, "partname:my-barebox-state-partition";
+		backend = &of_partition;
+
+By using the 2nd format the state can be stored within a MBR or GPT partition.
+This has the adavantage that magic holes and offsets of partitions can be
+avoided.
+
+.. note::
+   Please ensure that your dt-utils user space package is
+   recent enough to handle this format. If unsure use format 1.
 
 The ``backend-type`` should be ``raw`` or ``dtb``. Refer
 :ref:`Backend Types <state_framework,backend_types>` for further details.
diff --git a/Documentation/user/state.rst b/Documentation/user/state.rst
index a0e27d4fe8..d5fff18672 100644
--- a/Documentation/user/state.rst
+++ b/Documentation/user/state.rst
@@ -542,10 +542,14 @@ SD/eMMC and ATA
 
 The following devicetree node entry defines some kind of SD/eMMC memory and
 a partition at a specific offset inside it to be used as the backend for the
-*state* variable set. Note that currently there is no support for on-disk
-partition tables. Instead, an ofpart partition description must be used. You
-have to make sure that this partition does not conflict with any other partition
-in the partition table.
+*state* variable set. The partition can be specified by an ofpart partition
+or by on-disk partition tables. Below is an example for an ofpart partition.
+You have to make sure that this partition does not conflict with any other
+partition in the partition table.
+
+.. note::
+   For on-disk partition table support a very recent dt-utils package is
+   required. If unsure use the ofpart partition scheme.
 
 .. code-block:: text
 
diff --git a/common/state/state.c b/common/state/state.c
index cee83ebde4..6272333f45 100644
--- a/common/state/state.c
+++ b/common/state/state.c
@@ -432,7 +432,6 @@ static int of_state_fixup(struct device_node *root, void *ctx)
 	const char *compatible = "barebox,state";
 	struct state *state = ctx;
 	struct property *p;
-	phandle phandle;
 	int ret;
 
 	node = of_find_node_by_path_from(root, state->of_path);
@@ -499,10 +498,20 @@ static int of_state_fixup(struct device_node *root, void *ctx)
 		goto out;
 	}
 
-	phandle = of_node_create_phandle(backend_node);
-	ret = of_property_write_u32(new_node, "backend", phandle);
-	if (ret)
-		goto out;
+	if (state->backend_is_new_format) {
+		ret = of_property_write_strings(new_node, "backend",
+						backend_node->full_name,
+						state->backend_dts_partition, NULL);
+		if (ret)
+			goto out;
+	} else {
+		phandle phandle;
+
+		phandle = of_node_create_phandle(backend_node);
+		ret = of_property_write_u32(new_node, "backend", phandle);
+		if (ret)
+			goto out;
+	}
 
 	if (!strcmp("raw", state->format->name)) {
 		struct digest *digest =
@@ -574,6 +583,7 @@ void state_release(struct state *state)
 	state_format_free(state->format);
 	free(state->backend_dev_path);
 	free(state->backend_dts_dev_or_part);
+	free(state->backend_dts_partition);
 	free(state->of_path);
 	free(state);
 }
@@ -582,11 +592,15 @@ void state_release(struct state *state)
 
 #define state_of_find_path_by_node(node, path, flags, offset, size) \
 	of_find_path_by_node(node, path, flags)
+#define state_of_find_path(node, propname, outpath, flags, offset, size) \
+	of_find_path(node, propname, outpath, flags)
 
 #else
 
 #define state_of_find_path_by_node(node, path, flags, offset, size) \
 	of_get_devicepath(node, path, offset, size)
+#define state_of_find_path(node, propname, outpath, flags, offset, size) \
+	of_find_path(node, propname, outpath, offset, size)
 
 #endif /* __BAREBOX__ */
 
@@ -614,6 +628,39 @@ state_parse_old_backend_format(struct device_node *backend_node,
 	return 0;
 }
 
+static int
+state_parse_new_backend_format(struct device_node *state_node,
+			       struct state *state, off_t *offset, size_t *size)
+{
+	struct device_node *backend_node;
+	const char *backend_dev_or_part;
+	const char *part = NULL;
+	int ret;
+
+	backend_dev_or_part = of_get_property(state_node, "backend", NULL);
+	if (!backend_dev_or_part)
+		return -EINVAL;
+
+	backend_node = of_find_node_by_path(backend_dev_or_part);
+	if (!backend_node)
+		return -ENODEV;
+
+	if (is_of_partition(backend_node))
+		ret = of_partition_ensure_probed(backend_node);
+	else
+		ret = of_device_ensure_probed(backend_node);
+	if (ret)
+		return ret;
+
+	of_property_read_string_index(state_node, "backend", 1, &part);
+	state->backend_dts_dev_or_part = of_get_reproducible_name(backend_node);
+	state->backend_dts_partition = xstrdup(part);
+	state->backend_is_new_format = true;
+
+	return state_of_find_path(state_node, "backend",
+				  &state->backend_dev_path, 0, offset, size);
+}
+
 /*
  * state_new_from_node - create a new state instance from a device_node
  *
@@ -643,14 +690,21 @@ struct state *state_new_from_node(struct device_node *node, bool readonly)
 	if (IS_ERR(state))
 		return state;
 
+	/*
+	 * backend can be in three formats:
+	 *  old) backend = <&state_part>;
+	 *  new) backend = &state_part;
+	 *       backend = &state_part_dev, "partname:0";
+	 *
+	 * Start with the old format for compatibility reasons.
+	 */
 	backend_node = of_parse_phandle(node, "backend", 0);
-	if (!backend_node) {
-		dev_err(&state->dev, "Cannot resolve \"backend\" phandle\n");
-		ret = -EINVAL;
-		goto out_release_state;
-	}
-
-	ret = state_parse_old_backend_format(backend_node, state, &offset, &size);
+	if (backend_node)
+		ret = state_parse_old_backend_format(backend_node, state,
+						     &offset, &size);
+	else
+		ret = state_parse_new_backend_format(node, state,
+						     &offset, &size);
 	if (ret)
 		goto out_release_state;
 
diff --git a/common/state/state.h b/common/state/state.h
index 83a7fae4a5..fdc6b3fc40 100644
--- a/common/state/state.h
+++ b/common/state/state.h
@@ -121,6 +121,8 @@ struct state {
 	struct state_backend_storage storage;
 	char *backend_dev_path;
 	char *backend_dts_dev_or_part;
+	char *backend_dts_partition;
+	bool backend_is_new_format;
 };
 
 enum state_convert {
-- 
2.30.2

Re: [PATCH 8/8] state: add support for new backend format

[Sascha Hauer]

On Fri, Oct 14, 2022 at 06:35:34PM +0200, Marco Felsch wrote:
> The backend can now be specified by either:
> 
>   backend = <&of_partition>;
> 
> or by using the path binding:
> 
>   backend = &mmc2, "partname:2";
> 
>   backend = &of_partition;

We discussed this internally and came to the conclusion that we don't
want to spread the usage of this binding that is currently used for the
barebox environment as well.

Instead we want to go into another direction:

The MBR/GPT partitions should be described in a node under the physical
device and then the state node can continue using the phandle only, so
something like:

backend = <&state_part>;

&mmc {
	partitions {
		compatible = "gpr-partitions";

		state_part {
			partuuid = "16367da7-c518-499f-9aad-e1f366692365"
		};
	};
};

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 |

Re: [PATCH 0/8] Barebox-State on-disk partition support

[Marco Felsch]

On 22-10-14, Marco Felsch wrote:
> Hi,
> 
> this small series adds the support to store the barebox-state on a
> on-disk partition like mbr/gpt. Note for testing you need a very recent
> dt-utils package. I will send the patches soon as well and will add a
> link to the patches here later.

The required user-space patches can be found here:

https://lore.pengutronix.de/oss-tools/20221014164204.3812506-1-m.felsch@pengutronix.de/T/#m2f222141796f930be0f93e11401f143335b00e7c

Regards,
  Marco