Re: [net-next RFC PATCH 1/6] net: phy: add support for defining multiple PHY IDs in PHY driver

[Christian Marangi]

On Sun, Feb 18, 2024 at 07:33:06PM +0000, Russell King (Oracle) wrote:
> On Sun, Feb 18, 2024 at 08:00:27PM +0100, Christian Marangi wrote:
> > Some PHY driver might implement the same OPs for different PHY ID and
> > using a mask is not enough to match similar PHYs.
> > 
> > To reduce code duplication, add support for defining multiple PHY IDs in
> > PHY driver struct.
> > 
> > Introduce a new variable in phy_driver struct, .ids, where a table array of
> > mdio_device_id can be defined to reference multiple PHY IDs (with their
> > own masks) supporting the same group of OPs and flags.
> > 
> > Introduce a new variable in phy_device, .dev_id, where the matching
> > mdio_device_id is stored. PHYs supporting multiple PHYs for one PHY
> > driver struct, should use this instead of matching for phy_id.
> > 
> > Single PHY ID implementation is still supported and dev_id is filled
> > with the data from phy_driver in this case.
> 
> This looks like it's been reworked somewhat with my suggestion, or maybe
> we just came across a similar structure for comparing the IDs?
>

Hi, I forgot to include this question in the cover letter. Yes the
matching logic is from your suggestion but I changed the other part of
the logic. What credits should I use? From and Sob? Suggested-by?
Make it a separate patch?

> > +	phy_dev_id = (struct mdio_device_id *)&phydev->dev_id;
> 
> Why this cast? Try to write code that doesn't need casts.
> 

This cast is needed to keep the dev_id const in the phy_device struct so
that other are warned to not modify it and should only be handled by
phy_probe since it's the one that fills it.

Alternative is to drop const and drop the warning.

> > +	/* Fill the mdio_device_id for the PHY istance.
> > +	 * If PHY driver provide an array of PHYs, search the right one,
> > +	 * in the other case fill it with the phy_driver data.
> > +	 */
> > +	if (phy_driver_match(phydrv, phydev, &dev_id) && dev_id) {
> > +		memcpy(phy_dev_id, dev_id, sizeof(*dev_id));
> > +	} else {
> > +		phy_dev_id->phy_id = phydrv->phy_id;
> > +		phy_dev_id->phy_id_mask = phydrv->phy_id_mask;
> 
> So this is the _driver_ phy_id.
> 

Ok I think I need some help with the naming here.

phy_id refer to the real PHY ID (but it's empty with C45)
and anything in dev_id would be the one from the phy_driver.

I was confused by this as I wasn't thinking that phy_id from driver
might apply mask and is not always MATCH_EXACT.

> >  static inline bool phydev_id_compare(struct phy_device *phydev, u32 id)
> >  {
> > -	return phy_id_compare(id, phydev->phy_id, phydev->drv->phy_id_mask);
> > +	return phy_id_compare(id, phydev->dev_id.phy_id,
> > +			      phydev->dev_id.phy_id_mask);
> 
> And thus this code is now different (since it _was_ comparing the
> phydev phy_id, and you've changed it to effectively the driver's
> phy_id. While that should be the same for a matched driver, that
> is still a change that probably is't intentional.
>

Yes this change was done with the assumption that MATCH_EXACT is always
used but this is not the case and actually makes the thing wrong. Will
drop thanks for poitining this out!

My original idea was to "migrate" to device_match_id and trying to
deprecate phy_id but this doesn't makes sense at all since they reflect
different kind of data.

-- 
	Ansuel