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

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

[Neundorf, Alexander]

> -----Ursprüngliche Nachricht-----
> Von: Andrew Lunn [mailto:andrew@lunn.ch]
> Gesendet: Montag, 18. Oktober 2004 16:50
> An: Neundorf, Alexander
> Cc: ecos-devel@sources.redhat.com
> Betreff: Re: contributing a failsafe update meachanism for FIS from
> within ecos applications
> 
> > > You should have CDL to control where the second flash 
> block is. Its
> > > bad to assume its the next block. 
> > 
> > Really ?
> > IMO it would be ok if the blocks are directly one after the 
> other. One could think of it as a fis dir twice as big as 
> before containing two copies of this fis dir.
> 
> But what about people who want to add this to an existing project and
> there only free blocks in flash are somewhere else.

Ok.

...
> I would think about trying to use the existing format so that it
> remains backward compatible. I think about add a dummy image to the
> fis list. Dummy in that it start and length are 0. But the name would
> be something like. "Fis Valid*" The sting is 16 charactors long. So
> there are 5 unused charactors after the \0. You could put your magic
> numbers there. This will probably be backward compatible with old
> redboots. They will just see it as a normal entry with some junk after
> the end of the string. Where i showed "*" in the sting, if you think
> carefully about ASCII and the binary representation, you might be able
> to come up with a set of charactors which differ by resetting 1's to
> give some sort of meaningful mapping to the state of the FIS
> directory.

Ok, I'll post a modified patch which uses "$Magic[01]" as "magic number" in the next days, with "$Magic0" for the fis table 0 and "$Magic1" for fis table 1. This leaves 8 bytes room for the valid_flag and the version_count.

Bye
Alex

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

[Andrew Lunn]

On Fri, Oct 29, 2004 at 12:43:55PM +0200, Neundorf, Alexander wrote:
> 
> ...
> > > 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;
> };

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. 

> -changesDone  (close)
> -get all fields for image i  (stat)

Stat only needs one field, the length. 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

check crc
calculate crc

        Andrew

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

[Andrew Lunn]

On Fri, Oct 29, 2004 at 10:46:31AM +0200, Neundorf, Alexander wrote:
> > > 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.

True, but because one thing is not ideal does not mean you should
follow the pattern and add more problems.

> > 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 ?

You can never be sure defaults will always suites every
application. But its better than doing completely the wrong thing
because the structure is different and will be wrongly interperted.
 
> 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.

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

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

[Andrew Lunn]

On Thu, Oct 28, 2004 at 11:05:23PM +0200, Alexander Neundorf wrote:
> On Thursday 28 October 2004 21:29, Andrew Lunn wrote:
> > > createImage() does: create a new entry, writes the data, marks the new
> > > entry as valid.
> > >
> > > It consists of the following steps:
> > > startUpdate  (redboot) - modify the fis table contents in RAM and flash,
> > > mark them in progress
> > > writeData    (app) - either all at once, or in flash block sized chunks
> > > finishUpdate (redboot) - mark the new fis table as valid in flash
> >
> > So the first step maps to open()
> > The second step to write()
> > and the third set to close().
> 
> Yes, I realized this too in the meantime.
> (*) It seems the only remaining problem is how to give the additional 
> paramters (flash_base, entry_point etc.) to the open call. I'd say a special 
> function which takes all these parameters (i.e. not the standard open()), 
> better ideas ?

OK. There are two different operations here. There is creat() and
there is open(path,flags) where flag != O_CREAT. You only need
something special when you actually need to create a FIS entry. Just
opening an existing file entry is not a problem.

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.

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.)

> For transferring the parameters from the application API to the redboot VV I'd 
> suggest a struct like this which can easily be translated to fis_image_desc:
> 
> struct fis_image_desc_VV {
>     unsigned char name[16];      // Null terminated name
>     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
> //maybe:
>     unsigned long file_cksum;    // Checksum over image data
> };
> 

No, no, no...

> 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? 

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 actuall
writes it to FLASH.

        Andrew

P.S.
        Im on Holiday for the next week.....

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

[Alexander Neundorf]

On Thursday 28 October 2004 21:29, Andrew Lunn wrote:
> > createImage() does: create a new entry, writes the data, marks the new
> > entry as valid.
> >
> > It consists of the following steps:
> > startUpdate  (redboot) - modify the fis table contents in RAM and flash,
> > mark them in progress
> > writeData    (app) - either all at once, or in flash block sized chunks
> > finishUpdate (redboot) - mark the new fis table as valid in flash
>
> So the first step maps to open()
> The second step to write()
> and the third set to close().

Yes, I realized this too in the meantime.
(*) It seems the only remaining problem is how to give the additional 
paramters (flash_base, entry_point etc.) to the open call. I'd say a special 
function which takes all these parameters (i.e. not the standard open()), 
better ideas ?

For transferring the parameters from the application API to the redboot VV I'd 
suggest a struct like this which can easily be translated to fis_image_desc:

struct fis_image_desc_VV {
    unsigned char name[16];      // Null terminated name
    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
//maybe:
    unsigned long file_cksum;    // Checksum over image data
};

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).

...
> > Without redundant fis:
> > startUpdate doesn't change the flash contents, the new fis table contents
> > are written in finishUpdate, so it will work too (except that power
> > failure.... well you know).
>
> The point is the user gets the choice. They can have a totaly safe
> system, or a system that works 99.9% of the time but needs one less
> flash block.

Yes. No problem there.

> > > You are again breaking the abstract. You are doing the CRC creation in
> > > the application where as it should be redboot doing it.
> >
> > My main reason for this: I'd like to have the new fis table already
> > completely correct on the flash except the valid_flag before the
> > actual writing process starts, so that the final step really only
> > has to set the valid_flag to valid.
>
> I cannot think of a reason why this is actually needed? But maybe im
> missing something.

By modifying the to-be-used flash block of the new fis table before the 
writing starts it is possible to detect whether an update process has been 
interrupted, without having to check all crc's. I.e. not only for the 
firmware image, where the crc is checked by load, but also for data images.

