Re: [PATCH RFC net-next] net: pcs: add helper module for standalone drivers

["Russell King (Oracle)" <linux@armlinux.org.uk>]

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.

-- 
RMK's Patch system: https://www.armlinux.org.uk/developer/patches/
FTTP is here! 80Mbps down 10Mbps up. Decent connectivity at last!