* [PATCH v2 0/5] staging: vc04_services: vchiq_core: Stop kthreads on shutdown
@ 2024-03-21 13:07 Umang Jain
2024-03-21 13:07 ` [PATCH v2 1/5] staging: vc04_services: Remove unused function declarations Umang Jain
` (4 more replies)
0 siblings, 5 replies; 18+ messages in thread
From: Umang Jain @ 2024-03-21 13:07 UTC (permalink / raw
To: linux-staging
Cc: Stefan Wahren, Dan Carpenter, Kieran Bingham, Laurent Pinchart,
Dave Stevenson, Phil Elwell, Greg KH, Umang Jain
The series addresses the following TODO item:
```
* Fix kernel module support
Even the VPU firmware doesn't support a VCHI re-connect, the driver
should properly handle a module unload. This also includes that all
resources must be freed (kthreads, debugfs entries, ...) and global
variables avoided.
```
Patch 1/5 to 3/5 are log cleanups spotted during the reading of the
driver.
Patch 4/5 implements .remove() function vptr so that individual
devices(bcm2835-audio, bcm2835-camera, bcm2835-isp etc.)
can run their cleanup when removed from the vchiq_bus, during their
own module unload.
Patch 5/5 stops the kthreads started by vchiq - on shutdown path.
Rest of the module cleanup (debugfs entries, deregister char device
etc.) is already done as part of vchiq_remove().
Testing on RPi4 for vchiq module unload:
```
uajain@ATX:~$ uname -r
6.8.0-rc1-00128-gab2b09f632fa-dirty
uajain@ATX:~$ dmesg | grep vchiq
[ 21.401426] vchiq: module is from the staging directory, the quality is unknown, you have been warned.
uajain@ATX:~$ sudo modprobe bcm2835_mmal_vchiq
sudo: unable to resolve host ATX: Temporary failure in name resolution
uajain@ATX:~$ dmesg | grep vchiq
[ 21.401426] vchiq: module is from the staging directory, the quality is unknown, you have been warned.
[ 96.388148] bcm2835_mmal_vchiq: module is from the staging directory, the quality is unknown, you have been warned.
uajain@ATX:~$ lsmod | grep vchiq
bcm2835_mmal_vchiq 40960 0
vchiq 581632 1 bcm2835_mmal_vchiq
uajain@ATX:~$ sudo rmmod bcm2835_mmal_vchiq vchiq
sudo: unable to resolve host ATX: Temporary failure in name resolution
uajain@ATX:~$ lsmod | grep vchiq
uajain@ATX:~$ dmesg | grep vchiq
[ 21.401426] vchiq: module is from the staging directory, the quality is unknown, you have been warned.
[ 96.388148] bcm2835_mmal_vchiq: module is from the staging directory, the quality is unknown, you have been warned.
uajain@ATX:~$
```
changes in v2:
- Drop one of WARN_ON from patch 2/5 (will be handled separately)
- Remove curly braces for single statement in 3/5
- null-check driver->remove in 4/5
Umang Jain (5):
staging: vc04_services: Remove unused function declarations
staging: vc04_services: vchiq_arm: Use appropriate dev_* log helpers
staging: vc04_services: Do not log error on kzalloc()
staging: vc04_services: Implement vchiq_bus .remove
staging: vc04_services: vchiq_core: Stop kthreads on shutdown
drivers/staging/vc04_services/interface/TODO | 7 -------
.../interface/vchiq_arm/vchiq_arm.c | 19 ++++++++++---------
.../interface/vchiq_arm/vchiq_bus.c | 10 ++++++++++
.../interface/vchiq_arm/vchiq_core.c | 10 +++++++---
.../interface/vchiq_arm/vchiq_core.h | 6 ------
5 files changed, 27 insertions(+), 25 deletions(-)
base-commit: 68bb540b1aefded1d58a9f956568d5316643d291
--
2.43.0
^ permalink raw reply [flat|nested] 18+ messages in thread
* [PATCH v2 1/5] staging: vc04_services: Remove unused function declarations
2024-03-21 13:07 [PATCH v2 0/5] staging: vc04_services: vchiq_core: Stop kthreads on shutdown Umang Jain
@ 2024-03-21 13:07 ` Umang Jain
2024-03-21 13:07 ` [PATCH v2 2/5] staging: vc04_services: vchiq_arm: Use appropriate dev_* log helpers Umang Jain
` (3 subsequent siblings)
4 siblings, 0 replies; 18+ messages in thread
From: Umang Jain @ 2024-03-21 13:07 UTC (permalink / raw
To: linux-staging
Cc: Stefan Wahren, Dan Carpenter, Kieran Bingham, Laurent Pinchart,
Dave Stevenson, Phil Elwell, Greg KH, Umang Jain
vchiq_loud_error-* are not implemented hence, remove their
declarations. This seem to be remnants of custom logging helpers
which were removed earlier.
Signed-off-by: Umang Jain <umang.jain@ideasonboard.com>
Reviewed-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
Reviewed-by: Kieran Bingham <kieran.bingham@ideasonboard.com>
---
.../staging/vc04_services/interface/vchiq_arm/vchiq_core.h | 6 ------
1 file changed, 6 deletions(-)
diff --git a/drivers/staging/vc04_services/interface/vchiq_arm/vchiq_core.h b/drivers/staging/vc04_services/interface/vchiq_arm/vchiq_core.h
index c8527551b58c..5fbf173d9c56 100644
--- a/drivers/staging/vc04_services/interface/vchiq_arm/vchiq_core.h
+++ b/drivers/staging/vc04_services/interface/vchiq_arm/vchiq_core.h
@@ -470,12 +470,6 @@ vchiq_bulk_transfer(struct vchiq_instance *instance, unsigned int handle, void *
extern void
vchiq_dump_state(struct seq_file *f, struct vchiq_state *state);
-extern void
-vchiq_loud_error_header(void);
-
-extern void
-vchiq_loud_error_footer(void);
-
extern void
request_poll(struct vchiq_state *state, struct vchiq_service *service,
int poll_type);
--
2.43.0
^ permalink raw reply related [flat|nested] 18+ messages in thread
* [PATCH v2 2/5] staging: vc04_services: vchiq_arm: Use appropriate dev_* log helpers
2024-03-21 13:07 [PATCH v2 0/5] staging: vc04_services: vchiq_core: Stop kthreads on shutdown Umang Jain
2024-03-21 13:07 ` [PATCH v2 1/5] staging: vc04_services: Remove unused function declarations Umang Jain
@ 2024-03-21 13:07 ` Umang Jain
2024-03-21 13:07 ` [PATCH v2 3/5] staging: vc04_services: Do not log error on kzalloc() Umang Jain
` (2 subsequent siblings)
4 siblings, 0 replies; 18+ messages in thread
From: Umang Jain @ 2024-03-21 13:07 UTC (permalink / raw
To: linux-staging
Cc: Stefan Wahren, Dan Carpenter, Kieran Bingham, Laurent Pinchart,
Dave Stevenson, Phil Elwell, Greg KH, Umang Jain
Re-evaluate logs on error code paths and fix a few error logs
with appropriate dev_* logging helpers.
No functional changes intended in this patch.
Signed-off-by: Umang Jain <umang.jain@ideasonboard.com>
Reviewed-by: Kieran Bingham <kieran.bingham@ideasonboard.com>
---
.../staging/vc04_services/interface/vchiq_arm/vchiq_arm.c | 6 +++---
1 file changed, 3 insertions(+), 3 deletions(-)
diff --git a/drivers/staging/vc04_services/interface/vchiq_arm/vchiq_arm.c b/drivers/staging/vc04_services/interface/vchiq_arm/vchiq_arm.c
index 1579bd4e5263..b3021739b170 100644
--- a/drivers/staging/vc04_services/interface/vchiq_arm/vchiq_arm.c
+++ b/drivers/staging/vc04_services/interface/vchiq_arm/vchiq_arm.c
@@ -1317,7 +1317,7 @@ vchiq_keepalive_thread_func(void *v)
long rc = 0, uc = 0;
if (wait_for_completion_interruptible(&arm_state->ka_evt)) {
- dev_err(state->dev, "suspend: %s: interrupted\n", __func__);
+ dev_dbg(state->dev, "suspend: %s: interrupted\n", __func__);
flush_signals(current);
continue;
}
@@ -1753,7 +1753,7 @@ static int vchiq_probe(struct platform_device *pdev)
*/
err = vchiq_register_chrdev(&pdev->dev);
if (err) {
- dev_warn(&pdev->dev, "arm: Failed to initialize vchiq cdev\n");
+ dev_err(&pdev->dev, "arm: Failed to initialize vchiq cdev\n");
goto error_exit;
}
@@ -1763,7 +1763,7 @@ static int vchiq_probe(struct platform_device *pdev)
return 0;
failed_platform_init:
- dev_warn(&pdev->dev, "arm: Could not initialize vchiq platform\n");
+ dev_err(&pdev->dev, "arm: Could not initialize vchiq platform\n");
error_exit:
return err;
}
--
2.43.0
^ permalink raw reply related [flat|nested] 18+ messages in thread
* [PATCH v2 3/5] staging: vc04_services: Do not log error on kzalloc()
2024-03-21 13:07 [PATCH v2 0/5] staging: vc04_services: vchiq_core: Stop kthreads on shutdown Umang Jain
2024-03-21 13:07 ` [PATCH v2 1/5] staging: vc04_services: Remove unused function declarations Umang Jain
2024-03-21 13:07 ` [PATCH v2 2/5] staging: vc04_services: vchiq_arm: Use appropriate dev_* log helpers Umang Jain
@ 2024-03-21 13:07 ` Umang Jain
2024-03-21 17:41 ` Kieran Bingham
2024-03-21 13:07 ` [PATCH v2 4/5] staging: vc04_services: Implement vchiq_bus .remove Umang Jain
2024-03-21 13:07 ` [PATCH v2 5/5] staging: vc04_services: vchiq_core: Stop kthreads on shutdown Umang Jain
4 siblings, 1 reply; 18+ messages in thread
From: Umang Jain @ 2024-03-21 13:07 UTC (permalink / raw
To: linux-staging
Cc: Stefan Wahren, Dan Carpenter, Kieran Bingham, Laurent Pinchart,
Dave Stevenson, Phil Elwell, Greg KH, Umang Jain
Do not log any error for kzalloc() error path. kzalloc() already reports
such errors.
Signed-off-by: Umang Jain <umang.jain@ideasonboard.com>
---
.../staging/vc04_services/interface/vchiq_arm/vchiq_arm.c | 5 +----
1 file changed, 1 insertion(+), 4 deletions(-)
diff --git a/drivers/staging/vc04_services/interface/vchiq_arm/vchiq_arm.c b/drivers/staging/vc04_services/interface/vchiq_arm/vchiq_arm.c
index b3021739b170..ad506016fc93 100644
--- a/drivers/staging/vc04_services/interface/vchiq_arm/vchiq_arm.c
+++ b/drivers/staging/vc04_services/interface/vchiq_arm/vchiq_arm.c
@@ -690,7 +690,6 @@ int vchiq_initialise(struct vchiq_instance **instance_out)
instance = kzalloc(sizeof(*instance), GFP_KERNEL);
if (!instance) {
- dev_err(state->dev, "core: %s: Cannot allocate vchiq instance\n", __func__);
ret = -ENOMEM;
goto failed;
}
@@ -956,10 +955,8 @@ vchiq_blocking_bulk_transfer(struct vchiq_instance *instance, unsigned int handl
}
} else {
waiter = kzalloc(sizeof(*waiter), GFP_KERNEL);
- if (!waiter) {
- dev_err(service->state->dev, "core: %s: - Out of memory\n", __func__);
+ if (!waiter)
return -ENOMEM;
- }
}
status = vchiq_bulk_transfer(instance, handle, data, NULL, size,
--
2.43.0
^ permalink raw reply related [flat|nested] 18+ messages in thread
* [PATCH v2 4/5] staging: vc04_services: Implement vchiq_bus .remove
2024-03-21 13:07 [PATCH v2 0/5] staging: vc04_services: vchiq_core: Stop kthreads on shutdown Umang Jain
` (2 preceding siblings ...)
2024-03-21 13:07 ` [PATCH v2 3/5] staging: vc04_services: Do not log error on kzalloc() Umang Jain
@ 2024-03-21 13:07 ` Umang Jain
2024-03-21 17:18 ` Kieran Bingham
2024-03-21 13:07 ` [PATCH v2 5/5] staging: vc04_services: vchiq_core: Stop kthreads on shutdown Umang Jain
4 siblings, 1 reply; 18+ messages in thread
From: Umang Jain @ 2024-03-21 13:07 UTC (permalink / raw
To: linux-staging
Cc: Stefan Wahren, Dan Carpenter, Kieran Bingham, Laurent Pinchart,
Dave Stevenson, Phil Elwell, Greg KH, Umang Jain
Implement the struct vchiq_bus .remove() so that cleanup
paths can be executed by the devices registered to this
bus, when being removed.
Signed-off-by: Umang Jain <umang.jain@ideasonboard.com>
---
.../vc04_services/interface/vchiq_arm/vchiq_bus.c | 10 ++++++++++
1 file changed, 10 insertions(+)
diff --git a/drivers/staging/vc04_services/interface/vchiq_arm/vchiq_bus.c b/drivers/staging/vc04_services/interface/vchiq_arm/vchiq_bus.c
index 68f830d75531..93609716df84 100644
--- a/drivers/staging/vc04_services/interface/vchiq_arm/vchiq_bus.c
+++ b/drivers/staging/vc04_services/interface/vchiq_arm/vchiq_bus.c
@@ -37,11 +37,21 @@ static int vchiq_bus_probe(struct device *dev)
return driver->probe(device);
}
+static void vchiq_bus_remove(struct device *dev)
+{
+ struct vchiq_device *device = to_vchiq_device(dev);
+ struct vchiq_driver *driver = to_vchiq_driver(dev->driver);
+
+ if (driver->remove)
+ driver->remove(device);
+}
+
const struct bus_type vchiq_bus_type = {
.name = "vchiq-bus",
.match = vchiq_bus_type_match,
.uevent = vchiq_bus_uevent,
.probe = vchiq_bus_probe,
+ .remove = vchiq_bus_remove,
};
static void vchiq_device_release(struct device *dev)
--
2.43.0
^ permalink raw reply related [flat|nested] 18+ messages in thread
* [PATCH v2 5/5] staging: vc04_services: vchiq_core: Stop kthreads on shutdown
2024-03-21 13:07 [PATCH v2 0/5] staging: vc04_services: vchiq_core: Stop kthreads on shutdown Umang Jain
` (3 preceding siblings ...)
2024-03-21 13:07 ` [PATCH v2 4/5] staging: vc04_services: Implement vchiq_bus .remove Umang Jain
@ 2024-03-21 13:07 ` Umang Jain
2024-03-26 21:22 ` Stefan Wahren
` (2 more replies)
4 siblings, 3 replies; 18+ messages in thread
From: Umang Jain @ 2024-03-21 13:07 UTC (permalink / raw
To: linux-staging
Cc: Stefan Wahren, Dan Carpenter, Kieran Bingham, Laurent Pinchart,
Dave Stevenson, Phil Elwell, Greg KH, Umang Jain
The various kthreads thread functions (slot_handler_func, sync_func,
recycle_func) in vchiq_core and vchiq_keepalive_thread_func in
vchiq_arm should be stopped on vchiq_shutdown().
This also address the following TODO item:
* Fix kernel module support
hence drop it from the TODO item list.
Signed-off-by: Umang Jain <umang.jain@ideasonboard.com>
Reviewed-by: Kieran Bingham <kieran.bingham@ideasonboard.com>
---
drivers/staging/vc04_services/interface/TODO | 7 -------
.../vc04_services/interface/vchiq_arm/vchiq_arm.c | 8 ++++++--
.../vc04_services/interface/vchiq_arm/vchiq_core.c | 10 +++++++---
3 files changed, 13 insertions(+), 12 deletions(-)
diff --git a/drivers/staging/vc04_services/interface/TODO b/drivers/staging/vc04_services/interface/TODO
index 05eb5140d096..57a2d6761f9b 100644
--- a/drivers/staging/vc04_services/interface/TODO
+++ b/drivers/staging/vc04_services/interface/TODO
@@ -16,13 +16,6 @@ some of the ones we want:
to manage these buffers as dmabufs so that we can zero-copy import
camera images into vc4 for rendering/display.
-* Fix kernel module support
-
-Even the VPU firmware doesn't support a VCHI re-connect, the driver
-should properly handle a module unload. This also includes that all
-resources must be freed (kthreads, debugfs entries, ...) and global
-variables avoided.
-
* Documentation
A short top-down description of this driver's architecture (function of
diff --git a/drivers/staging/vc04_services/interface/vchiq_arm/vchiq_arm.c b/drivers/staging/vc04_services/interface/vchiq_arm/vchiq_arm.c
index ad506016fc93..1d21f9cfbfe5 100644
--- a/drivers/staging/vc04_services/interface/vchiq_arm/vchiq_arm.c
+++ b/drivers/staging/vc04_services/interface/vchiq_arm/vchiq_arm.c
@@ -726,8 +726,9 @@ void free_bulk_waiter(struct vchiq_instance *instance)
int vchiq_shutdown(struct vchiq_instance *instance)
{
- int status = 0;
struct vchiq_state *state = instance->state;
+ struct vchiq_arm_state *arm_state;
+ int status = 0;
if (mutex_lock_killable(&state->mutex))
return -EAGAIN;
@@ -739,6 +740,9 @@ int vchiq_shutdown(struct vchiq_instance *instance)
dev_dbg(state->dev, "core: (%p): returning %d\n", instance, status);
+ arm_state = vchiq_platform_get_arm_state(state);
+ kthread_stop(arm_state->ka_thread);
+
free_bulk_waiter(instance);
kfree(instance);
@@ -1310,7 +1314,7 @@ vchiq_keepalive_thread_func(void *v)
goto shutdown;
}
- while (1) {
+ while (!kthread_should_stop()) {
long rc = 0, uc = 0;
if (wait_for_completion_interruptible(&arm_state->ka_evt)) {
diff --git a/drivers/staging/vc04_services/interface/vchiq_arm/vchiq_core.c b/drivers/staging/vc04_services/interface/vchiq_arm/vchiq_core.c
index 76c27778154a..953ccd87f425 100644
--- a/drivers/staging/vc04_services/interface/vchiq_arm/vchiq_core.c
+++ b/drivers/staging/vc04_services/interface/vchiq_arm/vchiq_core.c
@@ -1936,7 +1936,7 @@ slot_handler_func(void *v)
DEBUG_INITIALISE(local);
- while (1) {
+ while (!kthread_should_stop()) {
DEBUG_COUNT(SLOT_HANDLER_COUNT);
DEBUG_TRACE(SLOT_HANDLER_LINE);
remote_event_wait(&state->trigger_event, &local->trigger);
@@ -1978,7 +1978,7 @@ recycle_func(void *v)
if (!found)
return -ENOMEM;
- while (1) {
+ while (!kthread_should_stop()) {
remote_event_wait(&state->recycle_event, &local->recycle);
process_free_queue(state, found, length);
@@ -1997,7 +1997,7 @@ sync_func(void *v)
state->remote->slot_sync);
int svc_fourcc;
- while (1) {
+ while (!kthread_should_stop()) {
struct vchiq_service *service;
int msgid, size;
int type;
@@ -2844,6 +2844,10 @@ vchiq_shutdown_internal(struct vchiq_state *state, struct vchiq_instance *instan
(void)vchiq_remove_service(instance, service->handle);
vchiq_service_put(service);
}
+
+ kthread_stop(state->sync_thread);
+ kthread_stop(state->recycle_thread);
+ kthread_stop(state->slot_handler_thread);
}
int
--
2.43.0
^ permalink raw reply related [flat|nested] 18+ messages in thread
* Re: [PATCH v2 4/5] staging: vc04_services: Implement vchiq_bus .remove
2024-03-21 13:07 ` [PATCH v2 4/5] staging: vc04_services: Implement vchiq_bus .remove Umang Jain
@ 2024-03-21 17:18 ` Kieran Bingham
2024-03-22 4:21 ` Umang Jain
0 siblings, 1 reply; 18+ messages in thread
From: Kieran Bingham @ 2024-03-21 17:18 UTC (permalink / raw
To: Umang Jain, linux-staging
Cc: Stefan Wahren, Dan Carpenter, Laurent Pinchart, Dave Stevenson,
Phil Elwell, Greg KH, Umang Jain
Quoting Umang Jain (2024-03-21 13:07:36)
> Implement the struct vchiq_bus .remove() so that cleanup
> paths can be executed by the devices registered to this
> bus, when being removed.
>
> Signed-off-by: Umang Jain <umang.jain@ideasonboard.com>
> ---
Don't forget a changelog when possible.
v2:
- Check driver->remove before calling
Reviewed-by: Kieran Bingham <kieran.bingham@ideasonboard.com>
> .../vc04_services/interface/vchiq_arm/vchiq_bus.c | 10 ++++++++++
> 1 file changed, 10 insertions(+)
>
> diff --git a/drivers/staging/vc04_services/interface/vchiq_arm/vchiq_bus.c b/drivers/staging/vc04_services/interface/vchiq_arm/vchiq_bus.c
> index 68f830d75531..93609716df84 100644
> --- a/drivers/staging/vc04_services/interface/vchiq_arm/vchiq_bus.c
> +++ b/drivers/staging/vc04_services/interface/vchiq_arm/vchiq_bus.c
> @@ -37,11 +37,21 @@ static int vchiq_bus_probe(struct device *dev)
> return driver->probe(device);
> }
>
> +static void vchiq_bus_remove(struct device *dev)
> +{
> + struct vchiq_device *device = to_vchiq_device(dev);
> + struct vchiq_driver *driver = to_vchiq_driver(dev->driver);
> +
> + if (driver->remove)
> + driver->remove(device);
> +}
> +
> const struct bus_type vchiq_bus_type = {
> .name = "vchiq-bus",
> .match = vchiq_bus_type_match,
> .uevent = vchiq_bus_uevent,
> .probe = vchiq_bus_probe,
> + .remove = vchiq_bus_remove,
> };
>
> static void vchiq_device_release(struct device *dev)
> --
> 2.43.0
>
^ permalink raw reply [flat|nested] 18+ messages in thread
* Re: [PATCH v2 3/5] staging: vc04_services: Do not log error on kzalloc()
2024-03-21 13:07 ` [PATCH v2 3/5] staging: vc04_services: Do not log error on kzalloc() Umang Jain
@ 2024-03-21 17:41 ` Kieran Bingham
0 siblings, 0 replies; 18+ messages in thread
From: Kieran Bingham @ 2024-03-21 17:41 UTC (permalink / raw
To: Umang Jain, linux-staging
Cc: Stefan Wahren, Dan Carpenter, Laurent Pinchart, Dave Stevenson,
Phil Elwell, Greg KH, Umang Jain
Quoting Umang Jain (2024-03-21 13:07:35)
> Do not log any error for kzalloc() error path. kzalloc() already reports
> such errors.
>
> Signed-off-by: Umang Jain <umang.jain@ideasonboard.com>
Given I read lately how I don't think these can actually ever fail:
- https://lwn.net/Articles/964793/
and
- https://lwn.net/Articles/723317/
- https://lwn.net/Articles/627419
Removing these messages seems fine to me.
Reviewed-by: Kieran Bingham <kieran.bingham@ideasonboard.com>
> ---
> .../staging/vc04_services/interface/vchiq_arm/vchiq_arm.c | 5 +----
> 1 file changed, 1 insertion(+), 4 deletions(-)
>
> diff --git a/drivers/staging/vc04_services/interface/vchiq_arm/vchiq_arm.c b/drivers/staging/vc04_services/interface/vchiq_arm/vchiq_arm.c
> index b3021739b170..ad506016fc93 100644
> --- a/drivers/staging/vc04_services/interface/vchiq_arm/vchiq_arm.c
> +++ b/drivers/staging/vc04_services/interface/vchiq_arm/vchiq_arm.c
> @@ -690,7 +690,6 @@ int vchiq_initialise(struct vchiq_instance **instance_out)
>
> instance = kzalloc(sizeof(*instance), GFP_KERNEL);
> if (!instance) {
> - dev_err(state->dev, "core: %s: Cannot allocate vchiq instance\n", __func__);
> ret = -ENOMEM;
> goto failed;
> }
> @@ -956,10 +955,8 @@ vchiq_blocking_bulk_transfer(struct vchiq_instance *instance, unsigned int handl
> }
> } else {
> waiter = kzalloc(sizeof(*waiter), GFP_KERNEL);
> - if (!waiter) {
> - dev_err(service->state->dev, "core: %s: - Out of memory\n", __func__);
> + if (!waiter)
> return -ENOMEM;
> - }
> }
>
> status = vchiq_bulk_transfer(instance, handle, data, NULL, size,
> --
> 2.43.0
>
^ permalink raw reply [flat|nested] 18+ messages in thread
* Re: [PATCH v2 4/5] staging: vc04_services: Implement vchiq_bus .remove
2024-03-21 17:18 ` Kieran Bingham
@ 2024-03-22 4:21 ` Umang Jain
0 siblings, 0 replies; 18+ messages in thread
From: Umang Jain @ 2024-03-22 4:21 UTC (permalink / raw
To: Kieran Bingham, linux-staging
Cc: Stefan Wahren, Dan Carpenter, Laurent Pinchart, Dave Stevenson,
Phil Elwell, Greg KH
Hi Kieran,
On 21/03/24 10:48 pm, Kieran Bingham wrote:
> Quoting Umang Jain (2024-03-21 13:07:36)
>> Implement the struct vchiq_bus .remove() so that cleanup
>> paths can be executed by the devices registered to this
>> bus, when being removed.
>>
>> Signed-off-by: Umang Jain <umang.jain@ideasonboard.com>
>> ---
> Don't forget a changelog when possible.
>
> v2:
> - Check driver->remove before calling
Already mentioned in cover-letter v2 changelog
>
> Reviewed-by: Kieran Bingham <kieran.bingham@ideasonboard.com>
>
>> .../vc04_services/interface/vchiq_arm/vchiq_bus.c | 10 ++++++++++
>> 1 file changed, 10 insertions(+)
>>
>> diff --git a/drivers/staging/vc04_services/interface/vchiq_arm/vchiq_bus.c b/drivers/staging/vc04_services/interface/vchiq_arm/vchiq_bus.c
>> index 68f830d75531..93609716df84 100644
>> --- a/drivers/staging/vc04_services/interface/vchiq_arm/vchiq_bus.c
>> +++ b/drivers/staging/vc04_services/interface/vchiq_arm/vchiq_bus.c
>> @@ -37,11 +37,21 @@ static int vchiq_bus_probe(struct device *dev)
>> return driver->probe(device);
>> }
>>
>> +static void vchiq_bus_remove(struct device *dev)
>> +{
>> + struct vchiq_device *device = to_vchiq_device(dev);
>> + struct vchiq_driver *driver = to_vchiq_driver(dev->driver);
>> +
>> + if (driver->remove)
>> + driver->remove(device);
>> +}
>> +
>> const struct bus_type vchiq_bus_type = {
>> .name = "vchiq-bus",
>> .match = vchiq_bus_type_match,
>> .uevent = vchiq_bus_uevent,
>> .probe = vchiq_bus_probe,
>> + .remove = vchiq_bus_remove,
>> };
>>
>> static void vchiq_device_release(struct device *dev)
>> --
>> 2.43.0
>>
^ permalink raw reply [flat|nested] 18+ messages in thread
* Re: [PATCH v2 5/5] staging: vc04_services: vchiq_core: Stop kthreads on shutdown
2024-03-21 13:07 ` [PATCH v2 5/5] staging: vc04_services: vchiq_core: Stop kthreads on shutdown Umang Jain
@ 2024-03-26 21:22 ` Stefan Wahren
2024-03-27 2:52 ` Umang Jain
2024-03-27 18:19 ` Stefan Wahren
2024-03-28 14:53 ` Mark Brown
2 siblings, 1 reply; 18+ messages in thread
From: Stefan Wahren @ 2024-03-26 21:22 UTC (permalink / raw
To: Umang Jain, linux-staging
Cc: Dan Carpenter, Kieran Bingham, Laurent Pinchart, Dave Stevenson,
Phil Elwell, Greg KH
Hi Umang,
Am 21.03.24 um 14:07 schrieb Umang Jain:
> The various kthreads thread functions (slot_handler_func, sync_func,
> recycle_func) in vchiq_core and vchiq_keepalive_thread_func in
> vchiq_arm should be stopped on vchiq_shutdown().
>
> This also address the following TODO item:
> * Fix kernel module support
>
> hence drop it from the TODO item list.
>
> Signed-off-by: Umang Jain <umang.jain@ideasonboard.com>
> Reviewed-by: Kieran Bingham <kieran.bingham@ideasonboard.com>
> ---
i applied this series on top of Linux 6.9-rc1 and it seems to corrupt
the VideoCore firmware (at least on Raspberry Pi 3 B+ /
multi_v7_defconfig + all VCHIQ modules) so badly that X on Raspberry Pi
OS (32 bit, Bullseye) doesn't start:
[ 10.670310] vchiq: module is from the staging directory, the quality
is unknown, you have been warned.
[ 10.983666] usbcore: registered new device driver onboard-usb-hub
[ 10.983915] usb 1-1.1: USB disconnect, device number 3
[ 10.983931] usb 1-1.1.1: USB disconnect, device number 6
[ 10.988341] Bluetooth: Core ver 2.22
[ 10.988469] NET: Registered PF_BLUETOOTH protocol family
[ 10.988479] Bluetooth: HCI device and connection manager initialized
[ 10.988501] Bluetooth: HCI socket layer initialized
[ 10.988513] Bluetooth: L2CAP socket layer initialized
[ 10.988530] Bluetooth: SCO socket layer initialized
[ 11.038416] Bluetooth: HCI UART driver ver 2.3
[ 11.038447] Bluetooth: HCI UART protocol H4 registered
[ 11.038833] Bluetooth: HCI UART protocol Broadcom registered
[ 11.040011] hci_uart_bcm serial0-0: supply vbat not found, using
dummy regulator
[ 11.040155] hci_uart_bcm serial0-0: supply vddio not found, using
dummy regulator
[ 11.120024] usb 1-1.1.2: USB disconnect, device number 5
[ 11.136892] cfg80211: Loading compiled-in X.509 certificates for
regulatory database
[ 11.160172] usb 1-1.3: USB disconnect, device number 4
[ 11.331526] Loaded X.509 cert 'sforshee: 00b28ddf47aef9cea7'
[ 11.333122] Loaded X.509 cert 'wens:
61c038651aabdcf94bd0ac7ff06c7248db18c600'
[ 11.384304] brcmfmac: brcmf_fw_alloc_request: using
brcm/brcmfmac43455-sdio for chip BCM4345/6
[ 11.418684] Bluetooth: hci0: BCM: chip id 107
[ 11.418993] Bluetooth: hci0: BCM: features 0x2f
[ 11.420509] Bluetooth: hci0: BCM4345C0
[ 11.420533] Bluetooth: hci0: BCM4345C0 (003.001.025) build 0000
[ 11.424274] Console: switching to colour dummy device 80x30
[ 11.437793] vc4-drm soc:gpu: bound 3f400000.hvs (ops vc4_hvs_ops [vc4])
[ 11.441693] vc4-drm soc:gpu: bound 3f902000.hdmi (ops vc4_hdmi_ops [vc4])
[ 11.441938] vc4-drm soc:gpu: bound 3f806000.vec (ops vc4_vec_ops [vc4])
[ 11.442151] vc4-drm soc:gpu: bound 3f004000.txp (ops vc4_txp_ops [vc4])
[ 11.442330] vc4-drm soc:gpu: bound 3f206000.pixelvalve (ops
vc4_crtc_ops [vc4])
[ 11.442509] vc4-drm soc:gpu: bound 3f207000.pixelvalve (ops
vc4_crtc_ops [vc4])
[ 11.442681] vc4-drm soc:gpu: bound 3f807000.pixelvalve (ops
vc4_crtc_ops [vc4])
[ 11.442826] vc4-drm soc:gpu: bound 3fc00000.v3d (ops vc4_v3d_ops [vc4])
[ 11.446831] cfg80211: loaded regulatory.db is malformed or signature
is missing/invalid
[ 11.448067] [drm] Initialized vc4 0.0.0 20140616 for soc:gpu on minor 0
[ 11.461177] Bluetooth: hci0: BCM4345C0
'brcm/BCM4345C0.raspberrypi,3-model-b-plus.hcd' Patch
[ 11.554003] Console: switching to colour frame buffer device 240x75
[ 11.586937] vc4-drm soc:gpu: [drm] fb0: vc4drmfb frame buffer device
[ 11.690580] brcmfmac: brcmf_c_process_txcap_blob: no txcap_blob
available (err=-2)
[ 11.691029] brcmfmac: brcmf_c_preinit_dcmds: Firmware: BCM4345/6 wl0:
Nov 1 2021 00:37:25 version 7.45.241 (1a2f2fa CY) FWID 01-703fd60
[ 11.851054] hub 1-1:1.0: USB hub found
[ 11.851192] hub 1-1:1.0: 4 ports detected
[ 11.854947] onboard-usb-hub 3f980000.usb:usb-port@1: supply vdd not
found, using dummy regulator
[ 11.866465] hub 1-1:1.0: USB hub found
[ 11.867729] hub 1-1:1.0: 4 ports detected
[ 11.880932] onboard-usb-hub 3f980000.usb:usb-port@1:usb-port@1:
supply vdd not found, using dummy regulator
[ 12.198004] usb 1-1.1: new high-speed USB device number 7 using dwc2
[ 12.413343] hub 1-1.1:1.0: USB hub found
[ 12.417836] hub 1-1.1:1.0: 3 ports detected
[ 12.521956] Bluetooth: hci0: BCM: features 0x2f
[ 12.523795] Bluetooth: hci0: BCM43455 37.4MHz Raspberry Pi 3+-0190
[ 12.523821] Bluetooth: hci0: BCM4345C0 (003.001.025) build 0382
[ 12.708052] usb 1-1.3: new low-speed USB device number 8 using dwc2
[ 12.870179] input: HID 046a:0011 as
/devices/platform/soc/3f980000.usb/usb1/1-1/1-1.3/1-1.3:1.0/0003:046A:0011.0003/input/input2
[ 12.940173] hid-generic 0003:046A:0011.0003: input: USB HID v1.11
Keyboard [HID 046a:0011] on usb-3f980000.usb-1.3/input0
[ 12.948025] usb 1-1.1.2: new low-speed USB device number 9 using dwc2
[ 13.095578] input: PixArt Microsoft USB Optical Mouse as
/devices/platform/soc/3f980000.usb/usb1/1-1/1-1.1/1-1.1.2/1-1.1.2:1.0/0003:045E:00CB.0004/input/input3
[ 13.095764] hid-generic 0003:045E:00CB.0004: input: USB HID v1.11
Mouse [PixArt Microsoft USB Optical Mouse] on usb-3f980000.usb-1.1.2/input0
[ 13.388013] usb 1-1.1.1: new high-speed USB device number 10 using dwc2
[ 13.497783] snd_bcm2835: module is from the staging directory, the
quality is unknown, you have been warned.
[ 13.505953] bcm2835-audio bcm2835-audio: card created with 8 channels
[ 13.806061] lan78xx 1-1.1.1:1.0 (unnamed net_device) (uninitialized):
No External EEPROM. Setting MAC Speed
[ 15.033962] mc: Linux media interface: v0.10
[ 16.894910] videodev: Linux video capture interface: v2.00
[ 17.061120] bcm2835_mmal_vchiq: module is from the staging directory,
the quality is unknown, you have been warned.
[ 17.214469] bcm2835_v4l2: module is from the staging directory, the
quality is unknown, you have been warned.
[ 17.222047] bcm2835_vchiq 3f00b840.mailbox: sync: error: 0: sf
unexpected msgid 0@58908716,0
I also noticed that not all vchiq kthreads are functional and a vcgencmd
command from the startup is hanging endlessly:
PID TTY STAT TIME COMMAND
1 ? Ss 0:05 /sbin/init splash
2 ? S 0:00 [kthreadd]
3 ? S 0:00 [pool_workqueue_release]
4 ? I< 0:00 [kworker/R-rcu_g]
5 ? I< 0:00 [kworker/R-slub_]
6 ? I 0:00 [kworker/0:0-events_freezable]
8 ? I 0:00 [kworker/0:1-cgroup_destroy]
9 ? I 0:00 [kworker/u16:0-ipv6_addrconf]
10 ? I< 0:00 [kworker/R-mm_pe]
11 ? S 0:00 [ksoftirqd/0]
12 ? I 0:00 [rcu_sched]
13 ? S 0:00 [rcu_exp_par_gp_kthread_worker/0]
14 ? S 0:00 [rcu_exp_gp_kthread_worker]
15 ? S 0:00 [migration/0]
16 ? S 0:00 [cpuhp/0]
17 ? S 0:00 [cpuhp/1]
18 ? S 0:00 [migration/1]
19 ? S 0:00 [ksoftirqd/1]
20 ? I 0:00 [kworker/1:0-pm]
22 ? S 0:00 [cpuhp/2]
23 ? S 0:00 [migration/2]
24 ? S 0:00 [ksoftirqd/2]
26 ? I< 0:00 [kworker/2:0H-kblockd]
27 ? S 0:00 [cpuhp/3]
28 ? S 0:00 [migration/3]
29 ? S 0:00 [ksoftirqd/3]
30 ? I 0:00 [kworker/3:0-events]
32 ? I 0:01 [kworker/u17:0-async]
33 ? I 0:00 [kworker/u18:0-events_unbound]
34 ? I 0:00 [kworker/u19:0-events_unbound]
35 ? I 0:00 [kworker/u20:0-writeback]
36 ? S 0:00 [kdevtmpfs]
37 ? I< 0:00 [kworker/R-inet_]
38 ? I 0:00 [kworker/u17:1-events_unbound]
39 ? S 0:00 [oom_reaper]
40 ? I< 0:00 [kworker/R-write]
41 ? S 0:00 [kcompactd0]
42 ? I< 0:00 [kworker/R-kbloc]
43 ? I< 0:00 [kworker/R-ata_s]
44 ? I< 0:00 [kworker/R-edac-]
45 ? I< 0:00 [kworker/R-devfr]
46 ? I 0:00 [kworker/3:1-mm_percpu_wq]
47 ? S 0:00 [watchdogd]
48 ? I 0:00 [kworker/u20:1-writeback]
49 ? I 0:00 [kworker/1:1-events_power_efficient]
50 ? I< 0:00 [kworker/R-rpcio]
51 ? I< 0:00 [kworker/1:1H-kblockd]
52 ? I< 0:00 [kworker/R-xprti]
53 ? S 0:00 [kswapd0]
54 ? I 0:00 [kworker/u18:1-events_unbound]
55 ? I< 0:00 [kworker/R-nfsio]
56 ? I 0:00 [kworker/u17:2-events_unbound]
58 ? S 0:00 [hwrng]
61 ? S 0:00 [irq/113-mmc0]
62 ? I< 0:00 [kworker/R-mld]
63 ? I< 0:00 [kworker/R-ipv6_]
64 ? I 0:00 [kworker/u16:1]
65 ? I< 0:00 [kworker/R-mmc_c]
67 ? I< 0:00 [kworker/0:1H-kblockd]
68 ? I 0:00 [kworker/1:2-events]
69 ? I< 0:00 [kworker/u21:0-hci0]
70 ? I< 0:00 [kworker/u22:0]
71 ? I< 0:00 [kworker/u23:0]
72 ? I< 0:00 [kworker/u24:0]
73 ? I< 0:00 [kworker/u25:0]
74 ? I< 0:00 [kworker/R-sdhci]
75 ? S 0:00 [irq/119-mmc1]
77 ? I 0:00 [kworker/2:2-events]
79 ? I 0:00 [kworker/2:4-events]
80 ? I 0:00 [kworker/0:2-events]
81 ? S 0:00 [jbd2/mmcblk0p2-8]
82 ? I< 0:00 [kworker/R-ext4-]
91 ? I< 0:00 [kworker/0:2H-kblockd]
97 ? I< 0:00 [kworker/3:1H-mmc_complete]
98 ? I< 0:00 [kworker/3:2H]
124 ? Ss 0:00 /lib/systemd/systemd-journald
131 ? I 0:00 [kworker/3:2-events]
138 ? I 0:00 [kworker/u19:1-writeback]
145 ? I< 0:00 [kworker/2:2H-kblockd]
146 ? I 0:00 [kworker/u20:2-events_unbound]
149 ? Ss 0:04 /lib/systemd/systemd-udevd
150 ? I< 0:00 [kworker/1:2H-kblockd]
151 ? S 0:00 /lib/systemd/systemd-udevd
152 ? S 0:01 /lib/systemd/systemd-udevd
153 ? S 0:01 /lib/systemd/systemd-udevd
154 ? S 0:01 /lib/systemd/systemd-udevd
156 ? S 0:00 /lib/systemd/systemd-udevd
157 ? S 0:00 /lib/systemd/systemd-udevd
158 ? S 0:00 /lib/systemd/systemd-udevd
159 ? S 0:01 /lib/systemd/systemd-udevd
160 ? D 0:01 /lib/systemd/systemd-udevd
161 ? S 0:01 /lib/systemd/systemd-udevd
211 ? I< 0:00 [kworker/R-cfg80]
212 ? I< 0:00 [kworker/R-brcmf]
213 ? S 0:00 [brcmf_wdog/mmc1:0001:1]
214 ? S 0:00 [card0-crtc0]
215 ? S 0:00 [card0-crtc1]
216 ? S 0:00 [card0-crtc2]
217 ? S 0:00 [card0-crtc3]
220 ? I 0:00 [kworker/0:3-pm]
221 ? I< 0:00 [kworker/u21:3-hci0]
223 ? I 0:00 [kworker/u19:2-events_unbound]
253 ? D 0:00 [vchiq-keep/0]
269 ? S 0:00 [irq/126-usb-001:010:01]
298 ? I 0:00 [kworker/3:3-events]
302 ? Ssl 0:00 /lib/systemd/systemd-timesyncd
307 ? I< 0:00 [kworker/R-mmal-]
346 ? Ss 0:00 avahi-daemon: running [raspberrypi.local]
347 ? Ss 0:00 /usr/sbin/cron -f
348 ? Ss 0:00 /usr/bin/dbus-daemon --system
--address=systemd: --nofork --nopidfile --systemd-activation --syslog-only
354 ? Ssl 0:00 /usr/libexec/polkitd --no-debug
363 ? Ssl 0:00 /usr/sbin/rsyslogd -n -iNONE
364 ? Ss 0:00 /lib/systemd/systemd-logind
365 ? Ss 0:00 /usr/sbin/thd --triggers
/etc/triggerhappy/triggers.d/ --socket /run/thd.socket --user nobody
--deviceglob /dev/input/event*
366 ? Ssl 0:00 /usr/libexec/udisks2/udisksd
367 ? Ss 0:00 /sbin/wpa_supplicant -u -s -O
/run/wpa_supplicant
391 ? Ss 0:00 /usr/sbin/dhcpcd -b -q
394 ? S 0:00 avahi-daemon: chroot helper
424 ? Ss 0:00 /usr/libexec/bluetooth/bluetoothd
466 ? SLsl 0:00 /usr/sbin/rngd -r /dev/hwrng
486 ? Ss 0:00 /usr/sbin/cupsd -l
489 ? Ss 0:00 wpa_supplicant -B
-c/etc/wpa_supplicant/wpa_supplicant.conf -iwlan0
498 ? Ssl 0:00 /usr/sbin/lightdm
506 ? I 0:00 [kworker/3:4]
507 ? Ssl 0:00 /usr/sbin/ModemManager
515 ? Ss 0:00 sshd: /usr/sbin/sshd -D [listener] 0 of
10-100 startups
520 ? I< 0:00 [kworker/R-crypt]
521 tty7 Ssl+ 0:00 /usr/lib/xorg/Xorg :0 -seat seat0 -auth
/var/run/lightdm/root/:0 -nolisten tcp vt7 -novtswitch
522 tty1 Ss 0:00 /bin/login -f
527 ttyS1 Ss 0:00 /bin/login -p --
533 ? S 0:00 /usr/lib/cups/notifier/dbus dbus://
534 ? S 0:00 /usr/lib/cups/notifier/dbus dbus://
535 ? Ssl 0:00 /usr/sbin/cups-browsed
544 ? I 0:00 [kworker/u19:3]
553 ? Ss 0:01 /lib/systemd/systemd --user
557 ? S 0:00 (sd-pam)
594 ? I 0:00 [kworker/u20:3]
596 ? Sl 0:00 lightdm --session-child 14 17
597 ? S<sl 0:00 /usr/bin/pipewire
604 ? S<sl 0:00 /usr/bin/pulseaudio --daemonize=no
--log-target=journal
606 ? SNsl 0:00 /usr/libexec/rtkit-daemon
612 tty1 S+ 0:00 -bash
615 ? Ss 0:00 /bin/sh /usr/bin/x-session-manager
626 ? Ss 0:00 /usr/bin/dbus-daemon --session
--address=systemd: --nofork --nopidfile --systemd-activation --syslog-only
640 ? S<l 0:00 /usr/bin/pipewire-media-session
645 ? I 0:00 [kworker/u17:3-events_unbound]
684 ? Ss 0:00 /usr/bin/ssh-agent x-session-manager
703 ? S 0:00 /bin/sh /usr/bin/x-session-manager
704 ? D 0:00 vcgencmd get_config total_mem
705 ? S 0:00 cut -d= -f2
706 ? I 0:00 [kworker/0:4]
732 ttyS1 S 0:00 -bash
751 ttyS1 R+ 0:00 ps ax
All patches of this series before this one are fine. Just this one is bad.
Regards
^ permalink raw reply [flat|nested] 18+ messages in thread
* Re: [PATCH v2 5/5] staging: vc04_services: vchiq_core: Stop kthreads on shutdown
2024-03-26 21:22 ` Stefan Wahren
@ 2024-03-27 2:52 ` Umang Jain
0 siblings, 0 replies; 18+ messages in thread
From: Umang Jain @ 2024-03-27 2:52 UTC (permalink / raw
To: Stefan Wahren, linux-staging
Cc: Dan Carpenter, Kieran Bingham, Laurent Pinchart, Dave Stevenson,
Phil Elwell, Greg KH
Hi Stefan
On 27/03/24 2:52 am, Stefan Wahren wrote:
> Hi Umang,
>
> Am 21.03.24 um 14:07 schrieb Umang Jain:
>> The various kthreads thread functions (slot_handler_func, sync_func,
>> recycle_func) in vchiq_core and vchiq_keepalive_thread_func in
>> vchiq_arm should be stopped on vchiq_shutdown().
>>
>> This also address the following TODO item:
>> * Fix kernel module support
>>
>> hence drop it from the TODO item list.
>>
>> Signed-off-by: Umang Jain <umang.jain@ideasonboard.com>
>> Reviewed-by: Kieran Bingham <kieran.bingham@ideasonboard.com>
>> ---
>
> i applied this series on top of Linux 6.9-rc1 and it seems to corrupt
> the VideoCore firmware (at least on Raspberry Pi 3 B+ /
> multi_v7_defconfig + all VCHIQ modules) so badly that X on Raspberry Pi
> OS (32 bit, Bullseye) doesn't start:
>
> [ 10.670310] vchiq: module is from the staging directory, the quality
> is unknown, you have been warned.
> [ 10.983666] usbcore: registered new device driver onboard-usb-hub
> [ 10.983915] usb 1-1.1: USB disconnect, device number 3
> [ 10.983931] usb 1-1.1.1: USB disconnect, device number 6
> [ 10.988341] Bluetooth: Core ver 2.22
> [ 10.988469] NET: Registered PF_BLUETOOTH protocol family
> [ 10.988479] Bluetooth: HCI device and connection manager initialized
> [ 10.988501] Bluetooth: HCI socket layer initialized
> [ 10.988513] Bluetooth: L2CAP socket layer initialized
> [ 10.988530] Bluetooth: SCO socket layer initialized
> [ 11.038416] Bluetooth: HCI UART driver ver 2.3
> [ 11.038447] Bluetooth: HCI UART protocol H4 registered
> [ 11.038833] Bluetooth: HCI UART protocol Broadcom registered
> [ 11.040011] hci_uart_bcm serial0-0: supply vbat not found, using
> dummy regulator
> [ 11.040155] hci_uart_bcm serial0-0: supply vddio not found, using
> dummy regulator
> [ 11.120024] usb 1-1.1.2: USB disconnect, device number 5
> [ 11.136892] cfg80211: Loading compiled-in X.509 certificates for
> regulatory database
> [ 11.160172] usb 1-1.3: USB disconnect, device number 4
> [ 11.331526] Loaded X.509 cert 'sforshee: 00b28ddf47aef9cea7'
> [ 11.333122] Loaded X.509 cert 'wens:
> 61c038651aabdcf94bd0ac7ff06c7248db18c600'
> [ 11.384304] brcmfmac: brcmf_fw_alloc_request: using
> brcm/brcmfmac43455-sdio for chip BCM4345/6
> [ 11.418684] Bluetooth: hci0: BCM: chip id 107
> [ 11.418993] Bluetooth: hci0: BCM: features 0x2f
> [ 11.420509] Bluetooth: hci0: BCM4345C0
> [ 11.420533] Bluetooth: hci0: BCM4345C0 (003.001.025) build 0000
> [ 11.424274] Console: switching to colour dummy device 80x30
> [ 11.437793] vc4-drm soc:gpu: bound 3f400000.hvs (ops vc4_hvs_ops
> [vc4])
> [ 11.441693] vc4-drm soc:gpu: bound 3f902000.hdmi (ops vc4_hdmi_ops
> [vc4])
> [ 11.441938] vc4-drm soc:gpu: bound 3f806000.vec (ops vc4_vec_ops
> [vc4])
> [ 11.442151] vc4-drm soc:gpu: bound 3f004000.txp (ops vc4_txp_ops
> [vc4])
> [ 11.442330] vc4-drm soc:gpu: bound 3f206000.pixelvalve (ops
> vc4_crtc_ops [vc4])
> [ 11.442509] vc4-drm soc:gpu: bound 3f207000.pixelvalve (ops
> vc4_crtc_ops [vc4])
> [ 11.442681] vc4-drm soc:gpu: bound 3f807000.pixelvalve (ops
> vc4_crtc_ops [vc4])
> [ 11.442826] vc4-drm soc:gpu: bound 3fc00000.v3d (ops vc4_v3d_ops
> [vc4])
> [ 11.446831] cfg80211: loaded regulatory.db is malformed or signature
> is missing/invalid
> [ 11.448067] [drm] Initialized vc4 0.0.0 20140616 for soc:gpu on
> minor 0
> [ 11.461177] Bluetooth: hci0: BCM4345C0
> 'brcm/BCM4345C0.raspberrypi,3-model-b-plus.hcd' Patch
> [ 11.554003] Console: switching to colour frame buffer device 240x75
> [ 11.586937] vc4-drm soc:gpu: [drm] fb0: vc4drmfb frame buffer device
> [ 11.690580] brcmfmac: brcmf_c_process_txcap_blob: no txcap_blob
> available (err=-2)
> [ 11.691029] brcmfmac: brcmf_c_preinit_dcmds: Firmware: BCM4345/6 wl0:
> Nov 1 2021 00:37:25 version 7.45.241 (1a2f2fa CY) FWID 01-703fd60
> [ 11.851054] hub 1-1:1.0: USB hub found
> [ 11.851192] hub 1-1:1.0: 4 ports detected
> [ 11.854947] onboard-usb-hub 3f980000.usb:usb-port@1: supply vdd not
> found, using dummy regulator
> [ 11.866465] hub 1-1:1.0: USB hub found
> [ 11.867729] hub 1-1:1.0: 4 ports detected
> [ 11.880932] onboard-usb-hub 3f980000.usb:usb-port@1:usb-port@1:
> supply vdd not found, using dummy regulator
> [ 12.198004] usb 1-1.1: new high-speed USB device number 7 using dwc2
> [ 12.413343] hub 1-1.1:1.0: USB hub found
> [ 12.417836] hub 1-1.1:1.0: 3 ports detected
> [ 12.521956] Bluetooth: hci0: BCM: features 0x2f
> [ 12.523795] Bluetooth: hci0: BCM43455 37.4MHz Raspberry Pi 3+-0190
> [ 12.523821] Bluetooth: hci0: BCM4345C0 (003.001.025) build 0382
> [ 12.708052] usb 1-1.3: new low-speed USB device number 8 using dwc2
> [ 12.870179] input: HID 046a:0011 as
> /devices/platform/soc/3f980000.usb/usb1/1-1/1-1.3/1-1.3:1.0/0003:046A:0011.0003/input/input2
>
> [ 12.940173] hid-generic 0003:046A:0011.0003: input: USB HID v1.11
> Keyboard [HID 046a:0011] on usb-3f980000.usb-1.3/input0
> [ 12.948025] usb 1-1.1.2: new low-speed USB device number 9 using dwc2
> [ 13.095578] input: PixArt Microsoft USB Optical Mouse as
> /devices/platform/soc/3f980000.usb/usb1/1-1/1-1.1/1-1.1.2/1-1.1.2:1.0/0003:045E:00CB.0004/input/input3
>
> [ 13.095764] hid-generic 0003:045E:00CB.0004: input: USB HID v1.11
> Mouse [PixArt Microsoft USB Optical Mouse] on
> usb-3f980000.usb-1.1.2/input0
> [ 13.388013] usb 1-1.1.1: new high-speed USB device number 10 using
> dwc2
> [ 13.497783] snd_bcm2835: module is from the staging directory, the
> quality is unknown, you have been warned.
> [ 13.505953] bcm2835-audio bcm2835-audio: card created with 8 channels
> [ 13.806061] lan78xx 1-1.1.1:1.0 (unnamed net_device) (uninitialized):
> No External EEPROM. Setting MAC Speed
> [ 15.033962] mc: Linux media interface: v0.10
> [ 16.894910] videodev: Linux video capture interface: v2.00
> [ 17.061120] bcm2835_mmal_vchiq: module is from the staging directory,
> the quality is unknown, you have been warned.
> [ 17.214469] bcm2835_v4l2: module is from the staging directory, the
> quality is unknown, you have been warned.
> [ 17.222047] bcm2835_vchiq 3f00b840.mailbox: sync: error: 0: sf
> unexpected msgid 0@58908716,0
>
> I also noticed that not all vchiq kthreads are functional and a vcgencmd
> command from the startup is hanging endlessly:
Atleast I can say from looking at it, that this is not expected at all.
This patch is stopping the kthreads on shutdown path(i.e. the module
being unloaded) and the corruption here is on module loading...
I will have to reproduce on rpi-3-b, I have one lying around, and shall
get back to you on this.
>
> PID TTY STAT TIME COMMAND
> 1 ? Ss 0:05 /sbin/init splash
> 2 ? S 0:00 [kthreadd]
> 3 ? S 0:00 [pool_workqueue_release]
> 4 ? I< 0:00 [kworker/R-rcu_g]
> 5 ? I< 0:00 [kworker/R-slub_]
> 6 ? I 0:00 [kworker/0:0-events_freezable]
> 8 ? I 0:00 [kworker/0:1-cgroup_destroy]
> 9 ? I 0:00 [kworker/u16:0-ipv6_addrconf]
> 10 ? I< 0:00 [kworker/R-mm_pe]
> 11 ? S 0:00 [ksoftirqd/0]
> 12 ? I 0:00 [rcu_sched]
> 13 ? S 0:00 [rcu_exp_par_gp_kthread_worker/0]
> 14 ? S 0:00 [rcu_exp_gp_kthread_worker]
> 15 ? S 0:00 [migration/0]
> 16 ? S 0:00 [cpuhp/0]
> 17 ? S 0:00 [cpuhp/1]
> 18 ? S 0:00 [migration/1]
> 19 ? S 0:00 [ksoftirqd/1]
> 20 ? I 0:00 [kworker/1:0-pm]
> 22 ? S 0:00 [cpuhp/2]
> 23 ? S 0:00 [migration/2]
> 24 ? S 0:00 [ksoftirqd/2]
> 26 ? I< 0:00 [kworker/2:0H-kblockd]
> 27 ? S 0:00 [cpuhp/3]
> 28 ? S 0:00 [migration/3]
> 29 ? S 0:00 [ksoftirqd/3]
> 30 ? I 0:00 [kworker/3:0-events]
> 32 ? I 0:01 [kworker/u17:0-async]
> 33 ? I 0:00 [kworker/u18:0-events_unbound]
> 34 ? I 0:00 [kworker/u19:0-events_unbound]
> 35 ? I 0:00 [kworker/u20:0-writeback]
> 36 ? S 0:00 [kdevtmpfs]
> 37 ? I< 0:00 [kworker/R-inet_]
> 38 ? I 0:00 [kworker/u17:1-events_unbound]
> 39 ? S 0:00 [oom_reaper]
> 40 ? I< 0:00 [kworker/R-write]
> 41 ? S 0:00 [kcompactd0]
> 42 ? I< 0:00 [kworker/R-kbloc]
> 43 ? I< 0:00 [kworker/R-ata_s]
> 44 ? I< 0:00 [kworker/R-edac-]
> 45 ? I< 0:00 [kworker/R-devfr]
> 46 ? I 0:00 [kworker/3:1-mm_percpu_wq]
> 47 ? S 0:00 [watchdogd]
> 48 ? I 0:00 [kworker/u20:1-writeback]
> 49 ? I 0:00 [kworker/1:1-events_power_efficient]
> 50 ? I< 0:00 [kworker/R-rpcio]
> 51 ? I< 0:00 [kworker/1:1H-kblockd]
> 52 ? I< 0:00 [kworker/R-xprti]
> 53 ? S 0:00 [kswapd0]
> 54 ? I 0:00 [kworker/u18:1-events_unbound]
> 55 ? I< 0:00 [kworker/R-nfsio]
> 56 ? I 0:00 [kworker/u17:2-events_unbound]
> 58 ? S 0:00 [hwrng]
> 61 ? S 0:00 [irq/113-mmc0]
> 62 ? I< 0:00 [kworker/R-mld]
> 63 ? I< 0:00 [kworker/R-ipv6_]
> 64 ? I 0:00 [kworker/u16:1]
> 65 ? I< 0:00 [kworker/R-mmc_c]
> 67 ? I< 0:00 [kworker/0:1H-kblockd]
> 68 ? I 0:00 [kworker/1:2-events]
> 69 ? I< 0:00 [kworker/u21:0-hci0]
> 70 ? I< 0:00 [kworker/u22:0]
> 71 ? I< 0:00 [kworker/u23:0]
> 72 ? I< 0:00 [kworker/u24:0]
> 73 ? I< 0:00 [kworker/u25:0]
> 74 ? I< 0:00 [kworker/R-sdhci]
> 75 ? S 0:00 [irq/119-mmc1]
> 77 ? I 0:00 [kworker/2:2-events]
> 79 ? I 0:00 [kworker/2:4-events]
> 80 ? I 0:00 [kworker/0:2-events]
> 81 ? S 0:00 [jbd2/mmcblk0p2-8]
> 82 ? I< 0:00 [kworker/R-ext4-]
> 91 ? I< 0:00 [kworker/0:2H-kblockd]
> 97 ? I< 0:00 [kworker/3:1H-mmc_complete]
> 98 ? I< 0:00 [kworker/3:2H]
> 124 ? Ss 0:00 /lib/systemd/systemd-journald
> 131 ? I 0:00 [kworker/3:2-events]
> 138 ? I 0:00 [kworker/u19:1-writeback]
> 145 ? I< 0:00 [kworker/2:2H-kblockd]
> 146 ? I 0:00 [kworker/u20:2-events_unbound]
> 149 ? Ss 0:04 /lib/systemd/systemd-udevd
> 150 ? I< 0:00 [kworker/1:2H-kblockd]
> 151 ? S 0:00 /lib/systemd/systemd-udevd
> 152 ? S 0:01 /lib/systemd/systemd-udevd
> 153 ? S 0:01 /lib/systemd/systemd-udevd
> 154 ? S 0:01 /lib/systemd/systemd-udevd
> 156 ? S 0:00 /lib/systemd/systemd-udevd
> 157 ? S 0:00 /lib/systemd/systemd-udevd
> 158 ? S 0:00 /lib/systemd/systemd-udevd
> 159 ? S 0:01 /lib/systemd/systemd-udevd
> 160 ? D 0:01 /lib/systemd/systemd-udevd
> 161 ? S 0:01 /lib/systemd/systemd-udevd
> 211 ? I< 0:00 [kworker/R-cfg80]
> 212 ? I< 0:00 [kworker/R-brcmf]
> 213 ? S 0:00 [brcmf_wdog/mmc1:0001:1]
> 214 ? S 0:00 [card0-crtc0]
> 215 ? S 0:00 [card0-crtc1]
> 216 ? S 0:00 [card0-crtc2]
> 217 ? S 0:00 [card0-crtc3]
> 220 ? I 0:00 [kworker/0:3-pm]
> 221 ? I< 0:00 [kworker/u21:3-hci0]
> 223 ? I 0:00 [kworker/u19:2-events_unbound]
> 253 ? D 0:00 [vchiq-keep/0]
> 269 ? S 0:00 [irq/126-usb-001:010:01]
> 298 ? I 0:00 [kworker/3:3-events]
> 302 ? Ssl 0:00 /lib/systemd/systemd-timesyncd
> 307 ? I< 0:00 [kworker/R-mmal-]
> 346 ? Ss 0:00 avahi-daemon: running [raspberrypi.local]
> 347 ? Ss 0:00 /usr/sbin/cron -f
> 348 ? Ss 0:00 /usr/bin/dbus-daemon --system
> --address=systemd: --nofork --nopidfile --systemd-activation
> --syslog-only
> 354 ? Ssl 0:00 /usr/libexec/polkitd --no-debug
> 363 ? Ssl 0:00 /usr/sbin/rsyslogd -n -iNONE
> 364 ? Ss 0:00 /lib/systemd/systemd-logind
> 365 ? Ss 0:00 /usr/sbin/thd --triggers
> /etc/triggerhappy/triggers.d/ --socket /run/thd.socket --user nobody
> --deviceglob /dev/input/event*
> 366 ? Ssl 0:00 /usr/libexec/udisks2/udisksd
> 367 ? Ss 0:00 /sbin/wpa_supplicant -u -s -O
> /run/wpa_supplicant
> 391 ? Ss 0:00 /usr/sbin/dhcpcd -b -q
> 394 ? S 0:00 avahi-daemon: chroot helper
> 424 ? Ss 0:00 /usr/libexec/bluetooth/bluetoothd
> 466 ? SLsl 0:00 /usr/sbin/rngd -r /dev/hwrng
> 486 ? Ss 0:00 /usr/sbin/cupsd -l
> 489 ? Ss 0:00 wpa_supplicant -B
> -c/etc/wpa_supplicant/wpa_supplicant.conf -iwlan0
> 498 ? Ssl 0:00 /usr/sbin/lightdm
> 506 ? I 0:00 [kworker/3:4]
> 507 ? Ssl 0:00 /usr/sbin/ModemManager
> 515 ? Ss 0:00 sshd: /usr/sbin/sshd -D [listener] 0 of
> 10-100 startups
> 520 ? I< 0:00 [kworker/R-crypt]
> 521 tty7 Ssl+ 0:00 /usr/lib/xorg/Xorg :0 -seat seat0 -auth
> /var/run/lightdm/root/:0 -nolisten tcp vt7 -novtswitch
> 522 tty1 Ss 0:00 /bin/login -f
> 527 ttyS1 Ss 0:00 /bin/login -p --
> 533 ? S 0:00 /usr/lib/cups/notifier/dbus dbus://
> 534 ? S 0:00 /usr/lib/cups/notifier/dbus dbus://
> 535 ? Ssl 0:00 /usr/sbin/cups-browsed
> 544 ? I 0:00 [kworker/u19:3]
> 553 ? Ss 0:01 /lib/systemd/systemd --user
> 557 ? S 0:00 (sd-pam)
> 594 ? I 0:00 [kworker/u20:3]
> 596 ? Sl 0:00 lightdm --session-child 14 17
> 597 ? S<sl 0:00 /usr/bin/pipewire
> 604 ? S<sl 0:00 /usr/bin/pulseaudio --daemonize=no
> --log-target=journal
> 606 ? SNsl 0:00 /usr/libexec/rtkit-daemon
> 612 tty1 S+ 0:00 -bash
> 615 ? Ss 0:00 /bin/sh /usr/bin/x-session-manager
> 626 ? Ss 0:00 /usr/bin/dbus-daemon --session
> --address=systemd: --nofork --nopidfile --systemd-activation
> --syslog-only
> 640 ? S<l 0:00 /usr/bin/pipewire-media-session
> 645 ? I 0:00 [kworker/u17:3-events_unbound]
> 684 ? Ss 0:00 /usr/bin/ssh-agent x-session-manager
> 703 ? S 0:00 /bin/sh /usr/bin/x-session-manager
> 704 ? D 0:00 vcgencmd get_config total_mem
> 705 ? S 0:00 cut -d= -f2
> 706 ? I 0:00 [kworker/0:4]
> 732 ttyS1 S 0:00 -bash
> 751 ttyS1 R+ 0:00 ps ax
>
> All patches of this series before this one are fine. Just this one is
> bad.
>
> Regards
^ permalink raw reply [flat|nested] 18+ messages in thread
* Re: [PATCH v2 5/5] staging: vc04_services: vchiq_core: Stop kthreads on shutdown
2024-03-21 13:07 ` [PATCH v2 5/5] staging: vc04_services: vchiq_core: Stop kthreads on shutdown Umang Jain
2024-03-26 21:22 ` Stefan Wahren
@ 2024-03-27 18:19 ` Stefan Wahren
2024-03-27 20:00 ` Umang Jain
2024-03-28 14:53 ` Mark Brown
2 siblings, 1 reply; 18+ messages in thread
From: Stefan Wahren @ 2024-03-27 18:19 UTC (permalink / raw
To: Umang Jain, linux-staging
Cc: Dan Carpenter, Kieran Bingham, Laurent Pinchart, Dave Stevenson,
Phil Elwell, Greg KH
Hi Umang,
Am 21.03.24 um 14:07 schrieb Umang Jain:
> The various kthreads thread functions (slot_handler_func, sync_func,
> recycle_func) in vchiq_core and vchiq_keepalive_thread_func in
> vchiq_arm should be stopped on vchiq_shutdown().
>
> This also address the following TODO item:
> * Fix kernel module support
>
> hence drop it from the TODO item list.
>
> Signed-off-by: Umang Jain <umang.jain@ideasonboard.com>
> Reviewed-by: Kieran Bingham <kieran.bingham@ideasonboard.com>
> ---
> drivers/staging/vc04_services/interface/TODO | 7 -------
> .../vc04_services/interface/vchiq_arm/vchiq_arm.c | 8 ++++++--
> .../vc04_services/interface/vchiq_arm/vchiq_core.c | 10 +++++++---
> 3 files changed, 13 insertions(+), 12 deletions(-)
>
> diff --git a/drivers/staging/vc04_services/interface/TODO b/drivers/staging/vc04_services/interface/TODO
> index 05eb5140d096..57a2d6761f9b 100644
> --- a/drivers/staging/vc04_services/interface/TODO
> +++ b/drivers/staging/vc04_services/interface/TODO
> @@ -16,13 +16,6 @@ some of the ones we want:
> to manage these buffers as dmabufs so that we can zero-copy import
> camera images into vc4 for rendering/display.
>
> -* Fix kernel module support
> -
> -Even the VPU firmware doesn't support a VCHI re-connect, the driver
> -should properly handle a module unload. This also includes that all
> -resources must be freed (kthreads, debugfs entries, ...) and global
> -variables avoided.
> -
> * Documentation
>
> A short top-down description of this driver's architecture (function of
> diff --git a/drivers/staging/vc04_services/interface/vchiq_arm/vchiq_arm.c b/drivers/staging/vc04_services/interface/vchiq_arm/vchiq_arm.c
> index ad506016fc93..1d21f9cfbfe5 100644
> --- a/drivers/staging/vc04_services/interface/vchiq_arm/vchiq_arm.c
> +++ b/drivers/staging/vc04_services/interface/vchiq_arm/vchiq_arm.c
> @@ -726,8 +726,9 @@ void free_bulk_waiter(struct vchiq_instance *instance)
>
> int vchiq_shutdown(struct vchiq_instance *instance)
> {
> - int status = 0;
> struct vchiq_state *state = instance->state;
> + struct vchiq_arm_state *arm_state;
> + int status = 0;
>
> if (mutex_lock_killable(&state->mutex))
> return -EAGAIN;
> @@ -739,6 +740,9 @@ int vchiq_shutdown(struct vchiq_instance *instance)
>
> dev_dbg(state->dev, "core: (%p): returning %d\n", instance, status);
>
> + arm_state = vchiq_platform_get_arm_state(state);
> + kthread_stop(arm_state->ka_thread);
> +
> free_bulk_waiter(instance);
> kfree(instance);
>
> @@ -1310,7 +1314,7 @@ vchiq_keepalive_thread_func(void *v)
> goto shutdown;
> }
>
> - while (1) {
> + while (!kthread_should_stop()) {
> long rc = 0, uc = 0;
>
> if (wait_for_completion_interruptible(&arm_state->ka_evt)) {
> diff --git a/drivers/staging/vc04_services/interface/vchiq_arm/vchiq_core.c b/drivers/staging/vc04_services/interface/vchiq_arm/vchiq_core.c
> index 76c27778154a..953ccd87f425 100644
> --- a/drivers/staging/vc04_services/interface/vchiq_arm/vchiq_core.c
> +++ b/drivers/staging/vc04_services/interface/vchiq_arm/vchiq_core.c
> @@ -1936,7 +1936,7 @@ slot_handler_func(void *v)
>
> DEBUG_INITIALISE(local);
>
> - while (1) {
> + while (!kthread_should_stop()) {
> DEBUG_COUNT(SLOT_HANDLER_COUNT);
> DEBUG_TRACE(SLOT_HANDLER_LINE);
> remote_event_wait(&state->trigger_event, &local->trigger);
> @@ -1978,7 +1978,7 @@ recycle_func(void *v)
> if (!found)
> return -ENOMEM;
>
> - while (1) {
> + while (!kthread_should_stop()) {
> remote_event_wait(&state->recycle_event, &local->recycle);
>
> process_free_queue(state, found, length);
> @@ -1997,7 +1997,7 @@ sync_func(void *v)
> state->remote->slot_sync);
> int svc_fourcc;
>
> - while (1) {
> + while (!kthread_should_stop()) {
> struct vchiq_service *service;
> int msgid, size;
> int type;
> @@ -2844,6 +2844,10 @@ vchiq_shutdown_internal(struct vchiq_state *state, struct vchiq_instance *instan
> (void)vchiq_remove_service(instance, service->handle);
> vchiq_service_put(service);
> }
> +
> + kthread_stop(state->sync_thread);
> + kthread_stop(state->recycle_thread);
> + kthread_stop(state->slot_handler_thread);
i had a little bit time to look at the corrupt issue, i think this
caused by this change. This function is called by vchiq_shutdown which
is exported and used by VCHIQ user drivers like bcm2835-audio or
mmal-vchiq. So this is absolutely not the right place to stop the kernel
threads and the issue is not specific to the Raspberry Pi 3.
> }
>
> int
^ permalink raw reply [flat|nested] 18+ messages in thread
* Re: [PATCH v2 5/5] staging: vc04_services: vchiq_core: Stop kthreads on shutdown
2024-03-27 18:19 ` Stefan Wahren
@ 2024-03-27 20:00 ` Umang Jain
2024-03-27 20:40 ` Stefan Wahren
0 siblings, 1 reply; 18+ messages in thread
From: Umang Jain @ 2024-03-27 20:00 UTC (permalink / raw
To: Stefan Wahren, linux-staging
Cc: Dan Carpenter, Kieran Bingham, Laurent Pinchart, Dave Stevenson,
Phil Elwell, Greg KH
Hi Stefan
On 27/03/24 11:49 pm, Stefan Wahren wrote:
> Hi Umang,
>
> Am 21.03.24 um 14:07 schrieb Umang Jain:
>> The various kthreads thread functions (slot_handler_func, sync_func,
>> recycle_func) in vchiq_core and vchiq_keepalive_thread_func in
>> vchiq_arm should be stopped on vchiq_shutdown().
>>
>> This also address the following TODO item:
>> * Fix kernel module support
>>
>> hence drop it from the TODO item list.
>>
>> Signed-off-by: Umang Jain <umang.jain@ideasonboard.com>
>> Reviewed-by: Kieran Bingham <kieran.bingham@ideasonboard.com>
>> ---
>> drivers/staging/vc04_services/interface/TODO | 7 -------
>> .../vc04_services/interface/vchiq_arm/vchiq_arm.c | 8 ++++++--
>> .../vc04_services/interface/vchiq_arm/vchiq_core.c | 10 +++++++---
>> 3 files changed, 13 insertions(+), 12 deletions(-)
>>
>> diff --git a/drivers/staging/vc04_services/interface/TODO
>> b/drivers/staging/vc04_services/interface/TODO
>> index 05eb5140d096..57a2d6761f9b 100644
>> --- a/drivers/staging/vc04_services/interface/TODO
>> +++ b/drivers/staging/vc04_services/interface/TODO
>> @@ -16,13 +16,6 @@ some of the ones we want:
>> to manage these buffers as dmabufs so that we can zero-copy import
>> camera images into vc4 for rendering/display.
>>
>> -* Fix kernel module support
>> -
>> -Even the VPU firmware doesn't support a VCHI re-connect, the driver
>> -should properly handle a module unload. This also includes that all
>> -resources must be freed (kthreads, debugfs entries, ...) and global
>> -variables avoided.
>> -
>> * Documentation
>>
>> A short top-down description of this driver's architecture
>> (function of
>> diff --git
>> a/drivers/staging/vc04_services/interface/vchiq_arm/vchiq_arm.c
>> b/drivers/staging/vc04_services/interface/vchiq_arm/vchiq_arm.c
>> index ad506016fc93..1d21f9cfbfe5 100644
>> --- a/drivers/staging/vc04_services/interface/vchiq_arm/vchiq_arm.c
>> +++ b/drivers/staging/vc04_services/interface/vchiq_arm/vchiq_arm.c
>> @@ -726,8 +726,9 @@ void free_bulk_waiter(struct vchiq_instance
>> *instance)
>>
>> int vchiq_shutdown(struct vchiq_instance *instance)
>> {
>> - int status = 0;
>> struct vchiq_state *state = instance->state;
>> + struct vchiq_arm_state *arm_state;
>> + int status = 0;
>>
>> if (mutex_lock_killable(&state->mutex))
>> return -EAGAIN;
>> @@ -739,6 +740,9 @@ int vchiq_shutdown(struct vchiq_instance *instance)
>>
>> dev_dbg(state->dev, "core: (%p): returning %d\n", instance,
>> status);
>>
>> + arm_state = vchiq_platform_get_arm_state(state);
>> + kthread_stop(arm_state->ka_thread);
>> +
>> free_bulk_waiter(instance);
>> kfree(instance);
>>
>> @@ -1310,7 +1314,7 @@ vchiq_keepalive_thread_func(void *v)
>> goto shutdown;
>> }
>>
>> - while (1) {
>> + while (!kthread_should_stop()) {
>> long rc = 0, uc = 0;
>>
>> if (wait_for_completion_interruptible(&arm_state->ka_evt)) {
>> diff --git
>> a/drivers/staging/vc04_services/interface/vchiq_arm/vchiq_core.c
>> b/drivers/staging/vc04_services/interface/vchiq_arm/vchiq_core.c
>> index 76c27778154a..953ccd87f425 100644
>> --- a/drivers/staging/vc04_services/interface/vchiq_arm/vchiq_core.c
>> +++ b/drivers/staging/vc04_services/interface/vchiq_arm/vchiq_core.c
>> @@ -1936,7 +1936,7 @@ slot_handler_func(void *v)
>>
>> DEBUG_INITIALISE(local);
>>
>> - while (1) {
>> + while (!kthread_should_stop()) {
>> DEBUG_COUNT(SLOT_HANDLER_COUNT);
>> DEBUG_TRACE(SLOT_HANDLER_LINE);
>> remote_event_wait(&state->trigger_event, &local->trigger);
>> @@ -1978,7 +1978,7 @@ recycle_func(void *v)
>> if (!found)
>> return -ENOMEM;
>>
>> - while (1) {
>> + while (!kthread_should_stop()) {
>> remote_event_wait(&state->recycle_event, &local->recycle);
>>
>> process_free_queue(state, found, length);
>> @@ -1997,7 +1997,7 @@ sync_func(void *v)
>> state->remote->slot_sync);
>> int svc_fourcc;
>>
>> - while (1) {
>> + while (!kthread_should_stop()) {
>> struct vchiq_service *service;
>> int msgid, size;
>> int type;
>> @@ -2844,6 +2844,10 @@ vchiq_shutdown_internal(struct vchiq_state
>> *state, struct vchiq_instance *instan
>> (void)vchiq_remove_service(instance, service->handle);
>> vchiq_service_put(service);
>> }
>> +
>> + kthread_stop(state->sync_thread);
>> + kthread_stop(state->recycle_thread);
>> + kthread_stop(state->slot_handler_thread);
>
> i had a little bit time to look at the corrupt issue, i think this
> caused by this change. This function is called by vchiq_shutdown which
> is exported and used by VCHIQ user drivers like bcm2835-audio or
> mmal-vchiq. So this is absolutely not the right place to stop the kernel
> threads and the issue is not specific to the Raspberry Pi 3.
Can you try building with ARCH=64-bit and testing it please? Please
refer to the cover section where I have tested 64-bit on rpi4.
Regarding 32-bit build for my Pi4 - everything gets broken for me RPi4
(if I try to test via SD-card) ... and I am also having problems with
setting up a NFS-mounted 32-bit rootfs today so can't boot the pi
through NFS with 32-bit kernel :(
My Pi3 power cable/microUSB went missing, so I have ordered a new one.
It will take a couple of days to arrive before I can test it there.
>
>> }
>>
>> int
^ permalink raw reply [flat|nested] 18+ messages in thread
* Re: [PATCH v2 5/5] staging: vc04_services: vchiq_core: Stop kthreads on shutdown
2024-03-27 20:00 ` Umang Jain
@ 2024-03-27 20:40 ` Stefan Wahren
2024-03-28 5:18 ` Umang Jain
0 siblings, 1 reply; 18+ messages in thread
From: Stefan Wahren @ 2024-03-27 20:40 UTC (permalink / raw
To: Umang Jain, linux-staging
Cc: Dan Carpenter, Kieran Bingham, Laurent Pinchart, Dave Stevenson,
Phil Elwell, Greg KH
Hi Umang,
Am 27.03.24 um 21:00 schrieb Umang Jain:
> Hi Stefan
>
> On 27/03/24 11:49 pm, Stefan Wahren wrote:
>> Hi Umang,
>>
>> Am 21.03.24 um 14:07 schrieb Umang Jain:
>>> The various kthreads thread functions (slot_handler_func, sync_func,
>>> recycle_func) in vchiq_core and vchiq_keepalive_thread_func in
>>> vchiq_arm should be stopped on vchiq_shutdown().
>>>
>>> This also address the following TODO item:
>>> * Fix kernel module support
>>>
>>> hence drop it from the TODO item list.
>>>
>>> Signed-off-by: Umang Jain <umang.jain@ideasonboard.com>
>>> Reviewed-by: Kieran Bingham <kieran.bingham@ideasonboard.com>
>>> ---
>>> drivers/staging/vc04_services/interface/TODO | 7 -------
>>> .../vc04_services/interface/vchiq_arm/vchiq_arm.c | 8 ++++++--
>>> .../vc04_services/interface/vchiq_arm/vchiq_core.c | 10
>>> +++++++---
>>> 3 files changed, 13 insertions(+), 12 deletions(-)
>>>
>>> diff --git a/drivers/staging/vc04_services/interface/TODO
>>> b/drivers/staging/vc04_services/interface/TODO
>>> index 05eb5140d096..57a2d6761f9b 100644
>>> --- a/drivers/staging/vc04_services/interface/TODO
>>> +++ b/drivers/staging/vc04_services/interface/TODO
>>> @@ -16,13 +16,6 @@ some of the ones we want:
>>> to manage these buffers as dmabufs so that we can zero-copy import
>>> camera images into vc4 for rendering/display.
>>>
>>> -* Fix kernel module support
>>> -
>>> -Even the VPU firmware doesn't support a VCHI re-connect, the driver
>>> -should properly handle a module unload. This also includes that all
>>> -resources must be freed (kthreads, debugfs entries, ...) and global
>>> -variables avoided.
>>> -
>>> * Documentation
>>>
>>> A short top-down description of this driver's architecture
>>> (function of
>>> diff --git
>>> a/drivers/staging/vc04_services/interface/vchiq_arm/vchiq_arm.c
>>> b/drivers/staging/vc04_services/interface/vchiq_arm/vchiq_arm.c
>>> index ad506016fc93..1d21f9cfbfe5 100644
>>> --- a/drivers/staging/vc04_services/interface/vchiq_arm/vchiq_arm.c
>>> +++ b/drivers/staging/vc04_services/interface/vchiq_arm/vchiq_arm.c
>>> @@ -726,8 +726,9 @@ void free_bulk_waiter(struct vchiq_instance
>>> *instance)
>>>
>>> int vchiq_shutdown(struct vchiq_instance *instance)
>>> {
>>> - int status = 0;
>>> struct vchiq_state *state = instance->state;
>>> + struct vchiq_arm_state *arm_state;
>>> + int status = 0;
>>>
>>> if (mutex_lock_killable(&state->mutex))
>>> return -EAGAIN;
>>> @@ -739,6 +740,9 @@ int vchiq_shutdown(struct vchiq_instance *instance)
>>>
>>> dev_dbg(state->dev, "core: (%p): returning %d\n", instance,
>>> status);
>>>
>>> + arm_state = vchiq_platform_get_arm_state(state);
>>> + kthread_stop(arm_state->ka_thread);
>>> +
>>> free_bulk_waiter(instance);
>>> kfree(instance);
>>>
>>> @@ -1310,7 +1314,7 @@ vchiq_keepalive_thread_func(void *v)
>>> goto shutdown;
>>> }
>>>
>>> - while (1) {
>>> + while (!kthread_should_stop()) {
>>> long rc = 0, uc = 0;
>>>
>>> if (wait_for_completion_interruptible(&arm_state->ka_evt)) {
>>> diff --git
>>> a/drivers/staging/vc04_services/interface/vchiq_arm/vchiq_core.c
>>> b/drivers/staging/vc04_services/interface/vchiq_arm/vchiq_core.c
>>> index 76c27778154a..953ccd87f425 100644
>>> --- a/drivers/staging/vc04_services/interface/vchiq_arm/vchiq_core.c
>>> +++ b/drivers/staging/vc04_services/interface/vchiq_arm/vchiq_core.c
>>> @@ -1936,7 +1936,7 @@ slot_handler_func(void *v)
>>>
>>> DEBUG_INITIALISE(local);
>>>
>>> - while (1) {
>>> + while (!kthread_should_stop()) {
>>> DEBUG_COUNT(SLOT_HANDLER_COUNT);
>>> DEBUG_TRACE(SLOT_HANDLER_LINE);
>>> remote_event_wait(&state->trigger_event, &local->trigger);
>>> @@ -1978,7 +1978,7 @@ recycle_func(void *v)
>>> if (!found)
>>> return -ENOMEM;
>>>
>>> - while (1) {
>>> + while (!kthread_should_stop()) {
>>> remote_event_wait(&state->recycle_event, &local->recycle);
>>>
>>> process_free_queue(state, found, length);
>>> @@ -1997,7 +1997,7 @@ sync_func(void *v)
>>> state->remote->slot_sync);
>>> int svc_fourcc;
>>>
>>> - while (1) {
>>> + while (!kthread_should_stop()) {
>>> struct vchiq_service *service;
>>> int msgid, size;
>>> int type;
>>> @@ -2844,6 +2844,10 @@ vchiq_shutdown_internal(struct vchiq_state
>>> *state, struct vchiq_instance *instan
>>> (void)vchiq_remove_service(instance, service->handle);
>>> vchiq_service_put(service);
>>> }
>>> +
>>> + kthread_stop(state->sync_thread);
>>> + kthread_stop(state->recycle_thread);
>>> + kthread_stop(state->slot_handler_thread);
>>
>> i had a little bit time to look at the corrupt issue, i think this
>> caused by this change. This function is called by vchiq_shutdown which
>> is exported and used by VCHIQ user drivers like bcm2835-audio or
>> mmal-vchiq. So this is absolutely not the right place to stop the kernel
>> threads and the issue is not specific to the Raspberry Pi 3.
>
> Can you try building with ARCH=64-bit and testing it please? Please
> refer to the cover section where I have tested 64-bit on rpi4.
sorry, i don't understand the point behind this. As i mention it's clear
that we cannot call kthread_stop() from vchiq_shutdown_internal because
this function is also used by different drivers. The calls to create the
kthreads should be symmetric to the stop calls. From my understanding
the kthreads are created from vchiq_probe(), so the kthreads should
stopped directly or indirectly from vchiq_remove().
This has nothing to do with Raspberry Pi 3 vs 4 or 32 bit vs 64 bit. It
was just luck that it worked in your case.
>
> Regarding 32-bit build for my Pi4 - everything gets broken for me RPi4
> (if I try to test via SD-card) ... and I am also having problems with
> setting up a NFS-mounted 32-bit rootfs today so can't boot the pi
> through NFS with 32-bit kernel :(
>
> My Pi3 power cable/microUSB went missing, so I have ordered a new one.
> It will take a couple of days to arrive before I can test it there.
>
>
>>
>>> }
>>>
>>> int
>
^ permalink raw reply [flat|nested] 18+ messages in thread
* Re: [PATCH v2 5/5] staging: vc04_services: vchiq_core: Stop kthreads on shutdown
2024-03-27 20:40 ` Stefan Wahren
@ 2024-03-28 5:18 ` Umang Jain
2024-03-28 10:45 ` Dave Stevenson
0 siblings, 1 reply; 18+ messages in thread
From: Umang Jain @ 2024-03-28 5:18 UTC (permalink / raw
To: Stefan Wahren, linux-staging
Cc: Dan Carpenter, Kieran Bingham, Laurent Pinchart, Dave Stevenson,
Phil Elwell, Greg KH
Hi Stefan,
On 28/03/24 2:10 am, Stefan Wahren wrote:
> Hi Umang,
>
> Am 27.03.24 um 21:00 schrieb Umang Jain:
>> Hi Stefan
>>
>> On 27/03/24 11:49 pm, Stefan Wahren wrote:
>>> Hi Umang,
>>>
>>> Am 21.03.24 um 14:07 schrieb Umang Jain:
>>>> The various kthreads thread functions (slot_handler_func, sync_func,
>>>> recycle_func) in vchiq_core and vchiq_keepalive_thread_func in
>>>> vchiq_arm should be stopped on vchiq_shutdown().
>>>>
>>>> This also address the following TODO item:
>>>> * Fix kernel module support
>>>>
>>>> hence drop it from the TODO item list.
>>>>
>>>> Signed-off-by: Umang Jain <umang.jain@ideasonboard.com>
>>>> Reviewed-by: Kieran Bingham <kieran.bingham@ideasonboard.com>
>>>> ---
>>>> drivers/staging/vc04_services/interface/TODO | 7 -------
>>>> .../vc04_services/interface/vchiq_arm/vchiq_arm.c | 8 ++++++--
>>>> .../vc04_services/interface/vchiq_arm/vchiq_core.c | 10
>>>> +++++++---
>>>> 3 files changed, 13 insertions(+), 12 deletions(-)
>>>>
>>>> diff --git a/drivers/staging/vc04_services/interface/TODO
>>>> b/drivers/staging/vc04_services/interface/TODO
>>>> index 05eb5140d096..57a2d6761f9b 100644
>>>> --- a/drivers/staging/vc04_services/interface/TODO
>>>> +++ b/drivers/staging/vc04_services/interface/TODO
>>>> @@ -16,13 +16,6 @@ some of the ones we want:
>>>> to manage these buffers as dmabufs so that we can zero-copy import
>>>> camera images into vc4 for rendering/display.
>>>>
>>>> -* Fix kernel module support
>>>> -
>>>> -Even the VPU firmware doesn't support a VCHI re-connect, the driver
>>>> -should properly handle a module unload. This also includes that all
>>>> -resources must be freed (kthreads, debugfs entries, ...) and global
>>>> -variables avoided.
>>>> -
>>>> * Documentation
>>>>
>>>> A short top-down description of this driver's architecture
>>>> (function of
>>>> diff --git
>>>> a/drivers/staging/vc04_services/interface/vchiq_arm/vchiq_arm.c
>>>> b/drivers/staging/vc04_services/interface/vchiq_arm/vchiq_arm.c
>>>> index ad506016fc93..1d21f9cfbfe5 100644
>>>> --- a/drivers/staging/vc04_services/interface/vchiq_arm/vchiq_arm.c
>>>> +++ b/drivers/staging/vc04_services/interface/vchiq_arm/vchiq_arm.c
>>>> @@ -726,8 +726,9 @@ void free_bulk_waiter(struct vchiq_instance
>>>> *instance)
>>>>
>>>> int vchiq_shutdown(struct vchiq_instance *instance)
>>>> {
>>>> - int status = 0;
>>>> struct vchiq_state *state = instance->state;
>>>> + struct vchiq_arm_state *arm_state;
>>>> + int status = 0;
>>>>
>>>> if (mutex_lock_killable(&state->mutex))
>>>> return -EAGAIN;
>>>> @@ -739,6 +740,9 @@ int vchiq_shutdown(struct vchiq_instance
>>>> *instance)
>>>>
>>>> dev_dbg(state->dev, "core: (%p): returning %d\n", instance,
>>>> status);
>>>>
>>>> + arm_state = vchiq_platform_get_arm_state(state);
>>>> + kthread_stop(arm_state->ka_thread);
>>>> +
>>>> free_bulk_waiter(instance);
>>>> kfree(instance);
>>>>
>>>> @@ -1310,7 +1314,7 @@ vchiq_keepalive_thread_func(void *v)
>>>> goto shutdown;
>>>> }
>>>>
>>>> - while (1) {
>>>> + while (!kthread_should_stop()) {
>>>> long rc = 0, uc = 0;
>>>>
>>>> if (wait_for_completion_interruptible(&arm_state->ka_evt)) {
>>>> diff --git
>>>> a/drivers/staging/vc04_services/interface/vchiq_arm/vchiq_core.c
>>>> b/drivers/staging/vc04_services/interface/vchiq_arm/vchiq_core.c
>>>> index 76c27778154a..953ccd87f425 100644
>>>> --- a/drivers/staging/vc04_services/interface/vchiq_arm/vchiq_core.c
>>>> +++ b/drivers/staging/vc04_services/interface/vchiq_arm/vchiq_core.c
>>>> @@ -1936,7 +1936,7 @@ slot_handler_func(void *v)
>>>>
>>>> DEBUG_INITIALISE(local);
>>>>
>>>> - while (1) {
>>>> + while (!kthread_should_stop()) {
>>>> DEBUG_COUNT(SLOT_HANDLER_COUNT);
>>>> DEBUG_TRACE(SLOT_HANDLER_LINE);
>>>> remote_event_wait(&state->trigger_event, &local->trigger);
>>>> @@ -1978,7 +1978,7 @@ recycle_func(void *v)
>>>> if (!found)
>>>> return -ENOMEM;
>>>>
>>>> - while (1) {
>>>> + while (!kthread_should_stop()) {
>>>> remote_event_wait(&state->recycle_event, &local->recycle);
>>>>
>>>> process_free_queue(state, found, length);
>>>> @@ -1997,7 +1997,7 @@ sync_func(void *v)
>>>> state->remote->slot_sync);
>>>> int svc_fourcc;
>>>>
>>>> - while (1) {
>>>> + while (!kthread_should_stop()) {
>>>> struct vchiq_service *service;
>>>> int msgid, size;
>>>> int type;
>>>> @@ -2844,6 +2844,10 @@ vchiq_shutdown_internal(struct vchiq_state
>>>> *state, struct vchiq_instance *instan
>>>> (void)vchiq_remove_service(instance, service->handle);
>>>> vchiq_service_put(service);
>>>> }
>>>> +
>>>> + kthread_stop(state->sync_thread);
>>>> + kthread_stop(state->recycle_thread);
>>>> + kthread_stop(state->slot_handler_thread);
>>>
>>> i had a little bit time to look at the corrupt issue, i think this
>>> caused by this change. This function is called by vchiq_shutdown which
>>> is exported and used by VCHIQ user drivers like bcm2835-audio or
>>> mmal-vchiq. So this is absolutely not the right place to stop the
>>> kernel
>>> threads and the issue is not specific to the Raspberry Pi 3.
>>
>> Can you try building with ARCH=64-bit and testing it please? Please
>> refer to the cover section where I have tested 64-bit on rpi4.
> sorry, i don't understand the point behind this. As i mention it's clear
> that we cannot call kthread_stop() from vchiq_shutdown_internal because
> this function is also used by different drivers. The calls to create the
> kthreads should be symmetric to the stop calls. From my understanding
> the kthreads are created from vchiq_probe(), so the kthreads should
> stopped directly or indirectly from vchiq_remove().
>
> This has nothing to do with Raspberry Pi 3 vs 4 or 32 bit vs 64 bit. It
> was just luck that it worked in your case.
It is my paranoia that there are probably firmware related differences
between 32-bit and 64-bit.
Since, I had no way to booting a 32-bit kernel yesterday and then I saw
such reports
https://github.com/raspberrypi/firmware/issues/1795#issuecomment-1478976548
https://github.com/raspberrypi/firmware/issues/1795#issuecomment-1483176397
Even the "32-bit" RPi OS came with 64-bit kernel (only rootfs was
32-bit). It made me thinking if the support for 32-bit kernel is still
around or dropped somehow.
See https://forums.raspberrypi.com/viewtopic.php?t=367889
Anyway, I reproduced the issue on rpi-3-b as your setup and prepared a
patch. It's on the list now.
Thanks for reporting!
>>
>> Regarding 32-bit build for my Pi4 - everything gets broken for me RPi4
>> (if I try to test via SD-card) ... and I am also having problems with
>> setting up a NFS-mounted 32-bit rootfs today so can't boot the pi
>> through NFS with 32-bit kernel :(
>>
>> My Pi3 power cable/microUSB went missing, so I have ordered a new one.
>> It will take a couple of days to arrive before I can test it there.
>>
>>
>>>
>>>> }
>>>>
>>>> int
>>
>
^ permalink raw reply [flat|nested] 18+ messages in thread
* Re: [PATCH v2 5/5] staging: vc04_services: vchiq_core: Stop kthreads on shutdown
2024-03-28 5:18 ` Umang Jain
@ 2024-03-28 10:45 ` Dave Stevenson
0 siblings, 0 replies; 18+ messages in thread
From: Dave Stevenson @ 2024-03-28 10:45 UTC (permalink / raw
To: Umang Jain
Cc: Stefan Wahren, linux-staging, Dan Carpenter, Kieran Bingham,
Laurent Pinchart, Phil Elwell, Greg KH
Hi Umang
On Thu, 28 Mar 2024 at 05:18, Umang Jain <umang.jain@ideasonboard.com> wrote:
>
> Hi Stefan,
>
> On 28/03/24 2:10 am, Stefan Wahren wrote:
> > Hi Umang,
> >
> > Am 27.03.24 um 21:00 schrieb Umang Jain:
> >> Hi Stefan
> >>
> >> On 27/03/24 11:49 pm, Stefan Wahren wrote:
> >>> Hi Umang,
> >>>
> >>> Am 21.03.24 um 14:07 schrieb Umang Jain:
> >>>> The various kthreads thread functions (slot_handler_func, sync_func,
> >>>> recycle_func) in vchiq_core and vchiq_keepalive_thread_func in
> >>>> vchiq_arm should be stopped on vchiq_shutdown().
> >>>>
> >>>> This also address the following TODO item:
> >>>> * Fix kernel module support
> >>>>
> >>>> hence drop it from the TODO item list.
> >>>>
> >>>> Signed-off-by: Umang Jain <umang.jain@ideasonboard.com>
> >>>> Reviewed-by: Kieran Bingham <kieran.bingham@ideasonboard.com>
> >>>> ---
> >>>> drivers/staging/vc04_services/interface/TODO | 7 -------
> >>>> .../vc04_services/interface/vchiq_arm/vchiq_arm.c | 8 ++++++--
> >>>> .../vc04_services/interface/vchiq_arm/vchiq_core.c | 10
> >>>> +++++++---
> >>>> 3 files changed, 13 insertions(+), 12 deletions(-)
> >>>>
> >>>> diff --git a/drivers/staging/vc04_services/interface/TODO
> >>>> b/drivers/staging/vc04_services/interface/TODO
> >>>> index 05eb5140d096..57a2d6761f9b 100644
> >>>> --- a/drivers/staging/vc04_services/interface/TODO
> >>>> +++ b/drivers/staging/vc04_services/interface/TODO
> >>>> @@ -16,13 +16,6 @@ some of the ones we want:
> >>>> to manage these buffers as dmabufs so that we can zero-copy import
> >>>> camera images into vc4 for rendering/display.
> >>>>
> >>>> -* Fix kernel module support
> >>>> -
> >>>> -Even the VPU firmware doesn't support a VCHI re-connect, the driver
> >>>> -should properly handle a module unload. This also includes that all
> >>>> -resources must be freed (kthreads, debugfs entries, ...) and global
> >>>> -variables avoided.
> >>>> -
> >>>> * Documentation
> >>>>
> >>>> A short top-down description of this driver's architecture
> >>>> (function of
> >>>> diff --git
> >>>> a/drivers/staging/vc04_services/interface/vchiq_arm/vchiq_arm.c
> >>>> b/drivers/staging/vc04_services/interface/vchiq_arm/vchiq_arm.c
> >>>> index ad506016fc93..1d21f9cfbfe5 100644
> >>>> --- a/drivers/staging/vc04_services/interface/vchiq_arm/vchiq_arm.c
> >>>> +++ b/drivers/staging/vc04_services/interface/vchiq_arm/vchiq_arm.c
> >>>> @@ -726,8 +726,9 @@ void free_bulk_waiter(struct vchiq_instance
> >>>> *instance)
> >>>>
> >>>> int vchiq_shutdown(struct vchiq_instance *instance)
> >>>> {
> >>>> - int status = 0;
> >>>> struct vchiq_state *state = instance->state;
> >>>> + struct vchiq_arm_state *arm_state;
> >>>> + int status = 0;
> >>>>
> >>>> if (mutex_lock_killable(&state->mutex))
> >>>> return -EAGAIN;
> >>>> @@ -739,6 +740,9 @@ int vchiq_shutdown(struct vchiq_instance
> >>>> *instance)
> >>>>
> >>>> dev_dbg(state->dev, "core: (%p): returning %d\n", instance,
> >>>> status);
> >>>>
> >>>> + arm_state = vchiq_platform_get_arm_state(state);
> >>>> + kthread_stop(arm_state->ka_thread);
> >>>> +
> >>>> free_bulk_waiter(instance);
> >>>> kfree(instance);
> >>>>
> >>>> @@ -1310,7 +1314,7 @@ vchiq_keepalive_thread_func(void *v)
> >>>> goto shutdown;
> >>>> }
> >>>>
> >>>> - while (1) {
> >>>> + while (!kthread_should_stop()) {
> >>>> long rc = 0, uc = 0;
> >>>>
> >>>> if (wait_for_completion_interruptible(&arm_state->ka_evt)) {
> >>>> diff --git
> >>>> a/drivers/staging/vc04_services/interface/vchiq_arm/vchiq_core.c
> >>>> b/drivers/staging/vc04_services/interface/vchiq_arm/vchiq_core.c
> >>>> index 76c27778154a..953ccd87f425 100644
> >>>> --- a/drivers/staging/vc04_services/interface/vchiq_arm/vchiq_core.c
> >>>> +++ b/drivers/staging/vc04_services/interface/vchiq_arm/vchiq_core.c
> >>>> @@ -1936,7 +1936,7 @@ slot_handler_func(void *v)
> >>>>
> >>>> DEBUG_INITIALISE(local);
> >>>>
> >>>> - while (1) {
> >>>> + while (!kthread_should_stop()) {
> >>>> DEBUG_COUNT(SLOT_HANDLER_COUNT);
> >>>> DEBUG_TRACE(SLOT_HANDLER_LINE);
> >>>> remote_event_wait(&state->trigger_event, &local->trigger);
> >>>> @@ -1978,7 +1978,7 @@ recycle_func(void *v)
> >>>> if (!found)
> >>>> return -ENOMEM;
> >>>>
> >>>> - while (1) {
> >>>> + while (!kthread_should_stop()) {
> >>>> remote_event_wait(&state->recycle_event, &local->recycle);
> >>>>
> >>>> process_free_queue(state, found, length);
> >>>> @@ -1997,7 +1997,7 @@ sync_func(void *v)
> >>>> state->remote->slot_sync);
> >>>> int svc_fourcc;
> >>>>
> >>>> - while (1) {
> >>>> + while (!kthread_should_stop()) {
> >>>> struct vchiq_service *service;
> >>>> int msgid, size;
> >>>> int type;
> >>>> @@ -2844,6 +2844,10 @@ vchiq_shutdown_internal(struct vchiq_state
> >>>> *state, struct vchiq_instance *instan
> >>>> (void)vchiq_remove_service(instance, service->handle);
> >>>> vchiq_service_put(service);
> >>>> }
> >>>> +
> >>>> + kthread_stop(state->sync_thread);
> >>>> + kthread_stop(state->recycle_thread);
> >>>> + kthread_stop(state->slot_handler_thread);
> >>>
> >>> i had a little bit time to look at the corrupt issue, i think this
> >>> caused by this change. This function is called by vchiq_shutdown which
> >>> is exported and used by VCHIQ user drivers like bcm2835-audio or
> >>> mmal-vchiq. So this is absolutely not the right place to stop the
> >>> kernel
> >>> threads and the issue is not specific to the Raspberry Pi 3.
> >>
> >> Can you try building with ARCH=64-bit and testing it please? Please
> >> refer to the cover section where I have tested 64-bit on rpi4.
> > sorry, i don't understand the point behind this. As i mention it's clear
> > that we cannot call kthread_stop() from vchiq_shutdown_internal because
> > this function is also used by different drivers. The calls to create the
> > kthreads should be symmetric to the stop calls. From my understanding
> > the kthreads are created from vchiq_probe(), so the kthreads should
> > stopped directly or indirectly from vchiq_remove().
> >
> > This has nothing to do with Raspberry Pi 3 vs 4 or 32 bit vs 64 bit. It
> > was just luck that it worked in your case.
>
> It is my paranoia that there are probably firmware related differences
> between 32-bit and 64-bit.
>
> Since, I had no way to booting a 32-bit kernel yesterday and then I saw
> such reports
>
> https://github.com/raspberrypi/firmware/issues/1795#issuecomment-1478976548
> https://github.com/raspberrypi/firmware/issues/1795#issuecomment-1483176397
>
> Even the "32-bit" RPi OS came with 64-bit kernel (only rootfs was
> 32-bit). It made me thinking if the support for 32-bit kernel is still
> around or dropped somehow.
> See https://forums.raspberrypi.com/viewtopic.php?t=367889
Only on Pi4.
A 64bit kernel is preferable to LPAE where there is a need to address
more than 4GB of RAM.
Pi0-2 are only 32bit processors so will always run a 32bit kernel.
Pi3 & 02 only have 1GB and 512MB of RAM respectively, so having the
smaller pointer size of 32bit is generally an advantage, even though
they can run a 64bit kernel (and OS).
Dave
> Anyway, I reproduced the issue on rpi-3-b as your setup and prepared a
> patch. It's on the list now.
> Thanks for reporting!
>
>
> >>
> >> Regarding 32-bit build for my Pi4 - everything gets broken for me RPi4
> >> (if I try to test via SD-card) ... and I am also having problems with
> >> setting up a NFS-mounted 32-bit rootfs today so can't boot the pi
> >> through NFS with 32-bit kernel :(
> >>
> >> My Pi3 power cable/microUSB went missing, so I have ordered a new one.
> >> It will take a couple of days to arrive before I can test it there.
> >>
> >>
> >>>
> >>>> }
> >>>>
> >>>> int
> >>
> >
>
^ permalink raw reply [flat|nested] 18+ messages in thread
* Re: [PATCH v2 5/5] staging: vc04_services: vchiq_core: Stop kthreads on shutdown
2024-03-21 13:07 ` [PATCH v2 5/5] staging: vc04_services: vchiq_core: Stop kthreads on shutdown Umang Jain
2024-03-26 21:22 ` Stefan Wahren
2024-03-27 18:19 ` Stefan Wahren
@ 2024-03-28 14:53 ` Mark Brown
2024-03-28 15:29 ` Stefan Wahren
2 siblings, 1 reply; 18+ messages in thread
From: Mark Brown @ 2024-03-28 14:53 UTC (permalink / raw
To: Umang Jain
Cc: linux-staging, Stefan Wahren, Dan Carpenter, Kieran Bingham,
Laurent Pinchart, Dave Stevenson, Phil Elwell, Greg KH,
Umang Jain
[-- Attachment #1: Type: text/plain, Size: 9287 bytes --]
On Thu, Mar 21, 2024 at 06:37:37PM +0530, Umang Jain wrote:
> The various kthreads thread functions (slot_handler_func, sync_func,
> recycle_func) in vchiq_core and vchiq_keepalive_thread_func in
> vchiq_arm should be stopped on vchiq_shutdown().
>
> This also address the following TODO item:
> * Fix kernel module support
>
> hence drop it from the TODO item list.
This has landed in -next as d9c60badccc183eb971e0941bb86f9475d4b9551
and appears to be causing substantial issues on the original Raspberry
Pi. I'm seeing a lot of failed boots and one that worked is showing a
bunch of faults the first of which is:
[ 18.010575] bcm2835_vchiq 2000b840.mailbox: sync: error: 0: sf unexpected msgid 0@3776f2e5,0
[ 18.027151] ------------[ cut here ]------------
Setting prompt string to ['-+\\[ end trace \\w* \\]-+[^\\n]*\\r', '/ #', 'Login timed out', 'Login incorrect']
[ 18.035622] WARNING: CPU: 0 PID: 97 at lib/refcount.c:25 kthread_stop+0x60/0xc4
[ 18.046703] refcount_t: addition on 0; use-after-free.
[ 18.055566] Modules linked in: bcm2835_v4l2(C+) videobuf2_vmalloc videobuf2_memops bcm2835_mmal_vchiq(C) videobuf2_v4l2 videobuf2_common snd_bcm2835(C) raspberrypi_hwmon bcm2835_rng rng_core vchiq(C)
[ 18.084302] CPU: 0 PID: 97 Comm: vchiq-keep/0 Tainted: G C 6.9.0-rc1-next-20240328 #1
[ 18.100553] Hardware name: BCM2835
[ 18.107531] Call trace:
[ 18.107582] unwind_backtrace from show_stack+0x18/0x1c
[ 18.122480] show_stack from dump_stack_lvl+0x38/0x48
[ 18.131126] dump_stack_lvl from __warn+0x88/0xec
[ 18.139507] __warn from warn_slowpath_fmt+0x80/0xbc
[ 18.148071] warn_slowpath_fmt from kthread_stop+0x60/0xc4
[ 18.157210] kthread_stop from vchiq_shutdown_internal+0x48/0x94 [vchiq]
[ 18.167974] vchiq_shutdown_internal [vchiq] from vchiq_shutdown+0x38/0x98 [vchiq]
[ 18.179664] vchiq_shutdown [vchiq] from vchiq_keepalive_thread_func+0xb8/0x258 [vchiq]
[ 18.191793] vchiq_keepalive_thread_func [vchiq] from kthread+0xc4/0xd0
[ 18.202482] kthread from ret_from_fork+0x14/0x28
[ 18.210834] Exception stack(0xdc8a9fb0 to 0xdc8a9ff8)
[ 18.219579] 9fa0: 00000000 00000000 00000000 00000000
[ 18.234691] 9fc0: 00000000 00000000 00000000 00000000 00000000 00000000 00000000 00000000
[ 18.249717] 9fe0: 00000000 00000000 00000000 00000000 00000013 00000000
[ 18.260498] ---[ end trace 0000000000000000 ]---
A bisect landed on this commit which does look plausibe, a bisect log is
below (note that due to the way my CI runs this isn't actually the
bisect running, it's what happens when you feed the results from a
bisect into bisect on a separate machine but it does show the commits):
# bad: [a6bd6c9333397f5a0e2667d4d82fef8c970108f2] Add linux-next specific files for 20240328
# good: [1f6fc07c0c4d1e30d2bae9962205a2e74f6b00b2] Merge branch 'for-linux-next-fixes' of https://gitlab.freedesktop.org/drm/misc/kernel.git
# good: [846ad4e37aa837616354d335187c3e79b20e2f1b] Merge branch 'togreg' of git://git.kernel.org/pub/scm/linux/kernel/git/jic23/iio.git
# good: [08515f60ff706253d5f195f9983f6ec21fef7256] Merge branch 'for-linux-next' of https://gitlab.freedesktop.org/drm/misc/kernel.git
# good: [bbac19c294ebd1e2e55d5bc1130f6751a41c0d23] Merge branch 'for-next' of git://git.kernel.org/pub/scm/linux/kernel/git/broonie/spi.git
# good: [d5449432f794e75cd4f5e46bc33bfe6ce20b657d] spi: pxa2xx: Switch to use dev_err_probe()
# good: [7b95ee0db7e0a7f99077f1b926323c7bf0d2e8f8] ASoC: soc-jack: Get rid of legacy GPIO support
# good: [59ffeb15b2f7b44cf934fd778dc0d98a35aa6a84] ASoC: Intel: sof_sdw: Add support for cs42l43 optional speaker output
# good: [4ed0915f5bc4bcc81bca783a5b984f3d81e9764e] ASoC: codecs: Add RK3308 internal audio codec driver
# good: [ea5fee227ff3dae209062ac9544906debe1e9ac1] ASoC: hdac_hda: improve error logs
# good: [1e90a846493c716e3e6b4d901fc0844e9eea6430] ASoC: soc-dai: Note valid values of sysclock direction
# good: [61cafaeab5bca2d3e6a68ee8fa92b5c10b8610ca] ASoC: Intel: sof_rt5682: board id cleanup for cml boards
# good: [9b163e0d330debbf7dcc14b2c3e2dc19a3b50a1d] spi: remove struct spi_message::is_dma_mapped
# good: [b340f56a74b62d8ce8617650c8ab4a26c87ba5c5] ASoC: dt-bindings: wm8974: Convert to dtschema
# good: [bf1894900b53f9047eb9b96c89717a9a22329f6a] staging: vc04_services: Implement vchiq_bus .remove
# good: [f119f9e050940070c3da3a7476b1bf7a63c803c4] staging: rtl8712: rename tmpVal to avg_val
# good: [57c0b41bbe7b43c519f5824a964a406d1e7f60a3] staging: vc04_services: vchiq_arm: Use appropriate dev_* log helpers
# good: [bdeef5dcea6b164f4bd614655821b1ef12ebec9a] spi: rspi: Get rid of unused struct rspi_plat_data
# good: [885dd75f41f9fff5b277bc6ab28ad798f98a37b4] ASoC: dt-bindings: fsl-esai: Convert fsl,esai.txt to yaml
# good: [10402419f2d60890525f590b54d0eaec3de0d87a] spi: spi-mt65xx: Rename a variable in interrupt handler
# good: [9855f05e553637f05494cf47a3154cbf9a5cfc67] ASoC: fsl: imx-es8328: Switch to using gpiod API
# good: [5f39231888c63f0a7708abc86b51b847476379d8] ASoC: mediatek: Assign dummy when codec not specified for a DAI link
# good: [5edeb7d312628961046eec9b26a7e72f44baf846] regulator: pca9450: add pca9451a support
# good: [c14445bdcb98feddf9afaeb05e6193cc1f8eec1a] ASoC: fsl: imx-rpmsg: Update to correct DT node
# good: [cee28113db17f0de58df0eaea4e2756c404ee01f] ASoC: dmaengine_pcm: Allow passing component name via config
# good: [b5867a5c0d7a6bf36f59f3d472c7aed33ca4d02c] spi: pxa2xx: Use proper SSP header in soc/pxa/ssp.c
# good: [60c10c678b582d41532fefa12667d8adca75811b] ASoC: Intel: avs: i2s_test: Remove redundant dapm routes
# good: [aad6b35290f52639d3601063d33d9621c0948a04] regmap: maple: Remove second semicolon
# good: [a39111b1cf0864b1782f30f9a1fa65260d057327] spi: xilinx: Make num_chipselect 8-bit in the struct xspi_platform_data
# good: [e6913c6ef83c80aa7569c9e08204542222fbf542] ASoC: codecs: ES8326: Delete unused REG_SUPPLY
# good: [ab470abe58c09b2fbe2c1478e67a904fd803e84f] regulator: rpi-panel-attiny: convert to use maple tree register cache
# good: [0c5f77f4eaef8ed9fe752d21f40ac471dd511cfc] dt-bindings: regulator: qcom,usb-vbus-regulator: Add PM7250B compatible
# good: [21fa98f4197bb3365dda1417708b318f403c13c1] ASoC: sun8i-codec: Implement jack and accessory detection
git bisect start 'a6bd6c9333397f5a0e2667d4d82fef8c970108f2' '1f6fc07c0c4d1e30d2bae9962205a2e74f6b00b2' '846ad4e37aa837616354d335187c3e79b20e2f1b' '08515f60ff706253d5f195f9983f6ec21fef7256' 'bbac19c294ebd1e2e55d5bc1130f6751a41c0d23' 'd5449432f794e75cd4f5e46bc33bfe6ce20b657d' '7b95ee0db7e0a7f99077f1b926323c7bf0d2e8f8' '59ffeb15b2f7b44cf934fd778dc0d98a35aa6a84' '4ed0915f5bc4bcc81bca783a5b984f3d81e9764e' 'ea5fee227ff3dae209062ac9544906debe1e9ac1' '1e90a846493c716e3e6b4d901fc0844e9eea6430' '61cafaeab5bca2d3e6a68ee8fa92b5c10b8610ca' '9b163e0d330debbf7dcc14b2c3e2dc19a3b50a1d' 'b340f56a74b62d8ce8617650c8ab4a26c87ba5c5' 'bf1894900b53f9047eb9b96c89717a9a22329f6a' 'f119f9e050940070c3da3a7476b1bf7a63c803c4' '57c0b41bbe7b43c519f5824a964a406d1e7f60a3' 'bdeef5dcea6b164f4bd614655821b1ef12ebec9a' '885dd75f41f9fff5b277bc6ab28ad798f98a37b4' '10402419f2d60890525f590b54d0eaec3de0d87a' '9855f05e553637f05494cf47a3154cbf9a5cfc67' '5f39231888c63f0a7708abc86b51b847476379d8' '5edeb7d312628961046eec9b26a7e72f44baf846' 'c14445bdcb98feddf9afaeb05e6193cc1f8eec1a' 'cee28113db17f0de58df0eaea4e2756c404ee01f' 'b5867a5c0d7a6bf36f59f3d472c7aed33ca4d02c' '60c10c678b582d41532fefa12667d8adca75811b' 'aad6b35290f52639d3601063d33d9621c0948a04' 'a39111b1cf0864b1782f30f9a1fa65260d057327' 'e6913c6ef83c80aa7569c9e08204542222fbf542' 'ab470abe58c09b2fbe2c1478e67a904fd803e84f' '0c5f77f4eaef8ed9fe752d21f40ac471dd511cfc' '21fa98f4197bb3365dda1417708b318f403c13c1'
# bad: [a6bd6c9333397f5a0e2667d4d82fef8c970108f2] Add linux-next specific files for 20240328
git bisect bad a6bd6c9333397f5a0e2667d4d82fef8c970108f2
# bad: [86c322a4c2ba300ebd086f8f70cec07bb9ca7fce] Merge branch 'for-next' of git://git.kernel.org/pub/scm/linux/kernel/git/remoteproc/linux.git
git bisect bad 86c322a4c2ba300ebd086f8f70cec07bb9ca7fce
# bad: [acb95230d1324d9cc8de9bca38664b607e57fbe3] Merge branch 'counter-next' of git://git.kernel.org/pub/scm/linux/kernel/git/wbg/counter.git
git bisect bad acb95230d1324d9cc8de9bca38664b607e57fbe3
# bad: [20952655235dd9b1447829591774f1d8561f7c6a] staging: wlan-ng: Remove broken driver prism2_usb
git bisect bad 20952655235dd9b1447829591774f1d8561f7c6a
# bad: [a1ba19a1ae7cd1e324685ded4ab563e78fe68648] greybus: lights: check return of get_channel_from_mode
git bisect bad a1ba19a1ae7cd1e324685ded4ab563e78fe68648
# bad: [d9c60badccc183eb971e0941bb86f9475d4b9551] staging: vc04_services: vchiq_core: Stop kthreads on shutdown
git bisect bad d9c60badccc183eb971e0941bb86f9475d4b9551
# first bad commit: [d9c60badccc183eb971e0941bb86f9475d4b9551] staging: vc04_services: vchiq_core: Stop kthreads on shutdown
# bad: [d9c60badccc183eb971e0941bb86f9475d4b9551] staging: vc04_services: vchiq_core: Stop kthreads on shutdown
git bisect bad d9c60badccc183eb971e0941bb86f9475d4b9551
# first bad commit: [d9c60badccc183eb971e0941bb86f9475d4b9551] staging: vc04_services: vchiq_core: Stop kthreads on shutdown
[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]
^ permalink raw reply [flat|nested] 18+ messages in thread
* Re: [PATCH v2 5/5] staging: vc04_services: vchiq_core: Stop kthreads on shutdown
2024-03-28 14:53 ` Mark Brown
@ 2024-03-28 15:29 ` Stefan Wahren
0 siblings, 0 replies; 18+ messages in thread
From: Stefan Wahren @ 2024-03-28 15:29 UTC (permalink / raw
To: Mark Brown, Umang Jain
Cc: linux-staging, Dan Carpenter, Kieran Bingham, Laurent Pinchart,
Dave Stevenson, Phil Elwell, Greg KH
Hi Mark,
Am 28.03.24 um 15:53 schrieb Mark Brown:
> On Thu, Mar 21, 2024 at 06:37:37PM +0530, Umang Jain wrote:
>> The various kthreads thread functions (slot_handler_func, sync_func,
>> recycle_func) in vchiq_core and vchiq_keepalive_thread_func in
>> vchiq_arm should be stopped on vchiq_shutdown().
>>
>> This also address the following TODO item:
>> * Fix kernel module support
>>
>> hence drop it from the TODO item list.
>
> This has landed in -next as d9c60badccc183eb971e0941bb86f9475d4b9551
> and appears to be causing substantial issues on the original Raspberry
> Pi. I'm seeing a lot of failed boots and one that worked is showing a
> bunch of faults the first of which is:
>
yes this broken patch was applied before i to tested it with Linux
6.9rc1. The bugfix is currently under review [1]
[1] -
https://lore.kernel.org/linux-staging/171161507013.3072637.12125782507523919379@ping.linuxembedded.co.uk/T/#m1d3de7d2fa73b2447274858353bbd4a0c3a8ba14
^ permalink raw reply [flat|nested] 18+ messages in thread
end of thread, other threads:[~2024-03-28 15:30 UTC | newest]
Thread overview: 18+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2024-03-21 13:07 [PATCH v2 0/5] staging: vc04_services: vchiq_core: Stop kthreads on shutdown Umang Jain
2024-03-21 13:07 ` [PATCH v2 1/5] staging: vc04_services: Remove unused function declarations Umang Jain
2024-03-21 13:07 ` [PATCH v2 2/5] staging: vc04_services: vchiq_arm: Use appropriate dev_* log helpers Umang Jain
2024-03-21 13:07 ` [PATCH v2 3/5] staging: vc04_services: Do not log error on kzalloc() Umang Jain
2024-03-21 17:41 ` Kieran Bingham
2024-03-21 13:07 ` [PATCH v2 4/5] staging: vc04_services: Implement vchiq_bus .remove Umang Jain
2024-03-21 17:18 ` Kieran Bingham
2024-03-22 4:21 ` Umang Jain
2024-03-21 13:07 ` [PATCH v2 5/5] staging: vc04_services: vchiq_core: Stop kthreads on shutdown Umang Jain
2024-03-26 21:22 ` Stefan Wahren
2024-03-27 2:52 ` Umang Jain
2024-03-27 18:19 ` Stefan Wahren
2024-03-27 20:00 ` Umang Jain
2024-03-27 20:40 ` Stefan Wahren
2024-03-28 5:18 ` Umang Jain
2024-03-28 10:45 ` Dave Stevenson
2024-03-28 14:53 ` Mark Brown
2024-03-28 15:29 ` Stefan Wahren
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.