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

Sean Anderson
2025-04-17T14:56:17Z

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