diff mbox series

[v3,1/4] staging: vc04_services: Drop vchiq_log_error() in favour of dev_err

Message ID 20231205084157.73819-2-umang.jain@ideasonboard.com
State Accepted
Commit e70f17ed997cb7ee6c34089f2cdc2a9edc886503
Headers show
Series staging: vc04_services: Drop custom logging | expand

Commit Message

Umang Jain Dec. 5, 2023, 8:41 a.m. UTC
Drop vchiq_log_error() macro which wraps dev_dbg(). Introduce the usage
of dev_err() directly.

Signed-off-by: Umang Jain <umang.jain@ideasonboard.com>
---
 .../interface/vchiq_arm/vchiq_arm.c           |  50 ++++----
 .../interface/vchiq_arm/vchiq_connected.c     |   6 +-
 .../interface/vchiq_arm/vchiq_core.c          | 113 +++++++++---------
 .../interface/vchiq_arm/vchiq_core.h          |   4 -
 .../interface/vchiq_arm/vchiq_dev.c           |  45 ++++---
 5 files changed, 101 insertions(+), 117 deletions(-)

Comments

Stefan Wahren Dec. 5, 2023, 7:32 p.m. UTC | #1
Hi Umang,

Am 05.12.23 um 09:41 schrieb Umang Jain:
> Drop vchiq_log_error() macro which wraps dev_dbg(). Introduce the usage
> of dev_err() directly.
sorry, i missed to review the last change. So the change won't be that
trivial.
>
> Signed-off-by: Umang Jain <umang.jain@ideasonboard.com>
> ---
>   .../interface/vchiq_arm/vchiq_arm.c           |  50 ++++----
>   .../interface/vchiq_arm/vchiq_connected.c     |   6 +-
>   .../interface/vchiq_arm/vchiq_core.c          | 113 +++++++++---------
>   .../interface/vchiq_arm/vchiq_core.h          |   4 -
>   .../interface/vchiq_arm/vchiq_dev.c           |  45 ++++---
>   5 files changed, 101 insertions(+), 117 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 7978fb6dc4fb..b403400edd8e 100644
> --- a/drivers/staging/vc04_services/interface/vchiq_arm/vchiq_arm.c
> +++ b/drivers/staging/vc04_services/interface/vchiq_arm/vchiq_arm.c
> @@ -683,8 +683,7 @@ int vchiq_initialise(struct vchiq_instance **instance_out)
>   		usleep_range(500, 600);
>   	}
>   	if (i == VCHIQ_INIT_RETRIES) {
> -		vchiq_log_error(state->dev, VCHIQ_CORE, "%s: videocore not initialized\n",
> -				__func__);
> +		dev_err(state->dev, "core: %s: Videocore not initialized\n", __func__);
>   		ret = -ENOTCONN;
>   		goto failed;
>   	} else if (i > 0) {
> @@ -694,8 +693,7 @@ int vchiq_initialise(struct vchiq_instance **instance_out)
>
>   	instance = kzalloc(sizeof(*instance), GFP_KERNEL);
>   	if (!instance) {
> -		vchiq_log_error(state->dev, VCHIQ_CORE,
> -				"%s: error allocating vchiq instance\n", __func__);
> +		dev_err(state->dev, "core: %s: Cannot allocate vchiq instance\n", __func__);
please drop because kzalloc already report such errors
>   		ret = -ENOMEM;
>   		goto failed;
>   	}
> @@ -967,8 +965,7 @@ vchiq_blocking_bulk_transfer(struct vchiq_instance *instance, unsigned int handl
>   	} else {
>   		waiter = kzalloc(sizeof(*waiter), GFP_KERNEL);
>   		if (!waiter) {
> -			vchiq_log_error(service->state->dev, VCHIQ_CORE,
> -					"%s - out of memory", __func__);
> +			dev_err(service->state->dev, "core: %s: - Out of memory\n", __func__);
ditto
>   			return -ENOMEM;
>   		}
>   	}
> @@ -1290,8 +1287,8 @@ vchiq_keepalive_vchiq_callback(struct vchiq_instance *instance,
>   			       struct vchiq_header *header,
>   			       unsigned int service_user, void *bulk_user)
>   {
> -	vchiq_log_error(instance->state->dev, VCHIQ_SUSPEND,
> -			"%s callback reason %d", __func__, reason);
> +	dev_err(instance->state->dev, "suspend: %s: callback reason %d\n",
> +		__func__, reason);
>   	return 0;
>   }
>
> @@ -1315,22 +1312,20 @@ vchiq_keepalive_thread_func(void *v)
>
>   	ret = vchiq_initialise(&instance);
>   	if (ret) {
> -		vchiq_log_error(state->dev, VCHIQ_SUSPEND,
> -				"%s vchiq_initialise failed %d", __func__, ret);
> +		dev_err(state->dev, "suspend: %s: vchiq_initialise failed %d\n", __func__, ret);
>   		goto exit;
>   	}
>
>   	status = vchiq_connect(instance);
>   	if (status) {
> -		vchiq_log_error(state->dev, VCHIQ_SUSPEND,
> -				"%s vchiq_connect failed %d", __func__, status);
> +		dev_err(state->dev, "suspend: %s: vchiq_connect failed %d\n", __func__, status);
>   		goto shutdown;
>   	}
>
>   	status = vchiq_add_service(instance, &params, &ka_handle);
>   	if (status) {
> -		vchiq_log_error(state->dev, VCHIQ_SUSPEND,
> -				"%s vchiq_open_service failed %d", __func__, status);
> +		dev_err(state->dev, "suspend: %s: vchiq_open_service failed %d\n",
> +			__func__, status);
>   		goto shutdown;
>   	}
>
> @@ -1338,8 +1333,7 @@ vchiq_keepalive_thread_func(void *v)
>   		long rc = 0, uc = 0;
>
>   		if (wait_for_completion_interruptible(&arm_state->ka_evt)) {
> -			vchiq_log_error(state->dev, VCHIQ_SUSPEND,
> -					"%s interrupted", __func__);
> +			dev_err(state->dev, "suspend: %s: interrupted\n", __func__);
I think this isn't an error and we should use dev_dbg here.
>   			flush_signals(current);
>   			continue;
>   		}
> @@ -1359,16 +1353,15 @@ vchiq_keepalive_thread_func(void *v)
>   			atomic_inc(&arm_state->ka_use_ack_count);
>   			status = vchiq_use_service(instance, ka_handle);
>   			if (status) {
> -				vchiq_log_error(state->dev, VCHIQ_SUSPEND,
> -						"%s vchiq_use_service error %d", __func__, status);
> +				dev_err(state->dev, "suspend: %s: vchiq_use_service error %d\n",
> +					__func__, status);
>   			}
>   		}
>   		while (rc--) {
>   			status = vchiq_release_service(instance, ka_handle);
>   			if (status) {
> -				vchiq_log_error(state->dev, VCHIQ_SUSPEND,
> -						"%s vchiq_release_service error %d", __func__,
> -						status);
> +				dev_err(state->dev, "suspend: %s: vchiq_release_service error %d\n",
> +					__func__, status);
>   			}
>   		}
>   	}
> @@ -1403,7 +1396,7 @@ vchiq_use_internal(struct vchiq_state *state, struct vchiq_service *service,
>   			 service->client_id);
>   		entity_uc = &service->service_use_count;
>   	} else {
> -		vchiq_log_error(state->dev, VCHIQ_SUSPEND, "%s null service ptr", __func__);
> +		dev_err(state->dev, "suspend: %s: null service ptr\n", __func__);
Instead of dev_err() i would suggest a WARN() here
>   		ret = -EINVAL;
>   		goto out;
>   	}
> @@ -1686,10 +1679,10 @@ vchiq_check_service(struct vchiq_service *service)
>   	read_unlock_bh(&arm_state->susp_res_lock);
>
>   	if (ret) {
> -		vchiq_log_error(service->state->dev, VCHIQ_SUSPEND,
> -				"%s ERROR - %p4cc:%d service count %d, state count %d", __func__,
> -				&service->base.fourcc, service->client_id,
> -				service->service_use_count, arm_state->videocore_use_count);
> +		dev_err(service->state->dev,
> +			"suspend: %s:  %p4cc:%d service count %d, state count %d\n",
> +			__func__, &service->base.fourcc, service->client_id,
> +			service->service_use_count, arm_state->videocore_use_count);
>   		vchiq_dump_service_use_state(service->state);
>   	}
>   out:
> @@ -1722,9 +1715,8 @@ void vchiq_platform_conn_state_changed(struct vchiq_state *state,
>   					      (void *)state,
>   					      threadname);
>   	if (IS_ERR(arm_state->ka_thread)) {
> -		vchiq_log_error(state->dev, VCHIQ_SUSPEND,
> -				"vchiq: FATAL: couldn't create thread %s",
> -				threadname);
> +		dev_err(state->dev, "suspend: Couldn't create thread %s\n",
> +			threadname);
>   	} else {
>   		wake_up_process(arm_state->ka_thread);
>   	}
> diff --git a/drivers/staging/vc04_services/interface/vchiq_arm/vchiq_connected.c b/drivers/staging/vc04_services/interface/vchiq_arm/vchiq_connected.c
> index 21f9fa1a1713..3cad13f09e37 100644
> --- a/drivers/staging/vc04_services/interface/vchiq_arm/vchiq_connected.c
> +++ b/drivers/staging/vc04_services/interface/vchiq_arm/vchiq_connected.c
> @@ -39,9 +39,9 @@ void vchiq_add_connected_callback(struct vchiq_device *device, void (*callback)(
>   		callback();
>   	} else {
>   		if (g_num_deferred_callbacks >= MAX_CALLBACKS) {
> -			vchiq_log_error(&device->dev, VCHIQ_CORE,
> -					"There already %d callback registered - please increase MAX_CALLBACKS",
> -					g_num_deferred_callbacks);
> +			dev_err(&device->dev,
> +				"core: There already %d callback registered - please increase MAX_CALLBACKS\n",
> +				g_num_deferred_callbacks);
>   		} else {
>   			g_deferred_callback[g_num_deferred_callbacks] =
>   				callback;
> 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 e0022acb4c58..63f412815a32 100644
> --- a/drivers/staging/vc04_services/interface/vchiq_arm/vchiq_core.c
> +++ b/drivers/staging/vc04_services/interface/vchiq_arm/vchiq_core.c
> @@ -741,10 +741,10 @@ process_free_data_message(struct vchiq_state *state, u32 *service_found,
>   		 */
>   		complete(&quota->quota_event);
>   	} else if (count == 0) {
> -		vchiq_log_error(state->dev, VCHIQ_CORE,
> -				"service %d message_use_count=%d (header %pK, msgid %x, header->msgid %x, header->size %x)",
> -				port, quota->message_use_count, header, msgid, header->msgid,
> -				header->size);
> +		dev_err(state->dev,
> +			"core: service %d message_use_count=%d (header %pK, msgid %x, header->msgid %x, header->size %x)\n",
> +			port, quota->message_use_count, header, msgid,
> +			header->msgid, header->size);
>   		WARN(1, "invalid message use count\n");
>   	}
>   	if (!BITSET_IS_SET(service_found, port)) {
> @@ -766,9 +766,9 @@ process_free_data_message(struct vchiq_state *state, u32 *service_found,
>   			vchiq_log_trace(state->dev, VCHIQ_CORE, "%d: pfq:%d %x@%pK - slot_use->%d",
>   					state->id, port, header->size, header, count - 1);
>   		} else {
> -			vchiq_log_error(state->dev, VCHIQ_CORE,
> -					"service %d slot_use_count=%d (header %pK, msgid %x, header->msgid %x, header->size %x)",
> -					port, count, header, msgid, header->msgid, header->size);
> +			dev_err(state->dev,
> +				"core: service %d slot_use_count=%d (header %pK, msgid %x, header->msgid %x, header->size %x)\n",
> +				port, count, header, msgid, header->msgid, header->size);
>   			WARN(1, "bad slot use count\n");
>   		}
>   	}
> @@ -831,9 +831,9 @@ process_free_queue(struct vchiq_state *state, u32 *service_found,
>
>   			pos += calc_stride(header->size);
>   			if (pos > VCHIQ_SLOT_SIZE) {
> -				vchiq_log_error(state->dev, VCHIQ_CORE,
> -						"pfq - pos %x: header %pK, msgid %x, header->msgid %x, header->size %x",
> -						pos, header, msgid, header->msgid, header->size);
> +				dev_err(state->dev,
> +					"core: pfq - pos %x: header %pK, msgid %x, header->msgid %x, header->size %x\n",
> +					pos, header, msgid, header->msgid, header->size);
>   				WARN(1, "invalid slot position\n");
>   			}
>   		}
> @@ -1167,8 +1167,8 @@ queue_message_sync(struct vchiq_state *state, struct vchiq_service *service,
>   		int oldmsgid = header->msgid;
>
>   		if (oldmsgid != VCHIQ_MSGID_PADDING)
> -			vchiq_log_error(state->dev, VCHIQ_CORE, "%d: qms - msgid %x, not PADDING",
> -					state->id, oldmsgid);
> +			dev_err(state->dev, "core: %d: qms - msgid %x, not PADDING\n",
> +				state->id, oldmsgid);
>   	}
>
>   	vchiq_log_debug(state->dev, VCHIQ_SYNC,
> @@ -1616,10 +1616,10 @@ parse_message(struct vchiq_state *state, struct vchiq_header *header)
>   		}
>
>   		if (!service) {
> -			vchiq_log_error(state->dev, VCHIQ_CORE,
> -					"%d: prs %s@%pK (%d->%d) - invalid/closed service %d",
> -					state->id, msg_type_str(type), header, remoteport,
> -					localport, localport);
> +			dev_err(state->dev,
> +				"core: %d: prs %s@%pK (%d->%d) - invalid/closed service %d\n",
> +				state->id, msg_type_str(type), header, remoteport,
> +				localport, localport);
>   			goto skip_message;
>   		}
>   		break;
> @@ -1640,9 +1640,8 @@ parse_message(struct vchiq_state *state, struct vchiq_header *header)
>
>   	if (((unsigned long)header & VCHIQ_SLOT_MASK) +
>   	    calc_stride(size) > VCHIQ_SLOT_SIZE) {
> -		vchiq_log_error(state->dev, VCHIQ_CORE,
> -				"header %pK (msgid %x) - size %x too big for slot",
> -				header, (unsigned int)msgid, (unsigned int)size);
> +		dev_err(state->dev, "core: header %pK (msgid %x) - size %x too big for slot\n",
> +			header, (unsigned int)msgid, (unsigned int)size);
>   		WARN(1, "oversized for slot\n");
>   	}
>
> @@ -1668,8 +1667,8 @@ parse_message(struct vchiq_state *state, struct vchiq_header *header)
>   			set_service_state(service, VCHIQ_SRVSTATE_OPEN);
>   			complete(&service->remove_event);
>   		} else {
> -			vchiq_log_error(state->dev, VCHIQ_CORE, "OPENACK received in state %s",
> -					srvstate_names[service->srvstate]);
> +			dev_err(state->dev, "core: OPENACK received in state %s\n",
> +				srvstate_names[service->srvstate]);
>   		}
>   		break;
>   	case VCHIQ_MSG_CLOSE:
> @@ -1740,11 +1739,10 @@ parse_message(struct vchiq_state *state, struct vchiq_header *header)
>   			}
>   			if ((int)(queue->remote_insert -
>   				queue->local_insert) >= 0) {
> -				vchiq_log_error(state->dev, VCHIQ_CORE,
> -						"%d: prs %s@%pK (%d->%d) unexpected (ri=%d,li=%d)",
> -						state->id, msg_type_str(type), header, remoteport,
> -						localport, queue->remote_insert,
> -						queue->local_insert);
> +				dev_err(state->dev,
> +					"core: %d: prs %s@%pK (%d->%d) unexpected (ri=%d,li=%d)\n",
> +					state->id, msg_type_str(type), header, remoteport,
> +					localport, queue->remote_insert, queue->local_insert);
>   				mutex_unlock(&service->bulk_mutex);
>   				break;
>   			}
> @@ -1790,8 +1788,8 @@ parse_message(struct vchiq_state *state, struct vchiq_header *header)
>   		vchiq_log_trace(state->dev, VCHIQ_CORE, "%d: prs PAUSE@%pK,%x",
>   				state->id, header, size);
>   		if (state->conn_state == VCHIQ_CONNSTATE_PAUSED) {
> -			vchiq_log_error(state->dev, VCHIQ_CORE,
> -					"%d: PAUSE received in state PAUSED", state->id);
> +			dev_err(state->dev, "core: %d: PAUSE received in state PAUSED\n",
> +				state->id);
>   			break;
>   		}
>   		if (state->conn_state != VCHIQ_CONNSTATE_PAUSE_SENT) {
> @@ -1821,8 +1819,8 @@ parse_message(struct vchiq_state *state, struct vchiq_header *header)
>   		break;
>
>   	default:
> -		vchiq_log_error(state->dev, VCHIQ_CORE, "%d: prs invalid msgid %x@%pK,%x",
> -				state->id, msgid, header, size);
> +		dev_err(state->dev, "core: %d: prs invalid msgid %x@%pK,%x\n",
> +			state->id, msgid, header, size);
>   		WARN(1, "invalid message\n");
>   		break;
>   	}
> @@ -1932,7 +1930,7 @@ handle_poll(struct vchiq_state *state)
>   			 * since the PAUSE should have flushed
>   			 * through outstanding messages.
>   			 */
> -			vchiq_log_error(state->dev, VCHIQ_CORE, "Failed to send RESUME message");
> +			dev_err(state->dev, "core: Failed to send RESUME message\n");
>   		}
>   		break;
>   	default:
> @@ -2032,10 +2030,10 @@ sync_func(void *v)
>   		service = find_service_by_port(state, localport);
>
>   		if (!service) {
> -			vchiq_log_error(state->dev, VCHIQ_SYNC,
> -					"%d: sf %s@%pK (%d->%d) - invalid/closed service %d",
> -					state->id, msg_type_str(type), header,
> -					remoteport, localport, localport);
> +			dev_err(state->dev,
> +				"sync: %d: sf %s@%pK (%d->%d) - invalid/closed service %d\n",
> +				state->id, msg_type_str(type), header, remoteport,
> +				localport, localport);
>   			release_message_sync(state, header);
>   			continue;
>   		}
> @@ -2077,15 +2075,15 @@ sync_func(void *v)
>   			    (service->srvstate == VCHIQ_SRVSTATE_OPENSYNC)) {
>   				if (make_service_callback(service, VCHIQ_MESSAGE_AVAILABLE, header,
>   							  NULL) == -EAGAIN)
> -					vchiq_log_error(state->dev, VCHIQ_SYNC,
> -							"synchronous callback to service %d returns -EAGAIN",
> -							localport);
> +					dev_err(state->dev,
> +						"sync: error: synchronous callback to service %d returns -EAGAIN\n",
> +						localport);
>   			}
>   			break;
>
>   		default:
> -			vchiq_log_error(state->dev, VCHIQ_SYNC, "%d: sf unexpected msgid %x@%pK,%x",
> -					state->id, msgid, header, size);
> +			dev_err(state->dev, "sync: error: %d: sf unexpected msgid %x@%pK,%x\n",
> +				state->id, msgid, header, size);
>   			release_message_sync(state, header);
>   			break;
>   		}
> @@ -2118,8 +2116,8 @@ vchiq_init_slots(struct device *dev, void *mem_base, int mem_size)
>   	num_slots -= first_data_slot;
>
>   	if (num_slots < 4) {
> -		vchiq_log_error(dev, VCHIQ_CORE, "%s - insufficient memory %x bytes",
> -				__func__, mem_size);
> +		dev_err(dev, "core: %s: Insufficient memory %x bytes\n",
> +			__func__, mem_size);
>   		return NULL;
>   	}
>
> @@ -2500,11 +2498,10 @@ vchiq_open_service_internal(struct vchiq_service *service, int client_id)
>   	} else if ((service->srvstate != VCHIQ_SRVSTATE_OPEN) &&
>   		   (service->srvstate != VCHIQ_SRVSTATE_OPENSYNC)) {
>   		if (service->srvstate != VCHIQ_SRVSTATE_CLOSEWAIT)
> -			vchiq_log_error(service->state->dev, VCHIQ_CORE,
> -					"%d: osi - srvstate = %s (ref %u)",
> -					service->state->id,
> -					srvstate_names[service->srvstate],
> -					kref_read(&service->ref_count));
> +			dev_err(service->state->dev,
> +				"core: %d: osi - srvstate = %s (ref %u)\n",
> +				service->state->id, srvstate_names[service->srvstate],
> +				kref_read(&service->ref_count));
>   		status = -EINVAL;
>   		VCHIQ_SERVICE_STATS_INC(service, error_count);
>   		vchiq_release_service_internal(service);
> @@ -2565,9 +2562,9 @@ release_service_messages(struct vchiq_service *service)
>   			}
>   			pos += calc_stride(header->size);
>   			if (pos > VCHIQ_SLOT_SIZE) {
> -				vchiq_log_error(state->dev, VCHIQ_CORE,
> -						"fsi - pos %x: header %pK, msgid %x, header->msgid %x, header->size %x",
> -						pos, header, msgid, header->msgid, header->size);
> +				dev_err(state->dev,
> +					"core: fsi - pos %x: header %pK, msgid %x, header->msgid %x, header->size %x\n",
> +					pos, header, msgid, header->msgid, header->size);
>   				WARN(1, "invalid slot position\n");
>   			}
>   		}
> @@ -2621,8 +2618,8 @@ close_service_complete(struct vchiq_service *service, int failstate)
>   	case VCHIQ_SRVSTATE_LISTENING:
>   		break;
>   	default:
> -		vchiq_log_error(service->state->dev, VCHIQ_CORE, "%s(%x) called in state %s",
> -				__func__, service->handle, srvstate_names[service->srvstate]);
> +		dev_err(service->state->dev, "core: (%x) called in state %s\n",
> +			service->handle, srvstate_names[service->srvstate]);
>   		WARN(1, "%s in unexpected state\n", __func__);
>   		return -EINVAL;
>   	}
> @@ -2677,8 +2674,8 @@ vchiq_close_service_internal(struct vchiq_service *service, int close_recvd)
>   	case VCHIQ_SRVSTATE_LISTENING:
>   	case VCHIQ_SRVSTATE_CLOSEWAIT:
>   		if (close_recvd) {
> -			vchiq_log_error(state->dev, VCHIQ_CORE, "%s(1) called in state %s",
> -					__func__, srvstate_names[service->srvstate]);
> +			dev_err(state->dev, "core: (1) called in state %s\n",
> +				srvstate_names[service->srvstate]);
>   		} else if (is_server) {
>   			if (service->srvstate == VCHIQ_SRVSTATE_LISTENING) {
>   				status = -EINVAL;
> @@ -2765,8 +2762,8 @@ vchiq_close_service_internal(struct vchiq_service *service, int close_recvd)
>   		break;
>
>   	default:
> -		vchiq_log_error(state->dev, VCHIQ_CORE, "%s(%d) called in state %s", __func__,
> -				close_recvd, srvstate_names[service->srvstate]);
> +		dev_err(state->dev, "core: (%d) called in state %s\n",
> +			close_recvd, srvstate_names[service->srvstate]);
>   		break;
>   	}
>
> @@ -2805,8 +2802,8 @@ vchiq_free_service_internal(struct vchiq_service *service)
>   	case VCHIQ_SRVSTATE_CLOSEWAIT:
>   		break;
>   	default:
> -		vchiq_log_error(state->dev, VCHIQ_CORE, "%d: fsi - (%d) in state %s", state->id,
> -				service->localport, srvstate_names[service->srvstate]);
> +		dev_err(state->dev, "core: %d: fsi - (%d) in state %s\n",
> +			state->id, service->localport, srvstate_names[service->srvstate]);
>   		return;
>   	}
>
> diff --git a/drivers/staging/vc04_services/interface/vchiq_arm/vchiq_core.h b/drivers/staging/vc04_services/interface/vchiq_arm/vchiq_core.h
> index 564b5c707267..d7dcd17e4bff 100644
> --- a/drivers/staging/vc04_services/interface/vchiq_arm/vchiq_core.h
> +++ b/drivers/staging/vc04_services/interface/vchiq_arm/vchiq_core.h
> @@ -53,10 +53,6 @@ static inline const char *log_category_str(enum vchiq_log_category c)
>   	return strings[c];
>   };
>
> -#ifndef vchiq_log_error
> -#define vchiq_log_error(dev, cat, fmt, ...) \
> -	do { dev_dbg(dev, "%s error: " fmt, log_category_str(cat), ##__VA_ARGS__); } while (0)
> -#endif
>   #ifndef vchiq_log_warning
>   #define vchiq_log_warning(dev, cat, fmt, ...) \
>   	do { dev_dbg(dev, "%s warning: " fmt, log_category_str(cat), ##__VA_ARGS__); } while (0)
> diff --git a/drivers/staging/vc04_services/interface/vchiq_arm/vchiq_dev.c b/drivers/staging/vc04_services/interface/vchiq_arm/vchiq_dev.c
> index 307a2d76cbb7..ba287cb4c87b 100644
> --- a/drivers/staging/vc04_services/interface/vchiq_arm/vchiq_dev.c
> +++ b/drivers/staging/vc04_services/interface/vchiq_arm/vchiq_dev.c
> @@ -271,9 +271,9 @@ static int vchiq_ioc_dequeue_message(struct vchiq_instance *instance,
>   			ret = -EFAULT;
>   		}
>   	} else {
> -		vchiq_log_error(service->state->dev, VCHIQ_ARM,
> -				"header %pK: bufsize %x < size %x",
> -				header, args->bufsize, header->size);
> +		dev_err(service->state->dev,
> +			"arm: header %pK: bufsize %x < size %x\n",
> +			header, args->bufsize, header->size);
>   		WARN(1, "invalid size\n");
>   		ret = -EMSGSIZE;
>   	}
> @@ -318,8 +318,8 @@ static int vchiq_irq_queue_bulk_tx_rx(struct vchiq_instance *instance,
>   		}
>   		mutex_unlock(&instance->bulk_waiter_list_mutex);
>   		if (!waiter) {
> -			vchiq_log_error(service->state->dev, VCHIQ_ARM,
> -					"no bulk_waiter found for pid %d", current->pid);
> +			dev_err(service->state->dev,
> +				"arm: no bulk_waiter found for pid %d\n", current->pid);
>   			ret = -ESRCH;
>   			goto out;
>   		}
> @@ -501,10 +501,10 @@ static int vchiq_ioc_await_completion(struct vchiq_instance *instance,
>   			msglen = header->size + sizeof(struct vchiq_header);
>   			/* This must be a VCHIQ-style service */
>   			if (args->msgbufsize < msglen) {
> -				vchiq_log_error(service->state->dev, VCHIQ_ARM,
> -						"header %pK: msgbufsize %x < msglen %x",
> -						header, args->msgbufsize, msglen);
> -						WARN(1, "invalid message size\n");
> +				dev_err(service->state->dev,
> +					"arm: header %pK: msgbufsize %x < msglen %x\n",
> +					header, args->msgbufsize, msglen);
> +				WARN(1, "invalid message size\n");
>   				if (ret == 0)
>   					ret = -EMSGSIZE;
>   				break;
> @@ -618,9 +618,9 @@ vchiq_ioctl(struct file *file, unsigned int cmd, unsigned long arg)
>   		}
>   		rc = mutex_lock_killable(&instance->state->mutex);
>   		if (rc) {
> -			vchiq_log_error(instance->state->dev, VCHIQ_ARM,
> -					"vchiq: connect: could not lock mutex for state %d: %d",
> -					instance->state->id, rc);
> +			dev_err(instance->state->dev,
> +				"arm: vchiq: connect: could not lock mutex for state %d: %d\n",
> +				instance->state->id, rc);
I think this should be dev_dbg
>   			ret = -EINTR;
>   			break;
>   		}
> @@ -630,8 +630,8 @@ vchiq_ioctl(struct file *file, unsigned int cmd, unsigned long arg)
>   		if (!status)
>   			instance->connected = 1;
>   		else
> -			vchiq_log_error(instance->state->dev, VCHIQ_ARM,
> -					"vchiq: could not connect: %d", status);
> +			dev_err(instance->state->dev,
> +				"arm: vchiq: could not connect: %d\n", status);
>   		break;
>
>   	case VCHIQ_IOC_CREATE_SERVICE: {
> @@ -700,13 +700,13 @@ vchiq_ioctl(struct file *file, unsigned int cmd, unsigned long arg)
>   				vchiq_use_service_internal(service) :
>   				vchiq_release_service_internal(service);
>   			if (ret) {
> -				vchiq_log_error(instance->state->dev, VCHIQ_SUSPEND,
> -						"%s: cmd %s returned error %ld for service %p4cc:%03d",
> -						__func__, (cmd == VCHIQ_IOC_USE_SERVICE) ?
> -						"VCHIQ_IOC_USE_SERVICE" :
> -						"VCHIQ_IOC_RELEASE_SERVICE",
> -						ret, &service->base.fourcc,
> -						service->client_id);
> +				dev_err(instance->state->dev,
> +					"suspend: cmd %s returned error %ld for service %p4cc:%03d\n",
> +					(cmd == VCHIQ_IOC_USE_SERVICE) ?
> +					"VCHIQ_IOC_USE_SERVICE" :
> +					"VCHIQ_IOC_RELEASE_SERVICE",
> +					ret, &service->base.fourcc,
> +					service->client_id);
>   			}
>   		} else {
>   			ret = -EINVAL;
> @@ -1173,8 +1173,7 @@ static int vchiq_open(struct inode *inode, struct file *file)
>   	vchiq_log_debug(state->dev, VCHIQ_ARM, "vchiq_open");
>
>   	if (!state) {
> -		vchiq_log_error(state->dev, VCHIQ_ARM,
> -				"vchiq has no connection to VideoCore");
> +		dev_err(state->dev, "arm: vchiq has no connection to VideoCore\n");
>   		return -ENOTCONN;
>   	}
>
Umang Jain Dec. 6, 2023, 5:27 a.m. UTC | #2
Hi Stefan

On 12/6/23 1:02 AM, Stefan Wahren wrote:
> Hi Umang,
>
> Am 05.12.23 um 09:41 schrieb Umang Jain:
>> Drop vchiq_log_error() macro which wraps dev_dbg(). Introduce the usage
>> of dev_err() directly.
> sorry, i missed to review the last change. So the change won't be that
> trivial.

All the changes you said are valid and I have taken a note of it.

However, I think it would best that if we can address them in a separate 
series on top of this (and I will do it). It would seem to be 
streamlined on top of this.
>>
>> Signed-off-by: Umang Jain <umang.jain@ideasonboard.com>
>> ---
>>   .../interface/vchiq_arm/vchiq_arm.c           |  50 ++++----
>>   .../interface/vchiq_arm/vchiq_connected.c     |   6 +-
>>   .../interface/vchiq_arm/vchiq_core.c          | 113 +++++++++---------
>>   .../interface/vchiq_arm/vchiq_core.h          |   4 -
>>   .../interface/vchiq_arm/vchiq_dev.c           |  45 ++++---
>>   5 files changed, 101 insertions(+), 117 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 7978fb6dc4fb..b403400edd8e 100644
>> --- a/drivers/staging/vc04_services/interface/vchiq_arm/vchiq_arm.c
>> +++ b/drivers/staging/vc04_services/interface/vchiq_arm/vchiq_arm.c
>> @@ -683,8 +683,7 @@ int vchiq_initialise(struct vchiq_instance 
>> **instance_out)
>>           usleep_range(500, 600);
>>       }
>>       if (i == VCHIQ_INIT_RETRIES) {
>> -        vchiq_log_error(state->dev, VCHIQ_CORE, "%s: videocore not 
>> initialized\n",
>> -                __func__);
>> +        dev_err(state->dev, "core: %s: Videocore not initialized\n", 
>> __func__);
>>           ret = -ENOTCONN;
>>           goto failed;
>>       } else if (i > 0) {
>> @@ -694,8 +693,7 @@ int vchiq_initialise(struct vchiq_instance 
>> **instance_out)
>>
>>       instance = kzalloc(sizeof(*instance), GFP_KERNEL);
>>       if (!instance) {
>> -        vchiq_log_error(state->dev, VCHIQ_CORE,
>> -                "%s: error allocating vchiq instance\n", __func__);
>> +        dev_err(state->dev, "core: %s: Cannot allocate vchiq 
>> instance\n", __func__);
> please drop because kzalloc already report such errors
>>           ret = -ENOMEM;
>>           goto failed;
>>       }
>> @@ -967,8 +965,7 @@ vchiq_blocking_bulk_transfer(struct 
>> vchiq_instance *instance, unsigned int handl
>>       } else {
>>           waiter = kzalloc(sizeof(*waiter), GFP_KERNEL);
>>           if (!waiter) {
>> -            vchiq_log_error(service->state->dev, VCHIQ_CORE,
>> -                    "%s - out of memory", __func__);
>> +            dev_err(service->state->dev, "core: %s: - Out of 
>> memory\n", __func__);
> ditto
>>               return -ENOMEM;
>>           }
>>       }
>> @@ -1290,8 +1287,8 @@ vchiq_keepalive_vchiq_callback(struct 
>> vchiq_instance *instance,
>>                      struct vchiq_header *header,
>>                      unsigned int service_user, void *bulk_user)
>>   {
>> -    vchiq_log_error(instance->state->dev, VCHIQ_SUSPEND,
>> -            "%s callback reason %d", __func__, reason);
>> +    dev_err(instance->state->dev, "suspend: %s: callback reason %d\n",
>> +        __func__, reason);
>>       return 0;
>>   }
>>
>> @@ -1315,22 +1312,20 @@ vchiq_keepalive_thread_func(void *v)
>>
>>       ret = vchiq_initialise(&instance);
>>       if (ret) {
>> -        vchiq_log_error(state->dev, VCHIQ_SUSPEND,
>> -                "%s vchiq_initialise failed %d", __func__, ret);
>> +        dev_err(state->dev, "suspend: %s: vchiq_initialise failed 
>> %d\n", __func__, ret);
>>           goto exit;
>>       }
>>
>>       status = vchiq_connect(instance);
>>       if (status) {
>> -        vchiq_log_error(state->dev, VCHIQ_SUSPEND,
>> -                "%s vchiq_connect failed %d", __func__, status);
>> +        dev_err(state->dev, "suspend: %s: vchiq_connect failed 
>> %d\n", __func__, status);
>>           goto shutdown;
>>       }
>>
>>       status = vchiq_add_service(instance, &params, &ka_handle);
>>       if (status) {
>> -        vchiq_log_error(state->dev, VCHIQ_SUSPEND,
>> -                "%s vchiq_open_service failed %d", __func__, status);
>> +        dev_err(state->dev, "suspend: %s: vchiq_open_service failed 
>> %d\n",
>> +            __func__, status);
>>           goto shutdown;
>>       }
>>
>> @@ -1338,8 +1333,7 @@ vchiq_keepalive_thread_func(void *v)
>>           long rc = 0, uc = 0;
>>
>>           if (wait_for_completion_interruptible(&arm_state->ka_evt)) {
>> -            vchiq_log_error(state->dev, VCHIQ_SUSPEND,
>> -                    "%s interrupted", __func__);
>> +            dev_err(state->dev, "suspend: %s: interrupted\n", 
>> __func__);
> I think this isn't an error and we should use dev_dbg here.
>>               flush_signals(current);
>>               continue;
>>           }
>> @@ -1359,16 +1353,15 @@ vchiq_keepalive_thread_func(void *v)
>>               atomic_inc(&arm_state->ka_use_ack_count);
>>               status = vchiq_use_service(instance, ka_handle);
>>               if (status) {
>> -                vchiq_log_error(state->dev, VCHIQ_SUSPEND,
>> -                        "%s vchiq_use_service error %d", __func__, 
>> status);
>> +                dev_err(state->dev, "suspend: %s: vchiq_use_service 
>> error %d\n",
>> +                    __func__, status);
>>               }
>>           }
>>           while (rc--) {
>>               status = vchiq_release_service(instance, ka_handle);
>>               if (status) {
>> -                vchiq_log_error(state->dev, VCHIQ_SUSPEND,
>> -                        "%s vchiq_release_service error %d", __func__,
>> -                        status);
>> +                dev_err(state->dev, "suspend: %s: 
>> vchiq_release_service error %d\n",
>> +                    __func__, status);
>>               }
>>           }
>>       }
>> @@ -1403,7 +1396,7 @@ vchiq_use_internal(struct vchiq_state *state, 
>> struct vchiq_service *service,
>>                service->client_id);
>>           entity_uc = &service->service_use_count;
>>       } else {
>> -        vchiq_log_error(state->dev, VCHIQ_SUSPEND, "%s null service 
>> ptr", __func__);
>> +        dev_err(state->dev, "suspend: %s: null service ptr\n", 
>> __func__);
> Instead of dev_err() i would suggest a WARN() here
>>           ret = -EINVAL;
>>           goto out;
>>       }
>> @@ -1686,10 +1679,10 @@ vchiq_check_service(struct vchiq_service 
>> *service)
>>       read_unlock_bh(&arm_state->susp_res_lock);
>>
>>       if (ret) {
>> -        vchiq_log_error(service->state->dev, VCHIQ_SUSPEND,
>> -                "%s ERROR - %p4cc:%d service count %d, state count 
>> %d", __func__,
>> -                &service->base.fourcc, service->client_id,
>> -                service->service_use_count, 
>> arm_state->videocore_use_count);
>> +        dev_err(service->state->dev,
>> +            "suspend: %s:  %p4cc:%d service count %d, state count 
>> %d\n",
>> +            __func__, &service->base.fourcc, service->client_id,
>> +            service->service_use_count, 
>> arm_state->videocore_use_count);
>>           vchiq_dump_service_use_state(service->state);
>>       }
>>   out:
>> @@ -1722,9 +1715,8 @@ void vchiq_platform_conn_state_changed(struct 
>> vchiq_state *state,
>>                             (void *)state,
>>                             threadname);
>>       if (IS_ERR(arm_state->ka_thread)) {
>> -        vchiq_log_error(state->dev, VCHIQ_SUSPEND,
>> -                "vchiq: FATAL: couldn't create thread %s",
>> -                threadname);
>> +        dev_err(state->dev, "suspend: Couldn't create thread %s\n",
>> +            threadname);
>>       } else {
>>           wake_up_process(arm_state->ka_thread);
>>       }
>> diff --git 
>> a/drivers/staging/vc04_services/interface/vchiq_arm/vchiq_connected.c 
>> b/drivers/staging/vc04_services/interface/vchiq_arm/vchiq_connected.c
>> index 21f9fa1a1713..3cad13f09e37 100644
>> --- 
>> a/drivers/staging/vc04_services/interface/vchiq_arm/vchiq_connected.c
>> +++ 
>> b/drivers/staging/vc04_services/interface/vchiq_arm/vchiq_connected.c
>> @@ -39,9 +39,9 @@ void vchiq_add_connected_callback(struct 
>> vchiq_device *device, void (*callback)(
>>           callback();
>>       } else {
>>           if (g_num_deferred_callbacks >= MAX_CALLBACKS) {
>> -            vchiq_log_error(&device->dev, VCHIQ_CORE,
>> -                    "There already %d callback registered - please 
>> increase MAX_CALLBACKS",
>> -                    g_num_deferred_callbacks);
>> +            dev_err(&device->dev,
>> +                "core: There already %d callback registered - please 
>> increase MAX_CALLBACKS\n",
>> +                g_num_deferred_callbacks);
>>           } else {
>>               g_deferred_callback[g_num_deferred_callbacks] =
>>                   callback;
>> 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 e0022acb4c58..63f412815a32 100644
>> --- a/drivers/staging/vc04_services/interface/vchiq_arm/vchiq_core.c
>> +++ b/drivers/staging/vc04_services/interface/vchiq_arm/vchiq_core.c
>> @@ -741,10 +741,10 @@ process_free_data_message(struct vchiq_state 
>> *state, u32 *service_found,
>>            */
>>           complete(&quota->quota_event);
>>       } else if (count == 0) {
>> -        vchiq_log_error(state->dev, VCHIQ_CORE,
>> -                "service %d message_use_count=%d (header %pK, msgid 
>> %x, header->msgid %x, header->size %x)",
>> -                port, quota->message_use_count, header, msgid, 
>> header->msgid,
>> -                header->size);
>> +        dev_err(state->dev,
>> +            "core: service %d message_use_count=%d (header %pK, 
>> msgid %x, header->msgid %x, header->size %x)\n",
>> +            port, quota->message_use_count, header, msgid,
>> +            header->msgid, header->size);
>>           WARN(1, "invalid message use count\n");
>>       }
>>       if (!BITSET_IS_SET(service_found, port)) {
>> @@ -766,9 +766,9 @@ process_free_data_message(struct vchiq_state 
>> *state, u32 *service_found,
>>               vchiq_log_trace(state->dev, VCHIQ_CORE, "%d: pfq:%d 
>> %x@%pK - slot_use->%d",
>>                       state->id, port, header->size, header, count - 1);
>>           } else {
>> -            vchiq_log_error(state->dev, VCHIQ_CORE,
>> -                    "service %d slot_use_count=%d (header %pK, msgid 
>> %x, header->msgid %x, header->size %x)",
>> -                    port, count, header, msgid, header->msgid, 
>> header->size);
>> +            dev_err(state->dev,
>> +                "core: service %d slot_use_count=%d (header %pK, 
>> msgid %x, header->msgid %x, header->size %x)\n",
>> +                port, count, header, msgid, header->msgid, 
>> header->size);
>>               WARN(1, "bad slot use count\n");
>>           }
>>       }
>> @@ -831,9 +831,9 @@ process_free_queue(struct vchiq_state *state, u32 
>> *service_found,
>>
>>               pos += calc_stride(header->size);
>>               if (pos > VCHIQ_SLOT_SIZE) {
>> -                vchiq_log_error(state->dev, VCHIQ_CORE,
>> -                        "pfq - pos %x: header %pK, msgid %x, 
>> header->msgid %x, header->size %x",
>> -                        pos, header, msgid, header->msgid, 
>> header->size);
>> +                dev_err(state->dev,
>> +                    "core: pfq - pos %x: header %pK, msgid %x, 
>> header->msgid %x, header->size %x\n",
>> +                    pos, header, msgid, header->msgid, header->size);
>>                   WARN(1, "invalid slot position\n");
>>               }
>>           }
>> @@ -1167,8 +1167,8 @@ queue_message_sync(struct vchiq_state *state, 
>> struct vchiq_service *service,
>>           int oldmsgid = header->msgid;
>>
>>           if (oldmsgid != VCHIQ_MSGID_PADDING)
>> -            vchiq_log_error(state->dev, VCHIQ_CORE, "%d: qms - msgid 
>> %x, not PADDING",
>> -                    state->id, oldmsgid);
>> +            dev_err(state->dev, "core: %d: qms - msgid %x, not 
>> PADDING\n",
>> +                state->id, oldmsgid);
>>       }
>>
>>       vchiq_log_debug(state->dev, VCHIQ_SYNC,
>> @@ -1616,10 +1616,10 @@ parse_message(struct vchiq_state *state, 
>> struct vchiq_header *header)
>>           }
>>
>>           if (!service) {
>> -            vchiq_log_error(state->dev, VCHIQ_CORE,
>> -                    "%d: prs %s@%pK (%d->%d) - invalid/closed 
>> service %d",
>> -                    state->id, msg_type_str(type), header, remoteport,
>> -                    localport, localport);
>> +            dev_err(state->dev,
>> +                "core: %d: prs %s@%pK (%d->%d) - invalid/closed 
>> service %d\n",
>> +                state->id, msg_type_str(type), header, remoteport,
>> +                localport, localport);
>>               goto skip_message;
>>           }
>>           break;
>> @@ -1640,9 +1640,8 @@ parse_message(struct vchiq_state *state, struct 
>> vchiq_header *header)
>>
>>       if (((unsigned long)header & VCHIQ_SLOT_MASK) +
>>           calc_stride(size) > VCHIQ_SLOT_SIZE) {
>> -        vchiq_log_error(state->dev, VCHIQ_CORE,
>> -                "header %pK (msgid %x) - size %x too big for slot",
>> -                header, (unsigned int)msgid, (unsigned int)size);
>> +        dev_err(state->dev, "core: header %pK (msgid %x) - size %x 
>> too big for slot\n",
>> +            header, (unsigned int)msgid, (unsigned int)size);
>>           WARN(1, "oversized for slot\n");
>>       }
>>
>> @@ -1668,8 +1667,8 @@ parse_message(struct vchiq_state *state, struct 
>> vchiq_header *header)
>>               set_service_state(service, VCHIQ_SRVSTATE_OPEN);
>>               complete(&service->remove_event);
>>           } else {
>> -            vchiq_log_error(state->dev, VCHIQ_CORE, "OPENACK 
>> received in state %s",
>> -                    srvstate_names[service->srvstate]);
>> +            dev_err(state->dev, "core: OPENACK received in state %s\n",
>> +                srvstate_names[service->srvstate]);
>>           }
>>           break;
>>       case VCHIQ_MSG_CLOSE:
>> @@ -1740,11 +1739,10 @@ parse_message(struct vchiq_state *state, 
>> struct vchiq_header *header)
>>               }
>>               if ((int)(queue->remote_insert -
>>                   queue->local_insert) >= 0) {
>> -                vchiq_log_error(state->dev, VCHIQ_CORE,
>> -                        "%d: prs %s@%pK (%d->%d) unexpected 
>> (ri=%d,li=%d)",
>> -                        state->id, msg_type_str(type), header, 
>> remoteport,
>> -                        localport, queue->remote_insert,
>> -                        queue->local_insert);
>> +                dev_err(state->dev,
>> +                    "core: %d: prs %s@%pK (%d->%d) unexpected 
>> (ri=%d,li=%d)\n",
>> +                    state->id, msg_type_str(type), header, remoteport,
>> +                    localport, queue->remote_insert, 
>> queue->local_insert);
>>                   mutex_unlock(&service->bulk_mutex);
>>                   break;
>>               }
>> @@ -1790,8 +1788,8 @@ parse_message(struct vchiq_state *state, struct 
>> vchiq_header *header)
>>           vchiq_log_trace(state->dev, VCHIQ_CORE, "%d: prs 
>> PAUSE@%pK,%x",
>>                   state->id, header, size);
>>           if (state->conn_state == VCHIQ_CONNSTATE_PAUSED) {
>> -            vchiq_log_error(state->dev, VCHIQ_CORE,
>> -                    "%d: PAUSE received in state PAUSED", state->id);
>> +            dev_err(state->dev, "core: %d: PAUSE received in state 
>> PAUSED\n",
>> +                state->id);
>>               break;
>>           }
>>           if (state->conn_state != VCHIQ_CONNSTATE_PAUSE_SENT) {
>> @@ -1821,8 +1819,8 @@ parse_message(struct vchiq_state *state, struct 
>> vchiq_header *header)
>>           break;
>>
>>       default:
>> -        vchiq_log_error(state->dev, VCHIQ_CORE, "%d: prs invalid 
>> msgid %x@%pK,%x",
>> -                state->id, msgid, header, size);
>> +        dev_err(state->dev, "core: %d: prs invalid msgid %x@%pK,%x\n",
>> +            state->id, msgid, header, size);
>>           WARN(1, "invalid message\n");
>>           break;
>>       }
>> @@ -1932,7 +1930,7 @@ handle_poll(struct vchiq_state *state)
>>                * since the PAUSE should have flushed
>>                * through outstanding messages.
>>                */
>> -            vchiq_log_error(state->dev, VCHIQ_CORE, "Failed to send 
>> RESUME message");
>> +            dev_err(state->dev, "core: Failed to send RESUME 
>> message\n");
>>           }
>>           break;
>>       default:
>> @@ -2032,10 +2030,10 @@ sync_func(void *v)
>>           service = find_service_by_port(state, localport);
>>
>>           if (!service) {
>> -            vchiq_log_error(state->dev, VCHIQ_SYNC,
>> -                    "%d: sf %s@%pK (%d->%d) - invalid/closed service 
>> %d",
>> -                    state->id, msg_type_str(type), header,
>> -                    remoteport, localport, localport);
>> +            dev_err(state->dev,
>> +                "sync: %d: sf %s@%pK (%d->%d) - invalid/closed 
>> service %d\n",
>> +                state->id, msg_type_str(type), header, remoteport,
>> +                localport, localport);
>>               release_message_sync(state, header);
>>               continue;
>>           }
>> @@ -2077,15 +2075,15 @@ sync_func(void *v)
>>                   (service->srvstate == VCHIQ_SRVSTATE_OPENSYNC)) {
>>                   if (make_service_callback(service, 
>> VCHIQ_MESSAGE_AVAILABLE, header,
>>                                 NULL) == -EAGAIN)
>> -                    vchiq_log_error(state->dev, VCHIQ_SYNC,
>> -                            "synchronous callback to service %d 
>> returns -EAGAIN",
>> -                            localport);
>> +                    dev_err(state->dev,
>> +                        "sync: error: synchronous callback to 
>> service %d returns -EAGAIN\n",
>> +                        localport);
>>               }
>>               break;
>>
>>           default:
>> -            vchiq_log_error(state->dev, VCHIQ_SYNC, "%d: sf 
>> unexpected msgid %x@%pK,%x",
>> -                    state->id, msgid, header, size);
>> +            dev_err(state->dev, "sync: error: %d: sf unexpected 
>> msgid %x@%pK,%x\n",
>> +                state->id, msgid, header, size);
>>               release_message_sync(state, header);
>>               break;
>>           }
>> @@ -2118,8 +2116,8 @@ vchiq_init_slots(struct device *dev, void 
>> *mem_base, int mem_size)
>>       num_slots -= first_data_slot;
>>
>>       if (num_slots < 4) {
>> -        vchiq_log_error(dev, VCHIQ_CORE, "%s - insufficient memory 
>> %x bytes",
>> -                __func__, mem_size);
>> +        dev_err(dev, "core: %s: Insufficient memory %x bytes\n",
>> +            __func__, mem_size);
>>           return NULL;
>>       }
>>
>> @@ -2500,11 +2498,10 @@ vchiq_open_service_internal(struct 
>> vchiq_service *service, int client_id)
>>       } else if ((service->srvstate != VCHIQ_SRVSTATE_OPEN) &&
>>              (service->srvstate != VCHIQ_SRVSTATE_OPENSYNC)) {
>>           if (service->srvstate != VCHIQ_SRVSTATE_CLOSEWAIT)
>> -            vchiq_log_error(service->state->dev, VCHIQ_CORE,
>> -                    "%d: osi - srvstate = %s (ref %u)",
>> -                    service->state->id,
>> -                    srvstate_names[service->srvstate],
>> -                    kref_read(&service->ref_count));
>> +            dev_err(service->state->dev,
>> +                "core: %d: osi - srvstate = %s (ref %u)\n",
>> +                service->state->id, srvstate_names[service->srvstate],
>> +                kref_read(&service->ref_count));
>>           status = -EINVAL;
>>           VCHIQ_SERVICE_STATS_INC(service, error_count);
>>           vchiq_release_service_internal(service);
>> @@ -2565,9 +2562,9 @@ release_service_messages(struct vchiq_service 
>> *service)
>>               }
>>               pos += calc_stride(header->size);
>>               if (pos > VCHIQ_SLOT_SIZE) {
>> -                vchiq_log_error(state->dev, VCHIQ_CORE,
>> -                        "fsi - pos %x: header %pK, msgid %x, 
>> header->msgid %x, header->size %x",
>> -                        pos, header, msgid, header->msgid, 
>> header->size);
>> +                dev_err(state->dev,
>> +                    "core: fsi - pos %x: header %pK, msgid %x, 
>> header->msgid %x, header->size %x\n",
>> +                    pos, header, msgid, header->msgid, header->size);
>>                   WARN(1, "invalid slot position\n");
>>               }
>>           }
>> @@ -2621,8 +2618,8 @@ close_service_complete(struct vchiq_service 
>> *service, int failstate)
>>       case VCHIQ_SRVSTATE_LISTENING:
>>           break;
>>       default:
>> -        vchiq_log_error(service->state->dev, VCHIQ_CORE, "%s(%x) 
>> called in state %s",
>> -                __func__, service->handle, 
>> srvstate_names[service->srvstate]);
>> +        dev_err(service->state->dev, "core: (%x) called in state %s\n",
>> +            service->handle, srvstate_names[service->srvstate]);
>>           WARN(1, "%s in unexpected state\n", __func__);
>>           return -EINVAL;
>>       }
>> @@ -2677,8 +2674,8 @@ vchiq_close_service_internal(struct 
>> vchiq_service *service, int close_recvd)
>>       case VCHIQ_SRVSTATE_LISTENING:
>>       case VCHIQ_SRVSTATE_CLOSEWAIT:
>>           if (close_recvd) {
>> -            vchiq_log_error(state->dev, VCHIQ_CORE, "%s(1) called in 
>> state %s",
>> -                    __func__, srvstate_names[service->srvstate]);
>> +            dev_err(state->dev, "core: (1) called in state %s\n",
>> +                srvstate_names[service->srvstate]);
>>           } else if (is_server) {
>>               if (service->srvstate == VCHIQ_SRVSTATE_LISTENING) {
>>                   status = -EINVAL;
>> @@ -2765,8 +2762,8 @@ vchiq_close_service_internal(struct 
>> vchiq_service *service, int close_recvd)
>>           break;
>>
>>       default:
>> -        vchiq_log_error(state->dev, VCHIQ_CORE, "%s(%d) called in 
>> state %s", __func__,
>> -                close_recvd, srvstate_names[service->srvstate]);
>> +        dev_err(state->dev, "core: (%d) called in state %s\n",
>> +            close_recvd, srvstate_names[service->srvstate]);
>>           break;
>>       }
>>
>> @@ -2805,8 +2802,8 @@ vchiq_free_service_internal(struct 
>> vchiq_service *service)
>>       case VCHIQ_SRVSTATE_CLOSEWAIT:
>>           break;
>>       default:
>> -        vchiq_log_error(state->dev, VCHIQ_CORE, "%d: fsi - (%d) in 
>> state %s", state->id,
>> -                service->localport, srvstate_names[service->srvstate]);
>> +        dev_err(state->dev, "core: %d: fsi - (%d) in state %s\n",
>> +            state->id, service->localport, 
>> srvstate_names[service->srvstate]);
>>           return;
>>       }
>>
>> diff --git 
>> a/drivers/staging/vc04_services/interface/vchiq_arm/vchiq_core.h 
>> b/drivers/staging/vc04_services/interface/vchiq_arm/vchiq_core.h
>> index 564b5c707267..d7dcd17e4bff 100644
>> --- a/drivers/staging/vc04_services/interface/vchiq_arm/vchiq_core.h
>> +++ b/drivers/staging/vc04_services/interface/vchiq_arm/vchiq_core.h
>> @@ -53,10 +53,6 @@ static inline const char *log_category_str(enum 
>> vchiq_log_category c)
>>       return strings[c];
>>   };
>>
>> -#ifndef vchiq_log_error
>> -#define vchiq_log_error(dev, cat, fmt, ...) \
>> -    do { dev_dbg(dev, "%s error: " fmt, log_category_str(cat), 
>> ##__VA_ARGS__); } while (0)
>> -#endif
>>   #ifndef vchiq_log_warning
>>   #define vchiq_log_warning(dev, cat, fmt, ...) \
>>       do { dev_dbg(dev, "%s warning: " fmt, log_category_str(cat), 
>> ##__VA_ARGS__); } while (0)
>> diff --git 
>> a/drivers/staging/vc04_services/interface/vchiq_arm/vchiq_dev.c 
>> b/drivers/staging/vc04_services/interface/vchiq_arm/vchiq_dev.c
>> index 307a2d76cbb7..ba287cb4c87b 100644
>> --- a/drivers/staging/vc04_services/interface/vchiq_arm/vchiq_dev.c
>> +++ b/drivers/staging/vc04_services/interface/vchiq_arm/vchiq_dev.c
>> @@ -271,9 +271,9 @@ static int vchiq_ioc_dequeue_message(struct 
>> vchiq_instance *instance,
>>               ret = -EFAULT;
>>           }
>>       } else {
>> -        vchiq_log_error(service->state->dev, VCHIQ_ARM,
>> -                "header %pK: bufsize %x < size %x",
>> -                header, args->bufsize, header->size);
>> +        dev_err(service->state->dev,
>> +            "arm: header %pK: bufsize %x < size %x\n",
>> +            header, args->bufsize, header->size);
>>           WARN(1, "invalid size\n");
>>           ret = -EMSGSIZE;
>>       }
>> @@ -318,8 +318,8 @@ static int vchiq_irq_queue_bulk_tx_rx(struct 
>> vchiq_instance *instance,
>>           }
>> mutex_unlock(&instance->bulk_waiter_list_mutex);
>>           if (!waiter) {
>> -            vchiq_log_error(service->state->dev, VCHIQ_ARM,
>> -                    "no bulk_waiter found for pid %d", current->pid);
>> +            dev_err(service->state->dev,
>> +                "arm: no bulk_waiter found for pid %d\n", 
>> current->pid);
>>               ret = -ESRCH;
>>               goto out;
>>           }
>> @@ -501,10 +501,10 @@ static int vchiq_ioc_await_completion(struct 
>> vchiq_instance *instance,
>>               msglen = header->size + sizeof(struct vchiq_header);
>>               /* This must be a VCHIQ-style service */
>>               if (args->msgbufsize < msglen) {
>> -                vchiq_log_error(service->state->dev, VCHIQ_ARM,
>> -                        "header %pK: msgbufsize %x < msglen %x",
>> -                        header, args->msgbufsize, msglen);
>> -                        WARN(1, "invalid message size\n");
>> +                dev_err(service->state->dev,
>> +                    "arm: header %pK: msgbufsize %x < msglen %x\n",
>> +                    header, args->msgbufsize, msglen);
>> +                WARN(1, "invalid message size\n");
>>                   if (ret == 0)
>>                       ret = -EMSGSIZE;
>>                   break;
>> @@ -618,9 +618,9 @@ vchiq_ioctl(struct file *file, unsigned int cmd, 
>> unsigned long arg)
>>           }
>>           rc = mutex_lock_killable(&instance->state->mutex);
>>           if (rc) {
>> -            vchiq_log_error(instance->state->dev, VCHIQ_ARM,
>> -                    "vchiq: connect: could not lock mutex for state 
>> %d: %d",
>> -                    instance->state->id, rc);
>> +            dev_err(instance->state->dev,
>> +                "arm: vchiq: connect: could not lock mutex for state 
>> %d: %d\n",
>> +                instance->state->id, rc);
> I think this should be dev_dbg
>>               ret = -EINTR;
>>               break;
>>           }
>> @@ -630,8 +630,8 @@ vchiq_ioctl(struct file *file, unsigned int cmd, 
>> unsigned long arg)
>>           if (!status)
>>               instance->connected = 1;
>>           else
>> -            vchiq_log_error(instance->state->dev, VCHIQ_ARM,
>> -                    "vchiq: could not connect: %d", status);
>> +            dev_err(instance->state->dev,
>> +                "arm: vchiq: could not connect: %d\n", status);
>>           break;
>>
>>       case VCHIQ_IOC_CREATE_SERVICE: {
>> @@ -700,13 +700,13 @@ vchiq_ioctl(struct file *file, unsigned int 
>> cmd, unsigned long arg)
>>                   vchiq_use_service_internal(service) :
>>                   vchiq_release_service_internal(service);
>>               if (ret) {
>> -                vchiq_log_error(instance->state->dev, VCHIQ_SUSPEND,
>> -                        "%s: cmd %s returned error %ld for service 
>> %p4cc:%03d",
>> -                        __func__, (cmd == VCHIQ_IOC_USE_SERVICE) ?
>> -                        "VCHIQ_IOC_USE_SERVICE" :
>> -                        "VCHIQ_IOC_RELEASE_SERVICE",
>> -                        ret, &service->base.fourcc,
>> -                        service->client_id);
>> +                dev_err(instance->state->dev,
>> +                    "suspend: cmd %s returned error %ld for service 
>> %p4cc:%03d\n",
>> +                    (cmd == VCHIQ_IOC_USE_SERVICE) ?
>> +                    "VCHIQ_IOC_USE_SERVICE" :
>> +                    "VCHIQ_IOC_RELEASE_SERVICE",
>> +                    ret, &service->base.fourcc,
>> +                    service->client_id);
>>               }
>>           } else {
>>               ret = -EINVAL;
>> @@ -1173,8 +1173,7 @@ static int vchiq_open(struct inode *inode, 
>> struct file *file)
>>       vchiq_log_debug(state->dev, VCHIQ_ARM, "vchiq_open");
>>
>>       if (!state) {
>> -        vchiq_log_error(state->dev, VCHIQ_ARM,
>> -                "vchiq has no connection to VideoCore");
>> +        dev_err(state->dev, "arm: vchiq has no connection to 
>> VideoCore\n");
>>           return -ENOTCONN;
>>       }
>>
>
Stefan Wahren Dec. 6, 2023, 9:34 p.m. UTC | #3
Hi Umang,

Am 06.12.23 um 06:27 schrieb Umang Jain:
> Hi Stefan
>
> On 12/6/23 1:02 AM, Stefan Wahren wrote:
>> Hi Umang,
>>
>> Am 05.12.23 um 09:41 schrieb Umang Jain:
>>> Drop vchiq_log_error() macro which wraps dev_dbg(). Introduce the usage
>>> of dev_err() directly.
>> sorry, i missed to review the last change. So the change won't be that
>> trivial.
>
> All the changes you said are valid and I have taken a note of it.
>
> However, I think it would best that if we can address them in a
> separate series on top of this (and I will do it). It would seem to be
> streamlined on top of this.
i would address the comments as separate patches and then finally
convert the trivial rest with such a patch within one series. This
avoids unnecessary changes and reviewes. But that's just my opinion.

Best regards
Greg KH Dec. 7, 2023, 2:39 a.m. UTC | #4
On Wed, Dec 06, 2023 at 10:57:59AM +0530, Umang Jain wrote:
> Hi Stefan
> 
> On 12/6/23 1:02 AM, Stefan Wahren wrote:
> > Hi Umang,
> > 
> > Am 05.12.23 um 09:41 schrieb Umang Jain:
> > > Drop vchiq_log_error() macro which wraps dev_dbg(). Introduce the usage
> > > of dev_err() directly.
> > sorry, i missed to review the last change. So the change won't be that
> > trivial.
> 
> All the changes you said are valid and I have taken a note of it.
> 
> However, I think it would best that if we can address them in a separate
> series on top of this (and I will do it). It would seem to be streamlined on
> top of this.

Agreed, I'll merge this now so you can make forward progress here,
thanks!

greg k-h
Umang Jain Dec. 7, 2023, 8:41 a.m. UTC | #5
Hi Stefan

On 12/7/23 3:04 AM, Stefan Wahren wrote:
> Hi Umang,
>
> Am 06.12.23 um 06:27 schrieb Umang Jain:
>> Hi Stefan
>>
>> On 12/6/23 1:02 AM, Stefan Wahren wrote:
>>> Hi Umang,
>>>
>>> Am 05.12.23 um 09:41 schrieb Umang Jain:
>>>> Drop vchiq_log_error() macro which wraps dev_dbg(). Introduce the 
>>>> usage
>>>> of dev_err() directly.
>>> sorry, i missed to review the last change. So the change won't be that
>>> trivial.
>>
>> All the changes you said are valid and I have taken a note of it.
>>
>> However, I think it would best that if we can address them in a
>> separate series on top of this (and I will do it). It would seem to be
>> streamlined on top of this.
> i would address the comments as separate patches and then finally
> convert the trivial rest with such a patch within one series. This
> avoids unnecessary changes and reviewes. But that's just my opinion.
>

Series is merged by Greg KH (Thank you).

Do you have more comments on 3/4 and 4/4 or are they waiting on your 
review queue. Let me know so that I can prepare a follow up series in 
-one- go.

thanks!
> Best regards
Stefan Wahren Dec. 7, 2023, 10:28 p.m. UTC | #6
Hi Umang,

Am 07.12.23 um 09:41 schrieb Umang Jain:
> Hi Stefan
>
> On 12/7/23 3:04 AM, Stefan Wahren wrote:
>> Hi Umang,
>>
>> Am 06.12.23 um 06:27 schrieb Umang Jain:
>>> Hi Stefan
>>>
>>> On 12/6/23 1:02 AM, Stefan Wahren wrote:
>>>> Hi Umang,
>>>>
>>>> Am 05.12.23 um 09:41 schrieb Umang Jain:
>>>>> Drop vchiq_log_error() macro which wraps dev_dbg(). Introduce the
>>>>> usage
>>>>> of dev_err() directly.
>>>> sorry, i missed to review the last change. So the change won't be that
>>>> trivial.
>>>
>>> All the changes you said are valid and I have taken a note of it.
>>>
>>> However, I think it would best that if we can address them in a
>>> separate series on top of this (and I will do it). It would seem to be
>>> streamlined on top of this.
>> i would address the comments as separate patches and then finally
>> convert the trivial rest with such a patch within one series. This
>> avoids unnecessary changes and reviewes. But that's just my opinion.
>>
>
> Series is merged by Greg KH (Thank you).
>
> Do you have more comments on 3/4 and 4/4 or are they waiting on your
> review queue. Let me know so that I can prepare a follow up series in
> -one- go.
i was busy. I will look at the rest tomorrow.

Best regards
>
> thanks!
>> Best regards
>
diff mbox series

Patch

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 7978fb6dc4fb..b403400edd8e 100644
--- a/drivers/staging/vc04_services/interface/vchiq_arm/vchiq_arm.c
+++ b/drivers/staging/vc04_services/interface/vchiq_arm/vchiq_arm.c
@@ -683,8 +683,7 @@  int vchiq_initialise(struct vchiq_instance **instance_out)
 		usleep_range(500, 600);
 	}
 	if (i == VCHIQ_INIT_RETRIES) {
-		vchiq_log_error(state->dev, VCHIQ_CORE, "%s: videocore not initialized\n",
-				__func__);
+		dev_err(state->dev, "core: %s: Videocore not initialized\n", __func__);
 		ret = -ENOTCONN;
 		goto failed;
 	} else if (i > 0) {
@@ -694,8 +693,7 @@  int vchiq_initialise(struct vchiq_instance **instance_out)
 
 	instance = kzalloc(sizeof(*instance), GFP_KERNEL);
 	if (!instance) {
-		vchiq_log_error(state->dev, VCHIQ_CORE,
-				"%s: error allocating vchiq instance\n", __func__);
+		dev_err(state->dev, "core: %s: Cannot allocate vchiq instance\n", __func__);
 		ret = -ENOMEM;
 		goto failed;
 	}
@@ -967,8 +965,7 @@  vchiq_blocking_bulk_transfer(struct vchiq_instance *instance, unsigned int handl
 	} else {
 		waiter = kzalloc(sizeof(*waiter), GFP_KERNEL);
 		if (!waiter) {
-			vchiq_log_error(service->state->dev, VCHIQ_CORE,
-					"%s - out of memory", __func__);
+			dev_err(service->state->dev, "core: %s: - Out of memory\n", __func__);
 			return -ENOMEM;
 		}
 	}
@@ -1290,8 +1287,8 @@  vchiq_keepalive_vchiq_callback(struct vchiq_instance *instance,
 			       struct vchiq_header *header,
 			       unsigned int service_user, void *bulk_user)
 {
-	vchiq_log_error(instance->state->dev, VCHIQ_SUSPEND,
-			"%s callback reason %d", __func__, reason);
+	dev_err(instance->state->dev, "suspend: %s: callback reason %d\n",
+		__func__, reason);
 	return 0;
 }
 
@@ -1315,22 +1312,20 @@  vchiq_keepalive_thread_func(void *v)
 
 	ret = vchiq_initialise(&instance);
 	if (ret) {
-		vchiq_log_error(state->dev, VCHIQ_SUSPEND,
-				"%s vchiq_initialise failed %d", __func__, ret);
+		dev_err(state->dev, "suspend: %s: vchiq_initialise failed %d\n", __func__, ret);
 		goto exit;
 	}
 
 	status = vchiq_connect(instance);
 	if (status) {
-		vchiq_log_error(state->dev, VCHIQ_SUSPEND,
-				"%s vchiq_connect failed %d", __func__, status);
+		dev_err(state->dev, "suspend: %s: vchiq_connect failed %d\n", __func__, status);
 		goto shutdown;
 	}
 
 	status = vchiq_add_service(instance, &params, &ka_handle);
 	if (status) {
-		vchiq_log_error(state->dev, VCHIQ_SUSPEND,
-				"%s vchiq_open_service failed %d", __func__, status);
+		dev_err(state->dev, "suspend: %s: vchiq_open_service failed %d\n",
+			__func__, status);
 		goto shutdown;
 	}
 
@@ -1338,8 +1333,7 @@  vchiq_keepalive_thread_func(void *v)
 		long rc = 0, uc = 0;
 
 		if (wait_for_completion_interruptible(&arm_state->ka_evt)) {
-			vchiq_log_error(state->dev, VCHIQ_SUSPEND,
-					"%s interrupted", __func__);
+			dev_err(state->dev, "suspend: %s: interrupted\n", __func__);
 			flush_signals(current);
 			continue;
 		}
@@ -1359,16 +1353,15 @@  vchiq_keepalive_thread_func(void *v)
 			atomic_inc(&arm_state->ka_use_ack_count);
 			status = vchiq_use_service(instance, ka_handle);
 			if (status) {
-				vchiq_log_error(state->dev, VCHIQ_SUSPEND,
-						"%s vchiq_use_service error %d", __func__, status);
+				dev_err(state->dev, "suspend: %s: vchiq_use_service error %d\n",
+					__func__, status);
 			}
 		}
 		while (rc--) {
 			status = vchiq_release_service(instance, ka_handle);
 			if (status) {
-				vchiq_log_error(state->dev, VCHIQ_SUSPEND,
-						"%s vchiq_release_service error %d", __func__,
-						status);
+				dev_err(state->dev, "suspend: %s: vchiq_release_service error %d\n",
+					__func__, status);
 			}
 		}
 	}
@@ -1403,7 +1396,7 @@  vchiq_use_internal(struct vchiq_state *state, struct vchiq_service *service,
 			 service->client_id);
 		entity_uc = &service->service_use_count;
 	} else {
-		vchiq_log_error(state->dev, VCHIQ_SUSPEND, "%s null service ptr", __func__);
+		dev_err(state->dev, "suspend: %s: null service ptr\n", __func__);
 		ret = -EINVAL;
 		goto out;
 	}
@@ -1686,10 +1679,10 @@  vchiq_check_service(struct vchiq_service *service)
 	read_unlock_bh(&arm_state->susp_res_lock);
 
 	if (ret) {
-		vchiq_log_error(service->state->dev, VCHIQ_SUSPEND,
-				"%s ERROR - %p4cc:%d service count %d, state count %d", __func__,
-				&service->base.fourcc, service->client_id,
-				service->service_use_count, arm_state->videocore_use_count);
+		dev_err(service->state->dev,
+			"suspend: %s:  %p4cc:%d service count %d, state count %d\n",
+			__func__, &service->base.fourcc, service->client_id,
+			service->service_use_count, arm_state->videocore_use_count);
 		vchiq_dump_service_use_state(service->state);
 	}
 out:
@@ -1722,9 +1715,8 @@  void vchiq_platform_conn_state_changed(struct vchiq_state *state,
 					      (void *)state,
 					      threadname);
 	if (IS_ERR(arm_state->ka_thread)) {
-		vchiq_log_error(state->dev, VCHIQ_SUSPEND,
-				"vchiq: FATAL: couldn't create thread %s",
-				threadname);
+		dev_err(state->dev, "suspend: Couldn't create thread %s\n",
+			threadname);
 	} else {
 		wake_up_process(arm_state->ka_thread);
 	}
diff --git a/drivers/staging/vc04_services/interface/vchiq_arm/vchiq_connected.c b/drivers/staging/vc04_services/interface/vchiq_arm/vchiq_connected.c
index 21f9fa1a1713..3cad13f09e37 100644
--- a/drivers/staging/vc04_services/interface/vchiq_arm/vchiq_connected.c
+++ b/drivers/staging/vc04_services/interface/vchiq_arm/vchiq_connected.c
@@ -39,9 +39,9 @@  void vchiq_add_connected_callback(struct vchiq_device *device, void (*callback)(
 		callback();
 	} else {
 		if (g_num_deferred_callbacks >= MAX_CALLBACKS) {
-			vchiq_log_error(&device->dev, VCHIQ_CORE,
-					"There already %d callback registered - please increase MAX_CALLBACKS",
-					g_num_deferred_callbacks);
+			dev_err(&device->dev,
+				"core: There already %d callback registered - please increase MAX_CALLBACKS\n",
+				g_num_deferred_callbacks);
 		} else {
 			g_deferred_callback[g_num_deferred_callbacks] =
 				callback;
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 e0022acb4c58..63f412815a32 100644
--- a/drivers/staging/vc04_services/interface/vchiq_arm/vchiq_core.c
+++ b/drivers/staging/vc04_services/interface/vchiq_arm/vchiq_core.c
@@ -741,10 +741,10 @@  process_free_data_message(struct vchiq_state *state, u32 *service_found,
 		 */
 		complete(&quota->quota_event);
 	} else if (count == 0) {
-		vchiq_log_error(state->dev, VCHIQ_CORE,
-				"service %d message_use_count=%d (header %pK, msgid %x, header->msgid %x, header->size %x)",
-				port, quota->message_use_count, header, msgid, header->msgid,
-				header->size);
+		dev_err(state->dev,
+			"core: service %d message_use_count=%d (header %pK, msgid %x, header->msgid %x, header->size %x)\n",
+			port, quota->message_use_count, header, msgid,
+			header->msgid, header->size);
 		WARN(1, "invalid message use count\n");
 	}
 	if (!BITSET_IS_SET(service_found, port)) {
@@ -766,9 +766,9 @@  process_free_data_message(struct vchiq_state *state, u32 *service_found,
 			vchiq_log_trace(state->dev, VCHIQ_CORE, "%d: pfq:%d %x@%pK - slot_use->%d",
 					state->id, port, header->size, header, count - 1);
 		} else {
-			vchiq_log_error(state->dev, VCHIQ_CORE,
-					"service %d slot_use_count=%d (header %pK, msgid %x, header->msgid %x, header->size %x)",
-					port, count, header, msgid, header->msgid, header->size);
+			dev_err(state->dev,
+				"core: service %d slot_use_count=%d (header %pK, msgid %x, header->msgid %x, header->size %x)\n",
+				port, count, header, msgid, header->msgid, header->size);
 			WARN(1, "bad slot use count\n");
 		}
 	}
@@ -831,9 +831,9 @@  process_free_queue(struct vchiq_state *state, u32 *service_found,
 
 			pos += calc_stride(header->size);
 			if (pos > VCHIQ_SLOT_SIZE) {
-				vchiq_log_error(state->dev, VCHIQ_CORE,
-						"pfq - pos %x: header %pK, msgid %x, header->msgid %x, header->size %x",
-						pos, header, msgid, header->msgid, header->size);
+				dev_err(state->dev,
+					"core: pfq - pos %x: header %pK, msgid %x, header->msgid %x, header->size %x\n",
+					pos, header, msgid, header->msgid, header->size);
 				WARN(1, "invalid slot position\n");
 			}
 		}
@@ -1167,8 +1167,8 @@  queue_message_sync(struct vchiq_state *state, struct vchiq_service *service,
 		int oldmsgid = header->msgid;
 
 		if (oldmsgid != VCHIQ_MSGID_PADDING)
-			vchiq_log_error(state->dev, VCHIQ_CORE, "%d: qms - msgid %x, not PADDING",
-					state->id, oldmsgid);
+			dev_err(state->dev, "core: %d: qms - msgid %x, not PADDING\n",
+				state->id, oldmsgid);
 	}
 
 	vchiq_log_debug(state->dev, VCHIQ_SYNC,
@@ -1616,10 +1616,10 @@  parse_message(struct vchiq_state *state, struct vchiq_header *header)
 		}
 
 		if (!service) {
-			vchiq_log_error(state->dev, VCHIQ_CORE,
-					"%d: prs %s@%pK (%d->%d) - invalid/closed service %d",
-					state->id, msg_type_str(type), header, remoteport,
-					localport, localport);
+			dev_err(state->dev,
+				"core: %d: prs %s@%pK (%d->%d) - invalid/closed service %d\n",
+				state->id, msg_type_str(type), header, remoteport,
+				localport, localport);
 			goto skip_message;
 		}
 		break;
@@ -1640,9 +1640,8 @@  parse_message(struct vchiq_state *state, struct vchiq_header *header)
 
 	if (((unsigned long)header & VCHIQ_SLOT_MASK) +
 	    calc_stride(size) > VCHIQ_SLOT_SIZE) {
-		vchiq_log_error(state->dev, VCHIQ_CORE,
-				"header %pK (msgid %x) - size %x too big for slot",
-				header, (unsigned int)msgid, (unsigned int)size);
+		dev_err(state->dev, "core: header %pK (msgid %x) - size %x too big for slot\n",
+			header, (unsigned int)msgid, (unsigned int)size);
 		WARN(1, "oversized for slot\n");
 	}
 
@@ -1668,8 +1667,8 @@  parse_message(struct vchiq_state *state, struct vchiq_header *header)
 			set_service_state(service, VCHIQ_SRVSTATE_OPEN);
 			complete(&service->remove_event);
 		} else {
-			vchiq_log_error(state->dev, VCHIQ_CORE, "OPENACK received in state %s",
-					srvstate_names[service->srvstate]);
+			dev_err(state->dev, "core: OPENACK received in state %s\n",
+				srvstate_names[service->srvstate]);
 		}
 		break;
 	case VCHIQ_MSG_CLOSE:
@@ -1740,11 +1739,10 @@  parse_message(struct vchiq_state *state, struct vchiq_header *header)
 			}
 			if ((int)(queue->remote_insert -
 				queue->local_insert) >= 0) {
-				vchiq_log_error(state->dev, VCHIQ_CORE,
-						"%d: prs %s@%pK (%d->%d) unexpected (ri=%d,li=%d)",
-						state->id, msg_type_str(type), header, remoteport,
-						localport, queue->remote_insert,
-						queue->local_insert);
+				dev_err(state->dev,
+					"core: %d: prs %s@%pK (%d->%d) unexpected (ri=%d,li=%d)\n",
+					state->id, msg_type_str(type), header, remoteport,
+					localport, queue->remote_insert, queue->local_insert);
 				mutex_unlock(&service->bulk_mutex);
 				break;
 			}
@@ -1790,8 +1788,8 @@  parse_message(struct vchiq_state *state, struct vchiq_header *header)
 		vchiq_log_trace(state->dev, VCHIQ_CORE, "%d: prs PAUSE@%pK,%x",
 				state->id, header, size);
 		if (state->conn_state == VCHIQ_CONNSTATE_PAUSED) {
-			vchiq_log_error(state->dev, VCHIQ_CORE,
-					"%d: PAUSE received in state PAUSED", state->id);
+			dev_err(state->dev, "core: %d: PAUSE received in state PAUSED\n",
+				state->id);
 			break;
 		}
 		if (state->conn_state != VCHIQ_CONNSTATE_PAUSE_SENT) {
@@ -1821,8 +1819,8 @@  parse_message(struct vchiq_state *state, struct vchiq_header *header)
 		break;
 
 	default:
-		vchiq_log_error(state->dev, VCHIQ_CORE, "%d: prs invalid msgid %x@%pK,%x",
-				state->id, msgid, header, size);
+		dev_err(state->dev, "core: %d: prs invalid msgid %x@%pK,%x\n",
+			state->id, msgid, header, size);
 		WARN(1, "invalid message\n");
 		break;
 	}
@@ -1932,7 +1930,7 @@  handle_poll(struct vchiq_state *state)
 			 * since the PAUSE should have flushed
 			 * through outstanding messages.
 			 */
-			vchiq_log_error(state->dev, VCHIQ_CORE, "Failed to send RESUME message");
+			dev_err(state->dev, "core: Failed to send RESUME message\n");
 		}
 		break;
 	default:
@@ -2032,10 +2030,10 @@  sync_func(void *v)
 		service = find_service_by_port(state, localport);
 
 		if (!service) {
-			vchiq_log_error(state->dev, VCHIQ_SYNC,
-					"%d: sf %s@%pK (%d->%d) - invalid/closed service %d",
-					state->id, msg_type_str(type), header,
-					remoteport, localport, localport);
+			dev_err(state->dev,
+				"sync: %d: sf %s@%pK (%d->%d) - invalid/closed service %d\n",
+				state->id, msg_type_str(type), header, remoteport,
+				localport, localport);
 			release_message_sync(state, header);
 			continue;
 		}
@@ -2077,15 +2075,15 @@  sync_func(void *v)
 			    (service->srvstate == VCHIQ_SRVSTATE_OPENSYNC)) {
 				if (make_service_callback(service, VCHIQ_MESSAGE_AVAILABLE, header,
 							  NULL) == -EAGAIN)
-					vchiq_log_error(state->dev, VCHIQ_SYNC,
-							"synchronous callback to service %d returns -EAGAIN",
-							localport);
+					dev_err(state->dev,
+						"sync: error: synchronous callback to service %d returns -EAGAIN\n",
+						localport);
 			}
 			break;
 
 		default:
-			vchiq_log_error(state->dev, VCHIQ_SYNC, "%d: sf unexpected msgid %x@%pK,%x",
-					state->id, msgid, header, size);
+			dev_err(state->dev, "sync: error: %d: sf unexpected msgid %x@%pK,%x\n",
+				state->id, msgid, header, size);
 			release_message_sync(state, header);
 			break;
 		}
@@ -2118,8 +2116,8 @@  vchiq_init_slots(struct device *dev, void *mem_base, int mem_size)
 	num_slots -= first_data_slot;
 
 	if (num_slots < 4) {
-		vchiq_log_error(dev, VCHIQ_CORE, "%s - insufficient memory %x bytes",
-				__func__, mem_size);
+		dev_err(dev, "core: %s: Insufficient memory %x bytes\n",
+			__func__, mem_size);
 		return NULL;
 	}
 
@@ -2500,11 +2498,10 @@  vchiq_open_service_internal(struct vchiq_service *service, int client_id)
 	} else if ((service->srvstate != VCHIQ_SRVSTATE_OPEN) &&
 		   (service->srvstate != VCHIQ_SRVSTATE_OPENSYNC)) {
 		if (service->srvstate != VCHIQ_SRVSTATE_CLOSEWAIT)
-			vchiq_log_error(service->state->dev, VCHIQ_CORE,
-					"%d: osi - srvstate = %s (ref %u)",
-					service->state->id,
-					srvstate_names[service->srvstate],
-					kref_read(&service->ref_count));
+			dev_err(service->state->dev,
+				"core: %d: osi - srvstate = %s (ref %u)\n",
+				service->state->id, srvstate_names[service->srvstate],
+				kref_read(&service->ref_count));
 		status = -EINVAL;
 		VCHIQ_SERVICE_STATS_INC(service, error_count);
 		vchiq_release_service_internal(service);
@@ -2565,9 +2562,9 @@  release_service_messages(struct vchiq_service *service)
 			}
 			pos += calc_stride(header->size);
 			if (pos > VCHIQ_SLOT_SIZE) {
-				vchiq_log_error(state->dev, VCHIQ_CORE,
-						"fsi - pos %x: header %pK, msgid %x, header->msgid %x, header->size %x",
-						pos, header, msgid, header->msgid, header->size);
+				dev_err(state->dev,
+					"core: fsi - pos %x: header %pK, msgid %x, header->msgid %x, header->size %x\n",
+					pos, header, msgid, header->msgid, header->size);
 				WARN(1, "invalid slot position\n");
 			}
 		}
