[PATCH] ravb: Fixed the problem that rmmod can not be done

[PATCH] ravb: Fixed the problem that rmmod can not be done

[Yuusuke Ashizuka]

ravb is a module driver, but I cannot rmmod it after insmod it.
ravb does 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

Fixed to execute mdio_init() at open and free_mdio() at close, thereby rmmod is
possible in the ifdown state.

Signed-off-by: Yuusuke Ashizuka <ashiduka@fujitsu.com>
---
 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 99f7aae102ce..c8a6176bc330 100644
--- a/drivers/net/ethernet/renesas/ravb_main.c
+++ b/drivers/net/ethernet/renesas/ravb_main.c
@@ -1342,6 +1342,39 @@ 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 = &pdev->dev;
+	int error;
+
+	/* Bitbang init */
+	priv->mdiobb.ops = &bb_ops;
+
+	/* MII controller setting */
+	priv->mii_bus = alloc_mdio_bitbang(&priv->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;
+}
+
 /* Network device open function for Ethernet AVB */
 static int ravb_open(struct net_device *ndev)
 {
@@ -1350,6 +1383,13 @@ static int ravb_open(struct net_device *ndev)
 	struct device *dev = &pdev->dev;
 	int error;
 
+	/* MDIO bus init */
+	error = ravb_mdio_init(priv);
+	if (error) {
+		netdev_err(ndev, "failed to initialize MDIO\n");
+		return error;
+	}
+
 	napi_enable(&priv->napi[RAVB_BE]);
 	napi_enable(&priv->napi[RAVB_NC]);
 
@@ -1427,6 +1467,7 @@ static int ravb_open(struct net_device *ndev)
 out_napi_off:
 	napi_disable(&priv->napi[RAVB_NC]);
 	napi_disable(&priv->napi[RAVB_BE]);
+	ravb_mdio_release(priv);
 	return error;
 }
 
@@ -1682,6 +1723,18 @@ static void ravb_set_rx_mode(struct net_device *ndev)
 	spin_unlock_irqrestore(&priv->lock, flags);
 }
 
+/* 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;
+}
+
 /* Device close function for Ethernet AVB */
 static int ravb_close(struct net_device *ndev)
 {
@@ -1736,6 +1789,8 @@ static int ravb_close(struct net_device *ndev)
 	ravb_ring_free(ndev, RAVB_BE);
 	ravb_ring_free(ndev, RAVB_NC);
 
+	ravb_mdio_release(priv);
+
 	return 0;
 }
 
@@ -1887,51 +1942,6 @@ static const struct net_device_ops ravb_netdev_ops = {
 	.ndo_set_features	= ravb_set_features,
 };
 
