| Message ID | 20221003095725.978129-1-m.chetan.kumar@linux.intel.com (mailing list archive) |
|---|---|
| State | Deferred |
| Delegated to: | Netdev Maintainers |
| Headers | show |
| Series | [V3,net-next] net: wwan: t7xx: Add port for modem logging | expand |
On Mon, 3 Oct 2022 15:27:25 +0530 m.chetan.kumar@linux.intel.com wrote: > From: Moises Veleta <moises.veleta@linux.intel.com> > > The Modem Logging (MDL) port provides an interface to collect modem > logs for debugging purposes. MDL is supported by the relay interface, > and the mtk_t7xx port infrastructure. MDL allows user-space apps to > control logging via mbim command and to collect logs via the relay > interface, while port infrastructure facilitates communication between > the driver and the modem. net-next is closed, please repost once Linus tags 6.1-rc1 http://vger.kernel.org/~davem/net-next.html
Hello Chetan, On Mon, Oct 3, 2022 at 8:29 AM <m.chetan.kumar@linux.intel.com> wrote: > The Modem Logging (MDL) port provides an interface to collect modem > logs for debugging purposes. MDL is supported by the relay interface, > and the mtk_t7xx port infrastructure. MDL allows user-space apps to > control logging via mbim command and to collect logs via the relay > interface, while port infrastructure facilitates communication between > the driver and the modem. [skip] > diff --git a/drivers/net/wwan/t7xx/t7xx_port.h b/drivers/net/wwan/t7xx/t7xx_port.h > index dc4133eb433a..702e7aa2ef31 100644 > --- a/drivers/net/wwan/t7xx/t7xx_port.h > +++ b/drivers/net/wwan/t7xx/t7xx_port.h > @@ -122,6 +122,11 @@ struct t7xx_port { > int rx_length_th; > bool chan_enable; > struct task_struct *thread; > +#ifdef CONFIG_WWAN_DEBUGFS > + void *relaych; > + struct dentry *debugfs_dir; > + struct dentry *debugfs_wwan_dir; Both of these debugfs directories are device-wide, why did you place these pointers in the item port context?
Hi Sergey, On 10/16/2022 9:35 PM, Sergey Ryazanov wrote: > Hello Chetan, > > On Mon, Oct 3, 2022 at 8:29 AM <m.chetan.kumar@linux.intel.com> wrote: >> The Modem Logging (MDL) port provides an interface to collect modem >> logs for debugging purposes. MDL is supported by the relay interface, >> and the mtk_t7xx port infrastructure. MDL allows user-space apps to >> control logging via mbim command and to collect logs via the relay >> interface, while port infrastructure facilitates communication between >> the driver and the modem. > > [skip] > >> diff --git a/drivers/net/wwan/t7xx/t7xx_port.h b/drivers/net/wwan/t7xx/t7xx_port.h >> index dc4133eb433a..702e7aa2ef31 100644 >> --- a/drivers/net/wwan/t7xx/t7xx_port.h >> +++ b/drivers/net/wwan/t7xx/t7xx_port.h >> @@ -122,6 +122,11 @@ struct t7xx_port { >> int rx_length_th; >> bool chan_enable; >> struct task_struct *thread; >> +#ifdef CONFIG_WWAN_DEBUGFS >> + void *relaych; >> + struct dentry *debugfs_dir; >> + struct dentry *debugfs_wwan_dir; > > Both of these debugfs directories are device-wide, why did you place > these pointers in the item port context? > I guess it was kept inside port so that it could be accessed directly from port context. Thanks for pointing it out. I think we should move out the complete #ifdef CONFIG_WWAN_DEBUGFS block of port container. I am planning to add trace.h file and put changes under it. Below is the new code changes [1]. I am yet to verify. Please share your comments. [1] --- a/drivers/net/wwan/t7xx/t7xx_pci.h +++ b/drivers/net/wwan/t7xx/t7xx_pci.h @@ -24,6 +24,7 @@ #include <linux/spinlock.h> #include <linux/types.h> +#include "t7xx_port_trace.h" #include "t7xx_reg.h" /* struct t7xx_addr_base - holds base addresses @@ -59,6 +60,7 @@ typedef irqreturn_t (*t7xx_intr_callback)(int irq, void *param); * @md_pm_lock: protects PCIe sleep lock * @sleep_disable_count: PCIe L1.2 lock counter * @sleep_lock_acquire: indicates that sleep has been disabled + * @trace: relayfs and debugfs data struct */ struct t7xx_pci_dev { t7xx_intr_callback intr_handler[EXT_INT_NUM]; @@ -78,6 +80,7 @@ struct t7xx_pci_dev { spinlock_t md_pm_lock; /* Protects PCI resource lock */ unsigned int sleep_disable_count; struct completion sleep_lock_acquire; + struct t7xx_trace trace; }; enum t7xx_pm_id { diff --git a/drivers/net/wwan/t7xx/t7xx_port.h b/drivers/net/wwan/t7xx/t7xx_port.h index 702e7aa2ef31..dc4133eb433a 100644 --- a/drivers/net/wwan/t7xx/t7xx_port.h +++ b/drivers/net/wwan/t7xx/t7xx_port.h @@ -122,11 +122,6 @@ struct t7xx_port { int rx_length_th; bool chan_enable; struct task_struct *thread; -#ifdef CONFIG_WWAN_DEBUGFS - void *relaych; - struct dentry *debugfs_dir; - struct dentry *debugfs_wwan_dir; -#endif }; struct sk_buff *t7xx_port_alloc_skb(int payload); diff --git a/drivers/net/wwan/t7xx/t7xx_port_proxy.c b/drivers/net/wwan/t7xx/t7xx_port_proxy.c index 894b1d11b2c9..3377573568c6 100644 --- a/drivers/net/wwan/t7xx/t7xx_port_proxy.c +++ b/drivers/net/wwan/t7xx/t7xx_port_proxy.c @@ -35,6 +35,7 @@ #include "t7xx_modem_ops.h" #include "t7xx_port.h" #include "t7xx_port_proxy.h" +#include "t7xx_port_trace.h" #include "t7xx_state_monitor.h" #define Q_IDX_CTRL 0 diff --git a/drivers/net/wwan/t7xx/t7xx_port_proxy.h b/drivers/net/wwan/t7xx/t7xx_port_proxy.h index 81d059fbc0fb..c537f5b5ff60 100644 --- a/drivers/net/wwan/t7xx/t7xx_port_proxy.h +++ b/drivers/net/wwan/t7xx/t7xx_port_proxy.h @@ -87,9 +87,6 @@ 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); +++ b/drivers/net/wwan/t7xx/t7xx_port_trace.h @@ -0,0 +1,21 @@ +// SPDX-License-Identifier: GPL-2.0-only +/* + * Copyright (C) 2022 Intel Corporation. + */ + +#ifndef __T7XX_PORT_TRACE_H__ +#define __T7XX_PORT_TRACE_H__ + +struct t7xx_trace { +#ifdef CONFIG_WWAN_DEBUGFS + void *relaych; + struct dentry *debugfs_dir; + struct dentry *debugfs_wwan_dir; +#endif +}; + +#ifdef CONFIG_WWAN_DEBUGFS +extern struct port_ops t7xx_trace_port_ops; +#endif + +#endif /* __T7XX_PORT_TRACE_H__ */ diff --git a/drivers/net/wwan/t7xx/t7xx_port_trace.c b/drivers/net/wwan/t7xx/t7xx_port_trace.c index 3f740db318a8..1f5224fb0e5d 100644 --- a/drivers/net/wwan/t7xx/t7xx_port_trace.c +++ b/drivers/net/wwan/t7xx/t7xx_port_trace.c @@ -10,6 +10,7 @@ #include "t7xx_port.h" #include "t7xx_port_proxy.h" +#include "t7xx_port_trace.h" #include "t7xx_state_monitor.h" #define T7XX_TRC_SUB_BUFF_SIZE 131072 @@ -51,19 +52,19 @@ static struct rchan_callbacks relay_callbacks = { static void t7xx_trace_port_uninit(struct t7xx_port *port) { - struct rchan *relaych = port->relaych; + struct t7xx_trace *trace = &port->t7xx_dev->trace; - if (!relaych) + if (!trace->relaych) return; - relay_close(relaych); - debugfs_remove_recursive(port->debugfs_dir); - wwan_put_debugfs_dir(port->debugfs_wwan_dir); + relay_close(trace->relaych); + debugfs_remove_recursive(trace->debugfs_dir); + wwan_put_debugfs_dir(trace->debugfs_wwan_dir); } static int t7xx_trace_port_recv_skb(struct t7xx_port *port, struct sk_buff *skb) { - struct rchan *relaych = port->relaych; + struct rchan *relaych = port->t7xx_dev->trace.relaych; if (!relaych) return -EINVAL; @@ -75,33 +76,34 @@ static int t7xx_trace_port_recv_skb(struct t7xx_port *port, struct sk_buff *skb) static void t7xx_port_trace_md_state_notify(struct t7xx_port *port, unsigned int state) { + struct t7xx_trace *trace = &port->t7xx_dev->trace; struct rchan *relaych; - if (state != MD_STATE_READY || port->relaych) + if (state != MD_STATE_READY || trace->relaych) return; - port->debugfs_wwan_dir = wwan_get_debugfs_dir(port->dev); - if (IS_ERR(port->debugfs_wwan_dir)) + trace->debugfs_wwan_dir = wwan_get_debugfs_dir(port->dev); + if (IS_ERR(trace->debugfs_wwan_dir)) return; - port->debugfs_dir = debugfs_create_dir(KBUILD_MODNAME, port->debugfs_wwan_dir); - if (IS_ERR_OR_NULL(port->debugfs_dir)) { - wwan_put_debugfs_dir(port->debugfs_wwan_dir); + trace->debugfs_dir = debugfs_create_dir(KBUILD_MODNAME, trace->debugfs_wwan_dir); + if (IS_ERR_OR_NULL(trace->debugfs_dir)) { + wwan_put_debugfs_dir(trace->debugfs_wwan_dir); dev_err(port->dev, "Unable to create debugfs for trace"); return; } - relaych = relay_open("relay_ch", port->debugfs_dir, T7XX_TRC_SUB_BUFF_SIZE, + relaych = relay_open("relay_ch", trace->debugfs_dir, T7XX_TRC_SUB_BUFF_SIZE, T7XX_TRC_N_SUB_BUFF, &relay_callbacks, NULL); if (!relaych) goto err_rm_debugfs_dir; - port->relaych = relaych; + trace->relaych = relaych; return; err_rm_debugfs_dir: - debugfs_remove_recursive(port->debugfs_dir); - wwan_put_debugfs_dir(port->debugfs_wwan_dir); + debugfs_remove_recursive(trace->debugfs_dir); + wwan_put_debugfs_dir(trace->debugfs_wwan_dir); dev_err(port->dev, "Unable to create trace port %s", port->port_conf->name); }
On Mon, Oct 17, 2022 at 1:16 PM Kumar, M Chetan <m.chetan.kumar@linux.intel.com> wrote: > On 10/16/2022 9:35 PM, Sergey Ryazanov wrote: >>>> On Mon, Oct 3, 2022 at 8:29 AM <m.chetan.kumar@linux.intel.com> wrote: >>>>> The Modem Logging (MDL) port provides an interface to collect modem >>>>> logs for debugging purposes. MDL is supported by the relay interface, >>>>> and the mtk_t7xx port infrastructure. MDL allows user-space apps to >>>>> control logging via mbim command and to collect logs via the relay >>>>> interface, while port infrastructure facilitates communication between >>>>> the driver and the modem. >>>> >>>> [skip] >>>> >>>>> diff --git a/drivers/net/wwan/t7xx/t7xx_port.h b/drivers/net/wwan/t7xx/t7xx_port.h >>>>> index dc4133eb433a..702e7aa2ef31 100644 >>>>> --- a/drivers/net/wwan/t7xx/t7xx_port.h >>>>> +++ b/drivers/net/wwan/t7xx/t7xx_port.h >>>>> @@ -122,6 +122,11 @@ struct t7xx_port { >>>>> int rx_length_th; >>>>> bool chan_enable; >>>>> struct task_struct *thread; >>>>> +#ifdef CONFIG_WWAN_DEBUGFS >>>>> + void *relaych; >>>>> + struct dentry *debugfs_dir; >>>>> + struct dentry *debugfs_wwan_dir; >>>> >>>> Both of these debugfs directories are device-wide, why did you place >>>> these pointers in the item port context? > > I guess it was kept inside port so that it could be accessed directly > from port context. > > Thanks for pointing it out. I think we should move out the complete > #ifdef CONFIG_WWAN_DEBUGFS block of port container. Moving out debugfs directory pointers sounds like a good idea. Introduction of any new debugfs knob will be a much easier if these pointers are available in some device-wide state container. But the relaych pointer looks like a logging port specific element. Why should it be moved out? > I am planning to add trace.h file and put changes under it. The word 'trace' usually used for entities related to use of the kernel tracing framework, i.e. driver itself tracing. If you really need this dedicated header, can we entitle it like 'mdm_log' or 'logging' or something like that? > Below is the new code changes [1]. I am yet to verify. > Please share your comments. > > [1] > --- a/drivers/net/wwan/t7xx/t7xx_pci.h > +++ b/drivers/net/wwan/t7xx/t7xx_pci.h > @@ -24,6 +24,7 @@ > #include <linux/spinlock.h> > #include <linux/types.h> > > +#include "t7xx_port_trace.h" > #include "t7xx_reg.h" > > /* struct t7xx_addr_base - holds base addresses > @@ -59,6 +60,7 @@ typedef irqreturn_t (*t7xx_intr_callback)(int irq, > void *param); > * @md_pm_lock: protects PCIe sleep lock > * @sleep_disable_count: PCIe L1.2 lock counter > * @sleep_lock_acquire: indicates that sleep has been disabled > + * @trace: relayfs and debugfs data struct > */ > struct t7xx_pci_dev { > t7xx_intr_callback intr_handler[EXT_INT_NUM]; > @@ -78,6 +80,7 @@ struct t7xx_pci_dev { > spinlock_t md_pm_lock; /* Protects PCI resource lock */ > unsigned int sleep_disable_count; > struct completion sleep_lock_acquire; > + struct t7xx_trace trace; > }; > > enum t7xx_pm_id { > diff --git a/drivers/net/wwan/t7xx/t7xx_port.h > b/drivers/net/wwan/t7xx/t7xx_port.h > index 702e7aa2ef31..dc4133eb433a 100644 > --- a/drivers/net/wwan/t7xx/t7xx_port.h > +++ b/drivers/net/wwan/t7xx/t7xx_port.h > @@ -122,11 +122,6 @@ struct t7xx_port { > int rx_length_th; > bool chan_enable; > struct task_struct *thread; > -#ifdef CONFIG_WWAN_DEBUGFS > - void *relaych; > - struct dentry *debugfs_dir; > - struct dentry *debugfs_wwan_dir; > -#endif > }; > > struct sk_buff *t7xx_port_alloc_skb(int payload); > diff --git a/drivers/net/wwan/t7xx/t7xx_port_proxy.c > b/drivers/net/wwan/t7xx/t7xx_port_proxy.c > index 894b1d11b2c9..3377573568c6 100644 > --- a/drivers/net/wwan/t7xx/t7xx_port_proxy.c > +++ b/drivers/net/wwan/t7xx/t7xx_port_proxy.c > @@ -35,6 +35,7 @@ > #include "t7xx_modem_ops.h" > #include "t7xx_port.h" > #include "t7xx_port_proxy.h" > +#include "t7xx_port_trace.h" > #include "t7xx_state_monitor.h" > > #define Q_IDX_CTRL 0 > diff --git a/drivers/net/wwan/t7xx/t7xx_port_proxy.h > b/drivers/net/wwan/t7xx/t7xx_port_proxy.h > index 81d059fbc0fb..c537f5b5ff60 100644 > --- a/drivers/net/wwan/t7xx/t7xx_port_proxy.h > +++ b/drivers/net/wwan/t7xx/t7xx_port_proxy.h > @@ -87,9 +87,6 @@ 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); > > +++ b/drivers/net/wwan/t7xx/t7xx_port_trace.h > @@ -0,0 +1,21 @@ > +// SPDX-License-Identifier: GPL-2.0-only > +/* > + * Copyright (C) 2022 Intel Corporation. > + */ > + > +#ifndef __T7XX_PORT_TRACE_H__ > +#define __T7XX_PORT_TRACE_H__ > + > +struct t7xx_trace { > +#ifdef CONFIG_WWAN_DEBUGFS > + void *relaych; > + struct dentry *debugfs_dir; > + struct dentry *debugfs_wwan_dir; > +#endif > +}; The relaych pointer is port specific, so it can be left in the port structure. We do not need to keep the debugfs_dir pointer. It is only needed during debugfs initialization to create the modem-specific subdirectory. So it can be a local variable. There is only one debugfs_wwan_dir pointer left, which can be placed somewhere in the t7xx_modem structure. > +#ifdef CONFIG_WWAN_DEBUGFS > +extern struct port_ops t7xx_trace_port_ops; > +#endif > + > +#endif /* __T7XX_PORT_TRACE_H__ */ > > diff --git a/drivers/net/wwan/t7xx/t7xx_port_trace.c > b/drivers/net/wwan/t7xx/t7xx_port_trace.c > index 3f740db318a8..1f5224fb0e5d 100644 > --- a/drivers/net/wwan/t7xx/t7xx_port_trace.c > +++ b/drivers/net/wwan/t7xx/t7xx_port_trace.c > @@ -10,6 +10,7 @@ > > #include "t7xx_port.h" > #include "t7xx_port_proxy.h" > +#include "t7xx_port_trace.h" > #include "t7xx_state_monitor.h" > > #define T7XX_TRC_SUB_BUFF_SIZE 131072 > @@ -51,19 +52,19 @@ static struct rchan_callbacks relay_callbacks = { > > static void t7xx_trace_port_uninit(struct t7xx_port *port) > { > - struct rchan *relaych = port->relaych; > + struct t7xx_trace *trace = &port->t7xx_dev->trace; > > - if (!relaych) > + if (!trace->relaych) > return; > > - relay_close(relaych); > - debugfs_remove_recursive(port->debugfs_dir); > - wwan_put_debugfs_dir(port->debugfs_wwan_dir); > + relay_close(trace->relaych); > + debugfs_remove_recursive(trace->debugfs_dir); > + wwan_put_debugfs_dir(trace->debugfs_wwan_dir); > } > > static int t7xx_trace_port_recv_skb(struct t7xx_port *port, struct > sk_buff *skb) > { > - struct rchan *relaych = port->relaych; > + struct rchan *relaych = port->t7xx_dev->trace.relaych; > > if (!relaych) > return -EINVAL; > @@ -75,33 +76,34 @@ static int t7xx_trace_port_recv_skb(struct t7xx_port > *port, struct sk_buff *skb) > > static void t7xx_port_trace_md_state_notify(struct t7xx_port *port, > unsigned int state) > { > + struct t7xx_trace *trace = &port->t7xx_dev->trace; > struct rchan *relaych; > > - if (state != MD_STATE_READY || port->relaych) > + if (state != MD_STATE_READY || trace->relaych) > return; > > - port->debugfs_wwan_dir = wwan_get_debugfs_dir(port->dev); > - if (IS_ERR(port->debugfs_wwan_dir)) > + trace->debugfs_wwan_dir = wwan_get_debugfs_dir(port->dev); > + if (IS_ERR(trace->debugfs_wwan_dir)) > return; > > - port->debugfs_dir = debugfs_create_dir(KBUILD_MODNAME, > port->debugfs_wwan_dir); > - if (IS_ERR_OR_NULL(port->debugfs_dir)) { > - wwan_put_debugfs_dir(port->debugfs_wwan_dir); > + trace->debugfs_dir = debugfs_create_dir(KBUILD_MODNAME, > trace->debugfs_wwan_dir); > + if (IS_ERR_OR_NULL(trace->debugfs_dir)) { > + wwan_put_debugfs_dir(trace->debugfs_wwan_dir); > dev_err(port->dev, "Unable to create debugfs for trace"); > return; > } > > - relaych = relay_open("relay_ch", port->debugfs_dir, > T7XX_TRC_SUB_BUFF_SIZE, > + relaych = relay_open("relay_ch", trace->debugfs_dir, > T7XX_TRC_SUB_BUFF_SIZE, > T7XX_TRC_N_SUB_BUFF, &relay_callbacks, NULL); > if (!relaych) > goto err_rm_debugfs_dir; > > - port->relaych = relaych; > + trace->relaych = relaych; > return; > > err_rm_debugfs_dir: > - debugfs_remove_recursive(port->debugfs_dir); > - wwan_put_debugfs_dir(port->debugfs_wwan_dir); > + debugfs_remove_recursive(trace->debugfs_dir); > + wwan_put_debugfs_dir(trace->debugfs_wwan_dir); > dev_err(port->dev, "Unable to create trace port %s", > port->port_conf->name); > }
On 10/18/2022 5:29 AM, Sergey Ryazanov wrote: > On Mon, Oct 17, 2022 at 1:16 PM Kumar, M Chetan > <m.chetan.kumar@linux.intel.com> wrote: >> On 10/16/2022 9:35 PM, Sergey Ryazanov wrote: >>>>> On Mon, Oct 3, 2022 at 8:29 AM <m.chetan.kumar@linux.intel.com> wrote: >>>>>> The Modem Logging (MDL) port provides an interface to collect modem >>>>>> logs for debugging purposes. MDL is supported by the relay interface, >>>>>> and the mtk_t7xx port infrastructure. MDL allows user-space apps to >>>>>> control logging via mbim command and to collect logs via the relay >>>>>> interface, while port infrastructure facilitates communication between >>>>>> the driver and the modem. >>>>> >>>>> [skip] >>>>> >>>>>> diff --git a/drivers/net/wwan/t7xx/t7xx_port.h b/drivers/net/wwan/t7xx/t7xx_port.h >>>>>> index dc4133eb433a..702e7aa2ef31 100644 >>>>>> --- a/drivers/net/wwan/t7xx/t7xx_port.h >>>>>> +++ b/drivers/net/wwan/t7xx/t7xx_port.h >>>>>> @@ -122,6 +122,11 @@ struct t7xx_port { >>>>>> int rx_length_th; >>>>>> bool chan_enable; >>>>>> struct task_struct *thread; >>>>>> +#ifdef CONFIG_WWAN_DEBUGFS >>>>>> + void *relaych; >>>>>> + struct dentry *debugfs_dir; >>>>>> + struct dentry *debugfs_wwan_dir; >>>>> >>>>> Both of these debugfs directories are device-wide, why did you place >>>>> these pointers in the item port context? >> >> I guess it was kept inside port so that it could be accessed directly >> from port context. >> >> Thanks for pointing it out. I think we should move out the complete >> #ifdef CONFIG_WWAN_DEBUGFS block of port container. > > Moving out debugfs directory pointers sounds like a good idea. > Introduction of any new debugfs knob will be a much easier if these > pointers are available in some device-wide state container. But the > relaych pointer looks like a logging port specific element. Why should > it be moved out? t7xx_port is a common port container. keeping relaych pointer inside port container is like having relaych pointer for all port instance (AT/MBIM) though it is not required for others. So planning to move out. You suggest to keep it or move out ? > >> I am planning to add trace.h file and put changes under it. > > The word 'trace' usually used for entities related to use of the > kernel tracing framework, i.e. driver itself tracing. If you really > need this dedicated header, can we entitle it like 'mdm_log' or > 'logging' or something like that? > >> Below is the new code changes [1]. I am yet to verify. >> Please share your comments. >> >> [1] >> --- a/drivers/net/wwan/t7xx/t7xx_pci.h >> +++ b/drivers/net/wwan/t7xx/t7xx_pci.h >> @@ -24,6 +24,7 @@ >> #include <linux/spinlock.h> >> #include <linux/types.h> >> >> +#include "t7xx_port_trace.h" >> #include "t7xx_reg.h" >> >> /* struct t7xx_addr_base - holds base addresses >> @@ -59,6 +60,7 @@ typedef irqreturn_t (*t7xx_intr_callback)(int irq, >> void *param); >> * @md_pm_lock: protects PCIe sleep lock >> * @sleep_disable_count: PCIe L1.2 lock counter >> * @sleep_lock_acquire: indicates that sleep has been disabled >> + * @trace: relayfs and debugfs data struct >> */ >> struct t7xx_pci_dev { >> t7xx_intr_callback intr_handler[EXT_INT_NUM]; >> @@ -78,6 +80,7 @@ struct t7xx_pci_dev { >> spinlock_t md_pm_lock; /* Protects PCI resource lock */ >> unsigned int sleep_disable_count; >> struct completion sleep_lock_acquire; >> + struct t7xx_trace trace; >> }; >> >> enum t7xx_pm_id { >> diff --git a/drivers/net/wwan/t7xx/t7xx_port.h >> b/drivers/net/wwan/t7xx/t7xx_port.h >> index 702e7aa2ef31..dc4133eb433a 100644 >> --- a/drivers/net/wwan/t7xx/t7xx_port.h >> +++ b/drivers/net/wwan/t7xx/t7xx_port.h >> @@ -122,11 +122,6 @@ struct t7xx_port { >> int rx_length_th; >> bool chan_enable; >> struct task_struct *thread; >> -#ifdef CONFIG_WWAN_DEBUGFS >> - void *relaych; >> - struct dentry *debugfs_dir; >> - struct dentry *debugfs_wwan_dir; >> -#endif >> }; >> >> struct sk_buff *t7xx_port_alloc_skb(int payload); >> diff --git a/drivers/net/wwan/t7xx/t7xx_port_proxy.c >> b/drivers/net/wwan/t7xx/t7xx_port_proxy.c >> index 894b1d11b2c9..3377573568c6 100644 >> --- a/drivers/net/wwan/t7xx/t7xx_port_proxy.c >> +++ b/drivers/net/wwan/t7xx/t7xx_port_proxy.c >> @@ -35,6 +35,7 @@ >> #include "t7xx_modem_ops.h" >> #include "t7xx_port.h" >> #include "t7xx_port_proxy.h" >> +#include "t7xx_port_trace.h" >> #include "t7xx_state_monitor.h" >> >> #define Q_IDX_CTRL 0 >> diff --git a/drivers/net/wwan/t7xx/t7xx_port_proxy.h >> b/drivers/net/wwan/t7xx/t7xx_port_proxy.h >> index 81d059fbc0fb..c537f5b5ff60 100644 >> --- a/drivers/net/wwan/t7xx/t7xx_port_proxy.h >> +++ b/drivers/net/wwan/t7xx/t7xx_port_proxy.h >> @@ -87,9 +87,6 @@ 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); >> >> +++ b/drivers/net/wwan/t7xx/t7xx_port_trace.h >> @@ -0,0 +1,21 @@ >> +// SPDX-License-Identifier: GPL-2.0-only >> +/* >> + * Copyright (C) 2022 Intel Corporation. >> + */ >> + >> +#ifndef __T7XX_PORT_TRACE_H__ >> +#define __T7XX_PORT_TRACE_H__ >> + >> +struct t7xx_trace { >> +#ifdef CONFIG_WWAN_DEBUGFS >> + void *relaych; >> + struct dentry *debugfs_dir; >> + struct dentry *debugfs_wwan_dir; >> +#endif >> +}; > > The relaych pointer is port specific, so it can be left in the port > structure. We do not need to keep the debugfs_dir pointer. It is only > needed during debugfs initialization to create the modem-specific > subdirectory. So it can be a local variable. There is only one > debugfs_wwan_dir pointer left, which can be placed somewhere in the > t7xx_modem structure. Ok. Will drop debugfs_dir and use local var instead. > >> +#ifdef CONFIG_WWAN_DEBUGFS >> +extern struct port_ops t7xx_trace_port_ops; >> +#endif >> + >> +#endif /* __T7XX_PORT_TRACE_H__ */ >> >> diff --git a/drivers/net/wwan/t7xx/t7xx_port_trace.c >> b/drivers/net/wwan/t7xx/t7xx_port_trace.c >> index 3f740db318a8..1f5224fb0e5d 100644 >> --- a/drivers/net/wwan/t7xx/t7xx_port_trace.c >> +++ b/drivers/net/wwan/t7xx/t7xx_port_trace.c >> @@ -10,6 +10,7 @@ >> >> #include "t7xx_port.h" >> #include "t7xx_port_proxy.h" >> +#include "t7xx_port_trace.h" >> #include "t7xx_state_monitor.h" >> >> #define T7XX_TRC_SUB_BUFF_SIZE 131072 >> @@ -51,19 +52,19 @@ static struct rchan_callbacks relay_callbacks = { >> >> static void t7xx_trace_port_uninit(struct t7xx_port *port) >> { >> - struct rchan *relaych = port->relaych; >> + struct t7xx_trace *trace = &port->t7xx_dev->trace; >> >> - if (!relaych) >> + if (!trace->relaych) >> return; >> >> - relay_close(relaych); >> - debugfs_remove_recursive(port->debugfs_dir); >> - wwan_put_debugfs_dir(port->debugfs_wwan_dir); >> + relay_close(trace->relaych); >> + debugfs_remove_recursive(trace->debugfs_dir); >> + wwan_put_debugfs_dir(trace->debugfs_wwan_dir); >> } >> >> static int t7xx_trace_port_recv_skb(struct t7xx_port *port, struct >> sk_buff *skb) >> { >> - struct rchan *relaych = port->relaych; >> + struct rchan *relaych = port->t7xx_dev->trace.relaych; >> >> if (!relaych) >> return -EINVAL; >> @@ -75,33 +76,34 @@ static int t7xx_trace_port_recv_skb(struct t7xx_port >> *port, struct sk_buff *skb) >> >> static void t7xx_port_trace_md_state_notify(struct t7xx_port *port, >> unsigned int state) >> { >> + struct t7xx_trace *trace = &port->t7xx_dev->trace; >> struct rchan *relaych; >> >> - if (state != MD_STATE_READY || port->relaych) >> + if (state != MD_STATE_READY || trace->relaych) >> return; >> >> - port->debugfs_wwan_dir = wwan_get_debugfs_dir(port->dev); >> - if (IS_ERR(port->debugfs_wwan_dir)) >> + trace->debugfs_wwan_dir = wwan_get_debugfs_dir(port->dev); >> + if (IS_ERR(trace->debugfs_wwan_dir)) >> return; >> >> - port->debugfs_dir = debugfs_create_dir(KBUILD_MODNAME, >> port->debugfs_wwan_dir); >> - if (IS_ERR_OR_NULL(port->debugfs_dir)) { >> - wwan_put_debugfs_dir(port->debugfs_wwan_dir); >> + trace->debugfs_dir = debugfs_create_dir(KBUILD_MODNAME, >> trace->debugfs_wwan_dir); >> + if (IS_ERR_OR_NULL(trace->debugfs_dir)) { >> + wwan_put_debugfs_dir(trace->debugfs_wwan_dir); >> dev_err(port->dev, "Unable to create debugfs for trace"); >> return; >> } >> >> - relaych = relay_open("relay_ch", port->debugfs_dir, >> T7XX_TRC_SUB_BUFF_SIZE, >> + relaych = relay_open("relay_ch", trace->debugfs_dir, >> T7XX_TRC_SUB_BUFF_SIZE, >> T7XX_TRC_N_SUB_BUFF, &relay_callbacks, NULL); >> if (!relaych) >> goto err_rm_debugfs_dir; >> >> - port->relaych = relaych; >> + trace->relaych = relaych; >> return; >> >> err_rm_debugfs_dir: >> - debugfs_remove_recursive(port->debugfs_dir); >> - wwan_put_debugfs_dir(port->debugfs_wwan_dir); >> + debugfs_remove_recursive(trace->debugfs_dir); >> + wwan_put_debugfs_dir(trace->debugfs_wwan_dir); >> dev_err(port->dev, "Unable to create trace port %s", >> port->port_conf->name); >> } >
On Tue, Oct 18, 2022 at 8:29 AM Kumar, M Chetan <m.chetan.kumar@linux.intel.com> wrote: > On 10/18/2022 5:29 AM, Sergey Ryazanov wrote: >> On Mon, Oct 17, 2022 at 1:16 PM Kumar, M Chetan wrote: >>> On 10/16/2022 9:35 PM, Sergey Ryazanov wrote: >>>>>> On Mon, Oct 3, 2022 at 8:29 AM <m.chetan.kumar@linux.intel.com> wrote: >>>>>>> The Modem Logging (MDL) port provides an interface to collect modem >>>>>>> logs for debugging purposes. MDL is supported by the relay interface, >>>>>>> and the mtk_t7xx port infrastructure. MDL allows user-space apps to >>>>>>> control logging via mbim command and to collect logs via the relay >>>>>>> interface, while port infrastructure facilitates communication between >>>>>>> the driver and the modem. >>>>>> >>>>>> [skip] >>>>>> >>>>>>> diff --git a/drivers/net/wwan/t7xx/t7xx_port.h b/drivers/net/wwan/t7xx/t7xx_port.h >>>>>>> index dc4133eb433a..702e7aa2ef31 100644 >>>>>>> --- a/drivers/net/wwan/t7xx/t7xx_port.h >>>>>>> +++ b/drivers/net/wwan/t7xx/t7xx_port.h >>>>>>> @@ -122,6 +122,11 @@ struct t7xx_port { >>>>>>> int rx_length_th; >>>>>>> bool chan_enable; >>>>>>> struct task_struct *thread; >>>>>>> +#ifdef CONFIG_WWAN_DEBUGFS >>>>>>> + void *relaych; >>>>>>> + struct dentry *debugfs_dir; >>>>>>> + struct dentry *debugfs_wwan_dir; >>>>>> >>>>>> Both of these debugfs directories are device-wide, why did you place >>>>>> these pointers in the item port context? >>> >>> I guess it was kept inside port so that it could be accessed directly >>> from port context. >>> >>> Thanks for pointing it out. I think we should move out the complete >>> #ifdef CONFIG_WWAN_DEBUGFS block of port container. >> >> Moving out debugfs directory pointers sounds like a good idea. >> Introduction of any new debugfs knob will be a much easier if these >> pointers are available in some device-wide state container. But the >> relaych pointer looks like a logging port specific element. Why should >> it be moved out? > > t7xx_port is a common port container. keeping relaych pointer inside > port container is like having relaych pointer for all port instance > (AT/MBIM) though it is not required for others. So planning to move out. > > You suggest to keep it or move out ? The t7xx_port structure already has a WWAN port specific field - wwan_port. We can group such specific and otherwise useless fields into a union. Like this: --- a/drivers/net/wwan/t7xx/t7xx_port.h +++ b/drivers/net/wwan/t7xx/t7xx_port.h @@ -99,7 +99,6 @@ struct t7xx_port_conf { struct t7xx_port { /* Members not initialized in definition */ const struct t7xx_port_conf *port_conf; - struct wwan_port *wwan_port; struct t7xx_pci_dev *t7xx_dev; struct device *dev; u16 seq_nums[2]; /* TX/RX sequence numbers */ @@ -122,6 +121,10 @@ struct t7xx_port { int rx_length_th; bool chan_enable; struct task_struct *thread; + union { /* Port type specific data */ + struct wwan_port *wwan_port; + struct rchan *relaych; + }; }; Or even like this: --- a/drivers/net/wwan/t7xx/t7xx_port.h +++ b/drivers/net/wwan/t7xx/t7xx_port.h @@ -99,7 +99,6 @@ struct t7xx_port_conf { struct t7xx_port { /* Members not initialized in definition */ const struct t7xx_port_conf *port_conf; - struct wwan_port *wwan_port; struct t7xx_pci_dev *t7xx_dev; struct device *dev; u16 seq_nums[2]; /* TX/RX sequence numbers */ @@ -122,6 +121,14 @@ struct t7xx_port { int rx_length_th; bool chan_enable; struct task_struct *thread; + union { /* Port type specific data */ + struct { + struct wwan_port *wwan_port; + } wwan; + struct { + struct rchan *relaych; + } log; + }; }; Or, if we want more isolation, we can define per port type structures and make the field in t7xx_port opaque: @@ -99,7 +99,6 @@ struct t7xx_port_conf { struct t7xx_port { /* Members not initialized in definition */ const struct t7xx_port_conf *port_conf; - struct wwan_port *wwan_port; struct t7xx_pci_dev *t7xx_dev; struct device *dev; u16 seq_nums[2]; /* TX/RX sequence numbers */ @@ -122,8 +121,23 @@ struct t7xx_port { int rx_length_th; bool chan_enable; struct task_struct *thread; + char priv[0x10]; /* Port type private data */ }; +#define t7xx_port_priv(__p) ((void *)&((__p)->priv)) + +struct t7xx_port_wwan { + struct wwan_port *wwan_port; +}; + +BUILD_BUG_ON(sizeof(struct t7xx_port_wwan) > sizeof(port->priv)); + +struct t7xx_port_log { + struct rchan *relaych; +}; + +BUILD_BUG_ON(sizeof(struct t7xx_port_log) > sizeof(port->priv)); I do not want to suggest any specific solution, it is always up to you how to develop your code. I just wanted to point out the unexpected pointers location and the possible difficulty of accessing the debugfs directory pointer in the future.
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 6ff30cb8eb16..aec3a18d44bd 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..702e7aa2ef31 100644 --- a/drivers/net/wwan/t7xx/t7xx_port.h +++ b/drivers/net/wwan/t7xx/t7xx_port.h @@ -122,6 +122,11 @@ struct t7xx_port { int rx_length_th; bool chan_enable; struct task_struct *thread; +#ifdef CONFIG_WWAN_DEBUGFS + void *relaych; + struct dentry *debugfs_dir; + struct dentry *debugfs_wwan_dir; +#endif }; struct sk_buff *t7xx_port_alloc_skb(int payload); diff --git a/drivers/net/wwan/t7xx/t7xx_port_proxy.c b/drivers/net/wwan/t7xx/t7xx_port_proxy.c index d4de047ff0d4..894b1d11b2c9 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, 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..3f740db318a8 --- /dev/null +++ b/drivers/net/wwan/t7xx/t7xx_port_trace.c @@ -0,0 +1,112 @@ +// SPDX-License-Identifier: GPL-2.0-only +/* + * Copyright (C) 2022 Intel Corporation. + */ + +#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 + +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 void t7xx_trace_port_uninit(struct t7xx_port *port) +{ + struct rchan *relaych = port->relaych; + + if (!relaych) + return; + + relay_close(relaych); + debugfs_remove_recursive(port->debugfs_dir); + wwan_put_debugfs_dir(port->debugfs_wwan_dir); +} + +static int t7xx_trace_port_recv_skb(struct t7xx_port *port, struct sk_buff *skb) +{ + struct rchan *relaych = port->relaych; + + if (!relaych) + return -EINVAL; + + relay_write(relaych, skb->data, skb->len); + dev_kfree_skb(skb); + return 0; +} + +static void t7xx_port_trace_md_state_notify(struct t7xx_port *port, unsigned int state) +{ + struct rchan *relaych; + + if (state != MD_STATE_READY || port->relaych) + return; + + port->debugfs_wwan_dir = wwan_get_debugfs_dir(port->dev); + if (IS_ERR(port->debugfs_wwan_dir)) + return; + + port->debugfs_dir = debugfs_create_dir(KBUILD_MODNAME, port->debugfs_wwan_dir); + if (IS_ERR_OR_NULL(port->debugfs_dir)) { + wwan_put_debugfs_dir(port->debugfs_wwan_dir); + dev_err(port->dev, "Unable to create debugfs for trace"); + return; + } + + relaych = relay_open("relay_ch", port->debugfs_dir, T7XX_TRC_SUB_BUFF_SIZE, + T7XX_TRC_N_SUB_BUFF, &relay_callbacks, NULL); + if (!relaych) + goto err_rm_debugfs_dir; + + port->relaych = relaych; + return; + +err_rm_debugfs_dir: + debugfs_remove_recursive(port->debugfs_dir); + wwan_put_debugfs_dir(port->debugfs_wwan_dir); + dev_err(port->dev, "Unable to create trace port %s", port->port_conf->name); +} + +struct port_ops t7xx_trace_port_ops = { + .recv_skb = t7xx_trace_port_recv_skb, + .uninit = t7xx_trace_port_uninit, + .md_state_notify = t7xx_port_trace_md_state_notify, +};