Re: [PATCH 13/23] net: hpe: sl: add media and cable handling

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 12 potential issue(s) to consider:
- [High] net: hpe: sl: unchecked cable_db_idx and s8 -1 fw_ver format in sl_media_eeprom.c
- [High] net: hpe: sl: off-by-one memset in lgrp string getters leaks stack byte
- [Medium] net: hpe: sl: broken 800GAUI/400GAUI AppSel tracking blocks cable upshift
- [Medium] net: hpe: sl: false shift state in sl_media_data_jack_cable_hw_shift_state_get()
- [Medium] net: hpe: sl: uninitialized port_count in sl_media_lgrp_cable_temp_flags_clr()
- [Medium] net: hpe: sl: state corruption and error handling bugs in sl_media_jack_cable_insert()
- [Medium] net: hpe: sl: sl_media_lgrp_media_has_error() ignores headshell fault bit
- [Medium] net: hpe: sl: missing data_lock around serdes_settings and eeprom pages
- [Low] net: hpe: sl: 32-byte scan overreads 16-byte EEPROM vendor field in sl_media_eeprom_vendor_get()
- [Low] net: hpe: sl: truncation ellipsis overwrites prior token in sl_media_jack_attr_error_map_str()
- [Low] net: hpe: sl: initialize __free() variables at declaration in sl_media_jack.c
- [Low] net: hpe: sl: duplicate static cable_db table across translation units
--

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

net: hpe: sl: add media and cable handling

This commit adds cable and media handling for the HPE Slingshot driver,
including EEPROM parsing, the cable database, per-jack data management, and
emulated jack support.

> diff --git a/drivers/net/ethernet/hpe/sl/media/base/sl_media_eeprom.c b/drivers/net/ethernet/hpe/sl/media/base/sl_media_eeprom.c
> new file mode 100644
> index 0000000000000..80113ec4860d3
> --- /dev/null
> +++ b/drivers/net/ethernet/hpe/sl/media/base/sl_media_eeprom.c
> @@ -0,0 +1,611 @@
[ ... ]
> +#include "sl_media_eeprom.h"
> +#include "sl_media_lgrp.h"
> +#include "sl_media_jack.h"
> +#include "base/sl_media_log.h"
> +#include "data/sl_media_data_cable_db.h"

[Severity: Low]
This isn't a bug, but data/sl_media_data_cable_db.h defines a 327-entry
(~21 KB) non-const static array:

  static struct sl_media_cable_attr cable_db[] = { ... };

Including data/sl_media_data_cable_db.h and indexing cable_db[] in both
sl_media_eeprom.c and sl_media_data_cable_db_ops.c (as well as pulling it in
via sl_ctrl_ldev.h) instantiates separate mutable copies of the table across
translation units. Could cable_db be defined once as const in a single .c
file with its declaration or size exposed in the header?