...
> > I would prefer an obviously different API for the updating process
> > since it is "dangerous" for the whole system.  With my createImage()
> > which writes a complete image at once there is also ensured that
> > there can be at most one corrupt image at a time. When splitting
> > open, write and close there can be more than one corrupt
> > image. open() for writing should check that there is no other file
> > open.
>
> That is not a problem. The filesystem can easily enforce this and
> return EAGAN when open() is called when another file has been open'd
> for writing.

Ok.

So it seems we have one issue left, how to transfer the parameters for a new 
image (*). Better ideas than my suggestion ?

Bye
Alex
-- 
Work: alexander.neundorf@jenoptik.com - http://www.jenoptik-los.de
Home: neundorf@kde.org                - http://www.kde.org
      alex@neundorf.net               - http://www.neundorf.net

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

[Andrew Lunn]

> createImage() does: create a new entry, writes the data, marks the new entry 
> as valid.
> 
> It consists of the following steps:
> startUpdate  (redboot) - modify the fis table contents in RAM and flash, mark
>                          them in progress
> writeData    (app) - either all at once, or in flash block sized chunks
> finishUpdate (redboot) - mark the new fis table as valid in flash

So the first step maps to open()
The second step to write()
and the third set to close().


> > I also want to make sure that the design you propose is flexiable
> > enough to support other peoples needs. So it seems you have enough
> > memory to hold a complete image, but i want to ensure the same design
> > can do multiple writes in a clean way using the same API. I would also
> > like it to work without actually needed the redundant FIS block. Not
> > everybody is so paranoid about power failure, but would like to be
> > able to upgrade there application from within the application.
> 
> Well, paranoid...
> If it fails the device doesn't work anymore...
> 
> Without redundant fis: 
> startUpdate doesn't change the flash contents, the new fis table contents are 
> written in finishUpdate, so it will work too (except that power failure.... 
> well you know).

The point is the user gets the choice. They can have a totaly safe
system, or a system that works 99.9% of the time but needs one less
flash block.

> > You are again breaking the abstract. You are doing the CRC creation in
> > the application where as it should be redboot doing it.

> My main reason for this: I'd like to have the new fis table already
> completely correct on the flash except the valid_flag before the
> actual writing process starts, so that the final step really only
> has to set the valid_flag to valid.

I cannot think of a reason why this is actually needed? But maybe im
missing something.

> Apart from that, is it possible for redboot to calculate the crc if
> it doesn't have enough ram to hold the complete image while updating
> and if the application is responsible for the actual writing ?
> Which ram is actually available in a VV function ? (sorry for stupid
> questions)

In redboot side of the VV: Only the stack and any variables in
redboots BSS. But it does not need any RAM. The image is in flash so
it justs runs the CRC over that.

> [OT] why is crc32 used instead of the posix crc ?

Redboot came before posix crc. It also make little difference. crc32
is OK. Its the same one used on ethernet frames.
 
> ...
> > Assumption 1. All the needed FIS entries exist.
> > Assumption 2. Your boot script is:
> > fis load app
> > go
> > fis load app.bak
> > go
> 
> This second step is cool :-)
> 
> > open(/foo) does two VV call to get the start and length of the image
> > in flash and allocates the block cache.
> >
> > write() would copy the data into the block cache. If this fills the
> > block cache it simply erases and then writes. As soon as the erase
> > starts, the CRC is wrong. So in terms of redboot, this image is now
> > corrupt.
> >
> > close() flushes the block cache. 
> 
> Is this is all done in the application ?

Well this is the API between the application and the fisfs. I've
described the actions that fisfs does for each API call. open needs to
call a VV, but write can do all the work without calling redboot.
 
> > It then does VV calls to ask redboot
> > to recalculate the CRC and put it into the in memory copy of the FIS
> > directory. It then calls a VV function to commit the FIS directory.
> > Redboot does an atomic write, with respect to power failure, of the
> > FIS directory using the valid fields in the redundant FIS blocks etc.
> >
> > So how do you do a safe upgrade of the application:
> >
> > open("app");
> > write();         CRC is now wrong, so app.bak would be booted.
> > write();         CRC is now wrong, so app.bak would be booted.
> > write();         CRC is now wrong, so app.bak would be booted.
> > close();         CRC is now valid, so the new image would be booted.
> > open("app.bak");
> > write();         CRC is now wrong, but it does not matter, app is valid
> > write();         CRC is now wrong, but it does not matter, app is valid
> > write();         CRC is now wrong, but it does not matter, app is valid
> > close();         CRC is now valid and we have two identical apps.
> 

> I would prefer an obviously different API for the updating process
> since it is "dangerous" for the whole system.  With my createImage()
> which writes a complete image at once there is also ensured that
> there can be at most one corrupt image at a time. When splitting
> open, write and close there can be more than one corrupt
> image. open() for writing should check that there is no other file
> open.

That is not a problem. The filesystem can easily enforce this and
return EAGAN when open() is called when another file has been open'd
for writing.

        Andrew

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

[Alexander Neundorf]

Hi,

seems we are getting closer...

On Thursday 28 October 2004 19:23, Andrew Lunn wrote:
> On Thu, Oct 28, 2004 at 02:43:16PM +0200, Neundorf, Alexander wrote:
> > > Von: Andrew Lunn [mailto:andrew@lunn.ch]
> > >
...
> It however does not need to know where in the structure they are, what
> other member of the structure are, etc. By passing the fis_image_desc
> structure back and forth you are tying redboot and the application
> together. They both need to have the same definition of the structure.
> Maybe somebody adds a new member to the structure, recompiles redboot
> and installs it. You application is now broken since it does not know
> about this new member and it access the wrong things withing the
> structure. If however you just use the VV get functions everything
> works fine, because redboot knows how to get what you need from the
> structure. The implementation changes, the abstract interface stays
> the same. Classic object orianatated approach.
>
> > This way one would not have to call 8 functions to collect all
> > members for one entry.
>
> In practice you actually only need two. The base and the length. The
> other members are of no use to a filesystem.

Ok, maybe the other entries (entry point, crc) might also be interesting, 
maybe not.
But for creating a new image at least the following entries must be known:
-name
-flash_base
-data_length
-size
-entry_point
-mem_base
(maybe: crc)

So I would like a struct containing these entries.
I don't see a way around this.

...
> OK. Maybe a terminology problem here. What does your createImage() do?
> Does it create a new entry in the fis directory? Does it write a new
> image into flash at the location the fis entry says belongs to this
> image?
>
> For me these are two different things. If there was a filesystem
> creat() call it would do the first. A number of write()s would do the
> second.

createImage() does: create a new entry, writes the data, marks the new entry 
as valid.

It consists of the following steps:
startUpdate  (redboot) - modify the fis table contents in RAM and flash, mark
                         them in progress
writeData    (app) - either all at once, or in flash block sized chunks
finishUpdate (redboot) - mark the new fis table as valid in flash

> I also want to make sure that the design you propose is flexiable
> enough to support other peoples needs. So it seems you have enough
> memory to hold a complete image, but i want to ensure the same design
> can do multiple writes in a clean way using the same API. I would also
> like it to work without actually needed the redundant FIS block. Not
> everybody is so paranoid about power failure, but would like to be
> able to upgrade there application from within the application.

Well, paranoid...
If it fails the device doesn't work anymore...

Without redundant fis: 
startUpdate doesn't change the flash contents, the new fis table contents are 
written in finishUpdate, so it will work too (except that power failure.... 
well you know).

> > 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
>
> I don't see why the application has to be involved in the erase. All
> you need is that the removing of the FIS entry is atomic with respect
> to a power cut. Redboot can do that.

I don't understand exactly.
If the app is responsible for writing the contents of the image, it should 
also be responsible for deleting the contents of the image. Ok, deleting 
could also mean simply removing the entry from the table, and the actual 
deletion is done before the programming, but I don't see a big difference 
here and really erasing the flash would seem cleaner to me. 

...
> > 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
>
> You are again breaking the abstract. You are doing the CRC creation in
> the application where as it should be redboot doing it.

My main reason for this: I'd like to have the new fis table already completely 
correct on the flash except the valid_flag before the actual writing process 
starts, so that the final step really only has to set the valid_flag to 
valid.
Apart from that, is it possible for redboot to calculate the crc if it doesn't 
have enough ram to hold the complete image while updating and if the 
application is responsible for the actual writing ?
Which ram is actually available in a VV function ? (sorry for stupid 
questions)

[OT] why is crc32 used instead of the posix crc ?

...
> Assumption 1. All the needed FIS entries exist.
> Assumption 2. Your boot script is:
> fis load app
> go
> fis load app.bak
> go

This second step is cool :-)

> open(/foo) does two VV call to get the start and length of the image
> in flash and allocates the block cache.
>
> write() would copy the data into the block cache. If this fills the
> block cache it simply erases and then writes. As soon as the erase
> starts, the CRC is wrong. So in terms of redboot, this image is now
> corrupt.
>
> close() flushes the block cache. 

Is this is all done in the application ?

> It then does VV calls to ask redboot
> to recalculate the CRC and put it into the in memory copy of the FIS
> directory. It then calls a VV function to commit the FIS directory.
> Redboot does an atomic write, with respect to power failure, of the
> FIS directory using the valid fields in the redundant FIS blocks etc.
>
> So how do you do a safe upgrade of the application:
>
> open("app");
> write();         CRC is now wrong, so app.bak would be booted.
> write();         CRC is now wrong, so app.bak would be booted.
> write();         CRC is now wrong, so app.bak would be booted.
> close();         CRC is now valid, so the new image would be booted.
> open("app.bak");
> write();         CRC is now wrong, but it does not matter, app is valid
> write();         CRC is now wrong, but it does not matter, app is valid
> write();         CRC is now wrong, but it does not matter, app is valid
> close();         CRC is now valid and we have two identical apps.

I would prefer an obviously different API for the updating process since it is 
"dangerous" for the whole system. 
With my createImage() which writes a complete image at once there is also 
ensured that there can be at most one corrupt image at a time. When splitting 
open, write and close there can be more than one corrupt image. open() for 
writing should check that there is no other file open. Because of this 
special semantics I'd prefer a non-standard API for the updating functions.
But since this is done in the application it doesn't influence the interface 
to redboot that much.

Bye
Alex
-- 
Work: alexander.neundorf@jenoptik.com - http://www.jenoptik-los.de
Home: neundorf@kde.org                - http://www.kde.org
      alex@neundorf.net               - http://www.neundorf.net

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

[Andrew Lunn]

On Thu, Oct 28, 2004 at 02:43:16PM +0200, Neundorf, Alexander wrote:
> 
> > 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.

It however does not need to know where in the structure they are, what
other member of the structure are, etc. By passing the fis_image_desc
structure back and forth you are tying redboot and the application
together. They both need to have the same definition of the structure.
Maybe somebody adds a new member to the structure, recompiles redboot
and installs it. You application is now broken since it does not know
about this new member and it access the wrong things withing the
structure. If however you just use the VV get functions everything
works fine, because redboot knows how to get what you need from the
structure. The implementation changes, the abstract interface stays
the same. Classic object orianatated approach.

> This way one would not have to call 8 functions to collect all
> members for one entry.

In practice you actually only need two. The base and the length. The
other members are of no use to a 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.

OK. Maybe a terminology problem here. What does your createImage() do?
Does it create a new entry in the fis directory? Does it write a new
image into flash at the location the fis entry says belongs to this
image?

For me these are two different things. If there was a filesystem
creat() call it would do the first. A number of write()s would do the
second. 

I also want to make sure that the design you propose is flexiable
enough to support other peoples needs. So it seems you have enough
memory to hold a complete image, but i want to ensure the same design
can do multiple writes in a clean way using the same API. I would also
like it to work without actually needed the redundant FIS block. Not
everybody is so paranoid about power failure, but would like to be
able to upgrade there application from within the application.

> 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

I don't see why the application has to be involved in the erase. All
you need is that the removing of the FIS entry is atomic with respect
to a power cut. Redboot can do that.
 
> 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
 
You are again breaking the abstract. You are doing the CRC creation in
the application where as it should be redboot doing it. 

Let me describe how i see it working. I will talk in terms of
filesystem operations. So when i say open() i mean exactly what
man 2 open says.

Assumption 1. All the needed FIS entries exist. 
Assumption 2. Your boot script is:
fis load app
go
fis load app.bak
go

When redboot loads app it wil check the crc. If the crc fails, the go
will refuse to start the application and instead fall through to the
next command. So it then loads app.bak and starts that. The current
redboot does all this already. No changes needed here.

open(/foo) does two VV call to get the start and length of the image
in flash and allocates the block cache.

write() would copy the data into the block cache. If this fills the
block cache it simply erases and then writes. As soon as the erase
starts, the CRC is wrong. So in terms of redboot, this image is now
corrupt. 

close() flushes the block cache. It then does VV calls to ask redboot
to recalculate the CRC and put it into the in memory copy of the FIS
directory. It then calls a VV function to commit the FIS directory.
Redboot does an atomic write, with respect to power failure, of the
FIS directory using the valid fields in the redundant FIS blocks etc.

So how do you do a safe upgrade of the application:

open("app");
write();         CRC is now wrong, so app.bak would be booted. 
write();         CRC is now wrong, so app.bak would be booted. 
write();         CRC is now wrong, so app.bak would be booted. 
close();         CRC is now valid, so the new image would be booted. 
open("app.bak");   
write();         CRC is now wrong, but it does not matter, app is valid
write();         CRC is now wrong, but it does not matter, app is valid
write();         CRC is now wrong, but it does not matter, app is valid
close();         CRC is now valid and we have two identical apps.

So you need 4 VV calls, or which 2 already exist. 

        Andrew

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

[Andrew Lunn]

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.

> 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?  

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
wanting to spread around two copies of the same code into two
packages. That way leads to real maintinance problems, bugs etc.

        Andrew

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

[Andrew Lunn]

> 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 ?

Anything that deals with the layout of the FIS directory in flash
should be handled by redboot.

The current VV functions take a name of a image in the directory. You
can then get the base, size, memory base, entry point and the data
length. You will probably need to add a rename VV function so you can
change the name of a file. This just changes the im memory copy of the
FIS directory. You then need a commit function which writes the in
memory image to flash, ie calls fis_update_directory(). For systems
without the redundant block the current functionality of
fis_update_directory() is probably all that is needed. For systems
with the redundant block you need to add all the inteligence into
fis_update_directory(), but you can do that later, once you have the
basic filesystem working.

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.

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.

creat is interesting. How do you control the size and location? The
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.

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.

mount is nearly a NOP. All you need to do is see if the redboot
actually supports what you need. umount is probably also a NOP.

        Andrew
        

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

[Andrew Lunn]

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....

> 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 ?

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

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

[Andrew Lunn]

On Mon, Oct 25, 2004 at 11:07:42AM +0200, Neundorf, Alexander wrote:
> 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;

The pointer to the structure is still unaligned which for me makes it
potentially dangerous when access members of the structure.

> 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_SIZE_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.

>  
> > > 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() 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.

        Andrew

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

[Andrew Lunn]

> 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);

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.
 
> instead of:
> 
> struct fis_valid_info* fvi0 =(struct fis_valid_info*)buf0;
> struct fis_valid_info* fvi1 =(struct fis_valid_info*)buf1;

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. 

> which IMHO makes it much easier to mix things up and introduce bugs,
> while it doesn't help against the padding/alignment problem (AFAIK).

It should help the padding/allignment problem becasue my structure is
aligned where as yours is not. I also memcpy it back and for which is
always safe to do. And the assert() i added should tell us if its gone
wrong.

> 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...

        Andrew

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

[Slawek]

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.

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.

-- 
Slawomir Piotrowski

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

[Neundorf, Alexander]

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

Hi,

so here comes a patch with all these changes, including a cdl-entry.
In fis_init() I didn't find the correct place where to erase the additional flash block, there I'd need help.

Main change:

#define EFIS_MAGIC_LENGTH 10
#define EFIS_MAGIC ".FisValid"  //exactly 10 bytes

#define EFIS_VALID       (0xa5a5)
#define EFIS_IN_PROGRESS (0xfdfd)
#define EFIS_EMPTY       (0xffff)

struct fis_valid_info
{
   unsigned char magic_name[EFIS_MAGIC_LENGTH];
   unsigned short valid_flag;
   unsigned long version_count;
};

So it fits again completely in the 16 bytes for the name.

Please comment :-)

Bye
Alex