@@ -2621,8 +2618,8 @@  close_service_complete(struct vchiq_service *service, int failstate)
 	case VCHIQ_SRVSTATE_LISTENING:
 		break;
 	default:
-		vchiq_log_error(service->state->dev, VCHIQ_CORE, "%s(%x) called in state %s",
-				__func__, service->handle, srvstate_names[service->srvstate]);
+		dev_err(service->state->dev, "core: (%x) called in state %s\n",
+			service->handle, srvstate_names[service->srvstate]);
 		WARN(1, "%s in unexpected state\n", __func__);
 		return -EINVAL;
 	}
@@ -2677,8 +2674,8 @@  vchiq_close_service_internal(struct vchiq_service *service, int close_recvd)
 	case VCHIQ_SRVSTATE_LISTENING:
 	case VCHIQ_SRVSTATE_CLOSEWAIT:
 		if (close_recvd) {
-			vchiq_log_error(state->dev, VCHIQ_CORE, "%s(1) called in state %s",
-					__func__, srvstate_names[service->srvstate]);
+			dev_err(state->dev, "core: (1) called in state %s\n",
+				srvstate_names[service->srvstate]);
 		} else if (is_server) {
 			if (service->srvstate == VCHIQ_SRVSTATE_LISTENING) {
 				status = -EINVAL;
@@ -2765,8 +2762,8 @@  vchiq_close_service_internal(struct vchiq_service *service, int close_recvd)
 		break;
 
 	default:
-		vchiq_log_error(state->dev, VCHIQ_CORE, "%s(%d) called in state %s", __func__,
-				close_recvd, srvstate_names[service->srvstate]);
+		dev_err(state->dev, "core: (%d) called in state %s\n",
+			close_recvd, srvstate_names[service->srvstate]);
 		break;
 	}
 
@@ -2805,8 +2802,8 @@  vchiq_free_service_internal(struct vchiq_service *service)
 	case VCHIQ_SRVSTATE_CLOSEWAIT:
 		break;
 	default:
-		vchiq_log_error(state->dev, VCHIQ_CORE, "%d: fsi - (%d) in state %s", state->id,
-				service->localport, srvstate_names[service->srvstate]);
+		dev_err(state->dev, "core: %d: fsi - (%d) in state %s\n",
+			state->id, service->localport, srvstate_names[service->srvstate]);
 		return;
 	}
 
