mbox series

[RESEND,net-next,0/5] WWAN debugfs tweaks

Message ID 20211128125522.23357-1-ryazanov.s.a@gmail.com (mailing list archive)
Headers show
Series WWAN debugfs tweaks | expand

Message

Sergey Ryazanov Nov. 28, 2021, 12:55 p.m. UTC 
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 <m.chetan.kumar@intel.com>
Cc: Intel Corporation <linuxwwan@intel.com>
Cc: Loic Poulain <loic.poulain@linaro.org>
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

Comments

Leon Romanovsky Nov. 28, 2021, 6:27 p.m. UTC | #1 
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
Johannes Berg Nov. 30, 2021, 8:03 a.m. UTC | #2 
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