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

uart: Superfluous syscalls for driver API #10911

Closed
pfalcon opened this issue Oct 29, 2018 · 4 comments
Closed

uart: Superfluous syscalls for driver API #10911

pfalcon opened this issue Oct 29, 2018 · 4 comments
Labels
area: UART Universal Asynchronous Receiver-Transmitter area: Userspace Userspace priority: low Low impact/importance bug

Comments

@pfalcon
Copy link
Contributor

pfalcon commented Oct 29, 2018

Split from #10761 (comment)

"Even more surprising was that bunch of IRQ-related functionality of UART API was made userspace syscalls. @andrewboie, how did we get to that? Even with reading the docstring, it's pretty clear that uart_irq_tx_enable() is IRQ related. What's the point of exporting it userspace, like, user calls uart_irq_tx_enable(), what happens next? It's not possible to define ISR callback from userspace anyway. And most of the UART API is strictly related to interrupt-driven mode. Only uart_poll_in()/uart_poll_out() could be exposed to userspace (and perhaps err_check()). They're also not too useful, but why not? IMHO, we should fix that ASAP."

@pfalcon pfalcon added the area: UART Universal Asynchronous Receiver-Transmitter label Oct 29, 2018
@pfalcon
Copy link
Contributor Author

pfalcon commented Oct 29, 2018

@nashif : I don't see a good label for userspace-vs-kernel split and/or syscalls. I suggest to create one, as many already introduced syscalls are questionable, so might be a regular topic.

@nashif nashif added area: Userspace Userspace bug The issue is a bug, or the PR is fixing a bug labels Oct 29, 2018
@galak galak added the priority: medium Medium impact/importance bug label Nov 20, 2018
@andrewboie
Copy link
Contributor

andrewboie commented Nov 28, 2018

What's the point of exporting it userspace, like, user calls uart_irq_tx_enable(), what happens next? It's not possible to define ISR callback from userspace anyway.

The thought process for the current state of the code was that the interrupt callback could be setup at application initialization, but once everything is set up and the app drops most of its threads to user mode, enabling/disabling the interrupt could still be controlled from a user mode thread granted access to that uart device.

If this scenario would never be useful, I have no objections to removing these syscalls.

@pfalcon
Copy link
Contributor Author

pfalcon commented Nov 29, 2018

The thought process for the current state of the code was that the interrupt callback could be setup at application initialization, but once everything is set up and the app drops most of its threads to user mode, enabling/disabling the interrupt could still be controlled from a user mode thread granted access to that uart device.

That's exactly the kind of feedback I'm looking for, thanks! And thanks for expanding beyond the original "If you don't think those calls should be there, remove them.". So, I definitely have a stereotype thinking that usermode would work as in Linux, where an app starts in user mode right away. This is both inflexible thinking and pretty far from Zephyr model, we "applications" don't "start", but rather a single one runs on boot.

I'm not closing this right away, let me think on this until release times. Even if there's a theoretic usecase, we may want to account of API status ("legacy" and being replaced by others) and pragmatic matters like keeping syscall set bounded.

@pfalcon pfalcon added priority: low Low impact/importance bug and removed priority: medium Medium impact/importance bug labels Nov 29, 2018
@andrewboie andrewboie removed the bug The issue is a bug, or the PR is fixing a bug label Nov 29, 2018
@galak galak added the question label Dec 4, 2018
@pfalcon
Copy link
Contributor Author

pfalcon commented Dec 7, 2018

I guess this should be closed after all. I definitely not up to speed with complete understanding of userspace and its APIs, and even if I'd think I am, I'd be reluctant to change something a more experience colleague set forth, given the good explanations given.

Thanks for guidance @andrewboie here and elsewhere.

@pfalcon pfalcon closed this as completed Dec 7, 2018
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 area: Userspace Userspace priority: low Low impact/importance bug
Projects
None yet
Development

No branches or pull requests

4 participants