Re: [PATCH net-next v16 07/12] net: Document PCS subsystem

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [Medium] phylink: validate_mac_and_pcs incorrectly validates missing required PCS
--

commit cc21dbb17c411b60be48a08479995c35804697fd
Author: Christian Marangi <ansuelsmth@gmail.com>

    net: Document PCS subsystem
    
    This commit adds extensive documentation of the new PCS subsystem and the
    fwnode implementation with producer/consumer API. It also updates the
    sfp-phylink migration guide.

> diff --git a/Documentation/networking/pcs.rst b/Documentation/networking/pcs.rst
> new file mode 100644
> index 0000000000000..2cae702319484
> --- /dev/null
> +++ b/Documentation/networking/pcs.rst
> @@ -0,0 +1,234 @@
[ ... ]
> +Late PCS registration handling
> +------------------------------
> +
> +It's possible that a PCS becomes available after the MAC finished probing.
> +Contrary to the usual producer/consumer implementation, when a PCS is not
> +registered and can't be found, the fwnode parser helper returns ``-ENODEV``
> +instead of ``-EPROBE_DEFER``.
> +
> +This is to prevent race condition with particular devices that register
> +MAC and PCS with USB or PCIe and require the MAC to be registered before
> +the PCS.
> +
> +The phylink logic correctly handle this special case and keep the phylink
> +instance in a fail condition.

[Severity: Medium]
Does the phylink logic actually handle this correctly and keep the instance
in a fail condition?

Looking at phylink_validate_mac_and_pcs() in drivers/net/phy/phylink.c, if a
PCS is missing during phylink_create() due to late attach, it appears to
bypass the pcs_validate restrictions:

	} else if (test_bit(state->interface, pl->config->pcs_interfaces)) {
		struct phylink_pcs *tmp;

		list_for_each_entry(tmp, &pl->pcs_list, list) {
			if (!phylink_validate_pcs_interface(tmp, state->interface)) {
				pcs = tmp;
				break;
			}
		}
	}

	if (pcs) {
		...

If it falls through without returning -EINVAL, pl->supported receives the full
MAC capabilities.

Because pl->supported is never recalculated when the PCS attaches later, the
advertised link modes remain too broad.

This could lead to incorrect auto-negotiation masks and potential link
configuration failures observable via ethtool.

Should this function return -EINVAL if pcs is NULL but the interface requires
a PCS, rather than silently falling through to MAC validation?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260916235200.20712-1-ansuelsmth@gmail.com?part=7