Add platform error codes #1993
Add platform error codes #1993
Conversation
Add error codes for the platform module, to be used by the setup \ terminate API.
RonEld
added
enhancement
mbed TLS team
needs-review
Remove the invalid input for the platform module, as it's not currently used in the Mbed OS platform setup \ termination code.
include/mbedtls/platform.h
Outdated
| @@ -43,6 +43,8 @@ | |||
| #include "platform_time.h" | |||
| #endif | |||
|
|
|||
| #define MBEDTLS_ERR_PLATFORM_HW_FAILED -0x0080 /**< Hardware platform function failed. */ | |||
There was a problem hiding this comment.
Should this just say "Hardware failure"?
There was a problem hiding this comment.
All of our HW failure error codes have the suffix of HW_ACCEL_FAILED.
For exampleMBEDTLS_ARIA_HW_ACCEL_FAILED
This is not a HW accelerator, but more of a platform HW failure, so I removed the "ACCEL" part.
Since the error code was already merged to Mbed OS, unless this is a blocker, I prefer to keep the same error code.
Unless your comment was about the error comment, then I'll change
There was a problem hiding this comment.
Yeah, my comment was about the error message/comment.
There was a problem hiding this comment.
I'll change, but this error is for the platform function, not a generic hardware failure.
How about " Hardware platform operation failure." ?
There was a problem hiding this comment.
I'd suggest:
#define MBEDTLS_ERR_PLATFORM_HW_FAILED -0x0080 /**< Hardware failed platform operation. */
``
include/mbedtls/error.h
Outdated
| @@ -80,6 +80,7 @@ | |||
| * CHACHA20 3 0x0051-0x0055 | |||
| * POLY1305 3 0x0057-0x005B | |||
| * CHACHAPOLY 2 0x0054-0x0056 | |||
| * PLATFORM 1 0x0080-0x0080 | |||
There was a problem hiding this comment.
Petty - but the alignment is out by one column isn't it?
| @@ -43,6 +43,8 @@ | |||
| #include "platform_time.h" | |||
| #endif | |||
|
|
|||
| #define MBEDTLS_ERR_PLATFORM_HW_FAILED -0x0080 /**< Hardware platform function failed. */ | |||
|
|
|||
There was a problem hiding this comment.
Is this really the only error code we want to add?
How about "already in use/busy"? Or "Invalid operation"? I'd suggest out of memory, but I think we already have plans for that.
There was a problem hiding this comment.
I do not know what are the plans to handle the wide range of error codes that underlying platform/hardware might come across, but my guess is that each platform is most likely to end up returning their specific error codes. So I think its probably OK to just have this very generic error macro and add more as needed or see how we can integrate the platform errors with ours in some other way...
There was a problem hiding this comment.
I did have an "invalid operation" error code, but since I wasn't using it at the end, I didn't see a point of adding it.
We could think of many error codes to add, as the hw can fal on numerous reasons. I think that if we find ourselves in a need for other error codes, we could add. We already are low in available error values
There was a problem hiding this comment.
Yes, let's add the error codes when we have a need for them (not in this PR).
|
There are already two reviews with a couple of minor rework suggestions. I will remove the 'needs review' label and add 'needs rework' to highlight that these should be considered (either by code change or replying to the comments) before further review and approval. |
andresag01
added
needs-work
and removed
needs-review
1. Rephrase error description. 2. fix alignment of error list.
RonEld
added
needs-review
|
@sbutcher-arm @Patater I addressed your comments. |
| @@ -43,6 +43,8 @@ | |||
| #include "platform_time.h" | |||
| #endif | |||
|
|
|||
| #define MBEDTLS_ERR_PLATFORM_HW_FAILED -0x0080 /**< Hardware platform function failed. */ | |||
|
|
|||
There was a problem hiding this comment.
Yes, let's add the error codes when we have a need for them (not in this PR).
simonbutcher
added
approved
include/mbedtls/error.h
Outdated
| @@ -80,6 +80,7 @@ | |||
| * CHACHA20 3 0x0051-0x0055 | |||
| * POLY1305 3 0x0057-0x005B | |||
| * CHACHAPOLY 2 0x0054-0x0056 | |||
| * PLATFORM 1 0x0080-0x0080 | |||
There was a problem hiding this comment.
The low level error codes are limited up to 0x7f
This causes mbedtls_strerror() not to recognize `0x80, as it is as a low level error code, but the value belongs to a high level error code.
The valuse should be changed, but it is already merged in Mbed OS
@sbutcher-arm Should we change the value, causing possible ABI issues if users have specifically expect this error code or should we change the limit of the low level error code?
There was a problem hiding this comment.
@RonEld - Can you please change the line to RESERVED? The value isn't properly allocated, so can be reused at some point in the future. It never touched the Mbed TLS standalone release.
There was a problem hiding this comment.
Actually, I should change this description to 0x0070
What do you want me to change to reserved? Since it hasn't been merged yet to Mbed TLS, why should we reserve this value?
Rename the PLATFORM HW error, to avoid ABI breakage with Mbed OS. The value changed as well, as previous value was not in the range of Mbed TLS low level error codes.
RonEld
added
needs-review
include/mbedtls/error.h
Outdated
| @@ -80,6 +80,7 @@ | |||
| * CHACHA20 3 0x0051-0x0055 | |||
| * POLY1305 3 0x0057-0x005B | |||
| * CHACHAPOLY 2 0x0054-0x0056 | |||
| * PLATFORM 1 0x0080-0x0080 | |||
There was a problem hiding this comment.
@RonEld - Can you please change the line to RESERVED? The value isn't properly allocated, so can be reused at some point in the future. It never touched the Mbed TLS standalone release.
library/error.c
Outdated
| @@ -821,6 +825,11 @@ void mbedtls_strerror( int ret, char *buf, size_t buflen ) | |||
| mbedtls_snprintf( buf, buflen, "PADLOCK - Input data should be aligned" ); | |||
| #endif /* MBEDTLS_PADLOCK_C */ | |||
|
|
|||
| #if defined(MBEDTLS_PLATFORM_C) | |||
| if( use_ret == -(MBEDTLS_ERR_PLATFORM_HW_ACCEL_FAILED) ) | |||
| mbedtls_snprintf( buf, buflen, "PLATFORM - Hardware failed platform operation" ); | |||
There was a problem hiding this comment.
I appreciate you probably haven't changed the error text since the PR was first approved, but the error looks confusing to me. We haven't failed a platform operation - but some crypto operation. Could we change the phrasing to:
PLATFORM - Hardware accelerator failed
It's vaguer, but truer to the intent of the error.
1. Change error description to a clearer one. 2. Change value in the error codes ranges description.
|
@sbutcher-arm I addressed your comments. |
RonEld
removed
the
needs-review
simonbutcher
added
approved
|
I was suggesting to mark 0x0080 to be reserved because we had already used it in Mbed OS, but then had removed it's usage. Technically the value had never been properly allocated, but there was still some risk of collision if someone picked up that transient version of Mbed OS. It's not a big deal, and if we mark the value as reserved in a year's time no one will remember what for or why, so best left as it is I think. |
simonbutcher
removed
the
needs-review
Description
Add error codes for the platform module, to be used by
the setup \ terminate API, as suggested in ARMmbed/mbed-os#7099 (comment)
Status
READY
Requires Backporting
NO