From mboxrd@z Thu Jan 1 00:00:00 1970 From: Max Krummenacher Date: Sat, 19 Nov 2016 13:52:56 +0100 Subject: [U-Boot] [PATCH] tools/env: fix environment alignment tests for block devices In-Reply-To: References: <1479465523-319-1-git-send-email-max.krummenacher@toradex.com> Message-ID: <1479559976.5509.8.camel@gmail.com> List-Id: MIME-Version: 1.0 Content-Type: text/plain; charset="us-ascii" Content-Transfer-Encoding: 7bit To: u-boot@lists.denx.de Hi Andreas Am Freitag, den 18.11.2016, 14:26 +0100 schrieb Andreas Fenkart: > 2016-11-18 11:38 GMT+01:00 Max Krummenacher : > > commit 183923d3e412500bdc597d1745e2fb6f7f679ec7 enforces that the > > environment must start at an erase block boundary. > > > > For block devices the sample fw_env.config does not mandate a erase block size > > for block devices. A missing setting defaults to the full env size. > > > > Depending on the environment location the alignment check now errors out for > > perfectly legal settings. > > > > Fix this by defaulting to the standard blocksize of 0x200 for environments > > stored in a block device. > > That keeps the fw_env.config files for block devices working even with that > > new check. > > > > Signed-off-by: Max Krummenacher > > --- > > > > tools/env/fw_env.c | 22 ++++++++++++++++++---- > > 1 file changed, 18 insertions(+), 4 deletions(-) > > > > diff --git a/tools/env/fw_env.c b/tools/env/fw_env.c > > index 3dc0d53..f2126f8 100644 > > --- a/tools/env/fw_env.c > > +++ b/tools/env/fw_env.c > > @@ -1335,10 +1335,25 @@ static int check_device_config(int dev) > > goto err; > > } > > DEVTYPE(dev) = mtdinfo.type; > > There is 'erasesize' in that struct, not sure how reliable that is and > if we shall rely on it. Any experiences on this? > In general we should take that and in case it is wrong the user has to > overwrite it in fw_env.config > > # mtdinfo /dev/mtd5 > mtd5 > Name: spi32766.0 > Type: nor > Eraseblock size: 65536 bytes, 64.0 KiB > Amount of eraseblocks: 8 (524288 bytes, 512.0 KiB) > Minimum input/output unit size: 1 byte > Sub-page size: 1 byte > Character device major/minor: 90:10 > Bad blocks are allowed: false > Device is writable: true > That is exactly the bug I try to fix. (e)MMC/SD cards are not MTD devices, the concept of an erase block does not exist for these devices: # mtdinfo /dev/mmcblk0 libmtd: error!: "/dev/mmcblk0" is not a character device mtdinfo: error!: cannot get information about MTD device "/dev/mmcblk0" error 22 (Invalid argument) > > + if (DEVESIZE(dev) == -1) { > > + /* Assume the erase size is the same as the env-size */ > > + DEVESIZE(dev) = ENVSIZE(dev); > > + /* Assume enough env sectors to cover the environment */ > > + ENVSECTORS(dev) = (ENVSIZE(dev) + DEVESIZE(dev) - 1) / > > + DEVESIZE(dev); > > DIV_ROUND_UP I will try if we are allowed to include the relevant header in this userspace application. Ups, someone defined that macro inside the file already, will use it. > > > + } > > } else { > > uint64_t size; > > DEVTYPE(dev) = MTD_ABSENT; > > > > + if (DEVESIZE(dev) == -1) { > > + /* Assume the erase size to be 512 bytes */ > > + DEVESIZE(dev) = 0x200; > > + /* Assume enough env sectors to cover the environment */ > > + ENVSECTORS(dev) = (ENVSIZE(dev) + DEVESIZE(dev) - 1) / > > + DEVESIZE(dev); > > DIV_ROUND_UP Ok. > > > + } > > + > > /* > > * Check for negative offsets, treat it as backwards offset > > * from the end of the block device > > @@ -1467,10 +1482,9 @@ static int get_config (char *fname) > > DEVNAME(i) = devname; > > > > if (rc < 4) > > - /* Assume the erase size is the same as the env-size */ > > - DEVESIZE(i) = ENVSIZE(i); > > - > > - if (rc < 5) > > + /* Fixup later depending on DEVTYPE */ > > + DEVESIZE(i) = -1; > > I think we can rely on '0', since that is invalid already. Actually that uncovered a bug I introduced. The fixup has to come before the test 'if (DEVOFFSET(dev) % DEVESIZE(dev) != 0)' which uses the values. I will change to 0, aka rely on global variables defaulting to 0. > > > > + else if (rc < 5) > > /* Assume enough env sectors to cover the environment */ > > ENVSECTORS (i) = (ENVSIZE(i) + DEVESIZE(i) - 1) / DEVESIZE(i); > > I guess the !defined(CONFIG_FILE) has the same problem. Let's just > ignore the cases rc < 4,5 here and handle them all in check_config. I will remove the default handling in both cases in favor of handling this later in check_device_config(). Will send a V2 shortly. Regards Max