-/* MDIO bus init function */
-static int ravb_mdio_init(struct ravb_private *priv)
-{
-	struct platform_device *pdev = priv->pdev;
-	struct device *dev = &pdev->dev;
-	int error;
-
-	/* Bitbang init */
-	priv->mdiobb.ops = &bb_ops;
-
-	/* MII controller setting */
-	priv->mii_bus = alloc_mdio_bitbang(&priv->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;
-}
-
 static const struct of_device_id ravb_match_table[] = {
 	{ .compatible = "renesas,etheravb-r8a7790", .data = (void *)RCAR_GEN2 },
 	{ .compatible = "renesas,etheravb-r8a7794", .data = (void *)RCAR_GEN2 },
@@ -2174,13 +2184,6 @@ static int ravb_probe(struct platform_device *pdev)
 		eth_hw_addr_random(ndev);
 	}
 
-	/* MDIO bus init */
-	error = ravb_mdio_init(priv);
-	if (error) {
-		dev_err(&pdev->dev, "failed to initialize MDIO\n");
-		goto out_dma_free;
-	}
-
 	netif_napi_add(ndev, &priv->napi[RAVB_BE], ravb_poll, 64);
 	netif_napi_add(ndev, &priv->napi[RAVB_NC], ravb_poll, 64);
 
@@ -2202,8 +2205,6 @@ static int ravb_probe(struct platform_device *pdev)
 out_napi_del:
 	netif_napi_del(&priv->napi[RAVB_NC]);
 	netif_napi_del(&priv->napi[RAVB_BE]);
-	ravb_mdio_release(priv);
-out_dma_free:
 	dma_free_coherent(ndev->dev.parent, priv->desc_bat_size, priv->desc_bat,
 			  priv->desc_bat_dma);
 
@@ -2235,7 +2236,6 @@ static int ravb_remove(struct platform_device *pdev)
 	unregister_netdev(ndev);
 	netif_napi_del(&priv->napi[RAVB_NC]);
 	netif_napi_del(&priv->napi[RAVB_BE]);
-	ravb_mdio_release(priv);
 	pm_runtime_disable(&pdev->dev);
 	free_netdev(ndev);
 	platform_set_drvdata(pdev, NULL);
-- 
2.17.1

Re: [PATCH] ravb: Fixed the problem that rmmod can not be done

[kernel test robot]

[-- Attachment #1: Type: text/plain, Size: 7544 bytes --]

Hi Yuusuke,

Thank you for the patch! Yet something to improve:

[auto build test ERROR on ipvs/master]
[also build test ERROR on linus/master v5.8-rc7 next-20200729]
[If your patch is applied to the wrong git tree, kindly drop us a note.
And when submitting patch, we suggest to use '--base' as documented in
https://git-scm.com/docs/git-format-patch]

url:    https://github.com/0day-ci/linux/commits/Yuusuke-Ashizuka/ravb-Fixed-the-problem-that-rmmod-can-not-be-done/20200730-120910
base:   https://git.kernel.org/pub/scm/linux/kernel/git/horms/ipvs.git master
config: xtensa-allyesconfig (attached as .config)
compiler: xtensa-linux-gcc (GCC) 9.3.0
reproduce (this is a W=1 build):
        wget https://raw.githubusercontent.com/intel/lkp-tests/master/sbin/make.cross -O ~/bin/make.cross
        chmod +x ~/bin/make.cross
        # save the attached .config to linux build tree
        COMPILER_INSTALL_PATH=$HOME/0day COMPILER=gcc-9.3.0 make.cross ARCH=xtensa 

If you fix the issue, kindly add following tag as appropriate
Reported-by: kernel test robot <lkp@intel.com>

All errors (new ones prefixed by >>):

   In file included from include/linux/err.h:5,
                    from include/linux/clk.h:12,
                    from drivers/net/ethernet/renesas/ravb_main.c:12:
   include/linux/scatterlist.h: In function 'sg_set_buf':
   arch/xtensa/include/asm/page.h:193:9: warning: comparison of unsigned expression >= 0 is always true [-Wtype-limits]
     193 |  ((pfn) >= ARCH_PFN_OFFSET && ((pfn) - ARCH_PFN_OFFSET) < max_mapnr)
         |         ^~
   include/linux/compiler.h:78:42: note: in definition of macro 'unlikely'
      78 | # define unlikely(x) __builtin_expect(!!(x), 0)
         |                                          ^
   include/linux/scatterlist.h:143:2: note: in expansion of macro 'BUG_ON'
     143 |  BUG_ON(!virt_addr_valid(buf));
         |  ^~~~~~
   arch/xtensa/include/asm/page.h:201:32: note: in expansion of macro 'pfn_valid'
     201 | #define virt_addr_valid(kaddr) pfn_valid(__pa(kaddr) >> PAGE_SHIFT)
         |                                ^~~~~~~~~
   include/linux/scatterlist.h:143:10: note: in expansion of macro 'virt_addr_valid'
     143 |  BUG_ON(!virt_addr_valid(buf));
         |          ^~~~~~~~~~~~~~~
   In file included from ./arch/xtensa/include/generated/asm/bug.h:1,
                    from include/linux/bug.h:5,
                    from include/linux/thread_info.h:12,
                    from arch/xtensa/include/asm/current.h:18,
                    from include/linux/mutex.h:14,
                    from include/linux/notifier.h:14,
                    from include/linux/clk.h:14,
                    from drivers/net/ethernet/renesas/ravb_main.c:12:
   include/linux/dma-mapping.h: In function 'dma_map_resource':
   arch/xtensa/include/asm/page.h:193:9: warning: comparison of unsigned expression >= 0 is always true [-Wtype-limits]
     193 |  ((pfn) >= ARCH_PFN_OFFSET && ((pfn) - ARCH_PFN_OFFSET) < max_mapnr)
         |         ^~
   include/asm-generic/bug.h:144:27: note: in definition of macro 'WARN_ON_ONCE'
     144 |  int __ret_warn_once = !!(condition);   \
         |                           ^~~~~~~~~
   include/linux/dma-mapping.h:352:19: note: in expansion of macro 'pfn_valid'
     352 |  if (WARN_ON_ONCE(pfn_valid(PHYS_PFN(phys_addr))))
         |                   ^~~~~~~~~
   drivers/net/ethernet/renesas/ravb_main.c: In function 'ravb_open':
>> drivers/net/ethernet/renesas/ravb_main.c:1470:2: error: implicit declaration of function 'ravb_mdio_release' [-Werror=implicit-function-declaration]
    1470 |  ravb_mdio_release(priv);
         |  ^~~~~~~~~~~~~~~~~
   drivers/net/ethernet/renesas/ravb_main.c: At top level:
>> drivers/net/ethernet/renesas/ravb_main.c:1705:12: error: static declaration of 'ravb_mdio_release' follows non-static declaration
    1705 | static int ravb_mdio_release(struct ravb_private *priv)
         |            ^~~~~~~~~~~~~~~~~
   drivers/net/ethernet/renesas/ravb_main.c:1470:2: note: previous implicit declaration of 'ravb_mdio_release' was here
    1470 |  ravb_mdio_release(priv);
         |  ^~~~~~~~~~~~~~~~~
   cc1: some warnings being treated as errors

vim +/ravb_mdio_release +1470 drivers/net/ethernet/renesas/ravb_main.c

  1377	
  1378	/* Network device open function for Ethernet AVB */
  1379	static int ravb_open(struct net_device *ndev)
  1380	{
  1381		struct ravb_private *priv = netdev_priv(ndev);
  1382		struct platform_device *pdev = priv->pdev;
  1383		struct device *dev = &pdev->dev;
  1384		int error;
  1385	
  1386		/* MDIO bus init */
  1387		error = ravb_mdio_init(priv);
  1388		if (error) {
  1389			netdev_err(ndev, "failed to initialize MDIO\n");
  1390			return error;
  1391		}
  1392	
  1393		napi_enable(&priv->napi[RAVB_BE]);
  1394		napi_enable(&priv->napi[RAVB_NC]);
  1395	
  1396		if (priv->chip_id == RCAR_GEN2) {
  1397			error = request_irq(ndev->irq, ravb_interrupt, IRQF_SHARED,
  1398					    ndev->name, ndev);
  1399			if (error) {
  1400				netdev_err(ndev, "cannot request IRQ\n");
  1401				goto out_napi_off;
  1402			}
  1403		} else {
  1404			error = ravb_hook_irq(ndev->irq, ravb_multi_interrupt, ndev,
  1405					      dev, "ch22:multi");
  1406			if (error)
  1407				goto out_napi_off;
  1408			error = ravb_hook_irq(priv->emac_irq, ravb_emac_interrupt, ndev,
  1409					      dev, "ch24:emac");
  1410			if (error)
  1411				goto out_free_irq;
  1412			error = ravb_hook_irq(priv->rx_irqs[RAVB_BE], ravb_be_interrupt,
  1413					      ndev, dev, "ch0:rx_be");
  1414			if (error)
  1415				goto out_free_irq_emac;
  1416			error = ravb_hook_irq(priv->tx_irqs[RAVB_BE], ravb_be_interrupt,
  1417					      ndev, dev, "ch18:tx_be");
  1418			if (error)
  1419				goto out_free_irq_be_rx;
  1420			error = ravb_hook_irq(priv->rx_irqs[RAVB_NC], ravb_nc_interrupt,
  1421					      ndev, dev, "ch1:rx_nc");
  1422			if (error)
  1423				goto out_free_irq_be_tx;
  1424			error = ravb_hook_irq(priv->tx_irqs[RAVB_NC], ravb_nc_interrupt,
  1425					      ndev, dev, "ch19:tx_nc");
  1426			if (error)
  1427				goto out_free_irq_nc_rx;
  1428		}
  1429	
  1430		/* Device init */
  1431		error = ravb_dmac_init(ndev);
  1432		if (error)
  1433			goto out_free_irq_nc_tx;
  1434		ravb_emac_init(ndev);
  1435	
  1436		/* Initialise PTP Clock driver */
  1437		if (priv->chip_id == RCAR_GEN2)
  1438			ravb_ptp_init(ndev, priv->pdev);
  1439	
  1440		netif_tx_start_all_queues(ndev);
  1441	
  1442		/* PHY control start */
  1443		error = ravb_phy_start(ndev);
  1444		if (error)
  1445			goto out_ptp_stop;
  1446	
  1447		return 0;
  1448	
  1449	out_ptp_stop:
  1450		/* Stop PTP Clock driver */
  1451		if (priv->chip_id == RCAR_GEN2)
  1452			ravb_ptp_stop(ndev);
  1453	out_free_irq_nc_tx:
  1454		if (priv->chip_id == RCAR_GEN2)
  1455			goto out_free_irq;
  1456		free_irq(priv->tx_irqs[RAVB_NC], ndev);
  1457	out_free_irq_nc_rx:
  1458		free_irq(priv->rx_irqs[RAVB_NC], ndev);
  1459	out_free_irq_be_tx:
  1460		free_irq(priv->tx_irqs[RAVB_BE], ndev);
  1461	out_free_irq_be_rx:
  1462		free_irq(priv->rx_irqs[RAVB_BE], ndev);
  1463	out_free_irq_emac:
  1464		free_irq(priv->emac_irq, ndev);
  1465	out_free_irq:
  1466		free_irq(ndev->irq, ndev);
  1467	out_napi_off:
  1468		napi_disable(&priv->napi[RAVB_NC]);
  1469		napi_disable(&priv->napi[RAVB_BE]);
> 1470		ravb_mdio_release(priv);
  1471		return error;
  1472	}
  1473	

---
0-DAY CI Kernel Test Service, Intel Corporation
https://lists.01.org/hyperkitty/list/kbuild-all@lists.01.org

[-- Attachment #2: .config.gz --]
[-- Type: application/gzip, Size: 64407 bytes --]

[PATCH v2] ravb: Fixed the problem that rmmod can not be done

[Yuusuke Ashizuka]

ravb is a module driver, but I cannot rmmod it after insmod it.
ravb does 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

Fixed to execute mdio_init() at open and free_mdio() at close, thereby rmmod is
possible in the ifdown state.

Signed-off-by: Yuusuke Ashizuka <ashiduka@fujitsu.com>
---
Changes in v2:
 - Fix build error

 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 99f7aae102ce..df89d09b253e 100644
--- a/drivers/net/ethernet/renesas/ravb_main.c
+++ b/drivers/net/ethernet/renesas/ravb_main.c
@@ -1342,6 +1342,51 @@ 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 = &pdev->dev;
+	int error;
+
+	/* Bitbang init */
+	priv->mdiobb.ops = &bb_ops;
+
+	/* MII controller setting */
+	priv->mii_bus = alloc_mdio_bitbang(&priv->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)
 {
@@ -1350,6 +1395,13 @@ static int ravb_open(struct net_device *ndev)
 	struct device *dev = &pdev->dev;
 	int error;
 
+	/* MDIO bus init */
+	error = ravb_mdio_init(priv);
+	if (error) {
+		netdev_err(ndev, "failed to initialize MDIO\n");
+		return error;
+	}
+
 	napi_enable(&priv->napi[RAVB_BE]);
 	napi_enable(&priv->napi[RAVB_NC]);
 
@@ -1427,6 +1479,7 @@ static int ravb_open(struct net_device *ndev)
 out_napi_off:
 	napi_disable(&priv->napi[RAVB_NC]);
 	napi_disable(&priv->napi[RAVB_BE]);
+	ravb_mdio_release(priv);
 	return error;
 }
 
@@ -1736,6 +1789,8 @@ static int ravb_close(struct net_device *ndev)
 	ravb_ring_free(ndev, RAVB_BE);
 	ravb_ring_free(ndev, RAVB_NC);
 
+	ravb_mdio_release(priv);
+
 	return 0;
 }
 
@@ -1887,51 +1942,6 @@ static const struct net_device_ops ravb_netdev_ops = {
 	.ndo_set_features	= ravb_set_features,
 };
 
-/* MDIO bus init function */
-static int ravb_mdio_init(struct ravb_private *priv)
-{
-	struct platform_device *pdev = priv->pdev;
-	struct device *dev = &pdev->dev;
-	int error;
-
-	/* Bitbang init */
-	priv->mdiobb.ops = &bb_ops;
-
-	/* MII controller setting */
-	priv->mii_bus = alloc_mdio_bitbang(&priv->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;
-}
-
 static const struct of_device_id ravb_match_table[] = {
 	{ .compatible = "renesas,etheravb-r8a7790", .data = (void *)RCAR_GEN2 },
 	{ .compatible = "renesas,etheravb-r8a7794", .data = (void *)RCAR_GEN2 },
@@ -2174,13 +2184,6 @@ static int ravb_probe(struct platform_device *pdev)
 		eth_hw_addr_random(ndev);
 	}
 
-	/* MDIO bus init */
-	error = ravb_mdio_init(priv);
-	if (error) {
-		dev_err(&pdev->dev, "failed to initialize MDIO\n");
-		goto out_dma_free;
-	}
-
 	netif_napi_add(ndev, &priv->napi[RAVB_BE], ravb_poll, 64);
 	netif_napi_add(ndev, &priv->napi[RAVB_NC], ravb_poll, 64);
 
@@ -2202,8 +2205,6 @@ static int ravb_probe(struct platform_device *pdev)
 out_napi_del:
 	netif_napi_del(&priv->napi[RAVB_NC]);
 	netif_napi_del(&priv->napi[RAVB_BE]);
-	ravb_mdio_release(priv);
-out_dma_free:
 	dma_free_coherent(ndev->dev.parent, priv->desc_bat_size, priv->desc_bat,
 			  priv->desc_bat_dma);
 
@@ -2235,7 +2236,6 @@ static int ravb_remove(struct platform_device *pdev)
 	unregister_netdev(ndev);
 	netif_napi_del(&priv->napi[RAVB_NC]);
 	netif_napi_del(&priv->napi[RAVB_BE]);
-	ravb_mdio_release(priv);
 	pm_runtime_disable(&pdev->dev);
 	free_netdev(ndev);
 	platform_set_drvdata(pdev, NULL);
-- 
2.17.1

RE: [PATCH v2] ravb: Fixed the problem that rmmod can not be done

[Yoshihiro Shimoda]

Hi Ashizuka-san,

> From: Yuusuke Ashizuka, Sent: Thursday, July 30, 2020 7:02 PM
> Subject: [PATCH v2] ravb: Fixed the problem that rmmod can not be done

Thank you for the patch! I found a similar patch for another driver [1].
So, we should apply this patch to the ravb driver.

[1]
fd5f375c1628 ("net-next: ax88796: Attach MII bus only when open")

> ravb is a module driver, but I cannot rmmod it after insmod it.

I think "When this driver is a module, I cannot ..." is better.

> ravb does mdio_init() at the time of probe, and module->refcnt is incremented

I think "This is because that this driver calls ravb_mdio_init() ..." is better.

According to scripts/checkpatch.pl, I think it's better to be a maximum
75 chars per line in the commit description.

> 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
> 
> Fixed to execute mdio_init() at open and free_mdio() at close, thereby rmmod is

I think "Fixed to call ravb_mdio_init() at open and ravb_mdio_release() ..." is better.
However, I'm not sure whether that Sergei who is the reviwer of this driver accepts
the descriptions which I suggested though :)

By the way, I think you have to send this patch to the following maintainers too:
# We can get it by using scripts/get_maintainers.pl.
David S. Miller <davem@davemloft.net> (maintainer:NETWORKING DRIVERS,commit_signer:8/8=100%)
Jakub Kicinski <kuba@kernel.org> (maintainer:NETWORKING DRIVERS)

Best regards,
Yoshihiro Shimoda

> possible in the ifdown state.
> 
> Signed-off-by: Yuusuke Ashizuka <ashiduka@fujitsu.com>
> ---
> Changes in v2:
>  - Fix build error
> 
>  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 99f7aae102ce..df89d09b253e 100644
> --- a/drivers/net/ethernet/renesas/ravb_main.c
> +++ b/drivers/net/ethernet/renesas/ravb_main.c
> @@ -1342,6 +1342,51 @@ 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 = &pdev->dev;
> +	int error;
> +
> +	/* Bitbang init */
> +	priv->mdiobb.ops = &bb_ops;
> +
> +	/* MII controller setting */
> +	priv->mii_bus = alloc_mdio_bitbang(&priv->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)
>  {
> @@ -1350,6 +1395,13 @@ static int ravb_open(struct net_device *ndev)
>  	struct device *dev = &pdev->dev;
>  	int error;
> 
> +	/* MDIO bus init */
> +	error = ravb_mdio_init(priv);
> +	if (error) {
> +		netdev_err(ndev, "failed to initialize MDIO\n");
> +		return error;
> +	}
> +
>  	napi_enable(&priv->napi[RAVB_BE]);
>  	napi_enable(&priv->napi[RAVB_NC]);
> 
> @@ -1427,6 +1479,7 @@ static int ravb_open(struct net_device *ndev)
>  out_napi_off:
>  	napi_disable(&priv->napi[RAVB_NC]);
>  	napi_disable(&priv->napi[RAVB_BE]);
> +	ravb_mdio_release(priv);
>  	return error;
>  }
> 
> @@ -1736,6 +1789,8 @@ static int ravb_close(struct net_device *ndev)
>  	ravb_ring_free(ndev, RAVB_BE);
>  	ravb_ring_free(ndev, RAVB_NC);
> 
> +	ravb_mdio_release(priv);
> +
>  	return 0;
>  }
> 
> @@ -1887,51 +1942,6 @@ static const struct net_device_ops ravb_netdev_ops = {
>  	.ndo_set_features	= ravb_set_features,
>  };
> 
> -/* MDIO bus init function */
> -static int ravb_mdio_init(struct ravb_private *priv)
> -{
> -	struct platform_device *pdev = priv->pdev;
> -	struct device *dev = &pdev->dev;
> -	int error;
> -
> -	/* Bitbang init */
> -	priv->mdiobb.ops = &bb_ops;
> -
> -	/* MII controller setting */
> -	priv->mii_bus = alloc_mdio_bitbang(&priv->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;
> -}
> -
>  static const struct of_device_id ravb_match_table[] = {
>  	{ .compatible = "renesas,etheravb-r8a7790", .data = (void *)RCAR_GEN2 },
>  	{ .compatible = "renesas,etheravb-r8a7794", .data = (void *)RCAR_GEN2 },
> @@ -2174,13 +2184,6 @@ static int ravb_probe(struct platform_device *pdev)
>  		eth_hw_addr_random(ndev);
>  	}
> 
> -	/* MDIO bus init */
> -	error = ravb_mdio_init(priv);
> -	if (error) {
> -		dev_err(&pdev->dev, "failed to initialize MDIO\n");
> -		goto out_dma_free;
> -	}
> -
>  	netif_napi_add(ndev, &priv->napi[RAVB_BE], ravb_poll, 64);
>  	netif_napi_add(ndev, &priv->napi[RAVB_NC], ravb_poll, 64);
> 
> @@ -2202,8 +2205,6 @@ static int ravb_probe(struct platform_device *pdev)
>  out_napi_del:
>  	netif_napi_del(&priv->napi[RAVB_NC]);
>  	netif_napi_del(&priv->napi[RAVB_BE]);
> -	ravb_mdio_release(priv);
> -out_dma_free:
>  	dma_free_coherent(ndev->dev.parent, priv->desc_bat_size, priv->desc_bat,
>  			  priv->desc_bat_dma);
> 
> @@ -2235,7 +2236,6 @@ static int ravb_remove(struct platform_device *pdev)
>  	unregister_netdev(ndev);
>  	netif_napi_del(&priv->napi[RAVB_NC]);
>  	netif_napi_del(&priv->napi[RAVB_BE]);
> -	ravb_mdio_release(priv);
>  	pm_runtime_disable(&pdev->dev);
>  	free_netdev(ndev);
>  	platform_set_drvdata(pdev, NULL);
> --
> 2.17.1

Re: [PATCH v2] ravb: Fixed the problem that rmmod can not be done

[Sergei Shtylyov]

Hello!

On 7/30/20 1:01 PM, Yuusuke Ashizuka wrote:

> ravb is a module driver, but I cannot rmmod it after insmod it.

   Modular. And "insmod'ing it".

> ravb does mdio_init() at the time of probe, and module->refcnt is incremented
> by alloc_mdio_bitbang() called after that.

   That seems a common pattern, inlluding the Renesas sh_eth driver...

> Therefore, even if ifup is not performed, the driver is in use and rmmod cannot
> be performed.

   No, the driver's remove() method calls ravb_mdio_release() and that one calls
free_mdio_bitbang() that calls module_put(); the actual reason lies somewehre deeper
than this... Unfortunately I don't have the affected hardware anymore... :-(

[...]

MBR, Sergei

Re: [PATCH v2] ravb: Fixed the problem that rmmod can not be done

[Sergei Shtylyov]

Hello!

On 7/30/20 2:37 PM, Yoshihiro Shimoda wrote:

>> From: Yuusuke Ashizuka, Sent: Thursday, July 30, 2020 7:02 PM
>> Subject: [PATCH v2] ravb: Fixed the problem that rmmod can not be done
> 
> Thank you for the patch! I found a similar patch for another driver [1].

   It's not the same case -- that driver hadn't had the MDIO release code at all
before that patch.

> So, we should apply this patch to the ravb driver.

   I believe the driver is innocent. :-)

> [1]
> fd5f375c1628 ("net-next: ax88796: Attach MII bus only when open")
> 
>> ravb is a module driver, but I cannot rmmod it after insmod it.
> 
> I think "When this driver is a module, I cannot ..." is better.

   Perhaps "... is built as a module".

>> ravb does mdio_init() at the time of probe, and module->refcnt is incremented
> 
> I think "This is because that this driver calls ravb_mdio_init() ..." is better.

   Yep.

> According to scripts/checkpatch.pl, I think it's better to be a maximum
> 75 chars per line in the commit description.

   Yes.
   (Note that for the source code the new length limit is 100, not 80.)

>> by alloc_mdio_bitbang() called after that.
>> Therefore, even if ifup is not performed, the driver is in use and rmmod cannot
>> be performed.

   That's not really obvious...

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

   Shouldn't the driver core call the remove() method for the affected devices
first, before checking the refcount? 

>> Fixed to execute mdio_init() at open and free_mdio() at close, thereby rmmod is
> 
> I think "Fixed to call ravb_mdio_init() at open and ravb_mdio_release() ..." is better.
> However, I'm not sure whether that Sergei who is the reviwer of this driver accepts
> the descriptions which I suggested though :)

   The language barrier isn't the only obstacle. :-)

> By the way, I think you have to send this patch to the following maintainers too:
> # We can get it by using scripts/get_maintainers.pl.
> David S. Miller <davem@davemloft.net> (maintainer:NETWORKING DRIVERS,commit_signer:8/8=100%)
> Jakub Kicinski <kuba@kernel.org> (maintainer:NETWORKING DRIVERS)
> 
> Best regards,
> Yoshihiro Shimoda

   For the future, please trim your reply before the patch starts as you
don't comment on the patch itself anyway...

>> possible in the ifdown state.
>>
>> Signed-off-by: Yuusuke Ashizuka <ashiduka@fujitsu.com>

[...]

MBR, Sergei

RE: [PATCH v2] ravb: Fixed the problem that rmmod can not be done

[Yoshihiro Shimoda]

Hello!

> From: Sergei Shtylyov, Sent: Friday, July 31, 2020 1:24 AM
> 
> Hello!
> 
> On 7/30/20 2:37 PM, Yoshihiro Shimoda wrote:
> 
> >> From: Yuusuke Ashizuka, Sent: Thursday, July 30, 2020 7:02 PM
> >> Subject: [PATCH v2] ravb: Fixed the problem that rmmod can not be done
> >
> > Thank you for the patch! I found a similar patch for another driver [1].
> 
>    It's not the same case -- that driver hadn't had the MDIO release code at all
> before that patch.

You're correct. I didn't realized it...

> > So, we should apply this patch to the ravb driver.
> 
>    I believe the driver is innocent. :-)

