binder: fix sparse warnings on locking context
Todd Kjos
maintainerPATCH v2 1/3 binder: fix sparse warnings on locking context
Add __acquire()/__release() annnotations to fix warnings in sparse context checking
There is one case where the warning was due to a lack of a "default:" case in a switch statement where a lock was being released in each of the cases, so the default case was added.
---
v2: no change, just resubmitted as #1 of 3 patches instead of by itself
drivers/android/binder.c | 43 +++++++++++++++++++++++++++++++++- drivers/android/binder_alloc.c | 1 + 2 files changed, 43 insertions(+), 1 deletion(-)
drivers/android/binder.c +42 -1
@@ -660,6 +660,7 @@ struct binder_transaction {#define binder_proc_lock (proc) _binder_proc_lock(proc, __LINE__)static void _binder_proc_lock (struct binder_proc *proc,int line) + __acquires(&proc->outer_lock) {binder_debug (BINDER_DEBUG_SPINLOCKS ,"%s: line=%d\n" , __func__, line);
@@ -675,6 +676,7 @@ _binder_proc_lock(struct binder_proc *proc, int line)#define binder_proc_unlock (_proc) _binder_proc_unlock(_proc, __LINE__)static void _binder_proc_unlock (struct binder_proc *proc,int line) + __releases(&proc->outer_lock) {binder_debug (BINDER_DEBUG_SPINLOCKS ,"%s: line=%d\n" , __func__, line);
@@ -690,6 +692,7 @@ _binder_proc_unlock(struct binder_proc *proc, int line)#define binder_inner_proc_lock (proc) _binder_inner_proc_lock(proc, __LINE__)static void _binder_inner_proc_lock (struct binder_proc *proc,int line) + __acquires(&proc->inner_lock) {binder_debug (BINDER_DEBUG_SPINLOCKS ,"%s: line=%d\n" , __func__, line);
@@ -705,6 +708,7 @@ _binder_inner_proc_lock(struct binder_proc *proc, int line)#define binder_inner_proc_unlock (proc) _binder_inner_proc_unlock(proc, __LINE__)static void _binder_inner_proc_unlock (struct binder_proc *proc,int line) + __releases(&proc->inner_lock) {binder_debug (BINDER_DEBUG_SPINLOCKS ,"%s: line=%d\n" , __func__, line);
@@ -720,6 +724,7 @@ _binder_inner_proc_unlock(struct binder_proc *proc, int line)#define binder_node_lock (node) _binder_node_lock(node, __LINE__)static void _binder_node_lock (struct binder_node *node,int line) + __acquires(&node->lock) {binder_debug (BINDER_DEBUG_SPINLOCKS ,"%s: line=%d\n" , __func__, line);
@@ -735,6 +740,7 @@ _binder_node_lock(struct binder_node *node, int line)#define binder_node_unlock (node) _binder_node_unlock(node, __LINE__)static void _binder_node_unlock (struct binder_node *node,int line) + __releases(&node->lock) {binder_debug (BINDER_DEBUG_SPINLOCKS ,"%s: line=%d\n" , __func__, line);
@@ -751,12 +757,16 @@ _binder_node_unlock(struct binder_node *node, int line)#define binder_node_inner_lock (node) _binder_node_inner_lock(node, __LINE__)static void _binder_node_inner_lock (struct binder_node *node,int line) + __acquires(&node->lock) __acquires(&node->proc->inner_lock) {binder_debug (BINDER_DEBUG_SPINLOCKS ,"%s: line=%d\n" , __func__, line);spin_lock (&node->lock);if (node->proc)binder_inner_proc_lock (node->proc); +else +/* annotation for sparse */ +__acquire (&node->proc->inner_lock); }/**
@@ -768,6 +778,7 @@ _binder_node_inner_lock(struct binder_node *node, int line)#define binder_node_inner_unlock (node) _binder_node_inner_unlock(node, __LINE__)static void _binder_node_inner_unlock (struct binder_node *node,int line) + __releases(&node->lock) __releases(&node->proc->inner_lock) {struct binder_proc *proc = node->proc;
@@ -775,6 +786,9 @@ _binder_node_inner_unlock(struct binder_node *node, int line) "%s: line=%d\n", __func__, line);if (proc)binder_inner_proc_unlock (proc); +else +/* annotation for sparse */ +__release (&node->proc->inner_lock);spin_unlock (&node->lock); }
@@ -1384,10 +1398,14 @@ static void binder_dec_node_tmpref(struct binder_node *node)binder_node_inner_lock (node);if (!node->proc)spin_lock (&binder_dead_nodes_lock); +else +__acquire (&binder_dead_nodes_lock); node->tmp_refs--;BUG_ON (node->tmp_refs <0 );if (!node->proc)spin_unlock (&binder_dead_nodes_lock); +else +__release (&binder_dead_nodes_lock);/* * Call binder_dec_node() to check if all refcounts are 0 * and cleanup is needed. Calling with strong=0 and internal=1
@@ -1890,18 +1908,22 @@ static struct binder_thread *binder_get_txn_from(*/ static struct binder_thread *binder_get_txn_from_and_acq_inner (struct binder_transaction *t) + __acquires(&t->from->proc->inner_lock) {struct binder_thread *from; from =binder_get_txn_from (t); -if (!from) +if (!from) { +__acquire (&from->proc->inner_lock);return NULL ; + }binder_inner_proc_lock (from->proc);if (t->from) {BUG_ON (from != t->from);return from; }binder_inner_proc_unlock (from->proc); +__acquire (&from->proc->inner_lock);binder_thread_dec_tmpref (from);return NULL ; }
@@ -1973,6 +1995,8 @@ static void binder_send_failed_reply(struct binder_transaction *t,binder_thread_dec_tmpref (target_thread);binder_free_transaction (t);return ; + }else { +__release (&target_thread->proc->inner_lock); } next = t->from_parent;
@@ -2394,11 +2418,15 @@ static int binder_translate_handle(struct flat_binder_object *fp, fp->cookie = node->cookie;if (node->proc)binder_inner_proc_lock (node->proc); +else +__acquire (&node->proc->inner_lock);binder_inc_node_nilocked (node, fp->hdr.type ==BINDER_TYPE_BINDER ,0 ,NULL );if (node->proc)binder_inner_proc_unlock (node->proc); +else +__release (&node->proc->inner_lock);trace_binder_transaction_ref_to_node (t, node, &src_rdata); binder_debug(BINDER_DEBUG_TRANSACTION, " ref %d desc %d -> node %d u%016llx\n",
@@ -2762,6 +2790,8 @@ static void binder_transaction(struct binder_proc *proc,binder_set_nice (in_reply_to->saved_priority); target_thread =binder_get_txn_from_and_acq_inner (in_reply_to);if (target_thread ==NULL ) { +/* annotation for sparse */ +__release (&target_thread->proc->inner_lock); return_error =BR_DEAD_REPLY ; return_error_line = __LINE__; goto err_dead_binder;
@@ -4164,6 +4194,11 @@ static int binder_thread_read(struct binder_proc *proc,if (cmd ==BR_DEAD_BINDER ) goto done;/* DEAD_BINDER notifications can cause transactions */ }break ; +default : +binder_inner_proc_unlock (proc); +pr_err ("%d:%d: bad work type %d\n" , + proc->pid, thread->pid, w->type); +break ; }if (!t)
@@ -4467,6 +4502,8 @@ static int binder_thread_release(struct binder_proc *proc,spin_lock (&t->lock);if (t->to_thread == thread) send_reply = t; + }else { +__acquire (&t->lock); } thread->is_dead = true;
@@ -4495,7 +4532,11 @@ static int binder_thread_release(struct binder_proc *proc,spin_unlock (&last_t->lock);if (t)spin_lock (&t->lock); +else +__acquire (&t->lock); } +/* annotation for sparse, lock not acquired in last iteration above */ +__release (&t->lock);/* * If this thread used poll, make sure we remove the waitqueue
drivers/android/binder_alloc.c +1 -0
@@ -939,6 +939,7 @@ enum lru_status binder_alloc_free_page(struct list_head *item,struct list_lru_one *lru, spinlock_t *lock, void *cb_arg) + __must_hold(lock) { struct mm_struct *mm = NULL; struct binder_lru_page *page = container_of(item,
Signature · 1 line
2.20.0.rc1.387.gf8505762e3-goog
Todd Kjos
maintainerPATCH 2/3 binder: fix kerneldoc header for struct binder_buffer
Fix the incomplete kerneldoc header for struct binder_buffer.
---
v2: no code change. Removed needless "Change-Id:"
There is no dependancy on patch 1/3
drivers/android/binder_alloc.h | 20 ++++++++++---------- 1 file changed, 10 insertions(+), 10 deletions(-)
drivers/android/binder_alloc.h +10 -10
@@ -30,16 +30,16 @@ struct binder_transaction;* struct binder_buffer - buffer used for binder transactions * @entry: entry alloc->buffers * @rb_node: node for allocated_buffers/free_buffers rb trees -* @free: true if buffer is free -* @allow_user_free: describe the second member of struct blah, -* @async_transaction: describe the second member of struct blah, -* @debug_id: describe the second member of struct blah, -* @transaction: describe the second member of struct blah, -* @target_node: describe the second member of struct blah, -* @data_size: describe the second member of struct blah, -* @offsets_size: describe the second member of struct blah, -* @extra_buffers_size: describe the second member of struct blah, -* @data:i describe the second member of struct blah, +* @free: %true if buffer is free +* @allow_user_free: %true if user is allowed to free buffer +* @async_transaction: %true if buffer is in use for an async txn +* @debug_id: unique ID for debugging +* @transaction: pointer to associated struct binder_transaction +* @target_node: struct binder_node associated with this buffer +* @data_size: size of @transaction data +* @offsets_size: size of array of offsets +* @extra_buffers_size: size of space for other objects (like sg lists) +* @data: pointer to base of buffer space * * Bookkeeping structure for binder transaction buffers */
Signature · 1 line
2.20.0.rc1.387.gf8505762e3-goog
Todd Kjos
maintainerPATCH 3/3 binder: filter out nodes when showing binder procs
In reply to Todd KjosWhen dumping out binder transactions via a debug node, the output is too verbose if a process has many nodes. Change the output for transaction dumps to only display nodes with pending async transactions.
---
v2: no change, just resubmitted as #3 of 3 patches instead of by itself There is no actual dependancy on patches 1 and 2 of the series
drivers/android/binder.c | 3 +++ 1 file changed, 3 insertions(+)
drivers/android/binder.c +3 -0
@@ -5440,6 +5440,9 @@ static void print_binder_proc(struct seq_file *m,for (n =rb_first (&proc->nodes); n !=NULL ; n =rb_next (n)) { struct binder_node *node = rb_entry(n, struct binder_node, rb_node); +if (!print_all && !node->has_async_transaction) +continue ; +/* * take a temporary reference on the node so it * survives and isn't removed from the tree
Signature · 1 line
2.20.0.rc1.387.gf8505762e3-goog
Greg KH
maintainerQuoted 11 lines · Todd Kjos
Add __acquire()/__release() annnotations to fix warnings in sparse context checking
There is one case where the warning was due to a lack of a "default:" case in a switch statement where a lock was being released in each of the cases, so the default case was added.
Signed-off-by Todd Kjos <[email protected]>---
v2: no change, just resubmitted as #1 of 3 patches instead of by itself
I've already applied this one, right?
thanks,
greg k-h
Quoted 10 lines · Greg KH
Quoted 8 lines · Todd Kjos
Add __acquire()/__release() annnotations to fix warnings in sparse context checking
There is one case where the warning was due to a lack of a "default:" case in a switch statement where a lock was being released in each of the cases, so the default case was added.
Quoted 5 lines
Quoted 3 lines
Signed-off-by Todd Kjos <[email protected]>---
v2: no change, just resubmitted as #1 of 3 patches instead of by itselfI've already applied this one, right?
No, not yet. I confused you by failing to add the "v2" in the subject for the other two submitted with it that you applied this morning:
binder: filter out nodes when showing binder procs
binder: fix kerneldoc header for struct binder_buffer
Sorry about the confusion. This one still needs to be applied.
-Todd
Quoted 4 lines
thanks,
greg k-h
Greg Kroah-Hartman
maintainerQuoted 17 lines · Todd Kjos
Quoted 10 lines · Greg KH
Quoted 8 lines · Todd Kjos
Add __acquire()/__release() annnotations to fix warnings in sparse context checking
There is one case where the warning was due to a lack of a "default:" case in a switch statement where a lock was being released in each of the cases, so the default case was added.
Quoted 5 lines
Quoted 3 lines
Signed-off-by Todd Kjos <[email protected]>---
v2: no change, just resubmitted as #1 of 3 patches instead of by itselfI've already applied this one, right?
Quoted 7 lines
No, not yet. I confused you by failing to add the "v2" in the subject for the other two submitted with it that you applied this morning:
binder: filter out nodes when showing binder procs
binder: fix kerneldoc header for structbinder_bufferSorry about the confusion. This one still needs to be applied.
But I thought I applied this back on November 26:
https://git.kernel.org/pub/scm/linux/kernel/git/gregkh/char-misc.git/commit/?h=char-misc-testing&id=324fa64cf4189094bc4df744a9e7214a1b81d845
You should have gotten an email from my scripts at that time.
or is this somehow a different patch with the same commit log? Or a v2 that changed from the one that I had applied? Do I need to revert this one and apply your new one?
confused,
greg k-h
On Thu, Dec 6, 2018 at 11:08 PM Greg Kroah-Hartman <[email protected]> wrote:
...
Quoted 8 lines
But I thought I applied this back on November 26:
https://git.kernel.org/pub/scm/linux/kernel/git/gregkh/char-misc.git/commit/?h=char-misc-testing&id=324fa64cf4189094bc4df744a9e7214a1b81d845You should have gotten an email from my scripts at that time.
or is this somehow a different patch with the same commit log? Or a v2 that changed from the one that I had applied? Do I need to revert this one and apply your new one?
Sorry, you are right, you already applied it. I was thrown off by a subsequent mail (https://lkml.org/lkml/2018/12/5/1042) in which you said "Ok, no need for numbering them, but they do need to be resent based on the feedback I gave. I've dropped them all from my queue because of that.".
So I thought it had been dropped. Anyway, glad its there and no need for further action on it.
-Todd