From: Florian Fainelli <f.fainelli@gmail.com>
To: Vladimir Oltean <vladimir.oltean@nxp.com>, netdev@vger.kernel.org
Cc: Andrew Lunn <andrew@lunn.ch>,
Heiner Kallweit <hkallweit1@gmail.com>,
Russell King <linux@armlinux.org.uk>,
"David S. Miller" <davem@davemloft.net>,
Jakub Kicinski <kuba@kernel.org>,
Gerhard Engleder <gerhard@engleder-embedded.com>,
linux-kernel@vger.kernel.org
Subject: Re: [PATCH v2 net] Revert "net: phy: Uniform PHY driver access"
Date: Tue, 14 Sep 2021 11:56:27 -0700 [thread overview]
Message-ID: <28f240f8-3fd8-d9ff-3169-50ae06cf8df5@gmail.com> (raw)
In-Reply-To: <20210914140515.2311548-1-vladimir.oltean@nxp.com>
On 9/14/2021 7:05 AM, Vladimir Oltean wrote:
> This reverts commit 3ac8eed62596387214869319379c1fcba264d8c6, which did
> more than it said on the box, and not only it replaced to_phy_driver
> with phydev->drv, but it also removed the "!drv" check, without actually
> explaining why that is fine.
>
> That patch in fact breaks suspend/resume on any system which has PHY
> devices with no drivers bound.
>
> The stack trace is:
>
> Unable to handle kernel NULL pointer dereference at virtual address 00000000000000e8
> pc : mdio_bus_phy_suspend+0xd8/0xec
> lr : dpm_run_callback+0x38/0x90
> Call trace:
> mdio_bus_phy_suspend+0xd8/0xec
> dpm_run_callback+0x38/0x90
> __device_suspend+0x108/0x3cc
> dpm_suspend+0x140/0x210
> dpm_suspend_start+0x7c/0xa0
> suspend_devices_and_enter+0x13c/0x540
> pm_suspend+0x2a4/0x330
>
> Examples why that assumption is not fine:
>
> - There is an MDIO bus with a PHY device that doesn't have a specific
> PHY driver loaded, because mdiobus_register() automatically creates a
> PHY device for it but there is no specific PHY driver in the system.
> Normally under those circumstances, the generic PHY driver will be
> bound lazily to it (at phy_attach_direct time). But some Ethernet
> drivers attach to their PHY at .ndo_open time. Until then it, the
> to-be-driven-by-genphy PHY device will not have a driver. The blamed
> patch amounts to saying "you need to open all net devices before the
> system can suspend, to avoid the NULL pointer dereference".
>
> - There is any raw MDIO device which has 'plausible' values in the PHY
> ID registers 2 and 3, which is located on an MDIO bus whose driver
> does not set bus->phy_mask = ~0 (which prevents auto-scanning of PHY
> devices). An example could be a MAC's internal MDIO bus with PCS
> devices on it, for serial links such as SGMII. PHY devices will get
> created for those PCSes too, due to that MDIO bus auto-scanning, and
> although those PHY devices are not used, they do not bother anybody
> either. PCS devices are usually managed in Linux as raw MDIO devices.
> Nonetheless, they do not have a PHY driver, nor does anybody attempt
> to connect to them (because they are not a PHY), and therefore this
> patch breaks that.
>
> The goal itself of the patch is questionable, so I am going for a
> straight revert. to_phy_driver does not seem to have a need to be
> replaced by phydev->drv, in fact that might even trigger code paths
> which were not given too deep of a thought.
>
> For instance:
>
> phy_probe populates phydev->drv at the beginning, but does not clean it
> up on any error (including EPROBE_DEFER). So if the phydev driver
> requests probe deferral, phydev->drv will remain populated despite there
> being no driver bound.
>
> If a system suspend starts in between the initial probe deferral request
> and the subsequent probe retry, we will be calling the phydev->drv->suspend
> method, but _before_ any phydev->drv->probe call has succeeded.
>
> That is to say, if the phydev->drv is allocating any driver-private data
> structure in ->probe, it pretty much expects that data structure to be
> available in ->suspend. But it may not. That is a pretty insane
> environment to present to PHY drivers.
>
> In the code structure before the blamed patch, mdio_bus_phy_may_suspend
> would just say "no, don't suspend" to any PHY device which does not have
> a driver pointer _in_the_device_structure_ (not the phydev->drv). That
> would essentially ensure that ->suspend will never get called for a
> device that has not yet successfully completed probe. This is the code
> structure the patch is returning to, via the revert.
>
> Fixes: 3ac8eed62596 ("net: phy: Uniform PHY driver access")
> Signed-off-by: Vladimir Oltean <vladimir.oltean@nxp.com>
Acked-by: Florian Fainelli <f.fainelli@gmail.com>
--
Florian
next prev parent reply other threads:[~2021-09-14 18:56 UTC|newest]
Thread overview: 3+ messages / expand[flat|nested] mbox.gz Atom feed top
2021-09-14 14:05 [PATCH v2 net] Revert "net: phy: Uniform PHY driver access" Vladimir Oltean
2021-09-14 18:56 ` Florian Fainelli [this message]
2021-09-15 22:20 ` patchwork-bot+netdevbpf
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=28f240f8-3fd8-d9ff-3169-50ae06cf8df5@gmail.com \
--to=f.fainelli@gmail.com \
--cc=andrew@lunn.ch \
--cc=davem@davemloft.net \
--cc=gerhard@engleder-embedded.com \
--cc=hkallweit1@gmail.com \
--cc=kuba@kernel.org \
--cc=linux-kernel@vger.kernel.org \
--cc=linux@armlinux.org.uk \
--cc=netdev@vger.kernel.org \
--cc=vladimir.oltean@nxp.com \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).