[RFC PATCH v2 net-next 0/9] Convert xpcs to phylink_pcs_ops

[RFC PATCH v2 net-next 0/9] Convert xpcs to phylink_pcs_ops

[Vladimir Oltean]

From: Vladimir Oltean <vladimir.oltean@nxp.com>

This patch series is partially tested (some code paths have been covered
on the NXP SJA1105) but since I don't have stmmac hardware, it would
still be appreciated if people from Intel could give this another run.

Background: the sja1105 DSA driver currently drives a Designware XPCS
for SGMII and 2500base-X, and it would be nice to reuse some code with
the xpcs module. This would also help consolidate the phylink_pcs_ops,
since the only other dedicated PCS driver, currently, is the lynx_pcs.

Therefore, this series makes the xpcs expose the same kind of API that
the lynx_pcs module does. The main changes are getting rid of struct
mdio_xpcs_ops, being compatible with struct phylink_pcs_ops and being
less reliant on the phy_interface_t passed to xpcs_probe (now renamed to
xpcs_create).

Vladimir Oltean (9):
  net: pcs: xpcs: delete shim definition for mdio_xpcs_get_ops()
  net: pcs: xpcs: there is only one PHY ID
  net: pcs: xpcs: make the checks related to the PHY interface mode
    stateless
  net: pcs: xpcs: export xpcs_validate
  net: pcs: xpcs: export xpcs_config_eee
  net: pcs: xpcs: export xpcs_probe
  net: pcs: xpcs: use mdiobus_c45_addr in xpcs_{read,write}
  net: pcs: xpcs: convert to mdio_device
  net: pcs: xpcs: convert to phylink_pcs_ops

 drivers/net/ethernet/stmicro/stmmac/common.h  |   3 +-
 drivers/net/ethernet/stmicro/stmmac/hwif.h    |  14 -
 .../ethernet/stmicro/stmmac/stmmac_ethtool.c  |  12 +-
 .../net/ethernet/stmicro/stmmac/stmmac_main.c |  44 +--
 .../net/ethernet/stmicro/stmmac/stmmac_mdio.c |  47 ++-
 drivers/net/pcs/pcs-xpcs.c                    | 371 ++++++++++++------
 include/linux/pcs/pcs-xpcs.h                  |  40 +-
 7 files changed, 306 insertions(+), 225 deletions(-)

-- 
2.25.1

[RFC PATCH v2 net-next 1/9] net: pcs: xpcs: delete shim definition for mdio_xpcs_get_ops()

[Vladimir Oltean]

From: Vladimir Oltean <vladimir.oltean@nxp.com>

CONFIG_STMMAC_ETH selects CONFIG_PCS_XPCS, so there should be no
situation where the shim should be needed.

Signed-off-by: Vladimir Oltean <vladimir.oltean@nxp.com>
---
 include/linux/pcs/pcs-xpcs.h | 7 -------
 1 file changed, 7 deletions(-)

diff --git a/include/linux/pcs/pcs-xpcs.h b/include/linux/pcs/pcs-xpcs.h
index 5938ced805f4..c4d0a2c469c7 100644
--- a/include/linux/pcs/pcs-xpcs.h
+++ b/include/linux/pcs/pcs-xpcs.h
@@ -36,13 +36,6 @@ struct mdio_xpcs_ops {
 			  int enable);
 };
 
-#if IS_ENABLED(CONFIG_PCS_XPCS)
 struct mdio_xpcs_ops *mdio_xpcs_get_ops(void);
-#else
-static inline struct mdio_xpcs_ops *mdio_xpcs_get_ops(void)
-{
-	return NULL;
-}
-#endif
 
 #endif /* __LINUX_PCS_XPCS_H */
-- 
2.25.1

[RFC PATCH v2 net-next 2/9] net: pcs: xpcs: there is only one PHY ID

[Vladimir Oltean]

From: Vladimir Oltean <vladimir.oltean@nxp.com>

The xpcs driver has an apparently inadequate structure for the actual
hardware it drives.

These defines and the xpcs_probe() function would suggest that there is
one PHY ID per supported PHY interface type, and the driver simply
validates whether the mode it should operate in (the argument of
xpcs_probe) matches what the hardware is capable of:

	#define SYNOPSYS_XPCS_USXGMII_ID	0x7996ced0
	#define SYNOPSYS_XPCS_10GKR_ID		0x7996ced0
	#define SYNOPSYS_XPCS_XLGMII_ID		0x7996ced0
	#define SYNOPSYS_XPCS_SGMII_ID		0x7996ced0
	#define SYNOPSYS_XPCS_MASK		0xffffffff

but that is not the case, because upon closer inspection, all the above
4 PHY ID definitions are in fact equal.

So it is the same XPCS that is compatible with all 4 sets of PHY
interface types.

This change introduces an array of struct xpcs_compat which is populated
by the single struct xpcs_id instance. It also eliminates the bogus
defines for multiple Synopsys XPCS PHY IDs and replaces them with a
single XPCS_ID, which better reflects the way in which the hardware
operates.

Signed-off-by: Vladimir Oltean <vladimir.oltean@nxp.com>
---
 drivers/net/pcs/pcs-xpcs.c | 103 +++++++++++++++++++++++--------------
 1 file changed, 65 insertions(+), 38 deletions(-)

diff --git a/drivers/net/pcs/pcs-xpcs.c b/drivers/net/pcs/pcs-xpcs.c
index aa985a5aae8d..8491cfe1c11d 100644
--- a/drivers/net/pcs/pcs-xpcs.c
+++ b/drivers/net/pcs/pcs-xpcs.c
@@ -12,10 +12,7 @@
 #include <linux/phylink.h>
 #include <linux/workqueue.h>
 
-#define SYNOPSYS_XPCS_USXGMII_ID	0x7996ced0
-#define SYNOPSYS_XPCS_10GKR_ID		0x7996ced0
-#define SYNOPSYS_XPCS_XLGMII_ID		0x7996ced0
-#define SYNOPSYS_XPCS_SGMII_ID		0x7996ced0
+#define SYNOPSYS_XPCS_ID		0x7996ced0
 #define SYNOPSYS_XPCS_MASK		0xffffffff
 
 /* Vendor regs access */
