| Message ID | 20220407223629.21487-1-ricardo.martinez@linux.intel.com |
|---|---|
| Headers | show |
| Series | net: wwan: t7xx: PCIe driver for MediaTek M.2 modem | expand |
On 4/12/2022 5:04 AM, Ilpo Järvinen wrote: > On Thu, 7 Apr 2022, Ricardo Martinez 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. >> >> 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> >> >> Reviewed-by: Ilpo Järvinen <ilpo.jarvinen@linux.intel.com> >> +static int t7xx_prepare_device_rt_data(struct t7xx_sys_info *core, struct device *dev, >> + void *data) >> +{ >> + struct feature_query *md_feature = data; >> + struct mtk_runtime_feature *rt_feature; >> + unsigned int i, rt_data_len = 0; >> + struct sk_buff *skb; >> + >> + /* Parse MD runtime data query */ >> + if (le32_to_cpu(md_feature->head_pattern) != MD_FEATURE_QUERY_ID || >> + le32_to_cpu(md_feature->tail_pattern) != MD_FEATURE_QUERY_ID) { >> + dev_err(dev, "Invalid feature pattern: head 0x%x, tail 0x%x\n", >> + le32_to_cpu(md_feature->head_pattern), >> + le32_to_cpu(md_feature->tail_pattern)); >> + return -EINVAL; >> + } >> + >> + for (i = 0; i < FEATURE_COUNT; i++) { >> + if (FIELD_GET(FEATURE_MSK, md_feature->feature_set[i]) != >> + MTK_FEATURE_MUST_BE_SUPPORTED) >> + rt_data_len += sizeof(*rt_feature); >> + } >> + >> + skb = t7xx_ctrl_alloc_skb(rt_data_len); >> + if (!skb) >> + return -ENOMEM; >> + >> + rt_feature = skb_put(skb, rt_data_len); >> + memset(rt_feature, 0, rt_data_len); >> + >> + /* Fill runtime feature */ >> + for (i = 0; i < FEATURE_COUNT; i++) { >> + u8 md_feature_mask = FIELD_GET(FEATURE_MSK, md_feature->feature_set[i]); >> + >> + if (md_feature_mask == MTK_FEATURE_MUST_BE_SUPPORTED) >> + continue; >> + >> + rt_feature->feature_id = i; >> + if (md_feature_mask == MTK_FEATURE_DOES_NOT_EXIST) >> + rt_feature->support_info = md_feature->feature_set[i]; >> + >> + rt_feature++; >> + } >> + >> + /* Send HS3 message to device */ >> + t7xx_port_send_ctl_skb(core->ctl_port, skb, CTL_ID_HS3_MSG, 0); >> + return 0; >> +} >> + >> +static int t7xx_parse_host_rt_data(struct t7xx_fsm_ctl *ctl, struct t7xx_sys_info *core, >> + struct device *dev, void *data, int data_length) >> +{ >> + enum mtk_feature_support_type ft_spt_st, ft_spt_cfg; >> + struct mtk_runtime_feature *rt_feature; >> + int i, offset; >> + >> + offset = sizeof(struct feature_query); >> + for (i = 0; i < FEATURE_COUNT && offset < data_length; i++) { >> + rt_feature = data + offset; >> + offset += sizeof(*rt_feature) + le32_to_cpu(rt_feature->data_len); >> + >> + ft_spt_cfg = FIELD_GET(FEATURE_MSK, core->feature_set[i]); >> + if (ft_spt_cfg != MTK_FEATURE_MUST_BE_SUPPORTED) >> + continue; > Do MTK_FEATURE_MUST_BE_SUPPORTED appear in the host rt_features > (unlike in the device rt_features)? > Yes, in the first step of the handshake protocol, the host creates its rt_feature list with the proper support label and sends the list to the device. t7xx_parse_host_rt_data() is part of the handshake step 2, the host received the response from the device and now it will verify that the host rt_features labeled as MTK_FEATURE_MUST_BE_SUPPORTED are also supported in the device rt_features.
On Thu, 7 Apr 2022, Ricardo Martinez 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 > > Note: This patch does not enable compilation since it has dependencies > such as t7xx_pcie_mac_clear_int()/t7xx_pcie_mac_set_int() and > struct t7xx_pci_dev which are added by the core patch. > > 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> > > Reviewed-by: Ilpo Järvinen <ilpo.jarvinen@linux.intel.com> > +struct cldma_tgpd { > + u8 gpd_flags; > + u8 not_used1; > + u8 not_used2; > + u8 debug_id; > + __le32 next_gpd_ptr_h; > + __le32 next_gpd_ptr_l; > + __le32 data_buff_bd_ptr_h; > + __le32 data_buff_bd_ptr_l; > + __le16 data_buff_len; > + __le16 not_used3; > +}; > + > +struct cldma_rgpd { > + u8 gpd_flags; > + u8 not_used1; > + __le16 data_allow_len; > + __le32 next_gpd_ptr_h; > + __le32 next_gpd_ptr_l; > + __le32 data_buff_bd_ptr_h; > + __le32 data_buff_bd_ptr_l; > + __le16 data_buff_len; > + u8 not_used2; > + u8 debug_id; > +}; A small additional thing... If you put next_gpd_ptr_h, next_gpd_ptr_l, data_buff_bd_ptr_h, and data_buff_bd_ptr_l into another struct inside these structs, t7xx_cldma_tgpd_set_data_ptr() and friends don't require duplicated versions for tgpd and rgpd.
On Fri, Apr 8, 2022 at 1:37 AM Ricardo Martinez <ricardo.martinez@linux.intel.com> wrote: > 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 > > Note: This patch does not enable compilation since it has dependencies > such as t7xx_pcie_mac_clear_int()/t7xx_pcie_mac_set_int() and > struct t7xx_pci_dev which are added by the core patch. > > 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> > > Reviewed-by: Ilpo Järvinen <ilpo.jarvinen@linux.intel.com> [skipped] > diff --git a/drivers/net/wwan/t7xx/t7xx_hif_cldma.c b/drivers/net/wwan/t7xx/t7xx_hif_cldma.c [skipped] > +static int t7xx_cldma_gpd_rx_from_q(struct cldma_queue *queue, int budget, bool *over_budget) > +{ > + struct cldma_ctrl *md_ctrl = queue->md_ctrl; > + unsigned int hwo_polling_count = 0; > + struct t7xx_cldma_hw *hw_info; > + bool rx_not_done = true; > + unsigned long flags; > + int count = 0; > + > + hw_info = &md_ctrl->hw_info; > + > + do { > + struct cldma_request *req; > + struct cldma_rgpd *rgpd; > + struct sk_buff *skb; > + int ret; > + > + req = queue->tr_done; > + if (!req) > + return -ENODATA; > + > + rgpd = req->gpd; > + if ((rgpd->gpd_flags & GPD_FLAGS_HWO) || !req->skb) { > + dma_addr_t gpd_addr; > + > + if (!pci_device_is_present(to_pci_dev(md_ctrl->dev))) { > + dev_err(md_ctrl->dev, "PCIe Link disconnected\n"); > + return -ENODEV; > + } > + > + gpd_addr = ioread64(hw_info->ap_pdn_base + REG_CLDMA_DL_CURRENT_ADDRL_0 + > + queue->index * sizeof(u64)); > + if (req->gpd_addr == gpd_addr || hwo_polling_count++ >= 100) > + return 0; > + > + udelay(1); > + continue; > + } > + > + hwo_polling_count = 0; > + skb = req->skb; > + > + if (req->mapped_buff) { > + dma_unmap_single(md_ctrl->dev, req->mapped_buff, > + t7xx_skb_data_area_size(skb), DMA_FROM_DEVICE); > + req->mapped_buff = 0; > + } > + > + skb->len = 0; > + skb_reset_tail_pointer(skb); > + skb_put(skb, le16_to_cpu(rgpd->data_buff_len)); > + > + ret = md_ctrl->recv_skb(queue, skb); > + if (ret < 0) > + return ret; The execution flow can not be broken here even in case of error. The .recv_skb() callback consumes (frees) skb even if there is an error. But the skb field in req (struct cldma_request) will keep the skb pointer if the function returns from here. And this stale skb pointer will cause a use-after-free or double-free error. I have not dug too deeply into the CLDMA layer and can not suggest any solution. But the error handling path needs to be rechecked. > + req->skb = NULL; > + t7xx_cldma_rgpd_set_data_ptr(rgpd, 0); > + > + spin_lock_irqsave(&queue->ring_lock, flags); > + queue->tr_done = list_next_entry_circular(req, &queue->tr_ring->gpd_ring, entry); > + spin_unlock_irqrestore(&queue->ring_lock, flags); > + req = queue->rx_refill; > + > + ret = t7xx_cldma_alloc_and_map_skb(md_ctrl, req, queue->tr_ring->pkt_size); > + if (ret) > + return ret; > + > + rgpd = req->gpd; > + t7xx_cldma_rgpd_set_data_ptr(rgpd, req->mapped_buff); > + rgpd->data_buff_len = 0; > + rgpd->gpd_flags = GPD_FLAGS_IOC | GPD_FLAGS_HWO; > + > + spin_lock_irqsave(&queue->ring_lock, flags); > + queue->rx_refill = list_next_entry_circular(req, &queue->tr_ring->gpd_ring, entry); > + spin_unlock_irqrestore(&queue->ring_lock, flags); > + > + rx_not_done = ++count < budget || !need_resched(); > + } while (rx_not_done); > + > + *over_budget = true; > + return 0; > +} -- Sergey
On Fri, Apr 8, 2022 at 1:37 AM Ricardo Martinez <ricardo.martinez@linux.intel.com> wrote: > 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> Reviewed-by: Sergey Ryazanov <ryazanov.s.a@gmail.com>
On Fri, Apr 8, 2022 at 1:37 AM Ricardo Martinez <ricardo.martinez@linux.intel.com> wrote: > Port-proxy provides a common interface to interact with different types > of ports. Ports export their configuration via `struct t7xx_port` and > operate as defined by `struct port_ops`. > > 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> [skipped] > diff --git a/drivers/net/wwan/t7xx/t7xx_port_proxy.c b/drivers/net/wwan/t7xx/t7xx_port_proxy.c [skipped] > +static struct t7xx_port_conf t7xx_md_port_conf[] = {}; Please spell this definition in two lines (i.e. move closing brace into next line): +static struct t7xx_port_conf t7xx_md_port_conf[] = { +}; Such spelling in this patch will help you avoid editing the line when you add the first entry in the next (control port introducing) patch: -static struct t7xx_port_conf t7xx_md_port_conf[] = {}; +static struct t7xx_port_conf t7xx_md_port_conf[] = { + { + ... + .name = "t7xx_ctrl", + }, +}; It will become as simple as: static struct t7xx_port_conf t7xx_md_port_conf[] = { + { + ... + .name = "t7xx_ctrl", + }, }; BTW, if the t7xx_md_port_conf contents are not expected to change at run-time, should this array, as well as all pointers to it, be const? [skipped] > +int t7xx_port_enqueue_skb(struct t7xx_port *port, struct sk_buff *skb) > +{ > + unsigned long flags; > + > + spin_lock_irqsave(&port->rx_wq.lock, flags); > + if (port->rx_skb_list.qlen >= port->rx_length_th) { > + spin_unlock_irqrestore(&port->rx_wq.lock, flags); Probably skb should be freed here before returning. The caller assumes that skb will be consumed even in case of error. > + return -ENOBUFS; > + } > + __skb_queue_tail(&port->rx_skb_list, skb); > + spin_unlock_irqrestore(&port->rx_wq.lock, flags); > + > + wake_up_all(&port->rx_wq); > + return 0; > +} [skipped] > +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_conf *port_conf; > + 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_conf = port->port_conf; > + skb_pull(skb, sizeof(*ccci_h)); > + > + ret = port_conf->ops->recv_skb(port, skb); > + if (ret) { > + skb_push(skb, sizeof(*ccci_h)); Header can not be pushed back here, since the .recv_skb() callback consumes (frees) skb even in case of error. See t7xx_port_wwan_recv_skb() and my comment in t7xx_port_enqueue_skb(). Pushing the header back after failure will trigger the use-after-free error. > + return ret; > + } > + > + port->seq_nums[MTK_RX] = seq_num; The expected sequence number updated only on successful .recv_skb() exit. This will trigger the out-of-order seqno warning on a next message after a .recv_skb() failure. Is this intentional behaviour? Maybe the expected sequence number should be updated before the .recv_skb() call? Or even the sequence number update should be moved to t7xx_port_next_rx_seq_num()? > + return 0; > + > +drop_skb: > + dev_kfree_skb_any(skb); > + return 0; > +} -- Sergey
On Fri, Apr 8, 2022 at 1:37 AM Ricardo Martinez <ricardo.martinez@linux.intel.com> wrote: > 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. > > 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> > > Reviewed-by: Ilpo Järvinen <ilpo.jarvinen@linux.intel.com> Reviewed-by: Sergey Ryazanov <ryazanov.s.a@gmail.com>
On Fri, Apr 8, 2022 at 1:37 AM Ricardo Martinez <ricardo.martinez@linux.intel.com> wrote: > 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> Reviewed-by: Sergey Ryazanov <ryazanov.s.a@gmail.com>
On Fri, Apr 8, 2022 at 1:37 AM Ricardo Martinez <ricardo.martinez@linux.intel.com> wrote: > 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> Reviewed-by: Sergey Ryazanov <ryazanov.s.a@gmail.com>
On Fri, Apr 8, 2022 at 1:37 AM Ricardo Martinez <ricardo.martinez@linux.intel.com> wrote: > 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> Reviewed-by: Sergey Ryazanov <ryazanov.s.a@gmail.com> and a small question below. > diff --git a/drivers/net/wwan/t7xx/t7xx_hif_dpmaif_rx.c b/drivers/net/wwan/t7xx/t7xx_hif_dpmaif_rx.c > ... > +static bool t7xx_alloc_and_map_skb_info(const struct dpmaif_ctrl *dpmaif_ctrl, > + const unsigned int size, struct dpmaif_bat_skb *cur_skb) > +{ > + dma_addr_t data_bus_addr; > + struct sk_buff *skb; > + size_t data_len; > + > + skb = __dev_alloc_skb(size, GFP_KERNEL); > + if (!skb) > + return false; > + > + data_len = skb_end_pointer(skb) - skb->data; Earlier you use a nice t7xx_skb_data_area_size() function here, but now you opencode it. Is it a consequence of t7xx_common.h removing? I would even encourage you to make this function common and place it into include/linux/skbuff.h with a dedicated patch and call it something like skb_data_size(). Surprisingly, there is no such helper function in the kernel, and several other drivers will benefit from it: $ grep -rn 'skb_end_pointer(.*) [-]' drivers/net/ drivers/net/ethernet/marvell/mv643xx_eth.c:628: size = skb_end_pointer(skb) - skb->data; drivers/net/ethernet/marvell/pxa168_eth.c:322: size = skb_end_pointer(skb) - skb->data; drivers/net/ethernet/micrel/ksz884x.c:4764: if (skb_end_pointer(skb) - skb->data >= 50) { drivers/net/ethernet/netronome/nfp/ccm_mbox.c:492: undersize = max_reply_size - (skb_end_pointer(skb) - skb->data); drivers/net/ethernet/nvidia/forcedeth.c:2073: (skb_end_pointer(np->rx_skb[i].skb) - drivers/net/ethernet/nvidia/forcedeth.c:5238: (skb_end_pointer(tx_skb) - tx_skb->data), drivers/net/veth.c:767: frame_sz = skb_end_pointer(skb) - skb->head; drivers/net/wwan/t7xx/t7xx_hif_cldma.c:106: return skb_end_pointer(skb) - skb->data; drivers/net/wwan/t7xx/t7xx_hif_dpmaif_rx.c:160: data_len = skb_end_pointer(skb) - skb->data; > + data_bus_addr = dma_map_single(dpmaif_ctrl->dev, skb->data, data_len, DMA_FROM_DEVICE); > + if (dma_mapping_error(dpmaif_ctrl->dev, data_bus_addr)) { > + dev_err_ratelimited(dpmaif_ctrl->dev, "DMA mapping error\n"); > + dev_kfree_skb_any(skb); > + return false; > + } > + > + cur_skb->skb = skb; > + cur_skb->data_bus_addr = data_bus_addr; > + cur_skb->data_len = data_len; > + > + return true; > +} -- Sergey
On Fri, Apr 8, 2022 at 1:37 AM Ricardo Martinez <ricardo.martinez@linux.intel.com> wrote: > 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 and change MTU. > > 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> Reviewed-by: Sergey Ryazanov <ryazanov.s.a@gmail.com>
On Fri, Apr 8, 2022 at 1:37 AM Ricardo Martinez <ricardo.martinez@linux.intel.com> wrote: > Adds maintainers and documentation for MediaTek t7xx 5G WWAN modem > device driver. > > Signed-off-by: Ricardo Martinez <ricardo.martinez@linux.intel.com> > > From a WWAN framework perspective: > Reviewed-by: Loic Poulain <loic.poulain@linaro.org> Reviewed-by: Sergey Ryazanov <ryazanov.s.a@gmail.com>
Hello Ricardo, Loic, Ilpo, On Fri, Apr 8, 2022 at 1:37 AM Ricardo Martinez <ricardo.martinez@linux.intel.com> wrote: > ... > 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> > > Reviewed-by: Ilpo Järvinen <ilpo.jarvinen@linux.intel.com> This line with "From a WWAN framework perspective" looks confusing to me. Anyone not familiar with all of the iterations will be in doubt as to whether it belongs only to Loic's review or to both of them. How about to format this block like this: > Co-developed-by: Ricardo Martinez <ricardo.martinez@linux.intel.com> > Signed-off-by: Ricardo Martinez <ricardo.martinez@linux.intel.com> > Reviewed-by: Loic Poulain <loic.poulain@linaro.org> (WWAN framework) > Reviewed-by: Ilpo Järvinen <ilpo.jarvinen@linux.intel.com> or like this: > Co-developed-by: Ricardo Martinez <ricardo.martinez@linux.intel.com> > Signed-off-by: Ricardo Martinez <ricardo.martinez@linux.intel.com> > Reviewed-by: Loic Poulain <loic.poulain@linaro.org> # WWAN framework > Reviewed-by: Ilpo Järvinen <ilpo.jarvinen@linux.intel.com> Parentheses vs. comment sign. I saw people use both of these formats, I just do not know which is better. What do you think? -- Sergey
On Tue, 26 Apr 2022, Sergey Ryazanov wrote: > On Fri, Apr 8, 2022 at 1:37 AM Ricardo Martinez > <ricardo.martinez@linux.intel.com> wrote: > > 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> > > Reviewed-by: Sergey Ryazanov <ryazanov.s.a@gmail.com> > > and a small question below. > > > diff --git a/drivers/net/wwan/t7xx/t7xx_hif_dpmaif_rx.c b/drivers/net/wwan/t7xx/t7xx_hif_dpmaif_rx.c > > ... > > +static bool t7xx_alloc_and_map_skb_info(const struct dpmaif_ctrl *dpmaif_ctrl, > > + const unsigned int size, struct dpmaif_bat_skb *cur_skb) > > +{ > > + dma_addr_t data_bus_addr; > > + struct sk_buff *skb; > > + size_t data_len; > > + > > + skb = __dev_alloc_skb(size, GFP_KERNEL); > > + if (!skb) > > + return false; > > + > > + data_len = skb_end_pointer(skb) - skb->data; > > Earlier you use a nice t7xx_skb_data_area_size() function here, but > now you opencode it. Is it a consequence of t7xx_common.h removing? > > I would even encourage you to make this function common and place it > into include/linux/skbuff.h with a dedicated patch and call it > something like skb_data_size(). Surprisingly, there is no such helper > function in the kernel, and several other drivers will benefit from > it: I agree other than the name. IMHO, skb_data_size sounds too much "data size" which it exactly isn't but just how large the memory area is (we already have "datalen" anyway and on language level, those two don't sound different at all). The memory area allocated may or may not have actual data in it, I suggested adding "area" into it.
On Tue, Apr 26, 2022 at 10:30 AM Ilpo Järvinen <ilpo.jarvinen@linux.intel.com> wrote: > On Tue, 26 Apr 2022, Sergey Ryazanov wrote: >> On Fri, Apr 8, 2022 at 1:37 AM Ricardo Martinez >> <ricardo.martinez@linux.intel.com> wrote: >>> 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> >> >> Reviewed-by: Sergey Ryazanov <ryazanov.s.a@gmail.com> >> >> and a small question below. >> >>> diff --git a/drivers/net/wwan/t7xx/t7xx_hif_dpmaif_rx.c b/drivers/net/wwan/t7xx/t7xx_hif_dpmaif_rx.c >>> ... >>> +static bool t7xx_alloc_and_map_skb_info(const struct dpmaif_ctrl *dpmaif_ctrl, >>> + const unsigned int size, struct dpmaif_bat_skb *cur_skb) >>> +{ >>> + dma_addr_t data_bus_addr; >>> + struct sk_buff *skb; >>> + size_t data_len; >>> + >>> + skb = __dev_alloc_skb(size, GFP_KERNEL); >>> + if (!skb) >>> + return false; >>> + >>> + data_len = skb_end_pointer(skb) - skb->data; >> >> Earlier you use a nice t7xx_skb_data_area_size() function here, but >> now you opencode it. Is it a consequence of t7xx_common.h removing? >> >> I would even encourage you to make this function common and place it >> into include/linux/skbuff.h with a dedicated patch and call it >> something like skb_data_size(). Surprisingly, there is no such helper >> function in the kernel, and several other drivers will benefit from >> it: > > I agree other than the name. IMHO, skb_data_size sounds too much "data > size" which it exactly isn't but just how large the memory area is (we > already have "datalen" anyway and on language level, those two don't sound > different at all). The memory area allocated may or may not have actual > data in it, I suggested adding "area" into it. I agree, using the "area" word in the helper name gives more clue about its purpose, thanks. -- Sergey
Hello Moises, On Tue, Apr 26, 2022 at 10:46 PM Veleta, Moises <moises.veleta@intel.com> wrote: > From: Sergey Ryazanov <ryazanov.s.a@gmail.com> > Sent: Monday, April 25, 2022 4:53 PM > To: Ricardo Martinez <ricardo.martinez@linux.intel.com> > Cc: netdev@vger.kernel.org <netdev@vger.kernel.org>; linux-wireless@vger.kernel.org <linux-wireless@vger.kernel.org>; Jakub Kicinski <kuba@kernel.org>; David Miller <davem@davemloft.net>; Johannes Berg <johannes@sipsolutions.net>; Loic Poulain <loic.poulain@linaro.org>; Kumar, M Chetan <m.chetan.kumar@intel.com>; Devegowda, Chandrashekar <chandrashekar.devegowda@intel.com>; linuxwwan <linuxwwan@intel.com>; chiranjeevi.rapolu@linux.intel.com <chiranjeevi.rapolu@linux.intel.com>; Liu, Haijun <haijun.liu@mediatek.com>; Hanania, Amir <amir.hanania@intel.com>; Andy Shevchenko <andriy.shevchenko@linux.intel.com>; Sharma, Dinesh <dinesh.sharma@intel.com>; Lee, Eliot <eliot.lee@intel.com>; Jarvinen, Ilpo Johannes <ilpo.johannes.jarvinen@intel.com>; Veleta, Moises <moises.veleta@intel.com>; Bossart, Pierre-louis <pierre-louis.bossart@intel.com>; Sethuraman, Muralidharan <muralidharan.sethuraman@intel.com>; Mishra, Soumya Prakash <soumya.prakash.mishra@intel.com>; Kancharla, Sreehari <sreehari.kancharla@intel.com>; Sahu, Madhusmita <madhusmita.sahu@intel.com> > Subject: Re: [PATCH net-next v6 04/13] net: wwan: t7xx: Add port proxy infrastructure > > On Fri, Apr 8, 2022 at 1:37 AM Ricardo Martinez > <ricardo.martinez@linux.intel.com> wrote: >> Port-proxy provides a common interface to interact with different types >> of ports. Ports export their configuration via `struct t7xx_port` and >> operate as defined by `struct port_ops`. >> >> 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> > > [skipped] > >> +int t7xx_port_enqueue_skb(struct t7xx_port *port, struct sk_buff *skb) >> +{ >> + unsigned long flags; >> + >> + spin_lock_irqsave(&port->rx_wq.lock, flags); >> + if (port->rx_skb_list.qlen >= port->rx_length_th) { >> + spin_unlock_irqrestore(&port->rx_wq.lock, flags); > > Probably skb should be freed here before returning. The caller assumes > that skb will be consumed even in case of error. > > [MV] We do not drop port ctrl messages. We keep them and try again later. > Whereas WWAN skbs are dropped if conditions are met. I missed that the WWAN port returns no error when it drops the skb. And then I concluded that any failure to process the CCCI message should be accomplished with the skb freeing. Now the handling of CCCI messages is more clear to me. Thank you for the clarifications! To avoid similar misinterpretation in the future, I thought that the skb freeing in the WWAN port worth a comment. Something to describe that despite dropping the message, the return code is zero, indicating skb consumption. Similarly in this (t7xx_port_enqueue_skb) function. Something like: "return an error here, the caller will try again later". And maybe in t7xx_cldma_gpd_rx_from_q() near the loop break after the .skb_recv() failure test. Something like: "break message processing for now will try this message later". What do you think? >> + return -ENOBUFS; >> + } >> + __skb_queue_tail(&port->rx_skb_list, skb); >> + spin_unlock_irqrestore(&port->rx_wq.lock, flags); >> + >> + wake_up_all(&port->rx_wq); >> + return 0; >> +} > > [skipped] > >> +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_conf *port_conf; >> + 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_conf = port->port_conf; >> + skb_pull(skb, sizeof(*ccci_h)); >> + >> + ret = port_conf->ops->recv_skb(port, skb); >> + if (ret) { >> + skb_push(skb, sizeof(*ccci_h)); > > Header can not be pushed back here, since the .recv_skb() callback > consumes (frees) skb even in case of error. See > t7xx_port_wwan_recv_skb() and my comment in t7xx_port_enqueue_skb(). > Pushing the header back after failure will trigger the use-after-free > error. > > [MV] Since t7xx_port_wwan_recv_skb returns 0 when dropping a skb, it skips this push operation and > will not trigger the error stated. Inside of that function an error is printed. > >> + return ret; >> + } >> + >> + port->seq_nums[MTK_RX] = seq_num; > > The expected sequence number updated only on successful .recv_skb() > exit. This will trigger the out-of-order seqno warning on a next > message after a .recv_skb() failure. Is this intentional behaviour? > > Maybe the expected sequence number should be updated before the > .recv_skb() call? Or even the sequence number update should be moved > to t7xx_port_next_rx_seq_num()? > > [MV] For t7xx_port_wwan_recv_skb, it drops skb and seq_nums is updated. > for t7xx_port_enqueue_skb, it retains skb and can be processed again later, skipping this update upon error. Now this is clear to me. Thanks. >> + return 0; >> + >> +drop_skb: >> + dev_kfree_skb_any(skb); >> + return 0; >> +}
On Wed, Apr 27, 2022 at 4:14 AM Veleta, Moises <moises.veleta@intel.com> wrote: > From: Sergey Ryazanov <ryazanov.s.a@gmail.com> > Sent: Tuesday, April 26, 2022 4:06 PM > To: Veleta, Moises <moises.veleta@intel.com> > Cc: Ricardo Martinez <ricardo.martinez@linux.intel.com>; netdev@vger.kernel.org <netdev@vger.kernel.org>; linux-wireless@vger.kernel.org <linux-wireless@vger.kernel.org>; Jakub Kicinski <kuba@kernel.org>; David Miller <davem@davemloft.net>; Johannes Berg <johannes@sipsolutions.net>; Loic Poulain <loic.poulain@linaro.org>; Kumar, M Chetan <m.chetan.kumar@intel.com>; Devegowda, Chandrashekar <chandrashekar.devegowda@intel.com>; linuxwwan <linuxwwan@intel.com>; chiranjeevi.rapolu@linux.intel.com <chiranjeevi.rapolu@linux.intel.com>; Liu, Haijun <haijun.liu@mediatek.com>; Hanania, Amir <amir.hanania@intel.com>; Andy Shevchenko <andriy.shevchenko@linux.intel.com>; Sharma, Dinesh <dinesh.sharma@intel.com>; Lee, Eliot <eliot.lee@intel.com>; Jarvinen, Ilpo Johannes <ilpo.johannes.jarvinen@intel.com>; Bossart, Pierre-louis <pierre-louis.bossart@intel.com>; Sethuraman, Muralidharan <muralidharan.sethuraman@intel.com>; Mishra, Soumya Prakash <soumya.prakash.mishra@intel.com>; Kancharla, Sreehari <sreehari.kancharla@intel.com>; Sahu, Madhusmita <madhusmita.sahu@intel.com> > Subject: Re: [PATCH net-next v6 04/13] net: wwan: t7xx: Add port proxy infrastructure > > Hello Moises, > > On Tue, Apr 26, 2022 at 10:46 PM Veleta, Moises <moises.veleta@intel.com> wrote: >> From: Sergey Ryazanov <ryazanov.s.a@gmail.com> >> Sent: Monday, April 25, 2022 4:53 PM >> To: Ricardo Martinez <ricardo.martinez@linux.intel.com> >> Cc: netdev@vger.kernel.org <netdev@vger.kernel.org>; linux-wireless@vger.kernel.org <linux-wireless@vger.kernel.org>; Jakub Kicinski <kuba@kernel.org>; David Miller <davem@davemloft.net>; Johannes Berg <johannes@sipsolutions.net>; Loic Poulain <loic.poulain@linaro.org>; Kumar, M Chetan <m.chetan.kumar@intel.com>; Devegowda, Chandrashekar <chandrashekar.devegowda@intel.com>; linuxwwan <linuxwwan@intel.com>; chiranjeevi.rapolu@linux.intel.com <chiranjeevi.rapolu@linux.intel.com>; Liu, Haijun <haijun.liu@mediatek.com>; Hanania, Amir <amir.hanania@intel.com>; Andy Shevchenko <andriy.shevchenko@linux.intel.com>; Sharma, Dinesh <dinesh.sharma@intel.com>; Lee, Eliot <eliot.lee@intel.com>; Jarvinen, Ilpo Johannes <ilpo.johannes.jarvinen@intel.com>; Veleta, Moises <moises.veleta@intel.com>; Bossart, Pierre-louis <pierre-louis.bossart@intel.com>; Sethuraman, Muralidharan <muralidharan.sethuraman@intel.com>; Mishra, Soumya Prakash <soumya.prakash.mishra@intel.com>; Kancharla, Sreehari <sreehari.kancharla@intel.com>; Sahu, Madhusmita <madhusmita.sahu@intel.com> >> Subject: Re: [PATCH net-next v6 04/13] net: wwan: t7xx: Add port proxy infrastructure >> >> On Fri, Apr 8, 2022 at 1:37 AM Ricardo Martinez >> <ricardo.martinez@linux.intel.com> wrote: >>> Port-proxy provides a common interface to interact with different types >>> of ports. Ports export their configuration via `struct t7xx_port` and >>> operate as defined by `struct port_ops`. >>> >>> 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> >> >> [skipped] >> >>> +int t7xx_port_enqueue_skb(struct t7xx_port *port, struct sk_buff *skb) >>> +{ >>> + unsigned long flags; >>> + >>> + spin_lock_irqsave(&port->rx_wq.lock, flags); >>> + if (port->rx_skb_list.qlen >= port->rx_length_th) { >>> + spin_unlock_irqrestore(&port->rx_wq.lock, flags); >> >> Probably skb should be freed here before returning. The caller assumes >> that skb will be consumed even in case of error. >> >> [MV] We do not drop port ctrl messages. We keep them and try again later. >> Whereas WWAN skbs are dropped if conditions are met. > > I missed that the WWAN port returns no error when it drops the skb. > And then I concluded that any failure to process the CCCI message > should be accomplished with the skb freeing. Now the handling of CCCI > messages is more clear to me. Thank you for the clarifications! > > To avoid similar misinterpretation in the future, I thought that the > skb freeing in the WWAN port worth a comment. Something to describe > that despite dropping the message, the return code is zero, indicating > skb consumption. Similarly in this (t7xx_port_enqueue_skb) function. > Something like: "return an error here, the caller will try again > later". And maybe in t7xx_cldma_gpd_rx_from_q() near the loop break > after the .skb_recv() failure test. Something like: "break message > processing for now will try this message later". > > What do you think? > > Yes, that would remove the unintended obfuscation . Unless, you think this approach is inappropriate > and should be refactored. Otherwise, I will proceed with adding those clarifying comments. > Thank you. I am Ok with the current approach. It does not contain obvious errors. It might look puzzled, but proper comments should solve this.
On Tue, 26 Apr 2022 at 02:19, Sergey Ryazanov <ryazanov.s.a@gmail.com> wrote: > > Hello Ricardo, Loic, Ilpo, > > On Fri, Apr 8, 2022 at 1:37 AM Ricardo Martinez > <ricardo.martinez@linux.intel.com> wrote: > > ... > > 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> > > > > Reviewed-by: Ilpo Järvinen <ilpo.jarvinen@linux.intel.com> > > This line with "From a WWAN framework perspective" looks confusing to > me. Anyone not familiar with all of the iterations will be in doubt as > to whether it belongs only to Loic's review or to both of them. > > How about to format this block like this: > > > Co-developed-by: Ricardo Martinez <ricardo.martinez@linux.intel.com> > > Signed-off-by: Ricardo Martinez <ricardo.martinez@linux.intel.com> > > Reviewed-by: Loic Poulain <loic.poulain@linaro.org> (WWAN framework) > > Reviewed-by: Ilpo Järvinen <ilpo.jarvinen@linux.intel.com> > > or like this: > > > Co-developed-by: Ricardo Martinez <ricardo.martinez@linux.intel.com> > > Signed-off-by: Ricardo Martinez <ricardo.martinez@linux.intel.com> > > Reviewed-by: Loic Poulain <loic.poulain@linaro.org> # WWAN framework > > Reviewed-by: Ilpo Järvinen <ilpo.jarvinen@linux.intel.com> > > Parentheses vs. comment sign. I saw people use both of these formats, > I just do not know which is better. What do you think? My initial comment was to highlight that someone else should double check the network code, but it wasn't expected to end up in the commit message. Maybe simply drop this extra comment? Regards, Loic
On Wed, Apr 27, 2022 at 3:35 PM Loic Poulain <loic.poulain@linaro.org> wrote: > On Tue, 26 Apr 2022 at 02:19, Sergey Ryazanov <ryazanov.s.a@gmail.com> wrote: >> On Fri, Apr 8, 2022 at 1:37 AM Ricardo Martinez >> <ricardo.martinez@linux.intel.com> wrote: >>> ... >>> 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> >>> >>> Reviewed-by: Ilpo Järvinen <ilpo.jarvinen@linux.intel.com> >> >> This line with "From a WWAN framework perspective" looks confusing to >> me. Anyone not familiar with all of the iterations will be in doubt as >> to whether it belongs only to Loic's review or to both of them. >> >> How about to format this block like this: >> >>> Co-developed-by: Ricardo Martinez <ricardo.martinez@linux.intel.com> >>> Signed-off-by: Ricardo Martinez <ricardo.martinez@linux.intel.com> >>> Reviewed-by: Loic Poulain <loic.poulain@linaro.org> (WWAN framework) >>> Reviewed-by: Ilpo Järvinen <ilpo.jarvinen@linux.intel.com> >> >> or like this: >> >>> Co-developed-by: Ricardo Martinez <ricardo.martinez@linux.intel.com> >>> Signed-off-by: Ricardo Martinez <ricardo.martinez@linux.intel.com> >>> Reviewed-by: Loic Poulain <loic.poulain@linaro.org> # WWAN framework >>> Reviewed-by: Ilpo Järvinen <ilpo.jarvinen@linux.intel.com> >> >> Parentheses vs. comment sign. I saw people use both of these formats, >> I just do not know which is better. What do you think? > > My initial comment was to highlight that someone else should double > check the network code, but it wasn't expected to end up in the commit > message. Maybe simply drop this extra comment? Yep, this drastically solves the problem with comment format :) I do not mind.
On 4/26/2022 1:00 AM, Sergey Ryazanov wrote: > On Tue, Apr 26, 2022 at 10:30 AM Ilpo Järvinen > <ilpo.jarvinen@linux.intel.com> wrote: >> On Tue, 26 Apr 2022, Sergey Ryazanov wrote: >>> On Fri, Apr 8, 2022 at 1:37 AM Ricardo Martinez >>> <ricardo.martinez@linux.intel.com> wrote: >>>> 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> >>> Reviewed-by: Sergey Ryazanov <ryazanov.s.a@gmail.com> >>> >>> and a small question below. >>> >>>> diff --git a/drivers/net/wwan/t7xx/t7xx_hif_dpmaif_rx.c b/drivers/net/wwan/t7xx/t7xx_hif_dpmaif_rx.c >>>> ... >>>> +static bool t7xx_alloc_and_map_skb_info(const struct dpmaif_ctrl *dpmaif_ctrl, >>>> + const unsigned int size, struct dpmaif_bat_skb *cur_skb) >>>> +{ >>>> + dma_addr_t data_bus_addr; >>>> + struct sk_buff *skb; >>>> + size_t data_len; >>>> + >>>> + skb = __dev_alloc_skb(size, GFP_KERNEL); >>>> + if (!skb) >>>> + return false; >>>> + >>>> + data_len = skb_end_pointer(skb) - skb->data; >>> Earlier you use a nice t7xx_skb_data_area_size() function here, but >>> now you opencode it. Is it a consequence of t7xx_common.h removing? >>> >>> I would even encourage you to make this function common and place it >>> into include/linux/skbuff.h with a dedicated patch and call it >>> something like skb_data_size(). Surprisingly, there is no such helper >>> function in the kernel, and several other drivers will benefit from >>> it: >> I agree other than the name. IMHO, skb_data_size sounds too much "data >> size" which it exactly isn't but just how large the memory area is (we >> already have "datalen" anyway and on language level, those two don't sound >> different at all). The memory area allocated may or may not have actual >> data in it, I suggested adding "area" into it. > I agree, using the "area" word in the helper name gives more clue > about its purpose, thanks. > Sounds good. I'll add a patch to introduce skb_data_area_size(), I'm not planning to update other drivers to use it, at least in this series. Please let me know if you think otherwise.
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 V6: - Remove unneeded initializations and bit masks. - Remove t7xx_common.h file. - Add comment to circular linking in GPD list. - Use min instead of min_t. - Use int for local indexes instead of short or char. - Update the commit message in CLDMA patch about dependencies on core patch. - Add space between contributor name and email address. - Rename registers with double negatives e.g. DIS_ASPM_LOWPWR_CLR_0 -> ENABLE_ASPM_LOWPWR. - Fix a race condition in pci sleep resource locking. - Initialize interrupts with t7xx_pcie_mac_set_int() instead of 'clear'. - Remove duplicate spin_lock_init(&md->exp_lock). - Remove .ndo_select_queue callback due to singular TX queue. - Remove call to deprecated netif_rx_any_context(). - Fix include guard name in t7xx_hif_dpmaif.h. - Remove unused q_num parameter in DPMAIF functions. - Do not serialize the drb_wr_idx write in t7xx_dpmaif_add_skb_to_ring() and the read from t7xx_txq_drb_wr_available(). - Fix potential leak in t7xx_dpmaif_add_skb_to_ring(). - Unionize: DRB structs: msg and pd. PIT structs: msg and pd. - Replace list_head & spinlock with skb_buff_head in dpmaif_tx_queue. - Remove rx_length_th check in TX WWAN port flow. - Remove wwan_remove_port() from the critical section in WWAN port uninit. - Use skb_end_pointer() to avoid conditional compilation. - Simplify the loop in t7xx_port_ctrl_tx() by checking the buffer offset instead of calculating the number of required packets. - Remove the code for unused channel PORT_CH_STATUS_RX. - Remove bit flags from ports. Ports can check chan_enable instead of the PORT_F_RX_ALLOW_DROP flag. - Use INVALID_SEQ_NUM to identify the first seq number. - Rename port_static to port_conf and ports_private to ports. - Implement t7xx_port_send_skb() and t7xx_port_send_ctl_skb() in a layered approach to reduce duplicated code and simplify the CCCI header handling. - Move wwan_port_rx() call from port-proxy to WWAN port. - Rename t7xx_port_recv_skb() to t7xx_port_enqueue_skb(). - Move control message parsing logic from port-proxy to control port, preserve the endianness when parsing the message and make port-proxy export a function to enable/disable ports. - Use flexible arrays for: port-proxy ports. payload data in t7xx_fsm_event, port_msg, and mtk_runtime_feature. 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 | 180 +++ drivers/net/wwan/t7xx/t7xx_dpmaif.c | 1283 ++++++++++++++++ drivers/net/wwan/t7xx/t7xx_dpmaif.h | 179 +++ drivers/net/wwan/t7xx/t7xx_hif_cldma.c | 1357 +++++++++++++++++ drivers/net/wwan/t7xx/t7xx_hif_cldma.h | 141 ++ drivers/net/wwan/t7xx/t7xx_hif_dpmaif.c | 574 +++++++ drivers/net/wwan/t7xx/t7xx_hif_dpmaif.h | 208 +++ drivers/net/wwan/t7xx/t7xx_hif_dpmaif_rx.c | 1246 +++++++++++++++ drivers/net/wwan/t7xx/t7xx_hif_dpmaif_rx.h | 116 ++ drivers/net/wwan/t7xx/t7xx_hif_dpmaif_tx.c | 691 +++++++++ drivers/net/wwan/t7xx/t7xx_hif_dpmaif_tx.h | 79 + drivers/net/wwan/t7xx/t7xx_mhccif.c | 122 ++ drivers/net/wwan/t7xx/t7xx_mhccif.h | 37 + drivers/net/wwan/t7xx/t7xx_modem_ops.c | 727 +++++++++ drivers/net/wwan/t7xx/t7xx_modem_ops.h | 88 ++ drivers/net/wwan/t7xx/t7xx_netdev.c | 423 +++++ drivers/net/wwan/t7xx/t7xx_netdev.h | 55 + drivers/net/wwan/t7xx/t7xx_pci.c | 761 +++++++++ drivers/net/wwan/t7xx/t7xx_pci.h | 120 ++ drivers/net/wwan/t7xx/t7xx_pcie_mac.c | 262 ++++ drivers/net/wwan/t7xx/t7xx_pcie_mac.h | 31 + drivers/net/wwan/t7xx/t7xx_port.h | 139 ++ drivers/net/wwan/t7xx/t7xx_port_ctrl_msg.c | 273 ++++ drivers/net/wwan/t7xx/t7xx_port_proxy.c | 510 +++++++ drivers/net/wwan/t7xx/t7xx_port_proxy.h | 98 ++ drivers/net/wwan/t7xx/t7xx_port_wwan.c | 180 +++ drivers/net/wwan/t7xx/t7xx_reg.h | 350 +++++ drivers/net/wwan/t7xx/t7xx_state_monitor.c | 550 +++++++ drivers/net/wwan/t7xx/t7xx_state_monitor.h | 135 ++ include/linux/list.h | 26 + 37 files changed, 11389 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_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