net: dsa: mxl862xx: SerDes ports
Daniel Golle
maintainerv5 cover net: dsa: mxl862xx: SerDes ports
Add support for the two SerDes PCS interfaces of the MxL862xx switch ICs, which can both either be used to connect PHYs or SFP cages, or as CPU port(s). 1000Base-X, 2500Base-X, 10GBase-R, 10GBase-KR, SGMII, QSGMII and USXGMII (single 10G or quad 2.5G) are supported.
The firmware only added the API to directly control the PCS as of version 1.0.84, so the PCS features are gated behind a version check.
As the driver is growing do some refactoring to break out the phylink parts into mxl862xx-phylink.h.
---
Changes since v4:
* use FIELD_GET/FIELD_PREP and macro definitions for the bitfields instead of endian-aware structs with bit-sized members
* do not error out on old firmware, so driver at least probes and CPU port keeps working in firmware-configured mode. Issue a warning instead.
Changes since v3:
* replace atomic_t serdes_refcount with a plain int guarded by a new serdes_lock mutex
Changes since v2:
* get rid of endian-specific union handling firmware version
* replace serdes_active bitmap with atomic_t serdes_refcount
* defer mpcs->interface assignment until after firmware ack
* handle firmware error codes in pcs_config
* set st.usx_lane_mode in pcs_get_state
* set lu.usx_subport and lu.usx_lane_mode in pcs_link_up
* use phylink_mii_c22_pcs_encode_advertisement() in CL37 adv
* rework commit message
Changes since v1:
* drop custom ethtool stats (former patch 5/5)
* add __{LE,BE}_BITFIELD layouts to ABI structs
* per-sub-port QSGMII AN restart via usx_subport / usx_lane_mode
* shared-SerDes refcount in pcs_disable via per-XPCS slot bitmap
* let every sub-port call pcs_config
* cache phy_interface_t instead of firmware type
* skip pcs_link_up when inband-AN enabled
* gate phylink_get_caps SerDes modes on same FW version as select_pcs
* interpret xpcs_pcs_cfg.result as signed (s16)
* drop dead MXL862XX_PCS_PORT macro
* drop misleading "downshift detection" line from commit message
Daniel Golle (4):
net: dsa: mxl862xx: store firmware version for feature gating
net: dsa: mxl862xx: move phylink stubs to mxl862xx-phylink.c
net: dsa: mxl862xx: move API macros to mxl862xx-host.h
net: dsa: mxl862xx: add support for SerDes ports
drivers/net/dsa/mxl862xx/Makefile | 2 +- drivers/net/dsa/mxl862xx/mxl862xx-api.h | 215 ++++++++++ drivers/net/dsa/mxl862xx/mxl862xx-cmd.h | 9 + drivers/net/dsa/mxl862xx/mxl862xx-host.h | 8 + drivers/net/dsa/mxl862xx/mxl862xx-phylink.c | 433 ++++++++++++++++++++ drivers/net/dsa/mxl862xx/mxl862xx-phylink.h | 21 + drivers/net/dsa/mxl862xx/mxl862xx.c | 55 +-- drivers/net/dsa/mxl862xx/mxl862xx.h | 57 +++ 8 files changed, 754 insertions(+), 46 deletions(-) create mode 100644 drivers/net/dsa/mxl862xx/mxl862xx-phylink.c create mode 100644 drivers/net/dsa/mxl862xx/mxl862xx-phylink.h
Signature · 1 line
2.54.0
Daniel Golle
maintainerPATCH v5 1/4 net: dsa: mxl862xx: store firmware version for feature gating
Query the firmware version at init (already done in wait_ready), cache it in priv->fw_version, and provide MXL862XX_FW_VER_MIN() for version-gated code paths throughout the driver.
MXL862XX_FW_VER() packs major/minor/revision into a u32 with bitwise shifts so that versions compare with natural ordering, independent of host endianness.
---
v5: no changes
v4: no changes
v3: use bitwise shifts in macro instead of endian-specific union
v2: no changes
drivers/net/dsa/mxl862xx/mxl862xx.c | 3 +++ drivers/net/dsa/mxl862xx/mxl862xx.h | 23 +++++++++++++++++++++++ 2 files changed, 26 insertions(+)
drivers/net/dsa/mxl862xx/mxl862xx.c +3 -0
@@ -257,6 +257,9 @@ static int mxl862xx_wait_ready(struct dsa_switch *ds) ver.iv_major, ver.iv_minor,le16_to_cpu (ver.iv_revision), le32_to_cpu(ver.iv_build_num)); + priv->fw_version.major = ver.iv_major; + priv->fw_version.minor = ver.iv_minor; + priv->fw_version.revision =le16_to_cpu (ver.iv_revision);return 0 ; not_ready_yet:
drivers/net/dsa/mxl862xx/mxl862xx.h +23 -0
@@ -3,6 +3,7 @@#ifndef __MXL862XX_H#define __MXL862XX_H +#include <asm/byteorder.h> #include <linux/mdio.h> #include <linux/workqueue.h> #include <net/dsa.h>
@@ -241,6 +242,25 @@ struct mxl862xx_port {spinlock_t stats_lock;/* protects stats accumulators */ }; +/** +* struct mxl862xx_fw_version - firmware version for comparison and display +* @major: firmware major version +* @minor: firmware minor version +* @revision: firmware revision number +*/ +struct mxl862xx_fw_version { +u8 major; +u8 minor; +u16 revision; +}; + +#define MXL862XX_FW_VER (maj, min, rev) \ + (((u32)(maj) << 24) | ((u32)(min) << 16) | (rev)) +#define MXL862XX_FW_VER_MIN (priv, maj, min, rev) \ + (MXL862XX_FW_VER((priv)->fw_version.major, (priv)->fw_version.minor, \ + (priv)->fw_version.revision) >= \ + MXL862XX_FW_VER(maj, min, rev)) +/* Bit indices for struct mxl862xx_priv::flags */ #define MXL862XX_FLAG_CRC_ERR 0#define MXL862XX_FLAG_WORK_STOPPED 1
@@ -258,6 +278,8 @@ struct mxl862xx_port {* @drop_meter: index of the single shared zero-rate firmware meter * used to unconditionally drop traffic (used to block * flooding) +* @fw_version: cached firmware version, populated at probe and +* compared with MXL862XX_FW_VER_MIN() * @ports: per-port state, indexed by switch port number * @bridges: maps DSA bridge number to firmware bridge ID; * zero means no firmware bridge allocated for that
@@ -275,6 +297,7 @@ struct mxl862xx_priv {struct work_struct crc_err_work;unsigned long flags;u16 drop_meter; +struct mxl862xx_fw_version fw_version;struct mxl862xx_port ports[MXL862XX_MAX_PORTS ];u16 bridges[MXL862XX_MAX_BRIDGES +1 ];u16 evlan_ingress_size;
Signature · 1 line
2.54.0
Daniel Golle
maintainerPATCH v5 2/4 net: dsa: mxl862xx: move phylink stubs to mxl862xx-phylink.c
In reply to Daniel GolleMove the phylink MAC operations and get_caps callback from mxl862xx.c into a dedicated mxl862xx-phylink.c file. This prepares for the SerDes PCS implementation which adds substantial phylink/PCS code -- keeping it in a separate file avoids function-position churn in the main driver file.
No functional change.
---
v5: no changes
v4: no changes
v3: no changes
v2: no changes
drivers/net/dsa/mxl862xx/Makefile | 2 +- drivers/net/dsa/mxl862xx/mxl862xx-phylink.c | 51 +++++++++++++++++++++ drivers/net/dsa/mxl862xx/mxl862xx-phylink.h | 14 ++++++ drivers/net/dsa/mxl862xx/mxl862xx.c | 38 +-------------- 4 files changed, 67 insertions(+), 38 deletions(-) create mode 100644 drivers/net/dsa/mxl862xx/mxl862xx-phylink.c create mode 100644 drivers/net/dsa/mxl862xx/mxl862xx-phylink.h
drivers/net/dsa/mxl862xx/Makefile +1 -1
@@ -1,3 +1,3 @@ # SPDX-License-Identifier: GPL-2.0 obj-$(CONFIG_NET_DSA_MXL862) += mxl862xx_dsa.o -mxl862xx_dsa-y := mxl862xx.o mxl862xx-host.o +mxl862xx_dsa-y := mxl862xx.o mxl862xx-host.o mxl862xx-phylink.o
drivers/net/dsa/mxl862xx/mxl862xx-phylink.c new file +51 -0
51 lines not on this page · raw message
drivers/net/dsa/mxl862xx/mxl862xx-phylink.h new file +14 -0
@@ -0,0 +1,14 @@ +/* SPDX-License-Identifier: GPL-2.0-or-later */ + +#ifndef __MXL862XX_PHYLINK_H +#define __MXL862XX_PHYLINK_H + +#include <linux/phylink.h> + +#include "mxl862xx.h" + +extern const struct phylink_mac_ops mxl862xx_phylink_mac_ops; +void mxl862xx_phylink_get_caps (struct dsa_switch *ds,int port, +struct phylink_config *config); + +#endif /* __MXL862XX_PHYLINK_H */
drivers/net/dsa/mxl862xx/mxl862xx.c +1 -37
@@ -22,6 +22,7 @@#include "mxl862xx-api.h" #include "mxl862xx-cmd.h" #include "mxl862xx-host.h" +#include "mxl862xx-phylink.h" #define MXL862XX_API_WRITE (dev, cmd, data) \ mxl862xx_api_wrap(dev, cmd, &(data), sizeof((data)), false, false)
@@ -1424,16 +1425,6 @@ static void mxl862xx_port_teardown(struct dsa_switch *ds, int port) priv->ports[port].setup_done = false; } -static void mxl862xx_phylink_get_caps (struct dsa_switch *ds,int port, -struct phylink_config *config) -{ - config->mac_capabilities =MAC_ASYM_PAUSE |MAC_SYM_PAUSE |MAC_10 | -MAC_100 |MAC_1000 |MAC_2500FD ; - -__set_bit (PHY_INTERFACE_MODE_INTERNAL , - config->supported_interfaces); -} -static int mxl862xx_get_fid (struct dsa_switch *ds,struct dsa_db db) {struct mxl862xx_priv *priv = ds->priv;
@@ -2099,33 +2090,6 @@ static const struct dsa_switch_ops mxl862xx_switch_ops = { .get_stats64 = mxl862xx_get_stats64, }; -static void mxl862xx_phylink_mac_config (struct phylink_config *config, -unsigned int mode, -const struct phylink_link_state *state) -{ -} - -static void mxl862xx_phylink_mac_link_down (struct phylink_config *config, -unsigned int mode, -phy_interface_t interface) -{ -} - -static void mxl862xx_phylink_mac_link_up (struct phylink_config *config, -struct phy_device *phydev, -unsigned int mode, -phy_interface_t interface, -int speed,int duplex, -bool tx_pause,bool rx_pause) -{ -} - -static const struct phylink_mac_ops mxl862xx_phylink_mac_ops = { - .mac_config = mxl862xx_phylink_mac_config, - .mac_link_down = mxl862xx_phylink_mac_link_down, - .mac_link_up = mxl862xx_phylink_mac_link_up, -}; -static int mxl862xx_probe (struct mdio_device *mdiodev) {struct device *dev = &mdiodev->dev;
Signature · 1 line
2.54.0
Simon Horman
reviewerRe: [PATCH net-next v5 2/4] net: dsa: mxl862xx: move phylink stubs to mxl862xx-phylink.c
This is an AI-generated review of your patch. The human sending this email has considered the AI review valid, or at least plausible. Full review at: https://sashiko.dev
---
net: dsa: mxl862xx: move phylink stubs to mxl862xx-phylink.c
Move the phylink MAC operations and get_caps callback from mxl862xx.c into a dedicated mxl862xx-phylink.c file. This prepares for the SerDes PCS implementation which adds substantial phylink/PCS code.
drivers/net/dsa/mxl862xx/mxl862xx.c
Quoted 4 lines
drivers/net/dsa/mxl862xx/mxl862xx.c +0
-0
[ ... ]
[Severity: Critical]
This is a pre-existing issue, but the crc_err_work can execute after dsa_unregister_switch(), leading to a use-after-free of ds->dst and dsa_port structures.
In mxl862xx_remove(), dsa_unregister_switch() is called before the asynchronous crc_err_work is cancelled via mxl862xx_host_shutdown():
drivers/net/dsa/mxl862xx/mxl862xx.c:mxl862xx_remove() {
...
dsa_unregister_switch(ds);
mxl862xx_host_shutdown(priv);
...
}dsa_unregister_switch() tears down the DSA tree, frees all dsa_port objects, and drops the reference to ds->dst.
If mxl862xx_crc_err_work_fn executes concurrently or after dsa_unregister_switch() has returned, it traverses already freed dsa_port entries:
drivers/net/dsa/mxl862xx/mxl862xx-host.c:mxl862xx_crc_err_work_fn() {
...
dsa_switch_for_each_cpu_port(dp, priv->ds)
dev_close(dp->conduit);
...
}Could this order in mxl862xx_remove() result in a use-after-free, and should mxl862xx_host_shutdown() be called before dsa_unregister_switch()?
Daniel Golle
maintainerRe: [PATCH net-next v5 2/4] net: dsa: mxl862xx: move phylink stubs to mxl862xx-phylink.c
Quoted 47 lines · Simon Horman
This is an AI-generated review of your patch. The human sending this email has considered the AI review valid, or at least plausible. Full review at: https://sashiko.dev
---
net: dsa: mxl862xx: move phylink stubs tomxl862xx-phylink.cMove the phylink MAC operations and
get_capscallback frommxl862xx.cinto a dedicatedmxl862xx-phylink.cfile. This prepares for the SerDes PCS implementation which adds substantial phylink/PCS code.drivers/net/dsa/mxl862xx/mxl862xx.c
Quoted 4 lines
drivers/net/dsa/mxl862xx/mxl862xx.c +0
-0[ ... ]
[Severity: Critical]
This is a pre-existing issue, but thecrc_err_workcan execute afterdsa_unregister_switch(), leading to a use-after-free ofds->dstanddsa_portstructures.In
mxl862xx_remove(),dsa_unregister_switch()is called before the asynchronouscrc_err_workis cancelled viamxl862xx_host_shutdown():drivers/net/dsa/mxl862xx/mxl862xx.c:mxl862xx_remove() { ... dsa_unregister_switch(ds); mxl862xx_host_shutdown(priv); ... }
dsa_unregister_switch()tears down the DSA tree, frees alldsa_portobjects, and drops the reference tods->dst.If
mxl862xx_crc_err_work_fnexecutes concurrently or afterdsa_unregister_switch()has returned, it traverses already freeddsa_portentries:drivers/net/dsa/mxl862xx/mxl862xx-host.c:mxl862xx_crc_err_work_fn() { ... dsa_switch_for_each_cpu_port(dp, priv->ds) dev_close(dp->conduit); ... }
Quoted 2 lines
Could this order in
mxl862xx_remove()result in a use-after-free, and shouldmxl862xx_host_shutdown()be called beforedsa_unregister_switch()?
To really address this a guard in mxl862xx_host_shutdown() will be needed. I'd rather do this in a separate Fixes:-patch targetting 'net' rather than in this series targetting 'net-next', right?
Simon Horman
reviewerRe: [PATCH net-next v5 2/4] net: dsa: mxl862xx: move phylink stubs to mxl862xx-phylink.c
Quoted 48 lines · Daniel Golle
Quoted 47 lines · Simon Horman
This is an AI-generated review of your patch. The human sending this email has considered the AI review valid, or at least plausible. Full review at: https://sashiko.dev
---
net: dsa: mxl862xx: move phylink stubs tomxl862xx-phylink.cMove the phylink MAC operations and
get_capscallback frommxl862xx.cinto a dedicatedmxl862xx-phylink.cfile. This prepares for the SerDes PCS implementation which adds substantial phylink/PCS code.drivers/net/dsa/mxl862xx/mxl862xx.c
Quoted 4 lines
drivers/net/dsa/mxl862xx/mxl862xx.c +0
-0[ ... ]
[Severity: Critical]
This is a pre-existing issue, but thecrc_err_workcan execute afterdsa_unregister_switch(), leading to a use-after-free ofds->dstanddsa_portstructures.In
mxl862xx_remove(),dsa_unregister_switch()is called before the asynchronouscrc_err_workis cancelled viamxl862xx_host_shutdown():drivers/net/dsa/mxl862xx/mxl862xx.c:mxl862xx_remove() { ... dsa_unregister_switch(ds); mxl862xx_host_shutdown(priv); ... }
dsa_unregister_switch()tears down the DSA tree, frees alldsa_portobjects, and drops the reference tods->dst.If
mxl862xx_crc_err_work_fnexecutes concurrently or afterdsa_unregister_switch()has returned, it traverses already freeddsa_portentries:drivers/net/dsa/mxl862xx/mxl862xx-host.c:mxl862xx_crc_err_work_fn() { ... dsa_switch_for_each_cpu_port(dp, priv->ds) dev_close(dp->conduit); ... }
Quoted 6 lines
Quoted 2 lines
Could this order in
mxl862xx_remove()result in a use-after-free, and shouldmxl862xx_host_shutdown()be called beforedsa_unregister_switch()?To really address this a guard in
mxl862xx_host_shutdown()will be needed. I'd rather do this in a separate Fixes:-patch targetting 'net' rather than in this series targetting 'net-next', right?
Yes, that sounds like a good approach to me.
Daniel Golle
maintainerPATCH v5 3/4 net: dsa: mxl862xx: move API macros to mxl862xx-host.h
In reply to Daniel GolleMove the MXL862XX_API_WRITE, MXL862XX_API_READ and MXL862XX_API_READ_QUIET convenience macros from mxl862xx.c to mxl862xx-host.h next to the mxl862xx_api_wrap() prototype they wrap. This makes them available to other compilation units that include mxl862xx-host.h, which is needed once the SerDes PCS code in mxl862xx-phylink.c also calls firmware commands.
No functional change.
---
v5: no changes
v4: no changes
v3: no changes
v2: no changes
drivers/net/dsa/mxl862xx/mxl862xx-host.h | 8 ++++++++ drivers/net/dsa/mxl862xx/mxl862xx.c | 7 ------- 2 files changed, 8 insertions(+), 7 deletions(-)
drivers/net/dsa/mxl862xx/mxl862xx-host.h +8 -0
@@ -9,6 +9,14 @@ void mxl862xx_host_init(struct mxl862xx_priv *priv);void mxl862xx_host_shutdown (struct mxl862xx_priv *priv);int mxl862xx_api_wrap (struct mxl862xx_priv *priv,u16 cmd,void *data,u16 size,bool read,bool quiet); + +#define MXL862XX_API_WRITE (dev, cmd, data) \ + mxl862xx_api_wrap(dev, cmd, &(data), sizeof((data)), false, false) +#define MXL862XX_API_READ (dev, cmd, data) \ + mxl862xx_api_wrap(dev, cmd, &(data), sizeof((data)), true, false) +#define MXL862XX_API_READ_QUIET (dev, cmd, data) \ + mxl862xx_api_wrap(dev, cmd, &(data), sizeof((data)), true, true) +int mxl862xx_reset (struct mxl862xx_priv *priv);#endif /* __MXL862XX_HOST_H */
drivers/net/dsa/mxl862xx/mxl862xx.c +0 -7
@@ -24,13 +24,6 @@#include "mxl862xx-host.h" #include "mxl862xx-phylink.h" -#define MXL862XX_API_WRITE (dev, cmd, data) \ - mxl862xx_api_wrap(dev, cmd, &(data), sizeof((data)), false, false) -#define MXL862XX_API_READ (dev, cmd, data) \ - mxl862xx_api_wrap(dev, cmd, &(data), sizeof((data)), true, false) -#define MXL862XX_API_READ_QUIET (dev, cmd, data) \ - mxl862xx_api_wrap(dev, cmd, &(data), sizeof((data)), true, true) -/* Polling interval for RMON counter accumulation. At 2.5 Gbps with * minimum-size (64-byte) frames, a 32-bit packet counter wraps in ~880s. * 2s gives a comfortable margin.
Signature · 1 line
2.54.0
Daniel Golle
maintainerPATCH v5 4/4 net: dsa: mxl862xx: add support for SerDes ports
In reply to Daniel GolleThe MxL862xx has two XPCS/SerDes interfaces (XPCS0 for ports 9-12, XPCS1 for ports 13-16). Each can operate in various single-lane modes (SGMII, 1000Base-X, 2500Base-X, 10GBase-R, 10GBase-KR, USXGMII) or as QSGMII or 10G_QXGMII providing four sub-ports per interface.
Implement phylink PCS operations using the firmware's XPCS API:
- pcs_config: configure negotiation mode and CL37/SGMII advertising.
- pcs_get_state: read link state and the link-partner ability word from firmware and decode using phylink's standard CL37, SGMII, and USXGMII decoders.
- pcs_an_restart: restart CL37 or CL73 auto-negotiation.
- pcs_link_up: force speed/duplex for SGMII.
- pcs_inband_caps: report per-mode in-band status capabilities.
Register a PCS instance for each SerDes interface and QSGMII/10G_QXGMII sub-ports during setup. Advertise the supported interface modes in phylink_get_caps based on port number.
Lacking support for expressing PHY-side role modes in Linux only the MAC-side of SGMII, QSGMII, USXGMII and 10G_QXGMII are implemented for now.
---
v5:
* use FIELD_GET/FIELD_PREP and macro definitions for the bitfields instead of endian-aware structs with bit-sized members
* do not error out on old firmware, so driver at least probes and CPU port keeps working in firmware-configured mode. Issue a warning instead.
v4:
* replace atomic_t serdes_refcount with a plain int guarded by a new serdes_lock mutex; pcs_disable now holds the lock across the count and the XPCS power-down so a sibling sub-port enable cannot race the transition to zero (the atomic only made the counter safe, not the decision-and-act)
v3:
* replace serdes_active bitmap with atomic_t serdes_refcount
* defer mpcs->interface assignment until after firmware ack
* handle firmware error codes in pcs_config
* set st.usx_lane_mode in pcs_get_state
* set lu.usx_subport and lu.usx_lane_mode in pcs_link_up
* use phylink_mii_c22_pcs_encode_advertisement() in CL37 adv
* rework commit message
v2:
* add __{LE,BE}_BITFIELD layouts to ABI structs
* per-sub-port QSGMII AN restart via usx_subport / usx_lane_mode
* shared-SerDes refcount in pcs_disable via per-XPCS slot bitmap
* let every sub-port call pcs_config
* cache phy_interface_t instead of firmware type
* skip pcs_link_up when inband-AN enabled
* gate phylink_get_caps SerDes modes on same FW version as select_pcs
* interpret xpcs_pcs_cfg.result as signed (s16)
* drop dead MXL862XX_PCS_PORT macro
* drop misleading "downshift detection" line from commit message
drivers/net/dsa/mxl862xx/mxl862xx-api.h | 215 +++++++++++ drivers/net/dsa/mxl862xx/mxl862xx-cmd.h | 9 + drivers/net/dsa/mxl862xx/mxl862xx-phylink.c | 386 +++++++++++++++++++- drivers/net/dsa/mxl862xx/mxl862xx-phylink.h | 7 + drivers/net/dsa/mxl862xx/mxl862xx.c | 7 +- drivers/net/dsa/mxl862xx/mxl862xx.h | 34 ++ 6 files changed, 655 insertions(+), 3 deletions(-)
drivers/net/dsa/mxl862xx/mxl862xx-api.h +215 -0
@@ -1366,4 +1366,219 @@ struct mxl862xx_rmon_port_cnt {__le64 tx_good_bytes; } __packed; +/* XPCS interface mode, MXL862XX_XPCS_*_INTERFACE field values */ +#define MXL862XX_XPCS_IF_SGMII 0 +#define MXL862XX_XPCS_IF_1000BASEX 1 +#define MXL862XX_XPCS_IF_2500BASEX 2 +#define MXL862XX_XPCS_IF_USXGMII 3/* single or quad */ +#define MXL862XX_XPCS_IF_10GBASER 4 +#define MXL862XX_XPCS_IF_10GKR 5/* 10GBASE-KR */ +#define MXL862XX_XPCS_IF_5GBASER 6 +#define MXL862XX_XPCS_IF_QSGMII 7 + +/* PCS negotiation mode, MXL862XX_XPCS_CFG_NEG_MODE field values */ +#define MXL862XX_XPCS_NEG_NONE 0/* no inband negotiation */ +#define MXL862XX_XPCS_NEG_INBAND_AN_OFF 1/* inband, AN disabled */ +#define MXL862XX_XPCS_NEG_INBAND_AN_ON 2/* inband, AN enabled */ + +/* +* PCS protocol role, MXL862XX_XPCS_CFG_ROLE field value. Selects the role +* the XPCS plays in protocols with an asymmetric AN code word (Cisco SGMII +* / QSGMII / USXGMII), driving VR_MII_AN_CTRL.TX_CONFIG: MAC means the +* local end receives the partner's AN word, PHY means it sources one. +* Ignored for symmetric protocols (1000BASE-X, 2500BASE-X, 10GBASE-R/KR). +*/ +#define MXL862XX_XPCS_ROLE_MAC 0/* local end is MAC side */ +#define MXL862XX_XPCS_ROLE_PHY 1/* local end is PHY side */ + +/* USXGMII lane mode, MXL862XX_XPCS_*_USX_LANE_MODE field values */ +#define MXL862XX_XPCS_USX_SINGLE 0/* single USXGMII lane */ +#define MXL862XX_XPCS_USX_QUAD 1/* quad USXGMII, 4 ports/lane */ + +/** +* union mxl862xx_xpcs_an_word - XPCS AN code word, tagged by interface mode +* @cl37: 16-bit base-page word exchanged over the CL37 hardware AN path +* (SR_MII_AN_ADV on write, SR_MII_LP_BABL on read). Carries the +* 802.3 CL37 base page for 1000BASE-X/2500BASE-X and the Cisco +* SGMII config word for SGMII/QSGMII. +* @usx: USXGMII 16-bit AN code word, MDIO_USXGMII_* layout +* @cl73: CL73 48-bit base page (10GBASE-KR), three 16-bit registers per +* 802.3 Annex 28C +* @cl73.adv1: CL73 SR_AN_ADV1 / SR_AN_LP_ABL1 +* @cl73.adv2: CL73 SR_AN_ADV2 / SR_AN_LP_ABL2 +* @cl73.adv3: CL73 SR_AN_ADV3 / SR_AN_LP_ABL3 +* +* The host picks the right member based on the interface field of the +* surrounding struct (and, for the asymmetric protocols, on the role). +*/ +union mxl862xx_xpcs_an_word { +__le16 cl37; +__le16 usx; +struct { +__le16 adv1; +__le16 adv2; +__le16 adv3; + } cl73; +} __packed; + +/* PCS duplex mode, MXL862XX_XPCS_*_DUPLEX field values */ +#define MXL862XX_XPCS_DUPLEX_HALF 0 +#define MXL862XX_XPCS_DUPLEX_FULL 1 + +/** +* enum mxl862xx_xpcs_loopback_mode - XPCS loopback mode +* @MXL862XX_XPCS_LB_DISABLE: disable all loopback +* @MXL862XX_XPCS_LB_PCS_SERIAL: PCS TX-to-RX serial loopback +* @MXL862XX_XPCS_LB_PCS_PARALLEL: PCS RX-to-TX parallel loopback +* @MXL862XX_XPCS_LB_PMA_SERIAL: PMA TX-to-RX serial loopback +* @MXL862XX_XPCS_LB_PMA_PARALLEL: PMA RX-to-TX parallel loopback +*/ +enum mxl862xx_xpcs_loopback_mode { +MXL862XX_XPCS_LB_DISABLE =0 , +MXL862XX_XPCS_LB_PCS_SERIAL =1 , +MXL862XX_XPCS_LB_PCS_PARALLEL =2 , +MXL862XX_XPCS_LB_PMA_SERIAL =3 , +MXL862XX_XPCS_LB_PMA_PARALLEL =4 , +}; + +/* Fields of mxl862xx_xpcs_pcs_cfg.mode */ +#define MXL862XX_XPCS_CFG_PORT_ID GENMASK(1, 0) +#define MXL862XX_XPCS_CFG_INTERFACE GENMASK(7, 2) +#define MXL862XX_XPCS_CFG_NEG_MODE GENMASK(9, 8) +#define MXL862XX_XPCS_CFG_PERMIT_PAUSE BIT(10) +#define MXL862XX_XPCS_CFG_USX_LANE_MODE GENMASK(12, 11) +#define MXL862XX_XPCS_CFG_ROLE BIT(13) +#define MXL862XX_XPCS_CFG_USX_SUBPORT GENMASK(15, 14) + +/** +* struct mxl862xx_xpcs_pcs_cfg - PCS configuration parameters +* @mode: Packed interface and negotiation parameters, see +* MXL862XX_XPCS_CFG_*. port_id is the XPCS port index (0-3); +* interface is the PCS interface mode (MXL862XX_XPCS_IF_*); +* neg_mode is the negotiation mode (MXL862XX_XPCS_NEG_*); +* permit_pause allows pause to MAC; usx_lane_mode is the USXGMII +* lane mode (MXL862XX_XPCS_USX_*); role is the protocol role +* (MXL862XX_XPCS_ROLE_*); usx_subport is the sub-port (0-3) within +* the XPCS -- despite the name it also identifies the QSGMII +* sub-port -- used by the firmware to set MAC pause per sub-port +* and ignored for the XPCS-wide bringup, which is idempotent across +* slots. +* @advertising: AN code word the local end transmits. The active union +* member is selected by the interface field (and, for the +* asymmetric protocols, by role). Ignored when the local end +* does not transmit an AN word (role=MAC for SGMII/QSGMII/ +* USXGMII, 10GBASE-R, 5GBASE-R) or when neg_mode is not +* INBAND_AN_ON. Pass all-zero to keep the firmware default +* advertisement. +* @result: Firmware result. >0 means the host must follow with an AN +* restart, 0 means no host follow-up is needed, <0 is an errno. +*/ +struct mxl862xx_xpcs_pcs_cfg { +__le16 mode; +union mxl862xx_xpcs_an_word advertising; +__le16 result; +} __packed; + +/* Fields of mxl862xx_xpcs_pcs_state.mode */ +#define MXL862XX_XPCS_ST_PORT_ID GENMASK(1, 0) +#define MXL862XX_XPCS_ST_INTERFACE GENMASK(7, 2) +#define MXL862XX_XPCS_ST_USX_LANE_MODE GENMASK(9, 8) +#define MXL862XX_XPCS_ST_USX_SUBPORT GENMASK(11, 10) +#define MXL862XX_XPCS_ST_LINK BIT(12) +#define MXL862XX_XPCS_ST_AN_COMPLETE BIT(13) +#define MXL862XX_XPCS_ST_DUPLEX BIT(14) +#define MXL862XX_XPCS_ST_PCS_FAULT BIT(15) +#define MXL862XX_XPCS_ST_PAUSE GENMASK(17, 16) +#define MXL862XX_XPCS_ST_LP_EEE_CAP BIT(18) +#define MXL862XX_XPCS_ST_LP_EEE_CS_CAP BIT(19) + +/** +* struct mxl862xx_xpcs_pcs_state - PCS link state +* @mode: Packed input parameters and firmware status, see +* MXL862XX_XPCS_ST_*. The host writes port_id (XPCS port index 0-3), +* interface (MXL862XX_XPCS_IF_*), usx_lane_mode +* (MXL862XX_XPCS_USX_*) and usx_subport (0-3); the firmware fills in +* link, an_complete, duplex (MXL862XX_XPCS_DUPLEX_*), pcs_fault, +* pause (bit 0 symmetric, bit 1 asymmetric), lp_eee_cap and +* lp_eee_cs_cap. +* @speed: Resolved speed in Mbit/s (output) +* @lpa: Link partner ability word (output). Same union as +* &union mxl862xx_xpcs_an_word; the host picks the member based on +* the interface field. +*/ +struct mxl862xx_xpcs_pcs_state { +__le32 mode; +__le16 speed;/* Mbit/s */ +union mxl862xx_xpcs_an_word lpa; +} __packed; + +/** +* struct mxl862xx_xpcs_pcs_disable - PCS disable parameters +* @port_id: XPCS port index +* @__pad: padding +* @result: Firmware result. 0 on success, <0 on error. +* +* Asserts IDDQ + PHY + XPCS resets to power down the SERDES when the +* port is admin-down or no module is plugged in. The next PCS config +* implicitly powers it back up and reprograms the desired interface. +*/ +struct mxl862xx_xpcs_pcs_disable { +u8 port_id; +u8 __pad; +__le16 result; +} __packed; + +/* Fields of mxl862xx_xpcs_an_restart.mode */ +#define MXL862XX_XPCS_ANR_PORT_ID GENMASK(1, 0) +#define MXL862XX_XPCS_ANR_INTERFACE GENMASK(7, 2) +#define MXL862XX_XPCS_ANR_USX_LANE_MODE GENMASK(9, 8) +#define MXL862XX_XPCS_ANR_USX_SUBPORT GENMASK(11, 10) + +/** +* struct mxl862xx_xpcs_an_restart - AN restart parameters +* @mode: Packed input parameters, see MXL862XX_XPCS_ANR_*. port_id is the +* XPCS port index (0-3); interface is the PCS interface mode +* (MXL862XX_XPCS_IF_*); usx_lane_mode is the USX lane mode +* (MXL862XX_XPCS_USX_*); usx_subport (0-3) selects the lane whose +* AN is restarted for QSGMII and QUSXGMII and is ignored by +* single-lane modes. +* @result: Firmware result. 0 on success, <0 on error. +* +* Restarts auto-negotiation on a single sub-port of the XPCS. The +* SERDES must already be configured. +*/ +struct mxl862xx_xpcs_an_restart { +__le16 mode; +__le16 result; +} __packed; + +/* Fields of mxl862xx_xpcs_pcs_link_up.mode */ +#define MXL862XX_XPCS_LU_PORT_ID GENMASK(1, 0) +#define MXL862XX_XPCS_LU_INTERFACE GENMASK(7, 2) +#define MXL862XX_XPCS_LU_DUPLEX BIT(8) +#define MXL862XX_XPCS_LU_USX_LANE_MODE GENMASK(10, 9) +#define MXL862XX_XPCS_LU_USX_SUBPORT GENMASK(12, 11) + +/** +* struct mxl862xx_xpcs_pcs_link_up - PCS link-up parameters +* @mode: Packed input parameters, see MXL862XX_XPCS_LU_*. port_id is the +* XPCS port index (0-3); interface is the PCS interface mode +* (MXL862XX_XPCS_IF_*); duplex is the duplex mode +* (MXL862XX_XPCS_DUPLEX_*); usx_lane_mode is the USX lane mode +* (USXGMII only, ignored otherwise, MXL862XX_XPCS_USX_*); +* usx_subport (0-3) selects the sub-port for QUSXGMII and QSGMII +* (despite the name) and is ignored otherwise. +* @speed: Resolved speed in Mbit/s +* @result: Firmware result. 0 on success, <0 is errno. +* +* Called once per link-up event after the host has resolved the +* line-side speed/duplex (from the PHY's read_status, from a preceding +* PCS get-state, or from a fixed-link description). +*/ +struct mxl862xx_xpcs_pcs_link_up { +__le16 mode; +__le16 speed;/* Mbit/s */ +__le16 result; +} __packed; +#endif /* __MXL862XX_API_H */
drivers/net/dsa/mxl862xx/mxl862xx-cmd.h +9 -0
@@ -24,6 +24,7 @@#define MXL862XX_SS_MAGIC 0x1600#define GPY_GPY2XX_MAGIC 0x1800#define SYS_MISC_MAGIC 0x1900 +#define MXL862XX_XPCS_MAGIC 0x1a00#define MXL862XX_COMMON_CFGGET (MXL862XX_COMMON_MAGIC + 0x9)#define MXL862XX_COMMON_CFGSET (MXL862XX_COMMON_MAGIC + 0xa)
@@ -71,6 +72,14 @@#define SYS_MISC_FW_VERSION (SYS_MISC_MAGIC + 0x2) +#define MXL862XX_XPCS_PCS_CONFIG (MXL862XX_XPCS_MAGIC + 0x1) +#define MXL862XX_XPCS_PCS_GET_STATE (MXL862XX_XPCS_MAGIC + 0x2) +#define MXL862XX_XPCS_PCS_DISABLE (MXL862XX_XPCS_MAGIC + 0x4) +#define MXL862XX_XPCS_AN_RESTART (MXL862XX_XPCS_MAGIC + 0x5) +#define MXL862XX_XPCS_PCS_LINK_UP (MXL862XX_XPCS_MAGIC + 0x7) +#define MXL862XX_XPCS_LOOPBACK (MXL862XX_XPCS_MAGIC + 0x8) +#define MXL862XX_XPCS_RESET (MXL862XX_XPCS_MAGIC + 0x9) +#define MMD_API_MAXIMUM_ID 0x7fff#endif /* __MXL862XX_CMD_H */
drivers/net/dsa/mxl862xx/mxl862xx-phylink.c +384 -2
408 lines not on this page · raw message
drivers/net/dsa/mxl862xx/mxl862xx-phylink.h +7 -0
@@ -7,8 +7,15 @@#include "mxl862xx.h" +#define MXL862XX_SERDES_SLOT (port) \ + (((port) - MXL862XX_FIRST_SERDES_PORT) % MXL862XX_SERDES_SLOTS) +#define MXL862XX_SERDES_PORT_ID (port) \ + (((port) - MXL862XX_FIRST_SERDES_PORT) / MXL862XX_SERDES_SLOTS) +extern const struct phylink_mac_ops mxl862xx_phylink_mac_ops;void mxl862xx_phylink_get_caps (struct dsa_switch *ds,int port,struct phylink_config *config); +void mxl862xx_setup_pcs (struct mxl862xx_priv *priv,struct mxl862xx_pcs *pcs, +int port);#endif /* __MXL862XX_PHYLINK_H */
drivers/net/dsa/mxl862xx/mxl862xx.c +6 -1
@@ -622,7 +622,7 @@ static int mxl862xx_setup(struct dsa_switch *ds)int n_user_ports =0 , max_vlans;int ingress_finals, vid_rules;struct dsa_port *dp; -int ret; +int ret, i; ret =mxl862xx_reset (priv);if (ret)
@@ -632,6 +632,11 @@ static int mxl862xx_setup(struct dsa_switch *ds)if (ret)return ret; +mutex_init (&priv->serdes_lock); +for (i =0 ; i <ARRAY_SIZE (priv->serdes_ports); i++) +mxl862xx_setup_pcs (priv, &priv->serdes_ports[i], + i +MXL862XX_FIRST_SERDES_PORT ); +/* Calculate Extended VLAN block sizes. * With VLAN Filter handling VID membership checks: * Ingress: only final catchall rules (PVID insertion, 802.1Q
drivers/net/dsa/mxl862xx/mxl862xx.h +34 -0
@@ -11,6 +11,9 @@struct mxl862xx_priv ;#define MXL862XX_MAX_PORTS 17 +#define MXL862XX_FIRST_SERDES_PORT 9 +#define MXL862XX_SERDES_SLOTS 4 +#define MXL862XX_DEFAULT_BRIDGE 0#define MXL862XX_MAX_BRIDGES 48#define MXL862XX_MAX_BRIDGE_PORTS 128
@@ -242,6 +245,26 @@ struct mxl862xx_port {spinlock_t stats_lock;/* protects stats accumulators */ }; +/** +* struct mxl862xx_pcs - link SerDes interfaces to bridge ports +* @pcs: &struct phylink_pcs instance +* @priv: pointer to &struct mxl862xx_priv +* @serdes_id: SerDes instance index (0 or 1) +* @slot: slot within the SerDes (0-3 for QSGMII/QUSXGMII, 0 otherwise) +* @interface: cached PHY interface, last value passed to pcs_config(). +* %PHY_INTERFACE_MODE_NA before the first successful +* pcs_config(). Used by pcs_an_restart() to populate the +* firmware command and by pcs_disable() to skip the +* firmware power-down for shared (QSGMII/QUSXGMII) modes. +*/ +struct mxl862xx_pcs { +struct phylink_pcs pcs; +struct mxl862xx_priv *priv; +int serdes_id; +int slot; +phy_interface_t interface; +}; +/** * struct mxl862xx_fw_version - firmware version for comparison and display * @major: firmware major version
@@ -280,6 +303,14 @@ struct mxl862xx_fw_version {* flooding) * @fw_version: cached firmware version, populated at probe and * compared with MXL862XX_FW_VER_MIN() +* @serdes_ports: SerDes interfaces incl. sub-interfaces in case of +* 10G_QXGMII or QSGMII +* @serdes_refcount: per-XPCS count of sub-ports enabled by phylink; +* pcs_disable powers an XPCS down when the count +* reaches zero. Protected by @serdes_lock. +* @serdes_lock: serializes the @serdes_refcount transitions with +* the XPCS power-down so a sibling sub-port enable +* cannot race a power-down to zero * @ports: per-port state, indexed by switch port number * @bridges: maps DSA bridge number to firmware bridge ID; * zero means no firmware bridge allocated for that
@@ -298,6 +329,9 @@ struct mxl862xx_priv {unsigned long flags;u16 drop_meter;struct mxl862xx_fw_version fw_version; +struct mxl862xx_pcs serdes_ports[8 ]; +int serdes_refcount[2 ]; +struct mutex serdes_lock;struct mxl862xx_port ports[MXL862XX_MAX_PORTS ];u16 bridges[MXL862XX_MAX_BRIDGES +1 ];u16 evlan_ingress_size;
Signature · 1 line
2.54.0
Simon Horman
reviewerRe: [PATCH net-next v5 4/4] net: dsa: mxl862xx: add support for SerDes ports
This is an AI-generated review of your patch. The human sending this email has considered the AI review valid, or at least plausible. Full review at: https://sashiko.dev
---
net: dsa: mxl862xx: add support for SerDes ports
The MxL862xx has two XPCS/SerDes interfaces. This commit implements phylink PCS operations using the firmware's XPCS API and registers a PCS instance for each SerDes interface and QSGMII/10G_QXGMII sub-ports.
Quoted 3 lines
drivers/net/dsa/mxl862xx/mxl862xx-phylink.c +0
-0
[ ... ]
Quoted 33 lines
drivers/net/dsa/mxl862xx/mxl862xx-phylink.c +15
-2@@ -7,20 +7,401 @@* Copyright (C) 2025 Daniel Golle <[email protected]> */ +#include <linux/bitfield.h> +#include <linux/mutex.h> #include <linux/phylink.h> #include <net/dsa.h> #include "mxl862xx.h" +#include "mxl862xx-api.h" +#include "mxl862xx-cmd.h" +#include "mxl862xx-host.h" #include "mxl862xx-phylink.h" void mxl862xx_phylink_get_caps (struct dsa_switch *ds,int port,struct phylink_config *config) { +struct mxl862xx_priv *priv = ds->priv; + config->mac_capabilities =MAC_ASYM_PAUSE |MAC_SYM_PAUSE |MAC_10 |MAC_100 |MAC_1000 |MAC_2500FD ; -__set_bit (PHY_INTERFACE_MODE_INTERNAL , - config->supported_interfaces); +switch (port) { + case 1 ... 8: +__set_bit (PHY_INTERFACE_MODE_INTERNAL , + config->supported_interfaces); +break ; +case 9 : +case 13 : +__set_bit (PHY_INTERFACE_MODE_SGMII , config->supported_interfaces);
drivers/net/dsa/mxl862xx/mxl862xx-phylink.c line 46
Quoted 10 lines
drivers/net/dsa/mxl862xx/mxl862xx-phylink.c +10
-0+__set_bit (PHY_INTERFACE_MODE_1000BASEX , config->supported_interfaces); +__set_bit (PHY_INTERFACE_MODE_2500BASEX , config->supported_interfaces); +__set_bit (PHY_INTERFACE_MODE_10GBASER , config->supported_interfaces); +__set_bit (PHY_INTERFACE_MODE_10GKR , config->supported_interfaces); +__set_bit (PHY_INTERFACE_MODE_USXGMII , config->supported_interfaces); + fallthrough; + case 10 ... 12: + case 14 ... 16: +if (!MXL862XX_FW_VER_MIN (priv,1 ,0 ,84 )) +break ;
[Severity: High]
Since the firmware version check is performed after the supported interface bits are set for ports 9 and 13, won't SGMII and other modes remain advertised on older firmware versions?
Because mxl862xx_phylink_mac_select_pcs() returns NULL for ports 9-16 on older firmware, phylink would accept the configuration without a PCS, potentially leaving the port silently unconfigured.
[ ... ]
Quoted 18 lines
drivers/net/dsa/mxl862xx/mxl862xx-phylink.c +18
-0+static void mxl862xx_pcs_disable (struct phylink_pcs *pcs) +{ +struct mxl862xx_pcs *mpcs =pcs_to_mxl862xx_pcs (pcs); +struct mxl862xx_xpcs_pcs_disable dis = {}; +struct mxl862xx_priv *priv = mpcs->priv; + + dis.port_id = mpcs->serdes_id; + +/* The SerDes is shared across QSGMII/QUSXGMII sub-ports; only +* power it down once the last active sub-port goes away. Hold +* serdes_lock across the count and the power-down so a sibling +* sub-port enable cannot race the transition to zero. +*/ +mutex_lock (&priv->serdes_lock); +if (--priv->serdes_refcount[mpcs->serdes_id] ==0 ) +MXL862XX_API_WRITE (priv,MXL862XX_XPCS_PCS_DISABLE , dis); +mutex_unlock (&priv->serdes_lock); +}
[ ... ]
Quoted 37 lines
drivers/net/dsa/mxl862xx/mxl862xx-phylink.c +37
-0+static int mxl862xx_pcs_config(struct phylink_pcs *pcs, unsigned int neg_mode, + phy_interface_t interface, + const unsigned long *advertising, + bool permit_pause_to_mac) +{ + struct mxl862xx_pcs *mpcs = pcs_to_mxl862xx_pcs(pcs); + struct mxl862xx_priv *priv = mpcs->priv; + struct mxl862xx_xpcs_pcs_cfg cfg = {}; + int if_mode, lane, ret, adv; + + if_mode = mxl862xx_xpcs_if_mode(interface); + if (if_mode < 0) { + dev_err(priv->ds->dev, "unsupported interface: %s\n", + phy_modes(interface)); + return if_mode; + } + + /* The XPCS bringup is per-instance and idempotent in the + * firmware: every QSGMII/QUSXGMII sub-port may call pcs_config + * and the firmware will skip the bringup if the requested mode + * matches the cached one, then update MAC pause for the + * sub-port indicated by @usx_subport. + */ + lane = (interface == PHY_INTERFACE_MODE_10G_QXGMII) ? + MXL862XX_XPCS_USX_QUAD : MXL862XX_XPCS_USX_SINGLE; + + cfg.mode = cpu_to_le16(FIELD_PREP(MXL862XX_XPCS_CFG_PORT_ID, + mpcs->serdes_id) | + FIELD_PREP(MXL862XX_XPCS_CFG_USX_SUBPORT, + mpcs->slot) | + FIELD_PREP(MXL862XX_XPCS_CFG_USX_LANE_MODE, lane) | + FIELD_PREP(MXL862XX_XPCS_CFG_INTERFACE, if_mode) | + FIELD_PREP(MXL862XX_XPCS_CFG_NEG_MODE, + mxl862xx_xpcs_neg_mode(neg_mode)) | + FIELD_PREP(MXL862XX_XPCS_CFG_ROLE, + MXL862XX_XPCS_ROLE_MAC) | + FIELD_PREP(MXL862XX_XPCS_CFG_PERMIT_PAUSE,
drivers/net/dsa/mxl862xx/mxl862xx-phylink.c line 192
Quoted 10 lines
drivers/net/dsa/mxl862xx/mxl862xx-phylink.c +10
-0+ permit_pause_to_mac)); + +if (neg_mode &PHYLINK_PCS_NEG_INBAND ) { + adv =phylink_mii_c22_pcs_encode_advertisement (interface, + advertising); +if (adv >=0 ) + cfg.advertising.cl37 =cpu_to_le16 (adv); + } + + ret =MXL862XX_API_READ (priv,MXL862XX_XPCS_PCS_CONFIG , cfg);
[Severity: High]
Can mxl862xx_pcs_disable() on one sub-port race with mxl862xx_pcs_config() on another sub-port, potentially leaving the shared XPCS powered down?
mxl862xx_pcs_disable() synchronizes the hardware disable command using serdes_lock, but it seems mxl862xx_pcs_config() issues the firmware config command (which implicitly powers up the hardware) without holding serdes_lock. If mxl862xx_pcs_disable() executes its firmware command right after mxl862xx_pcs_config() executes, could this cause a permanent link failure on the newly configured sub-port?
Quoted 15 lines
drivers/net/dsa/mxl862xx/mxl862xx-phylink.c +15
-0+if (ret) +return ret; + + ret = (s16 )le16_to_cpu (cfg.result); +if (ret <0 ) +return mxl862xx_xpcs_errno (ret); + + mpcs->interface = interface; +return ret >0 ?1 :0 ; +} + +static void mxl862xx_pcs_get_state (struct phylink_pcs *pcs, +unsigned int neg_mode, +struct phylink_link_state *state) +{
drivers/net/dsa/mxl862xx/mxl862xx-phylink.c line 217
Quoted 10 lines
drivers/net/dsa/mxl862xx/mxl862xx-phylink.c +10
-0+struct mxl862xx_pcs *mpcs =pcs_to_mxl862xx_pcs (pcs); +struct mxl862xx_priv *priv = mpcs->priv; +struct mxl862xx_xpcs_pcs_state st = {}; +int if_mode, lane, ret; +u32 mode; +u16 bmsr; + + if_mode =mxl862xx_xpcs_if_mode (state->interface); +if (if_mode <0 ) +return ;
[Severity: High]
Does returning early here without setting state->link = 0 cause issues?
Phylink initializes state->link = 1 before calling pcs_get_state(), expecting the callback to clear it if the link is down or an error occurs. Leaving it as 1 might cause phylink to process uninitialized parameters.
Quoted 4 lines
drivers/net/dsa/mxl862xx/mxl862xx-phylink.c +4
-0+ + lane = (state->interface ==PHY_INTERFACE_MODE_10G_QXGMII ) ? +MXL862XX_XPCS_USX_QUAD :MXL862XX_XPCS_USX_SINGLE ; +
drivers/net/dsa/mxl862xx/mxl862xx-phylink.c line 231
Quoted 10 lines
drivers/net/dsa/mxl862xx/mxl862xx-phylink.c +10
-0+ st.mode =cpu_to_le32 (FIELD_PREP (MXL862XX_XPCS_ST_PORT_ID , + mpcs->serdes_id) | +FIELD_PREP (MXL862XX_XPCS_ST_INTERFACE , if_mode) | +FIELD_PREP (MXL862XX_XPCS_ST_USX_SUBPORT , + mpcs->slot) | +FIELD_PREP (MXL862XX_XPCS_ST_USX_LANE_MODE , lane)); + + ret =MXL862XX_API_READ (priv,MXL862XX_XPCS_PCS_GET_STATE , st); +if (ret) +return ;
[Severity: High]
Could the early return here also leave state->link = 1 incorrectly when the firmware command fails?
Daniel Golle
maintainerRe: [PATCH net-next v5 4/4] net: dsa: mxl862xx: add support for SerDes ports
Quoted 63 lines · Simon Horman
This is an AI-generated review of your patch. The human sending this email has considered the AI review valid, or at least plausible. Full review at: https://sashiko.dev
---
net: dsa: mxl862xx: add support for SerDes portsThe MxL862xx has two XPCS/SerDes interfaces. This commit implements phylink PCS operations using the firmware's XPCS API and registers a PCS instance for each SerDes interface and QSGMII/10G_QXGMII sub-ports.
Quoted 3 lines
drivers/net/dsa/mxl862xx/mxl862xx-phylink.c +0
-0[ ... ]
Quoted 33 lines
drivers/net/dsa/mxl862xx/mxl862xx-phylink.c +15
-2@@ -7,20 +7,401 @@* Copyright (C) 2025 Daniel Golle <[email protected]> */ +#include <linux/bitfield.h> +#include <linux/mutex.h> #include <linux/phylink.h> #include <net/dsa.h> #include "mxl862xx.h" +#include "mxl862xx-api.h" +#include "mxl862xx-cmd.h" +#include "mxl862xx-host.h" #include "mxl862xx-phylink.h" void mxl862xx_phylink_get_caps (struct dsa_switch *ds,int port,struct phylink_config *config) { +struct mxl862xx_priv *priv = ds->priv; + config->mac_capabilities =MAC_ASYM_PAUSE |MAC_SYM_PAUSE |MAC_10 |MAC_100 |MAC_1000 |MAC_2500FD ; -__set_bit (PHY_INTERFACE_MODE_INTERNAL , - config->supported_interfaces); +switch (port) { + case 1 ... 8: +__set_bit (PHY_INTERFACE_MODE_INTERNAL , + config->supported_interfaces); +break ; +case 9 : +case 13 : +__set_bit (PHY_INTERFACE_MODE_SGMII , config->supported_interfaces);drivers/net/dsa/mxl862xx/mxl862xx-phylink.c line 46
Quoted 10 lines
drivers/net/dsa/mxl862xx/mxl862xx-phylink.c +10
-0+__set_bit (PHY_INTERFACE_MODE_1000BASEX , config->supported_interfaces); +__set_bit (PHY_INTERFACE_MODE_2500BASEX , config->supported_interfaces); +__set_bit (PHY_INTERFACE_MODE_10GBASER , config->supported_interfaces); +__set_bit (PHY_INTERFACE_MODE_10GKR , config->supported_interfaces); +__set_bit (PHY_INTERFACE_MODE_USXGMII , config->supported_interfaces); + fallthrough; + case 10 ... 12: + case 14 ... 16: +if (!MXL862XX_FW_VER_MIN (priv,1 ,0 ,84 )) +break ;[Severity: High]
Since the firmware version check is performed after the supported interface bits are set for ports 9 and 13, won't SGMII and other modes remain advertised on older firmware versions?
Quoted 3 lines
Because
mxl862xx_phylink_mac_select_pcs()returns NULL for ports 9-16 on older firmware, phylink would accept the configuration without a PCS, potentially leaving the port silently unconfigured.
This is intentional. The firmware configures the SerDes interfaces to the modes defined in a on-flash configuration. This means the CPU port typically works fine if DT reflects what is in the on-flash configuration without needing the driver to do anything for that.
On the other hand, if we error out here, the driver will fail to probe because it cannot bring up the CPU port. The problem is that this would also prevent firmware updates via devlink (which are going to be introduced in a later follow series), which require the driver to at least probe. A dev_warn() is issued in this case, so the user is informed about SerDes PCS configuration being unsupported on the old firmware version.
Quoted 74 lines
[ ... ]
Quoted 18 lines
drivers/net/dsa/mxl862xx/mxl862xx-phylink.c +18
-0+static void mxl862xx_pcs_disable (struct phylink_pcs *pcs) +{ +struct mxl862xx_pcs *mpcs =pcs_to_mxl862xx_pcs (pcs); +struct mxl862xx_xpcs_pcs_disable dis = {}; +struct mxl862xx_priv *priv = mpcs->priv; + + dis.port_id = mpcs->serdes_id; + +/* The SerDes is shared across QSGMII/QUSXGMII sub-ports; only +* power it down once the last active sub-port goes away. Hold +* serdes_lock across the count and the power-down so a sibling +* sub-port enable cannot race the transition to zero. +*/ +mutex_lock (&priv->serdes_lock); +if (--priv->serdes_refcount[mpcs->serdes_id] ==0 ) +MXL862XX_API_WRITE (priv,MXL862XX_XPCS_PCS_DISABLE , dis); +mutex_unlock (&priv->serdes_lock); +}[ ... ]
Quoted 37 lines
drivers/net/dsa/mxl862xx/mxl862xx-phylink.c +37
-0+static int mxl862xx_pcs_config(struct phylink_pcs *pcs, unsigned int neg_mode, + phy_interface_t interface, + const unsigned long *advertising, + bool permit_pause_to_mac) +{ + struct mxl862xx_pcs *mpcs = pcs_to_mxl862xx_pcs(pcs); + struct mxl862xx_priv *priv = mpcs->priv; + struct mxl862xx_xpcs_pcs_cfg cfg = {}; + int if_mode, lane, ret, adv; + + if_mode = mxl862xx_xpcs_if_mode(interface); + if (if_mode < 0) { + dev_err(priv->ds->dev, "unsupported interface: %s\n", + phy_modes(interface)); + return if_mode; + } + + /* The XPCS bringup is per-instance and idempotent in the + * firmware: every QSGMII/QUSXGMII sub-port may call pcs_config + * and the firmware will skip the bringup if the requested mode + * matches the cached one, then update MAC pause for the + * sub-port indicated by @usx_subport. + */ + lane = (interface == PHY_INTERFACE_MODE_10G_QXGMII) ? + MXL862XX_XPCS_USX_QUAD : MXL862XX_XPCS_USX_SINGLE; + + cfg.mode = cpu_to_le16(FIELD_PREP(MXL862XX_XPCS_CFG_PORT_ID, + mpcs->serdes_id) | + FIELD_PREP(MXL862XX_XPCS_CFG_USX_SUBPORT, + mpcs->slot) | + FIELD_PREP(MXL862XX_XPCS_CFG_USX_LANE_MODE, lane) | + FIELD_PREP(MXL862XX_XPCS_CFG_INTERFACE, if_mode) | + FIELD_PREP(MXL862XX_XPCS_CFG_NEG_MODE, + mxl862xx_xpcs_neg_mode(neg_mode)) | + FIELD_PREP(MXL862XX_XPCS_CFG_ROLE, + MXL862XX_XPCS_ROLE_MAC) | + FIELD_PREP(MXL862XX_XPCS_CFG_PERMIT_PAUSE,drivers/net/dsa/mxl862xx/mxl862xx-phylink.c line 192
Quoted 10 lines
drivers/net/dsa/mxl862xx/mxl862xx-phylink.c +10
-0+ permit_pause_to_mac)); + +if (neg_mode &PHYLINK_PCS_NEG_INBAND ) { + adv =phylink_mii_c22_pcs_encode_advertisement (interface, + advertising); +if (adv >=0 ) + cfg.advertising.cl37 =cpu_to_le16 (adv); + } + + ret =MXL862XX_API_READ (priv,MXL862XX_XPCS_PCS_CONFIG , cfg);[Severity: High]
Canmxl862xx_pcs_disable()on one sub-port race withmxl862xx_pcs_config()on another sub-port, potentially leaving the shared XPCS powered down?
Quoted 6 lines
mxl862xx_pcs_disable()synchronizes the hardware disable command usingserdes_lock, but it seemsmxl862xx_pcs_config()issues the firmware config command (which implicitly powers up the hardware) without holdingserdes_lock. Ifmxl862xx_pcs_disable()executes its firmware command right aftermxl862xx_pcs_config()executes, could this cause a permanent link failure on the newly configured sub-port?
Yes, mxl862xx_pcs_config() needs to hold the lock as well, I'll fix that.
Quoted 21 lines
Quoted 21 lines
drivers/net/dsa/mxl862xx/mxl862xx-phylink.c +20
-0+if (ret) +return ret; + + ret = (s16 )le16_to_cpu (cfg.result); +if (ret <0 ) +return mxl862xx_xpcs_errno (ret); + + mpcs->interface = interface; +return ret >0 ?1 :0 ; +} + +static void mxl862xx_pcs_get_state (struct phylink_pcs *pcs, +unsigned int neg_mode, +struct phylink_link_state *state) +{ +struct mxl862xx_pcs *mpcs =pcs_to_mxl862xx_pcs (pcs); +struct mxl862xx_priv *priv = mpcs->priv; +struct mxl862xx_xpcs_pcs_state st = {}; +int if_mode, lane, ret; +u32 mode;
Quoted 8 lines
drivers/net/dsa/mxl862xx/mxl862xx-phylink.c line 217
Quoted 5 lines
drivers/net/dsa/mxl862xx/mxl862xx-phylink.c +5
-0+u16 bmsr; + + if_mode =mxl862xx_xpcs_if_mode (state->interface); +if (if_mode <0 ) +return ;[Severity: High]
Does returning early here without settingstate->link= 0 cause issues?
In practise this is not a problem as phylink prevents the function from being called with any other interface mode than the ones defined in the capabilities. If at all, then we should issue a WARN here.
Quoted 15 lines
Phylink initializes
state->link= 1 before callingpcs_get_state(), expecting the callback to clear it if the link is down or an error occurs. Leaving it as 1 might cause phylink to process uninitialized parameters.Quoted 10 lines
drivers/net/dsa/mxl862xx/mxl862xx-phylink.c +10
-0+ + lane = (state->interface ==PHY_INTERFACE_MODE_10G_QXGMII ) ? +MXL862XX_XPCS_USX_QUAD :MXL862XX_XPCS_USX_SINGLE ; + + st.mode =cpu_to_le32 (FIELD_PREP (MXL862XX_XPCS_ST_PORT_ID , + mpcs->serdes_id) | +FIELD_PREP (MXL862XX_XPCS_ST_INTERFACE , if_mode) | +FIELD_PREP (MXL862XX_XPCS_ST_USX_SUBPORT , + mpcs->slot) | +FIELD_PREP (MXL862XX_XPCS_ST_USX_LANE_MODE , lane));
Quoted 8 lines
drivers/net/dsa/mxl862xx/mxl862xx-phylink.c line 231
Quoted 4 lines
drivers/net/dsa/mxl862xx/mxl862xx-phylink.c +4
-0+ + ret =MXL862XX_API_READ (priv,MXL862XX_XPCS_PCS_GET_STATE , st); +if (ret) +return ;[Severity: High]
Could the early return here also leavestate->link= 1 incorrectly when the firmware command fails?
Yes, and that's intentional. A failing firmware command doesn't mean that the link went down -- it means that there has been a problem with the MDIO bus communication with the firmware, or the specific firmware command returning an error. As pcs-polling will retry pcs_get_state() I figure that just not touching state->link in this case is the best thing to do.