> +
> +#define LOG_NAME SL_MEDIA_EEPROM_LOG_NAME
> +
> +static void sl_media_eeprom_appsel_info_store(struct sl_media_jack *media_jack, u8 host_interface,
> +					      unsigned long *speeds_map, u8 appsel_num, u8 lane_count)
> +{
> +	switch (host_interface) {
[ ... ]
> +	case SL_MEDIA_SS2_HOST_INTERFACE_400GAUI_4_S_C2M:
> +		*speeds_map |= SL_MEDIA_SPEEDS_SUPPORT_CK_400G;
> +		if (media_jack->appsel_num_400_gaui) /* we prefer 4_l_c2m over 4_s_c2m for default ck400G speed */
> +			break;
> +		media_jack->appsel_num_400_gaui = appsel_num;
> +		media_jack->lane_count_400_gaui = lane_count;
> +		media_jack->host_interface_400_gaui = host_interface;
> +		break;
> +	case SL_MEDIA_SS1_HOST_INTERFACE_400GAUI_4_L_C2M:
> +		*speeds_map |= SL_MEDIA_SPEEDS_SUPPORT_CK_400G;
> +		media_jack->appsel_num_400_gaui = appsel_num;
> +		media_jack->lane_count_400_gaui = lane_count;
> +		media_jack->host_interface_400_gaui = host_interface;
> +		fallthrough;
[ ... ]
> +	case SL_MEDIA_SS2_HOST_INTERFACE_800GAUI_8_S_C2M:
> +		*speeds_map |= SL_MEDIA_SPEEDS_SUPPORT_CK_400G;
> +		*speeds_map |= SL_MEDIA_SPEEDS_SUPPORT_CK_800G;
> +		if (!media_jack->host_interface_400_gaui) {
> +			media_jack->appsel_num_400_gaui = appsel_num;
> +			media_jack->host_interface_400_gaui = host_interface;
> +		}
> +		break;

[Severity: Medium]
Can an active cable advertising SL_MEDIA_SS2_HOST_INTERFACE_800GAUI_8_S_C2M
ever upshift back to 400G after downshifting to 200G?

When SL_MEDIA_SS2_HOST_INTERFACE_800GAUI_8_S_C2M is parsed first in
sl_media_eeprom_appsel_info_store(), it sets media_jack->appsel_num_400_gaui
and media_jack->host_interface_400_gaui, leaving
media_jack->lane_count_400_gaui as 0. If a subsequent AppSel entry is
SL_MEDIA_SS2_HOST_INTERFACE_400GAUI_4_S_C2M, the check:

  if (media_jack->appsel_num_400_gaui)
      break;

skips recording the 400GAUI_4_S_C2M descriptor because appsel_num_400_gaui is
already non-zero.

Then in sl_media_jack_cable_shift_checks() in sl_media_jack.c,
SL_MEDIA_JACK_CABLE_DOWNSHIFT only checks that
media_jack->host_interface_400_gaui != 0, which succeeds, whereas
SL_MEDIA_JACK_CABLE_UPSHIFT requires:

  if (((media_lgrp->media_jack->host_interface_400_gaui != SL_MEDIA_SS2_HOST_INTERFACE_400GAUI_4_S_C2M) &&
       (media_lgrp->media_jack->host_interface_400_gaui != SL_MEDIA_SS1_HOST_INTERFACE_400GAUI_4_L_C2M)) ||
      (media_lgrp->media_jack->lane_count_400_gaui != 0x44))

which fails on both host_interface_400_gaui and lane_count_400_gaui, leaving
the cable unable to upshift back to 400G.

[ ... ]
> +#define CMIS_VENDOR_OFFSET      129
> +#define SFF8436_VENDOR_OFFSET   148
> +#define VENDOR_NAME_MAX_LEN     32
> +struct vendor {
> +	char name[VENDOR_NAME_MAX_LEN];
> +	u32  type;
> +};
[ ... ]
> +static int sl_media_eeprom_vendor_get(struct sl_media_jack *media_jack, u8 format, u32 *vendor)
> +{
> +	const char *vendor_ptr;
> +	int         x;
> +
> +	if (format == SL_MEDIA_MGMT_IF_CMIS)
> +		vendor_ptr = &(media_jack->eeprom_page0[CMIS_VENDOR_OFFSET]);
> +	else
> +		vendor_ptr = &(media_jack->eeprom_page0[SFF8436_VENDOR_OFFSET]);
> +
> +	for (x = 0; x < ARRAY_SIZE(vendor_list); ++x) {
> +		if (strnstr(vendor_ptr, vendor_list[x].name, VENDOR_NAME_MAX_LEN) == NULL)
> +			continue;
> +		*vendor = vendor_list[x].type;
> +		sl_media_log_dbg(media_jack, LOG_NAME,
> +				 "vendor get (vendor = %u %s)", *vendor, sl_media_vendor_str(*vendor));
> +		return 0;
> +	}
> +
> +	*vendor = 0;

[Severity: Low]
In both CMIS (bytes 129..144) and SFF-8436/8636 (bytes 148..163), the EEPROM
vendor name field is 16 bytes of space-padded ASCII without a NUL terminator.
Passing VENDOR_NAME_MAX_LEN (32) to strnstr() in sl_media_eeprom_vendor_get()
scans 16 bytes past the end of the vendor field into the adjacent OUI and
vendor part number fields in eeprom_page0.

Also, when no entry in vendor_list matches, should *vendor be set to
SL_MEDIA_VENDOR_UNKNOWN rather than 0 (which sl_media_vendor_str() reports as
"invalid")?

[ ... ]
> +bool sl_media_eeprom_is_fw_version_supported(struct sl_media_jack *media_jack, struct sl_media_attr *media_attr)
> +{
> +	sl_media_log_dbg(media_jack, LOG_NAME,
> +			 "is fw version supported (type = 0x%X %s, shape = %u %s, end = %u %s, idx = %d)",
> +			 media_attr->type, sl_media_type_str(media_attr->type),
> +			 media_attr->shape, sl_media_shape_str(media_attr->shape),
> +			 media_jack->cable_end, sl_media_cable_end_str(media_jack->cable_end),
> +			 media_jack->cable_db_idx);
> +	sl_media_log_dbg(media_jack, LOG_NAME,
> +			 "is fw version supported (fw_ver = %02X.%02X, db_fw_ver = %02X.%02X, db_split_fw_ver = %02X.%02X)",
> +			 media_attr->fw_ver[0], media_attr->fw_ver[1],
> +			 cable_db[media_jack->cable_db_idx].fw_ver.major,
> +			 cable_db[media_jack->cable_db_idx].fw_ver.minor,
> +			 cable_db[media_jack->cable_db_idx].fw_ver.split_major,
> +			 cable_db[media_jack->cable_db_idx].fw_ver.split_minor);
> +
> +	if (media_attr->shape == SL_MEDIA_SHAPE_SPLITTER && media_jack->cable_end != SL_MEDIA_CABLE_END_DD)
> +		return ((media_attr->fw_ver[0] > cable_db[media_jack->cable_db_idx].fw_ver.split_major) ||
> +			((media_attr->fw_ver[0] == cable_db[media_jack->cable_db_idx].fw_ver.split_major) &&
> +			(media_attr->fw_ver[1] >= cable_db[media_jack->cable_db_idx].fw_ver.split_minor)));
> +
> +	return ((media_attr->fw_ver[0] > cable_db[media_jack->cable_db_idx].fw_ver.major) ||
> +		((media_attr->fw_ver[0] == cable_db[media_jack->cable_db_idx].fw_ver.major) &&
> +		(media_attr->fw_ver[1] >= cable_db[media_jack->cable_db_idx].fw_ver.minor)));
> +}
> +
> +int sl_media_eeprom_target_fw_ver_str_get(struct sl_media_jack *media_jack, char *target_fw_str, size_t target_fw_size)
> +{
[ ... ]
> +	if (media_jack->cable_info[0].media_attr.shape == SL_MEDIA_SHAPE_SPLITTER &&
> +	    media_jack->cable_end != SL_MEDIA_CABLE_END_DD) {
> +		snprintf(target_fw_str, target_fw_size, "%02X.%02X\n",
> +			 cable_db[media_jack->cable_db_idx].fw_ver.split_major,
> +			 cable_db[media_jack->cable_db_idx].fw_ver.split_minor);
> +		return 0;
> +	}
> +
> +	snprintf(target_fw_str, target_fw_size, "%02X.%02X\n",
> +		 cable_db[media_jack->cable_db_idx].fw_ver.major,
> +		 cable_db[media_jack->cable_db_idx].fw_ver.minor);
> +
> +	return 0;
> +}

[Severity: High]
Can sl_media_eeprom_target_fw_ver_str_get() and
sl_media_eeprom_is_fw_version_supported() read out-of-bounds at cable_db[-1]
or format signed -1 firmware versions as "FFFFFFFF.FFFFFFFF"?

First, media_jack->cable_db_idx is initialized and cleared to -1 in
sl_media_data_jack_new() and sl_media_data_jack_cable_state_clr(). Unlike
sl_media_data_cable_db_ops_serdes_settings_get(), which checks:

  if (media_jack->cable_db_idx < 0 || media_jack->cable_db_idx >= ARRAY_SIZE(cable_db))

neither sl_media_eeprom_target_fw_ver_str_get() nor
sl_media_eeprom_is_fw_version_supported() validates media_jack->cable_db_idx
before indexing cable_db[media_jack->cable_db_idx], and
sl_media_eeprom_target_fw_ver_str_get() does not hold media_jack->data_lock.
If target_firmware_version_hex_show() calls
sl_media_eeprom_target_fw_ver_str_get() while a fake active cable is set,
concurrently with cable insert/remove, or after a duplicate
sl_media_jack_cable_insert() call resets cable_db_idx to -1, this will index
cable_db[-1].

Second, in struct sl_media_cable_attr, fw_ver.major, fw_ver.minor,
fw_ver.split_major, and fw_ver.split_minor are signed s8 fields set to -1 in
cable_db for active cables that have no target firmware version (for example,
several SL_MEDIA_TYPE_AEC and non-ss200 SL_MEDIA_TYPE_POC entries). When
passed to snprintf() with "%02X.%02X\n", -1 is sign-extended to 0xFFFFFFFF
and printed as "FFFFFFFF.FFFFFFFF\n".

> diff --git a/drivers/net/ethernet/hpe/sl/media/data/nic/sl_media_data_jack.c b/drivers/net/ethernet/hpe/sl/media/data/nic/sl_media_data_jack.c
> new file mode 100644
> index 0000000000000..9d46b3f13de2c
> --- /dev/null
> +++ b/drivers/net/ethernet/hpe/sl/media/data/nic/sl_media_data_jack.c
> @@ -0,0 +1,496 @@
[ ... ]
> +int sl_media_data_jack_cable_hw_shift_state_get(struct sl_media_jack *media_jack)
> +{
> +	int rtn;
> +	int i;
> +	u8  downshift_lower_lane_config;
> +	u8  upshift_lower_lane_config;
> +	u8  read_data[4];
> +
> +	sl_media_log_dbg(media_jack, LOG_NAME, "data jack cable hw shift state get");
> +
> +	downshift_lower_lane_config = (media_jack->appsel_num_200_gaui << 4) | DATA_PATH_LOWER_LANE_CONFIG;
> +	upshift_lower_lane_config = (media_jack->appsel_num_400_gaui << 4) | DATA_PATH_LOWER_LANE_CONFIG;
> +
> +	for (i = 0; i < 4; ++i) {
> +		rtn = sl_media_io_read8(media_jack, 0x10, 145 + i, &read_data[i]);
> +		if (rtn) {
> +			sl_media_log_err_trace(media_jack, LOG_NAME,
> +					       "SCS0 configuration - config lanes 1-4 - read failed [%d]", rtn);
> +			return SL_MEDIA_JACK_CABLE_HW_SHIFT_IO_ERROR;
> +		}
> +	}
> +
> +	if ((read_data[0] == downshift_lower_lane_config) &&
> +	    (read_data[1] == downshift_lower_lane_config) &&
> +	    (read_data[2] == downshift_lower_lane_config) &&
> +	    (read_data[3] == downshift_lower_lane_config)) {
> +		sl_media_log_dbg(media_jack, LOG_NAME, "cable is downshifted");
> +		return SL_MEDIA_JACK_CABLE_HW_SHIFT_STATE_DOWNSHIFTED;
> +	} else if ((read_data[0] == upshift_lower_lane_config) &&
> +		   (read_data[1] == upshift_lower_lane_config) &&
> +		   (read_data[2] == upshift_lower_lane_config) &&
> +		   (read_data[3] == upshift_lower_lane_config)) {
> +		sl_media_log_dbg(media_jack, LOG_NAME, "cable is upshifted");
> +		return SL_MEDIA_JACK_CABLE_HW_SHIFT_STATE_UPSHIFTED;
> +	}
> +
> +	sl_media_log_dbg(media_jack, LOG_NAME, "cable is in unknown shift state");
> +	return SL_MEDIA_JACK_CABLE_HW_SHIFT_STATE_UNKNOWN;
> +}

[Severity: Medium]
What happens in sl_media_data_jack_cable_hw_shift_state_get() when an active
cable does not advertise a 200GAUI-4 or 400GAUI-4 AppSel entry?

Because DATA_PATH_LOWER_LANE_CONFIG is 0x00, if media_jack->appsel_num_200_gaui
(or appsel_num_400_gaui) is 0, downshift_lower_lane_config (or
upshift_lower_lane_config) evaluates to 0x00. When SCS0 bytes 145..148 on page
0x10 read as 0x00 (unconfigured after soft reset), read_data[0..3] matches
0x00 and reports SL_MEDIA_JACK_CABLE_HW_SHIFT_STATE_DOWNSHIFTED (or
UPSHIFTED).

During sl_media_jack_cable_insert(), this marks the cable as downshifted,
causing sl_core_hw_serdes_link_up_settings() to disable SerDes scrambling and
sl_media_jack_cable_downshift() to return early before running
sl_media_jack_cable_shift_checks(). Should
sl_media_data_jack_cable_hw_shift_state_get() check that
appsel_num_200_gaui / appsel_num_400_gaui are non-zero before matching?

> diff --git a/drivers/net/ethernet/hpe/sl/media/data/sl_media_data_jack.c b/drivers/net/ethernet/hpe/sl/media/data/sl_media_data_jack.c
> new file mode 100644
> index 0000000000000..17631509afa22
> --- /dev/null
> +++ b/drivers/net/ethernet/hpe/sl/media/data/sl_media_data_jack.c
> @@ -0,0 +1,344 @@
[ ... ]
> +void sl_media_data_cable_serdes_settings_clr(struct sl_media_jack *media_jack)
> +{
> +	sl_media_log_dbg(media_jack, LOG_NAME, "serdes settings clr");
> +
> +	memset(&(media_jack->serdes_settings), 0, sizeof(struct sl_media_serdes_settings));
> +}
> +
> +void sl_media_data_jack_eeprom_clr(struct sl_media_jack *media_jack)
> +{
> +	sl_media_log_dbg(media_jack, LOG_NAME, "eeprom clr");
> +
> +	memset(media_jack->eeprom_page0, 0, SL_MEDIA_EEPROM_PAGE_SIZE);
> +	memset(media_jack->eeprom_page1, 0, SL_MEDIA_EEPROM_PAGE_SIZE);
> +}

[Severity: Medium]
Should media_jack->serdes_settings, eeprom_page0, and eeprom_page1 be
protected by media_jack->data_lock?

In sl_media_eeprom.c, sl_media_eeprom_media_interface_get() and
sl_media_eeprom_supported_flags_advertised_get() read eeprom_page0 and
eeprom_page1 under spin_lock(&media_jack->data_lock), and all other getters in
sl_media_lgrp.c acquire media_jack->data_lock. However,
sl_media_data_jack_eeprom_clr(), sl_media_data_cable_serdes_settings_clr(),
sl_media_data_cable_db_ops_serdes_settings_get(), and
sl_media_lgrp_media_serdes_settings_get() read and write these fields without
holding media_jack->data_lock, which can result in torn reads of
struct sl_media_serdes_settings or eeprom_page0 if link bring-up work or sysfs
reads run concurrently with cable insert/remove.

> diff --git a/drivers/net/ethernet/hpe/sl/media/nic/sl_media_jack.c b/drivers/net/ethernet/hpe/sl/media/nic/sl_media_jack.c
> new file mode 100644
> index 0000000000000..33f1a2a20ab78
> --- /dev/null
> +++ b/drivers/net/ethernet/hpe/sl/media/nic/sl_media_jack.c
> @@ -0,0 +1,315 @@
[ ... ]
> +static void sl_media_jack_cable_attr_errors_update(struct sl_media_jack *media_jack, u32 errors)
> +{
> +	sl_media_log_dbg(media_jack, LOG_NAME, "cable attr errors update");
> +
> +	/*
> +	 * only first element is valid in cable_info since single lgrp on nic
> +	 */
> +	media_jack->cable_info[0].media_attr.errors |= errors;
> +}
[ ... ]
> +int sl_media_jack_cable_insert(u8 ldev_num, u8 lgrp_num, u8 jack_num,
> +			       u8 *eeprom_page0, u8 *eeprom_page1, u32 flags)
> +{
> +	int                   rtn;
> +	struct sl_media_attr  media_attr;
> +	struct sl_media_jack *media_jack;
> +
> +	media_jack = sl_media_data_jack_get(ldev_num, jack_num);
> +	if (!media_jack) {
> +		sl_media_log_err(NULL, LOG_NAME, "media_data_jack_get failed");
> +		return -EFAULT;
> +	}
> +
> +	sl_media_log_dbg(media_jack, LOG_NAME, "cable insert");
> +
> +	/* Start clean so nothing from the previous cable is inherited */
> +	sl_media_data_jack_cable_state_clr(media_jack);
> +
> +	sl_media_jack_fault_cause_set(media_jack, SL_MEDIA_FAULT_CAUSE_NONE);
> +
> +	memset(&media_attr, 0, sizeof(struct sl_media_attr));
> +
> +	media_attr.magic    = SL_MEDIA_ATTR_MAGIC;
> +	media_attr.ver      = SL_MEDIA_ATTR_VER;
> +	media_attr.size     = sizeof(struct sl_media_attr);
> +	media_attr.errors   = 0;
> +	media_attr.info     = 0;
> +
> +	if (flags & SL_MEDIA_TYPE_BACKPLANE) {
[ ... ]
> +	} else {
> +		memcpy(media_jack->eeprom_page0, eeprom_page0, SL_MEDIA_EEPROM_PAGE_SIZE);
> +
> +		sl_media_data_jack_eeprom_page1_get(media_jack, eeprom_page1);
> +
> +		rtn = sl_media_eeprom_format_get(media_jack, &(media_attr.format));
> +		if (rtn) {
> +			sl_media_log_warn_trace(media_jack, LOG_NAME, "eeprom format unsupported");
> +			memset(&media_attr, 0, sizeof(struct sl_media_attr));
> +			media_attr.errors |= SL_MEDIA_ERROR_CABLE_FORMAT_UNSUPPORTED;
> +			media_attr.errors |= SL_MEDIA_ERROR_TRYABLE;
> +			rtn = sl_media_jack_cable_attr_set(media_jack, ldev_num, lgrp_num, &media_attr);
> +			if (rtn)
> +				sl_media_log_err_trace(media_jack, LOG_NAME, "cable attr set failed [%d]", rtn);
> +			sl_media_data_jack_cable_attr_send(media_jack);
> +			return 0;
> +		}
[ ... ]
> +	}
> +
> +	rtn = sl_media_jack_cable_attr_set(media_jack, ldev_num, lgrp_num, &media_attr);
> +	if (rtn) {
> +		sl_media_log_err_trace(media_jack, LOG_NAME, "cable attr set failed [%d]", rtn);
> +		return 0;
> +	}
> +
> +	if (media_jack->is_cable_unsupported)
> +		flags |= SL_MEDIA_TYPE_UNSUPPORTED;
> +	if (media_attr.vendor == SL_MEDIA_VENDOR_MULTILANE)
> +		flags |= SL_MEDIA_TYPE_LOOPBACK;
> +	rtn = sl_media_data_cable_db_ops_serdes_settings_get(media_jack, media_attr.type, flags);
> +	if (rtn) {
> +		sl_media_log_err_trace(media_jack, LOG_NAME, "serdes settings get failed [%d]", rtn);
> +		sl_media_jack_fault_cause_set(media_jack, SL_MEDIA_FAULT_CAUSE_SERDES_SETTINGS_GET);
> +		sl_media_data_jack_cable_attr_send(media_jack);
> +		return 0;
> +	}
> +
> +	if (SL_MEDIA_LGRP_MEDIA_TYPE_IS_ACTIVE(media_attr.type)) {
> +		rtn = sl_media_data_jack_cable_soft_reset(media_jack);
> +		if (rtn) {
> +			sl_media_log_err_trace(media_jack, LOG_NAME, "cable soft reset failed [%d]", rtn);
> +			sl_media_jack_state_set(media_jack, SL_MEDIA_JACK_CABLE_ERROR);
> +			sl_media_jack_cable_attr_errors_update(media_jack, SL_MEDIA_ERROR_CABLE_HEADSHELL_FAULT);
> +			sl_media_data_jack_cable_attr_send(media_jack);
> +			return rtn;
> +		}
> +
> +		/* must be last in the sequence because it needs time to load firmware */
> +		rtn = sl_media_jack_cable_high_power_set(ldev_num, jack_num);
> +		if (rtn) {
> +			sl_media_log_err_trace(media_jack, LOG_NAME, "cable high power set failed [%d]", rtn);
> +			sl_media_jack_state_set(media_jack, SL_MEDIA_JACK_CABLE_ERROR);
> +			sl_media_jack_cable_attr_errors_update(media_jack, SL_MEDIA_ERROR_CABLE_HEADSHELL_FAULT);
> +			sl_media_data_jack_cable_attr_send(media_jack);
> +			return 0;
> +		}
> +	}
> +
> +	rtn = sl_sysfs_media_speeds_create(ldev_num, lgrp_num);
> +	if (rtn) {
> +		sl_media_log_err_trace(media_jack, LOG_NAME, "media speeds create failed [%d]", rtn);
> +		sl_media_data_jack_cable_attr_send(media_jack);
> +		return rtn;
> +	}

[Severity: Medium]
There appear to be several state-handling issues in
sl_media_jack_cable_insert():

1. When sl_media_eeprom_format_get() fails, memset(&media_attr, 0, ...) wipes
   out media_attr.magic, media_attr.ver, and media_attr.size that were set
   earlier, and returns 0 without setting serdes_settings or updating
   media_jack->state from SL_MEDIA_JACK_CABLE_REMOVED (even though
   SL_MEDIA_ERROR_TRYABLE is set).

2. sl_media_data_jack_cable_state_clr(media_jack) is called at the start of
   sl_media_jack_cable_insert(), and eeprom_page0/page1, cable_db_idx, and
   appsel_* are overwritten before sl_media_jack_cable_attr_set() checks
   whether real_cable_status == CABLE_MEDIA_ATTR_ADDED. If a cable is already
   inserted, the existing cable's state on media_jack is clobbered and the
   -EEXIST error from sl_media_jack_cable_attr_set() is swallowed by returning
   0.

3. Conversely, after sl_media_jack_cable_attr_set() sets real_cable_status to
   CABLE_MEDIA_ATTR_ADDED, if sl_media_data_jack_cable_soft_reset() or
   sl_sysfs_media_speeds_create() fails, sl_media_jack_cable_insert() returns
   a non-zero error while leaving real_cable_status as CABLE_MEDIA_ATTR_ADDED,
   which causes subsequent sl_media_jack_cable_insert() calls to fail with
   -EEXIST.

4. sl_media_jack_cable_attr_errors_update() modifies
   media_jack->cable_info[0].media_attr.errors without holding
   media_jack->data_lock, and does not update stashed_media_attr.errors when
   real_cable_status is CABLE_MEDIA_ATTR_STASHED.

Could sl_media_jack_cable_insert() check for an existing cable before clearing
jack state, preserve magic/ver/size on format error, and keep
real_cable_status consistent on error returns?

> diff --git a/drivers/net/ethernet/hpe/sl/media/sl_media_jack.c b/drivers/net/ethernet/hpe/sl/media/sl_media_jack.c
> new file mode 100644
> index 0000000000000..14be1d19c097e
> --- /dev/null
> +++ b/drivers/net/ethernet/hpe/sl/media/sl_media_jack.c
> @@ -0,0 +1,1067 @@
[ ... ]
> +int sl_media_jack_cable_downshift(u8 ldev_num, u8 lgrp_num, u8 link_num)
> +{
> +	int                   rtn;
> +	struct sl_media_lgrp *media_lgrp;
> +	struct sl_core_link *core_link __free(sl_core_link_put) = NULL;
> +	struct sl_media_attr  media_attr;
> +
> +	media_lgrp = sl_media_lgrp_get(ldev_num, lgrp_num);
> +	core_link = sl_core_link_get(ldev_num, lgrp_num, link_num);
> +	if (!core_link)
> +		return -ENODEV;
[ ... ]
> +int sl_media_jack_cable_upshift(u8 ldev_num, u8 lgrp_num, u8 link_num)
> +{
> +	int                   rtn;
> +	struct sl_media_lgrp *media_lgrp;
> +	struct sl_core_link *core_link __free(sl_core_link_put) = NULL;
> +	struct sl_media_attr  media_attr;
> +
> +	media_lgrp = sl_media_lgrp_get(ldev_num, lgrp_num);
> +	core_link = sl_core_link_get(ldev_num, lgrp_num, link_num);
> +	if (!core_link)
> +		return -ENODEV;

[Severity: Low]
This isn't a bug, but per the kernel cleanup.h conventions, could core_link
in sl_media_jack_cable_downshift() and sl_media_jack_cable_upshift() be
defined and initialized in a single statement rather than declared as "= NULL"
and assigned later?

[ ... ]
> +int sl_media_jack_attr_error_map_str(unsigned long error_map, char *error_str, unsigned int error_str_size)
> +{
[ ... ]
> +		if (str_pos + rtn >= error_str_size) {
> +			error_str[str_pos - 2] = '.';
> +			error_str[str_pos - 3] = '.';
> +			error_str[str_pos - 4] = '.';
> +			break;
> +		}
> +		str_pos += rtn;
> +	}
> +
> +out:
> +	error_str[str_pos - 1] = '\0';
> +
> +	return 0;
> +}

[Severity: Low]
When str_pos + rtn >= error_str_size in sl_media_jack_attr_error_map_str(),
does writing to error_str[str_pos - 2..str_pos - 4] and NUL-terminating at
error_str[str_pos - 1] overwrite the end of the preceding token instead of
placing the ellipsis at the end of error_str (unlike
sl_link_down_cause_map_masked_str() in sl_link.c, which sets
str_pos = cause_str_size - 1 first)?

> diff --git a/drivers/net/ethernet/hpe/sl/media/sl_media_lgrp.c b/drivers/net/ethernet/hpe/sl/media/sl_media_lgrp.c
> new file mode 100644
> index 0000000000000..b15805a822702
> --- /dev/null
> +++ b/drivers/net/ethernet/hpe/sl/media/sl_media_lgrp.c
> @@ -0,0 +1,498 @@
[ ... ]
> +bool sl_media_lgrp_media_has_error(struct sl_media_lgrp *media_lgrp)
> +{
> +	struct sl_media_attr media_attr;
> +
> +	sl_media_lgrp_media_attr_get(media_lgrp->media_ldev->num, media_lgrp->num, &media_attr);
> +
> +	sl_media_log_dbg(media_lgrp, SL_MEDIA_LGRP_LOG_NAME,
> +			 "media has error (media_attr.errors = 0x%X)", media_attr.errors);
> +
> +	return ((media_attr.errors & (SL_MEDIA_ERROR_CABLE_FORMAT_UNSUPPORTED |
> +				      SL_MEDIA_ERROR_CABLE_FW_UNSUPPORTED)) != 0);
> +}

[Severity: Medium]
Should sl_media_lgrp_media_has_error() also check for
SL_MEDIA_ERROR_CABLE_HEADSHELL_FAULT?

In sl_media_jack_cable_insert(), when sl_media_jack_cable_high_power_set()
fails, it sets media_jack->state to SL_MEDIA_JACK_CABLE_ERROR, sets
SL_MEDIA_ERROR_CABLE_HEADSHELL_FAULT in media_attr.errors, and returns 0.
Because sl_media_lgrp_media_has_error() does not test
SL_MEDIA_ERROR_CABLE_HEADSHELL_FAULT, sl_core_hw_link_media_check() treats a
cable with a headshell fault as having no media error during link bring-up.

[ ... ]
> +void sl_media_lgrp_cable_temp_flags_clr(u8 ldev_num, u8 lgrp_num)
> +{
> +	u8                   i;
> +	struct sl_media_lgrp *media_lgrp;
> +
> +	media_lgrp = sl_media_lgrp_get(ldev_num, lgrp_num);
> +
> +	sl_media_log_dbg(media_lgrp, SL_MEDIA_LGRP_LOG_NAME, "cable temp flags clr");
> +
> +	for (i = 0; i < media_lgrp->media_jack->port_count; ++i) {
> +		if (sl_media_lgrp_media_type_is_active(ldev_num, lgrp_num)) {
> +			sl_media_lgrp_hot_client_ready_set(ldev_num, lgrp_num, false);
> +			sl_media_jack_cable_hot_notif_sent_set(media_lgrp->media_jack,
> +							       &media_lgrp->media_jack->cable_info[i], false);
> +			sl_media_lgrp_warm_client_ready_set(ldev_num, lgrp_num, false);
> +			sl_media_jack_cable_warm_notif_sent_set(media_lgrp->media_jack,
> +								&media_lgrp->media_jack->cable_info[i], false);
> +			sl_media_lgrp_cold_client_ready_set(ldev_num, lgrp_num, false);
> +			sl_media_jack_cable_cold_notif_sent_set(media_lgrp->media_jack,
> +								&media_lgrp->media_jack->cable_info[i], false);
> +		}
> +	}
> +}

[Severity: Medium]
Does the loop in sl_media_lgrp_cable_temp_flags_clr() ever execute?

Neither sl_media_data_jack_new() nor sl_media_data_jack_scan() initializes
media_jack->port_count, so it remains 0 and none of the client_ready or
notif_sent flags are cleared when sl_ctrl_lgrp_notif_callback_unreg() calls
sl_media_lgrp_cable_temp_flags_clr(). Also, if an active cable is removed
before the client unregisters its notification callback, gating the cleanup on
sl_media_lgrp_media_type_is_active() will leave hot_client_ready,
warm_client_ready, and cold_client_ready set.

[ ... ]
> +int sl_media_lgrp_vendor_pn_str_get(struct sl_media_lgrp *media_lgrp, char *vendor_pn_str)
> +{
> +	spin_lock(&media_lgrp->media_jack->data_lock);
> +	switch (media_lgrp->cable_info->real_cable_status) {
> +	case CABLE_MEDIA_ATTR_ADDED:
> +		strscpy_pad(vendor_pn_str,
> +			    media_lgrp->cable_info->media_attr.vendor_pn_str, SL_MEDIA_VENDOR_PN_SIZE);
> +		break;
> +	case CABLE_MEDIA_ATTR_STASHED:
> +		strscpy_pad(vendor_pn_str,
> +			    media_lgrp->cable_info->stashed_media_attr.vendor_pn_str, SL_MEDIA_VENDOR_PN_SIZE);
> +		break;
> +	default:
> +		memset(vendor_pn_str, '0', SL_MEDIA_VENDOR_PN_SIZE - 2);
> +		vendor_pn_str[SL_MEDIA_VENDOR_PN_SIZE - 1] = '\0';
> +	}
> +	spin_unlock(&media_lgrp->media_jack->data_lock);
> +
> +	return 0;
> +}
[ ... ]
> +int sl_media_lgrp_hpe_pn_str_get(struct sl_media_lgrp *media_lgrp, char *hpe_pn_str)
> +{
> +	spin_lock(&media_lgrp->media_jack->data_lock);
> +	switch (media_lgrp->cable_info->real_cable_status) {
> +	case CABLE_MEDIA_ATTR_ADDED:
> +		strscpy_pad(hpe_pn_str,
> +			    media_lgrp->cable_info->media_attr.hpe_pn_str, SL_MEDIA_HPE_PN_SIZE);
> +		break;
> +	case CABLE_MEDIA_ATTR_STASHED:
> +		strscpy_pad(hpe_pn_str,
> +			    media_lgrp->cable_info->stashed_media_attr.hpe_pn_str, SL_MEDIA_HPE_PN_SIZE);
> +		break;
> +	default:
> +		memset(hpe_pn_str, '0', SL_MEDIA_HPE_PN_SIZE - 2);
> +		hpe_pn_str[SL_MEDIA_HPE_PN_SIZE - 1] = '\0';
> +	}
> +	spin_unlock(&media_lgrp->media_jack->data_lock);
> +
> +	return 0;
> +}
[ ... ]
> +int sl_media_lgrp_date_code_str_get(struct sl_media_lgrp *media_lgrp, char *date_code_str)
> +{
> +	spin_lock(&media_lgrp->media_jack->data_lock);
> +	switch (media_lgrp->cable_info->real_cable_status) {
> +	case CABLE_MEDIA_ATTR_ADDED:
> +		strscpy_pad(date_code_str,
> +			    media_lgrp->cable_info->media_attr.date_code_str, SL_MEDIA_DATE_CODE_SIZE);
> +		break;
> +	case CABLE_MEDIA_ATTR_STASHED:
> +		strscpy_pad(date_code_str,
> +			    media_lgrp->cable_info->stashed_media_attr.date_code_str, SL_MEDIA_DATE_CODE_SIZE);
> +		break;
> +	default:
> +		memset(date_code_str, '0', SL_MEDIA_DATE_CODE_SIZE - 2);
> +		date_code_str[SL_MEDIA_DATE_CODE_SIZE - 1] = '\0';
> +	}
> +	spin_unlock(&media_lgrp->media_jack->data_lock);
> +
> +	return 0;
> +}

[Severity: High]
In the default branches of sl_media_lgrp_vendor_pn_str_get(),
sl_media_lgrp_hpe_pn_str_get(), and sl_media_lgrp_date_code_str_get(), calling
memset(..., '0', SIZE - 2) fills indices 0 through SIZE - 3 and then sets
index SIZE - 1 to '\0', leaving the byte at index SIZE - 2 uninitialized.

When vendor_pn_show(), hpe_pn_show(), or date_code_show() in sl_sysfs_media.c
passes an uninitialized stack buffer and prints the string with sysfs_emit(),
doesn't this leak an uninitialized stack byte at index SIZE - 2 to userspace
(unlike sl_media_lgrp_serial_num_str_get(), which uses
SL_MEDIA_SERIAL_NUM_SIZE - 1)?

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