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.

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

shell: fix unsafe API calls and add configurable autoflush behavior #84290

Merged
merged 2 commits into from
Jan 23, 2025

Conversation

jakub-uC
Copy link
Contributor

@jakub-uC jakub-uC commented Jan 21, 2025

Fixes an issue where the shell API could block indefinitely when called from threads other than the shell's processing thread, especially when the transport (e.g. USB CDC ACM) was unavailable or inactive.

Changes made:

  1. Replaced k_mutex_lock calls with an indefinite timeout (K_FOREVER) by using a fixed timeout (K_MSEC(SHELL_TX_MTX_TIMEOUT_MS)) in shell API functions to prevent indefinite blocking.
  2. Added a new Kconfig option SHELL_PRINTF_AUTOFLUSH to allow configurable autoflush behavior for shell printing functions.
  3. Updated Z_SHELL_FPRINTF_DEFINE to use the CONFIG_SHELL_PRINTF_AUTOFLUSH setting instead of hardcoding the autoflush behavior to true.

Links: #84274

@zephyrbot zephyrbot added the area: Shell Shell subsystem label Jan 21, 2025
@jakub-uC jakub-uC requested a review from nordic-krch January 21, 2025 10:03
@jakub-uC jakub-uC added bug The issue is a bug, or the PR is fixing a bug priority: high High impact/importance bug labels Jan 21, 2025
@jakub-uC jakub-uC force-pushed the fix/shell-print-deadlock branch from c6785c0 to ac27480 Compare January 21, 2025 10:16
@pdgendt pdgendt added backport v3.7-branch Request backport to the v3.7-branch backport v4.0-branch Backport to the v4.0-branch labels Jan 21, 2025
Copy link
Collaborator

@pdgendt pdgendt left a comment

Choose a reason for hiding this comment

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

Can you split this into 2 commits?

  1. addition of CONFIG_SHELL_PRINTF_AUTOFLUSH
  2. Changing the timeout

@jakub-uC jakub-uC force-pushed the fix/shell-print-deadlock branch from c223c7b to f1488c3 Compare January 21, 2025 11:39
@jakub-uC
Copy link
Contributor Author

@pdgendt done

pdgendt
pdgendt previously approved these changes Jan 21, 2025
nordic-krch
nordic-krch previously approved these changes Jan 21, 2025
Copy link
Collaborator

@kartben kartben left a comment

Choose a reason for hiding this comment

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

commit message should not include github references https://docs.zephyrproject.org/latest/contribute/guidelines.html#adding-links

Introduced a new Kconfig option `SHELL_PRINTF_AUTOFLUSH` to allow
configuring the autoflush behavior of shell printing functions.

Updated `Z_SHELL_FPRINTF_DEFINE` to use the
`CONFIG_SHELL_PRINTF_AUTOFLUSH` setting instead of hardcoding
the autoflush behavior to `true`.

Signed-off-by: Jakub Rzeszutko <[email protected]>
Fixes an issue where the shell API could block indefinitely when called
from threads other than the shell's processing thread, especially when
the transport (e.g. USB CDC ACM) was unavailable or inactive.

Replaced `k_mutex_lock` calls with an indefinite timeout (`K_FOREVER`)
by using a fixed timeout (`K_MSEC(SHELL_TX_MTX_TIMEOUT_MS)`) in shell
API functions to prevent indefinite blocking.

Link: zephyrproject-rtos#84274

Signed-off-by: Jakub Rzeszutko <[email protected]>
@jakub-uC jakub-uC dismissed stale reviews from nordic-krch and pdgendt via 04fa35e January 22, 2025 08:13
@jakub-uC jakub-uC force-pushed the fix/shell-print-deadlock branch from f1488c3 to 04fa35e Compare January 22, 2025 08:13
@jakub-uC
Copy link
Contributor Author

@kartben Fixed, I missed that new rule

@kartben kartben merged commit b0a0feb into zephyrproject-rtos:main Jan 23, 2025
25 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area: Shell Shell subsystem backport v3.7-branch Request backport to the v3.7-branch backport v4.0-branch Backport to the v4.0-branch bug The issue is a bug, or the PR is fixing a bug priority: high High impact/importance bug
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants