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 <gregkh@xxxxxxxxxxxxxxxxxxx>:
> > 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 <benjamin.gaignard@xxxxxxxxxx>
> >> ---
> >>  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 <greg@xxxxxxxxx> 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(&ion_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 documentation for the new sysfs files and the new ioctl
> > 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?
> >
> 
> I do not add ioctl just creating a device node per heap while keeping alive
> the legacy mode on the misc device.
> Until now all the users of ion have the same access rights on all the heaps
> because they must use a common device misc to do the allocation.
> Creating on device node per heap will allow to customize the access rights
> per heap.

This is not explaining your sysfs representation at all.  I have no idea
what you are wanting to do here.

You are confusing struct device with a character device node here I
think.  They are two totally different things, yet are related in some
ways.

> 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?

thanks,

greg k-h