| Message ID | ba4e359584a6b3bc4b3470822c42186d5b0856f9.1721910728.git.daniel@makrotopia.org (mailing list archive) |
|---|---|
| State | RFC |
| Delegated to: | Netdev Maintainers |
| Headers | show |
| Series | [RFC,net-next] net: pcs: add helper module for standalone drivers | expand |
On Thu, Jul 25, 2024 at 01:44:49PM +0100, Daniel Golle wrote: > Implement helper module for standalone PCS drivers which allows > standaline PCS drivers to register and users to get instances of > 'struct phylink_pcs' using device tree nodes. > > At this point only a single instance for each device tree node is > supported, once we got devices providing more than one PCS we can > extend it and introduce an xlate function as well as '#pcs-cells', > similar to how this is done by the PHY framework. > > Signed-off-by: Daniel Golle <daniel@makrotopia.org> > --- > This is meant to provide the infrastructure suggested by > Russell King in an earlier review. It just took me a long while to > find the time to implement this. > Users are going to be the standalone PCS drivers for 8/10 LynxI as > well as 64/66 USXGMII PCS found on MediaTek MT7988 SoC. > See also https://patchwork.kernel.org/comment/25636726/ > > The full tree where this is being used can be found at > > https://github.com/dangowrt/linux/commits/mt7988-for-next/ Hi Daniel, I realise this is an RFC, but I'm guessing a user will need to be submitted for this to progress into net-next. ... > +++ b/drivers/net/pcs/pcs-standalone.c ... > +static struct pcs_standalone *of_pcs_locate(const struct device_node *_np, u32 index) nit: This could trivially be line wrapped to 80 columns wide > +{ > + struct device_node *np; > + struct pcs_standalone *iter, *pcssa = NULL; nit: Reverse xmas tree ... > +struct phylink_pcs *devm_of_pcs_get(struct device *dev, > + const struct device_node *np, > + unsigned int index) > +{ > + struct pcs_standalone *pcssa; > + > + pcssa = of_pcs_locate(np ?: dev->of_node, index); > + if (IS_ERR_OR_NULL(pcssa)) > + return ERR_PTR(PTR_ERR(pcssa)); nit: Perhaps ERR_CAST() ? > + > + device_link_add(dev, pcssa->dev, DL_FLAG_AUTOREMOVE_CONSUMER); > + > + return pcssa->pcs; > +} > +EXPORT_SYMBOL_GPL(devm_of_pcs_get); > + > +MODULE_DESCRIPTION("Helper for standalone PCS drivers"); > +MODULE_AUTHOR("Daniel Golle <daniel@makrotopia.org>"); > +MODULE_LICENSE("GPL"); > diff --git a/include/linux/pcs/pcs-standalone.h b/include/linux/pcs/pcs-standalone.h Please consider adding this file to MAINTAINERS. ... > +static inline int devm_pcs_register(struct device *dev, struct phylink_pcs *pcs); > + return -EOPNOTSUPP; > +} The above does not compile. ...
On Thu, Jul 25, 2024 at 01:44:49PM +0100, Daniel Golle wrote: > Implement helper module for standalone PCS drivers which allows > standaline PCS drivers to register and users to get instances of > 'struct phylink_pcs' using device tree nodes. > > At this point only a single instance for each device tree node is > supported, once we got devices providing more than one PCS we can > extend it and introduce an xlate function as well as '#pcs-cells', > similar to how this is done by the PHY framework. > > Signed-off-by: Daniel Golle <daniel@makrotopia.org> > --- > This is meant to provide the infrastructure suggested by > Russell King in an earlier review. It just took me a long while to > find the time to implement this. > Users are going to be the standalone PCS drivers for 8/10 LynxI as > well as 64/66 USXGMII PCS found on MediaTek MT7988 SoC. > See also https://patchwork.kernel.org/comment/25636726/ > > The full tree where this is being used can be found at > > https://github.com/dangowrt/linux/commits/mt7988-for-next/ > > drivers/net/pcs/Kconfig | 4 ++ > drivers/net/pcs/Makefile | 1 + > drivers/net/pcs/pcs-standalone.c | 95 +++++++++++++++++++++++++++++ > include/linux/pcs/pcs-standalone.h | 25 ++++++++ > 4 files changed, 129 insertions(+) > create mode 100644 drivers/net/pcs/pcs-standalone.c > create mode 100644 include/linux/pcs/pcs-standalone.h > > diff --git a/drivers/net/pcs/Kconfig b/drivers/net/pcs/Kconfig > index f6aa437473de..2b02b9351fa4 100644 > --- a/drivers/net/pcs/Kconfig > +++ b/drivers/net/pcs/Kconfig > @@ -5,6 +5,10 @@ > > menu "PCS device drivers" > > +config PCS_STANDALONE > + tristate > + select PHYLINK > + > config PCS_XPCS > tristate "Synopsys DesignWare Ethernet XPCS" > select PHYLINK > diff --git a/drivers/net/pcs/Makefile b/drivers/net/pcs/Makefile > index 4f7920618b90..0cb0057f2b8e 100644 > --- a/drivers/net/pcs/Makefile > +++ b/drivers/net/pcs/Makefile > @@ -4,6 +4,7 @@ > pcs_xpcs-$(CONFIG_PCS_XPCS) := pcs-xpcs.o pcs-xpcs-plat.o \ > pcs-xpcs-nxp.o pcs-xpcs-wx.o > > +obj-$(CONFIG_PCS_STANDALONE) += pcs-standalone.o > obj-$(CONFIG_PCS_XPCS) += pcs_xpcs.o > obj-$(CONFIG_PCS_LYNX) += pcs-lynx.o > obj-$(CONFIG_PCS_MTK_LYNXI) += pcs-mtk-lynxi.o > diff --git a/drivers/net/pcs/pcs-standalone.c b/drivers/net/pcs/pcs-standalone.c > new file mode 100644 > index 000000000000..1569793328a1 > --- /dev/null > +++ b/drivers/net/pcs/pcs-standalone.c > @@ -0,0 +1,95 @@ > +// SPDX-License-Identifier: GPL-2.0-or-later > +/* > + * Helpers for standalone PCS drivers > + * > + * Copyright (C) 2024 Daniel Golle <daniel@makrotopia.org> > + */ > + > +#include <linux/pcs/pcs-standalone.h> > +#include <linux/phylink.h> > + > +static LIST_HEAD(pcs_list); > +static DEFINE_MUTEX(pcs_mutex); > + > +struct pcs_standalone { > + struct device *dev; > + struct phylink_pcs *pcs; > + struct list_head list; > +}; > + > +static void devm_pcs_provider_release(struct device *dev, void *res) > +{ > + struct pcs_standalone *pcssa = (struct pcs_standalone *)res; > + > + mutex_lock(&pcs_mutex); > + list_del(&pcssa->list); > + mutex_unlock(&pcs_mutex); This needs to do notify phylink if the PCS has gone away, but the locking for this would be somewhat difficult (because pcs->phylink could change if the PCS changes.) That would need to be solved somehow. > +} > + > +int devm_pcs_register(struct device *dev, struct phylink_pcs *pcs) > +{ > + struct pcs_standalone *pcssa; > + > + pcssa = devres_alloc(devm_pcs_provider_release, sizeof(*pcssa), > + GFP_KERNEL); > + if (!pcssa) > + return -ENOMEM; > + > + devres_add(dev, pcssa); > + pcssa->pcs = pcs; > + pcssa->dev = dev; > + > + mutex_lock(&pcs_mutex); > + list_add_tail(&pcssa->list, &pcs_list); > + mutex_unlock(&pcs_mutex); > + > + return 0; > +} > +EXPORT_SYMBOL_GPL(devm_pcs_register); > + > +static struct pcs_standalone *of_pcs_locate(const struct device_node *_np, u32 index) > +{ > + struct device_node *np; > + struct pcs_standalone *iter, *pcssa = NULL; > + > + if (!_np) > + return NULL; > + > + np = of_parse_phandle(_np, "pcs-handle", index); > + if (!np) > + return NULL; > + > + mutex_lock(&pcs_mutex); > + list_for_each_entry(iter, &pcs_list, list) { > + if (iter->dev->of_node != np) > + continue; > + > + pcssa = iter; > + break; > + } > + mutex_unlock(&pcs_mutex); > + > + of_node_put(np); > + > + return pcssa ?: ERR_PTR(-ENODEV); > +} > + > +struct phylink_pcs *devm_of_pcs_get(struct device *dev, > + const struct device_node *np, > + unsigned int index) > +{ > + struct pcs_standalone *pcssa; > + > + pcssa = of_pcs_locate(np ?: dev->of_node, index); > + if (IS_ERR_OR_NULL(pcssa)) > + return ERR_PTR(PTR_ERR(pcssa)); > + > + device_link_add(dev, pcssa->dev, DL_FLAG_AUTOREMOVE_CONSUMER); This is really not a nice solution when one has a network device that has multiple interfaces. This will cause all interfaces on that device to be purged from the system when a PCS for one of the interfaces goes away. If the system is using NFS-root, that could result in the rootfs being lost. We should handle this more gracefully.
Hi Russell, thank you for commenting on this patch. Please help me understand which direction I should work towards to see support for the MT7988 SoC Ethernet in future kernels, see my questions below. On Mon, Jul 29, 2024 at 12:28:15PM +0100, Russell King (Oracle) wrote: > On Thu, Jul 25, 2024 at 01:44:49PM +0100, Daniel Golle wrote: > > +static void devm_pcs_provider_release(struct device *dev, void *res) > > +{ > > + struct pcs_standalone *pcssa = (struct pcs_standalone *)res; > > + > > + mutex_lock(&pcs_mutex); > > + list_del(&pcssa->list); > > + mutex_unlock(&pcs_mutex); > > This needs to do notify phylink if the PCS has gone away, but the > locking for this would be somewhat difficult (because pcs->phylink > could change if the PCS changes.) That would need to be solved > somehow. From my understanding the only way the PCS would "go away" is by rmmod, which is prevented by the usage counter if still in use by the Ethernet driver (removal of instances by using unbind in sysfs is prevented by .suppress_bind_attrs). I understand that Ethernet MAC and PCS both being built-into the SoC may not be the only case we may want to support in the long run, but it is the case for the MT7988 SoC which I'd like to see supported in upstream Linux. So imho this is something quite hypothetical which can be prevented by setting .suppress_bind_attrs and bumping the module usage counter, as those are not really dedicated devices on some kind of hotplug-able bus what-so-ever, but all just components built-into the SoC itself. They won't just go away. At least in case of the SoC I'm looking at. If you have other use-cases in mind which this infrastructure should be suitable for, it'd be helpful if you would spell them out. If your criticism was meant to be directed towards the whole idea of using standlone drivers for the PCS units of the SoC then the easiest would of course be to just not do that and instead keep handling the PCS as part of the Ethernet driver. The main reason why I like the idea of the PCS driver being separate is because it is not even needed on older platforms, and those are quite resource constraint so it would be a waste to carry all the USXGMII logic, let's say, on devices with MT7621 or even MT7628. However, there are of course other ways to achieve nearly the same, such as Kconfig symbols which select parts of the driver to be included or not. Hence my question: Do you think it is worth going down this road and introducing standalone PCS drivers, given that the infrastructure requirements include graceful removal of any PCS instance? Also note that the same situation (things which may "go away") applies to PHYs (as in: drivers/phy, not drivers/net/phy) as well, and I don't see this being addressed for any of the in-SoC Ethernet controllers supported by the kernel. I was hoping for clarification regarding this but never received a reply, see https://lkml.org/lkml/2024/2/7/101 And what about used instances of drivers/pinctrl, drivers/reset, drivers/clk, ...? Should a SoC Ethernet driver be built in a way that all those may gracefully "go away" as well? I'm totally up to work on improving the overall situation there, but it'd be good to know which direction I should be aiming for. (as in: pre-removal call-back functions? just setting .suppress_bind_attrs for all drivers/phy/ and such by default? extending phylink itself to handle drivers/phy instances and their disappearance, as well as potentially more than one PCS instance per net_device? ...) > > [...] > > + device_link_add(dev, pcssa->dev, DL_FLAG_AUTOREMOVE_CONSUMER); > > This is really not a nice solution when one has a network device that > has multiple interfaces. This will cause all interfaces on that device > to be purged from the system when a PCS for one of the interfaces > goes away. If the system is using NFS-root, that could result in the > rootfs being lost. We should handle this more gracefully. "DL_FLAG_AUTOREMOVE_CONSUMER causes the device link to be automatically purged when the consumer fails to probe or later unbinds." (from Documentation/driver-api/device_link.rst) The consumer is the Ethernet driver in this case. Hence the automatic purge is only applied in case the Ethernet device goes away, and meanwhile it would prevent the PCS driver from being rmmod'ed (which in my case is the only way for the PCS to "disappear"). Also note that the same flag is used in pcs-rzn1-miic.c as well. Thank you for your patiente and helping me to understand how to proceed. Cheers Daniel
diff --git a/drivers/net/pcs/Kconfig b/drivers/net/pcs/Kconfig index f6aa437473de..2b02b9351fa4 100644 --- a/drivers/net/pcs/Kconfig +++ b/drivers/net/pcs/Kconfig @@ -5,6 +5,10 @@ menu "PCS device drivers" +config PCS_STANDALONE + tristate + select PHYLINK + config PCS_XPCS tristate "Synopsys DesignWare Ethernet XPCS" select PHYLINK diff --git a/drivers/net/pcs/Makefile b/drivers/net/pcs/Makefile index 4f7920618b90..0cb0057f2b8e 100644 --- a/drivers/net/pcs/Makefile +++ b/drivers/net/pcs/Makefile @@ -4,6 +4,7 @@ pcs_xpcs-$(CONFIG_PCS_XPCS) := pcs-xpcs.o pcs-xpcs-plat.o \ pcs-xpcs-nxp.o pcs-xpcs-wx.o +obj-$(CONFIG_PCS_STANDALONE) += pcs-standalone.o obj-$(CONFIG_PCS_XPCS) += pcs_xpcs.o obj-$(CONFIG_PCS_LYNX) += pcs-lynx.o obj-$(CONFIG_PCS_MTK_LYNXI) += pcs-mtk-lynxi.o diff --git a/drivers/net/pcs/pcs-standalone.c b/drivers/net/pcs/pcs-standalone.c new file mode 100644 index 000000000000..1569793328a1 --- /dev/null +++ b/drivers/net/pcs/pcs-standalone.c @@ -0,0 +1,95 @@ +// SPDX-License-Identifier: GPL-2.0-or-later +/* + * Helpers for standalone PCS drivers + * + * Copyright (C) 2024 Daniel Golle <daniel@makrotopia.org> + */ + +#include <linux/pcs/pcs-standalone.h> +#include <linux/phylink.h> + +static LIST_HEAD(pcs_list); +static DEFINE_MUTEX(pcs_mutex); + +struct pcs_standalone { + struct device *dev; + struct phylink_pcs *pcs; + struct list_head list; +}; + +static void devm_pcs_provider_release(struct device *dev, void *res) +{ + struct pcs_standalone *pcssa = (struct pcs_standalone *)res; + + mutex_lock(&pcs_mutex); + list_del(&pcssa->list); + mutex_unlock(&pcs_mutex); +} + +int devm_pcs_register(struct device *dev, struct phylink_pcs *pcs) +{ + struct pcs_standalone *pcssa; + + pcssa = devres_alloc(devm_pcs_provider_release, sizeof(*pcssa), + GFP_KERNEL); + if (!pcssa) + return -ENOMEM; + + devres_add(dev, pcssa); + pcssa->pcs = pcs; + pcssa->dev = dev; + + mutex_lock(&pcs_mutex); + list_add_tail(&pcssa->list, &pcs_list); + mutex_unlock(&pcs_mutex); + + return 0; +} +EXPORT_SYMBOL_GPL(devm_pcs_register); + +static struct pcs_standalone *of_pcs_locate(const struct device_node *_np, u32 index) +{ + struct device_node *np; + struct pcs_standalone *iter, *pcssa = NULL; + + if (!_np) + return NULL; + + np = of_parse_phandle(_np, "pcs-handle", index); + if (!np) + return NULL; + + mutex_lock(&pcs_mutex); + list_for_each_entry(iter, &pcs_list, list) { + if (iter->dev->of_node != np) + continue; + + pcssa = iter; + break; + } + mutex_unlock(&pcs_mutex); + + of_node_put(np); + + return pcssa ?: ERR_PTR(-ENODEV); +} + +struct phylink_pcs *devm_of_pcs_get(struct device *dev, + const struct device_node *np, + unsigned int index) +{ + struct pcs_standalone *pcssa; + + pcssa = of_pcs_locate(np ?: dev->of_node, index); + if (IS_ERR_OR_NULL(pcssa)) + return ERR_PTR(PTR_ERR(pcssa)); + + device_link_add(dev, pcssa->dev, DL_FLAG_AUTOREMOVE_CONSUMER); + + return pcssa->pcs; +} +EXPORT_SYMBOL_GPL(devm_of_pcs_get); + +MODULE_DESCRIPTION("Helper for standalone PCS drivers"); +MODULE_AUTHOR("Daniel Golle <daniel@makrotopia.org>"); +MODULE_LICENSE("GPL"); diff --git a/include/linux/pcs/pcs-standalone.h b/include/linux/pcs/pcs-standalone.h new file mode 100644 index 000000000000..ad7819f4a2eb --- /dev/null +++ b/include/linux/pcs/pcs-standalone.h @@ -0,0 +1,25 @@ +/* SPDX-License-Identifier: GPL-2.0 */ +#ifndef __LINUX_PCS_STANDALONE_H +#define __LINUX_PCS_STANDALONE_H + +#include <linux/device.h> +#include <linux/phylink.h> +#include <linux/phy/phy.h> +#include <linux/of.h> + +#if IS_ENABLED(CONFIG_PCS_STANDALONE) +int devm_pcs_register(struct device *dev, struct phylink_pcs *pcs); +struct phylink_pcs *devm_of_pcs_get(struct device *dev, + const struct device_node *np, unsigned int index); +#else +static inline int devm_pcs_register(struct device *dev, struct phylink_pcs *pcs); + return -EOPNOTSUPP; +} +static inline struct phylink_pcs *devm_of_pcs_get(struct device *dev, + const struct device_node *np, + unsigned int index) +{ + return ERR_PTR(-EOPNOTSUPP); +} +#endif /* CONFIG_PCS_STANDALONE */ +#endif /* __LINUX_PCS_STANDALONE_H */
Implement helper module for standalone PCS drivers which allows standaline PCS drivers to register and users to get instances of 'struct phylink_pcs' using device tree nodes. At this point only a single instance for each device tree node is supported, once we got devices providing more than one PCS we can extend it and introduce an xlate function as well as '#pcs-cells', similar to how this is done by the PHY framework. Signed-off-by: Daniel Golle <daniel@makrotopia.org> --- This is meant to provide the infrastructure suggested by Russell King in an earlier review. It just took me a long while to find the time to implement this. Users are going to be the standalone PCS drivers for 8/10 LynxI as well as 64/66 USXGMII PCS found on MediaTek MT7988 SoC. See also https://patchwork.kernel.org/comment/25636726/ The full tree where this is being used can be found at https://github.com/dangowrt/linux/commits/mt7988-for-next/ drivers/net/pcs/Kconfig | 4 ++ drivers/net/pcs/Makefile | 1 + drivers/net/pcs/pcs-standalone.c | 95 +++++++++++++++++++++++++++++ include/linux/pcs/pcs-standalone.h | 25 ++++++++ 4 files changed, 129 insertions(+) create mode 100644 drivers/net/pcs/pcs-standalone.c create mode 100644 include/linux/pcs/pcs-standalone.h