Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1751750AbdISLzk (ORCPT ); Tue, 19 Sep 2017 07:55:40 -0400 Received: from mail-qt0-f169.google.com ([209.85.216.169]:47351 "EHLO mail-qt0-f169.google.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1751109AbdISLzi (ORCPT ); Tue, 19 Sep 2017 07:55:38 -0400 X-Google-Smtp-Source: AOwi7QDCPV6fNGoBI1UluMmvIKxmLRc9S8Abp14Ybcw+yzBIZmSxZo0BdRvMVIEjixjaxOrscaDRchTVPy8g108Hmfw= MIME-Version: 1.0 In-Reply-To: <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> <20170919110254.GB4511@kroah.com> From: Benjamin Gaignard Date: Tue, 19 Sep 2017 13:55:36 +0200 Message-ID: Subject: Re: [PATCH v2 2/2] staging: ion: create one device entry per heap To: Greg KH Cc: Laura Abbott , Sumit Semwal , =?UTF-8?B?QXJ2ZSBIasO4bm5ldsOlZw==?= , Riley Andrews , Mark Brown , Dan Carpenter , driverdevel , Linux Kernel Mailing List Content-Type: text/plain; charset="UTF-8" Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org Content-Length: 5179 Lines: 146 2017-09-19 13:02 GMT+02:00 Greg KH : > 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 >> --- >> 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 :( I will change that >> + >> 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? > My understanding of alloc_chrdev_region() is that major number is chosen dynamically but I don't understand the link with device node, sorry. > Wow, that's a lot of userspace breakage (both major number, and name), > how did you test this? I had to write a test by myself: https://git.linaro.org/people/benjamin.gaignard/ion_test_application.git/log/?h=one_device_per_heap Laura have tried to push a test VGEM but I believe it hasn't be accepted yet (I will check) > > Have you gotten "upstream" to agree to these changes? We can't take > these until they think it's ok as well. Split /dev/ion into multiple nodes is one of the task listed in staging/android/TODO file before been able to de-stage ion. Since it has been a big bang in ion ABI on 4.12 and the fact that ion.h is still in staging/android/uapi/ directory I do believe that userland is still unstable. I hope this kind of patch will help to clarify what is still need to be done to de-stage ion even if this patch is NACK-ed we can at least the item from the TODO list. Benjamin > > thanks, > > greg k-h