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@xxxxxxxxx> > > 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
