Re: [PATCH net-next v09 1/1] hinic3: module initialization and tx/rx logic

[Jakub Kicinski <kuba@kernel.org>]

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