Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1751758AbdISK7D (ORCPT ); Tue, 19 Sep 2017 06:59:03 -0400 Received: from mail.linuxfoundation.org ([140.211.169.12]:52636 "EHLO mail.linuxfoundation.org" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1751549AbdISK7B (ORCPT ); Tue, 19 Sep 2017 06:59:01 -0400 Date: Tue, 19 Sep 2017 12:59:12 +0200 From: Greg Kroah-Hartman To: Benjamin Gaignard Cc: Tomas Winkler , driverdevel , Riley Andrews , Linux Kernel Mailing List , Arve =?iso-8859-1?B?SGr4bm5lduVn?= , Mark Brown , Sumit Semwal , Dan Carpenter Subject: Re: [PATCH] staging: ion: create one device entry per heap Message-ID: <20170919105912.GA4511@kroah.com> References: <1505746726-11443-1-git-send-email-benjamin.gaignard@linaro.org> <20170919094004.bkuvomr5wmnpci6c@mwanda> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: User-Agent: Mutt/1.9.0 (2017-09-02) Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org Content-Length: 4250 Lines: 117 On Tue, Sep 19, 2017 at 12:20:15PM +0200, Benjamin Gaignard wrote: > 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. So, what did you just break in userspace? :( Do you also have userspace patches submitted anywhere to handle this change? thanks, greg k-h