| lists.openwall.net | lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC | |
|
Open Source and information security mailing list archives
| ||
|
Date: Mon, 18 Apr 2016 15:17:58 -0700
From: Florian Fainelli <f.fainelli@...il.com>
To: Alexandre Belloni <alexandre.belloni@...e-electrons.com>
Cc: Andrew Lunn <andrew@...n.ch>,
"David S . Miller" <davem@...emloft.net>,
Nicolas Ferre <nicolas.ferre@...el.com>,
netdev@...r.kernel.org, linux-kernel@...r.kernel.org
Subject: Re: [PATCH] net: phy: Ensure the state machine is called when phy is
UP
On 18/04/16 15:14, Alexandre Belloni wrote:
> On 15/04/2016 at 15:23:39 -0700, Florian Fainelli wrote :
>> On 15/04/16 15:17, Alexandre Belloni wrote:
>>> On 16/04/2016 at 00:05:08 +0200, Andrew Lunn wrote :
>>>>> Trace without my patch:
>>>>> libphy: MACB_mii_bus: probed
>>>>> macb f8020000.ethernet eth0: Cadence GEM rev 0x00020120 at 0xf8020000 irq 27 (fc:c2:3d:0c:6e:05)
>>>>> Micrel KSZ8081 or KSZ8091 f8020000.etherne:01: attached PHY driver [Micrel KSZ8081 or KSZ8091] (mii_bus:phy_addr=f8020000.etherne:01, irq=171)
>>>>> Micrel KSZ8081 or KSZ8091 f8020000.etherne:01: PHY state change READY -> READY
>>>>> [...]
>>>>> Micrel KSZ8081 or KSZ8091 f8020000.etherne:01: PHY state change READY -> READY
>>>>
>>>> Are there some state changes before this? How is it getting to state
>>>> READY? It would expect it to start in DOWN, from when the phy device
>>>> was created in phy_device_create().
>>>>
>>>
>>> No other changes. I forgot to mention that this is when booting with a
>>> cable plugged in. Unplugging and replugging the cable makes the link
>>> detection work fine even without the patch.
>>
>> OK, so the last hunk of the change in d5c3d84657db ("net: phy: Avoid
>> polling PHY with PHY_IGNORE_INTERRUPTS"):
>>
>> - queue_delayed_work(system_power_efficient_wq, &phydev->state_queue,
>> - PHY_STATE_TIME * HZ);
>> + /* Only re-schedule a PHY state machine change if we are polling the
>> + * PHY, if PHY_IGNORE_INTERRUPT is set, then we will be moving
>> + * between states from phy_mac_interrupt()
>> + */
>> + if (phydev->irq == PHY_POLL)
>> + queue_delayed_work(system_power_efficient_wq,
>> &phydev->state_queue,
>> + PHY_STATE_TIME * HZ);
>>
>>
>> is presumably what broke for you, right?
>>
>> Could you also give this patch a spin and see if it works better with
>> it? The macb driver does something racy with how the MDIO and PHY are
>> probe wrt. registering the netdev, that needs fixing too.
>>
>> diff --git a/drivers/net/ethernet/cadence/macb.c
>> b/drivers/net/ethernet/cadence/macb.c
>> index eec3200ade4a..98b99149ce0b 100644
>> --- a/drivers/net/ethernet/cadence/macb.c
>> +++ b/drivers/net/ethernet/cadence/macb.c
>> @@ -3005,28 +3005,36 @@ static int macb_probe(struct platform_device *pdev)
>> if (err)
>> goto err_out_free_netdev;
>>
>> + err = macb_mii_init(bp);
>> + if (err)
>> + goto err_out_free_netdev;
>> +
>> + phydev = bp->phy_dev;
>> + phy_attached_info(phydev);
>> +
>> + netif_carrier_off(dev);
>> +
>> err = register_netdev(dev);
>> if (err) {
>> dev_err(&pdev->dev, "Cannot register net device,
>> aborting.\n");
>> goto err_out_unregister_netdev;
>> }
>>
>> - err = macb_mii_init(bp);
>> - if (err)
>> - goto err_out_unregister_netdev;
>> -
>> - netif_carrier_off(dev);
>> -
>> netdev_info(dev, "Cadence %s rev 0x%08x at 0x%08lx irq %d (%pM)\n",
>> macb_is_gem(bp) ? "GEM" : "MACB", macb_readl(bp, MID),
>> dev->base_addr, dev->irq, dev->dev_addr);
>>
>> - phydev = bp->phy_dev;
>> - phy_attached_info(phydev);
>> -
>> return 0;
>>
>> err_out_unregister_netdev:
>> + phy_disconnect(bp->phy_dev);
>> + mdiobus_unregister(bp->mii_bus);
>> + mdiobus_free(bp->mii_bus);
>> +
>> + /* Shutdown the PHY if there is a GPIO reset */
>> + if (bp->reset_gpio)
>> + gpiod_set_value(bp->reset_gpio, 0);
>> +
>> unregister_netdev(dev);
>>
>> err_out_free_netdev:
>>
>
> Well, this fails with:
>
> [ 2.780000] ------------[ cut here ]------------
> [ 2.780000] WARNING: CPU: 0 PID: 1 at lib/kobject.c:597 kobject_get+0x6c/0xa0
> [ 2.790000] kobject: '(null)' (df532280): is not initialized, yet kobject_get() is being called.
> [ 2.800000] Modules linked in:
> [ 2.800000] CPU: 0 PID: 1 Comm: swapper Not tainted 4.6.0-rc1+ #46
> [ 2.810000] Hardware name: Atmel SAMA5
> [ 2.810000] [<c010cc44>] (unwind_backtrace) from [<c010a76c>] (show_stack+0x10/0x14)
> [ 2.820000] [<c010a76c>] (show_stack) from [<c0115a70>] (__warn+0xe4/0xfc)
> [ 2.830000] [<c0115a70>] (__warn) from [<c0115ac0>] (warn_slowpath_fmt+0x38/0x48)
> [ 2.840000] [<c0115ac0>] (warn_slowpath_fmt) from [<c02d5524>] (kobject_get+0x6c/0xa0)
> [ 2.840000] [<c02d5524>] (kobject_get) from [<c0343b24>] (device_add+0xac/0x56c)
> [ 2.850000] [<c0343b24>] (device_add) from [<c03aa900>] (__mdiobus_register+0x8c/0x198)
> [ 2.860000] [<c03aa900>] (__mdiobus_register) from [<c045ed0c>] (of_mdiobus_register+0x20/0x184)
> [ 2.870000] [<c045ed0c>] (of_mdiobus_register) from [<c03b18f0>] (macb_probe+0x488/0x898)
> [ 2.880000] [<c03b18f0>] (macb_probe) from [<c0347ff0>] (platform_drv_probe+0x4c/0xb0)
> [ 2.880000] [<c0347ff0>] (platform_drv_probe) from [<c0346838>] (driver_probe_device+0x214/0x2c0)
> [ 2.890000] [<c0346838>] (driver_probe_device) from [<c034699c>] (__driver_attach+0xb8/0xbc)
> [ 2.900000] [<c034699c>] (__driver_attach) from [<c0344b8c>] (bus_for_each_dev+0x68/0x9c)
> [ 2.910000] [<c0344b8c>] (bus_for_each_dev) from [<c0345cd0>] (bus_add_driver+0x1a0/0x218)
> [ 2.920000] [<c0345cd0>] (bus_add_driver) from [<c0347084>] (driver_register+0x78/0xf8)
> [ 2.930000] [<c0347084>] (driver_register) from [<c010166c>] (do_one_initcall+0x90/0x1dc)
> [ 2.930000] [<c010166c>] (do_one_initcall) from [<c0800d78>] (kernel_init_freeable+0x134/0x1d4)
> [ 2.940000] [<c0800d78>] (kernel_init_freeable) from [<c05e7bd0>] (kernel_init+0x8/0x110)
> [ 2.950000] [<c05e7bd0>] (kernel_init) from [<c0107598>] (ret_from_fork+0x14/0x3c)
> [ 2.960000] ---[ end trace 81bf87ef8c18d052 ]---
>
>
> I'm not completely clear why but I think one of the parent is not initialized
> until register_netdev() is called.
Yes, seems like it, how about adding this:
diff --git a/drivers/net/ethernet/cadence/macb.c
b/drivers/net/ethernet/cadence/macb.c
index 98b99149ce0b..21096dfb0e83 100644
--- a/drivers/net/ethernet/cadence/macb.c
+++ b/drivers/net/ethernet/cadence/macb.c
@@ -440,7 +440,7 @@ static int macb_mii_init(struct macb *bp)
snprintf(bp->mii_bus->id, MII_BUS_ID_SIZE, "%s-%x",
bp->pdev->name, bp->pdev->id);
bp->mii_bus->priv = bp;
- bp->mii_bus->parent = &bp->dev->dev;
+ bp->mii_bus->parent = &bp->pdev->dev;
pdata = dev_get_platdata(&bp->pdev->dev);
dev_set_drvdata(&bp->dev->dev, bp->mii_bus);
--
Florian
Powered by blists - more mailing lists