Re: [PATCH net-next v6 06/12] net: Document PCS subsystem

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
- [High] Late PCS registration handling is structurally broken and causes `phylink_create` to fail permanently.
- [Low] Multiple grammatical and spelling errors in the PCS subsystem documentation.
--

commit 84279427a07ae5692e53d4ed76f418a074b2aea9
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.

> diff --git a/Documentation/networking/pcs.rst b/Documentation/networking/pcs.rst
> new file mode 100644
> index 0000000000000..9436ba43cebdf
> --- /dev/null
> +++ b/Documentation/networking/pcs.rst

[ ... ]

> +Looking up PCS Devices (fwnode implementation)
> +-----------------------------------------------
> +
> +The lookup of a PCS device follows the common producer/consumer implementation
> +used by similar subsystem with a ``#pcs-cells`` on the producer and a
> +``pcs-handle`` property on the consumer::

[Severity: Low]
This isn't a bug, but there are a few minor grammatical and spelling errors in
the newly added documentation. Should "subsystem" be "subsystems" here?

Additionally, a few other minor typos are present in this document:
- "The fwnode implementation expose" -> "exposes"
- "should be used to relase them" -> "release"
- "takes three arg" -> "takes three arguments"

[ ... ]

> +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 ``-EINVAL``
> +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: High]
Is this description of the failure state accurate with the current
implementation?

Looking at phylink_create() in drivers/net/phy/phylink.c, if a PCS is
missing, it appears to abort the creation entirely:

phylink_create() {
    ...
    ret = phylink_fill_available_pcs(pl, config);
    if (ret) {
        kfree(pl);
        return ERR_PTR(ret);
    }
    ...
}

Similarly, if fwnode_phylink_pcs_parse() in drivers/net/pcs/pcs.c cannot
find a PCS, it returns an error:

fwnode_phylink_pcs_parse() {
    ...
    pcs = fwnode_pcs_get(fwnode, i);
    if (IS_ERR(pcs))
        return PTR_ERR(pcs);
    ...
}

If this error propagates up through the MAC driver's fill_available_pcs()
callback to phylink_fill_available_pcs(), wouldn't phylink_create()
immediately free the phylink instance and return the error before the late
registration notifier is ever registered?

Could you clarify how the instance is kept in a fail condition if it gets
freed during phylink_create()?

[ ... ]

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