RE: [dpdk-dev v3 1/1] lib/cryptodev: multi-process IPC request handler

["Power, Ciara" <ciara.power@intel.com>]

Hi Kai,

Some small comments below.

> -----Original Message-----
> From: Kai Ji <kai.ji@intel.com>
> Sent: Sunday 2 October 2022 23:45
> To: dev@dpdk.org
> Cc: gakhil@marvell.com; Ji, Kai <kai.ji@intel.com>
> Subject: [dpdk-dev v3 1/1] lib/cryptodev: multi-process IPC request handler
> 
> This patch add a function to support queue-pair configuration request to allow
> the primary or secondary process to setup/free the queue-pair via IPC handler.
> Add in queue pair in-used by process id array in rte_cryptodev_data for pid
> tracking.
> 
> Signed-off-by: Kai Ji <kai.ji@intel.com>
<snip>
> +static int
> +rte_cryptodev_ipc_request(const struct rte_mp_msg *mp_msg, const void
> +*peer) {
> +	struct rte_mp_msg mp_res;
> +	struct rte_cryptodev_mp_param *res =
> +		(struct rte_cryptodev_mp_param *)mp_res.param;
> +	const struct rte_cryptodev_mp_param *param =
> +		(const struct rte_cryptodev_mp_param *)mp_msg->param;
[CP] 
The "param" name could be improved I think - maybe follow the "res" naming and call this one "msg"
Both are "mp_***->param" so I think it is confusing to have one named "param"

> +
> +	int ret;
> +	struct rte_cryptodev *dev;
> +	uint16_t *qps_in_used_by_pid;
[CP] 
Possible typo -   change to "qp_in_use_by_pid" to match the naming of "dev->data->qp_in_use_by_pid"

<snip>
> +int rte_cryptodev_mp_request_register(void)
> +{
> +	RTE_ASSERT(rte_eal_process_type() == RTE_PROC_PRIMARY);
> +	return rte_mp_action_register(CRYPTODEV_MP_REQ,
> +				rte_cryptodev_ipc_request);
> +}
> +
> +void rte_cryptodev_mp_request_unregister(void)
> +{
> +	RTE_ASSERT(rte_eal_process_type() == RTE_PROC_PRIMARY);
> +	rte_mp_action_unregister(CRYPTODEV_MP_REQ);
> +}
> +
[CP] 
The above two functions should have the function return type on its own line.

<snip>
> +/**
> + * Register multi process request IPC handler
> + *
> + * @return
> + *   - 0: Success registered
> + *	 - 1: Failed registration failed
> + *	 - -EINVAL: device was not configured
[CP] 
Indent is off for success result

<snip>

Thanks,
Ciara