Re: [RFC PATCH net-next 1/7] net: phy: introduce phy numbering and phy namespaces

[Maxime Chevallier <maxime.chevallier@bootlin.com>]

On Thu, 7 Sep 2023 11:14:08 +0100
"Russell King (Oracle)" <linux@armlinux.org.uk> wrote:

> On Thu, Sep 07, 2023 at 11:23:59AM +0200, Maxime Chevallier wrote:
> > Link topologies containing multiple network PHYs attached to the same
> > net_device can be found when using a PHY as a media converter for use
> > with an SFP connector, on which an SFP transceiver containing a PHY can
> > be used.
> > 
> > With the current model, the transceiver's PHY can't be used for
> > operations such as cable testing, timestamping, macsec offload, etc.
> > 
> > The reason being that most of the logic for these configuration, coming
> > from either ethtool netlink or ioctls tend to use netdev->phydev, which
> > in multi-phy systems will reference the PHY closest to the MAC.
> > 
> > Introduce a numbering scheme allowing to enumerate PHY devices that
> > belong to any netdev, which can in turn allow userspace to take more
> > precise decisions with regard to each PHY's configuration.
> > 
> > The numbering is maintained per-netdev, hence the notion of PHY
> > namespaces. The numbering works similarly to a netdevice's ifindex, with
> > identifiers that are only recycled once INT_MAX has been reached.
> > 
> > This prevents races that could occur between PHY listing and SFP
> > transceiver removal/insertion.
> > 
> > The identifiers are assigned at phy_attach time, as the numbering
> > depends on the netdevice the phy is attached to.  
> 
> I think you can simplify this code quite a bit by using idr.
> idr_alloc_cyclic() looks like it will do the allocation you want,
> plus the IDR subsystem will store the pointer to the object (in
> this case the phy device) and allow you to look that up. That
> probably gets rid of quite a bit of code.
> 
> You will need to handle the locking around IDR however.

Oh thanks for pointing this out. I had considered idr but I didn't spot
the _cyclic() helper, and I had ruled that out thinking it would re-use
ids directly after freeing them. I'll be more than happy to use that.

Thanks,

Maxime