(no subject)

[Pranay Kr. Srivastava]

Sending both patches as series instead. Made the required changes as
suggessted in earlier versions and fixed the warnings reported from
kbuild test robot.

[PATCH-SPEAKUP 1/2] return same error value from spk_set_key_info

[Pranay Kr. Srivastava]

This patch makes spk_set_key_info return -EINVAL
in case of failure instead of returning 4 different
values for the type of error that occurred.

Print the offending values instead as debug message.

Signed-off-by: Pranay Kr. Srivastava <pranjas@gmail.com>
---
 drivers/staging/speakup/main.c | 27 +++++++++++++++++++--------
 1 file changed, 19 insertions(+), 8 deletions(-)

diff --git a/drivers/staging/speakup/main.c b/drivers/staging/speakup/main.c
index c2f70ef..6667cf2 100644
--- a/drivers/staging/speakup/main.c
+++ b/drivers/staging/speakup/main.c
@@ -1216,13 +1216,19 @@ int spk_set_key_info(const u_char *key_info, u_char *k_buffer)
 	u_char ch, version, num_keys;
 
 	version = *cp++;
-	if (version != KEY_MAP_VER)
-		return -1;
+	if (version != KEY_MAP_VER) {
+		pr_debug("version found %d should be %d\n",
+			 version, KEY_MAP_VER);
+		return -EINVAL;
+	}
 	num_keys = *cp;
 	states = (int)cp[1];
 	key_data_len = (states + 1) * (num_keys + 1);
-	if (key_data_len + SHIFT_TBL_SIZE + 4 >= sizeof(spk_key_buf))
-		return -2;
+	if (key_data_len + SHIFT_TBL_SIZE + 4 >= sizeof(spk_key_buf)) {
+		pr_debug("too many key_infos (%d over %lu)\n",
+			 key_data_len + SHIFT_TBL_SIZE + 4, sizeof(spk_key_buf));
+		return -EINVAL;
+	}
 	memset(k_buffer, 0, SHIFT_TBL_SIZE);
 	memset(spk_our_keys, 0, sizeof(spk_our_keys));
 	spk_shift_table = k_buffer;
@@ -1233,14 +1239,19 @@ int spk_set_key_info(const u_char *key_info, u_char *k_buffer)
 	cp1 += 2;		/* now pointing at shift states */
 	for (i = 1; i <= states; i++) {
 		ch = *cp1++;
-		if (ch >= SHIFT_TBL_SIZE)
-			return -3;
+		if (ch >= SHIFT_TBL_SIZE) {
+			pr_debug("(%d) not valid shift state (max_allowed = %d)\n", ch,
+				 SHIFT_TBL_SIZE);
+			return -EINVAL;
+		}
 		spk_shift_table[ch] = i;
 	}
 	keymap_flags = *cp1++;
 	while ((ch = *cp1)) {
-		if (ch >= MAX_KEY)
-			return -4;
+		if (ch >= MAX_KEY) {
+			pr_debug("(%d), not valid key, (max_allowed = %d)\n", ch, MAX_KEY);
+			return -EINVAL;
+		}
 		spk_our_keys[ch] = cp1;
 		cp1 += states + 1;
 	}
-- 
2.10.2

Re: [PATCH-SPEAKUP 1/2] return same error value from spk_set_key_info

[Samuel Thibault]

Pranay Kr. Srivastava, on mer. 22 févr. 2017 12:50:26 +0530, wrote:
> This patch makes spk_set_key_info return -EINVAL
> in case of failure instead of returning 4 different
> values for the type of error that occurred.
> 
> Print the offending values instead as debug message.
> 
> Signed-off-by: Pranay Kr. Srivastava <pranjas@gmail.com>

Reviewed-by: Samuel Thibault <samuel.thibault@ens-lyon.org>

