diff mbox series

[RFC,net-next,3/9] net: pcs: Add helpers for registering and finding PCSs

Message ID 20220711160519.741990-4-sean.anderson@seco.com
State New
Headers show
Series None | expand

Commit Message

Sean Anderson July 11, 2022, 4:05 p.m. UTC
This adds support for getting PCS devices from the device tree. PCS
drivers must first register with phylink_register_pcs. After that, MAC
drivers may look up their PCS using phylink_get_pcs.

To prevent the PCS driver from leaving suddenly, we use try_module_get. To
provide some ordering during probing/removal, we use device links managed
by of_fwnode_add_links. This will reduce the number of probe failures due
to deferral. It will not prevent this for non-standard properties (aka
pcsphy-handle), but the worst that happens is that we re-probe a few times.

At the moment there is no support for specifying the interface used to
talk to the PCS. The MAC driver is expected to know how to talk to the
PCS. This is not a change, but it is perhaps an area for improvement.

Signed-off-by: Sean Anderson <sean.anderson@seco.com>
---
This is adapted from [1], primarily incorporating the changes discussed
there.

[1] https://lore.kernel.org/netdev/9f73bc4f-5f99-95f5-78fa-dac96f9e0146@seco.com/

 MAINTAINERS              |   1 +
 drivers/net/pcs/Kconfig  |  12 +++
 drivers/net/pcs/Makefile |   2 +
 drivers/net/pcs/core.c   | 226 +++++++++++++++++++++++++++++++++++++++
 drivers/of/property.c    |   2 +
 include/linux/pcs.h      |  33 ++++++
 include/linux/phylink.h  |   6 ++
 7 files changed, 282 insertions(+)
 create mode 100644 drivers/net/pcs/core.c
 create mode 100644 include/linux/pcs.h

Comments

Sean Anderson July 12, 2022, 11:15 p.m. UTC | #1
On 7/12/22 11:51 AM, Russell King (Oracle) wrote:
> On Mon, Jul 11, 2022 at 05:47:26PM -0400, Sean Anderson wrote:
>> Hi Russell,
>> 
>> On 7/11/22 4:59 PM, Russell King (Oracle) wrote:
>> > Hi Sean,
>> > 
>> > It's a good attempt and may be nice to have, but I'm afraid the
>> > implementation has a flaw to do with the lifetime of data structures
>> > which always becomes a problem when we have multiple devices being
>> > used in aggregate.
>> > 
>> > On Mon, Jul 11, 2022 at 12:05:13PM -0400, Sean Anderson wrote:
>> >> +/**
>> >> + * pcs_get_tail() - Finish getting a PCS
>> >> + * @pcs: The PCS to get, or %NULL if one could not be found
>> >> + *
>> >> + * This performs common operations necessary when getting a PCS (chiefly
>> >> + * incrementing reference counts)
>> >> + *
>> >> + * Return: @pcs, or an error pointer on failure
>> >> + */
>> >> +static struct phylink_pcs *pcs_get_tail(struct phylink_pcs *pcs)
>> >> +{
>> >> +	if (!pcs)
>> >> +		return ERR_PTR(-EPROBE_DEFER);
>> >> +
>> >> +	if (!try_module_get(pcs->ops->owner))
>> >> +		return ERR_PTR(-ENODEV);
>> > 
>> > What you're trying to prevent here is the PCS going away - but holding a
>> > reference to the module doesn't prevent that with the driver model. The
>> > driver model design is such that a device can be unbound from its driver
>> > at any moment. Taking a reference to the module doesn't prevent that,
>> > all it does is ensure that the user can't remove the module. It doesn't
>> > mean that the "pcs" structure will remain allocated.
>> 
>> So how do things like (serdes) phys work? Presumably the same hazard
>> occurs any time a MAC uses a phy, because the phy can disappear at any time.
>> 
>> As it happens I can easily trigger an Oops by unbinding my serdes driver
>> and the plugging in an ethernet cable. Presumably this means that the phy
>> subsystem needs the devlink treatment? There are already several in-tree
>> MAC drivers using phys...
> 
> It's sadly another example of this kind of thing. When you consider
> that the system should operate in a safe manner with as few "gotchas"
> as possible, then being able to "easily trigger an Oops" is something
> that we should be avoiding. It's not hard to avoid - we have multiple
> mechanisms in the kernel now to deal with it. 

OK, so as mentioned above this exists in several MAC drivers already. How do
you propose to fix this?

> We have the component
> helper. We have devlinks. We can come up with other solutions such
> as what I mentioned in my previous reply (which I consider to be the
> superior solution in this case - because it doesn't mess up cases
> where a single struct device is associated with multiple network
> devices (such as on Armada 8040 based systems.)
> 
> It's really about "Quality of Implementation" - and I expect high
> quality. I don't want my systems crashing because I've tried to
> temporarily unbind some device.
> 
>> > The second issue that this creates is if a MAC driver creates the PCS
>> > and then "gets" it through this interface, then the MAC driver module
>> > ends up being locked in until the MAC driver devices are all unbound,
>> > which isn't friendly at all.
>> 
>> The intention here is not to use this for "internal" PCSs, but only for
>> external ones. I suppose you're referring to 
> 
> I wish I could say that intentions for use bear the test of time, but
> sadly I can not.

Well, we can burn that bridge when we come to it. For now, yes if you call
pcs_get_by_* from the same device where you call pcs_register then the device
will be "locked in".

>> > So, anything that proposes to create a new subsystem where we have
>> > multiple devices that make up an aggregate device needs to nicely cope
>> > with any of those devices going away. For that to happen in this
>> > instance, phylink would need to know that its in-use PCS for a
>> > particular MAC is going away, then it could force the link down before
>> > removing all references to the PCS device.
>> > 
>> > Another solution would be devlinks, but I am really not a fan of that
>> > when there may be a single struct device backing multiple network
>> > interfaces, where some of them may require PCS and others do not. One
>> > wouldn't want the network interface with nfs-root to suddenly go away
>> > because a PCS was unbound from its driver!
>> 
>> Well, you can also do
>> 
>> echo "mmc0:0001" > /sys/bus/mmc/drivers/mmcblk/unbind
>> 
>> which will (depending on your system) have the same effect.
>> 
>> If being able to unbind any driver at any time is intended,
>> then I don't think we can save userspace from itself.
> 
> If you unbind the device that contains your rootfs, you are absolutely
> correct. It's the same as taking down the network interface that gives
> you access to your NFS root.
> 
> However, neither of these cause the kernel to crash - they make
> userspace unusable.
> 
> So, let's say that it is acceptable that the kernel crashes if one
> unbinds a device. Why then bother with try_module_get() - if the user
> is silly enough to remove the module containing the PCS code, doesn't
> the same argument apply? "Shouldn't have done that then."
> 
> I don't see the logic.
> 

This was in response to your opposition to using devlink to manage the
PCS, since it would unbind the MAC as well. So what would happen here is
that someone would unbind the PCS, which would in turn unbind the MAC,
having the same effect as if the user manually unbound the MAC directly.

If you really want to avoid this, we'd need some kind of callback from
devlink to allow the MAC to say "well, I wasn't using that PCS anyway,"
or at the very least "let me clean up this (soon-to-be) dangling pointer."

--Sean
diff mbox series

Patch

diff --git a/MAINTAINERS b/MAINTAINERS
index ca95b1833b97..3965d49753d3 100644
--- a/MAINTAINERS
+++ b/MAINTAINERS
@@ -7450,6 +7450,7 @@  F:	include/linux/*mdio*.h
 F:	include/linux/mdio/*.h
 F:	include/linux/mii.h
 F:	include/linux/of_net.h
+F:	include/linux/pcs.h
 F:	include/linux/phy.h
 F:	include/linux/phy_fixed.h
 F:	include/linux/platform_data/mdio-bcm-unimac.h
diff --git a/drivers/net/pcs/Kconfig b/drivers/net/pcs/Kconfig
index 22ba7b0b476d..fed6264fdf33 100644
--- a/drivers/net/pcs/Kconfig
+++ b/drivers/net/pcs/Kconfig
@@ -5,6 +5,18 @@ 
 
 menu "PCS device drivers"
 
+config PCS
+	bool "PCS subsystem"
+	help
+	  This provides common helper functions for registering and looking up
+	  Physical Coding Sublayer (PCS) devices. PCS devices translate between
+	  different interface types. In some use cases, they may either
+	  translate between different types of Medium-Independent Interfaces
+	  (MIIs), such as translating GMII to SGMII. This allows using a fast
+	  serial interface to talk to the phy which translates the MII to the
+	  Medium-Dependent Interface. Alternatively, they may translate a MII
+	  directly to an MDI, such as translating GMII to 1000Base-X.
+
 config PCS_XPCS
 	tristate "Synopsys DesignWare XPCS controller"
 	depends on MDIO_DEVICE && MDIO_BUS
diff --git a/drivers/net/pcs/Makefile b/drivers/net/pcs/Makefile
index 0603d469bd57..1fd21a1619d4 100644
--- a/drivers/net/pcs/Makefile
+++ b/drivers/net/pcs/Makefile
@@ -1,6 +1,8 @@ 
 # SPDX-License-Identifier: GPL-2.0
 # Makefile for Linux PCS drivers
 
+obj-$(CONFIG_PCS)		+= core.o
+
 pcs_xpcs-$(CONFIG_PCS_XPCS)	:= pcs-xpcs.o pcs-xpcs-nxp.o
 
 obj-$(CONFIG_PCS_XPCS)		+= pcs_xpcs.o
diff --git a/drivers/net/pcs/core.c b/drivers/net/pcs/core.c
new file mode 100644
index 000000000000..b39ff1ccdb34
--- /dev/null
+++ b/drivers/net/pcs/core.c
@@ -0,0 +1,226 @@ 
+// SPDX-License-Identifier: GPL-2.0
+/*
+ * Copyright (C) 2022 Sean Anderson <sean.anderson@seco.com>
+ */
+
+#include <linux/fwnode.h>
+#include <linux/list.h>
+#include <linux/mutex.h>
+#include <linux/pcs.h>
+#include <linux/phylink.h>
+#include <linux/property.h>
+
+static LIST_HEAD(pcs_devices);
+static DEFINE_MUTEX(pcs_mutex);
+
+/**
+ * pcs_register() - register a new PCS
+ * @pcs: the PCS to register
+ *
+ * Registers a new PCS which can be automatically attached to a phylink.
+ *
+ * Return: 0 on success, or -errno on error
+ */
+int pcs_register(struct phylink_pcs *pcs)
+{
+	if (!pcs->dev || !pcs->ops)
+		return -EINVAL;
+	if (!pcs->ops->pcs_an_restart || !pcs->ops->pcs_config ||
+	    !pcs->ops->pcs_get_state)
+		return -EINVAL;
+
+	INIT_LIST_HEAD(&pcs->list);
+	mutex_lock(&pcs_mutex);
+	list_add(&pcs->list, &pcs_devices);
+	mutex_unlock(&pcs_mutex);
+	return 0;
+}
+EXPORT_SYMBOL_GPL(pcs_register);
+
+/**
+ * pcs_unregister() - unregister a PCS
+ * @pcs: a PCS previously registered with pcs_register()
+ */
+void pcs_unregister(struct phylink_pcs *pcs)
+{
+	mutex_lock(&pcs_mutex);
+	list_del(&pcs->list);
+	mutex_unlock(&pcs_mutex);
+}
+EXPORT_SYMBOL_GPL(pcs_unregister);
+
+static void devm_pcs_release(struct device *dev, void *res)
+{
+	pcs_unregister(*(struct phylink_pcs **)res);
+}
+
+/**
+ * devm_pcs_register - resource managed pcs_register()
+ * @dev: device that is registering this PCS
+ * @pcs: the PCS to register
+ *
+ * Managed pcs_register(). For PCSs registered by this function,
+ * pcs_unregister() is automatically called on driver detach. See
+ * pcs_register() for more information.
+ *
+ * Return: 0 on success, or -errno on failure
+ */
+int devm_pcs_register(struct device *dev, struct phylink_pcs *pcs)
+{
+	struct phylink_pcs **pcsp;
+	int ret;
+
+	pcsp = devres_alloc(devm_pcs_release, sizeof(*pcsp),
+			    GFP_KERNEL);
+	if (!pcsp)
+		return -ENOMEM;
+
+	ret = pcs_register(pcs);
+	if (ret) {
+		devres_free(pcsp);
+		return ret;
+	}
+
+	*pcsp = pcs;
+	devres_add(dev, pcsp);
+
+	return ret;
+}
+EXPORT_SYMBOL_GPL(devm_pcs_register);
+
+/**
+ * pcs_find() - Find the PCS associated with a fwnode or device
+ * @fwnode: The PCS's fwnode
+ * @dev: The PCS's device
+ *
+ * Search PCSs registered with pcs_register() for one with a matching
+ * fwnode or device. Either @fwnode or @dev may be %NULL if matching against a
+ * fwnode or device is not desired (respectively).
+ *
+ * Return: a matching PCS, or %NULL if not found
+ */
+static struct phylink_pcs *pcs_find(const struct fwnode_handle *fwnode,
+				    const struct device *dev)
+{
+	struct phylink_pcs *pcs;
+
+	mutex_lock(&pcs_mutex);
+	list_for_each_entry(pcs, &pcs_devices, list) {
+		if (dev && pcs->dev == dev)
+			goto out;
+		if (fwnode && pcs->dev->fwnode == fwnode)
+			goto out;
+	}
+	pcs = NULL;
+
+out:
+	mutex_unlock(&pcs_mutex);
+	pr_devel("%s: looking for %pfwf or %s %s...%s found\n", __func__,
+		 fwnode, dev ? dev_driver_string(dev) : "(null)",
+		 dev ? dev_name(dev) : "(null)", pcs ? " not" : "");
+	return pcs;
+}
+
+/**
+ * pcs_get_tail() - Finish getting a PCS
+ * @pcs: The PCS to get, or %NULL if one could not be found
+ *
+ * This performs common operations necessary when getting a PCS (chiefly
+ * incrementing reference counts)
+ *
+ * Return: @pcs, or an error pointer on failure
+ */
+static struct phylink_pcs *pcs_get_tail(struct phylink_pcs *pcs)
+{
+	if (!pcs)
+		return ERR_PTR(-EPROBE_DEFER);
+
+	if (!try_module_get(pcs->ops->owner))
+		return ERR_PTR(-ENODEV);
+	get_device(pcs->dev);
+
+	return pcs;
+}
+
+/**
+ * _pcs_get_by_fwnode() - Get a PCS from a fwnode property
+ * @fwnode: The fwnode to get an associated PCS of
+ * @id: The name of the PCS to get. May be %NULL to get the first PCS.
+ * @optional: Whether the PCS is optional or not
+ *
+ * Look up a PCS associated with @fwnode and return a reference to it. Every
+ * call to pcs_get_by_fwnode() must be balanced with one to pcs_put().
+ *
+ * If @optional is true, and @id is non-%NULL, then if @id cannot be found in
+ * pcs-names, %NULL is returned (instead of an error). If @optional is true and
+ * @id is %NULL, then no error is returned if pcs-handle is absent.
+ *
+ * Return: a PCS if found, or an error pointer on failure
+ */
+struct phylink_pcs *_pcs_get_by_fwnode(const struct fwnode_handle *fwnode,
+				       const char *id, bool optional)
+{
+	int index;
+	struct phylink_pcs *pcs;
+	struct fwnode_handle *pcs_fwnode;
+
+	if (id)
+		index = fwnode_property_match_string(fwnode, "pcs-names", id);
+	else
+		index = 0;
+	if (index < 0) {
+		if (optional && (index == -EINVAL || index == -ENODATA))
+			return NULL;
+		return ERR_PTR(index);
+	}
+
+	/* First try pcs-handle, and if that doesn't work fall back to the
+	 * (legacy) pcsphy-handle.
+	 */
+	pcs_fwnode = fwnode_find_reference(fwnode, "pcs-handle", index);
+	if (PTR_ERR(pcs_fwnode) == -ENOENT)
+		pcs_fwnode = fwnode_find_reference(fwnode, "pcsphy-handle",
+						   index);
+	if (optional && !id && PTR_ERR(pcs_fwnode) == -ENOENT)
+		return NULL;
+	else if (IS_ERR(pcs_fwnode))
+		return ERR_CAST(pcs_fwnode);
+
+	pcs = pcs_find(pcs_fwnode, NULL);
+	fwnode_handle_put(pcs_fwnode);
+	return pcs_get_tail(pcs);
+}
+EXPORT_SYMBOL_GPL(pcs_get_by_fwnode);
+
+/**
+ * pcs_get_by_provider() - Get a PCS from an existing provider
+ * @dev: The device providing the PCS
+ *
+ * This finds the first PCS registersed by @dev and returns a reference to it.
+ * Every call to pcs_get_by_provider() must be balanced with one to
+ * pcs_put().
+ *
+ * Return: a PCS if found, or an error pointer on failure
+ */
+struct phylink_pcs *pcs_get_by_provider(const struct device *dev)
+{
+	return pcs_get_tail(pcs_find(NULL, dev));
+}
+EXPORT_SYMBOL_GPL(pcs_get_by_provider);
+
+/**
+ * pcs_put() - Release a previously-acquired PCS
+ * @pcs: The PCS to put
+ *
+ * This frees resources associated with the PCS which were acquired when it was
+ * gotten.
+ */
+void pcs_put(struct phylink_pcs *pcs)
+{
+	if (!pcs)
+		return;
+
+	put_device(pcs->dev);
+	module_put(pcs->ops->owner);
+}
+EXPORT_SYMBOL_GPL(pcs_put);
diff --git a/drivers/of/property.c b/drivers/of/property.c
index 967f79b59016..860d35bde5e9 100644
--- a/drivers/of/property.c
+++ b/drivers/of/property.c
@@ -1318,6 +1318,7 @@  DEFINE_SIMPLE_PROP(pinctrl6, "pinctrl-6", NULL)
 DEFINE_SIMPLE_PROP(pinctrl7, "pinctrl-7", NULL)
 DEFINE_SIMPLE_PROP(pinctrl8, "pinctrl-8", NULL)
 DEFINE_SIMPLE_PROP(remote_endpoint, "remote-endpoint", NULL)
+DEFINE_SIMPLE_PROP(pcs_handle, "pcs-handle", NULL)
 DEFINE_SIMPLE_PROP(pwms, "pwms", "#pwm-cells")
 DEFINE_SIMPLE_PROP(resets, "resets", "#reset-cells")
 DEFINE_SIMPLE_PROP(leds, "leds", NULL)
@@ -1406,6 +1407,7 @@  static const struct supplier_bindings of_supplier_bindings[] = {
 	{ .parse_prop = parse_pinctrl7, },
 	{ .parse_prop = parse_pinctrl8, },
 	{ .parse_prop = parse_remote_endpoint, .node_not_dev = true, },
+	{ .parse_prop = parse_pcs_handle, },
 	{ .parse_prop = parse_pwms, },
 	{ .parse_prop = parse_resets, },
 	{ .parse_prop = parse_leds, },
diff --git a/include/linux/pcs.h b/include/linux/pcs.h
new file mode 100644
index 000000000000..00e76594e03c
--- /dev/null
+++ b/include/linux/pcs.h
@@ -0,0 +1,33 @@ 
+/* SPDX-License-Identifier: GPL-2.0 */
+/*
+ * Copyright (C) 2022 Sean Anderson <sean.anderson@seco.com>
+ */
+
+#ifndef _PCS_H
+#define _PCS_H
+
+struct phylink_pcs;
+struct fwnode;
+
+int pcs_register(struct phylink_pcs *pcs);
+void pcs_unregister(struct phylink_pcs *pcs);
+int devm_pcs_register(struct device *dev, struct phylink_pcs *pcs);
+struct phylink_pcs *_pcs_get_by_fwnode(const struct fwnode_handle *fwnode,
+				       const char *id, bool optional);
+struct phylink_pcs *pcs_get_by_provider(const struct device *dev);
+void pcs_put(struct phylink_pcs *pcs);
+
+static inline struct phylink_pcs
+*pcs_get_by_fwnode(const struct fwnode_handle *fwnode,
+		   const char *id)
+{
+	return _pcs_get_by_fwnode(fwnode, id, false);
+}
+
+static inline struct phylink_pcs
+*pcs_get_by_fwnode_optional(const struct fwnode_handle *fwnode, const char *id)
+{
+	return _pcs_get_by_fwnode(fwnode, id, true);
+}
+
+#endif /* PCS_H */
diff --git a/include/linux/phylink.h b/include/linux/phylink.h
index 6d06896fc20d..a713e70108a1 100644
--- a/include/linux/phylink.h
+++ b/include/linux/phylink.h
@@ -396,19 +396,24 @@  struct phylink_pcs_ops;
 
 /**
  * struct phylink_pcs - PHYLINK PCS instance
+ * @dev: the device associated with this PCS
  * @ops: a pointer to the &struct phylink_pcs_ops structure
+ * @list: internal list of PCS devices
  * @poll: poll the PCS for link changes
  *
  * This structure is designed to be embedded within the PCS private data,
  * and will be passed between phylink and the PCS.
  */
 struct phylink_pcs {
+	struct device *dev;
 	const struct phylink_pcs_ops *ops;
+	struct list_head list;
 	bool poll;
 };
 
 /**
  * struct phylink_pcs_ops - MAC PCS operations structure.
+ * @owner: the module which implements this PCS.
  * @pcs_validate: validate the link configuration.
  * @pcs_get_state: read the current MAC PCS link state from the hardware.
  * @pcs_config: configure the MAC PCS for the selected mode and state.
@@ -417,6 +422,7 @@  struct phylink_pcs {
  *               (where necessary).
  */
 struct phylink_pcs_ops {
+	struct module *owner;
 	int (*pcs_validate)(struct phylink_pcs *pcs, unsigned long *supported,
 			    const struct phylink_link_state *state);
 	void (*pcs_get_state)(struct phylink_pcs *pcs,