[PATCH 0/4] Cosmetic changes to the android binder proc release code

[PATCH 0/4] Cosmetic changes to the android binder proc release code

[Mirsal Ennaime]

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

[PATCH 1/4] drivers: android: binder: Move the node release code to a separate function

[Mirsal Ennaime]

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 <mirsal@mirsal.fr>
---
 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

[PATCH 2/4] drivers: android: binder: Fix code style

[Mirsal Ennaime]

 * Use tabs
 * Remove a couple of "80-columns" checkpatch warnings
 * Separate code paths with empty lines for readability

Signed-off-by: Mirsal Ennaime <mirsal@mirsal.fr>
---
 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

[PATCH 3/4] drivers: android: binder: Remove excessive indentation

[Mirsal Ennaime]

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 <mirsal@mirsal.fr>
---
 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

[PATCH 4/4] drivers: android: binder: Fix compiler warning

[Mirsal Ennaime]

Fix an instance of -Wdeclaration-after-statement

Signed-off-by: Mirsal Ennaime <mirsal@mirsal.fr>
---
 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

Re: [PATCH 3/4] drivers: android: binder: Remove excessive indentation

[Joe Perches]

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

Re: [PATCH 3/4] drivers: android: binder: Remove excessive indentation

[mirsal]

[-- Attachment #1: Type: text/plain, Size: 1197 bytes --]

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 

[-- Attachment #2: This is a digitally signed message part --]
[-- Type: application/pgp-signature, Size: 490 bytes --]

Re: [PATCH 4/4] drivers: android: binder: Fix compiler warning

[Dan Carpenter]

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 <mirsal@mirsal.fr>
> ---
>  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

Re: [PATCH 1/4] drivers: android: binder: Move the node release code to a separate function

[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

Re: [PATCH 2/4] drivers: android: binder: Fix code style

[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

Re: [PATCH 2/4] drivers: android: binder: Fix code style

[mirsal]

[-- Attachment #1: Type: text/plain, Size: 1278 bytes --]

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 

[-- Attachment #2: This is a digitally signed message part --]
[-- Type: application/pgp-signature, Size: 490 bytes --]

[PATCH v2 0/3] Cosmetic changes to the android binder proc release code

[Mirsal Ennaime]

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

[PATCH v2 1/3] drivers: android: binder: Move the node release code to a separate function

[Mirsal Ennaime]

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 <mirsal@mirsal.fr>
---
 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

[PATCH v2 2/3] drivers: android: binder: Fix code style

[Mirsal Ennaime]

 * Use tabs
 * Remove a few "80-columns" checkpatch warnings
 * Separate code paths with empty lines for readability

Signed-off-by: Mirsal Ennaime <mirsal@mirsal.fr>
---
 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

[PATCH v2 3/3] drivers: android: binder: Remove excessive indentation

[Mirsal Ennaime]

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 <mirsal@mirsal.fr>
---
 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

Re: [PATCH v2 2/3] drivers: android: binder: Fix code style

[Arve Hjønnevåg]

On Mon, Mar 11, 2013 at 4:26 PM, Mirsal Ennaime <mirsal@mirsal.fr> wrote:
>  * Use tabs
>  * Remove a few "80-columns" checkpatch warnings
>  * Separate code paths with empty lines for readability
>
> Signed-off-by: Mirsal Ennaime <mirsal@mirsal.fr>
> ---
>  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

Re: [PATCH v2 3/3] drivers: android: binder: Remove excessive indentation

[Joe Perches]

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)

Re: [PATCH v2 3/3] drivers: android: binder: Remove excessive indentation

[Arve Hjønnevåg]

On Mon, Mar 11, 2013 at 5:04 PM, Joe Perches <joe@perches.com> 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

Re: [PATCH v2 3/3] drivers: android: binder: Remove excessive indentation

[Joe Perches]

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 <joe@perches.com> 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.

Re: [PATCH v2 2/3] drivers: android: binder: Fix code style

[mirsal]

[-- Attachment #1: Type: text/plain, Size: 1924 bytes --]

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 <mirsal@mirsal.fr> wrote:
> >  * Use tabs
> >  * Remove a few "80-columns" checkpatch warnings
> >  * Separate code paths with empty lines for readability
> >
> > Signed-off-by: Mirsal Ennaime <mirsal@mirsal.fr>
> > ---
> >  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 

[-- Attachment #2: This is a digitally signed message part --]
[-- Type: application/pgp-signature, Size: 490 bytes --]

Re: [PATCH v2 3/3] drivers: android: binder: Remove excessive indentation

[mirsal]

[-- Attachment #1: Type: text/plain, Size: 2447 bytes --]

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 

[-- Attachment #2: This is a digitally signed message part --]
[-- Type: application/pgp-signature, Size: 490 bytes --]

[PATCH v3 0/4] Cosmetic changes to the android binder proc release code

[Mirsal Ennaime]

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

[PATCH v3 1/4] drivers: android: binder: Move the node release code to a separate function

[Mirsal Ennaime]

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 <mirsal@mirsal.fr>
---
 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

[PATCH v3 2/4] drivers: android: binder: Fix code style in binder_deferred_release

[Mirsal Ennaime]

 * Use tabs where applicable
 * Remove a few "80-columns" checkpatch warnings
 * Separate code paths with empty lines for readability

Signed-off-by: Mirsal Ennaime <mirsal@mirsal.fr>
---
 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

[PATCH v3 3/4] drivers: android: binder: Remove excessive indentation

[Mirsal Ennaime]

Remove one level of indentation from the binder proc page release code
by using slightly different control semantics.

Signed-off-by: Mirsal Ennaime <mirsal@mirsal.fr>
---
 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

[PATCH v3 4/4] drivers: android: binder: Use __func__ in debug messages

[Mirsal Ennaime]

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 <mirsal@mirsal.fr>
---
 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

Re: [PATCH v3 0/4] Cosmetic changes to the android binder proc release code

[Dan Carpenter]

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 <dan.carpenter@oracle.com>

regards,
dan carpenter