On Mon, 17 Mar 2025 11:40:39 +0200 Gur Stavi wrote:
> +static int hinic3_poll(struct napi_struct *napi, int budget)
> +{
> + struct hinic3_irq_cfg *irq_cfg =
> + container_of(napi, struct hinic3_irq_cfg, napi);
> + struct hinic3_nic_dev *nic_dev;
> + int tx_pkts, rx_pkts;
> +
> + nic_dev = netdev_priv(irq_cfg->netdev);
> + rx_pkts = hinic3_rx_poll(irq_cfg->rxq, budget);
> +
> + tx_pkts = hinic3_tx_poll(irq_cfg->txq, budget);
You should service Tx first, it frees skbs into a cache which Rx
can then use, while they are hopefully still cache-warm.
> + if (tx_pkts >= budget || rx_pkts >= budget)
> + return budget;
> +
> + napi_complete(napi);
Please use napi_complete_done().
> + hinic3_set_msix_state(nic_dev->hwdev, irq_cfg->msix_entry_idx,
> + HINIC3_MSIX_ENABLE);
> +
> + return max(tx_pkts, rx_pkts);
> +}
> +static int hinic3_nic_probe(struct auxiliary_device *adev,
> + const struct auxiliary_device_id *id)
> + err = register_netdev(netdev);
> + if (err)
> + goto err_register_netdev;
> +
> + netif_carrier_off(netdev);
You should carrier_off before you register
> + err = pci_enable_device(pdev);
> + if (err) {
> + dev_err(&pdev->dev, "Failed to enable PCI device\n");
> + goto err_pci_enable;
> + }
> +
> + err = pci_request_regions(pdev, HINIC3_NIC_DRV_NAME);
> + if (err) {
> + dev_err(&pdev->dev, "Failed to request regions\n");
> + goto err_pci_regions;
> + }
> +
> + pci_set_master(pdev);
> +
> + err = dma_set_mask_and_coherent(&pdev->dev, DMA_BIT_MASK(64));
> + if (err) {
> + dev_warn(&pdev->dev, "Couldn't set 64-bit DMA mask\n");
> + /* try 32 bit DMA mask if 64 bit fails */
> + err = dma_set_mask_and_coherent(&pdev->dev, DMA_BIT_MASK(32));
> + if (err) {
> + dev_err(&pdev->dev, "Failed to set DMA mask\n");
> + goto err_dma_mask;
> + }
> + }
> +
> + return 0;
> +
> +err_dma_mask:
> + pci_clear_master(pdev);
> + pci_release_regions(pdev);
> +
> +err_pci_regions:
> + pci_disable_device(pdev);
> +
> +err_pci_enable:
> + pci_set_drvdata(pdev, NULL);
> + mutex_destroy(&pci_adapter->pdev_mutex);
> + kfree(pci_adapter);
Please name the error labels after the target, not the source.
Quoting documentation:
Choose label names which say what the goto does or why the goto exists. An
example of a good name could be ``out_free_buffer:`` if the goto frees ``buffer``.
See: https://www.kernel.org/doc/html/next/process/coding-style.html#centralized-exiting-of-functions
--
pw-bot: cr