I hope so :)

<snip>
> >> $ lsmod
> >> Module                  Size  Used by
> >> ravb                   40960  1
> >> $ rmmod ravb
> >> rmmod: ERROR: Module ravb is in use
> 
>    Shouldn't the driver core call the remove() method for the affected devices
> first, before checking the refcount?

In this case, an mii bus of "mdiobb_ops bb_ops" is affected "device" by the ravb driver.
And the ravb driver sets the owner of mii bus as THIS_MODULE like below:

static struct mdiobb_ops bb_ops = {
        .owner = THIS_MODULE,
        .set_mdc = ravb_set_mdc,
        .set_mdio_dir = ravb_set_mdio_dir,
        .set_mdio_data = ravb_set_mdio_data,
        .get_mdio_data = ravb_get_mdio_data,
};

So, I don't think the driver core can call the remove() method for the mii bus
because it's a part of the ravb driver...

By the way, about the mdio-gpio driver, I'm wondering if the mdio-gpio
driver cannot be removed by rmmod too. (perhaps, we need "rmmod -f" to remove it.)

> >> Fixed to execute mdio_init() at open and free_mdio() at close, thereby rmmod is
> >
> > I think "Fixed to call ravb_mdio_init() at open and ravb_mdio_release() ..." is better.
> > However, I'm not sure whether that Sergei who is the reviwer of this driver accepts
> > the descriptions which I suggested though :)
> 
>    The language barrier isn't the only obstacle. :-)

