netdev - Re: [PATCH v3 2/5] net: rnpgbe: Add n500/n210 chip support

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] [thread-next>] [day] [month] [year] [list]

Message-ID: <69d797eb-4a17-4d54-a7c0-8409fa8bc066@linux.dev>
Date: Tue, 12 Aug 2025 16:49:33 +0100
From: Vadim Fedorenko <vadim.fedorenko@...ux.dev>
To: Dong Yibo <dong100@...se.com>, andrew+netdev@...n.ch,
 davem@...emloft.net, edumazet@...gle.com, kuba@...nel.org,
 pabeni@...hat.com, horms@...nel.org, corbet@....net, gur.stavi@...wei.com,
 maddy@...ux.ibm.com, mpe@...erman.id.au, danishanwar@...com, lee@...ger.us,
 gongfan1@...wei.com, lorenzo@...nel.org, geert+renesas@...der.be,
 Parthiban.Veerasooran@...rochip.com, lukas.bulwahn@...hat.com,
 alexanderduyck@...com, richardcochran@...il.com
Cc: netdev@...r.kernel.org, linux-doc@...r.kernel.org,
 linux-kernel@...r.kernel.org
Subject: Re: [PATCH v3 2/5] net: rnpgbe: Add n500/n210 chip support


> +struct mucse_dma_info {
> +	void __iomem *dma_base_addr;
> +	void __iomem *dma_ring_addr;
> +	void *back;

it might be better to keep the type of back pointer and give it
a bit more meaningful name ...

> +	u32 dma_version;
> +};
> +
> +struct mucse_eth_info {
> +	void __iomem *eth_base_addr;
> +	void *back;

.. here ...

> +};
> +
> +struct mucse_mac_info {
> +	void __iomem *mac_addr;
> +	void *back;

and here...

> +};
> +
> +struct mucse_mbx_info {
> +	/* fw <--> pf mbx */
> +	u32 fw_pf_shm_base;
> +	u32 pf2fw_mbox_ctrl;
> +	u32 pf2fw_mbox_mask;
> +	u32 fw_pf_mbox_mask;
> +	u32 fw2pf_mbox_vec;
> +};
> +
> +struct mucse_hw {
> +	void *back;

you can also use container_of() as all these structures are embedded and
simple pointer math can give you proper result.

> +	void __iomem *hw_addr;
> +	void __iomem *ring_msix_base;
> +	struct pci_dev *pdev;
> +	enum rnpgbe_hw_type hw_type;
> +	struct mucse_dma_info dma;
> +	struct mucse_eth_info eth;
> +	struct mucse_mac_info mac;
> +	struct mucse_mbx_info mbx;
> +	u32 driver_version;
> +	u16 usecstocount;
> +};
> +
>   struct mucse {
>   	struct net_device *netdev;
>   	struct pci_dev *pdev;
> +	struct mucse_hw hw;
>   	u16 bd_number;
>   };
>   

[...]

> +/**
> + * rnpgbe_add_adapter - Add netdev for this pci_dev
> + * @pdev: PCI device information structure
> + * @info: chip info structure
> + *
> + * rnpgbe_add_adapter initializes a netdev for this pci_dev
> + * structure. Initializes Bar map, private structure, and a
> + * hardware reset occur.
> + *
> + * @return: 0 on success, negative on failure
> + **/
> +static int rnpgbe_add_adapter(struct pci_dev *pdev,
> +			      const struct rnpgbe_info *info)
> +{
> +	struct net_device *netdev;
> +	void __iomem *hw_addr;
> +	static int bd_number;

it's not clear from the patchset why do you need this static variable...

> +	struct mucse *mucse;
> +	struct mucse_hw *hw;
> +	u32 dma_version = 0;
> +	u32 queues;
> +	int err;
> +
> +	queues = info->total_queue_pair_cnts;
> +	netdev = alloc_etherdev_mq(sizeof(struct mucse), queues);
> +	if (!netdev)
> +		return -ENOMEM;
> +
> +	SET_NETDEV_DEV(netdev, &pdev->dev);
> +	mucse = netdev_priv(netdev);
> +	mucse->netdev = netdev;
> +	mucse->pdev = pdev;
> +	mucse->bd_number = bd_number++;

... but this code is racy by design

> +	pci_set_drvdata(pdev, mucse);
> +
> +	hw = &mucse->hw;
> +	hw->back = mucse;
> +	hw->hw_type = info->hw_type;
> +	hw->pdev = pdev;
> +