Date:   Fri, 20 May 2022 13:37:44 -0700
From:   "moises.veleta" <moises.veleta@...ux.intel.com>
To:     "Kumar, M Chetan" <m.chetan.kumar@...el.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


On 5/20/22 01:33, Kumar, M Chetan wrote:
>> -----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 \
>
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;
>> +}

Powered by blists - more mailing lists