| Message ID | 20220223223326.28021-1-ricardo.martinez@linux.intel.com |
|---|---|
| Headers | show |
| Series | net: wwan: t7xx: PCIe driver for MediaTek M.2 modem | expand |
From: Haijun Liu <haijun.liu@mediatek.com> Registers the t7xx device driver with the kernel. Setup all the core components: PCIe layer, Modem Host Cross Core Interface (MHCCIF), modem control operations, modem state machine, and build infrastructure. * PCIe layer code implements driver probe and removal. * MHCCIF provides interrupt channels to communicate events such as handshake, PM and port enumeration. * Modem control implements the entry point for modem init, reset and exit. * The modem status monitor is a state machine used by modem control to complete initialization and stop. It is used also to propagate exception events reported by other components. Signed-off-by: Haijun Liu <haijun.liu@mediatek.com> Signed-off-by: Chandrashekar Devegowda <chandrashekar.devegowda@intel.com> Co-developed-by: Ricardo Martinez <ricardo.martinez@linux.intel.com> Signed-off-by: Ricardo Martinez <ricardo.martinez@linux.intel.com>
From: Haijun Liu <haijun.liu@mediatek.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`. Signed-off-by: Haijun Liu <haijun.liu@mediatek.com> Co-developed-by: Chandrashekar Devegowda <chandrashekar.devegowda@intel.com> Signed-off-by: Chandrashekar Devegowda <chandrashekar.devegowda@intel.com> Co-developed-by: Ricardo Martinez <ricardo.martinez@linux.intel.com> Signed-off-by: Ricardo Martinez <ricardo.martinez@linux.intel.com>
From: Chandrashekar Devegowda <chandrashekar.devegowda@intel.com> Adds AT and MBIM ports to the port proxy infrastructure. The initialization method is responsible for creating the corresponding ports using the WWAN framework infrastructure. The implemented WWAN port operations are start, stop, and TX. Signed-off-by: Chandrashekar Devegowda <chandrashekar.devegowda@intel.com> Co-developed-by: Ricardo Martinez <ricardo.martinez@linux.intel.com> Signed-off-by: Ricardo Martinez <ricardo.martinez@linux.intel.com>
From: Haijun Liu <haijun.liu@mediatek.com> Data Path Modem AP Interface (DPMAIF) HW layer provides HW abstraction for the upper layer (DPMAIF HIF). It implements functions to do the HW configuration, TX/RX control and interrupt handling. Signed-off-by: Haijun Liu <haijun.liu@mediatek.com> Signed-off-by: Chandrashekar Devegowda <chandrashekar.devegowda@intel.com> Co-developed-by: Ricardo Martinez <ricardo.martinez@linux.intel.com> Signed-off-by: Ricardo Martinez <ricardo.martinez@linux.intel.com>
From: Haijun Liu <haijun.liu@mediatek.com> Data Path Modem AP Interface (DPMAIF) HIF layer provides methods for initialization, ISR, control and event handling of TX/RX flows. DPMAIF TX Exposes the `dmpaif_tx_send_skb` function which can be used by the network device to transmit packets. The uplink data management uses a Descriptor Ring Buffer (DRB). First DRB entry is a message type that will be followed by 1 or more normal DRB entries. Message type DRB will hold the skb information and each normal DRB entry holds a pointer to the skb payload. DPMAIF RX The downlink buffer management uses Buffer Address Table (BAT) and Packet Information Table (PIT) rings. The BAT ring holds the address of skb data buffer for the HW to use, while the PIT contains metadata about a whole network packet including a reference to the BAT entry holding the data buffer address. The driver reads the PIT and BAT entries written by the modem, when reaching a threshold, the driver will reload the PIT and BAT rings. Signed-off-by: Haijun Liu <haijun.liu@mediatek.com> Signed-off-by: Chandrashekar Devegowda <chandrashekar.devegowda@intel.com> Co-developed-by: Ricardo Martinez <ricardo.martinez@linux.intel.com> Signed-off-by: Ricardo Martinez <ricardo.martinez@linux.intel.com>
On Wed, 23 Feb 2022, Ricardo Martinez wrote: > From: Haijun Liu <haijun.liu@mediatek.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`. > > Signed-off-by: Haijun Liu <haijun.liu@mediatek.com> > Co-developed-by: Chandrashekar Devegowda <chandrashekar.devegowda@intel.com> > Signed-off-by: Chandrashekar Devegowda <chandrashekar.devegowda@intel.com> > Co-developed-by: Ricardo Martinez <ricardo.martinez@linux.intel.com> > Signed-off-by: Ricardo Martinez <ricardo.martinez@linux.intel.com> > > >From a WWAN framework perspective: > Reviewed-by: Loic Poulain <loic.poulain@linaro.org> > --- > +static u16 t7xx_port_next_rx_seq_num(struct t7xx_port *port, struct ccci_header *ccci_h) > +{ > + u16 seq_num, next_seq_num, assert_bit; assert_bit could be bool. > + > + seq_num = FIELD_GET(CCCI_H_SEQ_FLD, le32_to_cpu(ccci_h->status)); > + next_seq_num = (seq_num + 1) & FIELD_MAX(CCCI_H_SEQ_FLD); > + assert_bit = !!(le32_to_cpu(ccci_h->status) & CCCI_H_AST_BIT); > + if (!assert_bit || port->seq_nums[MTK_RX] > FIELD_MAX(CCCI_H_SEQ_FLD)) Is this an obfuscated way to say: ... || port->seq_nums[MTK_RX] == INVALID_SEQ_NUM ? > +int t7xx_port_proxy_node_control(struct t7xx_modem *md, struct port_msg *port_msg) > +{ > + u32 *port_info_base = (void *)port_msg + sizeof(*port_msg); __le32 *port_info_base = (void *)port_msg + sizeof(*port_msg); As port_msg has __le32 fields, the endianess likely should disappear in this casting? > + struct device *dev = &md->t7xx_dev->pdev->dev; > + unsigned int version, ports, i; > + > + version = FIELD_GET(PORT_MSG_VERSION, le32_to_cpu(port_msg->info)); > + if (version != PORT_ENUM_VER || > + le32_to_cpu(port_msg->head_pattern) != PORT_ENUM_HEAD_PATTERN || > + le32_to_cpu(port_msg->tail_pattern) != PORT_ENUM_TAIL_PATTERN) { > + dev_err(dev, "Invalid port control message %x:%x:%x\n", > + version, le32_to_cpu(port_msg->head_pattern), > + le32_to_cpu(port_msg->tail_pattern)); > + return -EFAULT; > + } > + > + ports = FIELD_GET(PORT_MSG_PRT_CNT, le32_to_cpu(port_msg->info)); > + for (i = 0; i < ports; i++) { > + struct t7xx_port_static *port_static; > + u32 *port_info = port_info_base + i; __le32 *port_info = port_info_base + i; > + struct t7xx_port *port; > + unsigned int ch_id; > + bool en_flag; > + > + ch_id = FIELD_GET(PORT_INFO_CH_ID, *port_info); ch_id = FIELD_GET(PORT_INFO_CH_ID, le32_to_cpu(*port_info)); > + port = t7xx_proxy_get_port_by_ch(md->port_prox, ch_id); > + if (!port) { > + dev_dbg(dev, "Port:%x not found\n", ch_id); > + continue; > + } > + > + en_flag = !!(PORT_INFO_ENFLG & *port_info); *port_info & PORT_INFO_ENFLG
On Wed, 23 Feb 2022, Ricardo Martinez wrote: > From: Chandrashekar Devegowda <chandrashekar.devegowda@intel.com> > > Adds AT and MBIM ports to the port proxy infrastructure. > The initialization method is responsible for creating the corresponding > ports using the WWAN framework infrastructure. The implemented WWAN port > operations are start, stop, and TX. > > Signed-off-by: Chandrashekar Devegowda <chandrashekar.devegowda@intel.com> > Co-developed-by: Ricardo Martinez <ricardo.martinez@linux.intel.com> > Signed-off-by: Ricardo Martinez <ricardo.martinez@linux.intel.com> > > >From a WWAN framework perspective: > Reviewed-by: Loic Poulain <loic.poulain@linaro.org> > --- > +static int t7xx_port_ctrl_tx(struct wwan_port *port, struct sk_buff *skb) > +{ ... > + if (!len || !port_private->rx_length_th || !port_private->chan_enable) It raises eyebrows to see rx_xx being used on something called "tx". Is it trying to tests port not inited?
On Wed, 23 Feb 2022, Ricardo Martinez wrote: > From: Haijun Liu <haijun.liu@mediatek.com> > > Data Path Modem AP Interface (DPMAIF) HW layer provides HW abstraction > for the upper layer (DPMAIF HIF). It implements functions to do the HW > configuration, TX/RX control and interrupt handling. > > Signed-off-by: Haijun Liu <haijun.liu@mediatek.com> > Signed-off-by: Chandrashekar Devegowda <chandrashekar.devegowda@intel.com> > Co-developed-by: Ricardo Martinez <ricardo.martinez@linux.intel.com> > Signed-off-by: Ricardo Martinez <ricardo.martinez@linux.intel.com> > > >From a WWAN framework perspective: > Reviewed-by: Loic Poulain <loic.poulain@linaro.org> > --- > +static void t7xx_dpmaif_dl_set_ao_bat_rsv_length(struct dpmaif_hw_info *hw_info) > +{ > + unsigned int value; > + > + value = ioread32(hw_info->pcie_base + DPMAIF_AO_DL_PKTINFO_CON2); > + value &= ~DPMAIF_BAT_RSV_LEN_MSK; > + value |= DPMAIF_HW_BAT_RSVLEN & DPMAIF_BAT_RSV_LEN_MSK; Drop & DPMAIF_BAT_RSV_LEN_MSK > +static void t7xx_dpmaif_dl_set_ao_frg_check_thres(struct dpmaif_hw_info *hw_info) > +{ > + unsigned int value; > + > + value = ioread32(hw_info->pcie_base + DPMAIF_AO_DL_RDY_CHK_FRG_THRES); > + value &= ~DPMAIF_FRG_CHECK_THRES_MSK; > + value |= (DPMAIF_HW_CHK_FRG_NUM & DPMAIF_FRG_CHECK_THRES_MSK); Ditto. > + value |= DPMAIF_DL_PIT_SEQ_VALUE & DPMAIF_DL_PIT_SEQ_MSK; Ditto.
On Wed, 23 Feb 2022, Ricardo Martinez wrote: > From: Haijun Liu <haijun.liu@mediatek.com> > > Data Path Modem AP Interface (DPMAIF) HIF layer provides methods > for initialization, ISR, control and event handling of TX/RX flows. > > DPMAIF TX > Exposes the `dmpaif_tx_send_skb` function which can be used by the > network device to transmit packets. > The uplink data management uses a Descriptor Ring Buffer (DRB). > First DRB entry is a message type that will be followed by 1 or more > normal DRB entries. Message type DRB will hold the skb information > and each normal DRB entry holds a pointer to the skb payload. > > DPMAIF RX > The downlink buffer management uses Buffer Address Table (BAT) and > Packet Information Table (PIT) rings. > The BAT ring holds the address of skb data buffer for the HW to use, > while the PIT contains metadata about a whole network packet including > a reference to the BAT entry holding the data buffer address. > The driver reads the PIT and BAT entries written by the modem, when > reaching a threshold, the driver will reload the PIT and BAT rings. > > Signed-off-by: Haijun Liu <haijun.liu@mediatek.com> > Signed-off-by: Chandrashekar Devegowda <chandrashekar.devegowda@intel.com> > Co-developed-by: Ricardo Martinez <ricardo.martinez@linux.intel.com> > Signed-off-by: Ricardo Martinez <ricardo.martinez@linux.intel.com> > > >From a WWAN framework perspective: > Reviewed-by: Loic Poulain <loic.poulain@linaro.org> > --- > +static int t7xx_dpmaif_update_bat_wr_idx(struct dpmaif_ctrl *dpmaif_ctrl, > + const unsigned char q_num, const unsigned int bat_cnt) > +{ > + struct dpmaif_rx_queue *rxq = &dpmaif_ctrl->rxq[q_num]; > + unsigned short old_rl_idx, new_wr_idx, old_wr_idx; unsigned int. I thought you listed a change for these idx local variables but there were still many unsigned shorts. Please check the whole change. > +static int t7xx_dpmaif_rx_data_collect(struct dpmaif_ctrl *dpmaif_ctrl, > + const unsigned char q_num, const unsigned int budget) > +{ > + struct dpmaif_rx_queue *rxq = &dpmaif_ctrl->rxq[q_num]; > + unsigned long time_limit; > + unsigned int cnt; > + > + time_limit = jiffies + msecs_to_jiffies(DPMAIF_WQ_TIME_LIMIT_MS); > + > + while ((cnt = t7xx_dpmaifq_poll_pit(rxq))) { > + unsigned int rd_cnt; > + int real_cnt; > + > + rd_cnt = min_t(unsigned int, cnt, budget); This can be min(cnt, budget); because they're now both unsigned ints. min_t is only needed if the args are of different type. > + t7xx_dpmaif_ul_update_hw_drb_cnt(&dpmaif_ctrl->hw_info, txq->index, > + drb_send_cnt * DPMAIF_UL_DRB_SIZE_WORD); This is the only callsite for t7xx_dpmaif_ul_update_hw_drb_cnt that returns int (in 07). Change it to void? > + /* Wait for active Tx to be doneg */ doneg -> done.
On Wed, 23 Feb 2022, Ricardo Martinez wrote: > From: Haijun Liu <haijun.liu@mediatek.com> > > Implements suspend, resumes, freeze, thaw, poweroff, and restore > `dev_pm_ops` callbacks. > > >From the host point of view, the t7xx driver is one entity. But, the > device has several modules that need to be addressed in different ways > during power management (PM) flows. > The driver uses the term 'PM entities' to refer to the 2 DPMA and > 2 CLDMA HW blocks that need to be managed during PM flows. > When a dev_pm_ops function is called, the PM entities list is iterated > and the matching function is called for each entry in the list. > > Signed-off-by: Haijun Liu <haijun.liu@mediatek.com> > Signed-off-by: Chandrashekar Devegowda <chandrashekar.devegowda@intel.com> > Co-developed-by: Ricardo Martinez <ricardo.martinez@linux.intel.com> > Signed-off-by: Ricardo Martinez <ricardo.martinez@linux.intel.com> > --- > +static int __t7xx_pci_pm_suspend(struct pci_dev *pdev) > +{ ... > + iowrite32(L1_DISABLE_BIT(0), IREG_BASE(t7xx_dev) + DIS_ASPM_LOWPWR_CLR_0); > + return 0; The success path does this same iowrite32 to DIS_ASPM_LOWPWR_CLR_0 as the failure paths. Is that intended? The function looks much better now!
On Thu, Feb 24, 2022 at 1:35 AM Ricardo Martinez <ricardo.martinez@linux.intel.com> wrote: > From: Haijun Liu <haijun.liu@mediatek.com> > > Cross Layer DMA (CLDMA) Hardware interface (HIF) enables the control > path of Host-Modem data transfers. CLDMA HIF layer provides a common > interface to the Port Layer. > > CLDMA manages 8 independent RX/TX physical channels with data flow > control in HW queues. CLDMA uses ring buffers of General Packet > Descriptors (GPD) for TX/RX. GPDs can represent multiple or single > data buffers (DB). > > CLDMA HIF initializes GPD rings, registers ISR handlers for CLDMA > interrupts, and initializes CLDMA HW registers. > > CLDMA TX flow: > 1. Port Layer write > 2. Get DB address > 3. Configure GPD > 4. Triggering processing via HW register write > > CLDMA RX flow: > 1. CLDMA HW sends a RX "done" to host > 2. Driver starts thread to safely read GPD > 3. DB is sent to Port layer > 4. Create a new buffer for GPD ring > > Signed-off-by: Haijun Liu <haijun.liu@mediatek.com> > Signed-off-by: Chandrashekar Devegowda <chandrashekar.devegowda@intel.com> > Co-developed-by: Ricardo Martinez <ricardo.martinez@linux.intel.com> > Signed-off-by: Ricardo Martinez <ricardo.martinez@linux.intel.com> > > From a WWAN framework perspective: > Reviewed-by: Loic Poulain <loic.poulain@linaro.org> > --- > drivers/net/wwan/t7xx/t7xx_cldma.c | 281 ++++++ > drivers/net/wwan/t7xx/t7xx_cldma.h | 176 ++++ > drivers/net/wwan/t7xx/t7xx_common.h | 40 + > drivers/net/wwan/t7xx/t7xx_hif_cldma.c | 1204 ++++++++++++++++++++++++ > drivers/net/wwan/t7xx/t7xx_hif_cldma.h | 141 +++ > drivers/net/wwan/t7xx/t7xx_reg.h | 33 + > 6 files changed, 1875 insertions(+) > create mode 100644 drivers/net/wwan/t7xx/t7xx_cldma.c > create mode 100644 drivers/net/wwan/t7xx/t7xx_cldma.h > create mode 100644 drivers/net/wwan/t7xx/t7xx_common.h > create mode 100644 drivers/net/wwan/t7xx/t7xx_hif_cldma.c > create mode 100644 drivers/net/wwan/t7xx/t7xx_hif_cldma.h > create mode 100644 drivers/net/wwan/t7xx/t7xx_reg.h > > diff --git a/drivers/net/wwan/t7xx/t7xx_cldma.c b/drivers/net/wwan/t7xx/t7xx_cldma.c > new file mode 100644 > index 000000000000..2713d9a6034b > --- /dev/null > +++ b/drivers/net/wwan/t7xx/t7xx_cldma.c > @@ -0,0 +1,281 @@ > +// SPDX-License-Identifier: GPL-2.0-only > +/* > + * Copyright (c) 2021, MediaTek Inc. > + * Copyright (c) 2021-2022, Intel Corporation. > + * > + * Authors: > + * Haijun Liu <haijun.liu@mediatek.com> > + * Moises Veleta <moises.veleta@intel.com> > + * Ricardo Martinez<ricardo.martinez@linux.intel.com> The space is missed between name and email. The same issue repeated in every file of the series. [skipped] > +#ifdef NET_SKBUFF_DATA_USES_OFFSET > +static inline unsigned int t7xx_skb_data_area_size(struct sk_buff *skb) > +{ > + return skb->head + skb->end - skb->data; > +} > +#else > +static inline unsigned int t7xx_skb_data_area_size(struct sk_buff *skb) > +{ > + return skb->end - skb->data; > +} > +#endif You can use skb_end_pointer() to avoid conditional compilation. -- Sergey
On Thu, Feb 24, 2022 at 1:35 AM Ricardo Martinez <ricardo.martinez@linux.intel.com> wrote: > From: Haijun Liu <haijun.liu@mediatek.com> > > Registers the t7xx device driver with the kernel. Setup all the core > components: PCIe layer, Modem Host Cross Core Interface (MHCCIF), > modem control operations, modem state machine, and build > infrastructure. > > * PCIe layer code implements driver probe and removal. > * MHCCIF provides interrupt channels to communicate events > such as handshake, PM and port enumeration. > * Modem control implements the entry point for modem init, > reset and exit. > * The modem status monitor is a state machine used by modem control > to complete initialization and stop. It is used also to propagate > exception events reported by other components. [skipped] > +static struct t7xx_modem *t7xx_md_alloc(struct t7xx_pci_dev *t7xx_dev) > +{ > + struct device *dev = &t7xx_dev->pdev->dev; > + struct t7xx_modem *md; > + > + md = devm_kzalloc(dev, sizeof(*md), GFP_KERNEL); > + if (!md) > + return NULL; > + > + md->t7xx_dev = t7xx_dev; > + t7xx_dev->md = md; > + md->core_md.ready = false; > + spin_lock_init(&md->exp_lock); > + md->handshake_wq = alloc_workqueue("%s", WQ_UNBOUND | WQ_MEM_RECLAIM | WQ_HIGHPRI, > + 0, "md_hk_wq"); > + if (!md->handshake_wq) > + return NULL; > + > + INIT_WORK(&md->handshake_work, t7xx_md_hk_wq); > + return md; > +} > + > +int t7xx_md_reset(struct t7xx_pci_dev *t7xx_dev) > +{ > + struct t7xx_modem *md = t7xx_dev->md; > + > + md->md_init_finish = false; > + md->exp_id = 0; > + spin_lock_init(&md->exp_lock); Looks like a duplicated initialization, the first time the lock was initialized in the t7xx_md_alloc() above. > + t7xx_fsm_reset(md); > + t7xx_cldma_reset(md->md_ctrl[CLDMA_ID_MD]); > + md->md_init_finish = true; > + return 0; > +} [skipped] > +void t7xx_pcie_set_mac_msix_cfg(struct t7xx_pci_dev *t7xx_dev, unsigned int irq_count) > +{ > + u32 val; > + > + val = ffs(irq_count) * 2 - 1; Move this initialization to the variable declaration. > + iowrite32(val, IREG_BASE(t7xx_dev) + T7XX_PCIE_CFG_MSIX); > +} -- Sergey
On Thu, Feb 24, 2022 at 1:35 AM Ricardo Martinez <ricardo.martinez@linux.intel.com> wrote: > From: Haijun Liu <haijun.liu@mediatek.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] > --- /dev/null > +++ b/drivers/net/wwan/t7xx/t7xx_port.h > ... > +#define PORT_F_RX_ALLOW_DROP BIT(0) /* Packet will be dropped if port's RX buffer full */ The flag is advertised as an overflow policy, but it is actually only used for WWAN (AT&MBIM) ports, and only to free skb in case of an error. Should it be removed and skb consumption rethought? > +#define PORT_F_RX_FULLED BIT(1) /* RX buffer has been detected to be full */ This flag is set, but is never checked. > +#define PORT_F_USER_HEADER BIT(2) /* CCCI header will be provided by user, but not by CCCI */ Above flag is never set and looks like a leftover from earlier driver development. Should it be removed? > +#define PORT_F_RX_EXCLUSIVE BIT(3) /* RX queue only has this one port */ This flag is not used at all. > +#define PORT_F_RX_ADJUST_HEADER BIT(4) /* Check whether need remove CCCI header while recv skb */ This flag is set for WWAN ports, but the control port code unconditionally strips the CCCI header. Should the header be always stripped and this flag removed from the code? > +#define PORT_F_RX_CH_TRAFFIC BIT(5) /* Enable port channel traffic */ This flag is set for wwan ports, but is not ever checked. > +#define PORT_F_RX_CHAR_NODE BIT(7) /* Requires exporting char dev node to userspace */ This flag is not used at all. > +#define PORT_F_CHAR_NODE_SHOW BIT(10) /* The char dev node is shown to userspace by default */ This flag is checked but is never set. [skipped] > +#define PORT_INVALID_CH_ID GENMASK(15, 0) Looks like this macro is never used, should it be removed? And BTW, why is this value defined as a mask? > +/* 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"? > + 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 { I do not care too much, but the purpose of this structure is to carry static port configuration. And the main word here is "configuration", not "static". So why not name this structure port_conf or port_params? > + enum port_ch tx_ch; > + enum port_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; This field is not used. > + struct port_ops *ops; > + char *name; > + enum wwan_port_type port_type; > +}; [skipped] > --- /dev/null > +++ b/drivers/net/wwan/t7xx/t7xx_port_proxy.c > ... > +static struct t7xx_port_static t7xx_md_ports[1]; This array can be defined from the beginning as: static struct t7xx_port_static t7xx_md_ports[] = { }; This will reduce a number of ping-pong changes in the further patches. [skipped] > +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). [skipped] > +int t7xx_port_proxy_send_skb(struct t7xx_port *port, struct sk_buff *skb) > +{ > + struct ccci_header *ccci_h = (struct ccci_header *)(skb->data); > + struct cldma_ctrl *md_ctrl; > + unsigned char tx_qno; > + int ret; > + > + tx_qno = t7xx_port_get_queue_no(port); > + t7xx_port_proxy_set_tx_seq_num(port, ccci_h); > + > + md_ctrl = get_md_ctrl(port); > + ret = t7xx_cldma_send_skb(md_ctrl, tx_qno, skb); > + if (ret) { > + dev_err(port->dev, "Failed to send skb: %d\n", ret); > + return ret; > + } > + > + port->seq_nums[MTK_TX]++; > + > + return 0; > +} > + > +int t7xx_port_send_skb_to_md(struct t7xx_port *port, struct sk_buff *skb) This function looks like a duplicate of the above t7xx_port_proxy_send_skb(). Why can not t7xx_port_proxy_send_skb() be used to send a WWAN (MBIM&AT) port skb? If you need to bypass some checks in t7xx_port_send_skb_to_md() then you can create an exception as already done for PORT_CH_MD_LOG_TX. Or, better, create a set of two functions: first will perform checks and call an actual skb send routine. E.g. int __t7xx_port_proxy_send_skb(...) { return t7xx_cldma_send_skb(...); } int t7xx_port_proxy_send_skb(...) { /* Perform checks here */ return __t7xx_port_proxy_send_skb(...) } This way code duplication will be reduced and if someone calls __t7xx_port_proxy_send_skb() directly bypassing state checks, then it will be clear that such call is a special case. > +{ > + 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? > + return -EBUSY; > + > + if (md_state == MD_STATE_STOPPED || md_state == MD_STATE_WAITING_TO_STOP || > + md_state == MD_STATE_INVALID) > + return -ENODEV; Did you consider to convert the above tests into a big switch? e.g.: switch (md_state) { case MD_STATE_WAITING_FOR_HS1: case ...: case MD_STATE_INVALID: return -ENODEV; case MD_STATE_EXCEPTION: if (port_static->tx_ch != PORT_CH_MD_LOG_TX && port_static->tx_ch != PORT_CH_UART1_TX) return -EBUSY; break; } > + } > + > + md_ctrl = get_md_ctrl(port); > + return t7xx_cldma_send_skb(md_ctrl, t7xx_port_get_queue_no(port), skb); > +} [skipped] > +static struct t7xx_port *t7xx_port_proxy_find_port(struct t7xx_pci_dev *t7xx_dev, > + struct cldma_queue *queue, u16 channel) > +{ > + struct port_proxy *port_prox = t7xx_dev->md->port_prox; > + struct list_head *port_list; > + struct t7xx_port *port; > + u8 ch_id; > + > + ch_id = FIELD_GET(PORT_CH_ID_MASK, channel); > + port_list = &port_prox->rx_ch_ports[ch_id]; > + list_for_each_entry(port, port_list, entry) { > + struct t7xx_port_static *port_static = port->port_static; > + > + if (queue->md_ctrl->hif_id == port_static->path_id && > + channel == port_static->rx_ch) > + return port; > + } > + > + return NULL; > +} > + > +/** > + * t7xx_port_proxy_recv_skb() - Dispatch received skb. > + * @queue: CLDMA queue. > + * @skb: Socket buffer. > + * > + * Return: > + ** 0 - Packet consumed. > + ** -ERROR - Failed to process skb. > + */ > +static int t7xx_port_proxy_recv_skb(struct cldma_queue *queue, struct sk_buff *skb) > +{ > + struct ccci_header *ccci_h = (struct ccci_header *)skb->data; > + struct t7xx_pci_dev *t7xx_dev = queue->md_ctrl->t7xx_dev; > + struct t7xx_fsm_ctl *ctl = t7xx_dev->md->fsm_ctl; > + struct device *dev = queue->md_ctrl->dev; > + struct t7xx_port_static *port_static; > + struct t7xx_port *port; > + u16 seq_num, channel; > + int ret; > + > + if (!skb) > + return -EINVAL; > + > + channel = FIELD_GET(CCCI_H_CHN_FLD, le32_to_cpu(ccci_h->status)); > + if (t7xx_fsm_get_md_state(ctl) == MD_STATE_INVALID) { > + dev_err_ratelimited(dev, "Packet drop on channel 0x%x, modem not ready\n", channel); > + goto drop_skb; > + } > + > + port = t7xx_port_proxy_find_port(t7xx_dev, queue, channel); > + if (!port) { > + dev_err_ratelimited(dev, "Packet drop on channel 0x%x, port not found\n", channel); > + goto drop_skb; > + } > + > + seq_num = t7xx_port_next_rx_seq_num(port, ccci_h); > + port_static = port->port_static; > + ret = port_static->ops->recv_skb(port, skb); > + if (ret && port->flags & PORT_F_RX_ALLOW_DROP) { > + port->seq_nums[MTK_RX] = seq_num; > + dev_err_ratelimited(dev, "Packed drop on port %s, error %d\n", > + port_static->name, ret); > + goto drop_skb; > + } > + > + if (ret) > + return ret; > + > + port->seq_nums[MTK_RX] = seq_num; > + return 0; > + > +drop_skb: > + dev_kfree_skb_any(skb); > + return 0; > +} [skipped] > +/** > + * t7xx_port_proxy_node_control() - Create/remove node. > + * @md: Modem. > + * @port_msg: Message. > + * > + * Used to control create/remove device node. > + * > + * Return: > + * * 0 - Success. > + * * -EFAULT - Message check failure. > + */ > +int t7xx_port_proxy_node_control(struct t7xx_modem *md, struct port_msg *port_msg) > +{ > + u32 *port_info_base = (void *)port_msg + sizeof(*port_msg); > + struct device *dev = &md->t7xx_dev->pdev->dev; > + unsigned int version, ports, i; > + > + version = FIELD_GET(PORT_MSG_VERSION, le32_to_cpu(port_msg->info)); > + if (version != PORT_ENUM_VER || > + le32_to_cpu(port_msg->head_pattern) != PORT_ENUM_HEAD_PATTERN || > + le32_to_cpu(port_msg->tail_pattern) != PORT_ENUM_TAIL_PATTERN) { > + dev_err(dev, "Invalid port control message %x:%x:%x\n", > + version, le32_to_cpu(port_msg->head_pattern), > + le32_to_cpu(port_msg->tail_pattern)); > + return -EFAULT; > + } > + > + ports = FIELD_GET(PORT_MSG_PRT_CNT, le32_to_cpu(port_msg->info)); > + for (i = 0; i < ports; i++) { > + struct t7xx_port_static *port_static; > + u32 *port_info = port_info_base + i; > + struct t7xx_port *port; > + unsigned int ch_id; > + bool en_flag; > + > + ch_id = FIELD_GET(PORT_INFO_CH_ID, *port_info); > + port = t7xx_proxy_get_port_by_ch(md->port_prox, ch_id); > + if (!port) { > + dev_dbg(dev, "Port:%x not found\n", ch_id); > + continue; > + } > + > + en_flag = !!(PORT_INFO_ENFLG & *port_info); > + > + if (t7xx_fsm_get_md_state(md->fsm_ctl) == MD_STATE_READY) { > + port_static = port->port_static; > + > + if (en_flag) { > + if (port_static->ops->enable_chl) > + port_static->ops->enable_chl(port); > + } else { > + if (port_static->ops->disable_chl) > + port_static->ops->disable_chl(port); > + } > + } else { > + port->chan_enable = en_flag; > + } > + } > + > + return 0; > +} Should most of this function (message parsing and verification code) be moved to the control port code, and left only the channel enable/disable code here? The port_msg structure definition should probably be moved to the control port code too. I mean, the port message belongs more to the control protocol and the proxy should not care too much about this top level protocol internals. [skipped] > +struct port_proxy { > + int port_number; > + struct t7xx_port_static *ports_shared; This field is not actually used and probably should be removed. > + struct t7xx_port *ports_private; Since the 'ports_shared' field should be removed, this field can be renamed to 'ports' for simplicity. BTW, the ports array can be moved to the end of the structure in form of flexible array: struct t7xx_port ports[]; so it will be possible to allocate memory for the main proxy data and for ports array as a single block. > + struct list_head rx_ch_ports[PORT_CH_ID_MASK + 1]; > + struct list_head queue_ports[CLDMA_NUM][MTK_QUEUES]; > + struct device *dev; > +}; -- Sergey
On Thu, Feb 24, 2022 at 1:35 AM Ricardo Martinez <ricardo.martinez@linux.intel.com> wrote: > From: Haijun Liu <haijun.liu@mediatek.com> > > Control Port implements driver control messages such as modem-host > handshaking, controls port enumeration, and handles exception messages. > > The handshaking process between the driver and the modem happens during > the init sequence. The process involves the exchange of a list of > supported runtime features to make sure that modem and host are ready > to provide proper feature lists including port enumeration. Further > features can be enabled and controlled in this handshaking process. [skipped] > +struct feature_query { > + __le32 head_pattern; > + u8 feature_set[FEATURE_COUNT]; > + __le32 tail_pattern; > +}; > + > +static void t7xx_prepare_host_rt_data_query(struct t7xx_sys_info *core) > +{ > + struct t7xx_port_static *port_static = core->ctl_port->port_static; > + struct ctrl_msg_header *ctrl_msg_h; > + struct feature_query *ft_query; > + struct ccci_header *ccci_h; > + struct sk_buff *skb; > + size_t packet_size; > + > + packet_size = sizeof(*ccci_h) + sizeof(*ctrl_msg_h) + sizeof(*ft_query); > + skb = __dev_alloc_skb(packet_size, GFP_KERNEL); > + if (!skb) > + return; > + > + skb_put(skb, packet_size); > + > + ccci_h = (struct ccci_header *)skb->data; > + t7xx_ccci_header_init(ccci_h, 0, packet_size, port_static->tx_ch, 0); > + ccci_h->status &= cpu_to_le32(~CCCI_H_SEQ_FLD); > + > + ctrl_msg_h = (struct ctrl_msg_header *)(skb->data + sizeof(*ccci_h)); > + t7xx_ctrl_msg_header_init(ctrl_msg_h, CTL_ID_HS1_MSG, 0, sizeof(*ft_query)); > + > + ft_query = (struct feature_query *)(skb->data + sizeof(*ccci_h) + sizeof(*ctrl_msg_h)); > + ft_query->head_pattern = cpu_to_le32(MD_FEATURE_QUERY_ID); > + memcpy(ft_query->feature_set, core->feature_set, FEATURE_COUNT); > + ft_query->tail_pattern = cpu_to_le32(MD_FEATURE_QUERY_ID); > + > + /* Send HS1 message to device */ > + t7xx_port_proxy_send_skb(core->ctl_port, skb); > +} I do not care too much, but this code and many other places could be greatly simplified. It looks like the modem communication protocol has a layered design, skb and its API are also designed to handle layered protocols. It just needs to rearrange the code a bit. For example, to avoid manual accounting of each header in the stack, skb allocation can be implemented using a stack of allocation functions: struct sk_buff *t7xx_port_alloc_skb(int payload) { struct sk_buff *skb = alloc_skb(payload + sizeof(struct ccci_header), ...); if (skb) skb_reserve(skb, sizeof(struct ccci_header)); return skb; } struct sk_buff *t7xx_ctrl_alloc_skb(int payload) { struct sk_buff *skb = t7xx_port_alloc_skb(payload + sizeof(struct ctlr_msg_header), ...); if (skb) skb_reserve(skb, sizeof(struct ctrl_msg_header)); return skb; } Message sending operation can also be perfectly stacked: int t7xx_port_proxy_send_skb(*port, *skb) { struct ccci_header *ccci_h = skb_push(skb, sizeof(*ccci_h)); /* Build CCCI header (including seqno assignment) */ ccci_h->packet_len = cpu_to_le32(skb->len); res = cldma_send_skb(..., skb, ...); if (res) return res; /* Update seqno counter here */ return 0; } int t7xx_ctrl_send_msg(port, msg_id, skb) { int len = skb->len; /* Preserve payload len */ struct ctrl_msg_header *ctrl_msg_h = skb_push(skb, sizeof(*ctrl_msg_h)); /* Build ctrl msg header here */ ctrl_msg_h->data_length = cpu_to_le32(len); return t7xx_port_proxy_send_skb(port, skb); } So the above features request becomes as simple as: void t7xx_prepare_host_rt_data_query(struct t7xx_sys_info *core) { struct feature_query *ft_query; struct sk_buff *skb; skb = t7xx_ctrl_alloc_skb(sizeof(*ft_query)); if (!skb) return; ft_query = skb_put(skb, sizeof(*ft_query)); /* Build features request here */ if (t7xx_ctrl_send_msg(core->ctl_port, CTL_ID_HS1_MSG, skb)) kfree_skb(skb); } Once the allocation and sending functions are implemented in a stacked way, many other places can be simplified in a similar way. [skipped] > +static void t7xx_core_hk_handler(struct t7xx_modem *md, struct t7xx_fsm_ctl *ctl, > + enum t7xx_fsm_event_state event_id, > + enum t7xx_fsm_event_state err_detect) > +{ > + struct t7xx_sys_info *core_info = &md->core_md; > + struct device *dev = &md->t7xx_dev->pdev->dev; > + struct t7xx_fsm_event *event, *event_next; > + unsigned long flags; > + void *event_data; > + int ret; > + > + t7xx_prepare_host_rt_data_query(core_info); > + > + while (!kthread_should_stop()) { > + bool event_received = false; > + > + spin_lock_irqsave(&ctl->event_lock, flags); > + list_for_each_entry_safe(event, event_next, &ctl->event_queue, entry) { > + if (event->event_id == err_detect) { > + list_del(&event->entry); > + spin_unlock_irqrestore(&ctl->event_lock, flags); > + dev_err(dev, "Core handshake error event received\n"); > + goto err_free_event; > + } else if (event->event_id == event_id) { > + list_del(&event->entry); > + event_received = true; > + break; > + } > + } > + spin_unlock_irqrestore(&ctl->event_lock, flags); > + > + if (event_received) > + break; > + > + wait_event_interruptible(ctl->event_wq, !list_empty(&ctl->event_queue) || > + kthread_should_stop()); > + if (kthread_should_stop()) > + goto err_free_event; > + } > + > + if (ctl->exp_flg) > + goto err_free_event; > + > + event_data = (void *)event + sizeof(*event); In the V2, the event structure has a data field. But then it was dropped and now the attached data offset is manually calculated. Why did you do this, why event->data is not suitable here? > + ret = t7xx_parse_host_rt_data(ctl, core_info, dev, event_data, event->length); > + if (ret) { > + dev_err(dev, "Host failure parsing runtime data: %d\n", ret); > + goto err_free_event; > + } > + > + if (ctl->exp_flg) > + goto err_free_event; > + > + ret = t7xx_prepare_device_rt_data(core_info, dev, event_data, event->length); > + if (ret) { > + dev_err(dev, "Device failure parsing runtime data: %d", ret); > + goto err_free_event; > + } > + > + core_info->ready = true; > + core_info->handshake_ongoing = false; > + wake_up(&ctl->async_hk_wq); > +err_free_event: > + kfree(event); > +} [skipped] > +static int port_ctl_init(struct t7xx_port *port) > +{ > + struct t7xx_port_static *port_static = port->port_static; > + > + port->skb_handler = &control_msg_handler; & is not necessary here and only misguides readers. > + port->thread = kthread_run(port_ctl_rx_thread, port, "%s", port_static->name); > + if (IS_ERR(port->thread)) { > + dev_err(port->dev, "Failed to start port control thread\n"); > + return PTR_ERR(port->thread); > + } > + > + port->rx_length_th = CTRL_QUEUE_MAXLEN; > + return 0; > +} [skipped] > -static struct t7xx_port_static t7xx_md_ports[1]; > +static struct t7xx_port_static t7xx_md_ports[] = { > + { > + .tx_ch = PORT_CH_CONTROL_TX, > + .rx_ch = PORT_CH_CONTROL_RX, > + .txq_index = Q_IDX_CTRL, > + .rxq_index = Q_IDX_CTRL, > + .txq_exp_index = 0, > + .rxq_exp_index = 0, > + .path_id = CLDMA_ID_MD, > + .flags = 0, Zero initializer is not needed here, a static variable is filled with zeros automatically. > + .ops = &ctl_port_ops, > + .name = "t7xx_ctrl", > + }, > +}; [skipped] > +void t7xx_port_proxy_send_msg_to_md(struct port_proxy *port_prox, enum port_ch ch, > + unsigned int msg, unsigned int ex_msg) This function is called only from the control port code and only with ch = PORT_CH_CONTROL_TX, so I would like to recommend to move it there and drop the ch argument. > +{ > + struct ctrl_msg_header *ctrl_msg_h; > + struct ccci_header *ccci_h; > + struct t7xx_port *port; > + struct sk_buff *skb; > + int ret; > + > + port = t7xx_proxy_get_port_by_ch(port_prox, ch); > + if (!port) > + return; > + > + skb = __dev_alloc_skb(sizeof(*ccci_h), GFP_KERNEL); > + if (!skb) > + return; > + > + if (ch == PORT_CH_CONTROL_TX) { > + ccci_h = (struct ccci_header *)(skb->data); > + t7xx_ccci_header_init(ccci_h, CCCI_HEADER_NO_DATA, > + sizeof(*ctrl_msg_h) + sizeof(*ccci_h), ch, 0); > + ctrl_msg_h = (struct ctrl_msg_header *)(skb->data + sizeof(*ccci_h)); > + t7xx_ctrl_msg_header_init(ctrl_msg_h, msg, ex_msg, 0); > + skb_put(skb, sizeof(*ccci_h) + sizeof(*ctrl_msg_h)); > + } else { > + ccci_h = skb_put(skb, sizeof(*ccci_h)); > + t7xx_ccci_header_init(ccci_h, CCCI_HEADER_NO_DATA, msg, ch, ex_msg); > + } > + > + ret = t7xx_port_proxy_send_skb(port, skb); > + if (ret) { > + struct t7xx_port_static *port_static = port->port_static; > + > + dev_kfree_skb_any(skb); > + dev_err(port->dev, "port%s send to MD fail\n", port_static->name); > + } > +} -- Sergey
On Thu, Feb 24, 2022 at 1:35 AM Ricardo Martinez <ricardo.martinez@linux.intel.com> wrote: > From: Chandrashekar Devegowda <chandrashekar.devegowda@intel.com> > > Adds AT and MBIM ports to the port proxy infrastructure. > The initialization method is responsible for creating the corresponding > ports using the WWAN framework infrastructure. The implemented WWAN port > operations are start, stop, and TX. [skipped] > +static int t7xx_port_ctrl_tx(struct wwan_port *port, struct sk_buff *skb) > +{ > + struct t7xx_port *port_private = wwan_port_get_drvdata(port); > + size_t actual_len, alloc_size, txq_mtu = CLDMA_MTU; > + struct t7xx_port_static *port_static; > + unsigned int len, i, packets; > + struct t7xx_fsm_ctl *ctl; > + enum md_state md_state; > + > + len = skb->len; > + if (!len || !port_private->rx_length_th || !port_private->chan_enable) > + return -EINVAL; > + > + port_static = port_private->port_static; > + ctl = port_private->t7xx_dev->md->fsm_ctl; > + md_state = t7xx_fsm_get_md_state(ctl); > + if (md_state == MD_STATE_WAITING_FOR_HS1 || md_state == MD_STATE_WAITING_FOR_HS2) { > + dev_warn(port_private->dev, "Cannot write to %s port when md_state=%d\n", > + port_static->name, md_state); > + return -ENODEV; > + } > + > + alloc_size = min_t(size_t, txq_mtu, len + CCCI_HEADROOM); > + actual_len = alloc_size - CCCI_HEADROOM; > + packets = DIV_ROUND_UP(len, txq_mtu - CCCI_HEADROOM); > + > + for (i = 0; i < packets; i++) { > + struct ccci_header *ccci_h; > + struct sk_buff *skb_ccci; > + int ret; > + > + if (packets > 1 && packets == i + 1) { > + actual_len = len % (txq_mtu - CCCI_HEADROOM); > + alloc_size = actual_len + CCCI_HEADROOM; > + } Why do you track the packet number? Why not track the offset in the passed data? E.g.: for (off = 0; off < len; off += chunklen) { chunklen = min(len - off, CLDMA_MTU - sizeof(struct ccci_header); skb_ccci = alloc_skb(chunklen + sizeof(struct ccci_header), ...); skb_put_data(skb_ccci, skb->data + off, chunklen); /* Send skb_ccci */ } > + skb_ccci = __dev_alloc_skb(alloc_size, GFP_KERNEL); > + if (!skb_ccci) > + return -ENOMEM; > + > + ccci_h = skb_put(skb_ccci, sizeof(*ccci_h)); > + t7xx_ccci_header_init(ccci_h, 0, actual_len + sizeof(*ccci_h), > + port_static->tx_ch, 0); > + skb_put_data(skb_ccci, skb->data + i * (txq_mtu - CCCI_HEADROOM), actual_len); > + t7xx_port_proxy_set_tx_seq_num(port_private, ccci_h); > + > + ret = t7xx_port_send_skb_to_md(port_private, skb_ccci); > + if (ret) { > + dev_kfree_skb_any(skb_ccci); > + dev_err(port_private->dev, "Write error on %s port, %d\n", > + port_static->name, ret); > + return ret; > + } > + > + port_private->seq_nums[MTK_TX]++; Sequence number tracking as well as CCCI header construction are common operations, so why not move them to t7xx_port_send_skb_to_md()? > + } > + > + dev_kfree_skb(skb); > + return 0; > +} -- Sergey
On Thu, Feb 24, 2022 at 1:35 AM Ricardo Martinez <ricardo.martinez@linux.intel.com> wrote: > From: Haijun Liu <haijun.liu@mediatek.com> > > Data Path Modem AP Interface (DPMAIF) HIF layer provides methods > for initialization, ISR, control and event handling of TX/RX flows. > > DPMAIF TX > Exposes the `dmpaif_tx_send_skb` function which can be used by the > network device to transmit packets. > The uplink data management uses a Descriptor Ring Buffer (DRB). > First DRB entry is a message type that will be followed by 1 or more > normal DRB entries. Message type DRB will hold the skb information > and each normal DRB entry holds a pointer to the skb payload. > > DPMAIF RX > The downlink buffer management uses Buffer Address Table (BAT) and > Packet Information Table (PIT) rings. > The BAT ring holds the address of skb data buffer for the HW to use, > while the PIT contains metadata about a whole network packet including > a reference to the BAT entry holding the data buffer address. > The driver reads the PIT and BAT entries written by the modem, when > reaching a threshold, the driver will reload the PIT and BAT rings. [skipped] > --- /dev/null > +++ b/drivers/net/wwan/t7xx/t7xx_hif_dpmaif.h > ... > +#ifndef __T7XX_DPMA_TX_H__ > +#define __T7XX_DPMA_TX_H__ Maybe __T7XX_HIF_DPMAIF_H__ ? [skipped] > +struct dpmaif_tx_queue { > + unsigned char index; > + bool que_started; > + atomic_t tx_budget; > + void *drb_base; > + dma_addr_t drb_bus_addr; > + unsigned int drb_size_cnt; > + unsigned short drb_wr_idx; > + unsigned short drb_rd_idx; > + unsigned short drb_release_rd_idx; > + void *drb_skb_base; > + wait_queue_head_t req_wq; > + struct workqueue_struct *worker; > + struct work_struct dpmaif_tx_work; > + spinlock_t tx_lock; /* Protects txq DRB */ > + atomic_t tx_processing; > + > + struct dpmaif_ctrl *dpmaif_ctrl; > + spinlock_t tx_skb_lock; /* Protects TX thread skb list */ > + struct list_head tx_skb_queue; Should this be the sk_buff_head struct? So you will be able to use a regular skb_queue_foo() functions and have the embedded spinlock? > + unsigned int tx_submit_skb_cnt; > + unsigned int tx_list_max_len; > + unsigned int tx_skb_stat; > +}; [skipped] > +static void t7xx_dpmaif_parse_msg_pit(const struct dpmaif_rx_queue *rxq, > + const struct dpmaif_msg_pit *msg_pit, > + struct dpmaif_cur_rx_skb_info *skb_info) > +{ > + skb_info->cur_chn_idx = FIELD_GET(MSG_PIT_CHANNEL_ID, le32_to_cpu(msg_pit->dword1)); > + skb_info->check_sum = FIELD_GET(MSG_PIT_CHECKSUM, le32_to_cpu(msg_pit->dword1)); > + skb_info->pit_dp = FIELD_GET(MSG_PIT_DP, le32_to_cpu(msg_pit->dword1)); Here you can first convert dword1 field value to a native endians and store it in a local variable, and then use it in the FIELD_GET(). > + skb_info->pkt_type = FIELD_GET(MSG_PIT_IP, le32_to_cpu(msg_pit->dword4)); > +} [skipped] > +/* Structure of DL PIT */ > +struct dpmaif_normal_pit { > + __le32 pit_header; > + __le32 p_data_addr; > + __le32 data_addr_ext; > + __le32 pit_footer; > +}; > > ... > > +struct dpmaif_msg_pit { > + __le32 dword1; > + __le32 dword2; > + __le32 dword3; > + __le32 dword4; > +}; Just an idea. Both PIT normal (aka PD) and PIT Msg structs have the same size and even partially share the first header bits, so why not define both formats in a single structure using union: struct dpmaif_pit { __le32 header; union { struct { __le32 data_addr_l; __le32 data_addr_h; __le32 footer; } pd; struct { __le32 dword2; __le32 dword3; __le32 dword4; } msg; }; }; Defining the format in this way eliminates the cast and allows to turn the pit_base field from a void pointer into a struct dpmaif_pit pointer and handle it as an array. [skipped] > +static void t7xx_setup_payload_drb(struct dpmaif_ctrl *dpmaif_ctrl, unsigned char q_num, > + unsigned short cur_idx, dma_addr_t data_addr, > + unsigned int pkt_size, bool last_one) > +{ > + struct dpmaif_drb_pd *drb_base = dpmaif_ctrl->txq[q_num].drb_base; > + struct dpmaif_drb_pd *drb = drb_base + cur_idx; > + > + drb->header &= cpu_to_le32(~DRB_PD_DTYP); > + drb->header |= cpu_to_le32(FIELD_PREP(DRB_PD_DTYP, DES_DTYP_PD)); > + drb->header &= cpu_to_le32(~DRB_PD_CONT); > + > + if (!last_one) > + drb->header |= cpu_to_le32(FIELD_PREP(DRB_PD_CONT, 1)); Empty line between DRB_PD_CONT field clean and setup looks odd. > + > + drb->header &= cpu_to_le32(~(u32)DRB_PD_DATA_LEN); > + drb->header |= cpu_to_le32(FIELD_PREP(DRB_PD_DATA_LEN, pkt_size)); > + drb->p_data_addr = cpu_to_le32(lower_32_bits(data_addr)); > + drb->data_addr_ext = cpu_to_le32(upper_32_bits(data_addr)); > +} [skipped] > +static int t7xx_dpmaif_add_skb_to_ring(struct dpmaif_ctrl *dpmaif_ctrl, struct sk_buff *skb) > +{ > + unsigned short cur_idx, drb_wr_idx_backup; > ... > + txq = &dpmaif_ctrl->txq[skb_cb->txq_number]; > ... > + cur_idx = txq->drb_wr_idx; > + drb_wr_idx_backup = cur_idx; > ... > + for (wr_cnt = 0; wr_cnt < payload_cnt; wr_cnt++) { > ... > + bus_addr = dma_map_single(dpmaif_ctrl->dev, data_addr, data_len, DMA_TO_DEVICE); > + if (dma_mapping_error(dpmaif_ctrl->dev, bus_addr)) { > + dev_err(dpmaif_ctrl->dev, "DMA mapping fail\n"); > + atomic_set(&txq->tx_processing, 0); > + > + spin_lock_irqsave(&txq->tx_lock, flags); > + txq->drb_wr_idx = drb_wr_idx_backup; > + spin_unlock_irqrestore(&txq->tx_lock, flags); What is the purpose of locking here? > + return -ENOMEM; > + } > ... > + } > ... > +} [skipped] > +struct dpmaif_drb_pd { > + __le32 header; > + __le32 p_data_addr; > + __le32 data_addr_ext; > + __le32 reserved2; > +}; > ... > +struct dpmaif_drb_msg { > + __le32 header_dw1; > + __le32 header_dw2; > + __le32 reserved4; > + __le32 reserved5; > +}; Like PIT, DRB can be defined using a single structure with union. With the same benefits as for PIT. struct dpmaif_drb { __le32 header; union { struct { __le32 data_addr_l; __le32 data_addr_h; __le32 reserved2; } pd; struct { __le32 msghdr; __le32 reserved4; __le32 reserved5; } msg; }; }; But it is up to you how you define and handle these formats. I just like unions, as you can see :) -- Sergey
On Thu, Feb 24, 2022 at 1:35 AM Ricardo Martinez <ricardo.martinez@linux.intel.com> wrote: > From: Haijun Liu <haijun.liu@mediatek.com> > > Creates the Cross Core Modem Network Interface (CCMNI) which implements > the wwan_ops for registration with the WWAN framework, CCMNI also > implements the net_device_ops functions used by the network device. > Network device operations include open, close, start transmission, TX > timeout, change MTU, and select queue. [skipped] > +static u16 t7xx_ccmni_select_queue(struct net_device *dev, struct sk_buff *skb, > + struct net_device *sb_dev) > +{ > + return DPMAIF_TX_DEFAULT_QUEUE; > +} [skipped] > +static const struct net_device_ops ccmni_netdev_ops = { > + .ndo_open = t7xx_ccmni_open, > + .ndo_stop = t7xx_ccmni_close, > + .ndo_start_xmit = t7xx_ccmni_start_xmit, > + .ndo_tx_timeout = t7xx_ccmni_tx_timeout, > + .ndo_select_queue = t7xx_ccmni_select_queue, Since the driver works in the single queue mode, this callback is unneeded. > +}; -- Sergey
Hello Ricardo, On Thu, Feb 24, 2022 at 1:35 AM Ricardo Martinez <ricardo.martinez@linux.intel.com> wrote: > t7xx is the PCIe host device driver for Intel 5G 5000 M.2 solution which > is based on MediaTek's T700 modem to provide WWAN connectivity. > The driver uses the WWAN framework infrastructure to create the following > control ports and network interfaces: > * /dev/wwan0mbim0 - Interface conforming to the MBIM protocol. > Applications like libmbim [1] or Modem Manager [2] from v1.16 onwards > with [3][4] can use it to enable data communication towards WWAN. > * /dev/wwan0at0 - Interface that supports AT commands. > * wwan0 - Primary network interface for IP traffic. > > The main blocks in t7xx driver are: > * PCIe layer - Implements probe, removal, and power management callbacks. > * Port-proxy - Provides a common interface to interact with different types > of ports such as WWAN ports. > * Modem control & status monitor - Implements the entry point for modem > initialization, reset and exit, as well as exception handling. > * CLDMA (Control Layer DMA) - Manages the HW used by the port layer to send > control messages to the modem using MediaTek's CCCI (Cross-Core > Communication Interface) protocol. > * DPMAIF (Data Plane Modem AP Interface) - Controls the HW that provides > uplink and downlink queues for the data path. The data exchange takes > place using circular buffers to share data buffer addresses and metadata > to describe the packets. > * MHCCIF (Modem Host Cross-Core Interface) - Provides interrupt channels > for bidirectional event notification such as handshake, exception, PM and > port enumeration. > > The compilation of the t7xx driver is enabled by the CONFIG_MTK_T7XX config > option which depends on CONFIG_WWAN. > This driver was originally developed by MediaTek. Intel adapted t7xx to > the WWAN framework, optimized and refactored the driver source code in close > collaboration with MediaTek. This will enable getting the t7xx driver on the > Approved Vendor List for interested OEM's and ODM's productization plans > with Intel 5G 5000 M.2 solution. > > List of contributors: > Amir Hanania <amir.hanania@intel.com> > Andriy Shevchenko <andriy.shevchenko@linux.intel.com> > Chandrashekar Devegowda <chandrashekar.devegowda@intel.com> > Dinesh Sharma <dinesh.sharma@intel.com> > Eliot Lee <eliot.lee@intel.com> > Haijun Liu <haijun.liu@mediatek.com> > M Chetan Kumar <m.chetan.kumar@intel.com> > Mika Westerberg <mika.westerberg@linux.intel.com> > Moises Veleta <moises.veleta@intel.com> > Pierre-louis Bossart <pierre-louis.bossart@intel.com> > Chiranjeevi Rapolu <chiranjeevi.rapolu@intel.com> > Ricardo Martinez <ricardo.martinez@linux.intel.com> > Madhusmita Sahu <madhusmita.sahu@intel.com> > Muralidharan Sethuraman <muralidharan.sethuraman@intel.com> > Soumya Prakash Mishra <Soumya.Prakash.Mishra@intel.com> > Sreehari Kancharla <sreehari.kancharla@intel.com> > Suresh Nagaraj <suresh.nagaraj@intel.com> > > [1] https://www.freedesktop.org/software/libmbim/ > [2] https://www.freedesktop.org/software/ModemManager/ > [3] https://gitlab.freedesktop.org/mobile-broadband/ModemManager/-/merge_requests/582 > [4] https://gitlab.freedesktop.org/mobile-broadband/ModemManager/-/merge_requests/523 > > v5: > - Update Intel's copyright years to 2021-2022. > - Remove circular dependency between DPMAIF HW (07) and HIF (08). > - Keep separate patches for CLDMA (02) and Core (03) > but improve the code split by decoupling CLDMA from > modem ops and cleaning up t7xx_common.h. > - Rename ID_CLDMA0/ID_CLDMA1 to CLDMA_ID_AP/CLDMA_ID_MD. > - Consistently use CLDMA's ring_lock to protect tr_ring. > - Free resources first and then print messages. > - Implement suggested name changes. > - Do not explicitly include dev_printk.h. > - Remove redundant dev_err()s. > - Fix possible memory leak during probe. > - Remove infrastructure for legacy interrupts. > - Remove unused macros and variables, including those that > can be replaced with constants. > - Remove PCIE_MAC_MSIX_MSK_SET macro which is duplicated code. > - Refactor __t7xx_pci_pm_suspend() for clarity. > - Refactor t7xx_cldma_rx_ring_init() and t7xx_cldma_tx_ring_init(). > - Do not use & for function callbacks. > - Declare a structure to access skb->cb[] data. > - Use skb_put_data instead of memcpy. > - No need to use kfree_sensitive. > - Use dev_kfree_skb() instead of kfree_skb(). > - Refactor t7xx_prepare_device_rt_data() to remove potential leaks, > avoid unneeded memset and keep rt_data and packet_size updates > inside the same 'if' block. > - Set port's rx_length_th back to 0 during uninit. > - Remove unneeded 'blocking' parameter from t7xx_cldma_send_skb(). > - Return -EIO in t7xx_cldma_send_skb() if the queue is inactive. > - Refactor t7xx_cldma_qs_are_active() to use pci_device_is_present(). > - Simplify t7xx_cldma_stop_q() and rename it to t7xx_cldma_stop_all_qs(). > - Fix potential leaks in t7xx_cldma_init(). > - Improve error handling in fsm_append_event and fsm_routine_starting(). > - Propagate return codes from fsm_append_cmd() and t7xx_fsm_append_event(). > - Refactor fsm_wait_for_event() to avoid unnecessary sleep. > - Create the WWAN ports and net device only after the modem is in > the ready state. > - Refactor t7xx_port_proxy_recv_skb() and port_recv_skb(). > - Rename t7xx_port_check_rx_seq_num() as t7xx_port_next_rx_seq_num() > and fix the seq_num logic to handle overflows. > - Declare seq_nums as u16 instead of short. > - Use unsigned int for local indexes. > - Use min_t instead of the ternary operator. > - Refactor the loop in t7xx_dpmaif_rx_data_collect() to avoid a dead > condition check. > - Use a bitmap (bat_bitmap) instead of an array to keep track of > the DRB status. Used in t7xx_dpmaif_avail_pkt_bat_cnt(). > - Refactor t7xx_dpmaif_tx_send_skb() to protect tx_submit_skb_cnt > with spinlock and remove the misleading tx_drb_available variable. > - Consolidate bit operations before endianness conversion. > - Use C bit fields in dpmaif_drb_skb struct which is not HW related. > - Add back the que_started check in t7xx_select_tx_queue(). > - Create a helper function to get the DRB count. > - Simplify the use of 'usage' during t7xx_ccmni_close(). > - Enforce CCMNI MTU selection with BUILD_BUG_ON() instead of a comment. > - Remove t7xx_ccmni_ctrl->capability parameter which remains constant. > > v4: > - Implement list_prev_entry_circular() and list_next_entry_circular() macros. > - Remove inline from all c files. > - Define ioread32_poll_timeout_atomic() helper macro. > - Fix return code for WWAN port tx op. > - Allow AT commands fragmentation same as MBIM commands. > - Introduce t7xx_common.h file in the first patch. > - Rename functions and variables as suggested in v3. > - Reduce code duplication by creating fsm_wait_for_event() helper function. > - Remove unneeded dev_err in t7xx_fsm_clr_event(). > - Remove unused variable last_state from struct t7xx_fsm_ctl. > - Remove unused variable txq_select_times from struct dpmaif_ctrl. > - Replace ETXTBSY with EBUSY. > - Refactor t7xx_dpmaif_rx_buf_alloc() to remove an unneeded allocation. > - Fix potential leak at t7xx_dpmaif_rx_frag_alloc(). > - Simplify return value handling at t7xx_dpmaif_rx_start(). > - Add a helper to handle the common part of CCCI header initialization. > - Make sure interrupts are enabled during PM resume. > - Add a parameter to t7xx_fsm_append_cmd() to tell if it is in interrupt context. > > v3: > - Avoid unneeded ping-pong changes between patches. > - Use t7xx_ prefix in functions. > - Use t7xx_ prefix in generic structs where mtk_ or ccci prefix was used. > - Update Authors/Contributors header. > - Remove skb pools used for control path. > - Remove skb pools used for RX data path. > - Do not use dedicated TX queue for ACK-only packets. > - Remove __packed attribute from GPD structs. > - Remove the infrastructure for test and debug ports. > - Use the skb control buffer to store metadata. > - Get the IP packet type from RX PIT. > - Merge variable declaration and simple assignments. > - Use preferred coding patterns. > - Remove global variables. > - Declare HW facing structure members as little endian. > - Rename goto tags to describe what is going to be done. > - Do not use variable length arrays. > - Remove unneeded blank lines source code and kdoc headers. > - Use C99 initialization format for port-proxy ports. > - Clean up comments. > - Review included headers. > - Better use of 100 column limit. > - Remove unneeded mb() in CLDMA. > - Remove unneeded spin locks and atomics. > - Handle read_poll_timeout error. > - Use dev_err_ratelimited() where required. > - Fix resource leak when requesting IRQs. > - Use generic DEFAULT_TX_QUEUE_LEN instead custom macro. > - Use ETH_DATA_LEN instead of defining WWAN_DEFAULT_MTU. > - Use sizeof() instead of defines when the size of structures is required. > - Remove unneeded code from netdev: > No need to configure HW address length > No need to implement .ndo_change_mtu > Remove random address generation > - Code simplifications by using kernel provided functions and macros such as: > module_pci_driver > PTR_ERR_OR_ZERO > for_each_set_bit > pci_device_is_present > skb_queue_purge > list_prev_entry > __ffs64 > > v2: > - Replace pdev->driver->name with dev_driver_string(&pdev->dev). > - Replace random_ether_addr() with eth_random_addr(). > - Update kernel-doc comment for enum data_policy. > - Indicate the driver is 'Supported' instead of 'Maintained'. > - Fix the Signed-of-by and Co-developed-by tags in the patches. > - Added authors and contributors in the top comment of the src files. Sorry for the delay in review. There are too many changes from version to version and it took some time to dig through them. I would like to admit that the driver now looks much better. Good job! Please find per-patch comments. -- Sergey
On 2/25/2022 2:54 AM, Ilpo Järvinen wrote: > On Wed, 23 Feb 2022, Ricardo Martinez wrote: ... >> +/** >> + * t7xx_cldma_send_skb() - Send control data to modem. >> + * @md_ctrl: CLDMA context structure. >> + * @qno: Queue number. >> + * @skb: Socket buffer. >> + * >> + * Return: > ... >> + * * -EBUSY - Resource lock failure. > Leftover? This function can still return -EBUSY if t7xx_pci_sleep_disable_complete() fails. Maybe returning -ETIMED would make more sense based on the implementation of t7xx_pci_sleep_disable_complete(). > > ...with those addressed > Reviewed-by: Ilpo Järvinen <ilpo.jarvinen@linux.intel.com> > > > And kudos for rx_not_done -> pending_rx_int change. It was a minor > thing for me but the logic is so much easier to understand now with > that better name :-). > > Some other nice cleanups compared with v4 also noted. > >
On 2/25/2022 3:10 AM, Ilpo Järvinen wrote: > On Wed, 23 Feb 2022, Ricardo Martinez wrote: > >> From: Haijun Liu <haijun.liu@mediatek.com> >> >> Registers the t7xx device driver with the kernel. Setup all the core >> components: PCIe layer, Modem Host Cross Core Interface (MHCCIF), >> modem control operations, modem state machine, and build >> infrastructure. >> >> * PCIe layer code implements driver probe and removal. >> * MHCCIF provides interrupt channels to communicate events >> such as handshake, PM and port enumeration. >> * Modem control implements the entry point for modem init, >> reset and exit. >> * The modem status monitor is a state machine used by modem control >> to complete initialization and stop. It is used also to propagate >> exception events reported by other components. >> >> Signed-off-by: Haijun Liu <haijun.liu@mediatek.com> >> Signed-off-by: Chandrashekar Devegowda <chandrashekar.devegowda@intel.com> >> Co-developed-by: Ricardo Martinez <ricardo.martinez@linux.intel.com> >> Signed-off-by: Ricardo Martinez <ricardo.martinez@linux.intel.com> >> >> >From a WWAN framework perspective: >> Reviewed-by: Loic Poulain <loic.poulain@linaro.org> >> --- >> + /* IPs enable interrupts when ready */ >> + for (i = 0; i < EXT_INT_NUM; i++) >> + t7xx_pcie_mac_clear_int(t7xx_dev, i); > In v4, PCIE_MAC_MSIX_MSK_SET() wrote to IMASK_HOST_MSIX_SET_GRP0_0. > In v5, t7xx_pcie_mac_clear_int() writes to IMASK_HOST_MSIX_CLR_GRP0_0. > > t7xx_pcie_mac_set_int() would write to IMASK_HOST_MSIX_SET_GRP0_0 > matching to what v4 did. So you probably want to call > t7xx_pcie_mac_set_int() instead of t7xx_pcie_mac_clear_int()? Yes, this should call t7xx_pcie_mac_set_int(). >
On 2/25/2022 3:47 AM, Ilpo Järvinen wrote: > On Wed, 23 Feb 2022, Ricardo Martinez wrote: > >> From: Haijun Liu <haijun.liu@mediatek.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`. >> >> Signed-off-by: Haijun Liu <haijun.liu@mediatek.com> >> Co-developed-by: Chandrashekar Devegowda <chandrashekar.devegowda@intel.com> >> Signed-off-by: Chandrashekar Devegowda <chandrashekar.devegowda@intel.com> >> Co-developed-by: Ricardo Martinez <ricardo.martinez@linux.intel.com> >> Signed-off-by: Ricardo Martinez <ricardo.martinez@linux.intel.com> >> >> >From a WWAN framework perspective: >> Reviewed-by: Loic Poulain <loic.poulain@linaro.org> >> --- ... >> + >> + seq_num = FIELD_GET(CCCI_H_SEQ_FLD, le32_to_cpu(ccci_h->status)); >> + next_seq_num = (seq_num + 1) & FIELD_MAX(CCCI_H_SEQ_FLD); >> + assert_bit = !!(le32_to_cpu(ccci_h->status) & CCCI_H_AST_BIT); >> + if (!assert_bit || port->seq_nums[MTK_RX] > FIELD_MAX(CCCI_H_SEQ_FLD)) > Is this an obfuscated way to say: > ... || port->seq_nums[MTK_RX] == INVALID_SEQ_NUM > ? Right, that condition is true only for the first packet, when seq num is set to INVALID_SEQ_NUM. The next version will explicitly compare seq_nums[MTK_RX] against INVALID_SEQ_NUM.
On 3/1/2022 5:26 AM, Ilpo Järvinen wrote: > On Wed, 23 Feb 2022, Ricardo Martinez wrote: > >> From: Haijun Liu <haijun.liu@mediatek.com> >> >> Implements suspend, resumes, freeze, thaw, poweroff, and restore >> `dev_pm_ops` callbacks. >> >> >From the host point of view, the t7xx driver is one entity. But, the >> device has several modules that need to be addressed in different ways >> during power management (PM) flows. >> The driver uses the term 'PM entities' to refer to the 2 DPMA and >> 2 CLDMA HW blocks that need to be managed during PM flows. >> When a dev_pm_ops function is called, the PM entities list is iterated >> and the matching function is called for each entry in the list. >> >> Signed-off-by: Haijun Liu <haijun.liu@mediatek.com> >> Signed-off-by: Chandrashekar Devegowda <chandrashekar.devegowda@intel.com> >> Co-developed-by: Ricardo Martinez <ricardo.martinez@linux.intel.com> >> Signed-off-by: Ricardo Martinez <ricardo.martinez@linux.intel.com> >> --- >> +static int __t7xx_pci_pm_suspend(struct pci_dev *pdev) >> +{ > ... >> + iowrite32(L1_DISABLE_BIT(0), IREG_BASE(t7xx_dev) + DIS_ASPM_LOWPWR_CLR_0); >> + return 0; > The success path does this same iowrite32 to DIS_ASPM_LOWPWR_CLR_0 > as the failure paths. Is that intended? Yes, that's intended. This function disables low power mode at the beginning and it has to re-enable it before returning, regardless of the success or failure path. The next iteration will contain some naming changes to avoid double negatives : - iowrite32(L1_DISABLE_BIT(0), IREG_BASE(t7xx_dev) + DIS_ASPM_LOWPWR_CLR_0); + iowrite32(T7XX_L1_BIT(0), IREG_BASE(t7xx_dev) + DISABLE_ASPM_LOWPWR); > The function looks much better now! > >
On 3/6/2022 6:56 PM, Sergey Ryazanov wrote: > On Thu, Feb 24, 2022 at 1:35 AM Ricardo Martinez > <ricardo.martinez@linux.intel.com> wrote: >> From: Chandrashekar Devegowda <chandrashekar.devegowda@intel.com> >> >> Adds AT and MBIM ports to the port proxy infrastructure. >> The initialization method is responsible for creating the corresponding >> ports using the WWAN framework infrastructure. The implemented WWAN port >> operations are start, stop, and TX. > [skipped] > >> +static int t7xx_port_ctrl_tx(struct wwan_port *port, struct sk_buff *skb) >> +{ >> + struct t7xx_port *port_private = wwan_port_get_drvdata(port); >> + size_t actual_len, alloc_size, txq_mtu = CLDMA_MTU; >> + struct t7xx_port_static *port_static; >> + unsigned int len, i, packets; >> + struct t7xx_fsm_ctl *ctl; >> + enum md_state md_state; >> + >> + len = skb->len; >> + if (!len || !port_private->rx_length_th || !port_private->chan_enable) >> + return -EINVAL; >> + >> + port_static = port_private->port_static; >> + ctl = port_private->t7xx_dev->md->fsm_ctl; >> + md_state = t7xx_fsm_get_md_state(ctl); >> + if (md_state == MD_STATE_WAITING_FOR_HS1 || md_state == MD_STATE_WAITING_FOR_HS2) { >> + dev_warn(port_private->dev, "Cannot write to %s port when md_state=%d\n", >> + port_static->name, md_state); >> + return -ENODEV; >> + } >> + >> + alloc_size = min_t(size_t, txq_mtu, len + CCCI_HEADROOM); >> + actual_len = alloc_size - CCCI_HEADROOM; >> + packets = DIV_ROUND_UP(len, txq_mtu - CCCI_HEADROOM); >> + >> + for (i = 0; i < packets; i++) { >> + struct ccci_header *ccci_h; >> + struct sk_buff *skb_ccci; >> + int ret; >> + >> + if (packets > 1 && packets == i + 1) { >> + actual_len = len % (txq_mtu - CCCI_HEADROOM); >> + alloc_size = actual_len + CCCI_HEADROOM; >> + } > Why do you track the packet number? Why not track the offset in the > passed data? E.g.: > > for (off = 0; off < len; off += chunklen) { > chunklen = min(len - off, CLDMA_MTU - sizeof(struct ccci_header); > skb_ccci = alloc_skb(chunklen + sizeof(struct ccci_header), ...); > skb_put_data(skb_ccci, skb->data + off, chunklen); > /* Send skb_ccci */ > } Sure, I'll make that change. >> + skb_ccci = __dev_alloc_skb(alloc_size, GFP_KERNEL); >> + if (!skb_ccci) >> + return -ENOMEM; >> + >> + ccci_h = skb_put(skb_ccci, sizeof(*ccci_h)); >> + t7xx_ccci_header_init(ccci_h, 0, actual_len + sizeof(*ccci_h), >> + port_static->tx_ch, 0); >> + skb_put_data(skb_ccci, skb->data + i * (txq_mtu - CCCI_HEADROOM), actual_len); >> + t7xx_port_proxy_set_tx_seq_num(port_private, ccci_h); >> + >> + ret = t7xx_port_send_skb_to_md(port_private, skb_ccci); >> + if (ret) { >> + dev_kfree_skb_any(skb_ccci); >> + dev_err(port_private->dev, "Write error on %s port, %d\n", >> + port_static->name, ret); >> + return ret; >> + } >> + >> + port_private->seq_nums[MTK_TX]++; > Sequence number tracking as well as CCCI header construction are > common operations, so why not move them to t7xx_port_send_skb_to_md()? Sequence number should be set as part of CCCI header construction. I think it's a bit more readable to initialize the CCCI header right after the corresponding skb_put(). Not a big deal, any thoughts? Note that the upcoming fw update feature doesn't require a CCCI header, so we could rename the TX function as t7xx_port_send_ccci_skb_to_md(), this would give a hint that it is taking care of the CCCI header. >> + } >> + >> + dev_kfree_skb(skb); >> + return 0; >> +} > -- > Sergey
On Wed, Mar 9, 2022 at 3:02 AM Martinez, Ricardo <ricardo.martinez@linux.intel.com> wrote: > On 3/6/2022 6:56 PM, Sergey Ryazanov wrote: >> On Thu, Feb 24, 2022 at 1:35 AM Ricardo Martinez >> <ricardo.martinez@linux.intel.com> wrote: >>> From: Chandrashekar Devegowda <chandrashekar.devegowda@intel.com> >>> >>> Adds AT and MBIM ports to the port proxy infrastructure. >>> The initialization method is responsible for creating the corresponding >>> ports using the WWAN framework infrastructure. The implemented WWAN port >>> operations are start, stop, and TX. >> [skipped] >> >>> +static int t7xx_port_ctrl_tx(struct wwan_port *port, struct sk_buff *skb) >>> +{ >>> + struct t7xx_port *port_private = wwan_port_get_drvdata(port); >>> + size_t actual_len, alloc_size, txq_mtu = CLDMA_MTU; >>> + struct t7xx_port_static *port_static; >>> + unsigned int len, i, packets; >>> + struct t7xx_fsm_ctl *ctl; >>> + enum md_state md_state; >>> + >>> + len = skb->len; >>> + if (!len || !port_private->rx_length_th || !port_private->chan_enable) >>> + return -EINVAL; >>> + >>> + port_static = port_private->port_static; >>> + ctl = port_private->t7xx_dev->md->fsm_ctl; >>> + md_state = t7xx_fsm_get_md_state(ctl); >>> + if (md_state == MD_STATE_WAITING_FOR_HS1 || md_state == MD_STATE_WAITING_FOR_HS2) { >>> + dev_warn(port_private->dev, "Cannot write to %s port when md_state=%d\n", >>> + port_static->name, md_state); >>> + return -ENODEV; >>> + } >>> + >>> + alloc_size = min_t(size_t, txq_mtu, len + CCCI_HEADROOM); >>> + actual_len = alloc_size - CCCI_HEADROOM; >>> + packets = DIV_ROUND_UP(len, txq_mtu - CCCI_HEADROOM); >>> + >>> + for (i = 0; i < packets; i++) { >>> + struct ccci_header *ccci_h; >>> + struct sk_buff *skb_ccci; >>> + int ret; >>> + >>> + if (packets > 1 && packets == i + 1) { >>> + actual_len = len % (txq_mtu - CCCI_HEADROOM); >>> + alloc_size = actual_len + CCCI_HEADROOM; >>> + } >> >> Why do you track the packet number? Why not track the offset in the >> passed data? E.g.: >> >> for (off = 0; off < len; off += chunklen) { >> chunklen = min(len - off, CLDMA_MTU - sizeof(struct ccci_header); >> skb_ccci = alloc_skb(chunklen + sizeof(struct ccci_header), ...); >> skb_put_data(skb_ccci, skb->data + off, chunklen); >> /* Send skb_ccci */ >> } > > Sure, I'll make that change. > >>> + skb_ccci = __dev_alloc_skb(alloc_size, GFP_KERNEL); >>> + if (!skb_ccci) >>> + return -ENOMEM; >>> + >>> + ccci_h = skb_put(skb_ccci, sizeof(*ccci_h)); >>> + t7xx_ccci_header_init(ccci_h, 0, actual_len + sizeof(*ccci_h), >>> + port_static->tx_ch, 0); >>> + skb_put_data(skb_ccci, skb->data + i * (txq_mtu - CCCI_HEADROOM), actual_len); >>> + t7xx_port_proxy_set_tx_seq_num(port_private, ccci_h); >>> + >>> + ret = t7xx_port_send_skb_to_md(port_private, skb_ccci); >>> + if (ret) { >>> + dev_kfree_skb_any(skb_ccci); >>> + dev_err(port_private->dev, "Write error on %s port, %d\n", >>> + port_static->name, ret); >>> + return ret; >>> + } >>> + >>> + port_private->seq_nums[MTK_TX]++; >> >> Sequence number tracking as well as CCCI header construction are >> common operations, so why not move them to t7xx_port_send_skb_to_md()? > > Sequence number should be set as part of CCCI header construction. > > I think it's a bit more readable to initialize the CCCI header right > after the corresponding skb_put(). Not a big deal, any thoughts? I do not _think_ creating the CCCI header in the WWAN or CTRL port functions is any good idea. In case of stacked protocols, each layer should create its own header, pass the packet down the stack, and then a next layer will create a next header. In case of the CTRL port, this means that the control port code should take an opaque data block from an upper layer (e.g. features request), prepend it with a control msg header, and pass it down the stack to the port proxy layer, where the CCCI header will be prepended. In case a WWAN port, all headers are passed from user space, so there шы nothing to prepend. And the only remaining function is to fragment a user input, and then pass all the fragments to the port proxy layer, where the CCCI header will be prepended. This way, you do not overload the CTRL/WWAN port with code of other protocols (i.e. CCCI), reduce code duplication. Which in itself improves the code maintainability and future development. Creating a CCCI header at the WWAN port layer is like forcing a user to manually create IP and UDP headers before writing a data block into a network socket :) Anyway, it is up to you to decide exactly how to create headers and assign sequence numbers. I just wanted to point out the code inconsistency. It does not make the code wrong, it just makes the code look stranger. > Note that the upcoming fw update feature doesn't require a CCCI header, > so we could rename the TX function as t7xx_port_send_ccci_skb_to_md(), > this would give a hint that it is taking care of the CCCI header. Does this mean the firmware upgrade does not utilize the channel id, and just pushes data directly to a specific CLDMA queue? In that case it looks like the firmware upgrade code needs to entirely bypass the port proxy layer and communicate directly with CLDMA. Isn't it? >>> + } >>> + >>> + dev_kfree_skb(skb); >>> + return 0; >>> +}
On 3/9/2022 4:13 PM, Sergey Ryazanov wrote: > On Wed, Mar 9, 2022 at 3:02 AM Martinez, Ricardo > <ricardo.martinez@linux.intel.com> wrote: >> On 3/6/2022 6:56 PM, Sergey Ryazanov wrote: >>> On Thu, Feb 24, 2022 at 1:35 AM Ricardo Martinez >>> <ricardo.martinez@linux.intel.com> wrote: >>>> From: Chandrashekar Devegowda <chandrashekar.devegowda@intel.com> >>>> >>>> Adds AT and MBIM ports to the port proxy infrastructure. >>>> The initialization method is responsible for creating the corresponding >>>> ports using the WWAN framework infrastructure. The implemented WWAN port >>>> operations are start, stop, and TX. >>> [skipped] >>> >>>> +static int t7xx_port_ctrl_tx(struct wwan_port *port, struct sk_buff *skb) >>>> +{ >>>> + struct t7xx_port *port_private = wwan_port_get_drvdata(port); >>>> + size_t actual_len, alloc_size, txq_mtu = CLDMA_MTU; >>>> + struct t7xx_port_static *port_static; >>>> + unsigned int len, i, packets; >>>> + struct t7xx_fsm_ctl *ctl; >>>> + enum md_state md_state; >>>> + >>>> + len = skb->len; >>>> + if (!len || !port_private->rx_length_th || !port_private->chan_enable) >>>> + return -EINVAL; >>>> + >>>> + port_static = port_private->port_static; >>>> + ctl = port_private->t7xx_dev->md->fsm_ctl; >>>> + md_state = t7xx_fsm_get_md_state(ctl); >>>> + if (md_state == MD_STATE_WAITING_FOR_HS1 || md_state == MD_STATE_WAITING_FOR_HS2) { >>>> + dev_warn(port_private->dev, "Cannot write to %s port when md_state=%d\n", >>>> + port_static->name, md_state); >>>> + return -ENODEV; >>>> + } >>>> + >>>> + alloc_size = min_t(size_t, txq_mtu, len + CCCI_HEADROOM); >>>> + actual_len = alloc_size - CCCI_HEADROOM; >>>> + packets = DIV_ROUND_UP(len, txq_mtu - CCCI_HEADROOM); >>>> + >>>> + for (i = 0; i < packets; i++) { >>>> + struct ccci_header *ccci_h; >>>> + struct sk_buff *skb_ccci; >>>> + int ret; >>>> + >>>> + if (packets > 1 && packets == i + 1) { >>>> + actual_len = len % (txq_mtu - CCCI_HEADROOM); >>>> + alloc_size = actual_len + CCCI_HEADROOM; >>>> + } >>> Why do you track the packet number? Why not track the offset in the >>> passed data? E.g.: >>> >>> for (off = 0; off < len; off += chunklen) { >>> chunklen = min(len - off, CLDMA_MTU - sizeof(struct ccci_header); >>> skb_ccci = alloc_skb(chunklen + sizeof(struct ccci_header), ...); >>> skb_put_data(skb_ccci, skb->data + off, chunklen); >>> /* Send skb_ccci */ >>> } >> Sure, I'll make that change. >> >>>> + skb_ccci = __dev_alloc_skb(alloc_size, GFP_KERNEL); >>>> + if (!skb_ccci) >>>> + return -ENOMEM; >>>> + >>>> + ccci_h = skb_put(skb_ccci, sizeof(*ccci_h)); >>>> + t7xx_ccci_header_init(ccci_h, 0, actual_len + sizeof(*ccci_h), >>>> + port_static->tx_ch, 0); >>>> + skb_put_data(skb_ccci, skb->data + i * (txq_mtu - CCCI_HEADROOM), actual_len); >>>> + t7xx_port_proxy_set_tx_seq_num(port_private, ccci_h); >>>> + >>>> + ret = t7xx_port_send_skb_to_md(port_private, skb_ccci); >>>> + if (ret) { >>>> + dev_kfree_skb_any(skb_ccci); >>>> + dev_err(port_private->dev, "Write error on %s port, %d\n", >>>> + port_static->name, ret); >>>> + return ret; >>>> + } >>>> + >>>> + port_private->seq_nums[MTK_TX]++; >>> Sequence number tracking as well as CCCI header construction are >>> common operations, so why not move them to t7xx_port_send_skb_to_md()? >> Sequence number should be set as part of CCCI header construction. >> >> I think it's a bit more readable to initialize the CCCI header right >> after the corresponding skb_put(). Not a big deal, any thoughts? > I do not _think_ creating the CCCI header in the WWAN or CTRL port > functions is any good idea. In case of stacked protocols, each layer > should create its own header, pass the packet down the stack, and then > a next layer will create a next header. > > In case of the CTRL port, this means that the control port code should > take an opaque data block from an upper layer (e.g. features request), > prepend it with a control msg header, and pass it down the stack to > the port proxy layer, where the CCCI header will be prepended. > > In case a WWAN port, all headers are passed from user space, so there > шы nothing to prepend. And the only remaining function is to fragment > a user input, and then pass all the fragments to the port proxy > layer, where the CCCI header will be prepended. > > This way, you do not overload the CTRL/WWAN port with code of other > protocols (i.e. CCCI), reduce code duplication. Which in itself > improves the code maintainability and future development. Creating a > CCCI header at the WWAN port layer is like forcing a user to manually > create IP and UDP headers before writing a data block into a network > socket :) > > Anyway, it is up to you to decide exactly how to create headers and > assign sequence numbers. I just wanted to point out the code > inconsistency. It does not make the code wrong, it just makes the code > look stranger. Agree, the next iteration will implement a layered approach. >> Note that the upcoming fw update feature doesn't require a CCCI header, >> so we could rename the TX function as t7xx_port_send_ccci_skb_to_md(), >> this would give a hint that it is taking care of the CCCI header. > Does this mean the firmware upgrade does not utilize the channel id, > and just pushes data directly to a specific CLDMA queue? In that case > it looks like the firmware upgrade code needs to entirely bypass the > port proxy layer and communicate directly with CLDMA. Isn't it? It could bypass port proxy, or it could use a new helper function implemented for the layered approach, this function (t7xx_port_send_raw_skb) sends an skb to the right CLDMA instance and queue based on the port configuration. > >>>> + } >>>> + >>>> + dev_kfree_skb(skb); >>>> + return 0; >>>> +}
On 3/6/2022 6:52 PM, Sergey Ryazanov wrote: > On Thu, Feb 24, 2022 at 1:35 AM Ricardo Martinez > <ricardo.martinez@linux.intel.com> wrote: >> From: Haijun Liu <haijun.liu@mediatek.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. ...
Hi Sergey, On 3/6/2022 6:55 PM, Sergey Ryazanov wrote: > On Thu, Feb 24, 2022 at 1:35 AM Ricardo Martinez > <ricardo.martinez@linux.intel.com> wrote: >> From: Haijun Liu <haijun.liu@mediatek.com> >> >> Control Port implements driver control messages such as modem-host >> handshaking, controls port enumeration, and handles exception messages. >> >> The handshaking process between the driver and the modem happens during >> the init sequence. The process involves the exchange of a list of >> supported runtime features to make sure that modem and host are ready >> to provide proper feature lists including port enumeration. Further >> features can be enabled and controlled in this handshaking process. >> ... >> +static void t7xx_core_hk_handler(struct t7xx_modem *md, struct t7xx_fsm_ctl *ctl, >> + enum t7xx_fsm_event_state event_id, >> + enum t7xx_fsm_event_state err_detect) >> +{ >> + struct t7xx_sys_info *core_info = &md->core_md; >> + struct device *dev = &md->t7xx_dev->pdev->dev; >> + struct t7xx_fsm_event *event, *event_next; >> + unsigned long flags; >> + void *event_data; >> + int ret; >> + >> + t7xx_prepare_host_rt_data_query(core_info); >> + >> + while (!kthread_should_stop()) { >> + bool event_received = false; >> + >> + spin_lock_irqsave(&ctl->event_lock, flags); >> + list_for_each_entry_safe(event, event_next, &ctl->event_queue, entry) { >> + if (event->event_id == err_detect) { >> + list_del(&event->entry); >> + spin_unlock_irqrestore(&ctl->event_lock, flags); >> + dev_err(dev, "Core handshake error event received\n"); >> + goto err_free_event; >> + } else if (event->event_id == event_id) { >> + list_del(&event->entry); >> + event_received = true; >> + break; >> + } >> + } >> + spin_unlock_irqrestore(&ctl->event_lock, flags); >> + >> + if (event_received) >> + break; >> + >> + wait_event_interruptible(ctl->event_wq, !list_empty(&ctl->event_queue) || >> + kthread_should_stop()); >> + if (kthread_should_stop()) >> + goto err_free_event; >> + } >> + >> + if (ctl->exp_flg) >> + goto err_free_event; >> + >> + event_data = (void *)event + sizeof(*event); > In the V2, the event structure has a data field. But then it was > dropped and now the attached data offset is manually calculated. Why > did you do this, why event->data is not suitable here? It was removed along with other zero length arrays, although it was declared as an empty array. The next iteration will use C99 flexible arrays where required, instead of calculating the data offset manually. ...
Hi Sergey, On 3/6/2022 6:58 PM, Sergey Ryazanov wrote: > On Thu, Feb 24, 2022 at 1:35 AM Ricardo Martinez > <ricardo.martinez@linux.intel.com> wrote: >> From: Haijun Liu <haijun.liu@mediatek.com> >> >> Data Path Modem AP Interface (DPMAIF) HIF layer provides methods >> for initialization, ISR, control and event handling of TX/RX flows. >> >> DPMAIF TX >> Exposes the `dmpaif_tx_send_skb` function which can be used by the >> network device to transmit packets. >> The uplink data management uses a Descriptor Ring Buffer (DRB). >> First DRB entry is a message type that will be followed by 1 or more >> normal DRB entries. Message type DRB will hold the skb information >> and each normal DRB entry holds a pointer to the skb payload. >> >> DPMAIF RX >> The downlink buffer management uses Buffer Address Table (BAT) and >> Packet Information Table (PIT) rings. >> The BAT ring holds the address of skb data buffer for the HW to use, >> while the PIT contains metadata about a whole network packet including >> a reference to the BAT entry holding the data buffer address. >> The driver reads the PIT and BAT entries written by the modem, when >> reaching a threshold, the driver will reload the PIT and BAT rings. ... >> +static int t7xx_dpmaif_add_skb_to_ring(struct dpmaif_ctrl *dpmaif_ctrl, struct sk_buff *skb) >> +{ >> + unsigned short cur_idx, drb_wr_idx_backup; >> ... >> + txq = &dpmaif_ctrl->txq[skb_cb->txq_number]; >> ... >> + cur_idx = txq->drb_wr_idx; >> + drb_wr_idx_backup = cur_idx; >> ... >> + for (wr_cnt = 0; wr_cnt < payload_cnt; wr_cnt++) { >> ... >> + bus_addr = dma_map_single(dpmaif_ctrl->dev, data_addr, data_len, DMA_TO_DEVICE); >> + if (dma_mapping_error(dpmaif_ctrl->dev, bus_addr)) { >> + dev_err(dpmaif_ctrl->dev, "DMA mapping fail\n"); >> + atomic_set(&txq->tx_processing, 0); >> + >> + spin_lock_irqsave(&txq->tx_lock, flags); >> + txq->drb_wr_idx = drb_wr_idx_backup; >> + spin_unlock_irqrestore(&txq->tx_lock, flags); > What is the purpose of locking here? The intention is to protect against concurrent access of drb_wr_idx by t7xx_txq_drb_wr_available() > >> + return -ENOMEM; >> + } >> ... >> + } >> ... >> +} >
Hello Ricardo, On Tue, Apr 5, 2022 at 2:29 AM Martinez, Ricardo <ricardo.martinez@linux.intel.com> wrote: > On 3/6/2022 6:58 PM, Sergey Ryazanov wrote: >> On Thu, Feb 24, 2022 at 1:35 AM Ricardo Martinez >> <ricardo.martinez@linux.intel.com> wrote: >>> From: Haijun Liu <haijun.liu@mediatek.com> >>> >>> Data Path Modem AP Interface (DPMAIF) HIF layer provides methods >>> for initialization, ISR, control and event handling of TX/RX flows. >>> >>> DPMAIF TX >>> Exposes the `dmpaif_tx_send_skb` function which can be used by the >>> network device to transmit packets. >>> The uplink data management uses a Descriptor Ring Buffer (DRB). >>> First DRB entry is a message type that will be followed by 1 or more >>> normal DRB entries. Message type DRB will hold the skb information >>> and each normal DRB entry holds a pointer to the skb payload. >>> >>> DPMAIF RX >>> The downlink buffer management uses Buffer Address Table (BAT) and >>> Packet Information Table (PIT) rings. >>> The BAT ring holds the address of skb data buffer for the HW to use, >>> while the PIT contains metadata about a whole network packet including >>> a reference to the BAT entry holding the data buffer address. >>> The driver reads the PIT and BAT entries written by the modem, when >>> reaching a threshold, the driver will reload the PIT and BAT rings. > ... >>> +static int t7xx_dpmaif_add_skb_to_ring(struct dpmaif_ctrl *dpmaif_ctrl, struct sk_buff *skb) >>> +{ >>> + unsigned short cur_idx, drb_wr_idx_backup; >>> ... >>> + txq = &dpmaif_ctrl->txq[skb_cb->txq_number]; >>> ... >>> + cur_idx = txq->drb_wr_idx; >>> + drb_wr_idx_backup = cur_idx; >>> ... >>> + for (wr_cnt = 0; wr_cnt < payload_cnt; wr_cnt++) { >>> ... >>> + bus_addr = dma_map_single(dpmaif_ctrl->dev, data_addr, data_len, DMA_TO_DEVICE); >>> + if (dma_mapping_error(dpmaif_ctrl->dev, bus_addr)) { >>> + dev_err(dpmaif_ctrl->dev, "DMA mapping fail\n"); >>> + atomic_set(&txq->tx_processing, 0); >>> + >>> + spin_lock_irqsave(&txq->tx_lock, flags); >>> + txq->drb_wr_idx = drb_wr_idx_backup; >>> + spin_unlock_irqrestore(&txq->tx_lock, flags); >> >> What is the purpose of locking here? > > The intention is to protect against concurrent access of drb_wr_idx by t7xx_txq_drb_wr_available() t7xx_txq_drb_wr_available() only reads the drb_wr_idx field, why would you serialize that concurrent read? >>> + return -ENOMEM; >>> + } >>> ... >>> + } >>> ... >>> +}
t7xx is the PCIe host device driver for Intel 5G 5000 M.2 solution which is based on MediaTek's T700 modem to provide WWAN connectivity. The driver uses the WWAN framework infrastructure to create the following control ports and network interfaces: * /dev/wwan0mbim0 - Interface conforming to the MBIM protocol. Applications like libmbim [1] or Modem Manager [2] from v1.16 onwards with [3][4] can use it to enable data communication towards WWAN. * /dev/wwan0at0 - Interface that supports AT commands. * wwan0 - Primary network interface for IP traffic. The main blocks in t7xx driver are: * PCIe layer - Implements probe, removal, and power management callbacks. * Port-proxy - Provides a common interface to interact with different types of ports such as WWAN ports. * Modem control & status monitor - Implements the entry point for modem initialization, reset and exit, as well as exception handling. * CLDMA (Control Layer DMA) - Manages the HW used by the port layer to send control messages to the modem using MediaTek's CCCI (Cross-Core Communication Interface) protocol. * DPMAIF (Data Plane Modem AP Interface) - Controls the HW that provides uplink and downlink queues for the data path. The data exchange takes place using circular buffers to share data buffer addresses and metadata to describe the packets. * MHCCIF (Modem Host Cross-Core Interface) - Provides interrupt channels for bidirectional event notification such as handshake, exception, PM and port enumeration. The compilation of the t7xx driver is enabled by the CONFIG_MTK_T7XX config option which depends on CONFIG_WWAN. This driver was originally developed by MediaTek. Intel adapted t7xx to the WWAN framework, optimized and refactored the driver source code in close collaboration with MediaTek. This will enable getting the t7xx driver on the Approved Vendor List for interested OEM's and ODM's productization plans with Intel 5G 5000 M.2 solution. List of contributors: Amir Hanania <amir.hanania@intel.com> Andriy Shevchenko <andriy.shevchenko@linux.intel.com> Chandrashekar Devegowda <chandrashekar.devegowda@intel.com> Dinesh Sharma <dinesh.sharma@intel.com> Eliot Lee <eliot.lee@intel.com> Haijun Liu <haijun.liu@mediatek.com> M Chetan Kumar <m.chetan.kumar@intel.com> Mika Westerberg <mika.westerberg@linux.intel.com> Moises Veleta <moises.veleta@intel.com> Pierre-louis Bossart <pierre-louis.bossart@intel.com> Chiranjeevi Rapolu <chiranjeevi.rapolu@intel.com> Ricardo Martinez <ricardo.martinez@linux.intel.com> Madhusmita Sahu <madhusmita.sahu@intel.com> Muralidharan Sethuraman <muralidharan.sethuraman@intel.com> Soumya Prakash Mishra <Soumya.Prakash.Mishra@intel.com> Sreehari Kancharla <sreehari.kancharla@intel.com> Suresh Nagaraj <suresh.nagaraj@intel.com> [1] https://www.freedesktop.org/software/libmbim/ [2] https://www.freedesktop.org/software/ModemManager/ [3] https://gitlab.freedesktop.org/mobile-broadband/ModemManager/-/merge_requests/582 [4] https://gitlab.freedesktop.org/mobile-broadband/ModemManager/-/merge_requests/523 v5: - Update Intel's copyright years to 2021-2022. - Remove circular dependency between DPMAIF HW (07) and HIF (08). - Keep separate patches for CLDMA (02) and Core (03) but improve the code split by decoupling CLDMA from modem ops and cleaning up t7xx_common.h. - Rename ID_CLDMA0/ID_CLDMA1 to CLDMA_ID_AP/CLDMA_ID_MD. - Consistently use CLDMA's ring_lock to protect tr_ring. - Free resources first and then print messages. - Implement suggested name changes. - Do not explicitly include dev_printk.h. - Remove redundant dev_err()s. - Fix possible memory leak during probe. - Remove infrastructure for legacy interrupts. - Remove unused macros and variables, including those that can be replaced with constants. - Remove PCIE_MAC_MSIX_MSK_SET macro which is duplicated code. - Refactor __t7xx_pci_pm_suspend() for clarity. - Refactor t7xx_cldma_rx_ring_init() and t7xx_cldma_tx_ring_init(). - Do not use & for function callbacks. - Declare a structure to access skb->cb[] data. - Use skb_put_data instead of memcpy. - No need to use kfree_sensitive. - Use dev_kfree_skb() instead of kfree_skb(). - Refactor t7xx_prepare_device_rt_data() to remove potential leaks, avoid unneeded memset and keep rt_data and packet_size updates inside the same 'if' block. - Set port's rx_length_th back to 0 during uninit. - Remove unneeded 'blocking' parameter from t7xx_cldma_send_skb(). - Return -EIO in t7xx_cldma_send_skb() if the queue is inactive. - Refactor t7xx_cldma_qs_are_active() to use pci_device_is_present(). - Simplify t7xx_cldma_stop_q() and rename it to t7xx_cldma_stop_all_qs(). - Fix potential leaks in t7xx_cldma_init(). - Improve error handling in fsm_append_event and fsm_routine_starting(). - Propagate return codes from fsm_append_cmd() and t7xx_fsm_append_event(). - Refactor fsm_wait_for_event() to avoid unnecessary sleep. - Create the WWAN ports and net device only after the modem is in the ready state. - Refactor t7xx_port_proxy_recv_skb() and port_recv_skb(). - Rename t7xx_port_check_rx_seq_num() as t7xx_port_next_rx_seq_num() and fix the seq_num logic to handle overflows. - Declare seq_nums as u16 instead of short. - Use unsigned int for local indexes. - Use min_t instead of the ternary operator. - Refactor the loop in t7xx_dpmaif_rx_data_collect() to avoid a dead condition check. - Use a bitmap (bat_bitmap) instead of an array to keep track of the DRB status. Used in t7xx_dpmaif_avail_pkt_bat_cnt(). - Refactor t7xx_dpmaif_tx_send_skb() to protect tx_submit_skb_cnt with spinlock and remove the misleading tx_drb_available variable. - Consolidate bit operations before endianness conversion. - Use C bit fields in dpmaif_drb_skb struct which is not HW related. - Add back the que_started check in t7xx_select_tx_queue(). - Create a helper function to get the DRB count. - Simplify the use of 'usage' during t7xx_ccmni_close(). - Enforce CCMNI MTU selection with BUILD_BUG_ON() instead of a comment. - Remove t7xx_ccmni_ctrl->capability parameter which remains constant. v4: - Implement list_prev_entry_circular() and list_next_entry_circular() macros. - Remove inline from all c files. - Define ioread32_poll_timeout_atomic() helper macro. - Fix return code for WWAN port tx op. - Allow AT commands fragmentation same as MBIM commands. - Introduce t7xx_common.h file in the first patch. - Rename functions and variables as suggested in v3. - Reduce code duplication by creating fsm_wait_for_event() helper function. - Remove unneeded dev_err in t7xx_fsm_clr_event(). - Remove unused variable last_state from struct t7xx_fsm_ctl. - Remove unused variable txq_select_times from struct dpmaif_ctrl. - Replace ETXTBSY with EBUSY. - Refactor t7xx_dpmaif_rx_buf_alloc() to remove an unneeded allocation. - Fix potential leak at t7xx_dpmaif_rx_frag_alloc(). - Simplify return value handling at t7xx_dpmaif_rx_start(). - Add a helper to handle the common part of CCCI header initialization. - Make sure interrupts are enabled during PM resume. - Add a parameter to t7xx_fsm_append_cmd() to tell if it is in interrupt context. v3: - Avoid unneeded ping-pong changes between patches. - Use t7xx_ prefix in functions. - Use t7xx_ prefix in generic structs where mtk_ or ccci prefix was used. - Update Authors/Contributors header. - Remove skb pools used for control path. - Remove skb pools used for RX data path. - Do not use dedicated TX queue for ACK-only packets. - Remove __packed attribute from GPD structs. - Remove the infrastructure for test and debug ports. - Use the skb control buffer to store metadata. - Get the IP packet type from RX PIT. - Merge variable declaration and simple assignments. - Use preferred coding patterns. - Remove global variables. - Declare HW facing structure members as little endian. - Rename goto tags to describe what is going to be done. - Do not use variable length arrays. - Remove unneeded blank lines source code and kdoc headers. - Use C99 initialization format for port-proxy ports. - Clean up comments. - Review included headers. - Better use of 100 column limit. - Remove unneeded mb() in CLDMA. - Remove unneeded spin locks and atomics. - Handle read_poll_timeout error. - Use dev_err_ratelimited() where required. - Fix resource leak when requesting IRQs. - Use generic DEFAULT_TX_QUEUE_LEN instead custom macro. - Use ETH_DATA_LEN instead of defining WWAN_DEFAULT_MTU. - Use sizeof() instead of defines when the size of structures is required. - Remove unneeded code from netdev: No need to configure HW address length No need to implement .ndo_change_mtu Remove random address generation - Code simplifications by using kernel provided functions and macros such as: module_pci_driver PTR_ERR_OR_ZERO for_each_set_bit pci_device_is_present skb_queue_purge list_prev_entry __ffs64 v2: - Replace pdev->driver->name with dev_driver_string(&pdev->dev). - Replace random_ether_addr() with eth_random_addr(). - Update kernel-doc comment for enum data_policy. - Indicate the driver is 'Supported' instead of 'Maintained'. - Fix the Signed-of-by and Co-developed-by tags in the patches. - Added authors and contributors in the top comment of the src files. Ricardo Martinez (13): list: Add list_next_entry_circular() and list_prev_entry_circular() net: wwan: t7xx: Add control DMA interface net: wwan: t7xx: Add core components net: wwan: t7xx: Add port proxy infrastructure net: wwan: t7xx: Add control port net: wwan: t7xx: Add AT and MBIM WWAN ports net: wwan: t7xx: Data path HW layer net: wwan: t7xx: Add data path interface net: wwan: t7xx: Add WWAN network interface net: wwan: t7xx: Introduce power management net: wwan: t7xx: Runtime PM net: wwan: t7xx: Device deep sleep lock/unlock net: wwan: t7xx: Add maintainers and documentation .../networking/device_drivers/wwan/index.rst | 1 + .../networking/device_drivers/wwan/t7xx.rst | 120 ++ MAINTAINERS | 11 + drivers/net/wwan/Kconfig | 14 + drivers/net/wwan/Makefile | 1 + drivers/net/wwan/t7xx/Makefile | 20 + drivers/net/wwan/t7xx/t7xx_cldma.c | 281 ++++ drivers/net/wwan/t7xx/t7xx_cldma.h | 176 +++ drivers/net/wwan/t7xx/t7xx_common.h | 66 + drivers/net/wwan/t7xx/t7xx_dpmaif.c | 1294 ++++++++++++++++ drivers/net/wwan/t7xx/t7xx_dpmaif.h | 179 +++ drivers/net/wwan/t7xx/t7xx_hif_cldma.c | 1351 +++++++++++++++++ drivers/net/wwan/t7xx/t7xx_hif_cldma.h | 142 ++ drivers/net/wwan/t7xx/t7xx_hif_dpmaif.c | 574 +++++++ drivers/net/wwan/t7xx/t7xx_hif_dpmaif.h | 211 +++ drivers/net/wwan/t7xx/t7xx_hif_dpmaif_rx.c | 1248 +++++++++++++++ drivers/net/wwan/t7xx/t7xx_hif_dpmaif_rx.h | 114 ++ drivers/net/wwan/t7xx/t7xx_hif_dpmaif_tx.c | 729 +++++++++ drivers/net/wwan/t7xx/t7xx_hif_dpmaif_tx.h | 84 + drivers/net/wwan/t7xx/t7xx_mhccif.c | 122 ++ drivers/net/wwan/t7xx/t7xx_mhccif.h | 37 + drivers/net/wwan/t7xx/t7xx_modem_ops.c | 764 ++++++++++ drivers/net/wwan/t7xx/t7xx_modem_ops.h | 87 ++ drivers/net/wwan/t7xx/t7xx_netdev.c | 430 ++++++ drivers/net/wwan/t7xx/t7xx_netdev.h | 56 + drivers/net/wwan/t7xx/t7xx_pci.c | 768 ++++++++++ drivers/net/wwan/t7xx/t7xx_pci.h | 122 ++ drivers/net/wwan/t7xx/t7xx_pcie_mac.c | 263 ++++ drivers/net/wwan/t7xx/t7xx_pcie_mac.h | 31 + drivers/net/wwan/t7xx/t7xx_port.h | 149 ++ drivers/net/wwan/t7xx/t7xx_port_ctrl_msg.c | 205 +++ drivers/net/wwan/t7xx/t7xx_port_proxy.c | 621 ++++++++ drivers/net/wwan/t7xx/t7xx_port_proxy.h | 100 ++ drivers/net/wwan/t7xx/t7xx_port_wwan.c | 210 +++ drivers/net/wwan/t7xx/t7xx_reg.h | 352 +++++ drivers/net/wwan/t7xx/t7xx_state_monitor.c | 550 +++++++ drivers/net/wwan/t7xx/t7xx_state_monitor.h | 125 ++ include/linux/list.h | 26 + 38 files changed, 11634 insertions(+) create mode 100644 Documentation/networking/device_drivers/wwan/t7xx.rst create mode 100644 drivers/net/wwan/t7xx/Makefile create mode 100644 drivers/net/wwan/t7xx/t7xx_cldma.c create mode 100644 drivers/net/wwan/t7xx/t7xx_cldma.h create mode 100644 drivers/net/wwan/t7xx/t7xx_common.h create mode 100644 drivers/net/wwan/t7xx/t7xx_dpmaif.c create mode 100644 drivers/net/wwan/t7xx/t7xx_dpmaif.h create mode 100644 drivers/net/wwan/t7xx/t7xx_hif_cldma.c create mode 100644 drivers/net/wwan/t7xx/t7xx_hif_cldma.h create mode 100644 drivers/net/wwan/t7xx/t7xx_hif_dpmaif.c create mode 100644 drivers/net/wwan/t7xx/t7xx_hif_dpmaif.h create mode 100644 drivers/net/wwan/t7xx/t7xx_hif_dpmaif_rx.c create mode 100644 drivers/net/wwan/t7xx/t7xx_hif_dpmaif_rx.h create mode 100644 drivers/net/wwan/t7xx/t7xx_hif_dpmaif_tx.c create mode 100644 drivers/net/wwan/t7xx/t7xx_hif_dpmaif_tx.h create mode 100644 drivers/net/wwan/t7xx/t7xx_mhccif.c create mode 100644 drivers/net/wwan/t7xx/t7xx_mhccif.h create mode 100644 drivers/net/wwan/t7xx/t7xx_modem_ops.c create mode 100644 drivers/net/wwan/t7xx/t7xx_modem_ops.h create mode 100644 drivers/net/wwan/t7xx/t7xx_netdev.c create mode 100644 drivers/net/wwan/t7xx/t7xx_netdev.h create mode 100644 drivers/net/wwan/t7xx/t7xx_pci.c create mode 100644 drivers/net/wwan/t7xx/t7xx_pci.h create mode 100644 drivers/net/wwan/t7xx/t7xx_pcie_mac.c create mode 100644 drivers/net/wwan/t7xx/t7xx_pcie_mac.h create mode 100644 drivers/net/wwan/t7xx/t7xx_port.h create mode 100644 drivers/net/wwan/t7xx/t7xx_port_ctrl_msg.c create mode 100644 drivers/net/wwan/t7xx/t7xx_port_proxy.c create mode 100644 drivers/net/wwan/t7xx/t7xx_port_proxy.h create mode 100644 drivers/net/wwan/t7xx/t7xx_port_wwan.c create mode 100644 drivers/net/wwan/t7xx/t7xx_reg.h create mode 100644 drivers/net/wwan/t7xx/t7xx_state_monitor.c create mode 100644 drivers/net/wwan/t7xx/t7xx_state_monitor.h