Re: [PATCH 10/23] net: hpe: sl: add the module entry points

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 6 potential issue(s) to consider:
- [Medium] net: hpe: sl: 64-bit cause_map truncation in sl_link_down_cause_map_masked_str()
- [Medium] net: hpe: sl: missing unsupported-cable case in sl_link_down_cause_map_masked_str()
- [Low] net: hpe: sl: undocumented module parameter serdes_core_io_trace_lgrp_map
- [Low] net: hpe: sl: SL_GIT_HASH_STR stringifies undefined GIT_HASH macro
- [Low] net: hpe: sl: missing option cases in sl_lgrp_config_opt_str() and sl_link_config_opt_str()
- [Low] net: hpe: sl: dead stub file sl_media_nic.c and uninitialized size fields
--

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

net: hpe: sl: add the module entry points

This commit adds the module entry points and the logical device, link group,
link, MAC, LLR, FEC, and media objects that make up the SL API used by the
NIC driver.

> diff --git a/drivers/net/ethernet/hpe/sl/sl_lgrp.c b/drivers/net/ethernet/hpe/sl/sl_lgrp.c
> new file mode 100644
> index 0000000000000..bb2dc68f4198a
> --- /dev/null
> +++ b/drivers/net/ethernet/hpe/sl/sl_lgrp.c
[ ... ]
> +const char *sl_lgrp_config_opt_str(u32 option)
> +{
> +	switch (option) {
> +	case SL_LGRP_CONFIG_OPT_SERDES_LOOPBACK_ENABLE:
> +		return "loopback-serdes-enable";
> +	default:
> +		return "unknown";
> +	}
> +}
> +EXPORT_SYMBOL(sl_lgrp_config_opt_str);

[Severity: Low]
In drivers/net/ethernet/hpe/sl/sl_lgrp.c, sl_lgrp_config_opt_str() only
handles SL_LGRP_CONFIG_OPT_SERDES_LOOPBACK_ENABLE, while
include/linux/hpe/sl/sl_lgrp.h also defines SL_LGRP_CONFIG_OPT_FABRIC,
SL_LGRP_CONFIG_OPT_R1, and SL_LGRP_CONFIG_OPT_8BYTE_PREAMBLE.

Similarly, in drivers/net/ethernet/hpe/sl/sl_link.c, sl_link_config_opt_str()
omits cases for SL_LINK_CONFIG_OPT_ALD_ENABLE,
SL_LINK_CONFIG_OPT_LOS_LOL_UP_FAIL_HIDE, and
SL_LINK_CONFIG_OPT_PML_REC_ENABLE defined in include/linux/hpe/sl/sl_link.h.

Should sl_lgrp_config_opt_str() and sl_link_config_opt_str() include cases
for those remaining configuration option flags so callers do not get
"unknown" for valid options?

