[PATCH v3 4/4] staging: vc04_services: Drop vchiq_log_debug() in favour of dev_dbg
Stefan Wahren
wahrenst at gmx.net
Fri Dec 8 05:00:05 PST 2023
Hi Umang,
Am 05.12.23 um 09:41 schrieb Umang Jain:
> Drop vchiq_log_debug() macro which wraps dev_dbg(). Introduce the usage
> of dev_dbg() directly.
>
> Meanwhile at it, drop the usage of __func__ from the logs.
> Dynamic debug supports this via the 'f' decorator flag.
>
> Remove the entry from TODO regarding custom logging. VC04 is now
> aligned according to the standard kernel logging mechanisms.
personally i would have mentioned that this patch also drop the now
unused logging categories. Except of this i'm fine with the patch.
Possibled future improvements are mentioned below
>
> Signed-off-by: Umang Jain <umang.jain at ideasonboard.com>
> ---
> drivers/staging/vc04_services/interface/TODO | 5 -
> .../interface/vchiq_arm/vchiq_arm.c | 45 +++--
> .../interface/vchiq_arm/vchiq_core.c | 159 ++++++++----------
> .../interface/vchiq_arm/vchiq_core.h | 26 ---
> .../interface/vchiq_arm/vchiq_dev.c | 32 ++--
> 5 files changed, 106 insertions(+), 161 deletions(-)
>
> diff --git a/drivers/staging/vc04_services/interface/TODO b/drivers/staging/vc04_services/interface/TODO
> index 6d9d4a800aa7..05eb5140d096 100644
> --- a/drivers/staging/vc04_services/interface/TODO
> +++ b/drivers/staging/vc04_services/interface/TODO
> @@ -23,11 +23,6 @@ should properly handle a module unload. This also includes that all
> resources must be freed (kthreads, debugfs entries, ...) and global
> variables avoided.
>
> -* Cleanup logging mechanism
> -
> -The driver should probably be using the standard kernel logging mechanisms
> -such as dev_info, dev_dbg, and friends.
> -
> * 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 e337b8387647..4b4ff469d3a3 100644
> --- a/drivers/staging/vc04_services/interface/vchiq_arm/vchiq_arm.c
> +++ b/drivers/staging/vc04_services/interface/vchiq_arm/vchiq_arm.c
> @@ -310,9 +310,8 @@ create_pagelist(struct vchiq_instance *instance, char *buf, char __user *ubuf,
> type == PAGELIST_READ, pages);
>
> if (actual_pages != num_pages) {
> - vchiq_log_debug(instance->state->dev, VCHIQ_ARM,
> - "%s - only %d/%d pages locked",
> - __func__, actual_pages, num_pages);
> + dev_dbg(instance->state->dev, "arm: Only %d/%d pages locked\n",
> + actual_pages, num_pages);
>
> /* This is probably due to the process being killed */
> if (actual_pages > 0)
> @@ -554,8 +553,8 @@ static int vchiq_platform_init(struct platform_device *pdev, struct vchiq_state
> return -ENXIO;
> }
>
> - vchiq_log_debug(&pdev->dev, VCHIQ_ARM, "vchiq_init - done (slots %pK, phys %pad)",
> - vchiq_slot_zero, &slot_phys);
> + dev_dbg(&pdev->dev, "arm: vchiq_init - done (slots %pK, phys %pad)\n",
> + vchiq_slot_zero, &slot_phys);
>
> vchiq_call_connected_callbacks();
>
> @@ -719,9 +718,9 @@ void free_bulk_waiter(struct vchiq_instance *instance)
> list_for_each_entry_safe(waiter, next,
> &instance->bulk_waiter_list, list) {
> list_del(&waiter->list);
> - vchiq_log_debug(instance->state->dev, VCHIQ_ARM,
> - "bulk_waiter - cleaned up %pK for pid %d",
> - waiter, waiter->pid);
> + dev_dbg(instance->state->dev,
> + "arm: bulk_waiter - cleaned up %pK for pid %d\n",
> + waiter, waiter->pid);
> kfree(waiter);
> }
> }
> @@ -981,9 +980,8 @@ vchiq_blocking_bulk_transfer(struct vchiq_instance *instance, unsigned int handl
> mutex_lock(&instance->bulk_waiter_list_mutex);
> list_add(&waiter->list, &instance->bulk_waiter_list);
> mutex_unlock(&instance->bulk_waiter_list_mutex);
> - vchiq_log_debug(instance->state->dev, VCHIQ_ARM,
> - "saved bulk_waiter %pK for pid %d", waiter,
> - current->pid);
> + dev_dbg(instance->state->dev, "arm: saved bulk_waiter %pK for pid %d\n",
> + waiter, current->pid);
> }
>
> return status;
> @@ -1006,12 +1004,10 @@ add_completion(struct vchiq_instance *instance, enum vchiq_reason reason,
> dev_dbg(instance->state->dev, "core: completion queue full\n");
> DEBUG_COUNT(COMPLETION_QUEUE_FULL_COUNT);
> if (wait_for_completion_interruptible(&instance->remove_event)) {
> - vchiq_log_debug(instance->state->dev, VCHIQ_ARM,
> - "service_callback interrupted");
> + dev_dbg(instance->state->dev, "arm: service_callback interrupted\n");
> return -EAGAIN;
> } else if (instance->closing) {
> - vchiq_log_debug(instance->state->dev, VCHIQ_ARM,
> - "service_callback closing");
> + dev_dbg(instance->state->dev, "arm: service_callback closing\n");
> return 0;
> }
> DEBUG_TRACE(SERVICE_CALLBACK_LINE);
> @@ -1113,8 +1109,8 @@ service_callback(struct vchiq_instance *instance, enum vchiq_reason reason,
> instance->completion_remove) < 0) {
> int status;
>
> - vchiq_log_debug(instance->state->dev, VCHIQ_ARM,
> - "Inserting extra MESSAGE_AVAILABLE");
> + dev_dbg(instance->state->dev,
> + "arm: Inserting extra MESSAGE_AVAILABLE\n");
> DEBUG_TRACE(SERVICE_CALLBACK_LINE);
> status = add_completion(instance, reason, NULL, user_service,
> bulk_userdata);
> @@ -1127,14 +1123,12 @@ service_callback(struct vchiq_instance *instance, enum vchiq_reason reason,
>
> DEBUG_TRACE(SERVICE_CALLBACK_LINE);
> if (wait_for_completion_interruptible(&user_service->remove_event)) {
> - vchiq_log_debug(instance->state->dev, VCHIQ_ARM,
> - "%s interrupted", __func__);
> + dev_dbg(instance->state->dev, "arm: interrupted\n");
> DEBUG_TRACE(SERVICE_CALLBACK_LINE);
> vchiq_service_put(service);
> return -EAGAIN;
> } else if (instance->closing) {
> - vchiq_log_debug(instance->state->dev, VCHIQ_ARM,
> - "%s closing", __func__);
> + dev_dbg(instance->state->dev, "arm: closing\n");
> DEBUG_TRACE(SERVICE_CALLBACK_LINE);
> vchiq_service_put(service);
> return -EINVAL;
> @@ -1686,8 +1680,8 @@ void vchiq_platform_conn_state_changed(struct vchiq_state *state,
> struct vchiq_arm_state *arm_state = vchiq_platform_get_arm_state(state);
> char threadname[16];
>
> - vchiq_log_debug(state->dev, VCHIQ_SUSPEND, "%d: %s->%s", state->id,
> - get_conn_state_name(oldstate), get_conn_state_name(newstate));
> + dev_dbg(state->dev, "suspend: %d: %s->%s\n",
> + state->id, get_conn_state_name(oldstate), get_conn_state_name(newstate));
> if (state->conn_state != VCHIQ_CONNSTATE_CONNECTED)
> return;
>
> @@ -1751,9 +1745,8 @@ static int vchiq_probe(struct platform_device *pdev)
>
> vchiq_debugfs_init();
>
> - vchiq_log_debug(&pdev->dev, VCHIQ_ARM,
> - "vchiq: platform initialised - version %d (min %d)",
> - VCHIQ_VERSION, VCHIQ_VERSION_MIN);
> + dev_dbg(&pdev->dev, "arm: platform initialised - version %d (min %d)\n",
> + VCHIQ_VERSION, VCHIQ_VERSION_MIN);
>
> /*
> * Simply exit on error since the function handles cleanup in
> 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 d197e4504bd4..76c27778154a 100644
> --- a/drivers/staging/vc04_services/interface/vchiq_arm/vchiq_core.c
> +++ b/drivers/staging/vc04_services/interface/vchiq_arm/vchiq_core.c
> @@ -217,10 +217,10 @@ static const char *msg_type_str(unsigned int msg_type)
> static inline void
> set_service_state(struct vchiq_service *service, int newstate)
> {
> - vchiq_log_debug(service->state->dev, VCHIQ_CORE, "%d: srv:%d %s->%s",
> - service->state->id, service->localport,
> - srvstate_names[service->srvstate],
> - srvstate_names[newstate]);
> + dev_dbg(service->state->dev, "core: %d: srv:%d %s->%s\n",
> + service->state->id, service->localport,
> + srvstate_names[service->srvstate],
> + srvstate_names[newstate]);
> service->srvstate = newstate;
> }
>
> @@ -245,8 +245,7 @@ find_service_by_handle(struct vchiq_instance *instance, unsigned int handle)
> return service;
> }
> rcu_read_unlock();
> - vchiq_log_debug(instance->state->dev, VCHIQ_CORE,
> - "Invalid service handle 0x%x", handle);
> + dev_dbg(instance->state->dev, "core: Invalid service handle 0x%x\n", handle);
From my PoV this is a real error, so maybe a WARN_ONCE. But looking
deeper at this shows that the error handling must be reworked first :-(
More information about the linux-arm-kernel
mailing list