Re: [PATCH 18/23] net: hpe: ss1: add the link layer glue

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 16 potential issue(s) to consider:
- [High] net: hpe: ss1: mismatched PHY state and link events in SBL and SL
- [High] net: hpe: ss1: missing STARTING/STOPPING states in cass_sl_link_up()
- [High] net: hpe: ss1: broken SerDes match masks in cass_sbl_serdes_config_defaults()
- [High] net: hpe: ss1: IRQ deadlock on hw->port->lock via cass_sbl_async_alert()
- [High] net: hpe: ss1: cass_sl_link_down() returns -EBUSY on -EALREADY
- [High] net: hpe: ss1: link_state and step_complete races in cass_sl_link_up/down()
- [Medium] net: hpe: ss1: cass_sbl_reset() uses wrong lock for port state updates
- [Medium] net: hpe: ss1: cass_sbl_mode_set() ignores link_info->speed
- [Medium] net: hpe: ss1: cass_sl_init() passes monotonic cxi_num to sl_ldev_new()
- [Medium] net: hpe: ss1: cass_sl_dt_info_get() omits default RX lane mapping
- [Medium] net: hpe: ss1: unserialized shared DMA buffer copy in cass_sl_dmac_xfer()
- [Medium] net: hpe: ss1: unlocked concurrent access to cass_dev->sl config structs
- [Medium] net: hpe: ss1: cass_sl_mode_set() overwrites saved autoneg/LT on loopback
- [Medium] net: hpe: ss1: uninitialized read data and -1 return in cass_sl_pmi_rd()
- [Medium] net: hpe: ss1: inconsistent PML recovery state and polarity in SBL and SL
- [Low] net: hpe: ss1: undocumented module parameters in cass_sbl.c and cass_sl.c
--

commit bde39545549e590832e0ff7bfa02d02603cda726
Author: Doug Ledford <doug.ledford@hpe.com>

net: hpe: ss1: add the link layer glue

This commit adds the glue layer connecting the Cassini core driver with the
SBL and SL link layers, along with PHY state machine and port helper
functions.

> diff --git a/drivers/net/ethernet/hpe/ss1/cass_sbl.c b/drivers/net/ethernet/hpe/ss1/cass_sbl.c
> new file mode 100644
> index 0000000000000..3b39e917fd5a4
> --- /dev/null
> +++ b/drivers/net/ethernet/hpe/ss1/cass_sbl.c
> @@ -0,0 +1,1706 @@

[ ... ]

> +static unsigned int pml_rec_timeout = SBL_DFLT_PML_REC_TIMEOUT;
> +module_param(pml_rec_timeout, uint, 0644);
> +MODULE_PARM_DESC(pml_rec_timeout, "PML recovery timeout in ms");
> +
> +static unsigned int pml_rec_rl_max_dur = SBL_DFLT_PML_REC_RL_MAX_DURATION;
> +module_param(pml_rec_rl_max_dur, uint, 0644);
> +MODULE_PARM_DESC(pml_rec_rl_max_dur, "PML recovery max time in ms per window");
> +
> +static unsigned int pml_rec_rl_win_size = SBL_DFLT_PML_REC_RL_WINDOW_SIZE;
> +module_param(pml_rec_rl_win_size, uint, 0644);
> +MODULE_PARM_DESC(pml_rec_rl_win_size, "PML recovery rate limiter window in ms");

[Severity: Low]
Should these new module parameters in cass_sbl.c (pml_rec_timeout,
pml_rec_rl_max_dur, and pml_rec_rl_win_size), as well as the corresponding
ones in cass_sl.c (cass_sl_pml_rec_timeout_ms,
cass_sl_pml_rec_rate_limit_max_time_ms, and
cass_sl_pml_rec_rate_limit_win_size_ms), be documented in
Documentation/admin-guide/kernel-parameters.txt?

[ ... ]

> +void cass_sbl_mode_set(struct cass_dev *hw, const struct cxi_link_info *link_info)
> +{
> +	struct cass_link_attr attrs = hw->port->lattr;
> +
> +	switch (link_info->flags & LOOPBACK_MODE) {
> +	case 0:
> +		attrs.bl.loopback_mode = SBL_LOOPBACK_MODE_OFF;
> +		break;
> +	case CXI_ETH_PF_INTERNAL_LOOPBACK:
> +		attrs.bl.loopback_mode = SBL_LOOPBACK_MODE_LOCAL;
> +		break;
> +	case CXI_ETH_PF_EXTERNAL_LOOPBACK:
> +		attrs.bl.loopback_mode = SBL_LOOPBACK_MODE_REMOTE;
> +		break;
> +	default:
> +		/* Invalid configuration, no change */
> +		break;
> +	}
> +
> +	if (link_info->flags & CXI_ETH_PF_LLR)
> +		attrs.bl.llr_mode = SBL_LLR_MODE_ON;
> +	else
> +		attrs.bl.llr_mode = SBL_LLR_MODE_OFF;
> +
> +	if (link_info->flags & CXI_ETH_PF_PRECODING)
> +		attrs.bl.precoding = SBL_PRECODING_ON;
> +	else
> +		attrs.bl.precoding = SBL_PRECODING_OFF;
> +
> +	if (link_info->flags & CXI_ETH_PF_IFG_HPC)
> +		attrs.bl.ifg_config = SBL_IFG_CONFIG_HPC;
> +	else
> +		attrs.bl.ifg_config = SBL_IFG_CONFIG_IEEE_200G;
> +
> +	if (link_info->flags & CXI_ETH_PF_PML_REC)
> +		attrs.bl.options |= SBL_DISABLE_PML_RECOVERY;
> +	else
> +		attrs.bl.options &= ~SBL_DISABLE_PML_RECOVERY;
> +
> +	if (attrs.mattr.media != SBL_LINK_MEDIA_OPTICAL) {
> +		if (link_info->autoneg == AUTONEG_DISABLE)
> +			attrs.bl.pec.an_mode = SBL_AN_MODE_OFF;
> +		else
> +			attrs.bl.pec.an_mode = SBL_AN_MODE_ON;
> +	}
> +
> +	if (!cass_sbl_link_attr_cmp(hw, &attrs))
> +		hw->link_config_dirty = false;
> +	else
> +		hw->link_config_dirty = true;
> +
> +	if (hw->link_config_dirty) {
> +		hw->port->lattr = attrs;
> +		/* Bounce the link to apply the new config */
> +		cass_phy_bounce(hw);
> +	}
> +}

