Erase addr size / section of the flash memory with st-flash

[antoinefaure]

Hi,

This PR is to add the possibility to only erase a section of the flash, whereas as of today it is only possible to erase the whole flash (as far as I know).

I've added this functionality in a different function, as I didn't want to mess with the parallelism in stlink_erase_flash_mass
Calling erase without specifying an address or size will have the same behavior as before, i.e. erasing the whole flash.

Let me know if you have any remarks.
Thanks!

[slyshykO]

Hi.
The newly added function 'stlink_erase_flash_section' didn't work in most cases. Because few MCUs have sectors of the same size. Even more, many MCUs have sector sizes that differ from those contained in sl->flash_pgsz.

For example, I've tested erase sections with STM32F746G-DISCO.
The first sector of stm32f746 is 32kB.
But sl->flash_pgsz for it equal only 2kB (I don't know why).
So, I run

st-flash.exe erase 0x8000000 4096

And the application erased 2 sectors with a total volume of 64kB, instead of 1 sector.

>st-flash.exe erase 0x8000000 4096
st-flash 1.7.0-130-g42790f3
2022-01-09T16:06:02 INFO common.c: F74x/F75x: 320 KiB SRAM, 1024 KiB flash in at least 2 KiB pages.
EraseFlash - Sector:0x0 Size:0x8000 -> Flash page at     0/    2 erased
EraseFlash - Sector:0x1 Size:0x8000 -> Flash page at     1/    2 erased

[Nightwalker-87]

I could have spotted that indeed... Good that we usually have at least two reviewers.

[antoinefaure]

Hi,
Thanks for your feedback, that makes sense.
I'll update the PR to use stlink_calculate_pagesize to get the size of each page that's going to be erased, does this seem right to you ?

[slyshykO]

@antoinefaure
Yes, the calculation of the size of all sections to be erased is the correct way. But also, you should check user input in st-flash.
The address should be a flash address and you should not try to erase absent sectors.

[Nightwalker-87]

@antoinefaure Please read the respective value from the .chip files located in /config/chips along with struct stlink_chipid_params in chipid.h.
However we should keep in mind that changes will apply to this file soon, but as long as we stay on the develop branch there should be no conflict with current achievements. Most important is not to make use of the old chip definitions in chipid.c. This would lead to additional efforts when merging the current feature branch. It's probably best if you have a look at branch chipid_cleanup to get an idea on what is going on in the background.

[antoinefaure]

I've just pushed 3 new commits:

• d3bf145 to use stlink_calculate_pagesize to get the page size, instead of relying on sl->flash_pgsz
• 8a1535e to check the validity of the size and address in stlink_erase_flash_section instead of main. That way, if an invalid size or address is given, it will return an error instead of erasing the whole flash (which was the behavior in my first commit).
• f277fdb adds a few safety checks, to make sure the address and size are aligned with a page. It will therefore return an error if an user tries to erase half a page for example.

@slyshykO
I'm not sure what you had in mind when saying this :

you should not try to erase absent sectors

Is there something else that needs to be checked ?

@Nightwalker-87
What should I read in the chips files ? It seems I have everything I need in sl and with stlink_calculate_pagesize.
Am I missing something ?

[Nightwalker-87]

@antoinefaure Take a look at chipid.h where we have struct stlink_chipid_params. This is the format structure of the previously mentioned chip-files. You need to ensure that flash_pagesize is read for the current chip on which you try to erase a flash section.

[Nightwalker-87]

Ah, I see - this is still an old implementation from common.c... All addresses are hardcoded for a second time (it did not even read from the old chipid data-base in chipid.c). OMG what a mess! 😞
Yeah, use it for now, but we should have that re-coded as well to read the flash pagesizes from the chip-files instead.

[antoinefaure]

That's what I understood when going through chipid.h and common.c, hence my confusion.
Thanks for checking!

[slyshykO]

@Nightwalker-87
Maybe we should wait with reviewing this PR till chipid_cleanup is being merged?

[Nightwalker-87]

@slyshykO I thought about that also at first, but actually as I'm not planning to change anything in common.c yet, it should be ok I think. It may be better to start a new approach for the latter (otherwise the changes become too large for a merge). We can also safely cancel some points mentioned in #1212 which have separate tickets anyway. I simply could not foresee the evolution of the changes that we went through by now, but that's ok.

[Ant-ON]

@antoinefaure In my opinion, this changes does not check the correctness of entering the starting address of the first page. I would add checks like these:

stlink/src/common.c

Lines 3472 to 3514 in 7cc1fda

	// check addr range is inside the flash
	stlink_calculate_pagesize(sl, addr);
	if (addr < sl->flash_base) {
	ELOG("addr too low %#x < %#x\n", addr, sl->flash_base);
	return (-1);
	} else if ((addr + len) < addr) {
	ELOG("addr overruns\n");
	return (-1);
	} else if ((addr + len) > (sl->flash_base + sl->flash_size)) {
	ELOG("addr too high\n");
	return (-1);
	} else if (addr & 1) {
	ELOG("unaligned addr 0x%x\n", addr);
	return (-1);
	} else if (len & 1) {
	WLOG("unaligned len 0x%x -- padding with zero\n", len);
	len += 1;
	} else if (addr & (sl->flash_pgsz - 1)) {
	ELOG("addr not a multiple of current pagesize (%u bytes), not supported, "
	"check page start address and compare with flash module organisation "
	"in related ST reference manual of your device.\n",
	(unsigned)(sl->flash_pgsz));
	return (-1);
	}
	// make sure we've loaded the context with the chip details
	stlink_core_id(sl);
	// Erase each page
	int page_count = 0;
	for (off = 0; off < len;
	off += stlink_calculate_pagesize(sl, addr + (uint32_t)off)) {
	// addr must be an addr inside the page
	if (stlink_erase_flash_page(sl, addr + (uint32_t)off) == -1) {
	ELOG("Failed to erase_flash_page(%#x) == -1\n", (unsigned)(addr + off));
	return (-1);
	}
	ILOG("Flash page at addr: 0x%08lx erased\n", (unsigned long)(addr + off));
	page_count++;
	}

It would also be nice to fix stlink_write_flash and add a call to stlink_erase_flash_section there instead of the erase code.

[antoinefaure]

@Ant-ON Correct me if I'm wrong, but I think most of the checks you've linked are wrong:

• } else if ((addr + len) < addr) { cannot happen as len is a uint32_t (or in my case a size_t). It can therefore never be < 0, so this test should always pass.
• } else if (addr & 1) { doesn't check that an address is aligned with a page, it just checks if the last bit is a 0. e.g. 0x08000010 & 0x01 = 0x00 so the test will pass, but it is not aligned with a page.
• } else if (len & 1) { same problem, it doesn't make sure len is aligned with a page
• } else if (addr & (sl->flash_pgsz - 1)) { unfortunately we've seen before that we can't use sl->flash_pgsz as some flash have different page sizes depending on the address (first few pages are 32kB, the next ones 128kB for example).

That leaves us with 2 tests :

• if (addr < sl->flash_base) {
• } else if ((addr + len) > (sl->flash_base + sl->flash_size)) {

Both of them are done here:

int stlink_erase_flash_section(stlink_t *sl, stm32_addr_t base_addr, size_t size) {
  if (base_addr < sl->flash_base || (base_addr + size) > (sl->flash_base + sl->flash_size)) {
    ELOG("Invalid address or size\n");
    return (-1);
  }

The alignment checks are done here :

  // Make sure the requested address is aligned with the beginning of a page
  while (addr < base_addr) {
    addr += stlink_calculate_pagesize(sl, addr);
  }
  if (addr != base_addr) {
    ELOG("The address to erase is not aligned with the beginning of a page\n");
    return -1;
  }

And here :

    if ((addr + page_size) > (base_addr + size)) {
      ELOG("Invalid size (not aligned with a page). Page size at address %#x is %#lx\n", addr, page_size);
      return -1;
    }

Unless I missed something, I think we are all good with the checks.

It would also be nice to fix stlink_write_flash and add a call to stlink_erase_flash_section there instead of the erase code.

That's a good catch, I'll fix this!

[Ant-ON]

@antoinefaure

• } else if ((addr + len) < addr) { happened in case of overflow. For example, when the address is 0xFFFFFF10 and the len is 0xFF. Yes, an unobvious check and redundant.
• } else if (addr & 1) { simple checks of address typo. I think this can be redundant.
• } else if (len & 1) { this check is needed only for the write operation. Due to its limitations.
• } else if (addr & (sl->flash_pgsz - 1)) { yes, may write better. This is the simplest test.

Sorry. At a cursory glance, I missed these checks. It is better to separate the checks so that the user understands the problem better.

  if (base_addr < sl->flash_base || base_addr >= (sl->flash_base + sl->flash_size)) {
    ELOG("Address is out of the flash (0x%08X - 0x%08X)\n", sl->flash_base, sl->flash_base + sl->flash_size);
    return (-1);
  }
  if ((base_addr + size) > (sl->flash_base + sl->flash_size)) {
    ELOG("The size exceeds the size of the flash (%u bytes available)\n", sl->flash_base + sl->flash_size - base_addr);
    return (-1);
  }

There is still some redundancy.

stlink/src/common.c

Lines 3472 to 3499 in 7cc1fda

	// check addr range is inside the flash
	stlink_calculate_pagesize(sl, addr);
	if (addr < sl->flash_base) {
	ELOG("addr too low %#x < %#x\n", addr, sl->flash_base);
	return (-1);
	} else if ((addr + len) < addr) {
	ELOG("addr overruns\n");
	return (-1);
	} else if ((addr + len) > (sl->flash_base + sl->flash_size)) {
	ELOG("addr too high\n");
	return (-1);
	} else if (addr & 1) {
	ELOG("unaligned addr 0x%x\n", addr);
	return (-1);
	} else if (len & 1) {
	WLOG("unaligned len 0x%x -- padding with zero\n", len);
	len += 1;
	} else if (addr & (sl->flash_pgsz - 1)) {
	ELOG("addr not a multiple of current pagesize (%u bytes), not supported, "
	"check page start address and compare with flash module organisation "
	"in related ST reference manual of your device.\n",
	(unsigned)(sl->flash_pgsz));
	return (-1);
	}
	// make sure we've loaded the context with the chip details
	stlink_core_id(sl);

This code can be shortened to (all checks done in the stlink_erase_flash_section):

  if (len & 1) {
    WLOG("unaligned len 0x%x -- padding with zero\n", len);
    len += 1;
  }

[Nightwalker-87]

@antoinefaure @Ant-ON Thanks for the comments. Both looks good to me.
It would be indeed very helpful to have some of the explanations from above as inline comments as well.

[antoinefaure]

Hey @Ant-ON @Nightwalker-87
I've moved the checks to 2 separate functions, that way stlink_write_flash also benefits from the updated checks, and from a proper alignment test.

Testing this has been a bit frustrating, as after running make install I ended up with this issue : #1199.
But I've covered all the cases that came to mind : invalid address, invalid (address + size), address not aligned

[Ant-ON]

@antoinefaure Thank you! Everything looks very good!
Yes, the bug with configs has not been fixed yet...(

[Nightwalker-87]

@antoinefaure Very good and valuable contribution. Thank you. 🥇
Technically everything seems to be addressed by now.
Yes, we're working on #1199 don't bother too much about it.
Now we are only waiting to hear @slyshykO and then this can go for a merge.

[Nightwalker-87]

@slyshykO No, we shouldn't continue with this practise of continued explicit conversions.
It should be fixed at the root of the problem and not in in the further course.

[antoinefaure]

Hey guys, thanks for your help and for getting this merged in!
Cheers

[slyshykO]

@slyshykO No, we shouldn't continue with this practise of continued explicit conversions. It should be fixed at the root of the problem and not in in the further course.

The root of the problem is C-lang itself and its many realizations for numerous platforms. You will never guess which base type equal ssize_t unsigned or unsigned long. Wherein, C doesn't expand printf-like format.

[Nightwalker-87]

A solution to this is to work with fixed-length typedefs like int16_t, int32_t, etc.
The described issue is one of the reasons why they were defined.

Have you tested this in the meanwhile?
If not, please do so to allow for final closure of this PR.

[slyshykO]

A solution to this is to work with fixed-length typedefs like int16_t, int32_t, etc. The described issue is one of the reasons why they were defined.

Please, hear me. This is no solution at all when we deal with printf format lines cause its syntax to know nothing about sized types.
Even more, the same uint32_t for GCC is unsigned long but for clang, it is just unsigned. So what format we should use %lx or %x ?

https://godbolt.org/#z:OYLghAFBqd5QCxAYwPYBMCmBRdBLAF1QCcAaPECAMzwBtMA7AQwFtMQByARg9KtQYEAysib0QXACx8BBAKoBnTAAUAHpwAMvAFYTStJg1DIApACYAQuYukl9ZATwDKjdAGFUtAK4sGe1wAyeAyYAHI%2BAEaYxHoADqgKhE4MHt6%2BcQlJAkEh4SxRMVy2mPaOAkIETMQEqT5%2BRXaYDskVVQQ5YZHRegqV1bXpDX3twZ353VwAlLaoXsTI7BwA1CYaAIIrZgDMwcjeWCtbbr34qAB0CCZb2Ksb5jsMe14HV8cE%2BIIXVzfrtyvrwQISxYTGCEEmtxMAHYrOslvCll5AVszAB9IGqQ4AESWZjMV1hGwRsWIgKoEHM%2BLMAFZaKoTNS3AxKaQlqoIVtCX8EcRMAQ5gwlhoCZCoTiAPTipZ4KhLJhcaUKJYMVBA9ACTByoEEBB4JXxQGsrC9YheBwkJaoWXypYkwFKykACRKtFQS0kVLMSwQ0UwHGmtE41N4fg4WlIqE4bms1iWClm8019x4pAImn90wA1iAtlCztT9JxJCH0xHOLwFCANKn09M4LAYIgUKgWLE6NFyJQ0K32zFgFwuGY%2BHQCNFKxAIqWIsEqgBPTgp7tsQQAeQYtHnYd4WBBRnEW9I%2BF5zQAbphKwfMKoml5RwveICSqXaHgIsQ5x4sKWCKSWPfplQBjAAoABqeCYAA7iusSMPeMiCCIYjsFI8HyEoailroRQGEYKAxpY%2BivpWkDTKgsRlAwF4ALQAOrRMQJAViUTQUS4DDuJ4dT%2BOxHR5AUGSJBRAz1KQ8SCckvFdIUxSlC0wzCT0zHNOUwySeM0m9G0ClDG0an8VMMxzAseg/pgiw8AGQYlgekYcKoAAcABsVGOZISzAMgyBLAOZxehA0aWNYrK4IQFrJqyHg9vQxCbFsUy8GmW6TFmEjUmcjk0oWHDFqQf7UtWobhrZFZVjWSWkPWTazAQsS3p2EDdm20X%2BPgRDSfwCGiOIKEdWhKjqAepnmbwEHvrE/5ZcGpCFbwtkrretVAlabJOS5bkeV5Pl%2BZFTXRLF8VlVoyWkD6TBYDE4JZTleUFaWxW2KViVHSlWySL5kgaFwGhQgAnFwWyOY5P0aK5WVbNZRXlod/oVY2lUQEgjW9vVSPNXshj9lsGjVjQtCjsQ46Tge07MMQm6Li2y4EGuG6ljuGP7uGR4sXgZ4XuGV43ne3APoIT4Hi%2Bb4fhgizhj%2BeB/jzAFAaB4FQTBoYpr1iHddIvWKP1mEgEOOHGPhNiC8Rl1kRR1F0cQDHEExsnOBArjaaQgSjHxEyiZkQmcYMbvidkztSYpNsMK0/SeyJjTKUHql%2B%2BpPTyaHse6dH%2BnTPGRnIamvLDZZHBTTNZZ2atrlLOjRjeVsZwaBXSz%2BfrwWtWF2xFEsO29rFZiTAltYpWY9m%2BdSP32T9ZgaE5WxYz9ANXbwN3TXdUOVtWT0w/DSDVYtKMtrtMRDpg9ftbIKvIWrsgaxhg2Z3Bo1MONUuTRDs2cPNNW3pasoOc5Rcl8AZcV1X/mb63e47dO5JWmKdc6lBs7XRAPlWeNl56PS7qQbMPc%2B4DyHiPRyY8NAT0cmDe%2B%2BcQHPSymYAh90l7HTPATZIIBJBAA%3D

[gszy]

Please, hear me. This is no solution at all when we deal with printf format lines cause its syntax to know nothing about sized types. Even more, the same uint32_t for GCC is unsigned long but for clang, it is just unsigned. So what format we should use %lx or %x ?

PRIu32, defined in inttypes.h, see https://en.cppreference.com/w/c/types/integer.

[Nightwalker-87]

Please open a new PR for this issue and reference the respective comment above.
This PR is now closed as the original issue has been addressed.