Date: Tue, 19 Sep 2017 13:02:54 +0200
From: Greg KH <gregkh@linuxfoundation.org>
To: Benjamin Gaignard <benjamin.gaignard@linaro.org>
Cc: labbott@redhat.com, sumit.semwal@linaro.org, arve@android.com,
        riandrews@android.com, broonie@kernel.org, dan.carpenter@oracle.com,
        devel@driverdev.osuosl.org, linux-kernel@vger.kernel.org
Subject: Re: [PATCH v2 2/2] staging: ion: create one device entry per heap
Message-ID: <20170919110254.GB4511@kroah.com>
References: <1505816738-30017-1-git-send-email-benjamin.gaignard@linaro.org>
 <1505816738-30017-3-git-send-email-benjamin.gaignard@linaro.org>
MIME-Version: 1.0
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
In-Reply-To: <1505816738-30017-3-git-send-email-benjamin.gaignard@linaro.org>
User-Agent: Mutt/1.9.0 (2017-09-02)
Sender: linux-kernel-owner@vger.kernel.org
Content-Length: 3825
Lines: 119

On Tue, Sep 19, 2017 at 12:25:38PM +0200, Benjamin Gaignard wrote:
> Instead a getting one common device "/dev/ion" for
> all the heaps this patch allow to create one device
> entry ("/dev/ionX") per heap.
> Getting an entry per heap could allow to set security rules
> per heap and global ones for all heaps.
> 
> Allocation requests will be only allowed if the mask_id
> match with device minor.
> Query request could be done on any of the devices.
> 
> Signed-off-by: Benjamin Gaignard <benjamin.gaignard@linaro.org>
> ---
>  drivers/staging/android/ion/ion-ioctl.c | 11 +++++++++--
>  drivers/staging/android/ion/ion.c       | 23 +++++++++++++++++------
>  drivers/staging/android/ion/ion.h       | 10 +++++++---
>  3 files changed, 33 insertions(+), 11 deletions(-)
> 
> diff --git a/drivers/staging/android/ion/ion-ioctl.c b/drivers/staging/android/ion/ion-ioctl.c
> index e26b786..c8c906c 100644
> --- a/drivers/staging/android/ion/ion-ioctl.c
> +++ b/drivers/staging/android/ion/ion-ioctl.c
> @@ -25,8 +25,11 @@ union ion_ioctl_arg {
>  	struct ion_heap_query query;
>  };
>  
> -static int validate_ioctl_arg(unsigned int cmd, union ion_ioctl_arg *arg)
> +static int validate_ioctl_arg(struct file *filp,
> +			      unsigned int cmd, union ion_ioctl_arg *arg)
>  {
> +	int mask = 1 << iminor(filp->f_inode);
> +
>  	switch (cmd) {
>  	case ION_IOC_HEAP_QUERY:
>  		if (arg->query.reserved0 ||
> @@ -34,6 +37,10 @@ static int validate_ioctl_arg(unsigned int cmd, union ion_ioctl_arg *arg)
>  		    arg->query.reserved2 )
>  			return -EINVAL;
>  		break;
> +	case ION_IOC_ALLOC:
> +		if (!(arg->allocation.heap_id_mask & mask))
> +			return -EINVAL;
> +		break;
>  	default:
>  		break;
>  	}
> @@ -69,7 +76,7 @@ long ion_ioctl(struct file *filp, unsigned int cmd, unsigned long arg)
>  	if (copy_from_user(&data, (void __user *)arg, _IOC_SIZE(cmd)))
>  		return -EFAULT;
>  
> -	ret = validate_ioctl_arg(cmd, &data);
> +	ret = validate_ioctl_arg(filp, cmd, &data);
>  	if (WARN_ON_ONCE(ret))
>  		return ret;
>  
> diff --git a/drivers/staging/android/ion/ion.c b/drivers/staging/android/ion/ion.c
> index 93e2c90..3f8b595 100644
> --- a/drivers/staging/android/ion/ion.c
> +++ b/drivers/staging/android/ion/ion.c
> @@ -40,6 +40,8 @@
>  
>  #include "ion.h"
>  
> +#define ION_DEV_MAX 32
> +
>  static struct ion_device *internal_dev;
>  static int heap_id;
>  
> @@ -541,11 +543,24 @@ void ion_device_add_heap(struct ion_heap *heap)
>  {
>  	struct dentry *debug_file;
>  	struct ion_device *dev = internal_dev;
> +	int ret;
>  
>  	if (!heap->ops->allocate || !heap->ops->free)
>  		pr_err("%s: can not add heap with invalid ops struct.\n",
>  		       __func__);
>  
> +	if (heap_id >= ION_DEV_MAX)
> +		return -EBUSY;
> +
> +	heap->ddev.devt = MKDEV(MAJOR(dev->devt), heap_id);
> +	dev_set_name(&heap->ddev, "ion%d", heap_id);
> +	device_initialize(&heap->ddev);
> +	cdev_init(&heap->chrdev, &ion_fops);
> +	heap->chrdev.owner = THIS_MODULE;
> +	ret = cdev_device_add(&heap->chrdev, &heap->ddev);
> +	if (ret < 0)
> +		return;

No cleanup needed?  No reporting an error happened back up the chain?
Not nice :(

> +
>  	spin_lock_init(&heap->free_lock);
>  	heap->free_list_size = 0;
>  
> @@ -595,13 +610,9 @@ static int ion_device_create(void)
>  	if (!idev)
>  		return -ENOMEM;
>  
> -	idev->dev.minor = MISC_DYNAMIC_MINOR;
> -	idev->dev.name = "ion";
> -	idev->dev.fops = &ion_fops;
> -	idev->dev.parent = NULL;
> -	ret = misc_register(&idev->dev);
> +	ret = alloc_chrdev_region(&idev->devt, 0, ION_DEV_MAX, "ion");

Did you just change the major number for the device node as well?

Wow, that's a lot of userspace breakage (both major number, and name),
how did you test this?

Have you gotten "upstream" to agree to these changes?  We can't take
these until they think it's ok as well.

thanks,

greg k-h