Received-SPF: pass (google.com: domain of linux-kernel+bounces-64846-linux.lists.archive=gmail.com@vger.kernel.org designates 2604:1380:45e3:2400::1 as permitted sender) client-ip=2604:1380:45e3:2400::1;
Message-ID: <0e03d788-a4d1-494e-a103-9f57ad81b016@linaro.org>
Date: Wed, 14 Feb 2024 07:46:46 +0000
Precedence: bulk
MIME-Version: 1.0
User-Agent: Mozilla Thunderbird
Subject: Re: [PATCH v1 04/16] misc: fastrpc: Add fastrpc multimode invoke
 request support
Content-Language: en-US
To: Ekansh Gupta <quic_ekangupt@quicinc.com>, linux-arm-msm@vger.kernel.org
Cc: gregkh@linuxfoundation.org, linux-kernel@vger.kernel.org
References: <20240202064039.15505-1-quic_ekangupt@quicinc.com>
 <20240202064039.15505-5-quic_ekangupt@quicinc.com>
From: Srinivas Kandagatla <srinivas.kandagatla@linaro.org>
In-Reply-To: <20240202064039.15505-5-quic_ekangupt@quicinc.com>
Content-Type: text/plain; charset=UTF-8; format=flowed
Content-Transfer-Encoding: 7bit


On 02/02/2024 06:40, Ekansh Gupta wrote:
> Multimode invocation request is intended to support multiple
> different type of requests. This will include enhanced invoke
> request to support CRC check and performance counter enablement.
> This will also support few driver level user controllable
> mechanisms like usage of shared context banks, wakelock support,
> etc. This IOCTL is also added with the aim to support few
> new fastrpc features like DSP PD notification framework,
> DSP Signalling mechanism etc.
> 
> Signed-off-by: Ekansh Gupta <quic_ekangupt@quicinc.com>
> ---
>   drivers/misc/fastrpc.c      | 176 ++++++++++++++++++++++++++----------
>   include/uapi/misc/fastrpc.h |  26 ++++++
>   2 files changed, 154 insertions(+), 48 deletions(-)
> 
..

> diff --git a/include/uapi/misc/fastrpc.h b/include/uapi/misc/fastrpc.h
> index f33d914d8f46..45c15be1de58 100644
> --- a/include/uapi/misc/fastrpc.h
> +++ b/include/uapi/misc/fastrpc.h
> @@ -16,6 +16,7 @@
>   #define FASTRPC_IOCTL_INIT_CREATE_STATIC _IOWR('R', 9, struct fastrpc_init_create_static)
>   #define FASTRPC_IOCTL_MEM_MAP		_IOWR('R', 10, struct fastrpc_mem_map)
>   #define FASTRPC_IOCTL_MEM_UNMAP		_IOWR('R', 11, struct fastrpc_mem_unmap)
> +#define FASTRPC_IOCTL_MULTIMODE_INVOKE	_IOWR('R', 12, struct fastrpc_ioctl_multimode_invoke)
>   #define FASTRPC_IOCTL_GET_DSP_INFO	_IOWR('R', 13, struct fastrpc_ioctl_capability)
>   
>   /**
> @@ -80,6 +81,31 @@ struct fastrpc_invoke {
>   	__u64 args;
>   };
>   
> +struct fastrpc_enhanced_invoke {
> +	struct fastrpc_invoke inv;
> +	__u64 crc;
> +	__u64 perf_kernel;
> +	__u64 perf_dsp;
> +	__u32 reserved[8];	/* keeping reserved bits for new requirements */
> +};
> +
> +struct fastrpc_ioctl_multimode_invoke {
This struct needs some documentation.

> +	__u32 req;
we use req here and then in next few lines we define the same as 
fastrpc_multimode_invoke_type. I would recommend to make this type 
instead of req.

> +	__u32 rsvd;		/* padding field */
reserved?

<---
> +	__u64 invparam;
> +	__u64 size;
-->
Isn't size obvious when we know request type?

This is also opening up a path for userspace to pass some random structures.

It makes more sense to have a union of all the request structures.

Why not add all the enhanced invoke uapi structures as part of this patch?

> +	__u32 reserved[8];	/* keeping reserved bits for new requirements */
> +};
> +
> +enum fastrpc_multimode_invoke_type {
> +	FASTRPC_INVOKE			= 1,
> +	FASTRPC_INVOKE_ENHANCED	= 2,
> +	FASTRPC_INVOKE_CONTROL = 3,
> +	FASTRPC_INVOKE_DSPSIGNAL = 4,
> +	FASTRPC_INVOKE_NOTIF = 5,
> +	FASTRPC_INVOKE_MULTISESSION = 6,

All of these needs a proper documentation. Its impossible to understand 
what they actually mean.

This applies to all the enums that are added as part of other patches to 
the uapi headers.

thanks,
Srini


> +};
> +
>   struct fastrpc_init_create {
>   	__u32 filelen;	/* elf file length */
>   	__s32 filefd;	/* fd for the file */