Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1161406AbdDUOSf (ORCPT ); Fri, 21 Apr 2017 10:18:35 -0400 Received: from lelnx193.ext.ti.com ([198.47.27.77]:36833 "EHLO lelnx193.ext.ti.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1040372AbdDUOS3 (ORCPT ); Fri, 21 Apr 2017 10:18:29 -0400 Subject: Re: [PATCH v1] net: phy: fix auto-negotiation stall due to unavailable interrupt To: Alexander Kochetkov , Florian Fainelli , , , Sergei Shtylyov , Madalin Bucur References: <1492686004-30527-1-git-send-email-al.kochet@gmail.com> <1492686004-30527-2-git-send-email-al.kochet@gmail.com> From: Roger Quadros Message-ID: <2f44a2db-da7a-2ca9-ea5e-6cc551c3008c@ti.com> Date: Fri, 21 Apr 2017 17:18:21 +0300 User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:45.0) Gecko/20100101 Thunderbird/45.8.0 MIME-Version: 1.0 In-Reply-To: <1492686004-30527-2-git-send-email-al.kochet@gmail.com> Content-Type: text/plain; charset="windows-1252" Content-Transfer-Encoding: 7bit Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org Content-Length: 4879 Lines: 129 On 20/04/17 14:00, Alexander Kochetkov wrote: > The Ethernet link on an interrupt driven PHY was not coming up if the Ethernet > cable was plugged before the Ethernet interface was brought up. > > The patch trigger PHY state machine to update link state if PHY was requested to > do auto-negotiation and auto-negotiation complete flag already set. > > During power-up cycle the PHY do auto-negotiation, generate interrupt and set > auto-negotiation complete flag. Interrupt is handled by PHY state machine but > doesn't update link state because PHY is in PHY_READY state. After some time > MAC bring up, start and request PHY to do auto-negotiation. If there are no new > settings to advertise genphy_config_aneg() doesn't start PHY auto-negotiation. > PHY continue to stay in auto-negotiation complete state and doesn't fire > interrupt. At the same time PHY state machine expect that PHY started > auto-negotiation and is waiting for interrupt from PHY and it won't get it. > > Signed-off-by: Alexander Kochetkov > Cc: stable # v4.9+ Tested-by: Roger Quadros I think the following commit broke functionality with interrupt driven PHYs 3c293f4e08b5 ("net: phy: Trigger state machine on state change and not polling.") cheers, -roger > --- > drivers/net/phy/phy.c | 40 ++++++++++++++++++++++++++++++++++++---- > include/linux/phy.h | 1 + > 2 files changed, 37 insertions(+), 4 deletions(-) > > diff --git a/drivers/net/phy/phy.c b/drivers/net/phy/phy.c > index 7cc1b7d..2d9975b 100644 > --- a/drivers/net/phy/phy.c > +++ b/drivers/net/phy/phy.c > @@ -591,16 +591,18 @@ int phy_mii_ioctl(struct phy_device *phydev, struct ifreq *ifr, int cmd) > EXPORT_SYMBOL(phy_mii_ioctl); > > /** > - * phy_start_aneg - start auto-negotiation for this PHY device > + * phy_start_aneg_priv - start auto-negotiation for this PHY device > * @phydev: the phy_device struct > + * @sync: indicate whether we should wait for the workqueue cancelation > * > * Description: Sanitizes the settings (if we're not autonegotiating > * them), and then calls the driver's config_aneg function. > * If the PHYCONTROL Layer is operating, we change the state to > * reflect the beginning of Auto-negotiation or forcing. > */ > -int phy_start_aneg(struct phy_device *phydev) > +static int phy_start_aneg_priv(struct phy_device *phydev, bool sync) > { > + bool trigger = 0; > int err; > > mutex_lock(&phydev->lock); > @@ -625,10 +627,40 @@ int phy_start_aneg(struct phy_device *phydev) > } > } > > + /* Re-schedule a PHY state machine to check PHY status because > + * negotiation may already be done and aneg interrupt may not be > + * generated. > + */ > + if (phy_interrupt_is_valid(phydev) && (phydev->state == PHY_AN)) { > + err = phy_aneg_done(phydev); > + if (err > 0) { > + trigger = true; > + err = 0; > + } > + } > + > out_unlock: > mutex_unlock(&phydev->lock); > + > + if (trigger) > + phy_trigger_machine(phydev, sync); > + > return err; > } > + > +/** > + * phy_start_aneg - start auto-negotiation for this PHY device > + * @phydev: the phy_device struct > + * > + * Description: Sanitizes the settings (if we're not autonegotiating > + * them), and then calls the driver's config_aneg function. > + * If the PHYCONTROL Layer is operating, we change the state to > + * reflect the beginning of Auto-negotiation or forcing. > + */ > +int phy_start_aneg(struct phy_device *phydev) > +{ > + return phy_start_aneg_priv(phydev, true); > +} > EXPORT_SYMBOL(phy_start_aneg); > > /** > @@ -656,7 +688,7 @@ void phy_start_machine(struct phy_device *phydev) > * state machine runs. > */ > > -static void phy_trigger_machine(struct phy_device *phydev, bool sync) > +void phy_trigger_machine(struct phy_device *phydev, bool sync) > { > if (sync) > cancel_delayed_work_sync(&phydev->state_queue); > @@ -1151,7 +1183,7 @@ void phy_state_machine(struct work_struct *work) > mutex_unlock(&phydev->lock); > > if (needs_aneg) > - err = phy_start_aneg(phydev); > + err = phy_start_aneg_priv(phydev, false); > else if (do_suspend) > phy_suspend(phydev); > > diff --git a/include/linux/phy.h b/include/linux/phy.h > index 7fc1105..b19ae66 100644 > --- a/include/linux/phy.h > +++ b/include/linux/phy.h > @@ -840,6 +840,7 @@ int phy_drivers_register(struct phy_driver *new_driver, int n, > void phy_mac_interrupt(struct phy_device *phydev, int new_link); > void phy_start_machine(struct phy_device *phydev); > void phy_stop_machine(struct phy_device *phydev); > +void phy_trigger_machine(struct phy_device *phydev, bool sync); > int phy_ethtool_sset(struct phy_device *phydev, struct ethtool_cmd *cmd); > int phy_ethtool_gset(struct phy_device *phydev, struct ethtool_cmd *cmd); > int phy_ethtool_ksettings_get(struct phy_device *phydev, >