Re: [PATCH 04/23] net: hpe: sbl: add the SerDes layer

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 13 potential issue(s) to consider:
- [High] net: hpe: sbl: out-of-bounds ring indexing and broken flash on NIC1
- [High] net: hpe: sbl: lock order inversion in sbl_spico_reset()
- [High] net: hpe: sbl: unbounded retry loop in sbl_serdes_tuning() on eye failure
- [Medium] net: hpe: sbl: leak of tps on error in sbl_check_serdes_tuning_params()
- [Medium] net: hpe: sbl: unlocked clear and valid magic in sbl_serdes_reset()
- [Medium] net: hpe: sbl: corrupted tuning_params left valid on save error
- [Medium] net: hpe: sbl: missing SBL_SC_MAX_INTR check on num_intr in config
- [Medium] net: hpe: sbl: stale values.num_intr and uninit result in sbl_serdes_config()
- [Medium] net: hpe: sbl: 16-bit pointer cast of uint in sbl_serdes_get_fw_vers()
- [Medium] net: hpe: sbl: sbl_port_dfe_tune_start() tunes TX-only SerDes lanes
- [Medium] net: hpe: sbl: SPICO_INT_SBMS_FW_STS used as SBus reg addr in flash_safe()
- [Low] net: hpe: sbl: commit message describes absent file and ops table
- [Low] net: hpe: sbl: pointer-sign mismatches in sbl_serdes_fn.c
--

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

net: hpe: sbl: add the SerDes layer

This commit adds the SerDes layer implementation in sbl_serdes.c and
sbl_serdes_fn.c to manage SerDes firmware loading, per-lane configuration,
tuning, and status reporting.

>     net: hpe: sbl: add the SerDes layer
>     
>     Add the SerDes layer: per-lane configuration, the autogenerated lane
>     map, and the function-pointer table that lets the PML layer drive the
>     SerDes without knowing which SerDes core is present.
>     
>     sbl_serdes_map_autogen.h is generated from the hardware lane
>     description and is kept in generated form, so style checkers will flag
>     it for line length and macro layout.
>     
>     checkpatch --strict (100 columns) still reports the following in this
>     patch. They need coordination with the internal repositories this code
>     is maintained in and will be fixed in V2:
>     - WARNING: LONG_LINE (45), BLOCK_COMMENT_STYLE (36), ALLOC_WITH_SIZEOF
>       (2), ENOTSUPP (1), LONG_LINE_COMMENT (1), LONG_LINE_STRING (1),
>       TYPO_SPELLING (1)
>     - CHECK: UNNECESSARY_PARENTHESES (37), BRACES (13), ALLOC_SIZEOF_STRUCT
>       (2), PARENTHESIS_ALIGNMENT (2), COMPARISON_TO_NULL (1), LINE_SPACING
>       (1)
>     
>     Co-developed-by: Mamila Santosh <santosh-kumar.mamila@hpe.com>
>     Signed-off-by: Mamila Santosh <santosh-kumar.mamila@hpe.com>
>     Signed-off-by: Doug Ledford <doug.ledford@hpe.com>

[Severity: Low]
Does the commit message match the contents of this patch?

The changelog states that this patch adds sbl_serdes_map_autogen.h and a
function-pointer table for driving the SerDes layer, but this commit only
adds drivers/net/ethernet/hpe/sbl/sbl_serdes.c and
drivers/net/ethernet/hpe/sbl/sbl_serdes_fn.c with direct C functions.

No file named sbl_serdes_map_autogen.h is present in the tree (the lane map
headers include/uapi/ethernet/sbl_serdes_map.h and
include/uapi/ethernet/sbl_serdes_map_init.h were added in commit
16e18859cf46 ("net: hpe: sbl: Add header files for SBL")).

> diff --git a/drivers/net/ethernet/hpe/sbl/sbl_serdes.c b/drivers/net/ethernet/hpe/sbl/sbl_serdes.c
> new file mode 100644
> index 0000000000000..4db5bb1a0bb91
> --- /dev/null
> +++ b/drivers/net/ethernet/hpe/sbl/sbl_serdes.c

[ ... ]

