Re: [PATCH v6 2/2] staging: ion: create one device entry per heap

[Greg KH]

On Tue, Dec 05, 2017 at 03:01:42PM -0800, Laura Abbott wrote:
> On 12/02/2017 07:53 AM, Greg KH wrote:
> > > This is one of the item in the TODO list before been able to unstage ION
> > > which is my real need.
> > Why does it matter where in the tree this code is?  Don't go adding new
> > things to it that are not needed.  Who needs this?  What userspace code
> > wants this type of multiple ion devices?
> > 
> 
> Requirements came in from several places to split /dev/ion -> /dev/ion0
> and /dev/ion1 so that security policy (i.e. selinux) could be used to
> protect access to certain heaps. I wanted the ABI to be settled before
> trying to move out of staging, hence the line in the TODO list about
> doing the split.

Ok, but we should have some way of testing it works, right?  :)

Re: [PATCH v6 2/2] staging: ion: create one device entry per heap

[Greg KH]

On Tue, Dec 05, 2017 at 03:01:42PM -0800, Laura Abbott wrote:
> On 12/02/2017 07:53 AM, Greg KH wrote:
> > > This is one of the item in the TODO list before been able to unstage ION
> > > which is my real need.
> > Why does it matter where in the tree this code is?  Don't go adding new
> > things to it that are not needed.  Who needs this?  What userspace code
> > wants this type of multiple ion devices?
> > 
> 
> Requirements came in from several places to split /dev/ion -> /dev/ion0
> and /dev/ion1 so that security policy (i.e. selinux) could be used to
> protect access to certain heaps. I wanted the ABI to be settled before
> trying to move out of staging, hence the line in the TODO list about
> doing the split.

Ok, but we should have some way of testing it works, right?  :)

Re: [PATCH v6 2/2] staging: ion: create one device entry per heap

[Laura Abbott]

On 12/02/2017 07:53 AM, Greg KH wrote:

This is one of the item in the TODO list before been able to unstage ION
which is my real need.

Why does it matter where in the tree this code is?  Don't go adding new
things to it that are not needed.  Who needs this?  What userspace code
wants this type of multiple ion devices?



Requirements came in from several places to split /dev/ion -> /dev/ion0
and /dev/ion1 so that security policy (i.e. selinux) could be used to
protect access to certain heaps. I wanted the ABI to be settled before
trying to move out of staging, hence the line in the TODO list about
doing the split.


thanks,

greg k-h


Thanks,
Laura

Re: [PATCH v6 2/2] staging: ion: create one device entry per heap

[Laura Abbott]

On 12/02/2017 07:53 AM, Greg KH wrote:

This is one of the item in the TODO list before been able to unstage ION
which is my real need.

Why does it matter where in the tree this code is?  Don't go adding new
things to it that are not needed.  Who needs this?  What userspace code
wants this type of multiple ion devices?



Requirements came in from several places to split /dev/ion -> /dev/ion0
and /dev/ion1 so that security policy (i.e. selinux) could be used to
protect access to certain heaps. I wanted the ABI to be settled before
trying to move out of staging, hence the line in the TODO list about
doing the split.


thanks,

greg k-h


Thanks,
Laura

Re: [PATCH v6 2/2] staging: ion: create one device entry per heap

[Greg KH]

On Wed, Nov 29, 2017 at 03:00:56PM +0100, Benjamin Gaignard wrote:
> 2017-11-28 14:32 GMT+01:00 Greg KH :
> > On Mon, Nov 06, 2017 at 04:59:45PM +0100, Benjamin Gaignard wrote:
> >> Instead a getting only 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/TODO|  1 -
> >>  drivers/staging/android/ion/Kconfig |  7 
> >>  drivers/staging/android/ion/ion-ioctl.c | 18 --
> >>  drivers/staging/android/ion/ion.c   | 62 
> >> +
> >>  drivers/staging/android/ion/ion.h   | 15 ++--
> >>  5 files changed, 91 insertions(+), 12 deletions(-)
> >>
> >> diff --git a/drivers/staging/android/TODO b/drivers/staging/android/TODO
> >> index 687e0ea..8a11931 100644
> >> --- a/drivers/staging/android/TODO
> >> +++ b/drivers/staging/android/TODO
> >> @@ -8,7 +8,6 @@ TODO:
> >>  ion/
> >>   - Add dt-bindings for remaining heaps (chunk and carveout heaps). This 
> >> would
> >> involve putting appropriate bindings in a memory node for Ion to find.
> >> - - Split /dev/ion up into multiple nodes (e.g. /dev/ion/heap0)
> >>   - Better test framework (integration with VGEM was suggested)
> >>
> >>  Please send patches to Greg Kroah-Hartman  and Cc:
> >> diff --git a/drivers/staging/android/ion/Kconfig 
> >> b/drivers/staging/android/ion/Kconfig
> >> index a517b2d..cb4666e 100644
> >> --- a/drivers/staging/android/ion/Kconfig
> >> +++ b/drivers/staging/android/ion/Kconfig
> >> @@ -10,6 +10,13 @@ menuconfig ION
> >> If you're not using Android its probably safe to
> >> say N here.
> >>
> >> +config ION_LEGACY_DEVICE_API
> >> + bool "Keep using Ion legacy misc device API"
> >> + depends on ION
> >
> > You want to default to Y here, so when you do 'make oldconfig' nothing
> > breaks, right?
> >
> 
> I will add it.
> 
> >> + help
> >> +   Choose this option to keep using Ion legacy misc device API
> >> +   i.e. /dev/ion
> >
> > You need more text here to describe the trade offs.  Why would I not
> > want to keep doing this?  What does turning this off get me?  What does
> > keeping it on keep me from doing?
> >
> Does describe it like that sound better ?
> "Choose this option to keep using ION legacy misc device API
>   i.e. /dev/ion. If this option isn't selected you will only
>   have per heap device node (i.e /dev/ionX) and allocating buffer
>   from an unique device node won't be possible."

I still don't know why I would not select such an option, other than it
would break my working Android system if I did so :)

Please try to explain it a bit better.  For example, I really don't know
why you want to do this at all.

