linux-kernel - Re: [PATCH net-next v2 09/21] motorcomm:yt6801: Implement some hw

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: <11e26658-670f-49fa-8001-0654670b541e@motor-comm.com>
Date: Mon, 25 Nov 2024 17:49:19 +0800
From: Frank Sae <Frank.Sae@...or-comm.com>
To: Andrew Lunn <andrew@...n.ch>
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 09/21] motorcomm:yt6801: Implement some hw_ops
 function

Hi Andrew,

On 2024/11/23 09:03, Andrew Lunn wrote:
> It took a lot of effort to find your MDIO code. And MDIO bus driver
> makes a good patch on its own.
> 

Sorry about that.
There is too many codes in yt6801_hw.c file. If I put the MDIO bus driver
in one patch, it's would be very difficult to limit to 15 patches. 

>> +static int mdio_loop_wait(struct fxgmac_pdata *pdata, u32 max_cnt)
>> +{
>> +	u32 val, i;
>> +
>> +	for (i = 0; i < max_cnt; i++) {
>> +		val = rd32_mac(pdata, MAC_MDIO_ADDRESS);
>> +		if ((val & MAC_MDIO_ADDR_BUSY) == 0)
>> +			break;
>> +
>> +		fsleep(10);
>> +	}
>> +
>> +	if (i >= max_cnt) {
>> +		WARN_ON(1);
>> +		yt_err(pdata, "%s timeout. used cnt:%d, reg_val=%x.\n",
>> +		       __func__, i + 1, val);
>> +
>> +		return -ETIMEDOUT;
>> +	}
> 
> Please replace this using one of the helpers in
> include/linux/iopoll.h.
> 
>> +#define PHY_WR_CONFIG(reg_offset)		(0x8000205 + ((reg_offset) * 0x10000))
>> +static int fxgmac_phy_write_reg(struct fxgmac_pdata *pdata, u32 reg_id, u32 data)
>> +{
>> +	int ret;
>> +
>> +	wr32_mac(pdata, data, MAC_MDIO_DATA);
>> +	wr32_mac(pdata, PHY_WR_CONFIG(reg_id), MAC_MDIO_ADDRESS);
>> +	ret = mdio_loop_wait(pdata, PHY_MDIO_MAX_TRY);
>> +	if (ret < 0)
>> +		return ret;
>> +
>> +	yt_dbg(pdata, "%s, id:%x %s, ctrl:0x%08x, data:0x%08x\n", __func__,
>> +	       reg_id, (ret == 0) ? "ok" : "err", PHY_WR_CONFIG(reg_id), data);
>> +
>> +	return ret;
>> +}
>> +
>> +#define PHY_RD_CONFIG(reg_offset)		(0x800020d + ((reg_offset) * 0x10000))
>> +static int fxgmac_phy_read_reg(struct fxgmac_pdata *pdata, u32 reg_id)
>> +{
>> +	u32 val;
>> +	int ret;
>> +
>> +	wr32_mac(pdata, PHY_RD_CONFIG(reg_id), MAC_MDIO_ADDRESS);
>> +	ret =  mdio_loop_wait(pdata, PHY_MDIO_MAX_TRY);
>> +	if (ret < 0)
>> +		return ret;
>> +
>> +	val = rd32_mac(pdata, MAC_MDIO_DATA);  /* Read data */
>> +	yt_dbg(pdata, "%s, id:%x ok, ctrl:0x%08x, val:0x%08x.\n", __func__,
>> +	       reg_id, PHY_RD_CONFIG(reg_id), val);
>> +
>> +	return val;
>> +}
> 
> And where is the rest of the MDIO bus driver?

There is no separate reset of MDIO bus driver.

> 
>> +static int fxgmac_config_flow_control(struct fxgmac_pdata *pdata)
>> +{
>> +	u32 val = 0;
>> +	int ret;
>> +
>> +	fxgmac_config_tx_flow_control(pdata);
>> +	fxgmac_config_rx_flow_control(pdata);
>> +
>> +	/* Set auto negotiation advertisement pause ability */
>> +	if (pdata->tx_pause || pdata->rx_pause)
>> +		val |= ADVERTISE_PAUSE_CAP | ADVERTISE_PAUSE_ASYM;
>> +
>> +	ret = phy_modify(pdata->phydev, MII_ADVERTISE,
>> +			 ADVERTISE_PAUSE_CAP | ADVERTISE_PAUSE_ASYM, val);
>> +	if (ret < 0)
>> +		return ret;
>> +
>> +	return phy_modify(pdata->phydev, MII_BMCR, BMCR_RESET, BMCR_RESET);
>> +}
> 
> 
> Yet more code messing with the PHY. This all needs to go.
> 
>> +static int fxgmac_phy_clear_interrupt(struct fxgmac_pdata *pdata)
>> +{
>> +	u32 stats_pre, stats;
>> +
>> +	if (mutex_trylock(&pdata->phydev->mdio.bus->mdio_lock) == 0) {
>> +		yt_dbg(pdata, "lock not ready!\n");
>> +		return 0;
>> +	}
>> +
>> +	stats_pre = fxgmac_phy_read_reg(pdata, PHY_INT_STATUS);
>> +	if (stats_pre < 0)
>> +		goto unlock;
>> +
>> +	stats = fxgmac_phy_read_reg(pdata, PHY_INT_STATUS);
>> +	if (stats < 0)
>> +		goto unlock;
>> +
>> +	phy_unlock_mdio_bus(pdata->phydev);
>> +
>> +#define LINK_DOWN	0x800
>> +#define LINK_UP		0x400
>> +#define LINK_CHANGE	(LINK_DOWN | LINK_UP)
>> +	if ((stats_pre & LINK_CHANGE) != (stats & LINK_CHANGE)) {
>> +		yt_dbg(pdata, "phy link change\n");
>> +		return 1;
>> +	}
>> +
>> +	return 0;
>> +unlock:
>> +	phy_unlock_mdio_bus(pdata->phydev);
>> +	yt_err(pdata, "fxgmac_phy_read_reg err!\n");
>> +	return  -ETIMEDOUT;
>> +}
> 
> You need to rework your PHY interrupt handling. The PHY driver is
> responsible for handing the interrupt registers in the PHY. Ideally
> you just want to export an interrupt to phylib, so it can do all the
> work.

I'm sorry. Could you please give me more information about export
 an interrupt to phylib?

> 
> 	Andrew