Re: [PATCH net] Revert "ravb: Fixed to be able to unload modules"

From: Sergei Shtylyov <sergei.shtylyov@gmail.com>
To: Geert Uytterhoeven <geert+renesas@glider.be>,
	"David S . Miller" <davem@davemloft.net>,
	Jakub Kicinski <kuba@kernel.org>,
	Yuusuke Ashizuka <ashiduka@fujitsu.com>
Cc: Andrew Lunn <andrew@lunn.ch>,
	Heiner Kallweit <hkallweit1@gmail.com>,
	Russell King <linux@armlinux.org.uk>,
	netdev@vger.kernel.org, linux-renesas-soc@vger.kernel.org,
	linux-kernel@vger.kernel.org, stable@vger.kernel.org
Subject: Re: [PATCH net] Revert "ravb: Fixed to be able to unload modules"
Date: Tue, 22 Sep 2020 12:53:37 +0300	[thread overview]
Message-ID: <32351a6c-07ea-2a5c-0edd-4ef92dc38002@gmail.com> (raw)
In-Reply-To: <20200922072931.2148-1-geert+renesas@glider.be>

On 22.09.2020 10:29, Geert Uytterhoeven wrote:

> This reverts commit 1838d6c62f57836639bd3d83e7855e0ee4f6defc.
> 
> This commit moved the ravb_mdio_init() call (and thus the
> of_mdiobus_register() call) from the ravb_probe() to the ravb_open()
> call.  This causes a regression during system resume (s2idle/s2ram), as
> new PHY devices cannot be bound while suspended.
> 
> During boot, the Micrel PHY is detected like this:
> 
>      Micrel KSZ9031 Gigabit PHY e6800000.ethernet-ffffffff:00: attached PHY driver [Micrel KSZ9031 Gigabit PHY] (mii_bus:phy_addr=e6800000.ethernet-ffffffff:00, irq=228)
>      ravb e6800000.ethernet eth0: Link is Up - 1Gbps/Full - flow control off
> 
> During system suspend, (A) defer_all_probes is set to true, and (B)
> usermodehelper_disabled is set to UMH_DISABLED, to avoid drivers being
> probed while suspended.
> 
>    A. If CONFIG_MODULES=n, phy_device_register() calling device_add()
>       merely adds the device, but does not probe it yet, as
>       really_probe() returns early due to defer_all_probes being set:
> 
>         dpm_resume+0x128/0x4f8
> 	 device_resume+0xcc/0x1b0
> 	   dpm_run_callback+0x74/0x340
> 	     ravb_resume+0x190/0x1b8
> 	       ravb_open+0x84/0x770
> 		 of_mdiobus_register+0x1e0/0x468
> 		   of_mdiobus_register_phy+0x1b8/0x250
> 		     of_mdiobus_phy_device_register+0x178/0x1e8
> 		       phy_device_register+0x114/0x1b8
> 			 device_add+0x3d4/0x798
> 			   bus_probe_device+0x98/0xa0
> 			     device_initial_probe+0x10/0x18
> 			       __device_attach+0xe4/0x140
> 				 bus_for_each_drv+0x64/0xc8
> 				   __device_attach_driver+0xb8/0xe0
> 				     driver_probe_device.part.11+0xc4/0xd8
> 				       really_probe+0x32c/0x3b8
> 
>       Later, phy_attach_direct() notices no PHY driver has been bound,
>       and falls back to the Generic PHY, leading to degraded operation:
> 
>         Generic PHY e6800000.ethernet-ffffffff:00: attached PHY driver [Generic PHY] (mii_bus:phy_addr=e6800000.ethernet-ffffffff:00, irq=POLL)
>         ravb e6800000.ethernet eth0: Link is Up - 1Gbps/Full - flow control off
> 
>    B. If CONFIG_MODULES=y, request_module() returns early with -EBUSY due
>       to UMH_DISABLED, and MDIO initialization fails completely:
> 
>         mdio_bus e6800000.ethernet-ffffffff:00: error -16 loading PHY driver module for ID 0x00221622
>         ravb e6800000.ethernet eth0: failed to initialize MDIO
>         PM: dpm_run_callback(): ravb_resume+0x0/0x1b8 returns -16
>         PM: Device e6800000.ethernet failed to resume: error -16
> 
>       Ignoring -EBUSY in phy_request_driver_module(), like was done for
>       -ENOENT in commit 21e194425abd65b5 ("net: phy: fix issue with loading
>       PHY driver w/o initramfs"), would makes it fall back to the Generic
>       PHY, like in the CONFIG_MODULES=n case.
> 
> Signed-off-by: Geert Uytterhoeven <geert+renesas@glider.be>
> Cc: stable@vger.kernel.org

Reviewed-by: Sergei Shtylyov <sergei.shtylyov@gmail.com>

> ---
> Commit 1838d6c62f578366 ("ravb: Fixed to be able to unload modules") was
> already backported to stable v4.4, v4.9, v4.14, v4.19, v5.4, and v5.8),
> and thus needs to be reverted there, too.

    Ugh!
    Sorry for not noticing the issue with the original patch... :-/

MBR, Sergei