drivers: flash: nrf_qspi_nor: refine alignment requirements #24128
Conversation
drivers/flash/nrf_qspi_nor.c
Outdated
| @@ -479,13 +479,13 @@ static inline int qspi_nor_read_id(struct device *dev, | |||
| static int qspi_nor_read(struct device *dev, off_t addr, void *dest, | |||
| size_t size) | |||
| { | |||
| if (!dest) { | |||
| /* destination must be aligned to a 32-bit word. */ | |||
| if ((dest == NULL) || (((uintptr_t)dest % sizeof(uint32_t)) != 0)) { | |||
There was a problem hiding this comment.
nit: u32_t ? I see this is mostly used in this file as well.
There was a problem hiding this comment.
Also, what is the gain of casting the dest to uintptr_t ?
There was a problem hiding this comment.
Isn't void * wider pointre type than uintptr_t?
There was a problem hiding this comment.
Would nrfx_is_word_aligned work as a check here?
There was a problem hiding this comment.
Switched to u32_t (original incorrectly had uint32_t, didn't notice).
dest is a pointer. Modulo operation on a pointer is not defined; the pointer must be cast to an integer. The only integral types to which pointers can legally be cast are intptr_t and uintptr_t, and we don't need signed modulo so uintptr_t is correct.
I was unaware of nrfx_is_word_aligned and don't see it used elsewhere in Zephyr.
|
Possibly one for a separate PR, but would it make sense to check the return code of
|
|
Hi @pabigot, IMO the flash read API should not put any limits on input address and read size or on output address. Only the flash write should do this. |
It's not sufficient that the read size be a multiple of four bytes; the destination address for the read must also be properly aligned. Signed-off-by: Peter Bigot <peter.bigot@nordicsemi.no>
pabigot
force-pushed
the
nordic/20200406a
branch
from
ba1bdca to
4caa587
Compare
Yes, that's probably the underlying cause of the behavior you see. It's a pervasive error throughout this driver. @kamlaz do you want to fix this or should I? |
The existing API documents potential read alignment requirements for offset and size, and does not document any alignment requirements for write. I would expect both to be the same, and to include buffer alignment requirements. |
It's not sufficient that the read size be a multiple of four bytes;
the destination address for the read must also be properly aligned.
Relates to #24122