On 3/6/2022 6:52 PM, Sergey Ryazanov wrote:
On Thu, Feb 24, 2022 at 1:35 AM Ricardo Martinez
<ricardo.martinez@xxxxxxxxxxxxxxx> wrote:
From: Haijun Liu <haijun.liu@xxxxxxxxxxxx>
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]
...
+/* Channel ID and Message ID definitions.
+ * The channel number consists of peer_id(15:12) , channel_id(11:0)
+ * peer_id:
+ * 0:reserved, 1: to sAP, 2: to MD
+ */
+enum port_ch {
+ /* to MD */
+ PORT_CH_CONTROL_RX = 0x2000,
+ PORT_CH_CONTROL_TX = 0x2001,
+ PORT_CH_UART1_RX = 0x2006, /* META */
+ PORT_CH_UART1_TX = 0x2008,
+ PORT_CH_UART2_RX = 0x200a, /* AT */
+ PORT_CH_UART2_TX = 0x200c,
+ PORT_CH_MD_LOG_RX = 0x202a, /* MD logging */
+ PORT_CH_MD_LOG_TX = 0x202b,
+ PORT_CH_LB_IT_RX = 0x203e, /* Loop back test */
+ PORT_CH_LB_IT_TX = 0x203f,
+ PORT_CH_STATUS_RX = 0x2043, /* Status polling */
There is no STATUS_TX channel, so how is the polling performed? Is it
performed through the CONTROL_TX channel? Or should the comment be
changed to "status events"?
Currently there's no port listening to this channel, the suggested
comment would be more accurate.
+ PORT_CH_MIPC_RX = 0x20ce, /* MIPC */
+ PORT_CH_MIPC_TX = 0x20cf,
+ PORT_CH_MBIM_RX = 0x20d0,
+ PORT_CH_MBIM_TX = 0x20d1,
+ PORT_CH_DSS0_RX = 0x20d2,
+ PORT_CH_DSS0_TX = 0x20d3,
+ PORT_CH_DSS1_RX = 0x20d4,
+ PORT_CH_DSS1_TX = 0x20d5,
+ PORT_CH_DSS2_RX = 0x20d6,
+ PORT_CH_DSS2_TX = 0x20d7,
+ PORT_CH_DSS3_RX = 0x20d8,
+ PORT_CH_DSS3_TX = 0x20d9,
+ PORT_CH_DSS4_RX = 0x20da,
+ PORT_CH_DSS4_TX = 0x20db,
+ PORT_CH_DSS5_RX = 0x20dc,
+ PORT_CH_DSS5_TX = 0x20dd,
+ PORT_CH_DSS6_RX = 0x20de,
+ PORT_CH_DSS6_TX = 0x20df,
+ PORT_CH_DSS7_RX = 0x20e0,
+ PORT_CH_DSS7_TX = 0x20e1,
+};
+
...
+
+struct t7xx_port_static {
...
+int t7xx_port_recv_skb(struct t7xx_port *port, struct sk_buff *skb)
+{
+ struct ccci_header *ccci_h;
+ unsigned long flags;
+ u32 channel;
+ int ret = 0;
+
+ spin_lock_irqsave(&port->rx_wq.lock, flags);
+ if (port->rx_skb_list.qlen >= port->rx_length_th) {
+ port->flags |= PORT_F_RX_FULLED;
+ spin_unlock_irqrestore(&port->rx_wq.lock, flags);
+
+ return -ENOBUFS;
+ }
+ ccci_h = (struct ccci_header *)skb->data;
+ port->flags &= ~PORT_F_RX_FULLED;
+ if (port->flags & PORT_F_RX_ADJUST_HEADER)
+ t7xx_port_adjust_skb(port, skb);
+ channel = FIELD_GET(CCCI_H_CHN_FLD, le32_to_cpu(ccci_h->status));
+ if (channel == PORT_CH_STATUS_RX) {
+ ret = port->skb_handler(port, skb);
This handler will never be called. A message with channel =
PORT_CH_STATUS_RX will be dropped in the t7xx_port_proxy_recv_skb()
function, since the corresponding port is nonexistent.
+ } else {
+ if (port->wwan_port)
+ wwan_port_rx(port->wwan_port, skb);
+ else
+ __skb_queue_tail(&port->rx_skb_list, skb);
+ }
+ spin_unlock_irqrestore(&port->rx_wq.lock, flags);
+
+ wake_up_all(&port->rx_wq);
+ return ret;
+}
Whole this function looks like a big unintentional duct tape. On the
one hand, each port type has a specific recv_skb callback. But in
fact, all message processing paths pass through this place. And here
the single function forced to take into account the specialties of
each port type:
a) immediately passes status events to the handler via the indirect call;
b) enqueues control messages to the rx queue;
c) directly passes WWAN management (MBIM & AT) message to the WWAN subsystem.
I would like to suggest the following reworking plan for the function:
1) move the common processing code (header stripping code) to the
t7xx_port_proxy_recv_skb() function, where it belongs;
2) add a dedicated port ops for the PORT_CH_STATUS_RX channel and call
control_msg_handler() from its recv_skb callback (lets call it
t7xx_port_status_recv_skb()); this will solve both issues: status
messages will no more dropped and status message hook will be removed;
3) move the wwan_port_rx() call to the t7xx_port_wwan_recv_skb()
callback; this will remove another one hook;
4) finally rename t7xx_port_recv_skb() to t7xx_port_enqueue_skb(),
since after the hooks removing, the only purpose of this function will
be to enqueue received skb(s).
Thanks for the suggestions.
After the changes this function will just figure out the channel by
reading the CCCI header and invoke the corresponding port's recv_skb().
I do not think we want to remove the CCCI header yet since recv_skb()
may fail and the caller might decide to try again later.
The generic t7xx_port_enqueue_skb() function will remove the CCCI header
before enqueuing the skb, t7xx_port_wwan_recv_skb() should do the same
before calling wwan_port_rx().
...
+{
+ struct t7xx_port_static *port_static = port->port_static;
+ struct t7xx_fsm_ctl *ctl = port->t7xx_dev->md->fsm_ctl;
+ struct cldma_ctrl *md_ctrl;
+ enum md_state md_state;
+ unsigned int fsm_state;
+
+ md_state = t7xx_fsm_get_md_state(ctl);
+
+ fsm_state = t7xx_fsm_get_ctl_state(ctl);
+ if (fsm_state != FSM_STATE_PRE_START) {
+ if (md_state == MD_STATE_WAITING_FOR_HS1 || md_state == MD_STATE_WAITING_FOR_HS2)
+ return -ENODEV;
+
+ if (md_state == MD_STATE_EXCEPTION && port_static->tx_ch != PORT_CH_MD_LOG_TX &&
+ port_static->tx_ch != PORT_CH_UART1_TX)
There are no ports defined for PORT_CH_MD_LOG_TX and PORT_CH_UART1_TX
channels, should this check be removed?
PORT_CH_UART1_TX should be removed, but PORT_CH_MD_LOG_TX is going to be used by the upcoming modem logging port feature.
...
