[net-next v2 1/2] wwan: core: Add WWAN ADB and MIPC port type

[net-next v2 1/2] wwan: core: Add WWAN ADB and MIPC port type

[Jinjian Song]

Add new WWAN ports that connect to the device's ADB protocol interface and MTK
MIPC diagnostic interface.

Signed-off-by: Jinjian Song <jinjian.song@fibocom.com>
---
 drivers/net/wwan/wwan_core.c | 8 ++++++++
 include/linux/wwan.h         | 4 ++++
 2 files changed, 12 insertions(+)

diff --git a/drivers/net/wwan/wwan_core.c b/drivers/net/wwan/wwan_core.c
index 17431f1b1a0c..5ffa70d5de85 100644
--- a/drivers/net/wwan/wwan_core.c
+++ b/drivers/net/wwan/wwan_core.c
@@ -334,6 +334,14 @@ static const struct {
 		.name = "FASTBOOT",
 		.devsuf = "fastboot",
 	},
+	[WWAN_PORT_ADB] = {
+		.name = "ADB",
+		.devsuf = "adb",
+	},
+	[WWAN_PORT_MIPC] = {
+		.name = "MIPC",
+		.devsuf = "mipc",
+	},
 };
 
 static ssize_t type_show(struct device *dev, struct device_attribute *attr,
diff --git a/include/linux/wwan.h b/include/linux/wwan.h
index 170fdee6339c..79c781875c09 100644
--- a/include/linux/wwan.h
+++ b/include/linux/wwan.h
@@ -17,6 +17,8 @@
  * @WWAN_PORT_FIREHOSE: XML based command protocol
  * @WWAN_PORT_XMMRPC: Control protocol for Intel XMM modems
  * @WWAN_PORT_FASTBOOT: Fastboot protocol control
+ * @WWAN_PORT_ADB: ADB protocol control
+ * @WWAN_PORT_MIPC: MTK MIPC diagnostic interface
  *
  * @WWAN_PORT_MAX: Highest supported port types
  * @WWAN_PORT_UNKNOWN: Special value to indicate an unknown port type
@@ -30,6 +32,8 @@ enum wwan_port_type {
 	WWAN_PORT_FIREHOSE,
 	WWAN_PORT_XMMRPC,
 	WWAN_PORT_FASTBOOT,
+	WWAN_PORT_ADB,
+	WWAN_PORT_MIPC,
 
 	/* Add new port types above this line */
 
-- 
2.34.1

Re: [net-next v2 1/2] wwan: core: Add WWAN ADB and MIPC port type

[Sergey Ryazanov]

On 25.06.2024 11:45, Jinjian Song wrote:
> Add new WWAN ports that connect to the device's ADB protocol interface and MTK
> MIPC diagnostic interface.
> 
> Signed-off-by: Jinjian Song <jinjian.song@fibocom.com>

Reviewed-by: Sergey Ryazanov <ryazanov.s.a@gmail.com>

[net-next v2 2/2] net: wwan: t7xx: Add debug port

[Jinjian Song]

Add support for userspace to switch on the debug port(ADB,MIPC).
 - ADB port: /dev/wwan0adb0
 - MIPC port: /dev/wwan0mipc0

Application can use ADB (Android Debg Bridge) port to implement
functions (shell, pull, push ...) by ADB protocol commands.
E.g., ADB commands:
 - A_OPEN: OPEN(local-id, 0, "destination")
 - A_WRTE: WRITE(local-id, remote-id, "data")
 - A_OKEY: READY(local-id, remote-id, "")
 - A_CLSE: CLOSE(local-id, remote-id, "")

Link: https://android.googlesource.com/platform/packages/modules/adb/+/refs/heads/main/README.md

Application can use MIPC (Modem Information Process Center) port
to debug antenna tunner or noise profiling through this MTK modem
diagnostic interface.

By default, debug ports are not exposed, so using the command
to enable or disable debug ports.

Switch on debug port:
 - debug: 'echo debug > /sys/bus/pci/devices/${bdf}/t7xx_mode

Switch off debug port:
 - normal: 'echo normal > /sys/bus/pci/devices/${bdf}/t7xx_mode

Signed-off-by: Jinjian Song <jinjian.song@fibocom.com>
---
 .../networking/device_drivers/wwan/t7xx.rst   | 29 ++++++++++++
 drivers/net/wwan/t7xx/t7xx_pci.c              |  7 +++
 drivers/net/wwan/t7xx/t7xx_pci.h              |  2 +
 drivers/net/wwan/t7xx/t7xx_port.h             |  3 ++
 drivers/net/wwan/t7xx/t7xx_port_proxy.c       | 45 ++++++++++++++++++-
 drivers/net/wwan/t7xx/t7xx_port_proxy.h       |  1 +
 drivers/net/wwan/t7xx/t7xx_port_wwan.c        |  8 +++-
 7 files changed, 91 insertions(+), 4 deletions(-)

diff --git a/Documentation/networking/device_drivers/wwan/t7xx.rst b/Documentation/networking/device_drivers/wwan/t7xx.rst
index f346f5f85f15..3d70c5e3f769 100644
--- a/Documentation/networking/device_drivers/wwan/t7xx.rst
+++ b/Documentation/networking/device_drivers/wwan/t7xx.rst
@@ -56,6 +56,10 @@ Device mode:
 - ``fastboot_switching`` represents that device in fastboot switching status
 - ``fastboot_download`` represents that device in fastboot download status
 - ``fastboot_dump`` represents that device in fastboot dump status
+- ``debug`` represents switching on debug ports (write only)
+- ``normal`` represents switching off debug ports (write only)
+
+Currently supported debug ports (ADB/MIPC).
 
 Read from userspace to get the current device mode.
 
@@ -139,6 +143,25 @@ Please note that driver needs to be reloaded to export /dev/wwan0fastboot0
 port, because device needs a cold reset after enter ``fastboot_switching``
 mode.
 
+ADB port userspace ABI
+----------------------
+
+/dev/wwan0adb0 character device
+~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
+The driver exposes a ADB protocol interface by implementing ADB WWAN Port.
+The userspace end of the ADB channel pipe is a /dev/wwan0adb0 character device.
+Application shall use this interface for ADB protocol communication.
+
+MIPC port userspace ABI
+-----------------------
+
+/dev/wwan0mipc0 character device
+~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
+The driver exposes a diagnostic interface by implementing MIPC (Modem
+Information Process Center) WWAN Port. The userspace end of the MIPC channel
+pipe is a /dev/wwan0mipc0 character device.
+Application shall use this interface for MTK modem diagnostic communication.
+
 The MediaTek's T700 modem supports the 3GPP TS 27.007 [4] specification.
 
 References
@@ -164,3 +187,9 @@ speak the Mobile Interface Broadband Model (MBIM) protocol"*
 [5] *fastboot "a mechanism for communicating with bootloaders"*
 
 - https://android.googlesource.com/platform/system/core/+/refs/heads/main/fastboot/README.md
+
+[6] *ADB (Android Debug Bridge) "a mechanism to keep track of Android devices and
+emulators instances connected to or running on a given host developer machine with
+ADB protocol"*
+
+- https://android.googlesource.com/platform/packages/modules/adb/+/refs/heads/main/README.md
diff --git a/drivers/net/wwan/t7xx/t7xx_pci.c b/drivers/net/wwan/t7xx/t7xx_pci.c
index e0b1e7a616ca..6b18460d626c 100644
--- a/drivers/net/wwan/t7xx/t7xx_pci.c
+++ b/drivers/net/wwan/t7xx/t7xx_pci.c
@@ -41,6 +41,7 @@
 #include "t7xx_pcie_mac.h"
 #include "t7xx_reg.h"
 #include "t7xx_state_monitor.h"
+#include "t7xx_port_proxy.h"
 
 #define T7XX_PCI_IREG_BASE		0
 #define T7XX_PCI_EREG_BASE		2
@@ -59,6 +60,8 @@ static const char * const t7xx_mode_names[] = {
 	[T7XX_FASTBOOT_SWITCHING] = "fastboot_switching",
 	[T7XX_FASTBOOT_DOWNLOAD] = "fastboot_download",
 	[T7XX_FASTBOOT_DUMP] = "fastboot_dump",
+	[T7XX_DEBUG] = "debug",
+	[T7XX_NORMAL] = "normal",
 };
 
 static_assert(ARRAY_SIZE(t7xx_mode_names) == T7XX_MODE_LAST);
@@ -82,6 +85,10 @@ static ssize_t t7xx_mode_store(struct device *dev,
 	} else if (index == T7XX_RESET) {
 		WRITE_ONCE(t7xx_dev->mode, T7XX_RESET);
 		t7xx_acpi_pldr_func(t7xx_dev);
+	} else if (index == T7XX_DEBUG) {
+		t7xx_proxy_port_debug(t7xx_dev, true);
+	} else if (index == T7XX_NORMAL) {
+		t7xx_proxy_port_debug(t7xx_dev, false);
 	}
 
 	return count;
diff --git a/drivers/net/wwan/t7xx/t7xx_pci.h b/drivers/net/wwan/t7xx/t7xx_pci.h
index 49a11586d8d8..bdcadeb035e0 100644
--- a/drivers/net/wwan/t7xx/t7xx_pci.h
+++ b/drivers/net/wwan/t7xx/t7xx_pci.h
@@ -50,6 +50,8 @@ enum t7xx_mode {
 	T7XX_FASTBOOT_SWITCHING,
 	T7XX_FASTBOOT_DOWNLOAD,
 	T7XX_FASTBOOT_DUMP,
+	T7XX_DEBUG,
+	T7XX_NORMAL,
 	T7XX_MODE_LAST, /* must always be last */
 };
 
diff --git a/drivers/net/wwan/t7xx/t7xx_port.h b/drivers/net/wwan/t7xx/t7xx_port.h
index f74d3bab810d..9f5d6d288c97 100644
--- a/drivers/net/wwan/t7xx/t7xx_port.h
+++ b/drivers/net/wwan/t7xx/t7xx_port.h
@@ -42,6 +42,8 @@ enum port_ch {
 	/* to AP */
 	PORT_CH_AP_CONTROL_RX = 0x1000,
 	PORT_CH_AP_CONTROL_TX = 0x1001,
+	PORT_CH_AP_ADB_RX = 0x100a,
+	PORT_CH_AP_ADB_TX = 0x100b,
 
 	/* to MD */
 	PORT_CH_CONTROL_RX = 0x2000,
@@ -100,6 +102,7 @@ struct t7xx_port_conf {
 	struct port_ops		*ops;
 	char			*name;
 	enum wwan_port_type	port_type;
+	bool			debug;
 };
 
 struct t7xx_port {
diff --git a/drivers/net/wwan/t7xx/t7xx_port_proxy.c b/drivers/net/wwan/t7xx/t7xx_port_proxy.c
index 7d6388bf1d7c..3510f9013811 100644
--- a/drivers/net/wwan/t7xx/t7xx_port_proxy.c
+++ b/drivers/net/wwan/t7xx/t7xx_port_proxy.c
@@ -39,6 +39,8 @@
 
 #define Q_IDX_CTRL			0
 #define Q_IDX_MBIM			2
+#define Q_IDX_MIPC			2
+#define Q_IDX_ADB			3
 #define Q_IDX_AT_CMD			5
 
 #define INVALID_SEQ_NUM			GENMASK(15, 0)
@@ -100,7 +102,27 @@ static const struct t7xx_port_conf t7xx_port_conf[] = {
 		.path_id = CLDMA_ID_AP,
 		.ops = &ctl_port_ops,
 		.name = "t7xx_ap_ctrl",
-	},
+	}, {
+		.tx_ch = PORT_CH_AP_ADB_TX,
+		.rx_ch = PORT_CH_AP_ADB_RX,
+		.txq_index = Q_IDX_ADB,
+		.rxq_index = Q_IDX_ADB,
+		.path_id = CLDMA_ID_AP,
+		.ops = &wwan_sub_port_ops,
+		.name = "adb",
+		.port_type = WWAN_PORT_ADB,
+		.debug = true,
+	}, {
+		.tx_ch = PORT_CH_MIPC_TX,
+		.rx_ch = PORT_CH_MIPC_RX,
+		.txq_index = Q_IDX_MIPC,
+		.rxq_index = Q_IDX_MIPC,
+		.path_id = CLDMA_ID_MD,
+		.ops = &wwan_sub_port_ops,
+		.name = "mipc",
+		.port_type = WWAN_PORT_MIPC,
+		.debug = true,
+	}
 };
 
 static const struct t7xx_port_conf t7xx_early_port_conf[] = {
@@ -505,13 +527,32 @@ static void t7xx_proxy_init_all_ports(struct t7xx_modem *md)
 		spin_lock_init(&port->port_update_lock);
 		port->chan_enable = false;
 
-		if (port_conf->ops && port_conf->ops->init)
+		if (!port_conf->debug && port_conf->ops && port_conf->ops->init)
 			port_conf->ops->init(port);
 	}
 
 	t7xx_proxy_setup_ch_mapping(port_prox);
 }
 
+void t7xx_proxy_port_debug(struct t7xx_pci_dev *t7xx_dev, bool show)
+{
+	struct port_proxy *port_prox = t7xx_dev->md->port_prox;
+	struct t7xx_port *port;
+	int i;
+
+	for_each_proxy_port(i, port, port_prox) {
+		const struct t7xx_port_conf *port_conf = port->port_conf;
+
+		spin_lock_init(&port->port_update_lock);
+		if (port_conf->debug && port_conf->ops && port_conf->ops->init) {
+			if (show)
+				port_conf->ops->init(port);
+			else
+				port_conf->ops->uninit(port);
+		}
+	}
+}
+
 void t7xx_port_proxy_set_cfg(struct t7xx_modem *md, enum port_cfg_id cfg_id)
 {
 	struct port_proxy *port_prox = md->port_prox;
diff --git a/drivers/net/wwan/t7xx/t7xx_port_proxy.h b/drivers/net/wwan/t7xx/t7xx_port_proxy.h
index 7f5706811445..a9c19c1253e6 100644
--- a/drivers/net/wwan/t7xx/t7xx_port_proxy.h
+++ b/drivers/net/wwan/t7xx/t7xx_port_proxy.h
@@ -98,6 +98,7 @@ extern struct port_ops ctl_port_ops;
 extern struct port_ops t7xx_trace_port_ops;
 #endif
 
+void t7xx_proxy_port_debug(struct t7xx_pci_dev *t7xx_dev, bool show);
 void t7xx_port_proxy_reset(struct port_proxy *port_prox);
 void t7xx_port_proxy_uninit(struct port_proxy *port_prox);
 int t7xx_port_proxy_init(struct t7xx_modem *md);
diff --git a/drivers/net/wwan/t7xx/t7xx_port_wwan.c b/drivers/net/wwan/t7xx/t7xx_port_wwan.c
index 4b23ba693f3f..7fc569565ff9 100644
--- a/drivers/net/wwan/t7xx/t7xx_port_wwan.c
+++ b/drivers/net/wwan/t7xx/t7xx_port_wwan.c
@@ -169,7 +169,9 @@ static int t7xx_port_wwan_init(struct t7xx_port *port)
 {
 	const struct t7xx_port_conf *port_conf = port->port_conf;
 
-	if (port_conf->port_type == WWAN_PORT_FASTBOOT)
+	if (port_conf->port_type == WWAN_PORT_FASTBOOT ||
+	    port_conf->port_type == WWAN_PORT_ADB ||
+	    port_conf->port_type == WWAN_PORT_MIPC)
 		t7xx_port_wwan_create(port);
 
 	port->rx_length_th = RX_QUEUE_MAXLEN;
@@ -224,7 +226,9 @@ static void t7xx_port_wwan_md_state_notify(struct t7xx_port *port, unsigned int
 {
 	const struct t7xx_port_conf *port_conf = port->port_conf;
 
-	if (port_conf->port_type == WWAN_PORT_FASTBOOT)
+	if (port_conf->port_type == WWAN_PORT_FASTBOOT ||
+	    port_conf->port_type == WWAN_PORT_ADB ||
+	    port_conf->port_type == WWAN_PORT_MIPC)
 		return;
 
 	if (state != MD_STATE_READY)
-- 
2.34.1

Re: [net-next v2 2/2] net: wwan: t7xx: Add debug port

[Sergey Ryazanov]

Hello Jinjian,

On 25.06.2024 11:45, Jinjian Song wrote:
> Add support for userspace to switch on the debug port(ADB,MIPC).
>   - ADB port: /dev/wwan0adb0
>   - MIPC port: /dev/wwan0mipc0
> 
> Application can use ADB (Android Debg Bridge) port to implement
> functions (shell, pull, push ...) by ADB protocol commands.
> E.g., ADB commands:
>   - A_OPEN: OPEN(local-id, 0, "destination")
>   - A_WRTE: WRITE(local-id, remote-id, "data")
>   - A_OKEY: READY(local-id, remote-id, "")
>   - A_CLSE: CLOSE(local-id, remote-id, "")
> 
> Link: https://android.googlesource.com/platform/packages/modules/adb/+/refs/heads/main/README.md
> 
> Application can use MIPC (Modem Information Process Center) port
> to debug antenna tunner or noise profiling through this MTK modem
> diagnostic interface.
> 
> By default, debug ports are not exposed, so using the command
> to enable or disable debug ports.
> 
> Switch on debug port:
>   - debug: 'echo debug > /sys/bus/pci/devices/${bdf}/t7xx_mode
> 
> Switch off debug port:
>   - normal: 'echo normal > /sys/bus/pci/devices/${bdf}/t7xx_mode
> 
> Signed-off-by: Jinjian Song <jinjian.song@fibocom.com>
> ---
>   .../networking/device_drivers/wwan/t7xx.rst   | 29 ++++++++++++
>   drivers/net/wwan/t7xx/t7xx_pci.c              |  7 +++
>   drivers/net/wwan/t7xx/t7xx_pci.h              |  2 +
>   drivers/net/wwan/t7xx/t7xx_port.h             |  3 ++
>   drivers/net/wwan/t7xx/t7xx_port_proxy.c       | 45 ++++++++++++++++++-
>   drivers/net/wwan/t7xx/t7xx_port_proxy.h       |  1 +
>   drivers/net/wwan/t7xx/t7xx_port_wwan.c        |  8 +++-
>   7 files changed, 91 insertions(+), 4 deletions(-)
> 
> diff --git a/Documentation/networking/device_drivers/wwan/t7xx.rst b/Documentation/networking/device_drivers/wwan/t7xx.rst
> index f346f5f85f15..3d70c5e3f769 100644
> --- a/Documentation/networking/device_drivers/wwan/t7xx.rst
> +++ b/Documentation/networking/device_drivers/wwan/t7xx.rst
> @@ -56,6 +56,10 @@ Device mode:
>   - ``fastboot_switching`` represents that device in fastboot switching status
>   - ``fastboot_download`` represents that device in fastboot download status
>   - ``fastboot_dump`` represents that device in fastboot dump status
> +- ``debug`` represents switching on debug ports (write only)
> +- ``normal`` represents switching off debug ports (write only)
> +
> +Currently supported debug ports (ADB/MIPC)

Could you clarify a bit the availability of these debug ports (ADB and 
MIPC)? Looks like these ports are always available when a modem is 
booted in a 'normal' mode. They are available among other types of ports 
like AT, MBIM, etc. And you want to hide them, or speaking more 
precisely, you want to make debug ports availability configurable. Do I 
understand it right?

I just have doubts regarding the chosen configuration approach. We 
already have a 'ready' mode indicating a normal operation. Now we are 
introducing a 'debug' mode that only makes available ADB/MIPC ports, but 
reading from the 't7xx_mode' file will return 'ready'. And also we are 
going to introduce a 'normal' mode, that actually means 'hide this debug 
ports please'. While easy to introduce, looks like puzzle for a user.

I also would like to mention a potentially dangerous case. If a modem is 
already booted in the 'fastboot_download' mode, and someone writes 
'normal' into the 't7xx_mode' file. Will it switch the modem into a 
normal operational state. Also the activation itself lacks a couple of 
checks regarding a double port activation. Please see below.

How we can make this configuration process less puzzling? Should we 
rework the state machine more carefully or should we introduce a 
dedicated control file this purpose?

>   Read from userspace to get the current device mode.
>   
> @@ -139,6 +143,25 @@ Please note that driver needs to be reloaded to export /dev/wwan0fastboot0
>   port, because device needs a cold reset after enter ``fastboot_switching``
>   mode.
>   
> +ADB port userspace ABI
> +----------------------
> +
> +/dev/wwan0adb0 character device
> +~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
> +The driver exposes a ADB protocol interface by implementing ADB WWAN Port.
> +The userspace end of the ADB channel pipe is a /dev/wwan0adb0 character device.
> +Application shall use this interface for ADB protocol communication.
> +
> +MIPC port userspace ABI
> +-----------------------
> +
> +/dev/wwan0mipc0 character device
> +~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
> +The driver exposes a diagnostic interface by implementing MIPC (Modem
> +Information Process Center) WWAN Port. The userspace end of the MIPC channel
> +pipe is a /dev/wwan0mipc0 character device.
> +Application shall use this interface for MTK modem diagnostic communication.
> +
>   The MediaTek's T700 modem supports the 3GPP TS 27.007 [4] specification.
>   
>   References
> @@ -164,3 +187,9 @@ speak the Mobile Interface Broadband Model (MBIM) protocol"*
>   [5] *fastboot "a mechanism for communicating with bootloaders"*
>   
>   - https://android.googlesource.com/platform/system/core/+/refs/heads/main/fastboot/README.md
> +
> +[6] *ADB (Android Debug Bridge) "a mechanism to keep track of Android devices and
> +emulators instances connected to or running on a given host developer machine with
> +ADB protocol"*
> +
> +- https://android.googlesource.com/platform/packages/modules/adb/+/refs/heads/main/README.md
> diff --git a/drivers/net/wwan/t7xx/t7xx_pci.c b/drivers/net/wwan/t7xx/t7xx_pci.c
> index e0b1e7a616ca..6b18460d626c 100644
> --- a/drivers/net/wwan/t7xx/t7xx_pci.c
> +++ b/drivers/net/wwan/t7xx/t7xx_pci.c
> @@ -41,6 +41,7 @@
>   #include "t7xx_pcie_mac.h"
>   #include "t7xx_reg.h"
>   #include "t7xx_state_monitor.h"
> +#include "t7xx_port_proxy.h"
>   
>   #define T7XX_PCI_IREG_BASE		0
>   #define T7XX_PCI_EREG_BASE		2
> @@ -59,6 +60,8 @@ static const char * const t7xx_mode_names[] = {
>   	[T7XX_FASTBOOT_SWITCHING] = "fastboot_switching",
>   	[T7XX_FASTBOOT_DOWNLOAD] = "fastboot_download",
>   	[T7XX_FASTBOOT_DUMP] = "fastboot_dump",
> +	[T7XX_DEBUG] = "debug",
> +	[T7XX_NORMAL] = "normal",
>   };
>   
>   static_assert(ARRAY_SIZE(t7xx_mode_names) == T7XX_MODE_LAST);
> @@ -82,6 +85,10 @@ static ssize_t t7xx_mode_store(struct device *dev,
>   	} else if (index == T7XX_RESET) {
>   		WRITE_ONCE(t7xx_dev->mode, T7XX_RESET);
>   		t7xx_acpi_pldr_func(t7xx_dev);
> +	} else if (index == T7XX_DEBUG) {
> +		t7xx_proxy_port_debug(t7xx_dev, true);
> +	} else if (index == T7XX_NORMAL) {
> +		t7xx_proxy_port_debug(t7xx_dev, false);
>   	}
>   
>   	return count;
> diff --git a/drivers/net/wwan/t7xx/t7xx_pci.h b/drivers/net/wwan/t7xx/t7xx_pci.h
> index 49a11586d8d8..bdcadeb035e0 100644
> --- a/drivers/net/wwan/t7xx/t7xx_pci.h
> +++ b/drivers/net/wwan/t7xx/t7xx_pci.h
> @@ -50,6 +50,8 @@ enum t7xx_mode {
>   	T7XX_FASTBOOT_SWITCHING,
>   	T7XX_FASTBOOT_DOWNLOAD,
>   	T7XX_FASTBOOT_DUMP,
> +	T7XX_DEBUG,
> +	T7XX_NORMAL,
>   	T7XX_MODE_LAST, /* must always be last */
>   };
>   
> diff --git a/drivers/net/wwan/t7xx/t7xx_port.h b/drivers/net/wwan/t7xx/t7xx_port.h
> index f74d3bab810d..9f5d6d288c97 100644
> --- a/drivers/net/wwan/t7xx/t7xx_port.h
> +++ b/drivers/net/wwan/t7xx/t7xx_port.h
> @@ -42,6 +42,8 @@ enum port_ch {
>   	/* to AP */
>   	PORT_CH_AP_CONTROL_RX = 0x1000,
>   	PORT_CH_AP_CONTROL_TX = 0x1001,
> +	PORT_CH_AP_ADB_RX = 0x100a,
> +	PORT_CH_AP_ADB_TX = 0x100b,
>   
>   	/* to MD */
>   	PORT_CH_CONTROL_RX = 0x2000,
> @@ -100,6 +102,7 @@ struct t7xx_port_conf {
>   	struct port_ops		*ops;
>   	char			*name;
>   	enum wwan_port_type	port_type;
> +	bool			debug;
>   };
>   
>   struct t7xx_port {
> diff --git a/drivers/net/wwan/t7xx/t7xx_port_proxy.c b/drivers/net/wwan/t7xx/t7xx_port_proxy.c
> index 7d6388bf1d7c..3510f9013811 100644
> --- a/drivers/net/wwan/t7xx/t7xx_port_proxy.c
> +++ b/drivers/net/wwan/t7xx/t7xx_port_proxy.c
> @@ -39,6 +39,8 @@
>   
>   #define Q_IDX_CTRL			0
>   #define Q_IDX_MBIM			2
> +#define Q_IDX_MIPC			2
> +#define Q_IDX_ADB			3
>   #define Q_IDX_AT_CMD			5
>   
>   #define INVALID_SEQ_NUM			GENMASK(15, 0)
> @@ -100,7 +102,27 @@ static const struct t7xx_port_conf t7xx_port_conf[] = {
>   		.path_id = CLDMA_ID_AP,
>   		.ops = &ctl_port_ops,
>   		.name = "t7xx_ap_ctrl",
> -	},
> +	}, {
> +		.tx_ch = PORT_CH_AP_ADB_TX,
> +		.rx_ch = PORT_CH_AP_ADB_RX,
> +		.txq_index = Q_IDX_ADB,
> +		.rxq_index = Q_IDX_ADB,
> +		.path_id = CLDMA_ID_AP,
> +		.ops = &wwan_sub_port_ops,
> +		.name = "adb",
> +		.port_type = WWAN_PORT_ADB,
> +		.debug = true,
> +	}, {
> +		.tx_ch = PORT_CH_MIPC_TX,
> +		.rx_ch = PORT_CH_MIPC_RX,
> +		.txq_index = Q_IDX_MIPC,
> +		.rxq_index = Q_IDX_MIPC,
> +		.path_id = CLDMA_ID_MD,
> +		.ops = &wwan_sub_port_ops,
> +		.name = "mipc",
> +		.port_type = WWAN_PORT_MIPC,
> +		.debug = true,
> +	}
>   };
>   
>   static const struct t7xx_port_conf t7xx_early_port_conf[] = {
> @@ -505,13 +527,32 @@ static void t7xx_proxy_init_all_ports(struct t7xx_modem *md)
>   		spin_lock_init(&port->port_update_lock);
>   		port->chan_enable = false;
>   
> -		if (port_conf->ops && port_conf->ops->init)
> +		if (!port_conf->debug && port_conf->ops && port_conf->ops->init)
>   			port_conf->ops->init(port);
>   	}
>   
>   	t7xx_proxy_setup_ch_mapping(port_prox);
>   }
>   
> +void t7xx_proxy_port_debug(struct t7xx_pci_dev *t7xx_dev, bool show)
> +{
> +	struct port_proxy *port_prox = t7xx_dev->md->port_prox;
> +	struct t7xx_port *port;
> +	int i;
> +
> +	for_each_proxy_port(i, port, port_prox) {
> +		const struct t7xx_port_conf *port_conf = port->port_conf;
> +
> +		spin_lock_init(&port->port_update_lock);

This lock initialization does not seems correct. Should we reinitialize 
the lock on port hiding? And looks like the lock was already initialized 
in the t7xx_proxy_init_all_ports() function.

> +		if (port_conf->debug && port_conf->ops && port_conf->ops->init) {
> +			if (show)
> +				port_conf->ops->init(port);
> +			else
> +				port_conf->ops->uninit(port);

This part is also does not seems correct. Existing of the 'init' 
operation does not imply existing of 'uninit' operation. See 
t7xx_port_proxy_uninit() function.

Also t7xx_port_proxy_uninit() will call the uninitialization operation 
for us. Is it safe to call the uninitialization operation twice? Once to 
hide the ports and another one time on a driver unloading. Or what 
happens if someone will write 'normal' into 't7xx_mode' twice?

The same question is valid regarding the initialization (ports showing). 
If someone will write 'debug' into 't7xx_mode' twice, then we will 
register the same ADB port twice. Isn't it?

> +		}
> +	}
> +}
> +
>   void t7xx_port_proxy_set_cfg(struct t7xx_modem *md, enum port_cfg_id cfg_id)
>   {
>   	struct port_proxy *port_prox = md->port_prox;
> diff --git a/drivers/net/wwan/t7xx/t7xx_port_proxy.h b/drivers/net/wwan/t7xx/t7xx_port_proxy.h
> index 7f5706811445..a9c19c1253e6 100644
> --- a/drivers/net/wwan/t7xx/t7xx_port_proxy.h
> +++ b/drivers/net/wwan/t7xx/t7xx_port_proxy.h
> @@ -98,6 +98,7 @@ extern struct port_ops ctl_port_ops;
>   extern struct port_ops t7xx_trace_port_ops;
>   #endif
>   
> +void t7xx_proxy_port_debug(struct t7xx_pci_dev *t7xx_dev, bool show);
>   void t7xx_port_proxy_reset(struct port_proxy *port_prox);
>   void t7xx_port_proxy_uninit(struct port_proxy *port_prox);
>   int t7xx_port_proxy_init(struct t7xx_modem *md);
> diff --git a/drivers/net/wwan/t7xx/t7xx_port_wwan.c b/drivers/net/wwan/t7xx/t7xx_port_wwan.c
> index 4b23ba693f3f..7fc569565ff9 100644
> --- a/drivers/net/wwan/t7xx/t7xx_port_wwan.c
> +++ b/drivers/net/wwan/t7xx/t7xx_port_wwan.c
> @@ -169,7 +169,9 @@ static int t7xx_port_wwan_init(struct t7xx_port *port)
>   {
>   	const struct t7xx_port_conf *port_conf = port->port_conf;
>   
> -	if (port_conf->port_type == WWAN_PORT_FASTBOOT)
> +	if (port_conf->port_type == WWAN_PORT_FASTBOOT ||
> +	    port_conf->port_type == WWAN_PORT_ADB ||
> +	    port_conf->port_type == WWAN_PORT_MIPC)
>   		t7xx_port_wwan_create(port);
>   
>   	port->rx_length_th = RX_QUEUE_MAXLEN;
> @@ -224,7 +226,9 @@ static void t7xx_port_wwan_md_state_notify(struct t7xx_port *port, unsigned int
>   {
>   	const struct t7xx_port_conf *port_conf = port->port_conf;
>   
> -	if (port_conf->port_type == WWAN_PORT_FASTBOOT)
> +	if (port_conf->port_type == WWAN_PORT_FASTBOOT ||
> +	    port_conf->port_type == WWAN_PORT_ADB ||
> +	    port_conf->port_type == WWAN_PORT_MIPC)
>   		return;
>   
>   	if (state != MD_STATE_READY)

Re: [net-next v2 2/2] net: wwan: t7xx: Add debug port

[Jinjian Song]

On 25.06.2024 11:45, Jinjian Song wrote:
>> --- a/Documentation/networking/device_drivers/wwan/t7xx.rst
>> +++ b/Documentation/networking/device_drivers/wwan/t7xx.rst
>> @@ -56,6 +56,10 @@ Device mode:
>>   - ``fastboot_switching`` represents that device in fastboot switching status
>>   - ``fastboot_download`` represents that device in fastboot download status
>>   - ``fastboot_dump`` represents that device in fastboot dump status
>> +- ``debug`` represents switching on debug ports (write only)
>> +- ``normal`` represents switching off debug ports (write only)
>> +
>> +Currently supported debug ports (ADB/MIPC)
>
>Could you clarify a bit the availability of these debug ports (ADB and 
>MIPC)? Looks like these ports are always available when a modem is 
>booted in a 'normal' mode. They are available among other types of ports 
>like AT, MBIM, etc. And you want to hide them, or speaking more 
>precisely, you want to make debug ports availability configurable. Do I 
>understand it right?

Yes, your understanding is basically corret.
1. ADB port, When we want to pull/push some files from modem or run shell command
on modem, we usually use the adb command.
e.g., "pcie-adb shell", then commands we input will run in modem shell terminal, if
we input shell command like "ls,pwd" it feedback the result on modem side.
or "pcie-adb pull xxx.file" to get the xxx.file from modem file system.
So if we want to get some file(config or dump) from modem or change some file on 
modem, it will work.

MIPC port, When we optimize modem antenna tunner or noise profiling, we will use
this port to set parameters through this MTK diagnostic interface, the command
through this MIPC port defined by MTK.

2.All the port channel configuration is determined, so if modem open the port
channel, driver can communicate with modem. It means if modem configure 
AT/MBIM/ADB/MIPC channel open, then when modem booting up with them.
I want to hidden ADB and MIPC port, because the ports channel is off default
by modem. If needed when debugging, we can open them.

>I just have doubts regarding the chosen configuration approach. We 
>already have a 'ready' mode indicating a normal operation. Now we are 
>introducing a 'debug' mode that only makes available ADB/MIPC ports, but 
>reading from the 't7xx_mode' file will return 'ready'. And also we are 
>going to introduce a 'normal' mode, that actually means 'hide this debug 
>ports please'. While easy to introduce, looks like puzzle for a user.

Yes, it looks like puzzle for a user, 't7xx_mode' should be the running
state machine of modem, I want to express the port status by 'debug' and
'normal' of t7xx driver.

>I also would like to mention a potentially dangerous case. If a modem is 
>already booted in the 'fastboot_download' mode, and someone writes 
>'normal' into the 't7xx_mode' file. Will it switch the modem into a 
>normal operational state. Also the activation itself lacks a couple of 
>checks regarding a double port activation. Please see below.
>
>How we can make this configuration process less puzzling? Should we 
>rework the state machine more carefully or should we introduce a 
>dedicated control file this purpose?

'debug' and 'normal' only effect the ports which configurate the attribute,
'.debug = true', when modem in 'fastboot_download', user set 'normal', then
driver will find the ports proxy configuration which '.debug' is true and 
call the port->unint to release the port. 'fastboot_download' has no 
attribute '.debug = true', so no oprate will occured.

How about create a new sysfs node named 't7xx_port' to control the ports
state in t7xx driver? 'debug' or 'normal'.


>   
> @@ -100,7 +102,27 @@ static const struct t7xx_port_conf t7xx_port_conf[] = {
>   		.path_id = CLDMA_ID_AP,
>   		.ops = &ctl_port_ops,
>   		.name = "t7xx_ap_ctrl",
> -	},
> +	}, {
> +		.tx_ch = PORT_CH_AP_ADB_TX,
> +		.rx_ch = PORT_CH_AP_ADB_RX,
> +		.txq_index = Q_IDX_ADB,
> +		.rxq_index = Q_IDX_ADB,
> +		.path_id = CLDMA_ID_AP,
> +		.ops = &wwan_sub_port_ops,
> +		.name = "adb",
> +		.port_type = WWAN_PORT_ADB,
> +		.debug = true,
> +	}, {
> +		.tx_ch = PORT_CH_MIPC_TX,
> +		.rx_ch = PORT_CH_MIPC_RX,
> +		.txq_index = Q_IDX_MIPC,
> +		.rxq_index = Q_IDX_MIPC,
> +		.path_id = CLDMA_ID_MD,
> +		.ops = &wwan_sub_port_ops,
> +		.name = "mipc",
> +		.port_type = WWAN_PORT_MIPC,
> +		.debug = true,
> +	}
>   };
>   
...

>> +void t7xx_proxy_port_debug(struct t7xx_pci_dev *t7xx_dev, bool show)
>> +{
>> +	struct port_proxy *port_prox = t7xx_dev->md->port_prox;
>> +	struct t7xx_port *port;
>> +	int i;
>> +
>> +	for_each_proxy_port(i, port, port_prox) {
>> +		const struct t7xx_port_conf *port_conf = port->port_conf;
>> +
>> +		spin_lock_init(&port->port_update_lock);
>
>This lock initialization does not seems correct. Should we reinitialize 
>the lock on port hiding? And looks like the lock was already initialized 

Yes, it should't reinitalize, let me fix it.

>> +		if (port_conf->debug && port_conf->ops && port_conf->ops->init) {
>> +			if (show)
>> +				port_conf->ops->init(port);
>> +			else
>> +				port_conf->ops->uninit(port);
>
>This part is also does not seems correct. Existing of the 'init' 
>operation does not imply existing of 'uninit' operation. See 
>t7xx_port_proxy_uninit() function.
>
>Also t7xx_port_proxy_uninit() will call the uninitialization operation 
>for us. Is it safe to call the uninitialization operation twice? Once to 
>hide the ports and another one time on a driver unloading. Or what 
>happens if someone will write 'normal' into 't7xx_mode' twice?
>
>The same question is valid regarding the initialization (ports showing). 
>If someone will write 'debug' into 't7xx_mode' twice, then we will 
>register the same ADB port twice. Isn't it?

init() and uninit() has check the pointer of the port, if porinter is NULL, there
will no more operation, so it will not call then twice.

Code 'if (!port->wwan.wwan_port)' will check if the port has inited.

static void t7xx_port_wwan_create(struct t7xx_port *port)
{
	const struct t7xx_port_conf *port_conf = port->port_conf;
	unsigned int header_len = sizeof(struct ccci_header), mtu;
	struct wwan_port_caps caps;

	if (!port->wwan.wwan_port) {
		mtu = t7xx_get_port_mtu(port);
		caps.frag_len = mtu - header_len;
		caps.headroom_len = header_len;
		port->wwan.wwan_port = wwan_create_port(port->dev, port_conf->port_type,
							&wwan_ops, &caps, port);
		if (IS_ERR(port->wwan.wwan_port))
			dev_err(port->dev, "Unable to create WWAN port %s", port_conf->name);
	}
}

static int t7xx_port_wwan_init(struct t7xx_port *port)
{
	const struct t7xx_port_conf *port_conf = port->port_conf;

	if (port_conf->port_type == WWAN_PORT_FASTBOOT)
		t7xx_port_wwan_create(port);

	port->rx_length_th = RX_QUEUE_MAXLEN;
	return 0;
}

Code 'if (!port->wwan.wwan_port)' will check whether port has uninited.

static void t7xx_port_wwan_uninit(struct t7xx_port *port)
{
	if (!port->wwan.wwan_port)
		return;

	port->rx_length_th = 0;
	wwan_remove_port(port->wwan.wwan_port);
	port->wwan.wwan_port = NULL;
}


Thanks.

Jinjian,
Best Regards.