I agree with you :)

> > By the way, I think you have to send this patch to the following maintainers too:
> > # We can get it by using scripts/get_maintainers.pl.
> > David S. Miller <davem@davemloft.net> (maintainer:NETWORKING DRIVERS,commit_signer:8/8=100%)
> > Jakub Kicinski <kuba@kernel.org> (maintainer:NETWORKING DRIVERS)
> >
> > Best regards,
> > Yoshihiro Shimoda
> 
>    For the future, please trim your reply before the patch starts as you
> don't comment on the patch itself anyway...

Oops, I'm sorry. I'll do that for the future.

Best regards,
Yoshihiro Shimoda

RE: [PATCH v2] ravb: Fixed the problem that rmmod can not be done

[ashiduka]

Hi Sergei,

I understand that the commit log needs to be corrected.
(Shimoda-san's point is also correct)

If there is anything else that needs to be corrected, please point it out.

>    That seems a common pattern, inlluding the Renesas sh_eth
> driver...

Yes.
If I can get an R-Car Gen2 board, I will also fix sh_eth driver.

>    No, the driver's remove() method calls ravb_mdio_release() and
> that one calls
> free_mdio_bitbang() that calls module_put(); the actual reason lies
> somewehre deeper than this...

No.
Running rmmod calls delete_module() in kernel/module.c before ravb_mdio_release() is called.
delete_module()
   -> try_stop_module()
     -> try_release_module_ref()
In try_release_module_ref(), check refcnt and if it is counted up, ravb_mdio_release() is not
called and rmmod is terminated.

Thanks & Best Regards,
Yuusuke Ashizuka <ashiduka@fujitsu.com>

> -----Original Message-----
> From: Sergei Shtylyov <sergei.shtylyov@gmail.com>
> Sent: Friday, July 31, 2020 1:04 AM
> To: Ashizuka, Yuusuke/芦塚 雄介 <ashiduka@fujitsu.com>
> Cc: netdev@vger.kernel.org; linux-renesas-soc@vger.kernel.org
> Subject: Re: [PATCH v2] ravb: Fixed the problem that rmmod can not
> be done
> 
> Hello!
> 
> On 7/30/20 1:01 PM, Yuusuke Ashizuka wrote:
> 
> > ravb is a module driver, but I cannot rmmod it after insmod it.
> 
>    Modular. And "insmod'ing it".
> 
> > ravb does mdio_init() at the time of probe, and module->refcnt
> is incremented
> > by alloc_mdio_bitbang() called after that.
> 
>    That seems a common pattern, inlluding the Renesas sh_eth
> driver...
> 
> > Therefore, even if ifup is not performed, the driver is in use
> and rmmod cannot
> > be performed.
> 
>    No, the driver's remove() method calls ravb_mdio_release() and
> that one calls
> free_mdio_bitbang() that calls module_put(); the actual reason lies
> somewehre deeper
> than this... Unfortunately I don't have the affected hardware
> anymore... :-(
> 
> [...]
> 
> MBR, Sergei

Re: [PATCH v2] ravb: Fixed the problem that rmmod can not be done

[Sergei Shtylyov]

Hello!

On 7/31/20 1:18 PM, ashiduka@fujitsu.com wrote:

> I understand that the commit log needs to be corrected.

   The subject also could be more concise...

> (Shimoda-san's point is also correct)
> 
> If there is anything else that needs to be corrected, please point it out.

   OK, I'll try to post a proper patch review...

>>    That seems a common pattern, inlluding the Renesas sh_eth
>> driver...
> 
> Yes.

    Not at all so common as I thought! Only 4 drivers use mdio-bitbang, 2 of them are
for the Renesas SoCs...

> If I can get an R-Car Gen2 board, I will also fix sh_eth driver.

    Do yuo have R-Car V3H at hand, by chance? It does have a GEther controler used for
booting up... 

>>    No, the driver's remove() method calls ravb_mdio_release() and
>> that one calls
>> free_mdio_bitbang() that calls module_put(); the actual reason lies
>> somewehre deeper than this...
> 
> No.
> Running rmmod calls delete_module() in kernel/module.c before ravb_mdio_release() is called.
> delete_module()
>    -> try_stop_module()
>      -> try_release_module_ref()
> In try_release_module_ref(), check refcnt and if it is counted up, ravb_mdio_release() is not
> called and rmmod is terminated.

   Yes, after some rummaging in the module support code, I have to agree here. I was
just surprised with you finding such a critical bug so late in the drivers' life cycle.
Well, due to usually using NFS the EtherAVB (and Ether too) driver is probably alwaysbuilt in-kernel...

> Thanks & Best Regards,
> Yuusuke Ashizuka <ashiduka@fujitsu.com>

   Trim your messages after your goodbye. That original message stuff typically isn't
tolerated in the Linux mailing lists, nearly the same as top-posting...

[...]

MBR, Sergei

Re: [PATCH v2] ravb: Fixed the problem that rmmod can not be done

[Sergei Shtylyov]

Hello!


On 7/31/20 9:43 AM, Yoshihiro Shimoda wrote:

>>>> From: Yuusuke Ashizuka, Sent: Thursday, July 30, 2020 7:02 PM
>>>> Subject: [PATCH v2] ravb: Fixed the problem that rmmod can not be done
>>>
>>> Thank you for the patch! I found a similar patch for another driver [1].
>>
>>    It's not the same case -- that driver hadn't had the MDIO release code at all
>> before that patch.
> 
> You're correct. I didn't realized it...

   The patch description was somewhat incomplete there...

>>> So, we should apply this patch to the ravb driver.
>>
>>    I believe the driver is innocent. :-)
> 
> I hope so :)

   Looks like I was wrong in this case. It's very fortunate that the MDIO bitbang
is not as popular as I thought.

> <snip>
>>>> $ lsmod
>>>> Module                  Size  Used by
>>>> ravb                   40960  1
>>>> $ rmmod ravb
>>>> rmmod: ERROR: Module ravb is in use
>>
>>    Shouldn't the driver core call the remove() method for the affected devices
>> first, before checking the refcount?
> 
> In this case, an mii bus of "mdiobb_ops bb_ops" is affected "device" by the ravb driver.
> And the ravb driver sets the owner of mii bus as THIS_MODULE like below:
> 
> static struct mdiobb_ops bb_ops = {
>         .owner = THIS_MODULE,
>         .set_mdc = ravb_set_mdc,
>         .set_mdio_dir = ravb_set_mdio_dir,
>         .set_mdio_data = ravb_set_mdio_data,
>         .get_mdio_data = ravb_get_mdio_data,
> };
> 
> So, I don't think the driver core can call the remove() method for the mii bus
> because it's a part of the ravb driver...

   And because the MDIO module just doesn't have the usual method! :-)
(I meant the EtherAVB driver's remove() method, and that one would be called after
a successful reference count check...)

> By the way, about the mdio-gpio driver, I'm wondering if the mdio-gpio
> driver cannot be removed by rmmod too. (perhaps, we need "rmmod -f" to remove it.)

   You're on your own here. It's fortunate for this patch that I'm not currently loaded
at work! :-)

