[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]
Message-ID: <20240306143743.5732b298@bootlin.com>
Date: Wed, 6 Mar 2024 14:37:43 +0100
From: Herve Codina <herve.codina@...tlin.com>
To: Ratheesh Kannoth <rkannoth@...vell.com>
Cc: <netdev@...r.kernel.org>, <linux-kernel@...r.kernel.org>
Subject: Re: [PATCH v6 1/5] net: wan: Add support for QMC HDLC
Hi Ratheesh
On Wed, 6 Mar 2024 16:26:51 +0530
Ratheesh Kannoth <rkannoth@...vell.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é
Powered by blists - more mailing lists