| Message ID | 20240402-rzn1-gmac1-v1-2-5be2b2894d8c@bootlin.com (mailing list archive) |
|---|---|
| State | Superseded |
| Delegated to: | Netdev Maintainers |
| Headers | show |
| Series | net: stmmac: Add support for RZN1 GMAC devices | expand |
On Tue, Apr 02, 2024 at 02:37:01PM +0200, Romain Gantois wrote: > + ret = stmmac_dvr_probe(dev, plat_dat, &stmmac_res); > + if (ret) > + return ret; > + > + ndev = platform_get_drvdata(pdev); > + priv = netdev_priv(ndev); > + > + pcs_node = of_parse_phandle(np, "pcs-handle", 0); > + if (pcs_node) { > + pcs = miic_create(dev, pcs_node); > + of_node_put(pcs_node); > + if (IS_ERR(pcs)) > + return PTR_ERR(pcs); > + > + priv->hw->phylink_pcs = pcs; > + } I'm afraid that this fails at one of the most basic principles of kernel multi-threaded programming. stmmac_dvr_probe() as part of its work calls register_netdev() which publishes to userspace the network device. Everything that is required must be setup _prior_ to publication to userspace to avoid races, because as soon as the network device is published, userspace can decide to bring that interface up. If one hasn't finished the initialisation, the interface can be brought up before that initialisation is complete. I don't see anything obvious in the stmmac data structures that would allow you to hook in at an appropriate point before the register_netdev() but after the netdev has been created. The priv->hw data structure is created by stmmac_hwif_init() I see that drivers/net/ethernet/stmicro/stmmac/dwmac-socfpga.c is also guilty of this as well, and should be fixed. It's even worse because it does a truck load of stuff after stmmac_dvr_probe() which it most definitely should not be doing. I definitely get the feeling that the structure of the stmmac driver is really getting out of hand, and is making stuff harder for people, and it's not improving over time - in fact, it's getting worse. It needs a *lot* of work to bring it back to a sane model.
On Tue, Apr 02, 2024 at 02:49:48PM +0100, Russell King (Oracle) wrote: > On Tue, Apr 02, 2024 at 02:37:01PM +0200, Romain Gantois wrote: > > + ret = stmmac_dvr_probe(dev, plat_dat, &stmmac_res); > > + if (ret) > > + return ret; > > + > > + ndev = platform_get_drvdata(pdev); > > + priv = netdev_priv(ndev); > > + > > + pcs_node = of_parse_phandle(np, "pcs-handle", 0); > > + if (pcs_node) { > > + pcs = miic_create(dev, pcs_node); > > + of_node_put(pcs_node); > > + if (IS_ERR(pcs)) > > + return PTR_ERR(pcs); > > + > > + priv->hw->phylink_pcs = pcs; > > + } > > I'm afraid that this fails at one of the most basic principles of kernel > multi-threaded programming. stmmac_dvr_probe() as part of its work calls > register_netdev() which publishes to userspace the network device. > > Everything that is required must be setup _prior_ to publication to > userspace to avoid races, because as soon as the network device is > published, userspace can decide to bring that interface up. If one > hasn't finished the initialisation, the interface can be brought up > before that initialisation is complete. > > I don't see anything obvious in the stmmac data structures that would > allow you to hook in at an appropriate point before the > register_netdev() but after the netdev has been created. The > priv->hw data structure is created by stmmac_hwif_init() > > I see that drivers/net/ethernet/stmicro/stmmac/dwmac-socfpga.c is also > guilty of this as well, and should be fixed. It's even worse because it > does a truck load of stuff after stmmac_dvr_probe() which it most > definitely should not be doing. > > I definitely get the feeling that the structure of the stmmac driver > is really getting out of hand, and is making stuff harder for people, > and it's not improving over time - in fact, it's getting worse. It > needs a *lot* of work to bring it back to a sane model. I'm not going to say that the two patches threaded to this email are "sane" but at least it avoids the problem. socfpga still has issues with initialisation done after register_netdev() though.
Hello Russell, On Tue, 2 Apr 2024, Russell King (Oracle) wrote: > > I'm afraid that this fails at one of the most basic principles of kernel > > multi-threaded programming. stmmac_dvr_probe() as part of its work calls > > register_netdev() which publishes to userspace the network device. > > > > Everything that is required must be setup _prior_ to publication to > > userspace to avoid races, because as soon as the network device is > > published, userspace can decide to bring that interface up. If one > > hasn't finished the initialisation, the interface can be brought up > > before that initialisation is complete. ... > > I'm not going to say that the two patches threaded to this email are > "sane" but at least it avoids the problem. socfpga still has issues > with initialisation done after register_netdev() though. Thanks a lot for providing a fix to this issue, introducing new pcs_init/exit() hooks seems like the best solution at this time, I'll make sure to integrate those patches in the v2 for this series. Thanks,
diff --git a/MAINTAINERS b/MAINTAINERS index 6a233e1a3cf2..9735c7d2ee38 100644 --- a/MAINTAINERS +++ b/MAINTAINERS @@ -18833,6 +18833,12 @@ F: include/dt-bindings/net/pcs-rzn1-miic.h F: include/linux/pcs-rzn1-miic.h F: net/dsa/tag_rzn1_a5psw.c +RENESAS RZ/N1 DWMAC GLUE LAYER +M: Romain Gantois <romain.gantois@bootlin.com> +S: Maintained +F: Documentation/devicetree/bindings/net/renesas,rzn1-gmac.yaml +F: drivers/net/ethernet/stmicro/stmmac/dwmac-rzn1.c + RENESAS RZ/N1 RTC CONTROLLER DRIVER M: Miquel Raynal <miquel.raynal@bootlin.com> L: linux-rtc@vger.kernel.org diff --git a/drivers/net/ethernet/stmicro/stmmac/Kconfig b/drivers/net/ethernet/stmicro/stmmac/Kconfig index 4ec61f1ee71a..05cc07b8f48c 100644 --- a/drivers/net/ethernet/stmicro/stmmac/Kconfig +++ b/drivers/net/ethernet/stmicro/stmmac/Kconfig @@ -142,6 +142,18 @@ config DWMAC_ROCKCHIP This selects the Rockchip RK3288 SoC glue layer support for the stmmac device driver. +config DWMAC_RZN1 + tristate "Renesas RZ/N1 dwmac support" + default ARCH_RZN1 + depends on OF && (ARCH_RZN1 || COMPILE_TEST) + select PCS_RZN1_MIIC + help + Support for Ethernet controller on Renesas RZ/N1 SoC family. + + This selects the Renesas RZ/N1 SoC glue layer support for + the stmmac device driver. This support can make use of a custom MII + converter PCS device. + config DWMAC_SOCFPGA tristate "SOCFPGA dwmac support" default ARCH_INTEL_SOCFPGA diff --git a/drivers/net/ethernet/stmicro/stmmac/Makefile b/drivers/net/ethernet/stmicro/stmmac/Makefile index 26cad4344701..c2f0e91f6bf8 100644 --- a/drivers/net/ethernet/stmicro/stmmac/Makefile +++ b/drivers/net/ethernet/stmicro/stmmac/Makefile @@ -21,6 +21,7 @@ obj-$(CONFIG_DWMAC_MEDIATEK) += dwmac-mediatek.o obj-$(CONFIG_DWMAC_MESON) += dwmac-meson.o dwmac-meson8b.o obj-$(CONFIG_DWMAC_QCOM_ETHQOS) += dwmac-qcom-ethqos.o obj-$(CONFIG_DWMAC_ROCKCHIP) += dwmac-rk.o +obj-$(CONFIG_DWMAC_RZN1) += dwmac-rzn1.o obj-$(CONFIG_DWMAC_SOCFPGA) += dwmac-altr-socfpga.o obj-$(CONFIG_DWMAC_STARFIVE) += dwmac-starfive.o obj-$(CONFIG_DWMAC_STI) += dwmac-sti.o diff --git a/drivers/net/ethernet/stmicro/stmmac/dwmac-rzn1.c b/drivers/net/ethernet/stmicro/stmmac/dwmac-rzn1.c new file mode 100644 index 000000000000..5216d7890992 --- /dev/null +++ b/drivers/net/ethernet/stmicro/stmmac/dwmac-rzn1.c @@ -0,0 +1,87 @@ +// SPDX-License-Identifier: GPL-2.0-or-later +/* + * Copyright (C) 2024 Schneider-Electric + * + * Clément Léger <clement.leger@bootlin.com> + */ + +#include <linux/of.h> +#include <linux/pcs-rzn1-miic.h> +#include <linux/phylink.h> +#include <linux/platform_device.h> + +#include "stmmac_platform.h" +#include "stmmac.h" + +static int rzn1_dwmac_probe(struct platform_device *pdev) +{ + struct device_node *np = pdev->dev.of_node; + struct plat_stmmacenet_data *plat_dat; + struct stmmac_resources stmmac_res; + struct device *dev = &pdev->dev; + struct device_node *pcs_node; + struct stmmac_priv *priv; + struct phylink_pcs *pcs; + struct net_device *ndev; + int ret; + + ret = stmmac_get_platform_resources(pdev, &stmmac_res); + if (ret) + return ret; + + plat_dat = devm_stmmac_probe_config_dt(pdev, stmmac_res.mac); + if (IS_ERR(plat_dat)) + return PTR_ERR(plat_dat); + + plat_dat->bsp_priv = plat_dat; + + ret = stmmac_dvr_probe(dev, plat_dat, &stmmac_res); + if (ret) + return ret; + + ndev = platform_get_drvdata(pdev); + priv = netdev_priv(ndev); + + pcs_node = of_parse_phandle(np, "pcs-handle", 0); + if (pcs_node) { + pcs = miic_create(dev, pcs_node); + of_node_put(pcs_node); + if (IS_ERR(pcs)) + return PTR_ERR(pcs); + + priv->hw->phylink_pcs = pcs; + } + + return 0; +} + +static void rzn1_dwmac_remove(struct platform_device *pdev) +{ + struct net_device *ndev = platform_get_drvdata(pdev); + struct stmmac_priv *priv = netdev_priv(ndev); + + stmmac_pltfr_remove(pdev); + + if (priv->hw->phylink_pcs) + miic_destroy(priv->hw->phylink_pcs); +} + +static const struct of_device_id rzn1_dwmac_match[] = { + { .compatible = "renesas,rzn1-gmac" }, + { } +}; +MODULE_DEVICE_TABLE(of, rzn1_dwmac_match); + +static struct platform_driver rzn1_dwmac_driver = { + .probe = rzn1_dwmac_probe, + .remove_new = rzn1_dwmac_remove, + .driver = { + .name = "rzn1-dwmac", + .of_match_table = rzn1_dwmac_match, + }, +}; +module_platform_driver(rzn1_dwmac_driver); + +MODULE_AUTHOR("Clément Léger <clement.leger@bootlin.com>"); +MODULE_DESCRIPTION("Renesas RZN1 DWMAC specific glue layer"); +MODULE_LICENSE("GPL");