| lists.openwall.net | lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC | |
|
Open Source and information security mailing list archives
| ||
|
Message-ID: <eb68e77a-518e-3b3d-4c63-2a06390cce43@linux.intel.com>
Date: Tue, 18 Oct 2022 09:59:46 +0530
From: "Kumar, M Chetan" <m.chetan.kumar@...ux.intel.com>
To: Sergey Ryazanov <ryazanov.s.a@...il.com>
Cc: netdev@...r.kernel.org, kuba@...nel.org, davem@...emloft.net,
johannes@...solutions.net, loic.poulain@...aro.org,
krishna.c.sudi@...el.com, linuxwwan@...el.com,
Moises Veleta <moises.veleta@...ux.intel.com>,
Devegowda Chandrashekar <chandrashekar.devegowda@...el.com>,
Ricardo Martinez <ricardo.martinez@...ux.intel.com>
Subject: Re: [PATCH V3 net-next] net: wwan: t7xx: Add port for modem logging
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@...ux.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@...ux.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);
>> }
>
--
Chetan
Powered by blists - more mailing lists