netdev - Re: [PATCH v2] bcm63xx_enet: fix internal phy IRQ assignment

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

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Date:   Thu, 25 Feb 2021 20:12:23 -0800
From:   Florian Fainelli <f.fainelli@...il.com>
To:     Heiner Kallweit <hkallweit1@...il.com>,
        Daniel González Cabanelas <dgcbueu@...il.com>
Cc:     Andrew Lunn <andrew@...n.ch>,
        "David S. Miller" <davem@...emloft.net>,
        Jakub Kicinski <kuba@...nel.org>, gregkh@...uxfoundation.org,
        netdev@...r.kernel.org,
        Álvaro Fernández Rojas <noltari@...il.com>
Subject: Re: [PATCH v2] bcm63xx_enet: fix internal phy IRQ assignment



On 2/25/2021 2:56 PM, Heiner Kallweit wrote:
>>>>> It's still an ugly workaround and a proper root cause analysis should be done
>>>>> first. I can only imagine that phydev->irq is overwritten in phy_probe()
>>>>> because phy_drv_supports_irq() is false. Can you please check whether
>>>>> phydev->irq is properly set in phy_device_create(), and if yes, whether
>>>>> it's reset to PHY_POLL in phy_probe()?.
>>>>>
>>>>
>>>> Hi Heiner, I added some kernel prints:
>>>>
>>>> [    2.712519] libphy: Fixed MDIO Bus: probed
>>>> [    2.721969] =======phy_device_create===========
>>>> [    2.726841] phy_device_create: dev->irq = 17
>>>> [    2.726841]
>>>> [    2.832620] =======phy_probe===========
>>>> [    2.836846] phy_probe: phydev->irq = 17
>>>> [    2.840950] phy_probe: phy_drv_supports_irq = 0, phy_interrupt_is_valid = 1
>>>> [    2.848267] phy_probe: phydev->irq = -1
>>>> [    2.848267]
>>>> [    2.854059] =======phy_probe===========
>>>> [    2.858174] phy_probe: phydev->irq = -1
>>>> [    2.862253] phy_probe: phydev->irq = -1
>>>> [    2.862253]
>>>> [    2.868121] libphy: bcm63xx_enet MII bus: probed
>>>> [    2.873320] Broadcom BCM63XX (1) bcm63xx_enet-0:01: attached PHY
>>>> driver [Broadcom BCM63XX (1)] (mii_bus:phy_addr=bcm63xx_enet-0:01,
>>>> irq=POLL)
>>>>
>>>> Currently using kernel 5.4.99. I still have no idea what's going on.
>>>>
>>> Thanks for debugging. This confirms my assumption that the interrupt
>>> is overwritten in phy_probe(). I'm just scratching my head how
>>> phy_drv_supports_irq() can return 0. In 5.4.99 it's defined as:
>>>
>>> static bool phy_drv_supports_irq(struct phy_driver *phydrv)
>>> {
>>>         return phydrv->config_intr && phydrv->ack_interrupt;
>>> }
>>>
>>> And that's the PHY driver:
>>>
>>> static struct phy_driver bcm63xx_driver[] = {
>>> {
>>>         .phy_id         = 0x00406000,
>>>         .phy_id_mask    = 0xfffffc00,
>>>         .name           = "Broadcom BCM63XX (1)",
>>>         /* PHY_BASIC_FEATURES */
>>>         .flags          = PHY_IS_INTERNAL,
>>>         .config_init    = bcm63xx_config_init,
>>>         .ack_interrupt  = bcm_phy_ack_intr,
>>>         .config_intr    = bcm63xx_config_intr,
>>> }
>>>
>>> So both callbacks are set. Can you extend your debugging and check
>>> in phy_drv_supports_irq() which of the callbacks is missing?
>>>
>>
>> Hi, both callbacks are missing on the first check. However on the next
>> calls they're there.
>>
>> [    2.263909] libphy: Fixed MDIO Bus: probed
> 
> This is weird. The phy_device seems to show up on both MDIO buses,
> the fixed one *and* the bcm63xx_enet bus.

Yes that does not make sense to me at all, but maybe something broke at
some point for non-Device Tree systems and we are just catching it now.
The largest rework that occurred during v4.4 and v4.9 was the
introduction of mdio_device.
-- 
Florian