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

[Sergei Shtylyov]

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@xxxxxxxxx>
> Cc: stable@xxxxxxxxxxxxxxx

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

> ---
> 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