Re: [PATCH 05/23] net: hpe: sbl: add the PML link layer

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 7 potential issue(s) to consider:
- [High] net: hpe: sbl: non-raw spinlock_t acquired in TIMER_IRQSAFE recovery timer
- [Medium] net: hpe: sbl: down_origin overwritten after degrade failure in sbl_pml_hdlr()
- [Medium] net: hpe: sbl: race in sbl_pml_recovery_monitor_fallback_timer() drops faults
- [Medium] net: hpe: sbl: non-zero llr_loop_time left on error in sbl_pml_llr_start()
- [Medium] net: hpe: sbl: stale link->llr_options across restarts in sbl_pml_llr_config()
- [Medium] net: hpe: sbl: missing link_info flag update in sbl_pml_llr_enable()
- [Low] net: hpe: sbl: duplicate state flag in sbl_pml_pcs_am_start()
--

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

net: hpe: sbl: add the PML link layer

This commit adds the physical media layer (PML) implementation for the HPE
Slingshot base link driver, covering MAC, PCS, LLR configuration, and SerDes
operation hooks.

> diff --git a/drivers/net/ethernet/hpe/sbl/sbl_pml.c b/drivers/net/ethernet/hpe/sbl/sbl_pml.c
> new file mode 100644
> index 0000000000000..367c3184fb799
> --- /dev/null
> +++ b/drivers/net/ethernet/hpe/sbl/sbl_pml.c
> @@ -0,0 +1,787 @@

[ ... ]

> +static void sbl_pml_recovery_monitor_fallback_timer(struct timer_list *t)
> +{
> +	struct sbl_pml_recovery *pml_recovery = container_of(t, struct sbl_pml_recovery, timer);
> +	struct sbl_inst *sbl;
> +	struct sbl_link *link;
> +	int port_num;
> +	enum sbl_link_down_origin down_origin;
> +	u32 elapsed;
> +	u32 rl_total_time;
> +	unsigned long timeout;
> +	unsigned long irq_flags;
> +
> +	if (!pml_recovery || !pml_recovery->sbl)
> +		return;
> +
> +	spin_lock_irqsave(&pml_recovery->lock, irq_flags);
> +	if (!pml_recovery->enabled || !pml_recovery->started) {
> +		spin_unlock_irqrestore(&pml_recovery->lock, irq_flags);
> +		return;
> +	}
> +	spin_unlock_irqrestore(&pml_recovery->lock, irq_flags);
> +
> +	sbl = pml_recovery->sbl;
> +	port_num = pml_recovery->port_num;
> +	down_origin = pml_recovery->down_origin;
> +	link = sbl->link + port_num;
> +	timeout = pml_recovery->init_jiffies + msecs_to_jiffies(pml_recovery->timeout);
> +	elapsed = jiffies_to_msecs(jiffies - link->pml_recovery.init_jiffies);
> +	link->pml_recovery.rl_time_remaining -=
> +		jiffies_to_msecs(jiffies -
> +				 link->pml_recovery.last_poll_jiffies);
> +	link->pml_recovery.last_poll_jiffies  = jiffies;
> +
> +	if (sbl_pml_recovery_no_faults(sbl, port_num)) {
> +		sbl_dev_info(sbl->dev, "%d: PML recovered successfully in %ums", port_num, elapsed);
> +		sbl_link_counters_incr(sbl, port_num, pml_recovery_successes);
> +		sbl_pml_recovery_origin_counter_update(sbl, port_num, down_origin);
> +		sbl_link_counters_incr(sbl, port_num, (elapsed < SBL_PML_REC_HISTOGRAM_MAX) ?
> +				      (pml_recovery_histogram_0_9ms + elapsed / 10) :
> +				       pml_recovery_histogram_high);
> +		goto out;
> +	} else if (time_is_before_eq_jiffies(timeout)) {
> +		sbl_dev_info(sbl->dev, "%d: PML recovery monitor timed out (%ums)", port_num, elapsed);
> +		goto out_fail;
> +	} else if (!sbl_pml_recovery_rate_test(sbl, port_num)) {
> +		rl_total_time = link->blattr.pml_recovery.rl_max_duration - link->pml_recovery.rl_time_remaining;
> +		sbl_dev_err(sbl->dev, "%d: PML recovery rate exceeded (%ums/%ums) after %ums", port_num, rl_total_time,
> +			    link->blattr.pml_recovery.rl_window_size, elapsed);
> +		sbl_link_counters_incr(sbl, port_num, pml_recovery_rate_exceeded);
> +		goto out_fail;
> +	}
> +
> +	/* start timer for next poll */
> +	spin_lock_irqsave(&pml_recovery->lock, irq_flags);
> +	if (pml_recovery->enabled)
> +		mod_timer(&pml_recovery->timer,
> +			  jiffies + msecs_to_jiffies(SBL_PML_REC_POLL_INTERVAL));
> +	spin_unlock_irqrestore(&pml_recovery->lock, irq_flags);
> +	return;
> +
> +out_fail:
> +	sbl_pml_link_down_async_alert(sbl, port_num, down_origin);
> +
> +out:
> +	spin_lock_irqsave(&link->fec_discard_lock, irq_flags);
> +	link->fec_discard_time = jiffies;
> +	link->fec_discard_type = SBL_FEC_DISCARD_TYPE_PML_REC_END;
> +	spin_unlock_irqrestore(&link->fec_discard_lock, irq_flags);
> +	spin_lock_irqsave(&pml_recovery->lock, irq_flags);
> +	pml_recovery->started = false;
> +	spin_unlock_irqrestore(&pml_recovery->lock, irq_flags);
> +}

