> From: Herve Codina <[email protected]>
> Sent: Wednesday, March 6, 2024 7:08 PM
> To: Ratheesh Kannoth <[email protected]>
> Cc: [email protected]; [email protected]
> Subject: [EXTERNAL] Re: [PATCH v6 1/5] net: wan: Add support for QMC HDLC
>
> > > +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.
>
ACK. Usually I get comments to split declaration and assignment for my patches in upstream.
> ...
> > > +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
Same as above.
>
> > > +
> > > + 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 ?
ACK. Thanks for clarification.
>
> >
> > > + 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 ?
My bad. ACK.
> >
> > > + 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é
On Wed, 6 Mar 2024 16:01:48 +0000 Ratheesh Kannoth wrote:
> > > > + 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.
> >
> ACK. Usually I get comments to split declaration and assignment for
> my patches in upstream.
Yup, that's our general preference, to split the init out of
the definition.
Hi Jakub,
On Wed, 6 Mar 2024 08:22:30 -0800
Jakub Kicinski <[email protected]> wrote:
> On Wed, 6 Mar 2024 16:01:48 +0000 Ratheesh Kannoth wrote:
> > > > > + 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.
> > >
> > ACK. Usually I get comments to split declaration and assignment for
> > my patches in upstream.
>
> Yup, that's our general preference, to split the init out of
> the definition.
Does it mean that I need to update in a next iteration ?
Best regards,
Hervé
On Wed, 6 Mar 2024 18:46:11 +0100 Herve Codina wrote:
> > > ACK. Usually I get comments to split declaration and assignment for
> > > my patches in upstream.
> >
> > Yup, that's our general preference, to split the init out of
> > the definition.
>
> Does it mean that I need to update in a next iteration ?
If you'd be so kind.