Re: [PATCH 2/7] tty: core: Add tty_debug() for printk(KERN_DEBUG) messages

2015-08-05 Thread Greg Kroah-Hartman 
On Wed, Aug 05, 2015 at 02:33:37PM -0400, Peter Hurley wrote:
> On 07/23/2015 09:35 PM, Greg Kroah-Hartman wrote:
> > On Sun, Jul 12, 2015 at 10:49:08PM -0400, Peter Hurley wrote:
> >> Introduce tty_debug() macro to output uniform debug information for
> >> tty core debug messages (function name and tty name).
> >>
> >> Note: printk(KERN_DEBUG) is retained here over pr_debug() since
> >> messages can be enabled in non-DEBUG builds.
> > 
> > But pr_debug() is the "standard" way to enable/disable debugging
> > messages, so I'd really like to see that be used here.
> 
> Ok, I can do that; I'll roll Joe's suggestions in at that time.
> 
> > Even better, this is a tty device, so it should be using dev_dbg(),
> > which gives us tons of good information built-in for the tty and can
> > properly be parsed by userspace tools to know exactly what device caused
> > what message at what point in time.
> > 
> > So I'll take this for now, but moving it to use dev_dbg() would be best
> > eventually.
> 
> The issue with using dev_dbg is that (SysV) ptys are not devices.

