From a016becd3a56cdb260013c8fe1d4aefc6edc4989 Mon Sep 17 00:00:00 2001 From: Heiner Kallweit Date: Wed, 23 Jan 2019 07:27:21 +0100 Subject: [PATCH 1/4] net: phy: start state machine in phy_start only The state machine is a no-op before phy_start() has been called. Therefore let's enable it in phy_start() only. In phy_start() let's call phy_start_machine() instead of phy_trigger_machine(). phy_start_machine is an alias for phy_trigger_machine but it makes clearer that we start the state machine here instead of just triggering a run. Signed-off-by: Heiner Kallweit Signed-off-by: David S. Miller --- drivers/net/phy/phy.c | 2 +- drivers/net/phy/phy_device.c | 1 - drivers/net/phy/phylink.c | 1 - 3 files changed, 1 insertion(+), 3 deletions(-) diff --git a/drivers/net/phy/phy.c b/drivers/net/phy/phy.c index 631ed33fe9d9..a7fca15888b6 100644 --- a/drivers/net/phy/phy.c +++ b/drivers/net/phy/phy.c @@ -878,7 +878,7 @@ void phy_start(struct phy_device *phydev) } mutex_unlock(&phydev->lock); - phy_trigger_machine(phydev); + phy_start_machine(phydev); } EXPORT_SYMBOL(phy_start); diff --git a/drivers/net/phy/phy_device.c b/drivers/net/phy/phy_device.c index 509d9402fa9a..64c25a0684ac 100644 --- a/drivers/net/phy/phy_device.c +++ b/drivers/net/phy/phy_device.c @@ -951,7 +951,6 @@ int phy_connect_direct(struct net_device *dev, struct phy_device *phydev, return rc; phy_prepare_link(phydev, handler); - phy_start_machine(phydev); if (phydev->irq > 0) phy_start_interrupts(phydev); diff --git a/drivers/net/phy/phylink.c b/drivers/net/phy/phylink.c index 7d9910da5018..c1b6e05ba60c 100644 --- a/drivers/net/phy/phylink.c +++ b/drivers/net/phy/phylink.c @@ -676,7 +676,6 @@ static int phylink_bringup_phy(struct phylink *pl, struct phy_device *phy) __ETHTOOL_LINK_MODE_MASK_NBITS, pl->supported, __ETHTOOL_LINK_MODE_MASK_NBITS, phy->advertising); - phy_start_machine(phy); if (phy->irq > 0) phy_start_interrupts(phy); From 217962615662f1ab7a60978a194444023039f0a4 Mon Sep 17 00:00:00 2001 From: Heiner Kallweit Date: Wed, 23 Jan 2019 07:30:38 +0100 Subject: [PATCH 2/4] net: phy: warn if phy_start is called from invalid state phy_start() should be called from states PHY_READY or PHY_HALTED only. Check for this to detect misbehaving drivers. Also the state machine should be started only when being called from one of the valid states. Some more background: For all invalid states phy_start() basically was a no-op. All it did was triggering a state machine run, but for all "running" states the poll loop was active anyway. And if called from PHY_DOWN, the state machine does nothing. v3: - extended commit message Signed-off-by: Heiner Kallweit Signed-off-by: David S. Miller --- drivers/net/phy/phy.c | 11 +++++++++-- 1 file changed, 9 insertions(+), 2 deletions(-) diff --git a/drivers/net/phy/phy.c b/drivers/net/phy/phy.c index a7fca15888b6..dedd57b86c80 100644 --- a/drivers/net/phy/phy.c +++ b/drivers/net/phy/phy.c @@ -856,9 +856,16 @@ void phy_start(struct phy_device *phydev) mutex_lock(&phydev->lock); + if (phydev->state != PHY_READY && phydev->state != PHY_HALTED) { + WARN(1, "called from state %s\n", + phy_state_to_str(phydev->state)); + goto out; + } + switch (phydev->state) { case PHY_READY: phydev->state = PHY_UP; + phy_start_machine(phydev); break; case PHY_HALTED: /* if phy was suspended, bring the physical link up again */ @@ -872,13 +879,13 @@ void phy_start(struct phy_device *phydev) } phydev->state = PHY_RESUMING; + phy_start_machine(phydev); break; default: break; } +out: mutex_unlock(&phydev->lock); - - phy_start_machine(phydev); } EXPORT_SYMBOL(phy_start); From 9e573cfc35c6ffac45e385ed4f14f5187c09090a Mon Sep 17 00:00:00 2001 From: Heiner Kallweit Date: Wed, 23 Jan 2019 07:31:23 +0100 Subject: [PATCH 3/4] net: phy: start interrupts in phy_start Interrupts don't have to be enabled before calling phy_start(). Therefore let's enable them in phy_start(). In a subsequent step we'll remove enabling interrupts from phy_connect_direct(). Signed-off-by: Heiner Kallweit Signed-off-by: David S. Miller --- drivers/net/phy/phy.c | 36 +++++++++++++++--------------------- 1 file changed, 15 insertions(+), 21 deletions(-) diff --git a/drivers/net/phy/phy.c b/drivers/net/phy/phy.c index dedd57b86c80..079b6a617fc8 100644 --- a/drivers/net/phy/phy.c +++ b/drivers/net/phy/phy.c @@ -852,7 +852,7 @@ EXPORT_SYMBOL(phy_stop); */ void phy_start(struct phy_device *phydev) { - int err = 0; + int err; mutex_lock(&phydev->lock); @@ -862,28 +862,22 @@ void phy_start(struct phy_device *phydev) goto out; } - switch (phydev->state) { - case PHY_READY: - phydev->state = PHY_UP; - phy_start_machine(phydev); - break; - case PHY_HALTED: - /* if phy was suspended, bring the physical link up again */ - __phy_resume(phydev); + /* if phy was suspended, bring the physical link up again */ + __phy_resume(phydev); - /* make sure interrupts are re-enabled for the PHY */ - if (phy_interrupt_is_valid(phydev)) { - err = phy_enable_interrupts(phydev); - if (err < 0) - break; - } - - phydev->state = PHY_RESUMING; - phy_start_machine(phydev); - break; - default: - break; + /* make sure interrupts are enabled for the PHY */ + if (phy_interrupt_is_valid(phydev)) { + err = phy_enable_interrupts(phydev); + if (err < 0) + goto out; } + + if (phydev->state == PHY_READY) + phydev->state = PHY_UP; + else + phydev->state = PHY_RESUMING; + + phy_start_machine(phydev); out: mutex_unlock(&phydev->lock); } From 434a4315b9617bf1742bc64712bf44a208502f7f Mon Sep 17 00:00:00 2001 From: Heiner Kallweit Date: Wed, 23 Jan 2019 07:31:58 +0100 Subject: [PATCH 4/4] net: phy: change phy_start_interrupts to phy_request_interrupt Now that we enable the interrupts in phy_start() we don't have to do it before. Therefore remove enabling interrupts from phy_start_interrupts() and rename this function to reflect the changed functionality. v2: - improve warning to clearly state that we fall back to polling Signed-off-by: Heiner Kallweit Signed-off-by: David S. Miller --- drivers/net/phy/phy.c | 25 ++++++++++++------------- drivers/net/phy/phy_device.c | 4 ++-- drivers/net/phy/phylink.c | 4 ++-- include/linux/phy.h | 2 +- 4 files changed, 17 insertions(+), 18 deletions(-) diff --git a/drivers/net/phy/phy.c b/drivers/net/phy/phy.c index 079b6a617fc8..d12aa512b7f5 100644 --- a/drivers/net/phy/phy.c +++ b/drivers/net/phy/phy.c @@ -785,28 +785,27 @@ static int phy_enable_interrupts(struct phy_device *phydev) } /** - * phy_start_interrupts - request and enable interrupts for a PHY device + * phy_request_interrupt - request interrupt for a PHY device * @phydev: target phy_device struct * * Description: Request the interrupt for the given PHY. * If this fails, then we set irq to PHY_POLL. - * Otherwise, we enable the interrupts in the PHY. * This should only be called with a valid IRQ number. - * Returns 0 on success or < 0 on error. */ -int phy_start_interrupts(struct phy_device *phydev) +void phy_request_interrupt(struct phy_device *phydev) { - if (request_threaded_irq(phydev->irq, NULL, phy_interrupt, - IRQF_ONESHOT | IRQF_SHARED, - phydev_name(phydev), phydev) < 0) { - phydev_warn(phydev, "Can't get IRQ %d\n", phydev->irq); - phydev->irq = PHY_POLL; - return 0; - } + int err; - return phy_enable_interrupts(phydev); + err = request_threaded_irq(phydev->irq, NULL, phy_interrupt, + IRQF_ONESHOT | IRQF_SHARED, + phydev_name(phydev), phydev); + if (err) { + phydev_warn(phydev, "Error %d requesting IRQ %d, falling back to polling\n", + err, phydev->irq); + phydev->irq = PHY_POLL; + } } -EXPORT_SYMBOL(phy_start_interrupts); +EXPORT_SYMBOL(phy_request_interrupt); /** * phy_stop - Bring down the PHY link, and stop checking the status diff --git a/drivers/net/phy/phy_device.c b/drivers/net/phy/phy_device.c index 64c25a0684ac..891e0178b97f 100644 --- a/drivers/net/phy/phy_device.c +++ b/drivers/net/phy/phy_device.c @@ -951,8 +951,8 @@ int phy_connect_direct(struct net_device *dev, struct phy_device *phydev, return rc; phy_prepare_link(phydev, handler); - if (phydev->irq > 0) - phy_start_interrupts(phydev); + if (phy_interrupt_is_valid(phydev)) + phy_request_interrupt(phydev); return 0; } diff --git a/drivers/net/phy/phylink.c b/drivers/net/phy/phylink.c index c1b6e05ba60c..2e21ce42e388 100644 --- a/drivers/net/phy/phylink.c +++ b/drivers/net/phy/phylink.c @@ -676,8 +676,8 @@ static int phylink_bringup_phy(struct phylink *pl, struct phy_device *phy) __ETHTOOL_LINK_MODE_MASK_NBITS, pl->supported, __ETHTOOL_LINK_MODE_MASK_NBITS, phy->advertising); - if (phy->irq > 0) - phy_start_interrupts(phy); + if (phy_interrupt_is_valid(phy)) + phy_request_interrupt(phy); return 0; } diff --git a/include/linux/phy.h b/include/linux/phy.h index 1f3873a2ff29..70f83d0d7469 100644 --- a/include/linux/phy.h +++ b/include/linux/phy.h @@ -1047,7 +1047,7 @@ void phy_ethtool_ksettings_get(struct phy_device *phydev, int phy_ethtool_ksettings_set(struct phy_device *phydev, const struct ethtool_link_ksettings *cmd); int phy_mii_ioctl(struct phy_device *phydev, struct ifreq *ifr, int cmd); -int phy_start_interrupts(struct phy_device *phydev); +void phy_request_interrupt(struct phy_device *phydev); void phy_print_status(struct phy_device *phydev); int phy_set_max_speed(struct phy_device *phydev, u32 max_speed); void phy_remove_link_mode(struct phy_device *phydev, u32 link_mode);