On 28.05.2019 15:15, Russell King - ARM Linux admin wrote:
> On Mon, May 27, 2019 at 08:29:45PM +0200, Heiner Kallweit wrote:
>> Especially with fibre links there may be very short link drops. And if
>> interrupt handling is slow we may miss such a link drop. To deal with
>> this we remove the double link status read from the generic link status
>> read functions, and call the state machine twice instead.
>> The flag for double-reading link status can be set by phy_mac_interrupt
>> from hard irq context, therefore we have to use an atomic operation.
>
> I came up with a different solution to this - I haven't extensively
> tested it yet though:
>
> drivers/net/phy/phy-c45.c | 12 ------------
> drivers/net/phy/phy.c | 29 +++++++++++++++++++----------
> drivers/net/phy/phy_device.c | 14 --------------
> 3 files changed, 19 insertions(+), 36 deletions(-)
>
> diff --git a/drivers/net/phy/phy-c45.c b/drivers/net/phy/phy-c45.c
> index 9e24d9569424..756d7711cbc5 100644
> --- a/drivers/net/phy/phy-c45.c
> +++ b/drivers/net/phy/phy-c45.c
> @@ -222,18 +222,6 @@ int genphy_c45_read_link(struct phy_device *phydev)
> devad = __ffs(mmd_mask);
> mmd_mask &= ~BIT(devad);
>
> - /* The link state is latched low so that momentary link
> - * drops can be detected. Do not double-read the status
> - * in polling mode to detect such short link drops.
> - */
> - if (!phy_polling_mode(phydev)) {
> - val = phy_read_mmd(phydev, devad, MDIO_STAT1);
> - if (val < 0)
> - return val;
> - else if (val & MDIO_STAT1_LSTATUS)
> - continue;
> - }
> -
> val = phy_read_mmd(phydev, devad, MDIO_STAT1);
> if (val < 0)
> return val;
> diff --git a/drivers/net/phy/phy.c b/drivers/net/phy/phy.c
> index 7b3c5eec0129..2e7f0428e8fa 100644
> --- a/drivers/net/phy/phy.c
> +++ b/drivers/net/phy/phy.c
> @@ -507,20 +507,29 @@ static int phy_config_aneg(struct phy_device *phydev)
> */
> static int phy_check_link_status(struct phy_device *phydev)
> {
> - int err;
> + int err, i;
>
> WARN_ON(!mutex_is_locked(&phydev->lock));
>
> - err = phy_read_status(phydev);
> - if (err)
> - return err;
> + /* The link state is latched low so that momentary link drops can
> + * be detected. If the link has failed, re-read the link status
> + * to ensure that we are up to date with the current link state,
> + * while notifying that the link status has changed.
> + */
> + for (i = 0; i < 2; i++) {
> + err = phy_read_status(phydev);
> + if (err)
> + return err;
>
> - if (phydev->link && phydev->state != PHY_RUNNING) {
> - phydev->state = PHY_RUNNING;
> - phy_link_up(phydev);
> - } else if (!phydev->link && phydev->state != PHY_NOLINK) {
> - phydev->state = PHY_NOLINK;
> - phy_link_down(phydev, true);
> + if (phydev->link && phydev->state != PHY_RUNNING) {
> + phydev->state = PHY_RUNNING;
> + phy_link_up(phydev);
> + } else if (!phydev->link && phydev->state != PHY_NOLINK) {
> + phydev->state = PHY_NOLINK;
> + phy_link_down(phydev, true);
> + }
> + if (phydev->link)
> + break;
One drawback of this approach may be that if we have an up-down-up
cycle then callback link_change_notify isn't called for either
transition. In most cases this shouldn't be an issue, but it's not
the expected behavior.
> }
>
> return 0;
> [...]
