Message-ID: <98dbfe79-9ee7-40bc-85bd-3401310eacdd@linux.dev>
Date: Thu, 17 Apr 2025 10:56:08 -0400
From: Sean Anderson <sean.anderson@...ux.dev>
To: Simon Horman <horms@...nel.org>
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 4/17/25 04:35, 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@...ux.dev>
> 
> Hi Sean,
> 
> Overall this looks quite clean to me.
> Please find minor some nits flagged by tooling below.
> 
>> +/**
>> + * struct pcs_wrapper - Wrapper for a registered PCS
>> + * @pcs: the wrapping PCS
>> + * @refcnt: refcount for the wrapper
>> + * @list: list head for pcs_wrappers
>> + * @dev: the device associated with this PCS
>> + * @fwnode: this PCS's firmware node; typically @dev.fwnode
>> + * @wrapped: the backing PCS
>> + */
>> +struct pcs_wrapper {
>> +	struct phylink_pcs pcs;
>> +	refcount_t refcnt;
>> +	struct list_head list;
>> +	struct device *dev;
>> +	struct fwnode_handle *fwnode;
>> +	struct phylink_pcs *wrapped;
>> +};
> 
> I think that wrapped needs an __rcu annotation.
> 
> Flagged by Sparse.
> 
> ...

Will add.

>> +static int pcs_post_config(struct phylink_pcs *pcs,
>> +			   phy_interface_t interface)
>> +{
>> +	struct pcs_wrapper *wrapper = pcs_to_wrapper(pcs);
> 
> The line above dereferences pcs.
> 
>> +	struct phylink_pcs *wrapped;
>> +	int ret, idx;
>> +
>> +	idx = srcu_read_lock(&pcs_srcu);
>> +
>> +	wrapped = srcu_dereference(wrapper->wrapped, &pcs_srcu);
>> +	if (pcs && wrapped->ops->pcs_post_config)
> 
> But here it is assumed that pcs may be NULL.
> This does not seem consistent.
> 
> Flagged by Smatch.

This should be if(wrapped && ...

>> +		ret = wrapped->ops->pcs_post_config(wrapped, interface);
>> +	else
>> +		ret = 0;
>> +
>> +	srcu_read_unlock(&pcs_srcu, idx);
>> +	return ret;
>> +}
> 
> ...
> 
>> +/**
>> + * pcs_unregister() - unregister a PCS
>> + * @pcs: a PCS previously registered with pcs_register()
>> + */
>> +void pcs_unregister(struct phylink_pcs *pcs)
>> +{
>> +	struct pcs_wrapper *wrapper;
>> +
>> +	mutex_lock(&pcs_mutex);
>> +	list_for_each_entry(wrapper, &pcs_wrappers, list) {
>> +		if (wrapper->wrapped == pcs)
> 
> Assuming that rcu_access_pointer() works with srcu,
> I think that this should be:
> 
> 		if (rcu_access_pointer(wrapper->wrapped) == pcs)
> 
> Also flagged by Sparse

OK

>> +			goto found;
>> +	}
>> +
>> +	mutex_unlock(&pcs_mutex);
>> +	WARN(1, "trying to unregister an already-unregistered PCS\n");
>> +	return;
>> +
>> +found:
>> +	list_del(&wrapper->list);
>> +	mutex_unlock(&pcs_mutex);
>> +
>> +	put_device(wrapper->dev);
>> +	fwnode_handle_put(wrapper->fwnode);
>> +	rcu_replace_pointer(wrapper->wrapped, NULL, true);
>> +	synchronize_srcu(&pcs_srcu);
>> +
>> +	if (!wrapper->pcs.poll)
>> +		phylink_pcs_change(&wrapper->pcs, false);
>> +	if (refcount_dec_and_test(&wrapper->refcnt))
>> +		kfree(wrapper);
>> +}
>> +EXPORT_SYMBOL_GPL(pcs_unregister);
>> +
>> +static void devm_pcs_unregister(void *pcs)
>> +{
>> +	pcs_unregister(pcs);
>> +}
>> +
>> +/**
>> + * devm_pcs_register - resource managed pcs_register()
> 
> nit: devm_pcs_register_full
> 
>      Flagged by W=1 builds, and ./scripts/kernel-doc -none

OK

>> + * @dev: device that is registering this PCS
>> + * @fwnode: The PCS's firmware node; typically @dev.fwnode
>> + * @pcs: the PCS to register
>> + *
>> + * Managed pcs_register(). For PCSs registered by this function,
>> + * pcs_unregister() is automatically called on driver detach. See
>> + * pcs_register() for more information.
>> + *
>> + * Return: 0 on success, or -errno on failure
>> + */
>> +int devm_pcs_register_full(struct device *dev, struct fwnode_handle *fwnode,
> 
> ...
> 
>> +/**
>> + * pcs_find_fwnode() - Find a PCS's fwnode
>> + * @mac_node: The fwnode referencing the PCS
>> + * @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's fwnode, as referenced by @mac_node. This fwnode can later be
>> + * used with _pcs_get_tail() to get the actual PCS. ``pcs-handle-names`` is
>> + * used to match @id, then the fwnode is found using ``pcs-handle``.
>> + *
>> + * This function is internal to the PCS subsystem from a consumer
>> + * point-of-view. However, it may be used to implement fallbacks for legacy
>> + * behavior in PCS providers.
>> + *
>> + * Return: %NULL if @optional is set and the PCS cannot be found. Otherwise,
>> + * *       returns a PCS if found or an error pointer on failure.
>> + */
>> +struct fwnode_handle *pcs_find_fwnode(const struct fwnode_handle *mac_node,
>> +				      const char *id, const char *fallback,
>> +				      bool optional)
>> +{
>> +	int index;
>> +	struct fwnode_handle *pcs_fwnode;
> 
> Reverse xmas tree here please.

OK

> Edward Cree's xmastree tool can be helpful:
> https://github.com/ecree-solarflare/xmastree

I wonder if we could get this into checkpatch...

Thanks for the review.

--Sean

Powered by blists - more mailing lists