[PATCH] NET: bcm63xx_enet: move phy_(dis)connect into probe/remove

[PATCH] NET: bcm63xx_enet: move phy_(dis)connect into probe/remove

[Jonas Gorski]

Only connect/disconnect the phy during probe and remove, not during open
and close. The phy seldom changes during the runtime, and disconnecting
the phy during close will prevent the phy driver from keeping any
configuration over a down/up cycle.

Signed-off-by: Jonas Gorski <jonas.gorski@gmail.com>
---
 drivers/net/ethernet/broadcom/bcm63xx_enet.c |   84 +++++++++++++-------------
 1 files changed, 41 insertions(+), 43 deletions(-)

diff --git a/drivers/net/ethernet/broadcom/bcm63xx_enet.c b/drivers/net/ethernet/broadcom/bcm63xx_enet.c
index c7ca7ec..2744cf0 100644
--- a/drivers/net/ethernet/broadcom/bcm63xx_enet.c
+++ b/drivers/net/ethernet/broadcom/bcm63xx_enet.c
@@ -784,10 +784,8 @@ static int bcm_enet_open(struct net_device *dev)
 	struct bcm_enet_priv *priv;
 	struct sockaddr addr;
 	struct device *kdev;
-	struct phy_device *phydev;
 	int i, ret;
 	unsigned int size;
-	char phy_id[MII_BUS_ID_SIZE + 3];
 	void *p;
 	u32 val;
 
@@ -795,40 +793,10 @@ static int bcm_enet_open(struct net_device *dev)
 	kdev = &priv->pdev->dev;
 
 	if (priv->has_phy) {
-		/* connect to PHY */
-		snprintf(phy_id, sizeof(phy_id), PHY_ID_FMT,
-			 priv->mii_bus->id, priv->phy_id);
-
-		phydev = phy_connect(dev, phy_id, bcm_enet_adjust_phy_link, 0,
-				     PHY_INTERFACE_MODE_MII);
-
-		if (IS_ERR(phydev)) {
-			dev_err(kdev, "could not attach to PHY\n");
-			return PTR_ERR(phydev);
-		}
-
-		/* mask with MAC supported features */
-		phydev->supported &= (SUPPORTED_10baseT_Half |
-				      SUPPORTED_10baseT_Full |
-				      SUPPORTED_100baseT_Half |
-				      SUPPORTED_100baseT_Full |
-				      SUPPORTED_Autoneg |
-				      SUPPORTED_Pause |
-				      SUPPORTED_MII);
-		phydev->advertising = phydev->supported;
-
-		if (priv->pause_auto && priv->pause_rx && priv->pause_tx)
-			phydev->advertising |= SUPPORTED_Pause;
-		else
-			phydev->advertising &= ~SUPPORTED_Pause;
-
-		dev_info(kdev, "attached PHY at address %d [%s]\n",
-			 phydev->addr, phydev->drv->name);
-
+		/* Reset state */
 		priv->old_link = 0;
 		priv->old_duplex = -1;
 		priv->old_pause = -1;
-		priv->phydev = phydev;
 	}
 
 	/* mask all interrupts and request them */
@@ -838,7 +806,7 @@ static int bcm_enet_open(struct net_device *dev)
 
 	ret = request_irq(dev->irq, bcm_enet_isr_mac, 0, dev->name, dev);
 	if (ret)
-		goto out_phy_disconnect;
+		return ret;
 
 	ret = request_irq(priv->irq_rx, bcm_enet_isr_dma, IRQF_DISABLED,
 			  dev->name, dev);
@@ -1025,9 +993,6 @@ out_freeirq_rx:
 out_freeirq:
 	free_irq(dev->irq, dev);
 
-out_phy_disconnect:
-	phy_disconnect(priv->phydev);
-
 	return ret;
 }
 
@@ -1132,12 +1097,6 @@ static int bcm_enet_stop(struct net_device *dev)
 	free_irq(priv->irq_rx, dev);
 	free_irq(dev->irq, dev);
 
