diff mbox series

[net-next,2/3] net: stmmac: add support for RZ/N1 GMAC

Message ID 20240402-rzn1-gmac1-v1-2-5be2b2894d8c@bootlin.com (mailing list archive)
State New, archived
Headers show
Series net: stmmac: Add support for RZN1 GMAC devices | expand

Commit Message

Romain Gantois April 2, 2024, 12:37 p.m. UTC
From: Clément Léger <clement.leger@bootlin.com>

Add support for the Renesas RZ/N1 GMAC. This support can make use of a
custom RZ/N1 PCS which is fetched by parsing the pcs-handle device tree
property.

Signed-off-by: "Clément Léger" <clement.leger@bootlin.com>
Co-developed-by: Romain Gantois <romain.gantois@bootlin.com>
Signed-off-by: Romain Gantois <romain.gantois@bootlin.com>
---
 MAINTAINERS                                      |  6 ++
 drivers/net/ethernet/stmicro/stmmac/Kconfig      | 12 ++++
 drivers/net/ethernet/stmicro/stmmac/Makefile     |  1 +
 drivers/net/ethernet/stmicro/stmmac/dwmac-rzn1.c | 87 ++++++++++++++++++++++++
 4 files changed, 106 insertions(+)

Comments

Russell King (Oracle) April 2, 2024, 1:49 p.m. UTC | #1
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.
Russell King (Oracle) April 2, 2024, 3:48 p.m. UTC | #2
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.
Romain Gantois April 2, 2024, 4:04 p.m. UTC | #3
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 mbox series

Patch

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");