On 16.10.2019 22:20, Florian Fainelli wrote: > On 10/16/19 12:53 PM, Heiner Kallweit wrote: >> Currently there's a bug in the module subsystem [0] preventing load of >> the PHY driver module on certain systems (as one symptom). >> This results in a NPE on such systems for the following reason: >> Instead of the correct PHY driver the genphy driver is loaded that >> doesn't implement the read_page/write_page callbacks. Every call to >> phy_read_paged() et al will result in a NPE therefore. >> >> In parallel to fixing the root cause we should make sure that this one >> and maybe similar issues in other subsystems don't result in a NPE >> in phylib. So let's check for the callbacks before using them and warn >> once if they are not available. > > Everywhere else in the PHY library we tend to do: > > if (!phydev->drv) > return -EIO; > A driver is bound, but genphy instead of the dedicated one. I think this check won't catch the error.
> maybe not the best choice for an error code, but we should be consistent. > > Is the issue really that we do have a driver we are bound to, but > somehow we cannot resolve the read_page/write_page callbacks to a valid > function pointer? > The issue is that loading the dedicated PHY driver module fails, therefore the (built-in) genphy driver is bound. Code in the r8169 driver expects that the dedicated driver for the internal PHY is bound and uses phy_read_page() et al what fails miserably with the genphy driver. >> >> [0] https://marc.info/?t=157072642100001&r=1&w=2 >> >> Signed-off-by: Heiner Kallweit <hkallwe...@gmail.com> >> --- >> drivers/net/phy/phy-core.c | 6 ++++++ >> 1 file changed, 6 insertions(+) >> >> diff --git a/drivers/net/phy/phy-core.c b/drivers/net/phy/phy-core.c >> index 9412669b5..0ae1722ba 100644 >> --- a/drivers/net/phy/phy-core.c >> +++ b/drivers/net/phy/phy-core.c >> @@ -689,11 +689,17 @@ EXPORT_SYMBOL_GPL(phy_modify_mmd); >> >> static int __phy_read_page(struct phy_device *phydev) >> { >> + if (WARN_ONCE(!phydev->drv->read_page, "read_page callback not >> available, PHY driver not loaded?\n")) >> + return -EOPNOTSUPP; >> + >> return phydev->drv->read_page(phydev); >> } >> >> static int __phy_write_page(struct phy_device *phydev, int page) >> { >> + if (WARN_ONCE(!phydev->drv->write_page, "write_page callback not >> available, PHY driver not loaded?\n")) >> + return -EOPNOTSUPP; >> + >> return phydev->drv->write_page(phydev, page); >> } >> >> > >
