Re: [PATCH v2 07/18] nitro_enclaves: Init misc device providing the ioctl interface

[Greg KH]

On Fri, May 22, 2020 at 09:29:35AM +0300, Andra Paraschiv wrote:
> +static char *ne_cpus;
> +module_param(ne_cpus, charp, 0644);
> +MODULE_PARM_DESC(ne_cpus, "<cpu-list> - CPU pool used for Nitro Enclaves");

This is not the 1990's, don't use module parameters if you can help it.
Why is this needed, and where is it documented?

> +/* CPU pool used for Nitro Enclaves. */
> +struct ne_cpu_pool {
> +	/* Available CPUs in the pool. */
> +	cpumask_var_t avail;
> +	struct mutex mutex;
> +};
> +
> +static struct ne_cpu_pool ne_cpu_pool;
> +
> +static int ne_open(struct inode *node, struct file *file)
> +{
> +	return 0;
> +}

If open does nothing, just don't even provide it.

> +
> +static long ne_ioctl(struct file *file, unsigned int cmd, unsigned long arg)
> +{
> +	switch (cmd) {
> +
> +	default:
> +		return -ENOTTY;
> +	}
> +
> +	return 0;
> +}

Same for ioctl.

> +
> +static int ne_release(struct inode *inode, struct file *file)
> +{
> +	return 0;
> +}

Same for release.

> +
> +static const struct file_operations ne_fops = {
> +	.owner		= THIS_MODULE,
> +	.llseek		= noop_llseek,
> +	.unlocked_ioctl	= ne_ioctl,
> +	.open		= ne_open,
> +	.release	= ne_release,
> +};
> +
> +struct miscdevice ne_miscdevice = {
> +	.minor	= MISC_DYNAMIC_MINOR,
> +	.name	= NE_DEV_NAME,
> +	.fops	= &ne_fops,
> +	.mode	= 0660,
> +};
> +
> +static int __init ne_init(void)
> +{
> +	unsigned int cpu = 0;
> +	unsigned int cpu_sibling = 0;
> +	int rc = -EINVAL;
> +
> +	memset(&ne_cpu_pool, 0, sizeof(ne_cpu_pool));

Why did you just set a structure to 0 that was already initialized by
the system to 0?  Are you sure about this?

> +
> +	if (!zalloc_cpumask_var(&ne_cpu_pool.avail, GFP_KERNEL))
> +		return -ENOMEM;
> +
> +	mutex_init(&ne_cpu_pool.mutex);
> +
> +	rc = cpulist_parse(ne_cpus, ne_cpu_pool.avail);
> +	if (rc < 0) {
> +		pr_err_ratelimited(NE "Error in cpulist parse [rc=%d]\n", rc);

Again, drop all ratelimited stuff please.

thanks,

greg k-h