[PATCH net-next 1/1] net: wwan: t7xx: Add port for modem logging

[PATCH net-next 1/1] net: wwan: t7xx: Add port for modem logging

[Moises Veleta]

The Modem Logging (MDL) port provides an interface to collect modem
logs for debugging purposes. MDL is supported by debugfs, the relay
interface, and the mtk_t7xx port infrastructure. MDL allows user-space
applications to control logging via debugfs and to collect logs via
the relay interface, while port infrastructure facilitates
communication between the driver and the modem.

Signed-off-by: Moises Veleta <moises.veleta@linux.intel.com>
Acked-by: Ricardo Martinez <ricardo.martinez@linux.intel.com>
---
 drivers/net/wwan/Kconfig                |   1 +
 drivers/net/wwan/t7xx/Makefile          |   3 +
 drivers/net/wwan/t7xx/t7xx_hif_cldma.c  |   2 +
 drivers/net/wwan/t7xx/t7xx_port.h       |   5 +
 drivers/net/wwan/t7xx/t7xx_port_proxy.c |  22 +++
 drivers/net/wwan/t7xx/t7xx_port_proxy.h |   4 +
 drivers/net/wwan/t7xx/t7xx_port_trace.c | 174 ++++++++++++++++++++++++
 7 files changed, 211 insertions(+)
 create mode 100644 drivers/net/wwan/t7xx/t7xx_port_trace.c

diff --git a/drivers/net/wwan/Kconfig b/drivers/net/wwan/Kconfig
index 3486ffe94ac4..32149029c891 100644
--- a/drivers/net/wwan/Kconfig
+++ b/drivers/net/wwan/Kconfig
@@ -108,6 +108,7 @@ config IOSM
 config MTK_T7XX
 	tristate "MediaTek PCIe 5G WWAN modem T7xx device"
 	depends on PCI
+	select RELAY if WWAN_DEBUGFS
 	help
 	  Enables MediaTek PCIe based 5G WWAN modem (T7xx series) device.
 	  Adapts WWAN framework and provides network interface like wwan0
diff --git a/drivers/net/wwan/t7xx/Makefile b/drivers/net/wwan/t7xx/Makefile
index dc6a7d682c15..268ff9e87e5b 100644
--- a/drivers/net/wwan/t7xx/Makefile
+++ b/drivers/net/wwan/t7xx/Makefile
@@ -18,3 +18,6 @@ mtk_t7xx-y:=	t7xx_pci.o \
 		t7xx_hif_dpmaif_rx.o  \
 		t7xx_dpmaif.o \
 		t7xx_netdev.o
+
+mtk_t7xx-$(CONFIG_WWAN_DEBUGFS) += \
+		t7xx_port_trace.o \
diff --git a/drivers/net/wwan/t7xx/t7xx_hif_cldma.c b/drivers/net/wwan/t7xx/t7xx_hif_cldma.c
index 0c52801ed0de..dcd480720edf 100644
--- a/drivers/net/wwan/t7xx/t7xx_hif_cldma.c
+++ b/drivers/net/wwan/t7xx/t7xx_hif_cldma.c
@@ -1018,6 +1018,8 @@ static int t7xx_cldma_late_init(struct cldma_ctrl *md_ctrl)
 			dev_err(md_ctrl->dev, "control TX ring init fail\n");
 			goto err_free_tx_ring;
 		}
