Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1751479AbdISKH5 (ORCPT ); Tue, 19 Sep 2017 06:07:57 -0400 Received: from mail-qk0-f175.google.com ([209.85.220.175]:46474 "EHLO mail-qk0-f175.google.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1751057AbdISKHz (ORCPT ); Tue, 19 Sep 2017 06:07:55 -0400 X-Google-Smtp-Source: AOwi7QAVKExTDjV+hf7SenVTv4IatXGRzFqWqPeJepfwKCJ6RXgX4SwcywIDcx7srHcCfZ3edar6wpvGG+Sr4Onq89M= MIME-Version: 1.0 In-Reply-To: <20170919094004.bkuvomr5wmnpci6c@mwanda> References: <1505746726-11443-1-git-send-email-benjamin.gaignard@linaro.org> <20170919094004.bkuvomr5wmnpci6c@mwanda> From: Benjamin Gaignard Date: Tue, 19 Sep 2017 12:07:54 +0200 Message-ID: Subject: Re: [PATCH] staging: ion: create one device entry per heap To: Dan Carpenter Cc: Laura Abbott , Sumit Semwal , Greg Kroah-Hartman , =?UTF-8?B?QXJ2ZSBIasO4bm5ldsOlZw==?= , Riley Andrews , Mark Brown , devel@driverdev.osuosl.org, 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-Transfer-Encoding: 8bit X-MIME-Autoconverted: from quoted-printable to 8bit by nfs id v8JA83pZ008655 Content-Length: 3429 Lines: 115 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; >> > > regards, > dan carpenter > > -- Benjamin Gaignard Graphic Study Group Linaro.org │ Open source software for ARM SoCs Follow Linaro: Facebook | Twitter | Blog