[PATCH] Revert "phy: add support for a reset-gpio specification"

[PATCH] Revert "phy: add support for a reset-gpio specification"

[Fabio Estevam]

Commit da47b4572056 ("phy: add support for a reset-gpio specification")
causes the following xtensa qemu crash according to Guenter Roeck:

[    9.366256] libphy: ethoc-mdio: probed
[    9.367389]  (null): could not attach to PHY
[    9.368555]  (null): failed to probe MDIO bus
[    9.371540] Unable to handle kernel paging request at virtual address 0000001c
[    9.371540]  pc = d0320926, ra = 903209d1
[    9.375358] Oops: sig: 11 [#1]

This reverts commit da47b4572056487fd7941c26f73b3e8815ff712a.

Reported-by: Guenter Roeck <linux@roeck-us.net>
Signed-off-by: Fabio Estevam <fabio.estevam@nxp.com>
---
 Documentation/devicetree/bindings/net/phy.txt | 3 ---
 drivers/net/phy/phy_device.c                  | 8 --------
 2 files changed, 11 deletions(-)

diff --git a/Documentation/devicetree/bindings/net/phy.txt b/Documentation/devicetree/bindings/net/phy.txt
index c00a9a8..bc1c3c8 100644
--- a/Documentation/devicetree/bindings/net/phy.txt
+++ b/Documentation/devicetree/bindings/net/phy.txt
@@ -35,8 +35,6 @@ Optional Properties:
 - broken-turn-around: If set, indicates the PHY device does not correctly
   release the turn around line low at the end of a MDIO transaction.
 
-- reset-gpios: Reference to a GPIO used to reset the phy.
-
 Example:
 
 ethernet-phy@0 {
@@ -44,5 +42,4 @@ ethernet-phy@0 {
 	interrupt-parent = <40000>;
 	interrupts = <35 1>;
 	reg = <0>;
-	reset-gpios = <&gpio1 17 GPIO_ACTIVE_LOW>;
 };
diff --git a/drivers/net/phy/phy_device.c b/drivers/net/phy/phy_device.c
index 307f72a..e977ba9 100644
--- a/drivers/net/phy/phy_device.c
+++ b/drivers/net/phy/phy_device.c
@@ -34,7 +34,6 @@
 #include <linux/io.h>
 #include <linux/uaccess.h>
 #include <linux/of.h>
-#include <linux/gpio/consumer.h>
 
 #include <asm/irq.h>
 
@@ -1571,16 +1570,9 @@ static int phy_probe(struct device *dev)
 	struct device_driver *drv = phydev->mdio.dev.driver;
 	struct phy_driver *phydrv = to_phy_driver(drv);
 	int err = 0;
-	struct gpio_descs *reset_gpios;
 
 	phydev->drv = phydrv;
 
-	/* take phy out of reset */
-	reset_gpios = devm_gpiod_get_array_optional(dev, "reset",
-						    GPIOD_OUT_LOW);
-	if (IS_ERR(reset_gpios))
-		return PTR_ERR(reset_gpios);
-
 	/* Disable the interrupt if the PHY doesn't support it
 	 * but the interrupt is still a valid one
 	 */
-- 
1.9.1

Re: [PATCH] Revert "phy: add support for a reset-gpio specification"

[Florian Fainelli]

On 05/18/2016 09:05 AM, Fabio Estevam wrote:
> Commit da47b4572056 ("phy: add support for a reset-gpio specification")
> causes the following xtensa qemu crash according to Guenter Roeck:
> 
> [    9.366256] libphy: ethoc-mdio: probed
> [    9.367389]  (null): could not attach to PHY
> [    9.368555]  (null): failed to probe MDIO bus
> [    9.371540] Unable to handle kernel paging request at virtual address 0000001c
> [    9.371540]  pc = d0320926, ra = 903209d1
> [    9.375358] Oops: sig: 11 [#1]
> 
> This reverts commit da47b4572056487fd7941c26f73b3e8815ff712a.
> 
> Reported-by: Guenter Roeck <linux@roeck-us.net>
> Signed-off-by: Fabio Estevam <fabio.estevam@nxp.com>

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

Thanks!
-- 
Florian

Re: [PATCH] Revert "phy: add support for a reset-gpio specification"

[Guenter Roeck]

On Wed, May 18, 2016 at 09:52:22AM -0700, Florian Fainelli wrote:
> On 05/18/2016 09:05 AM, Fabio Estevam wrote:
> > Commit da47b4572056 ("phy: add support for a reset-gpio specification")
> > causes the following xtensa qemu crash according to Guenter Roeck:
> > 
> > [    9.366256] libphy: ethoc-mdio: probed
> > [    9.367389]  (null): could not attach to PHY
> > [    9.368555]  (null): failed to probe MDIO bus
> > [    9.371540] Unable to handle kernel paging request at virtual address 0000001c
> > [    9.371540]  pc = d0320926, ra = 903209d1
> > [    9.375358] Oops: sig: 11 [#1]
> > 
> > This reverts commit da47b4572056487fd7941c26f73b3e8815ff712a.
> > 
> > Reported-by: Guenter Roeck <linux@roeck-us.net>
> > Signed-off-by: Fabio Estevam <fabio.estevam@nxp.com>
> 
> Acked-by: Florian Fainelli <f.fainelli@gmail.com>
> 
Tested-by: Guenter Roeck <linux@roeck-us.net>

Re: [PATCH] Revert "phy: add support for a reset-gpio specification"

[Sergei Shtylyov]

Hello.

On 5/18/2016 7:05 PM, Fabio Estevam wrote:

> Commit da47b4572056 ("phy: add support for a reset-gpio specification")
> causes the following xtensa qemu crash according to Guenter Roeck:
>
> [    9.366256] libphy: ethoc-mdio: probed
> [    9.367389]  (null): could not attach to PHY
> [    9.368555]  (null): failed to probe MDIO bus
> [    9.371540] Unable to handle kernel paging request at virtual address 0000001c
> [    9.371540]  pc = d0320926, ra = 903209d1
> [    9.375358] Oops: sig: 11 [#1]

    Of course, I'd like this patch to be reverted (as it was applied 
erroneously instead of my patches)... but where's the backtrace? It's not 
immediately clear how this code can cause kernel oops...

> This reverts commit da47b4572056487fd7941c26f73b3e8815ff712a.
>
> Reported-by: Guenter Roeck <linux@roeck-us.net>
> Signed-off-by: Fabio Estevam <fabio.estevam@nxp.com>
[...]
> diff --git a/drivers/net/phy/phy_device.c b/drivers/net/phy/phy_device.c
> index 307f72a..e977ba9 100644
> --- a/drivers/net/phy/phy_device.c
> +++ b/drivers/net/phy/phy_device.c
[...]
> @@ -1571,16 +1570,9 @@ static int phy_probe(struct device *dev)
>  	struct device_driver *drv = phydev->mdio.dev.driver;
>  	struct phy_driver *phydrv = to_phy_driver(drv);
>  	int err = 0;
> -	struct gpio_descs *reset_gpios;
>
>  	phydev->drv = phydrv;
>
> -	/* take phy out of reset */
> -	reset_gpios = devm_gpiod_get_array_optional(dev, "reset",
> -						    GPIOD_OUT_LOW);
> -	if (IS_ERR(reset_gpios))
> -		return PTR_ERR(reset_gpios);
> -
>  	/* Disable the interrupt if the PHY doesn't support it
>  	 * but the interrupt is still a valid one
>  	 */

MBR, Sergei

Re: [PATCH] Revert "phy: add support for a reset-gpio specification"

[Guenter Roeck]

On 05/19/2016 06:11 AM, Sergei Shtylyov wrote:
> Hello.
>
> On 5/18/2016 7:05 PM, Fabio Estevam wrote:
>
>> Commit da47b4572056 ("phy: add support for a reset-gpio specification")
>> causes the following xtensa qemu crash according to Guenter Roeck:
>>
>> [    9.366256] libphy: ethoc-mdio: probed
>> [    9.367389]  (null): could not attach to PHY
>> [    9.368555]  (null): failed to probe MDIO bus
>> [    9.371540] Unable to handle kernel paging request at virtual address 0000001c
>> [    9.371540]  pc = d0320926, ra = 903209d1
>> [    9.375358] Oops: sig: 11 [#1]
>
>     Of course, I'd like this patch to be reverted (as it was applied erroneously instead of my patches)... but where's the backtrace? It's not immediately clear how this code can cause kernel oops...
>

See http://www.spinics.net/lists/kernel/msg2258611.html.

Guenter

>> This reverts commit da47b4572056487fd7941c26f73b3e8815ff712a.
>>
>> Reported-by: Guenter Roeck <linux@roeck-us.net>
>> Signed-off-by: Fabio Estevam <fabio.estevam@nxp.com>
> [...]
>> diff --git a/drivers/net/phy/phy_device.c b/drivers/net/phy/phy_device.c
>> index 307f72a..e977ba9 100644
>> --- a/drivers/net/phy/phy_device.c
>> +++ b/drivers/net/phy/phy_device.c
> [...]
>> @@ -1571,16 +1570,9 @@ static int phy_probe(struct device *dev)
>>      struct device_driver *drv = phydev->mdio.dev.driver;
>>      struct phy_driver *phydrv = to_phy_driver(drv);
>>      int err = 0;
>> -    struct gpio_descs *reset_gpios;
>>
>>      phydev->drv = phydrv;
>>
>> -    /* take phy out of reset */
>> -    reset_gpios = devm_gpiod_get_array_optional(dev, "reset",
>> -                            GPIOD_OUT_LOW);
>> -    if (IS_ERR(reset_gpios))
>> -        return PTR_ERR(reset_gpios);
>> -
>>      /* Disable the interrupt if the PHY doesn't support it
>>       * but the interrupt is still a valid one
>>       */
>
> MBR, Sergei
>
>

Re: [PATCH] Revert "phy: add support for a reset-gpio specification"

[Sergei Shtylyov]

Hello.

On 05/19/2016 04:29 PM, Guenter Roeck wrote:

>>> Commit da47b4572056 ("phy: add support for a reset-gpio specification")
>>> causes the following xtensa qemu crash according to Guenter Roeck:
>>>
>>> [    9.366256] libphy: ethoc-mdio: probed
>>> [    9.367389]  (null): could not attach to PHY
>>> [    9.368555]  (null): failed to probe MDIO bus
>>> [    9.371540] Unable to handle kernel paging request at virtual address
>>> 0000001c
>>> [    9.371540]  pc = d0320926, ra = 903209d1
>>> [    9.375358] Oops: sig: 11 [#1]
>>
>>     Of course, I'd like this patch to be reverted (as it was applied
>> erroneously instead of my patches)... but where's the backtrace? It's not
>> immediately clear how this code can cause kernel oops...
>>
> See http://www.spinics.net/lists/kernel/msg2258611.html.

    OK, I'll try to comment on your analysis posted there:

> GPIOLIB is not configured in my test case, meaning gpiod_get_optional()
> returns -ENOSYS, and phy_probe() thus returns an error. Question here is if
> it is really appropriate for the XXX_optional() gpiolib functions to return
> an error if GPIOLIB is not configured. Either case, result is that pretty
> much all phy registrations will now fail if GPIOLIB is not configured.

    Yes, that's the primary issue. That's what needs to be fixed...

> Also, I suspect that there may be a bug in the error handling path
> of ethoc_probe(). No idea what exactly is wrong, though. Other drivers
> use pretty much the same code sequence for mdio registration and associated
> error handling.

    I tried analyzing this code and it wasn't clear what's the issue...
    I must mention that this code actually looked strange -- other drivers 
call phy_connect_direct() etc. from their ndo_open() method, this driver does 
this from the probe. One problem I'm seing is that iff register_netdev() 
fails, it forgets to disconnect from the PHY.

> Last but not least, something seems to be wrong with the use of dev_err()
> with &netdev->dev if register_netdev() has not yet been called. Maybe someone
> has some insight ?

    Yes, this needs fixing as well.

> Guenter

MBR, Sergei

Re: [PATCH] Revert "phy: add support for a reset-gpio specification"

[Guenter Roeck]

On Thu, May 19, 2016 at 08:11:22PM +0300, Sergei Shtylyov wrote:
> Hello.
> 
> On 05/19/2016 04:29 PM, Guenter Roeck wrote:
> 
> >>>Commit da47b4572056 ("phy: add support for a reset-gpio specification")
> >>>causes the following xtensa qemu crash according to Guenter Roeck:
> >>>
> >>>[    9.366256] libphy: ethoc-mdio: probed
> >>>[    9.367389]  (null): could not attach to PHY
> >>>[    9.368555]  (null): failed to probe MDIO bus
> >>>[    9.371540] Unable to handle kernel paging request at virtual address
> >>>0000001c
> >>>[    9.371540]  pc = d0320926, ra = 903209d1
> >>>[    9.375358] Oops: sig: 11 [#1]
> >>
> >>    Of course, I'd like this patch to be reverted (as it was applied
> >>erroneously instead of my patches)... but where's the backtrace? It's not
> >>immediately clear how this code can cause kernel oops...
> >>
> >See http://www.spinics.net/lists/kernel/msg2258611.html.
> 
>    OK, I'll try to comment on your analysis posted there:
> 
> >GPIOLIB is not configured in my test case, meaning gpiod_get_optional()
> >returns -ENOSYS, and phy_probe() thus returns an error. Question here is if
> >it is really appropriate for the XXX_optional() gpiolib functions to return
> >an error if GPIOLIB is not configured. Either case, result is that pretty
> >much all phy registrations will now fail if GPIOLIB is not configured.
> 
>    Yes, that's the primary issue. That's what needs to be fixed...
> 
As I pointed out, Uwe (who had submitted the offending patch) had objected
to a patch trying to do that last year. I think the problem is quite deep
in the gpio infrastructure: If GPIOLIB is not configured, the infrastructure
needs to do more than just return -ENOSYS. It should check if the property
exists, and only return -ENOSYS if that is the case, but NULL otherwise.

Unfortunately, that would not be easy to implement. It would have to evaluate
devicetree data, ACPI data, and platform data. Today that code is spread around
all over the gpio subsystem, so getting the semantics straight would be a
challenge, and it would be vulnerable to code changes in gpiolib. It would also
be too complex (or at least I think so) to be implemented as as static inline
functions, so some kind of basic gpiolib core code would be required.

Overall, state of the art is that XXX_optional() gpiolib functions simply
can not be used if the calling code must also work if GPIOLIB is not
configured.

Guenter 

Re: [PATCH] Revert "phy: add support for a reset-gpio specification"

[David Miller]

From: Fabio Estevam <fabio.estevam@nxp.com>
Date: Wed, 18 May 2016 13:05:00 -0300

> Commit da47b4572056 ("phy: add support for a reset-gpio specification")
> causes the following xtensa qemu crash according to Guenter Roeck:
 ...
> This reverts commit da47b4572056487fd7941c26f73b3e8815ff712a.
> 
> Reported-by: Guenter Roeck <linux@roeck-us.net>
> Signed-off-by: Fabio Estevam <fabio.estevam@nxp.com>

Applied, thanks.