Received-SPF: pass (google.com: best guess record for domain of linux-kernel-owner@vger.kernel.org designates 209.132.180.67 as permitted sender) client-ip=209.132.180.67;
Date:   Mon, 13 May 2019 10:29:10 +0300
From:   Serge Semin <fancer.lancer@gmail.com>
To:     Heiner Kallweit <hkallweit1@gmail.com>
Cc:     Vicente Bergas <vicencb@gmail.com>,
        Russell King <rmk+kernel@armlinux.org.uk>,
        Andrew Lunn <andrew@lunn.ch>,
        Florian Fainelli <f.fainelli@gmail.com>,
        netdev@vger.kernel.org, linux-kernel@vger.kernel.org
Subject: Re: net: phy: realtek: regression, kernel null pointer dereference
Message-ID: <20190513072909.behzxeisc7l3nprq@mobilestation>
References: <16f75ff4-e3e3-4d96-b084-e772e3ce1c2b@gmail.com>
 <742a2235-4571-aa7d-af90-14c708205c6f@gmail.com>
 <11446b0b-c8a4-4e5f-bfa0-0892b500f467@gmail.com>
 <61831f43-3b24-47d9-ec6f-15be6a4568c5@gmail.com>
MIME-Version: 1.0
Content-Type: text/plain; charset=iso-8859-1
Content-Disposition: inline
Content-Transfer-Encoding: 8bit
In-Reply-To: <61831f43-3b24-47d9-ec6f-15be6a4568c5@gmail.com>
User-Agent: NeoMutt/20180716
Sender: linux-kernel-owner@vger.kernel.org
Precedence: bulk

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