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 PHC | |
Open Source and information security mailing list archives
| ||
|
Date: Fri, 11 Mar 2022 19:45:22 -0800 From: "Martinez, Ricardo" <ricardo.martinez@...ux.intel.com> To: Sergey Ryazanov <ryazanov.s.a@...il.com> Cc: netdev@...r.kernel.org, linux-wireless@...r.kernel.org, Jakub Kicinski <kuba@...nel.org>, David Miller <davem@...emloft.net>, Johannes Berg <johannes@...solutions.net>, Loic Poulain <loic.poulain@...aro.org>, M Chetan Kumar <m.chetan.kumar@...el.com>, chandrashekar.devegowda@...el.com, Intel Corporation <linuxwwan@...el.com>, chiranjeevi.rapolu@...ux.intel.com, Haijun Liu (刘海军) <haijun.liu@...iatek.com>, amir.hanania@...el.com, Andy Shevchenko <andriy.shevchenko@...ux.intel.com>, dinesh.sharma@...el.com, eliot.lee@...el.com, ilpo.johannes.jarvinen@...el.com, moises.veleta@...el.com, pierre-louis.bossart@...el.com, muralidharan.sethuraman@...el.com, Soumya.Prakash.Mishra@...el.com, sreehari.kancharla@...el.com, madhusmita.sahu@...el.com Subject: Re: [PATCH net-next v5 04/13] net: wwan: t7xx: Add port proxy infrastructure On 3/6/2022 6:52 PM, Sergey Ryazanov wrote: > On Thu, Feb 24, 2022 at 1:35 AM Ricardo Martinez > <ricardo.martinez@...ux.intel.com> wrote: >> From: Haijun Liu <haijun.liu@...iatek.com> >> >> 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. ...
Powered by blists - more mailing lists