| Message ID | 8480c38d6f3bc5c4f6c332a788bc82999266aab7.1551437561.git.joabreu@synopsys.com |
|---|---|
| State | Changes Requested |
| Delegated to: | David Miller |
| Headers | show |
| Series | Use C45 Helpers when possible | expand |
On Fri, Mar 01, 2019 at 11:54:24AM +0100, Jose Abreu wrote: > +static inline int phy_update_link(struct phy_device *phydev) > +{ > + if (!phydev->drv) > + return -EIO; > + > + if (phydev->drv->read_status) > + return phydev->drv->read_status(phydev); > + else if (phydev->is_c45) > + return gen10g_read_status(phydev); > + else > + return genphy_update_link(phydev); > +} Hi Jose The asymmetry here could be an issue. We might fall into the trap that a c45 PHY has the full state in phydev updated, were as a c22 only has the link updated. Somebody testing on C45 might miss a bug for a C22 device. Maybe this should be called phy_read_state(), and calls genphy_read_status() not genphy_update_link(). Andrew
Hi Andrew, On 3/1/2019 1:53 PM, Andrew Lunn wrote: > On Fri, Mar 01, 2019 at 11:54:24AM +0100, Jose Abreu wrote: >> +static inline int phy_update_link(struct phy_device *phydev) >> +{ >> + if (!phydev->drv) >> + return -EIO; >> + >> + if (phydev->drv->read_status) >> + return phydev->drv->read_status(phydev); >> + else if (phydev->is_c45) >> + return gen10g_read_status(phydev); >> + else >> + return genphy_update_link(phydev); >> +} > > Hi Jose > > The asymmetry here could be an issue. We might fall into the trap > that a c45 PHY has the full state in phydev updated, were as a c22 > only has the link updated. Somebody testing on C45 might miss a bug > for a C22 device. Notice that this phy_update_link() is called from PHY_FORCING state which in my case happens when autoneg is not enabled / is not supported. I think it makes sense, in this case, to only update link status, no ? Thanks, Jose Miguel Abreu > > Maybe this should be called phy_read_state(), and calls > genphy_read_status() not genphy_update_link(). > > Andrew >
On Mon, Mar 04, 2019 at 03:07:24PM +0000, Jose Abreu wrote: > Hi Andrew, > > On 3/1/2019 1:53 PM, Andrew Lunn wrote: > > On Fri, Mar 01, 2019 at 11:54:24AM +0100, Jose Abreu wrote: > >> +static inline int phy_update_link(struct phy_device *phydev) > >> +{ > >> + if (!phydev->drv) > >> + return -EIO; > >> + > >> + if (phydev->drv->read_status) > >> + return phydev->drv->read_status(phydev); > >> + else if (phydev->is_c45) > >> + return gen10g_read_status(phydev); > >> + else > >> + return genphy_update_link(phydev); > >> +} > > > > Hi Jose > > > > The asymmetry here could be an issue. We might fall into the trap > > that a c45 PHY has the full state in phydev updated, were as a c22 > > only has the link updated. Somebody testing on C45 might miss a bug > > for a C22 device. > > Notice that this phy_update_link() is called from PHY_FORCING > state which in my case happens when autoneg is not enabled / is > not supported. > > I think it makes sense, in this case, to only update link status, > no ? Hi Jose It is actually quite difficult to determine when the link is up. I personally would not trust gen10g_read_status() to get this right, and would always implement the read_status callback. Which PHY driver are you using, which does not support read_status(). All the mainline PHY drivers do seem to have read_status implemented. Andrew
On 04.03.2019 16:44, Andrew Lunn wrote: > On Mon, Mar 04, 2019 at 03:07:24PM +0000, Jose Abreu wrote: >> Hi Andrew, >> >> On 3/1/2019 1:53 PM, Andrew Lunn wrote: >>> On Fri, Mar 01, 2019 at 11:54:24AM +0100, Jose Abreu wrote: >>>> +static inline int phy_update_link(struct phy_device *phydev) >>>> +{ >>>> + if (!phydev->drv) >>>> + return -EIO; >>>> + >>>> + if (phydev->drv->read_status) >>>> + return phydev->drv->read_status(phydev); >>>> + else if (phydev->is_c45) >>>> + return gen10g_read_status(phydev); >>>> + else >>>> + return genphy_update_link(phydev); >>>> +} >>> >>> Hi Jose >>> >>> The asymmetry here could be an issue. We might fall into the trap >>> that a c45 PHY has the full state in phydev updated, were as a c22 >>> only has the link updated. Somebody testing on C45 might miss a bug >>> for a C22 device. >> >> Notice that this phy_update_link() is called from PHY_FORCING >> state which in my case happens when autoneg is not enabled / is >> not supported. >> >> I think it makes sense, in this case, to only update link status, >> no ? > > Hi Jose > > It is actually quite difficult to determine when the link is up. I > personally would not trust gen10g_read_status() to get this right, and > would always implement the read_status callback. > Not to forget that we just stopped exporting gen10g_read_status(). genphy_c45_read_link() seems to be the right function to be used in phy_update_link(). > Which PHY driver are you using, which does not support > read_status(). All the mainline PHY drivers do seem to have > read_status implemented. > > Andrew > Heiner
diff --git a/drivers/net/phy/phy.c b/drivers/net/phy/phy.c index c5675df5fc6f..1ef5dbc384d0 100644 --- a/drivers/net/phy/phy.c +++ b/drivers/net/phy/phy.c @@ -940,7 +940,7 @@ void phy_state_machine(struct work_struct *work) err = phy_check_link_status(phydev); break; case PHY_FORCING: - err = genphy_update_link(phydev); + err = phy_update_link(phydev); if (err) break; diff --git a/include/linux/phy.h b/include/linux/phy.h index 872899136fdc..67caa7eb93b1 100644 --- a/include/linux/phy.h +++ b/include/linux/phy.h @@ -1036,6 +1036,19 @@ static inline int phy_read_status(struct phy_device *phydev) return genphy_read_status(phydev); } +static inline int phy_update_link(struct phy_device *phydev) +{ + if (!phydev->drv) + return -EIO; + + if (phydev->drv->read_status) + return phydev->drv->read_status(phydev); + else if (phydev->is_c45) + return gen10g_read_status(phydev); + else + return genphy_update_link(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);
When using a C45 PHY that is in PHY_FORCING state we are currently not taking into account that this kind of PHY has different update_link functions. Use the C45 Helpers instead or the driver built-in read_status() helper, if possible. Signed-off-by: Jose Abreu <joabreu@synopsys.com> Cc: Andrew Lunn <andrew@lunn.ch> Cc: Florian Fainelli <f.fainelli@gmail.com> Cc: "David S. Miller" <davem@davemloft.net> Cc: Joao Pinto <joao.pinto@synopsys.com> --- drivers/net/phy/phy.c | 2 +- include/linux/phy.h | 13 +++++++++++++ 2 files changed, 14 insertions(+), 1 deletion(-)