Re: [PATCH v3 net-next] octeontx2-pf: report port connector type in ethtool link ksettings

[netdev-bot+sashiko@kernel.org]

Thank you for your contribution! Sashiko AI review found 1 potential
issue(s) to consider:

Critical: 0 · High: 0 · Medium: 1 · Low: 0

- [Medium] otx2_get_link_ksettings()
  (drivers/net/ethernet/marvell/octeontx2/nic/otx2_ethtool.c:1236)…

--

octeontx2-pf: report port connector type in ethtool link ksettings

otx2_get_link_ksettings() now reads the connector type out of the CGX
firmware shared data (struct cgx_lmac_fwdata_s.port), masks the low eight
bits, checks the result against a list of ethtool PORT_* values and stores
it into cmd->base.port.  Unrecognized values become PORT_OTHER.

> diff --git a/drivers/net/ethernet/marvell/octeontx2/nic/otx2_ethtool.c b/drivers/net/ethernet/marvell/octeontx2/nic/otx2_ethtool.c
> index 9bee1b91eeaa..a05dee0085a3 100644
> --- a/drivers/net/ethernet/marvell/octeontx2/nic/otx2_ethtool.c
> +++ b/drivers/net/ethernet/marvell/octeontx2/nic/otx2_ethtool.c

[ ... ]

> @@ -1231,6 +1232,23 @@ static int otx2_get_link_ksettings(struct net_device *netdev,
>  				OTX2_MODE_SUPPORTED, cmd);
>  	otx2_get_fec_info(rsp->fwdata.supported_fec,
>  			  OTX2_MODE_SUPPORTED, cmd);
> +
> +	port = FIELD_GET(GENMASK(7, 0), rsp->fwdata.port);
> +	switch (port) {
> +	case PORT_TP:
> +	case PORT_AUI:
> +	case PORT_MII:
> +	case PORT_FIBRE:
> +	case PORT_BNC:
> +	case PORT_DA:
> +	case PORT_NONE:
> +		cmd->base.port = port;
> +		break;
> +	default:
> +		cmd->base.port = PORT_OTHER;
> +		break;
> +	}

[Severity: Medium]
Can this report a wrong connector type when firmware never populated the
field?

PORT_TP is 0x00 in include/uapi/linux/ethtool.h, and the ethtool core
zero-fills the settings before calling into the driver:

net/ethtool/ioctl.c:netif_get_link_ksettings() {
	memset(link_ksettings, 0, sizeof(*link_ksettings));
	return dev->ethtool_ops->get_link_ksettings(dev, link_ksettings);
}

So a zero firmware word takes the "case PORT_TP:" arm rather than the
default PORT_OTHER arm, and ethtool prints "Port: Twisted Pair" on a
fibre, DAC or backplane board.

Is a zero word possible here?  The member was carved out of the previously
reserved (zero-filled) tail of struct cgx_lmac_fwdata_s by commit
49f02e6877d1b, which shrank LMAC_FWDATA_RESERVED_MEM from 1021 to 1018:

drivers/net/ethernet/marvell/octeontx2/af/mbox.h:
	u64 mgmt_port:1;
	u64 advertised_an:1;
	u64 port;
#define LMAC_FWDATA_RESERVED_MEM 1018
	u64 reserved[LMAC_FWDATA_RESERVED_MEM];

RVU_FWDATA_VERSION was not bumped at the same time, and
is_rvu_fwdata_valid() in af/rvu.h only compares header_magic and version,
so firmware images that predate the field still pass the ABI gate and hand
back zeros for this word.

Would it make sense to also consult rw_valid from the same struct?  It is
the per-LMAC validity flag, and grep shows it is currently only referenced
in mbox.h and rvu_debugfs.c, never by this new consumer.  Note also that
the debugfs dumper added alongside the field prints rw_valid, lmac_type,
portm_idx and mgmt_port, but not port.

Separately, does the masking defeat part of the validation?  FIELD_GET()
with GENMASK(7, 0) drops bits 63:8 before the switch runs, so firmware
values such as 0x100 or 0x103 are published as PORT_TP and PORT_FIBRE
respectively.  mbox.h documents no bitfield layout, mask or reserved-bit
contract for this bare u64, so it isn't clear the upper bits can be
discarded rather than treated as "not a valid connector code".

Related to that, the commit message says:

  "Extract only the low 8 bits of the firmware field and validate the
   value against the standard ethtool PORT_* definitions before
   publishing it to userspace."

Given that neither the unpopulated (zero) case nor invalid values living in
bits 63:8 can be rejected by this check, could the description be made
more precise about what is actually validated?

One more question on the encoding itself: is there an in-tree statement
that fwdata.port uses ethtool PORT_* numbering?  The only other firmware
description of a "port" value is struct cgx_lnk_sts.port:8 with
RESP_LINKSTAT_PORT in af/cgx_fw_if.h, and that macro is defined but never
decoded by cgx.c, so the mapping to PORT_TP/PORT_FIBRE/... appears
undocumented here.

-- 
Sashiko AI review · https://netdev-ai.bots.linux.dev/sashiko/#/patchset/20260909040621.397255-1-rkannoth%40marvell.com