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

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

[Neundorf, Alexander]

> Von: Andrew Lunn [mailto:andrew@lunn.ch]
...
> > And that's why I changed from 
> > unsigned short valid_flag;
> > to 
> > unsigned char valid_flag1;
> > unsigned char valid_flag2;
> 
> The pointer to the structure is still unaligned which for me makes it
> potentially dangerous when access members of the structure.

You mean the structure i.e. the buffer in which the fis table is read itself ?
Yes, this is indeed right.
 
> > Maybe something with a union would make it more explicit ?
> > 
> > union
> > {
> >    struct fis_image_desc img;
> >    struct fis_valid_info valid;
> > } give_me_a_name;
> 
> I was thinking along the same lines, but slightly differently...
> 
> struct fis_image_desc {
>     union {
>         name[16];
>         struct fis_valid_info valid;
>     } u;    
>     CYG_ADDRESS   flash_base;    // Address within FLASH of image
>     CYG_ADDRESS   mem_base;      // Address in memory where 
> it executes
>     unsigned long size;          // Length of image
>     CYG_ADDRESS   entry_point;   // Execution entry point
>     unsigned long data_length;   // Length of actual data
>     unsigned char 
> _pad[CYGNUM_REDBOOT_FIS_DIRECTORY_ENTRY_SIZE-FIS_IMAGE_DESC_SI
> ZE_UNPADDED];
>     unsigned long desc_cksum;    // Checksum over image descriptor
>     unsigned long file_cksum;    // Checksum over image data
> };
> 
> The problem with this is that its much more invasive. You have to
> change all current references of name to u.name.

Would this be a problem ? I'd be willing to implement this change.
Ok, so what's your final word ?
I'll send a patch with this tonight/tomorrow.

> > > > This doesn't only apply to the redboot code, but even 
> more to the
> > > > fisfs implementation, there this has to be done more often.
> > > 
> > > I took a quick look at your fisfs code. We need to talk 
> about that...
> > 
> > Yes, sure :-)
> 
> Im note sure your general approach is right. I would put all the logic
> for updating FIS into fis_update_directory() 

fis_update_directory() is not enough. There have to be two two functions, so that I can mark the beginning of the manipulation of the data on the flash and after this is done the end.
If I am able to do this, it is possible to check (e.g. during mount()) whether there has been an update process interrupted. 

> and extend the current
> virtual vector support to allow more access to the FIS information.
> Putting code into a seperate package which manipulates the FIS
> directory is probably not a good idea in terms of maintainability. If
> all the code is in redboot it will be much less of a problem.

This is right. OTOH redboot doesn't really need to be able to perform the safe updates. If somebody accesses redboot directly (i.e. a developer), he should know what he's doing.
So all the shared code currently consists of fis.h (which is identical) and the function for finding the valid table. 
How about simply sharing the source files ?
How does the virtual vector stuff work ?

Bye
Alex

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

[Neundorf, Alexander]

Hi Andrew,

> Von: Andrew Lunn [mailto:andrew@lunn.ch]
> 
> On Tue, Oct 26, 2004 at 06:37:26PM +0200, Neundorf, Alexander wrote:
> > Hi,
> > 
> > here comes the next try, I hope I fixed all known issues.
> 
> +#ifdef CYGSEM_REDBOOT_FLASH_LOCK_SPECIAL
> +    // Ensure [quietly] that the directory is locked after the update
> +    flash_lock((void *)fis_addr, flash_block_size, (void 
> **)&err_addr);
> +#endif
> +}
> 
> This should be (void *)redundant_fis_addr. My error i think....

I hope you had a nice holiday :-)
Climbing'n stuff ?

Well, how about applying the first part of my patch ?
The copyright assignment is on the way (since wednesday or thursday last week), if it didn't arrive yet, I can also send it via fax if this would be of any help.

After this we can continue with the other parts (ecos fs and VV interface).

Bye
Alex

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

[Neundorf, Alexander]

...
> > 
> > struct item
> > {
> >    int key;
> >    void* value;
> > };
> 
> Why do you need the key? Surely you know what is being passed to each
> VV call. 
> 
> > int do_something_VV(int operation, int item_count, struct 
> item* list_of_items); ?
> 
> O, I C. You want to do complex list manipulation rather than simply do
> lots of calls. KISS.
> 
> > One VV for: 
> > -version
> > -number of images
> > -erase entry
> > -create
> > -create with backup, maybe also just as a parameter to the one above
> 
> I still don't see why you need this. Its just added complexity. 

I'm all for KISS.
Here I see KISS the other way round. With single calls I can mess around in various entries. If it is all combined in one call (create with all parameters) this is not possible.
If I have single calls the application can effectively modify the fis table as it wants to and create invalid entries. With having atomic functions which take all parameters at once this is not possible.
 
> > -changesDone  (close)
> > -get all fields for image i  (stat)
> 
> Stat only needs one field, the length.

...and the name. But sometimes the entry point and the crc might also be interesting.

> There is already a VV call to
> return this. Might as well use it rather than add something new.
> 
> You seem to be missing
> 
> set length
> set entry point
> set flash_base
> 
> which you need to call after doing a create. Plus

"you need to call" from the application. If I don't call them or call them for other entries or .... a lot of things can go wrong. It is much easier if it is combined into one function call.

With the list they all can be given at once.
And with the list your main point, that the app doesn't have to care about the data structures used in redboot is fulfilled.

Bye
Alex

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

[Neundorf, Alexander]

...
> > IMO this is the same what we are doing here. I really would want to
> > make sure that the application which wants to write a new firmware
> > image knows with which "interface version" of redboot it is working,
> > and uses this interface correctly instead of relying on unknown
> > default parameters.
> 
> Well, add a version VV. If the application finds reboot is running a
> different version of the interface than what the application supports
> it can then decide if it wants to accept the defaults, or simply abort
> the upgrade.
> 
>         Andrew

Ok, how about something like this:

struct item
{
   int key;
   void* value;
};

int do_something_VV(int operation, int item_count, struct item* list_of_items); ?

