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

drivers: serial: fix potential overflow in fifo_fill and fifo_read #80601

Merged
merged 1 commit into from
Nov 6, 2024

Conversation

ken4647
Copy link
Contributor

@ken4647 ken4647 commented Oct 30, 2024

try to fix issue #80599 , a error of type transform(?) in serial drivers. For fifo_read it may cause wrong read data if parameter size is over 255, while endless looping for fifo_fill, if uart is running faster than cpu.

Fixes #80599

@zephyrbot zephyrbot added platform: NXP Drivers NXP Semiconductors, drivers platform: GD32 GigaDevice platform: Silabs Silicon Labs platform: STM32 ST Micro STM32 platform: Renesas RA Renesas Electronics Corporation, RA area: UART Universal Asynchronous Receiver-Transmitter platform: nRF Nordic nRFx labels Oct 30, 2024
Copy link

Hello @ken4647, and thank you very much for your first pull request to the Zephyr project!
Our Continuous Integration pipeline will execute a series of checks on your Pull Request commit messages and code, and you are expected to address any failures by updating the PR. Please take a look at our commit message guidelines to find out how to format your commit messages, and at our contribution workflow to understand how to update your Pull Request. If you haven't already, please make sure to review the project's Contributor Expectations and update (by amending and force-pushing the commits) your pull request if necessary.
If you are stuck or need help please join us on Discord and ask your question there. Additionally, you can escalate the review when applicable. 😊

@ycsin
Copy link
Member

ycsin commented Oct 30, 2024

The patch looks fine but the your commit message doesn't meet our contribution guideline and the CI is going to fail, please take a look at https://docs.zephyrproject.org/latest/contribute/guidelines.html#commit-message-guidelines

Basically, you need something like this

drivers: serial: fix problem for a possible overflow in serial fifo_fill or fifo_read | <- this title is too long, max is 72 char, current is 86

commit message line 1                                                     | <- each line should be 75 chars or less
commit message line 2                                                     | <- each line should be 75 chars or less
..                                                                        | <- each line should be 75 chars or less
commit message line n                                                     | <- each line should be 75 chars or less

Signed-off-by: John Doe <[email protected]>

I guess you can change the title to be drivers: serial: fix potential overflow in fifo_fill and fifo_read

@ken4647
Copy link
Contributor Author

ken4647 commented Oct 30, 2024

drivers: serial: fix potential overflow in fifo_fill and fifo_read

Thanks, I didn't notice the length. I have re-push my commit with new legal commit message.

@ken4647 ken4647 changed the title drivers: serial: fix problem for a possible overflow in serial drivers. drivers: serial: fix potential overflow in fifo_fill and fifo_read Oct 30, 2024
@jhedberg
Copy link
Member

@ken4647 you still need an actual body to the commit message. You could just put the PR description there. In addition to that you also need a Signed-off-by line (git commit will auto-generate that for you when you pass -s to it).

@ken4647
Copy link
Contributor Author

ken4647 commented Oct 30, 2024

@ken4647 you still need an actual body to the commit message. You could just put the PR description there. In addition to that you also need a Signed-off-by line (git commit will auto-generate that for you when you pass -s to it).

Sorry, I noticed it yet. I will read the guideline seriously to avoid the mistake.

Copy link
Contributor

@gautierg-st gautierg-st left a comment

Choose a reason for hiding this comment

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

For STM32, some other functions are also impacted by this change and should be updated.
Note that I only reviewed the STM32 driver.

drivers/serial/uart_stm32.c Show resolved Hide resolved
drivers/serial/uart_stm32.c Show resolved Hide resolved
@ken4647 ken4647 force-pushed the main branch 2 times, most recently from aaa6548 to cfc353a Compare October 30, 2024 11:37
gautierg-st
gautierg-st previously approved these changes Oct 30, 2024
Copy link
Contributor

@gautierg-st gautierg-st left a comment

Choose a reason for hiding this comment

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

LGTM for STM32

Change the type of num_tx/num_rx to avoid overflow.

Fixes zephyrproject-rtos#80599

Signed-off-by: Zheng Wu <[email protected]>
@ken4647
Copy link
Contributor Author

ken4647 commented Oct 31, 2024

The latest push is to eliminate the notices from CI for meeting the clang-format.

@ken4647 ken4647 requested a review from gautierg-st October 31, 2024 09:06
Copy link
Member

@soburi soburi left a comment

Choose a reason for hiding this comment

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

LGTM for renesas_ra_sci.

@soburi
Copy link
Member

soburi commented Oct 31, 2024

@duynguyenxa
Please check drivers/serial/uart_renesas_ra8_sci_b.c.

Copy link
Member

@duynguyenxa duynguyenxa left a comment

Choose a reason for hiding this comment

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

LGTM for drivers/serial/uart_renesas_ra8_sci_b.c.

@mmahadevan108
Copy link
Collaborator

@dcpleung , can you help review

@mmahadevan108 mmahadevan108 added the bug The issue is a bug, or the PR is fixing a bug label Nov 5, 2024
@dkalowsk dkalowsk merged commit f59e247 into zephyrproject-rtos:main Nov 6, 2024
27 checks passed
Copy link

github-actions bot commented Nov 6, 2024

Hi @ken4647!
Congratulations on getting your very first Zephyr pull request merged 🎉🥳. This is a fantastic achievement, and we're thrilled to have you as part of our community!

To celebrate this milestone and showcase your contribution, we'd love to award you the Zephyr Technical Contributor badge. If you're interested, please claim your badge by filling out this form: Claim Your Zephyr Badge.

Thank you for your valuable input, and we look forward to seeing more of your contributions in the future! 🪁

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area: UART Universal Asynchronous Receiver-Transmitter bug The issue is a bug, or the PR is fixing a bug platform: GD32 GigaDevice platform: nRF Nordic nRFx platform: NXP Drivers NXP Semiconductors, drivers platform: Renesas RA Renesas Electronics Corporation, RA platform: Silabs Silicon Labs platform: STM32 ST Micro STM32
Projects
None yet
Development

Successfully merging this pull request may close these issues.

a overflow loop may happens when uart_stm32_fifo_read_visitor .