> ---
>  drivers/staging/speakup/main.c | 27 +++++++++++++++++++--------
>  1 file changed, 19 insertions(+), 8 deletions(-)
> 
> diff --git a/drivers/staging/speakup/main.c b/drivers/staging/speakup/main.c
> index c2f70ef..6667cf2 100644
> --- a/drivers/staging/speakup/main.c
> +++ b/drivers/staging/speakup/main.c
> @@ -1216,13 +1216,19 @@ int spk_set_key_info(const u_char *key_info, u_char *k_buffer)
>  	u_char ch, version, num_keys;
>  
>  	version = *cp++;
> -	if (version != KEY_MAP_VER)
> -		return -1;
> +	if (version != KEY_MAP_VER) {
> +		pr_debug("version found %d should be %d\n",
> +			 version, KEY_MAP_VER);
> +		return -EINVAL;
> +	}
>  	num_keys = *cp;
>  	states = (int)cp[1];
>  	key_data_len = (states + 1) * (num_keys + 1);
> -	if (key_data_len + SHIFT_TBL_SIZE + 4 >= sizeof(spk_key_buf))
> -		return -2;
> +	if (key_data_len + SHIFT_TBL_SIZE + 4 >= sizeof(spk_key_buf)) {
> +		pr_debug("too many key_infos (%d over %lu)\n",
> +			 key_data_len + SHIFT_TBL_SIZE + 4, sizeof(spk_key_buf));
> +		return -EINVAL;
> +	}
>  	memset(k_buffer, 0, SHIFT_TBL_SIZE);
>  	memset(spk_our_keys, 0, sizeof(spk_our_keys));
>  	spk_shift_table = k_buffer;
> @@ -1233,14 +1239,19 @@ int spk_set_key_info(const u_char *key_info, u_char *k_buffer)
>  	cp1 += 2;		/* now pointing at shift states */
>  	for (i = 1; i <= states; i++) {
>  		ch = *cp1++;
> -		if (ch >= SHIFT_TBL_SIZE)
> -			return -3;
> +		if (ch >= SHIFT_TBL_SIZE) {
> +			pr_debug("(%d) not valid shift state (max_allowed = %d)\n", ch,
> +				 SHIFT_TBL_SIZE);
> +			return -EINVAL;
> +		}
>  		spk_shift_table[ch] = i;
>  	}
>  	keymap_flags = *cp1++;
>  	while ((ch = *cp1)) {
> -		if (ch >= MAX_KEY)
> -			return -4;
> +		if (ch >= MAX_KEY) {
> +			pr_debug("(%d), not valid key, (max_allowed = %d)\n", ch, MAX_KEY);
> +			return -EINVAL;
> +		}
>  		spk_our_keys[ch] = cp1;
>  		cp1 += states + 1;
>  	}
> -- 
> 2.10.2
> 

-- 
Samuel
<b> j'en ai parlé à xavier, il n'est pas interdit qu'il le change un jour
 -+- #sos - a le bras long vers le chameau -+-

Re: [PATCH-SPEAKUP 1/2] return same error value from spk_set_key_info

[kbuild test robot]

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

Hi Pranay,

[auto build test WARNING on staging/staging-testing]
[also build test WARNING on v4.10]
[if your patch is applied to the wrong git tree, please drop us a note to help improve the system]

url:    https://github.com/0day-ci/linux/commits/Pranay-Kr-Srivastava/return-same-error-value-from-spk_set_key_info/20170222-152440
config: i386-randconfig-c0-02242210 (attached as .config)
compiler: gcc-4.9 (Debian 4.9.4-2) 4.9.4
reproduce:
        # save the attached .config to linux build tree
        make ARCH=i386 

All warnings (new ones prefixed by >>):

   In file included from include/linux/dynamic_debug.h:5:0,
                    from include/linux/printk.h:320,
                    from include/linux/kernel.h:13,
                    from drivers/staging/speakup/main.c:21:
   drivers/staging/speakup/main.c: In function 'spk_set_key_info':
>> include/linux/jump_label.h:377:59: warning: format '%lu' expects argument of type 'long unsigned int', but argument 4 has type 'unsigned int' [-Wformat=]
     else if (__builtin_types_compatible_p(typeof(*x), struct static_key_false)) \
                                                              ^
>> include/linux/dynamic_debug.h:102:2: note: in expansion of macro 'static_branch_unlikely'
     static_branch_unlikely(&descriptor.key.dd_key_false)
     ^
