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: <46206a81-e230-411c-8a78-d461d238b171@lunn.ch>
Date: Sat, 23 Nov 2024 02:03:08 +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 09/21] motorcomm:yt6801: Implement some
 hw_ops function

It took a lot of effort to find your MDIO code. And MDIO bus driver
makes a good patch on its own.

> +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?

> +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.

	Andrew