Re: [PATCH v2 16/34] staging: mmal-vchiq: Fix client_component for 64 bit kernel

[Hans Verkuil <hverkuil@xs4all.nl>]

On 04/05/2020 11:25, Laurent Pinchart wrote:
> From: Dave Stevenson <dave.stevenson@raspberrypi.org>
> 
> The MMAL client_component field is used with the event
> mechanism to allow the client to identify the component for
> which the event is generated.
> The field is only 32bits in size, therefore we can't use a
> pointer to the component in a 64 bit kernel.
> 
> Component handles are already held in an array per VCHI
> instance, so use the array index as the client_component handle
> to avoid having to create a new IDR for this purpose.
> 
> Signed-off-by: Dave Stevenson <dave.stevenson@raspberrypi.org>
> Signed-off-by: Jacopo Mondi <jacopo@jmondi.org>
> ---
>  .../staging/vc04_services/vchiq-mmal/mmal-vchiq.c    | 12 +++++++++---
>  .../staging/vc04_services/vchiq-mmal/mmal-vchiq.h    |  1 +
>  2 files changed, 10 insertions(+), 3 deletions(-)
> 
> diff --git a/drivers/staging/vc04_services/vchiq-mmal/mmal-vchiq.c b/drivers/staging/vc04_services/vchiq-mmal/mmal-vchiq.c
> index c65c262cffbb..5872690b404d 100644
> --- a/drivers/staging/vc04_services/vchiq-mmal/mmal-vchiq.c
> +++ b/drivers/staging/vc04_services/vchiq-mmal/mmal-vchiq.c
> @@ -467,9 +467,9 @@ buffer_from_host(struct vchiq_mmal_instance *instance,
>  static void event_to_host_cb(struct vchiq_mmal_instance *instance,
>  			     struct mmal_msg *msg, u32 msg_len)
>  {
> -	/* FIXME: Not going to work on 64 bit */
> +	int comp_idx = msg->u.event_to_host.client_component;
>  	struct vchiq_mmal_component *component =
> -		(struct vchiq_mmal_component *)msg->u.event_to_host.client_component;
> +					&instance->component[comp_idx];
>  	struct vchiq_mmal_port *port = NULL;
>  	struct mmal_msg_context *msg_context;
>  	u32 port_num = msg->u.event_to_host.port_num;
> @@ -1068,7 +1068,7 @@ static int create_component(struct vchiq_mmal_instance *instance,
>  
>  	/* build component create message */
>  	m.h.type = MMAL_MSG_TYPE_COMPONENT_CREATE;
> -	m.u.component_create.client_component = (u32)(unsigned long)component;
> +	m.u.component_create.client_component = component->client_component;
>  	strncpy(m.u.component_create.name, name,
>  		sizeof(m.u.component_create.name));

I recommend that another patch is made that replaces any strcpy/strlcpy/strncpy
calls to strscpy (since that's the standard for media drivers).

Regards,

	Hans

>  
> @@ -1863,6 +1863,12 @@ int vchiq_mmal_component_init(struct vchiq_mmal_instance *instance,
>  		goto unlock;
>  	}
>  
> +	/* We need a handle to reference back to our component structure.
> +	 * Use the array index in instance->component rather than rolling
> +	 * another IDR.
> +	 */
> +	component->client_component = idx;
> +
>  	ret = create_component(instance, component, name);
>  	if (ret < 0) {
>  		pr_err("%s: failed to create component %d (Not enough GPU mem?)\n",
> diff --git a/drivers/staging/vc04_services/vchiq-mmal/mmal-vchiq.h b/drivers/staging/vc04_services/vchiq-mmal/mmal-vchiq.h
> index ae6c69ba16ee..247521fbcc1d 100644
> --- a/drivers/staging/vc04_services/vchiq-mmal/mmal-vchiq.h
> +++ b/drivers/staging/vc04_services/vchiq-mmal/mmal-vchiq.h
> @@ -97,6 +97,7 @@ struct vchiq_mmal_component {
>  	struct vchiq_mmal_port input[MAX_PORT_COUNT]; /* input ports */
>  	struct vchiq_mmal_port output[MAX_PORT_COUNT]; /* output ports */
>  	struct vchiq_mmal_port clock[MAX_PORT_COUNT]; /* clock ports */
> +	u32 client_component;	/* Used to ref back to client struct */
>  };
>  
>  int vchiq_mmal_init(struct vchiq_mmal_instance **out_instance);
>