| Message ID | AM9PR04MB8506791F9A2A1EF4B33AAAF4E2282@AM9PR04MB8506.eurprd04.prod.outlook.com (mailing list archive) |
|---|---|
| State | Deferred |
| Delegated to: | Netdev Maintainers |
| Headers | show |
| Series | [net-next,v2] net: phy: don't resume device not in use | expand |
On Fri, 2024-03-15 at 07:55 +0000, Jan Petrous (OSS) wrote: > In the case when an MDIO bus contains PHY device not attached to > any netdev or is attached to the external netdev, controlled > by another driver and the driver is disabled, the bus, when PM resume > occurs, is trying to resume also the unattached phydev. > > /* Synopsys DWMAC built-in driver (stmmac) */ > gmac0: ethernet@4033c000 { > compatible = "snps,dwc-qos-ethernet", "nxp,s32cc-dwmac"; > > phy-handle = <&gmac0_mdio_c_phy4>; > phy-mode = "rgmii-id"; > > gmac0_mdio: mdio@0 { > compatible = "snps,dwmac-mdio"; > > /* AQR107 */ > gmac0_mdio_c_phy1: ethernet-phy@1 { > compatible = "ethernet-phy-ieee802.3-c45"; > reg = <1>; > }; > > /* KSZ9031RNX */ > gmac0_mdio_c_phy4: ethernet-phy@4 { > reg = <4>; > }; > }; > }; > > /* PFE controller, loadable driver pfeng.ko */ > pfe: pfe@46000000 { > compatible = "nxp,s32g-pfe"; > > /* Network interface 'pfe1' */ > pfe_netif1: ethernet@11 { > compatible = "nxp,s32g-pfe-netif"; > > phy-mode = "sgmii"; > phy-handle = <&gmac0_mdio_c_phy1>; > }; > }; > > Because such device didn't go through attach process, internal > parameters like phy_dev->interface are set to default values, which > can be incorrect for some drivers. Ie. Aquantia PHY AQR107 doesn't > support PHY_INTERFACE_MODE_GMII and trying to use phy_init() > in mdio_bus_phy_resume ends up with the following error caused > by initial check of supported interfaces in aqr107_config_init(): > > [ 63.927708] Aquantia AQR113C stmmac-0:08: PM: failed to resume: error -19'] > > The fix is intentionally assymetric to support PM suspend of the device. > > Signed-off-by: Jan Petrous <jan.petrous@oss.nxp.com> Please note that the 'net-next' tree is closed for the merge window. You will have to repost in when the tree will re-open in a week or so. However this change could be suitable for the 'net' tree, if Andrew agrees. If, please re-sent targeting such tree and including a reasonable 'Fixes' tag. Thanks, Paolo
On Fri, Mar 15, 2024 at 07:55:48AM +0000, Jan Petrous (OSS) wrote: > In the case when an MDIO bus contains PHY device not attached to > any netdev or is attached to the external netdev, controlled > by another driver and the driver is disabled, the bus, when PM resume > occurs, is trying to resume also the unattached phydev. > > /* Synopsys DWMAC built-in driver (stmmac) */ > gmac0: ethernet@4033c000 { > compatible = "snps,dwc-qos-ethernet", "nxp,s32cc-dwmac"; > > phy-handle = <&gmac0_mdio_c_phy4>; > phy-mode = "rgmii-id"; > > gmac0_mdio: mdio@0 { > compatible = "snps,dwmac-mdio"; > > /* AQR107 */ > gmac0_mdio_c_phy1: ethernet-phy@1 { > compatible = "ethernet-phy-ieee802.3-c45"; > reg = <1>; > }; > > /* KSZ9031RNX */ > gmac0_mdio_c_phy4: ethernet-phy@4 { > reg = <4>; > }; > }; > }; > > /* PFE controller, loadable driver pfeng.ko */ > pfe: pfe@46000000 { > compatible = "nxp,s32g-pfe"; > > /* Network interface 'pfe1' */ > pfe_netif1: ethernet@11 { > compatible = "nxp,s32g-pfe-netif"; > > phy-mode = "sgmii"; > phy-handle = <&gmac0_mdio_c_phy1>; > }; > }; > > Because such device didn't go through attach process, internal > parameters like phy_dev->interface are set to default values, which > can be incorrect for some drivers. Ie. Aquantia PHY AQR107 doesn't > support PHY_INTERFACE_MODE_GMII and trying to use phy_init() > in mdio_bus_phy_resume ends up with the following error caused > by initial check of supported interfaces in aqr107_config_init(): > > [ 63.927708] Aquantia AQR113C stmmac-0:08: PM: failed to resume: error -19'] > > The fix is intentionally assymetric to support PM suspend of the device. > > Signed-off-by: Jan Petrous <jan.petrous@oss.nxp.com> Reviewed-by: Andrew Lunn <andrew@lunn.ch> Andrew
> Please note that the 'net-next' tree is closed for the merge window. > You will have to repost in when the tree will re-open in a week or so. > > However this change could be suitable for the 'net' tree, if Andrew > agrees. If, please re-sent targeting such tree and including a > reasonable 'Fixes' tag. This is the sort of change that i think it should only be in net-next. Suspend/resume is complex and not tested too well. There is a chance of regression with this change. So we should introduce it slowly. Andrew
On 3/19/2024 5:05 AM, Andrew Lunn wrote: >> Please note that the 'net-next' tree is closed for the merge window. >> You will have to repost in when the tree will re-open in a week or so. >> >> However this change could be suitable for the 'net' tree, if Andrew >> agrees. If, please re-sent targeting such tree and including a >> reasonable 'Fixes' tag. > > This is the sort of change that i think it should only be in net-next. > Suspend/resume is complex and not tested too well. There is a chance > of regression with this change. So we should introduce it slowly. Totally agree.
diff --git a/drivers/net/phy/phy_device.c b/drivers/net/phy/phy_device.c index 8297ef681bf5..507eb0570e0e 100644 --- a/drivers/net/phy/phy_device.c +++ b/drivers/net/phy/phy_device.c @@ -355,6 +355,10 @@ static __maybe_unused int mdio_bus_phy_resume(struct device *dev) struct phy_device *phydev = to_phy_device(dev); int ret; + /* Don't resume device which wasn't previously in use state */ + if (phydev->state <= PHY_READY) + return 0; + if (phydev->mac_managed_pm) return 0;
In the case when an MDIO bus contains PHY device not attached to any netdev or is attached to the external netdev, controlled by another driver and the driver is disabled, the bus, when PM resume occurs, is trying to resume also the unattached phydev. /* Synopsys DWMAC built-in driver (stmmac) */ gmac0: ethernet@4033c000 { compatible = "snps,dwc-qos-ethernet", "nxp,s32cc-dwmac"; phy-handle = <&gmac0_mdio_c_phy4>; phy-mode = "rgmii-id"; gmac0_mdio: mdio@0 { compatible = "snps,dwmac-mdio"; /* AQR107 */ gmac0_mdio_c_phy1: ethernet-phy@1 { compatible = "ethernet-phy-ieee802.3-c45"; reg = <1>; }; /* KSZ9031RNX */ gmac0_mdio_c_phy4: ethernet-phy@4 { reg = <4>; }; }; }; /* PFE controller, loadable driver pfeng.ko */ pfe: pfe@46000000 { compatible = "nxp,s32g-pfe"; /* Network interface 'pfe1' */ pfe_netif1: ethernet@11 { compatible = "nxp,s32g-pfe-netif"; phy-mode = "sgmii"; phy-handle = <&gmac0_mdio_c_phy1>; }; }; Because such device didn't go through attach process, internal parameters like phy_dev->interface are set to default values, which can be incorrect for some drivers. Ie. Aquantia PHY AQR107 doesn't support PHY_INTERFACE_MODE_GMII and trying to use phy_init() in mdio_bus_phy_resume ends up with the following error caused by initial check of supported interfaces in aqr107_config_init(): [ 63.927708] Aquantia AQR113C stmmac-0:08: PM: failed to resume: error -19'] The fix is intentionally assymetric to support PM suspend of the device. Signed-off-by: Jan Petrous <jan.petrous@oss.nxp.com> --- drivers/net/phy/phy_device.c | 4 ++++ 1 file changed, 4 insertions(+)