[ECOS] Re: Check for illegal address range in io/flash

[ECOS] Re: Check for illegal address range in io/flash

[Andrew Lunn]

On Tue, Dec 11, 2007 at 11:09:40AM +0100, ?yvind Harboe wrote:
> On Dec 11, 2007 11:03 AM, Andrew Lunn <andrew@lunn.ch> wrote:
> >
> > > I mainly want the busted check in flash_erase() to be removed.
> > > ASSERT's are probably fine.
> >

What is actually wrong with this check? 

I think it is trying to deal with flash which are placed at the very
top of memory. The last valid byte is 0xffffffff. However is you try
to erase 0xffff0000 with length 0x10000, which is valid, end_addr
becomes 0x0 and things probably go wrong. This code i think it trying
to spot this and set end_addr to 0xffffffff. 

I don't want to remove this until i fully understand what is causing
your problem and how this code is wrong. Please could you explain what
you are seeing.

Thanks
        Andrew

-- 
Before posting, please read the FAQ: http://ecos.sourceware.org/fom/ecos
and search the list archive: http://ecos.sourceware.org/ml/ecos-discuss

[ECOS] Re: Check for illegal address range in io/flash

[Øyvind Harboe]

On Dec 11, 2007 2:51 PM, Andrew Lunn <andrew@lunn.ch> wrote:
> On Tue, Dec 11, 2007 at 11:09:40AM +0100, ?yvind Harboe wrote:
> > On Dec 11, 2007 11:03 AM, Andrew Lunn <andrew@lunn.ch> wrote:
> > >
> > > > I mainly want the busted check in flash_erase() to be removed.
> > > > ASSERT's are probably fine.
> > >
>
> What is actually wrong with this check?

I have no idea what this check is supposed to do.

If a valid range is passed in, it is a no-operation, otherwise it does
something mysterious
that looks just wrong to me.

I'm afraid all I can conclude is that it should be deleted as plain
wrong/unused. The
fact that it is harmless for valid ranges has allowed it to survive to date,
that's the antrophic principle for you I guess.

Someone else would have to speak up for it.

It would be a shame if plain wrong harmless code that nobody understands
could never be deleted from eCos :-)

> I don't want to remove this until i fully understand what is causing
> your problem and how this code is wrong. Please could you explain what
> you are seeing.

The problem is that I relied on flash_erase() to return an error upon illegal
address range. When I tested with an illegal address range,
flash_erase() did not
return an error. Upon inspecting the code, I saw something that looked
like it is *trying* to check the address, but I have found no explanation
for that code, so I see a couple of alternatives:

- if nobody understands this code, delete it after a week or two to
wait for someone to
speak up and see what happens. Probably nothing.
- keep this code forever as it is harmless(with valid addresses)
nobody will be hurt by it.
- add address check to flash_erase() & program and return error message


-- 
Øyvind Harboe
http://www.zylin.com - eCos ARM & FPGA  developer kit

--
Before posting, please read the FAQ: http://ecos.sourceware.org/fom/ecos
and search the list archive: http://ecos.sourceware.org/ml/ecos-discuss

Re: [ECOS] Re: Check for illegal address range in io/flash

[Gary Thomas]

Øyvind Harboe wrote:
> On Dec 11, 2007 2:51 PM, Andrew Lunn <andrew@lunn.ch> wrote:
>> On Tue, Dec 11, 2007 at 11:09:40AM +0100, ?yvind Harboe wrote:
>>> On Dec 11, 2007 11:03 AM, Andrew Lunn <andrew@lunn.ch> wrote:
>>>>> I mainly want the busted check in flash_erase() to be removed.
>>>>> ASSERT's are probably fine.
>> What is actually wrong with this check?
> 
> I have no idea what this check is supposed to do.
> 
> If a valid range is passed in, it is a no-operation, otherwise it does
> something mysterious
> that looks just wrong to me.
> 
> I'm afraid all I can conclude is that it should be deleted as plain
> wrong/unused. The
> fact that it is harmless for valid ranges has allowed it to survive to date,
> that's the antrophic principle for you I guess.
> 
> Someone else would have to speak up for it.
> 
> It would be a shame if plain wrong harmless code that nobody understands
> could never be deleted from eCos :-)
> 
>> I don't want to remove this until i fully understand what is causing
>> your problem and how this code is wrong. Please could you explain what
>> you are seeing.
> 
> The problem is that I relied on flash_erase() to return an error upon illegal
> address range. When I tested with an illegal address range,
> flash_erase() did not
> return an error. Upon inspecting the code, I saw something that looked
> like it is *trying* to check the address, but I have found no explanation
> for that code, so I see a couple of alternatives:
> 
> - if nobody understands this code, delete it after a week or two to
> wait for someone to
> speak up and see what happens. Probably nothing.
> - keep this code forever as it is harmless(with valid addresses)
> nobody will be hurt by it.
> - add address check to flash_erase() & program and return error message

As I read the code, the "check" you are talking about is only to make
sure that the address [range] which gets printed makes sense.  There is
similar code within the loop to handle the edge case where the FLASH
region being erased buts up against the top of possible address space.

I don't see any error checking present at all.


-- 
Before posting, please read the FAQ: http://ecos.sourceware.org/fom/ecos
and search the list archive: http://ecos.sourceware.org/ml/ecos-discuss

Re: [ECOS] Re: Check for illegal address range in io/flash

[Øyvind Harboe]

> As I read the code, the "check" you are talking about is only to make
> sure that the address [range] which gets printed makes sense.  There is
> similar code within the loop to handle the edge case where the FLASH
> region being erased buts up against the top of possible address space.

Gotit. I don't see the corresponding code in flash_program_buf().

The explanation above would be helpful in the code I think.

-- 
Øyvind Harboe
http://www.zylin.com - eCos ARM & FPGA  developer kit

--
Before posting, please read the FAQ: http://ecos.sourceware.org/fom/ecos
and search the list archive: http://ecos.sourceware.org/ml/ecos-discuss