This is the mail archive of the
ecos-devel@sources.redhat.com
mailing list for the eCos project.
Re: AW: contributing a failsafe update meachanism for FIS from within ecos applications
- From: Andrew Lunn <andrew at lunn dot ch>
- To: "Neundorf, Alexander" <Alexander dot Neundorf at jenoptik dot com>
- Cc: Andrew Lunn <andrew at lunn dot ch>, Slawek <sgp at telsatgp dot com dot pl>,ecos-devel at sources dot redhat dot com
- Date: Mon, 25 Oct 2004 11:34:05 +0200
- Subject: Re: AW: contributing a failsafe update meachanism for FIS from within ecos applications
- References: <5A8A17126B73AC4C83968F6C4505E3C501372B07@JO-EX01.JENOPTIK.NET>
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