| Message ID | b066560d-2cc3-2ea5-5233-e63a612c5aa1@gmail.com |
|---|---|
| State | RFC |
| Delegated to: | David Miller |
| Headers | show |
| Series | net: phy: improve fixed_phy / swphy handling | expand |
On 8/13/19 2:25 PM, Heiner Kallweit wrote: > In cases like a fixed phy that is never attached to a net_device we > may want to bind the genphy driver at probe time. Setting a PHY ID of > 0xffffffff to bind the genphy driver would fail due to a check in > get_phy_device(). Therefore let's change the PHY ID the genphy driver > binds to to 0xfffffffe. This still shouldn't match any real PHY, > and it will pass the check in get_phy_devcie(). > > Signed-off-by: Heiner Kallweit <hkallweit1@gmail.com> > --- > drivers/net/phy/phy_device.c | 3 +-- > include/linux/phy.h | 4 ++++ > 2 files changed, 5 insertions(+), 2 deletions(-) > > diff --git a/drivers/net/phy/phy_device.c b/drivers/net/phy/phy_device.c > index 163295dbc..54f80af31 100644 > --- a/drivers/net/phy/phy_device.c > +++ b/drivers/net/phy/phy_device.c > @@ -2388,8 +2388,7 @@ void phy_drivers_unregister(struct phy_driver *drv, int n) > EXPORT_SYMBOL(phy_drivers_unregister); > > static struct phy_driver genphy_driver = { > - .phy_id = 0xffffffff, > - .phy_id_mask = 0xffffffff, > + PHY_ID_MATCH_EXACT(GENPHY_ID), > .name = "Generic PHY", > .soft_reset = genphy_no_soft_reset, > .get_features = genphy_read_abilities, > diff --git a/include/linux/phy.h b/include/linux/phy.h > index 5ac7d2137..3b07bce78 100644 > --- a/include/linux/phy.h > +++ b/include/linux/phy.h > @@ -37,6 +37,10 @@ > #define PHY_1000BT_FEATURES (SUPPORTED_1000baseT_Half | \ > SUPPORTED_1000baseT_Full) > > +#define GENPHY_ID_HIGH 0xffffU > +#define GENPHY_ID_LOW 0xfffeU > +#define GENPHY_ID ((GENPHY_ID_HIGH << 16) | GENPHY_ID_LOW) This is a possible user ABI change here, if there is anything that relies on reading 0xffff_ffff as a valid PHY OUI, you would be breaking it. We might as well try to assign ourselves a specific PHY OUI, very much like the Linux USB hubs show up with a Linux Foundation vendor ID.
On 14.08.2019 00:53, Florian Fainelli wrote: > On 8/13/19 2:25 PM, Heiner Kallweit wrote: >> In cases like a fixed phy that is never attached to a net_device we >> may want to bind the genphy driver at probe time. Setting a PHY ID of >> 0xffffffff to bind the genphy driver would fail due to a check in >> get_phy_device(). Therefore let's change the PHY ID the genphy driver >> binds to to 0xfffffffe. This still shouldn't match any real PHY, >> and it will pass the check in get_phy_devcie(). >> >> Signed-off-by: Heiner Kallweit <hkallweit1@gmail.com> >> --- >> drivers/net/phy/phy_device.c | 3 +-- >> include/linux/phy.h | 4 ++++ >> 2 files changed, 5 insertions(+), 2 deletions(-) >> >> diff --git a/drivers/net/phy/phy_device.c b/drivers/net/phy/phy_device.c >> index 163295dbc..54f80af31 100644 >> --- a/drivers/net/phy/phy_device.c >> +++ b/drivers/net/phy/phy_device.c >> @@ -2388,8 +2388,7 @@ void phy_drivers_unregister(struct phy_driver *drv, int n) >> EXPORT_SYMBOL(phy_drivers_unregister); >> >> static struct phy_driver genphy_driver = { >> - .phy_id = 0xffffffff, >> - .phy_id_mask = 0xffffffff, >> + PHY_ID_MATCH_EXACT(GENPHY_ID), >> .name = "Generic PHY", >> .soft_reset = genphy_no_soft_reset, >> .get_features = genphy_read_abilities, >> diff --git a/include/linux/phy.h b/include/linux/phy.h >> index 5ac7d2137..3b07bce78 100644 >> --- a/include/linux/phy.h >> +++ b/include/linux/phy.h >> @@ -37,6 +37,10 @@ >> #define PHY_1000BT_FEATURES (SUPPORTED_1000baseT_Half | \ >> SUPPORTED_1000baseT_Full) >> >> +#define GENPHY_ID_HIGH 0xffffU >> +#define GENPHY_ID_LOW 0xfffeU >> +#define GENPHY_ID ((GENPHY_ID_HIGH << 16) | GENPHY_ID_LOW) > > This is a possible user ABI change here, if there is anything that > relies on reading 0xffff_ffff as a valid PHY OUI, you would be breaking > it. We might as well try to assign ourselves a specific PHY OUI, very > much like the Linux USB hubs show up with a Linux Foundation vendor ID. > I see the point. However in get_phy_device() we have the following check that should cause a PHY with ID 0xffff_ffff to be ignored. Therefore I doubt there's any such PHY ID in use. /* If the phy_id is mostly Fs, there is no device there */ if ((phy_id & 0x1fffffff) == 0x1fffffff) return ERR_PTR(-ENODEV); Heiner
On 8/13/19 4:02 PM, Heiner Kallweit wrote: > On 14.08.2019 00:53, Florian Fainelli wrote: >> On 8/13/19 2:25 PM, Heiner Kallweit wrote: >>> In cases like a fixed phy that is never attached to a net_device we >>> may want to bind the genphy driver at probe time. Setting a PHY ID of >>> 0xffffffff to bind the genphy driver would fail due to a check in >>> get_phy_device(). Therefore let's change the PHY ID the genphy driver >>> binds to to 0xfffffffe. This still shouldn't match any real PHY, >>> and it will pass the check in get_phy_devcie(). >>> >>> Signed-off-by: Heiner Kallweit <hkallweit1@gmail.com> >>> --- >>> drivers/net/phy/phy_device.c | 3 +-- >>> include/linux/phy.h | 4 ++++ >>> 2 files changed, 5 insertions(+), 2 deletions(-) >>> >>> diff --git a/drivers/net/phy/phy_device.c b/drivers/net/phy/phy_device.c >>> index 163295dbc..54f80af31 100644 >>> --- a/drivers/net/phy/phy_device.c >>> +++ b/drivers/net/phy/phy_device.c >>> @@ -2388,8 +2388,7 @@ void phy_drivers_unregister(struct phy_driver *drv, int n) >>> EXPORT_SYMBOL(phy_drivers_unregister); >>> >>> static struct phy_driver genphy_driver = { >>> - .phy_id = 0xffffffff, >>> - .phy_id_mask = 0xffffffff, >>> + PHY_ID_MATCH_EXACT(GENPHY_ID), >>> .name = "Generic PHY", >>> .soft_reset = genphy_no_soft_reset, >>> .get_features = genphy_read_abilities, >>> diff --git a/include/linux/phy.h b/include/linux/phy.h >>> index 5ac7d2137..3b07bce78 100644 >>> --- a/include/linux/phy.h >>> +++ b/include/linux/phy.h >>> @@ -37,6 +37,10 @@ >>> #define PHY_1000BT_FEATURES (SUPPORTED_1000baseT_Half | \ >>> SUPPORTED_1000baseT_Full) >>> >>> +#define GENPHY_ID_HIGH 0xffffU >>> +#define GENPHY_ID_LOW 0xfffeU >>> +#define GENPHY_ID ((GENPHY_ID_HIGH << 16) | GENPHY_ID_LOW) >> >> This is a possible user ABI change here, if there is anything that >> relies on reading 0xffff_ffff as a valid PHY OUI, you would be breaking >> it. We might as well try to assign ourselves a specific PHY OUI, very >> much like the Linux USB hubs show up with a Linux Foundation vendor ID. >> > > I see the point. However in get_phy_device() we have the following check > that should cause a PHY with ID 0xffff_ffff to be ignored. Therefore > I doubt there's any such PHY ID in use. > > /* If the phy_id is mostly Fs, there is no device there */ > if ((phy_id & 0x1fffffff) == 0x1fffffff) > return ERR_PTR(-ENODEV); Indeed, it looks like the phy_id reported through sysfs for fixed PHY is actually 0, so your change should be fine then, thanks!
diff --git a/drivers/net/phy/phy_device.c b/drivers/net/phy/phy_device.c index 163295dbc..54f80af31 100644 --- a/drivers/net/phy/phy_device.c +++ b/drivers/net/phy/phy_device.c @@ -2388,8 +2388,7 @@ void phy_drivers_unregister(struct phy_driver *drv, int n) EXPORT_SYMBOL(phy_drivers_unregister); static struct phy_driver genphy_driver = { - .phy_id = 0xffffffff, - .phy_id_mask = 0xffffffff, + PHY_ID_MATCH_EXACT(GENPHY_ID), .name = "Generic PHY", .soft_reset = genphy_no_soft_reset, .get_features = genphy_read_abilities, diff --git a/include/linux/phy.h b/include/linux/phy.h index 5ac7d2137..3b07bce78 100644 --- a/include/linux/phy.h +++ b/include/linux/phy.h @@ -37,6 +37,10 @@ #define PHY_1000BT_FEATURES (SUPPORTED_1000baseT_Half | \ SUPPORTED_1000baseT_Full) +#define GENPHY_ID_HIGH 0xffffU +#define GENPHY_ID_LOW 0xfffeU +#define GENPHY_ID ((GENPHY_ID_HIGH << 16) | GENPHY_ID_LOW) + extern __ETHTOOL_DECLARE_LINK_MODE_MASK(phy_basic_features) __ro_after_init; extern __ETHTOOL_DECLARE_LINK_MODE_MASK(phy_basic_t1_features) __ro_after_init; extern __ETHTOOL_DECLARE_LINK_MODE_MASK(phy_gbit_features) __ro_after_init;
In cases like a fixed phy that is never attached to a net_device we may want to bind the genphy driver at probe time. Setting a PHY ID of 0xffffffff to bind the genphy driver would fail due to a check in get_phy_device(). Therefore let's change the PHY ID the genphy driver binds to to 0xfffffffe. This still shouldn't match any real PHY, and it will pass the check in get_phy_devcie(). Signed-off-by: Heiner Kallweit <hkallweit1@gmail.com> --- drivers/net/phy/phy_device.c | 3 +-- include/linux/phy.h | 4 ++++ 2 files changed, 5 insertions(+), 2 deletions(-)