> >> -void ion_device_add_heap(struct ion_heap *heap)
> >> +static struct device ion_bus = {
> >> + .init_name = ION_NAME,
> >> +};
> >
> > Oh look, a struct device on the stack.  Watch bad things happen :(
> >
> > This is a dynamic device, make it dynamic, or else you had better know
> > exactly what you are doing...
> 
> The naming is bad here, I will rename it ion_parent because I use it to 
> provide
> a parent to ion heap device either they won't go in /sys/bus/ion

Again, never use a static struct device, if you do, it is a _huge_ hint
the code is incorrect.

> >> - idev->debug_root = debugfs_create_dir("ion", NULL);
> >> - if (!idev->debug_root) {
> >> + ret = device_register(_bus);
> >
> > You call device_register for something you are calling a bus???  Are you
> > _sure_ about this?
> >
> > What exactly are you creating in sysfs here?  You are throwing around
> > "raw" devices, which is almost never what you ever want to do.
> > Especially for some random char driver.
> 
> ion heap devices need a parent to be correctly put in /sys/bus/ion either they
> will go directly in /sys/bus directory. You are right name it ion_bus
> is confusing
> I will rename it ion_parent.

Again, what are you trying to show in sysfs?  What type of
representation?  What is the bus?  What device type?  Why in sysfs at
all?

> > How many different reference counted objects do you now have in this
> > structure?  And what exactly is the lifetime rules involved in them?
> >
> > Hint, you never tried removing any of these from the system, otherwise
> > you would have seen the kernel warnings...
> >
> 
> No I have never try because ION doesn't allow to be removed.

That seems odd, there's no way to free up all resources and remove the
module?

> > Step back here, what exactly do you want to do?  What do you expect
> > sysfs to look 

Re: [PATCH v6 2/2] staging: ion: create one device entry per heap

[Greg KH]

On Wed, Nov 29, 2017 at 03:00:56PM +0100, Benjamin Gaignard wrote:
> 2017-11-28 14:32 GMT+01:00 Greg KH :
> > On Mon, Nov 06, 2017 at 04:59:45PM +0100, Benjamin Gaignard wrote:
> >> Instead a getting only 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/TODO|  1 -
> >>  drivers/staging/android/ion/Kconfig |  7 
> >>  drivers/staging/android/ion/ion-ioctl.c | 18 --
> >>  drivers/staging/android/ion/ion.c   | 62 
> >> +
> >>  drivers/staging/android/ion/ion.h   | 15 ++--
> >>  5 files changed, 91 insertions(+), 12 deletions(-)
> >>
> >> diff --git a/drivers/staging/android/TODO b/drivers/staging/android/TODO
> >> index 687e0ea..8a11931 100644
> >> --- a/drivers/staging/android/TODO
> >> +++ b/drivers/staging/android/TODO
> >> @@ -8,7 +8,6 @@ TODO:
> >>  ion/
> >>   - Add dt-bindings for remaining heaps (chunk and carveout heaps). This 
> >> would
> >> involve putting appropriate bindings in a memory node for Ion to find.
> >> - - Split /dev/ion up into multiple nodes (e.g. /dev/ion/heap0)
> >>   - Better test framework (integration with VGEM was suggested)
> >>
> >>  Please send patches to Greg Kroah-Hartman  and Cc:
> >> diff --git a/drivers/staging/android/ion/Kconfig 
> >> b/drivers/staging/android/ion/Kconfig
> >> index a517b2d..cb4666e 100644
> >> --- a/drivers/staging/android/ion/Kconfig
> >> +++ b/drivers/staging/android/ion/Kconfig
> >> @@ -10,6 +10,13 @@ menuconfig ION
> >> If you're not using Android its probably safe to
> >> say N here.
> >>
> >> +config ION_LEGACY_DEVICE_API
> >> + bool "Keep using Ion legacy misc device API"
> >> + depends on ION
> >
> > You want to default to Y here, so when you do 'make oldconfig' nothing
> > breaks, right?
> >
> 
> I will add it.
> 
> >> + help
> >> +   Choose this option to keep using Ion legacy misc device API
> >> +   i.e. /dev/ion
> >
> > You need more text here to describe the trade offs.  Why would I not
> > want to keep doing this?  What does turning this off get me?  What does
> > keeping it on keep me from doing?
> >
> Does describe it like that sound better ?
> "Choose this option to keep using ION legacy misc device API
>   i.e. /dev/ion. If this option isn't selected you will only
>   have per heap device node (i.e /dev/ionX) and allocating buffer
>   from an unique device node won't be possible."

I still don't know why I would not select such an option, other than it
would break my working Android system if I did so :)

Please try to explain it a bit better.  For example, I really don't know
why you want to do this at all.

> >> -void ion_device_add_heap(struct ion_heap *heap)
> >> +static struct device ion_bus = {
> >> + .init_name = ION_NAME,
> >> +};
> >
> > Oh look, a struct device on the stack.  Watch bad things happen :(
> >
> > This is a dynamic device, make it dynamic, or else you had better know
> > exactly what you are doing...
> 
> The naming is bad here, I will rename it ion_parent because I use it to 
> provide
> a parent to ion heap device either they won't go in /sys/bus/ion

Again, never use a static struct device, if you do, it is a _huge_ hint
the code is incorrect.

> >> - idev->debug_root = debugfs_create_dir("ion", NULL);
> >> - if (!idev->debug_root) {
> >> + ret = device_register(_bus);
> >
> > You call device_register for something you are calling a bus???  Are you
> > _sure_ about this?
> >
> > What exactly are you creating in sysfs here?  You are throwing around
> > "raw" devices, which is almost never what you ever want to do.
> > Especially for some random char driver.
> 
> ion heap devices need a parent to be correctly put in /sys/bus/ion either they
> will go directly in /sys/bus directory. You are right name it ion_bus
> is confusing
> I will rename it ion_parent.

Again, what are you trying to show in sysfs?  What type of
representation?  What is the bus?  What device type?  Why in sysfs at
all?

> > How many different reference counted objects do you now have in this
> > structure?  And what exactly is the lifetime rules involved in them?
> >
> > Hint, you never tried removing any of these from the system, otherwise
> > you would have seen the kernel warnings...
> >
> 
> No I have never try because ION doesn't allow to be removed.

That seems odd, there's no way to free up all resources and remove the
module?

> > Step back here, what exactly do you want to do?  What do you expect
> > sysfs to look like?  What do you want /dev/ to look like?
> >
> > Where is the 

Re: [PATCH v6 2/2] staging: ion: create one device entry per heap

[Benjamin Gaignard]

2017-11-28 14:32 GMT+01:00 Greg KH :
> On Mon, Nov 06, 2017 at 04:59:45PM +0100, Benjamin Gaignard wrote:
>> Instead a getting only 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/TODO|  1 -
>>  drivers/staging/android/ion/Kconfig |  7 
>>  drivers/staging/android/ion/ion-ioctl.c | 18 --
>>  drivers/staging/android/ion/ion.c   | 62 
>> +
>>  drivers/staging/android/ion/ion.h   | 15 ++--
>>  5 files changed, 91 insertions(+), 12 deletions(-)
>>
>> diff --git a/drivers/staging/android/TODO b/drivers/staging/android/TODO
>> index 687e0ea..8a11931 100644
>> --- a/drivers/staging/android/TODO
>> +++ b/drivers/staging/android/TODO
>> @@ -8,7 +8,6 @@ TODO:
>>  ion/
>>   - Add dt-bindings for remaining heaps (chunk and carveout heaps). This 
>> would
>> involve putting appropriate bindings in a memory node for Ion to find.
>> - - Split /dev/ion up into multiple nodes (e.g. /dev/ion/heap0)
>>   - Better test framework (integration with VGEM was suggested)
>>
>>  Please send patches to Greg Kroah-Hartman  and Cc:
>> diff --git a/drivers/staging/android/ion/Kconfig 
>> b/drivers/staging/android/ion/Kconfig
>> index a517b2d..cb4666e 100644
>> --- a/drivers/staging/android/ion/Kconfig
>> +++ b/drivers/staging/android/ion/Kconfig
>> @@ -10,6 +10,13 @@ menuconfig ION
>> If you're not using Android its probably safe to
>> say N here.
>>
>> +config ION_LEGACY_DEVICE_API
>> + bool "Keep using Ion legacy misc device API"
>> + depends on ION
>
> You want to default to Y here, so when you do 'make oldconfig' nothing
> breaks, right?
>

I will add it.

>> + help
>> +   Choose this option to keep using Ion legacy misc device API
>> +   i.e. /dev/ion
>
> You need more text here to describe the trade offs.  Why would I not
> want to keep doing this?  What does turning this off get me?  What does
> keeping it on keep me from doing?
>
Does describe it like that sound better ?
"Choose this option to keep using ION legacy misc device API
  i.e. /dev/ion. If this option isn't selected you will only
  have per heap device node (i.e /dev/ionX) and allocating buffer
  from an unique device node won't be possible."


>> +
>>  config ION_SYSTEM_HEAP
>>   bool "Ion system heap"
>>   depends on ION
>> diff --git a/drivers/staging/android/ion/ion-ioctl.c 
>> b/drivers/staging/android/ion/ion-ioctl.c
>> index e26b786..bb5c77b 100644
>> --- a/drivers/staging/android/ion/ion-ioctl.c
>> +++ b/drivers/staging/android/ion/ion-ioctl.c
>> @@ -25,7 +25,8 @@ 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)
>>  {
>>   switch (cmd) {
>>   case ION_IOC_HEAP_QUERY:
>> @@ -34,6 +35,19 @@ static int validate_ioctl_arg(unsigned int cmd, union 
>> ion_ioctl_arg *arg)
>>   arg->query.reserved2 )
>>   return -EINVAL;
>>   break;
>> +
>> + case ION_IOC_ALLOC:
>> + {
>> + int mask = 1 << iminor(filp->f_inode);
>> +
>> +#ifdef CONFIG_ION_LEGACY_DEVICE_API
>> + if (imajor(filp->f_inode) == MISC_MAJOR)
>> + return 0;
>
> Why return 0?  Because it is already allocated?

No it is because in legacy mode all mask are valids so to keep legacy behavoir
it will not test if the requested heap  match with the device.

I will add a comment about that.

>
> Some comments here as to exactly what you are doing would be nice.
>
>> +#endif
>> + if (!(arg->allocation.heap_id_mask & mask))
>
> This doesn't allocate anthing, just check to see if it is?
>

No this function only check if ioctl args are correct.

>> + return -EINVAL;
>> + break;
>> + }
>>   default:
>>   break;
>>   }
>> @@ -69,7 +83,7 @@ long ion_ioctl(struct file *filp, unsigned int cmd, 
>> unsigned long arg)
>>   if (copy_from_user(, (void __user *)arg, _IOC_SIZE(cmd)))
>>   return -EFAULT;
>>
>> - ret = validate_ioctl_arg(cmd, );
>> + ret = validate_ioctl_arg(filp, cmd, );
>>   if (WARN_ON_ONCE(ret))
>>   return ret;
>>
>> diff --git a/drivers/staging/android/ion/ion.c 
>> b/drivers/staging/android/ion/ion.c
>> index fda9756..2c2568b 100644
>> --- a/drivers/staging/android/ion/ion.c
>> +++ 

Re: [PATCH v6 2/2] staging: ion: create one device entry per heap

[Benjamin Gaignard]

2017-11-28 14:32 GMT+01:00 Greg KH :
> On Mon, Nov 06, 2017 at 04:59:45PM +0100, Benjamin Gaignard wrote:
>> Instead a getting only 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/TODO|  1 -
>>  drivers/staging/android/ion/Kconfig |  7 
>>  drivers/staging/android/ion/ion-ioctl.c | 18 --
>>  drivers/staging/android/ion/ion.c   | 62 
>> +
>>  drivers/staging/android/ion/ion.h   | 15 ++--
>>  5 files changed, 91 insertions(+), 12 deletions(-)
>>
>> diff --git a/drivers/staging/android/TODO b/drivers/staging/android/TODO
>> index 687e0ea..8a11931 100644
>> --- a/drivers/staging/android/TODO
>> +++ b/drivers/staging/android/TODO
>> @@ -8,7 +8,6 @@ TODO:
>>  ion/
>>   - Add dt-bindings for remaining heaps (chunk and carveout heaps). This 
>> would
>> involve putting appropriate bindings in a memory node for Ion to find.
>> - - Split /dev/ion up into multiple nodes (e.g. /dev/ion/heap0)
>>   - Better test framework (integration with VGEM was suggested)
>>
>>  Please send patches to Greg Kroah-Hartman  and Cc:
>> diff --git a/drivers/staging/android/ion/Kconfig 
>> b/drivers/staging/android/ion/Kconfig
>> index a517b2d..cb4666e 100644
>> --- a/drivers/staging/android/ion/Kconfig
>> +++ b/drivers/staging/android/ion/Kconfig
>> @@ -10,6 +10,13 @@ menuconfig ION
>> If you're not using Android its probably safe to
>> say N here.
>>
>> +config ION_LEGACY_DEVICE_API
>> + bool "Keep using Ion legacy misc device API"
>> + depends on ION
>
> You want to default to Y here, so when you do 'make oldconfig' nothing
> breaks, right?
>

I will add it.

>> + help
>> +   Choose this option to keep using Ion legacy misc device API
>> +   i.e. /dev/ion
>
> You need more text here to describe the trade offs.  Why would I not
> want to keep doing this?  What does turning this off get me?  What does
> keeping it on keep me from doing?
>
Does describe it like that sound better ?
"Choose this option to keep using ION legacy misc device API
  i.e. /dev/ion. If this option isn't selected you will only
  have per heap device node (i.e /dev/ionX) and allocating buffer
  from an unique device node won't be possible."


>> +
>>  config ION_SYSTEM_HEAP
>>   bool "Ion system heap"
>>   depends on ION
>> diff --git a/drivers/staging/android/ion/ion-ioctl.c 
>> b/drivers/staging/android/ion/ion-ioctl.c
>> index e26b786..bb5c77b 100644
>> --- a/drivers/staging/android/ion/ion-ioctl.c
>> +++ b/drivers/staging/android/ion/ion-ioctl.c
>> @@ -25,7 +25,8 @@ 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)
>>  {
>>   switch (cmd) {
>>   case ION_IOC_HEAP_QUERY:
>> @@ -34,6 +35,19 @@ static int validate_ioctl_arg(unsigned int cmd, union 
>> ion_ioctl_arg *arg)
>>   arg->query.reserved2 )
>>   return -EINVAL;
>>   break;
>> +
>> + case ION_IOC_ALLOC:
>> + {
>> + int mask = 1 << iminor(filp->f_inode);
>> +
>> +#ifdef CONFIG_ION_LEGACY_DEVICE_API
>> + if (imajor(filp->f_inode) == MISC_MAJOR)
>> + return 0;
>
> Why return 0?  Because it is already allocated?

No it is because in legacy mode all mask are valids so to keep legacy behavoir
it will not test if the requested heap  match with the device.

I will add a comment about that.

>
> Some comments here as to exactly what you are doing would be nice.
>
>> +#endif
>> + if (!(arg->allocation.heap_id_mask & mask))
>
> This doesn't allocate anthing, just check to see if it is?
>

No this function only check if ioctl args are correct.

>> + return -EINVAL;
>> + break;
>> + }
>>   default:
>>   break;
>>   }
>> @@ -69,7 +83,7 @@ long ion_ioctl(struct file *filp, unsigned int cmd, 
>> unsigned long arg)
>>   if (copy_from_user(, (void __user *)arg, _IOC_SIZE(cmd)))
>>   return -EFAULT;
>>
>> - ret = validate_ioctl_arg(cmd, );
>> + ret = validate_ioctl_arg(filp, cmd, );
>>   if (WARN_ON_ONCE(ret))
>>   return ret;
>>
>> diff --git a/drivers/staging/android/ion/ion.c 
>> b/drivers/staging/android/ion/ion.c
>> index fda9756..2c2568b 100644
>> --- a/drivers/staging/android/ion/ion.c
>> +++ b/drivers/staging/android/ion/ion.c
>> @@ -40,6 +40,9 @@
>>
>>  #include "ion.h"
>>

Re: [PATCH v6 2/2] staging: ion: create one device entry per heap

[Greg KH]

On Tue, Nov 28, 2017 at 05:37:53PM +, Mark Brown wrote:
> On Tue, Nov 28, 2017 at 06:28:38PM +0100, Greg KH wrote:
> > On Tue, Nov 28, 2017 at 05:12:23PM +, Mark Brown wrote:
> 
> > > I think it's reasonable to ask for userspace, I'm querying why it needs
> > > to specifically be Android.
> 
> > Does anyone other than Android use this interface?
> 
> There's plenty of other userspaces that have the same requirements for
> allocation for applications like video capture, I believe some of them
> have actually moved to ION already and they're certainly where some of
> the requirements here are coming from.

Then the Kconfig option should start to describe some of this :)

Re: [PATCH v6 2/2] staging: ion: create one device entry per heap

[Greg KH]

On Tue, Nov 28, 2017 at 05:37:53PM +, Mark Brown wrote:
> On Tue, Nov 28, 2017 at 06:28:38PM +0100, Greg KH wrote:
> > On Tue, Nov 28, 2017 at 05:12:23PM +, Mark Brown wrote:
> 
> > > I think it's reasonable to ask for userspace, I'm querying why it needs
> > > to specifically be Android.
> 
> > Does anyone other than Android use this interface?
> 
> There's plenty of other userspaces that have the same requirements for
> allocation for applications like video capture, I believe some of them
> have actually moved to ION already and they're certainly where some of
> the requirements here are coming from.

Then the Kconfig option should start to describe some of this :)

Re: [PATCH v6 2/2] staging: ion: create one device entry per heap

[Mark Brown]

On Tue, Nov 28, 2017 at 06:28:38PM +0100, Greg KH wrote:
> On Tue, Nov 28, 2017 at 05:12:23PM +, Mark Brown wrote:

> > I think it's reasonable to ask for userspace, I'm querying why it needs
> > to specifically be Android.

> Does anyone other than Android use this interface?

There's plenty of other userspaces that have the same requirements for
allocation for applications like video capture, I believe some of them
have actually moved to ION already and they're certainly where some of
the requirements here are coming from.


signature.asc
Description: PGP signature

Re: [PATCH v6 2/2] staging: ion: create one device entry per heap

[Mark Brown]

On Tue, Nov 28, 2017 at 06:28:38PM +0100, Greg KH wrote:
> On Tue, Nov 28, 2017 at 05:12:23PM +, Mark Brown wrote:

> > I think it's reasonable to ask for userspace, I'm querying why it needs
> > to specifically be Android.

> Does anyone other than Android use this interface?

There's plenty of other userspaces that have the same requirements for
allocation for applications like video capture, I believe some of them
have actually moved to ION already and they're certainly where some of
the requirements here are coming from.


signature.asc
Description: PGP signature

Re: [PATCH v6 2/2] staging: ion: create one device entry per heap

[Greg KH]

On Tue, Nov 28, 2017 at 05:12:23PM +, Mark Brown wrote:
> On Tue, Nov 28, 2017 at 06:08:22PM +0100, Greg KH wrote:
> > On Tue, Nov 28, 2017 at 04:26:20PM +, Mark Brown wrote:
> > > On Tue, Nov 28, 2017 at 02:32:17PM +0100, Greg KH wrote:
> 
> > > > call you added?  What did you do to test this out?  Where are the AOSP
> > > > patches to use this?  Happen to have a VTS test for it?
> 
> > > Do we need to convert Android for this to be accepted?  The single
> > > device is being kept around for it and the use case was from non-Android
> > > users wasn't it?
> 
> > So we are just going to add kernel features with no userspace users of
> > it at all?  Why would we do that?  How was it even tested?
> 
> I think it's reasonable to ask for userspace, I'm querying why it needs
> to specifically be Android.

Does anyone other than Android use this interface?

Re: [PATCH v6 2/2] staging: ion: create one device entry per heap

[Greg KH]

On Tue, Nov 28, 2017 at 05:12:23PM +, Mark Brown wrote:
> On Tue, Nov 28, 2017 at 06:08:22PM +0100, Greg KH wrote:
> > On Tue, Nov 28, 2017 at 04:26:20PM +, Mark Brown wrote:
> > > On Tue, Nov 28, 2017 at 02:32:17PM +0100, Greg KH wrote:
> 
> > > > call you added?  What did you do to test this out?  Where are the AOSP
> > > > patches to use this?  Happen to have a VTS test for it?
> 
> > > Do we need to convert Android for this to be accepted?  The single
> > > device is being kept around for it and the use case was from non-Android
> > > users wasn't it?
> 
> > So we are just going to add kernel features with no userspace users of
> > it at all?  Why would we do that?  How was it even tested?
> 
> I think it's reasonable to ask for userspace, I'm querying why it needs
> to specifically be Android.

Does anyone other than Android use this interface?

Re: [PATCH v6 2/2] staging: ion: create one device entry per heap

[Mark Brown]

On Tue, Nov 28, 2017 at 06:08:22PM +0100, Greg KH wrote:
> On Tue, Nov 28, 2017 at 04:26:20PM +, Mark Brown wrote:
> > On Tue, Nov 28, 2017 at 02:32:17PM +0100, Greg KH wrote:

> > > call you added?  What did you do to test this out?  Where are the AOSP
> > > patches to use this?  Happen to have a VTS test for it?

> > Do we need to convert Android for this to be accepted?  The single
> > device is being kept around for it and the use case was from non-Android
> > users wasn't it?

> So we are just going to add kernel features with no userspace users of
> it at all?  Why would we do that?  How was it even tested?

