RFC: API change: Add flash write protection and readout protection functions

[duda-patryk]

Introduction

This RFC was created to discuss introducing flash write protection (WP) and readout protection (RDP) support in flash API

Problem description

Flash controllers in most MCUs (e.g. all STM devices) provide additional features like WP and RDP. With existing flash API, it's not possible to use the features.

Proposed change

I propose to extend flash API with following functions

typedef int (*flash_api_set_write_protect)(const struct device *dev,
					   off_t offset, size_t size,
					   bool enable);

typedef int (*flash_api_get_write_protect)(const struct device *dev,
					   off_t offset, size_t size,
					   bool *enabled);

typedef int (*flash_api_set_readout_protection)(const struct device *dev,
						off_t offset, size_t size,
						bool enable, bool permanent);

typedef int (*flash_api_get_readout_protection)(const struct device *dev,
						off_t offset, size_t size,
						bool *enabled, bool *permanent);

Detailed RFC

Write protection protects flash contents from accidental erases and writes due to program or human error.
Persistent flash write protection is usually implemented in two ways:

• Sector based - protected area can be controlled per sector.
• Range based - protected area is defined by a pair of offsets. On most devices it's possible to define more than one pair.

Readout protection protects against reading flash contents using debugger and internal bootloader. Disabling the protection usually erases protected area. It's possible to enable RDP permanently. On STM MCUs RDP is enabled on entire flash. I haven't met a device with different RDP control, but I can imagine RDP controlled on sector level.

Proposed change (Detailed)

Pull request: #53441
Implementation for STM32F4: #52980

Extend flash API with 4 functions. Two functions to set/get WP state and two functions to set/get RDP state. All functions take offset and size which is flexible interface. Driver will be responsible to translate offset and size to e.g. sectors. If it's not possible to protected requested area or protected area will be bigger than requested, the driver should return an error.

Extend flash Kconfig with following options. They are meant to increase security or prevent from doing irreversible things:

• FLASH_BLOCK_WRITE_PROTECT_DISABLING blocks disabling flash write protection
• FLASH_ALLOW_DISABLING_READOUT_PROTECTION allows disabling readout protection
• FLASH_ALLOW_PERMANENT_READOUT_PROTECTION allows enabling readout protection permanently

Dependencies

Proposed change neither doesn't depend nor affects anything in Zephyr.

Concerns and Unresolved Questions

Using offset and size to describe flash area is a good way to ensure compatibility with all flash controllers but if the area we want to protect is not continuous then we need to call function multiple times. This causes multiple erase + write cycles which can affect flash lifespan if changing WP settings frequently.

Alternatives

I'm not aware of any alternatives.

[Laczen]

@semihalf-duda-patryk, thank you for the RFC. I have some questions:

1. Should this be accompanied with a "flash page layout" definition describing the blocks ?
2. Would it not be more appropiate to define a set flash_api_set_config and flash_api_get_config instead of defining a api extension for each function ?

[duda-patryk]

Should this be accompanied with a "flash page layout" definition describing the blocks ?

No. Some STM32 MCUs have range based write protection so, they don't need layout to work properly. Another example is RDP on STM32 - we can only protect entire flash, so information about layout is not needed.

If flash layout is needed (e.g. sector based write protection) Kconfig for flash driver can define FLASH_HAS_WRITE_PROTECT only when flash page layout is enabled.

Would it not be more appropiate to define a set flash_api_set_config and flash_api_get_config instead of defining a api extension for each function ?

Do you think about something like this?

struct flash_config {
    bool wp_enabled;
    bool rdp_enabled;
    bool rdp_permanent;
}

typedef int (*flash_api_set_config)(const struct device *dev,
				    off_t offset, size_t size,
				    struct flash_config *config);

This has some drawbacks, e.g. on STM32 we are not able to enable/disable RDP on part of flash, so we would need some kind of tri-state enum (enable, disable, leave unchanged). Probably the better solution would be using an union:

struct flash_config {
    union {
        // For RDP
        struct {
            bool rdp_enabled;
            bool rdp_permanent;
        };
        // For WP
       struct {
           bool wp_enabled;
       };
    };
};

typedef int (*flash_api_set_config)(const struct device *dev,
				    off_t offset, size_t size,
                                    enum flash_config_type type,
			            struct flash_config *config);

But I don't know if we want to go this way.