>>> By the way, I think you have to send this patch to the following maintainers too:
>>> # We can get it by using scripts/get_maintainers.pl.
>>> David S. Miller <davem@davemloft.net> (maintainer:NETWORKING DRIVERS,commit_signer:8/8=100%)
>>> Jakub Kicinski <kuba@kernel.org> (maintainer:NETWORKING DRIVERS)

   Not critical, as DaveM uses the patchwork anyway. He started to be CC'ed on netdev patches
only recently. :-)

[...]

> Best regards,
> Yoshihiro Shimoda

MBR, Sergei

Re: [PATCH v2] ravb: Fixed the problem that rmmod can not be done

[Sergei Shtylyov]

On 7/30/20 1:01 PM, Yuusuke Ashizuka wrote:

   CCing DaveM (as you should have done from the start)...

> ravb is a module driver, but I cannot rmmod it after insmod it.
> ravb does 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

   Did you also build mdio-bitbang.c as a module? For the in-kernal driver, not
being able to rmmod the 'ravb' one sounds logical. :-)

> ravb                   40960  1
> $ rmmod ravb
> rmmod: ERROR: Module ravb is in use
> 
> Fixed to execute mdio_init() at open and free_mdio() at close, thereby rmmod is

    Call ravb_mdio_init() at open and free_mdio_bitbang() at close.

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