> @@ -382,56 +382,56 @@ int sbl_serdes_reset(struct sbl_inst *sbl, int port_num)
>  	link = sbl->link + port_num;
>  	link->sstate = SBL_SERDES_STATUS_RESETTING;
>  
>  	/* optionally clear any saved tuning params */
>  	if (link->blattr.options & SBL_OPT_RESET_CLEAR_PARAMS) {
>  		link->tuning_params.tp_state_hash0 = 0;
>  		link->tuning_params.tp_state_hash1 = 0;
>  		for (serdes = 0; serdes < sbl->switch_info->num_serdes; ++serdes) {
>  			memset(link->tuning_params.params[serdes].ctle, 0,
>  			       sizeof(u16) * NUM_CTLE_PARAMS);
>  			memset(link->tuning_params.params[serdes].ffe, 0,
>  			       sizeof(u16) * NUM_FFE_PARAMS);
>  			memset(link->tuning_params.params[serdes].dfe, 0,
>  			       sizeof(u16) * NUM_DFE_PARAMS);
>  			memset(link->tuning_params.params[serdes].rxvs, 0,
>  			       sizeof(u16) * NUM_RXVS_PARAMS);
>  			memset(link->tuning_params.params[serdes].rxvc, 0,
>  			       sizeof(u16) * NUM_RXVC_PARAMS);
>  			memset(link->tuning_params.params[serdes].rsdo, 0,
>  			       sizeof(u16) * NUM_RSDO_PARAMS);
>  			memset(link->tuning_params.params[serdes].rsdc, 0,
>  			       sizeof(u16) * NUM_RSDC_PARAMS);
>  			memset(link->tuning_params.params[serdes].rsto, 0,
>  			       sizeof(u16) * NUM_RSTO_PARAMS);
>  			memset(link->tuning_params.params[serdes].rstc, 0,
>  			       sizeof(u16) * NUM_RSTC_PARAMS);
>  			memset(link->tuning_params.params[serdes].eh, 0,
>  			       sizeof(u16) * NUM_EH_PARAMS);
>  			memset(link->tuning_params.params[serdes].gtp, 0,
>  			       sizeof(u16) * NUM_GTP_PARAMS);
>  			memset(link->tuning_params.params[serdes].dccd, 0,
>  			       sizeof(u16) * NUM_DCCD_PARAMS);
>  			memset(link->tuning_params.params[serdes].p4lv, 0,
>  			       sizeof(u16) * NUM_P4LV_PARAMS);
>  			memset(link->tuning_params.params[serdes].afec, 0,
>  			       sizeof(u16) * NUM_AFEC_PARAMS);
>  		}
>  	}

[Severity: Medium]
Can clearing link->tuning_params in sbl_serdes_reset() when
SBL_OPT_RESET_CLEAR_PARAMS is set race with concurrent readers or writers
and leave magic set to SBL_TUNING_PARAM_MAGIC?

Other accessors of link->tuning_params (sbl_serdes_get_tuning_params(),
sbl_serdes_set_tuning_params(), sbl_serdes_invalidate_tuning_params(), and
sbl_serdes_tuning()) serialize access under link->tuning_params_mtx. Here,
link->tuning_params is modified without holding link->tuning_params_mtx
(and in sbl_serdes_set_tuning_params(), tp_state_hash0 and tp_state_hash1
are also read for sbl_dev_dbg() after unlocking link->tuning_params_mtx).

In addition, sbl_serdes_reset() does not clear link->tuning_params.magic = 0
like sbl_serdes_invalidate_tuning_params() does. Because magic remains
SBL_TUNING_PARAM_MAGIC, a subsequent call to sbl_serdes_get_tuning_params()
will copy out the zeroed tuning_params structure instead of returning
-ENODATA.

> diff --git a/drivers/net/ethernet/hpe/sbl/sbl_serdes_fn.c b/drivers/net/ethernet/hpe/sbl/sbl_serdes_fn.c
> new file mode 100644
> index 0000000000000..f085d5c41da13
> --- /dev/null
> +++ b/drivers/net/ethernet/hpe/sbl/sbl_serdes_fn.c

[ ... ]

> +void sbl_serdes_get_fw_vers(struct sbl_inst *sbl, int port_num, int serdes,
> +			    uint *fw_rev, uint *fw_build)
> +{
> +	if (sbl_serdes_spico_int(sbl, port_num, serdes, SPICO_INT_CM4_REV_ID,
> +				 SPICO_INT_DATA_NONE, (uint16_t *)fw_rev,
> +				SPICO_INT_RETURN_RESULT)) {
> +		/* Failure expected when Spico is in reset */
> +		dev_dbg(sbl->dev, "p%ds%d: Failed to read firmware rev!", port_num, serdes);
> +		*fw_rev = 0x0;
> +	}
> +	if (sbl_serdes_spico_int(sbl, port_num, serdes, SPICO_INT_CM4_BUILD_ID,
> +				 SPICO_INT_DATA_NONE, (uint16_t *)fw_build,
> +				SPICO_INT_RETURN_RESULT)) {
> +		/* Failure expected when Spico is in reset */
> +		dev_dbg(sbl->dev, "p%ds%d: Failed to read firmware build!", port_num, serdes);
> +		*fw_build = 0x0;
> +	}
> +}

[Severity: Medium]
Does casting uint *fw_rev and uint *fw_build to uint16_t * in
sbl_serdes_get_fw_vers() leave the upper 16 bits uninitialized on success
and write to the wrong halfword on big-endian architectures?

sbl_serdes_spico_int() only writes a 16-bit u16 value through its result
pointer. When sbl_serdes_spico_int() succeeds, the other 16 bits of the
32-bit uint pointed to by fw_rev and fw_build are never written by
sbl_serdes_get_fw_vers() (also note that in sbl_sbm_fw_sysfs_sprint() in
sbl_serdes.c, uint fw_rev is declared without initialization alongside
fw_build = 0).

