Hello,
These are cleanup patches related to the binder_deferred_release function
in the android binder staging driver which improve readability while removing
checkpatch and compiler warnings.
drivers/staging/android/binder.c | 121 +++++++++++++++++++++++++++----------------
1 file changed, 77 insertions(+), 44 deletions(-)
Hopefully I have submitted them correctly, this time :)
Thank you for your time and sorry for the noise,
Best regards,
--
mirsal
The binder_deferred_release() function has many levels of indentation
which makes it difficult to read. This patch moves the code which deals
with disposing of a binder node to a separate binder_node_deferred_release()
function, thus removing one level of indentation and allowing the code to
fit in 80 columns.
Signed-off-by: Mirsal Ennaime <[email protected]>
---
drivers/staging/android/binder.c | 80 +++++++++++++++++++++++---------------
1 file changed, 49 insertions(+), 31 deletions(-)
diff --git a/drivers/staging/android/binder.c b/drivers/staging/android/binder.c
index 24456a0..11b3f7b 100644
--- a/drivers/staging/android/binder.c
+++ b/drivers/staging/android/binder.c
@@ -2878,11 +2878,57 @@ static int binder_release(struct inode *nodp, struct file *filp)
return 0;
}
+static int binder_node_deferred_release(struct binder_node *node, int refs)
+{
+ struct binder_ref *ref;
+ int death = 0;
+
+ list_del_init(&node->work.entry);
+ binder_release_work(&node->async_todo);
+
+ if (hlist_empty(&node->refs)) {
+ kfree(node);
+ binder_stats_deleted(BINDER_STAT_NODE);
+
+ return refs;
+ }
+
+ node->proc = NULL;
+ node->local_strong_refs = 0;
+ node->local_weak_refs = 0;
+ hlist_add_head(&node->dead_node, &binder_dead_nodes);
+
+ hlist_for_each_entry(ref, &node->refs, node_entry) {
+ refs++;
+
+ if (!ref->death)
+ goto out;
+
+ death++;
+
+ if (list_empty(&ref->death->work.entry)) {
+ ref->death->work.type = BINDER_WORK_DEAD_BINDER;
+ list_add_tail(&ref->death->work.entry,
+ &ref->proc->todo);
+ wake_up_interruptible(&ref->proc->wait);
+ } else
+ BUG();
+ }
+
+out:
+ binder_debug(BINDER_DEBUG_DEAD_BINDER,
+ "node %d now dead, refs %d, death %d\n",
+ node->debug_id, refs, death);
+
+ return refs;
+}
+
static void binder_deferred_release(struct binder_proc *proc)
{
struct binder_transaction *t;
struct rb_node *n;
- int threads, nodes, incoming_refs, outgoing_refs, buffers, active_transactions, page_count;
+ int threads, nodes, incoming_refs, outgoing_refs, nd_refs,
+ buffers, active_transactions, page_count;
BUG_ON(proc->vma);
BUG_ON(proc->files);
@@ -2909,36 +2955,8 @@ static void binder_deferred_release(struct binder_proc *proc)
nodes++;
rb_erase(&node->rb_node, &proc->nodes);
- list_del_init(&node->work.entry);
- binder_release_work(&node->async_todo);
- if (hlist_empty(&node->refs)) {
- kfree(node);
- binder_stats_deleted(BINDER_STAT_NODE);
- } else {
- struct binder_ref *ref;
- int death = 0;
-
- node->proc = NULL;
- node->local_strong_refs = 0;
- node->local_weak_refs = 0;
- hlist_add_head(&node->dead_node, &binder_dead_nodes);
-
- hlist_for_each_entry(ref, &node->refs, node_entry) {
- incoming_refs++;
- if (ref->death) {
- death++;
- if (list_empty(&ref->death->work.entry)) {
- ref->death->work.type = BINDER_WORK_DEAD_BINDER;
- list_add_tail(&ref->death->work.entry, &ref->proc->todo);
- wake_up_interruptible(&ref->proc->wait);
- } else
- BUG();
- }
- }
- binder_debug(BINDER_DEBUG_DEAD_BINDER,
- "node %d now dead, refs %d, death %d\n",
- node->debug_id, incoming_refs, death);
- }
+ nd_refs = binder_node_deferred_release(node, incoming_refs);
+ incoming_refs = nd_refs;
}
outgoing_refs = 0;
while ((n = rb_first(&proc->refs_by_desc))) {
--
1.7.10.4
* Use tabs
* Remove a couple of "80-columns" checkpatch warnings
* Separate code paths with empty lines for readability
Signed-off-by: Mirsal Ennaime <[email protected]>
---
drivers/staging/android/binder.c | 22 +++++++++++++++++-----
1 file changed, 17 insertions(+), 5 deletions(-)
diff --git a/drivers/staging/android/binder.c b/drivers/staging/android/binder.c
index 11b3f7b..49cc573 100644
--- a/drivers/staging/android/binder.c
+++ b/drivers/staging/android/binder.c
@@ -2934,6 +2934,7 @@ static void binder_deferred_release(struct binder_proc *proc)
BUG_ON(proc->files);
hlist_del(&proc->proc_node);
+
if (binder_context_mgr_node && binder_context_mgr_node->proc == proc) {
binder_debug(BINDER_DEBUG_DEAD_BINDER,
"binder_release: %d context_mgr_node gone\n",
@@ -2943,28 +2944,39 @@ static void binder_deferred_release(struct binder_proc *proc)
threads = 0;
active_transactions = 0;
+
while ((n = rb_first(&proc->threads))) {
- struct binder_thread *thread = rb_entry(n, struct binder_thread, rb_node);
+ struct binder_thread *thread = rb_entry(n,
+ struct binder_thread,
+ rb_node);
+
threads++;
active_transactions += binder_free_thread(proc, thread);
}
+
nodes = 0;
incoming_refs = 0;
+
while ((n = rb_first(&proc->nodes))) {
- struct binder_node *node = rb_entry(n, struct binder_node, rb_node);
+ struct binder_node *node = rb_entry(n,
+ struct binder_node,
+ rb_node);
nodes++;
rb_erase(&node->rb_node, &proc->nodes);
nd_refs = binder_node_deferred_release(node, incoming_refs);
incoming_refs = nd_refs;
}
+
outgoing_refs = 0;
+
while ((n = rb_first(&proc->refs_by_desc))) {
struct binder_ref *ref = rb_entry(n, struct binder_ref,
rb_node_desc);
outgoing_refs++;
binder_delete_ref(ref);
}
+
binder_release_work(&proc->todo);
binder_release_work(&proc->delivered_death);
buffers = 0;
@@ -2993,9 +3005,9 @@ static void binder_deferred_release(struct binder_proc *proc)
if (proc->pages[i]) {
void *page_addr = proc->buffer + i * PAGE_SIZE;
binder_debug(BINDER_DEBUG_BUFFER_ALLOC,
- "binder_release: %d: page %d at %p not freed\n",
- proc->pid, i,
- page_addr);
+ "binder_release: %d: page %d at %p not freed\n",
+ proc->pid, i,
+ page_addr);
unmap_kernel_range((unsigned long)page_addr,
PAGE_SIZE);
__free_page(proc->pages[i]);
--
1.7.10.4
Fix an instance of -Wdeclaration-after-statement
Signed-off-by: Mirsal Ennaime <[email protected]>
---
drivers/staging/android/binder.c | 3 ++-
1 file changed, 2 insertions(+), 1 deletion(-)
diff --git a/drivers/staging/android/binder.c b/drivers/staging/android/binder.c
index 18a83e2..c833b53 100644
--- a/drivers/staging/android/binder.c
+++ b/drivers/staging/android/binder.c
@@ -3001,11 +3001,12 @@ static void binder_deferred_release(struct binder_proc *proc)
page_count = 0;
if (proc->pages) {
int i;
+ void *page_addr;
for (i = 0; i < proc->buffer_size / PAGE_SIZE; i++) {
if (!proc->pages[i])
continue;
- void *page_addr = proc->buffer + i * PAGE_SIZE;
+ page_addr = proc->buffer + i * PAGE_SIZE;
binder_debug(BINDER_DEBUG_BUFFER_ALLOC,
"binder_release: %d: page %d at %p not freed\n",
proc->pid, i,
--
1.7.10.4
Remove one level of indentation from the binder proc page release code
by using slightly different control semantics.
This is a cosmetic patch which removes checkpatch "80-columns" warnings
Signed-off-by: Mirsal Ennaime <[email protected]>
---
drivers/staging/android/binder.c | 24 +++++++++++++-----------
1 file changed, 13 insertions(+), 11 deletions(-)
diff --git a/drivers/staging/android/binder.c b/drivers/staging/android/binder.c
index 49cc573..18a83e2 100644
--- a/drivers/staging/android/binder.c
+++ b/drivers/staging/android/binder.c
@@ -3002,18 +3002,20 @@ static void binder_deferred_release(struct binder_proc *proc)
if (proc->pages) {
int i;
for (i = 0; i < proc->buffer_size / PAGE_SIZE; i++) {
- if (proc->pages[i]) {
- void *page_addr = proc->buffer + i * PAGE_SIZE;
- binder_debug(BINDER_DEBUG_BUFFER_ALLOC,
- "binder_release: %d: page %d at %p not freed\n",
- proc->pid, i,
- page_addr);
- unmap_kernel_range((unsigned long)page_addr,
- PAGE_SIZE);
- __free_page(proc->pages[i]);
- page_count++;
- }
+ if (!proc->pages[i])
+ continue;
+
+ void *page_addr = proc->buffer + i * PAGE_SIZE;
+ binder_debug(BINDER_DEBUG_BUFFER_ALLOC,
+ "binder_release: %d: page %d at %p not freed\n",
+ proc->pid, i,
+ page_addr);
+ unmap_kernel_range((unsigned long)page_addr,
+ PAGE_SIZE);
+ __free_page(proc->pages[i]);
+ page_count++;
}
+
kfree(proc->pages);
vfree(proc->buffer);
}
--
1.7.10.4
On Mon, 2013-03-11 at 20:31 +0100, Mirsal Ennaime wrote:
> Remove one level of indentation from the binder proc page release code
> by using slightly different control semantics.
[]
> diff --git a/drivers/staging/android/binder.c b/drivers/staging/android/binder.c
[]
> @@ -3002,18 +3002,20 @@ static void binder_deferred_release(struct binder_proc *proc)
> if (proc->pages) {
> int i;
> for (i = 0; i < proc->buffer_size / PAGE_SIZE; i++) {
> - if (proc->pages[i]) {
> - void *page_addr = proc->buffer + i * PAGE_SIZE;
> - binder_debug(BINDER_DEBUG_BUFFER_ALLOC,
> - "binder_release: %d: page %d at %p not freed\n",
> - proc->pid, i,
> - page_addr);
> - unmap_kernel_range((unsigned long)page_addr,
> - PAGE_SIZE);
> - __free_page(proc->pages[i]);
> - page_count++;
> - }
> + if (!proc->pages[i])
> + continue;
> +
> + void *page_addr = proc->buffer + i * PAGE_SIZE;
Please declare variables immediately after an open brace.
for (i = 0; i < proc->buffer_size / PAGE_SIZE; i++) {
void *page_addr;
if (!proc->pages[i])
continue;
page_addr = proc->buffer + i * PAGE_SIZE;
etc...
On Mon, 2013-03-11 at 13:25 -0700, Joe Perches wrote:
> On Mon, 2013-03-11 at 20:31 +0100, Mirsal Ennaime wrote:
> > Remove one level of indentation from the binder proc page release code
> > by using slightly different control semantics.
> []
> > diff --git a/drivers/staging/android/binder.c b/drivers/staging/android/binder.c
> []
> > @@ -3002,18 +3002,20 @@ static void binder_deferred_release(struct binder_proc *proc)
> > if (proc->pages) {
> > int i;
> > for (i = 0; i < proc->buffer_size / PAGE_SIZE; i++) {
> > - if (proc->pages[i]) {
> > - void *page_addr = proc->buffer + i * PAGE_SIZE;
> > - binder_debug(BINDER_DEBUG_BUFFER_ALLOC,
> > - "binder_release: %d: page %d at %p not freed\n",
> > - proc->pid, i,
> > - page_addr);
> > - unmap_kernel_range((unsigned long)page_addr,
> > - PAGE_SIZE);
> > - __free_page(proc->pages[i]);
> > - page_count++;
> > - }
> > + if (!proc->pages[i])
> > + continue;
> > +
> > + void *page_addr = proc->buffer + i * PAGE_SIZE;
>
> Please declare variables immediately after an open brace.
> [...]
Indeed, I shall merge patches 3 and 4, then.
thanks!
--
mirsal
On Mon, Mar 11, 2013 at 08:31:55PM +0100, Mirsal Ennaime wrote:
> Fix an instance of -Wdeclaration-after-statement
>
That's a compiler warning you introduced yourself on the previous
patch. Obviously, we're not going to accept the original patch. :P
> Signed-off-by: Mirsal Ennaime <[email protected]>
> ---
> drivers/staging/android/binder.c | 3 ++-
> 1 file changed, 2 insertions(+), 1 deletion(-)
>
> diff --git a/drivers/staging/android/binder.c b/drivers/staging/android/binder.c
> index 18a83e2..c833b53 100644
> --- a/drivers/staging/android/binder.c
> +++ b/drivers/staging/android/binder.c
> @@ -3001,11 +3001,12 @@ static void binder_deferred_release(struct binder_proc *proc)
> page_count = 0;
> if (proc->pages) {
> int i;
> + void *page_addr;
Put a blank line after the variable declaration section. Really
the "int i" should go at the start of the function and the page_addr
should go inside the for loop.
regards,
dan carpenter
On Mon, Mar 11, 2013 at 08:31:52PM +0100, Mirsal Ennaime wrote:
> static void binder_deferred_release(struct binder_proc *proc)
> {
> struct binder_transaction *t;
> struct rb_node *n;
> - int threads, nodes, incoming_refs, outgoing_refs, buffers, active_transactions, page_count;
> + int threads, nodes, incoming_refs, outgoing_refs, nd_refs,
> + buffers, active_transactions, page_count;
Don't introduce the new "nd_refs" variable.
regards,
dan carpenter
On Mon, Mar 11, 2013 at 08:31:53PM +0100, Mirsal Ennaime wrote:
> @@ -2943,28 +2944,39 @@ static void binder_deferred_release(struct binder_proc *proc)
>
> threads = 0;
> active_transactions = 0;
> +
The blank line here isn't really appropriate. The initialization is
logically a part of the loop. It's part of the same paragraph.
> while ((n = rb_first(&proc->threads))) {
> - struct binder_thread *thread = rb_entry(n, struct binder_thread, rb_node);
> + struct binder_thread *thread = rb_entry(n,
> + struct binder_thread,
> + rb_node);
Do this instead:
struct binder_thread *thread;
thread = rb_entry(n, struct binder_thread, rb_node);
> +
> threads++;
> active_transactions += binder_free_thread(proc, thread);
> }
> +
> nodes = 0;
> incoming_refs = 0;
> +
> while ((n = rb_first(&proc->nodes))) {
> - struct binder_node *node = rb_entry(n, struct binder_node, rb_node);
> + struct binder_node *node = rb_entry(n,
> + struct binder_node,
> + rb_node);
>
Same thing again.
regards,
dan carpenter
On Tue, 2013-03-12 at 00:54 +0300, Dan Carpenter wrote:
> On Mon, Mar 11, 2013 at 08:31:53PM +0100, Mirsal Ennaime wrote:
> > @@ -2943,28 +2944,39 @@ static void binder_deferred_release(struct binder_proc *proc)
> >
> > threads = 0;
> > active_transactions = 0;
> > +
>
> The blank line here isn't really appropriate. The initialization is
> logically a part of the loop. It's part of the same paragraph.
>
> > while ((n = rb_first(&proc->threads))) {
> > - struct binder_thread *thread = rb_entry(n, struct binder_thread, rb_node);
> > + struct binder_thread *thread = rb_entry(n,
> > + struct binder_thread,
> > + rb_node);
>
> Do this instead:
> struct binder_thread *thread;
>
> thread = rb_entry(n, struct binder_thread, rb_node);
>
> > +
> > threads++;
> > active_transactions += binder_free_thread(proc, thread);
> > }
> > +
> > nodes = 0;
> > incoming_refs = 0;
> > +
> > while ((n = rb_first(&proc->nodes))) {
> > - struct binder_node *node = rb_entry(n, struct binder_node, rb_node);
> > + struct binder_node *node = rb_entry(n,
> > + struct binder_node,
> > + rb_node);
> >
>
> Same thing again.
Resending, thank you so much for reviewing this!
All the best,
--
mirsal
* Use tabs
* Remove a few "80-columns" checkpatch warnings
* Separate code paths with empty lines for readability
Signed-off-by: Mirsal Ennaime <[email protected]>
---
drivers/staging/android/binder.c | 42 +++++++++++++++++++++++++-------------
1 file changed, 28 insertions(+), 14 deletions(-)
diff --git a/drivers/staging/android/binder.c b/drivers/staging/android/binder.c
index 43f823d..4652cd8 100644
--- a/drivers/staging/android/binder.c
+++ b/drivers/staging/android/binder.c
@@ -2927,57 +2927,69 @@ static void binder_deferred_release(struct binder_proc *proc)
{
struct binder_transaction *t;
struct rb_node *n;
- int threads, nodes, incoming_refs, outgoing_refs, buffers, active_transactions, page_count;
+ int threads, nodes, incoming_refs, outgoing_refs, buffers,
+ active_transactions, page_count;
BUG_ON(proc->vma);
BUG_ON(proc->files);
hlist_del(&proc->proc_node);
+
if (binder_context_mgr_node && binder_context_mgr_node->proc == proc) {
binder_debug(BINDER_DEBUG_DEAD_BINDER,
- "binder_release: %d context_mgr_node gone\n",
- proc->pid);
+ "binder_release: %d context_mgr_node gone\n",
+ proc->pid);
binder_context_mgr_node = NULL;
}
threads = 0;
active_transactions = 0;
while ((n = rb_first(&proc->threads))) {
- struct binder_thread *thread = rb_entry(n, struct binder_thread, rb_node);
+ struct binder_thread *thread;
+
+ thread = rb_entry(n, struct binder_thread, rb_node);
threads++;
active_transactions += binder_free_thread(proc, thread);
}
+
nodes = 0;
incoming_refs = 0;
while ((n = rb_first(&proc->nodes))) {
- struct binder_node *node = rb_entry(n, struct binder_node, rb_node);
+ struct binder_node *node;
+ node = rb_entry(n, struct binder_node, rb_node);
nodes++;
rb_erase(&node->rb_node, &proc->nodes);
incoming_refs = binder_node_release(node, incoming_refs);
}
+
outgoing_refs = 0;
while ((n = rb_first(&proc->refs_by_desc))) {
- struct binder_ref *ref = rb_entry(n, struct binder_ref,
- rb_node_desc);
+ struct binder_ref *ref;
+
+ ref = rb_entry(n, struct binder_ref, rb_node_desc);
outgoing_refs++;
binder_delete_ref(ref);
}
+
binder_release_work(&proc->todo);
binder_release_work(&proc->delivered_death);
- buffers = 0;
+ buffers = 0;
while ((n = rb_first(&proc->allocated_buffers))) {
- struct binder_buffer *buffer = rb_entry(n, struct binder_buffer,
- rb_node);
+ struct binder_buffer *buffer;
+
+ buffer = rb_entry(n, struct binder_buffer, rb_node);
+
t = buffer->transaction;
if (t) {
t->buffer = NULL;
buffer->transaction = NULL;
pr_err("release proc %d, transaction %d, not freed\n",
- proc->pid, t->debug_id);
+ proc->pid, t->debug_id);
/*BUG();*/
}
+
binder_free_buf(proc, buffer);
buffers++;
}
@@ -2987,19 +2999,21 @@ static void binder_deferred_release(struct binder_proc *proc)
page_count = 0;
if (proc->pages) {
int i;
+
for (i = 0; i < proc->buffer_size / PAGE_SIZE; i++) {
if (proc->pages[i]) {
void *page_addr = proc->buffer + i * PAGE_SIZE;
binder_debug(BINDER_DEBUG_BUFFER_ALLOC,
- "binder_release: %d: page %d at %p not freed\n",
- proc->pid, i,
- page_addr);
+ "binder_release: %d: page %d at %p not freed\n",
+ proc->pid, i,
+ page_addr);
unmap_kernel_range((unsigned long)page_addr,
PAGE_SIZE);
__free_page(proc->pages[i]);
page_count++;
}
}
+
kfree(proc->pages);
vfree(proc->buffer);
}
--
1.7.10.4
The binder_deferred_release() function has many levels of indentation
which makes it difficult to read. This patch moves the code which deals
with disposing of a binder node to a separate binder_node_release()
function, thus removing one level of indentation and allowing the code to
fit in 80 columns.
Signed-off-by: Mirsal Ennaime <[email protected]>
---
drivers/staging/android/binder.c | 76 +++++++++++++++++++++++---------------
1 file changed, 46 insertions(+), 30 deletions(-)
diff --git a/drivers/staging/android/binder.c b/drivers/staging/android/binder.c
index 24456a0..43f823d 100644
--- a/drivers/staging/android/binder.c
+++ b/drivers/staging/android/binder.c
@@ -2878,6 +2878,51 @@ static int binder_release(struct inode *nodp, struct file *filp)
return 0;
}
+static int binder_node_release(struct binder_node *node, int refs)
+{
+ struct binder_ref *ref;
+ int death = 0;
+
+ list_del_init(&node->work.entry);
+ binder_release_work(&node->async_todo);
+
+ if (hlist_empty(&node->refs)) {
+ kfree(node);
+ binder_stats_deleted(BINDER_STAT_NODE);
+
+ return refs;
+ }
+
+ node->proc = NULL;
+ node->local_strong_refs = 0;
+ node->local_weak_refs = 0;
+ hlist_add_head(&node->dead_node, &binder_dead_nodes);
+
+ hlist_for_each_entry(ref, &node->refs, node_entry) {
+ refs++;
+
+ if (!ref->death)
+ goto out;
+
+ death++;
+
+ if (list_empty(&ref->death->work.entry)) {
+ ref->death->work.type = BINDER_WORK_DEAD_BINDER;
+ list_add_tail(&ref->death->work.entry,
+ &ref->proc->todo);
+ wake_up_interruptible(&ref->proc->wait);
+ } else
+ BUG();
+ }
+
+out:
+ binder_debug(BINDER_DEBUG_DEAD_BINDER,
+ "node %d now dead, refs %d, death %d\n",
+ node->debug_id, refs, death);
+
+ return refs;
+}
+
static void binder_deferred_release(struct binder_proc *proc)
{
struct binder_transaction *t;
@@ -2909,36 +2954,7 @@ static void binder_deferred_release(struct binder_proc *proc)
nodes++;
rb_erase(&node->rb_node, &proc->nodes);
- list_del_init(&node->work.entry);
- binder_release_work(&node->async_todo);
- if (hlist_empty(&node->refs)) {
- kfree(node);
- binder_stats_deleted(BINDER_STAT_NODE);
- } else {
- struct binder_ref *ref;
- int death = 0;
-
- node->proc = NULL;
- node->local_strong_refs = 0;
- node->local_weak_refs = 0;
- hlist_add_head(&node->dead_node, &binder_dead_nodes);
-
- hlist_for_each_entry(ref, &node->refs, node_entry) {
- incoming_refs++;
- if (ref->death) {
- death++;
- if (list_empty(&ref->death->work.entry)) {
- ref->death->work.type = BINDER_WORK_DEAD_BINDER;
- list_add_tail(&ref->death->work.entry, &ref->proc->todo);
- wake_up_interruptible(&ref->proc->wait);
- } else
- BUG();
- }
- }
- binder_debug(BINDER_DEBUG_DEAD_BINDER,
- "node %d now dead, refs %d, death %d\n",
- node->debug_id, incoming_refs, death);
- }
+ incoming_refs = binder_node_release(node, incoming_refs);
}
outgoing_refs = 0;
while ((n = rb_first(&proc->refs_by_desc))) {
--
1.7.10.4
Remove one level of indentation from the binder proc page release code
by using slightly different control semantics.
This is a cosmetic patch which removes checkpatch "80-columns" warnings
Signed-off-by: Mirsal Ennaime <[email protected]>
---
drivers/staging/android/binder.c | 25 ++++++++++++++-----------
1 file changed, 14 insertions(+), 11 deletions(-)
diff --git a/drivers/staging/android/binder.c b/drivers/staging/android/binder.c
index 4652cd8..db214ce 100644
--- a/drivers/staging/android/binder.c
+++ b/drivers/staging/android/binder.c
@@ -3001,17 +3001,20 @@ static void binder_deferred_release(struct binder_proc *proc)
int i;
for (i = 0; i < proc->buffer_size / PAGE_SIZE; i++) {
- if (proc->pages[i]) {
- void *page_addr = proc->buffer + i * PAGE_SIZE;
- binder_debug(BINDER_DEBUG_BUFFER_ALLOC,
- "binder_release: %d: page %d at %p not freed\n",
- proc->pid, i,
- page_addr);
- unmap_kernel_range((unsigned long)page_addr,
- PAGE_SIZE);
- __free_page(proc->pages[i]);
- page_count++;
- }
+ void *page_addr;
+
+ if (!proc->pages[i])
+ continue;
+
+ page_addr = proc->buffer + i * PAGE_SIZE;
+ binder_debug(BINDER_DEBUG_BUFFER_ALLOC,
+ "binder_release: %d: page %d at %p not freed\n",
+ proc->pid, i,
+ page_addr);
+ unmap_kernel_range((unsigned long)page_addr,
+ PAGE_SIZE);
+ __free_page(proc->pages[i]);
+ page_count++;
}
kfree(proc->pages);
--
1.7.10.4
Hello,
These are cleanup patches related to the binder_deferred_release function
in the android binder staging driver which improve readability while removing
checkpatch and compiler warnings.
drivers/staging/android/binder.c | 137 +++++++++++++++++-----------
1 file changed, 85 insertions(+), 52 deletions(-)
Changes from v1:
* squashed patches 3 and 4 together instead of introducing a warning
in patch 3 and then fix it in patch 4. (Following Joe Perches' review)
* removed the extra nd_refs variable from patch 1 and fixed whitespace
in patch 2 (Following Dan Carpenter's reviews)
Thank you for your time and sorry again for the noise,
Best regards,
--
mirsal
On Mon, Mar 11, 2013 at 4:26 PM, Mirsal Ennaime <[email protected]> wrote:
> * Use tabs
> * Remove a few "80-columns" checkpatch warnings
> * Separate code paths with empty lines for readability
>
> Signed-off-by: Mirsal Ennaime <[email protected]>
> ---
> drivers/staging/android/binder.c | 42 +++++++++++++++++++++++++-------------
> 1 file changed, 28 insertions(+), 14 deletions(-)
>
> diff --git a/drivers/staging/android/binder.c b/drivers/staging/android/binder.c
> index 43f823d..4652cd8 100644
> --- a/drivers/staging/android/binder.c
> +++ b/drivers/staging/android/binder.c
> @@ -2927,57 +2927,69 @@ static void binder_deferred_release(struct binder_proc *proc)
> {
> struct binder_transaction *t;
> struct rb_node *n;
> - int threads, nodes, incoming_refs, outgoing_refs, buffers, active_transactions, page_count;
> + int threads, nodes, incoming_refs, outgoing_refs, buffers,
> + active_transactions, page_count;
>
> BUG_ON(proc->vma);
> BUG_ON(proc->files);
>
> hlist_del(&proc->proc_node);
> +
> if (binder_context_mgr_node && binder_context_mgr_node->proc == proc) {
> binder_debug(BINDER_DEBUG_DEAD_BINDER,
> - "binder_release: %d context_mgr_node gone\n",
> - proc->pid);
> + "binder_release: %d context_mgr_node gone\n",
> + proc->pid);
I don't like this change (and the others like it). If is not uncommon
the align arguments on that don't fit the first line with the
arguments on the first line, so why change it here?
--
Arve Hj?nnev?g
On Tue, 2013-03-12 at 00:26 +0100, Mirsal Ennaime wrote:
> Remove one level of indentation from the binder proc page release code
> by using slightly different control semantics.
>
> This is a cosmetic patch which removes checkpatch "80-columns" warnings
More trivia:
> diff --git a/drivers/staging/android/binder.c b/drivers/staging/android/binder.c
[]
> @@ -3001,17 +3001,20 @@ static void binder_deferred_release(struct binder_proc *proc)
> int i;
>
> for (i = 0; i < proc->buffer_size / PAGE_SIZE; i++) {
> - if (proc->pages[i]) {
> - void *page_addr = proc->buffer + i * PAGE_SIZE;
> - binder_debug(BINDER_DEBUG_BUFFER_ALLOC,
> - "binder_release: %d: page %d at %p not freed\n",
> - proc->pid, i,
> - page_addr);
> - unmap_kernel_range((unsigned long)page_addr,
> - PAGE_SIZE);
> - __free_page(proc->pages[i]);
> - page_count++;
> - }
> + void *page_addr;
> +
> + if (!proc->pages[i])
> + continue;
> +
> + page_addr = proc->buffer + i * PAGE_SIZE;
> + binder_debug(BINDER_DEBUG_BUFFER_ALLOC,
> + "binder_release: %d: page %d at %p not freed\n",
> + proc->pid, i,
> + page_addr);
> + unmap_kernel_range((unsigned long)page_addr,
> + PAGE_SIZE);
Please align single function call args to open parenthesis.
Please fill to 80 chars where appropriate.
I think using %s, __func__ is better than embedded function names.
like:
binder_debug(BINDER_DEBUG_BUFFER_ALLOC,
"%s: %d: page %d at %p not freed\n",
__func__, proc->pid, i, page_addr);
unmap_kernel_range((unsigned long)page_addr, PAGE_SIZE);
Also for the binder folk:
I think it's odd to use pr_info in binder_debug.
Why not use KERN_DEBUG or pr_debug/dynamic_debugging?
#define binder_debug(mask, x...) \
do { \
if (binder_debug_mask & mask) \
pr_info(x); \
} while (0)
On Mon, Mar 11, 2013 at 5:04 PM, Joe Perches <[email protected]> wrote:
...
> I think it's odd to use pr_info in binder_debug.
> Why not use KERN_DEBUG or pr_debug/dynamic_debugging?
>
> #define binder_debug(mask, x...) \
> do { \
> if (binder_debug_mask & mask) \
> pr_info(x); \
> } while (0)
>
>
This code predates the dynamic_debugging framework, but I also find it
easier to use so I would be reluctant to convert it unless there is an
easy way to match the current behavior. It is useful to turn a set of
debug messages on by class and to have some classes on by default.
--
Arve Hj?nnev?g
On Mon, 2013-03-11 at 17:21 -0700, Arve Hj?nnev?g wrote:
> On Mon, Mar 11, 2013 at 5:04 PM, Joe Perches <[email protected]> wrote:
> ...
> > I think it's odd to use pr_info in binder_debug.
> > Why not use KERN_DEBUG or pr_debug/dynamic_debugging?
> >
> > #define binder_debug(mask, x...) \
> > do { \
> > if (binder_debug_mask & mask) \
> > pr_info(x); \
> > } while (0)
> >
> >
>
> This code predates the dynamic_debugging framework, but I also find it
> easier to use so I would be reluctant to convert it unless there is an
> easy way to match the current behavior. It is useful to turn a set of
> debug messages on by class and to have some classes on by default.
No doubt it's easier this way and there are many, many
macros like it sprinkled throughout the kernel sources.
The dynamic_debug framework currently has no mask/level
use than can turn on/off classes of messages.
Emitting at KERN_INFO instead of KERN_DEBUG though is odd.
On Mon, 2013-03-11 at 16:57 -0700, Arve Hjønnevåg wrote:
> On Mon, Mar 11, 2013 at 4:26 PM, Mirsal Ennaime <[email protected]> wrote:
> > * Use tabs
> > * Remove a few "80-columns" checkpatch warnings
> > * Separate code paths with empty lines for readability
> >
> > Signed-off-by: Mirsal Ennaime <[email protected]>
> > ---
> > drivers/staging/android/binder.c | 42 +++++++++++++++++++++++++-------------
> > 1 file changed, 28 insertions(+), 14 deletions(-)
> >
> > diff --git a/drivers/staging/android/binder.c b/drivers/staging/android/binder.c
> > index 43f823d..4652cd8 100644
> > --- a/drivers/staging/android/binder.c
> > +++ b/drivers/staging/android/binder.c
> > @@ -2927,57 +2927,69 @@ static void binder_deferred_release(struct binder_proc *proc)
> > {
> > struct binder_transaction *t;
> > struct rb_node *n;
> > - int threads, nodes, incoming_refs, outgoing_refs, buffers, active_transactions, page_count;
> > + int threads, nodes, incoming_refs, outgoing_refs, buffers,
> > + active_transactions, page_count;
> >
> > BUG_ON(proc->vma);
> > BUG_ON(proc->files);
> >
> > hlist_del(&proc->proc_node);
> > +
> > if (binder_context_mgr_node && binder_context_mgr_node->proc == proc) {
> > binder_debug(BINDER_DEBUG_DEAD_BINDER,
> > - "binder_release: %d context_mgr_node gone\n",
> > - proc->pid);
> > + "binder_release: %d context_mgr_node gone\n",
> > + proc->pid);
>
> I don't like this change (and the others like it). If is not uncommon
> the align arguments on that don't fit the first line with the
> arguments on the first line, so why change it here?
I actually took the "no tabs for indentation" rule a bit too literally.
Fixed in v3, thank you!
--
mirsal
On Mon, 2013-03-11 at 17:04 -0700, Joe Perches wrote:
> On Tue, 2013-03-12 at 00:26 +0100, Mirsal Ennaime wrote:
> > Remove one level of indentation from the binder proc page release code
> > by using slightly different control semantics.
> >
> > This is a cosmetic patch which removes checkpatch "80-columns" warnings
>
> More trivia:
>
> > diff --git a/drivers/staging/android/binder.c b/drivers/staging/android/binder.c
> []
> > @@ -3001,17 +3001,20 @@ static void binder_deferred_release(struct binder_proc *proc)
> > int i;
> >
> > for (i = 0; i < proc->buffer_size / PAGE_SIZE; i++) {
> > - if (proc->pages[i]) {
> > - void *page_addr = proc->buffer + i * PAGE_SIZE;
> > - binder_debug(BINDER_DEBUG_BUFFER_ALLOC,
> > - "binder_release: %d: page %d at %p not freed\n",
> > - proc->pid, i,
> > - page_addr);
> > - unmap_kernel_range((unsigned long)page_addr,
> > - PAGE_SIZE);
> > - __free_page(proc->pages[i]);
> > - page_count++;
> > - }
> > + void *page_addr;
> > +
> > + if (!proc->pages[i])
> > + continue;
> > +
> > + page_addr = proc->buffer + i * PAGE_SIZE;
> > + binder_debug(BINDER_DEBUG_BUFFER_ALLOC,
> > + "binder_release: %d: page %d at %p not freed\n",
> > + proc->pid, i,
> > + page_addr);
> > + unmap_kernel_range((unsigned long)page_addr,
> > + PAGE_SIZE);
>
> Please align single function call args to open parenthesis.
> Please fill to 80 chars where appropriate.
Fixed in v3, thanks!
> I think using %s, __func__ is better than embedded function names.
>
> like:
> binder_debug(BINDER_DEBUG_BUFFER_ALLOC,
> "%s: %d: page %d at %p not freed\n",
> __func__, proc->pid, i, page_addr);
> unmap_kernel_range((unsigned long)page_addr, PAGE_SIZE);
Indeed, however binder_release is not the name of the calling function,
nor is it further up the stack. You are probably right in that __func__
should be printed rather than binder_release as it is a bit misleading.
I'm adding a separate patch for this.
> Also for the binder folk:
>
> I think it's odd to use pr_info in binder_debug.
> Why not use KERN_DEBUG or pr_debug/dynamic_debugging?
>
> #define binder_debug(mask, x...) \
> do { \
> if (binder_debug_mask & mask) \
> pr_info(x); \
> } while (0)
I'd be happy to change it to use pr_debug if that is correct.
Best regards,
--
mirsal
The binder_deferred_release() function has many levels of indentation
which makes it difficult to read. This patch moves the code which deals
with disposing of a binder node to a separate binder_node_release()
function, thus removing one level of indentation and allowing the code to
fit in 80 columns.
Signed-off-by: Mirsal Ennaime <[email protected]>
---
drivers/staging/android/binder.c | 76 +++++++++++++++++++++++---------------
1 file changed, 46 insertions(+), 30 deletions(-)
diff --git a/drivers/staging/android/binder.c b/drivers/staging/android/binder.c
index 24456a0..9180a5b 100644
--- a/drivers/staging/android/binder.c
+++ b/drivers/staging/android/binder.c
@@ -2878,6 +2878,51 @@ static int binder_release(struct inode *nodp, struct file *filp)
return 0;
}
+static int binder_node_release(struct binder_node *node, int refs)
+{
+ struct binder_ref *ref;
+ int death = 0;
+
+ list_del_init(&node->work.entry);
+ binder_release_work(&node->async_todo);
+
+ if (hlist_empty(&node->refs)) {
+ kfree(node);
+ binder_stats_deleted(BINDER_STAT_NODE);
+
+ return refs;
+ }
+
+ node->proc = NULL;
+ node->local_strong_refs = 0;
+ node->local_weak_refs = 0;
+ hlist_add_head(&node->dead_node, &binder_dead_nodes);
+
+ hlist_for_each_entry(ref, &node->refs, node_entry) {
+ refs++;
+
+ if (!ref->death)
+ goto out;
+
+ death++;
+
+ if (list_empty(&ref->death->work.entry)) {
+ ref->death->work.type = BINDER_WORK_DEAD_BINDER;
+ list_add_tail(&ref->death->work.entry,
+ &ref->proc->todo);
+ wake_up_interruptible(&ref->proc->wait);
+ } else
+ BUG();
+ }
+
+out:
+ binder_debug(BINDER_DEBUG_DEAD_BINDER,
+ "node %d now dead, refs %d, death %d\n",
+ node->debug_id, refs, death);
+
+ return refs;
+}
+
static void binder_deferred_release(struct binder_proc *proc)
{
struct binder_transaction *t;
@@ -2909,36 +2954,7 @@ static void binder_deferred_release(struct binder_proc *proc)
nodes++;
rb_erase(&node->rb_node, &proc->nodes);
- list_del_init(&node->work.entry);
- binder_release_work(&node->async_todo);
- if (hlist_empty(&node->refs)) {
- kfree(node);
- binder_stats_deleted(BINDER_STAT_NODE);
- } else {
- struct binder_ref *ref;
- int death = 0;
-
- node->proc = NULL;
- node->local_strong_refs = 0;
- node->local_weak_refs = 0;
- hlist_add_head(&node->dead_node, &binder_dead_nodes);
-
- hlist_for_each_entry(ref, &node->refs, node_entry) {
- incoming_refs++;
- if (ref->death) {
- death++;
- if (list_empty(&ref->death->work.entry)) {
- ref->death->work.type = BINDER_WORK_DEAD_BINDER;
- list_add_tail(&ref->death->work.entry, &ref->proc->todo);
- wake_up_interruptible(&ref->proc->wait);
- } else
- BUG();
- }
- }
- binder_debug(BINDER_DEBUG_DEAD_BINDER,
- "node %d now dead, refs %d, death %d\n",
- node->debug_id, incoming_refs, death);
- }
+ incoming_refs = binder_node_release(node, incoming_refs);
}
outgoing_refs = 0;
while ((n = rb_first(&proc->refs_by_desc))) {
--
1.7.10.4
* Use tabs where applicable
* Remove a few "80-columns" checkpatch warnings
* Separate code paths with empty lines for readability
Signed-off-by: Mirsal Ennaime <[email protected]>
---
drivers/staging/android/binder.c | 32 ++++++++++++++++++++++----------
1 file changed, 22 insertions(+), 10 deletions(-)
diff --git a/drivers/staging/android/binder.c b/drivers/staging/android/binder.c
index 9180a5b..ccf3087 100644
--- a/drivers/staging/android/binder.c
+++ b/drivers/staging/android/binder.c
@@ -2927,12 +2927,14 @@ static void binder_deferred_release(struct binder_proc *proc)
{
struct binder_transaction *t;
struct rb_node *n;
- int threads, nodes, incoming_refs, outgoing_refs, buffers, active_transactions, page_count;
+ int threads, nodes, incoming_refs, outgoing_refs, buffers,
+ active_transactions, page_count;
BUG_ON(proc->vma);
BUG_ON(proc->files);
hlist_del(&proc->proc_node);
+
if (binder_context_mgr_node && binder_context_mgr_node->proc == proc) {
binder_debug(BINDER_DEBUG_DEAD_BINDER,
"binder_release: %d context_mgr_node gone\n",
@@ -2943,33 +2945,42 @@ static void binder_deferred_release(struct binder_proc *proc)
threads = 0;
active_transactions = 0;
while ((n = rb_first(&proc->threads))) {
- struct binder_thread *thread = rb_entry(n, struct binder_thread, rb_node);
+ struct binder_thread *thread;
+
+ thread = rb_entry(n, struct binder_thread, rb_node);
threads++;
active_transactions += binder_free_thread(proc, thread);
}
+
nodes = 0;
incoming_refs = 0;
while ((n = rb_first(&proc->nodes))) {
- struct binder_node *node = rb_entry(n, struct binder_node, rb_node);
+ struct binder_node *node;
+ node = rb_entry(n, struct binder_node, rb_node);
nodes++;
rb_erase(&node->rb_node, &proc->nodes);
incoming_refs = binder_node_release(node, incoming_refs);
}
+
outgoing_refs = 0;
while ((n = rb_first(&proc->refs_by_desc))) {
- struct binder_ref *ref = rb_entry(n, struct binder_ref,
- rb_node_desc);
+ struct binder_ref *ref;
+
+ ref = rb_entry(n, struct binder_ref, rb_node_desc);
outgoing_refs++;
binder_delete_ref(ref);
}
+
binder_release_work(&proc->todo);
binder_release_work(&proc->delivered_death);
- buffers = 0;
+ buffers = 0;
while ((n = rb_first(&proc->allocated_buffers))) {
- struct binder_buffer *buffer = rb_entry(n, struct binder_buffer,
- rb_node);
+ struct binder_buffer *buffer;
+
+ buffer = rb_entry(n, struct binder_buffer, rb_node);
+
t = buffer->transaction;
if (t) {
t->buffer = NULL;
@@ -2978,6 +2989,7 @@ static void binder_deferred_release(struct binder_proc *proc)
proc->pid, t->debug_id);
/*BUG();*/
}
+
binder_free_buf(proc, buffer);
buffers++;
}
@@ -2987,13 +2999,13 @@ static void binder_deferred_release(struct binder_proc *proc)
page_count = 0;
if (proc->pages) {
int i;
+
for (i = 0; i < proc->buffer_size / PAGE_SIZE; i++) {
if (proc->pages[i]) {
void *page_addr = proc->buffer + i * PAGE_SIZE;
binder_debug(BINDER_DEBUG_BUFFER_ALLOC,
"binder_release: %d: page %d at %p not freed\n",
- proc->pid, i,
- page_addr);
+ proc->pid, i, page_addr);
unmap_kernel_range((unsigned long)page_addr,
PAGE_SIZE);
__free_page(proc->pages[i]);
--
1.7.10.4
Hello,
These are cleanup patches related to the binder_deferred_release function
in the android binder staging driver which improve readability while removing
checkpatch and compiler warnings.
drivers/staging/android/binder.c | 138 +++++++++++++++++-----------
1 file changed, 84 insertions(+), 54 deletions(-)
Changes from v1:
* squashed patches 3 and 4 together instead of introducing a warning
in patch 3 then fix it in patch 4. (Following Joe Perches' review)
* removed the extra nd_refs variable from patch 1 and fixed whitespace
in patch 2 (Following Dan Carpenter's reviews)
* renamed the new binder_node_release_deferred function to a shorter
binder_node_release as releasing a node will allways be deferred
Changes from v2
* kept arguments aligned with parentesis (Following
Arve Hjønnevåg's review)
* added a fourth patch which removes function names from string litterals
in favor __func__ (Following Dan Carpenter's review)
* improved commit log messages slightly
Please excuse my wasting of your time for such trivia.
I do want to get the basics of submitting patches right
before starting any heavier lifting.
Thank you for your time and sorry again for the noise,
Best regards,
--
mirsal
Remove one level of indentation from the binder proc page release code
by using slightly different control semantics.
Signed-off-by: Mirsal Ennaime <[email protected]>
---
drivers/staging/android/binder.c | 22 ++++++++++++----------
1 file changed, 12 insertions(+), 10 deletions(-)
diff --git a/drivers/staging/android/binder.c b/drivers/staging/android/binder.c
index ccf3087..9db21b4 100644
--- a/drivers/staging/android/binder.c
+++ b/drivers/staging/android/binder.c
@@ -3001,16 +3001,18 @@ static void binder_deferred_release(struct binder_proc *proc)
int i;
for (i = 0; i < proc->buffer_size / PAGE_SIZE; i++) {
- if (proc->pages[i]) {
- void *page_addr = proc->buffer + i * PAGE_SIZE;
- binder_debug(BINDER_DEBUG_BUFFER_ALLOC,
- "binder_release: %d: page %d at %p not freed\n",
- proc->pid, i, page_addr);
- unmap_kernel_range((unsigned long)page_addr,
- PAGE_SIZE);
- __free_page(proc->pages[i]);
- page_count++;
- }
+ void *page_addr;
+
+ if (!proc->pages[i])
+ continue;
+
+ page_addr = proc->buffer + i * PAGE_SIZE;
+ binder_debug(BINDER_DEBUG_BUFFER_ALLOC,
+ "binder_release: %d: page %d at %p not freed\n",
+ proc->pid, i, page_addr);
+ unmap_kernel_range((unsigned long)page_addr, PAGE_SIZE);
+ __free_page(proc->pages[i]);
+ page_count++;
}
kfree(proc->pages);
vfree(proc->buffer);
--
1.7.10.4
Debug messages sent in binder_deferred_release begin with
"binder_release:" which is a bit misleading as binder_release is not
directly part of the call stack. Use __func__ instead for debug messages
in binder_deferred_release.
Signed-off-by: Mirsal Ennaime <[email protected]>
---
drivers/staging/android/binder.c | 14 +++++++-------
1 file changed, 7 insertions(+), 7 deletions(-)
diff --git a/drivers/staging/android/binder.c b/drivers/staging/android/binder.c
index 9db21b4..1567ac2 100644
--- a/drivers/staging/android/binder.c
+++ b/drivers/staging/android/binder.c
@@ -2937,8 +2937,8 @@ static void binder_deferred_release(struct binder_proc *proc)
if (binder_context_mgr_node && binder_context_mgr_node->proc == proc) {
binder_debug(BINDER_DEBUG_DEAD_BINDER,
- "binder_release: %d context_mgr_node gone\n",
- proc->pid);
+ "%s: %d context_mgr_node gone\n",
+ __func__, proc->pid);
binder_context_mgr_node = NULL;
}
@@ -3008,8 +3008,8 @@ static void binder_deferred_release(struct binder_proc *proc)
page_addr = proc->buffer + i * PAGE_SIZE;
binder_debug(BINDER_DEBUG_BUFFER_ALLOC,
- "binder_release: %d: page %d at %p not freed\n",
- proc->pid, i, page_addr);
+ "%s: %d: page %d at %p not freed\n",
+ __func__, proc->pid, i, page_addr);
unmap_kernel_range((unsigned long)page_addr, PAGE_SIZE);
__free_page(proc->pages[i]);
page_count++;
@@ -3021,9 +3021,9 @@ static void binder_deferred_release(struct binder_proc *proc)
put_task_struct(proc->tsk);
binder_debug(BINDER_DEBUG_OPEN_CLOSE,
- "binder_release: %d threads %d, nodes %d (ref %d), refs %d, active transactions %d, buffers %d, pages %d\n",
- proc->pid, threads, nodes, incoming_refs, outgoing_refs,
- active_transactions, buffers, page_count);
+ "%s: %d threads %d, nodes %d (ref %d), refs %d, active transactions %d, buffers %d, pages %d\n",
+ __func__, proc->pid, threads, nodes, incoming_refs,
+ outgoing_refs, active_transactions, buffers, page_count);
kfree(proc);
}
--
1.7.10.4
On Tue, Mar 12, 2013 at 11:41:58AM +0100, Mirsal Ennaime wrote:
> Changes from v2
>
> * kept arguments aligned with parentesis (Following
> Arve Hj?nnev?g's review)
> * added a fourth patch which removes function names from string litterals
> in favor __func__ (Following Dan Carpenter's review)
Actually that was Joe.
Looks good.
Reviewed-by: Dan Carpenter <[email protected]>
regards,
dan carpenter