Skip to contentSkip to conversation

PATCH Superseded Pranay Kr. Srivastava 4 participants 8 messages Feb 22, 2017 – Feb 28, 2017 Staging

v1 of 3 Superseded Latest is v3 →

lore
Share & tools
lore

Pranay Kr. Srivastava

author
#0 lore raw

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

author
#1 lore raw

PATCH 1/2 return same error value from spk_set_key_info

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 <[email protected]>
---
 drivers/staging/speakup/main.c | 27 +++++++++++++++++++--------
 1 file changed, 19 insertions(+), 8 deletions(-)
drivers/staging/speakup/main.c +19 -8
index c2f70ef..6667cf2 100644
@@ -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;
 	}
Signature · 1 line
2.10.2
Reviewed-by on v1 1/2
#2 lore raw

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

Quoted 7 lines · 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 <[email protected]>
Reviewed-by Samuel Thibault <[email protected]>
Quoted 59 lines
---
 drivers/staging/speakup/main.c | 27 +++++++++++++++++++--------
 1 file changed, 19 insertions(+), 8 deletions(-)
drivers/staging/speakup/main.c +19 -8
index c2f70ef..6667cf2 100644
@@ -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;
 	}
Signature · 1 line
2.10.2
Signature · 3 lines
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 -+-
build bot
#3 lore raw

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

In reply to Pranay Kr. Srivastava
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…

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 <[email protected]>
:::::: CC: Ingo Molnar <[email protected]>

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

maintainer
#4 lore raw

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

In reply to Pranay Kr. Srivastava
Quoted 10 lines · 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 <[email protected]>
---
 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

build bot
#5 lore raw

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

In reply to Pranay Kr. Srivastava
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…

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 <[email protected]>
:::::: CC: Ingo Molnar <[email protected]>

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

author
#6 lore raw

PATCH 2/2 remove unecessary initial allocation of vc

In reply to 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 <[email protected]>
---
 drivers/staging/speakup/main.c | 19 ++++---------------
 1 file changed, 4 insertions(+), 15 deletions(-)
drivers/staging/speakup/main.c +4 -15
index 6667cf2..f81a936 100644
@@ -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:
Signature · 1 line
2.10.2
Reviewed-by on v1 2/2
#7 lore raw

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

Quoted 11 lines · 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 <[email protected]>
Reviewed-by Samuel Thibault <[email protected]>
Quoted 73 lines
---
 drivers/staging/speakup/main.c | 19 ++++---------------
 1 file changed, 4 insertions(+), 15 deletions(-)
drivers/staging/speakup/main.c +4 -15
index 6667cf2..f81a936 100644
@@ -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:
Signature · 1 line
2.10.2
Signature · 4 lines
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) *