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

Avoid lock collision b/w SerialBase & UARTSerial #4538

Merged
merged 1 commit into from Jun 26, 2017

Conversation

Projects
None yet
@hasnainvirk
Contributor

hasnainvirk commented Jun 13, 2017

Description

Fixes issue #4537. SerialBase and UARTSerial happened to have same names
for the mutex locking that confused the system of holding a mutex in interrupt context.
UARTSerial has to change so as to provide empty implementations for lock() and unlock()
becuase it uses SerialBase from interrupt context only or from its own critical section,
so no extra locks required.
Private locks for UARTSerial itself are renamed to api_lock() and api_unlock().

Status

READY

Migrations

Doesn't change behaviour.

NO

@hasnainvirk

This comment has been minimized.

Contributor

hasnainvirk commented Jun 13, 2017

@RobMeades

This comment has been minimized.

Contributor

RobMeades commented Jun 13, 2017

I have tested this and it works for me.

@hasnainvirk

This comment has been minimized.

Contributor

hasnainvirk commented Jun 13, 2017

@RobMeades Thanks a lot for going through this and testing.

Avoid lock collision b/w SerialBase & UARTSerial
Fixes issue #4537. SerialBase and UARTSerial happened to have same names
for the mutex locking that confused the system of holding a mutex in interrupt context.
UARTSerial has to change so as to provide empty implementations for lock() and unlock()
becuase it uses SerialBase from interrupt context only or from its own critical section,
so no extra locks required.
Private locks for UARTSerial itself are renamed to api_lock() and api_unlock().

@hasnainvirk hasnainvirk force-pushed the hasnainvirk:avoid_mtx_lock branch to 64a92df Jun 13, 2017

@bridadan

This comment has been minimized.

Contributor

bridadan commented Jun 13, 2017

/morph test

@geky

This comment has been minimized.

Member

geky commented Jun 13, 2017

@c1728p9 this seem fine to you?

@c1728p9

This comment has been minimized.

Contributor

c1728p9 commented Jun 13, 2017

Its unfortunate that you can't override the existing lock function to provide meaningful synchronization. The new locks are private at least so this isn't part of the public API. I'm fine with this patch.

@mbed-bot

This comment has been minimized.

mbed-bot commented Jun 13, 2017

Result: SUCCESS

Your command has finished executing! Here's what you wrote!

/morph test

Output

mbed Build Number: 543

All builds and test passed!

@bridadan

@0xc0170 or @c1728p9 can you sanity check me here?

@@ -14,7 +14,7 @@
* limitations under the License.
*/
#if DEVICE_SERIAL
#if (DEVICE_SERIAL && DEVICE_INTERRUPTIN)

This comment has been minimized.

@bridadan

bridadan Jun 13, 2017

Contributor

Seems a bit funny to add this check here? Not seeing this using the InterruptIn APIs here, which usually what this guard is used for.

There currently isn't a target that has SERIAL but not INTERRUPTIN listed in its device_has descriptor, so this won't cause immediate issues. Just seems unnecessary.

This comment has been minimized.

@Nodraak

Nodraak Jun 14, 2017

Contributor

That was discussed in #4524 (issue is #4515). Interrupt is declared here :)

This comment has been minimized.

@hasnainvirk

hasnainvirk Jun 14, 2017

Contributor

@bridadan Totally agree with you. Probably I should have added this bit in commit message. UARTSerial do use InterruptIn class, but yes in agreement with you, nobody can use UARTSerial without having interrupts. But just for consistency of the system, added this flag.

This comment has been minimized.

@bridadan

bridadan Jun 14, 2017

Contributor

Dude, my bad totally missed it. Definitely the right time to use the flag! Good catch.

@c1728p9

This comment has been minimized.

Contributor

c1728p9 commented Jun 14, 2017

As a simpler fix you could skip the call to SerialBase::writeable(). If the TX interrupt fires then you should be safe to write the next byte. That way you could use SerialBase's lock safely.

@hasnainvirk

