Skip to content

serial: break from read after closing #2036

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
Merged
davids5 merged 1 commit into from
Oct 21, 2020
Merged

serial: break from read after closing #2036

davids5 merged 1 commit into from
Oct 21, 2020

Conversation

@AlexanderVasiljev
Copy link
Contributor

@AlexanderVasiljev AlexanderVasiljev commented Oct 19, 2020

Summary

Break from read function after closing serial device.
A serial device hangs on read and doesn't quit read function after closing. It blocks on semaphore.

Impact

Serial device driver.

Testing

Open non-console serial device. Read from it in one thread and close it in another thread. With this patch read function should quit.

xiaoxiang781216
xiaoxiang781216 approved these changes Oct 19, 2020 •
edited
Loading
View reviewed changes
xiaoxiang781216
xiaoxiang781216 requested changes Oct 19, 2020
View reviewed changes
xiaoxiang781216
xiaoxiang781216 approved these changes Oct 20, 2020
View reviewed changes
Copy link
Contributor

@xiaoxiang781216 xiaoxiang781216 left a comment

Choose a reason for hiding this comment

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

LTGM.

@jerpelea
Copy link
Contributor

jerpelea commented Oct 20, 2020

LGTM

davids5
davids5 reviewed Oct 20, 2020
View reviewed changes
drivers/serial/serial.c Outdated

/* Wake up reading function if it is blocked */

if (dev->recvwaiting)
Copy link
Contributor

@davids5 davids5 Oct 20, 2020

Choose a reason for hiding this comment

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

What about the recvwaiting life cycle (being reset) and any pollwaiters? What releases them? The LL driver would call "uart_datareceived"

Copy link
Contributor Author

@AlexanderVasiljev AlexanderVasiljev Oct 20, 2020

Choose a reason for hiding this comment

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

I was sure polls are canceled at higher level of nx_close. But they are not.

We can add
uart_pollnotify(dev, POLLIN);
to the close function.

This is not the logic any low level driver should care of.

Copy link
Contributor

@davids5 davids5 Oct 20, 2020 •
edited
Loading

Choose a reason for hiding this comment

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

Can you just use uart_datareceived() to release the waiters and the rx thread and manage the recvwaiting state?

Copy link
Contributor Author

@AlexanderVasiljev AlexanderVasiljev Oct 20, 2020

Choose a reason for hiding this comment

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

OK!

@davids5
Copy link
Contributor

davids5 commented Oct 20, 2020

@AlexanderVasiljev - Great, I glad that works. Can you please squash this and force push it?

davids5
davids5 reviewed Oct 21, 2020
View reviewed changes
Copy link
Contributor

@davids5 davids5 left a comment

Choose a reason for hiding this comment

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

@AlexanderVasiljev - thank you.

Are you satisfied with your testing, that this solves the problem and does not introduce any others?

@AlexanderVasiljev
Copy link
Contributor Author

AlexanderVasiljev commented Oct 21, 2020

@AlexanderVasiljev - thank you.

Are you satisfied with your testing, that this solves the problem and does not introduce any others?

Yes, it works as expected. I didn't notice any problems.

@davids5 davids5 merged commit d523757 into apache:master Oct 21, 2020
extinguish pushed a commit to extinguish/nuttx that referenced this pull request Aug 6, 2024
@fxysunshine @extinguish
toolchain/ghs: fix the inline assembly code register alloc compile error
1105ac1 
The following are the compile error that reported
by GreenHills compiler:

"/mnt/yang/qixinwei_commit/nuttx/include/arch/syscall.h", line 156 (col. 17): error apache#2036-D:
          cannot allocate "reg0" to specified caller-saved register

 "/mnt/yang/qixinwei_commit/nuttx/include/arch/syscall.h", line 157 (col. 17): error apache#2036-D:
          cannot allocate "reg1" to specified caller-saved register

Then we fix this greenhills compilation error by explicitly specifying
the registers in the clobber list in the inline assembly code.
This fix is successful in compiling on the
nuttx/boards/arm/mps/mps2-an500/configs/nsh platform and passes
the ostest test.
However, if we keep the implementation the same for both the default
and Greenhills compilers, the default compiler will report the
following two issues:
1. the "sys_call6" function will report compile error when
compiling on "./vendor/qemu/boards/smartspeaker/configs/smartspeaker-knsh"
platform, the detailed error info:

CC:  proxies/PROXY_mq_getattr.c In file included from /home/guoshichao/work_profile/vela_os/vela_qemu_1/nuttx/include/sys/syscall.h:35,
                 from /home/guoshichao/work_profile/vela_os/vela_qemu_1/nuttx/include/syscall.h:30,
                 from proxies/PROXY_mmap.c:5:
