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: <708dd5d9-5e06-4446-9b1d-f6f9561eb9c3@motor-comm.com>
Date: Tue, 26 Nov 2024 11:04:40 +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,
  Thanks for your suggestions.
  I will follow your advice, thanks again!


On 2024/11/25 22:39, Andrew Lunn wrote:
> 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