[Laczen]

Should this be accompanied with a "flash page layout" definition describing the blocks ?

No. Some STM32 MCUs have range based write protection so, they don't need layout to work properly. Another example is RDP on STM32 - we can only protect entire flash, so information about layout is not needed.

If flash layout is needed (e.g. sector based write protection) Kconfig for flash driver can define FLASH_HAS_WRITE_PROTECT only when flash page layout is enabled.

I know that on many devices the protection layout is different from the sector (erase-block) layout. To verify the requested range this layout should be known, this is why I think that this would be required. If there is a difference between read and write protection there should even be 2.

Would it not be more appropiate to define a set flash_api_set_config and flash_api_get_config instead of defining a api extension for each function ?

Do you think about something like this?

struct flash_config {
    bool wp_enabled;
    bool rdp_enabled;
    bool rdp_permanent;
}

...  
But I don't know if we want to go this way.

No, I was more thinking about something comparable to int disk_access_ioctl(const char *pdrv, uint8_t cmd, void *buff); for flash devices, where the cmd entry would be used to specify what is being configured/read and the buff used to pass configuration parameters. Internally the driver then calls the appropiate routines, but this is hidden for the user.

[duda-patryk]

To verify the requested range this layout should be known

Yes. In sector based write protection the driver has to convert offset + size range to sectors (or write protection units to be precise), but I think we should add requirement for flash layout in driver Kconfig rather than general flash Kconfig.

I was more thinking about something comparable to int disk_access_ioctl(const char *pdrv, uint8_t cmd, void *buff);

I like it!
So, for write protection it will look like this?

#define FLASH_CONFIG_WRITE_PROTECTION 1

struct flash_config_write_protection {
    off_t offset;
    size_t size;
    bool enabled;
};

typedef int (*flash_api_set_config)(const struct device *dev,
				    uint8_t cmd, void *buf);
typedef int (*flash_api_get_config)(const struct device *dev,
				    uint8_t cmd, void *buf);

I think we can extend struct flash_config_write_protection with bool next field or pass buffer size in syscall. This way, it will be possible to set write protection on discontinuous area in one syscall (this was my concern).

[Laczen]

To verify the requested range this layout should be known

Yes. In sector based write protection the driver has to convert offset + size range to sectors (or write protection units to be precise), but I think we should add requirement for flash layout in driver Kconfig rather than general flash Kconfig.

In some devices the write protection units differ from the sectors (erase-blocks), so I think it is needed to make a construct like the flash_layout for the write protection units.

[Laczen]

I was more thinking about something comparable to int disk_access_ioctl(const char *pdrv, uint8_t cmd, void *buff);

I like it! So, for write protection it will look like this?

#define FLASH_CONFIG_WRITE_PROTECTION 1

struct flash_config_write_protection {
    off_t offset;
    size_t size;
    bool enabled;
};

typedef int (*flash_api_set_config)(const struct device *dev,
				    uint8_t cmd, void *buf);
typedef int (*flash_api_get_config)(const struct device *dev,
				    uint8_t cmd, void *buf);

I think we can extend struct flash_config_write_protection with bool next field or pass buffer size in syscall. This way, it will be possible to set write protection on discontinuous area in one syscall (this was my concern).

I was thinking about only one routine and to use the cmd entry to either use it as a get or a set command, but I am also open to two routines.

@de-nordic, @erwango, @FRASTM, what is your opinion on adding a routine to "configure" the behavior of flash ? Would it be good to seperate get from set and to provide 2 routines ?

[de-nordic]

I would go with single API call, something like flash_ex_op(const struct device *dev, uint32_t code, const uintptr_t *in, uintptr_t *out) (extended operations).
code could have reserved range for Zephyr codes and vendor codes; in and out would have code specific definitions and may not be required for all calls.
I would not like to separate set and get, I would go the ioctl way as proposed by @Laczen.

The proposed callbacks would go into vendor codes, as currently there has not been enough research put into other write/read protection capabilities of other vendors to have one common Zephyr code to cover the features.

IMHO layout query functions (flash layout) are not needed, because write/read protection has very specific and targeted use and will be applied by user with knowledge of what is to be protected; the driver should still be able to respond with error when requested protection change does not exactly cover flash range or protection bits do not apply to selected range. When user can not exactly request rage or proper bits for range, there is no recovery.
Another problem with layout is that there may be multi level layout which again complicates responding to query operation.