I think it's reasonable to ask for userspace, I'm querying why it needs
to specifically be Android.


signature.asc
Description: PGP signature

Re: [PATCH v6 2/2] staging: ion: create one device entry per heap

[Mark Brown]

On Tue, Nov 28, 2017 at 06:08:22PM +0100, Greg KH wrote:
> On Tue, Nov 28, 2017 at 04:26:20PM +, Mark Brown wrote:
> > On Tue, Nov 28, 2017 at 02:32:17PM +0100, Greg KH wrote:

> > > call you added?  What did you do to test this out?  Where are the AOSP
> > > patches to use this?  Happen to have a VTS test for it?

> > Do we need to convert Android for this to be accepted?  The single
> > device is being kept around for it and the use case was from non-Android
> > users wasn't it?

> So we are just going to add kernel features with no userspace users of
> it at all?  Why would we do that?  How was it even tested?

I think it's reasonable to ask for userspace, I'm querying why it needs
to specifically be Android.


signature.asc
Description: PGP signature

Re: [PATCH v6 2/2] staging: ion: create one device entry per heap

[Greg KH]

On Tue, Nov 28, 2017 at 04:26:20PM +, Mark Brown wrote:
> On Tue, Nov 28, 2017 at 02:32:17PM +0100, Greg KH wrote:
> 
> > Where is the documentation for the new sysfs files and the new ioctl
> 
> Didn't see any sysfs files in there?

New struct devices were created and registered.  Why would that happen
if there was not a need for sysfs files? :)

> > call you added?  What did you do to test this out?  Where are the AOSP
> > patches to use this?  Happen to have a VTS test for it?
> 
> Do we need to convert Android for this to be accepted?  The single
> device is being kept around for it and the use case was from non-Android
> users wasn't it?

So we are just going to add kernel features with no userspace users of
it at all?  Why would we do that?  How was it even tested?

greg k-h

Re: [PATCH v6 2/2] staging: ion: create one device entry per heap

[Greg KH]

On Tue, Nov 28, 2017 at 04:26:20PM +, Mark Brown wrote:
> On Tue, Nov 28, 2017 at 02:32:17PM +0100, Greg KH wrote:
> 
> > Where is the documentation for the new sysfs files and the new ioctl
> 
> Didn't see any sysfs files in there?

New struct devices were created and registered.  Why would that happen
if there was not a need for sysfs files? :)

> > call you added?  What did you do to test this out?  Where are the AOSP
> > patches to use this?  Happen to have a VTS test for it?
> 
> Do we need to convert Android for this to be accepted?  The single
> device is being kept around for it and the use case was from non-Android
> users wasn't it?

So we are just going to add kernel features with no userspace users of
it at all?  Why would we do that?  How was it even tested?

greg k-h

Re: [PATCH v6 2/2] staging: ion: create one device entry per heap

[Mark Brown]

On Tue, Nov 28, 2017 at 02:32:17PM +0100, Greg KH wrote:

> Where is the documentation for the new sysfs files and the new ioctl

Didn't see any sysfs files in there?

> call you added?  What did you do to test this out?  Where are the AOSP
> patches to use this?  Happen to have a VTS test for it?

Do we need to convert Android for this to be accepted?  The single
device is being kept around for it and the use case was from non-Android
users wasn't it?


signature.asc
Description: PGP signature

Re: [PATCH v6 2/2] staging: ion: create one device entry per heap

[Mark Brown]

On Tue, Nov 28, 2017 at 02:32:17PM +0100, Greg KH wrote:

> Where is the documentation for the new sysfs files and the new ioctl

Didn't see any sysfs files in there?

> call you added?  What did you do to test this out?  Where are the AOSP
> patches to use this?  Happen to have a VTS test for it?

Do we need to convert Android for this to be accepted?  The single
device is being kept around for it and the use case was from non-Android
users wasn't it?


signature.asc
Description: PGP signature

Re: [PATCH v6 2/2] staging: ion: create one device entry per heap

[Greg KH]

On Mon, Nov 06, 2017 at 04:59:45PM +0100, Benjamin Gaignard wrote:
> Instead a getting only 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/TODO|  1 -
>  drivers/staging/android/ion/Kconfig |  7 
>  drivers/staging/android/ion/ion-ioctl.c | 18 --
>  drivers/staging/android/ion/ion.c   | 62 
> +
>  drivers/staging/android/ion/ion.h   | 15 ++--
>  5 files changed, 91 insertions(+), 12 deletions(-)
> 
> diff --git a/drivers/staging/android/TODO b/drivers/staging/android/TODO
> index 687e0ea..8a11931 100644
> --- a/drivers/staging/android/TODO
> +++ b/drivers/staging/android/TODO
> @@ -8,7 +8,6 @@ TODO:
>  ion/
>   - Add dt-bindings for remaining heaps (chunk and carveout heaps). This would
> involve putting appropriate bindings in a memory node for Ion to find.
> - - Split /dev/ion up into multiple nodes (e.g. /dev/ion/heap0)
>   - Better test framework (integration with VGEM was suggested)
>  
>  Please send patches to Greg Kroah-Hartman  and Cc:
> diff --git a/drivers/staging/android/ion/Kconfig 
> b/drivers/staging/android/ion/Kconfig
> index a517b2d..cb4666e 100644
> --- a/drivers/staging/android/ion/Kconfig
> +++ b/drivers/staging/android/ion/Kconfig
> @@ -10,6 +10,13 @@ menuconfig ION
> If you're not using Android its probably safe to
> say N here.
>  
> +config ION_LEGACY_DEVICE_API
> + bool "Keep using Ion legacy misc device API"
> + depends on ION

You want to default to Y here, so when you do 'make oldconfig' nothing
breaks, right?

> + help
> +   Choose this option to keep using Ion legacy misc device API
> +   i.e. /dev/ion

You need more text here to describe the trade offs.  Why would I not
want to keep doing this?  What does turning this off get me?  What does
keeping it on keep me from doing?

> +
>  config ION_SYSTEM_HEAP
>   bool "Ion system heap"
>   depends on ION
> diff --git a/drivers/staging/android/ion/ion-ioctl.c 
> b/drivers/staging/android/ion/ion-ioctl.c
> index e26b786..bb5c77b 100644
> --- a/drivers/staging/android/ion/ion-ioctl.c
> +++ b/drivers/staging/android/ion/ion-ioctl.c
> @@ -25,7 +25,8 @@ 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)
>  {
>   switch (cmd) {
>   case ION_IOC_HEAP_QUERY:
> @@ -34,6 +35,19 @@ static int validate_ioctl_arg(unsigned int cmd, union 
> ion_ioctl_arg *arg)
>   arg->query.reserved2 )
>   return -EINVAL;
>   break;
> +
> + case ION_IOC_ALLOC:
> + {
> + int mask = 1 << iminor(filp->f_inode);
> +
> +#ifdef CONFIG_ION_LEGACY_DEVICE_API
> + if (imajor(filp->f_inode) == MISC_MAJOR)
> + return 0;

Why return 0?  Because it is already allocated?

Some comments here as to exactly what you are doing would be nice.

> +#endif
> + if (!(arg->allocation.heap_id_mask & mask))

This doesn't allocate anthing, just check to see if it is?

> + return -EINVAL;
> + break;
> + }
>   default:
>   break;
>   }
> @@ -69,7 +83,7 @@ long ion_ioctl(struct file *filp, unsigned int cmd, 
> unsigned long arg)
>   if (copy_from_user(, (void __user *)arg, _IOC_SIZE(cmd)))
>   return -EFAULT;
>  
> - ret = validate_ioctl_arg(cmd, );
> + ret = validate_ioctl_arg(filp, cmd, );
>   if (WARN_ON_ONCE(ret))
>   return ret;
>  
> diff --git a/drivers/staging/android/ion/ion.c 
> b/drivers/staging/android/ion/ion.c
> index fda9756..2c2568b 100644
> --- a/drivers/staging/android/ion/ion.c
> +++ b/drivers/staging/android/ion/ion.c
> @@ -40,6 +40,9 @@
>  
>  #include "ion.h"
>  
> +#define ION_DEV_MAX 32

Why 32?  Where did that number come from?

> +#define ION_NAME "ion"
> +
>  static struct ion_device *internal_dev;
>  static int heap_id;
>  
> @@ -535,15 +538,38 @@ static int debug_shrink_get(void *data, u64 *val)
>  DEFINE_SIMPLE_ATTRIBUTE(debug_shrink_fops, debug_shrink_get,
>   debug_shrink_set, "%llu\n");
>  
> -void ion_device_add_heap(struct ion_heap *heap)
> +static struct device ion_bus = {
> + .init_name = ION_NAME,
> +};

Oh look, a struct device on the stack.  Watch bad things happen :(

This is a dynamic device, make it dynamic, or else you had better know
exactly what you 

Re: [PATCH v6 2/2] staging: ion: create one device entry per heap

[Greg KH]

On Mon, Nov 06, 2017 at 04:59:45PM +0100, Benjamin Gaignard wrote:
> Instead a getting only 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/TODO|  1 -
>  drivers/staging/android/ion/Kconfig |  7 
>  drivers/staging/android/ion/ion-ioctl.c | 18 --
>  drivers/staging/android/ion/ion.c   | 62 
> +
>  drivers/staging/android/ion/ion.h   | 15 ++--
>  5 files changed, 91 insertions(+), 12 deletions(-)
> 
> diff --git a/drivers/staging/android/TODO b/drivers/staging/android/TODO
> index 687e0ea..8a11931 100644
> --- a/drivers/staging/android/TODO
> +++ b/drivers/staging/android/TODO
> @@ -8,7 +8,6 @@ TODO:
>  ion/
>   - Add dt-bindings for remaining heaps (chunk and carveout heaps). This would
> involve putting appropriate bindings in a memory node for Ion to find.
> - - Split /dev/ion up into multiple nodes (e.g. /dev/ion/heap0)
>   - Better test framework (integration with VGEM was suggested)
>  
>  Please send patches to Greg Kroah-Hartman  and Cc:
> diff --git a/drivers/staging/android/ion/Kconfig 
> b/drivers/staging/android/ion/Kconfig
> index a517b2d..cb4666e 100644
> --- a/drivers/staging/android/ion/Kconfig
> +++ b/drivers/staging/android/ion/Kconfig
> @@ -10,6 +10,13 @@ menuconfig ION
> If you're not using Android its probably safe to
> say N here.
>  
> +config ION_LEGACY_DEVICE_API
> + bool "Keep using Ion legacy misc device API"
> + depends on ION

You want to default to Y here, so when you do 'make oldconfig' nothing
breaks, right?

> + help
> +   Choose this option to keep using Ion legacy misc device API
> +   i.e. /dev/ion

You need more text here to describe the trade offs.  Why would I not
want to keep doing this?  What does turning this off get me?  What does
keeping it on keep me from doing?

> +
>  config ION_SYSTEM_HEAP
>   bool "Ion system heap"
>   depends on ION
> diff --git a/drivers/staging/android/ion/ion-ioctl.c 
> b/drivers/staging/android/ion/ion-ioctl.c
> index e26b786..bb5c77b 100644
> --- a/drivers/staging/android/ion/ion-ioctl.c
> +++ b/drivers/staging/android/ion/ion-ioctl.c
> @@ -25,7 +25,8 @@ 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)
>  {
>   switch (cmd) {
>   case ION_IOC_HEAP_QUERY:
> @@ -34,6 +35,19 @@ static int validate_ioctl_arg(unsigned int cmd, union 
> ion_ioctl_arg *arg)
>   arg->query.reserved2 )
>   return -EINVAL;
>   break;
> +
> + case ION_IOC_ALLOC:
> + {
> + int mask = 1 << iminor(filp->f_inode);
> +
> +#ifdef CONFIG_ION_LEGACY_DEVICE_API
> + if (imajor(filp->f_inode) == MISC_MAJOR)
> + return 0;

Why return 0?  Because it is already allocated?

Some comments here as to exactly what you are doing would be nice.

> +#endif
> + if (!(arg->allocation.heap_id_mask & mask))

This doesn't allocate anthing, just check to see if it is?

> + return -EINVAL;
> + break;
> + }
>   default:
>   break;
>   }
> @@ -69,7 +83,7 @@ long ion_ioctl(struct file *filp, unsigned int cmd, 
> unsigned long arg)
>   if (copy_from_user(, (void __user *)arg, _IOC_SIZE(cmd)))
>   return -EFAULT;
>  
> - ret = validate_ioctl_arg(cmd, );
> + ret = validate_ioctl_arg(filp, cmd, );
>   if (WARN_ON_ONCE(ret))
>   return ret;
>  
> diff --git a/drivers/staging/android/ion/ion.c 
> b/drivers/staging/android/ion/ion.c
> index fda9756..2c2568b 100644
> --- a/drivers/staging/android/ion/ion.c
> +++ b/drivers/staging/android/ion/ion.c
> @@ -40,6 +40,9 @@
>  
>  #include "ion.h"
>  
> +#define ION_DEV_MAX 32

Why 32?  Where did that number come from?

> +#define ION_NAME "ion"
> +
>  static struct ion_device *internal_dev;
>  static int heap_id;
>  
> @@ -535,15 +538,38 @@ static int debug_shrink_get(void *data, u64 *val)
>  DEFINE_SIMPLE_ATTRIBUTE(debug_shrink_fops, debug_shrink_get,
>   debug_shrink_set, "%llu\n");
>  
> -void ion_device_add_heap(struct ion_heap *heap)
> +static struct device ion_bus = {
> + .init_name = ION_NAME,
> +};

