[PATCH 1/2] Revert commit 1b0a83ac04e383e3bed21332962b90710fcf2828

[PATCH 1/2] Revert commit 1b0a83ac04e383e3bed21332962b90710fcf2828

[Badel, Laurent]

Description: This patch reverts commit 1b0a83ac04e3 
("net: fec: add phy_reset_after_clk_enable() support")
which produces undesirable behavior when PHY interrupts are enabled.

Rationale: the SMSC LAN8720 (and possibly other chips) is known
to require a reset after the external clock is enabled. Calls to
phy_reset_after_clk_enable() in fec_main.c have been introduced in 
commit 1b0a83ac04e3 ("net: fec: add phy_reset_after_clk_enable() support")
to handle the chip reset after enabling the clock.
However, this breaks when interrupts are enabled because
the reset reverts the configuration of the PHY interrupt mask to default
(in addition it also reverts the "energy detect" mode setting).
As a result the driver does not receive the link status change
and other notifications resulting in loss of connectivity. 

Proposed solution: revert commit 1b0a83ac04e3 and bring the reset 
before the PHY configuration by adding it to phy_init_hw() [phy_device.c].

Test results: using an iMX28-EVK-based board, these 2 patches successfully
restore network interface functionality when interrupts are enabled.
Tested using both linux-5.4.23 and latest mainline (5.6.0) kernels.

Fixes: 1b0a83ac04e383e3bed21332962b90710fcf2828 ("net: fec: add phy_reset_after_clk_enable() support")
Signed-off-by: Laurent Badel <laurentbadel@eaton.com>

---
 drivers/net/ethernet/freescale/fec_main.c | 19 -------------------
 1 file changed, 19 deletions(-)

diff --git a/drivers/net/ethernet/freescale/fec_main.c b/drivers/net/ethernet/freescale/fec_main.c
index 23c5fef2f..02b014837 100644
--- a/drivers/net/ethernet/freescale/fec_main.c
+++ b/drivers/net/ethernet/freescale/fec_main.c
@@ -1918,7 +1918,6 @@ static int fec_enet_clk_enable(struct net_device *ndev, bool enable)
 		if (ret)
 			goto failed_clk_ref;
 
