Received-SPF: pass (google.com: domain of linux-kernel+bounces-93985-linux.lists.archive=gmail.com@vger.kernel.org designates 2604:1380:45e3:2400::1 as permitted sender) client-ip=2604:1380:45e3:2400::1;
Date: Wed, 6 Mar 2024 14:37:43 +0100
From: Herve Codina <herve.codina@bootlin.com>
To: Ratheesh Kannoth <rkannoth@marvell.com>
Cc: <netdev@vger.kernel.org>, <linux-kernel@vger.kernel.org>
Subject: Re: [PATCH v6 1/5] net: wan: Add support for QMC HDLC
Message-ID: <20240306143743.5732b298@bootlin.com>
In-Reply-To: <20240306105651.1210286-1-rkannoth@marvell.com>
References: <20240306080726.167338-2-herve.codina@bootlin.com>
	<20240306105651.1210286-1-rkannoth@marvell.com>
Organization: Bootlin
Precedence: bulk
MIME-Version: 1.0
Content-Type: text/plain; charset=UTF-8
Content-Transfer-Encoding: 8bit

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é