[ ... ]

> +/* Parse the vers string into rev and build */
> +static int sbl_parse_version_string(struct sbl_inst *sbl, const char *fw_fname,
> +				    int *fw_rev, int *fw_build)
> +{
> +	char  rev_str[SBL_MAX_STR_LEN];
> +	char  build_str[SBL_MAX_STR_LEN];
> +	char  *p;
> +	int err;
> +
> +	p = strstr(fw_fname, ".");
> +	if (p == NULL) {
> +		sbl_dev_err(sbl->dev, "Bad firmware file name: %s", fw_fname);
> +		return -EINVAL;
> +	}
> +
> +	if (strlen(p) < strlen(".0x0000_0000")) {
> +		sbl_dev_err(sbl->dev, "Bad firmware file name: %s", fw_fname);
> +		return -EINVAL;
> +	}
> +
> +	memcpy(rev_str, &(p[3]), SBL_FW_REV_LEN);
> +	rev_str[SBL_FW_REV_LEN] = '\n';
> +	rev_str[SBL_FW_REV_LEN + 1] = '\0';
> +	memcpy(build_str, &(p[8]), SBL_FW_BUILD_LEN);
> +	build_str[SBL_FW_BUILD_LEN] = '\n';
> +	build_str[SBL_FW_BUILD_LEN + 1] = '\0';
> +
> +	err = kstrtou32(rev_str, 16, fw_rev);
> +	if (err) {
> +		sbl_dev_err(sbl->dev, "Failed to convert %s to an integer [%d]", rev_str, err);
> +		return err;
> +	}
> +
> +	err = kstrtou32(build_str, 16, fw_build);
> +	if (err) {
> +		sbl_dev_err(sbl->dev, "Failed to convert %s to an integer [%d]", build_str, err);
> +		return err;
> +	}
> +	return 0;
> +}

[Severity: Low]
Will passing signed int *fw_rev and int *fw_build to kstrtou32() here, and
passing signed __s16 * parameter array elements (ctle, ffe, dfe, rxvs, rxvc,
rsdo, rsdc, rsto, rstc, eh, gtp, dccd, p4lv, afec) from struct
sbl_serdes_params to the u16 * result parameter of sbl_serdes_spico_int() in
sbl_get_serdes_tuning_params(), trigger -Wpointer-sign compiler warnings?

> +
> +void sbl_sbm_get_fw_vers(struct sbl_inst *sbl, int sbus_ring, uint *fw_rev, uint *fw_build)
> +{
> +	uint sbus_addr = SBUS_ADDR(sbus_ring, SBUS_BCAST_SBM_SPICO);
> +
> +	/* SBUS Critical Section */
> +	mutex_lock(&sbl->sbus_ring_mtx[sbus_ring]);
> +
> +	if (sbl_sbm_spico_int(sbl, sbus_addr, SPICO_INT_SBMS_REV_ID,
> +			      SPICO_INT_DATA_NONE, fw_rev)) {
> +		/* Failure expected when Spico is in reset */
> +		dev_dbg(sbl->dev, "sbm%d: Failed to read firmware rev from 0x%x", sbus_ring, sbus_addr);
> +		*fw_rev = 0x0;
> +	}
> +	if (sbl_sbm_spico_int(sbl, sbus_addr, SPICO_INT_SBMS_BUILD_ID,
> +			      SPICO_INT_DATA_NONE, fw_build)) {
> +		/* Failure expected when Spico is in reset */
> +		dev_dbg(sbl->dev, "sbm%d: Failed to read firmware build from 0x%x", sbus_ring, sbus_addr);
> +		*fw_build = 0x0;
> +	}
> +	mutex_unlock(&sbl->sbus_ring_mtx[sbus_ring]);
> +}
> +
> +int sbl_sbm_firmware_flash(struct sbl_inst *sbl)
> +{
> +	int err;
> +
> +	err = sbl_sbm_firmware_flash_ring(sbl, 0,
> +					  sbl->switch_info->num_sbus_rings - 1, false);
> +	if (err)
> +		/* sending port 0 in event as this event doesn't apply to any specific port */
> +		sbl_async_alert(sbl, 0, SBL_ASYNC_ALERT_SBM_FW_LOAD_FAILURE, NULL, 0);
> +
> +	return err;
> +}
> +
> +int sbl_sbm_firmware_flash_ring(struct sbl_inst *sbl, int first_ring,
> +				int last_ring, bool force)
> +{
> +	bool flash_needed, fw_requested = false;
> +	int sbus_ring;
> +	int err;
> +	int fw_rev, fw_build;
> +	const struct firmware *fw = NULL;
> +
> +	if ((last_ring < first_ring) || (first_ring < 0) ||
> +	    (last_ring > sbl->switch_info->num_sbus_rings - 1)) {
> +		sbl_dev_err(sbl->dev, "Invalid rings specified first:%d last:%d",
> +			    first_ring, last_ring);
> +		return -EINVAL;
> +	}

[ ... ]

> +static void sbl_serdes_firmware_validation(struct sbl_inst *sbl, const struct firmware *fw,
> +					   u32 sbus_addr, u32 result, u32 sbus_ring)
> +{
> +	int rc, i;
> +	u8 data;
> +	bool corruption_found = false;
> +
> +	for (i = 0; i < fw->size; ++i) {
> +		if (i % 2 == 0) {
> +			rc = sbl_sbus_wr(sbl, sbus_addr,
> +					SPICO_SBR_ADDR_IMEM, i / 2);
> +			if (rc)
> +				break;
> +			rc = sbl_sbus_rd(sbl, sbus_addr,
> +					 SPICO_SBR_ADDR_RDATA,
> +					&result);
> +			if (rc)
> +				break;
> +			data = (result & 0xff00) >> 8;
> +		} else {
> +			data = result & 0xff;
> +		}
> +		if (data != fw->data[i]) {
> +			corruption_found = true;
> +			sbl_dev_warn(sbl->dev, "0x%x: Act 0x%4.4x Exp 0x%4.4x", i, data, fw->data[i]);
> +		}
> +	}
> +	rc = sbl_sbus_wr(sbl, sbus_addr, SPICO_SBR_ADDR_CTL,
> +			 SPICO_SBR_DATA_IMEM_CNTL_DIS);
> +	if (rc)
> +		sbl_dev_err(sbl->dev, "SBM Imem rd disable failed [%d]", rc);
> +	if (corruption_found)
> +		sbl_dev_err(sbl->dev, "r%d: SBM FW corruption found", sbus_ring);
> +	else
> +		sbl_dev_info(sbl->dev, "r%d: No SBM FW corruption found", sbus_ring);
> +}
> +
> +int sbl_serdes_firmware_flash_safe(struct sbl_inst *sbl, int port_num,
> +				   bool force)
> +{
> +	int rc;
> +	u32 sbus_addr, crc_result, sbus_ring;
> +	u32 curr_fw_rev = 0, curr_fw_build = 0;
> +	u32 result;
> +	const struct firmware *fw = NULL;
> +	int fw_rev, fw_build;
> +	int serdes = 0;
> +
> +	if (!sbl->is_hw)
> +		return 0;
> +
> +	if (port_num == SBL_ALL_PORTS)
> +		return -ENOTSUPP;
> +
> +	sbus_ring = sbl->switch_info->ports[port_num].serdes[serdes].sbus_ring;
> +
> +	/* First, try the FW flash */
> +	rc = sbl_serdes_firmware_flash(sbl, port_num, force);
> +	if (rc)
> +		goto sbm_fw_reload;
> +
> +	/* Now, validate the SerDes FW - this also validates
> +	 * SPICO interrupts are working correctly.
> +	 */
> +	rc = sbl_parse_version_string(sbl, sbl->iattr.serdes_fw_fname,
> +				      &fw_rev, &fw_build);
> +	if (rc) {
> +		sbl_dev_err(sbl->dev,
> +			    "Failed to parse version string %s [%d]",
> +			sbl->iattr.sbm_fw_fname, rc);
> +		return rc;
> +	}

[ ... ]

> + sbm_fw_reload:
> +	if (sbl_debug_option(sbl, port_num, SBL_DEBUG_INHIBIT_RELOAD_SBM_FW)) {
> +		sbl_dev_warn(sbl->dev, "p%d: SBus Master FW reload inhibited", port_num);
> +		return rc;
> +	}
> +
> +	/* We may trigger a sbus master FW reload from multiple ports at the
> +	 * same time. Ensure we only actually reload the firmware once per
> +	 * sbus ring
> +	 */
> +	sbl->reload_sbm_fw[sbus_ring] = true;
> +	mutex_lock(SBM_FW_MTX(sbl, sbus_ring));
> +	if (!sbl->reload_sbm_fw[sbus_ring]) {
> +		sbl_dev_info(sbl->dev, "r%d: Sbus master FW reload no longer needed", sbus_ring);
> +	} else {

[ ... ]

> +		/* FW Status */
> +		rc = sbl_sbus_rd(sbl, sbus_addr, SPICO_INT_SBMS_FW_STS,
> +				 &result);
> +		if (rc) {
> +			sbl_dev_err(sbl->dev,
> +				    "p%d(0x%x): FW status read failed (%d)!",
> +				port_num, sbus_addr, rc);
> +		} else {
> +			sbl_dev_info(sbl->dev, "p%d(0x%x): FW status: 0x%x",
> +				     port_num, sbus_addr, result);
> +		}

[Severity: Medium]
Should the FW status query in sbl_serdes_firmware_flash_safe() call
sbl_sbm_spico_int() instead of sbl_sbus_rd()?

SPICO_INT_SBMS_FW_STS (0x20) is defined as an SBus Master SPICO interrupt
code alongside SPICO_INT_SBMS_REV_ID, SPICO_INT_SBMS_BUILD_ID, and
SPICO_INT_SBMS_DO_CRC, whereas valid SBus receiver register addresses
(SPICO_SBR_ADDR_*) are 0x0..0x1b and 0xff. Passing SPICO_INT_SBMS_FW_STS to
sbl_sbus_rd() reads register 0x20 instead of issuing the SPICO interrupt.

Also in this recovery path:
1. In sbl_serdes_firmware_validation(), if sbl_sbus_wr() or sbl_sbus_rd()
   fails mid-loop and breaks out early while corruption_found is false, it
   still logs "No SBM FW corruption found".
2. Earlier in sbl_serdes_firmware_flash_safe(), when
   sbl_parse_version_string() fails on sbl->iattr.serdes_fw_fname, the
   error message logs sbl->iattr.sbm_fw_fname instead.

> +
> +		rc = request_firmware(&fw, sbl->iattr.sbm_fw_fname, sbl->dev);
> +		if (rc != 0) {
> +			sbl_dev_err(sbl->dev, "firmware request failed [%d]", rc);
> +		} else {
> +			sbl_dev_info(sbl->dev,
> +				     "p%d(0x%x): Checking SBM FW for corruption...",
> +				 port_num, sbus_addr);
> +			rc = sbl_sbus_wr(sbl, sbus_addr, SPICO_SBR_ADDR_CTL,
> +					 SPICO_SBR_DATA_IMEM_CNTL_EN_RD);
> +			if (rc)
> +				sbl_dev_err(sbl->dev, "SBM Imem rd enable failed [%d]", rc);
> +			else
> +				sbl_serdes_firmware_validation(sbl, fw, sbus_addr, result, sbus_ring);
> +
> +			release_firmware(fw);
> +		}
> +
> +		mutex_unlock(SBUS_RING_MTX(sbl, sbus_ring));
> +
> +		/* Now, try reloading the sbus master FW */
> +		rc = sbl_sbm_firmware_flash_ring(sbl, sbus_ring, sbus_ring, true);
> +		if (rc)
> +			sbl_dev_err(sbl->dev, "r%d: SBM FW flash failed (%d)!", sbus_ring, rc);
> +		else
> +			sbl_dev_info(sbl->dev, "r%d: SBM FW flash succeeded", sbus_ring);
> +
> +		sbl->reload_sbm_fw[sbus_ring] = false;
> +	}
> +	mutex_unlock(SBM_FW_MTX(sbl, sbus_ring));

[Severity: High]
Does indexing by hardware sbus_ring cause out-of-bounds array accesses and
failed firmware flashing on Sawtooth NIC1?

In sbl_new_instance() (drivers/net/ethernet/hpe/sbl/sbl_inst.c),
sbl->sbus_ring_mtx, sbl->sbm_fw_mtx, sbl->reload_sbm_fw, and
sbl->sbm_fw_reload_count are allocated with sbl->switch_info->num_sbus_rings
(1) element, so only index 0 is valid. However, on Sawtooth NIC1
(cas_nic1_switch_info / SBL_CASSINI_NIC1_SW_INFO_INITIALIZER),
num_sbus_rings is 1 while ports[0].serdes[0..3].sbus_ring is 1.

When running on NIC1:
1. sbl_serdes_firmware_flash_safe() reads sbus_ring = 1 from
   sbl->switch_info->ports[port_num].serdes[serdes].sbus_ring and accesses
   sbl->reload_sbm_fw[1] out of bounds on the 1-element array (and also
   writes sbl->reload_sbm_fw[sbus_ring] = true before locking SBM_FW_MTX and
   clears it to false even when sbl_sbm_firmware_flash_ring() fails).
2. sbl_serdes_firmware_flash_safe() calls
   sbl_sbm_firmware_flash_ring(sbl, 1, 1, true), which fails the check
   last_ring > sbl->switch_info->num_sbus_rings - 1 (1 > 0) and returns
   -EINVAL.
3. sbl_sbm_firm_upload() increments sbl->sbm_fw_reload_count[sbus_ring] out
   of bounds when sbus_ring is 1, and sbl_sbm_get_fw_vers() indexes
   &sbl->sbus_ring_mtx[sbus_ring] directly instead of using
   SBUS_RING_MTX(sbl, sbus_ring).
4. Functions that loop from 0 to num_sbus_rings - 1, including
   sbl_sbm_firmware_flash(), sbl_serdes_firm_upload() when port_num ==
   SBL_ALL_PORTS, and sbl_apply_sbus_divider(), only iterate ring 0 and
   never address NIC1's hardware ring 1 (and in sbl_serdes_firmware_flash(),
   looping sr over mutex_lock(SBM_FW_MTX(sbl, sr)) would self-deadlock if
   num_sbus_rings > 1 because SBM_FW_MTX ignores sr).

[ ... ]

> +static int sbl_get_serdes_tuning_params(struct sbl_inst *sbl, int port_num,
> +					struct sbl_tuning_params *tps)
> +{
> +	int i, serdes, err;
> +
> +	if (!tps)
> +		return -EINVAL;
> +
> +	tps->magic = SBL_TUNING_PARAM_MAGIC;
> +	tps->version = SBL_TUNING_PARAM_VERSION;

[ ... ]

> +int sbl_check_serdes_tuning_params(struct sbl_inst *sbl, int port_num)
> +{
> +	int i, serdes, err;
> +	struct sbl_tuning_params *tps;
> +	int min, max;
> +	int num_oob_params;
> +	int max_oob_params = SBL_MAX_OOB_SERDES_PARAMS;
> +
> +	tps = kzalloc(sizeof(struct sbl_tuning_params), GFP_KERNEL);
> +	if (!tps)
> +		return -ENOMEM;

[ ... ]

> +		if (num_oob_params > max_oob_params) {
> +			sbl_dev_err(sbl->dev, "p%ds%d: Too many tuning params out of bounds(%d, max %d)!",
> +				    port_num, serdes, num_oob_params, max_oob_params);
> +			return -EDQUOT;
> +		} else if (num_oob_params > 0) {
> +			sbl_dev_info(sbl->dev, "p%ds%d: Some tuning params out of bounds(%d, max %d)",
> +				     port_num, serdes, num_oob_params, max_oob_params);
> +		}
> +	}
> +
> +	kfree(tps);
> +	return 0;
> +}

[Severity: Medium]
Does sbl_check_serdes_tuning_params() leak tps when returning -EDQUOT?

When num_oob_params > max_oob_params, the function returns -EDQUOT directly
without freeing tps allocated at the start of
sbl_check_serdes_tuning_params().

> +
> +int sbl_save_serdes_tuning_params(struct sbl_inst *sbl, int port_num)
> +{
> +	int err;
> +
> +	sbl->link[port_num].tuning_params.tp_state_hash0 =
> +		sbl_get_tp_hash0(sbl, port_num);
> +	sbl->link[port_num].tuning_params.tp_state_hash1 =
> +		sbl_get_tp_hash1(sbl, port_num);
> +	sbl_dev_dbg(sbl->dev, "Updated hash0 to 0x%llx, hash1 to 0x%llx",
> +		    sbl->link[port_num].tuning_params.tp_state_hash0,
> +		sbl->link[port_num].tuning_params.tp_state_hash1);
> +
> +	err = sbl_get_serdes_tuning_params(sbl, port_num,
> +					   &sbl->link[port_num].tuning_params);
> +
> +	return err;
> +}

[Severity: Medium]
What happens to link->tuning_params if sbl_get_serdes_tuning_params() fails
partway through when called from sbl_save_serdes_tuning_params()?

sbl_save_serdes_tuning_params() writes tp_state_hash0 and tp_state_hash1
in-place, and sbl_get_serdes_tuning_params() sets tps->magic =
SBL_TUNING_PARAM_MAGIC and tps->version = SBL_TUNING_PARAM_VERSION before
reading the per-lane parameters from hardware.

If a SPICO read or the sbl_validate_serdes_fw_vers() check fails after lane
0's CTLE/FFE/DFE values have been written, sbl_serdes_tuning() logs a
warning and continues without invalidating link->tuning_params. That leaves
partially populated tuning parameters marked with valid magic and matching
state hashes, so sbl_serdes_get_tuning_params() will return corrupted data
and sbl_is_retune() will treat the incomplete parameters as valid on the
next link start.

[ ... ]

> +int sbl_serdes_dfe_tune_wait(struct sbl_inst *sbl, int port_num)
> +{

[ ... ]

> +		if (!in_progress_mask)
> +			break;
> +
> +		msleep(link->blattr.dfe_poll_interval);
> +	} while (time_is_after_jiffies(last_jiffy) &&
> +		 !sbl_start_timeout(sbl, port_num) &&
> +		 !sbl_base_link_start_cancelled(sbl, port_num));
> +
> +	if (serdes_mask == tuned_mask)
> +		return 0;
> +
> +	if (sbl_base_link_start_cancelled(sbl, port_num))
> +		return -ECANCELED;
> +
> +	if (sbl_start_timeout(sbl, port_num)) {
> +		sbl_dev_dbg(sbl->dev, "p%d: start timeout waiting for DFE to tune to complete\n",
> +			    port_num);
> +		return -ETIMEDOUT;
> +	}
> +
> +	/* just didnt finish */
> +	sbl_dev_dbg(sbl->dev, "p%d serdes_mask:0x%x: Timeout waiting for DFE to complete (timeout:%ds)",
> +		    port_num, serdes_mask, link->blattr.dfe_timeout);
> +	return -ETIME;
> +}
> +
> +int sbl_port_dfe_tune_start(struct sbl_inst *sbl, int port_num, bool is_retune)
> +{
> +	int err, serdes;
> +
> +	sbl_dev_dbg(sbl->dev, "p%d: DFE tune retune:%d", port_num, is_retune);
> +
> +	/* DFE Tune */
> +	for (serdes = 0; serdes < sbl->switch_info->num_serdes; ++serdes) {
> +		if (!(rx_serdes_required_for_link_mode(sbl, port_num, serdes) ||
> +		      tx_serdes_required_for_link_mode(sbl, port_num, serdes)))
> +			continue;
> +		err = sbl_serdes_dfe_tune_start(sbl, port_num, serdes,
> +						is_retune);
> +		if (err) {
> +			sbl_dev_err(sbl->dev, "p%d: Failed to start DFE tune!",
> +				    port_num);
> +			return err;
> +		}
> +	}
> +
> +	return 0;
> +}