diff --git a/drivers/staging/vc04_services/interface/vchiq_arm/vchiq_core.h b/drivers/staging/vc04_services/interface/vchiq_arm/vchiq_core.h
index 564b5c707267..d7dcd17e4bff 100644
--- a/drivers/staging/vc04_services/interface/vchiq_arm/vchiq_core.h
+++ b/drivers/staging/vc04_services/interface/vchiq_arm/vchiq_core.h
@@ -53,10 +53,6 @@  static inline const char *log_category_str(enum vchiq_log_category c)
 	return strings[c];
 };
 
-#ifndef vchiq_log_error
-#define vchiq_log_error(dev, cat, fmt, ...) \
-	do { dev_dbg(dev, "%s error: " fmt, log_category_str(cat), ##__VA_ARGS__); } while (0)
-#endif
 #ifndef vchiq_log_warning
 #define vchiq_log_warning(dev, cat, fmt, ...) \
 	do { dev_dbg(dev, "%s warning: " fmt, log_category_str(cat), ##__VA_ARGS__); } while (0)
diff --git a/drivers/staging/vc04_services/interface/vchiq_arm/vchiq_dev.c b/drivers/staging/vc04_services/interface/vchiq_arm/vchiq_dev.c
index 307a2d76cbb7..ba287cb4c87b 100644
--- a/drivers/staging/vc04_services/interface/vchiq_arm/vchiq_dev.c
+++ b/drivers/staging/vc04_services/interface/vchiq_arm/vchiq_dev.c
@@ -271,9 +271,9 @@  static int vchiq_ioc_dequeue_message(struct vchiq_instance *instance,
 			ret = -EFAULT;
 		}
 	} else {
-		vchiq_log_error(service->state->dev, VCHIQ_ARM,
-				"header %pK: bufsize %x < size %x",
-				header, args->bufsize, header->size);
+		dev_err(service->state->dev,
+			"arm: header %pK: bufsize %x < size %x\n",
+			header, args->bufsize, header->size);
 		WARN(1, "invalid size\n");
 		ret = -EMSGSIZE;
 	}
@@ -318,8 +318,8 @@  static int vchiq_irq_queue_bulk_tx_rx(struct vchiq_instance *instance,
 		}
 		mutex_unlock(&instance->bulk_waiter_list_mutex);
 		if (!waiter) {
-			vchiq_log_error(service->state->dev, VCHIQ_ARM,
-					"no bulk_waiter found for pid %d", current->pid);
+			dev_err(service->state->dev,
+				"arm: no bulk_waiter found for pid %d\n", current->pid);
 			ret = -ESRCH;
 			goto out;
 		}
@@ -501,10 +501,10 @@  static int vchiq_ioc_await_completion(struct vchiq_instance *instance,
 			msglen = header->size + sizeof(struct vchiq_header);
 			/* This must be a VCHIQ-style service */
 			if (args->msgbufsize < msglen) {
-				vchiq_log_error(service->state->dev, VCHIQ_ARM,
-						"header %pK: msgbufsize %x < msglen %x",
-						header, args->msgbufsize, msglen);
-						WARN(1, "invalid message size\n");
+				dev_err(service->state->dev,
+					"arm: header %pK: msgbufsize %x < msglen %x\n",
+					header, args->msgbufsize, msglen);
+				WARN(1, "invalid message size\n");
 				if (ret == 0)
 					ret = -EMSGSIZE;
 				break;
@@ -618,9 +618,9 @@  vchiq_ioctl(struct file *file, unsigned int cmd, unsigned long arg)
 		}
 		rc = mutex_lock_killable(&instance->state->mutex);
 		if (rc) {
-			vchiq_log_error(instance->state->dev, VCHIQ_ARM,
-					"vchiq: connect: could not lock mutex for state %d: %d",
-					instance->state->id, rc);
+			dev_err(instance->state->dev,
+				"arm: vchiq: connect: could not lock mutex for state %d: %d\n",
+				instance->state->id, rc);
 			ret = -EINTR;
 			break;
 		}
@@ -630,8 +630,8 @@  vchiq_ioctl(struct file *file, unsigned int cmd, unsigned long arg)
 		if (!status)
 			instance->connected = 1;
 		else
-			vchiq_log_error(instance->state->dev, VCHIQ_ARM,
-					"vchiq: could not connect: %d", status);
+			dev_err(instance->state->dev,
+				"arm: vchiq: could not connect: %d\n", status);
 		break;
 
 	case VCHIQ_IOC_CREATE_SERVICE: {
@@ -700,13 +700,13 @@  vchiq_ioctl(struct file *file, unsigned int cmd, unsigned long arg)
 				vchiq_use_service_internal(service) :
 				vchiq_release_service_internal(service);
 			if (ret) {
-				vchiq_log_error(instance->state->dev, VCHIQ_SUSPEND,
-						"%s: cmd %s returned error %ld for service %p4cc:%03d",
-						__func__, (cmd == VCHIQ_IOC_USE_SERVICE) ?
-						"VCHIQ_IOC_USE_SERVICE" :
-						"VCHIQ_IOC_RELEASE_SERVICE",
-						ret, &service->base.fourcc,
-						service->client_id);
+				dev_err(instance->state->dev,
+					"suspend: cmd %s returned error %ld for service %p4cc:%03d\n",
+					(cmd == VCHIQ_IOC_USE_SERVICE) ?
+					"VCHIQ_IOC_USE_SERVICE" :
+					"VCHIQ_IOC_RELEASE_SERVICE",
+					ret, &service->base.fourcc,
+					service->client_id);
 			}
 		} else {
 			ret = -EINVAL;
@@ -1173,8 +1173,7 @@  static int vchiq_open(struct inode *inode, struct file *file)
 	vchiq_log_debug(state->dev, VCHIQ_ARM, "vchiq_open");
 
 	if (!state) {
-		vchiq_log_error(state->dev, VCHIQ_ARM,
-				"vchiq has no connection to VideoCore");
+		dev_err(state->dev, "arm: vchiq has no connection to VideoCore\n");
 		return -ENOTCONN;
 	}