netdev - Re: [PATCH 4/5] net: phy: dp83tg720: Added OA script

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: <ZvD6_GJEsNq6yiB-@pengutronix.de>
Date: Mon, 23 Sep 2024 07:22:04 +0200
From: Oleksij Rempel <o.rempel@...gutronix.de>
To: Andrew Lunn <andrew@...n.ch>
Cc: "Alvaro (Al-vuh-roe) Reyes" <a-reyes1@...com>, netdev@...r.kernel.org,
	hkallweit1@...il.com, linux@...linux.org.uk,
	maxime.chevallier@...tlin.com, spatton@...com, r-kommineni@...com,
	e-mayhew@...com, praneeth@...com, p-varis@...com, d-qiu@...com,
	nm@...com
Subject: Re: [PATCH 4/5] net: phy: dp83tg720: Added OA script

On Thu, Sep 19, 2024 at 11:44:49PM +0200, Andrew Lunn wrote:
> Also 0x834 is BASE-T1 PMA/PMD control. Which is MDIO_PMA_PMD_BT1_CTRL
> 
> We also have:
> #define MDIO_PMA_PMD_BT1_CTRL_STRAP_B1000 0x0001 /* Select 1000BASE-T1 */
> #define MDIO_PMA_PMD_BT1_CTRL_CFG_MST		0x4000 /* MASTER-SLAVE config value */
> 
> 802.3 says bit 15 is read only, so you don't need to set it.
> 
> The rest might be magic which nobody outside of TI will understand,
> but you can fully document this.

Yes, this values will affect products of our customers. It is important
for us to know what and why is changed.

> > +static int dp83tg720_reset(struct phy_device *phydev, bool hw_reset)
> > +{
> > +	int ret;
> > +
> > +	if (hw_reset)
> > +		ret = phy_write_mmd(phydev, MMD1F, DP83TG720_PHY_RESET_CTRL,
> > +				DP83TG720_HW_RESET);
> > +	else
> > +		ret = phy_write_mmd(phydev, MMD1F, DP83TG720_PHY_RESET_CTRL,
> > +				DP83TG720_SW_RESET);
> > +	if (ret)
> > +		return ret;
> > +
> > +	mdelay(100);
> 
> Does the bit not self clear when it has completed? That would be
> common for a reset bit.

Not sure about status bit, but the time seems to be incorrect.
In case of HW reset, about 1ms delay is documented. This time should not
be needed for the SW reset. If it is really needed, please add comment
on where it is described.

With 100ms delay, this chip will be able to establish the link within
10ms (depending on the strap configuration) and then we will disable it again.

Even worse, this script includes already soft reset and power management
state changes. Im strongly against this script in that form.

Regards,
Oleksij
-- 
Pengutronix e.K.                           |                             |
Steuerwalder Str. 21                       | http://www.pengutronix.de/  |
31137 Hildesheim, Germany                  | Phone: +49-5121-206917-0    |
Amtsgericht Hildesheim, HRA 2686           | Fax:   +49-5121-206917-5555 |