Multi-threading flash access is not supported by flash_write_protection_set()

[nvlsianpu]

The problem
When two or more threads writes to flash device it is possible that flash protection will be enabled by one of threads despite another thread(s) still needs it disabled.

Impact

flash_write() or flash_erase() operation might fail unexpectedly while flash device is written or erased from multiple thread. This will be race condition.

Solution

Remember that below code is allowed:

flash_write_protection_set(flash_dev, false);
flash_erase(flash_dev, page_offset, page_size);

flash_write_protection_set(flash_dev, false);
flash_write(flash_dev, offset, data, sizeof(data));

flash_write_protection_set(flash_dev, true); // enable is recommended

Count flash protection disable/enable operation (with thread instance awareness, a thread might only increase/decrease counter by one).
Re-enable protection only once is zero.

Solution Alternative 1
make flash_write_protection_set() a nop and to include this in flash_write() and flash_erase().

Solution Alternative 2
flash_write_protection_set() reserves flash access for the caller. Looks like this approach can be combined with Solution Alternative 1.

[nvlsianpu]

cc @anangl @carlescufi

[armandciejak]

@nvlsianpu Your proposed solution would not work for flash chip that automatically enables write protection after write or erase command.

I have a custom board with such a flash chip and I'm impacted by this bug. The only solution I found so far is to hack the driver so that the lock is acquired by flash_write_protection_set(flash_dev, false); and released by flash_erase or flash_write. flash_write_protection_set(flash_dev, true); is a no-op for such flash.

[armandciejak]

IMHO the only proper way to address this problem is to change the flash API. Removing the flash_write_protection_set function and changing flash_erase and flash_write so that they acquire the lock, disable the write protection do the erase/write operation, enable the write-protection and finally release the lock.

[nvlsianpu]

@armandciejak

@nvlsianpu Your proposed solution would not work for flash chip that automatically enables write protection after write or erase command.

I think you understand me bad. I put the code sample in the description (same here https://docs.zephyrproject.org/latest/reference/peripherals/flash.html#c.flash_write_protection_set) - this code sample reflect usage with the automatic flash protection your mentioned, that why ther are dedicated flash disable calls before each write/erase operations. Solution I suggested will work for this case as well.

API change you mentioned is also possible solution worth to be considered.

[Laczen]

@nvlsianpu, the thread awareness seems unnecessary complex. Maybe it is better to make flash_write_protection_set a nop and to include this in flash_write and flash_erase.

[nvlsianpu]

@Laczen Agree, that will be much simpler but requires modification in each driver. I would go this way. Probably need to first present this on the API meeting.

[Laczen]

Maybe it is best to standardize to the way it is used in soc_flash_lpc.c and soc_flash_mcux.c.

[nvlsianpu]

^^That's another option, and can be used besides flash_write_protection_set is a nop in the zephyr codebase (as soc_flash_nrf.c driver already does).

[carlescufi]

API meeting 9th March 2021

Conclusion is to go with the approach described as Alternative 1: deprecate flash_write_protection_set () and unprotect/protect the flash on each call to write or erase.

[Laczen]

@carlescufi, agreed.