[Severity: Medium]
Should cass_sbl_mode_set() also update attrs.bl.link_mode from
link_info->speed using cass_sbl_link_speed_to_mode()?

When cxi_set_link_ksettings() calls cxi_link_mode_set() with a new
link_info.speed, cass_sbl_mode_set() never checks link_info->speed, leaving
requested link speed changes ignored and cass_sbl_link_speed_to_mode() unused.

> +
> +void cass_sbl_pml_recovery_set(struct cass_dev *hw, bool set)
> +{
> +	sbl_disable_pml_recovery(hw->sbl, 0, set);
> +}

[ ... ]

> +static void cass_sbl_async_alert(void *accessor, int port_num, int alert_type,
> +				 void *alert_data, int size)
> +{
> +	struct cass_dev *hw = (struct cass_dev *)accessor;
> +	int down_origin;
> +
> +	cxidev_dbg(&hw->cdev, "(sbl): %s alert", sbl_async_alert_str(alert_type));
> +
> +	switch (alert_type) {
> +	case SBL_ASYNC_ALERT_LINK_DOWN:
> +		cxidev_warn(&hw->cdev, "(sbl): %d: link down alert\n",
> +			    port_num);
> +		down_origin = (int)(uintptr_t)alert_data;
> +		cass_sbl_link_down_async_handler(hw, port_num, down_origin);
> +		break;

[Severity: High]
Can this cause a spinlock deadlock on hw->port->lock when called from the SBL
PML recovery timer?

In sbl_pml_recovery_init(), recovery->timer is set up with TIMER_IRQSAFE and
calls:

  sbl_pml_recovery_monitor_fallback_timer()
    -> sbl_pml_link_down_async_alert()
      -> sbl_async_alert()
        -> cass_sbl_async_alert()
          -> cass_sbl_link_down_async_handler()
            -> cass_link_async_down()

cass_link_async_down() and its callees (cass_lmon_get_dirn(),
cass_link_get_down_origin(), and cass_lmon_request_down()) acquire
hw->port->lock using plain spin_lock(). Because process-context callers such
as cass_link_get_state(), cass_link_set_state(), cass_link_async_down_wait(),
and cass_link_sysfs_sprint() also acquire hw->port->lock with plain
spin_lock() without disabling interrupts or softirqs, if recovery->timer
fires on a CPU already holding hw->port->lock, the timer handler will spin
forever on the same lock (and under PREEMPT_RT, acquiring a sleeping
spinlock_t from a TIMER_IRQSAFE handler is invalid).

[ ... ]

> +int cass_sbl_power_on(struct cass_dev *hw)
> +{

[ ... ]

> +	atomic_inc(&hw->sbl_counters[link_up]);
> +
> +	/* report success */
> +out:
> +	cass_link_set_state(hw, CASS_LINK_STATUS_UP, 0);
> +	cass_phy_link_up(hw);
> +
> +	hw->port->start_time = ktime_get_seconds();
> +
> +	return 0;

[ ... ]

> +static int cass_sbl_serdes_config_defaults(struct cass_dev *hw)
> +{
> +	struct sbl_sc_values vals = { 0 };
> +	int rc, err;
> +
> +	/* CONFIG 1: 200G and 100G PEC to switch */
> +	u64 port_mask = 0x1;
> +	u8  serdes_mask = 0xf;
> +	u64 tp_state_mask0 = sbl_create_tp_hash0((u8)CASS_SBL_SC_MASKED_IN,
> +						 (u8)CASS_SBL_SC_MASKED_IN,
> +						 (u8)CASS_SBL_SC_MASKED_OUT,
> +						 (u8)CASS_SBL_SC_MASKED_OUT,
> +						 (u8)CASS_SBL_SC_MASKED_IN,
> +						 (u16)CASS_SBL_SC_MASKED_OUT);
> +	u64 tp_state_mask1 = sbl_create_tp_hash1(CASS_SBL_SC_MASKED_IN);
> +	u64 tp_state_match0 = sbl_create_tp_hash0(CASS_SBL_SC_LP_SWITCH,
> +						 CASS_SBL_SC_LBM_ANY,
> +						 CASS_SBL_SC_NA,
> +						 CASS_SBL_SC_NA,
> +						 CASS_SBL_SC_MT_ELEC,
> +						 CASS_SBL_SC_NA);
> +	u64 tp_state_match1 = sbl_create_tp_hash1(CASS_SBL_SC_NA);

[ ... ]

> +	// CONFIG 2: 100G AOC to switch
> +	// All the same mask/match params, save for the following:
> +	tp_state_match0 = sbl_create_tp_hash0((u8)CASS_SBL_SC_LP_SWITCH,
> +					      (u8)CASS_SBL_SC_LBM_ANY,
> +					      (u8)CASS_SBL_SC_NA,
> +					      (u8)CASS_SBL_SC_LM_100,
> +					      (u8)CASS_SBL_SC_MT_OPT_ANY,
> +					      (u16)CASS_SBL_SC_NA);

[ ... ]

> +	err = sbl_serdes_add_config(hw->sbl, tp_state_mask0, tp_state_mask1,
> +				    tp_state_match0, tp_state_match1, port_mask,
> +				   serdes_mask, &vals, false);
> +	if ((err == 0) || (err == -EEXIST))
> +		rc += 0;
> +	else
> +		rc += err;
> +
> +	// CONFIG 3: 200G AOC to switch
> +	// All the same mask/match params, save for the following:
> +	tp_state_match0 = sbl_create_tp_hash0((u8)CASS_SBL_SC_LP_SWITCH,
> +					      (u8)CASS_SBL_SC_LBM_ANY,
> +					      (u8)CASS_SBL_SC_NA,
> +					      (u8)CASS_SBL_SC_LM_200,
> +					      (u8)CASS_SBL_SC_MT_OPT_ANY,
> +					      (u16)CASS_SBL_SC_NA);

[ ... ]

> +	err = sbl_serdes_add_config(hw->sbl, tp_state_mask0, tp_state_mask1,
> +				    tp_state_match0, tp_state_match1, port_mask,
> +				   serdes_mask, &vals, false);
> +	if ((err == 0) || (err == -EEXIST))
> +		rc += 0;
> +	else
> +		rc += err;
> +
> +	if (rc)
> +		cxidev_err(&hw->cdev, "Failed to update SBL config [%d]\n", rc);
> +	return rc;
> +}

[Severity: High]
Are the state masks in cass_sbl_serdes_config_defaults() inverted or missing
fields?

First, tp_state_mask1 is set to sbl_create_tp_hash1(CASS_SBL_SC_MASKED_IN)
(~0ULL) while tp_state_match1 is sbl_create_tp_hash1(CASS_SBL_SC_NA) (0). In
sbl_get_serdes_config_values(), an entry matches hash1 only when:

  (((sc->tp_state_mask1 & hash1) & ~(sc->tp_state_mask1 & sc->tp_state_match1)) == 0)

With tp_state_mask1 = ~0ULL and tp_state_match1 = 0, this requires hash1
(which is sbl->link[port_num].mattr.len) to be 0, preventing any cable with a
non-zero length from matching any of the three configs. Should tp_state_mask1
use CASS_SBL_SC_MASKED_OUT instead?

Second, CONFIG 2 (CASS_SBL_SC_LM_100) and CONFIG 3 (CASS_SBL_SC_LM_200) match
on link_mode in tp_state_match0, but reuse tp_state_mask0 from CONFIG 1 where
the 4th argument (link_mode_mask) is CASS_SBL_SC_MASKED_OUT. Because
link_mode is masked out, sbl_get_serdes_config_values() ignores link_mode and
CONFIG 3 (inserted at the head of serdes_config_list after CONFIG 2) always
shadows CONFIG 2.

Finally, accumulating errors via rc += err sums negative errno values if more
than one sbl_serdes_add_config() call fails, returning a corrupted error code.

[ ... ]

> +int cass_sbl_reset(struct cass_dev *hw)
> +{
> +	int bl_state;
> +	int err;
> +
> +	cxidev_dbg(&hw->cdev, "link: reset (from %s)\n",
> +		   cass_link_state_str(cass_link_get_state(hw)));
> +
> +	atomic_inc(&hw->sbl_counters[link_reset_tries]);
> +
> +	cass_link_set_state(hw, CASS_LINK_STATUS_RESETTING, 0);
> +
> +	/* reset the base link */
> +	sbl_base_link_get_status(hw->sbl, 0, &bl_state, NULL, NULL, NULL,
> +				 NULL, NULL);
> +
> +	if (bl_state != SBL_BASE_LINK_STATUS_UNCONFIGURED) {
> +		err = sbl_base_link_reset(hw->sbl, 0);
> +		if (err)
> +			cxidev_err(&hw->cdev,
> +				   "link: reset, base link reset failed [%d]\n",
> +				   err);
> +	}
> +
> +	spin_lock(&hw->sbl_state_lock);
> +	hw->port->prev_lstate = hw->port->lstate;
> +	hw->port->lstate = CASS_LINK_STATUS_UNCONFIGURED;
> +	hw->port->lerr = 0;
> +	hw->port->config_state &= ~CASS_LINK_CONFIGURED;
> +	spin_unlock(&hw->sbl_state_lock);

[Severity: Medium]
Should cass_sbl_reset() acquire hw->port->lock (or call cass_link_set_state())
instead of hw->sbl_state_lock here?

Every other reader and writer of hw->port->lstate, hw->port->prev_lstate, and
hw->port->lerr (including cass_link_set_state() earlier in cass_sbl_reset())
synchronizes on hw->port->lock, while hw->sbl_state_lock is not used anywhere
else in the driver.

[ ... ]

> diff --git a/drivers/net/ethernet/hpe/ss1/cass_sl.c b/drivers/net/ethernet/hpe/ss1/cass_sl.c
> new file mode 100644
> index 0000000000000..72c8724a33276
> --- /dev/null
> +++ b/drivers/net/ethernet/hpe/ss1/cass_sl.c
> @@ -0,0 +1,1524 @@

[ ... ]

> +void cass_sl_mode_set(struct cass_dev *cass_dev, const struct cxi_link_info *link_info)
> +{

[ ... ]

> +	/* loopback */
> +	cxidev_dbg(&cass_dev->cdev, "sl mode set (loopback_mode = %lu)\n",
> +		   link_info->flags & LOOPBACK_MODE);
> +	switch (link_info->flags & LOOPBACK_MODE) {
> +	case 0:
> +		if (!(lgrp_config->options & SL_LGRP_CONFIG_OPT_SERDES_LOOPBACK_ENABLE) &&
> +		    !(link_config->options & SL_LINK_CONFIG_OPT_REMOTE_LOOPBACK_ENABLE))
> +			break;
> +		cxidev_dbg(&cass_dev->cdev, "sl mode set loopback to off\n");
> +		lgrp_config->options &= ~SL_LGRP_CONFIG_OPT_SERDES_LOOPBACK_ENABLE;
> +		link_config->options &= ~SL_LINK_CONFIG_OPT_REMOTE_LOOPBACK_ENABLE;
> +		if (cass_dev->sl.old_an_mode == AUTONEG_ENABLE)
> +			cass_sl_mode_set_autoneg_enable(cass_dev);
> +		else
> +			cass_sl_mode_set_autoneg_disable(cass_dev);
> +		if (cass_dev->sl.old_lt_mode)
> +			link_config->hpe_map |= SL_LINK_CONFIG_HPE_LINKTRAIN;
> +		else
> +			link_config->hpe_map &= ~SL_LINK_CONFIG_HPE_LINKTRAIN;
> +		is_mode_changed = true;
> +		break;
> +	case CXI_ETH_PF_INTERNAL_LOOPBACK:
> +		if (lgrp_config->options & SL_LGRP_CONFIG_OPT_SERDES_LOOPBACK_ENABLE)
> +			break;
> +		cxidev_dbg(&cass_dev->cdev, "sl mode set serdes loopback to on\n");
> +		lgrp_config->options |= SL_LGRP_CONFIG_OPT_SERDES_LOOPBACK_ENABLE;
> +		cass_sl_mode_set_autoneg_disable(cass_dev);
> +		link_config->options &= ~SL_LINK_CONFIG_OPT_REMOTE_LOOPBACK_ENABLE;
> +		cass_dev->sl.old_an_mode = link_info->autoneg;
> +		cass_dev->sl.old_lt_mode = (link_config->hpe_map & SL_LINK_CONFIG_HPE_LINKTRAIN);
> +		link_config->hpe_map &= ~SL_LINK_CONFIG_HPE_LINKTRAIN;
> +		is_mode_changed = true;
> +		break;
> +	case CXI_ETH_PF_EXTERNAL_LOOPBACK:
> +		if (link_config->options & SL_LINK_CONFIG_OPT_REMOTE_LOOPBACK_ENABLE)
> +			break;
> +		cxidev_dbg(&cass_dev->cdev, "sl mode set remote loopback to on\n");
> +		link_config->options |= SL_LINK_CONFIG_OPT_REMOTE_LOOPBACK_ENABLE;
> +		cass_sl_mode_set_autoneg_disable(cass_dev);
> +		lgrp_config->options &= ~SL_LGRP_CONFIG_OPT_SERDES_LOOPBACK_ENABLE;
> +		cass_dev->sl.old_an_mode = link_info->autoneg;
> +		cass_dev->sl.old_lt_mode = (link_config->hpe_map & SL_LINK_CONFIG_HPE_LINKTRAIN);
> +		link_config->hpe_map &= ~SL_LINK_CONFIG_HPE_LINKTRAIN;
> +		is_mode_changed = true;
> +		break;
> +	}

[Severity: Medium]
If a user switches directly from internal loopback to external loopback (or
vice versa) without turning loopback off first, won't this overwrite
cass_dev->sl.old_an_mode and cass_dev->sl.old_lt_mode with the
loopback-disabled values (AUTONEG_DISABLE and 0), preventing case 0 from
restoring the original autonegotiation and link-training settings when
loopback is later disabled?

[ ... ]

> +void cass_sl_pml_recovery_set(struct cass_dev *cass_dev, bool set)
> +{
> +	cxidev_dbg(&cass_dev->cdev, "pml recovery set\n");
> +
> +	if (set)
> +		cass_dev->sl.link_config.options |= SL_LINK_CONFIG_OPT_PML_REC_ENABLE;
> +	else
> +		cass_dev->sl.link_config.options &= ~SL_LINK_CONFIG_OPT_PML_REC_ENABLE;
> +}

[Severity: Medium]
How are PML recovery state and polarity meant to work across SBL and SL?

1. In cass_sbl_pml_recovery_set(), passing set == true calls
sbl_disable_pml_recovery(hw->sbl, 0, true), which disables PML recovery
(without updating hw->port->lattr.bl.options). In cass_sl_pml_recovery_set(),
passing set == true sets SL_LINK_CONFIG_OPT_PML_REC_ENABLE, which enables PML
recovery. Both implement the exported cxi_pml_recovery_set() API with opposite
boolean semantics.

2. In cass_sl_config_init(), SL_LINK_CONFIG_OPT_PML_REC_ENABLE is set by
default, but cass_sl_mode_get() never sets CXI_ETH_PF_PML_REC in
link_info->flags, and cass_sl_mode_set() never handles CXI_ETH_PF_PML_REC. As
a result, ethtool private flags always report pml-recovery as off on Cassini 2,
and attempting to disable it via ethtool sees no flag change.

3. Neither cass_sl_pml_recovery_set() nor cass_sl_mode_set() calls
sl_link_config_set() or bounces the link when toggling PML recovery, so changes
to cass_dev->sl.link_config.options are not applied to an active SL link.

[ ... ]

> +static int cass_sl_dt_info_get(void *dt_accessor, u8 ldev_num, u8 lgrp_num,
> +			       struct sl_dt_lgrp_info *info)
> +{

[ ... ]

> +	default:  /* straight */
> +		info->lane_info[3].tx_source = 3;
> +		info->lane_info[3].tx_invert = 0;
> +		info->lane_info[2].tx_source = 2;
> +		info->lane_info[2].tx_invert = 0;
> +		info->lane_info[1].tx_source = 1;
> +		info->lane_info[1].tx_invert = 0;
> +		info->lane_info[0].tx_source = 0;
> +		info->lane_info[0].tx_invert = 0;
> +		break;
> +	}
> +
> +	return 0;
> +}

[Severity: Medium]
In the default straight-mapping case of cass_sl_dt_info_get(), should
rx_source and rx_invert also be initialized for info->lane_info[0..3]?

Without initializing rx_source in the default branch,
sl_core_data_lgrp_new() and sl_core_hw_serdes_swizzles() leave all four RX
lanes mapped to physical lane 0 (or with stale values after SerDes loopback).

[ ... ]

> +static int cass_sl_dmac_xfer(void *dmac_accessor, void *data)
> +{
> +	int              rtn;
> +	struct cass_dev *cass_dev = dmac_accessor;
> +
> +	cxidev_dbg(&cass_dev->cdev, "sl dmac xfer\n");
> +
> +	rtn = cxi_dmac_xfer(&cass_dev->cdev, cass_dev->sl.fec.dma_id);
> +	if (rtn) {
> +		cxidev_err(&cass_dev->cdev, "Unable to get fec cntrs [%d]\n", rtn);
> +		return rtn;
> +	}
> +
> +	memcpy(data, cass_dev->sl.fec.cntrs, cass_dev->sl.fec.cntrs_size);
> +
> +	return 0;
> +}

[Severity: Medium]
Can concurrent callers of cass_sl_dmac_xfer() race on the shared DMA buffer
cass_dev->sl.fec.cntrs?

cxi_dmac_xfer() releases hw->dmac.lock before returning, so if a sysfs FEC
counter read (such as ccw_show() or ucw_show()) and the periodic FEC monitor
work item call sl_core_hw_fec_dmac_xfer() -> cass_sl_dmac_xfer() concurrently,
a second DMA transfer into cass_dev->sl.fec.cntrs can overwrite the buffer
while the first caller is still running memcpy().

[ ... ]

> +static void cass_sl_callback(void *tag, struct sl_lgrp_notif_msg *msg)
> +{
> +	struct cass_dev *cass_dev = tag;
> +
> +	cxidev_dbg(&cass_dev->cdev, "callback (type = 0x%X)\n", msg->type);
> +
> +	switch (msg->type) {
> +	case SL_LGRP_NOTIF_MEDIA_PRESENT:
> +		cass_dev->sl.media_attr = msg->info.media_attr;
> +		if (msg->info.media_attr.type & SL_MEDIA_TYPE_ACTIVE) {
> +			cass_dev->sl.link_config.hpe_map &= ~SL_LINK_CONFIG_HPE_LINKTRAIN;
> +			cass_sl_mode_set_autoneg_disable(cass_dev);
> +		} else {
> +			cass_dev->sl.link_config.hpe_map |= SL_LINK_CONFIG_HPE_LINKTRAIN;
> +			cass_sl_mode_set_autoneg_enable(cass_dev);
> +		}
> +		cass_dev->sl.has_cable = true;
> +		break;
> +	case SL_LGRP_NOTIF_MEDIA_NOT_PRESENT:
> +		memset(&cass_dev->sl.media_attr, 0, sizeof(cass_dev->sl.media_attr));
> +		cass_dev->sl.has_cable = false;
> +		break;

[Severity: Medium]
What serializes access to cass_dev->sl.link_config, cass_dev->sl.lgrp_config,
cass_dev->sl.link_policy, and cass_dev->sl.media_attr?

cass_sl_callback() runs on SL's notification workqueue and updates
media_attr, link_config.hpe_map, link_config.options, and
lgrp_config.tech_map without locking, while cass_sl_mode_set(),
cass_sl_mode_get(), cass_sl_pml_recovery_set(), the debugfs handlers in
cxi_link.c, and cass_sl_link_up() concurrently read and modify those same
fields.

[ ... ]

> +int cass_sl_init(struct cass_dev *cass_dev)
> +{

[ ... ]

> +	cass_dev->sl.ldev = sl_ldev_new(cass_dev->cdev.cxi_num,
> +					NULL, &(cass_dev->sl.ldev_config));
> +	if (IS_ERR(cass_dev->sl.ldev)) {
> +		rtn = PTR_ERR(cass_dev->sl.ldev);
> +		cxidev_err(&cass_dev->cdev, "sl_ldev_new failed [%d]\n", rtn);
> +		goto out_kobject_put_port_num;
> +	}

[Severity: Medium]
Can passing cass_dev->cdev.cxi_num as ldev_num to sl_ldev_new() cause probe
failures after 16 device probes?

In cass_probe(), hw->cdev.cxi_num is assigned via atomic_inc_return(&cxi_num)
for every PF and VF probe and is never decremented or recycled. However,
sl_ldev_new() requires ldev_num < SL_ASIC_MAX_LDEVS (16) and returns -EINVAL
once cxi_num reaches 16 (for example, after enabling SR-IOV VFs or unbinding
and rebinding PFs).

[ ... ]

> +int cass_sl_link_up(struct cass_dev *cass_dev)
> +{
> +	int          rtn;
> +	unsigned int timeleft;
> +
> +	cxidev_dbg(&cass_dev->cdev, "sl link up\n");
> +
> +	if (!cass_dev->sl.has_cable)
> +		return 0;
> +
> +	if (cass_dev->sl.is_canceled)
> +		return 0;
> +
> +	/* initial delay between attempts */
> +	msleep(1000);
> +
> +	cass_dev->sl.link_state       = SL_LINK_STATE_DOWN;
> +	cass_dev->sl.llr_state        = SL_LLR_STATE_OFF;
> +	cass_dev->sl.link_up_mode     = 0;
> +	cass_dev->sl.link_up_fec_type = 0;

[Severity: High]
Should cass_sl_link_up() transition hw->port->lstate to
CASS_LINK_STATUS_STARTING before sleeping, and should cass_sl_link_down() set
CASS_LINK_STATUS_STOPPING (matching cass_sbl_power_on() and
cass_sbl_power_off())?

Because cass_sl_link_up() leaves hw->port->lstate at CASS_LINK_STATUS_DOWN
throughout link bring-up (which can sleep for tens of seconds across
msleep(1000), sl_ldev_serdes_init(), sl_link_up(), sl_llr_setup(), and
sl_llr_start()), any concurrent caller of cass_link_async_down_wait() (such as
cass_phy_stop(hw, true) during cass_phy_bounce() or cass_remove(), or
cass_link_headshell_down_no_media() on cable removal) or
cass_lmon_request_down() sees hw->port->lstate == CASS_LINK_STATUS_DOWN and
returns 0 immediately while cass_sl_link_up() is still running on the lmon
kthread.

[ ... ]

> +	if (cass_dev->sl.is_canceled)
> +		goto out_llr_stop;
> +
> +	cass_link_set_state(cass_dev, CASS_LINK_STATUS_UP, 0);
> +
> +	return 0;
> +
> +out_llr_stop:
> +	sl_llr_stop(cass_dev->sl.llr);
> +	/* need to stop twice to make sure both setup and start are stopped */
> +	sl_llr_stop(cass_dev->sl.llr);
> +
> +out_mac_rx_stop:
> +	sl_mac_rx_stop(cass_dev->sl.mac);
> +
> +out_mac_tx_stop:
> +	sl_mac_tx_stop(cass_dev->sl.mac);
> +
> +out_link_down:
> +	reinit_completion(&(cass_dev->sl.step_complete));
> +	rtn = sl_link_down(cass_dev->sl.link);
> +	switch (rtn) {
> +	case -EALREADY:
> +		break;
> +	case -EINPROGRESS:
> +		msleep(500);
> +		break;
> +	case 0:
> +		timeleft = wait_for_completion_timeout(&(cass_dev->sl.step_complete),
> +			msecs_to_jiffies(2 * CASS_SL_LINK_DOWN_TIMEOUT_MS));
> +		if (timeleft == 0)
> +			cxidev_dbg(&cass_dev->cdev, "sl_link_down (link_up) timeout\n");
> +		break;
> +	default:
> +		cxidev_warn(&cass_dev->cdev, "sl_link_down (link_up) failed [%d]\n", rtn);
> +		break;
> +	}
> +
> +out_no_link:
> +
> +	return -ENOLINK;
> +}
> +
> +int cass_sl_link_down(struct cass_dev *cass_dev)
> +{

[ ... ]

> +	/* down link */
> +	if (cass_dev->sl.link_state != SL_LINK_STATE_DOWN) {
> +		reinit_completion(&(cass_dev->sl.step_complete));
> +		rtn = sl_link_down(cass_dev->sl.link);
> +		switch (rtn) {
> +		case -EALREADY:
> +			break;
> +		case -EINPROGRESS:
> +		case 0:
> +			timeleft = wait_for_completion_timeout(&(cass_dev->sl.step_complete),
> +				msecs_to_jiffies(2 * CASS_SL_LINK_DOWN_TIMEOUT_MS));
> +			if (timeleft == 0) {
> +				cxidev_err(&cass_dev->cdev,
> +					   "sl_link_down (link_down) timed out\n");
> +				return -ETIMEDOUT;
> +			}
> +			break;
> +		default:
> +			cxidev_warn(&cass_dev->cdev, "sl_link_down (link_down) failed [%d]\n", rtn);
> +			return rtn;
> +		}
> +		if (cass_dev->sl.link_state != SL_LINK_STATE_DOWN)
> +			return -EBUSY;
> +	}
> +
> +	if (err)
> +		return err;
> +
> +	cass_link_set_state(cass_dev, CASS_LINK_STATUS_DOWN, 0);
> +
> +	return 0;
> +}

[Severity: High]
Won't cass_sl_link_down() return -EBUSY whenever sl_link_down() returns
-EALREADY, causing cass_sl_link_fini() to abort teardown and leak SL and port
resources?

In cass_sl_init(), memset(&(cass_dev->sl), 0, sizeof(cass_dev->sl)) sets
cass_dev->sl.link_state to 0 (SL_LINK_STATE_INVALID), whereas sl_link_new()
initializes the underlying ctrl_link->state to SL_LINK_STATE_DOWN (1). If no
cable is present (!cass_dev->sl.has_cable), cass_sl_link_up() returns early
before setting cass_dev->sl.link_state = SL_LINK_STATE_DOWN.

When cass_sl_link_fini() then calls cass_sl_link_down(),
cass_dev->sl.link_state != SL_LINK_STATE_DOWN (0 != 1) is true, so it calls
sl_link_down(). Because ctrl_link->state is already SL_LINK_STATE_DOWN,
sl_link_down() returns -EALREADY without queuing a notification. The
-EALREADY case breaks out of the switch without setting
cass_dev->sl.link_state = SL_LINK_STATE_DOWN, and the check immediately after
the switch returns -EBUSY, aborting cass_sl_link_fini() and leaking ldev,
lgrp, link, mac, llr, port_num_kobj, port_kobj, and cass_dev->port.

[Severity: High]
Can cass_dev->sl.link_state and cass_dev->sl.step_complete also get out of
sync with SL's asynchronous notifications?

1. When an async link-down or fault occurs in SL, ctrl_link->state is set to
SL_LINK_STATE_DOWN before the notification workqueue runs cass_sl_callback()
to update cass_dev->sl.link_state. If cass_sl_link_down() runs during that
window, cass_dev->sl.link_state is still SL_LINK_STATE_UP, sl_link_down()
returns -EALREADY, and cass_sl_link_down() returns -EBUSY.

2. Conversely, while sl_link_up() is in progress (SL_LINK_STATE_STARTING),
cass_dev->sl.link_state is SL_LINK_STATE_DOWN, so cass_sl_link_down() sees
cass_dev->sl.link_state == SL_LINK_STATE_DOWN and skips calling
sl_link_down().

3. Because a single step_complete completion is shared between link
notifications (SL_LGRP_NOTIF_LINK_*) and LLR notifications
(SL_LGRP_NOTIF_LLR_*), and out_link_down in cass_sl_link_up() only sleeps
500ms on -EINPROGRESS instead of waiting for completion, an in-flight
notification from one step can prematurely satisfy wait_for_completion_timeout()
in a later step.

[Severity: High]
There also appear to be several mismatches between how SBL, SL, and the PHY
state machine handle link up/down events:

1. On Cassini 1 (SBL), cass_sbl_power_on() sets CASS_LINK_STATUS_UP and calls
cass_phy_link_up(hw) directly while hw->phy.state is still CASS_PHY_NOLINK.
When cass_phy_state_machine() runs its next poll, cass_phy_check_link_status()
sees hw->phy.state != CASS_PHY_RUNNING, transitions to CASS_PHY_RUNNING, and
calls cass_phy_link_up(hw) a second time, emitting a duplicate
CXI_EVENT_LINK_UP.

2. Once hw->phy.state reaches CASS_PHY_RUNNING, cass_phy_state_machine() in
cass_phy.c stops rescheduling hw->phy.state_queue:

	new_state = hw->phy.state;
	if (new_state != CASS_PHY_RUNNING)
		cass_phy_queue_locked(hw, HZ);

On Cassini 2 (SL), cass_sl_link_down() only calls cass_link_set_state() and
never calls cass_phy_link_down(). When cass_phy_stop() cancels state_queue and
brings the link down, or when the link goes down while state_queue is not
polling, cass_phy_link_down() is never called and CXI_EVENT_LINK_DOWN is never
sent.

3. Unlike cass_sbl_power_on(), cass_sl_link_up() does not check if
cass_link_get_state(cass_dev) == CASS_LINK_STATUS_UP at entry (so calling it
when already up resets cass_dev->sl.link_state to SL_LINK_STATE_DOWN, fails
sl_link_up() with -EALREADY, and tears the link down in out_link_down), nor
does it update cass_dev->port->start_time = ktime_get_seconds().

[ ... ]

> diff --git a/drivers/net/ethernet/hpe/ss1/cass_sl_io.c b/drivers/net/ethernet/hpe/ss1/cass_sl_io.c
> new file mode 100644
> index 0000000000000..5eca53cb09691
> --- /dev/null
> +++ b/drivers/net/ethernet/hpe/ss1/cass_sl_io.c
> @@ -0,0 +1,368 @@

[ ... ]

> +int cass_sl_sbus_rd(void *sbus_accessor, u32 sbus_addr, u8 reg_addr, u32 *rd_data)
> +{

[ ... ]

> +	mutex_lock(&cass_dev->sl.sbus_lock);
> +
> +	rtn = cxi_sbus_op(&cass_dev->cdev, &params, rd_data, &result_code, &overrun);
> +	if (rtn != 0) {
> +		cxidev_err(&cass_dev->cdev, "sl sbus rd - cxi_sbus_op failed [%d]\n", rtn);
> +		goto out;
> +	}

[ ... ]

> +int cass_sl_pmi_rd(void *pmi_accessor, u8 lgrp_num, u32 addr, u16 *data)
> +{
> +	int                           rtn;
> +	struct cass_dev              *cass_dev = pmi_accessor;
> +	int                           x;
> +	union c2_hni_serdes_pmi_ctl   pmi_ctl;
> +	union c2_hni_serdes_pmi_data  pmi_data;
> +
> +	mutex_lock(&cass_dev->sl.pmi_lock);
> +
> +	/* check ready */
> +	for (x = 0; x < CASS_SL_PMI_RD_READY_MAX_COUNT; ++x) {
> +		cass_read(cass_dev, C2_HNI_SERDES_PMI_CTL, &pmi_ctl, sizeof(pmi_ctl));
> +		if (!pmi_ctl.pmi_lp_ack)
> +			break;
> +		msleep(CASS_SL_PMI_RD_READY_DELAY_MS);
> +	}
> +	if (x >= CASS_SL_PMI_RD_READY_MAX_COUNT) {
> +		cxidev_err(&cass_dev->cdev,
> +			   "sl pmi rd - ready timeout (ctl = 0x%016llX)\n", pmi_ctl.qw);
> +		rtn = -1;
> +		goto out;
> +	}
> +
> +	/* start */
> +	pmi_ctl.qw          = 0ULL;
> +	pmi_ctl.pmi_lp_addr = addr;
> +	pmi_ctl.pmi_lp_en   = 1;
> +	cxidev_dbg(&cass_dev->cdev, "sl pmi rd - ctrl 0x%016llX -> 0x%08X\n",
> +		   pmi_ctl.qw, C2_HNI_SERDES_PMI_CTL);
> +	cass_write(cass_dev, C2_HNI_SERDES_PMI_CTL, &pmi_ctl, sizeof(pmi_ctl));
> +
> +	/* poll for done */
> +	for (x = 0; x < CASS_SL_PMI_RD_MAX_COUNT; ++x) {
> +		cass_read(cass_dev, C2_HNI_SERDES_PMI_CTL, &pmi_ctl, sizeof(pmi_ctl));
> +		cxidev_dbg(&cass_dev->cdev, "sl pmi rd - check #%d 0x%08X = 0x%016llX\n",
> +			   x, C2_HNI_SERDES_PMI_CTL, pmi_ctl.qw);
> +		if (pmi_ctl.pmi_lp_error) {
> +			cxidev_err(&cass_dev->cdev, "sl pml rd - error\n");
> +			rtn = -1;
> +			goto out;
> +		}
> +		if (pmi_ctl.pmi_lp_ack)
> +			break;
> +		msleep(CASS_SL_PMI_RD_DELAY_MS);
> +	}
> +	if (x >= CASS_SL_PMI_RD_MAX_COUNT) {
> +		cxidev_err(&cass_dev->cdev, "sl pmi rd - timeout\n");
> +		rtn = -ETIMEDOUT;
> +		goto out;
> +	}
> +
> +	/* check valid */
> +	if (!pmi_ctl.pmi_lp_read_vld) {
> +		cxidev_err(&cass_dev->cdev, "sl pmi rd - data not valid\n");
> +		rtn = -1;
> +		goto out;
> +	}

[Severity: Medium]
When cass_sl_pmi_rd() or cass_sl_sbus_rd() fails and jumps to out without
initializing *data or *rd_data, can uninitialized kernel stack memory be
exposed via sysfs and trace logs?

Callers such as sl_core_hw_serdes_lane_pre1_get() pass uninitialized stack
variables into SL_CORE_HW_PMI_RD() -> pmi_rd() -> sl_core_pmi_rd() ->
cass_sl_pmi_op() -> cass_sl_pmi_rd() and SL_CORE_HW_SBUS_FIELD_RD() ->
sbus_field_rd() -> sl_core_sbus_rd() -> cass_sl_sbus_op() ->
cass_sl_sbus_rd(). On error, sl_core_pmi_rd(), pmi_rd(), sl_core_sbus_rd(),
sbus_rd(), and sbus_field_rd() still dereference *data, log it, and store it
into core_lgrp->pmi.rd.data and core_lgrp->sbus.rd.data, which are readable by
unprivileged users via 0444 sysfs attributes.

Also, in cass_sl_pmi_rd() and cass_sl_pmi_wr(), should the ready-timeout,
pmi_lp_error, and !pmi_lp_read_vld error paths return standard negative errno
codes (-ETIMEDOUT / -EIO) instead of -1 (-EPERM)?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20261030-cxi-netdev-submission-v1-0-8c2c391cc283@hpe.com?part=18