On Wed, Sep 16, 2020 at 11:31 AM Geert Uytterhoeven geert@linux-m68k.org wrote:

On Thu, Aug 20, 2020 at 2:55 PM Yuusuke Ashizuka ashiduka@fujitsu.com wrote:

...

When this driver is built as a module, I cannot rmmod it after insmoding it. This is because that this driver calls ravb_mdio_init() at the time of probe, and module->refcnt is incremented by alloc_mdio_bitbang() called after that. Therefore, even if ifup is not performed, the driver is in use and rmmod cannot be performed.

$ lsmod Module Size Used by ravb 40960 1 $ rmmod ravb rmmod: ERROR: Module ravb is in use

Call ravb_mdio_init() at open and free_mdio_bitbang() at close, thereby rmmod is possible in the ifdown state.

Fixes: c156633f1353 ("Renesas Ethernet AVB driver proper") Signed-off-by: Yuusuke Ashizuka ashiduka@fujitsu.com Reviewed-by: Sergei Shtylyov sergei.shtylyov@gmail.com

Thanks for your patch, which is now commit 1838d6c62f578366 ("ravb: Fixed to be able to unload modules") in v5.9-rc4 (backported to stable v4.4, v4.9, v4.14, v4.19, v5.4, and v5.8).

This is causing a regression during resume from s2idle/s2ram on (at least) Salvator-X(S) and Ebisu. Reverting that commit fixes this.

During boot, the Micrel PHY is detected correctly:

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

phy_device_register() calls device_add(), which is immediately bound to the micrel driver.

During resume, if CONFIG_MODULES=n, it falls back to the Generic PHY (case A):

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

and Ethernet still works (degraded, due to polling).

During resume, if CONFIG_MODULES=y, MDIO initialization fails (case B):

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

and Ethernet no longer works.

Case B happens because usermodehelper_disabled is set to UMH_DISABLED during system suspend, causing request_module() to return -EBUSY. 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"), makes it fall back to the Generic PHY, cfr. case A.

For case A, I haven't found out yet why it falls back to the Generic PHY.

During system suspend, defer_all_probes is set to true, to avoid drivers being probed while suspended. Hence phy_device_register() calling device_add() merely adds the device, but does not probe it yet (really_probe() returns early)"

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

Hence registering PHY devices from a net_device's ndo_open() call back must not be done.

I will send a formal revert later today.

Gr{oetje,eeting}s,

Geert