Oh look, a struct device on the stack.  Watch bad things happen :(

This is a dynamic device, make it dynamic, or else you had better know
exactly what you are doing...

> +
> +static struct bus_type 

Re: [PATCH v6 2/2] staging: ion: create one device entry per heap

[Mark Brown]

On Mon, Nov 27, 2017 at 05:12:23PM +0100, Daniel Vetter wrote:

> commit model ftw, we have 400+ patches for 4.16 already merged and tested
> and all ready, right when -rc1 gets tagged. Makes the merge window the
> most relaxed time of all, because all the other maintainers are drowning
> and wont pester you.

> Just saying, this is an entirely fixable problem :-P

Or even just pre-review things and flag them in a mailbox if you want to
wait for -rc1 before applying things.


signature.asc
Description: PGP signature

Re: [PATCH v6 2/2] staging: ion: create one device entry per heap

[Mark Brown]

On Mon, Nov 27, 2017 at 05:12:23PM +0100, Daniel Vetter wrote:

> commit model ftw, we have 400+ patches for 4.16 already merged and tested
> and all ready, right when -rc1 gets tagged. Makes the merge window the
> most relaxed time of all, because all the other maintainers are drowning
> and wont pester you.

> Just saying, this is an entirely fixable problem :-P

Or even just pre-review things and flag them in a mailbox if you want to
wait for -rc1 before applying things.


signature.asc
Description: PGP signature

Re: [PATCH v6 2/2] staging: ion: create one device entry per heap

[Daniel Vetter]

On Mon, Nov 27, 2017 at 12:43:57PM +0100, Greg Kroah-Hartman wrote:
> On Mon, Nov 27, 2017 at 11:46:18AM +0100, Benjamin Gaignard wrote:
> > 2017-11-09 22:17 GMT+01:00 Laura Abbott :
> > > On 11/06/2017 07:59 AM, Benjamin Gaignard wrote:
> > >>
> > >> Instead a getting only 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.
> > >>
> > >
> > > With this patch, sysfs looks like:
> > >
> > > $ ls /sys/devices/
> > > breakpoint ion platform software system virtual
> > >
> > > From an Ion perspective, you can have
> > >
> > > Acked-by: Laura Abbott 
> > >
> > > Another Ack for the device model stuff would be good but I'll
> > > assume deafening silence means nobody hates it.
> > 
> > Greg, can we get your point of view of this ?
> 
> It's 1 day after the merge window has closed, and my todo patch queue
> looks like this:
>   $ mdfrm -c ~/mail/todo/
>   1523 messages in /home/gregkh/mail/todo/
> 
> Please give me a chance to catch up...

commit model ftw, we have 400+ patches for 4.16 already merged and tested
and all ready, right when -rc1 gets tagged. Makes the merge window the
most relaxed time of all, because all the other maintainers are drowning
and wont pester you.

Just saying, this is an entirely fixable problem :-P

Cheers, Daniel
-- 
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch

Re: [PATCH v6 2/2] staging: ion: create one device entry per heap

[Daniel Vetter]

On Mon, Nov 27, 2017 at 12:43:57PM +0100, Greg Kroah-Hartman wrote:
> On Mon, Nov 27, 2017 at 11:46:18AM +0100, Benjamin Gaignard wrote:
> > 2017-11-09 22:17 GMT+01:00 Laura Abbott :
> > > On 11/06/2017 07:59 AM, Benjamin Gaignard wrote:
> > >>
> > >> Instead a getting only 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.
> > >>
> > >
> > > With this patch, sysfs looks like:
> > >
> > > $ ls /sys/devices/
> > > breakpoint ion platform software system virtual
> > >
> > > From an Ion perspective, you can have
> > >
> > > Acked-by: Laura Abbott 
> > >
> > > Another Ack for the device model stuff would be good but I'll
> > > assume deafening silence means nobody hates it.
> > 
> > Greg, can we get your point of view of this ?
> 
> It's 1 day after the merge window has closed, and my todo patch queue
> looks like this:
>   $ mdfrm -c ~/mail/todo/
>   1523 messages in /home/gregkh/mail/todo/
> 
> Please give me a chance to catch up...

commit model ftw, we have 400+ patches for 4.16 already merged and tested
and all ready, right when -rc1 gets tagged. Makes the merge window the
most relaxed time of all, because all the other maintainers are drowning
and wont pester you.

Just saying, this is an entirely fixable problem :-P

Cheers, Daniel
-- 
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch

Re: [PATCH v6 2/2] staging: ion: create one device entry per heap

[Greg Kroah-Hartman]

On Mon, Nov 27, 2017 at 11:46:18AM +0100, Benjamin Gaignard wrote:
> 2017-11-09 22:17 GMT+01:00 Laura Abbott :
> > On 11/06/2017 07:59 AM, Benjamin Gaignard wrote:
> >>
> >> Instead a getting only 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.
> >>
> >
> > With this patch, sysfs looks like:
> >
> > $ ls /sys/devices/
> > breakpoint ion platform software system virtual
> >
> > From an Ion perspective, you can have
> >
> > Acked-by: Laura Abbott 
> >
> > Another Ack for the device model stuff would be good but I'll
> > assume deafening silence means nobody hates it.
> 
> Greg, can we get your point of view of this ?

It's 1 day after the merge window has closed, and my todo patch queue
looks like this:
$ mdfrm -c ~/mail/todo/
1523 messages in /home/gregkh/mail/todo/

Please give me a chance to catch up...

greg k-h

Re: [PATCH v6 2/2] staging: ion: create one device entry per heap

[Greg Kroah-Hartman]

On Mon, Nov 27, 2017 at 11:46:18AM +0100, Benjamin Gaignard wrote:
> 2017-11-09 22:17 GMT+01:00 Laura Abbott :
> > On 11/06/2017 07:59 AM, Benjamin Gaignard wrote:
> >>
> >> Instead a getting only 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.
> >>
> >
> > With this patch, sysfs looks like:
> >
> > $ ls /sys/devices/
> > breakpoint ion platform software system virtual
> >
> > From an Ion perspective, you can have
> >
> > Acked-by: Laura Abbott 
> >
> > Another Ack for the device model stuff would be good but I'll
> > assume deafening silence means nobody hates it.
> 
> Greg, can we get your point of view of this ?

It's 1 day after the merge window has closed, and my todo patch queue
looks like this:
$ mdfrm -c ~/mail/todo/
1523 messages in /home/gregkh/mail/todo/

Please give me a chance to catch up...

greg k-h

Re: [PATCH v6 2/2] staging: ion: create one device entry per heap

[Benjamin Gaignard]

2017-11-09 22:17 GMT+01:00 Laura Abbott :
> On 11/06/2017 07:59 AM, Benjamin Gaignard wrote:
>>
>> Instead a getting only 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.
>>
>
> With this patch, sysfs looks like:
>
> $ ls /sys/devices/
> breakpoint ion platform software system virtual
>
> From an Ion perspective, you can have
>
> Acked-by: Laura Abbott 
>
> Another Ack for the device model stuff would be good but I'll
> assume deafening silence means nobody hates it.

Greg, can we get your point of view of this ?

Thanks,
Benjamin

>
> Thanks,
> Laura
>
>
>> Signed-off-by: Benjamin Gaignard 
>> ---
>>   drivers/staging/android/TODO|  1 -
>>   drivers/staging/android/ion/Kconfig |  7 
>>   drivers/staging/android/ion/ion-ioctl.c | 18 --
>>   drivers/staging/android/ion/ion.c   | 62
>> +
>>   drivers/staging/android/ion/ion.h   | 15 ++--
>>   5 files changed, 91 insertions(+), 12 deletions(-)
>>
>> diff --git a/drivers/staging/android/TODO b/drivers/staging/android/TODO
>> index 687e0ea..8a11931 100644
>> --- a/drivers/staging/android/TODO
>> +++ b/drivers/staging/android/TODO
>> @@ -8,7 +8,6 @@ TODO:
>>   ion/
>>- Add dt-bindings for remaining heaps (chunk and carveout heaps). This
>> would
>>  involve putting appropriate bindings in a memory node for Ion to
>> find.
>> - - Split /dev/ion up into multiple nodes (e.g. /dev/ion/heap0)
>>- Better test framework (integration with VGEM was suggested)
>> Please send patches to Greg Kroah-Hartman  and Cc:
>> diff --git a/drivers/staging/android/ion/Kconfig
>> b/drivers/staging/android/ion/Kconfig
>> index a517b2d..cb4666e 100644
>> --- a/drivers/staging/android/ion/Kconfig
>> +++ b/drivers/staging/android/ion/Kconfig
>> @@ -10,6 +10,13 @@ menuconfig ION
>>   If you're not using Android its probably safe to
>>   say N here.
>>   +config ION_LEGACY_DEVICE_API
>> +   bool "Keep using Ion legacy misc device API"
>> +   depends on ION
>> +   help
>> + Choose this option to keep using Ion legacy misc device API
>> + i.e. /dev/ion
>> +
>>   config ION_SYSTEM_HEAP
>> bool "Ion system heap"
>> depends on ION
>> diff --git a/drivers/staging/android/ion/ion-ioctl.c
>> b/drivers/staging/android/ion/ion-ioctl.c
>> index e26b786..bb5c77b 100644
>> --- a/drivers/staging/android/ion/ion-ioctl.c
>> +++ b/drivers/staging/android/ion/ion-ioctl.c
>> @@ -25,7 +25,8 @@ 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)
>>   {
>> switch (cmd) {
>> case ION_IOC_HEAP_QUERY:
>> @@ -34,6 +35,19 @@ static int validate_ioctl_arg(unsigned int cmd, union
>> ion_ioctl_arg *arg)
>> arg->query.reserved2 )
>> return -EINVAL;
>> break;
>> +
>> +   case ION_IOC_ALLOC:
>> +   {
>> +   int mask = 1 << iminor(filp->f_inode);
>> +
>> +#ifdef CONFIG_ION_LEGACY_DEVICE_API
>> +   if (imajor(filp->f_inode) == MISC_MAJOR)
>> +   return 0;
>> +#endif
>> +   if (!(arg->allocation.heap_id_mask & mask))
>> +   return -EINVAL;
>> +   break;
>> +   }
>> default:
>> break;
>> }
>> @@ -69,7 +83,7 @@ long ion_ioctl(struct file *filp, unsigned int cmd,
>> unsigned long arg)
>> if (copy_from_user(, (void __user *)arg, _IOC_SIZE(cmd)))
>> return -EFAULT;
>>   - ret = validate_ioctl_arg(cmd, );
>> +   ret = validate_ioctl_arg(filp, cmd, );
>> if (WARN_ON_ONCE(ret))
>> return ret;
>>   diff --git a/drivers/staging/android/ion/ion.c
>> b/drivers/staging/android/ion/ion.c
>> index fda9756..2c2568b 100644
>> --- a/drivers/staging/android/ion/ion.c
>> +++ b/drivers/staging/android/ion/ion.c
>> @@ -40,6 +40,9 @@
>> #include "ion.h"
>>   +#define ION_DEV_MAX 32
>> +#define ION_NAME "ion"
>> +
>>   static struct ion_device *internal_dev;
>>   static int heap_id;
>>   @@ -535,15 +538,38 @@ static int debug_shrink_get(void *data, u64 *val)
>>   DEFINE_SIMPLE_ATTRIBUTE(debug_shrink_fops, debug_shrink_get,
>> debug_shrink_set, "%llu\n");
>>   -void ion_device_add_heap(struct ion_heap *heap)
>> +static struct device ion_bus = {
>> +   .init_name = ION_NAME,
>> +};
>> +

Re: [PATCH v6 2/2] staging: ion: create one device entry per heap

[Benjamin Gaignard]

2017-11-09 22:17 GMT+01:00 Laura Abbott :
> On 11/06/2017 07:59 AM, Benjamin Gaignard wrote:
>>
>> Instead a getting only 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.
>>
>
> With this patch, sysfs looks like:
>
> $ ls /sys/devices/
> breakpoint ion platform software system virtual
>
> From an Ion perspective, you can have
>
> Acked-by: Laura Abbott 
>
> Another Ack for the device model stuff would be good but I'll
> assume deafening silence means nobody hates it.

Greg, can we get your point of view of this ?

Thanks,
Benjamin

