On Mon, Nov 29, 2021 at 02:45:16AM +0300, Sergey Ryazanov wrote:

Add Leon to CC to merge both conversations.

On Sun, Nov 28, 2021 at 8:01 PM Johannes Berg [off-list ref] wrote:

quoted

On Sun, 2021-11-28 at 15:55 +0300, Sergey Ryazanov wrote:

quoted

+config WWAN_DEBUGFS
+     bool "WWAN subsystem common debugfs interface"
+     depends on DEBUG_FS
+     help
+       Enables common debugfs infrastructure for WWAN devices.
+
+       If unsure, say N.

I wonder if that really should even say "If unsure, say N." because
really, once you have DEBUG_FS enabled, you can expect things to show up
there?

And I'd probably even argue that it should be

        bool "..." if EXPERT
        default y
        depends on DEBUG_FS

so most people aren't even bothered by the question?

quoted

 config WWAN_HWSIM
      tristate "Simulated WWAN device"
      help

@@ -83,6 +91,7 @@ config IOSM
 config IOSM_DEBUGFS
      bool "IOSM Debugfs support"
      depends on IOSM && DEBUG_FS
+     select WWAN_DEBUGFS

I guess it's kind of a philosophical question, but perhaps it would make
more sense for that to be "depends on" (and then you can remove &&
DEBUG_FS"), since that way it becomes trivial to disable all of WWAN
debugfs and not have to worry about individual driver settings?


And after that change, I'd probably just make this one "def_bool y"
instead of asking the user.

When I was preparing this series, my primary considered use case was
embedded firmwares. For example, in OpenWrt, you can not completely
disable debugfs, as a lot of wireless stuff can only be configured and
monitored with the debugfs knobs. At the same time, reducing the size
of a kernel and modules is an essential task in the world of embedded
software. Disabling the WWAN and IOSM debugfs interfaces allows us to
save 50K (x86-64 build) of space for module storage. Not much, but
already considerable when you only have 16MB of storage.

I personally like Johannes' suggestion to enable these symbols by
default to avoid bothering PC users with such negligible things for
them. One thing that makes me doubtful is whether we should hide the
debugfs disabling option under the EXPERT. Or it would be an EXPERT
option misuse, since the debugfs knobs existence themself does not
affect regular WWAN device use.

Leon, would it be Ok with you to add these options to the kernel
configuration and enable them by default?

I didn't block your previous proposal either. Just pointed that your
description doesn't correlate with the actual rationale for the patches.

Instead of security claims, just use your OpenWrt case as a base for
the commit message, which is very reasonable and valuable case.

However you should ask yourself if both IOSM_DEBUGFS and WWAN_DEBUGFS
are needed. You wrote that wwan debugfs is empty without ioasm. Isn't
better to allow user to select WWAN_DEBUGFS and change iosm code to
rely on it instead of IOSM_DEBUGFS?

Thanks

-- 
Sergey