From: Sascha Hauer <sha@pengutronix.de>
To: Michael Olbrich <m.olbrich@pengutronix.de>
Cc: barebox@lists.infradead.org
Subject: Re: [PATCH 3/3] state: support backend-diskuuid / backend-offset
Date: Wed, 26 Jan 2022 11:15:24 +0100 [thread overview]
Message-ID: <20220126101524.GH23490@pengutronix.de> (raw)
In-Reply-To: <20220126093513.GI11273@pengutronix.de>
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.
> > > + }
> > > + 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.
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 |
_______________________________________________
barebox mailing list
barebox@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/barebox
next prev parent reply other threads:[~2022-01-26 10:16 UTC|newest]
Thread overview: 12+ messages / expand[flat|nested] mbox.gz Atom feed top
2022-01-24 10:04 [PATCH 0/3] state: find backend with UUID but without a partition Michael Olbrich
2022-01-24 10:04 ` [PATCH 1/3] cdev: rename partuuid to uuid Michael Olbrich
2022-01-24 10:04 ` [PATCH 2/3] cdev: add diskuuid support Michael Olbrich
2022-01-24 10:04 ` [PATCH 3/3] state: support backend-diskuuid / backend-offset Michael Olbrich
2022-01-26 7:57 ` Sascha Hauer
2022-01-26 9:35 ` Michael Olbrich
2022-01-26 10:15 ` Sascha Hauer [this message]
2022-01-26 11:16 ` Michael Olbrich
2022-01-27 0:18 ` Trent Piepho
2022-01-27 12:39 ` Sascha Hauer
2022-01-27 18:53 ` Trent Piepho
2022-01-28 7:48 ` Sascha Hauer
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=20220126101524.GH23490@pengutronix.de \
--to=sha@pengutronix.de \
--cc=barebox@lists.infradead.org \
--cc=m.olbrich@pengutronix.de \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox