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

[Andrew Lunn <andrew at lunn dot ch>]

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