Re: [PATCH v3] ravb: Fixed to be able to unload modules

* Re: [PATCH v3] ravb: Fixed to be able to unload modules
       [not found] <20200820094307.3977-1-ashiduka@fujitsu.com>
@ 2020-09-16  9:31 ` Geert Uytterhoeven
  2020-09-21 13:16   ` Geert Uytterhoeven
  0 siblings, 1 reply; 2+ messages in thread
From: Geert Uytterhoeven @ 2020-09-16  9:31 UTC (permalink / raw)
  To: Yuusuke Ashizuka
  Cc: Sergei Shtylyov, David S. Miller, Jakub Kicinski, netdev,
	Linux-Renesas, Andrew Lunn, Heiner Kallweit, Russell King,
	stable

Hi Ashizuka-san,

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

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.

Thanks for your comments!

Gr{oetje,eeting}s,

                        Geert

-- 
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@linux-m68k.org

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

^ permalink raw reply	[flat|nested] 2+ messages in thread