[U-Boot] [PATCH] tools/env: fix environment alignment tests for block devices

[Max Krummenacher]

Hi Andreas

Am Freitag, den 18.11.2016, 14:26 +0100 schrieb Andreas Fenkart:
> 2016-11-18 11:38 GMT+01:00 Max Krummenacher <max.oss.09 at gmail.com>:
> > 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 <max.krummenacher at toradex.com>
> > ---
> > 
> >  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