Re: [PATCH] staging: android: binder: move to the "real" part of the kernel
On Tue, Oct 21, 2014 at 08:10:32PM -0700, Rom Lemarchand wrote: > We would also like to make kernel-t...@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 -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH] staging: android: binder: move to the real part of the kernel
On Tue, Oct 21, 2014 at 08:10:32PM -0700, Rom Lemarchand wrote: We would also like to make kernel-t...@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 -- To unsubscribe from this list: send the line unsubscribe linux-kernel in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH] staging: android: binder: move to the "real" part of the kernel
On Tue, 2014-10-21 at 20:10 -0700, Rom Lemarchand wrote: > On the topic of maintainers for binder: both Arve Hjønnevåg > (a...@android.com) and Riley Andrews (riandr...@android.com) have > volunteered to be co-maintainers with Greg. > > We would also like to make kernel-t...@android.com the maintainer of > the whole android directory. It's generally better to have people as maintainers than using nameless email addresses. -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH] staging: android: binder: move to the "real" part of the kernel
On the topic of maintainers for binder: both Arve Hjønnevåg (a...@android.com) and Riley Andrews (riandr...@android.com) have volunteered to be co-maintainers with Greg. We would also like to make kernel-t...@android.com the maintainer of the whole android directory. -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH] staging: android: binder: move to the "real" part of the kernel
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 > > > > wrote: > > > > > From: Greg Kroah-Hartman > > > > > > 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 -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH] staging: android: binder: move to the "real" part of the kernel
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
> > > wrote:
> > > > From: Greg Kroah-Hartman
>
> > > 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(_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
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH] staging: android: binder: move to the "real" part of the kernel
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. -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH] staging: android: binder: move to the "real" part of the kernel
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
> > wrote:
> > > From: Greg Kroah-Hartman
> > 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(_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
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH] staging: android: binder: move to the "real" part of the kernel
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 > > wrote: > > > From: Greg Kroah-Hartman > > > > > > 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 -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH] staging: android: binder: move to the real part of the kernel
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 gre...@linuxfoundation.org wrote: From: Greg Kroah-Hartman gre...@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 -- To unsubscribe from this list: send the line unsubscribe linux-kernel in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH] staging: android: binder: move to the real part of the kernel
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
gre...@linuxfoundation.org wrote:
From: Greg Kroah-Hartman gre...@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
--
To unsubscribe from this list: send the line unsubscribe linux-kernel in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH] staging: android: binder: move to the real part of the kernel
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. -- To unsubscribe from this list: send the line unsubscribe linux-kernel in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH] staging: android: binder: move to the real part of the kernel
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
gre...@linuxfoundation.org wrote:
From: Greg Kroah-Hartman gre...@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
--
To unsubscribe from this list: send the line unsubscribe linux-kernel in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH] staging: android: binder: move to the real part of the kernel
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 gre...@linuxfoundation.org wrote: From: Greg Kroah-Hartman gre...@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 -- To unsubscribe from this list: send the line unsubscribe linux-kernel in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH] staging: android: binder: move to the real part of the kernel
On the topic of maintainers for binder: both Arve Hjønnevåg (a...@android.com) and Riley Andrews (riandr...@android.com) have volunteered to be co-maintainers with Greg. We would also like to make kernel-t...@android.com the maintainer of the whole android directory. -- To unsubscribe from this list: send the line unsubscribe linux-kernel in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH] staging: android: binder: move to the real part of the kernel
On Tue, 2014-10-21 at 20:10 -0700, Rom Lemarchand wrote: On the topic of maintainers for binder: both Arve Hjønnevåg (a...@android.com) and Riley Andrews (riandr...@android.com) have volunteered to be co-maintainers with Greg. We would also like to make kernel-t...@android.com the maintainer of the whole android directory. It's generally better to have people as maintainers than using nameless email addresses. -- To unsubscribe from this list: send the line unsubscribe linux-kernel in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH] staging: android: binder: move to the "real" part of the kernel
On Mon, Oct 20, 2014 at 2:20 AM, Dan Carpenter 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",
>445current->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, >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(_buffer->entry, >entry);
>732
Re: [PATCH] staging: android: binder: move to the "real" part of the kernel
On Thursday 16 October 2014 14:47:41 Greg Kroah-Hartman wrote: > From: Greg Kroah-Hartman > > 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 > --- > > 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 -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH] staging: android: binder: move to the "real" part of the kernel
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 -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH] staging: android: binder: move to the "real" part of the kernel
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",
445current->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, >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(_buffer->entry, >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
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH] staging: android: binder: move to the real part of the kernel
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,
445current-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
--
To unsubscribe from this list: send the line unsubscribe linux-kernel in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH] staging: android: binder: move to the real part of the kernel
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 -- To unsubscribe from this list: send the line unsubscribe linux-kernel in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH] staging: android: binder: move to the real part of the kernel
On Thursday 16 October 2014 14:47:41 Greg Kroah-Hartman wrote: From: Greg Kroah-Hartman gre...@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 gre...@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 -- To unsubscribe from this list: send the line unsubscribe linux-kernel in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH] staging: android: binder: move to the real part of the kernel
On Mon, Oct 20, 2014 at 2:20 AM, Dan Carpenter dan.carpen...@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,
445current-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 }
Re: [PATCH] staging: android: binder: move to the "real" part of the kernel
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 -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH] staging: android: binder: move to the "real" part of the kernel
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 > > > > 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 -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH] staging: android: binder: move to the "real" part of the kernel
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 -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH] staging: android: binder: move to the real part of the kernel
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 -- To unsubscribe from this list: send the line unsubscribe linux-kernel in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH] staging: android: binder: move to the real part of the kernel
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 gre...@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 -- To unsubscribe from this list: send the line unsubscribe linux-kernel in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH] staging: android: binder: move to the real part of the kernel
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 -- To unsubscribe from this list: send the line unsubscribe linux-kernel in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH] staging: android: binder: move to the "real" part of the kernel
> 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 -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH] staging: android: binder: move to the real part of the kernel
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 -- To unsubscribe from this list: send the line unsubscribe linux-kernel in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH] staging: android: binder: move to the "real" part of the kernel
On Thu, Oct 16, 2014 at 02:47:41PM +0200, Greg Kroah-Hartman wrote: > From: Greg Kroah-Hartman > > 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. -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH] staging: android: binder: move to the "real" part of the kernel
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 -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH] staging: android: binder: move to the "real" part of the kernel
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 > 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 > >> wrote: > >> > From: Greg Kroah-Hartman > >> > > >> > 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 -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH] staging: android: binder: move to the real part of the kernel
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 gre...@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 gre...@linuxfoundation.org wrote: From: Greg Kroah-Hartman gre...@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 -- To unsubscribe from this list: send the line unsubscribe linux-kernel in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH] staging: android: binder: move to the real part of the kernel
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 -- To unsubscribe from this list: send the line unsubscribe linux-kernel in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH] staging: android: binder: move to the real part of the kernel
On Thu, Oct 16, 2014 at 02:47:41PM +0200, Greg Kroah-Hartman wrote: From: Greg Kroah-Hartman gre...@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. -- To unsubscribe from this list: send the line unsubscribe linux-kernel in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH] staging: android: binder: move to the "real" part of the kernel
On Thu, Oct 16, 2014 at 4:12 PM, 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 >> wrote: >> > From: Greg Kroah-Hartman >> > >> > 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 -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH] staging: android: binder: move to the "real" part of the kernel
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 > wrote: > > From: Greg Kroah-Hartman > > > > 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 -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH] staging: android: binder: move to the "real" part of the kernel
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 > wrote: > > From: Greg Kroah-Hartman > > > > 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 -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH] staging: android: binder: move to the "real" part of the kernel
On Thu, Oct 16, 2014 at 5:47 AM, Greg Kroah-Hartman wrote: > From: Greg Kroah-Hartman > > 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 > --- > > 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 -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH] staging: android: binder: move to the "real" part of the kernel
On Thu, Oct 16, 2014 at 2:47 PM, Greg Kroah-Hartman
wrote:
> From: Greg Kroah-Hartman
>
> 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
> ---
>
> 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 ..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 ..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
>
> #endif /* _LINUX_BINDER_H */
>
> diff --git a/drivers/staging/android/binder_trace.h
> b/drivers/android/binder_trace.h
> similarity index 100%
> rename
[PATCH] staging: android: binder: move to the "real" part of the kernel
From: Greg Kroah-Hartman
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
---
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 ..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 ..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
#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
[PATCH] staging: android: binder: move to the real part of the kernel
From: Greg Kroah-Hartman gre...@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 gre...@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 ..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 ..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
Re: [PATCH] staging: android: binder: move to the real part of the kernel
On Thu, Oct 16, 2014 at 2:47 PM, Greg Kroah-Hartman
gre...@linuxfoundation.org wrote:
From: Greg Kroah-Hartman gre...@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 gre...@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 ..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 ..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
Re: [PATCH] staging: android: binder: move to the real part of the kernel
On Thu, Oct 16, 2014 at 5:47 AM, Greg Kroah-Hartman gre...@linuxfoundation.org wrote: From: Greg Kroah-Hartman gre...@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 gre...@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 -- To unsubscribe from this list: send the line unsubscribe linux-kernel in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH] staging: android: binder: move to the real part of the kernel
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 gre...@linuxfoundation.org wrote: From: Greg Kroah-Hartman gre...@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 -- To unsubscribe from this list: send the line unsubscribe linux-kernel in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH] staging: android: binder: move to the real part of the kernel
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 gre...@linuxfoundation.org wrote: From: Greg Kroah-Hartman gre...@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 -- To unsubscribe from this list: send the line unsubscribe linux-kernel in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH] staging: android: binder: move to the real part of the kernel
On Thu, Oct 16, 2014 at 4:12 PM, Greg Kroah-Hartman gre...@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 gre...@linuxfoundation.org wrote: From: Greg Kroah-Hartman gre...@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 -- To unsubscribe from this list: send the line unsubscribe linux-kernel in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