+
+		md_ctrl->tx_ring[i].pkt_size = CLDMA_MTU;
 	}
 
 	for (j = 0; j < CLDMA_RXQ_NUM; j++) {
diff --git a/drivers/net/wwan/t7xx/t7xx_port.h b/drivers/net/wwan/t7xx/t7xx_port.h
index dc4133eb433a..e35efb18ea09 100644
--- a/drivers/net/wwan/t7xx/t7xx_port.h
+++ b/drivers/net/wwan/t7xx/t7xx_port.h
@@ -122,10 +122,15 @@ struct t7xx_port {
 	int				rx_length_th;
 	bool				chan_enable;
 	struct task_struct		*thread;
+#ifdef CONFIG_WWAN_DEBUGFS
+	struct t7xx_trace		*trace;
+	struct dentry			*debugfs_dir;
+#endif
 };
 
 struct sk_buff *t7xx_port_alloc_skb(int payload);
 struct sk_buff *t7xx_ctrl_alloc_skb(int payload);
+int t7xx_port_mtu(struct t7xx_port *port);
 int t7xx_port_enqueue_skb(struct t7xx_port *port, struct sk_buff *skb);
 int t7xx_port_send_skb(struct t7xx_port *port, struct sk_buff *skb, unsigned int pkt_header,
 		       unsigned int ex_msg);
diff --git a/drivers/net/wwan/t7xx/t7xx_port_proxy.c b/drivers/net/wwan/t7xx/t7xx_port_proxy.c
index 7d2c0e81e33d..fb9d057d6a84 100644
--- a/drivers/net/wwan/t7xx/t7xx_port_proxy.c
+++ b/drivers/net/wwan/t7xx/t7xx_port_proxy.c
@@ -70,6 +70,18 @@ static const struct t7xx_port_conf t7xx_md_port_conf[] = {
 		.name = "MBIM",
 		.port_type = WWAN_PORT_MBIM,
 	}, {
+#ifdef CONFIG_WWAN_DEBUGFS
+		.tx_ch = PORT_CH_MD_LOG_TX,
+		.rx_ch = PORT_CH_MD_LOG_RX,
+		.txq_index = 7,
+		.rxq_index = 7,
+		.txq_exp_index = 7,
+		.rxq_exp_index = 7,
+		.path_id = CLDMA_ID_MD,
+		.ops = &t7xx_trace_port_ops,
+		.name = "mdlog",
+	}, {
+#endif
 		.tx_ch = PORT_CH_CONTROL_TX,
 		.rx_ch = PORT_CH_CONTROL_RX,
 		.txq_index = Q_IDX_CTRL,
@@ -194,6 +206,16 @@ int t7xx_port_enqueue_skb(struct t7xx_port *port, struct sk_buff *skb)
 	return 0;
 }
 
+int t7xx_port_mtu(struct t7xx_port *port)
+{
+	enum cldma_id path_id = port->port_conf->path_id;
+	int tx_qno = t7xx_port_get_queue_no(port);
+	struct cldma_ctrl *md_ctrl;
+
+	md_ctrl = port->t7xx_dev->md->md_ctrl[path_id];
+	return md_ctrl->tx_ring[tx_qno].pkt_size;
+}
+
 static int t7xx_port_send_raw_skb(struct t7xx_port *port, struct sk_buff *skb)
 {
 	enum cldma_id path_id = port->port_conf->path_id;
diff --git a/drivers/net/wwan/t7xx/t7xx_port_proxy.h b/drivers/net/wwan/t7xx/t7xx_port_proxy.h
index bc1ff5c6c700..81d059fbc0fb 100644
--- a/drivers/net/wwan/t7xx/t7xx_port_proxy.h
+++ b/drivers/net/wwan/t7xx/t7xx_port_proxy.h
@@ -87,6 +87,10 @@ struct ctrl_msg_header {
 extern struct port_ops wwan_sub_port_ops;
 extern struct port_ops ctl_port_ops;
 
+#ifdef CONFIG_WWAN_DEBUGFS
+extern struct port_ops t7xx_trace_port_ops;
+#endif
+
 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_trace.c b/drivers/net/wwan/t7xx/t7xx_port_trace.c
new file mode 100644
index 000000000000..87529316b183
--- /dev/null
+++ b/drivers/net/wwan/t7xx/t7xx_port_trace.c
@@ -0,0 +1,174 @@
+// SPDX-License-Identifier: GPL-2.0-only
+/*
+ * Copyright (C) 2022 Intel Corporation.
+ */
+
+#include <linux/bitfield.h>
+#include <linux/debugfs.h>
+#include <linux/relay.h>
+#include <linux/skbuff.h>
+#include <linux/wwan.h>
+
+#include "t7xx_port.h"
+#include "t7xx_port_proxy.h"
+#include "t7xx_state_monitor.h"
+
+#define T7XX_TRC_SUB_BUFF_SIZE		131072
+#define T7XX_TRC_N_SUB_BUFF		32
+#define T7XX_TRC_FILE_PERM		0600
+
+struct t7xx_trace {
+	struct rchan			*t7xx_rchan;
+	struct dentry			*ctrl_file;
+};
+
+static struct dentry *t7xx_trace_create_buf_file_handler(const char *filename,
+							 struct dentry *parent,
+							 umode_t mode,
+							 struct rchan_buf *buf,
+							 int *is_global)
+{
+	*is_global = 1;
+	return debugfs_create_file(filename, mode, parent, buf,
+				   &relay_file_operations);
+}
+
+static int t7xx_trace_remove_buf_file_handler(struct dentry *dentry)
+{
+	debugfs_remove(dentry);
+	return 0;
+}
+
+static int t7xx_trace_subbuf_start_handler(struct rchan_buf *buf, void *subbuf,
+					   void *prev_subbuf,
+					   size_t prev_padding)
+{
+	if (relay_buf_full(buf)) {
+		pr_err_ratelimited("Relay_buf full dropping traces");
+		return 0;
+	}
+
+	return 1;
+}
+
+static struct rchan_callbacks relay_callbacks = {
+	.subbuf_start = t7xx_trace_subbuf_start_handler,
+	.create_buf_file = t7xx_trace_create_buf_file_handler,
+	.remove_buf_file = t7xx_trace_remove_buf_file_handler,
+};
+
+static ssize_t t7xx_port_trace_write(struct file *file, const char __user *buf,
+				     size_t len, loff_t *ppos)
+{
+	struct t7xx_port *port = file->private_data;
+	size_t actual_len, alloc_size, txq_mtu;
+	const struct t7xx_port_conf *port_conf;
+	enum md_state md_state;
+	struct sk_buff *skb;
+	int ret;
+
+	port_conf = port->port_conf;
+	md_state = t7xx_fsm_get_md_state(port->t7xx_dev->md->fsm_ctl);
+	if (md_state == MD_STATE_WAITING_FOR_HS1 || md_state == MD_STATE_WAITING_FOR_HS2) {
+		dev_warn(port->dev, "port: %s ch: %d, write fail when md_state: %d\n",
+			 port_conf->name, port_conf->tx_ch, md_state);
+		return -ENODEV;
+	}
+
+	txq_mtu = t7xx_port_mtu(port);
+	alloc_size = min_t(size_t, txq_mtu, len + sizeof(struct ccci_header));
+	actual_len = alloc_size - sizeof(struct ccci_header);
+	skb = t7xx_port_alloc_skb(alloc_size);
+	if (!skb) {
+		ret = -ENOMEM;
+		goto err_out;
+	}
+
+	ret = copy_from_user(skb_put(skb, actual_len), buf, actual_len);
+	if (ret) {
+		ret = -EFAULT;
+		goto err_out;
+	}
+
+	ret = t7xx_port_send_skb(port, skb, 0, 0);
+	if (ret)
+		goto err_out;
+
+	return actual_len;
+
+err_out:
+	dev_err(port->dev, "write error done on %s, size: %zu, ret: %d\n",
+		port_conf->name, actual_len, ret);
+	dev_kfree_skb(skb);
+	return ret;
+}
+
+static const struct file_operations t7xx_trace_fops = {
+	.owner = THIS_MODULE,
+	.open = simple_open,
+	.write = t7xx_port_trace_write,
+};
+
+static int t7xx_trace_port_init(struct t7xx_port *port)
+{
+	struct dentry *debugfs_pdev = wwan_get_debugfs_dir(port->dev);
+
+	if (IS_ERR(debugfs_pdev))
+		debugfs_pdev = NULL;
+
+	port->debugfs_dir = debugfs_create_dir(KBUILD_MODNAME, debugfs_pdev);
+	if (IS_ERR_OR_NULL(port->debugfs_dir))
+		return -ENOMEM;
+
+	port->trace = devm_kzalloc(port->dev, sizeof(*port->trace), GFP_KERNEL);
+	if (!port->trace)
+		goto err_debugfs_dir;
+
+	port->trace->ctrl_file = debugfs_create_file("mdlog_ctrl",
+						     T7XX_TRC_FILE_PERM,
+						     port->debugfs_dir,
+						     port,
+						     &t7xx_trace_fops);
+	if (!port->trace->ctrl_file)
+		goto err_debugfs_dir;
+
+	port->trace->t7xx_rchan = relay_open("relay_ch",
+					     port->debugfs_dir,
+					     T7XX_TRC_SUB_BUFF_SIZE,
+					     T7XX_TRC_N_SUB_BUFF,
+					     &relay_callbacks, NULL);
+	if (!port->trace->t7xx_rchan)
+		goto err_debugfs_dir;
+
+	return 0;
+
+err_debugfs_dir:
+	debugfs_remove_recursive(port->debugfs_dir);
+	return -ENOMEM;
+}
+
+static void t7xx_trace_port_uninit(struct t7xx_port *port)
+{
+	struct t7xx_trace *trace = port->trace;
+
+	relay_close(trace->t7xx_rchan);
+	debugfs_remove_recursive(port->debugfs_dir);
+}
+
+static int t7xx_trace_port_recv_skb(struct t7xx_port *port, struct sk_buff *skb)
+{
+	struct t7xx_trace *t7xx_trace = port->trace;
+
+	if (!t7xx_trace->t7xx_rchan)
+		return -EINVAL;
+
+	relay_write(t7xx_trace->t7xx_rchan, skb->data, skb->len);
+	dev_kfree_skb(skb);
+	return 0;
+}
+
+struct port_ops t7xx_trace_port_ops = {
+	.init =	t7xx_trace_port_init,
+	.recv_skb = t7xx_trace_port_recv_skb,
+	.uninit = t7xx_trace_port_uninit,
+};
-- 
2.17.1

RE: [PATCH net-next 1/1] net: wwan: t7xx: Add port for modem logging

[Kumar, M Chetan]

> -----Original Message-----
> From: Moises Veleta <moises.veleta@linux.intel.com>
> Sent: Thursday, May 19, 2022 11:57 PM
> To: netdev@vger.kernel.org
> Cc: kuba@kernel.org; davem@davemloft.net; johannes@sipsolutions.net;
> ryazanov.s.a@gmail.com; loic.poulain@linaro.org; Kumar, M Chetan
> <m.chetan.kumar@intel.com>; Devegowda, Chandrashekar
> <chandrashekar.devegowda@intel.com>; linuxwwan
> <linuxwwan@intel.com>; Liu, Haijun <haijun.liu@mediatek.com>;
> andriy.shevchenko@linux.intel.com; ilpo.jarvinen@linux.intel.com;
> ricardo.martinez@linux.intel.com; Kancharla, Sreehari
> <sreehari.kancharla@intel.com>; Sharma, Dinesh
> <dinesh.sharma@intel.com>; Moises Veleta
> <moises.veleta@linux.intel.com>
> Subject: [PATCH net-next 1/1] net: wwan: t7xx: Add port for modem logging
> 
> The Modem Logging (MDL) port provides an interface to collect modem logs
> for debugging purposes. MDL is supported by debugfs, the relay interface,
> and the mtk_t7xx port infrastructure. MDL allows user-space applications to
> control logging via debugfs and to collect logs via the relay interface, while
> port infrastructure facilitates communication between the driver and the
> modem.
> 
> Signed-off-by: Moises Veleta <moises.veleta@linux.intel.com>
> Acked-by: Ricardo Martinez <ricardo.martinez@linux.intel.com>
> ---
>  drivers/net/wwan/Kconfig                |   1 +
>  drivers/net/wwan/t7xx/Makefile          |   3 +
>  drivers/net/wwan/t7xx/t7xx_hif_cldma.c  |   2 +
>  drivers/net/wwan/t7xx/t7xx_port.h       |   5 +
>  drivers/net/wwan/t7xx/t7xx_port_proxy.c |  22 +++
>  drivers/net/wwan/t7xx/t7xx_port_proxy.h |   4 +
>  drivers/net/wwan/t7xx/t7xx_port_trace.c | 174
> ++++++++++++++++++++++++
>  7 files changed, 211 insertions(+)
>  create mode 100644 drivers/net/wwan/t7xx/t7xx_port_trace.c
> 
> diff --git a/drivers/net/wwan/Kconfig b/drivers/net/wwan/Kconfig index
> 3486ffe94ac4..32149029c891 100644
> --- a/drivers/net/wwan/Kconfig
> +++ b/drivers/net/wwan/Kconfig
> @@ -108,6 +108,7 @@ config IOSM
>  config MTK_T7XX
>  	tristate "MediaTek PCIe 5G WWAN modem T7xx device"
>  	depends on PCI
> +	select RELAY if WWAN_DEBUGFS
>  	help
>  	  Enables MediaTek PCIe based 5G WWAN modem (T7xx series)
> device.
>  	  Adapts WWAN framework and provides network interface like
> wwan0 diff --git a/drivers/net/wwan/t7xx/Makefile
> b/drivers/net/wwan/t7xx/Makefile index dc6a7d682c15..268ff9e87e5b
> 100644
> --- a/drivers/net/wwan/t7xx/Makefile
> +++ b/drivers/net/wwan/t7xx/Makefile
> @@ -18,3 +18,6 @@ mtk_t7xx-y:=	t7xx_pci.o \
>  		t7xx_hif_dpmaif_rx.o  \
>  		t7xx_dpmaif.o \
>  		t7xx_netdev.o
> +
> +mtk_t7xx-$(CONFIG_WWAN_DEBUGFS) += \
> +		t7xx_port_trace.o \

Drop \

> diff --git a/drivers/net/wwan/t7xx/t7xx_hif_cldma.c
> b/drivers/net/wwan/t7xx/t7xx_hif_cldma.c
> index 0c52801ed0de..dcd480720edf 100644
> --- a/drivers/net/wwan/t7xx/t7xx_hif_cldma.c
> +++ b/drivers/net/wwan/t7xx/t7xx_hif_cldma.c
> @@ -1018,6 +1018,8 @@ static int t7xx_cldma_late_init(struct cldma_ctrl
> *md_ctrl)
>  			dev_err(md_ctrl->dev, "control TX ring init fail\n");
>  			goto err_free_tx_ring;
>  		}
> +
> +		md_ctrl->tx_ring[i].pkt_size = CLDMA_MTU;
>  	}
> 
>  	for (j = 0; j < CLDMA_RXQ_NUM; j++) {
> diff --git a/drivers/net/wwan/t7xx/t7xx_port.h
> b/drivers/net/wwan/t7xx/t7xx_port.h
> index dc4133eb433a..e35efb18ea09 100644
> --- a/drivers/net/wwan/t7xx/t7xx_port.h
> +++ b/drivers/net/wwan/t7xx/t7xx_port.h
> @@ -122,10 +122,15 @@ struct t7xx_port {
>  	int				rx_length_th;
>  	bool				chan_enable;
>  	struct task_struct		*thread;
> +#ifdef CONFIG_WWAN_DEBUGFS
> +	struct t7xx_trace		*trace;
> +	struct dentry			*debugfs_dir;
> +#endif
>  };
> 
>  struct sk_buff *t7xx_port_alloc_skb(int payload);  struct sk_buff
> *t7xx_ctrl_alloc_skb(int payload);
> +int t7xx_port_mtu(struct t7xx_port *port);
>  int t7xx_port_enqueue_skb(struct t7xx_port *port, struct sk_buff *skb);  int
> t7xx_port_send_skb(struct t7xx_port *port, struct sk_buff *skb, unsigned int
> pkt_header,
>  		       unsigned int ex_msg);
> diff --git a/drivers/net/wwan/t7xx/t7xx_port_proxy.c
> b/drivers/net/wwan/t7xx/t7xx_port_proxy.c
> index 7d2c0e81e33d..fb9d057d6a84 100644
> --- a/drivers/net/wwan/t7xx/t7xx_port_proxy.c
> +++ b/drivers/net/wwan/t7xx/t7xx_port_proxy.c
> @@ -70,6 +70,18 @@ static const struct t7xx_port_conf
> t7xx_md_port_conf[] = {
>  		.name = "MBIM",
>  		.port_type = WWAN_PORT_MBIM,
>  	}, {
> +#ifdef CONFIG_WWAN_DEBUGFS
> +		.tx_ch = PORT_CH_MD_LOG_TX,
> +		.rx_ch = PORT_CH_MD_LOG_RX,
> +		.txq_index = 7,
> +		.rxq_index = 7,
> +		.txq_exp_index = 7,
> +		.rxq_exp_index = 7,
> +		.path_id = CLDMA_ID_MD,
> +		.ops = &t7xx_trace_port_ops,
> +		.name = "mdlog",
> +	}, {
> +#endif

Why do you want keep mdlog port under flag ?

>  		.tx_ch = PORT_CH_CONTROL_TX,
>  		.rx_ch = PORT_CH_CONTROL_RX,
>  		.txq_index = Q_IDX_CTRL,
> @@ -194,6 +206,16 @@ int t7xx_port_enqueue_skb(struct t7xx_port *port,
> struct sk_buff *skb)
>  	return 0;
>  }
> 
> +int t7xx_port_mtu(struct t7xx_port *port) {
> +	enum cldma_id path_id = port->port_conf->path_id;
> +	int tx_qno = t7xx_port_get_queue_no(port);
> +	struct cldma_ctrl *md_ctrl;
> +
> +	md_ctrl = port->t7xx_dev->md->md_ctrl[path_id];
> +	return md_ctrl->tx_ring[tx_qno].pkt_size;
> +}
> +
>  static int t7xx_port_send_raw_skb(struct t7xx_port *port, struct sk_buff
> *skb)  {
>  	enum cldma_id path_id = port->port_conf->path_id; diff --git
> a/drivers/net/wwan/t7xx/t7xx_port_proxy.h
> b/drivers/net/wwan/t7xx/t7xx_port_proxy.h
> index bc1ff5c6c700..81d059fbc0fb 100644
> --- a/drivers/net/wwan/t7xx/t7xx_port_proxy.h
> +++ b/drivers/net/wwan/t7xx/t7xx_port_proxy.h
> @@ -87,6 +87,10 @@ struct ctrl_msg_header {  extern struct port_ops
> wwan_sub_port_ops;  extern struct port_ops ctl_port_ops;
> 
> +#ifdef CONFIG_WWAN_DEBUGFS
> +extern struct port_ops t7xx_trace_port_ops; #endif
> +
>  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_trace.c
> b/drivers/net/wwan/t7xx/t7xx_port_trace.c
> new file mode 100644
> index 000000000000..87529316b183
> --- /dev/null
> +++ b/drivers/net/wwan/t7xx/t7xx_port_trace.c
> @@ -0,0 +1,174 @@
> +// SPDX-License-Identifier: GPL-2.0-only
> +/*
> + * Copyright (C) 2022 Intel Corporation.
> + */
> +
> +#include <linux/bitfield.h>
> +#include <linux/debugfs.h>
> +#include <linux/relay.h>
> +#include <linux/skbuff.h>
> +#include <linux/wwan.h>
> +
> +#include "t7xx_port.h"
> +#include "t7xx_port_proxy.h"
> +#include "t7xx_state_monitor.h"
> +
> +#define T7XX_TRC_SUB_BUFF_SIZE		131072
> +#define T7XX_TRC_N_SUB_BUFF		32
> +#define T7XX_TRC_FILE_PERM		0600
> +
> +struct t7xx_trace {
> +	struct rchan			*t7xx_rchan;
> +	struct dentry			*ctrl_file;
> +};
> +
> +static struct dentry *t7xx_trace_create_buf_file_handler(const char
> *filename,
> +							 struct dentry
> *parent,
> +							 umode_t mode,
> +							 struct rchan_buf
> *buf,
> +							 int *is_global)
> +{
> +	*is_global = 1;
> +	return debugfs_create_file(filename, mode, parent, buf,
> +				   &relay_file_operations);
> +}
> +
> +static int t7xx_trace_remove_buf_file_handler(struct dentry *dentry) {
> +	debugfs_remove(dentry);
> +	return 0;
> +}
> +
> +static int t7xx_trace_subbuf_start_handler(struct rchan_buf *buf, void
> *subbuf,
> +					   void *prev_subbuf,
> +					   size_t prev_padding)
> +{
> +	if (relay_buf_full(buf)) {
> +		pr_err_ratelimited("Relay_buf full dropping traces");
> +		return 0;
> +	}
> +
> +	return 1;
> +}
> +
> +static struct rchan_callbacks relay_callbacks = {
> +	.subbuf_start = t7xx_trace_subbuf_start_handler,
> +	.create_buf_file = t7xx_trace_create_buf_file_handler,
> +	.remove_buf_file = t7xx_trace_remove_buf_file_handler,
> +};
> +
> +static ssize_t t7xx_port_trace_write(struct file *file, const char __user *buf,
> +				     size_t len, loff_t *ppos)
> +{
> +	struct t7xx_port *port = file->private_data;
> +	size_t actual_len, alloc_size, txq_mtu;
> +	const struct t7xx_port_conf *port_conf;
> +	enum md_state md_state;
> +	struct sk_buff *skb;
> +	int ret;
> +
> +	port_conf = port->port_conf;
> +	md_state = t7xx_fsm_get_md_state(port->t7xx_dev->md->fsm_ctl);
> +	if (md_state == MD_STATE_WAITING_FOR_HS1 || md_state ==
> MD_STATE_WAITING_FOR_HS2) {
> +		dev_warn(port->dev, "port: %s ch: %d, write fail when
> md_state: %d\n",
> +			 port_conf->name, port_conf->tx_ch, md_state);
> +		return -ENODEV;
> +	}

This means debugfs knob is available to application even before driver & device handshake
 is complete. Is not possible to defer debugfs knob creation until handshake is complete ?

> +
> +	txq_mtu = t7xx_port_mtu(port);
> +	alloc_size = min_t(size_t, txq_mtu, len + sizeof(struct ccci_header));

To keep it even we can drop +sizeof(struct ccci_header).

> +	actual_len = alloc_size - sizeof(struct ccci_header);
> +	skb = t7xx_port_alloc_skb(alloc_size);

alloc_size contains the actual len and t7xx_port_alloc_skb() is considering alloc_size + 
sizeof(struct ccci_header); So actual_len calculation is redundant.

> +	if (!skb) {
> +		ret = -ENOMEM;
> +		goto err_out;

In skb failure case an attempt is made to free skb() by calling dev_kfree_skb().
Better to add new label and simply return ?

> +	}
> +
> +	ret = copy_from_user(skb_put(skb, actual_len), buf, actual_len);
> +	if (ret) {
> +		ret = -EFAULT;
> +		goto err_out;
> +	}
> +
> +	ret = t7xx_port_send_skb(port, skb, 0, 0);
> +	if (ret)
> +		goto err_out;
> +
> +	return actual_len;

If len report is txq_mtu then actual_len is returning - sizeof(struct ccci_header);
Instead return len.

> +
> +err_out:
> +	dev_err(port->dev, "write error done on %s, size: %zu, ret: %d\n",
> +		port_conf->name, actual_len, ret);
> +	dev_kfree_skb(skb);
> +	return ret;
> +}
> +

> +static const struct file_operations t7xx_trace_fops = {
> +	.owner = THIS_MODULE,
> +	.open = simple_open,
> +	.write = t7xx_port_trace_write,
> +};
> +
> +static int t7xx_trace_port_init(struct t7xx_port *port) {
> +	struct dentry *debugfs_pdev = wwan_get_debugfs_dir(port->dev);
> +
> +	if (IS_ERR(debugfs_pdev))
> +		debugfs_pdev = NULL;
> +
> +	port->debugfs_dir = debugfs_create_dir(KBUILD_MODNAME,
> debugfs_pdev);
> +	if (IS_ERR_OR_NULL(port->debugfs_dir))
> +		return -ENOMEM;
> +
> +	port->trace = devm_kzalloc(port->dev, sizeof(*port->trace),
> GFP_KERNEL);
> +	if (!port->trace)
> +		goto err_debugfs_dir;
> +
> +	port->trace->ctrl_file = debugfs_create_file("mdlog_ctrl",
> +						     T7XX_TRC_FILE_PERM,
> +						     port->debugfs_dir,
> +						     port,
> +						     &t7xx_trace_fops);
> +	if (!port->trace->ctrl_file)
> +		goto err_debugfs_dir;
> +
> +	port->trace->t7xx_rchan = relay_open("relay_ch",
> +					     port->debugfs_dir,
> +					     T7XX_TRC_SUB_BUFF_SIZE,
> +					     T7XX_TRC_N_SUB_BUFF,
> +					     &relay_callbacks, NULL);
> +	if (!port->trace->t7xx_rchan)
> +		goto err_debugfs_dir;

Even though trace resource is allocated using managed API good to call devm_kfree() in error paths ?

> +
> +	return 0;
> +
> +err_debugfs_dir:
> +	debugfs_remove_recursive(port->debugfs_dir);
> +	return -ENOMEM;
> +}
> +
> +static void t7xx_trace_port_uninit(struct t7xx_port *port) {
> +	struct t7xx_trace *trace = port->trace;
> +
> +	relay_close(trace->t7xx_rchan);
> +	debugfs_remove_recursive(port->debugfs_dir);
> +}
> +
> +static int t7xx_trace_port_recv_skb(struct t7xx_port *port, struct
> +sk_buff *skb) {
> +	struct t7xx_trace *t7xx_trace = port->trace;
> +
> +	if (!t7xx_trace->t7xx_rchan)
> +		return -EINVAL;

skb free not required is it considered by caller ?

> +
> +	relay_write(t7xx_trace->t7xx_rchan, skb->data, skb->len);
> +	dev_kfree_skb(skb);
> +	return 0;
> +}

Re: [PATCH net-next 1/1] net: wwan: t7xx: Add port for modem logging

[Jakub Kicinski]

On Thu, 19 May 2022 11:27:03 -0700 Moises Veleta wrote:
> +	ret = copy_from_user(skb_put(skb, actual_len), buf, actual_len);
> +	if (ret) {
> +		ret = -EFAULT;
> +		goto err_out;
> +	}
> +
> +	ret = t7xx_port_send_skb(port, skb, 0, 0);
> +	if (ret)
> +		goto err_out;

We don't allow using debugfs to pass random data from user space 
to firmware in networking. You need to find another way.

Re: [PATCH net-next 1/1] net: wwan: t7xx: Add port for modem logging

[moises.veleta]

On 5/20/22 10:37, Jakub Kicinski wrote:
> On Thu, 19 May 2022 11:27:03 -0700 Moises Veleta wrote:
>> +	ret = copy_from_user(skb_put(skb, actual_len), buf, actual_len);
>> +	if (ret) {
>> +		ret = -EFAULT;
>> +		goto err_out;
>> +	}
>> +
>> +	ret = t7xx_port_send_skb(port, skb, 0, 0);
>> +	if (ret)
>> +		goto err_out;
> We don't allow using debugfs to pass random data from user space
> to firmware in networking. You need to find another way.


Can we use debugfs to send an "on" or "off" commands, wherein the driver 
then sends special command sequences to the the firmware triggering the 
modem logging on and off?

Re: [PATCH net-next 1/1] net: wwan: t7xx: Add port for modem logging

[Jakub Kicinski]

On Fri, 20 May 2022 10:42:56 -0700 moises.veleta wrote:
> On 5/20/22 10:37, Jakub Kicinski wrote:
> > On Thu, 19 May 2022 11:27:03 -0700 Moises Veleta wrote:  
> >> +	ret = copy_from_user(skb_put(skb, actual_len), buf, actual_len);
> >> +	if (ret) {
> >> +		ret = -EFAULT;
> >> +		goto err_out;
> >> +	}
> >> +
> >> +	ret = t7xx_port_send_skb(port, skb, 0, 0);
> >> +	if (ret)
> >> +		goto err_out;  
> > We don't allow using debugfs to pass random data from user space
> > to firmware in networking. You need to find another way.  
> 
> 
> Can we use debugfs to send an "on" or "off" commands, wherein the driver 
> then sends special command sequences to the the firmware triggering the 
> modem logging on and off?

On/off for all logging or for particular types of messages?
If it's for all logging can't the act of opening the debugfs 
file be used as on "on" signal and closing as "off"?

Where do the logging messages go?

Re: [PATCH net-next 1/1] net: wwan: t7xx: Add port for modem logging

[moises.veleta]

On 5/20/22 10:48, Jakub Kicinski wrote:
> On Fri, 20 May 2022 10:42:56 -0700 moises.veleta wrote:
>> On 5/20/22 10:37, Jakub Kicinski wrote:
>>> On Thu, 19 May 2022 11:27:03 -0700 Moises Veleta wrote:
>>>> +	ret = copy_from_user(skb_put(skb, actual_len), buf, actual_len);
>>>> +	if (ret) {
>>>> +		ret = -EFAULT;
>>>> +		goto err_out;
>>>> +	}
>>>> +
>>>> +	ret = t7xx_port_send_skb(port, skb, 0, 0);
>>>> +	if (ret)
>>>> +		goto err_out;
>>> We don't allow using debugfs to pass random data from user space
>>> to firmware in networking. You need to find another way.
>>
>> Can we use debugfs to send an "on" or "off" commands, wherein the driver
>> then sends special command sequences to the the firmware triggering the
>> modem logging on and off?
> On/off for all logging or for particular types of messages?
> If it's for all logging can't the act of opening the debugfs
> file be used as on "on" signal and closing as "off"?
>
> Where do the logging messages go?

It would be "on/off" for all logging.
Yes, opening the debugfs file can be used for "on" and closing for "off" 
without needing to use copy_from_user.

Logging messages would go to the relay interface file for consumption by 
a user application.

Re: [PATCH net-next 1/1] net: wwan: t7xx: Add port for modem logging

[Jakub Kicinski]

On Fri, 20 May 2022 11:01:31 -0700 moises.veleta wrote:
> On 5/20/22 10:48, Jakub Kicinski wrote:
> > On Fri, 20 May 2022 10:42:56 -0700 moises.veleta wrote:  
> >> Can we use debugfs to send an "on" or "off" commands, wherein the driver
> >> then sends special command sequences to the the firmware triggering the
> >> modem logging on and off?  
> > On/off for all logging or for particular types of messages?
> > If it's for all logging can't the act of opening the debugfs
> > file be used as on "on" signal and closing as "off"?
> >
> > Where do the logging messages go?  
> 
> It would be "on/off" for all logging.
> Yes, opening the debugfs file can be used for "on" and closing for "off" 
> without needing to use copy_from_user.

Sounds good. Can we also divert the actual logs so that they can be
read out of that debugfs file? That'd feel most natural to me..

> Logging messages would go to the relay interface file for consumption by 
> a user application.

What's the relay interface? a special netdev? chardev? tty?

Re: [PATCH net-next 1/1] net: wwan: t7xx: Add port for modem logging

[moises.veleta]

On 5/20/22 01:33, Kumar, M Chetan wrote:
>> -----Original Message-----
>> From: Moises Veleta <moises.veleta@linux.intel.com>
>> Sent: Thursday, May 19, 2022 11:57 PM
>> To: netdev@vger.kernel.org
>> Cc: kuba@kernel.org; davem@davemloft.net; johannes@sipsolutions.net;
>> ryazanov.s.a@gmail.com; loic.poulain@linaro.org; Kumar, M Chetan
>> <m.chetan.kumar@intel.com>; Devegowda, Chandrashekar
>> <chandrashekar.devegowda@intel.com>; linuxwwan
>> <linuxwwan@intel.com>; Liu, Haijun <haijun.liu@mediatek.com>;
>> andriy.shevchenko@linux.intel.com; ilpo.jarvinen@linux.intel.com;
>> ricardo.martinez@linux.intel.com; Kancharla, Sreehari
>> <sreehari.kancharla@intel.com>; Sharma, Dinesh
>> <dinesh.sharma@intel.com>; Moises Veleta
>> <moises.veleta@linux.intel.com>
>> Subject: [PATCH net-next 1/1] net: wwan: t7xx: Add port for modem logging
>>
>> The Modem Logging (MDL) port provides an interface to collect modem logs
>> for debugging purposes. MDL is supported by debugfs, the relay interface,
>> and the mtk_t7xx port infrastructure. MDL allows user-space applications to
>> control logging via debugfs and to collect logs via the relay interface, while
>> port infrastructure facilitates communication between the driver and the
>> modem.
>>
>> Signed-off-by: Moises Veleta <moises.veleta@linux.intel.com>
>> Acked-by: Ricardo Martinez <ricardo.martinez@linux.intel.com>
>> ---
>>   drivers/net/wwan/Kconfig                |   1 +
>>   drivers/net/wwan/t7xx/Makefile          |   3 +
>>   drivers/net/wwan/t7xx/t7xx_hif_cldma.c  |   2 +
>>   drivers/net/wwan/t7xx/t7xx_port.h       |   5 +
>>   drivers/net/wwan/t7xx/t7xx_port_proxy.c |  22 +++
>>   drivers/net/wwan/t7xx/t7xx_port_proxy.h |   4 +
>>   drivers/net/wwan/t7xx/t7xx_port_trace.c | 174
>> ++++++++++++++++++++++++
>>   7 files changed, 211 insertions(+)
>>   create mode 100644 drivers/net/wwan/t7xx/t7xx_port_trace.c
>>
>> diff --git a/drivers/net/wwan/Kconfig b/drivers/net/wwan/Kconfig index
>> 3486ffe94ac4..32149029c891 100644
>> --- a/drivers/net/wwan/Kconfig
>> +++ b/drivers/net/wwan/Kconfig
>> @@ -108,6 +108,7 @@ config IOSM
>>   config MTK_T7XX
>>   	tristate "MediaTek PCIe 5G WWAN modem T7xx device"
>>   	depends on PCI
>> +	select RELAY if WWAN_DEBUGFS
>>   	help
>>   	  Enables MediaTek PCIe based 5G WWAN modem (T7xx series)
>> device.
>>   	  Adapts WWAN framework and provides network interface like
>> wwan0 diff --git a/drivers/net/wwan/t7xx/Makefile
>> b/drivers/net/wwan/t7xx/Makefile index dc6a7d682c15..268ff9e87e5b
>> 100644
>> --- a/drivers/net/wwan/t7xx/Makefile
>> +++ b/drivers/net/wwan/t7xx/Makefile
>> @@ -18,3 +18,6 @@ mtk_t7xx-y:=	t7xx_pci.o \
>>   		t7xx_hif_dpmaif_rx.o  \
>>   		t7xx_dpmaif.o \
>>   		t7xx_netdev.o
>> +
>> +mtk_t7xx-$(CONFIG_WWAN_DEBUGFS) += \
>> +		t7xx_port_trace.o \
> Drop \
>
Will do, thanks.
>> diff --git a/drivers/net/wwan/t7xx/t7xx_hif_cldma.c
>> b/drivers/net/wwan/t7xx/t7xx_hif_cldma.c
>> index 0c52801ed0de..dcd480720edf 100644
>> --- a/drivers/net/wwan/t7xx/t7xx_hif_cldma.c
>> +++ b/drivers/net/wwan/t7xx/t7xx_hif_cldma.c
>> @@ -1018,6 +1018,8 @@ static int t7xx_cldma_late_init(struct cldma_ctrl
>> *md_ctrl)
>>   			dev_err(md_ctrl->dev, "control TX ring init fail\n");
>>   			goto err_free_tx_ring;
>>   		}
>> +
>> +		md_ctrl->tx_ring[i].pkt_size = CLDMA_MTU;
>>   	}
>>
>>   	for (j = 0; j < CLDMA_RXQ_NUM; j++) {
>> diff --git a/drivers/net/wwan/t7xx/t7xx_port.h
>> b/drivers/net/wwan/t7xx/t7xx_port.h
>> index dc4133eb433a..e35efb18ea09 100644
>> --- a/drivers/net/wwan/t7xx/t7xx_port.h
>> +++ b/drivers/net/wwan/t7xx/t7xx_port.h
>> @@ -122,10 +122,15 @@ struct t7xx_port {
>>   	int				rx_length_th;
>>   	bool				chan_enable;
>>   	struct task_struct		*thread;
>> +#ifdef CONFIG_WWAN_DEBUGFS
>> +	struct t7xx_trace		*trace;
>> +	struct dentry			*debugfs_dir;
>> +#endif
>>   };
>>
>>   struct sk_buff *t7xx_port_alloc_skb(int payload);  struct sk_buff
>> *t7xx_ctrl_alloc_skb(int payload);
>> +int t7xx_port_mtu(struct t7xx_port *port);
>>   int t7xx_port_enqueue_skb(struct t7xx_port *port, struct sk_buff *skb);  int
>> t7xx_port_send_skb(struct t7xx_port *port, struct sk_buff *skb, unsigned int
>> pkt_header,
>>   		       unsigned int ex_msg);
>> diff --git a/drivers/net/wwan/t7xx/t7xx_port_proxy.c
>> b/drivers/net/wwan/t7xx/t7xx_port_proxy.c
>> index 7d2c0e81e33d..fb9d057d6a84 100644
>> --- a/drivers/net/wwan/t7xx/t7xx_port_proxy.c
>> +++ b/drivers/net/wwan/t7xx/t7xx_port_proxy.c
>> @@ -70,6 +70,18 @@ static const struct t7xx_port_conf
>> t7xx_md_port_conf[] = {
>>   		.name = "MBIM",
>>   		.port_type = WWAN_PORT_MBIM,
>>   	}, {
>> +#ifdef CONFIG_WWAN_DEBUGFS
>> +		.tx_ch = PORT_CH_MD_LOG_TX,
>> +		.rx_ch = PORT_CH_MD_LOG_RX,
>> +		.txq_index = 7,
>> +		.rxq_index = 7,
>> +		.txq_exp_index = 7,
>> +		.rxq_exp_index = 7,
>> +		.path_id = CLDMA_ID_MD,
>> +		.ops = &t7xx_trace_port_ops,
>> +		.name = "mdlog",
>> +	}, {
>> +#endif
> Why do you want keep mdlog port under flag ?
>
Modem logging will depends WWAN debugfs functions which also use this 
flag. Also, it should only be built on a debugging image with 
CONFIG_WWAN_DEBUGFS and not by default.

>>   		.tx_ch = PORT_CH_CONTROL_TX,
>>   		.rx_ch = PORT_CH_CONTROL_RX,
>>   		.txq_index = Q_IDX_CTRL,
>> @@ -194,6 +206,16 @@ int t7xx_port_enqueue_skb(struct t7xx_port *port,
>> struct sk_buff *skb)
>>   	return 0;
>>   }
>>
>> +int t7xx_port_mtu(struct t7xx_port *port) {
>> +	enum cldma_id path_id = port->port_conf->path_id;
>> +	int tx_qno = t7xx_port_get_queue_no(port);
>> +	struct cldma_ctrl *md_ctrl;
>> +
>> +	md_ctrl = port->t7xx_dev->md->md_ctrl[path_id];
>> +	return md_ctrl->tx_ring[tx_qno].pkt_size;
>> +}
>> +
>>   static int t7xx_port_send_raw_skb(struct t7xx_port *port, struct sk_buff
>> *skb)  {
>>   	enum cldma_id path_id = port->port_conf->path_id; diff --git
>> a/drivers/net/wwan/t7xx/t7xx_port_proxy.h
>> b/drivers/net/wwan/t7xx/t7xx_port_proxy.h
>> index bc1ff5c6c700..81d059fbc0fb 100644
>> --- a/drivers/net/wwan/t7xx/t7xx_port_proxy.h
>> +++ b/drivers/net/wwan/t7xx/t7xx_port_proxy.h
>> @@ -87,6 +87,10 @@ struct ctrl_msg_header {  extern struct port_ops
>> wwan_sub_port_ops;  extern struct port_ops ctl_port_ops;
>>
>> +#ifdef CONFIG_WWAN_DEBUGFS
>> +extern struct port_ops t7xx_trace_port_ops; #endif
>> +
>>   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_trace.c
>> b/drivers/net/wwan/t7xx/t7xx_port_trace.c
>> new file mode 100644
>> index 000000000000..87529316b183
>> --- /dev/null
>> +++ b/drivers/net/wwan/t7xx/t7xx_port_trace.c
>> @@ -0,0 +1,174 @@
>> +// SPDX-License-Identifier: GPL-2.0-only
>> +/*
>> + * Copyright (C) 2022 Intel Corporation.
>> + */
>> +
>> +#include <linux/bitfield.h>
>> +#include <linux/debugfs.h>
>> +#include <linux/relay.h>
>> +#include <linux/skbuff.h>
>> +#include <linux/wwan.h>
>> +
>> +#include "t7xx_port.h"
>> +#include "t7xx_port_proxy.h"
>> +#include "t7xx_state_monitor.h"
>> +
>> +#define T7XX_TRC_SUB_BUFF_SIZE		131072
>> +#define T7XX_TRC_N_SUB_BUFF		32
>> +#define T7XX_TRC_FILE_PERM		0600
>> +
>> +struct t7xx_trace {
>> +	struct rchan			*t7xx_rchan;
>> +	struct dentry			*ctrl_file;
>> +};
>> +
>> +static struct dentry *t7xx_trace_create_buf_file_handler(const char
>> *filename,
>> +							 struct dentry
>> *parent,
>> +							 umode_t mode,
>> +							 struct rchan_buf
>> *buf,
>> +							 int *is_global)
>> +{
>> +	*is_global = 1;
>> +	return debugfs_create_file(filename, mode, parent, buf,
>> +				   &relay_file_operations);
>> +}
>> +
>> +static int t7xx_trace_remove_buf_file_handler(struct dentry *dentry) {
>> +	debugfs_remove(dentry);
>> +	return 0;
>> +}
>> +
>> +static int t7xx_trace_subbuf_start_handler(struct rchan_buf *buf, void
>> *subbuf,
>> +					   void *prev_subbuf,
>> +					   size_t prev_padding)
>> +{
>> +	if (relay_buf_full(buf)) {
>> +		pr_err_ratelimited("Relay_buf full dropping traces");
>> +		return 0;
>> +	}
>> +
>> +	return 1;
>> +}
>> +
>> +static struct rchan_callbacks relay_callbacks = {
>> +	.subbuf_start = t7xx_trace_subbuf_start_handler,
>> +	.create_buf_file = t7xx_trace_create_buf_file_handler,
>> +	.remove_buf_file = t7xx_trace_remove_buf_file_handler,
>> +};
>> +
>> +static ssize_t t7xx_port_trace_write(struct file *file, const char __user *buf,
>> +				     size_t len, loff_t *ppos)
>> +{
>> +	struct t7xx_port *port = file->private_data;
>> +	size_t actual_len, alloc_size, txq_mtu;
>> +	const struct t7xx_port_conf *port_conf;
>> +	enum md_state md_state;
>> +	struct sk_buff *skb;
>> +	int ret;
>> +
>> +	port_conf = port->port_conf;
>> +	md_state = t7xx_fsm_get_md_state(port->t7xx_dev->md->fsm_ctl);
>> +	if (md_state == MD_STATE_WAITING_FOR_HS1 || md_state ==
>> MD_STATE_WAITING_FOR_HS2) {
>> +		dev_warn(port->dev, "port: %s ch: %d, write fail when
>> md_state: %d\n",
>> +			 port_conf->name, port_conf->tx_ch, md_state);
>> +		return -ENODEV;
>> +	}
> This means debugfs knob is available to application even before driver & device handshake
>   is complete. Is not possible to defer debugfs knob creation until handshake is complete ?
We can implement a "md_state_notify" method, like the one that is done 
for t7xx_port_wwan_md_state_notify, to remedy this issue.
>> +
>> +	txq_mtu = t7xx_port_mtu(port);
>> +	alloc_size = min_t(size_t, txq_mtu, len + sizeof(struct ccci_header));
> To keep it even we can drop +sizeof(struct ccci_header).
Ok, we can drop actual_len and just use alloc_size if we subtract the 
sizeof ccci_header, as such:

     alloc_size = min_t(size_t, txq_mtu - sizeof(struct ccci_header), len);

     skb = t7xx_port_alloc_skb(alloc_size);

This removes the need for actual_len altogether.

>> +	actual_len = alloc_size - sizeof(struct ccci_header);
>> +	skb = t7xx_port_alloc_skb(alloc_size);
> alloc_size contains the actual len and t7xx_port_alloc_skb() is considering alloc_size +
> sizeof(struct ccci_header); So actual_len calculation is redundant.
Should be addressed with the change mentioned above for alloc_size.
>> +	if (!skb) {
>> +		ret = -ENOMEM;
>> +		goto err_out;
> In skb failure case an attempt is made to free skb() by calling dev_kfree_skb().
> Better to add new label and simply return ?
No need for label, we can just return since its a skb allocation error.

     skb = t7xx_port_alloc_skb(alloc_size);
     if (!skb)

         return -ENOMEM;

This is similar to the skb error return in t7xx_port_ctrl_tx in 
t7xx_port_wwan.

>> +	}
>> +
>> +	ret = copy_from_user(skb_put(skb, actual_len), buf, actual_len);
>> +	if (ret) {
>> +		ret = -EFAULT;
>> +		goto err_out;
>> +	}
>> +
>> +	ret = t7xx_port_send_skb(port, skb, 0, 0);
>> +	if (ret)
>> +		goto err_out;
>> +
>> +	return actual_len;
> If len report is txq_mtu then actual_len is returning - sizeof(struct ccci_header);
> Instead return len.
>
Should be addressed with the change mentioned above for alloc_size.
>> +
>> +err_out:
>> +	dev_err(port->dev, "write error done on %s, size: %zu, ret: %d\n",
>> +		port_conf->name, actual_len, ret);
>> +	dev_kfree_skb(skb);
>> +	return ret;
>> +}
>> +
>> +static const struct file_operations t7xx_trace_fops = {
>> +	.owner = THIS_MODULE,
>> +	.open = simple_open,
>> +	.write = t7xx_port_trace_write,
>> +};
>> +
>> +static int t7xx_trace_port_init(struct t7xx_port *port) {
>> +	struct dentry *debugfs_pdev = wwan_get_debugfs_dir(port->dev);
>> +
>> +	if (IS_ERR(debugfs_pdev))
>> +		debugfs_pdev = NULL;
>> +
>> +	port->debugfs_dir = debugfs_create_dir(KBUILD_MODNAME,
>> debugfs_pdev);
>> +	if (IS_ERR_OR_NULL(port->debugfs_dir))
>> +		return -ENOMEM;
>> +
>> +	port->trace = devm_kzalloc(port->dev, sizeof(*port->trace),
>> GFP_KERNEL);
>> +	if (!port->trace)
>> +		goto err_debugfs_dir;
>> +
>> +	port->trace->ctrl_file = debugfs_create_file("mdlog_ctrl",
>> +						     T7XX_TRC_FILE_PERM,
>> +						     port->debugfs_dir,
>> +						     port,
>> +						     &t7xx_trace_fops);
>> +	if (!port->trace->ctrl_file)
>> +		goto err_debugfs_dir;
>> +
>> +	port->trace->t7xx_rchan = relay_open("relay_ch",
>> +					     port->debugfs_dir,
>> +					     T7XX_TRC_SUB_BUFF_SIZE,
>> +					     T7XX_TRC_N_SUB_BUFF,
>> +					     &relay_callbacks, NULL);
>> +	if (!port->trace->t7xx_rchan)
>> +		goto err_debugfs_dir;
> Even though trace resource is allocated using managed API good to call devm_kfree() in error paths ?
To my understanding, we not do need call "devm_kfree()" in error paths. 
If anyone can comment on this further, please do.
>> +
>> +	return 0;
>> +
>> +err_debugfs_dir:
>> +	debugfs_remove_recursive(port->debugfs_dir);
>> +	return -ENOMEM;
>> +}
>> +
>> +static void t7xx_trace_port_uninit(struct t7xx_port *port) {
>> +	struct t7xx_trace *trace = port->trace;
>> +
>> +	relay_close(trace->t7xx_rchan);
>> +	debugfs_remove_recursive(port->debugfs_dir);
>> +}
>> +
>> +static int t7xx_trace_port_recv_skb(struct t7xx_port *port, struct
>> +sk_buff *skb) {
>> +	struct t7xx_trace *t7xx_trace = port->trace;
>> +
>> +	if (!t7xx_trace->t7xx_rchan)
>> +		return -EINVAL;
> skb free not required is it considered by caller ?
>
We can change this to

     if (t7xx_trace->t7xx_rchan)
         relay_write(t7xx_trace->t7xx_rchan, skb->data, skb->len);

     dev_kfree_skb(skb);
     return 0;

This would drop skb if there is an issue with the relay channel that is 
out of our control.

>> +
>> +	relay_write(t7xx_trace->t7xx_rchan, skb->data, skb->len);
>> +	dev_kfree_skb(skb);
>> +	return 0;
>> +}

Re: [PATCH net-next 1/1] net: wwan: t7xx: Add port for modem logging

[moises.veleta]

On 5/20/22 14:46, Jakub Kicinski wrote:
> On Fri, 20 May 2022 12:16:19 -0700 moises.veleta wrote:
>> On 5/20/22 11:15, Jakub Kicinski wrote:
>>> On Fri, 20 May 2022 11:01:31 -0700 moises.veleta wrote:
>>>> On 5/20/22 10:48, Jakub Kicinski wrote:
>>>>> On Fri, 20 May 2022 10:42:56 -0700 moises.veleta wrote:
>>>>>> Can we use debugfs to send an "on" or "off" commands, wherein the driver
>>>>>> then sends special command sequences to the the firmware triggering the
>>>>>> modem logging on and off?
>>>>> On/off for all logging or for particular types of messages?
>>>>> If it's for all logging can't the act of opening the debugfs
>>>>> file be used as on "on" signal and closing as "off"?
>>>>>
>>>>> Where do the logging messages go?
>>>> It would be "on/off" for all logging.
>>>> Yes, opening the debugfs file can be used for "on" and closing for "off"
>>>> without needing to use copy_from_user.
>>> Sounds good. Can we also divert the actual logs so that they can be
>>> read out of that debugfs file? That'd feel most natural to me..
>>>
>>>> Logging messages would go to the relay interface file for consumption by
>>>> a user application.
>>> What's the relay interface? a special netdev? chardev? tty?
>>>
>> The relay interface is a 'relay channel' where a userspace applications
>> can read from and retrieve data as it becomes available. The driver
>> would relay modem logs to this created channel file.
>>
>> https://www.kernel.org/doc/html/latest/filesystems/relay.html
> The API for this thing seems confusing, does it not give the kernel
> side any signal that user space has attached / shown interest in the
> data?
>
> If not a simple debugfs file which only accepts on/off or 0/1 SGTM.

For modem logging, the driver is relaying the logs (a large amount 
varying on modem traffic) to the relay interface file that will be 
overwritten if not consumed. It does not care if the user application is 
reading them or not. That is up to the user application to read and process.

The IOSM driver in WWAN uses this relay interface & debugfs combination, 
in a similar fashion, please see "iosm_ipc_trace.c" for their 
implementation. Should have mentioned that earlier, pardon the terseness.

Re: [PATCH net-next 1/1] net: wwan: t7xx: Add port for modem logging

[moises.veleta]

On 5/20/22 15:00, moises.veleta wrote:
>
> On 5/20/22 14:46, Jakub Kicinski wrote:
>> On Fri, 20 May 2022 12:16:19 -0700 moises.veleta wrote:
>>> On 5/20/22 11:15, Jakub Kicinski wrote:
>>>> On Fri, 20 May 2022 11:01:31 -0700 moises.veleta wrote:
>>>>> On 5/20/22 10:48, Jakub Kicinski wrote:
>>>>>> On Fri, 20 May 2022 10:42:56 -0700 moises.veleta wrote:
>>>>>>> Can we use debugfs to send an "on" or "off" commands, wherein 
>>>>>>> the driver
>>>>>>> then sends special command sequences to the the firmware 
>>>>>>> triggering the
>>>>>>> modem logging on and off?
>>>>>> On/off for all logging or for particular types of messages?
>>>>>> If it's for all logging can't the act of opening the debugfs
>>>>>> file be used as on "on" signal and closing as "off"?
>>>>>>
>>>>>> Where do the logging messages go?
>>>>> It would be "on/off" for all logging.
>>>>> Yes, opening the debugfs file can be used for "on" and closing for 
>>>>> "off"
>>>>> without needing to use copy_from_user.
>>>> Sounds good. Can we also divert the actual logs so that they can be
>>>> read out of that debugfs file? That'd feel most natural to me..
>>>>
>>>>> Logging messages would go to the relay interface file for 
>>>>> consumption by
>>>>> a user application.
>>>> What's the relay interface? a special netdev? chardev? tty?
>>>>
>>> The relay interface is a 'relay channel' where a userspace applications
>>> can read from and retrieve data as it becomes available. The driver
>>> would relay modem logs to this created channel file.
>>>
>>> https://www.kernel.org/doc/html/latest/filesystems/relay.html
>> The API for this thing seems confusing, does it not give the kernel
>> side any signal that user space has attached / shown interest in the
>> data?
>>
>> If not a simple debugfs file which only accepts on/off or 0/1 SGTM.
>
> For modem logging, the driver is relaying the logs (a large amount 
> varying on modem traffic) to the relay interface file that will be 
> overwritten if not consumed. It does not care if the user application 
> is reading them or not. That is up to the user application to read and 
> process.
>
> The IOSM driver in WWAN uses this relay interface & debugfs 
> combination, in a similar fashion, please see "iosm_ipc_trace.c" for 
> their implementation. Should have mentioned that earlier, pardon the 
> terseness.
>
Is the issue with using the debugfs to send random data to the modem an 
issue with validation?
If so, what if the modem itself is validating the input string would 
that suffice to allow this debugfs control port to act as the medium 
between user space applications and the modem? Leaving the driver blind 
to that process.
Or is it preferable to have the driver do the validation on the user 
space input to the modem?

Re: [PATCH net-next 1/1] net: wwan: t7xx: Add port for modem logging

[Jakub Kicinski]

On Mon, 23 May 2022 22:11:00 -0700 moises.veleta wrote:
> Is the issue with using the debugfs to send random data to the modem an 
> issue with validation?
> If so, what if the modem itself is validating the input string would 
> that suffice to allow this debugfs control port to act as the medium 
> between user space applications and the modem? Leaving the driver blind 
> to that process.
> Or is it preferable to have the driver do the validation on the user 
> space input to the modem?

Yes, the driver is not supposed to be blind to the process.