In function 'sys_call6',
    inlined from 'mmap' at proxies/PROXY_mmap.c:9:22:
/home/guoshichao/work_profile/vela_os/vela_qemu_1/nuttx/include/arch/syscall.h:297:3: error: 'asm' operand has impossible constraints
  297 |   __asm__ __volatile__
      |   ^~~~~~~

2. when running on qemu-armv7-a platform, the modification to
"smh_call()" function will make the system fail to boot up, so
we need to keep the default compiler implementation and greenhills
compiler implementation separate

Signed-off-by: fangxinyong <fangxinyong@xiaomi.com>
extinguish pushed a commit to extinguish/nuttx that referenced this pull request Aug 6, 2024
@fxysunshine @extinguish
toolchain/ghs: fix the inline assembly code register alloc compile error
1fbc9d3 
The following are the compile error that reported
by GreenHills compiler:

"/mnt/yang/qixinwei_commit/nuttx/include/arch/syscall.h", line 156 (col. 17): error apache#2036-D:
          cannot allocate "reg0" to specified caller-saved register

 "/mnt/yang/qixinwei_commit/nuttx/include/arch/syscall.h", line 157 (col. 17): error apache#2036-D:
          cannot allocate "reg1" to specified caller-saved register

Then we fix this greenhills compilation error by explicitly specifying
the registers in the clobber list in the inline assembly code.
This fix is successful in compiling on the
nuttx/boards/arm/mps/mps2-an500/configs/nsh platform and passes
the ostest test.
However, if we keep the implementation the same for both the default
and Greenhills compilers, the default compiler will report the
following two issues:
1. the "sys_call6" function will report compile error when
compiling on "./vendor/qemu/boards/smartspeaker/configs/smartspeaker-knsh"
platform, the detailed error info:

CC:  proxies/PROXY_mq_getattr.c In file included from /home/guoshichao/work_profile/vela_os/vela_qemu_1/nuttx/include/sys/syscall.h:35,
                 from /home/guoshichao/work_profile/vela_os/vela_qemu_1/nuttx/include/syscall.h:30,
                 from proxies/PROXY_mmap.c:5:
In function 'sys_call6',
    inlined from 'mmap' at proxies/PROXY_mmap.c:9:22:
/home/guoshichao/work_profile/vela_os/vela_qemu_1/nuttx/include/arch/syscall.h:297:3: error: 'asm' operand has impossible constraints
  297 |   __asm__ __volatile__
      |   ^~~~~~~

2. when running on qemu-armv7-a platform, the modification to
"smh_call()" function will make the system fail to boot up, so
we need to keep the default compiler implementation and greenhills
compiler implementation separate

Signed-off-by: fangxinyong <fangxinyong@xiaomi.com>
extinguish pushed a commit to extinguish/nuttx that referenced this pull request Aug 10, 2024
@fxysunshine @extinguish
toolchain/ghs: fix the inline assembly code register alloc compile error
15315cd 
The following are the compile error that reported
by GreenHills compiler:

"/mnt/yang/qixinwei_commit/nuttx/include/arch/syscall.h", line 156 (col. 17): error apache#2036-D:
          cannot allocate "reg0" to specified caller-saved register

 "/mnt/yang/qixinwei_commit/nuttx/include/arch/syscall.h", line 157 (col. 17): error apache#2036-D:
          cannot allocate "reg1" to specified caller-saved register

Then we fix this greenhills compilation error by explicitly specifying
the registers in the clobber list in the inline assembly code.
This fix is successful in compiling on the
nuttx/boards/arm/mps/mps2-an500/configs/nsh platform and passes
the ostest test.
However, if we keep the implementation the same for both the default
and Greenhills compilers, the default compiler will report the
following two issues:
1. the "sys_call6" function will report compile error when
compiling on "./vendor/qemu/boards/smartspeaker/configs/smartspeaker-knsh"
platform, the detailed error info:

CC:  proxies/PROXY_mq_getattr.c In file included from /home/guoshichao/work_profile/vela_os/vela_qemu_1/nuttx/include/sys/syscall.h:35,
                 from /home/guoshichao/work_profile/vela_os/vela_qemu_1/nuttx/include/syscall.h:30,
                 from proxies/PROXY_mmap.c:5:
In function 'sys_call6',
    inlined from 'mmap' at proxies/PROXY_mmap.c:9:22:
/home/guoshichao/work_profile/vela_os/vela_qemu_1/nuttx/include/arch/syscall.h:297:3: error: 'asm' operand has impossible constraints
  297 |   __asm__ __volatile__
      |   ^~~~~~~

2. when running on qemu-armv7-a platform, the modification to
"smh_call()" function will make the system fail to boot up, so
we need to keep the default compiler implementation and greenhills
compiler implementation separate