>
> Thanks,
> Laura
>
>
>> Signed-off-by: Benjamin Gaignard 
>> ---
>>   drivers/staging/android/TODO|  1 -
>>   drivers/staging/android/ion/Kconfig |  7 
>>   drivers/staging/android/ion/ion-ioctl.c | 18 --
>>   drivers/staging/android/ion/ion.c   | 62
>> +
>>   drivers/staging/android/ion/ion.h   | 15 ++--
>>   5 files changed, 91 insertions(+), 12 deletions(-)
>>
>> diff --git a/drivers/staging/android/TODO b/drivers/staging/android/TODO
>> index 687e0ea..8a11931 100644
>> --- a/drivers/staging/android/TODO
>> +++ b/drivers/staging/android/TODO
>> @@ -8,7 +8,6 @@ TODO:
>>   ion/
>>- Add dt-bindings for remaining heaps (chunk and carveout heaps). This
>> would
>>  involve putting appropriate bindings in a memory node for Ion to
>> find.
>> - - Split /dev/ion up into multiple nodes (e.g. /dev/ion/heap0)
>>- Better test framework (integration with VGEM was suggested)
>> Please send patches to Greg Kroah-Hartman  and Cc:
>> diff --git a/drivers/staging/android/ion/Kconfig
>> b/drivers/staging/android/ion/Kconfig
>> index a517b2d..cb4666e 100644
>> --- a/drivers/staging/android/ion/Kconfig
>> +++ b/drivers/staging/android/ion/Kconfig
>> @@ -10,6 +10,13 @@ menuconfig ION
>>   If you're not using Android its probably safe to
>>   say N here.
>>   +config ION_LEGACY_DEVICE_API
>> +   bool "Keep using Ion legacy misc device API"
>> +   depends on ION
>> +   help
>> + Choose this option to keep using Ion legacy misc device API
>> + i.e. /dev/ion
>> +
>>   config ION_SYSTEM_HEAP
>> bool "Ion system heap"
>> depends on ION
>> diff --git a/drivers/staging/android/ion/ion-ioctl.c
>> b/drivers/staging/android/ion/ion-ioctl.c
>> index e26b786..bb5c77b 100644
>> --- a/drivers/staging/android/ion/ion-ioctl.c
>> +++ b/drivers/staging/android/ion/ion-ioctl.c
>> @@ -25,7 +25,8 @@ 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)
>>   {
>> switch (cmd) {
>> case ION_IOC_HEAP_QUERY:
>> @@ -34,6 +35,19 @@ static int validate_ioctl_arg(unsigned int cmd, union
>> ion_ioctl_arg *arg)
>> arg->query.reserved2 )
>> return -EINVAL;
>> break;
>> +
>> +   case ION_IOC_ALLOC:
>> +   {
>> +   int mask = 1 << iminor(filp->f_inode);
>> +
>> +#ifdef CONFIG_ION_LEGACY_DEVICE_API
>> +   if (imajor(filp->f_inode) == MISC_MAJOR)
>> +   return 0;
>> +#endif
>> +   if (!(arg->allocation.heap_id_mask & mask))
>> +   return -EINVAL;
>> +   break;
>> +   }
>> default:
>> break;
>> }
>> @@ -69,7 +83,7 @@ long ion_ioctl(struct file *filp, unsigned int cmd,
>> unsigned long arg)
>> if (copy_from_user(, (void __user *)arg, _IOC_SIZE(cmd)))
>> return -EFAULT;
>>   - ret = validate_ioctl_arg(cmd, );
>> +   ret = validate_ioctl_arg(filp, cmd, );
>> if (WARN_ON_ONCE(ret))
>> return ret;
>>   diff --git a/drivers/staging/android/ion/ion.c
>> b/drivers/staging/android/ion/ion.c
>> index fda9756..2c2568b 100644
>> --- a/drivers/staging/android/ion/ion.c
>> +++ b/drivers/staging/android/ion/ion.c
>> @@ -40,6 +40,9 @@
>> #include "ion.h"
>>   +#define ION_DEV_MAX 32
>> +#define ION_NAME "ion"
>> +
>>   static struct ion_device *internal_dev;
>>   static int heap_id;
>>   @@ -535,15 +538,38 @@ static int debug_shrink_get(void *data, u64 *val)
>>   DEFINE_SIMPLE_ATTRIBUTE(debug_shrink_fops, debug_shrink_get,
>> debug_shrink_set, "%llu\n");
>>   -void ion_device_add_heap(struct ion_heap *heap)
>> +static struct device ion_bus = {
>> +   .init_name = ION_NAME,
>> +};
>> +
>> +static struct bus_type ion_bus_type = {
>> +   .name = ION_NAME,
>> +};
>> +
>> 

Re: [PATCH v6 2/2] staging: ion: create one device entry per heap

[Laura Abbott]

On 11/06/2017 07:59 AM, Benjamin Gaignard wrote:

Instead a getting only 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.



With this patch, sysfs looks like:

$ ls /sys/devices/
breakpoint ion platform software system virtual

From an Ion perspective, you can have

Acked-by: Laura Abbott 

Another Ack for the device model stuff would be good but I'll
assume deafening silence means nobody hates it.

Thanks,
Laura


Signed-off-by: Benjamin Gaignard 
---
  drivers/staging/android/TODO|  1 -
  drivers/staging/android/ion/Kconfig |  7 
  drivers/staging/android/ion/ion-ioctl.c | 18 --
  drivers/staging/android/ion/ion.c   | 62 +
  drivers/staging/android/ion/ion.h   | 15 ++--
  5 files changed, 91 insertions(+), 12 deletions(-)

diff --git a/drivers/staging/android/TODO b/drivers/staging/android/TODO
index 687e0ea..8a11931 100644
--- a/drivers/staging/android/TODO
+++ b/drivers/staging/android/TODO
@@ -8,7 +8,6 @@ TODO:
  ion/
   - Add dt-bindings for remaining heaps (chunk and carveout heaps). This would
 involve putting appropriate bindings in a memory node for Ion to find.
- - Split /dev/ion up into multiple nodes (e.g. /dev/ion/heap0)
   - Better test framework (integration with VGEM was suggested)
  
  Please send patches to Greg Kroah-Hartman  and Cc:

diff --git a/drivers/staging/android/ion/Kconfig 
b/drivers/staging/android/ion/Kconfig
index a517b2d..cb4666e 100644
--- a/drivers/staging/android/ion/Kconfig
+++ b/drivers/staging/android/ion/Kconfig
@@ -10,6 +10,13 @@ menuconfig ION
  If you're not using Android its probably safe to
  say N here.
  
+config ION_LEGACY_DEVICE_API

+   bool "Keep using Ion legacy misc device API"
+   depends on ION
+   help
+ Choose this option to keep using Ion legacy misc device API
+ i.e. /dev/ion
+
  config ION_SYSTEM_HEAP
bool "Ion system heap"
depends on ION
diff --git a/drivers/staging/android/ion/ion-ioctl.c 
b/drivers/staging/android/ion/ion-ioctl.c
index e26b786..bb5c77b 100644
--- a/drivers/staging/android/ion/ion-ioctl.c
+++ b/drivers/staging/android/ion/ion-ioctl.c
@@ -25,7 +25,8 @@ 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)
  {
switch (cmd) {
case ION_IOC_HEAP_QUERY:
@@ -34,6 +35,19 @@ static int validate_ioctl_arg(unsigned int cmd, union 
ion_ioctl_arg *arg)
arg->query.reserved2 )
return -EINVAL;
break;
+
+   case ION_IOC_ALLOC:
+   {
+   int mask = 1 << iminor(filp->f_inode);
+
+#ifdef CONFIG_ION_LEGACY_DEVICE_API
+   if (imajor(filp->f_inode) == MISC_MAJOR)
+   return 0;
+#endif
+   if (!(arg->allocation.heap_id_mask & mask))
+   return -EINVAL;
+   break;
+   }
default:
break;
}
@@ -69,7 +83,7 @@ long ion_ioctl(struct file *filp, unsigned int cmd, unsigned 
long arg)
if (copy_from_user(, (void __user *)arg, _IOC_SIZE(cmd)))
return -EFAULT;
  
-	ret = validate_ioctl_arg(cmd, );

+   ret = validate_ioctl_arg(filp, cmd, );
if (WARN_ON_ONCE(ret))
return ret;
  
diff --git a/drivers/staging/android/ion/ion.c b/drivers/staging/android/ion/ion.c

index fda9756..2c2568b 100644
--- a/drivers/staging/android/ion/ion.c
+++ b/drivers/staging/android/ion/ion.c
@@ -40,6 +40,9 @@
  
  #include "ion.h"
  
+#define ION_DEV_MAX 32

+#define ION_NAME "ion"
+
  static struct ion_device *internal_dev;
  static int heap_id;
  
@@ -535,15 +538,38 @@ static int debug_shrink_get(void *data, u64 *val)

  DEFINE_SIMPLE_ATTRIBUTE(debug_shrink_fops, debug_shrink_get,
debug_shrink_set, "%llu\n");
  
-void ion_device_add_heap(struct ion_heap *heap)

+static struct device ion_bus = {
+   .init_name = ION_NAME,
+};
+
+static struct bus_type ion_bus_type = {
+   .name = ION_NAME,
+};
+
+int ion_device_add_heap(struct ion_heap *heap)
  {
struct dentry *debug_file;
struct ion_device *dev = internal_dev;
+   int ret = 0;
  
  	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.parent 

Re: [PATCH v6 2/2] staging: ion: create one device entry per heap

[Laura Abbott]

On 11/06/2017 07:59 AM, Benjamin Gaignard wrote:

Instead a getting only 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.



With this patch, sysfs looks like:

$ ls /sys/devices/
breakpoint ion platform software system virtual

From an Ion perspective, you can have

Acked-by: Laura Abbott 

Another Ack for the device model stuff would be good but I'll
assume deafening silence means nobody hates it.

Thanks,
Laura


Signed-off-by: Benjamin Gaignard 
---
  drivers/staging/android/TODO|  1 -
  drivers/staging/android/ion/Kconfig |  7 
  drivers/staging/android/ion/ion-ioctl.c | 18 --
  drivers/staging/android/ion/ion.c   | 62 +
  drivers/staging/android/ion/ion.h   | 15 ++--
  5 files changed, 91 insertions(+), 12 deletions(-)

diff --git a/drivers/staging/android/TODO b/drivers/staging/android/TODO
index 687e0ea..8a11931 100644
--- a/drivers/staging/android/TODO
+++ b/drivers/staging/android/TODO
@@ -8,7 +8,6 @@ TODO:
  ion/
   - Add dt-bindings for remaining heaps (chunk and carveout heaps). This would
 involve putting appropriate bindings in a memory node for Ion to find.
- - Split /dev/ion up into multiple nodes (e.g. /dev/ion/heap0)
   - Better test framework (integration with VGEM was suggested)
  
  Please send patches to Greg Kroah-Hartman  and Cc:

diff --git a/drivers/staging/android/ion/Kconfig 
b/drivers/staging/android/ion/Kconfig
index a517b2d..cb4666e 100644
--- a/drivers/staging/android/ion/Kconfig
+++ b/drivers/staging/android/ion/Kconfig
@@ -10,6 +10,13 @@ menuconfig ION
  If you're not using Android its probably safe to
  say N here.
  
+config ION_LEGACY_DEVICE_API

+   bool "Keep using Ion legacy misc device API"
+   depends on ION
+   help
+ Choose this option to keep using Ion legacy misc device API
+ i.e. /dev/ion
+
  config ION_SYSTEM_HEAP
bool "Ion system heap"
depends on ION
diff --git a/drivers/staging/android/ion/ion-ioctl.c 
b/drivers/staging/android/ion/ion-ioctl.c
index e26b786..bb5c77b 100644
--- a/drivers/staging/android/ion/ion-ioctl.c
+++ b/drivers/staging/android/ion/ion-ioctl.c
@@ -25,7 +25,8 @@ 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)
  {
switch (cmd) {
case ION_IOC_HEAP_QUERY:
@@ -34,6 +35,19 @@ static int validate_ioctl_arg(unsigned int cmd, union 
ion_ioctl_arg *arg)
arg->query.reserved2 )
return -EINVAL;
break;
+
+   case ION_IOC_ALLOC:
+   {
+   int mask = 1 << iminor(filp->f_inode);
+
+#ifdef CONFIG_ION_LEGACY_DEVICE_API
+   if (imajor(filp->f_inode) == MISC_MAJOR)
+   return 0;
+#endif
+   if (!(arg->allocation.heap_id_mask & mask))
+   return -EINVAL;
+   break;
+   }
default:
break;
}
@@ -69,7 +83,7 @@ long ion_ioctl(struct file *filp, unsigned int cmd, unsigned 
long arg)
if (copy_from_user(, (void __user *)arg, _IOC_SIZE(cmd)))
return -EFAULT;
  
-	ret = validate_ioctl_arg(cmd, );

+   ret = validate_ioctl_arg(filp, cmd, );
if (WARN_ON_ONCE(ret))
return ret;
  
diff --git a/drivers/staging/android/ion/ion.c b/drivers/staging/android/ion/ion.c

index fda9756..2c2568b 100644
--- a/drivers/staging/android/ion/ion.c
+++ b/drivers/staging/android/ion/ion.c
@@ -40,6 +40,9 @@
  
  #include "ion.h"
  
+#define ION_DEV_MAX 32

+#define ION_NAME "ion"
+
  static struct ion_device *internal_dev;
  static int heap_id;
  
@@ -535,15 +538,38 @@ static int debug_shrink_get(void *data, u64 *val)

  DEFINE_SIMPLE_ATTRIBUTE(debug_shrink_fops, debug_shrink_get,
debug_shrink_set, "%llu\n");
  
-void ion_device_add_heap(struct ion_heap *heap)

+static struct device ion_bus = {
+   .init_name = ION_NAME,
+};
+
+static struct bus_type ion_bus_type = {
+   .name = ION_NAME,
+};
+
+int ion_device_add_heap(struct ion_heap *heap)
  {
struct dentry *debug_file;
struct ion_device *dev = internal_dev;
+   int ret = 0;
  
  	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.parent = _bus;
+   heap->ddev.bus = _bus_type;
+   

[PATCH v6 2/2] staging: ion: create one device entry per heap

[Benjamin Gaignard]

Instead a getting only 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/TODO|  1 -
 drivers/staging/android/ion/Kconfig |  7 
 drivers/staging/android/ion/ion-ioctl.c | 18 --
 drivers/staging/android/ion/ion.c   | 62 +
 drivers/staging/android/ion/ion.h   | 15 ++--
 5 files changed, 91 insertions(+), 12 deletions(-)

diff --git a/drivers/staging/android/TODO b/drivers/staging/android/TODO
index 687e0ea..8a11931 100644
--- a/drivers/staging/android/TODO
+++ b/drivers/staging/android/TODO
@@ -8,7 +8,6 @@ TODO:
 ion/
  - Add dt-bindings for remaining heaps (chunk and carveout heaps). This would
involve putting appropriate bindings in a memory node for Ion to find.
- - Split /dev/ion up into multiple nodes (e.g. /dev/ion/heap0)
  - Better test framework (integration with VGEM was suggested)
 
 Please send patches to Greg Kroah-Hartman  and Cc:
diff --git a/drivers/staging/android/ion/Kconfig 
b/drivers/staging/android/ion/Kconfig
index a517b2d..cb4666e 100644
--- a/drivers/staging/android/ion/Kconfig
+++ b/drivers/staging/android/ion/Kconfig
@@ -10,6 +10,13 @@ menuconfig ION
  If you're not using Android its probably safe to
  say N here.
 
+config ION_LEGACY_DEVICE_API
+   bool "Keep using Ion legacy misc device API"
+   depends on ION
+   help
+ Choose this option to keep using Ion legacy misc device API
+ i.e. /dev/ion
+
 config ION_SYSTEM_HEAP
bool "Ion system heap"
depends on ION
diff --git a/drivers/staging/android/ion/ion-ioctl.c 
b/drivers/staging/android/ion/ion-ioctl.c
index e26b786..bb5c77b 100644
--- a/drivers/staging/android/ion/ion-ioctl.c
+++ b/drivers/staging/android/ion/ion-ioctl.c
@@ -25,7 +25,8 @@ 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)
 {
switch (cmd) {
case ION_IOC_HEAP_QUERY:
@@ -34,6 +35,19 @@ static int validate_ioctl_arg(unsigned int cmd, union 
ion_ioctl_arg *arg)
arg->query.reserved2 )
return -EINVAL;
break;
+
+   case ION_IOC_ALLOC:
+   {
+   int mask = 1 << iminor(filp->f_inode);
+
+#ifdef CONFIG_ION_LEGACY_DEVICE_API
+   if (imajor(filp->f_inode) == MISC_MAJOR)
+   return 0;
+#endif
+   if (!(arg->allocation.heap_id_mask & mask))
+   return -EINVAL;
+   break;
+   }
default:
break;
}
@@ -69,7 +83,7 @@ long ion_ioctl(struct file *filp, unsigned int cmd, unsigned 
long arg)
if (copy_from_user(, (void __user *)arg, _IOC_SIZE(cmd)))
return -EFAULT;
 
