Hi,
this series tries to address an issue that came up in Raspbian's kernel
tree [1] and upstream distros [2][3].
We adopted some changes that moved wait calls from a custom
implementation to the more standard killable family of functions. Users
complained that all the VCHIQ threads showed up in D state (which is the
expected behaviour).
The custom implementation we deleted tried to mimic the killable family
of functions, yet accepted more signals than the later; SIGKILL |
SIGINT | SIGQUIT | SIGTRAP | SIGSTOP | SIGCONT for the custom
implementation as opposed to plain old SIGKILL.
Raspbian maintainers decided roll back some of those changes and leave
the wait functions as interruptible. Hence creating some divergence
between both trees.
One could argue that not liking having the threads stuck in D state is
not really a software issue. It's more a cosmetic thing that can scare
people when they look at "uptime". On the other hand, if we are ever to
unstage this driver, we'd really need a proper justification for using
the killable family of functions. Which I think it's not really clear at
the moment.
As Raspbian's kernel has been working for a while with interruptible
waits I propose we follow through. If needed we can always go back to
killable. But at least we'll have a proper understanding on the actual
needs. In the end the driver is in staging, and the potential for errors
small.
The first 3 commits fix the issue, and should probably get in as soon as
possible, the last commit is just cosmetic and can wait until 5.3.
Regards,
Nicolas
[1] https://github.com/raspberrypi/linux/issues/2881
[2] https://archlinuxarm.org/forum/viewtopic.php?f=65&t=13485
[3] https://lists.fedoraproject.org/archives/list/[email protected]/message/GBXGJ7DOV5CQQXFPOZCXTRD6W4BEPT4Q/
--
Changes since v2:
- Cleaned up revert commit message
- Rebase & merge conflict resolutions
- Add code cleanup suggested by Dan Carpenter
Changes since v1:
- Proplery format revert commits
- Add code comment to remind of this issue
- Add Fixes tags
Nicolas Saenz Julienne (4):
staging: vchiq_2835_arm: revert "quit using custom
down_interruptible()"
staging: vchiq: revert "switch to wait_for_completion_killable"
staging: vchiq: make wait events interruptible
staging: vchiq: stop explicitly comparing with zero to catch errors
.../bcm2835-camera/bcm2835-camera.c | 11 ++-
.../interface/vchiq_arm/vchiq_2835_arm.c | 2 +-
.../interface/vchiq_arm/vchiq_arm.c | 85 +++++++++----------
.../interface/vchiq_arm/vchiq_connected.c | 4 +-
.../interface/vchiq_arm/vchiq_core.c | 53 +++++++-----
.../interface/vchiq_arm/vchiq_debugfs.c | 4 +-
.../interface/vchiq_arm/vchiq_util.c | 6 +-
7 files changed, 82 insertions(+), 83 deletions(-)
--
2.21.0
The killable version of down() is meant to be used on situations where
it should not fail at all costs, but still have the convenience of being
able to kill it if really necessary. VCHIQ doesn't fit this criteria, as
it's mainly used as an interface to V4L2 and ALSA devices.
Fixes: ff5979ad8636 ("staging: vchiq_2835_arm: quit using custom down_interruptible()")
Signed-off-by: Nicolas Saenz Julienne <[email protected]>
---
.../staging/vc04_services/interface/vchiq_arm/vchiq_2835_arm.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/drivers/staging/vc04_services/interface/vchiq_arm/vchiq_2835_arm.c b/drivers/staging/vc04_services/interface/vchiq_arm/vchiq_2835_arm.c
index a9a22917ecdb..49d3b39b1059 100644
--- a/drivers/staging/vc04_services/interface/vchiq_arm/vchiq_2835_arm.c
+++ b/drivers/staging/vc04_services/interface/vchiq_arm/vchiq_2835_arm.c
@@ -514,7 +514,7 @@ create_pagelist(char __user *buf, size_t count, unsigned short type)
(g_cache_line_size - 1)))) {
char *fragments;
- if (down_killable(&g_free_fragments_sema)) {
+ if (down_interruptible(&g_free_fragments_sema) != 0) {
cleanup_pagelistinfo(pagelistinfo);
return NULL;
}
--
2.21.0
On 09.05.19 16:31, Nicolas Saenz Julienne wrote:
> The killable version of down() is meant to be used on situations where
> it should not fail at all costs, but still have the convenience of being
> able to kill it if really necessary. VCHIQ doesn't fit this criteria, as
> it's mainly used as an interface to V4L2 and ALSA devices.
>
> Fixes: ff5979ad8636 ("staging: vchiq_2835_arm: quit using custom down_interruptible()")
> Signed-off-by: Nicolas Saenz Julienne <[email protected]>
This whole series is:
Acked-by: Stefan Wahren <[email protected]>