[...]
> diff --git a/drivers/net/ethernet/renesas/ravb_main.c b/drivers/net/ethernet/renesas/ravb_main.c
> index 99f7aae102ce..df89d09b253e 100644
> --- a/drivers/net/ethernet/renesas/ravb_main.c
> +++ b/drivers/net/ethernet/renesas/ravb_main.c
> @@ -1342,6 +1342,51 @@ 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 = &pdev->dev;
> +	int error;
> +
> +	/* Bitbang init */
> +	priv->mdiobb.ops = &bb_ops;
> +
> +	/* MII controller setting */
> +	priv->mii_bus = alloc_mdio_bitbang(&priv->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;
> +}
> +
[...]
> @@ -1887,51 +1942,6 @@ static const struct net_device_ops ravb_netdev_ops = {
>  	.ndo_set_features	= ravb_set_features,
>  };
>  
> -/* MDIO bus init function */
> -static int ravb_mdio_init(struct ravb_private *priv)
> -{
> -	struct platform_device *pdev = priv->pdev;
> -	struct device *dev = &pdev->dev;
> -	int error;
> -
> -	/* Bitbang init */
> -	priv->mdiobb.ops = &bb_ops;
> -
> -	/* MII controller setting */
> -	priv->mii_bus = alloc_mdio_bitbang(&priv->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;
> -}
> -

   Dave, would you tolerate the forward declarations here instead (to avoid the function moves, to be later
done in the net-next tree)?

[...]

MBR, Sergei

RE: [PATCH v2] ravb: Fixed the problem that rmmod can not be done

[ashiduka]

Hi Sergei,

>    The subject also could be more concise...

I'll think about it. Thank you!

>     Not at all so common as I thought! Only 4 drivers use mdio-bitbang,
> 2 of them are for the Renesas SoCs...

Yes.

>     Do yuo have R-Car V3H at hand, by chance? It does have a GEther
> controler used for booting up...

I'm sorry. I don't have it.
There is a SILK board of R-Car Gen2 in the office where I work.
But I can't go to the office now because of the COVID-19 problem. 
If I can go to the office, I'll bring home the SILK board.

> Well, due to usually using NFS the EtherAVB (and Ether too) driver is
> probably alwaysbuilt in-kernel...

Yes. I think so, too.
Since it is necessary to reduce the Image size for embedded use, I found
this problem when changing to a module and testing.

>    Trim your messages after your goodbye. That original message stuff
> typically isn't tolerated in the Linux mailing lists, nearly the same as
> top-posting...

OK. Thanks.

Thanks & Best Regards,
Yuusuke Ashizuka <ashiduka@fujitsu.com>

RE: [PATCH v2] ravb: Fixed the problem that rmmod can not be done

[ashiduka]

Hi Sergei,

>    CCing DaveM (as you should have done from the start)...

Thank you. I appreciate your help.

>    Did you also build mdio-bitbang.c as a module?

Yes. Sure.

> For the in-kernal driver, not being able to rmmod the 'ravb' one sounds
> logical. :-)

root@rcar-gen3:~# lsmod|grep ravb
ravb                   40960  1
mdio_bitbang           16384  1 ravb
root@rcar-gen3:~# modprobe -r ravb
modprobe: FATAL: Module ravb is in use.
root@rcar-gen3:~# modprobe -r mdio_bitbang
modprobe: FATAL: Module mdio_bitbang is in use.

>> Fixed to execute mdio_init() at open and free_mdio() at close, thereby rmmod is
>    Call ravb_mdio_init() at open and free_mdio_bitbang() at close.

OK. 
Include the exact function name in the commit log, not the abbreviated function name.

>   Dave, would you tolerate the forward declarations here instead (to avoid the function moves, to be later
> done in the net-next tree)?

Wait for Dave's reply for a while.
(If Dave's reply is slow, I will only correct Sergei's issue and post it)

Thanks & Best Regards,
Yuusuke Ashizuka <ashiduka@fujitsu.com>