Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

ta: os_test: fix TA time wrap test #771

Open
wants to merge 2 commits into
base: master
Choose a base branch
from

Conversation

jenswi-linaro
Copy link
Contributor

In the test for wrapped TA time in test_time() we check that the time has wrapped and that not too much time has passed. The limit for this is currently 2 seconds of which 1 second is passed in TEE_Wait(). This is currently not enough time with the recent introduction of fTPM being probed in parallel with the test. So in order to avoid occasional false negatives increase the limit to 60 seconds.

Fixes: a2c1ce3 ("ta: os_test: fix TA time wrap test")

Copy link
Contributor

@jforissier jforissier left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Acked-by: Jerome Forissier <jerome.forissier@linaro.org>

It might indicate that fTPM is holding resources for too long though...

@jenswi-linaro
Copy link
Contributor Author

Yeah, I don't know. Maybe it's only starving the TA for CPU time.

In the test for wrapped TA time in test_time() we check that the time
has wrapped and that not too much time has passed. The limit for this is
currently 2 seconds of which 1 second is passed in TEE_Wait(). This is
currently not enough time with the recent introduction of fTPM being
probed in parallel with the test. So in order to avoid occasional false
negatives increase the limit to 60 seconds.

Fixes: a2c1ce3 ("ta: os_test: fix TA time wrap test")
Signed-off-by: Jens Wiklander <jens.wiklander@linaro.org>
Acked-by: Jerome Forissier <jerome.forissier@linaro.org>
@jenswi-linaro
Copy link
Contributor Author

Tag applied

@@ -623,7 +623,7 @@ static TEE_Result test_time(void)
}
MSG("TA time %"PRIu32".%03"PRIu32, t.seconds, t.millis);

if (t.seconds > 1) {
if (t.seconds > 60) {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Test at line 603 is not impacted by the same CPU overload issue?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't know, it's at least far less likely since we're not exiting to the normal world in between.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Maybe fragile, depends on the scheduler. Won't this trigger rare CI test failures?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sure, I'll increase the time there too.

Addressing a comment.

Signed-off-by: Jens Wiklander <jens.wiklander@linaro.org>
@jenswi-linaro
Copy link
Contributor Author

Update

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants