Hi Vincent,
On Sun, 17 May 2026 at 14:49, Vincent Jardin <[email protected]> wrote:
>
> Hi Simon,
>
> Thanks. See below,
>
> Before I send v2, I'd like your feedbacks,
>
> Le 15/05/26 06:43, Simon Glass a écrit :
> > 1. Typo: 'thue' should be 'the'
>
> ok for v2
>
> > 2. If cmd/ubifs.c is the duplication this patch obsoletes, please
> > either remove it in the same series or reword the motivation - as it
> > stands the duplication is still in-tree after the patch, so the reader
> > is left wondering whether 'load ubifs ...' is now the preferred entry
> > point or whether ubifsload remains supported
>
> I'll reword in v2's commit body. The point of this
> patch is to make load ubifs <addr> <file> a first-class entry
> point that routes through fs_set_blk_dev() like every other
> fstype. ubifsload (cmd/ubifs.c) stays in-tree for backward
> compatibility; removing (or change it to a preferred entry),
> it is not the goal of this commit, but it seems to be a quick win.
>
> >
> > Please also spell out which user-visible commands get this new support
> > ('load semihosting ...', 'load ubifs ...').
>
> ok, v2's commit log will show them, per the following.
> For instance, based on my records from today,
>
> My board: NXP LX2160A, U-Boot 2026.04-00933-g0a14371fb76c with
> OpenOCD attached using J-Link for SYS_OPEN / SYS_READ / SYS_FLEN / SYS_CLOSE
> from
> the host filesystem,
>
> load semihosting
>
> => load semihosting - 0xc0000000 semihost-test.txt
> 76 bytes read in 835 ms (0 Bytes/s)
> => md.b 0xc0000000 80
> c0000000: 53 65 6d 69 68 6f 73 74 69 6e 67 20 74 65 73 74 Semihosting
> test
> c0000010: 20 66 69 6c 65 20 66 72 6f 6d 20 55 2d 42 6f 6f file from
> U-Boo
> c0000020: 74 20 70 61 74 63 68 20 76 65 72 69 66 69 63 61 t patch
> verifica
> c0000030: 74 69 6f 6e 20 2d 20 32 30 32 36 2d 30 35 2d 31 tion -
> 2026-05-1
> c0000040: 37 54 31 35 3a 31 38 3a 34 33 5a 0a ef be ad de
> 7T15:18:43Z.....
>
> size semihosting
>
> => size semihosting - semihost-test.txt
> => printenv filesize
> filesize=4c
>
> # OpenOCD debug logs
> semihosting_common(): op=0x1 (OPEN), param=0xfbb37fe8
> semihosting_common(): open('/tmp/semihost-test.txt')=12
> semihosting_common(): op=0xc (FLEN), param=0xfbb37ff8
> semihosting_common(): op=0x2 (CLOSE), param=0xfbb37ff8
>
> ls semihosting : fs/semihostingfs.c does not registers ls hook in its
> fstype_info, so fs_ls() has no work to do. OpenOCD's
> log confirms no semihosting transaction was issued during
> the ls attempts, as expected:
>
> => ls semihosting - .
> => ls semihosting - semihost-test.txt
> (silent return, no error, no semihosting trap fired)
>
> Without this patch all those commands fail in fs_set_blk_dev()
> with
> ** Bad device specification semihosting **
> before the FS layer is called.
>
> For ubifs, it is code analysis only, I do not have hardware boards
> to check.
>
> From the code review:
>
> - fs/ubifs/ubifs.c:ubifs_set_blk_dev() takes (struct blk_desc *,
> struct disk_partition *) and ignores both, it only checks the
> global ubifs_mounted.
>
> - The patched fs_set_blk_dev() dispatch path, for any fstype
> whose null_dev_desc_ok flag is set, therefore feeds
> ubifs_set_blk_dev() the NULLs it has been receiving
> from cmd/ubifs.c all along. No new control flow through
> fs/ubifs/ ; the patch only adds a second entry point with
> identical preconditions.
>
> Do you agree ?
Yes.
>
> The intended behaviour is that after ubi part ... + ubifsmount ...
> have attached and mounted a volume,
> load ubifs <addr> <file> and
> ls ubifs <path>
> shall work like every other <fstype> family.
>
> I found those from code reviews, but it seems logic.
>
> >> if (strcmp(info->name, ifname) != 0)
> > if (strcmp(...))
>
> I'll change to if (!strcmp(...)) in v2.
>
> > Can you add a test for this, that loads via the new semihosting
> > ifname? Do we need a doc/ update?
>
> OK, for v2, I'll check to add both:
>
> - test/py/tests/test_semihosting.py exercising load semihosting
> + comparing the loaded bytes via md.b against a host-staged
> fixture. Should you have some test logics, I'll appreciate, I am
> learning with uboot and uboot test frameowrk.
>
> - a section in doc/usage/cmd/load.rst documenting the
> ifname-routed null-block-device path and listing the fstypes
> that opts into it (semihosting, ubifs).
>
> For ubifs, since I cannot add hardware coverage, I'll try to wire
> load ubifs into test/py/tests/test_ubifs.py alongside the
> existing ubifsload invocations.
>
> >
> > BTW, what do you think of this 'desc being NULL' approach? We should
> > perhaps improve it so that we can support non-block filesystems more
> > generally.
>
> You are right that the open-coded "treat NULL block_desc as the
> null-device signal" reads as a bad layering.
>
> Maybe, as an improvement fro v2: introduce a small
> helper near fs_set_blk_dev_with_part() that names the convention
> explicitly, so the dispatch site is one self-documenting line:
>
> /*
> * Some fstypes (semihosting, ubifs) have no underlying block
> * device and ignore the block_desc argument of their set_blk_dev
> * hook. The legacy commands (ubifsload, semihosting) just pass
> * NULL; for load <iface> ... to behave the
> * same, the dispatcher opts those fstypes in by name.
> */
> static const struct fstype_info *
> fs_lookup_null_dev_info(const char *ifname)
> {
> const struct fstype_info *info = fs_get_info_by_ifname(ifname);
>
> return (info && info->null_dev_desc_ok) ? info : NULL;
> }
>
> ...then the dispatch:
>
> info = fs_lookup_null_dev_info(ifname);
> if (info)
> return info->set_blk_dev(NULL, NULL);
>
> If you'd rather keep the dispatch open-coded with the shorter
> if (!strcmp()) form you already pointed out, I'll just do that
> and drop the helper; I believe it's a nice-to-have.
Yes this helper approach seems good to me.
>
> Your feedbacks are very welcomed and appreciated, I'll send the v2 asap.
Thanks for working in this piece. It needs an overhaul at some point,
perhaps as part of introducing the FS uclass, etc.
Regards,
Simon