[ ... ]
> diff --git a/drivers/net/ethernet/hpe/sl/sl_link.c b/drivers/net/ethernet/hpe/sl/sl_link.c
> new file mode 100644
> index 0000000000000..c66e48a627481
> --- /dev/null
> +++ b/drivers/net/ethernet/hpe/sl/sl_link.c
[ ... ]
> +#define SL_LINK_DOWN_INFO_MASK (     \
> +	SL_LINK_DOWN_RETRYABLE      |\
> +	SL_LINK_DOWN_ORIGIN_ASYNC   |\
> +	SL_LINK_DOWN_ORIGIN_LINK_UP)
> +
> +static int sl_link_down_cause_map_masked_str(unsigned long cause_map, char *cause_str, unsigned int cause_str_size,
> +					     u64 mask)
> +{
> +	int rtn;
> +	int str_pos;
> +	int which;
> +
> +	if (!cause_str)
> +		return -EINVAL;
> +
> +	if (cause_str_size < SL_LINK_DOWN_CAUSE_STR_SIZE_MIN)
> +		return -EINVAL;
> +
> +	cause_map &= mask;
> +
> +	if (cause_map == SL_LINK_DOWN_CAUSE_NONE) {
> +		scnprintf(cause_str, cause_str_size, "none");
> +		return 0;
> +	}
> +
> +	str_pos = 0;
> +
> +	for_each_set_bit(which, &cause_map, sizeof(cause_map) * BITS_PER_BYTE) {
> +
> +		switch (BIT(which)) {

[Severity: Medium]
In drivers/net/ethernet/hpe/sl/sl_link.c, sl_link_down_cause_map_str() and
sl_link_down_cause_map_with_info_str() pass a u64 cause_map to
sl_link_down_cause_map_masked_str(), which narrows cause_map to unsigned long
and switches on BIT(which).

In include/linux/hpe/sl/sl_link.h, the upper cause and info flags are also
defined with BIT():

  #define SL_LINK_DOWN_CAUSE_TX_LOL            BIT(32)
  #define SL_LINK_DOWN_CAUSE_RX_LOL            BIT(33)
  #define SL_LINK_DOWN_CAUSE_TX_LOS            BIT(34)
  #define SL_LINK_DOWN_CAUSE_RX_LOS            BIT(35)

  #define SL_LINK_DOWN_RETRYABLE               BIT(61)
  #define SL_LINK_DOWN_ORIGIN_ASYNC            BIT(62)
  #define SL_LINK_DOWN_ORIGIN_LINK_UP          BIT(63)

On 32-bit architectures where unsigned long is 32 bits wide, will this
truncate the upper 32 bits of cause_map and overflow BIT(which) and the
switch case labels at bit 32 and above? Should those definitions and the
switch in sl_link_down_cause_map_masked_str() use u64 and BIT_ULL() instead?

> +		case SL_LINK_DOWN_CAUSE_UCW:
> +			rtn = snprintf(cause_str + str_pos, cause_str_size - str_pos, "ucw ");
> +			break;
[ ... ]
> +		case SL_LINK_DOWN_CAUSE_TIMEOUT:
> +			rtn = snprintf(cause_str + str_pos, cause_str_size - str_pos, "timeout ");
> +			break;
> +		case SL_LINK_DOWN_CAUSE_CANCELED:
> +			rtn = snprintf(cause_str + str_pos, cause_str_size - str_pos, "canceled ");
> +			break;
> +		case SL_LINK_DOWN_CAUSE_COMMAND:
> +			rtn = snprintf(cause_str + str_pos, cause_str_size - str_pos, "command ");
> +			break;

[Severity: Medium]
In include/linux/hpe/sl/sl_link.h, SL_LINK_DOWN_CAUSE_UNSUPPORTED_CABLE is
defined as BIT(12) between SL_LINK_DOWN_CAUSE_CANCELED (BIT(11)) and
SL_LINK_DOWN_CAUSE_COMMAND (BIT(13)), and is set via
SL_LINK_DOWN_CAUSE_UNSUPPORTED_CABLE_MAP in
drivers/net/ethernet/hpe/sl/core/sl_core_link.h.

However, sl_link_down_cause_map_masked_str() in
drivers/net/ethernet/hpe/sl/sl_link.c does not have a case for
SL_LINK_DOWN_CAUSE_UNSUPPORTED_CABLE, causing it to fall through to default
and format the cause as "unknown ".

Also, SL_LINK_DOWN_CAUSE_STR_SIZE_MIN is defined as 4, which is smaller than
sizeof("none") and smaller than any single cause string before the truncation
logic writes "..." at cause_str[str_pos - 3].

Was a case for SL_LINK_DOWN_CAUSE_UNSUPPORTED_CABLE missed in
sl_link_down_cause_map_masked_str()?

[ ... ]
> diff --git a/drivers/net/ethernet/hpe/sl/sl_media_nic.c b/drivers/net/ethernet/hpe/sl/sl_media_nic.c
> new file mode 100644
> index 0000000000000..23af513e6bd90
> --- /dev/null
> +++ b/drivers/net/ethernet/hpe/sl/sl_media_nic.c
> @@ -0,0 +1,23 @@
> +// SPDX-License-Identifier: GPL-2.0
> +/* Copyright 2025 Hewlett Packard Enterprise Development LP */
> +
> +#include <linux/module.h>
> +#include <linux/types.h>
> +
> +#include <linux/hpe/sl/sl_lgrp.h>
> +
> +#include "sl_lgrp.h"
> +
> +int sl_media_cable_insert(struct sl_lgrp *lgrp, u8 *eeprom_page0, u8 *eeprom_page1, u32 flags)
> +{
> +	/* placeholder for future functionality */
> +	return 0;
> +}
> +EXPORT_SYMBOL(sl_media_cable_insert);
> +
> +int sl_media_cable_remove(struct sl_lgrp *lgrp)
> +{
> +	/* placeholder for future functionality */
> +	return 0;
> +}
> +EXPORT_SYMBOL(sl_media_cable_remove);

[Severity: Low]
This isn't a runtime bug, but drivers/net/ethernet/hpe/sl/sl_media_nic.c
defines and exports no-op stubs for sl_media_cable_insert() and
sl_media_cable_remove(), while drivers/net/ethernet/hpe/sl/nic/sl_media.c
provides the actual implementations and EXPORT_SYMBOL() definitions for both
functions, and drivers/net/ethernet/hpe/sl/Makefile only builds
nic/sl_media.o.

In addition, struct sl_lgrp and struct sl_link declare a u32 size field
alongside magic and ver just like struct sl_ldev, and sl_ldev_init() in
drivers/net/ethernet/hpe/sl/sl_ldev.c sets ldevs[ldev_num].size, whereas
sl_lgrp_init() in drivers/net/ethernet/hpe/sl/sl_lgrp.c and sl_link_init() in
drivers/net/ethernet/hpe/sl/sl_link.c leave size uninitialized.

Should drivers/net/ethernet/hpe/sl/sl_media_nic.c be dropped and the size
fields initialized (or removed if unused) for consistency?

> diff --git a/drivers/net/ethernet/hpe/sl/sl_module.c b/drivers/net/ethernet/hpe/sl/sl_module.c
> new file mode 100644
> index 0000000000000..b63efedf94ff5
> --- /dev/null
> +++ b/drivers/net/ethernet/hpe/sl/sl_module.c
[ ... ]
> +static unsigned long serdes_core_io_trace_lgrp_map;
> +module_param(serdes_core_io_trace_lgrp_map, ulong, 0644);
> +MODULE_PARM_DESC(serdes_core_io_trace_lgrp_map, "serdes core io trace enable link group map");

[Severity: Low]
In drivers/net/ethernet/hpe/sl/sl_module.c, serdes_core_io_trace_lgrp_map is
registered as a module parameter via module_param(), making it configurable
at boot and runtime, but Documentation/admin-guide/kernel-parameters.txt is
not updated in this patch.

Should serdes_core_io_trace_lgrp_map be documented in
Documentation/admin-guide/kernel-parameters.txt?

> +
> +u64 sl_serdes_core_io_trace_lgrp_map_get(void)
> +{
> +	return serdes_core_io_trace_lgrp_map;
> +}
> +
> +#define SL_DEVICE_NAME "sl"
> +
> +#define tostr(x)             #x
> +#define version_str(a, b, c) tostr(a.b.c)
> +#define SL_VERSION_STR       version_str(SL_VERSION_MAJOR, SL_VERSION_MINOR, SL_VERSION_INC)
> +#define hash_str(h)          tostr(h)
> +#define SL_GIT_HASH_STR      hash_str(GIT_HASH)
> +
> +void sl_version_get(int *major, int *minor, int *inc)
> +{
> +	if (major)
> +		*major = SL_VERSION_MAJOR;
> +	if (minor)
> +		*minor = SL_VERSION_MINOR;
> +	if (inc)
> +		*inc = SL_VERSION_INC;
> +}
> +EXPORT_SYMBOL(sl_version_get);
> +
> +char *sl_version_str_get(void)
> +{
> +	return SL_VERSION_STR;
> +}
> +
> +char *sl_git_hash_str_get(void)
> +{
> +	return (strlen(SL_GIT_HASH_STR) == 0) ? "unavailable" : SL_GIT_HASH_STR;
> +}
> +EXPORT_SYMBOL(sl_git_hash_str_get);

[Severity: Low]
In drivers/net/ethernet/hpe/sl/sl_module.c, GIT_HASH is not defined in any
header or Makefile, so SL_GIT_HASH_STR expands hash_str(GIT_HASH) into the
string literal "GIT_HASH".

As a result, strlen(SL_GIT_HASH_STR) == 0 in sl_git_hash_str_get() is never
true, sl_git_hash_str_get() returns "GIT_HASH" instead of "unavailable", and
sl_init() logs "hash GIT_HASH".

Is SL_GIT_HASH_STR / sl_git_hash_str_get() needed in an in-tree kernel build,
or should GIT_HASH default to an empty string when not defined?

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