Re: [PATCH 07/23] net: hpe: sbl: add the SBM serial bus layer

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 5 potential issue(s) to consider:
- [High] net: hpe: sbl: mask before shift zeroes data in sbl_sbm_spico_int()
- [High] net: hpe: sbl: clobbered err and uninit result read in sbl_sbus_op_aux()
- [High] net: hpe: sbl: ABBA lock inversion in sbl_serdes_spico_int() via spico_reset
- [Low] net: hpe: sbl: non-static inline helpers in sbl_sbm.h lack prototypes
- [Low] net: hpe: sbl: excessive stack buffers in sbus_msg() and sbus_cmd_to_string()
--

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

net: hpe: sbl: add the SBM serial bus layer

This commit adds the Serial Bus Master (SBM) layer for accessing SerDes
and PMI registers along with NIC-specific SerDes address helpers.

> diff --git a/drivers/net/ethernet/hpe/sbl/sbl_sbm_serdes.c b/drivers/net/ethernet/hpe/sbl/sbl_sbm_serdes.c
> new file mode 100644
> index 0000000000000..1d1c4d01e1598
> --- /dev/null
> +++ b/drivers/net/ethernet/hpe/sbl/sbl_sbm_serdes.c
> @@ -0,0 +1,1114 @@
> +// SPDX-License-Identifier: GPL-2.0
> +
> +/* Copyright 2025-2026 Hewlett Packard Enterprise Development LP. All Rights Reserved. */
> +
> +#include <linux/kernel.h>
> +#include <linux/device.h>
> +#include <linux/delay.h>
> +
> +#include <uapi/ethernet/sbl_sbm_constants.h>
> +
> +#include "sbl_sbm_serdes.h"
> +#include "sbl_serdes_map.h"
> +#include "sbl_sbm.h"

[Severity: Low]
In drivers/net/ethernet/hpe/sbl/sbl_sbm.h, the nine inline helper functions
(sbl_sbm_sbus_op(), sbl_sbm_sbus_op_reset(), sbl_sbm_pml_serdes_op(),
sbl_sbm_get_sbus_op_timeout_ms(), sbl_sbm_get_sbus_int_timeout(),
sbl_sbm_get_sbus_int_poll_interval(), sbl_sbm_get_serdes_op_timeout_ms(),
sbl_sbm_get_sbus_op_flags(), and sbl_sbm_get_serdes_op_flags()) are defined
with bare inline rather than static inline:

drivers/net/ethernet/hpe/sbl/sbl_sbm.h:
inline int sbl_sbm_sbus_op(struct sbl_inst *sbl, int ring, u32 req_data,
			     u8 data_addr, u8 rx_addr, u8 command,
			     u32 *rsp_data, u8 *result_code, u8 *overrun,
			     int timeout, u32 flags)
{
	return sbl_sbus_op(sbl, ring, req_data, data_addr, rx_addr, command,
			   rsp_data, result_code, overrun, timeout, flags);
}

Because the kernel defines inline with __gnu_inline, bare inline functions
without static or extern have external linkage and emit out-of-line global
definitions without prior prototypes when sbl_sbm.h is included here.

Should these helpers in sbl_sbm.h be marked static inline to avoid
-Wmissing-prototypes and -Wmissing-declarations warnings (and multiple
definition errors if sbl_sbm.h is included elsewhere)?

[ ... ]

> +static void
> +sbus_cmd_to_string(u8 command, char *cmd_str)
> +{
> +	char str0[SBL_MAX_STR_LEN], str1[SBL_MAX_STR_LEN], str2[SBL_MAX_STR_LEN];
> +
> +	switch (command & SBUS_CMD_MODE_MASK) {
> +	case SBUS_CMD_MODE_CTLR:
> +		sprintf(str0, "CM_CTLR");
> +		break;
> +	case SBUS_CMD_MODE_RCVR:
> +		sprintf(str0, "CM_RCVR");
> +		break;
> +	default:
> +		sprintf(str0, "CM_UNKNOWN");
> +		break;
> +	}

[ ... ]

> +	sprintf(cmd_str, "%s:%s:%s", str0, str1, str2);
> +}

[ ... ]

> +static void sbus_msg(void *inst, u32 sbus_addr, u32 req_data,
> +		     u8 reg_addr, u8 command, u32 rsp_data,
> +		u8 result_code, u8 overrun, int timeout,
> +		u32 flags, int rc, int severity)
> +{
> +	struct sbl_inst *sbl = inst;
> +	char sbus_addr_str[SBL_HALF_MAX_STR_LEN];
> +	char reg_addr_str[SBL_HALF_MAX_STR_LEN];
> +	char cmd_str[SBL_HALF_MAX_STR_LEN];
> +	char rc_str[SBL_HALF_MAX_STR_LEN];
> +	char message[SBL_MAX_STR_LEN];
> +
> +	if (!sbl)
> +		return;
> +
> +	sbus_addr_to_string(sbus_addr, sbus_addr_str);
> +	sbus_reg_addr_to_string(sbus_addr, reg_addr, reg_addr_str);
> +	sbus_cmd_to_string(command, cmd_str);

[Severity: Low]
sbus_msg() allocates four SBL_HALF_MAX_STR_LEN (128-byte) arrays and one
SBL_MAX_STR_LEN (256-byte) array on the stack (768 bytes total), and its
single-caller helper sbus_cmd_to_string() allocates another three
SBL_MAX_STR_LEN (256-byte) arrays (768 bytes) just to copy string literals
before formatting them into the 128-byte cmd_str buffer.

When sbus_cmd_to_string() is inlined into sbus_msg(), the combined stack
frame exceeds 1536 bytes (and over 1792 bytes when called via
sbl_sbm_spico_int()), which triggers -Wframe-larger-than= warnings when
CONFIG_FRAME_WARN is 1024 or 1280.

Could sbus_cmd_to_string() use const char * pointers for str0, str1, and
str2 instead of stack arrays?

[ ... ]

> +int sbl_sbus_wr(void *inst, u32 sbus_addr, u8 reg_addr,
> +		u32 sbus_data)
> +{
> +	struct sbl_inst *sbl = inst;
> +	u32 unused;
> +
> +	return sbl_sbus_op_aux(sbl, sbus_addr, reg_addr, SBUS_IFACE_DST_CORE |
> +			       SBUS_CMD_WRITE, sbus_data, &unused);
> +}

[ ... ]

> +int sbl_sbus_op_aux(void *inst, u32 sbus_addr, u8 reg_addr,
> +		    u8 command, u32 sbus_data, u32 *result)
> +{
> +	struct sbl_inst *sbl = inst;
> +	u8 result_code = 0, overrun = 0;
> +	int err = -1;
> +	int retry_cnt = 0;
> +	int retry_limit = 5;
> +	u32 sbus_ring = SBUS_RING(sbus_addr);
> +	u32 rx_addr = SBUS_RX_ADDR(sbus_addr);
> +	bool valid_result = false;
> +	int sbus_op_timeout_ms = sbl_sbm_get_sbus_op_timeout_ms(sbl);
> +	int sbus_op_flags = sbl_sbm_get_sbus_op_flags(sbl);
> +	void *accessor = sbl;
> +
> +	if (!sbl->is_hw) {
> +		*result = 0;
> +		return 0;
> +	}
> +	/* Safety check */
> +	if (!mutex_is_locked(SBUS_RING_MTX(sbl, sbus_ring))) {
> +		sbus_msg(sbl, sbus_addr, sbus_data, reg_addr, command,
> +			 0, result_code, overrun, sbus_op_timeout_ms,
> +			 sbus_op_flags, err, LEVEL_WARN);
> +		WARN(1, "%s: Unlocked SBUS ring %d!", __func__, sbus_ring);
> +	}
> +
> +	/* Perform SBus operation */
> +	while (retry_cnt++ < retry_limit) {
> +		err = sbl_sbm_sbus_op(accessor, sbus_ring, sbus_data,
> +				      reg_addr, rx_addr, command, result,
> +					     &result_code, &overrun,
> +					     sbus_op_timeout_ms,
> +					     sbus_op_flags);
> +		if (err) {
> +			sbus_msg(sbl, sbus_addr, sbus_data, reg_addr, command,
> +				 0, result_code, overrun, sbus_op_timeout_ms,
> +				 sbus_op_flags, err, LEVEL_WARN);
> +		}
> +		if (err || overrun) {
> +			SBL_INFO(sbl->dev, "Resetting SBUS ring %d!", sbus_ring);
> +			err = sbl_sbm_sbus_op_reset(sbl, sbus_ring);
> +			if (err) {
> +				SBL_WARN(sbl->dev,
> +					 "sbl_sbm_sbus_op_reset failed! sbus_ring:%d rc:%d",
> +					 sbus_ring, err);
> +				return err;
> +			}
> +		} else {
> +			break;
> +		}
> +	}
> +
> +	/* Validate results */
> +	if (err || overrun) {
> +		SBL_WARN(sbl->dev, "roshms_sbus_op failed!");
> +		return -EIO;
> +	}
> +	switch (command & 0x3) {
> +	case SBUS_CMD_RESET:
> +		if (result_code == SBUS_RC_RESET)
> +			valid_result = true;
> +
> +		break;
> +	case SBUS_CMD_WRITE:
> +		if (result_code == SBUS_RC_WRITE_COMPLETE)
> +			valid_result = true;
> +
> +		break;
> +	case SBUS_CMD_READ:
> +		if (result_code == SBUS_RC_READ_COMPLETE)
> +			valid_result = true;
> +
> +		break;
> +	case SBUS_CMD_READ_RESULT:
> +		if (result_code == SBUS_RC_READ_ALL_COMPLETE)
> +			valid_result = true;
> +
> +		break;
> +	}
> +
> +	if (!valid_result) {
> +		SBL_WARN(sbl->dev, "Unexpected result code (%d) 0x%x!",
> +			 result_code, command);
> +		sbus_msg(sbl, sbus_addr, sbus_data, reg_addr, command,
> +			 *result, result_code, overrun, sbus_op_timeout_ms,
> +				sbus_op_flags, err, LEVEL_WARN);
> +		return -ENOMSG;
> +	}
> +	sbus_msg(sbl, sbus_addr, sbus_data, reg_addr, command,
> +		 *result, result_code, overrun, sbus_op_timeout_ms,
> +			sbus_op_flags, err, LEVEL_DBG);
> +
> +	return 0;
> +}

[Severity: High]
In sbl_sbus_op_aux(), when sbl_sbm_sbus_op() fails (err != 0), the error
recovery path overwrites err with the return value of
sbl_sbm_sbus_op_reset(sbl, sbus_ring).

If sbl_sbm_sbus_op() fails on all 5 retries with overrun == 0 while
sbl_sbm_sbus_op_reset() succeeds (returns 0), the loop terminates with
err == 0 and overrun == 0, bypassing the post-loop if (err || overrun)
check.

Because result_code was initialized to 0 (which equals SBUS_RC_RESET), a
failed SBUS_CMD_RESET operation (such as in sbl_sbm_firm_upload() and
sbl_serdes_soft_reset()) will set valid_result = true and return 0
(success), while failed read or write operations will return -ENOMSG
instead of -EIO.

In addition, sbl_sbus_wr() (as well as sbl_sbm_firm_upload() and
sbl_serdes_soft_reset()) passes an uninitialized stack variable (u32 unused)
as result to sbl_sbus_op_aux(), which unconditionally dereferences *result
when calling sbus_msg() even if sbl_sbm_sbus_op() did not write to *result.

Should sbl_sbm_sbus_op_reset() use a separate return variable so err from
sbl_sbm_sbus_op() is preserved, and should *result (or unused in callers)
be initialized to 0 before the operation?

> +
> +/**
> + * sbl_sbm_spico_int() - spico intilization
> + * @inst: Generic pointer used by various framework
> + * @sbus_addr: address describing a serdes ring and rxaddr
> + * @code: interrupt command
> + * @data: interrupt data
> + * @result: location to store result of interrupt
> + *
> + * Write an interrupt request to a target SBM Spico
> + *
> + * Return: 0 on success, negative error code on failure
> + */
> +int sbl_sbm_spico_int(void *inst, u32 sbus_addr, int code, int data,
> +		      u32 *result)
> +{
> +	struct sbl_inst *sbl = inst;
> +	int err;
> +	u32 intr_out;
> +	u32 intr_in;
> +	char intr_str[SBL_MAX_STR_LEN];
> +	int sbus_int_timeout = sbl_sbm_get_sbus_int_timeout(sbl);
> +	int sbus_int_poll_interval = sbl_sbm_get_sbus_int_poll_interval(sbl);
> +	unsigned long last_jiffy;
> +	u32 sbus_ring = SBUS_RING(sbus_addr);
> +
> +	if (!sbl->is_hw) {
> +		*result = 0;
> +		return 0;
> +	}
> +	/* Safety check */
> +	if (!mutex_is_locked(SBUS_RING_MTX(sbl, sbus_ring))) {
> +		SBL_WARN(sbl->dev, "%s: Unlocked SBUS ring %d!", __func__, sbus_ring);
> +		WARN(1, "sbl_sbus_op_aux: Unlocked SBUS ring %d!", sbus_ring);
> +	}
> +
> +	spico_interrupt_to_string(sbus_addr, code, intr_str);
> +
> +	/* Inject the interrupt via SBus writes */
> +	intr_in = ((data & SBMS_INTERRUPT_DATA_MASK) << SBMS_INTERRUPT_DATA_OFFSET) |
> +		((code & SBMS_INTERRUPT_CODE_MASK) << SBMS_INTERRUPT_CODE_OFFSET);

[Severity: High]
In include/uapi/ethernet/sbl_sbm_constants.h, SBMS_INTERRUPT_DATA_MASK is
defined as the already-shifted upper 16-bit mask:

#define SBMS_INTERRUPT_DATA_MASK         0xffff0000
#define SBMS_INTERRUPT_DATA_OFFSET       16

which matches how sbl_sbm_spico_int() extracts the result further down:

	*result = (intr_out & SBMS_INTERRUPT_DATA_MASK) >>
		SBMS_INTERRUPT_DATA_OFFSET;

When packing intr_in above, masking the 32-bit data with 0xffff0000 before
shifting left by 16 clears bits 15:0 and then shifts bits 31:16 out of the
32-bit word, so the data portion always evaluates to 0.

Should this shift data before masking with SBMS_INTERRUPT_DATA_MASK:

	intr_in = ((data << SBMS_INTERRUPT_DATA_OFFSET) & SBMS_INTERRUPT_DATA_MASK) |
		((code << SBMS_INTERRUPT_CODE_OFFSET) & SBMS_INTERRUPT_CODE_MASK);

so that the interrupt data payload is not zeroed out?

[ ... ]

> +int sbl_serdes_spico_int(void *inst, u32 port_num, u32 serdes,
> +			 int code, int data, u16 *result, u8 result_action)
> +{
> +	struct sbl_inst *sbl = inst;
> +	int err;
> +	char intr_str[SBL_MAX_STR_LEN];
> +	u16 result_out = 0;
> +	u16 *p_result;
> +	u32 sbus_addr;
> +	int serdes_op_timeout_ms = sbl_sbm_get_serdes_op_timeout_ms(sbl);
> +	int serdes_op_flags = sbl_sbm_get_serdes_op_flags(sbl);
> +	void *accessor = sbl;
> +
> +	if ((result_action == SPICO_INT_RETURN_RESULT) && (result == NULL)) {
> +		SBL_ERR(sbl->dev, "result pointer was NULL!");
> +		return -1;
> +	}
> +	if (result_action != SPICO_INT_RETURN_RESULT)
> +		p_result = &result_out;
> +	else
> +		p_result = result;
> +
> +	if (!sbl->is_hw) {
> +		*p_result = code;
> +		return 0;
> +	}
> +	/* Make a fake SerDes sbus_addr so spico_interrupt_to_string gives us the
> +	   right debug string. This function is only called for CM4 SerDes, so this
> +	   will always give the correct string. */
> +	sbl_sbus_addr_get(&sbus_addr);
> +	spico_interrupt_to_string(sbus_addr, code, intr_str);
> +
> +	err = sbl_sbm_pml_serdes_op(accessor, port_num, serdes, code,
> +				    data, p_result, serdes_op_timeout_ms,
> +					   serdes_op_flags);

[Severity: High]
Can calling sbl_sbm_pml_serdes_op() here cause an ABBA deadlock between
sbl->link[port_num].serdes_mtx and SBUS_RING_MTX(sbl, sbus_ring)?

In drivers/net/ethernet/hpe/sbl/sbl_serdes_fn.c, sbl_serdes_soft_reset()
and sbl_serdes_firm_upload() acquire serdes_mtx before sbus_ring_mtx:

sbl_serdes_soft_reset()
  mutex_lock(&sbl->link[port_num].serdes_mtx);
  mutex_lock(SBUS_RING_MTX(sbl, sbus_ring));

However, sbl_spico_reset() (called from sbl_serdes_stop_internal() and
sbl_test_serdes_stop()) acquires SBUS_RING_MTX(sbl, sbus_ring) first and
then calls sbl_serdes_spico_int() -> sbl_sbm_pml_serdes_op() ->
sbl_pml_serdes_op(), which locks &link->serdes_mtx:

sbl_spico_reset()
  mutex_lock(SBUS_RING_MTX(sbl, sbus_ring));
  sbl_serdes_spico_int()
    sbl_sbm_pml_serdes_op()
      sbl_pml_serdes_op()
        mutex_lock_interruptible(&link->serdes_mtx);

This inverts the lock order to sbus_ring_mtx -> serdes_mtx. Should
sbl_spico_reset() and its callers follow the serdes_mtx -> sbus_ring_mtx
lock hierarchy (or release sbus_ring_mtx before invoking
sbl_serdes_spico_int())?

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