Re: [PATCH v2] staging: vchiq_arm: use standard print helpers

[Greg Kroah-Hartman <gregkh@linuxfoundation.org>]

On Thu, Apr 28, 2022 at 11:05:49AM -0400, Adrien Thierry wrote:
> Replace the custom debug print macros with the standard dev_err() and
> friends.
> 
> This handles TODO item "Cleanup logging mechanism".
> 
> Signed-off-by: Adrien Thierry <athierry@redhat.com>
> ---
> 
> Changes since v1: removed function name in dev_dbg() calls
> 
>  .../interface/vchiq_arm/vchiq_arm.c           | 157 +++---
>  .../interface/vchiq_arm/vchiq_connected.c     |   7 +-
>  .../interface/vchiq_arm/vchiq_connected.h     |   4 +-
>  .../interface/vchiq_arm/vchiq_core.c          | 495 ++++++++----------
>  .../interface/vchiq_arm/vchiq_core.h          |  43 +-
>  .../interface/vchiq_arm/vchiq_debugfs.c       | 105 ----
>  .../interface/vchiq_arm/vchiq_dev.c           |  86 ++-
>  7 files changed, 341 insertions(+), 556 deletions(-)

Try doing this in smaller chunks.  There's a lot of churn here, and not
all of it is correct.

Try removing these one-function-at-a-time and then when it's all
finished, you can remove the debugfs and function calls as no one is
calling them.

That way it's also easier to review, as-is, this is a tough review.
Would you want to review this at once?

A few odd things that jumped out at me:

> @@ -1332,6 +1325,8 @@ vchiq_keepalive_thread_func(void *v)
>  	struct vchiq_state *state = (struct vchiq_state *)v;
>  	struct vchiq_arm_state *arm_state = vchiq_platform_get_arm_state(state);
>  
> +	struct device *dev = state->dev;
> +

Checkpatch should have warned you about the extra blank line here.  Put
all variables one after each other please.

> --- a/drivers/staging/vc04_services/interface/vchiq_arm/vchiq_connected.c
> +++ b/drivers/staging/vc04_services/interface/vchiq_arm/vchiq_connected.c
> @@ -27,7 +27,7 @@ static void connected_init(void)
>   * be made immediately, otherwise it will be deferred until
>   * vchiq_call_connected_callbacks is called.
>   */
> -void vchiq_add_connected_callback(void (*callback)(void))
> +void vchiq_add_connected_callback(struct device *dev, void (*callback)(void))

Pass in the real vchiq device pointer, not a struct device.

> --- a/drivers/staging/vc04_services/interface/vchiq_arm/vchiq_connected.h
> +++ b/drivers/staging/vc04_services/interface/vchiq_arm/vchiq_connected.h
> @@ -4,7 +4,9 @@
>  #ifndef VCHIQ_CONNECTED_H
>  #define VCHIQ_CONNECTED_H
>  
> -void vchiq_add_connected_callback(void (*callback)(void));
> +#include <linux/device.h>

Don't include the .h file here, it shouldn't be needed if you make this
the same real device type.

>  
>  struct vchiq_state {
> +	struct device *dev;

Careful now, have you properly handled the reference counting?  I can't
tell so you should do this type of change on it's own to make it obvious
you handle it properly.

> --- a/drivers/staging/vc04_services/interface/vchiq_arm/vchiq_dev.c
> +++ b/drivers/staging/vc04_services/interface/vchiq_arm/vchiq_dev.c
> @@ -37,6 +37,8 @@ static const char *const ioctl_names[] = {
>  	"CLOSE_DELIVERED"
>  };
>  
> +static struct miscdevice vchiq_miscdev;

That looks really odd.  If you create this, where are you initializing
it?  Did you test this code?

thanks,

greg k-h