[PATCH 0/1] staging: vc04_services: Use %p4cc to print fourcc

[PATCH 0/1] staging: vc04_services: Use %p4cc to print fourcc

[Umang Jain]

The following patch drop VCHIQ_FOURCC_AS_4CHARS macro in favour of %p4cc
format modifier to print FourCC codes in the logs.

*Before this patch*
`mailbox: vchiq_core_msg debug: Sent Msg DATA(5) to AUDS s:1 d:62 len:20`

*After this patch*
bcm2835_vchiq 3f00b840.mailbox: vchiq_core_msg debug: Sent Msg DATA(5) to SDUA little-endian (0x41554453) s:1 d:62 len:20

The inversion of AUDS to SDUA as per usage of %p4cc
Does it hamper readability ? Feedback is appreciated.

As documented in the commit message, the 'entity' char array length is
increased to hold more characters for the log output. Not doing so,
causes kernel stack corruption at runtime.

Based on top of:
- [PATCH v2 0/8] staging: vc04: Drop custom logging based on printk

Umang Jain (1):
  staging: vc04_services: Use %p4cc format modifier to print FourCC
    codes

 .../interface/vchiq_arm/vchiq_arm.c           | 20 +++++-----
 .../interface/vchiq_arm/vchiq_core.c          | 40 +++++++++----------
 .../interface/vchiq_arm/vchiq_core.h          |  6 ---
 .../interface/vchiq_arm/vchiq_dev.c           |  7 ++--
 4 files changed, 33 insertions(+), 40 deletions(-)

-- 
2.41.0

[PATCH 1/1] staging: vc04_services: Use %p4cc format modifier to print FourCC codes

[Umang Jain]

Drop VCHIQ_FOURCC_AS_4CHARS macro in favour of %p4cc format
modifier to print FourCC codes in the logs.

vchiq_use_internal() and vchiq_release_internal() uses entity
character-array to store a transient string that contains
a FourCC code. Increase the length of entity array(to 64 bytes)
since %p4cc requires more bytes to hold the output characters.

Suggested-by: Kieran Bingham <kieran.bingham@ideasonboard.com>
Signed-off-by: Umang Jain <umang.jain@ideasonboard.com>
---
 .../interface/vchiq_arm/vchiq_arm.c           | 20 +++++-----
 .../interface/vchiq_arm/vchiq_core.c          | 40 +++++++++----------
 .../interface/vchiq_arm/vchiq_core.h          |  6 ---
 .../interface/vchiq_arm/vchiq_dev.c           |  7 ++--
 4 files changed, 33 insertions(+), 40 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 fc6d33ec5e95..de6a24304a4d 100644
