Avoid lock collision b/w SerialBase & UARTSerial

[hasnainvirk]

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]

@kjbracey-arm @0xc0170 Please review. @RobMeades

[RobMeades]

I have tested this and it works for me.

[hasnainvirk]

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

[bridadan]

/morph test

[geky]

@c1728p9 this seem fine to you?

[c1728p9]

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]

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?

[bridadan]

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.

[Nodraak]

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

[hasnainvirk]

@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.

[bridadan]

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

[c1728p9]

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]

@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]

@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]

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]

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]

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]

@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]

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.

[amq]

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

[theotherjimmy]

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