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

On Wed, Nov 29, 2017 at 03:00:56PM +0100, Benjamin Gaignard wrote:

2017-11-28 14:32 GMT+01:00 Greg KH [off-list ref]:

quoted

On Mon, Nov 06, 2017 at 04:59:45PM +0100, Benjamin Gaignard wrote:

quoted

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 <redacted>
---
 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@kroah.com> 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.

quoted

quoted

+     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.

quoted

quoted

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

quoted

quoted

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

quoted

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?

quoted

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
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel