Dear Jakub,
Thanks for your comments~
On Thu, 2022-06-23 at 21:34 -0700, Jakub Kicinski wrote:
> On Wed, 22 Jun 2022 17:05:44 +0800 Biao Huang wrote:
> > + if (rx || tx) {
> > + spin_lock_irqsave(&priv->lock, flags);
> > + /* mask Rx and TX Complete interrupt */
> > + mtk_star_disable_dma_irq(priv, rx, tx);
> > + spin_unlock_irqrestore(&priv->lock, flags);
>
> You do _irqsave / _irqrestore here
We should invoke spin_lock, no need save/store irq here.
>
> > + if (rx)
> > + __napi_schedule_irqoff(&priv->rx_napi);
> > + if (tx)
> > + __napi_schedule_irqoff(&priv->tx_napi);
>
> Yet assume _irqoff here.
>
> So can this be run from non-IRQ context or not?
seems __napi_schedule is more proper for our case, we'll modify it in
next send.
>
> > - if (mtk_star_ring_full(ring))
> > + if (unlikely(mtk_star_tx_ring_avail(ring) < MAX_SKB_FRAGS + 1))
> > netif_stop_queue(ndev);
>
> Please look around other drivers (like ixgbe) and copy the way they
> handle safe stopping of the queues. You need to add some barriers and
> re-check after disabling.
Yes, we look drivers from other vendors, and will do similar thing in
next send.
>
> > - spin_unlock_bh(&priv->lock);
> > -
> > mtk_star_dma_resume_tx(priv);
> >
> > return NETDEV_TX_OK;
>
>
> > + while ((entry != head) && (count < MTK_STAR_RING_NUM_DESCS -
> > 1)) {
> >
>
> Parenthesis unnecessary, so is the empty line after the while ().
Yes, the empty line will be removed in next send.
>
> > ret = mtk_star_tx_complete_one(priv);
> > if (ret < 0)
> > break;
> > +
> > + count++;
> > + pkts_compl++;
> > + bytes_compl += ret;
> > + entry = ring->tail;
> > }
> >
> > + __netif_tx_lock_bh(netdev_get_tx_queue(priv->ndev, 0));
> > netdev_completed_queue(ndev, pkts_compl, bytes_compl);
> > + __netif_tx_unlock_bh(netdev_get_tx_queue(priv->ndev, 0));
>
> what are you taking this lock for?
In this version, we encounter some issue related to
__QUEUE_STATE_STACK_OFF,
and if we add __netif_tx_lock_bh here, it disappears.
When recieve your comments, we survey netdev_completed_queue handles in
drivers from other vendors, we beleive the __QUEUE_STATE_STACK_OFF
issue may caused by unproper usage of __napi_schedule_irqoff in
previous lines, and we'll remove __netif_tx_lock_bh, and have another
try.
If our local stress test pass, corresponding modification will be added
in next send.
>
> > - if (wake && netif_queue_stopped(ndev))
> > + if (unlikely(netif_queue_stopped(ndev)) &&
> > + (mtk_star_tx_ring_avail(ring) > MTK_STAR_TX_THRESH))
> > netif_wake_queue(ndev);
> >
> > - spin_unlock(&priv->lock);
> > + if (napi_complete(napi)) {
> > + spin_lock_irqsave(&priv->lock, flags);
> > + mtk_star_enable_dma_irq(priv, false, true);
> > + spin_unlock_irqrestore(&priv->lock, flags);
> > + }
> > +
> > + return 0;
> > }
> > @@ -1475,6 +1514,7 @@ static int mtk_star_set_timing(struct
> > mtk_star_priv *priv)
> >
> > return regmap_write(priv->regs, MTK_STAR_REG_TEST0, delay_val);
> > }
> > +
> > static int mtk_star_probe(struct platform_device *pdev)
> > {
> > struct device_node *of_node;
>
> spurious whitespace change
Yes, will fix it in next send.
Best Regards!
Biao
