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

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

[SF Markus Elfring]

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

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

[SF Markus Elfring]

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

[PATCH 1/4] Input-gameport: Use kmalloc_array() in joydump_connect()

[SF Markus Elfring]

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

* A multiplication for the size determination of a memory allocation
  indicated that an array data structure should be processed.
  Thus use the corresponding function "kmalloc_array".

  This issue was detected by using the Coccinelle software.

* Replace the specification of a data structure by a pointer dereference
  to make the corresponding size determination a bit safer according to
  the Linux coding style convention.

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

diff --git a/drivers/input/joystick/joydump.c b/drivers/input/joystick/joydump.c
index d1c6e48..e515058 100644
--- a/drivers/input/joystick/joydump.c
+++ b/drivers/input/joystick/joydump.c
@@ -79,8 +79,7 @@ static int joydump_connect(struct gameport *gameport, struct gameport_driver *dr
 	}
 
 	timeout = gameport_time(gameport, 10000); /* 10 ms */
-
-	buf = kmalloc(BUF_SIZE * sizeof(struct joydump), GFP_KERNEL);
+	buf = kmalloc_array(BUF_SIZE, sizeof(*buf), GFP_KERNEL);
 	if (!buf) {
 		printk(KERN_INFO "joydump: no memory for testing\n");
 		goto jd_end;
-- 
2.10.0

[PATCH 1/4] Input-gameport: Use kmalloc_array() in joydump_connect()

[SF Markus Elfring]

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

* A multiplication for the size determination of a memory allocation
  indicated that an array data structure should be processed.
  Thus use the corresponding function "kmalloc_array".

  This issue was detected by using the Coccinelle software.

* Replace the specification of a data structure by a pointer dereference
  to make the corresponding size determination a bit safer according to
  the Linux coding style convention.

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

diff --git a/drivers/input/joystick/joydump.c b/drivers/input/joystick/joydump.c
index d1c6e48..e515058 100644
--- a/drivers/input/joystick/joydump.c
+++ b/drivers/input/joystick/joydump.c
@@ -79,8 +79,7 @@ static int joydump_connect(struct gameport *gameport, struct gameport_driver *dr
 	}
 
 	timeout = gameport_time(gameport, 10000); /* 10 ms */
-
-	buf = kmalloc(BUF_SIZE * sizeof(struct joydump), GFP_KERNEL);
+	buf = kmalloc_array(BUF_SIZE, sizeof(*buf), GFP_KERNEL);
 	if (!buf) {
 		printk(KERN_INFO "joydump: no memory for testing\n");
 		goto jd_end;
-- 
2.10.0

Re: [PATCH 1/4] Input-gameport: Use kmalloc_array() in joydump_connect()

[Dmitry Torokhov]

On Sat, Sep 24, 2016 at 03:46:21PM +0200, SF Markus Elfring wrote:
> From: Markus Elfring <elfring@users.sourceforge.net>
> Date: Sat, 24 Sep 2016 13:50:20 +0200
> 
> * A multiplication for the size determination of a memory allocation
>   indicated that an array data structure should be processed.
>   Thus use the corresponding function "kmalloc_array".
> 

-ENOPARSE.

>   This issue was detected by using the Coccinelle software.
> 
> * Replace the specification of a data structure by a pointer dereference
>   to make the corresponding size determination a bit safer according to
>   the Linux coding style convention.
> 
> Signed-off-by: Markus Elfring <elfring@users.sourceforge.net>
> ---
>  drivers/input/joystick/joydump.c | 3 +--
>  1 file changed, 1 insertion(+), 2 deletions(-)
> 
> diff --git a/drivers/input/joystick/joydump.c b/drivers/input/joystick/joydump.c
> index d1c6e48..e515058 100644
> --- a/drivers/input/joystick/joydump.c
> +++ b/drivers/input/joystick/joydump.c
> @@ -79,8 +79,7 @@ static int joydump_connect(struct gameport *gameport, struct gameport_driver *dr
>  	}
>  
>  	timeout = gameport_time(gameport, 10000); /* 10 ms */
> -
> -	buf = kmalloc(BUF_SIZE * sizeof(struct joydump), GFP_KERNEL);
> +	buf = kmalloc_array(BUF_SIZE, sizeof(*buf), GFP_KERNEL);
>  	if (!buf) {
>  		printk(KERN_INFO "joydump: no memory for testing\n");
>  		goto jd_end;
> -- 
> 2.10.0
> 

-- 
Dmitry

Re: [PATCH 1/4] Input-gameport: Use kmalloc_array() in joydump_connect()

[Dmitry Torokhov]

On Sat, Sep 24, 2016 at 03:46:21PM +0200, SF Markus Elfring wrote:
> From: Markus Elfring <elfring@users.sourceforge.net>
> Date: Sat, 24 Sep 2016 13:50:20 +0200
> 
> * A multiplication for the size determination of a memory allocation
>   indicated that an array data structure should be processed.
>   Thus use the corresponding function "kmalloc_array".
> 

-ENOPARSE.

>   This issue was detected by using the Coccinelle software.
> 
> * Replace the specification of a data structure by a pointer dereference
>   to make the corresponding size determination a bit safer according to
>   the Linux coding style convention.
> 
> Signed-off-by: Markus Elfring <elfring@users.sourceforge.net>
> ---
>  drivers/input/joystick/joydump.c | 3 +--
>  1 file changed, 1 insertion(+), 2 deletions(-)
> 
> diff --git a/drivers/input/joystick/joydump.c b/drivers/input/joystick/joydump.c
> index d1c6e48..e515058 100644
> --- a/drivers/input/joystick/joydump.c
> +++ b/drivers/input/joystick/joydump.c
> @@ -79,8 +79,7 @@ static int joydump_connect(struct gameport *gameport, struct gameport_driver *dr
>  	}
>  
>  	timeout = gameport_time(gameport, 10000); /* 10 ms */
> -
> -	buf = kmalloc(BUF_SIZE * sizeof(struct joydump), GFP_KERNEL);
> +	buf = kmalloc_array(BUF_SIZE, sizeof(*buf), GFP_KERNEL);
>  	if (!buf) {
>  		printk(KERN_INFO "joydump: no memory for testing\n");
>  		goto jd_end;
> -- 
> 2.10.0
> 

-- 
Dmitry

Re: Input-gameport: Use kmalloc_array() in joydump_connect()

[SF Markus Elfring]

>> * A multiplication for the size determination of a memory allocation
>>   indicated that an array data structure should be processed.
>>   Thus use the corresponding function "kmalloc_array".
>>
> 
> -ENOPARSE.

Do you find an other wording better?


The script "checkpatch.pl" can point information out like the following.

WARNING: Prefer kmalloc_array over kmalloc with multiply


Regards,
Markus

Re: Input-gameport: Use kmalloc_array() in joydump_connect()

[SF Markus Elfring]

>> * A multiplication for the size determination of a memory allocation
>>   indicated that an array data structure should be processed.
>>   Thus use the corresponding function "kmalloc_array".
>>
> 
> -ENOPARSE.

Do you find an other wording better?


The script "checkpatch.pl" can point information out like the following.

WARNING: Prefer kmalloc_array over kmalloc with multiply


Regards,
Markus

[PATCH 2/4] Input-gameport: Delete an error message for a failed memory allocation

[SF Markus Elfring]

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

Omit an extra message for a memory allocation failure in this function.

Link: http://events.linuxfoundation.org/sites/events/files/slides/LCJ16-Refactor_Strings-WSang_0.pdf

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

diff --git a/drivers/input/joystick/joydump.c b/drivers/input/joystick/joydump.c
index e515058..f9f6cbe 100644
--- a/drivers/input/joystick/joydump.c
+++ b/drivers/input/joystick/joydump.c
@@ -80,10 +80,9 @@ static int joydump_connect(struct gameport *gameport, struct gameport_driver *dr
 
 	timeout = gameport_time(gameport, 10000); /* 10 ms */
 	buf = kmalloc_array(BUF_SIZE, sizeof(*buf), GFP_KERNEL);
-	if (!buf) {
-		printk(KERN_INFO "joydump: no memory for testing\n");
+	if (!buf)
 		goto jd_end;
-	}
+
 	dump = buf;
 	t = 0;
 	i = 1;
-- 
2.10.0

[PATCH 2/4] Input-gameport: Delete an error message for a failed memory allocation

[SF Markus Elfring]

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

Omit an extra message for a memory allocation failure in this function.

Link: http://events.linuxfoundation.org/sites/events/files/slides/LCJ16-Refactor_Strings-WSang_0.pdf

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

diff --git a/drivers/input/joystick/joydump.c b/drivers/input/joystick/joydump.c
index e515058..f9f6cbe 100644
--- a/drivers/input/joystick/joydump.c
+++ b/drivers/input/joystick/joydump.c
@@ -80,10 +80,9 @@ static int joydump_connect(struct gameport *gameport, struct gameport_driver *dr
 
 	timeout = gameport_time(gameport, 10000); /* 10 ms */
 	buf = kmalloc_array(BUF_SIZE, sizeof(*buf), GFP_KERNEL);
-	if (!buf) {
-		printk(KERN_INFO "joydump: no memory for testing\n");
+	if (!buf)
 		goto jd_end;
-	}
+
 	dump = buf;
 	t = 0;
 	i = 1;
-- 
2.10.0

[PATCH 3/4] Input-gameport: Add the macro "pr_fmt" for module "joydump"

[SF Markus Elfring]

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

Add a definition for the macro "pr_fmt" so that its information can be used
for consistent message output.

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

diff --git a/drivers/input/joystick/joydump.c b/drivers/input/joystick/joydump.c
index f9f6cbe..a38f10e 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 pr_fmt(fmt) KBUILD_MODNAME ": " fmt
+
 #include <linux/module.h>
 #include <linux/gameport.h>
 #include <linux/kernel.h>
-- 
2.10.0

[PATCH 3/4] Input-gameport: Add the macro "pr_fmt" for module "joydump"

[SF Markus Elfring]

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

Add a definition for the macro "pr_fmt" so that its information can be used
for consistent message output.

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

diff --git a/drivers/input/joystick/joydump.c b/drivers/input/joystick/joydump.c
index f9f6cbe..a38f10e 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 pr_fmt(fmt) KBUILD_MODNAME ": " fmt
+
 #include <linux/module.h>
 #include <linux/gameport.h>
 #include <linux/kernel.h>
-- 
2.10.0

Re: [PATCH 3/4] Input-gameport: Add the macro "pr_fmt" for module "joydump"

[Dmitry Torokhov]

On Sat, Sep 24, 2016 at 03:48:54PM +0200, SF Markus Elfring wrote:
> From: Markus Elfring <elfring@users.sourceforge.net>
> Date: Sat, 24 Sep 2016 14:30:44 +0200
> 
> Add a definition for the macro "pr_fmt" so that its information can be used
> for consistent message output.
> 
> Signed-off-by: Markus Elfring <elfring@users.sourceforge.net>
> ---
>  drivers/input/joystick/joydump.c | 2 ++
>  1 file changed, 2 insertions(+)
> 
> diff --git a/drivers/input/joystick/joydump.c b/drivers/input/joystick/joydump.c
> index f9f6cbe..a38f10e 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 pr_fmt(fmt) KBUILD_MODNAME ": " fmt
> +

Why did you split it off from the patch where you attempt to use it?
What good does this change do on its own?

>  #include <linux/module.h>
>  #include <linux/gameport.h>
>  #include <linux/kernel.h>
> -- 
> 2.10.0
> 

-- 
Dmitry

Re: [PATCH 3/4] Input-gameport: Add the macro "pr_fmt" for module "joydump"

[Dmitry Torokhov]

On Sat, Sep 24, 2016 at 03:48:54PM +0200, SF Markus Elfring wrote:
> From: Markus Elfring <elfring@users.sourceforge.net>
> Date: Sat, 24 Sep 2016 14:30:44 +0200
> 
> Add a definition for the macro "pr_fmt" so that its information can be used
> for consistent message output.
> 
> Signed-off-by: Markus Elfring <elfring@users.sourceforge.net>
> ---
>  drivers/input/joystick/joydump.c | 2 ++
>  1 file changed, 2 insertions(+)
> 
> diff --git a/drivers/input/joystick/joydump.c b/drivers/input/joystick/joydump.c
> index f9f6cbe..a38f10e 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 pr_fmt(fmt) KBUILD_MODNAME ": " fmt
> +

Why did you split it off from the patch where you attempt to use it?
What good does this change do on its own?

>  #include <linux/module.h>
>  #include <linux/gameport.h>
>  #include <linux/kernel.h>
> -- 
> 2.10.0
> 

-- 
Dmitry

Re: [PATCH 3/4] Input-gameport: Add the macro "pr_fmt" for module "joydump"

[SF Markus Elfring]

>> +#define pr_fmt(fmt) KBUILD_MODNAME ": " fmt
>> +
> 
> Why did you split it off from the patch where you attempt to use it?

I chose a special patch granularity once again.


> What good does this change do on its own?

I find that it is a preparation. - If this addition could not be accepted,
the following update step would also be discussed under an other perspective,
wouldn't it?

Regards,
Markus

Re: [PATCH 3/4] Input-gameport: Add the macro "pr_fmt" for module "joydump"

[SF Markus Elfring]

>> +#define pr_fmt(fmt) KBUILD_MODNAME ": " fmt
>> +
> 
> Why did you split it off from the patch where you attempt to use it?

I chose a special patch granularity once again.


> What good does this change do on its own?

I find that it is a preparation. - If this addition could not be accepted,
the following update step would also be discussed under an other perspective,
wouldn't it?

Regards,
Markus

Re: [PATCH 3/4] Input-gameport: Add the macro "pr_fmt" for module "joydump"

[Joe Perches]

On Sat, 2016-09-24 at 18:55 +0200, SF Markus Elfring wrote:
> > > +#define pr_fmt(fmt) KBUILD_MODNAME ": " fmt
> > Why did you split it off from the patch where you attempt to use it?
> I chose a special patch granularity once again.
> > What good does this change do on its own?
> I find that it is a preparation. - If this addition could not be accepted,
> the following update step would also be discussed under an other perspective,
> wouldn't it?

It's purposeless, creates unnecessary patches to review
and generally wastes other people's time.

Please don't purposefully waste other people's time.
It makes your patch proposals _less_ likely to be applied.

Re: [PATCH 3/4] Input-gameport: Add the macro "pr_fmt" for module "joydump"

[Joe Perches]

On Sat, 2016-09-24 at 18:55 +0200, SF Markus Elfring wrote:
> > > +#define pr_fmt(fmt) KBUILD_MODNAME ": " fmt
> > Why did you split it off from the patch where you attempt to use it?
> I chose a special patch granularity once again.
> > What good does this change do on its own?
> I find that it is a preparation. - If this addition could not be accepted,
> the following update step would also be discussed under an other perspective,
> wouldn't it?

It's purposeless, creates unnecessary patches to review
and generally wastes other people's time.

Please don't purposefully waste other people's time.
It makes your patch proposals _less_ likely to be applied.

Re: Input-gameport: Add the macro "pr_fmt" for module "joydump"

[SF Markus Elfring]

>> I find that it is a preparation. - If this addition could not be accepted,
>> the following update step would also be discussed under an other perspective,
>> wouldn't it?
> 
> It's purposeless, creates unnecessary patches to review
> and generally wastes other people's time.

I have got an other opinion about this.


> Please don't purposefully waste other people's time.

I do not want to "waste" your time. - But I can imagine that I stress your
software development attention to some degree as I am publishing a significant
number of update suggestions according to a bit of static source code analysis.


> It makes your patch proposals _less_ likely to be applied.

The acceptance varies as usual.


I see also another option.

* Can the first three update steps from this small patch series be integrated
  while the fourth needs further adjustments (where I went a bit too far)?

* Do you prefer to squash the last two update steps together?

Regards,
Markus

Re: Input-gameport: Add the macro "pr_fmt" for module "joydump"

[SF Markus Elfring]

>> I find that it is a preparation. - If this addition could not be accepted,
>> the following update step would also be discussed under an other perspective,
>> wouldn't it?
> 
> It's purposeless, creates unnecessary patches to review
> and generally wastes other people's time.

I have got an other opinion about this.


> Please don't purposefully waste other people's time.

I do not want to "waste" your time. - But I can imagine that I stress your
software development attention to some degree as I am publishing a significant
number of update suggestions according to a bit of static source code analysis.


> It makes your patch proposals _less_ likely to be applied.

The acceptance varies as usual.


I see also another option.

* Can the first three update steps from this small patch series be integrated
  while the fourth needs further adjustments (where I went a bit too far)?

* Do you prefer to squash the last two update steps together?

Regards,
Markus

Re: Input-gameport: Add the macro "pr_fmt" for module "joydump"

[Joe Perches]

On Sat, 2016-09-24 at 19:45 +0200, SF Markus Elfring wrote:
> > It's purposeless, creates unnecessary patches to review
> > and generally wastes other people's time.
> I have got an other opinion about this.

Nice for you, not nice for others that have to act on
your patch proposals to get them forwarded upstream.

> > Please don't purposefully waste other people's time.
> I do not want to "waste" your time.

When a chorus of voices says to you that you are wasting
their time, perhaps you listen to their song.

> > It makes your patch proposals _less_ likely to be applied.
> The acceptance varies as usual.

Usual for whom?

It seems to me your patch proposals have a relatively high
unapplied patch percentage and there is an increase in the
number of upstream maintainers that ignore you.

> I see also another option.
> 
> * Can the first three update steps from this small patch series be integrated
>   while the fourth needs further adjustments (where I went a bit too far)?
> 
> * Do you prefer to squash the last two update steps together?

Yes, the overall number of patches should be minimized when
the suggested patches are highly related.

--
To unsubscribe from this list: send the line "unsubscribe kernel-janitors" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: Input-gameport: Add the macro "pr_fmt" for module "joydump"

[Joe Perches]

On Sat, 2016-09-24 at 19:45 +0200, SF Markus Elfring wrote:
> > It's purposeless, creates unnecessary patches to review
> > and generally wastes other people's time.
> I have got an other opinion about this.

Nice for you, not nice for others that have to act on
your patch proposals to get them forwarded upstream.

> > Please don't purposefully waste other people's time.
> I do not want to "waste" your time.

When a chorus of voices says to you that you are wasting
their time, perhaps you listen to their song.

> > It makes your patch proposals _less_ likely to be applied.
> The acceptance varies as usual.

Usual for whom?

It seems to me your patch proposals have a relatively high
unapplied patch percentage and there is an increase in the
number of upstream maintainers that ignore you.

> I see also another option.
> 
> * Can the first three update steps from this small patch series be integrated
>   while the fourth needs further adjustments (where I went a bit too far)?
> 
> * Do you prefer to squash the last two update steps together?

Yes, the overall number of patches should be minimized when
the suggested patches are highly related.

Re: Input-gameport: Add the macro "pr_fmt" for module "joydump"

[SF Markus Elfring]

> When a chorus of voices says to you that you are wasting
> their time, perhaps you listen to their song.

I am listening to some degree. - I guess that I am making some decisions
and conclusions in a way that you would prefer to be different.

I know that my update suggestions won't be liked by all contributors.


> It seems to me your patch proposals have a relatively high
> unapplied patch percentage

I agree here. - This impression can occur for a while.

I find that this situation is hardly avoidable according to my evolving number
of update suggestions.
In which time frames do you look for the corresponding software development process?


> and there is an increase in the number of upstream maintainers that ignore you.

It's a pity. - I hope that the corresponding software development can still
be continued in constructive ways.


> Yes, the overall number of patches should be minimized when
> the suggested patches are highly related.

I have got a special opinion about patch squashing. But I can consider it
also for this update step when a corresponding consensus could be
achieved between contributors.

Regards,
Markus

Re: Input-gameport: Add the macro "pr_fmt" for module "joydump"

[SF Markus Elfring]

> When a chorus of voices says to you that you are wasting
> their time, perhaps you listen to their song.

I am listening to some degree. - I guess that I am making some decisions
and conclusions in a way that you would prefer to be different.

I know that my update suggestions won't be liked by all contributors.


> It seems to me your patch proposals have a relatively high
> unapplied patch percentage

I agree here. - This impression can occur for a while.

I find that this situation is hardly avoidable according to my evolving number
of update suggestions.
In which time frames do you look for the corresponding software development process?


> and there is an increase in the number of upstream maintainers that ignore you.

It's a pity. - I hope that the corresponding software development can still
be continued in constructive ways.


> Yes, the overall number of patches should be minimized when
> the suggested patches are highly related.

I have got a special opinion about patch squashing. But I can consider it
also for this update step when a corresponding consensus could be
achieved between contributors.

Regards,
Markus

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

[SF Markus Elfring]

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

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

[SF Markus Elfring]

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

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

[Joe Perches]

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.

--
To unsubscribe from this list: send the line "unsubscribe kernel-janitors" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

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

[Joe Perches]

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.

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

[SF Markus Elfring]

>> @@ -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

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

[SF Markus Elfring]

>> @@ -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

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

[Dmitry Torokhov]

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

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

[Dmitry Torokhov]

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

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

[SF Markus Elfring]

> 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

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

[SF Markus Elfring]

> 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

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

[Joe Perches]

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.

--
To unsubscribe from this list: send the line "unsubscribe kernel-janitors" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

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

[Joe Perches]

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.

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

[SF Markus Elfring]

>> 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

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

[SF Markus Elfring]

>> 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

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

[SF Markus Elfring]

>> @@ -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

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

[SF Markus Elfring]

>> @@ -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

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

[SF Markus Elfring]

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

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

[SF Markus Elfring]

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

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

[Joe Perches]

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.
--
To unsubscribe from this list: send the line "unsubscribe kernel-janitors" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

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

[Joe Perches]

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.

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

[SF Markus Elfring]

> 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

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

[SF Markus Elfring]

> 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