Hello Andrew,
On Fri, 22 Apr 2022 22:19:43 +0200
Andrew Lunn <andrew@xxxxxxx> wrote:
Thanks for the review :)
> > +static int ipqess_axi_probe(struct platform_device *pdev)
> > +{
> > + struct device_node *np = pdev->dev.of_node;
> > + struct net_device *netdev;
> > + phy_interface_t phy_mode;
> > + struct resource *res;
> > + struct ipqess *ess;
> > + int i, err = 0;
> > +
> > + netdev = devm_alloc_etherdev_mqs(&pdev->dev, sizeof(struct
> > ipqess),
> > + IPQESS_NETDEV_QUEUES,
> > + IPQESS_NETDEV_QUEUES);
> > + if (!netdev)
> > + return -ENOMEM;
> > +
> > + ess = netdev_priv(netdev);
> > + ess->netdev = netdev;
> > + ess->pdev = pdev;
> > + spin_lock_init(&ess->stats_lock);
> > + SET_NETDEV_DEV(netdev, &pdev->dev);
> > + platform_set_drvdata(pdev, netdev);
>
> ....
>
> > +
> > + ipqess_set_ethtool_ops(netdev);
> > +
> > + err = register_netdev(netdev);
> > + if (err)
> > + goto err_out;
>
> Before register_netdev() even returns, your devices can be in use, the
> open callback called and packets sent. This is particularly true for
> NFS root. Which means any setup done after this is probably wrong.
Nice catch, thank you !
> > +
> > + err = ipqess_hw_init(ess);
> > + if (err)
> > + goto err_out;
> > +
> > + for (i = 0; i < IPQESS_NETDEV_QUEUES; i++) {
> > + int qid;
> > +
> > + netif_tx_napi_add(netdev, &ess->tx_ring[i].napi_tx,
> > + ipqess_tx_napi, 64);
> > + netif_napi_add(netdev,
> > + &ess->rx_ring[i].napi_rx,
> > + ipqess_rx_napi, 64);
> > +
> > + qid = ess->tx_ring[i].idx;
> > + err = devm_request_irq(&ess->netdev->dev,
> > ess->tx_irq[qid],
> > + ipqess_interrupt_tx, 0,
> > + ess->tx_irq_names[qid],
> > + &ess->tx_ring[i]);
> > + if (err)
> > + goto err_out;
> > +
> > + qid = ess->rx_ring[i].idx;
> > + err = devm_request_irq(&ess->netdev->dev,
> > ess->rx_irq[qid],
> > + ipqess_interrupt_rx, 0,
> > + ess->rx_irq_names[qid],
> > + &ess->rx_ring[i]);
> > + if (err)
> > + goto err_out;
> > + }
>
> All this should probably go before netdev_register().
I'll fix this for V2.
> > +static int ipqess_get_strset_count(struct net_device *netdev, int
> > sset) +{
> > + switch (sset) {
> > + case ETH_SS_STATS:
> > + return ARRAY_SIZE(ipqess_stats);
> > + default:
> > + netdev_dbg(netdev, "%s: Invalid string set",
> > __func__);
>
> Unsupported would be better than invalid.
That's right, thanks
> > + return -EOPNOTSUPP;
> > + }
> > +}
>
> Andrew
Best Regards,
Maxime