This way I can at give all parameters in one function call while still having the property of not having to know the actual data structure used in redboot.
And also consuming less code and less runtime and easier to understand logic (IMO).
One VV for: 
-version
-number of images
-erase entry
-create
-create with backup, maybe also just as a parameter to the one above
-changesDone  (close)
-get all fields for image i  (stat)

Without the last one I have to call 
noi=number_of_images()
for (i=0; i<noi; i++)
{
   n=get_name(i)             // <- this one has to be added
   l=get(name, id_data_length)  // <- exists already
   f=get(name, id_flash_base
   ...
}

with a combined call I can

noi=number_of_images()
for (i=0; i<noi; i++)
{
   get_all(i, list)       // <- this one has to be added
   //use data from list
}

which uses less code and less runtime, i.e. much less string comparisons and fis table iterations, and one new VV for retrieving the information by the index has to be added anyway.

Ok with you ?

Bye
Alex

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

[Neundorf, Alexander]

Hi,

> Von: ecos-devel-owner@sources.redhat.com
> Now creat() is tricky. You don't know enough in the application to
> reliably create new entries. Using the filesystem metaphore you don't
> know where files are stored on the storage medium. So how can you pick
> the empty space? Maybe somebody has used the serial port and added a
> new FIS entry which your application does not know about? You then
> create the new one right in the middle? The only simple way around
> this is to ask redboot to allocate the space. You set the data_length
> field and then ask redboot to find an empty location big enough. This
> might work, or it might not. And when it does not, you are in
> trouble. So i think create is fundementaly unreliable unless its done
> from the redboot prompt by a human.

Ok. That's why I want to give the create (or set_config) function basically the same parameters a human at a prompt would give, i.e. everything manually and inside only checking that nothing overlaps.
 
> Now having said that, if i were to implement create, which i would
> not, i would implement the creation of a FIS entry using
> cyg_io_set_config(). I suggest you read the source code and work out
> how that works. (This is one place you can pass a structure, since its
> internal to the application.)

Ok.
 
...
> > which has to know all the fields required for creating a correct fis
> > image, I don't see another way (except giving all parameter
> > key-value based one by one to redboot, but I don't see a real
> > difference: if a new redboot in the future should support additional
> > informations, then an old application won't be able to supply these,
> > no matter whether via key-value or this struct. Not being able to
> > provide all parameters means not being able to provide then
> > potentially essential new parameters and by this not being able to
> > create correct images).
> 
> OK. How do you know when the applicaton and redboot are using the same
> version of the structure? 

...the same applies e.g. for the existing flash_cfg VV IMO.
 
> By having individual setters you don't have this problem. You have one
> VV which does "create a new FIS entry which name XYZ, and set the
> other parameters to default values". You then have a number of VV
> functions that allow you to set the other parameters one by one. If
> somebody were to add a new parameter, no problem, it uses the default
> value if the application does not set it. Once the application has set
> everything it wants to set, it then calls the commit VV which actually
> writes it to FLASH.

And you are sure that the new default parameter will always be what the user wants without any unexpected side effects ?

I see it differently. Usually (e.g. linux, windows, ...) calls into libraries or calls into the kernel have a fixed interface. If the interface to the kernel changes, libc has to adapt to this change. If a function in libc changes, the application using this function has to be adapted. Now the developers (kernel, libc, others) do their best to avoid those changes.
IMO this is the same what we are doing here. I really would want to make sure that the application which wants to write a new firmware image knows with which "interface version" of redboot it is working, and uses this interface correctly instead of relying on unknown default parameters.
(that's why several people don't like the ioctl()-style functions in the linux kernel and are trying to reduce their number or at least not to increase their number too much).

So I would say a firmware which wants to use fis for writing from applications has to use the same fis version as the underlying redboot does. It should fail with an assertion if the versions don't match. The version matching could be done by changing to a new value for the VV operation.
If you really think that this is not required I can implement the key-value scheme but I don't think it would be the better way. (after all the interface doesn't change so often, fis.h has currently version 1.6)

Bye
Alex

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

[Neundorf, Alexander]

> Von: Andrew Lunn [mailto:andrew@lunn.ch]
> 
> On Thu, Oct 28, 2004 at 01:32:46PM +0200, Neundorf, Alexander wrote:
> > For this I'd like to add a function which simply returns a pointer
> > to the current valid fis table. Then this can be iterated (and
> > modified) in "userspace".
> 
> I don't like that. You are spreading around the knowledge of what the
> FIS image looks like, how you operate on it etc. Thats bad engineering
> practice. There currently exists an abstraction on top of this which
> does most of what you need. Just extend the abstraction a little.

You mean hal_if.c flash_fis_op() ?
Instead of returning struct fis_image_desc it returns the members of a fis_image_desc one by one. So the app still has to know which members a fis_image_desc has.
How about a VV which returns the number of fis_image_desc's in the table and a VV something like this:

int fill_image_desc(int index, struct fis_image_desc* img)
{
   /* fill img from the contents of the fis table, can be memcpy(),
   could also be a member-by-member translation */
   memcpy(img, fis_table[index], sizeof(fis_image_desc));
   return 0;
}

This way one would not have to call 8 functions to collect all members for one entry. And there would be the possibility to insert a "translation" in get_image_desc(), so that the struct returned there and the layout on the flash don't have to be identical.
But since currently fis_image_desc is the used layout, it would IMO make sense to use this struct also for this purpose. Otherwise a new struct with more or less the same members has to be defined.

Some similar function for changing the entries and the two functions for starting and finishing the update. Still the contents of the entries given to the modification functions (flash_base etc.) would have to be filled by the application, preferably with manually defined fixed addresses. This is one of the main points, we (the user/developer) have the chance to define exactly at which address on the flash the image will be placed, we can rely on this, there is no magic happening in between which collects garbage or searches free chunks or whatever. It is simple and easy to understand what's going on. That's essential for our purpose.
 
