[PATCH 1/2] staging: android: binder: remove unnecessary comment

[PATCH 1/2] staging: android: binder: remove unnecessary comment

[SeongJae Park]

Signed-off-by: SeongJae Park <sj38.park@gmail.com>
---
 drivers/staging/android/binder.c | 2 --
 1 file changed, 2 deletions(-)

diff --git a/drivers/staging/android/binder.c b/drivers/staging/android/binder.c
index eaec1da..b23cbc9 100644
--- a/drivers/staging/android/binder.c
+++ b/drivers/staging/android/binder.c
@@ -2553,8 +2553,6 @@ static long binder_ioctl(struct file *filp, unsigned int cmd, unsigned long arg)
 	unsigned int size = _IOC_SIZE(cmd);
 	void __user *ubuf = (void __user *)arg;
 
-	/*pr_info("binder_ioctl: %d:%d %x %lx\n", proc->pid, current->pid, cmd, arg);*/
-
 	trace_binder_ioctl(cmd, arg);
 
 	ret = wait_event_interruptible(binder_user_error_wait, binder_stop_on_user_error < 2);
-- 
1.8.3.2

[PATCH 2/2] staging: android: binder: use stack for locally used variable

[SeongJae Park]

The variable `binder_debugfs_dir_entry_root` is declared as static
global variable although it is accessed from init function only. Declare
it as init function's local variable because it would be better to read
and memory efficiency.

Signed-off-by: SeongJae Park <sj38.park@gmail.com>
---
 drivers/staging/android/binder.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/staging/android/binder.c b/drivers/staging/android/binder.c
index b23cbc9..78de730 100644
--- a/drivers/staging/android/binder.c
+++ b/drivers/staging/android/binder.c
@@ -49,7 +49,6 @@ static HLIST_HEAD(binder_procs);
 static HLIST_HEAD(binder_deferred_list);
 static HLIST_HEAD(binder_dead_nodes);
 
-static struct dentry *binder_debugfs_dir_entry_root;
 static struct dentry *binder_debugfs_dir_entry_proc;
 static struct binder_node *binder_context_mgr_node;
 static kuid_t binder_context_mgr_uid = INVALID_UID;
