Messages in this thread | ![/](/images/icornerl.gif) | | Date | Tue, 21 Nov 2023 01:24:47 +0100 | From | Andrew Lunn <> | Subject | Re: [RFC PATCH net-next v2 01/10] net: phy: Introduce ethernet link topology representation |
| |
> +int link_topo_add_phy(struct link_topology *lt, struct phy_device *phy, > + enum phy_upstream upt, void *upstream) > +{ > + ret = xa_alloc_cyclic(<->phys, &phy->phyindex, pdn, xa_limit_32b, > + <->next_phy_index, GFP_KERNEL); > + if (ret) > + goto err; > + > + return 0;
It looks like that could be just return xa_alloc_cyclic(...);
> diff --git a/include/linux/link_topology.h b/include/linux/link_topology.h
I think this filename is too generic. Maybe phy_link_topology.h, or move it into include/net.
> +struct phy_device *link_topo_get_phy(struct link_topology *lt, int phyindex); > +int link_topo_add_phy(struct link_topology *lt, struct phy_device *phy, > + enum phy_upstream upt, void *upstream); > + > +void link_topo_del_phy(struct link_topology *lt, struct phy_device *phy);
What is the locking for these functions? Are you assuming RTNL? Maybe add ASSERT_RTNL(); into them to make this clear.
> diff --git a/include/linux/link_topology_core.h b/include/linux/link_topology_core.h
Again, i think this filename is too generic.
> diff --git a/include/linux/netdevice.h b/include/linux/netdevice.h > index a16c9cc063fe..7021a0d3d982 100644 > --- a/include/linux/netdevice.h > +++ b/include/linux/netdevice.h > @@ -40,7 +40,6 @@ > #include <net/dcbnl.h> > #endif > #include <net/netprio_cgroup.h> > - > #include <linux/netdev_features.h>
Whitespace change.
> #include <linux/neighbour.h> > #include <uapi/linux/netdevice.h> > @@ -52,6 +51,7 @@ > #include <net/net_trackers.h> > #include <net/net_debug.h> > #include <net/dropreason-core.h> > +#include <linux/link_topology_core.h> > > struct netpoll_info; > struct device; > @@ -2405,6 +2405,7 @@ struct net_device { > #if IS_ENABLED(CONFIG_CGROUP_NET_PRIO) > struct netprio_map __rcu *priomap; > #endif > + struct link_topology link_topo; > struct phy_device *phydev; > struct sfp_bus *sfp_bus; > struct lock_class_key *qdisc_tx_busylock; > diff --git a/include/linux/phy.h b/include/linux/phy.h > index 3cc52826f18e..d698180b1df0 100644 > --- a/include/linux/phy.h > +++ b/include/linux/phy.h > @@ -543,6 +543,8 @@ struct macsec_ops; > * @drv: Pointer to the driver for this PHY instance > * @devlink: Create a link between phy dev and mac dev, if the external phy > * used by current mac interface is managed by another mac interface. > + * @phyindex: Unique id across the phy's parent tree of phys to address the PHY > + * from userspace, similar to ifindex. It's never recycled. > * @phy_id: UID for this device found during discovery > * @c45_ids: 802.3-c45 Device Identifiers if is_c45. > * @is_c45: Set to true if this PHY uses clause 45 addressing. > @@ -640,6 +642,7 @@ struct phy_device { > > struct device_link *devlink; > > + int phyindex;
Is this int, or unsigned int? Is a negative value possible and legal?
> +enum phy_upstream { > + PHY_UPSTREAM_MAC, > + PHY_UPSTREAM_SFP, > + PHY_UPSTREAM_PHY, > +};
Please document what these actually mean.
Andrew
| ![\](/images/icornerr.gif) |