On Wed, Feb 19, 2025 at 06:46:57PM +0800, Lei Wei wrote:

quoted

quoted

2) there's yet another open coded "_get" function for getting the
PCS given a DT node which is different from every other "_get"
function - this one checks the parent DT node has an appropriate
compatible whereas others don't. The whole poliferation of "_get"
methods that are specific to each PCS still needs solving, and I
still have the big question around what happens when the PCS driver
gets unbound - and whether that causes the kernel to oops. I'm also
not a fan of "look up the struct device and then get its driver data".
There is *no* locking over accessing the driver data.

The PCS device in IPQ9574 chipset is built into the SoC chip and is not
pluggable. Also, the PCS driver module is not unloadable until the MAC
driver that depends on it is unloaded. Therefore, marking the driver
'.suppress_bind_attrs = true' to disable user unbind action may be good
enough to cover all possible scenarios of device going away for IPQ9574 PCS
driver.

What I am concerned about is the proliferation of these various PCS
specific "_get" methods. Where the PCS is looked up by firmware
reference, we should have a common way to do that, rather than all
these PCS specific ways.

I did start work on that, but I just haven't had the time to take it
forward. This is about as far as I'd got:

diff --git a/drivers/net/pcs/Makefile b/drivers/net/pcs/Makefile
index 4f7920618b90..0b670fee0757 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_PHYLINK)		+= pcs-core.o
+
  pcs_xpcs-$(CONFIG_PCS_XPCS)	:= pcs-xpcs.o pcs-xpcs-plat.o \
  				   pcs-xpcs-nxp.o pcs-xpcs-wx.o
  

diff --git a/drivers/net/phy/phylink.c b/drivers/net/phy/phylink.c
index 976e569feb70..1c5492dab00e 100644
--- a/drivers/net/phy/phylink.c
+++ b/drivers/net/phy/phylink.c

@@ -2483,6 +2483,15 @@ void phylink_pcs_change(struct phylink_pcs *pcs, bool up)
  }
  EXPORT_SYMBOL_GPL(phylink_pcs_change);
  
+/**
+ * phylink_pcs_remove() - notify phylink that a PCS is going away
+ * @pcs: PCS that is going away
+ */
+void phylink_pcs_remove(struct phylink_pcs *pcs)
+{
+	
+}
+
  static irqreturn_t phylink_link_handler(int irq, void *data)
  {
  	struct phylink *pl = data;

diff --git a/include/linux/phylink.h b/include/linux/phylink.h
index 071ed4683c8c..1e6b7ce0fa7a 100644
--- a/include/linux/phylink.h
+++ b/include/linux/phylink.h

@@ -1,6 +1,7 @@
  #ifndef NETDEV_PCS_H
  #define NETDEV_PCS_H
  
+#include <linux/list.h>
  #include <linux/phy.h>
  #include <linux/spinlock.h>
  #include <linux/workqueue.h>

@@ -435,9 +436,11 @@ int mac_enable_tx_lpi(struct phylink_config *config, u32 timer,
  #endif
  
  struct phylink_pcs_ops;
+struct pcs_lookup;
  
  /**
   * struct phylink_pcs - PHYLINK PCS instance
+ * @lookup: private member for PCS core management
   * @supported_interfaces: describing which PHY_INTERFACE_MODE_xxx
   *                        are supported by this PCS.
   * @ops: a pointer to the &struct phylink_pcs_ops structure

@@ -455,6 +458,7 @@ struct phylink_pcs_ops;
   * the PCS driver.
   */
  struct phylink_pcs {
+	struct pcs_lookup *lookup;
  	DECLARE_PHY_INTERFACE_MASK(supported_interfaces);
  	const struct phylink_pcs_ops *ops;
  	struct phylink *phylink;

@@ -692,6 +696,7 @@ int phylink_set_fixed_link(struct phylink *,
  
  void phylink_mac_change(struct phylink *, bool up);
  void phylink_pcs_change(struct phylink_pcs *, bool up);
+void phylink_pcs_remove(struct phylink_pcs *);
  
  int phylink_pcs_pre_init(struct phylink *pl, struct phylink_pcs *pcs);
  

@@ -790,4 +795,11 @@ void phylink_mii_c45_pcs_get_state(struct mdio_device *pcs,
  
  void phylink_decode_usxgmii_word(struct phylink_link_state *state,
  				 uint16_t lpa);
+
+/* PCS lookup */
+struct phylink_pcs *pcs_find(void *id);
+void pcs_remove(struct phylink_pcs *pcs);
+int pcs_add(struct phylink_pcs *pcs, void *id);
+int devm_pcs_add(struct device *dev, struct phylink_pcs *pcs, void *id);
+
  #endif

The idea is that you add the device using whatever identifier you decide
(the pointer value is what's matched). For example, a fwnode. You can
then find it using pcs_find().

If it returns NULL, then it's not (yet) registered - if you know that it
should exist (e.g. because the fwnode is marked as available) then you
can return -EPROBE_DEFER or fail.

There is a hook present so phylink can do something on PCS removal -
that's still to be implemented with this. I envision keeping a list
of phylink instances, and walking that list to discover if any phylink
instances are currently using the PCS. If they are, then we can take
the link down.