[PATCH] Input - elantech: fix debug dump of the current packet

[PATCH] Input - elantech: fix debug dump of the current packet

[Benjamin Tissoires]

The use of mixed psmouse_printk() and printk creates 2 lines in the log,
while the use of %*ph solves everything.

Signed-off-by: Benjamin Tissoires <benjamin.tissoires@redhat.com>
---
 drivers/input/mouse/elantech.c | 8 ++------
 1 file changed, 2 insertions(+), 6 deletions(-)

diff --git a/drivers/input/mouse/elantech.c b/drivers/input/mouse/elantech.c
index 615d23e..3461d04 100644
--- a/drivers/input/mouse/elantech.c
+++ b/drivers/input/mouse/elantech.c
@@ -222,12 +222,8 @@ static int elantech_write_reg(struct psmouse *psmouse, unsigned char reg,
  */
 static void elantech_packet_dump(struct psmouse *psmouse)
 {
-	int	i;
-
-	psmouse_printk(KERN_DEBUG, psmouse, "PS/2 packet [");
-	for (i = 0; i < psmouse->pktsize; i++)
-		printk("%s0x%02x ", i ? ", " : " ", psmouse->packet[i]);
-	printk("]\n");
+	psmouse_printk(KERN_DEBUG, psmouse, "PS/2 packet [%*ph]",
+		       psmouse->pktsize, psmouse->packet);
 }
 
 /*
-- 
2.5.5

Re: [PATCH] Input - elantech: fix debug dump of the current packet

[Dmitry Torokhov]

On Thu, Jul 28, 2016 at 11:58:55AM +0200, Benjamin Tissoires wrote:
> The use of mixed psmouse_printk() and printk creates 2 lines in the log,
> while the use of %*ph solves everything.
> 
> Signed-off-by: Benjamin Tissoires <benjamin.tissoires@redhat.com>

Applied, thank you.

> ---
>  drivers/input/mouse/elantech.c | 8 ++------
>  1 file changed, 2 insertions(+), 6 deletions(-)
> 
> diff --git a/drivers/input/mouse/elantech.c b/drivers/input/mouse/elantech.c
> index 615d23e..3461d04 100644
> --- a/drivers/input/mouse/elantech.c
> +++ b/drivers/input/mouse/elantech.c
> @@ -222,12 +222,8 @@ static int elantech_write_reg(struct psmouse *psmouse, unsigned char reg,
>   */
>  static void elantech_packet_dump(struct psmouse *psmouse)
>  {
> -	int	i;
> -
> -	psmouse_printk(KERN_DEBUG, psmouse, "PS/2 packet [");
> -	for (i = 0; i < psmouse->pktsize; i++)
> -		printk("%s0x%02x ", i ? ", " : " ", psmouse->packet[i]);
> -	printk("]\n");
> +	psmouse_printk(KERN_DEBUG, psmouse, "PS/2 packet [%*ph]",
> +		       psmouse->pktsize, psmouse->packet);
>  }
>  
>  /*
> -- 
> 2.5.5
> 

-- 
Dmitry

Re: [PATCH] Input - elantech: fix debug dump of the current packet

[Joe Perches]

On Thu, 2016-07-28 at 11:58 +0200, Benjamin Tissoires wrote:
> The use of mixed psmouse_printk() and printk creates 2 lines in the log,
> while the use of %*ph solves everything.
> 
> Signed-off-by: Benjamin Tissoires <benjamin.tissoires@redhat.com>
> ---
>  drivers/input/mouse/elantech.c | 8 ++------
>  1 file changed, 2 insertions(+), 6 deletions(-)
> 
> diff --git a/drivers/input/mouse/elantech.c b/drivers/input/mouse/elantech.c
> index 615d23e..3461d04 100644
> --- a/drivers/input/mouse/elantech.c
> +++ b/drivers/input/mouse/elantech.c
> @@ -222,12 +222,8 @@ static int elantech_write_reg(struct psmouse *psmouse, unsigned char reg,
>   */
>  static void elantech_packet_dump(struct psmouse *psmouse)
>  {
> -	int	i;
> -
> -	psmouse_printk(KERN_DEBUG, psmouse, "PS/2 packet [");
> -	for (i = 0; i < psmouse->pktsize; i++)
> -		printk("%s0x%02x ", i ? ", " : " ", psmouse->packet[i]);
> -	printk("]\n");
> +	psmouse_printk(KERN_DEBUG, psmouse, "PS/2 packet [%*ph]",
> +		       psmouse->pktsize, psmouse->packet);
>  }
>  
>  /*

This should add a newline "\n" format termination but
how does the original produce multiple lines of output?

It looks more like psmouse_printk should be removed altogether
and dev_debug should be used instead.

Re: [PATCH] Input - elantech: fix debug dump of the current packet

[Dmitry Torokhov]

On Thu, Jul 28, 2016 at 11:30:56AM -0700, Joe Perches wrote:
> On Thu, 2016-07-28 at 11:58 +0200, Benjamin Tissoires wrote:
> > The use of mixed psmouse_printk() and printk creates 2 lines in the log,
> > while the use of %*ph solves everything.
> > 
> > Signed-off-by: Benjamin Tissoires <benjamin.tissoires@redhat.com>
> > ---
> >  drivers/input/mouse/elantech.c | 8 ++------
> >  1 file changed, 2 insertions(+), 6 deletions(-)
> > 
> > diff --git a/drivers/input/mouse/elantech.c b/drivers/input/mouse/elantech.c
> > index 615d23e..3461d04 100644
> > --- a/drivers/input/mouse/elantech.c
> > +++ b/drivers/input/mouse/elantech.c
> > @@ -222,12 +222,8 @@ static int elantech_write_reg(struct psmouse *psmouse, unsigned char reg,
> >   */
> >  static void elantech_packet_dump(struct psmouse *psmouse)
> >  {
> > -	int	i;
> > -
> > -	psmouse_printk(KERN_DEBUG, psmouse, "PS/2 packet [");
> > -	for (i = 0; i < psmouse->pktsize; i++)
> > -		printk("%s0x%02x ", i ? ", " : " ", psmouse->packet[i]);
> > -	printk("]\n");
> > +	psmouse_printk(KERN_DEBUG, psmouse, "PS/2 packet [%*ph]",
> > +		       psmouse->pktsize, psmouse->packet);
> >  }
> >  
> >  /*
> 
> This should add a newline "\n" format termination but

I added it, thanks for alerting me.

> how does the original produce multiple lines of output?
> 
> It looks more like psmouse_printk should be removed altogether
> and dev_debug should be used instead.

That does not produce output by default whereas elantech driver uses
elantech_packet_dump() to drop unexpected packets.

Thanks.

-- 
Dmitry

Re: [PATCH] Input - elantech: fix debug dump of the current packet

[Joe Perches]

On Thu, 2016-07-28 at 11:40 -0700, Dmitry Torokhov wrote:
> On Thu, Jul 28, 2016 at 11:30:56AM -0700, Joe Perches wrote:
> > 
> > On Thu, 2016-07-28 at 11:58 +0200, Benjamin Tissoires wrote:
> > > 
> > > The use of mixed psmouse_printk() and printk creates 2 lines in the log,
> > > while the use of %*ph solves everything.
> > > 
> > > Signed-off-by: Benjamin Tissoires <benjamin.tissoires@redhat.com>
> > > ---
> > >  drivers/input/mouse/elantech.c | 8 ++------
> > >  1 file changed, 2 insertions(+), 6 deletions(-)
> > > 
> > > diff --git a/drivers/input/mouse/elantech.c b/drivers/input/mouse/elantech.c
> > > index 615d23e..3461d04 100644
> > > --- a/drivers/input/mouse/elantech.c
> > > +++ b/drivers/input/mouse/elantech.c
> > > @@ -222,12 +222,8 @@ static int elantech_write_reg(struct psmouse *psmouse, unsigned char reg,
> > >   */
> > >  static void elantech_packet_dump(struct psmouse *psmouse)
> > >  {
> > > -	int	i;
> > > -
> > > -	psmouse_printk(KERN_DEBUG, psmouse, "PS/2 packet [");
> > > -	for (i = 0; i < psmouse->pktsize; i++)
> > > -		printk("%s0x%02x ", i ? ", " : " ", psmouse->packet[i]);
> > > -	printk("]\n");
> > > +	psmouse_printk(KERN_DEBUG, psmouse, "PS/2 packet [%*ph]",
> > > +		       psmouse->pktsize, psmouse->packet);
> > >  }
> > >  
> > >  /*
> > This should add a newline "\n" format termination but
> I added it, thanks for alerting me.
> 
> > 
> > how does the original produce multiple lines of output?
> > 
> > It looks more like psmouse_printk should be removed altogether
> > and dev_debug should be used instead.
> That does not produce output by default whereas elantech driver uses
> elantech_packet_dump() to drop unexpected packets.

So use dev_printk(KERN_DEBUG, etc...


But I still don't see a mechanism for the original code to be
emitted on multiple lines.

How long is the longest packet?

This might truncate it.

Should this be ratelimited?

Re: [PATCH] Input - elantech: fix debug dump of the current packet

[Dmitry Torokhov]

On Thu, Jul 28, 2016 at 11:49:57AM -0700, Joe Perches wrote:
> On Thu, 2016-07-28 at 11:40 -0700, Dmitry Torokhov wrote:
> > On Thu, Jul 28, 2016 at 11:30:56AM -0700, Joe Perches wrote:
> > > 
> > > On Thu, 2016-07-28 at 11:58 +0200, Benjamin Tissoires wrote:
> > > > 
> > > > The use of mixed psmouse_printk() and printk creates 2 lines in the log,
> > > > while the use of %*ph solves everything.
> > > > 
> > > > Signed-off-by: Benjamin Tissoires <benjamin.tissoires@redhat.com>
> > > > ---
> > > >  drivers/input/mouse/elantech.c | 8 ++------
> > > >  1 file changed, 2 insertions(+), 6 deletions(-)
> > > > 
> > > > diff --git a/drivers/input/mouse/elantech.c b/drivers/input/mouse/elantech.c
> > > > index 615d23e..3461d04 100644
> > > > --- a/drivers/input/mouse/elantech.c
> > > > +++ b/drivers/input/mouse/elantech.c
> > > > @@ -222,12 +222,8 @@ static int elantech_write_reg(struct psmouse *psmouse, unsigned char reg,
> > > >   */
> > > >  static void elantech_packet_dump(struct psmouse *psmouse)
> > > >  {
> > > > -	int	i;
> > > > -
> > > > -	psmouse_printk(KERN_DEBUG, psmouse, "PS/2 packet [");
> > > > -	for (i = 0; i < psmouse->pktsize; i++)
> > > > -		printk("%s0x%02x ", i ? ", " : " ", psmouse->packet[i]);
> > > > -	printk("]\n");
> > > > +	psmouse_printk(KERN_DEBUG, psmouse, "PS/2 packet [%*ph]",
> > > > +		       psmouse->pktsize, psmouse->packet);
> > > >  }
> > > >  
> > > >  /*
> > > This should add a newline "\n" format termination but
> > I added it, thanks for alerting me.
> > 
> > > 
> > > how does the original produce multiple lines of output?
> > > 
> > > It looks more like psmouse_printk should be removed altogether
> > > and dev_debug should be used instead.
> > That does not produce output by default whereas elantech driver uses
> > elantech_packet_dump() to drop unexpected packets.
> 
> So use dev_printk(KERN_DEBUG, etc...

#define psmouse_printk(level, psmouse, format, ...)	\
	dev_printk(level,				\
		   &(psmouse)->ps2dev.serio->dev,	\
		   psmouse_fmt(format), ##__VA_ARGS__)

I want to see current protocol module in th eoutput and I do not want to
write "&(psmouse)->ps2dev.serio->dev" all the time.

> 
> 
> But I still don't see a mechanism for the original code to be
> emitted on multiple lines.

Message from another driver snuck in?

> 
> How long is the longest packet?
> 
> This might truncate it.

No, buffer is 8 bytes.

> 
> Should this be ratelimited?

So far there wasn't a need.

-- 
Dmitry

Re: [PATCH] Input - elantech: fix debug dump of the current packet

[Joe Perches]

On Thu, 2016-07-28 at 11:54 -0700, Dmitry Torokhov wrote:
> On Thu, Jul 28, 2016 at 11:49:57AM -0700, Joe Perches wrote:
[]
> > So use dev_printk(KERN_DEBUG, etc...
> #define psmouse_printk(level, psmouse, format, ...)	\
> 	dev_printk(level,				\
> 		   &(psmouse)->ps2dev.serio->dev,	\
> 		   psmouse_fmt(format), ##__VA_ARGS__)
> 
> I want to see current protocol module in th eoutput and I do not want to
> write "&(psmouse)->ps2dev.serio->dev" all the time.

There are only 2 uses of the macro.

$ git grep psmouse_printk
drivers/input/mouse/elantech.c:                 psmouse_printk(KERN_DEBUG, psmouse,             \
drivers/input/mouse/elantech.c: psmouse_printk(KERN_DEBUG, psmouse, "PS/2 packet [");
drivers/input/mouse/psmouse.h:#define psmouse_printk(level, psmouse, format, ...)       \

Re: [PATCH] Input - elantech: fix debug dump of the current packet

[Dmitry Torokhov]

On July 28, 2016 11:58:29 AM PDT, Joe Perches <joe@perches.com> wrote:
>On Thu, 2016-07-28 at 11:54 -0700, Dmitry Torokhov wrote:
>> On Thu, Jul 28, 2016 at 11:49:57AM -0700, Joe Perches wrote:
>[]
>> > So use dev_printk(KERN_DEBUG, etc...
>> #define psmouse_printk(level, psmouse, format, ...)	\
>> 	dev_printk(level,				\
>> 		   &(psmouse)->ps2dev.serio->dev,	\
>> 		   psmouse_fmt(format), ##__VA_ARGS__)
>> 
>> I want to see current protocol module in th eoutput and I do not want
>to
>> write "&(psmouse)->ps2dev.serio->dev" all the time.
>
>There are only 2 uses of the macro.
>
>$ git grep psmouse_printk
>drivers/input/mouse/elantech.c:                 psmouse_printk(KERN_DEBUG,
>psmouse,             \
>drivers/input/mouse/elantech.c: psmouse_printk(KERN_DEBUG, psmouse,
>"PS/2 packet [");
>drivers/input/mouse/psmouse.h:#define psmouse_printk(level, psmouse,
>format, ...)       \

Yes, but many more of psmouse_<severity> calls and I like consistency.


Thanks.

-- 
Dmitry