| Message ID | 20240218190034.15447-2-ansuelsmth@gmail.com (mailing list archive) |
|---|---|
| State | Superseded |
| Delegated to: | Netdev Maintainers |
| Headers | show |
| Series | net: phy: support multi PHY in phy_driver Was: net: phy: detach PHY driver OPs from phy_driver struct | expand |
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? > + phy_dev_id = (struct mdio_device_id *)&phydev->dev_id; Why this cast? Try to write code that doesn't need casts. > + /* 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. > 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.
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.
> > > + 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. Can you propagate the const. Make phy_dev_id point to a const? Andrew
On Sun, Feb 18, 2024 at 09:10:30PM +0100, Andrew Lunn wrote: > > > > + 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. > > Can you propagate the const. Make phy_dev_id point to a const? > Mhh not following, I tried changing to const struct mdio_device_id *phy_dev_id but that results in memcpy complain (dest is void * not const) and writing in read-only for the single PHY part (the else part) An alternative might be to make dev_id a pointer in struct phy_device and dynamically allocate a mdio_device_id for the case of single PHY (else case). That effectively remove the need of this cast but I would love to skip checking for -ENOMEM (this is why i made that local) If it's OK to dynamically allocate for the else case then I will make this change. I just tested this implementation and works correctly with not warning.
On Sun, Feb 18, 2024 at 09:27:22PM +0100, Christian Marangi wrote: > On Sun, Feb 18, 2024 at 09:10:30PM +0100, Andrew Lunn wrote: > > > > > + 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. > > > > Can you propagate the const. Make phy_dev_id point to a const? > > > > Mhh not following, I tried changing to const struct mdio_device_id *phy_dev_id > but that results in memcpy complain (dest is void * not const) and > writing in read-only for the single PHY part (the else part) > > An alternative might be to make dev_id a pointer in struct phy_device > and dynamically allocate a mdio_device_id for the case of single PHY > (else case). That effectively remove the need of this cast but I would > love to skip checking for -ENOMEM (this is why i made that local) > > If it's OK to dynamically allocate for the else case then I will make > this change. I just tested this implementation and works correctly with > not warning. Why do we need memcpy() etc - as I demonstrated in my proposal, it's not necessary if we introduce a mdio_device_id within struct phy_driver and we can just store a const pointer to the mdio_device_id that matched. That was very much an intentional decision in my proposal to make the code simple.
On Sun, Feb 18, 2024 at 08:34:16PM +0000, Russell King (Oracle) wrote: > On Sun, Feb 18, 2024 at 09:27:22PM +0100, Christian Marangi wrote: > > On Sun, Feb 18, 2024 at 09:10:30PM +0100, Andrew Lunn wrote: > > > > > > + 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. > > > > > > Can you propagate the const. Make phy_dev_id point to a const? > > > > > > > Mhh not following, I tried changing to const struct mdio_device_id *phy_dev_id > > but that results in memcpy complain (dest is void * not const) and > > writing in read-only for the single PHY part (the else part) > > > > An alternative might be to make dev_id a pointer in struct phy_device > > and dynamically allocate a mdio_device_id for the case of single PHY > > (else case). That effectively remove the need of this cast but I would > > love to skip checking for -ENOMEM (this is why i made that local) > > > > If it's OK to dynamically allocate for the else case then I will make > > this change. I just tested this implementation and works correctly with > > not warning. > > Why do we need memcpy() etc - as I demonstrated in my proposal, it's > not necessary if we introduce a mdio_device_id within struct phy_driver > and we can just store a const pointer to the mdio_device_id that > matched. That was very much an intentional decision in my proposal to > make the code simple. > With the allocated mdio_devic_id it would result in this snipped const struct mdio_device_id *driver_dev_id; struct mdio_device_id *dev_id; int err = 0; phydev->drv = phydrv; /* 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, &driver_dev_id) && driver_dev_id) { /* If defined, overwrite the PHY driver dev name with a * more specific one from the matching dev_id. */ phydev->dev_id = driver_dev_id; if (driver_dev_id->name) drv->name = driver_dev_id->name; } else { dev_id = kzalloc(sizeof(*phydev->dev_id), GFP_KERNEL); if (!dev_id) { err = -ENOMEM; goto out; } dev_id->phy_id = phydrv->phy_id; dev_id->phy_id_mask = phydrv->phy_id_mask; dev_id->name = phydrv->name; phydev->dev_id = dev_id; } Is it ok? (in phy.h the thing is const struct mdio_device_id *ids) I don't really like modifying phy_driver too much.
On Sun, Feb 18, 2024 at 09:44:03PM +0100, Christian Marangi wrote: > On Sun, Feb 18, 2024 at 08:34:16PM +0000, Russell King (Oracle) wrote: > > On Sun, Feb 18, 2024 at 09:27:22PM +0100, Christian Marangi wrote: > > > On Sun, Feb 18, 2024 at 09:10:30PM +0100, Andrew Lunn wrote: > > > > > > > + 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. > > > > > > > > Can you propagate the const. Make phy_dev_id point to a const? > > > > > > > > > > Mhh not following, I tried changing to const struct mdio_device_id *phy_dev_id > > > but that results in memcpy complain (dest is void * not const) and > > > writing in read-only for the single PHY part (the else part) > > > > > > An alternative might be to make dev_id a pointer in struct phy_device > > > and dynamically allocate a mdio_device_id for the case of single PHY > > > (else case). That effectively remove the need of this cast but I would > > > love to skip checking for -ENOMEM (this is why i made that local) > > > > > > If it's OK to dynamically allocate for the else case then I will make > > > this change. I just tested this implementation and works correctly with > > > not warning. > > > > Why do we need memcpy() etc - as I demonstrated in my proposal, it's > > not necessary if we introduce a mdio_device_id within struct phy_driver > > and we can just store a const pointer to the mdio_device_id that > > matched. That was very much an intentional decision in my proposal to > > make the code simple. > > > > With the allocated mdio_devic_id it would result in this snipped > > const struct mdio_device_id *driver_dev_id; > struct mdio_device_id *dev_id; > int err = 0; > > phydev->drv = phydrv; > /* 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, &driver_dev_id) && driver_dev_id) { > /* If defined, overwrite the PHY driver dev name with a > * more specific one from the matching dev_id. > */ > phydev->dev_id = driver_dev_id; > if (driver_dev_id->name) > drv->name = driver_dev_id->name; > } else { > dev_id = kzalloc(sizeof(*phydev->dev_id), GFP_KERNEL); > if (!dev_id) { > err = -ENOMEM; > goto out; > } > dev_id->phy_id = phydrv->phy_id; > dev_id->phy_id_mask = phydrv->phy_id_mask; > dev_id->name = phydrv->name; > phydev->dev_id = dev_id; > } > > Is it ok? (in phy.h the thing is const struct mdio_device_id *ids) > I don't really like modifying phy_driver too much. The thing that I don't like about this is that we need to free this allocation (and know that we need to free it) which adds more complexity and more possibilities for things leaking. The advantage to putting it in the phy_driver structure is that its lifetime is inherently tied to the driver structure and we don't have the issue of having to free it - nor do we need to have separate allocations for each PHY device.
> With the allocated mdio_devic_id it would result in this snipped > > const struct mdio_device_id *driver_dev_id; > struct mdio_device_id *dev_id; > int err = 0; > > phydev->drv = phydrv; > /* 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, &driver_dev_id) && driver_dev_id) { > /* If defined, overwrite the PHY driver dev name with a > * more specific one from the matching dev_id. > */ > phydev->dev_id = driver_dev_id; > if (driver_dev_id->name) > drv->name = driver_dev_id->name; What is drv here? You should not be changing the name within the driver structure, since that is shared by a number of devices. Andrew
diff --git a/drivers/net/phy/phy_device.c b/drivers/net/phy/phy_device.c index d63dca535746..9b96357e4de8 100644 --- a/drivers/net/phy/phy_device.c +++ b/drivers/net/phy/phy_device.c @@ -522,12 +522,74 @@ static int phy_scan_fixups(struct phy_device *phydev) return 0; } +static int phy_driver_match_id(struct phy_driver *phydrv, u32 id, + const struct mdio_device_id **dev_id) +{ + const struct mdio_device_id *ids = phydrv->ids; + + /* PHY driver might provide an array of different PHY IDs and + * masks. Walk them if this is the case and compare with ID. + */ + if (ids) { + /* From mdio_device_id struct phy_id_mask MUST + * be used as sentinel. + */ + while (ids->phy_id_mask) { + if (phy_id_compare(id, ids->phy_id, ids->phy_id_mask)) { + if (dev_id) + *dev_id = ids; + + return 1; + } + + ids++; + } + } + + if (phy_id_compare(id, phydrv->phy_id, phydrv->phy_id_mask)) + return 1; + + return 0; +} + +/** + * phy_driver_match - match a phydriver with a given PHY istance + * @phydrv: PHY driver to compare with + * @phydev: PHY istance to use for comparison. Either PHY ID will be used or + * with C45 PHY ID is extracted from Package regs. + * @dev_id: Pointer where to store pointer to a matchin mdio_device_id. + * mdio_device_id are assumed to be statically allocated for each PHY driver, + * hence the reference to this struct is returned here. + * + * Returns 1 if matching, 0 otherwise. dev_id can be passed as NULL to skip + * referecing a matching mdio_device_id if found. + */ +static int phy_driver_match(struct phy_driver *phydrv, struct phy_device *phydev, + const struct mdio_device_id **dev_id) +{ + const int num_ids = ARRAY_SIZE(phydev->c45_ids.device_ids); + int i; + + if (!phydev->is_c45) + return phy_driver_match_id(phydrv, phydev->phy_id, + dev_id); + + for (i = 1; i < num_ids; i++) { + if (phydev->c45_ids.device_ids[i] == 0xffffffff) + continue; + + if (phy_driver_match_id(phydrv, phydev->c45_ids.device_ids[i], + dev_id)) + return 1; + } + + return 0; +} + 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 +597,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, NULL); } static ssize_t @@ -3410,9 +3459,22 @@ static int phy_probe(struct device *dev) struct phy_device *phydev = to_phy_device(dev); struct device_driver *drv = phydev->mdio.dev.driver; struct phy_driver *phydrv = to_phy_driver(drv); + const struct mdio_device_id *dev_id = NULL; + struct mdio_device_id *phy_dev_id; int err = 0; phydev->drv = phydrv; + phy_dev_id = (struct mdio_device_id *)&phydev->dev_id; + /* 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; + } /* Disable the interrupt if the PHY doesn't support it * but the interrupt is still a valid one diff --git a/include/linux/phy.h b/include/linux/phy.h index c2dda21b39e1..f0313b9e0173 100644 --- a/include/linux/phy.h +++ b/include/linux/phy.h @@ -547,6 +547,7 @@ 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. + * @dev_id: The matched device ID for this PHY instance * @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. @@ -645,6 +646,7 @@ struct phy_device { struct device_link *devlink; + const struct mdio_device_id dev_id; u32 phy_id; struct phy_c45_device_ids c45_ids; @@ -885,6 +887,8 @@ 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_mask) * @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 @@ -906,6 +910,7 @@ struct phy_led { */ struct phy_driver { struct mdio_driver_common mdiodrv; + const struct mdio_device_id *ids; u32 phy_id; char *name; u32 phy_id_mask; @@ -1206,7 +1211,8 @@ 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); + return phy_id_compare(id, phydev->dev_id.phy_id, + phydev->dev_id.phy_id_mask); } /* A Structure for boards to register fixups with the PHY Lib */
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. Signed-off-by: Christian Marangi <ansuelsmth@gmail.com> --- drivers/net/phy/phy_device.c | 94 ++++++++++++++++++++++++++++++------ include/linux/phy.h | 8 ++- 2 files changed, 85 insertions(+), 17 deletions(-)