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

critical: fix set exclusive access if not yet defined #5597

Merged
merged 2 commits into from Jan 17, 2018

Conversation

Projects
None yet
10 participants
@0xc0170
Member

0xc0170 commented Nov 28, 2017

Fixes #5555 bug.
In case there is not yet defined __EXCLUSIVE_ACCESS, neither MBED_EXCLUSIVE_ACCESS that
we are introducing, use architecture macros to find out if MBED_EXCLUSIVE_ACCESS can be
enabled.

First I thought that these could be added to CORE_LABELS that we have in tools/targets/init file. That might be better placement, but @theotherjimmy did not like the proposal. If we add a new core, this would need to be defined. In this case it might be overlooked.

@pan-

This comment has been minimized.

Member

pan- commented Nov 28, 2017

It would be nice if that information was available system wide. If we add a new architecture that support LDREX/STREX then we have to apply modifications in RTX and mbed_critical to enable atomic support for that architecture.

// if __EXCLUSIVE_ACCESS rtx macro not defined, we need to get this via own-set architecture macros
#ifndef __EXCLUSIVE_ACCESS
#ifndef MBED_EXCLUSIVE_ACCESS
#if ((__ARM_ARCH_7M__ == 1U) || \

This comment has been minimized.

@alzix

alzix Nov 28, 2017

Contributor

i would suggest adding any kind of reminder to update this upon new architecture arrival

This comment has been minimized.

@0xc0170

0xc0170 Nov 28, 2017

Member

Added in the new commit

#else
#define MBED_EXCLUSIVE_ACCESS 0U
#endif
#endif

This comment has been minimized.

@kjbracey-arm

kjbracey-arm Nov 28, 2017

Contributor

You're not doing anything with __EXCLUSIVE_ACCESS here. Did you mean to have an #else #define MBED_EXCLUSIVE_ACCESS __EXCLUSIVE_ACCCESS?

This comment has been minimized.

@0xc0170

0xc0170 Nov 28, 2017

Member

+1, added new commit addressing it

// if __EXCLUSIVE_ACCESS rtx macro not defined, we need to get this via own-set architecture macros
#ifndef __EXCLUSIVE_ACCESS
#ifndef MBED_EXCLUSIVE_ACCESS
#if ((__ARM_ARCH_7M__ == 1U) || \

This comment has been minimized.

@kjbracey-arm

kjbracey-arm Nov 28, 2017

Contributor

I know this is copying the logic from RTX, but does kind of feel like the test should be the other way around - 0 only if 6M (or 6A). I think we can assume any future architecture will have exclusive access instructions, and we're not backporting to pre-6.

(On the other hand, I'm aware of platforms where the core supported exclusive access instructions, but the memory system lacked a correctly-functioning global monitor so they didn't work properly - so it's worth having the possibility for platform override.)

This comment has been minimized.

@0xc0170

0xc0170 Nov 28, 2017

Member

I know this is copying the logic from RTX, but does kind of feel like the test should be the other way around - 0 only if 6M (or 6A). I think we can assume any future architecture will have exclusive access instructions, and we're not backporting to pre-6.

I could do that. I added a new commit that lists all architecture we are aware of and emits an error if not recognized (or can do warning and default to non-exclusive access, however that does not make much sense?).

This comment has been minimized.

@kjbracey-arm

kjbracey-arm Nov 28, 2017

Contributor

That error version seems fine. I think it's worth double-checking this sort of code when adding a new arch anyway.

@pan-

pan- approved these changes Nov 28, 2017

@0xc0170 0xc0170 added needs: CI and removed needs: review labels Nov 28, 2017

@0xc0170

This comment has been minimized.

Member

0xc0170 commented Nov 28, 2017

/morph build

@theotherjimmy

This comment has been minimized.

Contributor

theotherjimmy commented Nov 28, 2017

I don't like adding it to any python file as it adds magic to the tools. I'm generally working to remove magic where I can, and adding MBED_EXCLUSIVE_ACCESS to the tools feels like magic to me.

@mbed-ci

This comment has been minimized.

mbed-ci commented Nov 28, 2017

Build : SUCCESS

Build number : 615
Build artifacts/logs : http://mbed-os.s3-website-eu-west-1.amazonaws.com/?prefix=builds/5597/

Triggering tests

/morph test
/morph uvisor-test
/morph export-build

@mbed-ci

This comment has been minimized.

@mbed-ci

This comment has been minimized.

@0xc0170

This comment has been minimized.

Member

0xc0170 commented Nov 29, 2017

/morph test

@mbed-ci

This comment has been minimized.

@0xc0170

This comment has been minimized.

Member

0xc0170 commented Nov 29, 2017

This fails NUCLEO_F746ZG-IAR.tests-netsocket-gethostbyname.tests-netsocket-gethostbyname . I'll find how to reproduce this, if I find locally a board or will ask for help. It might be that this fix results in IAR only failure for this board...

@0xc0170

This comment has been minimized.

Member

0xc0170 commented Nov 29, 2017

/morph test

@mbed-ci

This comment has been minimized.

@kegilbert

This comment has been minimized.

Contributor

kegilbert commented Nov 29, 2017

Had storage errors on the CI end, rekicking off the test.
/morph test

@mbed-ci

This comment has been minimized.

@0xc0170

This comment has been minimized.

Member

0xc0170 commented Nov 30, 2017

@kegilbert This might be real failure. I'll find a board somewhere to reproduce this locally.

@adbridge

This comment has been minimized.

Contributor

adbridge commented Dec 29, 2017

@0xc0170 Any update on this ?

@mbed-ci

This comment has been minimized.

@0xc0170

This comment has been minimized.

Member

0xc0170 commented Jan 9, 2018

/morph test

@mbed-ci

This comment has been minimized.

@0xc0170 0xc0170 added ready for merge and removed needs: CI labels Jan 9, 2018

@cmonr

This comment has been minimized.

Contributor

cmonr commented Jan 16, 2018

That was odd. Resolving merge conflicts resulted in two additional commits. Rebuilding, just in case.

/morph build

@mbed-ci

This comment has been minimized.

mbed-ci commented Jan 16, 2018

@0xc0170

This comment has been minimized.

Member

0xc0170 commented Jan 17, 2018

Ill rebase this PR and restart CI

@0xc0170 0xc0170 force-pushed the 0xc0170:fix_#5555 branch from 7da96b5 to c978be7 Jan 17, 2018

0xc0170 added some commits Nov 28, 2017

critical: fix set exclusive access if not yet defined
Fixes #5555 bug.
In case there is not yet defined __EXCLUSIVE_ACCESS, neither MBED_EXCLUSIVE_ACCESS that
we are introducing, use architecture macros to find out if MBED_EXCLUSIVE_ACCESS can be
enabled.
critical: fix non valid configuration
If any architecture is added, needs  to update critical exclusive access.

Also fixing if exclusive access is defined, we use the value.

@0xc0170 0xc0170 force-pushed the 0xc0170:fix_#5555 branch from c978be7 to 4118542 Jan 17, 2018

@0xc0170

This comment has been minimized.

Member

0xc0170 commented Jan 17, 2018

That was odd. Resolving merge conflicts resulted in two additional commits. Rebuilding, just in case.

This leads to merge commits that are not necessary, I rebased this locally to clean the history

/morph build

@mbed-ci

This comment has been minimized.

mbed-ci commented Jan 17, 2018

Build : SUCCESS

Build number : 882
Build artifacts/logs : http://mbed-os.s3-website-eu-west-1.amazonaws.com/?prefix=builds/5597/

Triggering tests

/morph test
/morph uvisor-test
/morph export-build

@mbed-ci

This comment has been minimized.

@0xc0170

This comment has been minimized.

Member

0xc0170 commented Jan 17, 2018

/morph export-build

@mbed-ci

This comment has been minimized.

@mbed-ci

This comment has been minimized.

@cmonr cmonr added ready for merge and removed needs: CI labels Jan 17, 2018

@cmonr cmonr merged commit bb8ab66 into ARMmbed:master Jan 17, 2018

19 checks passed

ARM mbed CI Verification build successful.
Details
AWS-CI uVisor Build & Test Success
Details
ci-morph-build build completed
Details
ci-morph-exporter build completed
Details
ci-morph-test test completed
Details
continuous-integration/jenkins/pr-head This commit looks good
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
travis-ci/docs Local docs testing has passed
Details
travis-ci/events Local events testing has passed
Details
travis-ci/littlefs Local littlefs testing has passed
Details
travis-ci/mbed2-ATMEL Local mbed2-ATMEL testing has passed
Details
travis-ci/mbed2-MAXIM Local mbed2-MAXIM testing has passed
Details
travis-ci/mbed2-NORDIC Local mbed2-NORDIC testing has passed
Details
travis-ci/mbed2-NUVOTON Local mbed2-NUVOTON testing has passed
Details
travis-ci/mbed2-NXP Local mbed2-NXP testing has passed
Details
travis-ci/mbed2-RENESAS Local mbed2-RENESAS testing has passed
Details
travis-ci/mbed2-SILICON_LABS Local mbed2-SILICON_LABS testing has passed
Details
travis-ci/mbed2-STM Local mbed2-STM testing has passed
Details
travis-ci/tools Local tools testing has passed
Details

@cmonr cmonr removed the ready for merge label Jan 17, 2018

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment