Skip to content
New issue

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

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

Already on GitHub? # to your account

sys: newlib: make _read_r thread safe #3136

Merged

Conversation

kaspar030
Copy link
Contributor

previously, there was a potential race condition because both ISR and the thread could access the ringbuffer simultaneously. This PR wraps the ringbuffer access from the thread into disable/restreIRQ calls.

(depends on #3111)

@kaspar030 kaspar030 added Type: bug The issue reports a bug / The PR fixes a bug (including spelling errors) Impact: minor The PR is small in size and might only require a quick look of a knowledgeable reviewer State: waiting for other PR State: The PR requires another PR to be merged first labels Jun 1, 2015
@jnohlgard
Copy link
Member

Good catch!
Patch looks good too.

@kaspar030 kaspar030 force-pushed the make_newlib__read_r_thread_safe branch from be4fc65 to 5549d04 Compare June 1, 2015 12:27
@kaspar030
Copy link
Contributor Author

-rebased

@kaspar030 kaspar030 added CI: ready for build If set, CI server will compile all applications for all available boards for the labeled PR and removed State: waiting for other PR State: The PR requires another PR to be merged first labels Jun 1, 2015
@jnohlgard
Copy link
Member

ACK for the change

@jnohlgard
Copy link
Member

Did you test this on the IoT-Lab?

@kaspar030
Copy link
Contributor Author

Yes, working fine.

@kaspar030
Copy link
Contributor Author

no wait, I was testing default, and that uses uart0 ;) sec...

@kaspar030
Copy link
Contributor Author

Checked iot-lab.

count = count < rx_buf.avail ? count : rx_buf.avail;

return ringbuffer_get(&rx_buf, (char*)buffer, count);
res = ringbuffer_get(&rx_buf, (char*)buffer, count);
Copy link
Member

Choose a reason for hiding this comment

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

sorry for a late comment: I noticed that ringbuffer_get returns an unsigned int, do we need to cast that here?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

hm, before we just returned ringbuffer_get directly. And the compiler doesn't complain. And no RIOT device in the near future will have a ring buffer with 2^31 bytes, so not casting should be fine. :)

Copy link
Member

Choose a reason for hiding this comment

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

Yeah, just nitpicking. We can merge this as-is in my opinion

@jnohlgard
Copy link
Member

ACK

jnohlgard pushed a commit that referenced this pull request Jun 1, 2015
@jnohlgard jnohlgard merged commit baf568f into RIOT-OS:master Jun 1, 2015
@kaspar030 kaspar030 deleted the make_newlib__read_r_thread_safe branch February 7, 2017 22:12
# for free to join this conversation on GitHub. Already have an account? # to comment
Labels
CI: ready for build If set, CI server will compile all applications for all available boards for the labeled PR Impact: minor The PR is small in size and might only require a quick look of a knowledgeable reviewer Type: bug The issue reports a bug / The PR fixes a bug (including spelling errors)
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants