Re: [PATCH 12/23] net: hpe: sl: add the control layer

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 24 potential issue(s) to consider:
- [Critical] net: hpe: sl: use-after-free on del_complete and racy object lookup/delete
- [Critical] net: hpe: sl: UAF on ctrl_link from re-armed FEC monitor timer
- [High] net: hpe: sl: broken __free(sl_ctrl_link_put) conversion and dead checks
- [High] net: hpe: sl: partial writes corrupt byte-mode notification kfifo stream
- [High] net: hpe: sl: notif kfifo freed before unpublishing lgrp in sl_ctrl_lgrp_release()
- [High] net: hpe: sl: broken link state transitions in sl_ctrl_link_reset() and down
- [High] net: hpe: sl: recursive config_lock deadlock in lgrp trace enable setters
- [High] net: hpe: sl: core_link reference leak across loop in link_config_check()
- [High] net: hpe: sl: uninitialized down_complete in sl_ctrl_link_new()
- [High] net: hpe: sl: sl_ctrl_link_reset_cmd() fails to cancel starting link
- [High] net: hpe: sl: sl_ctrl_link_down() wedges link state on command failure
- [High] net: hpe: sl: divide-by-zero and HZ unit bug in sl_ctrl_link_fec_ber_calc()
- [High] net: hpe: sl: LLR start retry in sl_ctrl_llr_start_callback() always fails
- [Medium] net: hpe: sl: uninitialized up_count read in sl_ctrl_link_up_callback()
- [Medium] net: hpe: sl: homebrew list_state synchronization in sl_ctrl_lgrp_notif.c
- [Medium] net: hpe: sl: wrong lock in link config getters and unlocked fec_info copy
- [Medium] net: hpe: sl: double LINK_UP_CANCEL_CMD count in sl_ctrl_link_release_work()
- [Medium] net: hpe: sl: uninitialized read and lock bug in sl_ctrl_link_fec_data_check()
- [Medium] net: hpe: sl: uninitialized link_caps read in sl_ctrl_link_fec_limit_calc()
- [Medium] net: hpe: sl: uninitialized stack variables used on getter failure
- [Medium] net: hpe: sl: unlocked read of fec_data.info in sl_ctrl_link_fec_data_check()
- [Medium] net: hpe: sl: wrong imap passed in sl_ctrl_llr_start_callback() error path
- [Medium] net: hpe: sl: sl_ctrl_mac_reset() checks tx_rtn && rx_rtn instead of ||
- [Medium] net: hpe: sl: missing FEC down cause counters in sl_ctrl_link_fec_data_check()
--

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

net: hpe: sl: add the control layer

This commit adds the SL control layer to validate and serialize requests from
the NIC driver and sysfs onto the core state machines and deliver link group
notifications.

> diff --git a/drivers/net/ethernet/hpe/sl/ctrl/data/sl_ctrl_data_lgrp.c b/drivers/net/ethernet/hpe/sl/ctrl/data/sl_ctrl_data_lgrp.c
> new file mode 100644
> index 0000000000000..279e41770f485
> --- /dev/null
> +++ b/drivers/net/ethernet/hpe/sl/ctrl/data/sl_ctrl_data_lgrp.c
> @@ -0,0 +1,164 @@
> +// SPDX-License-Identifier: GPL-2.0
> +/* Copyright 2025-2026 Hewlett Packard Enterprise Development LP */
> +
> +#include "base/sl_ctrl_log.h"
> +#include "sl_ctrl_ldev.h"
> +#include "sl_media_lgrp.h"
> +#include "sl_core_lgrp.h"
> +
> +#include "data/sl_core_data_lgrp.h"
> +#include "data/sl_media_data_lgrp.h"
> +
> +#include "sl_ctrl_data_lgrp.h"
> +
> +#define LOG_NAME SL_CTRL_LGRP_LOG_NAME
> +
> +int sl_ctrl_data_lgrp_err_trace_enable_set(struct sl_ctrl_lgrp *ctrl_lgrp, bool err_trace_enable)
> +{
> +	struct sl_core_lgrp *held_lgrp_1 __free(sl_core_lgrp_put) = NULL;
> +	int rtn;
> +
> +	sl_ctrl_log_dbg(ctrl_lgrp, LOG_NAME, "set (err_trace_enable = %s)", err_trace_enable ? "true" : "false");
> +
> +	held_lgrp_1 = sl_core_lgrp_get(ctrl_lgrp->ctrl_ldev->num, ctrl_lgrp->num);
> +	if (!held_lgrp_1)
> +		return -ENODEV;
> +	spin_lock(&ctrl_lgrp->config_lock);
> +	rtn = sl_core_data_lgrp_err_trace_enable_set(held_lgrp_1,
> +						     err_trace_enable);
> +	if (rtn) {
> +		sl_ctrl_log_err_trace(ctrl_lgrp, LOG_NAME,
> +				      "core trace enable set failed [%d]", rtn);
> +		spin_unlock(&ctrl_lgrp->config_lock);
> +		return rtn;
> +	}

[Severity: High]
Does calling sl_ctrl_log_err_trace() while holding ctrl_lgrp->config_lock
cause a recursive spinlock deadlock on the same CPU?

In sl_ctrl_data_lgrp_err_trace_enable_set() and
sl_ctrl_data_lgrp_warn_trace_set(), if
sl_core_data_lgrp_*_trace_enable_set() or
sl_media_data_lgrp_*_trace_enable_set() returns an error,
sl_ctrl_log_err_trace(ctrl_lgrp, ...) is called before releasing
ctrl_lgrp->config_lock.

That macro calls sl_log_err_trace() ->
sl_ctrl_data_lgrp_is_err_trace_enabled(ctrl_lgrp, ...), which also acquires
ctrl_lgrp->config_lock:

drivers/net/ethernet/hpe/sl/ctrl/data/sl_ctrl_data_lgrp_trace.c:
sl_ctrl_data_lgrp_is_err_trace_enabled() {
	spin_lock(&ctrl_lgrp->config_lock);
	*err_trace_enable = ctrl_lgrp->err_trace_enable;
	spin_unlock(&ctrl_lgrp->config_lock);
    ...
}

Should ctrl_lgrp->config_lock be unlocked before calling
sl_ctrl_log_err_trace()?

> diff --git a/drivers/net/ethernet/hpe/sl/ctrl/data/sl_ctrl_data_link.c b/drivers/net/ethernet/hpe/sl/ctrl/data/sl_ctrl_data_link.c
> new file mode 100644
> index 0000000000000..dfb1b820d5ef7
> --- /dev/null
> +++ b/drivers/net/ethernet/hpe/sl/ctrl/data/sl_ctrl_data_link.c

[ ... ]

> +int sl_ctrl_data_link_fec_up_settle_wait_ms_get(struct sl_ctrl_link *ctrl_link, u32 *fec_up_settle_wait_ms)
> +{
> +	spin_lock(&ctrl_link->data_lock);
> +	*fec_up_settle_wait_ms = ctrl_link->config.fec_up_settle_wait_ms;
> +	spin_unlock(&ctrl_link->data_lock);
> +
> +	sl_ctrl_log_dbg(ctrl_link, LOG_NAME,
> +			"get (fec_up_settle_wait_ms = %d)", *fec_up_settle_wait_ms);
> +
> +	return 0;
> +}

[Severity: Medium]
Should the ctrl_link->config getters in sl_ctrl_data_link.c be taking
ctrl_link->config_lock instead of ctrl_link->data_lock?

In sl_ctrl_link_config_set_cmd(), sl_ctrl_link_config_get(), and
sl_ctrl_link_up_callback(), ctrl_link->config is protected by
ctrl_link->config_lock. However, all nine config getters in
sl_ctrl_data_link.c (sl_ctrl_data_link_fec_up_settle_wait_ms_get(),
sl_ctrl_data_link_fec_up_check_wait_ms_get(),
sl_ctrl_data_link_fec_up_ucw_limit_get(),
sl_ctrl_data_link_fec_up_ccw_limit_get(),
sl_ctrl_data_link_up_timeout_ms_get(),
sl_ctrl_data_link_up_tries_max_get(),
sl_ctrl_data_link_config_options_get(),
sl_ctrl_data_link_pause_map_get(), and
sl_ctrl_data_link_hpe_map_get()) acquire ctrl_link->data_lock instead.
(In addition, sl_ctrl_link_config_set_cmd() reads
ctrl_link->config.link_up_tries_max after dropping ctrl_link->config_lock.)

> diff --git a/drivers/net/ethernet/hpe/sl/ctrl/sl_ctrl_ldev.c b/drivers/net/ethernet/hpe/sl/ctrl/sl_ctrl_ldev.c
> new file mode 100644
> index 0000000000000..0295dd902d6de
> --- /dev/null
> +++ b/drivers/net/ethernet/hpe/sl/ctrl/sl_ctrl_ldev.c

[ ... ]

> +	spin_lock(&ctrl_ldevs_lock);
> +	ctrl_ldevs[ldev_num] = NULL;
> +	spin_unlock(&ctrl_ldevs_lock);
> +
> +	complete_all(&ctrl_ldev->del_complete);
> +
> +	kfree(ctrl_ldev);
> +	return;
> +
> +retry:
> +	kref_init(&ctrl_ldev->ref_cnt);
> +	complete_all(&ctrl_ldev->del_complete);
> +}
> +
> +int sl_ctrl_ldev_put(struct sl_ctrl_ldev *ctrl_ldev)
> +{
> +	return kref_put(&ctrl_ldev->ref_cnt, sl_ctrl_ldev_release);
> +}
> +
> +int sl_ctrl_ldev_del(u8 ldev_num)
> +{
> +	struct sl_ctrl_ldev *ctrl_ldev;
> +	unsigned long       timeleft;
> +
> +	ctrl_ldev = sl_ctrl_ldev_get(ldev_num);
> +	if (!ctrl_ldev) {
> +		sl_ctrl_log_err_trace(NULL, LOG_NAME,
> +				      "del not found (ldev_num = %u)", ldev_num);
> +		return -EBADRQC;
> +	}
> +
> +	sl_ctrl_log_dbg(ctrl_ldev, LOG_NAME, "del (ldev = 0x%p)", ctrl_ldev);
> +
> +	/*
> +	 * Release cannot run before the put below, so rearming a completion
> +	 * left over from an earlier failed del is safe.
> +	 */
> +	reinit_completion(&ctrl_ldev->del_complete);
> +
> +	if (!sl_ctrl_ldev_put(ctrl_ldev)) {
> +		timeleft = wait_for_completion_timeout(&ctrl_ldev->del_complete,
> +						       msecs_to_jiffies(SL_CTRL_LDEV_DEL_TIMEOUT_MS));
> +
> +		sl_ctrl_log_dbg(NULL, LOG_NAME,
> +				"del completion_timeout (timeleft = %lums)", timeleft);
> +
> +		if (timeleft == 0) {
> +			sl_ctrl_log_err(NULL, LOG_NAME,
> +					"del timed out (ctrl_ldev = 0x%p)", ctrl_ldev);
> +			return -ETIMEDOUT;
> +		}
> +	}

[Severity: Critical]
Can this trigger a use-after-free on del_complete and on objects returned by
sl_ctrl_ldev_get(), sl_ctrl_lgrp_get(), sl_ctrl_llr_get(), and
sl_ctrl_mac_get()?

First, in sl_ctrl_ldev_del(), sl_ctrl_lgrp_del(), and sl_ctrl_llr_del(), if
another thread holds a reference on ref_cnt (for example in
sl_ctrl_ldev_serdes_init(), which can sleep for up to 3000ms while
SL_CTRL_LDEV_DEL_TIMEOUT_MS is 2000ms), sl_ctrl_*_put() returns 0 and the
caller waits in wait_for_completion_timeout(&obj->del_complete, ...). When
the other thread drops the final reference, sl_ctrl_ldev_release(),
sl_ctrl_lgrp_release(), or sl_ctrl_llr_release() calls
complete_all(&obj->del_complete) and immediately calls kfree(obj) without a
separate memory reference (unlike sl_ctrl_link and sl_ctrl_mac, which use
mem_ref), freeing del_complete while wait_for_completion_timeout() is still
accessing it or after it times out.

Second, sl_ctrl_ldev_get(), sl_ctrl_lgrp_get(), sl_ctrl_llr_get(), and
sl_ctrl_mac_get() drop the global table spinlock and return a raw pointer
without acquiring a reference or checking a delete_requested flag under the
lock. Callers (including sl_ctrl_mac_del() calling
kref_get(&ctrl_mac->mem_ref) and callers invoking kref_get_unless_zero())
can dereference a pointer that was concurrently deleted and freed on another
CPU, and concurrent or retried del() calls can invoke reinit_completion()
and kref_put() a second time without owning a reference.

Could ldev, lgrp, llr, and mac use the same delete_requested + mem_ref +
kref_get_unless_zero()-under-table-lock pattern as sl_ctrl_link?

> diff --git a/drivers/net/ethernet/hpe/sl/ctrl/sl_ctrl_lgrp.c b/drivers/net/ethernet/hpe/sl/ctrl/sl_ctrl_lgrp.c
> new file mode 100644
> index 0000000000000..1e7240325fcc1
> --- /dev/null
> +++ b/drivers/net/ethernet/hpe/sl/ctrl/sl_ctrl_lgrp.c

[ ... ]

> +	cancel_work_sync(&ctrl_lgrp->notif_work);
> +	kfifo_free(&ctrl_lgrp->ctrl_notif.fifo);
> +
> +	spin_lock(&ctrl_lgrps_lock);
> +	ctrl_lgrps[ldev_num][lgrp_num] = NULL;
> +	spin_unlock(&ctrl_lgrps_lock);

[Severity: High]
In sl_ctrl_lgrp_release(), ctrl_lgrp->notif_work is canceled and
ctrl_lgrp->ctrl_notif.fifo is freed before ctrl_lgrps[ldev_num][lgrp_num] is
cleared under ctrl_lgrps_lock. If a concurrent caller looks up ctrl_lgrp via
sl_ctrl_lgrp_get() and calls sl_ctrl_lgrp_notif_enqueue() before the pointer
is unpublished, can it write to the freed kfifo and re-queue notif_work right
before kfree(ctrl_lgrp)?

[ ... ]

> +	rtn = sl_core_lgrp_config_set(ldev_num, lgrp_num, lgrp_config);
> +	if (rtn) {
> +		sl_ctrl_log_err_trace(ctrl_lgrp, LOG_NAME, "core_lgrp_config_set failed [%d]", rtn);
> +		goto out;
> +	}

[Severity: High]
When sl_ctrl_lgrp_config_set() calls sl_core_lgrp_config_set(), it invokes
sl_core_data_lgrp_link_config_check():

drivers/net/ethernet/hpe/sl/core/data/sl_core_data_lgrp.c:
sl_core_data_lgrp_link_config_check() {
	struct sl_core_link *core_link __free(sl_core_link_put) = NULL;
	...
	for (link_num = 0; link_num < SL_ASIC_MAX_LINKS; ++link_num) {
		core_link = sl_core_link_get(core_lgrp->core_ldev->num, core_lgrp->num, link_num);
		if (!core_link)
			continue;
		...
	}
}

Because core_link is declared with __free(sl_core_link_put) outside the
for-loop, doesn't each iteration overwrite core_link without releasing the
reference from the previous iteration, leaking core_link references and
causing subsequent sl_core_link_del() / sl_ctrl_link_del() calls to fail with
-EBUSY?

> diff --git a/drivers/net/ethernet/hpe/sl/ctrl/sl_ctrl_lgrp_notif.c b/drivers/net/ethernet/hpe/sl/ctrl/sl_ctrl_lgrp_notif.c
> new file mode 100644
> index 0000000000000..298b8ab0306cf
> --- /dev/null
> +++ b/drivers/net/ethernet/hpe/sl/ctrl/sl_ctrl_lgrp_notif.c

[ ... ]

> +	counter = 0;
> +	while (sl_ctrl_lgrp_notif_list_state_get(ctrl_lgrp) == SL_CTRL_LGRP_NOTIF_LIST_STATE_SENDING) {
> +		msleep(20);
> +		counter++;
> +		if (counter >= 10) {
> +			sl_ctrl_log_err(ctrl_lgrp, LOG_NAME, "registration timed out");
> +			rtn = -ETIMEDOUT;
> +			goto out;
> +		}
> +	}

[Severity: Medium]
In sl_ctrl_lgrp_notif_callback_reg(), sl_ctrl_lgrp_notif_callback_unreg(),
and sl_ctrl_lgrp_notif_work(), ctrl_lgrp->ctrl_notif.list_state is set to
SL_CTRL_LGRP_NOTIF_LIST_STATE_SENDING on entry to the work item and polled
with msleep(20) loops during registration and unregistration while
sl_ctrl_lgrp_notif_work() iterates reg_entry[] without holding a lock.

Could this custom list_state flag and sleep-polling loop be replaced with a
standard kernel synchronization primitive (such as a mutex or RCU/SRCU)?

[ ... ]

> +	spin_lock(&ctrl_lgrp->ctrl_notif.lock);
> +	if (kfifo_is_full(&ctrl_lgrp->ctrl_notif.fifo)) {
> +		spin_unlock(&ctrl_lgrp->ctrl_notif.lock);
> +		sl_ctrl_log_err(ctrl_lgrp, LOG_NAME, "notification fifo is full");
> +		return -ENOSPC;
> +	}
> +	spin_unlock(&ctrl_lgrp->ctrl_notif.lock);
> +
> +	if (info)
> +		notif_msg.info = *info;
> +	else
> +		memset(&notif_msg.info, 0, sizeof(notif_msg.info));
> +
> +	notif_msg.ldev_num = ctrl_lgrp->ctrl_ldev->num;
> +	notif_msg.lgrp_num = ctrl_lgrp->num;
> +	notif_msg.link_num = link_num;
> +	notif_msg.type     = type;
> +	notif_msg.info_map = info_map;
> +
> +	rtn = kfifo_in_spinlocked(&ctrl_lgrp->ctrl_notif.fifo, &notif_msg, sizeof(notif_msg),\n> +				  &ctrl_lgrp->ctrl_notif.lock);
> +	if (rtn != sizeof(notif_msg)) {
> +		sl_ctrl_log_err(ctrl_lgrp, LOG_NAME, "write to kfifo failed");
> +		return -EFAULT;
> +	}
> +
> +	/* Don't catch the return as it might already be running.  This is ok */
> +	queue_work(ctrl_lgrp->ctrl_ldev->notif_workq, &ctrl_lgrp->notif_work);
> +
> +	return 0;
> +}
> +
> +void sl_ctrl_lgrp_notif_work(struct work_struct *notif_work)
> +{
> +	struct sl_ctrl_lgrp      *ctrl_lgrp;
> +	struct sl_lgrp_notif_msg  notif_msg;
> +	u8                        reg_idx;
> +	int                       rtn;
> +	bool                      is_empty;
> +
> +	ctrl_lgrp = container_of(notif_work, struct sl_ctrl_lgrp, notif_work);
> +
> +	sl_ctrl_log_dbg(ctrl_lgrp, LOG_NAME, "notif work");
> +
> +	spin_lock(&ctrl_lgrp->ctrl_notif.lock);
> +	ctrl_lgrp->ctrl_notif.list_state = SL_CTRL_LGRP_NOTIF_LIST_STATE_SENDING;
> +	spin_unlock(&ctrl_lgrp->ctrl_notif.lock);
> +
> +	do {
> +		rtn = kfifo_out_spinlocked(&ctrl_lgrp->ctrl_notif.fifo,
> +					   &notif_msg, sizeof(notif_msg),
> +					   &ctrl_lgrp->ctrl_notif.lock);
> +		if (rtn != sizeof(notif_msg)) {
> +			sl_ctrl_log_err(ctrl_lgrp, LOG_NAME, "read from kfifo failed");
> +			goto out;
> +		}
> +		for (reg_idx = 0; reg_idx < SL_CTRL_LGRP_NOTIF_REG_COUNT; ++reg_idx) {
> +			if (notif_msg.type & ctrl_lgrp->ctrl_notif.reg_entry[reg_idx].types)
> +				(ctrl_lgrp->ctrl_notif.reg_entry[reg_idx].callback)
> +				(ctrl_lgrp->ctrl_notif.reg_entry[reg_idx].tag, &notif_msg);
> +		}
> +		spin_lock(&ctrl_lgrp->ctrl_notif.lock);
> +		is_empty = kfifo_is_empty(&ctrl_lgrp->ctrl_notif.fifo);
> +		spin_unlock(&ctrl_lgrp->ctrl_notif.lock);
> +	} while (!is_empty);

[Severity: High]
Can partial writes into ctrl_lgrp->ctrl_notif.fifo corrupt the notification
message stream?

In sl_ctrl_lgrp_notif.h, ctrl_lgrp->ctrl_notif.fifo is declared as a
byte-stream struct kfifo (element size 1), and kfifo_alloc() in
sl_ctrl_lgrp_new() rounds the byte size up to a power of two that is not a
multiple of sizeof(struct sl_lgrp_notif_msg). In
sl_ctrl_lgrp_notif_enqueue(), kfifo_is_full() only checks whether 0 unused
bytes remain (and drops ctrl_notif.lock before kfifo_in_spinlocked()). When
fewer than sizeof(notif_msg) unused bytes remain, kfifo_in_spinlocked()
writes a partial notif_msg into the FIFO and advances fifo.in, permanently
misaligning all subsequent sizeof(notif_msg) reads in
sl_ctrl_lgrp_notif_work().

Also, in sl_ctrl_lgrp_notif_work(), if sl_ctrl_lgrp_notif_enqueue() calls
queue_work() while sl_ctrl_lgrp_notif_work() is already running, the first
invocation's do-while loop drains the newly queued message, and the second
scheduled invocation enters the do-while loop on an empty FIFO and logs
"read from kfifo failed". (Note also that ctrl_notif.lock is acquired with
spin_lock_irqsave() inside kfifo_in_spinlocked()/kfifo_out_spinlocked() but
with plain spin_lock() elsewhere in this file.)

Would using a typedDECLARE_KFIFO_PTR(fifo, struct sl_lgrp_notif_msg) and a
while (kfifo_get(...)) loop under consistent locking avoid both issues?

> diff --git a/drivers/net/ethernet/hpe/sl/ctrl/sl_ctrl_link.c b/drivers/net/ethernet/hpe/sl/ctrl/sl_ctrl_link.c
> new file mode 100644
> index 0000000000000..f3a8246fcb670
> --- /dev/null
> +++ b/drivers/net/ethernet/hpe/sl/ctrl/sl_ctrl_link.c

[ ... ]

> +	kref_init(&ctrl_link->ref_cnt);
> +	kref_init(&ctrl_link->mem_ref);
> +	INIT_WORK(&ctrl_link->release_work, sl_ctrl_link_release_work);
> +	mutex_init(&ctrl_link->delete_lock);
> +	init_completion(&ctrl_link->del_complete);

[Severity: High]
Should ctrl_link->down_complete also be initialized via init_completion() in
sl_ctrl_link_new(), with sl_ctrl_link_up_cmd() calling reinit_completion()
instead of init_completion()?

Currently, down_complete is only initialized in sl_ctrl_link_up_cmd() after
sl_ctrl_link_up() has already set ctrl_link->state = SL_LINK_STATE_STARTING
and dropped ctrl_link->data_lock. If sl_ctrl_link_down(),
sl_ctrl_link_reset(), or sl_ctrl_link_release_work() runs in that window
before init_completion(&ctrl_link->down_complete) executes (or if a fault
callback completes down_complete before the first link-up),
wait_for_completion_timeout() or complete_all() will operate on an
uninitialized completion waitqueue, and subsequent link-ups will overwrite
down_complete.wait via init_completion().

[ ... ]

> +	spin_lock(&ctrl_link->data_lock);
> +	link_state = ctrl_link->state;
> +	switch (link_state) {
> +	case SL_LINK_STATE_STARTING:
> +		ctrl_link->is_canceled = true;
> +		spin_unlock(&ctrl_link->data_lock);
> +		SL_CTRL_LINK_COUNTER_INC(ctrl_link, LINK_UP_CANCEL_CMD);
> +		sl_ctrl_link_cancel_cmd(ctrl_link);
> +		sl_ctrl_link_down_wait(ctrl_link);
> +		break;

[Severity: Medium]
In sl_ctrl_link_release_work(), doesn't the SL_LINK_STATE_STARTING case
increment LINK_UP_CANCEL_CMD twice (once here and once inside
sl_ctrl_link_cancel_cmd()), and omit setting ctrl_link->state =
SL_LINK_STATE_STOPPING before dropping ctrl_link->data_lock?

> +	case SL_LINK_STATE_UP:
> +	case SL_LINK_STATE_UP_DOWN_REQ:
> +		ctrl_link->state = SL_LINK_STATE_STOPPING;
> +		sl_ctrl_log_dbg(ctrl_link, LOG_NAME, "release stopping");
> +		spin_unlock(&ctrl_link->data_lock);
> +		sl_ctrl_link_down_cmd(ctrl_link);
> +		sl_ctrl_link_down_wait(ctrl_link);
> +		break;
> +	case SL_LINK_STATE_STOPPING:
> +		sl_ctrl_log_dbg(ctrl_link, LOG_NAME, "release already stopping");
> +		spin_unlock(&ctrl_link->data_lock);
> +		sl_ctrl_link_down_wait(ctrl_link);
> +		break;
> +	case SL_LINK_STATE_DOWN:
> +		sl_ctrl_log_dbg(ctrl_link, LOG_NAME, "release already down");
> +		spin_unlock(&ctrl_link->data_lock);
> +		break;

[ ... ]

> +int sl_ctrl_link_config_set(u8 ldev_num, u8 lgrp_num, u8 link_num,
> +			    struct sl_link_config *link_config)
> +{
> +	int                  rtn;
> +	struct sl_ctrl_link *ctrl_link __free(sl_ctrl_link_put) = NULL;
> +	u32                  link_state;
> +
> +	ctrl_link = sl_ctrl_link_get(ldev_num, lgrp_num, link_num);
> +	if (!ctrl_link)
> +		return -ENODEV;
> +	if (!ctrl_link) {
> +		sl_ctrl_log_err(NULL, LOG_NAME,
> +				"config set NULL link (ldev_num = %u, lgrp_num = %u, link_num = %u)",
> +			ldev_num, lgrp_num, link_num);
> +		return -EBADRQC;
> +	}
> +
> +	if (!sl_ctrl_link_kref_get_unless_zero(ctrl_link)) {
> +		sl_ctrl_log_err(ctrl_link, LOG_NAME, "config set link ref unavailable (ctrl_link = 0x%p)",
> +				ctrl_link);
> +		return -EBADRQC;
> +	}

[Severity: High]
Across 16 functions in sl_ctrl_link.c (sl_ctrl_link_config_set(),
sl_ctrl_link_policy_set(), sl_ctrl_link_an_lp_caps_get(),
sl_ctrl_link_an_lp_caps_stop(), sl_ctrl_link_up(), sl_ctrl_link_down(),
sl_ctrl_link_reset(), sl_ctrl_link_up_clocks_get(),
sl_ctrl_link_up_count_get(), sl_ctrl_link_state_get_cmd(),
sl_ctrl_link_an_lp_caps_state_get(), sl_ctrl_link_an_fail_cause_get(), and
sl_ctrl_link_info_map_get()), sl_ctrl_link_fec.c
(sl_ctrl_link_fec_info_get() and sl_ctrl_link_fec_tail_get()), and
sl_ctrl_link_priv.c (sl_ctrl_link_fault_start_callback()), it looks like an
automated __free(sl_ctrl_link_put) conversion was partially applied on top of
existing manual reference counting:

1. An extra "if (!ctrl_link) return -ENODEV;" was inserted right before the
existing "if (!ctrl_link)" block, making the second block dead code and
changing the return values when the link does not exist. In particular, in
sl_ctrl_link_state_get_cmd() this returns -ENODEV instead of returning 0 with
*state = SL_LINK_STATE_INVALID, and in sl_ctrl_link_an_lp_caps_state_get()
it returns -ENODEV instead of -EINVAL.

2. sl_ctrl_link_get() already acquires a reference on ctrl_link->ref_cnt via
kref_get_unless_zero(), and __free(sl_ctrl_link_put) will release it at scope
exit, but these functions still call sl_ctrl_link_kref_get_unless_zero() to
take a second reference and manually call sl_ctrl_link_put() at the end or
via goto out. Because two references are held, the manual
"if (sl_ctrl_link_put(ctrl_link))" checks can never observe the final put,
and mixing __free() with goto cleanup (as well as declaring __free()
variables as NULL at the top of the function instead of at initialization,
including held_lgrp_1 and held_link_1) goes against kernel Cleanup API
conventions.

Should the duplicate NULL checks, extra sl_ctrl_link_kref_get_unless_zero()
calls, and manual sl_ctrl_link_put() / goto out paths be removed so __free()
manages the single reference returned by sl_ctrl_link_get()?

[ ... ]

> +	spin_lock(&ctrl_link->data_lock);
> +	link_state = ctrl_link->state;
> +	switch (link_state) {
> +	case SL_LINK_STATE_STARTING:
> +		ctrl_link->state = SL_LINK_STATE_STOPPING;
> +		ctrl_link->is_canceled = true;
> +		sl_ctrl_log_dbg(ctrl_link, LOG_NAME, "down - canceling");
> +		spin_unlock(&ctrl_link->data_lock);
> +		rtn = sl_ctrl_link_cancel_cmd(ctrl_link);
> +		if (rtn) {
> +			sl_ctrl_log_err_trace(ctrl_link, LOG_NAME, "cancel_cmd failed [%d]", rtn);
> +			goto out;
> +		}
> +
> +		rtn = 0;
> +		goto out;
> +	case SL_LINK_STATE_UP:
> +	case SL_LINK_STATE_UP_DOWN_REQ:
> +		ctrl_link->state = SL_LINK_STATE_STOPPING;
> +		sl_ctrl_log_dbg(ctrl_link, LOG_NAME, "down - stopping");
> +		spin_unlock(&ctrl_link->data_lock);
> +		rtn = sl_ctrl_link_down_cmd(ctrl_link);
> +		if (rtn) {
> +			sl_ctrl_log_err_trace(ctrl_link, LOG_NAME, "down_cmd failed [%d]", rtn);
> +			goto out;
> +		}

[Severity: High]
In sl_ctrl_link_down(), if sl_ctrl_link_cancel_cmd() or
sl_ctrl_link_down_cmd() fails, ctrl_link->state is left in
SL_LINK_STATE_STOPPING when jumping to out, and no down callback will run to
transition it to SL_LINK_STATE_DOWN. Should ctrl_link->state be restored on
error, similar to sl_ctrl_link_async_down()?

[ ... ]

> +static int sl_ctrl_link_reset_cmd(struct sl_ctrl_link *ctrl_link)
> +{
> +	int           rtn;
> +	u8            ldev_num;
> +	u8            lgrp_num;
> +	u8            link_num;
> +	unsigned long timeleft;

[ ... ]

> +	rtn = sl_core_link_down(ldev_num, lgrp_num, link_num, sl_ctrl_link_down_callback,
> +				ctrl_link, SL_LINK_DOWN_CAUSE_COMMAND_MAP);
> +	if (rtn)
> +		sl_ctrl_log_warn(ctrl_link, LOG_NAME, "core_link_down failed [%d]", rtn);
> +
> +	timeleft = wait_for_completion_timeout(&ctrl_link->down_complete,
> +					       msecs_to_jiffies(SL_CTRL_LINK_DOWN_WAIT_TIMEOUT_MS));
> +	if (timeleft == 0)
> +		sl_ctrl_log_err(ctrl_link, LOG_NAME,
> +				"completion_timeout (down_complete = 0x%p)", &ctrl_link->down_complete);
> +
> +	sl_ctrl_log_dbg(ctrl_link, LOG_NAME, "completion (timeleft = %lu)", timeleft);
> +
> +	sl_core_link_reset(ldev_num, lgrp_num, link_num);
> +
> +	return 0;
> +}
> +
> +int sl_ctrl_link_reset(u8 ldev_num, u8 lgrp_num, u8 link_num)
> +{

[ ... ]

> +	spin_lock(&ctrl_link->data_lock);
> +	link_state = ctrl_link->state;
> +	switch (link_state) {
> +	case SL_LINK_STATE_STARTING:
> +		ctrl_link->is_canceled = true;
> +		SL_CTRL_LINK_COUNTER_INC(ctrl_link, LINK_UP_CANCEL_CMD);
> +		fallthrough;
> +	case SL_LINK_STATE_UP:
> +	case SL_LINK_STATE_UP_DOWN_REQ:
> +		ctrl_link->state = SL_LINK_STATE_STOPPING;
> +		sl_ctrl_log_dbg(ctrl_link, LOG_NAME, "reset - stopping");
> +		spin_unlock(&ctrl_link->data_lock);
> +		rtn = sl_ctrl_link_reset_cmd(ctrl_link);

[Severity: High]
When sl_ctrl_link_reset() is called while ctrl_link->state is
SL_LINK_STATE_STARTING, it falls through to sl_ctrl_link_reset_cmd(), which
calls sl_core_link_down() instead of sl_core_link_cancel().

Because the core link state during bring-up is SL_CORE_LINK_STATE_GOING_UP or
SL_CORE_LINK_STATE_AN, sl_core_link_down() rejects the request with -EBADRQC
without registering sl_ctrl_link_down_callback. Doesn't this cause
wait_for_completion_timeout(&ctrl_link->down_complete, ...) to stall for the
full 2000ms timeout, pulse hardware reset via sl_core_link_reset() while core
bring-up timers and work items are still active, and leave ctrl_link->state
stuck in SL_LINK_STATE_STOPPING?

Should sl_ctrl_link_reset_cmd() call sl_core_link_cancel() when resetting from
SL_LINK_STATE_STARTING?

> diff --git a/drivers/net/ethernet/hpe/sl/ctrl/sl_ctrl_link_fec.c b/drivers/net/ethernet/hpe/sl/ctrl/sl_ctrl_link_fec.c
> new file mode 100644
> index 0000000000000..2c54795281e00
> --- /dev/null
> +++ b/drivers/net/ethernet/hpe/sl/ctrl/sl_ctrl_link_fec.c

[ ... ]

> +	/* B = BER
> +	 * n = [U/C]CW count
> +	 * g = Good CW count
> +	 * T = total CW = ccw_count + g
> +	 * t = time = (jiffies / HZ)
> +	 *
> +	 * B = (n/T) / t; B = (n * HZ) / (T * jiffies)
> +	 */
> +
> +	/* We use RS(544/514) with 10 bit symbols which is a total of 5440 bits per CW. */
> +	total = fec_info->ccw + (fec_info->gcw * 5440);
> +
> +	/* CCW count * CCW multiplier */
> +	ccw_rate = (fec_info->ccw * HZ * SL_CTRL_LINK_FEC_CCW_BER_MULT) / (total * fec_info->period_ms);

[Severity: High]
In sl_ctrl_link_fec_ber_calc(), what happens if fec_info->period_ms is 0 (for
example, if a caller passes period_ms == 0 or two samples have the same
jiffies timestamp)? Neither total == 0 nor the MAX_CCW_COUNT / MAX_UCW_COUNT
checks guard against fec_info->period_ms == 0, so dividing by
(total * fec_info->period_ms) here and in the ucw_rate calculation below will
trigger a divide-by-zero fault in the kernel.

Also, the comment notes "B = (n * HZ) / (T * jiffies)", but the denominator
uses fec_info->period_ms (milliseconds) while the numerator multiplies by HZ
instead of MSEC_PER_SEC (1000). Won't that produce an incorrect BER whenever
HZ != 1000?

> diff --git a/drivers/net/ethernet/hpe/sl/ctrl/sl_ctrl_link_fec_priv.c b/drivers/net/ethernet/hpe/sl/ctrl/sl_ctrl_link_fec_priv.c
> new file mode 100644
> index 0000000000000..a1f50d1a3b3c6
> --- /dev/null
> +++ b/drivers/net/ethernet/hpe/sl/ctrl/sl_ctrl_link_fec_priv.c

[ ... ]

> +	if (ctrl_link->fec_data.info.monitor.ucw_down_limit &&
> +	    (ctrl_link->fec_data.info.monitor.ucw_warn_limit > ctrl_link->fec_data.info.monitor.ucw_down_limit))
> +		sl_ctrl_log_warn(ctrl_link, LOG_NAME,
> +				 "CCW warning limit set greater than down limit (%d > %d)",
> +			ctrl_link->fec_data.info.monitor.ucw_warn_limit,
> +			ctrl_link->fec_data.info.monitor.ucw_down_limit);

[ ... ]

> +int sl_ctrl_link_fec_data_check(struct sl_ctrl_link *ctrl_link)
> +{
> +	int                rtn;
> +	s32                ccw_down_limit;
> +	s32                ccw_warn_limit;
> +	s32                ucw_down_limit;
> +	s32                ucw_warn_limit;
> +	struct sl_fec_info fec_info;
> +	bool               is_limit_crossed;
> +	time64_t           limit_crossed_time;
> +
> +	sl_ctrl_log_dbg(ctrl_link, LOG_NAME, "data check");
> +
> +	spin_lock(&ctrl_link->fec_data.lock);
> +	ccw_down_limit = ctrl_link->fec_data.info.monitor.ccw_down_limit;
> +	ccw_warn_limit = ctrl_link->fec_data.info.monitor.ccw_warn_limit;
> +	ucw_down_limit = ctrl_link->fec_data.info.monitor.ucw_down_limit;
> +	ucw_warn_limit = ctrl_link->fec_data.info.monitor.ucw_warn_limit;
> +	spin_unlock(&ctrl_link->fec_data.lock);
> +
> +	fec_info = ctrl_link->fec_data.info;

[Severity: Medium]
In sl_ctrl_link_fec_data_check(), fec_info = ctrl_link->fec_data.info is
copied right after releasing ctrl_link->fec_data.lock (and
sl_ctrl_link_fec_data_calc() similarly reads ctrl_link->fec_data.info and
ctrl_link->fec_data.tail after dropping ctrl_link->fec_data.lock). Should
fec_info be copied while holding ctrl_link->fec_data.lock so it cannot race
with concurrent updates in sl_ctrl_link_fec_mon_limits_calc()?

> +
> +	sl_ctrl_log_dbg(ctrl_link, LOG_NAME,
> +			"data check config (ccw_down = %d, ucw_down = %d, ccw_warn = %d, ucw_warn = %d)",
> +		ccw_down_limit, ucw_down_limit, ccw_warn_limit, ucw_warn_limit);
> +
> +	sl_ctrl_log_dbg(ctrl_link, LOG_NAME,
> +			"data check info (UCW = %llu, CCW = %llu, GCW = %llu, period = %ums)",
> +		fec_info.ucw, fec_info.ccw, fec_info.gcw, fec_info.period_ms);
> +
> +	if (sl_ctrl_link_fec_ucw_chance_limit_check(ctrl_link, ucw_down_limit, &fec_info)) {
> +		sl_ctrl_log_err(ctrl_link, LOG_NAME,
> +				"UCW exceeded down limit (UCW = %llu, CCW = %llu, ucw_chance = %u)",
> +			fec_info.ucw, fec_info.ccw, ctrl_link->fec_ucw_chance);
> +		ctrl_link->fec_ucw_chance = 0;
> +		rtn = sl_ctrl_link_async_down(ctrl_link, SL_LINK_DOWN_CAUSE_UCW_MAP, false);
> +		if (rtn) {
> +			sl_ctrl_log_err_trace(ctrl_link, LOG_NAME,
> +					      "data check UCW ctrl_link_async_down failed [%d]", rtn);
> +			return rtn;
> +		}
> +		return -EIO;
> +	}
> +
> +	if (sl_ctrl_link_fec_ccw_chance_limit_check(ctrl_link, ccw_down_limit, &fec_info)) {
> +		sl_ctrl_log_err(ctrl_link, LOG_NAME,
> +				"CCW exceeded down limit (UCW = %llu, CCW = %llu, ccw_chance = %u)",
> +			fec_info.ucw, fec_info.ccw, ctrl_link->fec_ccw_chance);
> +		ctrl_link->fec_ccw_chance = 0;
> +		rtn = sl_ctrl_link_async_down(ctrl_link, SL_LINK_DOWN_CAUSE_CCW_MAP, false);
> +		if (rtn) {
> +			sl_ctrl_log_err_trace(ctrl_link, LOG_NAME,
> +					      "data check CCW ctrl_link_async_down failed [%d]", rtn);
> +			return rtn;
> +		}
> +		return -EIO;
> +	}

[Severity: Medium]
When sl_ctrl_link_fec_ucw_chance_limit_check() or
sl_ctrl_link_fec_ccw_chance_limit_check() triggers a link down here, should
SL_CTRL_LINK_COUNTER_INC(ctrl_link, LINK_DOWN_UCW_CAUSE) and
SL_CTRL_LINK_COUNTER_INC(ctrl_link, LINK_DOWN_CCW_CAUSE) be incremented?
Those counters are initialized in sl_ctrl_link_counters_init() but never
incremented anywhere.

Also, in sl_ctrl_link_fec_mon_limits_calc() above, the warning message for
ucw_warn_limit > ucw_down_limit says "CCW warning limit set greater than down
limit" instead of "UCW".

> +
> +	if (SL_CTRL_LINK_FEC_UCW_LIMIT_CHECK(ucw_warn_limit, &fec_info)) {
> +		rtn = sl_core_link_ucw_warn_limit_crossed_get(ctrl_link->ctrl_lgrp->ctrl_ldev->num,
> +							      ctrl_link->ctrl_lgrp->num, ctrl_link->num, &is_limit_crossed, &limit_crossed_time);
> +		if (rtn)
> +			sl_core_log_warn_trace(ctrl_link, LOG_NAME, "ucw_warn_limit_crossed_get failed [%d]", rtn);
> +
> +		if (!is_limit_crossed) {

[Severity: Medium]
In sl_ctrl_link_fec_data_check(), is_limit_crossed is not initialized on the
stack. If sl_core_link_ucw_warn_limit_crossed_get() (or
sl_core_link_ccw_warn_limit_crossed_get() below) returns an error such as
-ENODEV, isn't is_limit_crossed read uninitialized in "if (!is_limit_crossed)"?

[ ... ]

> +	held_link_1 = sl_core_link_get(ctrl_link->ctrl_lgrp->ctrl_ldev->num,
> +				       ctrl_link->ctrl_lgrp->num, ctrl_link->num);
> +	if (!held_link_1)
> +		return;
> +	if (sl_core_link_is_pml_recovery_running(held_link_1)) {
> +		sl_ctrl_log_warn_trace(ctrl_link, LOG_NAME, "fec monitor skipped due to pml recovery running");
> +		goto start_mon;
> +	}
> +
> +	spin_lock(&ctrl_link->fec_mon_timer_lock);
> +	stop = ctrl_link->fec_mon_timer_stop;
> +	spin_unlock(&ctrl_link->fec_mon_timer_lock);
> +	if (stop) {
> +		sl_ctrl_log_dbg(ctrl_link, LOG_NAME, "monitor stopped");
> +		return;
> +	}
> +
> +	rtn = sl_ctrl_link_fec_data_get(ctrl_link->ctrl_lgrp->ctrl_ldev->num,
> +					ctrl_link->ctrl_lgrp->num, ctrl_link->num, &cw_cntrs, &lane_cntrs,
> +					&tail_cntrs);
> +	if (rtn) {
> +		sl_ctrl_log_err_trace(ctrl_link, LOG_NAME,
> +				      "core_link_fec_tail_cntrs_get failed [%d]", rtn);
> +		goto start_mon;
> +	}

[ ... ]

> +	spin_lock(&ctrl_link->fec_mon_timer_lock);
> +	stop = ctrl_link->fec_mon_timer_stop;
> +	spin_unlock(&ctrl_link->fec_mon_timer_lock);
> +	if (stop) {
> +		sl_ctrl_log_dbg(ctrl_link, LOG_NAME, "monitor stopped");
> +		return;
> +	}
> +
> +start_mon:
> +	mod_timer(&ctrl_link->fec_mon_timer, jiffies + msecs_to_jiffies(period));
> +	sl_ctrl_log_dbg(ctrl_link, LOG_NAME, "monitor started");
> +}
> +
> +void sl_ctrl_link_fec_mon_timer(struct timer_list *timer)
> +{
> +	struct sl_ctrl_link *ctrl_link;
> +
> +	ctrl_link = timer_container_of(ctrl_link, timer, fec_mon_timer);
> +
> +	sl_ctrl_log_dbg(ctrl_link, LOG_NAME, "monitor timer");
> +
> +	queue_work(ctrl_link->ctrl_lgrp->ctrl_ldev->workq, &ctrl_link->fec_mon_timer_work);
> +}
> +
> +void sl_ctrl_link_fec_mon_stop(struct sl_ctrl_link *ctrl_link)
> +{
> +	bool stop;
> +
> +	sl_ctrl_log_dbg(ctrl_link, LOG_NAME, "monitor stop");
> +
> +	spin_lock(&ctrl_link->fec_mon_timer_lock);
> +	stop = ctrl_link->fec_mon_timer_stop;
> +	ctrl_link->fec_mon_timer_stop = true;
> +	ctrl_link->fec_mon_state = SL_CTRL_LINK_FEC_MON_OFF;
> +	spin_unlock(&ctrl_link->fec_mon_timer_lock);
> +
> +	if (stop) {
> +		sl_ctrl_log_dbg(ctrl_link, LOG_NAME, "monitor already stopped");
> +		return;
> +	}
> +
> +	timer_delete_sync(&ctrl_link->fec_mon_timer);
> +}

[Severity: Critical]
Can fec_mon_timer be re-armed after sl_ctrl_link_fec_mon_stop() and fire
after ctrl_link is freed?

1. In sl_ctrl_link_fec_mon_timer_work(), both "goto start_mon" branches
(when sl_core_link_is_pml_recovery_running() is true and when
sl_ctrl_link_fec_data_get() returns an error such as -ENOLINK during link
down) jump directly to mod_timer(), bypassing the fec_mon_timer_stop check.
Even on the non-error path, fec_mon_timer_lock is dropped before calling
mod_timer().

2. Callers that stop FEC monitoring call sl_ctrl_link_fec_mon_stop() before
cancel_work_sync(&ctrl_link->fec_mon_timer_work). If
sl_ctrl_link_fec_mon_timer_work() was already running when
sl_ctrl_link_fec_mon_stop() set fec_mon_timer_stop = true and called
timer_delete_sync(), the running work item can then execute mod_timer() at
start_mon. Because sl_ctrl_link_fec_mon_stop() returns early without calling
timer_delete_sync() when fec_mon_timer_stop is already true, later calls to
sl_ctrl_link_fec_mon_stop() will not delete the re-armed timer.

3. In addition, sl_ctrl_link_policy_set() checks link_state via
sl_ctrl_data_link_state_get() and then calls sl_ctrl_link_fec_mon_start()
without holding data_lock across the state check, and
sl_ctrl_link_release_work() frees ctrl_link when in SL_LINK_STATE_DOWN
without calling sl_ctrl_link_fec_mon_stop() or
cancel_work_sync(&ctrl_link->fec_mon_timer_work).

Should start_mon check !ctrl_link->fec_mon_timer_stop under
fec_mon_timer_lock before calling mod_timer(), and should
sl_ctrl_link_release_work() unconditionally stop the timer and cancel
fec_mon_timer_work before freeing ctrl_link?

> +
> +#define SL_CTRL_LINK_FEC_LIMIT_25   25781250000ULL
> +#define SL_CTRL_LINK_FEC_LIMIT_50   53125000000ULL
> +#define SL_CTRL_LINK_FEC_LIMIT_100 106250000000ULL
> +u32 sl_ctrl_link_fec_limit_calc(struct sl_ctrl_link *ctrl_link, u32 mant, int exp)
> +{
> +	u64                 limit;
> +	int                 x;
> +	struct sl_link_caps link_caps;
> +
> +	sl_core_link_caps_get(ctrl_link->ctrl_lgrp->ctrl_ldev->num,
> +			      ctrl_link->ctrl_lgrp->num, ctrl_link->num, &link_caps);
> +
> +	sl_ctrl_log_dbg(ctrl_link, LOG_NAME,
> +			"fec limit calc (tech_map = 0x%X, mant = %u, exp = %d)",
> +		link_caps.tech_map, mant, exp);

[Severity: Medium]
In sl_ctrl_link_fec_limit_calc(), link_caps is uninitialized on the stack and
the return value of sl_core_link_caps_get() is not checked. If
sl_core_link_caps_get() returns -ENODEV, won't this read an uninitialized
link_caps.tech_map?

> diff --git a/drivers/net/ethernet/hpe/sl/ctrl/sl_ctrl_link_priv.c b/drivers/net/ethernet/hpe/sl/ctrl/sl_ctrl_link_priv.c
> new file mode 100644
> index 0000000000000..252673b57a0b3
> --- /dev/null
> +++ b/drivers/net/ethernet/hpe/sl/ctrl/sl_ctrl_link_priv.c

[ ... ]

> +int sl_ctrl_link_up_callback(void *tag, struct sl_core_link_up_info *up_info)
> +{
> +	struct sl_ctrl_link         *ctrl_link;
> +	char                         core_imap_str[SL_LINK_INFO_STRLEN];
> +	int                          max_up_tries;
> +	u32                          up_count;
> +	int                          rtn;
> +	s64                          attempt_time;
> +	s64                          total_time;
> +	s64                          up_time;

[ ... ]

> +		/* check up tries */
> +		rtn = sl_ctrl_link_up_count_get(ctrl_link->ctrl_lgrp->ctrl_ldev->num,
> +						ctrl_link->ctrl_lgrp->num, ctrl_link->num, &up_count);
> +		if (rtn)
> +			sl_ctrl_log_warn_trace(ctrl_link, LOG_NAME, "link_up_count_get failed [%d]", rtn);
> +
> +		if ((up_count >= max_up_tries) && (max_up_tries != SL_LINK_INFINITE_UP_TRIES)) {

[Severity: Medium]
In sl_ctrl_link_up_callback(), up_count, attempt_time, total_time, and
up_time are uninitialized on the stack. Although sl_ctrl_link_up_callback()
already has the ctrl_link pointer, it calls sl_ctrl_link_up_clocks_get() and
sl_ctrl_link_up_count_get(), which look up ctrl_link via sl_ctrl_link_get().

When sl_ctrl_link_del() has set ctrl_link->delete_requested = true,
sl_ctrl_link_get() returns NULL and both getters return -ENODEV without
writing their output arguments. Doesn't sl_ctrl_link_up_callback() then use
the uninitialized up_count in
"if ((up_count >= max_up_tries) && (max_up_tries != SL_LINK_INFINITE_UP_TRIES))"
(and log uninitialized up_count, attempt_time, and total_time in the
SL_CORE_LINK_STATE_UP case), similar to the uninitialized stack reads in
sl_ctrl_link_fec_data_check() and sl_ctrl_link_fec_limit_calc()?

Reading ctrl_link->up_clock directly via internal helpers Taking ctrl_link
(or initializing the variables and handling the error) would avoid this.

> diff --git a/drivers/net/ethernet/hpe/sl/ctrl/sl_ctrl_llr.c b/drivers/net/ethernet/hpe/sl/ctrl/sl_ctrl_llr.c
> new file mode 100644
> index 0000000000000..016ea353da4f5
> --- /dev/null
> +++ b/drivers/net/ethernet/hpe/sl/ctrl/sl_ctrl_llr.c

[ ... ]

> +	case SL_CORE_LLR_STATE_START_TIMEOUT:
> +		SL_CTRL_LLR_COUNTER_INC(ctrl_llr, LLR_START_TIMEOUT);
> +		if (ctrl_llr->policy.options & SL_LLR_POLICY_OPT_CONTINUOUS_START_TRIES) {
> +			sl_ctrl_log_dbg(ctrl_llr, LOG_NAME, "start TIMEOUT retry");
> +			rtn = sl_core_llr_start(ctrl_llr->ctrl_lgrp->ctrl_ldev->num, ctrl_llr->ctrl_lgrp->num,
> +						ctrl_llr->num, sl_ctrl_llr_start_callback, ctrl_llr, 0);
> +			if (rtn)
> +				sl_ctrl_log_warn_trace(ctrl_llr, LOG_NAME,
> +						       "start TIMEOUT core_llr_start retry failed [%d]", rtn);
> +		}
> +		rtn = sl_ctrl_lgrp_notif_enqueue(ctrl_llr->ctrl_lgrp, ctrl_llr->num,
> +						 SL_LGRP_NOTIF_LLR_START_TIMEOUT, NULL, ctrl_llr->start.imap);
> +		if (rtn)
> +			sl_ctrl_log_warn_trace(ctrl_llr, LOG_NAME,
> +					       "start TIMEOUT ctrl_lgrp_notif_enqueue failed [%d[", rtn);
> +		return;

[Severity: High]
Does the SL_LLR_POLICY_OPT_CONTINUOUS_START_TRIES retry in
sl_ctrl_llr_start_callback() ever succeed?

In drivers/net/ethernet/hpe/sl/core/hw/sl_core_hw_llr.c,
sl_core_hw_llr_start_timeout_work() sets core_llr->state to
SL_CORE_LLR_STATE_START_TIMEOUT before invoking sl_ctrl_llr_start_callback(),
and only transitions core_llr->state back to SL_CORE_LLR_STATE_SETUP after
the callback returns:

sl_core_hw_llr_start_timeout_work() {
    ...
	core_llr->state = SL_CORE_LLR_STATE_START_TIMEOUT;
	spin_unlock_irqrestore(&core_llr->data_lock, irq_flags);
    ...
	sl_core_hw_llr_start_callback(core_llr);

	spin_lock_irqsave(&core_llr->data_lock, irq_flags);
	core_llr->state = SL_CORE_LLR_STATE_SETUP;
	spin_unlock_irqrestore(&core_llr->data_lock, irq_flags);
}

Because sl_core_llr_start() only accepts SL_CORE_LLR_STATE_SETUP and returns
-EBADRQC for SL_CORE_LLR_STATE_START_TIMEOUT, calling sl_core_llr_start()
synchronously inside sl_ctrl_llr_start_callback() will always fail with
-EBADRQC (and even if it succeeded, sl_core_hw_llr_start_timeout_work() would
immediately overwrite core_llr->state back to SL_CORE_LLR_STATE_SETUP).
(Also note that ctrl_llr->policy.options, ctrl_llr->setup, and
ctrl_llr->start are accessed in these callbacks without holding
ctrl_llr->data_lock.)

> +	case SL_CORE_LLR_STATE_SETUP:
> +		SL_CTRL_LLR_COUNTER_INC(ctrl_llr, LLR_SETUP);
> +		rtn = sl_ctrl_lgrp_notif_enqueue(ctrl_llr->ctrl_lgrp, ctrl_llr->num,
> +						 SL_LGRP_NOTIF_LLR_CANCELED, NULL, ctrl_llr->start.imap);
> +		if (rtn)
> +			sl_ctrl_log_warn_trace(ctrl_llr, LOG_NAME,
> +					       "start CONFIGURED ctrl_lgrp_notif_enqueue failed [%d[", rtn);
> +		return;
> +	default:
> +		memset(&ctrl_llr->setup.data, 0, sizeof(struct sl_llr_data));
> +		sl_ctrl_log_err(ctrl_llr, LOG_NAME, "start ERROR invalid (state = %u %s)",
> +				core_llr_state, sl_core_llr_state_str(core_llr_state));
> +		info.error = -ENOMSG;
> +		rtn = sl_ctrl_lgrp_notif_enqueue(ctrl_llr->ctrl_lgrp, ctrl_llr->num, SL_LGRP_NOTIF_LLR_ERROR,
> +						 &info, ctrl_llr->setup.imap);

[Severity: Medium]
In the default branch of sl_ctrl_llr_start_callback(), should
sl_ctrl_lgrp_notif_enqueue() be passed ctrl_llr->start.imap instead of
ctrl_llr->setup.imap?

> diff --git a/drivers/net/ethernet/hpe/sl/ctrl/sl_ctrl_mac.c b/drivers/net/ethernet/hpe/sl/ctrl/sl_ctrl_mac.c
> new file mode 100644
> index 0000000000000..a4202757d238d
> --- /dev/null
> +++ b/drivers/net/ethernet/hpe/sl/ctrl/sl_ctrl_mac.c

[ ... ]

> +	tx_rtn = sl_core_mac_tx_stop(ldev_num, lgrp_num, mac_num);
> +	if (tx_rtn) {
> +		/* Log error, but allow the function to continue to mac rx stop */
> +		sl_ctrl_log_err_trace(ctrl_mac, LOG_NAME, "reset - core_mac_tx_stop failed [%d]", tx_rtn);
> +	}
> +
> +	rx_rtn = sl_core_mac_rx_stop(ldev_num, lgrp_num, mac_num);
> +	if (rx_rtn) {
> +		/* Log error, but allow the function to continue execution */
> +		sl_ctrl_log_err_trace(ctrl_mac, LOG_NAME, "core_mac_rx_stop failed [%d]", rx_rtn);
> +	}
> +
> +	if (tx_rtn && rx_rtn) {
> +		SL_CTRL_MAC_COUNTER_INC(ctrl_mac, MAC_RESET_FAIL);
> +		rtn = -EIO;
> +	} else {
> +		SL_CTRL_MAC_COUNTER_INC(ctrl_mac, MAC_RESET);
> +	}

[Severity: Medium]
In sl_ctrl_mac_reset(), should this check "if (tx_rtn || rx_rtn)" instead of
"if (tx_rtn && rx_rtn)" so that a failure in either sl_core_mac_tx_stop() or
sl_core_mac_rx_stop() is reported as a reset failure?

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