-		phy_reset_after_clk_enable(ndev->phydev);
 	} else {
 		clk_disable_unprepare(fep->clk_enet_out);
 		if (fep->clk_ptp) {
@@ -2895,7 +2894,6 @@ fec_enet_open(struct net_device *ndev)
 {
 	struct fec_enet_private *fep = netdev_priv(ndev);
 	int ret;
-	bool reset_again;
 
 	ret = pm_runtime_get_sync(&fep->pdev->dev);
 	if (ret < 0)
@@ -2906,17 +2904,6 @@ fec_enet_open(struct net_device *ndev)
 	if (ret)
 		goto clk_enable;
 
-	/* During the first fec_enet_open call the PHY isn't probed at this
-	 * point. Therefore the phy_reset_after_clk_enable() call within
-	 * fec_enet_clk_enable() fails. As we need this reset in order to be
-	 * sure the PHY is working correctly we check if we need to reset again
-	 * later when the PHY is probed
-	 */
-	if (ndev->phydev && ndev->phydev->drv)
-		reset_again = false;
-	else
-		reset_again = true;
-
 	/* I should reset the ring buffers here, but I don't yet know
 	 * a simple way to do that.
 	 */
@@ -2933,12 +2920,6 @@ fec_enet_open(struct net_device *ndev)
 	if (ret)
 		goto err_enet_mii_probe;
 
-	/* Call phy_reset_after_clk_enable() again if it failed during
-	 * phy_reset_after_clk_enable() before because the PHY wasn't probed.
-	 */
-	if (reset_again)
-		phy_reset_after_clk_enable(ndev->phydev);
-
 	if (fep->quirks & FEC_QUIRK_ERR006687)
 		imx6q_cpuidle_fec_irqs_used();
 
-- 
2.17.1


-----------------------------
Eaton Industries Manufacturing GmbH ~ Registered place of business: Route de la Longeraie 7, 1110, Morges, Switzerland 

-----------------------------

Re: [PATCH 1/2] Revert commit 1b0a83ac04e383e3bed21332962b90710fcf2828

[gregkh]

On Wed, Apr 29, 2020 at 09:03:32AM +0000, Badel, Laurent wrote:
> Description: This patch reverts commit 1b0a83ac04e3 
> ("net: fec: add phy_reset_after_clk_enable() support")
> which produces undesirable behavior when PHY interrupts are enabled.
> 
> Rationale: the SMSC LAN8720 (and possibly other chips) is known
> to require a reset after the external clock is enabled. Calls to
> phy_reset_after_clk_enable() in fec_main.c have been introduced in 
> commit 1b0a83ac04e3 ("net: fec: add phy_reset_after_clk_enable() support")
> to handle the chip reset after enabling the clock.
> However, this breaks when interrupts are enabled because
> the reset reverts the configuration of the PHY interrupt mask to default
> (in addition it also reverts the "energy detect" mode setting).
> As a result the driver does not receive the link status change
> and other notifications resulting in loss of connectivity. 
> 
> Proposed solution: revert commit 1b0a83ac04e3 and bring the reset 
> before the PHY configuration by adding it to phy_init_hw() [phy_device.c].
> 
> Test results: using an iMX28-EVK-based board, these 2 patches successfully
> restore network interface functionality when interrupts are enabled.
> Tested using both linux-5.4.23 and latest mainline (5.6.0) kernels.
> 
> Fixes: 1b0a83ac04e383e3bed21332962b90710fcf2828 ("net: fec: add phy_reset_after_clk_enable() support")

Please read Documentation/process/submitting-patches.rst and the section
"2) Describe your changes" at the end, it says how to do lines like this
"properly".

thanks,

greg k-h

RE: [EXTERNAL] Re: [PATCH 1/2] Revert commit 1b0a83ac04e383e3bed21332962b90710fcf2828

[Badel, Laurent]

Dear Greg, 

Thanks for your reply and sorry for my mistake.
Looks to me like the issue is the commit hash which should be 12 chars.
Does that mean I need to fix and resend the whole thing to everyone? 

Best regards,

Laurent

> 

-----------------------------
Eaton Industries Manufacturing GmbH ~ Registered place of business: Route de la Longeraie 7, 1110, Morges, Switzerland 

-----------------------------

-----Original Message-----
> From: gregkh@linuxfoundation.org <gregkh@linuxfoundation.org>
> Sent: Wednesday, April 29, 2020 11:40 AM
> To: Badel, Laurent <LaurentBadel@eaton.com>
> Cc: fugang.duan@nxp.com; netdev@vger.kernel.org; andrew@lunn.ch;
> f.fainelli@gmail.com; hkallweit1@gmail.com; linux@armlinux.org.uk;
> richard.leitner@skidata.com; davem@davemloft.net;
> alexander.levin@microsoft.com; Quette, Arnaud
> <ArnaudQuette@Eaton.com>
> Subject: [EXTERNAL] Re: [PATCH 1/2] Revert commit
> 1b0a83ac04e383e3bed21332962b90710fcf2828
> 
> On Wed, Apr 29, 2020 at 09:03:32AM +0000, Badel, Laurent wrote:
> > Description: This patch reverts commit 1b0a83ac04e3
> > ("net: fec: add phy_reset_after_clk_enable() support") which produces
> > undesirable behavior when PHY interrupts are enabled.
> >
> > Rationale: the SMSC LAN8720 (and possibly other chips) is known to
> > require a reset after the external clock is enabled. Calls to
> > phy_reset_after_clk_enable() in fec_main.c have been introduced in
> > commit 1b0a83ac04e3 ("net: fec: add phy_reset_after_clk_enable()
> > support") to handle the chip reset after enabling the clock.
> > However, this breaks when interrupts are enabled because the reset
> > reverts the configuration of the PHY interrupt mask to default (in
> > addition it also reverts the "energy detect" mode setting).
> > As a result the driver does not receive the link status change and
> > other notifications resulting in loss of connectivity.
> >
> > Proposed solution: revert commit 1b0a83ac04e3 and bring the reset
> > before the PHY configuration by adding it to phy_init_hw() [phy_device.c].
> >
> > Test results: using an iMX28-EVK-based board, these 2 patches
> > successfully restore network interface functionality when interrupts are
> enabled.
> > Tested using both linux-5.4.23 and latest mainline (5.6.0) kernels.
> >
> > Fixes: 1b0a83ac04e383e3bed21332962b90710fcf2828 ("net: fec: add
> > phy_reset_after_clk_enable() support")
> 
> Please read Documentation/process/submitting-patches.rst and the section
> "2) Describe your changes" at the end, it says how to do lines like this
> "properly".
> 
> thanks,
> 
> greg k-h

Re: [EXTERNAL] Re: [PATCH 1/2] Revert commit 1b0a83ac04e383e3bed21332962b90710fcf2828

[gregkh]

On Wed, Apr 29, 2020 at 11:22:58AM +0000, Badel, Laurent wrote:
> Dear Greg, 
> 
> Thanks for your reply and sorry for my mistake.
> Looks to me like the issue is the commit hash which should be 12 chars.
> Does that mean I need to fix and resend the whole thing to everyone? 

Yes please, never force a maintainer to hand-edit a patch, they will not
do so :)

thanks,

greg k-h

Re: [PATCH 1/2] Revert commit 1b0a83ac04e383e3bed21332962b90710fcf2828

[Andrew Lunn]

On Wed, Apr 29, 2020 at 09:03:32AM +0000, Badel, Laurent wrote:
> Description: This patch reverts commit 1b0a83ac04e3 
> ("net: fec: add phy_reset_after_clk_enable() support")
> which produces undesirable behavior when PHY interrupts are enabled.
> 
> Rationale: the SMSC LAN8720 (and possibly other chips) is known
> to require a reset after the external clock is enabled. Calls to
> phy_reset_after_clk_enable() in fec_main.c have been introduced in 
> commit 1b0a83ac04e3 ("net: fec: add phy_reset_after_clk_enable() support")
> to handle the chip reset after enabling the clock.
> However, this breaks when interrupts are enabled because
> the reset reverts the configuration of the PHY interrupt mask to default
> (in addition it also reverts the "energy detect" mode setting).
> As a result the driver does not receive the link status change
> and other notifications resulting in loss of connectivity. 
> 
> Proposed solution: revert commit 1b0a83ac04e3 and bring the reset 
> before the PHY configuration by adding it to phy_init_hw() [phy_device.c].
> 
> Test results: using an iMX28-EVK-based board, these 2 patches successfully
> restore network interface functionality when interrupts are enabled.
> Tested using both linux-5.4.23 and latest mainline (5.6.0) kernels.
> 
> Fixes: 1b0a83ac04e383e3bed21332962b90710fcf2828 ("net: fec: add phy_reset_after_clk_enable() support")
> Signed-off-by: Laurent Badel <laurentbadel@eaton.com>

Hi Laurent

Please also read https://www.kernel.org/doc/Documentation/networking/netdev-FAQ.txt

       Andrew

Re: [PATCH 1/2] Revert commit 1b0a83ac04e383e3bed21332962b90710fcf2828

[Andrew Lunn]

> Test results: using an iMX28-EVK-based board, this patch successfully
> restores network interface functionality when interrupts are enabled.
> Tested using both linux-5.4.23 and latest mainline (5.6.0) kernels.

Hi Laurent

What tree are these patches against?

That is why i pointed you to the netdev FAQ.

Also, for a multi-part series, you should add a cover latter.

     Andrew

Re: [PATCH 1/2] Revert commit 1b0a83ac04e383e3bed21332962b90710fcf2828

[Badel, Laurent]

Description: This patch reverts commit 1b0a83ac04e3 
("net: fec: add phy_reset_after_clk_enable() support")
which produces undesirable behavior when PHY interrupts are enabled.

Rationale: the SMSC LAN8720 (and possibly other chips) is known
to require a reset after the external clock is enabled. Calls to
phy_reset_after_clk_enable() in fec_main.c have been introduced in 
commit 1b0a83ac04e3 ("net: fec: add phy_reset_after_clk_enable() support")
to handle the chip reset after enabling the clock.
However, this breaks when interrupts are enabled because
the reset reverts the configuration of the PHY interrupt mask to default
(in addition it also reverts the "energy detect" mode setting).
As a result the driver does not receive the link status change
and other notifications resulting in loss of connectivity. 

Proposed solution: revert commit 1b0a83ac04e3 and bring the reset 
before the PHY configuration by adding it to phy_init_hw() [phy_device.c].

Test results: using an iMX28-EVK-based board, this patch successfully
restores network interface functionality when interrupts are enabled.
Tested using both linux-5.4.23 and latest mainline (5.6.0) kernels.

Fixes: 1b0a83ac04e3 ("net: fec: add phy_reset_after_clk_enable() support")
Signed-off-by: Laurent Badel <laurentbadel@eaton.com>

---
 drivers/net/ethernet/freescale/fec_main.c | 19 -------------------
 1 file changed, 19 deletions(-)

diff --git a/drivers/net/ethernet/freescale/fec_main.c b/drivers/net/ethernet/freescale/fec_main.c
index 23c5fef2f..02b014837 100644
--- a/drivers/net/ethernet/freescale/fec_main.c
+++ b/drivers/net/ethernet/freescale/fec_main.c
@@ -1918,7 +1918,6 @@ static int fec_enet_clk_enable(struct net_device *ndev, bool enable)
 		if (ret)
 			goto failed_clk_ref;
 
-		phy_reset_after_clk_enable(ndev->phydev);
 	} else {
 		clk_disable_unprepare(fep->clk_enet_out);
 		if (fep->clk_ptp) {
@@ -2895,7 +2894,6 @@ fec_enet_open(struct net_device *ndev)
 {
 	struct fec_enet_private *fep = netdev_priv(ndev);
 	int ret;
-	bool reset_again;
 
 	ret = pm_runtime_get_sync(&fep->pdev->dev);
 	if (ret < 0)
@@ -2906,17 +2904,6 @@ fec_enet_open(struct net_device *ndev)
 	if (ret)
 		goto clk_enable;
 
-	/* During the first fec_enet_open call the PHY isn't probed at this
-	 * point. Therefore the phy_reset_after_clk_enable() call within
-	 * fec_enet_clk_enable() fails. As we need this reset in order to be
-	 * sure the PHY is working correctly we check if we need to reset again
-	 * later when the PHY is probed
-	 */
-	if (ndev->phydev && ndev->phydev->drv)
-		reset_again = false;
-	else
-		reset_again = true;
-
 	/* I should reset the ring buffers here, but I don't yet know
 	 * a simple way to do that.
 	 */
@@ -2933,12 +2920,6 @@ fec_enet_open(struct net_device *ndev)
 	if (ret)
 		goto err_enet_mii_probe;
 
-	/* Call phy_reset_after_clk_enable() again if it failed during
-	 * phy_reset_after_clk_enable() before because the PHY wasn't probed.
-	 */
-	if (reset_again)
-		phy_reset_after_clk_enable(ndev->phydev);
-
 	if (fep->quirks & FEC_QUIRK_ERR006687)
 		imx6q_cpuidle_fec_irqs_used();
 
-- 
2.17.1


> 

-----------------------------
Eaton Industries Manufacturing GmbH ~ Registered place of business: Route de la Longeraie 7, 1110, Morges, Switzerland 

-----------------------------

-----Original Message-----
> From: gregkh@linuxfoundation.org <gregkh@linuxfoundation.org>
> Sent: Wednesday, April 29, 2020 1:55 PM
> To: Badel, Laurent <LaurentBadel@eaton.com>
> Cc: fugang.duan@nxp.com; netdev@vger.kernel.org; andrew@lunn.ch;
> f.fainelli@gmail.com; hkallweit1@gmail.com; linux@armlinux.org.uk;
> richard.leitner@skidata.com; davem@davemloft.net;
> alexander.levin@microsoft.com; Quette, Arnaud
> <ArnaudQuette@Eaton.com>
> Subject: Re: [EXTERNAL] Re: [PATCH 1/2] Revert commit
> 1b0a83ac04e383e3bed21332962b90710fcf2828
> 
> On Wed, Apr 29, 2020 at 11:22:58AM +0000, Badel, Laurent wrote:
> > Dear Greg,
> >
> > Thanks for your reply and sorry for my mistake.
> > Looks to me like the issue is the commit hash which should be 12 chars.
> > Does that mean I need to fix and resend the whole thing to everyone?
> 
> Yes please, never force a maintainer to hand-edit a patch, they will not do
> so :)
> 
> thanks,
> 
> greg k-h