Re: [PATCH net-next v19 08/10] netlink: specs: Add ethernet port listing with ethtool

[netdev-bot+sashiko@kernel.org]

Thank you for your contribution! Sashiko AI review found more severe
issue(s) in other patches of this series. This mail is only an FYI, in case
you want to address the nit picks below when you respin.

Low priority / nit picks for your consideration:

- [Low] Series-ordering note only, not a bug in the final series.
- [Low] The new uAPI enum port-type
  (Documentation/netlink/specs/ethtool.yaml line 216, generated as enum…

--

netlink: specs: Add ethernet port listing with ethtool

This adds the YAML spec, the documentation and the generated uAPI header
definitions for a new ETHTOOL_MSG_PORT_GET command. The command reports
the front-facing ports tracked by the phy_port infrastructure. It defines
a new port-type enum (mdi, sfp) and a port attribute set with id,
supported modes, supported interfaces, type and upstream port.

> diff --git a/Documentation/netlink/specs/ethtool.yaml b/Documentation/netlink/specs/ethtool.yaml
> index a66594ad9e739..a9cb025387830 100644
> --- a/Documentation/netlink/specs/ethtool.yaml
> +++ b/Documentation/netlink/specs/ethtool.yaml
> @@ -212,6 +212,11 @@ definitions:
>        -
>          name: discard
>          value: 31
> +  -
> +    name: port-type
> +    doc: type of physical connector
> +    type: enum
> +    entries: [mdi, sfp]

[Severity: Low]
Could the meaning of this enum be spelled out more clearly? The doc
string "type of physical connector" is close to what
ETHTOOL_A_LINKINFO_PORT already carries with the legacy PORT_* values
(PORT_TP, PORT_FIBRE, PORT_OTHER, ...).

The existing phy_port_get_type() helper also uses the "port type" name,
but it returns PORT_* values:

drivers/net/phy/phy_port.c:phy_port_get_type() {
	if (port->mediums & BIT(ETHTOOL_LINK_MEDIUM_BASET))
		return PORT_TP;
    ...
}

So ETHTOOL_PORT_TYPE_MDI and PORT_TP are both 0, but they mean different
things. The PORT_GET section in ethtool-netlink.rst describes
ETHTOOL_A_PORT_TYPE only as "the port type". It does not name enum
ethtool_port_type or list the values.

The mdi and sfp entries also have no per-entry doc. Here is
port_fill_reply() in net/ethtool/port.c from the next patch in the
series:

	port_type = ETHTOOL_PORT_TYPE_MDI;
	if (reply->mii || reply->sfp)
		port_type = ETHTOOL_PORT_TYPE_SFP;

With this code an SFP cage reports sfp. The port of an SFP module plugged
into that cage (upstream_port set, is_sfp not set) reports mdi.

These values become fixed uAPI once released. Would it make sense to
document what mdi and sfp mean in each of these cases, and to reference
the enum from the rst?

[ ... ]

> diff --git a/include/uapi/linux/ethtool_netlink_generated.h b/include/uapi/linux/ethtool_netlink_generated.h
> index 8134baf7860f3..660e0c08a766a 100644
> --- a/include/uapi/linux/ethtool_netlink_generated.h
> +++ b/include/uapi/linux/ethtool_netlink_generated.h

[ ... ]

> @@ -893,6 +913,7 @@ enum {
>  	ETHTOOL_MSG_RSS_CREATE_ACT,
>  	ETHTOOL_MSG_RSS_DELETE_ACT,
>  	ETHTOOL_MSG_MSE_GET,
> +	ETHTOOL_MSG_PORT_GET,
>  
>  	__ETHTOOL_MSG_USER_CNT,
>  	ETHTOOL_MSG_USER_MAX = (__ETHTOOL_MSG_USER_CNT - 1)

[Severity: Low]
This isn't a bug in the final series. At this commit, though, nothing
under net/ethtool/ handles ETHTOOL_MSG_PORT_GET: there is no policy, no
ethnl_default_requests entry and no genl op. A request therefore fails
with -EOPNOTSUPP. Meanwhile, the new PORT_GET section in
ethtool-netlink.rst already describes working DO and DUMP behaviour.

The next patch, "net: ethtool: Introduce ethtool command to list ports",
adds net/ethtool/port.c with ethnl_port_request_ops, ethnl_port_dumpit()
and ethnl_port_get_policy. It also registers ETHTOOL_MSG_PORT_GET in
net/ethtool/netlink.c, so the gap is closed by the end of the series.

Is this intermediate state intended? Splitting the spec from the
implementation is common netdev practice, so this is only a note.

-- 
Sashiko AI review · https://netdev-ai.bots.linux.dev/sashiko/#/patchset/20261016133107.54054-1-maxime.chevallier%40bootlin.com