Error in _lx_nor_flash_driver_read and potentially elsewhere with zero starting flash address and extended cache.

[maxkunes]

When the extended cache is enabled AND the base address of the flash device given to levelx is zero, _lx_nor_flash_driver_read will never return.

The nor flash's base address is set, at least in my case, in my driver as follows. This is very convenient as for writing as I need to provide a flash address which is not offset by any amount. For reading from the device, I offset as necessary.

nor_flash->lx_nor_flash_base_address = 0;

When _lx_nor_flash_driver_read checks if there is a cache entry for an address, there is a check to see if cache_entry_start is non zero. This causes the caching of the first sector to fail and when _lx_nor_flash_driver_read tries to read the first sector, it will never return as it can never see the cache entry which exists.

Specifically at the very least, this line:

if ((cache_entry_start) && (flash_address >= cache_entry_start) && (flash_address < cache_entry_end))

Should not simply check if cache_entry_start is non-zero. However, I don't think the solution is just to remove that check, but I'm not sure.

Given this error, I'm concerned there are other errors in the code regarding a flash address base of zero. I'm assuming this is just a mistake, as if it was not, the init function should check for the zeroed flash address.

[maxkunes]

I recently upgraded to 6.1.9, but the issue exists in previous versions also.

[maxkunes]

This is still a bug in main. The safest way around this is to pass a fake base address and correct for it in the driver callbacks.

[TheFlyvio]

I can confirm this is happening, just lost 8hs because of this error

[daytonflores-jcmglobal]

I can confirm issue exists in 6.2.1 also.

Reviewing the latest release at time of this comment (6.4.1), the logic still doesn't appear to have been changed but we have not actually tested with 6.4.1 (we are currently locked into 6.2.1). The way we have chosen to work around this issue is:

In _lx_nor_flash_extended_cache_enable, instead of initializing each cache entry's lx_nor_flash_extended_cache_entry_sector_address to LX_NULL, we are instead initializing it to (ULONG *)(0xFFFFFFFF - LX_NOR_SECTOR_SIZE).

We are subtracting LX_NOR_SECTOR_SIZE because in the 3 functions below that we will be modifying, I see the logic cache_entry_end = cache_entry_start + LX_NOR_SECTOR_SIZE; so we are subtracting to prevent any overflow in comparison logic.

In the below 3 functions:

_lx_nor_flash_driver_read
_lx_nor_flash_driver_write
_lx_nor_flash_driver_block_erase

We modify the line (called out by @maxkunes above) from:

if ((cache_entry_start) && (flash_address >= cache_entry_start) && (flash_address < cache_entry_end))

to:

if ((cache_entry_start != (ULONG *)(0xFFFFFFFF - LX_NOR_SECTOR_SIZE)) && (flash_address >= cache_entry_start) && (flash_address < cache_entry_end))

Additionally, in _lx_nor_flash_driver_block_erase we also modify this line from:

/* Yes, this cache entry is in the block to be erased so invalidate it.  */
nor_flash -> lx_nor_flash_extended_cache[i].lx_nor_flash_extended_cache_entry_sector_address = LX_NULL;

to:

/* Yes, this cache entry is in the block to be erased so invalidate it.  */
nor_flash -> lx_nor_flash_extended_cache[i].lx_nor_flash_extended_cache_entry_sector_address = (ULONG *)(0xFFFFFFFF - LX_NOR_SECTOR_SIZE);

This is essentially the same as simply removing the check for cache_entry_start to be non-0, but for error reporting this gives us a bit more context whether that cache entry has been previously set or not. Note that this solution is only valid for our use-case since we do not expect any actual address to be (0xFFFFFFFF - LX_NOR_SECTOR_SIZE) since our memory is not that large.

Would love to have a real solution in place for this. @fdesbiens

[simonebonetti987]

I can confirm issue exists in 6.4.1 also.

This need a proper fix.
Thanks.