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

[Serge Semin]

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

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