-   ret = validate_ioctl_arg(cmd, );
+   ret = validate_ioctl_arg(filp, cmd, );
if (WARN_ON_ONCE(ret))
return ret;
 
diff --git a/drivers/staging/android/ion/ion.c 
b/drivers/staging/android/ion/ion.c
index fda9756..2c2568b 100644
--- a/drivers/staging/android/ion/ion.c
+++ b/drivers/staging/android/ion/ion.c
@@ -40,6 +40,9 @@
 
 #include "ion.h"
 
+#define ION_DEV_MAX 32
+#define ION_NAME "ion"
+
 static struct ion_device *internal_dev;
 static int heap_id;
 
@@ -535,15 +538,38 @@ static int debug_shrink_get(void *data, u64 *val)
 DEFINE_SIMPLE_ATTRIBUTE(debug_shrink_fops, debug_shrink_get,
debug_shrink_set, "%llu\n");
 
-void ion_device_add_heap(struct ion_heap *heap)
+static struct device ion_bus = {
+   .init_name = ION_NAME,
+};
+
+static struct bus_type ion_bus_type = {
+   .name = ION_NAME,
+};
+
+int ion_device_add_heap(struct ion_heap *heap)
 {
struct dentry *debug_file;
struct ion_device *dev = internal_dev;
+   int ret = 0;
 
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.parent = _bus;
+   heap->ddev.bus = _bus_type;
+   heap->ddev.devt = MKDEV(MAJOR(internal_dev->devt), heap_id);
+   dev_set_name(>ddev, ION_NAME"%d", heap_id);
+   device_initialize(>ddev);
+   cdev_init(>chrdev, _fops);
+   heap->chrdev.owner = THIS_MODULE;
+   ret = cdev_device_add(>chrdev, >ddev);
+   if (ret < 0)
+   return ret;
+

[PATCH v6 2/2] staging: ion: create one device entry per heap

[Benjamin Gaignard]

Instead a getting only 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/TODO|  1 -
 drivers/staging/android/ion/Kconfig |  7 
 drivers/staging/android/ion/ion-ioctl.c | 18 --
 drivers/staging/android/ion/ion.c   | 62 +
 drivers/staging/android/ion/ion.h   | 15 ++--
 5 files changed, 91 insertions(+), 12 deletions(-)

diff --git a/drivers/staging/android/TODO b/drivers/staging/android/TODO
index 687e0ea..8a11931 100644
--- a/drivers/staging/android/TODO
+++ b/drivers/staging/android/TODO
@@ -8,7 +8,6 @@ TODO:
 ion/
  - Add dt-bindings for remaining heaps (chunk and carveout heaps). This would
involve putting appropriate bindings in a memory node for Ion to find.
- - Split /dev/ion up into multiple nodes (e.g. /dev/ion/heap0)
  - Better test framework (integration with VGEM was suggested)
 
 Please send patches to Greg Kroah-Hartman  and Cc:
diff --git a/drivers/staging/android/ion/Kconfig 
b/drivers/staging/android/ion/Kconfig
index a517b2d..cb4666e 100644
--- a/drivers/staging/android/ion/Kconfig
+++ b/drivers/staging/android/ion/Kconfig
@@ -10,6 +10,13 @@ menuconfig ION
  If you're not using Android its probably safe to
  say N here.
 
+config ION_LEGACY_DEVICE_API
+   bool "Keep using Ion legacy misc device API"
+   depends on ION
+   help
+ Choose this option to keep using Ion legacy misc device API
+ i.e. /dev/ion
+
 config ION_SYSTEM_HEAP
bool "Ion system heap"
depends on ION
diff --git a/drivers/staging/android/ion/ion-ioctl.c 
b/drivers/staging/android/ion/ion-ioctl.c
index e26b786..bb5c77b 100644
--- a/drivers/staging/android/ion/ion-ioctl.c
+++ b/drivers/staging/android/ion/ion-ioctl.c
@@ -25,7 +25,8 @@ 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)
 {
switch (cmd) {
case ION_IOC_HEAP_QUERY:
@@ -34,6 +35,19 @@ static int validate_ioctl_arg(unsigned int cmd, union 
ion_ioctl_arg *arg)
arg->query.reserved2 )
return -EINVAL;
break;
+
+   case ION_IOC_ALLOC:
+   {
+   int mask = 1 << iminor(filp->f_inode);
+
+#ifdef CONFIG_ION_LEGACY_DEVICE_API
+   if (imajor(filp->f_inode) == MISC_MAJOR)
+   return 0;
+#endif
+   if (!(arg->allocation.heap_id_mask & mask))
+   return -EINVAL;
+   break;
+   }
default:
break;
}
@@ -69,7 +83,7 @@ long ion_ioctl(struct file *filp, unsigned int cmd, unsigned 
long arg)
if (copy_from_user(, (void __user *)arg, _IOC_SIZE(cmd)))
return -EFAULT;
 
-   ret = validate_ioctl_arg(cmd, );
+   ret = validate_ioctl_arg(filp, cmd, );
if (WARN_ON_ONCE(ret))
return ret;
 
diff --git a/drivers/staging/android/ion/ion.c 
b/drivers/staging/android/ion/ion.c
index fda9756..2c2568b 100644
--- a/drivers/staging/android/ion/ion.c
+++ b/drivers/staging/android/ion/ion.c
@@ -40,6 +40,9 @@
 
 #include "ion.h"
 
+#define ION_DEV_MAX 32
+#define ION_NAME "ion"
+
 static struct ion_device *internal_dev;
 static int heap_id;
 
@@ -535,15 +538,38 @@ static int debug_shrink_get(void *data, u64 *val)
 DEFINE_SIMPLE_ATTRIBUTE(debug_shrink_fops, debug_shrink_get,
debug_shrink_set, "%llu\n");
 