>> include/linux/dynamic_debug.h:125:6: note: in expansion of macro 'DYNAMIC_DEBUG_BRANCH'
     if (DYNAMIC_DEBUG_BRANCH(descriptor))   \
         ^
   include/linux/printk.h:324:2: note: in expansion of macro 'dynamic_pr_debug'
     dynamic_pr_debug(fmt, ##__VA_ARGS__)
     ^
   drivers/staging/speakup/main.c:1228:3: note: in expansion of macro 'pr_debug'
      pr_debug("too many key_infos (%d over %lu)\n",
      ^

vim +377 include/linux/jump_label.h

11276d53 Peter Zijlstra 2015-07-24  361  ({										\
11276d53 Peter Zijlstra 2015-07-24  362  	bool branch;								\
11276d53 Peter Zijlstra 2015-07-24  363  	if (__builtin_types_compatible_p(typeof(*x), struct static_key_true))	\
11276d53 Peter Zijlstra 2015-07-24  364  		branch = !arch_static_branch(&(x)->key, true);			\
11276d53 Peter Zijlstra 2015-07-24  365  	else if (__builtin_types_compatible_p(typeof(*x), struct static_key_false)) \
11276d53 Peter Zijlstra 2015-07-24  366  		branch = !arch_static_branch_jump(&(x)->key, true);		\
11276d53 Peter Zijlstra 2015-07-24  367  	else									\
11276d53 Peter Zijlstra 2015-07-24  368  		branch = ____wrong_branch_error();				\
11276d53 Peter Zijlstra 2015-07-24  369  	branch;									\
11276d53 Peter Zijlstra 2015-07-24  370  })
11276d53 Peter Zijlstra 2015-07-24  371  
11276d53 Peter Zijlstra 2015-07-24  372  #define static_branch_unlikely(x)						\
11276d53 Peter Zijlstra 2015-07-24  373  ({										\
11276d53 Peter Zijlstra 2015-07-24  374  	bool branch;								\
11276d53 Peter Zijlstra 2015-07-24  375  	if (__builtin_types_compatible_p(typeof(*x), struct static_key_true))	\
11276d53 Peter Zijlstra 2015-07-24  376  		branch = arch_static_branch_jump(&(x)->key, false);		\
11276d53 Peter Zijlstra 2015-07-24 @377  	else if (__builtin_types_compatible_p(typeof(*x), struct static_key_false)) \
11276d53 Peter Zijlstra 2015-07-24  378  		branch = arch_static_branch(&(x)->key, false);			\
11276d53 Peter Zijlstra 2015-07-24  379  	else									\
11276d53 Peter Zijlstra 2015-07-24  380  		branch = ____wrong_branch_error();				\
11276d53 Peter Zijlstra 2015-07-24  381  	branch;									\
11276d53 Peter Zijlstra 2015-07-24  382  })
11276d53 Peter Zijlstra 2015-07-24  383  
11276d53 Peter Zijlstra 2015-07-24  384  #else /* !HAVE_JUMP_LABEL */
11276d53 Peter Zijlstra 2015-07-24  385  

:::::: The code at line 377 was first introduced by commit
:::::: 11276d5306b8e5b438a36bbff855fe792d7eaa61 locking/static_keys: Add a new static_key interface

:::::: TO: Peter Zijlstra <peterz@infradead.org>
:::::: CC: Ingo Molnar <mingo@kernel.org>

---
0-DAY kernel test infrastructure                Open Source Technology Center
https://lists.01.org/pipermail/kbuild-all                   Intel Corporation

[-- Attachment #2: .config.gz --]
[-- Type: application/gzip, Size: 26365 bytes --]

Re: [PATCH-SPEAKUP 1/2] return same error value from spk_set_key_info

[Greg KH]

On Wed, Feb 22, 2017 at 12:50:26PM +0530, Pranay Kr. Srivastava wrote:
> This patch makes spk_set_key_info return -EINVAL
> in case of failure instead of returning 4 different
> values for the type of error that occurred.
> 
> Print the offending values instead as debug message.
> 
> Signed-off-by: Pranay Kr. Srivastava <pranjas@gmail.com>
> ---
>  drivers/staging/speakup/main.c | 27 +++++++++++++++++++--------
>  1 file changed, 19 insertions(+), 8 deletions(-)

Please fix the build errors and resend this series.

thanks,

greg k-h

Re: [PATCH-SPEAKUP 1/2] return same error value from spk_set_key_info

[kbuild test robot]

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

Hi Pranay,

[auto build test WARNING on staging/staging-testing]
[also build test WARNING on v4.10 next-20170228]
[if your patch is applied to the wrong git tree, please drop us a note to help improve the system]

url:    https://github.com/0day-ci/linux/commits/Pranay-Kr-Srivastava/return-same-error-value-from-spk_set_key_info/20170222-152440
config: i386-randconfig-c0-02281459 (attached as .config)
compiler: gcc-4.9 (Debian 4.9.4-2) 4.9.4
reproduce:
        # save the attached .config to linux build tree
        make ARCH=i386 

All warnings (new ones prefixed by >>):

   In file included from include/linux/linkage.h:4:0,
                    from include/linux/kernel.h:6,
                    from drivers/staging/speakup/main.c:21:
   drivers/staging/speakup/main.c: In function 'spk_set_key_info':
>> include/linux/compiler.h:117:18: warning: format '%lu' expects argument of type 'long unsigned int', but argument 4 has type 'unsigned int' [-Wformat=]
       static struct ftrace_branch_data  \
                     ^
   include/linux/compiler.h:160:16: note: in definition of macro '__trace_if'
      ______r = !!(cond);     \
                   ^
   include/linux/dynamic_debug.h:125:2: note: in expansion of macro 'if'
     if (DYNAMIC_DEBUG_BRANCH(descriptor))   \
     ^
   include/linux/compiler.h:139:58: note: in expansion of macro '__branch_check__'
    #  define unlikely(x) (__builtin_constant_p(x) ? !!(x) : __branch_check__(x, 0))
                                                             ^
   include/linux/dynamic_debug.h:117:2: note: in expansion of macro 'unlikely'
     unlikely(descriptor.flags & _DPRINTK_FLAGS_PRINT)
     ^
   include/linux/dynamic_debug.h:125:6: note: in expansion of macro 'DYNAMIC_DEBUG_BRANCH'
     if (DYNAMIC_DEBUG_BRANCH(descriptor))   \
         ^
   include/linux/printk.h:324:2: note: in expansion of macro 'dynamic_pr_debug'
     dynamic_pr_debug(fmt, ##__VA_ARGS__)
     ^
   drivers/staging/speakup/main.c:1228:3: note: in expansion of macro 'pr_debug'
      pr_debug("too many key_infos (%d over %lu)\n",
      ^

vim +117 include/linux/compiler.h

2bcd521a Steven Rostedt  2008-11-21  101  	};
2bcd521a Steven Rostedt  2008-11-21  102  };
2ed84eeb Steven Rostedt  2008-11-12  103  
2ed84eeb Steven Rostedt  2008-11-12  104  /*
2ed84eeb Steven Rostedt  2008-11-12  105   * Note: DISABLE_BRANCH_PROFILING can be used by special lowlevel code
2ed84eeb Steven Rostedt  2008-11-12  106   * to disable branch tracing on a per file basis.
2ed84eeb Steven Rostedt  2008-11-12  107   */
d9ad8bc0 Bart Van Assche 2009-04-05  108  #if defined(CONFIG_TRACE_BRANCH_PROFILING) \
d9ad8bc0 Bart Van Assche 2009-04-05  109      && !defined(DISABLE_BRANCH_PROFILING) && !defined(__CHECKER__)
2ed84eeb Steven Rostedt  2008-11-12  110  void ftrace_likely_update(struct ftrace_branch_data *f, int val, int expect);
1f0d69a9 Steven Rostedt  2008-11-12  111  
1f0d69a9 Steven Rostedt  2008-11-12  112  #define likely_notrace(x)	__builtin_expect(!!(x), 1)
1f0d69a9 Steven Rostedt  2008-11-12  113  #define unlikely_notrace(x)	__builtin_expect(!!(x), 0)
1f0d69a9 Steven Rostedt  2008-11-12  114  
45b79749 Steven Rostedt  2008-11-21  115  #define __branch_check__(x, expect) ({					\
1f0d69a9 Steven Rostedt  2008-11-12  116  			int ______r;					\
2ed84eeb Steven Rostedt  2008-11-12 @117  			static struct ftrace_branch_data		\
1f0d69a9 Steven Rostedt  2008-11-12  118  				__attribute__((__aligned__(4)))		\
45b79749 Steven Rostedt  2008-11-21  119  				__attribute__((section("_ftrace_annotated_branch"))) \
1f0d69a9 Steven Rostedt  2008-11-12  120  				______f = {				\
1f0d69a9 Steven Rostedt  2008-11-12  121  				.func = __func__,			\
1f0d69a9 Steven Rostedt  2008-11-12  122  				.file = __FILE__,			\
1f0d69a9 Steven Rostedt  2008-11-12  123  				.line = __LINE__,			\
1f0d69a9 Steven Rostedt  2008-11-12  124  			};						\
1f0d69a9 Steven Rostedt  2008-11-12  125  			______r = likely_notrace(x);			\

:::::: The code at line 117 was first introduced by commit
:::::: 2ed84eeb8808cf3c9f039213ca137ffd7d753f0e trace: rename unlikely profiler to branch profiler

:::::: TO: Steven Rostedt <srostedt@redhat.com>
:::::: CC: Ingo Molnar <mingo@elte.hu>

---
0-DAY kernel test infrastructure                Open Source Technology Center
https://lists.01.org/pipermail/kbuild-all                   Intel Corporation

[-- Attachment #2: .config.gz --]
[-- Type: application/gzip, Size: 27857 bytes --]

[PATCH-SPEAKUP 2/2] remove unecessary initial allocation of vc

[Pranay Kr. Srivastava]

This patch removes the unnecessary allocation of
current foreground vc during initialization.

This initialization is already handled in the loop
that follows it for all available virtual consoles.

Also change the prototype of speakup_allocate to take
extra argument of gfp_* flags. Thus not requiring
GFP_ATOMIC during initialization.

Signed-off-by: Pranay Kr. Srivastava <pranjas@gmail.com>
---
 drivers/staging/speakup/main.c | 19 ++++---------------
 1 file changed, 4 insertions(+), 15 deletions(-)

diff --git a/drivers/staging/speakup/main.c b/drivers/staging/speakup/main.c
index 6667cf2..f81a936 100644
--- a/drivers/staging/speakup/main.c
+++ b/drivers/staging/speakup/main.c
@@ -1327,14 +1327,14 @@ static int edit_bits(struct vc_data *vc, u_char type, u_char ch, u_short key)
 }
 
 /* Allocation concurrency is protected by the console semaphore */
-static int speakup_allocate(struct vc_data *vc)
+static int speakup_allocate(struct vc_data *vc, gfp_t gfp_flags)
 {
 	int vc_num;
 
 	vc_num = vc->vc_num;
 	if (speakup_console[vc_num] == NULL) {
 		speakup_console[vc_num] = kzalloc(sizeof(*speakup_console[0]),
-						  GFP_ATOMIC);
+						  gfp_flags);
 		if (speakup_console[vc_num] == NULL)
 			return -ENOMEM;
 		speakup_date(vc);
@@ -2257,7 +2257,7 @@ static int vt_notifier_call(struct notifier_block *nb,
 	switch (code) {
 	case VT_ALLOCATE:
 		if (vc->vc_mode == KD_TEXT)
-			speakup_allocate(vc);
+			speakup_allocate(vc, GFP_ATOMIC);
 		break;
 	case VT_DEALLOCATE:
 		speakup_deallocate(vc);
@@ -2317,7 +2317,6 @@ static int __init speakup_init(void)
 {
 	int i;
 	long err = 0;
-	struct st_spk_t *first_console;
 	struct vc_data *vc = vc_cons[fg_console].d;
 	struct var_t *var;
 
@@ -2342,18 +2341,9 @@ static int __init speakup_init(void)
 	if (err)
 		goto error_virtkeyboard;
 
-	first_console = kzalloc(sizeof(*first_console), GFP_KERNEL);
-	if (!first_console) {
-		err = -ENOMEM;
-		goto error_alloc;
-	}
-
-	speakup_console[vc->vc_num] = first_console;
-	speakup_date(vc);
-
 	for (i = 0; i < MAX_NR_CONSOLES; i++)
 		if (vc_cons[i].d) {
-			err = speakup_allocate(vc_cons[i].d);
+			err = speakup_allocate(vc_cons[i].d, GFP_KERNEL);
 			if (err)
 				goto error_kobjects;
 		}
@@ -2412,7 +2402,6 @@ static int __init speakup_init(void)
 	for (i = 0; i < MAX_NR_CONSOLES; i++)
 		kfree(speakup_console[i]);
 
-error_alloc:
 	speakup_remove_virtual_keyboard();
 
 error_virtkeyboard:
-- 
2.10.2

Re: [PATCH-SPEAKUP 2/2] remove unecessary initial allocation of vc

[Samuel Thibault]

Pranay Kr. Srivastava, on mer. 22 févr. 2017 12:50:27 +0530, wrote:
> This patch removes the unnecessary allocation of
> current foreground vc during initialization.
> 
> This initialization is already handled in the loop
> that follows it for all available virtual consoles.
> 
> Also change the prototype of speakup_allocate to take
> extra argument of gfp_* flags. Thus not requiring
> GFP_ATOMIC during initialization.
> 
> Signed-off-by: Pranay Kr. Srivastava <pranjas@gmail.com>

Reviewed-by: Samuel Thibault <samuel.thibault@ens-lyon.org>

> ---
>  drivers/staging/speakup/main.c | 19 ++++---------------
>  1 file changed, 4 insertions(+), 15 deletions(-)
> 
> diff --git a/drivers/staging/speakup/main.c b/drivers/staging/speakup/main.c
> index 6667cf2..f81a936 100644
> --- a/drivers/staging/speakup/main.c
> +++ b/drivers/staging/speakup/main.c
> @@ -1327,14 +1327,14 @@ static int edit_bits(struct vc_data *vc, u_char type, u_char ch, u_short key)
>  }
>  
>  /* Allocation concurrency is protected by the console semaphore */
> -static int speakup_allocate(struct vc_data *vc)
> +static int speakup_allocate(struct vc_data *vc, gfp_t gfp_flags)
>  {
>  	int vc_num;
>  
>  	vc_num = vc->vc_num;
>  	if (speakup_console[vc_num] == NULL) {
>  		speakup_console[vc_num] = kzalloc(sizeof(*speakup_console[0]),
> -						  GFP_ATOMIC);
> +						  gfp_flags);
>  		if (speakup_console[vc_num] == NULL)
>  			return -ENOMEM;
>  		speakup_date(vc);
> @@ -2257,7 +2257,7 @@ static int vt_notifier_call(struct notifier_block *nb,
>  	switch (code) {
>  	case VT_ALLOCATE:
>  		if (vc->vc_mode == KD_TEXT)
> -			speakup_allocate(vc);
> +			speakup_allocate(vc, GFP_ATOMIC);
>  		break;
>  	case VT_DEALLOCATE:
>  		speakup_deallocate(vc);
> @@ -2317,7 +2317,6 @@ static int __init speakup_init(void)
>  {
>  	int i;
>  	long err = 0;
> -	struct st_spk_t *first_console;
>  	struct vc_data *vc = vc_cons[fg_console].d;
>  	struct var_t *var;
>  
> @@ -2342,18 +2341,9 @@ static int __init speakup_init(void)
>  	if (err)
>  		goto error_virtkeyboard;
>  
> -	first_console = kzalloc(sizeof(*first_console), GFP_KERNEL);
> -	if (!first_console) {
> -		err = -ENOMEM;
> -		goto error_alloc;
> -	}
> -
> -	speakup_console[vc->vc_num] = first_console;
> -	speakup_date(vc);
> -
>  	for (i = 0; i < MAX_NR_CONSOLES; i++)
>  		if (vc_cons[i].d) {
> -			err = speakup_allocate(vc_cons[i].d);
> +			err = speakup_allocate(vc_cons[i].d, GFP_KERNEL);
>  			if (err)
>  				goto error_kobjects;
>  		}
> @@ -2412,7 +2402,6 @@ static int __init speakup_init(void)
>  	for (i = 0; i < MAX_NR_CONSOLES; i++)
>  		kfree(speakup_console[i]);
>  
> -error_alloc:
>  	speakup_remove_virtual_keyboard();
>  
>  error_virtkeyboard:
> -- 
> 2.10.2
> 

-- 
Samuel
 Créer une hiérarchie supplementaire pour remedier à un problème (?) de
 dispersion est d'une logique digne des Shadocks.
 * BT in: Guide du Cabaliste Usenet - La Cabale vote oui (les Shadocks aussi) *