| Message ID | 20211128125522.23357-6-ryazanov.s.a@gmail.com (mailing list archive) |
|---|---|
| State | Superseded |
| Delegated to: | Netdev Maintainers |
| Headers | show |
| Series | WWAN debugfs tweaks | expand |
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. johannes
Hi Sergey,
I love your patch! Perhaps something to improve:
[auto build test WARNING on net-next/master]
url: https://github.com/0day-ci/linux/commits/Sergey-Ryazanov/WWAN-debugfs-tweaks/20211128-210031
base: https://git.kernel.org/pub/scm/linux/kernel/git/davem/net-next.git d40ce48cb3a68b54be123a1f99157c5ac613e260
config: i386-randconfig-a001-20211128 (https://download.01.org/0day-ci/archive/20211129/202111290026.HEuigppG-lkp@intel.com/config)
compiler: clang version 14.0.0 (https://github.com/llvm/llvm-project 5c64d8ef8cc0c0ed3e0f2ae693d99e7f70f20a84)
reproduce (this is a W=1 build):
wget https://raw.githubusercontent.com/intel/lkp-tests/master/sbin/make.cross -O ~/bin/make.cross
chmod +x ~/bin/make.cross
# https://github.com/0day-ci/linux/commit/89654e5e973a53b8375f37395c03359c59b63a99
git remote add linux-review https://github.com/0day-ci/linux
git fetch --no-tags linux-review Sergey-Ryazanov/WWAN-debugfs-tweaks/20211128-210031
git checkout 89654e5e973a53b8375f37395c03359c59b63a99
# save the config file to linux build tree
mkdir build_dir
COMPILER_INSTALL_PATH=$HOME/0day COMPILER=clang make.cross W=1 O=build_dir ARCH=i386 SHELL=/bin/bash drivers/net/wwan/
If you fix the issue, kindly add following tag as appropriate
Reported-by: kernel test robot <lkp@intel.com>
All warnings (new ones prefixed by >>):
>> drivers/net/wwan/wwan_core.c:171:14: warning: variable 'wwandev_name' set but not used [-Wunused-but-set-variable]
const char *wwandev_name;
^
1 warning generated.
vim +/wwandev_name +171 drivers/net/wwan/wwan_core.c
c4804670026b93f M Chetan Kumar 2021-11-20 162
9a44c1cc6388762 Loic Poulain 2021-04-16 163 /* This function allocates and registers a new WWAN device OR if a WWAN device
9a44c1cc6388762 Loic Poulain 2021-04-16 164 * already exist for the given parent, it gets a reference and return it.
9a44c1cc6388762 Loic Poulain 2021-04-16 165 * This function is not exported (for now), it is called indirectly via
9a44c1cc6388762 Loic Poulain 2021-04-16 166 * wwan_create_port().
9a44c1cc6388762 Loic Poulain 2021-04-16 167 */
9a44c1cc6388762 Loic Poulain 2021-04-16 168 static struct wwan_device *wwan_create_dev(struct device *parent)
9a44c1cc6388762 Loic Poulain 2021-04-16 169 {
9a44c1cc6388762 Loic Poulain 2021-04-16 170 struct wwan_device *wwandev;
c4804670026b93f M Chetan Kumar 2021-11-20 @171 const char *wwandev_name;
9a44c1cc6388762 Loic Poulain 2021-04-16 172 int err, id;
9a44c1cc6388762 Loic Poulain 2021-04-16 173
9a44c1cc6388762 Loic Poulain 2021-04-16 174 /* The 'find-alloc-register' operation must be protected against
9a44c1cc6388762 Loic Poulain 2021-04-16 175 * concurrent execution, a WWAN device is possibly shared between
9a44c1cc6388762 Loic Poulain 2021-04-16 176 * multiple callers or concurrently unregistered from wwan_remove_dev().
9a44c1cc6388762 Loic Poulain 2021-04-16 177 */
9a44c1cc6388762 Loic Poulain 2021-04-16 178 mutex_lock(&wwan_register_lock);
9a44c1cc6388762 Loic Poulain 2021-04-16 179
9a44c1cc6388762 Loic Poulain 2021-04-16 180 /* If wwandev already exists, return it */
9a44c1cc6388762 Loic Poulain 2021-04-16 181 wwandev = wwan_dev_get_by_parent(parent);
9a44c1cc6388762 Loic Poulain 2021-04-16 182 if (!IS_ERR(wwandev))
9a44c1cc6388762 Loic Poulain 2021-04-16 183 goto done_unlock;
9a44c1cc6388762 Loic Poulain 2021-04-16 184
9a44c1cc6388762 Loic Poulain 2021-04-16 185 id = ida_alloc(&wwan_dev_ids, GFP_KERNEL);
d9d5b8961284b00 Andy Shevchenko 2021-08-11 186 if (id < 0) {
d9d5b8961284b00 Andy Shevchenko 2021-08-11 187 wwandev = ERR_PTR(id);
9a44c1cc6388762 Loic Poulain 2021-04-16 188 goto done_unlock;
d9d5b8961284b00 Andy Shevchenko 2021-08-11 189 }
9a44c1cc6388762 Loic Poulain 2021-04-16 190
9a44c1cc6388762 Loic Poulain 2021-04-16 191 wwandev = kzalloc(sizeof(*wwandev), GFP_KERNEL);
9a44c1cc6388762 Loic Poulain 2021-04-16 192 if (!wwandev) {
d9d5b8961284b00 Andy Shevchenko 2021-08-11 193 wwandev = ERR_PTR(-ENOMEM);
9a44c1cc6388762 Loic Poulain 2021-04-16 194 ida_free(&wwan_dev_ids, id);
9a44c1cc6388762 Loic Poulain 2021-04-16 195 goto done_unlock;
9a44c1cc6388762 Loic Poulain 2021-04-16 196 }
9a44c1cc6388762 Loic Poulain 2021-04-16 197
9a44c1cc6388762 Loic Poulain 2021-04-16 198 wwandev->dev.parent = parent;
9a44c1cc6388762 Loic Poulain 2021-04-16 199 wwandev->dev.class = wwan_class;
9a44c1cc6388762 Loic Poulain 2021-04-16 200 wwandev->dev.type = &wwan_dev_type;
9a44c1cc6388762 Loic Poulain 2021-04-16 201 wwandev->id = id;
9a44c1cc6388762 Loic Poulain 2021-04-16 202 dev_set_name(&wwandev->dev, "wwan%d", wwandev->id);
9a44c1cc6388762 Loic Poulain 2021-04-16 203
9a44c1cc6388762 Loic Poulain 2021-04-16 204 err = device_register(&wwandev->dev);
9a44c1cc6388762 Loic Poulain 2021-04-16 205 if (err) {
9a44c1cc6388762 Loic Poulain 2021-04-16 206 put_device(&wwandev->dev);
d9d5b8961284b00 Andy Shevchenko 2021-08-11 207 wwandev = ERR_PTR(err);
d9d5b8961284b00 Andy Shevchenko 2021-08-11 208 goto done_unlock;
9a44c1cc6388762 Loic Poulain 2021-04-16 209 }
9a44c1cc6388762 Loic Poulain 2021-04-16 210
c4804670026b93f M Chetan Kumar 2021-11-20 211 wwandev_name = kobject_name(&wwandev->dev.kobj);
89654e5e973a53b Sergey Ryazanov 2021-11-28 212 #ifdef CONFIG_WWAN_DEBUGFS
c4804670026b93f M Chetan Kumar 2021-11-20 213 wwandev->debugfs_dir = debugfs_create_dir(wwandev_name,
c4804670026b93f M Chetan Kumar 2021-11-20 214 wwan_debugfs_dir);
89654e5e973a53b Sergey Ryazanov 2021-11-28 215 #endif
c4804670026b93f M Chetan Kumar 2021-11-20 216
9a44c1cc6388762 Loic Poulain 2021-04-16 217 done_unlock:
9a44c1cc6388762 Loic Poulain 2021-04-16 218 mutex_unlock(&wwan_register_lock);
9a44c1cc6388762 Loic Poulain 2021-04-16 219
9a44c1cc6388762 Loic Poulain 2021-04-16 220 return wwandev;
9a44c1cc6388762 Loic Poulain 2021-04-16 221 }
9a44c1cc6388762 Loic Poulain 2021-04-16 222
---
0-DAY CI Kernel Test Service, Intel Corporation
https://lists.01.org/hyperkitty/list/kbuild-all@lists.01.org
On Sun, 2021-11-28 at 15:55 +0300, Sergey Ryazanov wrote: > > +#ifdef CONFIG_WWAN_DEBUGFS > struct dentry *wwan_get_debugfs_dir(struct device *parent); > +#else > +static inline struct dentry *wwan_get_debugfs_dir(struct device *parent) > +{ > + return NULL; > +} > +#endif Now I have to send another email anyway ... but this one probably should be ERR_PTR(-ENODEV) or something, a la debugfs_create_dir() if debugfs is disabled, because then a trivial user of wwan's debugfs doesn't even have to care about whether it's enabled or not, it can just debugfs_create_dir() for its own and the debugfs core code will check and return immediately. Yes that's a bit more code space, but if you just have a debugfs file or two, having an extra Kconfig option is possibly overkill too. Especially if we get into this path because DEBUG_FS is disabled *entirely*, and thus all the functions will be empty inlines (but it might not be, so it should be consistent with debugfs always returning non-NULL). johannes
On Sun, Nov 28, 2021 at 8:05 PM Johannes Berg <johannes@sipsolutions.net> wrote: > On Sun, 2021-11-28 at 15:55 +0300, Sergey Ryazanov wrote: >> +#ifdef CONFIG_WWAN_DEBUGFS >> struct dentry *wwan_get_debugfs_dir(struct device *parent); >> +#else >> +static inline struct dentry *wwan_get_debugfs_dir(struct device *parent) >> +{ >> + return NULL; >> +} >> +#endif > > Now I have to send another email anyway ... but this one probably should > be ERR_PTR(-ENODEV) or something, a la debugfs_create_dir() if debugfs > is disabled, because then a trivial user of wwan's debugfs doesn't even > have to care about whether it's enabled or not, it can just > debugfs_create_dir() for its own and the debugfs core code will check > and return immediately. Yes that's a bit more code space, but if you > just have a debugfs file or two, having an extra Kconfig option is > possibly overkill too. Especially if we get into this path because > DEBUG_FS is disabled *entirely*, and thus all the functions will be > empty inlines (but it might not be, so it should be consistent with > debugfs always returning non-NULL). Nice catch, thank you! Will rework in V2 to return ERR_PTR(-ENODEV).
Add Leon to CC to merge both conversations. On Sun, Nov 28, 2021 at 8:01 PM Johannes Berg <johannes@sipsolutions.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?
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@sipsolutions.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. 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
On Mon, Nov 29, 2021 at 9:40 AM Leon Romanovsky <leon@kernel.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@sipsolutions.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. > 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. 1. https://lore.kernel.org/all/20211101035635.26999-14-ricardo.martinez@linux.intel.com
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@kernel.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@sipsolutions.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
On Tue, Nov 30, 2021 at 1:05 PM Leon Romanovsky <leon@kernel.org> wrote: > 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@kernel.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@sipsolutions.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. Ironically, I took your "don't over-engineer" argument and started removing the IOSM specific configuration option when I realized that the IOSM debugfs interface depends on relayfs and so it should select the RELAY option. Without the IOSM debugfs option, we should either place RELAY selection to an option that enables the driver itself, or to the WWAN subsystem debugfs enabling option. The former will cause the kernel inflation even with the WWAN debugfs interface disabled. The latter will simply be misleading. In the end, I decided to keep both config options in the V2. -- Sergey
On Thu, Dec 02, 2021 at 01:03:33AM +0300, Sergey Ryazanov wrote: > On Tue, Nov 30, 2021 at 1:05 PM Leon Romanovsky <leon@kernel.org> wrote: > > 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@kernel.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@sipsolutions.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. > > Ironically, I took your "don't over-engineer" argument and started > removing the IOSM specific configuration option when I realized that > the IOSM debugfs interface depends on relayfs and so it should select > the RELAY option. Without the IOSM debugfs option, we should either > place RELAY selection to an option that enables the driver itself, or > to the WWAN subsystem debugfs enabling option. The former will cause > the kernel inflation even with the WWAN debugfs interface disabled. > The latter will simply be misleading. In the end, I decided to keep > both config options in the V2. Something like this will do the trick. diff --git a/drivers/net/wwan/Kconfig b/drivers/net/wwan/Kconfig index bdb2b0e46c12..ebb7292421a1 100644 --- a/drivers/net/wwan/Kconfig +++ b/drivers/net/wwan/Kconfig @@ -85,6 +85,7 @@ config IOSM tristate "IOSM Driver for Intel M.2 WWAN Device" depends on INTEL_IOMMU select NET_DEVLINK + select RELAY if WWAN_DEBUGFS help This driver enables Intel M.2 WWAN Device communication. > > -- > Sergey
On Thu, Dec 2, 2021 at 3:03 PM Leon Romanovsky <leon@kernel.org> wrote: > On Thu, Dec 02, 2021 at 01:03:33AM +0300, Sergey Ryazanov wrote: >> Ironically, I took your "don't over-engineer" argument and started >> removing the IOSM specific configuration option when I realized that >> the IOSM debugfs interface depends on relayfs and so it should select >> the RELAY option. Without the IOSM debugfs option, we should either >> place RELAY selection to an option that enables the driver itself, or >> to the WWAN subsystem debugfs enabling option. The former will cause >> the kernel inflation even with the WWAN debugfs interface disabled. >> The latter will simply be misleading. In the end, I decided to keep >> both config options in the V2. > > Something like this will do the trick. > > diff --git a/drivers/net/wwan/Kconfig b/drivers/net/wwan/Kconfig > index bdb2b0e46c12..ebb7292421a1 100644 > --- a/drivers/net/wwan/Kconfig > +++ b/drivers/net/wwan/Kconfig > @@ -85,6 +85,7 @@ config IOSM > tristate "IOSM Driver for Intel M.2 WWAN Device" > depends on INTEL_IOMMU > select NET_DEVLINK > + select RELAY if WWAN_DEBUGFS > help > This driver enables Intel M.2 WWAN Device communication. Yes! This is exactly what I need to finally solve this puzzle. Thank you!
diff --git a/drivers/net/wwan/Kconfig b/drivers/net/wwan/Kconfig index e204e74edcec..6e1ef08650c9 100644 --- a/drivers/net/wwan/Kconfig +++ b/drivers/net/wwan/Kconfig @@ -16,6 +16,14 @@ config WWAN if WWAN +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. + 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 help Enables debugfs driver interface for traces collection. diff --git a/drivers/net/wwan/wwan_core.c b/drivers/net/wwan/wwan_core.c index 5bf62dc35ac7..b41104129d1a 100644 --- a/drivers/net/wwan/wwan_core.c +++ b/drivers/net/wwan/wwan_core.c @@ -146,6 +146,7 @@ static struct wwan_device *wwan_dev_get_by_name(const char *name) return to_wwan_dev(dev); } +#ifdef CONFIG_WWAN_DEBUGFS struct dentry *wwan_get_debugfs_dir(struct device *parent) { struct wwan_device *wwandev; @@ -157,6 +158,7 @@ struct dentry *wwan_get_debugfs_dir(struct device *parent) return wwandev->debugfs_dir; } EXPORT_SYMBOL_GPL(wwan_get_debugfs_dir); +#endif /* This function allocates and registers a new WWAN device OR if a WWAN device * already exist for the given parent, it gets a reference and return it. @@ -207,8 +209,10 @@ static struct wwan_device *wwan_create_dev(struct device *parent) } wwandev_name = kobject_name(&wwandev->dev.kobj); +#ifdef CONFIG_WWAN_DEBUGFS wwandev->debugfs_dir = debugfs_create_dir(wwandev_name, wwan_debugfs_dir); +#endif done_unlock: mutex_unlock(&wwan_register_lock); @@ -240,7 +244,9 @@ static void wwan_remove_dev(struct wwan_device *wwandev) ret = device_for_each_child(&wwandev->dev, NULL, is_wwan_child); if (!ret) { +#ifdef CONFIG_WWAN_DEBUGFS debugfs_remove_recursive(wwandev->debugfs_dir); +#endif device_unregister(&wwandev->dev); } else { put_device(&wwandev->dev); @@ -1140,7 +1146,9 @@ static int __init wwan_init(void) goto destroy; } +#ifdef CONFIG_WWAN_DEBUGFS wwan_debugfs_dir = debugfs_create_dir("wwan", NULL); +#endif return 0; diff --git a/include/linux/wwan.h b/include/linux/wwan.h index 1646aa3e6779..b84ccf7d34da 100644 --- a/include/linux/wwan.h +++ b/include/linux/wwan.h @@ -171,6 +171,13 @@ int wwan_register_ops(struct device *parent, const struct wwan_ops *ops, void wwan_unregister_ops(struct device *parent); +#ifdef CONFIG_WWAN_DEBUGFS struct dentry *wwan_get_debugfs_dir(struct device *parent); +#else +static inline struct dentry *wwan_get_debugfs_dir(struct device *parent) +{ + return NULL; +} +#endif #endif /* __WWAN_H */
Current WWAN debugfs interface does not take too much space, but it is useless without driver-specific debugfs interfaces. To avoid overloading debugfs with empty directories, make the common WWAN debugfs interface optional. And force its selection if any driver-specific interface (only IOSM at the moment) is enabled by user. Signed-off-by: Sergey Ryazanov <ryazanov.s.a@gmail.com> --- drivers/net/wwan/Kconfig | 9 +++++++++ drivers/net/wwan/wwan_core.c | 8 ++++++++ include/linux/wwan.h | 7 +++++++ 3 files changed, 24 insertions(+)