[net-next,RFC,1/6] net: phy: add support for defining multiple PHY IDs in PHY driver
Commit Message
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(-)
Comments
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.
> 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
@@ -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
@@ -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 */