-void ion_device_add_heap(struct ion_heap *heap)
+static struct device ion_bus = {
+   .init_name = ION_NAME,
+};
+
+static struct bus_type ion_bus_type = {
+   .name = ION_NAME,
+};
+
+int ion_device_add_heap(struct ion_heap *heap)
 {
struct dentry *debug_file;
struct ion_device *dev = internal_dev;
+   int ret = 0;
 
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.parent = _bus;
+   heap->ddev.bus = _bus_type;
+   heap->ddev.devt = MKDEV(MAJOR(internal_dev->devt), heap_id);
+   dev_set_name(>ddev, ION_NAME"%d", heap_id);
+   device_initialize(>ddev);
+   cdev_init(>chrdev, _fops);
+   heap->chrdev.owner = THIS_MODULE;
+   ret = cdev_device_add(>chrdev, >ddev);
+   if (ret < 0)
+   return ret;
+
spin_lock_init(>free_lock);

Re: [PATCH v6 2/2] staging: ion: create one device entry per heap

[Greg KH]

On Mon, Nov 06, 2017 at 03:42:04PM +0100, Benjamin Gaignard wrote:
> 2017-11-02 12:10 GMT+01:00 Mark Brown :
> > On Thu, Nov 02, 2017 at 11:44:07AM +0100, Greg KH wrote:
> >> On Tue, Oct 31, 2017 at 07:11:53PM +, Mark Brown wrote:
> >
> >> > There was a discussion a while ago in the context of I2C/SPI MFDs
> >> > which concluded that if you need a bus and it's going to be effectively
> >> > noop then you should just use the platform bus as anything else will
> >> > consist almost entirely of cut'n'paste from the platform bus with some
> >> > light sed usage and code duplication is bad.  It's not super lovely as
> >> > it's not actually a memory mapped device but it's the best idea we've
> >> > got.
> >
> >> Ugh, I hate that.  What's wrong with using a "virtual" device instead?
> >
> > It was the duplication, initially everyone was making buses.
> >
> >> I can create a "virtual" bus for things like this if they really want a
> >> "simple" bus, abusing platform for this is the major reason I hate the
> >> platform bus code...
> >
> > In the MFD case they're physical devices, they're just usually on the
> > wrong side of an I2C or SPI link.  Plus MFD already handles platform
> > devices for things that are memory mapped so it's a bit of a more
> > natural fit there.
> 
> What I can do is to register an ion bus (like cec one for example),
> add one ion parent device so heaps will appear in /sys/bus/ion/ion*
> and /sys/devices/ion/ion*
> 
> Does that could sound good enough ?

I would like to see that...

thanks,

greg k-h

Re: [PATCH v6 2/2] staging: ion: create one device entry per heap

[Greg KH]

On Mon, Nov 06, 2017 at 03:42:04PM +0100, Benjamin Gaignard wrote:
> 2017-11-02 12:10 GMT+01:00 Mark Brown :
> > On Thu, Nov 02, 2017 at 11:44:07AM +0100, Greg KH wrote:
> >> On Tue, Oct 31, 2017 at 07:11:53PM +, Mark Brown wrote:
> >
> >> > There was a discussion a while ago in the context of I2C/SPI MFDs
> >> > which concluded that if you need a bus and it's going to be effectively
> >> > noop then you should just use the platform bus as anything else will
> >> > consist almost entirely of cut'n'paste from the platform bus with some
> >> > light sed usage and code duplication is bad.  It's not super lovely as
> >> > it's not actually a memory mapped device but it's the best idea we've
> >> > got.
> >
> >> Ugh, I hate that.  What's wrong with using a "virtual" device instead?
> >
> > It was the duplication, initially everyone was making buses.
> >
> >> I can create a "virtual" bus for things like this if they really want a
> >> "simple" bus, abusing platform for this is the major reason I hate the
> >> platform bus code...
> >
> > In the MFD case they're physical devices, they're just usually on the
> > wrong side of an I2C or SPI link.  Plus MFD already handles platform
> > devices for things that are memory mapped so it's a bit of a more
> > natural fit there.
> 
> What I can do is to register an ion bus (like cec one for example),
> add one ion parent device so heaps will appear in /sys/bus/ion/ion*
> and /sys/devices/ion/ion*
> 
> Does that could sound good enough ?

I would like to see that...

thanks,

greg k-h

Re: [PATCH v6 2/2] staging: ion: create one device entry per heap

[Benjamin Gaignard]

2017-11-02 12:10 GMT+01:00 Mark Brown :
> On Thu, Nov 02, 2017 at 11:44:07AM +0100, Greg KH wrote:
>> On Tue, Oct 31, 2017 at 07:11:53PM +, Mark Brown wrote:
>
>> > There was a discussion a while ago in the context of I2C/SPI MFDs
>> > which concluded that if you need a bus and it's going to be effectively
>> > noop then you should just use the platform bus as anything else will
>> > consist almost entirely of cut'n'paste from the platform bus with some
>> > light sed usage and code duplication is bad.  It's not super lovely as
>> > it's not actually a memory mapped device but it's the best idea we've
>> > got.
>
>> Ugh, I hate that.  What's wrong with using a "virtual" device instead?
>
> It was the duplication, initially everyone was making buses.
>
>> I can create a "virtual" bus for things like this if they really want a
>> "simple" bus, abusing platform for this is the major reason I hate the
>> platform bus code...
>
> In the MFD case they're physical devices, they're just usually on the
> wrong side of an I2C or SPI link.  Plus MFD already handles platform
> devices for things that are memory mapped so it's a bit of a more
> natural fit there.

What I can do is to register an ion bus (like cec one for example),
add one ion parent device so heaps will appear in /sys/bus/ion/ion*
and /sys/devices/ion/ion*

Does that could sound good enough ?

Benjamin

Re: [PATCH v6 2/2] staging: ion: create one device entry per heap

[Benjamin Gaignard]

2017-11-02 12:10 GMT+01:00 Mark Brown :
> On Thu, Nov 02, 2017 at 11:44:07AM +0100, Greg KH wrote:
>> On Tue, Oct 31, 2017 at 07:11:53PM +, Mark Brown wrote:
>
>> > There was a discussion a while ago in the context of I2C/SPI MFDs
>> > which concluded that if you need a bus and it's going to be effectively
>> > noop then you should just use the platform bus as anything else will
>> > consist almost entirely of cut'n'paste from the platform bus with some
>> > light sed usage and code duplication is bad.  It's not super lovely as
>> > it's not actually a memory mapped device but it's the best idea we've
>> > got.
>
>> Ugh, I hate that.  What's wrong with using a "virtual" device instead?
>
> It was the duplication, initially everyone was making buses.
>
>> I can create a "virtual" bus for things like this if they really want a
>> "simple" bus, abusing platform for this is the major reason I hate the
>> platform bus code...
>
> In the MFD case they're physical devices, they're just usually on the
> wrong side of an I2C or SPI link.  Plus MFD already handles platform
> devices for things that are memory mapped so it's a bit of a more
> natural fit there.

What I can do is to register an ion bus (like cec one for example),
add one ion parent device so heaps will appear in /sys/bus/ion/ion*
and /sys/devices/ion/ion*

Does that could sound good enough ?

Benjamin

Re: [PATCH v6 2/2] staging: ion: create one device entry per heap

[Mark Brown]

On Thu, Nov 02, 2017 at 11:44:07AM +0100, Greg KH wrote:
> On Tue, Oct 31, 2017 at 07:11:53PM +, Mark Brown wrote:

> > There was a discussion a while ago in the context of I2C/SPI MFDs
> > which concluded that if you need a bus and it's going to be effectively
> > noop then you should just use the platform bus as anything else will
> > consist almost entirely of cut'n'paste from the platform bus with some
> > light sed usage and code duplication is bad.  It's not super lovely as
> > it's not actually a memory mapped device but it's the best idea we've
> > got.

> Ugh, I hate that.  What's wrong with using a "virtual" device instead?

It was the duplication, initially everyone was making buses.  

> I can create a "virtual" bus for things like this if they really want a
> "simple" bus, abusing platform for this is the major reason I hate the
> platform bus code...

In the MFD case they're physical devices, they're just usually on the
wrong side of an I2C or SPI link.  Plus MFD already handles platform
devices for things that are memory mapped so it's a bit of a more
natural fit there.


signature.asc
Description: PGP signature

Re: [PATCH v6 2/2] staging: ion: create one device entry per heap

[Mark Brown]

On Thu, Nov 02, 2017 at 11:44:07AM +0100, Greg KH wrote:
> On Tue, Oct 31, 2017 at 07:11:53PM +, Mark Brown wrote:

> > There was a discussion a while ago in the context of I2C/SPI MFDs
> > which concluded that if you need a bus and it's going to be effectively
> > noop then you should just use the platform bus as anything else will
> > consist almost entirely of cut'n'paste from the platform bus with some
> > light sed usage and code duplication is bad.  It's not super lovely as
> > it's not actually a memory mapped device but it's the best idea we've
> > got.

> Ugh, I hate that.  What's wrong with using a "virtual" device instead?

It was the duplication, initially everyone was making buses.  

> I can create a "virtual" bus for things like this if they really want a
> "simple" bus, abusing platform for this is the major reason I hate the
> platform bus code...

In the MFD case they're physical devices, they're just usually on the
wrong side of an I2C or SPI link.  Plus MFD already handles platform
devices for things that are memory mapped so it's a bit of a more
natural fit there.


signature.asc
Description: PGP signature

Re: [PATCH v6 2/2] staging: ion: create one device entry per heap

[Greg KH]

On Tue, Oct 31, 2017 at 07:11:53PM +, Mark Brown wrote:
> On Tue, Oct 31, 2017 at 12:03:35PM -0700, Laura Abbott wrote:
> 
> > I'm not a fan of the platform bus but I have mixed feelings about
> > creating a dedicated bus type. I guess if we really need a bus
> > type we can do it later?
> 
> There was a discussion a while ago in the context of I2C/SPI MFDs
> which concluded that if you need a bus and it's going to be effectively
> noop then you should just use the platform bus as anything else will
> consist almost entirely of cut'n'paste from the platform bus with some
> light sed usage and code duplication is bad.  It's not super lovely as
> it's not actually a memory mapped device but it's the best idea we've
> got.

Ugh, I hate that.  What's wrong with using a "virtual" device instead?

I can create a "virtual" bus for things like this if they really want a
"simple" bus, abusing platform for this is the major reason I hate the
platform bus code...

thanks,

greg k-h

Re: [PATCH v6 2/2] staging: ion: create one device entry per heap

[Greg KH]

On Tue, Oct 31, 2017 at 07:11:53PM +, Mark Brown wrote:
> On Tue, Oct 31, 2017 at 12:03:35PM -0700, Laura Abbott wrote:
> 
> > I'm not a fan of the platform bus but I have mixed feelings about
> > creating a dedicated bus type. I guess if we really need a bus
> > type we can do it later?
> 
> There was a discussion a while ago in the context of I2C/SPI MFDs
> which concluded that if you need a bus and it's going to be effectively
> noop then you should just use the platform bus as anything else will
> consist almost entirely of cut'n'paste from the platform bus with some
> light sed usage and code duplication is bad.  It's not super lovely as
> it's not actually a memory mapped device but it's the best idea we've
> got.

Ugh, I hate that.  What's wrong with using a "virtual" device instead?

I can create a "virtual" bus for things like this if they really want a
"simple" bus, abusing platform for this is the major reason I hate the
platform bus code...

thanks,

greg k-h

Re: [PATCH v6 2/2] staging: ion: create one device entry per heap

[Laura Abbott]

On 10/31/2017 12:11 PM, Mark Brown wrote:
> On Tue, Oct 31, 2017 at 12:03:35PM -0700, Laura Abbott wrote:
> 
>> I'm not a fan of the platform bus but I have mixed feelings about
>> creating a dedicated bus type. I guess if we really need a bus
>> type we can do it later?
> 
> There was a discussion a while ago in the context of I2C/SPI MFDs
> which concluded that if you need a bus and it's going to be effectively
> noop then you should just use the platform bus as anything else will
> consist almost entirely of cut'n'paste from the platform bus with some
> light sed usage and code duplication is bad.  It's not super lovely as
> it's not actually a memory mapped device but it's the best idea we've
> got.
> 

Thanks for the pointer.

Re: [PATCH v6 2/2] staging: ion: create one device entry per heap

[Laura Abbott]

On 10/31/2017 12:11 PM, Mark Brown wrote:
> On Tue, Oct 31, 2017 at 12:03:35PM -0700, Laura Abbott wrote:
> 
>> I'm not a fan of the platform bus but I have mixed feelings about
>> creating a dedicated bus type. I guess if we really need a bus
>> type we can do it later?
> 
> There was a discussion a while ago in the context of I2C/SPI MFDs
> which concluded that if you need a bus and it's going to be effectively
> noop then you should just use the platform bus as anything else will
> consist almost entirely of cut'n'paste from the platform bus with some
> light sed usage and code duplication is bad.  It's not super lovely as
> it's not actually a memory mapped device but it's the best idea we've
> got.
> 

Thanks for the pointer.

Re: [PATCH v6 2/2] staging: ion: create one device entry per heap

[Mark Brown]

On Tue, Oct 31, 2017 at 12:03:35PM -0700, Laura Abbott wrote:

> I'm not a fan of the platform bus but I have mixed feelings about
> creating a dedicated bus type. I guess if we really need a bus
> type we can do it later?

There was a discussion a while ago in the context of I2C/SPI MFDs
which concluded that if you need a bus and it's going to be effectively
noop then you should just use the platform bus as anything else will
consist almost entirely of cut'n'paste from the platform bus with some
light sed usage and code duplication is bad.  It's not super lovely as
it's not actually a memory mapped device but it's the best idea we've
got.


signature.asc
Description: PGP signature

Re: [PATCH v6 2/2] staging: ion: create one device entry per heap

[Mark Brown]

On Tue, Oct 31, 2017 at 12:03:35PM -0700, Laura Abbott wrote:

> I'm not a fan of the platform bus but I have mixed feelings about
> creating a dedicated bus type. I guess if we really need a bus
> type we can do it later?

There was a discussion a while ago in the context of I2C/SPI MFDs
which concluded that if you need a bus and it's going to be effectively
noop then you should just use the platform bus as anything else will
consist almost entirely of cut'n'paste from the platform bus with some
light sed usage and code duplication is bad.  It's not super lovely as
it's not actually a memory mapped device but it's the best idea we've
got.


signature.asc
Description: PGP signature

Re: [PATCH v6 2/2] staging: ion: create one device entry per heap

[Laura Abbott]

On 10/23/2017 08:55 AM, Benjamin Gaignard wrote:
> Instead a getting only 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.
> 

I'm wondering if we should always keep /dev/ion for the query
ioctl and just disallow allocation from /dev/ion. I guess
running the query ioctl on /dev/ion0 always wouldn't be too
bad? Anyone else have strong opinions?

> Ion devices are parentless so it is need to add platform_bus as
> parent and platform_bus_type as bus to be put in /sys/device/paltform.
> Those two parameters need platform_device.h to be included but 
> include files weren't in alphabetic order so I reorder them correctly.
> 

I'm not a fan of the platform bus but I have mixed feelings about
creating a dedicated bus type. I guess if we really need a bus
type we can do it later?


Thanks,
Laura

> Signed-off-by: Benjamin Gaignard 
> ---
>  drivers/staging/android/TODO|  1 -
>  drivers/staging/android/ion/Kconfig |  7 +
>  drivers/staging/android/ion/ion-ioctl.c | 18 +--
>  drivers/staging/android/ion/ion.c   | 53 
> ++---
>  drivers/staging/android/ion/ion.h   | 15 --
>  5 files changed, 79 insertions(+), 15 deletions(-)
> 
> diff --git a/drivers/staging/android/TODO b/drivers/staging/android/TODO
> index 5f14247..d770ffa 100644
> --- a/drivers/staging/android/TODO
> +++ b/drivers/staging/android/TODO
> @@ -9,7 +9,6 @@ TODO:
>  ion/
>   - Add dt-bindings for remaining heaps (chunk and carveout heaps). This would
> involve putting appropriate bindings in a memory node for Ion to find.
> - - Split /dev/ion up into multiple nodes (e.g. /dev/ion/heap0)
>   - Better test framework (integration with VGEM was suggested)
>  
>  Please send patches to Greg Kroah-Hartman  and Cc:
> diff --git a/drivers/staging/android/ion/Kconfig 
> b/drivers/staging/android/ion/Kconfig
> index a517b2d..cb4666e 100644
> --- a/drivers/staging/android/ion/Kconfig
> +++ b/drivers/staging/android/ion/Kconfig
> @@ -10,6 +10,13 @@ menuconfig ION
> If you're not using Android its probably safe to
> say N here.
>  
> +config ION_LEGACY_DEVICE_API
> + bool "Keep using Ion legacy misc device API"
> + depends on ION
> + help
> +   Choose this option to keep using Ion legacy misc device API
> +   i.e. /dev/ion
> +
>  config ION_SYSTEM_HEAP
>   bool "Ion system heap"
>   depends on ION
> diff --git a/drivers/staging/android/ion/ion-ioctl.c 
> b/drivers/staging/android/ion/ion-ioctl.c
> index e26b786..bb5c77b 100644
> --- a/drivers/staging/android/ion/ion-ioctl.c
> +++ b/drivers/staging/android/ion/ion-ioctl.c
> @@ -25,7 +25,8 @@ 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)
>  {
>   switch (cmd) {
>   case ION_IOC_HEAP_QUERY:
> @@ -34,6 +35,19 @@ static int validate_ioctl_arg(unsigned int cmd, union 
> ion_ioctl_arg *arg)
>   arg->query.reserved2 )
>   return -EINVAL;
>   break;
> +
> + case ION_IOC_ALLOC:
> + {
> + int mask = 1 << iminor(filp->f_inode);
> +
> +#ifdef CONFIG_ION_LEGACY_DEVICE_API
> + if (imajor(filp->f_inode) == MISC_MAJOR)
> + return 0;
> +#endif
> + if (!(arg->allocation.heap_id_mask & mask))
> + return -EINVAL;
> + break;
> + }
>   default:
>   break;
>   }
> @@ -69,7 +83,7 @@ long ion_ioctl(struct file *filp, unsigned int cmd, 
> unsigned long arg)
>   if (copy_from_user(, (void __user *)arg, _IOC_SIZE(cmd)))
>   return -EFAULT;
>  
> - ret = validate_ioctl_arg(cmd, );
> + ret = validate_ioctl_arg(filp, cmd, );
>   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..dd66f55 100644
> --- a/drivers/staging/android/ion/ion.c
> +++ b/drivers/staging/android/ion/ion.c
> @@ -15,31 +15,35 @@
>   *
>   */
>  
> +#include 
> +#include 
>  #include 
> +#include 
>  #include 
> +#include 
>  #include 
>  #include 
>  #include 
> -#include 
> +#include 
>  #include 
>  #include 
>  #include 
>  #include 
> -#include 
>  #include 
>  #include 
> +#include 
>  #include 
> -#include 
> +#include 
>  #include 
> +#include 
>  #include 
>  #include 
> -#include 
> -#include 
> -#include 
> -#include 
>  
>  #include "ion.h"
>  
> +#define 

Re: [PATCH v6 2/2] staging: ion: create one device entry per heap

[Laura Abbott]

On 10/23/2017 08:55 AM, Benjamin Gaignard wrote:
> Instead a getting only 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.
> 

I'm wondering if we should always keep /dev/ion for the query
ioctl and just disallow allocation from /dev/ion. I guess
running the query ioctl on /dev/ion0 always wouldn't be too
bad? Anyone else have strong opinions?

> Ion devices are parentless so it is need to add platform_bus as
> parent and platform_bus_type as bus to be put in /sys/device/paltform.
> Those two parameters need platform_device.h to be included but 
> include files weren't in alphabetic order so I reorder them correctly.
> 

I'm not a fan of the platform bus but I have mixed feelings about
creating a dedicated bus type. I guess if we really need a bus
type we can do it later?


Thanks,
Laura

