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

[Sean Anderson <sean.anderson@linux.dev>]

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);
> 
> ...