Groups | Search | Server Info | Keyboard shortcuts | Login | Register [http] [https] [nntp] [nntps]

Groups > linux.kernel > #1490652 > unrolled thread

[PATCH 0/4] Input-gameport: Fine-tuning for joydump_connect()

Back to article view | Back to linux.kernel

Contents

  [PATCH 0/4] Input-gameport: Fine-tuning for joydump_connect() SF Markus Elfring <elfring@users.sourceforge.net> - 2016-09-24 15:50 +0200
    [PATCH 4/4] Input-gameport: Replace some printk() calls by pr_info()  in joydump_connect() SF Markus Elfring <elfring@users.sourceforge.net> - 2016-09-24 16:00 +0200
      Re: [PATCH 4/4] Input-gameport: Replace some printk() calls by  pr_info() in joydump_connect() Joe Perches <joe@perches.com> - 2016-09-24 18:20 +0200
        Re: [PATCH 4/4] Input-gameport: Replace some printk() calls by  pr_info() in joydump_connect() Dmitry Torokhov <dmitry.torokhov@gmail.com> - 2016-09-24 18:40 +0200
          Re: Input-gameport: Replace some printk() calls by pr_info() in  joydump_connect() SF Markus Elfring <elfring@users.sourceforge.net> - 2016-09-24 19:30 +0200
        Re: [PATCH 4/4] Input-gameport: Replace some printk() calls by  pr_info() in joydump_connect() SF Markus Elfring <elfring@users.sourceforge.net> - 2016-09-24 18:40 +0200
          Re: [PATCH 4/4] Input-gameport: Replace some printk() calls by  pr_info() in joydump_connect() Joe Perches <joe@perches.com> - 2016-09-24 18:50 +0200
            Re: Input-gameport: Replace some printk() calls by pr_info() in  joydump_connect() SF Markus Elfring <elfring@users.sourceforge.net> - 2016-09-24 22:00 +0200
        Re: [PATCH 4/4] Input-gameport: Replace some printk() calls by  pr_info() in joydump_connect() SF Markus Elfring <elfring@users.sourceforge.net> - 2016-09-24 18:50 +0200
        [PATCH v2 3/3] Input-gameport: Replace some printk() calls by  pr_info() in joydump_connect() SF Markus Elfring <elfring@users.sourceforge.net> - 2016-09-25 09:20 +0200
          Re: [PATCH v2 3/3] Input-gameport: Replace some printk() calls by  pr_info() in joydump_connect() Joe Perches <joe@perches.com> - 2016-09-25 09:40 +0200
            Re: [PATCH v2 3/3] Input-gameport: Replace some printk() calls by  pr_info() in joydump_connect() SF Markus Elfring <elfring@users.sourceforge.net> - 2016-09-25 09:50 +0200

#1490652 — [PATCH 0/4] Input-gameport: Fine-tuning for joydump_connect()

From: Markus Elfring <elfring@users.sourceforge.net>
Date: Sat, 24 Sep 2016 15:34:15 +0200

A few update suggestions were taken into account
from static source code analysis.

Markus Elfring (4):
  Use kmalloc_array()
  Delete an error message for a failed memory allocation
  Add the macro "pr_fmt"
  Replace some printk() calls by pr_info()

 drivers/input/joystick/joydump.c | 46 ++++++++++++++++++++--------------------
 1 file changed, 23 insertions(+), 23 deletions(-)

-- 
2.10.0

[toc] | [next] | [standalone]

#1490654 — [PATCH 4/4] Input-gameport: Replace some printk() calls by pr_info() in joydump_connect()

From: Markus Elfring <elfring@users.sourceforge.net>
Date: Sat, 24 Sep 2016 15:12:21 +0200

* Prefer usage of the macro "pr_info" over the interface "printk"
  in this function.

* Reduce number of output function calls.

Signed-off-by: Markus Elfring <elfring@users.sourceforge.net>
---
 drivers/input/joystick/joydump.c | 36 ++++++++++++++++++------------------
 1 file changed, 18 insertions(+), 18 deletions(-)

