Re: net: phy: realtek: regression, kernel null pointer dereference

[Serge Semin]

On Mon, May 13, 2019 at 01:29:42PM +0300, Serge Semin wrote:
> Hello Vincente,
> 
> On Sat, May 11, 2019 at 05:06:04PM +0200, Vicente Bergas wrote:
> > On Saturday, May 11, 2019 4:56:56 PM CEST, Heiner Kallweit wrote:
> > > On 11.05.2019 16:46, Vicente Bergas wrote:
> > > > On Friday, May 10, 2019 10:28:06 PM CEST, Heiner Kallweit wrote:
> > > > > On 10.05.2019 17:05, Vicente Bergas wrote: ...
> > > > 
> > > > Hello Heiner,
> > > > just tried your patch and indeed the NPE is gone. But still no network...
> > > > The MAC <-> PHY link was working before, so, maybe the rgmii delays
> > > > are not
> > > > correctly configured.
> > > 
> > > That's a question to the author of the original patch. My patch was just
> > > meant to fix the NPE. In which configuration are you using the RTL8211E?
> > > As a standalone PHY (with which MAC/driver?) or is it the integrated PHY
> > > in a member of the RTL8168 family?
> > 
> > It is the one on the Sapphire board, so is connected to the MAC on the
> > RK3399 SoC. It is on page 8 of the schematics:
> > http://dl.vamrs.com/products/sapphire_excavator/RK_SAPPHIRE_SOCBOARD_RK3399_LPDDR3D178P232SD8_V12_20161109HXS.pdf
> > 
> 
> Thanks for sending this bug report.
> 
> As I said in the commit-message. The idea of this patch is to provide a way
> to setup the RGMII delays in the PHY drivers (similar to the most of the PHY
> drivers). Before this commit phy-mode dts-node hadn't been taked into account
> by the PHY driver, so any PHY-delay setups provided via external pins strapping
> were accepted as is. But now rtl8211e phy-mode is parsed as follows:
> phy-mode="rgmii" - delays aren't set by PHY (current dts setting in rk3399-sapphire.dtsi)
> phy-mode="rgmii-id" - both RX and TX delays are setup on the PHY side,
> phy-mode="rgmii-rxid" - only RX delay is setup on the PHY side,
> phy-mode="rgmii-txid" - only TX delay is setup on the PHY side.
> 
> It means, that now matter what the rtl8211e TXDLY/RXDLY pins are grounded or pulled
> high, the delays are going to be setup in accordance with the dts phy-mode settings,
> which is supposed to reflect the real hardware setup.
> 
> So since you get the problem with MAC<->PHY link, it means your dts-file didn't provide a
> correct interface mode. Indeed seeing the sheet on page 7 in the sepphire pdf-file your
> rtl8211e PHY is setup to have TXDLY/RXDLY being pulled high, which means to add 2ns delays
> by the PHY. This setup corresponds to phy-mode="rgmii-id". As soon as you set it this way
> in the rk3399 dts-file, the MAC-PHY link shall work correctly as before.
> 
> -Sergey

Hmm, just figured out, that in the datasheet RXDLY/TXDLY pins are actually grounded, so
phy-mode="rgmii" should work for you. Are you sure that on your actual hardware the
resistors aren't placed differently?

The current config register state can be read from the 0x1c extension page. Something
like this:
--- a/drivers/net/phy/realtek.c
+++ b/drivers/net/phy/realtek.c
@@ -221,6 +221,9 @@ static int rtl8211e_config_init(struct phy_device *phydev)
 	if (ret)
 		goto err_restore_page;
 
+	ret = phy_read(phydev, 0x1c);
+	dev_info(&phydev->mdio.dev, "PHY config register %08x\n", ret);
+
 	ret = phy_modify(phydev, 0x1c, RTL8211E_TX_DELAY | RTL8211E_RX_DELAY,
 			 val);
 
-Sergey

> 
> > > Serge: The issue with the NPE gave a hint already that you didn't test your
> > > patch. Was your patch based on an actual issue on some board and did you
> > > test it? We may have to consider reverting the patch.
> > > 
> > > > With this change it is back to working:
> > > > --- a/drivers/net/phy/realtek.c
> > > > +++ b/drivers/net/phy/realtek.c
> > > > @@ -300,7 +300,6 @@
> > > >     }, {
> > > >         PHY_ID_MATCH_EXACT(0x001cc915),
> > > >         .name        = "RTL8211E Gigabit Ethernet",
> > > > -        .config_init    = &rtl8211e_config_init,
> > > >         .ack_interrupt    = &rtl821x_ack_interrupt,
> > > >         .config_intr    = &rtl8211e_config_intr,
> > > >         .suspend    = genphy_suspend,
> > > > That is basically reverting the patch.
> > > > 
> > > > Regards,
> > > >  Vicenç.
> > > > 
> > > > > Nevertheless your proposed patch looks good to me, just one small change
> > > > > would be needed and it should be splitted.
> > > > > 
> > > > > The change to phy-core I would consider a fix and it should be fine to
> > > > > submit it to net (net-next is closed currently).
> > > > > 
> > > > > Adding the warning to the Realtek driver is fine, but this would be ...
> >