> > I didn't plan to open files for writing. They can only be opened for
> > reading, and additionally there is a set of functions (eraseImage(),
> > createImage()) which can modify the contents of the fis. If any file
> > is opened when one of these functions is called, it returns with an
> > error. IMO the fisfs shouldn't try to act like a real RW file
> > system, for this we have jffs2, but like an RO file system,
> > accompanied by some utility functions to update the contents of this
> > RO filesystem.
> 
> I guess your createImage() call accepts the complete image. Many
> systems don't have enought RAM to be able to hold the complete image
> when doing an upgrade, but they do have enought RAM to hold one flash
> block. What they want to do is download the new image from a server a
> packet at a time and pass it to the filesystem to write to the
> flash. The filesystem caches the writes until it has a complete flash
> block and then writes it to flash.
> 
> How do you handle this case with your createImage() call?  

Currently not at all and for our solution we definitely won't put any filesystem cache/logic  in between (I already mentioned earlier ;-)
Anyway, if the createImage() function doesn't come from redboot (as you suggest if I understand correctly), there's nothing which hinders implementing writing block by block. 
 
> My solution seems much more natural if you are using the filesystem
> metaphore.
> 
> > Well, actually mount() is the point where the interesting things
> > happen, i.e. deciding which fis table to use I'd say.
> 
> Why. Redboot already knows that. Its just another example of you

Well, from the application POV the mount() call decides about the valid fis table. For the application it doesn't matter whether this is done in application code, via VV in redboot by examing the flash blocks again or via a VV by redboot just returning what redboot already knows. I'm also just not yet so deep into VV's to know everything what can be done there.

So how about this:
erase:
app to redboot: I want to erase foo
redboot: erases foo from the fis table and marks it as in progress
app: erases foo contents from the flash
app to redboot: I'm done with it
redboot: mark it valid

create:
app to redboot: I want to create "foo" at this and that address and that's the crc (all parameters preferably transferred in one function call, be it a fis_image_desc or something else)
redboot: creates foo in the fis table and marks it as in progress
app: writes foo contents on the flash, all at once or block by block
app to redboot: I'm done with it
redboot: mark it valid

create with backup:
app to redboot: I want to create "foo" at this and that address and that's the crc, and please rename existing foo to foo~
redboot: creates foo in the fis table, renames previous foo to foo~ and marks the table as in progress
app: writes new foo contents on the flash
app to redboot: I'm done with it
redboot: mark it valid

This last one renaming step is the key for the firmware update, it enables safe updating without having to modify the flash config boot script. If the process succeeds, redboot will start the new image the next time, if it fails, it will still start the old image again since still the old table will be valid.

Bye
Alex

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

[Neundorf, Alexander]

Hi,

> Von: Andrew Lunn [mailto:andrew@lunn.ch]
> 
...
> Anything that deals with the layout of the FIS directory in flash
> should be handled by redboot.
...
 
> For opendir() and readdir() you need to add a way to enumerate the
> names of images in the FIS. Once you have the names the rest is easy.

For this I'd like to add a function which simply returns a pointer to the current valid fis table. Then this can be iterated (and modified) in "userspace".

In my current fisfs I have startUpdateDirectory(), which 
-for "old" fis: does nothing
-for "new" fis: writes the modified fis table to the flash but marked as in progress

and finishUpdateDirectory() which
-for "old" fis: writes the modified fis table to the flash
-for "new" fis: markes the already written fis table as valid


So the modification process would be as follows:

-application: modify the fis table in RAM (including addresses and crc)
-redboot: startUpdateDirectory()
-application: write/erase the image on flash
-redboot: finishUpdateDirectory()

> The CRC over the image is a bit tricky. You probably need to add VV
> calls to recalculate the CRC over an image, and to test the CRC over
> an image. When a file that which has been opened for writing is
> closed, you call the recalculate function followed by commit so that
> the CRC in FLASH in the FIS directory matches the image. When open is
> called, you first call the crc check function, and if the CRC fails
> you only allow the file to be open with O_TRUNC so effectively erasing
> the corrupt content.

I didn't plan to open files for writing. They can only be opened for reading, and additionally there is a set of functions (eraseImage(), createImage()) which can modify the contents of the fis. If any file is opened when one of these functions is called, it returns with an error. IMO the fisfs shouldn't try to act like a real RW file system, for this we have jffs2, but like an RO file system, accompanied by some utility functions to update the contents of this RO filesystem.
 
> creat is interesting. How do you control the size and location? The

The mentioned special createImage() call, same for erasing.

> standard filesystem API has no concept of the application specifing
> where a file should be placed on the storage medium and how big the
> maximum size of the file can be. Probably the easiest solution is to
> simply not allow creat. And if you don't have creat then erase is
> probably no use either. You have to create and erase the files using
> RedBoots CLI where you can specify all these parameters. The other
> option is to use cyg_set_config() on the filesystem to do a create,
> passing the maximum size of the image, entry point etc, and letting
> redboot find a space in the flash to put it. But there are all sorts
> of horrible fragmentation issues to worry about.

Yes, I really want to avoid this under all circumstances and that's why leave it to the user to give a start address and length to the createImage() function. This function then only checks that the given addresses don't overlap existing images.
 
> reading and writing can all be done outside of redboot. You need to
> think about how to handle flash blocks. You probably need a block
> cache where you cache your writes until you have a full flash block
> which you can then erase and write. seeking off the end of the
> allocated space will not be allowed.

Simply calling flash_write() on the complete image buffer works without problems for me so far. As I mentioned, there's no real write() function, only a createImage() function.
 
> mount is nearly a NOP. All you need to do is see if the redboot

Well, actually mount() is the point where the interesting things happen, i.e. deciding which fis table to use I'd say.

Bye
Alex

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

[Neundorf, Alexander]

> Von: Andrew Lunn [mailto:andrew@lunn.ch]
...
> > attached.  It would be nice if the fisfs-implementation could get
> > the information about the two flash blocks reserved for the two fis
> > tables somehow from redboot during runtime. Is there a way to do
> > this ?
> 
> You should take a look what the virtual vectors supports. There are a
> couple of examples of it being used in:
> 
> packages/io/flash/current/src/flashiodev.c:flashiodev_init() and see
> packages/hal/common/current/src/hal_if.c:flash_fis_op().
> 
> As i said before, i think this is the way the FISFS should work,
> redboot doing most of the work using VV as the interface between the
> application and redboot.
> 
>         Andrew

