Kernel crashes in phy_attach_direct()

On Wed, Feb 8, 2017 at 12:40 PM, Andrew Lunn <andrew@lunn.ch> wrote:

> And i guess you don't have a specific PHY driver for you PHYs. You are
> using the generic PHY driver?

Yes, I am:
Generic PHY 63fec000.ethernet-1:00: attached PHY driver [Generic PHY]
(mii_bus:phy_addr=63fec000.ethernet-1:00, irq=-1)

>
> Could you try modifying phy_attach_direct() such that the
> try_module_get() is after:
>
>        /* Assume that if there is no driver, that it doesn't
>          * exist, and we should use the genphy driver.
>          */
>         if (!d->driver) {
>         <snip>
>         }

Yes, this is what I am using right now:

        /* Assume that if there is no driver, that it doesn't
@@ -946,6 +941,11 @@ int phy_attach_direct(struct net_device *dev,
struct phy_device *phydev,
                        goto error;
        }

+       if (!try_module_get(d->driver->owner)) {
+               dev_err(&dev->dev, "failed to get the device driver module\n");
+               return -EIO;
+       }
+
        if (phydev->attached_dev) {
                dev_err(&dev->dev, "PHY already attached\n");
                err = -EBUSY;

Would you like me to submit this one?

Thanks

> Yes, this is what I am using right now:
> 
> --- a/drivers/net/phy/phy_device.c
> +++ b/drivers/net/phy/phy_device.c
> @@ -920,11 +920,6 @@ int phy_attach_direct(struct net_device *dev,
> struct phy_device *phydev,
>                 return -EIO;
>         }
> 
> -       if (!try_module_get(d->driver->owner)) {
> -               dev_err(&dev->dev, "failed to get the device driver module\n");
> -               return -EIO;
> -       }
> -
>         get_device(d);
> 
>         /* Assume that if there is no driver, that it doesn't
> @@ -946,6 +941,11 @@ int phy_attach_direct(struct net_device *dev,
> struct phy_device *phydev,
>                         goto error;
>         }
> 
> +       if (!try_module_get(d->driver->owner)) {
> +               dev_err(&dev->dev, "failed to get the device driver module\n");
> +               return -EIO;
> +       }
> +
>         if (phydev->attached_dev) {
>                 dev_err(&dev->dev, "PHY already attached\n");
>                 err = -EBUSY;
> 
> Would you like me to submit this one?

I'm just wondering about the get_device(d); Does the ordering matter
here? Lets wait for Florian before submitting a patch.

      Andrew

On 02/08/2017 06:51 AM, Andrew Lunn wrote:
>> Yes, this is what I am using right now:
>>
>> --- a/drivers/net/phy/phy_device.c
>> +++ b/drivers/net/phy/phy_device.c
>> @@ -920,11 +920,6 @@ int phy_attach_direct(struct net_device *dev,
>> struct phy_device *phydev,
>>                 return -EIO;
>>         }
>>
>> -       if (!try_module_get(d->driver->owner)) {
>> -               dev_err(&dev->dev, "failed to get the device driver module\n");
>> -               return -EIO;
>> -       }
>> -
>>         get_device(d);
>>
>>         /* Assume that if there is no driver, that it doesn't
>> @@ -946,6 +941,11 @@ int phy_attach_direct(struct net_device *dev,
>> struct phy_device *phydev,
>>                         goto error;
>>         }
>>
>> +       if (!try_module_get(d->driver->owner)) {
>> +               dev_err(&dev->dev, "failed to get the device driver module\n");
>> +               return -EIO;
>> +       }
>> +
>>         if (phydev->attached_dev) {
>>                 dev_err(&dev->dev, "PHY already attached\n");
>>                 err = -EBUSY;
>>
>> Would you like me to submit this one?
> 
> I'm just wondering about the get_device(d); Does the ordering matter
> here? Lets wait for Florian before submitting a patch.

I sent a fix for that last night:

https://patchwork.ozlabs.org/patch/725522/

Sorry about that!