On 15.10.2019 00:12, Russell King - ARM Linux admin wrote:

On Mon, Oct 14, 2019 at 10:20:15PM +0200, Heiner Kallweit wrote:

quoted

On 14.10.2019 21:51, Stefan Wahren wrote:

quoted

[add more recipients]

Am 14.10.19 um 21:25 schrieb Daniel Wagner:

quoted

Moving the phy_prepare_link() up in phy_connect_direct() ensures that
phydev->adjust_link is set when the phy_check_link_status() is called.

diff --git a/drivers/net/phy/phy_device.c

b/drivers/net/phy/phy_device.c index 9d2bbb13293e..2a61812bcb0d 100644

--- a/drivers/net/phy/phy_device.c +++ b/drivers/net/phy/phy_device.c

@@ -951,11 +951,12 @@ int phy_connect_direct(struct net_device *dev,

struct phy_device *phydev, if (!dev) return -EINVAL;

+       phy_prepare_link(phydev, handler);
+
        rc = phy_attach_direct(dev, phydev, phydev->dev_flags, interface);
        if (rc)

If phy_attach_direct() fails we may have to reset phydev->adjust_link to NULL,
as we do in phy_disconnect(). Apart from that change looks good to me.

Sorry, but it doesn't look good to me.

I think there's a deeper question here - why is the phy state machine
trying to call the link change function during attach?

After your comment I had a closer look at the lm78xx driver and few things
look suspicious:

- lan78xx_phy_init() (incl. the call to phy_connect_direct()) is called
  after register_netdev(). This may cause races.

- The following is wrong, irq = 0 doesn't mean polling.
  PHY_POLL is defined as -1. Also in case of irq = 0 phy_interrupt_is_valid()
  returns true.

	/* if phyirq is not set, use polling mode in phylib */
	if (dev->domain_data.phyirq > 0)
		phydev->irq = dev->domain_data.phyirq;
	else
		phydev->irq = 0;

- Manually calling genphy_config_aneg() in lan78xx_phy_init() isn't
  needed, however this should not cause our problem.

Bugs in the network driver would also explain why the issue doesn't occur
on other systems. Once we know more about the actual root cause
maybe phylib can be extended to detect that situation and warn.

At this point, the PHY hasn't been "started" so it shouldn't be
doing that.

Note the documentation, specifically phy.rst's "Keeping Close Tabs on
the PAL" section.  Drivers are at liberty to use phy_prepare_link()
_after_ phy_attach(), which means there is a window for
phydev->adjust_link to be NULL.  It should _not_ be called at this
point.