Re: [OSS-Tools] [PATCH v3 3/3] state: automatically find state.dtb in the ESP

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

On Tue, May 10, 2022 at 03:59:24PM +0200, Ahmad Fatoum wrote:
> Hello Michael,
> 
> On 10.05.22 08:57, Michael Olbrich wrote:
> > Systemd mounts the EFI System Partition (ESP) to /boot or /efi.
> > So look there for the state.dtb when the devicetree in sysfs/procfs is
> > not available.
> > 
> > This way barebox-state can be used on EFI systems without manually
> > specifying the devicetree file.
> > 
> > Signed-off-by: Michael Olbrich <m.olbrich@pengutronix.de>
> > ---
> >  src/barebox-state.c | 24 ++++++++++++++++++++++++
> >  1 file changed, 24 insertions(+)
> > 
> > diff --git a/src/barebox-state.c b/src/barebox-state.c
> > index 334aed6f3d43..bf67340d4dc6 100644
> > --- a/src/barebox-state.c
> > +++ b/src/barebox-state.c
> > @@ -342,6 +342,30 @@ struct state *state_get(const char *name, const char *filename, bool readonly, b
> >  		}
> >  	} else {
> >  		root = of_read_proc_devicetree();
> > +
> > +		/* No device-tree in procfs / sysfs, try dtb file in the ESP */
> > +		if (-PTR_ERR(root) == ENOENT) {
> > +			const char *paths[] = {
> > +				/* default mount paths used by systemd */
> > +				"/boot/EFI/BAREBOX/state.dtb",
> > +				"/efi/EFI/BAREBOX/state.dtb",
> 
> On my Debian, it is /boot/efi/EFI. Do you mind appending that one to the list? :-)

ok.

> > +				NULL
> > +			};
> > +			void *fdt;
> > +			int i;
> > +
> > +			for (i = 0; paths[i]; ++i) {
> > +				fdt = read_file(paths[i], NULL);
> > +				if (fdt)
> > +					break;
> > +			}
> > +			if (fdt) {
> > +				root = of_unflatten_dtb(fdt);
> > +				free(fdt);
> > +			}
> > +			else
> > +				root = ERR_PTR(-ENOENT);
> 
> This duplicates code in the first if clause. Can you move this into
> a common helper (just a static function above state_get suffices)
> and use it?

It's similar but not the same. Here we loop over multiple files and take
the first that works, so there should be no error messages when some of
them fail.
A share function would require conditional error messages and at that
point, I don't think it actually simplifies the code any more.

Michael

> > +		}
> >  		if (IS_ERR(root)) {
> >  			pr_err("Unable to read devicetree. %s\n",
> >  			       strerror(-PTR_ERR(root)));
> 
> 
> -- 
> 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 |