Re: [PATCH 3/3] state: support backend-diskuuid / backend-offset

[Michael Olbrich <m.olbrich@pengutronix.de>]

On Wed, Jan 26, 2022 at 11:15:24AM +0100, Sascha Hauer wrote:
> On Wed, Jan 26, 2022 at 10:35:13AM +0100, Michael Olbrich wrote:
> > On Wed, Jan 26, 2022 at 08:57:27AM +0100, Sascha Hauer wrote:
> > > On Mon, Jan 24, 2022 at 11:04:58AM +0100, Michael Olbrich wrote:
> > > > On some platforms (e.g. EFI on x86_64) the state backend can only be
> > > > selected by a partiton UUID. On existing devices with a DOS partition
> > > > table, there may be no spare partition available for state.
> > > > 
> > > > This makes it possible to select the disk via UUID. The exact position is
> > > > defined by an explicitly specified offset.
> > > > 
> > > > Signed-off-by: Michael Olbrich <m.olbrich@pengutronix.de>
> > > > ---
> > > > 
> > > > I wasn't sure where to add the helper function. Is include/fs.h ok or
> > > > should I put it somewhere else?
> > > > 
> > > > I'll implement the same helper for dt-utils, so we can avoid additional
> > > > #ifdef here.
> > > > 
> > > >  common/state/state.c | 55 +++++++++++++++++++++++++++++---------------
> > > >  include/fs.h         | 12 ++++++++++
> > > >  2 files changed, 49 insertions(+), 18 deletions(-)
> > > > 
> > > > diff --git a/common/state/state.c b/common/state/state.c
> > > > index 8c34ae83e52b..2a8b12d20c5a 100644
> > > > --- a/common/state/state.c
> > > > +++ b/common/state/state.c
> > > > @@ -592,6 +592,7 @@ struct state *state_new_from_node(struct device_node *node, bool readonly)
> > > >  	const char *backend_type;
> > > >  	const char *storage_type = NULL;
> > > >  	const char *alias;
> > > > +	const char *diskuuid;
> > > >  	uint32_t stridesize;
> > > >  	struct device_node *partition_node;
> > > >  	off_t offset = 0;
> > > > @@ -607,30 +608,48 @@ 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) {
> > > > -		dev_err(&state->dev, "Cannot resolve \"backend\" phandle\n");
> > > > -		ret = -EINVAL;
> > > > -		goto out_release_state;
> > > > -	}
> > > > +	ret = of_property_read_string(node, "backend-diskuuid", &diskuuid);
> > > 
> > > This needs some documentation in
> > > Documentation/devicetree/bindings/barebox/barebox,state.rst.
> > 
> > I can do that.
> > 
> > > > +	if (ret == 0) {
> > > > +		u64 off;
> > > > +
> > > > +		ret = devpath_from_diskuuid(diskuuid, &state->backend_path);
> > > > +		if (ret) {
> > > > +			dev_err(&state->dev, "state failed find backend device for diskuuid='%s'\n",
> > > > +				diskuuid);
> > > > +			goto out_release_state;
> > > > +		}
> > > > +		ret = of_property_read_u64(node, "backend-offset", &off);
> > > 
> > > I stumbled upon this because you have to use a 64bit type here instead
> > > of using 'offset' directly. I think 'offset' should be 64bit instead so
> > > that larger offsets can be used.
> > 
> > It's not that simple. 'offset' used as a 'off_t' and 'ssize_t' all over the
> > place in the state framework. On 32bit architecture both are defined as
> > 'long' or 'int'. Both are 32 bit types so changing 'offset' to a 64bit type
> > here doesn't really help.
> 
> Of course not, we would have to replace all variables which are used as
> offset into a device to 64bit types. That's a separate topic which
> doesn't have to be solved as part of this series.

So what should I do here?
- use 'u64' for 'offset' and remove the separate variable
- use 'loff_t' for 'offset'
- keep it as it is
- something else?

> > > > +		}
> > > > +		offset = off;
> > > 
> > > What about the size of the state partition? This is not set anywhere in
> > > this case so it's still zero. It should be specified the in device tree
> > > as well. At the same time I'm a bit nervous that it apparently still
> > > works with size zero.
> > 
> > The code explicitly checks if the size is specified and skips any range
> > checks if it's not. From what I can tell, it has been like that from the
> > beginning.
> 
> That's likely ok for real partitions. When reading/writing them past the end
> we'll get an error from the lower layers which can be handled, but not
> so in this case where size is potentially the rest of the whole disk.
> 
> > 
> > If that's not ok, then I could add a 'backend-size' property, or do you
> > have something else in mind?
> 
> Yes, that's what I had in mind. And I think it should be mandatory.

So, if we add a size, then it should also be validated in some way, right?
Maybe ensure that it does not overlap with existing partitions?

Michael

-- 
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 |

_______________________________________________
barebox mailing list
barebox@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/barebox