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

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

[Sergey Ryazanov]

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

-- 
2.32.0

[PATCH RESEND net-next 1/5] net: wwan: iosm: consolidate trace port init code

[Sergey Ryazanov]

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(-)

diff --git a/drivers/net/wwan/iosm/iosm_ipc_imem.c b/drivers/net/wwan/iosm/iosm_ipc_imem.c
index 1be07114c85d..49bdadb855e5 100644
--- a/drivers/net/wwan/iosm/iosm_ipc_imem.c
+++ b/drivers/net/wwan/iosm/iosm_ipc_imem.c
@@ -554,7 +554,7 @@ static void ipc_imem_run_state_worker(struct work_struct *instance)
 		ctrl_chl_idx++;
 	}
 
-	ipc_imem->trace = ipc_imem_trace_channel_init(ipc_imem);
+	ipc_imem->trace = ipc_trace_init(ipc_imem);
 	if (!ipc_imem->trace) {
 		dev_err(ipc_imem->dev, "trace channel init failed");
 		return;
diff --git a/drivers/net/wwan/iosm/iosm_ipc_imem_ops.c b/drivers/net/wwan/iosm/iosm_ipc_imem_ops.c
index 43f1796a8984..d2072a84ab08 100644
--- a/drivers/net/wwan/iosm/iosm_ipc_imem_ops.c
+++ b/drivers/net/wwan/iosm/iosm_ipc_imem_ops.c
@@ -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. */
 int ipc_imem_sys_wwan_open(struct iosm_imem *ipc_imem, int if_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 - Initializes trace channel.
- * @ipc_imem:          Pointer to iosm_imem struct.
- *
- * Returns: Pointer to trace instance on success else NULL
- */
-struct iosm_trace *ipc_imem_trace_channel_init(struct iosm_imem *ipc_imem)
-{
-	struct ipc_chnl_cfg chnl_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);
-
-	return ipc_trace_init(ipc_imem);
-}
-
 /* Map SKB to DMA for transfer */
 static int ipc_imem_map_skb_to_dma(struct iosm_imem *ipc_imem,
 				   struct sk_buff *skb)
diff --git a/drivers/net/wwan/iosm/iosm_ipc_imem_ops.h b/drivers/net/wwan/iosm/iosm_ipc_imem_ops.h
index e36ee2782629..f8afb217d9e2 100644
--- a/drivers/net/wwan/iosm/iosm_ipc_imem_ops.h
+++ b/drivers/net/wwan/iosm/iosm_ipc_imem_ops.h
@@ -141,5 +141,5 @@ int ipc_imem_sys_devlink_read(struct iosm_devlink *ipc_devlink, u8 *data,
  */
 int ipc_imem_sys_devlink_write(struct iosm_devlink *ipc_devlink,
 			       unsigned char *buf, int count);
-struct iosm_trace *ipc_imem_trace_channel_init(struct iosm_imem *ipc_imem);
+
 #endif
diff --git a/drivers/net/wwan/iosm/iosm_ipc_trace.c b/drivers/net/wwan/iosm/iosm_ipc_trace.c
index c5fa12599c2b..5f5cfd39bede 100644
--- a/drivers/net/wwan/iosm/iosm_ipc_trace.c
+++ b/drivers/net/wwan/iosm/iosm_ipc_trace.c
@@ -132,9 +132,15 @@ static const struct file_operations ipc_trace_fops = {
  */
 struct iosm_trace *ipc_trace_init(struct iosm_imem *ipc_imem)
 {
-	struct iosm_trace *ipc_trace = kzalloc(sizeof(*ipc_trace), GFP_KERNEL);
+	struct ipc_chnl_cfg chnl_cfg = { 0 };
+	struct iosm_trace *ipc_trace;
 	struct dentry *debugfs_pdev;
 
+	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);
+
+	ipc_trace = kzalloc(sizeof(*ipc_trace), GFP_KERNEL);
 	if (!ipc_trace)
 		return NULL;
 
-- 
2.32.0

[PATCH RESEND net-next 2/5] net: wwan: iosm: allow trace port be uninitialized

[Sergey Ryazanov]

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(-)

diff --git a/drivers/net/wwan/iosm/iosm_ipc_imem.c b/drivers/net/wwan/iosm/iosm_ipc_imem.c
index 49bdadb855e5..a60b93cefd2e 100644
--- a/drivers/net/wwan/iosm/iosm_ipc_imem.c
+++ b/drivers/net/wwan/iosm/iosm_ipc_imem.c
@@ -272,7 +272,7 @@ static void ipc_imem_dl_skb_process(struct iosm_imem *ipc_imem,
 		if (port_id == IPC_MEM_CTRL_CHL_ID_7)
 			ipc_imem_sys_devlink_notify_rx(ipc_imem->ipc_devlink,
 						       skb);
-		else if (port_id == ipc_imem->trace->chl_id)
+		else if (ipc_is_trace_channel(ipc_imem, port_id))
 			ipc_trace_port_rx(ipc_imem->trace, skb);
 		else
 			wwan_port_rx(ipc_imem->ipc_port[port_id]->iosm_port,
@@ -555,10 +555,8 @@ static void ipc_imem_run_state_worker(struct work_struct *instance)
 	}
 
 	ipc_imem->trace = ipc_trace_init(ipc_imem);
-	if (!ipc_imem->trace) {
-		dev_err(ipc_imem->dev, "trace channel init failed");
-		return;
-	}
+	if (!ipc_imem->trace)
+		dev_warn(ipc_imem->dev, "trace channel init failed");
 
 	ipc_task_queue_send_task(ipc_imem, ipc_imem_send_mdm_rdy_cb, 0, NULL, 0,
 				 false);
diff --git a/drivers/net/wwan/iosm/iosm_ipc_trace.c b/drivers/net/wwan/iosm/iosm_ipc_trace.c
index 5f5cfd39bede..c588a394cd94 100644
--- a/drivers/net/wwan/iosm/iosm_ipc_trace.c
+++ b/drivers/net/wwan/iosm/iosm_ipc_trace.c
@@ -172,6 +172,9 @@ struct iosm_trace *ipc_trace_init(struct iosm_imem *ipc_imem)
  */
 void ipc_trace_deinit(struct iosm_trace *ipc_trace)
 {
+	if (!ipc_trace)
+		return;
+
 	debugfs_remove(ipc_trace->ctrl_file);
 	relay_close(ipc_trace->ipc_rchan);
 	mutex_destroy(&ipc_trace->trc_mutex);
diff --git a/drivers/net/wwan/iosm/iosm_ipc_trace.h b/drivers/net/wwan/iosm/iosm_ipc_trace.h
index 53346183af9c..419540c91219 100644
--- a/drivers/net/wwan/iosm/iosm_ipc_trace.h
+++ b/drivers/net/wwan/iosm/iosm_ipc_trace.h
@@ -45,6 +45,11 @@ struct iosm_trace {
 	enum trace_ctrl_mode mode;
 };
 
+static inline bool ipc_is_trace_channel(struct iosm_imem *ipc_mem, u16 chl_id)
+{
+	return ipc_mem->trace && ipc_mem->trace->chl_id == chl_id;
+}
+
 struct iosm_trace *ipc_trace_init(struct iosm_imem *ipc_imem);
 void ipc_trace_deinit(struct iosm_trace *ipc_trace);
 void ipc_trace_port_rx(struct iosm_trace *ipc_trace, struct sk_buff *skb);
-- 
2.32.0

[PATCH RESEND net-next 3/5] net: wwan: iosm: move debugfs knobs into a subdir

[Sergey Ryazanov]

The modem traces collection is a device (and so driver) specific option.
Therefore, move the related debugfs files into a driver-specific
subdirectory under the common per WWAN device directory.

Signed-off-by: Sergey Ryazanov <ryazanov.s.a@gmail.com>
---
 drivers/net/wwan/iosm/Makefile           |  1 +
 drivers/net/wwan/iosm/iosm_ipc_debugfs.c | 29 ++++++++++++++++++++++++
 drivers/net/wwan/iosm/iosm_ipc_debugfs.h | 12 ++++++++++
 drivers/net/wwan/iosm/iosm_ipc_imem.c    |  7 +++---
 drivers/net/wwan/iosm/iosm_ipc_imem.h    |  1 +
 drivers/net/wwan/iosm/iosm_ipc_trace.c   |  6 ++---
 6 files changed, 48 insertions(+), 8 deletions(-)
 create mode 100644 drivers/net/wwan/iosm/iosm_ipc_debugfs.c
 create mode 100644 drivers/net/wwan/iosm/iosm_ipc_debugfs.h

diff --git a/drivers/net/wwan/iosm/Makefile b/drivers/net/wwan/iosm/Makefile
index 5c2528beca2a..5091f664af0d 100644
--- a/drivers/net/wwan/iosm/Makefile
+++ b/drivers/net/wwan/iosm/Makefile
@@ -22,6 +22,7 @@ iosm-y = \
 	iosm_ipc_devlink.o		\
 	iosm_ipc_flash.o		\
 	iosm_ipc_coredump.o		\
+	iosm_ipc_debugfs.o		\
 	iosm_ipc_trace.o
 
 obj-$(CONFIG_IOSM) := iosm.o
diff --git a/drivers/net/wwan/iosm/iosm_ipc_debugfs.c b/drivers/net/wwan/iosm/iosm_ipc_debugfs.c
new file mode 100644
index 000000000000..f2f57751a7d2
--- /dev/null
+++ b/drivers/net/wwan/iosm/iosm_ipc_debugfs.c
@@ -0,0 +1,29 @@
+// SPDX-License-Identifier: GPL-2.0-only
+/*
+ * Copyright (C) 2020-2021 Intel Corporation.
+ */
+
+#include <linux/debugfs.h>
+#include <linux/wwan.h>
+
+#include "iosm_ipc_imem.h"
+#include "iosm_ipc_trace.h"
+#include "iosm_ipc_debugfs.h"
+
+void ipc_debugfs_init(struct iosm_imem *ipc_imem)
+{
+	struct dentry *debugfs_pdev = wwan_get_debugfs_dir(ipc_imem->dev);
+
+	ipc_imem->debugfs_dir = debugfs_create_dir(KBUILD_MODNAME,
+						   debugfs_pdev);
+
+	ipc_imem->trace = ipc_trace_init(ipc_imem);
+	if (!ipc_imem->trace)
+		dev_warn(ipc_imem->dev, "trace channel init failed");
+}
+
+void ipc_debugfs_deinit(struct iosm_imem *ipc_imem)
+{
+	ipc_trace_deinit(ipc_imem->trace);
+	debugfs_remove_recursive(ipc_imem->debugfs_dir);
+}
diff --git a/drivers/net/wwan/iosm/iosm_ipc_debugfs.h b/drivers/net/wwan/iosm/iosm_ipc_debugfs.h
new file mode 100644
index 000000000000..35788039f13f
--- /dev/null
+++ b/drivers/net/wwan/iosm/iosm_ipc_debugfs.h
@@ -0,0 +1,12 @@
+/* SPDX-License-Identifier: GPL-2.0-only
+ *
+ * Copyright (C) 2020-2021 Intel Corporation.
+ */
+
+#ifndef IOSM_IPC_DEBUGFS_H
+#define IOSM_IPC_DEBUGFS_H
+
+void ipc_debugfs_init(struct iosm_imem *ipc_imem);
+void ipc_debugfs_deinit(struct iosm_imem *ipc_imem);
+
+#endif
diff --git a/drivers/net/wwan/iosm/iosm_ipc_imem.c b/drivers/net/wwan/iosm/iosm_ipc_imem.c
index a60b93cefd2e..25b889922912 100644
--- a/drivers/net/wwan/iosm/iosm_ipc_imem.c
+++ b/drivers/net/wwan/iosm/iosm_ipc_imem.c
@@ -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. */
 static int ipc_imem_check_wwan_ips(struct ipc_mem_channel *chnl)
@@ -554,9 +555,7 @@ static void ipc_imem_run_state_worker(struct work_struct *instance)
 		ctrl_chl_idx++;
 	}
 
-	ipc_imem->trace = ipc_trace_init(ipc_imem);
-	if (!ipc_imem->trace)
-		dev_warn(ipc_imem->dev, "trace channel init failed");
+	ipc_debugfs_init(ipc_imem);
 
 	ipc_task_queue_send_task(ipc_imem, ipc_imem_send_mdm_rdy_cb, 0, NULL, 0,
 				 false);
@@ -1173,7 +1172,7 @@ void ipc_imem_cleanup(struct iosm_imem *ipc_imem)
 
 	if (test_and_clear_bit(FULLY_FUNCTIONAL, &ipc_imem->flag)) {
 		ipc_mux_deinit(ipc_imem->mux);
-		ipc_trace_deinit(ipc_imem->trace);
+		ipc_debugfs_deinit(ipc_imem);
 		ipc_wwan_deinit(ipc_imem->wwan);
 		ipc_port_deinit(ipc_imem->ipc_port);
 	}
diff --git a/drivers/net/wwan/iosm/iosm_ipc_imem.h b/drivers/net/wwan/iosm/iosm_ipc_imem.h
index cec38009c44a..df3b471f6fa9 100644
--- a/drivers/net/wwan/iosm/iosm_ipc_imem.h
+++ b/drivers/net/wwan/iosm/iosm_ipc_imem.h
@@ -380,6 +380,7 @@ struct iosm_imem {
 	   ev_mux_net_transmit_pending:1,
 	   reset_det_n:1,
 	   pcie_wake_n:1;
+	struct dentry *debugfs_dir;
 };
 
 /**
diff --git a/drivers/net/wwan/iosm/iosm_ipc_trace.c b/drivers/net/wwan/iosm/iosm_ipc_trace.c
index c588a394cd94..5243ead90b5f 100644
--- a/drivers/net/wwan/iosm/iosm_ipc_trace.c
+++ b/drivers/net/wwan/iosm/iosm_ipc_trace.c
@@ -134,7 +134,6 @@ struct iosm_trace *ipc_trace_init(struct iosm_imem *ipc_imem)
 {
 	struct ipc_chnl_cfg chnl_cfg = { 0 };
 	struct iosm_trace *ipc_trace;
-	struct dentry *debugfs_pdev;
 
 	ipc_chnl_cfg_get(&chnl_cfg, IPC_MEM_CTRL_CHL_ID_3);
 	ipc_imem_channel_init(ipc_imem, IPC_CTYPE_CTRL, chnl_cfg,
@@ -150,15 +149,14 @@ struct iosm_trace *ipc_trace_init(struct iosm_imem *ipc_imem)
 	ipc_trace->chl_id = IPC_MEM_CTRL_CHL_ID_3;
 
 	mutex_init(&ipc_trace->trc_mutex);
-	debugfs_pdev = wwan_get_debugfs_dir(ipc_imem->dev);
 
 	ipc_trace->ctrl_file = debugfs_create_file(IOSM_TRC_DEBUGFS_TRACE_CTRL,
 						   IOSM_TRC_FILE_PERM,
-						   debugfs_pdev,
+						   ipc_imem->debugfs_dir,
 						   ipc_trace, &ipc_trace_fops);
 
 	ipc_trace->ipc_rchan = relay_open(IOSM_TRC_DEBUGFS_TRACE,
-					  debugfs_pdev,
+					  ipc_imem->debugfs_dir,
 					  IOSM_TRC_SUB_BUFF_SIZE,
 					  IOSM_TRC_N_SUB_BUFF,
 					  &relay_callbacks, NULL);
-- 
2.32.0

[PATCH RESEND net-next 4/5] net: wwan: iosm: make debugfs optional

[Sergey Ryazanov]

Collecting modem firmware traces is optional for the regular modem use.
Some distros and users will want to disable this feature for security or
kernel size reasons. So add a configuration option that allows to
completely disable the driver debugfs interface.

Signed-off-by: Sergey Ryazanov <ryazanov.s.a@gmail.com>
---
 drivers/net/wwan/Kconfig                 |  8 ++++++++
 drivers/net/wwan/iosm/Makefile           |  4 +++-
 drivers/net/wwan/iosm/iosm_ipc_debugfs.h |  5 +++++
 drivers/net/wwan/iosm/iosm_ipc_imem.c    |  2 +-
 drivers/net/wwan/iosm/iosm_ipc_imem.h    |  4 ++++
 drivers/net/wwan/iosm/iosm_ipc_trace.c   |  6 ++++--
 drivers/net/wwan/iosm/iosm_ipc_trace.h   | 20 +++++++++++++++++++-
 7 files changed, 44 insertions(+), 5 deletions(-)

diff --git a/drivers/net/wwan/Kconfig b/drivers/net/wwan/Kconfig
index 17543be14665..e204e74edcec 100644
--- a/drivers/net/wwan/Kconfig
+++ b/drivers/net/wwan/Kconfig
@@ -80,6 +80,14 @@ config IOSM
 
 	  If unsure, say N.
 
+config IOSM_DEBUGFS
+	bool "IOSM Debugfs support"
+	depends on IOSM && DEBUG_FS
+	help
+	  Enables debugfs driver interface for traces collection.
+
+	  If unsure, say N.
+
 endif # WWAN
 
 endmenu
diff --git a/drivers/net/wwan/iosm/Makefile b/drivers/net/wwan/iosm/Makefile
index 5091f664af0d..bf28b29f6151 100644
--- a/drivers/net/wwan/iosm/Makefile
+++ b/drivers/net/wwan/iosm/Makefile
@@ -21,7 +21,9 @@ iosm-y = \
 	iosm_ipc_mux_codec.o		\
 	iosm_ipc_devlink.o		\
 	iosm_ipc_flash.o		\
-	iosm_ipc_coredump.o		\
+	iosm_ipc_coredump.o
+
+iosm-$(CONFIG_IOSM_DEBUGFS) += \
 	iosm_ipc_debugfs.o		\
 	iosm_ipc_trace.o
 
diff --git a/drivers/net/wwan/iosm/iosm_ipc_debugfs.h b/drivers/net/wwan/iosm/iosm_ipc_debugfs.h
index 35788039f13f..3e3bb968fa03 100644
--- a/drivers/net/wwan/iosm/iosm_ipc_debugfs.h
+++ b/drivers/net/wwan/iosm/iosm_ipc_debugfs.h
@@ -6,7 +6,12 @@
 #ifndef IOSM_IPC_DEBUGFS_H
 #define IOSM_IPC_DEBUGFS_H
 
+#ifdef CONFIG_IOSM_DEBUGFS
 void ipc_debugfs_init(struct iosm_imem *ipc_imem);
 void ipc_debugfs_deinit(struct iosm_imem *ipc_imem);
+#else
+static inline void ipc_debugfs_init(struct iosm_imem *ipc_imem) {}
+static inline void ipc_debugfs_deinit(struct iosm_imem *ipc_imem) {}
+#endif
 
 #endif
diff --git a/drivers/net/wwan/iosm/iosm_ipc_imem.c b/drivers/net/wwan/iosm/iosm_ipc_imem.c
index 25b889922912..2a6ddd7c6c88 100644
--- a/drivers/net/wwan/iosm/iosm_ipc_imem.c
+++ b/drivers/net/wwan/iosm/iosm_ipc_imem.c
@@ -274,7 +274,7 @@ static void ipc_imem_dl_skb_process(struct iosm_imem *ipc_imem,
 			ipc_imem_sys_devlink_notify_rx(ipc_imem->ipc_devlink,
 						       skb);
 		else if (ipc_is_trace_channel(ipc_imem, port_id))
-			ipc_trace_port_rx(ipc_imem->trace, skb);
+			ipc_trace_port_rx(ipc_imem, skb);
 		else
 			wwan_port_rx(ipc_imem->ipc_port[port_id]->iosm_port,
 				     skb);
diff --git a/drivers/net/wwan/iosm/iosm_ipc_imem.h b/drivers/net/wwan/iosm/iosm_ipc_imem.h
index df3b471f6fa9..cca4b32c63cd 100644
--- a/drivers/net/wwan/iosm/iosm_ipc_imem.h
+++ b/drivers/net/wwan/iosm/iosm_ipc_imem.h
@@ -350,7 +350,9 @@ struct iosm_imem {
 	struct iosm_mux *mux;
 	struct iosm_cdev *ipc_port[IPC_MEM_MAX_CHANNELS];
 	struct iosm_pcie *pcie;
+#ifdef CONFIG_IOSM_DEBUGFS
 	struct iosm_trace *trace;
+#endif
 	struct device *dev;
 	enum ipc_mem_device_ipc_state ipc_requested_state;
 	struct ipc_mem_channel channels[IPC_MEM_MAX_CHANNELS];
@@ -380,7 +382,9 @@ struct iosm_imem {
 	   ev_mux_net_transmit_pending:1,
 	   reset_det_n:1,
 	   pcie_wake_n:1;
+#ifdef CONFIG_IOSM_DEBUGFS
 	struct dentry *debugfs_dir;
+#endif
 };
 
 /**
diff --git a/drivers/net/wwan/iosm/iosm_ipc_trace.c b/drivers/net/wwan/iosm/iosm_ipc_trace.c
index 5243ead90b5f..eeecfa3d10c5 100644
--- a/drivers/net/wwan/iosm/iosm_ipc_trace.c
+++ b/drivers/net/wwan/iosm/iosm_ipc_trace.c
@@ -17,11 +17,13 @@
 
 /**
  * ipc_trace_port_rx - Receive trace packet from cp and write to relay buffer
- * @ipc_trace:  Pointer to the ipc trace data-struct
+ * @ipc_imem:   Pointer to iosm_imem structure
  * @skb:        Pointer to struct sk_buff
  */
-void ipc_trace_port_rx(struct iosm_trace *ipc_trace, struct sk_buff *skb)
+void ipc_trace_port_rx(struct iosm_imem *ipc_imem, struct sk_buff *skb)
 {
+	struct iosm_trace *ipc_trace = ipc_imem->trace;
+
 	if (ipc_trace->ipc_rchan)
 		relay_write(ipc_trace->ipc_rchan, skb->data, skb->len);
 
diff --git a/drivers/net/wwan/iosm/iosm_ipc_trace.h b/drivers/net/wwan/iosm/iosm_ipc_trace.h
index 419540c91219..0d74836df90c 100644
--- a/drivers/net/wwan/iosm/iosm_ipc_trace.h
+++ b/drivers/net/wwan/iosm/iosm_ipc_trace.h
@@ -45,6 +45,8 @@ struct iosm_trace {
 	enum trace_ctrl_mode mode;
 };
 
+#ifdef CONFIG_IOSM_DEBUGFS
+
 static inline bool ipc_is_trace_channel(struct iosm_imem *ipc_mem, u16 chl_id)
 {
 	return ipc_mem->trace && ipc_mem->trace->chl_id == chl_id;
@@ -52,5 +54,21 @@ static inline bool ipc_is_trace_channel(struct iosm_imem *ipc_mem, u16 chl_id)
 
 struct iosm_trace *ipc_trace_init(struct iosm_imem *ipc_imem);
 void ipc_trace_deinit(struct iosm_trace *ipc_trace);
-void ipc_trace_port_rx(struct iosm_trace *ipc_trace, struct sk_buff *skb);
+void ipc_trace_port_rx(struct iosm_imem *ipc_imem, struct sk_buff *skb);
+
+#else
+
+static inline bool ipc_is_trace_channel(struct iosm_imem *ipc_mem, u16 chl_id)
+{
+	return false;
+}
+
+static inline void ipc_trace_port_rx(struct iosm_imem *ipc_imem,
+				     struct sk_buff *skb)
+{
+	dev_kfree_skb(skb);
+}
+
+#endif
+
 #endif
-- 
2.32.0

[PATCH RESEND net-next 5/5] net: wwan: core: make debugfs optional

[Sergey Ryazanov]

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(+)

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 */
-- 
2.32.0

Re: [PATCH RESEND net-next 5/5] net: wwan: core: make debugfs optional

[Johannes Berg]

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

Re: [PATCH RESEND net-next 5/5] net: wwan: core: make debugfs optional

[Sergey Ryazanov]

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?

-- 
Sergey

Re: [PATCH RESEND net-next 5/5] net: wwan: core: make debugfs optional

[Leon Romanovsky]

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

Re: [PATCH RESEND net-next 5/5] net: wwan: core: make debugfs optional

[Sergey Ryazanov]

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

-- 
Sergey

Re: [PATCH RESEND net-next 5/5] net: wwan: core: make debugfs optional

[Leon Romanovsky]

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

Re: [PATCH RESEND net-next 5/5] net: wwan: core: make debugfs optional

[Sergey Ryazanov]

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

Re: [PATCH RESEND net-next 5/5] net: wwan: core: make debugfs optional

[Leon Romanovsky]

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

Re: [PATCH RESEND net-next 5/5] net: wwan: core: make debugfs optional

[Sergey Ryazanov]

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!

-- 
Sergey

Re: [PATCH RESEND net-next 5/5] net: wwan: core: make debugfs optional

[kernel test robot]

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

Re: [PATCH RESEND net-next 5/5] net: wwan: core: make debugfs optional

[kernel test robot]

[-- Attachment #1: Type: text/plain, Size: 6449 bytes --]

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(a)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(a)lists.01.org

Re: [PATCH RESEND net-next 5/5] net: wwan: core: make debugfs optional

[Johannes Berg]

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

Re: [PATCH RESEND net-next 5/5] net: wwan: core: make debugfs optional

[Sergey Ryazanov]

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).

-- 
Sergey

Re: [PATCH RESEND net-next 0/5] WWAN debugfs tweaks

[Leon Romanovsky]

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

Re: [PATCH RESEND net-next 0/5] WWAN debugfs tweaks

[Johannes Berg]

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