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

[Serge Semin <fancer.lancer@gmail.com>]

Hello Heiner and net-folks,

On Sat, May 11, 2019 at 04:56:56PM +0200, 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,
> >>> there is a regression on linux v5.1-9573-gb970afcfcabd with a kernel null
> >>> pointer dereference.
> >>> The issue is the commit f81dadbcf7fd067baf184b63c179fc392bdb226e
> >>>  net: phy: realtek: Add rtl8211e rx/tx delays config ...
> >> The page operation callbacks are missing in the RTL8211E driver.
> >> I just submitted a fix adding these callbacks to few Realtek PHY drivers
> >> including RTl8211E. This should fix the issue.
> > 
> > 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?
> 
> 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.
> 

I'm sorry for the problems the patch caused. My fault I couldn't predict the
paged-operations weren't defined for the E-revision of the PHY.

Regarding the patch tests. As I mention in the patchset discussions, the patch
was ported from earlier versions of the kernel. In particular I created it for
kernels 4.4 and 4.9, where paged-operations weren't introduced. So when I moved
it to the modern kernel I found the paged operations availability and decided to
use them, which simplified the code providing a ready-to-use interface to access
the PHY' extension pages. I also found they were defined in the driver with
"rtl821x_" prefix and mistakenly decided, that they were also used for any
rtl-like device. That's where I let the bug to creep in.

Regarding the MAC-PHY link. Without this functionality our custom board of
MAC and rtl8211e PHY doesn't provide a fully supported network, because the
RXDLY and TXDLY pins are grounded so there is no a simple way to set the
RGMII delays on the PHY side.

Concerning the MAC-PHY link problem Vincente found I'll respond to the
corresponding email in three hours.

-Sergey

> > 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
> >> something for net-next once it's open again.
> >>
> >>> Regards,
> >>>  Vicenç.
> >>>
> >> Heiner
> >>
> >>> --- a/drivers/net/phy/phy-core.c
> >>> +++ b/drivers/net/phy/phy-core.c
> >>> @@ -648,11 +648,17 @@
> >>>
> >>> static int __phy_read_page(struct phy_device *phydev)
> >>> { ...
> >>
> >> Here phydev_warn() should be used.
> >>
> >>> +        return 0;
> >>> +    }
> >>>
> >>>     ret = phy_write(phydev, RTL821x_EXT_PAGE_SELECT, 0xa4);
> >>>     if (ret)
> > 
> > 
>