[-- Attachment #2: fis.fail_save-3.patch --]
[-- Type: application/octet-stream, Size: 8961 bytes --]

diff -rbup current.orig/cdl/redboot.cdl current/cdl/redboot.cdl
--- current.orig/cdl/redboot.cdl	Sun Sep 19 19:49:59 2004
+++ current/cdl/redboot.cdl	Wed Oct 20 18:06:18 2004
@@ -623,6 +623,17 @@ cdl_package CYGPKG_REDBOOT {
                   determine what's free and what's not."
             }
     
+            cdl_option CYGOPT_REDBOOT_ENHANCED_FIS {
+                display         "RedBoot Enhanced Flash Image System support"
+                default_value   0
+                doc             ref/flash-image-system.html
+                description "
+                    This option enables the Enhanced Flash Image System commands
+                    and support within RedBoot.  If enabled a flash block will be
+                  reserved for a second copy of the fis table.
+                  "
+            }
+    
             cdl_component CYGPKG_REDBOOT_FIS_CONTENTS {
                 display       "Flash Image System default directory contents"
                 active_if     CYGOPT_REDBOOT_FIS
@@ -632,6 +643,18 @@ cdl_package CYGPKG_REDBOOT {
                     display         "Flash block containing the Directory"
                     flavor          data
                     default_value   (-1)
+                    description "
+                      Which block of flash should hold the 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 CYGNUM_REDBOOT_FIS_ADDITIONAL_DIRECTORY_BLOCK {
+                    display         "Flash block containing the backup Directory"
+                    active_if       CYGOPT_REDBOOT_ENHANCED_FIS
+                    flavor          data
+                    default_value   (-3)
                     description "
                       Which block of flash should hold the directory 
                       information. Positive numbers are absolute block numbers. 
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	Wed Oct 20 17:56:25 2004
@@ -73,7 +73,22 @@ struct fis_image_desc {
     unsigned long file_cksum;    // Checksum over image data
 };
 
+#define EFIS_MAGIC_LENGTH 10
+#define EFIS_MAGIC ".FisValid"  //exactly 10 bytes
+
+#define EFIS_VALID       (0xa5a5)
+#define EFIS_IN_PROGRESS (0xfdfd)
+#define EFIS_EMPTY       (0xffff)
+
+struct fis_valid_info
+{
+   unsigned char magic_name[EFIS_MAGIC_LENGTH];
+   unsigned short valid_flag;
+   unsigned long version_count;
+};
+
 struct fis_image_desc *fis_lookup(char *name, int *num);
 
 #endif // CYGOPT_REDBOOT_FIS
 #endif // _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	Wed Oct 20 18:03:45 2004
@@ -169,6 +169,8 @@ int flash_block_size, flash_num_blocks;
 #ifdef CYGOPT_REDBOOT_FIS
 void *fis_work_block;
 void *fis_addr;
+void *fis_addr0;
+void *fis_addr1;
 int fisdir_size;  // Size of FIS directory.
 #endif
 #ifdef CYGSEM_REDBOOT_FLASH_CONFIG
@@ -278,6 +280,7 @@ fis_init(int argc, char *argv[])
 {
     int stat;
     struct fis_image_desc *img;
+    struct fis_valid_info* fvi=NULL;
     void *err_addr;
     bool full_init = false;
     struct option_info opts[1];
@@ -301,9 +304,21 @@ 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_ENHANCED_FIS
+    //create the valid flag entry
+    fvi=(struct fis_valid_info*)img;
+    memset(img, 0, sizeof(struct fis_image_desc));
+    strcpy(fvi->magic_name, EFIS_MAGIC);
+
+    fvi->valid_flag=EFIS_VALID;
+    fvi->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)");
@@ -357,10 +372,18 @@ fis_init(int argc, char *argv[])
     // And a descriptor for the descriptor table itself
     memset(img, 0, sizeof(*img));
     strcpy(img->name, "FIS directory");
-    img->flash_base = (CYG_ADDRESS)fis_addr;
-    img->mem_base = (CYG_ADDRESS)fis_addr;
+    img->flash_base = (CYG_ADDRESS)fis_addr0;
+    img->mem_base = (CYG_ADDRESS)fis_addr0;
+    img->size = fisdir_size;
+    img++;
+#ifdef CYGOPT_REDBOOT_ENHANCED_FIS
+    memset(img, 0, sizeof(*img));
+    strcpy(img->name, "FIS directory~");
+    img->flash_base = (CYG_ADDRESS)fis_addr1;
+    img->mem_base = (CYG_ADDRESS)fis_addr1;
     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
@@ -1351,10 +1374,36 @@ _flash_info(void)
            flash_start, (CYG_ADDRWORD)flash_end + 1, flash_num_blocks, (void *)flash_block_size);
 }
 
+#ifdef CYGOPT_REDBOOT_ENHANCED_FIS
+static int
+get_valid_buf(struct fis_valid_info* fvi0, struct fis_valid_info* fvi1)
+{
+   if (strncmp(fvi0->magic_name, EFIS_MAGIC, EFIS_MAGIC_LENGTH)!=0)  //buf1 must be valid
+      return 1;
+   else if (strncmp(fvi1->magic_name, EFIS_MAGIC, EFIS_MAGIC_LENGTH)!=0) //buf0 must be valid
+      return 0;
+
+   //magic is ok for both, now check the valid flag
+   if (fvi0->valid_flag!=EFIS_VALID) //buf1 must be valid
+      return 1;
+   else if (fvi1->valid_flag!=EFIS_VALID) //buf0 must be valid
+      return 0;
+
+   //now check the version
+   if (fvi1->version_count == (fvi0->version_count+1)) //buf1 must be valid
+      return 1;
+
+   return 0;
+}
+#endif
+
 bool
 do_flash_init(void)
 {
     int stat;
+    void *err_addr=0;
+    struct fis_valid_info fvi0;
+    struct fis_valid_info fvi1;
 
     if (!__flash_init) {
         __flash_init = 1;
@@ -1379,17 +1428,64 @@ do_flash_init(void)
         workspace_end = (unsigned char *)(workspace_end-fisdir_size);
         fis_work_block = workspace_end;
 # endif
+
+        fis_addr0 = NULL;
+        fis_addr1 = NULL;
+
         if (CYGNUM_REDBOOT_FIS_DIRECTORY_BLOCK < 0) {
-            fis_addr = (void *)((CYG_ADDRESS)flash_end + 1 +
+            fis_addr0 = (void *)((CYG_ADDRESS)flash_end + 1 +
                                 (CYGNUM_REDBOOT_FIS_DIRECTORY_BLOCK*flash_block_size));
         } else {
-            fis_addr = (void *)((CYG_ADDRESS)flash_start + 
+            fis_addr0 = (void *)((CYG_ADDRESS)flash_start +
                                 (CYGNUM_REDBOOT_FIS_DIRECTORY_BLOCK*flash_block_size));
         }
-        if (((CYG_ADDRESS)fis_addr + fisdir_size - 1) > (CYG_ADDRESS)flash_end) {
+
+#ifdef CYGOPT_REDBOOT_ENHANCED_FIS
+        if (CYGNUM_REDBOOT_FIS_ADDITIONAL_DIRECTORY_BLOCK < 0) {
+           fis_addr1 = (void *)((CYG_ADDRESS)flash_end + 1 +
+                                (CYGNUM_REDBOOT_FIS_ADDITIONAL_DIRECTORY_BLOCK*flash_block_size));
+        } else {
+           fis_addr1 = (void *)((CYG_ADDRESS)flash_start +
+                                (CYGNUM_REDBOOT_FIS_ADDITIONAL_DIRECTORY_BLOCK*flash_block_size));
+        }
+
+        if ((((CYG_ADDRESS)fis_addr0 + fisdir_size - 1) > (CYG_ADDRESS)flash_end)
+            || (((CYG_ADDRESS)fis_addr1 + fisdir_size - 1) > (CYG_ADDRESS)flash_end)
+            || (fis_addr0==fis_addr1)) {
             diag_printf("FIS directory doesn't fit\n");
             return false;
         }
+
+        FLASH_READ(fis_addr0, &fvi0, sizeof(struct fis_valid_info), (void **)&err_addr);
+        FLASH_READ(fis_addr1, &fvi1, sizeof(struct fis_valid_info), (void **)&err_addr);
+
+#ifdef REDBOOT_FLASH_REVERSE_BYTEORDER
+        fvi0.magic=CYG_SWAP32(fvi0.magic);
+        fvi0.valid_flag=CYG_SWAP32(fvi0.valid_flag);
+        fvi0.version_count=CYG_SWAP32(fvi0.version_count);
+        fvi1.magic=CYG_SWAP32(fvi1.magic);
+        fvi1.valid_flag=CYG_SWAP32(fvi1.valid_flag);
+        fvi1.version_count=CYG_SWAP32(fvi1.version_count);
+#endif
+
+        if ((strncmp(fvi0.magic_name, EFIS_MAGIC, EFIS_MAGIC_LENGTH)==0) || (memcmp(fvi1.magic_name, EFIS_MAGIC, EFIS_MAGIC_LENGTH)==0)) {
+           if (get_valid_buf(&fvi0, &fvi1)==0)
+              fis_addr=fis_addr0;
+           else
+              fis_addr=fis_addr1;
+        } else {
+           fis_addr1=NULL;
+           fis_addr=fis_addr0;
+        }
+#else  //no enhanced fis
+        if (((CYG_ADDRESS)fis_addr0 + fisdir_size - 1) > (CYG_ADDRESS)flash_end) {
+            diag_printf("FIS directory doesn't fit\n");
+            return false;
+        }
+        fis_addr1=NULL;
+        fis_addr=fis_addr0;
+#endif
+
         fis_read_directory();
 #endif
     }
Only in current/src: flash.c~

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

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

[Neundorf, Alexander]

Hi,

one more word to the actual firmware update, the code which does the actual update later on looks like this:

#define FIS_START <insert your flash start address here>
#define FIS_APP_1 0x040000  //fixed location of firmware image 1
#define FIS_APP_1 0x240000  //fixed location of firmware image 2

-----------------

CYG_ADDRESS addr=FIS_START+FIS_APP_1;
fis_image_desc* img=FisFS::getEntry("/myapp");
if (img!=0)
{
   //find out which image is currently running 
   //and choose the other address for the new image
   if (img->flash_base==FIS_START+FIS_APP_1)  
      addr=FIS_START+FIS_APP_2;
   else
      addr=FIS_START+FIS_APP_1;
}

//write the image and create a backup 
int res=FisFS::createImage("/myapp", firmwareImageBuffer,
                              addr, 0x20000, maxLength,
                              0x20040, size, true);
------

And the fis boot script looks like this:

fis load myapp; go

So after createImage() was successfully, the old /myapp is renamed to /myapp~, and redboot will find the new image under the name /myapp.
If createImage() fails, redboot will still read the "old" fis table where /myapp is still the unchanged old firmware image.

Bye
Alex

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

[Neundorf, Alexander]

> 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

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

[Neundorf, Alexander]

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

Hi all,

> Von: Andrew Lunn [mailto:andrew@lunn.ch]
> Betreff: Re: contributing a failsafe update meachanism for FIS from
> within ecos applications
> 
> > Ok, I'll post a modified patch which uses "$Magic[01]" as "magic
> > number" in the next days, with "$Magic0" for the fis table 0 and
> > "$Magic1" for fis table 1. This leaves 8 bytes room for the
> > valid_flag and the version_count.
> 
> The problem with "Magic" is that it does not indicate what its for. If
> you don't know what its for, somebody will delete it. "fis valid" is
> less likely to be deleted since it sounds more important. I also don't
> see why you need Magic0 and Magic1. 
> 
> You don't need 8 bytes. All you really need is 4 bits. 2 bits for
> valid, in progress and empty, plus 2 bits for the version. 


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).
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.

So, here comes a modified patch. 

With the basic changes being:

#define EFIS_VALID "$_FisValid_"

struct fis_valid_info
{
   char magic_name[12];
   unsigned long valid_flag;
   CYG_ADDRESS unused_flash_base;
   unsigned long version_count;
};

This will ensure that also older redboots will recognize this entry in the table as being used but consuming no space on the flash (length and flash_base ==0).
$_FisValid_ isn't listed by "fis list" since its address isn't found (see the loop in fis list).
The flash block of the second fis table is now defined with a cdl-option, I didn't get around yet to define an option to switch the second table on and off.
The fisfs-2.tar.gz contains the changes required in the application code. Still no ecos fs (but will come).

Bye
Alex


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

[-- Attachment #3: fis.fail_save.patch --]
[-- Type: application/octet-stream, Size: 7485 bytes --]

diff -rbup current.orig/cdl/redboot.cdl current/cdl/redboot.cdl
--- current.orig/cdl/redboot.cdl	Sun Sep 19 19:49:59 2004
+++ current/cdl/redboot.cdl	Tue Oct 19 17:16:18 2004
@@ -638,6 +638,19 @@ 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_option CYGNUM_REDBOOT_FIS_ADDITIONAL_DIRECTORY_BLOCK {
+                    display         "Flash block containing the backup Directory"
+                    flavor          data
+                    default_value   (-1)
+                    description "
+                      Which block of flash should hold the 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 19 17:17:02 2004
@@ -73,7 +73,22 @@ struct fis_image_desc {
     unsigned long file_cksum;    // Checksum over image data
 };
 
+#define EFIS_MAGIC "$_FisValid_"  //exactly 12 bytes
+
+#define EFIS_VALID       (0xa5a5a5a5)
+#define EFIS_IN_PROGRESS (0xfdfdfdfd)
+#define EFIS_EMPTY       (0xffffffff)
+
+struct fis_valid_info
+{
+   unsigned char magic_name[12];
+   unsigned long valid_flag;
+   CYG_ADDRESS unused_flash_base;
+   unsigned long version_count;
+};
+
 struct fis_image_desc *fis_lookup(char *name, int *num);
 
 #endif // CYGOPT_REDBOOT_FIS
 #endif // _FIS_H_
+
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 19 17:15:26 2004
@@ -169,6 +169,8 @@ int flash_block_size, flash_num_blocks;
 #ifdef CYGOPT_REDBOOT_FIS
 void *fis_work_block;
 void *fis_addr;
+void *fis_addr0;
+void *fis_addr1;
 int fisdir_size;  // Size of FIS directory.
 #endif
 #ifdef CYGSEM_REDBOOT_FLASH_CONFIG
@@ -278,6 +280,7 @@ fis_init(int argc, char *argv[])
 {
     int stat;
     struct fis_image_desc *img;
+    struct fis_valid_info* fvi=NULL;
     void *err_addr;
     bool full_init = false;
     struct option_info opts[1];
@@ -301,9 +304,19 @@ 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
+
+    //create the valid flag entry
+    fvi=(struct fis_valid_info*)img;
+    memset(img, 0, sizeof(struct fis_image_desc));
+    memcpy(fvi->magic_name, EFIS_MAGIC, 12);
+
+    fvi->valid_flag=EFIS_VALID;
+    fvi->version_count=0;
+    img++;
+
+    // Create a pseudo image for RedBoot
 #ifdef CYGOPT_REDBOOT_FIS_RESERVED_BASE
     memset(img, 0, sizeof(*img));
     strcpy(img->name, "(reserved)");
@@ -357,8 +370,14 @@ fis_init(int argc, char *argv[])
     // And a descriptor for the descriptor table itself
     memset(img, 0, sizeof(*img));
     strcpy(img->name, "FIS directory");
-    img->flash_base = (CYG_ADDRESS)fis_addr;
-    img->mem_base = (CYG_ADDRESS)fis_addr;
+    img->flash_base = (CYG_ADDRESS)fis_addr0;
+    img->mem_base = (CYG_ADDRESS)fis_addr0;
+    img->size = fisdir_size;
+    img++;
+    memset(img, 0, sizeof(*img));
+    strcpy(img->name, "FIS directory~");
+    img->flash_base = (CYG_ADDRESS)fis_addr1;
+    img->mem_base = (CYG_ADDRESS)fis_addr1;
     img->size = fisdir_size;
     img++;
 
@@ -1351,10 +1370,35 @@ _flash_info(void)
            flash_start, (CYG_ADDRWORD)flash_end + 1, flash_num_blocks, (void *)flash_block_size);
 }
 
+static int
+get_valid_buf(struct fis_valid_info* fvi0, struct fis_valid_info* fvi1)
+{
+   if (memcmp(fvi0->magic_name, EFIS_MAGIC, 12)!=0)  //buf1 must be valid
+      return 1;
+   else if (memcmp(fvi1->magic_name, EFIS_MAGIC, 12)!=0) //buf0 must be valid
+      return 0;
+
+   //magic is ok for both, now check the valid flag
+   if (fvi0->valid_flag!=EFIS_VALID) //buf1 must be valid
+      return 1;
+   else if (fvi1->valid_flag!=EFIS_VALID) //buf0 must be valid
+      return 0;
+
+   //now check the version
+   if ((fvi1->version_count > fvi0->version_count) //buf1 must be valid
+       && (fvi1->version_count != 0xffffffff))
+      return 1;
+
+   return 0;
+}
+
 bool
 do_flash_init(void)
 {
     int stat;
+    void *err_addr=0;
+    struct fis_valid_info fvi0;
+    struct fis_valid_info fvi1;
 
     if (!__flash_init) {
         __flash_init = 1;
@@ -1379,17 +1423,55 @@ do_flash_init(void)
         workspace_end = (unsigned char *)(workspace_end-fisdir_size);
         fis_work_block = workspace_end;
 # endif
+
+        fis_addr0 = NULL;
+        fis_addr1 = NULL;
+
         if (CYGNUM_REDBOOT_FIS_DIRECTORY_BLOCK < 0) {
-            fis_addr = (void *)((CYG_ADDRESS)flash_end + 1 +
+            fis_addr0 = (void *)((CYG_ADDRESS)flash_end + 1 +
                                 (CYGNUM_REDBOOT_FIS_DIRECTORY_BLOCK*flash_block_size));
         } else {
-            fis_addr = (void *)((CYG_ADDRESS)flash_start + 
+            fis_addr0 = (void *)((CYG_ADDRESS)flash_start +
                                 (CYGNUM_REDBOOT_FIS_DIRECTORY_BLOCK*flash_block_size));
         }
-        if (((CYG_ADDRESS)fis_addr + fisdir_size - 1) > (CYG_ADDRESS)flash_end) {
+
+        if (CYGNUM_REDBOOT_FIS_ADDITIONAL_DIRECTORY_BLOCK < 0) {
+           fis_addr1 = (void *)((CYG_ADDRESS)flash_end + 1 +
+                                (CYGNUM_REDBOOT_FIS_ADDITIONAL_DIRECTORY_BLOCK*flash_block_size));
+        } else {
+           fis_addr1 = (void *)((CYG_ADDRESS)flash_start +
+                                (CYGNUM_REDBOOT_FIS_ADDITIONAL_DIRECTORY_BLOCK*flash_block_size));
+        }
+
+        if ((((CYG_ADDRESS)fis_addr0 + fisdir_size - 1) > (CYG_ADDRESS)flash_end)
+            || (((CYG_ADDRESS)fis_addr1 + fisdir_size - 1) > (CYG_ADDRESS)flash_end)
+            || (fis_addr0==fis_addr1)) {
             diag_printf("FIS directory doesn't fit\n");
             return false;
         }
+
+        FLASH_READ(fis_addr0, &fvi0, sizeof(struct fis_valid_info), (void **)&err_addr);
+        FLASH_READ(fis_addr1, &fvi1, sizeof(struct fis_valid_info), (void **)&err_addr);
+
+#ifdef REDBOOT_FLASH_REVERSE_BYTEORDER
+        fvi0.magic=CYG_SWAP32(fvi0.magic);
+        fvi0.valid_flag=CYG_SWAP32(fvi0.valid_flag);
+        fvi0.version_count=CYG_SWAP32(fvi0.version_count);
+        fvi1.magic=CYG_SWAP32(fvi1.magic);
+        fvi1.valid_flag=CYG_SWAP32(fvi1.valid_flag);
+        fvi1.version_count=CYG_SWAP32(fvi1.version_count);
+#endif
+
+        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;
+           else
+              fis_addr=fis_addr1;
+        } else {
+           fis_addr1=NULL;
+           fis_addr=fis_addr0;
+        }
+
         fis_read_directory();
 #endif
     }
Only in current/src: flash.c~

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

[Neundorf, Alexander]

> -----Ursprüngliche Nachricht-----
> Von: Andrew Lunn [mailto:andrew@lunn.ch]
> Gesendet: Montag, 18. Oktober 2004 14:53
> An: Neundorf, Alexander
> Cc: ecos-devel@sources.redhat.com
> Betreff: Re: contributing a failsafe update meachanism for FIS from
> within ecos applications
> 
> 
> On Mon, Oct 18, 2004 at 02:25:53PM +0200, Neundorf, Alexander wrote:
> > Any technical comments ?
> 
> You need to make some of the code conditionally compiled on CDL. ie by
> default there is no valid marker, only one flash block is used etc. 

Yes.
 
> You should have CDL to control where the second flash block is. Its
> bad to assume its the next block. 

Really ?
IMO it would be ok if the blocks are directly one after the other. One could think of it as a fis dir twice as big as before containing two copies of this fis dir.
 
> What happens when an old redboot reads the fis directory of a new
> redboot with these valid/invalid entry? 

If a new redboot reads an old fis dir everything should be ok. If not, then I've overlooked something.

If an old redboot reads a new fis dir there will be some problems.
The "valid marker" entry will be considered empty. 
So it won't show up on "fis list". Since it doesn't allocate space in the flash, the space allocation algorithm will work correctly. There will be a problem with "fis create" when an empty entry in the fis table is searched, the "valid marker" entry might be overwritten.
But how can this happen ? A new fis dir can only be created using a new redboot. Why should after a new fis dir has been created using a new redboot an old redboot run on this flash ?

> Does the entry show up on the fis list? 

No, it's hidden. Should it be visible ?

> Does it have a sensible name?

Since it's hidden it currently doesn't have a name. With my current approach a name is slightly complicated, since I use the first bytes of the name for the valid marker.
Maybe I could change the "magic number" from "0xff1234ff" to "$VALID0\0" and "$VALID1\0".
This would still leave room for 2 integers (valid_flag and version_count) and be even more compatible with the current redboot. It would be listed by fis list, not overwritten by an old redboot, and via the last digit you could even recognize which one is currently active. OTOH this would make it impossible to use "$VALID[01]" for "normal" fis images (which might exist right now, at least theoretically).

> Is there any way to see if its valid/invalid etc.

What do you suggest ?
This mean adding commands to fis. Until now I wanted to avoid this and stay completely compatible on the outside. 
 
> At the moment you seemed to of focused on the mechanics of how you
> boot/update reliably. Thats the easy part. 
> You now need to see how it fits into the rest of redboot and eCos. 
> Hopefully some of the
> questions i asked above with get you started in that direction.

Yes, that's why I asked. What I need is that we reach a consensus on the layout of the data on the flash.

Bye
Alex

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

[Neundorf, Alexander]

> Von: Andrew Lunn [mailto:andrew@lunn.ch]
> Gesendet: Montag, 18. Oktober 2004 14:05
> An: Neundorf, Alexander
> Cc: ecos-devel@sources.redhat.com
> Betreff: Re: contributing a failsafe update meachanism for FIS from
> within ecos applications
> 
> 
> > Ok, I already had a quick look at the ecos and ecoscentrics web
> > pages but didn't find information about the copyright
> > assignement. Can you give me a link on the procedure ?
> 
> http://ecos.sourceware.org/assign.html
> 
>         Andrew

Ok.

Any technical comments ?

Bye
Alex

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

[Neundorf, Alexander]

> -----Ursprüngliche Nachricht-----
> Von: Andrew Lunn [mailto:andrew@lunn.ch]
> Gesendet: Montag, 18. Oktober 2004 13:16
> An: Neundorf, Alexander
> Cc: ecos-devel@sources.redhat.com
> Betreff: Re: contributing a failsafe update meachanism for FIS from
> within ecos applications
> 
> 
> On Mon, Oct 18, 2004 at 01:05:06PM +0200, Neundorf, Alexander wrote:
> > Hi,
> > 
> > I already posted to ecos-discuss, but without much response.
> 
> Actually, Gary is waiting for a copyright assignment from you, plus he
> has just moved house so i guess there is some degree of chaos
> involved.
> 
> Once the copyright assignment is sorted out we can take this further.
> Until then i doubt anything will make it into anoncvs.

Ok, I already had a quick look at the ecos and ecoscentrics web pages but didn't find information about the copyright assignement. Can you give me a link on the procedure ?

Bye
Alex