Messages in this thread | | | Date | Thu, 23 Nov 2023 14:34:08 +0100 | From | Maxime Chevallier <> | Subject | Re: [RFC PATCH net-next v2 01/10] net: phy: Introduce ethernet link topology representation |
| |
Hello Andrew,
On Tue, 21 Nov 2023 01:24:47 +0100 Andrew Lunn <andrew@lunn.ch> wrote:
> > +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(...);
Indeed, nice catch
> > > 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.
Indeed I assume it, I'll add an assert and document that clearly then.
> > > 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?
You're right this should be unsigned. I've also missed properly documenting correct values and their meaning (like phyindex 0 would be no index assigned).
> > > +enum phy_upstream { > > + PHY_UPSTREAM_MAC, > > + PHY_UPSTREAM_SFP, > > + PHY_UPSTREAM_PHY, > > +}; > > Please document what these actually mean.
I'll do this as well.
Thank you for reviewing,
Maxime
> > Andrew
| |