[PATCH] dynamic_debug: allow to work if debugfs is disabled

[PATCH] dynamic_debug: allow to work if debugfs is disabled

[Greg Kroah-Hartman]

With the realization that having debugfs enabled on "production" systems is
generally not a good idea, debugfs is being disabled from more and more
platforms over time.  However, the functionality of dynamic debugging still is
needed at times, and since it relies on debugfs for its user api, having
debugfs disabled also forces dynamic debug to be disabled.

To get around this, move the "control" file for dynamic_debug to procfs IFF
debugfs is disabled.  This lets people turn on debugging as needed at runtime
for individual driverfs and subsystems.

Reported-by: many different companies
Cc: Jason Baron <jbaron@akamai.com>
Signed-off-by: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
---
 .../admin-guide/dynamic-debug-howto.rst         |  3 +++
 lib/Kconfig.debug                               |  2 +-
 lib/dynamic_debug.c                             | 17 ++++++++++++++---
 3 files changed, 18 insertions(+), 4 deletions(-)

diff --git a/Documentation/admin-guide/dynamic-debug-howto.rst b/Documentation/admin-guide/dynamic-debug-howto.rst
index 252e5ef324e5..41f43a373a6a 100644
--- a/Documentation/admin-guide/dynamic-debug-howto.rst
+++ b/Documentation/admin-guide/dynamic-debug-howto.rst
@@ -54,6 +54,9 @@ If you make a mistake with the syntax, the write will fail thus::
 				<debugfs>/dynamic_debug/control
   -bash: echo: write error: Invalid argument
 
+Note, for systems without 'debugfs' enabled, the control file can be
+also found in ``/proc/dynamic_debug/control``.
+
 Viewing Dynamic Debug Behaviour
 ===============================
 
diff --git a/lib/Kconfig.debug b/lib/Kconfig.debug
index 5ffe144c9794..01d4add8b963 100644
--- a/lib/Kconfig.debug
+++ b/lib/Kconfig.debug
@@ -98,7 +98,7 @@ config DYNAMIC_DEBUG
 	bool "Enable dynamic printk() support"
 	default n
 	depends on PRINTK
-	depends on DEBUG_FS
+	depends on (DEBUG_FS || PROC_FS)
 	help
 
 	  Compiles debug level messages into the kernel, which would not
diff --git a/lib/dynamic_debug.c b/lib/dynamic_debug.c
index c60409138e13..077b2d6623ac 100644
--- a/lib/dynamic_debug.c
+++ b/lib/dynamic_debug.c
@@ -993,13 +993,24 @@ static __initdata int ddebug_init_success;
 
 static int __init dynamic_debug_init_debugfs(void)
 {
-	struct dentry *dir;
+	struct dentry *debugfs_dir;
+	struct proc_dir_entry *procfs_dir;
 
 	if (!ddebug_init_success)
 		return -ENODEV;
 
-	dir = debugfs_create_dir("dynamic_debug", NULL);
-	debugfs_create_file("control", 0644, dir, NULL, &ddebug_proc_fops);
+	/* Create the control file in debugfs if it is enabled */
+	if (debugfs_initialized) {
+		debugfs_dir = debugfs_create_dir("dynamic_debug", NULL);
+		debugfs_create_file("control", 0644, debugfs_dir, NULL,
+				    &ddebug_proc_fops);
+		return 0;
+	}
+
+	/* No debugfs so put it in procfs instead */
+	procfs_dir = proc_mkdir("dynamic_debug", NULL);
+	if (procfs_dir)
+		proc_create("control", 0x644, procfs_dir, &ddebug_proc_fops);
 
 	return 0;
 }
-- 
2.25.0

Re: [PATCH] dynamic_debug: allow to work if debugfs is disabled

[Will Deacon]

On Wed, Jan 22, 2020 at 08:43:43AM +0100, Greg Kroah-Hartman wrote:
> With the realization that having debugfs enabled on "production" systems is
> generally not a good idea, debugfs is being disabled from more and more
> platforms over time.  However, the functionality of dynamic debugging still is
> needed at times, and since it relies on debugfs for its user api, having
> debugfs disabled also forces dynamic debug to be disabled.

Why is the dyndbg= command-line option not sufficient for these use-cases?

> To get around this, move the "control" file for dynamic_debug to procfs IFF
> debugfs is disabled.  This lets people turn on debugging as needed at runtime
> for individual driverfs and subsystems.

Hmm. If something called "dynamic_debug" is getting moved out of debugfs,
this does raise the question as to what (if anything) should be left behind.
I worry this is a bit of a slippery slope...

Anywho, comments below.

> Reported-by: many different companies
> Cc: Jason Baron <jbaron@akamai.com>
> Signed-off-by: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
> ---
>  .../admin-guide/dynamic-debug-howto.rst         |  3 +++
>  lib/Kconfig.debug                               |  2 +-
>  lib/dynamic_debug.c                             | 17 ++++++++++++++---
>  3 files changed, 18 insertions(+), 4 deletions(-)
> 
> diff --git a/Documentation/admin-guide/dynamic-debug-howto.rst b/Documentation/admin-guide/dynamic-debug-howto.rst
> index 252e5ef324e5..41f43a373a6a 100644
> --- a/Documentation/admin-guide/dynamic-debug-howto.rst
> +++ b/Documentation/admin-guide/dynamic-debug-howto.rst
> @@ -54,6 +54,9 @@ If you make a mistake with the syntax, the write will fail thus::
>  				<debugfs>/dynamic_debug/control
>    -bash: echo: write error: Invalid argument
>  
> +Note, for systems without 'debugfs' enabled, the control file can be
> +also found in ``/proc/dynamic_debug/control``.
> +
>  Viewing Dynamic Debug Behaviour
>  ===============================
>  
> diff --git a/lib/Kconfig.debug b/lib/Kconfig.debug
> index 5ffe144c9794..01d4add8b963 100644
> --- a/lib/Kconfig.debug
> +++ b/lib/Kconfig.debug
> @@ -98,7 +98,7 @@ config DYNAMIC_DEBUG
>  	bool "Enable dynamic printk() support"
>  	default n
>  	depends on PRINTK
> -	depends on DEBUG_FS
> +	depends on (DEBUG_FS || PROC_FS)
>  	help
>  
>  	  Compiles debug level messages into the kernel, which would noti

The help text here also needs updating, since it refers to debugfs.

