Re: [net-next PATCH v3 03/11] net: pcs: Add subsystem

Sean Anderson
2025-04-17T15:05:32Z

On 4/17/25 05:19, Simon Horman wrote:
> On Tue, Apr 15, 2025 at 03:33:15PM -0400, Sean Anderson wrote:
>> This adds support for getting PCS devices from the device tree. PCS
>> drivers must first register with phylink_register_pcs. After that, MAC
>> drivers may look up their PCS using phylink_get_pcs.
>>
>> We wrap registered PCSs in another PCS. This wrapper PCS is refcounted
>> and can outlive the wrapped PCS (such as if the wrapped PCS's driver is
>> unbound). The wrapper forwards all PCS callbacks to the wrapped PCS,
>> first checking to make sure the wrapped PCS still exists. This design
>> was inspired by Bartosz Golaszewski's talk at LPC [1].
>>
>> pcs_get_by_fwnode_compat is a bit hairy, but it's necessary for
>> compatibility with existing drivers, which often attach to (devicetree)
>> nodes directly. We use the devicetree changeset system instead of
>> adding a (secondary) software node because mdio_bus_match calls
>> of_driver_match_device to match devices, and that function only works on
>> devicetree nodes.
>>
>> [1] https://lpc.events/event/17/contributions/1627/
>>
>> Signed-off-by: Sean Anderson <sean.anderson@linux.dev>
>
> Hi Sean,
>
> I noticed a few build problems after sending my previous email.
>
> I was able to exercise them using variants of the following to
> generate small configs. I include this here in case it is useful to you.
>
> make tinyconfig
>
> cat >> .config << __EOF__
> CONFIG_MODULES=y
> CONFIG_NET=y
> CONFIG_NETDEVICES=y
> CONFIG_PCS=y
> CONFIG_PHYLIB=m
> __EOF__
>
> cat >> .config << __EOF__
> CONFIG_OF=y
> CONFIG_OF_UNITTEST=y
> CONFIG_OF_DYNAMIC=y
> __EOF__
>
> yes "" | make oldconfig
>
> ...
>
>> diff --git a/drivers/net/pcs/core.c b/drivers/net/pcs/core.c
>
> ...

Thanks, I was able to reproduce/fix these issues.

How did you find these? By inspection?

I often end up missing build issues like this because I mostly
test with everything enabled.

--Sean

