AW: contributing a failsafe update meachanism for FIS from within ecos applications

["Neundorf, Alexander" <Alexander dot Neundorf at jenoptik dot com>]

> Von: Andrew Lunn [mailto:andrew@lunn.ch]
> 
> > No two bits for "valid or in_progress or empty" is not enough. In
> > the case that power is lost while writing exactly these two bits the
> > state of these two bits is undefined. So the probability that they
> > end up as valid when I actually wanted to write in_progress is 25%,
> > at least >>0. If the valid_flag is 32 bits and only one combination
> > is considered valid, then the probability is 1/(2**32).
> 
> Maybe im missing something here....
> 
> You have three states:
> 
> Empty        11b
> in_progress  01b
> valid        00b
> 
> Since only one bit is ever changed you should always have a defined
> state. The write either happened, or it did not. 

Well, at least the function call will take at least one byte to write. In this one byte only one bit has changed. Nevertheless is it really impossible that if while writing this byte the power goes down, the flash writes something undefined, or the flash received the write command while the power went down and didn't receive the correct data byte and the power is still enough that it writes the incorrect byte ?
If it can happen, it will happen ;-)
I'd really like to use more than two bits for this.

> > Two bits for the version are with my proposed scheme also not
> > enough. The old table will never be touched, the new one will
> > increase the version of the old table by one. So no wrap-around may
> > happen. This won't happen with 32 bits.
> 
> 640K is enought memory for any system.
> 4294967295 is enought IP addresses.
> 512 files in the root directory should be enough.
>
> Why not just do it right and use modular arithmatic to handle wrap
> around?

Well, (2**32) means 4.000.000.000 updates, if one update takes 1 second, this lasts for 136 years, if the flash lasts this long.

So, instead of 

if (fvi1->version_count > fvi0->version_count)
   return 1;

if (fvi1->version_count == ((fvi0->version_count +1) % MAX_VERSION_COUNT)
   return 1; 

?
 
> diff -rbup current.orig/cdl/redboot.cdl current/cdl/redboot.cdl
...
> Setting the default as -1 is not a good idea. Thats the same as the
> primary block. You probably want a requires statement that the
> additional block is different from the primary block just to stop
> people doing this....

Ok, it was a my first try at cdl.
 
> +#define EFIS_MAGIC "$_FisValid_
> 
> Any particular reason for the $_ and _?

To make it look important and obscure ;-)

> +struct fis_valid_info
> +{
> +   unsigned char magic_name[12];
> +   unsigned long valid_flag;
> +   CYG_ADDRESS unused_flash_base;
> +   unsigned long version_count;
> +};
> 
> This does not look good. The version_count is in the same place as the
> size. I expect uses are going to wonder why the size keeps
> increasing. I would try to hide this inside the name.

So how about

//a name consisting of only 7 characters would be even better, but I didn't find a good one
#define EFIS_MAGIC ".FisValid"  //the dot suggests that it is hidden and kind of internal

struct fis_valid_info
{
   char valid_name[10];
   unsigned short valid_flag;  //a cyg_uint16 or something like that should be used here
   unsigned long version_count;
};

?

> Its also probably safer to use the existing struct fis_image_desc if
> you can get it all inside the name. It means less trouble if somebody
> comes along and adds a new field at the beginning etc.
> 
> +    memcpy(fvi->magic_name, EFIS_MAGIC, 12);
> 
> Using a memcpy on a string is not a good idea. Its much safer to do a
> strcpy or strncpy.

Well, I just thought usually strcpy() is considered unsafe since you rely on the terminating 0. That's why I chose memcpy() for safety ;-) If you think I can change it.
 
> +        if ((memcmp(fvi0.magic_name, EFIS_MAGIC, 12)==0) || 
> (memcmp(fvi1.magic_name, EFIS_MAGIC, 12)==0)) {
> +           if (get_valid_buf(&fvi0, &fvi1)==0)
> +              fis_addr=fis_addr0;
> 
> This looks ugly. I would put the memcmp (which should be strncpy())
> inside get_valid_buf().

This is done this way so that get_valid_buf() is only called if such an "enhanced" fis table is detected. If there is such a table, then get_valid_buf() should be called, otherwise not. 
 
> fis init should also probably erase the secondary fis directory just
> to be safe.

Ok.

Bye
Alex