> diff --git a/lib/dynamic_debug.c b/lib/dynamic_debug.c
> index c60409138e13..077b2d6623ac 100644
> --- a/lib/dynamic_debug.c
> +++ b/lib/dynamic_debug.c
> @@ -993,13 +993,24 @@ static __initdata int ddebug_init_success;
>  
>  static int __init dynamic_debug_init_debugfs(void)
>  {
> -	struct dentry *dir;
> +	struct dentry *debugfs_dir;
> +	struct proc_dir_entry *procfs_dir;
>  
>  	if (!ddebug_init_success)
>  		return -ENODEV;
>  
> -	dir = debugfs_create_dir("dynamic_debug", NULL);
> -	debugfs_create_file("control", 0644, dir, NULL, &ddebug_proc_fops);
> +	/* Create the control file in debugfs if it is enabled */
> +	if (debugfs_initialized) {
> +		debugfs_dir = debugfs_create_dir("dynamic_debug", NULL);
> +		debugfs_create_file("control", 0644, debugfs_dir, NULL,
> +				    &ddebug_proc_fops);
> +		return 0;
> +	}
> +
> +	/* No debugfs so put it in procfs instead */
> +	procfs_dir = proc_mkdir("dynamic_debug", NULL);
> +	if (procfs_dir)
> +		proc_create("control", 0x644, procfs_dir, &ddebug_proc_fops);

Shouldn't this be octal rather than hex? Even then, I don't understand what
use it is being able to read but not write to this file. Perhaps make it
0600 for /proc ?

Will

Re: [PATCH] dynamic_debug: allow to work if debugfs is disabled

[Greg Kroah-Hartman]

On Wed, Jan 22, 2020 at 08:03:53AM +0000, Will Deacon wrote:
> On Wed, Jan 22, 2020 at 08:43:43AM +0100, Greg Kroah-Hartman wrote:
> > With the realization that having debugfs enabled on "production" systems is
> > generally not a good idea, debugfs is being disabled from more and more
> > platforms over time.  However, the functionality of dynamic debugging still is
> > needed at times, and since it relies on debugfs for its user api, having
> > debugfs disabled also forces dynamic debug to be disabled.
> 
> Why is the dyndbg= command-line option not sufficient for these use-cases?

They want to enable things after booting, and changing the kernel
command line is not something you can do on many systems (i.e.
locked-down-bootloaders like embedded systems).

Also, the whole option is prevented to be booted if debugfs is not
enabled, so the command line wouldn't even work in that situation :)

> > To get around this, move the "control" file for dynamic_debug to procfs IFF
> > debugfs is disabled.  This lets people turn on debugging as needed at runtime
> > for individual driverfs and subsystems.
> 
> Hmm. If something called "dynamic_debug" is getting moved out of debugfs,
> this does raise the question as to what (if anything) should be left behind.
> I worry this is a bit of a slippery slope...

I totally agree, but dynamic_debug is independant of debugfs with the
exception of the control file itself.

> > Reported-by: many different companies
> > Cc: Jason Baron <jbaron@akamai.com>
> > Signed-off-by: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
> > ---
> >  .../admin-guide/dynamic-debug-howto.rst         |  3 +++
> >  lib/Kconfig.debug                               |  2 +-
> >  lib/dynamic_debug.c                             | 17 ++++++++++++++---
> >  3 files changed, 18 insertions(+), 4 deletions(-)
> > 
> > diff --git a/Documentation/admin-guide/dynamic-debug-howto.rst b/Documentation/admin-guide/dynamic-debug-howto.rst
> > index 252e5ef324e5..41f43a373a6a 100644
> > --- a/Documentation/admin-guide/dynamic-debug-howto.rst
> > +++ b/Documentation/admin-guide/dynamic-debug-howto.rst
> > @@ -54,6 +54,9 @@ If you make a mistake with the syntax, the write will fail thus::
> >  				<debugfs>/dynamic_debug/control
> >    -bash: echo: write error: Invalid argument
> >  
> > +Note, for systems without 'debugfs' enabled, the control file can be
> > +also found in ``/proc/dynamic_debug/control``.
> > +
> >  Viewing Dynamic Debug Behaviour
> >  ===============================
> >  
> > diff --git a/lib/Kconfig.debug b/lib/Kconfig.debug
> > index 5ffe144c9794..01d4add8b963 100644
> > --- a/lib/Kconfig.debug
> > +++ b/lib/Kconfig.debug
> > @@ -98,7 +98,7 @@ config DYNAMIC_DEBUG
> >  	bool "Enable dynamic printk() support"
> >  	default n
> >  	depends on PRINTK
> > -	depends on DEBUG_FS
> > +	depends on (DEBUG_FS || PROC_FS)
> >  	help
> >  
> >  	  Compiles debug level messages into the kernel, which would noti
> 
> The help text here also needs updating, since it refers to debugfs.

Oops, missed that, thanks!

> > diff --git a/lib/dynamic_debug.c b/lib/dynamic_debug.c
> > index c60409138e13..077b2d6623ac 100644
> > --- a/lib/dynamic_debug.c
> > +++ b/lib/dynamic_debug.c
> > @@ -993,13 +993,24 @@ static __initdata int ddebug_init_success;
> >  
> >  static int __init dynamic_debug_init_debugfs(void)
> >  {
> > -	struct dentry *dir;
> > +	struct dentry *debugfs_dir;
> > +	struct proc_dir_entry *procfs_dir;
> >  
> >  	if (!ddebug_init_success)
> >  		return -ENODEV;
> >  
> > -	dir = debugfs_create_dir("dynamic_debug", NULL);
> > -	debugfs_create_file("control", 0644, dir, NULL, &ddebug_proc_fops);
> > +	/* Create the control file in debugfs if it is enabled */
> > +	if (debugfs_initialized) {
> > +		debugfs_dir = debugfs_create_dir("dynamic_debug", NULL);
> > +		debugfs_create_file("control", 0644, debugfs_dir, NULL,
> > +				    &ddebug_proc_fops);
> > +		return 0;
> > +	}
> > +
> > +	/* No debugfs so put it in procfs instead */
> > +	procfs_dir = proc_mkdir("dynamic_debug", NULL);
> > +	if (procfs_dir)
> > +		proc_create("control", 0x644, procfs_dir, &ddebug_proc_fops);
> 
> Shouldn't this be octal rather than hex? Even then, I don't understand what
> use it is being able to read but not write to this file. Perhaps make it
> 0600 for /proc ?

Argh, my fault, fingers are used to typing hex.

You can read from the file to see what the current settings are, I was
just trying to mirror the debugfs permissions.

thanks,

greg k-h

[PATCH v2] dynamic_debug: allow to work if debugfs is disabled

[Greg Kroah-Hartman]

With the realization that having debugfs enabled on "production" systems is
generally not a good idea, debugfs is being disabled from more and more
platforms over time.  However, the functionality of dynamic debugging still is
needed at times, and since it relies on debugfs for its user api, having
debugfs disabled also forces dynamic debug to be disabled.

To get around this, move the "control" file for dynamic_debug to procfs IFF
debugfs is disabled.  This lets people turn on debugging as needed at runtime
for individual driverfs and subsystems.

Reported-by: many different companies
Cc: Jason Baron <jbaron@akamai.com>
Signed-off-by: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
---
v2: Fix up octal permissions and add procfs reference to the Kconfig
    entry, thanks to Will for the review.

 .../admin-guide/dynamic-debug-howto.rst         |  3 +++
 lib/Kconfig.debug                               |  7 ++++---
 lib/dynamic_debug.c                             | 17 ++++++++++++++---
 3 files changed, 21 insertions(+), 6 deletions(-)

diff --git a/Documentation/admin-guide/dynamic-debug-howto.rst b/Documentation/admin-guide/dynamic-debug-howto.rst
index 252e5ef324e5..41f43a373a6a 100644
--- a/Documentation/admin-guide/dynamic-debug-howto.rst
+++ b/Documentation/admin-guide/dynamic-debug-howto.rst
@@ -54,6 +54,9 @@ If you make a mistake with the syntax, the write will fail thus::
 				<debugfs>/dynamic_debug/control
   -bash: echo: write error: Invalid argument
 
+Note, for systems without 'debugfs' enabled, the control file can be
+also found in ``/proc/dynamic_debug/control``.
+
 Viewing Dynamic Debug Behaviour
 ===============================
 
diff --git a/lib/Kconfig.debug b/lib/Kconfig.debug
index 5ffe144c9794..49980eb8c18e 100644
--- a/lib/Kconfig.debug
+++ b/lib/Kconfig.debug
@@ -98,7 +98,7 @@ config DYNAMIC_DEBUG
 	bool "Enable dynamic printk() support"
 	default n
 	depends on PRINTK
-	depends on DEBUG_FS
+	depends on (DEBUG_FS || PROC_FS)
 	help
 
 	  Compiles debug level messages into the kernel, which would not
@@ -116,8 +116,9 @@ config DYNAMIC_DEBUG
 	  Usage:
 
 	  Dynamic debugging is controlled via the 'dynamic_debug/control' file,
-	  which is contained in the 'debugfs' filesystem. Thus, the debugfs
-	  filesystem must first be mounted before making use of this feature.
+	  which is contained in the 'debugfs' filesystem or procfs if
+	  debugfs is not present. Thus, the debugfs or procfs filesystem
+	  must first be mounted before making use of this feature.
 	  We refer the control file as: <debugfs>/dynamic_debug/control. This
 	  file contains a list of the debug statements that can be enabled. The
 	  format for each line of the file is:
diff --git a/lib/dynamic_debug.c b/lib/dynamic_debug.c
index c60409138e13..0f1b26f10fb2 100644
--- a/lib/dynamic_debug.c
+++ b/lib/dynamic_debug.c
@@ -993,13 +993,24 @@ static __initdata int ddebug_init_success;
 
 static int __init dynamic_debug_init_debugfs(void)
 {
-	struct dentry *dir;
+	struct dentry *debugfs_dir;
+	struct proc_dir_entry *procfs_dir;
 
 	if (!ddebug_init_success)
 		return -ENODEV;
 
-	dir = debugfs_create_dir("dynamic_debug", NULL);
-	debugfs_create_file("control", 0644, dir, NULL, &ddebug_proc_fops);
+	/* Create the control file in debugfs if it is enabled */
+	if (debugfs_initialized) {
+		debugfs_dir = debugfs_create_dir("dynamic_debug", NULL);
+		debugfs_create_file("control", 0644, debugfs_dir, NULL,
+				    &ddebug_proc_fops);
+		return 0;
+	}
+
+	/* No debugfs so put it in procfs instead */
+	procfs_dir = proc_mkdir("dynamic_debug", NULL);
+	if (procfs_dir)
+		proc_create("control", 0644, procfs_dir, &ddebug_proc_fops);
 
 	return 0;
 }
-- 
2.25.0

Re: [PATCH v2] dynamic_debug: allow to work if debugfs is disabled

[Jason Baron]

On 1/22/20 8:53 AM, Greg Kroah-Hartman wrote:
> 
> With the realization that having debugfs enabled on "production" systems is
> generally not a good idea, debugfs is being disabled from more and more
> platforms over time.  However, the functionality of dynamic debugging still is
> needed at times, and since it relies on debugfs for its user api, having
> debugfs disabled also forces dynamic debug to be disabled.
> 
> To get around this, move the "control" file for dynamic_debug to procfs IFF
> debugfs is disabled.  This lets people turn on debugging as needed at runtime
> for individual driverfs and subsystems.
> 

Hi Greg,

Thanks for updating this. Just a comment below.


> Reported-by: many different companies
> Cc: Jason Baron <jbaron@akamai.com>
> Signed-off-by: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
> ---
> v2: Fix up octal permissions and add procfs reference to the Kconfig
>     entry, thanks to Will for the review.
> 
>  .../admin-guide/dynamic-debug-howto.rst         |  3 +++
>  lib/Kconfig.debug                               |  7 ++++---
>  lib/dynamic_debug.c                             | 17 ++++++++++++++---
>  3 files changed, 21 insertions(+), 6 deletions(-)
> 
> diff --git a/Documentation/admin-guide/dynamic-debug-howto.rst b/Documentation/admin-guide/dynamic-debug-howto.rst
> index 252e5ef324e5..41f43a373a6a 100644
> --- a/Documentation/admin-guide/dynamic-debug-howto.rst
> +++ b/Documentation/admin-guide/dynamic-debug-howto.rst
> @@ -54,6 +54,9 @@ If you make a mistake with the syntax, the write will fail thus::
>  				<debugfs>/dynamic_debug/control
>    -bash: echo: write error: Invalid argument
>  
> +Note, for systems without 'debugfs' enabled, the control file can be
> +also found in ``/proc/dynamic_debug/control``.
> +
>  Viewing Dynamic Debug Behaviour
>  ===============================
>  
> diff --git a/lib/Kconfig.debug b/lib/Kconfig.debug
> index 5ffe144c9794..49980eb8c18e 100644
> --- a/lib/Kconfig.debug
> +++ b/lib/Kconfig.debug
> @@ -98,7 +98,7 @@ config DYNAMIC_DEBUG
>  	bool "Enable dynamic printk() support"
>  	default n
>  	depends on PRINTK
> -	depends on DEBUG_FS
> +	depends on (DEBUG_FS || PROC_FS)
>  	help
>  
>  	  Compiles debug level messages into the kernel, which would not
> @@ -116,8 +116,9 @@ config DYNAMIC_DEBUG
>  	  Usage:
>  
>  	  Dynamic debugging is controlled via the 'dynamic_debug/control' file,
> -	  which is contained in the 'debugfs' filesystem. Thus, the debugfs
> -	  filesystem must first be mounted before making use of this feature.
> +	  which is contained in the 'debugfs' filesystem or procfs if
> +	  debugfs is not present. Thus, the debugfs or procfs filesystem
> +	  must first be mounted before making use of this feature.
>  	  We refer the control file as: <debugfs>/dynamic_debug/control. This
>  	  file contains a list of the debug statements that can be enabled. The
>  	  format for each line of the file is:
> diff --git a/lib/dynamic_debug.c b/lib/dynamic_debug.c
> index c60409138e13..0f1b26f10fb2 100644
> --- a/lib/dynamic_debug.c
> +++ b/lib/dynamic_debug.c
> @@ -993,13 +993,24 @@ static __initdata int ddebug_init_success;
>  
>  static int __init dynamic_debug_init_debugfs(void)
>  {

The naming now is a little confusing - dynamic_debug_init_control ?


> -	struct dentry *dir;
> +	struct dentry *debugfs_dir;
> +	struct proc_dir_entry *procfs_dir;
>  
>  	if (!ddebug_init_success)
>  		return -ENODEV;
>  
> -	dir = debugfs_create_dir("dynamic_debug", NULL);
> -	debugfs_create_file("control", 0644, dir, NULL, &ddebug_proc_fops);
> +	/* Create the control file in debugfs if it is enabled */
> +	if (debugfs_initialized) {
> +		debugfs_dir = debugfs_create_dir("dynamic_debug", NULL);
> +		debugfs_create_file("control", 0644, debugfs_dir, NULL,
> +				    &ddebug_proc_fops);
> +		return 0;
> +	}
> +
> +	/* No debugfs so put it in procfs instead */
> +	procfs_dir = proc_mkdir("dynamic_debug", NULL);
> +	if (procfs_dir)
> +		proc_create("control", 0644, procfs_dir, &ddebug_proc_fops);
>  
>  	return 0;
>  }
> 

Re: [PATCH v2] dynamic_debug: allow to work if debugfs is disabled

[Greg Kroah-Hartman]

On Wed, Jan 22, 2020 at 01:56:48PM -0500, Jason Baron wrote:
> 
> 
> On 1/22/20 8:53 AM, Greg Kroah-Hartman wrote:
> > 
> > With the realization that having debugfs enabled on "production" systems is
> > generally not a good idea, debugfs is being disabled from more and more
> > platforms over time.  However, the functionality of dynamic debugging still is
> > needed at times, and since it relies on debugfs for its user api, having
> > debugfs disabled also forces dynamic debug to be disabled.
> > 
> > To get around this, move the "control" file for dynamic_debug to procfs IFF
> > debugfs is disabled.  This lets people turn on debugging as needed at runtime
> > for individual driverfs and subsystems.
> > 
> 
> Hi Greg,
> 
> Thanks for updating this. Just a comment below.
> 
> 
> > Reported-by: many different companies
> > Cc: Jason Baron <jbaron@akamai.com>
> > Signed-off-by: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
> > ---
> > v2: Fix up octal permissions and add procfs reference to the Kconfig
> >     entry, thanks to Will for the review.
> > 
> >  .../admin-guide/dynamic-debug-howto.rst         |  3 +++
> >  lib/Kconfig.debug                               |  7 ++++---
> >  lib/dynamic_debug.c                             | 17 ++++++++++++++---
> >  3 files changed, 21 insertions(+), 6 deletions(-)
> > 
> > diff --git a/Documentation/admin-guide/dynamic-debug-howto.rst b/Documentation/admin-guide/dynamic-debug-howto.rst
> > index 252e5ef324e5..41f43a373a6a 100644
> > --- a/Documentation/admin-guide/dynamic-debug-howto.rst
> > +++ b/Documentation/admin-guide/dynamic-debug-howto.rst
> > @@ -54,6 +54,9 @@ If you make a mistake with the syntax, the write will fail thus::
> >  				<debugfs>/dynamic_debug/control
> >    -bash: echo: write error: Invalid argument
> >  
> > +Note, for systems without 'debugfs' enabled, the control file can be
> > +also found in ``/proc/dynamic_debug/control``.
> > +
> >  Viewing Dynamic Debug Behaviour
> >  ===============================
> >  
> > diff --git a/lib/Kconfig.debug b/lib/Kconfig.debug
> > index 5ffe144c9794..49980eb8c18e 100644
> > --- a/lib/Kconfig.debug
> > +++ b/lib/Kconfig.debug
> > @@ -98,7 +98,7 @@ config DYNAMIC_DEBUG
> >  	bool "Enable dynamic printk() support"
> >  	default n
> >  	depends on PRINTK
> > -	depends on DEBUG_FS
> > +	depends on (DEBUG_FS || PROC_FS)
> >  	help
> >  
> >  	  Compiles debug level messages into the kernel, which would not
> > @@ -116,8 +116,9 @@ config DYNAMIC_DEBUG
> >  	  Usage:
> >  
> >  	  Dynamic debugging is controlled via the 'dynamic_debug/control' file,
> > -	  which is contained in the 'debugfs' filesystem. Thus, the debugfs
> > -	  filesystem must first be mounted before making use of this feature.
> > +	  which is contained in the 'debugfs' filesystem or procfs if
> > +	  debugfs is not present. Thus, the debugfs or procfs filesystem
> > +	  must first be mounted before making use of this feature.
> >  	  We refer the control file as: <debugfs>/dynamic_debug/control. This
> >  	  file contains a list of the debug statements that can be enabled. The
> >  	  format for each line of the file is:
> > diff --git a/lib/dynamic_debug.c b/lib/dynamic_debug.c
> > index c60409138e13..0f1b26f10fb2 100644
> > --- a/lib/dynamic_debug.c
> > +++ b/lib/dynamic_debug.c
> > @@ -993,13 +993,24 @@ static __initdata int ddebug_init_success;
> >  
> >  static int __init dynamic_debug_init_debugfs(void)
> >  {
> 
> The naming now is a little confusing - dynamic_debug_init_control ?

Ah, good point.  I'll go fix that up, and fix up the foolish build
warning that I missed...

thanks,

greg k-h

[PATCH v3] dynamic_debug: allow to work if debugfs is disabled

[Greg Kroah-Hartman]

With the realization that having debugfs enabled on "production" systems is
generally not a good idea, debugfs is being disabled from more and more
platforms over time.  However, the functionality of dynamic debugging still is
needed at times, and since it relies on debugfs for its user api, having
debugfs disabled also forces dynamic debug to be disabled.

To get around this, move the "control" file for dynamic_debug to procfs IFF
debugfs is disabled.  This lets people turn on debugging as needed at runtime
for individual driverfs and subsystems.

Reported-by: many different companies
Cc: Jason Baron <jbaron@akamai.com>
Signed-off-by: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
---
v3: rename init function as it is now no longer just for debugfs, thanks
    to Jason for the review.
    Fix build warning for debugfs_initialized call.
v2: Fix up octal permissions and add procfs reference to the Kconfig
    entry, thanks to Will for the review.


 .../admin-guide/dynamic-debug-howto.rst       |  3 +++
 lib/Kconfig.debug                             |  7 ++++---
 lib/dynamic_debug.c                           | 21 ++++++++++++++-----
 3 files changed, 23 insertions(+), 8 deletions(-)

diff --git a/Documentation/admin-guide/dynamic-debug-howto.rst b/Documentation/admin-guide/dynamic-debug-howto.rst
index 252e5ef324e5..41f43a373a6a 100644
--- a/Documentation/admin-guide/dynamic-debug-howto.rst
+++ b/Documentation/admin-guide/dynamic-debug-howto.rst
@@ -54,6 +54,9 @@ If you make a mistake with the syntax, the write will fail thus::
 				<debugfs>/dynamic_debug/control
   -bash: echo: write error: Invalid argument
 
+Note, for systems without 'debugfs' enabled, the control file can be
+also found in ``/proc/dynamic_debug/control``.
+
 Viewing Dynamic Debug Behaviour
 ===============================
 
diff --git a/lib/Kconfig.debug b/lib/Kconfig.debug
index 5ffe144c9794..49980eb8c18e 100644
--- a/lib/Kconfig.debug
+++ b/lib/Kconfig.debug
@@ -98,7 +98,7 @@ config DYNAMIC_DEBUG
 	bool "Enable dynamic printk() support"
 	default n
 	depends on PRINTK
-	depends on DEBUG_FS
+	depends on (DEBUG_FS || PROC_FS)
 	help
 
 	  Compiles debug level messages into the kernel, which would not
@@ -116,8 +116,9 @@ config DYNAMIC_DEBUG
 	  Usage:
 
 	  Dynamic debugging is controlled via the 'dynamic_debug/control' file,
-	  which is contained in the 'debugfs' filesystem. Thus, the debugfs
-	  filesystem must first be mounted before making use of this feature.
+	  which is contained in the 'debugfs' filesystem or procfs if
+	  debugfs is not present. Thus, the debugfs or procfs filesystem
+	  must first be mounted before making use of this feature.
 	  We refer the control file as: <debugfs>/dynamic_debug/control. This
 	  file contains a list of the debug statements that can be enabled. The
 	  format for each line of the file is:
diff --git a/lib/dynamic_debug.c b/lib/dynamic_debug.c
index c60409138e13..118e928cdbda 100644
--- a/lib/dynamic_debug.c
+++ b/lib/dynamic_debug.c
@@ -991,15 +991,26 @@ static void ddebug_remove_all_tables(void)
 
 static __initdata int ddebug_init_success;
 
-static int __init dynamic_debug_init_debugfs(void)
+static int __init dynamic_debug_init_control(void)
 {
-	struct dentry *dir;
+	struct proc_dir_entry *procfs_dir;
+	struct dentry *debugfs_dir;
 
 	if (!ddebug_init_success)
 		return -ENODEV;
 
-	dir = debugfs_create_dir("dynamic_debug", NULL);
-	debugfs_create_file("control", 0644, dir, NULL, &ddebug_proc_fops);
+	/* Create the control file in debugfs if it is enabled */
+	if (debugfs_initialized()) {
+		debugfs_dir = debugfs_create_dir("dynamic_debug", NULL);
+		debugfs_create_file("control", 0644, debugfs_dir, NULL,
+				    &ddebug_proc_fops);
+		return 0;
+	}
+
+	/* No debugfs so put it in procfs instead */
+	procfs_dir = proc_mkdir("dynamic_debug", NULL);
+	if (procfs_dir)
+		proc_create("control", 0644, procfs_dir, &ddebug_proc_fops);
 
 	return 0;
 }
@@ -1077,4 +1088,4 @@ static int __init dynamic_debug_init(void)
 early_initcall(dynamic_debug_init);
 
 /* Debugfs setup must be done later */
-fs_initcall(dynamic_debug_init_debugfs);
+fs_initcall(dynamic_debug_init_control);
-- 
2.25.0

Re: [PATCH v3] dynamic_debug: allow to work if debugfs is disabled

[Randy Dunlap]

Hi Greg,

If you make any more changes:

On 1/22/20 11:31 AM, Greg Kroah-Hartman wrote:
> diff --git a/Documentation/admin-guide/dynamic-debug-howto.rst b/Documentation/admin-guide/dynamic-debug-howto.rst
> index 252e5ef324e5..41f43a373a6a 100644
> --- a/Documentation/admin-guide/dynamic-debug-howto.rst
> +++ b/Documentation/admin-guide/dynamic-debug-howto.rst
> @@ -54,6 +54,9 @@ If you make a mistake with the syntax, the write will fail thus::
>  				<debugfs>/dynamic_debug/control
>    -bash: echo: write error: Invalid argument
>  
> +Note, for systems without 'debugfs' enabled, the control file can be
> +also found in ``/proc/dynamic_debug/control``.

Mostly drop the "also".  How about:

> +Note, for systems without 'debugfs' enabled, the control file is located
> +in ``/proc/dynamic_debug/control``.


> +
>  Viewing Dynamic Debug Behaviour
>  ===============================
>  


-- 
~Randy

Re: [PATCH v3] dynamic_debug: allow to work if debugfs is disabled

[Greg Kroah-Hartman]

On Wed, Jan 22, 2020 at 01:43:46PM -0800, Randy Dunlap wrote:
> Hi Greg,
> 
> If you make any more changes:
> 
> On 1/22/20 11:31 AM, Greg Kroah-Hartman wrote:
> > diff --git a/Documentation/admin-guide/dynamic-debug-howto.rst b/Documentation/admin-guide/dynamic-debug-howto.rst
> > index 252e5ef324e5..41f43a373a6a 100644
> > --- a/Documentation/admin-guide/dynamic-debug-howto.rst
> > +++ b/Documentation/admin-guide/dynamic-debug-howto.rst
> > @@ -54,6 +54,9 @@ If you make a mistake with the syntax, the write will fail thus::
> >  				<debugfs>/dynamic_debug/control
> >    -bash: echo: write error: Invalid argument
> >  
> > +Note, for systems without 'debugfs' enabled, the control file can be
> > +also found in ``/proc/dynamic_debug/control``.
> 
> Mostly drop the "also".  How about:
> 
> > +Note, for systems without 'debugfs' enabled, the control file is located
> > +in ``/proc/dynamic_debug/control``.

Much nicer, I'll respin it with this change, thanks for the review!

greg k-h

[PATCH v4] dynamic_debug: allow to work if debugfs is disabled

[Greg Kroah-Hartman]

With the realization that having debugfs enabled on "production" systems is
generally not a good idea, debugfs is being disabled from more and more
platforms over time.  However, the functionality of dynamic debugging still is
needed at times, and since it relies on debugfs for its user api, having
debugfs disabled also forces dynamic debug to be disabled.

To get around this, move the "control" file for dynamic_debug to procfs IFF
debugfs is disabled.  This lets people turn on debugging as needed at runtime
for individual driverfs and subsystems.

Reported-by: many different companies
Cc: Jason Baron <jbaron@akamai.com>
Signed-off-by: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
---
v4: tweaks to the .rst text thanks to Randy's review
v3: rename init function as it is now no longer just for debugfs, thanks
    to Jason for the review.
    Fix build warning for debugfs_initialized call.
v2: Fix up octal permissions and add procfs reference to the Kconfig
    entry, thanks to Will for the review.

 .../admin-guide/dynamic-debug-howto.rst       |  3 +++
 lib/Kconfig.debug                             |  7 ++++---
 lib/dynamic_debug.c                           | 21 ++++++++++++++-----
 3 files changed, 23 insertions(+), 8 deletions(-)

diff --git a/Documentation/admin-guide/dynamic-debug-howto.rst b/Documentation/admin-guide/dynamic-debug-howto.rst
index 252e5ef324e5..60679dda6007 100644
--- a/Documentation/admin-guide/dynamic-debug-howto.rst
+++ b/Documentation/admin-guide/dynamic-debug-howto.rst
@@ -54,6 +54,9 @@ If you make a mistake with the syntax, the write will fail thus::
 				<debugfs>/dynamic_debug/control
   -bash: echo: write error: Invalid argument
 
+Note, for systems without 'debugfs' enabled, the control file is located
+in ``/proc/dynamic_debug/control``.
+
 Viewing Dynamic Debug Behaviour
 ===============================
 
diff --git a/lib/Kconfig.debug b/lib/Kconfig.debug
index 5ffe144c9794..49980eb8c18e 100644
--- a/lib/Kconfig.debug
+++ b/lib/Kconfig.debug
@@ -98,7 +98,7 @@ config DYNAMIC_DEBUG
 	bool "Enable dynamic printk() support"
 	default n
 	depends on PRINTK
-	depends on DEBUG_FS
+	depends on (DEBUG_FS || PROC_FS)
 	help
 
 	  Compiles debug level messages into the kernel, which would not
@@ -116,8 +116,9 @@ config DYNAMIC_DEBUG
 	  Usage:
 
 	  Dynamic debugging is controlled via the 'dynamic_debug/control' file,
-	  which is contained in the 'debugfs' filesystem. Thus, the debugfs
-	  filesystem must first be mounted before making use of this feature.
+	  which is contained in the 'debugfs' filesystem or procfs if
+	  debugfs is not present. Thus, the debugfs or procfs filesystem
+	  must first be mounted before making use of this feature.
 	  We refer the control file as: <debugfs>/dynamic_debug/control. This
 	  file contains a list of the debug statements that can be enabled. The
 	  format for each line of the file is:
diff --git a/lib/dynamic_debug.c b/lib/dynamic_debug.c
index c60409138e13..118e928cdbda 100644
--- a/lib/dynamic_debug.c
+++ b/lib/dynamic_debug.c
@@ -991,15 +991,26 @@ static void ddebug_remove_all_tables(void)
 
 static __initdata int ddebug_init_success;
 
-static int __init dynamic_debug_init_debugfs(void)
+static int __init dynamic_debug_init_control(void)
 {
-	struct dentry *dir;
+	struct proc_dir_entry *procfs_dir;
+	struct dentry *debugfs_dir;
 
 	if (!ddebug_init_success)
 		return -ENODEV;
 
-	dir = debugfs_create_dir("dynamic_debug", NULL);
-	debugfs_create_file("control", 0644, dir, NULL, &ddebug_proc_fops);
+	/* Create the control file in debugfs if it is enabled */
+	if (debugfs_initialized()) {
+		debugfs_dir = debugfs_create_dir("dynamic_debug", NULL);
+		debugfs_create_file("control", 0644, debugfs_dir, NULL,
+				    &ddebug_proc_fops);
+		return 0;
+	}
+
+	/* No debugfs so put it in procfs instead */
+	procfs_dir = proc_mkdir("dynamic_debug", NULL);
+	if (procfs_dir)
+		proc_create("control", 0644, procfs_dir, &ddebug_proc_fops);
 
 	return 0;
 }
@@ -1077,4 +1088,4 @@ static int __init dynamic_debug_init(void)
 early_initcall(dynamic_debug_init);
 
 /* Debugfs setup must be done later */
-fs_initcall(dynamic_debug_init_debugfs);
+fs_initcall(dynamic_debug_init_control);
-- 
2.25.0

Re: [PATCH v4] dynamic_debug: allow to work if debugfs is disabled

[Will Deacon]

On Thu, Jan 23, 2020 at 09:50:15AM +0100, Greg Kroah-Hartman wrote:
> With the realization that having debugfs enabled on "production" systems is
> generally not a good idea, debugfs is being disabled from more and more
> platforms over time.  However, the functionality of dynamic debugging still is
> needed at times, and since it relies on debugfs for its user api, having
> debugfs disabled also forces dynamic debug to be disabled.
> 
> To get around this, move the "control" file for dynamic_debug to procfs IFF
> debugfs is disabled.  This lets people turn on debugging as needed at runtime
> for individual driverfs and subsystems.
> 
> Reported-by: many different companies
> Cc: Jason Baron <jbaron@akamai.com>
> Signed-off-by: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
> ---
> v4: tweaks to the .rst text thanks to Randy's review
> v3: rename init function as it is now no longer just for debugfs, thanks
>     to Jason for the review.
>     Fix build warning for debugfs_initialized call.
> v2: Fix up octal permissions and add procfs reference to the Kconfig
>     entry, thanks to Will for the review.
> 
>  .../admin-guide/dynamic-debug-howto.rst       |  3 +++
>  lib/Kconfig.debug                             |  7 ++++---
>  lib/dynamic_debug.c                           | 21 ++++++++++++++-----
>  3 files changed, 23 insertions(+), 8 deletions(-)

I had a brief "oh crap" moment when I thought you were exposing both the
procfs and debugfs interfaces at the same time, but thankfully that's not
the case. Whilst it's a bit of a shame that it's come to this, the code
looks pretty decent to me, so:

Acked-by: Will Deacon <will@kernel.org>

Thanks,

Will

Re: [PATCH v3] dynamic_debug: allow to work if debugfs is disabled

[Theodore Y. Ts'o]

On Wed, Jan 22, 2020 at 08:31:18PM +0100, Greg Kroah-Hartman wrote:
> With the realization that having debugfs enabled on "production" systems is
> generally not a good idea, debugfs is being disabled from more and more
> platforms over time.  However, the functionality of dynamic debugging still is
> needed at times, and since it relies on debugfs for its user api, having
> debugfs disabled also forces dynamic debug to be disabled.
> 
> To get around this, move the "control" file for dynamic_debug to procfs IFF
> debugfs is disabled.  This lets people turn on debugging as needed at runtime
> for individual driverfs and subsystems.

Instead of moving the control file IFF debugfs is enabled, what about
always making it available in /proc, and marking the control file for
dynamic_debug in debugfs as deprecated?  It would seem to me that this
would cause less confusion in the future....

					- Ted

Re: [PATCH v3] dynamic_debug: allow to work if debugfs is disabled

[Greg Kroah-Hartman]

On Thu, Jan 23, 2020 at 10:53:40AM -0500, Theodore Y. Ts'o wrote:
> On Wed, Jan 22, 2020 at 08:31:18PM +0100, Greg Kroah-Hartman wrote:
> > With the realization that having debugfs enabled on "production" systems is
> > generally not a good idea, debugfs is being disabled from more and more
> > platforms over time.  However, the functionality of dynamic debugging still is
> > needed at times, and since it relies on debugfs for its user api, having
> > debugfs disabled also forces dynamic debug to be disabled.
> > 
> > To get around this, move the "control" file for dynamic_debug to procfs IFF
> > debugfs is disabled.  This lets people turn on debugging as needed at runtime
> > for individual driverfs and subsystems.
> 
> Instead of moving the control file IFF debugfs is enabled, what about
> always making it available in /proc, and marking the control file for
> dynamic_debug in debugfs as deprecated?  It would seem to me that this
> would cause less confusion in the future....

Why deprecate it?  It's fine where it is, and most developer's have
debugfs enabled so all is good.  I'd rather only use /proc as a
last-resort.

thanks,

greg k-h

Re: [PATCH v3] dynamic_debug: allow to work if debugfs is disabled

[Theodore Y. Ts'o]

On Thu, Jan 23, 2020 at 06:55:36PM +0100, Greg Kroah-Hartman wrote:
> > Instead of moving the control file IFF debugfs is enabled, what about
> > always making it available in /proc, and marking the control file for
> > dynamic_debug in debugfs as deprecated?  It would seem to me that this
> > would cause less confusion in the future....
> 
> Why deprecate it?  It's fine where it is, and most developer's have
> debugfs enabled so all is good.  I'd rather only use /proc as a
> last-resort.

This makes life difficult for scripts that manipulate the control
file, since they now need to check two different locations -- either
/sys/kernel/debug or /proc.  It's likely that people who normally use
distribution kernels where debugfs is disabled will have scripts which
are hard-coded to look in /proc, and then when they build a kernel
with debugfs enabled, the /proc entry will go **poof**, and their
script will break.

So regardless of what we do with the control file in debugfs, it might
be nice if moving forward, scripts can count on the /proc file
existing.

Cheers,

					- Ted

Re: [PATCH v3] dynamic_debug: allow to work if debugfs is disabled

[Greg Kroah-Hartman]

On Fri, Jan 24, 2020 at 01:02:00AM -0500, Theodore Y. Ts'o wrote:
> On Thu, Jan 23, 2020 at 06:55:36PM +0100, Greg Kroah-Hartman wrote:
> > > Instead of moving the control file IFF debugfs is enabled, what about
> > > always making it available in /proc, and marking the control file for
> > > dynamic_debug in debugfs as deprecated?  It would seem to me that this
> > > would cause less confusion in the future....
> > 
> > Why deprecate it?  It's fine where it is, and most developer's have
> > debugfs enabled so all is good.  I'd rather only use /proc as a
> > last-resort.
> 
> This makes life difficult for scripts that manipulate the control
> file, since they now need to check two different locations -- either
> /sys/kernel/debug or /proc.

That is literally 2 extra lines in a script file.  If you point me at
any that actually used the existing control file, I will be glad to fix
them up :)

> It's likely that people who normally use
> distribution kernels where debugfs is disabled will have scripts which
> are hard-coded to look in /proc, and then when they build a kernel
> with debugfs enabled, the /proc entry will go **poof**, and their
> script will break.

**poof** they didn't test it :)

Seriously, I am doing this change to make it _easier_ for those people
who want debugfs disabled, yet want this type of debugging.  This is
much better than having no debugging at all.

Don't put extra complexity in the kernel for when it can be trivially
handled in userspace, you know this :)

thanks,

greg k-h

Re: [PATCH] dynamic_debug: allow to work if debugfs is disabled

[kbuild test robot]

[-- Attachment #1: Type: text/plain, Size: 2342 bytes --]

Hi Greg,

I love your patch! Perhaps something to improve:

[auto build test WARNING on linux/master]
[also build test WARNING on linus/master v5.5-rc7]
[if your patch is applied to the wrong git tree, please drop us a note to help
improve the system. BTW, we also suggest to use '--base' option to specify the
base tree in git format-patch, please see https://stackoverflow.com/a/37406982]

url:    https://github.com/0day-ci/linux/commits/Greg-Kroah-Hartman/dynamic_debug-allow-to-work-if-debugfs-is-disabled/20200124-140304
base:   https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git 1522d9da40bdfe502c91163e6d769332897201fa
config: m68k-allmodconfig (attached as .config)
compiler: m68k-linux-gcc (GCC) 7.5.0
reproduce:
        wget https://raw.githubusercontent.com/intel/lkp-tests/master/sbin/make.cross -O ~/bin/make.cross
        chmod +x ~/bin/make.cross
        # save the attached .config to linux build tree
        GCC_VERSION=7.5.0 make.cross ARCH=m68k 

If you fix the issue, kindly add following tag
Reported-by: kbuild test robot <lkp@intel.com>

All warnings (new ones prefixed by >>):

   lib/dynamic_debug.c: In function 'dynamic_debug_init_debugfs':
>> lib/dynamic_debug.c:1003:6: warning: the address of 'debugfs_initialized' will always evaluate as 'true' [-Waddress]
     if (debugfs_initialized) {
         ^~~~~~~~~~~~~~~~~~~

vim +1003 lib/dynamic_debug.c

   993	
   994	static int __init dynamic_debug_init_debugfs(void)
   995	{
   996		struct dentry *debugfs_dir;
   997		struct proc_dir_entry *procfs_dir;
   998	
   999		if (!ddebug_init_success)
  1000			return -ENODEV;
  1001	
  1002		/* Create the control file in debugfs if it is enabled */
> 1003		if (debugfs_initialized) {
  1004			debugfs_dir = debugfs_create_dir("dynamic_debug", NULL);
  1005			debugfs_create_file("control", 0644, debugfs_dir, NULL,
  1006					    &ddebug_proc_fops);
  1007			return 0;
  1008		}
  1009	
  1010		/* No debugfs so put it in procfs instead */
  1011		procfs_dir = proc_mkdir("dynamic_debug", NULL);
  1012		if (procfs_dir)
  1013			proc_create("control", 0x644, procfs_dir, &ddebug_proc_fops);
  1014	
  1015		return 0;
  1016	}
  1017	

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

[-- Attachment #2: .config.gz --]
[-- Type: application/gzip, Size: 51852 bytes --]

Re: [PATCH v3] dynamic_debug: allow to work if debugfs is disabled

[Theodore Y. Ts'o]

On Fri, Jan 24, 2020 at 08:29:40AM +0100, Greg Kroah-Hartman wrote:
> > It's likely that people who normally use
> > distribution kernels where debugfs is disabled will have scripts which
> > are hard-coded to look in /proc, and then when they build a kernel
> > with debugfs enabled, the /proc entry will go **poof**, and their
> > script will break.
> 
> **poof** they didn't test it :)
> 
> Seriously, I am doing this change to make it _easier_ for those people
> who want debugfs disabled, yet want this type of debugging.  This is
> much better than having no debugging at all.
> 
> Don't put extra complexity in the kernel for when it can be trivially
> handled in userspace, you know this :)

It's also trivial to handle this in the kernel by potentially having
the control file appear in two places.  Consider what it would be like
to explain this in the Linux documentation --- "the control file could
be here, or it could be there", depending on how the kernel is
configured.  The complexity of documenting this, and the complexity in
userspace; and we have to have the code in the source code for the
file to be in the appear in both places *anyway*.

We've done a lot more to maintain userspace backwards compatibility
that Linus demands; and while I recognize this is not exactly the same
thing, being consistent about where to find the control file would be
even *easier* for userspace programmers.  And is it really that hard
to provide this consistency in the kernel?

Cheers,

					- Ted

Re: [PATCH v3] dynamic_debug: allow to work if debugfs is disabled

[Jonathan Corbet]

On Fri, 24 Jan 2020 20:42:31 -0500
"Theodore Y. Ts'o" <tytso@mit.edu> wrote:

> On Fri, Jan 24, 2020 at 08:29:40AM +0100, Greg Kroah-Hartman wrote:
> > > It's likely that people who normally use
> > > distribution kernels where debugfs is disabled will have scripts which
> > > are hard-coded to look in /proc, and then when they build a kernel
> > > with debugfs enabled, the /proc entry will go **poof**, and their
> > > script will break.  
> > 
> > **poof** they didn't test it :)
> > 
> > Seriously, I am doing this change to make it _easier_ for those people
> > who want debugfs disabled, yet want this type of debugging.  This is
> > much better than having no debugging at all.
> > 
> > Don't put extra complexity in the kernel for when it can be trivially
> > handled in userspace, you know this :)  
> 
> It's also trivial to handle this in the kernel by potentially having
> the control file appear in two places.  Consider what it would be like
> to explain this in the Linux documentation --- "the control file could
> be here, or it could be there", depending on how the kernel is
> configured.  The complexity of documenting this, and the complexity in
> userspace; and we have to have the code in the source code for the
> file to be in the appear in both places *anyway*.

FWIW, avoiding the need to document something like this has been a
motivating factor behind a number of my patches over the years.

Moving a control knob based on kernel configuration is a user-hostile
feature.  Scripts can be made to cope with this kind of behavior in one
place; if you let such things accumulate in a system, though, it gets to
the point where it's really hard for anybody to keep track of all the
pieces and be sure that their code will work.

If dynamic debug is meant to be a feature supported on all kernels, it
should, IMO, be lifted out of debugfs and made into a proper feature - with
a compatibility link left behind in debugfs for as long as it's needed, of
course.

</sermon> :)

jon

Re: [PATCH v3] dynamic_debug: allow to work if debugfs is disabled

[Saravana Kannan]

On Sat, Jan 25, 2020 at 9:11 AM Jonathan Corbet <corbet@lwn.net> wrote:
>
> On Fri, 24 Jan 2020 20:42:31 -0500
> "Theodore Y. Ts'o" <tytso@mit.edu> wrote:
>
> > On Fri, Jan 24, 2020 at 08:29:40AM +0100, Greg Kroah-Hartman wrote:
> > > > It's likely that people who normally use
> > > > distribution kernels where debugfs is disabled will have scripts which
> > > > are hard-coded to look in /proc, and then when they build a kernel
> > > > with debugfs enabled, the /proc entry will go **poof**, and their
> > > > script will break.
> > >
> > > **poof** they didn't test it :)
> > >
> > > Seriously, I am doing this change to make it _easier_ for those people
> > > who want debugfs disabled, yet want this type of debugging.  This is
> > > much better than having no debugging at all.
> > >
> > > Don't put extra complexity in the kernel for when it can be trivially
> > > handled in userspace, you know this :)
> >
> > It's also trivial to handle this in the kernel by potentially having
> > the control file appear in two places.  Consider what it would be like
> > to explain this in the Linux documentation --- "the control file could
> > be here, or it could be there", depending on how the kernel is
> > configured.  The complexity of documenting this, and the complexity in
> > userspace; and we have to have the code in the source code for the
> > file to be in the appear in both places *anyway*.
>
> FWIW, avoiding the need to document something like this has been a
> motivating factor behind a number of my patches over the years.
>
> Moving a control knob based on kernel configuration is a user-hostile
> feature.  Scripts can be made to cope with this kind of behavior in one
> place; if you let such things accumulate in a system, though, it gets to
> the point where it's really hard for anybody to keep track of all the
> pieces and be sure that their code will work.
>
> If dynamic debug is meant to be a feature supported on all kernels, it
> should, IMO, be lifted out of debugfs and made into a proper feature - with
> a compatibility link left behind in debugfs for as long as it's needed, of
> course.
>
> </sermon> :)

My 2 cents -- agree with what Ted and Jon have said.

-Saravana

[PATCH v5] dynamic_debug: allow to work if debugfs is disabled

[Greg Kroah-Hartman]

With the realization that having debugfs enabled on "production" systems
is generally not a good idea, debugfs is being disabled from more and
more platforms over time.  However, the functionality of dynamic
debugging still is needed at times, and since it relies on debugfs for
its user api, having debugfs disabled also forces dynamic debug to be
disabled.

To get around this, also create the "control" file for dynamic_debug in
procfs.  This allows people turn on debugging as needed at runtime for
individual driverfs and subsystems.

Reported-by: many different companies
Cc: Jason Baron <jbaron@akamai.com>
Signed-off-by: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
---
v5: as many people asked for it, now enable the control file in both
    debugfs and procfs at the same time.
v4: tweaks to the .rst text thanks to Randy's review
v3: rename init function as it is now no longer just for debugfs, thanks
    to Jason for the review.
    Fix build warning for debugfs_initialized call.
v2: Fix up octal permissions and add procfs reference to the Kconfig
    entry, thanks to Will for the review.

 .../admin-guide/dynamic-debug-howto.rst       |  3 +++
 lib/Kconfig.debug                             |  7 ++++---
 lib/dynamic_debug.c                           | 20 ++++++++++++++-----
 3 files changed, 22 insertions(+), 8 deletions(-)

diff --git a/Documentation/admin-guide/dynamic-debug-howto.rst b/Documentation/admin-guide/dynamic-debug-howto.rst
index 252e5ef324e5..585451d12608 100644
--- a/Documentation/admin-guide/dynamic-debug-howto.rst
+++ b/Documentation/admin-guide/dynamic-debug-howto.rst
@@ -54,6 +54,9 @@ If you make a mistake with the syntax, the write will fail thus::
 				<debugfs>/dynamic_debug/control
   -bash: echo: write error: Invalid argument
 
+Note, for systems without 'debugfs' enabled, the control file can also
+be found in ``/proc/dynamic_debug/control``.
+
 Viewing Dynamic Debug Behaviour
 ===============================
 
diff --git a/lib/Kconfig.debug b/lib/Kconfig.debug
index 69def4a9df00..a15dde66dc4c 100644
--- a/lib/Kconfig.debug
+++ b/lib/Kconfig.debug
@@ -98,7 +98,7 @@ config DYNAMIC_DEBUG
 	bool "Enable dynamic printk() support"
 	default n
 	depends on PRINTK
-	depends on DEBUG_FS
+	depends on (DEBUG_FS || PROC_FS)
 	help
 
 	  Compiles debug level messages into the kernel, which would not
@@ -116,8 +116,9 @@ config DYNAMIC_DEBUG
 	  Usage:
 
 	  Dynamic debugging is controlled via the 'dynamic_debug/control' file,
-	  which is contained in the 'debugfs' filesystem. Thus, the debugfs
-	  filesystem must first be mounted before making use of this feature.
+	  which is contained in the 'debugfs' filesystem or procfs if
+	  debugfs is not present. Thus, the debugfs or procfs filesystem
+	  must first be mounted before making use of this feature.
 	  We refer the control file as: <debugfs>/dynamic_debug/control. This
 	  file contains a list of the debug statements that can be enabled. The
 	  format for each line of the file is:
diff --git a/lib/dynamic_debug.c b/lib/dynamic_debug.c
index c60409138e13..c220c1891729 100644
--- a/lib/dynamic_debug.c
+++ b/lib/dynamic_debug.c
@@ -991,15 +991,25 @@ static void ddebug_remove_all_tables(void)
 
 static __initdata int ddebug_init_success;
 
-static int __init dynamic_debug_init_debugfs(void)
+static int __init dynamic_debug_init_control(void)
 {
-	struct dentry *dir;
+	struct proc_dir_entry *procfs_dir;
+	struct dentry *debugfs_dir;
 
 	if (!ddebug_init_success)
 		return -ENODEV;
 
-	dir = debugfs_create_dir("dynamic_debug", NULL);
-	debugfs_create_file("control", 0644, dir, NULL, &ddebug_proc_fops);
+	/* Create the control file in debugfs if it is enabled */
+	if (debugfs_initialized()) {
+		debugfs_dir = debugfs_create_dir("dynamic_debug", NULL);
+		debugfs_create_file("control", 0644, debugfs_dir, NULL,
+				    &ddebug_proc_fops);
+	}
+
+	/* Also create the control file in procfs */
+	procfs_dir = proc_mkdir("dynamic_debug", NULL);
+	if (procfs_dir)
+		proc_create("control", 0644, procfs_dir, &ddebug_proc_fops);
 
 	return 0;
 }
@@ -1077,4 +1087,4 @@ static int __init dynamic_debug_init(void)
 early_initcall(dynamic_debug_init);
 
 /* Debugfs setup must be done later */
-fs_initcall(dynamic_debug_init_debugfs);
+fs_initcall(dynamic_debug_init_control);
-- 
2.25.0

Re: [PATCH v5] dynamic_debug: allow to work if debugfs is disabled

[Joe Perches]

On Sun, 2020-02-09 at 12:05 +0100, Greg Kroah-Hartman wrote:
> With the realization that having debugfs enabled on "production" systems
> is generally not a good idea, debugfs is being disabled from more and
> more platforms over time.  However, the functionality of dynamic
> debugging still is needed at times, and since it relies on debugfs for
> its user api, having debugfs disabled also forces dynamic debug to be
> disabled.
> 
> To get around this, also create the "control" file for dynamic_debug in
> procfs.  This allows people turn on debugging as needed at runtime for
> individual driverfs and subsystems.
> 
> Reported-by: many different companies
> Cc: Jason Baron <jbaron@akamai.com>
> Signed-off-by: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
> ---
> v5: as many people asked for it, now enable the control file in both
>     debugfs and procfs at the same time.

So now there can be differences in the two control files
and these are readable files are sometimes parsed by
scripts.

It'd be better to figure out how to soft link the files.

Re: [PATCH v5] dynamic_debug: allow to work if debugfs is disabled

[Greg Kroah-Hartman]

On Sun, Feb 09, 2020 at 07:53:38AM -0800, Joe Perches wrote:
> On Sun, 2020-02-09 at 12:05 +0100, Greg Kroah-Hartman wrote:
> > With the realization that having debugfs enabled on "production" systems
> > is generally not a good idea, debugfs is being disabled from more and
> > more platforms over time.  However, the functionality of dynamic
> > debugging still is needed at times, and since it relies on debugfs for
> > its user api, having debugfs disabled also forces dynamic debug to be
> > disabled.
> > 
> > To get around this, also create the "control" file for dynamic_debug in
> > procfs.  This allows people turn on debugging as needed at runtime for
> > individual driverfs and subsystems.
> > 
> > Reported-by: many different companies
> > Cc: Jason Baron <jbaron@akamai.com>
> > Signed-off-by: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
> > ---
> > v5: as many people asked for it, now enable the control file in both
> >     debugfs and procfs at the same time.
> 
> So now there can be differences in the two control files
> and these are readable files are sometimes parsed by
> scripts.

What difference will there be?  They should both be the same, as they
point to the identical fops behind the virtual file.

> It'd be better to figure out how to soft link the files.

A symlink is not going to work, but this should be fine from what I can
tell.  I'll do some more debugging tonight, but all was fine last I
tried this last week.

thanks,

greg k-h

Re: [PATCH v6] dynamic_debug: allow to work if debugfs is disabled

[Greg Kroah-Hartman]

With the realization that having debugfs enabled on "production" systems
is generally not a good idea, debugfs is being disabled from more and
more platforms over time.  However, the functionality of dynamic
debugging still is needed at times, and since it relies on debugfs for
its user api, having debugfs disabled also forces dynamic debug to be
disabled.

To get around this, also create the "control" file for dynamic_debug in
procfs.  This allows people turn on debugging as needed at runtime for
individual driverfs and subsystems.

Reported-by: many different companies
Cc: Jason Baron <jbaron@akamai.com>
Signed-off-by: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
---
v6: fix up Kconfig help, it was a bit incorrect,thanks to Saravana for
    the review.
v5: as many people asked for it, now enable the control file in both
    debugfs and procfs at the same time.
v4: tweaks to the .rst text thanks to Randy's review
v3: rename init function as it is now no longer just for debugfs, thanks
    to Jason for the review.
    Fix build warning for debugfs_initialized call.
v2: Fix up octal permissions and add procfs reference to the Kconfig
    entry, thanks to Will for the review.
 .../admin-guide/dynamic-debug-howto.rst       |  3 +++
 lib/Kconfig.debug                             |  7 ++++---
 lib/dynamic_debug.c                           | 20 ++++++++++++++-----
 3 files changed, 22 insertions(+), 8 deletions(-)

diff --git a/Documentation/admin-guide/dynamic-debug-howto.rst b/Documentation/admin-guide/dynamic-debug-howto.rst
index 252e5ef324e5..585451d12608 100644
--- a/Documentation/admin-guide/dynamic-debug-howto.rst
+++ b/Documentation/admin-guide/dynamic-debug-howto.rst
@@ -54,6 +54,9 @@ If you make a mistake with the syntax, the write will fail thus::
 				<debugfs>/dynamic_debug/control
   -bash: echo: write error: Invalid argument
 
+Note, for systems without 'debugfs' enabled, the control file can also
+be found in ``/proc/dynamic_debug/control``.
+
 Viewing Dynamic Debug Behaviour
 ===============================
 
diff --git a/lib/Kconfig.debug b/lib/Kconfig.debug
index 69def4a9df00..7f4992fd8a2e 100644
--- a/lib/Kconfig.debug
+++ b/lib/Kconfig.debug
@@ -98,7 +98,7 @@ config DYNAMIC_DEBUG
 	bool "Enable dynamic printk() support"
 	default n
 	depends on PRINTK
-	depends on DEBUG_FS
+	depends on (DEBUG_FS || PROC_FS)
 	help
 
 	  Compiles debug level messages into the kernel, which would not
@@ -116,8 +116,9 @@ config DYNAMIC_DEBUG
 	  Usage:
 
 	  Dynamic debugging is controlled via the 'dynamic_debug/control' file,
-	  which is contained in the 'debugfs' filesystem. Thus, the debugfs
-	  filesystem must first be mounted before making use of this feature.
+	  which is contained in the 'debugfs' filesystem or procfs.
+	  Thus, the debugfs or procfs filesystem must first be mounted before
+	  making use of this feature.
 	  We refer the control file as: <debugfs>/dynamic_debug/control. This
 	  file contains a list of the debug statements that can be enabled. The
 	  format for each line of the file is:
diff --git a/lib/dynamic_debug.c b/lib/dynamic_debug.c
index c60409138e13..c220c1891729 100644
--- a/lib/dynamic_debug.c
+++ b/lib/dynamic_debug.c
@@ -991,15 +991,25 @@ static void ddebug_remove_all_tables(void)
 
 static __initdata int ddebug_init_success;
 
-static int __init dynamic_debug_init_debugfs(void)
+static int __init dynamic_debug_init_control(void)
 {
-	struct dentry *dir;
+	struct proc_dir_entry *procfs_dir;
+	struct dentry *debugfs_dir;
 
 	if (!ddebug_init_success)
 		return -ENODEV;
 
-	dir = debugfs_create_dir("dynamic_debug", NULL);
-	debugfs_create_file("control", 0644, dir, NULL, &ddebug_proc_fops);
+	/* Create the control file in debugfs if it is enabled */
+	if (debugfs_initialized()) {
+		debugfs_dir = debugfs_create_dir("dynamic_debug", NULL);
+		debugfs_create_file("control", 0644, debugfs_dir, NULL,
+				    &ddebug_proc_fops);
+	}
+
+	/* Also create the control file in procfs */
+	procfs_dir = proc_mkdir("dynamic_debug", NULL);
+	if (procfs_dir)
+		proc_create("control", 0644, procfs_dir, &ddebug_proc_fops);
 
 	return 0;
 }
@@ -1077,4 +1087,4 @@ static int __init dynamic_debug_init(void)
 early_initcall(dynamic_debug_init);
 
 /* Debugfs setup must be done later */
-fs_initcall(dynamic_debug_init_debugfs);
+fs_initcall(dynamic_debug_init_control);

base-commit: bb6d3fb354c5ee8d6bde2d576eb7220ea09862b9
-- 
2.25.0

Re: [PATCH v6] dynamic_debug: allow to work if debugfs is disabled

[Randy Dunlap]

On 2/10/20 1:11 PM, Greg Kroah-Hartman wrote:
> diff --git a/Documentation/admin-guide/dynamic-debug-howto.rst b/Documentation/admin-guide/dynamic-debug-howto.rst
> index 252e5ef324e5..585451d12608 100644
> --- a/Documentation/admin-guide/dynamic-debug-howto.rst
> +++ b/Documentation/admin-guide/dynamic-debug-howto.rst
> @@ -54,6 +54,9 @@ If you make a mistake with the syntax, the write will fail thus::
>  				<debugfs>/dynamic_debug/control
>    -bash: echo: write error: Invalid argument
>  
> +Note, for systems without 'debugfs' enabled, the control file can also

Hi Greg,
If you make any more changes, please drop the word "also" here       ^^^^

> +be found in ``/proc/dynamic_debug/control``.
> +
>  Viewing Dynamic Debug Behaviour
>  ===============================
>  


-- 
~Randy

Re: [PATCH v6] dynamic_debug: allow to work if debugfs is disabled

[Will Deacon]

On Mon, Feb 10, 2020 at 01:11:42PM -0800, Greg Kroah-Hartman wrote:
> With the realization that having debugfs enabled on "production" systems
> is generally not a good idea, debugfs is being disabled from more and
> more platforms over time.  However, the functionality of dynamic
> debugging still is needed at times, and since it relies on debugfs for
> its user api, having debugfs disabled also forces dynamic debug to be
> disabled.
> 
> To get around this, also create the "control" file for dynamic_debug in
> procfs.  This allows people turn on debugging as needed at runtime for
> individual driverfs and subsystems.
> 
> Reported-by: many different companies
> Cc: Jason Baron <jbaron@akamai.com>
> Signed-off-by: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
> ---
> v6: fix up Kconfig help, it was a bit incorrect,thanks to Saravana for
>     the review.
> v5: as many people asked for it, now enable the control file in both
>     debugfs and procfs at the same time.

The 'ddebug_lock' mutex looks like it resolves all of the races here, so:

Acked-by: Will Deacon <will@kernel.org>

Will

Re: [PATCH v6] dynamic_debug: allow to work if debugfs is disabled

[Greg Kroah-Hartman]

On Mon, Feb 10, 2020 at 01:15:50PM -0800, Randy Dunlap wrote:
> On 2/10/20 1:11 PM, Greg Kroah-Hartman wrote:
> > diff --git a/Documentation/admin-guide/dynamic-debug-howto.rst b/Documentation/admin-guide/dynamic-debug-howto.rst
> > index 252e5ef324e5..585451d12608 100644
> > --- a/Documentation/admin-guide/dynamic-debug-howto.rst
> > +++ b/Documentation/admin-guide/dynamic-debug-howto.rst
> > @@ -54,6 +54,9 @@ If you make a mistake with the syntax, the write will fail thus::
> >  				<debugfs>/dynamic_debug/control
> >    -bash: echo: write error: Invalid argument
> >  
> > +Note, for systems without 'debugfs' enabled, the control file can also
> 
> Hi Greg,
> If you make any more changes, please drop the word "also" here       ^^^^

I will delete it by hand when I apply the patch now, thanks!

greg k-h