[PATCH 10/13] android: binder: refactor binder_thread_read loop

[Riley Andrews]

Add dedicated functions for every work type in the switch statement.
Refactor the loop logic to remove the while 1, and make it explicit
which work items cause the loop to exit.

Signed-off-by: Riley Andrews <riandrews@xxxxxxxxxxx>
---
 drivers/android/binder.c | 433 +++++++++++++++++++++++++----------------------
 1 file changed, 231 insertions(+), 202 deletions(-)

diff --git a/drivers/android/binder.c b/drivers/android/binder.c
index abd5556..b69ca0a 100644
--- a/drivers/android/binder.c
+++ b/drivers/android/binder.c
@@ -2327,6 +2327,207 @@ static int binder_has_thread_work(struct binder_thread *thread)
 		(thread->looper & BINDER_LOOPER_STATE_NEED_RETURN);
 }
 
+static int binder_work_transaction(struct binder_thread *thread,
+				   struct binder_work *w, void * __user *ptr)
+{
+	struct binder_proc *proc = thread->proc;
+	struct binder_transaction_data tr;
+	struct binder_transaction *t;
+	uint32_t cmd;
+
+	t = container_of(w, struct binder_transaction, work);
+	BUG_ON(!t->buffer);
+	if (t->buffer->target_node) {
+		struct binder_node *target_node = t->buffer->target_node;
+
+		tr.target.ptr = target_node->ptr;
+		tr.cookie =  target_node->cookie;
+		t->saved_priority = task_nice(current);
+		if (t->priority < target_node->min_priority &&
+		    !(t->flags & TF_ONE_WAY))
+			binder_set_nice(t->priority);
+		else if (!(t->flags & TF_ONE_WAY) ||
+			 t->saved_priority > target_node->min_priority)
+			binder_set_nice(target_node->min_priority);
+		cmd = BR_TRANSACTION;
+	} else {
+		tr.target.ptr = 0;
+		tr.cookie = 0;
+		cmd = BR_REPLY;
+	}
+	tr.code = t->code;
+	tr.flags = t->flags;
+	tr.sender_euid = from_kuid(current_user_ns(), t->sender_euid);
+
+	if (t->from) {
+		struct task_struct *sender = t->from->proc->tsk;
+
+		tr.sender_pid = task_tgid_nr_ns(sender,
+						task_active_pid_ns(current));
+	} else {
+		tr.sender_pid = 0;
+	}
+
+	tr.data_size = t->buffer->data_size;
+	tr.offsets_size = t->buffer->offsets_size;
+	tr.data.ptr.buffer = (binder_uintptr_t)((uintptr_t)t->buffer->data +
+				proc->user_buffer_offset);
+	tr.data.ptr.offsets = tr.data.ptr.buffer + ALIGN(t->buffer->data_size,
+				    sizeof(void *));
+
+	if (put_user(cmd, (uint32_t __user *)*ptr))
+		return -EFAULT;
+	*ptr += sizeof(uint32_t);
+	if (copy_to_user(*ptr, &tr, sizeof(tr)))
+		return -EFAULT;
+	*ptr += sizeof(tr);
+
+	trace_binder_transaction_received(t);
+	binder_stat_br(proc, thread, cmd);
+	binder_debug(BINDER_DEBUG_TRANSACTION,
+		     "%d:%d %s %d %d:%d, cmd %d size %zd-%zd ptr %016llx-%016llx\n",
+		     proc->pid, thread->pid,
+		     (cmd == BR_TRANSACTION) ? "BR_TRANSACTION" : "BR_REPLY",
+		     t->debug_id, t->from ? t->from->proc->pid : 0,
+		     t->from ? t->from->pid : 0, cmd,
+		     t->buffer->data_size, t->buffer->offsets_size,
+		     (u64)tr.data.ptr.buffer, (u64)tr.data.ptr.offsets);
+
+	list_del(&t->work.entry);
+	t->buffer->allow_user_free = 1;
+	if (cmd == BR_TRANSACTION && !(t->flags & TF_ONE_WAY)) {
+		t->to_thread = thread;
+		binder_dst_save_transaction(thread, t);
+	} else {
+		t->buffer->transaction = NULL;
+		kfree(t);
+		binder_stats_deleted(BINDER_STAT_TRANSACTION);
+	}
+	return 0;
+}
+
+static int binder_work_node(struct binder_thread *thread,
+			    struct binder_work *w, void * __user *ptr)
+{
+	struct binder_node *node = container_of(w, struct binder_node, work);
+	struct binder_proc *proc = thread->proc;
+	uint32_t cmd = BR_NOOP;
+	const char *cmd_name;
+	int strong = node->internal_strong_refs || node->local_strong_refs;
+	int weak = !hlist_empty(&node->refs) || node->local_weak_refs || strong;
+
+	if (weak && !node->has_weak_ref) {
+		cmd = BR_INCREFS;
+		cmd_name = "BR_INCREFS";
+		node->has_weak_ref = 1;
+		node->pending_weak_ref = 1;
+		node->local_weak_refs++;
+	} else if (strong && !node->has_strong_ref) {
+		cmd = BR_ACQUIRE;
+		cmd_name = "BR_ACQUIRE";
+		node->has_strong_ref = 1;
+		node->pending_strong_ref = 1;
+		node->local_strong_refs++;
+	} else if (!strong && node->has_strong_ref) {
+		cmd = BR_RELEASE;
+		cmd_name = "BR_RELEASE";
+		node->has_strong_ref = 0;
+	} else if (!weak && node->has_weak_ref) {
+		cmd = BR_DECREFS;
+		cmd_name = "BR_DECREFS";
+		node->has_weak_ref = 0;
+	}
+	if (cmd != BR_NOOP) {
+		if (put_user(cmd, (uint32_t __user *)*ptr))
+			return -EFAULT;
+		*ptr += sizeof(uint32_t);
+		if (put_user(node->ptr, (binder_uintptr_t __user *)*ptr))
+			return -EFAULT;
+		*ptr += sizeof(binder_uintptr_t);
+		if (put_user(node->cookie, (binder_uintptr_t __user *)*ptr))
+			return -EFAULT;
+		*ptr += sizeof(binder_uintptr_t);
+
+		binder_stat_br(proc, thread, cmd);
+		binder_debug(BINDER_DEBUG_USER_REFS,
+			     "%d:%d %s %d u%016llx c%016llx\n",
+			     proc->pid, thread->pid, cmd_name, node->debug_id,
+			     (u64)node->ptr, (u64)node->cookie);
+	} else {
+		list_del_init(&w->entry);
+		if (!weak && !strong) {
+			binder_debug(BINDER_DEBUG_INTERNAL_REFS,
+				     "%d:%d node %d u%016llx c%016llx deleted\n",
+				     proc->pid, thread->pid, node->debug_id,
+				     (u64)node->ptr, (u64)node->cookie);
+			rb_erase(&node->rb_node, &proc->nodes);
+			kfree(node);
+			binder_stats_deleted(BINDER_STAT_NODE);
+		} else {
+			binder_debug(BINDER_DEBUG_INTERNAL_REFS,
+				     "%d:%d node %d u%016llx c%016llx state unchanged\n",
+				     proc->pid, thread->pid, node->debug_id,
+				     (u64)node->ptr, (u64)node->cookie);
+		}
+	}
+	return 0;
+}
+
+static int binder_work_dead_binder(struct binder_thread *thread,
+				   struct binder_work *w, void * __user *ptr)
+{
+	struct binder_ref_death *death;
+	struct binder_proc *proc = thread->proc;
+	uint32_t cmd;
+
+	death = container_of(w, struct binder_ref_death, work);
+	if (w->type == BINDER_WORK_CLEAR_DEATH_NOTIFICATION)
+		cmd = BR_CLEAR_DEATH_NOTIFICATION_DONE;
+	else
+		cmd = BR_DEAD_BINDER;
+	if (put_user(cmd, (uint32_t __user *)*ptr))
+		return -EFAULT;
+	*ptr += sizeof(uint32_t);
+	if (put_user(death->cookie, (binder_uintptr_t __user *)*ptr))
+		return -EFAULT;
+	*ptr += sizeof(binder_uintptr_t);
+	binder_stat_br(proc, thread, cmd);
+	binder_debug(BINDER_DEBUG_DEATH_NOTIFICATION, "%d:%d %s %016llx\n",
+		     proc->pid, thread->pid, cmd == BR_DEAD_BINDER ?
+		     "BR_DEAD_BINDER" : "BR_CLEAR_DEATH_NOTIFICATION_DONE",
+		     (u64)death->cookie);
+
+	if (w->type == BINDER_WORK_CLEAR_DEATH_NOTIFICATION) {
+		list_del(&w->entry);
+		kfree(death);
+		binder_stats_deleted(BINDER_STAT_DEATH);
+	} else {
+		list_move(&w->entry, &proc->delivered_death);
+	}
+	return 0;
+}
+
+static int binder_work_tr_complete(struct binder_thread *thread,
+				   struct binder_work *w, void * __user *ptr)
+{
+	uint32_t cmd = BR_TRANSACTION_COMPLETE;
+	struct binder_proc *proc = thread->proc;
+
+	if (put_user(cmd, (uint32_t __user *)*ptr))
+		return -EFAULT;
+	*ptr += sizeof(uint32_t);
+
+	binder_stat_br(proc, thread, cmd);
+	binder_debug(BINDER_DEBUG_TRANSACTION_COMPLETE,
+		     "%d:%d BR_TRANSACTION_COMPLETE\n",
+		     proc->pid, thread->pid);
+
+	list_del(&w->entry);
+	kfree(w);
+	binder_stats_deleted(BINDER_STAT_TRANSACTION_COMPLETE);
+	return 0;
+}
+
 static int binder_thread_read(struct binder_proc *proc,
 			      struct binder_thread *thread,
 			      binder_uintptr_t binder_buffer, size_t size,
@@ -2408,11 +2609,9 @@ retry:
 	if (ret)
 		return ret;
 
-	while (1) {
-		uint32_t cmd;
-		struct binder_transaction_data tr;
+	while (((end - ptr) >= sizeof(struct binder_transaction_data) + 4)) {
 		struct binder_work *w;
-		struct binder_transaction *t = NULL;
+		bool done_processing_work = false;
 
 		if (!list_empty(&thread->todo)) {
 			w = list_first_entry(&thread->todo, struct binder_work,
@@ -2428,209 +2627,39 @@ retry:
 			break;
 		}
 
-		if (end - ptr < sizeof(tr) + 4)
-			break;
-
 		switch (w->type) {
-		case BINDER_WORK_TRANSACTION: {
-			t = container_of(w, struct binder_transaction, work);
-		} break;
-		case BINDER_WORK_TRANSACTION_COMPLETE: {
-			cmd = BR_TRANSACTION_COMPLETE;
-			if (put_user(cmd, (uint32_t __user *)ptr))
-				return -EFAULT;
-			ptr += sizeof(uint32_t);
-
-			binder_stat_br(proc, thread, cmd);
-			binder_debug(BINDER_DEBUG_TRANSACTION_COMPLETE,
-				     "%d:%d BR_TRANSACTION_COMPLETE\n",
-				     proc->pid, thread->pid);
-
-			list_del(&w->entry);
-			kfree(w);
-			binder_stats_deleted(BINDER_STAT_TRANSACTION_COMPLETE);
-		} break;
-		case BINDER_WORK_NODE: {
-			struct binder_node *node = container_of(w, struct binder_node, work);
-			uint32_t cmd = BR_NOOP;
-			const char *cmd_name;
-			int strong = node->internal_strong_refs || node->local_strong_refs;
-			int weak = !hlist_empty(&node->refs) || node->local_weak_refs || strong;
-
-			if (weak && !node->has_weak_ref) {
-				cmd = BR_INCREFS;
-				cmd_name = "BR_INCREFS";
-				node->has_weak_ref = 1;
-				node->pending_weak_ref = 1;
-				node->local_weak_refs++;
-			} else if (strong && !node->has_strong_ref) {
-				cmd = BR_ACQUIRE;
-				cmd_name = "BR_ACQUIRE";
-				node->has_strong_ref = 1;
-				node->pending_strong_ref = 1;
-				node->local_strong_refs++;
-			} else if (!strong && node->has_strong_ref) {
-				cmd = BR_RELEASE;
-				cmd_name = "BR_RELEASE";
-				node->has_strong_ref = 0;
-			} else if (!weak && node->has_weak_ref) {
-				cmd = BR_DECREFS;
-				cmd_name = "BR_DECREFS";
-				node->has_weak_ref = 0;
-			}
-			if (cmd != BR_NOOP) {
-				if (put_user(cmd, (uint32_t __user *)ptr))
-					return -EFAULT;
-				ptr += sizeof(uint32_t);
-				if (put_user(node->ptr,
-					     (binder_uintptr_t __user *)ptr))
-					return -EFAULT;
-				ptr += sizeof(binder_uintptr_t);
-				if (put_user(node->cookie,
-					     (binder_uintptr_t __user *)ptr))
-					return -EFAULT;
-				ptr += sizeof(binder_uintptr_t);
-
-				binder_stat_br(proc, thread, cmd);
-				binder_debug(BINDER_DEBUG_USER_REFS,
-					     "%d:%d %s %d u%016llx c%016llx\n",
-					     proc->pid, thread->pid, cmd_name,
-					     node->debug_id,
-					     (u64)node->ptr, (u64)node->cookie);
-			} else {
-				list_del_init(&w->entry);
-				if (!weak && !strong) {
-					binder_debug(BINDER_DEBUG_INTERNAL_REFS,
-						     "%d:%d node %d u%016llx c%016llx deleted\n",
-						     proc->pid, thread->pid,
-						     node->debug_id,
-						     (u64)node->ptr,
-						     (u64)node->cookie);
-					rb_erase(&node->rb_node, &proc->nodes);
-					kfree(node);
-					binder_stats_deleted(BINDER_STAT_NODE);
-				} else {
-					binder_debug(BINDER_DEBUG_INTERNAL_REFS,
-						     "%d:%d node %d u%016llx c%016llx state unchanged\n",
-						     proc->pid, thread->pid,
-						     node->debug_id,
-						     (u64)node->ptr,
-						     (u64)node->cookie);
-				}
-			}
-		} break;
+		case BINDER_WORK_TRANSACTION:
+			ret = binder_work_transaction(thread, w, &ptr);
+			/*
+			 * Threads can only handle one incoming transaction,
+			 * at a time, so return before processing any more
+			 * transaction work nodes.
+			 */
+			done_processing_work = true;
+			break;
+		case BINDER_WORK_TRANSACTION_COMPLETE:
+			ret = binder_work_tr_complete(thread, w, &ptr);
+			break;
+		case BINDER_WORK_NODE:
+			ret = binder_work_node(thread, w, &ptr);
+			break;
 		case BINDER_WORK_DEAD_BINDER:
 		case BINDER_WORK_DEAD_BINDER_AND_CLEAR:
-		case BINDER_WORK_CLEAR_DEATH_NOTIFICATION: {
-			struct binder_ref_death *death;
-			uint32_t cmd;
-
-			death = container_of(w, struct binder_ref_death, work);
-			if (w->type == BINDER_WORK_CLEAR_DEATH_NOTIFICATION)
-				cmd = BR_CLEAR_DEATH_NOTIFICATION_DONE;
-			else
-				cmd = BR_DEAD_BINDER;
-			if (put_user(cmd, (uint32_t __user *)ptr))
-				return -EFAULT;
-			ptr += sizeof(uint32_t);
-			if (put_user(death->cookie,
-				     (binder_uintptr_t __user *)ptr))
-				return -EFAULT;
-			ptr += sizeof(binder_uintptr_t);
-			binder_stat_br(proc, thread, cmd);
-			binder_debug(BINDER_DEBUG_DEATH_NOTIFICATION,
-				     "%d:%d %s %016llx\n",
-				      proc->pid, thread->pid,
-				      cmd == BR_DEAD_BINDER ?
-				      "BR_DEAD_BINDER" :
-				      "BR_CLEAR_DEATH_NOTIFICATION_DONE",
-				      (u64)death->cookie);
-
-			if (w->type == BINDER_WORK_CLEAR_DEATH_NOTIFICATION) {
-				list_del(&w->entry);
-				kfree(death);
-				binder_stats_deleted(BINDER_STAT_DEATH);
-			} else
-				list_move(&w->entry, &proc->delivered_death);
-			if (cmd == BR_DEAD_BINDER)
-				goto done; /* DEAD_BINDER notifications can cause transactions */
-		} break;
-		}
-
-		if (!t)
-			continue;
-
-		BUG_ON(t->buffer == NULL);
-		if (t->buffer->target_node) {
-			struct binder_node *target_node = t->buffer->target_node;
-
-			tr.target.ptr = target_node->ptr;
-			tr.cookie =  target_node->cookie;
-			t->saved_priority = task_nice(current);
-			if (t->priority < target_node->min_priority &&
-			    !(t->flags & TF_ONE_WAY))
-				binder_set_nice(t->priority);
-			else if (!(t->flags & TF_ONE_WAY) ||
-				 t->saved_priority > target_node->min_priority)
-				binder_set_nice(target_node->min_priority);
-			cmd = BR_TRANSACTION;
-		} else {
-			tr.target.ptr = 0;
-			tr.cookie = 0;
-			cmd = BR_REPLY;
-		}
-		tr.code = t->code;
-		tr.flags = t->flags;
-		tr.sender_euid = from_kuid(current_user_ns(), t->sender_euid);
-
-		if (t->from) {
-			struct task_struct *sender = t->from->proc->tsk;
-
-			tr.sender_pid = task_tgid_nr_ns(sender,
-							task_active_pid_ns(current));
-		} else {
-			tr.sender_pid = 0;
+			/*
+			 * Dead binder notifications can cause userspace to
+			 * issue transactions, so break out here so that only
+			 * one notification is given to userspace at a time.
+			 */
+			done_processing_work = true;
+		case BINDER_WORK_CLEAR_DEATH_NOTIFICATION:
+			ret = binder_work_dead_binder(thread, w, &ptr);
+			break;
 		}
+		if (ret < 0)
+			return ret;
 
-		tr.data_size = t->buffer->data_size;
-		tr.offsets_size = t->buffer->offsets_size;
-		tr.data.ptr.buffer = (binder_uintptr_t)(
-					(uintptr_t)t->buffer->data +
-					proc->user_buffer_offset);
-		tr.data.ptr.offsets = tr.data.ptr.buffer +
-					ALIGN(t->buffer->data_size,
-					    sizeof(void *));
-
-		if (put_user(cmd, (uint32_t __user *)ptr))
-			return -EFAULT;
-		ptr += sizeof(uint32_t);
-		if (copy_to_user(ptr, &tr, sizeof(tr)))
-			return -EFAULT;
-		ptr += sizeof(tr);
-
-		trace_binder_transaction_received(t);
-		binder_stat_br(proc, thread, cmd);
-		binder_debug(BINDER_DEBUG_TRANSACTION,
-			     "%d:%d %s %d %d:%d, cmd %d size %zd-%zd ptr %016llx-%016llx\n",
-			     proc->pid, thread->pid,
-			     (cmd == BR_TRANSACTION) ? "BR_TRANSACTION" :
-			     "BR_REPLY",
-			     t->debug_id, t->from ? t->from->proc->pid : 0,
-			     t->from ? t->from->pid : 0, cmd,
-			     t->buffer->data_size, t->buffer->offsets_size,
-			     (u64)tr.data.ptr.buffer, (u64)tr.data.ptr.offsets);
-
-		list_del(&t->work.entry);
-		t->buffer->allow_user_free = 1;
-		if (cmd == BR_TRANSACTION && !(t->flags & TF_ONE_WAY)) {
-			t->to_thread = thread;
-			binder_dst_save_transaction(thread, t);
-		} else {
-			t->buffer->transaction = NULL;
-			kfree(t);
-			binder_stats_deleted(BINDER_STAT_TRANSACTION);
-		}
-		break;
+		if (done_processing_work)
+			break;
 	}
 
 done:
-- 
2.2.0.rc0.207.ga3a616c

--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/