tmedicci commented on code in PR #16016: URL: https://github.com/apache/nuttx/pull/16016#discussion_r2021024832
########## arch/xtensa/src/esp32/esp32_sha.c: ########## Review Comment: Hi @PruteanuVlad , thanks for your effort on updating it! I see a couple of problems regarding the common implementation: 1. `arch/xtensa/src/common/espressif/esp_sha.c` refers to ESP32, while it should be a common interface for all SoCs; 2. `arch/xtensa/src/esp32/esp32_crypto.c` should also be moved to the common driver (considering we would have the common driver implementation); But... Let me make some considerations first: these two files are too coupled, and I don't recall exactly why they were split into two different files. I don't think this is the best approach. In fact, I've studied it a bit further and it should be even easier to implement the common driver approach. You don't need to use the functions from [`sha_ll.h`](https://github.com/espressif/esp-hal-3rdparty/blob/555c2af12b4b08777cf225e265ad28e9f2471a02/components/hal/esp32/include/hal/sha_ll.h). All you need is to add the [`components/hal/sha_hal.c`](https://github.com/espressif/esp-hal-3rdparty/blob/066b294e58c78622175134cd88b5f27567d050e1/components/hal/sha_hal.c) file to be built by NuttX. Note that this file already provides wrappers around the HAL functions, like `sha_hal_read_digest` or `sha_hal_hash_block`. These functions are used by [`components/mbedtls/port/sha/parallel_engine/sha.c`](https://github.com/espressif/esp-hal-3rdparty/blob/a98c920c566debefe050356e3cb6c1320496c186/components/mbedtls/port/sha/parallel_engine/sha.c). Finally, the functions provided are used by [`components/mbedtls/port/sha/parallel_engine/esp_sha1.c`](https://github.com/espressif/esp-hal-3rdpa rty/blob/891617b1e2699c6e3c7d232466816269e1b86ad5/components/mbedtls/port/sha/parallel_engine/esp_sha1.c) (and the other files at the same folder level that implements SHA256 and SHA512 on `mbedtls`). Please note that, for ESP32, both [`components/mbedtls/port/sha/parallel_engine/sha.c`](https://github.com/espressif/esp-hal-3rdparty/blob/a98c920c566debefe050356e3cb6c1320496c186/components/mbedtls/port/sha/parallel_engine/sha.c) and [`components/mbedtls/port/sha/parallel_engine/esp_sha1.c`](https://github.com/espressif/esp-hal-3rdparty/blob/891617b1e2699c6e3c7d232466816269e1b86ad5/components/mbedtls/port/sha/parallel_engine/esp_sha1.c) uses the so-called "parallel_engine". This is because we have different hardware solutions for each SoC, as can be seen [here](https://github.com/espressif/esp-hal-3rdparty/blob/9acdbe277311ec3f2c443fa6dc0384d7b308a30c/components/mbedtls/CMakeLists.txt#L153). In summary, a general common driver implementation isn't possible considering that we have different implementations depending on the SoC's capabilities. That being said, we can keep `arch/xtensa/src/esp32/esp32_crypto.c` at that location but, instead of reimplementing a lot of functions, just use the functions provided by [`components/hal/sha_hal.c`](https://github.com/espressif/esp-hal-3rdparty/blob/066b294e58c78622175134cd88b5f27567d050e1/components/hal/sha_hal.c). Basically, this file would implement on NuttX the equivalent of [`components/mbedtls/port/sha/parallel_engine/sha.c`](https://github.com/espressif/esp-hal-3rdparty/blob/a98c920c566debefe050356e3cb6c1320496c186/components/mbedtls/port/sha/parallel_engine/sha.c), [`components/mbedtls/port/sha/parallel_engine/esp_sha1.c`](https://github.com/espressif/esp-hal-3rdparty/blob/891617b1e2699c6e3c7d232466816269e1b86ad5/components/mbedtls/port/sha/parallel_engine/esp_sha1.c), [`components/mbedtls/port/sha/parallel_engine/esp_s ha256.c`](https://github.com/espressif/esp-hal-3rdparty/blob/891617b1e2699c6e3c7d232466816269e1b86ad5/components/mbedtls/port/sha/parallel_engine/esp_sha256.c) and [`components/mbedtls/port/sha/parallel_engine/esp_sha512.c`](https://github.com/espressif/esp-hal-3rdparty/blob/891617b1e2699c6e3c7d232466816269e1b86ad5/components/mbedtls/port/sha/parallel_engine/esp_sha512.c). `arch/xtensa/src/common/espressif/esp_sha.c` (and any of common-driver approach) can be dropped. I'm sorry for the confusion. Sometimes the solution is even simpler than what we've expected. Most of your work is done here. -- This is an automated message from the Apache Git Service. To respond to the message, please log on to GitHub and use the URL above to go to the specific comment. To unsubscribe, e-mail: commits-unsubscr...@nuttx.apache.org For queries about this service, please contact Infrastructure at: us...@infra.apache.org
