netdev - Re: [PATCH net-next v11 5/5] net: rnpgbe: Add register

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: <6C0625E60A7E1E95+20250910062236.GA1832954@nic-Precision-5820-Tower>
Date: Wed, 10 Sep 2025 14:22:36 +0800
From: Yibo Dong <dong100@...se.com>
To: Vadim Fedorenko <vadim.fedorenko@...ux.dev>
Cc: 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, kees@...nel.org,
	gustavoars@...nel.org, rdunlap@...radead.org,
	netdev@...r.kernel.org, linux-doc@...r.kernel.org,
	linux-kernel@...r.kernel.org, linux-hardening@...r.kernel.org
Subject: Re: [PATCH net-next v11 5/5] net: rnpgbe: Add register_netdev

On Tue, Sep 09, 2025 at 03:37:19PM +0100, Vadim Fedorenko wrote:
> On 09/09/2025 13:09, Dong Yibo wrote:
> > Complete the network device (netdev) registration flow for Mucse Gbe
> > Ethernet chips, including:
> > 1. Hardware state initialization:
> >     - Send powerup notification to firmware (via echo_fw_status)
> >     - Sync with firmware
> >     - Reset hardware
> > 2. MAC address handling:
> >     - Retrieve permanent MAC from firmware (via mucse_mbx_get_macaddr)
> >     - Fallback to random valid MAC (eth_random_addr) if not valid mac
> >       from Fw
> > 
> > Signed-off-by: Dong Yibo <dong100@...se.com>
> 
> [...]
> 
> > +struct mucse_hw;
> 
> why do you need this forward declaration ...> +
> > +struct mucse_hw_operations {
> > +	int (*reset_hw)(struct mucse_hw *hw);
> > +	int (*get_perm_mac)(struct mucse_hw *hw);
> > +	int (*mbx_send_notify)(struct mucse_hw *hw, bool enable, int mode);
> > +};
> > +
> > +enum {
> > +	mucse_fw_powerup,
> > +};
> > +
> >   struct mucse_hw {
> >   	void __iomem *hw_addr;
> > +	struct pci_dev *pdev;
> > +	const struct mucse_hw_operations *ops;
> > +	struct mucse_dma_info dma;
> >   	struct mucse_mbx_info mbx;
> > +	int port;
> > +	u8 perm_addr[ETH_ALEN];
> >   	u8 pfvfnum;
> >   };
> 
> ... if you can simply move mucse_hw_operations down here?
> 

Got it, I will update it. At the beginning I defined 
'struct mucse_hw_operations ops' in 'struct mucse_hw'. I missed to
consider this when it is changed to 'struct mucse_hw_operations *ops'.

> > @@ -54,4 +76,7 @@ int rnpgbe_init_hw(struct mucse_hw *hw, int board_type);
> >   #define PCI_DEVICE_ID_N500_DUAL_PORT 0x8318
> >   #define PCI_DEVICE_ID_N210 0x8208
> >   #define PCI_DEVICE_ID_N210L 0x820a
> > +
> > +#define rnpgbe_dma_wr32(dma, reg, val) \
> > +	writel((val), (dma)->dma_base_addr + (reg))
> 
> [...]
> 
> > @@ -48,8 +127,14 @@ static void rnpgbe_init_n210(struct mucse_hw *hw)
> >    **/
> >   int rnpgbe_init_hw(struct mucse_hw *hw, int board_type)
> >   {
> > +	struct mucse_dma_info *dma = &hw->dma;
> >   	struct mucse_mbx_info *mbx = &hw->mbx;
> > +	hw->ops = &rnpgbe_hw_ops;
> > +	hw->port = 0;
> > +
> > +	dma->dma_base_addr = hw->hw_addr;
> 
> not quite sure why do you need additional structure just to store the
> value that already exists in mucse_hw?
> 

The original meaning was that all submodules have their own base, such
as dma, mbx... Just that 'dma_base_addr' is 'offset 0 from hw_addr'.
And maybe it is not good to do it like this?
...
#define MUCSE_GBE_DMA_BASE 0
dma->dma_base_addr = hw->hw_addr + MUCSE_GBE_DMA_BASE;
...
If so, I will remove dma_base_addr, and use hw_addr instead.

> > +
> >   	mbx->pf2fw_mbx_ctrl = MUCSE_GBE_PFFW_MBX_CTRL_OFFSET;
> >   	mbx->fwpf_mbx_mask = MUCSE_GBE_FWPF_MBX_MASK_OFFSET;
> 

Thanks for your feedback.