Re: [PATCH v11 4/4] net: phy: bcm-phy-lib: Implement BroadR-Reach link modes

[Andrew Lunn]

On Fri, Jul 12, 2024 at 05:10:48PM +0200, Kamil Horák (2N) wrote:
> 
> On 7/11/24 21:01, Andrew Lunn wrote:
> > > +static int bcm5481x_get_brrmode(struct phy_device *phydev, u8 *data)
> > >   {
> > > -	int err, reg;
> > > +	int reg;
> > > -	/* Disable BroadR-Reach function. */
> > >   	reg = bcm_phy_read_exp(phydev, BCM54810_EXP_BROADREACH_LRE_MISC_CTL);
> > > -	reg &= ~BCM54810_EXP_BROADREACH_LRE_MISC_CTL_EN;
> > > -	err = bcm_phy_write_exp(phydev, BCM54810_EXP_BROADREACH_LRE_MISC_CTL,
> > > -				reg);
> > > -	if (err < 0)
> > bcm_phy_read_exp() could fail. So you should keep the test. Also, the
> > caller of this function does look at the return value.
> True - it can at least return -EOPNOTSUPP from __mdiobus_read()
> Trying to handle it.
> 
> This neglect can be found elsewhere such as bcm-phy-ptp.c  and eg.
> bcm54xx_config_init()
> 
> function. I feel that at least the latest one should be fixed but it would
> be unrelated to bcm54811,
> 
> so leaving it as-is for now.

In general PHY drivers are a bit hit and miss with checking error
codes. If the first access works, it is very likely all further
accesses will work. If they fail, the hardware is probably dead and
there is little you can do about it other than report the error. So i
would say probe, suspend and resume should always check the error
codes, since that is where clock problems are likely to be. But after
that it is good practice to check error codes, but a driver is
unlikely to be NACKed because of missing checks.

> Done. Now we rely on the DT setting and never read the PHY state. It is
> vulnerable to external manipulation
> 
> of MDIO registers and PHY reset as both hardware and software (bit 15 of
> register 0 in both
> 
> IEEE and LRE modes) reset switch to IEEE mode.

I don't think this is any worse. With the old code you would of
silently swapped to standard IEEE modes, which cannot work. Now you
continue programming BRR registers, which just get ignored because it
is no longer in that mode.

But if somebody performed some sort of external manipulation, all bets
are off anyway. 

	Andrew