[Severity: Medium]
Can releasing pml_recovery->lock before polling hardware status and only
clearing pml_recovery->started at the end of
sbl_pml_recovery_monitor_fallback_timer() cause a concurrent fault interrupt
to be dropped?

After sbl_pml_recovery_no_faults() reads SBL_PML_STS_RX_PCS_OFFSET and
returns true, the callback logs recovery success, updates counters, and
takes link->fec_discard_lock before re-acquiring pml_recovery->lock to set
pml_recovery->started = false.

If a new fault interrupt arrives in sbl_pml_hdlr() during that window:

CPU0 (timer)                                  CPU1 (interrupt)
sbl_pml_recovery_monitor_fallback_timer()
  sbl_pml_recovery_no_faults() -> true
                                              sbl_pml_hdlr()
                                                sbl_pml_recovery_monitor()
                                                  if (!recovery->enabled ||
                                                      recovery->started)
                                                    return;
                                                sbl_write64(... SBL_PML_ERR_CLR_OFFSET,
                                                            raised_flgs);
  pml_recovery->started = false;

Wouldn't sbl_pml_recovery_monitor() return early without restarting alignment
or arming the timer while sbl_pml_hdlr() clears the hardware error flag?

Also, on the out_fail path, sbl_pml_link_down_async_alert() is called without
re-checking pml_recovery->enabled, and rl_time_remaining, last_poll_jiffies,
and rl_window_start are updated without holding pml_recovery->lock. If
sbl_pml_recovery_cancel() sets recovery->enabled = false and waits in
timer_delete_sync(), could out_fail still emit a spurious link-down alert?

> +
> +void sbl_pml_recovery_init(struct sbl_pml_recovery *recovery)
> +{
> +	spin_lock_init(&recovery->lock);
> +	recovery->enabled = false;
> +	timer_setup(&recovery->timer, sbl_pml_recovery_monitor_fallback_timer,
> +		    TIMER_IRQSAFE);
> +}

[Severity: High]
Does setting up recovery->timer with TIMER_IRQSAFE in sbl_pml_recovery_init()
trigger an invalid wait context warning on PREEMPT_RT?

Because TIMER_IRQSAFE callbacks run with hardware interrupts disabled,
sbl_pml_recovery_monitor_fallback_timer() acquires non-raw spinlock_t locks
(pml_recovery->lock, link->fec_discard_lock, and sbl->name_lock via
sbl_dev_info() and sbl_dev_err()) while IRQs are disabled. Since
sbl_pml_recovery_cancel() is called from process context, is TIMER_IRQSAFE
needed here?

[ ... ]