>> +/**
>> + * _pcs_get() - Get a PCS from a fwnode property
>> + * @dev: The device to get a PCS for
>> + * @fwnode: The fwnode to find the PCS with
>> + * @id: The name of the PCS to get. May be %NULL to get the first PCS.
>> + * @fallback: An optional fallback property to use if pcs-handle is absent
>> + * @optional: Whether the PCS is optional
>> + *
>> + * Find a PCS referenced by @mac_node and return a reference to it. Every call
>> + * to _pcs_get_by_fwnode() must be balanced with one to pcs_put().
>> + *
>> + * Return: a PCS if found, %NULL if not, or an error pointer on failure
>> + */
>> +struct phylink_pcs *_pcs_get(struct device *dev, struct fwnode_handle *fwnode,
>> + const char *id, const char *fallback,
>> + bool optional)
>> +{
>> + struct fwnode_handle *pcs_fwnode;
>> + struct phylink_pcs *pcs;
>> +
>> + pcs_fwnode = pcs_find_fwnode(fwnode, id, fallback, optional);
>> + if (IS_ERR(pcs_fwnode))
>> + return ERR_CAST(pcs_fwnode);
>> +
>> + pcs = _pcs_get_tail(dev, pcs_fwnode, NULL);
>> + fwnode_handle_put(pcs_fwnode);
>> + return pcs;
>> +}
>> +EXPORT_SYMBOL_GPL(_pcs_get);
>> +
>> +static __maybe_unused void of_changeset_cleanup(void *data)
>> +{
>> + struct of_changeset *ocs = data;
>
> Code in pcs_get_by_fwnode_compat is conditionally compiled
> based on CONFIG_OF_DYNAMIC. I think that is needed here too,
> because of_changeset_revert() doesn't exist unless CONFIG_OF_DYNAMIC is set.
>
>> +
>> + if (WARN(of_changeset_revert(ocs),
>> + "could not revert changeset; leaking memory\n"))
>> + return;
>> +
>> + of_changeset_destroy(ocs);
>> + kfree(ocs);
>> +}
>> +
>> +/**
>> + * pcs_get_by_fwnode_compat() - Get a PCS with a compatibility fallback
>> + * @dev: The device requesting the PCS
>> + * @fwnode: The &struct fwnode_handle of the PCS itself
>> + * @fixup: Callback to fix up @fwnode for compatibility
>> + * @data: Passed to @fixup
>> + *
>> + * This function looks up a PCS and retries on failure after fixing up @fwnode.
>> + * It is intended to assist in backwards-compatible behavior for drivers that
>> + * used to create a PCS directly from a &struct device_node. This function
>> + * should NOT be used in new drivers.
>> + *
>> + * @fixup modifies a devicetree changeset to create any properties necessary to
>> + * bind the PCS's &struct device_node. At the very least, it should use
>> + * of_changeset_add_prop_string() to add a compatible property.
>> + *
>> + * Note that unlike pcs_get_by_fwnode, @fwnode is the &struct fwnode_handle of
>> + * the PCS itself, and not that of the requesting device. @fwnode could be
>> + * looked up with pcs_find_fwnode() or determined by some other means for
>> + * compatibility.
>> + *
>> + * Return: A PCS on success or an error pointer on failure
>> + */
>> +struct phylink_pcs *
>> +pcs_get_by_fwnode_compat(struct device *dev, struct fwnode_handle *fwnode,
>> + int (*fixup)(struct of_changeset *ocs,
>> + struct device_node *np, void *data),
>> + void *data)
>> +{
>> +#ifdef CONFIG_OF_DYNAMIC
>> + struct mdio_device *mdiodev;
>> + struct of_changeset *ocs;
>> + struct phylink_pcs *pcs;
>> + struct device_node *np;
>> + struct device *pcsdev;
>> + int err;
>> +
>> + /* First attempt */
>> + pcs = _pcs_get_tail(dev, fwnode, NULL);
>> + if (PTR_ERR(pcs) != -EPROBE_DEFER)
>> + return pcs;
>> +
>> + /* No luck? Maybe there's no compatible... */
>> + np = to_of_node(fwnode);
>> + if (!np || of_property_present(np, "compatible"))
>> + return pcs;
>> +
>> + /* OK, let's try fixing things up */
>> + pr_warn("%pOF is missing a compatible\n", np);
>> + ocs = kmalloc(sizeof(*ocs), GFP_KERNEL);
>> + if (!ocs)
>> + return ERR_PTR(-ENOMEM);
>> +
>> + of_changeset_init(ocs);
>> + err = fixup(ocs, np, data);
>> + if (err)
>> + goto err_ocs;
>> +
>> + err = of_changeset_apply(ocs);
>> + if (err)
>> + goto err_ocs;
>> +
>> + err = devm_add_action_or_reset(dev, of_changeset_cleanup, ocs);
>> + if (err)
>> + return ERR_PTR(err);
>> +
>> + mdiodev = fwnode_mdio_find_device(fwnode);
>
> fwnode_mdio_find_device() is unavailable for linking if PHYLIB is a module
> (and PCS is built-in).
>
>> + if (mdiodev) {
>> + /* Clear that pesky PHY flag so we can match PCS drivers */
>> + device_lock(&mdiodev->dev);
>> + mdiodev->flags &= ~MDIO_DEVICE_FLAG_PHY;
>> + device_unlock(&mdiodev->dev);
>> + pcsdev = &mdiodev->dev;
>> + } else {
>> + pcsdev = get_device(fwnode->dev);
>> + if (!pcsdev)
>> + return ERR_PTR(-EPROBE_DEFER);
>> + }
>> +
>> + err = device_reprobe(pcsdev);
>> + put_device(pcsdev);
>> + if (err)
>> + return ERR_PTR(err);
>> +
>> + return _pcs_get_tail(dev, fwnode, NULL);
>> +
>> +err_ocs:
>> + of_changeset_destroy(ocs);
>> + kfree(ocs);
>> + return ERR_PTR(err);
>> +#else
>> + return _pcs_get_tail(dev, fwnode, NULL);
>> +#endif
>> +}
>> +EXPORT_SYMBOL_GPL(pcs_get_by_fwnode_compat);
>
> ...