2023-01-27 Updated function signature with missing device pointer.

[duda-patryk]

code could have reserved range for Zephyr codes and vendor codes

Having a range of codes reserved for vendor specific operations is a great idea. Are there any examples of how the codes can be divided?

The proposed callbacks would go into vendor codes, as currently there has not been enough research put into other write/read protection capabilities of other vendors to have one common Zephyr code to cover the features.

Do you think we can have more than one code for the same operation? Let's take write protection for example: on STM32F4 each sector is a write protection unit, so protected sectors can be represented using bitmask. I'm sure that Zephyr code won't use bitmask, because there are flash controllers in which protected area is defined using offsets. In the future we could end up with two codes. Zephyr code will be universal (e.g. offset + size) and vendor code will be flash specific (in STM32F4 case it will be bitmask). I actually think that this is a nice feature.

IMHO layout query functions (flash layout) are not needed, because write/read protection has very specific and targeted use and will be applied by user with knowledge of what is to be protected; the driver should still be able to respond with error when requested protection change does not exactly cover flash range or protection bits do not apply to selected range. When user can not exactly request rage or proper bits for range, there is no recovery.
Another problem with layout is that there may be multi level layout which again complicates responding to query operation.

👍

[de-nordic]

Having a range of codes reserved for vendor specific operations is a great idea. Are there any examples of how the codes can be divided?

Assuming that we have uint16_t for code we could have higher half, so everything with MSb == 1 is vendor codes.
We have single Flash API call flash_ex_op that then will be calling device provided flash_ex_op call. The lower range will be formally reserved and available via Flash API headers, the lower range would require including vendor headers. There should be no collision because even if both vendors define some code identifier with the same numeric code, the code is used in context of the device object.
For example let's assume ST has some code defined in stm_ex_op.h

#define STM_LOW_POWER 0x8001 // takes no params

Then in code you would have:

#if USING_STM_DEVICE
#include <some/path/to/headers/stm_ex_op.h>
#endif

...

#if USING_STM_DEVICE
    rc = flash_ex_op(dev, STM_LOW_POWER, NULL, NULL);

    if (rc == -ENOTSUP) {
        // The device does not support the code, but we accept it;
        LOG_DBG("Unsupported STM_LOW__POWER on %p\n", dev);
        ...
    } else if (rc != 0) {
        // It should be supported so error
       LOG_ERR("...");
       ... 
    }
    ...
#endif

Do you think we can have more than one code for the same operation? Let's take write protection for example: on STM32F4 each sector is a write protection unit, so protected sectors can be represented using bitmask. I'm sure that Zephyr code won't use bitmask, because there are flash controllers in which protected area is defined using offsets. In the future we could end up with two codes. Zephyr code will be universal (e.g. offset + size) and vendor code will be flash specific (in STM32F4 case it will be bitmask). I actually think that this is a nice feature.

Yes, you may have several of these, but I would not use it for bitmaps because you will run out of the code space quickly (and block others).
The vendor code for the vendor codes can take in and out pointers and do whatever vendor wants with it.
You can have your own struct like:

struct some_st_in {
   uint32_t size_of_this;
   ... some stuff
}

and in code you would use:

struct some_st_in param_to_call = { .size_of_this = sizeof(some_st_in), ... };
flash_ex_op(dev, STM_SOME, &param_to_call, NULL);
...

(Note: I am still wondering whether size params for in and out structures should be provided so that the Flash API call could do copy-in/copy-out, when needed, instead of dropping that on the device driver).

In case when enough vendors do they versions of certain API extended operation, for example the read/write locks, we can then gather the info and provide common code in the non-vendor space, with common definition of structures and so on.
Basically vendor space will allow us to evaluate how can we approach a solution in common code, while we lack knowledge on approach by other vendors.

[duda-patryk]

Thanks! I'll try to implement it next week.

[duda-patryk]

I applied changes to both PRs #53441 (API change) and #52980 (Implementation for STM32F4) as discussed here.

[heinwessels]

Great to see this being worked on, was about to make an issue myself. My use case is the STM32 QSPI Nor Flash which can have writes silently fail if the write-protection bits (BP) are set. Being able to verify the write protection first will solve the problem, so we will likely extend flash_stm32_qspi.c once the API change is merged.