> +int sbl_pml_hdlr(struct sbl_inst *sbl, int port_num, void *data)
> +{

[ ... ]

> +	result = SBL_PML_ERR_FLG_PCS_TX_DEGRADE_FAILURE_GET(raised_flgs);
> +
> +	if (result) {
> +		alert = SBL_ASYNC_ALERT_TX_DEGRADE_FAILURE;
> +		sbl_async_alert(sbl, port_num, alert, NULL, 0);
> +	}
> +
> +	result = SBL_PML_ERR_FLG_PCS_RX_DEGRADE_FAILURE_GET(raised_flgs);
> +
> +	if (result) {
> +		alert = SBL_ASYNC_ALERT_RX_DEGRADE_FAILURE;
> +		sbl_async_alert(sbl, port_num, alert, NULL, 0);
> +	}
> +
> +	if (alert != SBL_ASYNC_ALERT_INVALID) {
> +		/* if Auto Lane Degrade is enabled, we will print this message regardless of whether
> +		 * the link went down because of lane degrade failure or not
> +		 */
> +		sbl_dev_err(sbl->dev, "%d: pml hdlr - link going down - all lanes down [%d]", port_num, alert);
> +		down_origin = SBL_LINK_DOWN_ORIGIN_DEGRADE_FAILURE;
> +	}
> +
> +	/* autoneg err flags */
> +	if (raised_flgs & SBL_AUTONEG_ERR_FLGS) {
> +		sbl_pml_disable_intr_handler(sbl, port_num, SBL_AUTONEG_ERR_FLGS);
> +		complete(&link->an_hw_change);
> +	}
> +
> +	/* link faults */
> +	if (SBL_PML_ERR_FLG_PCS_HI_SER_GET(raised_flgs)) {
> +		if (sbl_debug_option(sbl, port_num, SBL_DEBUG_IGNORE_HISER)) {
> +			sbl_dev_warn(sbl->dev, "%d: pml hdlr - hiser - ignored", port_num);
> +		} else {
> +			sbl_dev_dbg(sbl->dev, "%d: pml hdlr - hiser", port_num);
> +			down_origin = SBL_LINK_DOWN_ORIGIN_HISER;
> +		}
> +	}
> +
> +	val64 = sbl_read64(sbl, base | SBL_PML_CFG_LLR_SM_OFFSET);
> +	if (SBL_PML_ERR_FLG_LLR_REPLAY_AT_MAX_GET(raised_flgs) &&
> +	    SBL_PML_CFG_LLR_SM_REPLAY_CT_MAX_GET(val64) < SBL_LLR_REPLAY_CT_MAX_UNLIMITED) {
> +		sbl_dev_dbg(sbl->dev, "%d: pml hdlr - max llr replay", port_num);
> +		down_origin = SBL_LINK_DOWN_ORIGIN_LLR_MAX;
> +	}
> +
> +	if (SBL_PML_ERR_FLG_PCS_LINK_DOWN_GET(raised_flgs)) {
> +		val64 = sbl_read64(sbl, base | SBL_PML_STS_RX_PCS_OFFSET);
> +
> +		if (sbl_debug_option(sbl, port_num, SBL_DEBUG_TRACE_PML_INT)) {
> +			sbl_dev_info(sbl->dev, "%d: pml hdlr - link down (%s)", port_num,
> +				     sbl_pml_pcs_state_str(sbl, port_num, pcs_state_str,
> +							   SBL_PCS_STATE_STR_LEN));
> +		}
> +
> +		if (!SBL_PML_STS_RX_PCS_ALIGN_STATUS_GET(val64)) {
> +			if (sbl_debug_option(sbl, port_num, SBL_DEBUG_IGNORE_ALIGN)) {
> +				sbl_dev_warn(sbl->dev, "%d: pml hdlr - align - ignored", port_num);
> +			} else {
> +				sbl_dev_dbg(sbl->dev, "%d: pml hdlr - align", port_num);
> +				down_origin = SBL_LINK_DOWN_ORIGIN_ALIGN;
> +			}
> +		} else if (SBL_PML_STS_RX_PCS_LOCAL_FAULT_GET(val64)) {
> +			sbl_dev_dbg(sbl->dev, "%d: pml hdlr - local fault", port_num);
> +			down_origin = SBL_LINK_DOWN_ORIGIN_LOCAL_FAULT;
> +		} else if (SBL_PML_STS_RX_PCS_FAULT_GET(val64)) {
> +			if (link->blattr.options & SBL_DISABLE_PML_RECOVERY ||
> +			    sbl_debug_option(sbl, port_num, SBL_DEBUG_REMOTE_FAULT_RECOVERY)) {
> +				sbl_dev_dbg(sbl->dev, "%d: pml hdlr - remote fault", port_num);
> +				down_origin = SBL_LINK_DOWN_ORIGIN_REMOTE_FAULT;
> +			} else {
> +				sbl_dev_dbg(sbl->dev, "%d: pml hdlr - remote fault - ignored", port_num);
> +			}
> +		} else {
> +			sbl_dev_dbg(sbl->dev, "%d: pml hdlr - link down", port_num);
> +			down_origin = SBL_LINK_DOWN_ORIGIN_LINK_DOWN;
> +		}
> +	}
> +
> +	if (down_origin != SBL_LINK_DOWN_ORIGIN_UNKNOWN) {
> +		if (!(link->blattr.options & SBL_DISABLE_PML_RECOVERY) && !link->is_degraded) {
> +			if (sbl_pml_recovery_ignore_down_origin_fault(down_origin)) {
> +				sbl_dev_dbg(sbl->dev, "%d: PML recovery, pml hdlr fault(%d) is ignored", port_num, down_origin);
> +				sbl_pml_recovery_monitor(sbl, port_num, down_origin);
> +
> +			} else
> +				sbl_pml_link_down_async_alert(sbl, port_num, down_origin);
> +		} else {
> +			sbl_dev_info(sbl->dev, "%d: PML recovery is disabled", port_num);
> +			sbl_pml_link_down_async_alert(sbl, port_num, down_origin);
> +		}
> +	}

[Severity: Medium]
In sbl_pml_hdlr(), when a TX or RX degrade failure occurs, down_origin is set
to SBL_LINK_DOWN_ORIGIN_DEGRADE_FAILURE. In
sbl_pml_recovery_ignore_down_origin_fault(), that is the only origin that
returns false so that the fault bypasses sbl_pml_recovery_monitor() and
immediately calls sbl_pml_link_down_async_alert().

However, if SBL_PML_ERR_FLG_PCS_HI_SER, SBL_PML_ERR_FLG_LLR_REPLAY_AT_MAX, or
SBL_PML_ERR_FLG_PCS_LINK_DOWN is raised in raised_flgs at the same time, the
subsequent checks unconditionally overwrite down_origin with a recoverable
origin (such as SBL_LINK_DOWN_ORIGIN_ALIGN or SBL_LINK_DOWN_ORIGIN_LINK_DOWN).

Won't that cause sbl_pml_recovery_ignore_down_origin_fault(down_origin) to
return true and enter sbl_pml_recovery_monitor() instead of immediately
sending the link-down alert with SBL_LINK_DOWN_ORIGIN_DEGRADE_FAILURE?

[ ... ]

> diff --git a/drivers/net/ethernet/hpe/sbl/sbl_pml_llr.c b/drivers/net/ethernet/hpe/sbl/sbl_pml_llr.c
> new file mode 100644
> index 0000000000000..a190f6890b4ef
> --- /dev/null
> +++ b/drivers/net/ethernet/hpe/sbl/sbl_pml_llr.c
> @@ -0,0 +1,736 @@

[ ... ]

> +static int sbl_pml_llr_measure_loop_time_ns(struct sbl_inst *sbl, int port_num,
> +					    u64 *llr_loop_time)
> +{
> +	struct sbl_link *link;
> +	u32              base;
> +	u64              min_loop_time;
> +	u64              max_loop_time;
> +	int              x;
> +	u64              val64;
> +	u64              time64;
> +	int              err;
> +
> +	sbl_link_info_set(sbl, port_num, SBL_LINK_INFO_LLR_MEASURE);
> +
> +	link = sbl->link + port_num;
> +	base = SBL_PML_BASE(port_num);
> +
> +	min_loop_time  = SBL_PML_LLR_MIN_LOOP_TIME;
> +	max_loop_time  = SBL_PML_LLR_MAX_LOOP_TIME;
> +	*llr_loop_time = SBL_PML_LLR_MAX_LOOP_TIME;
> +	time64         = 0ULL;
> +
> +	sbl_dev_dbg(sbl->dev, "%d: LLR measure loop time (len = %s)(media = %s)",
> +		    port_num, sbl_link_len_str(link->mattr.len),
> +		    sbl_link_media_str(link->mattr.media));
> +
> +	/* calc loop time bounds */
> +	if (link->mattr.len != SBL_LINK_LEN_INVALID) {
> +		err = sbl_media_calc_loop_time(sbl, port_num, &min_loop_time);
> +		if (err) {
> +			sbl_dev_err(sbl->dev, "%d: LLR measure calc loop time failed [%d]", port_num, err);
> +			goto out;
> +		}
> +		/* arbitrary range for max */
> +		max_loop_time = min_loop_time + 100 /* ns */;
> +	}
> +	sbl_dev_dbg(sbl->dev, "%d: LLR measure loop time bounds: min = %lldns, max = %lldns",
> +		    port_num, min_loop_time, max_loop_time);
> +
> +	/* measure loop time */
> +	for (x = 0; x < SBL_PML_LLR_TIMING_LOOPS; ++x) {
> +
> +		if (sbl_start_timeout(sbl, port_num)) {
> +			sbl_dev_err(sbl->dev, "%d: LLR measure loop time timeout", port_num);
> +			err = -ETIMEDOUT;
> +			goto out;
> +		}
> +
> +		if (sbl_base_link_start_cancelled(sbl, port_num)) {
> +			sbl_dev_err(sbl->dev, "%d: LLR measure loop time cancelled", port_num);
> +			err = -ECANCELED;
> +			goto out;
> +		}
> +
> +		/* start a measurement */
> +		sbl_write64(sbl, base | SBL_PML_STS_LLR_LOOP_TIME_OFFSET, 0ULL);
> +		sbl_read64(sbl, base | SBL_PML_STS_LLR_LOOP_TIME_OFFSET);  /* flush */
> +		/* give the measurement time to complete */
> +		udelay(SBL_PML_LLR_TIMING_MEASURE_DELAY);
> +		/* read the measurement */
> +		val64 = sbl_read64(sbl, base | SBL_PML_STS_LLR_LOOP_TIME_OFFSET);
> +
> +		time64 = SBL_PML_STS_LLR_LOOP_TIME_LOOP_TIME_GET(val64); /* ns */
> +		sbl_dev_dbg(sbl->dev, "%d: LLR measure time = %lldns (%d)", port_num, time64, x);
> +
> +		/* if we didn't get a valid time, then wait to retry */
> +		if (!time64)
> +			msleep(SBL_PML_LLR_TIMING_RETRY_DELAY);
> +
> +		/* if we did get a valid time, then check it here */
> +		else if ((time64 >= min_loop_time) && (time64 <= max_loop_time)) {
> +			/* save the fastest time here */
> +			if (time64 < *llr_loop_time)
> +				*llr_loop_time = time64;
> +		}
> +	}
> +
> +	/* check result here */
> +	if (*llr_loop_time == SBL_PML_LLR_MAX_LOOP_TIME) {
> +		if (sbl_debug_option(sbl, port_num, SBL_DEBUG_ALLOW_LOOP_TIME_FAIL)) {
> +			sbl_dev_err(sbl->dev, "%d: LLR measure loop time failed (min = %lld, max = %lld, last = %lld)",
> +				    port_num, min_loop_time, max_loop_time, time64);
> +			err = -ENODATA;
> +			goto out;
> +		}
> +		/* set to max calculated loop time */
> +		*llr_loop_time = max_loop_time;
> +	}
> +
> +	sbl_dev_dbg(sbl->dev, "%d: LLR measure loop time = %lldns", port_num, *llr_loop_time);
> +	err = 0;
> +
> +out:
> +	sbl_link_info_clear(sbl, port_num, SBL_LINK_INFO_LLR_MEASURE);
> +	return err;
> +}

[Severity: Medium]
In sbl_pml_llr_measure_loop_time_ns(), *llr_loop_time (which points to
link->llr_loop_time in sbl_pml_llr_start()) is initialized to
SBL_PML_LLR_MAX_LOOP_TIME at entry.

If sbl_media_calc_loop_time() fails, or the measurement loop times out, is
cancelled, or fails with SBL_DEBUG_ALLOW_LOOP_TIME_FAIL set, the function
jumps to out and returns a negative error code while leaving
link->llr_loop_time non-zero.

Since sbl_pml_llr_start() only calls sbl_pml_llr_measure_loop_time_ns() when
!link->llr_loop_time, won't a subsequent call to sbl_pml_llr_start() skip
measuring the loop time and program the LLR replay timer and capacity using
that stale value?

[ ... ]

> +/* get LLR mode */
> +static int sbl_pml_llr_mode_get(struct sbl_inst *sbl, int port_num, u32 *llr_mode)
> +{
> +	struct sbl_link *link = sbl->link + port_num;
> +	int               err = -1;
> +
> +	sbl_dev_dbg(sbl->dev, "%d: LLR mode get (%d)", port_num, link->blattr.llr_mode);
> +
> +	switch (link->blattr.llr_mode) {

[ ... ]

> +	case SBL_LLR_MODE_AUTO:
> +		/* check fabric link here */
> +		if (link->blattr.options & SBL_OPT_FABRIC_LINK) {
> +			sbl_dev_dbg(sbl->dev, "%d: LLR mode on fabric", port_num);
> +			goto out_on;
> +		}
> +		/* check ether link here */
> +		if (!(link->blattr.options & SBL_OPT_ENABLE_ETHER_LLR))
> +			goto out_off;
> +		/* check AN options here */
> +		if (link->an_options & AN_OPT_LLR) {
> +			if (link->an_options & AN_OPT_ETHER_LLR) {
> +				if (link->an_options & AN_OPT_HPC_WITH_LLR) {
> +					link->llr_options |= SBL_PML_LLR_OPTION_HPC_WIH_LLR;
> +					link->ifg_config = SBL_IFG_CONFIG_HPC;
> +					sbl_dev_dbg(sbl->dev, "%d: LLR mode on with HPC from AN", port_num);
> +					goto out_on;
> +				}
> +				sbl_dev_dbg(sbl->dev, "%d: LLR mode on without HPC from AN", port_num);
> +				goto out_on;
> +			}
> +			goto out_off;
> +		}
> +		/* do loop detect here */
> +		err = sbl_pml_llr_detect(sbl, port_num, &(link->llr_mode));
> +		switch (err) {
> +		case 0:
> +			if (link->llr_mode == SBL_LLR_MODE_OFF)
> +				goto out_off;
> +			if (link->blattr.options & SBL_OPT_ENABLE_IFG_HPC_WITH_LLR) {
> +				link->llr_options |= SBL_PML_LLR_OPTION_HPC_WIH_LLR;
> +				link->ifg_config = SBL_IFG_CONFIG_HPC;
> +				sbl_dev_dbg(sbl->dev, "%d: LLR mode on with HPC from LOOP", port_num);
> +				goto out_on;
> +			}
> +			sbl_dev_dbg(sbl->dev, "%d: LLR mode on without HPC from LOOP", port_num);
> +			goto out_on;

[ ... ]

> +/* configure */
> +void sbl_pml_llr_config(struct sbl_inst *sbl, int port_num)
> +{
> +	struct sbl_link *link = sbl->link + port_num;
> +	u32 base = SBL_PML_BASE(port_num);
> +	u64 val64;
> +
> +	sbl_dev_dbg(sbl->dev, "%d: LLR config", port_num);
> +
> +	/* init to original requested */
> +	link->llr_mode = link->blattr.llr_mode;

[Severity: Medium]
Should sbl_pml_llr_config() (or sbl_pml_llr_stop()) also reset
link->llr_options to 0?

In sbl_pml_llr_mode_get(), SBL_PML_LLR_OPTION_HPC_WIH_LLR is ORed into
link->llr_options when AN_OPT_HPC_WITH_LLR or SBL_OPT_ENABLE_IFG_HPC_WITH_LLR
is active, and sbl_pml_llr_start() checks that bit to call
sbl_pml_mac_hpc_set(). Because link->llr_options is never cleared when the
link is stopped or reconfigured, won't a subsequent link start without HPC
still see SBL_PML_LLR_OPTION_HPC_WIH_LLR set and overwrite TX MAC IFG with
HPC mode?

[ ... ]

> +void sbl_pml_llr_disable(struct sbl_inst *sbl, int port_num)
> +{
> +	u32 base = SBL_PML_BASE(port_num);
> +	u64 val64;
> +
> +	sbl_dev_dbg(sbl->dev, "%d: LLR disable", port_num);
> +
> +	val64 = sbl_read64(sbl, base | SBL_PML_CFG_LLR_OFFSET);
> +	val64 = SBL_PML_CFG_LLR_LLR_MODE_UPDATE(val64, 0ULL); /* LLR OFF */
> +	sbl_write64(sbl, base | SBL_PML_CFG_LLR_OFFSET, val64);
> +	sbl_read64(sbl, base | SBL_PML_CFG_LLR_OFFSET);  /* flush */
> +
> +	sbl_link_info_set(sbl, port_num, SBL_LINK_INFO_LLR_DISABLED);
> +	sbl_link_info_clear(sbl, port_num, SBL_LINK_INFO_LLR_RUN);
> +}
> +EXPORT_SYMBOL(sbl_pml_llr_disable);
> +
> +/**
> + * sbl_pml_llr_enable() - Enable pml llr
> + * @sbl: A slingshot base link device instance
> + * @port_num: port number
> + *
> + * Sets the LLR mode based on the link configuration to
> + * enable the LLR feature.
> + */
> +void sbl_pml_llr_enable(struct sbl_inst *sbl, int port_num)
> +{
> +	struct sbl_link *link = sbl->link + port_num;
> +
> +	sbl_dev_dbg(sbl->dev, "%d: LLR enable", port_num);
> +
> +	sbl_pml_llr_mode_set(sbl, port_num, link->llr_mode);
> +}
> +EXPORT_SYMBOL(sbl_pml_llr_enable);

[Severity: Medium]
sbl_pml_llr_disable() sets SBL_LINK_INFO_LLR_DISABLED and clears
SBL_LINK_INFO_LLR_RUN in link->link_info, but sbl_pml_llr_enable() (and
sbl_pml_llr_check_is_ready()) does not update link->link_info when LLR is
re-enabled. Should sbl_pml_llr_enable() clear SBL_LINK_INFO_LLR_DISABLED and
restore SBL_LINK_INFO_LLR_RUN when link->llr_mode != SBL_LLR_MODE_OFF?

[ ... ]

> diff --git a/drivers/net/ethernet/hpe/sbl/sbl_pml_pcs.c b/drivers/net/ethernet/hpe/sbl/sbl_pml_pcs.c
> new file mode 100644
> index 0000000000000..81484e1773943
> --- /dev/null
> +++ b/drivers/net/ethernet/hpe/sbl/sbl_pml_pcs.c
> @@ -0,0 +1,1085 @@

[ ... ]

> +int  sbl_pml_pcs_am_start(struct sbl_inst *sbl, int port_num)
> +{
> +	struct sbl_link *link;
> +	int err;
> +
> +	err = sbl_validate_instance(sbl);
> +	if (err)
> +		return err;
> +
> +	err = sbl_validate_port_num(sbl, port_num);
> +	if (err)
> +		return err;
> +
> +	link = sbl->link + port_num;
> +
> +	if (!(link->blstate & (SBL_BASE_LINK_STATUS_STARTING | SBL_BASE_LINK_STATUS_STARTING))) {
> +		sbl_dev_err(sbl->dev, "pcs_am_start %d: wrong state (%s)\n", port_num,
> +			    sbl_link_state_str(link->blstate));
> +		return -ENAVAIL;
> +	}

[Severity: Low]
In sbl_pml_pcs_am_start(), SBL_BASE_LINK_STATUS_STARTING is bitwise-ORed with
itself when checking link->blstate. Was another SBL_BASE_LINK_STATUS_* flag
intended here, or should the duplicate operand be removed?

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