Re: [PATCH 1/5] misc: fs_loader: fix ubifs not unmounted on dev_get_priv error

[Daniel Golle <daniel@makrotopia.org>]

On Tue, Mar 03, 2026 at 02:43:29PM +0100, Christian Marangi wrote:
> On Tue, Mar 03, 2026 at 01:38:03PM +0000, Daniel Golle wrote:
> > On Tue, Mar 03, 2026 at 02:29:08PM +0100, Christian Marangi wrote:
> > > When dev_get_priv errors out, the ubifs is not unbounted (if used)
> > 
> > I don't understand why or where UBIFS is being used here.
> > Did you mean to say "UBI is not attached"?
> > UBI != UBIFS.
> >
> 
> Function that is not called is umount_ubifs. That calls cmd_ubifs_umount.
> unbounted is a typo for unmounted.

Ok, so it *is* UBIFS, because you cannot mount UBI. You can only
"attach" an MTD partition to serve as UBI device (`ubi part ...` command
in U-Boot).

> 
> Probably the confusion is present already in the FS loader driver where ubi
> needed to be used instead of ubifs?

If you are loading a file from a directory and rely on full POSIX-like
permissions, file ownership, ... then you need a proper filesystem, like
UBIFS, or SquashFS, ...
If all you need is to load raw data early at boot, using a static UBI
volume to hold the raw data is probably better (see below).

> 
> There are mount_ubifs and umount_ubifs but they should have been mount_ubi and
> umount_ubi ? But then they use ubifs cmd OPs. I'm a bit confused here.

UBIFS is a read/write filesystem used on top of UBI.

UBI is a bit like a light version of LVM2, you can have multiple named
volumes, dynamically create them, rename or resize them.

Volumes can be static (ie. write-once, CRC32 over the whole volume
which is validated on access) or dynamic (no CRC validation, content
may change).

Inside dynamic volumes you may have either a filesystem or just raw
data. Any read-only filesystem can be used on top of UBI with the help
of ubiblock devices (eg. squashfs, erofs, ...).

In static volumes you typically store raw data, like it is the case
for U-Boot SPL loading U-Boot proper from UBI requires u-boot.bin
stored in a raw static volume, **not** inside a filesystem).

Imho using static volumes also makes a lot of sense for storing
those PHY or Ethernet firmware blobs, potentially even storing them
redundantly (ie. two static volumes, if one got broken CRC, move on
to the next one).


>  
> > > 
> > > Correctly handle this handle condition and while at it also return -EINVAL
> > > instead of -ENOMEM as a better error since no memory is allocated but is
> > > actually an invalid scenario for fw_get_filesystem_firmware().
> > > 
> > > Signed-off-by: Christian Marangi <ansuelsmth@gmail.com>
> > > ---
> > >  drivers/misc/fs_loader.c | 6 ++++--
> > >  1 file changed, 4 insertions(+), 2 deletions(-)
> > > 
> > > diff --git a/drivers/misc/fs_loader.c b/drivers/misc/fs_loader.c
> > > index 2928cf75f89e..7e432a7ebd62 100644
> > > --- a/drivers/misc/fs_loader.c
> > > +++ b/drivers/misc/fs_loader.c
> > > @@ -178,8 +178,10 @@ static int fw_get_filesystem_firmware(struct udevice *dev)
> > >  
> > >  	struct firmware *firmwarep = dev_get_priv(dev);
> > >  
> > > -	if (!firmwarep)
> > > -		return -ENOMEM;
> > > +	if (!firmwarep) {
> > > +		ret = -EINVAL;
> > > +		goto out;
> > > +	}
> > >  
> > >  	ret = fs_read(firmwarep->name, (ulong)map_to_sysmem(firmwarep->data),
> > >  			firmwarep->offset, firmwarep->size, &actread);
> > > -- 
> > > 2.51.0
> > > 
> 
> -- 
> 	Ansuel