Ok, I'll have a look.
What would you say needs to be transferred to redboot ?
For implementing an ecos fisfs I need a bunch of functions.
open(), read(), close(), opendir(), readdir() are more or less trivial I'd say and transferring these functions to redboot wouldn't make much sense.

I guess what would make sense would be at least the mount() (i.e. finding the valid fis table and returning a pointer to it) call, and probably the eraseImage() and createImage() functions.
What do you think ?

Bye
Alex

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

[Neundorf, Alexander]

[-- Attachment #1: Type: text/plain, Size: 1833 bytes --]

Hi,

here comes the next try, I hope I fixed all known issues.

Main change:

#define CYG_REDBOOT_RFIS_VALID_MAGIC_LENGTH 10
#define CYG_REDBOOT_RFIS_VALID_MAGIC ".FisValid"  //exactly 10 bytes
#define CYG_REDBOOT_RFIS_VALID       (0xa5)
#define CYG_REDBOOT_RFIS_IN_PROGRESS (0xfd)
#define CYG_REDBOOT_RFIS_EMPTY       (0xff)

struct fis_valid_info
{
   char magic_name[CYG_REDBOOT_RFIS_VALID_MAGIC_LENGTH];
   unsigned char valid_flag[2]; //this should be safe for all alignment issues
   unsigned long version_count;
};
 
struct fis_image_desc {
    union
    {
      unsigned char name[16];      // Null terminated name
      struct fis_valid_info valid_info;
    } u;
    CYG_ADDRESS   flash_base;    // Address within FLASH of image
    CYG_ADDRESS   mem_base;      // Address in memory where it executes
    unsigned long size;          // Length of image


So now the valid_info is explicitely related with the name field. There are also two asserts() in the code which check the size of fis_valid_info and try to check the alignment.
The only member which might have alignment issues is version_count. If fis_image_desc is accessed via the pointer work_buf, there is no problem, it is aligned exactly the same as flash_base and friends. If a fis_image_desc is created on the stack the compiler should make sure that all members of the structure are aligned correctly. This is ensured by the two asserts().

Otherwise not much news. Except the copyright issue (work in progress), which issues are left now ?

An adapted version of my current fisfs implementation is also attached.
It would be nice if the fisfs-implementation could get the information about the two flash blocks reserved for the two fis tables somehow from redboot during runtime. Is there a way to do this ?

Bye
Alex

[-- Attachment #2: redboot-redundant-fis.patch --]
[-- Type: application/octet-stream, Size: 17117 bytes --]

diff -rbup current.orig/ChangeLog current/ChangeLog
--- current.orig/ChangeLog	Wed Oct 13 23:22:53 2004
+++ current/ChangeLog	Tue Oct 26 18:29:22 2004
@@ -1,3 +1,13 @@
+2004-10-22	Alexander Neundorf <Alexander.Neundorf@jenoptik.com>
+                Andrew Lunn        <andrew.lunn@ascom.ch>
+
+	* include/fis.h
+	* src/flash.c: Added basic support for a redundant FIS directory.
+	Currently there is no way to actually write a redundant directory,
+	but if such a directory where to exist, we can check if the
+	primary directory is valid and if not use the redundant one
+	instead.
+	
 2004-10-10	Iztok Zupet	 <iz@elsis.si>
 
 	* cdl/redboot.cdl: added CYGSEM_REDBOOT_DISK_IDE_VMWARE option.
Only in current: ChangeLog~
diff -rbup current.orig/cdl/redboot.cdl current/cdl/redboot.cdl
--- current.orig/cdl/redboot.cdl	Wed Oct 13 23:22:53 2004
+++ current/cdl/redboot.cdl	Tue Oct 26 17:58:01 2004
@@ -638,6 +638,32 @@ cdl_package CYGPKG_REDBOOT {
                       Negative block numbers count backwards from the last block.
                       eg 2 means block 2, -2 means the last but one block."
                 }
+
+                cdl_component CYGOPT_REDBOOT_REDUNDANT_FIS {
+                    display         "Redundant Flash Image System Directory Support"
+                    default_value   0
+                    requires { 0 == CYGSEM_REDBOOT_FLASH_COMBINED_FIS_AND_CONFIG }
+                    description "
+                        This option enables the use of a redundant FIS 
+                        directory within RedBoot.  If enabled a flash block 
+                        will be reserved for a second copy of the fis 
+                        directory. Doing this allow for power failure safe 
+                        updates of the directory by the application."
+ 
+                    cdl_option CYGNUM_REDBOOT_FIS_REDUNDANT_DIRECTORY_BLOCK {
+                        display         "Flash block containing the backup Directory"
+                        flavor          data
+                        default_value   (-3)
+                        requires { CYGNUM_REDBOOT_FIS_REDUNDANT_DIRECTORY_BLOCK !=
+                                   CYGNUM_REDBOOT_FIS_DIRECTORY_BLOCK }    
+                        description "
+                           Which block of flash should hold the redundant 
+                           directory information. Positive numbers are 
+                           absolute block numbers. Negative block numbers 
+                           count backwards from the last block. eg 2 means
+                           block 2, -2 means the last but one block."
+                    }
+                }
     
                 cdl_option CYGOPT_REDBOOT_FIS_RESERVED_BASE {
                     display         "Pseudo-file to describe reserved area"
Only in current/cdl: redboot.cdl~
diff -rbup current.orig/include/fis.h current/include/fis.h
--- current.orig/include/fis.h	Sat Aug 24 13:20:55 2002
+++ current/include/fis.h	Tue Oct 26 17:56:07 2004
@@ -58,11 +58,35 @@
 #ifdef CYGOPT_REDBOOT_FIS
 #include <cyg/infra/cyg_type.h>
 
+// These will need to move somewhere else when the fisfs needs to use
+// the following definitions.
+#ifdef CYGOPT_REDBOOT_REDUNDANT_FIS
+#define CYG_REDBOOT_RFIS_VALID_MAGIC_LENGTH 10
+#define CYG_REDBOOT_RFIS_VALID_MAGIC ".FisValid"  //exactly 10 bytes
+
+#define CYG_REDBOOT_RFIS_VALID       (0xa5)
+#define CYG_REDBOOT_RFIS_IN_PROGRESS (0xfd)
+#define CYG_REDBOOT_RFIS_EMPTY       (0xff)
+
+struct fis_valid_info
+{
+   char magic_name[CYG_REDBOOT_RFIS_VALID_MAGIC_LENGTH];
+   unsigned char valid_flag[2]; //this should be safe for all alignment issues
+   unsigned long version_count;
+};
+#endif // CYGOPT_REDBOOT_REDUNDANT_FIS
+
 #define FIS_IMAGE_DESC_SIZE_UNPADDED \
   (16 + 4 * sizeof(unsigned long) + 3 * sizeof(CYG_ADDRESS))
 
 struct fis_image_desc {
+    union
+    {
     unsigned char name[16];      // Null terminated name
+#ifdef CYGOPT_REDBOOT_REDUNDANT_FIS
+       struct fis_valid_info valid_info;
+#endif
+    } u;
     CYG_ADDRESS   flash_base;    // Address within FLASH of image
     CYG_ADDRESS   mem_base;      // Address in memory where it executes
     unsigned long size;          // Length of image
Only in current/include: fis.h~
diff -rbup current.orig/src/flash.c current/src/flash.c
--- current.orig/src/flash.c	Wed Sep  1 23:21:30 2004
+++ current/src/flash.c	Tue Oct 26 18:16:16 2004
@@ -169,6 +169,9 @@ int flash_block_size, flash_num_blocks;
 #ifdef CYGOPT_REDBOOT_FIS
 void *fis_work_block;
 void *fis_addr;
+#ifdef CYGOPT_REDBOOT_REDUNDANT_FIS
+void *redundant_fis_addr;
+#endif
 int fisdir_size;  // Size of FIS directory.
 #endif
 #ifdef CYGSEM_REDBOOT_FLASH_CONFIG
@@ -233,8 +236,8 @@ fis_lookup(char *name, int *num)
 
     img = (struct fis_image_desc *)fis_work_block;
     for (i = 0;  i < fisdir_size/sizeof(*img);  i++, img++) {
-        if ((img->name[0] != (unsigned char)0xFF) && 
-            (strcasecmp(name, img->name) == 0)) {
+        if ((img->u.name[0] != (unsigned char)0xFF) &&
+            (strcasecmp(name, img->u.name) == 0)) {
             if (num) *num = i;
             return img;
         }
@@ -273,6 +276,57 @@ fis_update_directory(void)
     fis_endian_fixup(fis_work_block);
 }
 
+#ifdef CYGOPT_REDBOOT_REDUNDANT_FIS
+
+int
+fis_get_valid_buf(struct fis_image_desc* img0, struct fis_image_desc* img1)
+{
+   if (strncmp(img1->u.valid_info.magic_name, CYG_REDBOOT_RFIS_VALID_MAGIC, CYG_REDBOOT_RFIS_VALID_MAGIC_LENGTH)!=0)  //buf0 must be valid
+      return 0;
+   else if (strncmp(img0->u.valid_info.magic_name, CYG_REDBOOT_RFIS_VALID_MAGIC, CYG_REDBOOT_RFIS_VALID_MAGIC_LENGTH)!=0)  //buf1 must be valid
+      return 1;
+
+   //magic is ok for both, now check the valid flag
+   if ((img1->u.valid_info.valid_flag[0]!=CYG_REDBOOT_RFIS_VALID)
+       || (img1->u.valid_info.valid_flag[1]!=CYG_REDBOOT_RFIS_VALID)) //buf0 must be valid
+      return 0;
+   else if ((img0->u.valid_info.valid_flag[0]!=CYG_REDBOOT_RFIS_VALID)
+       || (img0->u.valid_info.valid_flag[1]!=CYG_REDBOOT_RFIS_VALID)) //buf1 must be valid
+      return 1;
+
+   //now check the version
+   if (img1->u.valid_info.version_count == (img0->u.valid_info.version_count+1)) //buf1 must be valid
+      return 1;
+
+   return 0;
+}
+
+void
+fis_erase_redundant_directory(void)
+{
+    int stat;
+    void *err_addr;
+
+#ifdef CYGSEM_REDBOOT_FLASH_LOCK_SPECIAL
+    // Ensure [quietly] that the directory is unlocked before trying
+    // to update
+    flash_unlock((void *)redundant_fis_addr, flash_block_size,
+                 (void **)&err_addr);
+#endif
+    if ((stat = flash_erase(redundant_fis_addr, flash_block_size,
+                            (void **)&err_addr)) != 0) {
+         diag_printf("Error erasing FIS directory at %p: %s\n",
+                     err_addr, flash_errmsg(stat));
+    }
+#ifdef CYGSEM_REDBOOT_FLASH_LOCK_SPECIAL
+    // Ensure [quietly] that the directory is locked after the update
+    flash_lock((void *)fis_addr, flash_block_size, (void **)&err_addr);
+#endif
+}
+
+#endif
+
+
 static void
 fis_init(int argc, char *argv[])
 {
@@ -301,12 +355,23 @@ fis_init(int argc, char *argv[])
     redboot_image_size = flash_block_size > MIN_REDBOOT_IMAGE_SIZE ? 
                          flash_block_size : MIN_REDBOOT_IMAGE_SIZE;
 
-    // Create a pseudo image for RedBoot
     img = (struct fis_image_desc *)fis_work_block;
     memset(img, 0xFF, fisdir_size);  // Start with erased data
+
+#ifdef CYGOPT_REDBOOT_REDUNDANT_FIS
+    //create the valid flag entry
+    memset(img, 0, sizeof(struct fis_image_desc));
+    strcpy(img->u.valid_info.magic_name, CYG_REDBOOT_RFIS_VALID_MAGIC);
+    img->u.valid_info.valid_flag[0]=CYG_REDBOOT_RFIS_VALID;
+    img->u.valid_info.valid_flag[1]=CYG_REDBOOT_RFIS_VALID;
+    img->u.valid_info.version_count=0;
+    img++;
+#endif
+
+    // Create a pseudo image for RedBoot
 #ifdef CYGOPT_REDBOOT_FIS_RESERVED_BASE
     memset(img, 0, sizeof(*img));
-    strcpy(img->name, "(reserved)");
+    strcpy(img->u.name, "(reserved)");
     img->flash_base = (CYG_ADDRESS)flash_start;
     img->mem_base = (CYG_ADDRESS)flash_start;
     img->size = CYGNUM_REDBOOT_FLASH_RESERVED_BASE;
@@ -315,7 +380,7 @@ fis_init(int argc, char *argv[])
     redboot_flash_start = (CYG_ADDRESS)flash_start + CYGBLD_REDBOOT_FLASH_BOOT_OFFSET;
 #ifdef CYGOPT_REDBOOT_FIS_REDBOOT
     memset(img, 0, sizeof(*img));
-    strcpy(img->name, "RedBoot");
+    strcpy(img->u.name, "RedBoot");
     img->flash_base = redboot_flash_start;
     img->mem_base = redboot_flash_start;
     img->size = redboot_image_size;
@@ -328,7 +393,7 @@ fis_init(int argc, char *argv[])
     redboot_flash_start = (CYG_ADDRESS)flash_start + CYGNUM_REDBOOT_FIS_REDBOOT_POST_OFFSET;
 #endif
     memset(img, 0, sizeof(*img));
-    strcpy(img->name, "RedBoot[post]");
+    strcpy(img->u.name, "RedBoot[post]");
     img->flash_base = redboot_flash_start;
     img->mem_base = redboot_flash_start;
     img->size = redboot_image_size;
@@ -338,7 +403,7 @@ fis_init(int argc, char *argv[])
 #ifdef CYGOPT_REDBOOT_FIS_REDBOOT_BACKUP
     // And a backup image
     memset(img, 0, sizeof(*img));
-    strcpy(img->name, "RedBoot[backup]");
+    strcpy(img->u.name, "RedBoot[backup]");
     img->flash_base = redboot_flash_start;
     img->mem_base = redboot_flash_start;
     img->size = redboot_image_size;
@@ -348,7 +413,7 @@ fis_init(int argc, char *argv[])
 #if defined(CYGSEM_REDBOOT_FLASH_CONFIG) && defined(CYGHWR_REDBOOT_FLASH_CONFIG_MEDIA_FLASH)
     // And a descriptor for the configuration data
     memset(img, 0, sizeof(*img));
-    strcpy(img->name, "RedBoot config");
+    strcpy(img->u.name, "RedBoot config");
     img->flash_base = (CYG_ADDRESS)cfg_base;
     img->mem_base = (CYG_ADDRESS)cfg_base;
     img->size = cfg_size;
@@ -356,12 +421,22 @@ fis_init(int argc, char *argv[])
 #endif
     // And a descriptor for the descriptor table itself
     memset(img, 0, sizeof(*img));
-    strcpy(img->name, "FIS directory");
+    strcpy(img->u.name, "FIS directory");
     img->flash_base = (CYG_ADDRESS)fis_addr;
     img->mem_base = (CYG_ADDRESS)fis_addr;
     img->size = fisdir_size;
     img++;
 
+    //create the entry for the redundant fis table
+#ifdef CYGOPT_REDBOOT_REDUNDANT_FIS
+    memset(img, 0, sizeof(*img));
+    strcpy(img->u.name, "Redundant FIS");
+    img->flash_base = (CYG_ADDRESS)redundant_fis_addr;
+    img->mem_base = (CYG_ADDRESS)redundant_fis_addr;
+    img->size = fisdir_size;
+    img++;
+#endif
+
 #ifdef CYGOPT_REDBOOT_FIS_DIRECTORY_ARM_SIB_ID
     // FIS gets the size of a full block - note, this should be changed
     // if support is added for multi-block FIS structures.
@@ -475,6 +550,9 @@ fis_init(int argc, char *argv[])
 #endif
     }
     fis_update_directory();
+#ifdef CYGOPT_REDBOOT_REDUNDANT_FIS
+    fis_erase_redundant_directory();
+#endif
 }
 
 static void
@@ -523,7 +601,7 @@ fis_list(int argc, char *argv[])
         lowest_addr = 0xFFFFFFFF;
         img = (struct fis_image_desc *) fis_work_block;
         for (i = 0;  i < fisdir_size/sizeof(*img);  i++, img++) {
-            if (img->name[0] != (unsigned char)0xFF) {
+            if (img->u.name[0] != (unsigned char)0xFF) {
                 if ((img->flash_base > last_addr) && (img->flash_base < lowest_addr)) {
                     lowest_addr = img->flash_base;
                     image_found = true;
@@ -534,7 +612,7 @@ fis_list(int argc, char *argv[])
         if (image_found) {
             img = (struct fis_image_desc *) fis_work_block;
             img += image_indx;
-            diag_printf("%-16s  0x%08lX  0x%08lX  0x%08lX  0x%08lX\n", img->name, 
+            diag_printf("%-16s  0x%08lX  0x%08lX  0x%08lX  0x%08lX\n", img->u.name,
                         img->flash_base, 
 #ifdef CYGSEM_REDBOOT_FIS_CRC_CHECK
                         show_cksums ? img->file_cksum : img->mem_base, 
@@ -572,7 +650,7 @@ find_free(struct free_chunk *chunks)
     fis_read_directory();
     img = (struct fis_image_desc *) fis_work_block;
     for (i = 0;  i < fisdir_size/sizeof(*img);  i++, img++) {
-        if (img->name[0] != (unsigned char)0xFF) {
+        if (img->u.name[0] != (unsigned char)0xFF) {
             // Figure out which chunk this is in and split it
             for (idx = 0;  idx < num_chunks;  idx++) {
                 if ((img->flash_base >= chunks[idx].start) && 
@@ -836,8 +914,8 @@ fis_create(int argc, char *argv[])
         diag_printf("   must be 0x%x aligned\n", flash_block_size);
         return;
     }
-    if (strlen(name) >= sizeof(img->name)) {
-        diag_printf("Name is too long, must be less than %d chars\n", (int)sizeof(img->name));
+    if (strlen(name) >= sizeof(img->u.name)) {
+        diag_printf("Name is too long, must be less than %d chars\n", (int)sizeof(img->u.name));
         return;
     }
     if (!no_copy) {
@@ -902,7 +980,7 @@ fis_create(int argc, char *argv[])
         // If not image by that name, try and find an empty slot
         img = (struct fis_image_desc *)fis_work_block;
         for (i = 0;  i < fisdir_size/sizeof(*img);  i++, img++) {
-            if (img->name[0] == (unsigned char)0xFF) {
+            if (img->u.name[0] == (unsigned char)0xFF) {
                 break;
             }
         }
@@ -931,7 +1009,7 @@ fis_create(int argc, char *argv[])
     if (prog_ok) {
         // Update directory
         memset(img, 0, sizeof(*img));
-        strcpy(img->name, name);
+        strcpy(img->u.name, name);
         img->flash_base = flash_addr;
         img->mem_base = exec_addr_set ? exec_addr : (flash_addr_set ? flash_addr : mem_addr);
         img->entry_point = entry_addr_set ? entry_addr : (CYG_ADDRESS)entry_address;  // Hope it's been set
@@ -992,7 +1070,7 @@ fis_delete(int argc, char *argv[])
     img = fis_lookup(name, &i);
     if (img) {
         if (i < num_reserved) {
-            diag_printf("Sorry, '%s' is a reserved image and cannot be deleted\n", img->name);
+            diag_printf("Sorry, '%s' is a reserved image and cannot be deleted\n", img->u.name);
             return;
         }
         if (!verify_action("Delete image '%s'", name)) {
@@ -1006,7 +1084,7 @@ fis_delete(int argc, char *argv[])
     if ((stat = flash_erase((void *)img->flash_base, img->size, (void **)&err_addr)) != 0) {
         diag_printf("Error erasing at %p: %s\n", err_addr, flash_errmsg(stat));
     } else {
-        img->name[0] = (unsigned char)0xFF;    
+        img->u.name[0] = (unsigned char)0xFF;
         fis_update_directory();
     }
 }
@@ -1356,6 +1434,19 @@ do_flash_init(void)
 {
     int stat;
 
+    void *err_addr;
+#ifdef CYGOPT_REDBOOT_REDUNDANT_FIS
+    struct fis_image_desc img0;
+    struct fis_image_desc img1;
+
+    //check the size of fis_valid_info
+    CYG_ASSERT((sizeof(struct fis_valid_info)<=sizeof(img0.u.name)), "fis_valid_info size mismatch");
+    //try to check the alignment of version_count
+    CYG_ASSERT((((unsigned long)&img0.u.valid_info.version_count - (unsigned long)&img0) % sizeof(unsigned long) == 0), "alignment problem");
+#endif
+
+
+
     if (!__flash_init) {
         __flash_init = 1;
         if ((stat = flash_init(diag_printf)) != 0) {
@@ -1390,6 +1481,47 @@ do_flash_init(void)
             diag_printf("FIS directory doesn't fit\n");
             return false;
         }
+#ifdef CYGOPT_REDBOOT_REDUNDANT_FIS
+
+        if (CYGNUM_REDBOOT_FIS_REDUNDANT_DIRECTORY_BLOCK < 0) {
+            redundant_fis_addr = (void *)((CYG_ADDRESS)flash_end + 1 +
+                                          (CYGNUM_REDBOOT_FIS_REDUNDANT_DIRECTORY_BLOCK*flash_block_size));
+        } else {
+            redundant_fis_addr = (void *)((CYG_ADDRESS)flash_start +
+                                (CYGNUM_REDBOOT_FIS_REDUNDANT_DIRECTORY_BLOCK*flash_block_size));
+        }
+
+        if (((CYG_ADDRESS)redundant_fis_addr + fisdir_size - 1) > (CYG_ADDRESS)flash_end) {
+            diag_printf("Redundant FIS directory doesn't fit\n");
+            return false;
+        }
+        FLASH_READ(fis_addr, &img0, sizeof(img0), (void **)&err_addr);
+        FLASH_READ(redundant_fis_addr, &img1, sizeof(img1), (void **)&err_addr);
+
+        if (strncmp(img0.u.valid_info.magic_name, CYG_REDBOOT_RFIS_VALID_MAGIC, CYG_REDBOOT_RFIS_VALID_MAGIC_LENGTH)!=0)
+        {
+           memset(&img0, 0, sizeof(img0));
+        }
+
+        if (strncmp(img1.u.valid_info.magic_name, CYG_REDBOOT_RFIS_VALID_MAGIC, CYG_REDBOOT_RFIS_VALID_MAGIC_LENGTH)!=0)
+        {
+           memset(&img1, 0, sizeof(img0));
+        }
+
+#ifdef REDBOOT_FLASH_REVERSE_BYTEORDER
+        img0.u.valid_info.version_count = CYG_SWAP32(img0.u.valid_info.version_count);
+        img1.u.valid_info.version_count = CYG_SWAP32(img1.u.valid_info.version_count);
+#endif
+
+        if (fis_get_valid_buf(&img0, &img1)==1)
+        {
+           // Valid, so swap primary and secondary
+           void * tmp;
+           tmp = fis_addr;
+           fis_addr = redundant_fis_addr;
+           redundant_fis_addr = tmp;
+        }
+#endif
         fis_read_directory();
 #endif
     }
Only in current/src: flash.c~

[-- Attachment #3: fisfs-3.tar.gz --]
[-- Type: application/x-gzip, Size: 9533 bytes --]

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

[Neundorf, Alexander]

Hi,

> Von: Andrew Lunn [mailto:andrew@lunn.ch]
...
> > Andrew: in your patch you removed the magic_name field from 
> the valid_info struct. While this works, it requires quite a 
> lot more typing:
> > 
...
> This is wrong and dangerous and i do not do that in my code. Becasue
> sizeof(CYG_REDBOOT_VALID_MAGIC) == 10, your fvi is not word
> aligned. When you then access fvi0->valid_flag you do a none aligned
> access which on ARM and probably other targets will throw an BUS
> exception. That why my code does a memcpy() from the tail of the name
> array into fvi.

And that's why I changed from 
unsigned short valid_flag;
to 
unsigned char valid_flag1;
unsigned char valid_flag2;

struct fis_image_desc* img0=(struct fis_image_desc*)buf0;
struct fis_image_desc* img1=(struct fis_image_desc*)buf1;
struct fis_valid_info fvi0;
struct fis_valid_info fvi1;
memcpy(&fvi0, img0->name+sizeof(CYG_REDBOOT_VALID_MAGIC), sizeof(CYG_REDBOOT_VALID_MAGIC));
memcpy(&fvi1, img1->name+sizeof(CYG_REDBOOT_VALID_MAGIC), sizeof(CYG_REDBOOT_VALID_MAGIC));

vs. 

struct fis_valid_info* fvi0 =(struct fis_valid_info*)buf0;
struct fis_valid_info* fvi1 =(struct fis_valid_info*)buf1;

which is IMO easier readable and has less potential for bugs, while also having no alignment problems (with my change to two characters).
 
> What im trying to do is use fis_image_desc as much as possible becasue
> thats what we have to be compatible with. By defining a new structure
> there is the danger that fis_image_desc gets changed and
> fis_valid_info does not. 

This has to be kept in mind anyway when changing the binary format of fis_image_desc. Changing this format means caring about compatibility with previous redboot versions, so it doesn't happen easily (as we are doing right now ;-)
Maybe something with a union would make it more explicit ?

union
{
   struct fis_image_desc img;
   struct fis_valid_info valid;
} give_me_a_name;
 
> > This doesn't only apply to the redboot code, but even more to the
> > fisfs implementation, there this has to be done more often.
> 
> I took a quick look at your fisfs code. We need to talk about that...

Yes, sure :-)

(I may have mentioned it already: the current implementation is not intended to go straight into ecos cvs, the conversion to an ecos fs will happen around end of november)
Issues I know of: eocs package and cdl-optins, namespace to ecos fs conversion incl. data structures, adding special function for update.

Bye
Alex

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

[Neundorf, Alexander]

...
> Still the size can be checked with
> CYG_ASSERT(sizeof(struct fis_valid_info) <=sizeof(struct 
> fis_image_desc), "size mismatch");

I meant something like:

CYG_ASSERT(sizeof(struct fis_valid_info) <=sizeof(img.name), "size mismatch");

Alex

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

[Neundorf, Alexander]

> Von: Slawek [mailto:sgp@telsatgp.com.pl]
> 
> Hello!
> In message to "Andrew Lunn" <andrew@lunn.ch> sent Wed, 20 Oct 
> 2004 18:11:18
> +0200 you wrote:
> 
> NA> #define EFIS_VALID       (0xa5a5)
> NA> #define EFIS_IN_PROGRESS (0xfdfd)
> NA> #define EFIS_EMPTY       (0xffff)
> 
> Some additional ideas from somebody who watches the conversation:
> 
> 1) Why do we need EFIS_IN_PROGRESS? Isn't EFIS_EMPTY enough? 
> Both can't be used to load the application anyway.

Hmmm, ok. With IN_PROGRESS differing from 0xffff it is possible to decide whether the writing process has already started. Ok, this doesn't help that much...
 
> 2) ".FisValid" suggest this is valid FIS entry while it 
> doesn't need to be.
> Why don't use separate name for valid and for invalid (empty 
> or in progress)
> fis tables? This could also save additional space used to mark
> "valid/invalid" as this could be decided be the name.

At the moment ".FisValid" is read from the fis table it is considered to be valid.
This can't be used to separate valid and invalid since the change from ".Invalid" to ".Valid" would need an extra erase operation in between, which would kill the purpose of this whole thing.

Andrew: in your patch you removed the magic_name field from the valid_info struct. While this works, it requires quite a lot more typing:

struct fis_image_desc* img0=(struct fis_image_desc*)buf0;
struct fis_image_desc* img1=(struct fis_image_desc*)buf1;
struct fis_valid_info* fvi0
     =(struct fis_valid_info*)img0->name+sizeof(CYG_REDBOOT_VALID_MAGIC);
struct fis_valid_info* fvi1
     =(struct fis_valid_info*)img1->name+sizeof(CYG_REDBOOT_VALID_MAGIC);

instead of:

struct fis_valid_info* fvi0 =(struct fis_valid_info*)buf0;
struct fis_valid_info* fvi1 =(struct fis_valid_info*)buf1;

which IMHO makes it much easier to mix things up and introduce bugs, while it doesn't help against the padding/alignment problem (AFAIK).
This doesn't only apply to the redboot code, but even more to the fisfs implementation, there this has to be done more often.

So I'd like to modify this structure like this:

#define CYG_REDBOOT_VALID_MAGIC        ".FisValid"
#define CYG_REDBOOT_VALID_MAGIC_LENGTH 10

#define CYG_REDBOOT_RFIS_VALID       0xa5
//maybe get rid of one of both:
#define CYG_REDBOOT_RFIS_IN_PROGRESS 0xfd
#define CYG_REDBOOT_RFIS_EMPTY       0xff

struct fis_valid_info
{
   char magic_name[CYG_REDBOOT_VALID_MAGIC_LENGTH];
   unsigned char valid_flag1;
   unsigned char valid_flag2
   unsigned long version_count:
};

Still the size can be checked with
CYG_ASSERT(sizeof(struct fis_valid_info) <=sizeof(struct fis_image_desc), "size mismatch");

What do you think ?

Bye
Alex