| Message ID | 20240217194116.8565-1-ansuelsmth@gmail.com (mailing list archive) |
|---|---|
| Headers | show |
| Series | net: phy: detach PHY driver OPs from phy_driver struct | expand |
On Sat, Feb 17, 2024 at 08:41:11PM +0100, Christian Marangi wrote: > Posting as RFC due to the massive change to a fundamental struct. > > While adding some PHY ID for Aquantia, I notice that there is a > big problem with duplicating OPs with each PHY. > > The original idea to prevent this was to use mask on the PHY ID > and identify PHY Family. Problem is that OEM started to use all > kind of PHY ID and this is not doable, hence for PHY that have > the same OPs, we have to duplicate all of them. > > This is present in Aquantia PHY, but is much more present in > other PHY, especially in the BCM7XXX where they use a big macro > for common PHYs. > > To reduce patch delta, I added the additional variable without > adding tabs as this would have resulted in a massive patch. > Also to have patch bisectable, this change has to be in one go > hence I had to use this trick to reduce patch delta. > > Other solution to this problem were to introduce additional > variables to phy_driver struct but that would have resulted > in having 2 different way to do the same thing and that is not O.K. > > I took care to compile-test all the PHY, only exception is the unique > RUST driver, where I still have to learn that funny language and > I didn't had time to update it, so that is the only driver that > I think require some fixup. > > I posted 2 example that would benefits from this change, but I can > find much more in other PHY driver. Would it make more sense instead of this big churn, to instead introduce into struct phy_driver: struct mdio_device_id *ids; which would then allow a phy_driver structure to be matched by several device IDs? We then would not need to touch any of the existing drivers initially, and a later cleanup could be to identify those where all the ops are the same for several phy_driver structures, and convert them over.
On Sat, Feb 17, 2024 at 08:41:11PM +0100, Christian Marangi wrote: > Posting as RFC due to the massive change to a fundamental struct. > > While adding some PHY ID for Aquantia, I notice that there is a > big problem with duplicating OPs with each PHY. > > The original idea to prevent this was to use mask on the PHY ID > and identify PHY Family. Problem is that OEM started to use all > kind of PHY ID and this is not doable, hence for PHY that have > the same OPs, we have to duplicate all of them. > > This is present in Aquantia PHY, but is much more present in > other PHY, especially in the BCM7XXX where they use a big macro > for common PHYs. > > To reduce patch delta, I added the additional variable without > adding tabs as this would have resulted in a massive patch. > Also to have patch bisectable, this change has to be in one go > hence I had to use this trick to reduce patch delta. To me, this makes the code look ugly. I also expect many text editors which understand indentation will be unhappy, saying the indentation is wrong. > 64 files changed, 737 insertions(+), 291 deletions(-) These statistics are not good. If you had deleted more lines than you added, then maybe it might be an O.K. idea. Sometimes KISS is best. Andrew
On Sat, Feb 17, 2024 at 2:41 PM Christian Marangi <ansuelsmth@gmail.com> wrote: > > I took care to compile-test all the PHY, only exception is the unique > RUST driver, where I still have to learn that funny language and > I didn't had time to update it, so that is the only driver that > I think require some fixup. This will only need an update in the abstraction at rust/kernel/net/phy.rs, roughly (untested) @@ -5,15 +5,22 @@ pub const fn create_phy_driver<T: Driver>() -> DriverV flags: T::FLAGS, phy_id: T::PHY_DEVICE_ID.id, phy_id_mask: T::PHY_DEVICE_ID.mask_as_int(), - soft_reset: if T::HAS_SOFT_RESET { - Some(Adapter::<T>::soft_reset_callback) - } else { - None + phy_driver_ops: &bindings::phy_driver_ops { + soft_reset: if T::HAS_SOFT_RESET { + Some(Adapter::<T>::soft_reset_callback) + } else { + None + }, + get_features: if T::HAS_GET_FEATURES { + Some(Adapter::<T>::get_features_callback) + } else { + None + }, + // ... Other fields now in phy_driver_ops + + // SAFETY: The rest is zeroed out to finalize `struct phy_driver_ops`, + // which expects null function pointers for unused fields + ..unsafe { core::mem::MaybeUninit::<bindings::phy_driver_ops>::zeroed().assume_init() } }, - get_features: if T::HAS_GET_FEATURES { - Some(Adapter::<T>::get_features_callback) - } else { - None - }, })) } We can send a patch if needed once this series moves out of RFC. - Trevor
On Sat, Feb 17, 2024 at 07:53:08PM +0000, Russell King (Oracle) wrote: > On Sat, Feb 17, 2024 at 08:41:11PM +0100, Christian Marangi wrote: > > Posting as RFC due to the massive change to a fundamental struct. > > > > While adding some PHY ID for Aquantia, I notice that there is a > > big problem with duplicating OPs with each PHY. > > > > The original idea to prevent this was to use mask on the PHY ID > > and identify PHY Family. Problem is that OEM started to use all > > kind of PHY ID and this is not doable, hence for PHY that have > > the same OPs, we have to duplicate all of them. > > > > This is present in Aquantia PHY, but is much more present in > > other PHY, especially in the BCM7XXX where they use a big macro > > for common PHYs. > > > > To reduce patch delta, I added the additional variable without > > adding tabs as this would have resulted in a massive patch. > > Also to have patch bisectable, this change has to be in one go > > hence I had to use this trick to reduce patch delta. > > > > Other solution to this problem were to introduce additional > > variables to phy_driver struct but that would have resulted > > in having 2 different way to do the same thing and that is not O.K. > > > > I took care to compile-test all the PHY, only exception is the unique > > RUST driver, where I still have to learn that funny language and > > I didn't had time to update it, so that is the only driver that > > I think require some fixup. > > > > I posted 2 example that would benefits from this change, but I can > > find much more in other PHY driver. > > Would it make more sense instead of this big churn, to instead > introduce into struct phy_driver: > > struct mdio_device_id *ids; > > which would then allow a phy_driver structure to be matched by > several device IDs? Yes that was an alternative idea, but is it good to then have 2 way to declare PHY ID? Also the name should be changed... Maybe an array of a struct PHY_ID, name that ends with a sentinel? > > We then would not need to touch any of the existing drivers initially, > and a later cleanup could be to identify those where all the ops are > the same for several phy_driver structures, and convert them over. We have many PHY that already have macro to define the same OPs and change only name PHY ID and mask.
On Sat, Feb 17, 2024 at 11:21:44PM +0100, Andrew Lunn wrote: > On Sat, Feb 17, 2024 at 08:41:11PM +0100, Christian Marangi wrote: > > Posting as RFC due to the massive change to a fundamental struct. > > > > While adding some PHY ID for Aquantia, I notice that there is a > > big problem with duplicating OPs with each PHY. > > > > The original idea to prevent this was to use mask on the PHY ID > > and identify PHY Family. Problem is that OEM started to use all > > kind of PHY ID and this is not doable, hence for PHY that have > > the same OPs, we have to duplicate all of them. > > > > This is present in Aquantia PHY, but is much more present in > > other PHY, especially in the BCM7XXX where they use a big macro > > for common PHYs. > > > > To reduce patch delta, I added the additional variable without > > adding tabs as this would have resulted in a massive patch. > > Also to have patch bisectable, this change has to be in one go > > hence I had to use this trick to reduce patch delta. > > To me, this makes the code look ugly. I also expect many text editors > which understand indentation will be unhappy, saying the indentation is > wrong. > Yes, it was done to limit the patch delta, if I had to account for the tab for each new section we would be in the order of 2000+ changes I think. > > 64 files changed, 737 insertions(+), 291 deletions(-) > > These statistics are not good. If you had deleted more lines than you > added, then maybe it might be an O.K. idea. > > Sometimes KISS is best. > Well IMHO these stats are a bit flawed, the additional code is really just extra check if ops is defined and the new .ops variable in each phy_driver. If you check patch 2 and 3 you can already see some code is removed. Also while minimal the final .o produced results in bcm7xxx going from 53kb to 45kb and the aquantia driver from 48kb to 45kb. I understand that this major rework might be too much, so I think the only way to handle this is with introducing support for declaring multiple PHY ID for one phy_driver struct.
> Yes, it was done to limit the patch delta, if I had to account for the > tab for each new section we would be in the order of 2000+ changes I > think. > > > > 64 files changed, 737 insertions(+), 291 deletions(-) > > > > These statistics are not good. If you had deleted more lines than you > > added, then maybe it might be an O.K. idea. > > > > Sometimes KISS is best. > > > > Well IMHO these stats are a bit flawed, the additional code is really > just extra check if ops is defined and the new .ops variable in each > phy_driver. > > If you check patch 2 and 3 you can already see some code is removed. Yes, the problem is, it probably needs another 50 patches to remove all the duplication. I have to question, is that really going to happen? Are you going to keep working on this until every driver has its duplicates removed? It probably needs some tooling to help. Something which can decode the object file, and tell you which ops structures are identical. That can then guide you when editing all the PHY drivers. Andrew
On Sun, Feb 18, 2024 at 12:22:45AM +0100, Christian Marangi wrote: > On Sat, Feb 17, 2024 at 07:53:08PM +0000, Russell King (Oracle) wrote: > > Would it make more sense instead of this big churn, to instead > > introduce into struct phy_driver: > > > > struct mdio_device_id *ids; > > > > which would then allow a phy_driver structure to be matched by > > several device IDs? > > Yes that was an alternative idea, but is it good to then have 2 way to > declare PHY ID? > > Also the name should be changed... Maybe an array of a struct PHY_ID, > name that ends with a sentinel? We already have arrays of mdio_device_id in every driver, whether they can be re-used or not is a separate issue. In any case, merely adding support for this (only build tested patch below) is a much smaller change. drivers/net/phy/phy_device.c | 64 +++++++++++++++++++++++++++++++++----------- include/linux/phy.h | 16 ++++++++++- 2 files changed, 63 insertions(+), 17 deletions(-) diff --git a/drivers/net/phy/phy_device.c b/drivers/net/phy/phy_device.c index 2e7d5bfb338e..7e02bf51a2a5 100644 --- a/drivers/net/phy/phy_device.c +++ b/drivers/net/phy/phy_device.c @@ -522,12 +522,51 @@ static int phy_scan_fixups(struct phy_device *phydev) return 0; } +static const struct mdio_device_id * +phy_driver_match_id(struct phy_driver *phydrv, u32 id) +{ + const struct mdio_device_id *ids = phydrv->ids; + + if (ids) { + while (ids->phy_id) { + if (phy_id_compare(id, ids->phy_id, ids->phy_id_mask)) + return ids; + ids++; + } + } + + if (phy_id_compare(id, phydrv->id.phy_id, phydrv->id.phy_id_mask)) + return &phydrv->id; + + return NULL; +} + +static const struct mdio_device_id * +phy_driver_match(struct phy_driver *phydrv, struct phy_device *phydev) +{ + const int num_ids = ARRAY_SIZE(phydev->c45_ids.device_ids); + const struct mdio_device_id *id; + int i; + + if (!phydev->is_c45) + return phy_driver_match_id(phydrv, phydev->phy_id); + + for (i = 1; i < num_ids; i++) { + if (phydev->c45_ids.device_ids[i] == 0xffffffff) + continue; + + id = phy_driver_match_id(phydrv, phydev->c45_ids.device_ids[i]); + if (id) + return id; + } + + return NULL; +} + static int phy_bus_match(struct device *dev, struct device_driver *drv) { struct phy_device *phydev = to_phy_device(dev); struct phy_driver *phydrv = to_phy_driver(drv); - const int num_ids = ARRAY_SIZE(phydev->c45_ids.device_ids); - int i; if (!(phydrv->mdiodrv.flags & MDIO_DEVICE_IS_PHY)) return 0; @@ -535,20 +574,7 @@ static int phy_bus_match(struct device *dev, struct device_driver *drv) if (phydrv->match_phy_device) return phydrv->match_phy_device(phydev); - if (phydev->is_c45) { - for (i = 1; i < num_ids; i++) { - if (phydev->c45_ids.device_ids[i] == 0xffffffff) - continue; - - if (phy_id_compare(phydev->c45_ids.device_ids[i], - phydrv->phy_id, phydrv->phy_id_mask)) - return 1; - } - return 0; - } else { - return phy_id_compare(phydev->phy_id, phydrv->phy_id, - phydrv->phy_id_mask); - } + return !!phy_driver_match(phydrv, phydev); } static ssize_t @@ -3311,6 +3337,7 @@ static int phy_probe(struct device *dev) int err = 0; phydev->drv = phydrv; + phydev->drv_id = phy_driver_match(phydrv, phydev); /* Disable the interrupt if the PHY doesn't support it * but the interrupt is still a valid one @@ -3485,6 +3512,11 @@ int phy_driver_register(struct phy_driver *new_driver, struct module *owner) new_driver->mdiodrv.driver.owner = owner; new_driver->mdiodrv.driver.probe_type = PROBE_FORCE_SYNCHRONOUS; + if (!new_driver->id.phy_id) { + new_driver->id.phy_id = new_driver->phy_id; + new_driver->id.phy_id_mask = new_driver->phy_id_mask; + } + retval = driver_register(&new_driver->mdiodrv.driver); if (retval) { pr_err("%s: Error %d in registering driver\n", diff --git a/include/linux/phy.h b/include/linux/phy.h index fd8dbea9b4d9..2f2ebbd41535 100644 --- a/include/linux/phy.h +++ b/include/linux/phy.h @@ -542,6 +542,7 @@ struct macsec_ops; * * @mdio: MDIO bus this PHY is on * @drv: Pointer to the driver for this PHY instance + * @drv_id: The matched driver ID 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. * @phy_id: UID for this device found during discovery @@ -639,6 +640,7 @@ struct phy_device { /* Information about the PHY type */ /* And management functions */ const struct phy_driver *drv; + const struct mdio_device_id *drv_id; struct device_link *devlink; @@ -882,6 +884,9 @@ struct phy_led { * struct phy_driver - Driver structure for a particular PHY type * * @mdiodrv: Data common to all MDIO devices + * @ids: array of mdio device IDs to match this driver (terminated with + * zero phy_id) + * @id: mdio device ID to match * @phy_id: The result of reading the UID registers of this PHY * type, and ANDing them with the phy_id_mask. This driver * only works for PHYs with IDs which match this field @@ -903,6 +908,8 @@ struct phy_led { */ struct phy_driver { struct mdio_driver_common mdiodrv; + const struct mdio_device_id *ids; + struct mdio_device_id id; u32 phy_id; char *name; u32 phy_id_mask; @@ -1203,7 +1210,14 @@ static inline bool phy_id_compare(u32 id1, u32 id2, u32 mask) */ 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); + u32 mask; + + if (phydev->drv_id) + mask = phydev->drv_id->phy_id_mask; + else + mask = phydev->drv->phy_id_mask; + + return phy_id_compare(id, phydev->phy_id, mask); } /* A Structure for boards to register fixups with the PHY Lib */