Message-ID: <20250417091936.GB2430521@horms.kernel.org>
Date: Thu, 17 Apr 2025 10:19:36 +0100
From: Simon Horman <horms@...nel.org>
To: Sean Anderson <sean.anderson@...ux.dev>
Cc: netdev@...r.kernel.org, Andrew Lunn <andrew+netdev@...n.ch>,
	"David S. Miller" <davem@...emloft.net>,
	Eric Dumazet <edumazet@...gle.com>,
	Jakub Kicinski <kuba@...nel.org>, Paolo Abeni <pabeni@...hat.com>,
	Russell King <linux@...linux.org.uk>, upstream@...oha.com,
	Christian Marangi <ansuelsmth@...il.com>,
	linux-kernel@...r.kernel.org,
	Kory Maincent <kory.maincent@...tlin.com>,
	Heiner Kallweit <hkallweit1@...il.com>,
	Jonathan Corbet <corbet@....net>, linux-doc@...r.kernel.org
Subject: Re: [net-next PATCH v3 03/11] net: pcs: Add subsystem

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@...ux.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

...

> +/**
> + * _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);

...

Powered by blists - more mailing lists