uart: update meaning of uart_irq_tx_ready return code

[JordanYates]

Update the documented value range that uart_irq_tx_ready returns in
order to provide more information to the callers about the number of
bytes that could be written with uart_fifo_fill without fragmentation.

Does not break any current users of the API, unless they are explicitly checking for uart_irq_tx_ready() == 1.
Other UART drivers can be updated in the future without violating the API documentation.

Implements #70937.

[JordanYates]

Looking for initial feedback on approach before going through stable API change process

[dcpleung]

Part of me thinks that the term "ready" is a boolean entity: whether it is ready or not (and in this case if there are any errors). So I think a new API can be introduced in this case to return number of bytes that can be transmitted. uart_irq_tx_ready can then use this API to return true or false. Impact to applications would be limited as this does not change the nature of uart_irq_tx_ready. If you can convert all the drivers to use the new API, you can remove the . element in the driver API struct so there is no impact of wasted space regarding to the API struct. However, this will affect any downstream projects with out of tree drivers.

[tmon-nordic]

Looks fine to me, my only concern is about the potential race conditions. What guarantees that there is no concurrent call from other context to uart_fifo_fill() in between uart_irq_tx_ready() and the uart_fifo_fill() using output from uart_irq_tx_ready()?

[JordanYates]

Looks fine to me, my only concern is about the potential race conditions. What guarantees that there is no concurrent call from other context to uart_fifo_fill() in between uart_irq_tx_ready() and the uart_fifo_fill() using output from uart_irq_tx_ready()?

All of these functions are only valid to call inside the interrupt context (triggered by uart_irq_tx_enable), other uses are already specified as undefined behaviour.

[tmon-nordic]

Looks fine to me, my only concern is about the potential race conditions. What guarantees that there is no concurrent call from other context to uart_fifo_fill() in between uart_irq_tx_ready() and the uart_fifo_fill() using output from uart_irq_tx_ready()?

All of these functions are only valid to call inside the interrupt context (triggered by uart_irq_tx_enable), other uses are already specified as undefined behaviour.

What about SMP?

[JordanYates]

Looks fine to me, my only concern is about the potential race conditions. What guarantees that there is no concurrent call from other context to uart_fifo_fill() in between uart_irq_tx_ready() and the uart_fifo_fill() using output from uart_irq_tx_ready()?

All of these functions are only valid to call inside the interrupt context (triggered by uart_irq_tx_enable), other uses are already specified as undefined behaviour.

What about SMP?

Great question, I have no idea. This is not really any different from the current situation though. uart_irq_tx_ready could return 1 to indicate the device is ready to accept more data and then a second core swoops in and makes it not ready.

My suspicion is that the API is not really designed to handle SMP, as the requirement for all operations to be run from an interrupt context sounds like a band-aid to avoid multi-threading issues.

Does the situation of multiple cores trying to access/modify/control a single hardware peripheral even make sense though? That sounds like a recipe for disaster no matter what we do at an API level, short of wrapping everything in mutexes or spinlocks.

[tmon-nordic]

Great question, I have no idea. This is not really any different from the current situation though.

Yes, it is not different from the current situation and this is why I did approve this PR despite this concern.

Does the situation of multiple cores trying to access/modify/control a single hardware peripheral even make sense though? That sounds like a recipe for disaster no matter what we do at an API level, short of wrapping everything in mutexes or spinlocks.

The only case in my mind would be that the interrupt itself is handled by multiple cores. My concern is about the SMP where the interrupt handling can end up in any core, not the heterogeneous multicore systems where there it seems to be out of scope.

On SMP, I believe this can be handled by using spinlocks instead of irq locking, but I don't currently work on any SMP-capable Zephyr target.

[jfischer-no]

Does not break any current users of the API, unless they are explicitly checking for uart_irq_tx_ready() == 1.
Other UART drivers can be updated in the future without violating the API documentation.

I suggest at least mentioning it in the migration guide.

[jfischer-no]

USB LGTM

[nordic-krch]

shouldn't 1 be returned instead of INT_MAX?

[JordanYates]

uart_nrfx_fifo_fill will accept any length buffer without truncating it

[nordic-krch]

data->tx.len contains TX buffer size so it should be returned instead of INT_MAX.

[JordanYates]

Ah, I thought it was the length of the currently sending buffer, will fix

[github-actions]

This pull request has been marked as stale because it has been open (more than) 60 days with no activity. Remove the stale label or add a comment saying that you would like to have the label removed otherwise this pull request will automatically be closed in 14 days. Note, that you can always re-open a closed pull request at any time.

[dkalowsk]

LGTM. Thanks for the clear migration guide addition!