Link modes representation in phylib

Hello everyone,

I'm currently working on adding support for 2.5GBaseT on some Marvell PHYs (the marvell10g family, including the 88X3310).

However, phylib doesn't quite support these modes yet. Its stores the different supported and advertised modes in u32 fields, which can't contain the relevant values for 2500BaseT mode (and all other modes that come after the 31st one).

I'm refering to the "advertising", "lp_advertising" and "supported" fields in struct phy_device [1] and also to the "features" in struct phy_driver [2].

newer representation, using the ETHTOOL_G/SLINKSETTINGS API for example.

in net/ethernet that still directly access the "phydev->supported" fields (about 30 of them) to use phy_ethtool_ksettings_{g/s}et, and also using another way to declare the "features" in phy_driver.

There are quite a lot of references to the "legacy" ways of representing modes, this would be another step closer to switching the a newer representation, but since this has an impact in a lot of places in net/, so I wanted to have your opinion on that point before attempting anything.

So would you have any advice on how we should start supporting newer modes in PHY drivers ?

Thanks,

Maxime

[1]
https://elixir.bootlin.com/linux/v4.18-rc1/source/include/linux/phy.h#L441

[2]
https://elixir.bootlin.com/linux/v4.18-rc1/source/include/linux/phy.h#L512

Maxime Chevallier, Bootlin (formerly Free Electrons)
Embedded Linux and kernel engineering
https://bootlin.com

Hi Maxime

Did you look at phylink? I think it already gets this right. It could be, any MAC which needs to use > bit 31 should use phylink, not phylib.

That narrows the problem down to just the PHY drivers. We might be able to mass convert those. Or maybe we can consider just doing some conversion work on PHYs which support > 1Gbps?

Andrew

Hello Andrew,

Thanks for your feedback !

Indeed, drivers that use phylink dont directly access these u32 fields.

I think that we can consider converting only the concerned PHYs for the moment.

What I propose is that we add 3 link_mode fields in phy_device, and keep the legacy fields for now. It would be up to the driver to fill the new "supported" field in config_init, kind of like what's done in the marvell10g driver.

There already are phy_ethtool_ksettings_{g|s}et accessors, that are used by phylink so that would easily integrate with the above solution of only supporting phylink for these modes.

That would involve a bit of info duplication, but I think that would allow for a smooth transition to a newer representation.

Would that be acceptable ?

Thanks,

Maxime

Hi Maxime

You can do this conversion in the core. If features == 0, and some bits are set in the features link_mode, do the conversion at probe time. The same can be done for lp_advertising, when the call into the drivers read_status() has completed.

It sounds reasonable. Lets see what the code looks like.

Andrew

Hello Andrew,

Thanks for the suggestion. I see how this can be done with phydrv->supported and phydev->lp_advertising, however I'm not sure how we should deal with the fact that ethernet drivers directly access fields such as "advertising" or "supported".

Should we update the new fields in phy_device when functions such as "phy_start" or "phy_start_aneg" are called, just in case the ethernet driver modified the phydev->supported / phydev->advertising fields in the meantime ?

My concern is that phylink will rely on the new link_mode representation to be up-to-date, and ethernet drivers that aren't converted to phylink, but link to a new PHY will rely on the legacy representation.

That might be just fine if we take good care making sure both the legacy and the new representation are well sync'd, but since I don't have a good big-picture view of all this, I prefer having your opinion first :)

Ok I'll be glad to send a series for that, I just want to make sure I go in the right direction

Thanks,

Maxime

Hi Maxime

I started cleaning up some of the MAC drivers. Take a look at

https://github.com/lunn/linux.git v4.18-rc1-net-next-phy-cleanup

That moved a lot of the MAC code into helpers, making it easier to update. It might make sense to add a couple of more helpers, for what remains.

Andrew

Hello Andrew,

Wow indeed that will help a lot. Just so that we're in sync, do you plan to add those helpers, or should I take this branch as a base for the conversion and go on ?

Thanks for this,

Maxime

Maxime Chevallier, Bootlin (formerly Free Electrons)
Embedded Linux and kernel engineering
https://bootlin.com

I'm still working on it. I can probably push again in the next few minutes. But they won't be compile tested, i.e. broken...

It might make more sense if you work on the core, and leave me to cleanup the MAC drivers. You can use my branch, if you need to change the helpers, just expect it to be unstable, get rebased, and might not even compile...

Andrew

Ok no problem, I'll keep working on the core while keeping an eye on your branch then.

Thanks a lot,

Maxime