Re: [PATCH v3 4/4] staging: vc04_services: Drop vchiq_log_debug() in favour of dev_dbg

Stefan Wahren <wahrenst@xxxxxxx> · Fri, 8 Dec 2023 14:00:05 +0100

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@xxxxxxxxxxxxxxxx>
---
  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 :-(








