| Message ID | 20200730100151.7490-1-ashiduka@fujitsu.com (mailing list archive) |
|---|---|
| State | Accepted |
| Commit | 1838d6c62f57836639bd3d83e7855e0ee4f6defc |
| Delegated to: | Geert Uytterhoeven |
| Headers | show |
| Series | [v2] ravb: Fixed the problem that rmmod can not be done | expand |
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
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
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
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
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
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
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
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
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>
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>
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);
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(-)