Message-ID: <YaX3V12BTl6pXsYr@unreal>
Date:   Tue, 30 Nov 2021 12:05:11 +0200
From:   Leon Romanovsky <leon@...nel.org>
To:     Sergey Ryazanov <ryazanov.s.a@...il.com>
Cc:     Johannes Berg <johannes@...solutions.net>,
        "David S . Miller" <davem@...emloft.net>,
        Jakub Kicinski <kuba@...nel.org>, netdev@...r.kernel.org,
        M Chetan Kumar <m.chetan.kumar@...el.com>,
        Intel Corporation <linuxwwan@...el.com>,
        Loic Poulain <loic.poulain@...aro.org>
Subject: Re: [PATCH RESEND net-next 5/5] net: wwan: core: make debugfs
 optional

On Tue, Nov 30, 2021 at 02:44:35AM +0300, Sergey Ryazanov wrote:
> On Mon, Nov 29, 2021 at 9:40 AM Leon Romanovsky <leon@...nel.org> wrote:
> > 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 <johannes@...solutions.net> wrote:
> >>> On Sun, 2021-11-28 at 15:55 +0300, Sergey Ryazanov wrote:
> >>>>
> >>>> +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?
> >>>
> >>>
> >>>>  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.
> 
> Sure. Previous messages were too shallow and unclear. Thanks for
> pointing me to this issue. I will improve them based on the feedback
> received.
> 
> I still think we need separate options for the subsystem and for the
> driver (see the rationale below). And I doubt, should I place the
> detailed description of the OpenWrt case in each commit message, or it
> would be sufficient to place it in a cover letter and add a shorter
> version to each commit message. On the one hand, the cover letter
> would not show up in the git log. On the other hand, it is not
> genteelly to blow up each commit message with the duplicated story.

I didn't check who is going to apply your patches, but many maintainers
use cover letter as a description for merge commit. I would write about
OpenWrt in the cover letter only.

> 
> > 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?
> 
> Yep, WWAN debugfs interface is useless without driver-specific knobs.
> At the moment, only the IOSM driver implements the specific debugfs
> interface. But a WWAN modem is a complex device with a lot of
> features. For example, see a set of debug and test interfaces
> implemented in the proposed driver for the Mediatek T7xx chipset [1].
> Without general support from the kernel, all of these debug and test
> features will most probably be implemented using the debugfs
> interface.
> 
> Initially, I also had a plan to implement a single subsystem-wide
> option to disable debugfs entirely. But then I considered how many
> driver developers would want to create a driver-specific debugfs
> interface, and changed my mind in favor of individual options. Just to
> avoid an all-or-nothing case.

Usually, the answer here is "don't over-engineer". Once such
functionality will be needed, it will be implemented pretty easily.

> 
> 1. https://lore.kernel.org/all/20211101035635.26999-14-ricardo.martinez@linux.intel.com
> 
> -- 
> Sergey

Powered by blists - more mailing lists