[PATCH net-next 1/1] net: phy: unbind phy device from generic and specifical driver

[PATCH net-next 1/1] net: phy: unbind phy device from generic and specifical driver

[Fugang Duan]

The current .phy_detach() function only unbind generic phy driver, which causes
specifical driver suspend/resume function still work like Atheros AT803X PHYs.

For example:
ifconfig eth0 down
echo mem > /sys/power/status

After eth0 interface down, driver call phy_detach to unbind phy driver, and then
do suspend/resume operation, at803x_suspend()/at803x_resume() functions still get
called that call mdio bus read/write function. When eth0 interface down, MAC driver
may close all clocks and mdio bus cannot work. So the issue happens.

The patch can unbind generic and specifical driver.

Signed-off-by: Fugang Duan <B38611@freescale.com>
---
 drivers/net/phy/phy_device.c |   15 ++-------------
 1 files changed, 2 insertions(+), 13 deletions(-)

diff --git a/drivers/net/phy/phy_device.c b/drivers/net/phy/phy_device.c
index 3fc91e8..8adbc5d 100644
--- a/drivers/net/phy/phy_device.c
+++ b/drivers/net/phy/phy_device.c
@@ -672,8 +672,6 @@ EXPORT_SYMBOL(phy_attach);
  */
 void phy_detach(struct phy_device *phydev)
 {
-	int i;
-
 	if (phydev->bus->dev.driver)
 		module_put(phydev->bus->dev.driver->owner);
 
@@ -681,17 +679,8 @@ void phy_detach(struct phy_device *phydev)
 	phydev->attached_dev = NULL;
 	phy_suspend(phydev);
 
-	/* If the device had no specific driver before (i.e. - it
-	 * was using the generic driver), we unbind the device
-	 * from the generic driver so that there's a chance a
-	 * real driver could be loaded
-	 */
-	for (i = 0; i < ARRAY_SIZE(genphy_driver); i++) {
-		if (phydev->dev.driver == &genphy_driver[i].driver) {
-			device_release_driver(&phydev->dev);
-			break;
-		}
-	}
+	if (phydev->dev.driver)
+		device_release_driver(&phydev->dev);
 }
 EXPORT_SYMBOL(phy_detach);
 
-- 
1.7.8

Re: [PATCH net-next 1/1] net: phy: unbind phy device from generic and specifical driver

[Florian Fainelli]

On 26/01/15 00:14, Fugang Duan wrote:
> The current .phy_detach() function only unbind generic phy driver, which causes
> specifical driver suspend/resume function still work like Atheros AT803X PHYs.
> 
> For example:
> ifconfig eth0 down
> echo mem > /sys/power/status
> 
> After eth0 interface down, driver call phy_detach to unbind phy driver, and then
> do suspend/resume operation, at803x_suspend()/at803x_resume() functions still get
> called that call mdio bus read/write function. When eth0 interface down, MAC driver
> may close all clocks and mdio bus cannot work. So the issue happens.

I was just hitting this problem on Friday evening and was about to
submit a similar change. Thanks!

> 
> The patch can unbind generic and specifical driver.
> 
> Signed-off-by: Fugang Duan <B38611@freescale.com>

Acked-by: Florian Fainelli <f.fainelli@gmail.com>
Tested-by: Florian Fainelli <f.fainelli@gmail.com>


> ---
>  drivers/net/phy/phy_device.c |   15 ++-------------
>  1 files changed, 2 insertions(+), 13 deletions(-)
> 
> diff --git a/drivers/net/phy/phy_device.c b/drivers/net/phy/phy_device.c
> index 3fc91e8..8adbc5d 100644
> --- a/drivers/net/phy/phy_device.c
> +++ b/drivers/net/phy/phy_device.c
> @@ -672,8 +672,6 @@ EXPORT_SYMBOL(phy_attach);
>   */
>  void phy_detach(struct phy_device *phydev)
>  {
> -	int i;
> -
>  	if (phydev->bus->dev.driver)
>  		module_put(phydev->bus->dev.driver->owner);
>  
> @@ -681,17 +679,8 @@ void phy_detach(struct phy_device *phydev)
>  	phydev->attached_dev = NULL;
>  	phy_suspend(phydev);
>  
> -	/* If the device had no specific driver before (i.e. - it
> -	 * was using the generic driver), we unbind the device
> -	 * from the generic driver so that there's a chance a
> -	 * real driver could be loaded
> -	 */
> -	for (i = 0; i < ARRAY_SIZE(genphy_driver); i++) {
> -		if (phydev->dev.driver == &genphy_driver[i].driver) {
> -			device_release_driver(&phydev->dev);
> -			break;
> -		}
> -	}
> +	if (phydev->dev.driver)
> +		device_release_driver(&phydev->dev);
>  }
>  EXPORT_SYMBOL(phy_detach);
>  
> 


-- 
Florian

