linux-kernel - Re: [PATCH net-next v2 05/21] motorcomm:yt6801: Implement the fxgmac

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: <82e1860b-cbbf-4c82-9f1b-bf4a283e3585@lunn.ch>
Date: Mon, 25 Nov 2024 15:18:05 +0100
From: Andrew Lunn <andrew@...n.ch>
To: Frank Sae <Frank.Sae@...or-comm.com>
Cc: davem@...emloft.net, edumazet@...gle.com, kuba@...nel.org,
	pabeni@...hat.com, netdev@...r.kernel.org,
	linux-kernel@...r.kernel.org, xiaogang.fan@...or-comm.com,
	fei.zhang@...or-comm.com, hua.sun@...or-comm.com
Subject: Re: [PATCH net-next v2 05/21] motorcomm:yt6801: Implement the
 fxgmac_start function

> > RGMII is unusual, you normally want RGMII_ID. Where are the 2ns delays
> > added?
> > 
> 
> Yes, you are right. PHY_INTERFACE_MODE_RGMII should be PHY_INTERFACE_MODE_RGMII_ID.
> YT6801 NIC integrated with YT8531S phy, and the 2ns delays added in the phy driver.
> https://elixir.bootlin.com/linux/v6.12/source/drivers/net/phy/motorcomm.c#L895

But if you pass PHY_INTERFACE_MODE_RGMII to the PHY it is not adding
the 2ns delay. So how does this work now?

> >> +int fxgmac_start(struct fxgmac_pdata *pdata)
> >> +{
> >> +	struct fxgmac_hw_ops *hw_ops = &pdata->hw_ops;
> >> +	u32 val;
> >> +	int ret;
> >> +
> >> +	if (pdata->dev_state != FXGMAC_DEV_OPEN &&
> >> +	    pdata->dev_state != FXGMAC_DEV_STOP &&
> >> +	    pdata->dev_state != FXGMAC_DEV_RESUME) {
> >> +		yt_dbg(pdata, " dev_state err:%x\n", pdata->dev_state);
> >> +		return 0;
> >> +	}
> >> +
> >> +	if (pdata->dev_state != FXGMAC_DEV_STOP) {
> >> +		hw_ops->reset_phy(pdata);
> >> +		hw_ops->release_phy(pdata);
> >> +		yt_dbg(pdata, "reset phy.\n");
> >> +	}
> >> +
> >> +	if (pdata->dev_state == FXGMAC_DEV_OPEN) {
> >> +		ret = fxgmac_phy_connect(pdata);
> >> +		if (ret < 0)
> >> +			return ret;
> >> +
> >> +		yt_dbg(pdata, "fxgmac_phy_connect.\n");
> >> +	}
> >> +
> >> +	phy_init_hw(pdata->phydev);
> >> +	phy_resume(pdata->phydev);
> > 
> > The MAC should not be doing this.
> 
> Does this mean deleting 'phy_resume(pdata->phydev)'?

There are only a few phylib API calls you should be using

phy_connect() or one of its variants.
phy_start()
phy_stop()
phy_disconnect()

Those four are the core. Those should be all you need to minimum
support.

phy_support_asym_pause()
phy_support_eee()
phy_speed_up()
phy_speed_down()

and these are just nice to have to let phylib know about things the
MAC supports, so phylib can manage the PHY to make them available to
the MAC. This is the API between the MAC driver and phylib. phylib
will then manage the PHY. Any time you want to use a phy_* function,
look to see if other MAC drivers do. If they don't you should not
either.

> >> +	hw_ops->pcie_init(pdata);
> >> +	if (test_bit(FXGMAC_POWER_STATE_DOWN, &pdata->powerstate)) {
> >> +		yt_err(pdata,
> >> +		       "fxgmac powerstate is %lu when config power up.\n",
> >> +		       pdata->powerstate);
> >> +	}
> >> +
> >> +	hw_ops->config_power_up(pdata);
> >> +	hw_ops->dismiss_all_int(pdata);
> >> +	ret = hw_ops->init(pdata);
> >> +	if (ret < 0) {
> >> +		yt_err(pdata, "fxgmac hw init error.\n");
> >> +		return ret;
> >> +	}
> >> +
> >> +	fxgmac_napi_enable(pdata);
> >> +	ret = fxgmac_request_irqs(pdata);
> >> +	if (ret < 0)
> >> +		return ret;
> >> +
> >> +	/* Config interrupt to level signal */
> >> +	val = rd32_mac(pdata, DMA_MR);
> >> +	fxgmac_set_bits(&val, DMA_MR_INTM_POS, DMA_MR_INTM_LEN, 2);
> >> +	fxgmac_set_bits(&val, DMA_MR_QUREAD_POS, DMA_MR_QUREAD_LEN, 1);
> >> +	wr32_mac(pdata, val, DMA_MR);
> >> +
> >> +	hw_ops->enable_mgm_irq(pdata);
> >> +	hw_ops->set_interrupt_moderation(pdata);
> >> +
> >> +	if (pdata->per_channel_irq) {
> >> +		fxgmac_enable_msix_irqs(pdata);
> >> +		ret = fxgmac_phy_irq_enable(pdata, true);
> >> +		if (ret < 0)
> >> +			goto dis_napi;
> >> +	}
> >> +
> >> +	fxgmac_enable_rx_tx_ints(pdata);
> >> +	phy_speed_up(pdata->phydev);
> >> +	genphy_soft_reset(pdata->phydev);
> > 
> > More things the MAC driver should not be doing.
> 
> Does this mean deleting 'phy_speed_up(pdata->phydev);' and 'genphy_soft_reset(pdata->phydev);' ?

Two things here:

phy_speed_up()/phy_speed_down() is part of suspend/resume when using
WoL. This code has nothing to do with that. So why is it here?

There should not be any need to call genphy_soft_reset(). You should
figure out why you need it, because it could be a PHY driver bug, or a
MAC driver bug.

	Andrew