On Mon, Nov 1, 2021 at 6:57 AM Ricardo Martinez
[off-list ref] wrote:

Port-proxy provides a common interface to interact with different types
of ports. Ports export their configuration via `struct t7xx_port` and
operate as defined by `struct port_ops`.

[skipped]

As already suggested by Andy, use C99 initializers to initialize the
md_ccci_ports array and drop the above comment about the strict order
requirements.

+       enum ccci_ch            tx_ch;
+       enum ccci_ch            rx_ch;
+       unsigned char           txq_index;
+       unsigned char           rxq_index;
+       unsigned char           txq_exp_index;
+       unsigned char           rxq_exp_index;
+       enum cldma_id           path_id;
+       unsigned int            flags;
+       struct port_ops         *ops;
+       unsigned int            minor;
+       char                    *name;

Why did you need these two fields with the port name and minor number?
The WWAN subsystem will care about these data for you. It is its
purpose.

+       enum wwan_port_type     mtk_port_type;
+
+       /* members un-initialized in definition */
+       struct wwan_port        *mtk_wwan_port;
+       struct mtk_pci_dev      *mtk_dev;
+       struct device           *dev;
+       short                   seq_nums[2];
+       struct port_proxy       *port_proxy;
+       atomic_t                usage_cnt;
+       struct                  list_head entry;
+       struct                  list_head queue_entry;
+       unsigned int            major;
+       unsigned int            minor_base;
+       /* TX and RX flows are asymmetric since ports are multiplexed on
+        * queues.
+        *
+        * TX: data blocks are sent directly to a queue. Each port
+        * does not maintain a TX list; instead, they only provide
+        * a wait_queue_head for blocking writes.
+        *
+        * RX: Each port uses a RX list to hold packets,
+        * allowing the modem to dispatch RX packet as quickly as possible.
+        */
+       struct sk_buff_head     rx_skb_list;
+       bool                    skb_from_pool;
+       spinlock_t              port_update_lock; /* protects port configuration */
+       wait_queue_head_t       rx_wq;
+       int                     rx_length_th;
+       port_skb_handler        skb_handler;
+       unsigned char           chan_enable;
+       unsigned char           chn_crt_stat;
+       struct cdev             *cdev;
+       struct task_struct      *thread;
+       struct mutex            tx_mutex_lock; /* protects the seq number operation */
+};

You should split the t7xx_port structure at least for two parts. A
first part with static configuration can remain in the structure and
statically initialized in the md_ccci_ports array. All non-shareable
runtime state fields (e.g. SKB lists, pointers to dynamically
allocated device instance structures) should be moved to a device
state container.

[skipped]

There is a clear statement in the include/uapi/linux/netlink.h file
that NETLINK_USERSOCK is reserved for user mode socket protocols.
Please do not abuse netlink protocol numbers.

If you really need a special Netlink interface to communicate with
userspace, consider creating a new generic netlink family. But it
looks like all the Netlink stuff here is a leftover of a debug
interface that was used at an earlier driver development stage. So I
suggest to just remove all Netlink usage here and consider using
dynamic debug logging, or switch to the kernel tracing.

...
+static struct port_proxy *port_prox;

This is another one pointer that should be placed into a device
runtime state structure to avoid driver crash with multiple modems.

...
+static struct port_ops dummy_port_ops;

Why do you need this dummy ops structure? You anyway remove it in the
next patch. If you need it as a placeholder for the md_ccci_ports
array below, then it is safe to define an empty md_ccci_ports array
and then just fill it. Please consider removing this structure to
avoid ping-pong changes.

