Re: [PATCH V3 net-next] net: wwan: t7xx: Add port for modem logging

[Sergey Ryazanov <ryazanov.s.a@gmail.com>]

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.

-- 
Sergey