> Signed-off-by: Benjamin Gaignard 
> ---
>  drivers/staging/android/TODO|  1 -
>  drivers/staging/android/ion/Kconfig |  7 +
>  drivers/staging/android/ion/ion-ioctl.c | 18 +--
>  drivers/staging/android/ion/ion.c   | 53 
> ++---
>  drivers/staging/android/ion/ion.h   | 15 --
>  5 files changed, 79 insertions(+), 15 deletions(-)
> 
> diff --git a/drivers/staging/android/TODO b/drivers/staging/android/TODO
> index 5f14247..d770ffa 100644
> --- a/drivers/staging/android/TODO
> +++ b/drivers/staging/android/TODO
> @@ -9,7 +9,6 @@ TODO:
>  ion/
>   - Add dt-bindings for remaining heaps (chunk and carveout heaps). This would
> involve putting appropriate bindings in a memory node for Ion to find.
> - - Split /dev/ion up into multiple nodes (e.g. /dev/ion/heap0)
>   - Better test framework (integration with VGEM was suggested)
>  
>  Please send patches to Greg Kroah-Hartman  and Cc:
> diff --git a/drivers/staging/android/ion/Kconfig 
> b/drivers/staging/android/ion/Kconfig
> index a517b2d..cb4666e 100644
> --- a/drivers/staging/android/ion/Kconfig
> +++ b/drivers/staging/android/ion/Kconfig
> @@ -10,6 +10,13 @@ menuconfig ION
> If you're not using Android its probably safe to
> say N here.
>  
> +config ION_LEGACY_DEVICE_API
> + bool "Keep using Ion legacy misc device API"
> + depends on ION
> + help
> +   Choose this option to keep using Ion legacy misc device API
> +   i.e. /dev/ion
> +
>  config ION_SYSTEM_HEAP
>   bool "Ion system heap"
>   depends on ION
> diff --git a/drivers/staging/android/ion/ion-ioctl.c 
> b/drivers/staging/android/ion/ion-ioctl.c
> index e26b786..bb5c77b 100644
> --- a/drivers/staging/android/ion/ion-ioctl.c
> +++ b/drivers/staging/android/ion/ion-ioctl.c
> @@ -25,7 +25,8 @@ 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)
>  {
>   switch (cmd) {
>   case ION_IOC_HEAP_QUERY:
> @@ -34,6 +35,19 @@ static int validate_ioctl_arg(unsigned int cmd, union 
> ion_ioctl_arg *arg)
>   arg->query.reserved2 )
>   return -EINVAL;
>   break;
> +
> + case ION_IOC_ALLOC:
> + {
> + int mask = 1 << iminor(filp->f_inode);
> +
> +#ifdef CONFIG_ION_LEGACY_DEVICE_API
> + if (imajor(filp->f_inode) == MISC_MAJOR)
> + return 0;
> +#endif
> + if (!(arg->allocation.heap_id_mask & mask))
> + return -EINVAL;
> + break;
> + }
>   default:
>   break;
>   }
> @@ -69,7 +83,7 @@ long ion_ioctl(struct file *filp, unsigned int cmd, 
> unsigned long arg)
>   if (copy_from_user(, (void __user *)arg, _IOC_SIZE(cmd)))
>   return -EFAULT;
>  
> - ret = validate_ioctl_arg(cmd, );
> + ret = validate_ioctl_arg(filp, cmd, );
>   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..dd66f55 100644
> --- a/drivers/staging/android/ion/ion.c
> +++ b/drivers/staging/android/ion/ion.c
> @@ -15,31 +15,35 @@
>   *
>   */
>  
> +#include 
> +#include 
>  #include 
> +#include 
>  #include 
> +#include 
>  #include 
>  #include 
>  #include 
> -#include 
> +#include 
>  #include 
>  #include 
>  #include 
>  #include 
> -#include 
>  #include 
>  #include 
> +#include 
>  #include 
> -#include 
> +#include 
>  #include 
> +#include 
>  #include 
>  #include 
> -#include 
> -#include 
> -#include 
> -#include 
>  
>  #include "ion.h"
>  
> +#define ION_DEV_MAX 32
> +#define ION_NAME "ion"
> +
>  static 

Re: [PATCH v6 2/2] staging: ion: create one device entry per heap

[Jordan Crouse]

On Mon, Oct 23, 2017 at 05:55:37PM +0200, Benjamin Gaignard wrote:
> Instead a getting only 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.
> 
> Ion devices are parentless so it is need to add platform_bus as
> parent and platform_bus_type as bus to be put in /sys/device/paltform.

nit: paltform -> platform.

Jordan

-- 
The Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum,
a Linux Foundation Collaborative Project

Re: [PATCH v6 2/2] staging: ion: create one device entry per heap

[Jordan Crouse]

On Mon, Oct 23, 2017 at 05:55:37PM +0200, Benjamin Gaignard wrote:
> Instead a getting only 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.
> 
> Ion devices are parentless so it is need to add platform_bus as
> parent and platform_bus_type as bus to be put in /sys/device/paltform.

nit: paltform -> platform.

Jordan

-- 
The Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum,
a Linux Foundation Collaborative Project

[PATCH v6 2/2] staging: ion: create one device entry per heap

[Benjamin Gaignard]

Instead a getting only 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.

Ion devices are parentless so it is need to add platform_bus as
parent and platform_bus_type as bus to be put in /sys/device/paltform.
Those two parameters need platform_device.h to be included but 
include files weren't in alphabetic order so I reorder them correctly.

Signed-off-by: Benjamin Gaignard 
---
 drivers/staging/android/TODO|  1 -
 drivers/staging/android/ion/Kconfig |  7 +
 drivers/staging/android/ion/ion-ioctl.c | 18 +--
 drivers/staging/android/ion/ion.c   | 53 ++---
 drivers/staging/android/ion/ion.h   | 15 --
 5 files changed, 79 insertions(+), 15 deletions(-)

diff --git a/drivers/staging/android/TODO b/drivers/staging/android/TODO
index 5f14247..d770ffa 100644
--- a/drivers/staging/android/TODO
+++ b/drivers/staging/android/TODO
@@ -9,7 +9,6 @@ TODO:
 ion/
  - Add dt-bindings for remaining heaps (chunk and carveout heaps). This would
involve putting appropriate bindings in a memory node for Ion to find.
- - Split /dev/ion up into multiple nodes (e.g. /dev/ion/heap0)
  - Better test framework (integration with VGEM was suggested)
 
 Please send patches to Greg Kroah-Hartman  and Cc:
diff --git a/drivers/staging/android/ion/Kconfig 
b/drivers/staging/android/ion/Kconfig
index a517b2d..cb4666e 100644
--- a/drivers/staging/android/ion/Kconfig
+++ b/drivers/staging/android/ion/Kconfig
@@ -10,6 +10,13 @@ menuconfig ION
  If you're not using Android its probably safe to
  say N here.
 
+config ION_LEGACY_DEVICE_API
+   bool "Keep using Ion legacy misc device API"
+   depends on ION
+   help
+ Choose this option to keep using Ion legacy misc device API
+ i.e. /dev/ion
+
 config ION_SYSTEM_HEAP
bool "Ion system heap"
depends on ION
diff --git a/drivers/staging/android/ion/ion-ioctl.c 
b/drivers/staging/android/ion/ion-ioctl.c
index e26b786..bb5c77b 100644
--- a/drivers/staging/android/ion/ion-ioctl.c
+++ b/drivers/staging/android/ion/ion-ioctl.c
@@ -25,7 +25,8 @@ 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)
 {
switch (cmd) {
case ION_IOC_HEAP_QUERY:
@@ -34,6 +35,19 @@ static int validate_ioctl_arg(unsigned int cmd, union 
ion_ioctl_arg *arg)
arg->query.reserved2 )
return -EINVAL;
break;
+
+   case ION_IOC_ALLOC:
+   {
+   int mask = 1 << iminor(filp->f_inode);
+
+#ifdef CONFIG_ION_LEGACY_DEVICE_API
+   if (imajor(filp->f_inode) == MISC_MAJOR)
+   return 0;
+#endif
+   if (!(arg->allocation.heap_id_mask & mask))
+   return -EINVAL;
+   break;
+   }
default:
break;
}
@@ -69,7 +83,7 @@ long ion_ioctl(struct file *filp, unsigned int cmd, unsigned 
long arg)
if (copy_from_user(, (void __user *)arg, _IOC_SIZE(cmd)))
return -EFAULT;
 
-   ret = validate_ioctl_arg(cmd, );
+   ret = validate_ioctl_arg(filp, cmd, );
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..dd66f55 100644
--- a/drivers/staging/android/ion/ion.c
+++ b/drivers/staging/android/ion/ion.c
@@ -15,31 +15,35 @@
  *
  */
 
+#include 
+#include 
 #include 
+#include 
 #include 
+#include 
 #include 
 #include 
 #include 
-#include 
+#include 
 #include 
 #include 
 #include 
 #include 
-#include 
 #include 
 #include 
+#include 
 #include 
-#include 
+#include 
 #include 
+#include 
 #include 
 #include 
-#include 
-#include 
-#include 
-#include 
 
 #include "ion.h"
 
+#define ION_DEV_MAX 32
+#define ION_NAME "ion"
+
 static struct ion_device *internal_dev;
 static int heap_id;
 
@@ -537,15 +541,30 @@ static int debug_shrink_get(void *data, u64 *val)
 DEFINE_SIMPLE_ATTRIBUTE(debug_shrink_fops, debug_shrink_get,
debug_shrink_set, "%llu\n");
 
-void ion_device_add_heap(struct ion_heap *heap)
+int ion_device_add_heap(struct ion_heap *heap)
 {
struct dentry *debug_file;
struct ion_device *dev = internal_dev;
+   int ret = 0;
 
if (!heap->ops->allocate || !heap->ops->free)
pr_err("%s: can not add heap with invalid ops struct.\n",
   __func__);
 
+   if 

[PATCH v6 2/2] staging: ion: create one device entry per heap

[Benjamin Gaignard]

Instead a getting only 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.

Ion devices are parentless so it is need to add platform_bus as
parent and platform_bus_type as bus to be put in /sys/device/paltform.
Those two parameters need platform_device.h to be included but 
include files weren't in alphabetic order so I reorder them correctly.

Signed-off-by: Benjamin Gaignard 
---
 drivers/staging/android/TODO|  1 -
 drivers/staging/android/ion/Kconfig |  7 +
 drivers/staging/android/ion/ion-ioctl.c | 18 +--
 drivers/staging/android/ion/ion.c   | 53 ++---
 drivers/staging/android/ion/ion.h   | 15 --
 5 files changed, 79 insertions(+), 15 deletions(-)

diff --git a/drivers/staging/android/TODO b/drivers/staging/android/TODO
index 5f14247..d770ffa 100644
--- a/drivers/staging/android/TODO
+++ b/drivers/staging/android/TODO
@@ -9,7 +9,6 @@ TODO:
 ion/
  - Add dt-bindings for remaining heaps (chunk and carveout heaps). This would
involve putting appropriate bindings in a memory node for Ion to find.
- - Split /dev/ion up into multiple nodes (e.g. /dev/ion/heap0)
  - Better test framework (integration with VGEM was suggested)
 
 Please send patches to Greg Kroah-Hartman  and Cc:
diff --git a/drivers/staging/android/ion/Kconfig 
b/drivers/staging/android/ion/Kconfig
index a517b2d..cb4666e 100644
--- a/drivers/staging/android/ion/Kconfig
+++ b/drivers/staging/android/ion/Kconfig
@@ -10,6 +10,13 @@ menuconfig ION
  If you're not using Android its probably safe to
  say N here.
 
+config ION_LEGACY_DEVICE_API
+   bool "Keep using Ion legacy misc device API"
+   depends on ION
+   help
+ Choose this option to keep using Ion legacy misc device API
+ i.e. /dev/ion
+
 config ION_SYSTEM_HEAP
bool "Ion system heap"
depends on ION
diff --git a/drivers/staging/android/ion/ion-ioctl.c 
b/drivers/staging/android/ion/ion-ioctl.c
index e26b786..bb5c77b 100644
--- a/drivers/staging/android/ion/ion-ioctl.c
+++ b/drivers/staging/android/ion/ion-ioctl.c
@@ -25,7 +25,8 @@ 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)
 {
switch (cmd) {
case ION_IOC_HEAP_QUERY:
@@ -34,6 +35,19 @@ static int validate_ioctl_arg(unsigned int cmd, union 
ion_ioctl_arg *arg)
arg->query.reserved2 )
return -EINVAL;
break;
+
+   case ION_IOC_ALLOC:
+   {
+   int mask = 1 << iminor(filp->f_inode);
+
+#ifdef CONFIG_ION_LEGACY_DEVICE_API
+   if (imajor(filp->f_inode) == MISC_MAJOR)
+   return 0;
+#endif
+   if (!(arg->allocation.heap_id_mask & mask))
+   return -EINVAL;
+   break;
+   }
default:
break;
}
@@ -69,7 +83,7 @@ long ion_ioctl(struct file *filp, unsigned int cmd, unsigned 
long arg)
if (copy_from_user(, (void __user *)arg, _IOC_SIZE(cmd)))
return -EFAULT;
 
-   ret = validate_ioctl_arg(cmd, );
+   ret = validate_ioctl_arg(filp, cmd, );
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..dd66f55 100644
--- a/drivers/staging/android/ion/ion.c
+++ b/drivers/staging/android/ion/ion.c
@@ -15,31 +15,35 @@
  *
  */
 
+#include 
+#include 
 #include 
+#include 
 #include 
+#include 
 #include 
 #include 
 #include 
-#include 
+#include 
 #include 
 #include 
 #include 
 #include 
-#include 
 #include 
 #include 
+#include 
 #include 
-#include 
+#include 
 #include 
+#include 
 #include 
 #include 
-#include 
-#include 
-#include 
-#include 
 
 #include "ion.h"
 
+#define ION_DEV_MAX 32
+#define ION_NAME "ion"
+
 static struct ion_device *internal_dev;
 static int heap_id;
 
@@ -537,15 +541,30 @@ static int debug_shrink_get(void *data, u64 *val)
 DEFINE_SIMPLE_ATTRIBUTE(debug_shrink_fops, debug_shrink_get,
debug_shrink_set, "%llu\n");
 
-void ion_device_add_heap(struct ion_heap *heap)
+int ion_device_add_heap(struct ion_heap *heap)
 {
struct dentry *debug_file;
struct ion_device *dev = internal_dev;
+   int ret = 0;
 
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)
+