+static struct t7xx_port md_ccci_ports[] = {
+       {0, 0, 0, 0, 0, 0, ID_CLDMA1, 0, &dummy_port_ops, 0xff, "dummy_port",},

As already suggested by Andy, use C99 initializers here. E.g.

        {
                .tx_ch = 0,
                .rx_ch = 0,
                .txq_index = 0,
                .rxq_index = 0,
                .txq_exp_index = 0,
                .rxq_exp_index = 0,
                .path_id = ID_CLDMA1,
                .ops = &dummy_port_ops,
                .name = "dummy_port",
        }, {
               ...
       }, {
               ...
       }

...
+/* Sequence numbering to track for lost packets */
+void port_proxy_set_seq_num(struct t7xx_port *port, struct ccci_header *ccci_h)
+{
+       if (ccci_h && port) {
+               ccci_h->status &= ~HDR_FLD_SEQ;
+               ccci_h->status |= FIELD_PREP(HDR_FLD_SEQ, port->seq_nums[MTK_OUT]);
+               ccci_h->status &= ~HDR_FLD_AST;
+               ccci_h->status |= FIELD_PREP(HDR_FLD_AST, 1);
+       }

Endians handling required here.

+}
+
+static u16 port_check_rx_seq_num(struct t7xx_port *port, struct ccci_header *ccci_h)
+{
+       u16 channel, seq_num, assert_bit;
+
+       channel = FIELD_GET(HDR_FLD_CHN, ccci_h->status);
+       seq_num = FIELD_GET(HDR_FLD_SEQ, ccci_h->status);
+       assert_bit = FIELD_GET(HDR_FLD_AST, ccci_h->status);

Field endians handling is missed here. Probably you should first
convert status field data to CPU endians and only then parse it. E.g.

        u32 status = le32_to_cpu(ccci_h->status);

        channel = FIELD_GET(HDR_FLD_CHN, status);
        seq_num = FIELD_GET(HDR_FLD_SEQ, status);
        assert_bit = FIELD_GET(HDR_FLD_AST, status);

+       if (assert_bit && port->seq_nums[MTK_IN] &&
+           ((seq_num - port->seq_nums[MTK_IN]) & CHECK_RX_SEQ_MASK) != 1) {
+               dev_err(port->dev, "channel %d seq number out-of-order %d->%d (data: %X, %X)\n",
+                       channel, seq_num, port->seq_nums[MTK_IN],
+                       ccci_h->data[0], ccci_h->data[1]);

dev_err_ratelimited() ?

+       }
+
+       return seq_num;
+}
...
+int port_recv_skb(struct t7xx_port *port, struct sk_buff *skb)
+{
...
+       if (port->flags & PORT_F_RX_ALLOW_DROP) {
+               dev_err(port->dev, "port %s RX full, drop packet\n", port->name);

Should the ratelimited variant be used here? And why is so high
message level used?

+               return -ENETDOWN;

Why ENETDOWN on buffer space exhaustion?

+       }
+
+       return -ENOBUFS;
+}
...
+int port_proxy_send_skb(struct t7xx_port *port, struct sk_buff *skb, bool from_pool)
+{
+       struct ccci_header *ccci_h;
...
+       ccci_h = (struct ccci_header *)(skb->data);
...
+       port_proxy_set_seq_num(port, (struct ccci_header *)ccci_h);

cchi_h is already of type ccci_header, no casting is required here.

...
+
+/* inject CCCI message to modem */
+void port_proxy_send_msg_to_md(int ch, unsigned int msg, unsigned int resv)

This function is not called by any code in this patch. Should the
function be moved to the "net: wwan: t7xx: Add control port" patch
along with the ctrl_msg_header structure definition?

+{
+       struct ctrl_msg_header *ctrl_msg_h;
+       struct ccci_header *ccci_h;
+       struct t7xx_port *port;
+       struct sk_buff *skb;
+       int ret;
+
+       port = port_get_by_ch(ch);
+       if (!port)
+               return;
+
+       skb = ccci_alloc_skb_from_pool(&port->mtk_dev->pools, sizeof(struct ccci_header),
+                                      GFS_BLOCKING);
+       if (!skb)
+               return;
+
+       if (ch == CCCI_CONTROL_TX) {
+               ccci_h = (struct ccci_header *)(skb->data);
+               ccci_h->data[0] = CCCI_HEADER_NO_DATA;
+               ccci_h->data[1] = sizeof(struct ctrl_msg_header) + CCCI_H_LEN;
+               ccci_h->status &= ~HDR_FLD_CHN;
+               ccci_h->status |= FIELD_PREP(HDR_FLD_CHN, ch);
+               ccci_h->reserved = 0;
+               ctrl_msg_h = (struct ctrl_msg_header *)(skb->data + CCCI_H_LEN);
+               ctrl_msg_h->data_length = 0;
+               ctrl_msg_h->reserved = resv;
+               ctrl_msg_h->ctrl_msg_id = msg;
+               skb_put(skb, CCCI_H_LEN + sizeof(struct ctrl_msg_header));
+       } else {
+               ccci_h = skb_put(skb, sizeof(struct ccci_header));
+               ccci_h->data[0] = CCCI_HEADER_NO_DATA;
+               ccci_h->data[1] = msg;
+               ccci_h->status &= ~HDR_FLD_CHN;
+               ccci_h->status |= FIELD_PREP(HDR_FLD_CHN, ch);
+               ccci_h->reserved = resv;
+       }

Endians handling missed here as well.

+       ret = port_proxy_send_skb(port, skb, port->skb_from_pool);
+       if (ret) {
+               dev_err(port->dev, "port%s send to MD fail\n", port->name);
+               ccci_free_skb(&port->mtk_dev->pools, skb);
+       }
+}
...
+static int proxy_register_char_dev(void)
+{
+       dev_t dev = 0;
+       int ret;
+
+       if (port_prox->major) {
+               dev = MKDEV(port_prox->major, port_prox->minor_base);
+               ret = register_chrdev_region(dev, TTY_IPC_MINOR_BASE, MTK_DEV_NAME);
+       } else {
+               ret = alloc_chrdev_region(&dev, port_prox->minor_base,
+                                         TTY_IPC_MINOR_BASE, MTK_DEV_NAME);
+               if (ret)
+                       dev_err(port_prox->dev, "failed to alloc chrdev region, ret=%d\n", ret);
+
+               port_prox->major = MAJOR(dev);
+       }

For what do you need these character devices? The WWAN subsystem
already handle all these tasks.

+       return ret;
+}
...
+static int proxy_alloc(struct mtk_modem *md)
+{
+       int ret;
+
+       port_prox = devm_kzalloc(&md->mtk_dev->pdev->dev, sizeof(*port_prox), GFP_KERNEL);
+       if (!port_prox)
+               return -ENOMEM;

This pointer should be placed into the mtk_modem, not to a global variable.

...
+int port_proxy_broadcast_state(struct t7xx_port *port, int state)
+{
+       char msg[PORT_NETLINK_MSG_MAX_PAYLOAD];
+
+       if (state >= MTK_PORT_STATE_INVALID)
+               return -EINVAL;
+
+       switch (state) {
+       case MTK_PORT_STATE_ENABLE:
+               snprintf(msg, PORT_NETLINK_MSG_MAX_PAYLOAD, "enable %s", port->name);
+               break;
+
+       case MTK_PORT_STATE_DISABLE:
+               snprintf(msg, PORT_NETLINK_MSG_MAX_PAYLOAD, "disable %s", port->name);
+               break;
+
+       default:
+               snprintf(msg, PORT_NETLINK_MSG_MAX_PAYLOAD, "invalid operation");
+               break;
+       }
+
+       return port_netlink_send_msg(port, PORT_STATE_BROADCAST_GROUP, msg, strlen(msg) + 1);

Netlink by nature is a binary protocol. You need to emit messages of
different types per port state with a port name attribute inside. Or
emit a single type message with two separate attributes: one to carry
a port state and a separate attribute to carry a port name.

Or, as I suggested above, just drop this Netlink abuse and switch to
dynamic debug logging. Or even better, consider switching to the
kernel tracing API.

[skipped]

All three of these fields should be of type __be32, since the
structure is passed to the modem as is.

+       u8                      data[0];
+};
+
+struct port_msg {
+       u32                     head_pattern;
+       u32                     info;
+       u32                     tail_pattern;

Same here.

+       u8                      data[0]; /* port set info */
+};

[skipped]

Looks like this change does not belong to this patch. The "ttyCMdLog"
port entry will be created only by the last patch.

--
Sergey