Date:   Fri, 20 May 2022 08:33:06 +0000
From:   "Kumar, M Chetan" <m.chetan.kumar@...el.com>
To:     Moises Veleta <moises.veleta@...ux.intel.com>,
        "netdev@...r.kernel.org" <netdev@...r.kernel.org>
CC:     "kuba@...nel.org" <kuba@...nel.org>,
        "davem@...emloft.net" <davem@...emloft.net>,
        "johannes@...solutions.net" <johannes@...solutions.net>,
        "ryazanov.s.a@...il.com" <ryazanov.s.a@...il.com>,
        "loic.poulain@...aro.org" <loic.poulain@...aro.org>,
        "Devegowda, Chandrashekar" <chandrashekar.devegowda@...el.com>,
        linuxwwan <linuxwwan@...el.com>,
        "Liu, Haijun" <haijun.liu@...iatek.com>,
        "andriy.shevchenko@...ux.intel.com" 
        <andriy.shevchenko@...ux.intel.com>,
        "ilpo.jarvinen@...ux.intel.com" <ilpo.jarvinen@...ux.intel.com>,
        "ricardo.martinez@...ux.intel.com" <ricardo.martinez@...ux.intel.com>,
        "Kancharla, Sreehari" <sreehari.kancharla@...el.com>,
        "Sharma, Dinesh" <dinesh.sharma@...el.com>
Subject: RE: [PATCH net-next 1/1] net: wwan: t7xx: Add port for modem logging

> -----Original Message-----
> From: Moises Veleta <moises.veleta@...ux.intel.com>
> Sent: Thursday, May 19, 2022 11:57 PM
> To: netdev@...r.kernel.org
> Cc: kuba@...nel.org; davem@...emloft.net; johannes@...solutions.net;
> ryazanov.s.a@...il.com; loic.poulain@...aro.org; Kumar, M Chetan
> <m.chetan.kumar@...el.com>; Devegowda, Chandrashekar
> <chandrashekar.devegowda@...el.com>; linuxwwan
> <linuxwwan@...el.com>; Liu, Haijun <haijun.liu@...iatek.com>;
> andriy.shevchenko@...ux.intel.com; ilpo.jarvinen@...ux.intel.com;
> ricardo.martinez@...ux.intel.com; Kancharla, Sreehari
> <sreehari.kancharla@...el.com>; Sharma, Dinesh
> <dinesh.sharma@...el.com>; Moises Veleta
> <moises.veleta@...ux.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@...ux.intel.com>
> Acked-by: Ricardo Martinez <ricardo.martinez@...ux.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;
> +}

Powered by blists - more mailing lists