On Fri, Apr 8, 2022 at 1:37 AM Ricardo Martinez
<ricardo.martinez@xxxxxxxxxxxxxxx> 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@xxxxxxxxxxxx>
> Co-developed-by: Chandrashekar Devegowda <chandrashekar.devegowda@xxxxxxxxx>
> Signed-off-by: Chandrashekar Devegowda <chandrashekar.devegowda@xxxxxxxxx>
> Co-developed-by: Ricardo Martinez <ricardo.martinez@xxxxxxxxxxxxxxx>
> Signed-off-by: Ricardo Martinez <ricardo.martinez@xxxxxxxxxxxxxxx>
>
> From a WWAN framework perspective:
> Reviewed-by: Loic Poulain <loic.poulain@xxxxxxxxxx>
[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
