Skip to content
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.

Sign up for GitHub

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

Jump to bottom

uart_stm32: Fix conflit between poll_out and irq API #40173

Merged
merged 1 commit into from
Nov 23, 2021
Merged

uart_stm32: Fix conflit between poll_out and irq API #40173

merged 1 commit into from
Nov 23, 2021

Conversation

jdascenzio
Copy link
Contributor

Despite the fix #39752 I sent, I continue to have some troubles with the stm32 usart when I'm using console and shell in same time.

I made this branch to illustrate this behavior: https://github.com/jdascenzio/zephyr/commits/shell_bug
The bug can be triggered in shell_module sample
When a "help" command is sent, the answer is received character by character with the printk text:

*** Booting Zephyr OS build v2.7.99-1161-g9db9ca0d2e98  ***
printk! nucleo_l432kc


uart:~$ printk! nucleo_l432kc
printk! nucleo_l432kc
printk! nucleo_l432kc
printk! nucleo_l432kc
printk! nucleo_l432kc
printk! nucleo_l432kc
printk! nucleo_l432kc
printk! nucleo_l432kc
printk! nucleo_l432kc
printk! nucleo_l432kc
printk! nucleo_l432kc
helpprintk! nucleo_l432kc

Please press the <Tab> button to see all available commands.
You can also use the <Tab> button to prompt or auto-complete all commands or its subcommands.
You can try to call commands with <-h> or <--help> parameter for more information.

Shell supports following meta-keys:
  Ctrl + (a key from: abcdefklnpuw)
  Alt  + (a key from: bf)
Please refer to shell documentation for morprintk! nucleo_l432kc
perintk! nucleo_l432kc
p rintk! nucleo_l432kc
pdrintk! nucleo_l432kc
perintk! nucleo_l432kc
ptrintk! nucleo_l432kc
parintk! nucleo_l432kc
pirintk! nucleo_l432kc
plrintk! nucleo_l432kc
psrintk! nucleo_l432kc

To fix this, I added a lock between poll_out and irq API

@github-actions github-actions bot added the platform: STM32 ST Micro STM32 label Nov 8, 2021
@erwango
Copy link
Member

erwango commented Nov 8, 2021

Thanks @jdascenzio for working on this.

@erwango erwango mentioned this pull request Nov 8, 2021
Closed
@zephyrbot zephyrbot added the area: UART Universal Asynchronous Receiver-Transmitter label Nov 8, 2021
@zephyrbot zephyrbot requested review from dcpleung and gmarull November 8, 2021 21:52
evgeniy-paltsev
evgeniy-paltsev reviewed Nov 9, 2021
View reviewed changes
drivers/serial/uart_stm32.h
@@ -58,6 +58,7 @@ struct uart_stm32_data {
uint32_t baud_rate;
/* clock device */
const struct device *clock;
atomic_t tx_lock;
Copy link
Collaborator

Choose a reason for hiding this comment

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

Shouldn't we use Zephyr spinlock instead of re-implementing it in this driver?

Copy link
Member

Choose a reason for hiding this comment

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

Is spinlock available before kernel init ?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

We don't want to lock interrupt so spinlock isn't adapted. I used a simple lock because poll_out could be call in interrupt function.

Copy link
Collaborator

Choose a reason for hiding this comment

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

Is spinlock available before kernel init ?

Yes.

Copy link
Collaborator

Choose a reason for hiding this comment

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

We don't want to lock interrupt so spinlock isn't adapted. I used a simple lock because poll_out could be call in interrupt function.

Well, you are checking the nucleo_l432kc which is based on STM32L432xxxx chip which is based on ARM Cortex-M4 which doesn't have atomic instructions (please correct me if I wrong). So, in this case we end up with atomics-in-C implementation which simply uses spinlock internally. And for single core Zephyr spinlock is just interrupt lock :)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

We don't want to lock interrupt so spinlock isn't adapted. I used a simple lock because poll_out could be call in interrupt function.

Well, you are checking the nucleo_l432kc which is based on STM32L432xxxx chip which is based on ARM Cortex-M4 which doesn't have atomic instructions (please correct me if I wrong). So, in this case we end up with atomics-in-C implementation which simply uses spinlock internally. And for single core Zephyr spinlock is just interrupt lock :)

Yes, you're right but I don't understand what you want to do when you say "use Zephyr spinlock instead of re-implementing it in this drive". The function z_impl_atomic_cas protect correctly my lock variable against the uart interrupt thanks spinlock.

Copy link
Member

Choose a reason for hiding this comment

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

@evgeniy-paltsev Do you have more comments ?

@emillindq
Copy link
Contributor

@jdascenzio @erwango thanks for this patch! It seems to work well.

dcpleung
dcpleung approved these changes Nov 22, 2021
View reviewed changes
Copy link
Member

@dcpleung dcpleung left a comment •
edited
Loading

Choose a reason for hiding this comment

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

nit. use atomic_set() to set data->tx_lock for consistence as data->tx_lock is an atomic variable.

@jdascenzio
uart_stm32: Fix conflit between poll_out and irq API
755e5c8 
A lock was added to manage situation where the API poll_out and irq API
are used in same time.

Signed-off-by: Julien D'ascenzio <[email protected]>
@jdascenzio
Copy link
Contributor Author

nit. use atomic_set() to set data->tx_lock for consistence as data->tx_lock is an atomic variable.

I applied your remarks to the patch

@nashif nashif merged commit d42cef1 into zephyrproject-rtos:main Nov 23, 2021
@erwango erwango mentioned this pull request Nov 30, 2021
Closed
@ycsin
Copy link
Member

ycsin commented Feb 8, 2022 •
edited
Loading

@erwango @cfriedt I think this PR and the following ones (#40782, #41116) are a continuation to fix a bug mentioned in #39752:

If a transmission is made with poll_out and immediately after an other transmission is made with interrupt api the transmission is locked.

@FRASTM @ABOSTM Should they be backported?

@ycsin ycsin added the backport v2.7-branch Request backport to the v2.7-branch label Feb 14, 2022
@zephyrbot zephyrbot mentioned this pull request Feb 14, 2022
Closed
@ycsin ycsin mentioned this pull request Feb 24, 2022
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@dcpleung dcpleung dcpleung approved these changes

@erwango erwango erwango approved these changes

@ABOSTM ABOSTM Awaiting requested review from ABOSTM

@FRASTM FRASTM Awaiting requested review from FRASTM

@gmarull gmarull Awaiting requested review from gmarull

@evgeniy-paltsev evgeniy-paltsev Awaiting requested review from evgeniy-paltsev

@carlescufi carlescufi Awaiting requested review from carlescufi

@cfriedt cfriedt Awaiting requested review from cfriedt

Assignees

@erwango erwango

Labels
area: UART Universal Asynchronous Receiver-Transmitter backport v2.7-branch Request backport to the v2.7-branch platform: STM32 ST Micro STM32
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
8 participants
@jdascenzio @erwango @emillindq @ycsin @evgeniy-paltsev @dcpleung @nashif @zephyrbot