This comment has been minimized.

Contributor

hasnainvirk commented Jun 15, 2017

@c1728p9 The thing is tx_irq() method can also be called from UARTSerial::write(), that's why we can't only rely on tx interrupt.

@c1728p9

This comment has been minimized.

Contributor

c1728p9 commented Jun 15, 2017

@hasnainvirk in the case of UARTSerial::write() you already check the flag _tx_irq_enabled which should be sufficient. It is only set to true false on boot or after a TX interrupt has fired and there is no more data to send.

@kjbracey-arm

This comment has been minimized.

Contributor

kjbracey-arm commented Jun 15, 2017

tx_irq_enabled is false to start with. It's only true when we find we managed to fill the hardware buffers in a write.

I guess you could deduce we're probably writable if tx_irq_enabled is false, but that isn't 100% I think - Iwould have to ponder exact buffer fills.

And it doesn't solve the original problem or lock confusion - we're also calling SerialBase::writeable() from interrupt, not just that write call. I think we have to do it like that to ensure we fill the serial buffer to deassert and retrigger the interrupt, if it's edge-based. Writing 1 byte per interrupt may not be sufficient - IRQ as we became writable, after writing a byte still still writable, no new interrupt.

The interrupt enable/disable logic and pump flow was largely copied from the original BufferedSerial - basically because I had good confidence that it had worked with a wide variety of serial HALs. I didn't fancy trying anything new, figuring that it was managing to avoid any API landmines.

@c1728p9

This comment has been minimized.

Contributor

c1728p9 commented Jun 15, 2017

Hi @kjbracey-arm, that is understandable. I think buffered serial got around the problem by not having a lock at all.

As for the call to writeable, this is only done in tx_irq(). If there isn't a transfer in progress, write() simulates an isr by calling tx_irq() in a critical section, which is where the problem comes in. If you remove the check of writeable from tx_irq then it would fix both write() and the isr.

I'm not saying you need to go with this change. I just wanted to propose it, since could potentially be a smaller and more straight forward change. The lock and unlock functions of SerialBase could be used as the lock for both. I'll leave the call to if you want to do it this way, or with the existing implementation up to you and @hasnainvirk. Either way works for me.

@kjbracey-arm

This comment has been minimized.

Contributor

kjbracey-arm commented Jun 16, 2017

How would I remove the check of writable in the ISR though? The ISR is "while (writeable) fill HW", and I'm not sure what the alternative is. I'm pretty certain "write one byte and return" wouldn't guarantee another interrupt for the next byte.

@theotherjimmy

This comment has been minimized.

Contributor

theotherjimmy commented Jun 19, 2017

@hasnainvirk @c1728p9 @kjbracey-arm Have we reached a conclusion from all of that discussion? Does this PR need to be updated as a result?

@kjbracey-arm

This comment has been minimized.

Contributor

kjbracey-arm commented Jun 20, 2017

I don't believe any changes are required to this PR.

@c1728p9 has been suggesting some different ways of implementing the data pump, but they're as yet unproven. And he said "Either way works for me."

This PR avoids the new asserts without changing the proven data pump behaviour. It was never intended that there be any SerialBase mutex claims, and during development the accidental mutex claims just failed silently, as they do now with debug off.

@0xc0170 0xc0170 added needs: CI and removed needs: review labels Jun 20, 2017

@amq

This comment has been minimized.

Contributor

amq commented Jun 23, 2017

This fixes a breaking issue in the cellular functionality. Do you think it could still land in 5.5.2?

@theotherjimmy theotherjimmy merged commit 17ae9b9 into ARMmbed:master Jun 26, 2017

4 checks passed

Cam-CI uvisor Build & Test Success
Details
ci/morph-test Job has completed
Details
continuous-integration/jenkins/pr-head This commit looks good
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
@theotherjimmy

This comment has been minimized.

Contributor

theotherjimmy commented Jun 26, 2017

@amq yep. It's merged and scheduled for 5.5.2

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