[Severity: Medium]
Should sbl_port_dfe_tune_start() only check
rx_serdes_required_for_link_mode() instead of
(rx_serdes_required_for_link_mode() || tx_serdes_required_for_link_mode())?

In SBL_LINK_MODE_CD_50G and SBL_LINK_MODE_CD_100G,
tx_serdes_required_for_link_mode() is true for physical lane 0 (and active
TX lanes) even when rx_serdes_required_for_link_mode() is false and
sbl_serdes_config() disabled the receiver on that lane. All other RX DFE and
calibration functions (sbl_serdes_dfe_tune_wait(), sbl_port_check_eyes(),
sbl_check_serdes_tuning_params(), sbl_port_start_pcal(), and
sbl_port_stop_pcal()) skip lanes where !rx_serdes_required_for_link_mode(),
so starting RX DFE calibration on TX-only lanes here tunes disabled
receivers that sbl_serdes_dfe_tune_wait() never waits on.

[ ... ]

> +int sbl_serdes_config(struct sbl_inst *sbl, int port_num, bool allow_an)
> +{

[ ... ]

> +	/* Set port config values */
> +	if (!allow_an) {
> +		for (serdes = 0; serdes < sbl->switch_info->num_serdes; ++serdes) {
> +			if (!(rx_serdes_required_for_link_mode(sbl, port_num, serdes) ||
> +			      tx_serdes_required_for_link_mode(sbl, port_num, serdes)))
> +				continue;
> +			use_default_tx_eq = false;
> +			use_default_gs    = false;
> +			err = sbl_get_serdes_config_values(sbl, port_num,
> +							   serdes,
> +								&values);
> +			if (err) {
> +				sbl_dev_warn(sbl->dev,
> +						     "p%ds%d: Unable to read config list!",
> +						     port_num, serdes);
> +				use_default_tx_eq = true;
> +				use_default_gs    = true;
> +			}

[ ... ]

> +			if (values.num_intr) {
> +				sbl_dev_dbg(sbl->dev,
> +					    "p%ds%d: Applying %d interrupts",
> +					port_num, serdes, values.num_intr);
> +			}
> +			for (j = 0; j < values.num_intr; ++j) {
> +				sbl_dev_dbg(sbl->dev,
> +					    "p%d: Applying interrupt 0x%x with data 0x%x",
> +				port_num, values.intr_val[j],
> +				values.data_val[j]);
> +				err = sbl_serdes_spico_int(sbl, port_num,
> +							   serdes, values.intr_val[j],
> +						values.data_val[j], &result,
> +						SPICO_INT_RETURN_RESULT);
> +				if (err) {
> +					sbl_dev_warn(sbl->dev,
> +						     "p%ds%d: interrupt 0x%x data 0x%x failed!",
> +						     port_num, serdes, values.intr_val[j],
> +						     values.data_val[j]);
> +				}
> +				if (result != values.intr_val[j]) {
> +					sbl_dev_dbg(sbl->dev,
> +						    "p%ds%d: interrupt:0x%x data:0x%x result:0x%x != code:0x%x. This is okay in some cases.",
> +						 port_num, serdes, values.intr_val[j], values.data_val[j], result, values.intr_val[j]);
> +				}
> +			}
> +		}
> +	}

[Severity: Medium]
Can this loop in sbl_serdes_config() overflow values.intr_val[] and
values.data_val[]?

In struct sbl_sc_values, intr_val and data_val are sized to SBL_SC_MAX_INTR
(16), but neither sbl_serdes_add_config() in sbl_serdes.c nor
sbl_serdes_config() validates that num_intr <= SBL_SC_MAX_INTR.

[Severity: Medium]
Also in sbl_serdes_config(), what happens if sbl_get_serdes_config_values()
succeeds on an earlier lane with values.num_intr > 0 and then fails on a
subsequent lane?

Because struct sbl_sc_values values is declared outside the per-SerDes loop
and the error path only sets use_default_tx_eq = true and
use_default_gs = true without resetting values.num_intr = 0, the previous
lane's SPICO interrupts will be re-applied to the current lane.

In addition, u16 result is uninitialized at declaration; if
sbl_serdes_spico_int() fails and returns an error without writing *result,
the check if (result != values.intr_val[j]) still reads the uninitialized or
stale value of result.

[ ... ]

> +int sbl_serdes_tuning(struct sbl_inst *sbl, int port_num)
> +{

[ ... ]

> +	sbl_link_tune_zero_total_timespec(sbl, port_num);
> +	link->dfe_tune_count = -1;
> +	while (true) {
> +		link->dfe_tune_count++;
> +		link->serr = sbl_port_dfe_tune(sbl, port_num, is_retune);
> +
> +		switch (link->serr) {
> +		case 0:
> +			sbl_dev_dbg(sbl->dev, "p%d: dfe %stune good\n", port_num,
> +				    is_retune ? "re" : "");

[ ... ]

> +		case -ELNRNG:   /* tuning params out of range */
> +			if (link->tune_param_oob_count >= SBL_MAX_TUNE_PARAM_OOB_FAILS) {
> +				sbl_dev_info(sbl->dev, "%d: too many params oob fails\n", port_num);
> +				/* give up and return error
> +				   The calling framework should reset the spico to delete any
> +				   applied tuning params so we can do a full tune */
> +				goto out;
> +			}
> +			fallthrough;
> +
> +		case -ETIME:    /* tuning did not complete */
> +		case -ECHRNG:   /* eye heights bad */
> +
> +			/*  try to tune again */
> +
> +			break;
> +
> +		case -ECANCELED:
> +			sbl_dev_dbg(sbl->dev, "p%d: %stune cancelled\n", port_num,
> +				    is_retune ? "re" : "");
> +			goto out;
> +
> +		case -ETIMEDOUT:
> +			sbl_dev_dbg(sbl->dev, "p%d: %stune timed out\n", port_num,
> +				    is_retune ? "re" : "");
> +			goto out;
> +
> +		default:
> +			sbl_dev_dbg(sbl->dev, "p%d: %stune failed [%d]\n", port_num,
> +				    is_retune ? "re" : "", link->serr);
> +			goto out;
> +		}
> +	}

[Severity: High]
Can this while (true) loop in sbl_serdes_tuning() spin indefinitely when
sbl_port_check_eyes() repeatedly returns -ECHRNG?

In sbl_serdes_dfe_tune_wait(), as soon as all active RX lanes complete DFE
calibration, the loop breaks at if (!in_progress_mask) break and immediately
returns 0 at if (serdes_mask == tuned_mask) return 0 before checking
sbl_base_link_start_cancelled() or sbl_start_timeout().

If sbl_port_check_eyes() in sbl_port_dfe_tune_wait() then returns -ECHRNG,
sbl_serdes_tuning() retries in while (true) without ever checking
sbl_start_timeout() or sbl_base_link_start_cancelled().

> +
> +out:
> +	sbl_dev_dbg(sbl->dev, "p%d: SerDes tuning complete", port_num);
> +	return link->serr;
> +}
> +
> +int sbl_spico_reset(struct sbl_inst *sbl, int port_num)
> +{
> +	int serdes;
> +	int err = 0;
> +	int pass;
> +	u32 sbus_addr;
> +	u32 sbus_ring;
> +	u32 result[SBL_SERDES_LANES_PER_PORT];
> +	unsigned long last_jiffy;
> +
> +	sbl_dev_dbg(sbl->dev, "p%d: spico reset start", port_num);
> +
> +	sbus_ring = sbl->switch_info->ports[port_num].serdes[0].sbus_ring;
> +	if (mutex_is_locked(SBUS_RING_MTX(sbl, sbus_ring)))
> +		sbl_dev_dbg(sbl->dev, "spico_reset: Sbus contention detected, sbus_ring_mtx[%d] locked", sbus_ring);
> +
> +	/* SBUS Critical Section */
> +	mutex_lock(SBUS_RING_MTX(sbl, sbus_ring));
> +
> +	/* Issue the reset */
> +	for (serdes = 0; serdes < sbl->switch_info->num_serdes; ++serdes) {
> +		if (!(rx_serdes_required_for_link_mode(sbl, port_num, serdes) ||
> +		      tx_serdes_required_for_link_mode(sbl, port_num, serdes)))
> +			continue;
> +		err = sbl_serdes_spico_int(sbl, port_num, serdes,
> +					   SPICO_INT_CM4_PROC_RESET,
> +				SPICO_INT_DATA_PROC_RESET, NULL,
> +				SPICO_INT_IGNORE_RESULT);

[Severity: High]
Does sbl_spico_reset() invert the lock ordering between
SBUS_RING_MTX(sbl, sbus_ring) and sbl->link[port_num].serdes_mtx?

In sbl_serdes_soft_reset() and sbl_serdes_firm_upload(),
sbl->link[port_num].serdes_mtx is acquired first, followed by
SBUS_RING_MTX(sbl, sbus_ring).

Here in sbl_spico_reset(), SBUS_RING_MTX(sbl, sbus_ring) is acquired first,
and then sbl_serdes_spico_int() -> sbl_sbm_pml_serdes_op() ->
sbl_pml_serdes_op() acquires sbl->link[port_num].serdes_mtx while holding
SBUS_RING_MTX(sbl, sbus_ring), creating an ABBA deadlock risk between the
two mutexes.

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