linux-kernel - Re: [PATCH] [RFC] net: phy: Fix reboot crash if CONFIG_IP

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 for Android: free password hash cracker in your pocket

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

Date:   Mon, 4 Jan 2021 13:30:36 -0800
From:   Florian Fainelli <f.fainelli@...il.com>
To:     Ioana Ciornei <ioana.ciornei@....com>, Andrew Lunn <andrew@...n.ch>
Cc:     Geert Uytterhoeven <geert@...ux-m68k.org>,
        Heiner Kallweit <hkallweit1@...il.com>,
        "David S . Miller" <davem@...emloft.net>,
        Jakub Kicinski <kuba@...nel.org>,
        Russell King <linux@...linux.org.uk>,
        Wolfram Sang <wsa+renesas@...g-engineering.com>,
        "netdev@...r.kernel.org" <netdev@...r.kernel.org>,
        "linux-renesas-soc@...r.kernel.org" 
        <linux-renesas-soc@...r.kernel.org>,
        "linux-kernel@...r.kernel.org" <linux-kernel@...r.kernel.org>
Subject: Re: [PATCH] [RFC] net: phy: Fix reboot crash if CONFIG_IP_PNP is not
 set

On 1/4/21 10:43 AM, Ioana Ciornei wrote:
> On Mon, Jan 04, 2021 at 06:31:05PM +0100, Andrew Lunn wrote:
>>> The basic rules here should be, if the MDIO bus is registered, it is
>>> usable. There are things like PHY statistics, HWMON temperature
>>> sensors, etc, DSA switches, all which have a life cycle separate to
>>> the interface being up.
>>
>> [Goes and looks at the code]
>>
>> Yes, this is runtime PM which is broken.
>>
>> sh_mdio_init() needs to wrap the mdp->mii_bus->read and
>> mdp->mii_bus->write calls with calls to
>>
>> pm_runtime_get_sync(&mdp->pdev->dev);
>>
>> and
>>
>> pm_runtime_put_sync(&mdp->pdev->dev);
>>
> 
> Agree. Thanks for actually looking into it.. I'm not really well versed
> in runtime PM.
> 
>> The KSZ8041RNLI supports statistics, which ethtool --phy-stats can
>> read, and these will also going to cause problems.
>>
> 
> Not really, this driver connects to the PHY on .ndo_open(), thus any
> try to actually dump the PHY statistics before an ifconfig up would get
> an -EOPNOTSUPP since the dev->phydev is not yet populated.
> 
> This is exactly why I do not understand why some drivers insist on
> calling of_phy_connect() and its variants on .ndo_open() and not while
> probing the device - you can access the debug stats only if the
> interface was started.

Doing the connect in ndo_open() allows you to keep the PHY in whatever
the state it was prior to the kernel managing it, which if everything is
correctly designed means in a low power state.

Your Ethernet driver's probe function may be called on boot and you may
never use the network device at all, so it is a waste of energy to power
on the PHY, have it potentially link with its link partner while you
still have no chance of doing any configuration to it because you have
not brought up the network interface.
-- 
Florian