On Wed, Sep 16, 2020 at 11:31 AM Geert Uytterhoeven
<geert@xxxxxxxxxxxxxx> wrote:
> On Thu, Aug 20, 2020 at 2:55 PM Yuusuke Ashizuka <ashiduka@xxxxxxxxxxx> 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@xxxxxxxxxxx>
> > Reviewed-by: Sergei Shtylyov <sergei.shtylyov@xxxxxxxxx>
>
> 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
--
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@xxxxxxxxxxxxxx
In personal conversations with technical people, I call myself a hacker. But
when I'm talking to journalists I just say "programmer" or something like that.
-- Linus Torvalds
