Re: [PATCH 2/5] misc: fastrpc: Add secure device node support

[Srinivas Kandagatla <srinivas.kandagatla@xxxxxxxxxx>]

Hi Jeya,

Thanks for the patch,

On 25/11/2021 14:15, Jeya R wrote:
> Register and deregister secure device node. Check for device name during
> device open get proper channel context.
>
> Signed-off-by: Jeya R <jeyr@xxxxxxxxxxxxxx>
> ---
>   drivers/misc/fastrpc.c | 33 +++++++++++++++++++++++++++++++--
>   1 file changed, 31 insertions(+), 2 deletions(-)
>
> diff --git a/drivers/misc/fastrpc.c b/drivers/misc/fastrpc.c
> index 39aca77..0775554e 100644
> --- a/drivers/misc/fastrpc.c
> +++ b/drivers/misc/fastrpc.c
> @@ -79,6 +79,7 @@
>   #define SENSORS_PD	(2)
>   
>   #define miscdev_to_cctx(d) container_of(d, struct fastrpc_channel_ctx, miscdev)
> +#define securedev_to_cctx(d) container_of(d, struct fastrpc_channel_ctx, securedev)
>   
>   static const char *domains[FASTRPC_DEV_MAX] = { "adsp", "mdsp",
>   						"sdsp", "cdsp"};
> @@ -213,6 +214,7 @@ struct fastrpc_channel_ctx {
>   	struct idr ctx_idr;
>   	struct list_head users;
>   	struct miscdevice miscdev;
> +	struct miscdevice securedev;
>   	struct kref refcount;
>   };
>   
> @@ -1218,10 +1220,23 @@ static int fastrpc_device_release(struct inode *inode, struct file *file)
>   
>   static int fastrpc_device_open(struct inode *inode, struct file *filp)
>   {
> -	struct fastrpc_channel_ctx *cctx = miscdev_to_cctx(filp->private_data);
> +	struct fastrpc_channel_ctx *cctx = NULL;
>   	struct fastrpc_user *fl = NULL;
> +	struct miscdevice *currdev = NULL;
>   	unsigned long flags;
>   
> +	if (!filp)
> +		return -EFAULT;
> +
> +	currdev = (struct miscdevice *)(filp->private_data);
> +	if (!currdev)
> +		return -EFAULT;
> +
> +	if (strstr(currdev->name, "secure") != NULL)
> +		cctx = securedev_to_cctx(filp->private_data);
> +	else
> +		cctx = miscdev_to_cctx(filp->private_data);
> +


Now we only have one of the two possibilities,
Either device node is "non secure" or a "secure"

If you create just one device node based on the device tree bindings 
then you would not need to do any of these runtime checks.

something like this in fastrpc_rpmsg_probe() should do:

----------------------->cut<----------------------------------
data->secure = !(of_property_read_bool(rdev->of_node, 
"qcom,non-secure-domain"));
data->miscdev.minor = MISC_DYNAMIC_MINOR;
data->miscdev.name = devm_kasprintf(rdev, GFP_KERNEL, "fastrpc-%s-%s",
				    domains[domain_id],
				    data->secure ? "secure" : "");
data->miscdev.fops = &fastrpc_fops;
err = misc_register(&data->miscdev);
if (err) {
	kfree(data);
	return err;
}
----------------------->cut<----------------------------------


--srini

>   	fl = kzalloc(sizeof(*fl), GFP_KERNEL);
>   	if (!fl)
>   		return -ENOMEM;
> @@ -1644,6 +1659,15 @@ static int fastrpc_rpmsg_probe(struct rpmsg_device *rpdev)
>   		kfree(data);
>   		return err;
>   	}
> +	data->securedev.minor = MISC_DYNAMIC_MINOR;
> +	data->securedev.name = devm_kasprintf(rdev, GFP_KERNEL,
> +				"fastrpc-%s-secure", domains[domain_id]);
> +	data->securedev.fops = &fastrpc_fops;
> +	err = misc_register(&data->securedev);
> +	if (err) {
> +		kfree(data);
> +		return err;
> +	}
>   
>   	kref_init(&data->refcount);
>   
> @@ -1655,7 +1679,11 @@ static int fastrpc_rpmsg_probe(struct rpmsg_device *rpdev)
>   	data->domain_id = domain_id;
>   	data->rpdev = rpdev;
>   
> -	return of_platform_populate(rdev->of_node, NULL, NULL, rdev);
> +	err = of_platform_populate(rdev->of_node, NULL, NULL, rdev);
> +	dev_info(rdev, "%s done for %s with nodes non-secure(%d), secure(%d) return: %d\n",
> +		__func__, domains[domain_id],
> +		data->miscdev.minor, data->securedev.minor, err);
> +	return err;
>   }
>   
>   static void fastrpc_notify_users(struct fastrpc_user *user)
> @@ -1680,6 +1708,7 @@ static void fastrpc_rpmsg_remove(struct rpmsg_device *rpdev)
>   	spin_unlock_irqrestore(&cctx->lock, flags);
>   
>   	misc_deregister(&cctx->miscdev);
> +	misc_deregister(&cctx->securedev);
>   	of_platform_depopulate(&rpdev->dev);
>   
>   	cctx->rpdev = NULL;