Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1751988AbdITHra (ORCPT ); Wed, 20 Sep 2017 03:47:30 -0400 Received: from mail-qk0-f170.google.com ([209.85.220.170]:54256 "EHLO mail-qk0-f170.google.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1751510AbdITHr3 (ORCPT ); Wed, 20 Sep 2017 03:47:29 -0400 X-Google-Smtp-Source: AOwi7QBhWLgcYxt6iAJ8KZTGN6rHV42TUogZ4u0kQ0biNFTDvdhFwxhxq1HfpCy7/3RUPqsupJwbSJpJzLVqT/AGjBU= MIME-Version: 1.0 In-Reply-To: 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: Wed, 20 Sep 2017 09:47:27 +0200 Message-ID: Subject: Re: [PATCH v2 2/2] staging: ion: create one device entry per heap To: Laura Abbott Cc: Greg KH , 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: 6372 Lines: 180 2017-09-20 3:01 GMT+02:00 Laura Abbott : > On 09/19/2017 04:55 AM, Benjamin Gaignard wrote: >> >> 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. >> > > Thanks for sending this Benjamin. > > At plumbers, it was requested to not break the ABI too much or do it in > one last big bang before moving out of staging. My thought was to > keep the old /dev/ion the same and allow all allocation and access via > a Kconfig. I'm also going to be at XDC this week so I was going to > float some ideas there as well. I won't have a chance to do much with > this until next week though. > Hi Laura, I will send a v3 to fix the comments already done. I hope that could help to clarify if this patch is needed or not at XDC. If splitting /dev/ion is not needed to de-stage driver code than we will only have to remove the item from the TODO list. Regards, Benjamin > Thanks, > Laura