Re: [PATCH net-next v3 1/5] net: enetc: Add pseudo-MAC support for ENETCv4 Ports via a fixed-link node

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
- [Medium] net: enetc: NXP_ENETC4 selects OF_DYNAMIC without depending on OF
- [Medium] net: enetc: enetc_phylink_match_pseudo_mac_speed() returns >10G fixed-link
--

commit 47b0c7dd1742d2d679a7876866a314bac673517d
Author: Claudiu Manoil <claudiu.manoil@nxp.com>

net: enetc: Add pseudo-MAC support for ENETCv4 Ports via a fixed-link node

This commit adds pseudo-MAC support for ENETCv4 ports by synthesizing a
runtime fixed-link devicetree subnode via an of_changeset using the boot-time
speed read from the port PCR register.

> diff --git a/drivers/net/ethernet/freescale/enetc/Kconfig b/drivers/net/ethernet/freescale/enetc/Kconfig
> index f425f82a6213a..c51d07db2c869 100644
> --- a/drivers/net/ethernet/freescale/enetc/Kconfig
> +++ b/drivers/net/ethernet/freescale/enetc/Kconfig
> @@ -55,6 +55,7 @@ config NXP_ENETC4
>  	select NXP_NTMP
>  	select PHYLINK
>  	select DIMLIB
> +	select OF_DYNAMIC

[Severity: Medium]
As reported by kernel test robot on v2 [1], does selecting OF_DYNAMIC here
without depending on OF cause Kconfig unmet dependency warnings and link
failures when CONFIG_OF is disabled?

In drivers/of/Kconfig, OF_DYNAMIC (and OF_KOBJ, which it selects) is defined
inside the if OF block:

drivers/of/Kconfig:
if OF
...
config OF_DYNAMIC
	bool "Support for dynamic device trees" if OF_UNITTEST
	select OF_KOBJ

When building with CONFIG_OF=n and CONFIG_NXP_ENETC4 enabled (for example,
under CONFIG_COMPILE_TEST=y on x86_64), selecting OF_DYNAMIC forces
CONFIG_OF_DYNAMIC=y while CONFIG_OF remains disabled.

Because CONFIG_OF_DYNAMIC=y is set, include/linux/of.h exposes extern
declarations rather than inline stubs for the of_changeset_*() and
of_node_put() helpers, while drivers/Makefile only descends into drivers/of/
when CONFIG_OF is enabled. As a result, the calls to of_changeset_init(),
of_changeset_create_node(), of_changeset_add_prop_u32(),
of_changeset_add_prop_bool(), of_changeset_apply(), of_changeset_revert(),
of_changeset_destroy(), and of_node_put() in enetc4_add_ppm_fixed_link() and
enetc4_put_ppm_fixed_link() fail with undefined references at link time.

Should NXP_ENETC4 depend on OF, or only select OF_DYNAMIC if OF (with the
relevant calls or stubs guarded accordingly)?

[ ... ]

> diff --git a/drivers/net/ethernet/freescale/enetc/enetc_pf_common.c b/drivers/net/ethernet/freescale/enetc/enetc_pf_common.c
> index 8206884294a47..3ba46c3a7670b 100644
> --- a/drivers/net/ethernet/freescale/enetc/enetc_pf_common.c
> +++ b/drivers/net/ethernet/freescale/enetc/enetc_pf_common.c
> @@ -436,6 +436,43 @@ void enetc_mdiobus_destroy(struct enetc_pf *pf)
>  }
>  EXPORT_SYMBOL_GPL(enetc_mdiobus_destroy);
>  
> +static struct {
> +	unsigned long mac_cap;
> +	int speed; /* descending order sorted  */
> +} enetc_phylink_pseudo_mac_caps[] = {
> +	{ MAC_25000FD,  SPEED_25000 },
> +	{ MAC_20000FD,  SPEED_20000 },
> +	{ MAC_10000FD,  SPEED_10000 },
> +	{ MAC_5000FD,   SPEED_5000  },
> +	{ MAC_2500FD,   SPEED_2500  },
> +	{ MAC_1000FD,   SPEED_1000  },
> +	{ MAC_100FD,    SPEED_100   },
> +	{ MAC_10FD,     SPEED_10    },
> +};
> +
> +static unsigned long enetc_phylink_get_pseudo_mac_caps(void)
> +{
> +	unsigned long mac_caps = 0;
> +	int i;
> +
> +	for (i = 0; i < ARRAY_SIZE(enetc_phylink_pseudo_mac_caps); i++)
> +		mac_caps |= enetc_phylink_pseudo_mac_caps[i].mac_cap;
> +
> +	return mac_caps;
> +}
> +
> +int enetc_phylink_match_pseudo_mac_speed(int speed)
> +{
> +	int i;
> +
> +	for (i = 0; i < ARRAY_SIZE(enetc_phylink_pseudo_mac_caps); i++)
> +		if (enetc_phylink_pseudo_mac_caps[i].speed <= speed)
> +			return enetc_phylink_pseudo_mac_caps[i].speed;
> +
> +	return SPEED_UNKNOWN;
> +}
> +EXPORT_SYMBOL_GPL(enetc_phylink_match_pseudo_mac_speed);

[Severity: Medium]
What happens in phylink_parse_fixedlink() when an ENETCv4 pseudo-MAC port has
PCR[PSPEED] configured for 20000 or 25000 Mbps at boot?

In enetc4_link_init() -> enetc4_add_ppm_fixed_link(), the driver reads the
hardware speed via enetc4_get_pcr_speed(), maps it through
enetc_phylink_match_pseudo_mac_speed(), and writes the resulting speed
(SPEED_20000 or SPEED_25000) into the synthesized fixed-link node's speed
property before calling enetc_phylink_create() -> phylink_create().

However, in drivers/net/phy/phylink.c, phylink_parse_fixedlink() populates
pl->supported using phylink_fill_fixedlink_supported(), which only sets link
modes up to 10000baseT_Full:

drivers/net/phy/phylink.c:phylink_fill_fixedlink_supported() {
    linkmode_set_bit(ETHTOOL_LINK_MODE_10baseT_Half_BIT, supported);
    linkmode_set_bit(ETHTOOL_LINK_MODE_10baseT_Full_BIT, supported);
    linkmode_set_bit(ETHTOOL_LINK_MODE_100baseT_Half_BIT, supported);
    linkmode_set_bit(ETHTOOL_LINK_MODE_100baseT_Full_BIT, supported);
    linkmode_set_bit(ETHTOOL_LINK_MODE_1000baseT_Half_BIT, supported);
    linkmode_set_bit(ETHTOOL_LINK_MODE_1000baseT_Full_BIT, supported);
    linkmode_set_bit(ETHTOOL_LINK_MODE_2500baseT_Full_BIT, supported);
    linkmode_set_bit(ETHTOOL_LINK_MODE_5000baseT_Full_BIT, supported);
    linkmode_set_bit(ETHTOOL_LINK_MODE_10000baseT_Full_BIT, supported);
}

Because pl->supported has no 20000 or 25000 Mbps link modes set,
phy_caps_lookup() in phylink_parse_fixedlink() returns NULL for SPEED_20000
and SPEED_25000, triggering:

    phylink_warn(pl, "fixed link %s duplex %dMbps not recognised\n", ...);

and leaving pl->supported, pl->link_config.advertising, and
pl->link_config.lp_advertising with no speed link modes set.

[1] https://lore.kernel.org/r/202610151650.UHqL0Sb7-lkp@intel.com

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/cover.1793712309.git.claudiu.manoil@nxp.com?part=1