diff --git a/drivers/input/joystick/joydump.c b/drivers/input/joystick/joydump.c
index a38f10e..02ea60c 100644
--- a/drivers/input/joystick/joydump.c
+++ b/drivers/input/joystick/joydump.c
@@ -57,27 +57,29 @@ static int joydump_connect(struct gameport *gameport, struct gameport_driver *dr
 	unsigned long flags;
 	unsigned char u;
 
-	printk(KERN_INFO "joydump: ,------------------ START ----------------.\n");
-	printk(KERN_INFO "joydump: | Dumping: %30s |\n", gameport->phys);
-	printk(KERN_INFO "joydump: | Speed: %28d kHz |\n", gameport->speed);
+	pr_info(",------------------ START ----------------.\n"
+		"| Dumping: %30s |\n"
+		"| Speed: %28d kHz |\n",
+		gameport->phys,
+		gameport->speed);
 
 	if (gameport_open(gameport, drv, GAMEPORT_MODE_RAW)) {
-
-		printk(KERN_INFO "joydump: | Raw mode not available - trying cooked.    |\n");
-
+		pr_info("| Raw mode not available - trying cooked.    |\n");
 		if (gameport_open(gameport, drv, GAMEPORT_MODE_COOKED)) {
-
-			printk(KERN_INFO "joydump: | Cooked not available either. Failing.   |\n");
-			printk(KERN_INFO "joydump: `------------------- END -----------------'\n");
+			pr_info("| Cooked not available either. Failing.   |\n"
+				"`------------------- END -----------------'\n");
 			return -ENODEV;
 		}
 
 		gameport_cooked_read(gameport, axes, &buttons);
 
 		for (i = 0; i < 4; i++)
-			printk(KERN_INFO "joydump: | Axis %d: %4d.                           |\n", i, axes[i]);
-		printk(KERN_INFO "joydump: | Buttons %02x.                             |\n", buttons);
-		printk(KERN_INFO "joydump: `------------------- END -----------------'\n");
+			pr_info("| Axis %d: %4d.                           |\n",
+				i,
+				axes[i]);
+		pr_info("| Buttons %02x.                             |\n"
+			"`------------------- END -----------------'\n",
+			buttons);
 	}
 
 	timeout = gameport_time(gameport, 10000); /* 10 ms */
@@ -121,16 +123,15 @@ static int joydump_connect(struct gameport *gameport, struct gameport_driver *dr
 	t = i;
 	dump = buf;
 	prev = dump;
-
-	printk(KERN_INFO "joydump: >------------------ DATA -----------------<\n");
-	printk(KERN_INFO "joydump: | index: %3d delta: %3d us data: ", 0, 0);
+	pr_info(">------------------ DATA -----------------<\n"
+		"| index: %3d delta: %3d us data: ", 0, 0);
 	for (j = 7; j >= 0; j--)
 		printk("%d", (dump->data >> j) & 1);
 	printk(" |\n");
 	dump++;
 
 	for (i = 1; i < t; i++, dump++, prev++) {
-		printk(KERN_INFO "joydump: | index: %3d delta: %3d us data: ",
+		pr_info("| index: %3d delta: %3d us data: ",
 			i, dump->time - prev->time);
 		for (j = 7; j >= 0; j--)
 			printk("%d", (dump->data >> j) & 1);
@@ -139,8 +140,7 @@ static int joydump_connect(struct gameport *gameport, struct gameport_driver *dr
 	kfree(buf);
 
 jd_end:
-	printk(KERN_INFO "joydump: `------------------- END -----------------'\n");
-
+	pr_info("`------------------- END -----------------'\n");
 	return 0;
 }
 
-- 
2.10.0

[toc] | [prev] | [next] | [standalone]

#1490674 — Re: [PATCH 4/4] Input-gameport: Replace some printk() calls by pr_info() in joydump_connect()

On Sat, 2016-09-24 at 15:50 +0200, SF Markus Elfring wrote:

> * Prefer usage of the macro "pr_info" over the interface "printk"
>   in this function.
> * Reduce number of output function calls.

Did you test this?  I doubt it.

> diff --git a/drivers/input/joystick/joydump.c b/drivers/input/joystick/joydump.c
[]
> @@ -57,27 +57,29 @@ static int joydump_connect(struct gameport *gameport, struct gameport_driver *dr
> >  	unsigned long flags;
> >  	unsigned char u;
>  
> > -	printk(KERN_INFO "joydump: ,------------------ START ----------------.\n");
> > -	printk(KERN_INFO "joydump: | Dumping: %30s |\n", gameport->phys);
> > -	printk(KERN_INFO "joydump: | Speed: %28d kHz |\n", gameport->speed);
> > +	pr_info(",------------------ START ----------------.\n"
> > +		"| Dumping: %30s |\n"
> > +		"| Speed: %28d kHz |\n",
> > +		gameport->phys,
> > +		gameport->speed);

Not the same output.

[toc] | [prev] | [next] | [standalone]

#1490676 — Re: [PATCH 4/4] Input-gameport: Replace some printk() calls by pr_info() in joydump_connect()

On Sat, Sep 24, 2016 at 06:32:29PM +0200, SF Markus Elfring wrote:
> >> @@ -57,27 +57,29 @@ static int joydump_connect(struct gameport *gameport, struct gameport_driver *dr
> >>>  	unsigned long flags;
> >>>  	unsigned char u;
> >>  
> >>> -	printk(KERN_INFO "joydump: ,------------------ START ----------------.\n");
> >>> -	printk(KERN_INFO "joydump: | Dumping: %30s |\n", gameport->phys);
> >>> -	printk(KERN_INFO "joydump: | Speed: %28d kHz |\n", gameport->speed);
> >>> +	pr_info(",------------------ START ----------------.\n"
> >>> +		"| Dumping: %30s |\n"
> >>> +		"| Speed: %28d kHz |\n",
> >>> +		gameport->phys,
> >>> +		gameport->speed);
> > 
> > Not the same output.
> 
> Should the desired output be the same when the relevant data are passed by a single function call
> (instead of three as before)?

The desired output should not be broken in conversion, which you did. Do
you know how syslog works and why the transformation is not correct.

I am also curious as to why you are patching joydump? Are you working on
extending it?

Thanks.

-- 
Dmitry

[toc] | [prev] | [next] | [standalone]

#1490694 — Re: Input-gameport: Replace some printk() calls by pr_info() in joydump_connect()

> The desired output should not be broken in conversion, which you did.

I dared to change another source code place a bit too much perhaps.


> Do you know how syslog works and why the transformation is not correct.

I imagine that there are chances to improve the software situation a bit more,
aren't there?


> I am also curious as to why you are patching joydump?

The discussed function implementation contains update candidates.

One of them was detected by the execution of a script for the semantic patch
language (Coccinelle software).
I identified further update possibilities after the opportunity for using
the function "kmalloc_array" also in this software module.


> Are you working on extending it?

I attempted just another software refactoring.

Regards,
Markus

[toc] | [prev] | [next] | [standalone]

#1490680 — Re: [PATCH 4/4] Input-gameport: Replace some printk() calls by pr_info() in joydump_connect()

>> @@ -57,27 +57,29 @@ static int joydump_connect(struct gameport *gameport, struct gameport_driver *dr
>>>  	unsigned long flags;
>>>  	unsigned char u;
>>  
>>> -	printk(KERN_INFO "joydump: ,------------------ START ----------------.\n");
>>> -	printk(KERN_INFO "joydump: | Dumping: %30s |\n", gameport->phys);
>>> -	printk(KERN_INFO "joydump: | Speed: %28d kHz |\n", gameport->speed);
>>> +	pr_info(",------------------ START ----------------.\n"
>>> +		"| Dumping: %30s |\n"
>>> +		"| Speed: %28d kHz |\n",
>>> +		gameport->phys,
>>> +		gameport->speed);
> 
> Not the same output.

Should the desired output be the same when the relevant data are passed by a single function call
(instead of three as before)?

Regards,
Markus

[toc] | [prev] | [next] | [standalone]

#1490683 — Re: [PATCH 4/4] Input-gameport: Replace some printk() calls by pr_info() in joydump_connect()

On Sat, 2016-09-24 at 18:32 +0200, SF Markus Elfring wrote:
> > > @@ -57,27 +57,29 @@ static int joydump_connect(struct gameport *gameport, struct gameport_driver *dr
> > >  	unsigned long flags;
> > >  	unsigned char u;
> > 
> >  
> > > -	printk(KERN_INFO "joydump: ,------------------ START ----------------.\n");
> > > -	printk(KERN_INFO "joydump: | Dumping: %30s |\n", gameport->phys);
> > > -	printk(KERN_INFO "joydump: | Speed: %28d kHz |\n", gameport->speed);
> > > +	pr_info(",------------------ START ----------------.\n"
> > > +		"| Dumping: %30s |\n"
> > > +		"| Speed: %28d kHz |\n",
> > > +		gameport->phys,
> > > +		gameport->speed);
> 
> 
> > Not the same output.
> 
> 
> Should the desired output be the same when the relevant data are passed by a single function call
> (instead of three as before)?

Adding a singleton for a pr_fmt #define constant string and
updating the printk subsystem to prepend that constant string
to each use of a pr_<level> at runtime would be an improvement
as it could reduce constant data used by the format strings.

That would be a _real_ improvement.

Please try to implement something like that before submitting
more of these incorrect patches.

[toc] | [prev] | [next] | [standalone]

#1490736 — Re: Input-gameport: Replace some printk() calls by pr_info() in joydump_connect()

>> Should the desired output be the same when the relevant data are passed by a single function call
>> (instead of three as before)?
> 
> Adding a singleton for a pr_fmt #define constant string and
> updating the printk subsystem to prepend that constant string
> to each use of a pr_<level> at runtime would be an improvement
> as it could reduce constant data used by the format strings.

Thanks for for this constructive feedback.


> That would be a _real_ improvement.

This sounds interesting for me too.


> Please try to implement something like that before submitting
> more of these incorrect patches.

Nice wish!

Did any other software developer try to implement such enhanced
logging functionality already?

Regards,
Markus

[toc] | [prev] | [next] | [standalone]

#1490684 — Re: [PATCH 4/4] Input-gameport: Replace some printk() calls by pr_info() in joydump_connect()

>> @@ -57,27 +57,29 @@ static int joydump_connect(struct gameport *gameport, struct gameport_driver *dr
>>>  	unsigned long flags;
>>>  	unsigned char u;
>>  
>>> -	printk(KERN_INFO "joydump: ,------------------ START ----------------.\n");
>>> -	printk(KERN_INFO "joydump: | Dumping: %30s |\n", gameport->phys);
>>> -	printk(KERN_INFO "joydump: | Speed: %28d kHz |\n", gameport->speed);
>>> +	pr_info(",------------------ START ----------------.\n"
>>> +		"| Dumping: %30s |\n"
>>> +		"| Speed: %28d kHz |\n",
>>> +		gameport->phys,
>>> +		gameport->speed);
> 
> Not the same output.

Do you insist that each line from a multi-line text that is passed by such a single logging call
contains the same module prefix?

Regards,
Markus

[toc] | [prev] | [next] | [standalone]

#1490825 — [PATCH v2 3/3] Input-gameport: Replace some printk() calls by pr_info() in joydump_connect()

From: Markus Elfring <elfring@users.sourceforge.net>
Date: Sun, 25 Sep 2016 08:40:34 +0200

1. Add a definition for the macros "MY_LOG_PREFIX" and "pr_fmt" so that
   their information can be used for consistent message output.

2. Prefer usage of the macro "pr_info" over the interface "printk"
   in this function.

3. Reduce number of output function calls.

Signed-off-by: Markus Elfring <elfring@users.sourceforge.net>
---

v2: Yesterday a software development discussion pointed weaknesses out around
    the previous update steps "3" and "4".
    Now I propose this update variant in the hope that my second approach
    for this software module will work as desired and can be accepted
    a bit easier.

    Will the script "checkpatch.pl" need another improvement so that
    the following information can be avoided for my use case anyhow?

    ERROR: Macros with complex values should be enclosed in parentheses


 drivers/input/joystick/joydump.c | 40 ++++++++++++++++++++++------------------
 1 file changed, 22 insertions(+), 18 deletions(-)

diff --git a/drivers/input/joystick/joydump.c b/drivers/input/joystick/joydump.c
index f9f6cbe..e23c499 100644
--- a/drivers/input/joystick/joydump.c
+++ b/drivers/input/joystick/joydump.c
@@ -27,6 +27,8 @@
  * Vojtech Pavlik, Simunkova 1594, Prague 8, 182 00 Czech Republic
  */
 
+#define MY_LOG_PREFIX KBUILD_MODNAME ": "
+#define pr_fmt(fmt) MY_LOG_PREFIX fmt
 #include <linux/module.h>
 #include <linux/gameport.h>
 #include <linux/kernel.h>
@@ -55,27 +57,31 @@ static int joydump_connect(struct gameport *gameport, struct gameport_driver *dr
 	unsigned long flags;
 	unsigned char u;
 
-	printk(KERN_INFO "joydump: ,------------------ START ----------------.\n");
-	printk(KERN_INFO "joydump: | Dumping: %30s |\n", gameport->phys);
-	printk(KERN_INFO "joydump: | Speed: %28d kHz |\n", gameport->speed);
+	pr_info(",------------------ START ----------------.\n"
+		MY_LOG_PREFIX "| Dumping: %30s |\n"
+		MY_LOG_PREFIX "| Speed: %28d kHz |\n",
+		gameport->phys,
+		gameport->speed);
 
 	if (gameport_open(gameport, drv, GAMEPORT_MODE_RAW)) {
-
-		printk(KERN_INFO "joydump: | Raw mode not available - trying cooked.    |\n");
-
+		pr_info("| Raw mode not available - trying cooked.    |\n");
 		if (gameport_open(gameport, drv, GAMEPORT_MODE_COOKED)) {
-
-			printk(KERN_INFO "joydump: | Cooked not available either. Failing.   |\n");
-			printk(KERN_INFO "joydump: `------------------- END -----------------'\n");
+			pr_info("| Cooked not available either. Failing.   |\n"
+				MY_LOG_PREFIX
+				"`------------------- END -----------------'\n");
 			return -ENODEV;
 		}
 
 		gameport_cooked_read(gameport, axes, &buttons);
 
 		for (i = 0; i < 4; i++)
-			printk(KERN_INFO "joydump: | Axis %d: %4d.                           |\n", i, axes[i]);
-		printk(KERN_INFO "joydump: | Buttons %02x.                             |\n", buttons);
-		printk(KERN_INFO "joydump: `------------------- END -----------------'\n");
+			pr_info("| Axis %d: %4d.                           |\n",
+				i,
+				axes[i]);
+		pr_info("| Buttons %02x.                             |\n"
+			MY_LOG_PREFIX
+			"`------------------- END -----------------'\n",
+			buttons);
 	}
 
 	timeout = gameport_time(gameport, 10000); /* 10 ms */
@@ -119,16 +125,15 @@ static int joydump_connect(struct gameport *gameport, struct gameport_driver *dr
 	t = i;
 	dump = buf;
 	prev = dump;
-
-	printk(KERN_INFO "joydump: >------------------ DATA -----------------<\n");
-	printk(KERN_INFO "joydump: | index: %3d delta: %3d us data: ", 0, 0);
+	pr_info(">------------------ DATA -----------------<\n"
+		MY_LOG_PREFIX "| index: %3d delta: %3d us data: ", 0, 0);
 	for (j = 7; j >= 0; j--)
 		printk("%d", (dump->data >> j) & 1);
 	printk(" |\n");
 	dump++;
 
 	for (i = 1; i < t; i++, dump++, prev++) {
-		printk(KERN_INFO "joydump: | index: %3d delta: %3d us data: ",
+		pr_info("| index: %3d delta: %3d us data: ",
 			i, dump->time - prev->time);
 		for (j = 7; j >= 0; j--)
 			printk("%d", (dump->data >> j) & 1);
@@ -137,8 +142,7 @@ static int joydump_connect(struct gameport *gameport, struct gameport_driver *dr
 	kfree(buf);
 
 jd_end:
-	printk(KERN_INFO "joydump: `------------------- END -----------------'\n");
-
+	pr_info("`------------------- END -----------------'\n");
 	return 0;
 }
 
-- 
2.10.0

[toc] | [prev] | [next] | [standalone]

#1490833 — Re: [PATCH v2 3/3] Input-gameport: Replace some printk() calls by pr_info() in joydump_connect()

On Sun, 2016-09-25 at 09:15 +0200, SF Markus Elfring wrote:
> 1. Add a definition for the macros "MY_LOG_PREFIX" and "pr_fmt" so that
>    their information can be used for consistent message output.
> 
> 
> 2. Prefer usage of the macro "pr_info" over the interface "printk"
>    in this function.
> 
> 
> 3. Reduce number of output function calls.
> 
> 
> Signed-off-by: Markus Elfring <elfring@users.sourceforge.net>
> ---
> 
> 
> v2: Yesterday a software development discussion pointed weaknesses out around
>     the previous update steps "3" and "4".
>     Now I propose this update variant in the hope that my second approach
>     for this software module will work as desired and can be accepted
>     a bit easier.

No thank you.

This is not a good change as it messes with dmesg timestamps.

Simpler to read and more straightforward is multiple
individual function calls.

[toc] | [prev] | [next] | [standalone]

#1490839 — Re: [PATCH v2 3/3] Input-gameport: Replace some printk() calls by pr_info() in joydump_connect()

> This is not a good change as it messes with dmesg timestamps.

Thanks for your quick feedback.

Have you got any more concerns around multi-line text output?


> Simpler to read and more straightforward is multiple
> individual function calls.

Will it become feasible to pass the desired data also only
by one logging call directly?

Regards,
Markus

[toc] | [prev] | [standalone]

Back to top | Article view | linux.kernel

csiph-web