-	/* release phy */
-	if (priv->has_phy) {
-		phy_disconnect(priv->phydev);
-		priv->phydev = NULL;
-	}
-
 	return 0;
 }
 
@@ -1714,6 +1673,8 @@ static int __devinit bcm_enet_probe(struct platform_device *pdev)
 
 	/* MII bus registration */
 	if (priv->has_phy) {
+		struct phy_device *phydev;
+		char phy_id[MII_BUS_ID_SIZE + 3];
 
 		priv->mii_bus = mdiobus_alloc();
 		if (!priv->mii_bus) {
@@ -1750,6 +1711,38 @@ static int __devinit bcm_enet_probe(struct platform_device *pdev)
 			dev_err(&pdev->dev, "unable to register mdio bus\n");
 			goto out_free_mdio;
 		}
+
+		/* connect to PHY */
+		snprintf(phy_id, sizeof(phy_id), PHY_ID_FMT,
+			 priv->mii_bus->id, priv->phy_id);
+
+		phydev = phy_connect(dev, phy_id, bcm_enet_adjust_phy_link, 0,
+				     PHY_INTERFACE_MODE_MII);
+
+		if (IS_ERR(phydev)) {
+			dev_err(&pdev->dev, "could not attach to PHY\n");
+			goto out_unregister_mdio;
+		}
+
+		/* mask with MAC supported features */
+		phydev->supported &= (SUPPORTED_10baseT_Half |
+				      SUPPORTED_10baseT_Full |
+				      SUPPORTED_100baseT_Half |
+				      SUPPORTED_100baseT_Full |
+				      SUPPORTED_Autoneg |
+				      SUPPORTED_Pause |
+				      SUPPORTED_MII);
+		phydev->advertising = phydev->supported;
+
+		if (priv->pause_auto && priv->pause_rx && priv->pause_tx)
+			phydev->advertising |= SUPPORTED_Pause;
+		else
+			phydev->advertising &= ~SUPPORTED_Pause;
+
+		dev_info(&pdev->dev, "attached PHY at address %d [%s]\n",
+			 phydev->addr, phydev->drv->name);
+
+		priv->phydev = phydev;
 	} else {
 
 		/* run platform code to initialize PHY device */
@@ -1795,6 +1788,9 @@ static int __devinit bcm_enet_probe(struct platform_device *pdev)
 	return 0;
 
 out_unregister_mdio:
+	if (!IS_ERR_OR_NULL(priv->phydev))
+		phy_disconnect(priv->phydev);
+
 	if (priv->mii_bus) {
 		mdiobus_unregister(priv->mii_bus);
 		kfree(priv->mii_bus->irq);
@@ -1845,6 +1841,8 @@ static int __devexit bcm_enet_remove(struct platform_device *pdev)
 	enet_writel(priv, 0, ENET_MIISC_REG);
 
 	if (priv->has_phy) {
+		phy_disconnect(priv->phydev);
+		priv->phydev = NULL;
 		mdiobus_unregister(priv->mii_bus);
 		kfree(priv->mii_bus->irq);
 		mdiobus_free(priv->mii_bus);
-- 
1.7.2.5

Re: [PATCH] NET: bcm63xx_enet: move phy_(dis)connect into probe/remove

[Florian Fainelli]

Le 04/18/12 14:02, Jonas Gorski a écrit :
> Only connect/disconnect the phy during probe and remove, not during open
> and close. The phy seldom changes during the runtime, and disconnecting
> the phy during close will prevent the phy driver from keeping any
> configuration over a down/up cycle.
>
> Signed-off-by: Jonas Gorski<jonas.gorski@gmail.com>

Acked-by: Florian Fainelli <florian@openwrt.org>

> ---
>   drivers/net/ethernet/broadcom/bcm63xx_enet.c |   84 +++++++++++++-------------
>   1 files changed, 41 insertions(+), 43 deletions(-)
>
> diff --git a/drivers/net/ethernet/broadcom/bcm63xx_enet.c b/drivers/net/ethernet/broadcom/bcm63xx_enet.c
> index c7ca7ec..2744cf0 100644
> --- a/drivers/net/ethernet/broadcom/bcm63xx_enet.c
> +++ b/drivers/net/ethernet/broadcom/bcm63xx_enet.c
> @@ -784,10 +784,8 @@ static int bcm_enet_open(struct net_device *dev)
>   	struct bcm_enet_priv *priv;
>   	struct sockaddr addr;
>   	struct device *kdev;
> -	struct phy_device *phydev;
>   	int i, ret;
>   	unsigned int size;
> -	char phy_id[MII_BUS_ID_SIZE + 3];
>   	void *p;
>   	u32 val;
>
> @@ -795,40 +793,10 @@ static int bcm_enet_open(struct net_device *dev)
>   	kdev =&priv->pdev->dev;
>
>   	if (priv->has_phy) {
> -		/* connect to PHY */
> -		snprintf(phy_id, sizeof(phy_id), PHY_ID_FMT,
> -			 priv->mii_bus->id, priv->phy_id);
> -
> -		phydev = phy_connect(dev, phy_id, bcm_enet_adjust_phy_link, 0,
> -				     PHY_INTERFACE_MODE_MII);
> -
> -		if (IS_ERR(phydev)) {
> -			dev_err(kdev, "could not attach to PHY\n");
> -			return PTR_ERR(phydev);
> -		}
> -
> -		/* mask with MAC supported features */
> -		phydev->supported&= (SUPPORTED_10baseT_Half |
> -				      SUPPORTED_10baseT_Full |
> -				      SUPPORTED_100baseT_Half |
> -				      SUPPORTED_100baseT_Full |
> -				      SUPPORTED_Autoneg |
> -				      SUPPORTED_Pause |
> -				      SUPPORTED_MII);
> -		phydev->advertising = phydev->supported;
> -
> -		if (priv->pause_auto&&  priv->pause_rx&&  priv->pause_tx)
> -			phydev->advertising |= SUPPORTED_Pause;
> -		else
> -			phydev->advertising&= ~SUPPORTED_Pause;
> -
> -		dev_info(kdev, "attached PHY at address %d [%s]\n",
> -			 phydev->addr, phydev->drv->name);
> -
> +		/* Reset state */
>   		priv->old_link = 0;
>   		priv->old_duplex = -1;
>   		priv->old_pause = -1;
> -		priv->phydev = phydev;
>   	}
>
>   	/* mask all interrupts and request them */
> @@ -838,7 +806,7 @@ static int bcm_enet_open(struct net_device *dev)
>
>   	ret = request_irq(dev->irq, bcm_enet_isr_mac, 0, dev->name, dev);
>   	if (ret)
> -		goto out_phy_disconnect;
> +		return ret;
>
>   	ret = request_irq(priv->irq_rx, bcm_enet_isr_dma, IRQF_DISABLED,
>   			  dev->name, dev);
> @@ -1025,9 +993,6 @@ out_freeirq_rx:
>   out_freeirq:
>   	free_irq(dev->irq, dev);
>
> -out_phy_disconnect:
> -	phy_disconnect(priv->phydev);
> -
>   	return ret;
>   }
>
> @@ -1132,12 +1097,6 @@ static int bcm_enet_stop(struct net_device *dev)
>   	free_irq(priv->irq_rx, dev);
>   	free_irq(dev->irq, dev);
>
> -	/* release phy */
> -	if (priv->has_phy) {
> -		phy_disconnect(priv->phydev);
> -		priv->phydev = NULL;
> -	}
> -
>   	return 0;
>   }
>
> @@ -1714,6 +1673,8 @@ static int __devinit bcm_enet_probe(struct platform_device *pdev)
>
>   	/* MII bus registration */
>   	if (priv->has_phy) {
> +		struct phy_device *phydev;
> +		char phy_id[MII_BUS_ID_SIZE + 3];
>
>   		priv->mii_bus = mdiobus_alloc();
>   		if (!priv->mii_bus) {
> @@ -1750,6 +1711,38 @@ static int __devinit bcm_enet_probe(struct platform_device *pdev)
>   			dev_err(&pdev->dev, "unable to register mdio bus\n");
>   			goto out_free_mdio;
>   		}
> +
> +		/* connect to PHY */
> +		snprintf(phy_id, sizeof(phy_id), PHY_ID_FMT,
> +			 priv->mii_bus->id, priv->phy_id);
> +
> +		phydev = phy_connect(dev, phy_id, bcm_enet_adjust_phy_link, 0,
> +				     PHY_INTERFACE_MODE_MII);
> +
> +		if (IS_ERR(phydev)) {
> +			dev_err(&pdev->dev, "could not attach to PHY\n");
> +			goto out_unregister_mdio;
> +		}
> +
> +		/* mask with MAC supported features */
> +		phydev->supported&= (SUPPORTED_10baseT_Half |
> +				      SUPPORTED_10baseT_Full |
> +				      SUPPORTED_100baseT_Half |
> +				      SUPPORTED_100baseT_Full |
> +				      SUPPORTED_Autoneg |
> +				      SUPPORTED_Pause |
> +				      SUPPORTED_MII);
> +		phydev->advertising = phydev->supported;
> +
> +		if (priv->pause_auto&&  priv->pause_rx&&  priv->pause_tx)
> +			phydev->advertising |= SUPPORTED_Pause;
> +		else
> +			phydev->advertising&= ~SUPPORTED_Pause;
> +
> +		dev_info(&pdev->dev, "attached PHY at address %d [%s]\n",
> +			 phydev->addr, phydev->drv->name);
> +
> +		priv->phydev = phydev;
>   	} else {
>
>   		/* run platform code to initialize PHY device */
> @@ -1795,6 +1788,9 @@ static int __devinit bcm_enet_probe(struct platform_device *pdev)
>   	return 0;
>
>   out_unregister_mdio:
> +	if (!IS_ERR_OR_NULL(priv->phydev))
> +		phy_disconnect(priv->phydev);
> +
>   	if (priv->mii_bus) {
>   		mdiobus_unregister(priv->mii_bus);
>   		kfree(priv->mii_bus->irq);
> @@ -1845,6 +1841,8 @@ static int __devexit bcm_enet_remove(struct platform_device *pdev)
>   	enet_writel(priv, 0, ENET_MIISC_REG);
>
>   	if (priv->has_phy) {
> +		phy_disconnect(priv->phydev);
> +		priv->phydev = NULL;
>   		mdiobus_unregister(priv->mii_bus);
>   		kfree(priv->mii_bus->irq);
>   		mdiobus_free(priv->mii_bus);

Re: [PATCH] NET: bcm63xx_enet: move phy_(dis)connect into probe/remove

[Maxime Bizon]

On Wed, 2012-04-18 at 14:02 +0200, Jonas Gorski wrote:

> Only connect/disconnect the phy during probe and remove, not during open
> and close. The phy seldom changes during the runtime, and disconnecting
> the phy during close will prevent the phy driver from keeping any
> configuration over a down/up cycle.
> 
> Signed-off-by: Jonas Gorski <jonas.gorski@gmail.com>

please CC me, I wrote this driver

> -		phydev = phy_connect(dev, phy_id, bcm_enet_adjust_phy_link, 0,
> -				     PHY_INTERFACE_MODE_MII);

bcm_enet_adjust_link() may modify some dma registers that are reset by
bcm_enet_open(), since it can now be called after probe, we may end up
with broken flow control depending on whatever was called first.


-- 
Maxime

Re: [PATCH] NET: bcm63xx_enet: move phy_(dis)connect into probe/remove

[Jonas Gorski]

Hi Maxime,

On 18 April 2012 14:48, Maxime Bizon <mbizon@freebox.fr> wrote:
>
> On Wed, 2012-04-18 at 14:02 +0200, Jonas Gorski wrote:
>
>> Only connect/disconnect the phy during probe and remove, not during open
>> and close. The phy seldom changes during the runtime, and disconnecting
>> the phy during close will prevent the phy driver from keeping any
>> configuration over a down/up cycle.
>>
>> Signed-off-by: Jonas Gorski <jonas.gorski@gmail.com>
>
> please CC me, I wrote this driver

Oops, sorry, that wasn't intentional. I used get_maintainer.pl, which
didn't catch you, and I never checked the actual file. Duly noted for
v2.

>
>> -             phydev = phy_connect(dev, phy_id, bcm_enet_adjust_phy_link, 0,
>> -                                  PHY_INTERFACE_MODE_MII);
>
> bcm_enet_adjust_link() may modify some dma registers that are reset by
> bcm_enet_open(), since it can now be called after probe, we may end up
> with broken flow control depending on whatever was called first.

I assume you mean bcm_enet_adjust_phy_link()? bcm_enet_adjust_link()
gets only called if there is no phydev.

I have to admit, I fail to see the race (but feel free to correct me):

On boot:

The phy state machine will start in PHY_READY after phy_connect, which
will result in NOPs, so no call to bcm_enet_adjust_phy_link() after
_probe() and before _open(). The state machine starts doing real work
only after calling phy_start(), which happens only after the dma
register accesses in open(). So no race here.

In case of an down/up cycle:

_close() will call phy_stop() first, which will either block until the
current state machine run is complete if there is one, or will block
the next run until it set the state to PHY_HALTED.

When in PHY_HALTED, bcm_enet_adjust_phy_link() will be called once,
but with phy_dev->link = 0, and only phy_dev->link = 1 can result in
register writes in bcm_enet_adjust_phy_link():

	if (phydev->link && phydev->duplex != priv->old_duplex) {
		bcm_enet_set_duplex(priv,
				    (phydev->duplex == DUPLEX_FULL) ? 1 : 0);
		...
	}

	if (phydev->link && phydev->pause != priv->old_pause) {
		...
		bcm_enet_set_flow(priv, rx_pause_en, tx_pause_en);
		...
	}

So no problem there either.

There is a theoretical race in writing to priv->old_link in _open()
and the bcm_enet_adjust_phy_link() call right after phy_stop(), but
since both write the same value to priv->old_link I see no problem
here.


Regards
Jonas

Re: [PATCH] NET: bcm63xx_enet: move phy_(dis)connect into probe/remove

[Maxime Bizon]

On Wed, 2012-04-18 at 21:30 +0200, Jonas Gorski wrote:
> 
> The phy state machine will start in PHY_READY after phy_connect, which
> will result in NOPs, so no call to bcm_enet_adjust_phy_link() after
> _probe() and before _open(). The state machine starts doing real work
> only after calling phy_start(), which happens only after the dma
> register accesses in open(). So no race here. 

If I read the phylib code correctly, after phy_connect() the phy is in
READY state, and you can access it via ethtool.

-- 
Maxime

Re: [PATCH] NET: bcm63xx_enet: move phy_(dis)connect into probe/remove

[Jonas Gorski]

On 19 April 2012 15:33, Maxime Bizon <mbizon@freebox.fr> wrote:
>
> On Wed, 2012-04-18 at 21:30 +0200, Jonas Gorski wrote:
>>
>> The phy state machine will start in PHY_READY after phy_connect, which
>> will result in NOPs, so no call to bcm_enet_adjust_phy_link() after
>> _probe() and before _open(). The state machine starts doing real work
>> only after calling phy_start(), which happens only after the dma
>> register accesses in open(). So no race here.
>
> If I read the phylib code correctly, after phy_connect() the phy is in
> READY state, and you can access it via ethtool.

Yes, but none of the ethtool functions cause register writes in the
priv->has_phy = true case when in PHY_READY or PHY_HALTED state. All
they do is modify the phy_device's settings.
The settings only get taken over when the phy is started and the
phy_state_machine is running, so after phy_start() was and before
phy_stop().

In the priv->has_phy = false case they do cause register writes since
bcm_enet_adjust_link() might get called, but these never use the phy
state machine, so my patch does not change anything there.


Jonas

Re: [PATCH] NET: bcm63xx_enet: move phy_(dis)connect into probe/remove

[Maxime Bizon]

On Thu, 2012-04-19 at 16:52 +0200, Jonas Gorski wrote:

> Yes, but none of the ethtool functions cause register writes in the
> priv->has_phy = true case when in PHY_READY or PHY_HALTED state. All
> they do is modify the phy_device's settings.

unless I'm mistaken:

phy_ethtool_sset() => phy_start_aneg()

will kick the state machine even when state is PHY_READY

-- 
Maxime

Re: [PATCH] NET: bcm63xx_enet: move phy_(dis)connect into probe/remove

[Jonas Gorski]

On 19 April 2012 18:17, Maxime Bizon <mbizon@freebox.fr> wrote:
>
> On Thu, 2012-04-19 at 16:52 +0200, Jonas Gorski wrote:
>
>> Yes, but none of the ethtool functions cause register writes in the
>> priv->has_phy = true case when in PHY_READY or PHY_HALTED state. All
>> they do is modify the phy_device's settings.
>
> unless I'm mistaken:
>
> phy_ethtool_sset() => phy_start_aneg()
>
> will kick the state machine even when state is PHY_READY

Hmm. I see what you mean. I wonder if it is intended that you can do
that without having phy_start() called first.

@Andy, can you perhaps shed some light on this? How are ethernet
drivers supposed to behave/when should they call
phy_connect()/phy_start()? Currently most drivers call phy_connect()
in their _probe(), and phy_start() in _open(), so many seem to have
the issue that the phy state machine is in PHY_READY after _probe(),
and can be kicked into running through ethtool even if the interface
is down.

This problem goes away after the first ifup/ifdown cycle, since the
phy state machine is then in PHY_HALTED, which gets properly caught in
phy_start_aneg().

To me it looks like phy_start_aneg() should check for some more
states, as it currently would also overwrite a PHY_STARTING or
PHY_PENDING state, which looks definitely wrong to me.


Jonas

Re: [PATCH] NET: bcm63xx_enet: move phy_(dis)connect into probe/remove

[Andy Fleming]

On Sun, Apr 22, 2012 at 6:31 AM, Jonas Gorski <jonas.gorski@gmail.com> wrote:
> On 19 April 2012 18:17, Maxime Bizon <mbizon@freebox.fr> wrote:
>>
>> On Thu, 2012-04-19 at 16:52 +0200, Jonas Gorski wrote:
>>
>>> Yes, but none of the ethtool functions cause register writes in the
>>> priv->has_phy = true case when in PHY_READY or PHY_HALTED state. All
>>> they do is modify the phy_device's settings.
>>
>> unless I'm mistaken:
>>
>> phy_ethtool_sset() => phy_start_aneg()
>>
>> will kick the state machine even when state is PHY_READY
>
> Hmm. I see what you mean. I wonder if it is intended that you can do
> that without having phy_start() called first.
>
> @Andy, can you perhaps shed some light on this? How are ethernet
> drivers supposed to behave/when should they call
> phy_connect()/phy_start()? Currently most drivers call phy_connect()
> in their _probe(), and phy_start() in _open(), so many seem to have
> the issue that the phy state machine is in PHY_READY after _probe(),
> and can be kicked into running through ethtool even if the interface
> is down.
>
> This problem goes away after the first ifup/ifdown cycle, since the
> phy state machine is then in PHY_HALTED, which gets properly caught in
> phy_start_aneg().
>
> To me it looks like phy_start_aneg() should check for some more
> states, as it currently would also overwrite a PHY_STARTING or
> PHY_PENDING state, which looks definitely wrong to me.

Ugh, it looks like much has gone wrong with the state machine
(possibly from when I wrote it). I'm thinking that what we should
probably do is eliminate PHY_UP and PHY_READY, and have the PHY come
up to PHY_HALTED. For some reason, PHY_RESUMING always enables
interrupts, even if phydev->irq is in POLL mode, so that should be
fixed.

Other than that, it looks like PHY_RESUMING should work in the place
of PHY_UP, and PHY_HALTED should be the same as PHY_READY...

Andy