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

[Ratheesh Kannoth <rkannoth@marvell.com>]

On 2026-09-10 at 10:02:43, netdev-bot+sashiko@kernel.org (netdev-bot+sashiko@kernel.org) wrote:
> 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)…
FW is setting it properly.

>
> --
>
> 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