* Re: [PATCH] staging: android: binder: move to the "real" part of the kernel
2014-10-16 12:47 [PATCH] staging: android: binder: move to the "real" part of the kernel Greg Kroah-Hartman
@ 2014-10-16 14:18 ` Michael Kerrisk (man-pages)
2014-10-16 23:14 ` Greg Kroah-Hartman
2014-10-16 17:09 ` John Stultz
` (3 subsequent siblings)
4 siblings, 1 reply; 25+ messages in thread
From: Michael Kerrisk (man-pages) @ 2014-10-16 14:18 UTC (permalink / raw)
To: Greg Kroah-Hartman
Cc: lkml, devel, Linux API, Santosh Shilimkar, John Stultz,
Arve Hjønnevåg, Sumit Semwal, Rebecca Schultz Zavin,
Christoffer Dall, Anup Patel
On Thu, Oct 16, 2014 at 2:47 PM, Greg Kroah-Hartman
<gregkh@linuxfoundation.org> wrote:
> From: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
>
> The Android binder code has been "stable" for many years now. No matter
> what comes in the future, we are going to have to support this API, so
> might as well move it to the "real" part of the kernel as there's no
> real work that needs to be done to the existing code.
Where does one find the canonical documentation of the user-space API?
Thanks,
Michael
> Signed-off-by: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
> ---
>
> This was discussed in the Android miniconf at the Plumbers conference.
> If anyone has any objections to this, please let me know, otherwise I'm
> queueing this up for 3.19-rc1
>
>
> drivers/Kconfig | 2 ++
> drivers/Makefile | 1 +
> drivers/android/Kconfig | 37 ++++++++++++++++++++++
> drivers/android/Makefile | 3 ++
> drivers/{staging => }/android/binder.c | 0
> drivers/{staging => }/android/binder.h | 2 +-
> drivers/{staging => }/android/binder_trace.h | 0
> drivers/staging/android/Kconfig | 30 ------------------
> drivers/staging/android/Makefile | 1 -
> include/uapi/linux/Kbuild | 1 +
> include/uapi/linux/android/Kbuild | 2 ++
> .../uapi => include/uapi/linux/android}/binder.h | 0
> 12 files changed, 47 insertions(+), 32 deletions(-)
> create mode 100644 drivers/android/Kconfig
> create mode 100644 drivers/android/Makefile
> rename drivers/{staging => }/android/binder.c (100%)
> rename drivers/{staging => }/android/binder.h (95%)
> rename drivers/{staging => }/android/binder_trace.h (100%)
> create mode 100644 include/uapi/linux/android/Kbuild
> rename {drivers/staging/android/uapi => include/uapi/linux/android}/binder.h (100%)
>
> diff --git a/drivers/Kconfig b/drivers/Kconfig
> index 1a693d3f9d51..569ff7886dc3 100644
> --- a/drivers/Kconfig
> +++ b/drivers/Kconfig
> @@ -182,4 +182,6 @@ source "drivers/ras/Kconfig"
>
> source "drivers/thunderbolt/Kconfig"
>
> +source "drivers/android/Kconfig"
> +
> endmenu
> diff --git a/drivers/Makefile b/drivers/Makefile
> index ebee55537a05..60d19820a4d4 100644
> --- a/drivers/Makefile
> +++ b/drivers/Makefile
> @@ -161,3 +161,4 @@ obj-$(CONFIG_POWERCAP) += powercap/
> obj-$(CONFIG_MCB) += mcb/
> obj-$(CONFIG_RAS) += ras/
> obj-$(CONFIG_THUNDERBOLT) += thunderbolt/
> +obj-$(CONFIG_ANDROID) += android/
> diff --git a/drivers/android/Kconfig b/drivers/android/Kconfig
> new file mode 100644
> index 000000000000..bdfc6c6f4f5a
> --- /dev/null
> +++ b/drivers/android/Kconfig
> @@ -0,0 +1,37 @@
> +menu "Android"
> +
> +config ANDROID
> + bool "Android Drivers"
> + ---help---
> + Enable support for various drivers needed on the Android platform
> +
> +if ANDROID
> +
> +config ANDROID_BINDER_IPC
> + bool "Android Binder IPC Driver"
> + depends on MMU
> + default n
> + ---help---
> + Binder is used in Android for both communication between processes,
> + and remote method invocation.
> +
> + This means one Android process can call a method/routine in another
> + Android process, using Binder to identify, invoke and pass arguments
> + between said processes.
> +
> +config ANDROID_BINDER_IPC_32BIT
> + bool
> + depends on !64BIT && ANDROID_BINDER_IPC
> + default y
> + ---help---
> + The Binder API has been changed to support both 32 and 64bit
> + applications in a mixed environment.
> +
> + Enable this to support an old 32-bit Android user-space (v4.4 and
> + earlier).
> +
> + Note that enabling this will break newer Android user-space.
> +
> +endif # if ANDROID
> +
> +endmenu
> diff --git a/drivers/android/Makefile b/drivers/android/Makefile
> new file mode 100644
> index 000000000000..3b7e4b072c58
> --- /dev/null
> +++ b/drivers/android/Makefile
> @@ -0,0 +1,3 @@
> +ccflags-y += -I$(src) # needed for trace events
> +
> +obj-$(CONFIG_ANDROID_BINDER_IPC) += binder.o
> diff --git a/drivers/staging/android/binder.c b/drivers/android/binder.c
> similarity index 100%
> rename from drivers/staging/android/binder.c
> rename to drivers/android/binder.c
> diff --git a/drivers/staging/android/binder.h b/drivers/android/binder.h
> similarity index 95%
> rename from drivers/staging/android/binder.h
> rename to drivers/android/binder.h
> index eb0834656dfe..5dc6a66b0665 100644
> --- a/drivers/staging/android/binder.h
> +++ b/drivers/android/binder.h
> @@ -24,7 +24,7 @@
> #define BINDER_IPC_32BIT 1
> #endif
>
> -#include "uapi/binder.h"
> +#include <uapi/linux/android/binder.h>
>
> #endif /* _LINUX_BINDER_H */
>
> diff --git a/drivers/staging/android/binder_trace.h b/drivers/android/binder_trace.h
> similarity index 100%
> rename from drivers/staging/android/binder_trace.h
> rename to drivers/android/binder_trace.h
> diff --git a/drivers/staging/android/Kconfig b/drivers/staging/android/Kconfig
> index 7a0e28852965..7e012f37792b 100644
> --- a/drivers/staging/android/Kconfig
> +++ b/drivers/staging/android/Kconfig
> @@ -1,37 +1,7 @@
> menu "Android"
>
> -config ANDROID
> - bool "Android Drivers"
> - ---help---
> - Enable support for various drivers needed on the Android platform
> -
> if ANDROID
>
> -config ANDROID_BINDER_IPC
> - bool "Android Binder IPC Driver"
> - depends on MMU
> - default n
> - ---help---
> - Binder is used in Android for both communication between processes,
> - and remote method invocation.
> -
> - This means one Android process can call a method/routine in another
> - Android process, using Binder to identify, invoke and pass arguments
> - between said processes.
> -
> -config ANDROID_BINDER_IPC_32BIT
> - bool
> - depends on !64BIT && ANDROID_BINDER_IPC
> - default y
> - ---help---
> - The Binder API has been changed to support both 32 and 64bit
> - applications in a mixed environment.
> -
> - Enable this to support an old 32-bit Android user-space (v4.4 and
> - earlier).
> -
> - Note that enabling this will break newer Android user-space.
> -
> config ASHMEM
> bool "Enable the Anonymous Shared Memory Subsystem"
> default n
> diff --git a/drivers/staging/android/Makefile b/drivers/staging/android/Makefile
> index 517ad5ffa429..479b2b86f8c8 100644
> --- a/drivers/staging/android/Makefile
> +++ b/drivers/staging/android/Makefile
> @@ -2,7 +2,6 @@ ccflags-y += -I$(src) # needed for trace events
>
> obj-y += ion/
>
> -obj-$(CONFIG_ANDROID_BINDER_IPC) += binder.o
> obj-$(CONFIG_ASHMEM) += ashmem.o
> obj-$(CONFIG_ANDROID_LOGGER) += logger.o
> obj-$(CONFIG_ANDROID_TIMED_OUTPUT) += timed_output.o
> diff --git a/include/uapi/linux/Kbuild b/include/uapi/linux/Kbuild
> index 70e150ebc6c9..1bbaf6457861 100644
> --- a/include/uapi/linux/Kbuild
> +++ b/include/uapi/linux/Kbuild
> @@ -1,4 +1,5 @@
> # UAPI Header export list
> +header-y += android/
> header-y += byteorder/
> header-y += can/
> header-y += caif/
> diff --git a/include/uapi/linux/android/Kbuild b/include/uapi/linux/android/Kbuild
> new file mode 100644
> index 000000000000..ca011eec252a
> --- /dev/null
> +++ b/include/uapi/linux/android/Kbuild
> @@ -0,0 +1,2 @@
> +# UAPI Header export list
> +header-y += binder.h
> diff --git a/drivers/staging/android/uapi/binder.h b/include/uapi/linux/android/binder.h
> similarity index 100%
> rename from drivers/staging/android/uapi/binder.h
> rename to include/uapi/linux/android/binder.h
> --
> 2.1.2
>
> --
> To unsubscribe from this list: send the line "unsubscribe linux-api" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at http://vger.kernel.org/majordomo-info.html
--
Michael Kerrisk
Linux man-pages maintainer; http://www.kernel.org/doc/man-pages/
Linux/UNIX System Programming Training: http://man7.org/training/
^ permalink raw reply [flat|nested] 25+ messages in thread* Re: [PATCH] staging: android: binder: move to the "real" part of the kernel
2014-10-16 14:18 ` Michael Kerrisk (man-pages)
@ 2014-10-16 23:14 ` Greg Kroah-Hartman
2014-10-20 12:45 ` Dan Carpenter
2014-10-21 10:01 ` Pavel Machek
0 siblings, 2 replies; 25+ messages in thread
From: Greg Kroah-Hartman @ 2014-10-16 23:14 UTC (permalink / raw)
To: Michael Kerrisk (man-pages)
Cc: lkml, devel, Linux API, Santosh Shilimkar, John Stultz,
Arve Hjønnevåg, Sumit Semwal, Rebecca Schultz Zavin,
Christoffer Dall, Anup Patel
On Thu, Oct 16, 2014 at 04:18:02PM +0200, Michael Kerrisk (man-pages) wrote:
> On Thu, Oct 16, 2014 at 2:47 PM, Greg Kroah-Hartman
> <gregkh@linuxfoundation.org> wrote:
> > From: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
> >
> > The Android binder code has been "stable" for many years now. No matter
> > what comes in the future, we are going to have to support this API, so
> > might as well move it to the "real" part of the kernel as there's no
> > real work that needs to be done to the existing code.
>
> Where does one find the canonical documentation of the user-space API?
There really is only one "canonical" thing, and that is in the libbinder
code in the Android userspace repository. And it's not really
"documentation" so much as, "a C file that interacts with the ioctls in
the binder kernel code" :(
Think of this as just a random character driver with some funny ioctls
that will never get really documented as there is only one user of it.
sorry,
greg k-h
^ permalink raw reply [flat|nested] 25+ messages in thread
* Re: [PATCH] staging: android: binder: move to the "real" part of the kernel
2014-10-16 23:14 ` Greg Kroah-Hartman
@ 2014-10-20 12:45 ` Dan Carpenter
2014-10-21 10:01 ` Pavel Machek
1 sibling, 0 replies; 25+ messages in thread
From: Dan Carpenter @ 2014-10-20 12:45 UTC (permalink / raw)
To: Greg Kroah-Hartman
Cc: Michael Kerrisk (man-pages),
devel, Anup Patel, Linux API, lkml, Arve Hjønnevåg,
Santosh Shilimkar, Rebecca Schultz Zavin, John Stultz,
Sumit Semwal, Christoffer Dall
Having documentation would help reviewers.
Here is some documentation I found online.
http://www.phonesdevelopers.com/1793504/
But it seems like it was originally written in another language and auto
translated to English.
I don't see who is going to maintain this, if no one has time to even
document how it's supposed to work...
regards,
dan carpenter
^ permalink raw reply [flat|nested] 25+ messages in thread
* Re: [PATCH] staging: android: binder: move to the "real" part of the kernel
2014-10-16 23:14 ` Greg Kroah-Hartman
2014-10-20 12:45 ` Dan Carpenter
@ 2014-10-21 10:01 ` Pavel Machek
1 sibling, 0 replies; 25+ messages in thread
From: Pavel Machek @ 2014-10-21 10:01 UTC (permalink / raw)
To: Greg Kroah-Hartman
Cc: Michael Kerrisk (man-pages),
lkml, devel, Linux API, Santosh Shilimkar, John Stultz,
Arve Hjønnevåg, Sumit Semwal, Rebecca Schultz Zavin,
Christoffer Dall, Anup Patel
On Fri 2014-10-17 01:14:57, Greg Kroah-Hartman wrote:
> On Thu, Oct 16, 2014 at 04:18:02PM +0200, Michael Kerrisk (man-pages) wrote:
> > On Thu, Oct 16, 2014 at 2:47 PM, Greg Kroah-Hartman
> > <gregkh@linuxfoundation.org> wrote:
> > > From: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
> > >
> > > The Android binder code has been "stable" for many years now. No matter
> > > what comes in the future, we are going to have to support this API, so
> > > might as well move it to the "real" part of the kernel as there's no
> > > real work that needs to be done to the existing code.
> >
> > Where does one find the canonical documentation of the user-space API?
>
> There really is only one "canonical" thing, and that is in the libbinder
> code in the Android userspace repository. And it's not really
> "documentation" so much as, "a C file that interacts with the ioctls in
> the binder kernel code" :(
>
> Think of this as just a random character driver with some funny ioctls
> that will never get really documented as there is only one user of it.
This is not random character driver, it is communication mechanism. It
should _not_ be a character driver.
And it should really be documented.
Pavel
--
(english) http://www.livejournal.com/~pavelmachek
(cesky, pictures) http://atrey.karlin.mff.cuni.cz/~pavel/picture/horses/blog.html
^ permalink raw reply [flat|nested] 25+ messages in thread
* Re: [PATCH] staging: android: binder: move to the "real" part of the kernel
2014-10-16 12:47 [PATCH] staging: android: binder: move to the "real" part of the kernel Greg Kroah-Hartman
2014-10-16 14:18 ` Michael Kerrisk (man-pages)
@ 2014-10-16 17:09 ` John Stultz
2014-10-16 23:12 ` Greg Kroah-Hartman
2014-10-17 9:26 ` Dan Carpenter
` (2 subsequent siblings)
4 siblings, 1 reply; 25+ messages in thread
From: John Stultz @ 2014-10-16 17:09 UTC (permalink / raw)
To: Greg Kroah-Hartman
Cc: lkml, devel, Linux API, Santosh Shilimkar,
Arve Hjønnevåg, Sumit Semwal, Rebecca Schultz Zavin,
Christoffer Dall, Anup Patel
On Thu, Oct 16, 2014 at 5:47 AM, Greg Kroah-Hartman
<gregkh@linuxfoundation.org> wrote:
> From: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
>
> The Android binder code has been "stable" for many years now. No matter
Well, ignoring the ABI break that landed in the last year. :)
> what comes in the future, we are going to have to support this API, so
> might as well move it to the "real" part of the kernel as there's no
> real work that needs to be done to the existing code.
>
> Signed-off-by: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
> ---
>
> This was discussed in the Android miniconf at the Plumbers conference.
> If anyone has any objections to this, please let me know, otherwise I'm
> queueing this up for 3.19-rc1
So my main concerns/thoughts here are:
Who is going to maintain this? Has Arve agreed? Are the Android guys
comfortable with the ABI stability rules they'll now face? Currently
in the android space no one but libbinder should use the kernel
interface. Can we enforce that no one use this interface out-side of
android (To ensure its one of those "if the abi breaks and no one
notices..." edge cases)?
I'm still hopeful that a libbinder over kdbus will eventually pan out.
I still see having two core IPC mechanisms (even if the use cases are
different in style) as a negative, and I worry this might reduce
motivation to unify things at the lower level. Granted, such work can
still continue, but the incentives do change.
thanks
-john
^ permalink raw reply [flat|nested] 25+ messages in thread* Re: [PATCH] staging: android: binder: move to the "real" part of the kernel
2014-10-16 17:09 ` John Stultz
@ 2014-10-16 23:12 ` Greg Kroah-Hartman
2014-10-17 3:25 ` John Stultz
` (2 more replies)
0 siblings, 3 replies; 25+ messages in thread
From: Greg Kroah-Hartman @ 2014-10-16 23:12 UTC (permalink / raw)
To: John Stultz
Cc: lkml, devel, Linux API, Santosh Shilimkar,
Arve Hjønnevåg, Sumit Semwal, Rebecca Schultz Zavin,
Christoffer Dall, Anup Patel
On Thu, Oct 16, 2014 at 10:09:04AM -0700, John Stultz wrote:
> On Thu, Oct 16, 2014 at 5:47 AM, Greg Kroah-Hartman
> <gregkh@linuxfoundation.org> wrote:
> > From: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
> >
> > The Android binder code has been "stable" for many years now. No matter
>
> Well, ignoring the ABI break that landed in the last year. :)
As no one noticed, it wasn't a "break" :)
> > This was discussed in the Android miniconf at the Plumbers conference.
> > If anyone has any objections to this, please let me know, otherwise I'm
> > queueing this up for 3.19-rc1
>
> So my main concerns/thoughts here are:
>
> Who is going to maintain this? Has Arve agreed?
Do we really need someone to do more work that has been done on it in
the past as an official "maintainer"? I'll be glad to do it, as I doubt
it will require any time at all.
> Are the Android guys comfortable with the ABI stability rules they'll
> now face?
Just because something is in staging, doesn't mean you don't have to
follow the same ABI stability rules as the rest of the kernel. If a
change had happened to this code that broke userspace in the past, I
would have reverted it. So this should not be anything different from
what has been happening inthe past.
And the Android developers said they were happy to see this code move
into the "real" part of the kernel.
> Currently in the android space no one but libbinder should use the
> kernel interface.
That is correct. If you do that, you deserve all of the pain and
suffering and rooted machines you will get.
> Can we enforce that no one use this interface out-side of android (To
> ensure its one of those "if the abi breaks and no one notices..." edge
> cases)?
This is the kernel, we can not dictate "use", that's the good part of
the GPLv2 :)
Again, as no one has done this before, I strongly doubt it will happen
in the future.
> I'm still hopeful that a libbinder over kdbus will eventually pan out.
> I still see having two core IPC mechanisms (even if the use cases are
> different in style) as a negative, and I worry this might reduce
> motivation to unify things at the lower level. Granted, such work can
> still continue, but the incentives do change.
Yes, things are going to change in the future, there is work happening
here, and there was a presentation at Plumbers about what is going to be
coming.
But all of the changes will be in new code. Be it kdbus, or something
else if that doesn't work out. This existing binder.c file will not be
changing at all. This existing ABI, and codebase, is something that we
have to maintain forever for those millions of devices out there in the
real world today. So as there really is nothing left to do on it, it
deserves to be in the main part of the kernel source tree.
thanks,
greg k-h
^ permalink raw reply [flat|nested] 25+ messages in thread* Re: [PATCH] staging: android: binder: move to the "real" part of the kernel
2014-10-16 23:12 ` Greg Kroah-Hartman
@ 2014-10-17 3:25 ` John Stultz
2014-10-17 8:01 ` Greg Kroah-Hartman
2014-10-18 21:36 ` One Thousand Gnomes
2014-10-21 10:36 ` Pavel Machek
2 siblings, 1 reply; 25+ messages in thread
From: John Stultz @ 2014-10-17 3:25 UTC (permalink / raw)
To: Greg Kroah-Hartman
Cc: lkml, devel, Linux API, Santosh Shilimkar,
Arve Hjønnevåg, Sumit Semwal, Rebecca Schultz Zavin,
Christoffer Dall, Anup Patel
On Thu, Oct 16, 2014 at 4:12 PM, Greg Kroah-Hartman
<gregkh@linuxfoundation.org> wrote:
> On Thu, Oct 16, 2014 at 10:09:04AM -0700, John Stultz wrote:
>> On Thu, Oct 16, 2014 at 5:47 AM, Greg Kroah-Hartman
>> <gregkh@linuxfoundation.org> wrote:
>> > From: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
>> >
>> > The Android binder code has been "stable" for many years now. No matter
>>
>> Well, ignoring the ABI break that landed in the last year. :)
>
> As no one noticed, it wasn't a "break" :)
>
>> > This was discussed in the Android miniconf at the Plumbers conference.
>> > If anyone has any objections to this, please let me know, otherwise I'm
>> > queueing this up for 3.19-rc1
>>
>> So my main concerns/thoughts here are:
>>
>> Who is going to maintain this? Has Arve agreed?
>
> Do we really need someone to do more work that has been done on it in
> the past as an official "maintainer"? I'll be glad to do it, as I doubt
> it will require any time at all.
Ok. The only caution I have if Arve isn't involved is that we have in
the past merged cleanup changes that introduced bugs, and it wasn't
until Arve took at look at the new kernel that these were sorted out
(see e194fd8a5d8e0a7eeed239a8534460724b62fe2d). So I think if at all
possible getting his ack on things would be good.
>> Are the Android guys comfortable with the ABI stability rules they'll
>> now face?
>
> Just because something is in staging, doesn't mean you don't have to
> follow the same ABI stability rules as the rest of the kernel. If a
> change had happened to this code that broke userspace in the past, I
> would have reverted it. So this should not be anything different from
> what has been happening inthe past.
>
> And the Android developers said they were happy to see this code move
> into the "real" part of the kernel.
Alright then.
>> Currently in the android space no one but libbinder should use the
>> kernel interface.
>
> That is correct. If you do that, you deserve all of the pain and
> suffering and rooted machines you will get.
You reference this issue quite a bit, and I have a handwavy sense of
the problem, but do you have a more detailed link to the specific
issue here?
>> Can we enforce that no one use this interface out-side of android (To
>> ensure its one of those "if the abi breaks and no one notices..." edge
>> cases)?
>
> This is the kernel, we can not dictate "use", that's the good part of
> the GPLv2 :)
>
> Again, as no one has done this before, I strongly doubt it will happen
> in the future.
Well, the longer something is in the kernel, the more likely someone
will depend on it.
>> I'm still hopeful that a libbinder over kdbus will eventually pan out.
>> I still see having two core IPC mechanisms (even if the use cases are
>> different in style) as a negative, and I worry this might reduce
>> motivation to unify things at the lower level. Granted, such work can
>> still continue, but the incentives do change.
>
> Yes, things are going to change in the future, there is work happening
> here, and there was a presentation at Plumbers about what is going to be
> coming.
>
> But all of the changes will be in new code. Be it kdbus, or something
> else if that doesn't work out. This existing binder.c file will not be
> changing at all. This existing ABI, and codebase, is something that we
> have to maintain forever for those millions of devices out there in the
> real world today. So as there really is nothing left to do on it, it
> deserves to be in the main part of the kernel source tree.
Well, realistically, those millions of devices out there will almost
*never* get a kernel version bump
But yes, I'm fine with it going in. I'm just a bit surprised at how
quickly thoughts change here.
This does raise the question if the same standard could be held to
ashmem then? That's a *much* simpler and easier to maintain chunk of
code, and is just as isolated, logic wise. And while I think it would
be ideal if the unpinning feature could be done more generically, if
volatile ranges really doesn't have a future, then its maybe silly to
hold ashmem out as well?
thanks
-john
^ permalink raw reply [flat|nested] 25+ messages in thread
* Re: [PATCH] staging: android: binder: move to the "real" part of the kernel
2014-10-17 3:25 ` John Stultz
@ 2014-10-17 8:01 ` Greg Kroah-Hartman
0 siblings, 0 replies; 25+ messages in thread
From: Greg Kroah-Hartman @ 2014-10-17 8:01 UTC (permalink / raw)
To: John Stultz
Cc: lkml, devel, Linux API, Santosh Shilimkar,
Arve Hjønnevåg, Sumit Semwal, Rebecca Schultz Zavin,
Christoffer Dall, Anup Patel
On Thu, Oct 16, 2014 at 08:25:33PM -0700, John Stultz wrote:
> On Thu, Oct 16, 2014 at 4:12 PM, Greg Kroah-Hartman
> <gregkh@linuxfoundation.org> wrote:
> > On Thu, Oct 16, 2014 at 10:09:04AM -0700, John Stultz wrote:
> >> On Thu, Oct 16, 2014 at 5:47 AM, Greg Kroah-Hartman
> >> <gregkh@linuxfoundation.org> wrote:
> >> > From: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
> >> >
> >> > The Android binder code has been "stable" for many years now. No matter
> >>
> >> Well, ignoring the ABI break that landed in the last year. :)
> >
> > As no one noticed, it wasn't a "break" :)
> >
> >> > This was discussed in the Android miniconf at the Plumbers conference.
> >> > If anyone has any objections to this, please let me know, otherwise I'm
> >> > queueing this up for 3.19-rc1
> >>
> >> So my main concerns/thoughts here are:
> >>
> >> Who is going to maintain this? Has Arve agreed?
> >
> > Do we really need someone to do more work that has been done on it in
> > the past as an official "maintainer"? I'll be glad to do it, as I doubt
> > it will require any time at all.
>
> Ok. The only caution I have if Arve isn't involved is that we have in
> the past merged cleanup changes that introduced bugs, and it wasn't
> until Arve took at look at the new kernel that these were sorted out
> (see e194fd8a5d8e0a7eeed239a8534460724b62fe2d). So I think if at all
> possible getting his ack on things would be good.
I'll make sure to get his Ack on anything that could possibly cause a
problem.
> But yes, I'm fine with it going in. I'm just a bit surprised at how
> quickly thoughts change here.
Over the past year I've realized that code in staging needs to progress
either out of the kernel due to no need/use, or into the main part of
the kernel as people rely on it. The android code is something that
people rely on, and this code is "stable", so it should be merged into
the main kernel tree.
So yes, this does seem to be a change in the public stance, but it's
something that I have been talking about with people at the zillion
conferences I go to around the world.
> This does raise the question if the same standard could be held to
> ashmem then?
Ah, you beat me to it, I was going to wait to talk to you in person
about this :)
> That's a *much* simpler and easier to maintain chunk of
> code, and is just as isolated, logic wise. And while I think it would
> be ideal if the unpinning feature could be done more generically, if
> volatile ranges really doesn't have a future, then its maybe silly to
> hold ashmem out as well?
Yes, that was the second thing I was going to move, I'll send a patch
for that later today.
thanks,
greg k-h
^ permalink raw reply [flat|nested] 25+ messages in thread
* Re: [PATCH] staging: android: binder: move to the "real" part of the kernel
2014-10-16 23:12 ` Greg Kroah-Hartman
2014-10-17 3:25 ` John Stultz
@ 2014-10-18 21:36 ` One Thousand Gnomes
2014-10-19 22:01 ` Greg Kroah-Hartman
2014-10-21 10:36 ` Pavel Machek
2 siblings, 1 reply; 25+ messages in thread
From: One Thousand Gnomes @ 2014-10-18 21:36 UTC (permalink / raw)
To: Greg Kroah-Hartman
Cc: John Stultz, lkml, devel, Linux API, Santosh Shilimkar,
Arve Hjønnevåg, Sumit Semwal, Rebecca Schultz Zavin,
Christoffer Dall, Anup Patel
> Do we really need someone to do more work that has been done on it in
> the past as an official "maintainer"? I'll be glad to do it, as I doubt
> it will require any time at all.
Well every time in the past that Al Viro looked in its direction he broke
it so probably. Someone is going to have to clean up or fix the fact it
pokes around in the depths of the low level fd I/O code and calls stuff
like __fd_install and __alloc_fd directly, or mend it if it breaks.
I'm curious what Al Viro thinks of it
> > Currently in the android space no one but libbinder should use the
> > kernel interface.
>
> That is correct. If you do that, you deserve all of the pain and
> suffering and rooted machines you will get.
So what is the Android side model for its security. That probably also
should be described so nobody goes off and uses it for something like
systemd because "it looked neat".
> But all of the changes will be in new code. Be it kdbus, or something
> else if that doesn't work out. This existing binder.c file will not be
> changing at all. This existing ABI, and codebase, is something that we
> have to maintain forever for those millions of devices out there in the
> real world today.
95% of those devices are locked down, most of them have non replaceable
batteries that will dead and irreplacable (sanely anyway) in 3-5 years.
"Forever" in the phone world is mercifully rather short.
Alan
^ permalink raw reply [flat|nested] 25+ messages in thread
* Re: [PATCH] staging: android: binder: move to the "real" part of the kernel
2014-10-18 21:36 ` One Thousand Gnomes
@ 2014-10-19 22:01 ` Greg Kroah-Hartman
0 siblings, 0 replies; 25+ messages in thread
From: Greg Kroah-Hartman @ 2014-10-19 22:01 UTC (permalink / raw)
To: One Thousand Gnomes
Cc: John Stultz, lkml, devel, Linux API, Santosh Shilimkar,
Arve Hjønnevåg, Sumit Semwal, Rebecca Schultz Zavin,
Christoffer Dall, Anup Patel
On Sat, Oct 18, 2014 at 10:36:30PM +0100, One Thousand Gnomes wrote:
> > Do we really need someone to do more work that has been done on it in
> > the past as an official "maintainer"? I'll be glad to do it, as I doubt
> > it will require any time at all.
>
> Well every time in the past that Al Viro looked in its direction he broke
> it so probably. Someone is going to have to clean up or fix the fact it
> pokes around in the depths of the low level fd I/O code and calls stuff
> like __fd_install and __alloc_fd directly, or mend it if it breaks.
As it is, it is ok, but bad things happen if you allow more than one
process to open the device node. In android systems, that doesn't
happen, so all should be acceptable.
> I'm curious what Al Viro thinks of it
His last comments were along the lines of "don't let anything open that
device node other than libbinder".
> > > Currently in the android space no one but libbinder should use the
> > > kernel interface.
> >
> > That is correct. If you do that, you deserve all of the pain and
> > suffering and rooted machines you will get.
>
> So what is the Android side model for its security. That probably also
> should be described so nobody goes off and uses it for something like
> systemd because "it looked neat".
The side model is "one owner that knows what they are doing as they have
root privileges". I don't know a way to codify that, and we all know no
one reads documentation...
> > But all of the changes will be in new code. Be it kdbus, or something
> > else if that doesn't work out. This existing binder.c file will not be
> > changing at all. This existing ABI, and codebase, is something that we
> > have to maintain forever for those millions of devices out there in the
> > real world today.
>
> 95% of those devices are locked down, most of them have non replaceable
> batteries that will dead and irreplacable (sanely anyway) in 3-5 years.
> "Forever" in the phone world is mercifully rather short.
I still see brand new devices with 2 year old Android userspace being
shipped today. With a total mis-mash of random kernel versions,
depending on what the SoC supported. If we can delete this in 2-5
years, I would be really happy.
thanks,
greg k-h
^ permalink raw reply [flat|nested] 25+ messages in thread
* Re: [PATCH] staging: android: binder: move to the "real" part of the kernel
2014-10-16 23:12 ` Greg Kroah-Hartman
2014-10-17 3:25 ` John Stultz
2014-10-18 21:36 ` One Thousand Gnomes
@ 2014-10-21 10:36 ` Pavel Machek
2014-10-21 14:12 ` Arnd Bergmann
2 siblings, 1 reply; 25+ messages in thread
From: Pavel Machek @ 2014-10-21 10:36 UTC (permalink / raw)
To: Greg Kroah-Hartman, viro
Cc: John Stultz, lkml, devel, Linux API, Santosh Shilimkar,
Arve Hjønnevåg, Sumit Semwal, Rebecca Schultz Zavin,
Christoffer Dall, Anup Patel
On Fri 2014-10-17 01:12:21, Greg Kroah-Hartman wrote:
> On Thu, Oct 16, 2014 at 10:09:04AM -0700, John Stultz wrote:
> > On Thu, Oct 16, 2014 at 5:47 AM, Greg Kroah-Hartman
> > <gregkh@linuxfoundation.org> wrote:
> > > From: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
> > Are the Android guys comfortable with the ABI stability rules they'll
> > now face?
>
> Just because something is in staging, doesn't mean you don't have to
> follow the same ABI stability rules as the rest of the kernel. If a
> change had happened to this code that broke userspace in the past, I
> would have reverted it. So this should not be anything different from
> what has been happening inthe past.
Actually, there's big difference.
If Al Viro changes core filesystem in a way that breaks
staging/binder, binder is broken, and if it can't be fixed... well it
can't be fixed.
If Al Viro changes core filesystem in a way that breaks
drivers/binder, Al's change is going to be reverted.
It is really hard to review without API documentation. Normally, API
documentation is required for stuff like this.
For example: does it add new files in /proc?
Given that it is stable, can we get rid of binder_debug() and
especially BINDER_DEBUG_ENTRY stuff?
Checkpatch warns about 98 too long lines. Some of them could be fixed
easily.
This looks scary:
trace_binder_transaction_fd(t, fp->handle,
target_fd);
binder_debug(BINDER_DEBUG_TRANSACTION,
" fd %d -> %d\n",
fp->handle, target_fd);
/* TODO: fput? */
fp->handle = target_fd;
} break;
Could binder_transcation() be split to smaller functions according to
CodingStyle? 17 goto targets at the end of function are not exactly
easy to read.
ginder_thread_read/write also needs splitting.
binder_ioctl_write_read: just use direct return, no need to goto out
if it just returns.
proc->user_buffer_offset = vma->vm_start - (uintptr_t)proc->buffer;
mutex_unlock(&binder_mmap_lock);
#ifdef CONFIG_CPU_CACHE_VIPT
if (cache_is_vipt_aliasing()) {
while (CACHE_COLOUR((vma->vm_start ^
(uint32_t)proc->buffer))) {
Should this be (uintptr_t)?
/*pr_info("binder_mmap: %d %lx-%lx maps %p\n",
Delete the code, don't comment it out. It is on more than one place.
static void print_binder_thread(struct seq_file *m,
struct binder_thread *thread,
int print_always)
{
struct binder_transaction *t;
struct binder_work *w;
size_t start_pos = m->count;
size_t header_pos;
seq_printf(m, " thread %d: l %02x\n", thread->pid,
thread->looper);
header_pos = m->count;
t = thread->transaction_stack;
while (t) {
if (t->from == thread) {
print_binder_transaction(m,
"
outgoing transaction", t);
t = t->from_parent;
Is anyone depending on the debugfs files? Can it be deleted?
Code indentation is "interesting" in binder_thread_read(). See the "}
break;" lines. {}s should not be needed...?
I don't think this code would get merged if it was submitted for
normal inclusion in kernel. I don't think it is good idea to push it
through the back door, without documenting what it does and without
patches even going to the lists.
Pavel
--
(english) http://www.livejournal.com/~pavelmachek
(cesky, pictures) http://atrey.karlin.mff.cuni.cz/~pavel/picture/horses/blog.html
^ permalink raw reply [flat|nested] 25+ messages in thread* Re: [PATCH] staging: android: binder: move to the "real" part of the kernel
2014-10-21 10:36 ` Pavel Machek
@ 2014-10-21 14:12 ` Arnd Bergmann
2014-10-21 20:05 ` Pavel Machek
0 siblings, 1 reply; 25+ messages in thread
From: Arnd Bergmann @ 2014-10-21 14:12 UTC (permalink / raw)
To: Pavel Machek
Cc: Greg Kroah-Hartman, viro, John Stultz, lkml, devel, Linux API,
Santosh Shilimkar, Arve Hjønnevåg, Sumit Semwal,
Rebecca Schultz Zavin, Christoffer Dall, Anup Patel
On Tuesday 21 October 2014 12:36:22 Pavel Machek wrote:
> On Fri 2014-10-17 01:12:21, Greg Kroah-Hartman wrote:
> > On Thu, Oct 16, 2014 at 10:09:04AM -0700, John Stultz wrote:
> > > On Thu, Oct 16, 2014 at 5:47 AM, Greg Kroah-Hartman
> > > <gregkh@linuxfoundation.org> wrote:
> > > > From: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
>
> > > Are the Android guys comfortable with the ABI stability rules they'll
> > > now face?
> >
> > Just because something is in staging, doesn't mean you don't have to
> > follow the same ABI stability rules as the rest of the kernel. If a
> > change had happened to this code that broke userspace in the past, I
> > would have reverted it. So this should not be anything different from
> > what has been happening inthe past.
>
> Actually, there's big difference.
>
> If Al Viro changes core filesystem in a way that breaks
> staging/binder, binder is broken, and if it can't be fixed... well it
> can't be fixed.
>
> If Al Viro changes core filesystem in a way that breaks
> drivers/binder, Al's change is going to be reverted.
One might have argued that we'd have to do that already, but the reasons
for doing that with binder in the main kernel are certainly stronger.
> It is really hard to review without API documentation. Normally, API
> documentation is required for stuff like this.
>
> For example: does it add new files in /proc?
>
> Given that it is stable, can we get rid of binder_debug() and
> especially BINDER_DEBUG_ENTRY stuff?
Good point. We require documentation for every single sysfs attribute
that gets added to a driver (some escape the review, but that doesn't
change the rule), so we should not make an exception for a new procfs
file here.
> Checkpatch warns about 98 too long lines. Some of them could be fixed
> easily.
I don't think this should be an argument.
> This looks scary:
>
> trace_binder_transaction_fd(t, fp->handle,
> target_fd);
> binder_debug(BINDER_DEBUG_TRANSACTION,
> " fd %d -> %d\n",
> fp->handle, target_fd);
> /* TODO: fput? */
> fp->handle = target_fd;
> } break;
>
> Could binder_transcation() be split to smaller functions according to
> CodingStyle? 17 goto targets at the end of function are not exactly
> easy to read.
>
> ginder_thread_read/write also needs splitting.
Yes, in principle, but this is still a detail that would mainly serve
to simplify review. The problem is more the lack of review and
documentation of the API.
> binder_ioctl_write_read: just use direct return, no need to goto out
> if it just returns.
details, and a lot of people actually like the style used there.
> proc->user_buffer_offset = vma->vm_start - (uintptr_t)proc->buffer;
> mutex_unlock(&binder_mmap_lock);
>
> #ifdef CONFIG_CPU_CACHE_VIPT
> if (cache_is_vipt_aliasing()) {
> while (CACHE_COLOUR((vma->vm_start ^
> (uint32_t)proc->buffer))) {
>
> Should this be (uintptr_t)?
It should probably call an architecture specific helper.
Arnd
^ permalink raw reply [flat|nested] 25+ messages in thread* Re: [PATCH] staging: android: binder: move to the "real" part of the kernel
2014-10-21 14:12 ` Arnd Bergmann
@ 2014-10-21 20:05 ` Pavel Machek
0 siblings, 0 replies; 25+ messages in thread
From: Pavel Machek @ 2014-10-21 20:05 UTC (permalink / raw)
To: Arnd Bergmann
Cc: Greg Kroah-Hartman, viro, John Stultz, lkml, devel, Linux API,
Santosh Shilimkar, Arve Hjønnevåg, Sumit Semwal,
Rebecca Schultz Zavin, Christoffer Dall, Anup Patel
On Tue 2014-10-21 16:12:24, Arnd Bergmann wrote:
> On Tuesday 21 October 2014 12:36:22 Pavel Machek wrote:
> > On Fri 2014-10-17 01:12:21, Greg Kroah-Hartman wrote:
> > > On Thu, Oct 16, 2014 at 10:09:04AM -0700, John Stultz wrote:
> > > > On Thu, Oct 16, 2014 at 5:47 AM, Greg Kroah-Hartman
> > > > <gregkh@linuxfoundation.org> wrote:
> > > > > From: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
> >
> > > > Are the Android guys comfortable with the ABI stability rules they'll
> > > > now face?
> > >
> > > Just because something is in staging, doesn't mean you don't have to
> > > follow the same ABI stability rules as the rest of the kernel. If a
> > > change had happened to this code that broke userspace in the past, I
> > > would have reverted it. So this should not be anything different from
> > > what has been happening inthe past.
> >
> > Actually, there's big difference.
> >
> > If Al Viro changes core filesystem in a way that breaks
> > staging/binder, binder is broken, and if it can't be fixed... well it
> > can't be fixed.
> >
> > If Al Viro changes core filesystem in a way that breaks
> > drivers/binder, Al's change is going to be reverted.
>
> One might have argued that we'd have to do that already, but the reasons
> for doing that with binder in the main kernel are certainly stronger.
>
> > It is really hard to review without API documentation. Normally, API
> > documentation is required for stuff like this.
> >
> > For example: does it add new files in /proc?
> >
> > Given that it is stable, can we get rid of binder_debug() and
> > especially BINDER_DEBUG_ENTRY stuff?
>
> Good point. We require documentation for every single sysfs attribute
> that gets added to a driver (some escape the review, but that doesn't
> change the rule), so we should not make an exception for a new procfs
> file here.
Actually, it looked like it is debugfs file. Code was messy enough
that I was not sure.
> > Could binder_transcation() be split to smaller functions according to
> > CodingStyle? 17 goto targets at the end of function are not exactly
> > easy to read.
> >
> > ginder_thread_read/write also needs splitting.
>
> Yes, in principle, but this is still a detail that would mainly serve
> to simplify review. The problem is more the lack of review and
> documentation of the API.
Yes, the problem is that code is impossible to review without API
documentation.
Pavel
--
(english) http://www.livejournal.com/~pavelmachek
(cesky, pictures) http://atrey.karlin.mff.cuni.cz/~pavel/picture/horses/blog.html
^ permalink raw reply [flat|nested] 25+ messages in thread
* Re: [PATCH] staging: android: binder: move to the "real" part of the kernel
2014-10-16 12:47 [PATCH] staging: android: binder: move to the "real" part of the kernel Greg Kroah-Hartman
2014-10-16 14:18 ` Michael Kerrisk (man-pages)
2014-10-16 17:09 ` John Stultz
@ 2014-10-17 9:26 ` Dan Carpenter
2014-10-19 22:05 ` Greg Kroah-Hartman
2014-10-17 9:43 ` Christoph Hellwig
2014-10-20 17:06 ` Arnd Bergmann
4 siblings, 1 reply; 25+ messages in thread
From: Dan Carpenter @ 2014-10-17 9:26 UTC (permalink / raw)
To: Greg Kroah-Hartman
Cc: linux-kernel, devel, linux-api, Anup Patel,
Arve Hjønnevåg, John Stultz, Rebecca Schultz Zavin,
Santosh Shilimkar, Sumit Semwal, Christoffer Dall
The code isn't very beautiful and there are lots of details wrong like
the error codes.
Al had some critical things to say about it but it looks like most of
those issues have been fixed.
regards,
dan carpenter
^ permalink raw reply [flat|nested] 25+ messages in thread
* Re: [PATCH] staging: android: binder: move to the "real" part of the kernel
2014-10-17 9:26 ` Dan Carpenter
@ 2014-10-19 22:05 ` Greg Kroah-Hartman
2014-10-20 9:20 ` Dan Carpenter
0 siblings, 1 reply; 25+ messages in thread
From: Greg Kroah-Hartman @ 2014-10-19 22:05 UTC (permalink / raw)
To: Dan Carpenter
Cc: linux-kernel, devel, linux-api, Anup Patel,
Arve Hjønnevåg, John Stultz, Rebecca Schultz Zavin,
Santosh Shilimkar, Sumit Semwal, Christoffer Dall
On Fri, Oct 17, 2014 at 12:26:01PM +0300, Dan Carpenter wrote:
> The code isn't very beautiful and there are lots of details wrong like
> the error codes.
Really, what is wrong with the existing error code usages?
> Al had some critical things to say about it but it looks like most of
> those issues have been fixed.
All of the ones that can be fixed, have, as far as I know.
We are stuck with some that we can't fix, which is ok as it is safe in
the Android user model.
thanks,
greg k-h
^ permalink raw reply [flat|nested] 25+ messages in thread
* Re: [PATCH] staging: android: binder: move to the "real" part of the kernel
2014-10-19 22:05 ` Greg Kroah-Hartman
@ 2014-10-20 9:20 ` Dan Carpenter
2014-10-20 23:32 ` Arve Hjønnevåg
0 siblings, 1 reply; 25+ messages in thread
From: Dan Carpenter @ 2014-10-20 9:20 UTC (permalink / raw)
To: Greg Kroah-Hartman
Cc: devel, Anup Patel, linux-api, linux-kernel,
Arve Hjønnevåg, John Stultz, Rebecca Schultz Zavin,
Santosh Shilimkar, Sumit Semwal, Christoffer Dall
On Mon, Oct 20, 2014 at 06:05:47AM +0800, Greg Kroah-Hartman wrote:
> On Fri, Oct 17, 2014 at 12:26:01PM +0300, Dan Carpenter wrote:
> > The code isn't very beautiful and there are lots of details wrong like
> > the error codes.
>
> Really, what is wrong with the existing error code usages?
>
I guess I was mostly looking at binder_ioctl(), the rest of the code
seems better.
I feel like we went directly from "This code is never going in so there
is no need to look at it." to "This code is going in in one week so
there is no time to look at it."
How often do people rely on proc_no_lock=1 to make this work? People
are saying on the internet that you don't need acurate information so
you should disable locking as a speed up.
http://www.programdevelop.com/1821706/. It seems like a bad idea to
provide provide the "run fast and crash" option.
Why is binder_set_nice needed? Some comments would help here.
434 static void binder_set_nice(long nice)
435 {
436 long min_nice;
437
438 if (can_nice(current, nice)) {
439 set_user_nice(current, nice);
440 return;
441 }
442 min_nice = rlimit_to_nice(current->signal->rlim[RLIMIT_NICE].rlim_cur);
443 binder_debug(BINDER_DEBUG_PRIORITY_CAP,
444 "%d: nice value %ld not allowed use %ld instead\n",
445 current->pid, nice, min_nice);
446 set_user_nice(current, min_nice);
447 if (min_nice <= MAX_NICE)
448 return;
It's harmless but wierd to check if min_nice is valid after we already
called set_user_nice().
449 binder_user_error("%d RLIMIT_NICE not set\n", current->pid);
450 }
Random comment:
709 has_page_addr =
710 (void *)(((uintptr_t)buffer->data + buffer_size) & PAGE_MASK);
The casting on "buffer->data" ugly and inconsistent. There should be
a function:
void *buffer_data(struct binder_buffer *buffer)
{
return buffer.data;
}
That way this becomes:
has_page_addr = (buffer_data(buffer) + buffer_size) & PAGE_MASK);
The "has_page_addr" variable name is misleading, because we're not
storing true/false. We're storing the last page address.
711 if (n == NULL) {
712 if (size + sizeof(struct binder_buffer) + 4 >= buffer_size)
713 buffer_size = size; /* no room for other buffers */
714 else
715 buffer_size = size + sizeof(struct binder_buffer);
716 }
717 end_page_addr =
718 (void *)PAGE_ALIGN((uintptr_t)buffer->data + buffer_size);
719 if (end_page_addr > has_page_addr)
720 end_page_addr = has_page_addr;
721 if (binder_update_page_range(proc, 1,
722 (void *)PAGE_ALIGN((uintptr_t)buffer->data), end_page_addr, NULL))
723 return NULL;
The alignment here is confusing because we don't align buffer->data
below.
724
725 rb_erase(best_fit, &proc->free_buffers);
726 buffer->free = 0;
727 binder_insert_allocated_buffer(proc, buffer);
728 if (buffer_size != size) {
729 struct binder_buffer *new_buffer = (void *)buffer->data + size;
^^^^^^^^^^^^^^^^^^^^
I don't really understand when buffer->data has to be page aligned and
when not. This code has very few comments.
730
731 list_add(&new_buffer->entry, &buffer->entry);
732 new_buffer->free = 1;
733 binder_insert_free_buffer(proc, new_buffer);
734 }
Does the stop_on_user_error option work? There should be some
documentation for this stuff.
regards,
dan carpenter
^ permalink raw reply [flat|nested] 25+ messages in thread* Re: [PATCH] staging: android: binder: move to the "real" part of the kernel
2014-10-20 9:20 ` Dan Carpenter
@ 2014-10-20 23:32 ` Arve Hjønnevåg
2014-10-22 3:10 ` Rom Lemarchand
0 siblings, 1 reply; 25+ messages in thread
From: Arve Hjønnevåg @ 2014-10-20 23:32 UTC (permalink / raw)
To: Dan Carpenter
Cc: Greg Kroah-Hartman, devel, Anup Patel, linux-api, LKML,
John Stultz, Rebecca Schultz Zavin, Santosh Shilimkar,
Sumit Semwal, Christoffer Dall
On Mon, Oct 20, 2014 at 2:20 AM, Dan Carpenter <dan.carpenter@oracle.com> wrote:
> On Mon, Oct 20, 2014 at 06:05:47AM +0800, Greg Kroah-Hartman wrote:
>> On Fri, Oct 17, 2014 at 12:26:01PM +0300, Dan Carpenter wrote:
>> > The code isn't very beautiful and there are lots of details wrong like
>> > the error codes.
>>
>> Really, what is wrong with the existing error code usages?
>>
>
> I guess I was mostly looking at binder_ioctl(), the rest of the code
> seems better.
>
> I feel like we went directly from "This code is never going in so there
> is no need to look at it." to "This code is going in in one week so
> there is no time to look at it."
>
> How often do people rely on proc_no_lock=1 to make this work? People
> are saying on the internet that you don't need acurate information so
> you should disable locking as a speed up.
> http://www.programdevelop.com/1821706/. It seems like a bad idea to
> provide provide the "run fast and crash" option.
That is not what this switch is for. It is only there to debug the
driver if it gets stuck with the lock held. I would not object to
adding a config option to remove this param by default.
>
> Why is binder_set_nice needed? Some comments would help here.
The driver has some support for priority inheritance.
>
> 434 static void binder_set_nice(long nice)
> 435 {
> 436 long min_nice;
> 437
> 438 if (can_nice(current, nice)) {
> 439 set_user_nice(current, nice);
> 440 return;
> 441 }
> 442 min_nice = rlimit_to_nice(current->signal->rlim[RLIMIT_NICE].rlim_cur);
> 443 binder_debug(BINDER_DEBUG_PRIORITY_CAP,
> 444 "%d: nice value %ld not allowed use %ld instead\n",
> 445 current->pid, nice, min_nice);
> 446 set_user_nice(current, min_nice);
> 447 if (min_nice <= MAX_NICE)
> 448 return;
>
> It's harmless but wierd to check if min_nice is valid after we already
> called set_user_nice().
I don't remember why I did this, but I agree it is weird.
>
> 449 binder_user_error("%d RLIMIT_NICE not set\n", current->pid);
> 450 }
>
> Random comment:
>
> 709 has_page_addr =
> 710 (void *)(((uintptr_t)buffer->data + buffer_size) & PAGE_MASK);
>
> The casting on "buffer->data" ugly and inconsistent. There should be
> a function:
Other code in the kernel seems to do this the same way (although most
cast to unsigned long instead of uintptr_t). This code rounds down to
the start of of the page, and needs to cast to an integer type for
this. Adding a global kernel helper for this would be better than a
binder specific one. The existing PAGE_ALIGN function, which rounds
up, still needs casts to use with pointer types though.
> void *buffer_data(struct binder_buffer *buffer)
> {
> return buffer.data;
> }
>
> That way this becomes:
> has_page_addr = (buffer_data(buffer) + buffer_size) & PAGE_MASK);
I don't think this will compile.
>
> The "has_page_addr" variable name is misleading, because we're not
> storing true/false. We're storing the last page address.
It is the address of a page we already have mapped, not the last
address of the new allocation.
>
> 711 if (n == NULL) {
> 712 if (size + sizeof(struct binder_buffer) + 4 >= buffer_size)
> 713 buffer_size = size; /* no room for other buffers */
> 714 else
> 715 buffer_size = size + sizeof(struct binder_buffer);
> 716 }
> 717 end_page_addr =
> 718 (void *)PAGE_ALIGN((uintptr_t)buffer->data + buffer_size);
> 719 if (end_page_addr > has_page_addr)
> 720 end_page_addr = has_page_addr;
> 721 if (binder_update_page_range(proc, 1,
> 722 (void *)PAGE_ALIGN((uintptr_t)buffer->data), end_page_addr, NULL))
> 723 return NULL;
>
> The alignment here is confusing because we don't align buffer->data
> below.
binder_update_page_range allocates and maps pages. buffer->data will
point to a range within the allocated pages.
>
> 724
> 725 rb_erase(best_fit, &proc->free_buffers);
> 726 buffer->free = 0;
> 727 binder_insert_allocated_buffer(proc, buffer);
> 728 if (buffer_size != size) {
> 729 struct binder_buffer *new_buffer = (void *)buffer->data + size;
> ^^^^^^^^^^^^^^^^^^^^
> I don't really understand when buffer->data has to be page aligned and
> when not. This code has very few comments.
>
buffer->data never needs to be page aligned. The code above rounds to
page boundaries to allocate new pages as needed.
> 730
> 731 list_add(&new_buffer->entry, &buffer->entry);
> 732 new_buffer->free = 1;
> 733 binder_insert_free_buffer(proc, new_buffer);
> 734 }
>
> Does the stop_on_user_error option work? There should be some
> documentation for this stuff.
>
Yes.
> regards,
> dan carpenter
>
--
Arve Hjønnevåg
^ permalink raw reply [flat|nested] 25+ messages in thread* Re: [PATCH] staging: android: binder: move to the "real" part of the kernel
2014-10-20 23:32 ` Arve Hjønnevåg
@ 2014-10-22 3:10 ` Rom Lemarchand
2014-10-22 3:16 ` Joe Perches
2014-10-24 5:00 ` Dan Carpenter
0 siblings, 2 replies; 25+ messages in thread
From: Rom Lemarchand @ 2014-10-22 3:10 UTC (permalink / raw)
To: Arve Hjønnevåg
Cc: Dan Carpenter, Greg Kroah-Hartman, devel, Anup Patel, linux-api,
LKML, John Stultz, Rebecca Schultz Zavin, Santosh Shilimkar,
Sumit Semwal, Christoffer Dall
On the topic of maintainers for binder: both Arve Hjønnevåg
(arve@android.com) and Riley Andrews (riandrews@android.com) have
volunteered to be co-maintainers with Greg.
We would also like to make kernel-team@android.com the maintainer of
the whole android directory.
^ permalink raw reply [flat|nested] 25+ messages in thread
* Re: [PATCH] staging: android: binder: move to the "real" part of the kernel
2014-10-22 3:10 ` Rom Lemarchand
@ 2014-10-22 3:16 ` Joe Perches
2014-10-24 5:00 ` Dan Carpenter
1 sibling, 0 replies; 25+ messages in thread
From: Joe Perches @ 2014-10-22 3:16 UTC (permalink / raw)
To: Rom Lemarchand
Cc: Arve Hjønnevåg, Dan Carpenter, Greg Kroah-Hartman,
devel, Anup Patel, linux-api, LKML, John Stultz,
Rebecca Schultz Zavin, Santosh Shilimkar, Sumit Semwal,
Christoffer Dall
On Tue, 2014-10-21 at 20:10 -0700, Rom Lemarchand wrote:
> On the topic of maintainers for binder: both Arve Hjønnevåg
> (arve@android.com) and Riley Andrews (riandrews@android.com) have
> volunteered to be co-maintainers with Greg.
>
> We would also like to make kernel-team@android.com the maintainer of
> the whole android directory.
It's generally better to have people as maintainers than
using nameless email addresses.
^ permalink raw reply [flat|nested] 25+ messages in thread
* Re: [PATCH] staging: android: binder: move to the "real" part of the kernel
2014-10-22 3:10 ` Rom Lemarchand
2014-10-22 3:16 ` Joe Perches
@ 2014-10-24 5:00 ` Dan Carpenter
1 sibling, 0 replies; 25+ messages in thread
From: Dan Carpenter @ 2014-10-24 5:00 UTC (permalink / raw)
To: Rom Lemarchand
Cc: Arve Hjønnevåg, devel, Anup Patel, Greg Kroah-Hartman,
LKML, John Stultz, Christoffer Dall, linux-api,
Rebecca Schultz Zavin, Santosh Shilimkar, Sumit Semwal
On Tue, Oct 21, 2014 at 08:10:32PM -0700, Rom Lemarchand wrote:
> We would also like to make kernel-team@android.com the maintainer of
> the whole android directory.
Mailing lists can't be a maintainer, but you could add it as a private
list in the MAINTAINERS file.
regards,
dan carpenter
^ permalink raw reply [flat|nested] 25+ messages in thread
* Re: [PATCH] staging: android: binder: move to the "real" part of the kernel
2014-10-16 12:47 [PATCH] staging: android: binder: move to the "real" part of the kernel Greg Kroah-Hartman
` (2 preceding siblings ...)
2014-10-17 9:26 ` Dan Carpenter
@ 2014-10-17 9:43 ` Christoph Hellwig
2014-10-19 22:04 ` Greg Kroah-Hartman
2014-10-20 17:06 ` Arnd Bergmann
4 siblings, 1 reply; 25+ messages in thread
From: Christoph Hellwig @ 2014-10-17 9:43 UTC (permalink / raw)
To: Greg Kroah-Hartman
Cc: linux-kernel, devel, linux-api, Santosh Shilimkar, John Stultz,
Arve Hj?nnev?g, Sumit Semwal, Rebecca Schultz Zavin,
Christoffer Dall, Anup Patel
On Thu, Oct 16, 2014 at 02:47:41PM +0200, Greg Kroah-Hartman wrote:
> From: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
>
> The Android binder code has been "stable" for many years now. No matter
> what comes in the future, we are going to have to support this API, so
> might as well move it to the "real" part of the kernel as there's no
> real work that needs to be done to the existing code.
NAK. It's complete rubbish and does things to the FD code that it
really shouldn't. Android needs to completely redo the interface, and
there's been absolutely no work towards that.
This is exactly the sort of attitude I feared about when you started the
whole staging concepts, and it sounds like a good reason to disband
staging entirely, given that it's been mostly useless except for
boasting peoples commit statistics.
^ permalink raw reply [flat|nested] 25+ messages in thread* Re: [PATCH] staging: android: binder: move to the "real" part of the kernel
2014-10-17 9:43 ` Christoph Hellwig
@ 2014-10-19 22:04 ` Greg Kroah-Hartman
2014-10-21 10:46 ` Christoph Hellwig
0 siblings, 1 reply; 25+ messages in thread
From: Greg Kroah-Hartman @ 2014-10-19 22:04 UTC (permalink / raw)
To: Christoph Hellwig
Cc: devel, Anup Patel, linux-api, linux-kernel, Arve Hj?nnev?g,
Santosh Shilimkar, Rebecca Schultz Zavin, John Stultz,
Sumit Semwal, Christoffer Dall
On Fri, Oct 17, 2014 at 02:43:29AM -0700, Christoph Hellwig wrote:
> On Thu, Oct 16, 2014 at 02:47:41PM +0200, Greg Kroah-Hartman wrote:
> > From: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
> >
> > The Android binder code has been "stable" for many years now. No matter
> > what comes in the future, we are going to have to support this API, so
> > might as well move it to the "real" part of the kernel as there's no
> > real work that needs to be done to the existing code.
>
> NAK. It's complete rubbish and does things to the FD code that it
> really shouldn't. Android needs to completely redo the interface, and
> there's been absolutely no work towards that.
There is now work to resolve the interface, it requires someone who has
the rights to push to Android userspace. But that is going to be a
"total rewrite", and until then, this code needs to be used, no matter
how much we hate this.
> This is exactly the sort of attitude I feared about when you started the
> whole staging concepts, and it sounds like a good reason to disband
> staging entirely, given that it's been mostly useless except for
> boasting peoples commit statistics.
I know you hate staging, which is fine, it's not there for you. It's
there for others, and so much good stuff has happened with it, that I'm
not going to give it up.
But this code is different. It's something like a random driver that a
distro has been carrying for 5+ years that it has to ship due to
userspace that relies on it, so the api can't be changed. We accept
drivers like that at times, and we don't drop really old and crappy
drivers either, as long as someone is using it, and it is
self-contained.
I'll step up to maintain this and handle the interactions between grumpy
kernel developers who look at the code, and the Android developers who
are stuck with this monstrosity.
thanks,
greg k-h
^ permalink raw reply [flat|nested] 25+ messages in thread
* Re: [PATCH] staging: android: binder: move to the "real" part of the kernel
2014-10-19 22:04 ` Greg Kroah-Hartman
@ 2014-10-21 10:46 ` Christoph Hellwig
0 siblings, 0 replies; 25+ messages in thread
From: Christoph Hellwig @ 2014-10-21 10:46 UTC (permalink / raw)
To: Greg Kroah-Hartman
Cc: devel, Anup Patel, linux-api, linux-kernel, Arve Hj?nnev?g,
Santosh Shilimkar, Rebecca Schultz Zavin, John Stultz,
Sumit Semwal, Christoffer Dall
On Mon, Oct 20, 2014 at 06:04:50AM +0800, Greg Kroah-Hartman wrote:
> There is now work to resolve the interface, it requires someone who has
> the rights to push to Android userspace. But that is going to be a
> "total rewrite", and until then, this code needs to be used, no matter
> how much we hate this.
It helps to qualify why it absolutely has to, and why this is different
from other interfaces we haven't merged. Is this the last building
block to run upstream Linux on a common Android device out of the box
without needing any patches or out of tree drivers? Is there any other
good reason I might have missed.
To convince other people that merging a piece like this absolutely needs
to get merged I'd suggest you start with presenting factual argument,
and then let the broader audience weight their merrits.
I think with the known problems of the code, and the fact that the
real user ABI is a library anyway the stakes are quite high here.
So as a start please prepare a list of arguments, a detailed description
of the ABI, and post a proper patch (not a move) that suggests adding
this driver to all the relevant lists (most importantly linux-fsdevel
and linux-api) so that people with the right experience can review it.
^ permalink raw reply [flat|nested] 25+ messages in thread
* Re: [PATCH] staging: android: binder: move to the "real" part of the kernel
2014-10-16 12:47 [PATCH] staging: android: binder: move to the "real" part of the kernel Greg Kroah-Hartman
` (3 preceding siblings ...)
2014-10-17 9:43 ` Christoph Hellwig
@ 2014-10-20 17:06 ` Arnd Bergmann
4 siblings, 0 replies; 25+ messages in thread
From: Arnd Bergmann @ 2014-10-20 17:06 UTC (permalink / raw)
To: Greg Kroah-Hartman
Cc: linux-kernel, devel, linux-api, Santosh Shilimkar, John Stultz,
Arve Hjønnevåg, Sumit Semwal, Rebecca Schultz Zavin,
Christoffer Dall, Anup Patel
On Thursday 16 October 2014 14:47:41 Greg Kroah-Hartman wrote:
> From: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
>
> The Android binder code has been "stable" for many years now. No matter
> what comes in the future, we are going to have to support this API, so
> might as well move it to the "real" part of the kernel as there's no
> real work that needs to be done to the existing code.
>
> Signed-off-by: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
> ---
>
> This was discussed in the Android miniconf at the Plumbers conference.
> If anyone has any objections to this, please let me know, otherwise I'm
> queueing this up for 3.19-rc1
I'm worried about the user interface: since graduating binder out of
staging with the existing ioctl interface has never been discussed
as a real option and (I assume) everybody expected the way forward
would be to have a replacement, I don't think it ever received the
attention it should have. Specific concerns are:
- I don't think there has been an audit of which subset of the API
is actually required. IIRC, it was said initially that actual
applications don't use all the features, and that we should have
a smaller attack surface.
- Using kernel pointers in user space interfaces is an information
leak that can be used to construct an exploit. (I don't know if
this is still used that way, I think it was doing it last
time I checked).
- The driver supports two versions of the user interface (v7 and
v8), but only one of them can be selected at compile-time, and
an 'allmodconfig' kernel will only include the deprecated one
on 32-bit machines.
- The implementation is not namespace-aware and will cause
information to be shared across containers in a potentially
harmful way.
If we graduate the driver from staging, it should IMHO at least
be useful in containers to run Android user space safely.
Arnd
^ permalink raw reply [flat|nested] 25+ messages in thread