--- a/drivers/staging/vc04_services/interface/vchiq_arm/vchiq_arm.c
+++ b/drivers/staging/vc04_services/interface/vchiq_arm/vchiq_arm.c
@@ -1441,7 +1441,7 @@ vchiq_use_internal(struct vchiq_state *state, struct vchiq_service *service,
 {
 	struct vchiq_arm_state *arm_state = vchiq_platform_get_arm_state(state);
 	int ret = 0;
-	char entity[16];
+	char entity[64];
 	int *entity_uc;
 	int local_uc;
 
@@ -1454,8 +1454,8 @@ vchiq_use_internal(struct vchiq_state *state, struct vchiq_service *service,
 		sprintf(entity, "VCHIQ:   ");
 		entity_uc = &arm_state->peer_use_count;
 	} else if (service) {
-		sprintf(entity, "%c%c%c%c:%03d",
-			VCHIQ_FOURCC_AS_4CHARS(service->base.fourcc),
+		sprintf(entity, "%p4cc:%03d",
+			&service->base.fourcc,
 			service->client_id);
 		entity_uc = &service->service_use_count;
 	} else {
@@ -1497,7 +1497,7 @@ vchiq_release_internal(struct vchiq_state *state, struct vchiq_service *service)
 {
 	struct vchiq_arm_state *arm_state = vchiq_platform_get_arm_state(state);
 	int ret = 0;
-	char entity[16];
+	char entity[64];
 	int *entity_uc;
 
 	if (!arm_state) {
@@ -1506,8 +1506,8 @@ vchiq_release_internal(struct vchiq_state *state, struct vchiq_service *service)
 	}
 
 	if (service) {
-		sprintf(entity, "%c%c%c%c:%03d",
-			VCHIQ_FOURCC_AS_4CHARS(service->base.fourcc),
+		sprintf(entity, "%p4cc:%03d",
+			&service->base.fourcc,
 			service->client_id);
 		entity_uc = &service->service_use_count;
 	} else {
@@ -1713,8 +1713,8 @@ vchiq_dump_service_use_state(struct vchiq_state *state)
 
 	for (i = 0; i < found; i++) {
 		vchiq_log_warning(state->dev, VCHIQ_SUSPEND,
-				  "%c%c%c%c:%d service count %d %s",
-				  VCHIQ_FOURCC_AS_4CHARS(service_data[i].fourcc),
+				  "%p4cc:%d service count %d %s",
+				  &service_data[i].fourcc,
 				  service_data[i].clientid, service_data[i].use_count,
 				  service_data[i].use_count ? nz : "");
 	}
@@ -1743,8 +1743,8 @@ vchiq_check_service(struct vchiq_service *service)
 
 	if (ret) {
 		vchiq_log_error(service->state->dev, VCHIQ_SUSPEND,
-				"%s ERROR - %c%c%c%c:%d service count %d, state count %d", __func__,
-				VCHIQ_FOURCC_AS_4CHARS(service->base.fourcc), service->client_id,
+				"%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);
 		vchiq_dump_service_use_state(service->state);
 	}
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 45ba8f509a84..39b857da2d42 100644
--- a/drivers/staging/vc04_services/interface/vchiq_arm/vchiq_core.c
+++ b/drivers/staging/vc04_services/interface/vchiq_arm/vchiq_core.c
@@ -1112,9 +1112,9 @@ queue_message(struct vchiq_state *state, struct vchiq_service *service,
 			: VCHIQ_MAKE_FOURCC('?', '?', '?', '?');
 
 		vchiq_log_debug(state->dev, VCHIQ_CORE_MSG,
-				"Sent Msg %s(%u) to %c%c%c%c s:%u d:%d len:%zu",
+				"Sent Msg %s(%u) to %p4cc s:%u d:%d len:%zu",
 				msg_type_str(VCHIQ_MSG_TYPE(msgid)), VCHIQ_MSG_TYPE(msgid),
-				VCHIQ_FOURCC_AS_4CHARS(svc_fourcc), VCHIQ_MSG_SRCPORT(msgid),
+				&svc_fourcc, VCHIQ_MSG_SRCPORT(msgid),
 				VCHIQ_MSG_DSTPORT(msgid), size);
 	}
 
@@ -1206,9 +1206,9 @@ queue_message_sync(struct vchiq_state *state, struct vchiq_service *service,
 			     : VCHIQ_MAKE_FOURCC('?', '?', '?', '?');
 
 	vchiq_log_trace(state->dev, VCHIQ_SYNC,
-			"Sent Sync Msg %s(%u) to %c%c%c%c s:%u d:%d len:%d",
+			"Sent Sync Msg %s(%u) to %p4cc s:%u d:%d len:%d",
 			msg_type_str(VCHIQ_MSG_TYPE(msgid)), VCHIQ_MSG_TYPE(msgid),
-			VCHIQ_FOURCC_AS_4CHARS(svc_fourcc), VCHIQ_MSG_SRCPORT(msgid),
+			&svc_fourcc, VCHIQ_MSG_SRCPORT(msgid),
 			VCHIQ_MSG_DSTPORT(msgid), size);
 
 	remote_event_signal(&state->remote->sync_trigger);
@@ -1449,9 +1449,9 @@ abort_outstanding_bulks(struct vchiq_service *service,
 			vchiq_complete_bulk(service->instance, bulk);
 
 			vchiq_log_debug(service->state->dev, VCHIQ_CORE_MSG,
-					"%s %c%c%c%c d:%d ABORTED - tx len:%d, rx len:%d",
+					"%s %p4cc d:%d ABORTED - tx len:%d, rx len:%d",
 					is_tx ? "Send Bulk to" : "Recv Bulk from",
-					VCHIQ_FOURCC_AS_4CHARS(service->base.fourcc),
+					&service->base.fourcc,
 					service->remoteport, bulk->size, bulk->remote_size);
 		} else {
 			/* fabricate a matching dummy bulk */
@@ -1485,8 +1485,8 @@ parse_open(struct vchiq_state *state, struct vchiq_header *header)
 
 	payload = (struct vchiq_open_payload *)header->data;
 	fourcc = payload->fourcc;
-	vchiq_log_debug(state->dev, VCHIQ_CORE, "%d: prs OPEN@%pK (%d->'%c%c%c%c')",
-			state->id, header, localport, VCHIQ_FOURCC_AS_4CHARS(fourcc));
+	vchiq_log_debug(state->dev, VCHIQ_CORE, "%d: prs OPEN@%pK (%d->'%p4cc')",
+			state->id, header, localport, &fourcc);
 
 	service = get_listening_service(state, fourcc);
 	if (!service)
@@ -1498,8 +1498,8 @@ parse_open(struct vchiq_state *state, struct vchiq_header *header)
 
 	if ((service->version < version_min) || (version < service->version_min)) {
 		/* Version mismatch */
-		dev_err(state->dev, "%d: service %d (%c%c%c%c) version mismatch - local (%d, min %d) vs. remote (%d, min %d)",
-			state->id, service->localport, VCHIQ_FOURCC_AS_4CHARS(fourcc),
+		dev_err(state->dev, "%d: service %d (%p4cc) version mismatch - local (%d, min %d) vs. remote (%d, min %d)",
+			state->id, service->localport, &fourcc,
 			service->version, service->version_min, version, version_min);
 		vchiq_service_put(service);
 		service = NULL;
@@ -1632,8 +1632,8 @@ parse_message(struct vchiq_state *state, struct vchiq_header *header)
 			     : VCHIQ_MAKE_FOURCC('?', '?', '?', '?');
 
 	vchiq_log_debug(state->dev, VCHIQ_CORE_MSG,
-			"Rcvd Msg %s(%u) from %c%c%c%c s:%d d:%d len:%d",
-			msg_type_str(type), type, VCHIQ_FOURCC_AS_4CHARS(svc_fourcc),
+			"Rcvd Msg %s(%u) from %p4cc s:%d d:%d len:%d",
+			msg_type_str(type), type, &svc_fourcc,
 			remoteport, localport, size);
 	if (size > 0)
 		vchiq_log_dump_mem(state->dev, "Rcvd", 0, header->data, min(16, size));
@@ -1683,8 +1683,8 @@ parse_message(struct vchiq_state *state, struct vchiq_header *header)
 		if (vchiq_close_service_internal(service, CLOSE_RECVD) == -EAGAIN)
 			goto bail_not_ready;
 
-		vchiq_log_debug(state->dev, VCHIQ_CORE, "Close Service %c%c%c%c s:%u d:%d",
-				VCHIQ_FOURCC_AS_4CHARS(service->base.fourcc),
+		vchiq_log_debug(state->dev, VCHIQ_CORE, "Close Service %p4cc s:%u d:%d",
+				&service->base.fourcc,
 				service->localport, service->remoteport);
 		break;
 	case VCHIQ_MSG_DATA:
@@ -2044,8 +2044,8 @@ sync_func(void *v)
 				     : VCHIQ_MAKE_FOURCC('?', '?', '?', '?');
 
 		vchiq_log_trace(state->dev, VCHIQ_SYNC,
-				"Rcvd Msg %s from %c%c%c%c s:%d d:%d len:%d",
-				msg_type_str(type), VCHIQ_FOURCC_AS_4CHARS(svc_fourcc),
+				"Rcvd Msg %s from %p4cc s:%d d:%d len:%d",
+				msg_type_str(type), &svc_fourcc,
 				remoteport, localport, size);
 		if (size > 0)
 			vchiq_log_dump_mem(state->dev, "Rcvd", 0, header->data, min(16, size));
@@ -2462,9 +2462,9 @@ vchiq_add_service_internal(struct vchiq_state *state,
 	/* Bring this service online */
 	set_service_state(service, srvstate);
 
-	vchiq_log_debug(state->dev, VCHIQ_CORE_MSG, "%s Service %c%c%c%c SrcPort:%d",
+	vchiq_log_debug(state->dev, VCHIQ_CORE_MSG, "%s Service %p4cc SrcPort:%d",
 			(srvstate == VCHIQ_SRVSTATE_OPENING) ? "Open" : "Add",
-			VCHIQ_FOURCC_AS_4CHARS(params->fourcc), service->localport);
+			&params->fourcc, service->localport);
 
 	/* Don't unlock the service - leave it with a ref_count of 1. */
 
@@ -3543,8 +3543,8 @@ int vchiq_dump_service_state(void *dump_context, struct vchiq_service *service)
 		}
 
 		len += scnprintf(buf + len, sizeof(buf) - len,
-				 " '%c%c%c%c' remote %s (msg use %d/%d, slot use %d/%d)",
-				 VCHIQ_FOURCC_AS_4CHARS(fourcc), remoteport,
+				 " '%p4cc' remote %s (msg use %d/%d, slot use %d/%d)",
+				 &fourcc, remoteport,
 				 quota->message_use_count, quota->message_quota,
 				 quota->slot_use_count, quota->slot_quota);
 
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 10451d6765bd..161358db457c 100644
--- a/drivers/staging/vc04_services/interface/vchiq_arm/vchiq_core.h
+++ b/drivers/staging/vc04_services/interface/vchiq_arm/vchiq_core.h
@@ -74,12 +74,6 @@ static inline const char *log_category_str(enum vchiq_log_category c)
 #define VCHIQ_SLOT_ZERO_SLOTS  DIV_ROUND_UP(sizeof(struct vchiq_slot_zero), \
 					    VCHIQ_SLOT_SIZE)
 
-#define VCHIQ_FOURCC_AS_4CHARS(fourcc)	\
-	((fourcc) >> 24) & 0xff, \
-	((fourcc) >> 16) & 0xff, \
-	((fourcc) >>  8) & 0xff, \
-	(fourcc) & 0xff
-
 #define BITSET_SIZE(b)        ((b + 31) >> 5)
 #define BITSET_WORD(b)        (b >> 5)
 #define BITSET_BIT(b)         (1 << (b & 31))
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 b6b5804689fa..0bc93f48c14c 100644
--- a/drivers/staging/vc04_services/interface/vchiq_arm/vchiq_dev.c
+++ b/drivers/staging/vc04_services/interface/vchiq_arm/vchiq_dev.c
@@ -701,13 +701,12 @@ vchiq_ioctl(struct file *file, unsigned int cmd, unsigned long arg)
 				vchiq_release_service_internal(service);
 			if (ret) {
 				vchiq_log_error(instance->state->dev, VCHIQ_SUSPEND,
-						"%s: cmd %s returned error %ld for service %c%c%c%c:%03d",
+						"%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,
-					VCHIQ_FOURCC_AS_4CHARS(service->base.fourcc),
-					service->client_id);
+						ret, &service->base.fourcc,
+						service->client_id);
 			}
 		} else {
 			ret = -EINVAL;
-- 
2.41.0

Re: [PATCH 1/1] staging: vc04_services: Use %p4cc format modifier to print FourCC codes

[Dan Carpenter]

On Wed, Oct 25, 2023 at 02:07:17AM -0400, Umang Jain wrote:
> Drop VCHIQ_FOURCC_AS_4CHARS macro in favour of %p4cc format
> modifier to print FourCC codes in the logs.
> 
> vchiq_use_internal() and vchiq_release_internal() uses entity
> character-array to store a transient string that contains
> a FourCC code. Increase the length of entity array(to 64 bytes)
> since %p4cc requires more bytes to hold the output characters.
> 
> Suggested-by: Kieran Bingham <kieran.bingham@ideasonboard.com>
> Signed-off-by: Umang Jain <umang.jain@ideasonboard.com>
> ---
>  .../interface/vchiq_arm/vchiq_arm.c           | 20 +++++-----
>  .../interface/vchiq_arm/vchiq_core.c          | 40 +++++++++----------
>  .../interface/vchiq_arm/vchiq_core.h          |  6 ---
>  .../interface/vchiq_arm/vchiq_dev.c           |  7 ++--
>  4 files changed, 33 insertions(+), 40 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 fc6d33ec5e95..de6a24304a4d 100644
> --- a/drivers/staging/vc04_services/interface/vchiq_arm/vchiq_arm.c
> +++ b/drivers/staging/vc04_services/interface/vchiq_arm/vchiq_arm.c
> @@ -1441,7 +1441,7 @@ vchiq_use_internal(struct vchiq_state *state, struct vchiq_service *service,
>  {
>  	struct vchiq_arm_state *arm_state = vchiq_platform_get_arm_state(state);
>  	int ret = 0;
> -	char entity[16];
> +	char entity[64];
>  	int *entity_uc;
>  	int local_uc;
>  
> @@ -1454,8 +1454,8 @@ vchiq_use_internal(struct vchiq_state *state, struct vchiq_service *service,
>  		sprintf(entity, "VCHIQ:   ");
>  		entity_uc = &arm_state->peer_use_count;
>  	} else if (service) {
> -		sprintf(entity, "%c%c%c%c:%03d",
> -			VCHIQ_FOURCC_AS_4CHARS(service->base.fourcc),
> +		sprintf(entity, "%p4cc:%03d",

Not related to your patch but these sprintfs() make me very
uncomfortable.

KTODO: change sprintf() to snprintf() in staging/vc04_services/

regards,
dan carpenter

Re: [PATCH 1/1] staging: vc04_services: Use %p4cc format modifier to print FourCC codes

[Sakari Ailus]

On Wed, Oct 25, 2023 at 10:10:08AM +0300, Dan Carpenter wrote:
> On Wed, Oct 25, 2023 at 02:07:17AM -0400, Umang Jain wrote:
> > Drop VCHIQ_FOURCC_AS_4CHARS macro in favour of %p4cc format
> > modifier to print FourCC codes in the logs.
> > 
> > vchiq_use_internal() and vchiq_release_internal() uses entity
> > character-array to store a transient string that contains
> > a FourCC code. Increase the length of entity array(to 64 bytes)
> > since %p4cc requires more bytes to hold the output characters.
> > 
> > Suggested-by: Kieran Bingham <kieran.bingham@ideasonboard.com>
> > Signed-off-by: Umang Jain <umang.jain@ideasonboard.com>
> > ---
> >  .../interface/vchiq_arm/vchiq_arm.c           | 20 +++++-----
> >  .../interface/vchiq_arm/vchiq_core.c          | 40 +++++++++----------
> >  .../interface/vchiq_arm/vchiq_core.h          |  6 ---
> >  .../interface/vchiq_arm/vchiq_dev.c           |  7 ++--
> >  4 files changed, 33 insertions(+), 40 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 fc6d33ec5e95..de6a24304a4d 100644
> > --- a/drivers/staging/vc04_services/interface/vchiq_arm/vchiq_arm.c
> > +++ b/drivers/staging/vc04_services/interface/vchiq_arm/vchiq_arm.c
> > @@ -1441,7 +1441,7 @@ vchiq_use_internal(struct vchiq_state *state, struct vchiq_service *service,
> >  {
> >  	struct vchiq_arm_state *arm_state = vchiq_platform_get_arm_state(state);
> >  	int ret = 0;
> > -	char entity[16];
> > +	char entity[64];
> >  	int *entity_uc;
> >  	int local_uc;
> >  
> > @@ -1454,8 +1454,8 @@ vchiq_use_internal(struct vchiq_state *state, struct vchiq_service *service,
> >  		sprintf(entity, "VCHIQ:   ");
> >  		entity_uc = &arm_state->peer_use_count;
> >  	} else if (service) {
> > -		sprintf(entity, "%c%c%c%c:%03d",
> > -			VCHIQ_FOURCC_AS_4CHARS(service->base.fourcc),
> > +		sprintf(entity, "%p4cc:%03d",
> 
> Not related to your patch but these sprintfs() make me very
> uncomfortable.
> 
> KTODO: change sprintf() to snprintf() in staging/vc04_services/

Umang: how about one patch on top of this? :-) There are just five
instances of it.

-- 
Sakari Ailus

Re: [PATCH 1/1] staging: vc04_services: Use %p4cc format modifier to print FourCC codes

[Umang Jain]

On 10/25/23 2:58 PM, Sakari Ailus wrote:
> On Wed, Oct 25, 2023 at 10:10:08AM +0300, Dan Carpenter wrote:
>> On Wed, Oct 25, 2023 at 02:07:17AM -0400, Umang Jain wrote:
>>> Drop VCHIQ_FOURCC_AS_4CHARS macro in favour of %p4cc format
>>> modifier to print FourCC codes in the logs.
>>>
>>> vchiq_use_internal() and vchiq_release_internal() uses entity
>>> character-array to store a transient string that contains
>>> a FourCC code. Increase the length of entity array(to 64 bytes)
>>> since %p4cc requires more bytes to hold the output characters.
>>>
>>> Suggested-by: Kieran Bingham <kieran.bingham@ideasonboard.com>
>>> Signed-off-by: Umang Jain <umang.jain@ideasonboard.com>
>>> ---
>>>   .../interface/vchiq_arm/vchiq_arm.c           | 20 +++++-----
>>>   .../interface/vchiq_arm/vchiq_core.c          | 40 +++++++++----------
>>>   .../interface/vchiq_arm/vchiq_core.h          |  6 ---
>>>   .../interface/vchiq_arm/vchiq_dev.c           |  7 ++--
>>>   4 files changed, 33 insertions(+), 40 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 fc6d33ec5e95..de6a24304a4d 100644
>>> --- a/drivers/staging/vc04_services/interface/vchiq_arm/vchiq_arm.c
>>> +++ b/drivers/staging/vc04_services/interface/vchiq_arm/vchiq_arm.c
>>> @@ -1441,7 +1441,7 @@ vchiq_use_internal(struct vchiq_state *state, struct vchiq_service *service,
>>>   {
>>>   	struct vchiq_arm_state *arm_state = vchiq_platform_get_arm_state(state);
>>>   	int ret = 0;
>>> -	char entity[16];
>>> +	char entity[64];
>>>   	int *entity_uc;
>>>   	int local_uc;
>>>   
>>> @@ -1454,8 +1454,8 @@ vchiq_use_internal(struct vchiq_state *state, struct vchiq_service *service,
>>>   		sprintf(entity, "VCHIQ:   ");
>>>   		entity_uc = &arm_state->peer_use_count;
>>>   	} else if (service) {
>>> -		sprintf(entity, "%c%c%c%c:%03d",
>>> -			VCHIQ_FOURCC_AS_4CHARS(service->base.fourcc),
>>> +		sprintf(entity, "%p4cc:%03d",
>> Not related to your patch but these sprintfs() make me very
>> uncomfortable.
>>
>> KTODO: change sprintf() to snprintf() in staging/vc04_services/
> Umang: how about one patch on top of this? :-) There are just five
> instances of it.

Ricardo, how about this? Do you want to take a swing at this ? :-)

And send a v2. ?
>

Re: [PATCH 1/1] staging: vc04_services: Use %p4cc format modifier to print FourCC codes

[Greg Kroah-Hartman]

On Wed, Oct 25, 2023 at 03:04:04PM +0530, Umang Jain wrote:
> 
> 
> On 10/25/23 2:58 PM, Sakari Ailus wrote:
> > On Wed, Oct 25, 2023 at 10:10:08AM +0300, Dan Carpenter wrote:
> > > On Wed, Oct 25, 2023 at 02:07:17AM -0400, Umang Jain wrote:
> > > > Drop VCHIQ_FOURCC_AS_4CHARS macro in favour of %p4cc format
> > > > modifier to print FourCC codes in the logs.
> > > > 
> > > > vchiq_use_internal() and vchiq_release_internal() uses entity
> > > > character-array to store a transient string that contains
> > > > a FourCC code. Increase the length of entity array(to 64 bytes)
> > > > since %p4cc requires more bytes to hold the output characters.
> > > > 
> > > > Suggested-by: Kieran Bingham <kieran.bingham@ideasonboard.com>
> > > > Signed-off-by: Umang Jain <umang.jain@ideasonboard.com>
> > > > ---
> > > >   .../interface/vchiq_arm/vchiq_arm.c           | 20 +++++-----
> > > >   .../interface/vchiq_arm/vchiq_core.c          | 40 +++++++++----------
> > > >   .../interface/vchiq_arm/vchiq_core.h          |  6 ---
> > > >   .../interface/vchiq_arm/vchiq_dev.c           |  7 ++--
> > > >   4 files changed, 33 insertions(+), 40 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 fc6d33ec5e95..de6a24304a4d 100644
> > > > --- a/drivers/staging/vc04_services/interface/vchiq_arm/vchiq_arm.c
> > > > +++ b/drivers/staging/vc04_services/interface/vchiq_arm/vchiq_arm.c
> > > > @@ -1441,7 +1441,7 @@ vchiq_use_internal(struct vchiq_state *state, struct vchiq_service *service,
> > > >   {
> > > >   	struct vchiq_arm_state *arm_state = vchiq_platform_get_arm_state(state);
> > > >   	int ret = 0;
> > > > -	char entity[16];
> > > > +	char entity[64];
> > > >   	int *entity_uc;
> > > >   	int local_uc;
> > > > @@ -1454,8 +1454,8 @@ vchiq_use_internal(struct vchiq_state *state, struct vchiq_service *service,
> > > >   		sprintf(entity, "VCHIQ:   ");
> > > >   		entity_uc = &arm_state->peer_use_count;
> > > >   	} else if (service) {
> > > > -		sprintf(entity, "%c%c%c%c:%03d",
> > > > -			VCHIQ_FOURCC_AS_4CHARS(service->base.fourcc),
> > > > +		sprintf(entity, "%p4cc:%03d",
> > > Not related to your patch but these sprintfs() make me very
> > > uncomfortable.
> > > 
> > > KTODO: change sprintf() to snprintf() in staging/vc04_services/
> > Umang: how about one patch on top of this? :-) There are just five
> > instances of it.
> 
> Ricardo, how about this? Do you want to take a swing at this ? :-)
> 
> And send a v2. ?

No, that would be a separate change, this one is fine as-is.

thanks,

greg k-h

Re: [PATCH 1/1] staging: vc04_services: Use %p4cc format modifier to print FourCC codes

[Ricardo B. Marliere]

On 23/10/25 03:04PM, Umang Jain wrote:
> 
> 
> On 10/25/23 2:58 PM, Sakari Ailus wrote:
> > On Wed, Oct 25, 2023 at 10:10:08AM +0300, Dan Carpenter wrote:
> >> On Wed, Oct 25, 2023 at 02:07:17AM -0400, Umang Jain wrote:
> >>> Drop VCHIQ_FOURCC_AS_4CHARS macro in favour of %p4cc format
> >>> modifier to print FourCC codes in the logs.
> >>>
> >>> vchiq_use_internal() and vchiq_release_internal() uses entity
> >>> character-array to store a transient string that contains
> >>> a FourCC code. Increase the length of entity array(to 64 bytes)
> >>> since %p4cc requires more bytes to hold the output characters.
> >>>
> >>> Suggested-by: Kieran Bingham <kieran.bingham@ideasonboard.com>
> >>> Signed-off-by: Umang Jain <umang.jain@ideasonboard.com>
> >>> ---
> >>>   .../interface/vchiq_arm/vchiq_arm.c           | 20 +++++-----
> >>>   .../interface/vchiq_arm/vchiq_core.c          | 40 +++++++++----------
> >>>   .../interface/vchiq_arm/vchiq_core.h          |  6 ---
> >>>   .../interface/vchiq_arm/vchiq_dev.c           |  7 ++--
> >>>   4 files changed, 33 insertions(+), 40 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 fc6d33ec5e95..de6a24304a4d 100644
> >>> --- a/drivers/staging/vc04_services/interface/vchiq_arm/vchiq_arm.c
> >>> +++ b/drivers/staging/vc04_services/interface/vchiq_arm/vchiq_arm.c
> >>> @@ -1441,7 +1441,7 @@ vchiq_use_internal(struct vchiq_state *state, struct vchiq_service *service,
> >>>   {
> >>>   	struct vchiq_arm_state *arm_state = vchiq_platform_get_arm_state(state);
> >>>   	int ret = 0;
> >>> -	char entity[16];
> >>> +	char entity[64];
> >>>   	int *entity_uc;
> >>>   	int local_uc;
> >>>   
> >>> @@ -1454,8 +1454,8 @@ vchiq_use_internal(struct vchiq_state *state, struct vchiq_service *service,
> >>>   		sprintf(entity, "VCHIQ:   ");
> >>>   		entity_uc = &arm_state->peer_use_count;
> >>>   	} else if (service) {
> >>> -		sprintf(entity, "%c%c%c%c:%03d",
> >>> -			VCHIQ_FOURCC_AS_4CHARS(service->base.fourcc),
> >>> +		sprintf(entity, "%p4cc:%03d",
> >> Not related to your patch but these sprintfs() make me very
> >> uncomfortable.
> >>
> >> KTODO: change sprintf() to snprintf() in staging/vc04_services/
> > Umang: how about one patch on top of this? :-) There are just five
> > instances of it.
> 
> Ricardo, how about this? Do you want to take a swing at this ? :-)

Sure thing, I will send as another patch as Greg suggested.

Thank you,
-	Ricardo

Re: [PATCH 0/1] staging: vc04_services: Use %p4cc to print fourcc

[Stefan Wahren]

Hi,

[add Raspberry Pi guys]

Am 25.10.23 um 08:07 schrieb Umang Jain:
> The following patch drop VCHIQ_FOURCC_AS_4CHARS macro in favour of %p4cc
> format modifier to print FourCC codes in the logs.
>
> *Before this patch*
> `mailbox: vchiq_core_msg debug: Sent Msg DATA(5) to AUDS s:1 d:62 len:20`
>
> *After this patch*
> bcm2835_vchiq 3f00b840.mailbox: vchiq_core_msg debug: Sent Msg DATA(5) to SDUA little-endian (0x41554453) s:1 d:62 len:20
>
> The inversion of AUDS to SDUA as per usage of %p4cc
> Does it hamper readability ? Feedback is appreciated.
looks good to me. Any concerns Phil or Dave?
>
> As documented in the commit message, the 'entity' char array length is
> increased to hold more characters for the log output. Not doing so,
> causes kernel stack corruption at runtime.
>
> Based on top of:
> - [PATCH v2 0/8] staging: vc04: Drop custom logging based on printk
>
> Umang Jain (1):
>    staging: vc04_services: Use %p4cc format modifier to print FourCC
>      codes
>
>   .../interface/vchiq_arm/vchiq_arm.c           | 20 +++++-----
>   .../interface/vchiq_arm/vchiq_core.c          | 40 +++++++++----------
>   .../interface/vchiq_arm/vchiq_core.h          |  6 ---
>   .../interface/vchiq_arm/vchiq_dev.c           |  7 ++--
>   4 files changed, 33 insertions(+), 40 deletions(-)
>