True :(

> However,
> if you'd prefer, I could rework this macro to format output like dev_dbg;
> ie.,  : 

That would be good, as that's what userspace is expecting the messages
to look like.

thanks,

greg k-h
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Re: [PATCH 2/7] tty: core: Add tty_debug() for printk(KERN_DEBUG) messages

2015-08-05 Thread Peter Hurley 
On 07/23/2015 09:35 PM, Greg Kroah-Hartman wrote:
> On Sun, Jul 12, 2015 at 10:49:08PM -0400, Peter Hurley wrote:
>> Introduce tty_debug() macro to output uniform debug information for
>> tty core debug messages (function name and tty name).
>>
>> Note: printk(KERN_DEBUG) is retained here over pr_debug() since
>> messages can be enabled in non-DEBUG builds.
> 
> But pr_debug() is the "standard" way to enable/disable debugging
> messages, so I'd really like to see that be used here.

Ok, I can do that; I'll roll Joe's suggestions in at that time.

> Even better, this is a tty device, so it should be using dev_dbg(),
> which gives us tons of good information built-in for the tty and can
> properly be parsed by userspace tools to know exactly what device caused
> what message at what point in time.
> 
> So I'll take this for now, but moving it to use dev_dbg() would be best
> eventually.

The issue with using dev_dbg is that (SysV) ptys are not devices. However,
if you'd prefer, I could rework this macro to format output like dev_dbg;
ie.,  : 

Regards,
Peter Hurley
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Re: [PATCH 2/7] tty: core: Add tty_debug() for printk(KERN_DEBUG) messages

2015-07-23 Thread Greg Kroah-Hartman 
On Sun, Jul 12, 2015 at 10:49:08PM -0400, Peter Hurley wrote:
> Introduce tty_debug() macro to output uniform debug information for
> tty core debug messages (function name and tty name).
> 
> Note: printk(KERN_DEBUG) is retained here over pr_debug() since
> messages can be enabled in non-DEBUG builds.

But pr_debug() is the "standard" way to enable/disable debugging
messages, so I'd really like to see that be used here.

Even better, this is a tty device, so it should be using dev_dbg(),
which gives us tons of good information built-in for the tty and can
properly be parsed by userspace tools to know exactly what device caused
what message at what point in time.

So I'll take this for now, but moving it to use dev_dbg() would be best
eventually.

thanks,

greg k-h
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Re: [PATCH 2/7] tty: core: Add tty_debug() for printk(KERN_DEBUG) messages

2015-07-12 Thread Peter Hurley 
On 07/13/2015 12:30 AM, Joe Perches wrote:
> On Mon, 2015-07-13 at 00:25 -0400, Peter Hurley wrote:
>> On 07/12/2015 11:47 PM, Joe Perches wrote:
>>> On Sun, 2015-07-12 at 22:49 -0400, Peter Hurley wrote:
 Introduce tty_debug() macro to output uniform debug information for
 tty core debug messages (function name and tty name).
>>> []
 diff --git a/drivers/tty/tty_io.c b/drivers/tty/tty_io.c
>>> []
 @@ -768,7 +768,7 @@ static void do_tty_hangup(struct work_struct *work)
  void tty_hangup(struct tty_struct *tty)
  {
  #ifdef TTY_DEBUG_HANGUP
 -  printk(KERN_DEBUG "%s hangup...\n", tty_name(tty));
 +  tty_debug(tty, "\n");
>>>
>>> Why drop the "hangup..." ?
>>
>> tty_debug() prints the function name; in this case, tty_hangup().
> 
> maybe that #ifdef/#endif block could/should be removed

The #ifdef/#endif block is removed in the follow-on patch 3/7;
replaced with tty_debug_hangup().

> and the function tracer used to track this instead.

One of the advantages of the single macro site of tty_debug is that
I can blow in trace_printk() instead when necessary. But still leave
mainline as printk's.

Regards,
Peter Hurley
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Re: [PATCH 2/7] tty: core: Add tty_debug() for printk(KERN_DEBUG) messages

2015-07-12 Thread Joe Perches 
On Mon, 2015-07-13 at 00:25 -0400, Peter Hurley wrote:
> On 07/12/2015 11:47 PM, Joe Perches wrote:
> > On Sun, 2015-07-12 at 22:49 -0400, Peter Hurley wrote:
> >> Introduce tty_debug() macro to output uniform debug information for
> >> tty core debug messages (function name and tty name).
> > []
> >> diff --git a/drivers/tty/tty_io.c b/drivers/tty/tty_io.c
> > []
> >> @@ -768,7 +768,7 @@ static void do_tty_hangup(struct work_struct *work)
> >>  void tty_hangup(struct tty_struct *tty)
> >>  {
> >>  #ifdef TTY_DEBUG_HANGUP
> >> -  printk(KERN_DEBUG "%s hangup...\n", tty_name(tty));
> >> +  tty_debug(tty, "\n");
> > 
> > Why drop the "hangup..." ?
> 
> tty_debug() prints the function name; in this case, tty_hangup().

maybe that #ifdef/#endif block could/should be removed
and the function tracer used to track this instead.

cheers, Joe

--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Re: [PATCH 2/7] tty: core: Add tty_debug() for printk(KERN_DEBUG) messages

2015-07-12 Thread Peter Hurley 
On 07/12/2015 11:47 PM, Joe Perches wrote:
> On Sun, 2015-07-12 at 22:49 -0400, Peter Hurley wrote:
>> Introduce tty_debug() macro to output uniform debug information for
>> tty core debug messages (function name and tty name).
> []
>> diff --git a/drivers/tty/tty_io.c b/drivers/tty/tty_io.c
> []
>> @@ -768,7 +768,7 @@ static void do_tty_hangup(struct work_struct *work)
>>  void tty_hangup(struct tty_struct *tty)
>>  {
>>  #ifdef TTY_DEBUG_HANGUP
>> -printk(KERN_DEBUG "%s hangup...\n", tty_name(tty));
>> +tty_debug(tty, "\n");
> 
> Why drop the "hangup..." ?

tty_debug() prints the function name; in this case, tty_hangup().


>> diff --git a/include/linux/tty.h b/include/linux/tty.h
> []
>> +#define tty_debug(tty, f, args...)  \
>> +do {\
>> +printk(KERN_DEBUG "%s: %s: " f, __func__,   \
>> +   tty_name(tty), ##args);  \
>> +} while (0)
> 
> Single statement macros don't need do {} while (0)

Ah, yep. Old hold-over from when tty_name() needed a temp buffer.


> #define fmt, ... 
> using fmt, ##__VA_ARGS__
> 
> is more common.

Ok.

Regards,
Peter Hurley
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Re: [PATCH 2/7] tty: core: Add tty_debug() for printk(KERN_DEBUG) messages

2015-07-12 Thread Joe Perches 
On Sun, 2015-07-12 at 22:49 -0400, Peter Hurley wrote:
> Introduce tty_debug() macro to output uniform debug information for
> tty core debug messages (function name and tty name).
[]
> diff --git a/drivers/tty/tty_io.c b/drivers/tty/tty_io.c
[]
> @@ -768,7 +768,7 @@ static void do_tty_hangup(struct work_struct *work)
>  void tty_hangup(struct tty_struct *tty)
>  {
>  #ifdef TTY_DEBUG_HANGUP
> - printk(KERN_DEBUG "%s hangup...\n", tty_name(tty));
> + tty_debug(tty, "\n");

Why drop the "hangup..." ?

> diff --git a/include/linux/tty.h b/include/linux/tty.h
[]
> +#define tty_debug(tty, f, args...)   \
> + do {\
> + printk(KERN_DEBUG "%s: %s: " f, __func__,   \
> +tty_name(tty), ##args);  \
> + } while (0)

Single statement macros don't need do {} while (0)

#define fmt, ... 
using fmt, ##__VA_ARGS__

is more common.

--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

[PATCH 2/7] tty: core: Add tty_debug() for printk(KERN_DEBUG) messages

2015-07-12 Thread Peter Hurley 
Introduce tty_debug() macro to output uniform debug information for
tty core debug messages (function name and tty name).

Note: printk(KERN_DEBUG) is retained here over pr_debug() since
messages can be enabled in non-DEBUG builds.

Signed-off-by: Peter Hurley 
---
 drivers/tty/tty_io.c | 41 +
 include/linux/tty.h  |  6 ++
 2 files changed, 23 insertions(+), 24 deletions(-)

diff --git a/drivers/tty/tty_io.c b/drivers/tty/tty_io.c
index 2d9811a..6de2c36 100644
--- a/drivers/tty/tty_io.c
+++ b/drivers/tty/tty_io.c
@@ -524,8 +524,8 @@ static void __proc_set_tty(struct tty_struct *tty)
spin_unlock_irqrestore(&tty->ctrl_lock, flags);
tty->session = get_pid(task_session(current));
if (current->signal->tty) {
-   printk(KERN_DEBUG "%s: %s: current tty %s not NULL!!\n",
-  __func__, tty->name, current->signal->tty->name);
+   tty_debug(tty, "current tty %s not NULL!!\n",
+ current->signal->tty->name);
tty_kref_put(current->signal->tty);
}
put_pid(current->signal->tty_old_pgrp);
@@ -768,7 +768,7 @@ static void do_tty_hangup(struct work_struct *work)
 void tty_hangup(struct tty_struct *tty)
 {
 #ifdef TTY_DEBUG_HANGUP
-   printk(KERN_DEBUG "%s hangup...\n", tty_name(tty));
+   tty_debug(tty, "\n");
 #endif
schedule_work(&tty->hangup_work);
 }
@@ -787,7 +787,7 @@ EXPORT_SYMBOL(tty_hangup);
 void tty_vhangup(struct tty_struct *tty)
 {
 #ifdef TTY_DEBUG_HANGUP
-   printk(KERN_DEBUG "%s vhangup...\n", tty_name(tty));
+   tty_debug(tty, "\n")
 #endif
__tty_hangup(tty, 0);
 }
@@ -826,7 +826,7 @@ void tty_vhangup_self(void)
 static void tty_vhangup_session(struct tty_struct *tty)
 {
 #ifdef TTY_DEBUG_HANGUP
-   printk(KERN_DEBUG "%s vhangup session...\n", tty_name(tty));
+   tty_debug(tty, "\n");
 #endif
__tty_hangup(tty, 1);
 }
@@ -923,7 +923,7 @@ void disassociate_ctty(int on_exit)
tty_kref_put(tty);
} else {
 #ifdef TTY_DEBUG_HANGUP
-   printk(KERN_DEBUG "%s: no current tty\n", __func__);
+   tty_debug(tty, "no current tty\n");
 #endif
}
 
@@ -1705,8 +1705,7 @@ static int tty_release_checks(struct tty_struct *tty, int 
idx)
 {
 #ifdef TTY_PARANOIA_CHECK
if (idx < 0 || idx >= tty->driver->num) {
-   printk(KERN_DEBUG "%s: %s: bad idx %d\n",
-   __func__, tty->name, idx);
+   tty_debug(tty, "bad idx %d\n", idx);
return -1;
}
 
@@ -1715,22 +1714,20 @@ static int tty_release_checks(struct tty_struct *tty, 
int idx)
return 0;
 
if (tty != tty->driver->ttys[idx]) {
-   printk(KERN_DEBUG "%s: %s: bad driver table[%d] = %p\n",
-  __func__, tty->name, idx, tty->driver->ttys[idx]);
+   tty_debug(tty, "bad driver table[%d] = %p\n",
+ idx, tty->driver->ttys[idx]);
return -1;
}
if (tty->driver->other) {
struct tty_struct *o_tty = tty->link;
 
if (o_tty != tty->driver->other->ttys[idx]) {
-   printk(KERN_DEBUG "%s: %s: bad other table[%d] = %p\n",
-  __func__, tty->name, idx,
-  tty->driver->other->ttys[idx]);
+   tty_debug(tty, "bad other table[%d] = %p\n",
+ idx, tty->driver->other->ttys[idx]);
return -1;
}
if (o_tty->link != tty) {
-   printk(KERN_DEBUG "%s: %s: bad link = %p\n",
-  __func__, tty->name, o_tty->link);
+   tty_debug(tty, "bad link = %p\n", o_tty->link);
return -1;
}
}
@@ -1785,8 +1782,7 @@ int tty_release(struct inode *inode, struct file *filp)
}
 
 #ifdef TTY_DEBUG_HANGUP
-   printk(KERN_DEBUG "%s: %s (tty count=%d)...\n", __func__,
-   tty_name(tty), tty->count);
+   tty_debug(tty, "(tty count=%d)...\n", tty->count);
 #endif
 
if (tty->ops->close)
@@ -1898,7 +1894,7 @@ int tty_release(struct inode *inode, struct file *filp)
return 0;
 
 #ifdef TTY_DEBUG_HANGUP
-   printk(KERN_DEBUG "%s: %s: final close\n", __func__, tty_name(tty));
+   tty_debug(tty, "final close\n");
 #endif
/*
 * Ask the line discipline code to release its structures
@@ -1909,8 +1905,7 @@ int tty_release(struct inode *inode, struct file *filp)
tty_flush_works(tty);
 
 #ifdef TTY_DEBUG_HANGUP
-   printk(KERN_DEBUG "%s: %s: freeing structure...\n", __func__,
-  tty_name(tty));
+   tty_debug(tty, "freeing structure...\n");
 #endif
/*
 * The release_tty function takes care of the details of clearing
@@ -2101,8 +2096,7 @@ retry