| 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
| ||
|
Date: Tue, 18 Oct 2022 15:13:40 +0400
From: Sergey Ryazanov <ryazanov.s.a@...il.com>
To: "Kumar, M Chetan" <m.chetan.kumar@...ux.intel.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 Tue, Oct 18, 2022 at 8:29 AM Kumar, M Chetan
<m.chetan.kumar@...ux.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@...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 ?
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.
--
Sergey
Powered by blists - more mailing lists