@@ -163,58 +160,76 @@ static const int xpcs_sgmii_features[] = {
 
 static const phy_interface_t xpcs_usxgmii_interfaces[] = {
 	PHY_INTERFACE_MODE_USXGMII,
-	PHY_INTERFACE_MODE_MAX,
 };
 
 static const phy_interface_t xpcs_10gkr_interfaces[] = {
 	PHY_INTERFACE_MODE_10GKR,
-	PHY_INTERFACE_MODE_MAX,
 };
 
 static const phy_interface_t xpcs_xlgmii_interfaces[] = {
 	PHY_INTERFACE_MODE_XLGMII,
-	PHY_INTERFACE_MODE_MAX,
 };
 
 static const phy_interface_t xpcs_sgmii_interfaces[] = {
 	PHY_INTERFACE_MODE_SGMII,
-	PHY_INTERFACE_MODE_MAX,
 };
 
-static struct xpcs_id {
-	u32 id;
-	u32 mask;
+enum {
+	DW_XPCS_USXGMII,
+	DW_XPCS_10GKR,
+	DW_XPCS_XLGMII,
+	DW_XPCS_SGMII,
+	DW_XPCS_INTERFACE_MAX,
+};
+
+struct xpcs_compat {
 	const int *supported;
 	const phy_interface_t *interface;
+	int num_interfaces;
 	int an_mode;
-} xpcs_id_list[] = {
-	{
-		.id = SYNOPSYS_XPCS_USXGMII_ID,
-		.mask = SYNOPSYS_XPCS_MASK,
+};
+
+static const struct xpcs_compat synopsys_xpcs_compat[DW_XPCS_INTERFACE_MAX] = {
+	[DW_XPCS_USXGMII] = {
 		.supported = xpcs_usxgmii_features,
 		.interface = xpcs_usxgmii_interfaces,
+		.num_interfaces = ARRAY_SIZE(xpcs_usxgmii_interfaces),
 		.an_mode = DW_AN_C73,
-	}, {
-		.id = SYNOPSYS_XPCS_10GKR_ID,
-		.mask = SYNOPSYS_XPCS_MASK,
+	},
+	[DW_XPCS_10GKR] = {
 		.supported = xpcs_10gkr_features,
 		.interface = xpcs_10gkr_interfaces,
+		.num_interfaces = ARRAY_SIZE(xpcs_10gkr_interfaces),
 		.an_mode = DW_AN_C73,
-	}, {
-		.id = SYNOPSYS_XPCS_XLGMII_ID,
-		.mask = SYNOPSYS_XPCS_MASK,
+	},
+	[DW_XPCS_XLGMII] = {
 		.supported = xpcs_xlgmii_features,
 		.interface = xpcs_xlgmii_interfaces,
+		.num_interfaces = ARRAY_SIZE(xpcs_xlgmii_interfaces),
 		.an_mode = DW_AN_C73,
-	}, {
-		.id = SYNOPSYS_XPCS_SGMII_ID,
-		.mask = SYNOPSYS_XPCS_MASK,
+	},
+	[DW_XPCS_SGMII] = {
 		.supported = xpcs_sgmii_features,
 		.interface = xpcs_sgmii_interfaces,
+		.num_interfaces = ARRAY_SIZE(xpcs_sgmii_interfaces),
 		.an_mode = DW_AN_C37_SGMII,
 	},
 };
 
+struct xpcs_id {
+	u32 id;
+	u32 mask;
+	const struct xpcs_compat *compat;
+};
+
+static const struct xpcs_id xpcs_id_list[] = {
+	{
+		.id = SYNOPSYS_XPCS_ID,
+		.mask = SYNOPSYS_XPCS_MASK,
+		.compat = synopsys_xpcs_compat,
+	},
+};
+
 static int xpcs_read(struct mdio_xpcs_args *xpcs, int dev, u32 reg)
 {
 	u32 reg_addr = MII_ADDR_C45 | dev << 16 | reg;
@@ -911,35 +926,47 @@ static u32 xpcs_get_id(struct mdio_xpcs_args *xpcs)
 }
 
 static bool xpcs_check_features(struct mdio_xpcs_args *xpcs,
-				struct xpcs_id *match,
+				const struct xpcs_id *match,
 				phy_interface_t interface)
 {
-	int i;
+	int i, j;
 
-	for (i = 0; match->interface[i] != PHY_INTERFACE_MODE_MAX; i++) {
-		if (match->interface[i] == interface)
-			break;
-	}
+	for (i = 0; i < DW_XPCS_INTERFACE_MAX; i++) {
+		const struct xpcs_compat *compat = &match->compat[i];
+		bool supports_interface = false;
 
-	if (match->interface[i] == PHY_INTERFACE_MODE_MAX)
-		return false;
+		for (j = 0; j < compat->num_interfaces; j++) {
+			if (compat->interface[j] == interface) {
+				supports_interface = true;
+				break;
+			}
+		}
 
-	for (i = 0; match->supported[i] != __ETHTOOL_LINK_MODE_MASK_NBITS; i++)
-		set_bit(match->supported[i], xpcs->supported);
+		if (!supports_interface)
+			continue;
+
+		/* Populate the supported link modes for this
+		 * PHY interface type
+		 */
+		for (j = 0; compat->supported[j] != __ETHTOOL_LINK_MODE_MASK_NBITS; j++)
+			set_bit(compat->supported[j], xpcs->supported);
 
-	xpcs->an_mode = match->an_mode;
+		xpcs->an_mode = compat->an_mode;
+
+		return true;
+	}
 
-	return true;
+	return false;
 }
 
 static int xpcs_probe(struct mdio_xpcs_args *xpcs, phy_interface_t interface)
 {
+	const struct xpcs_id *match = NULL;
 	u32 xpcs_id = xpcs_get_id(xpcs);
-	struct xpcs_id *match = NULL;
 	int i;
 
 	for (i = 0; i < ARRAY_SIZE(xpcs_id_list); i++) {
-		struct xpcs_id *entry = &xpcs_id_list[i];
+		const struct xpcs_id *entry = &xpcs_id_list[i];
 
 		if ((xpcs_id & entry->mask) == entry->id) {
 			match = entry;
-- 
2.25.1

[RFC PATCH v2 net-next 3/9] net: pcs: xpcs: make the checks related to the PHY interface mode stateless

[Vladimir Oltean]

From: Vladimir Oltean <vladimir.oltean@nxp.com>

The operating mode of the driver is currently to populate its
struct mdio_xpcs_args::supported and struct mdio_xpcs_args::an_mode
statically in xpcs_probe(), based on the passed phy_interface_t,
and work with those.

However this is not the operation that phylink expects from a PCS
driver, because the port might be attached to an SFP cage that triggers
changes of the phy_interface_t dynamically as one SFP module is
unpluggged and another is plugged.

To migrate towards that model, the struct mdio_xpcs_args should not
cache anything related to the phy_interface_t, but just look up the
statically defined, const struct xpcs_compat structure corresponding to
the detected PCS OUI/model number.

So we delete the "supported" and "an_mode" members of struct
mdio_xpcs_args, and add the "id" structure there (since the ID is not
expected to change at runtime).

Since xpcs->supported is used deep in the code in _xpcs_config_aneg_c73(),
we need to modify some function headers to pass the xpcs_compat from all
callers. In turn, the xpcs_compat is always supplied externally to the
xpcs module:
- Most of the time by phylink
- In xpcs_probe() it is needed because xpcs_soft_reset() writes to
  MDIO_MMD_PCS or to MDIO_MMD_VEND2 depending on whether an_mode is clause
  37 or clause 73. In order to not introduce functional changes related
  to when the soft reset is issued, we continue to require the initial
  phy_interface_t argument to be passed to xpcs_probe() so we can pass
  this on to xpcs_soft_reset().
- stmmac_open() wants to know whether to call stmmac_init_phy() or not,
  and for that it looks inside xpcs->an_mode, because the clause 73
  (backplane) AN modes supposedly do not have a PHY. Because we moved
  an_mode outside of struct mdio_xpcs_args, this is now no longer
  directly possible, so we introduce a helper function xpcs_get_an_mode()
  which protects the data encapsulation of the xpcs module and requires
  a phy_interface_t to be passed as argument. This function can look up
  the appropriate compat based on the phy_interface_t.

Signed-off-by: Vladimir Oltean <vladimir.oltean@nxp.com>
---
 .../net/ethernet/stmicro/stmmac/stmmac_main.c |   4 +-
 drivers/net/pcs/pcs-xpcs.c                    | 175 +++++++++++-------
 include/linux/pcs/pcs-xpcs.h                  |   6 +-
 3 files changed, 120 insertions(+), 65 deletions(-)

diff --git a/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c b/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c
index 9962a1041d35..129b103cd2b5 100644
--- a/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c
+++ b/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c
@@ -3637,6 +3637,7 @@ static int stmmac_request_irq(struct net_device *dev)
 int stmmac_open(struct net_device *dev)
 {
 	struct stmmac_priv *priv = netdev_priv(dev);
+	int mode = priv->plat->phy_interface;
 	int bfsize = 0;
 	u32 chan;
 	int ret;
@@ -3649,7 +3650,8 @@ int stmmac_open(struct net_device *dev)
 
 	if (priv->hw->pcs != STMMAC_PCS_TBI &&
 	    priv->hw->pcs != STMMAC_PCS_RTBI &&
-	    priv->hw->xpcs_args.an_mode != DW_AN_C73) {
+	    (!priv->hw->xpcs ||
+	     xpcs_get_an_mode(&priv->hw->xpcs_args, mode) != DW_AN_C73)) {
 		ret = stmmac_init_phy(dev);
 		if (ret) {
 			netdev_err(priv->dev,
diff --git a/drivers/net/pcs/pcs-xpcs.c b/drivers/net/pcs/pcs-xpcs.c
index 8491cfe1c11d..4bc63d7e3bda 100644
--- a/drivers/net/pcs/pcs-xpcs.c
+++ b/drivers/net/pcs/pcs-xpcs.c
@@ -230,6 +230,49 @@ static const struct xpcs_id xpcs_id_list[] = {
 	},
 };
 
+static const struct xpcs_compat *xpcs_find_compat(const struct xpcs_id *id,
+						  phy_interface_t interface)
+{
+	int i, j;
+
+	for (i = 0; i < DW_XPCS_INTERFACE_MAX; i++) {
+		const struct xpcs_compat *compat = &id->compat[i];
+
+		for (j = 0; j < compat->num_interfaces; j++)
+			if (compat->interface[j] == interface)
+				return compat;
+	}
+
+	return NULL;
+}
+
+int xpcs_get_an_mode(struct mdio_xpcs_args *xpcs, phy_interface_t interface)
+{
+	const struct xpcs_compat *compat;
+
+	compat = xpcs_find_compat(xpcs->id, interface);
+	if (!compat)
+		return -ENODEV;
+
+	return compat->an_mode;
+}
+EXPORT_SYMBOL_GPL(xpcs_get_an_mode);
+
+static bool __xpcs_linkmode_supported(const struct xpcs_compat *compat,
+				      enum ethtool_link_mode_bit_indices linkmode)
+{
+	int i;
+
+	for (i = 0; compat->supported[i] != __ETHTOOL_LINK_MODE_MASK_NBITS; i++)
+		if (compat->supported[i] == linkmode)
+			return true;
+
+	return false;
+}
+
+#define xpcs_linkmode_supported(compat, mode) \
+	__xpcs_linkmode_supported(compat, ETHTOOL_LINK_MODE_ ## mode ## _BIT)
+
 static int xpcs_read(struct mdio_xpcs_args *xpcs, int dev, u32 reg)
 {
 	u32 reg_addr = MII_ADDR_C45 | dev << 16 | reg;
@@ -281,11 +324,12 @@ static int xpcs_poll_reset(struct mdio_xpcs_args *xpcs, int dev)
 	return (ret & MDIO_CTRL1_RESET) ? -ETIMEDOUT : 0;
 }
 
-static int xpcs_soft_reset(struct mdio_xpcs_args *xpcs)
+static int xpcs_soft_reset(struct mdio_xpcs_args *xpcs,
+			   const struct xpcs_compat *compat)
 {
 	int ret, dev;
 
-	switch (xpcs->an_mode) {
+	switch (compat->an_mode) {
 	case DW_AN_C73:
 		dev = MDIO_MMD_PCS;
 		break;
@@ -454,7 +498,8 @@ static int xpcs_config_usxgmii(struct mdio_xpcs_args *xpcs, int speed)
 	return xpcs_write_vpcs(xpcs, MDIO_CTRL1, ret | DW_USXGMII_RST);
 }
 
-static int _xpcs_config_aneg_c73(struct mdio_xpcs_args *xpcs)
+static int _xpcs_config_aneg_c73(struct mdio_xpcs_args *xpcs,
+				 const struct xpcs_compat *compat)
 {
 	int ret, adv;
 
@@ -466,7 +511,7 @@ static int _xpcs_config_aneg_c73(struct mdio_xpcs_args *xpcs)
 
 	/* SR_AN_ADV3 */
 	adv = 0;
-	if (phylink_test(xpcs->supported, 2500baseX_Full))
+	if (xpcs_linkmode_supported(compat, 2500baseX_Full))
 		adv |= DW_C73_2500KX;
 
 	/* TODO: 5000baseKR */
@@ -477,11 +522,11 @@ static int _xpcs_config_aneg_c73(struct mdio_xpcs_args *xpcs)
 
 	/* SR_AN_ADV2 */
 	adv = 0;
-	if (phylink_test(xpcs->supported, 1000baseKX_Full))
+	if (xpcs_linkmode_supported(compat, 1000baseKX_Full))
 		adv |= DW_C73_1000KX;
-	if (phylink_test(xpcs->supported, 10000baseKX4_Full))
+	if (xpcs_linkmode_supported(compat, 10000baseKX4_Full))
 		adv |= DW_C73_10000KX4;
-	if (phylink_test(xpcs->supported, 10000baseKR_Full))
+	if (xpcs_linkmode_supported(compat, 10000baseKR_Full))
 		adv |= DW_C73_10000KR;
 
 	ret = xpcs_write(xpcs, MDIO_MMD_AN, DW_SR_AN_ADV2, adv);
@@ -490,19 +535,20 @@ static int _xpcs_config_aneg_c73(struct mdio_xpcs_args *xpcs)
 
 	/* SR_AN_ADV1 */
 	adv = DW_C73_AN_ADV_SF;
-	if (phylink_test(xpcs->supported, Pause))
+	if (xpcs_linkmode_supported(compat, Pause))
 		adv |= DW_C73_PAUSE;
-	if (phylink_test(xpcs->supported, Asym_Pause))
+	if (xpcs_linkmode_supported(compat, Asym_Pause))
 		adv |= DW_C73_ASYM_PAUSE;
 
 	return xpcs_write(xpcs, MDIO_MMD_AN, DW_SR_AN_ADV1, adv);
 }
 
-static int xpcs_config_aneg_c73(struct mdio_xpcs_args *xpcs)
+static int xpcs_config_aneg_c73(struct mdio_xpcs_args *xpcs,
+				const struct xpcs_compat *compat)
 {
 	int ret;
 
-	ret = _xpcs_config_aneg_c73(xpcs);
+	ret = _xpcs_config_aneg_c73(xpcs, compat);
 	if (ret < 0)
 		return ret;
 
@@ -516,7 +562,8 @@ static int xpcs_config_aneg_c73(struct mdio_xpcs_args *xpcs)
 }
 
 static int xpcs_aneg_done_c73(struct mdio_xpcs_args *xpcs,
-			      struct phylink_link_state *state)
+			      struct phylink_link_state *state,
+			      const struct xpcs_compat *compat)
 {
 	int ret;
 
@@ -531,7 +578,7 @@ static int xpcs_aneg_done_c73(struct mdio_xpcs_args *xpcs,
 
 		/* Check if Aneg outcome is valid */
 		if (!(ret & DW_C73_AN_ADV_SF)) {
-			xpcs_config_aneg_c73(xpcs);
+			xpcs_config_aneg_c73(xpcs, compat);
 			return 0;
 		}
 
@@ -677,8 +724,31 @@ static int xpcs_validate(struct mdio_xpcs_args *xpcs,
 			 unsigned long *supported,
 			 struct phylink_link_state *state)
 {
-	linkmode_and(supported, supported, xpcs->supported);
-	linkmode_and(state->advertising, state->advertising, xpcs->supported);
+	__ETHTOOL_DECLARE_LINK_MODE_MASK(xpcs_supported);
+	const struct xpcs_compat *compat;
+	int i;
+
+	/* phylink expects us to report all supported modes with
+	 * PHY_INTERFACE_MODE_NA, just don't limit the supported and
+	 * advertising masks and exit.
+	 */
+	if (state->interface == PHY_INTERFACE_MODE_NA)
+		return 0;
+
+	bitmap_zero(xpcs_supported, __ETHTOOL_LINK_MODE_MASK_NBITS);
+
+	compat = xpcs_find_compat(xpcs->id, state->interface);
+
+	/* Populate the supported link modes for this
+	 * PHY interface type
+	 */
+	if (compat)
+		for (i = 0; compat->supported[i] != __ETHTOOL_LINK_MODE_MASK_NBITS; i++)
+			set_bit(compat->supported[i], xpcs_supported);
+
+	linkmode_and(supported, supported, xpcs_supported);
+	linkmode_and(state->advertising, state->advertising, xpcs_supported);
+
 	return 0;
 }
 
@@ -759,12 +829,17 @@ static int xpcs_config_aneg_c37_sgmii(struct mdio_xpcs_args *xpcs)
 static int xpcs_config(struct mdio_xpcs_args *xpcs,
 		       const struct phylink_link_state *state)
 {
+	const struct xpcs_compat *compat;
 	int ret;
 
-	switch (xpcs->an_mode) {
+	compat = xpcs_find_compat(xpcs->id, state->interface);
+	if (!compat)
+		return -ENODEV;
+
+	switch (compat->an_mode) {
 	case DW_AN_C73:
 		if (state->an_enabled) {
-			ret = xpcs_config_aneg_c73(xpcs);
+			ret = xpcs_config_aneg_c73(xpcs, compat);
 			if (ret)
 				return ret;
 		}
@@ -782,7 +857,8 @@ static int xpcs_config(struct mdio_xpcs_args *xpcs,
 }
 
 static int xpcs_get_state_c73(struct mdio_xpcs_args *xpcs,
-			      struct phylink_link_state *state)
+			      struct phylink_link_state *state,
+			      const struct xpcs_compat *compat)
 {
 	int ret;
 
@@ -792,7 +868,7 @@ static int xpcs_get_state_c73(struct mdio_xpcs_args *xpcs,
 	/* ... and then we check the faults. */
 	ret = xpcs_read_fault_c73(xpcs, state);
 	if (ret) {
-		ret = xpcs_soft_reset(xpcs);
+		ret = xpcs_soft_reset(xpcs, compat);
 		if (ret)
 			return ret;
 
@@ -801,7 +877,7 @@ static int xpcs_get_state_c73(struct mdio_xpcs_args *xpcs,
 		return xpcs_config(xpcs, state);
 	}
 
-	if (state->an_enabled && xpcs_aneg_done_c73(xpcs, state)) {
+	if (state->an_enabled && xpcs_aneg_done_c73(xpcs, state, compat)) {
 		state->an_complete = true;
 		xpcs_read_lpa_c73(xpcs, state);
 		xpcs_resolve_lpa_c73(xpcs, state);
@@ -858,11 +934,16 @@ static int xpcs_get_state_c37_sgmii(struct mdio_xpcs_args *xpcs,
 static int xpcs_get_state(struct mdio_xpcs_args *xpcs,
 			  struct phylink_link_state *state)
 {
+	const struct xpcs_compat *compat;
 	int ret;
 
-	switch (xpcs->an_mode) {
+	compat = xpcs_find_compat(xpcs->id, state->interface);
+	if (!compat)
+		return -ENODEV;
+
+	switch (compat->an_mode) {
 	case DW_AN_C73:
-		ret = xpcs_get_state_c73(xpcs, state);
+		ret = xpcs_get_state_c73(xpcs, state, compat);
 		if (ret)
 			return ret;
 		break;
@@ -925,55 +1006,25 @@ static u32 xpcs_get_id(struct mdio_xpcs_args *xpcs)
 	return 0xffffffff;
 }
 
-static bool xpcs_check_features(struct mdio_xpcs_args *xpcs,
-				const struct xpcs_id *match,
-				phy_interface_t interface)
-{
-	int i, j;
-
-	for (i = 0; i < DW_XPCS_INTERFACE_MAX; i++) {
-		const struct xpcs_compat *compat = &match->compat[i];
-		bool supports_interface = false;
-
-		for (j = 0; j < compat->num_interfaces; j++) {
-			if (compat->interface[j] == interface) {
-				supports_interface = true;
-				break;
-			}
-		}
-
-		if (!supports_interface)
-			continue;
-
-		/* Populate the supported link modes for this
-		 * PHY interface type
-		 */
-		for (j = 0; compat->supported[j] != __ETHTOOL_LINK_MODE_MASK_NBITS; j++)
-			set_bit(compat->supported[j], xpcs->supported);
-
-		xpcs->an_mode = compat->an_mode;
-
-		return true;
-	}
-
-	return false;
-}
-
 static int xpcs_probe(struct mdio_xpcs_args *xpcs, phy_interface_t interface)
 {
-	const struct xpcs_id *match = NULL;
 	u32 xpcs_id = xpcs_get_id(xpcs);
 	int i;
 
 	for (i = 0; i < ARRAY_SIZE(xpcs_id_list); i++) {
 		const struct xpcs_id *entry = &xpcs_id_list[i];
+		const struct xpcs_compat *compat;
 
-		if ((xpcs_id & entry->mask) == entry->id) {
-			match = entry;
+		if ((xpcs_id & entry->mask) != entry->id)
+			continue;
 
-			if (xpcs_check_features(xpcs, match, interface))
-				return xpcs_soft_reset(xpcs);
-		}
+		xpcs->id = entry;
+
+		compat = xpcs_find_compat(entry, interface);
+		if (!compat)
+			return -ENODEV;
+
+		return xpcs_soft_reset(xpcs, compat);
 	}
 
 	return -ENODEV;
diff --git a/include/linux/pcs/pcs-xpcs.h b/include/linux/pcs/pcs-xpcs.h
index c4d0a2c469c7..c2ec440d2c5d 100644
--- a/include/linux/pcs/pcs-xpcs.h
+++ b/include/linux/pcs/pcs-xpcs.h
@@ -14,11 +14,12 @@
 #define DW_AN_C73			1
 #define DW_AN_C37_SGMII			2
 
+struct xpcs_id;
+
 struct mdio_xpcs_args {
-	__ETHTOOL_DECLARE_LINK_MODE_MASK(supported);
 	struct mii_bus *bus;
+	const struct xpcs_id *id;
 	int addr;
-	int an_mode;
 };
 
 struct mdio_xpcs_ops {
@@ -36,6 +37,7 @@ struct mdio_xpcs_ops {
 			  int enable);
 };
 
+int xpcs_get_an_mode(struct mdio_xpcs_args *xpcs, phy_interface_t interface);
 struct mdio_xpcs_ops *mdio_xpcs_get_ops(void);
 
 #endif /* __LINUX_PCS_XPCS_H */
-- 
2.25.1

[RFC PATCH v2 net-next 4/9] net: pcs: xpcs: export xpcs_validate

[Vladimir Oltean]

From: Vladimir Oltean <vladimir.oltean@nxp.com>

Calling a function pointer with a single implementation through
struct mdio_xpcs_ops is clunky, and the stmmac_do_callback system forces
this to return int, even though it always returns zero.

Simply remove the "validate" function pointer from struct mdio_xpcs_ops
and replace it with an exported xpcs_validate symbol which is called
directly by stmmac.

priv->hw->xpcs is of the type "const struct mdio_xpcs_ops *" and is used
as a placeholder/synonym for priv->plat->mdio_bus_data->has_xpcs. It is
done that way because the mdio_bus_data pointer might or might not be
populated in all stmmac instantiations.

Signed-off-by: Vladimir Oltean <vladimir.oltean@nxp.com>
---
 drivers/net/ethernet/stmicro/stmmac/hwif.h        |  2 --
 drivers/net/ethernet/stmicro/stmmac/stmmac_main.c |  3 ++-
 drivers/net/pcs/pcs-xpcs.c                        | 11 ++++-------
 include/linux/pcs/pcs-xpcs.h                      |  5 ++---
 4 files changed, 8 insertions(+), 13 deletions(-)

diff --git a/drivers/net/ethernet/stmicro/stmmac/hwif.h b/drivers/net/ethernet/stmicro/stmmac/hwif.h
index 75a8b90c202a..a86b358feae9 100644
--- a/drivers/net/ethernet/stmicro/stmmac/hwif.h
+++ b/drivers/net/ethernet/stmicro/stmmac/hwif.h
@@ -613,8 +613,6 @@ struct stmmac_mmc_ops {
 	stmmac_do_void_callback(__priv, mmc, read, __args)
 
 /* XPCS callbacks */
-#define stmmac_xpcs_validate(__priv, __args...) \
-	stmmac_do_callback(__priv, xpcs, validate, __args)
 #define stmmac_xpcs_config(__priv, __args...) \
 	stmmac_do_callback(__priv, xpcs, config, __args)
 #define stmmac_xpcs_get_state(__priv, __args...) \
diff --git a/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c b/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c
index 129b103cd2b5..9f72e4dd1457 100644
--- a/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c
+++ b/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c
@@ -996,7 +996,8 @@ static void stmmac_validate(struct phylink_config *config,
 	linkmode_andnot(state->advertising, state->advertising, mask);
 
 	/* If PCS is supported, check which modes it supports. */
-	stmmac_xpcs_validate(priv, &priv->hw->xpcs_args, supported, state);
+	if (priv->hw->xpcs)
+		xpcs_validate(&priv->hw->xpcs_args, supported, state);
 }
 
 static void stmmac_mac_pcs_get_state(struct phylink_config *config,
diff --git a/drivers/net/pcs/pcs-xpcs.c b/drivers/net/pcs/pcs-xpcs.c
index 4bc63d7e3bda..a9bae263dcdb 100644
--- a/drivers/net/pcs/pcs-xpcs.c
+++ b/drivers/net/pcs/pcs-xpcs.c
@@ -720,9 +720,8 @@ static void xpcs_resolve_pma(struct mdio_xpcs_args *xpcs,
 	}
 }
 
-static int xpcs_validate(struct mdio_xpcs_args *xpcs,
-			 unsigned long *supported,
-			 struct phylink_link_state *state)
+void xpcs_validate(struct mdio_xpcs_args *xpcs, unsigned long *supported,
+		   struct phylink_link_state *state)
 {
 	__ETHTOOL_DECLARE_LINK_MODE_MASK(xpcs_supported);
 	const struct xpcs_compat *compat;
@@ -733,7 +732,7 @@ static int xpcs_validate(struct mdio_xpcs_args *xpcs,
 	 * advertising masks and exit.
 	 */
 	if (state->interface == PHY_INTERFACE_MODE_NA)
-		return 0;
+		return;
 
 	bitmap_zero(xpcs_supported, __ETHTOOL_LINK_MODE_MASK_NBITS);
 
@@ -748,9 +747,8 @@ static int xpcs_validate(struct mdio_xpcs_args *xpcs,
 
 	linkmode_and(supported, supported, xpcs_supported);
 	linkmode_and(state->advertising, state->advertising, xpcs_supported);
-
-	return 0;
 }
+EXPORT_SYMBOL_GPL(xpcs_validate);
 
 static int xpcs_config_eee(struct mdio_xpcs_args *xpcs, int mult_fact_100ns,
 			   int enable)
@@ -1031,7 +1029,6 @@ static int xpcs_probe(struct mdio_xpcs_args *xpcs, phy_interface_t interface)
 }
 
 static struct mdio_xpcs_ops xpcs_ops = {
-	.validate = xpcs_validate,
 	.config = xpcs_config,
 	.get_state = xpcs_get_state,
 	.link_up = xpcs_link_up,
diff --git a/include/linux/pcs/pcs-xpcs.h b/include/linux/pcs/pcs-xpcs.h
index c2ec440d2c5d..5ec9aaca01fe 100644
--- a/include/linux/pcs/pcs-xpcs.h
+++ b/include/linux/pcs/pcs-xpcs.h
@@ -23,9 +23,6 @@ struct mdio_xpcs_args {
 };
 
 struct mdio_xpcs_ops {
-	int (*validate)(struct mdio_xpcs_args *xpcs,
-			unsigned long *supported,
-			struct phylink_link_state *state);
 	int (*config)(struct mdio_xpcs_args *xpcs,
 		      const struct phylink_link_state *state);
 	int (*get_state)(struct mdio_xpcs_args *xpcs,
@@ -39,5 +36,7 @@ struct mdio_xpcs_ops {
 
 int xpcs_get_an_mode(struct mdio_xpcs_args *xpcs, phy_interface_t interface);
 struct mdio_xpcs_ops *mdio_xpcs_get_ops(void);
+void xpcs_validate(struct mdio_xpcs_args *xpcs, unsigned long *supported,
+		   struct phylink_link_state *state);
 
 #endif /* __LINUX_PCS_XPCS_H */
-- 
2.25.1

[RFC PATCH v2 net-next 5/9] net: pcs: xpcs: export xpcs_config_eee

[Vladimir Oltean]

From: Vladimir Oltean <vladimir.oltean@nxp.com>

There is no good reason why we need to go through:

stmmac_xpcs_config_eee
-> stmmac_do_callback
   -> mdio_xpcs_ops->config_eee
      -> xpcs_config_eee

when we can simply call xpcs_config_eee.

priv->hw->xpcs is of the type "const struct mdio_xpcs_ops *" and is used
as a placeholder/synonym for priv->plat->mdio_bus_data->has_xpcs. It is
done that way because the mdio_bus_data pointer might or might not be
populated in all stmmac instantiations.

Signed-off-by: Vladimir Oltean <vladimir.oltean@nxp.com>
---
 drivers/net/ethernet/stmicro/stmmac/hwif.h           |  2 --
 drivers/net/ethernet/stmicro/stmmac/stmmac_ethtool.c | 12 +++++++-----
 drivers/net/pcs/pcs-xpcs.c                           |  6 +++---
 include/linux/pcs/pcs-xpcs.h                         |  4 ++--
 4 files changed, 12 insertions(+), 12 deletions(-)

diff --git a/drivers/net/ethernet/stmicro/stmmac/hwif.h b/drivers/net/ethernet/stmicro/stmmac/hwif.h
index a86b358feae9..2d2843edaf21 100644
--- a/drivers/net/ethernet/stmicro/stmmac/hwif.h
+++ b/drivers/net/ethernet/stmicro/stmmac/hwif.h
@@ -621,8 +621,6 @@ struct stmmac_mmc_ops {
 	stmmac_do_callback(__priv, xpcs, link_up, __args)
 #define stmmac_xpcs_probe(__priv, __args...) \
 	stmmac_do_callback(__priv, xpcs, probe, __args)
-#define stmmac_xpcs_config_eee(__priv, __args...) \
-	stmmac_do_callback(__priv, xpcs, config_eee, __args)
 
 struct stmmac_regs_off {
 	u32 ptp_off;
diff --git a/drivers/net/ethernet/stmicro/stmmac/stmmac_ethtool.c b/drivers/net/ethernet/stmicro/stmmac/stmmac_ethtool.c
index 1f6d749fd9a3..ba7d0f40723a 100644
--- a/drivers/net/ethernet/stmicro/stmmac/stmmac_ethtool.c
+++ b/drivers/net/ethernet/stmicro/stmmac/stmmac_ethtool.c
@@ -720,11 +720,13 @@ static int stmmac_ethtool_op_set_eee(struct net_device *dev,
 		netdev_warn(priv->dev,
 			    "Setting EEE tx-lpi is not supported\n");
 
-	ret = stmmac_xpcs_config_eee(priv, &priv->hw->xpcs_args,
-				     priv->plat->mult_fact_100ns,
-				     edata->eee_enabled);
-	if (ret)
-		return ret;
+	if (priv->hw->xpcs) {
+		ret = xpcs_config_eee(&priv->hw->xpcs_args,
+				      priv->plat->mult_fact_100ns,
+				      edata->eee_enabled);
+		if (ret)
+			return ret;
+	}
 
 	if (!edata->eee_enabled)
 		stmmac_disable_eee_mode(priv);
diff --git a/drivers/net/pcs/pcs-xpcs.c b/drivers/net/pcs/pcs-xpcs.c
index a9bae263dcdb..7cd057f43200 100644
--- a/drivers/net/pcs/pcs-xpcs.c
+++ b/drivers/net/pcs/pcs-xpcs.c
@@ -750,8 +750,8 @@ void xpcs_validate(struct mdio_xpcs_args *xpcs, unsigned long *supported,
 }
 EXPORT_SYMBOL_GPL(xpcs_validate);
 
-static int xpcs_config_eee(struct mdio_xpcs_args *xpcs, int mult_fact_100ns,
-			   int enable)
+int xpcs_config_eee(struct mdio_xpcs_args *xpcs, int mult_fact_100ns,
+		    int enable)
 {
 	int ret;
 
@@ -782,6 +782,7 @@ static int xpcs_config_eee(struct mdio_xpcs_args *xpcs, int mult_fact_100ns,
 	ret |= DW_VR_MII_EEE_TRN_LPI;
 	return xpcs_write(xpcs, MDIO_MMD_VEND2, DW_VR_MII_EEE_MCTRL1, ret);
 }
+EXPORT_SYMBOL_GPL(xpcs_config_eee);
 
 static int xpcs_config_aneg_c37_sgmii(struct mdio_xpcs_args *xpcs)
 {
@@ -1033,7 +1034,6 @@ static struct mdio_xpcs_ops xpcs_ops = {
 	.get_state = xpcs_get_state,
 	.link_up = xpcs_link_up,
 	.probe = xpcs_probe,
-	.config_eee = xpcs_config_eee,
 };
 
 struct mdio_xpcs_ops *mdio_xpcs_get_ops(void)
diff --git a/include/linux/pcs/pcs-xpcs.h b/include/linux/pcs/pcs-xpcs.h
index 5ec9aaca01fe..ae74a336dcb9 100644
--- a/include/linux/pcs/pcs-xpcs.h
+++ b/include/linux/pcs/pcs-xpcs.h
@@ -30,13 +30,13 @@ struct mdio_xpcs_ops {
 	int (*link_up)(struct mdio_xpcs_args *xpcs, int speed,
 		       phy_interface_t interface);
 	int (*probe)(struct mdio_xpcs_args *xpcs, phy_interface_t interface);
-	int (*config_eee)(struct mdio_xpcs_args *xpcs, int mult_fact_100ns,
-			  int enable);
 };
 
 int xpcs_get_an_mode(struct mdio_xpcs_args *xpcs, phy_interface_t interface);
 struct mdio_xpcs_ops *mdio_xpcs_get_ops(void);
 void xpcs_validate(struct mdio_xpcs_args *xpcs, unsigned long *supported,
 		   struct phylink_link_state *state);
+int xpcs_config_eee(struct mdio_xpcs_args *xpcs, int mult_fact_100ns,
+		    int enable);
 
 #endif /* __LINUX_PCS_XPCS_H */
-- 
2.25.1

[RFC PATCH v2 net-next 6/9] net: pcs: xpcs: export xpcs_probe

[Vladimir Oltean]

From: Vladimir Oltean <vladimir.oltean@nxp.com>

Similar to the other recently functions, it is not necessary for
xpcs_probe to be a function pointer, so export it so that it can be
called directly.

Signed-off-by: Vladimir Oltean <vladimir.oltean@nxp.com>
---
 drivers/net/ethernet/stmicro/stmmac/hwif.h        | 2 --
 drivers/net/ethernet/stmicro/stmmac/stmmac_mdio.c | 2 +-
 drivers/net/pcs/pcs-xpcs.c                        | 4 ++--
 include/linux/pcs/pcs-xpcs.h                      | 2 +-
 4 files changed, 4 insertions(+), 6 deletions(-)

diff --git a/drivers/net/ethernet/stmicro/stmmac/hwif.h b/drivers/net/ethernet/stmicro/stmmac/hwif.h
index 2d2843edaf21..5014b260844b 100644
--- a/drivers/net/ethernet/stmicro/stmmac/hwif.h
+++ b/drivers/net/ethernet/stmicro/stmmac/hwif.h
@@ -619,8 +619,6 @@ struct stmmac_mmc_ops {
 	stmmac_do_callback(__priv, xpcs, get_state, __args)
 #define stmmac_xpcs_link_up(__priv, __args...) \
 	stmmac_do_callback(__priv, xpcs, link_up, __args)
-#define stmmac_xpcs_probe(__priv, __args...) \
-	stmmac_do_callback(__priv, xpcs, probe, __args)
 
 struct stmmac_regs_off {
 	u32 ptp_off;
diff --git a/drivers/net/ethernet/stmicro/stmmac/stmmac_mdio.c b/drivers/net/ethernet/stmicro/stmmac/stmmac_mdio.c
index e293bf1ce9f3..56deb92a8430 100644
--- a/drivers/net/ethernet/stmicro/stmmac/stmmac_mdio.c
+++ b/drivers/net/ethernet/stmicro/stmmac/stmmac_mdio.c
@@ -521,7 +521,7 @@ int stmmac_mdio_register(struct net_device *ndev)
 		for (addr = 0; addr < max_addr; addr++) {
 			xpcs->addr = addr;
 
-			ret = stmmac_xpcs_probe(priv, xpcs, mode);
+			ret = xpcs_probe(xpcs, mode);
 			if (!ret) {
 				found = 1;
 				break;
diff --git a/drivers/net/pcs/pcs-xpcs.c b/drivers/net/pcs/pcs-xpcs.c
index 7cd057f43200..964433849ef8 100644
--- a/drivers/net/pcs/pcs-xpcs.c
+++ b/drivers/net/pcs/pcs-xpcs.c
@@ -1005,7 +1005,7 @@ static u32 xpcs_get_id(struct mdio_xpcs_args *xpcs)
 	return 0xffffffff;
 }
 
-static int xpcs_probe(struct mdio_xpcs_args *xpcs, phy_interface_t interface)
+int xpcs_probe(struct mdio_xpcs_args *xpcs, phy_interface_t interface)
 {
 	u32 xpcs_id = xpcs_get_id(xpcs);
 	int i;
@@ -1028,12 +1028,12 @@ static int xpcs_probe(struct mdio_xpcs_args *xpcs, phy_interface_t interface)
 
 	return -ENODEV;
 }
+EXPORT_SYMBOL_GPL(xpcs_probe);
 
 static struct mdio_xpcs_ops xpcs_ops = {
 	.config = xpcs_config,
 	.get_state = xpcs_get_state,
 	.link_up = xpcs_link_up,
-	.probe = xpcs_probe,
 };
 
 struct mdio_xpcs_ops *mdio_xpcs_get_ops(void)
diff --git a/include/linux/pcs/pcs-xpcs.h b/include/linux/pcs/pcs-xpcs.h
index ae74a336dcb9..1d8581b74d81 100644
--- a/include/linux/pcs/pcs-xpcs.h
+++ b/include/linux/pcs/pcs-xpcs.h
@@ -29,7 +29,6 @@ struct mdio_xpcs_ops {
 			 struct phylink_link_state *state);
 	int (*link_up)(struct mdio_xpcs_args *xpcs, int speed,
 		       phy_interface_t interface);
-	int (*probe)(struct mdio_xpcs_args *xpcs, phy_interface_t interface);
 };
 
 int xpcs_get_an_mode(struct mdio_xpcs_args *xpcs, phy_interface_t interface);
@@ -38,5 +37,6 @@ void xpcs_validate(struct mdio_xpcs_args *xpcs, unsigned long *supported,
 		   struct phylink_link_state *state);
 int xpcs_config_eee(struct mdio_xpcs_args *xpcs, int mult_fact_100ns,
 		    int enable);
+int xpcs_probe(struct mdio_xpcs_args *xpcs, phy_interface_t interface);
 
 #endif /* __LINUX_PCS_XPCS_H */
-- 
2.25.1

[RFC PATCH v2 net-next 7/9] net: pcs: xpcs: use mdiobus_c45_addr in xpcs_{read,write}

[Vladimir Oltean]

From: Vladimir Oltean <vladimir.oltean@nxp.com>

Use the dedicated helper for abstracting away how the clause 45 address
is packed in reg_addr.

Signed-off-by: Vladimir Oltean <vladimir.oltean@nxp.com>
---
 drivers/net/pcs/pcs-xpcs.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/drivers/net/pcs/pcs-xpcs.c b/drivers/net/pcs/pcs-xpcs.c
index 964433849ef8..c3760192fe20 100644
--- a/drivers/net/pcs/pcs-xpcs.c
+++ b/drivers/net/pcs/pcs-xpcs.c
@@ -275,14 +275,14 @@ static bool __xpcs_linkmode_supported(const struct xpcs_compat *compat,
 
 static int xpcs_read(struct mdio_xpcs_args *xpcs, int dev, u32 reg)
 {
-	u32 reg_addr = MII_ADDR_C45 | dev << 16 | reg;
+	u32 reg_addr = mdiobus_c45_addr(dev, reg);
 
 	return mdiobus_read(xpcs->bus, xpcs->addr, reg_addr);
 }
 
 static int xpcs_write(struct mdio_xpcs_args *xpcs, int dev, u32 reg, u16 val)
 {
-	u32 reg_addr = MII_ADDR_C45 | dev << 16 | reg;
+	u32 reg_addr = mdiobus_c45_addr(dev, reg);
 
 	return mdiobus_write(xpcs->bus, xpcs->addr, reg_addr, val);
 }
-- 
2.25.1

[RFC PATCH v2 net-next 8/9] net: pcs: xpcs: convert to mdio_device

[Vladimir Oltean]

From: Vladimir Oltean <vladimir.oltean@nxp.com>

Unify the 2 existing PCS drivers (lynx and xpcs) by doing a similar
thing on probe, which is to have a *_create function that takes a
struct mdio_device * given by the caller, and builds a private PCS
structure around that.

This changes stmmac to hold only a pointer to the xpcs, as opposed to
the full structure. This will be used in the next patch when struct
mdio_xpcs_ops is removed. Currently a pointer to struct mdio_xpcs_ops
is used as a shorthand to determine whether the port has an XPCS or not.
We can do the same now with the mdio_xpcs_args pointer.

Signed-off-by: Vladimir Oltean <vladimir.oltean@nxp.com>
---
 drivers/net/ethernet/stmicro/stmmac/common.h  |  2 +-
 .../ethernet/stmicro/stmmac/stmmac_ethtool.c  |  2 +-
 .../net/ethernet/stmicro/stmmac/stmmac_main.c | 10 ++--
 .../net/ethernet/stmicro/stmmac/stmmac_mdio.c | 39 ++++++++------
 drivers/net/pcs/pcs-xpcs.c                    | 53 +++++++++++++++----
 include/linux/pcs/pcs-xpcs.h                  |  7 +--
 6 files changed, 76 insertions(+), 37 deletions(-)

diff --git a/drivers/net/ethernet/stmicro/stmmac/common.h b/drivers/net/ethernet/stmicro/stmmac/common.h
index 619e3c0760d6..4bcd1d340766 100644
--- a/drivers/net/ethernet/stmicro/stmmac/common.h
+++ b/drivers/net/ethernet/stmicro/stmmac/common.h
@@ -504,7 +504,7 @@ struct mac_device_info {
 	const struct stmmac_tc_ops *tc;
 	const struct stmmac_mmc_ops *mmc;
 	const struct mdio_xpcs_ops *xpcs;
-	struct mdio_xpcs_args xpcs_args;
+	struct mdio_xpcs_args *xpcs_args;
 	struct mii_regs mii;	/* MII register Addresses */
 	struct mac_link link;
 	void __iomem *pcsr;     /* vpointer to device CSRs */
diff --git a/drivers/net/ethernet/stmicro/stmmac/stmmac_ethtool.c b/drivers/net/ethernet/stmicro/stmmac/stmmac_ethtool.c
index ba7d0f40723a..050576ee704d 100644
--- a/drivers/net/ethernet/stmicro/stmmac/stmmac_ethtool.c
+++ b/drivers/net/ethernet/stmicro/stmmac/stmmac_ethtool.c
@@ -721,7 +721,7 @@ static int stmmac_ethtool_op_set_eee(struct net_device *dev,
 			    "Setting EEE tx-lpi is not supported\n");
 
 	if (priv->hw->xpcs) {
-		ret = xpcs_config_eee(&priv->hw->xpcs_args,
+		ret = xpcs_config_eee(priv->hw->xpcs_args,
 				      priv->plat->mult_fact_100ns,
 				      edata->eee_enabled);
 		if (ret)
diff --git a/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c b/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c
index 9f72e4dd1457..426c8f891f5a 100644
--- a/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c
+++ b/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c
@@ -997,7 +997,7 @@ static void stmmac_validate(struct phylink_config *config,
 
 	/* If PCS is supported, check which modes it supports. */
 	if (priv->hw->xpcs)
-		xpcs_validate(&priv->hw->xpcs_args, supported, state);
+		xpcs_validate(priv->hw->xpcs_args, supported, state);
 }
 
 static void stmmac_mac_pcs_get_state(struct phylink_config *config,
@@ -1006,7 +1006,7 @@ static void stmmac_mac_pcs_get_state(struct phylink_config *config,
 	struct stmmac_priv *priv = netdev_priv(to_net_dev(config->dev));
 
 	state->link = 0;
-	stmmac_xpcs_get_state(priv, &priv->hw->xpcs_args, state);
+	stmmac_xpcs_get_state(priv, priv->hw->xpcs_args, state);
 }
 
 static void stmmac_mac_config(struct phylink_config *config, unsigned int mode,
@@ -1014,7 +1014,7 @@ static void stmmac_mac_config(struct phylink_config *config, unsigned int mode,
 {
 	struct stmmac_priv *priv = netdev_priv(to_net_dev(config->dev));
 
-	stmmac_xpcs_config(priv, &priv->hw->xpcs_args, state);
+	stmmac_xpcs_config(priv, priv->hw->xpcs_args, state);
 }
 
 static void stmmac_mac_an_restart(struct phylink_config *config)
@@ -1061,7 +1061,7 @@ static void stmmac_mac_link_up(struct phylink_config *config,
 	struct stmmac_priv *priv = netdev_priv(to_net_dev(config->dev));
 	u32 ctrl;
 
-	stmmac_xpcs_link_up(priv, &priv->hw->xpcs_args, speed, interface);
+	stmmac_xpcs_link_up(priv, priv->hw->xpcs_args, speed, interface);
 
 	ctrl = readl(priv->ioaddr + MAC_CTRL_REG);
 	ctrl &= ~priv->hw->link.speed_mask;
@@ -3652,7 +3652,7 @@ int stmmac_open(struct net_device *dev)
 	if (priv->hw->pcs != STMMAC_PCS_TBI &&
 	    priv->hw->pcs != STMMAC_PCS_RTBI &&
 	    (!priv->hw->xpcs ||
-	     xpcs_get_an_mode(&priv->hw->xpcs_args, mode) != DW_AN_C73)) {
+	     xpcs_get_an_mode(priv->hw->xpcs_args, mode) != DW_AN_C73)) {
 		ret = stmmac_init_phy(dev);
 		if (ret) {
 			netdev_err(priv->dev,
diff --git a/drivers/net/ethernet/stmicro/stmmac/stmmac_mdio.c b/drivers/net/ethernet/stmicro/stmmac/stmmac_mdio.c
index 56deb92a8430..9b4bf78d2eaa 100644
--- a/drivers/net/ethernet/stmicro/stmmac/stmmac_mdio.c
+++ b/drivers/net/ethernet/stmicro/stmmac/stmmac_mdio.c
@@ -510,25 +510,27 @@ int stmmac_mdio_register(struct net_device *ndev)
 	}
 
 	/* Try to probe the XPCS by scanning all addresses. */
-	if (priv->hw->xpcs) {
-		struct mdio_xpcs_args *xpcs = &priv->hw->xpcs_args;
-		int ret, mode = priv->plat->phy_interface;
-		max_addr = PHY_MAX_ADDR;
-
-		xpcs->bus = new_bus;
-
-		found = 0;
-		for (addr = 0; addr < max_addr; addr++) {
-			xpcs->addr = addr;
-
-			ret = xpcs_probe(xpcs, mode);
-			if (!ret) {
-				found = 1;
-				break;
+	if (mdio_bus_data->has_xpcs) {
+		int mode = priv->plat->phy_interface;
+		struct mdio_device *mdiodev;
+		struct mdio_xpcs_args *xpcs;
+
+		for (addr = 0; addr < PHY_MAX_ADDR; addr++) {
+			mdiodev = mdio_device_create(new_bus, addr);
+			if (IS_ERR(mdiodev))
+				continue;
+
+			xpcs = xpcs_create(mdiodev, mode);
+			if (IS_ERR_OR_NULL(xpcs)) {
+				mdio_device_free(mdiodev);
+				continue;
 			}
+
+			priv->hw->xpcs_args = xpcs;
+			break;
 		}
 
-		if (!found && !mdio_node) {
+		if (!priv->hw->xpcs_args) {
 			dev_warn(dev, "No XPCS found\n");
 			err = -ENODEV;
 			goto no_xpcs_found;
@@ -560,6 +562,11 @@ int stmmac_mdio_unregister(struct net_device *ndev)
 	if (!priv->mii)
 		return 0;
 
+	if (priv->hw->xpcs) {
+		mdio_device_free(priv->hw->xpcs_args->mdiodev);
+		xpcs_destroy(priv->hw->xpcs_args);
+	}
+
 	mdiobus_unregister(priv->mii);
 	priv->mii->priv = NULL;
 	mdiobus_free(priv->mii);
diff --git a/drivers/net/pcs/pcs-xpcs.c b/drivers/net/pcs/pcs-xpcs.c
index c3760192fe20..3e09850b8318 100644
--- a/drivers/net/pcs/pcs-xpcs.c
+++ b/drivers/net/pcs/pcs-xpcs.c
@@ -276,15 +276,19 @@ static bool __xpcs_linkmode_supported(const struct xpcs_compat *compat,
 static int xpcs_read(struct mdio_xpcs_args *xpcs, int dev, u32 reg)
 {
 	u32 reg_addr = mdiobus_c45_addr(dev, reg);
+	struct mii_bus *bus = xpcs->mdiodev->bus;
+	int addr = xpcs->mdiodev->addr;
 
-	return mdiobus_read(xpcs->bus, xpcs->addr, reg_addr);
+	return mdiobus_read(bus, addr, reg_addr);
 }
 
 static int xpcs_write(struct mdio_xpcs_args *xpcs, int dev, u32 reg, u16 val)
 {
 	u32 reg_addr = mdiobus_c45_addr(dev, reg);
+	struct mii_bus *bus = xpcs->mdiodev->bus;
+	int addr = xpcs->mdiodev->addr;
 
-	return mdiobus_write(xpcs->bus, xpcs->addr, reg_addr, val);
+	return mdiobus_write(bus, addr, reg_addr, val);
 }
 
 static int xpcs_read_vendor(struct mdio_xpcs_args *xpcs, int dev, u32 reg)
@@ -350,7 +354,7 @@ static int xpcs_soft_reset(struct mdio_xpcs_args *xpcs,
 #define xpcs_warn(__xpcs, __state, __args...) \
 ({ \
 	if ((__state)->link) \
-		dev_warn(&(__xpcs)->bus->dev, ##__args); \
+		dev_warn(&(__xpcs)->mdiodev->dev, ##__args); \
 })
 
 static int xpcs_read_fault_c73(struct mdio_xpcs_args *xpcs,
@@ -1005,10 +1009,20 @@ static u32 xpcs_get_id(struct mdio_xpcs_args *xpcs)
 	return 0xffffffff;
 }
 
-int xpcs_probe(struct mdio_xpcs_args *xpcs, phy_interface_t interface)
+struct mdio_xpcs_args *xpcs_create(struct mdio_device *mdiodev,
+				   phy_interface_t interface)
 {
-	u32 xpcs_id = xpcs_get_id(xpcs);
-	int i;
+	struct mdio_xpcs_args *xpcs;
+	u32 xpcs_id;
+	int i, ret;
+
+	xpcs = kzalloc(sizeof(*xpcs), GFP_KERNEL);
+	if (!xpcs)
+		return NULL;
+
+	xpcs->mdiodev = mdiodev;
+
+	xpcs_id = xpcs_get_id(xpcs);
 
 	for (i = 0; i < ARRAY_SIZE(xpcs_id_list); i++) {
 		const struct xpcs_id *entry = &xpcs_id_list[i];
@@ -1020,15 +1034,32 @@ int xpcs_probe(struct mdio_xpcs_args *xpcs, phy_interface_t interface)
 		xpcs->id = entry;
 
 		compat = xpcs_find_compat(entry, interface);
-		if (!compat)
-			return -ENODEV;
+		if (!compat) {
+			ret = -ENODEV;
+			goto out;
+		}
 
-		return xpcs_soft_reset(xpcs, compat);
+		ret = xpcs_soft_reset(xpcs, compat);
+		if (ret)
+			goto out;
+
+		return xpcs;
 	}
 
-	return -ENODEV;
+	ret = -ENODEV;
+
+out:
+	kfree(xpcs);
+
+	return ERR_PTR(ret);
+}
+EXPORT_SYMBOL_GPL(xpcs_create);
+
+void xpcs_destroy(struct mdio_xpcs_args *xpcs)
+{
+	kfree(xpcs);
 }
-EXPORT_SYMBOL_GPL(xpcs_probe);
+EXPORT_SYMBOL_GPL(xpcs_destroy);
 
 static struct mdio_xpcs_ops xpcs_ops = {
 	.config = xpcs_config,
diff --git a/include/linux/pcs/pcs-xpcs.h b/include/linux/pcs/pcs-xpcs.h
index 1d8581b74d81..57a199393d63 100644
--- a/include/linux/pcs/pcs-xpcs.h
+++ b/include/linux/pcs/pcs-xpcs.h
@@ -17,9 +17,8 @@
 struct xpcs_id;
 
 struct mdio_xpcs_args {
-	struct mii_bus *bus;
+	struct mdio_device *mdiodev;
 	const struct xpcs_id *id;
-	int addr;
 };
 
 struct mdio_xpcs_ops {
@@ -37,6 +36,8 @@ void xpcs_validate(struct mdio_xpcs_args *xpcs, unsigned long *supported,
 		   struct phylink_link_state *state);
 int xpcs_config_eee(struct mdio_xpcs_args *xpcs, int mult_fact_100ns,
 		    int enable);
-int xpcs_probe(struct mdio_xpcs_args *xpcs, phy_interface_t interface);
+struct mdio_xpcs_args *xpcs_create(struct mdio_device *mdiodev,
+				   phy_interface_t interface);
+void xpcs_destroy(struct mdio_xpcs_args *xpcs);
 
 #endif /* __LINUX_PCS_XPCS_H */
-- 
2.25.1

[RFC PATCH v2 net-next 9/9] net: pcs: xpcs: convert to phylink_pcs_ops

[Vladimir Oltean]

From: Vladimir Oltean <vladimir.oltean@nxp.com>

Since all the remaining members of struct mdio_xpcs_ops have direct
equivalents in struct phylink_pcs_ops, it is about time we remove it
altogether.

Since the phylink ops return void, we need to remove the error
propagation from the various xpcs methods and simply print an error
message where appropriate.

Since xpcs_get_state_c73() detects link faults and attempts to reset the
link on its own by calling xpcs_config(), but xpcs_config() now has a
lot of phylink arguments which are not needed and cannot be simply
fabricated by anybody else except phylink, the actual implementation has
been moved into a smaller xpcs_do_config().

The const struct mdio_xpcs_ops *priv->hw->xpcs has been removed, so we
need to look at the struct mdio_xpcs_args pointer now as an indication
whether the port has an XPCS or not.

Signed-off-by: Vladimir Oltean <vladimir.oltean@nxp.com>
---
 drivers/net/ethernet/stmicro/stmmac/common.h  |  3 +-
 drivers/net/ethernet/stmicro/stmmac/hwif.h    |  8 --
 .../ethernet/stmicro/stmmac/stmmac_ethtool.c  |  2 +-
 .../net/ethernet/stmicro/stmmac/stmmac_main.c | 41 ++------
 .../net/ethernet/stmicro/stmmac/stmmac_mdio.c | 16 +--
 drivers/net/pcs/pcs-xpcs.c                    | 99 +++++++++++--------
 include/linux/pcs/pcs-xpcs.h                  | 11 +--
 7 files changed, 77 insertions(+), 103 deletions(-)

diff --git a/drivers/net/ethernet/stmicro/stmmac/common.h b/drivers/net/ethernet/stmicro/stmmac/common.h
index 4bcd1d340766..8a83f9e1e95b 100644
--- a/drivers/net/ethernet/stmicro/stmmac/common.h
+++ b/drivers/net/ethernet/stmicro/stmmac/common.h
@@ -503,8 +503,7 @@ struct mac_device_info {
 	const struct stmmac_hwtimestamp *ptp;
 	const struct stmmac_tc_ops *tc;
 	const struct stmmac_mmc_ops *mmc;
-	const struct mdio_xpcs_ops *xpcs;
-	struct mdio_xpcs_args *xpcs_args;
+	struct mdio_xpcs_args *xpcs;
 	struct mii_regs mii;	/* MII register Addresses */
 	struct mac_link link;
 	void __iomem *pcsr;     /* vpointer to device CSRs */
diff --git a/drivers/net/ethernet/stmicro/stmmac/hwif.h b/drivers/net/ethernet/stmicro/stmmac/hwif.h
index 5014b260844b..91f7592a0189 100644
--- a/drivers/net/ethernet/stmicro/stmmac/hwif.h
+++ b/drivers/net/ethernet/stmicro/stmmac/hwif.h
@@ -612,14 +612,6 @@ struct stmmac_mmc_ops {
 #define stmmac_mmc_read(__priv, __args...) \
 	stmmac_do_void_callback(__priv, mmc, read, __args)
 
-/* XPCS callbacks */
-#define stmmac_xpcs_config(__priv, __args...) \
-	stmmac_do_callback(__priv, xpcs, config, __args)
-#define stmmac_xpcs_get_state(__priv, __args...) \
-	stmmac_do_callback(__priv, xpcs, get_state, __args)
-#define stmmac_xpcs_link_up(__priv, __args...) \
-	stmmac_do_callback(__priv, xpcs, link_up, __args)
-
 struct stmmac_regs_off {
 	u32 ptp_off;
 	u32 mmc_off;
diff --git a/drivers/net/ethernet/stmicro/stmmac/stmmac_ethtool.c b/drivers/net/ethernet/stmicro/stmmac/stmmac_ethtool.c
index 050576ee704d..d0ce608b81c3 100644
--- a/drivers/net/ethernet/stmicro/stmmac/stmmac_ethtool.c
+++ b/drivers/net/ethernet/stmicro/stmmac/stmmac_ethtool.c
@@ -721,7 +721,7 @@ static int stmmac_ethtool_op_set_eee(struct net_device *dev,
 			    "Setting EEE tx-lpi is not supported\n");
 
 	if (priv->hw->xpcs) {
-		ret = xpcs_config_eee(priv->hw->xpcs_args,
+		ret = xpcs_config_eee(priv->hw->xpcs,
 				      priv->plat->mult_fact_100ns,
 				      edata->eee_enabled);
 		if (ret)
diff --git a/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c b/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c
index 426c8f891f5a..d5685a74f3b7 100644
--- a/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c
+++ b/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c
@@ -997,29 +997,7 @@ static void stmmac_validate(struct phylink_config *config,
 
 	/* If PCS is supported, check which modes it supports. */
 	if (priv->hw->xpcs)
-		xpcs_validate(priv->hw->xpcs_args, supported, state);
-}
-
-static void stmmac_mac_pcs_get_state(struct phylink_config *config,
-				     struct phylink_link_state *state)
-{
-	struct stmmac_priv *priv = netdev_priv(to_net_dev(config->dev));
-
-	state->link = 0;
-	stmmac_xpcs_get_state(priv, priv->hw->xpcs_args, state);
-}
-
-static void stmmac_mac_config(struct phylink_config *config, unsigned int mode,
-			      const struct phylink_link_state *state)
-{
-	struct stmmac_priv *priv = netdev_priv(to_net_dev(config->dev));
-
-	stmmac_xpcs_config(priv, priv->hw->xpcs_args, state);
-}
-
-static void stmmac_mac_an_restart(struct phylink_config *config)
-{
-	/* Not Supported */
+		xpcs_validate(priv->hw->xpcs, supported, state);
 }
 
 static void stmmac_fpe_link_state_handle(struct stmmac_priv *priv, bool is_up)
@@ -1061,8 +1039,6 @@ static void stmmac_mac_link_up(struct phylink_config *config,
 	struct stmmac_priv *priv = netdev_priv(to_net_dev(config->dev));
 	u32 ctrl;
 
-	stmmac_xpcs_link_up(priv, priv->hw->xpcs_args, speed, interface);
-
 	ctrl = readl(priv->ioaddr + MAC_CTRL_REG);
 	ctrl &= ~priv->hw->link.speed_mask;
 
@@ -1155,9 +1131,6 @@ static void stmmac_mac_link_up(struct phylink_config *config,
 
 static const struct phylink_mac_ops stmmac_phylink_mac_ops = {
 	.validate = stmmac_validate,
-	.mac_pcs_get_state = stmmac_mac_pcs_get_state,
-	.mac_config = stmmac_mac_config,
-	.mac_an_restart = stmmac_mac_an_restart,
 	.mac_link_down = stmmac_mac_link_down,
 	.mac_link_up = stmmac_mac_link_up,
 };
@@ -1234,6 +1207,7 @@ static int stmmac_init_phy(struct net_device *dev)
 
 static int stmmac_phy_setup(struct stmmac_priv *priv)
 {
+	struct stmmac_mdio_bus_data *mdio_bus_data = priv->plat->mdio_bus_data;
 	struct fwnode_handle *fwnode = of_fwnode_handle(priv->plat->phylink_node);
 	int mode = priv->plat->phy_interface;
 	struct phylink *phylink;
@@ -1241,8 +1215,7 @@ static int stmmac_phy_setup(struct stmmac_priv *priv)
 	priv->phylink_config.dev = &priv->dev->dev;
 	priv->phylink_config.type = PHYLINK_NETDEV;
 	priv->phylink_config.pcs_poll = true;
-	priv->phylink_config.ovr_an_inband =
-		priv->plat->mdio_bus_data->xpcs_an_inband;
+	priv->phylink_config.ovr_an_inband = mdio_bus_data->xpcs_an_inband;
 
 	if (!fwnode)
 		fwnode = dev_fwnode(priv->device);
@@ -1252,6 +1225,12 @@ static int stmmac_phy_setup(struct stmmac_priv *priv)
 	if (IS_ERR(phylink))
 		return PTR_ERR(phylink);
 
+	if (mdio_bus_data->has_xpcs) {
+		struct mdio_xpcs_args *xpcs = priv->hw->xpcs;
+
+		phylink_set_pcs(phylink, &xpcs->pcs);
+	}
+
 	priv->phylink = phylink;
 	return 0;
 }
@@ -3652,7 +3631,7 @@ int stmmac_open(struct net_device *dev)
 	if (priv->hw->pcs != STMMAC_PCS_TBI &&
 	    priv->hw->pcs != STMMAC_PCS_RTBI &&
 	    (!priv->hw->xpcs ||
-	     xpcs_get_an_mode(priv->hw->xpcs_args, mode) != DW_AN_C73)) {
+	     xpcs_get_an_mode(priv->hw->xpcs, mode) != DW_AN_C73)) {
 		ret = stmmac_init_phy(dev);
 		if (ret) {
 			netdev_err(priv->dev,
diff --git a/drivers/net/ethernet/stmicro/stmmac/stmmac_mdio.c b/drivers/net/ethernet/stmicro/stmmac/stmmac_mdio.c
index 9b4bf78d2eaa..6312a152c8ad 100644
--- a/drivers/net/ethernet/stmicro/stmmac/stmmac_mdio.c
+++ b/drivers/net/ethernet/stmicro/stmmac/stmmac_mdio.c
@@ -444,14 +444,6 @@ int stmmac_mdio_register(struct net_device *ndev)
 		max_addr = PHY_MAX_ADDR;
 	}
 
-	if (mdio_bus_data->has_xpcs) {
-		priv->hw->xpcs = mdio_xpcs_get_ops();
-		if (!priv->hw->xpcs) {
-			err = -ENODEV;
-			goto bus_register_fail;
-		}
-	}
-
 	if (mdio_bus_data->needs_reset)
 		new_bus->reset = &stmmac_mdio_reset;
 
@@ -526,11 +518,11 @@ int stmmac_mdio_register(struct net_device *ndev)
 				continue;
 			}
 
-			priv->hw->xpcs_args = xpcs;
+			priv->hw->xpcs = xpcs;
 			break;
 		}
 
-		if (!priv->hw->xpcs_args) {
+		if (!priv->hw->xpcs) {
 			dev_warn(dev, "No XPCS found\n");
 			err = -ENODEV;
 			goto no_xpcs_found;
@@ -563,8 +555,8 @@ int stmmac_mdio_unregister(struct net_device *ndev)
 		return 0;
 
 	if (priv->hw->xpcs) {
-		mdio_device_free(priv->hw->xpcs_args->mdiodev);
-		xpcs_destroy(priv->hw->xpcs_args);
+		mdio_device_free(priv->hw->xpcs->mdiodev);
+		xpcs_destroy(priv->hw->xpcs);
 	}
 
 	mdiobus_unregister(priv->mii);
diff --git a/drivers/net/pcs/pcs-xpcs.c b/drivers/net/pcs/pcs-xpcs.c
index 3e09850b8318..f1092771ab70 100644
--- a/drivers/net/pcs/pcs-xpcs.c
+++ b/drivers/net/pcs/pcs-xpcs.c
@@ -100,6 +100,9 @@
 /* VR MII EEE Control 1 defines */
 #define DW_VR_MII_EEE_TRN_LPI		BIT(0)	/* Transparent Mode Enable */
 
+#define phylink_pcs_to_xpcs(pl_pcs) \
+	container_of((pl_pcs), struct mdio_xpcs_args, pcs)
+
 static const int xpcs_usxgmii_features[] = {
 	ETHTOOL_LINK_MODE_Pause_BIT,
 	ETHTOOL_LINK_MODE_Asym_Pause_BIT,
@@ -448,7 +451,7 @@ static int xpcs_get_max_usxgmii_speed(const unsigned long *supported)
 	return max;
 }
 
-static int xpcs_config_usxgmii(struct mdio_xpcs_args *xpcs, int speed)
+static void xpcs_config_usxgmii(struct mdio_xpcs_args *xpcs, int speed)
 {
 	int ret, speed_sel;
 
@@ -473,33 +476,40 @@ static int xpcs_config_usxgmii(struct mdio_xpcs_args *xpcs, int speed)
 		break;
 	default:
 		/* Nothing to do here */
-		return -EINVAL;
+		return;
 	}
 
 	ret = xpcs_read_vpcs(xpcs, MDIO_CTRL1);
 	if (ret < 0)
-		return ret;
+		goto out;
 
 	ret = xpcs_write_vpcs(xpcs, MDIO_CTRL1, ret | DW_USXGMII_EN);
 	if (ret < 0)
-		return ret;
+		goto out;
 
 	ret = xpcs_read(xpcs, MDIO_MMD_VEND2, MDIO_CTRL1);
 	if (ret < 0)
-		return ret;
+		goto out;
 
 	ret &= ~DW_USXGMII_SS_MASK;
 	ret |= speed_sel | DW_USXGMII_FULL;
 
 	ret = xpcs_write(xpcs, MDIO_MMD_VEND2, MDIO_CTRL1, ret);
 	if (ret < 0)
-		return ret;
+		goto out;
 
 	ret = xpcs_read_vpcs(xpcs, MDIO_CTRL1);
 	if (ret < 0)
-		return ret;
+		goto out;
+
+	ret = xpcs_write_vpcs(xpcs, MDIO_CTRL1, ret | DW_USXGMII_RST);
+	if (ret < 0)
+		goto out;
+
+	return;
 
-	return xpcs_write_vpcs(xpcs, MDIO_CTRL1, ret | DW_USXGMII_RST);
+out:
+	pr_err("%s: XPCS access returned %pe\n", __func__, ERR_PTR(ret));
 }
 
 static int _xpcs_config_aneg_c73(struct mdio_xpcs_args *xpcs,
@@ -829,19 +839,19 @@ static int xpcs_config_aneg_c37_sgmii(struct mdio_xpcs_args *xpcs)
 	return xpcs_write(xpcs, MDIO_MMD_VEND2, DW_VR_MII_DIG_CTRL1, ret);
 }
 
-static int xpcs_config(struct mdio_xpcs_args *xpcs,
-		       const struct phylink_link_state *state)
+static int xpcs_do_config(struct mdio_xpcs_args *xpcs,
+			  phy_interface_t interface, unsigned int mode)
 {
 	const struct xpcs_compat *compat;
 	int ret;
 
-	compat = xpcs_find_compat(xpcs->id, state->interface);
+	compat = xpcs_find_compat(xpcs->id, interface);
 	if (!compat)
 		return -ENODEV;
 
 	switch (compat->an_mode) {
 	case DW_AN_C73:
-		if (state->an_enabled) {
+		if (phylink_autoneg_inband(mode)) {
 			ret = xpcs_config_aneg_c73(xpcs, compat);
 			if (ret)
 				return ret;
@@ -859,6 +869,16 @@ static int xpcs_config(struct mdio_xpcs_args *xpcs,
 	return 0;
 }
 
+static int xpcs_config(struct phylink_pcs *pcs, unsigned int mode,
+		       phy_interface_t interface,
+		       const unsigned long *advertising,
+		       bool permit_pause_to_mac)
+{
+	struct mdio_xpcs_args *xpcs = phylink_pcs_to_xpcs(pcs);
+
+	return xpcs_do_config(xpcs, interface, mode);
+}
+
 static int xpcs_get_state_c73(struct mdio_xpcs_args *xpcs,
 			      struct phylink_link_state *state,
 			      const struct xpcs_compat *compat)
@@ -877,7 +897,7 @@ static int xpcs_get_state_c73(struct mdio_xpcs_args *xpcs,
 
 		state->link = 0;
 
-		return xpcs_config(xpcs, state);
+		return xpcs_do_config(xpcs, state->interface, MLO_AN_INBAND);
 	}
 
 	if (state->an_enabled && xpcs_aneg_done_c73(xpcs, state, compat)) {
@@ -934,41 +954,45 @@ static int xpcs_get_state_c37_sgmii(struct mdio_xpcs_args *xpcs,
 	return 0;
 }
 
-static int xpcs_get_state(struct mdio_xpcs_args *xpcs,
-			  struct phylink_link_state *state)
+static void xpcs_get_state(struct phylink_pcs *pcs,
+			   struct phylink_link_state *state)
 {
+	struct mdio_xpcs_args *xpcs = phylink_pcs_to_xpcs(pcs);
 	const struct xpcs_compat *compat;
 	int ret;
 
 	compat = xpcs_find_compat(xpcs->id, state->interface);
 	if (!compat)
-		return -ENODEV;
+		return;
 
 	switch (compat->an_mode) {
 	case DW_AN_C73:
 		ret = xpcs_get_state_c73(xpcs, state, compat);
-		if (ret)
-			return ret;
+		if (ret) {
+			pr_err("xpcs_get_state_c73 returned %pe\n",
+			       ERR_PTR(ret));
+			return;
+		}
 		break;
 	case DW_AN_C37_SGMII:
 		ret = xpcs_get_state_c37_sgmii(xpcs, state);
-		if (ret)
-			return ret;
+		if (ret) {
+			pr_err("xpcs_get_state_c37_sgmii returned %pe\n",
+			       ERR_PTR(ret));
+		}
 		break;
 	default:
-		return -1;
+		return;
 	}
-
-	return 0;
 }
 
-static int xpcs_link_up(struct mdio_xpcs_args *xpcs, int speed,
-			phy_interface_t interface)
+static void xpcs_link_up(struct phylink_pcs *pcs, unsigned int mode,
+			 phy_interface_t interface, int speed, int duplex)
 {
+	struct mdio_xpcs_args *xpcs = phylink_pcs_to_xpcs(pcs);
+
 	if (interface == PHY_INTERFACE_MODE_USXGMII)
 		return xpcs_config_usxgmii(xpcs, speed);
-
-	return 0;
 }
 
 static u32 xpcs_get_id(struct mdio_xpcs_args *xpcs)
@@ -1009,6 +1033,12 @@ static u32 xpcs_get_id(struct mdio_xpcs_args *xpcs)
 	return 0xffffffff;
 }
 
+static const struct phylink_pcs_ops xpcs_phylink_ops = {
+	.pcs_config = xpcs_config,
+	.pcs_get_state = xpcs_get_state,
+	.pcs_link_up = xpcs_link_up,
+};
+
 struct mdio_xpcs_args *xpcs_create(struct mdio_device *mdiodev,
 				   phy_interface_t interface)
 {
@@ -1039,6 +1069,9 @@ struct mdio_xpcs_args *xpcs_create(struct mdio_device *mdiodev,
 			goto out;
 		}
 
+		xpcs->pcs.ops = &xpcs_phylink_ops;
+		xpcs->pcs.poll = true;
+
 		ret = xpcs_soft_reset(xpcs, compat);
 		if (ret)
 			goto out;
@@ -1061,16 +1094,4 @@ void xpcs_destroy(struct mdio_xpcs_args *xpcs)
 }
 EXPORT_SYMBOL_GPL(xpcs_destroy);
 
-static struct mdio_xpcs_ops xpcs_ops = {
-	.config = xpcs_config,
-	.get_state = xpcs_get_state,
-	.link_up = xpcs_link_up,
-};
-
-struct mdio_xpcs_ops *mdio_xpcs_get_ops(void)
-{
-	return &xpcs_ops;
-}
-EXPORT_SYMBOL_GPL(mdio_xpcs_get_ops);
-
 MODULE_LICENSE("GPL v2");
diff --git a/include/linux/pcs/pcs-xpcs.h b/include/linux/pcs/pcs-xpcs.h
index 57a199393d63..0860a5b59f10 100644
--- a/include/linux/pcs/pcs-xpcs.h
+++ b/include/linux/pcs/pcs-xpcs.h
@@ -19,19 +19,10 @@ struct xpcs_id;
 struct mdio_xpcs_args {
 	struct mdio_device *mdiodev;
 	const struct xpcs_id *id;
-};
-
-struct mdio_xpcs_ops {
-	int (*config)(struct mdio_xpcs_args *xpcs,
-		      const struct phylink_link_state *state);
-	int (*get_state)(struct mdio_xpcs_args *xpcs,
-			 struct phylink_link_state *state);
-	int (*link_up)(struct mdio_xpcs_args *xpcs, int speed,
-		       phy_interface_t interface);
+	struct phylink_pcs pcs;
 };
 
 int xpcs_get_an_mode(struct mdio_xpcs_args *xpcs, phy_interface_t interface);
-struct mdio_xpcs_ops *mdio_xpcs_get_ops(void);
 void xpcs_validate(struct mdio_xpcs_args *xpcs, unsigned long *supported,
 		   struct phylink_link_state *state);
 int xpcs_config_eee(struct mdio_xpcs_args *xpcs, int mult_fact_100ns,
-- 
2.25.1

Re: [RFC PATCH v2 net-next 0/9] Convert xpcs to phylink_pcs_ops

[Wong Vee Khee]

On Tue, Jun 01, 2021 at 03:33:16AM +0300, Vladimir Oltean wrote:
> From: Vladimir Oltean <vladimir.oltean@nxp.com>
> 
> This patch series is partially tested (some code paths have been covered
> on the NXP SJA1105) but since I don't have stmmac hardware, it would
> still be appreciated if people from Intel could give this another run.
> 
> Background: the sja1105 DSA driver currently drives a Designware XPCS
> for SGMII and 2500base-X, and it would be nice to reuse some code with
> the xpcs module. This would also help consolidate the phylink_pcs_ops,
> since the only other dedicated PCS driver, currently, is the lynx_pcs.
> 
> Therefore, this series makes the xpcs expose the same kind of API that
> the lynx_pcs module does. The main changes are getting rid of struct
> mdio_xpcs_ops, being compatible with struct phylink_pcs_ops and being
> less reliant on the phy_interface_t passed to xpcs_probe (now renamed to
> xpcs_create).
> 
> Vladimir Oltean (9):
>   net: pcs: xpcs: delete shim definition for mdio_xpcs_get_ops()
>   net: pcs: xpcs: there is only one PHY ID
>   net: pcs: xpcs: make the checks related to the PHY interface mode
>     stateless
>   net: pcs: xpcs: export xpcs_validate
>   net: pcs: xpcs: export xpcs_config_eee
>   net: pcs: xpcs: export xpcs_probe
>   net: pcs: xpcs: use mdiobus_c45_addr in xpcs_{read,write}
>   net: pcs: xpcs: convert to mdio_device
>   net: pcs: xpcs: convert to phylink_pcs_ops
> 
>  drivers/net/ethernet/stmicro/stmmac/common.h  |   3 +-
>  drivers/net/ethernet/stmicro/stmmac/hwif.h    |  14 -
>  .../ethernet/stmicro/stmmac/stmmac_ethtool.c  |  12 +-
>  .../net/ethernet/stmicro/stmmac/stmmac_main.c |  44 +--
>  .../net/ethernet/stmicro/stmmac/stmmac_mdio.c |  47 ++-
>  drivers/net/pcs/pcs-xpcs.c                    | 371 ++++++++++++------
>  include/linux/pcs/pcs-xpcs.h                  |  40 +-
>  7 files changed, 306 insertions(+), 225 deletions(-)

I am seeing kernel panic after applying this patch series on my Intel
Tigerlake board:

[   12.770067] intel-eth-pci 0000:00:1e.4 enp0s30f4: FPE workqueue start
[   12.776481] intel-eth-pci 0000:00:1e.4 enp0s30f4: configuring for inband/sgmii link mode
[   12.784527] BUG: kernel NULL pointer dereference, address: 0000000000000000
[   12.791454] #PF: supervisor instruction fetch in kernel mode
[   12.797083] #PF: error_code(0x0010) - not-present page
[   12.802203] PGD 0 P4D 0
[   12.804739] Oops: 0010 [#1] PREEMPT SMP NOPTI
[   12.809080] CPU: 2 PID: 2023 Comm: connmand Tainted: G     U            5.13.0-rc3-intel-lts #73
[   12.817813] Hardware name: Intel Corporation Tiger Lake Client Platform/TigerLake U DDR4 SODIMM RVP, BIOS TGLIFUI1.R00.3373.AF0.2009230546 09/23/2020
[   12.831105] RIP: 0010:0x0
[   12.833732] Code: Unable to access opcode bytes at RIP 0xffffffffffffffd6.
[   12.840566] RSP: 0018:ffff9f404052bb78 EFLAGS: 00010246
[   12.845771] RAX: 0000000000000000 RBX: ffff93e489bdda00 RCX: 0000000000000000
[   12.852867] RDX: ffff9f404052bba0 RSI: 0000000000000002 RDI: ffff93e4891244d8
[   12.859963] RBP: ffff9f404052bba0 R08: 0000000000000000 R09: ffff9f404052b888
[   12.867054] R10: 0000000000000001 R11: 0000000000000001 R12: 0000000000000001
[   12.874145] R13: ffff93e4891208c0 R14: 0000000000000006 R15: 0000000000000001
[   12.881242] FS:  00007fd656ece7c0(0000) GS:ffff93e617f00000(0000) knlGS:0000000000000000
[   12.889286] CS:  0010 DS: 0000 ES: 0000 CR0: 0000000080050033
[   12.895000] CR2: ffffffffffffffd6 CR3: 000000010e674004 CR4: 0000000000770ee0
[   12.902094] PKRU: 55555554
[   12.904796] Call Trace:
[   12.907245]  phylink_major_config+0x5e/0x1a0 [phylink]
[   12.912368]  phylink_start+0x204/0x2c0 [phylink]
[   12.916971]  stmmac_open+0x3d0/0x9f0 [stmmac]
[   12.921317]  __dev_open+0xe7/0x180
[   12.924710]  __dev_change_flags+0x174/0x1d0
[   12.928882]  ? __thaw_task+0x40/0x40
[   12.932453]  ? arch_stack_walk+0x9e/0xf0
[   12.936363]  dev_change_flags+0x21/0x60
[   12.940188]  devinet_ioctl+0x5e8/0x750
[   12.943925]  ? common_interrupt+0xc0/0xe0
[   12.947927]  inet_ioctl+0x190/0x1c0
[   12.951408]  ? dev_ioctl+0x26d/0x4c0
[   12.954972]  sock_do_ioctl+0x44/0x140
[   12.958627]  ? alloc_empty_file+0x61/0xb0
[   12.962628]  sock_ioctl+0x22c/0x320
[   12.966111]  __x64_sys_ioctl+0x80/0xb0
[   12.969852]  do_syscall_64+0x42/0x80
[   12.973418]  entry_SYSCALL_64_after_hwframe+0x44/0xae
[   12.978446] RIP: 0033:0x7fd65741b4bb
[   12.982015] Code: 0f 1e fa 48 8b 05 c5 69 0c 00 64 c7 00 26 00 00 00 48 c7 c0 ff ff ff ff c3 66 0f 1f 44 00 00 f3 0f 1e fa b8 10 00 00 00 0f 05 <48> 3d 01 f0 ff ff 73 01 c3 48 8b 0d 95 69 0c 00 f7 d8 64 89 01 48
[   13.000645] RSP: 002b:00007ffda8ca3288 EFLAGS: 00000246 ORIG_RAX: 0000000000000010
[   13.008170] RAX: ffffffffffffffda RBX: 00005576e8ee2100 RCX: 00007fd65741b4bb
[   13.015262] RDX: 00007ffda8ca3290 RSI: 0000000000008914 RDI: 0000000000000010
[   13.022355] RBP: 0000000000000010 R08: 00005576e8ee2100 R09: 0000000000000000
[   13.029447] R10: 00005576e8e8f010 R11: 0000000000000246 R12: 0000000000000000
[   13.036539] R13: 00007ffda8ca3290 R14: 0000000000000001 R15: 00007ffda8ca39d0
[   13.043633] Modules linked in: bluetooth ecryptfs hid_sensor_gyro_3d hid_sensor_incl_3d hid_sensor_magn_3d hid_sensor_accel_3d hid_sensor_als hid_sensor_trigger hid_sensor_iio_common hid_sensor_custom hid_sensor_hub intel_ishtp_loader intel_ishtp_hid intel_gpy ax88179_178a usbnet dwmac_intel mii x86_pkg_temp_thermal stmmac dwc3 kvm_intel pcs_xpcs mei_wdt mei_hdcp atkbd udc_core libps2 phylink kvm libphy i915 spi_pxa2xx_platform irqbypass intel_rapl_msr mei_me wdat_wdt i2c_i801 pcspkr dw_dmac dw_dmac_core intel_ish_ipc i2c_smbus mei intel_ishtp dwc3_pci tpm_crb thermal parport_pc i8042 tpm_tis parport tpm_tis_core tpm intel_pmc_core sch_fq_codel uhid fuse configfs snd_hda_intel snd_intel_dspcfg snd_intel_sdw_acpi snd_hda_codec snd_hda_core snd_pcm snd_timer snd soundcore
[   13.111247] CR2: 0000000000000000
[   13.114556] ---[ end trace aef3fc6d992073a6 ]---

Re: [RFC PATCH v2 net-next 9/9] net: pcs: xpcs: convert to phylink_pcs_ops

[Russell King (Oracle)]

On Tue, Jun 01, 2021 at 03:33:25AM +0300, Vladimir Oltean wrote:
>  static const struct phylink_mac_ops stmmac_phylink_mac_ops = {
>  	.validate = stmmac_validate,
> -	.mac_pcs_get_state = stmmac_mac_pcs_get_state,
> -	.mac_config = stmmac_mac_config,

mac_config is still a required function.

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

Re: [RFC PATCH v2 net-next 9/9] net: pcs: xpcs: convert to phylink_pcs_ops

[Vladimir Oltean]

On Tue, Jun 01, 2021 at 01:10:33PM +0100, Russell King (Oracle) wrote:
> On Tue, Jun 01, 2021 at 03:33:25AM +0300, Vladimir Oltean wrote:
> >  static const struct phylink_mac_ops stmmac_phylink_mac_ops = {
> >  	.validate = stmmac_validate,
> > -	.mac_pcs_get_state = stmmac_mac_pcs_get_state,
> > -	.mac_config = stmmac_mac_config,
> 
> mac_config is still a required function.

This is correct, thanks.

VK, would you mind testing again with this extra patch added to the mix?
If it works, I will add it to the series in v3, ordered properly.

-----------------------------[ cut here]-----------------------------
From a79863027998451c73d5bbfaf1b77cf6097a110c Mon Sep 17 00:00:00 2001
From: Vladimir Oltean <vladimir.oltean@nxp.com>
Date: Wed, 2 Jun 2021 16:35:55 +0300
Subject: [PATCH] net: phylink: allow the mac_config method to be missing if
 pcs_ops are provided

The pcs_config method from struct phylink_pcs_ops does everything that
the mac_config method from struct phylink_mac_ops used to do in the
legacy approach of driving a MAC PCS. So allow drivers to not implement
the mac_config method if there is nothing to do. Keep the method
required for setups that do not provide pcs_ops.

Signed-off-by: Vladimir Oltean <vladimir.oltean@nxp.com>
---
 drivers/net/phy/phylink.c | 9 +++++++++
 1 file changed, 9 insertions(+)

diff --git a/drivers/net/phy/phylink.c b/drivers/net/phy/phylink.c
index 96d8e88b4e46..a8842c6ce3a2 100644
--- a/drivers/net/phy/phylink.c
+++ b/drivers/net/phy/phylink.c
@@ -415,6 +415,9 @@ static void phylink_resolve_flow(struct phylink_link_state *state)
 static void phylink_mac_config(struct phylink *pl,
 			       const struct phylink_link_state *state)
 {
+	if (!pl->mac_ops->mac_config)
+		return;
+
 	phylink_dbg(pl,
 		    "%s: mode=%s/%s/%s/%s adv=%*pb pause=%02x link=%u an=%u\n",
 		    __func__, phylink_an_mode_str(pl->cur_link_an_mode),
@@ -1192,6 +1195,12 @@ void phylink_start(struct phylink *pl)
 
 	ASSERT_RTNL();
 
+	/* The mac_ops::mac_config method may be absent only if the
+	 * pcs_ops are present.
+	 */
+	if (WARN_ON_ONCE(!pl->mac_ops->mac_config && !pl->pcs_ops))
+		return;
+
 	phylink_info(pl, "configuring for %s/%s link mode\n",
 		     phylink_an_mode_str(pl->cur_link_an_mode),
 		     phy_modes(pl->link_config.interface));
-----------------------------[ cut here]-----------------------------

Re: [RFC PATCH v2 net-next 9/9] net: pcs: xpcs: convert to phylink_pcs_ops

[Russell King (Oracle)]

On Wed, Jun 02, 2021 at 04:43:21PM +0300, Vladimir Oltean wrote:
> On Tue, Jun 01, 2021 at 01:10:33PM +0100, Russell King (Oracle) wrote:
> > On Tue, Jun 01, 2021 at 03:33:25AM +0300, Vladimir Oltean wrote:
> > >  static const struct phylink_mac_ops stmmac_phylink_mac_ops = {
> > >  	.validate = stmmac_validate,
> > > -	.mac_pcs_get_state = stmmac_mac_pcs_get_state,
> > > -	.mac_config = stmmac_mac_config,
> > 
> > mac_config is still a required function.
> 
> This is correct, thanks.
> 
> VK, would you mind testing again with this extra patch added to the mix?
> If it works, I will add it to the series in v3, ordered properly.
> 
> -----------------------------[ cut here]-----------------------------
> From a79863027998451c73d5bbfaf1b77cf6097a110c Mon Sep 17 00:00:00 2001
> From: Vladimir Oltean <vladimir.oltean@nxp.com>
> Date: Wed, 2 Jun 2021 16:35:55 +0300
> Subject: [PATCH] net: phylink: allow the mac_config method to be missing if
>  pcs_ops are provided
> 
> The pcs_config method from struct phylink_pcs_ops does everything that
> the mac_config method from struct phylink_mac_ops used to do in the
> legacy approach of driving a MAC PCS. So allow drivers to not implement
> the mac_config method if there is nothing to do. Keep the method
> required for setups that do not provide pcs_ops.
> 
> Signed-off-by: Vladimir Oltean <vladimir.oltean@nxp.com>
> ---
>  drivers/net/phy/phylink.c | 9 +++++++++
>  1 file changed, 9 insertions(+)
> 
> diff --git a/drivers/net/phy/phylink.c b/drivers/net/phy/phylink.c
> index 96d8e88b4e46..a8842c6ce3a2 100644
> --- a/drivers/net/phy/phylink.c
> +++ b/drivers/net/phy/phylink.c
> @@ -415,6 +415,9 @@ static void phylink_resolve_flow(struct phylink_link_state *state)
>  static void phylink_mac_config(struct phylink *pl,
>  			       const struct phylink_link_state *state)
>  {
> +	if (!pl->mac_ops->mac_config)
> +		return;
> +
>  	phylink_dbg(pl,
>  		    "%s: mode=%s/%s/%s/%s adv=%*pb pause=%02x link=%u an=%u\n",
>  		    __func__, phylink_an_mode_str(pl->cur_link_an_mode),
> @@ -1192,6 +1195,12 @@ void phylink_start(struct phylink *pl)
>  
>  	ASSERT_RTNL();
>  
> +	/* The mac_ops::mac_config method may be absent only if the
> +	 * pcs_ops are present.
> +	 */
> +	if (WARN_ON_ONCE(!pl->mac_ops->mac_config && !pl->pcs_ops))
> +		return;
> +
>  	phylink_info(pl, "configuring for %s/%s link mode\n",
>  		     phylink_an_mode_str(pl->cur_link_an_mode),
>  		     phy_modes(pl->link_config.interface));

I would rather we didn't do that - I suspect your case is not the
common scenario, so please add a dummy function to stmmac instead.

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

Re: [RFC PATCH v2 net-next 9/9] net: pcs: xpcs: convert to phylink_pcs_ops

[Vladimir Oltean]

On Wed, Jun 02, 2021 at 02:47:49PM +0100, Russell King (Oracle) wrote:
> On Wed, Jun 02, 2021 at 04:43:21PM +0300, Vladimir Oltean wrote:
> > On Tue, Jun 01, 2021 at 01:10:33PM +0100, Russell King (Oracle) wrote:
> > > On Tue, Jun 01, 2021 at 03:33:25AM +0300, Vladimir Oltean wrote:
> > > >  static const struct phylink_mac_ops stmmac_phylink_mac_ops = {
> > > >  	.validate = stmmac_validate,
> > > > -	.mac_pcs_get_state = stmmac_mac_pcs_get_state,
> > > > -	.mac_config = stmmac_mac_config,
> > > 
> > > mac_config is still a required function.
> > 
> > This is correct, thanks.
> > 
> > VK, would you mind testing again with this extra patch added to the mix?
> > If it works, I will add it to the series in v3, ordered properly.
> > 
> > -----------------------------[ cut here]-----------------------------
> > From a79863027998451c73d5bbfaf1b77cf6097a110c Mon Sep 17 00:00:00 2001
> > From: Vladimir Oltean <vladimir.oltean@nxp.com>
> > Date: Wed, 2 Jun 2021 16:35:55 +0300
> > Subject: [PATCH] net: phylink: allow the mac_config method to be missing if
> >  pcs_ops are provided
> > 
> > The pcs_config method from struct phylink_pcs_ops does everything that
> > the mac_config method from struct phylink_mac_ops used to do in the
> > legacy approach of driving a MAC PCS. So allow drivers to not implement
> > the mac_config method if there is nothing to do. Keep the method
> > required for setups that do not provide pcs_ops.
> > 
> > Signed-off-by: Vladimir Oltean <vladimir.oltean@nxp.com>
> > ---
> >  drivers/net/phy/phylink.c | 9 +++++++++
> >  1 file changed, 9 insertions(+)
> > 
> > diff --git a/drivers/net/phy/phylink.c b/drivers/net/phy/phylink.c
> > index 96d8e88b4e46..a8842c6ce3a2 100644
> > --- a/drivers/net/phy/phylink.c
> > +++ b/drivers/net/phy/phylink.c
> > @@ -415,6 +415,9 @@ static void phylink_resolve_flow(struct phylink_link_state *state)
> >  static void phylink_mac_config(struct phylink *pl,
> >  			       const struct phylink_link_state *state)
> >  {
> > +	if (!pl->mac_ops->mac_config)
> > +		return;
> > +
> >  	phylink_dbg(pl,
> >  		    "%s: mode=%s/%s/%s/%s adv=%*pb pause=%02x link=%u an=%u\n",
> >  		    __func__, phylink_an_mode_str(pl->cur_link_an_mode),
> > @@ -1192,6 +1195,12 @@ void phylink_start(struct phylink *pl)
> >  
> >  	ASSERT_RTNL();
> >  
> > +	/* The mac_ops::mac_config method may be absent only if the
> > +	 * pcs_ops are present.
> > +	 */
> > +	if (WARN_ON_ONCE(!pl->mac_ops->mac_config && !pl->pcs_ops))
> > +		return;
> > +
> >  	phylink_info(pl, "configuring for %s/%s link mode\n",
> >  		     phylink_an_mode_str(pl->cur_link_an_mode),
> >  		     phy_modes(pl->link_config.interface));
> 
> I would rather we didn't do that - I suspect your case is not the
> common scenario, so please add a dummy function to stmmac instead.

Ok, in that case the only delta to be applied should be:

-----------------------------[ cut here]-----------------------------
From 998569108392befc591f790e46fe5dcd1d0b6278 Mon Sep 17 00:00:00 2001
From: Vladimir Oltean <vladimir.oltean@nxp.com>
Date: Wed, 2 Jun 2021 17:00:12 +0300
Subject: [PATCH] fixup! net: pcs: xpcs: convert to phylink_pcs_ops

Signed-off-by: Vladimir Oltean <vladimir.oltean@nxp.com>
---
 drivers/net/ethernet/stmicro/stmmac/stmmac_main.c | 6 ++++++
 1 file changed, 6 insertions(+)

diff --git a/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c b/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c
index d5685a74f3b7..704aa91b145a 100644
--- a/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c
+++ b/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c
@@ -1000,6 +1000,12 @@ static void stmmac_validate(struct phylink_config *config,
 		xpcs_validate(priv->hw->xpcs, supported, state);
 }
 
+static void stmmac_mac_config(struct phylink_config *config, unsigned int mode,
+			      const struct phylink_link_state *state)
+{
+	/* Nothing to do, xpcs_config() handles everything */
+}
+
 static void stmmac_fpe_link_state_handle(struct stmmac_priv *priv, bool is_up)
 {
 	struct stmmac_fpe_cfg *fpe_cfg = priv->plat->fpe_cfg;
-----------------------------[ cut here]-----------------------------

Re: [RFC PATCH v2 net-next 9/9] net: pcs: xpcs: convert to phylink_pcs_ops

[Wong Vee Khee]

On Wed, Jun 02, 2021 at 05:02:33PM +0300, Vladimir Oltean wrote:
> On Wed, Jun 02, 2021 at 02:47:49PM +0100, Russell King (Oracle) wrote:
> > On Wed, Jun 02, 2021 at 04:43:21PM +0300, Vladimir Oltean wrote:
> > > On Tue, Jun 01, 2021 at 01:10:33PM +0100, Russell King (Oracle) wrote:
> > > > On Tue, Jun 01, 2021 at 03:33:25AM +0300, Vladimir Oltean wrote:
> > > > >  static const struct phylink_mac_ops stmmac_phylink_mac_ops = {
> > > > >  	.validate = stmmac_validate,
> > > > > -	.mac_pcs_get_state = stmmac_mac_pcs_get_state,
> > > > > -	.mac_config = stmmac_mac_config,
> > > > 
> > > > mac_config is still a required function.
> > > 
> > > This is correct, thanks.
> > > 
> > > VK, would you mind testing again with this extra patch added to the mix?
> > > If it works, I will add it to the series in v3, ordered properly.
> > > 
> > > -----------------------------[ cut here]-----------------------------
> > > From a79863027998451c73d5bbfaf1b77cf6097a110c Mon Sep 17 00:00:00 2001
> > > From: Vladimir Oltean <vladimir.oltean@nxp.com>
> > > Date: Wed, 2 Jun 2021 16:35:55 +0300
> > > Subject: [PATCH] net: phylink: allow the mac_config method to be missing if
> > >  pcs_ops are provided
> > > 
> > > The pcs_config method from struct phylink_pcs_ops does everything that
> > > the mac_config method from struct phylink_mac_ops used to do in the
> > > legacy approach of driving a MAC PCS. So allow drivers to not implement
> > > the mac_config method if there is nothing to do. Keep the method
> > > required for setups that do not provide pcs_ops.
> > > 
> > > Signed-off-by: Vladimir Oltean <vladimir.oltean@nxp.com>
> > > ---
> > >  drivers/net/phy/phylink.c | 9 +++++++++
> > >  1 file changed, 9 insertions(+)
> > > 
> > > diff --git a/drivers/net/phy/phylink.c b/drivers/net/phy/phylink.c
> > > index 96d8e88b4e46..a8842c6ce3a2 100644
> > > --- a/drivers/net/phy/phylink.c
> > > +++ b/drivers/net/phy/phylink.c
> > > @@ -415,6 +415,9 @@ static void phylink_resolve_flow(struct phylink_link_state *state)
> > >  static void phylink_mac_config(struct phylink *pl,
> > >  			       const struct phylink_link_state *state)
> > >  {
> > > +	if (!pl->mac_ops->mac_config)
> > > +		return;
> > > +
> > >  	phylink_dbg(pl,
> > >  		    "%s: mode=%s/%s/%s/%s adv=%*pb pause=%02x link=%u an=%u\n",
> > >  		    __func__, phylink_an_mode_str(pl->cur_link_an_mode),
> > > @@ -1192,6 +1195,12 @@ void phylink_start(struct phylink *pl)
> > >  
> > >  	ASSERT_RTNL();
> > >  
> > > +	/* The mac_ops::mac_config method may be absent only if the
> > > +	 * pcs_ops are present.
> > > +	 */
> > > +	if (WARN_ON_ONCE(!pl->mac_ops->mac_config && !pl->pcs_ops))
> > > +		return;
> > > +
> > >  	phylink_info(pl, "configuring for %s/%s link mode\n",
> > >  		     phylink_an_mode_str(pl->cur_link_an_mode),
> > >  		     phy_modes(pl->link_config.interface));
> > 
> > I would rather we didn't do that - I suspect your case is not the
> > common scenario, so please add a dummy function to stmmac instead.
> 
> Ok, in that case the only delta to be applied should be:
> 
> -----------------------------[ cut here]-----------------------------
> >From 998569108392befc591f790e46fe5dcd1d0b6278 Mon Sep 17 00:00:00 2001
> From: Vladimir Oltean <vladimir.oltean@nxp.com>
> Date: Wed, 2 Jun 2021 17:00:12 +0300
> Subject: [PATCH] fixup! net: pcs: xpcs: convert to phylink_pcs_ops
> 
> Signed-off-by: Vladimir Oltean <vladimir.oltean@nxp.com>
> ---
>  drivers/net/ethernet/stmicro/stmmac/stmmac_main.c | 6 ++++++
>  1 file changed, 6 insertions(+)
> 
> diff --git a/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c b/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c
> index d5685a74f3b7..704aa91b145a 100644
> --- a/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c
> +++ b/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c
> @@ -1000,6 +1000,12 @@ static void stmmac_validate(struct phylink_config *config,
>  		xpcs_validate(priv->hw->xpcs, supported, state);
>  }
>  
> +static void stmmac_mac_config(struct phylink_config *config, unsigned int mode,
> +			      const struct phylink_link_state *state)
> +{
> +	/* Nothing to do, xpcs_config() handles everything */
> +}
> +
>  static void stmmac_fpe_link_state_handle(struct stmmac_priv *priv, bool is_up)
>  {
>  	struct stmmac_fpe_cfg *fpe_cfg = priv->plat->fpe_cfg;
> -----------------------------[ cut here]-----------------------------

No luck.. I am still seeing the same kernel panic on my side with the
additional patch applied:

[   12.513652] intel-eth-pci 0000:00:1e.4 enp0s30f4: FPE workqueue start
[   12.520079] intel-eth-pci 0000:00:1e.4 enp0s30f4: configuring for inband/sgmii link mode
[   12.528141] BUG: kernel NULL pointer dereference, address: 0000000000000000
[   12.535072] #PF: supervisor instruction fetch in kernel mode
[   12.540719] #PF: error_code(0x0010) - not-present page
[   12.545843] PGD 0 P4D 0
[   12.548382] Oops: 0010 [#1] PREEMPT SMP NOPTI
[   12.552733] CPU: 3 PID: 2657 Comm: connmand Tainted: G     U            5.13.0-rc3-intel-lts #79
[   12.561485] Hardware name: Intel Corporation Tiger Lake Client Platform/TigerLake U DDR4 SODIMM RVP, BIOS TGLIFUI1.R00.3373.AF0.2009230546 09/23/2020
[   12.574803] RIP: 0010:0x0
[   12.577436] Code: Unable to access opcode bytes at RIP 0xffffffffffffffd6.
[   12.584280] RSP: 0018:ffffaa05404efb78 EFLAGS: 00010246
[   12.589489] RAX: 0000000000000000 RBX: ffffa0fb91758c00 RCX: 0000000000000000
[   12.596600] RDX: ffffaa05404efba0 RSI: 0000000000000002 RDI: ffffa0facc63c4d8
[   12.603711] RBP: ffffaa05404efba0 R08: 0000000000000000 R09: ffffaa05404ef888
[   12.610825] R10: 0000000000000001 R11: 0000000000000001 R12: 0000000000000001
[   12.617935] R13: ffffa0facc6388c0 R14: 0000000000000006 R15: 0000000000000001
[   12.625040] FS:  00007f4a6d5b87c0(0000) GS:ffffa0fc57f80000(0000) knlGS:0000000000000000
[   12.633096] CS:  0010 DS: 0000 ES: 0000 CR0: 0000000080050033
[   12.638825] CR2: ffffffffffffffd6 CR3: 000000010e3f6004 CR4: 0000000000770ee0
[   12.645932] PKRU: 55555554
[   12.648641] Call Trace:
[   12.651095]  phylink_major_config+0x5e/0x1a0 [phylink]
[   12.656219]  phylink_start+0x204/0x2c0 [phylink]
[   12.660833]  stmmac_open+0x3d0/0x9f0 [stmmac]
[   12.665186]  __dev_open+0xe7/0x180
[   12.668594]  __dev_change_flags+0x174/0x1d0
[   12.672773]  ? __thaw_task+0x40/0x40
[   12.676348]  ? arch_stack_walk+0x9e/0xf0
[   12.680266]  dev_change_flags+0x21/0x60
[   12.684100]  devinet_ioctl+0x5e8/0x750
[   12.687847]  inet_ioctl+0x190/0x1c0
[   12.691335]  ? dev_ioctl+0x26d/0x4c0
[   12.694907]  sock_do_ioctl+0x44/0x140
[   12.698569]  ? alloc_empty_file+0x61/0xb0
[   12.702572]  sock_ioctl+0x22c/0x320
[   12.706061]  __x64_sys_ioctl+0x80/0xb0
[   12.709808]  do_syscall_64+0x42/0x80
[   12.713381]  entry_SYSCALL_64_after_hwframe+0x44/0xae
[   12.718425] RIP: 0033:0x7f4a6db054bb
[   12.721995] Code: 0f 1e fa 48 8b 05 c5 69 0c 00 64 c7 00 26 00 00 00 48 c7 c0 ff ff ff ff c3 66 0f 1f 44 00 00 f3 0f 1e fa b8 10 00 00 00 0f 05 <48> 3d 01 f0 ff ff 73 01 c3 48 8b 0d 95 69 0c 00 f7 d8 64 89 01 48
[   12.740656] RSP: 002b:00007ffdeab00da8 EFLAGS: 00000246 ORIG_RAX: 0000000000000010
[   12.748194] RAX: ffffffffffffffda RBX: 0000562741681100 RCX: 00007f4a6db054bb
[   12.755300] RDX: 00007ffdeab00db0 RSI: 0000000000008914 RDI: 0000000000000010
[   12.762405] RBP: 0000000000000010 R08: 0000562741681100 R09: 0000000000000000
[   12.769510] R10: 000056274162e010 R11: 0000000000000246 R12: 0000000000000000
[   12.776613] R13: 00007ffdeab00db0 R14: 0000000000000001 R15: 00007ffdeab014f0
[   12.783721] Modules linked in: bluetooth ecryptfs hid_sensor_incl_3d hid_sensor_magn_3d hid_sensor_gyro_3d hid_sensor_als hid_sensor_accel_3d hid_sensor_trigger hid_sensor_iio_common hid_sensor_custom hid_sensor_hub intel_ishtp_loader intel_ishtp_hid mxl_gpy x86_pkg_temp_thermal ax88179_178a dwmac_intel usbnet mii stmmac dwc3 atkbd kvm_intel pcs_xpcs mei_wdt phylink mei_hdcp udc_core kvm libps2 libphy mei_me i915 irqbypass spi_pxa2xx_platform intel_rapl_msr pcspkr dw_dmac wdat_wdt i2c_i801 dw_dmac_core i2c_smbus intel_ish_ipc mei intel_ishtp dwc3_pci thermal tpm_crb tpm_tis tpm_tis_core intel_pmc_core parport_pc i8042 parport tpm sch_fq_codel uhid fuse configfs snd_hda_intel snd_intel_dspcfg snd_intel_sdw_acpi snd_hda_codec snd_hda_core snd_pcm snd_timer snd soundcore
[   12.851290] CR2: 0000000000000000
[   12.854603] ---[ end trace 52b9ca69be06d1e3 ]---


My git log:-
dc25bc46886e (HEAD -> master) phy: maxlinear: add Maxlinear GPY115/21x/24x driver
d059f32fcef1 fixup! net: pcs: xpcs: convert to phylink_pcs_ops
1a1a9c7267c1 net: pcs: xpcs: convert to phylink_pcs_ops
ec25f3c80108 net: pcs: xpcs: convert to mdio_device
912a23a5768a net: pcs: xpcs: use mdiobus_c45_addr in xpcs_{read,write}
af2bd0a4e32c net: pcs: xpcs: export xpcs_probe
7e4eea51132f net: pcs: xpcs: export xpcs_config_eee
37d5e086fc83 net: pcs: xpcs: export xpcs_validate
d58487edb1a8 net: pcs: xpcs: make the checks related to the PHY interface mode stateless
2f0eba94af8d net: pcs: xpcs: there is only one PHY ID
67148dd06896 net: pcs: xpcs: delete shim definition for mdio_xpcs_get_ops()
5fe8e519e44f (origin/master, origin/HEAD) Merge git://git.kernel.org/pub/scm/linux/kernel/git/pablo/nf-next

HTH,
 VK

Re: [RFC PATCH v2 net-next 9/9] net: pcs: xpcs: convert to phylink_pcs_ops

[Vladimir Oltean]

On Wed, Jun 02, 2021 at 10:43:36PM +0800, Wong Vee Khee wrote:
> On Wed, Jun 02, 2021 at 05:02:33PM +0300, Vladimir Oltean wrote:
> > On Wed, Jun 02, 2021 at 02:47:49PM +0100, Russell King (Oracle) wrote:
> > > On Wed, Jun 02, 2021 at 04:43:21PM +0300, Vladimir Oltean wrote:
> > > > On Tue, Jun 01, 2021 at 01:10:33PM +0100, Russell King (Oracle) wrote:
> > > > > On Tue, Jun 01, 2021 at 03:33:25AM +0300, Vladimir Oltean wrote:
> > > > > >  static const struct phylink_mac_ops stmmac_phylink_mac_ops = {
> > > > > >  	.validate = stmmac_validate,
> > > > > > -	.mac_pcs_get_state = stmmac_mac_pcs_get_state,
> > > > > > -	.mac_config = stmmac_mac_config,
> > > > > 
> > > > > mac_config is still a required function.
> > > > 
> > > > This is correct, thanks.
> > > > 
> > > > VK, would you mind testing again with this extra patch added to the mix?
> > > > If it works, I will add it to the series in v3, ordered properly.
> > > > 
> > > > -----------------------------[ cut here]-----------------------------
> > > > From a79863027998451c73d5bbfaf1b77cf6097a110c Mon Sep 17 00:00:00 2001
> > > > From: Vladimir Oltean <vladimir.oltean@nxp.com>
> > > > Date: Wed, 2 Jun 2021 16:35:55 +0300
> > > > Subject: [PATCH] net: phylink: allow the mac_config method to be missing if
> > > >  pcs_ops are provided
> > > > 
> > > > The pcs_config method from struct phylink_pcs_ops does everything that
> > > > the mac_config method from struct phylink_mac_ops used to do in the
> > > > legacy approach of driving a MAC PCS. So allow drivers to not implement
> > > > the mac_config method if there is nothing to do. Keep the method
> > > > required for setups that do not provide pcs_ops.
> > > > 
> > > > Signed-off-by: Vladimir Oltean <vladimir.oltean@nxp.com>
> > > > ---
> > > >  drivers/net/phy/phylink.c | 9 +++++++++
> > > >  1 file changed, 9 insertions(+)
> > > > 
> > > > diff --git a/drivers/net/phy/phylink.c b/drivers/net/phy/phylink.c
> > > > index 96d8e88b4e46..a8842c6ce3a2 100644
> > > > --- a/drivers/net/phy/phylink.c
> > > > +++ b/drivers/net/phy/phylink.c
> > > > @@ -415,6 +415,9 @@ static void phylink_resolve_flow(struct phylink_link_state *state)
> > > >  static void phylink_mac_config(struct phylink *pl,
> > > >  			       const struct phylink_link_state *state)
> > > >  {
> > > > +	if (!pl->mac_ops->mac_config)
> > > > +		return;
> > > > +
> > > >  	phylink_dbg(pl,
> > > >  		    "%s: mode=%s/%s/%s/%s adv=%*pb pause=%02x link=%u an=%u\n",
> > > >  		    __func__, phylink_an_mode_str(pl->cur_link_an_mode),
> > > > @@ -1192,6 +1195,12 @@ void phylink_start(struct phylink *pl)
> > > >  
> > > >  	ASSERT_RTNL();
> > > >  
> > > > +	/* The mac_ops::mac_config method may be absent only if the
> > > > +	 * pcs_ops are present.
> > > > +	 */
> > > > +	if (WARN_ON_ONCE(!pl->mac_ops->mac_config && !pl->pcs_ops))
> > > > +		return;
> > > > +
> > > >  	phylink_info(pl, "configuring for %s/%s link mode\n",
> > > >  		     phylink_an_mode_str(pl->cur_link_an_mode),
> > > >  		     phy_modes(pl->link_config.interface));
> > > 
> > > I would rather we didn't do that - I suspect your case is not the
> > > common scenario, so please add a dummy function to stmmac instead.
> > 
> > Ok, in that case the only delta to be applied should be:
> > 
> > -----------------------------[ cut here]-----------------------------
> > >From 998569108392befc591f790e46fe5dcd1d0b6278 Mon Sep 17 00:00:00 2001
> > From: Vladimir Oltean <vladimir.oltean@nxp.com>
> > Date: Wed, 2 Jun 2021 17:00:12 +0300
> > Subject: [PATCH] fixup! net: pcs: xpcs: convert to phylink_pcs_ops
> > 
> > Signed-off-by: Vladimir Oltean <vladimir.oltean@nxp.com>
> > ---
> >  drivers/net/ethernet/stmicro/stmmac/stmmac_main.c | 6 ++++++
> >  1 file changed, 6 insertions(+)
> > 
> > diff --git a/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c b/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c
> > index d5685a74f3b7..704aa91b145a 100644
> > --- a/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c
> > +++ b/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c
> > @@ -1000,6 +1000,12 @@ static void stmmac_validate(struct phylink_config *config,
> >  		xpcs_validate(priv->hw->xpcs, supported, state);
> >  }
> >  
> > +static void stmmac_mac_config(struct phylink_config *config, unsigned int mode,
> > +			      const struct phylink_link_state *state)
> > +{
> > +	/* Nothing to do, xpcs_config() handles everything */
> > +}
> > +
> >  static void stmmac_fpe_link_state_handle(struct stmmac_priv *priv, bool is_up)
> >  {
> >  	struct stmmac_fpe_cfg *fpe_cfg = priv->plat->fpe_cfg;
> > -----------------------------[ cut here]-----------------------------
> 
> No luck.. I am still seeing the same kernel panic on my side with the
> additional patch applied:
> 
> [   12.513652] intel-eth-pci 0000:00:1e.4 enp0s30f4: FPE workqueue start
> [   12.520079] intel-eth-pci 0000:00:1e.4 enp0s30f4: configuring for inband/sgmii link mode
> [   12.528141] BUG: kernel NULL pointer dereference, address: 0000000000000000
> [   12.535072] #PF: supervisor instruction fetch in kernel mode
> [   12.540719] #PF: error_code(0x0010) - not-present page
> [   12.545843] PGD 0 P4D 0
> [   12.548382] Oops: 0010 [#1] PREEMPT SMP NOPTI
> [   12.552733] CPU: 3 PID: 2657 Comm: connmand Tainted: G     U            5.13.0-rc3-intel-lts #79
> [   12.561485] Hardware name: Intel Corporation Tiger Lake Client Platform/TigerLake U DDR4 SODIMM RVP, BIOS TGLIFUI1.R00.3373.AF0.2009230546 09/23/2020
> [   12.574803] RIP: 0010:0x0
> [   12.577436] Code: Unable to access opcode bytes at RIP 0xffffffffffffffd6.
> [   12.584280] RSP: 0018:ffffaa05404efb78 EFLAGS: 00010246
> [   12.589489] RAX: 0000000000000000 RBX: ffffa0fb91758c00 RCX: 0000000000000000
> [   12.596600] RDX: ffffaa05404efba0 RSI: 0000000000000002 RDI: ffffa0facc63c4d8
> [   12.603711] RBP: ffffaa05404efba0 R08: 0000000000000000 R09: ffffaa05404ef888
> [   12.610825] R10: 0000000000000001 R11: 0000000000000001 R12: 0000000000000001
> [   12.617935] R13: ffffa0facc6388c0 R14: 0000000000000006 R15: 0000000000000001
> [   12.625040] FS:  00007f4a6d5b87c0(0000) GS:ffffa0fc57f80000(0000) knlGS:0000000000000000
> [   12.633096] CS:  0010 DS: 0000 ES: 0000 CR0: 0000000080050033
> [   12.638825] CR2: ffffffffffffffd6 CR3: 000000010e3f6004 CR4: 0000000000770ee0
> [   12.645932] PKRU: 55555554
> [   12.648641] Call Trace:
> [   12.651095]  phylink_major_config+0x5e/0x1a0 [phylink]
> [   12.656219]  phylink_start+0x204/0x2c0 [phylink]
> [   12.660833]  stmmac_open+0x3d0/0x9f0 [stmmac]
> [   12.665186]  __dev_open+0xe7/0x180
> [   12.668594]  __dev_change_flags+0x174/0x1d0
> [   12.672773]  ? __thaw_task+0x40/0x40
> [   12.676348]  ? arch_stack_walk+0x9e/0xf0
> [   12.680266]  dev_change_flags+0x21/0x60
> [   12.684100]  devinet_ioctl+0x5e8/0x750
> [   12.687847]  inet_ioctl+0x190/0x1c0
> [   12.691335]  ? dev_ioctl+0x26d/0x4c0
> [   12.694907]  sock_do_ioctl+0x44/0x140
> [   12.698569]  ? alloc_empty_file+0x61/0xb0
> [   12.702572]  sock_ioctl+0x22c/0x320
> [   12.706061]  __x64_sys_ioctl+0x80/0xb0
> [   12.709808]  do_syscall_64+0x42/0x80
> [   12.713381]  entry_SYSCALL_64_after_hwframe+0x44/0xae
> 
> My git log:-
> dc25bc46886e (HEAD -> master) phy: maxlinear: add Maxlinear GPY115/21x/24x driver
> d059f32fcef1 fixup! net: pcs: xpcs: convert to phylink_pcs_ops
> 1a1a9c7267c1 net: pcs: xpcs: convert to phylink_pcs_ops
> ec25f3c80108 net: pcs: xpcs: convert to mdio_device
> 912a23a5768a net: pcs: xpcs: use mdiobus_c45_addr in xpcs_{read,write}
> af2bd0a4e32c net: pcs: xpcs: export xpcs_probe
> 7e4eea51132f net: pcs: xpcs: export xpcs_config_eee
> 37d5e086fc83 net: pcs: xpcs: export xpcs_validate
> d58487edb1a8 net: pcs: xpcs: make the checks related to the PHY interface mode stateless
> 2f0eba94af8d net: pcs: xpcs: there is only one PHY ID
> 67148dd06896 net: pcs: xpcs: delete shim definition for mdio_xpcs_get_ops()
> 5fe8e519e44f (origin/master, origin/HEAD) Merge git://git.kernel.org/pub/scm/linux/kernel/git/pablo/nf-next

Yikes, stupid me, I forgot to add this piece to the delta, the new
callback wasn't doing anything, but it also didn't warn me that the
static function is unused in my build:

-----------------------------[ cut here]-----------------------------
From 7a2b56091fd480ae1018ac964756368904a41a0c Mon Sep 17 00:00:00 2001
From: Vladimir Oltean <vladimir.oltean@nxp.com>
Date: Wed, 2 Jun 2021 17:54:58 +0300
Subject: [PATCH] fixup! fixup! net: pcs: xpcs: convert to phylink_pcs_ops

Signed-off-by: Vladimir Oltean <vladimir.oltean@nxp.com>
---
 drivers/net/ethernet/stmicro/stmmac/stmmac_main.c | 1 +
 1 file changed, 1 insertion(+)

diff --git a/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c b/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c
index 704aa91b145a..5b5edfdccab3 100644
--- a/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c
+++ b/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c
@@ -1137,6 +1137,7 @@ static void stmmac_mac_link_up(struct phylink_config *config,
 
 static const struct phylink_mac_ops stmmac_phylink_mac_ops = {
 	.validate = stmmac_validate,
+	.mac_config = stmmac_mac_config,
 	.mac_link_down = stmmac_mac_link_down,
 	.mac_link_up = stmmac_mac_link_up,
 };
-----------------------------[ cut here]-----------------------------

Re: [RFC PATCH v2 net-next 9/9] net: pcs: xpcs: convert to phylink_pcs_ops

[Wong Vee Khee]

On Wed, Jun 02, 2021 at 05:57:30PM +0300, Vladimir Oltean wrote:
> On Wed, Jun 02, 2021 at 10:43:36PM +0800, Wong Vee Khee wrote:
> > On Wed, Jun 02, 2021 at 05:02:33PM +0300, Vladimir Oltean wrote:
> > > On Wed, Jun 02, 2021 at 02:47:49PM +0100, Russell King (Oracle) wrote:
> > > > On Wed, Jun 02, 2021 at 04:43:21PM +0300, Vladimir Oltean wrote:
> > > > > On Tue, Jun 01, 2021 at 01:10:33PM +0100, Russell King (Oracle) wrote:
> > > > > > On Tue, Jun 01, 2021 at 03:33:25AM +0300, Vladimir Oltean wrote:
> > > > > > >  static const struct phylink_mac_ops stmmac_phylink_mac_ops = {
> > > > > > >  	.validate = stmmac_validate,
> > > > > > > -	.mac_pcs_get_state = stmmac_mac_pcs_get_state,
> > > > > > > -	.mac_config = stmmac_mac_config,
> > > > > > 
> > > > > > mac_config is still a required function.
> > > > > 
> > > > > This is correct, thanks.
> > > > > 
> > > > > VK, would you mind testing again with this extra patch added to the mix?
> > > > > If it works, I will add it to the series in v3, ordered properly.
> > > > > 
> > > > > -----------------------------[ cut here]-----------------------------
> > > > > From a79863027998451c73d5bbfaf1b77cf6097a110c Mon Sep 17 00:00:00 2001
> > > > > From: Vladimir Oltean <vladimir.oltean@nxp.com>
> > > > > Date: Wed, 2 Jun 2021 16:35:55 +0300
> > > > > Subject: [PATCH] net: phylink: allow the mac_config method to be missing if
> > > > >  pcs_ops are provided
> > > > > 
> > > > > The pcs_config method from struct phylink_pcs_ops does everything that
> > > > > the mac_config method from struct phylink_mac_ops used to do in the
> > > > > legacy approach of driving a MAC PCS. So allow drivers to not implement
> > > > > the mac_config method if there is nothing to do. Keep the method
> > > > > required for setups that do not provide pcs_ops.
> > > > > 
> > > > > Signed-off-by: Vladimir Oltean <vladimir.oltean@nxp.com>
> > > > > ---
> > > > >  drivers/net/phy/phylink.c | 9 +++++++++
> > > > >  1 file changed, 9 insertions(+)
> > > > > 
> > > > > diff --git a/drivers/net/phy/phylink.c b/drivers/net/phy/phylink.c
> > > > > index 96d8e88b4e46..a8842c6ce3a2 100644
> > > > > --- a/drivers/net/phy/phylink.c
> > > > > +++ b/drivers/net/phy/phylink.c
> > > > > @@ -415,6 +415,9 @@ static void phylink_resolve_flow(struct phylink_link_state *state)
> > > > >  static void phylink_mac_config(struct phylink *pl,
> > > > >  			       const struct phylink_link_state *state)
> > > > >  {
> > > > > +	if (!pl->mac_ops->mac_config)
> > > > > +		return;
> > > > > +
> > > > >  	phylink_dbg(pl,
> > > > >  		    "%s: mode=%s/%s/%s/%s adv=%*pb pause=%02x link=%u an=%u\n",
> > > > >  		    __func__, phylink_an_mode_str(pl->cur_link_an_mode),
> > > > > @@ -1192,6 +1195,12 @@ void phylink_start(struct phylink *pl)
> > > > >  
> > > > >  	ASSERT_RTNL();
> > > > >  
> > > > > +	/* The mac_ops::mac_config method may be absent only if the
> > > > > +	 * pcs_ops are present.
> > > > > +	 */
> > > > > +	if (WARN_ON_ONCE(!pl->mac_ops->mac_config && !pl->pcs_ops))
> > > > > +		return;
> > > > > +
> > > > >  	phylink_info(pl, "configuring for %s/%s link mode\n",
> > > > >  		     phylink_an_mode_str(pl->cur_link_an_mode),
> > > > >  		     phy_modes(pl->link_config.interface));
> > > > 
> > > > I would rather we didn't do that - I suspect your case is not the
> > > > common scenario, so please add a dummy function to stmmac instead.
> > > 
> > > Ok, in that case the only delta to be applied should be:
> > > 
> > > -----------------------------[ cut here]-----------------------------
> > > >From 998569108392befc591f790e46fe5dcd1d0b6278 Mon Sep 17 00:00:00 2001
> > > From: Vladimir Oltean <vladimir.oltean@nxp.com>
> > > Date: Wed, 2 Jun 2021 17:00:12 +0300
> > > Subject: [PATCH] fixup! net: pcs: xpcs: convert to phylink_pcs_ops
> > > 
> > > Signed-off-by: Vladimir Oltean <vladimir.oltean@nxp.com>
> > > ---
> > >  drivers/net/ethernet/stmicro/stmmac/stmmac_main.c | 6 ++++++
> > >  1 file changed, 6 insertions(+)
> > > 
> > > diff --git a/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c b/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c
> > > index d5685a74f3b7..704aa91b145a 100644
> > > --- a/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c
> > > +++ b/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c
> > > @@ -1000,6 +1000,12 @@ static void stmmac_validate(struct phylink_config *config,
> > >  		xpcs_validate(priv->hw->xpcs, supported, state);
> > >  }
> > >  
> > > +static void stmmac_mac_config(struct phylink_config *config, unsigned int mode,
> > > +			      const struct phylink_link_state *state)
> > > +{
> > > +	/* Nothing to do, xpcs_config() handles everything */
> > > +}
> > > +
> > >  static void stmmac_fpe_link_state_handle(struct stmmac_priv *priv, bool is_up)
> > >  {
> > >  	struct stmmac_fpe_cfg *fpe_cfg = priv->plat->fpe_cfg;
> > > -----------------------------[ cut here]-----------------------------
> > 
> > No luck.. I am still seeing the same kernel panic on my side with the
> > additional patch applied:
> > 
> > [   12.513652] intel-eth-pci 0000:00:1e.4 enp0s30f4: FPE workqueue start
> > [   12.520079] intel-eth-pci 0000:00:1e.4 enp0s30f4: configuring for inband/sgmii link mode
> > [   12.528141] BUG: kernel NULL pointer dereference, address: 0000000000000000
> > [   12.535072] #PF: supervisor instruction fetch in kernel mode
> > [   12.540719] #PF: error_code(0x0010) - not-present page
> > [   12.545843] PGD 0 P4D 0
> > [   12.548382] Oops: 0010 [#1] PREEMPT SMP NOPTI
> > [   12.552733] CPU: 3 PID: 2657 Comm: connmand Tainted: G     U            5.13.0-rc3-intel-lts #79
> > [   12.561485] Hardware name: Intel Corporation Tiger Lake Client Platform/TigerLake U DDR4 SODIMM RVP, BIOS TGLIFUI1.R00.3373.AF0.2009230546 09/23/2020
> > [   12.574803] RIP: 0010:0x0
> > [   12.577436] Code: Unable to access opcode bytes at RIP 0xffffffffffffffd6.
> > [   12.584280] RSP: 0018:ffffaa05404efb78 EFLAGS: 00010246
> > [   12.589489] RAX: 0000000000000000 RBX: ffffa0fb91758c00 RCX: 0000000000000000
> > [   12.596600] RDX: ffffaa05404efba0 RSI: 0000000000000002 RDI: ffffa0facc63c4d8
> > [   12.603711] RBP: ffffaa05404efba0 R08: 0000000000000000 R09: ffffaa05404ef888
> > [   12.610825] R10: 0000000000000001 R11: 0000000000000001 R12: 0000000000000001
> > [   12.617935] R13: ffffa0facc6388c0 R14: 0000000000000006 R15: 0000000000000001
> > [   12.625040] FS:  00007f4a6d5b87c0(0000) GS:ffffa0fc57f80000(0000) knlGS:0000000000000000
> > [   12.633096] CS:  0010 DS: 0000 ES: 0000 CR0: 0000000080050033
> > [   12.638825] CR2: ffffffffffffffd6 CR3: 000000010e3f6004 CR4: 0000000000770ee0
> > [   12.645932] PKRU: 55555554
> > [   12.648641] Call Trace:
> > [   12.651095]  phylink_major_config+0x5e/0x1a0 [phylink]
> > [   12.656219]  phylink_start+0x204/0x2c0 [phylink]
> > [   12.660833]  stmmac_open+0x3d0/0x9f0 [stmmac]
> > [   12.665186]  __dev_open+0xe7/0x180
> > [   12.668594]  __dev_change_flags+0x174/0x1d0
> > [   12.672773]  ? __thaw_task+0x40/0x40
> > [   12.676348]  ? arch_stack_walk+0x9e/0xf0
> > [   12.680266]  dev_change_flags+0x21/0x60
> > [   12.684100]  devinet_ioctl+0x5e8/0x750
> > [   12.687847]  inet_ioctl+0x190/0x1c0
> > [   12.691335]  ? dev_ioctl+0x26d/0x4c0
> > [   12.694907]  sock_do_ioctl+0x44/0x140
> > [   12.698569]  ? alloc_empty_file+0x61/0xb0
> > [   12.702572]  sock_ioctl+0x22c/0x320
> > [   12.706061]  __x64_sys_ioctl+0x80/0xb0
> > [   12.709808]  do_syscall_64+0x42/0x80
> > [   12.713381]  entry_SYSCALL_64_after_hwframe+0x44/0xae
> > 
> > My git log:-
> > dc25bc46886e (HEAD -> master) phy: maxlinear: add Maxlinear GPY115/21x/24x driver
> > d059f32fcef1 fixup! net: pcs: xpcs: convert to phylink_pcs_ops
> > 1a1a9c7267c1 net: pcs: xpcs: convert to phylink_pcs_ops
> > ec25f3c80108 net: pcs: xpcs: convert to mdio_device
> > 912a23a5768a net: pcs: xpcs: use mdiobus_c45_addr in xpcs_{read,write}
> > af2bd0a4e32c net: pcs: xpcs: export xpcs_probe
> > 7e4eea51132f net: pcs: xpcs: export xpcs_config_eee
> > 37d5e086fc83 net: pcs: xpcs: export xpcs_validate
> > d58487edb1a8 net: pcs: xpcs: make the checks related to the PHY interface mode stateless
> > 2f0eba94af8d net: pcs: xpcs: there is only one PHY ID
> > 67148dd06896 net: pcs: xpcs: delete shim definition for mdio_xpcs_get_ops()
> > 5fe8e519e44f (origin/master, origin/HEAD) Merge git://git.kernel.org/pub/scm/linux/kernel/git/pablo/nf-next
> 
> Yikes, stupid me, I forgot to add this piece to the delta, the new
> callback wasn't doing anything, but it also didn't warn me that the
> static function is unused in my build:
> 
> -----------------------------[ cut here]-----------------------------
> >From 7a2b56091fd480ae1018ac964756368904a41a0c Mon Sep 17 00:00:00 2001
> From: Vladimir Oltean <vladimir.oltean@nxp.com>
> Date: Wed, 2 Jun 2021 17:54:58 +0300
> Subject: [PATCH] fixup! fixup! net: pcs: xpcs: convert to phylink_pcs_ops
> 
> Signed-off-by: Vladimir Oltean <vladimir.oltean@nxp.com>
> ---
>  drivers/net/ethernet/stmicro/stmmac/stmmac_main.c | 1 +
>  1 file changed, 1 insertion(+)
> 
> diff --git a/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c b/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c
> index 704aa91b145a..5b5edfdccab3 100644
> --- a/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c
> +++ b/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c
> @@ -1137,6 +1137,7 @@ static void stmmac_mac_link_up(struct phylink_config *config,
>  
>  static const struct phylink_mac_ops stmmac_phylink_mac_ops = {
>  	.validate = stmmac_validate,
> +	.mac_config = stmmac_mac_config,
>  	.mac_link_down = stmmac_mac_link_down,
>  	.mac_link_up = stmmac_mac_link_up,
>  };
> -----------------------------[ cut here]-----------------------------

It works on my setup now after applying the last fixup patch! :)

 VK

Re: [RFC PATCH v2 net-next 9/9] net: pcs: xpcs: convert to phylink_pcs_ops

[Vladimir Oltean]

On Wed, Jun 02, 2021 at 11:10:56PM +0800, Wong Vee Khee wrote:
> On Wed, Jun 02, 2021 at 05:57:30PM +0300, Vladimir Oltean wrote:
> > On Wed, Jun 02, 2021 at 10:43:36PM +0800, Wong Vee Khee wrote:
> > > On Wed, Jun 02, 2021 at 05:02:33PM +0300, Vladimir Oltean wrote:
> > > > On Wed, Jun 02, 2021 at 02:47:49PM +0100, Russell King (Oracle) wrote:
> > > > > On Wed, Jun 02, 2021 at 04:43:21PM +0300, Vladimir Oltean wrote:
> > > > > > On Tue, Jun 01, 2021 at 01:10:33PM +0100, Russell King (Oracle) wrote:
> > > > > > > On Tue, Jun 01, 2021 at 03:33:25AM +0300, Vladimir Oltean wrote:
> > > > > > > >  static const struct phylink_mac_ops stmmac_phylink_mac_ops = {
> > > > > > > >  	.validate = stmmac_validate,
> > > > > > > > -	.mac_pcs_get_state = stmmac_mac_pcs_get_state,
> > > > > > > > -	.mac_config = stmmac_mac_config,
> > > > > > > 
> > > > > > > mac_config is still a required function.
> > > > > > 
> > > > > > This is correct, thanks.
> > > > > > 
> > > > > > VK, would you mind testing again with this extra patch added to the mix?
> > > > > > If it works, I will add it to the series in v3, ordered properly.
> > > > > > 
> > > > > > -----------------------------[ cut here]-----------------------------
> > > > > > From a79863027998451c73d5bbfaf1b77cf6097a110c Mon Sep 17 00:00:00 2001
> > > > > > From: Vladimir Oltean <vladimir.oltean@nxp.com>
> > > > > > Date: Wed, 2 Jun 2021 16:35:55 +0300
> > > > > > Subject: [PATCH] net: phylink: allow the mac_config method to be missing if
> > > > > >  pcs_ops are provided
> > > > > > 
> > > > > > The pcs_config method from struct phylink_pcs_ops does everything that
> > > > > > the mac_config method from struct phylink_mac_ops used to do in the
> > > > > > legacy approach of driving a MAC PCS. So allow drivers to not implement
> > > > > > the mac_config method if there is nothing to do. Keep the method
> > > > > > required for setups that do not provide pcs_ops.
> > > > > > 
> > > > > > Signed-off-by: Vladimir Oltean <vladimir.oltean@nxp.com>
> > > > > > ---
> > > > > >  drivers/net/phy/phylink.c | 9 +++++++++
> > > > > >  1 file changed, 9 insertions(+)
> > > > > > 
> > > > > > diff --git a/drivers/net/phy/phylink.c b/drivers/net/phy/phylink.c
> > > > > > index 96d8e88b4e46..a8842c6ce3a2 100644
> > > > > > --- a/drivers/net/phy/phylink.c
> > > > > > +++ b/drivers/net/phy/phylink.c
> > > > > > @@ -415,6 +415,9 @@ static void phylink_resolve_flow(struct phylink_link_state *state)
> > > > > >  static void phylink_mac_config(struct phylink *pl,
> > > > > >  			       const struct phylink_link_state *state)
> > > > > >  {
> > > > > > +	if (!pl->mac_ops->mac_config)
> > > > > > +		return;
> > > > > > +
> > > > > >  	phylink_dbg(pl,
> > > > > >  		    "%s: mode=%s/%s/%s/%s adv=%*pb pause=%02x link=%u an=%u\n",
> > > > > >  		    __func__, phylink_an_mode_str(pl->cur_link_an_mode),
> > > > > > @@ -1192,6 +1195,12 @@ void phylink_start(struct phylink *pl)
> > > > > >  
> > > > > >  	ASSERT_RTNL();
> > > > > >  
> > > > > > +	/* The mac_ops::mac_config method may be absent only if the
> > > > > > +	 * pcs_ops are present.
> > > > > > +	 */
> > > > > > +	if (WARN_ON_ONCE(!pl->mac_ops->mac_config && !pl->pcs_ops))
> > > > > > +		return;
> > > > > > +
> > > > > >  	phylink_info(pl, "configuring for %s/%s link mode\n",
> > > > > >  		     phylink_an_mode_str(pl->cur_link_an_mode),
> > > > > >  		     phy_modes(pl->link_config.interface));
> > > > > 
> > > > > I would rather we didn't do that - I suspect your case is not the
> > > > > common scenario, so please add a dummy function to stmmac instead.
> > > > 
> > > > Ok, in that case the only delta to be applied should be:
> > > > 
> > > > -----------------------------[ cut here]-----------------------------
> > > > >From 998569108392befc591f790e46fe5dcd1d0b6278 Mon Sep 17 00:00:00 2001
> > > > From: Vladimir Oltean <vladimir.oltean@nxp.com>
> > > > Date: Wed, 2 Jun 2021 17:00:12 +0300
> > > > Subject: [PATCH] fixup! net: pcs: xpcs: convert to phylink_pcs_ops
> > > > 
> > > > Signed-off-by: Vladimir Oltean <vladimir.oltean@nxp.com>
> > > > ---
> > > >  drivers/net/ethernet/stmicro/stmmac/stmmac_main.c | 6 ++++++
> > > >  1 file changed, 6 insertions(+)
> > > > 
> > > > diff --git a/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c b/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c
> > > > index d5685a74f3b7..704aa91b145a 100644
> > > > --- a/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c
> > > > +++ b/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c
> > > > @@ -1000,6 +1000,12 @@ static void stmmac_validate(struct phylink_config *config,
> > > >  		xpcs_validate(priv->hw->xpcs, supported, state);
> > > >  }
> > > >  
> > > > +static void stmmac_mac_config(struct phylink_config *config, unsigned int mode,
> > > > +			      const struct phylink_link_state *state)
> > > > +{
> > > > +	/* Nothing to do, xpcs_config() handles everything */
> > > > +}
> > > > +
> > > >  static void stmmac_fpe_link_state_handle(struct stmmac_priv *priv, bool is_up)
> > > >  {
> > > >  	struct stmmac_fpe_cfg *fpe_cfg = priv->plat->fpe_cfg;
> > > > -----------------------------[ cut here]-----------------------------
> > > 
> > > No luck.. I am still seeing the same kernel panic on my side with the
> > > additional patch applied:
> > > 
> > > [   12.513652] intel-eth-pci 0000:00:1e.4 enp0s30f4: FPE workqueue start
> > > [   12.520079] intel-eth-pci 0000:00:1e.4 enp0s30f4: configuring for inband/sgmii link mode
> > > [   12.528141] BUG: kernel NULL pointer dereference, address: 0000000000000000
> > > [   12.535072] #PF: supervisor instruction fetch in kernel mode
> > > [   12.540719] #PF: error_code(0x0010) - not-present page
> > > [   12.545843] PGD 0 P4D 0
> > > [   12.548382] Oops: 0010 [#1] PREEMPT SMP NOPTI
> > > [   12.552733] CPU: 3 PID: 2657 Comm: connmand Tainted: G     U            5.13.0-rc3-intel-lts #79
> > > [   12.561485] Hardware name: Intel Corporation Tiger Lake Client Platform/TigerLake U DDR4 SODIMM RVP, BIOS TGLIFUI1.R00.3373.AF0.2009230546 09/23/2020
> > > [   12.574803] RIP: 0010:0x0
> > > [   12.577436] Code: Unable to access opcode bytes at RIP 0xffffffffffffffd6.
> > > [   12.584280] RSP: 0018:ffffaa05404efb78 EFLAGS: 00010246
> > > [   12.589489] RAX: 0000000000000000 RBX: ffffa0fb91758c00 RCX: 0000000000000000
> > > [   12.596600] RDX: ffffaa05404efba0 RSI: 0000000000000002 RDI: ffffa0facc63c4d8
> > > [   12.603711] RBP: ffffaa05404efba0 R08: 0000000000000000 R09: ffffaa05404ef888
> > > [   12.610825] R10: 0000000000000001 R11: 0000000000000001 R12: 0000000000000001
> > > [   12.617935] R13: ffffa0facc6388c0 R14: 0000000000000006 R15: 0000000000000001
> > > [   12.625040] FS:  00007f4a6d5b87c0(0000) GS:ffffa0fc57f80000(0000) knlGS:0000000000000000
> > > [   12.633096] CS:  0010 DS: 0000 ES: 0000 CR0: 0000000080050033
> > > [   12.638825] CR2: ffffffffffffffd6 CR3: 000000010e3f6004 CR4: 0000000000770ee0
> > > [   12.645932] PKRU: 55555554
> > > [   12.648641] Call Trace:
> > > [   12.651095]  phylink_major_config+0x5e/0x1a0 [phylink]
> > > [   12.656219]  phylink_start+0x204/0x2c0 [phylink]
> > > [   12.660833]  stmmac_open+0x3d0/0x9f0 [stmmac]
> > > [   12.665186]  __dev_open+0xe7/0x180
> > > [   12.668594]  __dev_change_flags+0x174/0x1d0
> > > [   12.672773]  ? __thaw_task+0x40/0x40
> > > [   12.676348]  ? arch_stack_walk+0x9e/0xf0
> > > [   12.680266]  dev_change_flags+0x21/0x60
> > > [   12.684100]  devinet_ioctl+0x5e8/0x750
> > > [   12.687847]  inet_ioctl+0x190/0x1c0
> > > [   12.691335]  ? dev_ioctl+0x26d/0x4c0
> > > [   12.694907]  sock_do_ioctl+0x44/0x140
> > > [   12.698569]  ? alloc_empty_file+0x61/0xb0
> > > [   12.702572]  sock_ioctl+0x22c/0x320
> > > [   12.706061]  __x64_sys_ioctl+0x80/0xb0
> > > [   12.709808]  do_syscall_64+0x42/0x80
> > > [   12.713381]  entry_SYSCALL_64_after_hwframe+0x44/0xae
> > > 
> > > My git log:-
> > > dc25bc46886e (HEAD -> master) phy: maxlinear: add Maxlinear GPY115/21x/24x driver
> > > d059f32fcef1 fixup! net: pcs: xpcs: convert to phylink_pcs_ops
> > > 1a1a9c7267c1 net: pcs: xpcs: convert to phylink_pcs_ops
> > > ec25f3c80108 net: pcs: xpcs: convert to mdio_device
> > > 912a23a5768a net: pcs: xpcs: use mdiobus_c45_addr in xpcs_{read,write}
> > > af2bd0a4e32c net: pcs: xpcs: export xpcs_probe
> > > 7e4eea51132f net: pcs: xpcs: export xpcs_config_eee
> > > 37d5e086fc83 net: pcs: xpcs: export xpcs_validate
> > > d58487edb1a8 net: pcs: xpcs: make the checks related to the PHY interface mode stateless
> > > 2f0eba94af8d net: pcs: xpcs: there is only one PHY ID
> > > 67148dd06896 net: pcs: xpcs: delete shim definition for mdio_xpcs_get_ops()
> > > 5fe8e519e44f (origin/master, origin/HEAD) Merge git://git.kernel.org/pub/scm/linux/kernel/git/pablo/nf-next
> > 
> > Yikes, stupid me, I forgot to add this piece to the delta, the new
> > callback wasn't doing anything, but it also didn't warn me that the
> > static function is unused in my build:
> > 
> > -----------------------------[ cut here]-----------------------------
> > >From 7a2b56091fd480ae1018ac964756368904a41a0c Mon Sep 17 00:00:00 2001
> > From: Vladimir Oltean <vladimir.oltean@nxp.com>
> > Date: Wed, 2 Jun 2021 17:54:58 +0300
> > Subject: [PATCH] fixup! fixup! net: pcs: xpcs: convert to phylink_pcs_ops
> > 
> > Signed-off-by: Vladimir Oltean <vladimir.oltean@nxp.com>
> > ---
> >  drivers/net/ethernet/stmicro/stmmac/stmmac_main.c | 1 +
> >  1 file changed, 1 insertion(+)
> > 
> > diff --git a/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c b/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c
> > index 704aa91b145a..5b5edfdccab3 100644
> > --- a/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c
> > +++ b/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c
> > @@ -1137,6 +1137,7 @@ static void stmmac_mac_link_up(struct phylink_config *config,
> >  
> >  static const struct phylink_mac_ops stmmac_phylink_mac_ops = {
> >  	.validate = stmmac_validate,
> > +	.mac_config = stmmac_mac_config,
> >  	.mac_link_down = stmmac_mac_link_down,
> >  	.mac_link_up = stmmac_mac_link_up,
> >  };
> > -----------------------------[ cut here]-----------------------------
> 
> It works on my setup now after applying the last fixup patch! :)

Thanks again for testing.
Just for clarity, "it works" includes traffic at 10/100/1000, right?
If I'm not mistaken, you tested with sgmii and MLO_INBAND_AN, correct?
On the sja1105 I am testing with sgmii and no MLO_INBAND_AN, so not
exactly the same setup (that, plus my hardware needs a few more changes
still).

Re: [RFC PATCH v2 net-next 9/9] net: pcs: xpcs: convert to phylink_pcs_ops

[Wong Vee Khee]

On Wed, Jun 02, 2021 at 06:14:47PM +0300, Vladimir Oltean wrote:
> On Wed, Jun 02, 2021 at 11:10:56PM +0800, Wong Vee Khee wrote:
> > On Wed, Jun 02, 2021 at 05:57:30PM +0300, Vladimir Oltean wrote:
> > > On Wed, Jun 02, 2021 at 10:43:36PM +0800, Wong Vee Khee wrote:
> > > > On Wed, Jun 02, 2021 at 05:02:33PM +0300, Vladimir Oltean wrote:
> > > > > On Wed, Jun 02, 2021 at 02:47:49PM +0100, Russell King (Oracle) wrote:
> > > > > > On Wed, Jun 02, 2021 at 04:43:21PM +0300, Vladimir Oltean wrote:
> > > > > > > On Tue, Jun 01, 2021 at 01:10:33PM +0100, Russell King (Oracle) wrote:
> > > > > > > > On Tue, Jun 01, 2021 at 03:33:25AM +0300, Vladimir Oltean wrote:
> > > > > > > > >  static const struct phylink_mac_ops stmmac_phylink_mac_ops = {
> > > > > > > > >  	.validate = stmmac_validate,
> > > > > > > > > -	.mac_pcs_get_state = stmmac_mac_pcs_get_state,
> > > > > > > > > -	.mac_config = stmmac_mac_config,
> > > > > > > > 
> > > > > > > > mac_config is still a required function.
> > > > > > > 
> > > > > > > This is correct, thanks.
> > > > > > > 
> > > > > > > VK, would you mind testing again with this extra patch added to the mix?
> > > > > > > If it works, I will add it to the series in v3, ordered properly.
> > > > > > > 
> > > > > > > -----------------------------[ cut here]-----------------------------
> > > > > > > From a79863027998451c73d5bbfaf1b77cf6097a110c Mon Sep 17 00:00:00 2001
> > > > > > > From: Vladimir Oltean <vladimir.oltean@nxp.com>
> > > > > > > Date: Wed, 2 Jun 2021 16:35:55 +0300
> > > > > > > Subject: [PATCH] net: phylink: allow the mac_config method to be missing if
> > > > > > >  pcs_ops are provided
> > > > > > > 
> > > > > > > The pcs_config method from struct phylink_pcs_ops does everything that
> > > > > > > the mac_config method from struct phylink_mac_ops used to do in the
> > > > > > > legacy approach of driving a MAC PCS. So allow drivers to not implement
> > > > > > > the mac_config method if there is nothing to do. Keep the method
> > > > > > > required for setups that do not provide pcs_ops.
> > > > > > > 
> > > > > > > Signed-off-by: Vladimir Oltean <vladimir.oltean@nxp.com>
> > > > > > > ---
> > > > > > >  drivers/net/phy/phylink.c | 9 +++++++++
> > > > > > >  1 file changed, 9 insertions(+)
> > > > > > > 
> > > > > > > diff --git a/drivers/net/phy/phylink.c b/drivers/net/phy/phylink.c
> > > > > > > index 96d8e88b4e46..a8842c6ce3a2 100644
> > > > > > > --- a/drivers/net/phy/phylink.c
> > > > > > > +++ b/drivers/net/phy/phylink.c
> > > > > > > @@ -415,6 +415,9 @@ static void phylink_resolve_flow(struct phylink_link_state *state)
> > > > > > >  static void phylink_mac_config(struct phylink *pl,
> > > > > > >  			       const struct phylink_link_state *state)
> > > > > > >  {
> > > > > > > +	if (!pl->mac_ops->mac_config)
> > > > > > > +		return;
> > > > > > > +
> > > > > > >  	phylink_dbg(pl,
> > > > > > >  		    "%s: mode=%s/%s/%s/%s adv=%*pb pause=%02x link=%u an=%u\n",
> > > > > > >  		    __func__, phylink_an_mode_str(pl->cur_link_an_mode),
> > > > > > > @@ -1192,6 +1195,12 @@ void phylink_start(struct phylink *pl)
> > > > > > >  
> > > > > > >  	ASSERT_RTNL();
> > > > > > >  
> > > > > > > +	/* The mac_ops::mac_config method may be absent only if the
> > > > > > > +	 * pcs_ops are present.
> > > > > > > +	 */
> > > > > > > +	if (WARN_ON_ONCE(!pl->mac_ops->mac_config && !pl->pcs_ops))
> > > > > > > +		return;
> > > > > > > +
> > > > > > >  	phylink_info(pl, "configuring for %s/%s link mode\n",
> > > > > > >  		     phylink_an_mode_str(pl->cur_link_an_mode),
> > > > > > >  		     phy_modes(pl->link_config.interface));
> > > > > > 
> > > > > > I would rather we didn't do that - I suspect your case is not the
> > > > > > common scenario, so please add a dummy function to stmmac instead.
> > > > > 
> > > > > Ok, in that case the only delta to be applied should be:
> > > > > 
> > > > > -----------------------------[ cut here]-----------------------------
> > > > > >From 998569108392befc591f790e46fe5dcd1d0b6278 Mon Sep 17 00:00:00 2001
> > > > > From: Vladimir Oltean <vladimir.oltean@nxp.com>
> > > > > Date: Wed, 2 Jun 2021 17:00:12 +0300
> > > > > Subject: [PATCH] fixup! net: pcs: xpcs: convert to phylink_pcs_ops
> > > > > 
> > > > > Signed-off-by: Vladimir Oltean <vladimir.oltean@nxp.com>
> > > > > ---
> > > > >  drivers/net/ethernet/stmicro/stmmac/stmmac_main.c | 6 ++++++
> > > > >  1 file changed, 6 insertions(+)
> > > > > 
> > > > > diff --git a/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c b/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c
> > > > > index d5685a74f3b7..704aa91b145a 100644
> > > > > --- a/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c
> > > > > +++ b/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c
> > > > > @@ -1000,6 +1000,12 @@ static void stmmac_validate(struct phylink_config *config,
> > > > >  		xpcs_validate(priv->hw->xpcs, supported, state);
> > > > >  }
> > > > >  
> > > > > +static void stmmac_mac_config(struct phylink_config *config, unsigned int mode,
> > > > > +			      const struct phylink_link_state *state)
> > > > > +{
> > > > > +	/* Nothing to do, xpcs_config() handles everything */
> > > > > +}
> > > > > +
> > > > >  static void stmmac_fpe_link_state_handle(struct stmmac_priv *priv, bool is_up)
> > > > >  {
> > > > >  	struct stmmac_fpe_cfg *fpe_cfg = priv->plat->fpe_cfg;
> > > > > -----------------------------[ cut here]-----------------------------
> > > > 
> > > > No luck.. I am still seeing the same kernel panic on my side with the
> > > > additional patch applied:
> > > > 
> > > > [   12.513652] intel-eth-pci 0000:00:1e.4 enp0s30f4: FPE workqueue start
> > > > [   12.520079] intel-eth-pci 0000:00:1e.4 enp0s30f4: configuring for inband/sgmii link mode
> > > > [   12.528141] BUG: kernel NULL pointer dereference, address: 0000000000000000
> > > > [   12.535072] #PF: supervisor instruction fetch in kernel mode
> > > > [   12.540719] #PF: error_code(0x0010) - not-present page
> > > > [   12.545843] PGD 0 P4D 0
> > > > [   12.548382] Oops: 0010 [#1] PREEMPT SMP NOPTI
> > > > [   12.552733] CPU: 3 PID: 2657 Comm: connmand Tainted: G     U            5.13.0-rc3-intel-lts #79
> > > > [   12.561485] Hardware name: Intel Corporation Tiger Lake Client Platform/TigerLake U DDR4 SODIMM RVP, BIOS TGLIFUI1.R00.3373.AF0.2009230546 09/23/2020
> > > > [   12.574803] RIP: 0010:0x0
> > > > [   12.577436] Code: Unable to access opcode bytes at RIP 0xffffffffffffffd6.
> > > > [   12.584280] RSP: 0018:ffffaa05404efb78 EFLAGS: 00010246
> > > > [   12.589489] RAX: 0000000000000000 RBX: ffffa0fb91758c00 RCX: 0000000000000000
> > > > [   12.596600] RDX: ffffaa05404efba0 RSI: 0000000000000002 RDI: ffffa0facc63c4d8
> > > > [   12.603711] RBP: ffffaa05404efba0 R08: 0000000000000000 R09: ffffaa05404ef888
> > > > [   12.610825] R10: 0000000000000001 R11: 0000000000000001 R12: 0000000000000001
> > > > [   12.617935] R13: ffffa0facc6388c0 R14: 0000000000000006 R15: 0000000000000001
> > > > [   12.625040] FS:  00007f4a6d5b87c0(0000) GS:ffffa0fc57f80000(0000) knlGS:0000000000000000
> > > > [   12.633096] CS:  0010 DS: 0000 ES: 0000 CR0: 0000000080050033
> > > > [   12.638825] CR2: ffffffffffffffd6 CR3: 000000010e3f6004 CR4: 0000000000770ee0
> > > > [   12.645932] PKRU: 55555554
> > > > [   12.648641] Call Trace:
> > > > [   12.651095]  phylink_major_config+0x5e/0x1a0 [phylink]
> > > > [   12.656219]  phylink_start+0x204/0x2c0 [phylink]
> > > > [   12.660833]  stmmac_open+0x3d0/0x9f0 [stmmac]
> > > > [   12.665186]  __dev_open+0xe7/0x180
> > > > [   12.668594]  __dev_change_flags+0x174/0x1d0
> > > > [   12.672773]  ? __thaw_task+0x40/0x40
> > > > [   12.676348]  ? arch_stack_walk+0x9e/0xf0
> > > > [   12.680266]  dev_change_flags+0x21/0x60
> > > > [   12.684100]  devinet_ioctl+0x5e8/0x750
> > > > [   12.687847]  inet_ioctl+0x190/0x1c0
> > > > [   12.691335]  ? dev_ioctl+0x26d/0x4c0
> > > > [   12.694907]  sock_do_ioctl+0x44/0x140
> > > > [   12.698569]  ? alloc_empty_file+0x61/0xb0
> > > > [   12.702572]  sock_ioctl+0x22c/0x320
> > > > [   12.706061]  __x64_sys_ioctl+0x80/0xb0
> > > > [   12.709808]  do_syscall_64+0x42/0x80
> > > > [   12.713381]  entry_SYSCALL_64_after_hwframe+0x44/0xae
> > > > 
> > > > My git log:-
> > > > dc25bc46886e (HEAD -> master) phy: maxlinear: add Maxlinear GPY115/21x/24x driver
> > > > d059f32fcef1 fixup! net: pcs: xpcs: convert to phylink_pcs_ops
> > > > 1a1a9c7267c1 net: pcs: xpcs: convert to phylink_pcs_ops
> > > > ec25f3c80108 net: pcs: xpcs: convert to mdio_device
> > > > 912a23a5768a net: pcs: xpcs: use mdiobus_c45_addr in xpcs_{read,write}
> > > > af2bd0a4e32c net: pcs: xpcs: export xpcs_probe
> > > > 7e4eea51132f net: pcs: xpcs: export xpcs_config_eee
> > > > 37d5e086fc83 net: pcs: xpcs: export xpcs_validate
> > > > d58487edb1a8 net: pcs: xpcs: make the checks related to the PHY interface mode stateless
> > > > 2f0eba94af8d net: pcs: xpcs: there is only one PHY ID
> > > > 67148dd06896 net: pcs: xpcs: delete shim definition for mdio_xpcs_get_ops()
> > > > 5fe8e519e44f (origin/master, origin/HEAD) Merge git://git.kernel.org/pub/scm/linux/kernel/git/pablo/nf-next
> > > 
> > > Yikes, stupid me, I forgot to add this piece to the delta, the new
> > > callback wasn't doing anything, but it also didn't warn me that the
> > > static function is unused in my build:
> > > 
> > > -----------------------------[ cut here]-----------------------------
> > > >From 7a2b56091fd480ae1018ac964756368904a41a0c Mon Sep 17 00:00:00 2001
> > > From: Vladimir Oltean <vladimir.oltean@nxp.com>
> > > Date: Wed, 2 Jun 2021 17:54:58 +0300
> > > Subject: [PATCH] fixup! fixup! net: pcs: xpcs: convert to phylink_pcs_ops
> > > 
> > > Signed-off-by: Vladimir Oltean <vladimir.oltean@nxp.com>
> > > ---
> > >  drivers/net/ethernet/stmicro/stmmac/stmmac_main.c | 1 +
> > >  1 file changed, 1 insertion(+)
> > > 
> > > diff --git a/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c b/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c
> > > index 704aa91b145a..5b5edfdccab3 100644
> > > --- a/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c
> > > +++ b/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c
> > > @@ -1137,6 +1137,7 @@ static void stmmac_mac_link_up(struct phylink_config *config,
> > >  
> > >  static const struct phylink_mac_ops stmmac_phylink_mac_ops = {
> > >  	.validate = stmmac_validate,
> > > +	.mac_config = stmmac_mac_config,
> > >  	.mac_link_down = stmmac_mac_link_down,
> > >  	.mac_link_up = stmmac_mac_link_up,
> > >  };
> > > -----------------------------[ cut here]-----------------------------
> > 
> > It works on my setup now after applying the last fixup patch! :)
> 
> Thanks again for testing.
> Just for clarity, "it works" includes traffic at 10/100/1000, right?
> If I'm not mistaken, you tested with sgmii and MLO_INBAND_AN, correct?
> On the sja1105 I am testing with sgmii and no MLO_INBAND_AN, so not
> exactly the same setup (that, plus my hardware needs a few more changes
> still).

Yes, I tested by performing ping on different link speeds (10/100/1000M),
with SGMII and Auto-negotiation enabled. Link speed is changed manually
using ethtool.

 VK