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@xxxxxxxxxxxxxx>
> > > > ---
> > > >
> > > > 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@xxxxxxxxxxxxxxxxxxx
http://lists.infradead.org/mailman/listinfo/barebox
