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

2020-09-24 Thread Geert Uytterhoeven 
Hi David,

On Thu, Sep 24, 2020 at 2:40 AM David Miller  wrote:
> From: Geert Uytterhoeven 
> Date: Tue, 22 Sep 2020 09:29:31 +0200
>
> > 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.
>  ...
> >
> > Signed-off-by: Geert Uytterhoeven 
> > Cc: sta...@vger.kernel.org
>
> I noticed this too late, but please don't CC: stable on networking
> patches.  We have our own workflow as per the netdev FAQ.

OK, will try to remember.
I wanted to give a heads-up to stable that they've backported early a
patch which turned out to have issues.

> I've applied this but the inability to remove a module is an
> extremely serious bug and should be fixed properly.

Sure. As you stated in
https://lore.kernel.org/linux-renesas-soc/20200820.165244.540878641387937530.da...@davemloft.net/
that will need some rework in the MDIO subsystem...

Thanks!

Gr{oetje,eeting}s,

Geert

-- 
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- ge...@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

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

2020-09-23 Thread David Miller 
From: Geert Uytterhoeven 
Date: Tue, 22 Sep 2020 09:29:31 +0200

> 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.
 ...
> 
> Signed-off-by: Geert Uytterhoeven 
> Cc: sta...@vger.kernel.org

I noticed this too late, but please don't CC: stable on networking
patches.  We have our own workflow as per the netdev FAQ.

I've applied this but the inability to remove a module is an
extremely serious bug and should be fixed properly.

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

2020-09-22 Thread 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 e680.ethernet-:00: attached PHY 
driver [Micrel KSZ9031 Gigabit PHY] 
(mii_bus:phy_addr=e680.ethernet-:00, irq=228)
 ravb e680.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 e680.ethernet-:00: attached PHY driver [Generic 
PHY] (mii_bus:phy_addr=e680.ethernet-:00, irq=POLL)
ravb e680.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 e680.ethernet-:00: error -16 loading PHY driver 
module for ID 0x00221622
ravb e680.ethernet eth0: failed to initialize MDIO
PM: dpm_run_callback(): ravb_resume+0x0/0x1b8 returns -16
PM: Device e680.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 
Cc: sta...@vger.kernel.org


Reviewed-by: Sergei Shtylyov 


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

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

2020-09-22 Thread Geert Uytterhoeven 
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 e680.ethernet-:00: attached PHY 
driver [Micrel KSZ9031 Gigabit PHY] 
(mii_bus:phy_addr=e680.ethernet-:00, irq=228)
ravb e680.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 e680.ethernet-:00: attached PHY driver [Generic 
PHY] (mii_bus:phy_addr=e680.ethernet-:00, irq=POLL)
   ravb e680.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 e680.ethernet-:00: error -16 loading PHY driver 
module for ID 0x00221622
   ravb e680.ethernet eth0: failed to initialize MDIO
   PM: dpm_run_callback(): ravb_resume+0x0/0x1b8 returns -16
   PM: Device e680.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 
Cc: sta...@vger.kernel.org
---
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.
---
 drivers/net/ethernet/renesas/ravb_main.c | 110 +++
 1 file changed, 55 insertions(+), 55 deletions(-)

diff --git a/drivers/net/ethernet/renesas/ravb_main.c 
b/drivers/net/ethernet/renesas/ravb_main.c
index 5082c16bf9c060b2..9c4df4ede0111eae 100644
--- a/drivers/net/ethernet/renesas/ravb_main.c
+++ b/drivers/net/ethernet/renesas/ravb_main.c
@@ -1339,51 +1339,6 @@ static inline int ravb_hook_irq(unsigned int irq, 
irq_handler_t handler,
return error;
 }
 
-/* MDIO bus init function */
-static int ravb_mdio_init(struct ravb_private *priv)
-{
-   struct platform_device *pdev = priv->pdev;
-   struct device *dev = >dev;
-   int error;
-
-   /* Bitbang init */
-   priv->mdiobb.ops = _ops;
-
-   /* MII controller setting */
-   priv->mii_bus = alloc_mdio_bitbang(>mdiobb);
-   if (!priv->mii_bus)
-   return -ENOMEM;
-
-   /* Hook up MII support for ethtool */
-   priv->mii_bus->name = "ravb_mii";
-   priv->mii_bus->parent = dev;
-   snprintf(priv->mii_bus->id, MII_BUS_ID_SIZE, "%s-%x",
-pdev->name, pdev->id);
-
-   /* Register MDIO bus */
-   error = of_mdiobus_register(priv->mii_bus, dev->of_node);
-   if (error)
-   goto out_free_bus;
-
-   return 0;
-
-out_free_bus:
-   free_mdio_bitbang(priv->mii_bus);
-   return error;
-}
-
-/* MDIO bus release function */
-static int ravb_mdio_release(struct ravb_private *priv)
-{
-   /* Unregister mdio bus */
-   mdiobus_unregister(priv->mii_bus);
-
-   /* Free bitbang info */
-   free_mdio_bitbang(priv->mii_bus);
-
-   return 0;
-}
-
 /* Network device open function for Ethernet AVB */
 static int ravb_open(struct net_device *ndev)
 {
@@ -1392,13 +1347,6 @@ static int ravb_open(struct