drivers: flash: stm32: add "generic read/write" ex op to QSPI driver

[fogzot]

Some external flash modules have extra commands to support, for example, reading/writing an OTP zone. Given that the commands are highly specific and difficult to generalize, we add two ex ops that can be used to transmit a custom command (in the form of a full QSPI_CommandTypeDef) and then read or write a user-provided buffer.

As a reference here is the link to the discussion where one of the reviewers asked for using ex ops instead of custom code in a board.c file.

Link: #90264

[JarmouniA]

As a reference here is the link to the discussion where one of the
reviewers asked for using ex ops instead of custom code in a board.c file.

Link: 90264 

this can be removed from commit message

[de-nordic]

Dynamic allocation should be avoided. Allocating things on stack is risky.
I think driver should use either slab or statically pre allocated number of buffers; the number of buffers would be no more then number of parallel running threads, as not all threads will access flash at the same time.
What should software when driver returns -ENOMEM?

[etienne-lms]

Using 64 bytes of stack seems quite reasonable.
If it's the typical buffer size expected for theses extended ops, maybe simpler to hard code it in the driver rather than adding a config switch:

#ifdef CONFIG_USERSPACE
	QSPI_CommandTypeDef cmd_copy;
	uint8_t out_copy_def[64];

	bool syscall_trap = z_syscall_trap();

	if (syscall_trap) {
		K_OOPS(k_usermode_from_copy(&cmd_copy, cmd, sizeof(cmd_copy)));
		cmd = &cmd_copy;

		if (cmd->NbData <= sizeof(out_copy_def) ) {
			buffer = out_copy_def;
		} else {
			buffer = k_malloc(cmd->NbData);
			if (buffer == NULL) {
				return -ENOMEM;
			}
		}
	}
#endif

[de-nordic]

Using 64 bytes of stack seems quite reasonable.

This sentence is not true, as the value is configurable.

And https://docs.zephyrproject.org/latest/contribute/coding_guidelines/index.html rule 14

[fogzot]

If dynamic allocation should not be used at all, would it be OK to keep the Kconfig option and just return ENOMEM if cmd->NbData is greater than the static allocation. After all, the user is asking for a specific operation and knows exactly how much memory it is supposed to read/write.

[etienne-lms]

Using 64 bytes of stack seems quite reasonable.
This sentence is not true, as the value is configurable.

I was referring to the config default value proposed in the Kconfig file.
I could seem nice to have a config for this but what will prevent one from setting a value that would overflow the stack enough to have side effect and not enough to make those effects not visible?

If this feature is to be used on "reasonably-sized" buffer, I agree the dynamic allocation could be removed. Is alloca() considered a dynamic allocation since its handle by the toolchain? Is alloca() better than a raw variable-in-stack definition (like uint8_t loc_buf[CONFIG_xxx];)?

[etienne-lms]

If dynamic allocation should not be used at all, would it be OK to keep the Kconfig option and just return ENOMEM if cmd->NbData is greater than the static allocation. After all, the user is asking for a specific operation and knows exactly how much memory it is supposed to read/write.

I agree with your proposal.

[fogzot]

I just pushed a new version that removes the dynamic allocation while allowing the user to specify how much stack should be used via a Kconfig variable. I also implemented all other requested changes.

[etienne-lms]

Thanks for the update. The implementation looks good to me.

[etienne-lms]

Few things to update. s/less/less or equal/, s/options/option/, remove the dynamic allocation info.

I suggest either:

	  If the data buffer size is less than or equal to this value, the buffer
	  is allocated in the stack, otherwise operation will fail.

or

	  This option defines the max supported size of data being read/written
	  with the generic read and write extended operation. Be warned that
	  the related buffer is allocated in the stack and this value should be reasonable.

[mathieuchopstm]

Do we even need this? Per https://docs.zephyrproject.org/latest/kernel/usermode/syscalls.html#verification-memory-access-policies:

There is one exception in place, with respect to large data buffers which are only used to provide a memory area that is either only written to, or whose contents are never used for any validation or control flow. Further discussion of this later in this section.
...
Finally, we must consider large data buffers. These represent areas of user memory which either have data copied out of, or copied into. It is permitted to pass these pointers to the implementation function directly. The caller’s access to the buffer still must be validated with K_SYSCALL_MEMORY APIs. The following constraints need to be met:

<...refer to page....>

int z_vrfy_get_data_from_kernel(void *buf, size_t size)
{
   K_OOPS(K_SYSCALL_MEMORY_WRITE(buf, size));
   return z_impl_get_data_from_kernel(buf, size);
}

I think this is exactly the situation we are in.

[etienne-lms]

You mean using k_usermode_alloc_from_copy() and k_usermode_from_copy()/k_usermode_to_copy()?
Sounds good,

There is no k_usermode_alloc_to_copy() helper function. Since k_usermode_alloc_from_copy() relies on z_thread_malloc(), maybe the write operation could use z_thread_malloc() and k_usermode_to_copy().

[fogzot]

My fault, I forgot about the Kconfig description. I'll update it after the other changes if we still need it.

I don't think z_thread_malloc() is accessible outside the kernel proper but I'll investigate how to use the K_SYSCALL_MEMORY API and provide a new patch if that's the right way to go.

[fogzot]

I just pushed a new version that does not copy the "variable length" buffers (but does copy the cmd parameter before anything else because there is some logic based on it). As suggested by @mathieuchopstm the buffers are checked using K_SYSCALL_MEMORY_WRITE and K_SYSCALL_MEMORY_READ.

Also, I removed the Kconfig variable FLASH_STM32_QSPI_GENERIC_STACK_BUFFER_MAX_SIZE.

[sonarqubecloud]

Quality Gate passed

Issues
0 New issues
0 Accepted issues

Measures
0 Security Hotspots
0.0% Coverage on New Code
0.0% Duplication on New Code

See analysis details on SonarQube Cloud

[etienne-lms]

LAGTM