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@xxxxxxxxxxxxxxxx>
---
.../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@xxxxxxxxxxxxxxxx>
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
