Re: [PATCH v6 1/5] net: wan: Add support for QMC HDLC

[Herve Codina <herve.codina@bootlin.com>]

Hi Ratheesh

On Wed, 6 Mar 2024 16:26:51 +0530
Ratheesh Kannoth <rkannoth@marvell.com> wrote:

...

> > +static void qmc_hcld_recv_complete(void *context, size_t length, unsigned int flags)
> > +{
> > +	struct qmc_hdlc_desc *desc = context;
> > +	struct net_device *netdev = desc->netdev;
> > +	struct qmc_hdlc *qmc_hdlc = netdev_to_qmc_hdlc(netdev);  
> Reverse xmas tree

The reverse xmas tree order cannot be used here.
qmc_hdlc depends on netdev, netdev depends on desc.

...
> > +static void qmc_hdlc_xmit_complete(void *context)
> > +{
> > +	struct qmc_hdlc_desc *desc = context;
> > +	struct net_device *netdev = desc->netdev;
> > +	struct qmc_hdlc *qmc_hdlc = netdev_to_qmc_hdlc(netdev);
> > +	struct sk_buff *skb;  
> Reverse xmas tree

Ditto

> > +
> > +	scoped_guard(spinlock_irqsave, &qmc_hdlc->tx_lock) {
> > +		dma_unmap_single(qmc_hdlc->dev, desc->dma_addr, desc->dma_size, DMA_TO_DEVICE);
> > +		skb = desc->skb;
> > +		desc->skb = NULL; /* Release the descriptor */
> > +		if (netif_queue_stopped(netdev))
> > +			netif_wake_queue(netdev);
> > +	}
> > +
> > +	netdev->stats.tx_packets++;
> > +	netdev->stats.tx_bytes += skb->len;
> > +
> > +	dev_consume_skb_any(skb);
> > +}
> > +
...
> > +
> > +static netdev_tx_t qmc_hdlc_xmit(struct sk_buff *skb, struct net_device *netdev)
> > +{
> > +	struct qmc_hdlc *qmc_hdlc = netdev_to_qmc_hdlc(netdev);
> > +	struct qmc_hdlc_desc *desc;
> > +	int err;
> > +
> > +	guard(spinlock_irqsave)(&qmc_hdlc->tx_lock);
> > +
> > +	desc = &qmc_hdlc->tx_descs[qmc_hdlc->tx_out];
> > +	if (WARN_ONCE(desc->skb, "No tx descriptors available\n")) {
> > +		/* Should never happen.
> > +		 * Previous xmit should have already stopped the queue.
> > +		 */
> > +		netif_stop_queue(netdev);
> > +		return NETDEV_TX_BUSY;
> > +	}
> > +
> > +	desc->netdev = netdev;
> > +	desc->dma_size = skb->len;
> > +	desc->skb = skb;
> > +	err = qmc_hdlc_xmit_queue(qmc_hdlc, desc);
> > +	if (err) {
> > +		desc->skb = NULL; /* Release the descriptor */
> > +		if (err == -EBUSY) {
> > +			netif_stop_queue(netdev);
> > +			return NETDEV_TX_BUSY;
> > +		}
> > +		dev_kfree_skb(skb);
> > +		netdev->stats.tx_dropped++;
> > +		return NETDEV_TX_OK;
> > +	}
> > +
> > +	qmc_hdlc->tx_out = (qmc_hdlc->tx_out + 1) % ARRAY_SIZE(qmc_hdlc->tx_descs);
> > +
> > +	if (qmc_hdlc->tx_descs[qmc_hdlc->tx_out].skb)  
> wont it race if tx completion and this function context run in different cpu ?

We are protected by the qmc_hdlc->tx_lock spinlock.

guard() call in this function and scoped_guard() call in qmc_hdlc_xmit_complete().

What is the race you thought of ?

> 
> > +		netif_stop_queue(netdev);
> > +
> > +	return NETDEV_TX_OK;
> > +}
> > +
...
> > +	/* Queue as many recv descriptors as possible */
> > +	for (i = 0; i < ARRAY_SIZE(qmc_hdlc->rx_descs); i++) {
> > +		desc = &qmc_hdlc->rx_descs[i];
> > +
> > +		desc->netdev = netdev;
> > +		ret = qmc_hdlc_recv_queue(qmc_hdlc, desc, chan_param.hdlc.max_rx_buf_size);
> > +		if (ret == -EBUSY && i != 0)
> > +			break; /* We use all the QMC chan capability */
> > +		if (ret)
> > +			goto free_desc;
> > +	}
> > +
> > +	ret = qmc_chan_start(qmc_hdlc->qmc_chan, QMC_CHAN_ALL);
> > +	if (ret) {
> > +		dev_err(qmc_hdlc->dev, "qmc chan start failed (%d)\n", ret);
> > +		goto free_desc;
> > +	}
> > +
> > +	netif_start_queue(netdev);
> > +
> > +	return 0;
> > +
> > +free_desc:
> > +	qmc_chan_reset(qmc_hdlc->qmc_chan, QMC_CHAN_ALL);
> > +	while (i--) {  
> Double free ? i'th descriptor skb is freed in qmc_hdlc_recv_queue() function's error handler itself.
> Should we be predecrement of i ?

Suppose a failure on i = 5. The item 5 is already cleaned (done by 
qmc_hdlc_recv_queue() itself).
The 'while (i--)' set i to 4 and operations are performed with i = 4, 3, 2, 1 and 0.

Where is the double free ?
Do I miss something ?

> 
> > +		desc = &qmc_hdlc->rx_descs[i];
> > +		dma_unmap_single(qmc_hdlc->dev, desc->dma_addr, desc->dma_size,
> > +				 DMA_FROM_DEVICE);
> > +		kfree_skb(desc->skb);
> > +		desc->skb = NULL;
> > +	}
> > +hdlc_close:
> > +	hdlc_close(netdev);
> > +	return ret;
> > +}
> > +
> > +

Best regards,
Hervé