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