Re: [PATCH net-next 1/1] net: phy: unbind phy device from generic and specifical driver

[Florian Fainelli]

On 26/01/15 09:14, Florian Fainelli wrote:
> On 26/01/15 00:14, Fugang Duan wrote:
>> The current .phy_detach() function only unbind generic phy driver, which causes
>> specifical driver suspend/resume function still work like Atheros AT803X PHYs.
>>
>> For example:
>> ifconfig eth0 down
>> echo mem > /sys/power/status
>>
>> After eth0 interface down, driver call phy_detach to unbind phy driver, and then
>> do suspend/resume operation, at803x_suspend()/at803x_resume() functions still get
>> called that call mdio bus read/write function. When eth0 interface down, MAC driver
>> may close all clocks and mdio bus cannot work. So the issue happens.
> 
> I was just hitting this problem on Friday evening and was about to
> submit a similar change. Thanks!
> 
>>
>> The patch can unbind generic and specifical driver.
>>
>> Signed-off-by: Fugang Duan <B38611@freescale.com>
> 
> Acked-by: Florian Fainelli <f.fainelli@gmail.com>
> Tested-by: Florian Fainelli <f.fainelli@gmail.com>

Humm, this breaks a sequence of ifconfig down then up, the driver is
removed, and we never get to probe it again, does this work for you
using the FEC driver?

Another way to solve this double suspend problem is to make sure that we
track whether the PHY has already been suspended.
-- 
Florian

RE: [PATCH net-next 1/1] net: phy: unbind phy device from generic and specifical driver

[fugang.duan]

From: Florian Fainelli <f.fainelli@gmail.com> Sent: Tuesday, January 27, 2015 1:47 AM
> To: Duan Fugang-B38611; davem@davemloft.net
> Cc: netdev@vger.kernel.org; s.hauer@pengutronix.de;
> bhutchings@solarflare.com; stephen@networkplumber.org
> Subject: Re: [PATCH net-next 1/1] net: phy: unbind phy device from
> generic and specifical driver
> 
> On 26/01/15 09:14, Florian Fainelli wrote:
> > On 26/01/15 00:14, Fugang Duan wrote:
> >> The current .phy_detach() function only unbind generic phy driver,
> >> which causes specifical driver suspend/resume function still work like
> Atheros AT803X PHYs.
> >>
> >> For example:
> >> ifconfig eth0 down
> >> echo mem > /sys/power/status
> >>
> >> After eth0 interface down, driver call phy_detach to unbind phy
> >> driver, and then do suspend/resume operation,
> >> at803x_suspend()/at803x_resume() functions still get called that call
> >> mdio bus read/write function. When eth0 interface down, MAC driver may
> close all clocks and mdio bus cannot work. So the issue happens.
> >
> > I was just hitting this problem on Friday evening and was about to
> > submit a similar change. Thanks!
> >
> >>
> >> The patch can unbind generic and specifical driver.
> >>
> >> Signed-off-by: Fugang Duan <B38611@freescale.com>
> >
> > Acked-by: Florian Fainelli <f.fainelli@gmail.com>
> > Tested-by: Florian Fainelli <f.fainelli@gmail.com>
> 
> Humm, this breaks a sequence of ifconfig down then up, the driver is
> removed, and we never get to probe it again, does this work for you using
> the FEC driver?
> 
It works fine for FEC driver with below flow case.
echo enabled > /sys/class/tty/ttymxc0/power/wakeup
echo mem > /sys/power/state
ifconfig eth0 down
echo mem > /sys/power/state
ifconfig eth0 up
ping 10.192.242.204 //ping work fine
echo mem > /sys/power/state
...


I know the reason, the first fec driver use Atheros 8031 ethernet special phy driver, like:
fec 2188000.ethernet eth0: Freescale FEC PHY driver [Atheros 8031 ethernet] (mii_bus:phy_addr=2188000.ethernet:01, irq=-1)

After down and up, fec use generic phy driver, but it still can work fine:
fec 2188000.ethernet eth0: Freescale FEC PHY driver [Generic PHY] (mii_bus:phy_addr=2188000.ethernet:01, irq=-1)

After down and up, in here, phy driver only is initialized as generic phy driver.
int phy_attach_direct(struct net_device *dev, struct phy_device *phydev,
                      u32 flags, phy_interface_t interface)
{
	 ...
        /* Assume that if there is no driver, that it doesn't
         * exist, and we should use the genphy driver.
         */
        if (NULL == d->driver) {
                if (phydev->is_c45)
                        d->driver = &genphy_driver[GENPHY_DRV_10G].driver;
                else
                        d->driver = &genphy_driver[GENPHY_DRV_1G].driver;

                err = d->driver->probe(d);
                if (err >= 0)
                        err = device_bind_driver(d);

                if (err)
                        return err;
        }
	...
}

So, I suggest to use your patches set.

Regards,
Andy