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

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [day] [month] [year] [list]

Message-ID: <20250325034641.65536e13@kernel.org>
Date: Tue, 25 Mar 2025 03:46:41 -0700
From: Jakub Kicinski <kuba@...nel.org>
To: Gur Stavi <gur.stavi@...wei.com>
Cc: Fan Gong <gongfan1@...wei.com>, <netdev@...r.kernel.org>,
 <linux-kernel@...r.kernel.org>, "David S. Miller" <davem@...emloft.net>,
 Eric Dumazet <edumazet@...gle.com>, Paolo Abeni <pabeni@...hat.com>, Simon
 Horman <horms@...nel.org>, Andrew Lunn <andrew+netdev@...n.ch>, Lee Trager
 <lee@...ger.us>, <linux-doc@...r.kernel.org>, Jonathan Corbet
 <corbet@....net>, Bjorn Helgaas <helgaas@...nel.org>, Cai Huoqing
 <cai.huoqing@...ux.dev>, luosifu <luosifu@...wei.com>, Xin Guo
 <guoxin09@...wei.com>, Shen Chenyang <shenchenyang1@...ilicon.com>, Zhou
 Shuai <zhoushuai28@...wei.com>, Wu Like <wulike1@...wei.com>, Shi Jing
 <shijing34@...wei.com>, Meny Yossefi <meny.yossefi@...wei.com>, Suman Ghosh
 <sumang@...vell.com>, Przemek Kitszel <przemyslaw.kitszel@...el.com>, Joe
 Damato <jdamato@...tly.com>
Subject: Re: [PATCH net-next v09 1/1] hinic3: module initialization and
 tx/rx logic

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