@@ -3514,6 +3513,7 @@ BINDER_DEBUG_ENTRY(transaction_log);
 
 static int __init binder_init(void)
 {
+	struct dentry *binder_debugfs_dir_entry_root;
 	int ret;
 
 	binder_deferred_workqueue = create_singlethread_workqueue("binder");
-- 
1.8.3.2

Re: [PATCH 2/2] staging: android: binder: use stack for locally used variable

[Greg KH]

On Tue, Feb 18, 2014 at 08:23:26PM +0900, SeongJae Park wrote:
> The variable `binder_debugfs_dir_entry_root` is declared as static
> global variable although it is accessed from init function only. Declare
> it as init function's local variable because it would be better to read
> and memory efficiency.

Are you sure?  You now just lost that pointer and it can never be freed.

Not that it ever was, but before it was obvious as to what would need to
be done if the module could ever be made to be unloaded...

As it is, this code is fine, I can't take this patch.

greg k-h

Re: [PATCH 1/2] staging: android: binder: remove unnecessary comment

[Greg KH]

On Tue, Feb 18, 2014 at 08:23:25PM +0900, SeongJae Park wrote:
> Signed-off-by: SeongJae Park <sj38.park@gmail.com>
> ---
>  drivers/staging/android/binder.c | 2 --
>  1 file changed, 2 deletions(-)
> 
> diff --git a/drivers/staging/android/binder.c b/drivers/staging/android/binder.c
> index eaec1da..b23cbc9 100644
> --- a/drivers/staging/android/binder.c
> +++ b/drivers/staging/android/binder.c
> @@ -2553,8 +2553,6 @@ static long binder_ioctl(struct file *filp, unsigned int cmd, unsigned long arg)
>  	unsigned int size = _IOC_SIZE(cmd);
>  	void __user *ubuf = (void __user *)arg;
>  
> -	/*pr_info("binder_ioctl: %d:%d %x %lx\n", proc->pid, current->pid, cmd, arg);*/

It's useful for debugging, I'll leave it as-is, sorry.

Re: [PATCH 1/2] staging: android: binder: remove unnecessary comment

[SeongJae Park]

On Wed, Feb 19, 2014 at 2:07 AM, Greg KH <gregkh@linuxfoundation.org> wrote:
> On Tue, Feb 18, 2014 at 08:23:25PM +0900, SeongJae Park wrote:
>> Signed-off-by: SeongJae Park <sj38.park@gmail.com>
>> ---
>>  drivers/staging/android/binder.c | 2 --
>>  1 file changed, 2 deletions(-)
>>
>> diff --git a/drivers/staging/android/binder.c b/drivers/staging/android/binder.c
>> index eaec1da..b23cbc9 100644
>> --- a/drivers/staging/android/binder.c
>> +++ b/drivers/staging/android/binder.c
>> @@ -2553,8 +2553,6 @@ static long binder_ioctl(struct file *filp, unsigned int cmd, unsigned long arg)
>>       unsigned int size = _IOC_SIZE(cmd);
>>       void __user *ubuf = (void __user *)arg;
>>
>> -     /*pr_info("binder_ioctl: %d:%d %x %lx\n", proc->pid, current->pid, cmd, arg);*/
>
> It's useful for debugging, I'll leave it as-is, sorry.

Agree, thank you for quick replying.

Re: [PATCH 2/2] staging: android: binder: use stack for locally used variable

[SeongJae Park]

Hello,

On Wed, Feb 19, 2014 at 2:07 AM, Greg KH <gregkh@linuxfoundation.org> wrote:
> On Tue, Feb 18, 2014 at 08:23:26PM +0900, SeongJae Park wrote:
>> The variable `binder_debugfs_dir_entry_root` is declared as static
>> global variable although it is accessed from init function only. Declare
>> it as init function's local variable because it would be better to read
>> and memory efficiency.
>
> Are you sure?  You now just lost that pointer and it can never be freed.

Oops, you are right.

>
> Not that it ever was, but before it was obvious as to what would need to
> be done if the module could ever be made to be unloaded...
>
> As it is, this code is fine, I can't take this patch.

I agree. Thank you.

>
> greg k-h

Re: [PATCH 1/2] staging: android: binder: remove unnecessary comment

[Sachin Kamat]

On 19 February 2014 05:58, SeongJae Park <sj38.park@gmail.com> wrote:
> On Wed, Feb 19, 2014 at 2:07 AM, Greg KH <gregkh@linuxfoundation.org> wrote:
>> On Tue, Feb 18, 2014 at 08:23:25PM +0900, SeongJae Park wrote:
>>> Signed-off-by: SeongJae Park <sj38.park@gmail.com>
>>> ---
>>>  drivers/staging/android/binder.c | 2 --
>>>  1 file changed, 2 deletions(-)
>>>
>>> diff --git a/drivers/staging/android/binder.c b/drivers/staging/android/binder.c
>>> index eaec1da..b23cbc9 100644
>>> --- a/drivers/staging/android/binder.c
>>> +++ b/drivers/staging/android/binder.c
>>> @@ -2553,8 +2553,6 @@ static long binder_ioctl(struct file *filp, unsigned int cmd, unsigned long arg)
>>>       unsigned int size = _IOC_SIZE(cmd);
>>>       void __user *ubuf = (void __user *)arg;
>>>
>>> -     /*pr_info("binder_ioctl: %d:%d %x %lx\n", proc->pid, current->pid, cmd, arg);*/
>>
>> It's useful for debugging, I'll leave it as-is, sorry.

Or just convert pr_info to pr_debug and leave uncommented?

-- 
With warm regards,
Sachin

Re: [PATCH 1/2] staging: android: binder: remove unnecessary comment

[SeongJae Park]

On Wed, Feb 19, 2014 at 12:04 PM, Sachin Kamat <sachin.kamat@linaro.org> wrote:
> On 19 February 2014 05:58, SeongJae Park <sj38.park@gmail.com> wrote:
>> On Wed, Feb 19, 2014 at 2:07 AM, Greg KH <gregkh@linuxfoundation.org> wrote:
>>> On Tue, Feb 18, 2014 at 08:23:25PM +0900, SeongJae Park wrote:
>>>> Signed-off-by: SeongJae Park <sj38.park@gmail.com>
>>>> ---
>>>>  drivers/staging/android/binder.c | 2 --
>>>>  1 file changed, 2 deletions(-)
>>>>
>>>> diff --git a/drivers/staging/android/binder.c b/drivers/staging/android/binder.c
>>>> index eaec1da..b23cbc9 100644
>>>> --- a/drivers/staging/android/binder.c
>>>> +++ b/drivers/staging/android/binder.c
>>>> @@ -2553,8 +2553,6 @@ static long binder_ioctl(struct file *filp, unsigned int cmd, unsigned long arg)
>>>>       unsigned int size = _IOC_SIZE(cmd);
>>>>       void __user *ubuf = (void __user *)arg;
>>>>
>>>> -     /*pr_info("binder_ioctl: %d:%d %x %lx\n", proc->pid, current->pid, cmd, arg);*/
>>>
>>> It's useful for debugging, I'll leave it as-is, sorry.
>
> Or just convert pr_info to pr_debug and leave uncommented?

Sounds much better. Thank you for suggestion.
I will send patch with your recommend again soon.

>
> --
> With warm regards,
> Sachin