Resend with proper target tree. Also I should mention that the series is
mostly compile-tested since I do not have IOSM supported device, so it
needs Ack from the IOSM developers.
This is a follow-up series to just applied IOSM (and WWAN) debugfs
interface support [1]. The series has two main goals:
1. move the driver-specific debugfs knobs to a subdirectory;
2. make the debugfs interface optional for both IOSM and for the WWAN
core.
As for the first part, I must say that it was my mistake. I suggested to
place debugfs entries under a common per WWAN device directory. But I
missed the driver subdirectory in the example, so it become:
/sys/kernel/debugfs/wwan/wwan0/trace
Since the traces collection is a driver-specific feature, it is better
to keep it under the driver-specific subdirectory:
/sys/kernel/debugfs/wwan/wwan0/iosm/trace
It is desirable to be able to entirely disable the debugfs interface. It
can be disabled for several reasons, including security and consumed
storage space.
The changes themselves are relatively simple, but require a code
rearrangement. So to make changes clear, I chose to split them into
preparatory and main changes and properly describe each of them.
1. https://lore.kernel.org/netdev/20211120162155.1216081-1-m.chetan.kumar@linux.intel.com
Cc: M Chetan Kumar <redacted>
Cc: Intel Corporation <redacted>
Cc: Loic Poulain <redacted>
Cc: Johannes Berg <johannes@sipsolutions.net>
Sergey Ryazanov (5):
net: wwan: iosm: consolidate trace port init code
net: wwan: iosm: allow trace port be uninitialized
net: wwan: iosm: move debugfs knobs into a subdir
net: wwan: iosm: make debugfs optional
net: wwan: core: make debugfs optional
drivers/net/wwan/Kconfig | 17 +++++++++++++
drivers/net/wwan/iosm/Makefile | 5 +++-
drivers/net/wwan/iosm/iosm_ipc_debugfs.c | 29 +++++++++++++++++++++++
drivers/net/wwan/iosm/iosm_ipc_debugfs.h | 17 +++++++++++++
drivers/net/wwan/iosm/iosm_ipc_imem.c | 13 ++++------
drivers/net/wwan/iosm/iosm_ipc_imem.h | 5 ++++
drivers/net/wwan/iosm/iosm_ipc_imem_ops.c | 18 --------------
drivers/net/wwan/iosm/iosm_ipc_imem_ops.h | 2 +-
drivers/net/wwan/iosm/iosm_ipc_trace.c | 23 ++++++++++++------
drivers/net/wwan/iosm/iosm_ipc_trace.h | 25 ++++++++++++++++++-
drivers/net/wwan/wwan_core.c | 8 +++++++
include/linux/wwan.h | 7 ++++++
12 files changed, 133 insertions(+), 36 deletions(-)
create mode 100644 drivers/net/wwan/iosm/iosm_ipc_debugfs.c
create mode 100644 drivers/net/wwan/iosm/iosm_ipc_debugfs.h
--
2.32.0
Move the channel related structures initialization from
ipc_imem_channel_init() to ipc_trace_init() and call it directly. On the
one hand, this makes the trace port initialization symmetric to the
deitialization, that is, it removes the additional wrapper.
On the other hand, this change consolidates the trace port related code
into a single source file, what facilitates an upcoming disabling of
this functionality by a user choise.
Signed-off-by: Sergey Ryazanov <ryazanov.s.a@gmail.com>
---
drivers/net/wwan/iosm/iosm_ipc_imem.c | 2 +-
drivers/net/wwan/iosm/iosm_ipc_imem_ops.c | 18 ------------------
drivers/net/wwan/iosm/iosm_ipc_imem_ops.h | 2 +-
drivers/net/wwan/iosm/iosm_ipc_trace.c | 8 +++++++-
4 files changed, 9 insertions(+), 21 deletions(-)
@@ -11,7 +11,6 @@#include"iosm_ipc_imem_ops.h"#include"iosm_ipc_port.h"#include"iosm_ipc_task_queue.h"-#include"iosm_ipc_trace.h"/* Open a packet data online channel between the network layer and CP. */intipc_imem_sys_wwan_open(structiosm_imem*ipc_imem,intif_id)
@@ -108,23 +107,6 @@ void ipc_imem_wwan_channel_init(struct iosm_imem *ipc_imem,"failed to register the ipc_wwan interfaces");}-/**-*ipc_imem_trace_channel_init-Initializestracechannel.-*@ipc_imem:Pointertoiosm_imemstruct.-*-*Returns:PointertotraceinstanceonsuccesselseNULL-*/-structiosm_trace*ipc_imem_trace_channel_init(structiosm_imem*ipc_imem)-{-structipc_chnl_cfgchnl_cfg={0};--ipc_chnl_cfg_get(&chnl_cfg,IPC_MEM_CTRL_CHL_ID_3);-ipc_imem_channel_init(ipc_imem,IPC_CTYPE_CTRL,chnl_cfg,-IRQ_MOD_OFF);--returnipc_trace_init(ipc_imem);-}-/* Map SKB to DMA for transfer */staticintipc_imem_map_skb_to_dma(structiosm_imem*ipc_imem,structsk_buff*skb)
Collecting modem firmware traces is optional for the regular modem use.
There are not many reasons for aborting device initialization due to an
inability to initialize the trace port and (or) its debugfs interface.
So, demote the initialization failure erro message into a warning and do
not break the initialization sequence in this case. Rework packet
processing and deinitialization so that they do not crash in case of
uninitialized trace port.
This change is mainly a preparation for an upcoming configuration option
introduction that will help disable driver debugfs functionality.
Signed-off-by: Sergey Ryazanov <ryazanov.s.a@gmail.com>
---
drivers/net/wwan/iosm/iosm_ipc_imem.c | 8 +++-----
drivers/net/wwan/iosm/iosm_ipc_trace.c | 3 +++
drivers/net/wwan/iosm/iosm_ipc_trace.h | 5 +++++
3 files changed, 11 insertions(+), 5 deletions(-)
@@ -11,6 +11,7 @@#include"iosm_ipc_imem.h"#include"iosm_ipc_port.h"#include"iosm_ipc_trace.h"+#include"iosm_ipc_debugfs.h"/* Check the wwan ips if it is valid with Channel as input. */staticintipc_imem_check_wwan_ips(structipc_mem_channel*chnl)
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(+)
@@ -157,6 +158,7 @@ struct dentry *wwan_get_debugfs_dir(struct device *parent)returnwwandev->debugfs_dir;}EXPORT_SYMBOL_GPL(wwan_get_debugfs_dir);+#endif/* This function allocates and registers a new WWAN device OR if a WWAN device*alreadyexistforthegivenparent,itgetsareferenceandreturnit.
From: Johannes Berg <johannes@sipsolutions.net> Date: 2021-11-28 17:04:01
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?
quoted hunk
config WWAN_HWSIM
tristate "Simulated WWAN device"
help
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
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
From: Leon Romanovsky <leon@kernel.org> Date: 2021-11-28 18:31:57
On Sun, Nov 28, 2021 at 03:55:17PM +0300, Sergey Ryazanov wrote:
Resend with proper target tree. Also I should mention that the series is
mostly compile-tested since I do not have IOSM supported device, so it
needs Ack from the IOSM developers.
This is a follow-up series to just applied IOSM (and WWAN) debugfs
interface support [1]. The series has two main goals:
1. move the driver-specific debugfs knobs to a subdirectory;
2. make the debugfs interface optional for both IOSM and for the WWAN
core.
As for the first part, I must say that it was my mistake. I suggested to
place debugfs entries under a common per WWAN device directory. But I
missed the driver subdirectory in the example, so it become:
/sys/kernel/debugfs/wwan/wwan0/trace
Since the traces collection is a driver-specific feature, it is better
to keep it under the driver-specific subdirectory:
/sys/kernel/debugfs/wwan/wwan0/iosm/trace
It is desirable to be able to entirely disable the debugfs interface. It
can be disabled for several reasons, including security and consumed
storage space.
When such needs arise, the disable is done with CONFIG_DEBUGFS knob and
not with per-subsystem configs.
I personally see your CONFIG_*_DEBUGFS patches as a mistake, which
complicates code without any gain at all. Even an opposite is true,
by adding more knobs, you can find yourself with the system which
has CONFIG_DEBUGFS enabled but with your CONFIG_*_DEBUGFS disabled.
Thanks
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).
--
Sergey
Add Leon to CC to merge both conversations.
On Sun, Nov 28, 2021 at 8:01 PM Johannes Berg [off-list ref] wrote:
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
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?
--
Sergey
From: Leon Romanovsky <leon@kernel.org> Date: 2021-11-29 06:42:51
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
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
On Mon, Nov 29, 2021 at 9:40 AM Leon Romanovsky [off-list ref] wrote:
On Mon, Nov 29, 2021 at 02:45:16AM +0300, Sergey Ryazanov wrote:
quoted
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
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
--
Sergey
From: Johannes Berg <johannes@sipsolutions.net> Date: 2021-11-30 08:03:34
On Sun, 2021-11-28 at 20:27 +0200, Leon Romanovsky wrote:
I personally see your CONFIG_*_DEBUGFS patches as a mistake, which
complicates code without any gain at all. Even an opposite is true,
by adding more knobs, you can find yourself with the system which
has CONFIG_DEBUGFS enabled but with your CONFIG_*_DEBUGFS disabled.
I tend to agree with this - it has already happened to me "in the wild"
that I've had to walk people through a handful of DEBUGFS options to
finally get all the right data ...
johannes
From: Leon Romanovsky <leon@kernel.org> Date: 2021-11-30 10:05:19
On Tue, Nov 30, 2021 at 02:44:35AM +0300, Sergey Ryazanov wrote:
On Mon, Nov 29, 2021 at 9:40 AM Leon Romanovsky [off-list ref] wrote:
quoted
On Mon, Nov 29, 2021 at 02:45:16AM +0300, Sergey Ryazanov wrote:
quoted
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
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.
quoted
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.
On Tue, Nov 30, 2021 at 1:05 PM Leon Romanovsky [off-list ref] wrote:
On Tue, Nov 30, 2021 at 02:44:35AM +0300, Sergey Ryazanov wrote:
quoted
On Mon, Nov 29, 2021 at 9:40 AM Leon Romanovsky [off-list ref] wrote:
quoted
On Mon, Nov 29, 2021 at 02:45:16AM +0300, Sergey Ryazanov wrote:
quoted
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
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.
quoted
quoted
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
From: Leon Romanovsky <leon@kernel.org> Date: 2021-12-02 12:03:44
On Thu, Dec 02, 2021 at 01:03:33AM +0300, Sergey Ryazanov wrote:
On Tue, Nov 30, 2021 at 1:05 PM Leon Romanovsky [off-list ref] wrote:
quoted
On Tue, Nov 30, 2021 at 02:44:35AM +0300, Sergey Ryazanov wrote:
quoted
On Mon, Nov 29, 2021 at 9:40 AM Leon Romanovsky [off-list ref] wrote:
quoted
On Mon, Nov 29, 2021 at 02:45:16AM +0300, Sergey Ryazanov wrote:
quoted
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
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.
quoted
quoted
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.
On Thu, Dec 2, 2021 at 3:03 PM Leon Romanovsky [off-list ref] wrote:
quoted hunk
On Thu, Dec 02, 2021 at 01:03:33AM +0300, Sergey Ryazanov wrote:
quoted
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.