Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1751451AbdISKUS (ORCPT ); Tue, 19 Sep 2017 06:20:18 -0400 Received: from mail-qk0-f182.google.com ([209.85.220.182]:51184 "EHLO mail-qk0-f182.google.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1750948AbdISKUR (ORCPT ); Tue, 19 Sep 2017 06:20:17 -0400 X-Google-Smtp-Source: AOwi7QCBYEeqDhsjvPldFbmsQpRDCcl2OfrKfGOZITkvmmTnXJdNmfBPRBhgDnjCSPNeeNJ66FyDocVEzrNh3wLFT0A= MIME-Version: 1.0 In-Reply-To: References: <1505746726-11443-1-git-send-email-benjamin.gaignard@linaro.org> <20170919094004.bkuvomr5wmnpci6c@mwanda> From: Benjamin Gaignard Date: Tue, 19 Sep 2017 12:20:15 +0200 Message-ID: Subject: Re: [PATCH] staging: ion: create one device entry per heap To: Tomas Winkler Cc: Dan Carpenter , driverdevel , Greg Kroah-Hartman , =?UTF-8?B?QXJ2ZSBIasO4bm5ldsOlZw==?= , Linux Kernel Mailing List , Riley Andrews , Mark Brown , Sumit Semwal 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: 3845 Lines: 111 2017-09-19 12:15 GMT+02:00 Tomas Winkler : > On Tue, Sep 19, 2017 at 1:07 PM, Benjamin Gaignard > wrote: >> 2017-09-19 11:40 GMT+02:00 Dan Carpenter : >>> On Mon, Sep 18, 2017 at 04:58:46PM +0200, Benjamin Gaignard wrote: >>>> -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 ret = 0; >>>> + int mask = 1 << iminor(filp->f_inode); >>>> >>>> switch (cmd) { >>>> case ION_IOC_HEAP_QUERY: >>>> @@ -35,6 +37,9 @@ static int validate_ioctl_arg(unsigned int cmd, union ion_ioctl_arg *arg) >>>> ret |= arg->query.reserved1 != 0; >>>> ret |= arg->query.reserved2 != 0; >>>> break; >>>> + case ION_IOC_ALLOC: >>>> + ret = !(arg->allocation.heap_id_mask & mask); >>> >>> >>> validate_ioctl_arg() is really convoluted. From reading just the patch >>> I at first thought we were returning 1 on failure. Just say: >>> >>> if (!(arg->allocation.heap_id_mask & mask)) >>> return -EINVAL; >>> return 0; >>> >>> If you want to fix the surrounding code in a separate patch that would >>> be good. It would be more clear to say: >>> >>> if (arg->query.reserved0 != 0 || >>> arg->query.reserved1 != 0 || >>> arg->query.reserved2 != 0) >>> return -EINVAL; >> >> I agree I will add a fix for that in next version >> >>> >>>> + break; >>>> default: >>>> break; >>>> } >>>> @@ -70,7 +75,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..5144f1a 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,21 @@ 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__); >>>> >>> >>> I don't think it can happen in current code but we should proabably have >>> a check here for: >>> >>> if (heap_id >= ION_DEV_MAX) >>> return -EBUSY; >>> >>> (It's possible I have missed something). >>> >> >> You are right I will add that >> >> Thanks >>> >>>> + 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; >>>> + >>>> spin_lock_init(&heap->free_lock); >>>> heap->free_list_size = 0; > > What will happen to an application which looks for /dev/ion? /dev/ion will no more exist with this patch. Since ion ABI have already change a lot I don't think that could be a problem to change also ion device. > > Thanks > Tomas