[PATCH 0/5] staging: vc04_services: Address module cleanup

[PATCH 0/5] staging: vc04_services: Address module cleanup

[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:~$ 

```

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            | 18 ++++++++++--------
 .../interface/vchiq_arm/vchiq_bus.c            |  9 +++++++++
 .../interface/vchiq_arm/vchiq_core.c           | 10 +++++++---
 .../interface/vchiq_arm/vchiq_core.h           |  6 ------
 5 files changed, 26 insertions(+), 24 deletions(-)


base-commit: 68bb540b1aefded1d58a9f956568d5316643d291
-- 
2.43.0

[PATCH 1/5] staging: vc04_services: Remove unused function declarations

[Umang Jain]

vchiq_loud_error-* are unused 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>
---
 .../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

Re: [PATCH 1/5] staging: vc04_services: Remove unused function declarations

[Laurent Pinchart]

Hi Umang,

Thank you for the patch.

On Fri, Mar 15, 2024 at 04:26:55PM +0530, Umang Jain wrote:
> vchiq_loud_error-* are unused hence, remove their declarations.

I'd write "not implemented" instead of "unused".

> 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>

> ---
>  .../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);

-- 
Regards,

Laurent Pinchart

Re: [PATCH 1/5] staging: vc04_services: Remove unused function declarations

[Kieran Bingham]

Quoting Umang Jain (2024-03-15 10:56:55)
> vchiq_loud_error-* are unused hence, remove their declarations.
> This seem to be remnants of custom logging helpers which were
> removed earlier.
> 

Reviewed-by: Kieran Bingham <kieran.bingham@ideasonboard.com>

> Signed-off-by: Umang Jain <umang.jain@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
>

[PATCH 2/5] staging: vc04_services: vchiq_arm: Use appropriate dev_* log helpers

[Umang Jain]

Re-evaluate logs on error code paths and fix a few error logs
with  appropriate dev_* logging helpers.

For a case where a null ptr check is performed, use a WARN_ON()
instead of logging to dev_err().

No functional changes intended in this patch.

Signed-off-by: Umang Jain <umang.jain@ideasonboard.com>
---
 .../staging/vc04_services/interface/vchiq_arm/vchiq_arm.c | 8 ++++----
 1 file changed, 4 insertions(+), 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 1579bd4e5263..3c3e6f3e48ce 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;
 		}
@@ -1380,7 +1380,7 @@ vchiq_use_internal(struct vchiq_state *state, struct vchiq_service *service,
 			 service->client_id);
 		entity_uc = &service->service_use_count;
 	} else {
-		dev_err(state->dev, "suspend: %s: null service ptr\n", __func__);
+		WARN_ON(!service);
 		ret = -EINVAL;
 		goto out;
 	}
@@ -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

Re: [PATCH 2/5] staging: vc04_services: vchiq_arm: Use appropriate dev_* log helpers

[Kieran Bingham]

Quoting Umang Jain (2024-03-15 10:56:56)
> Re-evaluate logs on error code paths and fix a few error logs
> with  appropriate dev_* logging helpers.
> 
> For a case where a null ptr check is performed, use a WARN_ON()
> instead of logging to dev_err().
> 
> No functional changes intended in this patch.
> 
> Signed-off-by: Umang Jain <umang.jain@ideasonboard.com>
> ---
>  .../staging/vc04_services/interface/vchiq_arm/vchiq_arm.c | 8 ++++----
>  1 file changed, 4 insertions(+), 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 1579bd4e5263..3c3e6f3e48ce 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__);

This looks good.

>                         flush_signals(current);
>                         continue;
>                 }
> @@ -1380,7 +1380,7 @@ vchiq_use_internal(struct vchiq_state *state, struct vchiq_service *service,
>                          service->client_id);
>                 entity_uc = &service->service_use_count;
>         } else {
> -               dev_err(state->dev, "suspend: %s: null service ptr\n", __func__);
> +               WARN_ON(!service);

This sounds like something that shouldn't happen. Can it actually happen?

If it can happen - can it be caused by userspace through the VCHIQ
interfaces or is this just an internal code path?

Bumping up to a WARN_ON could probably need justification that deserves
it's own patch, but for the rest of the lines:


Reviewed-by: Kieran Bingham <kieran.bingham@ideasonboard.com>

>                 ret = -EINVAL;
>                 goto out;
>         }
> @@ -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
>

Re: [PATCH 2/5] staging: vc04_services: vchiq_arm: Use appropriate dev_* log helpers

[Umang Jain]

Hi Kieran,

On 16/03/24 3:58 pm, Kieran Bingham wrote:
> Quoting Umang Jain (2024-03-15 10:56:56)
>> Re-evaluate logs on error code paths and fix a few error logs
>> with  appropriate dev_* logging helpers.
>>
>> For a case where a null ptr check is performed, use a WARN_ON()
>> instead of logging to dev_err().
>>
>> No functional changes intended in this patch.
>>
>> Signed-off-by: Umang Jain <umang.jain@ideasonboard.com>
>> ---
>>   .../staging/vc04_services/interface/vchiq_arm/vchiq_arm.c | 8 ++++----
>>   1 file changed, 4 insertions(+), 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 1579bd4e5263..3c3e6f3e48ce 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__);
> This looks good.
>
>>                          flush_signals(current);
>>                          continue;
>>                  }
>> @@ -1380,7 +1380,7 @@ vchiq_use_internal(struct vchiq_state *state, struct vchiq_service *service,
>>                           service->client_id);
>>                  entity_uc = &service->service_use_count;
>>          } else {
>> -               dev_err(state->dev, "suspend: %s: null service ptr\n", __func__);
>> +               WARN_ON(!service);
> This sounds like something that shouldn't happen. Can it actually happen?
>
> If it can happen - can it be caused by userspace through the VCHIQ
> interfaces or is this just an internal code path?

Yes it can happen, when the char device /dev/vchiq is being closed by a 
process.  See vchiq_release()
calls the below call in 
drivers/staging/vc04_services/interface/vchiq_arm/vchiq_dev.c

....
vchiq_use_internal(instance->state, NULL, USE_TYPE_VCHIQ);
....

This was a review comment actually. I'll leave it out for this series 
and will handle it as standalone patch later.
>
> Bumping up to a WARN_ON could probably need justification that deserves
> it's own patch, but for the rest of the lines:

ack
>
>
> Reviewed-by: Kieran Bingham <kieran.bingham@ideasonboard.com>
>
>>                  ret = -EINVAL;
>>                  goto out;
>>          }
>> @@ -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
>>

[PATCH 3/5] staging: vc04_services: Do not log error on kzalloc()

[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>
---
 drivers/staging/vc04_services/interface/vchiq_arm/vchiq_arm.c | 2 --
 1 file changed, 2 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 3c3e6f3e48ce..1151cb4a84f9 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;
 	}
@@ -957,7 +956,6 @@ 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__);
 			return -ENOMEM;
 		}
 	}
-- 
2.43.0

Re: [PATCH 3/5] staging: vc04_services: Do not log error on kzalloc()

[Dan Carpenter]

On Fri, Mar 15, 2024 at 04:26:57PM +0530, Umang Jain wrote:
> 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 3c3e6f3e48ce..1151cb4a84f9 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;
>  	}
> @@ -957,7 +956,6 @@ 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__);
>  			return -ENOMEM;
>  		}

You need to remove the curly braces now as well.

regards,
dan carpenter

[PATCH 4/5] staging: vc04_services: Implement vchiq_bus .remove

[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        | 9 +++++++++
 1 file changed, 9 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..d9aa2f41ce13 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,20 @@ 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);
+
+	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

Re: [PATCH 4/5] staging: vc04_services: Implement vchiq_bus .remove

[Kieran Bingham]

Quoting Umang Jain (2024-03-15 10:56:58)
> 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        | 9 +++++++++
>  1 file changed, 9 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..d9aa2f41ce13 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,20 @@ 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);
> +
> +       driver->remove(device);

I suspect this be:

	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
>

[PATCH 5/5] staging: vc04_services: vchiq_core: Stop kthreads on shutdown

[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>
---
 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 1151cb4a84f9..60b4874049b4 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);
 
@@ -1311,7 +1315,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

Re: [PATCH 5/5] staging: vc04_services: vchiq_core: Stop kthreads on shutdown

[Kieran Bingham]

Quoting Umang Jain (2024-03-15 10:56:59)
> 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.


Reviewed-by: Kieran Bingham <kieran.bingham@ideasonboard.com>

> 
> Signed-off-by: Umang Jain <umang.jain@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 1151cb4a84f9..60b4874049b4 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);
>  
> @@ -1311,7 +1315,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
>

Re: [PATCH 0/5] staging: vc04_services: Address module cleanup

[Stefan Wahren]

Hi Umang,

Am 15.03.24 um 11:56 schrieb 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().

great work!

>
> 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:~$

What happens if you run "sudo modprobe bcm2835_mmal_vchiq" again?

Re: [PATCH 0/5] staging: vc04_services: Address module cleanup

[Umang Jain]

Hi Stefan,

On 17/03/24 5:38 pm, Stefan Wahren wrote:
> Hi Umang,
>
> Am 15.03.24 um 11:56 schrieb 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().
>
> great work!

:-)
>
>>
>> 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:~$
>
> What happens if you run "sudo modprobe bcm2835_mmal_vchiq" again?

Just seeing this right now - at bottom of the mail thread.

So trying to re-load the module fails :

[  207.665281] bcm2835_vchiq fe00b840.mailbox: failed to set channelbase 
(response: ffffffff)
[  207.674245] bcm2835_vchiq fe00b840.mailbox: arm: Could not initialize 
vchiq platform
[  223.527580] bcm2835_mmal_vchiq: module is from the staging directory, 
the quality is unknown, you.
[  223.539469] bcm2835_mmal_vchiq: unknown parameter 'vchiq' ignored

which is known as vchiq doesn't support re-connect.