Signed-off-by: fangxinyong <fangxinyong@xiaomi.com>
(cherry picked from commit cb48b74)
extinguish pushed a commit to extinguish/nuttx that referenced this pull request Aug 11, 2024
@fxysunshine @extinguish
toolchain/ghs: fix the inline assembly code register alloc compile error
b19042d 
The following are the compile error that reported
by GreenHills compiler:

"/mnt/yang/qixinwei_commit/nuttx/include/arch/syscall.h", line 156 (col. 17): error apache#2036-D:
          cannot allocate "reg0" to specified caller-saved register

 "/mnt/yang/qixinwei_commit/nuttx/include/arch/syscall.h", line 157 (col. 17): error apache#2036-D:
          cannot allocate "reg1" to specified caller-saved register

Then we fix this greenhills compilation error by explicitly specifying
the registers in the clobber list in the inline assembly code.
This fix is successful in compiling on the
nuttx/boards/arm/mps/mps2-an500/configs/nsh platform and passes
the ostest test.
However, if we keep the implementation the same for both the default
and Greenhills compilers, the default compiler will report the
following two issues:
1. the "sys_call6" function will report compile error when
compiling on "./vendor/qemu/boards/smartspeaker/configs/smartspeaker-knsh"
platform, the detailed error info:

CC:  proxies/PROXY_mq_getattr.c In file included from /home/guoshichao/work_profile/vela_os/vela_qemu_1/nuttx/include/sys/syscall.h:35,
                 from /home/guoshichao/work_profile/vela_os/vela_qemu_1/nuttx/include/syscall.h:30,
                 from proxies/PROXY_mmap.c:5:
In function 'sys_call6',
    inlined from 'mmap' at proxies/PROXY_mmap.c:9:22:
/home/guoshichao/work_profile/vela_os/vela_qemu_1/nuttx/include/arch/syscall.h:297:3: error: 'asm' operand has impossible constraints
  297 |   __asm__ __volatile__
      |   ^~~~~~~

2. when running on qemu-armv7-a platform, the modification to
"smh_call()" function will make the system fail to boot up, so
we need to keep the default compiler implementation and greenhills
compiler implementation separate

Signed-off-by: fangxinyong <fangxinyong@xiaomi.com>
(cherry picked from commit cb48b74)
acassis pushed a commit that referenced this pull request Aug 11, 2024
@fxysunshine
toolchain/ghs: fix the inline assembly code register alloc compile error
6da5257 
The following are the compile error that reported
by GreenHills compiler:

"/mnt/yang/qixinwei_commit/nuttx/include/arch/syscall.h", line 156 (col. 17): error #2036-D:
          cannot allocate "reg0" to specified caller-saved register

 "/mnt/yang/qixinwei_commit/nuttx/include/arch/syscall.h", line 157 (col. 17): error #2036-D:
          cannot allocate "reg1" to specified caller-saved register

Then we fix this greenhills compilation error by explicitly specifying
the registers in the clobber list in the inline assembly code.
This fix is successful in compiling on the
nuttx/boards/arm/mps/mps2-an500/configs/nsh platform and passes
the ostest test.
However, if we keep the implementation the same for both the default
and Greenhills compilers, the default compiler will report the
following two issues:
1. the "sys_call6" function will report compile error when
compiling on "./vendor/qemu/boards/smartspeaker/configs/smartspeaker-knsh"
platform, the detailed error info:

CC:  proxies/PROXY_mq_getattr.c In file included from /home/guoshichao/work_profile/vela_os/vela_qemu_1/nuttx/include/sys/syscall.h:35,
                 from /home/guoshichao/work_profile/vela_os/vela_qemu_1/nuttx/include/syscall.h:30,
                 from proxies/PROXY_mmap.c:5:
In function 'sys_call6',
    inlined from 'mmap' at proxies/PROXY_mmap.c:9:22:
/home/guoshichao/work_profile/vela_os/vela_qemu_1/nuttx/include/arch/syscall.h:297:3: error: 'asm' operand has impossible constraints
  297 |   __asm__ __volatile__
      |   ^~~~~~~

2. when running on qemu-armv7-a platform, the modification to
"smh_call()" function will make the system fail to boot up, so
we need to keep the default compiler implementation and greenhills
compiler implementation separate

Signed-off-by: fangxinyong <fangxinyong@xiaomi.com>
(cherry picked from commit cb48b74)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@davids5 davids5 davids5 left review comments

@xiaoxiang781216 xiaoxiang781216 Awaiting requested review from xiaoxiang781216 xiaoxiang781216 is a code owner

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

4 participants

@AlexanderVasiljev @jerpelea @davids5 @xiaoxiang781216