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: <8E12C5B26514F60A+20250722062120.GB99399@nic-Precision-5820-Tower>
Date: Tue, 22 Jul 2025 14:21:20 +0800
From: Yibo Dong <dong100@...se.com>
To: Andrew Lunn <andrew@...n.ch>
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,
	netdev@...r.kernel.org, linux-doc@...r.kernel.org,
	linux-kernel@...r.kernel.org
Subject: Re: [PATCH v2 02/15] net: rnpgbe: Add n500/n210 chip support

On Mon, Jul 21, 2025 at 05:25:12PM +0200, Andrew Lunn wrote:
> > +struct mii_regs {
> > +	unsigned int addr; /* MII Address */
> > +	unsigned int data; /* MII Data */
> > +	unsigned int addr_shift; /* MII address shift */
> > +	unsigned int reg_shift; /* MII reg shift */
> > +	unsigned int addr_mask; /* MII address mask */
> > +	unsigned int reg_mask; /* MII reg mask */
> > +	unsigned int clk_csr_shift;
> > +	unsigned int clk_csr_mask;
> > +};
> 
> So MII interests me, being the MDIO/PHY maintainer....
> 
> You have introduced this without any user, which is not good, so i
> cannot see how it is actually used. It is better to introduce
> structures in the patch which makes use of them.
> 
> Please add this only when you add the mdiobus driver, so i can see how
> it is used. Please look at the other structures you have here. Please
> add them as they are actually used.
> 

Yes, you are right, I should add the structures when they are actually
used. I will improve it.

> > +struct mucse_hw {
> > +	void *back;
> > +	u8 pfvfnum;
> > +	u8 pfvfnum_system;
> > +	u8 __iomem *hw_addr;
> > +	u8 __iomem *ring_msix_base;
> 
> I spotted this somewhere else. A u8 __iomem * is odd. Why is this not
> a void *? ioremap() returns a void __iomem *, and all the readb(),
> readw(), readX() functions expect a void * __iomem. So this looks odd.
> 

Got it, I will change it. I just consider the wrong cast before. Sorry
for not check this define error.

> > +#define m_rd_reg(reg) readl(reg)
> > +#define m_wr_reg(reg, val) writel((val), reg)
> 
> Please don't wrap standard functions like this. Everybody knows what
> readl() does. Nobody has any idea what m_rd_reg() does! You are just
> making your driver harder to understand and maintain.
> 

Got it.

> > +	mac->mii.addr = RNPGBE_MII_ADDR;
> > +	mac->mii.data = RNPGBE_MII_DATA;
> > +	mac->mii.addr_shift = 11;
> > +	mac->mii.addr_mask = 0x0000F800;
> 
> GENMASK()? If you are using these helpers correctly, you probably
> don't need the _shift members.
> 
> > +	mac->mii.reg_shift = 6;
> > +	mac->mii.reg_mask = 0x000007C0;
> > +	mac->mii.clk_csr_shift = 2;
> > +	mac->mii.clk_csr_mask = GENMASK(5, 2);
> > +	mac->clk_csr = 0x02; /* csr 25M */
> > +	/* hw fixed phy_addr */
> > +	mac->phy_addr = 0x11;
> 
> That is suspicious. But until i see the PHY handling code, it is hard
> to say.
> 

Those code should move to the patch which really use it.

> > +static void rnpgbe_get_invariants_n210(struct mucse_hw *hw)
> > +{
> > +	struct mucse_mbx_info *mbx = &hw->mbx;
> > +	/* get invariants based from n500 */
> > +	rnpgbe_get_invariants_n500(hw);
> > +
> > +	/* update msix base */
> > +	hw->ring_msix_base = hw->hw_addr + 0x29000;
> > +	/* update mbx offset */
> > +	mbx->vf2pf_mbox_vec_base = 0x29200;
> > +	mbx->fw2pf_mbox_vec = 0x29400;
> > +	mbx->pf_vf_shm_base = 0x29900;
> > +	mbx->mbx_mem_size = 64;
> > +	mbx->pf2vf_mbox_ctrl_base = 0x2aa00;
> > +	mbx->pf_vf_mbox_mask_lo = 0x2ab00;
> > +	mbx->pf_vf_mbox_mask_hi = 0;
> > +	mbx->fw_pf_shm_base = 0x2d900;
> > +	mbx->pf2fw_mbox_ctrl = 0x2e900;
> > +	mbx->fw_pf_mbox_mask = 0x2eb00;
> > +	mbx->fw_vf_share_ram = 0x2b900;
> > +	mbx->share_size = 512;
> > +	/* update hw feature */
> > +	hw->feature_flags |= M_HW_FEATURE_EEE;
> > +	hw->usecstocount = 62;
> 
> This variant does not have an MDIO bus?
> 

Some hw capabilies, such as queue numbers and hardware module 
reg-offset(dma_base_addr, eth_base_addr ..) in this function. Don't
have an MDIO bus now.

> > +#define RNPGBE_RING_BASE (0x1000)
> > +#define RNPGBE_MAC_BASE (0x20000)
> > +#define RNPGBE_ETH_BASE (0x10000)
> 
> Please drop all the () on plain constants. You only need () when it is
> an expression.
> 

Got it, I will fix this.

> > +			      const struct rnpgbe_info *ii)
> 
> I don't really see how the variable name ii has anything to do with
> rnpgbe_info. I know naming is hard, but why not call it info?
> 
> 

Got it, ii is unclear, I will use info instead.

> >  {
> >  	struct mucse *mucse = NULL;
> > +	struct mucse_hw *hw = NULL;
> > +	u8 __iomem *hw_addr = NULL;
> >  	struct net_device *netdev;
> >  	static int bd_number;
> > +	u32 dma_version = 0;
> > +	int err = 0;
> > +	u32 queues;
> >  
> > -	netdev = alloc_etherdev_mq(sizeof(struct mucse), 1);
> > +	queues = ii->total_queue_pair_cnts;
> > +	netdev = alloc_etherdev_mq(sizeof(struct mucse), queues);
> 
> I pointed out this before. Try to avoid changing code added in
> previous patches. I just wasted time looking up what the function is
> called which allocates a single queue, and writing a review comment.
> 
> Waiting reviewers time is a good way to get less/slower reviews.
> 
> 	Andrew
> 

Yes, I got it before, and I really tried to improve my code.
But this is really hard to avoid here. 'queues' is from ii->total_queue_pair_cnts
which is added in patch2. Maybe I should move the alloc_etherdev_mq to
patch2, never use it in patch1? And this conditon can improve.

thanks for your feedback.


Powered by blists - more mailing lists

Powered by Openwall GNU/*/Linux Powered by OpenVZ