On 3/6/2022 6:56 PM, Sergey Ryazanov wrote:
On Thu, Feb 24, 2022 at 1:35 AM Ricardo Martinez
<ricardo.martinez@xxxxxxxxxxxxxxx> wrote:
From: Chandrashekar Devegowda <chandrashekar.devegowda@xxxxxxxxx>
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
