| Message ID | c2616532-8c29-d798-a8c9-a564a807b5eb@gmail.com |
|---|---|
| State | Deferred, archived |
| Delegated to: | David Miller |
| Headers | show |
| Series | [1/2] net: phy: core: use genphy version of callbacks read_status and config_aneg per default | expand |
On 11/15/2017 01:42 PM, Heiner Kallweit wrote: > read_status and config_aneg are the only mandatory callbacks and most > of the time the generic implementation is used by drivers. > So make the core fall back to the generic version if a driver doesn't > implement the respective callback. > > Also currently the core doesn't seem to verify that drivers implement > the mandatory calls. If a driver doesn't do so we'd just get a NPE. Right, which is not an unusual way to signal that something is mandatory. > With this patch this potential issue doesn't exit any longer. > > Signed-off-by: Heiner Kallweit <hkallweit1@gmail.com> Reviewed-by: Florian Fainelli <f.fainelli@gmail.com> Note that net-next is closed at the moment, so you will have to resubmit this when the tree opens back again. > --- > drivers/net/phy/phy.c | 5 ++++- > include/linux/phy.h | 33 ++++++++++++++++++--------------- > 2 files changed, 22 insertions(+), 16 deletions(-) > > diff --git a/drivers/net/phy/phy.c b/drivers/net/phy/phy.c > index 2b1e67bc1..a0e7605dc 100644 > --- a/drivers/net/phy/phy.c > +++ b/drivers/net/phy/phy.c > @@ -493,7 +493,10 @@ static int phy_start_aneg_priv(struct phy_device *phydev, bool sync) > /* Invalidate LP advertising flags */ > phydev->lp_advertising = 0; > > - err = phydev->drv->config_aneg(phydev); > + if (phydev->drv->config_aneg) > + err = phydev->drv->config_aneg(phydev); > + else > + err = genphy_config_aneg(phydev); > if (err < 0) > goto out_unlock; > > diff --git a/include/linux/phy.h b/include/linux/phy.h > index dc82a07cb..958b5162a 100644 > --- a/include/linux/phy.h > +++ b/include/linux/phy.h > @@ -497,13 +497,13 @@ struct phy_device { > * flags: A bitfield defining certain other features this PHY > * supports (like interrupts) > * > - * The drivers must implement config_aneg and read_status. All > - * other functions are optional. Note that none of these > - * functions should be called from interrupt time. The goal is > - * for the bus read/write functions to be able to block when the > - * bus transaction is happening, and be freed up by an interrupt > - * (The MPC85xx has this ability, though it is not currently > - * supported in the driver). > + * All functions are optional. If config_aneg or read_status > + * are not implemented, the phy core uses the genphy versions. > + * Note that none of these functions should be called from > + * interrupt time. The goal is for the bus read/write functions > + * to be able to block when the bus transaction is happening, > + * and be freed up by an interrupt (The MPC85xx has this ability, > + * though it is not currently supported in the driver). > */ > struct phy_driver { > struct mdio_driver_common mdiodrv; > @@ -841,14 +841,6 @@ int phy_aneg_done(struct phy_device *phydev); > int phy_stop_interrupts(struct phy_device *phydev); > int phy_restart_aneg(struct phy_device *phydev); > > -static inline int phy_read_status(struct phy_device *phydev) > -{ > - if (!phydev->drv) > - return -EIO; > - > - return phydev->drv->read_status(phydev); > -} > - > #define phydev_err(_phydev, format, args...) \ > dev_err(&_phydev->mdio.dev, format, ##args) > > @@ -890,6 +882,17 @@ int genphy_c45_read_pma(struct phy_device *phydev); > int genphy_c45_pma_setup_forced(struct phy_device *phydev); > int genphy_c45_an_disable_aneg(struct phy_device *phydev); > > +static inline int phy_read_status(struct phy_device *phydev) > +{ > + if (!phydev->drv) > + return -EIO; > + > + if (phydev->drv->read_status) > + return phydev->drv->read_status(phydev); > + else > + return genphy_read_status(phydev); > +} > + > void phy_driver_unregister(struct phy_driver *drv); > void phy_drivers_unregister(struct phy_driver *drv, int n); > int phy_driver_register(struct phy_driver *new_driver, struct module *owner); >
Am 15.11.2017 um 22:56 schrieb Florian Fainelli: > On 11/15/2017 01:42 PM, Heiner Kallweit wrote: >> read_status and config_aneg are the only mandatory callbacks and most >> of the time the generic implementation is used by drivers. >> So make the core fall back to the generic version if a driver doesn't >> implement the respective callback. >> >> Also currently the core doesn't seem to verify that drivers implement >> the mandatory calls. If a driver doesn't do so we'd just get a NPE. > > Right, which is not an unusual way to signal that something is mandatory. > >> With this patch this potential issue doesn't exit any longer. >> >> Signed-off-by: Heiner Kallweit <hkallweit1@gmail.com> > > Reviewed-by: Florian Fainelli <f.fainelli@gmail.com> > > Note that net-next is closed at the moment, so you will have to resubmit > this when the tree opens back again. > I see that the two patches have status "deferred" in patchwork. So do I have to actually resubmit or are they going to be be picked up from patchwork? Rgds, Heiner >> --- >> drivers/net/phy/phy.c | 5 ++++- >> include/linux/phy.h | 33 ++++++++++++++++++--------------- >> 2 files changed, 22 insertions(+), 16 deletions(-) >> >> diff --git a/drivers/net/phy/phy.c b/drivers/net/phy/phy.c >> index 2b1e67bc1..a0e7605dc 100644 >> --- a/drivers/net/phy/phy.c >> +++ b/drivers/net/phy/phy.c >> @@ -493,7 +493,10 @@ static int phy_start_aneg_priv(struct phy_device *phydev, bool sync) >> /* Invalidate LP advertising flags */ >> phydev->lp_advertising = 0; >> >> - err = phydev->drv->config_aneg(phydev); >> + if (phydev->drv->config_aneg) >> + err = phydev->drv->config_aneg(phydev); >> + else >> + err = genphy_config_aneg(phydev); >> if (err < 0) >> goto out_unlock; >> >> diff --git a/include/linux/phy.h b/include/linux/phy.h >> index dc82a07cb..958b5162a 100644 >> --- a/include/linux/phy.h >> +++ b/include/linux/phy.h >> @@ -497,13 +497,13 @@ struct phy_device { >> * flags: A bitfield defining certain other features this PHY >> * supports (like interrupts) >> * >> - * The drivers must implement config_aneg and read_status. All >> - * other functions are optional. Note that none of these >> - * functions should be called from interrupt time. The goal is >> - * for the bus read/write functions to be able to block when the >> - * bus transaction is happening, and be freed up by an interrupt >> - * (The MPC85xx has this ability, though it is not currently >> - * supported in the driver). >> + * All functions are optional. If config_aneg or read_status >> + * are not implemented, the phy core uses the genphy versions. >> + * Note that none of these functions should be called from >> + * interrupt time. The goal is for the bus read/write functions >> + * to be able to block when the bus transaction is happening, >> + * and be freed up by an interrupt (The MPC85xx has this ability, >> + * though it is not currently supported in the driver). >> */ >> struct phy_driver { >> struct mdio_driver_common mdiodrv; >> @@ -841,14 +841,6 @@ int phy_aneg_done(struct phy_device *phydev); >> int phy_stop_interrupts(struct phy_device *phydev); >> int phy_restart_aneg(struct phy_device *phydev); >> >> -static inline int phy_read_status(struct phy_device *phydev) >> -{ >> - if (!phydev->drv) >> - return -EIO; >> - >> - return phydev->drv->read_status(phydev); >> -} >> - >> #define phydev_err(_phydev, format, args...) \ >> dev_err(&_phydev->mdio.dev, format, ##args) >> >> @@ -890,6 +882,17 @@ int genphy_c45_read_pma(struct phy_device *phydev); >> int genphy_c45_pma_setup_forced(struct phy_device *phydev); >> int genphy_c45_an_disable_aneg(struct phy_device *phydev); >> >> +static inline int phy_read_status(struct phy_device *phydev) >> +{ >> + if (!phydev->drv) >> + return -EIO; >> + >> + if (phydev->drv->read_status) >> + return phydev->drv->read_status(phydev); >> + else >> + return genphy_read_status(phydev); >> +} >> + >> void phy_driver_unregister(struct phy_driver *drv); >> void phy_drivers_unregister(struct phy_driver *drv, int n); >> int phy_driver_register(struct phy_driver *new_driver, struct module *owner); >> > >
From: Heiner Kallweit <hkallweit1@gmail.com> Date: Wed, 29 Nov 2017 21:47:16 +0100 > Am 15.11.2017 um 22:56 schrieb Florian Fainelli: >> On 11/15/2017 01:42 PM, Heiner Kallweit wrote: >>> read_status and config_aneg are the only mandatory callbacks and most >>> of the time the generic implementation is used by drivers. >>> So make the core fall back to the generic version if a driver doesn't >>> implement the respective callback. >>> >>> Also currently the core doesn't seem to verify that drivers implement >>> the mandatory calls. If a driver doesn't do so we'd just get a NPE. >> >> Right, which is not an unusual way to signal that something is mandatory. >> >>> With this patch this potential issue doesn't exit any longer. >>> >>> Signed-off-by: Heiner Kallweit <hkallweit1@gmail.com> >> >> Reviewed-by: Florian Fainelli <f.fainelli@gmail.com> >> >> Note that net-next is closed at the moment, so you will have to resubmit >> this when the tree opens back again. >> > I see that the two patches have status "deferred" in patchwork. > So do I have to actually resubmit or are they going to be be picked up > from patchwork? You must resubmit when the net-next tree opens back up.
diff --git a/drivers/net/phy/phy.c b/drivers/net/phy/phy.c index 2b1e67bc1..a0e7605dc 100644 --- a/drivers/net/phy/phy.c +++ b/drivers/net/phy/phy.c @@ -493,7 +493,10 @@ static int phy_start_aneg_priv(struct phy_device *phydev, bool sync) /* Invalidate LP advertising flags */ phydev->lp_advertising = 0; - err = phydev->drv->config_aneg(phydev); + if (phydev->drv->config_aneg) + err = phydev->drv->config_aneg(phydev); + else + err = genphy_config_aneg(phydev); if (err < 0) goto out_unlock; diff --git a/include/linux/phy.h b/include/linux/phy.h index dc82a07cb..958b5162a 100644 --- a/include/linux/phy.h +++ b/include/linux/phy.h @@ -497,13 +497,13 @@ struct phy_device { * flags: A bitfield defining certain other features this PHY * supports (like interrupts) * - * The drivers must implement config_aneg and read_status. All - * other functions are optional. Note that none of these - * functions should be called from interrupt time. The goal is - * for the bus read/write functions to be able to block when the - * bus transaction is happening, and be freed up by an interrupt - * (The MPC85xx has this ability, though it is not currently - * supported in the driver). + * All functions are optional. If config_aneg or read_status + * are not implemented, the phy core uses the genphy versions. + * Note that none of these functions should be called from + * interrupt time. The goal is for the bus read/write functions + * to be able to block when the bus transaction is happening, + * and be freed up by an interrupt (The MPC85xx has this ability, + * though it is not currently supported in the driver). */ struct phy_driver { struct mdio_driver_common mdiodrv; @@ -841,14 +841,6 @@ int phy_aneg_done(struct phy_device *phydev); int phy_stop_interrupts(struct phy_device *phydev); int phy_restart_aneg(struct phy_device *phydev); -static inline int phy_read_status(struct phy_device *phydev) -{ - if (!phydev->drv) - return -EIO; - - return phydev->drv->read_status(phydev); -} - #define phydev_err(_phydev, format, args...) \ dev_err(&_phydev->mdio.dev, format, ##args) @@ -890,6 +882,17 @@ int genphy_c45_read_pma(struct phy_device *phydev); int genphy_c45_pma_setup_forced(struct phy_device *phydev); int genphy_c45_an_disable_aneg(struct phy_device *phydev); +static inline int phy_read_status(struct phy_device *phydev) +{ + if (!phydev->drv) + return -EIO; + + if (phydev->drv->read_status) + return phydev->drv->read_status(phydev); + else + return genphy_read_status(phydev); +} + void phy_driver_unregister(struct phy_driver *drv); void phy_drivers_unregister(struct phy_driver *drv, int n); int phy_driver_register(struct phy_driver *new_driver, struct module *owner);
read_status and config_aneg are the only mandatory callbacks and most of the time the generic implementation is used by drivers. So make the core fall back to the generic version if a driver doesn't implement the respective callback. Also currently the core doesn't seem to verify that drivers implement the mandatory calls. If a driver doesn't do so we'd just get a NPE. With this patch this potential issue doesn't exit any longer. Signed-off-by: Heiner Kallweit <hkallweit1@gmail.com> --- drivers/net/phy/phy.c | 5 ++++- include/linux/phy.h | 33 ++++++++++++++++++--------------- 2 files changed, 22 insertions(+), 16 deletions(-)