netdev - 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: <cd57707a-2d7e-4549-aab1-d0bd6c24ad35@lunn.ch>
Date: Mon, 25 Nov 2024 15:39:57 +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

On Mon, Nov 25, 2024 at 05:49:19PM +0800, Frank Sae wrote:
> 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. 

You can easily limit this to 15 patches. Throw away 75% of the driver
for the moment. Do enough to get link and then send/receive
frames. Forget the rest. You don't need statistics, ethtool, WoL, and
everything else in the first submission. They can all be added later.

You need reviewers in order to get your driver merged. If you give
them a huge driver which is hard to find what they are interested in,
they won't review it, and it will not get merged. So break it up into
a number of patchsets. A minimum driver to just send/receive should be
nice and small, and can be split into 15 patches making it nice and
easy to find bits reviewers are interested in. That should get
reviewed and merged. Then add more and more features in nice small
chunks which are easy to review.

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

rest, not reset.

An MDIO driver is generally two to five functions to do bus
transactions, and then one or two functions to allocate the bus
structure, fill in the members and register the bus, and maybe a
function to undo that. I would expect these all to be in one
patch. They are not.

At some point, you need to justify your hw_ops structure. Why do you
have this? At the moment it just obfuscate the code. Maybe there is a
good reason for it, but given the size of the driver i've not been
able to find it.

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

I would actually suggest you first just let phylib poll the PHY. That
gets you something working. You can add interrupt support in a later
patchset. For ideas, look at ksz_common.c:

ds->user_mii_bus->irq[phy] = irq;

You have a whole tree of source code you can look at, there are other
examples of MAC drivers exporting an interrupt controller. And you
might find other ways to do this, look at other MAC drivers. There is
nothing special here, so don't invent something new, copy what other
MAC drivers do.


	Andrew