On Mon, Jan 9, 2023 at 3:20 PM Christian Marangi <ansuels...@gmail.com> wrote: > On Mon, Jan 09, 2023 at 02:34:48PM -0800, Brian Norris wrote: > > On Mon, Jan 9, 2023 at 1:53 PM Christian Marangi <ansuels...@gmail.com> > > wrote: > > > > > > On Fri, Jan 06, 2023 at 06:04:22PM -0800, Brian Norris wrote: > > > > We're assuming all root= arguments are /dev/ paths, but many targets > > > > utilize root=PARTUUID=<xxx> strategies. At least allow them to fall back > > > > to scanning all block devices. > > > > > > > > Signed-off-by: Brian Norris <computersforpe...@gmail.com> > > > > > > Can you elaborate this a bit more? > > > > This function currently assumes that if it can find a "root=" line, > > then it can parse it and use it to find "rootfs_data" on the same > > disk. But the rootdevname() function really chokes (does completely > > the wrong thing) when given "PARTUUID=<blah>". So this parser becomes > > useless. > > > > > This is dependent of the google based > > > devices with ipq806x but why? > > > > I guess it's not *strictly* a dependency, but as-is, things aren't great. > > > > With the above choking, I believe fstools will fall back to > > rootdisk.c, which will place rootfs_data in a different place -- > > appended to the squashfs filesystem, via a custom loopback device. > > This works OK I suppose, but has its downsides. > > > > But the real kicker is that if fstools / partname.c eventually learns > > how to find the right 'rootfs_data' partition, then suddenly our data > > filesystem will move, and that's not great for sysupgrade. So I'd like > > to get it right from the start, rather than change between rootdisk.c > > and partname.c approaches arbitrarily. > > > > > In theory we should have other device with sd card/eMMC that use uuid to > > > select the partition... > > > > Right. Search the tree for the "root=PARTUUID=" string, and you'll > > find several. (Some breadcrumbs in rockchip, x86, imx, and more.) > > Presumably they would run into the same problem if they tried to use a > > dedicated data partition on a block device -- right now, I believe > > Rockchip restricts itself to a 2-partition layout, for instance. Which > > is why I didn't specifically call this a ipq806x / Google-specific > > thing. > > > > This is what gets me most confused... The patch is correct and looks > good... Just what I can't understand is how things worked till today... > > They all fallback to rootdisk.c? It's worth to check and have some proof > of this theory. > > Since we are playing with the function mounting root the main concern > here is that we brake any device using PARTUUID that somehow are working > now so we need to be carefull in what we change as the risk of causing > regression is behind the corner.
Totally understood. > So we should just find a way to understand why thing are working (or are > not working and are using an alternative way currently) Just that and > this can be merged. I don't have any of these targets. (I suppose I could try to figure out how, if at all, the x86/generic target is handling this. But it seems like a very flexible target that can be used in many ways, and I'm not sure I'd find the Right(TM) ones to test.) But looking at sources, I see imx (Build/imx-combined-image) and rockchip (Build/pine64-img) are doing 2-partition layouts, with $(SCRIPT_DIR)/gen_image_generic.sh. So that skips "partname" and just does "rootdisk". On the other hand, Device/glinet_gl-b2200 is one of the few I could find with a 3-partition (with rootfs_data partition) layout (qsdk-ipq-app-gpt), and it has an explicit "root=/dev/mmcblk0p2" in its bootargs. (A related key point: it's the only one using `CI_DATAPART` for emmc.sh sysupgrade.) Most (all?) remaining rootfs_data references I see are related to MTD/UBI, and shouldn't really be relevant. So I don't have a full proof of it, but it appears that all relevant users are either 2-partition layouts that use rootdisk.c, or else using partname.c with a rootfs_data partition but only using /dev paths for root=. Exceptions would be if someone was manually modifying their partition layout with a spare rootfs_data partition, in which case it's possible this could pick it up now. I'm not sure we can guard against all local customizations though. I can try to look or play around some more, if you have hints on what I should investigate. Or wait around for someone (Daniel?) who has more background in this area? > > > Also can't we just ignore the device bootargs > > > and provide a custom one? > > > > This isn't about the device (as in, baked into a bootloader) bootargs; > > see "root=PARTUUID=%U/PARTNROFF=1" in my patch 7: > > https://patchwork.ozlabs.org/project/openwrt/patch/20230107074945.2140362-7-computersforpe...@gmail.com/ > > This is a better way of specifying root devices (say, rather than > > memorizing a "/dev/mmcblk0" or similar, which is *not* a stable > > contract; it also means boot-from-USB won't work), except that fstools > > doesn't like it. (And again, there are several other existing > > openwrt.git users for it.) > > > > I understand and I want this fixed. Sorry if I look confused but you can > understand how this was broken from ages and still we have many target > using the root=PARTUUID format makes me question a lot of thing ahahhaha Understood, I question things all the time, especially when they *seem* to be especially broken :) Brian > > > > --- > > > > > > > > Changes in v2: > > > > * fstools, not fsutils (sorry for the noise) > > > > > > > > libfstools/partname.c | 4 ++-- > > > > 1 file changed, 2 insertions(+), 2 deletions(-) > > > > > > > > diff --git a/libfstools/partname.c b/libfstools/partname.c > > > > index f59c52eb8f3c..9c27015643ad 100644 > > > > --- a/libfstools/partname.c > > > > +++ b/libfstools/partname.c > > > > @@ -128,12 +128,12 @@ static struct volume *partname_volume_find(char > > > > *name) > > > > return NULL; > > > > } > > > > > > > > - if (get_var_from_file("/proc/cmdline", "root", rootparam, > > > > sizeof(rootparam))) { > > > > + if (get_var_from_file("/proc/cmdline", "root", rootparam, > > > > sizeof(rootparam)) && rootparam[0] == '/') { > > > > rootdev = rootdevname(rootparam); > > > > /* find partition on same device as rootfs */ > > > > snprintf(ueventgstr, sizeof(ueventgstr), > > > > "%s/%s/*/uevent", block_dir_name, rootdev); > > > > } else { > > > > - /* no 'root=' kernel cmdline parameter, find on any block > > > > device */ > > > > + /* no useful 'root=' kernel cmdline parameter, find on > > > > any block device */ > > > > snprintf(ueventgstr, sizeof(ueventgstr), "%s/*/uevent", > > > > block_dir_name); > > > > } > > > > > > > > -- > > > > 2.39.0 > > > > > > > > > > > > _______________________________________________ > > > > openwrt-devel mailing list > > > > openwrt-devel@lists.openwrt.org > > > > https://lists.openwrt.org/mailman/listinfo/openwrt-devel > > > > > > -- > > > Ansuel > > -- > Ansuel _______________________________________________ openwrt-devel mailing list openwrt-devel@lists.openwrt.org https://lists.openwrt.org/mailman/listinfo/openwrt-devel