From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: X-Spam-Checker-Version: SpamAssassin 3.4.0 (2014-02-07) on aws-us-west-2-korg-lkml-1.web.codeaurora.org Received: from phobos.denx.de (phobos.denx.de [85.214.62.61]) (using TLSv1.2 with cipher ECDHE-RSA-AES128-GCM-SHA256 (128/128 bits)) (No client certificate requested) by smtp.lore.kernel.org (Postfix) with ESMTPS id 03BF0C433EF for ; Thu, 30 Jun 2022 06:47:09 +0000 (UTC) Received: from h2850616.stratoserver.net (localhost [IPv6:::1]) by phobos.denx.de (Postfix) with ESMTP id 7E214843E8; Thu, 30 Jun 2022 08:47:07 +0200 (CEST) Authentication-Results: phobos.denx.de; dmarc=none (p=none dis=none) header.from=thorsis.com Authentication-Results: phobos.denx.de; spf=pass smtp.mailfrom=u-boot-bounces@lists.denx.de Received: by phobos.denx.de (Postfix, from userid 109) id 54BE7843F9; Thu, 30 Jun 2022 08:47:06 +0200 (CEST) Received: from mail.thorsis.com (mail.thorsis.com [92.198.35.195]) by phobos.denx.de (Postfix) with ESMTP id 230B7842D0 for ; Thu, 30 Jun 2022 08:47:03 +0200 (CEST) Authentication-Results: phobos.denx.de; dmarc=none (p=none dis=none) header.from=thorsis.com Authentication-Results: phobos.denx.de; spf=pass smtp.mailfrom=ada@thorsis.com From: Alexander Dahl To: Pali =?ISO-8859-1?Q?Roh=E1r?= Cc: u-boot@lists.denx.de, Simon Glass , Sjoerd Simons , Govindaraji Sivanantham , Hiremath Gireesh , Marcel Ziswiler , Frieder Schrempf , Parthiban Nallathambi , Navin Sankar Velliangiri , "Derald D. Woods" , Martyn Welch , Patrick Delaunay , Patrice Chotard Subject: Re: [PATCH] distroboot: Fix ubifs Date: Thu, 30 Jun 2022 08:46:59 +0200 Message-ID: <1712310.U2yLUagofQ@ada> In-Reply-To: <20220629135527.iiazaufuq6qcwdfp@pali> References: <20220531083236.18968-1-pali@kernel.org> <8595563.AsjhMEDtd8@ada> <20220629135527.iiazaufuq6qcwdfp@pali> Content-Transfer-Encoding: quoted-printable Content-Type: text/plain; charset="iso-8859-1" X-Clacks-Overhead: GNU Terry Pratchett X-BeenThere: u-boot@lists.denx.de X-Mailman-Version: 2.1.39 Precedence: list List-Id: U-Boot discussion List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Errors-To: u-boot-bounces@lists.denx.de Sender: "U-Boot" X-Virus-Scanned: clamav-milter 0.103.6 at phobos.denx.de X-Virus-Status: Clean Hello, Am Mittwoch, 29. Juni 2022, 15:55:27 CEST schrieb Pali Roh=E1r: > Hello! >=20 > On Wednesday 29 June 2022 15:36:57 Alexander Dahl wrote: > > Hello Pali, > >=20 > > had a look at this patch, and have some questions. See below. > >=20 > > Am Dienstag, 31. Mai 2022, 10:32:36 CEST schrieb Pali Roh=E1r: > > > Fix multiple issues in ubifs distroboot code: > > >=20 > > > U-Boot supports attaching only one MTD device as UBI at the time. So > > > always call 'ubifsmount ubi0:${bootubivol}' for mounting UBI volume > > > ${bootubivol}. Usage of 'ubi${devnum}' is incorrect as 'ubi part' > > > command attach MTD device always as UBI device ubi0. > >=20 > > Agreed. > >=20 > > > Set distroboot ${bootfstype} variable to ubifs in ubifs_boot command. > > > Distroboot scripts require ${bootfstype} variable to be properly set = and > > > it > > > is already set for all other boot types. > >=20 > > From grepping through u-boot source, I can not quite follow. Where is > > that > > variable set and where is it used? >=20 > For non-ubifs boot targets, env variable ${bootfstype} is set by > scan_dev_for_boot_part shell fragment. Then it calls user boot script > which can use ${bootfstype} variable. In most cases it is used for > constructing cmdline for kernel, mainly as "roofstype=3D${bootfstype}". So this is no variable stored in env, but used in scripts only, right?=20 Especially in scripts not part of U-Boot itself? Maybe this should be=20 documented somewhere then? > > > Set distroboot ${distro_bootpart} variable to ${bootubivol} value. UBI > > > device does not have partitions, but has volumes. Distroboot scripts > > > require something to be set in ${distro_bootpart} variable, so set it= to > > > the UBI volume which is currently mounted by ubifs. > > >=20 > > > Set distroboot ${devnum} variable to fixed string "ubi0". ubifs code > > > differs from the other partition code that it requires "ubi" prefix > > > before > > > number. > >=20 > > Okay. > >=20 > > > Explicitly unmount ubifs volume after loading all data from it. This > > > allows > > > to detach UBI device from MTD device. > >=20 > > Okay. > >=20 > > > Move definition of MTD device with UBI and UBI volume with ubifs > > > filesystem > > > from global env variables ${bootubipart} and ${bootubivol} into the > > > distroboot "func" macro, defined in board include config files. UBIFS > > > distroboot macros then set ${bootubipart} and ${bootubivol} local > > > variables > > > for compatibility with existing distroboot scripts. > >=20 > > This might be problematic. For a downstream board we use the 'bootubiv= ol' > > env variable to switch between different volumes for update purposes.=20 > > Means on firmware update we modify U-Boot environment from Linux > > (fw_setenv) after successfully updating the inactive volume. >=20 > Interesting... I even did not thought about such use case as it is not > possible to do it with other boot filesystems. Well yes, it's somewhat of a hack, but it uses existing distroboot=20 infrastructure. We did comparable switching of volumes for active rootfs=20 without distroboot before. On newer targets we use distroboot together wit= h=20 RAUC and have one fix separate boot volume containing the boot script (the= =20 boot script is not part of those rootfs[01] anymore), which handles loading= =20 one or the other rootfs, no need to alter bootubivol anymore, it's constant. > > > This last change allows to define more UBIFS target devices and make = it > > > clear what is boot MTD/UBI device. > >=20 > > How would one switch the default UBI volume to be booted from then? >=20 > With this my change, it is probably not easily possible. One option is > to define more boot targets, e.g.: >=20 > func(UBIFS, ubifs, 0, UBI, volume0) > func(UBIFS, ubifs, 1, UBI, volume1) >=20 > And then change env ${boottargets} from ubifs0 to ubifs1, which can be > done via fs_setenv. This should work, yes. Maybe it's a little more complicated than this, e.g= =2E=20 switching=20 from 'mmc0 ubifs0' to 'mmc0 ubifs1'=20 or=20 from 'ubifs0 ubifs1' to 'ubifs1 ubifs0' but that's board dependent and no big deal in general I guess. > As with this change is finally possible to specify more ubifs targets > in BOOT_TARGET_DEVICES() u-boot macro. >=20 > This would be same as switching between other u-boot distroboot targets. Makes sense. >=20 > ... >=20 > But I can understand that you want to have this (probably undocumented) > behavior of switching UBI volume via env variable ${bootubivol}. >=20 > So what about allowing both configuration in U-Boot, with stored boot > volume in ENV and with hardcoded boot volume in config file? Just had another look at how 'bootcmd_ubifs0' is set with your patch, and I= =20 think with switching 'boot_targets' instead of 'bootubivol' it should work= =20 (not tested). Only problem would be a migration strategy for old boards or= a=20 more sophisticated logic in Linux to determine what env variable has to be= =20 updated, but don't let that be your concern. > And which / how many boards use this functionality? Seriously, I have no idea. You never know what all those downstream users = do,=20 especially in this case where functionality is in a boot script not part of= U- Boot itself. Maybe I'm the only one using it like this, maybe not. As of= =20 today I would recommend to use some update framework like RAUC anyways. Greets Alex >=20 > > Greets > > Alex > >=20 > > > All board include config files are adjusted to use this new scheme of > > > specifying boot MTD/UBI device. > > >=20 > > > Signed-off-by: Pali Roh=E1r > > > --- > > > CI test passed on https://github.com/u-boot/u-boot/pull/179 > > > --- > > >=20 > > > include/config_distro_bootcmd.h | 27 ++++++++++++++++----------- > > > include/configs/am335x_guardian.h | 3 +-- > > > include/configs/colibri-imx6ull.h | 1 - > > > include/configs/colibri_imx7.h | 1 - > > > include/configs/kontron-sl-mx6ul.h | 2 +- > > > include/configs/mys_6ulx.h | 2 +- > > > include/configs/npi_imx6ull.h | 2 +- > > > include/configs/omap3_beagle.h | 4 +--- > > > include/configs/omap3_evm.h | 4 +--- > > > include/configs/pcl063.h | 2 +- > > > include/configs/stm32mp15_common.h | 2 +- > > > include/configs/uniphier.h | 2 +- > > > 12 files changed, 25 insertions(+), 27 deletions(-) > > >=20 > > > diff --git a/include/config_distro_bootcmd.h > > > b/include/config_distro_bootcmd.h index c55023889cab..c6e9c497413d > > > 100644 > > > --- a/include/config_distro_bootcmd.h > > > +++ b/include/config_distro_bootcmd.h > > > @@ -70,18 +70,23 @@ > > >=20 > > > #ifdef CONFIG_CMD_UBIFS > > > #define BOOTENV_SHARED_UBIFS \ > > > =20 > > > "ubifs_boot=3D" \ > > >=20 > > > - "env exists bootubipart || " \ > > > - "env set bootubipart UBI; " \ > > > - "env exists bootubivol || " \ > > > - "env set bootubivol boot; " \ > > >=20 > > > "if ubi part ${bootubipart} && " \ > > >=20 > > > - "ubifsmount ubi${devnum}:${bootubivol}; " \ > > > + "ubifsmount ubi0:${bootubivol}; " \ > > >=20 > > > "then " \ > > > =09 > > > "devtype=3Dubi; " \ > > >=20 > > > + "devnum=3Dubi0; " \ > > > + "bootfstype=3Dubifs; " \ > > > + "distro_bootpart=3D${bootubivol}; " \ > > >=20 > > > "run scan_dev_for_boot; " \ > > >=20 > > > + "ubifsumount; " \ > > >=20 > > > "fi\0" > > >=20 > > > -#define BOOTENV_DEV_UBIFS BOOTENV_DEV_BLKDEV > > > -#define BOOTENV_DEV_NAME_UBIFS BOOTENV_DEV_NAME_BLKDEV > > > +#define BOOTENV_DEV_UBIFS(devtypeu, devtypel, instance, bootubipart, > > > bootubivol) \ + "bootcmd_ubifs" #instance "=3D" \ > > > + "bootubipart=3D" #bootubipart "; " \ > > > + "bootubivol=3D" #bootubivol "; " \ > > > + "run ubifs_boot\0" > > > +#define BOOTENV_DEV_NAME_UBIFS(devtypeu, devtypel, instance, > > > bootubipart, > > > bootubivol) \ + #devtypel #instance " " > > >=20 > > > #else > > > #define BOOTENV_SHARED_UBIFS > > > #define BOOTENV_DEV_UBIFS \ > > >=20 > > > @@ -411,13 +416,13 @@ > > >=20 > > > BOOT_TARGET_DEVICES_references_PXE_without_CONFIG_CMD_DHCP_or_PXE > > > =20 > > > #endif > > >=20 > > > -#define BOOTENV_DEV_NAME(devtypeu, devtypel, instance) \ > > > - BOOTENV_DEV_NAME_##devtypeu(devtypeu, devtypel, instance) > > > +#define BOOTENV_DEV_NAME(devtypeu, devtypel, instance, ...) \ > > > + BOOTENV_DEV_NAME_##devtypeu(devtypeu, devtypel, instance, ## > > > __VA_ARGS__) > > >=20 > > > #define BOOTENV_BOOT_TARGETS \ > > > =20 > > > "boot_targets=3D" BOOT_TARGET_DEVICES(BOOTENV_DEV_NAME) "\0" > > >=20 > > > -#define BOOTENV_DEV(devtypeu, devtypel, instance) \ > > > - BOOTENV_DEV_##devtypeu(devtypeu, devtypel, instance) > > > +#define BOOTENV_DEV(devtypeu, devtypel, instance, ...) \ > > > + BOOTENV_DEV_##devtypeu(devtypeu, devtypel, instance, ## __VA_ARGS__) > > >=20 > > > #define BOOTENV \ > > > =20 > > > BOOTENV_SHARED_HOST \ > > > BOOTENV_SHARED_MMC \ > > >=20 > > > diff --git a/include/configs/am335x_guardian.h > > > b/include/configs/am335x_guardian.h index b92703205cde..340715dad5c6 > > > 100644 > > > --- a/include/configs/am335x_guardian.h > > > +++ b/include/configs/am335x_guardian.h > > > @@ -29,7 +29,7 @@ > > >=20 > > > "ramdisk_addr_r=3D0x88080000\0" \ > > > =20 > > > #define BOOT_TARGET_DEVICES(func) \ > > >=20 > > > - func(UBIFS, ubifs, 0) > > > + func(UBIFS, ubifs, 0, UBI, rootfs) > > >=20 > > > #define AM335XX_BOARD_FDTFILE "fdtfile=3D" CONFIG_DEFAULT_DEVICE_TREE > > >=20 > > > ".dtb\0" > > >=20 > > > @@ -54,7 +54,6 @@ > > >=20 > > > GUARDIAN_DEFAULT_PROD_ENV \ > > > "autoload=3Dno\0" \ > > > "backlight_brightness=3D50\0" \ > > >=20 > > > - "bootubivol=3Drootfs\0" \ > > >=20 > > > "distro_bootcmd=3D" \ > > > =09 > > > "setenv rootflags \"bulk_read,chk_data_crc\"; " \ > > > "setenv ethact usb_ether; " \ > > >=20 > > > diff --git a/include/configs/colibri-imx6ull.h > > > b/include/configs/colibri-imx6ull.h index 9e5212acb2ee..a0a0e1767fe0 > > > 100644 > > > --- a/include/configs/colibri-imx6ull.h > > > +++ b/include/configs/colibri-imx6ull.h > > > @@ -91,7 +91,6 @@ > > >=20 > > > UBI_BOOTCMD \ > > > UBOOT_UPDATE \ > > > "boot_script_dhcp=3Dboot.scr\0" \ > > >=20 > > > - "bootubipart=3Dubi\0" \ > > >=20 > > > "console=3Dttymxc0\0" \ > > > "defargs=3Duser_debug=3D30\0" \ > > > "fdt_board=3Deval-v3\0" \ > > >=20 > > > diff --git a/include/configs/colibri_imx7.h > > > b/include/configs/colibri_imx7.h index 3dba7bcef258..3cad17777975 > > > 100644 > > > --- a/include/configs/colibri_imx7.h > > > +++ b/include/configs/colibri_imx7.h > > > @@ -131,7 +131,6 @@ > > >=20 > > > UBOOT_UPDATE \ > > > "boot_file=3DzImage\0" \ > > > "boot_script_dhcp=3Dboot.scr\0" \ > > >=20 > > > - "bootubipart=3Dubi\0" \ > > >=20 > > > "console=3Dttymxc0\0" \ > > > "defargs=3D\0" \ > > > "fdt_board=3Deval-v3\0" \ > > >=20 > > > diff --git a/include/configs/kontron-sl-mx6ul.h > > > b/include/configs/kontron-sl-mx6ul.h index 7bc402d578e8..b4808d2bbf75 > > > 100644 > > > --- a/include/configs/kontron-sl-mx6ul.h > > > +++ b/include/configs/kontron-sl-mx6ul.h > > > @@ -45,7 +45,7 @@ > > >=20 > > > #define BOOT_TARGET_DEVICES(func) \ > > > =20 > > > func(MMC, mmc, 1) \ > > > func(MMC, mmc, 0) \ > > >=20 > > > - func(UBIFS, ubifs, 0) \ > > > + func(UBIFS, ubifs, 0, UBI, boot) \ > > >=20 > > > func(USB, usb, 0) \ > > > func(PXE, pxe, na) \ > > > func(DHCP, dhcp, na) > > >=20 > > > diff --git a/include/configs/mys_6ulx.h b/include/configs/mys_6ulx.h > > > index 6801fc109eae..663820177a3e 100644 > > > --- a/include/configs/mys_6ulx.h > > > +++ b/include/configs/mys_6ulx.h > > > @@ -59,7 +59,7 @@ > > >=20 > > > #define BOOT_TARGET_DEVICES(func) \ > > > =20 > > > func(MMC, mmc, 0) \ > > >=20 > > > - func(UBIFS, ubifs, 0) \ > > > + func(UBIFS, ubifs, 0, UBI, boot) \ > > >=20 > > > func(PXE, pxe, na) \ > > > func(DHCP, dhcp, na) > > >=20 > > > diff --git a/include/configs/npi_imx6ull.h > > > b/include/configs/npi_imx6ull.h > > > index c250fa650603..ebb887544e08 100644 > > > --- a/include/configs/npi_imx6ull.h > > > +++ b/include/configs/npi_imx6ull.h > > > @@ -67,7 +67,7 @@ > > >=20 > > > #define BOOT_TARGET_DEVICES(func) \ > > > =20 > > > func(MMC, mmc, 0) \ > > >=20 > > > - func(UBIFS, ubifs, 0) \ > > > + func(UBIFS, ubifs, 0, UBI, boot) \ > > >=20 > > > func(PXE, pxe, na) \ > > > func(DHCP, dhcp, na) > > >=20 > > > diff --git a/include/configs/omap3_beagle.h > > > b/include/configs/omap3_beagle.h index 158773acedb9..f5da08cf3359 > > > 100644 > > > --- a/include/configs/omap3_beagle.h > > > +++ b/include/configs/omap3_beagle.h > > > @@ -65,7 +65,7 @@ > > >=20 > > > #define BOOT_TARGET_DEVICES(func) \ > > > =20 > > > func(MMC, mmc, 0) \ > > > func(LEGACY_MMC, legacy_mmc, 0) \ > > >=20 > > > - func(UBIFS, ubifs, 0) \ > > > + func(UBIFS, ubifs, 0, rootfs, rootfs) \ > > >=20 > > > func(NAND, nand, 0) > > > =20 > > > #else /* !CONFIG_MTD_RAW_NAND */ > > >=20 > > > @@ -87,8 +87,6 @@ > > >=20 > > > "bootenv=3DuEnv.txt\0" \ > > > "bootfile=3DzImage\0" \ > > > "bootpart=3D0:2\0" \ > > >=20 > > > - "bootubivol=3Drootfs\0" \ > > > - "bootubipart=3Drootfs\0" \ > > >=20 > > > "usbtty=3Dcdc_acm\0" \ > > > "mpurate=3Dauto\0" \ > > > "buddy=3Dnone\0" \ > > >=20 > > > diff --git a/include/configs/omap3_evm.h b/include/configs/omap3_evm.h > > > index eeb9ef8c741a..cc98e03096ab 100644 > > > --- a/include/configs/omap3_evm.h > > > +++ b/include/configs/omap3_evm.h > > > @@ -60,7 +60,7 @@ > > >=20 > > > #define BOOT_TARGET_DEVICES(func) \ > > > =20 > > > func(MMC, mmc, 0) \ > > > func(LEGACY_MMC, legacy_mmc, 0) \ > > >=20 > > > - func(UBIFS, ubifs, 0) \ > > > + func(UBIFS, ubifs, 0, rootfs, rootfs) \ > > >=20 > > > func(NAND, nand, 0) > > > =20 > > > #else /* !CONFIG_MTD_RAW_NAND */ > > >=20 > > > @@ -88,8 +88,6 @@ > > >=20 > > > "bootenv=3DuEnv.txt\0" \ > > > "bootfile=3DzImage\0" \ > > > "bootpart=3D0:2\0" \ > > >=20 > > > - "bootubivol=3Drootfs\0" \ > > > - "bootubipart=3Drootfs\0" \ > > >=20 > > > "optargs=3D\0" \ > > > "nandroot=3Dubi0:rootfs ubi.mtd=3Drootfs rw noinitrd\0" \ > > > "nandrootfstype=3Dubifs rootwait\0" \ > > >=20 > > > diff --git a/include/configs/pcl063.h b/include/configs/pcl063.h > > > index 31b7d07a24cd..c3f7e7eb2c4b 100644 > > > --- a/include/configs/pcl063.h > > > +++ b/include/configs/pcl063.h > > > @@ -71,7 +71,7 @@ > > >=20 > > > #define BOOT_TARGET_DEVICES(func) \ > > > =20 > > > func(MMC, mmc, 0) \ > > >=20 > > > - func(UBIFS, ubifs, 0) \ > > > + func(UBIFS, ubifs, 0, UBI, boot) \ > > >=20 > > > func(PXE, pxe, na) \ > > > func(DHCP, dhcp, na) > > >=20 > > > diff --git a/include/configs/stm32mp15_common.h > > > b/include/configs/stm32mp15_common.h index 6b40cdb01779..68ea56e69c98 > > > 100644 > > > --- a/include/configs/stm32mp15_common.h > > > +++ b/include/configs/stm32mp15_common.h > > > @@ -77,7 +77,7 @@ > > >=20 > > > #endif > > > =20 > > > #ifdef CONFIG_CMD_UBIFS > > >=20 > > > -#define BOOT_TARGET_UBIFS(func) func(UBIFS, ubifs, 0) > > > +#define BOOT_TARGET_UBIFS(func) func(UBIFS, ubifs, 0, UBI, boot) > > >=20 > > > #else > > > #define BOOT_TARGET_UBIFS(func) > > > #endif > > >=20 > > > diff --git a/include/configs/uniphier.h b/include/configs/uniphier.h > > > index f813f88cdd7a..640a29067d85 100644 > > > --- a/include/configs/uniphier.h > > > +++ b/include/configs/uniphier.h > > > @@ -20,7 +20,7 @@ > > >=20 > > > #endif > > > =20 > > > #ifdef CONFIG_CMD_UBIFS > > >=20 > > > -#define BOOT_TARGET_DEVICE_UBIFS(func) func(UBIFS, ubifs, 0) > > > +#define BOOT_TARGET_DEVICE_UBIFS(func) func(UBIFS, ubifs, 0, UBI,=20 boot) > > >=20 > > > #else > > > #define BOOT_TARGET_DEVICE_UBIFS(func) > > > #endif