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

[Kieran Bingham <kieran.bingham@ideasonboard.com>]

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
>